Re: [PATCH] mmc: sdhci-of-dwcmshc: add reset control support in suspend/resume

[Andy Shevchenko]

On Wed, Apr 15, 2026 at 03:34:10PM +0300, Artem Shimko wrote:
> The driver currently handles reset control only during initializations,
> but does not manage the controller reset signal during system
> suspend/resume. This can leave the hardware in an undefined state
> after resume. Additionally, the reset control is stored in vendor
> structs making it inaccessible for power management operations.
> 
> Move the reset control to dwcmshc_priv structure and add proper
> assertion during suspend and deassertion during resume. This ensures the
> controller is properly reset before entering low power state and correctly
> reinitialized upon wake.

...

> ---
> 
> Hello maintainers and reviewers,

No need to repeat the commit message in the comments. It does not add any value.

> This patch adds reset control support for the MMC controller
> during system suspend/resume.
> 
> Currently, the reset control is only handled during initialization and
> is stored in vendor-specific private structures. This makes it
> inaccessible to the common power management code and leaves the
> controller in an undefined state after resume.
> 
> The changes move the reset control to the generic dwcmshc_priv structure,
> update all platform-specific users (Rockchip, EIC7700), and add proper
> reset assertion/deassertion in dwcmshc_suspend/resume.

This is serious behaviour change (might not be, one needs to understand what it
does in comparison to no reset (and in accordance with datasheet).

Do you have any HW to test?

...

>  	clk_disable_unprepare(pltfm_host->clk);
>  	if (!IS_ERR(priv->bus_clk))
>  		clk_disable_unprepare(priv->bus_clk);

The driver seems can be refactored a lot (with no functional changes) by:
- replacing *sleep() with fsleep() API
- dropping unneeded checks like above, clk_disable_unprepare() is error
pointer-aware IIRC (or it can be made so it is either NULL or valid one)
- using 'return dev_err_probe()'

This is just a side note in case you are interested.

>  	if (ret)
>  		goto disable_bus_clk;

...

> -	ret = sdhci_resume_host(host);
> +	ret = reset_control_deassert(priv->reset);
>  	if (ret)
>  		goto disable_other_clks;
>  
> +

No need to add an extra blank line.

> +	ret = sdhci_resume_host(host);
> +	if (ret)
> +		goto assert_reset;

-- 
With Best Regards,
Andy Shevchenko