[Qemu-devel] [PATCH v4 00/20] SDCard: bugfixes, support UHS-I (part 5)

[Qemu-devel] [PATCH v4 00/20] SDCard: bugfixes, support UHS-I (part 5)

[Philippe Mathieu-Daudé]

Some refactors, few bugfixes, better SD/SPI support.

With this series apply, machines can use SD cards in UHS-I mode.
(mostly imported from Alistair Francis work)

MMC mode split out for another series,
so UHS enabled MMC cards are still not usable:

  kernel: mmc0: SDHCI controller on PCI [0000:00:05.0] using ADMA
  kernel: sd 0:0:0:0: Attached scsi generic sg0 type 0
  kernel: mmc0: Skipping voltage switch
  [mmc kthread looping]

Since v3:
- simpler SPI handling, improved descriptions (Alistair review)
- inverted patches 16/17 order

Since v2:
- split again in 2... other part is cleanup/tracing

Since v1:
- rewrote mostly all patches to keep it simpler.

$ git backport-diff
001/20:[----] [-C] 'sdcard: Don't always set the high capacity bit'
002/20:[----] [-C] 'sdcard: update the CSD CRC register regardless the CSD structure version'
003/20:[----] [-C] 'sdcard: fix the 'maximum data transfer rate' to 25MHz'
004/20:[----] [-C] 'sdcard: clean the SCR register and add few comments'
005/20:[----] [--] 'sdcard: remove commands from unsupported old MMC specification'
006/20:[----] [--] 'sdcard: simplify using the ldst API'
007/20:[0008] [FC] 'sdcard: use the correct masked OCR in the R3 reply'
008/20:[----] [-C] 'sdcard: use the registerfields API for the CARD_STATUS register masks'
009/20:[----] [--] 'sdcard: handle CMD54 (SDIO)'
010/20:[down] 'sdcard: handle the Security Specification commands'
011/20:[down] 'sdcard: use a more descriptive label 'unimplemented_spi_cmd''
012/20:[0034] [FC] 'sdcard: handles more commands in SPI mode'
013/20:[----] [--] 'sdcard: check the card is in correct state for APP CMD (CMD55)'
014/20:[----] [--] 'sdcard: warn if host uses an incorrect address for APP CMD (CMD55)'
015/20:[----] [--] 'sdcard: simplify SEND_IF_COND (CMD8)'
016/20:[----] [--] 'sdcard: simplify SD_SEND_OP_COND (ACMD41)'
017/20:[----] [--] 'sdcard: add SD SEND_TUNING_BLOCK (CMD19)'
018/20:[----] [--] 'sdcard: implement the UHS-I SWITCH_FUNCTION entries (Spec v3)'
019/20:[----] [-C] 'sdcard: add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit'
020/20:[----] [--] 'sdcard: add an enum for the SD PHY Spec version'

Based-on: 20180215220540.6556-12-f4bug@amsat.org

Philippe Mathieu-Daudé (20):
  sdcard: Don't always set the high capacity bit
  sdcard: update the CSD CRC register regardless the CSD structure
    version
  sdcard: fix the 'maximum data transfer rate' to 25MHz
  sdcard: clean the SCR register and add few comments
  sdcard: remove commands from unsupported old MMC specification
  sdcard: simplify using the ldst API
  sdcard: use the correct masked OCR in the R3 reply
  sdcard: use the registerfields API for the CARD_STATUS register masks
  sdcard: handle CMD54 (SDIO)
  sdcard: handle the Security Specification commands
  sdcard: use a more descriptive label 'unimplemented_spi_cmd'
  sdcard: handles more commands in SPI mode
  sdcard: check the card is in correct state for APP CMD (CMD55)
  sdcard: warn if host uses an incorrect address for APP CMD (CMD55)
  sdcard: simplify SEND_IF_COND (CMD8)
  sdcard: simplify SD_SEND_OP_COND (ACMD41)
  sdcard: add SD SEND_TUNING_BLOCK (CMD19)
  sdcard: implement the UHS-I SWITCH_FUNCTION entries (Spec v3)
  sdcard: add a 'uhs' property, update the OCR register
    ACCEPT_SWITCH_1V8 bit
  sdcard: add an enum for the SD PHY Spec version

 hw/sd/sd.c         | 498 ++++++++++++++++++++++++++++++++++++-----------------
 hw/sd/trace-events |   1 +
 2 files changed, 343 insertions(+), 156 deletions(-)

-- 
2.16.1

[Qemu-devel] [PATCH v4 01/20] sdcard: Don't always set the high capacity bit

[Philippe Mathieu-Daudé]

Don't set the high capacity bit by default as it will be set if required
in the sd_set_csd() function.

[based on a patch from Alistair Francis <alistair.francis@xilinx.com>
 and Peter Ogden <ogden@xilinx.com> from qemu/xilinx tag xilinx-v2015.4]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index fbee87afef..9dfbd65ac8 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -291,6 +291,10 @@ static void sd_ocr_powerup(void *opaque)
 
     /* card power-up OK */
     sd->ocr = FIELD_DP32(sd->ocr, OCR, CARD_POWER_UP, 1);
+
+    if (sd->size > 1 * G_BYTE) {
+        sd->ocr = FIELD_DP32(sd->ocr, OCR, CARD_CAPACITY, 1);
+    }
 }
 
 static void sd_set_scr(SDState *sd)
@@ -395,7 +399,6 @@ static void sd_set_csd(SDState *sd, uint64_t size)
         sd->csd[13] = 0x40;
         sd->csd[14] = 0x00;
         sd->csd[15] = 0x00;
-        sd->ocr |= 1 << 30;     /* High Capacity SD Memory Card */
     }
 }
 
-- 
2.16.1

[Qemu-devel] [PATCH v4 02/20] sdcard: update the CSD CRC register regardless the CSD structure version

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 9dfbd65ac8..c1ba098d86 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -379,7 +379,6 @@ static void sd_set_csd(SDState *sd, uint64_t size)
         sd->csd[13] = 0x20 |	/* Max. write data block length */
             ((HWBLOCK_SHIFT << 6) & 0xc0);
         sd->csd[14] = 0x00;	/* File format group */
-        sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
     } else {			/* SDHC */
         size /= 512 * 1024;
         size -= 1;
@@ -398,8 +397,8 @@ static void sd_set_csd(SDState *sd, uint64_t size)
         sd->csd[12] = 0x0a;
         sd->csd[13] = 0x40;
         sd->csd[14] = 0x00;
-        sd->csd[15] = 0x00;
     }
+    sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
 }
 
 static void sd_set_rca(SDState *sd)
-- 
2.16.1

[Qemu-devel] [PATCH v4 03/20] sdcard: fix the 'maximum data transfer rate' to 25MHz

[Philippe Mathieu-Daudé]

To comply with Spec v1.10 (and 2.00, 3.01):

. TRAN_SPEED

for current SD Memory Cards that field must be always 0_0110_010b (032h) which is
equal to 25MHz - the mandatory maximum operating frequency of SD Memory Card.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index c1ba098d86..14c1cb1332 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -358,7 +358,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
         sd->csd[0] = 0x00;	/* CSD structure */
         sd->csd[1] = 0x26;	/* Data read access-time-1 */
         sd->csd[2] = 0x00;	/* Data read access-time-2 */
-        sd->csd[3] = 0x5a;	/* Max. data transfer rate */
+        sd->csd[3] = 0x32;  /* Max. data transfer rate: 25 MHz */
         sd->csd[4] = 0x5f;	/* Card Command Classes */
         sd->csd[5] = 0x50 |	/* Max. read data block length */
             HWBLOCK_SHIFT;
-- 
2.16.1

[Qemu-devel] [PATCH v4 04/20] sdcard: clean the SCR register and add few comments

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sd.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 14c1cb1332..41fac9a4aa 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -299,10 +299,13 @@ static void sd_ocr_powerup(void *opaque)
 
 static void sd_set_scr(SDState *sd)
 {
-    sd->scr[0] = 0x00;		/* SCR Structure */
-    sd->scr[1] = 0x2f;		/* SD Security Support */
-    sd->scr[2] = 0x00;
+    sd->scr[0] = (0 << 4)       /* SCR version 1.0 */
+                 | 0;           /* Spec Versions 1.0 and 1.01 */
+    sd->scr[1] = (2 << 4)       /* SDSC Card (Security Version 1.01) */
+                 | 0b0101;      /* 1-bit or 4-bit width bus modes */
+    sd->scr[2] = 0x00;          /* Extended Security is not supported. */
     sd->scr[3] = 0x00;
+    /* reserved for manufacturer usage */
     sd->scr[4] = 0x00;
     sd->scr[5] = 0x00;
     sd->scr[6] = 0x00;
-- 
2.16.1

[Qemu-devel] [PATCH v4 05/20] sdcard: remove commands from unsupported old MMC specification

[Philippe Mathieu-Daudé]

This device does not model MMCA Specification previous to v4.2

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sd.c | 33 ---------------------------------
 1 file changed, 33 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 41fac9a4aa..4267919ce0 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1041,24 +1041,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         }
         break;
 
