[PATCH] mmc: sdhci-of-arasan: Override SDHCI_RETUNING_TIMER_COUNT_MASK on ZynqMP

[Marek Vasut]

On 12/22/22 10:20, Potthuri, Sai Krishna wrote:
> Hi Marek,
> 
>> -----Original Message-----
>> From: Marek Vasut <marex at denx.de>
>> Sent: Wednesday, December 21, 2022 3:10 PM
>> To: Potthuri, Sai Krishna <sai.krishna.potthuri at amd.com>; Adrian Hunter
>> <adrian.hunter at intel.com>; linux-mmc at vger.kernel.org
>> Cc: Michal Simek <michal.simek at xilinx.com>; Ulf Hansson
>> <ulf.hansson at linaro.org>; linux-arm-kernel at lists.infradead.org
>> Subject: Re: [PATCH] mmc: sdhci-of-arasan: Override
>> SDHCI_RETUNING_TIMER_COUNT_MASK on ZynqMP
>>
>> On 12/21/22 06:09, Potthuri, Sai Krishna wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>>>>>
>> https://www.xilinx.com/htmldocs/registers/ug1087/sdio___reg_capabil
>>>>>> it
>>>>>> ies.html#
>>>>>> Absolute Address  0x00FF160040 (SD0)
>>>>>> Reset Value       0x280737EC6481
>>>>>>
>>>>>> really reads 0x200737EC6481 . The interesting part is the top 32
>>>>>> bits, which are SDHCI_CAPABILITIES_1 = 0x2007. The missing 0x800 is
>>>>>> SDHCI_RETUNING_TIMER_COUNT_MASK=0, which makes the SDHCI
>> core
>>>> disable
>>>>>> retuning timer.
>>>>>>
>>>>>> Fix this up here by explicitly setting tuning_count to 8 as it
>>>>>> should be, otherwise an eMMC might fail in various thermal
>>>>>> conditions
>>>>>>
>>>>>> Note that the diff is best shown with -w option, this makes it
>>>>>> visible what happened with !sdhci_arasan->has_cqe conditional,
>>>>>> which is placed between sdhci_setup_host() and __sdhci_add_host()
>> calls.
>>>>>> Since sdhci_add_host() is also a sequence of these two calls and
>>>>>> host->tuning_count must be overriden before calling
>>>>>
>>>>> overriden -> overridden
>>>>
>>>> Fixed
>>>>
>>>>>> __sdhci_add_host(), call the two calls separately and do all the
>>>>>> adjustments between them in either case.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>>>>> ---
>>>>>> Cc: Michal Simek <michal.simek at xilinx.com>
>>>>>> Cc: Adrian Hunter <adrian.hunter at intel.com>
>>>>>> Cc: Ulf Hansson <ulf.hansson at linaro.org>
>>>>>> Cc: linux-arm-kernel at lists.infradead.org
>>>>>> To: linux-mmc at vger.kernel.org
>>>>>> ---
>>>>>>     drivers/mmc/host/sdhci-of-arasan.c | 57
>>>>>> ++++++++++++++++++++---------
>>>> -
>>>>>>     1 file changed, 38 insertions(+), 19 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>>>>>> b/drivers/mmc/host/sdhci-of-arasan.c
>>>>>> index 3997cad1f793d..465498f2a7c0f 100644
>>>>>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>>>>>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>>>>>> @@ -1521,37 +1521,56 @@ static int
>>>>>> sdhci_arasan_register_sdclk(struct
>>>> sdhci_arasan_data *sdhci_arasan,
>>>>>>        return 0;
>>>>>>     }
>>>>>>
>>>>>> -static int sdhci_arasan_add_host(struct sdhci_arasan_data
>>>>>> *sdhci_arasan)
>>>>>> +static int sdhci_arasan_add_host(struct sdhci_arasan_data
>>>> *sdhci_arasan,
>>>>>> +                             struct device *dev)
>>>>>>     {
>>>>>>        struct sdhci_host *host = sdhci_arasan->host;
>>>>>>        struct cqhci_host *cq_host;
>>>>>>        bool dma64;
>>>>>>        int ret;
>>>>>>
>>>>>> -    if (!sdhci_arasan->has_cqe)
>>>>>> -            return sdhci_add_host(host);
>>>>>> -
>>>>>>        ret = sdhci_setup_host(host);
>>>>>>        if (ret)
>>>>>>                return ret;
>>>>>>
>>>>>> -    cq_host = devm_kzalloc(host->mmc->parent,
>>>>>> -                           sizeof(*cq_host), GFP_KERNEL);
>>>>>> -    if (!cq_host) {
>>>>>> -            ret = -ENOMEM;
>>>>>> -            goto cleanup;
>>>>>> -    }
>>>>>> +    /*
>>>>>> +     * On Xilinx ZynqMP, the reg_capabilities (SDIO) Register
>>>>>> +     *
>>>>>> +     *
>>>>
>> https://www.xilinx.com/htmldocs/registers/ug1087/sdio___reg_capabilities.
>>>> html#
>>>>>> +     * Absolute Address  0x00FF160040 (SD0)
>>>>>> +     * Reset Value       0x280737EC6481
>>>>>> +     *
>>>>>> +     * really reads 0x200737EC6481 . The interesting part is the
>>>>>> +     * top 32 bits, which are SDHCI_CAPABILITIES_1 = 0x2007. The
>>>>>> +     * missing 0x800 is SDHCI_RETUNING_TIMER_COUNT_MASK=0,
>> which
>>>>>> +     * makes the SDHCI core disable retuning timer.
>>>>>
>>>>> Are you aware that caps can be changed in DT via "sdhci-caps" and
>>>>> "sdhci-caps-mask" ?
>>>>
>>>> No, I wasn't aware of those.
>>>>
>>>> Is that the preferred approach to this fix, over handling it in the driver ?
>>>>
>>>> I think the driver-side fix would be preferable, because it also
>>>> fixes systems which use legacy DTs without the sdhci-caps properties,
>>>> which would be all ZynqMP systems thus far.
>>>>
>>>> (and I would also still prefer to get feedback from Xilinx on why
>>>> does the value specified in UG1087 not match what is read out of the
>>>> hardware)
>>> Reset value of the retuning timer count is set to 0x0 via ZynqMP FSBL,
>>> we have an ERRATA for the same.
>>> https://support.xilinx.com/s/article/68550?language=en_US
>>>
>>> Xilinx recommendation is to program the appropriate value in the
>>> retuning timer count field based on the specific requirements via DT
>> property.
>>
>> Why is the retuning timer disabled for HS200 mode ?
> Based on discussions with the Xilinx IP design team, they told retuning is
> not required as Xilinx uses DLL for higher frequency modes.

Does this require the eMMC "DS" line ?

> So, we disabled retuning by default even in Xilinx next generation platforms
> like Versal.
> Even in our internal PVT testing also, without retuning we didn't see any issues.
> 
> Did you face any real issue without this re-tuning? If yes, could you please
> provide some details about the test case.

Yes, on devices with wider temperature range, the eMMC might suffer read 
failures over time.