Hello Michal,

On 9/21/2022 6:40 AM, Michal Simek wrote:

On 8/25/22 18:42, Jae Hyun Yoo wrote:

...

Refactor FRU handling support to remove Xilinx customization dependency. With this change, single or multiple custom board fields and multi-records can be added dynamically using linked list so that each board support can utilize them in their own custom way.

It's a preparation change for moving the FRU command support to common region.

Signed-off-by: Jae Hyun Yoo quic_jaehyoo@quicinc.com

Changes from v3:   * None.

Changes from v2:   * None.

Changes from v1:   * Newly added in v2.

board/xilinx/common/board.c   |  63 ++++++++--   board/xilinx/common/fru.c     |  12 +-   board/xilinx/common/fru.h     |  76 +++++++-----   board/xilinx/common/fru_ops.c | 227 ++++++++++++++++++++++++----------   4 files changed, 263 insertions(+), 115 deletions(-)

When this patch is applied fru board_gen is not working properly. Before: ZynqMP> fru board_gen 100000 Xilinx test 112545 12551 A ZynqMP> md 100000 00100000: 01000001 fe000000 00000701 58c60000  ...............X 00100010: 6e696c69 6574c478 31c67473 34353231  ilinx.test.11254 00100020: 3231c535 d0313535 6f422d55 6720746f  5.12551.U-Boot g 00100030: 72656e65 726f7461 00c141c1 e7000000  enerator.A...... 00100040: 03000000 04000000 38000000 ad0f2b63  ...........8c+.. 00100050: 03000000 11000000 00000000 42205444  ............DT B 00100060: 20626f6c 61657243 6e6f6974 00000000  lob Creation.... 00100070: 01000000 67616d69 00007365 01000000  ....images...... 00100080: 2d746466 74737973 742d6d65 0000706f  fdt-system-top.. 00100090: 03000000 04000000 54000000 d07f0000  ...........T.... 001000a0: 03000000 04000000 48000000 00000000  ...........H.... 001000b0: 03000000 04000000 00000000 004b4d53  ............SMK. 001000c0: 03000000 08000000 11000000 74616c66  ............flat 001000d0: 0074645f 03000000 06000000 16000000  _dt............. 001000e0: 366d7261 00000034 03000000 05000000  arm64........... 001000f0: 1b000000 656e6f6e 00000000 01000000  ....none........ ZynqMP> fru capture 100000 ZynqMP> fru display *****COMMON HEADER***** Version:1 *** No Internal Area *** *** No Chassis Info Area *** Board Area Offset:8 *** No Product Info Area *** *** No MultiRecord Area *** *****BOARD INFO***** Version:1 Board Area Length:56 Time in Minutes from 0:00hrs 1/1/96: 0  Manufacturer Name: Xilinx  Product Name: test  Serial No: 112545  Part Number: 12551  File ID: U-Boot generator ZynqMP>

when patch is applied.

ZynqMP> fru board_gen 100000 Xilinx test 112545 12551 A ZynqMP> md 100000 00100000: 01000001 fe000000 00000401 74c40000  ...............t 00100010: c6747365 35323131 31c53534 31353532  est.112545.12551 00100020: 00c141c1 f9000000 00000000 00000000  .A.............. 00100030: 00000000 00000000 01000000 00000000  ................ 00100040: 03000000 04000000 38000000 e3102b63  ...........8c+.. 00100050: 03000000 11000000 00000000 42205444  ............DT B 00100060: 20626f6c 61657243 6e6f6974 00000000  lob Creation.... 00100070: 01000000 67616d69 00007365 01000000  ....images...... 00100080: 2d746466 74737973 742d6d65 0000706f  fdt-system-top.. 00100090: 03000000 04000000 54000000 d07f0000  ...........T.... 001000a0: 03000000 04000000 48000000 00000000  ...........H.... 001000b0: 03000000 04000000 00000000 004b4d53  ............SMK. 001000c0: 03000000 08000000 11000000 74616c66  ............flat 001000d0: 0074645f 03000000 06000000 16000000  _dt............. 001000e0: 366d7261 00000034 03000000 05000000  arm64........... 001000f0: 1b000000 656e6f6e 00000000 01000000  ....none........ ZynqMP> fru capture 100000 Board area require minimum 5 fields ZynqMP>

it is clear that manufacturer parameter is ignored.

The 'fru board_gen' command will be replaced with 'fru generate -b' command if you apply all patches in this series.

fru generate -b <addr> <manufacturer> <board name> <serial number> <part number> <file id> [custom ...] - Generate FRU format with board info area filled based on parameters. <addr> is pointing to place where FRU is generated.

It works well using the new command like below. (I changed the last argument to AA fro your test example because field info length should be longer than 2)

=> fru generate -b 100000 Xilinx test 112545 12551 AA => md 100000 00100000: 01000001 fe000000 00000501 58c60000 ...............X 00100010: 6e696c69 6574c478 31c67473 34353231 ilinx.test.11254 00100020: 3231c535 c2313535 00c14141 74000000 5.12551.AA.....t 00100030: 00000000 00000000 00000000 00000000 ................ 00100040: 00000000 00000000 00000000 00000000 ................ 00100050: 00000000 00000000 00000000 00000000 ................ 00100060: 00000000 00000000 00000000 00000000 ................ 00100070: 00000000 00000000 00000000 00000000 ................ 00100080: 00000000 00000000 00000000 00000000 ................ 00100090: 00000000 00000000 00000000 00000000 ................ 001000a0: 00000000 00000000 00000000 00000000 ................ 001000b0: 00000000 00000000 00000000 00000000 ................ 001000c0: 00000000 00000000 00000000 00000000 ................ 001000d0: 00000000 00000000 00000000 00000000 ................ 001000e0: 00000000 00000000 00000000 00000000 ................ 001000f0: 00000000 00000000 00000000 00000000 ................ => fru capture 100000 => fru display *****COMMON HEADER***** Version:1 *** No Internal Area *** *** No Chassis Info Area *** Board Area Offset:8 *** No Product Info Area *** *** No MultiRecord Area *** *****BOARD INFO***** Version:1 Board Area Length:40 Time in Minutes from 0:00hrs 1/1/96: 0 Manufacturer Name: Xilinx Product Name: test Serial Number: 112545 Part Number: 12551 File ID: AA *****PRODUCT INFO***** Version:0 Product Area Length:0 *****MULTIRECORDS***** =>

I changed the previous hardcoded File ID setting as configurable so the last argument 'AA' is applied to the File ID field.

I agree with your comment on the 4th patch in this series that we need to keep the old command for a while before deprecating the command to support all previous user custom scripts. I'll add the 'fru board_gen' command back in the next version.

...

diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c index 9b4aded466ab..e979f0176273 100644 --- a/board/xilinx/common/board.c +++ b/board/xilinx/common/board.c @@ -88,6 +88,9 @@ int zynq_board_read_rom_ethaddr(unsigned char *ethaddr)   #define EEPROM_HDR_NO_OF_MAC_ADDR    4   #define EEPROM_HDR_ETH_ALEN        ETH_ALEN   #define EEPROM_HDR_UUID_LEN        16 +#define EEPROM_MULTIREC_TYPE_XILINX_OEM    0xD2 +#define EEPROM_MULTIREC_MAC_OFFSET    4 +#define EEPROM_MULTIREC_DUT_MACID    0x31   struct xilinx_board_description {       u32 header; @@ -116,6 +119,19 @@ struct xilinx_legacy_format {       char unused3[29]; /* 0xe3 */   }; +struct xilinx_multirec_mac { +    u8 xlnx_iana_id[3]; +    u8 ver; +    u8 macid[EEPROM_HDR_NO_OF_MAC_ADDR][ETH_ALEN]; +};

+enum xilinx_board_custom_field { +    brd_custom_field_rev = 0, +    brd_custom_field_pcie, +    brd_custom_field_uuid, +    brd_custom_field_max +};

