Hi Andrew,

On Mar 19, 2015, at 14:44 , Andrew Gabbasov andrew_gabbasov@mentor.com wrote:

The version flag constant name used in IS_MMC macro is incorrect/undefined.

Signed-off-by: Andrew Gabbasov andrew_gabbasov@mentor.com

include/mmc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/mmc.h b/include/mmc.h index 2ad0f19..a251531 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -61,7 +61,7 @@ #define SD_DATA_4BIT 0x00040000

#define IS_SD(x) ((x)->version & SD_VERSION_SD) -#define IS_MMC(x) ((x)->version & SD_VERSION_MMC) +#define IS_MMC(x) ((x)->version & MMC_VERSION_MMC)

#define MMC_DATA_READ 1

#define MMC_DATA_WRITE 2

2.1.0

Thanks, applied.

BTW, please make sure you CC me personally when sending patches since this slipped out of my attention.

Regards

— Pantelis

Hi Andrw,

On Mar 19, 2015, at 14:44 , Andrew Gabbasov andrew_gabbasov@mentor.com wrote:

The 'op_cond_response' field in mmc structure contains the response from the last SEND_OP_COND MMC command while making iterational polling of the card. Later it is copied to 'ocr' field, designed to contain the OCR register value, which is actually the same response from the same command. So, these fields have actually the same data, just in different time periods. It's easier to use the same 'ocr' field in both cases at once, without temporary using of the 'op_cond_response' field.

Signed-off-by: Andrew Gabbasov andrew_gabbasov@mentor.com

drivers/mmc/mmc.c | 13 +++++++------ include/mmc.h | 1 - 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index a13769e..fe00a19 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -362,8 +362,8 @@ static int mmc_send_op_cond_iter(struct mmc *mmc, struct mmc_cmd *cmd, if (use_arg && !mmc_host_is_spi(mmc)) { cmd->cmdarg = (mmc->cfg->voltages &

• 	(mmc->op_cond_response & OCR_VOLTAGE_MASK)) |
  

• 	(mmc->op_cond_response & OCR_ACCESS_MODE);
  

• 	(mmc->ocr & OCR_VOLTAGE_MASK)) |
  

• 	(mmc->ocr & OCR_ACCESS_MODE);
  
  

  if (mmc->cfg->host_caps & MMC_MODE_HC) cmd->cmdarg |= OCR_HCS;

@@ -371,7 +371,7 @@ static int mmc_send_op_cond_iter(struct mmc *mmc, struct mmc_cmd *cmd, err = mmc_send_cmd(mmc, cmd, NULL); if (err) return err;

• mmc->op_cond_response = cmd->response[0];

• mmc->ocr = cmd->response[0]; return 0;

}

@@ -391,7 +391,7 @@ static int mmc_send_op_cond(struct mmc *mmc) return err;

/* exit if not busy (flag seems to be inverted) */

• if (mmc->op_cond_response & OCR_BUSY)
  

• if (mmc->ocr & OCR_BUSY)
  return 0;
  

  } return IN_PROGRESS;

@@ -413,7 +413,7 @@ static int mmc_complete_op_cond(struct mmc *mmc) if (get_timer(start) > timeout) return UNUSABLE_ERR; udelay(100);

• } while (!(mmc->op_cond_response & OCR_BUSY));

• } while (!(mmc->ocr & OCR_BUSY));

  if (mmc_host_is_spi(mmc)) { /* read OCR for spi */ cmd.cmdidx = MMC_CMD_SPI_READ_OCR;

@@ -424,10 +424,11 @@ static int mmc_complete_op_cond(struct mmc *mmc)

if (err)
	return err;

• mmc->ocr = cmd.response[0];
  

  }

  mmc->version = MMC_VERSION_UNKNOWN;

• mmc->ocr = cmd.response[0];

  mmc->high_capacity = ((mmc->ocr & OCR_HCS) == OCR_HCS); mmc->rca = 1;

diff --git a/include/mmc.h b/include/mmc.h index a251531..644e3fa 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -356,7 +356,6 @@ struct mmc { char op_cond_pending; /* 1 if we are waiting on an op_cond command */ char init_in_progress; /* 1 if we have done mmc_start_init() */ char preinit; /* start init as early as possible */

• uint op_cond_response; /* the response byte from the last op_cond */ int ddr_mode;

};

-- 2.1.0

Thanks, applied.

Regards

— Pantelis

Hi Andrew,