-    case 11:	/* CMD11:  READ_DAT_UNTIL_STOP */
-        if (sd->spi)
-            goto bad_cmd;
-        switch (sd->state) {
-        case sd_transfer_state:
-            sd->state = sd_sendingdata_state;
-            sd->data_start = req.arg;
-            sd->data_offset = 0;
-
-            if (sd->data_start + sd->blk_len > sd->size)
-                sd->card_status |= ADDRESS_ERROR;
-            return sd_r0;
-
-        default:
-            break;
-        }
-        break;
-
     case 12:	/* CMD12:  STOP_TRANSMISSION */
         switch (sd->state) {
         case sd_sendingdata_state:
@@ -1871,21 +1853,6 @@ uint8_t sd_read_data(SDState *sd)
             sd->state = sd_transfer_state;
         break;
 
-    case 11:	/* CMD11:  READ_DAT_UNTIL_STOP */
-        if (sd->data_offset == 0)
-            BLK_READ_BLOCK(sd->data_start, io_len);
-        ret = sd->data[sd->data_offset ++];
-
-        if (sd->data_offset >= io_len) {
-            sd->data_start += io_len;
-            sd->data_offset = 0;
-            if (sd->data_start + io_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
-                break;
-            }
-        }
-        break;
-
     case 13:	/* ACMD13: SD_STATUS */
         ret = sd->sd_status[sd->data_offset ++];
 
-- 
2.16.1

[Qemu-devel] [PATCH v4 06/20] sdcard: simplify using the ldst API

[Philippe Mathieu-Daudé]

the code is easier to review/refactor.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sd.c | 38 +++++++++-----------------------------
 1 file changed, 9 insertions(+), 29 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 4267919ce0..b43baa2edd 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -432,57 +432,39 @@ static int sd_req_crc_validate(SDRequest *req)
 {
     uint8_t buffer[5];
     buffer[0] = 0x40 | req->cmd;
-    buffer[1] = (req->arg >> 24) & 0xff;
-    buffer[2] = (req->arg >> 16) & 0xff;
-    buffer[3] = (req->arg >> 8) & 0xff;
-    buffer[4] = (req->arg >> 0) & 0xff;
+    stl_be_p(&buffer[1], req->arg);
     return 0;
     return sd_crc7(buffer, 5) != req->crc;	/* TODO */
 }
 
 static void sd_response_r1_make(SDState *sd, uint8_t *response)
 {
-    uint32_t status = sd->card_status;
+    stl_be_p(response, sd->card_status);
+
     /* Clear the "clear on read" status bits */
     sd->card_status &= ~CARD_STATUS_C;
-
-    response[0] = (status >> 24) & 0xff;
-    response[1] = (status >> 16) & 0xff;
-    response[2] = (status >> 8) & 0xff;
-    response[3] = (status >> 0) & 0xff;
 }
 
 static void 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;
+    stl_be_p(response, sd->ocr);
 }
 
 static void sd_response_r6_make(SDState *sd, uint8_t *response)
 {
-    uint16_t arg;
     uint16_t status;
 
-    arg = sd->rca;
     status = ((sd->card_status >> 8) & 0xc000) |
              ((sd->card_status >> 6) & 0x2000) |
               (sd->card_status & 0x1fff);
     sd->card_status &= ~(CARD_STATUS_C & 0xc81fff);
-
-    response[0] = (arg >> 8) & 0xff;
-    response[1] = arg & 0xff;
-    response[2] = (status >> 8) & 0xff;
-    response[3] = status & 0xff;
+    stw_be_p(response + 0, sd->rca);
+    stw_be_p(response + 2, status);
 }
 
 static void 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;
+    stl_be_p(response, sd->vhs);
 }
 
 static inline uint64_t sd_addr_to_wpnum(uint64_t addr)
@@ -728,7 +710,7 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
 
 static void sd_function_switch(SDState *sd, uint32_t arg)
 {
-    int i, mode, new_func, crc;
+    int i, mode, new_func;
     mode = !!(arg & 0x80000000);
 
     sd->data[0] = 0x00;		/* Maximum current consumption */
@@ -752,9 +734,7 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
         sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
     }
     memset(&sd->data[17], 0, 47);
-    crc = sd_crc16(sd->data, 64);
-    sd->data[65] = crc >> 8;
-    sd->data[66] = crc & 0xff;
+    stw_be_p(sd->data + 65, sd_crc16(sd->data, 64));
 }
 
 static inline bool sd_wp_addr(SDState *sd, uint64_t addr)
-- 
2.16.1

[Qemu-devel] [PATCH v4 07/20] sdcard: use the correct masked OCR in the R3 reply

[Philippe Mathieu-Daudé]

use the registerfields API to access the OCR register

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sd.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b43baa2edd..3970e590e6 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -47,8 +47,6 @@
 
 //#define DEBUG_SD 1
 
-#define ACMD41_ENQUIRY_MASK     0x00ffffff
-
 typedef enum {
     sd_r0 = 0,    /* no response */
     sd_r1,        /* normal response command */
@@ -273,13 +271,26 @@ static uint16_t sd_crc16(void *message, size_t width)
 
 #define OCR_POWER_DELAY_NS      500000 /* 0.5ms */
 
+FIELD(OCR, VDD_VOLTAGE_WINDOW,          0, 24)
+FIELD(OCR, VDD_VOLTAGE_WIN_LO,          0,  8)
+FIELD(OCR, DUAL_VOLTAGE_CARD,           7,  1)
+FIELD(OCR, VDD_VOLTAGE_WIN_HI,          8, 16)
+FIELD(OCR, ACCEPT_SWITCH_1V8,          24,  1) /* Only UHS-I */
+FIELD(OCR, UHS_II_CARD,                29,  1) /* Only UHS-II */
 FIELD(OCR, CARD_CAPACITY,              30,  1) /* 0:SDSC, 1:SDHC/SDXC */
 FIELD(OCR, CARD_POWER_UP,              31,  1)
 
+#define ACMD41_ENQUIRY_MASK     0x00ffffff
+#define ACMD41_R3_MASK          (R_OCR_VDD_VOLTAGE_WIN_HI_MASK \
+                               | R_OCR_ACCEPT_SWITCH_1V8_MASK \
+                               | R_OCR_UHS_II_CARD_MASK \
+                               | R_OCR_CARD_CAPACITY_MASK \
+                               | R_OCR_CARD_POWER_UP_MASK)
+
 static void sd_set_ocr(SDState *sd)
 {
-    /* All voltages OK, Standard Capacity SD Memory Card, not yet powered up */
-    sd->ocr = 0x00ffff00;
+    /* All voltages OK */
+    sd->ocr = R_OCR_VDD_VOLTAGE_WIN_HI_MASK;
 }
 
 static void sd_ocr_powerup(void *opaque)
@@ -447,7 +458,7 @@ static void sd_response_r1_make(SDState *sd, uint8_t *response)
 
 static void sd_response_r3_make(SDState *sd, uint8_t *response)
 {
-    stl_be_p(response, sd->ocr);
+    stl_be_p(response, sd->ocr & ACMD41_R3_MASK);
 }
 
 static void sd_response_r6_make(SDState *sd, uint8_t *response)
-- 
2.16.1

[Qemu-devel] [PATCH v4 08/20] sdcard: use the registerfields API for the CARD_STATUS register masks

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sd.c | 48 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 3970e590e6..b567c44da8 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -420,14 +420,56 @@ static void sd_set_rca(SDState *sd)
     sd->rca += 0x4567;
 }
 
+FIELD(CSR, AKE_SEQ_ERROR,               3,  1)
+FIELD(CSR, APP_CMD,                     5,  1)
+FIELD(CSR, FX_EVENT,                    6,  1)
+FIELD(CSR, READY_FOR_DATA,              8,  1)
+FIELD(CSR, CURRENT_STATE,               9,  4)
+FIELD(CSR, ERASE_RESET,                13,  1)
+FIELD(CSR, CARD_ECC_DISABLED,          14,  1)
+FIELD(CSR, WP_ERASE_SKIP,              15,  1)
+FIELD(CSR, CSD_OVERWRITE,              16,  1)
+FIELD(CSR, DEFERRED_RESPONSE,          17,  1)
+FIELD(CSR, ERROR,                      19,  1)
+FIELD(CSR, CC_ERROR,                   20,  1)
+FIELD(CSR, CARD_ECC_FAILED,            21,  1)
+FIELD(CSR, ILLEGAL_COMMAND,            22,  1)
+FIELD(CSR, COM_CRC_ERROR,              23,  1)
+FIELD(CSR, LOCK_UNLOCK_FAILED,         24,  1)
+FIELD(CSR, CARD_IS_LOCKED,             25,  1)
+FIELD(CSR, WP_VIOLATION,               26,  1)
+FIELD(CSR, ERASE_PARAM,                27,  1)
+FIELD(CSR, ERASE_SEQ_ERROR,            28,  1)
+FIELD(CSR, BLOCK_LEN_ERROR,            29,  1)
+FIELD(CSR, ADDRESS_ERROR,              30,  1)
+FIELD(CSR, OUT_OF_RANGE,               31,  1)
+
 /* Card status bits, split by clear condition:
  * A : According to the card current state
  * B : Always related to the previous command
  * C : Cleared by read
  */
