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