[PATCH] mmc: disable card sleep via device-tree

[PATCH] mmc: disable card sleep via device-tree

[Lukasz Majewski]

From: Stanislav Meduna <stanislav.meduna@nxtcontrol.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@nxtcontrol.com>
Signed-off-by: Lukasz Majewski <lukma@denx.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)
 		err = mmc_sleep(host);
 	else if (!mmc_host_is_spi(host))
 		err = mmc_deselect_cards(host);
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 279b39008a33..c64d88e6de3b 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -304,8 +304,8 @@ struct mmc_card {
 	struct dentry		*debugfs_root;
 	struct mmc_part	part[MMC_NUM_PHY_PARTITION]; /* physical partitions */
 	unsigned int    nr_parts;
-
 	unsigned int		bouncesz;	/* Bounce buffer size */
+	bool	no_sleep_on_suspend;
 };
 
 static inline bool mmc_large_sector(struct mmc_card *card)
-- 
2.11.0

Re: [PATCH] mmc: disable card sleep via device-tree

[Vladimir Zapolskiy]

Hi Lukasz,

On 04/23/2018 12:31 AM, Lukasz Majewski wrote:
> From: Stanislav Meduna <stanislav.meduna@nxtcontrol.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@nxtcontrol.com>
> Signed-off-by: Lukasz Majewski <lukma@denx.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.

I would recommend to examine an option to reuse the existing host
controller property 'keep-power-in-suspend', note that a particular
host controller driver may ignore it.

In general the problem sounds like a PCB hardware bug, and if it is
eMMC card specific, please provide more information about that eMMC
rather than about the host.

--
With best wishes,
Vladimir

Re: [PATCH] mmc: disable card sleep via device-tree

[Vladimir Zapolskiy]

Hi Lukasz,

On 04/23/2018 12:31 AM, Lukasz Majewski wrote:
> From: Stanislav Meduna <stanislav.meduna@nxtcontrol.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@nxtcontrol.com>
> Signed-off-by: Lukasz Majewski <lukma@denx.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.

I would recommend to examine an option to reuse the existing host
controller property 'keep-power-in-suspend', note that a particular
host controller driver may ignore it.

In general the problem sounds like a PCB hardware bug, and if it is
eMMC card specific, please provide more information about that eMMC
rather than about the host.

--
With best wishes,
Vladimir

Re: [PATCH] mmc: disable card sleep via device-tree

[Ulf Hansson]

On 22 April 2018 at 23:31, Lukasz Majewski <lukma@denx.de> wrote:
> From: Stanislav Meduna <stanislav.meduna@nxtcontrol.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@nxtcontrol.com>
> Signed-off-by: Lukasz Majewski <lukma@denx.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(). Does your host driver cut power to VMMC and/or
VQMMC?

>                 err = mmc_sleep(host);
>         else if (!mmc_host_is_spi(host))
>                 err = mmc_deselect_cards(host);
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 279b39008a33..c64d88e6de3b 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -304,8 +304,8 @@ struct mmc_card {
>         struct dentry           *debugfs_root;
>         struct mmc_part part[MMC_NUM_PHY_PARTITION]; /* physical partitions */
>         unsigned int    nr_parts;
> -
>         unsigned int            bouncesz;       /* Bounce buffer size */
> +       bool    no_sleep_on_suspend;
>  };
>
>  static inline bool mmc_large_sector(struct mmc_card *card)
> --
> 2.11.0
>

My impression is that it seems like there are still some uncertainties
around what actually happens when the failure occurs during re-boot. I
am guessing this boils done to how VMMC and VQMMC are being managed.

BTW, in the the pwrseq_emmc, we register a restart handler to pull the
reset GPIO for the eMMC. Perhaps that can help to fix this problem as
well!?

Kind regards
Uffe

Re: [PATCH] mmc: disable card sleep via device-tree

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 6088 bytes --]

Hi Ulf,

> On 22 April 2018 at 23:31, Lukasz Majewski <lukma@denx.de> wrote:
> > From: Stanislav Meduna <stanislav.meduna@nxtcontrol.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@nxtcontrol.com>
> > Signed-off-by: Lukasz Majewski <lukma@denx.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.

It seems like the eMMC is always powered with 3.3V.

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


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

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]

