[PATCH 1/3] mmc: core: fix using wrong io voltage if mmc_select_hs200 fails

[PATCH 1/3] mmc: core: fix using wrong io voltage if mmc_select_hs200 fails

[Dong Aisheng]

Currently MMC core will keep going if HS200/HS timing switch failed
with -EBADMSG error by the assumption that the old timing is still valid.

However, for mmc_select_hs200 case, the signal voltage may have already
been switched. If the timing switch failed, we should fall back to
the old voltage in case the card is continue run with legacy timing.

If fall back signal voltage failed, we explicitly report an EIO error
to force retry during the next power cycle.

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/mmc/core/mmc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index b8aa12c..f6683a9 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1245,9 +1245,11 @@ static int mmc_select_hs200(struct mmc_card *card)
 	struct mmc_host *host = card->host;
 	bool send_status = true;
 	unsigned int old_timing;
+	unsigned int old_signal_voltage;
 	int err = -EINVAL;
 	u8 val;
 
+	old_signal_voltage = host->ios.signal_voltage;
 	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200_1_2V)
 		err = __mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120);
 
@@ -1256,7 +1258,7 @@ static int mmc_select_hs200(struct mmc_card *card)
 
 	/* If fails try again during next card power cycle */
 	if (err)
-		goto err;
+		return err;
 
 	mmc_select_driver_type(card);
 
@@ -1290,9 +1292,14 @@ static int mmc_select_hs200(struct mmc_card *card)
 		}
 	}
 err:
-	if (err)
+	if (err) {
+		/* fall back to the old signal voltage, if fails report error */
+		if (__mmc_set_signal_voltage(host, old_signal_voltage))
+			err = -EIO;
+
 		pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
 		       __func__, err);
+	}
 	return err;
 }
 
-- 
1.9.1

[PATCH 2/3] mmc: core: remove the invalid message in mmc_select_timing

[Dong Aisheng]

mmc_select_hs200() and mmc_select_hs() will keep the timing
as before if switch fails. So it's meaningless to print the
failed switched mode outside based on the current host timing.

Furthermore, the original print is wrong, it should be:
pr_warn("%s: switch to %s failed\n",
	mmc_hostname(card->host),
	mmc_card_hs(card) ? "high-speed" :
	(mmc_card_hs200(card) ? "hs200" : ""));

Since we already have error message in mmc_select_hs200(),
simply remove it outside.

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/mmc/core/mmc.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index f6683a9..55c8201 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1321,21 +1321,13 @@ static int mmc_select_timing(struct mmc_card *card)
 	if (err && err != -EBADMSG)
 		return err;
 
-	if (err) {
-		pr_warn("%s: switch to %s failed\n",
-			mmc_card_hs(card) ? "high-speed" :
-			(mmc_card_hs200(card) ? "hs200" : ""),
-			mmc_hostname(card->host));
-		err = 0;
-	}
-
 bus_speed:
 	/*
 	 * Set the bus speed to the selected bus timing.
 	 * If timing is not selected, backward compatible is the default.
 	 */
 	mmc_set_bus_speed(card);
