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

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

[Philippe Mathieu-Daudé]

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/12:[0007] [FC] 'sdcard: reorder SDState struct members'
002/12:[0002] [FC] 'sdcard: replace DPRINTF() by trace events'
003/12:[down] 'sdcard: add a trace event for command responses'
004/12:[down] 'sdcard: replace fprintf() by qemu_hexdump()'
005/12:[0035] [FC] 'sdcard: add more trace events'
006/12:[down] 'sdcard: do not trace CMD55 when expecting ACMD'
007/12:[down] 'sdcard: define SDMMC_CMD_MAX instead of using the magic '64''
008/12:[0083] [FC] 'sdcard: display command name when tracing CMD/ACMD'
009/12:[down] 'sdcard: display protocol used when tracing'
010/12:[----] [--] 'sdcard: rename sd_set_$REG() functions called once as sd_reset_$REG()'
011/12:[0002] [FC] 'sdcard: use G_BYTE from cutils'
012/12:[0008] [FC] 'sdcard: use the registerfields API to access the OCR register'

Based-on: 20180123030630.26613-15-f4bug@amsat.org

Philippe Mathieu-Daudé (12):
  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: rename sd_set_$REG() functions called once as sd_reset_$REG()
  sdcard: use G_BYTE from cutils
  sdcard: use the registerfields API to access the OCR register

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

-- 
2.15.1

[Qemu-devel] [PATCH v3 01/12] 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 ab9be561d2..55d2ba2dd7 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.15.1

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

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 55d2ba2dd7..f876973a2b 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,25 @@ 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";
+    }
+    return state_name[state];
+}
+
 static uint8_t sd_get_dat_lines(SDState *sd)
 {
     return sd->enable ? sd->dat_lines : 0;
@@ -776,6 +796,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 +812,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 +1331,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 +1364,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 */
@@ -1594,7 +1613,7 @@ send_response:
         DPRINTF("Response:");
         for (i = 0; i < rsplen; i++)
             fprintf(stderr, " %02x", response[i]);
-        fprintf(stderr, " state %d\n", sd->state);
+        fputc('\n', stderr);
     } else {
         DPRINTF("No response %d\n", sd->state);
     }
@@ -1605,8 +1624,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");
     }
@@ -1614,6 +1632,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.15.1

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

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index f876973a2b..3590099ce8 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -152,6 +152,26 @@ 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;
+    }
+    return response_name[rsp];
+}
+
 static uint8_t sd_get_dat_lines(SDState *sd)
 {
     return sd->enable ? sd->dat_lines : 0;
@@ -1595,10 +1615,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
@@ -1614,8 +1636,6 @@ send_response:
         for (i = 0; i < rsplen; i++)
             fprintf(stderr, " %02x", response[i]);
         fputc('\n', stderr);
-    } 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.15.1

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

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 3590099ce8..03263e08ae 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 */
@@ -1630,13 +1623,7 @@ send_response:
     }
 
 #ifdef DEBUG_SD
-    if (rsplen) {
-        int i;
-        DPRINTF("Response:");
-        for (i = 0; i < rsplen; i++)
-            fprintf(stderr, " %02x", response[i]);
-        fputc('\n', stderr);
-    }
+    qemu_hexdump((const char *)response, stderr, "Response", rsplen);
 #endif
 
     return rsplen;
-- 
2.15.1

[Qemu-devel] [PATCH v3 05/12] sdcard: add more trace events

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 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 03263e08ae..dc4b2329e4 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -177,6 +177,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 */
@@ -272,6 +274,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;
@@ -475,6 +478,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 {
@@ -528,7 +532,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;
@@ -660,6 +667,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;
@@ -749,6 +757,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 ||
@@ -1075,10 +1088,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;
 
@@ -1450,10 +1465,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));
+                    }
                 }
             }
 
@@ -1666,6 +1684,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;
@@ -1803,6 +1822,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.15.1

[Qemu-devel] [PATCH v3 06/12] sdcard: do not trace CMD55 when expecting ACMD

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 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 dc4b2329e4..27c08aa894 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -816,13 +816,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.15.1

[Qemu-devel] [PATCH v3 07/12] 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>
---
 hw/sd/sd.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 27c08aa894..7b1cbc0a2e 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -126,6 +126,8 @@ struct SDState {
     bool cmd_line;
 };
 
+#define SDMMC_CMD_MAX 64
+
 static const char *sd_state_name(enum SDCardStates state)
 {
     static const char *state_name[] = {
@@ -213,18 +215,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,
@@ -829,8 +834,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;
     }
 
@@ -1542,8 +1547,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,
@@ -1562,6 +1567,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.15.1

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

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sdmmc-internal.h b/hw/sd/sdmmc-internal.h
new file mode 100644
index 0000000000..7db6c63ee7
--- /dev/null
+++ b/hw/sd/sdmmc-internal.h
@@ -0,0 +1,20 @@
+/*
+ * 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
+
+#include "hw/sd/sd.h"
+
+#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 7b1cbc0a2e..e2c4d96d76 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
@@ -126,8 +127,6 @@ struct SDState {
     bool cmd_line;
 };
 
-#define SDMMC_CMD_MAX 64
-
 static const char *sd_state_name(enum SDCardStates state)
 {
     static const char *state_name[] = {
@@ -827,8 +826,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 */
@@ -1399,7 +1398,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 */
@@ -1697,7 +1697,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;
@@ -1835,7 +1836,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.15.1

[Qemu-devel] [PATCH v3 09/12] sdcard: display protocol used when tracing

[Philippe Mathieu-Daudé]

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 e2c4d96d76..c46e9c2818 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;
@@ -826,7 +827,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));
     }
 
@@ -1398,7 +1400,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) {
@@ -1697,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_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 */
@@ -1836,7 +1839,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 */
@@ -1978,6 +1982,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.15.1

[Qemu-devel] [PATCH v3 10/12] sdcard: rename sd_set_$REG() functions called once as sd_reset_$REG()

[Philippe Mathieu-Daudé]

All are only called once at reset.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index c46e9c2818..8b5022a7db 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -269,7 +269,7 @@ static uint16_t sd_crc16(void *message, size_t width)
     return shift_reg;
 }
 
-static void sd_set_ocr(SDState *sd)
+static void sd_reset_ocr(SDState *sd)
 {
     /* All voltages OK, Standard Capacity SD Memory Card, not yet powered up */
     sd->ocr = 0x00ffff00;
@@ -285,7 +285,7 @@ static void sd_ocr_powerup(void *opaque)
     sd->ocr |= OCR_POWER_UP;
 }
 
