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

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

[Adrian Hunter]

Hi

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

	https://lore.kernel.org/linux-mmc/20220721055924.9043-1-sh043.lee@samsung.com/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

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

[Adrian Hunter]

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")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/sd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index cee4c0b59f43..bc84d7dfc8e1 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -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;
 
-- 
2.25.1

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

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

Reported-by: Seunghui Lee <sh043.lee@samsung.com>
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/sd.c | 42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index bc84d7dfc8e1..06aa62ce0ed1 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -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 */
-- 
2.25.1

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

[Seunghui Lee]

> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@intel.com>
> Sent: Monday, August 15, 2022 4:33 PM
> To: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: linux-mmc <linux-mmc@vger.kernel.org>; Seunghui Lee
> <sh043.lee@samsung.com>; DooHyun Hwang <dh0421.hwang@samsung.com>
> 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.
> 
> Reported-by: Seunghui Lee <sh043.lee@samsung.com>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/core/sd.c | 42 ++++++++++++++++--------------------------
>  1 file changed, 16 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c index
> bc84d7dfc8e1..06aa62ce0ed1 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -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 */
> --
> 2.25.1

I've just tested this.
It works well.
Thank you for your work.
Reviewed-by: Seunghui Lee <sh043.lee@samsung.com>

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

[Seunghui Lee]

> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@intel.com>
> Sent: Monday, August 15, 2022 4:33 PM
> To: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: linux-mmc <linux-mmc@vger.kernel.org>; Seunghui Lee
> <sh043.lee@samsung.com>; DooHyun Hwang <dh0421.hwang@samsung.com>
> 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")
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/core/sd.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c index
> cee4c0b59f43..bc84d7dfc8e1 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -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;
> 
> --
> 2.25.1

Looks good to me.
Reviewed-by: Seunghui Lee <sh043.lee@samsung.com>

Re: [PATCH 0/2] mmc: sd: Fix inconsistent sd3_bus_mode with failed voltage switch

[Ulf Hansson]

On Mon, 15 Aug 2022 at 09:33, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> Hi
>
> Here are 2 SD card fixes prompted by the following email thread:
>
>         https://lore.kernel.org/linux-mmc/20220721055924.9043-1-sh043.lee@samsung.com/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(-)
>
>

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

Kind regards
Uffe