Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Javier Martinez Canillas]

Hello Ulf,

On 10/27/2015 11:10 AM, Ulf Hansson wrote:
> On 21 October 2015 at 17: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 
>> Tested-by: Markus Reichl 
>> Tested-by: Anand Moon 
>> Reviewed-by: Alim Akhtar 
>>
>> ---
>> 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
> 
> So it seems like there have been a good discussion around this. I
> don't have any objections, but is more concerned about potential
> regressions.
>

Yes, there was a lot of discussion indeed but it seems we all agree
on the approach and that the patch should not land before having a
lot of testing.
 
> I have queued it up for next (4.4) so we get some testing in
> linux-next. If anyone have issues, please report them.
>

great, some weeks sitting in -next to let the CI infrastructure to
play with it seems reasonable to me. Thanks a lot!
 
> Kind regards
> Uffe
>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Javier Martinez Canillas]

Hello Ulf,

On 10/27/2015 11:10 AM, Ulf Hansson wrote:
> On 21 October 2015 at 17: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 
>> Tested-by: Markus Reichl 
>> Tested-by: Anand Moon 
>> Reviewed-by: Alim Akhtar 
>>
>> ---
>> 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
> 
> So it seems like there have been a good discussion around this. I
> don't have any objections, but is more concerned about potential
> regressions.
>

Yes, there was a lot of discussion indeed but it seems we all agree
on the approach and that the patch should not land before having a
lot of testing.
 
> I have queued it up for next (4.4) so we get some testing in
> linux-next. If anyone have issues, please report them.
>

great, some weeks sitting in -next to let the CI infrastructure to
play with it seems reasonable to me. Thanks a lot!
 
> Kind regards
> Uffe
>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Ulf Hansson]

On 21 October 2015 at 17: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 
> Tested-by: Markus Reichl 
> Tested-by: Anand Moon 
> Reviewed-by: Alim Akhtar 
>
> ---
> 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

So it seems like there have been a good discussion around this. I
don't have any objections, but is more concerned about potential
regressions.

I have queued it up for next (4.4) so we get some testing in
linux-next. If anyone have issues, please report them.

Kind regards
Uffe

>
>  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;
> register_restart_handler(>reset_nb);
>
> pwrseq->pwrseq.ops = _pwrseq_emmc_ops;
> --
> 2.4.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Ulf Hansson]

On 21 October 2015 at 17: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 
> Tested-by: Markus Reichl 
> Tested-by: Anand Moon 
> Reviewed-by: Alim Akhtar 
>
> ---
> 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

So it seems like there have been a good discussion around this. I
don't have any objections, but is more concerned about potential
regressions.

I have queued it up for next (4.4) so we get some testing in
linux-next. If anyone have issues, please report them.

Kind regards
Uffe

>
>  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;
> register_restart_handler(>reset_nb);
>
> pwrseq->pwrseq.ops = _pwrseq_emmc_ops;
> --
> 2.4.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Alim Akhtar]

On 10/22/2015 09:04 PM, Doug Anderson wrote:

Krzysztof,

On Wed, Oct 21, 2015 at 6:43 PM, Krzysztof Kozlowski
 wrote:

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.

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.


Assuming I'm understanding things properly, veyron isn't using 129 as
a priority.  In the dts file:

 gpio-restart {
 compatible = "gpio-restart";
 gpios = < 13 GPIO_ACTIVE_HIGH>;
 pinctrl-names = "default";
 pinctrl-0 = <_warm_reset_h>;
 priority = <200>;
 };

...so it overrides the default 129 with 200.  Ah, but Javier already
pointed that out in his reply.


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?


On veyron boards the reset shouldn't hurt.  The eMMC reset will
actually get asserted at reset anyway since the reset will reset GPIO
states and the default GPIO state for the eMMC line asserts reset.

OK, I just picked this onto Heiko's somewhat "stable-tree"
(v4.3-rc3-876-g6509232) from
.  I put printouts in
__mmc_pwrseq_emmc_reset() to confirm it was getting called.  I then
rebooted.  I then saw:

