Re: [PATCH] mmc: mmc: do not use CMD13 to get status after speed mode switch

[Adrian Hunter <adrian.hunter@intel.com>]

On 04/05/16 09:54, Chaotian Jing wrote:
> Per JEDEC spec, it is not recommended to use CMD13 to get card status
> after speed mode switch. below are two reason about this:
> 1. CMD13 cannot be guaranteed due to the asynchronous operation.
> Therefore it is not recommended to use CMD13 to check busy completion
> of the timing change indication.
> 2. After switch to HS200, CMD13 will get response of 0x800, and even the
> busy signal gets de-asserted, the response of CMD13 is aslo 0x800.
> 
> this patch drops CMD13 when doing speed mode switch, if host do not
> support MMC_CAP_WAIT_WHILE_BUSY and there is no ops->card_busy(),
> then the only way is to wait a fixed timeout.

This looks like it should be 3 patches:
	1. Change __mmc_switch
	2. Change HS200 and HS400 selection
	3. Change HS selection

However there is another problem: card_busy is not the same as busy signal -
see below.  So that needs to be sorted out first.

> 
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> ---
>  drivers/mmc/core/mmc.c     |   82 ++++++++++++++++----------------------------
>  drivers/mmc/core/mmc_ops.c |   25 +++++++++-----
>  2 files changed, 45 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 4dbe3df..03ee7a4 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -962,7 +962,7 @@ static int mmc_select_hs(struct mmc_card *card)
>  	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>  			   EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS,
>  			   card->ext_csd.generic_cmd6_time,
> -			   true, true, true);
> +			   true, false, true);

If you are going to do that, then you still need to do mmc_switch_status(card).

But we have done CMD13 polling for HS for a long time, so this should be a
separate patch and separately justified.  Maybe we should still poll if
!(host->caps & MMC_CAP_WAIT_WHILE_BUSY) && !host->ops->card_busy.

