On 2/20/25 10:56, Oleksij Rempel wrote:
> 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"
Sorry!
>
>>> @@ -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.
If you're not implementing this for now, why not just drop the undervoltage
event at patch 1/6 if host doesn't have an eMMC attached?