[   36.175732] reboot: Restarting system
[   36.179400] DOUG: resetting emmc
[   36.182829] DOUG: resetting emmc done

...and the reboot worked normally (which means that the GPIO restart
got called since otherwise I would have gotten TPM errors).

So I'd say that for rk3288-veyron-jerry:

Tested-by: Douglas Anderson 


Thank you!


Note that personally I would only choose the "highest" priority as an
absolute last resort.  Leaving a little extra slack in there means
that when the next person comes up with a really good reason to run
before you do that they can do it without changing your code.  All
good BASIC programmers know to skip "10" in their first version for
just this reason.  ;)

If I were to pick a number, I suppose I'd pick something like "220",
but that's pretty arbitrary.  I would have picked 200 except that it
appears that would race with veyron's choice.

-Doug


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Alim Akhtar]

On 10/22/2015 09:04 PM, Doug Anderson wrote:

Krzysztof,

On Wed, Oct 21, 2015 at 6:43 PM, Krzysztof Kozlowski
 wrote:

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.

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.


Assuming I'm understanding things properly, veyron isn't using 129 as
a priority.  In the dts file:

 gpio-restart {
 compatible = "gpio-restart";
 gpios = < 13 GPIO_ACTIVE_HIGH>;
 pinctrl-names = "default";
 pinctrl-0 = <_warm_reset_h>;
 priority = <200>;
 };

...so it overrides the default 129 with 200.  Ah, but Javier already
pointed that out in his reply.


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?


On veyron boards the reset shouldn't hurt.  The eMMC reset will
actually get asserted at reset anyway since the reset will reset GPIO
states and the default GPIO state for the eMMC line asserts reset.

OK, I just picked this onto Heiko's somewhat "stable-tree"
(v4.3-rc3-876-g6509232) from
.  I put printouts in
__mmc_pwrseq_emmc_reset() to confirm it was getting called.  I then
rebooted.  I then saw:

[   36.175732] reboot: Restarting system
[   36.179400] DOUG: resetting emmc
[   36.182829] DOUG: resetting emmc done

...and the reboot worked normally (which means that the GPIO restart
got called since otherwise I would have gotten TPM errors).

So I'd say that for rk3288-veyron-jerry:

Tested-by: Douglas Anderson 


Thank you!


Note that personally I would only choose the "highest" priority as an
absolute last resort.  Leaving a little extra slack in there means
that when the next person comes up with a really good reason to run
before you do that they can do it without changing your code.  All
good BASIC programmers know to skip "10" in their first version for
just this reason.  ;)

If I were to pick a number, I suppose I'd pick something like "220",
but that's pretty arbitrary.  I would have picked 200 except that it
appears that would race with veyron's choice.

-Doug


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Javier Martinez Canillas]

Hello Doug,

On 10/22/2015 07:33 PM, Doug Anderson wrote:
> On Thu, Oct 22, 2015 at 9:07 AM, Javier Martinez Canillas

[snip]

>>
>> Do you know why the priority 200 was chosen for veyron gpi-restart ooi?
> 
> In David Riley's original patch the example had 200:
> https://patchwork.kernel.org/patch/4784611/
> 
> In the ChromeOS 3.14 kernel tree I believe we're still using the old
> patch (we still have /bits/ 8).  ...it looks like I'm the one who
> originally added it to the veyron dts file and I set it to 200, so I'd
> presume that I just copied the example and called it "good enough".
>

I see, thanks for the explanation. I asked because I noticed that the
gpio-restart handler default priority was 129 and I didn't find other
restart handler used for this board with a prio > 129 so at least in
mainline, the priority 200 should not be necessary.

But now I see that it was indeed 128 but was bumped to 129 in commit:

bcd56fe1aa97 ("power: reset: gpio-restart: increase priority slightly")

which explains why the priority 200 was in the veyron DTS even when is
not needed anymore after that commit.
 
> I'm sure the upstream dts just used the number from the ChromeOS 3.14 tree...
> 
> Note that the GPIO-restart definitely need to be higher priorities
> than others in the system.  The two I know of off the top of my head
> are the "dw watchdog" and the one in the CRU.  The "dw watchdog" has a
> priority of 128 and so does the one in "rockchip/clk.c".  Hrm,
> actually, the Rockchip-specific one should probably have its priority
> bumped up since it seems better not to just randomly pick between
> these two...