> 
> >                 err = mmc_sleep(host);
> >         else if (!mmc_host_is_spi(host))
> >                 err = mmc_deselect_cards(host);
> > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> > index 279b39008a33..c64d88e6de3b 100644
> > --- a/include/linux/mmc/card.h
> > +++ b/include/linux/mmc/card.h
> > @@ -304,8 +304,8 @@ struct mmc_card {
> >         struct dentry           *debugfs_root;
> >         struct mmc_part part[MMC_NUM_PHY_PARTITION]; /* physical
> > partitions */ unsigned int    nr_parts;
> > -
> >         unsigned int            bouncesz;       /* Bounce buffer
> > size */
> > +       bool    no_sleep_on_suspend;
> >  };
> >
> >  static inline bool mmc_large_sector(struct mmc_card *card)
> > --
> > 2.11.0
> >  
> 
> My impression is that it seems like there are still some uncertainties
> around what actually happens when the failure occurs during re-boot. I
> am guessing this boils done to how VMMC and VQMMC are being managed.
> 
> BTW, in the the pwrseq_emmc, we register a restart handler to pull the
> reset GPIO for the eMMC. Perhaps that can help to fix this problem as
> well!?
> 
> 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@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] mmc: disable card sleep via device-tree

[Ulf Hansson]

On 23 April 2018 at 11:36, Lukasz Majewski <lukma@denx.de> wrote:
> Hi Ulf,
>
>> On 22 April 2018 at 23:31, Lukasz Majewski <lukma@denx.de> wrote:
>> > From: Stanislav Meduna <stanislav.meduna@nxtcontrol.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@nxtcontrol.com>
>> > Signed-off-by: Lukasz Majewski <lukma@denx.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?

>
> 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? If
not, there is a delay (specific to the card) after the CMD5.

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

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

Re: [PATCH] mmc: disable card sleep via device-tree

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 6821 bytes --]

Hi Ulf,

> On 23 April 2018 at 11:36, Lukasz Majewski <lukma@denx.de> wrote:
> > Hi Ulf,
> >  
> >> On 22 April 2018 at 23:31, Lukasz Majewski <lukma@denx.de> wrote:  
> >> > From: Stanislav Meduna <stanislav.meduna@nxtcontrol.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@nxtcontrol.com>
> >> > Signed-off-by: Lukasz Majewski <lukma@denx.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@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] mmc: disable card sleep via device-tree

[Ulf Hansson]

On 23 April 2018 at 16:11, Lukasz Majewski <lukma@denx.de> wrote:
> Hi Ulf,
>
>> On 23 April 2018 at 11:36, Lukasz Majewski <lukma@denx.de> wrote:
>> > Hi Ulf,
>> >
>> >> On 22 April 2018 at 23:31, Lukasz Majewski <lukma@denx.de> wrote:
>> >> > From: Stanislav Meduna <stanislav.meduna@nxtcontrol.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@nxtcontrol.com>
>> >> > Signed-off-by: Lukasz Majewski <lukma@denx.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

Re: [PATCH] mmc: disable card sleep via device-tree

[Linus Walleij]

On Sun, Apr 22, 2018 at 11:31 PM, Lukasz Majewski <lukma@denx.de> wrote:

> From: Stanislav Meduna <stanislav.meduna@nxtcontrol.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@nxtcontrol.com>
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
(...)

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

As far as I understand this problem is not coming from the host
controller itself,
so it should not be tagged on to the host controller either. Rather the problem
is how the specific system has integrated the host controller, the problem is
in the fixture of this specific machine, as you say, in the i.MX53 ROM.

In that case, I would say use this:

if (of_machine_is_compatible("fsl,imx53")) {
   ..activate quirk...
}

Yours,
Linus Walleij

Re: [PATCH] mmc: disable card sleep via device-tree

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]

On Mon, 30 Apr 2018 11:13:45 +0200
Hi Linus,

> On Sun, Apr 22, 2018 at 11:31 PM, Lukasz Majewski <lukma@denx.de>
> wrote:
> 
> > From: Stanislav Meduna <stanislav.meduna@nxtcontrol.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@nxtcontrol.com>
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>  
> (...)
> 
> >  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.  
> 
> As far as I understand this problem is not coming from the host
> controller itself,
> so it should not be tagged on to the host controller either. Rather
> the problem is how the specific system has integrated the host
> controller, the problem is in the fixture of this specific machine,
> as you say, in the i.MX53 ROM.
> 
> In that case, I would say use this:
> 
> if (of_machine_is_compatible("fsl,imx53")) {
>    ..activate quirk...
> }

I see. Thanks for pointing this.

> 
> Yours,
> Linus Walleij




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@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]