[Qemu-devel] [PATCH v2 0/4] SDCard: support UHS-I

[Qemu-devel] [PATCH v2 0/4] SDCard: support UHS-I

[Philippe Mathieu-Daudé]

Since v1:
- corrected the CRC16 offset
- addressed Peter Maydell comments
  - add Edgar Iglesias S-o-b for Alistair Francis original patch at Xilinx
  - use SD_FG_MAX to have a clearer loop
  - rename uhs_enabled/uhs_mode and add documentation
  - check all functions in group are correct before setting the values

v1 here: http://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg02828.html

Regards,

Phil.

Philippe Mathieu-Daudé (4):
  sdcard: Update the SDState documentation
  sdcard: Correct CRC16 offset in sd_function_switch()
  sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)
  sdcard: Add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit

 hw/sd/sd.c         | 210 +++++++++++++++++++++++++++++++++++++++------
 hw/sd/trace-events |   1 +
 2 files changed, 183 insertions(+), 28 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH v2 1/4] sdcard: Update the SDState documentation

[Philippe Mathieu-Daudé]

Add more descriptive comments to keep a clear separation
between static property vs runtime changeable.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 235e0518d6..5fb4787671 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -90,12 +90,15 @@ struct SDState {
     uint32_t card_status;
     uint8_t sd_status[64];
 
-    /* Configurable properties */
+    /* Static properties */
+
     BlockBackend *blk;
     bool spi;
 
-    uint32_t mode;    /* current card mode, one of SDCardModes */
-    int32_t state;    /* current card state, one of SDCardStates */
+    /* Runtime changeables */
+
+    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;
@@ -109,8 +112,9 @@ struct SDState {
     uint32_t pwd_len;
     uint8_t function_group[6];
     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!
+    /**
+     * #True if we will handle the next command as an ACMD.
+     * Note that this does *not* track the APP_CMD status bit!
      */
     bool expecting_acmd;
     uint32_t blk_written;
-- 
2.17.0

[Qemu-devel] [PATCH v2 2/4] sdcard: Correct CRC16 offset in sd_function_switch()

[Philippe Mathieu-Daudé]

Per the Physical Layer Simplified Spec. "4.3.10.4 Switch Function Status":

  The block length is predefined to 512 bits

and "4.10.2 SD Status":

  The SD Status contains status bits that are related to the SD Memory Card
  proprietary features and may be used for future application-specific usage.
  The size of the SD Status is one data block of 512 bit. The content of this
  register is transmitted to the Host over the DAT bus along with a 16-bit CRC.

Thus the 16-bit CRC goes at offset 64.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 5fb4787671..24aaf0c767 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -791,7 +791,7 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
         sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
     }
     memset(&sd->data[17], 0, 47);
-    stw_be_p(sd->data + 65, sd_crc16(sd->data, 64));
+    stw_be_p(sd->data + 64, sd_crc16(sd->data, 64));
 }
 
 static inline bool sd_wp_addr(SDState *sd, uint64_t addr)
-- 
2.17.0

[Qemu-devel] [PATCH v2 3/4] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)

[Philippe Mathieu-Daudé]

[based on a patch from Alistair Francis <alistair.francis@xilinx.com>
 from qemu/xilinx tag xilinx-v2015.2]
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
[PMD: rebased, changed magic by definitions, use stw_be_p, add tracing,
 check all functions in group are correct before setting the values]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c         | 186 +++++++++++++++++++++++++++++++++++++++------
 hw/sd/trace-events |   1 +
 2 files changed, 165 insertions(+), 22 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 24aaf0c767..d8dd88f872 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -128,6 +128,11 @@ struct SDState {
     bool enable;
     uint8_t dat_lines;
     bool cmd_line;
+    /**
+     * Whether the card entered the UHS mode with voltage level adjusted
+     * (use by soft-reset)
+     */
+    bool uhs_activated;
 };
 
 static const char *sd_state_name(enum SDCardStates state)
@@ -567,6 +572,7 @@ static void sd_reset(DeviceState *dev)
     sd->expecting_acmd = false;
     sd->dat_lines = 0xf;
     sd->cmd_line = true;
+    sd->uhs_activated = false;
     sd->multi_blk_cnt = 0;
 }
 
@@ -765,32 +771,168 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
     return ret;
 }
 
