mmccam: Add mmc_sim, a generic sim for mmc driver to use

[manu]

Summary

This adds a generic sim that abstract a lot of what needs to be implemented
in a driver for mmccam support.
A new interface with three methods is added :

• mmc_sim_get_tran_settings: Use to get what the controller supports in term of capabilities, freq etc ...
• mmc_sim_set_tran_settings: Use to change the speed/freq/etc of the sdcard host controller
• mmc_sim_cam_request: Used for MMCIO requests

[imp]

sys/cam/mmc/mmc_sim.c

73

Is this synchronous w/o sleeping? Ditto below for the GET_TRAN_SETTINGS?

121

These two lines aren't needed.

[imp]

Generally, I like this idea. It's a bit more 'frameworky' than the rest of CAM, but I think that's a good thing since it will eliminate much code duplication.

You may be able to even do some of the mmc_sim_if stuff inside the sim if you share more state between it and the client sims. Only time will tell, though, if that's profitable or not.

sys/cam/mmc/mmc_sim_if.m

42

Re the comment above, I'd add a little more color to these calls.
The first two have to complete synchronously and likely never fail given their widespread use and in places w/o checking.
The third has to complete synchronously as well, but the completion code can be in progress.

Having these things documented would help in understanding for others converting things.

[manu]

sys/cam/mmc/mmc_sim.c

73

This one is okay, same for GET_TRAN_SETTINGS, problem is that we need SET_TRAN_SETTINGS to be async as we deal with clock and regulators and this make aquiring the global sx lock for clock/regulator.

sys/cam/mmc/mmc_sim_if.m

42

I plan to document before commiting, I just wanted to have a feedback of either "This is a good idea" or "This is stupid stop doing that" before writing man pages :)

[manu]

In D27485#614329, @imp wrote:

Generally, I like this idea. It's a bit more 'frameworky' than the rest of CAM, but I think that's a good thing since it will eliminate much code duplication.

It does eliminate a lot of dedup code and yes it's a bit more framworky than the rest of CAM but I think that most CAM drivers deal with standards while MMC controller are most of the time different.

You may be able to even do some of the mmc_sim_if stuff inside the sim if you share more state between it and the client sims. Only time will tell, though, if that's profitable or not.

[imp]

sys/cam/mmc/mmc_sim.c

73

In SCSI land, where CAM was born, the SIMs seem to just set internal tables/state rather than do anything more complicated than a few outl() or memory write to the controller... I'm not sure how this mismatch will play out... There's also a convention where if both the scsi settings and the spi settings are 0, it's just a probe to see if something it there... Not sure where that convention is implemented, or if we have to worry about it for MMC.... I do understand the quandary, though, because I know it can be hard to change actual modes in MMC/SD land. We may need a different CCB to actually do the change... I need to study more.

[kibab]

Nice! Seems it may solve the problem like we have in dwmmc where some generic methods get overridden with device-specific actions.

[scottl]

sys/cam/mmc/mmc_sim.c

73

This description isn't really true, unfortunately. Changing the negotiation parameters in parallel SCSI is an asynchronous operation too. Yes, the SIM is responsible for delivering the change request to the controller, but you typically don't know the results of the request until a new CDB is issued and completed. Instead of having the driver invent a synchronous mechanism to set the property and then check the results, CAM was designed around SET_TRANS_SETTINGS being a fire-and-forget mechanism. The action doesn't need to be completed, or even successful, for the CCB to complete (and in parallel, SCSI, it often is not complete and not successful). Instead, the results can be determined later. In fact, the XPT doesn't care much about the results; transport speed is just a user-visible parameter, nothing more. The driver is responsible for returning values in GET_TRANS_SETTINGS, and it's perfectly fine for it to return old value while the new settings are still being processed.

Please don't use an sx lock for this. I'll veto the review if you do. There's no reason that I can think of to use a sleepable lock in any I/O driver, and SX locks in FreeBSD are becoming anachronistic and should be avoided anyways. If you think that you need to use an sx lock, please contact me directly and we can review the architectural decisions that are driving that request.

[manu]

sys/cam/mmc/mmc_sim.c

73

SX lock aren't used in mmc drivers but in the clock and regulator framework.

How does one wait for SET_TRANS_SETTINGS to be done ?
Right now the mmccam code does xpt_action and then mtx_sleep on the periph mutex.
Is there a xpt_wait or something ?

For GET_TRANS_SETTINGS it's ok right now from what I could see.

[scottl]

sys/cam/mmc/mmc_sim.c

73

The key is to not wait for SET_TRANS_SETTINGS to be done. One idea would be to schedule the work into a taskqueue, and return immediately. You return an error in the CCB if the parameters are incorrect or if it's not possible to schedule the taskq, otherwise just return CAM_REQ_CMP. When your taskqueue runs and completes, you can mark some internal state so that your driver knows, but that's not important to the higher level of CAM. Also, don't synchronizing GET_TRANS_SETTINGS on this taskqueue (or anything else), it's fine to return stale information in it.

[manu]

sys/cam/mmc/mmc_sim.c

73

The problem isn't the driver side of thing, the problem is that a lot of stuff in mmc_xpt.c is done synchronously as it's needed by the mmc protocol (discovery phase for example).
So if nothing in cam exists to wait for an xpt action we need to totally rethink the whole discovery phase (maybe other part).