[PATCH v4 0/9] hw/sd: Support block read/write in SPI mode

[PATCH v4 0/9] hw/sd: Support block read/write in SPI mode

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

This includes the previously v3 series [1], and one single patch [2].

Compared to v3, this fixed the following issue in patch [v3,6/6]:
- Keep the card state to SSI_SD_CMD instead of SSI_SD_RESPONSE after
  receiving the STOP_TRAN token per the spec

All software tested so far (U-Boot/Linux/VxWorks) do work without
the fix, but it is better to comform with the spec.

In addition to [2], one more issue was exposed when testing with
VxWorks driver related to STOP_TRANSMISSION (CMD12) response.

[1] http://patchwork.ozlabs.org/project/qemu-devel/list/?series=226136
[2] http://patchwork.ozlabs.org/project/qemu-devel/patch/1611636214-52427-1-git-send-email-bmeng.cn@gmail.com/

Changes in v4:
- Keep the card state to SSI_SD_CMD instead of SSI_SD_RESPONSE after
  receiving the STOP_TRAN token per the spec
- new patch: fix STOP_TRANSMISSION (CMD12) response
- new patch: handle the rest commands with R1b response type

Bin Meng (9):
  hw/sd: ssi-sd: Support multiple block read
  hw/sd: sd: Remove duplicated codes in single/multiple block read/write
  hw/sd: sd: Allow single/multiple block write for SPI mode
  hw/sd: Introduce receive_ready() callback
  hw/sd: ssi-sd: Support single block write
  hw/sd: ssi-sd: Support multiple block write
  hw/sd: ssi-sd: Fix SEND_IF_COND (CMD8) response
  hw/sd: ssi-sd: Fix STOP_TRANSMISSION (CMD12) response
  hw/sd: ssi-sd: Handle the rest commands with R1b response type

 include/hw/sd/sd.h |   2 +
 hw/sd/core.c       |  13 +++++
 hw/sd/sd.c         |  56 ++-----------------
 hw/sd/ssi-sd.c     | 136 ++++++++++++++++++++++++++++++++++++++-------
 4 files changed, 137 insertions(+), 70 deletions(-)

-- 
2.25.1

[PATCH v4 1/9] hw/sd: ssi-sd: Support multiple block read

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

In the case of a multiple block read operation every transferred
block has its suffix of CRC16. Update the state machine logic to
handle multiple block read.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
[PMD: Change VMState version id 5 -> 6]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

