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

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

[Artem Shimko]

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.

Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>
---

Hello maintainers and reviewers,

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.

Kindly, please review.

Many thanks.

--
Regards,
Artem

 drivers/mmc/host/sdhci-of-dwcmshc.c | 43 +++++++++++++++++------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index 2b75a36c096b..aca176d2c809 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -258,13 +258,11 @@ enum dwcmshc_rk_type {
 };
 
 struct rk35xx_priv {
-	struct reset_control *reset;
 	enum dwcmshc_rk_type devtype;
 	u8 txclk_tapnum;
 };
 
 struct eic7700_priv {
-	struct reset_control *reset;
 	unsigned int drive_impedance;
 };
 
@@ -272,6 +270,7 @@ struct eic7700_priv {
 
 struct dwcmshc_priv {
 	struct clk	*bus_clk;
+	struct reset_control *reset;
 	int vendor_specific_area1; /* P_VENDOR_SPECIFIC_AREA1 reg */
 	int vendor_specific_area2; /* P_VENDOR_SPECIFIC_AREA2 reg */
 
@@ -829,16 +828,15 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
-	struct rk35xx_priv *priv = dwc_priv->priv;
 	u32 extra = sdhci_readl(host, DECMSHC_EMMC_MISC_CON);
 
 	if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL))
 		cqhci_deactivate(host->mmc);
 
-	if (mask & SDHCI_RESET_ALL && priv->reset) {
-		reset_control_assert(priv->reset);
+	if (mask & SDHCI_RESET_ALL && dwc_priv->reset) {
+		reset_control_assert(dwc_priv->reset);
 		udelay(1);
-		reset_control_deassert(priv->reset);
+		reset_control_deassert(dwc_priv->reset);
 	}
 
 	sdhci_reset(host, mask);
@@ -863,9 +861,9 @@ static int dwcmshc_rk35xx_init(struct device *dev, struct sdhci_host *host,
 	else
 		priv->devtype = DWCMSHC_RK3568;
 
-	priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc));
-	if (IS_ERR(priv->reset)) {
-		err = PTR_ERR(priv->reset);
+	dwc_priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc));
+	if (IS_ERR(dwc_priv->reset)) {
+		err = PTR_ERR(dwc_priv->reset);
 		dev_err(mmc_dev(host->mmc), "failed to get reset control %d\n", err);
 		return err;
 	}
@@ -1331,24 +1329,24 @@ static void sdhci_eic7700_reset(struct sdhci_host *host, u8 mask)
 		sdhci_eic7700_config_phy(host);
 }
 
