On 17/07/18 10:08, Aapo Vienamo wrote:
> On Mon, 16 Jul 2018 21:03:08 +0100
> Jon Hunter <jonathanh@xxxxxxxxxx> wrote:
>
>> On 16/07/18 15:34, Aapo Vienamo wrote:
>>> Tegra SDHCI controllers require the SDHCI clock divider to be configured
>>> to divide the clock by two in DDR50/52 modes. Incorrectly configured
>>> clock divider results in corrupted data.
>>>
>>> Prevent the possibility of incorrectly calculating the divider value due
>>> to clock rate rounding or low parent clock frequency by not assigning
>>> host->max_clk to clk_get_rate() on tegra_sdhci_set_clock().
>>>
>>> See the comments for further details.
>>>
>>> Fixes: a8e326a ("mmc: tegra: implement module external clock change")
>>> Signed-off-by: Aapo Vienamo <avienamo@xxxxxxxxxx>
>>> ---
>>> drivers/mmc/host/sdhci-tegra.c | 17 ++++++++++++++++-
>>> 1 file changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
>>> index ddf00166..908b23e 100644
>>> --- a/drivers/mmc/host/sdhci-tegra.c
>>> +++ b/drivers/mmc/host/sdhci-tegra.c
>>> @@ -210,9 +210,24 @@ static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>>> if (!clock)
>>> return sdhci_set_clock(host, clock);
>>>
>>> + /*
>>> + * In DDR50/52 modes the Tegra SDHCI controllers require the SDHCI
>>> + * divider to be configured to divided the host clock by two. The SDHC
>>> + * clock divider is calculated as part of sdhci_set_clock() by
>>> + * sdhci_calc_clk(). The divider is calculated from host->max_clk and
>>> + * the requested clock rate.
>>> + *
>>> + * By setting the host->max_clk to clock * 2 the divider calculation
>>> + * will always result in the correct value for DDR50/52 modes,
>>> + * regardless of clock rate rounding, which may happen if the value
>>> + * from clk_get_rate() is used.
>>> + */
>>> host_clk = tegra_host->ddr_signaling ? clock * 2 : clock;
>>> clk_set_rate(pltfm_host->clk, host_clk);
>>> - host->max_clk = clk_get_rate(pltfm_host->clk);
>>> + if (tegra_host->ddr_signaling)
>>> + host->max_clk = host_clk;
>>> + else
>>> + host->max_clk = clk_get_rate(pltfm_host->clk);
>>>
>>> sdhci_set_clock(host, clock);
>>>
>>
>> I see what you are saying, but should we be concerned that we are not
>> getting the rate we are requesting in the first place?
>
> The rates themselves aren't as critical as the divider on DDR50/52. It's
> true that in most sane configurations we should not hit the cases where
> the divider will not get configured correctly if the value from
> clk_get_rate() is used.
>
>> Maybe it would help if you could provide a specific example showing a
>> case where we request rate X and get Y, and then this leads to a bad
>> rate Z later.
>
> There are two possible cases where the divider will get configured
> incorrectly: either to divide by one or anything greater than two. I
> verified that at least on t124 greater dividers also fail in practice.
So looking at a few Tegra TRMs, I see comments to the effect ...
'In DDR50 mode, SD clock divisor should always be 2'
... which aligns with the above statement and your comment.
> One option is that the parent clock is unable to supply a rate low
> enough. Lets consider DDR50 mode: we request 100 MHz from the parent
> clock and let's say it gets rounded to 104 MHz. In this case we end up
> with a divider of four because 104 MHz / 2 <= 50 MHz is false, and the
> divider is incremented to the next step. This happens because the
> divider is calculated the following manner in sdhci_calc_clk(), where in
> this case host->max_clk would be 104 MHz and clock 50 MHz:
>
> for (div = 2; div < SDHCI_MAX_DIV_SPEC_300;
> div += 2) {
> if ((host->max_clk / div) <= clock)
> break;
> }
>
> With the patch we would get DDR50 mode runinng at 52 MHz and without
> the patch all IO to the device would fail.
>
> The less likely option is that divider of one is calculated if
> host->max_clk is less than or equal to clock. This would happen if the
> parent clock is unable to supply a high enough clock rate. So let's say
> we are setting up the clocks for DDR50 and request the parent clock to
> supply 100 MHz and we get say 50 MHz instead. In this case originally we
> would get I/O errors, but with the patch we end up with at DDR50 mode
> where the bus is actually run at 25 MHz.
Yes I guess my concern is that we end up running at a rate much less than
expected. I am not sure if it is worth warning against this or not.
> While at least the later case would most likely be a bug or
> misconfiguration, it would be still nice to have a mechanism for
> graceful dergadation instead of complete failure.
Agree.
> Another option I considered was verifying the parent clock behaviour
> during probe and masking DDR50/52 out from the host capability bits
> depending on the results. The problem with that is that the exact rate
> requested is determined based on CSD register values read from the MMC
> device itself.
>
> It's really unfortunate that we have this quirk in the hardware as
> dealing with it in a robust manner results in a fair bit of complexity.
> Oh well.
Thanks for the explanation. Looks good to me ...
Acked-by: Jon Hunter <jonathanh@xxxxxxxxxx>
Cheers
Jon
--
nvpublic
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
