RE: [PATCH v4 3/7] mmc: sdhci-esdhc-imx: restore pinctrl before restoring ios timing on resume

> -----Original Message-----
> From: Adrian Hunter <[email protected]>
> Sent: Sunday, July 5, 2026 4:15 PM
> To: Luke Wang (OSS) <[email protected]>; [email protected]; Bough
> Chen <[email protected]>; Frank Li <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; dl-S32 <[email protected]>;
> [email protected]; [email protected]
> Subject: Re: [PATCH v4 3/7] mmc: sdhci-esdhc-imx: restore pinctrl before
> restoring ios timing on resume
> 
> On 03/07/2026 13:42, [email protected] wrote:
> > From: Luke Wang <[email protected]>
> >
> > SDIO devices such as WiFi may keep power during suspend, so the MMC
> > core skips full card re-initialization on resume and directly restores
> > the host controller's ios timing to match the card. For DDR mode,
> > pm_runtime_force_resume() sets DDR_EN before the pin configuration is
> > restored from sleep state.
> >
> > This is related to the SoC IP integration: switching pinctrl setting
> > (changing alt from GPIO to USDHC) impacts the internal loopback path.
> > If pinctrl configures the pad to GPIO function, once DDR_EN is set, the
> > DLL delay will be fixed based on the GPIO function loopback path. When
> > the pinctrl is later changed to USDHC function, the internal loopback
> > path changes, making the original fixed sample point no longer suitable
> > for the current loopback path. This causes persistent read CRC errors on
> > subsequent data transfers.
> >
> > SD/eMMC running in DDR mode are unaffected as they are fully
> > re-initialized from legacy timing after resume.
> >
> > Fix this by restoring the pinctrl state based on current timing mode
> > using esdhc_change_pinstate() before pm_runtime_force_resume(). This
> > ensures the correct pin configuration (e.g., 100/200MHz for UHS modes)
> > is applied before DDR_EN is set. Only restore for non-wakeup devices
> > since wakeup devices kept their active pin state during suspend.
> >
> > Fixes: 676a83855614 ("mmc: host: sdhci-esdhc-imx: refactor the system PM
> logic")
> > Reviewed-by: Haibo Chen <[email protected]>
> > Signed-off-by: Luke Wang <[email protected]>
> > ---
> >  drivers/mmc/host/sdhci-esdhc-imx.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-
> esdhc-imx.c
> > index 7230d70e02ae..3b1e63425a19 100644
> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > @@ -2113,6 +2113,12 @@ static int sdhci_esdhc_resume(struct device
> *dev)
> >  	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
> >  	int ret;
> >
> > +	if (!device_may_wakeup(dev)) {
> > +		ret = esdhc_change_pinstate(host, host->timing);
> > +		if (ret)
> > +			dev_warn(dev, "Failed to restore pinctrl state\n");
> 
> Sashiko has a point about this dev_warn().  See its first comment in:
> 
> 	https://sashiko.dev/#/patchset/20260703104208.3426374-1-
> ziniu.wang_1%40oss.nxp.com?part=3

Thanks for catching this. It's a real issue.

The root cause is in esdhc_change_pinstate() - the early check for 
pins_100mhz/pins_200mhz blocks the default switch case from restoring 
pinctrl via pinctrl_select_default_state(), even for timings that don't
need UHS states.

Will send v5 patch fixing this.

Thanks,
Luke

> 
> > +	}
> > +
> >  	pm_runtime_force_resume(dev);
> >
> >  	ret = mmc_gpio_set_cd_wake(host->mmc, false);