mmc: sd: Remove superfluous error code assignment

PATCH Applied Yousong Zhou 5 participants 11 messages Sep 9, 2015 – Sep 17, 2015 MMC, SD and SDIO

lore

Share & tools

lore

Yousong Zhou

author

2015-09-09 16:37 UTC #0 lore raw

PATCH 1/3 mmc: sd: Remove superfluous error code assignment

Signed-off-by Yousong Zhou <[email protected]>

---
 drivers/mmc/core/sd.c |    2 --
 1 file changed, 2 deletions(-)

drivers/mmc/core/sd.c +0 -2

index 4e7366a..e28ebf3 100644
@@ -357,8 +357,6 @@ int mmc_sd_switch_hs(struct mmc_card *card)
 	if (card->sw_caps.hs_max_dtr == 0)
 		return 0;
 
-	err = -EIO;
-
 	status = kmalloc(64, GFP_KERNEL);
 	if (!status) {
 		pr_err("%s: could not allocate a buffer for "

Signature · 1 line

1.7.10.4

Yousong Zhou

author

2015-09-09 16:37 UTC #1 lore raw

PATCH 2/3 mmc: sd: Allow calling sd mode switch with retries

This will allow retrying access mode switch to High-Speed in the following commit.

Signed-off-by Yousong Zhou <[email protected]>

---
 drivers/mmc/core/core.c   |    4 ++++
 drivers/mmc/core/sd_ops.c |    5 +++--
 drivers/mmc/core/sd_ops.h |   10 ++++++++--
 3 files changed, 15 insertions(+), 4 deletions(-)

drivers/mmc/core/core.c +4 -0

index 9ad73f3..e726bb1 100644
@@ -468,11 +468,13 @@ static void mmc_wait_for_req_done(struct mmc_host *host,
 				  struct mmc_request *mrq)
 {
 	struct mmc_command *cmd;
+	struct mmc_data *data;
 
 	while (1) {
 		wait_for_completion(&mrq->completion);
 
 		cmd = mrq->cmd;
+		data = mrq->data;
 
 		/*
 		 * If host has timed out waiting for the sanitize

@@ -501,6 +503,8 @@ static void mmc_wait_for_req_done(struct mmc_host *host,
 			 mmc_hostname(host), cmd->opcode, cmd->error);
 		cmd->retries--;
 		cmd->error = 0;
+		if (data)
+			data->error = 0;
 		__mmc_start_request(host, mrq);
 	}

drivers/mmc/core/sd_ops.c +3 -2

index 48d0c93..22bef3c 100644
@@ -304,8 +304,8 @@ int mmc_app_send_scr(struct mmc_card *card, u32 *scr)
 	return 0;
 }
 
-int mmc_sd_switch(struct mmc_card *card, int mode, int group,
-	u8 value, u8 *resp)
+int __mmc_sd_switch(struct mmc_card *card, int mode, int group,
+	u8 value, u8 *resp, int retries)
 {
 	struct mmc_request mrq = {NULL};
 	struct mmc_command cmd = {0};

@@ -328,6 +328,7 @@ int mmc_sd_switch(struct mmc_card *card, int mode, int group,
 	cmd.arg &= ~(0xF << (group * 4));
 	cmd.arg |= value << (group * 4);
 	cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
+	cmd.retries = retries;
 
 	data.blksz = 64;
 	data.blocks = 1;

drivers/mmc/core/sd_ops.h +8 -2

index ffc2305..a53c51e 100644
@@ -17,9 +17,15 @@ int mmc_send_app_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr);
 int mmc_send_if_cond(struct mmc_host *host, u32 ocr);
 int mmc_send_relative_addr(struct mmc_host *host, unsigned int *rca);
 int mmc_app_send_scr(struct mmc_card *card, u32 *scr);
-int mmc_sd_switch(struct mmc_card *card, int mode, int group,
-	u8 value, u8 *resp);
 int mmc_app_sd_status(struct mmc_card *card, void *ssr);
 
+int __mmc_sd_switch(struct mmc_card *card, int mode, int group,
+	u8 value, u8 *resp, int retries);
+static inline int mmc_sd_switch(struct mmc_card *card, int mode, int group,
+	u8 value, u8 *resp)
+{
+	return __mmc_sd_switch(card, mode, group, value, resp, 0);
+}
+
 #endif

Signature · 1 line

1.7.10.4

Shawn Lin

Tested-by on v1 2/3

2015-09-10 00:33 UTC #2 lore raw

Re: [PATCH 2/3] mmc: sd: Allow calling sd mode switch with retries

Quoted 10 lines · Yousong Zhou

This will allow retrying access mode switch to High-Speed in the following commit.
Signed-off-by Yousong Zhou <[email protected]>
---
  drivers/mmc/core/core.c   |    4 ++++
  drivers/mmc/core/sd_ops.c |    5 +++--
  drivers/mmc/core/sd_ops.h |   10 ++++++++--
  3 files changed, 15 insertions(+), 4 deletions(-)

I test this patchset with my already-working sd cards, seems ok.
Actually I don't have a card to test like yours that need retry switch HS, but from the changes itself, it's harmless to other cards.

so,

Tested-by Shawn Lin <[email protected]>

BTW, I can't find the cover letter? And another should be clarified, plz see comment blow

Quoted 19 lines

drivers/mmc/core/core.c +2 -0

index 9ad73f3..e726bb1 100644
@@ -468,11 +468,13 @@ static void mmc_wait_for_req_done(struct mmc_host *host,
  				  struct mmc_request *mrq)
  {
  	struct mmc_command *cmd;
+	struct mmc_data *data;
 
  	while (1) {
  		wait_for_completion(&mrq->completion);
 
  		cmd = mrq->cmd;
+		data = mrq->data;
 
  		/*
  		 * If host has timed out waiting for the sanitize

drivers/mmc/core/core.c static void mmc_wait_for_req_done(struct mmc_host *host, · line 507

Quoted 5 lines

drivers/mmc/core/core.c +2 -0

@@ -501,6 +503,8 @@ static void mmc_wait_for_req_done(struct mmc_host *host,
  			 mmc_hostname(host), cmd->opcode, cmd->error);
  		cmd->retries--;
  		cmd->error = 0;
+		if (data)
+			data->error = 0;

What's this change for?

Quoted 49 lines

drivers/mmc/core/core.c +0 -0

  		__mmc_start_request(host, mrq);
  	}

drivers/mmc/core/sd_ops.c +3 -2

index 48d0c93..22bef3c 100644
@@ -304,8 +304,8 @@ int mmc_app_send_scr(struct mmc_card *card, u32 *scr)
  	return 0;
  }
 
-int mmc_sd_switch(struct mmc_card *card, int mode, int group,
-	u8 value, u8 *resp)
+int __mmc_sd_switch(struct mmc_card *card, int mode, int group,
+	u8 value, u8 *resp, int retries)
  {
  	struct mmc_request mrq = {NULL};
  	struct mmc_command cmd = {0};

@@ -328,6 +328,7 @@ int mmc_sd_switch(struct mmc_card *card, int mode, int group,
  	cmd.arg &= ~(0xF << (group * 4));
  	cmd.arg |= value << (group * 4);
  	cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
+	cmd.retries = retries;
 
  	data.blksz = 64;
  	data.blocks = 1;

drivers/mmc/core/sd_ops.h +8 -2

index ffc2305..a53c51e 100644
@@ -17,9 +17,15 @@ int mmc_send_app_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr);
  int mmc_send_if_cond(struct mmc_host *host, u32 ocr);
  int mmc_send_relative_addr(struct mmc_host *host, unsigned int *rca);
  int mmc_app_send_scr(struct mmc_card *card, u32 *scr);
-int mmc_sd_switch(struct mmc_card *card, int mode, int group,
-	u8 value, u8 *resp);
  int mmc_app_sd_status(struct mmc_card *card, void *ssr);
 
+int __mmc_sd_switch(struct mmc_card *card, int mode, int group,
+	u8 value, u8 *resp, int retries);
+static inline int mmc_sd_switch(struct mmc_card *card, int mode, int group,
+	u8 value, u8 *resp)
+{
+	return __mmc_sd_switch(card, mode, group, value, resp, 0);
+}
+
  #endif

Signature · 2 lines

Best Regards
Shawn Lin

Jaehoon Chung

2015-09-16 02:36 UTC #4 lore raw

Re: [PATCH 2/3] mmc: sd: Allow calling sd mode switch with retries

Hi,

Quoted 5 lines · Yousong Zhou

Quoted 4 lines · Shawn Lin
Quoted 3 lines · Yousong Zhou
This will allow retrying access mode switch to High-Speed in the following commit.

Well, it's not solution. (It's my preference.)
Did you consider the card removable case?

Could you share which vendor's card needs to retry?

Best Regards,
Jaehoon Chung

Quoted 133 lines

Quoted 9 lines

Quoted 8 lines

Signed-off-by Yousong Zhou <[email protected]>

---
  drivers/mmc/core/core.c   |    4 ++++
  drivers/mmc/core/sd_ops.c |    5 +++--
  drivers/mmc/core/sd_ops.h |   10 ++++++++--
  3 files changed, 15 insertions(+), 4 deletions(-)

Quoted 3 lines

I test this patchset with my already-working sd cards, seems ok. Actually I don't have a card to test like yours that need retry switch HS, but from the changes itself, it's harmless to other cards.

Thanks for the testing.

Those cards normally won't burn out after MMC_CMD_RETRIES access mode switch failures, right?

Quoted 26 lines

so,

Tested-by Shawn Lin <[email protected]>

BTW, I can't find the cover letter? And another should be clarified, plz see comment blow

Quoted 17 lines

drivers/mmc/core/core.c +2 -0

index 9ad73f3..e726bb1 100644
@@ -468,11 +468,13 @@ static void mmc_wait_for_req_done(struct mmc_host
 *host,
                                  struct mmc_request *mrq)
  {
        struct mmc_command *cmd;
+       struct mmc_data *data;
 
        while (1) {
                wait_for_completion(&mrq->completion);
 
                cmd = mrq->cmd;
+               data = mrq->data;

Quoted 12 lines

drivers/mmc/core/core.c static void mmc_wait_for_req_done(struct mmc_host · line 508

Quoted 9 lines

drivers/mmc/core/core.c +0 -0

/*

* If host has timed out waiting for the sanitize

drivers/mmc/core/core.c +2 -0

@@ -501,6 +503,8 @@ static void mmc_wait_for_req_done(struct mmc_host
 *host,
                         mmc_hostname(host), cmd->opcode, cmd->error);
                cmd->retries--;
                cmd->error = 0;
+               if (data)
+                       data->error = 0;

What's this change for?

sunxi-mmc will set both cmd->error and data->error to -ETIMEDOUT in case of any interrupt error bit was set (related code within `sunxi_finalize_request()').

`__mmc_sd_switch()' thought it was a error case if data->error != 0.

So the data->error bit needs to be cleared before next retry.

Regards,

yousnog

Quoted 51 lines

drivers/mmc/core/core.c +0 -0

                __mmc_start_request(host, mrq);
        }

drivers/mmc/core/sd_ops.c +3 -2

index 48d0c93..22bef3c 100644
@@ -304,8 +304,8 @@ int mmc_app_send_scr(struct mmc_card *card, u32 *scr)
        return 0;
  }
 
-int mmc_sd_switch(struct mmc_card *card, int mode, int group,
-       u8 value, u8 *resp)
+int __mmc_sd_switch(struct mmc_card *card, int mode, int group,
+       u8 value, u8 *resp, int retries)
  {
        struct mmc_request mrq = {NULL};
        struct mmc_command cmd = {0};

@@ -328,6 +328,7 @@ int mmc_sd_switch(struct mmc_card *card, int mode, int
 group,
        cmd.arg &= ~(0xF << (group * 4));
        cmd.arg |= value << (group * 4);
        cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
+       cmd.retries = retries;
 
        data.blksz = 64;

        data.blocks = 1;

drivers/mmc/core/sd_ops.h +8 -2

index ffc2305..a53c51e 100644
@@ -17,9 +17,15 @@ int mmc_send_app_op_cond(struct mmc_host *host, u32
 ocr, u32 *rocr);
  int mmc_send_if_cond(struct mmc_host *host, u32 ocr);
  int mmc_send_relative_addr(struct mmc_host *host, unsigned int *rca);
  int mmc_app_send_scr(struct mmc_card *card, u32 *scr);
-int mmc_sd_switch(struct mmc_card *card, int mode, int group,
-       u8 value, u8 *resp);
  int mmc_app_sd_status(struct mmc_card *card, void *ssr);
 
+int __mmc_sd_switch(struct mmc_card *card, int mode, int group,
+       u8 value, u8 *resp, int retries);
+static inline int mmc_sd_switch(struct mmc_card *card, int mode, int
 group,
+       u8 value, u8 *resp)
+{
+       return __mmc_sd_switch(card, mode, group, value, resp, 0);
+}
+

Quoted 9 lines

Quoted 3 lines
  #endif
--
Best Regards
Shawn Lin

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Yousong Zhou

author

2015-09-16 03:09 UTC #5 lore raw

Re: [PATCH 2/3] mmc: sd: Allow calling sd mode switch with retries

Hi,

Quoted 6 lines · Jaehoon Chung

Hi,
Quoted 3 lines · Yousong Zhou
Quoted 2 lines · Shawn Lin
Quoted 1 line · Yousong Zhou

Quoted 8 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
This will allow retrying access mode switch to High-Speed in the following commit.
Well, it's not solution. (It's my preference.)
Did you consider the card removable case?
Could you share which vendor's card needs to retry?

It's a SD card labeled as "pqi" made in Korea. The issue was found on sunxi-mmc controller where it reported response crc error when trying to switch the card into highspeed mode.

The idea about retrying the access mode switch was from U-Boot code of Allwinner (vendor of sunxi SoCs). The code comments there said this issue can happen with eMMC cards from Toshiba [1].

The above info was also said in a previous discussion [1]. A sunxi-mmc specific change as suggested by Hans de Geode should definitely be a safer move. But that requires more fundamental framework code change I guess.

[1] https://github.com/allwinner-zh/bootloader/blob/master/u-boot-2011.09/drivers/mmc/mmc.c#L2220
[2] http://thread.gmane.org/gmane.comp.hardware.netbook.arm.sunxi/18442/focus=18480

Regards,

yousong

Quoted 137 lines

Best Regards,
Jaehoon Chung

Quoted 133 lines

Quoted 9 lines

Quoted 8 lines

Signed-off-by Yousong Zhou <[email protected]>

---
  drivers/mmc/core/core.c   |    4 ++++
  drivers/mmc/core/sd_ops.c |    5 +++--
  drivers/mmc/core/sd_ops.h |   10 ++++++++--
  3 files changed, 15 insertions(+), 4 deletions(-)

Quoted 3 lines

I test this patchset with my already-working sd cards, seems ok. Actually I don't have a card to test like yours that need retry switch HS, but from the changes itself, it's harmless to other cards.

Thanks for the testing.

Those cards normally won't burn out after MMC_CMD_RETRIES access mode switch failures, right?

Quoted 26 lines

so,

Tested-by Shawn Lin <[email protected]>

BTW, I can't find the cover letter? And another should be clarified, plz see comment blow

Quoted 17 lines

drivers/mmc/core/core.c +2 -0

index 9ad73f3..e726bb1 100644
@@ -468,11 +468,13 @@ static void mmc_wait_for_req_done(struct mmc_host
 *host,
                                  struct mmc_request *mrq)
  {
        struct mmc_command *cmd;
+       struct mmc_data *data;
 
        while (1) {
                wait_for_completion(&mrq->completion);
 
                cmd = mrq->cmd;
+               data = mrq->data;

Quoted 12 lines

drivers/mmc/core/core.c static void mmc_wait_for_req_done(struct mmc_host · line 508

Quoted 9 lines

drivers/mmc/core/core.c +0 -0

/*

* If host has timed out waiting for the sanitize

drivers/mmc/core/core.c +2 -0

@@ -501,6 +503,8 @@ static void mmc_wait_for_req_done(struct mmc_host
 *host,
                         mmc_hostname(host), cmd->opcode, cmd->error);
                cmd->retries--;
                cmd->error = 0;
+               if (data)
+                       data->error = 0;

What's this change for?

sunxi-mmc will set both cmd->error and data->error to -ETIMEDOUT in case of any interrupt error bit was set (related code within `sunxi_finalize_request()').

`__mmc_sd_switch()' thought it was a error case if data->error != 0.

So the data->error bit needs to be cleared before next retry.

Regards,

yousnog

Quoted 51 lines

drivers/mmc/core/core.c +0 -0

                __mmc_start_request(host, mrq);
        }

drivers/mmc/core/sd_ops.c +3 -2

index 48d0c93..22bef3c 100644
@@ -304,8 +304,8 @@ int mmc_app_send_scr(struct mmc_card *card, u32 *scr)
        return 0;
  }
 
-int mmc_sd_switch(struct mmc_card *card, int mode, int group,
-       u8 value, u8 *resp)
+int __mmc_sd_switch(struct mmc_card *card, int mode, int group,
+       u8 value, u8 *resp, int retries)
  {
        struct mmc_request mrq = {NULL};
        struct mmc_command cmd = {0};

@@ -328,6 +328,7 @@ int mmc_sd_switch(struct mmc_card *card, int mode, int
 group,
        cmd.arg &= ~(0xF << (group * 4));
        cmd.arg |= value << (group * 4);
        cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
+       cmd.retries = retries;
 
        data.blksz = 64;

        data.blocks = 1;

drivers/mmc/core/sd_ops.h +8 -2

index ffc2305..a53c51e 100644
@@ -17,9 +17,15 @@ int mmc_send_app_op_cond(struct mmc_host *host, u32
 ocr, u32 *rocr);
  int mmc_send_if_cond(struct mmc_host *host, u32 ocr);
  int mmc_send_relative_addr(struct mmc_host *host, unsigned int *rca);
  int mmc_app_send_scr(struct mmc_card *card, u32 *scr);
-int mmc_sd_switch(struct mmc_card *card, int mode, int group,
-       u8 value, u8 *resp);
  int mmc_app_sd_status(struct mmc_card *card, void *ssr);
 
+int __mmc_sd_switch(struct mmc_card *card, int mode, int group,
+       u8 value, u8 *resp, int retries);
+static inline int mmc_sd_switch(struct mmc_card *card, int mode, int
 group,
+       u8 value, u8 *resp)
+{
+       return __mmc_sd_switch(card, mode, group, value, resp, 0);
+}
+

Quoted 9 lines

Quoted 3 lines
  #endif
--
Best Regards
Shawn Lin

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jaehoon Chung

2015-09-16 04:17 UTC #6 lore raw

Re: [PATCH 2/3] mmc: sd: Allow calling sd mode switch with retries

Hi, Yousong.

Quoted 15 lines · Yousong Zhou

Hi,
Quoted 12 lines · Jaehoon Chung
Hi,
Quoted 5 lines · Yousong Zhou
Quoted 4 lines · Shawn Lin
Quoted 3 lines · Yousong Zhou
This will allow retrying access mode switch to High-Speed in the following commit.
Well, it's not solution. (It's my preference.)
Did you consider the card removable case?

Quoted 6 lines

Quoted 2 lines
Could you share which vendor's card needs to retry?
It's a SD card labeled as "pqi" made in Korea. The issue was found on sunxi-mmc controller where it reported response crc error when trying to switch the card into highspeed mode.

Did you test on other target with that card?
On other words, is Sd-card working fine on your real phone or other target? If it's occurred on only sunxi-mmc controller, this is not solution.

And Your comment means "have only to retry when failed to switch high-speed mode.", doesn't? Do you feel it's reasonable? It's same that adds the delay.

Quoted 4 lines

The idea about retrying the access mode switch was from U-Boot code of Allwinner (vendor of sunxi SoCs). The code comments there said this issue can happen with eMMC cards from Toshiba [1].

Why did you compare to U-boot? There are too many differences for user-case.

Quoted 5 lines

The above info was also said in a previous discussion [1]. A sunxi-mmc specific change as suggested by Hans de Geode should definitely be a safer move. But that requires more fundamental framework code change I guess.

[1] maintained at local git? not mainline..

Quoted 3 lines

[1] https://github.com/allwinner-zh/bootloader/blob/master/u-boot-2011.09/drivers/mmc/mmc.c#L2220
[2] http://thread.gmane.org/gmane.comp.hardware.netbook.arm.sunxi/18442/focus=18480

Well, i don't know how ulf think about this.
I want to find the root cause and fix it.
If this is sunxi-mmc controller's problem, i think it can be fixed in sunxi-mmc controller.

Best Regards,
Jaehoon Chung

Quoted 147 lines

Regards,
yousong
Quoted 137 lines
Best Regards,
Jaehoon Chung
Quoted 133 lines
Quoted 9 lines
Quoted 8 lines
Signed-off-by Yousong Zhou <[email protected]>
---
  drivers/mmc/core/core.c   |    4 ++++
  drivers/mmc/core/sd_ops.c |    5 +++--
  drivers/mmc/core/sd_ops.h |   10 ++++++++--
  3 files changed, 15 insertions(+), 4 deletions(-)
Quoted 3 lines
I test this patchset with my already-working sd cards, seems ok. Actually I don't have a card to test like yours that need retry switch HS, but from the changes itself, it's harmless to other cards.
Thanks for the testing.
Those cards normally won't burn out after MMC_CMD_RETRIES access mode switch failures, right?
Quoted 26 lines
so,
Tested-by Shawn Lin <[email protected]>
BTW, I can't find the cover letter? And another should be clarified, plz see comment blow
Quoted 17 lines
drivers/mmc/core/core.c +2 -0
index 9ad73f3..e726bb1 100644
@@ -468,11 +468,13 @@ static void mmc_wait_for_req_done(struct mmc_host
 *host,
                                  struct mmc_request *mrq)
  {
        struct mmc_command *cmd;
+       struct mmc_data *data;
 
        while (1) {
                wait_for_completion(&mrq->completion);
 
                cmd = mrq->cmd;
+               data = mrq->data;
 
Quoted 12 lines
drivers/mmc/core/core.c static void mmc_wait_for_req_done(struct mmc_host · line 508
Quoted 9 lines
drivers/mmc/core/core.c +0 -0
                /*
* If host has timed out waiting for the sanitize
drivers/mmc/core/core.c +2 -0
@@ -501,6 +503,8 @@ static void mmc_wait_for_req_done(struct mmc_host
 *host,
                         mmc_hostname(host), cmd->opcode, cmd->error);
                cmd->retries--;
                cmd->error = 0;
+               if (data)
+                       data->error = 0;
What's this change for?
sunxi-mmc will set both cmd->error and data->error to -ETIMEDOUT in case of any interrupt error bit was set (related code within `sunxi_finalize_request()').
`__mmc_sd_switch()' thought it was a error case if data->error != 0.
So the data->error bit needs to be cleared before next retry.
Regards,
yousnog
Quoted 51 lines
Quoted 51 lines
drivers/mmc/core/core.c +0 -0
                __mmc_start_request(host, mrq);
        }
drivers/mmc/core/sd_ops.c +3 -2
index 48d0c93..22bef3c 100644
@@ -304,8 +304,8 @@ int mmc_app_send_scr(struct mmc_card *card, u32 *scr)
        return 0;
  }
 
-int mmc_sd_switch(struct mmc_card *card, int mode, int group,
-       u8 value, u8 *resp)
+int __mmc_sd_switch(struct mmc_card *card, int mode, int group,
+       u8 value, u8 *resp, int retries)
  {
        struct mmc_request mrq = {NULL};
        struct mmc_command cmd = {0};
@@ -328,6 +328,7 @@ int mmc_sd_switch(struct mmc_card *card, int mode, int
 group,
        cmd.arg &= ~(0xF << (group * 4));
        cmd.arg |= value << (group * 4);
        cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
+       cmd.retries = retries;
 
        data.blksz = 64;
        data.blocks = 1;
drivers/mmc/core/sd_ops.h +8 -2
index ffc2305..a53c51e 100644
@@ -17,9 +17,15 @@ int mmc_send_app_op_cond(struct mmc_host *host, u32
 ocr, u32 *rocr);
  int mmc_send_if_cond(struct mmc_host *host, u32 ocr);
  int mmc_send_relative_addr(struct mmc_host *host, unsigned int *rca);
  int mmc_app_send_scr(struct mmc_card *card, u32 *scr);
-int mmc_sd_switch(struct mmc_card *card, int mode, int group,
-       u8 value, u8 *resp);
  int mmc_app_sd_status(struct mmc_card *card, void *ssr);
 
+int __mmc_sd_switch(struct mmc_card *card, int mode, int group,
+       u8 value, u8 *resp, int retries);
+static inline int mmc_sd_switch(struct mmc_card *card, int mode, int
 group,
+       u8 value, u8 *resp)
+{
+       return __mmc_sd_switch(card, mode, group, value, resp, 0);
+}
+
Quoted 9 lines
Quoted 3 lines
  #endif
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Yousong Zhou

author

2015-09-17 08:29 UTC #7 lore raw

Re: [PATCH 2/3] mmc: sd: Allow calling sd mode switch with retries

Quoted 25 lines · Jaehoon Chung

Hi, Yousong.
Quoted 15 lines · Yousong Zhou
Hi,
Quoted 12 lines · Jaehoon Chung
Hi,
Quoted 5 lines · Yousong Zhou
Quoted 4 lines · Shawn Lin
Quoted 3 lines · Yousong Zhou
This will allow retrying access mode switch to High-Speed in the following commit.
Well, it's not solution. (It's my preference.)
Did you consider the card removable case?
Quoted 6 lines
Quoted 2 lines
Could you share which vendor's card needs to retry?
It's a SD card labeled as "pqi" made in Korea. The issue was found on sunxi-mmc controller where it reported response crc error when trying to switch the card into highspeed mode.

Quoted 7 lines

Did you test on other target with that card?
On other words, is Sd-card working fine on your real phone or other target? If it's occurred on only sunxi-mmc controller, this is not solution.
And Your comment means "have only to retry when failed to switch high-speed mode.", doesn't? Do you feel it's reasonable? It's same that adds the delay.

Not reasonable at all... I expected those cards would just work out of the box and never supposed that I needed to dig into so many places to debug such problems. sorry for the grunt

Quoted 7 lines

Quoted 4 lines
The idea about retrying the access mode switch was from U-Boot code of Allwinner (vendor of sunxi SoCs). The code comments there said this issue can happen with eMMC cards from Toshiba [1].
Why did you compare to U-boot? There are too many differences for user-case.

Because I was then guessing whether the vendor had also encountered the same issue and if so did they actually fixed or worked around it. Then viola, there they wrote in the comments in natural language describing exact the same error I encountered both in U-Boot and Linux.

Quoted 9 lines

Quoted 5 lines
The above info was also said in a previous discussion [1]. A sunxi-mmc specific change as suggested by Hans de Geode should definitely be a safer move. But that requires more fundamental framework code change I guess.
[1] maintained at local git? not mainline..
Quoted 1 line

Quoted 7 lines

Quoted 2 lines
[1] https://github.com/allwinner-zh/bootloader/blob/master/u-boot-2011.09/drivers/mmc/mmc.c#L2220
[2] http://thread.gmane.org/gmane.comp.hardware.netbook.arm.sunxi/18442/focus=18480
Well, i don't know how ulf think about this.
I want to find the root cause and fix it.
If this is sunxi-mmc controller's problem, i think it can be fixed in sunxi-mmc controller.

I agree. Root cause should be found and fix may eventually come. Other false rumours were already there in the Internet that sunxi mmc controller did not support cards smaller than 512MB... Well, I may try (not in the near future) how vendor's linux-sunxi-3.4 will work with this card.

Cheers,

yousong

Quoted 151 lines

Best Regards,
Jaehoon Chung
Quoted 147 lines
Regards,
yousong
Quoted 137 lines
Best Regards,
Jaehoon Chung
Quoted 133 lines
Quoted 9 lines
Quoted 8 lines
Signed-off-by Yousong Zhou <[email protected]>
---
  drivers/mmc/core/core.c   |    4 ++++
  drivers/mmc/core/sd_ops.c |    5 +++--
  drivers/mmc/core/sd_ops.h |   10 ++++++++--
  3 files changed, 15 insertions(+), 4 deletions(-)
Quoted 3 lines
I test this patchset with my already-working sd cards, seems ok. Actually I don't have a card to test like yours that need retry switch HS, but from the changes itself, it's harmless to other cards.
Thanks for the testing.
Those cards normally won't burn out after MMC_CMD_RETRIES access mode switch failures, right?
Quoted 26 lines
so,
Tested-by Shawn Lin <[email protected]>
BTW, I can't find the cover letter? And another should be clarified, plz see comment blow
Quoted 17 lines
drivers/mmc/core/core.c +2 -0
index 9ad73f3..e726bb1 100644
@@ -468,11 +468,13 @@ static void mmc_wait_for_req_done(struct mmc_host
 *host,
                                  struct mmc_request *mrq)
  {
        struct mmc_command *cmd;
+       struct mmc_data *data;
 
        while (1) {
                wait_for_completion(&mrq->completion);
 
                cmd = mrq->cmd;
+               data = mrq->data;
 
Quoted 12 lines
drivers/mmc/core/core.c static void mmc_wait_for_req_done(struct mmc_host · line 508
Quoted 9 lines
drivers/mmc/core/core.c +0 -0
                /*
* If host has timed out waiting for the sanitize
drivers/mmc/core/core.c +2 -0
@@ -501,6 +503,8 @@ static void mmc_wait_for_req_done(struct mmc_host
 *host,
                         mmc_hostname(host), cmd->opcode, cmd->error);
                cmd->retries--;
                cmd->error = 0;
+               if (data)
+                       data->error = 0;
What's this change for?
sunxi-mmc will set both cmd->error and data->error to -ETIMEDOUT in case of any interrupt error bit was set (related code within `sunxi_finalize_request()').
`__mmc_sd_switch()' thought it was a error case if data->error != 0.
So the data->error bit needs to be cleared before next retry.
Regards,
yousnog
Quoted 51 lines
Quoted 51 lines
drivers/mmc/core/core.c +0 -0
                __mmc_start_request(host, mrq);
        }
drivers/mmc/core/sd_ops.c +3 -2
index 48d0c93..22bef3c 100644
@@ -304,8 +304,8 @@ int mmc_app_send_scr(struct mmc_card *card, u32 *scr)
        return 0;
  }
 
-int mmc_sd_switch(struct mmc_card *card, int mode, int group,
-       u8 value, u8 *resp)
+int __mmc_sd_switch(struct mmc_card *card, int mode, int group,
+       u8 value, u8 *resp, int retries)
  {
        struct mmc_request mrq = {NULL};
        struct mmc_command cmd = {0};
@@ -328,6 +328,7 @@ int mmc_sd_switch(struct mmc_card *card, int mode, int
 group,
        cmd.arg &= ~(0xF << (group * 4));
        cmd.arg |= value << (group * 4);
        cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
+       cmd.retries = retries;
 
        data.blksz = 64;
        data.blocks = 1;
drivers/mmc/core/sd_ops.h +8 -2
index ffc2305..a53c51e 100644
@@ -17,9 +17,15 @@ int mmc_send_app_op_cond(struct mmc_host *host, u32
 ocr, u32 *rocr);
  int mmc_send_if_cond(struct mmc_host *host, u32 ocr);
  int mmc_send_relative_addr(struct mmc_host *host, unsigned int *rca);
  int mmc_app_send_scr(struct mmc_card *card, u32 *scr);
-int mmc_sd_switch(struct mmc_card *card, int mode, int group,
-       u8 value, u8 *resp);
  int mmc_app_sd_status(struct mmc_card *card, void *ssr);
 
+int __mmc_sd_switch(struct mmc_card *card, int mode, int group,
+       u8 value, u8 *resp, int retries);
+static inline int mmc_sd_switch(struct mmc_card *card, int mode, int
 group,
+       u8 value, u8 *resp)
+{
+       return __mmc_sd_switch(card, mode, group, value, resp, 0);
+}
+
Quoted 9 lines
Quoted 3 lines
  #endif
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Yousong Zhou

author

2015-09-09 16:37 UTC #8 lore raw

PATCH 3/3 mmc: sd: Retry switching to highspeed mode in case of error

In reply to Yousong Zhou

A SD card with sunxi-mmc can fail with the following error message (RCE for response CRC error) when trying to switch to highspeed mode. But the mode switch can almost always succeed on the second try.

Dmesg before this commit.

    [    1.112060] mmc0: host does not support reading read-only switch, assuming write-enable
    [    1.126527] sunxi-mmc 1c0f000.mmc: smc 0 err, cmd 6, RD RCE !!
    [    1.132388] sunxi-mmc 1c0f000.mmc: data error, sending stop command
    [    1.139451] sunxi-mmc 1c0f000.mmc: send stop command failed
    [    1.145056] mmc0: error -110 whilst initialising SD card
    [    1.150424] ehci-platform 1c1c000.usb: USB 2.0 started, EHCI 1.00
    [    1.156533] sunxi-mmc 1c0f000.mmc: smc 0 err, cmd 1, RTO !!

Dmesg after this commit.

    [    1.062306] mmc0: host does not support reading read-only switch, assuming write-enable
    [    1.070635] sunxi-mmc 1c0f000.mmc: smc 0 err, cmd 6, RD RCE !!
    [    1.295939] sunxi-mmc 1c0f000.mmc: data error, sending stop command
    [    1.322635] Waiting for root device /dev/mmcblk0p2...
    [    1.322691] sunxi-mmc 1c0f000.mmc: send stop command failed
    [    1.335686] mmc0: new high speed SD card at address 0002
    [    1.341557] mmcblk0: mmc0:0002 00000 974 MiB
    [    1.347021]  mmcblk0: p1 p2

Signed-off-by Yousong Zhou <[email protected]>

---
 drivers/mmc/core/sd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/core/sd.c +1 -1

index e28ebf3..7465b16 100644
@@ -364,7 +364,7 @@ int mmc_sd_switch_hs(struct mmc_card *card)
 		return -ENOMEM;
 	}
 
-	err = mmc_sd_switch(card, 1, 0, 1, status);
+	err = __mmc_sd_switch(card, 1, 0, 1, status, MMC_CMD_RETRIES);
 	if (err)
 		goto out;

Signature · 1 line

1.7.10.4

Hans de Goede

2015-09-09 19:29 UTC #9 lore raw

In reply to Yousong Zhou

Hi All,

I requested yousong to send this set to further discuss it, and hopefully get some fix for this in the kernel.

Yousong, I actually intended you to do a patch to the sunxi-mmc code, rather then too the core. Ah well. Since we've this set now lets see if people like it, it may help other cases too I guess.

Regards,

Hans

Quoted 19 lines · Yousong Zhou

Signed-off-by Yousong Zhou <[email protected]>

---
  drivers/mmc/core/sd.c |    2 --
  1 file changed, 2 deletions(-)

drivers/mmc/core/sd.c +0 -2

index 4e7366a..e28ebf3 100644
@@ -357,8 +357,6 @@ int mmc_sd_switch_hs(struct mmc_card *card)
  	if (card->sw_caps.hs_max_dtr == 0)
  		return 0;
 
-	err = -EIO;
-
  	status = kmalloc(64, GFP_KERNEL);
  	if (!status) {
  		pr_err("%s: could not allocate a buffer for "

Ulf Hansson

Applied

2015-09-15 11:34 UTC #10 lore raw

In reply to Yousong Zhou

Quoted 1 line · Yousong Zhou

Signed-off-by Yousong Zhou <[email protected]>

Thanks, applied for next!

I will comment on patch2 and patch3 separately.

Kind regards
Uffe

Quoted 20 lines

---
 drivers/mmc/core/sd.c |    2 --
 1 file changed, 2 deletions(-)

drivers/mmc/core/sd.c +0 -3

index 4e7366a..e28ebf3 100644
@@ -357,8 +357,6 @@ int mmc_sd_switch_hs(struct mmc_card *card)
        if (card->sw_caps.hs_max_dtr == 0)
                return 0;
 
-       err = -EIO;
-
        status = kmalloc(64, GFP_KERNEL);
        if (!status) {
                pr_err("%s: could not allocate a buffer for "
--

1.7.10.4