Hi Marek, > -----Original Message----- > From: Marek Vasut <marex@xxxxxxx> > Sent: Wednesday, January 4, 2023 2:17 AM > To: Potthuri, Sai Krishna <sai.krishna.potthuri@xxxxxxx>; Adrian Hunter > <adrian.hunter@xxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx > Cc: Michal Simek <michal.simek@xxxxxxxxxx>; Ulf Hansson > <ulf.hansson@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Goud, > Srinivas <srinivas.goud@xxxxxxx> > 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@xxxxxxx> > >> Sent: Wednesday, December 21, 2022 3:10 PM > >> To: Potthuri, Sai Krishna <sai.krishna.potthuri@xxxxxxx>; Adrian > >> Hunter <adrian.hunter@xxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx > >> Cc: Michal Simek <michal.simek@xxxxxxxxxx>; Ulf Hansson > >> <ulf.hansson@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > >> 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@xxxxxxx> > >>>>>> --- > >>>>>> Cc: Michal Simek <michal.simek@xxxxxxxxxx> > >>>>>> Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx> > >>>>>> Cc: Ulf Hansson <ulf.hansson@xxxxxxxxxx> > >>>>>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > >>>>>> To: linux-mmc@xxxxxxxxxxxxxxx > >>>>>> --- > >>>>>> 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