-static void sd_set_scr(SDState *sd)
+static void sd_reset_scr(SDState *sd)
 {
     sd->scr[0] = 0x00;		/* SCR Structure */
     sd->scr[1] = 0x2f;		/* SD Security Support */
@@ -304,7 +304,7 @@ static void sd_set_scr(SDState *sd)
 #define MDT_YR	2006
 #define MDT_MON	2
 
-static void sd_set_cid(SDState *sd)
+static void sd_reset_cid(SDState *sd)
 {
     sd->cid[0] = MID;		/* Fake card manufacturer ID (MID) */
     sd->cid[1] = OID[0];	/* OEM/Application ID (OID) */
@@ -336,7 +336,7 @@ static const uint8_t sd_csd_rw_mask[16] = {
     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xfc, 0xfe,
 };
 
-static void sd_set_csd(SDState *sd, uint64_t size)
+static void sd_reset_csd(SDState *sd, uint64_t size)
 {
     uint32_t csize = (size >> (CMULT_SHIFT + HWBLOCK_SHIFT)) - 1;
     uint32_t sectsize = (1 << (SECTOR_SHIFT + 1)) - 1;
@@ -391,6 +391,11 @@ static void sd_set_csd(SDState *sd, uint64_t size)
     }
 }
 
+static void sd_reset_rca(SDState *sd)
+{
+    sd->rca = 0;
+}
+
 static void sd_set_rca(SDState *sd)
 {
     sd->rca += 0x4567;
@@ -405,12 +410,12 @@ static void sd_set_rca(SDState *sd)
 #define CARD_STATUS_B	0x00c01e00
 #define CARD_STATUS_C	0xfd39a028
 
-static void sd_set_cardstatus(SDState *sd)
+static void sd_reset_cardstatus(SDState *sd)
 {
     sd->card_status = 0x00000100;
 }
 
-static void sd_set_sdstatus(SDState *sd)
+static void sd_reset_sdstatus(SDState *sd)
 {
     memset(sd->sd_status, 0, 64);
 }
@@ -494,13 +499,13 @@ static void sd_reset(DeviceState *dev)
     sect = sd_addr_to_wpnum(size) + 1;
 
     sd->state = sd_idle_state;
-    sd->rca = 0x0000;
-    sd_set_ocr(sd);
-    sd_set_scr(sd);
-    sd_set_cid(sd);
-    sd_set_csd(sd, size);
-    sd_set_cardstatus(sd);
-    sd_set_sdstatus(sd);
+    sd_reset_rca(sd);
+    sd_reset_ocr(sd);
+    sd_reset_scr(sd);
+    sd_reset_cid(sd);
+    sd_reset_csd(sd, size);
+    sd_reset_cardstatus(sd);
+    sd_reset_sdstatus(sd);
 
     g_free(sd->wp_groups);
     sd->wp_switch = sd->blk ? blk_is_read_only(sd->blk) : false;
-- 
2.15.1

[Qemu-devel] [PATCH v3 11/12] 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 8b5022a7db..f87e543f8f 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"
@@ -342,7 +343,7 @@ static void sd_reset_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.15.1

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

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 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 f87e543f8f..437ce25f79 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 */
@@ -270,6 +269,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_reset_ocr(SDState *sd)
 {
     /* All voltages OK, Standard Capacity SD Memory Card, not yet powered up */
@@ -281,9 +285,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_reset_scr(SDState *sd)
@@ -574,7 +579,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 = {
@@ -684,7 +689,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;
@@ -1476,7 +1481,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.15.1

Re: [Qemu-devel] [PATCH v3 04/12] sdcard: replace fprintf() by qemu_hexdump()

[Alistair Francis]

On Mon, Jan 22, 2018 at 7:21 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 | 15 +--------------
>  1 file changed, 1 insertion(+), 14 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 3590099ce8..03263e08ae 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 */
> @@ -1630,13 +1623,7 @@ send_response:
>      }
>
>  #ifdef DEBUG_SD
> -    if (rsplen) {
> -        int i;
> -        DPRINTF("Response:");
> -        for (i = 0; i < rsplen; i++)
> -            fprintf(stderr, " %02x", response[i]);
> -        fputc('\n', stderr);
> -    }
> +    qemu_hexdump((const char *)response, stderr, "Response", rsplen);
>  #endif
>
>      return rsplen;
> --
> 2.15.1
>
>

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

[Alistair Francis]

On Mon, Jan 22, 2018 at 7:21 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

Alistair

> ---
>  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 dc4b2329e4..27c08aa894 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -816,13 +816,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.15.1
>
>

Re: [Qemu-devel] [PATCH v3 12/12] sdcard: use the registerfields API to access the OCR register

[Alistair Francis]

On Mon, Jan 22, 2018 at 7:21 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

> ---
>  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 f87e543f8f..437ce25f79 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 */
> @@ -270,6 +269,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_reset_ocr(SDState *sd)
>  {
>      /* All voltages OK, Standard Capacity SD Memory Card, not yet powered up */
> @@ -281,9 +285,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_reset_scr(SDState *sd)
> @@ -574,7 +579,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 = {
> @@ -684,7 +689,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;
> @@ -1476,7 +1481,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.15.1
>
>

Re: [Qemu-devel] [PATCH v3 05/12] sdcard: add more trace events

[Alistair Francis]

On Mon, Jan 22, 2018 at 7:21 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

Alistair

> ---
>  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 03263e08ae..dc4b2329e4 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -177,6 +177,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 */
> @@ -272,6 +274,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;
> @@ -475,6 +478,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 {
> @@ -528,7 +532,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;
> @@ -660,6 +667,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;
> @@ -749,6 +757,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 ||
> @@ -1075,10 +1088,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;
>
> @@ -1450,10 +1465,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));
> +                    }
>                  }
>              }
>
> @@ -1666,6 +1684,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;
> @@ -1803,6 +1822,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.15.1
>
>

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

[Alistair Francis]

On Mon, Jan 22, 2018 at 7:21 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c         | 33 ++++++++++++++++++++++++++-------
>  hw/sd/trace-events |  6 ++++++
>  2 files changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 55d2ba2dd7..f876973a2b 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,25 @@ 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";
> +    }

There should be an assert here to make sure we never go off the end of
this array.

Is this used in future? It seems like a lot of work for one caller.

> +    return state_name[state];
> +}
> +
>  static uint8_t sd_get_dat_lines(SDState *sd)
>  {
>      return sd->enable ? sd->dat_lines : 0;
> @@ -776,6 +796,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 +812,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 +1331,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 +1364,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 */
> @@ -1594,7 +1613,7 @@ send_response:
>          DPRINTF("Response:");
>          for (i = 0; i < rsplen; i++)
>              fprintf(stderr, " %02x", response[i]);
> -        fprintf(stderr, " state %d\n", sd->state);
> +        fputc('\n', stderr);

Why change this?

Alistair

>      } else {
>          DPRINTF("No response %d\n", sd->state);
>      }
> @@ -1605,8 +1624,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");
>      }
> @@ -1614,6 +1632,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.15.1
>
>

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

[Alistair Francis]

On Mon, Jan 22, 2018 at 7:21 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c         | 26 +++++++++++++++++++++++---
>  hw/sd/trace-events |  1 +
>  2 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index f876973a2b..3590099ce8 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -152,6 +152,26 @@ 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;
> +    }

Same comments as patch number 2.

Alistair

> +    return response_name[rsp];
> +}
> +
>  static uint8_t sd_get_dat_lines(SDState *sd)
>  {
>      return sd->enable ? sd->dat_lines : 0;
> @@ -1595,10 +1615,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
> @@ -1614,8 +1636,6 @@ send_response:
>          for (i = 0; i < rsplen; i++)
>              fprintf(stderr, " %02x", response[i]);
>          fputc('\n', stderr);
> -    } 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.15.1
>
>

Re: [Qemu-devel] [PATCH v3 10/12] sdcard: rename sd_set_$REG() functions called once as sd_reset_$REG()

[Alistair Francis]

On Mon, Jan 22, 2018 at 7:21 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> All are only called once at reset.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

I don't think this patch gets us anything, in the future someone might
use the functions to set values and then this new name will be
confusing. Can we drop this patch?

Alistair