-#define CARD_STATUS_A	0x02004100
-#define CARD_STATUS_B	0x00c01e00
-#define CARD_STATUS_C	0xfd39a028
+#define CARD_STATUS_A           (R_CSR_READY_FOR_DATA_MASK \
+                               | R_CSR_CARD_ECC_DISABLED_MASK \
+                               | R_CSR_CARD_IS_LOCKED_MASK)
+#define CARD_STATUS_B           (R_CSR_CURRENT_STATE_MASK \
+                               | R_CSR_ILLEGAL_COMMAND_MASK \
+                               | R_CSR_COM_CRC_ERROR_MASK)
+#define CARD_STATUS_C           (R_CSR_AKE_SEQ_ERROR_MASK \
+                               | R_CSR_APP_CMD_MASK \
+                               | R_CSR_ERASE_RESET_MASK \
+                               | R_CSR_WP_ERASE_SKIP_MASK \
+                               | R_CSR_CSD_OVERWRITE_MASK \
+                               | R_CSR_ERROR_MASK \
+                               | R_CSR_CC_ERROR_MASK \
+                               | R_CSR_CARD_ECC_FAILED_MASK \
+                               | R_CSR_LOCK_UNLOCK_FAILED_MASK \
+                               | R_CSR_WP_VIOLATION_MASK \
+                               | R_CSR_ERASE_PARAM_MASK \
+                               | R_CSR_ERASE_SEQ_ERROR_MASK \
+                               | R_CSR_BLOCK_LEN_ERROR_MASK \
+                               | R_CSR_ADDRESS_ERROR_MASK \
+                               | R_CSR_OUT_OF_RANGE_MASK)
 
 static void sd_set_cardstatus(SDState *sd)
 {
-- 
2.16.1

[Qemu-devel] [PATCH v4 09/20] sdcard: handle CMD54 (SDIO)

[Philippe Mathieu-Daudé]

Linux uses it to poll the bus before polling for a card.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sd.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b567c44da8..30acd04ad7 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1375,9 +1375,8 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         }
         break;
 
-    case 52:
-    case 53:
-        /* CMD52, CMD53: reserved for SDIO cards
+    case 52 ... 54:
+        /* CMD52, CMD53, CMD54: reserved for SDIO cards
          * (see the SDIO Simplified Specification V2.0)
          * Handle as illegal command but do not complain
          * on stderr, as some OSes may use these in their
-- 
2.16.1

[Qemu-devel] [PATCH v4 10/20] sdcard: handle the Security Specification commands

[Philippe Mathieu-Daudé]

returning sd_illegal, since they are not implemented.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 30acd04ad7..0457f5214b 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1551,6 +1551,17 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
         }
         break;
 
+    case 18:    /* Reserved for SD security applications */
+    case 25:
+    case 26:
+    case 38:
+    case 43 ... 49:
+        /* Refer to the "SD Specifications Part3 Security Specification" for
+         * information about the SD Security Features */
+        qemu_log_mask(LOG_UNIMP, "SD: CMD%i Security not implemented\n",
+                      req.cmd);
+        return sd_illegal;
+
     default:
         /* Fall back to standard commands.  */
         return sd_normal_command(sd, req);
-- 
2.16.1

Re: [Qemu-devel] [PATCH v4 10/20] sdcard: handle the Security Specification commands

[Alistair Francis]

On Thu, Feb 15, 2018 at 2:13 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> returning sd_illegal, since they are not implemented.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 30acd04ad7..0457f5214b 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1551,6 +1551,17 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>          }
>          break;
>
> +    case 18:    /* Reserved for SD security applications */
> +    case 25:
> +    case 26:
> +    case 38:
> +    case 43 ... 49:
> +        /* Refer to the "SD Specifications Part3 Security Specification" for
> +         * information about the SD Security Features */

The */ should be on a new line.

Otherwise:

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Alistair

> +        qemu_log_mask(LOG_UNIMP, "SD: CMD%i Security not implemented\n",
> +                      req.cmd);
> +        return sd_illegal;
> +
>      default:
>          /* Fall back to standard commands.  */
>          return sd_normal_command(sd, req);
> --
> 2.16.1
>
>

Re: [Qemu-devel] [PATCH v4 10/20] sdcard: handle the Security Specification commands

[Philippe Mathieu-Daudé]

On 02/15/2018 07:54 PM, Alistair Francis wrote:
> On Thu, Feb 15, 2018 at 2:13 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> returning sd_illegal, since they are not implemented.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/sd.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 30acd04ad7..0457f5214b 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -1551,6 +1551,17 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>>          }
>>          break;
>>
>> +    case 18:    /* Reserved for SD security applications */
>> +    case 25:
>> +    case 26:
>> +    case 38:
>> +    case 43 ... 49:
>> +        /* Refer to the "SD Specifications Part3 Security Specification" for
>> +         * information about the SD Security Features */
> 
> The */ should be on a new line.

Thanks Peter for fixing this :)

> 
> Otherwise:
> 
> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks!

> 
> Alistair
> 
>> +        qemu_log_mask(LOG_UNIMP, "SD: CMD%i Security not implemented\n",
>> +                      req.cmd);
>> +        return sd_illegal;
>> +
>>      default:
>>          /* Fall back to standard commands.  */
>>          return sd_normal_command(sd, req);
>> --
>> 2.16.1
>>
>>

[Qemu-devel] [PATCH v4 11/20] sdcard: use a more descriptive label 'unimplemented_spi_cmd'

[Philippe Mathieu-Daudé]

Suggested-by: Alistair Francis <alistair.francis@xilinx.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 0457f5214b..4f82638735 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1183,8 +1183,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
     /* Block write commands (Class 4) */
     case 24:	/* CMD24:  WRITE_SINGLE_BLOCK */