>  	if (!err)
>  		mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
>  
> @@ -1056,7 +1056,6 @@ static int mmc_switch_status(struct mmc_card *card)
>  static int mmc_select_hs400(struct mmc_card *card)
>  {
>  	struct mmc_host *host = card->host;
> -	bool send_status = true;
>  	unsigned int max_dtr;
>  	int err = 0;
>  	u8 val;
> @@ -1068,9 +1067,6 @@ static int mmc_select_hs400(struct mmc_card *card)
>  	      host->ios.bus_width == MMC_BUS_WIDTH_8))
>  		return 0;
>  
> -	if (host->caps & MMC_CAP_WAIT_WHILE_BUSY)
> -		send_status = false;
> -
>  	/* Reduce frequency to HS frequency */
>  	max_dtr = card->ext_csd.hs_max_dtr;
>  	mmc_set_clock(host, max_dtr);
> @@ -1080,7 +1076,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>  	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>  			   EXT_CSD_HS_TIMING, val,
>  			   card->ext_csd.generic_cmd6_time,
> -			   true, send_status, true);
> +			   true, false, true);
>  	if (err) {
>  		pr_err("%s: switch to high-speed from hs200 failed, err:%d\n",
>  			mmc_hostname(host), err);
> @@ -1090,11 +1086,9 @@ static int mmc_select_hs400(struct mmc_card *card)
>  	/* Set host controller to HS timing */
>  	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
>  
> -	if (!send_status) {
> -		err = mmc_switch_status(card);
> -		if (err)
> -			goto out_err;
> -	}
> +	err = mmc_switch_status(card);
> +	if (err)
> +		goto out_err;
>  
>  	/* Switch card to DDR */
>  	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> @@ -1113,7 +1107,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>  	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>  			   EXT_CSD_HS_TIMING, val,
>  			   card->ext_csd.generic_cmd6_time,
> -			   true, send_status, true);
> +			   true, false, true);
>  	if (err) {
>  		pr_err("%s: switch to hs400 failed, err:%d\n",
>  			 mmc_hostname(host), err);
> @@ -1124,11 +1118,9 @@ static int mmc_select_hs400(struct mmc_card *card)
>  	mmc_set_timing(host, MMC_TIMING_MMC_HS400);
>  	mmc_set_bus_speed(card);
>  
> -	if (!send_status) {
> -		err = mmc_switch_status(card);
> -		if (err)
> -			goto out_err;
> -	}
> +	err = mmc_switch_status(card);
> +	if (err)
> +		goto out_err;
>  
>  	return 0;
>  
> @@ -1146,14 +1138,10 @@ int mmc_hs200_to_hs400(struct mmc_card *card)
>  int mmc_hs400_to_hs200(struct mmc_card *card)
>  {
>  	struct mmc_host *host = card->host;
> -	bool send_status = true;
>  	unsigned int max_dtr;
>  	int err;
>  	u8 val;
>  
> -	if (host->caps & MMC_CAP_WAIT_WHILE_BUSY)
> -		send_status = false;
> -
>  	/* Reduce frequency to HS */
>  	max_dtr = card->ext_csd.hs_max_dtr;
>  	mmc_set_clock(host, max_dtr);
> @@ -1162,49 +1150,43 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
>  	val = EXT_CSD_TIMING_HS;
>  	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
>  			   val, card->ext_csd.generic_cmd6_time,
> -			   true, send_status, true);
> +			   true, false, true);
>  	if (err)
>  		goto out_err;
>  
>  	mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
>  
> -	if (!send_status) {
> -		err = mmc_switch_status(card);
> -		if (err)
> -			goto out_err;
> -	}
> +	err = mmc_switch_status(card);
> +	if (err)
> +		goto out_err;
>  
>  	/* Switch HS DDR to HS */
>  	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH,
>  			   EXT_CSD_BUS_WIDTH_8, card->ext_csd.generic_cmd6_time,
> -			   true, send_status, true);
> +			   true, false, true);
>  	if (err)
>  		goto out_err;
>  
>  	mmc_set_timing(host, MMC_TIMING_MMC_HS);
>  
> -	if (!send_status) {
> -		err = mmc_switch_status(card);
> -		if (err)
> -			goto out_err;
> -	}
> +	err = mmc_switch_status(card);
> +	if (err)
> +		goto out_err;
>  
>  	/* Switch HS to HS200 */
>  	val = EXT_CSD_TIMING_HS200 |
>  	      card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
>  	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
>  			   val, card->ext_csd.generic_cmd6_time, true,
> -			   send_status, true);
> +			   false, true);
>  	if (err)
>  		goto out_err;
>  
>  	mmc_set_timing(host, MMC_TIMING_MMC_HS200);
>  
> -	if (!send_status) {
> -		err = mmc_switch_status(card);
> -		if (err)
> -			goto out_err;
> -	}
> +	err = mmc_switch_status(card);
> +	if (err)
> +		goto out_err;
>  
>  	mmc_set_bus_speed(card);
>  
> @@ -1243,7 +1225,6 @@ static void mmc_select_driver_type(struct mmc_card *card)
>  static int mmc_select_hs200(struct mmc_card *card)
>  {
>  	struct mmc_host *host = card->host;
> -	bool send_status = true;
>  	unsigned int old_timing;
>  	int err = -EINVAL;
>  	u8 val;
> @@ -1260,9 +1241,6 @@ static int mmc_select_hs200(struct mmc_card *card)
>  
>  	mmc_select_driver_type(card);
>  
> -	if (host->caps & MMC_CAP_WAIT_WHILE_BUSY)
> -		send_status = false;
> -
>  	/*
>  	 * Set the bus width(4 or 8) with host's support and
>  	 * switch to HS200 mode if bus width is set successfully.
> @@ -1274,20 +1252,18 @@ static int mmc_select_hs200(struct mmc_card *card)
>  		err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>  				   EXT_CSD_HS_TIMING, val,
>  				   card->ext_csd.generic_cmd6_time,
> -				   true, send_status, true);
> +				   true, false, true);
>  		if (err)
>  			goto err;
>  		old_timing = host->ios.timing;
>  		mmc_set_timing(host, MMC_TIMING_MMC_HS200);
> -		if (!send_status) {
> -			err = mmc_switch_status(card);
> -			/*
> -			 * mmc_select_timing() assumes timing has not changed if
> -			 * it is a switch error.
> -			 */
> -			if (err == -EBADMSG)
> -				mmc_set_timing(host, old_timing);
> -		}
> +		err = mmc_switch_status(card);
> +		/*
> +		 * mmc_select_timing() assumes timing has not changed if
> +		 * it is a switch error.
> +		 */
> +		if (err == -EBADMSG)
> +			mmc_set_timing(host, old_timing);
>  	}
>  err:
>  	if (err)
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 62355bd..32de144 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -480,6 +480,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>  	u32 status = 0;
>  	bool use_r1b_resp = use_busy_signal;
>  	bool expired = false;
> +	bool busy = false;
>  
>  	mmc_retune_hold(host);
>  
> @@ -535,19 +536,24 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>  	/* Must check status to be sure of no errors. */
>  	timeout = jiffies + msecs_to_jiffies(timeout_ms);
>  	do {
> +		/*
> +		 * Due to the possibility of being preempted after
> +		 * sending the status command, check the expiration
> +		 * time first.
> +		 */
> +		expired = time_after(jiffies, timeout);
>  		if (send_status) {
> -			/*
> -			 * Due to the possibility of being preempted after
> -			 * sending the status command, check the expiration
> -			 * time first.
> -			 */
> -			expired = time_after(jiffies, timeout);
>  			err = __mmc_send_status(card, &status, ignore_crc);
>  			if (err)
>  				goto out;
>  		}
>  		if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp)
>  			break;
> +		if (host->ops->card_busy) {

card_busy is a problem because it is designed for SD voltage switch which
checks DAT[3:0] whereas the busy signal is only DAT0.

> +			if (!host->ops->card_busy(host))
> +				break;
> +			busy = true;
> +		}
>  		if (mmc_host_is_spi(host))
>  			break;

