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

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

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".

> 
> (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)
> 
> [...]