[Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4)

[Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4)

[Philippe Mathieu-Daudé]

Since v3:
- use assert() in sd_state_name() and sd_response_name() (Alistair review)
- added sdmmc-internal.h & sdmmc-common.c to reuse helpers with hw/sd/core.c

Since v2:
- split again in 2... this part is cleanup/tracing
- add more tracepoints
- move some code reusable by sdbus in sdmmc-internal.h

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

$ git backport-diff
001/11:[----] [--] 'sdcard: reorder SDState struct members'
002/11:[0003] [FC] 'sdcard: replace DPRINTF() by trace events'
003/11:[0001] [FC] 'sdcard: add a trace event for command responses'
004/11:[0007] [FC] 'sdcard: replace fprintf() by qemu_hexdump()'
005/11:[----] [--] 'sdcard: add more trace events'
006/11:[----] [--] 'sdcard: do not trace CMD55 when expecting ACMD'
007/11:[0014] [FC] 'sdcard: define SDMMC_CMD_MAX instead of using the magic '64''
008/11:[0020] [FC] 'sdcard: display command name when tracing CMD/ACMD'
009/11:[----] [--] 'sdcard: display protocol used when tracing'
010/11:[----] [-C] 'sdcard: use G_BYTE from cutils'
011/11:[----] [-C] 'sdcard: use the registerfields API to access the OCR register'

Philippe Mathieu-Daudé (11):
  sdcard: reorder SDState struct members
  sdcard: replace DPRINTF() by trace events
  sdcard: add a trace event for command responses
  sdcard: replace fprintf() by qemu_hexdump()
  sdcard: add more trace events
  sdcard: do not trace CMD55 when expecting ACMD
  sdcard: define SDMMC_CMD_MAX instead of using the magic '64'
  sdcard: display command name when tracing CMD/ACMD
  sdcard: display protocol used when tracing
  sdcard: use G_BYTE from cutils
  sdcard: use the registerfields API to access the OCR register

 hw/sd/sdmmc-internal.h |  18 +++++
 include/hw/sd/sd.h     |   1 -
 hw/sd/sd.c             | 184 ++++++++++++++++++++++++++++++++++---------------
 hw/sd/sdmmc-common.c   |  72 +++++++++++++++++++
 hw/sd/Makefile.objs    |   2 +-
 hw/sd/trace-events     |  20 ++++++
 6 files changed, 241 insertions(+), 56 deletions(-)
 create mode 100644 hw/sd/sdmmc-internal.h
 create mode 100644 hw/sd/sdmmc-common.c

-- 
2.16.1

[Qemu-devel] [PATCH v4 01/11] sdcard: reorder SDState struct members

[Philippe Mathieu-Daudé]

place card registers first, this will ease further code movements.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 9ac9b63ff8..ce1f2fdf76 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -88,16 +88,21 @@ enum SDCardStates {
 struct SDState {
     DeviceState parent_obj;
 
-    uint32_t mode;    /* current card mode, one of SDCardModes */
-    int32_t state;    /* current card state, one of SDCardStates */
+    /* SD Memory Card Registers */
     uint32_t ocr;
-    QEMUTimer *ocr_power_timer;
     uint8_t scr[8];
     uint8_t cid[16];
     uint8_t csd[16];
     uint16_t rca;
     uint32_t card_status;
     uint8_t sd_status[64];
+
+    /* Configurable properties */
+    BlockBackend *blk;
+    bool spi;
+
+    uint32_t mode;    /* current card mode, one of SDCardModes */
+    int32_t state;    /* current card state, one of SDCardStates */
     uint32_t vhs;
     bool wp_switch;
     unsigned long *wp_groups;
@@ -110,8 +115,6 @@ struct SDState {
     uint8_t pwd[16];
     uint32_t pwd_len;
     uint8_t function_group[6];
-
-    bool spi;
     uint8_t current_cmd;
     /* True if we will handle the next command as an ACMD. Note that this does
      * *not* track the APP_CMD status bit!
@@ -123,8 +126,7 @@ struct SDState {
     uint8_t data[512];
     qemu_irq readonly_cb;
     qemu_irq inserted_cb;
-    BlockBackend *blk;
-
+    QEMUTimer *ocr_power_timer;
     bool enable;
     uint8_t dat_lines;
     bool cmd_line;
-- 
2.16.1

[Qemu-devel] [PATCH v4 02/11] sdcard: replace DPRINTF() by trace events

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c         | 32 ++++++++++++++++++++++++++------
 hw/sd/trace-events |  6 ++++++
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index ce1f2fdf76..72e9b47e34 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -40,6 +40,7 @@
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
 #include "qemu/log.h"
+#include "trace.h"
 
 //#define DEBUG_SD 1
 
@@ -132,6 +133,26 @@ struct SDState {
     bool cmd_line;
 };
 
+static const char *sd_state_name(enum SDCardStates state)
+{
+    static const char *state_name[] = {
+        [sd_idle_state]             = "idle",
+        [sd_ready_state]            = "ready",
+        [sd_identification_state]   = "identification",
+        [sd_standby_state]          = "standby",
+        [sd_transfer_state]         = "transfer",
+        [sd_sendingdata_state]      = "sendingdata",
+        [sd_receivingdata_state]    = "receivingdata",
+        [sd_programming_state]      = "programming",
+        [sd_disconnect_state]       = "disconnect",
+    };
+    if (state == sd_inactive_state) {
+        return "inactive";
+    }
+    assert(state <= ARRAY_SIZE(state_name));
+    return state_name[state];
+}
+
 static uint8_t sd_get_dat_lines(SDState *sd)
 {
     return sd->enable ? sd->dat_lines : 0;
@@ -776,6 +797,8 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
     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));
+
     /* Not interpreting this as an app command */
     sd->card_status &= ~APP_CMD;
 
@@ -790,7 +813,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
         sd->multi_blk_cnt = 0;
     }
 
-    DPRINTF("CMD%d 0x%08x state %d\n", req.cmd, req.arg, sd->state);
     switch (req.cmd) {
     /* Basic commands (Class 0 and Class 1) */
     case 0:	/* CMD0:   GO_IDLE_STATE */
@@ -1310,8 +1332,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
         return sd_r1;
 
     case 56:	/* CMD56:  GEN_CMD */
-        fprintf(stderr, "SD: GEN_CMD 0x%08x\n", req.arg);
-
         switch (sd->state) {
         case sd_transfer_state:
             sd->data_offset = 0;
@@ -1345,7 +1365,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
 static sd_rsp_type_t sd_app_command(SDState *sd,
                                     SDRequest req)
 {
-    DPRINTF("ACMD%d 0x%08x\n", req.cmd, req.arg);
+    trace_sdcard_app_command(req.cmd, req.arg);
     sd->card_status |= APP_CMD;
     switch (req.cmd) {
     case 6:	/* ACMD6:  SET_BUS_WIDTH */
@@ -1606,8 +1626,7 @@ send_response:
 
 static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
 {
-    DPRINTF("sd_blk_read: addr = 0x%08llx, len = %d\n",
-            (unsigned long long) addr, len);
+    trace_sdcard_read_block(addr, len);
     if (!sd->blk || blk_pread(sd->blk, addr, sd->data, len) < 0) {
         fprintf(stderr, "sd_blk_read: read error on host side\n");
     }
@@ -1615,6 +1634,7 @@ static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
 
 static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
 {
+    trace_sdcard_write_block(addr, len);
     if (!sd->blk || blk_pwrite(sd->blk, addr, sd->data, len, 0) < 0) {
         fprintf(stderr, "sd_blk_write: write error on host side\n");
     }
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 0f8536db32..75dac5a2cd 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -23,6 +23,12 @@ sdhci_read_dataport(uint16_t data_count) "all %u bytes of data have been read fr
 sdhci_write_dataport(uint16_t data_count) "write buffer filled with %u bytes of data"
 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_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"
+
 # hw/sd/milkymist-memcard.c
 milkymist_memcard_memory_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
 milkymist_memcard_memory_write(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
-- 
2.16.1

[Qemu-devel] [PATCH v4 03/11] sdcard: add a trace event for command responses

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c         | 27 ++++++++++++++++++++++++---
 hw/sd/trace-events |  1 +
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 72e9b47e34..8f72cde534 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -153,6 +153,27 @@ static const char *sd_state_name(enum SDCardStates state)
     return state_name[state];
 }
 
+static const char *sd_response_name(sd_rsp_type_t rsp)
+{
+    static const char *response_name[] = {
+        [sd_r0]     = "RESP#0 (no response)",
+        [sd_r1]     = "RESP#1 (normal cmd)",
+        [sd_r2_i]   = "RESP#2 (CID reg)",
+        [sd_r2_s]   = "RESP#2 (CSD reg)",
+        [sd_r3]     = "RESP#3 (OCR reg)",
+        [sd_r6]     = "RESP#6 (RCA)",
+        [sd_r7]     = "RESP#7 (operating voltage)",
+    };
+    if (rsp == sd_illegal) {
+        return "ILLEGAL RESP";
+    }
+    if (rsp == sd_r1b) {
+        rsp = sd_r1;
+    }
+    assert(rsp <= ARRAY_SIZE(response_name));
+    return response_name[rsp];
+}
+
 static uint8_t sd_get_dat_lines(SDState *sd)
 {
     return sd->enable ? sd->dat_lines : 0;
@@ -1596,10 +1617,12 @@ send_response:
 
     case sd_r0:
     case sd_illegal:
-    default:
         rsplen = 0;
         break;
+    default:
+        g_assert_not_reached();
     }
+    trace_sdcard_response(sd_response_name(rtype), rsplen);
 
     if (rtype != sd_illegal) {
         /* Clear the "clear on valid command" status bits now we've
@@ -1616,8 +1639,6 @@ send_response:
             DPRINTF(" %02x", response[i]);
         }
         DPRINTF(" state %d\n", sd->state);
-    } else {
-        DPRINTF("No response %d\n", sd->state);
     }
 #endif
 
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 75dac5a2cd..b2aa19ec0d 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -26,6 +26,7 @@ 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_response(const char *rspdesc, int rsplen) "%s (sz:%d)"
 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"
 
-- 
2.16.1

[Qemu-devel] [PATCH v4 04/11] sdcard: replace fprintf() by qemu_hexdump()

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 8f72cde534..ceab263970 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -44,13 +44,6 @@
 
 //#define DEBUG_SD 1
 
-#ifdef DEBUG_SD
-#define DPRINTF(fmt, ...) \
-do { fprintf(stderr, "SD: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) do {} while(0)
-#endif
-
 #define ACMD41_ENQUIRY_MASK     0x00ffffff
 #define OCR_POWER_UP            0x80000000
 #define OCR_POWER_DELAY_NS      500000 /* 0.5ms */
@@ -1632,14 +1625,7 @@ send_response:
     }
 
 #ifdef DEBUG_SD
-    if (rsplen) {
-        int i;
-        DPRINTF("Response:");
-        for (i = 0; i < rsplen; i++) {
-            DPRINTF(" %02x", response[i]);
-        }
-        DPRINTF(" state %d\n", sd->state);
-    }
+    qemu_hexdump((const char *)response, stderr, "Response", rsplen);
 #endif
 
     return rsplen;
-- 
2.16.1

[Qemu-devel] [PATCH v4 05/11] sdcard: add more trace events

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/sd/sd.c         | 32 ++++++++++++++++++++++++++------
 hw/sd/trace-events | 13 +++++++++++++
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index ceab263970..564f7a9bfd 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -179,6 +179,8 @@ static bool sd_get_cmd_line(SDState *sd)
 
 static void sd_set_voltage(SDState *sd, uint16_t millivolts)
 {
+    trace_sdcard_set_voltage(millivolts);
+
     switch (millivolts) {
     case 3001 ... 3600: /* SD_VOLTAGE_3_3V */
     case 2001 ... 3000: /* SD_VOLTAGE_3_0V */
@@ -274,6 +276,7 @@ static void sd_ocr_powerup(void *opaque)
 {
     SDState *sd = opaque;
 
+    trace_sdcard_powerup();
     /* Set powered up bit in OCR */
     assert(!(sd->ocr & OCR_POWER_UP));
     sd->ocr |= OCR_POWER_UP;
@@ -477,6 +480,7 @@ static void sd_reset(DeviceState *dev)
     uint64_t size;
     uint64_t sect;
 
+    trace_sdcard_reset();
     if (sd->blk) {
         blk_get_geometry(sd->blk, &sect);
     } else {
@@ -530,7 +534,10 @@ static void sd_cardchange(void *opaque, bool load, Error **errp)
     bool readonly = sd_get_readonly(sd);
 
     if (inserted) {
+        trace_sdcard_inserted(readonly);
         sd_reset(dev);
+    } else {
+        trace_sdcard_ejected();
     }
 
     /* The IRQ notification is for legacy non-QOM SD controller devices;
@@ -662,6 +669,7 @@ static void sd_erase(SDState *sd)
     uint64_t erase_start = sd->erase_start;
     uint64_t erase_end = sd->erase_end;
 
+    trace_sdcard_erase();
     if (!sd->erase_start || !sd->erase_end) {
         sd->card_status |= ERASE_SEQ_ERROR;
         return;
@@ -751,6 +759,11 @@ static void sd_lock_command(SDState *sd)
     else
         pwd_len = 0;
 
+    if (lock) {
+        trace_sdcard_lock();
+    } else {
+        trace_sdcard_unlock();
+    }
     if (erase) {
         if (!(sd->card_status & CARD_IS_LOCKED) || sd->blk_len > 1 ||
                         set_pwd || clr_pwd || lock || sd->wp_switch ||
@@ -1077,10 +1090,12 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
     case 16:	/* CMD16:  SET_BLOCKLEN */
         switch (sd->state) {
         case sd_transfer_state:
-            if (req.arg > (1 << HWBLOCK_SHIFT))
+            if (req.arg > (1 << HWBLOCK_SHIFT)) {
                 sd->card_status |= BLOCK_LEN_ERROR;
-            else
+            } else {
+                trace_sdcard_set_blocklen(req.arg);
                 sd->blk_len = req.arg;
+            }
 
             return sd_r1;
 
@@ -1452,10 +1467,13 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
                 if ((req.arg & ACMD41_ENQUIRY_MASK) != 0) {
                     timer_del(sd->ocr_power_timer);
                     sd_ocr_powerup(sd);
-                } else if (!timer_pending(sd->ocr_power_timer)) {
-                    timer_mod_ns(sd->ocr_power_timer,
-                                 (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)
-                                  + OCR_POWER_DELAY_NS));
+                } else {
+                    trace_sdcard_inquiry_cmd41();
+                    if (!timer_pending(sd->ocr_power_timer)) {
+                        timer_mod_ns(sd->ocr_power_timer,
+                                     (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)
+                                      + OCR_POWER_DELAY_NS));
+                    }
                 }
             }
 
@@ -1668,6 +1686,7 @@ 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);
     switch (sd->current_cmd) {
     case 24:	/* CMD24:  WRITE_SINGLE_BLOCK */
         sd->data[sd->data_offset ++] = value;
@@ -1805,6 +1824,7 @@ 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);
     switch (sd->current_cmd) {
     case 6:	/* CMD6:   SWITCH_FUNCTION */
         ret = sd->data[sd->data_offset ++];
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index b2aa19ec0d..3040d32560 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -27,8 +27,21 @@ sdhci_capareg(const char *desc, uint16_t val) "%s: %u"
 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_response(const char *rspdesc, int rsplen) "%s (sz:%d)"
+sdcard_powerup(void) ""
+sdcard_inquiry_cmd41(void) ""
+sdcard_set_enable(bool current_state, bool new_state) "%u -> %u"
+sdcard_reset(void) ""
+sdcard_set_blocklen(uint16_t length) "0x%04x"
+sdcard_inserted(bool readonly) "read_only: %u"
+sdcard_ejected(void) ""
+sdcard_erase(void) ""
+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_set_voltage(uint16_t millivolts) "%u mV"
 
 # hw/sd/milkymist-memcard.c
 milkymist_memcard_memory_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
-- 
2.16.1

[Qemu-devel] [PATCH v4 06/11] sdcard: do not trace CMD55 when expecting 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 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 564f7a9bfd..af4df2b104 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -818,13 +818,15 @@ 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));
+    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.1

[Qemu-devel] [PATCH v4 07/11] sdcard: define SDMMC_CMD_MAX instead of using the magic '64'

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
Since v3: add "sdmmc-internal.h"

 hw/sd/sdmmc-internal.h | 15 +++++++++++++++
 hw/sd/sd.c             | 22 ++++++++++++++++------
 2 files changed, 31 insertions(+), 6 deletions(-)
 create mode 100644 hw/sd/sdmmc-internal.h

diff --git a/hw/sd/sdmmc-internal.h b/hw/sd/sdmmc-internal.h
new file mode 100644
index 0000000000..0e96cb0081
--- /dev/null
+++ b/hw/sd/sdmmc-internal.h
@@ -0,0 +1,15 @@
+/*
+ * SD/MMC cards common
+ *
+ * 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
+ */
+#ifndef SD_INTERNAL_H
+#define SD_INTERNAL_H
+
+#define SDMMC_CMD_MAX 64
+
+#endif
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index af4df2b104..6acd6b3c5c 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -40,6 +40,7 @@
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
 #include "qemu/log.h"
+#include "sdmmc-internal.h"
 #include "trace.h"
 
 //#define DEBUG_SD 1
@@ -215,18 +216,21 @@ static void sd_set_mode(SDState *sd)
     }
 }
 
-static const sd_cmd_type_t sd_cmd_type[64] = {
+static const sd_cmd_type_t sd_cmd_type[SDMMC_CMD_MAX] = {
     sd_bc,   sd_none, sd_bcr,  sd_bcr,  sd_none, sd_none, sd_none, sd_ac,
     sd_bcr,  sd_ac,   sd_ac,   sd_adtc, sd_ac,   sd_ac,   sd_none, sd_ac,
+    /* 16 */
     sd_ac,   sd_adtc, sd_adtc, sd_none, sd_none, sd_none, sd_none, sd_none,
     sd_adtc, sd_adtc, sd_adtc, sd_adtc, sd_ac,   sd_ac,   sd_adtc, sd_none,
+    /* 32 */
     sd_ac,   sd_ac,   sd_none, sd_none, sd_none, sd_none, sd_ac,   sd_none,
     sd_none, sd_none, sd_bc,   sd_none, sd_none, sd_none, sd_none, sd_none,
+    /* 48 */
     sd_none, sd_none, sd_none, sd_none, sd_none, sd_none, sd_none, sd_ac,
     sd_adtc, sd_none, sd_none, sd_none, sd_none, sd_none, sd_none, sd_none,
 };
 
-static const int sd_cmd_class[64] = {
+static const int sd_cmd_class[SDMMC_CMD_MAX] = {
     0,  0,  0,  0,  0,  9, 10,  0,  0,  0,  0,  1,  0,  0,  0,  0,
     2,  2,  2,  2,  3,  3,  3,  3,  4,  4,  4,  4,  6,  6,  6,  6,
     5,  5, 10, 10, 10, 10,  5,  9,  9,  9,  7,  7,  7,  7,  7,  7,
@@ -831,8 +835,8 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     /* Not interpreting this as an app command */
     sd->card_status &= ~APP_CMD;
 
-    if (sd_cmd_type[req.cmd & 0x3F] == sd_ac
-        || sd_cmd_type[req.cmd & 0x3F] == sd_adtc) {
+    if (sd_cmd_type[req.cmd] == sd_ac
+        || sd_cmd_type[req.cmd] == sd_adtc) {
         rca = req.arg >> 16;
     }
 
@@ -1544,8 +1548,8 @@ static int cmd_valid_while_locked(SDState *sd, SDRequest *req)
     if (req->cmd == 16 || req->cmd == 55) {
         return 1;
     }
-    return sd_cmd_class[req->cmd & 0x3F] == 0
-            || sd_cmd_class[req->cmd & 0x3F] == 7;
+    return sd_cmd_class[req->cmd] == 0
+            || sd_cmd_class[req->cmd] == 7;
 }
 
 int sd_do_command(SDState *sd, SDRequest *req,
@@ -1564,6 +1568,12 @@ int sd_do_command(SDState *sd, SDRequest *req,
         goto send_response;
     }
 
+    if (req->cmd >= SDMMC_CMD_MAX) {
+        qemu_log_mask(LOG_GUEST_ERROR, "SD: incorrect command 0x%02x\n",
+                      req->cmd);
+        req->cmd &= 0x3f;
+    }
+
     if (sd->card_status & CARD_IS_LOCKED) {
         if (!cmd_valid_while_locked(sd, req)) {
             sd->card_status |= ILLEGAL_COMMAND;
-- 
2.16.1

[Qemu-devel] [PATCH v4 08/11] sdcard: display command name when tracing CMD/ACMD

[Philippe Mathieu-Daudé]

put the function in sdmmc-common.c since we will reuse it in hw/sd/core.c

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

diff --git a/hw/sd/sdmmc-internal.h b/hw/sd/sdmmc-internal.h
index 0e96cb0081..02b730089b 100644
--- a/hw/sd/sdmmc-internal.h
+++ b/hw/sd/sdmmc-internal.h
@@ -12,4 +12,7 @@
 
 #define SDMMC_CMD_MAX 64
 
+const char *sd_cmd_name(uint8_t cmd);
+const char *sd_acmd_name(uint8_t cmd);
+
 #endif
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 6acd6b3c5c..666ff3873f 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -828,8 +828,8 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     uint64_t addr = (sd->ocr & (1 << 30)) ? (uint64_t) req.arg << 9 : req.arg;
 
     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 */
@@ -1400,7 +1400,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 */
@@ -1698,7 +1699,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;
@@ -1836,7 +1838,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-common.c b/hw/sd/sdmmc-common.c
new file mode 100644
index 0000000000..1d0198b1ad
--- /dev/null
+++ b/hw/sd/sdmmc-common.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", /* FIXME */
+        [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..af4a248728 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-common.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.1

[Qemu-devel] [PATCH v4 09/11] sdcard: display protocol used when tracing

[Philippe Mathieu-Daudé]

put the function in sdmmc-common.c since we will reuse it in hw/sd/core.c

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.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 666ff3873f..6760815045 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -122,6 +122,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;
@@ -828,7 +829,8 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     uint64_t addr = (sd->ocr & (1 << 30)) ? (uint64_t) req.arg << 9 : req.arg;
 
     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));
     }
 
@@ -1400,7 +1402,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) {
@@ -1699,7 +1701,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 */
@@ -1838,7 +1841,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 */
@@ -1980,6 +1984,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.1

[Qemu-devel] [PATCH v4 10/11] sdcard: use G_BYTE from cutils

[Philippe Mathieu-Daudé]

code is now easier to read.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 6760815045..28837768d4 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -36,6 +36,7 @@
 #include "hw/sd/sd.h"
 #include "qapi/error.h"
 #include "qemu/bitmap.h"
+#include "qemu/cutils.h"
 #include "hw/qdev-properties.h"
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
@@ -344,7 +345,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
     uint32_t sectsize = (1 << (SECTOR_SHIFT + 1)) - 1;
     uint32_t wpsize = (1 << (WPGROUP_SHIFT + 1)) - 1;
 
-    if (size <= 0x40000000) {	/* Standard Capacity SD */
+    if (size <= 1 * G_BYTE) { /* Standard Capacity SD */
         sd->csd[0] = 0x00;	/* CSD structure */
         sd->csd[1] = 0x26;	/* Data read access-time-1 */
         sd->csd[2] = 0x00;	/* Data read access-time-2 */
-- 
2.16.1

[Qemu-devel] [PATCH v4 11/11] sdcard: use the registerfields API to access the OCR register

[Philippe Mathieu-Daudé]

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

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index bf1eb0713c..9bdb3c9285 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -53,7 +53,6 @@
 #define READY_FOR_DATA		(1 << 8)
 #define APP_CMD			(1 << 5)
 #define AKE_SEQ_ERROR		(1 << 3)
-#define OCR_CCS_BITN        30
 
 typedef enum {
     SD_VOLTAGE_0_4V     = 400,  /* currently not supported */
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 28837768d4..fbee87afef 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -32,6 +32,7 @@
 #include "qemu/osdep.h"
 #include "hw/qdev.h"
 #include "hw/hw.h"
+#include "hw/registerfields.h"
 #include "sysemu/block-backend.h"
 #include "hw/sd/sd.h"
 #include "qapi/error.h"
@@ -47,8 +48,6 @@
 //#define DEBUG_SD 1
 
 #define ACMD41_ENQUIRY_MASK     0x00ffffff
-#define OCR_POWER_UP            0x80000000
-#define OCR_POWER_DELAY_NS      500000 /* 0.5ms */
 
 typedef enum {
     sd_r0 = 0,    /* no response */
@@ -272,6 +271,11 @@ static uint16_t sd_crc16(void *message, size_t width)
     return shift_reg;
 }
 
+#define OCR_POWER_DELAY_NS      500000 /* 0.5ms */
+
+FIELD(OCR, CARD_CAPACITY,              30,  1) /* 0:SDSC, 1:SDHC/SDXC */
+FIELD(OCR, CARD_POWER_UP,              31,  1)
+
 static void sd_set_ocr(SDState *sd)
 {
     /* All voltages OK, Standard Capacity SD Memory Card, not yet powered up */
@@ -283,9 +287,10 @@ static void sd_ocr_powerup(void *opaque)
     SDState *sd = opaque;
 
     trace_sdcard_powerup();
-    /* Set powered up bit in OCR */
-    assert(!(sd->ocr & OCR_POWER_UP));
-    sd->ocr |= OCR_POWER_UP;
+    assert(!FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP));
+
+    /* card power-up OK */
+    sd->ocr = FIELD_DP32(sd->ocr, OCR, CARD_POWER_UP, 1);
 }
 
 static void sd_set_scr(SDState *sd)
@@ -571,7 +576,7 @@ static bool sd_ocr_vmstate_needed(void *opaque)
     SDState *sd = opaque;
 
     /* Include the OCR state (and timer) if it is not yet powered up */
-    return !(sd->ocr & OCR_POWER_UP);
+    return !FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP);
 }
 
 static const VMStateDescription sd_ocr_vmstate = {
@@ -681,7 +686,7 @@ static void sd_erase(SDState *sd)
         return;
     }
 
-    if (extract32(sd->ocr, OCR_CCS_BITN, 1)) {
+    if (FIELD_EX32(sd->ocr, OCR, CARD_CAPACITY)) {
         /* High capacity memory card: erase units are 512 byte blocks */
         erase_start *= 512;
         erase_end *= 512;
@@ -1473,7 +1478,7 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
              * UEFI, which sends an initial enquiry ACMD41, but
              * assumes that the card is in ready state as soon as it
              * sees the power up bit set. */
-            if (!(sd->ocr & OCR_POWER_UP)) {
+            if (!FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP)) {
                 if ((req.arg & ACMD41_ENQUIRY_MASK) != 0) {
                     timer_del(sd->ocr_power_timer);
                     sd_ocr_powerup(sd);
-- 
2.16.1

Re: [Qemu-devel] [PATCH v4 02/11] sdcard: replace DPRINTF() by trace events

[Alistair Francis]

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

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

Alistair

> ---
>  hw/sd/sd.c         | 32 ++++++++++++++++++++++++++------
>  hw/sd/trace-events |  6 ++++++
>  2 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index ce1f2fdf76..72e9b47e34 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -40,6 +40,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/timer.h"
>  #include "qemu/log.h"
> +#include "trace.h"
>
>  //#define DEBUG_SD 1
>
> @@ -132,6 +133,26 @@ struct SDState {
>      bool cmd_line;
>  };
>
> +static const char *sd_state_name(enum SDCardStates state)
> +{
> +    static const char *state_name[] = {
> +        [sd_idle_state]             = "idle",
> +        [sd_ready_state]            = "ready",
> +        [sd_identification_state]   = "identification",
> +        [sd_standby_state]          = "standby",
> +        [sd_transfer_state]         = "transfer",
> +        [sd_sendingdata_state]      = "sendingdata",
> +        [sd_receivingdata_state]    = "receivingdata",
> +        [sd_programming_state]      = "programming",
> +        [sd_disconnect_state]       = "disconnect",
> +    };
> +    if (state == sd_inactive_state) {
> +        return "inactive";
> +    }
> +    assert(state <= ARRAY_SIZE(state_name));
> +    return state_name[state];
> +}
> +
>  static uint8_t sd_get_dat_lines(SDState *sd)
>  {
>      return sd->enable ? sd->dat_lines : 0;
> @@ -776,6 +797,8 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
>      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));
> +
>      /* Not interpreting this as an app command */
>      sd->card_status &= ~APP_CMD;
>
> @@ -790,7 +813,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
>          sd->multi_blk_cnt = 0;
>      }
>
> -    DPRINTF("CMD%d 0x%08x state %d\n", req.cmd, req.arg, sd->state);
>      switch (req.cmd) {
>      /* Basic commands (Class 0 and Class 1) */
>      case 0:    /* CMD0:   GO_IDLE_STATE */
> @@ -1310,8 +1332,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
>          return sd_r1;
>
>      case 56:   /* CMD56:  GEN_CMD */
> -        fprintf(stderr, "SD: GEN_CMD 0x%08x\n", req.arg);
> -
>          switch (sd->state) {
>          case sd_transfer_state:
>              sd->data_offset = 0;
> @@ -1345,7 +1365,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
>  static sd_rsp_type_t sd_app_command(SDState *sd,
>                                      SDRequest req)
>  {
> -    DPRINTF("ACMD%d 0x%08x\n", req.cmd, req.arg);
> +    trace_sdcard_app_command(req.cmd, req.arg);
>      sd->card_status |= APP_CMD;
>      switch (req.cmd) {
>      case 6:    /* ACMD6:  SET_BUS_WIDTH */
> @@ -1606,8 +1626,7 @@ send_response:
>
>  static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
>  {
> -    DPRINTF("sd_blk_read: addr = 0x%08llx, len = %d\n",
> -            (unsigned long long) addr, len);
> +    trace_sdcard_read_block(addr, len);
>      if (!sd->blk || blk_pread(sd->blk, addr, sd->data, len) < 0) {
>          fprintf(stderr, "sd_blk_read: read error on host side\n");
>      }
> @@ -1615,6 +1634,7 @@ static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
>
>  static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
>  {
> +    trace_sdcard_write_block(addr, len);
>      if (!sd->blk || blk_pwrite(sd->blk, addr, sd->data, len, 0) < 0) {
>          fprintf(stderr, "sd_blk_write: write error on host side\n");
>      }
> diff --git a/hw/sd/trace-events b/hw/sd/trace-events
> index 0f8536db32..75dac5a2cd 100644
> --- a/hw/sd/trace-events
> +++ b/hw/sd/trace-events
> @@ -23,6 +23,12 @@ sdhci_read_dataport(uint16_t data_count) "all %u bytes of data have been read fr
>  sdhci_write_dataport(uint16_t data_count) "write buffer filled with %u bytes of data"
>  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_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"
> +
>  # hw/sd/milkymist-memcard.c
>  milkymist_memcard_memory_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
>  milkymist_memcard_memory_write(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
> --
> 2.16.1
>
>

Re: [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4)

[Philippe Mathieu-Daudé]

ping? :)

On 02/15/2018 07:05 PM, Philippe Mathieu-Daudé wrote:
> Since v3:
> - use assert() in sd_state_name() and sd_response_name() (Alistair review)
> - added sdmmc-internal.h & sdmmc-common.c to reuse helpers with hw/sd/core.c
> 
> Since v2:
> - split again in 2... this part is cleanup/tracing
> - add more tracepoints
> - move some code reusable by sdbus in sdmmc-internal.h
> 
> Since v1:
> - rewrote mostly all patches to keep it simpler.
> 
> $ git backport-diff
> 001/11:[----] [--] 'sdcard: reorder SDState struct members'
> 002/11:[0003] [FC] 'sdcard: replace DPRINTF() by trace events'
> 003/11:[0001] [FC] 'sdcard: add a trace event for command responses'
> 004/11:[0007] [FC] 'sdcard: replace fprintf() by qemu_hexdump()'
> 005/11:[----] [--] 'sdcard: add more trace events'
> 006/11:[----] [--] 'sdcard: do not trace CMD55 when expecting ACMD'
> 007/11:[0014] [FC] 'sdcard: define SDMMC_CMD_MAX instead of using the magic '64''
> 008/11:[0020] [FC] 'sdcard: display command name when tracing CMD/ACMD'
> 009/11:[----] [--] 'sdcard: display protocol used when tracing'
> 010/11:[----] [-C] 'sdcard: use G_BYTE from cutils'
> 011/11:[----] [-C] 'sdcard: use the registerfields API to access the OCR register'
> 
> Philippe Mathieu-Daudé (11):
>   sdcard: reorder SDState struct members
>   sdcard: replace DPRINTF() by trace events
>   sdcard: add a trace event for command responses
>   sdcard: replace fprintf() by qemu_hexdump()
>   sdcard: add more trace events
>   sdcard: do not trace CMD55 when expecting ACMD
>   sdcard: define SDMMC_CMD_MAX instead of using the magic '64'
>   sdcard: display command name when tracing CMD/ACMD
>   sdcard: display protocol used when tracing
>   sdcard: use G_BYTE from cutils
>   sdcard: use the registerfields API to access the OCR register
> 
>  hw/sd/sdmmc-internal.h |  18 +++++
>  include/hw/sd/sd.h     |   1 -
>  hw/sd/sd.c             | 184 ++++++++++++++++++++++++++++++++++---------------
>  hw/sd/sdmmc-common.c   |  72 +++++++++++++++++++
>  hw/sd/Makefile.objs    |   2 +-
>  hw/sd/trace-events     |  20 ++++++
>  6 files changed, 241 insertions(+), 56 deletions(-)
>  create mode 100644 hw/sd/sdmmc-internal.h
>  create mode 100644 hw/sd/sdmmc-common.c
> 

Re: [Qemu-devel] [PATCH v4 06/11] sdcard: do not trace CMD55 when expecting ACMD

[Peter Maydell]

On 15 February 2018 at 22:05, 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 | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 564f7a9bfd..af4df2b104 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -818,13 +818,15 @@ 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));
> +    if (req.cmd != 55 || sd->expecting_acmd) {
> +        trace_sdcard_normal_command(req.cmd, req.arg,
> +                                    sd_state_name(sd->state));
> +    }

The commit message says "don't trace CMD55 when expecting ACMD",
but the code says "don't trace CMD55 when *not* expecting ACMD" --
which is correct?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 08/11] sdcard: display command name when tracing CMD/ACMD

[Peter Maydell]

On 15 February 2018 at 22:05, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> put the function in sdmmc-common.c since we will reuse it in hw/sd/core.c
>

As a general note, your commit messages are generally a bit
shorter than I'd prefer. This is to some extent a personal
style question, but if you're writing lots of patches where
the only commit message is the subject-line summary then I
think that it's worth being a bit more descriptive.
Also, the "body" part of the commit message should really
be able to stand alone as a description, rather than relying
on the subject line to make sense.

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sdmmc-internal.h |  3 +++
>  hw/sd/sd.c             | 13 +++++----
>  hw/sd/sdmmc-common.c   | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/sd/Makefile.objs    |  2 +-
>  hw/sd/trace-events     |  8 +++---
>  5 files changed, 88 insertions(+), 10 deletions(-)
>  create mode 100644 hw/sd/sdmmc-common.c
>
> diff --git a/hw/sd/sdmmc-internal.h b/hw/sd/sdmmc-internal.h
> index 0e96cb0081..02b730089b 100644
> --- a/hw/sd/sdmmc-internal.h
> +++ b/hw/sd/sdmmc-internal.h
> @@ -12,4 +12,7 @@
>
>  #define SDMMC_CMD_MAX 64
>
> +const char *sd_cmd_name(uint8_t cmd);
> +const char *sd_acmd_name(uint8_t cmd);

Can we have doc comments for new function prototypes in header files,
please?


> diff --git a/hw/sd/sdmmc-common.c b/hw/sd/sdmmc-common.c
> new file mode 100644
> index 0000000000..1d0198b1ad
> --- /dev/null
> +++ b/hw/sd/sdmmc-common.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

Ooh, a SPDX identifier. The project doesn't actually use these
at the moment, but it doesn't hurt to have one here I guess.

> + */
> +
> +#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", /* FIXME */

What's this FIXME for? We should either fix whatever it is, or if
that's not practical the comment needs to describe the problem
so that a future reader of the code knows what it means and might
be able to fix it.

> +        [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";
> +}

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 09/11] sdcard: display protocol used when tracing

[Peter Maydell]

On 15 February 2018 at 22:05, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> put the function in sdmmc-common.c since we will reuse it in hw/sd/core.c
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Commit message talks about a function but the patch doesn't
add any new functions ? (Also, sentences should start with
a capital letter and end with a full stop, please.)

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4)

[Peter Maydell]

On 15 February 2018 at 22:05, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Since v3:
> - use assert() in sd_state_name() and sd_response_name() (Alistair review)
> - added sdmmc-internal.h & sdmmc-common.c to reuse helpers with hw/sd/core.c
>
> Since v2:
> - split again in 2... this part is cleanup/tracing
> - add more tracepoints
> - move some code reusable by sdbus in sdmmc-internal.h
>
> Since v1:
> - rewrote mostly all patches to keep it simpler.
>
> $ git backport-diff
> 001/11:[----] [--] 'sdcard: reorder SDState struct members'
> 002/11:[0003] [FC] 'sdcard: replace DPRINTF() by trace events'
> 003/11:[0001] [FC] 'sdcard: add a trace event for command responses'
> 004/11:[0007] [FC] 'sdcard: replace fprintf() by qemu_hexdump()'
> 005/11:[----] [--] 'sdcard: add more trace events'
> 006/11:[----] [--] 'sdcard: do not trace CMD55 when expecting ACMD'
> 007/11:[0014] [FC] 'sdcard: define SDMMC_CMD_MAX instead of using the magic '64''
> 008/11:[0020] [FC] 'sdcard: display command name when tracing CMD/ACMD'
> 009/11:[----] [--] 'sdcard: display protocol used when tracing'
> 010/11:[----] [-C] 'sdcard: use G_BYTE from cutils'
> 011/11:[----] [-C] 'sdcard: use the registerfields API to access the OCR register'

Hi. I've applied patches 1-5, 7, 10 and 11 to target-arm.next.
The rest I've left you review comments for.

-- PMM

Re: [Qemu-devel] [PATCH v4 03/11] sdcard: add a trace event for command responses

[Alistair Francis]

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

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

Alistair

> ---
>  hw/sd/sd.c         | 27 ++++++++++++++++++++++++---
>  hw/sd/trace-events |  1 +
>  2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 72e9b47e34..8f72cde534 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -153,6 +153,27 @@ static const char *sd_state_name(enum SDCardStates state)
>      return state_name[state];
>  }
>
> +static const char *sd_response_name(sd_rsp_type_t rsp)
> +{
> +    static const char *response_name[] = {
> +        [sd_r0]     = "RESP#0 (no response)",
> +        [sd_r1]     = "RESP#1 (normal cmd)",
> +        [sd_r2_i]   = "RESP#2 (CID reg)",
> +        [sd_r2_s]   = "RESP#2 (CSD reg)",
> +        [sd_r3]     = "RESP#3 (OCR reg)",
> +        [sd_r6]     = "RESP#6 (RCA)",
> +        [sd_r7]     = "RESP#7 (operating voltage)",
> +    };
> +    if (rsp == sd_illegal) {
> +        return "ILLEGAL RESP";
> +    }
> +    if (rsp == sd_r1b) {
> +        rsp = sd_r1;
> +    }
> +    assert(rsp <= ARRAY_SIZE(response_name));
> +    return response_name[rsp];
> +}
> +
>  static uint8_t sd_get_dat_lines(SDState *sd)
>  {
>      return sd->enable ? sd->dat_lines : 0;
> @@ -1596,10 +1617,12 @@ send_response:
>
>      case sd_r0:
>      case sd_illegal:
> -    default:
>          rsplen = 0;
>          break;
> +    default:
> +        g_assert_not_reached();
>      }
> +    trace_sdcard_response(sd_response_name(rtype), rsplen);
>
>      if (rtype != sd_illegal) {
>          /* Clear the "clear on valid command" status bits now we've
> @@ -1616,8 +1639,6 @@ send_response:
>              DPRINTF(" %02x", response[i]);
>          }
>          DPRINTF(" state %d\n", sd->state);
> -    } else {
> -        DPRINTF("No response %d\n", sd->state);
>      }
>  #endif
>
> diff --git a/hw/sd/trace-events b/hw/sd/trace-events
> index 75dac5a2cd..b2aa19ec0d 100644
> --- a/hw/sd/trace-events
> +++ b/hw/sd/trace-events
> @@ -26,6 +26,7 @@ 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_response(const char *rspdesc, int rsplen) "%s (sz:%d)"
>  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"
>
> --
> 2.16.1
>
>

Re: [Qemu-devel] [PATCH v4 09/11] sdcard: display protocol used when tracing

[Philippe Mathieu-Daudé]

On 02/22/2018 10:02 AM, Peter Maydell wrote:
> On 15 February 2018 at 22:05, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> put the function in sdmmc-common.c since we will reuse it in hw/sd/core.c
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> Commit message talks about a function but the patch doesn't
> add any new functions ? (Also, sentences should start with

I suppose I mistaken the commit message when I rebased, this message
belongs to the previous commit.

> a capital letter and end with a full stop, please.)

Got it!

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

Thanks!

Re: [Qemu-devel] [PATCH v4 06/11] sdcard: do not trace CMD55 when expecting ACMD

[Philippe Mathieu-Daudé]

Hi Peter,

On 02/22/2018 01:01 PM, Peter Maydell wrote:
> On 15 February 2018 at 22:05, 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 | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 564f7a9bfd..af4df2b104 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -818,13 +818,15 @@ 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));
>> +    if (req.cmd != 55 || sd->expecting_acmd) {
>> +        trace_sdcard_normal_command(req.cmd, req.arg,
>> +                                    sd_state_name(sd->state));
>> +    }
> 
> The commit message says "don't trace CMD55 when expecting ACMD",
> but the code says "don't trace CMD55 when *not* expecting ACMD" --
> which is correct?

If req.cmd == 55 we will expect an ACMD (setting sd->expecting_acmd =
true). Logging CMD55 is not interesting, since we prefer to log the ACMD
directly.

Now if we have another CMD55 after a CMD55, this is not an ACMD and we
want to log it.

I'll use the following message: "Do not trace CMD55, except when we
already expect an ACMD" and add the previous lines as comment.

Is that Ok to you? I don't have clearer if condition, the code looks
correct to me.

Thanks,

Phil.

Re: [Qemu-devel] [PATCH v4 08/11] sdcard: display command name when tracing CMD/ACMD

[Philippe Mathieu-Daudé]

Hi Peter,

On 02/22/2018 01:59 PM, Peter Maydell wrote:
> On 15 February 2018 at 22:05, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> put the function in sdmmc-common.c since we will reuse it in hw/sd/core.c
>>
> 
> As a general note, your commit messages are generally a bit
> shorter than I'd prefer. This is to some extent a personal
> style question, but if you're writing lots of patches where
> the only commit message is the subject-line summary then I
> think that it's worth being a bit more descriptive.

Got it, I'll try to be a bit more verbose. As you noticed this is not
natural for me ;) It took me time to understand what is obvious for
someone focused weeks in the same code, is not for a reviewer context
switching from an unrelated topic, and for a limited time.

> Also, the "body" part of the commit message should really
> be able to stand alone as a description, rather than relying
> on the subject line to make sense.

This one is easy to fix :)

> 
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/sdmmc-internal.h |  3 +++
>>  hw/sd/sd.c             | 13 +++++----
>>  hw/sd/sdmmc-common.c   | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  hw/sd/Makefile.objs    |  2 +-
>>  hw/sd/trace-events     |  8 +++---
>>  5 files changed, 88 insertions(+), 10 deletions(-)
>>  create mode 100644 hw/sd/sdmmc-common.c
>>
>> diff --git a/hw/sd/sdmmc-internal.h b/hw/sd/sdmmc-internal.h
>> index 0e96cb0081..02b730089b 100644
>> --- a/hw/sd/sdmmc-internal.h
>> +++ b/hw/sd/sdmmc-internal.h
>> @@ -12,4 +12,7 @@
>>
>>  #define SDMMC_CMD_MAX 64
>>
>> +const char *sd_cmd_name(uint8_t cmd);
>> +const char *sd_acmd_name(uint8_t cmd);
> 
> Can we have doc comments for new function prototypes in header files,
> please?

Yes :|

> 
> 
>> diff --git a/hw/sd/sdmmc-common.c b/hw/sd/sdmmc-common.c
>> new file mode 100644
>> index 0000000000..1d0198b1ad
>> --- /dev/null
>> +++ b/hw/sd/sdmmc-common.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
> 
> Ooh, a SPDX identifier. The project doesn't actually use these
> at the moment, but it doesn't hurt to have one here I guess.
> 
>> + */
>> +
>> +#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", /* FIXME */
> 
> What's this FIXME for? We should either fix whatever it is, or if
> that's not practical the comment needs to describe the problem
> so that a future reader of the code knows what it means and might
> be able to fix it.

Oops, I forgot to search for the CMD51 correct name.
Apparently some future Video extension, so I'll just remove the FIXME.

> 
>> +        [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";
>> +}
> 
> thanks
> -- PMM
>