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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The patch introduces unconditional kernel log spam ('Failed to restore pinctrl state') on every system resume for devices lacking high-speed pinctrl states.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but devices with a 'sleep' pinctrl state but lacking high-speed states ('100mhz'/'200mhz') fail to exit sleep state on resume.
--

commit 831a40635a59ef0e713e681b7fa2933e80e52391
Author: Luke Wang <ziniu.wang_1@nxp.com>

mmc: sdhci-esdhc-imx: restore pinctrl before restoring ios timing on resume

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.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 7230d70e02ae1..3b1e63425a193 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");

[Severity: Medium]
Does this new dev_warn() cause log spam on every system resume for devices
without high-speed pinctrl states?

If a board doesn't define the optional pins_100mhz or pins_200mhz states,
esdhc_change_pinstate() returns -EINVAL:

drivers/mmc/host/sdhci-esdhc-imx.c:esdhc_change_pinstate() {
	...
	if (IS_ERR(imx_data->pinctrl) ||
		IS_ERR(imx_data->pins_100mhz) ||
		IS_ERR(imx_data->pins_200mhz))
		return -EINVAL;
	...
}

This means standard SD setups without UHS pins will unconditionally see this
warning on every system resume.

[Severity: High]
This is a pre-existing issue, but does this early return in
esdhc_change_pinstate() prevent restoring the default pin state when high-speed
pins are not defined?

If esdhc_change_pinstate() returns early with -EINVAL, the default pin state
is never restored for legacy timings:

drivers/mmc/host/sdhci-esdhc-imx.c:esdhc_change_pinstate() {
	...
	switch (uhs) {
	...
	default:
		/* back to default state for other legacy timing */
		return pinctrl_select_default_state(mmc_dev(host->mmc));
	}
	...
}

For devices with a sleep pinctrl state but lacking high-speed states,
they would fail to exit the sleep state on resume, leaving the pins in a
low-power configuration and potentially making the device unusable.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703104208.3426374-1-ziniu.wang_1@oss.nxp.com?part=3