SD response type 6 vs. 1

SD response type 6 vs. 1

[andrzej zaborowski]

Hi,
  Linux has the same values for MMC_RSP_R1 and MMC_RSP_R6 thus making
the two response formats indistinguishable for mmc/sd hosts. Hardware
like my OMAP310 needs to know exactly whether to expect an R1 or R6
format, otherwise CMD3 fails.

Sure it would be nice if it didn't care, but nevertheless, it's not a
hardware bug - the two formats are specified separately in SD specs
because they *are* different. So the hardware has full right to know
the right response type. Rather it's a hack to rely on R1 and R6
having the same size, like Linux does now. It would also be a hack to
allow the CMD3 to fail with CRC error and ignore the error, like
omap.c did before, because it makes an assumption that's not in the
OMAP specs.

The following is an ugly fix. It's ugly because host code should be
agnostic to opcodes, as mentioned by Pierre Ossman.
I believe this was already submitted by Carlos Aguiar and rejected by
Pierre, which resulted in leaving the driver broken on some boards.
--- a/drivers/mmc/host/omap.c
+++ b/drivers/mmc/host/omap.c
@@ -22,6 +22,7 @@
 #include <linux/spinlock.h>
 #include <linux/timer.h>
 #include <linux/mmc/mmc.h>
+#include <linux/mmc/sd.h>
 #include <linux/mmc/host.h>
 #include <linux/mmc/card.h>
 #include <linux/clk.h>
@@ -211,6 +221,9 @@ mmc_omap_start_command(struct mmc_omap_host *host,
struct mmc_command *cmd)
        case MMC_RSP_R1B:
                /* resp 1, 1b, 6, 7 */
                resptype = 1;
+               if (mmc_card_sd(host->mmc->card) &&
+                               cmd->opcode == SD_SEND_RELATIVE_ADDR)
+                       resptype = 6;
                break;
        case MMC_RSP_R2:
                resptype = 2;
----8<---------------------------------

A more correct and more intrusive fix would look like this:
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -25,11 +25,12 @@ struct mmc_command {
 #define MMC_RSP_CRC    (1 << 2)                /* expect valid crc */
 #define MMC_RSP_BUSY   (1 << 3)                /* card may send busy */
 #define MMC_RSP_OPCODE (1 << 4)                /* response contains opcode */
-#define MMC_CMD_MASK   (3 << 5)                /* command type */
-#define MMC_CMD_AC     (0 << 5)
-#define MMC_CMD_ADTC   (1 << 5)
-#define MMC_CMD_BC     (2 << 5)
-#define MMC_CMD_BCR    (3 << 5)
+#define MMC_RSP_RCA    (1 << 5)                /* response contains an rca */
+#define MMC_CMD_MASK   (3 << 6)                /* command type */
+#define MMC_CMD_AC     (0 << 6)
+#define MMC_CMD_ADTC   (1 << 6)
+#define MMC_CMD_BC     (2 << 6)
+#define MMC_CMD_BCR    (3 << 6)

 /*
  * These are the response types, and correspond to valid bit
@@ -41,10 +42,10 @@ struct mmc_command {
 #define MMC_RSP_R1B
(MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE|MMC_RSP_BUSY)
 #define MMC_RSP_R2     (MMC_RSP_PRESENT|MMC_RSP_136|MMC_RSP_CRC)
 #define MMC_RSP_R3     (MMC_RSP_PRESENT)
-#define MMC_RSP_R6     (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
+#define MMC_RSP_R6     (MMC_RSP_PRESENT|MMC_RSP_RCA)
 #define MMC_RSP_R7     (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)

-#define mmc_resp_type(cmd)     ((cmd)->flags &
(MMC_RSP_PRESENT|MMC_RSP_136|MMC_RSP_CRC|MMC_RSP_BUSY|MMC_RSP_OPCODE))
+#define mmc_resp_type(cmd)     ((cmd)->flags &
(MMC_RSP_PRESENT|MMC_RSP_136|MMC_RSP_CRC|MMC_RSP_BUSY|MMC_RSP_OPCODE|MMC_RSP_RCA))

 /*
  * These are the command types.
diff --git a/drivers/mmc/host/au1xmmc.c b/drivers/mmc/host/au1xmmc.c
--- a/drivers/mmc/host/au1xmmc.c
+++ b/drivers/mmc/host/au1xmmc.c
@@ -206,6 +206,9 @@ static int au1xmmc_send_command(struct
au1xmmc_host *host, int wait,
        case MMC_RSP_R3:
                mmccmd |= SD_CMD_RT_3;
                break;
+       case MMC_RSP_R6:
+               mmccmd |= SD_CMD_RT_6;
+               break;
        default:
                printk(KERN_INFO "au1xmmc: unhandled response type %02x\n",
                        mmc_resp_type(cmd));
diff --git a/drivers/mmc/host/imxmmc.c b/drivers/mmc/host/imxmmc.c
--- a/drivers/mmc/host/imxmmc.c
+++ b/drivers/mmc/host/imxmmc.c
@@ -350,6 +350,9 @@ static void imxmci_start_cmd(struct imxmci_host
*host, struct mmc_command *cmd,
        case MMC_RSP_R3: /* short */
                cmdat |= CMD_DAT_CONT_RESPONSE_FORMAT_R3;
                break;
+       case MMC_RSP_R6: /* short RCA */
+               cmdat |= CMD_DAT_CONT_RESPONSE_FORMAT_R6;
+               break;
        default:
                break;
        }
