[U-Boot] [PATCH 0/6] mmc: Fix OCR polling and splitted initialization

[U-Boot] [PATCH 0/6] mmc: Fix OCR polling and splitted initialization

[Andrew Gabbasov]

Patch 4 contains a fix for a problem that really occures with some
MMC cards, that are capable to get to ready state within a single
polling call.

Patch 6 is a fix for an error, that may be not so important and is not
visible at the moment, since no platform does actually use pre-initialization.
However, it still makes sense to fix it. It also makes the code more
clean, straightforward, and understandable.

Patch 5 eliminates some unneeded delays in polling loops, thus improving
overall performance.

Patches 2 and 3 clean up the code, making it simpler and more correct.

Patch 1 is actually not directly related to the patched code area, but just
uses the occasion to fix the typo.

This is a series of patches, the next patch depending on previous ones,
so that should be applied in order, on top of previous ones.

Andrew Gabbasov (6):
  mmc: Fix typo in MMC type checking macro
  mmc: Avoid extra duplicate entry in mmc device structure
  mmc: Do not pass external mmc_cmd structure to mmc_send_op_cond_iter()
  mmc: Continue polling MMC card for OCR only if it is still not ready
  mmc: Restructure polling loops to avoid extra delays
  mmc: Fix splitting device initialization

 drivers/mmc/mmc.c | 90 ++++++++++++++++++++++++++++++-------------------------
 include/mmc.h     |  6 ++--
 2 files changed, 51 insertions(+), 45 deletions(-)

-- 
2.1.0

[U-Boot] [PATCH 1/6] mmc: Fix typo in MMC type checking macro

[Andrew Gabbasov]

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

[U-Boot] [PATCH 2/6] mmc: Avoid extra duplicate entry in mmc device structure

[Andrew Gabbasov]

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

[U-Boot] [PATCH 3/6] mmc: Do not pass external mmc_cmd structure to mmc_send_op_cond_iter()

[Andrew Gabbasov]

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

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Andrew Gabbasov]

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] [PATCH 5/6] mmc: Restructure polling loops to avoid extra delays

[Andrew Gabbasov]

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

[U-Boot] [PATCH 6/6] mmc: Fix splitting device initialization

[Andrew Gabbasov]

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

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Peng.Fan at freescale.com]

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 at lists.denx.de] On Behalf Of Andrew Gabbasov
Sent: Thursday, March 19, 2015 8:44 PM
To: u-boot at 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 at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Troy Kisky]

On 3/19/2015 6:51 PM, Peng.Fan at freescale.com wrote:
> 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 at lists.denx.de] On Behalf Of Andrew Gabbasov
> Sent: Thursday, March 19, 2015 8:44 PM
> To: u-boot at 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;
> --


Here's another patch that solves the problem a little earlier. It has this disadvantage of being
slightly bigger,
though it makes the code look better.


https://github.com/boundarydevices/u-boot-imx6/commit/c0260ca


I'll ack any version as they all seem to solve the problem.

Troy

[U-Boot] [PATCH 1/6] mmc: Fix typo in MMC type checking macro

[Pantelis Antoniou]

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

[U-Boot] [PATCH 2/6] mmc: Avoid extra duplicate entry in mmc device structure

[Pantelis Antoniou]

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

[U-Boot] [PATCH 3/6] mmc: Do not pass external mmc_cmd structure to mmc_send_op_cond_iter()

[Pantelis Antoniou]

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

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Pantelis Antoniou]

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.

[U-Boot] [PATCH 5/6] mmc: Restructure polling loops to avoid extra delays

[Pantelis Antoniou]

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

[U-Boot] [PATCH 6/6] mmc: Fix splitting device initialization

[Pantelis Antoniou]

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

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Pantelis Antoniou]

Hi Andrew,

