Page MenuHomeFreeBSD

mmccam: Add two new XPT for MMC and use them in mmc_sim and sdhci

Authored by manu on Apr 29 2021, 3:53 PM.



For the discovery phase of SD/eMMC we need to do some transaction in a async
The classic CAM XPT_{GET,SET}_TRAN_SETTING cannot be used in a async way.
This also allow us to split the discovery phase into a more complete state
machine and we don't mtx_sleep with a random number to wait for completion
of the tasks.
For mmc_sim we now do the SET_TRAN_SETTING in a taskqueue so we can call
the needed function for regulators/clocks without the cam lock(s). This part is
still needed to be done for sdhci.
We also now save the host OCR in the discovery phase as it wasn't done before and
only worked because the same ccb was reused.

Test Plan

Tested on rockpro64 with dwmmc and sdhci (sd and eMMC).

Diff Detail

rG FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

manu requested review of this revision.Apr 29 2021, 3:53 PM
manu edited the test plan for this revision. (Show Details)

Generally this looks good. A nice cleanup. I'm a little uneasy about XPT_MMC_GET_TRAN_SETTINGS,
however. I'd like a little more time to review CAM before approving things. I wanted to get you this
much of the review, however, since it had been a few days.


As I comment below, what's to prevent this from firing multiple times? I'd suggest that you set mmc_sim->ccb = NULL before returning here.


I'd be inclined not to have a special MMC_GET_TRAN_SETTINGS, actually. You're mixing up a queued and unqueued command code, and queued commands usually don't complete in the call itself. If there's any done routine associated, then that may be called immediately... I'm uneasy about this, but would need to think and study CAM a bit more to provide a more complete reason why I'm so uneasy.


Here (and below in XPT_MMC_IO) you need to set ccb_h.status |= CAM_SIM_QUEUED rather than setting it to CAM_REQ_INPROG. INPROG is what the action routine is entered with already. The queued bit is used to flag this is in progress elsewhere (there are times for other SIMs where the status is set to something else | CAM_SIM_QUEUED.

Also, what prevents 2 of these from requests happening at about same time and mmc_sim->ccb getting confused?


Generally, I'd avoid this construct. It's not used elsewhere in CAM, but cleaning it up is likely beyond the scope of this review.


Yup, I'll fix that.


Ok didn't knew about QUEUED, will change.


What construct ?
Also I didn't change a thing in here but I'll do more cleanup in mmccam.

/* notreached */

can be simply stated as


Address some of @imp's comments


Yes I agree with only having the special MMC xpt but until mmc_da.c is converted this isn't possible.
Is there a CAM XPT type that can say "I might be queue but I might not ?"

manu marked 2 inline comments as done.May 13 2021, 2:45 PM

OK. So long as there's a done routine, and so long as the mmc_start routine can tolerate the mmc_done routine recursively calling into the mmc_start routine (which is what happens when you finish a queued tagged item w/o queueing it), then I think it's OK. In general, this is needed for the immediate error path, so it's likely OK already, but I thought I'd mention it.

In an ideal world, mmc would follow the other SIMs in just noting the settings for later I/O transactions. However, given the structure of the code and some oddities in the MMC bring up protocols, I understand this might be somewhere between really hard and impossible.

I believe that this is OK to commit as it is now, though the return; /* notreached */; break pattern should be fixed in a followup

This revision is now accepted and ready to land.May 13 2021, 8:58 PM
bz added a subscriber: bz.
bz added inline comments.

The others seem to have descriptive comments.