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

[Michal Simek <michal.simek@amd.com>]

On 1/3/23 21:35, Marek Vasut wrote:
> On 1/2/23 09:24, Michal Simek wrote:
>>
>>
>> On 12/30/22 13:57, Adrian Hunter wrote:
>>> 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.
>>>
>>
>> Xilinx Device Tree Generator, which is the tool for DT generation, was never 
>> designed to be directly used without any change. It was designed to help you 
>> to describe the system as much as possible. It means you get the base and you 
>> need to change things which are not properly described. That's why just do it.
> 
> I am under the impression that petalinux does pull the XSA from Vivado and 
> directly builds U-Boot and Linux with DT somehow derived from the XSA, maybe 
> using DTG ?
> 
> (note that I am not using petalinux)

They do use it but they are not keeping backward compatibility that's why they 
don't need to solve this problem.

Thanks,
Michal