RE: [PATCH] mmc: sdhci-of-arasan: Ensure CD logic stabilization before power-up

["Potthuri, Sai Krishna" <sai.krishna.potthuri@amd.com>]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Adrian,

> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@intel.com>
> Sent: Friday, July 25, 2025 1:45 PM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Simek, Michal
> <michal.simek@amd.com>; Ulf Hansson <ulf.hansson@linaro.org>
> Cc: linux-mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>;
> saikrishna12468@gmail.com
> Subject: Re: [PATCH] mmc: sdhci-of-arasan: Ensure CD logic stabilization before
> power-up
>
> On 25/07/2025 08:49, Potthuri, Sai Krishna wrote:
> > [Public]
> >
> > Hi Adrian,
> >
> >> -----Original Message-----
> >> From: Adrian Hunter <adrian.hunter@intel.com>
> >> Sent: Thursday, July 24, 2025 9:50 PM
> >> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Simek,
> >> Michal <michal.simek@amd.com>; Ulf Hansson <ulf.hansson@linaro.org>
> >> Cc: linux-mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> linux- kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>;
> >> saikrishna12468@gmail.com
> >> Subject: Re: [PATCH] mmc: sdhci-of-arasan: Ensure CD logic
> >> stabilization before power-up
> >>
> >> On 21/07/2025 12:53, Sai Krishna Potthuri wrote:
> >>> During SD suspend/resume without a full card rescan (when using
> >>> non-removable SD cards for rootfs), the SD card initialization may
> >>> fail after resume. This occurs because, after a host controller
> >>> reset, the card detect logic may take time to stabilize due to debounce logic.
> >>> Without waiting for stabilization, the host may attempt powering up
> >>> the card prematurely, leading to command timeouts during resume flow.
> >>> Add sdhci_arasan_set_power_and_bus_voltage() to wait for the card
> >>> detect stable bit before power up the card. Since the stabilization
> >>> time is not fixed, a maximum timeout of one second is used to ensure
> >>> sufficient wait time for the card detect signal to stabilize.
> >>>
> >>> Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> >>> ---
> >>>  drivers/mmc/host/sdhci-of-arasan.c | 24 ++++++++++++++++++++++--
> >>>  1 file changed, 22 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
> >>> b/drivers/mmc/host/sdhci-of-arasan.c
> >>> index 42878474e56e..3ce55009ba4a 100644
> >>> --- a/drivers/mmc/host/sdhci-of-arasan.c
> >>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> >>> @@ -99,6 +99,9 @@
> >>>  #define HIWORD_UPDATE(val, mask, shift) \
> >>>             ((val) << (shift) | (mask) << ((shift) + 16))
> >>>
> >>> +#define CD_STABLE_TIMEOUT_US               1000000
> >>> +#define CD_STABLE_MAX_SLEEP_US             10
> >>> +
> >>>  /**
> >>>   * struct sdhci_arasan_soc_ctl_field - Field used in
> sdhci_arasan_soc_ctl_map
> >>>   *
> >>> @@ -514,6 +517,23 @@ static int sdhci_arasan_voltage_switch(struct
> >> mmc_host *mmc,
> >>>     return -EINVAL;
> >>>  }
> >>>
> >>> +static void sdhci_arasan_set_power_and_bus_voltage(struct
> >>> +sdhci_host *host,
> >> unsigned char mode,
> >>> +                                              unsigned short vdd) {
> >>> +   u32 reg;
> >>> +
> >>> +   /*
> >>> +    * Ensure that the card detect logic has stabilized before
> >>> + powering up, this
> >> is
> >>> +    * necessary after a host controller reset.
> >>> +    */
> >>> +   if (mode == MMC_POWER_UP) {
> >>> +           readl_poll_timeout(host->ioaddr + SDHCI_PRESENT_STATE,
> >>> + reg,
> >> reg & SDHCI_CD_STABLE,
> >>> +                              CD_STABLE_MAX_SLEEP_US,
> >> CD_STABLE_TIMEOUT_US);
> >>> +   }
> >>
> >> Doesn't need {}
> > Will remove in v2.
>
> Also probably better to access the register in a consistent manner i.e. use
> read_poll_timeout(sdhci_readl,...,host, SDHCI_PRESENT_STATE)
Sure, I will update.
>
> >
> >>
> >> Will this work with all Arasan variants?
> > Yes, this is expected to work across all Arasan variants that comply
> > with the standard SDHCI register definitions. The SDHCI_CD_STABLE bit
> > is defined in both the standard SDHCI specification and Arasan's user guide.
> > On Xilinx/AMD Versal and ZynqMP platforms, the CD stable bit is
> > typically set within a few milliseconds. However, to be on the safer
> > side and ensure compatibility across all Arasan variants, a timeout of 1 second is
> added.
>
> A lower timeout would have less issue if there were devices that did not have
> standard CD stable bit behaviour.
>
> > Please let me know if you prefer to increase the timeout or if this
> > logic should be enabled by a platform specific quirk.
>
> If you are 100% confident it won't negatively affect other devices, then it is OK.
> Otherwise it is better as a quirk.
For the devices that didn't have or broken CD stable behavior then they might see
increase in the initialization time that is the only impact I can see.
May be better I will enable the logic using platform specific quirk, so that other
devices will have zero impact and whoever needs it they will define the quirk.

Regards
Sai Krishna

>
> >
> > Regards
> > Sai Krishna
> >
> >>
> >>> +
> >>> +   sdhci_set_power_and_bus_voltage(host, mode, vdd); }
> >>> +
> >>>  static const struct sdhci_ops sdhci_arasan_ops = {
> >>>     .set_clock = sdhci_arasan_set_clock,
> >>>     .get_max_clock = sdhci_pltfm_clk_get_max_clock, @@ -521,7 +541,7
> >> @@
> >>> static const struct sdhci_ops sdhci_arasan_ops = {
> >>>     .set_bus_width = sdhci_set_bus_width,
> >>>     .reset = sdhci_arasan_reset,
> >>>     .set_uhs_signaling = sdhci_set_uhs_signaling,
> >>> -   .set_power = sdhci_set_power_and_bus_voltage,
> >>> +   .set_power = sdhci_arasan_set_power_and_bus_voltage,
> >>>     .hw_reset = sdhci_arasan_hw_reset,  };
> >>>
> >>> @@ -570,7 +590,7 @@ static const struct sdhci_ops sdhci_arasan_cqe_ops
> = {
> >>>     .set_bus_width = sdhci_set_bus_width,
> >>>     .reset = sdhci_arasan_reset,
> >>>     .set_uhs_signaling = sdhci_set_uhs_signaling,
> >>> -   .set_power = sdhci_set_power_and_bus_voltage,
> >>> +   .set_power = sdhci_arasan_set_power_and_bus_voltage,
> >>>     .irq = sdhci_arasan_cqhci_irq,
> >>>  };
> >>>
> >