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

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

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)

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

> 
> 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,
>>>  };
>>>
>