[PATCH v4 1/6] xilinx: common: refactor FRU handling support
Michal Simek
michal.simek at amd.com
Thu Sep 22 09:29:34 CEST 2022
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.
>
>>> + " 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.
>>> +#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.
>
>>> +
>>> +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
>>> }
>>> 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.
M
More information about the U-Boot
mailing list