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