[PATCH 1/1] CMD12 error recovery support for SD cards

[PATCH 1/1] CMD12 error recovery support for SD cards

[Mahadev Cholachagudda]

Dear All,

I've a kernel code base of 2.6.28.9, where in I find that CMD12 error
recovery is not been implemented. It is a wise idea to implement CMD12
error recovery, that involves sending CMD13 to get the status of the
card. If the card is in "trans" state, it means CMD12 error shall be
treated as successfull. If card state is not in "trans", then we have
to send CMD12 again to stop the data transfer and then checking of the
card status through CMD13. This sequence is detailed in the
"Simplified Host Controller specification" on SDCard website. I
believe this is the case for any controller, though I may be wrong.

With the modification done to the code which is attached as patch to
the latest mmc git, it is working at our site. And we do see some
errors with the SDIO controller (proprietary but meets Standard SD
host controller specification) that we are using.

Request you to comment on the patch.

diff -urNd linux-orig/drivers/mmc/card/block.c
linux-current/drivers/mmc/card/block.c
--- linux-orig/drivers/mmc/card/block.c    2009-09-16 15:43:03.667274139 +0530
+++ linux-current/drivers/mmc/card/block.c    2009-09-16
15:36:24.722272962 +0530
@@ -335,6 +335,76 @@
          * until later as we need to wait for the card to leave
          * programming mode even when things go wrong.
          */