diff --git a/drivers/mmc/host/omap.c b/drivers/mmc/host/omap.c
--- a/drivers/mmc/host/omap.c
+++ b/drivers/mmc/host/omap.c
@@ -209,7 +219,7 @@ mmc_omap_start_command(struct mmc_omap_host *host,
struct mmc_command *cmd)
                break;
        case MMC_RSP_R1:
        case MMC_RSP_R1B:
-               /* resp 1, 1b, 6, 7 */
+               /* resp 1, 1b, 7 */
                resptype = 1;
                break;
        case MMC_RSP_R2:
@@ -218,6 +228,9 @@ mmc_omap_start_command(struct mmc_omap_host *host,
struct mmc_command *cmd)
        case MMC_RSP_R3:
                resptype = 3;
                break;
+       case MMC_RSP_R6:
+               resptype = 6;
+               break;
        default:
                dev_err(mmc_dev(host->mmc), "Invalid response type:
%04x\n", mmc_resp_type(cmd));
                break;
diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -170,7 +170,8 @@ static void pxamci_start_cmd(struct pxamci_host
*host, struct mmc_command *cmd,

 #define RSP_TYPE(x)    ((x) & ~(MMC_RSP_BUSY|MMC_RSP_OPCODE))
        switch (RSP_TYPE(mmc_resp_type(cmd))) {
-       case RSP_TYPE(MMC_RSP_R1): /* r1, r1b, r6, r7 */
+       case RSP_TYPE(MMC_RSP_R1): /* r1, r1b, r7 */
+       case RSP_TYPE(MMC_RSP_R6):
                cmdat |= CMDAT_RESP_SHORT;
                break;
        case RSP_TYPE(MMC_RSP_R3):
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -549,7 +549,7 @@ static void sdhci_send_command(struct sdhci_host
*host, struct mmc_command *cmd)

        if (cmd->flags & MMC_RSP_CRC)
                flags |= SDHCI_CMD_CRC;
-       if (cmd->flags & MMC_RSP_OPCODE)
+       if (cmd->flags & (MMC_RSP_OPCODE | MMC_RSP_RCA))
                flags |= SDHCI_CMD_INDEX;
        if (cmd->data)
                flags |= SDHCI_CMD_DATA;
diff --git a/drivers/mmc/host/tifm_sd.c b/drivers/mmc/host/tifm_sd.c
--- a/drivers/mmc/host/tifm_sd.c
+++ b/drivers/mmc/host/tifm_sd.c
@@ -345,6 +345,9 @@ static unsigned int tifm_sd_op_flags(struct
mmc_command *cmd)
        case MMC_RSP_R3:
                rc |= TIFM_MMCSD_RSP_R3;
                break;
+       case MMC_RSP_R6:
+               rc |= TIFM_MMCSD_RSP_R6;
+               break;
        default:
                BUG();
        }

What's the preferred way to resolve this?

Regards,
Andrzej

Re: SD response type 6 vs. 1

[Pierre Ossman]

andrzej zaborowski wrote:
> Hi,
>  Linux has the same values for MMC_RSP_R1 and MMC_RSP_R6 thus making
> the two response formats indistinguishable for mmc/sd hosts. Hardware
> like my OMAP310 needs to know exactly whether to expect an R1 or R6
> format, otherwise CMD3 fails.
>
> Sure it would be nice if it didn't care, but nevertheless, it's not a
> hardware bug - the two formats are specified separately in SD specs
> because they *are* different. So the hardware has full right to know
> the right response type. Rather it's a hack to rely on R1 and R6
> having the same size, like Linux does now. It would also be a hack to
> allow the CMD3 to fail with CRC error and ignore the error, like
> omap.c did before, because it makes an assumption that's not in the
> OMAP specs.

If you're referring to CERR, then I suggest you reread the specs. They
even explicitly say its okay to ignore it.

Anyway, I'm not going into this discussion again without a valid
argument as to why the suggest path won't work.

>
> The following is an ugly fix. It's ugly because host code should be
> agnostic to opcodes, as mentioned by Pierre Ossman.
> I believe this was already submitted by Carlos Aguiar and rejected by
> Pierre, which resulted in leaving the driver broken on some boards.

This just hides the fact that the host parses the data more than we want
and expect it to do.

>
> A more correct and more intrusive fix would look like this:

Afraid not. The response type specifies format, not semantical contents.

The correct fix has already been discussed, but I have seen zero
attempts at implementing it:

http://linux.omap.com/pipermail/linux-omap-open-source/2007-April/009500.html

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: SD response type 6 vs. 1

[andrzej zaborowski]

On 04/06/07, Pierre Ossman <drzeus-list@drzeus.cx> wrote:
> andrzej zaborowski wrote:
> > Hi,
> >  Linux has the same values for MMC_RSP_R1 and MMC_RSP_R6 thus making
> > the two response formats indistinguishable for mmc/sd hosts. Hardware
> > like my OMAP310 needs to know exactly whether to expect an R1 or R6
> > format, otherwise CMD3 fails.
> >
> > Sure it would be nice if it didn't care, but nevertheless, it's not a
> > hardware bug - the two formats are specified separately in SD specs
> > because they *are* different. So the hardware has full right to know
> > the right response type. Rather it's a hack to rely on R1 and R6
> > having the same size, like Linux does now. It would also be a hack to
> > allow the CMD3 to fail with CRC error and ignore the error, like
> > omap.c did before, because it makes an assumption that's not in the
> > OMAP specs.
>
> If you're referring to CERR, then I suggest you reread the specs. They
> even explicitly say its okay to ignore it.

If CERR is same as Card_Err bit in MMC_STAT, then I can't find
anything about ignoring it in OMAP310 TRM.

But the assumption here is different - that the only effect of
specifying a wrong response type is this bit getting set wrongly. The
assumption is wrong, the hardware can do anything it likes with the
information we gave it, including parsing the opcode - as long as it
conforms with TRM. Nowhere does it say that it's okay to pass R1 in
place of R6, and if it doesn't say this, the effect is defined as
"undefined". We should avoid this.

>
> Anyway, I'm not going into this discussion again without a valid
> argument as to why the suggest path won't work.
>
> >
> > The following is an ugly fix. It's ugly because host code should be
> > agnostic to opcodes, as mentioned by Pierre Ossman.
> > I believe this was already submitted by Carlos Aguiar and rejected by
> > Pierre, which resulted in leaving the driver broken on some boards.
>
> This just hides the fact that the host parses the data more than we want
> and expect it to do.

We expect what the docs say.

Regards,
Andrzej

Re: SD response type 6 vs. 1

[Carlos Aguiar]

[-- Attachment #1: Type: text/plain, Size: 2453 bytes --]

ext Pierre Ossman wrote:
> andrzej zaborowski wrote:
>   
>> Hi,
>>  Linux has the same values for MMC_RSP_R1 and MMC_RSP_R6 thus making
>> the two response formats indistinguishable for mmc/sd hosts. Hardware
>> like my OMAP310 needs to know exactly whether to expect an R1 or R6
>> format, otherwise CMD3 fails.
>>
>> Sure it would be nice if it didn't care, but nevertheless, it's not a
>> hardware bug - the two formats are specified separately in SD specs
>> because they *are* different. So the hardware has full right to know
>> the right response type. Rather it's a hack to rely on R1 and R6
>> having the same size, like Linux does now. It would also be a hack to
>> allow the CMD3 to fail with CRC error and ignore the error, like
>> omap.c did before, because it makes an assumption that's not in the
>> OMAP specs.
>>     
>
> If you're referring to CERR, then I suggest you reread the specs. They
> even explicitly say its okay to ignore it.
>
> Anyway, I'm not going into this discussion again without a valid
> argument as to why the suggest path won't work.
>
>   
>> The following is an ugly fix. It's ugly because host code should be
>> agnostic to opcodes, as mentioned by Pierre Ossman.
>> I believe this was already submitted by Carlos Aguiar and rejected by
>> Pierre, which resulted in leaving the driver broken on some boards.
>>     
>
> This just hides the fact that the host parses the data more than we want
> and expect it to do.
>
>   
>> A more correct and more intrusive fix would look like this:
>>     
>
> Afraid not. The response type specifies format, not semantical contents.
>
> The correct fix has already been discussed, but I have seen zero
> attempts at implementing it:
>
> http://linux.omap.com/pipermail/linux-omap-open-source/2007-April/009500.html
>
>   
Hi Pierre and andrzej,

Regarding the correct fix mentioned (and discussed) by Pierre, I'd
implemented a piece of this fix by removing CERR, but it's not enough to
make SD works. The patch find attached to this mail.

The part of be able to leave it out to do the card error parsing with
software, like Pierre said, must be implemented.

I'm now busy working in a patches series to MMC multislot support for
OMAP (1510, H2, H3 and H4) platforms.

Regards,

Carlos.

-- 
Carlos Eduardo
Nokia Institute of Technology - INdT
Open Source Mobile Research Center - OSMRC
Phone: +55 92 2126-1079
Mobile: +55 92 8127-1797
E-mail: carlos.aguiar@indt.org.br


[-- Attachment #2: 0001-MMC-OMAP-Totally-remove-CERR-interrupt.diff --]
[-- Type: text/plain, Size: 1811 bytes --]

Totally disable of CERR interrupt, by not writing OMAP_MMC_STAT_CARD_ERR
bit to IE register and removing OMAP_MMC_STAT_CARD_ERR quiks on omap.c

Signed-off-by: Carlos Eduardo Aguiar <carlos.aguiar@indt.org.br>

Index: linux-omap/drivers/mmc/host/omap.c
===================================================================
--- linux-omap.orig/drivers/mmc/host/omap.c	2007-06-04 14:37:56.000000000 -0400
+++ linux-omap/drivers/mmc/host/omap.c	2007-06-04 14:45:49.000000000 -0400
@@ -253,8 +253,7 @@ mmc_omap_start_command(struct mmc_omap_h
 		       OMAP_MMC_STAT_A_EMPTY    | OMAP_MMC_STAT_A_FULL    |
 		       OMAP_MMC_STAT_CMD_CRC    | OMAP_MMC_STAT_CMD_TOUT  |
 		       OMAP_MMC_STAT_DATA_CRC   | OMAP_MMC_STAT_DATA_TOUT |
-		       OMAP_MMC_STAT_END_OF_CMD | OMAP_MMC_STAT_CARD_ERR  |
-		       OMAP_MMC_STAT_END_OF_DATA);
+		       OMAP_MMC_STAT_END_OF_CMD | OMAP_MMC_STAT_END_OF_DATA);
 	OMAP_MMC_WRITE(host, CMD, cmdreg);
 }
 
@@ -523,31 +522,6 @@ static irqreturn_t mmc_omap_irq(int irq,
 					"command CRC error without cmd?\n");
 		}
 
-		if (status & OMAP_MMC_STAT_CARD_ERR) {
-			if (host->cmd && host->cmd->opcode == MMC_STOP_TRANSMISSION) {
-				u32 response = OMAP_MMC_READ(host, RSP6)
-					| (OMAP_MMC_READ(host, RSP7) << 16);
-				/* STOP sometimes sets must-ignore bits */
-				if (!(response & (R1_CC_ERROR
-								| R1_ILLEGAL_COMMAND
-								| R1_COM_CRC_ERROR))) {
-					end_command = 1;
-					continue;
-				}
-			}
-
-			dev_dbg(mmc_dev(host->mmc), "card status error (CMD%d)\n",
-				host->cmd->opcode);
-			if (host->cmd) {
-				host->cmd->error = MMC_ERR_FAILED;
-				end_command = 1;
-			}
-			if (host->data) {
-				host->data->error = MMC_ERR_FAILED;
-				transfer_error = 1;
-			}
-		}
-
 		/*
 		 * NOTE: On 1610 the END_OF_CMD may come too early when
 		 * starting a write 

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: SD response type 6 vs. 1

[Pierre Ossman]

Carlos Aguiar wrote:
> Regarding the correct fix mentioned (and discussed) by Pierre, I'd
> implemented a piece of this fix by removing CERR, but it's not enough to
> make SD works. The patch find attached to this mail.
>
>   

I suspect you can't just ignore it. You should probably treat it as if
the command completed correctly (as END_OF_CMD I suppose).

Rgds

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Fwd: SD response type 6 vs. 1

[Ragner Magalhães]

[-- Attachment #1: Type: text/plain, Size: 1845 bytes --]

---------- Forwarded message ----------
From: Pierre Ossman <drzeus-list@drzeus.cx>
Date: Jun 7, 2007 4:24 AM
Subject: Re: SD response type 6 vs. 1
To: Ragner Magalhães <ragner@users.sourceforge.net>


Ragner Magalhães wrote:
> I tested this patch and it is working ...
>

Looks good. Could you send it out to the omap list so that others can
test it?

Somebody more could to test this patch?

---

 drivers/mmc/host/omap.c |   24 +++---------------------
 1 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/mmc/host/omap.c b/drivers/mmc/host/omap.c
index 383bd54..b6a55e1 100644
--- a/drivers/mmc/host/omap.c
+++ b/drivers/mmc/host/omap.c
@@ -524,28 +524,10 @@ static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
 		}

 		if (status & OMAP_MMC_STAT_CARD_ERR) {
-			if (host->cmd && host->cmd->opcode == MMC_STOP_TRANSMISSION) {
-				u32 response = OMAP_MMC_READ(host, RSP6)
-					| (OMAP_MMC_READ(host, RSP7) << 16);
-				/* STOP sometimes sets must-ignore bits */
-				if (!(response & (R1_CC_ERROR
-								| R1_ILLEGAL_COMMAND
-								| R1_COM_CRC_ERROR))) {
-					end_command = 1;
-					continue;
-				}
-			}
-
-			dev_dbg(mmc_dev(host->mmc), "card status error (CMD%d)\n",
+			dev_dbg(mmc_dev(host->mmc),
+				"ignoring card status error (CMD%d)\n",
 				host->cmd->opcode);
-			if (host->cmd) {
-				host->cmd->error = MMC_ERR_FAILED;
-				end_command = 1;
-			}
-			if (host->data) {
-				host->data->error = MMC_ERR_FAILED;
-				transfer_error = 1;
-			}
+			end_command = 1;
 		}

 		/*

I am sending it attached too ...

Best regards,

-- 
Ragner N Magalhães

Instituto Nokia de Tecnologia - INdT
Open Source Mobile Research Center - OSMRC
Linux Kernel Team

E-mail: ragner.magalhaes@indt.org.br
            ragner@users.sourceforge.net

[-- Attachment #2: omap-mmc_rsp_r6.diff --]
[-- Type: text/x-patch, Size: 1498 bytes --]

SD Card working on OMAP platforms

From: Ragner Magalhaes <ragner.magalhaes@indt.org.br>

Fixing reponse type R6 for CMD3 when SD Card will be attached.

Ignoring OMAP_MMC_STAT_CARD_ERR, treating it as if the command
completed correctly.

Signed-off-by: Ragner Magalhaes <ragner.magalhaes@indt.org.br>
Signed-off-by: Carlos Eduardo Aguiar <carlos.aguiar@indt.org.br>
---

 drivers/mmc/host/omap.c |   24 +++---------------------
 1 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/mmc/host/omap.c b/drivers/mmc/host/omap.c
index 383bd54..b6a55e1 100644
--- a/drivers/mmc/host/omap.c
+++ b/drivers/mmc/host/omap.c
@@ -524,28 +524,10 @@ static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
 		}
 
 		if (status & OMAP_MMC_STAT_CARD_ERR) {
-			if (host->cmd && host->cmd->opcode == MMC_STOP_TRANSMISSION) {
-				u32 response = OMAP_MMC_READ(host, RSP6)
-					| (OMAP_MMC_READ(host, RSP7) << 16);
-				/* STOP sometimes sets must-ignore bits */
-				if (!(response & (R1_CC_ERROR
-								| R1_ILLEGAL_COMMAND
-								| R1_COM_CRC_ERROR))) {
-					end_command = 1;
-					continue;
-				}
-			}
-
-			dev_dbg(mmc_dev(host->mmc), "card status error (CMD%d)\n",
+			dev_dbg(mmc_dev(host->mmc),
+				"ignoring card status error (CMD%d)\n",
 				host->cmd->opcode);
-			if (host->cmd) {
-				host->cmd->error = MMC_ERR_FAILED;
-				end_command = 1;
-			}
-			if (host->data) {
-				host->data->error = MMC_ERR_FAILED;
-				transfer_error = 1;
-			}
+			end_command = 1;
 		}
 
 		/*

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]