* [Qemu-devel] [PATCH v2 0/4] SDCard: support UHS-I @ 2018-05-09 6:01 Philippe Mathieu-Daudé 2018-05-09 6:01 ` [Qemu-devel] [PATCH v2 1/4] sdcard: Update the SDState documentation Philippe Mathieu-Daudé ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2018-05-09 6:01 UTC (permalink / raw) To: Peter Maydell, Edgar E . Iglesias, Alistair Francis Cc: Philippe Mathieu-Daudé, qemu-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v2 1/4] sdcard: Update the SDState documentation 2018-05-09 6:01 [Qemu-devel] [PATCH v2 0/4] SDCard: support UHS-I Philippe Mathieu-Daudé @ 2018-05-09 6:01 ` Philippe Mathieu-Daudé 2018-05-09 15:42 ` Alistair Francis 2018-05-09 6:01 ` [Qemu-devel] [PATCH v2 2/4] sdcard: Correct CRC16 offset in sd_function_switch() Philippe Mathieu-Daudé ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2018-05-09 6:01 UTC (permalink / raw) To: Peter Maydell, Edgar E . Iglesias, Alistair Francis Cc: Philippe Mathieu-Daudé, qemu-devel 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] sdcard: Update the SDState documentation 2018-05-09 6:01 ` [Qemu-devel] [PATCH v2 1/4] sdcard: Update the SDState documentation Philippe Mathieu-Daudé @ 2018-05-09 15:42 ` Alistair Francis 2018-05-10 0:24 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 13+ messages in thread From: Alistair Francis @ 2018-05-09 15:42 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Peter Maydell, Edgar E . Iglesias, Alistair Francis, qemu-devel@nongnu.org Developers 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 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] sdcard: Update the SDState documentation 2018-05-09 15:42 ` Alistair Francis @ 2018-05-10 0:24 ` Philippe Mathieu-Daudé 2018-05-14 15:49 ` Peter Maydell 0 siblings, 1 reply; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2018-05-10 0:24 UTC (permalink / raw) To: Alistair Francis Cc: Peter Maydell, Edgar E . Iglesias, Alistair Francis, qemu-devel@nongnu.org Developers 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 >> >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] sdcard: Update the SDState documentation 2018-05-10 0:24 ` Philippe Mathieu-Daudé @ 2018-05-14 15:49 ` Peter Maydell 2018-05-14 17:07 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 13+ messages in thread From: Peter Maydell @ 2018-05-14 15:49 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Alistair Francis, Edgar E . Iglesias, Alistair Francis, qemu-devel@nongnu.org Developers 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] sdcard: Update the SDState documentation 2018-05-14 15:49 ` Peter Maydell @ 2018-05-14 17:07 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2018-05-14 17:07 UTC (permalink / raw) To: Peter Maydell Cc: Alistair Francis, Edgar E . Iglesias, Alistair Francis, qemu-devel@nongnu.org Developers 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v2 2/4] sdcard: Correct CRC16 offset in sd_function_switch() 2018-05-09 6:01 [Qemu-devel] [PATCH v2 0/4] SDCard: support UHS-I Philippe Mathieu-Daudé 2018-05-09 6:01 ` [Qemu-devel] [PATCH v2 1/4] sdcard: Update the SDState documentation Philippe Mathieu-Daudé @ 2018-05-09 6:01 ` Philippe Mathieu-Daudé 2018-05-14 15:50 ` Peter Maydell 2018-05-09 6:01 ` [Qemu-devel] [PATCH v2 3/4] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3) Philippe Mathieu-Daudé 2018-05-09 6:01 ` [Qemu-devel] [PATCH v2 4/4] sdcard: Add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit Philippe Mathieu-Daudé 3 siblings, 1 reply; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2018-05-09 6:01 UTC (permalink / raw) To: Peter Maydell, Edgar E . Iglesias, Alistair Francis Cc: Philippe Mathieu-Daudé, qemu-devel 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] sdcard: Correct CRC16 offset in sd_function_switch() 2018-05-09 6:01 ` [Qemu-devel] [PATCH v2 2/4] sdcard: Correct CRC16 offset in sd_function_switch() Philippe Mathieu-Daudé @ 2018-05-14 15:50 ` Peter Maydell 0 siblings, 0 replies; 13+ messages in thread From: Peter Maydell @ 2018-05-14 15:50 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Edgar E . Iglesias, Alistair Francis, QEMU Developers 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v2 3/4] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3) 2018-05-09 6:01 [Qemu-devel] [PATCH v2 0/4] SDCard: support UHS-I Philippe Mathieu-Daudé 2018-05-09 6:01 ` [Qemu-devel] [PATCH v2 1/4] sdcard: Update the SDState documentation Philippe Mathieu-Daudé 2018-05-09 6:01 ` [Qemu-devel] [PATCH v2 2/4] sdcard: Correct CRC16 offset in sd_function_switch() Philippe Mathieu-Daudé @ 2018-05-09 6:01 ` Philippe Mathieu-Daudé 2018-05-14 15:38 ` Peter Maydell 2018-05-09 6:01 ` [Qemu-devel] [PATCH v2 4/4] sdcard: Add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit Philippe Mathieu-Daudé 3 siblings, 1 reply; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2018-05-09 6:01 UTC (permalink / raw) To: Peter Maydell, Edgar E . Iglesias, Alistair Francis Cc: Philippe Mathieu-Daudé, qemu-devel [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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3) 2018-05-09 6:01 ` [Qemu-devel] [PATCH v2 3/4] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3) Philippe Mathieu-Daudé @ 2018-05-14 15:38 ` Peter Maydell 2018-05-22 5:11 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 13+ messages in thread From: Peter Maydell @ 2018-05-14 15:38 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Edgar E . Iglesias, Alistair Francis, QEMU Developers 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3) 2018-05-14 15:38 ` Peter Maydell @ 2018-05-22 5:11 ` Philippe Mathieu-Daudé 2018-05-22 23:05 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2018-05-22 5:11 UTC (permalink / raw) To: Peter Maydell; +Cc: Edgar E . Iglesias, Alistair Francis, QEMU Developers 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3) 2018-05-22 5:11 ` Philippe Mathieu-Daudé @ 2018-05-22 23:05 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2018-05-22 23:05 UTC (permalink / raw) To: Peter Maydell; +Cc: Edgar E . Iglesias, Alistair Francis, QEMU Developers 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v2 4/4] sdcard: Add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit 2018-05-09 6:01 [Qemu-devel] [PATCH v2 0/4] SDCard: support UHS-I Philippe Mathieu-Daudé ` (2 preceding siblings ...) 2018-05-09 6:01 ` [Qemu-devel] [PATCH v2 3/4] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3) Philippe Mathieu-Daudé @ 2018-05-09 6:01 ` Philippe Mathieu-Daudé 3 siblings, 0 replies; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2018-05-09 6:01 UTC (permalink / raw) To: Peter Maydell, Edgar E . Iglesias, Alistair Francis Cc: Philippe Mathieu-Daudé, qemu-devel 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-05-22 23:05 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-09 6:01 [Qemu-devel] [PATCH v2 0/4] SDCard: support UHS-I Philippe Mathieu-Daudé 2018-05-09 6:01 ` [Qemu-devel] [PATCH v2 1/4] sdcard: Update the SDState documentation Philippe Mathieu-Daudé 2018-05-09 15:42 ` Alistair Francis 2018-05-10 0:24 ` Philippe Mathieu-Daudé 2018-05-14 15:49 ` Peter Maydell 2018-05-14 17:07 ` Philippe Mathieu-Daudé 2018-05-09 6:01 ` [Qemu-devel] [PATCH v2 2/4] sdcard: Correct CRC16 offset in sd_function_switch() Philippe Mathieu-Daudé 2018-05-14 15:50 ` Peter Maydell 2018-05-09 6:01 ` [Qemu-devel] [PATCH v2 3/4] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3) Philippe Mathieu-Daudé 2018-05-14 15:38 ` Peter Maydell 2018-05-22 5:11 ` Philippe Mathieu-Daudé 2018-05-22 23:05 ` Philippe Mathieu-Daudé 2018-05-09 6:01 ` [Qemu-devel] [PATCH v2 4/4] sdcard: Add a 'uhs' property, update the OCR register ACCEPT_SWITCH_1V8 bit Philippe Mathieu-Daudé
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.