Fix SPI SD emulation

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(-)

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.  */

> > -    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

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.

> >> 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