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

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

On 30/12/22 08:42, Marek Vasut wrote:
> On 12/29/22 13:51, Adrian Hunter wrote:
>> On 26/10/22 12:20, Marek Vasut wrote:
>>> On 10/26/22 08:07, Adrian Hunter wrote:
>>>> On 25/10/22 22:15, Marek Vasut wrote:
>>>>> 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.
>>>>>
>>>>> 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 guess ideally.  Mainline does not really need the driver
>> fix because it seems it can be done by DT.  Older kernels
>> are a separate issue really.
>>
>>>
>>> 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.
>>
>> You could backport support of the properties "sdhci-caps"
>> and "sdhci-caps-mask".
> 
> This won't help. Vivado (the xilinx FPGA design tool) is capable of generating DTs, so you can end up with a combination of new Linux kernel and old generated DT, which is still missing the sdhci-caps/sdhci-caps-mask .

That is a bit sad.  You might want to push for changing that situation.

Send an updated patch then.