+
+    /* cmd12 error recovery fix */
+    if( (brq.stop.error) && (brq.data.blocks > 1) &&
!mmc_host_is_spi(card->host) &&
+        mmc_card_sd(card) ) {
+
+      struct mmc_request my_mrq;
+      struct mmc_command my_cmd;
+
+      /*
+       * if there is an error for CMD12, get the state of the card by
+       * sending cmd13 and then check the state of the card.
+       * If the card state is "tran", consider the cmd12 is successfull
+       * If not, then send again CMD12 to stop the data transfer. This time,
+       * any failure to get response, will result in to the card being
+       * non-recoverable.
+       */
+
+      memset(&my_mrq, 0, sizeof(struct mmc_request));
+      memset(&my_cmd, 0, sizeof(struct mmc_command));
+      my_cmd.opcode = MMC_SEND_STATUS;
+      my_cmd.arg = card->rca << 16;
+      my_cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
+      my_mrq.cmd = &my_cmd;
+      mmc_wait_for_req(card->host, &my_mrq);
+
+      if(!my_mrq.cmd->error) {
+        if(R1_CURRENT_STATE(my_cmd.resp[0]) == 4) {
+          /* card is in "trans" state, so treat the CMD12 as succesfull */
+          brq.stop.error = 0;
+        }
+        else {
+          /*
+           * card is not still in trans state. Now send CMD12 again and repeat
+           * the process of checking the card status using CMD13
+           */
+          memset(&my_cmd, 0, sizeof(struct mmc_command));
+          my_cmd.opcode = MMC_STOP_TRANSMISSION;
+          my_cmd.arg = 0;
+          my_cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
+          my_mrq.cmd = &my_cmd;
+          mmc_wait_for_req(card->host, &my_mrq);
+
+          /* get the card status */
+          memset(&my_cmd, 0, sizeof(struct mmc_command));
+          my_cmd.opcode = MMC_SEND_STATUS;
+          my_cmd.arg = card->rca << 16;
+          my_cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
+          my_mrq.cmd = &my_cmd;
+          mmc_wait_for_req(card->host, &my_mrq);
+
+          if(!my_mrq.cmd->error) {
+            if(R1_CURRENT_STATE(my_cmd.resp[0]) == 4) {
+              /* card is in "trans" state, so treat the CMD12 as succesfull */
+              brq.stop.error = 0;
+            }
+            else {
+              /*
+               * card is not in "trans" state yet. Just post an error and
+               * error is not recoverable
+               */
+            }
+          }
+          else {
+            /* error for cmd13!!, then something is wrong with the card */
+          }
+        }
+      }
+    }
+    /* cmd12 error recovery fix */
+
         if (brq.cmd.error || brq.data.error || brq.stop.error) {
             if (brq.data.blocks > 1 && rq_data_dir(req) == READ) {
                 /* Redo read one sector at a time */

Regards,
Mahadev Cholachagudda

Re: [PATCH 1/1] CMD12 error recovery support for SD cards

[Mahadev Cholachagudda]

Dear Sir,

Any comments on this.

Regards,
Mahadev

2009/9/16 Mahadev Cholachagudda <mgudda@gmail.com>:
> Dear All,
>
> I've a kernel code base of 2.6.28.9, where in I find that CMD12 error
> recovery is not been implemented. It is a wise idea to implement CMD12
> error recovery, that involves sending CMD13 to get the status of the
> card. If the card is in "trans" state, it means CMD12 error shall be
> treated as successfull. If card state is not in "trans", then we have
> to send CMD12 again to stop the data transfer and then checking of the
> card status through CMD13. This sequence is detailed in the
> "Simplified Host Controller specification" on SDCard website. I
> believe this is the case for any controller, though I may be wrong.
>
> With the modification done to the code which is attached as patch to
> the latest mmc git, it is working at our site. And we do see some
> errors with the SDIO controller (proprietary but meets Standard SD
> host controller specification) that we are using.
>
> Request you to comment on the patch.
>
> diff -urNd linux-orig/drivers/mmc/card/block.c
> linux-current/drivers/mmc/card/block.c
> --- linux-orig/drivers/mmc/card/block.c    2009-09-16 15:43:03.667274139 +0530
> +++ linux-current/drivers/mmc/card/block.c    2009-09-16
> 15:36:24.722272962 +0530
> @@ -335,6 +335,76 @@
>           * until later as we need to wait for the card to leave
>           * programming mode even when things go wrong.
>           */
> +
> +    /* cmd12 error recovery fix */
> +    if( (brq.stop.error) && (brq.data.blocks > 1) &&
> !mmc_host_is_spi(card->host) &&
> +        mmc_card_sd(card) ) {
> +
> +      struct mmc_request my_mrq;
> +      struct mmc_command my_cmd;
> +
> +      /*
> +       * if there is an error for CMD12, get the state of the card by
> +       * sending cmd13 and then check the state of the card.
> +       * If the card state is "tran", consider the cmd12 is successfull
> +       * If not, then send again CMD12 to stop the data transfer. This time,
> +       * any failure to get response, will result in to the card being
> +       * non-recoverable.
> +       */
> +
> +      memset(&my_mrq, 0, sizeof(struct mmc_request));
> +      memset(&my_cmd, 0, sizeof(struct mmc_command));
> +      my_cmd.opcode = MMC_SEND_STATUS;
> +      my_cmd.arg = card->rca << 16;
> +      my_cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
> +      my_mrq.cmd = &my_cmd;
> +      mmc_wait_for_req(card->host, &my_mrq);
> +
> +      if(!my_mrq.cmd->error) {
> +        if(R1_CURRENT_STATE(my_cmd.resp[0]) == 4) {
> +          /* card is in "trans" state, so treat the CMD12 as succesfull */
> +          brq.stop.error = 0;
> +        }
> +        else {
> +          /*
> +           * card is not still in trans state. Now send CMD12 again and repeat
> +           * the process of checking the card status using CMD13
> +           */
> +          memset(&my_cmd, 0, sizeof(struct mmc_command));
> +          my_cmd.opcode = MMC_STOP_TRANSMISSION;
> +          my_cmd.arg = 0;
> +          my_cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
> +          my_mrq.cmd = &my_cmd;
> +          mmc_wait_for_req(card->host, &my_mrq);
> +
> +          /* get the card status */
> +          memset(&my_cmd, 0, sizeof(struct mmc_command));
> +          my_cmd.opcode = MMC_SEND_STATUS;
> +          my_cmd.arg = card->rca << 16;
> +          my_cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
> +          my_mrq.cmd = &my_cmd;
> +          mmc_wait_for_req(card->host, &my_mrq);
> +
> +          if(!my_mrq.cmd->error) {
> +            if(R1_CURRENT_STATE(my_cmd.resp[0]) == 4) {
> +              /* card is in "trans" state, so treat the CMD12 as succesfull */
> +              brq.stop.error = 0;
> +            }
> +            else {
> +              /*
> +               * card is not in "trans" state yet. Just post an error and
> +               * error is not recoverable
> +               */
> +            }
> +          }
> +          else {
> +            /* error for cmd13!!, then something is wrong with the card */
> +          }
> +        }
> +      }
> +    }
> +    /* cmd12 error recovery fix */
> +
>          if (brq.cmd.error || brq.data.error || brq.stop.error) {
>              if (brq.data.blocks > 1 && rq_data_dir(req) == READ) {
>                  /* Redo read one sector at a time */
>
> Regards,
> Mahadev Cholachagudda
>



-- 
Cheers,
Mahadev

Re: [PATCH 1/1] CMD12 error recovery support for SD cards

[Matt Fleming]

On Wed, Sep 16, 2009 at 04:09:11PM +0530, Mahadev Cholachagudda wrote:
> Dear All,
> 
> I've a kernel code base of 2.6.28.9, where in I find that CMD12 error
> recovery is not been implemented. It is a wise idea to implement CMD12
> error recovery, that involves sending CMD13 to get the status of the
> card. If the card is in "trans" state, it means CMD12 error shall be
> treated as successfull. If card state is not in "trans", then we have
> to send CMD12 again to stop the data transfer and then checking of the
> card status through CMD13. This sequence is detailed in the
> "Simplified Host Controller specification" on SDCard website. I
> believe this is the case for any controller, though I may be wrong.
> 
> With the modification done to the code which is attached as patch to
> the latest mmc git, it is working at our site. And we do see some
> errors with the SDIO controller (proprietary but meets Standard SD
> host controller specification) that we are using.
> 
> Request you to comment on the patch.
> 

In principal this patch seems OK. However, I recommend reading
Documentation/CodingStyle and sending this patch again once you've fixed
up the coding style and whitespace errors.

Is there a specific reason that you have used mmc_wait_for_req() instead
of mmc_wait_for_cmd() and reusing the "cmd" local variable? The empty
"else" clauses can also probably be deleted.

Re: [PATCH 1/1] CMD12 error recovery support for SD cards

[Mahadev Cholachagudda]

Dear Matt,
Thank you very much. Attached is the re-written patch with comments
included. This has been tested in our platform, where it recovers from
CMD12 cmd_conflict errors.

Regards,
Mahadev


diff -urNd linux-orig/drivers/mmc/card/block.c
linux-current/drivers/mmc/card/block.c
--- linux-orig/drivers/mmc/card/block.c	2009-09-16 15:43:03.667274139 +0530
+++ linux-current/drivers/mmc/card/block.c	2009-09-30 16:41:50.605273759 +0530
@@ -330,6 +330,67 @@

 		mmc_queue_bounce_post(mq);

+		/* cmd12 error recovery fix */
+		if( (brq.stop.error) && (brq.data.blocks > 1) &&
+				!mmc_host_is_spi(card->host) && mmc_card_sd(card) ) {
+
+			/*
+			 * if there is an error for CMD12, get the state of the card by
+			 * sending cmd13 and then check the state of the card.
+			 * If the card state is "tran", consider the cmd12 is successfull
+			 * If not, then send again CMD12 to stop the data transfer. This
+			 * time, any failure to get response, will result in to the card
+			 * being non-recoverable.
+			 */
+			memset(&cmd, 0, sizeof(struct mmc_command));
+			cmd.opcode = MMC_SEND_STATUS;
+			cmd.arg = card->rca << 16;
+			cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
+			mmc_wait_for_cmd(card->host, &cmd, 0);
+
+			if(!cmd.error && R1_CURRENT_STATE(cmd.resp[0]) == 4) {
+				/* card is in "trans" state, so treat the CMD12 as succesfull */
+				brq.stop.error = 0;
+			} else {
+				/*
+				 * card is not still in trans state. Now send CMD12 again and
+				 * repeat the process of checking the card status using CMD13
+				 */
+				memset(&cmd, 0, sizeof(struct mmc_command));
+				cmd.opcode = MMC_STOP_TRANSMISSION;
+				cmd.arg = 0;
+				cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
+				mmc_wait_for_cmd(card->host, &cmd, 0);
+
+				/* get the card status */
+				memset(&cmd, 0, sizeof(struct mmc_command));
+				cmd.opcode = MMC_SEND_STATUS;
+				cmd.arg = card->rca << 16;
+				cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
+				mmc_wait_for_cmd(card->host, &cmd, 0);
+
+				if(!cmd.error) {
+					if(R1_CURRENT_STATE(cmd.resp[0]) == 4) {
+						/* card is in "trans" state, so treat the CMD12 as succesfull */
+						brq.stop.error = 0;
+					} else {
+						printk(KERN_ERR "%s: CMD12 error not recoverable\n",
+								req->rq_disk->disk_name );
+						/*
+						 * card is not in "trans" state yet. Just post an error and
+						 * error is not recoverable
+						 */
+					}
+				} else {
+					printk(KERN_ERR "%s: CMD13 returns an error!!\n",
+							req->rq_disk->disk_name );
+					/* error for cmd13!!, then something is wrong with the card */
+				}
+			}
+		}
+		/* cmd12 error recovery fix */
+
+
 		/*
 		 * Check for errors here, but don't jump to cmd_err
 		 * until later as we need to wait for the card to leave



2009/9/23 Matt Fleming <matt@console-pimps.org>:
> On Wed, Sep 16, 2009 at 04:09:11PM +0530, Mahadev Cholachagudda wrote:
>> Dear All,
>>
>> I've a kernel code base of 2.6.28.9, where in I find that CMD12 error
>> recovery is not been implemented. It is a wise idea to implement CMD12
>> error recovery, that involves sending CMD13 to get the status of the
>> card. If the card is in "trans" state, it means CMD12 error shall be
>> treated as successfull. If card state is not in "trans", then we have
>> to send CMD12 again to stop the data transfer and then checking of the
>> card status through CMD13. This sequence is detailed in the
>> "Simplified Host Controller specification" on SDCard website. I
>> believe this is the case for any controller, though I may be wrong.
>>
>> With the modification done to the code which is attached as patch to
>> the latest mmc git, it is working at our site. And we do see some
>> errors with the SDIO controller (proprietary but meets Standard SD
>> host controller specification) that we are using.
>>
>> Request you to comment on the patch.
>>
>
> In principal this patch seems OK. However, I recommend reading
> Documentation/CodingStyle and sending this patch again once you've fixed
> up the coding style and whitespace errors.
>
> Is there a specific reason that you have used mmc_wait_for_req() instead
> of mmc_wait_for_cmd() and reusing the "cmd" local variable? The empty
> "else" clauses can also probably be deleted.
>



-- 
Cheers,
Mahadev

Re: [PATCH 1/1] CMD12 error recovery support for SD cards

[Matt Fleming]

[Pierre, I dropped your old email address from CC, hope that's OK]

On Wed, Sep 30, 2009 at 04:48:26PM +0530, Mahadev Cholachagudda wrote:
> Dear Matt,
> Thank you very much. Attached is the re-written patch with comments
> included. This has been tested in our platform, where it recovers from
> CMD12 cmd_conflict errors.
> 

I may be looking at an out of date SD Host Controller Spec, but can you
point me at the place where it says that it's ok to retry the CMD12 if
it failed? I can find the part explaining that you can issue CMD13 to
see if CMD12 really failed or not, but not the place where it says you
can reissue CMD12.

In your testcase, do you always end up issuing the second CMD12?

> 
> diff -urNd linux-orig/drivers/mmc/card/block.c
> linux-current/drivers/mmc/card/block.c
> --- linux-orig/drivers/mmc/card/block.c	2009-09-16 15:43:03.667274139 +0530
> +++ linux-current/drivers/mmc/card/block.c	2009-09-30 16:41:50.605273759 +0530
> @@ -330,6 +330,67 @@
> 
>  		mmc_queue_bounce_post(mq);
> 
> +		/* cmd12 error recovery fix */
> +		if( (brq.stop.error) && (brq.data.blocks > 1) &&
> +				!mmc_host_is_spi(card->host) && mmc_card_sd(card) ) {

Are you sure that this recovery procedure is not valid over SPI when
doing a multiblock read?

> +
> +			/*
> +			 * if there is an error for CMD12, get the state of the card by
> +			 * sending cmd13 and then check the state of the card.
> +			 * If the card state is "tran", consider the cmd12 is successfull

"successfull" should be "successful"

> +			 * If not, then send again CMD12 to stop the data transfer. This
> +			 * time, any failure to get response, will result in to the card
> +			 * being non-recoverable.
> +			 */
> +			memset(&cmd, 0, sizeof(struct mmc_command));
> +			cmd.opcode = MMC_SEND_STATUS;
> +			cmd.arg = card->rca << 16;
> +			cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
> +			mmc_wait_for_cmd(card->host, &cmd, 0);
> +
> +			if(!cmd.error && R1_CURRENT_STATE(cmd.resp[0]) == 4) {
> +				/* card is in "trans" state, so treat the CMD12 as succesfull */
> +				brq.stop.error = 0;
> +			} else {
> +				/*
> +				 * card is not still in trans state. Now send CMD12 again and
> +				 * repeat the process of checking the card status using CMD13
> +				 */
> +				memset(&cmd, 0, sizeof(struct mmc_command));
> +				cmd.opcode = MMC_STOP_TRANSMISSION;
> +				cmd.arg = 0;
> +				cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
> +				mmc_wait_for_cmd(card->host, &cmd, 0);
> +
> +				/* get the card status */
> +				memset(&cmd, 0, sizeof(struct mmc_command));
> +				cmd.opcode = MMC_SEND_STATUS;
> +				cmd.arg = card->rca << 16;
> +				cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
> +				mmc_wait_for_cmd(card->host, &cmd, 0);
> +
> +				if(!cmd.error) {
> +					if(R1_CURRENT_STATE(cmd.resp[0]) == 4) {
> +						/* card is in "trans" state, so treat the CMD12 as succesfull */
> +						brq.stop.error = 0;
> +					} else {
> +						printk(KERN_ERR "%s: CMD12 error not recoverable\n",
> +								req->rq_disk->disk_name );
> +						/*
> +						 * card is not in "trans" state yet. Just post an error and
> +						 * error is not recoverable
> +						 */
> +					}
> +				} else {
> +					printk(KERN_ERR "%s: CMD13 returns an error!!\n",
> +							req->rq_disk->disk_name );
> +					/* error for cmd13!!, then something is wrong with the card */

I'm not entirely sure any of these errors are needed. The code below
this in drivers/mmc/card/block.c will print an error message if
brq.stop.error is non-zero (and we will have left it non-zero if we
haven't recovered).

Re: [PATCH 1/1] CMD12 error recovery support for SD cards

[Mahadev Cholachagudda]

2009/10/2 Matt Fleming <matt@console-pimps.org>:
>
> I may be looking at an out of date SD Host Controller Spec, but can you
> point me at the place where it says that it's ok to retry the CMD12 if
> it failed? I can find the part explaining that you can issue CMD13 to
> see if CMD12 really failed or not, but not the place where it says you
> can reissue CMD12.
>
> In your testcase, do you always end up issuing the second CMD12?
>

In SD HostController Simplified Spec, page 106, tells about CMD12
failure and CMD13 to check the status.
Page 110, section 3.10.2, tells about AutoCMD12 error recovery, but we
can safely include this for checking with normal CMD12 error recovery.

Yep, In our target board, some-times, we end up issuing the second CMD12.

>
> Are you sure that this recovery procedure is not valid over SPI when
> doing a multiblock read?
>

I'm not able to get the details of recovery procedure for SPI.
Moreover the host controller spec 2.0 does not support SPI mode.

> "successfull" should be "successful"

Shall be updated.

>
> I'm not entirely sure any of these errors are needed. The code below
> this in drivers/mmc/card/block.c will print an error message if
> brq.stop.error is non-zero (and we will have left it non-zero if we
> haven't recovered).
>

OK. I shall update this too.

Regards,
Mahadev