[linux-sunxi] Re: [PATCH] mmc: pwrseq-simple: Add an optional post-power-on-delay

[Hans de Goede]

Hi,

On 23-06-16 11:33, Hans de Goede wrote:
> Hi,
>
> On 23-06-16 00:25, Rob Herring wrote:
>> On Wed, Jun 22, 2016 at 12:59 PM, Hans de Goede <hdegoede at redhat.com> wrote:
>>> Some devices need a while to boot their firmware after providing clks /
>>> de-asserting resets before they are ready to receive sdio commands.
>>>
>>> This commits adds a post-power-on-delay-ms devicetree property to
>>> mmc-pwrseq-simple for use with such devices.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>> ---
>>>  Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt | 2 ++
>>>  drivers/mmc/core/pwrseq_simple.c                            | 9 +++++++++
>>>  2 files changed, 11 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>>> index ce0e767..e254368 100644
>>> --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>>> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>>> @@ -16,6 +16,8 @@ Optional properties:
>>>    See ../clocks/clock-bindings.txt for details.
>>>  - clock-names : Must include the following entry:
>>>    "ext_clock" (External clock provided to the card).
>>> +- post-power-on-delay-ms : Delay in ms after powering the card and
>>> +       de-asserting the reset-gpios (if any)
>>
>> Presumably you need this delay post any reset, not just after power on
>
> mmc-pwrseq is only about doing power-on / off, not about providing
> reset functionality.
>
>> if you are waiting for firmware to boot. So the name is not all that
>> clear. How about a "reset-timing-ms" property that takes 3 values for
>> pre-assert time (normally 0), assertion time, post assert time. Of
>> course, I can still think of ways that breaks like when in this
>> sequence do clocks need to be turned on.
>
> If you look at bindings/mmc/mmc-pwrseq-simple.txt out side of
> the diff context it contains:
>
> - reset-gpios : contains a list of GPIO specifiers. The reset GPIOs are asserted
>         at initialization and prior we start the power up procedure of the card.
>         They will be de-asserted right after the power has been provided to the
>         card.
> - clocks : Must contain an entry for the entry in clock-names.
>   See ../clocks/clock-bindings.txt for details.
> - clock-names : Must include the following entry:
>   "ext_clock" (External clock provided to the card).
>
> Notice how the existing docs talk about a power-up procedure (which matches
> the pwrseq name and purpose of these bindings).
>
> I actually started with calling the property "post-reset-delay-ms", but then
> I realized that what we really want is the ability to specify a time to
> wait (for e.g. firmware to boot) after completing the power-up procedure
> (and before starting the probe). The power-up procedure currently can
> also includes enabling external clocks to the sdio device (if any) and
> enabling regulators, so having a "reset-timing-ms" property does not
> seem right, as that would suggest it is ok to do the wait after deasserting
> reset, but before e.g. enabling external clocks. Where what we really
> want is to enable all necessary resources (or iow complete the powerup
> procedure) and then wait.
>
> You're right that in some cases more complicated timings may be necessary,
> but that can get really complicated like e.g.: enable regulator1, wait 10 ms,
> enable regulator2, wait 15ms, enable external clock1, ...
>
> And such complex timings fall outside of the scope of the mmc-pwrseq-simple
> binding, the idea being that for complex cases we do a device specific
> pwrseq binding, and then the smarts are in implementation of that specific
> pwrseq driver. As you've said yourself before we do not want to turn
> devicetree into a scripting language.
>
> However having a single wait after doing a straight forward power-up cycle
> seems to me like something which is appropriate for the mmc-pwrseq-simple
> binding.

Rob, can you please let us know if you're happy with my explanation above
and if not explain how you would like this binding to look instead ?

Thanks & Regards,

Hans