> ---
>  hw/sd/sd.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index c46e9c2818..8b5022a7db 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -269,7 +269,7 @@ static uint16_t sd_crc16(void *message, size_t width)
>      return shift_reg;
>  }
>
> -static void sd_set_ocr(SDState *sd)
> +static void sd_reset_ocr(SDState *sd)
>  {
>      /* All voltages OK, Standard Capacity SD Memory Card, not yet powered up */
>      sd->ocr = 0x00ffff00;
> @@ -285,7 +285,7 @@ static void sd_ocr_powerup(void *opaque)
>      sd->ocr |= OCR_POWER_UP;
>  }
>
> -static void sd_set_scr(SDState *sd)
> +static void sd_reset_scr(SDState *sd)
>  {
>      sd->scr[0] = 0x00;         /* SCR Structure */
>      sd->scr[1] = 0x2f;         /* SD Security Support */
> @@ -304,7 +304,7 @@ static void sd_set_scr(SDState *sd)
>  #define MDT_YR 2006
>  #define MDT_MON        2
>
> -static void sd_set_cid(SDState *sd)
> +static void sd_reset_cid(SDState *sd)
>  {
>      sd->cid[0] = MID;          /* Fake card manufacturer ID (MID) */
>      sd->cid[1] = OID[0];       /* OEM/Application ID (OID) */
> @@ -336,7 +336,7 @@ static const uint8_t sd_csd_rw_mask[16] = {
>      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xfc, 0xfe,
>  };
>
> -static void sd_set_csd(SDState *sd, uint64_t size)
> +static void sd_reset_csd(SDState *sd, uint64_t size)
>  {
>      uint32_t csize = (size >> (CMULT_SHIFT + HWBLOCK_SHIFT)) - 1;
>      uint32_t sectsize = (1 << (SECTOR_SHIFT + 1)) - 1;
> @@ -391,6 +391,11 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>      }
>  }
>
> +static void sd_reset_rca(SDState *sd)
> +{
> +    sd->rca = 0;
> +}
> +
>  static void sd_set_rca(SDState *sd)
>  {
>      sd->rca += 0x4567;
> @@ -405,12 +410,12 @@ static void sd_set_rca(SDState *sd)
>  #define CARD_STATUS_B  0x00c01e00
>  #define CARD_STATUS_C  0xfd39a028
>
> -static void sd_set_cardstatus(SDState *sd)
> +static void sd_reset_cardstatus(SDState *sd)
>  {
>      sd->card_status = 0x00000100;
>  }
>
> -static void sd_set_sdstatus(SDState *sd)
> +static void sd_reset_sdstatus(SDState *sd)
>  {
>      memset(sd->sd_status, 0, 64);
>  }
> @@ -494,13 +499,13 @@ static void sd_reset(DeviceState *dev)
>      sect = sd_addr_to_wpnum(size) + 1;
>
>      sd->state = sd_idle_state;
> -    sd->rca = 0x0000;
> -    sd_set_ocr(sd);
> -    sd_set_scr(sd);
> -    sd_set_cid(sd);
> -    sd_set_csd(sd, size);
> -    sd_set_cardstatus(sd);
> -    sd_set_sdstatus(sd);
> +    sd_reset_rca(sd);
> +    sd_reset_ocr(sd);
> +    sd_reset_scr(sd);
> +    sd_reset_cid(sd);
> +    sd_reset_csd(sd, size);
> +    sd_reset_cardstatus(sd);
> +    sd_reset_sdstatus(sd);
>
>      g_free(sd->wp_groups);
>      sd->wp_switch = sd->blk ? blk_is_read_only(sd->blk) : false;
> --
> 2.15.1
>
>

Re: [Qemu-devel] [PATCH v3 10/12] sdcard: rename sd_set_$REG() functions called once as sd_reset_$REG()

[Philippe Mathieu-Daudé]

Hi Alistair,

On 01/31/2018 01:28 PM, Alistair Francis wrote:
> On Mon, Jan 22, 2018 at 7:21 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> All are only called once at reset.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> I don't think this patch gets us anything, in the future someone might
> use the functions to set values and then this new name will be
> confusing. Can we drop this patch?

This is indeed the purpose of this patch, be explicit than these
functions are here to reset default values and not set anything (except
value at reset).

I personally prefer to use explicit self-documenting names to avoid such
confusions.

I'll drop it from this series and eventually reintroduce it in last part
where MMC functions do set values different than reset values.

> 
> Alistair
> 
>> ---
>>  hw/sd/sd.c | 31 ++++++++++++++++++-------------
>>  1 file changed, 18 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index c46e9c2818..8b5022a7db 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -269,7 +269,7 @@ static uint16_t sd_crc16(void *message, size_t width)
>>      return shift_reg;
>>  }
>>
>> -static void sd_set_ocr(SDState *sd)
>> +static void sd_reset_ocr(SDState *sd)
>>  {
>>      /* All voltages OK, Standard Capacity SD Memory Card, not yet powered up */
>>      sd->ocr = 0x00ffff00;
>> @@ -285,7 +285,7 @@ static void sd_ocr_powerup(void *opaque)
>>      sd->ocr |= OCR_POWER_UP;
>>  }
>>
>> -static void sd_set_scr(SDState *sd)
>> +static void sd_reset_scr(SDState *sd)
>>  {
>>      sd->scr[0] = 0x00;         /* SCR Structure */
>>      sd->scr[1] = 0x2f;         /* SD Security Support */
>> @@ -304,7 +304,7 @@ static void sd_set_scr(SDState *sd)
>>  #define MDT_YR 2006
>>  #define MDT_MON        2
>>
>> -static void sd_set_cid(SDState *sd)
>> +static void sd_reset_cid(SDState *sd)
>>  {
>>      sd->cid[0] = MID;          /* Fake card manufacturer ID (MID) */
>>      sd->cid[1] = OID[0];       /* OEM/Application ID (OID) */
>> @@ -336,7 +336,7 @@ static const uint8_t sd_csd_rw_mask[16] = {
>>      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xfc, 0xfe,
>>  };
>>
>> -static void sd_set_csd(SDState *sd, uint64_t size)
>> +static void sd_reset_csd(SDState *sd, uint64_t size)
>>  {
>>      uint32_t csize = (size >> (CMULT_SHIFT + HWBLOCK_SHIFT)) - 1;
>>      uint32_t sectsize = (1 << (SECTOR_SHIFT + 1)) - 1;
>> @@ -391,6 +391,11 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>>      }
>>  }
>>
>> +static void sd_reset_rca(SDState *sd)
>> +{
>> +    sd->rca = 0;
>> +}
>> +
>>  static void sd_set_rca(SDState *sd)
>>  {
>>      sd->rca += 0x4567;

Here is an example where set_rsa() is not the same than reset_rsa().

>> @@ -405,12 +410,12 @@ static void sd_set_rca(SDState *sd)
>>  #define CARD_STATUS_B  0x00c01e00
>>  #define CARD_STATUS_C  0xfd39a028
>>
>> -static void sd_set_cardstatus(SDState *sd)
>> +static void sd_reset_cardstatus(SDState *sd)
>>  {
>>      sd->card_status = 0x00000100;
>>  }
>>
>> -static void sd_set_sdstatus(SDState *sd)
>> +static void sd_reset_sdstatus(SDState *sd)
>>  {
>>      memset(sd->sd_status, 0, 64);
>>  }
>> @@ -494,13 +499,13 @@ static void sd_reset(DeviceState *dev)
>>      sect = sd_addr_to_wpnum(size) + 1;
>>
>>      sd->state = sd_idle_state;
>> -    sd->rca = 0x0000;
>> -    sd_set_ocr(sd);
>> -    sd_set_scr(sd);
>> -    sd_set_cid(sd);
>> -    sd_set_csd(sd, size);
>> -    sd_set_cardstatus(sd);
>> -    sd_set_sdstatus(sd);
>> +    sd_reset_rca(sd);
>> +    sd_reset_ocr(sd);
>> +    sd_reset_scr(sd);
>> +    sd_reset_cid(sd);
>> +    sd_reset_csd(sd, size);
>> +    sd_reset_cardstatus(sd);
>> +    sd_reset_sdstatus(sd);
>>
>>      g_free(sd->wp_groups);
>>      sd->wp_switch = sd->blk ? blk_is_read_only(sd->blk) : false;
>> --
>> 2.15.1
>>
>>

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

[Philippe Mathieu-Daudé]

Hi Alistair,

On 01/31/2018 01:12 PM, Alistair Francis wrote:
> On Mon, Jan 22, 2018 at 7:21 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/sd.c         | 33 ++++++++++++++++++++++++++-------
>>  hw/sd/trace-events |  6 ++++++
>>  2 files changed, 32 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 55d2ba2dd7..f876973a2b 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,25 @@ 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";
>> +    }
> 
> There should be an assert here to make sure we never go off the end of
> this array.

This shouldn't happen since this function is local (static), but I'll
add a check.

> 
> Is this used in future? It seems like a lot of work for one caller.

This resulted useful to me when tracing (in particular when using the
MMC mode). Also note this code is only called when tracing is enabled.

http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg02387.html

> 
>> +    return state_name[state];
>> +}
>> +
>>  static uint8_t sd_get_dat_lines(SDState *sd)
>>  {
>>      return sd->enable ? sd->dat_lines : 0;
>> @@ -776,6 +796,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 +812,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 +1331,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 +1364,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 */
>> @@ -1594,7 +1613,7 @@ send_response:
>>          DPRINTF("Response:");
>>          for (i = 0; i < rsplen; i++)
>>              fprintf(stderr, " %02x", response[i]);
>> -        fprintf(stderr, " state %d\n", sd->state);
>> +        fputc('\n', stderr);
> 
> Why change this?

Probably leftover when rebasing, since trace_sdcard_normal_command() now
displays the state, I was not sure the best way to trace hexdump before
removing this debug code.

> 
> Alistair
> 
>>      } else {
>>          DPRINTF("No response %d\n", sd->state);
>>      }
>> @@ -1605,8 +1624,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");
>>      }
>> @@ -1614,6 +1632,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.15.1
>>
>>