Page MenuHomeFreeBSD

Discover cards on boot
Needs ReviewPublic

Authored by kibab on Jun 21 2018, 6:05 PM.

Details

Reviewers
manu
imp
Summary

Card insertion / removal detection is not implemented on AllWinner yet.
So just initiate bus scan unconditionally after succesfully attaching
to the controller.

This makes it possible to use inserted SD/MMC card as root file system.

Also refactor force-rescan code into a function and call it both from AllWinner SDHC controller and all SDHCI-based implementations.

Test Plan

Using this patch, I get after rebooting and without running camcontrol rescan:

On Nanopi M1+ (armv7, AllWinner):

root@nanopi:~ # camcontrol devlist
<SDHC SD16G 3.0 SN 7CA5C349 MFG 03/2016 >  at scbus0 target 0 lun 0 (pass0,sdda0)
<SDIO card>                        at scbus1 target 0 lun 0 (pass1,sdiob0)
<MMCHC 8WPD3R 0.0 SN 53CFF01B MFG 10/200>  at scbus2 target 0 lun 0 (pass2,sdda1)

On Pine64 (arm64, AllWinner):

root@r64:~ # camcontrol devlist
<SDHC SC32G 8.0 SN 1B240699 MFG 05/2018 >  at scbus0 target 0 lun 0 (pass0,sdda0)
<MMCHC M8B16G 2.8 SN 4E176927 MFG 07/200>  at scbus1 target 0 lun 0 (pass1,sdda1)

On Raspberry Pi 3 (arm64. SDHCI + SDHOST controllers):

root@rpi3:~ # camcontrol devlist
<SDHC SL16G 8.0 SN 19853490 MFG 01/2017 >  at scbus0 target 0 lun 0 (pass0,sdda0)
<SDIO card>                        at scbus1 target 0 lun 0 (pass1,sdiob0)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 27833

Event Timeline

kibab created this revision.Jun 21 2018, 6:05 PM
kevans added a subscriber: kevans.Jun 21 2018, 6:17 PM

Tested on: Lamobo R1 w/ root-on-SD

imp added inline comments.Jul 18 2018, 10:01 PM
sys/arm/allwinner/aw_mmc.c
428

I can't see what this protects in this code. Do you need it?

Hmm... any chance we can try to squeeze this in before the freeze?

manu added a comment.Aug 20 2018, 6:52 PM

Quoting myself :

20:51 < manu> I'm not sure this is the right solution
20:52 < manu
> it seems that it's cam only code
20:52 < manu__> so I don't understand why it's not in a generic function

bz added a subscriber: bz.Nov 12 2018, 3:13 PM
kibab marked an inline comment as done.Mar 19 2019, 10:23 PM
In D15955#357900, @manu wrote:

Quoting myself :
20:51 < manu> I'm not sure this is the right solution
20:52 < manu
> it seems that it's cam only code
20:52 < manu__> so I don't understand why it's not in a generic function

The correct solution is implementing proper card insertion detection. This is, however, out of scope for this change. I'm just replicating the way it's implemented for mmc(4) in this driver, for now.

sys/arm/allwinner/aw_mmc.c
428

Seems indeed this is excessive.

kibab marked an inline comment as done.Mar 19 2019, 10:23 PM
kibab updated this revision to Diff 55255.Mar 19 2019, 10:36 PM
kibab updated this revision to Diff 55260.Mar 19 2019, 11:16 PM
  • Fix spacing
manu added a comment.Mar 21 2019, 2:03 AM
In D15955#357900, @manu wrote:

Quoting myself :
20:51 < manu> I'm not sure this is the right solution
20:52 < manu
> it seems that it's cam only code
20:52 < manu__> so I don't understand why it's not in a generic function

The correct solution is implementing proper card insertion detection. This is, however, out of scope for this change. I'm just replicating the way it's implemented for mmc(4) in this driver, for now.

Yes and that's what I don't like, this is not the correct solution for this problem.

bz added a comment.Mar 21 2019, 9:00 AM
In D15955#420988, @manu wrote:
In D15955#357900, @manu wrote:

Quoting myself :

The correct solution is implementing proper card insertion detection. This is, however, out of scope for this change. I'm just replicating the way it's implemented for mmc(4) in this driver, for now.

Yes and that's what I don't like, this is not the correct solution for this problem.

Ok, stupid me, has two questions:

(a) what would be proper card insertion detection here for this one? (I am battling a similar issue elsewhere currently)

(b) if the API gets changed, so it works without the aw_mmc_softc, the function gets put into a central place outside this driver, and then gets called, is that what you are asking @manu ?

kibab added a comment.Mar 21 2019, 9:53 AM
In D15955#421067, @bz wrote:
In D15955#420988, @manu wrote:
In D15955#357900, @manu wrote:

Quoting myself :

The correct solution is implementing proper card insertion detection. This is, however, out of scope for this change. I'm just replicating the way it's implemented for mmc(4) in this driver, for now.

Yes and that's what I don't like, this is not the correct solution for this problem.

Ok, stupid me, has two questions:
(a) what would be proper card insertion detection here for this one? (I am battling a similar issue elsewhere currently)
(b) if the API gets changed, so it works without the aw_mmc_softc, the function gets put into a central place outside this driver, and then gets called, is that what you are asking @manu ?

The correct solution would be to start a separate task that checks the status of "Card present" register and initiates a rescan based on that.

@manu : you are right, I will withdraw this change and try to implement a proper solution.

kibab removed reviewers: imp, manu.Mar 21 2019, 9:54 AM
kibab updated this revision to Diff 64912.Tue, Nov 26, 9:09 PM
kibab edited the summary of this revision. (Show Details)
kibab set the repository for this revision to rS FreeBSD src repository.
kibab edited the test plan for this revision. (Show Details)Tue, Nov 26, 9:21 PM
manu accepted this revision.Tue, Nov 26, 9:39 PM
This revision is now accepted and ready to land.Tue, Nov 26, 9:39 PM
imp accepted this revision.Tue, Nov 26, 10:04 PM

Other than printing a warning here to help debugging should the alloc fail, I think this is good.

sys/cam/mmc/mmc_xpt.c
415

I'd print a warning here... why was no wait used though?

kibab updated this revision to Diff 65006.Thu, Nov 28, 3:45 PM

Updated to output an error message if various allocation functions failed. Also, revert to using alloc_ccb() from alloc_ccb_nowait() since it's pretty fine to wait in the contexts where start_discovery() is executed.

This revision now requires review to proceed.Thu, Nov 28, 3:45 PM
kibab marked an inline comment as done.Thu, Nov 28, 3:47 PM
kibab added inline comments.
sys/cam/mmc/mmc_xpt.c
415

Indeed _nowait() is excessive, it's fine to wait.

imp added inline comments.Fri, Nov 29, 6:48 AM
sys/cam/mmc/mmc_xpt.c
417

With the waiting version, you don't get null returns. .