-	return err;
+	return 0;
 }
 
 /*
-- 
1.9.1

[PATCH 2/3] mmc: core: remove the invalid message in mmc_select_timing

[Ulf Hansson]

On 20 April 2016 at 18:51, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> mmc_select_hs200() and mmc_select_hs() will keep the timing
> as before if switch fails. So it's meaningless to print the
> failed switched mode outside based on the current host timing.
>
> Furthermore, the original print is wrong, it should be:
> pr_warn("%s: switch to %s failed\n",
>         mmc_hostname(card->host),
>         mmc_card_hs(card) ? "high-speed" :
>         (mmc_card_hs200(card) ? "hs200" : ""));
>
> Since we already have error message in mmc_select_hs200(),
> simply remove it outside.
>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/mmc/core/mmc.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index f6683a9..55c8201 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1321,21 +1321,13 @@ static int mmc_select_timing(struct mmc_card *card)
>         if (err && err != -EBADMSG)
>                 return err;
>
> -       if (err) {
> -               pr_warn("%s: switch to %s failed\n",
> -                       mmc_card_hs(card) ? "high-speed" :
> -                       (mmc_card_hs200(card) ? "hs200" : ""),
> -                       mmc_hostname(card->host));
> -               err = 0;
> -       }
> -

No, this is not correct.

First, in patch1/3, you change to return an error code immediately
when __mmc_set_signal_voltage() fails, instead of using the "goto
err". Thus there will be no error printed for this case any more.

Second, mmc_select_hs() may fail and there is no print done within
that function which is why above print is also needed.

On the other hand I agree with you, it doesn't seems necessary to
print two messages when mmc_select_hs200() fails. One should be
enough. Can we change this so *only* mmc_select_timing() will deal
with the print at all errors?

>  bus_speed:
>         /*
>          * Set the bus speed to the selected bus timing.
>          * If timing is not selected, backward compatible is the default.
>          */
>         mmc_set_bus_speed(card);
> -       return err;
> +       return 0;
>  }
>
>  /*
> --
> 1.9.1
>

Kind regards
Uffe

[PATCH 2/3] mmc: core: remove the invalid message in mmc_select_timing

[Dong Aisheng]

On Thu, Apr 28, 2016 at 11:24:07AM +0200, Ulf Hansson wrote:
> On 20 April 2016 at 18:51, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> > mmc_select_hs200() and mmc_select_hs() will keep the timing
> > as before if switch fails. So it's meaningless to print the
> > failed switched mode outside based on the current host timing.
> >
> > Furthermore, the original print is wrong, it should be:
> > pr_warn("%s: switch to %s failed\n",
> >         mmc_hostname(card->host),
> >         mmc_card_hs(card) ? "high-speed" :
> >         (mmc_card_hs200(card) ? "hs200" : ""));
> >
> > Since we already have error message in mmc_select_hs200(),
> > simply remove it outside.
> >
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >  drivers/mmc/core/mmc.c | 10 +---------
> >  1 file changed, 1 insertion(+), 9 deletions(-)
> >
> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> > index f6683a9..55c8201 100644
> > --- a/drivers/mmc/core/mmc.c
> > +++ b/drivers/mmc/core/mmc.c
> > @@ -1321,21 +1321,13 @@ static int mmc_select_timing(struct mmc_card *card)
> >         if (err && err != -EBADMSG)
> >                 return err;
> >
> > -       if (err) {
> > -               pr_warn("%s: switch to %s failed\n",
> > -                       mmc_card_hs(card) ? "high-speed" :
> > -                       (mmc_card_hs200(card) ? "hs200" : ""),
> > -                       mmc_hostname(card->host));
> > -               err = 0;
> > -       }
> > -
> 
> No, this is not correct.
> 
> First, in patch1/3, you change to return an error code immediately
> when __mmc_set_signal_voltage() fails, instead of using the "goto
> err". Thus there will be no error printed for this case any more.
> 

I noticed this issue when i made up this patch.
Why i simply return is because i want to avoid write the following code:
static int mmc_select_hs200(struct mmc_card *card)
{
...
	err = voltage_switch();
        /* If fails try again during next card power cycle */
        if (err)
                goto err;
....
		/* swith HS TIMING */
                err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
                                   EXT_CSD_HS_TIMING, val,
                                   card->ext_csd.generic_cmd6_time,
                                   true, send_status, true);
                if (err)
                        goto err2;
..........

err2:
        if (err) {
                /* fall back to the old signal voltage, if fails report error */
                if (__mmc_set_signal_voltage(host, old_signal_voltage))
                        err = -EIO;
        }

err:
        if (err)
                pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
                       __func__, err);

        return err;
}

Because if first signal switch fails, we don't need fall back to original
voltage. But HS timing switch needs.

goto err makes the code a bit ugly.
So i choose to avoid it and simply return since it's not so important
and most host driver may already implement their debug IO switch debug info.

Do you think if we can accept it?

> Second, mmc_select_hs() may fail and there is no print done within
> that function which is why above print is also needed.
> 

The same as above, i think this print is not so important.
And __mmc_switch already has a err warn.

And the most important reason is the original print outside is
meaningless because it intends to print out the speed mode it
wants to switch, however,it actually prints out the restored
original speed mode.

> On the other hand I agree with you, it doesn't seems necessary to
> print two messages when mmc_select_hs200() fails. One should be
> enough. Can we change this so *only* mmc_select_timing() will deal
> with the print at all errors?
> 

Yes, i agree with your idea.
I also thought about it before, but it seems after patch 3,
it may not be so necessary.
Can you help check it?