On Mar 19, 2015, at 14:44 , Andrew Gabbasov andrew_gabbasov@mentor.com wrote:

The previous change to use 'ocr' structure field for storing send_op_cond command response also stopped using command response directly outside of mmc_send_op_cond_iter(). Now it becomes possible to use command structure in mmc_send_op_cond_iter() locally, removing a necessity to pass it as an argument from the caller.

Signed-off-by: Andrew Gabbasov andrew_gabbasov@mentor.com

drivers/mmc/mmc.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index fe00a19..d073d79 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -350,34 +350,32 @@ static int sd_send_op_cond(struct mmc *mmc) return 0; }

-/* We pass in the cmd since otherwise the init seems to fail */ -static int mmc_send_op_cond_iter(struct mmc *mmc, struct mmc_cmd *cmd,

• int use_arg)
  

+static int mmc_send_op_cond_iter(struct mmc *mmc, int use_arg) {

• struct mmc_cmd cmd; int err;

• cmd->cmdidx = MMC_CMD_SEND_OP_COND;
• cmd->resp_type = MMC_RSP_R3;
• cmd->cmdarg = 0;

• cmd.cmdidx = MMC_CMD_SEND_OP_COND;
• cmd.resp_type = MMC_RSP_R3;
• cmd.cmdarg = 0; if (use_arg && !mmc_host_is_spi(mmc)) {

• cmd->cmdarg =
  

• cmd.cmdarg =
  (mmc->cfg->voltages &
  (mmc->ocr & OCR_VOLTAGE_MASK)) |
  (mmc->ocr & OCR_ACCESS_MODE);
  
  

  if (mmc->cfg->host_caps & MMC_MODE_HC)

• 	cmd->cmdarg |= OCR_HCS;
  

• 	cmd.cmdarg |= OCR_HCS;
  

  }

• err = mmc_send_cmd(mmc, cmd, NULL);

• err = mmc_send_cmd(mmc, &cmd, NULL); if (err) return err;

• mmc->ocr = cmd->response[0];

• mmc->ocr = cmd.response[0]; return 0;

}

