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@xxxxxxxxxxxxxx>
> > ---
> > 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 |
