Message-ID: <20180423161113.463d4f0e@jawa>
Date:   Mon, 23 Apr 2018 16:11:13 +0200
From:   Lukasz Majewski <lukma@...x.de>
To:     Ulf Hansson <ulf.hansson@...aro.org>
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

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.

> 
> >
> > 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.

> 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.

> 
> 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).

> 
> >
> > As written in the commit message - problems started after sending
> > suspend sequence of eMMC commands to the eMMC device.
> >
> >
> > [The eMMC memory itself is Micron 4GiB - 4.4.1]  
> 
> [...]
> 
> Kind regards
> Uffe




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@...x.de

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists