[Qemu-devel] [PATCH 0/8] SDCard: improve tracing, support UHS-I

[Qemu-devel] [PATCH 0/8] SDCard: improve tracing, support UHS-I

[Philippe Mathieu-Daudé]

This series contains the leftover patches from these 2 series:
- SDCard: housekeeping, add tracing (part 4)
- SDCard: bugfixes, support UHS-I (part 5)
with Peter Maydell comments addressed.

I misunderstood the 2.12 train deadline, but better to have the series
standing in the list rather than forgot it.

Philippe Mathieu-Daudé (8):
  sdcard: Do not trace CMD55, except when we already expect an ACMD
  sdcard: Display command name when tracing CMD/ACMD
  sdcard: Display which protocol is used when tracing (SD or SPI)
  sdcard: Add the Tuning Command (CMD19)
  sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)
  sdcard: Add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit
  sdhci: Fix a typo in comment
  MAINTAINERS: Add entries for SD (SDHCI, SDBus, SDCard)

 hw/sd/sdmmc-internal.h |  20 +++++
 hw/sd/sd.c             | 208 ++++++++++++++++++++++++++++++++++++++++++-------
 hw/sd/sdhci.c          |   4 +-
 hw/sd/sdmmc-internal.c |  72 +++++++++++++++++
 hw/sd/Makefile.objs    |   2 +-
 hw/sd/trace-events     |   9 ++-
 MAINTAINERS            |   8 ++
 7 files changed, 288 insertions(+), 35 deletions(-)
 create mode 100644 hw/sd/sdmmc-internal.c

-- 
2.16.2

[Qemu-devel] [PATCH 1/8] sdcard: Do not trace CMD55, except when we already expect an ACMD

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 933890e86f..4a9520e38e 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -866,13 +866,18 @@ static void sd_lock_command(SDState *sd)
         sd->card_status &= ~CARD_IS_LOCKED;
 }
 
