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

[manu]

Summary

For the discovery phase of SD/eMMC we need to do some transaction in a async
way.
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).

[imp]

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.

sys/cam/mmc/mmc_sim.c

72

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.

111

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.

144

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?

sys/cam/mmc/mmc_xpt.c

798–799

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.

[manu]

sys/cam/mmc/mmc_sim.c

72

Yup, I'll fix that.

144

Ok didn't knew about QUEUED, will change.

sys/cam/mmc/mmc_xpt.c

798–799

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

[imp]

sys/cam/mmc/mmc_xpt.c

798–799

return;
/* notreached */
break;

can be simply stated as

return;

[manu]

Address some of @imp's comments

[manu]

sys/cam/mmc/mmc_sim.c

111

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 ?"

[imp]

sys/cam/mmc/mmc_sim.c

111

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.

[imp]

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

[bz]

sys/cam/cam_ccb.h

258

The others seem to have descriptive comments.