Received-SPF: pass (google.com: domain of linux-kernel+bounces-44574-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240126190110.148599-1-afd@ti.com>
In-Reply-To: <20240126190110.148599-1-afd@ti.com>
From: Ulf Hansson <ulf.hansson@linaro.org>
Date: Tue, 30 Jan 2024 13:04:44 +0100
Message-ID: <CAPDyKFpc38-CFrzhnhutS7c78tZTLM6Bg6XsTKENP8oVT6SQXg@mail.gmail.com>
Subject: Re: [PATCH] mmc: pwrseq: Use proper reboot notifier path
To: Andrew Davis <afd@ti.com>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Yangtao Li <frank.li@vivo.com>, 
	linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

On Fri, 26 Jan 2024 at 20:01, Andrew Davis <afd@ti.com> wrote:
>
> This driver registers itself as a reboot handler, which means it claims
> it can reboot the system. It does this so it is called during the system
> reboot sequence. The correct way to be notified during the reboot
> sequence is to register a notifier with register_reboot_notifier().
> Do this here.
>
> Note this will be called during normal reboots but not emergency reboots.
> This is the expected behavior, emergency reboot means emergency, not go
> do some cleanup with emmc pins.. The reboot notifiers are intentionally
> not called in the emergency path for a reason and working around that by
> pretending to be a reboot handler is a hack.

I understand the reason for the $subject patch, but it will not work,
unfortunately.

For eMMC we need to manage emergency reboots too. The fiddling with
GPIOs isn't a "cleanup", but tries to move the eMMC into a clean reset
state. This is needed on some platforms with broken bootloaders (ROM
code), that is expecting the eMMC to always start in a clean reset
state.

So, we need both parts, as was discussed here [1] too.

Kind regards
Uffe

[1]
https://lore.kernel.org/all/1445440540-21525-1-git-send-email-javier@osg.samsung.com/

>
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
>  drivers/mmc/core/pwrseq_emmc.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/mmc/core/pwrseq_emmc.c
> index 3b6d69cefb4eb..d5045fd1a02c1 100644
> --- a/drivers/mmc/core/pwrseq_emmc.c
> +++ b/drivers/mmc/core/pwrseq_emmc.c
> @@ -70,14 +70,8 @@ static int mmc_pwrseq_emmc_probe(struct platform_device *pdev)
>                 return PTR_ERR(pwrseq->reset_gpio);
>
>         if (!gpiod_cansleep(pwrseq->reset_gpio)) {
> -               /*
> -                * register reset handler to ensure emmc reset also from
> -                * emergency_reboot(), priority 255 is the highest priority
> -                * so it will be executed before any system reboot handler.
> -                */
>                 pwrseq->reset_nb.notifier_call = mmc_pwrseq_emmc_reset_nb;
> -               pwrseq->reset_nb.priority = 255;
> -               register_restart_handler(&pwrseq->reset_nb);
> +               register_reboot_notifier(&pwrseq->reset_nb);
>         } else {
>                 dev_notice(dev, "EMMC reset pin tied to a sleepy GPIO driver; reset on emergency-reboot disabled\n");
>         }
> --
> 2.39.2
>