-        if (sd->spi)
-            goto unimplemented_cmd;
+        if (sd->spi) {
+            goto unimplemented_spi_cmd;
+        }
         switch (sd->state) {
         case sd_transfer_state:
             /* Writing in SPI mode not implemented.  */
@@ -1209,8 +1210,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         break;
 
     case 25:	/* CMD25:  WRITE_MULTIPLE_BLOCK */
-        if (sd->spi)
-            goto unimplemented_cmd;
+        if (sd->spi) {
+            goto unimplemented_spi_cmd;
+        }
         switch (sd->state) {
         case sd_transfer_state:
             /* Writing in SPI mode not implemented.  */
@@ -1250,8 +1252,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         break;
 
     case 27:	/* CMD27:  PROGRAM_CSD */
-        if (sd->spi)
-            goto unimplemented_cmd;
+        if (sd->spi) {
+            goto unimplemented_spi_cmd;
+        }
         switch (sd->state) {
         case sd_transfer_state:
             sd->state = sd_receivingdata_state;
@@ -1361,8 +1364,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
     /* Lock card commands (Class 7) */
     case 42:	/* CMD42:  LOCK_UNLOCK */
-        if (sd->spi)
-            goto unimplemented_cmd;
+        if (sd->spi) {
+            goto unimplemented_spi_cmd;
+        }
         switch (sd->state) {
         case sd_transfer_state:
             sd->state = sd_receivingdata_state;
@@ -1413,7 +1417,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         qemu_log_mask(LOG_GUEST_ERROR, "SD: Unknown CMD%i\n", req.cmd);
         return sd_illegal;
 
-    unimplemented_cmd:
+    unimplemented_spi_cmd:
         /* Commands that are recognised but not yet implemented in SPI mode.  */
         qemu_log_mask(LOG_UNIMP, "SD: CMD%i not implemented in SPI mode\n",
                       req.cmd);
-- 
2.16.1

Re: [Qemu-devel] [PATCH v4 11/20] sdcard: use a more descriptive label 'unimplemented_spi_cmd'

[Alistair Francis]

On Thu, Feb 15, 2018 at 2:13 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Suggested-by: Alistair Francis <alistair.francis@xilinx.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Alistair

> ---
>  hw/sd/sd.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 0457f5214b..4f82638735 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1183,8 +1183,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>
>      /* Block write commands (Class 4) */
>      case 24:   /* CMD24:  WRITE_SINGLE_BLOCK */
> -        if (sd->spi)
> -            goto unimplemented_cmd;
> +        if (sd->spi) {
> +            goto unimplemented_spi_cmd;
> +        }
>          switch (sd->state) {
>          case sd_transfer_state:
>              /* Writing in SPI mode not implemented.  */
> @@ -1209,8 +1210,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          break;
>
>      case 25:   /* CMD25:  WRITE_MULTIPLE_BLOCK */
> -        if (sd->spi)
> -            goto unimplemented_cmd;
> +        if (sd->spi) {
> +            goto unimplemented_spi_cmd;
> +        }
>          switch (sd->state) {
>          case sd_transfer_state:
>              /* Writing in SPI mode not implemented.  */
> @@ -1250,8 +1252,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          break;
>
>      case 27:   /* CMD27:  PROGRAM_CSD */
> -        if (sd->spi)
> -            goto unimplemented_cmd;
> +        if (sd->spi) {
> +            goto unimplemented_spi_cmd;
> +        }
>          switch (sd->state) {
>          case sd_transfer_state:
>              sd->state = sd_receivingdata_state;
> @@ -1361,8 +1364,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>
>      /* Lock card commands (Class 7) */
>      case 42:   /* CMD42:  LOCK_UNLOCK */
> -        if (sd->spi)
> -            goto unimplemented_cmd;
> +        if (sd->spi) {
> +            goto unimplemented_spi_cmd;
> +        }
>          switch (sd->state) {
>          case sd_transfer_state:
>              sd->state = sd_receivingdata_state;
> @@ -1413,7 +1417,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          qemu_log_mask(LOG_GUEST_ERROR, "SD: Unknown CMD%i\n", req.cmd);
>          return sd_illegal;
>
> -    unimplemented_cmd:
> +    unimplemented_spi_cmd:
>          /* Commands that are recognised but not yet implemented in SPI mode.  */
>          qemu_log_mask(LOG_UNIMP, "SD: CMD%i not implemented in SPI mode\n",
>                        req.cmd);
> --
> 2.16.1
>
>

[Qemu-devel] [PATCH v4 12/20] sdcard: handles more commands in SPI mode

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 4f82638735..0ecb446fdb 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1390,9 +1390,11 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
     /* Application specific commands (Class 8) */
     case 55:	/* CMD55:  APP_CMD */
-        if (sd->rca != rca)
-            return sd_r0;
-
+        if (!sd->spi) {
+            if (sd->rca != rca) {
+                return sd_r0;
+            }
+        }
         sd->expecting_acmd = true;
         sd->card_status |= APP_CMD;
         return sd_r1;
@@ -1412,6 +1414,18 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         }
         break;
 
+    case 58:    /* CMD58:   READ_OCR (SPI) */
+        if (!sd->spi) {
+            goto bad_cmd;
+        }
+        return sd_r3;
+
+    case 59:    /* CMD59:   CRC_ON_OFF (SPI) */
+        if (!sd->spi) {
+            goto bad_cmd;
+        }
+        goto unimplemented_spi_cmd;
+
     default:
     bad_cmd:
         qemu_log_mask(LOG_GUEST_ERROR, "SD: Unknown CMD%i\n", req.cmd);
@@ -1436,6 +1450,9 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
     sd->card_status |= APP_CMD;
     switch (req.cmd) {
     case 6:	/* ACMD6:  SET_BUS_WIDTH */
+        if (sd->spi) {
+            goto unimplemented_spi_cmd;
+        }
         switch (sd->state) {
         case sd_transfer_state:
             sd->sd_status[0] &= 0x3f;
@@ -1569,6 +1586,12 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
     default:
         /* Fall back to standard commands.  */
         return sd_normal_command(sd, req);
+
+    unimplemented_spi_cmd:
+        /* Commands that are recognised but not yet implemented in SPI mode.  */
+        qemu_log_mask(LOG_UNIMP, "SD: CMD%i not implemented in SPI mode\n",
+                      req.cmd);
+        return sd_illegal;
     }
 
     qemu_log_mask(LOG_GUEST_ERROR, "SD: ACMD%i in a wrong state\n", req.cmd);
-- 
2.16.1

Re: [Qemu-devel] [PATCH v4 12/20] sdcard: handles more commands in SPI mode

[Alistair Francis]

On Thu, Feb 15, 2018 at 2:13 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Alistair

> ---
>  hw/sd/sd.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 4f82638735..0ecb446fdb 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1390,9 +1390,11 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>
>      /* Application specific commands (Class 8) */
>      case 55:   /* CMD55:  APP_CMD */
> -        if (sd->rca != rca)
> -            return sd_r0;
> -
> +        if (!sd->spi) {
> +            if (sd->rca != rca) {
> +                return sd_r0;
> +            }
> +        }
>          sd->expecting_acmd = true;
>          sd->card_status |= APP_CMD;
>          return sd_r1;
> @@ -1412,6 +1414,18 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          }
>          break;
>
> +    case 58:    /* CMD58:   READ_OCR (SPI) */
> +        if (!sd->spi) {
> +            goto bad_cmd;
> +        }
> +        return sd_r3;
> +
> +    case 59:    /* CMD59:   CRC_ON_OFF (SPI) */
> +        if (!sd->spi) {
> +            goto bad_cmd;
> +        }
> +        goto unimplemented_spi_cmd;
> +
>      default:
>      bad_cmd:
>          qemu_log_mask(LOG_GUEST_ERROR, "SD: Unknown CMD%i\n", req.cmd);
> @@ -1436,6 +1450,9 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>      sd->card_status |= APP_CMD;
>      switch (req.cmd) {
>      case 6:    /* ACMD6:  SET_BUS_WIDTH */
> +        if (sd->spi) {
> +            goto unimplemented_spi_cmd;
> +        }
>          switch (sd->state) {
>          case sd_transfer_state:
>              sd->sd_status[0] &= 0x3f;
> @@ -1569,6 +1586,12 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>      default:
>          /* Fall back to standard commands.  */
>          return sd_normal_command(sd, req);
> +
> +    unimplemented_spi_cmd:
> +        /* Commands that are recognised but not yet implemented in SPI mode.  */
> +        qemu_log_mask(LOG_UNIMP, "SD: CMD%i not implemented in SPI mode\n",
> +                      req.cmd);
> +        return sd_illegal;
>      }
>
>      qemu_log_mask(LOG_GUEST_ERROR, "SD: ACMD%i in a wrong state\n", req.cmd);
> --
> 2.16.1
>
>

[Qemu-devel] [PATCH v4 13/20] sdcard: check the card is in correct state for APP CMD (CMD55)

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 0ecb446fdb..c2f01a5fa3 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1390,6 +1390,14 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
     /* Application specific commands (Class 8) */
     case 55:	/* CMD55:  APP_CMD */
+        switch (sd->state) {
+        case sd_ready_state:
+        case sd_identification_state:
+        case sd_inactive_state:
+            return sd_illegal;
+        default:
+            break;
+        }
         if (!sd->spi) {
             if (sd->rca != rca) {
                 return sd_r0;
-- 
2.16.1

[Qemu-devel] [PATCH v4 14/20] sdcard: warn if host uses an incorrect address for APP CMD (CMD55)

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sd.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index c2f01a5fa3..5827379d0f 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1395,6 +1395,11 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         case sd_identification_state:
         case sd_inactive_state:
             return sd_illegal;
+        case sd_idle_state:
+            if (rca) {
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "SD: illegal RCA 0x%04x for APP_CMD\n", req.cmd);
+            }
         default:
             break;
         }
-- 
2.16.1

[Qemu-devel] [PATCH v4 15/20] sdcard: simplify SEND_IF_COND (CMD8)

[Philippe Mathieu-Daudé]

replace switch(single case) -> if()

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sd.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 5827379d0f..942143ab3e 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1012,23 +1012,19 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
     case 8:	/* CMD8:   SEND_IF_COND */
         /* Physical Layer Specification Version 2.00 command */
-        switch (sd->state) {
-        case sd_idle_state:
-            sd->vhs = 0;
-
-            /* No response if not exactly one VHS bit is set.  */
-            if (!(req.arg >> 8) || (req.arg >> (ctz32(req.arg & ~0xff) + 1))) {
-                return sd->spi ? sd_r7 : sd_r0;
-            }
-
-            /* Accept.  */
-            sd->vhs = req.arg;
-            return sd_r7;
-
-        default:
+        if (sd->state != sd_idle_state) {
             break;
         }
-        break;
+        sd->vhs = 0;
+
+        /* No response if not exactly one VHS bit is set.  */
+        if (!(req.arg >> 8) || (req.arg >> (ctz32(req.arg & ~0xff) + 1))) {
+            return sd->spi ? sd_r7 : sd_r0;
+        }
+
+        /* Accept.  */
+        sd->vhs = req.arg;
+        return sd_r7;
 
     case 9:	/* CMD9:   SEND_CSD */
         switch (sd->state) {
-- 
2.16.1

[Qemu-devel] [PATCH v4 16/20] sdcard: simplify SD_SEND_OP_COND (ACMD41)

[Philippe Mathieu-Daudé]

replace switch(single case) -> if()

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sd.c | 56 ++++++++++++++++++++++++++------------------------------
 1 file changed, 26 insertions(+), 30 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 942143ab3e..911aae6233 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1517,45 +1517,41 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
             sd->state = sd_transfer_state;
             return sd_r1;
         }
-        switch (sd->state) {
-        case sd_idle_state:
-            /* If it's the first ACMD41 since reset, we need to decide
-             * whether to power up. If this is not an enquiry ACMD41,
-             * we immediately report power on and proceed below to the
-             * ready state, but if it is, we set a timer to model a
-             * delay for power up. This works around a bug in EDK2
-             * UEFI, which sends an initial enquiry ACMD41, but
-             * assumes that the card is in ready state as soon as it
-             * sees the power up bit set. */
-            if (!FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP)) {
-                if ((req.arg & ACMD41_ENQUIRY_MASK) != 0) {
-                    timer_del(sd->ocr_power_timer);
-                    sd_ocr_powerup(sd);
-                } else {
-                    trace_sdcard_inquiry_cmd41();
-                    if (!timer_pending(sd->ocr_power_timer)) {
-                        timer_mod_ns(sd->ocr_power_timer,
-                                     (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)
-                                      + OCR_POWER_DELAY_NS));
-                    }
+        if (sd->state != sd_idle_state) {
+            break;
+        }
+        /* If it's the first ACMD41 since reset, we need to decide
+         * whether to power up. If this is not an enquiry ACMD41,
+         * we immediately report power on and proceed below to the
+         * ready state, but if it is, we set a timer to model a
+         * delay for power up. This works around a bug in EDK2
+         * UEFI, which sends an initial enquiry ACMD41, but
+         * assumes that the card is in ready state as soon as it
+         * sees the power up bit set. */
+        if (!FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP)) {
+            if ((req.arg & ACMD41_ENQUIRY_MASK) != 0) {
+                timer_del(sd->ocr_power_timer);
+                sd_ocr_powerup(sd);
+            } else {
+                trace_sdcard_inquiry_cmd41();
+                if (!timer_pending(sd->ocr_power_timer)) {
+                    timer_mod_ns(sd->ocr_power_timer,
+                                 (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)
+                                  + OCR_POWER_DELAY_NS));
                 }
             }
+        }
 
+        if (FIELD_EX32(sd->ocr & req.arg, OCR, VDD_VOLTAGE_WINDOW)) {
             /* We accept any voltage.  10000 V is nothing.
              *
              * Once we're powered up, we advance straight to ready state
              * unless it's an enquiry ACMD41 (bits 23:0 == 0).
              */
-            if (req.arg & ACMD41_ENQUIRY_MASK) {
-                sd->state = sd_ready_state;
-            }
-
-            return sd_r3;
-
-        default:
-            break;
+            sd->state = sd_ready_state;
         }
-        break;
+
+        return sd_r3;
 
     case 42:	/* ACMD42: SET_CLR_CARD_DETECT */
         switch (sd->state) {
-- 
2.16.1

[Qemu-devel] [PATCH v4 17/20] sdcard: add SD SEND_TUNING_BLOCK (CMD19)

[Philippe Mathieu-Daudé]

[based on a patch from Alistair Francis <alistair.francis@xilinx.com>
 from qemu/xilinx tag xilinx-v2015.2]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sd.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 911aae6233..4b0bb7992d 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1166,6 +1166,14 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         }
         break;
 
+    case 19:    /* CMD19: SEND_TUNING_BLOCK (SD) */
+        if (sd->state == sd_transfer_state) {
+            sd->state = sd_sendingdata_state;
+            sd->data_offset = 0;
+            return sd_r1;
+        }
+        break;
+
     case 23:    /* CMD23: SET_BLOCK_COUNT */
         switch (sd->state) {
         case sd_transfer_state:
@@ -1889,6 +1897,15 @@ void sd_write_data(SDState *sd, uint8_t value)
     }
 }
 
+#define SD_TUNING_BLOCK_SIZE    64
+
+static const uint32_t sd_tunning_data[SD_TUNING_BLOCK_SIZE / 4] = {
+    0xFF0FFF00, 0x0FFCC3CC, 0xC33CCCFF, 0xFEFFFEEF,
+    0xFFDFFFDD, 0xFFFBFFFB, 0XBFFF7FFF, 0X77F7BDEF,
+    0XFFF0FFF0, 0X0FFCCC3C, 0XCC33CCCF, 0XFFEFFFEE,
+    0XFFFDFFFD, 0XDFFFBFFF, 0XBBFFF7FF, 0XF77F7BDE,
+};
+
 uint8_t sd_read_data(SDState *sd)
 {
     /* TODO: Append CRCs */
@@ -1968,6 +1985,13 @@ uint8_t sd_read_data(SDState *sd)
         }
         break;
 
+    case 19:    /* CMD19:  SEND_TUNING_BLOCK (SD) */
+        if (sd->data_offset >= SD_TUNING_BLOCK_SIZE - 1) {
+            sd->state = sd_transfer_state;
+        }
+        ret = ((uint8_t *)(&sd_tunning_data))[sd->data_offset++];
+        break;
+
     case 22:	/* ACMD22: SEND_NUM_WR_BLOCKS */
         ret = sd->data[sd->data_offset ++];
 
-- 
2.16.1

Re: [Qemu-devel] [PATCH v4 17/20] sdcard: add SD SEND_TUNING_BLOCK (CMD19)

[Peter Maydell]

On 15 February 2018 at 22:13, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> [based on a patch from Alistair Francis <alistair.francis@xilinx.com>
>  from qemu/xilinx tag xilinx-v2015.2]
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>  hw/sd/sd.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 911aae6233..4b0bb7992d 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1166,6 +1166,14 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          }
>          break;
>
> +    case 19:    /* CMD19: SEND_TUNING_BLOCK (SD) */
> +        if (sd->state == sd_transfer_state) {
> +            sd->state = sd_sendingdata_state;
> +            sd->data_offset = 0;
> +            return sd_r1;
> +        }
> +        break;
> +
>      case 23:    /* CMD23: SET_BLOCK_COUNT */
>          switch (sd->state) {
>          case sd_transfer_state:
> @@ -1889,6 +1897,15 @@ void sd_write_data(SDState *sd, uint8_t value)
>      }
>  }
>
> +#define SD_TUNING_BLOCK_SIZE    64
> +
> +static const uint32_t sd_tunning_data[SD_TUNING_BLOCK_SIZE / 4] = {

Typo, s/tunning/tuning/.

> +    0xFF0FFF00, 0x0FFCC3CC, 0xC33CCCFF, 0xFEFFFEEF,
> +    0xFFDFFFDD, 0xFFFBFFFB, 0XBFFF7FFF, 0X77F7BDEF,

Can we have lowercase 'x' consistently here please?

> +    0XFFF0FFF0, 0X0FFCCC3C, 0XCC33CCCF, 0XFFEFFFEE,
> +    0XFFFDFFFD, 0XDFFFBFFF, 0XBBFFF7FF, 0XF77F7BDE,
> +};

A comment about what all these magic numbers are for would
be useful.


> +
>  uint8_t sd_read_data(SDState *sd)
>  {
>      /* TODO: Append CRCs */
> @@ -1968,6 +1985,13 @@ uint8_t sd_read_data(SDState *sd)
>          }
>          break;
>
> +    case 19:    /* CMD19:  SEND_TUNING_BLOCK (SD) */
> +        if (sd->data_offset >= SD_TUNING_BLOCK_SIZE - 1) {
> +            sd->state = sd_transfer_state;
> +        }
> +        ret = ((uint8_t *)(&sd_tunning_data))[sd->data_offset++];

If you're going to treat it as a uint8_t buffer it might be
cleaner to just declare it as a uint8_t buffer rather than
uint32_t: you'd get to avoid the cast here and the / 4 in
the array definition.

> +        break;
> +
>      case 22:   /* ACMD22: SEND_NUM_WR_BLOCKS */
>          ret = sd->data[sd->data_offset ++];
>
> --
> 2.16.1
>

thanks
-- PMM

[Qemu-devel] [PATCH v4 18/20] sdcard: implement the UHS-I SWITCH_FUNCTION entries (Spec v3)

[Philippe Mathieu-Daudé]

[based on a patch from Alistair Francis <alistair.francis@xilinx.com>
 from qemu/xilinx tag xilinx-v2015.2]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c         | 148 +++++++++++++++++++++++++++++++++++++++++++++--------
 hw/sd/trace-events |   1 +
 2 files changed, 127 insertions(+), 22 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 4b0bb7992d..ada96f5574 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -124,6 +124,7 @@ struct SDState {
     bool enable;
     uint8_t dat_lines;
     bool cmd_line;
+    bool uhs_enabled;
 };
 
 static const char *sd_state_name(enum SDCardStates state)
@@ -563,6 +564,7 @@ static void sd_reset(DeviceState *dev)
     sd->expecting_acmd = false;
     sd->dat_lines = 0xf;
     sd->cmd_line = true;
+    sd->uhs_enabled = false;
     sd->multi_blk_cnt = 0;
 }
 
@@ -761,30 +763,132 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
     return ret;
 }
 
+/* Function Group */
+enum {
+    SD_FG_MIN               = 1,
+    SD_FG_ACCESS_MODE       = 1,
+    SD_FG_COMMAND_SYSTEM    = 2,
+    SD_FG_DRIVER_STRENGTH   = 3,
+    SD_FG_CURRENT_LIMIT     = 4,
+    SD_FG_RSVD_5            = 5,
+    SD_FG_RSVD_6            = 6,
+    SD_FG_COUNT
+};
+
+/* Function name */
+#define SD_FN_COUNT 16
+
+static const char *sd_fn_grp_name[SD_FG_COUNT] = {
+    [SD_FG_ACCESS_MODE]     = "ACCESS_MODE",
+    [SD_FG_COMMAND_SYSTEM]  = "COMMAND_SYSTEM",
+    [SD_FG_DRIVER_STRENGTH] = "DRIVER_STRENGTH",
+    [SD_FG_CURRENT_LIMIT]   = "CURRENT_LIMIT",
+    [SD_FG_RSVD_5]          = "RSVD5",
+    [SD_FG_RSVD_6]          = "RSVD6",
+};
+
+typedef struct sd_fn_support {
+    const char *name;
+    bool uhs_only;
+    bool unimp;
+} sd_fn_support;
+
+static const sd_fn_support *sd_fn_support_defs[SD_FG_COUNT] = {
+    [SD_FG_ACCESS_MODE] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default/SDR12" },
+        [1] = { .name = "high-speed/SDR25" },
+        [2] = { .name = "SDR50",    .uhs_only = true },
+        [3] = { .name = "SDR104",   .uhs_only = true },
+        [4] = { .name = "DDR50",    .uhs_only = true },
+    },
+    [SD_FG_COMMAND_SYSTEM] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default" },
+        [1] = { .name = "For eC" },
+        [3] = { .name = "OTP",      .unimp = true },
+        [4] = { .name = "ASSD",     .unimp = true },
+    },
+    [SD_FG_DRIVER_STRENGTH] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default/Type B" },
+        [1] = { .name = "Type A",   .uhs_only = true },
+        [2] = { .name = "Type C",   .uhs_only = true },
+        [3] = { .name = "Type D",   .uhs_only = true },
+    },
+    [SD_FG_CURRENT_LIMIT] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default/200mA" },
+        [1] = { .name = "400mA",    .uhs_only = true },
+        [2] = { .name = "600mA",    .uhs_only = true },
+        [3] = { .name = "800mA",    .uhs_only = true },
+    },
+    [SD_FG_RSVD_5] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default" },
+    },
+    [SD_FG_RSVD_6] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default" },
+    },
+};
+
+#define SD_FN_NO_INFLUENCE          (1 << 15)
+
 static void sd_function_switch(SDState *sd, uint32_t arg)
 {
-    int i, mode, new_func;
-    mode = !!(arg & 0x80000000);
-
-    sd->data[0] = 0x00;		/* Maximum current consumption */
-    sd->data[1] = 0x01;
-    sd->data[2] = 0x80;		/* Supported group 6 functions */
-    sd->data[3] = 0x01;
-    sd->data[4] = 0x80;		/* Supported group 5 functions */
-    sd->data[5] = 0x01;
-    sd->data[6] = 0x80;		/* Supported group 4 functions */
-    sd->data[7] = 0x01;
-    sd->data[8] = 0x80;		/* Supported group 3 functions */
-    sd->data[9] = 0x01;
-    sd->data[10] = 0x80;	/* Supported group 2 functions */
-    sd->data[11] = 0x43;
-    sd->data[12] = 0x80;	/* Supported group 1 functions */
-    sd->data[13] = 0x03;
-    for (i = 0; i < 6; i ++) {
-        new_func = (arg >> (i * 4)) & 0x0f;
-        if (mode && new_func != 0x0f)
-            sd->function_group[i] = new_func;
-        sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
+    int fn_grp, new_func, i;
+    uint8_t *data_p;
+    bool mode = extract32(arg, 31, 1);  /* 0: check only, 1: do switch */
+
+    stw_be_p(sd->data + 0, 0x0001);     /* Maximum current consumption */
+
+    data_p = &sd->data[2];
+    for (fn_grp = SD_FG_COUNT - 1; fn_grp >= SD_FG_MIN; fn_grp--) {
+        uint16_t supported_fns = SD_FN_NO_INFLUENCE;
+        for (i = 0; i < SD_FN_COUNT; ++i) {
+            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][i];
+
+            if (def->name && !def->unimp &&
+                    !(def->uhs_only && !sd->uhs_enabled)) {
+                supported_fns |= 1 << i;
+            }
+        }
+        stw_be_p(data_p, supported_fns);
+        data_p += 2;
+    }
+
+    assert(data_p == &sd->data[14]);
+    for (fn_grp = SD_FG_COUNT - 1; fn_grp >= SD_FG_MIN; fn_grp--) {
+        new_func = (arg >> ((fn_grp - 1) * 4)) & 0x0f;
+        if (new_func == 0xf) {
+            new_func = sd->function_group[fn_grp - 1];
+        } else {
+            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][new_func];
+            if (mode) {
+                if (!def->name) {
+                    qemu_log_mask(LOG_GUEST_ERROR,
+                                  "Function %d not a valid for "
+                                  "function group %d\n",
+                                  new_func, fn_grp);
+                    new_func = 0xf;
+                } else if (def->unimp) {
+                    qemu_log_mask(LOG_UNIMP,
+                                  "Function %s (fn grp %d) not implemented\n",
+                                  def->name, fn_grp);
+                    new_func = 0xf;
+                } else if (def->uhs_only && !sd->uhs_enabled) {
+                    qemu_log_mask(LOG_GUEST_ERROR,
+                                  "Function %s (fn grp %d) only "
+                                  "valid in UHS mode\n",
+                                  def->name, fn_grp);
+                    new_func = 0xf;
+                } else {
+                    sd->function_group[fn_grp - 1] = new_func;
+                }
+            }
+            trace_sdcard_function_select(def->name, sd_fn_grp_name[fn_grp],
+                                         mode);
+        }
+        if (!(fn_grp & 0x1)) { /* evens go in high nibble */
+            *data_p = new_func << 4;
+        } else { /* odds go in low nibble */
+            *(data_p++) |= new_func;
+        }
     }
     memset(&sd->data[17], 0, 47);
     stw_be_p(sd->data + 65, sd_crc16(sd->data, 64));
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 2059ace61f..c106541a47 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -42,6 +42,7 @@ sdcard_write_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
 sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint8_t value) "%s %20s/ CMD%02d value 0x%02x"
 sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, int length) "%s %20s/ CMD%02d len %d"
 sdcard_set_voltage(uint16_t millivolts) "%u mV"