-static sd_rsp_type_t sd_normal_command(SDState *sd,
-                                       SDRequest req)
+static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 {
     uint32_t rca = 0x0000;
     uint64_t addr = (sd->ocr & (1 << 30)) ? (uint64_t) req.arg << 9 : req.arg;
 
-    trace_sdcard_normal_command(req.cmd, req.arg, sd_state_name(sd->state));
+    /* CMD55 precedes an ACMD, so we are not interested in tracing it.
+     * However there is no ACMD55, so we want to trace this particular case.
+     */
+    if (req.cmd != 55 || sd->expecting_acmd) {
+        trace_sdcard_normal_command(req.cmd, req.arg,
+                                    sd_state_name(sd->state));
+    }
 
     /* Not interpreting this as an app command */
     sd->card_status &= ~APP_CMD;
-- 
2.16.2

[Qemu-devel] [PATCH 2/8] sdcard: Display command name when tracing CMD/ACMD

[Philippe Mathieu-Daudé]

The SDBus will reuse these functions, so we put them in a new source file.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdmmc-internal.h | 20 ++++++++++++++
 hw/sd/sd.c             | 13 +++++----
 hw/sd/sdmmc-internal.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/sd/Makefile.objs    |  2 +-
 hw/sd/trace-events     |  8 +++---
 5 files changed, 105 insertions(+), 10 deletions(-)
 create mode 100644 hw/sd/sdmmc-internal.c

diff --git a/hw/sd/sdmmc-internal.h b/hw/sd/sdmmc-internal.h
index 0e96cb0081..bf6fb06128 100644
--- a/hw/sd/sdmmc-internal.h
+++ b/hw/sd/sdmmc-internal.h
@@ -12,4 +12,24 @@
 
 #define SDMMC_CMD_MAX 64
 
+/**
+ * sd_cmd_name:
+ * @cmd: A SD "normal" command, upto SDMMC_CMD_MAX.
+ *
+ * Returns an human useful name describing the command.
+ *
+ * Returns: The command name of @cmd or "UNKNOWN_CMD".
+ */
+const char *sd_cmd_name(uint8_t cmd);
+
+/**
+ * sd_acmd_name:
+ * @cmd: A SD "Application-Specific" command, upto SDMMC_CMD_MAX.
+ *
+ * Returns an human useful name describing the application command.
+ *
+ * Returns: The application command name of @cmd or "UNKNOWN_ACMD".
+ */
+const char *sd_acmd_name(uint8_t cmd);
+
 #endif
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 4a9520e38e..bb149aa644 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -875,8 +875,8 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
      * However there is no ACMD55, so we want to trace this particular case.
      */
     if (req.cmd != 55 || sd->expecting_acmd) {
-        trace_sdcard_normal_command(req.cmd, req.arg,
-                                    sd_state_name(sd->state));
+        trace_sdcard_normal_command(sd_cmd_name(req.cmd), req.cmd,
+                                    req.arg, sd_state_name(sd->state));
     }
 
     /* Not interpreting this as an app command */
@@ -1455,7 +1455,8 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 static sd_rsp_type_t sd_app_command(SDState *sd,
                                     SDRequest req)
 {
-    trace_sdcard_app_command(req.cmd, req.arg);
+    trace_sdcard_app_command(sd_acmd_name(req.cmd),
+                             req.cmd, req.arg, sd_state_name(sd->state));
     sd->card_status |= APP_CMD;
     switch (req.cmd) {
     case 6:	/* ACMD6:  SET_BUS_WIDTH */
@@ -1770,7 +1771,8 @@ void sd_write_data(SDState *sd, uint8_t value)
     if (sd->card_status & (ADDRESS_ERROR | WP_VIOLATION))
         return;
 
-    trace_sdcard_write_data(sd->current_cmd, value);
+    trace_sdcard_write_data(sd_acmd_name(sd->current_cmd),
+                            sd->current_cmd, value);
     switch (sd->current_cmd) {
     case 24:	/* CMD24:  WRITE_SINGLE_BLOCK */
         sd->data[sd->data_offset ++] = value;
@@ -1908,7 +1910,8 @@ uint8_t sd_read_data(SDState *sd)
 
     io_len = (sd->ocr & (1 << 30)) ? 512 : sd->blk_len;
 
-    trace_sdcard_read_data(sd->current_cmd, io_len);
+    trace_sdcard_read_data(sd_acmd_name(sd->current_cmd),
+                           sd->current_cmd, io_len);
     switch (sd->current_cmd) {
     case 6:	/* CMD6:   SWITCH_FUNCTION */
         ret = sd->data[sd->data_offset ++];
diff --git a/hw/sd/sdmmc-internal.c b/hw/sd/sdmmc-internal.c
new file mode 100644
index 0000000000..2053def3f1
--- /dev/null
+++ b/hw/sd/sdmmc-internal.c
@@ -0,0 +1,72 @@
+/*
+ * SD/MMC cards common helpers
+ *
+ * Copyright (c) 2018  Philippe Mathieu-Daudé <f4bug@amsat.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "sdmmc-internal.h"
+
+const char *sd_cmd_name(uint8_t cmd)
+{
+    static const char *cmd_abbrev[SDMMC_CMD_MAX] = {
+         [0]    = "GO_IDLE_STATE",
+         [2]    = "ALL_SEND_CID",            [3]    = "SEND_RELATIVE_ADDR",
+         [4]    = "SET_DSR",                 [5]    = "IO_SEND_OP_COND",
+         [6]    = "SWITCH_FUNC",             [7]    = "SELECT/DESELECT_CARD",
+         [8]    = "SEND_IF_COND",            [9]    = "SEND_CSD",
+        [10]    = "SEND_CID",               [11]    = "VOLTAGE_SWITCH",
+        [12]    = "STOP_TRANSMISSION",      [13]    = "SEND_STATUS",
+                                            [15]    = "GO_INACTIVE_STATE",
+        [16]    = "SET_BLOCKLEN",           [17]    = "READ_SINGLE_BLOCK",
+        [18]    = "READ_MULTIPLE_BLOCK",    [19]    = "SEND_TUNING_BLOCK",
+        [20]    = "SPEED_CLASS_CONTROL",    [21]    = "DPS_spec",
+                                            [23]    = "SET_BLOCK_COUNT",
+        [24]    = "WRITE_BLOCK",            [25]    = "WRITE_MULTIPLE_BLOCK",
+        [26]    = "MANUF_RSVD",             [27]    = "PROGRAM_CSD",
+        [28]    = "SET_WRITE_PROT",         [29]    = "CLR_WRITE_PROT",
+        [30]    = "SEND_WRITE_PROT",
+        [32]    = "ERASE_WR_BLK_START",     [33]    = "ERASE_WR_BLK_END",
+        [34]    = "SW_FUNC_RSVD",           [35]    = "SW_FUNC_RSVD",
+        [36]    = "SW_FUNC_RSVD",           [37]    = "SW_FUNC_RSVD",
+        [38]    = "ERASE",
+        [40]    = "DPS_spec",
+        [42]    = "LOCK_UNLOCK",            [43]    = "Q_MANAGEMENT",
+        [44]    = "Q_TASK_INFO_A",          [45]    = "Q_TASK_INFO_B",
+        [46]    = "Q_RD_TASK",              [47]    = "Q_WR_TASK",
+        [48]    = "READ_EXTR_SINGLE",       [49]    = "WRITE_EXTR_SINGLE",
+        [50]    = "SW_FUNC_RSVD",
+        [52]    = "IO_RW_DIRECT",           [53]    = "IO_RW_EXTENDED",
+        [54]    = "SDIO_RSVD",              [55]    = "APP_CMD",
+        [56]    = "GEN_CMD",                [57]    = "SW_FUNC_RSVD",
+        [58]    = "READ_EXTR_MULTI",        [59]    = "WRITE_EXTR_MULTI",
+        [60]    = "MANUF_RSVD",             [61]    = "MANUF_RSVD",
+        [62]    = "MANUF_RSVD",             [63]    = "MANUF_RSVD",
+    };
+    return cmd_abbrev[cmd] ? cmd_abbrev[cmd] : "UNKNOWN_CMD";
+}
+
+const char *sd_acmd_name(uint8_t cmd)
+{
+    static const char *acmd_abbrev[SDMMC_CMD_MAX] = {
+         [6] = "SET_BUS_WIDTH",
+        [13] = "SD_STATUS",
+        [14] = "DPS_spec",                  [15] = "DPS_spec",
+        [16] = "DPS_spec",
+        [18] = "SECU_spec",
+        [22] = "SEND_NUM_WR_BLOCKS",        [23] = "SET_WR_BLK_ERASE_COUNT",
+        [41] = "SD_SEND_OP_COND",
+        [42] = "SET_CLR_CARD_DETECT",
+        [51] = "SEND_SCR",
+        [52] = "SECU_spec",                 [53] = "SECU_spec",
+        [54] = "SECU_spec",
+        [56] = "SECU_spec",                 [57] = "SECU_spec",
+        [58] = "SECU_spec",                 [59] = "SECU_spec",
+    };
+
+    return acmd_abbrev[cmd] ? acmd_abbrev[cmd] : "UNKNOWN_ACMD";
+}
diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
index c2b7664264..a99d9fbb04 100644
--- a/hw/sd/Makefile.objs
+++ b/hw/sd/Makefile.objs
@@ -1,6 +1,6 @@
 common-obj-$(CONFIG_PL181) += pl181.o
 common-obj-$(CONFIG_SSI_SD) += ssi-sd.o
-common-obj-$(CONFIG_SD) += sd.o core.o
+common-obj-$(CONFIG_SD) += sd.o core.o sdmmc-internal.o
 common-obj-$(CONFIG_SDHCI) += sdhci.o
 
 obj-$(CONFIG_MILKYMIST) += milkymist-memcard.o
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 3040d32560..cdddca3dbf 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -24,8 +24,8 @@ sdhci_write_dataport(uint16_t data_count) "write buffer filled with %u bytes of
 sdhci_capareg(const char *desc, uint16_t val) "%s: %u"
 
 # hw/sd/sd.c
-sdcard_normal_command(uint8_t cmd, uint32_t arg, const char *state) "CMD%d arg 0x%08x (state %s)"
-sdcard_app_command(uint8_t acmd, uint32_t arg) "ACMD%d arg 0x%08x"
+sdcard_normal_command(const char *cmd_desc, uint8_t cmd, uint32_t arg, const char *state) "%20s/ CMD%02d arg 0x%08x (state %s)"
+sdcard_app_command(const char *acmd_desc, uint8_t acmd, uint32_t arg, const char *state) "%23s/ACMD%02d arg 0x%08x (state %s)"
 sdcard_response(const char *rspdesc, int rsplen) "%s (sz:%d)"
 sdcard_powerup(void) ""
 sdcard_inquiry_cmd41(void) ""
@@ -39,8 +39,8 @@ sdcard_lock(void) ""
 sdcard_unlock(void) ""
 sdcard_read_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
 sdcard_write_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
-sdcard_write_data(uint8_t cmd, uint8_t value) "CMD%02d value 0x%02x"
-sdcard_read_data(uint8_t cmd, int length) "CMD%02d len %d"
+sdcard_write_data(const char *cmd_desc, uint8_t cmd, uint8_t value) "%20s/ CMD%02d value 0x%02x"
+sdcard_read_data(const char *cmd_desc, uint8_t cmd, int length) "%20s/ CMD%02d len %d"
 sdcard_set_voltage(uint16_t millivolts) "%u mV"
 
 # hw/sd/milkymist-memcard.c
-- 
2.16.2

[Qemu-devel] [PATCH 3/8] sdcard: Display which protocol is used when tracing (SD or SPI)

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/sd.c         | 14 ++++++++++----
 hw/sd/trace-events |  8 ++++----
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index bb149aa644..dc50d6bbf7 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -120,6 +120,7 @@ struct SDState {
     qemu_irq readonly_cb;
     qemu_irq inserted_cb;
     QEMUTimer *ocr_power_timer;
+    const char *proto_name;
     bool enable;
     uint8_t dat_lines;
     bool cmd_line;
@@ -875,7 +876,8 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
      * However there is no ACMD55, so we want to trace this particular case.
      */
     if (req.cmd != 55 || sd->expecting_acmd) {
-        trace_sdcard_normal_command(sd_cmd_name(req.cmd), req.cmd,
+        trace_sdcard_normal_command(sd->proto_name,
+                                    sd_cmd_name(req.cmd), req.cmd,
                                     req.arg, sd_state_name(sd->state));
     }
 
@@ -1455,7 +1457,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 static sd_rsp_type_t sd_app_command(SDState *sd,
                                     SDRequest req)
 {
-    trace_sdcard_app_command(sd_acmd_name(req.cmd),
+    trace_sdcard_app_command(sd->proto_name, sd_acmd_name(req.cmd),
                              req.cmd, req.arg, sd_state_name(sd->state));
     sd->card_status |= APP_CMD;
     switch (req.cmd) {
@@ -1771,7 +1773,8 @@ void sd_write_data(SDState *sd, uint8_t value)
     if (sd->card_status & (ADDRESS_ERROR | WP_VIOLATION))
         return;
 
-    trace_sdcard_write_data(sd_acmd_name(sd->current_cmd),
+    trace_sdcard_write_data(sd->proto_name,
+                            sd_acmd_name(sd->current_cmd),
                             sd->current_cmd, value);
     switch (sd->current_cmd) {
     case 24:	/* CMD24:  WRITE_SINGLE_BLOCK */
@@ -1910,7 +1913,8 @@ uint8_t sd_read_data(SDState *sd)
 
     io_len = (sd->ocr & (1 << 30)) ? 512 : sd->blk_len;
 
-    trace_sdcard_read_data(sd_acmd_name(sd->current_cmd),
+    trace_sdcard_read_data(sd->proto_name,
+                           sd_acmd_name(sd->current_cmd),
                            sd->current_cmd, io_len);
     switch (sd->current_cmd) {
     case 6:	/* CMD6:   SWITCH_FUNCTION */
@@ -2037,6 +2041,8 @@ static void sd_realize(DeviceState *dev, Error **errp)
     SDState *sd = SD_CARD(dev);
     int ret;
 
+    sd->proto_name = sd->spi ? "SPI" : "SD";
+
     if (sd->blk && blk_is_read_only(sd->blk)) {
         error_setg(errp, "Cannot use read-only drive as SD card");
         return;
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index cdddca3dbf..2059ace61f 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -24,8 +24,8 @@ sdhci_write_dataport(uint16_t data_count) "write buffer filled with %u bytes of
 sdhci_capareg(const char *desc, uint16_t val) "%s: %u"
 
 # hw/sd/sd.c
-sdcard_normal_command(const char *cmd_desc, uint8_t cmd, uint32_t arg, const char *state) "%20s/ CMD%02d arg 0x%08x (state %s)"
-sdcard_app_command(const char *acmd_desc, uint8_t acmd, uint32_t arg, const char *state) "%23s/ACMD%02d arg 0x%08x (state %s)"
+sdcard_normal_command(const char *proto, const char *cmd_desc, uint8_t cmd, uint32_t arg, const char *state) "%s %20s/ CMD%02d arg 0x%08x (state %s)"
+sdcard_app_command(const char *proto, const char *acmd_desc, uint8_t acmd, uint32_t arg, const char *state) "%s %23s/ACMD%02d arg 0x%08x (state %s)"
 sdcard_response(const char *rspdesc, int rsplen) "%s (sz:%d)"
 sdcard_powerup(void) ""
 sdcard_inquiry_cmd41(void) ""
@@ -39,8 +39,8 @@ sdcard_lock(void) ""
 sdcard_unlock(void) ""
 sdcard_read_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
 sdcard_write_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
-sdcard_write_data(const char *cmd_desc, uint8_t cmd, uint8_t value) "%20s/ CMD%02d value 0x%02x"
-sdcard_read_data(const char *cmd_desc, uint8_t cmd, int length) "%20s/ CMD%02d len %d"
+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"
 
 # hw/sd/milkymist-memcard.c
-- 
2.16.2

[Qemu-devel] [PATCH 4/8] sdcard: Add the Tuning Command (CMD19)

[Philippe Mathieu-Daudé]

>From the "Physical Layer Simplified Specification Version 3.01":

  A known data block ("Tuning block") can be used to tune sampling
  point for tuning required hosts. [...]
  This procedure gives the system optimal timing for each specific
  host and card combination and compensates for static delays in
  the timing budget including process, voltage and different PCB
  loads and skews. [...]
  Data block, carried by DAT[3:0], contains a pattern for tuning
  sampling position to receive data on the CMD and DAT[3:0] line.

[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 | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index dc50d6bbf7..235e0518d6 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1169,6 +1169,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:
@@ -1893,6 +1901,20 @@ void sd_write_data(SDState *sd, uint8_t value)
     }
 }
 
+#define SD_TUNING_BLOCK_SIZE    64
+
+static const uint8_t sd_tuning_block_pattern[SD_TUNING_BLOCK_SIZE] = {
+    /* See: Physical Layer Simplified Specification Version 3.01, Table 4-2 */
+    0xff, 0x0f, 0xff, 0x00,         0x0f, 0xfc, 0xc3, 0xcc,
+    0xc3, 0x3c, 0xcc, 0xff,         0xfe, 0xff, 0xfe, 0xef,
+    0xff, 0xdf, 0xff, 0xdd,         0xff, 0xfb, 0xff, 0xfb,
+    0xbf, 0xff, 0x7f, 0xff,         0x77, 0xf7, 0xbd, 0xef,
+    0xff, 0xf0, 0xff, 0xf0,         0x0f, 0xfc, 0xcc, 0x3c,
+    0xcc, 0x33, 0xcc, 0xcf,         0xff, 0xef, 0xff, 0xee,
+    0xff, 0xfd, 0xff, 0xfd,         0xdf, 0xff, 0xbf, 0xff,
+    0xbb, 0xff, 0xf7, 0xff,         0xf7, 0x7f, 0x7b, 0xde,
+};
+
 uint8_t sd_read_data(SDState *sd)
 {
     /* TODO: Append CRCs */
@@ -1972,6 +1994,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 = sd_tuning_block_pattern[sd->data_offset++];
+        break;
+
     case 22:	/* ACMD22: SEND_NUM_WR_BLOCKS */
         ret = sd->data[sd->data_offset ++];
 
-- 
2.16.2

[Qemu-devel] [PATCH 5/8] 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 235e0518d6..b907d62aef 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.2

[Qemu-devel] [PATCH 6/8] 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>
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 b907d62aef..611094447c 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_mode;
 
     uint32_t mode;    /* current card mode, one of SDCardModes */
     int32_t state;    /* current card state, one of SDCardStates */
@@ -292,6 +293,9 @@ 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_mode != UHS_NOT_SUPPORTED);
 }
 
 static void sd_ocr_powerup(void *opaque)
@@ -2198,6 +2202,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_mode, UHS_NOT_SUPPORTED),
     DEFINE_PROP_END_OF_LIST()
 };
 
-- 
2.16.2

[Qemu-devel] [PATCH 7/8] sdhci: Fix a typo in comment

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 97b4a473c8..1b828b104d 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -433,13 +433,13 @@ static void sdhci_read_block_from_card(SDHCIState *s)
     for (index = 0; index < blk_size; index++) {
         data = sdbus_read_data(&s->sdbus);
         if (!FIELD_EX32(s->hostctl2, SDHC_HOSTCTL2, EXECUTE_TUNING)) {
-            /* Device is not in tunning */
+            /* Device is not in tuning */
             s->fifo_buffer[index] = data;
         }
     }
 
     if (FIELD_EX32(s->hostctl2, SDHC_HOSTCTL2, EXECUTE_TUNING)) {
-        /* Device is in tunning */
+        /* Device is in tuning */
         s->hostctl2 &= ~R_SDHC_HOSTCTL2_EXECUTE_TUNING_MASK;
         s->hostctl2 |= R_SDHC_HOSTCTL2_SAMPLING_CLKSEL_MASK;
         s->prnsts &= ~(SDHC_DAT_LINE_ACTIVE | SDHC_DOING_READ |
-- 
2.16.2

[Qemu-devel] [PATCH 8/8] MAINTAINERS: Add entries for SD (SDHCI, SDBus, SDCard)

[Philippe Mathieu-Daudé]

After spending months studying all the different SD Specifications
from the SD Association, voluntarily add myself as maintainer
for the SD code.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index b7c4130388..0446724642 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1089,6 +1089,14 @@ M: Peter Crosthwaite <crosthwaite.peter@gmail.com>
 S: Maintained
 F: hw/ssi/xilinx_*
 
+SD (Secure Card)
+M: Philippe Mathieu-Daudé <f4bug@amsat.org>
+S: Odd Fixes
+F: include/hw/sd/sd*
+F: hw/sd/core.c
+F: hw/sd/sd*
+F: tests/sd*
+
 USB
 M: Gerd Hoffmann <kraxel@redhat.com>
 S: Maintained
-- 
2.16.2

Re: [Qemu-devel] [PATCH 8/8] MAINTAINERS: Add entries for SD (SDHCI, SDBus, SDCard)

[Peter Maydell]

On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> After spending months studying all the different SD Specifications
> from the SD Association, voluntarily add myself as maintainer
> for the SD code.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  MAINTAINERS | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b7c4130388..0446724642 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1089,6 +1089,14 @@ M: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>  S: Maintained
>  F: hw/ssi/xilinx_*
>
> +SD (Secure Card)
> +M: Philippe Mathieu-Daudé <f4bug@amsat.org>
> +S: Odd Fixes
> +F: include/hw/sd/sd*
> +F: hw/sd/core.c
> +F: hw/sd/sd*
> +F: tests/sd*
> +
>  USB
>  M: Gerd Hoffmann <kraxel@redhat.com>
>  S: Maintained
> --
> 2.16.2

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 7/8] sdhci: Fix a typo in comment

[Peter Maydell]

On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sdhci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 97b4a473c8..1b828b104d 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -433,13 +433,13 @@ static void sdhci_read_block_from_card(SDHCIState *s)
>      for (index = 0; index < blk_size; index++) {
>          data = sdbus_read_data(&s->sdbus);
>          if (!FIELD_EX32(s->hostctl2, SDHC_HOSTCTL2, EXECUTE_TUNING)) {
> -            /* Device is not in tunning */
> +            /* Device is not in tuning */
>              s->fifo_buffer[index] = data;
>          }
>      }
>
>      if (FIELD_EX32(s->hostctl2, SDHC_HOSTCTL2, EXECUTE_TUNING)) {
> -        /* Device is in tunning */
> +        /* Device is in tuning */
>          s->hostctl2 &= ~R_SDHC_HOSTCTL2_EXECUTE_TUNING_MASK;
>          s->hostctl2 |= R_SDHC_HOSTCTL2_SAMPLING_CLKSEL_MASK;
>          s->prnsts &= ~(SDHC_DAT_LINE_ACTIVE | SDHC_DOING_READ |
> --
> 2.16.2

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/8] sdcard: Do not trace CMD55, except when we already expect an ACMD

[Peter Maydell]

On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Acked-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>  hw/sd/sd.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 933890e86f..4a9520e38e 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -866,13 +866,18 @@ static void sd_lock_command(SDState *sd)
>          sd->card_status &= ~CARD_IS_LOCKED;
>  }
>
> -static sd_rsp_type_t sd_normal_command(SDState *sd,
> -                                       SDRequest req)
> +static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>  {
>      uint32_t rca = 0x0000;
>      uint64_t addr = (sd->ocr & (1 << 30)) ? (uint64_t) req.arg << 9 : req.arg;
>
> -    trace_sdcard_normal_command(req.cmd, req.arg, sd_state_name(sd->state));
> +    /* CMD55 precedes an ACMD, so we are not interested in tracing it.
> +     * However there is no ACMD55, so we want to trace this particular case.
> +     */
> +    if (req.cmd != 55 || sd->expecting_acmd) {
> +        trace_sdcard_normal_command(req.cmd, req.arg,
> +                                    sd_state_name(sd->state));
> +    }
>
>      /* Not interpreting this as an app command */
>      sd->card_status &= ~APP_CMD;
> --
> 2.16.2

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/8] sdcard: Display command name when tracing CMD/ACMD

[Peter Maydell]

On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> The SDBus will reuse these functions, so we put them in a new source file.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sdmmc-internal.h | 20 ++++++++++++++
>  hw/sd/sd.c             | 13 +++++----
>  hw/sd/sdmmc-internal.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/sd/Makefile.objs    |  2 +-
>  hw/sd/trace-events     |  8 +++---
>  5 files changed, 105 insertions(+), 10 deletions(-)
>  create mode 100644 hw/sd/sdmmc-internal.c
>
> diff --git a/hw/sd/sdmmc-internal.h b/hw/sd/sdmmc-internal.h
> index 0e96cb0081..bf6fb06128 100644
> --- a/hw/sd/sdmmc-internal.h
> +++ b/hw/sd/sdmmc-internal.h
> @@ -12,4 +12,24 @@
>
>  #define SDMMC_CMD_MAX 64
>
> +/**
> + * sd_cmd_name:
> + * @cmd: A SD "normal" command, upto SDMMC_CMD_MAX.
> + *
> + * Returns an human useful name describing the command.
> + *
> + * Returns: The command name of @cmd or "UNKNOWN_CMD".
> + */
> +const char *sd_cmd_name(uint8_t cmd);
> +
> +/**
> + * sd_acmd_name:
> + * @cmd: A SD "Application-Specific" command, upto SDMMC_CMD_MAX.
> + *
> + * Returns an human useful name describing the application command.
> + *
> + * Returns: The application command name of @cmd or "UNKNOWN_ACMD".
> + */
> +const char *sd_acmd_name(uint8_t cmd);
> +

"up to", and "a human-readable name". Otherwise:

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/8] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)

[Peter Maydell]

On 9 March 2018 at 15:36, 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>

This should ideally have a Signed-off-by: from somebody @xilinx.com as
well as you, then.

> ---
>  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 235e0518d6..b907d62aef 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
> +};

Since the minimum isn't 0, this means SD_FG_COUNT isn't
actually the count of the number of FGs. I think having
    SD_FG_MAX = 6,

would make the loops below clearer, since they become
  for (fn_grp = SD_FG_MAX; fn_grp >= SD_FG_MIN; fn_grp--) {

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

Previously we were writing 0x00, 0x01 to the first 2 bytes of data;
now we will write 0x01, 0x00. Are you sure that's right ? I guess
it's the difference between claiming 1mA and 256mA.
(I can't make any sense of the table in the spec so I have no idea.)

Also the spec says that if the guest selects an invalid function then
this value should be 0.

> +
> +    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) {

/* Guest requested no influence, so this function group stays the same */

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

"not 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;

I think the spec says that if the guest makes an invalid selection
for one function in the group then we must ignore all the set values,
not just the one that was wrong, so we need to check everything
first before we start writing the new values back.

> +                }
> +            }
> +            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.2
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/8] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)

[Peter Maydell]

On 9 March 2018 at 15:36, 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>
> ---
>  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 235e0518d6..b907d62aef 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;

Oh, and what's the difference between s->uhs_enabled = false
(this patch) and s->uhs_mode = UHS_NOT_SUPPORTED (next patch) ?

Do we need both? If so, a comment noting the difference would help
people to know which one various bits of code should be checking.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/8] SDCard: improve tracing, support UHS-I

[Peter Maydell]

On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> This series contains the leftover patches from these 2 series:
> - SDCard: housekeeping, add tracing (part 4)
> - SDCard: bugfixes, support UHS-I (part 5)
> with Peter Maydell comments addressed.
>
> I misunderstood the 2.12 train deadline, but better to have the series
> standing in the list rather than forgot it.
>
> Philippe Mathieu-Daudé (8):
>   sdcard: Do not trace CMD55, except when we already expect an ACMD
>   sdcard: Display command name when tracing CMD/ACMD
>   sdcard: Display which protocol is used when tracing (SD or SPI)
>   sdcard: Add the Tuning Command (CMD19)
>   sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)
>   sdcard: Add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit
>   sdhci: Fix a typo in comment
>   MAINTAINERS: Add entries for SD (SDHCI, SDBus, SDCard)

Thanks. I've applied patches 1-4, 7 and 8 to target-arm.next, so
they will make 2.12. Patches 5 and 6 I had review comments on.

(We may be able to call 5/6 bugfix patches and get them into 2.12;
depends on timing.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/8] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)

[Edgar E. Iglesias]

On Fri, Mar 09, 2018 at 05:03:11PM +0000, Peter Maydell wrote:
> On 9 March 2018 at 15:36, 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>
> 
> This should ideally have a Signed-off-by: from somebody @xilinx.com as
> well as you, then.

Hi Philippe,

Feel free to add my SoB on the next spin of this:
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Cheers,
Edgar


> 
> > ---
> >  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 235e0518d6..b907d62aef 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
> > +};
> 
> Since the minimum isn't 0, this means SD_FG_COUNT isn't
> actually the count of the number of FGs. I think having
>     SD_FG_MAX = 6,
> 
> would make the loops below clearer, since they become
>   for (fn_grp = SD_FG_MAX; fn_grp >= SD_FG_MIN; fn_grp--) {
> 
> > +
> > +/* 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 */
> 
> Previously we were writing 0x00, 0x01 to the first 2 bytes of data;
> now we will write 0x01, 0x00. Are you sure that's right ? I guess
> it's the difference between claiming 1mA and 256mA.
> (I can't make any sense of the table in the spec so I have no idea.)
> 
> Also the spec says that if the guest selects an invalid function then
> this value should be 0.
> 
> > +
> > +    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) {
> 
> /* Guest requested no influence, so this function group stays the same */
> 
> > +            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 "
> 
> "not 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;
> 
> I think the spec says that if the guest makes an invalid selection
> for one function in the group then we must ignore all the set values,
> not just the one that was wrong, so we need to check everything
> first before we start writing the new values back.
> 
> > +                }
> > +            }
> > +            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.2
> >
> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 0/8] SDCard: improve tracing, support UHS-I

[Philippe Mathieu-Daudé]

On 03/09/2018 06:08 PM, Peter Maydell wrote:
> On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> This series contains the leftover patches from these 2 series:
>> - SDCard: housekeeping, add tracing (part 4)
>> - SDCard: bugfixes, support UHS-I (part 5)
>> with Peter Maydell comments addressed.
>>
>> I misunderstood the 2.12 train deadline, but better to have the series
>> standing in the list rather than forgot it.
>>
>> Philippe Mathieu-Daudé (8):
>>   sdcard: Do not trace CMD55, except when we already expect an ACMD
>>   sdcard: Display command name when tracing CMD/ACMD
>>   sdcard: Display which protocol is used when tracing (SD or SPI)
>>   sdcard: Add the Tuning Command (CMD19)
>>   sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)
>>   sdcard: Add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit
>>   sdhci: Fix a typo in comment
>>   MAINTAINERS: Add entries for SD (SDHCI, SDBus, SDCard)
> 
> Thanks. I've applied patches 1-4, 7 and 8 to target-arm.next, so
> they will make 2.12. Patches 5 and 6 I had review comments on.

