Add a CAM-Newbus SDIO support module.

[bz]

Summary

Add a CAM-Newbus SDIO support module.

This works provides a newbus infrastructure for device drivers wanting
to use SDIO. On the lower end while it is connected by newbus to SDHCI,
it talks CAM using the MMCCAM framework to get to it.

This also extendes the usbdevs framework to equally create sdiodev
header files with #defines for "vendors" and "products".

Submitted by: kibab (initial work, see D12467)
Sponsored by: The FreeBSD Foundation
MFC After: 1 month

[bz]

Just adding three comments to point out the not-so-obvious obvious apart from the fact that
(a) unloading is not yet supported.
(b) a man page is missing.

sys/dev/sdio/sdio_subr.h

58 ↗

(On Diff #55590)

This is one of the two parts where this differs from other implementations that I want to point out.

While on the hardware side with a max of 8 functions including F0 they saved a bit in a register, the software using an uint8_t has no such constraints; in addition we are used to count the element[0] of an array as 1 element and not as 0 elements.

I am willing to change it to equal to hardware and to be similar to other implementations if people feel like it.

sys/dev/sdio/sdiob.c

300 ↗

(On Diff #55590)

@kibab has D19674 open for lower layer (SDHCI) block mode support and will handle this.

We currently do not (yet) support block_count == 0 (endless operation which needs to be cancelled).

Also on my only test platform SDIO block mode writes lead to CRC Data Errors; I found references to this from other people but no solution yet. So for the moment we only automatically switch to block mode if this is a read. It's something I am aware off and there will be follow-ups as bit by bit gets sorted and tested.

558 ↗

(On Diff #55590)

This is the second place I want to point out we are currently differing from other implementations.

The comment should describe the problem sufficiently I hope. Again, I am willing to change that if people feel like. Personally I'd prefer to wait until we'll have a second or third SDIO consumer and possibly also have to handle a split-personality card and understand how that's done.

[imp]

I'd be happier if we copied usbdevs.awk to sdiodevs.awk.

sys/tools/usbdevs2h.awk

39 ↗

(On Diff #55590)

I think it's unwise to pollute the usb stack with sdio things that are only mildly related.
We've generally not done this when we've added new busses.

[imp]

I'd be happier if we copied usbdevs.awk to sdiodevs.awk.
I'll take a closer look next week when I'm back in the office.

sys/dev/sdio/sdio_subr.c

27 ↗

(On Diff #55590)

I suspect we need the SDIO association disclaimer in all these files as well.

[bz]

@imp if you can confirm that the two changes (usdbdevs.awk -> sdiodevs.awk) and the disclaimers should be added I'll be happy to go ahead.

sys/dev/sdio/sdio_subr.c

27 ↗

(On Diff #55590)

Do you think sdhci.c would also need it?
I see no harm adding them here. Shall I go ahead and do that?

At the moment this change won't compile until @kibab has completed and committed his blk mode review so making changes is easy :-)

sys/tools/usbdevs2h.awk

39 ↗

(On Diff #55590)

I am happy with more code duplication if it makes people sleep well. Should I?

[imp]

I'd be happier if we copied usbdevs.awk to sdiodevs.awk.
I'll take a closer look next week when I'm back in the office.

sys/dev/sdio/sdio_if.m

43 ↗

(On Diff #55590)

Note that this is the standard default implementation when none is provided... You could save yourself some code by just remove these...

sys/dev/sdio/sdio_subr.c

27 ↗

(On Diff #55590)

It's not urgent, but we should check on it...

sys/dev/sdio/sdiob.c

802 ↗

(On Diff #55590)

A #define for these (like SDIO_BAD_READ maybe?) wouldn't hurt.

sys/tools/usbdevs2h.awk

39 ↗

(On Diff #55590)

Please. the trouble is when you find you need to do something new, or odd or whatever for bus X that you share code with bus Y. It becomes harder. It might be good to make it so we just have different wrappers around the same core functionality (with pccard on its way out, much of the known weirdo cases will be going).

[kibab]

Generally LGTM with a few nits.

sys/dev/sdio/sdio_subr.c

94 ↗

(On Diff #55590)

I'm unsure if this matters, but Linux uses 2x CMD52 to write to this register. Which seems OK because you are effectively changing CMD53 parameters here, so doing it with something different than CMD53 seems logical. The spec doesn't forbid using CMD53 for writing to this register, though.

sys/dev/sdio/sdio_subr.h

60 ↗

(On Diff #55590)

Can you please rename it to "support_multiblock" or something like that? I understand that SMB is the name of the corresponding register in CCCR, but having such names doesn't contribute much to the code readability, sadly.

sys/dev/sdio/sdiob.c

300 ↗

(On Diff #55590)

I'd prefer this implementation to only do what has been asked -- transfer N bytes in X blocks of size Y, and not decide by ourselves if we want block mode or not.

Again, Linux has the same approach -- there is lower-level function that only does whatever was requested and then there is a wrapper that can break a given transfer into a number of blocks + a number of CMD52 transfers.

568 ↗

(On Diff #55590)

s/to/too/

572 ↗

(On Diff #55590)

Indeed Marvell SD8xxx chipsets have several functions each acting as a totall separate device, f1 being a WLAN and f2 being BT and f3 being FM radio. We can ignore this for now, though, given that the current range of devboards is equipped with Broadcom.

724 ↗

(On Diff #55590)

Maybe convert them to dprintf()'s ? So that it's still possible to debug but normally it's switched off.

920 ↗

(On Diff #55590)

Missed space :-)

[bz]

I've fixed all requests in my internal tree but the CMD53 request (which I left a comment for what I'll do).
I'll upload a new diff the next 48 hours (need to also adjust the driver to deal with multiple devices).

On a good note, it seems that blk write mode works now (I did not dissect if it was one of @kibab's changes or any of the changes from here) or anything else which happened in the last 4 weeks.

sys/dev/sdio/sdiob.c

300 ↗

(On Diff #55590)

@kibab You made me look an it seems there are 4 public KPIs there for r/w and inc/no inc, which are 1 line wrappers to the helper function, which indeed is a wrapper around the actual CMD53 function. The KPI has no idea about what a "Block" is, the "helper function" will decide that by itself in doing blk mode first if supported and byte more for the remainders. So basically what they have is an extra function split up in between there with an extra call, which makes the code a lot more read-able I admit but does not change the functionality to what I had here it seems (though I don't do blk mode if size is <= 4 bytes and do a byte mode instead).

I'll split things up internally into two functions as well to make the code more readable, and compared to linux the internal CMD53 function won't be exported and I will add assertions that we cannot end up with a last partial block then (as the only caller should catch this); I'll also add a comment that no-one should ever call it directly; just to avoid any confusion.

[bz]

Handle reviewer comments:

• add SDIO disclaimer
• fix typos
• split up CMD53 function
• add one device per function (and with that re-adjust to use the parent device as we no longer cache that, add ivar support rather than having hand-rolled accessor functions)
• Use CMD52 to set blocksize
• use a sdio specific awk to generate device and vendor defines
• remove the DEFAULT implementations from the sdio_if.m file as they were only providing the general default
• rework some error handling
• rename field "smb" into more descriptive
• Use dprintfs instead of printfs in the middle of a parsing API

• Also re-worked some comments, etc.

[kibab]

LGTM with a couple of tiny comments. Thanks!

sys/dev/sdio/sdiob.c

391 ↗

(On Diff #57179)

s/to/too/g

561 ↗

(On Diff #57179)

So IO suppose your first driver for Broadcom chipset will show how you implement a device that has to use all functions on the card?

[bz]

sys/dev/sdio/sdiob.c

561 ↗

(On Diff #57179)

Yo-Nope, that's only going to take 2/3 on the modern ones; I think on the old ones it might be the case.

[bz]

Warner if you have any further comments .. I'd like to commit this the next days. If not it'll go in by Sat 8 June.

[bz]

head/sys/conf/files

3012

WOW, these extra + were even visible here... Fixed after-commit in r348807.