diff mbox series

hw/sd: Correct CMD58's R3 response "in idle state" bit in SPI-mode

Message ID 20220126031345.3372-1-frank.chang@sifive.com
State New
Headers show
Series hw/sd: Correct CMD58's R3 response "in idle state" bit in SPI-mode | expand

Commit Message

Frank Chang Jan. 26, 2022, 3:13 a.m. UTC 
From: Frank Chang <frank.chang@sifive.com>

In SPI-mode, CMD58 returns R3 response with the format:

39          32 31                                  0
+------------+ +-----------------------------------+
|     R1     | |                OCR                |
+------------+ +-----------------------------------+

Where R1 has bits[0] indicating whether SD card is "in idle state".
However, according to SD card state transition table, CMD58 can only be
transited from trans to data state, which the "in idle state" bit should
not be set in CMD58's R3 response.
(But CMD8 should still have "in idle state" bit to be set in its
R7 response because it can only be transited from idle to idle state.)

Signed-off-by: Frank Chang <frank.chang@sifive.com>
---
 hw/sd/ssi-sd.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Peter Maydell Feb. 7, 2022, 3:13 p.m. UTC | #1 
On Wed, 26 Jan 2022 at 03:13, <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@sifive.com>
>
> In SPI-mode, CMD58 returns R3 response with the format:
>
> 39          32 31                                  0
> +------------+ +-----------------------------------+
> |     R1     | |                OCR                |
> +------------+ +-----------------------------------+
>
> Where R1 has bits[0] indicating whether SD card is "in idle state".
> However, according to SD card state transition table, CMD58 can only be
> transited from trans to data state, which the "in idle state" bit should
> not be set in CMD58's R3 response.
> (But CMD8 should still have "in idle state" bit to be set in its
> R7 response because it can only be transited from idle to idle state.)
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> ---
>  hw/sd/ssi-sd.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index 167c03b780..7faa969e82 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -176,12 +176,17 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
>                  s->arglen = 1;
>                  s->response[0] = 4;
>                  DPRINTF("SD command failed\n");
> -            } else if (s->cmd == 8 || s->cmd == 58) {
> -                /* CMD8/CMD58 returns R3/R7 response */
> -                DPRINTF("Returned R3/R7\n");
> +            } else if (s->cmd == 8) {
> +                /* CMD8 returns R7 response */
> +                DPRINTF("Returned R7\n");
>                  s->arglen = 5;
>                  s->response[0] = 1;
>                  memcpy(&s->response[1], longresp, 4);
> +            } else if (s->cmd == 58) {
> +                /* CMD58 returns R3 response */
> +                DPRINTF("Returned R3\n");
> +                s->arglen = 5;
> +                memcpy(&s->response[1], longresp, 4);
>              } else if (s->arglen != 4) {
>                  BADF("Unexpected response to cmd %d\n", s->cmd);
>                  /* Illegal command is about as near as we can get.  */

This demonstrates a problem with trying to implement SPI mode
as "SD mode but the controller has to fix up the differences".
Here CMD8 and CMD58 in SPI mode are supposed to return
5 bytes of data, of which one byte is format-R1 status information.
But our SD card emulation returns SD mode R3 or R7 format,
which don't include that information. In the current code
in ssi-sd.c, which this patch is tweaking, we try to fake up a
status byte. However, this isn't going to have the right behaviour,
because this should count as a read that clears the "clear on read"
status bits, and faking up the response byte won't do that.

(For that matter, presumably the clear-on-read bits that are
only present in the R2 2-byte response should not be cleared
when the SPI command gets a 1-byte R1 response. I think
that the other patch you've sent won't do that.)

That all leaves me wondering if what we should really do here
is make sd.c's SPI mode actually change the format of
responses, rather than leaving it to the controller to try
to do that. (This would also be useful if in future we need to
model more than one controller that puts the card in SPI mode.)

Side note, in sd.c sd_normal_command() we "return sd_r3" for
the cmd58 case, so I think this will cause us to actually send
the OCR data rather than the voltage-status data...

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 167c03b780..7faa969e82 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -176,12 +176,17 @@  static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
                 s->arglen = 1;
                 s->response[0] = 4;
                 DPRINTF("SD command failed\n");
-            } else if (s->cmd == 8 || s->cmd == 58) {
-                /* CMD8/CMD58 returns R3/R7 response */
-                DPRINTF("Returned R3/R7\n");
+            } else if (s->cmd == 8) {
+                /* CMD8 returns R7 response */
+                DPRINTF("Returned R7\n");
                 s->arglen = 5;
                 s->response[0] = 1;
                 memcpy(&s->response[1], longresp, 4);
+            } else if (s->cmd == 58) {
+                /* CMD58 returns R3 response */
+                DPRINTF("Returned R3\n");
+                s->arglen = 5;
+                memcpy(&s->response[1], longresp, 4);
             } else if (s->arglen != 4) {
                 BADF("Unexpected response to cmd %d\n", s->cmd);
                 /* Illegal command is about as near as we can get.  */