[PATCH] mmc: sdhci-of-arasan: Override SDHCI_RETUNING_TIMER_COUNT_MASK on ZynqMP
Potthuri, Sai Krishna
sai.krishna.potthuri at amd.com
Mon Feb 6 00:49:34 PST 2023
Hi Marek,
> -----Original Message-----
> From: Marek Vasut <marex at denx.de>
> Sent: Wednesday, January 4, 2023 2:17 AM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri at amd.com>; Adrian Hunter
> <adrian.hunter at intel.com>; linux-mmc at vger.kernel.org
> Cc: Michal Simek <michal.simek at xilinx.com>; Ulf Hansson
> <ulf.hansson at linaro.org>; linux-arm-kernel at lists.infradead.org; Goud,
> Srinivas <srinivas.goud at amd.com>
> Subject: Re: [PATCH] mmc: sdhci-of-arasan: Override
> SDHCI_RETUNING_TIMER_COUNT_MASK on ZynqMP
>
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
>
>
> On 12/22/22 10:20, Potthuri, Sai Krishna wrote:
> > Hi Marek,
> >
> >> -----Original Message-----
> >> From: Marek Vasut <marex at denx.de>
> >> Sent: Wednesday, December 21, 2022 3:10 PM
> >> To: Potthuri, Sai Krishna <sai.krishna.potthuri at amd.com>; Adrian
> >> Hunter <adrian.hunter at intel.com>; linux-mmc at vger.kernel.org
> >> Cc: Michal Simek <michal.simek at xilinx.com>; Ulf Hansson
> >> <ulf.hansson at linaro.org>; linux-arm-kernel at lists.infradead.org
> >> Subject: Re: [PATCH] mmc: sdhci-of-arasan: Override
> >> SDHCI_RETUNING_TIMER_COUNT_MASK on ZynqMP
> >>
> >> On 12/21/22 06:09, Potthuri, Sai Krishna wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>>>>>
> >> https://www.xilinx.com/htmldocs/registers/ug1087/sdio___reg_capabil
> >>>>>> it
> >>>>>> 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 at denx.de>
> >>>>>> ---
> >>>>>> Cc: Michal Simek <michal.simek at xilinx.com>
> >>>>>> Cc: Adrian Hunter <adrian.hunter at intel.com>
> >>>>>> Cc: Ulf Hansson <ulf.hansson at linaro.org>
> >>>>>> Cc: linux-arm-kernel at lists.infradead.org
> >>>>>> To: linux-mmc at 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 ?
> > Based on discussions with the Xilinx IP design team, they told
> > retuning is not required as Xilinx uses DLL for higher frequency modes.
>
> Does this require the eMMC "DS" line ?
No.
>
> > So, we disabled retuning by default even in Xilinx next generation
> > platforms like Versal.
> > Even in our internal PVT testing also, without retuning we didn't see any
> issues.
> >
> > Did you face any real issue without this re-tuning? If yes, could you
> > please provide some details about the test case.
>
> Yes, on devices with wider temperature range, the eMMC might suffer read
> failures over time.
During Xilinx internal PVT testing we didn't see any issues without doing the
retuning. If you can see the issue at a particular temperature, please let
us know.
In case if there is any issue, we can make use of the DT property as mentioned.
Regards
Sai Krishna
More information about the linux-arm-kernel
mailing list