* [Qemu-devel] [PATCH v4 01/11] sdcard: reorder SDState struct members
2018-02-15 22:05 [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4) Philippe Mathieu-Daudé
@ 2018-02-15 22:05 ` Philippe Mathieu-Daudé
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 02/11] sdcard: replace DPRINTF() by trace events Philippe Mathieu-Daudé
` (11 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-15 22:05 UTC (permalink / raw)
To: Alistair Francis, Peter Maydell, Igor Mitsyanko
Cc: Philippe Mathieu-Daudé,
qemu-devel, Edgar E . Iglesias, Prasad J Pandit,
Andrzej Zaborowski
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v4 02/11] sdcard: replace DPRINTF() by trace events
2018-02-15 22:05 [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4) Philippe Mathieu-Daudé
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 01/11] sdcard: reorder SDState struct members Philippe Mathieu-Daudé
@ 2018-02-15 22:05 ` Philippe Mathieu-Daudé
2018-02-16 21:01 ` Alistair Francis
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 03/11] sdcard: add a trace event for command responses Philippe Mathieu-Daudé
` (10 subsequent siblings)
12 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-15 22:05 UTC (permalink / raw)
To: Alistair Francis, Peter Maydell, Igor Mitsyanko
Cc: Philippe Mathieu-Daudé,
qemu-devel, Edgar E . Iglesias, Prasad J Pandit,
Andrzej Zaborowski
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v4 02/11] sdcard: replace DPRINTF() by trace events
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 02/11] sdcard: replace DPRINTF() by trace events Philippe Mathieu-Daudé
@ 2018-02-16 21:01 ` Alistair Francis
0 siblings, 0 replies; 22+ messages in thread
From: Alistair Francis @ 2018-02-16 21:01 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alistair Francis, Peter Maydell, Igor Mitsyanko,
Edgar E . Iglesias, Prasad J Pandit,
qemu-devel@nongnu.org Developers
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
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v4 03/11] sdcard: add a trace event for command responses
2018-02-15 22:05 [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4) Philippe Mathieu-Daudé
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 01/11] sdcard: reorder SDState struct members Philippe Mathieu-Daudé
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 02/11] sdcard: replace DPRINTF() by trace events Philippe Mathieu-Daudé
@ 2018-02-15 22:05 ` Philippe Mathieu-Daudé
2018-02-27 2:14 ` Alistair Francis
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 04/11] sdcard: replace fprintf() by qemu_hexdump() Philippe Mathieu-Daudé
` (9 subsequent siblings)
12 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-15 22:05 UTC (permalink / raw)
To: Alistair Francis, Peter Maydell, Igor Mitsyanko
Cc: Philippe Mathieu-Daudé,
qemu-devel, Edgar E . Iglesias, Prasad J Pandit,
Andrzej Zaborowski
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v4 03/11] sdcard: add a trace event for command responses
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 03/11] sdcard: add a trace event for command responses Philippe Mathieu-Daudé
@ 2018-02-27 2:14 ` Alistair Francis
0 siblings, 0 replies; 22+ messages in thread
From: Alistair Francis @ 2018-02-27 2:14 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alistair Francis, Peter Maydell, Igor Mitsyanko,
Edgar E . Iglesias, Prasad J Pandit,
qemu-devel@nongnu.org Developers
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
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v4 04/11] sdcard: replace fprintf() by qemu_hexdump()
2018-02-15 22:05 [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4) Philippe Mathieu-Daudé
` (2 preceding siblings ...)
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 03/11] sdcard: add a trace event for command responses Philippe Mathieu-Daudé
@ 2018-02-15 22:05 ` Philippe Mathieu-Daudé
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 05/11] sdcard: add more trace events Philippe Mathieu-Daudé
` (8 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-15 22:05 UTC (permalink / raw)
To: Alistair Francis, Peter Maydell, Igor Mitsyanko
Cc: Philippe Mathieu-Daudé,
qemu-devel, Edgar E . Iglesias, Prasad J Pandit,
Andrzej Zaborowski
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v4 05/11] sdcard: add more trace events
2018-02-15 22:05 [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4) Philippe Mathieu-Daudé
` (3 preceding siblings ...)
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 04/11] sdcard: replace fprintf() by qemu_hexdump() Philippe Mathieu-Daudé
@ 2018-02-15 22:05 ` Philippe Mathieu-Daudé
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 06/11] sdcard: do not trace CMD55 when expecting ACMD Philippe Mathieu-Daudé
` (7 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-15 22:05 UTC (permalink / raw)
To: Alistair Francis, Peter Maydell, Igor Mitsyanko
Cc: Philippe Mathieu-Daudé,
qemu-devel, Edgar E . Iglesias, Prasad J Pandit,
Andrzej Zaborowski
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, §);
} 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
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v4 06/11] sdcard: do not trace CMD55 when expecting ACMD
2018-02-15 22:05 [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4) Philippe Mathieu-Daudé
` (4 preceding siblings ...)
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 05/11] sdcard: add more trace events Philippe Mathieu-Daudé
@ 2018-02-15 22:05 ` Philippe Mathieu-Daudé
2018-02-22 12:01 ` Peter Maydell
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 07/11] sdcard: define SDMMC_CMD_MAX instead of using the magic '64' Philippe Mathieu-Daudé
` (6 subsequent siblings)
12 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-15 22:05 UTC (permalink / raw)
To: Alistair Francis, Peter Maydell, Igor Mitsyanko
Cc: Philippe Mathieu-Daudé,
qemu-devel, Edgar E . Iglesias, Prasad J Pandit,
Andrzej Zaborowski
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v4 06/11] sdcard: do not trace CMD55 when expecting ACMD
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 06/11] sdcard: do not trace CMD55 when expecting ACMD Philippe Mathieu-Daudé
@ 2018-02-22 12:01 ` Peter Maydell
2018-03-09 14:07 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2018-02-22 12:01 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alistair Francis, Igor Mitsyanko, QEMU Developers,
Edgar E . Iglesias, Prasad J Pandit, Andrzej Zaborowski
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v4 06/11] sdcard: do not trace CMD55 when expecting ACMD
2018-02-22 12:01 ` Peter Maydell
@ 2018-03-09 14:07 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-03-09 14:07 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Igor Mitsyanko, QEMU Developers,
Edgar E . Iglesias, Prasad J Pandit, Andrzej Zaborowski
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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v4 07/11] sdcard: define SDMMC_CMD_MAX instead of using the magic '64'
2018-02-15 22:05 [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4) Philippe Mathieu-Daudé
` (5 preceding siblings ...)
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 06/11] sdcard: do not trace CMD55 when expecting ACMD Philippe Mathieu-Daudé
@ 2018-02-15 22:05 ` Philippe Mathieu-Daudé
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 08/11] sdcard: display command name when tracing CMD/ACMD Philippe Mathieu-Daudé
` (5 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-15 22:05 UTC (permalink / raw)
To: Alistair Francis, Peter Maydell, Igor Mitsyanko
Cc: Philippe Mathieu-Daudé,
qemu-devel, Edgar E . Iglesias, Prasad J Pandit,
Andrzej Zaborowski
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v4 08/11] sdcard: display command name when tracing CMD/ACMD
2018-02-15 22:05 [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4) Philippe Mathieu-Daudé
` (6 preceding siblings ...)
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 07/11] sdcard: define SDMMC_CMD_MAX instead of using the magic '64' Philippe Mathieu-Daudé
@ 2018-02-15 22:05 ` Philippe Mathieu-Daudé
2018-02-22 12:59 ` Peter Maydell
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 09/11] sdcard: display protocol used when tracing Philippe Mathieu-Daudé
` (4 subsequent siblings)
12 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-15 22:05 UTC (permalink / raw)
To: Alistair Francis, Peter Maydell, Igor Mitsyanko
Cc: Philippe Mathieu-Daudé,
qemu-devel, Edgar E . Iglesias, Prasad J Pandit,
Andrzej Zaborowski
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v4 08/11] sdcard: display command name when tracing CMD/ACMD
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 08/11] sdcard: display command name when tracing CMD/ACMD Philippe Mathieu-Daudé
@ 2018-02-22 12:59 ` Peter Maydell
2018-03-09 14:51 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2018-02-22 12:59 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alistair Francis, Igor Mitsyanko, QEMU Developers,
Edgar E . Iglesias, Prasad J Pandit, Andrzej Zaborowski
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v4 08/11] sdcard: display command name when tracing CMD/ACMD
2018-02-22 12:59 ` Peter Maydell
@ 2018-03-09 14:51 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-03-09 14:51 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Igor Mitsyanko, QEMU Developers,
Edgar E . Iglesias, Prasad J Pandit, Andrzej Zaborowski
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
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v4 09/11] sdcard: display protocol used when tracing
2018-02-15 22:05 [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4) Philippe Mathieu-Daudé
` (7 preceding siblings ...)
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 08/11] sdcard: display command name when tracing CMD/ACMD Philippe Mathieu-Daudé
@ 2018-02-15 22:05 ` Philippe Mathieu-Daudé
2018-02-22 13:02 ` Peter Maydell
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 10/11] sdcard: use G_BYTE from cutils Philippe Mathieu-Daudé
` (3 subsequent siblings)
12 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-15 22:05 UTC (permalink / raw)
To: Alistair Francis, Peter Maydell, Igor Mitsyanko
Cc: Philippe Mathieu-Daudé,
qemu-devel, Edgar E . Iglesias, Prasad J Pandit,
Andrzej Zaborowski
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v4 09/11] sdcard: display protocol used when tracing
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 09/11] sdcard: display protocol used when tracing Philippe Mathieu-Daudé
@ 2018-02-22 13:02 ` Peter Maydell
2018-03-03 12:09 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2018-02-22 13:02 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alistair Francis, Igor Mitsyanko, QEMU Developers,
Edgar E . Iglesias, Prasad J Pandit, Andrzej Zaborowski
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v4 09/11] sdcard: display protocol used when tracing
2018-02-22 13:02 ` Peter Maydell
@ 2018-03-03 12:09 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-03-03 12:09 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Igor Mitsyanko, QEMU Developers,
Edgar E . Iglesias, Prasad J Pandit, Andrzej Zaborowski
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!
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v4 10/11] sdcard: use G_BYTE from cutils
2018-02-15 22:05 [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4) Philippe Mathieu-Daudé
` (8 preceding siblings ...)
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 09/11] sdcard: display protocol used when tracing Philippe Mathieu-Daudé
@ 2018-02-15 22:05 ` Philippe Mathieu-Daudé
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 11/11] sdcard: use the registerfields API to access the OCR register Philippe Mathieu-Daudé
` (2 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-15 22:05 UTC (permalink / raw)
To: Alistair Francis, Peter Maydell, Igor Mitsyanko
Cc: Philippe Mathieu-Daudé,
qemu-devel, Edgar E . Iglesias, Prasad J Pandit,
Andrzej Zaborowski
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v4 11/11] sdcard: use the registerfields API to access the OCR register
2018-02-15 22:05 [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4) Philippe Mathieu-Daudé
` (9 preceding siblings ...)
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 10/11] sdcard: use G_BYTE from cutils Philippe Mathieu-Daudé
@ 2018-02-15 22:05 ` Philippe Mathieu-Daudé
2018-02-22 9:04 ` [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4) Philippe Mathieu-Daudé
2018-02-22 13:08 ` Peter Maydell
12 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-15 22:05 UTC (permalink / raw)
To: Alistair Francis, Peter Maydell, Igor Mitsyanko
Cc: Philippe Mathieu-Daudé,
qemu-devel, Edgar E . Iglesias, Prasad J Pandit,
Andrzej Zaborowski
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4)
2018-02-15 22:05 [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4) Philippe Mathieu-Daudé
` (10 preceding siblings ...)
2018-02-15 22:05 ` [Qemu-devel] [PATCH v4 11/11] sdcard: use the registerfields API to access the OCR register Philippe Mathieu-Daudé
@ 2018-02-22 9:04 ` Philippe Mathieu-Daudé
2018-02-22 13:08 ` Peter Maydell
12 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-22 9:04 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Igor Mitsyanko, qemu-devel, Edgar E . Iglesias,
Prasad J Pandit, Andrzej Zaborowski
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
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4)
2018-02-15 22:05 [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4) Philippe Mathieu-Daudé
` (11 preceding siblings ...)
2018-02-22 9:04 ` [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4) Philippe Mathieu-Daudé
@ 2018-02-22 13:08 ` Peter Maydell
12 siblings, 0 replies; 22+ messages in thread
From: Peter Maydell @ 2018-02-22 13:08 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alistair Francis, Igor Mitsyanko, QEMU Developers,
Edgar E . Iglesias, Prasad J Pandit, Andrzej Zaborowski
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
^ permalink raw reply [flat|nested] 22+ messages in thread