+/* Function Group */
+enum {
+    SD_FG_MIN               = 1,
+    SD_FG_ACCESS_MODE       = 1,
+    SD_FG_COMMAND_SYSTEM    = 2,
+    SD_FG_DRIVER_STRENGTH   = 3,
+    SD_FG_CURRENT_LIMIT     = 4,
+    SD_FG_RSVD_5            = 5,
+    SD_FG_RSVD_6            = 6,
+    SD_FG_MAX               = 6,
+    SD_FG_COUNT
+};
+
+static const char *sd_fn_grp_name[SD_FG_COUNT] = {
+    [SD_FG_ACCESS_MODE]     = "ACCESS_MODE",
+    [SD_FG_COMMAND_SYSTEM]  = "COMMAND_SYSTEM",
+    [SD_FG_DRIVER_STRENGTH] = "DRIVER_STRENGTH",
+    [SD_FG_CURRENT_LIMIT]   = "CURRENT_LIMIT",
+    [SD_FG_RSVD_5]          = "RSVD5",
+    [SD_FG_RSVD_6]          = "RSVD6",
+};
+
+/* Function name */
+enum {
+    SD_FN_NO_INFLUENCE      = 0xf,
+    SD_FN_COUNT             = 16
+};
+
+typedef struct sd_fn_support {
+    const char *name;
+    bool uhs_only;
+    bool unimp;
+} sd_fn_support;
+
+static const sd_fn_support *sd_fn_support_defs[SD_FG_COUNT] = {
+    [SD_FG_ACCESS_MODE] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default/SDR12" },
+        [1] = { .name = "high-speed/SDR25" },
+        [2] = { .name = "SDR50",    .uhs_only = true },
+        [3] = { .name = "SDR104",   .uhs_only = true },
+        [4] = { .name = "DDR50",    .uhs_only = true },
+    },
+    [SD_FG_COMMAND_SYSTEM] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default" },
+        [1] = { .name = "For eC" },
+        [3] = { .name = "OTP",      .unimp = true },
+        [4] = { .name = "ASSD",     .unimp = true },
+    },
+    [SD_FG_DRIVER_STRENGTH] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default/Type B" },
+        [1] = { .name = "Type A",   .uhs_only = true },
+        [2] = { .name = "Type C",   .uhs_only = true },
+        [3] = { .name = "Type D",   .uhs_only = true },
+    },
+    [SD_FG_CURRENT_LIMIT] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default/200mA" },
+        [1] = { .name = "400mA",    .uhs_only = true },
+        [2] = { .name = "600mA",    .uhs_only = true },
+        [3] = { .name = "800mA",    .uhs_only = true },
+    },
+    [SD_FG_RSVD_5] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default" },
+    },
+    [SD_FG_RSVD_6] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default" },
+    },
+};
+
+#define SD_FN_BUFSZ                 (SD_FG_MAX * 4 / BITS_PER_BYTE)
+
 static void sd_function_switch(SDState *sd, uint32_t arg)
 {
-    int i, mode, new_func;
-    mode = !!(arg & 0x80000000);
-
-    sd->data[0] = 0x00;		/* Maximum current consumption */
-    sd->data[1] = 0x01;
-    sd->data[2] = 0x80;		/* Supported group 6 functions */
-    sd->data[3] = 0x01;
-    sd->data[4] = 0x80;		/* Supported group 5 functions */
-    sd->data[5] = 0x01;
-    sd->data[6] = 0x80;		/* Supported group 4 functions */
-    sd->data[7] = 0x01;
-    sd->data[8] = 0x80;		/* Supported group 3 functions */
-    sd->data[9] = 0x01;
-    sd->data[10] = 0x80;	/* Supported group 2 functions */
-    sd->data[11] = 0x43;
-    sd->data[12] = 0x80;	/* Supported group 1 functions */
-    sd->data[13] = 0x03;
-    for (i = 0; i < 6; i ++) {
-        new_func = (arg >> (i * 4)) & 0x0f;
-        if (mode && new_func != 0x0f)
-            sd->function_group[i] = new_func;
-        sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
+    bool invalid_function_selected = false;
+    int fn_grp, new_func, i;
+    uint8_t *data_p, tmpbuf[SD_FN_BUFSZ];
+    uint16_t max_current_mA = 1;
+    bool mode = extract32(arg, 31, 1);  /* 0: check only, 1: do switch */
+
+    /* Bits 400-495: information (16 bits each group) */
+    data_p = &sd->data[2];
+    for (fn_grp = SD_FG_MAX; fn_grp >= SD_FG_MIN; fn_grp--) {
+        uint16_t supported_fns = 1 << SD_FN_NO_INFLUENCE;
+        for (i = 0; i < SD_FN_COUNT; ++i) {
+            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][i];
+
+            if (def->name && !def->unimp &&
+                    !(def->uhs_only && !sd->uhs_activated)) {
+                supported_fns |= 1 << i;
+            }
+        }
+        stw_be_p(data_p, supported_fns);
+        data_p += 2;
     }
+
+    /* Bits 376-399: function (4 bits each group)
+     *
+     * Do not write the values back directly:
+     * Check everything first writing to 'tmpbuf'
+     */
+    data_p = tmpbuf;
+    for (fn_grp = SD_FG_MAX; fn_grp >= SD_FG_MIN; fn_grp--) {
+        new_func = (arg >> ((fn_grp - 1) * 4)) & 0x0f;
+        if (new_func == SD_FN_NO_INFLUENCE) {
+            /* Guest requested no influence, so this function group
+             * stays the same.
+             */
+            new_func = sd->function_group[fn_grp - 1];
+        } else {
+            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][new_func];
+            if (mode) {
+                if (!def->name) {
+                    qemu_log_mask(LOG_GUEST_ERROR,
+                                  "Function %d not valid for "
+                                  "function group %d\n",
+                                  new_func, fn_grp);
+                    invalid_function_selected = true;
+                    new_func = SD_FN_NO_INFLUENCE;
+                } else if (def->unimp) {
+                    qemu_log_mask(LOG_UNIMP,
+                                  "Function %s (fn grp %d) not implemented\n",
+                                  def->name, fn_grp);
+                    invalid_function_selected = true;
+                    new_func = SD_FN_NO_INFLUENCE;
+                } else if (def->uhs_only && !sd->uhs_activated) {
+                    qemu_log_mask(LOG_GUEST_ERROR,
+                                  "Function %s (fn grp %d) only "
+                                  "valid in UHS mode\n",
+                                  def->name, fn_grp);
+                    invalid_function_selected = true;
+                    new_func = SD_FN_NO_INFLUENCE;
+                } else {
+                    sd->function_group[fn_grp - 1] = new_func;
+                }
+            }
+            trace_sdcard_function_select(def->name, sd_fn_grp_name[fn_grp],
+                                         mode);
+        }
+        if (!(fn_grp & 0x1)) { /* evens go in high nibble */
+            *data_p = new_func << 4;
+        } else { /* odds go in low nibble */
+            *(data_p++) |= new_func;
+        }
+    }
+    if (invalid_function_selected) {
+        /* Ignore all the set values */
+        memset(&sd->data[14], 0, SD_FN_BUFSZ);
+    } else {
+        /* Write back if all functions in the group are correct. */
+        memcpy(&sd->data[14], tmpbuf, SD_FN_BUFSZ);
+    }
+
+    /* Data Structure Version = 0x00: 376 bits reserved (undefined)  */
     memset(&sd->data[17], 0, 47);