static void xilinx_eeprom_legacy_cleanup(char *eeprom, int size)   {       int i; @@ -200,9 +216,14 @@ static bool xilinx_detect_legacy(u8 *buffer)   static int xilinx_read_eeprom_fru(struct udevice *dev, char *name,                     struct xilinx_board_description *desc)   { +    u8 parsed_macid[EEPROM_HDR_NO_OF_MAC_ADDR][ETH_ALEN]= { 0 }; +    struct fru_custom_info custom_info[brd_custom_field_max] = { 0 }; +    struct fru_custom_field_node *ci_node; +    struct fru_multirec_node *mr_node; +    const struct fru_table *fru_data;       int i, ret, eeprom_size;       u8 *fru_content; -    u8 id = 0; +    u8 id = 0, field = 0;       /* FIXME this is shortcut - if eeprom type is wrong it will fail */       eeprom_size = i2c_eeprom_size(dev); @@ -237,30 +258,56 @@ static int xilinx_read_eeprom_fru(struct udevice *dev, char *name,           goto end;       } +    fru_data = fru_get_fru_data();

+    list_for_each_entry(ci_node, &fru_data->brd.custom_fields, list) { +        custom_info[field].type_len = ci_node->info.type_len; +        memcpy(custom_info[field].data, ci_node->info.data, +               ci_node->info.type_len & FRU_TYPELEN_LEN_MASK); +        if (++field < brd_custom_field_max) +            break; +    }

+    list_for_each_entry(mr_node, &fru_data->multi_recs, list) { +        struct fru_multirec_hdr *hdr= &mr_node->info.hdr;

+        if (hdr->rec_type == EEPROM_MULTIREC_TYPE_XILINX_OEM) { +            struct xilinx_multirec_mac *mac;

+            mac = (struct xilinx_multirec_mac *)mr_node->info.data; +            if (mac->ver == EEPROM_MULTIREC_DUT_MACID) { +                int mac_len = hdr->len - +                          EEPROM_MULTIREC_MAC_OFFSET;

+                memcpy(parsed_macid, mac->macid, mac_len); +            } +        } +    }

/* It is clear that FRU was captured and structures were filled */ -    strlcpy(desc->manufacturer, (char *)fru_data.brd.manufacturer_name, +    strlcpy(desc->manufacturer, (char *)fru_data->brd.manufacturer_name,           sizeof(desc->manufacturer)); -    strlcpy(desc->uuid, (char *)fru_data.brd.uuid, +    strlcpy(desc->uuid, (char *)custom_info[brd_custom_field_uuid].data,           sizeof(desc->uuid)); -    strlcpy(desc->name, (char *)fru_data.brd.product_name, +    strlcpy(desc->name, (char *)fru_data->brd.product_name,           sizeof(desc->name));       for (i = 0; i < sizeof(desc->name); i++) {           if (desc->name[i]== ' ')               desc->name[i] = '\0';       } -    strlcpy(desc->revision, (char *)fru_data.brd.rev, +    strlcpy(desc->revision, (char *)custom_info[brd_custom_field_rev].data,           sizeof(desc->revision));       for (i = 0; i < sizeof(desc->revision);i++) {           if (desc->revision[i] == ' ')               desc->revision[i] = '\0';       } -    strlcpy(desc->serial, (char *)fru_data.brd.serial_number, +    strlcpy(desc->serial, (char *)fru_data->brd.serial_number,           sizeof(desc->serial));       while (id < EEPROM_HDR_NO_OF_MAC_ADDR) { -        if (is_valid_ethaddr((const u8 *)fru_data.mac.macid[id])) +        if (is_valid_ethaddr((const u8 *)parsed_macid[id]))               memcpy(&desc->mac_addr[id], -                   (char *)fru_data.mac.macid[id], ETH_ALEN); +                   (char *)parsed_macid[id], ETH_ALEN);           id++;       } diff --git a/board/xilinx/common/fru.c b/board/xilinx/common/fru.c index f6ca46c3cecc..0d72911df04d 100644 --- a/board/xilinx/common/fru.c +++ b/board/xilinx/common/fru.c @@ -1,6 +1,7 @@   // SPDX-License-Identifier: GPL-2.0   /*    * (C) Copyright 2019 - 2020 Xilinx, Inc.

• • Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights

reserved.    */   #include <common.h> @@ -43,7 +44,7 @@ static int do_fru_generate(struct cmd_tbl *cmdtp, int flag, int argc,       addr = hextoul(argv[2], NULL); -    return fru_generate(addr, argv[3], argv[4], argv[5],argv[6], argv[7]); +    return fru_generate(addr, argc - 3, argv + 3);   }   static struct cmd_tbl cmd_fru_sub[] = { @@ -78,14 +79,15 @@ static char fru_help_text[] =       "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> <revision> - Generate FRU format with\n" -    "              board info area filled based on parameters. <addr> is\n" -    "              pointing to place where FRU is generated.\n" +    "              <part number> <file id> [custom ...] - Generate FRU\n"

That command was created for generating fragment with board revision. Here you are changing it to file id without any reason why.

Intention of this series is to move the 'fru' command into common place so I removed the Xilinx dependent custom field, 'revision'. The revision field can be added as one of generic custom info fields flexibly using the new command. I also modified 'board/xilinx/common/board.c' file to make it parse the custom revision field in a generic way so please check the code.

...

+    "              format with board info area filled based on\n" +    "                parameters. <addr> is pointing to place where FRU is\n" +    "                generated.\n"       ;   #endif   U_BOOT_CMD( -    fru, 8, 1, do_fru, +    fru, CONFIG_SYS_MAXARGS, 1, do_fru,

What's the intention to change this? We have this macro setup to 64. Do you want to support to write all fields?

As I described above, the new command can add multiple custom fields flexibly by taking multiple arguments so it's the reason of this change.

fru generate -b <addr> <manufacturer> <board name> <serial number> <part number> <file id> [custom ...] - Generate FRU format with board info area filled based on parameters. <addr> is pointing to place where FRU is generated.

...

"FRU table info",       fru_help_text   ) diff --git a/board/xilinx/common/fru.h b/board/xilinx/common/fru.h index 59f6b722cf12..41655864dbf5 100644 --- a/board/xilinx/common/fru.h +++ b/board/xilinx/common/fru.h @@ -2,11 +2,13 @@   /*    * (C) Copyright 2019 Xilinx, Inc.    * Siva Durga Prasad Paladugu siva.durga.paladugu@xilinx.com

• • Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights

reserved.    */   #ifndef __FRU_H   #define __FRU_H -#include <net.h>

+#include <linux/list.h>   struct fru_common_hdr {       u8 version; @@ -17,21 +19,31 @@ struct fru_common_hdr {       u8 off_multirec;       u8 pad;       u8 crc; -}; +} __packed; -#define FRU_BOARD_MAX_LEN    32 -#define FRU_MAX_NO_OF_MAC_ADDR    4 +#define FRU_INFO_FIELD_LEN_MAX        32

This is pretty much just macro name change in a lot of places. It should be done in separate patch.

FRU_MAX_NO_OF_MAC_ADDR is Xilinx dependent customization so I moved it to 'board/xilinx/common/board.c'.

FRU_BOARD_MAX_LEN means maximum length of info field in FRU so it should be used for the product info so I changed it to FRU_INFO_FIELD_LEN_MAX which is more generic macro name. This change is also a part of this patch.

...

+#define FRU_MULTIREC_DATA_LEN_MAX    255 -struct __packed fru_board_info_header { +struct fru_board_info_header {       u8 ver;       u8 len;       u8 lang_code;       u8 time[3]; -}; +} __packed; -struct __packed fru_board_info_member { +struct fru_board_info_member {       u8 type_len;       u8 *name; +} __packed;

Also pretty much self contained change without proper description.

I just moved __packed to the end of struct definition so it doesn't make any functional change but it improves browsability of this code since it's easier to search 'struct fru_board_info_header' instead of 'struct __packed fru_board_info_header'.

...

+struct fru_custom_info { +    u8 type_len; +    u8 data[FRU_INFO_FIELD_LEN_MAX]; +} __packed;

+struct fru_custom_field_node { +    struct list_head list; +    struct fru_custom_info info;   };   struct fru_board_data { @@ -40,22 +52,16 @@ struct fru_board_data {       u8 lang_code;       u8 time[3];       u8 manufacturer_type_len; -    u8 manufacturer_name[FRU_BOARD_MAX_LEN]; +    u8 manufacturer_name[FRU_INFO_FIELD_LEN_MAX];       u8 product_name_type_len; -    u8 product_name[FRU_BOARD_MAX_LEN]; +    u8 product_name[FRU_INFO_FIELD_LEN_MAX];       u8 serial_number_type_len; -    u8 serial_number[FRU_BOARD_MAX_LEN]; +    u8 serial_number[FRU_INFO_FIELD_LEN_MAX];       u8 part_number_type_len; -    u8 part_number[FRU_BOARD_MAX_LEN]; +    u8 part_number[FRU_INFO_FIELD_LEN_MAX];       u8 file_id_type_len; -    u8 file_id[FRU_BOARD_MAX_LEN]; -    /* Xilinx custom fields */ -    u8 rev_type_len; -    u8 rev[FRU_BOARD_MAX_LEN]; -    u8 pcie_type_len; -    u8 pcie[FRU_BOARD_MAX_LEN]; -    u8 uuid_type_len; -    u8 uuid[FRU_BOARD_MAX_LEN]; +    u8 file_id[FRU_INFO_FIELD_LEN_MAX]; +    struct list_head custom_fields;   };   struct fru_multirec_hdr { @@ -64,32 +70,35 @@ struct fru_multirec_hdr {       u8 len;       u8 csum;       u8 hdr_csum; -}; +} __packed; -struct fru_multirec_mac { -    u8 xlnx_iana_id[3]; -    u8 ver; -    u8 macid[FRU_MAX_NO_OF_MAC_ADDR][ETH_ALEN]; +struct fru_multirec_info { +    struct fru_multirec_hdr hdr; +    u8 data[FRU_MULTIREC_DATA_LEN_MAX]; +} __packed;

+struct fru_multirec_node { +    struct list_head list; +    struct fru_multirec_info info;   };   struct fru_table {       struct fru_common_hdr hdr;       struct fru_board_data brd; -    struct fru_multirec_mac mac; +    struct list_head multi_recs;       bool captured;   }; -#define FRU_TYPELEN_CODE_MASK    0xC0 -#define FRU_TYPELEN_LEN_MASK    0x3F +#define FRU_TYPELEN_CODE_MASK       0xC0 +#define FRU_TYPELEN_LEN_MASK        0x3F   #define FRU_COMMON_HDR_VER_MASK        0xF   #define FRU_COMMON_HDR_LEN_MULTIPLIER    8   #define FRU_LANG_CODE_ENGLISH        0   #define FRU_LANG_CODE_ENGLISH_1        25   #define FRU_TYPELEN_EOF            0xC1 -#define FRU_MULTIREC_TYPE_OEM       0xD2 -#define FRU_MULTIREC_MAC_OFFSET        4   #define FRU_LAST_REC            BIT(7) -#define FRU_DUT_MACID            0x31 +#define FRU_MULTIREC_TYPE_OEM_START    0xC0 +#define FRU_MULTIREC_TYPE_OEM_END    0xFF   /* This should be minimum of fields */   #define FRU_BOARD_AREA_TOTAL_FIELDS    5 @@ -99,10 +108,9 @@ struct fru_table {   int fru_display(int verbose);   int fru_capture(unsigned long addr); -int fru_generate(unsigned long addr, char *manufacturer, char *board_name, -         char *serial_no, char *part_no, char *revision); +int fru_generate(unsigned long addr, int argc, char *const argv[]);   u8 fru_checksum(u8 *addr, u8 len);

-extern struct fru_table fru_data; +int fru_check_type_len(u8 type_len, u8 language, u8 *type);

This is used just inside fru_ops.c that means that change in this patch is unrelated to change you need to do here. Please move it to patch which really needs this.

Oh, you are right. I'll remove this prototype definition from this header and will make the function as a static in fru_ops.c.

...

+const struct fru_table *fru_get_fru_data(void);   #endif /* FRU_H */ diff --git a/board/xilinx/common/fru_ops.c b/board/xilinx/common/fru_ops.c index 49846ae3d660..8291e347716b 100644 --- a/board/xilinx/common/fru_ops.c +++ b/board/xilinx/common/fru_ops.c @@ -1,21 +1,24 @@   // SPDX-License-Identifier: GPL-2.0   /*    * (C) Copyright 2019 - 2020 Xilinx, Inc.

• • Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights

reserved.    */   #include <common.h> -#include <cpu_func.h>   #include <env.h>   #include <fdtdec.h> +#include <hexdump.h>   #include <log.h>   #include <malloc.h> -#include <net.h>   #include <asm/io.h> -#include <asm/arch/hardware.h> +#include <linux/compat.h>   #include "fru.h" -struct fru_table fru_data __section(".data"); +struct fru_table fru_data __section(".data") = { +    .brd.custom_fields = LIST_HEAD_INIT(fru_data.brd.custom_fields), +    .multi_recs = LIST_HEAD_INIT(fru_data.multi_recs), +};   static u16 fru_cal_area_len(u8 len)   { @@ -57,7 +60,7 @@ u8 fru_checksum(u8 *addr, u8 len)       return checksum;   } -static int fru_check_type_len(u8 type_len, u8 language, u8 *type) +int fru_check_type_len(u8 type_len, u8 language, u8 *type)   {       int len; @@ -89,8 +92,7 @@ static u8 fru_gen_type_len(u8 *addr, char *name)       return 1 + len;   } -int fru_generate(unsigned long addr, char *manufacturer, char *board_name, -         char *serial_no, char *part_no, char *revision) +int fru_generate(unsigned long addr, int argc, char *const argv[])

This is again the self contained change where you want to support generation for more fields in the patch which is doing a lot of other things.

I am fine with changing it but in steps. It means improve fru generation. Renaming macros, extraction OEM fields from board are, change multi record handling.

Okay. I'll split this patch as you suggested.

...

{       struct fru_common_hdr *header = (structfru_common_hdr *)addr;       struct fru_board_info_header *board_info; @@ -126,18 +128,10 @@ int fru_generate(unsigned long addr, char *manufacturer, char *board_name,       /* Member fields are just after board_info header */       member = (u8 *)board_info + sizeof(*board_info); -    len = fru_gen_type_len(member, manufacturer); /* Board Manufacturer */ -    member += len; -    len = fru_gen_type_len(member, board_name); /* Board Product name */ -    member += len; -    len = fru_gen_type_len(member, serial_no); /* Board Serial number */ -    member += len; -    len = fru_gen_type_len(member, part_no); /* Board part number */ -    member += len; -    len = fru_gen_type_len(member, "U-Boot generator"); /* File ID */ -    member += len; -    len = fru_gen_type_len(member, revision); /* Revision */ -    member += len; +    while (--argc > 0 && ++argv) { +        len = fru_gen_type_len(member, *argv); +        member += len; +    }       *member++ = 0xc1; /* Indication of no more fields */ @@ -168,6 +162,35 @@ int fru_generate(unsigned long addr, char *manufacturer, char *board_name,       return 0;   } +static void fru_delete_custom_fields(struct list_head *custom_fields) +{ +    struct fru_custom_field_node *node, *next;

+    list_for_each_entry_safe(node, next, custom_fields, list) { +        list_del(&node->list); +        kfree(node); +    } +}

+static int fru_append_custom_info(unsigned long addr, +                  struct list_head *custom_fields) +{ +    struct fru_custom_info *info = (struct fru_custom_info *)addr; +    struct fru_custom_field_node *ci_new;

+    ci_new = kzalloc(sizeof(*ci_new), GFP_KERNEL); +    if (!ci_new) +        return -ENOMEM;

+    ci_new->info.type_len = info->type_len; +    memcpy(&ci_new->info.data, (void *)info->data, +           ci_new->info.type_len & FRU_TYPELEN_LEN_MASK);

+    list_add_tail(&ci_new->list, custom_fields);

+    return 0; +}

static int fru_parse_board(unsigned long addr)   {       u8 i, type; @@ -179,9 +202,10 @@ static int fru_parse_board(unsigned long addr)       data = (u8 *)&fru_data.brd.manufacturer_type_len;       /* Record max structure limit not to write data over allocated space */ -    limit = (u8 *)&fru_data.brd + sizeof(struct fru_board_data); +    limit = (u8 *)&fru_data.brd + sizeof(struct fru_board_data) - +        sizeof(struct fru_custom_field_node *); -    for (i = 0; ; i++, data += FRU_BOARD_MAX_LEN) { +    for (i = 0; ; i++, data += FRU_INFO_FIELD_LEN_MAX) {           len = fru_check_type_len(*(u8 *)addr, fru_data.brd.lang_code,                        &type);           /* @@ -190,11 +214,14 @@ static int fru_parse_board(unsigned long addr)           if (len == -EINVAL)               break; -        /* Stop when amount of charsis more then fields to record */ -        if (data + len > limit) -            break; -        /* This record type/len field */ -        *data++ = *(u8 *)addr; +        if (i < FRU_BOARD_AREA_TOTAL_FIELDS) { +            /* Stop when length is more then fields to record */ +            if (data + len > limit) +                break;

+            /* This record type/len field */ +            *data++ = *(u8 *)addr; +        }           /* Add offset to match data */           addr += 1; @@ -203,10 +230,23 @@ static int fru_parse_board(unsigned long addr)           if (!len)               continue; -        /* Record data field */ -        memcpy(data, (u8 *)addr, len); -        term = data + (u8)len; -        *term = 0; +        if (i < FRU_BOARD_AREA_TOTAL_FIELDS) { +            /* Record data field */ +            memcpy(data, (u8 *)addr, len); +            term= data + (u8)len; +            *term = 0; +        } else { +            struct list_head *custom_fields;

+            custom_fields = &fru_data.brd.custom_fields;

+            /* Add a custom info field */ +            if (fru_append_custom_info(addr - 1, custom_fields)) { +                fru_delete_custom_fields(custom_fields); +                return -EINVAL; +            } +        }

addr += len;       } @@ -219,34 +259,52 @@ static int fru_parse_board(unsigned long addr)       return 0;   } +static void fru_delete_multirecs(struct list_head *multi_recs) +{ +    struct fru_multirec_node *node, *next;

+    list_for_each_entry_safe(node, next, multi_recs, list) { +        list_del(&node->list); +        kfree(node); +    } +}

+static int fru_append_multirec(unsigned long addr, +                   struct list_head *multi_recs) +{ +    struct fru_multirec_info *mr_src = (struct fru_multirec_info *)addr; +    struct fru_multirec_node *mr_new;

+    mr_new = kzalloc(sizeof(*mr_new), GFP_KERNEL); +    if (!mr_new) +        return -ENOMEM;

+    memcpy(&mr_new->info.hdr, (void *)&mr_src->hdr, sizeof(mr_src->hdr)); +    memcpy(&mr_new->info.data, (void *)mr_src->data, mr_src->hdr.len);

+    list_add_tail(&mr_new->list, multi_recs);

+    return 0; +}

static int fru_parse_multirec(unsigned long addr)   { -    struct fru_multirec_hdr mrc; -    u8 checksum = 0;       u8 hdr_len = sizeof(struct fru_multirec_hdr); -    int mac_len = 0; +    struct fru_multirec_hdr *mr_hdr; -    debug("%s: multirec addr %lx\n", __func__, addr); +    debug("%s: multirec addr %lx\n", __func__, (ulong)addr);       do { -        memcpy(&mrc.rec_type, (void *)addr, hdr_len);

-        checksum = fru_checksum((u8 *)addr, hdr_len); -        if (checksum) { +        if (fru_checksum((u8 *)addr,hdr_len)) {               debug("%s header CRC error\n", __func__);               return -EINVAL;           } -        if (mrc.rec_type == FRU_MULTIREC_TYPE_OEM) { -            struct fru_multirec_mac *mac = (void *)addr + hdr_len; +        fru_append_multirec(addr, &fru_data.multi_recs); -            if (mac->ver == FRU_DUT_MACID) { -                mac_len = mrc.len - FRU_MULTIREC_MAC_OFFSET; -                memcpy(&fru_data.mac.macid, mac->macid, mac_len); -            } -        } -        addr += mrc.len + hdr_len; -    } while (!(mrc.type & FRU_LAST_REC)); +        mr_hdr = (struct fru_multirec_hdr *)addr; +        addr += mr_hdr->len + hdr_len; +    } while (!(mr_hdr->type & FRU_LAST_REC));       return 0;   } @@ -255,7 +313,6 @@ int fru_capture(unsigned long addr)   {       struct fru_common_hdr *hdr;       u8 checksum = 0; -    unsigned long multirec_addr = addr;       checksum = fru_checksum((u8 *)addr, sizeof(struct fru_common_hdr));       if (checksum) { @@ -264,33 +321,28 @@ int fru_capture(unsigned long addr)       }       hdr = (struct fru_common_hdr *)addr; +    fru_delete_custom_fields(&fru_data.brd.custom_fields); +    fru_delete_multirecs(&fru_data.multi_recs);       memset((void *)&fru_data, 0, sizeof(fru_data)); -    memcpy((void *)&fru_data, (void *)hdr, -           sizeof(struct fru_common_hdr)); +    INIT_LIST_HEAD(&fru_data.brd.custom_fields); +    INIT_LIST_HEAD(&fru_data.multi_recs); +    memcpy((void *)&fru_data, (void *)hdr, sizeof(struct fru_common_hdr));       fru_data.captured = true; -    if (hdr->off_board) { -        addr += fru_cal_area_len(hdr->off_board); -        fru_parse_board(addr); -    } +    if (hdr->off_board) +        fru_parse_board(addr + fru_cal_area_len(hdr->off_board)); -    env_set_hex("fru_addr", addr); +    if (hdr->off_multirec) +        fru_parse_multirec(addr + fru_cal_area_len(hdr->off_multirec)); -    if (hdr->off_multirec) { -        multirec_addr += fru_cal_area_len(hdr->off_multirec); -        fru_parse_multirec(multirec_addr); -    } +    env_set_hex("fru_addr", (ulong)addr);       return 0;   }   static int fru_display_board(struct fru_board_data *brd, int verbose)   { -    u32 time = 0; -    u8 type; -    int len; -    u8 *data;       static const char * const typecode[] = {           "Binary/Unspecified",           "BCD plus", @@ -301,12 +353,15 @@ static int fru_display_board(struct fru_board_data *brd, int verbose)       static const char * const boardinfo[] ={           "Manufacturer Name",           "Product Name", -        "Serial No", +        "Serial Number",           "Part Number",           "File ID", -        /* Xilinx spec */ -        "Revision Number",       }; +    struct fru_custom_field_node *node; +    u32 time = 0; +    u8 *data; +    u8 type; +    int len;       if (verbose) {           printf("*****BOARD INFO*****\n"); @@ -358,7 +413,32 @@ static int fru_display_board(struct fru_board_data *brd, int verbose)               debug("Unsupported type %x\n", type);           } -        data += FRU_BOARD_MAX_LEN; +        data += FRU_INFO_FIELD_LEN_MAX; +    }

+    list_for_each_entry(node, &brd->custom_fields, list){ +        printf(" Custom Type/Length:0x%x\n", node->info.type_len); +        print_hex_dump_bytes(" ", DUMP_PREFIX_OFFSET, node->info.data, +                     node->info.type_len & +                     FRU_TYPELEN_LEN_MASK);

this is pretty much debug information. Bootlog contains a lot of data and won't look good. I think this should be moved to verbose or debug.

File ID: 0x0  Custom Type/Length: 0xc8   00000000: 41 00 00 00 00 00 00 00                          A.......  Custom Type/Length: 0x8   00000000: 10 ee 00 00 00 00 00 00                          ........  Custom Type/Length: 0x10   00000000: 52 64 da 0f 73 44 46 03 ac 1b 1e f6 35 92 3c f1 Rd..sDF.....5.<. Type ID: 0x2, Type: 0x2 Length: 13  00000000: 01 f4 01 c2 01 26 02 64 00 00 00 a0 0f           .....&.d..... Type ID: 0xd2, Type: 0x2 Length: 10  00000000: da 10 00 31 00 0a 35 10 39 cd                    ...1..5.9. Type ID: 0xd3, Type: 0x82 Length: 87  00000000: da 10 00 4d 65 6d 6f 72 79 3a 20 51 53 50 49 3a  ...Memory: QSPI:  00000010: 35 31 32 4d 62 20 20 00 4d 65 6d 6f 72 79 3a 20  512Mb .Memory:  00000020: 65 4d 4d 43 3a 4e 6f 6e 65 20 20 20 00 4d 65 6d  eMMC:None .Mem  00000030: 6f 72 79 3a 20 50 53 44 44 52 34 3a 34 47 42 20  ory: PSDDR4:4GB  00000040: 20 00 4d 65 6d 6f 72 79 3a 20 50 4c 44 44 52 34   .Memory: PLDDR4  00000050: 3a 4e 6f 6e 65 20 00                             :None .

No, it's not a debug printing but a printing of custom field data in hex dump because the field can be any types other than string type.

...

+    }

+    return 0; +}

+static int fru_display_multirec(struct list_head *multi_recs, int verbose) +{ +    struct fru_multirec_node *node;

+    if (verbose) +        printf("*****MULTIRECORDS*****\n");

+    list_for_each_entry(node, multi_recs, list) { +        printf("Type ID: 0x%x, Type:0x%x Length: %d\n", +               node->info.hdr.rec_type, node->info.hdr.type, +               node->info.hdr.len); +        print_hex_dump_bytes(" ", DUMP_PREFIX_OFFSET, node->info.data, +                     node->info.hdr.len);

And here again the same.

No, it's also an intentional printing out in hex dump format to support it in generic way. For an example, if we add a MAX address as a multi record, it should be printed out in hex dump instead of a string format.

...

}       return 0; @@ -404,6 +484,8 @@ static void fru_display_common_hdr(struct fru_common_hdr *hdr, int verbose)   int fru_display(int verbose)   { +    int ret;

if (!fru_data.captured) {           printf("FRU data not available please run fru parse\n");           return -EINVAL; @@ -411,5 +493,14 @@ int fru_display(int verbose)       fru_display_common_hdr(&fru_data.hdr, verbose); -    return fru_display_board(&fru_data.brd, verbose); +    ret = fru_display_board(&fru_data.brd, verbose); +    if (ret) +        return ret;

+    return fru_display_multirec(&fru_data.multi_recs, verbose); +}

+const struct fru_table *fru_get_fru_data(void) +{ +    return &fru_data;   }

Definitely patch is going in the right direction to remove Xilinx custom parts but I would like to see this to be split to couple of patches. I don't think this is going to be problem and it will be much easier to review.

Okay. I'll split out this patch to couple of patches in the next version.

Thank you,

Jae

Hi,

On 9/22/2022 12:29 AM, Michal Simek wrote:

Hi,

On 9/22/22 08:39, Jae Hyun Yoo wrote:

...

Hello Michal,

On 9/21/2022 6:40 AM, Michal Simek wrote:

...

On 8/25/22 18:42, Jae Hyun Yoo wrote:

...

Refactor FRU handling support to remove Xilinx customization dependency. With this change, single or multiple custom board fields and multi-records can be added dynamically using linked list so that each board support can utilize them in their own custom way.

It's a preparation change for moving the FRU command support to common region.

Signed-off-by: Jae Hyun Yoo quic_jaehyoo@quicinc.com

Changes from v3:   * None.

Changes from v2:   * None.

Changes from v1:   * Newly added in v2.

board/xilinx/common/board.c   |  63 ++++++++--   board/xilinx/common/fru.c     |  12 +-   board/xilinx/common/fru.h     |  76 +++++++-----   board/xilinx/common/fru_ops.c | 227 ++++++++++++++++++++++++----------   4 files changed, 263 insertions(+), 115 deletions(-)

When this patch is applied fru board_gen is not working properly. Before: ZynqMP> fru board_gen 100000 Xilinx test 112545 12551 A ZynqMP> md 100000 00100000: 01000001 fe000000 00000701 58c60000  ...............X 00100010: 6e696c69 6574c478 31c67473 34353231  ilinx.test.11254 00100020: 3231c535 d0313535 6f422d55 6720746f  5.12551.U-Boot g 00100030: 72656e65 726f7461 00c141c1 e7000000  enerator.A...... 00100040: 03000000 04000000 38000000 ad0f2b63  ...........8c+.. 00100050: 03000000 11000000 00000000 42205444  ............DT B 00100060: 20626f6c 61657243 6e6f6974 00000000  lob Creation.... 00100070: 01000000 67616d69 00007365 01000000  ....images...... 00100080: 2d746466 74737973 742d6d65 0000706f  fdt-system-top.. 00100090: 03000000 04000000 54000000 d07f0000  ...........T.... 001000a0: 03000000 04000000 48000000 00000000  ...........H.... 001000b0: 03000000 04000000 00000000 004b4d53  ............SMK. 001000c0: 03000000 08000000 11000000 74616c66  ............flat 001000d0: 0074645f 03000000 06000000 16000000  _dt............. 001000e0: 366d7261 00000034 03000000 05000000  arm64........... 001000f0: 1b000000 656e6f6e 00000000 01000000  ....none........ ZynqMP> fru capture 100000 ZynqMP> fru display *****COMMON HEADER***** Version:1 *** No Internal Area *** *** No Chassis Info Area *** Board Area Offset:8 *** No Product Info Area *** *** No MultiRecord Area *** *****BOARD INFO***** Version:1 Board Area Length:56 Time in Minutes from 0:00hrs 1/1/96: 0   Manufacturer Name: Xilinx   Product Name: test   Serial No: 112545   Part Number: 12551   File ID: U-Boot generator ZynqMP>

when patch is applied.

ZynqMP> fru board_gen 100000 Xilinx test 112545 12551 A ZynqMP> md 100000 00100000: 01000001 fe000000 00000401 74c40000  ...............t 00100010: c6747365 35323131 31c53534 31353532  est.112545.12551 00100020: 00c141c1 f9000000 00000000 00000000  .A.............. 00100030: 00000000 00000000 01000000 00000000  ................ 00100040: 03000000 04000000 38000000 e3102b63  ...........8c+.. 00100050: 03000000 11000000 00000000 42205444  ............DT B 00100060: 20626f6c 61657243 6e6f6974 00000000  lob Creation.... 00100070: 01000000 67616d69 00007365 01000000  ....images...... 00100080: 2d746466 74737973 742d6d65 0000706f  fdt-system-top.. 00100090: 03000000 04000000 54000000 d07f0000  ...........T.... 001000a0: 03000000 04000000 48000000 00000000  ...........H.... 001000b0: 03000000 04000000 00000000 004b4d53  ............SMK. 001000c0: 03000000 08000000 11000000 74616c66  ............flat 001000d0: 0074645f 03000000 06000000 16000000  _dt............. 001000e0: 366d7261 00000034 03000000 05000000  arm64........... 001000f0: 1b000000 656e6f6e 00000000 01000000  ....none........ ZynqMP> fru capture 100000 Board area require minimum 5 fields ZynqMP>

it is clear that manufacturer parameter is ignored.

The 'fru board_gen' command will be replaced with 'fru generate -b' command if you apply all patches in this series.

fru generate -b <addr> <manufacturer> <board name> <serial number>                  <part number> <file id> [custom ...] - Generate FRU                  format with board info area filled based on                  parameters. <addr> is pointing to place where FRU is                  generated.

It works well using the new command like below. (I changed the last argument to AA fro your test example because field info length should be longer than 2)

=> fru generate -b 100000 Xilinx test 112545 12551 AA => md 100000 00100000: 01000001 fe000000 00000501 58c60000  ...............X 00100010: 6e696c69 6574c478 31c67473 34353231  ilinx.test.11254 00100020: 3231c535 c2313535 00c14141 74000000  5.12551.AA.....t 00100030: 00000000 00000000 00000000 00000000  ................ 00100040: 00000000 00000000 00000000 00000000  ................ 00100050: 00000000 00000000 00000000 00000000  ................ 00100060: 00000000 00000000 00000000 00000000  ................ 00100070: 00000000 00000000 00000000 00000000  ................ 00100080: 00000000 00000000 00000000 00000000  ................ 00100090: 00000000 00000000 00000000 00000000  ................ 001000a0: 00000000 00000000 00000000 00000000  ................ 001000b0: 00000000 00000000 00000000 00000000  ................ 001000c0: 00000000 00000000 00000000 00000000  ................ 001000d0: 00000000 00000000 00000000 00000000  ................ 001000e0: 00000000 00000000 00000000 00000000  ................ 001000f0: 00000000 00000000 00000000 00000000  ................ => fru capture 100000 => fru display *****COMMON HEADER***** Version:1 *** No Internal Area *** *** No Chassis Info Area *** Board Area Offset:8 *** No Product Info Area *** *** No MultiRecord Area *** *****BOARD INFO***** Version:1 Board Area Length:40 Time in Minutes from 0:00hrs 1/1/96: 0   Manufacturer Name: Xilinx   Product Name: test   Serial Number: 112545   Part Number: 12551   File ID: AA *****PRODUCT INFO***** Version:0 Product Area Length:0 *****MULTIRECORDS***** =>

I changed the previous hardcoded File ID setting as configurable so the last argument 'AA' is applied to the File ID field.

I agree with your comment on the 4th patch in this series that we need to keep the old command for a while before deprecating the command to support all previous user custom scripts. I'll add the 'fru board_gen' command back in the next version.

...

...

diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c index 9b4aded466ab..e979f0176273 100644 --- a/board/xilinx/common/board.c +++ b/board/xilinx/common/board.c @@ -88,6 +88,9 @@ int zynq_board_read_rom_ethaddr(unsigned char *ethaddr)   #define EEPROM_HDR_NO_OF_MAC_ADDR    4   #define EEPROM_HDR_ETH_ALEN        ETH_ALEN   #define EEPROM_HDR_UUID_LEN        16 +#define EEPROM_MULTIREC_TYPE_XILINX_OEM    0xD2 +#define EEPROM_MULTIREC_MAC_OFFSET    4 +#define EEPROM_MULTIREC_DUT_MACID    0x31   struct xilinx_board_description {       u32 header; @@ -116,6 +119,19 @@ struct xilinx_legacy_format {       char unused3[29]; /* 0xe3 */   }; +struct xilinx_multirec_mac { +    u8 xlnx_iana_id[3]; +    u8 ver; +    u8 macid[EEPROM_HDR_NO_OF_MAC_ADDR][ETH_ALEN]; +};

+enum xilinx_board_custom_field { +    brd_custom_field_rev = 0, +    brd_custom_field_pcie, +    brd_custom_field_uuid, +    brd_custom_field_max +};

static void xilinx_eeprom_legacy_cleanup(char *eeprom, int size)   {       int i; @@ -200,9 +216,14 @@ static bool xilinx_detect_legacy(u8 *buffer)   static int xilinx_read_eeprom_fru(struct udevice *dev, char *name,                     struct xilinx_board_description *desc)   { +    u8 parsed_macid[EEPROM_HDR_NO_OF_MAC_ADDR][ETH_ALEN]= { 0 }; +    struct fru_custom_info custom_info[brd_custom_field_max] = { 0 }; +    struct fru_custom_field_node *ci_node; +    struct fru_multirec_node *mr_node; +    const struct fru_table *fru_data;       int i, ret, eeprom_size;       u8 *fru_content; -    u8 id = 0; +    u8 id = 0, field = 0;       /* FIXME this is shortcut - if eeprom type is wrong it will fail */       eeprom_size = i2c_eeprom_size(dev); @@ -237,30 +258,56 @@ static int xilinx_read_eeprom_fru(struct udevice *dev, char *name,           goto end;       } +    fru_data = fru_get_fru_data();

+    list_for_each_entry(ci_node, &fru_data->brd.custom_fields, list) { +        custom_info[field].type_len = ci_node->info.type_len; +        memcpy(custom_info[field].data, ci_node->info.data, +               ci_node->info.type_len & FRU_TYPELEN_LEN_MASK); +        if (++field < brd_custom_field_max) +            break; +    }

+    list_for_each_entry(mr_node, &fru_data->multi_recs, list) { +        struct fru_multirec_hdr *hdr= &mr_node->info.hdr;

+        if (hdr->rec_type == EEPROM_MULTIREC_TYPE_XILINX_OEM) { +            struct xilinx_multirec_mac *mac;

+            mac = (struct xilinx_multirec_mac *)mr_node->info.data; +            if (mac->ver == EEPROM_MULTIREC_DUT_MACID) { +                int mac_len = hdr->len - +                          EEPROM_MULTIREC_MAC_OFFSET;

+                memcpy(parsed_macid, mac->macid, mac_len); +            } +        } +    }

/* It is clear that FRU was captured and structures were filled */ -    strlcpy(desc->manufacturer, (char *)fru_data.brd.manufacturer_name, +    strlcpy(desc->manufacturer, (char *)fru_data->brd.manufacturer_name,           sizeof(desc->manufacturer)); -    strlcpy(desc->uuid, (char *)fru_data.brd.uuid, +    strlcpy(desc->uuid, (char *)custom_info[brd_custom_field_uuid].data,           sizeof(desc->uuid)); -    strlcpy(desc->name, (char *)fru_data.brd.product_name, +    strlcpy(desc->name, (char *)fru_data->brd.product_name,           sizeof(desc->name));       for (i = 0; i < sizeof(desc->name); i++) {           if (desc->name[i]== ' ')               desc->name[i] = '\0';       } -    strlcpy(desc->revision, (char *)fru_data.brd.rev, +    strlcpy(desc->revision, (char *)custom_info[brd_custom_field_rev].data,           sizeof(desc->revision));       for (i = 0; i < sizeof(desc->revision);i++) {           if (desc->revision[i] == ' ')               desc->revision[i] = '\0';       } -    strlcpy(desc->serial, (char *)fru_data.brd.serial_number, +    strlcpy(desc->serial, (char *)fru_data->brd.serial_number,           sizeof(desc->serial));       while (id < EEPROM_HDR_NO_OF_MAC_ADDR) { -        if (is_valid_ethaddr((const u8 *)fru_data.mac.macid[id])) +        if (is_valid_ethaddr((const u8 *)parsed_macid[id]))               memcpy(&desc->mac_addr[id], -                   (char *)fru_data.mac.macid[id], ETH_ALEN); +                   (char *)parsed_macid[id], ETH_ALEN);           id++;       } diff --git a/board/xilinx/common/fru.c b/board/xilinx/common/fru.c index f6ca46c3cecc..0d72911df04d 100644 --- a/board/xilinx/common/fru.c +++ b/board/xilinx/common/fru.c @@ -1,6 +1,7 @@   // SPDX-License-Identifier: GPL-2.0   /*    * (C) Copyright 2019 - 2020 Xilinx, Inc.

• • Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights

reserved.    */   #include <common.h> @@ -43,7 +44,7 @@ static int do_fru_generate(struct cmd_tbl *cmdtp, int flag, int argc,       addr = hextoul(argv[2], NULL); -    return fru_generate(addr, argv[3], argv[4], argv[5],argv[6], argv[7]); +    return fru_generate(addr, argc - 3, argv + 3);   }   static struct cmd_tbl cmd_fru_sub[] = { @@ -78,14 +79,15 @@ static char fru_help_text[] =       "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> <revision> - Generate FRU format with\n" -    "              board info area filled based on parameters. <addr> is\n" -    "              pointing to place where FRU is generated.\n" +    "              <part number> <file id> [custom ...] - Generate FRU\n"

That command was created for generating fragment with board revision. Here you are changing it to file id without any reason why.

Intention of this series is to move the 'fru' command into common place so I removed the Xilinx dependent custom field, 'revision'. The revision field can be added as one of generic custom info fields flexibly using the new command. I also modified 'board/xilinx/common/board.c' file to make it parse the custom revision field in a generic way so please check the code.

I get it what you do. I am just saying that it is so many things in the single patch to review and focus. It means one thing is to extract that vendor specific bits. And second to extend it to make that generation generic for other fields.

Again no problem with doing it. I just have a problem that it is in all in one patch. If there is any problem finds in future and you will bisect it you will end up in this patch and you will have hard time to find which exact change does it.

Okay. I'll split it to a couple of patches in the next version.

...

...

...

+    "              format with board info area filled based on\n" +    "                parameters. <addr> is pointing to place where FRU is\n" +    "                generated.\n"       ;   #endif   U_BOOT_CMD( -    fru, 8, 1, do_fru, +    fru, CONFIG_SYS_MAXARGS, 1, do_fru,

What's the intention to change this? We have this macro setup to 64. Do you want to support to write all fields?

As I described above, the new command can add multiple custom fields flexibly by taking multiple arguments so it's the reason of this change.

fru generate -b <addr> <manufacturer> <board name> <serial number>                  <part number> <file id> [custom ...] - Generate FRU                  format with board info area filled based on                  parameters. <addr> is pointing to place where FRU is                  generated.

...

...

"FRU table info",       fru_help_text   ) diff --git a/board/xilinx/common/fru.h b/board/xilinx/common/fru.h index 59f6b722cf12..41655864dbf5 100644 --- a/board/xilinx/common/fru.h +++ b/board/xilinx/common/fru.h @@ -2,11 +2,13 @@   /*    * (C) Copyright 2019 Xilinx, Inc.    * Siva Durga Prasad Paladugu siva.durga.paladugu@xilinx.com

• • Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights

reserved.    */   #ifndef __FRU_H   #define __FRU_H -#include <net.h>

+#include <linux/list.h>   struct fru_common_hdr {       u8 version; @@ -17,21 +19,31 @@ struct fru_common_hdr {       u8 off_multirec;       u8 pad;       u8 crc; -}; +} __packed; -#define FRU_BOARD_MAX_LEN    32 -#define FRU_MAX_NO_OF_MAC_ADDR    4 +#define FRU_INFO_FIELD_LEN_MAX        32

This is pretty much just macro name change in a lot of places. It should be done in separate patch.

FRU_MAX_NO_OF_MAC_ADDR is Xilinx dependent customization so I moved it to 'board/xilinx/common/board.c'.

no problem - was commenting that s/FRU_BOARD_MAX_LEN/FRU_INFO_FIELD_LEN_MAX/

...

FRU_BOARD_MAX_LEN means maximum length of info field in FRU so it should be used for the product info so I changed it to FRU_INFO_FIELD_LEN_MAX which is more generic macro name. This change is also a part of this patch.

All good expect for last sentence. It shouldn't be the part of this patch. It should be done in separate patch and you have also commit message written in this paragraph. And it is easy to review and understand in single patch.

I'll split the macro renaming part as a separate patch.

...

...

...

+#define FRU_MULTIREC_DATA_LEN_MAX    255 -struct __packed fru_board_info_header { +struct fru_board_info_header {       u8 ver;       u8 len;       u8 lang_code;       u8 time[3]; -}; +} __packed; -struct __packed fru_board_info_member { +struct fru_board_info_member {       u8 type_len;       u8 *name; +} __packed;

Also pretty much self contained change without proper description.

I just moved __packed to the end of struct definition so it doesn't make any functional change but it improves browsability of this code since it's easier to search 'struct fru_board_info_header' instead of 'struct __packed fru_board_info_header'.

As above. Separate patch and you have commit message here too.

Will do it too.

...

...

...

+struct fru_custom_info { +    u8 type_len; +    u8 data[FRU_INFO_FIELD_LEN_MAX]; +} __packed;

+struct fru_custom_field_node { +    struct list_head list; +    struct fru_custom_info info;   };   struct fru_board_data { @@ -40,22 +52,16 @@ struct fru_board_data {       u8 lang_code;       u8 time[3];       u8 manufacturer_type_len; -    u8 manufacturer_name[FRU_BOARD_MAX_LEN]; +    u8 manufacturer_name[FRU_INFO_FIELD_LEN_MAX];       u8 product_name_type_len; -    u8 product_name[FRU_BOARD_MAX_LEN]; +    u8 product_name[FRU_INFO_FIELD_LEN_MAX];       u8 serial_number_type_len; -    u8 serial_number[FRU_BOARD_MAX_LEN]; +    u8 serial_number[FRU_INFO_FIELD_LEN_MAX];       u8 part_number_type_len; -    u8 part_number[FRU_BOARD_MAX_LEN]; +    u8 part_number[FRU_INFO_FIELD_LEN_MAX];       u8 file_id_type_len; -    u8 file_id[FRU_BOARD_MAX_LEN]; -    /* Xilinx custom fields */ -    u8 rev_type_len; -    u8 rev[FRU_BOARD_MAX_LEN]; -    u8 pcie_type_len; -    u8 pcie[FRU_BOARD_MAX_LEN]; -    u8 uuid_type_len; -    u8 uuid[FRU_BOARD_MAX_LEN]; +    u8 file_id[FRU_INFO_FIELD_LEN_MAX]; +    struct list_head custom_fields;   };   struct fru_multirec_hdr { @@ -64,32 +70,35 @@ struct fru_multirec_hdr {       u8 len;       u8 csum;       u8 hdr_csum; -}; +} __packed; -struct fru_multirec_mac { -    u8 xlnx_iana_id[3]; -    u8 ver; -    u8 macid[FRU_MAX_NO_OF_MAC_ADDR][ETH_ALEN]; +struct fru_multirec_info { +    struct fru_multirec_hdr hdr; +    u8 data[FRU_MULTIREC_DATA_LEN_MAX]; +} __packed;

+struct fru_multirec_node { +    struct list_head list; +    struct fru_multirec_info info;   };   struct fru_table {       struct fru_common_hdr hdr;       struct fru_board_data brd; -    struct fru_multirec_mac mac; +    struct list_head multi_recs;       bool captured;   }; -#define FRU_TYPELEN_CODE_MASK    0xC0 -#define FRU_TYPELEN_LEN_MASK    0x3F +#define FRU_TYPELEN_CODE_MASK       0xC0 +#define FRU_TYPELEN_LEN_MASK        0x3F   #define FRU_COMMON_HDR_VER_MASK        0xF   #define FRU_COMMON_HDR_LEN_MULTIPLIER    8   #define FRU_LANG_CODE_ENGLISH        0   #define FRU_LANG_CODE_ENGLISH_1        25   #define FRU_TYPELEN_EOF            0xC1 -#define FRU_MULTIREC_TYPE_OEM       0xD2 -#define FRU_MULTIREC_MAC_OFFSET        4   #define FRU_LAST_REC            BIT(7) -#define FRU_DUT_MACID            0x31 +#define FRU_MULTIREC_TYPE_OEM_START    0xC0 +#define FRU_MULTIREC_TYPE_OEM_END    0xFF   /* This should be minimum of fields */   #define FRU_BOARD_AREA_TOTAL_FIELDS    5 @@ -99,10 +108,9 @@ struct fru_table {   int fru_display(int verbose);   int fru_capture(unsigned long addr); -int fru_generate(unsigned long addr, char *manufacturer, char *board_name, -         char *serial_no, char *part_no, char *revision); +int fru_generate(unsigned long addr, int argc, char *const argv[]);   u8 fru_checksum(u8 *addr, u8 len);

-extern struct fru_table fru_data; +int fru_check_type_len(u8 type_len, u8 language, u8 *type);

This is used just inside fru_ops.c that means that change in this patch is unrelated to change you need to do here. Please move it to patch which really needs this.

Oh, you are right. I'll remove this prototype definition from this header and will make the function as a static in fru_ops.c.

...

...

+const struct fru_table *fru_get_fru_data(void);   #endif /* FRU_H */ diff --git a/board/xilinx/common/fru_ops.c b/board/xilinx/common/fru_ops.c index 49846ae3d660..8291e347716b 100644 --- a/board/xilinx/common/fru_ops.c +++ b/board/xilinx/common/fru_ops.c @@ -1,21 +1,24 @@   // SPDX-License-Identifier: GPL-2.0   /*    * (C) Copyright 2019 - 2020 Xilinx, Inc.

• • Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights

reserved.    */   #include <common.h> -#include <cpu_func.h>   #include <env.h>   #include <fdtdec.h> +#include <hexdump.h>   #include <log.h>   #include <malloc.h> -#include <net.h>   #include <asm/io.h> -#include <asm/arch/hardware.h> +#include <linux/compat.h>   #include "fru.h" -struct fru_table fru_data __section(".data"); +struct fru_table fru_data __section(".data") = { +    .brd.custom_fields = LIST_HEAD_INIT(fru_data.brd.custom_fields), +    .multi_recs = LIST_HEAD_INIT(fru_data.multi_recs), +};   static u16 fru_cal_area_len(u8 len)   { @@ -57,7 +60,7 @@ u8 fru_checksum(u8 *addr, u8 len)       return checksum;   } -static int fru_check_type_len(u8 type_len, u8 language, u8 *type) +int fru_check_type_len(u8 type_len, u8 language, u8 *type)   {       int len; @@ -89,8 +92,7 @@ static u8 fru_gen_type_len(u8 *addr, char *name)       return 1 + len;   } -int fru_generate(unsigned long addr, char *manufacturer, char *board_name, -         char *serial_no, char *part_no, char *revision) +int fru_generate(unsigned long addr, int argc, char *const argv[])

This is again the self contained change where you want to support generation for more fields in the patch which is doing a lot of other things.

I am fine with changing it but in steps. It means improve fru generation. Renaming macros, extraction OEM fields from board are, change multi record handling.

Okay. I'll split this patch as you suggested.

...

...

{       struct fru_common_hdr *header = (structfru_common_hdr *)addr;       struct fru_board_info_header *board_info; @@ -126,18 +128,10 @@ int fru_generate(unsigned long addr, char *manufacturer, char *board_name,       /* Member fields are just after board_info header */       member = (u8 *)board_info + sizeof(*board_info); -    len = fru_gen_type_len(member, manufacturer); /* Board Manufacturer */ -    member += len; -    len = fru_gen_type_len(member, board_name); /* Board Product name */ -    member += len; -    len = fru_gen_type_len(member, serial_no); /* Board Serial number */ -    member += len; -    len = fru_gen_type_len(member, part_no); /* Board part number */ -    member += len; -    len = fru_gen_type_len(member, "U-Boot generator"); /* File ID */ -    member += len; -    len = fru_gen_type_len(member, revision); /* Revision */ -    member += len; +    while (--argc > 0 && ++argv) { +        len = fru_gen_type_len(member, *argv); +        member += len; +    }       *member++ = 0xc1; /* Indication of no more fields */ @@ -168,6 +162,35 @@ int fru_generate(unsigned long addr, char *manufacturer, char *board_name,       return 0;   } +static void fru_delete_custom_fields(struct list_head *custom_fields) +{ +    struct fru_custom_field_node *node, *next;

+    list_for_each_entry_safe(node, next, custom_fields, list) { +        list_del(&node->list); +        kfree(node); +    } +}

+static int fru_append_custom_info(unsigned long addr, +                  struct list_head *custom_fields) +{ +    struct fru_custom_info *info = (struct fru_custom_info *)addr; +    struct fru_custom_field_node *ci_new;

+    ci_new = kzalloc(sizeof(*ci_new), GFP_KERNEL); +    if (!ci_new) +        return -ENOMEM;

+    ci_new->info.type_len = info->type_len; +    memcpy(&ci_new->info.data, (void *)info->data, +           ci_new->info.type_len & FRU_TYPELEN_LEN_MASK);

+    list_add_tail(&ci_new->list, custom_fields);

+    return 0; +}

static int fru_parse_board(unsigned long addr)   {       u8 i, type; @@ -179,9 +202,10 @@ static int fru_parse_board(unsigned long addr)       data = (u8 *)&fru_data.brd.manufacturer_type_len;       /* Record max structure limit not to write data over allocated space */ -    limit = (u8 *)&fru_data.brd + sizeof(struct fru_board_data); +    limit = (u8 *)&fru_data.brd + sizeof(struct fru_board_data) - +        sizeof(struct fru_custom_field_node *); -    for (i = 0; ; i++, data += FRU_BOARD_MAX_LEN) { +    for (i = 0; ; i++, data += FRU_INFO_FIELD_LEN_MAX) {           len = fru_check_type_len(*(u8 *)addr, fru_data.brd.lang_code,                        &type);           /* @@ -190,11 +214,14 @@ static int fru_parse_board(unsigned long addr)           if (len == -EINVAL)               break; -        /* Stop when amount of charsis more then fields to record */ -        if (data + len > limit) -            break; -        /* This record type/len field */ -        *data++ = *(u8 *)addr; +        if (i < FRU_BOARD_AREA_TOTAL_FIELDS) { +            /* Stop when length is more then fields to record */ +            if (data + len > limit) +                break;

+            /* This record type/len field */ +            *data++ = *(u8 *)addr; +        }           /* Add offset to match data */           addr += 1; @@ -203,10 +230,23 @@ static int fru_parse_board(unsigned long addr)           if (!len)               continue; -        /* Record data field */ -        memcpy(data, (u8 *)addr, len); -        term = data + (u8)len; -        *term = 0; +        if (i < FRU_BOARD_AREA_TOTAL_FIELDS) { +            /* Record data field */ +            memcpy(data, (u8 *)addr, len); +            term= data + (u8)len; +            *term = 0; +        } else { +            struct list_head *custom_fields;

+            custom_fields = &fru_data.brd.custom_fields;

+            /* Add a custom info field */ +            if (fru_append_custom_info(addr - 1, custom_fields)) { +                fru_delete_custom_fields(custom_fields); +                return -EINVAL; +            } +        }

addr += len;       } @@ -219,34 +259,52 @@ static int fru_parse_board(unsigned long addr)       return 0;   } +static void fru_delete_multirecs(struct list_head *multi_recs) +{ +    struct fru_multirec_node *node, *next;

+    list_for_each_entry_safe(node, next, multi_recs, list) { +        list_del(&node->list); +        kfree(node); +    } +}

+static int fru_append_multirec(unsigned long addr, +                   struct list_head *multi_recs) +{ +    struct fru_multirec_info *mr_src = (struct fru_multirec_info *)addr; +    struct fru_multirec_node *mr_new;

+    mr_new = kzalloc(sizeof(*mr_new), GFP_KERNEL); +    if (!mr_new) +        return -ENOMEM;

+    memcpy(&mr_new->info.hdr, (void *)&mr_src->hdr, sizeof(mr_src->hdr)); +    memcpy(&mr_new->info.data, (void *)mr_src->data, mr_src->hdr.len);

+    list_add_tail(&mr_new->list, multi_recs);

+    return 0; +}

static int fru_parse_multirec(unsigned long addr)   { -    struct fru_multirec_hdr mrc; -    u8 checksum = 0;       u8 hdr_len = sizeof(struct fru_multirec_hdr); -    int mac_len = 0; +    struct fru_multirec_hdr *mr_hdr; -    debug("%s: multirec addr %lx\n", __func__, addr); +    debug("%s: multirec addr %lx\n", __func__, (ulong)addr);       do { -        memcpy(&mrc.rec_type, (void *)addr, hdr_len);

-        checksum = fru_checksum((u8 *)addr, hdr_len); -        if (checksum) { +        if (fru_checksum((u8 *)addr,hdr_len)) {               debug("%s header CRC error\n", __func__);               return -EINVAL;           } -        if (mrc.rec_type == FRU_MULTIREC_TYPE_OEM) { -            struct fru_multirec_mac *mac = (void *)addr + hdr_len; +        fru_append_multirec(addr, &fru_data.multi_recs); -            if (mac->ver == FRU_DUT_MACID) { -                mac_len = mrc.len - FRU_MULTIREC_MAC_OFFSET; -                memcpy(&fru_data.mac.macid, mac->macid, mac_len); -            } -        } -        addr += mrc.len + hdr_len; -    } while (!(mrc.type & FRU_LAST_REC)); +        mr_hdr = (struct fru_multirec_hdr *)addr; +        addr += mr_hdr->len + hdr_len; +    } while (!(mr_hdr->type & FRU_LAST_REC));       return 0;   } @@ -255,7 +313,6 @@ int fru_capture(unsigned long addr)   {       struct fru_common_hdr *hdr;       u8 checksum = 0; -    unsigned long multirec_addr = addr;       checksum = fru_checksum((u8 *)addr, sizeof(struct fru_common_hdr));       if (checksum) { @@ -264,33 +321,28 @@ int fru_capture(unsigned long addr)       }       hdr = (struct fru_common_hdr *)addr; +    fru_delete_custom_fields(&fru_data.brd.custom_fields); +    fru_delete_multirecs(&fru_data.multi_recs);       memset((void *)&fru_data, 0, sizeof(fru_data)); -    memcpy((void *)&fru_data, (void *)hdr, -           sizeof(struct fru_common_hdr)); +    INIT_LIST_HEAD(&fru_data.brd.custom_fields); +    INIT_LIST_HEAD(&fru_data.multi_recs); +    memcpy((void *)&fru_data, (void *)hdr, sizeof(struct fru_common_hdr));       fru_data.captured = true; -    if (hdr->off_board) { -        addr += fru_cal_area_len(hdr->off_board); -        fru_parse_board(addr); -    } +    if (hdr->off_board) +        fru_parse_board(addr + fru_cal_area_len(hdr->off_board)); -    env_set_hex("fru_addr", addr); +    if (hdr->off_multirec) +        fru_parse_multirec(addr + fru_cal_area_len(hdr->off_multirec)); -    if (hdr->off_multirec) { -        multirec_addr += fru_cal_area_len(hdr->off_multirec); -        fru_parse_multirec(multirec_addr); -    } +    env_set_hex("fru_addr", (ulong)addr);       return 0;   }   static int fru_display_board(struct fru_board_data *brd, int verbose)   { -    u32 time = 0; -    u8 type; -    int len; -    u8 *data;       static const char * const typecode[] = {           "Binary/Unspecified",           "BCD plus", @@ -301,12 +353,15 @@ static int fru_display_board(struct fru_board_data *brd, int verbose)       static const char * const boardinfo[] ={           "Manufacturer Name",           "Product Name", -        "Serial No", +        "Serial Number",           "Part Number",           "File ID", -        /* Xilinx spec */ -        "Revision Number",       }; +    struct fru_custom_field_node *node; +    u32 time = 0; +    u8 *data; +    u8 type; +    int len;       if (verbose) {           printf("*****BOARD INFO*****\n"); @@ -358,7 +413,32 @@ static int fru_display_board(struct fru_board_data *brd, int verbose)               debug("Unsupported type %x\n", type);           } -        data += FRU_BOARD_MAX_LEN; +        data += FRU_INFO_FIELD_LEN_MAX; +    }

+    list_for_each_entry(node, &brd->custom_fields, list){ +        printf(" Custom Type/Length:0x%x\n", node->info.type_len); +        print_hex_dump_bytes(" ", DUMP_PREFIX_OFFSET, node->info.data, +                     node->info.type_len & +                     FRU_TYPELEN_LEN_MASK);

this is pretty much debug information. Bootlog contains a lot of data and won't look good. I think this should be moved to verbose or debug.

File ID: 0x0   Custom Type/Length: 0xc8    00000000: 41 00 00 00 00 00 00 00                          A.......   Custom Type/Length: 0x8    00000000: 10 ee 00 00 00 00 00 00                          ........   Custom Type/Length: 0x10    00000000: 52 64 da 0f 73 44 46 03 ac 1b 1e f6 35 92 3c f1 Rd..sDF.....5.<. Type ID: 0x2, Type: 0x2 Length: 13   00000000: 01 f4 01 c2 01 26 02 64 00 00 00 a0 0f .....&.d..... Type ID: 0xd2, Type: 0x2 Length: 10   00000000: da 10 00 31 00 0a 35 10 39 cd                    ...1..5.9. Type ID: 0xd3, Type: 0x82 Length: 87   00000000: da 10 00 4d 65 6d 6f 72 79 3a 20 51 53 50 49 3a ...Memory: QSPI:   00000010: 35 31 32 4d 62 20 20 00 4d 65 6d 6f 72 79 3a 20  512Mb .Memory:   00000020: 65 4d 4d 43 3a 4e 6f 6e 65 20 20 20 00 4d 65 6d eMMC:None .Mem   00000030: 6f 72 79 3a 20 50 53 44 44 52 34 3a 34 47 42 20  ory: PSDDR4:4GB   00000040: 20 00 4d 65 6d 6f 72 79 3a 20 50 4c 44 44 52 34 .Memory: PLDDR4   00000050: 3a 4e 6f 6e 65 20 00                             :None .

No, it's not a debug printing but a printing of custom field data in hex dump because the field can be any types other than string type.

...

...

...

+    }

+    return 0; +}

+static int fru_display_multirec(struct list_head *multi_recs, int verbose) +{ +    struct fru_multirec_node *node;

+    if (verbose) +        printf("*****MULTIRECORDS*****\n");

+    list_for_each_entry(node, multi_recs, list) { +        printf("Type ID: 0x%x, Type:0x%x Length: %d\n", +               node->info.hdr.rec_type, node->info.hdr.type, +               node->info.hdr.len); +        print_hex_dump_bytes(" ", DUMP_PREFIX_OFFSET, node->info.data, +                     node->info.hdr.len);

And here again the same.

No, it's also an intentional printing out in hex dump format to support it in generic way. For an example, if we add a MAX address as a multi record, it should be printed out in hex dump instead of a string format.

But keep it under verbose.

We are using this code for dtb reselection at boot and don't really want to see this is all the time when I boot our SOC. Take a look how it looks like now when your patch is applied. As I said use verbose parameter and I think we both will be fine with it.

U-Boot SPL 2022.10-rc5-00262-gd471037d6f89 (Sep 21 2022 - 15:11:03 +0200) PMUFW:  v1.1 Loading new PMUFW cfg obj (32 bytes) PMUFW no permission to change config object Loading new PMUFW cfg obj (2032 bytes) Silicon version:        3 EL Level:       EL3 Secure Boot:    not authenticated, not encrypted Multiboot:      64 Trying to boot from SPI NOTICE:  BL31: v2.7(release):v2.7.0-415-g8edd190e6416 NOTICE:  BL31: Built : 10:45:09, Sep 16 2022

U-Boot 2022.10-rc5-00256-gc829283b8093 (Sep 21 2022 - 15:18:37 +0200)

CPU:   ZynqMP Silicon: v3 Chip:  xck26 Detected name: zynqmp-smk-k26-xcl2g-revA-sck-kv-g-revB Model: ZynqMP SMK-K26 Rev1/B/A Board: Xilinx ZynqMP DRAM:  4 GiB PMUFW:  v1.1 Cannot load PMUFW configuration object (1) PMUFW returned 0x00000001 status! Xilinx I2C FRU format at nvmem0:  Manufacturer Name: XILINX  Product Name: SMK-K26-XCL2G  Serial Number: 50571A21CT4G  Part Number: 5057-04  File ID: 0x0  Custom Type/Length: 0xc8   00000000: 41 00 00 00 00 00 00 00                          A.......  Custom Type/Length: 0x8   00000000: 10 ee 00 00 00 00 00 00                          ........  Custom Type/Length: 0x10   00000000: 52 64 da 0f 73 44 46 03 ac 1b 1e f6 35 92 3c f1 Rd..sDF.....5.<. Type ID: 0x2, Type: 0x2 Length: 13  00000000: 01 f4 01 c2 01 26 02 64 00 00 00 a0 0f           .....&.d..... Type ID: 0xd2, Type: 0x2 Length: 10  00000000: da 10 00 31 00 0a 35 10 39 cd                    ...1..5.9. Type ID: 0xd3, Type: 0x82 Length: 87  00000000: da 10 00 4d 65 6d 6f 72 79 3a 20 51 53 50 49 3a  ...Memory: QSPI:  00000010: 35 31 32 4d 62 20 20 00 4d 65 6d 6f 72 79 3a 20  512Mb .Memory:  00000020: 65 4d 4d 43 3a 4e 6f 6e 65 20 20 20 00 4d 65 6d  eMMC:None .Mem  00000030: 6f 72 79 3a 20 50 53 44 44 52 34 3a 34 47 42 20  ory: PSDDR4:4GB  00000040: 20 00 4d 65 6d 6f 72 79 3a 20 50 4c 44 44 52 34   .Memory: PLDDR4  00000050: 3a 4e 6f 6e 65 20 00                             :None . Xilinx I2C FRU format at nvmem1:  Manufacturer Name: XILINX  Product Name: SCK-KV-G  Serial Number: 50582B112M07  Part Number: 5058-02  File ID: 0x0  Custom Type/Length: 0xc8   00000000: 42 00 00 00 00 00 00 00                          B.......  Custom Type/Length: 0x8   00000000: 10 ee 00 00 00 00 00 00                          ........  Custom Type/Length: 0x10   00000000: 7f f2 1d 3e 25 3f 4f 72 9d cb 48 8b e1 3c 6a 52 ...>%?Or..H..<jR Type ID: 0x2, Type: 0x2 Length: 13  00000000: 01 b0 04 7e 04 e2 04 64 00 00 00 b8 0b           ...~...d..... EL Level:       EL2 Secure Boot:    not authenticated, not encrypted Core:  124 devices, 33 uclasses, devicetree: fit NAND:  0 MiB MMC:   mmc@ff170000: 1 Loading Environment from nowhere... OK In:    serial Out:   serial Err:   serial Bootmode: QSPI_MODE Reset reason:   SOFT Net:   PHY reset timed out

ZYNQ GEM: ff0e0000, mdio bus ff0e0000, phyaddr 1, interface rgmii-id eth0: ethernet@ff0e0000

Okay. I'll move this printing under verbose.

...

...

...

}       return 0; @@ -404,6 +484,8 @@ static void fru_display_common_hdr(struct fru_common_hdr *hdr, int verbose)   int fru_display(int verbose)   { +    int ret;

if (!fru_data.captured) {           printf("FRU data not available please run fru parse\n");           return -EINVAL; @@ -411,5 +493,14 @@ int fru_display(int verbose)       fru_display_common_hdr(&fru_data.hdr, verbose); -    return fru_display_board(&fru_data.brd, verbose); +    ret = fru_display_board(&fru_data.brd, verbose); +    if (ret) +        return ret;

+    return fru_display_multirec(&fru_data.multi_recs, verbose); +}

+const struct fru_table *fru_get_fru_data(void) +{ +    return &fru_data;   }

Definitely patch is going in the right direction to remove Xilinx custom parts but I would like to see this to be split to couple of patches. I don't think this is going to be problem and it will be much easier to review.

Okay. I'll split out this patch to couple of patches in the next version.

Good.

Thanks for your review. I'll submit a new version soon.

Regards, Jae

Hello Michal,

On 9/21/2022 6:07 AM, Michal Simek wrote:

On 8/25/22 18:42, Jae Hyun Yoo wrote:

...

This command doesn't work with sandbox because direct memory access causes a segfault error. Fix it up using map_sysmem().

Signed-off-by: Jae Hyun Yoo quic_jaehyoo@quicinc.com Reviewed-by: Simon Glass sjg@chromium.org

---

Changes from v3:   * None.

Changes from v2:   * Added a 'Reviewed-by' tag. (Simon)

Changes from v1:   * Newly added in v2.

board/xilinx/common/board.c |  2 +-   cmd/fru.c                   | 19 ++++++++++++++++---   include/fru.h               |  4 ++--   lib/fru_ops.c               | 17 ++++++++---------   4 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c index e58b11d7f757..3292083c5250 100644 --- a/board/xilinx/common/board.c +++ b/board/xilinx/common/board.c @@ -241,7 +241,7 @@ static int xilinx_read_eeprom_fru(struct udevice *dev, char *name,           goto end;       } -    fru_capture((unsigned long)fru_content); +    fru_capture(fru_content);       if (gd->flags & GD_FLG_RELOC || (_DEBUG && CONFIG_IS_ENABLED(DTB_RESELECT))) {           printf("Xilinx I2C FRU format at %s:\n", name);           ret = fru_display(0); diff --git a/cmd/fru.c b/cmd/fru.c index 2ec5012af5ac..b2cadbec9780 100644 --- a/cmd/fru.c +++ b/cmd/fru.c @@ -9,12 +9,15 @@   #include <fdtdec.h>   #include <fru.h>   #include <malloc.h> +#include <mapmem.h>   static int do_fru_capture(struct cmd_tbl *cmdtp, int flag, int argc,                 char *const argv[])   {       unsigned long addr; +    const void *buf;       char *endp; +    int ret;       if (argc < cmdtp->maxargs)           return CMD_RET_USAGE; @@ -23,7 +26,11 @@ static int do_fru_capture(struct cmd_tbl *cmdtp, int flag, int argc,       if (*argv[1] == 0 || *endp != 0)           return -1; -    return fru_capture(addr); +    buf = map_sysmem(addr, 0); +    ret = fru_capture(buf); +    unmap_sysmem(buf);

+    return ret;   }   static int do_fru_display(struct cmd_tbl *cmdtp, int flag, int argc, @@ -37,13 +44,19 @@ static int do_fru_generate(struct cmd_tbl *cmdtp, int flag, int argc,                  char *const argv[])   {       unsigned long addr; +    const void *buf; +    int ret;       if (argc < cmdtp->maxargs)           return CMD_RET_USAGE; -    addr = hextoul(argv[2], NULL); +    addr = hextoul(argv[3], NULL);

What's the reason for changing arguments here? Wasn't origin version correct?

The 'board_gen' command is replaced with 'generate' command by this series and the 'generate' command has one more preceding argument to check '-b' and '-p' for board and product respectively. So actually this change is part of the 4th patch of this series. I'll move it to the patch in the next version.

Thanks for your pointing it out.

Jae

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@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.

" 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.

; #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.

• static u16 fru_cal_area_len(u8 len) { return len * FRU_COMMON_HDR_LEN_MULTIPLIER;

@@ -77,9 +86,9 @@ int fru_check_type_len(u8 type_len, u8 language, u8 *type) static u8 fru_gen_type_len(u8 *addr, char *name) { int len = strlen(name);

• struct fru_board_info_member *member;

• struct fru_common_info_member *member;

• member = (struct fru_board_info_member *)addr;

• member = (struct fru_common_info_member *)addr; member->type_len = FRU_TYPELEN_TYPE_ASCII8 << FRU_TYPELEN_TYPE_SHIFT; member->type_len |= len;

@@ -91,7 +100,7 @@ static u8 fru_gen_type_len(u8 *addr, char *name) return 1 + len; }

-int fru_generate(const void *addr, int argc, char *const argv[]) +int fru_board_generate(const void *addr, int argc, char *const argv[]) { struct fru_common_hdr *header = (struct fru_common_hdr *)addr; struct fru_board_info_header *board_info; @@ -161,6 +170,74 @@ int fru_generate(const void *addr, int argc, char *const argv[]) return 0; }

+int fru_product_generate(const void *addr, int argc, char *const argv[]) +{

• struct fru_common_hdr *header = (struct fru_common_hdr *)addr;
• struct fru_product_info_header *product_info;
• u8 *member;
• u8 len, pad, modulo;
• header->version = 1; /* Only version 1.0 is supported now */
• header->off_internal = 0; /* not present */
• header->off_chassis = 0; /* not present */
• header->off_board = 0; /* not present */
• header->off_product = (sizeof(*header)) / 8; /* Starting offset 8 */
• header->off_multirec = 0; /* not present */
• header->pad = 0;
• /*

• * This unsigned byte can be used to calculate a zero checksum
  

• * for the data area following the header. I.e. the modulo 256 sum of
  

• * the record data bytes plus the checksum byte equals zero.
  

• */
  

• header->crc = 0; /* Clear before calculation */
• header->crc = 0 - fru_checksum((u8 *)header, sizeof(*header));
• /* board info is just right after header */
• product_info = (void *)((u8 *)header + sizeof(*header));
• debug("header %lx, product_info %lx\n", (ulong)header,

•      (ulong)product_info);
  

• product_info->ver = 1; /* 1.0 spec */
• product_info->lang_code = 0; /* English */
• /* Member fields are just after board_info header */
• member = (u8 *)product_info + sizeof(*product_info);
• while (--argc > 0 && ++argv) {

• len = fru_gen_type_len(member, *argv);
  

• member += len;
  

• }
• *member++ = 0xc1; /* Indication of no more fields */
• len = member - (u8 *)product_info; /* Find current length */
• len += 1; /* Add checksum there too for calculation */
• modulo = len % 8;
• if (modulo) {

• /* Do not fill last item which is checksum */
  

• for (pad = 0; pad < 8 - modulo; pad++)
  

• 	*member++ = 0;
  

• /* Increase structure size */
  

• len += 8 - modulo;
  

• }
• product_info->len = len / 8; /* Size in multiples of 8 bytes */
• *member = 0; /* Clear before calculation */
• *member = 0 - fru_checksum((u8 *)product_info, len);
• debug("checksum %x(addr %x)\n", *member, len);
• env_set_hex("fru_addr", (ulong)addr);
• env_set_hex("filesize", (ulong)member - (ulong)addr + 1);
• return 0;

+}

• static void fru_delete_custom_fields(struct list_head *custom_fields) { struct fru_custom_field_node *node, *next;

@@ -258,6 +335,74 @@ static int fru_parse_board(const void *addr) return 0; }

+static int fru_parse_product(const void *addr) +{

• u8 i, type;
• int len;
• u8 *data, *term, *limit;
• memcpy(&fru_data.prd.ver, (void *)addr, 6);
• addr += 3;
• data = (u8 *)&fru_data.prd.manufacturer_type_len;
• /* Record max structure limit not to write data over allocated space */
• limit = (u8 *)&fru_data.prd + sizeof(struct fru_product_data) -

• sizeof(struct fru_custom_field_node *);
  

• for (i = 0; ; i++, data += FRU_INFO_FIELD_LEN_MAX) {

• len = fru_check_type_len(*(u8 *)addr, fru_data.prd.lang_code,
  

• 			 &type);
  

• /*
  

•  * Stop cature if it end of fields
  

•  */
  

• if (len == -EINVAL)
  

• 	break;
  

• if (i < FRU_PRODUCT_AREA_TOTAL_FIELDS) {
  

• 	/* Stop when length is more then fields to record */
  

• 	if (data + len > limit)
  

• 		break;
  

• 	/* This record type/len field */
  

• 	*data++ = *(u8 *)addr;
  

• }
  

• /* Add offset to match data */
  

• addr += 1;
  

• /* If len is 0 it means empty field that's why skip writing */
  

• if (!len)
  

• 	continue;
  

• if (i < FRU_PRODUCT_AREA_TOTAL_FIELDS) {
  

• 	/* Record data field */
  

• 	memcpy(data, (u8 *)addr, len);
  

• 	term = data + (u8)len;
  

• 	*term = 0;
  

• } else {
  

• 	struct list_head *custom_fields;
  

• 	custom_fields = &fru_data.prd.custom_fields;
  

• 	/* Add a custom info field */
  

• 	if (fru_append_custom_info(addr - 1, custom_fields)) {
  

• 		fru_delete_custom_fields(custom_fields);
  

• 		return -EINVAL;
  

• 	}
  

• }
  

• addr += len;
  

• }
• if (i < FRU_PRODUCT_AREA_TOTAL_FIELDS) {

• printf("Product area require minimum %d fields\n",
  

•        FRU_PRODUCT_AREA_TOTAL_FIELDS);
  

• return -EINVAL;
  

• }
• return 0;

+}

• static void fru_delete_multirecs(struct list_head *multi_recs) { struct fru_multirec_node *node, *next;

@@ -320,9 +465,11 @@ int fru_capture(const void *addr)

hdr = (struct fru_common_hdr *)addr; fru_delete_custom_fields(&fru_data.brd.custom_fields);

• fru_delete_custom_fields(&fru_data.prd.custom_fields); fru_delete_multirecs(&fru_data.multi_recs); memset((void *)&fru_data, 0, sizeof(fru_data)); INIT_LIST_HEAD(&fru_data.brd.custom_fields);
• INIT_LIST_HEAD(&fru_data.prd.custom_fields); INIT_LIST_HEAD(&fru_data.multi_recs); memcpy((void *)&fru_data, (void *)hdr, sizeof(struct fru_common_hdr));

@@ -331,6 +478,9 @@ int fru_capture(const void *addr) if (hdr->off_board) fru_parse_board(addr + fru_cal_area_len(hdr->off_board));

• if (hdr->off_product)

• fru_parse_product(addr + fru_cal_area_len(hdr->off_product));
  

• if (hdr->off_multirec) fru_parse_multirec(addr + fru_cal_area_len(hdr->off_multirec));

@@ -341,13 +491,6 @@ int fru_capture(const void *addr)

static int fru_display_board(struct fru_board_data *brd, int verbose) {

• static const char * const typecode[] = {

• "Binary/Unspecified",
  

• "BCD plus",
  

• "6-bit ASCII",
  

• "8-bit ASCII",
  

• "2-byte UNICODE"
  

• }; static const char * const boardinfo[] = { "Manufacturer Name", "Product Name",

@@ -389,9 +532,9 @@ static int fru_display_board(struct fru_board_data *brd, int verbose) if (type <= FRU_TYPELEN_TYPE_ASCII8 && (brd->lang_code == FRU_LANG_CODE_ENGLISH || brd->lang_code == FRU_LANG_CODE_ENGLISH_1))

• 	debug("Type code: %s\n", typecode[type]);
  

• 	debug("Type code: %s\n", fru_typecode_str[type]);
  

  else

• 	debug("Type code: %s\n", typecode[type + 1]);
  

• 	debug("Type code: %s\n", fru_typecode_str[type + 1]);
  
  

  if (!len) { debug("%s not found\n", boardinfo[i]);

@@ -424,6 +567,79 @@ static int fru_display_board(struct fru_board_data *brd, int verbose) return 0; }

+static int fru_display_product(struct fru_product_data *prd, int verbose) +{

• static const char * const productinfo[] = {

• "Manufacturer Name",
  

• "Product Name",
  

• "Part Number",
  

• "Version Number",
  

• "Serial Number",
  

• "Asset Number",
  

• "File ID",
  

• };
• struct fru_custom_field_node *node;
• u8 *data;
• u8 type;
• int len;
• if (verbose) {

• printf("*****PRODUCT INFO*****\n");
  

• printf("Version:%d\n", fru_version(prd->ver));
  

• printf("Product Area Length:%d\n", fru_cal_area_len(prd->len));
  

• }
• if (fru_check_language(prd->lang_code))

• return -EINVAL;
  

• data = (u8 *)&prd->manufacturer_type_len;
• for (u8 i = 0; i < (sizeof(productinfo) / sizeof(*productinfo)); i++) {

• len = fru_check_type_len(*data++, prd->lang_code,
  

• 			 &type);
  

• if (len == -EINVAL) {
  

• 	printf("**** EOF for Product Area ****\n");
  

• 	break;
  

• }
  

• if (type <= FRU_TYPELEN_TYPE_ASCII8 &&
  

•     (prd->lang_code == FRU_LANG_CODE_ENGLISH ||
  

•      prd->lang_code == FRU_LANG_CODE_ENGLISH_1))
  

• 	debug("Type code: %s\n", fru_typecode_str[type]);
  

• else
  

• 	debug("Type code: %s\n", fru_typecode_str[type + 1]);
  

• if (!len) {
  

• 	debug("%s not found\n", productinfo[i]);
  

• 	continue;
  

• }
  

• switch (type) {
  

• case FRU_TYPELEN_TYPE_BINARY:
  

• 	debug("Length: %d\n", len);
  

• 	printf(" %s: 0x%x\n", productinfo[i], *data);
  

• 	break;
  

• case FRU_TYPELEN_TYPE_ASCII8:
  

• 	debug("Length: %d\n", len);
  

• 	printf(" %s: %s\n", productinfo[i], data);
  

• 	break;
  

• default:
  

• 	debug("Unsupported type %x\n", type);
  

• }
  

• data += FRU_INFO_FIELD_LEN_MAX;
  

• }
• list_for_each_entry(node, &prd->custom_fields, list) {

• printf(" Custom Type/Length: 0x%x\n", node->info.type_len);
  

• print_hex_dump_bytes("  ", DUMP_PREFIX_OFFSET, node->info.data,
  

• 		     node->info.type_len &
  

• 		     FRU_TYPELEN_LEN_MASK);
  

• }
• return 0;

+}

• static int fru_display_multirec(struct list_head *multi_recs, int verbose) { struct fru_multirec_node *node;

@@ -495,6 +711,10 @@ int fru_display(int verbose) if (ret) return ret;

• ret = fru_display_product(&fru_data.prd, verbose);
• if (ret)

• return ret;
  

• return fru_display_multirec(&fru_data.multi_recs, verbose); }

M

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@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.

Thanks, Michal

Hello Michal,

On 9/21/2022 6:54 AM, Michal Simek wrote:

On 8/25/22 18:42, Jae Hyun Yoo wrote:

...

Add a usage document for the 'fru' u-boot command. Add kerneldocs for <fru.h>.

Signed-off-by: Jae Hyun Yoo quic_jaehyoo@quicinc.com

Changes from v3:   * None.

Changes from v2:   * Added kerneldocs to 'include/fru.h'. (Simon)

Changes from v1:   * Newly added in v2. (Heinrich)

doc/usage/cmd/fru.rst | 144 +++++++++++++++++++++++++++++++++   doc/usage/index.rst   |   1 +   include/fru.h         | 182 ++++++++++++++++++++++++++++++++++++++++++   3 files changed, 327 insertions(+)   create mode 100644 doc/usage/cmd/fru.rst

diff --git a/doc/usage/cmd/fru.rst b/doc/usage/cmd/fru.rst new file mode 100644 index 000000000000..d65bbc6dcbba --- /dev/null +++ b/doc/usage/cmd/fru.rst @@ -0,0 +1,144 @@ +.. SPDX-License-Identifier: GPL-2.0+

+fru command +===========

+Synopsis +--------

+::

+    fru capture <addr> +    fru display +    fru generate -b <addr> <manufacturer> <board name> <serial number> <part number> <file id> [<custom> ...] +    fru generate -p <addr> <manufacturer> <product name> <part number> <version number> <serial number> <asset number> <file id> [<custom> ...]

+Description +-----------

+The *fru* commands is used to generate, capture and display FRU (Field +Replaceable Unit) information data.

+Capture +~~~~~~~

+The *fru capture* command parses and captures FRU configuration table at +a specified address.

+    addr +        memory address which FRU configuration table is stored.

+Display +~~~~~~~

+The *fru display* command displays FRU information that is parsed using +fru capture command.

+Generate +~~~~~~~~

+The *fru generate* command generates a FRU configuration table which has Board +or Product Info Area using the given field parameters.

+    -b +        generate FRU which has board info area.

+        addr +            memory address which FRU configuration table will be stored.

+        manufacturer +            board manufacturer string.

+        board name +            board product name string.

+        serial number +            board serial number string.

+        serial number +            board serial number string.

+        part number +            board part number string.

+        file id +            FRU File ID string. The FRU File version field is a pre-defined +            field provided as a manufacturing aid for verifying the file that +            was used during manufacture or field update to load the FRU +            information. The content is manufacturer-specific.

+        custom +            additional custom board info area fields, if any.

+    -p +        generate FRU which has product info area.

+        addr +            memory address which FRU configuration table will be stored.

+        manufacturer +            product manufacturer string.

+        board name +            product name string.

+        part number +            product part/model number string.

+        version number +            product version number string.

+        serial number +            product serial number string.

+        asset number +            asset tag.

+        file id +            FRU File ID string. The FRU File version field is a pre-defined +            field provided as a manufacturing aid for verifying the file that +            was used during manufacture or field update to load the FRU +            information. The content is manufacturer-specific.

+        custom +            additional custom product info area fields, if any.

+Example +-------

+::

+    => fru generate -b 90000000 abc def ghi jkl mno prs tuv wxy +    => fru capture 90000000 +    => fru display +    *****COMMON HEADER***** +    Version:1 +    *** No Internal Area *** +    *** No Chassis Info Area *** +    Board Area Offset:8 +    *** No Product Info Area *** +    *** No MultiRecord Area *** +    *****BOARD INFO***** +    Version:1 +    Board Area Length:40 +    Time in Minutes from 0:00hrs 1/1/96: 0 +     Manufacturer Name: abc +     Product Name: def +     Serial Number: ghi +     Part Number: jkl +     File ID: mno +     Custom Type/Length: 0xc3 +      00000000: 70 72 73                                         prs +     Custom Type/Length: 0xc3 +      00000000: 74 75 76                                         tuv +     Custom Type/Length: 0xc3 +      00000000: 77 78 79                                         wxy +    *****PRODUCT INFO***** +    Version:0 +    Product Area Length:0 +    *****MULTIRECORDS*****

+Configuration +-------------

+The fru command is only available if CONFIG_CMD_FRU=y. diff --git a/doc/usage/index.rst b/doc/usage/index.rst index 28f9683a3e6f..e96a16356307 100644 --- a/doc/usage/index.rst +++ b/doc/usage/index.rst @@ -45,6 +45,7 @@ Shell commands      cmd/fatload      cmd/fdt      cmd/for +   cmd/fru      cmd/gpio      cmd/load      cmd/loadm diff --git a/include/fru.h b/include/fru.h index 2b19033a3843..1d11fd1a5964 100644 --- a/include/fru.h +++ b/include/fru.h @@ -10,6 +10,21 @@   #include <linux/list.h> +/**

• • struct fru_common_hdr - FRU common header
• • @version:        Common header format version
• • @off_internal:    Internal use area starting offset
• • @off_chassis:    Chassis info area starting offset
• • @off_board:        Board area starting offset
• • @off_product:    Product info area starting offset
• • @off_multirec:    MultiRecord area starting offset
• • @pad:        PAD, write as 00h
• • @crc:        Common header checksum (zero checksum)
• • Offsets are all in multiples of 8 bytes). 00h indicates that the

area is not

• • present.
• */

struct fru_common_hdr {       u8 version;       u8 off_internal; @@ -24,6 +39,17 @@ struct fru_common_hdr {   #define FRU_INFO_FIELD_LEN_MAX        32   #define FRU_MULTIREC_DATA_LEN_MAX    255 +/**

• • struct fru_board_info_header - Board info area header
• • @ver:    Board area format version
• • @len:    Board area length (in multiples of 8 bytes)
• • @lang_code:    Language code
• • @time:    Mfg. date / time
• *        Number of minutes from 0:00 hrs 1/1/96.
• *        LSbyte first (little endian)
• *        00_00_00h = unspecified
• */

struct fru_board_info_header {       u8 ver;       u8 len; @@ -31,27 +57,71 @@ struct fru_board_info_header {       u8 time[3];   } __packed; +/**

• • struct fru_product_info_header - Product info area header
• • @ver:    Product area format version
• • @len:    Product area length (in multiples of 8 bytes)
• • @lang_code:    Language code
• */

struct fru_product_info_header {       u8 ver;       u8 len;       u8 lang_code;   } __packed; +/**

• • struct fru_common_info_member - FRU common info member
• • @type_len:    type/length byte
• • @name:    Member information bytes
• */

struct fru_common_info_member {       u8 type_len;       u8 *name;   } __packed; +/**

• • struct fru_custom_info - Custom info field
• • @type_len:    Type/length byte
• • @data:    Custom information bytes
• */

struct fru_custom_info {       u8 type_len;       u8 data[FRU_INFO_FIELD_LEN_MAX];   } __packed; +/**

• • struct fru_custom_field_node - Linked list head for Custom info

fields

• • @list:    Linked list head
• • @info:    Custom info field of the node
• */

struct fru_custom_field_node {       struct list_head list;       struct fru_custom_info info;   }; +/**

• • struct fru_board_data - Board info area
• • @ver:            Board area format version
• • @len:            Board area length (in multiples of 8 bytes)
• • @lang_code:            Language code
• • @time:            Mfg. date / time
• • @manufacturer_type_len:    Type/length byte
• • @manufacturer_name:        Board manufacturer name
• • @product_name_type_len:    Type/length byte
• • @product_name:        Board product name
• • @serial_number_type_len:    Type/length byte
• • @serial_number:        Board serial number
• • @part_number_type_len:    Type/length byte
• • @part_number:        Board part number
• • @file_id_type_len:        Type/length byte
• • @file_id:            FRU file ID
• • @custom_fields:        Linked list head for Custom info fields
• */

struct fru_board_data {       u8 ver;       u8 len; @@ -70,6 +140,28 @@ struct fru_board_data {       struct list_head custom_fields;   }; +/**

• • struct fru_product_data - Product info area
• • @ver:            Product area format version
• • @len:            Product area length (in multiples of 8 bytes)
• • @lang_code:            Language code
• • @manufacturer_type_len:    Type/length byte
• • @manufacturer_name:        Product manufacturer name
• • @product_name_type_len:    Type/length byte
• • @product_name:        Product name
• • @part_number_type_len:    Type/length byte
• • @part_number:        Product part number
• • @version_number_type_len:    Type/length byte
• • @version_number:        Product version number
• • @serial_number_type_len:    Type/length byte
• • @serial_number:        Product serial number
• • @asset_number_type_len:    Type/length byte
• • @asset_number:        Product asset number
• • @file_id_type_len:        Type/length byte
• • @file_id:            FRU file ID
• • @custom_fields:        Linked list head for Custom info fields
• */

struct fru_product_data {       u8 ver;       u8 len; @@ -91,6 +183,15 @@ struct fru_product_data {       struct list_head custom_fields;   }; +/**

• • struct fru_multirec_hdr - MultiRecord area header
• • @rec_type:    Product area format version
• • @type:    Product area length (in multiples of 8 bytes)
• • @len:    Language code
• • @csum:    Type/length byte
• • @hdr_csum:    Product manufacturer name
• */

struct fru_multirec_hdr {       u8 rec_type;       u8 type; @@ -99,16 +200,37 @@ struct fru_multirec_hdr {       u8 hdr_csum;   } __packed; +/**

• • struct fru_multirec_info - MultiRecord info field
• • @hdr:    MultiRecord area header
• • @data:    MultiRecord information bytes
• */

struct fru_multirec_info {       struct fru_multirec_hdr hdr;       u8 data[FRU_MULTIREC_DATA_LEN_MAX];   } __packed; +/**

• • struct fru_multirec_node - Linked list head for MultiRecords
• • @list:    Linked list head
• • @info:    MultiRecord info field of the node
• */

struct fru_multirec_node {       struct list_head list;       struct fru_multirec_info info;   }; +/**

• • struct fru_table - FRU table storage
• • @hdr:    FRU common header
• • @brd:    Board info
• • @prd:    Product info
• • @multi_recs:    MultiRecords
• • @captured:    TRUE when this table is captured and parsed
• */

struct fru_table {       struct fru_common_hdr hdr;       struct fru_board_data brd; @@ -135,12 +257,72 @@ struct fru_table {   #define FRU_TYPELEN_TYPE_BINARY        0   #define FRU_TYPELEN_TYPE_ASCII8        3 +/**

• • fru_display() - display captured FRU information
• • @verbose:    Enable (1) verbose output or not (0)
• • Returns 0 on success or a negative error code.
• */

int fru_display(int verbose);

+/**

• • fru_display() - parse and capture FRU configuration table
• • @addr:    Address where the FRU configuration table is stored
• • Returns 0 on success or a negative error code.
• */

int fru_capture(const void *addr);

+/**

• • fru_board_generate() - generate FRU which has board info area
• • @addr:    Address to store generated FRU configuration table at
• • @argc:    Length of @argv
• • @argv:    Vector of arguments. See doc/usage/fru.rst for more details
• • Returns 0 on success or a negative error code.
• */

int fru_board_generate(const void *addr, int argc, char *const argv[]);

+/**

• • fru_product_generate() - generate FRU which has product info area
• • @addr:    Address to store generated FRU configuration table at
• • @argc:    Length of @argv
• • @argv:    Vector of arguments. See doc/usage/fru.rst for more details
• • Returns 0 on success or a negative error code.
• */

int fru_product_generate(const void *addr, int argc, char *const argv[]);

+/**

• • fru_checksum() - calculate checksum of FRU info
• • @addr:    Address of the FRU info data source
• • @len:    Length of the FRU info data
• • Returns a calculated checksum.
• */

u8 fru_checksum(u8 *addr, u8 len);

+/**

• • fru_check_type_len() - check and parse type/len byte
• • @type_len:    Type/len byte
• • @language:    Language code byte
• • @type:    Pointer to a variable to store parsed type
• • Returns length of the type, -EINVAL on the FRU_TYPELEN_EOF type
• */

int fru_check_type_len(u8 type_len, u8 language, u8 *type);

+/**

• • fru_get_fru_data() - get pointer to captured FRU info table
• • Returns pointer to captured FRU table
• */

const struct fru_table *fru_get_fru_data(void);   #endif /* FRU_H */

please fix these warnings.

$ ./scripts/kernel-doc -v -man include/fru.h >/dev/null include/fru.h:14: info: Scanning doc for struct fru_common_hdr include/fru.h:43: info: Scanning doc for struct fru_board_info_header include/fru.h:61: info: Scanning doc for struct fru_product_info_header include/fru.h:74: info: Scanning doc for struct fru_common_info_member include/fru.h:85: info: Scanning doc for struct fru_custom_info include/fru.h:96: info: Scanning doc for struct fru_custom_field_node include/fru.h:107: info: Scanning doc for struct fru_board_data include/fru.h:144: info: Scanning doc for struct fru_product_data include/fru.h:187: info: Scanning doc for struct fru_multirec_hdr include/fru.h:204: info: Scanning doc for struct fru_multirec_info include/fru.h:215: info: Scanning doc for struct fru_multirec_node include/fru.h:226: info: Scanning doc for struct fru_table include/fru.h:261: info: Scanning doc for fru_display include/fru.h:267: warning: No description found for return value of 'fru_display' include/fru.h:270: info: Scanning doc for fru_display include/fru.h:276: warning: No description found for return value of 'fru_capture' include/fru.h:279: info: Scanning doc for fru_board_generate include/fru.h:287: warning: No description found for return value of 'fru_board_generate' include/fru.h:290: info: Scanning doc for fru_product_generate include/fru.h:298: warning: No description found for return value of 'fru_product_generate' include/fru.h:301: info: Scanning doc for fru_checksum include/fru.h:308: warning: No description found for return value of 'fru_checksum' include/fru.h:311: info: Scanning doc for fru_check_type_len include/fru.h:319: warning: No description found for return value of 'fru_check_type_len' include/fru.h:322: info: Scanning doc for fru_get_fru_data include/fru.h:326: warning: No description found for return value of 'fru_get_fru_data' 7 warnings

I added the return value description like below for an example.

* Returns 0 on success or a negative error code.

But it's not detected as valid description by the kernel-doc script. Will change it like below.

* Return: 0 on success or a negative error code.

Checked that it doesn't make the warnings.

Thank you,

Jae