Patchwork Fix SPI SD emulation

login
register
mail settings
Submitter Paul Brook
Date April 25, 2012, 9:12 p.m.
Message ID <1335388332-28640-1-git-send-email-paul@codesourcery.com>
Download mbox | patch
Permalink /patch/155104/
State New
Headers show

Comments

Paul Brook - April 25, 2012, 9:12 p.m.
When in SPI mode, we give a bogus response to CMD8 (part of the SD physical
spec v2).

Fixing this also makes most drivers also issue CMD58.  Despite some
skeleton code in hw/ssi-sd.c this isn't actually implemented in hw/sd.h.

CMD58 is valid in both idle and active states, so the hardcoded status
byte is insufficient.  CMD58 is specific to SPI mode.  It is undefined in
regular SD mode.

Signed-off-by: Paul Brook <paul@codesourcery.com>
---
 hw/sd.c     |   57 ++++++++++++++++++++++++++++++++++++++++++---------------
 hw/ssi-sd.c |   11 +++++------
 2 files changed, 47 insertions(+), 21 deletions(-)
Mitsyanko Igor - April 26, 2012, 12:34 p.m.
On 04/26/2012 01:12 AM, Paul Brook wrote:
> When in SPI mode, we give a bogus response to CMD8 (part of the SD physical
> spec v2).
>
> Fixing this also makes most drivers also issue CMD58.  Despite some
> skeleton code in hw/ssi-sd.c this isn't actually implemented in hw/sd.h.
>
> CMD58 is valid in both idle and active states, so the hardcoded status
> byte is insufficient.  CMD58 is specific to SPI mode.  It is undefined in
> regular SD mode.
>
> Signed-off-by: Paul Brook<paul@codesourcery.com>
> ---
>   hw/sd.c     |   57 ++++++++++++++++++++++++++++++++++++++++++---------------
>   hw/ssi-sd.c |   11 +++++------
>   2 files changed, 47 insertions(+), 21 deletions(-)
>
> diff --git a/hw/sd.c b/hw/sd.c
> index 07eb263..ed7f5cb 100644
> --- a/hw/sd.c
> +++ b/hw/sd.c
> @@ -140,7 +140,7 @@ static const sd_cmd_type_t sd_cmd_type[64] = {
>       sd_ac,   sd_ac,   sd_none, sd_none, sd_none, sd_none, sd_ac,   sd_none,
>       sd_none, sd_none, sd_bc,   sd_none, sd_none, sd_none, sd_none, sd_none,
>       sd_none, sd_none, sd_none, sd_none, sd_none, sd_none, sd_none, sd_ac,
> -    sd_adtc, sd_none, sd_none, sd_none, sd_none, sd_none, sd_none, sd_none,
> +    sd_adtc, sd_none, sd_bc,   sd_bc,   sd_none, sd_none, sd_none, sd_none,
>   };

sd_bcr? not that it really matters though

