Re: [PATCH V4 01/15] mmc: host: Add facility to support re-tuning

[Adrian Hunter <adrian.hunter@intel.com>]

On 01/04/15 12:50, Ulf Hansson wrote:
> On 27 March 2015 at 21:57, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Currently, there is core support for tuning during
>> initialization. There can also be a need to re-tune
>> periodically (e.g. sdhci) or to re-tune after the
>> host controller is powered off (e.g. after PM
>> runtime suspend / resume) or to re-tune in response
>> to CRC errors.
>>
>> The main requirements for re-tuning are:
> 
> It's a bit hard to follow why these requirements.

Yes they are driven by SDHCI requirements.

> 
> I am giving some comments below, also for my own reference. Please
> tell me if I am way off.
> 
>>   - ability to enable / disable re-tuning
> 
> Handled internally by the mmc core.

The host controller driver enables re-tuning based on whether the host
controller requires it for that transfer mode. For example, only the SDHCI
host controller knows if tuning is required for SDR50 mode according to the
SDHCI capability register bit 45.

> 
> Or maybe SDIO func drivers needs an API to control this?

No - it is for the host controller drivers.

> 
>>   - ability to flag that re-tuning is needed
> 
> Both from mmc core and mmc host point of view.

At the moment only the host controller driver flags re-tuning is needed.

> 
>>   - ability to re-tune before any request
> 
> Internal mechanism by the core.

Yes

> 
>>   - ability to hold off re-tuning if the card is busy
> 
> What do you mean by "card is busy"?

I guess, more accurately, any time the card is in a state that is incapable
of supporting re-tuning. For example:
	- DAT0 busy
	- between dependent commands like erase start, end, etc
	- card is asleep
Also SDIO cards can have a custom sleep state where tuning won't work.

> Is this requirement due to that the re-tune timer may complete at any
> point, which then flags that a re-tune is needed?

Primarily. But control is also needed when handling recovery. Or in the SDIO
custom sleep case.

There is also SDHCI re-tuning modes 2 and 3 (presently not supported) where
the host controller itself indicates that re-tuning is needed via the
present state and interrupt status registers.

> 
>>   - ability to hold off re-tuning if re-tuning is in
>>   progress
> 
> This I don't understand. How can a re-tune sequence be triggered when
> there is already a request ongoing towards the host. I mean the host
> is claimed during the re-tuning process, so why do we need one more
> layer of protection?

This is primarily for safety. We shouldn't have to think about what would
happen if the need_retune flag is set at an inopportune time.

> 
>>   - ability to run a re-tuning timer
> 
> As we discussed earlier, it's not obvious when it make sense to run this timer.
> 
> For the SDIO case, we should likely run it all the time, since we want
> to prevent I/O errors as long as possible.
> 
> For MMC/SD, I would rather see that we perform re-tune as a part of an
> "idle operation" and not from a timer (yes I know about the SDHCI
> spec, but it doesn't make sense). We can do this because we are able
> to re-cover from I/O errors (re-trying when getting CRC errors for
> example).

It makes sense to attempt to re-tune before CRC errors occur. If the
hardware vendor has gone to the trouble to determine when that might be,
then it makes sense to use that timeout. It is surely an approximation
(SDHCI only has values in powers-of-2 with units of seconds) but it seems
unreasonable to use a completely different value.

Doing it in the idle operation would not work because we would then runtime
suspend and just have to do it again after resuming.

> 
> I will continue to review the rest of series in more detail.

Thank you! :-)