Agreed about bumping the prio for the rockchip specific restart handler.

> 
> 
> -Doug
> --

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Doug Anderson]

Hi,

On Thu, Oct 22, 2015 at 9:07 AM, Javier Martinez Canillas
 wrote:
>> Note that personally I would only choose the "highest" priority as an
>> absolute last resort.  Leaving a little extra slack in there means
>> that when the next person comes up with a really good reason to run
>> before you do that they can do it without changing your code.  All
>> good BASIC programmers know to skip "10" in their first version for
>> just this reason.  ;)
>>
>> If I were to pick a number, I suppose I'd pick something like "220",
>> but that's pretty arbitrary.  I would have picked 200 except that it
>> appears that would race with veyron's choice.
>>
>
> Yes, I actually gave some thought about choosing a number since I didn't
> want to come with another arbitrary one. That's why I tried to understand
> the policy as I mentioned before but I didn't find anything besides the
> values listed in the register_restart_handler() doc: 0, 128 and 255.
>
> It seems that most default system restart handlers use 128 and that's
> the reason why gpio-restart and mmc-pwrseq-emmc use 129 and other restart
> handlers that can be registered via DT use 192 (which is in the middle of
> 128 and 255).
>
> So I actually thought to use a number in between 192 and 255 (i.e: 220)
> but then there could be another platform that uses 221 instead of 200
> so eMMC restart won't work there. That's why I finally chose the highest.
>
> Do you know why the priority 200 was chosen for veyron gpi-restart ooi?

In David Riley's original patch the example had 200:
https://patchwork.kernel.org/patch/4784611/

In the ChromeOS 3.14 kernel tree I believe we're still using the old
patch (we still have /bits/ 8).  ...it looks like I'm the one who
originally added it to the veyron dts file and I set it to 200, so I'd
presume that I just copied the example and called it "good enough".

I'm sure the upstream dts just used the number from the ChromeOS 3.14 tree...

Note that the GPIO-restart definitely need to be higher priorities
than others in the system.  The two I know of off the top of my head
are the "dw watchdog" and the one in the CRU.  The "dw watchdog" has a
priority of 128 and so does the one in "rockchip/clk.c".  Hrm,
actually, the Rockchip-specific one should probably have its priority
bumped up since it seems better not to just randomly pick between
these two...


-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Javier Martinez Canillas]

Hello Doug,

On 10/22/2015 05:34 PM, Doug Anderson wrote:
> Krzysztof,
> 
> On Wed, Oct 21, 2015 at 6:43 PM, Krzysztof Kozlowski
>  wrote:
>> 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.
>>
>> 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.
> 
> Assuming I'm understanding things properly, veyron isn't using 129 as
> a priority.  In the dts file:
> 
> gpio-restart {
> compatible = "gpio-restart";
> gpios = < 13 GPIO_ACTIVE_HIGH>;
> pinctrl-names = "default";
> pinctrl-0 = <_warm_reset_h>;
> priority = <200>;
> };
> 
> ...so it overrides the default 129 with 200.  Ah, but Javier already
> pointed that out in his reply.
> 
>>> 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?
> 
> On veyron boards the reset shouldn't hurt.  The eMMC reset will
> actually get asserted at reset anyway since the reset will reset GPIO
> states and the default GPIO state for the eMMC line asserts reset.
>

Exactly, that was my point. Either the board is wired to do a eMMC reset
on reboot (like veyron), the SoC ROM bootloader has some logic to reset
the eMMC or the boards requires the kernel to do a eMMC reset so the hw
is in a known state to read from the eMMC on reboot (like Odroids).

So that's why I was arguing that it's very unlikely that doing an eMMC
reset could cause issues in other boards... but Krzysztof is correct
that you can't be sure without testing.
 
