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; 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-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |