Message-ID: <CAPDyKFoe7+M=ftQmMrP5O68TkuwkMSsoLuGrMa6N9ygew4eHPw@mail.gmail.com>
Date:   Mon, 23 Apr 2018 17:01:14 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Lukasz Majewski <lukma@...x.de>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Fabio Estevam <fabio.estevam@....com>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        Chanho Min <chanho.min@....com>, devicetree@...r.kernel.org,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        Stanislav Meduna <stanislav.meduna@...control.com>
Subject: Re: [PATCH] mmc: disable card sleep via device-tree

On 23 April 2018 at 16:11, Lukasz Majewski <lukma@...x.de> wrote:
> Hi Ulf,
>
>> On 23 April 2018 at 11:36, Lukasz Majewski <lukma@...x.de> wrote:
>> > Hi Ulf,
>> >
>> >> On 22 April 2018 at 23:31, Lukasz Majewski <lukma@...x.de> wrote:
>> >> > From: Stanislav Meduna <stanislav.meduna@...control.com>
>> >> >
>> >> > On a TQMa53 module the mmc_sleep leaves the eMMC card in a state
>> >> > that the imx53 rom boot code is unable to probe, resulting in
>> >> > reboot hanging. Add a device tree property to disable sleeping
>> >> > on suspend.
>> >> >
>> >> > For TQMa53 modules the exact commit to cause hang after reboot
>> >> > (v3.10 -> v3.11):
>> >> > commit 486fdbbc1483 ("mmc: core: Add shutdown callback for (e)MMC
>> >> > bus_ops")
>> >> >
>> >> > [The exact discussion can be found here:
>> >> > https://patchwork.kernel.org/patch/8881401/
>> >> > "i.MX53 restart via watchdog does not work"
>> >> >
>> >> > Signed-off-by: Stanislav Meduna <stanislav.meduna@...control.com>
>> >> > Signed-off-by: Lukasz Majewski <lukma@...x.de>
>> >> > ---
>> >> >  Documentation/devicetree/bindings/mmc/mmc-card.txt | 4 ++++
>> >> >  drivers/mmc/core/mmc.c                             | 7 +++++--
>> >> >  include/linux/mmc/card.h                           | 2 +-
>> >> >  3 files changed, 10 insertions(+), 3 deletions(-)
>> >> >
>> >> > diff --git a/Documentation/devicetree/bindings/mmc/mmc-card.txt
>> >> > b/Documentation/devicetree/bindings/mmc/mmc-card.txt index
>> >> > 8d2d71758907..c3ee151edd7c 100644 ---
>> >> > a/Documentation/devicetree/bindings/mmc/mmc-card.txt +++
>> >> > b/Documentation/devicetree/bindings/mmc/mmc-card.txt @@ -12,6
>> >> > +12,9 @@ Required properties: Optional properties:
>> >> >  -broken-hpi : Use this to indicate that the mmc-card has a
>> >> > broken hpi implementation, and that hpi should not be used
>> >> > +-no-sleep-on-suspend : Do not put the card to sleep when
>> >> > suspending.
>> >> > +              There are boards with bootloaders that are unable
>> >> > +              to probe such card when rebooting.
>> >> >
>> >> >  Example:
>> >> >
>> >> > @@ -26,5 +29,6 @@ Example:
>> >> >                 reg = <0>;
>> >> >                 compatible = "mmc-card";
>> >> >                 broken-hpi;
>> >> > +               no-sleep-on-suspend;
>> >> >         };
>> >> >  };
>> >> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> >> > index 208a762b87ef..a3b74b5c8893 100644
>> >> > --- a/drivers/mmc/core/mmc.c
>> >> > +++ b/drivers/mmc/core/mmc.c
>> >> > @@ -381,8 +381,11 @@ static int mmc_decode_ext_csd(struct
>> >> > mmc_card *card, u8 *ext_csd) }
>> >> >
>> >> >         np = mmc_of_find_child_device(card->host, 0);
>> >> > -       if (np && of_device_is_compatible(np, "mmc-card"))
>> >> > +       if (np && of_device_is_compatible(np, "mmc-card")) {
>> >> >                 broken_hpi = of_property_read_bool(np,
>> >> > "broken-hpi");
>> >> > +               card->no_sleep_on_suspend =
>> >> > +                       of_property_read_bool(np,
>> >> > "no-sleep-on-suspend");
>> >> > +       }
>> >> >         of_node_put(np);
>> >> >
>> >> >         /*
>> >> > @@ -1990,7 +1993,7 @@ static int _mmc_suspend(struct mmc_host
>> >> > *host, bool is_suspend) if (mmc_can_poweroff_notify(host->card)
>> >> > && ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE)
>> >> > || !is_suspend)) err = mmc_poweroff_notify(host->card,
>> >> > notify_type);
>> >> > -       else if (mmc_can_sleep(host->card))
>> >> > +       else if (mmc_can_sleep(host->card)
>> >> > && !host->card->no_sleep_on_suspend)
>> >>
>> >> No, this is wrong.
>> >>
>> >> This means that the mmc_power_off() a few lines below would start
>> >> to violate the eMMC spec.
>> >> That via powering off the card, without first
>> >> sending the sleep or power-off-notify command.
>> >> You are probably just lucky, as your particular eMMC card still
>> >> copes with this in-correct sequence (I know there are these kind
>> >> of cards).
>> >>
>> >> Well, what is also interesting in this regards, is what is
>> >> happening during mmc_power_off().
>> >
>> > The mmc_power_off() -> mmc_pwrseq_power_off() -> here the *pwrseq is
>> > null, so we do nothing with the power.
>> >
>> > In mmc_power_off() function we call mmc_set_initial_state(host);
>> >
>> >> Does your host driver cut power to VMMC and/or
>> >> VQMMC?
>> >
>> > The esdhci3 controller uses 'vmm-supply' which is a
>> > 'regulator-fixed' with 'regulator-always-on' attribute.
>>
>> Assume you mean vmmc-supply.
>>
>> Anyway, it seems a bit weird to have this regulator as fixed and
>> always on. Of course it's possible, but it's more common to have vqmmc
>> fixed and always on. Maybe double check the schematics.
>>
>> >
>> > It seems like the eMMC is always powered with 3.3V.
>>
>> What about vqmmc then?
>
> It seems like the vqmmc is not added/used in dts (imx53.dtsi). Only the
> vmmc-supply is used.

My point is, that perhaps DTS is wrong or too simplified.

Perhaps vmmc can be powered off/on?

>
>>
>> >
>> > Considering the above it seems like the card is still powered - the
>> > HW design also supports some backup powering (so the voltage from
>> > the board is not took immediately).
>>
>> I think you need to verify that the CMD5 (sleep) is actually sent and
>> successfully completed.
>>
>> In regards to that, do your host support MMC_CAP_WAIT_WHILE_BUSY?
>
> No. This flag is not supported (set) on this host.

Okay. One thing to remove from possible root causes.

>
>> If
>> not, there is a delay (specific to the card) after the CMD5.
>
> Yes, the delay is used (mmc.c Line 1890).
> The delay is 7ms (as read from card->ext_csd.sa_timeout).
>
> Increasing it by 10 (or 100) ms doesn't solve the problem.

Okay. To really be sure, I would also try 1000ms.

>
>>
>> So the support in your host driver for MMC_CAP_WAIT_WHILE_BUSY could
>> be broken, or the delay may be too short. Perhaps forcing to use the
>> delay and use a delay that for sure should work is worth to test.
>>
>> >
>> >
>> > To be even more strange the suspend to mem works without any issues
>> > (without this patch).
>> > With "reboot -f" it seems like we hang in imx53 BootROM (as I can
>> > read it from my JTAG debugger).
>>
>> So clearly there is a difference during reboot.
>>
>> A wild guess... Probably vmmc supply is turned on/off in this path,
>> while perhaps the CMD5 support has failed when the kernel issued
>> it...
>
> This needs to be checked.
>
> It may be also possible that the BootROM expects that the eMMC would
> not be in SLEEP state (after CMD5 issuing).

Well, in principle that shouldn't matter.

CMD0 is fine to use to wakeup the card from sleep state and that can
be followed by a regular initialization sequence. This is the way the
mmc core, during system resume does it.

Another possible option is that the power to vqmmc becomes power
cycled during reboot, while power to vmmc isn't.

Hope my ideas to narrow down the problem are helping and not adding
confusion. :-)

Kind regards
Uffe

Powered by blists - more mailing lists