Skip to contentSkip to conversation

mmc: sd: Fix inconsistent sd3_bus_mode with failed voltage switch

PATCH In review Adrian Hunter 3 participants 6 messages Aug 15, 2022 – Aug 19, 2022 MMC, SD and SDIO

lore
Share & tools
lore
#0 lore raw

cover mmc: sd: Fix inconsistent sd3_bus_mode with failed voltage switch

Hi

Here are 2 SD card fixes prompted by the following email thread:

	https://lore.kernel.org/linux-mmc/[email protected]/T/#u

Adrian Hunter (2):
mmc: sd: Fix 1.8V workaround branch
mmc: sd: Fix inconsistent sd3_bus_mode with failed voltage switch

 drivers/mmc/core/sd.c | 46 ++++++++++++++++++----------------------------
 1 file changed, 18 insertions(+), 28 deletions(-)

Regards
Adrian

#1 lore raw

PATCH 1/2 mmc: sd: Fix 1.8V workaround branch

When introduced, upon success, the 1.8V fixup workaround in mmc_sd_init_card() would branch to practically the end of the function, to a label named "done". Unfortunately, perhaps due to the label name, over time new code has been added that really should have come after "done" not before it. Move the label to the correct place and rename it "cont".

Fixes 045d705dc1fb ("mmc: core: Enable the MMC host software queue for the SD card")
---
 drivers/mmc/core/sd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/mmc/core/sd.c +3 -3
index cee4c0b59f43..bc84d7dfc8e1 100644
@@ -1498,7 +1498,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 					mmc_remove_card(card);
 				goto retry;
 			}
-			goto done;
+			goto cont;
 		}
 	}
 
@@ -1534,7 +1534,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 			mmc_set_bus_width(host, MMC_BUS_WIDTH_4);
 		}
 	}
-
+cont:
 	if (!oldcard) {
 		/* Read/parse the extension registers. */
 		err = sd_read_ext_regs(card);
@@ -1566,7 +1566,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 		err = -EINVAL;
 		goto free_card;
 	}
-done:
+
 	host->card = card;
 	return 0;
 
Signature · 1 line
2.25.1
Reviewed-by on v1 1/2
#2 lore raw

RE: [PATCH 1/2] mmc: sd: Fix 1.8V workaround branch

Quoted 56 lines

-----Original Message-----
From: Adrian Hunter <[email protected]>
Sent: Monday, August 15, 2022 4:33 PM
To: Ulf Hansson <[email protected]>
Cc: linux-mmc <[email protected]>; Seunghui Lee <[email protected]>; DooHyun Hwang <[email protected]>
Subject: [PATCH 1/2] mmc: sd: Fix 1.8V workaround branch

When introduced, upon success, the 1.8V fixup workaround in mmc_sd_init_card() would branch to practically the end of the function, to a label named "done". Unfortunately, perhaps due to the label name, over time new code has been added that really should have come after "done" not before it. Move the label to the correct place and rename it "cont".

Fixes 045d705dc1fb ("mmc: core: Enable the MMC host software queue for

the SD card")

---
 drivers/mmc/core/sd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/mmc/core/sd.c index renamed from drivers/mmc/core/sd.c +0 -0

cee4c0b59f43..bc84d7dfc8e1 100644

drivers/mmc/core/sd.c +2 -2
@@ -1498,7 +1498,7 @@ static int mmc_sd_init_card(struct mmc_host *host,
 u32 ocr,
 					mmc_remove_card(card);
 				goto retry;
 			}
-			goto done;
+			goto cont;
 		}
 	}
@@ -1534,7 +1534,7 @@ static int mmc_sd_init_card(struct mmc_host *host,
 u32 ocr,
 			mmc_set_bus_width(host, MMC_BUS_WIDTH_4);
 		}
 	}
-
+cont:
 	if (!oldcard) {
 		/* Read/parse the extension registers. */
drivers/mmc/core/sd.c +1 -2
 		err = sd_read_ext_regs(card);
@@ -1566,7 +1566,7 @@ static int mmc_sd_init_card(struct mmc_host *host,
 u32 ocr,
 		err = -EINVAL;
 		goto free_card;
 	}
-done:
+
 	host->card = card;
 	return 0;
 
--
Quoted 1 line

2.25.1

Looks good to me.

#3 lore raw

PATCH 2/2 mmc: sd: Fix inconsistent sd3_bus_mode with failed voltage switch

In reply to Adrian Hunter

If re-initialization results is a different signal voltage, because the voltage switch failed previously but not this time (or vice versa), then sd3_bus_mode will be inconsistent with the card because the SD_SWITCH command is done only upon first initialization.

Fix by always reading SD_SWITCH information during re-initialization which also means it does not need to be re-read later for the 1.8V fixup workaround.

Note, brief testing showed SD_SWITCH took about 1.8ms to 2ms which added about 1% to 1.5% to the re-initialization time, so not particularly significant.

---
 drivers/mmc/core/sd.c | 42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)
drivers/mmc/core/sd.c +16 -26
index bc84d7dfc8e1..06aa62ce0ed1 100644
@@ -949,15 +949,16 @@ int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
 
 		/* Erase init depends on CSD and SSR */
 		mmc_init_erase(card);
-
-		/*
-		 * Fetch switch information from card.
-		 */
-		err = mmc_read_switch(card);
-		if (err)
-			return err;
 	}
 