Thanks \o/

> 
> (We may be able to call 5/6 bugfix patches and get them into 2.12;
> depends on timing.)

I'll do my best, the "misc" pull didn't hit the list yet :)

Phil.

Re: [Qemu-devel] [PATCH 5/8] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)

[Philippe Mathieu-Daudé]

Hi Peter, Edgar.

On 03/09/2018 06:03 PM, Peter Maydell wrote:
> On 9 March 2018 at 15:36, 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>
> 
> This should ideally have a Signed-off-by: from somebody @xilinx.com as
> well as you, then.

The original commit is:

https://github.com/Xilinx/qemu/commit/02d2f0203dd#diff-4875140dbdae36a175ad74d6af9bc342R607

  commit 02d2f0203dd489ed30d9c8d90c14a52c57332b25 (tag: xilinx-v2015.2)
  Merge: aa351061db 732f348311
  Author: Alistair Francis <alistair.francis@xilinx.com>
  Date:   Thu Jul 9 14:32:46 2015 -0700

      PetaLinux QEMU v2015.2

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

As you can see the changes are:

- change magic by definitions (SD_FG_MIN, SD_FN_COUNT, SD_FN_COUNT),
- used stw_be_p() in 2 places,
- added tracing.

So no logical change.

Is that enough to add Alistair S-o-b?