>
>   static const sd_cmd_type_t sd_acmd_type[64] = {
> @@ -354,12 +354,20 @@ static void sd_response_r1_make(SDState *sd, uint8_t *response)
>       response[3] = (status>>  0)&  0xff;
>   }
>
> -static void sd_response_r3_make(SDState *sd, uint8_t *response)
> +static int sd_response_r3_make(SDState *sd, uint8_t *response)
>   {
> -    response[0] = (sd->ocr>>  24)&  0xff;
> -    response[1] = (sd->ocr>>  16)&  0xff;
> -    response[2] = (sd->ocr>>  8)&  0xff;
> -    response[3] = (sd->ocr>>  0)&  0xff;
> +    int len = 4;
> +
> +    if (sd->spi) {
> +        len = 5;
> +        *(response++) = (sd->state == sd_idle_state) ? 1 : 0;
> +    }
> +    *(response++) = (sd->ocr>>  24)&  0xff;
> +    *(response++) = (sd->ocr>>  16)&  0xff;
> +    *(response++) = (sd->ocr>>  8)&  0xff;
> +    *(response++) = (sd->ocr>>  0)&  0xff;
> +
> +    return len;
>   }
>
>   static void sd_response_r6_make(SDState *sd, uint8_t *response)
> @@ -379,12 +387,20 @@ static void sd_response_r6_make(SDState *sd, uint8_t *response)
>       response[3] = status&  0xff;
>   }
>
> -static void sd_response_r7_make(SDState *sd, uint8_t *response)
> +static int sd_response_r7_make(SDState *sd, uint8_t *response)
>   {
> -    response[0] = (sd->vhs>>  24)&  0xff;
> -    response[1] = (sd->vhs>>  16)&  0xff;
> -    response[2] = (sd->vhs>>   8)&  0xff;
> -    response[3] = (sd->vhs>>   0)&  0xff;
> +    int len = 4;
> +
> +    if (sd->spi) {
> +        len = 5;
> +        *(response++) = (sd->state == sd_idle_state) ? 1 : 0;
> +    }
> +    *(response++) = (sd->vhs>>  24)&  0xff;
> +    *(response++) = (sd->vhs>>  16)&  0xff;
> +    *(response++) = (sd->vhs>>   8)&  0xff;
> +    *(response++) = (sd->vhs>>   0)&  0xff;
> +
> +    return len;
>   }
>
>   static void sd_reset(SDState *sd, BlockDriverState *bdrv)
> @@ -1145,6 +1161,19 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
>               break;
>           }
>           break;
> +    case 58:    /* CMD58: READ_OCR */
> +        if (!sd->spi) {
> +            goto bad_cmd;
> +        }
> +        switch (sd->state) {
> +        case sd_idle_state:
> +        case sd_transfer_state:
> +            return sd_r3;

If this command could be issued in transfer state maybe in addition to 
IDLE_STATE you also need to set other bits (ADDRESS_ERROR, 
COM_CRC_ERROR, ILLEGAL_COMMAND, ERASE_SEQ_ERROR) in MSB of R3 response?

> +
> +        default:
> +            break;
> +        }
> +        break;
>
>       default:
>       bad_cmd:
> @@ -1354,8 +1383,7 @@ send_response:
>           break;
>
>       case sd_r3:
> -        sd_response_r3_make(sd, response);
> -        rsplen = 4;
> +        rsplen = sd_response_r3_make(sd, response);
>           break;
>
>       case sd_r6:
> @@ -1364,8 +1392,7 @@ send_response:
>           break;
>
>       case sd_r7:
> -        sd_response_r7_make(sd, response);
> -        rsplen = 4;
> +        rsplen = sd_response_r7_make(sd, response);
>           break;
>
>       case sd_r0:
> diff --git a/hw/ssi-sd.c b/hw/ssi-sd.c
> index b519bdb..b868df0 100644
> --- a/hw/ssi-sd.c
> +++ b/hw/ssi-sd.c
> @@ -99,12 +99,11 @@ static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val)
>                   s->arglen = 1;
>                   s->response[0] = 4;
>                   DPRINTF("SD command failed\n");
> -            } else if (s->cmd == 58) {
> -                /* CMD58 returns R3 response (OCR)  */
> -                DPRINTF("Returned OCR\n");
> -                s->arglen = 5;
> -                s->response[0] = 1;
> -                memcpy(&s->response[1], longresp, 4);
> +            } else if (s->arglen == 5) {
> +                /* CMD58 returns R3 response (OCR)
> +                   CMD8 returns R7 response (operating voltage)  */
> +                DPRINTF("Returned OCR or Voltage\n");
> +                memcpy(&s->response, longresp, 5);
>               } else if (s->arglen != 4) {
>                   BADF("Unexpected response to cmd %d\n", s->cmd);
>                   /* Illegal command is about as near as we can get.  */
Paul Brook - April 29, 2012, 3:31 p.m.
> > -    sd_adtc, sd_none, sd_none, sd_none, sd_none, sd_none, sd_none,
> > +    sd_adtc, sd_none, sd_bc,   sd_bc,   sd_none, sd_none,
> 
> sd_bcr? not that it really matters though

Err, probably.

> > +    case 58:    /* CMD58: READ_OCR */
> > +        if (!sd->spi) {
> > +            goto bad_cmd;
> > +        }
> > +        switch (sd->state) {
> > +        case sd_idle_state:
> > +        case sd_transfer_state:
> > +            return sd_r3;
> 
> If this command could be issued in transfer state maybe in addition to
> IDLE_STATE you also need to set other bits (ADDRESS_ERROR,
> COM_CRC_ERROR, ILLEGAL_COMMAND, ERASE_SEQ_ERROR) in MSB of R3 response?

In theory, yes.  I was thinking of a follow-up patch to move the spi status 
byte generation into sd.c.  Maybe I should do that first.

Paul
Igor Mitsyanko - April 29, 2012, 10:12 p.m.
On 29.04.2012 6:31 PM, Paul Brook wrote:
>>> -    sd_adtc, sd_none, sd_none, sd_none, sd_none, sd_none, sd_none,
>>> +    sd_adtc, sd_none, sd_bc,   sd_bc,   sd_none, sd_none,
>>
>> sd_bcr? not that it really matters though
>
> Err, probably.
>
>>> +    case 58:    /* CMD58: READ_OCR */
>>> +        if (!sd->spi) {
>>> +            goto bad_cmd;
>>> +        }
>>> +        switch (sd->state) {
>>> +        case sd_idle_state:
>>> +        case sd_transfer_state:
>>> +            return sd_r3;
>>
>> If this command could be issued in transfer state maybe in addition to
>> IDLE_STATE you also need to set other bits (ADDRESS_ERROR,
>> COM_CRC_ERROR, ILLEGAL_COMMAND, ERASE_SEQ_ERROR) in MSB of R3 response?
>
> In theory, yes.  I was thinking of a follow-up patch to move the spi status
> byte generation into sd.c.  Maybe I should do that first.

Do you mean the one in ssi-sd.c? That would be nice I think, a bit less 
confusing.
Paul Brook - April 30, 2012, 3:27 p.m.
> >> If this command could be issued in transfer state maybe in addition to
> >> IDLE_STATE you also need to set other bits (ADDRESS_ERROR,
> >> COM_CRC_ERROR, ILLEGAL_COMMAND, ERASE_SEQ_ERROR) in MSB of R3 response?
> > 
> > In theory, yes.  I was thinking of a follow-up patch to move the spi
> > status byte generation into sd.c.  Maybe I should do that first.
> 
> Do you mean the one in ssi-sd.c? That would be nice I think, a bit less
> confusing.

I posted v2 of the patch earlier today:

http://lists.nongnu.org/archive/html/qemu-devel/2012-04/msg04214.html

Paul

Patch

diff --git a/hw/sd.c b/hw/sd.c
index 07eb263..ed7f5cb 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -140,7 +140,7 @@  static const sd_cmd_type_t sd_cmd_type[64] = {
     sd_ac,   sd_ac,   sd_none, sd_none, sd_none, sd_none, sd_ac,   sd_none,
     sd_none, sd_none, sd_bc,   sd_none, sd_none, sd_none, sd_none, sd_none,
     sd_none, sd_none, sd_none, sd_none, sd_none, sd_none, sd_none, sd_ac,
-    sd_adtc, sd_none, sd_none, sd_none, sd_none, sd_none, sd_none, sd_none,
+    sd_adtc, sd_none, sd_bc,   sd_bc,   sd_none, sd_none, sd_none, sd_none,
 };
 
 static const sd_cmd_type_t sd_acmd_type[64] = {
@@ -354,12 +354,20 @@  static void sd_response_r1_make(SDState *sd, uint8_t *response)
     response[3] = (status >> 0) & 0xff;
 }
 
-static void sd_response_r3_make(SDState *sd, uint8_t *response)
+static int sd_response_r3_make(SDState *sd, uint8_t *response)
 {
-    response[0] = (sd->ocr >> 24) & 0xff;
-    response[1] = (sd->ocr >> 16) & 0xff;
-    response[2] = (sd->ocr >> 8) & 0xff;
-    response[3] = (sd->ocr >> 0) & 0xff;
+    int len = 4;
+
+    if (sd->spi) {
+        len = 5;
+        *(response++) = (sd->state == sd_idle_state) ? 1 : 0;
+    }
+    *(response++) = (sd->ocr >> 24) & 0xff;
+    *(response++) = (sd->ocr >> 16) & 0xff;
+    *(response++) = (sd->ocr >> 8) & 0xff;
+    *(response++) = (sd->ocr >> 0) & 0xff;
+
+    return len;
 }
 
 static void sd_response_r6_make(SDState *sd, uint8_t *response)
@@ -379,12 +387,20 @@  static void sd_response_r6_make(SDState *sd, uint8_t *response)
     response[3] = status & 0xff;
 }
 
-static void sd_response_r7_make(SDState *sd, uint8_t *response)
+static int sd_response_r7_make(SDState *sd, uint8_t *response)
 {
-    response[0] = (sd->vhs >> 24) & 0xff;
-    response[1] = (sd->vhs >> 16) & 0xff;
-    response[2] = (sd->vhs >>  8) & 0xff;
-    response[3] = (sd->vhs >>  0) & 0xff;
+    int len = 4;
+
+    if (sd->spi) {
+        len = 5;
+        *(response++) = (sd->state == sd_idle_state) ? 1 : 0;
+    }
+    *(response++) = (sd->vhs >> 24) & 0xff;
+    *(response++) = (sd->vhs >> 16) & 0xff;
+    *(response++) = (sd->vhs >>  8) & 0xff;
+    *(response++) = (sd->vhs >>  0) & 0xff;
+
+    return len;
 }
 
 static void sd_reset(SDState *sd, BlockDriverState *bdrv)
@@ -1145,6 +1161,19 @@  static sd_rsp_type_t sd_normal_command(SDState *sd,
             break;
         }
         break;
+    case 58:    /* CMD58: READ_OCR */
+        if (!sd->spi) {
+            goto bad_cmd;
+        }
+        switch (sd->state) {
+        case sd_idle_state:
+        case sd_transfer_state:
+            return sd_r3;
+
+        default:
+            break;
+        }
+        break;
 
     default:
     bad_cmd:
@@ -1354,8 +1383,7 @@  send_response:
         break;
 
     case sd_r3:
-        sd_response_r3_make(sd, response);
-        rsplen = 4;
+        rsplen = sd_response_r3_make(sd, response);
         break;
 
     case sd_r6:
@@ -1364,8 +1392,7 @@  send_response:
         break;
 
     case sd_r7:
-        sd_response_r7_make(sd, response);
-        rsplen = 4;
+        rsplen = sd_response_r7_make(sd, response);
         break;
 
     case sd_r0:
diff --git a/hw/ssi-sd.c b/hw/ssi-sd.c
index b519bdb..b868df0 100644
--- a/hw/ssi-sd.c
+++ b/hw/ssi-sd.c
@@ -99,12 +99,11 @@  static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val)
                 s->arglen = 1;
                 s->response[0] = 4;
                 DPRINTF("SD command failed\n");
-            } else if (s->cmd == 58) {
-                /* CMD58 returns R3 response (OCR)  */
-                DPRINTF("Returned OCR\n");
-                s->arglen = 5;
-                s->response[0] = 1;
-                memcpy(&s->response[1], longresp, 4);
+            } else if (s->arglen == 5) {
+                /* CMD58 returns R3 response (OCR)
+                   CMD8 returns R7 response (operating voltage)  */
+                DPRINTF("Returned OCR or Voltage\n");
+                memcpy(&s->response, longresp, 5);
             } else if (s->arglen != 4) {
                 BADF("Unexpected response to cmd %d\n", s->cmd);
                 /* Illegal command is about as near as we can get.  */