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

[Edgar E. Iglesias]

On Fri, Mar 09, 2018 at 05:03:11PM +0000, Peter Maydell wrote:
> On 9 March 2018 at 15:36, Philippe Mathieu-Daudé <address@hidden> wrote:
> > [based on a patch from Alistair Francis <address@hidden>
> >  from qemu/xilinx tag xilinx-v2015.2]
> > Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> 
> 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 <address@hidden>

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