> On Mar 24, 2015, at 21:02 , Andrew Gabbasov <andrew_gabbasov@mentor.com> wrote:
> 
>> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
>> Sent: Tuesday, March 24, 2015 9:03 PM
>> To: Gabbasov, Andrew; Peng.Fan at freescale.com; u-boot at lists.denx.de
>> Cc: Eric Nelson
>> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR
>> only if it is still not ready
>> 
>> On 3/24/2015 9:33 AM, Andrew Gabbasov wrote:
>>> Hi Troy,
>>> 
>>>> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
>>>> Sent: Monday, March 23, 2015 11:09 PM
>>>> To: Gabbasov, Andrew; Peng.Fan at freescale.com; u-boot at lists.denx.de
>>>> Cc: Eric Nelson
>>>> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for
>>>> OCR only if it is still not ready
>>>> 
>>>> On 3/23/2015 12:38 AM, Andrew Gabbasov wrote:
>>>>> Hi Troy,
>>>>> 
>>>>>> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
>>>>>> Sent: Friday, March 20, 2015 9:39 PM
>>>>>> To: Peng.Fan at freescale.com; Gabbasov, Andrew; u-boot at lists.denx.de
>>>>>> Cc: Eric Nelson
>>>>>> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card
>>>>>> for OCR only if it is still not ready
>>>>>> 
>>>>>> [skipped]
>>>>>> 
>>>>>> Here's another patch that solves the problem a little earlier. It
>>>>>> has this disadvantage of being slightly bigger, though it makes the
>>>>>> code look
>>>>> better.
>>>>>> 
>>>>>> https://github.com/boundarydevices/u-boot-imx6/commit/c0260ca
>>>>>> 
>>>>> 
>>>>> I have a couple of doubts regarding that patch.
>>>>> 
>>>>> First, my personal taste objects to such duplicating of the code (I
>>>>> mean setting of version, ocr, rca, etc fields of mmc structure).
>>>>> If we'll have to change or add anything to these settings, we'll
>>>>> have to make the same change in 2 different place, which is
>>>>> error-prone and extremely inconvenient from maintenance point of view.
>>>>> 
>>>>> Second, what about SPI mode? Doesn't this patch skip retrieving of
>>>>> OCR register with a special command for SPI host case (thus setting
>>>>> ocr field incorrectly), if the card comes to ready state with the
>>>>> first attempt?
>>>> 
>>>> That's a good argument for a subroutine to be doing that work instead
>>>> of
>>> in two
>>>> places.
>>> 
>>> In some sense the second function of these two
>>> (mmc_complete_op_cond()) is exactly such subroutine ;-) It is doing
>>> this work of final settings and actions after making OCR polling.
>>> Although completing the polling itself in some cases too.
>>> Actually, it seems that introducing of one more service function would
>>> make the code a little too "chopped" into too small pieces, and also
>>> further less similar to SD (as opposed to MMC) case.
>>> 
>>> Thanks.
>>> 
>> 
>> I've already said that I'm fine with any patch that fixes the problem, so
> there is
>> no need to convince me of anything. I just wanted to show that setting the
>> pending flag, when the command is actually complete, does not make a lot
> of
>> sense.
> 
> I absolutely agree. In fact, this flag in current implementation just
> indicates
> the branch that the MMC card case is being processed. If the version field,
> differing SD and MMC cases, would be set a little earlier, for example,
> directly in mmc_start_init(), we could just use !IS_SD() instead of this
> pending flag. And thinking further, it's not quite clear why the splitting
> of OCR polling was done for MMC case only, but not for SD too.
> So, there is still the room for further improving the code, may be even
> some reorganizing, but I had to stop somewhere, unless there is
> the direct necessity ;-)
> 

Your patch fixes the current problem nicely. There is a lot of cruft around, we?ll
get around looking it at some point in the future (yeah, maybe) :)

Thanks for the good work, it?s fine as it is right now.

> Thanks.
> 
> Best regards,
> Andrew
> 

Regards

? Pantelis

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Andrew Gabbasov]

> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
> Sent: Tuesday, March 24, 2015 9:03 PM
> To: Gabbasov, Andrew; Peng.Fan at freescale.com; u-boot at lists.denx.de
> Cc: Eric Nelson
> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR
> only if it is still not ready
> 
> On 3/24/2015 9:33 AM, Andrew Gabbasov wrote:
> > Hi Troy,
> >
> >> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
> >> Sent: Monday, March 23, 2015 11:09 PM
> >> To: Gabbasov, Andrew; Peng.Fan at freescale.com; u-boot at lists.denx.de
> >> Cc: Eric Nelson
> >> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for
> >> OCR only if it is still not ready
> >>
> >> On 3/23/2015 12:38 AM, Andrew Gabbasov wrote:
> >>> Hi Troy,
> >>>
> >>>> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
> >>>> Sent: Friday, March 20, 2015 9:39 PM
> >>>> To: Peng.Fan at freescale.com; Gabbasov, Andrew; u-boot at lists.denx.de
> >>>> Cc: Eric Nelson
> >>>> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card
> >>>> for OCR only if it is still not ready
> >>>>
> >>>> [skipped]
> >>>>
> >>>> Here's another patch that solves the problem a little earlier. It
> >>>> has this disadvantage of being slightly bigger, though it makes the
> >>>> code look
> >>> better.
> >>>>
> >>>> https://github.com/boundarydevices/u-boot-imx6/commit/c0260ca
> >>>>
> >>>
> >>> I have a couple of doubts regarding that patch.
> >>>
> >>> First, my personal taste objects to such duplicating of the code (I
> >>> mean setting of version, ocr, rca, etc fields of mmc structure).
> >>> If we'll have to change or add anything to these settings, we'll
> >>> have to make the same change in 2 different place, which is
> >>> error-prone and extremely inconvenient from maintenance point of view.
> >>>
> >>> Second, what about SPI mode? Doesn't this patch skip retrieving of
> >>> OCR register with a special command for SPI host case (thus setting
> >>> ocr field incorrectly), if the card comes to ready state with the
> >>> first attempt?
> >>
> >> That's a good argument for a subroutine to be doing that work instead
> >> of
> > in two
> >> places.
> >
> > In some sense the second function of these two
> > (mmc_complete_op_cond()) is exactly such subroutine ;-) It is doing
> > this work of final settings and actions after making OCR polling.
> > Although completing the polling itself in some cases too.
> > Actually, it seems that introducing of one more service function would
> > make the code a little too "chopped" into too small pieces, and also
> > further less similar to SD (as opposed to MMC) case.
> >
> > Thanks.
> >
> 
> I've already said that I'm fine with any patch that fixes the problem, so
there is
> no need to convince me of anything. I just wanted to show that setting the
> pending flag, when the command is actually complete, does not make a lot
of
> sense.

