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

[Marek Vasut <marex@denx.de>]

On 12/21/22 06:09, Potthuri, Sai Krishna wrote:
> Hi Marek,

Hi,

>>>> https://www.xilinx.com/htmldocs/registers/ug1087/sdio___reg_capabilit
>>>> 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@denx.de>
>>>> ---
>>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> To: linux-mmc@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 ?