(no changes since v1)

 hw/sd/ssi-sd.c | 42 +++++++++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index be1bb10164..6d20a240c6 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -52,6 +52,7 @@ struct ssi_sd_state {
     uint8_t cmdarg[4];
     uint8_t response[5];
     uint16_t crc16;
+    int32_t read_bytes;
     int32_t arglen;
     int32_t response_pos;
     int32_t stopping;
@@ -88,11 +89,26 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
 {
     ssi_sd_state *s = SSI_SD(dev);
 
-    /* Special case: allow CMD12 (STOP TRANSMISSION) while reading data.  */
-    if (s->mode == SSI_SD_DATA_READ && val == 0x4c) {
-        s->mode = SSI_SD_CMD;
-        /* There must be at least one byte delay before the card responds.  */
-        s->stopping = 1;
+    /*
+     * Special case: allow CMD12 (STOP TRANSMISSION) while reading data.
+     *
+     * See "Physical Layer Specification Version 8.00" chapter 7.5.2.2,
+     * to avoid conflict between CMD12 response and next data block,
+     * timing of CMD12 should be controlled as follows:
+     *
+     * - CMD12 issued at the timing that end bit of CMD12 and end bit of
+     *   data block is overlapped
+     * - CMD12 issued after one clock cycle after host receives a token
+     *   (either Start Block token or Data Error token)
+     *
+     * We need to catch CMD12 in all of the data read states.
+     */
+    if (s->mode >= SSI_SD_PREP_DATA && s->mode <= SSI_SD_DATA_CRC16) {
+        if (val == 0x4c) {
+            s->mode = SSI_SD_CMD;
+            /* There must be at least one byte delay before the card responds */
+            s->stopping = 1;
+        }
     }
 
     switch (s->mode) {
@@ -212,8 +228,9 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
         return SSI_TOKEN_SINGLE;
     case SSI_SD_DATA_READ:
         val = sdbus_read_byte(&s->sdbus);
+        s->read_bytes++;
         s->crc16 = crc_ccitt_false(s->crc16, (uint8_t *)&val, 1);
-        if (!sdbus_data_ready(&s->sdbus)) {
+        if (!sdbus_data_ready(&s->sdbus) || s->read_bytes == 512) {
             DPRINTF("Data read end\n");
             s->mode = SSI_SD_DATA_CRC16;
         }
@@ -224,7 +241,12 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
         s->response_pos++;
         if (s->response_pos == 2) {
             DPRINTF("CRC16 read end\n");
-            s->mode = SSI_SD_CMD;
+            if (s->read_bytes == 512 && s->cmd != 17) {
+                s->mode = SSI_SD_PREP_DATA;
+            } else {
+                s->mode = SSI_SD_CMD;
+            }
+            s->read_bytes = 0;
             s->response_pos = 0;
         }
         return val;
@@ -255,8 +277,8 @@ static int ssi_sd_post_load(void *opaque, int version_id)
 
 static const VMStateDescription vmstate_ssi_sd = {
     .name = "ssi_sd",
-    .version_id = 5,
-    .minimum_version_id = 5,
+    .version_id = 6,
+    .minimum_version_id = 6,
     .post_load = ssi_sd_post_load,
     .fields = (VMStateField []) {
         VMSTATE_UINT32(mode, ssi_sd_state),
@@ -264,6 +286,7 @@ static const VMStateDescription vmstate_ssi_sd = {
         VMSTATE_UINT8_ARRAY(cmdarg, ssi_sd_state, 4),
         VMSTATE_UINT8_ARRAY(response, ssi_sd_state, 5),
         VMSTATE_UINT16(crc16, ssi_sd_state),
+        VMSTATE_INT32(read_bytes, ssi_sd_state),
         VMSTATE_INT32(arglen, ssi_sd_state),
         VMSTATE_INT32(response_pos, ssi_sd_state),
         VMSTATE_INT32(stopping, ssi_sd_state),
@@ -316,6 +339,7 @@ static void ssi_sd_reset(DeviceState *dev)
     memset(s->cmdarg, 0, sizeof(s->cmdarg));
     memset(s->response, 0, sizeof(s->response));
     s->crc16 = 0;
+    s->read_bytes = 0;
     s->arglen = 0;
     s->response_pos = 0;
     s->stopping = 0;
-- 
2.25.1

[PATCH v4 2/9] hw/sd: sd: Remove duplicated codes in single/multiple block read/write

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

The single block read (CMD17) codes are the same as the multiple
block read (CMD18). Merge them into one. The same applies to single
block write (CMD24) and multiple block write (CMD25).

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

(no changes since v1)

 hw/sd/sd.c | 47 -----------------------------------------------
 1 file changed, 47 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b3952514fe..09753359bb 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1181,24 +1181,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         break;
 
     case 17:	/* CMD17:  READ_SINGLE_BLOCK */
-        switch (sd->state) {
-        case sd_transfer_state:
-
-            if (addr + sd->blk_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
-                return sd_r1;
-            }
-
-            sd->state = sd_sendingdata_state;
-            sd->data_start = addr;
-            sd->data_offset = 0;
-            return sd_r1;
-
-        default:
-            break;
-        }
-        break;
-
     case 18:	/* CMD18:  READ_MULTIPLE_BLOCK */
         switch (sd->state) {
         case sd_transfer_state:
@@ -1245,35 +1227,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
     /* Block write commands (Class 4) */
     case 24:	/* CMD24:  WRITE_SINGLE_BLOCK */
-        switch (sd->state) {
-        case sd_transfer_state:
-            /* Writing in SPI mode not implemented.  */
-            if (sd->spi)
-                break;
-
-            if (addr + sd->blk_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
-                return sd_r1;
-            }
-
-            sd->state = sd_receivingdata_state;
-            sd->data_start = addr;
-            sd->data_offset = 0;
-            sd->blk_written = 0;
-
-            if (sd_wp_addr(sd, sd->data_start)) {
-                sd->card_status |= WP_VIOLATION;
-            }
-            if (sd->csd[14] & 0x30) {
-                sd->card_status |= WP_VIOLATION;
-            }
-            return sd_r1;
-
-        default:
-            break;
-        }
-        break;
-
     case 25:	/* CMD25:  WRITE_MULTIPLE_BLOCK */
         switch (sd->state) {
         case sd_transfer_state:
-- 
2.25.1

[PATCH v4 3/9] hw/sd: sd: Allow single/multiple block write for SPI mode

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

At present the single/multiple block write in SPI mode is blocked
by sd_normal_command(). Remove the limitation.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

(no changes since v1)

 hw/sd/sd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 09753359bb..946036d87c 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1230,9 +1230,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     case 25:	/* CMD25:  WRITE_MULTIPLE_BLOCK */
         switch (sd->state) {
         case sd_transfer_state:
-            /* Writing in SPI mode not implemented.  */
-            if (sd->spi)
-                break;
 
             if (addr + sd->blk_len > sd->size) {
                 sd->card_status |= ADDRESS_ERROR;
-- 
2.25.1

[PATCH v4 4/9] hw/sd: Introduce receive_ready() callback

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

At present there is a data_ready() callback for the SD data read
path. Let's add a receive_ready() for the SD data write path.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

(no changes since v1)

 include/hw/sd/sd.h |  2 ++
 hw/sd/core.c       | 13 +++++++++++++
 hw/sd/sd.c         |  6 ++++++
 3 files changed, 21 insertions(+)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 05ef9b73e5..47360ba4ee 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -116,6 +116,7 @@ struct SDCardClass {
      * Return: byte value read
      */
     uint8_t (*read_byte)(SDState *sd);
+    bool (*receive_ready)(SDState *sd);
     bool (*data_ready)(SDState *sd);
     void (*set_voltage)(SDState *sd, uint16_t millivolts);
     uint8_t (*get_dat_lines)(SDState *sd);
@@ -187,6 +188,7 @@ void sdbus_write_data(SDBus *sdbus, const void *buf, size_t length);
  * Read multiple bytes of data on the data lines of a SD bus.
  */
 void sdbus_read_data(SDBus *sdbus, void *buf, size_t length);
+bool sdbus_receive_ready(SDBus *sd);
 bool sdbus_data_ready(SDBus *sd);
 bool sdbus_get_inserted(SDBus *sd);
 bool sdbus_get_readonly(SDBus *sd);
diff --git a/hw/sd/core.c b/hw/sd/core.c
index 08c93b5903..30ee62c510 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -160,6 +160,19 @@ void sdbus_read_data(SDBus *sdbus, void *buf, size_t length)
     }
 }
 
+bool sdbus_receive_ready(SDBus *sdbus)
+{
+    SDState *card = get_card(sdbus);
+
+    if (card) {
+        SDCardClass *sc = SD_CARD_GET_CLASS(card);
+
+        return sc->receive_ready(card);
+    }
+
+    return false;
+}
+
 bool sdbus_data_ready(SDBus *sdbus)
 {
     SDState *card = get_card(sdbus);
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 946036d87c..c99c0e93bb 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2037,6 +2037,11 @@ uint8_t sd_read_byte(SDState *sd)
     return ret;
 }
 
+static bool sd_receive_ready(SDState *sd)
+{
+    return sd->state == sd_receivingdata_state;
+}
+
 static bool sd_data_ready(SDState *sd)
 {
     return sd->state == sd_sendingdata_state;
@@ -2147,6 +2152,7 @@ static void sd_class_init(ObjectClass *klass, void *data)
     sc->do_command = sd_do_command;
     sc->write_byte = sd_write_byte;
     sc->read_byte = sd_read_byte;
+    sc->receive_ready = sd_receive_ready;
     sc->data_ready = sd_data_ready;
     sc->enable = sd_enable;
     sc->get_inserted = sd_get_inserted;
-- 
2.25.1

[PATCH v4 5/9] hw/sd: ssi-sd: Support single block write

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

Add 2 more states for the block write operation. The SPI host needs
to send a data start token to start the transfer, and the data block
written to the card will be acknowledged by a data response token.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
[PMD: Change VMState version id 6 -> 7]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

(no changes since v1)

 hw/sd/ssi-sd.c | 44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 6d20a240c6..1205ad8b52 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -43,6 +43,8 @@ typedef enum {
     SSI_SD_DATA_START,
     SSI_SD_DATA_READ,
     SSI_SD_DATA_CRC16,
+    SSI_SD_DATA_WRITE,
+    SSI_SD_SKIP_CRC16,
 } ssi_sd_mode;
 
 struct ssi_sd_state {
@@ -53,6 +55,7 @@ struct ssi_sd_state {
     uint8_t response[5];
     uint16_t crc16;
     int32_t read_bytes;
+    int32_t write_bytes;
     int32_t arglen;
     int32_t response_pos;
     int32_t stopping;
@@ -85,6 +88,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(ssi_sd_state, SSI_SD)
 /* dummy value - don't care */
 #define SSI_DUMMY               0xff
 
+/* data accepted */
+#define DATA_RESPONSE_ACCEPTED  0x05
+
 static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
 {
     ssi_sd_state *s = SSI_SD(dev);
@@ -113,10 +119,17 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
 
     switch (s->mode) {
     case SSI_SD_CMD:
-        if (val == SSI_DUMMY) {
+        switch (val) {
+        case SSI_DUMMY:
             DPRINTF("NULL command\n");
             return SSI_DUMMY;
+            break;
+        case SSI_TOKEN_SINGLE:
+            DPRINTF("Start write block\n");
+            s->mode = SSI_SD_DATA_WRITE;
+            return SSI_DUMMY;
         }
+
         s->cmd = val & 0x3f;
         s->mode = SSI_SD_CMDARG;
         s->arglen = 0;
@@ -250,6 +263,27 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
             s->response_pos = 0;
         }
         return val;
+    case SSI_SD_DATA_WRITE:
+        sdbus_write_byte(&s->sdbus, val);
+        s->write_bytes++;
+        if (!sdbus_receive_ready(&s->sdbus) || s->write_bytes == 512) {
+            DPRINTF("Data write end\n");
+            s->mode = SSI_SD_SKIP_CRC16;
+            s->response_pos = 0;
+        }
+        return val;
+    case SSI_SD_SKIP_CRC16:
+        /* we don't verify the crc16 */
+        s->response_pos++;
+        if (s->response_pos == 2) {
+            DPRINTF("CRC16 receive end\n");
+            s->mode = SSI_SD_RESPONSE;
+            s->write_bytes = 0;
+            s->arglen = 1;
+            s->response[0] = DATA_RESPONSE_ACCEPTED;
+            s->response_pos = 0;
+        }
+        return SSI_DUMMY;
     }
     /* Should never happen.  */
     return SSI_DUMMY;
@@ -259,7 +293,7 @@ static int ssi_sd_post_load(void *opaque, int version_id)
 {
     ssi_sd_state *s = (ssi_sd_state *)opaque;
 
-    if (s->mode > SSI_SD_DATA_CRC16) {
+    if (s->mode > SSI_SD_SKIP_CRC16) {
         return -EINVAL;
     }
     if (s->mode == SSI_SD_CMDARG &&
@@ -277,8 +311,8 @@ static int ssi_sd_post_load(void *opaque, int version_id)
 
 static const VMStateDescription vmstate_ssi_sd = {
     .name = "ssi_sd",
-    .version_id = 6,
-    .minimum_version_id = 6,
+    .version_id = 7,
+    .minimum_version_id = 7,
     .post_load = ssi_sd_post_load,
     .fields = (VMStateField []) {
         VMSTATE_UINT32(mode, ssi_sd_state),
@@ -287,6 +321,7 @@ static const VMStateDescription vmstate_ssi_sd = {
         VMSTATE_UINT8_ARRAY(response, ssi_sd_state, 5),
         VMSTATE_UINT16(crc16, ssi_sd_state),
         VMSTATE_INT32(read_bytes, ssi_sd_state),
+        VMSTATE_INT32(write_bytes, ssi_sd_state),
         VMSTATE_INT32(arglen, ssi_sd_state),
         VMSTATE_INT32(response_pos, ssi_sd_state),
         VMSTATE_INT32(stopping, ssi_sd_state),
@@ -340,6 +375,7 @@ static void ssi_sd_reset(DeviceState *dev)
     memset(s->response, 0, sizeof(s->response));
     s->crc16 = 0;
     s->read_bytes = 0;
+    s->write_bytes = 0;
     s->arglen = 0;
     s->response_pos = 0;
     s->stopping = 0;
-- 
2.25.1

[PATCH v4 6/9] hw/sd: ssi-sd: Support multiple block write

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

For a multiple block write operation, each block begins with a multi
write start token. Unlike the SD mode that the multiple block write
ends when receiving a STOP_TRAN command (CMD12), a special stop tran
token is used to signal the card.

Emulating this by manually sending a CMD12 to the SD card core, to
bring it out of the receiving data state.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

---

Changes in v4:
- Keep the card state to SSI_SD_CMD instead of SSI_SD_RESPONSE after
  receiving the STOP_TRAN token per the spec

 hw/sd/ssi-sd.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 1205ad8b52..200e885225 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -4,6 +4,11 @@
  * Copyright (c) 2007-2009 CodeSourcery.
  * Written by Paul Brook
  *
+ * Copyright (c) 2021 Wind River Systems, Inc.
+ * Improved by Bin Meng <bin.meng@windriver.com>
+ *
+ * Validated with U-Boot v2021.01 and Linux v5.10 mmc_spi driver
+ *
  * This code is licensed under the GNU GPL v2.
  *
  * Contributions after 2012-01-13 are licensed under the terms of the
@@ -82,6 +87,10 @@ OBJECT_DECLARE_SIMPLE_TYPE(ssi_sd_state, SSI_SD)
 #define SSI_SDR_ADDRESS_ERROR   0x2000
 #define SSI_SDR_PARAMETER_ERROR 0x4000
 
+/* multiple block write */
+#define SSI_TOKEN_MULTI_WRITE   0xfc
+/* terminate multiple block write */
+#define SSI_TOKEN_STOP_TRAN     0xfd
 /* single block read/write, multiple block read */
 #define SSI_TOKEN_SINGLE        0xfe
 
@@ -94,6 +103,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(ssi_sd_state, SSI_SD)
 static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
 {
     ssi_sd_state *s = SSI_SD(dev);
+    SDRequest request;
+    uint8_t longresp[16];
 
     /*
      * Special case: allow CMD12 (STOP TRANSMISSION) while reading data.
@@ -125,8 +136,28 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
             return SSI_DUMMY;
             break;
         case SSI_TOKEN_SINGLE:
+        case SSI_TOKEN_MULTI_WRITE:
             DPRINTF("Start write block\n");
             s->mode = SSI_SD_DATA_WRITE;
+            return SSI_DUMMY;
+        case SSI_TOKEN_STOP_TRAN:
+            DPRINTF("Stop multiple write\n");
+
+            /* manually issue cmd12 to stop the transfer */
+            request.cmd = 12;
+            request.arg = 0;
+            s->arglen = sdbus_do_command(&s->sdbus, &request, longresp);
+            if (s->arglen <= 0) {
+                s->arglen = 1;
+                /* a zero value indicates the card is busy */
+                s->response[0] = 0;
+                DPRINTF("SD card busy\n");
+            } else {
+                s->arglen = 1;
+                /* a non-zero value indicates the card is ready */
+                s->response[0] = SSI_DUMMY;
+            }
+
             return SSI_DUMMY;
         }
 
@@ -136,8 +167,6 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
         return SSI_DUMMY;
     case SSI_SD_CMDARG:
         if (s->arglen == 4) {
-            SDRequest request;
-            uint8_t longresp[16];
             /* FIXME: Check CRC.  */
             request.cmd = s->cmd;
             request.arg = ldl_be_p(s->cmdarg);
-- 
2.25.1

[PATCH v4 7/9] hw/sd: ssi-sd: Fix SEND_IF_COND (CMD8) response

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

The SEND_IF_COND command (CMD8) response is of format R7, but
current code returns R1 for CMD8. Fix it.

Fixes: 775616c3ae8c ("Partial SD card SPI mode support")
Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

---
When testing with VxWorks driver, this additional issue was exposed.
It looks like VxWorks has stricter parsing on command responses while
U-Boot/Linux drivers are all happy with exising QEMU CMD8 response.

(no changes since v1)

 hw/sd/ssi-sd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 200e885225..84c873b3fd 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -176,9 +176,9 @@ 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 == 58) {
-                /* CMD58 returns R3 response (OCR)  */
-                DPRINTF("Returned OCR\n");
+            } else if (s->cmd == 8 || s->cmd == 58) {
+                /* CMD8/CMD58 returns R3/R7 response */
+                DPRINTF("Returned R3/R7\n");
                 s->arglen = 5;
                 s->response[0] = 1;
                 memcpy(&s->response[1], longresp, 4);
-- 
2.25.1

[PATCH v4 8/9] hw/sd: ssi-sd: Fix STOP_TRANSMISSION (CMD12) response

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

CMD12's response type is R1b, which is basically a R1 plus optional
addition of the busy signal token that can be any number of bytes.
A zero value indicates card is busy and a non-zero value indicates
the card is ready for the next command.

Current implementation sends the busy signal token without sending
the R1 first. This does not break the U-Boot/Linux mmc_spi driver,
but it does not make the VxWorks driver happy.

Move the testing logic of s->stopping in the SSI_SD_RESPONSE state
a bit later, after the first byte of the card reponse is sent out,
to conform with the spec. After the busy signal token is sent, the
state should be transferred to SSI_SD_CMD.

Fixes: 775616c3ae8c ("Partial SD card SPI mode support")
Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

Changes in v4:
- new patch: fix STOP_TRANSMISSION (CMD12) response

 hw/sd/ssi-sd.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 84c873b3fd..907d681d19 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -243,14 +243,15 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
         s->mode = SSI_SD_RESPONSE;
         return SSI_DUMMY;
     case SSI_SD_RESPONSE:
-        if (s->stopping) {
-            s->stopping = 0;
-            return SSI_DUMMY;
-        }
         if (s->response_pos < s->arglen) {
             DPRINTF("Response 0x%02x\n", s->response[s->response_pos]);
             return s->response[s->response_pos++];
         }
+        if (s->stopping) {
+            s->stopping = 0;
+            s->mode = SSI_SD_CMD;
+            return SSI_DUMMY;
+        }
         if (sdbus_data_ready(&s->sdbus)) {
             DPRINTF("Data read\n");
             s->mode = SSI_SD_DATA_START;
-- 
2.25.1

[PATCH v4 9/9] hw/sd: ssi-sd: Handle the rest commands with R1b response type

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

Besides CMD12, the following command's reponse type is R1b:

- SET_WRITE_PROT (CMD28)
- CLR_WRITE_PROT (CMD29)
- ERASE (CMD38)

Reuse the same s->stopping to indicate a R1b reponse is needed.

Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

Changes in v4:
- new patch: handle the rest commands with R1b response type

 hw/sd/ssi-sd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 907d681d19..97ee58e20c 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -194,6 +194,12 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
                 /* CMD13 returns a 2-byte statuse work. Other commands
                    only return the first byte.  */
                 s->arglen = (s->cmd == 13) ? 2 : 1;
+
+                /* handle R1b */
+                if (s->cmd == 28 || s->cmd == 29 || s->cmd == 38) {
+                    s->stopping = 1;
+                }
+
                 cardstatus = ldl_be_p(longresp);
                 status = 0;
                 if (((cardstatus >> 9) & 0xf) < 4)
-- 
2.25.1

Re: [PATCH v4 0/9] hw/sd: Support block read/write in SPI mode

[Bin Meng]

On Thu, Jan 28, 2021 at 2:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> From: Bin Meng <bin.meng@windriver.com>
>
> This includes the previously v3 series [1], and one single patch [2].
>
> Compared to v3, this fixed the following issue in patch [v3,6/6]:
> - Keep the card state to SSI_SD_CMD instead of SSI_SD_RESPONSE after
>   receiving the STOP_TRAN token per the spec
>
> All software tested so far (U-Boot/Linux/VxWorks) do work without
> the fix, but it is better to comform with the spec.
>
> In addition to [2], one more issue was exposed when testing with
> VxWorks driver related to STOP_TRANSMISSION (CMD12) response.
>
> [1] http://patchwork.ozlabs.org/project/qemu-devel/list/?series=226136
> [2] http://patchwork.ozlabs.org/project/qemu-devel/patch/1611636214-52427-1-git-send-email-bmeng.cn@gmail.com/
>
> Changes in v4:
> - Keep the card state to SSI_SD_CMD instead of SSI_SD_RESPONSE after
>   receiving the STOP_TRAN token per the spec
> - new patch: fix STOP_TRANSMISSION (CMD12) response
> - new patch: handle the rest commands with R1b response type
>

Ping?

Re: [PATCH v4 9/9] hw/sd: ssi-sd: Handle the rest commands with R1b response type

[Philippe Mathieu-Daudé]

On 1/28/21 7:30 AM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> Besides CMD12, the following command's reponse type is R1b:
> 
> - SET_WRITE_PROT (CMD28)
> - CLR_WRITE_PROT (CMD29)
> - ERASE (CMD38)
> 
> Reuse the same s->stopping to indicate a R1b reponse is needed.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
> Changes in v4:
> - new patch: handle the rest commands with R1b response type
> 
>  hw/sd/ssi-sd.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index 907d681d19..97ee58e20c 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -194,6 +194,12 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
>                  /* CMD13 returns a 2-byte statuse work. Other commands
>                     only return the first byte.  */
>                  s->arglen = (s->cmd == 13) ? 2 : 1;
> +
> +                /* handle R1b */
> +                if (s->cmd == 28 || s->cmd == 29 || s->cmd == 38) {

Why not also check CMD13 for completeness?

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +                    s->stopping = 1;
> +                }
> +
>                  cardstatus = ldl_be_p(longresp);
>                  status = 0;
>                  if (((cardstatus >> 9) & 0xf) < 4)
> 

Re: [PATCH v4 9/9] hw/sd: ssi-sd: Handle the rest commands with R1b response type

[Bin Meng]

Hi Philippe,

On Mon, Feb 8, 2021 at 10:08 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 1/28/21 7:30 AM, Bin Meng wrote:
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > Besides CMD12, the following command's reponse type is R1b:
> >
> > - SET_WRITE_PROT (CMD28)
> > - CLR_WRITE_PROT (CMD29)
> > - ERASE (CMD38)
> >
> > Reuse the same s->stopping to indicate a R1b reponse is needed.
> >
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> >
> > ---
> >
> > Changes in v4:
> > - new patch: handle the rest commands with R1b response type
> >
> >  hw/sd/ssi-sd.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> > index 907d681d19..97ee58e20c 100644
> > --- a/hw/sd/ssi-sd.c
> > +++ b/hw/sd/ssi-sd.c
> > @@ -194,6 +194,12 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
> >                  /* CMD13 returns a 2-byte statuse work. Other commands
> >                     only return the first byte.  */
> >                  s->arglen = (s->cmd == 13) ? 2 : 1;
> > +
> > +                /* handle R1b */
> > +                if (s->cmd == 28 || s->cmd == 29 || s->cmd == 38) {
>
> Why not also check CMD13 for completeness?
>

I am not sure I got your point. CMD13 does not respond with R1b but R2.

> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> > +                    s->stopping = 1;
> > +                }
> > +
> >                  cardstatus = ldl_be_p(longresp);
> >                  status = 0;
> >                  if (((cardstatus >> 9) & 0xf) < 4)
> >

Regards,
Bin

Re: [PATCH v4 9/9] hw/sd: ssi-sd: Handle the rest commands with R1b response type

[Philippe Mathieu-Daudé]

On Mon, Feb 8, 2021 at 3:20 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Philippe,
>
> On Mon, Feb 8, 2021 at 10:08 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > On 1/28/21 7:30 AM, Bin Meng wrote:
> > > From: Bin Meng <bin.meng@windriver.com>
> > >
> > > Besides CMD12, the following command's reponse type is R1b:
> > >
> > > - SET_WRITE_PROT (CMD28)
> > > - CLR_WRITE_PROT (CMD29)
> > > - ERASE (CMD38)
> > >
> > > Reuse the same s->stopping to indicate a R1b reponse is needed.
> > >
> > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > >
> > > ---
> > >
> > > Changes in v4:
> > > - new patch: handle the rest commands with R1b response type
> > >
> > >  hw/sd/ssi-sd.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> > > index 907d681d19..97ee58e20c 100644
> > > --- a/hw/sd/ssi-sd.c
> > > +++ b/hw/sd/ssi-sd.c
> > > @@ -194,6 +194,12 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
> > >                  /* CMD13 returns a 2-byte statuse work. Other commands
> > >                     only return the first byte.  */
> > >                  s->arglen = (s->cmd == 13) ? 2 : 1;
> > > +
> > > +                /* handle R1b */
> > > +                if (s->cmd == 28 || s->cmd == 29 || s->cmd == 38) {
> >
> > Why not also check CMD13 for completeness?
> >
>
> I am not sure I got your point. CMD13 does not respond with R1b but R2.

Forget what I wrote, you are correct =)

BTW since you have a deep understanding of SD cards, would you like to
be listed as designated reviewer in the SD/MMC section?

> > Otherwise:
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >
> > > +                    s->stopping = 1;
> > > +                }
> > > +
> > >                  cardstatus = ldl_be_p(longresp);
> > >                  status = 0;
> > >                  if (((cardstatus >> 9) & 0xf) < 4)
> > >
>
> Regards,
> Bin

Re: [PATCH v4 9/9] hw/sd: ssi-sd: Handle the rest commands with R1b response type

[Bin Meng]

HI Philippe,

On Mon, Feb 8, 2021 at 10:27 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On Mon, Feb 8, 2021 at 3:20 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Philippe,
> >
> > On Mon, Feb 8, 2021 at 10:08 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > >
> > > On 1/28/21 7:30 AM, Bin Meng wrote:
> > > > From: Bin Meng <bin.meng@windriver.com>
> > > >
> > > > Besides CMD12, the following command's reponse type is R1b:
> > > >
> > > > - SET_WRITE_PROT (CMD28)
> > > > - CLR_WRITE_PROT (CMD29)
> > > > - ERASE (CMD38)
> > > >
> > > > Reuse the same s->stopping to indicate a R1b reponse is needed.
> > > >
> > > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > >
> > > > ---
> > > >
> > > > Changes in v4:
> > > > - new patch: handle the rest commands with R1b response type
> > > >
> > > >  hw/sd/ssi-sd.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> > > > index 907d681d19..97ee58e20c 100644
> > > > --- a/hw/sd/ssi-sd.c
> > > > +++ b/hw/sd/ssi-sd.c
> > > > @@ -194,6 +194,12 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
> > > >                  /* CMD13 returns a 2-byte statuse work. Other commands
> > > >                     only return the first byte.  */
> > > >                  s->arglen = (s->cmd == 13) ? 2 : 1;
> > > > +
> > > > +                /* handle R1b */
> > > > +                if (s->cmd == 28 || s->cmd == 29 || s->cmd == 38) {
> > >
> > > Why not also check CMD13 for completeness?
> > >
> >
> > I am not sure I got your point. CMD13 does not respond with R1b but R2.
>
> Forget what I wrote, you are correct =)
>
> BTW since you have a deep understanding of SD cards, would you like to
> be listed as designated reviewer in the SD/MMC section?

I would be honored to have a try. Thanks!

Regards,
Bin

Re: [PATCH v4 0/9] hw/sd: Support block read/write in SPI mode

[Bin Meng]

Hi Philippe,

On Thu, Feb 4, 2021 at 2:02 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Thu, Jan 28, 2021 at 2:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > This includes the previously v3 series [1], and one single patch [2].
> >
> > Compared to v3, this fixed the following issue in patch [v3,6/6]:
> > - Keep the card state to SSI_SD_CMD instead of SSI_SD_RESPONSE after
> >   receiving the STOP_TRAN token per the spec
> >
> > All software tested so far (U-Boot/Linux/VxWorks) do work without
> > the fix, but it is better to comform with the spec.
> >
> > In addition to [2], one more issue was exposed when testing with
> > VxWorks driver related to STOP_TRANSMISSION (CMD12) response.
> >
> > [1] http://patchwork.ozlabs.org/project/qemu-devel/list/?series=226136
> > [2] http://patchwork.ozlabs.org/project/qemu-devel/patch/1611636214-52427-1-git-send-email-bmeng.cn@gmail.com/
> >
> > Changes in v4:
> > - Keep the card state to SSI_SD_CMD instead of SSI_SD_RESPONSE after
> >   receiving the STOP_TRAN token per the spec
> > - new patch: fix STOP_TRANSMISSION (CMD12) response
> > - new patch: handle the rest commands with R1b response type
> >
>
> Ping?

Will a PR be sent soon to include this series so that the SiFive SPI
series can follow?

Regards,
Bin

Re: [PATCH v4 0/9] hw/sd: Support block read/write in SPI mode

[Philippe Mathieu-Daudé]

On 2/9/21 3:32 PM, Bin Meng wrote:
> Hi Philippe,
> 
> On Thu, Feb 4, 2021 at 2:02 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>> On Thu, Jan 28, 2021 at 2:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>>
>>> From: Bin Meng <bin.meng@windriver.com>
>>>
>>> This includes the previously v3 series [1], and one single patch [2].
>>>
>>> Compared to v3, this fixed the following issue in patch [v3,6/6]:
>>> - Keep the card state to SSI_SD_CMD instead of SSI_SD_RESPONSE after
>>>   receiving the STOP_TRAN token per the spec
>>>
>>> All software tested so far (U-Boot/Linux/VxWorks) do work without
>>> the fix, but it is better to comform with the spec.
>>>
>>> In addition to [2], one more issue was exposed when testing with
>>> VxWorks driver related to STOP_TRANSMISSION (CMD12) response.
>>>
>>> [1] http://patchwork.ozlabs.org/project/qemu-devel/list/?series=226136
>>> [2] http://patchwork.ozlabs.org/project/qemu-devel/patch/1611636214-52427-1-git-send-email-bmeng.cn@gmail.com/
>>>
>>> Changes in v4:
>>> - Keep the card state to SSI_SD_CMD instead of SSI_SD_RESPONSE after
>>>   receiving the STOP_TRAN token per the spec
>>> - new patch: fix STOP_TRANSMISSION (CMD12) response
>>> - new patch: handle the rest commands with R1b response type
>>>
>>
>> Ping?
> 
> Will a PR be sent soon to include this series so that the SiFive SPI
> series can follow?

I had it planned for yesterday but had problems with the mails from
the list, + the CVE (you fixed) took priority.

Missing review is patch #8 "Fix STOP_TRANSMISSION (CMD12) response"
for which I don't have test yet.

Re: [PATCH v4 0/9] hw/sd: Support block read/write in SPI mode

[Bin Meng]

On Wed, Feb 10, 2021 at 1:36 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 2/9/21 3:32 PM, Bin Meng wrote:
> > Hi Philippe,
> >
> > On Thu, Feb 4, 2021 at 2:02 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>
> >> On Thu, Jan 28, 2021 at 2:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>>
> >>> From: Bin Meng <bin.meng@windriver.com>
> >>>
> >>> This includes the previously v3 series [1], and one single patch [2].
> >>>
> >>> Compared to v3, this fixed the following issue in patch [v3,6/6]:
> >>> - Keep the card state to SSI_SD_CMD instead of SSI_SD_RESPONSE after
> >>>   receiving the STOP_TRAN token per the spec
> >>>
> >>> All software tested so far (U-Boot/Linux/VxWorks) do work without
> >>> the fix, but it is better to comform with the spec.
> >>>
> >>> In addition to [2], one more issue was exposed when testing with
> >>> VxWorks driver related to STOP_TRANSMISSION (CMD12) response.
> >>>
> >>> [1] http://patchwork.ozlabs.org/project/qemu-devel/list/?series=226136
> >>> [2] http://patchwork.ozlabs.org/project/qemu-devel/patch/1611636214-52427-1-git-send-email-bmeng.cn@gmail.com/
> >>>
> >>> Changes in v4:
> >>> - Keep the card state to SSI_SD_CMD instead of SSI_SD_RESPONSE after
> >>>   receiving the STOP_TRAN token per the spec
> >>> - new patch: fix STOP_TRANSMISSION (CMD12) response
> >>> - new patch: handle the rest commands with R1b response type
> >>>
> >>
> >> Ping?
> >
> > Will a PR be sent soon to include this series so that the SiFive SPI
> > series can follow?
>
> I had it planned for yesterday but had problems with the mails from
> the list, + the CVE (you fixed) took priority.
>
> Missing review is patch #8 "Fix STOP_TRANSMISSION (CMD12) response"
> for which I don't have test yet.

Ping? If there is any comment for patch #8?

Regards,
Bin

Re: [PATCH v4 0/9] hw/sd: Support block read/write in SPI mode

[Philippe Mathieu-Daudé]

On 2/16/21 12:53 PM, Bin Meng wrote:
> On Wed, Feb 10, 2021 at 1:36 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> On 2/9/21 3:32 PM, Bin Meng wrote:
>>> Hi Philippe,
>>>
>>> On Thu, Feb 4, 2021 at 2:02 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>
>>>> On Thu, Jan 28, 2021 at 2:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>>
>>>>> From: Bin Meng <bin.meng@windriver.com>
>>>>>
>>>>> This includes the previously v3 series [1], and one single patch [2].
>>>>>
>>>>> Compared to v3, this fixed the following issue in patch [v3,6/6]:
>>>>> - Keep the card state to SSI_SD_CMD instead of SSI_SD_RESPONSE after
>>>>>   receiving the STOP_TRAN token per the spec
>>>>>
>>>>> All software tested so far (U-Boot/Linux/VxWorks) do work without
>>>>> the fix, but it is better to comform with the spec.
>>>>>
>>>>> In addition to [2], one more issue was exposed when testing with
>>>>> VxWorks driver related to STOP_TRANSMISSION (CMD12) response.
>>>>>
>>>>> [1] http://patchwork.ozlabs.org/project/qemu-devel/list/?series=226136
>>>>> [2] http://patchwork.ozlabs.org/project/qemu-devel/patch/1611636214-52427-1-git-send-email-bmeng.cn@gmail.com/
>>>>>
>>>>> Changes in v4:
>>>>> - Keep the card state to SSI_SD_CMD instead of SSI_SD_RESPONSE after
>>>>>   receiving the STOP_TRAN token per the spec
>>>>> - new patch: fix STOP_TRANSMISSION (CMD12) response
>>>>> - new patch: handle the rest commands with R1b response type
>>>>>
>>>>
>>>> Ping?
>>>
>>> Will a PR be sent soon to include this series so that the SiFive SPI
>>> series can follow?
>>
>> I had it planned for yesterday but had problems with the mails from
>> the list, + the CVE (you fixed) took priority.
>>
>> Missing review is patch #8 "Fix STOP_TRANSMISSION (CMD12) response"
>> for which I don't have test yet.
> 
> Ping? If there is any comment for patch #8?

No more comment, series applied to sdmmc-next tree.

Thanks,

Phil.