Thanks,

Phil.

Re: [Qemu-devel] [PATCH 5/8] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)

[Philippe Mathieu-Daudé]

On 03/09/2018 06:03 PM, Peter Maydell wrote:
> On 9 March 2018 at 15:36, 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>
> 
> This should ideally have a Signed-off-by: from somebody @xilinx.com as
> well as you, then.
> 
>> ---
>>  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 235e0518d6..b907d62aef 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
>> +};
> 
> Since the minimum isn't 0, this means SD_FG_COUNT isn't
> actually the count of the number of FGs. I think having
>     SD_FG_MAX = 6,
> 
> would make the loops below clearer, since they become
>   for (fn_grp = SD_FG_MAX; fn_grp >= SD_FG_MIN; fn_grp--) {
> 
>> +
>> +/* 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 */
> 
> Previously we were writing 0x00, 0x01 to the first 2 bytes of data;
> now we will write 0x01, 0x00. Are you sure that's right ? I guess
> it's the difference between claiming 1mA and 256mA.
> (I can't make any sense of the table in the spec so I have no idea.)

Good catch. I'm not sure which default value we want here, I doubt 256
mA matches the card used, but the hw tests pass so I'll keep it.
We might change it to a property later.

> 
> Also the spec says that if the guest selects an invalid function then
> this value should be 0.
> 
>> +
>> +    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) {
> 
> /* Guest requested no influence, so this function group stays the same */
> 
>> +            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 "
> 
> "not 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;
> 
> I think the spec says that if the guest makes an invalid selection
> for one function in the group then we must ignore all the set values,
> not just the one that was wrong, so we need to check everything
> first before we start writing the new values back.

Yes, we missed that.

> 
>> +                }
>> +            }
>> +            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.2
>>
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH 5/8] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)