+
+    if (invalid_function_selected) {
+        /* Maximum current consumption: If one of the selected functions
+         * was wrong, the return value will be 0.
+         */
+        max_current_mA = 0;
+    }
+    stw_be_p(sd->data, max_current_mA);
+
     stw_be_p(sd->data + 64, sd_crc16(sd->data, 64));
 }
 
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index bfd1d62efc..eba5ab7b90 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -48,6 +48,7 @@ sdcard_write_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
 sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint8_t value) "%s %20s/ CMD%02d value 0x%02x"
 sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, int length) "%s %20s/ CMD%02d len %d"
 sdcard_set_voltage(uint16_t millivolts) "%u mV"
+sdcard_function_select(const char *fn_name, const char *grp_name, bool do_switch) "Function %s (group: %s, sw: %u)"
 
 # hw/sd/milkymist-memcard.c
 milkymist_memcard_memory_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
-- 
2.17.0

[Qemu-devel] [PATCH v2 4/4] sdcard: Add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index d8dd88f872..a0e8a1b46a 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -94,6 +94,10 @@ struct SDState {
 
     BlockBackend *blk;
     bool spi;
+    /**
+     * The UHS mode the card supports if any, else #UHS_NOT_SUPPORTED
+     */
+    uint8_t uhs_mode_supported;
 
     /* Runtime changeables */
 
@@ -300,6 +304,9 @@ static void sd_set_ocr(SDState *sd)
 {
     /* All voltages OK */
     sd->ocr = R_OCR_VDD_VOLTAGE_WIN_HI_MASK;
+
+    sd->ocr = FIELD_DP32(sd->ocr, OCR, ACCEPT_SWITCH_1V8,
+                         sd->uhs_mode_supported != UHS_NOT_SUPPORTED);
 }
 
 static void sd_ocr_powerup(void *opaque)
@@ -2240,6 +2247,7 @@ static Property sd_properties[] = {
      * board to ensure that ssi transfers only occur when the chip select
      * is asserted.  */
     DEFINE_PROP_BOOL("spi", SDState, spi, false),
+    DEFINE_PROP_UINT8("uhs", SDState, uhs_mode_supported, UHS_NOT_SUPPORTED),
     DEFINE_PROP_END_OF_LIST()
 };
 
-- 
2.17.0

Re: [Qemu-devel] [PATCH v2 1/4] sdcard: Update the SDState documentation

[Alistair Francis]

On Tue, May 8, 2018 at 11:01 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Add more descriptive comments to keep a clear separation
> between static property vs runtime changeable.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 235e0518d6..5fb4787671 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -90,12 +90,15 @@ struct SDState {
>      uint32_t card_status;
>      uint8_t sd_status[64];
>
> -    /* Configurable properties */
> +    /* Static properties */
> +
>      BlockBackend *blk;
>      bool spi;
>
> -    uint32_t mode;    /* current card mode, one of SDCardModes */
> -    int32_t state;    /* current card state, one of SDCardStates */
> +    /* Runtime changeables */
> +
> +    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;
> @@ -109,8 +112,9 @@ struct SDState {
>      uint32_t pwd_len;
>      uint8_t function_group[6];
>      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!
> +    /**
> +     * #True if we will handle the next command as an ACMD.

Why do we need a # here?

Otherwise:

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

Alistair


> +     * Note that this does *not* track the APP_CMD status bit!
>       */
>      bool expecting_acmd;
>      uint32_t blk_written;
> --
> 2.17.0
>
>

Re: [Qemu-devel] [PATCH v2 1/4] sdcard: Update the SDState documentation

[Philippe Mathieu-Daudé]

On 05/09/2018 12:42 PM, Alistair Francis wrote:
> On Tue, May 8, 2018 at 11:01 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Add more descriptive comments to keep a clear separation
>> between static property vs runtime changeable.
>>
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/sd.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 235e0518d6..5fb4787671 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -90,12 +90,15 @@ struct SDState {
>>      uint32_t card_status;
>>      uint8_t sd_status[64];
>>
>> -    /* Configurable properties */
>> +    /* Static properties */
>> +
>>      BlockBackend *blk;
>>      bool spi;
>>
>> -    uint32_t mode;    /* current card mode, one of SDCardModes */
>> -    int32_t state;    /* current card state, one of SDCardStates */
>> +    /* Runtime changeables */
>> +
>> +    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;
>> @@ -109,8 +112,9 @@ struct SDState {
>>      uint32_t pwd_len;
>>      uint8_t function_group[6];
>>      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!
>> +    /**
>> +     * #True if we will handle the next command as an ACMD.
> 
> Why do we need a # here?

I used the CPUState as a documentation example, but this might not be
the correct use...

  /**
   * CPUState:
   * @running: #true if CPU is currently running (lockless).
   ...

$ git grep -Ei '#(true|false)'
include/exec/memory.h:164:         * If present, and returns #false, the
transaction is not accepted
include/qom/cpu.h:280: * @running: #true if CPU is currently running
(lockless).
include/qom/cpu.h:281: * @has_waiter: #true if a CPU is currently
waiting for the cpu_exec_end;

MemoryRegionOps does not use the double '/**' style:

    bool unaligned;
    /*
     * If present, and returns #false, the transaction is not accepted
     * by the device (and results in machine dependent behaviour such
     * as a machine check exception).
     */

Peter if you take this, feel free to drop this '#'.

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

Thanks!

> 
> Alistair
> 
> 
>> +     * Note that this does *not* track the APP_CMD status bit!
>>       */
>>      bool expecting_acmd;
>>      uint32_t blk_written;
>> --
>> 2.17.0
>>
>>

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

[Peter Maydell]

On 9 May 2018 at 07:01, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> [based on a patch from Alistair Francis <alistair.francis@xilinx.com>
>  from qemu/xilinx tag xilinx-v2015.2]
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> [PMD: rebased, changed magic by definitions, use stw_be_p, add tracing,
>  check all functions in group are correct before setting the values]
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

> +    /* Bits 376-399: function (4 bits each group)
> +     *
> +     * Do not write the values back directly:
> +     * Check everything first writing to 'tmpbuf'
> +     */
> +    data_p = tmpbuf;

You don't need a tmpbuf here, because it doesn't matter if we
write something to the data array that it turns out we don't want
to write; we can always rewrite it later...

> +    for (fn_grp = SD_FG_MAX; fn_grp >= SD_FG_MIN; fn_grp--) {
> +        new_func = (arg >> ((fn_grp - 1) * 4)) & 0x0f;
> +        if (new_func == SD_FN_NO_INFLUENCE) {
> +            /* Guest requested no influence, so this function group
> +             * stays the same.
> +             */
> +            new_func = sd->function_group[fn_grp - 1];
> +        } else {
> +            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][new_func];
> +            if (mode) {
> +                if (!def->name) {
> +                    qemu_log_mask(LOG_GUEST_ERROR,
> +                                  "Function %d not valid for "
> +                                  "function group %d\n",
> +                                  new_func, fn_grp);
> +                    invalid_function_selected = true;
> +                    new_func = SD_FN_NO_INFLUENCE;
> +                } else if (def->unimp) {
> +                    qemu_log_mask(LOG_UNIMP,
> +                                  "Function %s (fn grp %d) not implemented\n",
> +                                  def->name, fn_grp);
> +                    invalid_function_selected = true;
> +                    new_func = SD_FN_NO_INFLUENCE;
> +                } else if (def->uhs_only && !sd->uhs_activated) {
> +                    qemu_log_mask(LOG_GUEST_ERROR,
> +                                  "Function %s (fn grp %d) only "
> +                                  "valid in UHS mode\n",
> +                                  def->name, fn_grp);
> +                    invalid_function_selected = true;
> +                    new_func = SD_FN_NO_INFLUENCE;
> +                } else {
> +                    sd->function_group[fn_grp - 1] = new_func;

...but don't want to update the function_group[n] to the new value until
we've checked that all the selected values in the command are OK,
so you either need a temporary for the new function values, or
you need to do one pass over the inputs to check and another one to set.

> +                }
> +            }
> +            trace_sdcard_function_select(def->name, sd_fn_grp_name[fn_grp],
> +                                         mode);
> +        }
> +        if (!(fn_grp & 0x1)) { /* evens go in high nibble */
> +            *data_p = new_func << 4;
> +        } else { /* odds go in low nibble */
> +            *(data_p++) |= new_func;
> +        }
> +    }
> +    if (invalid_function_selected) {
> +        /* Ignore all the set values */
> +        memset(&sd->data[14], 0, SD_FN_BUFSZ);

All-zeroes doesn't seem to match the spec. The spec says "The response
to an invalid selection of function will be 0xF", which is a bit unclear,
but has to mean at least that we return 0xf for the function groups which
were invalid selections. I'm not sure what we should return as status
for the function groups which weren't invalid; possibilities include:
 * 0xf
 * whatever the provided argument for that function group was
 * whatever the current status for that function group is

I don't suppose you're in a position to find out what an actual
hardware SD card does?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/4] sdcard: Update the SDState documentation

[Peter Maydell]

On 10 May 2018 at 01:24, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 05/09/2018 12:42 PM, Alistair Francis wrote:
>> On Tue, May 8, 2018 at 11:01 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> Add more descriptive comments to keep a clear separation
>>> between static property vs runtime changeable.

>> Why do we need a # here?
>
> I used the CPUState as a documentation example, but this might not be
> the correct use...

Our doc-comment syntax is rather inconsistent, because we've never
actually run a tool over it to autogenerate documentation from the
comments, and so there hasn't been anything syntax-checking them.
The original intention for the syntax was gtkdoc, I think, where
a leading '#' indicates a symbol that isn't a function, constant or
parameter. However, I think the most recent consensus is that we
should use kernel-doc format, which is similar but doesn't use the
'#' markup:
 https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt

(The plan being that as we switch over to Sphinx for our docs
tool workflow we can use the kernel's integration of kernel-doc
into sphinx.)

For this structure in particular, it is not a public struct,
but local to a .c file. I think that for those there is not
really any point in having any kind of doc-comment markup in
it at all (ie no '#', no leading '/**').

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 2/4] sdcard: Correct CRC16 offset in sd_function_switch()

[Peter Maydell]

On 9 May 2018 at 07:01, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Per the Physical Layer Simplified Spec. "4.3.10.4 Switch Function Status":
>
>   The block length is predefined to 512 bits
>
> and "4.10.2 SD Status":
>
>   The SD Status contains status bits that are related to the SD Memory Card
>   proprietary features and may be used for future application-specific usage.
>   The size of the SD Status is one data block of 512 bit. The content of this
>   register is transmitted to the Host over the DAT bus along with a 16-bit CRC.
>
> Thus the 16-bit CRC goes at offset 64.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 5fb4787671..24aaf0c767 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -791,7 +791,7 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
>          sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
>      }
>      memset(&sd->data[17], 0, 47);
> -    stw_be_p(sd->data + 65, sd_crc16(sd->data, 64));
> +    stw_be_p(sd->data + 64, sd_crc16(sd->data, 64));
>  }
>
>  static inline bool sd_wp_addr(SDState *sd, uint64_t addr)
> --

Oops, yes, off-by-one error that's been present forever (it was
there before we did the conversion to stw_be_p()).