> >  bus_speed:
> >         /*
> >          * Set the bus speed to the selected bus timing.
> >          * If timing is not selected, backward compatible is the default.
> >          */
> >         mmc_set_bus_speed(card);
> > -       return err;
> > +       return 0;
> >  }
> >
> >  /*
> > --
> > 1.9.1
> >
> 
> Kind regards
> Uffe

Regards
Dong Aisheng

[PATCH 2/3] mmc: core: remove the invalid message in mmc_select_timing

[Ulf Hansson]

On 28 April 2016 at 16:58, Dong Aisheng <dongas86@gmail.com> wrote:
> On Thu, Apr 28, 2016 at 11:24:07AM +0200, Ulf Hansson wrote:
>> On 20 April 2016 at 18:51, Dong Aisheng <aisheng.dong@nxp.com> wrote:
>> > mmc_select_hs200() and mmc_select_hs() will keep the timing
>> > as before if switch fails. So it's meaningless to print the
>> > failed switched mode outside based on the current host timing.
>> >
>> > Furthermore, the original print is wrong, it should be:
>> > pr_warn("%s: switch to %s failed\n",
>> >         mmc_hostname(card->host),
>> >         mmc_card_hs(card) ? "high-speed" :
>> >         (mmc_card_hs200(card) ? "hs200" : ""));
>> >
>> > Since we already have error message in mmc_select_hs200(),
>> > simply remove it outside.
>> >
>> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>> > ---
>> >  drivers/mmc/core/mmc.c | 10 +---------
>> >  1 file changed, 1 insertion(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> > index f6683a9..55c8201 100644
>> > --- a/drivers/mmc/core/mmc.c
>> > +++ b/drivers/mmc/core/mmc.c
>> > @@ -1321,21 +1321,13 @@ static int mmc_select_timing(struct mmc_card *card)
>> >         if (err && err != -EBADMSG)
>> >                 return err;
>> >
>> > -       if (err) {
>> > -               pr_warn("%s: switch to %s failed\n",
>> > -                       mmc_card_hs(card) ? "high-speed" :
>> > -                       (mmc_card_hs200(card) ? "hs200" : ""),
>> > -                       mmc_hostname(card->host));
>> > -               err = 0;
>> > -       }
>> > -
>>
>> No, this is not correct.
>>
>> First, in patch1/3, you change to return an error code immediately
>> when __mmc_set_signal_voltage() fails, instead of using the "goto
>> err". Thus there will be no error printed for this case any more.
>>
>
> I noticed this issue when i made up this patch.
> Why i simply return is because i want to avoid write the following code:
> static int mmc_select_hs200(struct mmc_card *card)
> {
> ...
>         err = voltage_switch();
>         /* If fails try again during next card power cycle */
>         if (err)
>                 goto err;
> ....
>                 /* swith HS TIMING */
>                 err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>                                    EXT_CSD_HS_TIMING, val,
>                                    card->ext_csd.generic_cmd6_time,
>                                    true, send_status, true);
>                 if (err)
>                         goto err2;
> ..........
>
> err2:
>         if (err) {
>                 /* fall back to the old signal voltage, if fails report error */
>                 if (__mmc_set_signal_voltage(host, old_signal_voltage))
>                         err = -EIO;
>         }
>
> err:
>         if (err)
>                 pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
>                        __func__, err);
>
>         return err;
> }
>
> Because if first signal switch fails, we don't need fall back to original
> voltage. But HS timing switch needs.
>
> goto err makes the code a bit ugly.
> So i choose to avoid it and simply return since it's not so important
> and most host driver may already implement their debug IO switch debug info.
>
> Do you think if we can accept it?

Okay.

