lkml.org 
[lkml]   [2015]   [Oct]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler
    From
    Date
    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,
    --
    Javier Martinez Canillas
    Open Source Group
    Samsung Research America


    \
     
     \ /
      Last update: 2015-10-22 05:01    [W:3.289 / U:0.892 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site