Re: [PATCH v2 4/6] mmc: core: add undervoltage handler for MMC/eMMC devices

[Oleksij Rempel <o.rempel@pengutronix.de>]

On Thu, Feb 20, 2025 at 10:47:01AM +0000, Christian Loehle wrote:
> On 2/20/25 07:44, Oleksij Rempel wrote:
> > Introduce _mmc_handle_undervoltage() to handle undervoltage events for
> > MMC/eMMC devices. This function interrupts ongoing operations using HPI
> > and performs a controlled suspend. After completing the sequence, the card
> > is marked as removed to prevent further interactions.
> > 
> > To support this, introduce __mmc_suspend() and __mmc_resume() as internal
> > helpers that omit mmc_claim_host()/mmc_release_host(), allowing them to be
> > called when the host is already claimed. Update mmc_shutdown() to skip the
> > normal shutdown sequence if the host is flagged as undervoltage to avoid
> > repeating of the shutdown procedure.
> 
> "of" can be removed here.
> 
> Given that this introduces large parts of the mmc handling IMO this commit
> deserves a lot more explanation of what steps exactly do for which cards
> and why.

ack

> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/mmc/core/mmc.c | 81 +++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 68 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> > index 86b608843232..e626213e7a52 100644
> > --- a/drivers/mmc/core/mmc.c
> > +++ b/drivers/mmc/core/mmc.c
> > @@ -2104,8 +2104,8 @@ static int _mmc_flush_cache(struct mmc_host *host)
> >  	return err;
> >  }
> >  
> > -static int _mmc_suspend(struct mmc_host *host, bool is_suspend,
> > -			bool is_undervoltage)
> > +static int __mmc_suspend(struct mmc_host *host, bool is_suspend,
> > +			 bool is_undervoltage)
> 
> The is_undervoltage doesn't do anything? Did you forget something here?

This was done in the previous patch "mmc: core: refactor _mmc_suspend()
for undervoltage handling"

> > @@ -2189,6 +2205,9 @@ static int mmc_shutdown(struct mmc_host *host)
> >  {
> >  	int err = 0;
> >  
> > +	if (host->undervoltage)
> > +		return 0;
> > +
> 
> Probably deserves a comment.

ack

> >  	/*
> >  	 * In a specific case for poweroff notify, we need to resume the card
> >  	 * before we can shutdown it properly.
> > @@ -2280,6 +2299,41 @@ static int _mmc_hw_reset(struct mmc_host *host)
> >  	return mmc_init_card(host, card->ocr, card);
> >  }
> >  
> > +static int _mmc_handle_undervoltage(struct mmc_host *host)
> > +{
> > +	struct mmc_card *card = host->card;
> > +	int err = 0;
> > +
> > +	mmc_claim_host(host);
> > +
> > +	if (!host->card)
> > +		goto out;
> > +
> > +	if (mmc_can_poweroff_notify(host->card) &&
> > +		!(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE))
> > +		err = __mmc_resume(host);
> 
> I'm not sure I follow this.
> Why would we power-up a card that currently doesn't have power when we
> know we are about to powerfail it?

It is part of the mmc_shutdown, but it is not used on my HW. So, can be
skip it.

> > +
> > +	if (!err) {
> > +		err = mmc_interrupt_hpi(card);
> > +		if (err)
> > +			pr_err("%s: Interrupt HPI failed, error %d\n",
> > +				mmc_hostname(host), err);
> 
> There's no point in calling this for SD but I don't see why it currently
> wouldn't be called for SD.

I tried to keep budget low, until we agree that it is the way to go.
After this patch stack is accepted, i can try to request more time to
add and test the SD handler.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |