[based on a patch from Alistair Francis <alistair.francis@xilinx.com>
 from qemu/xilinx tag xilinx-v2015.2]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c         | 148 +++++++++++++++++++++++++++++++++++++++++++++--------
 hw/sd/trace-events |   1 +
 2 files changed, 127 insertions(+), 22 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 235e0518d6..b907d62aef 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -124,6 +124,7 @@ struct SDState {
     bool enable;
     uint8_t dat_lines;
     bool cmd_line;
+    bool uhs_enabled;
 };
 
 static const char *sd_state_name(enum SDCardStates state)
@@ -563,6 +564,7 @@ static void sd_reset(DeviceState *dev)
     sd->expecting_acmd = false;
     sd->dat_lines = 0xf;
     sd->cmd_line = true;
+    sd->uhs_enabled = false;
     sd->multi_blk_cnt = 0;
 }
 
@@ -761,30 +763,132 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
     return ret;
 }
 
+/* Function Group */
+enum {
+    SD_FG_MIN               = 1,
+    SD_FG_ACCESS_MODE       = 1,
+    SD_FG_COMMAND_SYSTEM    = 2,
+    SD_FG_DRIVER_STRENGTH   = 3,
+    SD_FG_CURRENT_LIMIT     = 4,
+    SD_FG_RSVD_5            = 5,
+    SD_FG_RSVD_6            = 6,
+    SD_FG_COUNT
+};
+
+/* Function name */
+#define SD_FN_COUNT 16
+
+static const char *sd_fn_grp_name[SD_FG_COUNT] = {
+    [SD_FG_ACCESS_MODE]     = "ACCESS_MODE",
+    [SD_FG_COMMAND_SYSTEM]  = "COMMAND_SYSTEM",
+    [SD_FG_DRIVER_STRENGTH] = "DRIVER_STRENGTH",
+    [SD_FG_CURRENT_LIMIT]   = "CURRENT_LIMIT",
+    [SD_FG_RSVD_5]          = "RSVD5",
+    [SD_FG_RSVD_6]          = "RSVD6",
+};
+
+typedef struct sd_fn_support {
+    const char *name;
+    bool uhs_only;
+    bool unimp;
+} sd_fn_support;
+
+static const sd_fn_support *sd_fn_support_defs[SD_FG_COUNT] = {
+    [SD_FG_ACCESS_MODE] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default/SDR12" },
+        [1] = { .name = "high-speed/SDR25" },
+        [2] = { .name = "SDR50",    .uhs_only = true },
+        [3] = { .name = "SDR104",   .uhs_only = true },
+        [4] = { .name = "DDR50",    .uhs_only = true },
+    },
+    [SD_FG_COMMAND_SYSTEM] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default" },
+        [1] = { .name = "For eC" },
+        [3] = { .name = "OTP",      .unimp = true },
+        [4] = { .name = "ASSD",     .unimp = true },
+    },
+    [SD_FG_DRIVER_STRENGTH] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default/Type B" },
+        [1] = { .name = "Type A",   .uhs_only = true },
+        [2] = { .name = "Type C",   .uhs_only = true },
+        [3] = { .name = "Type D",   .uhs_only = true },
+    },
+    [SD_FG_CURRENT_LIMIT] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default/200mA" },
+        [1] = { .name = "400mA",    .uhs_only = true },
+        [2] = { .name = "600mA",    .uhs_only = true },
+        [3] = { .name = "800mA",    .uhs_only = true },
+    },
+    [SD_FG_RSVD_5] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default" },
+    },
+    [SD_FG_RSVD_6] = (sd_fn_support [SD_FN_COUNT]) {
+        [0] = { .name = "default" },
+    },
+};
+
+#define SD_FN_NO_INFLUENCE          (1 << 15)
+
 static void sd_function_switch(SDState *sd, uint32_t arg)
 {
-    int i, mode, new_func;
-    mode = !!(arg & 0x80000000);
-
-    sd->data[0] = 0x00;		/* Maximum current consumption */
-    sd->data[1] = 0x01;
-    sd->data[2] = 0x80;		/* Supported group 6 functions */
-    sd->data[3] = 0x01;
-    sd->data[4] = 0x80;		/* Supported group 5 functions */
-    sd->data[5] = 0x01;
-    sd->data[6] = 0x80;		/* Supported group 4 functions */
-    sd->data[7] = 0x01;
-    sd->data[8] = 0x80;		/* Supported group 3 functions */
-    sd->data[9] = 0x01;
-    sd->data[10] = 0x80;	/* Supported group 2 functions */
-    sd->data[11] = 0x43;
-    sd->data[12] = 0x80;	/* Supported group 1 functions */
-    sd->data[13] = 0x03;
-    for (i = 0; i < 6; i ++) {
-        new_func = (arg >> (i * 4)) & 0x0f;
-        if (mode && new_func != 0x0f)
-            sd->function_group[i] = new_func;
-        sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
+    int fn_grp, new_func, i;
+    uint8_t *data_p;
+    bool mode = extract32(arg, 31, 1);  /* 0: check only, 1: do switch */
+
+    stw_be_p(sd->data + 0, 0x0001);     /* Maximum current consumption */
+
+    data_p = &sd->data[2];
+    for (fn_grp = SD_FG_COUNT - 1; fn_grp >= SD_FG_MIN; fn_grp--) {
+        uint16_t supported_fns = SD_FN_NO_INFLUENCE;
+        for (i = 0; i < SD_FN_COUNT; ++i) {
+            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][i];
+
+            if (def->name && !def->unimp &&
+                    !(def->uhs_only && !sd->uhs_enabled)) {
+                supported_fns |= 1 << i;
+            }
+        }
+        stw_be_p(data_p, supported_fns);
+        data_p += 2;
+    }
+
+    assert(data_p == &sd->data[14]);
+    for (fn_grp = SD_FG_COUNT - 1; fn_grp >= SD_FG_MIN; fn_grp--) {
+        new_func = (arg >> ((fn_grp - 1) * 4)) & 0x0f;
+        if (new_func == 0xf) {
+            new_func = sd->function_group[fn_grp - 1];
+        } else {
+            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][new_func];
+            if (mode) {
+                if (!def->name) {
+                    qemu_log_mask(LOG_GUEST_ERROR,
+                                  "Function %d not a valid for "
+                                  "function group %d\n",
+                                  new_func, fn_grp);
+                    new_func = 0xf;
+                } else if (def->unimp) {
+                    qemu_log_mask(LOG_UNIMP,
+                                  "Function %s (fn grp %d) not implemented\n",
+                                  def->name, fn_grp);
+                    new_func = 0xf;
+                } else if (def->uhs_only && !sd->uhs_enabled) {
+                    qemu_log_mask(LOG_GUEST_ERROR,
+                                  "Function %s (fn grp %d) only "
+                                  "valid in UHS mode\n",
+                                  def->name, fn_grp);
+                    new_func = 0xf;
+                } else {
+                    sd->function_group[fn_grp - 1] = new_func;
+                }
+            }
+            trace_sdcard_function_select(def->name, sd_fn_grp_name[fn_grp],
+                                         mode);
+        }
+        if (!(fn_grp & 0x1)) { /* evens go in high nibble */
+            *data_p = new_func << 4;
+        } else { /* odds go in low nibble */
+            *(data_p++) |= new_func;
+        }
     }
     memset(&sd->data[17], 0, 47);
     stw_be_p(sd->data + 65, sd_crc16(sd->data, 64));
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 2059ace61f..c106541a47 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -42,6 +42,7 @@ sdcard_write_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
 sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint8_t value) "%s %20s/ CMD%02d value 0x%02x"
 sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, int length) "%s %20s/ CMD%02d len %d"
 sdcard_set_voltage(uint16_t millivolts) "%u mV"
+sdcard_function_select(const char *fn_name, const char *grp_name, bool do_switch) "Function %s (group: %s, sw: %u)"
 
 # hw/sd/milkymist-memcard.c
 milkymist_memcard_memory_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
-- 
2.16.2

On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> [based on a patch from Alistair Francis <alistair.francis@xilinx.com>
>  from qemu/xilinx tag xilinx-v2015.2]
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

> ---
>  hw/sd/sd.c         | 148 +++++++++++++++++++++++++++++++++++++++++++++--------
>  hw/sd/trace-events |   1 +
>  2 files changed, 127 insertions(+), 22 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 235e0518d6..b907d62aef 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -124,6 +124,7 @@ struct SDState {
>      bool enable;
>      uint8_t dat_lines;
>      bool cmd_line;
> +    bool uhs_enabled;
>  };
>
>  static const char *sd_state_name(enum SDCardStates state)
> @@ -563,6 +564,7 @@ static void sd_reset(DeviceState *dev)
>      sd->expecting_acmd = false;
>      sd->dat_lines = 0xf;
>      sd->cmd_line = true;
> +    sd->uhs_enabled = false;
>      sd->multi_blk_cnt = 0;
>  }
>
> @@ -761,30 +763,132 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
>      return ret;
>  }
>
> +/* Function Group */
> +enum {
> +    SD_FG_MIN               = 1,
> +    SD_FG_ACCESS_MODE       = 1,
> +    SD_FG_COMMAND_SYSTEM    = 2,
> +    SD_FG_DRIVER_STRENGTH   = 3,
> +    SD_FG_CURRENT_LIMIT     = 4,
> +    SD_FG_RSVD_5            = 5,
> +    SD_FG_RSVD_6            = 6,
> +    SD_FG_COUNT
> +};

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

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

> +
> +/* Function name */
> +#define SD_FN_COUNT 16
> +
> +static const char *sd_fn_grp_name[SD_FG_COUNT] = {
> +    [SD_FG_ACCESS_MODE]     = "ACCESS_MODE",
> +    [SD_FG_COMMAND_SYSTEM]  = "COMMAND_SYSTEM",
> +    [SD_FG_DRIVER_STRENGTH] = "DRIVER_STRENGTH",
> +    [SD_FG_CURRENT_LIMIT]   = "CURRENT_LIMIT",
> +    [SD_FG_RSVD_5]          = "RSVD5",
> +    [SD_FG_RSVD_6]          = "RSVD6",
> +};
> +
> +typedef struct sd_fn_support {
> +    const char *name;
> +    bool uhs_only;
> +    bool unimp;
> +} sd_fn_support;
> +
> +static const sd_fn_support *sd_fn_support_defs[SD_FG_COUNT] = {
> +    [SD_FG_ACCESS_MODE] = (sd_fn_support [SD_FN_COUNT]) {
> +        [0] = { .name = "default/SDR12" },
> +        [1] = { .name = "high-speed/SDR25" },
> +        [2] = { .name = "SDR50",    .uhs_only = true },
> +        [3] = { .name = "SDR104",   .uhs_only = true },
> +        [4] = { .name = "DDR50",    .uhs_only = true },
> +    },
> +    [SD_FG_COMMAND_SYSTEM] = (sd_fn_support [SD_FN_COUNT]) {
> +        [0] = { .name = "default" },
> +        [1] = { .name = "For eC" },
> +        [3] = { .name = "OTP",      .unimp = true },
> +        [4] = { .name = "ASSD",     .unimp = true },
> +    },
> +    [SD_FG_DRIVER_STRENGTH] = (sd_fn_support [SD_FN_COUNT]) {
> +        [0] = { .name = "default/Type B" },
> +        [1] = { .name = "Type A",   .uhs_only = true },
> +        [2] = { .name = "Type C",   .uhs_only = true },
> +        [3] = { .name = "Type D",   .uhs_only = true },
> +    },
> +    [SD_FG_CURRENT_LIMIT] = (sd_fn_support [SD_FN_COUNT]) {
> +        [0] = { .name = "default/200mA" },
> +        [1] = { .name = "400mA",    .uhs_only = true },
> +        [2] = { .name = "600mA",    .uhs_only = true },
> +        [3] = { .name = "800mA",    .uhs_only = true },
> +    },
> +    [SD_FG_RSVD_5] = (sd_fn_support [SD_FN_COUNT]) {
> +        [0] = { .name = "default" },
> +    },
> +    [SD_FG_RSVD_6] = (sd_fn_support [SD_FN_COUNT]) {
> +        [0] = { .name = "default" },
> +    },
> +};
> +
> +#define SD_FN_NO_INFLUENCE          (1 << 15)
> +
>  static void sd_function_switch(SDState *sd, uint32_t arg)
>  {
> -    int i, mode, new_func;
> -    mode = !!(arg & 0x80000000);
> -
> -    sd->data[0] = 0x00;                /* Maximum current consumption */
> -    sd->data[1] = 0x01;
> -    sd->data[2] = 0x80;                /* Supported group 6 functions */
> -    sd->data[3] = 0x01;
> -    sd->data[4] = 0x80;                /* Supported group 5 functions */
> -    sd->data[5] = 0x01;
> -    sd->data[6] = 0x80;                /* Supported group 4 functions */
> -    sd->data[7] = 0x01;
> -    sd->data[8] = 0x80;                /* Supported group 3 functions */
> -    sd->data[9] = 0x01;
> -    sd->data[10] = 0x80;       /* Supported group 2 functions */
> -    sd->data[11] = 0x43;
> -    sd->data[12] = 0x80;       /* Supported group 1 functions */
> -    sd->data[13] = 0x03;
> -    for (i = 0; i < 6; i ++) {
> -        new_func = (arg >> (i * 4)) & 0x0f;
> -        if (mode && new_func != 0x0f)
> -            sd->function_group[i] = new_func;
> -        sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
> +    int fn_grp, new_func, i;
> +    uint8_t *data_p;
> +    bool mode = extract32(arg, 31, 1);  /* 0: check only, 1: do switch */
> +
> +    stw_be_p(sd->data + 0, 0x0001);     /* Maximum current consumption */

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

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

> +
> +    data_p = &sd->data[2];
> +    for (fn_grp = SD_FG_COUNT - 1; fn_grp >= SD_FG_MIN; fn_grp--) {
> +        uint16_t supported_fns = SD_FN_NO_INFLUENCE;
> +        for (i = 0; i < SD_FN_COUNT; ++i) {
> +            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][i];
> +
> +            if (def->name && !def->unimp &&
> +                    !(def->uhs_only && !sd->uhs_enabled)) {
> +                supported_fns |= 1 << i;
> +            }
> +        }
> +        stw_be_p(data_p, supported_fns);
> +        data_p += 2;
> +    }
> +
> +    assert(data_p == &sd->data[14]);
> +    for (fn_grp = SD_FG_COUNT - 1; fn_grp >= SD_FG_MIN; fn_grp--) {
> +        new_func = (arg >> ((fn_grp - 1) * 4)) & 0x0f;
> +        if (new_func == 0xf) {

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

> +            new_func = sd->function_group[fn_grp - 1];
> +        } else {
> +            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][new_func];
> +            if (mode) {
> +                if (!def->name) {
> +                    qemu_log_mask(LOG_GUEST_ERROR,
> +                                  "Function %d not a valid for "

"not valid for"

> +                                  "function group %d\n",
> +                                  new_func, fn_grp);
> +                    new_func = 0xf;
> +                } else if (def->unimp) {
> +                    qemu_log_mask(LOG_UNIMP,
> +                                  "Function %s (fn grp %d) not implemented\n",
> +                                  def->name, fn_grp);
> +                    new_func = 0xf;
> +                } else if (def->uhs_only && !sd->uhs_enabled) {
> +                    qemu_log_mask(LOG_GUEST_ERROR,
> +                                  "Function %s (fn grp %d) only "
> +                                  "valid in UHS mode\n",
> +                                  def->name, fn_grp);
> +                    new_func = 0xf;
> +                } else {
> +                    sd->function_group[fn_grp - 1] = new_func;

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

> +                }
> +            }
> +            trace_sdcard_function_select(def->name, sd_fn_grp_name[fn_grp],
> +                                         mode);
> +        }
> +        if (!(fn_grp & 0x1)) { /* evens go in high nibble */
> +            *data_p = new_func << 4;
> +        } else { /* odds go in low nibble */
> +            *(data_p++) |= new_func;
> +        }
>      }
>      memset(&sd->data[17], 0, 47);
>      stw_be_p(sd->data + 65, sd_crc16(sd->data, 64));
> diff --git a/hw/sd/trace-events b/hw/sd/trace-events
> index 2059ace61f..c106541a47 100644
> --- a/hw/sd/trace-events
> +++ b/hw/sd/trace-events
> @@ -42,6 +42,7 @@ sdcard_write_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
>  sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint8_t value) "%s %20s/ CMD%02d value 0x%02x"
>  sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, int length) "%s %20s/ CMD%02d len %d"
>  sdcard_set_voltage(uint16_t millivolts) "%u mV"
> +sdcard_function_select(const char *fn_name, const char *grp_name, bool do_switch) "Function %s (group: %s, sw: %u)"
>
>  # hw/sd/milkymist-memcard.c
>  milkymist_memcard_memory_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
> --
> 2.16.2
>

thanks
-- PMM

On Fri, Mar 09, 2018 at 05:03:11PM +0000, Peter Maydell wrote:
> On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > [based on a patch from Alistair Francis <alistair.francis@xilinx.com>
> >  from qemu/xilinx tag xilinx-v2015.2]
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> This should ideally have a Signed-off-by: from somebody @xilinx.com as
> well as you, then.

Hi Philippe,

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

Cheers,
Edgar


> 
> > ---
> >  hw/sd/sd.c         | 148 +++++++++++++++++++++++++++++++++++++++++++++--------
> >  hw/sd/trace-events |   1 +
> >  2 files changed, 127 insertions(+), 22 deletions(-)
> >
> > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > index 235e0518d6..b907d62aef 100644
> > --- a/hw/sd/sd.c
> > +++ b/hw/sd/sd.c
> > @@ -124,6 +124,7 @@ struct SDState {
> >      bool enable;
> >      uint8_t dat_lines;
> >      bool cmd_line;
> > +    bool uhs_enabled;
> >  };
> >
> >  static const char *sd_state_name(enum SDCardStates state)
> > @@ -563,6 +564,7 @@ static void sd_reset(DeviceState *dev)
> >      sd->expecting_acmd = false;
> >      sd->dat_lines = 0xf;
> >      sd->cmd_line = true;
> > +    sd->uhs_enabled = false;
> >      sd->multi_blk_cnt = 0;
> >  }
> >
> > @@ -761,30 +763,132 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
> >      return ret;
> >  }
> >
> > +/* Function Group */
> > +enum {
> > +    SD_FG_MIN               = 1,
> > +    SD_FG_ACCESS_MODE       = 1,
> > +    SD_FG_COMMAND_SYSTEM    = 2,
> > +    SD_FG_DRIVER_STRENGTH   = 3,
> > +    SD_FG_CURRENT_LIMIT     = 4,
> > +    SD_FG_RSVD_5            = 5,
> > +    SD_FG_RSVD_6            = 6,
> > +    SD_FG_COUNT
> > +};
> 
> Since the minimum isn't 0, this means SD_FG_COUNT isn't
> actually the count of the number of FGs. I think having
>     SD_FG_MAX = 6,
> 
> would make the loops below clearer, since they become
>   for (fn_grp = SD_FG_MAX; fn_grp >= SD_FG_MIN; fn_grp--) {
> 
> > +
> > +/* Function name */
> > +#define SD_FN_COUNT 16
> > +
> > +static const char *sd_fn_grp_name[SD_FG_COUNT] = {
> > +    [SD_FG_ACCESS_MODE]     = "ACCESS_MODE",
> > +    [SD_FG_COMMAND_SYSTEM]  = "COMMAND_SYSTEM",
> > +    [SD_FG_DRIVER_STRENGTH] = "DRIVER_STRENGTH",
> > +    [SD_FG_CURRENT_LIMIT]   = "CURRENT_LIMIT",
> > +    [SD_FG_RSVD_5]          = "RSVD5",
> > +    [SD_FG_RSVD_6]          = "RSVD6",
> > +};
> > +
> > +typedef struct sd_fn_support {
> > +    const char *name;
> > +    bool uhs_only;
> > +    bool unimp;
> > +} sd_fn_support;
> > +
> > +static const sd_fn_support *sd_fn_support_defs[SD_FG_COUNT] = {
> > +    [SD_FG_ACCESS_MODE] = (sd_fn_support [SD_FN_COUNT]) {
> > +        [0] = { .name = "default/SDR12" },
> > +        [1] = { .name = "high-speed/SDR25" },
> > +        [2] = { .name = "SDR50",    .uhs_only = true },
> > +        [3] = { .name = "SDR104",   .uhs_only = true },
> > +        [4] = { .name = "DDR50",    .uhs_only = true },
> > +    },
> > +    [SD_FG_COMMAND_SYSTEM] = (sd_fn_support [SD_FN_COUNT]) {
> > +        [0] = { .name = "default" },
> > +        [1] = { .name = "For eC" },
> > +        [3] = { .name = "OTP",      .unimp = true },
> > +        [4] = { .name = "ASSD",     .unimp = true },
> > +    },
> > +    [SD_FG_DRIVER_STRENGTH] = (sd_fn_support [SD_FN_COUNT]) {
> > +        [0] = { .name = "default/Type B" },
> > +        [1] = { .name = "Type A",   .uhs_only = true },
> > +        [2] = { .name = "Type C",   .uhs_only = true },
> > +        [3] = { .name = "Type D",   .uhs_only = true },
> > +    },
> > +    [SD_FG_CURRENT_LIMIT] = (sd_fn_support [SD_FN_COUNT]) {
> > +        [0] = { .name = "default/200mA" },
> > +        [1] = { .name = "400mA",    .uhs_only = true },
> > +        [2] = { .name = "600mA",    .uhs_only = true },
> > +        [3] = { .name = "800mA",    .uhs_only = true },
> > +    },
> > +    [SD_FG_RSVD_5] = (sd_fn_support [SD_FN_COUNT]) {
> > +        [0] = { .name = "default" },
> > +    },
> > +    [SD_FG_RSVD_6] = (sd_fn_support [SD_FN_COUNT]) {
> > +        [0] = { .name = "default" },
> > +    },
> > +};
> > +
> > +#define SD_FN_NO_INFLUENCE          (1 << 15)
> > +
> >  static void sd_function_switch(SDState *sd, uint32_t arg)
> >  {
> > -    int i, mode, new_func;
> > -    mode = !!(arg & 0x80000000);
> > -
> > -    sd->data[0] = 0x00;                /* Maximum current consumption */
> > -    sd->data[1] = 0x01;
> > -    sd->data[2] = 0x80;                /* Supported group 6 functions */
> > -    sd->data[3] = 0x01;
> > -    sd->data[4] = 0x80;                /* Supported group 5 functions */
> > -    sd->data[5] = 0x01;
> > -    sd->data[6] = 0x80;                /* Supported group 4 functions */
> > -    sd->data[7] = 0x01;
> > -    sd->data[8] = 0x80;                /* Supported group 3 functions */
> > -    sd->data[9] = 0x01;
> > -    sd->data[10] = 0x80;       /* Supported group 2 functions */
> > -    sd->data[11] = 0x43;
> > -    sd->data[12] = 0x80;       /* Supported group 1 functions */
> > -    sd->data[13] = 0x03;
> > -    for (i = 0; i < 6; i ++) {
> > -        new_func = (arg >> (i * 4)) & 0x0f;
> > -        if (mode && new_func != 0x0f)
> > -            sd->function_group[i] = new_func;
> > -        sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
> > +    int fn_grp, new_func, i;
> > +    uint8_t *data_p;
> > +    bool mode = extract32(arg, 31, 1);  /* 0: check only, 1: do switch */
> > +
> > +    stw_be_p(sd->data + 0, 0x0001);     /* Maximum current consumption */
> 
> Previously we were writing 0x00, 0x01 to the first 2 bytes of data;
> now we will write 0x01, 0x00. Are you sure that's right ? I guess
> it's the difference between claiming 1mA and 256mA.
> (I can't make any sense of the table in the spec so I have no idea.)
> 
> Also the spec says that if the guest selects an invalid function then
> this value should be 0.
> 
> > +
> > +    data_p = &sd->data[2];
> > +    for (fn_grp = SD_FG_COUNT - 1; fn_grp >= SD_FG_MIN; fn_grp--) {
> > +        uint16_t supported_fns = SD_FN_NO_INFLUENCE;
> > +        for (i = 0; i < SD_FN_COUNT; ++i) {
> > +            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][i];
> > +
> > +            if (def->name && !def->unimp &&
> > +                    !(def->uhs_only && !sd->uhs_enabled)) {
> > +                supported_fns |= 1 << i;
> > +            }
> > +        }
> > +        stw_be_p(data_p, supported_fns);
> > +        data_p += 2;
> > +    }
> > +
> > +    assert(data_p == &sd->data[14]);
> > +    for (fn_grp = SD_FG_COUNT - 1; fn_grp >= SD_FG_MIN; fn_grp--) {
> > +        new_func = (arg >> ((fn_grp - 1) * 4)) & 0x0f;
> > +        if (new_func == 0xf) {
> 
> /* Guest requested no influence, so this function group stays the same */
> 
> > +            new_func = sd->function_group[fn_grp - 1];
> > +        } else {
> > +            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][new_func];
> > +            if (mode) {
> > +                if (!def->name) {
> > +                    qemu_log_mask(LOG_GUEST_ERROR,
> > +                                  "Function %d not a valid for "
> 
> "not valid for"
> 
> > +                                  "function group %d\n",
> > +                                  new_func, fn_grp);
> > +                    new_func = 0xf;
> > +                } else if (def->unimp) {
> > +                    qemu_log_mask(LOG_UNIMP,
> > +                                  "Function %s (fn grp %d) not implemented\n",
> > +                                  def->name, fn_grp);
> > +                    new_func = 0xf;
> > +                } else if (def->uhs_only && !sd->uhs_enabled) {
> > +                    qemu_log_mask(LOG_GUEST_ERROR,
> > +                                  "Function %s (fn grp %d) only "
> > +                                  "valid in UHS mode\n",
> > +                                  def->name, fn_grp);
> > +                    new_func = 0xf;
> > +                } else {
> > +                    sd->function_group[fn_grp - 1] = new_func;
> 
> I think the spec says that if the guest makes an invalid selection
> for one function in the group then we must ignore all the set values,
> not just the one that was wrong, so we need to check everything
> first before we start writing the new values back.
> 
> > +                }
> > +            }
> > +            trace_sdcard_function_select(def->name, sd_fn_grp_name[fn_grp],
> > +                                         mode);
> > +        }
> > +        if (!(fn_grp & 0x1)) { /* evens go in high nibble */
> > +            *data_p = new_func << 4;
> > +        } else { /* odds go in low nibble */
> > +            *(data_p++) |= new_func;
> > +        }
> >      }
> >      memset(&sd->data[17], 0, 47);
> >      stw_be_p(sd->data + 65, sd_crc16(sd->data, 64));
> > diff --git a/hw/sd/trace-events b/hw/sd/trace-events
> > index 2059ace61f..c106541a47 100644
> > --- a/hw/sd/trace-events
> > +++ b/hw/sd/trace-events
> > @@ -42,6 +42,7 @@ sdcard_write_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
> >  sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint8_t value) "%s %20s/ CMD%02d value 0x%02x"
> >  sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, int length) "%s %20s/ CMD%02d len %d"
> >  sdcard_set_voltage(uint16_t millivolts) "%u mV"
> > +sdcard_function_select(const char *fn_name, const char *grp_name, bool do_switch) "Function %s (group: %s, sw: %u)"
> >
> >  # hw/sd/milkymist-memcard.c
> >  milkymist_memcard_memory_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
> > --
> > 2.16.2
> >
> 
> thanks
> -- PMM

Hi Peter, Edgar.

On 03/09/2018 06:03 PM, Peter Maydell wrote:
> On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> [based on a patch from Alistair Francis <alistair.francis@xilinx.com>
>>  from qemu/xilinx tag xilinx-v2015.2]
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> This should ideally have a Signed-off-by: from somebody @xilinx.com as
> well as you, then.

The original commit is:

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

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

      PetaLinux QEMU v2015.2

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

As you can see the changes are:

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

So no logical change.

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

Thanks,

Phil.

On 03/09/2018 06:03 PM, Peter Maydell wrote:
> On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> [based on a patch from Alistair Francis <alistair.francis@xilinx.com>
>>  from qemu/xilinx tag xilinx-v2015.2]
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> This should ideally have a Signed-off-by: from somebody @xilinx.com as
> well as you, then.
> 
>> ---
>>  hw/sd/sd.c         | 148 +++++++++++++++++++++++++++++++++++++++++++++--------
>>  hw/sd/trace-events |   1 +
>>  2 files changed, 127 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 235e0518d6..b907d62aef 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -124,6 +124,7 @@ struct SDState {
>>      bool enable;
>>      uint8_t dat_lines;
>>      bool cmd_line;
>> +    bool uhs_enabled;
>>  };
>>
>>  static const char *sd_state_name(enum SDCardStates state)
>> @@ -563,6 +564,7 @@ static void sd_reset(DeviceState *dev)
>>      sd->expecting_acmd = false;
>>      sd->dat_lines = 0xf;
>>      sd->cmd_line = true;
>> +    sd->uhs_enabled = false;
>>      sd->multi_blk_cnt = 0;
>>  }
>>
>> @@ -761,30 +763,132 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
>>      return ret;
>>  }
>>
>> +/* Function Group */
>> +enum {
>> +    SD_FG_MIN               = 1,
>> +    SD_FG_ACCESS_MODE       = 1,
>> +    SD_FG_COMMAND_SYSTEM    = 2,
>> +    SD_FG_DRIVER_STRENGTH   = 3,
>> +    SD_FG_CURRENT_LIMIT     = 4,
>> +    SD_FG_RSVD_5            = 5,
>> +    SD_FG_RSVD_6            = 6,
>> +    SD_FG_COUNT
>> +};
> 
> Since the minimum isn't 0, this means SD_FG_COUNT isn't
> actually the count of the number of FGs. I think having
>     SD_FG_MAX = 6,
> 
> would make the loops below clearer, since they become
>   for (fn_grp = SD_FG_MAX; fn_grp >= SD_FG_MIN; fn_grp--) {
> 
>> +
>> +/* Function name */
>> +#define SD_FN_COUNT 16
>> +
>> +static const char *sd_fn_grp_name[SD_FG_COUNT] = {
>> +    [SD_FG_ACCESS_MODE]     = "ACCESS_MODE",
>> +    [SD_FG_COMMAND_SYSTEM]  = "COMMAND_SYSTEM",
>> +    [SD_FG_DRIVER_STRENGTH] = "DRIVER_STRENGTH",
>> +    [SD_FG_CURRENT_LIMIT]   = "CURRENT_LIMIT",
>> +    [SD_FG_RSVD_5]          = "RSVD5",
>> +    [SD_FG_RSVD_6]          = "RSVD6",
>> +};
>> +
>> +typedef struct sd_fn_support {
>> +    const char *name;
>> +    bool uhs_only;
>> +    bool unimp;
>> +} sd_fn_support;
>> +
>> +static const sd_fn_support *sd_fn_support_defs[SD_FG_COUNT] = {
>> +    [SD_FG_ACCESS_MODE] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default/SDR12" },
>> +        [1] = { .name = "high-speed/SDR25" },
>> +        [2] = { .name = "SDR50",    .uhs_only = true },
>> +        [3] = { .name = "SDR104",   .uhs_only = true },
>> +        [4] = { .name = "DDR50",    .uhs_only = true },
>> +    },
>> +    [SD_FG_COMMAND_SYSTEM] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default" },
>> +        [1] = { .name = "For eC" },
>> +        [3] = { .name = "OTP",      .unimp = true },
>> +        [4] = { .name = "ASSD",     .unimp = true },
>> +    },
>> +    [SD_FG_DRIVER_STRENGTH] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default/Type B" },
>> +        [1] = { .name = "Type A",   .uhs_only = true },
>> +        [2] = { .name = "Type C",   .uhs_only = true },
>> +        [3] = { .name = "Type D",   .uhs_only = true },
>> +    },
>> +    [SD_FG_CURRENT_LIMIT] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default/200mA" },
>> +        [1] = { .name = "400mA",    .uhs_only = true },
>> +        [2] = { .name = "600mA",    .uhs_only = true },
>> +        [3] = { .name = "800mA",    .uhs_only = true },
>> +    },
>> +    [SD_FG_RSVD_5] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default" },
>> +    },
>> +    [SD_FG_RSVD_6] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default" },
>> +    },
>> +};
>> +
>> +#define SD_FN_NO_INFLUENCE          (1 << 15)
>> +
>>  static void sd_function_switch(SDState *sd, uint32_t arg)
>>  {
>> -    int i, mode, new_func;
>> -    mode = !!(arg & 0x80000000);
>> -
>> -    sd->data[0] = 0x00;                /* Maximum current consumption */
>> -    sd->data[1] = 0x01;
>> -    sd->data[2] = 0x80;                /* Supported group 6 functions */
>> -    sd->data[3] = 0x01;
>> -    sd->data[4] = 0x80;                /* Supported group 5 functions */
>> -    sd->data[5] = 0x01;
>> -    sd->data[6] = 0x80;                /* Supported group 4 functions */
>> -    sd->data[7] = 0x01;
>> -    sd->data[8] = 0x80;                /* Supported group 3 functions */
>> -    sd->data[9] = 0x01;
>> -    sd->data[10] = 0x80;       /* Supported group 2 functions */
>> -    sd->data[11] = 0x43;
>> -    sd->data[12] = 0x80;       /* Supported group 1 functions */
>> -    sd->data[13] = 0x03;
>> -    for (i = 0; i < 6; i ++) {
>> -        new_func = (arg >> (i * 4)) & 0x0f;
>> -        if (mode && new_func != 0x0f)
>> -            sd->function_group[i] = new_func;
>> -        sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
>> +    int fn_grp, new_func, i;
>> +    uint8_t *data_p;
>> +    bool mode = extract32(arg, 31, 1);  /* 0: check only, 1: do switch */
>> +
>> +    stw_be_p(sd->data + 0, 0x0001);     /* Maximum current consumption */
> 
> Previously we were writing 0x00, 0x01 to the first 2 bytes of data;
> now we will write 0x01, 0x00. Are you sure that's right ? I guess
> it's the difference between claiming 1mA and 256mA.
> (I can't make any sense of the table in the spec so I have no idea.)

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

> 
> Also the spec says that if the guest selects an invalid function then
> this value should be 0.
> 
>> +
>> +    data_p = &sd->data[2];
>> +    for (fn_grp = SD_FG_COUNT - 1; fn_grp >= SD_FG_MIN; fn_grp--) {
>> +        uint16_t supported_fns = SD_FN_NO_INFLUENCE;
>> +        for (i = 0; i < SD_FN_COUNT; ++i) {
>> +            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][i];
>> +
>> +            if (def->name && !def->unimp &&
>> +                    !(def->uhs_only && !sd->uhs_enabled)) {
>> +                supported_fns |= 1 << i;
>> +            }
>> +        }
>> +        stw_be_p(data_p, supported_fns);
>> +        data_p += 2;
>> +    }
>> +
>> +    assert(data_p == &sd->data[14]);
>> +    for (fn_grp = SD_FG_COUNT - 1; fn_grp >= SD_FG_MIN; fn_grp--) {
>> +        new_func = (arg >> ((fn_grp - 1) * 4)) & 0x0f;
>> +        if (new_func == 0xf) {
> 
> /* Guest requested no influence, so this function group stays the same */
> 
>> +            new_func = sd->function_group[fn_grp - 1];
>> +        } else {
>> +            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][new_func];
>> +            if (mode) {
>> +                if (!def->name) {
>> +                    qemu_log_mask(LOG_GUEST_ERROR,
>> +                                  "Function %d not a valid for "
> 
> "not valid for"
> 
>> +                                  "function group %d\n",
>> +                                  new_func, fn_grp);
>> +                    new_func = 0xf;
>> +                } else if (def->unimp) {
>> +                    qemu_log_mask(LOG_UNIMP,
>> +                                  "Function %s (fn grp %d) not implemented\n",
>> +                                  def->name, fn_grp);
>> +                    new_func = 0xf;
>> +                } else if (def->uhs_only && !sd->uhs_enabled) {
>> +                    qemu_log_mask(LOG_GUEST_ERROR,
>> +                                  "Function %s (fn grp %d) only "
>> +                                  "valid in UHS mode\n",
>> +                                  def->name, fn_grp);
>> +                    new_func = 0xf;
>> +                } else {
>> +                    sd->function_group[fn_grp - 1] = new_func;
> 
> I think the spec says that if the guest makes an invalid selection
> for one function in the group then we must ignore all the set values,
> not just the one that was wrong, so we need to check everything
> first before we start writing the new values back.

Yes, we missed that.

> 
>> +                }
>> +            }
>> +            trace_sdcard_function_select(def->name, sd_fn_grp_name[fn_grp],
>> +                                         mode);
>> +        }
>> +        if (!(fn_grp & 0x1)) { /* evens go in high nibble */
>> +            *data_p = new_func << 4;
>> +        } else { /* odds go in low nibble */
>> +            *(data_p++) |= new_func;
>> +        }
>>      }
>>      memset(&sd->data[17], 0, 47);
>>      stw_be_p(sd->data + 65, sd_crc16(sd->data, 64));
>> diff --git a/hw/sd/trace-events b/hw/sd/trace-events
>> index 2059ace61f..c106541a47 100644
>> --- a/hw/sd/trace-events
>> +++ b/hw/sd/trace-events
>> @@ -42,6 +42,7 @@ sdcard_write_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
>>  sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint8_t value) "%s %20s/ CMD%02d value 0x%02x"
>>  sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, int length) "%s %20s/ CMD%02d len %d"
>>  sdcard_set_voltage(uint16_t millivolts) "%u mV"
>> +sdcard_function_select(const char *fn_name, const char *grp_name, bool do_switch) "Function %s (group: %s, sw: %u)"
>>
>>  # hw/sd/milkymist-memcard.c
>>  milkymist_memcard_memory_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
>> --
>> 2.16.2
>>
> 
> thanks
> -- PMM
> 

On 03/09/2018 06:03 PM, Peter Maydell wrote:
> On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> [based on a patch from Alistair Francis <alistair.francis@xilinx.com>
>>  from qemu/xilinx tag xilinx-v2015.2]
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> This should ideally have a Signed-off-by: from somebody @xilinx.com as
> well as you, then.
> 
>> ---
>>  hw/sd/sd.c         | 148 +++++++++++++++++++++++++++++++++++++++++++++--------
>>  hw/sd/trace-events |   1 +
>>  2 files changed, 127 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 235e0518d6..b907d62aef 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -124,6 +124,7 @@ struct SDState {
>>      bool enable;
>>      uint8_t dat_lines;
>>      bool cmd_line;
>> +    bool uhs_enabled;
>>  };
>>
>>  static const char *sd_state_name(enum SDCardStates state)
>> @@ -563,6 +564,7 @@ static void sd_reset(DeviceState *dev)
>>      sd->expecting_acmd = false;
>>      sd->dat_lines = 0xf;
>>      sd->cmd_line = true;
>> +    sd->uhs_enabled = false;
>>      sd->multi_blk_cnt = 0;
>>  }
>>
>> @@ -761,30 +763,132 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
>>      return ret;
>>  }
>>
>> +/* Function Group */
>> +enum {
>> +    SD_FG_MIN               = 1,
>> +    SD_FG_ACCESS_MODE       = 1,
>> +    SD_FG_COMMAND_SYSTEM    = 2,
>> +    SD_FG_DRIVER_STRENGTH   = 3,
>> +    SD_FG_CURRENT_LIMIT     = 4,
>> +    SD_FG_RSVD_5            = 5,
>> +    SD_FG_RSVD_6            = 6,
>> +    SD_FG_COUNT
>> +};
> 
> Since the minimum isn't 0, this means SD_FG_COUNT isn't
> actually the count of the number of FGs. I think having
>     SD_FG_MAX = 6,
> 
> would make the loops below clearer, since they become
>   for (fn_grp = SD_FG_MAX; fn_grp >= SD_FG_MIN; fn_grp--) {
> 
>> +
>> +/* Function name */
>> +#define SD_FN_COUNT 16
>> +
>> +static const char *sd_fn_grp_name[SD_FG_COUNT] = {
>> +    [SD_FG_ACCESS_MODE]     = "ACCESS_MODE",
>> +    [SD_FG_COMMAND_SYSTEM]  = "COMMAND_SYSTEM",
>> +    [SD_FG_DRIVER_STRENGTH] = "DRIVER_STRENGTH",
>> +    [SD_FG_CURRENT_LIMIT]   = "CURRENT_LIMIT",
>> +    [SD_FG_RSVD_5]          = "RSVD5",
>> +    [SD_FG_RSVD_6]          = "RSVD6",
>> +};
>> +
>> +typedef struct sd_fn_support {
>> +    const char *name;
>> +    bool uhs_only;
>> +    bool unimp;
>> +} sd_fn_support;
>> +
>> +static const sd_fn_support *sd_fn_support_defs[SD_FG_COUNT] = {
>> +    [SD_FG_ACCESS_MODE] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default/SDR12" },
>> +        [1] = { .name = "high-speed/SDR25" },
>> +        [2] = { .name = "SDR50",    .uhs_only = true },
>> +        [3] = { .name = "SDR104",   .uhs_only = true },
>> +        [4] = { .name = "DDR50",    .uhs_only = true },
>> +    },
>> +    [SD_FG_COMMAND_SYSTEM] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default" },
>> +        [1] = { .name = "For eC" },
>> +        [3] = { .name = "OTP",      .unimp = true },
>> +        [4] = { .name = "ASSD",     .unimp = true },
>> +    },
>> +    [SD_FG_DRIVER_STRENGTH] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default/Type B" },
>> +        [1] = { .name = "Type A",   .uhs_only = true },
>> +        [2] = { .name = "Type C",   .uhs_only = true },
>> +        [3] = { .name = "Type D",   .uhs_only = true },
>> +    },
>> +    [SD_FG_CURRENT_LIMIT] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default/200mA" },
>> +        [1] = { .name = "400mA",    .uhs_only = true },
>> +        [2] = { .name = "600mA",    .uhs_only = true },
>> +        [3] = { .name = "800mA",    .uhs_only = true },
>> +    },
>> +    [SD_FG_RSVD_5] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default" },
>> +    },
>> +    [SD_FG_RSVD_6] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default" },
>> +    },
>> +};
>> +
>> +#define SD_FN_NO_INFLUENCE          (1 << 15)
>> +
>>  static void sd_function_switch(SDState *sd, uint32_t arg)
>>  {
>> -    int i, mode, new_func;
>> -    mode = !!(arg & 0x80000000);
>> -
>> -    sd->data[0] = 0x00;                /* Maximum current consumption */
>> -    sd->data[1] = 0x01;
>> -    sd->data[2] = 0x80;                /* Supported group 6 functions */
>> -    sd->data[3] = 0x01;
>> -    sd->data[4] = 0x80;                /* Supported group 5 functions */
>> -    sd->data[5] = 0x01;
>> -    sd->data[6] = 0x80;                /* Supported group 4 functions */
>> -    sd->data[7] = 0x01;
>> -    sd->data[8] = 0x80;                /* Supported group 3 functions */
>> -    sd->data[9] = 0x01;
>> -    sd->data[10] = 0x80;       /* Supported group 2 functions */
>> -    sd->data[11] = 0x43;
>> -    sd->data[12] = 0x80;       /* Supported group 1 functions */
>> -    sd->data[13] = 0x03;
>> -    for (i = 0; i < 6; i ++) {
>> -        new_func = (arg >> (i * 4)) & 0x0f;
>> -        if (mode && new_func != 0x0f)
>> -            sd->function_group[i] = new_func;
>> -        sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
>> +    int fn_grp, new_func, i;
>> +    uint8_t *data_p;
>> +    bool mode = extract32(arg, 31, 1);  /* 0: check only, 1: do switch */
>> +
>> +    stw_be_p(sd->data + 0, 0x0001);     /* Maximum current consumption */
> 
> Previously we were writing 0x00, 0x01 to the first 2 bytes of data;
> now we will write 0x01, 0x00. Are you sure that's right ? I guess
> it's the difference between claiming 1mA and 256mA.
> (I can't make any sense of the table in the spec so I have no idea.)

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

> 
> Also the spec says that if the guest selects an invalid function then
> this value should be 0.
> 
>> +
>> +    data_p = &sd->data[2];
>> +    for (fn_grp = SD_FG_COUNT - 1; fn_grp >= SD_FG_MIN; fn_grp--) {
>> +        uint16_t supported_fns = SD_FN_NO_INFLUENCE;
>> +        for (i = 0; i < SD_FN_COUNT; ++i) {
>> +            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][i];
>> +
>> +            if (def->name && !def->unimp &&
>> +                    !(def->uhs_only && !sd->uhs_enabled)) {
>> +                supported_fns |= 1 << i;
>> +            }
>> +        }
>> +        stw_be_p(data_p, supported_fns);
>> +        data_p += 2;
>> +    }
>> +
>> +    assert(data_p == &sd->data[14]);
>> +    for (fn_grp = SD_FG_COUNT - 1; fn_grp >= SD_FG_MIN; fn_grp--) {
>> +        new_func = (arg >> ((fn_grp - 1) * 4)) & 0x0f;
>> +        if (new_func == 0xf) {
> 
> /* Guest requested no influence, so this function group stays the same */
> 
>> +            new_func = sd->function_group[fn_grp - 1];
>> +        } else {
>> +            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][new_func];
>> +            if (mode) {
>> +                if (!def->name) {
>> +                    qemu_log_mask(LOG_GUEST_ERROR,
>> +                                  "Function %d not a valid for "
> 
> "not valid for"
> 
>> +                                  "function group %d\n",
>> +                                  new_func, fn_grp);
>> +                    new_func = 0xf;
>> +                } else if (def->unimp) {
>> +                    qemu_log_mask(LOG_UNIMP,
>> +                                  "Function %s (fn grp %d) not implemented\n",
>> +                                  def->name, fn_grp);
>> +                    new_func = 0xf;
>> +                } else if (def->uhs_only && !sd->uhs_enabled) {
>> +                    qemu_log_mask(LOG_GUEST_ERROR,
>> +                                  "Function %s (fn grp %d) only "
>> +                                  "valid in UHS mode\n",
>> +                                  def->name, fn_grp);
>> +                    new_func = 0xf;
>> +                } else {
>> +                    sd->function_group[fn_grp - 1] = new_func;
> 
> I think the spec says that if the guest makes an invalid selection
> for one function in the group then we must ignore all the set values,
> not just the one that was wrong, so we need to check everything
> first before we start writing the new values back.

Yes, we missed that.

> 
>> +                }
>> +            }
>> +            trace_sdcard_function_select(def->name, sd_fn_grp_name[fn_grp],
>> +                                         mode);
>> +        }
>> +        if (!(fn_grp & 0x1)) { /* evens go in high nibble */
>> +            *data_p = new_func << 4;
>> +        } else { /* odds go in low nibble */
>> +            *(data_p++) |= new_func;
>> +        }
>>      }
>>      memset(&sd->data[17], 0, 47);
>>      stw_be_p(sd->data + 65, sd_crc16(sd->data, 64));
>> diff --git a/hw/sd/trace-events b/hw/sd/trace-events
>> index 2059ace61f..c106541a47 100644
>> --- a/hw/sd/trace-events
>> +++ b/hw/sd/trace-events
>> @@ -42,6 +42,7 @@ sdcard_write_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
>>  sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint8_t value) "%s %20s/ CMD%02d value 0x%02x"
>>  sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, int length) "%s %20s/ CMD%02d len %d"
>>  sdcard_set_voltage(uint16_t millivolts) "%u mV"
>> +sdcard_function_select(const char *fn_name, const char *grp_name, bool do_switch) "Function %s (group: %s, sw: %u)"
>>
>>  # hw/sd/milkymist-memcard.c
>>  milkymist_memcard_memory_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
>> --
>> 2.16.2
>>
> 
> thanks
> -- PMM
> 

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

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

thanks
-- PMM

Hi Peter,

On 03/09/2018 06:03 PM, Peter Maydell wrote:
> On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> [based on a patch from Alistair Francis <alistair.francis@xilinx.com>
>>  from qemu/xilinx tag xilinx-v2015.2]
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> This should ideally have a Signed-off-by: from somebody @xilinx.com as
> well as you, then.
> 
>> ---
>>  hw/sd/sd.c         | 148 +++++++++++++++++++++++++++++++++++++++++++++--------
>>  hw/sd/trace-events |   1 +
>>  2 files changed, 127 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 235e0518d6..b907d62aef 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -124,6 +124,7 @@ struct SDState {
>>      bool enable;
>>      uint8_t dat_lines;
>>      bool cmd_line;
>> +    bool uhs_enabled;
>>  };
>>
>>  static const char *sd_state_name(enum SDCardStates state)
>> @@ -563,6 +564,7 @@ static void sd_reset(DeviceState *dev)
>>      sd->expecting_acmd = false;
>>      sd->dat_lines = 0xf;
>>      sd->cmd_line = true;
>> +    sd->uhs_enabled = false;
>>      sd->multi_blk_cnt = 0;
>>  }
>>
>> @@ -761,30 +763,132 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
>>      return ret;
>>  }
>>
>> +/* Function Group */
>> +enum {
>> +    SD_FG_MIN               = 1,
>> +    SD_FG_ACCESS_MODE       = 1,
>> +    SD_FG_COMMAND_SYSTEM    = 2,
>> +    SD_FG_DRIVER_STRENGTH   = 3,
>> +    SD_FG_CURRENT_LIMIT     = 4,
>> +    SD_FG_RSVD_5            = 5,
>> +    SD_FG_RSVD_6            = 6,
>> +    SD_FG_COUNT
>> +};
> 
> Since the minimum isn't 0, this means SD_FG_COUNT isn't
> actually the count of the number of FGs. I think having
>     SD_FG_MAX = 6,
> 
> would make the loops below clearer, since they become
>   for (fn_grp = SD_FG_MAX; fn_grp >= SD_FG_MIN; fn_grp--) {
> 
>> +
>> +/* Function name */
>> +#define SD_FN_COUNT 16
>> +
>> +static const char *sd_fn_grp_name[SD_FG_COUNT] = {
>> +    [SD_FG_ACCESS_MODE]     = "ACCESS_MODE",
>> +    [SD_FG_COMMAND_SYSTEM]  = "COMMAND_SYSTEM",
>> +    [SD_FG_DRIVER_STRENGTH] = "DRIVER_STRENGTH",
>> +    [SD_FG_CURRENT_LIMIT]   = "CURRENT_LIMIT",
>> +    [SD_FG_RSVD_5]          = "RSVD5",
>> +    [SD_FG_RSVD_6]          = "RSVD6",
>> +};
>> +
>> +typedef struct sd_fn_support {
>> +    const char *name;
>> +    bool uhs_only;
>> +    bool unimp;
>> +} sd_fn_support;
>> +
>> +static const sd_fn_support *sd_fn_support_defs[SD_FG_COUNT] = {
>> +    [SD_FG_ACCESS_MODE] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default/SDR12" },
>> +        [1] = { .name = "high-speed/SDR25" },
>> +        [2] = { .name = "SDR50",    .uhs_only = true },
>> +        [3] = { .name = "SDR104",   .uhs_only = true },
>> +        [4] = { .name = "DDR50",    .uhs_only = true },
>> +    },
>> +    [SD_FG_COMMAND_SYSTEM] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default" },
>> +        [1] = { .name = "For eC" },
>> +        [3] = { .name = "OTP",      .unimp = true },
>> +        [4] = { .name = "ASSD",     .unimp = true },
>> +    },
>> +    [SD_FG_DRIVER_STRENGTH] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default/Type B" },
>> +        [1] = { .name = "Type A",   .uhs_only = true },
>> +        [2] = { .name = "Type C",   .uhs_only = true },
>> +        [3] = { .name = "Type D",   .uhs_only = true },
>> +    },
>> +    [SD_FG_CURRENT_LIMIT] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default/200mA" },
>> +        [1] = { .name = "400mA",    .uhs_only = true },
>> +        [2] = { .name = "600mA",    .uhs_only = true },
>> +        [3] = { .name = "800mA",    .uhs_only = true },
>> +    },
>> +    [SD_FG_RSVD_5] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default" },
>> +    },
>> +    [SD_FG_RSVD_6] = (sd_fn_support [SD_FN_COUNT]) {
>> +        [0] = { .name = "default" },
>> +    },
>> +};
>> +
>> +#define SD_FN_NO_INFLUENCE          (1 << 15)
>> +
>>  static void sd_function_switch(SDState *sd, uint32_t arg)
>>  {
>> -    int i, mode, new_func;
>> -    mode = !!(arg & 0x80000000);
>> -
>> -    sd->data[0] = 0x00;                /* Maximum current consumption */
>> -    sd->data[1] = 0x01;
>> -    sd->data[2] = 0x80;                /* Supported group 6 functions */
>> -    sd->data[3] = 0x01;
>> -    sd->data[4] = 0x80;                /* Supported group 5 functions */
>> -    sd->data[5] = 0x01;
>> -    sd->data[6] = 0x80;                /* Supported group 4 functions */
>> -    sd->data[7] = 0x01;
>> -    sd->data[8] = 0x80;                /* Supported group 3 functions */
>> -    sd->data[9] = 0x01;
>> -    sd->data[10] = 0x80;       /* Supported group 2 functions */
>> -    sd->data[11] = 0x43;
>> -    sd->data[12] = 0x80;       /* Supported group 1 functions */
>> -    sd->data[13] = 0x03;
>> -    for (i = 0; i < 6; i ++) {
>> -        new_func = (arg >> (i * 4)) & 0x0f;
>> -        if (mode && new_func != 0x0f)
>> -            sd->function_group[i] = new_func;
>> -        sd->data[14 + (i >> 1)] = new_func << ((i * 4) & 4);
>> +    int fn_grp, new_func, i;
>> +    uint8_t *data_p;
>> +    bool mode = extract32(arg, 31, 1);  /* 0: check only, 1: do switch */
>> +
>> +    stw_be_p(sd->data + 0, 0x0001);     /* Maximum current consumption */
> 
> Previously we were writing 0x00, 0x01 to the first 2 bytes of data;
> now we will write 0x01, 0x00. Are you sure that's right ? I guess
> it's the difference between claiming 1mA and 256mA.
> (I can't make any sense of the table in the spec so I have no idea.)
> 
> Also the spec says that if the guest selects an invalid function then
> this value should be 0.
> 
>> +
>> +    data_p = &sd->data[2];
>> +    for (fn_grp = SD_FG_COUNT - 1; fn_grp >= SD_FG_MIN; fn_grp--) {
>> +        uint16_t supported_fns = SD_FN_NO_INFLUENCE;
>> +        for (i = 0; i < SD_FN_COUNT; ++i) {
>> +            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][i];
>> +
>> +            if (def->name && !def->unimp &&
>> +                    !(def->uhs_only && !sd->uhs_enabled)) {
>> +                supported_fns |= 1 << i;
>> +            }
>> +        }
>> +        stw_be_p(data_p, supported_fns);
>> +        data_p += 2;
>> +    }
>> +
>> +    assert(data_p == &sd->data[14]);
>> +    for (fn_grp = SD_FG_COUNT - 1; fn_grp >= SD_FG_MIN; fn_grp--) {
>> +        new_func = (arg >> ((fn_grp - 1) * 4)) & 0x0f;
>> +        if (new_func == 0xf) {
> 
> /* Guest requested no influence, so this function group stays the same */
> 
>> +            new_func = sd->function_group[fn_grp - 1];
>> +        } else {
>> +            const sd_fn_support *def = &sd_fn_support_defs[fn_grp][new_func];
>> +            if (mode) {
>> +                if (!def->name) {
>> +                    qemu_log_mask(LOG_GUEST_ERROR,
>> +                                  "Function %d not a valid for "
> 
> "not valid for"
> 
>> +                                  "function group %d\n",
>> +                                  new_func, fn_grp);
>> +                    new_func = 0xf;
>> +                } else if (def->unimp) {
>> +                    qemu_log_mask(LOG_UNIMP,
>> +                                  "Function %s (fn grp %d) not implemented\n",
>> +                                  def->name, fn_grp);
>> +                    new_func = 0xf;
>> +                } else if (def->uhs_only && !sd->uhs_enabled) {
>> +                    qemu_log_mask(LOG_GUEST_ERROR,
>> +                                  "Function %s (fn grp %d) only "
>> +                                  "valid in UHS mode\n",
>> +                                  def->name, fn_grp);
>> +                    new_func = 0xf;
>> +                } else {
>> +                    sd->function_group[fn_grp - 1] = new_func;
> 
> I think the spec says that if the guest makes an invalid selection
> for one function in the group then we must ignore all the set values,

... for the current group? ...

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

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

  4.3.10.3 Mode 1 Operation - Set Function

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

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

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

> 
>> +                }
>> +            }
>> +            trace_sdcard_function_select(def->name, sd_fn_grp_name[fn_grp],
>> +                                         mode);
>> +        }
>> +        if (!(fn_grp & 0x1)) { /* evens go in high nibble */
>> +            *data_p = new_func << 4;
>> +        } else { /* odds go in low nibble */
>> +            *(data_p++) |= new_func;
>> +        }
>>      }
>>      memset(&sd->data[17], 0, 47);
>>      stw_be_p(sd->data + 65, sd_crc16(sd->data, 64));
>> diff --git a/hw/sd/trace-events b/hw/sd/trace-events
>> index 2059ace61f..c106541a47 100644
>> --- a/hw/sd/trace-events
>> +++ b/hw/sd/trace-events
>> @@ -42,6 +42,7 @@ sdcard_write_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
>>  sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint8_t value) "%s %20s/ CMD%02d value 0x%02x"
>>  sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, int length) "%s %20s/ CMD%02d len %d"
>>  sdcard_set_voltage(uint16_t millivolts) "%u mV"
>> +sdcard_function_select(const char *fn_name, const char *grp_name, bool do_switch) "Function %s (group: %s, sw: %u)"
>>
>>  # hw/sd/milkymist-memcard.c
>>  milkymist_memcard_memory_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
>> --
>> 2.16.2
>>
> 
> thanks
> -- PMM
> 

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

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

thanks
-- PMM

Hi Peter,

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

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

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

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

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

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

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

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

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

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

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

Finally those 2 incorrect functions at once:

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

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

Regards,

Phil.

Hi Peter,

On 03/09/2018 02:03 PM, Peter Maydell wrote:
> On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
[...]
>>  static void sd_function_switch(SDState *sd, uint32_t arg)
>>  {
>> -    int i, mode, new_func;
>> -    mode = !!(arg & 0x80000000);
>> -
>> -    sd->data[0] = 0x00;                /* Maximum current consumption */
>> -    sd->data[1] = 0x01;
>> -    sd->data[2] = 0x80;                /* Supported group 6 functions */
>> -    sd->data[3] = 0x01;
[...]
>> +
>> +    stw_be_p(sd->data + 0, 0x0001);     /* Maximum current consumption */
> 
> Previously we were writing 0x00, 0x01 to the first 2 bytes of data;
> now we will write 0x01, 0x00. Are you sure that's right ? I guess
> it's the difference between claiming 1mA and 256mA.

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

On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> [based on a patch from Alistair Francis <alistair.francis@xilinx.com>
>  from qemu/xilinx tag xilinx-v2015.2]
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c         | 148 +++++++++++++++++++++++++++++++++++++++++++++--------
>  hw/sd/trace-events |   1 +
>  2 files changed, 127 insertions(+), 22 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 235e0518d6..b907d62aef 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -124,6 +124,7 @@ struct SDState {
>      bool enable;
>      uint8_t dat_lines;
>      bool cmd_line;
> +    bool uhs_enabled;

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

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

thanks
-- PMM

On 03/09/2018 06:06 PM, Peter Maydell wrote:
> On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> [based on a patch from Alistair Francis <alistair.francis@xilinx.com>
>>  from qemu/xilinx tag xilinx-v2015.2]
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/sd.c         | 148 +++++++++++++++++++++++++++++++++++++++++++++--------
>>  hw/sd/trace-events |   1 +
>>  2 files changed, 127 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 235e0518d6..b907d62aef 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -124,6 +124,7 @@ struct SDState {
>>      bool enable;
>>      uint8_t dat_lines;
>>      bool cmd_line;
>> +    bool uhs_enabled;
> 
> Oh, and what's the difference between s->uhs_enabled = false
> (this patch) and s->uhs_mode = UHS_NOT_SUPPORTED (next patch) ?
> 
> Do we need both? If so, a comment noting the difference would help
> people to know which one various bits of code should be checking.

Ok.

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

> 
> thanks
> -- PMM
> 

On 12 March 2018 at 12:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 03/09/2018 06:06 PM, Peter Maydell wrote:
>> On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> [based on a patch from Alistair Francis <alistair.francis@xilinx.com>
>>>  from qemu/xilinx tag xilinx-v2015.2]
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/sd/sd.c         | 148 +++++++++++++++++++++++++++++++++++++++++++++--------
>>>  hw/sd/trace-events |   1 +
>>>  2 files changed, 127 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>> index 235e0518d6..b907d62aef 100644
>>> --- a/hw/sd/sd.c
>>> +++ b/hw/sd/sd.c
>>> @@ -124,6 +124,7 @@ struct SDState {
>>>      bool enable;
>>>      uint8_t dat_lines;
>>>      bool cmd_line;
>>> +    bool uhs_enabled;
>>
>> Oh, and what's the difference between s->uhs_enabled = false
>> (this patch) and s->uhs_mode = UHS_NOT_SUPPORTED (next patch) ?
>>
>> Do we need both? If so, a comment noting the difference would help
>> people to know which one various bits of code should be checking.
>
> Ok.
>
> The 'uhs_mode' is a read-only property before realize().
> Now if the sdcard supports any UHS, the host may negotiate to switch to
> UHS mode.
> To enter this mode with voltage level adjusted, the card needs to
> 'soft-reset' itself in the new mode. We use 'uhs_enabled' to track this
> runtime state. Maybe 'uhs_active' is more explicit?
> I intended to keep the properties vs runtime fields separated in the
> SDState.

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

thanks
-- PMM