Skip to contentSkip to conversation

mmc: core: fix using wrong io voltage if mmc_select_hs200 fails

PATCH New Dong Aisheng 4 participants 12 messages Apr 20, 2016 – May 26, 2016

Subsystems (2)
MMC, SD and SDIO Tegra architecture support
lore
Share & tools
lore

Dong Aisheng

author
#0 lore raw

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

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 <[email protected]>
---
 drivers/mmc/core/mmc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/mmc/core/mmc.c +9 -2
index b8aa12c..f6683a9 100644
@@ -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;
 }
Signature · 1 line
1.9.1
author
#1 lore raw

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

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 <[email protected]>
---
 drivers/mmc/core/mmc.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)
drivers/mmc/core/mmc.c +1 -9
index f6683a9..55c8201 100644
@@ -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;
 }
 
 /*
Signature · 1 line
1.9.1
#2 lore raw

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

Quoted 24 lines · 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 <[email protected]>
---
 drivers/mmc/core/mmc.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)
drivers/mmc/core/mmc.c +0 -0
index f6683a9..55c8201 100644
@@ -1321,21 +1321,13 @@ static int mmc_select_timing(struct mmc_card *card)
        if (err && err != -EBADMSG)

drivers/mmc/core/mmc.c static int mmc_select_timing(struct mmc_card *card) · line 1331

Quoted 10 lines
drivers/mmc/core/mmc.c +0 -8
                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?

Quoted 12 lines
drivers/mmc/core/mmc.c +1 -2
 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;
 }
 
 /*
--
Quoted 2 lines

1.9.1

Kind regards
Uffe

#3 lore raw

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

Quoted 38 lines · Ulf Hansson
Quoted 24 lines · 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 <[email protected]>
---
 drivers/mmc/core/mmc.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)
drivers/mmc/core/mmc.c +0 -0
index f6683a9..55c8201 100644
@@ -1321,21 +1321,13 @@ static int mmc_select_timing(struct mmc_card *card)
        if (err && err != -EBADMSG)

drivers/mmc/core/mmc.c static int mmc_select_timing(struct mmc_card *card) · line 1331

Quoted 10 lines
drivers/mmc/core/mmc.c +0 -8
                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.

Quoted 4 lines

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?

Quoted 3 lines

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.

Quoted 5 lines

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?

Quoted 10 lines
Quoted 10 lines
drivers/mmc/core/mmc.c +1 -1
 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;
 }
Quoted 7 lines
Quoted 4 lines
drivers/mmc/core/mmc.c +0 -1
 /*
--

1.9.1

Kind regards
Uffe

Regards
Dong Aisheng

#4 lore raw

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

Quoted 78 lines · Dong Aisheng
Quoted 38 lines · Ulf Hansson
Quoted 24 lines · 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 <[email protected]>
---
 drivers/mmc/core/mmc.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)
drivers/mmc/core/mmc.c +0 -0
index f6683a9..55c8201 100644
@@ -1321,21 +1321,13 @@ static int mmc_select_timing(struct mmc_card *card)
        if (err && err != -EBADMSG)

drivers/mmc/core/mmc.c static int mmc_select_timing(struct mmc_card *card) · line 1331

Quoted 10 lines
drivers/mmc/core/mmc.c +0 -8
                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.

Quoted 4 lines

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;
}
Quoted 8 lines

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.

Quoted 39 lines
Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 15 lines
Quoted 14 lines
drivers/mmc/core/mmc.c +1 -2
 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

Quoted 5 lines
Quoted 2 lines

Kind regards
Uffe

Regards
Dong Aisheng

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

Kind regards
Uffe

author
#5 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/mmc/core/mmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/mmc/core/mmc.c +2 -1
index 55c8201..b573dc7 100644
@@ -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)
Signature · 1 line
1.9.1
#6 lore raw

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

Quoted 23 lines · 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

Quoted 1 line
Signed-off-by Dong Aisheng <[email protected]>

Applied for next, thanks!

Kind regards
Uffe

Quoted 21 lines
---
 drivers/mmc/core/mmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/mmc/core/mmc.c +2 -2
index 55c8201..b573dc7 100644
@@ -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

#7 lore raw

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

In reply to Dong Aisheng
Quoted 40 lines · 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 <[email protected]>

---
?drivers/mmc/core/mmc.c | 3 ++-
?1 file changed, 2 insertions(+), 1 deletion(-)

drivers/mmc/core/mmc.c +0 -0
index 55c8201..b573dc7 100644
@@ -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);

drivers/mmc/core/mmc.c +2 -1
-	else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
+
+	if (err && (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS))
Quoted 3 lines

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

#8 lore raw

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

Quoted 81 lines · Marcel Ziswiler
Quoted 42 lines · 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 <[email protected]>
---
 drivers/mmc/core/mmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/mmc/core/mmc.c +2 -1
index 55c8201..b573dc7 100644
@@ -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);
Quoted 1 line
      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:

Quoted 13 lines
[    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

#9 lore raw

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

In reply to Marcel Ziswiler
Quoted 43 lines · Marcel Ziswiler
Quoted 42 lines · 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 <[email protected]>
---
 drivers/mmc/core/mmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/mmc/core/mmc.c +2 -1
index 55c8201..b573dc7 100644
@@ -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);
Quoted 10 lines
Quoted 1 line
      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

Quoted 41 lines

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)
#10 lore raw

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

In reply to Dong Aisheng
Quoted 20 lines · 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 <[email protected]>
---
 drivers/mmc/core/mmc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/mmc/core/mmc.c +0 -0
index b8aa12c..f6683a9 100644

drivers/mmc/core/mmc.c static int mmc_select_hs200(struct mmc_card *card) · line 1248

Quoted 4 lines
drivers/mmc/core/mmc.c +1 -0
@@ -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.

Quoted 33 lines
drivers/mmc/core/mmc.c +8 -3
        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;
 }
 
--
Quoted 2 lines

1.9.1

Kind regards
Uffe

#11 lore raw

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

Quoted 16 lines · Ulf Hansson
Quoted 15 lines · 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 <[email protected]>
---
 drivers/mmc/core/mmc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
Quoted 12 lines

drivers/mmc/core/mmc.c static int mmc_select_hs200(struct mmc_card *card) · line 1248

Quoted 9 lines
drivers/mmc/core/mmc.c +1 -0
index b8aa12c..f6683a9 100644
@@ -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