static int mmc_send_op_cond(struct mmc *mmc) {

• struct mmc_cmd cmd; int err, i;

  /* Some cards seem to need this */

@@ -386,7 +384,7 @@ static int mmc_send_op_cond(struct mmc *mmc) /* Asking to the card its capabilities */ mmc->op_cond_pending = 1; for (i = 0; i < 2; i++) {

• err = mmc_send_op_cond_iter(mmc, &cmd, i != 0);
  

• err = mmc_send_op_cond_iter(mmc, i != 0);
  

  if (err) return err;

@@ -407,7 +405,7 @@ static int mmc_complete_op_cond(struct mmc *mmc) mmc->op_cond_pending = 0; start = get_timer(0); do {

• err = mmc_send_op_cond_iter(mmc, &cmd, 1);
  

• err = mmc_send_op_cond_iter(mmc, 1);
  

  if (err) return err; if (get_timer(start) > timeout)

-- 2.1.0

Thanks, Applied.

— Pantelis

Hi, Andrew

There is already a patch to fix this issue. Patchwork: https://patchwork.ozlabs.org/patch/451775/

Regards, Peng.

-----Original Message----- From: U-Boot [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Andrew Gabbasov Sent: Thursday, March 19, 2015 8:44 PM To: u-boot@lists.denx.de Subject: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

Some MMC cards come to ready state quite quickly, so that the respective flag appears to be set in mmc_send_op_cond already. In this case trying to continue polling the card with CMD1 in mmc_complete_op_cond is incorrect and may lead to unpredictable results. So check the flag before polling and skip it appropriately.

Signed-off-by: Andrew Gabbasov andrew_gabbasov@mentor.com --- drivers/mmc/mmc.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index d073d79..42af47c 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -403,15 +403,17 @@ static int mmc_complete_op_cond(struct mmc *mmc) int err;

mmc->op_cond_pending = 0; - start = get_timer(0); - do { - err = mmc_send_op_cond_iter(mmc, 1); - if (err) - return err; - if (get_timer(start) > timeout) - return UNUSABLE_ERR; - udelay(100); - } while (!(mmc->ocr & OCR_BUSY)); + if (!(mmc->ocr & OCR_BUSY)) { + start = get_timer(0); + do { + err = mmc_send_op_cond_iter(mmc, 1); + if (err) + return err; + if (get_timer(start) > timeout) + return UNUSABLE_ERR; + udelay(100); + } while (!(mmc->ocr & OCR_BUSY)); + }

if (mmc_host_is_spi(mmc)) { /* read OCR for spi */ cmd.cmdidx = MMC_CMD_SPI_READ_OCR; -- 2.1.0

_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

Hi Andrew,

On Mar 19, 2015, at 14:44 , Andrew Gabbasov andrew_gabbasov@mentor.com wrote:

Some MMC cards come to ready state quite quickly, so that the respective flag appears to be set in mmc_send_op_cond already. In this case trying to continue polling the card with CMD1 in mmc_complete_op_cond is incorrect and may lead to unpredictable results. So check the flag before polling and skip it appropriately.

Signed-off-by: Andrew Gabbasov andrew_gabbasov@mentor.com

drivers/mmc/mmc.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index d073d79..42af47c 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -403,15 +403,17 @@ static int mmc_complete_op_cond(struct mmc *mmc) int err;

mmc->op_cond_pending = 0;

• start = get_timer(0);
• do {

• err = mmc_send_op_cond_iter(mmc, 1);
  

• if (err)
  

• 	return err;
  

• if (get_timer(start) > timeout)
  

• 	return UNUSABLE_ERR;
  

• udelay(100);
  

• } while (!(mmc->ocr & OCR_BUSY));

• if (!(mmc->ocr & OCR_BUSY)) {

• start = get_timer(0);
  

• do {
  

• 	err = mmc_send_op_cond_iter(mmc, 1);
  

• 	if (err)
  

• 		return err;
  

• 	if (get_timer(start) > timeout)
  

• 		return UNUSABLE_ERR;
  

• 	udelay(100);
  

• } while (!(mmc->ocr & OCR_BUSY));
  

• }

  if (mmc_host_is_spi(mmc)) { /* read OCR for spi */ cmd.cmdidx = MMC_CMD_SPI_READ_OCR;

-- 2.1.0

Thanks, Applied.

— Pantelis

PS. Peng, I’m picking up this instead of your patch.

Hi Andrew,

On Mar 19, 2015, at 14:44 , Andrew Gabbasov andrew_gabbasov@mentor.com wrote:

The polling loops in sd_send_op_cond and mmc_complete_op_cond functions check the ready flag state at the end of the loop, that is after executing a delay inside the loop, which, in case of exiting with no error, is not needed. Also, one of these loops, as well as the loop in mmc_send_status, have the delay just before exiting on timeout conditions.

Restructure all these loops to check the respective conditions before making a delay for the next loop pass, and to appropriately exit without the delay.

Signed-off-by: Andrew Gabbasov andrew_gabbasov@mentor.com

drivers/mmc/mmc.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 42af47c..b81533c 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -118,7 +118,7 @@ int mmc_send_status(struct mmc *mmc, int timeout) if (!mmc_host_is_spi(mmc)) cmd.cmdarg = mmc->rca << 16;

• do {

• while (1) { err = mmc_send_cmd(mmc, &cmd, NULL); if (!err) { if ((cmd.response[0] & MMC_STATUS_RDY_FOR_DATA) &&

@@ -135,9 +135,11 @@ int mmc_send_status(struct mmc *mmc, int timeout) } else if (--retries < 0) return err;

• udelay(1000);
  

• if (timeout-- <= 0)
  

• 	break;
  
  

• } while (timeout--);

• udelay(1000);
  

• }

#ifdef CONFIG_MMC_TRACE status = (cmd.response[0] & MMC_STATUS_CURR_STATE) >> 9; @@ -291,7 +293,7 @@ static int sd_send_op_cond(struct mmc *mmc) int err; struct mmc_cmd cmd;

• do {

• while (1) { cmd.cmdidx = MMC_CMD_APP_CMD; cmd.resp_type = MMC_RSP_R1; cmd.cmdarg = 0;

@@ -322,11 +324,14 @@ static int sd_send_op_cond(struct mmc *mmc) if (err) return err;

• udelay(1000);
  

• } while ((!(cmd.response[0] & OCR_BUSY)) && timeout--);

• if (cmd.response[0] & OCR_BUSY)
  

• 	break;
  
  

• if (timeout <= 0)

• return UNUSABLE_ERR;
  

• if (timeout-- <= 0)
  

• 	return UNUSABLE_ERR;
  

• udelay(1000);
  

• }

  if (mmc->version != SD_VERSION_2) mmc->version = SD_VERSION_1_0;

@@ -405,14 +410,16 @@ static int mmc_complete_op_cond(struct mmc *mmc) mmc->op_cond_pending = 0; if (!(mmc->ocr & OCR_BUSY)) { start = get_timer(0);

• do {
  

• while (1) {
  err = mmc_send_op_cond_iter(mmc, 1);
  if (err)
  	return err;
  

• 	if (mmc->ocr & OCR_BUSY)
  

• 		break;
  if (get_timer(start) > timeout)
  	return UNUSABLE_ERR;
  udelay(100);
  

• } while (!(mmc->ocr & OCR_BUSY));
  

• }
  

  }

  if (mmc_host_is_spi(mmc)) { /* read OCR for spi */

-- 2.1.0

Thanks, Applied.

— Pantelis

Hi Andrew,

On Mar 19, 2015, at 14:44 , Andrew Gabbasov andrew_gabbasov@mentor.com wrote:

Starting part of device initialization sets the init_in_progress flag only if the MMC card did not yet come to ready state and needs to continue polling. If the card is SD or if the MMC card became ready quickly, the flag is not set and (if using pre-initialization) the starting phase will be re-executed from mmc_init function.

Set the init_in_progress flag in all non-error cases. Also, move flags setting statements around so that the flags are not set in error paths. Also, IN_PROGRESS return status becomes unnecessary, so get rid of it.

Signed-off-by: Andrew Gabbasov andrew_gabbasov@mentor.com

drivers/mmc/mmc.c | 16 ++++++++-------- include/mmc.h | 3 +-- 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index b81533c..31f8647 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -387,7 +387,6 @@ static int mmc_send_op_cond(struct mmc *mmc) mmc_go_idle(mmc);

/* Asking to the card its capabilities */

• mmc->op_cond_pending = 1; for (i = 0; i < 2; i++) { err = mmc_send_op_cond_iter(mmc, i != 0); if (err)

@@ -395,9 +394,10 @@ static int mmc_send_op_cond(struct mmc *mmc)

/* exit if not busy (flag seems to be inverted) */
if (mmc->ocr & OCR_BUSY)

• 	return 0;
  

• 	break;
  

  }

• return IN_PROGRESS;

• mmc->op_cond_pending = 1;
• return 0;

}

static int mmc_complete_op_cond(struct mmc *mmc) @@ -1627,7 +1627,7 @@ int mmc_start_init(struct mmc *mmc) if (err == TIMEOUT) { err = mmc_send_op_cond(mmc);

• if (err && err != IN_PROGRESS) {
  

• if (err) {
  

#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT) printf("Card did not respond to voltage select!\n"); #endif @@ -1635,7 +1635,7 @@ int mmc_start_init(struct mmc *mmc) } }

• if (err == IN_PROGRESS)

• if (!err) mmc->init_in_progress = 1;

  return err;

@@ -1645,6 +1645,7 @@ static int mmc_complete_init(struct mmc *mmc) { int err = 0;

• mmc->init_in_progress = 0; if (mmc->op_cond_pending) err = mmc_complete_op_cond(mmc);

@@ -1654,13 +1655,12 @@ static int mmc_complete_init(struct mmc *mmc) mmc->has_init = 0; else mmc->has_init = 1;

• mmc->init_in_progress = 0; return err;

}

int mmc_init(struct mmc *mmc) {

• int err = IN_PROGRESS;

• int err = 0; unsigned start;

  if (mmc->has_init)

@@ -1671,7 +1671,7 @@ int mmc_init(struct mmc *mmc) if (!mmc->init_in_progress) err = mmc_start_init(mmc);

• if (!err || err == IN_PROGRESS)

• if (!err) err = mmc_complete_init(mmc); debug("%s: %d, time %lu\n", __func__, err, get_timer(start)); return err;

diff --git a/include/mmc.h b/include/mmc.h index 644e3fa..fbcbe35 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -70,8 +70,7 @@ #define UNUSABLE_ERR -17 /* Unusable Card */ #define COMM_ERR -18 /* Communications Error */ #define TIMEOUT -19 -#define IN_PROGRESS -20 /* operation is in progress */ -#define SWITCH_ERR -21 /* Card reports failure to switch mode */ +#define SWITCH_ERR -20 /* Card reports failure to switch mode */

#define MMC_CMD_GO_IDLE_STATE 0

#define MMC_CMD_SEND_OP_COND 1

2.1.0

Thanks, Applied.

— Pantelis