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.
Details
Details
Tested on rockpro64 with dwmmc and sdhci (sd and eMMC).
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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. | |
| sys/cam/mmc/mmc_xpt.c | ||
|---|---|---|
| 798–799 | return; /* notreached */ break; can be simply stated as return; | |
| 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. | |
| 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. | |
Comment Actions
I believe that this is OK to commit as it is now, though the return; /* notreached */; break pattern should be fixed in a followup
| sys/cam/cam_ccb.h | ||
|---|---|---|
| 258 | The others seem to have descriptive comments. | |