[PATCH v4 1/6] xilinx: common: refactor FRU handling support
Jae Hyun Yoo
quic_jaehyoo at quicinc.com
Thu Sep 22 18:22:10 CEST 2022
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 at 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 at 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 at 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 at 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
More information about the U-Boot
mailing list