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

[Doug Anderson]

Krzysztof,

On Wed, Oct 21, 2015 at 6:43 PM, Krzysztof Kozlowski
<k.kozlowski@xxxxxxxxxxx> 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@xxxxxxxxxxxx>


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

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

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