[Philippe Mathieu-Daudé]

On 03/09/2018 06:06 PM, Peter Maydell wrote:
> On 9 March 2018 at 15:36, 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>
>> ---
>>  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 235e0518d6..b907d62aef 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;
> 
> Oh, and what's the difference between s->uhs_enabled = false
> (this patch) and s->uhs_mode = UHS_NOT_SUPPORTED (next patch) ?
> 
> Do we need both? If so, a comment noting the difference would help
> people to know which one various bits of code should be checking.

Ok.

The 'uhs_mode' is a read-only property before realize().
Now if the sdcard supports any UHS, the host may negotiate to switch to
UHS mode.
To enter this mode with voltage level adjusted, the card needs to
'soft-reset' itself in the new mode. We use 'uhs_enabled' to track this
runtime state. Maybe 'uhs_active' is more explicit?
I intended to keep the properties vs runtime fields separated in the
SDState.

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH 5/8] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)

[Philippe Mathieu-Daudé]

On 03/09/2018 06:03 PM, Peter Maydell wrote:
> On 9 March 2018 at 15:36, 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>
> 
> This should ideally have a Signed-off-by: from somebody @xilinx.com as
> well as you, then.
> 
>> ---
>>  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 235e0518d6..b907d62aef 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
>> +};
> 
> Since the minimum isn't 0, this means SD_FG_COUNT isn't
> actually the count of the number of FGs. I think having
>     SD_FG_MAX = 6,
> 
> would make the loops below clearer, since they become
>   for (fn_grp = SD_FG_MAX; fn_grp >= SD_FG_MIN; fn_grp--) {
> 
>> +
>> +/* 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 */
> 
> Previously we were writing 0x00, 0x01 to the first 2 bytes of data;
> now we will write 0x01, 0x00. Are you sure that's right ? I guess
> it's the difference between claiming 1mA and 256mA.
> (I can't make any sense of the table in the spec so I have no idea.)

Good catch. I'm not sure which default value we want here, I doubt 256
mA matches the card used, but the hw tests pass so I'll keep it.
We might change it to a property later.

> 
> Also the spec says that if the guest selects an invalid function then
> this value should be 0.
> 
>> +
>> +    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) {
> 
> /* Guest requested no influence, so this function group stays the same */
> 
>> +            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 "
> 
> "not 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;
> 
> I think the spec says that if the guest makes an invalid selection
> for one function in the group then we must ignore all the set values,
> not just the one that was wrong, so we need to check everything
> first before we start writing the new values back.

Yes, we missed that.

> 
>> +                }
>> +            }
>> +            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.2
>>
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH 5/8] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)