+	/*
+	 * Fetch switch information from card. Note, sd3_bus_mode can change if
+	 * voltage switch outcome changes, so do this always.
+	 */
+	err = mmc_read_switch(card);
+	if (err)
+		return err;
+
 	/*
 	 * For SPI, enable CRC as appropriate.
 	 * This CRC enable is located AFTER the reading of the
@@ -1480,26 +1481,15 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 	if (!v18_fixup_failed && !mmc_host_is_spi(host) && mmc_host_uhs(host) &&
 	    mmc_sd_card_using_v18(card) &&
 	    host->ios.signal_voltage != MMC_SIGNAL_VOLTAGE_180) {
-		/*
-		 * Re-read switch information in case it has changed since
-		 * oldcard was initialized.
-		 */
-		if (oldcard) {
-			err = mmc_read_switch(card);
-			if (err)
-				goto free_card;
-		}
-		if (mmc_sd_card_using_v18(card)) {
-			if (mmc_host_set_uhs_voltage(host) ||
-			    mmc_sd_init_uhs_card(card)) {
-				v18_fixup_failed = true;
-				mmc_power_cycle(host, ocr);
-				if (!oldcard)
-					mmc_remove_card(card);
-				goto retry;
-			}
-			goto cont;
+		if (mmc_host_set_uhs_voltage(host) ||
+		    mmc_sd_init_uhs_card(card)) {
+			v18_fixup_failed = true;
+			mmc_power_cycle(host, ocr);
+			if (!oldcard)
+				mmc_remove_card(card);
+			goto retry;
 		}
+		goto cont;
 	}
 
 	/* Initialization sequence for UHS-I cards */
Signature · 1 line
2.25.1
Reviewed-by on v1 2/2
#4 lore raw
Quoted 95 lines

-----Original Message-----
From: Adrian Hunter <[email protected]>
Sent: Monday, August 15, 2022 4:33 PM
To: Ulf Hansson <[email protected]>
Cc: linux-mmc <[email protected]>; Seunghui Lee <[email protected]>; DooHyun Hwang <[email protected]>
Subject: [PATCH 2/2] mmc: sd: Fix inconsistent sd3_bus_mode with failed voltage switch

If re-initialization results is a different signal voltage, because the voltage switch failed previously but not this time (or vice versa), then sd3_bus_mode will be inconsistent with the card because the SD_SWITCH command is done only upon first initialization.

Fix by always reading SD_SWITCH information during re-initialization which also means it does not need to be re-read later for the 1.8V fixup workaround.

Note, brief testing showed SD_SWITCH took about 1.8ms to 2ms which added about 1% to 1.5% to the re-initialization time, so not particularly significant.

---
 drivers/mmc/core/sd.c | 42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)
drivers/mmc/core/sd.c index renamed from drivers/mmc/core/sd.c +0 -0

bc84d7dfc8e1..06aa62ce0ed1 100644

drivers/mmc/core/sd.c +2 -7
@@ -949,15 +949,16 @@ int mmc_sd_setup_card(struct mmc_host *host, struct
 mmc_card *card,
 
 		/* Erase init depends on CSD and SSR */
 		mmc_init_erase(card);
-
-		/*
-		 * Fetch switch information from card.
-		 */
-		err = mmc_read_switch(card);
-		if (err)
-			return err;
 	}
 
+	/*
+	 * Fetch switch information from card. Note, sd3_bus_mode can

change if

drivers/mmc/core/sd.c +7 -0
+	 * voltage switch outcome changes, so do this always.
+	 */
+	err = mmc_read_switch(card);
+	if (err)
+		return err;
+
 	/*
 	 * For SPI, enable CRC as appropriate.
 	 * This CRC enable is located AFTER the reading of the @@ -1480,26
+1481,15 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 	if (!v18_fixup_failed && !mmc_host_is_spi(host) &&

mmc_host_uhs(host) &&

drivers/mmc/core/sd.c +8 -20
 	    mmc_sd_card_using_v18(card) &&
 	    host->ios.signal_voltage != MMC_SIGNAL_VOLTAGE_180) {
-		/*
-		 * Re-read switch information in case it has changed since
-		 * oldcard was initialized.
-		 */
-		if (oldcard) {
-			err = mmc_read_switch(card);
-			if (err)
-				goto free_card;
-		}
-		if (mmc_sd_card_using_v18(card)) {
-			if (mmc_host_set_uhs_voltage(host) ||
-			    mmc_sd_init_uhs_card(card)) {
-				v18_fixup_failed = true;
-				mmc_power_cycle(host, ocr);
-				if (!oldcard)
-					mmc_remove_card(card);
-				goto retry;
-			}
-			goto cont;
+		if (mmc_host_set_uhs_voltage(host) ||
+		    mmc_sd_init_uhs_card(card)) {
+			v18_fixup_failed = true;
+			mmc_power_cycle(host, ocr);
+			if (!oldcard)
+				mmc_remove_card(card);
+			goto retry;
 		}
+		goto cont;
 	}
 
 	/* Initialization sequence for UHS-I cards */
--
Quoted 1 line

2.25.1

I've just tested this.
It works well.
Thank you for your work.

#5 lore raw
In reply to Adrian Hunter
Quoted 8 lines · Adrian Hunter

Hi

Here are 2 SD card fixes prompted by the following email thread:

        https://lore.kernel.org/linux-mmc/[email protected]/T/#u
Quoted 8 lines

Adrian Hunter (2):
mmc: sd: Fix 1.8V workaround branch
mmc: sd: Fix inconsistent sd3_bus_mode with failed voltage switch

 drivers/mmc/core/sd.c | 46 ++++++++++++++++++----------------------------
 1 file changed, 18 insertions(+), 28 deletions(-)

Applied for fixes and by adding stable tags to both the patches, thanks!

Kind regards
Uffe