> OK, I just picked this onto Heiko's somewhat "stable-tree"
> (v4.3-rc3-876-g6509232) from
> .  I put printouts in
> __mmc_pwrseq_emmc_reset() to confirm it was getting called.  I then
> rebooted.  I then saw:
> 
> [   36.175732] reboot: Restarting system
> [   36.179400] DOUG: resetting emmc
> [   36.182829] DOUG: resetting emmc done
> 
> ...and the reboot worked normally (which means that the GPIO restart
> got called since otherwise I would have gotten TPM errors).
> 
> So I'd say that for rk3288-veyron-jerry:
> 
> Tested-by: Douglas Anderson 
> 

Thanks a lot for testing!

> 
> Note that personally I would only choose the "highest" priority as an
> absolute last resort.  Leaving a little extra slack in there means
> that when the next person comes up with a really good reason to run
> before you do that they can do it without changing your code.  All
> good BASIC programmers know to skip "10" in their first version for
> just this reason.  ;)
>
> If I were to pick a number, I suppose I'd pick something like "220",
> but that's pretty arbitrary.  I would have picked 200 except that it
> appears that would race with veyron's choice.
>

Yes, I actually gave some thought about choosing a number since I didn't
want to come with another arbitrary one. That's why I tried to understand
the policy as I mentioned before but I didn't find anything besides the
values listed in the register_restart_handler() doc: 0, 128 and 255.

It seems that most default system restart handlers use 128 and that's
the reason why gpio-restart and mmc-pwrseq-emmc use 129 and other restart
handlers that can be registered via DT use 192 (which is in the middle of
128 and 255).

So I actually thought to use a number in between 192 and 255 (i.e: 220)
but then there could be another platform that uses 221 instead of 200
so eMMC restart won't work there. That's why I finally chose the highest.

Do you know why the priority 200 was chosen for veyron gpi-restart ooi?

> -Doug
> --

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Heiko Stübner]

Am Donnerstag, 22. Oktober 2015, 08:34:38 schrieb Doug Anderson:
> Note that personally I would only choose the "highest" priority as an
> absolute last resort.  Leaving a little extra slack in there means
> that when the next person comes up with a really good reason to run
> before you do that they can do it without changing your code. 

just to reiterate, restart-handlers are generally not meant as "things to do 
before restart", but "are supposed to restart the system, nothing else" [0].

Just in this case there hasn't been a better solution found for the needed 
reset even in emergency-reboots ... but this misappropriation of restart-
handlers should not spread into further realms, so there shouldn't be a "next 
person" ;-) .


[0] http://permalink.gmane.org/gmane.linux.kernel/1968815



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Doug Anderson]

Krzysztof,

On Wed, Oct 21, 2015 at 6:43 PM, Krzysztof Kozlowski
 wrote:
> 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.
>
> 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.

Assuming I'm understanding things properly, veyron isn't using 129 as
a priority.  In the dts file:

gpio-restart {
compatible = "gpio-restart";
gpios = < 13 GPIO_ACTIVE_HIGH>;
pinctrl-names = "default";
pinctrl-0 = <_warm_reset_h>;
priority = <200>;
};

...so it overrides the default 129 with 200.  Ah, but Javier already
pointed that out in his reply.

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

On veyron boards the reset shouldn't hurt.  The eMMC reset will
actually get asserted at reset anyway since the reset will reset GPIO
states and the default GPIO state for the eMMC line asserts reset.

OK, I just picked this onto Heiko's somewhat "stable-tree"
(v4.3-rc3-876-g6509232) from
.  I put printouts in
__mmc_pwrseq_emmc_reset() to confirm it was getting called.  I then
rebooted.  I then saw:

[   36.175732] reboot: Restarting system
[   36.179400] DOUG: resetting emmc
[   36.182829] DOUG: resetting emmc done

...and the reboot worked normally (which means that the GPIO restart
got called since otherwise I would have gotten TPM errors).

So I'd say that for rk3288-veyron-jerry:

Tested-by: Douglas Anderson 


Note that personally I would only choose the "highest" priority as an
absolute last resort.  Leaving a little extra slack in there means
that when the next person comes up with a really good reason to run
before you do that they can do it without changing your code.  All
good BASIC programmers know to skip "10" in their first version for
just this reason.  ;)

