Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler
From: Marek Szyprowski
Date: Thu Oct 22 2015 - 06:07:30 EST
Hello,
On 2015-10-22 06:14, Alim Akhtar wrote:
CCing Doug, Heiko and Enric Balletbo
To help us by testing on rk3288-veyron and am335x-sl50 boards.
On 10/22/2015 08:22 AM, Javier Martinez Canillas wrote:
Hello Krzysztof,
On 10/22/2015 03:43 AM, Krzysztof Kozlowski wrote:
On 22.10.2015 10:20, Javier Martinez Canillas wrote:> Hello Krzysztof,
Thanks for your feedback.
On 10/22/2015 02:36 AM, Krzysztof Kozlowski wrote:
On 22.10.2015 00:15, Javier Martinez Canillas wrote:
The pwrseq_emmc driver does a eMMC card reset before a system
reboot to
allow broken or limited ROM boot-loaders (that don't have an eMMC
reset
logic) to be able to read the second stage from the eMMC.
But this has to be called before a system reboot handler and
while most
of them use the priority 128, there are other restart handlers
(such as
the syscon-reboot one) that use a higher priority. So, use the
highest
priority to make sure that the eMMC hw is reset before a system
reboot.
Signed-off-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
Tested-by: Markus Reichl <m.reichl@xxxxxxxxxxxxx>
Tested-by: Anand Moon <linux.amoon@xxxxxxxxx>
Reviewed-by: Alim Akhtar <alim.akhtar@xxxxxxxxxxx>
---
Hello,
This patch was needed since a recent series from Alim [0] added
syscon reboot and poweroff support to Exynos SoCs and removed
the reset handler in the Exynos Power Management Unit (PMU) code.
But the PMU and syscon-reboot restart handler have a different
priority so [0] breaks restart when eMMC is used on these boards.
[0]: http://www.spinics.net/lists/arm-kernel/msg454396.html
So this patch must be merged before [0] to avoid regressions.
Best regards,
Javier
drivers/mmc/core/pwrseq_emmc.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/core/pwrseq_emmc.c
b/drivers/mmc/core/pwrseq_emmc.c
index 137c97fb7aa8..ad4f94ec7e8d 100644
--- a/drivers/mmc/core/pwrseq_emmc.c
+++ b/drivers/mmc/core/pwrseq_emmc.c
@@ -84,11 +84,11 @@ struct mmc_pwrseq
*mmc_pwrseq_emmc_alloc(struct mmc_host *host,
/*
* register reset handler to ensure emmc reset also from
- * emergency_reboot(), priority 129 schedules it just before
- * system reboot
+ * 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 = 129;
+ pwrseq->reset_nb.priority = 255;
I see the problem which you are trying to solve but this may be
tricker
then just kicking the number. Some of restart handlers are registered
with priority 192. I found few of such, like: at91_restart_nb,
zynq_slcr_restart_nb, rmobile_reset_nb (maybe more, I did not grep
too
much).
Yes, the syscon-reboot restart handler also uses a priority 192 and
that
is why reboot with eMMC broke with Alim's patches since the PMU
restart
handler priority is 128.
I guess they chose the "192" priority on purpose.
I tried to understand what's the policy w.r.t priority numbering for
restart handlers but only found this in the register_restart_handler
kernel-doc [0]:
/**
* register_restart_handler - Register function to be called to
reset
* the system
* @nb: Info about handler function to be called
* @nb->priority: Handler priority. Handlers should follow the
* following guidelines for setting priorities.
* 0: Restart handler of last resort,
* with limited restart capabilities
* 128: Default restart handler; use if no other
* restart handler is expected to be available,
* and/or if restart functionality is
* sufficient to restart the entire system
* 255: Highest priority restart handler, will
* preempt all other restart handlers
So, reading that is not clear to me if only the values 0, 128 and 255
should be used or any value from 0-255.
What's clear to me is that restart handlers to reset a specific hw
block
should be called before the restart handler that resets the whole
system.
The 192 seems to be used because there are other default restart
handlers
that are using a prio of 128. See for example the commit that
changed the
syscon-reboot prio from 128 to 192:
b81180b3fd48 power: reset: adjust priority of simple syscon reboot
driver
But were are here not talking about syscon handler but the others. Now
you will be ahead of them.
Yes, I know that. My point was that the platforms were either not
using the
mmc-pwrseq-emmc or their system restart handler already had a lower
priority
but that is not true for at least rk3288-veyron as you said.
So probably the 192 value was chosen because is in the middle of
128 and
255 but it seems to me a rather arbitrary value and I would prefer
it to
be documented in some place.
Effectively, now the emmc handler will be executed before their
handlers... is it an issue? Maybe some testing on these platforms is
necessary?
I don't think is an issue, the reason why I chose 255 is that it is
a documented value in the kernel-doc and since is the highest prio,
it makes sure the eMMC will be reset before any system restart
handler.
Also, the pwrseq_emmc driver is only used in platforms whose SoC ROM
can either leave the eMMC in an unknown state so the kernel needs to
hw reset the eMMC or does not have a reset logic so it can only read
from an eMMC if is in a known state (i.e: after a reset from kernel).
I think at least one platform may be affected because it used
mmc-pwrseq-emmc and gpio-restart.
Look at rk3288-veyron.dtsi.
Both of restart handlers had the priority of 129 which means that the
order of execution depends on probing sequence. Now you will make the
sequence strict - first mmc then gpio.
The behavior is going to change indeed in that board but no due probe
order but because the gpio-restart handler dev node has priority = <200>
which overrides the default 129 in the gpio-restart driver.
So before $SUBJECT the eMMC restart handler was not executed but now it
will be after this change.
You seems convinced that this is not a problem... I don't know. I would
prefer test this on affected platforms before risking to break them.
It's annoying if fix for one SoC breaks another.
Agreed.
Since the current mmc_pwrseq_emmc_reset_nb notifier priority is 129,
eMMC reset will not work if one of the platforms you mentioned needs
this since the system restart handler with prio 192 will be executed
before the eMMC one, leaving the eMMC in an unknown state on reboot.
And now you will "fix this" by making eMMC working correctly. So let's
make it straight:
1. Previously the eMMC could be left on these platforms in an unknown
state (because emmc handler was not executed).
2. No one complained! Which could mean that in fact this was working
fine...
3. Now you will change it.
4. Maybe someone will complain?
Just test it (or get an ack/tested tag). That's all what is needed.
Yes, I never meant that the patch should be merged without testing...
And $SUBJECT should not cause any regressions for the platforms that
are currently using the pwrseq_emmc, since the restart handler was
already being called before the system restart handler so bumping
the priority should not cause any effect.
I found at least one platform where the sequence *might* change. There
could be more of them.
Agreed, I missed that rk3288-veyron is using a restart handler with
higher
priority and could be other boards too as you said.
Let's see what is Marek's opinion since he added the pwrseq_emmc support
and also what Ulf thinks about always doing a eMMC reset before reboot.
I can't think how doing a eMMC card reset before reboot could affect a
board but you are right that we don't know without testing.
git grep result shows:
======
git grep mmc-pwrseq-emmc
Documentation/devicetree/bindings/mmc/mmc-pwrseq-emmc.txt:- compatible
: contains "mmc-pwrseq-emmc".
Documentation/devicetree/bindings/mmc/mmc-pwrseq-emmc.txt: compatible
= "mmc-pwrseq-emmc";
arch/arm/boot/dts/am335x-sl50.dts: compatible =
"mmc-pwrseq-emmc";
arch/arm/boot/dts/exynos4412-odroid-common.dtsi: compatible =
"mmc-pwrseq-emmc";
arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi: compatible =
"mmc-pwrseq-emmc";
arch/arm/boot/dts/rk3288-veyron.dtsi: compatible =
"mmc-pwrseq-emmc";
arch/arm64/boot/dts/rockchip/rk3368-r88.dts: compatible =
"mmc-pwrseq-emmc";
drivers/mmc/core/pwrseq.c: .compatible = "mmc-pwrseq-emmc",
=====
So apart from exynos, there are rk3xxx and am335x which used
pwrseq-emmc-restart. So lets wait for the feedback from these guys as
well.
Thanks.
The priority was initially chosen in such a way to do the emmc reset
just before performing system reboot. I wanted let other possible
handlers potentially use mmc if needed (although there is no such case
atm). Now it turns that this approach was not the best idea, because
there are other board-specific restart handlers with higher priority
than the default I was using. IMHO the change proposed by Javier seems
to be the best solution for now. The other possibility would be to
entirely get rid of restart handler usage and wire this logic to
mmc_shutdown(). This makes sense from the logical point of view, but the
drawback of this solution is the lack of proper reset sequence in case
of emergency reboot (shutdown callbacks are not called on emergency reboot).
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/