Message-ID: <be715535-8931-489f-a2be-50aa6ec423a1@arm.com>
Date: Thu, 20 Feb 2025 10:47:01 +0000
From: Christian Loehle <christian.loehle@....com>
To: Oleksij Rempel <o.rempel@...gutronix.de>,
 Ulf Hansson <ulf.hansson@...aro.org>
Cc: kernel@...gutronix.de, linux-kernel@...r.kernel.org,
 linux-mmc@...r.kernel.org, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Mark Brown <broonie@...nel.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
 Søren Andersen <san@...v.dk>
Subject: Re: [PATCH v2 4/6] mmc: core: add undervoltage handler for MMC/eMMC
 devices

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.

> 
> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.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?

>  {
>  	unsigned int notify_type;
>  	int err = 0;
> @@ -2115,8 +2115,6 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend,
>  	else
>  		notify_type = EXT_CSD_POWER_OFF_LONG;
>  
> -	mmc_claim_host(host);
> -
>  	if (mmc_card_suspended(host->card))
>  		goto out;
>  
> @@ -2140,7 +2138,18 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend,
>  		mmc_card_set_suspended(host->card);
>  	}
>  out:
> +	return err;
> +}
> +
> +static int _mmc_suspend(struct mmc_host *host, bool is_suspend,
> +			bool is_undervoltage)
> +{
> +	int err;
> +
> +	mmc_claim_host(host);
> +	err = __mmc_suspend(host, is_suspend, is_undervoltage);
>  	mmc_release_host(host);
> +
>  	return err;
>  }
>  
> @@ -2160,6 +2169,20 @@ static int mmc_suspend(struct mmc_host *host)
>  	return err;
>  }
>  
> +static int __mmc_resume(struct mmc_host *host)
> +{
> +	int err;
> +
> +	if (!mmc_card_suspended(host->card))
> +		return 0;
> +
> +	mmc_power_up(host, host->card->ocr);
> +	err = mmc_init_card(host, host->card->ocr, host->card);
> +	mmc_card_clr_suspended(host->card);
> +
> +	return err;
> +}
> +
>  /*
>   * This function tries to determine if the same card is still present
>   * and, if so, restore all state to it.
> @@ -2169,16 +2192,9 @@ static int _mmc_resume(struct mmc_host *host)
>  	int err = 0;
>  
>  	mmc_claim_host(host);
> -
> -	if (!mmc_card_suspended(host->card))
> -		goto out;
> -
> -	mmc_power_up(host, host->card->ocr);
> -	err = mmc_init_card(host, host->card->ocr, host->card);
> -	mmc_card_clr_suspended(host->card);
> -
> -out:
> +	err = __mmc_resume(host);
>  	mmc_release_host(host);
> +
>  	return err;
>  }
>  
> @@ -2189,6 +2205,9 @@ static int mmc_shutdown(struct mmc_host *host)
>  {
>  	int err = 0;
>  
> +	if (host->undervoltage)
> +		return 0;
> +

Probably deserves a comment.

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

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

> +
> +		err = __mmc_suspend(host, false, true);
> +	}
> +
> +	if (err)
> +		pr_err("%s: Undervoltage emergency stop failed\n",
> +			mmc_hostname(host));
> +
> +	mmc_card_set_removed(host->card);
> +
> +out:
> +	mmc_release_host(host);
> +
> +	return err;
> +}
> +
>  static const struct mmc_bus_ops mmc_ops = {
>  	.remove = mmc_remove,
>  	.detect = mmc_detect,
> @@ -2292,6 +2346,7 @@ static const struct mmc_bus_ops mmc_ops = {
>  	.hw_reset = _mmc_hw_reset,
>  	.cache_enabled = _mmc_cache_enabled,
>  	.flush_cache = _mmc_flush_cache,
> +	.handle_undervoltage = _mmc_handle_undervoltage,
>  };
>  
>  /*

Powered by blists - more mailing lists