-static int sdhci_eic7700_reset_init(struct device *dev, struct eic7700_priv *priv)
+static int sdhci_eic7700_reset_init(struct device *dev, struct dwcmshc_priv *dwc_priv)
 {
 	int ret;
 
-	priv->reset = devm_reset_control_array_get_optional_exclusive(dev);
-	if (IS_ERR(priv->reset)) {
-		ret = PTR_ERR(priv->reset);
+	dwc_priv->reset = devm_reset_control_array_get_optional_exclusive(dev);
+	if (IS_ERR(dwc_priv->reset)) {
+		ret = PTR_ERR(dwc_priv->reset);
 		dev_err(dev, "failed to get reset control %d\n", ret);
 		return ret;
 	}
 
-	ret = reset_control_assert(priv->reset);
+	ret = reset_control_assert(dwc_priv->reset);
 	if (ret) {
 		dev_err(dev, "Failed to assert reset signals: %d\n", ret);
 		return ret;
 	}
 	usleep_range(2000, 2100);
-	ret = reset_control_deassert(priv->reset);
+	ret = reset_control_deassert(dwc_priv->reset);
 	if (ret) {
 		dev_err(dev, "Failed to deassert reset signals: %d\n", ret);
 		return ret;
@@ -1607,7 +1605,7 @@ static int eic7700_init(struct device *dev, struct sdhci_host *host, struct dwcm
 
 	dwc_priv->priv = priv;
 
-	ret = sdhci_eic7700_reset_init(dev, dwc_priv->priv);
+	ret = sdhci_eic7700_reset_init(dev, dwc_priv);
 	if (ret) {
 		dev_err(dev, "failed to reset\n");
 		return ret;
@@ -2122,6 +2120,8 @@ static int dwcmshc_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
+	reset_control_assert(priv->reset);
+
 	clk_disable_unprepare(pltfm_host->clk);
 	if (!IS_ERR(priv->bus_clk))
 		clk_disable_unprepare(priv->bus_clk);
@@ -2152,18 +2152,25 @@ static int dwcmshc_resume(struct device *dev)
 	if (ret)
 		goto disable_bus_clk;
 
-	ret = sdhci_resume_host(host);
+	ret = reset_control_deassert(priv->reset);
 	if (ret)
 		goto disable_other_clks;
 
+
+	ret = sdhci_resume_host(host);
+	if (ret)
+		goto assert_reset;
+
 	if (host->mmc->caps2 & MMC_CAP2_CQE) {
 		ret = cqhci_resume(host->mmc);
 		if (ret)
-			goto disable_other_clks;
+			goto assert_reset;
 	}
 
 	return 0;
 
+assert_reset:
+	reset_control_assert(priv->reset);
 disable_other_clks:
 	clk_bulk_disable_unprepare(priv->num_other_clks, priv->other_clks);
 disable_bus_clk:
-- 
2.43.0

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

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

[Artem Shimko]

Hi Andy,

Thank you for your review!

On Wed, Apr 15, 2026 at 5:52 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
> No need to repeat the commit message in the comments. It does not add any value.

Got it =)

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

I made these changes so that our MMC controller (dwc) can handle the
reset signal not only during the initialization function, but also
during suspend and resume.
Since the reset control was previously stored in vendor-specific
private structures (rk35xx_priv and eic7700_priv), it was inaccessible
from the common PM code.
To make suspend/resume work for our platform, I had to move the reset
pointer to the generic dwcmshc_priv structure, which required updating
the Rockchip and EIC7700 variants accordingly.



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

Yes, sure, thank you!

> No need to add an extra blank line.
okay =)


Instead of moving the reset control directly into the common path,
perhaps a cleaner solution would be to introduce a set of platform PM
ops (e.g., suspend/resume callbacks in the variant data).
These ops could be checked for NULL in the common
dwcmshc_suspend/resume functions. For platforms that don't implement
them, the behavior remains exactly as it was before (no reset
assertion),
ensuring zero risk of regression for Rockchip and EIC7700. Our
platform would then simply implement these ops to handle the custom
reset sequence.

What do you think about it?
--
Regards,
Artem

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

[Adrian Hunter]

On 17/04/2026 13:45, Artem Shimko wrote:
> Hi Andy,
> 
> Thank you for your review!
> 
> On Wed, Apr 15, 2026 at 5:52 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
>> No need to repeat the commit message in the comments. It does not add any value.
> 
> Got it =)
> 
>> 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?
> 
> I made these changes so that our MMC controller (dwc) can handle the
> reset signal not only during the initialization function, but also
> during suspend and resume.
> Since the reset control was previously stored in vendor-specific
> private structures (rk35xx_priv and eic7700_priv), it was inaccessible
> from the common PM code.
> To make suspend/resume work for our platform, I had to move the reset
> pointer to the generic dwcmshc_priv structure, which required updating
> the Rockchip and EIC7700 variants accordingly.
> 
> 
> 
>> 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.
> 
> Yes, sure, thank you!
> 
>> No need to add an extra blank line.
> okay =)
> 
> 
> Instead of moving the reset control directly into the common path,
> perhaps a cleaner solution would be to introduce a set of platform PM
> ops (e.g., suspend/resume callbacks in the variant data).
> These ops could be checked for NULL in the common
> dwcmshc_suspend/resume functions. For platforms that don't implement
> them, the behavior remains exactly as it was before (no reset
> assertion),
> ensuring zero risk of regression for Rockchip and EIC7700. Our
> platform would then simply implement these ops to handle the custom
> reset sequence.
> 
> What do you think about it?

Only dwcmshc_rk35xx_init() and sdhci_eic7700_reset_init() assign
dwc_priv->reset, so I am confused about what devices you intend
this patch for.

You definitely need to limit the change to devices that you know
for certain it will work.

> --
> Regards,
> Artem

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

[Artem Shimko]

Hi Adrian,

On Tue, May 5, 2026 at 2:28 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> Only dwcmshc_rk35xx_init() and sdhci_eic7700_reset_init() assign
> dwc_priv->reset, so I am confused about what devices you intend
> this patch for.

Yeah, the change as proposed would affect other devices, and I should
not assume it's safe for them without testing. Yes, I was so fast to
send it.
My intention was to make the reset control accessible for PM
operations, but only for the specific SoC I'm working with.
However, since the custom MMC driver code is not part of this upstream
submission, the patch currently lacks the necessary condition to limit
the new behavior.

Could you please check my plan to prepare v2 to:
1. Keep the reset control moved to dwcmshc_priv (so it's accessible
where needed).
2. Add a dedicated flag (e.g., bool needs_reset_on_pm or similar) to
dwcmshc_priv so that only devices that explicitly opt in will trigger
the reset during suspend/resume.

That ensures the existing behavior for Rockchip RK35xx and EIC7700
remains unchanged.
This way, the infrastructure is in place for devices that need it,
without imposing it on platforms where it hasn't been validated.
Thank you again for the review!

Regards,
Artem

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

[Adrian Hunter]

On 05/05/2026 15:12, Artem Shimko wrote:
> Hi Adrian,
> 
> On Tue, May 5, 2026 at 2:28 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Only dwcmshc_rk35xx_init() and sdhci_eic7700_reset_init() assign
>> dwc_priv->reset, so I am confused about what devices you intend
>> this patch for.
> 
> Yeah, the change as proposed would affect other devices, and I should
> not assume it's safe for them without testing. Yes, I was so fast to
> send it.
> My intention was to make the reset control accessible for PM
> operations, but only for the specific SoC I'm working with.
> However, since the custom MMC driver code is not part of this upstream
> submission, the patch currently lacks the necessary condition to limit
> the new behavior.
> 
> Could you please check my plan to prepare v2 to:
> 1. Keep the reset control moved to dwcmshc_priv (so it's accessible
> where needed).
> 2. Add a dedicated flag (e.g., bool needs_reset_on_pm or similar) to
> dwcmshc_priv so that only devices that explicitly opt in will trigger
> the reset during suspend/resume.

Sounds fine

> 
> That ensures the existing behavior for Rockchip RK35xx and EIC7700
> remains unchanged.
> This way, the infrastructure is in place for devices that need it,
> without imposing it on platforms where it hasn't been validated.
> Thank you again for the review!
> 
> Regards,
> Artem

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

[Artem Shimko]

Hi Adrian,

On Tue, May 5, 2026 at 3:37 PM Adrian Hunter <adrian.hunter@intel.com> wrote:

> Sounds fine

Great! Let me send v2.

Thank you.

Regards,
Artem