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

[Marek Vasut <marex@denx.de>]

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)