Applied this one to target-arm.next.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/4] sdcard: Update the SDState documentation

[Philippe Mathieu-Daudé]

On 05/14/2018 12:49 PM, Peter Maydell wrote:
> On 10 May 2018 at 01:24, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 05/09/2018 12:42 PM, Alistair Francis wrote:
>>> On Tue, May 8, 2018 at 11:01 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> Add more descriptive comments to keep a clear separation
>>>> between static property vs runtime changeable.
> 
>>> Why do we need a # here?
>>
>> I used the CPUState as a documentation example, but this might not be
>> the correct use...
> 
> Our doc-comment syntax is rather inconsistent, because we've never
> actually run a tool over it to autogenerate documentation from the
> comments, and so there hasn't been anything syntax-checking them.
> The original intention for the syntax was gtkdoc, I think, where
> a leading '#' indicates a symbol that isn't a function, constant or
> parameter. However, I think the most recent consensus is that we
> should use kernel-doc format, which is similar but doesn't use the
> '#' markup:
>  https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt

Ok, I'll use this guide from now on.

> (The plan being that as we switch over to Sphinx for our docs
> tool workflow we can use the kernel's integration of kernel-doc
> into sphinx.)
> 
> For this structure in particular, it is not a public struct,
> but local to a .c file. I think that for those there is not
> really any point in having any kind of doc-comment markup in
> it at all (ie no '#', no leading '/**').