If I were to pick a number, I suppose I'd pick something like "220",
but that's pretty arbitrary.  I would have picked 200 except that it
appears that would race with veyron's choice.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Javier Martinez Canillas]

Hello Marek,

On 10/22/2015 12:07 PM, Marek Szyprowski wrote:
> On 2015-10-22 06:14, Alim Akhtar wrote:
>> On 10/22/2015 08:22 AM, Javier Martinez Canillas wrote:
>>> On 10/22/2015 03:43 AM, Krzysztof Kozlowski wrote:
 On 22.10.2015 10:20, Javier Martinez Canillas wrote:> Hello Krzysztof,

[snip]

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

Thanks a lot for your feedback, I'm glad that you agree with the change then.
 
> Best regards

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Marek Szyprowski]

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 
Tested-by: Markus Reichl 
Tested-by: Anand Moon 
Reviewed-by: Alim Akhtar 

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

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Anand Moon]

Hi Javier,

On 22 October 2015 at 14:06, Javier Martinez Canillas
 wrote:
> Hello Anand,
>
> On 10/22/2015 07:03 AM, Anand Moon wrote:
>> Hi Javier,
>>
>> On 22 October 2015 at 08:22, 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 
>>> Tested-by: Markus Reichl 
>>> Tested-by: Anand Moon 
>>> Reviewed-by: Alim Akhtar 
>>>
>>> ---
>>> 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 

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Javier Martinez Canillas]

Hello Anand,

On 10/22/2015 07:03 AM, Anand Moon wrote:
> Hi Javier,
> 
> On 22 October 2015 at 08:22, 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 
>> Tested-by: Markus Reichl 
>> Tested-by: Anand Moon 
>> Reviewed-by: Alim Akhtar 
>>
>> ---
>> 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

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Javier Martinez Canillas]

Hello Anand,

On 10/22/2015 07:03 AM, Anand Moon wrote:
> Hi Javier,
> 
> On 22 October 2015 at 08:22, 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 
>> Tested-by: Markus Reichl 
>> Tested-by: Anand Moon 
>> Reviewed-by: Alim Akhtar 
>>
>> ---
>> 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 

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Doug Anderson]

Hi,

On Thu, Oct 22, 2015 at 9:07 AM, Javier Martinez Canillas
 wrote:
>> Note that personally I would only choose the "highest" priority as an
>> absolute last resort.  Leaving a little extra slack in there means
>> that when the next person comes up with a really good reason to run
>> before you do that they can do it without changing your code.  All
>> good BASIC programmers know to skip "10" in their first version for
>> just this reason.  ;)
>>
>> If I were to pick a number, I suppose I'd pick something like "220",
>> but that's pretty arbitrary.  I would have picked 200 except that it
>> appears that would race with veyron's choice.
>>
>
> Yes, I actually gave some thought about choosing a number since I didn't
> want to come with another arbitrary one. That's why I tried to understand
> the policy as I mentioned before but I didn't find anything besides the
> values listed in the register_restart_handler() doc: 0, 128 and 255.
>
> It seems that most default system restart handlers use 128 and that's
> the reason why gpio-restart and mmc-pwrseq-emmc use 129 and other restart
> handlers that can be registered via DT use 192 (which is in the middle of
> 128 and 255).
>
> So I actually thought to use a number in between 192 and 255 (i.e: 220)
> but then there could be another platform that uses 221 instead of 200
> so eMMC restart won't work there. That's why I finally chose the highest.
>
> Do you know why the priority 200 was chosen for veyron gpi-restart ooi?

In David Riley's original patch the example had 200:
https://patchwork.kernel.org/patch/4784611/

In the ChromeOS 3.14 kernel tree I believe we're still using the old
patch (we still have /bits/ 8).  ...it looks like I'm the one who
originally added it to the veyron dts file and I set it to 200, so I'd
presume that I just copied the example and called it "good enough".

I'm sure the upstream dts just used the number from the ChromeOS 3.14 tree...