>
>> Second, mmc_select_hs() may fail and there is no print done within
>> that function which is why above print is also needed.
>>
>
> The same as above, i think this print is not so important.
> And __mmc_switch already has a err warn.
>
> And the most important reason is the original print outside is
> meaningless because it intends to print out the speed mode it
> wants to switch, however,it actually prints out the restored
> original speed mode.
>
>> On the other hand I agree with you, it doesn't seems necessary to
>> print two messages when mmc_select_hs200() fails. One should be
>> enough. Can we change this so *only* mmc_select_timing() will deal
>> with the print at all errors?
>>
>
> Yes, i agree with your idea.
> I also thought about it before, but it seems after patch 3,
> it may not be so necessary.
> Can you help check it?
>
>> >  bus_speed:
>> >         /*
>> >          * Set the bus speed to the selected bus timing.
>> >          * If timing is not selected, backward compatible is the default.
>> >          */
>> >         mmc_set_bus_speed(card);
>> > -       return err;
>> > +       return 0;
>> >  }
>> >
>> >  /*
>> > --
>> > 1.9.1
>> >
>>
>> Kind regards
>> Uffe
>
> Regards
> Dong Aisheng

I decided to apply this as is for next. Thanks!

Kind regards
Uffe

[PATCH 3/3] mmc: core: support hs speed mode if hs200 mode fails

[Dong Aisheng]

Currently if mmc_select_hs200 mode switch fails, MMC core
can only use legacy mode to run the card.
Let's retry HS speed mode if HS200 fails.

Before the fix:
mmc0: mmc_select_hs200 failed, error -74
: switch to mmc0 failed
mmc0: new MMC card at address 0001
mmcblk0: mmc0:0001 Q2J55L 7.12 GiB
mmcblk0boot0: mmc0:0001 Q2J55L partition 1 2.00 MiB
mmcblk0boot1: mmc0:0001 Q2J55L partition 2 2.00 MiB
mmcblk0rpmb: mmc0:0001 Q2J55L partition 3 4.00 MiB
 mmcblk0: p1 p2

After the fix:
mmc0: mmc_select_hs200 failed, error -74
mmc0: new DDR MMC card at address 0001
mmcblk0: mmc0:0001 Q2J55L 7.12 GiB
mmcblk0boot0: mmc0:0001 Q2J55L partition 1 2.00 MiB
mmcblk0boot1: mmc0:0001 Q2J55L partition 2 2.00 MiB
mmcblk0rpmb: mmc0:0001 Q2J55L partition 3 4.00 MiB
 mmcblk0: p1 p2

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/mmc/core/mmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 55c8201..b573dc7 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1315,7 +1315,8 @@ static int mmc_select_timing(struct mmc_card *card)
 
 	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
 		err = mmc_select_hs200(card);
-	else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
+
+	if (err && (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS))
 		err = mmc_select_hs(card);
 
 	if (err && err != -EBADMSG)
-- 
1.9.1

[PATCH 3/3] mmc: core: support hs speed mode if hs200 mode fails

[Ulf Hansson]

On 20 April 2016 at 18:51, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> Currently if mmc_select_hs200 mode switch fails, MMC core
> can only use legacy mode to run the card.
> Let's retry HS speed mode if HS200 fails.
>
> Before the fix:
> mmc0: mmc_select_hs200 failed, error -74
> : switch to mmc0 failed
> mmc0: new MMC card at address 0001
> mmcblk0: mmc0:0001 Q2J55L 7.12 GiB
> mmcblk0boot0: mmc0:0001 Q2J55L partition 1 2.00 MiB
> mmcblk0boot1: mmc0:0001 Q2J55L partition 2 2.00 MiB
> mmcblk0rpmb: mmc0:0001 Q2J55L partition 3 4.00 MiB
>  mmcblk0: p1 p2
>
> After the fix:
> mmc0: mmc_select_hs200 failed, error -74
> mmc0: new DDR MMC card at address 0001
> mmcblk0: mmc0:0001 Q2J55L 7.12 GiB
> mmcblk0boot0: mmc0:0001 Q2J55L partition 1 2.00 MiB
> mmcblk0boot1: mmc0:0001 Q2J55L partition 2 2.00 MiB
> mmcblk0rpmb: mmc0:0001 Q2J55L partition 3 4.00 MiB
>  mmcblk0: p1 p2
>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

Applied for next, thanks!

Kind regards
Uffe

> ---
>  drivers/mmc/core/mmc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 55c8201..b573dc7 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1315,7 +1315,8 @@ static int mmc_select_timing(struct mmc_card *card)
>
>         if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>                 err = mmc_select_hs200(card);
> -       else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
> +
> +       if (err && (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS))
>                 err = mmc_select_hs(card);
>
>         if (err && err != -EBADMSG)
> --
> 1.9.1
>

[PATCH 3/3] mmc: core: support hs speed mode if hs200 mode fails

[Marcel Ziswiler]

On Thu, 2016-04-21 at 00:51 +0800, Dong Aisheng wrote:
> Currently if mmc_select_hs200 mode switch fails, MMC core
> can only use legacy mode to run the card.
> Let's retry HS speed mode if HS200 fails.
> 
> Before the fix:
> mmc0: mmc_select_hs200 failed, error -74
> : switch to mmc0 failed
> mmc0: new MMC card at address 0001
> mmcblk0: mmc0:0001 Q2J55L 7.12 GiB
> mmcblk0boot0: mmc0:0001 Q2J55L partition 1 2.00 MiB
> mmcblk0boot1: mmc0:0001 Q2J55L partition 2 2.00 MiB
> mmcblk0rpmb: mmc0:0001 Q2J55L partition 3 4.00 MiB
> ?mmcblk0: p1 p2
> 
> After the fix:
> mmc0: mmc_select_hs200 failed, error -74
> mmc0: new DDR MMC card at address 0001
> mmcblk0: mmc0:0001 Q2J55L 7.12 GiB
> mmcblk0boot0: mmc0:0001 Q2J55L partition 1 2.00 MiB
> mmcblk0boot1: mmc0:0001 Q2J55L partition 2 2.00 MiB
> mmcblk0rpmb: mmc0:0001 Q2J55L partition 3 4.00 MiB
> ?mmcblk0: p1 p2
> 
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> ?drivers/mmc/core/mmc.c | 3 ++-
> ?1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 55c8201..b573dc7 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1315,7 +1315,8 @@ static int mmc_select_timing(struct mmc_card
> *card)
> ?
> ?	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
> ?		err = mmc_select_hs200(card);
> -	else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
> +
> +	if (err && (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS))
> ?		err = mmc_select_hs(card);
> ?
> ?	if (err && err != -EBADMSG)


This seems to break on TK1 where eMMC so far was detected as an 8-bit
high speed MMC card. With this patch it reverts to detecting a 1-bit only
MMC card! This has been observed both on a NVIDIA's Jetson TK1 as well as
our new Toradex Apalis TK1?(CC Tegra mailing list,?Jon Hunter and?Lucas
Stach as well). So far T30 is not affected as there we are still doing
full HS200 with whatever issues that has.

Before:
[????3.726894] sdhci: Secure Digital Host Controller Interface driver
[????3.733081] sdhci: Copyright(c) Pierre Ossman
[????3.737462] sdhci-pltfm: SDHCI platform and OF driver helper
[????3.753411] mmc0: Unknown controller version (3). You may experience
 problems.
[????3.768345] mmc0: Invalid maximum block size, assuming 512 bytes
[????3.814896] mmc0: SDHCI controller on 700b0600.sdhci [700b0600.sdhci]
 using ADMA 64-bit
[????3.892088] mmc0: new high speed MMC card at address 0001
[????3.899194] mmcblk0: mmc0:0001 SEM16G 14.7 GiB
[????3.904217] mmcblk0boot0: mmc0:0001 SEM16G partition 1 4.00 MiB
[????3.910645] mmcblk0boot1: mmc0:0001 SEM16G partition 2 4.00 MiB
[????3.917119] mmcblk0rpmb: mmc0:0001 SEM16G partition 3 4.00 MiB
[????3.926006]??mmcblk0: p1 p2 p3 p4 p5 p6 p7 p8 p9
root at jetson_tk1:~# cat /sys/kernel/debug/mmc0/ios?
clock:??????????52000000 Hz
actual clock:???51000000 Hz
vdd:????????????21 (3.3 ~ 3.4 V)
bus mode:???????2 (push-pull)
chip select:????0 (don't care)
power mode:?????2 (on)
bus width:??????3 (8 bits)
timing spec:????1 (mmc high-speed)
signal voltage: 0 (3.30 V)
driver type:????0 (driver type B)

After:
[????3.899918] mmc0: new MMC card at address 0001
root at jetson_tk1:~# cat /sys/kernel/debug/mmc0/ios?
clock:??????????25000000 Hz
actual clock:???24727273 Hz
vdd:????????????21 (3.3 ~ 3.4 V)
bus mode:???????2 (push-pull)
chip select:????0 (don't care)
power mode:?????2 (on)
bus width:??????0 (1 bits)
timing spec:????0 (legacy)
signal voltage: 0 (3.30 V)
driver type:????0 (driver type B)

[PATCH 3/3] mmc: core: support hs speed mode if hs200 mode fails

[Ulf Hansson]

On 13 May 2016 at 11:06, Marcel Ziswiler <marcel.ziswiler@toradex.com> wrote:
> On Thu, 2016-04-21 at 00:51 +0800, Dong Aisheng wrote:
>> Currently if mmc_select_hs200 mode switch fails, MMC core
>> can only use legacy mode to run the card.
>> Let's retry HS speed mode if HS200 fails.
>>
>> Before the fix:
>> mmc0: mmc_select_hs200 failed, error -74
>> : switch to mmc0 failed
>> mmc0: new MMC card at address 0001
>> mmcblk0: mmc0:0001 Q2J55L 7.12 GiB
>> mmcblk0boot0: mmc0:0001 Q2J55L partition 1 2.00 MiB
>> mmcblk0boot1: mmc0:0001 Q2J55L partition 2 2.00 MiB
>> mmcblk0rpmb: mmc0:0001 Q2J55L partition 3 4.00 MiB
>>  mmcblk0: p1 p2
>>
>> After the fix:
>> mmc0: mmc_select_hs200 failed, error -74
>> mmc0: new DDR MMC card at address 0001
>> mmcblk0: mmc0:0001 Q2J55L 7.12 GiB
>> mmcblk0boot0: mmc0:0001 Q2J55L partition 1 2.00 MiB
>> mmcblk0boot1: mmc0:0001 Q2J55L partition 2 2.00 MiB
>> mmcblk0rpmb: mmc0:0001 Q2J55L partition 3 4.00 MiB
>>  mmcblk0: p1 p2
>>
>> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>> ---
>>  drivers/mmc/core/mmc.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 55c8201..b573dc7 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1315,7 +1315,8 @@ static int mmc_select_timing(struct mmc_card
>> *card)
>>
>>       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>>               err = mmc_select_hs200(card);
>> -     else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
>> +
>> +     if (err && (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS))
>>               err = mmc_select_hs(card);
>>
>>       if (err && err != -EBADMSG)
>
>
> This seems to break on TK1 where eMMC so far was detected as an 8-bit
> high speed MMC card. With this patch it reverts to detecting a 1-bit only
> MMC card! This has been observed both on a NVIDIA's Jetson TK1 as well as
> our new Toradex Apalis TK1 (CC Tegra mailing list, Jon Hunter and Lucas
> Stach as well). So far T30 is not affected as there we are still doing
> full HS200 with whatever issues that has.
>
> Before:
> [    3.726894] sdhci: Secure Digital Host Controller Interface driver
> [    3.733081] sdhci: Copyright(c) Pierre Ossman
> [    3.737462] sdhci-pltfm: SDHCI platform and OF driver helper
> [    3.753411] mmc0: Unknown controller version (3). You may experience
>  problems.
> [    3.768345] mmc0: Invalid maximum block size, assuming 512 bytes
> [    3.814896] mmc0: SDHCI controller on 700b0600.sdhci [700b0600.sdhci]
>  using ADMA 64-bit
> [    3.892088] mmc0: new high speed MMC card at address 0001
> [    3.899194] mmcblk0: mmc0:0001 SEM16G 14.7 GiB
> [    3.904217] mmcblk0boot0: mmc0:0001 SEM16G partition 1 4.00 MiB
> [    3.910645] mmcblk0boot1: mmc0:0001 SEM16G partition 2 4.00 MiB
> [    3.917119] mmcblk0rpmb: mmc0:0001 SEM16G partition 3 4.00 MiB
> [    3.926006]  mmcblk0: p1 p2 p3 p4 p5 p6 p7 p8 p9
> root at jetson_tk1:~# cat /sys/kernel/debug/mmc0/ios
> clock:          52000000 Hz
> actual clock:   51000000 Hz
> vdd:            21 (3.3 ~ 3.4 V)
> bus mode:       2 (push-pull)
> chip select:    0 (don't care)
> power mode:     2 (on)
> bus width:      3 (8 bits)
> timing spec:    1 (mmc high-speed)
> signal voltage: 0 (3.30 V)
> driver type:    0 (driver type B)
>
> After:
> [    3.899918] mmc0: new MMC card at address 0001
> root at jetson_tk1:~# cat /sys/kernel/debug/mmc0/ios
> clock:          25000000 Hz
> actual clock:   24727273 Hz
> vdd:            21 (3.3 ~ 3.4 V)
> bus mode:       2 (push-pull)
> chip select:    0 (don't care)
> power mode:     2 (on)
> bus width:      0 (1 bits)
> timing spec:    0 (legacy)
> signal voltage: 0 (3.30 V)
> driver type:    0 (driver type B)
>

Thanks for testing and reporting. I have dropped this patch from my
next branch for now.

Kind regards
Uffe

[PATCH 3/3] mmc: core: support hs speed mode if hs200 mode fails

[Dong Aisheng]

On Fri, May 13, 2016 at 5:06 PM, Marcel Ziswiler
<marcel.ziswiler@toradex.com> wrote:
> On Thu, 2016-04-21 at 00:51 +0800, Dong Aisheng wrote:
>> Currently if mmc_select_hs200 mode switch fails, MMC core
>> can only use legacy mode to run the card.
>> Let's retry HS speed mode if HS200 fails.
>>
>> Before the fix:
>> mmc0: mmc_select_hs200 failed, error -74
>> : switch to mmc0 failed
>> mmc0: new MMC card at address 0001
>> mmcblk0: mmc0:0001 Q2J55L 7.12 GiB
>> mmcblk0boot0: mmc0:0001 Q2J55L partition 1 2.00 MiB
>> mmcblk0boot1: mmc0:0001 Q2J55L partition 2 2.00 MiB
>> mmcblk0rpmb: mmc0:0001 Q2J55L partition 3 4.00 MiB
>>  mmcblk0: p1 p2
>>
>> After the fix:
>> mmc0: mmc_select_hs200 failed, error -74
>> mmc0: new DDR MMC card at address 0001
>> mmcblk0: mmc0:0001 Q2J55L 7.12 GiB
>> mmcblk0boot0: mmc0:0001 Q2J55L partition 1 2.00 MiB
>> mmcblk0boot1: mmc0:0001 Q2J55L partition 2 2.00 MiB
>> mmcblk0rpmb: mmc0:0001 Q2J55L partition 3 4.00 MiB
>>  mmcblk0: p1 p2
>>
>> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>> ---
>>  drivers/mmc/core/mmc.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 55c8201..b573dc7 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1315,7 +1315,8 @@ static int mmc_select_timing(struct mmc_card
>> *card)
>>
>>       if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>>               err = mmc_select_hs200(card);
>> -     else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
>> +
>> +     if (err && (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS))
>>               err = mmc_select_hs(card);
>>
>>       if (err && err != -EBADMSG)
>
>
> This seems to break on TK1 where eMMC so far was detected as an 8-bit
> high speed MMC card. With this patch it reverts to detecting a 1-bit only
> MMC card! This has been observed both on a NVIDIA's Jetson TK1 as well as
> our new Toradex Apalis TK1 (CC Tegra mailing list, Jon Hunter and Lucas
> Stach as well). So far T30 is not affected as there we are still doing
> full HS200 with whatever issues that has.
>

Oh, sorry, just found there's a logic error in the patch.
It may wrongly bypass the HS mode select for non hs200 mode eMMCs
due to the 'err' condition.

Will send a fixed version.

Regards
Dong Aisheng

> Before:
> [    3.726894] sdhci: Secure Digital Host Controller Interface driver
> [    3.733081] sdhci: Copyright(c) Pierre Ossman
> [    3.737462] sdhci-pltfm: SDHCI platform and OF driver helper
> [    3.753411] mmc0: Unknown controller version (3). You may experience
>  problems.
> [    3.768345] mmc0: Invalid maximum block size, assuming 512 bytes
> [    3.814896] mmc0: SDHCI controller on 700b0600.sdhci [700b0600.sdhci]
>  using ADMA 64-bit
> [    3.892088] mmc0: new high speed MMC card at address 0001
> [    3.899194] mmcblk0: mmc0:0001 SEM16G 14.7 GiB
> [    3.904217] mmcblk0boot0: mmc0:0001 SEM16G partition 1 4.00 MiB
> [    3.910645] mmcblk0boot1: mmc0:0001 SEM16G partition 2 4.00 MiB
> [    3.917119] mmcblk0rpmb: mmc0:0001 SEM16G partition 3 4.00 MiB
> [    3.926006]  mmcblk0: p1 p2 p3 p4 p5 p6 p7 p8 p9
> root at jetson_tk1:~# cat /sys/kernel/debug/mmc0/ios
> clock:          52000000 Hz
> actual clock:   51000000 Hz
> vdd:            21 (3.3 ~ 3.4 V)
> bus mode:       2 (push-pull)
> chip select:    0 (don't care)
> power mode:     2 (on)
> bus width:      3 (8 bits)
> timing spec:    1 (mmc high-speed)
> signal voltage: 0 (3.30 V)
> driver type:    0 (driver type B)
>
> After:
> [    3.899918] mmc0: new MMC card at address 0001
> root at jetson_tk1:~# cat /sys/kernel/debug/mmc0/ios
> clock:          25000000 Hz
> actual clock:   24727273 Hz
> vdd:            21 (3.3 ~ 3.4 V)
> bus mode:       2 (push-pull)
> chip select:    0 (don't care)
> power mode:     2 (on)
> bus width:      0 (1 bits)
> timing spec:    0 (legacy)
> signal voltage: 0 (3.30 V)
> driver type:    0 (driver type B)
>

[PATCH 1/3] mmc: core: fix using wrong io voltage if mmc_select_hs200 fails

[Ulf Hansson]

On 20 April 2016 at 18:51, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> Currently MMC core will keep going if HS200/HS timing switch failed
> with -EBADMSG error by the assumption that the old timing is still valid.
>
> However, for mmc_select_hs200 case, the signal voltage may have already
> been switched. If the timing switch failed, we should fall back to
> the old voltage in case the card is continue run with legacy timing.
>
> If fall back signal voltage failed, we explicitly report an EIO error
> to force retry during the next power cycle.
>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/mmc/core/mmc.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index b8aa12c..f6683a9 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1245,9 +1245,11 @@ static int mmc_select_hs200(struct mmc_card *card)
>         struct mmc_host *host = card->host;
>         bool send_status = true;
>         unsigned int old_timing;
> +       unsigned int old_signal_voltage;

Nitpick; you can merge this change into the line above.

>         int err = -EINVAL;
>         u8 val;
>
> +       old_signal_voltage = host->ios.signal_voltage;
>         if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200_1_2V)
>                 err = __mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120);
>
> @@ -1256,7 +1258,7 @@ static int mmc_select_hs200(struct mmc_card *card)
>
>         /* If fails try again during next card power cycle */
>         if (err)
> -               goto err;
> +               return err;
>
>         mmc_select_driver_type(card);
>
> @@ -1290,9 +1292,14 @@ static int mmc_select_hs200(struct mmc_card *card)
>                 }
>         }
>  err:
> -       if (err)
> +       if (err) {
> +               /* fall back to the old signal voltage, if fails report error */
> +               if (__mmc_set_signal_voltage(host, old_signal_voltage))
> +                       err = -EIO;
> +
>                 pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
>                        __func__, err);
> +       }
>         return err;
>  }
>
> --
> 1.9.1
>

Kind regards
Uffe

[PATCH 1/3] mmc: core: fix using wrong io voltage if mmc_select_hs200 fails

[Ulf Hansson]

On 28 April 2016 at 11:26, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 20 April 2016 at 18:51, Dong Aisheng <aisheng.dong@nxp.com> wrote:
>> Currently MMC core will keep going if HS200/HS timing switch failed
>> with -EBADMSG error by the assumption that the old timing is still valid.
>>
>> However, for mmc_select_hs200 case, the signal voltage may have already
>> been switched. If the timing switch failed, we should fall back to
>> the old voltage in case the card is continue run with legacy timing.
>>
>> If fall back signal voltage failed, we explicitly report an EIO error
>> to force retry during the next power cycle.
>>
>> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>> ---
>>  drivers/mmc/core/mmc.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index b8aa12c..f6683a9 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1245,9 +1245,11 @@ static int mmc_select_hs200(struct mmc_card *card)
>>         struct mmc_host *host = card->host;
>>         bool send_status = true;
>>         unsigned int old_timing;
>> +       unsigned int old_signal_voltage;
>
> Nitpick; you can merge this change into the line above.
>

Applied for next with a minor change dealing with the nitpick above, thanks!

Kind regards
Uffe