Page MenuHomeFreeBSD

Fix setting RCA for MMC cards
ClosedPublic

Authored by kibab on Nov 12 2017, 7:56 PM.

Details

Summary

Unlike SD cards, that publish RCA in response to CMD3, MMC cards expect the host to set RCA itself.

Test Plan

This was found when testing on Intel NUC with the built-in eMMC chip. After applying this fix eMMC chip actually agrees to talk.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kibab created this revision.Nov 12 2017, 7:56 PM
imp added a comment.Nov 13 2017, 12:12 AM

A few nits. It seems reasonable, but I'm not sure what RCA == 2 means.

sys/cam/mmc/mmc_xpt.c
701 ↗(On Diff #35169)

why remove this?

717 ↗(On Diff #35169)

But insert this?

1021 ↗(On Diff #35169)

Don't need these {}

kibab updated this revision to Diff 35191.Nov 13 2017, 1:58 PM

Address review comments

kibab marked 3 inline comments as done.Nov 13 2017, 2:00 PM

RCA = 2 is just because I need to send _some_ RCA to the card. So I chose to send 2, could also choose any other number :-)

RCA = 2 is just because I need to send _some_ RCA to the card. So I chose to send 2, could also choose any other number :-)

Unlike SD prior to eSD, MMC is a bus which may have multiple children that in case of MMC require unique RCAs to be assigned. So while you may start with 2 for the first child, you need code which increments the RCA for additional children on the same bus.

marius requested changes to this revision.Nov 27 2017, 10:44 PM
This revision now requires changes to proceed.Nov 27 2017, 10:44 PM
kibab added a comment.Mar 10 2018, 2:29 PM

RCA = 2 is just because I need to send _some_ RCA to the card. So I chose to send 2, could also choose any other number :-)

Unlike SD prior to eSD, MMC is a bus which may have multiple children that in case of MMC require unique RCAs to be assigned. So while you may start with 2 for the first child, you need code which increments the RCA for additional children on the same bus.

AFAIK current MMC stack doesn't support multiple MMC cards on the same bus. Neither does the new stack support it. And I don't have any hardware to test if this works at all...

kibab requested review of this revision.Mar 10 2018, 2:31 PM

So, as I wrote in reply to the inline comment, supporting multiple MMC cards on the same bus is not something I would try to implement in the new stack, given that I have never seen any hardware with such setup and that the old stack AFAIK doesn't properly support this.
Apart from that, any comments?

Well, if you look at mmc_discover_cards(), you'll see that mmc(4) in fact will enumerate all MMC devices on a bus and assign different addresses to them:

while (1) {

<...>

ivar->rca = rca++;
err = mmc_set_relative_addr(sc, ivar->rca);

<...>

Likewise, mmc_calculate_clock() (somewhat a misnomer even before the addition of support for eMMC UHS-I modes) will arbitrate the fastest mode supported by all devices on the bus:

do {
        changed = false;
        for (i = 0; i < sc->child_count; i++) {

<...>

That said, there are a few (corner) cases with the bus support of mmc(4), like in mmc_go_discovery():

/* XXX recompute vdd based on new cards? */

I don't know how widespread MMC busses with multiple devices on them actually are. However, when ignoring the simple stuff found on maker boards and looking at e. g. industrial-grade SoCs, it's very common to make the maximum out of busses in order to reduce the pin count of the SoCs. For example, the NXP Layerscape series hangs all PHYs for the up to IIRC 3 Ethernet MACs off from a single MAC as far as the MDIO goes. Not that I think that the later is a good idea, but given that such weird arrangements do exist, it also wouldn't surprise me that there are contemporary designs with multiple MMC devices on a single bus, especially since true MMC controllers (as opposed SDHCI ones modified to support (e)MMC) apparently are still rather common for ARM SoCs.

In any case, I think support for busses is a must for MMCCAM due to eSD. Judging Linux code and commit messages I've read, designs with multiple eSDIO devices on a bus are real.

kibab added a comment.May 18 2018, 7:18 PM

In any case, I think support for busses is a must for MMCCAM due to eSD. Judging Linux code and commit messages I've read, designs with multiple eSDIO devices on a bus are real.

Thing is, even if I write some code for supporting it now, it will be impossible to test. I'd prefer to have something that works at least on those simple boards, and then if someone / some company needs multiple devices I / they can work on that. Unfortunately I don't have that much of spare time to hack on untestable stuff.
So, with that in mind, please give your opinion on this. Would adding a comment along the lines of "XXX Should be changed if need to implement multiple cards on the bus" work?

imp accepted this revision.Jun 19 2018, 3:35 PM

Add the comment and this will be sufficient for now.

sys/cam/mmc/mmc.h
95 ↗(On Diff #35191)

Add a comment here that says that we only currently support one mmc card/RCA and that the SIM should help us manage it since it knows how many children it has already probed, but that's future work.

kibab marked an inline comment as done.Jun 19 2018, 8:02 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 19 2018, 8:02 PM
Closed by commit rS335384: Fix setting RCA for MMC cards (authored by kibab). · Explain Why
This revision was automatically updated to reflect the committed changes.