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

From: "Seunghui Lee" <sh043.lee@samsung.com>
To: "'Adrian Hunter'" <adrian.hunter@intel.com>,
	"'Ulf Hansson'" <ulf.hansson@linaro.org>
Cc: "'linux-mmc'" <linux-mmc@vger.kernel.org>,
	"'DooHyun Hwang'" <dh0421.hwang@samsung.com>
Subject: RE: [PATCH 2/2] mmc: sd: Fix inconsistent sd3_bus_mode with failed voltage switch
Date: Tue, 16 Aug 2022 17:30:17 +0900	[thread overview]
Message-ID: <000001d8b14a$6a44a710$3ecdf530$@samsung.com> (raw)
In-Reply-To: <20220815073321.63382-3-adrian.hunter@intel.com>

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