I would tend to put all the changes together here after the
mmc_host_is_spi() check i.e.

		if (host->ops->card_busy) {
			expired = time_after(jiffies, timeout);
			if (!host->ops->card_busy(host))
				break;
			if (!expired) {
				cond_resched();
				continue;
			}
			pr_err("%s: Card stuck in busy state! %s\n",
			       mmc_hostname(host), __func__);
			err = -ETIMEDOUT;
			goto out;
		}

Also I think the changes to __mmc_switch() should be a separate patch.
However you can't go forward with this until you sort out what card_busy means.

>  
> @@ -556,19 +562,20 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>  		 * does'nt support MMC_CAP_WAIT_WHILE_BUSY, then we can only
>  		 * rely on waiting for the stated timeout to be sufficient.
>  		 */
> -		if (!send_status) {
> +		if (!send_status && !host->ops->card_busy) {
>  			mmc_delay(timeout_ms);
>  			goto out;
>  		}
>  
>  		/* Timeout if the device never leaves the program state. */
> -		if (expired && R1_CURRENT_STATE(status) == R1_STATE_PRG) {
> +		if (expired &&
> +		    (R1_CURRENT_STATE(status) == R1_STATE_PRG || busy)) {
>  			pr_err("%s: Card stuck in programming state! %s\n",
>  				mmc_hostname(host), __func__);
>  			err = -ETIMEDOUT;
>  			goto out;
>  		}
> -	} while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
> +	} while (R1_CURRENT_STATE(status) == R1_STATE_PRG || busy);
>  
>  	err = mmc_switch_status_error(host, status);
>  out:
>