+sdcard_function_select(const char *fn_name, const char *grp_name, bool do_switch) "Function %s (group: %s, sw: %u)"
 
 # hw/sd/milkymist-memcard.c
 milkymist_memcard_memory_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
-- 
2.16.1

[Qemu-devel] [PATCH v4 19/20] sdcard: add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index ada96f5574..b9429b06ca 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -93,6 +93,7 @@ struct SDState {
     /* Configurable properties */
     BlockBackend *blk;
     bool spi;
+    uint8_t uhs_supported;
 
     uint32_t mode;    /* current card mode, one of SDCardModes */
     int32_t state;    /* current card state, one of SDCardStates */
@@ -292,6 +293,8 @@ static void sd_set_ocr(SDState *sd)
 {
     /* All voltages OK */
     sd->ocr = R_OCR_VDD_VOLTAGE_WIN_HI_MASK;
+
+    sd->ocr = FIELD_DP32(sd->ocr, OCR, ACCEPT_SWITCH_1V8, !!sd->uhs_supported);
 }
 
 static void sd_ocr_powerup(void *opaque)
@@ -2189,6 +2192,7 @@ static Property sd_properties[] = {
      * board to ensure that ssi transfers only occur when the chip select
      * is asserted.  */
     DEFINE_PROP_BOOL("spi", SDState, spi, false),
+    DEFINE_PROP_UINT8("uhs", SDState, uhs_supported, UHS_NOT_SUPPORTED),
     DEFINE_PROP_END_OF_LIST()
 };
 
-- 
2.16.1

Re: [Qemu-devel] [PATCH v4 19/20] sdcard: add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit

[Alistair Francis]

On Thu, Feb 15, 2018 at 2:13 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Alistair

> ---
>  hw/sd/sd.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index ada96f5574..b9429b06ca 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -93,6 +93,7 @@ struct SDState {
>      /* Configurable properties */
>      BlockBackend *blk;
>      bool spi;
> +    uint8_t uhs_supported;
>
>      uint32_t mode;    /* current card mode, one of SDCardModes */
>      int32_t state;    /* current card state, one of SDCardStates */
> @@ -292,6 +293,8 @@ static void sd_set_ocr(SDState *sd)
>  {
>      /* All voltages OK */
>      sd->ocr = R_OCR_VDD_VOLTAGE_WIN_HI_MASK;
> +
> +    sd->ocr = FIELD_DP32(sd->ocr, OCR, ACCEPT_SWITCH_1V8, !!sd->uhs_supported);
>  }
>
>  static void sd_ocr_powerup(void *opaque)
> @@ -2189,6 +2192,7 @@ static Property sd_properties[] = {
>       * board to ensure that ssi transfers only occur when the chip select
>       * is asserted.  */
>      DEFINE_PROP_BOOL("spi", SDState, spi, false),
> +    DEFINE_PROP_UINT8("uhs", SDState, uhs_supported, UHS_NOT_SUPPORTED),
>      DEFINE_PROP_END_OF_LIST()
>  };
>
> --
> 2.16.1
>
>

Re: [Qemu-devel] [PATCH v4 19/20] sdcard: add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit

[Peter Maydell]

On 15 February 2018 at 22:13, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index ada96f5574..b9429b06ca 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -93,6 +93,7 @@ struct SDState {
>      /* Configurable properties */
>      BlockBackend *blk;
>      bool spi;
> +    uint8_t uhs_supported;
>
>      uint32_t mode;    /* current card mode, one of SDCardModes */
>      int32_t state;    /* current card state, one of SDCardStates */
> @@ -292,6 +293,8 @@ static void sd_set_ocr(SDState *sd)
>  {
>      /* All voltages OK */
>      sd->ocr = R_OCR_VDD_VOLTAGE_WIN_HI_MASK;
> +
> +    sd->ocr = FIELD_DP32(sd->ocr, OCR, ACCEPT_SWITCH_1V8, !!sd->uhs_supported);
>  }
>
>  static void sd_ocr_powerup(void *opaque)
> @@ -2189,6 +2192,7 @@ static Property sd_properties[] = {
>       * board to ensure that ssi transfers only occur when the chip select
>       * is asserted.  */
>      DEFINE_PROP_BOOL("spi", SDState, spi, false),
> +    DEFINE_PROP_UINT8("uhs", SDState, uhs_supported, UHS_NOT_SUPPORTED),
>      DEFINE_PROP_END_OF_LIST()
>  };

The field name and the use of the value suggest that this is just
a boolean flag, so why define it as a PROP_UINT8 rather than PROP_BOOL ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 19/20] sdcard: add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit

[Philippe Mathieu-Daudé]

Hi Peter,

On 02/22/2018 11:25 AM, Peter Maydell wrote:
> On 15 February 2018 at 22:13, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/sd.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index ada96f5574..b9429b06ca 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -93,6 +93,7 @@ struct SDState {
>>      /* Configurable properties */
>>      BlockBackend *blk;
>>      bool spi;
>> +    uint8_t uhs_supported;
>>
>>      uint32_t mode;    /* current card mode, one of SDCardModes */
>>      int32_t state;    /* current card state, one of SDCardStates */
>> @@ -292,6 +293,8 @@ static void sd_set_ocr(SDState *sd)
>>  {
>>      /* All voltages OK */
>>      sd->ocr = R_OCR_VDD_VOLTAGE_WIN_HI_MASK;
>> +
>> +    sd->ocr = FIELD_DP32(sd->ocr, OCR, ACCEPT_SWITCH_1V8, !!sd->uhs_supported);
>>  }
>>
>>  static void sd_ocr_powerup(void *opaque)
>> @@ -2189,6 +2192,7 @@ static Property sd_properties[] = {
>>       * board to ensure that ssi transfers only occur when the chip select
>>       * is asserted.  */
>>      DEFINE_PROP_BOOL("spi", SDState, spi, false),
>> +    DEFINE_PROP_UINT8("uhs", SDState, uhs_supported, UHS_NOT_SUPPORTED),
>>      DEFINE_PROP_END_OF_LIST()
>>  };
> 
> The field name and the use of the value suggest that this is just
> a boolean flag, so why define it as a PROP_UINT8 rather than PROP_BOOL ?

Here is the full enum:

typedef enum  {
    UHS_NOT_SUPPORTED   = 0,
    UHS_I               = 1,
    UHS_II              = 2,    /* currently not supported */
    UHS_III             = 3,    /* currently not supported */
} sd_uhs_mode_t;

I'll rename it "uhs_mode" to reflect this is not a boolean.

Some controllers modeled support UHS-II, I guess we'll need to implement
it at some point during the year.

Regards,

Phil.

[Qemu-devel] [PATCH v4 20/20] sdcard: add an enum for the SD PHY Spec version

[Philippe Mathieu-Daudé]

So far this device intends to model the Spec v1.10

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sd.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b9429b06ca..d4565626ce 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -47,6 +47,11 @@
 
 //#define DEBUG_SD 1
 
+typedef enum {
+    SD_PHY_SPEC_VER_1_10 = 110,
+    SD_PHY_SPEC_VER_2_00 = 200, /* not yet supported */
+} sd_phy_spec_ver_t;
+
 typedef enum {
     sd_r0 = 0,    /* no response */
     sd_r1,        /* normal response command */
@@ -122,6 +127,7 @@ struct SDState {
     qemu_irq inserted_cb;
     QEMUTimer *ocr_power_timer;
     const char *proto_name;
+    int spec_version;
     bool enable;
     uint8_t dat_lines;
     bool cmd_line;
@@ -2169,6 +2175,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
     int ret;
 
     sd->proto_name = sd->spi ? "SPI" : "SD";
+    sd->spec_version = SD_PHY_SPEC_VER_1_10;
 
     if (sd->blk && blk_is_read_only(sd->blk)) {
         error_setg(errp, "Cannot use read-only drive as SD card");
-- 
2.16.1

Re: [Qemu-devel] [PATCH v4 20/20] sdcard: add an enum for the SD PHY Spec version

[Peter Maydell]

On 15 February 2018 at 22:13, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> So far this device intends to model the Spec v1.10
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>  hw/sd/sd.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index b9429b06ca..d4565626ce 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -47,6 +47,11 @@
>
>  //#define DEBUG_SD 1
>
> +typedef enum {
> +    SD_PHY_SPEC_VER_1_10 = 110,
> +    SD_PHY_SPEC_VER_2_00 = 200, /* not yet supported */
> +} sd_phy_spec_ver_t;
> +
>  typedef enum {
>      sd_r0 = 0,    /* no response */
>      sd_r1,        /* normal response command */
> @@ -122,6 +127,7 @@ struct SDState {
>      qemu_irq inserted_cb;
>      QEMUTimer *ocr_power_timer;
>      const char *proto_name;
> +    int spec_version;
>      bool enable;
>      uint8_t dat_lines;
>      bool cmd_line;
> @@ -2169,6 +2175,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
>      int ret;
>
>      sd->proto_name = sd->spi ? "SPI" : "SD";
> +    sd->spec_version = SD_PHY_SPEC_VER_1_10;
>
>      if (sd->blk && blk_is_read_only(sd->blk)) {
>          error_setg(errp, "Cannot use read-only drive as SD card");

I think I'd prefer to see this patch with the ones that actually use
the field -- as it stands it's never used.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 20/20] sdcard: add an enum for the SD PHY Spec version

[Philippe Mathieu-Daudé]

On 02/22/2018 11:26 AM, Peter Maydell wrote:
> On 15 February 2018 at 22:13, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> So far this device intends to model the Spec v1.10
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>  hw/sd/sd.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index b9429b06ca..d4565626ce 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -47,6 +47,11 @@
>>
>>  //#define DEBUG_SD 1
>>
>> +typedef enum {
>> +    SD_PHY_SPEC_VER_1_10 = 110,
>> +    SD_PHY_SPEC_VER_2_00 = 200, /* not yet supported */
>> +} sd_phy_spec_ver_t;
>> +
>>  typedef enum {
>>      sd_r0 = 0,    /* no response */
>>      sd_r1,        /* normal response command */
>> @@ -122,6 +127,7 @@ struct SDState {
>>      qemu_irq inserted_cb;
>>      QEMUTimer *ocr_power_timer;
>>      const char *proto_name;
>> +    int spec_version;
>>      bool enable;
>>      uint8_t dat_lines;
>>      bool cmd_line;
>> @@ -2169,6 +2175,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
>>      int ret;
>>
>>      sd->proto_name = sd->spi ? "SPI" : "SD";
>> +    sd->spec_version = SD_PHY_SPEC_VER_1_10;
>>
>>      if (sd->blk && blk_is_read_only(sd->blk)) {
>>          error_setg(errp, "Cannot use read-only drive as SD card");
> 
> I think I'd prefer to see this patch with the ones that actually use
> the field -- as it stands it's never used.

Indeed I see, thanks.

Re: [Qemu-devel] [PATCH v4 00/20] SDCard: bugfixes, support UHS-I (part 5)

[Philippe Mathieu-Daudé]

ping? :)

On 02/15/2018 07:13 PM, Philippe Mathieu-Daudé wrote:
> Some refactors, few bugfixes, better SD/SPI support.
> 
> With this series apply, machines can use SD cards in UHS-I mode.
> (mostly imported from Alistair Francis work)
> 
> MMC mode split out for another series,
> so UHS enabled MMC cards are still not usable:
> 
>   kernel: mmc0: SDHCI controller on PCI [0000:00:05.0] using ADMA
>   kernel: sd 0:0:0:0: Attached scsi generic sg0 type 0
>   kernel: mmc0: Skipping voltage switch
>   [mmc kthread looping]
> 
> Since v3:
> - simpler SPI handling, improved descriptions (Alistair review)
> - inverted patches 16/17 order
> 
> Since v2:
> - split again in 2... other part is cleanup/tracing
> 
> Since v1:
> - rewrote mostly all patches to keep it simpler.
> 
> $ git backport-diff
> 001/20:[----] [-C] 'sdcard: Don't always set the high capacity bit'
> 002/20:[----] [-C] 'sdcard: update the CSD CRC register regardless the CSD structure version'
> 003/20:[----] [-C] 'sdcard: fix the 'maximum data transfer rate' to 25MHz'
> 004/20:[----] [-C] 'sdcard: clean the SCR register and add few comments'
> 005/20:[----] [--] 'sdcard: remove commands from unsupported old MMC specification'
> 006/20:[----] [--] 'sdcard: simplify using the ldst API'
> 007/20:[0008] [FC] 'sdcard: use the correct masked OCR in the R3 reply'
> 008/20:[----] [-C] 'sdcard: use the registerfields API for the CARD_STATUS register masks'
> 009/20:[----] [--] 'sdcard: handle CMD54 (SDIO)'
> 010/20:[down] 'sdcard: handle the Security Specification commands'
> 011/20:[down] 'sdcard: use a more descriptive label 'unimplemented_spi_cmd''
> 012/20:[0034] [FC] 'sdcard: handles more commands in SPI mode'
> 013/20:[----] [--] 'sdcard: check the card is in correct state for APP CMD (CMD55)'
> 014/20:[----] [--] 'sdcard: warn if host uses an incorrect address for APP CMD (CMD55)'
> 015/20:[----] [--] 'sdcard: simplify SEND_IF_COND (CMD8)'
> 016/20:[----] [--] 'sdcard: simplify SD_SEND_OP_COND (ACMD41)'
> 017/20:[----] [--] 'sdcard: add SD SEND_TUNING_BLOCK (CMD19)'
> 018/20:[----] [--] 'sdcard: implement the UHS-I SWITCH_FUNCTION entries (Spec v3)'
> 019/20:[----] [-C] 'sdcard: add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit'
> 020/20:[----] [--] 'sdcard: add an enum for the SD PHY Spec version'
> 
> Based-on: 20180215220540.6556-12-f4bug@amsat.org
> 
> Philippe Mathieu-Daudé (20):
>   sdcard: Don't always set the high capacity bit
>   sdcard: update the CSD CRC register regardless the CSD structure
>     version
>   sdcard: fix the 'maximum data transfer rate' to 25MHz
>   sdcard: clean the SCR register and add few comments
>   sdcard: remove commands from unsupported old MMC specification
>   sdcard: simplify using the ldst API
>   sdcard: use the correct masked OCR in the R3 reply
>   sdcard: use the registerfields API for the CARD_STATUS register masks
>   sdcard: handle CMD54 (SDIO)
>   sdcard: handle the Security Specification commands
>   sdcard: use a more descriptive label 'unimplemented_spi_cmd'
>   sdcard: handles more commands in SPI mode
>   sdcard: check the card is in correct state for APP CMD (CMD55)
>   sdcard: warn if host uses an incorrect address for APP CMD (CMD55)
>   sdcard: simplify SEND_IF_COND (CMD8)
>   sdcard: simplify SD_SEND_OP_COND (ACMD41)
>   sdcard: add SD SEND_TUNING_BLOCK (CMD19)
>   sdcard: implement the UHS-I SWITCH_FUNCTION entries (Spec v3)
>   sdcard: add a 'uhs' property, update the OCR register
>     ACCEPT_SWITCH_1V8 bit
>   sdcard: add an enum for the SD PHY Spec version
> 
>  hw/sd/sd.c         | 498 ++++++++++++++++++++++++++++++++++++-----------------
>  hw/sd/trace-events |   1 +
>  2 files changed, 343 insertions(+), 156 deletions(-)
> 

Re: [Qemu-devel] [PATCH v4 00/20] SDCard: bugfixes, support UHS-I (part 5)

[Peter Maydell]

On 15 February 2018 at 22:13, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Some refactors, few bugfixes, better SD/SPI support.
>
> With this series apply, machines can use SD cards in UHS-I mode.
> (mostly imported from Alistair Francis work)
>
> MMC mode split out for another series,
> so UHS enabled MMC cards are still not usable:
>
>   kernel: mmc0: SDHCI controller on PCI [0000:00:05.0] using ADMA
>   kernel: sd 0:0:0:0: Attached scsi generic sg0 type 0
>   kernel: mmc0: Skipping voltage switch
>   [mmc kthread looping]
>
> Since v3:
> - simpler SPI handling, improved descriptions (Alistair review)
> - inverted patches 16/17 order
>
> Since v2:
> - split again in 2... other part is cleanup/tracing
>
> Since v1:
> - rewrote mostly all patches to keep it simpler.
>
> $ git backport-diff
> 001/20:[----] [-C] 'sdcard: Don't always set the high capacity bit'
> 002/20:[----] [-C] 'sdcard: update the CSD CRC register regardless the CSD structure version'
> 003/20:[----] [-C] 'sdcard: fix the 'maximum data transfer rate' to 25MHz'
> 004/20:[----] [-C] 'sdcard: clean the SCR register and add few comments'
> 005/20:[----] [--] 'sdcard: remove commands from unsupported old MMC specification'
> 006/20:[----] [--] 'sdcard: simplify using the ldst API'
> 007/20:[0008] [FC] 'sdcard: use the correct masked OCR in the R3 reply'
> 008/20:[----] [-C] 'sdcard: use the registerfields API for the CARD_STATUS register masks'
> 009/20:[----] [--] 'sdcard: handle CMD54 (SDIO)'
> 010/20:[down] 'sdcard: handle the Security Specification commands'
> 011/20:[down] 'sdcard: use a more descriptive label 'unimplemented_spi_cmd''
> 012/20:[0034] [FC] 'sdcard: handles more commands in SPI mode'
> 013/20:[----] [--] 'sdcard: check the card is in correct state for APP CMD (CMD55)'
> 014/20:[----] [--] 'sdcard: warn if host uses an incorrect address for APP CMD (CMD55)'
> 015/20:[----] [--] 'sdcard: simplify SEND_IF_COND (CMD8)'
> 016/20:[----] [--] 'sdcard: simplify SD_SEND_OP_COND (ACMD41)'
> 017/20:[----] [--] 'sdcard: add SD SEND_TUNING_BLOCK (CMD19)'
> 018/20:[----] [--] 'sdcard: implement the UHS-I SWITCH_FUNCTION entries (Spec v3)'
> 019/20:[----] [-C] 'sdcard: add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit'
> 020/20:[----] [--] 'sdcard: add an enum for the SD PHY Spec version'

I've applied patches 1 to 16 to target-arm.next.

Removing the CMD11 support worries me a bit -- presumably it was put there
because some guest actually uses it -- but my test images seem to
still boot OK...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 00/20] SDCard: bugfixes, support UHS-I (part 5)

[Philippe Mathieu-Daudé]

Hi Peter,

On 02/22/2018 11:31 AM, Peter Maydell wrote:
> On 15 February 2018 at 22:13, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Some refactors, few bugfixes, better SD/SPI support.
>>
>> With this series apply, machines can use SD cards in UHS-I mode.
>> (mostly imported from Alistair Francis work)
>>
>> MMC mode split out for another series,
>> so UHS enabled MMC cards are still not usable:
>>
>>   kernel: mmc0: SDHCI controller on PCI [0000:00:05.0] using ADMA
>>   kernel: sd 0:0:0:0: Attached scsi generic sg0 type 0
>>   kernel: mmc0: Skipping voltage switch
>>   [mmc kthread looping]
>>
>> Since v3:
>> - simpler SPI handling, improved descriptions (Alistair review)
>> - inverted patches 16/17 order
>>
>> Since v2:
>> - split again in 2... other part is cleanup/tracing
>>
>> Since v1:
>> - rewrote mostly all patches to keep it simpler.
>>
>> $ git backport-diff
>> 001/20:[----] [-C] 'sdcard: Don't always set the high capacity bit'
>> 002/20:[----] [-C] 'sdcard: update the CSD CRC register regardless the CSD structure version'
>> 003/20:[----] [-C] 'sdcard: fix the 'maximum data transfer rate' to 25MHz'
>> 004/20:[----] [-C] 'sdcard: clean the SCR register and add few comments'
>> 005/20:[----] [--] 'sdcard: remove commands from unsupported old MMC specification'
>> 006/20:[----] [--] 'sdcard: simplify using the ldst API'
>> 007/20:[0008] [FC] 'sdcard: use the correct masked OCR in the R3 reply'
>> 008/20:[----] [-C] 'sdcard: use the registerfields API for the CARD_STATUS register masks'
>> 009/20:[----] [--] 'sdcard: handle CMD54 (SDIO)'
>> 010/20:[down] 'sdcard: handle the Security Specification commands'
>> 011/20:[down] 'sdcard: use a more descriptive label 'unimplemented_spi_cmd''
>> 012/20:[0034] [FC] 'sdcard: handles more commands in SPI mode'
>> 013/20:[----] [--] 'sdcard: check the card is in correct state for APP CMD (CMD55)'
>> 014/20:[----] [--] 'sdcard: warn if host uses an incorrect address for APP CMD (CMD55)'
>> 015/20:[----] [--] 'sdcard: simplify SEND_IF_COND (CMD8)'
>> 016/20:[----] [--] 'sdcard: simplify SD_SEND_OP_COND (ACMD41)'
>> 017/20:[----] [--] 'sdcard: add SD SEND_TUNING_BLOCK (CMD19)'
>> 018/20:[----] [--] 'sdcard: implement the UHS-I SWITCH_FUNCTION entries (Spec v3)'
>> 019/20:[----] [-C] 'sdcard: add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit'
>> 020/20:[----] [--] 'sdcard: add an enum for the SD PHY Spec version'
> 
> I've applied patches 1 to 16 to target-arm.next.

Thanks!

> Removing the CMD11 support worries me a bit -- presumably it was put there
> because some guest actually uses it -- but my test images seem to
> still boot OK...

Yes you are right, this command was available in the first MMC specs but
not included in the SD specs. The SD/MMC differences are not always easy
to track and implement, the last series is supposed to handle MMC in a
clearer way, I'll see to restore this command once there.

> 
> thanks
> -- PMM
>