Note that the GPIO-restart definitely need to be higher priorities
than others in the system.  The two I know of off the top of my head
are the "dw watchdog" and the one in the CRU.  The "dw watchdog" has a
priority of 128 and so does the one in "rockchip/clk.c".  Hrm,
actually, the Rockchip-specific one should probably have its priority
bumped up since it seems better not to just randomly pick between
these two...


-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Javier Martinez Canillas]

Hello Doug,

On 10/22/2015 07:33 PM, Doug Anderson wrote:
> On Thu, Oct 22, 2015 at 9:07 AM, Javier Martinez Canillas

[snip]

>>
>> Do you know why the priority 200 was chosen for veyron gpi-restart ooi?
> 
> In David Riley's original patch the example had 200:
> https://patchwork.kernel.org/patch/4784611/
> 
> In the ChromeOS 3.14 kernel tree I believe we're still using the old
> patch (we still have /bits/ 8).  ...it looks like I'm the one who
> originally added it to the veyron dts file and I set it to 200, so I'd
> presume that I just copied the example and called it "good enough".
>

I see, thanks for the explanation. I asked because I noticed that the
gpio-restart handler default priority was 129 and I didn't find other
restart handler used for this board with a prio > 129 so at least in
mainline, the priority 200 should not be necessary.

But now I see that it was indeed 128 but was bumped to 129 in commit:

bcd56fe1aa97 ("power: reset: gpio-restart: increase priority slightly")

which explains why the priority 200 was in the veyron DTS even when is
not needed anymore after that commit.
 
> I'm sure the upstream dts just used the number from the ChromeOS 3.14 tree...
> 
> Note that the GPIO-restart definitely need to be higher priorities
> than others in the system.  The two I know of off the top of my head
> are the "dw watchdog" and the one in the CRU.  The "dw watchdog" has a
> priority of 128 and so does the one in "rockchip/clk.c".  Hrm,
> actually, the Rockchip-specific one should probably have its priority
> bumped up since it seems better not to just randomly pick between
> these two...

Agreed about bumping the prio for the rockchip specific restart handler.

> 
> 
> -Doug
> --

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Javier Martinez Canillas]

Hello Marek,

On 10/22/2015 12:07 PM, Marek Szyprowski wrote:
> On 2015-10-22 06:14, Alim Akhtar wrote:
>> On 10/22/2015 08:22 AM, Javier Martinez Canillas wrote:
>>> On 10/22/2015 03:43 AM, Krzysztof Kozlowski wrote:
 On 22.10.2015 10:20, Javier Martinez Canillas wrote:> Hello Krzysztof,

[snip]

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

Thanks a lot for your feedback, I'm glad that you agree with the change then.
 
> Best regards

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Marek Szyprowski]

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 
Tested-by: Markus Reichl 
Tested-by: Anand Moon 
Reviewed-by: Alim Akhtar 

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

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Anand Moon]

Hi Javier,

On 22 October 2015 at 14:06, Javier Martinez Canillas
 wrote:
> Hello Anand,
>
> On 10/22/2015 07:03 AM, Anand Moon wrote:
>> Hi Javier,
>>
>> On 22 October 2015 at 08:22, 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 
>>> Tested-by: Markus Reichl 
>>> Tested-by: Anand Moon 
>>> Reviewed-by: Alim Akhtar 
>>>
>>> ---
>>> 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 

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Heiko Stübner]

Am Donnerstag, 22. Oktober 2015, 08:34:38 schrieb Doug Anderson:
> Note that personally I would only choose the "highest" priority as an
> absolute last resort.  Leaving a little extra slack in there means
> that when the next person comes up with a really good reason to run
> before you do that they can do it without changing your code. 

just to reiterate, restart-handlers are generally not meant as "things to do 
before restart", but "are supposed to restart the system, nothing else" [0].

Just in this case there hasn't been a better solution found for the needed 
reset even in emergency-reboots ... but this misappropriation of restart-
handlers should not spread into further realms, so there shouldn't be a "next 
person" ;-) .


[0] http://permalink.gmane.org/gmane.linux.kernel/1968815



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Doug Anderson]

Krzysztof,

