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

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