Ok.

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

[Philippe Mathieu-Daudé]

On 05/14/2018 12:38 PM, Peter Maydell wrote:
> On 9 May 2018 at 07:01, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> [based on a patch from Alistair Francis <alistair.francis@xilinx.com>
>>  from qemu/xilinx tag xilinx-v2015.2]
>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>> [PMD: rebased, changed magic by definitions, use stw_be_p, add tracing,
>>  check all functions in group are correct before setting the values]
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
> 
>> +    /* Bits 376-399: function (4 bits each group)
>> +     *
>> +     * Do not write the values back directly:
>> +     * Check everything first writing to 'tmpbuf'
>> +     */
>> +    data_p = tmpbuf;
> 
> You don't need a tmpbuf here, because it doesn't matter if we
> write something to the data array that it turns out we don't want
> to write; we can always rewrite it later...
> 
>> +    for (fn_grp = SD_FG_MAX; fn_grp >= SD_FG_MIN; fn_grp--) {
>> +        new_func = (arg >> ((fn_grp - 1) * 4)) & 0x0f;
>> +        if (new_func == SD_FN_NO_INFLUENCE) {
>> +            /* Guest requested no influence, so this function group
>> +             * stays the same.
>> +             */
>> +            new_func = sd->function_group[fn_grp - 1];
>> +        } else {
>> +            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][new_func];
>> +            if (mode) {
>> +                if (!def->name) {
>> +                    qemu_log_mask(LOG_GUEST_ERROR,
>> +                                  "Function %d not valid for "
>> +                                  "function group %d\n",
>> +                                  new_func, fn_grp);
>> +                    invalid_function_selected = true;
>> +                    new_func = SD_FN_NO_INFLUENCE;
>> +                } else if (def->unimp) {
>> +                    qemu_log_mask(LOG_UNIMP,
>> +                                  "Function %s (fn grp %d) not implemented\n",
>> +                                  def->name, fn_grp);
>> +                    invalid_function_selected = true;
>> +                    new_func = SD_FN_NO_INFLUENCE;
>> +                } else if (def->uhs_only && !sd->uhs_activated) {
>> +                    qemu_log_mask(LOG_GUEST_ERROR,
>> +                                  "Function %s (fn grp %d) only "
>> +                                  "valid in UHS mode\n",
>> +                                  def->name, fn_grp);
>> +                    invalid_function_selected = true;
>> +                    new_func = SD_FN_NO_INFLUENCE;
>> +                } else {
>> +                    sd->function_group[fn_grp - 1] = new_func;
> 
> ...but don't want to update the function_group[n] to the new value until
> we've checked that all the selected values in the command are OK,
> so you either need a temporary for the new function values, or
> you need to do one pass over the inputs to check and another one to set.
> 
>> +                }
>> +            }
>> +            trace_sdcard_function_select(def->name, sd_fn_grp_name[fn_grp],
>> +                                         mode);
>> +        }
>> +        if (!(fn_grp & 0x1)) { /* evens go in high nibble */
>> +            *data_p = new_func << 4;
>> +        } else { /* odds go in low nibble */
>> +            *(data_p++) |= new_func;
>> +        }
>> +    }
>> +    if (invalid_function_selected) {
>> +        /* Ignore all the set values */
>> +        memset(&sd->data[14], 0, SD_FN_BUFSZ);
> 
> All-zeroes doesn't seem to match the spec. The spec says "The response
> to an invalid selection of function will be 0xF", which is a bit unclear,
> but has to mean at least that we return 0xf for the function groups which
> were invalid selections. I'm not sure what we should return as status
> for the function groups which weren't invalid; possibilities include:
>  * 0xf
>  * whatever the provided argument for that function group was
>  * whatever the current status for that function group is
If selection is 0xF (No influence) to query, response is
"whatever the current status for that function group is"
per group.

> 
> I don't suppose you're in a position to find out what an actual
> hardware SD card does?

I tested some SanDisk 'Ultra' card.

Tests output posted on this thread:
http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg04840.html

I'll now rework sd_function_switch() before to respin.

Regards,

Phil.

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

[Philippe Mathieu-Daudé]

On 05/22/2018 02:11 AM, Philippe Mathieu-Daudé wrote:
> On 05/14/2018 12:38 PM, Peter Maydell wrote:
>> On 9 May 2018 at 07:01, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> [based on a patch from Alistair Francis <alistair.francis@xilinx.com>
>>>  from qemu/xilinx tag xilinx-v2015.2]
>>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>> [PMD: rebased, changed magic by definitions, use stw_be_p, add tracing,
>>>  check all functions in group are correct before setting the values]
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>
>>> +    /* Bits 376-399: function (4 bits each group)
>>> +     *
>>> +     * Do not write the values back directly:
>>> +     * Check everything first writing to 'tmpbuf'
>>> +     */
>>> +    data_p = tmpbuf;
>>
>> You don't need a tmpbuf here, because it doesn't matter if we
>> write something to the data array that it turns out we don't want
>> to write; we can always rewrite it later...
>>
>>> +    for (fn_grp = SD_FG_MAX; fn_grp >= SD_FG_MIN; fn_grp--) {
>>> +        new_func = (arg >> ((fn_grp - 1) * 4)) & 0x0f;
>>> +        if (new_func == SD_FN_NO_INFLUENCE) {
>>> +            /* Guest requested no influence, so this function group
>>> +             * stays the same.
>>> +             */
>>> +            new_func = sd->function_group[fn_grp - 1];
>>> +        } else {
>>> +            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][new_func];
>>> +            if (mode) {
>>> +                if (!def->name) {
>>> +                    qemu_log_mask(LOG_GUEST_ERROR,
>>> +                                  "Function %d not valid for "
>>> +                                  "function group %d\n",
>>> +                                  new_func, fn_grp);
>>> +                    invalid_function_selected = true;
>>> +                    new_func = SD_FN_NO_INFLUENCE;
>>> +                } else if (def->unimp) {
>>> +                    qemu_log_mask(LOG_UNIMP,
>>> +                                  "Function %s (fn grp %d) not implemented\n",
>>> +                                  def->name, fn_grp);
>>> +                    invalid_function_selected = true;
>>> +                    new_func = SD_FN_NO_INFLUENCE;
>>> +                } else if (def->uhs_only && !sd->uhs_activated) {
>>> +                    qemu_log_mask(LOG_GUEST_ERROR,
>>> +                                  "Function %s (fn grp %d) only "
>>> +                                  "valid in UHS mode\n",
>>> +                                  def->name, fn_grp);
>>> +                    invalid_function_selected = true;
>>> +                    new_func = SD_FN_NO_INFLUENCE;
>>> +                } else {
>>> +                    sd->function_group[fn_grp - 1] = new_func;
>>
>> ...but don't want to update the function_group[n] to the new value until
>> we've checked that all the selected values in the command are OK,
>> so you either need a temporary for the new function values, or
>> you need to do one pass over the inputs to check and another one to set.
>>
>>> +                }
>>> +            }
>>> +            trace_sdcard_function_select(def->name, sd_fn_grp_name[fn_grp],
>>> +                                         mode);
>>> +        }
>>> +        if (!(fn_grp & 0x1)) { /* evens go in high nibble */
>>> +            *data_p = new_func << 4;
>>> +        } else { /* odds go in low nibble */
>>> +            *(data_p++) |= new_func;
>>> +        }
>>> +    }
>>> +    if (invalid_function_selected) {
>>> +        /* Ignore all the set values */
>>> +        memset(&sd->data[14], 0, SD_FN_BUFSZ);
>>
>> All-zeroes doesn't seem to match the spec. The spec says "The response
>> to an invalid selection of function will be 0xF", which is a bit unclear,
>> but has to mean at least that we return 0xf for the function groups which
>> were invalid selections. I'm not sure what we should return as status
>> for the function groups which weren't invalid; possibilities include:
>>  * 0xf
>>  * whatever the provided argument for that function group was
>>  * whatever the current status for that function group is
> If selection is 0xF (No influence) to query, response is
> "whatever the current status for that function group is"
> per group.

Select:
Current Limit: 400mA (function name 1 to group No 4, arg slice [15:12]),
Command system: Vendor specific (function name 0xF to group No 2, arg
slice [7:4])

(gdb) p/x (1 << 31) | (1 << 12) | (0xf << 4)
0x800010f0
>>> do_cmd(6, 0x800010f0)
SWITCH_FUNC CMD06(0x800010f0) crc7:0xb7
00008001800180018001c001800100f00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008120

0000 // 0mA
8001 //6
8001 //5
8001 //4
8001 //3
c001 //2
8001 //1
0 //6
0 //5
f //function group 4 "0xF shows function set error with the argument."
0 //3
0 //function group 2 "The function which is result of the switch command"
0 //1
00 // v0

So the function 0xF of group No 2 was not selected.

--

Now let's try with 2 invalid functions (3 for group 3, 9 for group 2).

(gdb) p/x (1 << 31) | (0 << 12) | (0x3 << 8) | (0x9 << 4)
0x80000390

>>> do_cmd(6, 0x80000390)
SWITCH_FUNC CMD06(0x80000390) crc7:0x53
0000
8001
8001
8001
8001
c001
8001
0 //6
0 //5
0 //4
f //function group 3 "0xF shows function set error with the argument"
f //function group 2 "0xF shows function set error with the argument"
0 //1
00 // v0

--

Group 1 & 2 with valid selection, group 3 invalid (0x3):

(gdb) p/x (1 << 31) | (0x3 << 8) | (0xE << 4) | (0xf << 0)
0x800003ef

>>> do_cmd(6, 0x800003ef)
SWITCH_FUNC CMD06(0x800003ef) crc7:0x23
00008001800180018001c0018001000f0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001431

0000
8001
8001
8001
8001
c001
8001
0 //6
0 //5
0 //4
f //function group 3 "0xF shows function set error with the argument"
0 //2
0 //1
00 // v0

> 
>>
>> I don't suppose you're in a position to find out what an actual
>> hardware SD card does?
> 
> I tested some SanDisk 'Ultra' card.
> 
> Tests output posted on this thread:
> http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg04840.html
> 
> I'll now rework sd_function_switch() before to respin.