[Philippe Mathieu-Daudé]

Hi Peter,

On 03/09/2018 06:03 PM, Peter Maydell wrote:
> On 9 March 2018 at 15:36, 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>
> 
> This should ideally have a Signed-off-by: from somebody @xilinx.com as
> well as you, then.
> 
>> ---
>>  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 235e0518d6..b907d62aef 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
>> +};
> 
> Since the minimum isn't 0, this means SD_FG_COUNT isn't
> actually the count of the number of FGs. I think having
>     SD_FG_MAX = 6,
> 
> would make the loops below clearer, since they become
>   for (fn_grp = SD_FG_MAX; fn_grp >= SD_FG_MIN; fn_grp--) {
> 
>> +
>> +/* 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 */
> 
> Previously we were writing 0x00, 0x01 to the first 2 bytes of data;
> now we will write 0x01, 0x00. Are you sure that's right ? I guess
> it's the difference between claiming 1mA and 256mA.
> (I can't make any sense of the table in the spec so I have no idea.)
> 
> Also the spec says that if the guest selects an invalid function then
> this value should be 0.
> 
>> +
>> +    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) {
> 
> /* Guest requested no influence, so this function group stays the same */
> 
>> +            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 "
> 
> "not 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;
> 
> I think the spec says that if the guest makes an invalid selection
> for one function in the group then we must ignore all the set values,

... for the current group? ...

> not just the one that was wrong, so we need to check everything
> first before we start writing the new values back.

I'm following the "Physical Layer Simplified Specification Version 3.01".

  4.3.10.3 Mode 1 Operation - Set Function

  Switching to a new functionality is done by:
  • When a function cannot be switched because it is busy,
    the card returns the current function number (not returns 0xF),
    the other functions in the other groups may still be switched.

  In response to a set function, the switch function will return ...
  • The function that is the result of the switch command. In case
    of invalid selection of one function or more, all set values
    are ignored and no change will be done (identical to the case
    where the host selects 0xF for all functions groups). The
    response to an invalid selection of function will be 0xF.

I'm not sure how to interpret this paragraph, I understand it as:
"all set values are ignored [in the current group]" but this is
confusing because of the "identical to ... all functions groups".

> 
>> +                }
>> +            }
>> +            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.2
>>
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH 5/8] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)

