Re: [Qemu-devel] [PATCH v4 08/11] sdcard: display command name when tracing CMD/ACMD
Hi Peter,
On 02/22/2018 01:59 PM, Peter Maydell wrote:
> On 15 February 2018 at 22:05, Philippe Mathieu-Daudé wrote:
>> put the function in sdmmc-common.c since we will reuse it in hw/sd/core.c
>>
>
> As a general note, your commit messages are generally a bit
> shorter than I'd prefer. This is to some extent a personal
> style question, but if you're writing lots of patches where
> the only commit message is the subject-line summary then I
> think that it's worth being a bit more descriptive.
Got it, I'll try to be a bit more verbose. As you noticed this is not
natural for me ;) It took me time to understand what is obvious for
someone focused weeks in the same code, is not for a reviewer context
switching from an unrelated topic, and for a limited time.
> Also, the "body" part of the commit message should really
> be able to stand alone as a description, rather than relying
> on the subject line to make sense.
This one is easy to fix :)
>
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> hw/sd/sdmmc-internal.h | 3 +++
>> hw/sd/sd.c | 13 +
>> hw/sd/sdmmc-common.c | 72
>> ++
>> hw/sd/Makefile.objs| 2 +-
>> hw/sd/trace-events | 8 +++---
>> 5 files changed, 88 insertions(+), 10 deletions(-)
>> create mode 100644 hw/sd/sdmmc-common.c
>>
>> diff --git a/hw/sd/sdmmc-internal.h b/hw/sd/sdmmc-internal.h
>> index 0e96cb0081..02b730089b 100644
>> --- a/hw/sd/sdmmc-internal.h
>> +++ b/hw/sd/sdmmc-internal.h
>> @@ -12,4 +12,7 @@
>>
>> #define SDMMC_CMD_MAX 64
>>
>> +const char *sd_cmd_name(uint8_t cmd);
>> +const char *sd_acmd_name(uint8_t cmd);
>
> Can we have doc comments for new function prototypes in header files,
> please?
Yes :|
>
>
>> diff --git a/hw/sd/sdmmc-common.c b/hw/sd/sdmmc-common.c
>> new file mode 100644
>> index 00..1d0198b1ad
>> --- /dev/null
>> +++ b/hw/sd/sdmmc-common.c
>> @@ -0,0 +1,72 @@
>> +/*
>> + * SD/MMC cards common helpers
>> + *
>> + * Copyright (c) 2018 Philippe Mathieu-Daudé
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>
> Ooh, a SPDX identifier. The project doesn't actually use these
> at the moment, but it doesn't hurt to have one here I guess.
>
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "sdmmc-internal.h"
>> +
>> +const char *sd_cmd_name(uint8_t cmd)
>> +{
>> +static const char *cmd_abbrev[SDMMC_CMD_MAX] = {
>> + [0]= "GO_IDLE_STATE",
>> + [2]= "ALL_SEND_CID",[3]= "SEND_RELATIVE_ADDR",
>> + [4]= "SET_DSR", [5]= "IO_SEND_OP_COND",
>> + [6]= "SWITCH_FUNC", [7]=
>> "SELECT/DESELECT_CARD",
>> + [8]= "SEND_IF_COND",[9]= "SEND_CSD",
>> +[10]= "SEND_CID", [11]= "VOLTAGE_SWITCH",
>> +[12]= "STOP_TRANSMISSION", [13]= "SEND_STATUS",
>> +[15]= "GO_INACTIVE_STATE",
>> +[16]= "SET_BLOCKLEN", [17]= "READ_SINGLE_BLOCK",
>> +[18]= "READ_MULTIPLE_BLOCK",[19]= "SEND_TUNING_BLOCK",
>> +[20]= "SPEED_CLASS_CONTROL",[21]= "DPS_spec",
>> +[23]= "SET_BLOCK_COUNT",
>> +[24]= "WRITE_BLOCK",[25]=
>> "WRITE_MULTIPLE_BLOCK",
>> +[26]= "MANUF_RSVD", [27]= "PROGRAM_CSD",
>> +[28]= "SET_WRITE_PROT", [29]= "CLR_WRITE_PROT",
>> +[30]= "SEND_WRITE_PROT",
>> +[32]= "ERASE_WR_BLK_START", [33]= "ERASE_WR_BLK_END",
>> +[34]= "SW_FUNC_RSVD", [35]= "SW_FUNC_RSVD",
>> +[36]= "SW_FUNC_RSVD", [37]= "SW_FUNC_RSVD",
>> +[38]= "ERASE",
>> +[40]= "DPS_spec",
>> +[42]= "LOCK_UNLOCK",[43]= "Q_MANAGEMENT",
>> +[44]= "Q_TASK_INFO_A", [45]= "Q_TASK_INFO_B",
>> +[46]= "Q_RD_TASK", [47]= "Q_WR_TASK",
>> +[48]= "READ_EXTR_SINGLE", [49]= "WRITE_EXTR_SINGLE",
>> +[50]= "SW_FUNC_RSVD", /* FIXME */
>
> What's this FIXME for? We should either fix whatever it is, or if
> that's not practical the comment needs to describe the problem
> so that a future reader of the code knows what it means and might
> be able to fix it.
Oops, I forgot to search for the CMD51 correct name.
Apparently some future Video extension, so I'll just remove the FIXME.
>
>> +[52]= "IO_RW_DIRECT", [53]= "IO_RW_EXTENDED",
>> +[54]= "SDIO_RSVD", [55]= "APP_CMD",
>> +[56]= "GEN_CMD",[57]= "SW_FUNC_RSVD",
>> +[58]= "READ_EXTR_MULTI",[59]= "WRITE_EXTR_MU
Re: [Qemu-devel] [PATCH v4 08/11] sdcard: display command name when tracing CMD/ACMD
On 15 February 2018 at 22:05, Philippe Mathieu-Daudé wrote:
> put the function in sdmmc-common.c since we will reuse it in hw/sd/core.c
>
As a general note, your commit messages are generally a bit
shorter than I'd prefer. This is to some extent a personal
style question, but if you're writing lots of patches where
the only commit message is the subject-line summary then I
think that it's worth being a bit more descriptive.
Also, the "body" part of the commit message should really
be able to stand alone as a description, rather than relying
on the subject line to make sense.
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/sd/sdmmc-internal.h | 3 +++
> hw/sd/sd.c | 13 +
> hw/sd/sdmmc-common.c | 72
> ++
> hw/sd/Makefile.objs| 2 +-
> hw/sd/trace-events | 8 +++---
> 5 files changed, 88 insertions(+), 10 deletions(-)
> create mode 100644 hw/sd/sdmmc-common.c
>
> diff --git a/hw/sd/sdmmc-internal.h b/hw/sd/sdmmc-internal.h
> index 0e96cb0081..02b730089b 100644
> --- a/hw/sd/sdmmc-internal.h
> +++ b/hw/sd/sdmmc-internal.h
> @@ -12,4 +12,7 @@
>
> #define SDMMC_CMD_MAX 64
>
> +const char *sd_cmd_name(uint8_t cmd);
> +const char *sd_acmd_name(uint8_t cmd);
Can we have doc comments for new function prototypes in header files,
please?
> diff --git a/hw/sd/sdmmc-common.c b/hw/sd/sdmmc-common.c
> new file mode 100644
> index 00..1d0198b1ad
> --- /dev/null
> +++ b/hw/sd/sdmmc-common.c
> @@ -0,0 +1,72 @@
> +/*
> + * SD/MMC cards common helpers
> + *
> + * Copyright (c) 2018 Philippe Mathieu-Daudé
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + * SPDX-License-Identifier: GPL-2.0-or-later
Ooh, a SPDX identifier. The project doesn't actually use these
at the moment, but it doesn't hurt to have one here I guess.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "sdmmc-internal.h"
> +
> +const char *sd_cmd_name(uint8_t cmd)
> +{
> +static const char *cmd_abbrev[SDMMC_CMD_MAX] = {
> + [0]= "GO_IDLE_STATE",
> + [2]= "ALL_SEND_CID",[3]= "SEND_RELATIVE_ADDR",
> + [4]= "SET_DSR", [5]= "IO_SEND_OP_COND",
> + [6]= "SWITCH_FUNC", [7]= "SELECT/DESELECT_CARD",
> + [8]= "SEND_IF_COND",[9]= "SEND_CSD",
> +[10]= "SEND_CID", [11]= "VOLTAGE_SWITCH",
> +[12]= "STOP_TRANSMISSION", [13]= "SEND_STATUS",
> +[15]= "GO_INACTIVE_STATE",
> +[16]= "SET_BLOCKLEN", [17]= "READ_SINGLE_BLOCK",
> +[18]= "READ_MULTIPLE_BLOCK",[19]= "SEND_TUNING_BLOCK",
> +[20]= "SPEED_CLASS_CONTROL",[21]= "DPS_spec",
> +[23]= "SET_BLOCK_COUNT",
> +[24]= "WRITE_BLOCK",[25]= "WRITE_MULTIPLE_BLOCK",
> +[26]= "MANUF_RSVD", [27]= "PROGRAM_CSD",
> +[28]= "SET_WRITE_PROT", [29]= "CLR_WRITE_PROT",
> +[30]= "SEND_WRITE_PROT",
> +[32]= "ERASE_WR_BLK_START", [33]= "ERASE_WR_BLK_END",
> +[34]= "SW_FUNC_RSVD", [35]= "SW_FUNC_RSVD",
> +[36]= "SW_FUNC_RSVD", [37]= "SW_FUNC_RSVD",
> +[38]= "ERASE",
> +[40]= "DPS_spec",
> +[42]= "LOCK_UNLOCK",[43]= "Q_MANAGEMENT",
> +[44]= "Q_TASK_INFO_A", [45]= "Q_TASK_INFO_B",
> +[46]= "Q_RD_TASK", [47]= "Q_WR_TASK",
> +[48]= "READ_EXTR_SINGLE", [49]= "WRITE_EXTR_SINGLE",
> +[50]= "SW_FUNC_RSVD", /* FIXME */
What's this FIXME for? We should either fix whatever it is, or if
that's not practical the comment needs to describe the problem
so that a future reader of the code knows what it means and might
be able to fix it.
> +[52]= "IO_RW_DIRECT", [53]= "IO_RW_EXTENDED",
> +[54]= "SDIO_RSVD", [55]= "APP_CMD",
> +[56]= "GEN_CMD",[57]= "SW_FUNC_RSVD",
> +[58]= "READ_EXTR_MULTI",[59]= "WRITE_EXTR_MULTI",
> +[60]= "MANUF_RSVD", [61]= "MANUF_RSVD",
> +[62]= "MANUF_RSVD", [63]= "MANUF_RSVD",
> +};
> +return cmd_abbrev[cmd] ? cmd_abbrev[cmd] : "UNKNOWN_CMD";
> +}
> +
> +const char *sd_acmd_name(uint8_t cmd)
> +{
> +static const char *acmd_abbrev[SDMMC_CMD_MAX] = {
> + [6] = "SET_BUS_WIDTH",
> +[13] = "SD_STATUS",
> +[14] = "DPS_spec", [15] = "DPS_spec",
> +[16] = "DPS_spec",
> +[18] = "SECU_spec",
> +[22] = "SEND_NUM_WR_BLOCKS",[23] = "SET_WR_BLK_ERASE_COUNT",
> +[41]
