[PATCH v4 4/6] cmd: fru: add product info area parsing support
Jae Hyun Yoo
quic_jaehyoo at quicinc.com
Thu Sep 22 18:15:30 CEST 2022
Hello Michal,
On 9/22/2022 12:19 AM, Michal Simek wrote:
> Hi,
>
> On 9/22/22 08:39, Jae Hyun Yoo wrote:
>> Hello Michal,
>>
>> On 9/21/2022 6:52 AM, Michal Simek wrote:
>>>
>>>
>>> On 8/25/22 18:42, Jae Hyun Yoo wrote:
>>>> Add product info area parsing support. Custom board fields can be
>>>> added dynamically using linked list so that each board support can
>>>> utilize them in their own custom way.
>>>>
>>>> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo at quicinc.com>
>>>> ---
>>>> Changes from v3:
>>>> * None.
>>>>
>>>> Changes from v2:
>>>> * Changed 'struct fru_board_info_member' to 'struct
>>>> fru_common_info_member'.
>>>>
>>>> Changes from v1:
>>>> * Refactored using linked list instead of calling a custom parsing
>>>> callback.
>>>>
>>>> Changes from RFC:
>>>> * Added manufacturer custom product info fields parsing flow.
>>>>
>>>> cmd/fru.c | 28 ++++--
>>>> include/fru.h | 34 ++++++-
>>>> lib/fru_ops.c | 244
>>>> +++++++++++++++++++++++++++++++++++++++++++++++---
>>>> 3 files changed, 286 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/cmd/fru.c b/cmd/fru.c
>>>> index b2cadbec9780..42bdaae09449 100644
>>>> --- a/cmd/fru.c
>>>> +++ b/cmd/fru.c
>>>> @@ -43,11 +43,22 @@ static int do_fru_display(struct cmd_tbl *cmdtp,
>>>> int flag, int argc,
>>>> static int do_fru_generate(struct cmd_tbl *cmdtp, int flag, int argc,
>>>> char *const argv[])
>>>> {
>>>> + int (*fru_generate)(const void *addr, int argc, char*const
>>>> argv[]);
>>>> unsigned long addr;
>>>> const void *buf;
>>>> - int ret;
>>>> + int ret, maxargs;
>>>> +
>>>> + if (!strncmp(argv[2], "-b", 3)) {
>>>> + fru_generate = fru_board_generate;
>>>> + maxargs = cmdtp->maxargs +FRU_BOARD_AREA_TOTAL_FIELDS;
>>>> + } else if (!strncmp(argv[2], "-p", 3)) {
>>>> + fru_generate = fru_product_generate;
>>>> + maxargs = cmdtp->maxargs +FRU_PRODUCT_AREA_TOTAL_FIELDS;
>>>> + } else {
>>>> + return CMD_RET_USAGE;
>>>> + }
>>>> - if (argc < cmdtp->maxargs)
>>>> + if (argc < maxargs)
>>>> return CMD_RET_USAGE;
>>>> addr = hextoul(argv[3], NULL);
>>>> @@ -62,7 +73,7 @@ static int do_fru_generate(struct cmd_tbl *cmdtp,
>>>> int flag, int argc,
>>>> static struct cmd_tbl cmd_fru_sub[] = {
>>>> U_BOOT_CMD_MKENT(capture, 3, 0, do_fru_capture, "", ""),
>>>> U_BOOT_CMD_MKENT(display, 2, 0, do_fru_display, "", ""),
>>>> - U_BOOT_CMD_MKENT(board_gen, 8, 0, do_fru_generate, "", ""),
>>>> + U_BOOT_CMD_MKENT(generate, 4, 0, do_fru_generate, "", ""),
>>>> };
>>>> static int do_fru(struct cmd_tbl *cmdtp, int flag, int argc,
>>>> @@ -90,11 +101,16 @@ static char fru_help_text[] =
>>>> "capture <addr> - Parse and capture FRU table present at
>>>> address.\n"
>>>> "fru display - Displays content of FRU table that was captured
>>>> using\n"
>>>> " fru capture command\n"
>>>> - "fru board_gen <addr> <manufacturer> <board name> <serial
>>>> number>\n"
>>>> - " <part number> <file id> [custom ...] - Generate
>>>> FRU\n"
>>>> - " format with board info area filled based on\n"
>>>> + "fru generate -b <addr> <manufacturer> <board name> <serial
>>>> number>\n"
>>>> + " <part number> <file id> [custom ...] -
>>>> Generate FRU\n"
>>>> + " format with board info area filled based on\n"
>>>
>>> this simply means that all previous user custom scripts will stop to
>>> work.
>>> No problem to deprecated board_gen but with any transition time. It
>>> means keep origin option, create new one and add deprecate message to
>>> origin that scripts should be converted. After 3-4 releases we can
>>> remove it which should be enough time for users to do transition.
>>
>> I agree with you. I'll add the old command back in the next version and
>> will keep for 3-4 releases before deprecating the command.
>>
>>>> " parameters. <addr> is pointing to place where
>>>> FRU is\n"
>>>> " generated.\n"
>>>> + "fru generate -p <addr> <manufacturer> <product name> <part
>>>> number>\n"
>>>> + " <version number> <serial number> <asset
>>>> number>\n"
>>>> + " <file id> [custom ...] - Generate FRU format
>>>> with\n"
>>>> + " product info area filled based on parameters.
>>>> <addr>\n"
>>>> + " is pointing to place where FRU is generated.\n"
>>>
>>> Are you going to use this product generation. I mean it is fine how
>>> it is but maybe in future would make sense to have -b and -p together
>>> to be able to generate both of these fields.
>>> Definitely showing product information is key part here at least for me.
>>
>> Yes, that makes sense. I'll change these commands to 'gen_board' and
>> 'gen_product' with adding back the previous command 'board_gen' to keep
>> its compatibility. A command which can generate both board and product
>> into a single FRU could be added later using a separate series.
>>
>>>> ;
>>>> #endif
>>>> diff --git a/include/fru.h b/include/fru.h
>>>> index b158b80b5866..2b19033a3843 100644
>>>> --- a/include/fru.h
>>>> +++ b/include/fru.h
>>>> @@ -31,7 +31,13 @@ struct fru_board_info_header {
>>>> u8 time[3];
>>>> } __packed;
>>>> -struct fru_board_info_member {
>>>> +struct fru_product_info_header {
>>>> + u8 ver;
>>>> + u8 len;
>>>> + u8 lang_code;
>>>> +} __packed;
>>>> +
>>>> +struct fru_common_info_member {
>>>> u8 type_len;
>>>> u8 *name;
>>>> } __packed;
>>>> @@ -64,6 +70,27 @@ struct fru_board_data {
>>>> struct list_head custom_fields;
>>>> };
>>>> +struct fru_product_data {
>>>> + u8 ver;
>>>> + u8 len;
>>>> + u8 lang_code;
>>>> + u8 manufacturer_type_len;
>>>> + u8 manufacturer_name[FRU_INFO_FIELD_LEN_MAX];
>>>> + u8 product_name_type_len;
>>>> + u8 product_name[FRU_INFO_FIELD_LEN_MAX];
>>>> + u8 part_number_type_len;
>>>> + u8 part_number[FRU_INFO_FIELD_LEN_MAX];
>>>> + u8 version_number_type_len;
>>>> + u8 version_number[FRU_INFO_FIELD_LEN_MAX];
>>>> + u8 serial_number_type_len;
>>>> + u8 serial_number[FRU_INFO_FIELD_LEN_MAX];
>>>> + u8 asset_number_type_len;
>>>> + u8 asset_number[FRU_INFO_FIELD_LEN_MAX];
>>>> + u8 file_id_type_len;
>>>> + u8 file_id[FRU_INFO_FIELD_LEN_MAX];
>>>> + struct list_head custom_fields;
>>>> +};
>>>> +
>>>> struct fru_multirec_hdr {
>>>> u8 rec_type;
>>>> u8 type;
>>>> @@ -85,6 +112,7 @@ struct fru_multirec_node {
>>>> struct fru_table {
>>>> struct fru_common_hdr hdr;
>>>> struct fru_board_data brd;
>>>> + struct fru_product_data prd;
>>>> struct list_head multi_recs;
>>>> bool captured;
>>>> };
>>>> @@ -102,13 +130,15 @@ struct fru_table {
>>>> /* This should be minimum of fields */
>>>> #define FRU_BOARD_AREA_TOTAL_FIELDS 5
>>>> +#define FRU_PRODUCT_AREA_TOTAL_FIELDS 7
>>>> #define FRU_TYPELEN_TYPE_SHIFT 6
>>>> #define FRU_TYPELEN_TYPE_BINARY 0
>>>> #define FRU_TYPELEN_TYPE_ASCII8 3
>>>> int fru_display(int verbose);
>>>> int fru_capture(const void *addr);
>>>> -int fru_generate(const void *addr, int argc, char *const argv[]);
>>>> +int fru_board_generate(const void *addr, int argc, char *const
>>>> argv[]);
>>>> +int fru_product_generate(const void *addr, int argc, char *const
>>>> argv[]);
>>>> u8 fru_checksum(u8 *addr, u8 len);
>>>> int fru_check_type_len(u8 type_len, u8 language, u8 *type);
>>>> const struct fru_table *fru_get_fru_data(void);
>>>> diff --git a/lib/fru_ops.c b/lib/fru_ops.c
>>>> index a25ebccd110d..978d5f8e8a19 100644
>>>> --- a/lib/fru_ops.c
>>>> +++ b/lib/fru_ops.c
>>>> @@ -16,9 +16,18 @@
>>>> struct fru_table fru_data __section(".data") = {
>>>> .brd.custom_fields = LIST_HEAD_INIT(fru_data.brd.custom_fields),
>>>> + .prd.custom_fields = LIST_HEAD_INIT(fru_data.prd.custom_fields),
>>>> .multi_recs = LIST_HEAD_INIT(fru_data.multi_recs),
>>>> };
>>>> +static const char * const fru_typecode_str[] = {
>>>> + "Binary/Unspecified",
>>>> + "BCD plus",
>>>> + "6-bit ASCII",
>>>> + "8-bit ASCII",
>>>> + "2-byte UNICODE"
>>>> +};
>>>
>>> This can be done via separate patch to make this one smaller and
>>> easier to review.
>>
>> It's just moved out from 'fru_display_board' function because it can be
>> used for both 'fru_display_board' and 'fru_display_product' functions.
>> Definately it's a part of this patch and I don't think that it should be
>> submitted using a seperate patch.
>
> No doubt that that it can stay here. It is more about to have just small
> patches which is much much easier to review. It means if one patch moves
> this structure to common location for using for product area generation.
> I need to review 20 LOC. And this patch is smaller which is again easier
> to review without distraction from obvious/easy going changes.
Okay. I'll split this small piece as a separate one.
Thanks,
Jae
More information about the U-Boot
mailing list