[Peter Maydell]

On 12 March 2018 at 12:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 03/09/2018 06:06 PM, Peter Maydell wrote:
>> On 9 March 2018 at 15:36, 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>
>>> ---
>>>  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 235e0518d6..b907d62aef 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;
>>
>> Oh, and what's the difference between s->uhs_enabled = false
>> (this patch) and s->uhs_mode = UHS_NOT_SUPPORTED (next patch) ?
>>
>> Do we need both? If so, a comment noting the difference would help
>> people to know which one various bits of code should be checking.
>
> Ok.
>
> The 'uhs_mode' is a read-only property before realize().
> Now if the sdcard supports any UHS, the host may negotiate to switch to
> UHS mode.
> To enter this mode with voltage level adjusted, the card needs to
> 'soft-reset' itself in the new mode. We use 'uhs_enabled' to track this
> runtime state. Maybe 'uhs_active' is more explicit?
> I intended to keep the properties vs runtime fields separated in the
> SDState.

Yes, static property vs runtime changeable should indeed be kept
separate -- we should just make sure it's clear which is which,
with a suitable comment and/or field name.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/8] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)

[Peter Maydell]

On 12 March 2018 at 12:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 03/09/2018 06:03 PM, Peter Maydell wrote:
>> Previously we were writing 0x00, 0x01 to the first 2 bytes of data;
>> now we will write 0x01, 0x00. Are you sure that's right ? I guess
>> it's the difference between claiming 1mA and 256mA.
>> (I can't make any sense of the table in the spec so I have no idea.)
>
> Good catch. I'm not sure which default value we want here, I doubt 256
> mA matches the card used, but the hw tests pass so I'll keep it.
> We might change it to a property later.

Do the tests fail if we report 1mA ? If not, then I'd prefer us
to keep the current behaviour. (QEMU's implementation obviously
has no current draw limits, so reporting the lowest possible value
means we won't ever accidentally cause the guest to think it
can't do something because the current requirement is too high.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/8] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)

[Peter Maydell]

On 12 March 2018 at 13:03, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 03/09/2018 06:03 PM, Peter Maydell wrote:
>> I think the spec says that if the guest makes an invalid selection
>> for one function in the group then we must ignore all the set values,
>
> ... for the current group? ...
>
>> not just the one that was wrong, so we need to check everything
>> first before we start writing the new values back.
>
> I'm following the "Physical Layer Simplified Specification Version 3.01".
>
>   4.3.10.3 Mode 1 Operation - Set Function
>
>   Switching to a new functionality is done by:
>   • When a function cannot be switched because it is busy,
>     the card returns the current function number (not returns 0xF),
>     the other functions in the other groups may still be switched.
>
>   In response to a set function, the switch function will return ...
>   • The function that is the result of the switch command. In case
>     of invalid selection of one function or more, all set values
>     are ignored and no change will be done (identical to the case
>     where the host selects 0xF for all functions groups). The
>     response to an invalid selection of function will be 0xF.
>
> I'm not sure how to interpret this paragraph, I understand it as:
> "all set values are ignored [in the current group]" but this is
> confusing because of the "identical to ... all functions groups".

The command only lets you specify one value function in each
group, so "all set values" must mean "the set values for every
group", I think, and the parenthesised text confirms that --
it should act as if the command specified 0xf for everything.
It's slightly less clear what exactly the response should be:
should it return 0xf for the groups where there was an invalid
selection, and <whatever the current value is> for the groups
where the selection request was ok, or just 0xf for everything ?
(This is probably most easily answered by testing the behaviour
of a real sd card I guess...)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/8] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)