On Wed, Oct 21, 2015 at 6:43 PM, Krzysztof Kozlowski
 wrote:
> 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.
>
> 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.

Assuming I'm understanding things properly, veyron isn't using 129 as
a priority.  In the dts file:

gpio-restart {
compatible = "gpio-restart";
gpios = < 13 GPIO_ACTIVE_HIGH>;
pinctrl-names = "default";
pinctrl-0 = <_warm_reset_h>;
priority = <200>;
};

...so it overrides the default 129 with 200.  Ah, but Javier already
pointed that out in his reply.

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

On veyron boards the reset shouldn't hurt.  The eMMC reset will
actually get asserted at reset anyway since the reset will reset GPIO
states and the default GPIO state for the eMMC line asserts reset.

OK, I just picked this onto Heiko's somewhat "stable-tree"
(v4.3-rc3-876-g6509232) from
.  I put printouts in
__mmc_pwrseq_emmc_reset() to confirm it was getting called.  I then
rebooted.  I then saw:

[   36.175732] reboot: Restarting system
[   36.179400] DOUG: resetting emmc
[   36.182829] DOUG: resetting emmc done

...and the reboot worked normally (which means that the GPIO restart
got called since otherwise I would have gotten TPM errors).

So I'd say that for rk3288-veyron-jerry:

Tested-by: Douglas Anderson 


Note that personally I would only choose the "highest" priority as an
absolute last resort.  Leaving a little extra slack in there means
that when the next person comes up with a really good reason to run
before you do that they can do it without changing your code.  All
good BASIC programmers know to skip "10" in their first version for
just this reason.  ;)

If I were to pick a number, I suppose I'd pick something like "220",
but that's pretty arbitrary.  I would have picked 200 except that it
appears that would race with veyron's choice.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Javier Martinez Canillas]

Hello Doug,

On 10/22/2015 05:34 PM, Doug Anderson wrote:
> Krzysztof,
> 
> On Wed, Oct 21, 2015 at 6:43 PM, Krzysztof Kozlowski
>  wrote:
>> 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.
>>
>> 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.
> 
> Assuming I'm understanding things properly, veyron isn't using 129 as
> a priority.  In the dts file:
> 
> gpio-restart {
> compatible = "gpio-restart";
> gpios = < 13 GPIO_ACTIVE_HIGH>;
> pinctrl-names = "default";
> pinctrl-0 = <_warm_reset_h>;
> priority = <200>;
> };
> 
> ...so it overrides the default 129 with 200.  Ah, but Javier already
> pointed that out in his reply.
> 
>>> 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?
> 
> On veyron boards the reset shouldn't hurt.  The eMMC reset will
> actually get asserted at reset anyway since the reset will reset GPIO
> states and the default GPIO state for the eMMC line asserts reset.
>

Exactly, that was my point. Either the board is wired to do a eMMC reset
on reboot (like veyron), the SoC ROM bootloader has some logic to reset
the eMMC or the boards requires the kernel to do a eMMC reset so the hw
is in a known state to read from the eMMC on reboot (like Odroids).

So that's why I was arguing that it's very unlikely that doing an eMMC
reset could cause issues in other boards... but Krzysztof is correct
that you can't be sure without testing.
 
> OK, I just picked this onto Heiko's somewhat "stable-tree"
> (v4.3-rc3-876-g6509232) from
> .  I put printouts in
> __mmc_pwrseq_emmc_reset() to confirm it was getting called.  I then
> rebooted.  I then saw:
> 
> [   36.175732] reboot: Restarting system
> [   36.179400] DOUG: resetting emmc
> [   36.182829] DOUG: resetting emmc done
> 
> ...and the reboot worked normally (which means that the GPIO restart
> got called since otherwise I would have gotten TPM errors).
> 
> So I'd say that for rk3288-veyron-jerry:
> 
> Tested-by: Douglas Anderson 
> 

Thanks a lot for testing!

