Page MenuHomeFreeBSD

Add sdio(4) peripheral device
Needs ReviewPublic

Authored by kibab on Sep 22 2017, 3:47 PM.
Tags
None
Referenced Files
F104113536: D12467.diff
Tue, Dec 3, 5:36 PM
Unknown Object (File)
Thu, Nov 21, 10:24 PM
Unknown Object (File)
Sat, Nov 16, 9:11 AM
Unknown Object (File)
Oct 31 2024, 8:33 PM
Unknown Object (File)
Oct 23 2024, 6:41 PM
Unknown Object (File)
Oct 14 2024, 12:38 AM
Unknown Object (File)
Sep 19 2024, 2:23 AM
Unknown Object (File)
Sep 18 2024, 5:53 AM

Details

Reviewers
imp
mav
scottl
Group Reviewers
cam
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 Passed
Unit
No Test Coverage
Build Status
Buildable 11696
Build 12042: arc lint + arc unit

Event Timeline

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 - subversion.
kibab edited the test plan for this revision. (Show Details)

Properly do attachment of child devices

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 ↗(On Diff #33848)

Parent? Are you talking SIM or something else?

sys/dev/brcmwl/brcmwl.c
55–57

No. You can remove this comment.

sys/dev/mmcnull/mmcnull.c
129 ↗(On Diff #33848)

sim_dev might be better here.

sys/dev/sdio/sdio.c
166

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.

294

format nit: { on next line.

342

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

359–363

You almost certainly don't need this.

366

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.

374–378

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

415

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

452

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

sys/dev/sdio/sdio_subr.c
80

formatting

120

formatting (lots of places are like this.

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?

@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.