Page MenuHomeFreeBSD

MMC stack on top of CAM framework
AbandonedPublic

Authored by kibab on Jan 3 2016, 9:39 AM.

Details

Reviewers
imp
mav
adrian
ian
Summary

An attempt to create a new version of MMC stack that uses a CAM framework to route the requests and replies.
This will allow us to handle SDIO cards that generate interrupts when the stack doesn't expect it, and further unify FreeBSD device model.

Runs on the BeagleBone Black and uses a modified version of ti_sdhci driver to communicate with the card. Changes to the driver are minimal and can be ported to another SDHCI implementation easily.

  • The card is initialized only if it is inserted in the MMC slot on boot.
  • SD[HC] and MMC cards are supported in read-write mode.
  • There are some sleeps in the initialization path that should better be moved elsewhere to avoid blocking main CAM thread.
  • SDIO cards are detected, minimal initialization is done, at least N devices are created where N is the number of functions on the card.
Test Plan

Build armv6 kernel , kernel config file is BEAGLEBONE-CAM
Boot the BBB with the SDHC card inserted

You should see FREAKING LOTS OF DEBUG OUTPUT -- sorry for that :-) After boot you should be able to access two MMC/SD cards on the system, one is built-in eMMC and another one is inserted SD.

Try playing around: diskinfo -v sdda{0,1}, geom disk list, camcontrol devlist.

Unfortunately camcontrol devlist doesn't display useful data about the cards right now, this requires modifying camcontrol userspace utility, this is WIP.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

kibab updated this revision to Diff 11869.Jan 3 2016, 9:39 AM
kibab retitled this revision from to MMC stack on top of CAM framework.
kibab updated this object.
kibab edited the test plan for this revision. (Show Details)
kibab added reviewers: imp, ian, adrian, mav.
kibab set the repository for this revision to rS FreeBSD src repository.
kibab added a project: ARM.
imp edited edge metadata.Jan 4 2016, 6:49 AM

here's a few trivial comments to get my feet wet.

etc/mtree/BSD.include.dist
228–230

This change is unrelated and should be committed separately.

include/Makefile
43

this should be in alphabetical order.

lib/libcam/Makefile
38–39

This should be in alphabetical order, even though the old version isn't.

sys/Makefile
59 ↗(On Diff #11869)

this is unrelated.

kibab updated this revision to Diff 12011.Jan 7 2016, 5:18 PM
kibab edited edge metadata.
kibab removed rS FreeBSD src repository as the repository for this revision.

I've fixed the scanning and attaching of new devices. Additionally pass(4) attaches to all found devices now.

emaste added a subscriber: emaste.Feb 12 2016, 9:52 PM

for the next upload can you include context please -- e.g.

git diff -U999999 other-branch
svn diff --diff-cmd=diff -x -U999999

(or less than full context if you want, e.g. -U1000)

etc/mtree/BSD.include.dist
95–96

keep sorted

sys/arm/conf/BEAGLEBONE-CAM
2 ↗(On Diff #12011)

probably don't want to keep this copy of the header comment -- but perhaps the intent is to drop this later?

sys/cam/mmc/mmc.h
4–5

This comment ought to go after the copyright and license block, I think.

imp added a comment.Feb 16 2016, 5:57 AM

First off, this is looking quite good. I had a number of nit-picky and some
substantive changes (the sleeps gotta go). Some code organization
stuff too. I like it. It also might be easier to review in smaller chunks
too, which would make for better commits, but we can sort that out
iteratively. It's nice to have, but isn't needed for progress.

sys/arm/conf/BEAGLEBONE-CAM
1 ↗(On Diff #11869)

Not sure this file is useful to commit.

sys/cam/mmc/mmc_xpt.c
584–585

This isn't allowed in the start routine. You'll need more states to manage this transition.

590

Though no sleeps here, chances are you need a new state for this and clock through it in the done routine.

602–603

ditto

610–611

Bad sleep, see above :)

618–619

Bad sleep, see above :)

744–745

I'm not sure why you freeze the queue here. That doesn't make sense to me.

1076

I'd add a comment that the queue was frozen in the probe start routine. Tracking this down was a pita in other drivers / xpts

1102

This is a very odd style.

Also, this function belongs elsewhere, like cam_xpt.c

sys/cam/mmc/mmcreg.h
58

any reason not to use the one in dev? why move it here?

sys/conf/files
1560–1563

This looks like format tweaks. Should be done in another commit.

1966

This, and all the related files, look like a good candidate for a separate commit

sys/dev/sdhci/sdhci.c
63

This shouldn't be there...

526

This is gratuitous.

741–744

likely should be inside debug

793

stylistically, this should be at the top of this function, or at thetop of this block, not right here.

987–991

debug?

1555

I'd put this in a separate file.

kibab added a comment.Feb 16 2016, 2:51 PM

Thanks for the review. I will fix all your comments over the next few days and upload a new version.

sys/cam/mmc/mmc_xpt.c
1102

:-) Yes, it was auto-generated and there was an extra semicolon somewhere.

sys/cam/mmc/mmcreg.h
58

Yes: the one if dev/ depends on "mmc", and can be excluded from the build, whereas MMC CAM stuff cannot be excluded because it's CAM and it's not modular. Additionally, in the future it makes more sense to keep all related headers in sys/cam/mmc.

sys/dev/sdhci/sdhci.c
63

Agreed -- this is there because I didn't use ubdr to boot the kernel on BBB, so I couldn't really tune this. Now my main dev platform is Wandboard and it should be possible to set sdhci_debug via loader hints.

cem added a subscriber: cem.Feb 17 2016, 4:45 AM
kibab updated this revision to Diff 14398.Mar 17 2016, 1:45 PM
kibab set the repository for this revision to rS FreeBSD src repository.
kibab marked 4 inline comments as done.

I have started to cleanup my diff. This upload removes excessive kernel config files and fixes issues with alphabetical ordering in several places. So, no functional changes at all.

kibab abandoned this revision.Aug 5 2017, 8:16 AM

This was merged with some modifications.