> 
> Note that personally I would only choose the "highest" priority as an
> absolute last resort.  Leaving a little extra slack in there means
> that when the next person comes up with a really good reason to run
> before you do that they can do it without changing your code.  All
> good BASIC programmers know to skip "10" in their first version for
> just this reason.  ;)
>
> If I were to pick a number, I suppose I'd pick something like "220",
> but that's pretty arbitrary.  I would have picked 200 except that it
> appears that would race with veyron's choice.
>

Yes, I actually gave some thought about choosing a number since I didn't
want to come with another arbitrary one. That's why I tried to understand
the policy as I mentioned before but I didn't find anything besides the
values listed in the register_restart_handler() doc: 0, 128 and 255.

It seems that most default system restart handlers use 128 and that's
the reason why gpio-restart and mmc-pwrseq-emmc use 129 and other restart
handlers that can be registered via DT use 192 (which is in the middle of
128 and 255).

So I actually thought to use a number in between 192 and 255 (i.e: 220)
but then there could be another platform that uses 221 instead of 200
so eMMC restart won't work there. That's why I finally chose the highest.

Do you know why the priority 200 was chosen for veyron gpi-restart ooi?

> -Doug
> --

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Anand Moon]

Hi Javier,

On 22 October 2015 at 08:22, 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 
> Tested-by: Markus Reichl 
> Tested-by: Anand Moon 
> Reviewed-by: Alim Akhtar 
>
> ---
> 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 

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Alim Akhtar]

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 
Tested-by: Markus Reichl 
Tested-by: Anand Moon 
Reviewed-by: Alim Akhtar 

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

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Javier Martinez Canillas]

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 
 Tested-by: Markus Reichl 
 Tested-by: Anand Moon 
 Reviewed-by: Alim Akhtar 

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

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Krzysztof Kozlowski]

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 
>>> Tested-by: Markus Reichl 
>>> Tested-by: Anand Moon 
>>> Reviewed-by: Alim Akhtar 
>>>
>>> ---
>>> 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.

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

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Javier Martinez Canillas]

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 
>> Tested-by: Markus Reichl 
>> Tested-by: Anand Moon 
>> Reviewed-by: Alim Akhtar 
>>
>> ---
>> 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

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 

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Krzysztof Kozlowski]

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 
> Tested-by: Markus Reichl 
> Tested-by: Anand Moon 
> Reviewed-by: Alim Akhtar 
> 
> ---
> 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).

I guess they chose the "192" priority on purpose.

Effectively, now the emmc handler will be executed before their
handlers... is it an issue? Maybe some testing on these platforms is
necessary?

Best regards,
Krzysztof

>   register_restart_handler(>reset_nb);
>  
>   pwrseq->pwrseq.ops = _pwrseq_emmc_ops;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Anand Moon]

Hi Javier,

On 22 October 2015 at 08:22, 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 
> Tested-by: Markus Reichl 
> Tested-by: Anand Moon 
> Reviewed-by: Alim Akhtar 
>
> ---
> 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. 

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Krzysztof Kozlowski]

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 
>>> Tested-by: Markus Reichl 
>>> Tested-by: Anand Moon 
>>> Reviewed-by: Alim Akhtar 
>>>
>>> ---
>>> 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.

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

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Alim Akhtar]

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 
Tested-by: Markus Reichl 
Tested-by: Anand Moon 
Reviewed-by: Alim Akhtar 

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

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Javier Martinez Canillas]

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 
>> Tested-by: Markus Reichl 
>> Tested-by: Anand Moon 
>> Reviewed-by: Alim Akhtar 
>>
>> ---
>> 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

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 

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Krzysztof Kozlowski]

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 
> Tested-by: Markus Reichl 
> Tested-by: Anand Moon 
> Reviewed-by: Alim Akhtar 
> 
> ---
> 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).

I guess they chose the "192" priority on purpose.

Effectively, now the emmc handler will be executed before their
handlers... is it an issue? Maybe some testing on these platforms is
necessary?

Best regards,
Krzysztof

>   register_restart_handler(>reset_nb);
>  
>   pwrseq->pwrseq.ops = _pwrseq_emmc_ops;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Javier Martinez Canillas]

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 
 Tested-by: Markus Reichl 
 Tested-by: Anand Moon 
 Reviewed-by: Alim Akhtar 

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