Page MenuHomeFreeBSD

Add sdio(4) peripheral device
Needs ReviewPublic

Authored by kibab on Sep 22 2017, 3:47 PM.

Details

Reviewers
imp
mav
scottl
Summary

This device will attach to SDIO cards and provide a bus-like interface to
its children. Also it will read CCCR / FBR registers of the card
and get vendor and product IDs so that child devices are able to
properly do probing.

sdio.ko is dynamically unloadable, but nobody has been unloading peripheral drivers before. So the code might not be quite stable.

Also add brcmwl driver as example of SDIO device driver.

Please see the comment in sdio.c about my concerns with regard to CAM <-> newbus. I really need some opinion on how to move this forward.

Test Plan
  • Boot MMCCAM-enabled kernel
  • kldload mmcnull if running in the VM, or have an SDIO card on the system
  • kldload sdio
  • kldload brcmwl

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 11973
Build 12299: arc lint + arc unit

Event Timeline

kibab created this revision.Sep 22 2017, 3:47 PM
kibab edited the summary of this revision. (Show Details)Sep 22 2017, 3:51 PM
kibab edited the test plan for this revision. (Show Details)
kibab added reviewers: imp, mav.
kibab set the repository for this revision to rS FreeBSD src repository.
kibab updated this revision to Diff 33830.Oct 8 2017, 10:35 PM

newbus attach musings

kibab updated this revision to Diff 33848.Oct 9 2017, 6:44 PM
kibab edited the test plan for this revision. (Show Details)

Properly do attachment of child devices

imp added a comment.Nov 28 2017, 10:19 PM

OK. I'm not sure I like this much.

I think that you shouldn't try to have the pseudo device that's detached.

For each SIM, you should store the device_t that the SIM belongs to. MMC in this case. Under MMC I'd have a sdiobus device. Attach that unconditionally. When you're through parsing the CIS and whatever other info you need for the device, add one child per device that you've found to the sdiobus device, then call probe and detach. When the SIM notices that the device goes away, have it detach everything and then either delete the children from the sdiobus, or give the sdiobus a callback to tell it that the card went away.

So, you have something like:
pciX ---- sdhci0 --- sdiobus -- mwlbcm

  • mwlbt

where sdhci0 is the SIM that implements the sdio card. If it finds two functions, bt and wireless, hypothetically. It isn't clear that's what you'd wind up with.

I'd also be tempted to do the 'add the sim dev_t field' as a separate commit. I could do that if you're nervous about that.

sys/cam/cam_sim.h
110

Parent? Are you talking SIM or something else?

sys/dev/brcmwl/brcmwl.c
54–56

No. You can remove this comment.

sys/dev/mmcnull/mmcnull.c
129

sim_dev might be better here.

sys/dev/sdio/sdio.c
165

I'll just make this comment once: All these printfs would be unacceptable in production code, but for now they are likely fine. I'd consider having a DEBUG macro like others use to differentiate between the printfs that are just there to trace what's going on for debugging from the ones that will be permanent. This isn't a blocking comment.

293

format nit: { on next line.

341

This release happens in both branches, why not have it once,

358–362

You almost certainly don't need this.

365

This feels wrong... The name is wrong, since it does probe/attach. You seem to parse the card data to know that the child is here before calling it, so it should really just be adding children devices. See also the two-level stuff I've talked about.

373–377

This nonsense can be avoided by being careful about adding children and doing probe / attach and completely avoiding identify.

414

This is the other reason is why want two layers. When the driver is loaded it automatically scans for devices.

451

This should not be deleting the device from the tree. Detach and delete are two separate operations.

sys/dev/sdio/sdio_subr.c
79

formatting

119

formatting (lots of places are like this.

kibab added a subscriber: scottl.

will D12467 be included in 12.X?

bz added a subscriber: bz.Mon, Nov 12, 3:04 PM
bz added a comment.Thu, Nov 15, 4:01 PM
In D12467#277167, @imp wrote:

@imp I've done the style cleanup in a local repo and was trying to wrap my head around the CAM XPT/"bus" bits.

So, you have something like:
pciX ---- sdhci0 --- sdiobus -- mwlbcm

  • mwlbt

Two questions:

(1) the link in your ASCII art between shdci0 and sdiobus that's not a newbus link but XPT? If it is supposed to be a newbus link or something else, why?

(2) similarly, do we need the "sdiobus" at all? Wouldn't the two (staying with your theoretical example wifi and bt) devices just show up as two targets on the same scbus<n>? Or are you suggesting that the sdiobus is simply there for drivers to probe and see, then "take over" from sdiobus which detaches and the driver then bypasses it at attach and directly talks to XPT?

Could you please explain your idea a bit more? Maybe re-draw the ascii art with a bit more detail?

bz added a comment.Sat, Nov 24, 3:52 PM

@imp could you please comment on your architectural views (or wish-list) while there is still time? Otherwise I might have to sort it out after the facts; I am expecting to be at a point when I need to make a driver talk to some SDIO in about a week and that means I'll work on any middle-glue-code I'll see fit.