[Philippe Mathieu-Daudé]

Hi Peter,

On 03/09/2018 02:03 PM, Peter Maydell wrote:
> On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
[...]
>>  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;
[...]
>> +
>> +    stw_be_p(sd->data + 0, 0x0001);     /* Maximum current consumption */
> 
> Previously we were writing 0x00, 0x01 to the first 2 bytes of data;
> now we will write 0x01, 0x00. Are you sure that's right ? I guess
> it's the difference between claiming 1mA and 256mA.

Using stw_be_p() we still write [0x00, 0x01] (16-bit big endian), is
this correct?

Re: [Qemu-devel] [PATCH 5/8] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)

[Philippe Mathieu-Daudé]

Hi Peter,

On 03/12/2018 10:16 AM, Peter Maydell wrote:
> On 12 March 2018 at 13:03, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 03/09/2018 06:03 PM, Peter Maydell wrote:
>>> I think the spec says that if the guest makes an invalid selection
>>> for one function in the group then we must ignore all the set values,
>>
>> ... for the current group? ...
>>
>>> not just the one that was wrong, so we need to check everything
>>> first before we start writing the new values back.
>>
>> I'm following the "Physical Layer Simplified Specification Version 3.01".
>>
>>   4.3.10.3 Mode 1 Operation - Set Function
>>
>>   Switching to a new functionality is done by:
>>   • When a function cannot be switched because it is busy,
>>     the card returns the current function number (not returns 0xF),
>>     the other functions in the other groups may still be switched.
>>
>>   In response to a set function, the switch function will return ...
>>   • The function that is the result of the switch command. In case
>>     of invalid selection of one function or more, all set values
>>     are ignored and no change will be done (identical to the case
>>     where the host selects 0xF for all functions groups). The
>>     response to an invalid selection of function will be 0xF.
>>
>> I'm not sure how to interpret this paragraph, I understand it as:
>> "all set values are ignored [in the current group]" but this is
>> confusing because of the "identical to ... all functions groups".
> 
> The command only lets you specify one value function in each
> group, so "all set values" must mean "the set values for every
> group", I think, and the parenthesised text confirms that --
> it should act as if the command specified 0xf for everything.
> It's slightly less clear what exactly the response should be:
> should it return 0xf for the groups where there was an invalid
> selection, and <whatever the current value is> for the groups
> where the selection request was ok, or just 0xf for everything ?
> (This is probably most easily answered by testing the behaviour
> of a real sd card I guess...)

Sorry to let you wait so long, it took me days to have full setup and
tests :/

Testing, the behavior is as you said:
"it return 0xf for the groups where there was an invalid  selection, and
<whatever the current value is> for the groups where the selection
request was ok"

>>> do_cmd(6, 0x00fffff0)
"00648001800180018001c00180010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"
0064 // Maximum current consumption: 64mA
8001 // Function group 6, information. If a bit i is set, function i is
supported. 0=default
8001 // 5: default
8001 // 4: default
8001 // 3: default
c001 // 2: 0 = default + 0xE = Vendor specific
8001 // 1: default
0 //6 The function which can be switched in function group 6. 0xF shows
function set error with the argument.
0 //5
0 //4
0 //3
0 //2
0 //1
00 // Data Structure Version: 00h – bits 511:376 are defined
00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
// undef

>>> do_cmd(6, 0x0)
// same as do_cmd(6, 0x00fffff0)

Let's try to set Current Limit: 400mA (function name 1 to group No 4,
arg slice [15:12]):

we need to use CMD6 argument:
(gdb) p/x 0x00fffff0 & ~(0xf << 12) | (1 << 12)
0xff1ff0

>>> do_cmd(6, 0x00ff1ff0)
"00008001800180018001c001800100f0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"
0000 // 0mA
8001 //6
8001 //5
8001 //4
8001 //3
c001 //2
8001 //1
0 //6
0 //5
f //function group 4 "0xF shows function set error with the argument."
0 //3
0 //2
0 //1
00 // v0
00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

Now, let's try to set Command system: Advanced Security SD (function
name 4 to group No 2, arg slice [7:4]):

(gdb) p/x 0x00fffff0 & ~(0xf << 4) | (2 << 4)
0xffff20

>>> do_cmd(6, 0x00ffff20)
"00008001800180018001c00180010000f00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"
0000 // 0mA
8001 //6
8001 //5
8001 //4
8001 //3
c001 //2
8001 //1
0 //6
0 //5
0 //4
0 //3
f //function group 2 "0xF shows function set error with the argument."
0 //1
00 // v0
00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

Finally those 2 incorrect functions at once:

(gdb) p/x 0x00fffff0 & ~(0xf << 12 | 0xf << 4) | (1 << 12) | (2 << 4)
0xff1f20

>>> do_cmd(6, 0xff1f20)
"00008001800180018001c001800100f0f00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"
...
0 //6
0 //5
f //function group 4: error with the argument
0 //3
f //function group 2: error with the argument
0 //1
00 // v0
...

Regards,

Phil.