I absolutely agree. In fact, this flag in current implementation just
indicates
the branch that the MMC card case is being processed. If the version field,
differing SD and MMC cases, would be set a little earlier, for example,
directly in mmc_start_init(), we could just use !IS_SD() instead of this
pending flag. And thinking further, it's not quite clear why the splitting
of OCR polling was done for MMC case only, but not for SD too.
So, there is still the room for further improving the code, may be even
some reorganizing, but I had to stop somewhere, unless there is
the direct necessity ;-)

Thanks.

Best regards,
Andrew

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Troy Kisky]

On 3/24/2015 9:33 AM, Andrew Gabbasov wrote:
> Hi Troy,
> 
>> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
>> Sent: Monday, March 23, 2015 11:09 PM
>> To: Gabbasov, Andrew; Peng.Fan at freescale.com; u-boot at lists.denx.de
>> Cc: Eric Nelson
>> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR
>> only if it is still not ready
>>
>> On 3/23/2015 12:38 AM, Andrew Gabbasov wrote:
>>> Hi Troy,
>>>
>>>> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
>>>> Sent: Friday, March 20, 2015 9:39 PM
>>>> To: Peng.Fan at freescale.com; Gabbasov, Andrew; u-boot at lists.denx.de
>>>> Cc: Eric Nelson
>>>> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for
>>>> OCR only if it is still not ready
>>>>
>>>> [skipped]
>>>>
>>>> Here's another patch that solves the problem a little earlier. It has
>>>> this disadvantage of being slightly bigger, though it makes the code
>>>> look
>>> better.
>>>>
>>>> https://github.com/boundarydevices/u-boot-imx6/commit/c0260ca
>>>>
>>>
>>> I have a couple of doubts regarding that patch.
>>>
>>> First, my personal taste objects to such duplicating of the code (I
>>> mean setting of version, ocr, rca, etc fields of mmc structure).
>>> If we'll have to change or add anything to these settings, we'll have
>>> to make the same change in 2 different place, which is error-prone and
>>> extremely inconvenient from maintenance point of view.
>>>
>>> Second, what about SPI mode? Doesn't this patch skip retrieving of OCR
>>> register with a special command for SPI host case (thus setting ocr
>>> field incorrectly), if the card comes to ready state with the first
>>> attempt?
>>
>> That's a good argument for a subroutine to be doing that work instead of
> in two
>> places.
> 
> In some sense the second function of these two (mmc_complete_op_cond())
> is exactly such subroutine ;-) It is doing this work of final settings and
> actions
> after making OCR polling. Although completing the polling itself
> in some cases too.
> Actually, it seems that introducing of one more service function would
> make the code a little too "chopped" into too small pieces, and also
> further less similar to SD (as opposed to MMC) case.
> 
> Thanks.
> 


I've already said that I'm fine with any patch that fixes the problem, so there is
no need to convince me of anything. I just wanted to show that setting the pending
flag, when the command is actually complete, does not make a lot of sense.

Thanks
Troy

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Andrew Gabbasov]

Hi Troy,

> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
> Sent: Monday, March 23, 2015 11:09 PM
> To: Gabbasov, Andrew; Peng.Fan at freescale.com; u-boot at lists.denx.de
> Cc: Eric Nelson
> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR
> only if it is still not ready
> 
> On 3/23/2015 12:38 AM, Andrew Gabbasov wrote:
> > Hi Troy,
> >
> >> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
> >> Sent: Friday, March 20, 2015 9:39 PM
> >> To: Peng.Fan at freescale.com; Gabbasov, Andrew; u-boot at lists.denx.de
> >> Cc: Eric Nelson
> >> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for
> >> OCR only if it is still not ready
> >>
> >> [skipped]
> >>
> >> Here's another patch that solves the problem a little earlier. It has
> >> this disadvantage of being slightly bigger, though it makes the code
> >> look
> > better.
> >>
> >> https://github.com/boundarydevices/u-boot-imx6/commit/c0260ca
> >>
> >
> > I have a couple of doubts regarding that patch.
> >
> > First, my personal taste objects to such duplicating of the code (I
> > mean setting of version, ocr, rca, etc fields of mmc structure).
> > If we'll have to change or add anything to these settings, we'll have
> > to make the same change in 2 different place, which is error-prone and
> > extremely inconvenient from maintenance point of view.
> >
> > Second, what about SPI mode? Doesn't this patch skip retrieving of OCR
> > register with a special command for SPI host case (thus setting ocr
> > field incorrectly), if the card comes to ready state with the first
> > attempt?
> 
> That's a good argument for a subroutine to be doing that work instead of
in two
> places.

In some sense the second function of these two (mmc_complete_op_cond())
is exactly such subroutine ;-) It is doing this work of final settings and
actions
after making OCR polling. Although completing the polling itself
in some cases too.
Actually, it seems that introducing of one more service function would
make the code a little too "chopped" into too small pieces, and also
further less similar to SD (as opposed to MMC) case.

Thanks.

Best regards,
Andrew

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Troy Kisky]

On 3/23/2015 12:38 AM, Andrew Gabbasov wrote:
> Hi Troy,
> 
>> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
>> Sent: Friday, March 20, 2015 9:39 PM
>> To: Peng.Fan at freescale.com; Gabbasov, Andrew; u-boot at lists.denx.de
>> Cc: Eric Nelson
>> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR
>> only if it is still not ready
>>
>> [skipped]
>>
>> Here's another patch that solves the problem a little earlier. It has this
>> disadvantage of being slightly bigger, though it makes the code look
> better.
>>
>> https://github.com/boundarydevices/u-boot-imx6/commit/c0260ca
>>
> 
> I have a couple of doubts regarding that patch.
> 
> First, my personal taste objects to such duplicating of the code
> (I mean setting of version, ocr, rca, etc fields of mmc structure).
> If we'll have to change or add anything to these settings, we'll have to
> make
> the same change in 2 different place, which is error-prone and extremely
> inconvenient from maintenance point of view.
> 
> Second, what about SPI mode? Doesn't this patch skip retrieving of OCR
> register
> with a special command for SPI host case (thus setting ocr field
> incorrectly),
> if the card comes to ready state with the first attempt?

That's a good argument for a subroutine to be doing that work instead
of in two places.

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Andrew Gabbasov]

Hi Troy,

> From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
> Sent: Friday, March 20, 2015 9:39 PM
> To: Peng.Fan at freescale.com; Gabbasov, Andrew; u-boot at lists.denx.de
> Cc: Eric Nelson
> Subject: Re: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR
> only if it is still not ready
> 
> [skipped]
> 
> Here's another patch that solves the problem a little earlier. It has this
> disadvantage of being slightly bigger, though it makes the code look
better.
> 
> https://github.com/boundarydevices/u-boot-imx6/commit/c0260ca
> 

I have a couple of doubts regarding that patch.

First, my personal taste objects to such duplicating of the code
(I mean setting of version, ocr, rca, etc fields of mmc structure).
If we'll have to change or add anything to these settings, we'll have to
make
the same change in 2 different place, which is error-prone and extremely
inconvenient from maintenance point of view.

Second, what about SPI mode? Doesn't this patch skip retrieving of OCR
register
with a special command for SPI host case (thus setting ocr field
incorrectly),
if the card comes to ready state with the first attempt?

Thanks.

Best regards,
Andrew

[U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR only if it is still not ready

[Andrew Gabbasov]

Hi Peng,

> From: Peng.Fan at freescale.com [Peng.Fan at freescale.com]
> Sent: Friday, March 20, 2015 03:51
> To: Gabbasov, Andrew; u-boot at lists.denx.de
> Subject: RE: [U-Boot] [PATCH 4/6] mmc: Continue polling MMC card for OCR
only if        it is still not ready
> 
> Hi, Andrew
> 
> There is already a patch to fix this issue.
> Patchwork: https://patchwork.ozlabs.org/patch/451775/
> 
> Regards,
> Peng.

Yes, I noticed it just after I sent this series. ;-)
Unfortunately, the patch, that you mention, has some drawback that I
described
in a separate message (in response to the patch). Basically, it leaves the
code
in some not quite correct state.
This series hopefully does not have this drawback and besides fixing the
issue
has some more useful changes in a single complex.

Thanks.

Best regards,
Andrew


-----Original Message-----
From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Andrew
Gabbasov
Sent: Thursday, March 19, 2015 8:44 PM
To: u-boot at 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 at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot