Re: [PATCH] mmc:core: fix hs400 timing selection

[Adrian Hunter]

On 01/11/14 09:23, Chanho Min wrote:
>>> So change the operating mode and to lower the clock.
>>
>> I meant something different.
>>
>> With your patch I find that __mmc_switch() -> send_status() fails.
>>
>> So I suggest something like this:
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 92247c2..195d48a 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1045,21 +1045,28 @@ static int mmc_select_hs400(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, false);
>> +	if (!err) {
>> +		u32 status;
>> +
>> +		/*
>> +		 * According to JEDEC v5.01 spec (6.6.5), Clock frequency should
>> +		 * be set to a value not greater than 52MHz after the HS_TIMING
>> +		 * field is set to 0x1.
>> +		*/
>> +		mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
>> +		mmc_set_bus_speed(card);
>> +
>> +		err = __mmc_send_status(card, &status, false, 1);
> 
> Why was retry-count changed to 1?
> Is there any reason not to use this?
> +               err = mmc_send_status(card, &status);

Error bits are cleared when read, so if the status command is not
successful then error information may have been lost.  So it does
not make sense to retry.

> 
> 
>> +		if (!err)
>> +			err = mmc_check_switch_status(card->host, status);
>> +	}
>>  	if (err) {
>>  		pr_err("%s: switch to high-speed from hs200 failed, err:%d\n",
>>  			mmc_hostname(host), err);
>>  		return err;
>>  	}
>>
>> -	/*
>> -	 * According to JEDEC v5.01 spec (6.6.5), Clock frequency should
>> -	 * be set to a value not greater than 52MHz after the HS_TIMING
>> -	 * field is set to 0x1.
>> -	 */
>> -	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
>> -	mmc_set_bus_speed(card);
>> -
>>  	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>  			 EXT_CSD_BUS_WIDTH,
>>  			 EXT_CSD_DDR_BUS_WIDTH_8,
>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>> index 23aa3a3..f917527 100644
>> --- a/drivers/mmc/core/mmc_ops.c
>> +++ b/drivers/mmc/core/mmc_ops.c
>> @@ -23,15 +23,12 @@
>>
>>  #define MMC_OPS_TIMEOUT_MS	(10 * 60 * 1000) /* 10 minute timeout */
>>
>> -static inline int __mmc_send_status(struct mmc_card *card, u32 *status,
>> -				    bool ignore_crc)
>> +int __mmc_send_status(struct mmc_card *card, u32 *status, bool ignore_crc,
>> +		      int retries)
>>  {
>>  	int err;
>>  	struct mmc_command cmd = {0};
>>
>> -	BUG_ON(!card);
>> -	BUG_ON(!card->host);
>> -
>>  	cmd.opcode = MMC_SEND_STATUS;
>>  	if (!mmc_host_is_spi(card->host))
>>  		cmd.arg = card->rca << 16;
>> @@ -39,7 +36,7 @@ static inline int __mmc_send_status(struct mmc_card *card, u32 *status,
>>  	if (ignore_crc)
>>  		cmd.flags &= ~MMC_RSP_CRC;
>>
>> -	err = mmc_wait_for_cmd(card->host, &cmd, MMC_CMD_RETRIES);
>> +	err = mmc_wait_for_cmd(card->host, &cmd, retries);
>>  	if (err)
>>  		return err;
>>
>> @@ -54,7 +51,7 @@ static inline int __mmc_send_status(struct mmc_card *card, u32 *status,
>>
>>  int mmc_send_status(struct mmc_card *card, u32 *status)
>>  {
>> -	return __mmc_send_status(card, status, false);
>> +	return __mmc_send_status(card, status, false, MMC_CMD_RETRIES);
>>  }
>>
>>  static int _mmc_select_card(struct mmc_host *host, struct mmc_card *card)
>> @@ -419,6 +416,21 @@ int mmc_spi_set_crc(struct mmc_host *host, int use_crc)
>>  	return err;
>>  }
>>
>> +int mmc_check_switch_status(struct mmc_host *host, u32 status)
>> +{
>> +	if (mmc_host_is_spi(host)) {
>> +		if (status & R1_SPI_ILLEGAL_COMMAND)
>> +			return -EBADMSG;
>> +	} else {
>> +		if (status & 0xFDFFA000)
>> +			pr_warn("%s: unexpected status %#x after switch\n",
>> +				mmc_hostname(host), status);
>> +		if (status & R1_SWITCH_ERROR)
>> +			return -EBADMSG;
>> +	}
>> +	return 0;
>> +}
>> +
>>  /**
>>   *	__mmc_switch - modify EXT_CSD register
>>   *	@card: the MMC card associated with the data transfer
>> @@ -497,7 +509,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>>  	timeout = jiffies + msecs_to_jiffies(timeout_ms);
>>  	do {
>>  		if (send_status) {
>> -			err = __mmc_send_status(card, &status, ignore_crc);
>> +			err = __mmc_send_status(card, &status, ignore_crc, 1);
>>  			if (err)
>>  				return err;
>>  		}
>> @@ -524,18 +536,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>>  		}
>>  	} while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
>>
>> -	if (mmc_host_is_spi(host)) {
>> -		if (status & R1_SPI_ILLEGAL_COMMAND)
>> -			return -EBADMSG;
>> -	} else {
>> -		if (status & 0xFDFFA000)
>> -			pr_warn("%s: unexpected status %#x after switch\n",
>> -				mmc_hostname(host), status);
>> -		if (status & R1_SWITCH_ERROR)
>> -			return -EBADMSG;
>> -	}
>> -
>> -	return 0;
>> +	return mmc_check_switch_status(host, status);
>>  }
>>  EXPORT_SYMBOL_GPL(__mmc_switch);
>>
>> diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
>> index 6f4b00e..a59319c 100644
>> --- a/drivers/mmc/core/mmc_ops.h
>> +++ b/drivers/mmc/core/mmc_ops.h
>> @@ -20,7 +20,10 @@ int mmc_send_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr);
>>  int mmc_all_send_cid(struct mmc_host *host, u32 *cid);
>>  int mmc_set_relative_addr(struct mmc_card *card);
>>  int mmc_send_csd(struct mmc_card *card, u32 *csd);
>> +int __mmc_send_status(struct mmc_card *card, u32 *status, bool ignore_crc,
>> +		      int retries);
>>  int mmc_send_status(struct mmc_card *card, u32 *status);
>> +int mmc_check_switch_status(struct mmc_host *host, u32 status);
>>  int mmc_send_cid(struct mmc_host *host, u32 *cid);
>>  int mmc_spi_read_ocr(struct mmc_host *host, int highcap, u32 *ocrp);
>>  int mmc_spi_set_crc(struct mmc_host *host, int use_crc);
> 
> Thanks
> Chanho
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/