Re: [PATCH] mmc: sdhci-pltfm: Add a common clk API based implementation of get_timeout_clock

[Stephen Warren <swarren@wwwdotorg.org>]

On 01/29/2013 02:22 AM, Lars-Peter Clausen wrote:
> On 01/29/2013 06:45 AM, Stephen Warren wrote:
>> On 01/28/2013 11:27 AM, Lars-Peter Clausen wrote:
>>> Quite a few drivers have a implementation of the get_timeout_clock callback
>>> which simply returns the result of clk_get_rate on devices clock. This patch
>>> adds a common implementation of this to the sdhci-pltfm module and replaces all
>>> custom implementations with the common one.
>>>
>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>> ---
>>> I've only runtime tested this patch on a platform which is not yet upstream. For
>>> the drivers which are modified in this patch I've only done compile time
>>> testing. But I think all changes, but maybe the bcm2835 one, are straight
>>> forward.
>>
>> It seems to work fine for bcm2835. So,
>>
>> Tested-by: Stephen Warren <swarren@wwwdotorg.org>
>>
>>> @@ -148,9 +131,9 @@ static struct sdhci_ops bcm2835_sdhci_ops = {
>>>  	.read_l = bcm2835_sdhci_readl,
>>>  	.read_w = bcm2835_sdhci_readw,
>>>  	.read_b = bcm2835_sdhci_readb,
>>> -	.get_max_clock = bcm2835_sdhci_get_max_clock,
>>> +	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
>>>  	.get_min_clock = bcm2835_sdhci_get_min_clock,
>>> -	.get_timeout_clock = bcm2835_sdhci_get_timeout_clock,
>>> +	.get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
>>>  };
>>
>> Rather than requiring .get_max_clock and .get_timeout_clock to be set by
>> each driver, perhaps the SDHCI core can call
>> sdhci_pltfm_clk_get_max_clock() if the function pointer is NULL?
> 
> Yea, this part of the bcm2835 driver confused me a bit. So there is the
> SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK quirk which causes the sdhci core to use
> the max clock as a basis to calculate the timeout clock.

It's quite possible that sdhci-bcm2835.c should simply be modified to
set that quirk, and the implementation of .get_timeout_clock() removed
from sdhci-bcm2835.c. I see that a couple of downstream drivers for this
HW do in fact set that quirk, so it should be safe. I made this change
and it seems to work fine; I'll send the patch.

> But it divides it by 1000.

I wonder if that's just the units the clock is stored in; I see various
"* 1000" in the usage of host->timeout_clk. Unfortunately, there don't
appear to be any other implementations of .get_timeout_clock() to
compare with.

> I don't know the bcm2835 sdhci controller hardware, but is it
> possible that the current timeout clock value is too large by a factor of
> 1000? This wouldn't cause problems with normal transfers, but may increase
> the timeout delay for failed transfers.

Quite possibly.