Re: [PATCH v1 1/2] cmd: fru: move FRU handling support to common region

On 7/29/22 23:54, Jae Hyun Yoo wrote:

From: Graeme Gregory quic_ggregory@quicinc.com

The FRU handling was added as a Xilinx board dependent support but it would be useful for other boards too, so this commit moves the FRU handling support to the common region so that it can be enabled by CONFIG_CMD_FRU.

To provide manufacturer specific custom board info field parsing, it defines 'fru_parse_board_custom' as a weak function so that it can be replaced with the board specific implementation. In the same way, OEM Multirecord type (0xc0 - 0xff) parsing logic can be replaced with a board specific 'fru_parse_multirec' implementation.

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

Your patchset lacks documentation.

Please, add a man-page in doc/usage/cmd/fru.rst.

---

Changes from RFC:

• Made FRU typecode string table as a generic and sharable table. (Michal)
• Made OEM multirecord parsing call happen only on customizable type IDs. (Michal)
• Added manufacturer custom board info fields parsing flow. (Michal)

board/xilinx/Kconfig | 8 -- board/xilinx/common/Makefile | 3 - board/xilinx/common/board.c | 108 +++++++++++++++++-- cmd/Kconfig | 8 ++ cmd/Makefile | 1 + {board/xilinx/common => cmd}/fru.c | 5 +- common/Makefile | 2 + {board/xilinx/common => common}/fru_ops.c | 126 ++++++++++++++--------

Why should this live in common/. lib/ would make more sense.

{board/xilinx/common => include}/fru.h | 25 +---- 9 files changed, 197 insertions(+), 89 deletions(-) rename {board/xilinx/common => cmd}/fru.c (95%) rename {board/xilinx/common => common}/fru_ops.c (80%) rename {board/xilinx/common => include}/fru.h (76%)

diff --git a/board/xilinx/Kconfig b/board/xilinx/Kconfig index 17880661736d..110706b20fa3 100644 --- a/board/xilinx/Kconfig +++ b/board/xilinx/Kconfig @@ -74,11 +74,3 @@ config ZYNQ_GEM_I2C_MAC_OFFSET Set the MAC offset for i2C.

endif

-config CMD_FRU

• bool "FRU information for product"
• help

•  This option enables FRU commands to capture and display FRU
  

•  information present in the device. The FRU Information is used
  

•  to primarily to provide "inventory" information about the boards
  

•  that the FRU Information Device is located on.
  

diff --git a/board/xilinx/common/Makefile b/board/xilinx/common/Makefile index cdc3c9677432..e33baaae1159 100644 --- a/board/xilinx/common/Makefile +++ b/board/xilinx/common/Makefile @@ -8,6 +8,3 @@ obj-y += board.o ifndef CONFIG_ARCH_ZYNQ obj-$(CONFIG_DISPLAY_CPUINFO) += cpu-info.o endif -ifndef CONFIG_SPL_BUILD -obj-$(CONFIG_CMD_FRU) += fru.o fru_ops.o -endif diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c index 9b4aded466ab..484fb43c84e1 100644 --- a/board/xilinx/common/board.c +++ b/board/xilinx/common/board.c @@ -8,6 +8,7 @@ #include <efi.h> #include <efi_loader.h> #include <env.h> +#include <fru.h> #include <log.h> #include <asm/global_data.h> #include <asm/sections.h> @@ -25,8 +26,6 @@ #include <linux/kernel.h> #include <uuid.h>

-#include "fru.h"

• #if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT) struct efi_fw_image fw_images[] = { #if defined(XILINX_BOOT_IMAGE_GUID)

@@ -88,6 +87,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 +118,26 @@ 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];

+};

+static u8 parsed_macid[EEPROM_HDR_NO_OF_MAC_ADDR][ETH_ALEN]; +struct xilinx_board_custom_data {

• 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];

+} __packed;

+#define FRU_BOARD_CUSTOM_AREA_TOTAL_FIELDS 3

+struct xilinx_board_custom_data fru_brd_custom;

• static void xilinx_eeprom_legacy_cleanup(char *eeprom, int size) { int i;

@@ -197,9 +219,75 @@ static bool xilinx_detect_legacy(u8 *buffer) return true; }

+int fru_parse_board_custom(unsigned long addr) +{

• const struct fru_table *fru_data = fru_get_fru_data();
• u8 *data, *term, *limit;
• u8 i, type;
• int len;
• data = (u8 *)&fru_brd_custom;
• memset(data, 0, sizeof(struct xilinx_board_custom_data));
• /* Record max structure limit not to write data over allocated space */
• limit = (u8 *)&fru_brd_custom + sizeof(struct xilinx_board_custom_data);
• for (i = 0; i < FRU_BOARD_AREA_TOTAL_FIELDS;

•     i++, data += FRU_BOARD_MAX_LEN) {
  

• len = fru_check_type_len(*(u8 *)addr, fru_data->brd.lang_code,
  

• 			 &type);
  

• /* Stop cature if it end of fields */
  

• if (len == -EINVAL)
  

• 	break;
  

• /* Stop when amount of chars 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;
  

• /* Record data field */
  

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

• term = data + (u8)len;
  

• *term = 0;
  

• addr += len;
  

• }
• return 0;

+}

+int fru_parse_multirec_oem(unsigned long addr) +{

• u8 hdr_len = sizeof(struct fru_multirec_hdr);
• struct fru_multirec_hdr *mrc;
• mrc = (struct fru_multirec_hdr *)addr;
• if (mrc->rec_type == EEPROM_MULTIREC_TYPE_XILINX_OEM) {

• struct xilinx_multirec_mac *mac = (void *)addr + hdr_len;
  

• if (mac->ver == EEPROM_MULTIREC_DUT_MACID) {
  

• 	int mac_len = mrc->len - EEPROM_MULTIREC_MAC_OFFSET;
  

• 	memcpy(parsed_macid, mac->macid, mac_len);
  

• }
  

• }
• return 0;

+}

• static int xilinx_read_eeprom_fru(struct udevice *dev, char *name, struct xilinx_board_description *desc) {
• const struct fru_table *fru_data; int i, ret, eeprom_size; u8 *fru_content; u8 id = 0;

@@ -237,30 +325,32 @@ static int xilinx_read_eeprom_fru(struct udevice *dev, char *name, goto end; }

• fru_data = fru_get_fru_data();
• /* 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 *)fru_brd_custom.uuid, 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 *)fru_brd_custom.rev, 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/cmd/Kconfig b/cmd/Kconfig index a8260aa170d0..644c907bf83a 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1053,6 +1053,14 @@ config CMD_FPGAD fpga_get_reg() function. This functions similarly to the 'md' command.

+config CMD_FRU

• bool "FRU information for product"
• help

•  This option enables FRU commands to capture and display FRU
  

•  information present in the device. The FRU Information is used
  

•  to primarily to provide "inventory" information about the boards
  

•  that the FRU Information Device is located on.
  

Don't assume that a user knows what that FRU abbreviation stands for. Should it relate to a Field Replaceable Unit, please, write once the full text.

Which revision of the IPMI Storage FRU Layout does it support?

Where is the unit test for the command?

Best regards

Heinrich

• config CMD_FUSE bool "fuse - support for the fuse subssystem" help

diff --git a/cmd/Makefile b/cmd/Makefile index 5e43a1e022e8..10a18d02eb08 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -74,6 +74,7 @@ obj-$(CONFIG_CMD_SQUASHFS) += sqfs.o obj-$(CONFIG_CMD_FLASH) += flash.o obj-$(CONFIG_CMD_FPGA) += fpga.o obj-$(CONFIG_CMD_FPGAD) += fpgad.o +obj-$(CONFIG_CMD_FRU) += fru.o obj-$(CONFIG_CMD_FS_GENERIC) += fs.o obj-$(CONFIG_CMD_FUSE) += fuse.o obj-$(CONFIG_CMD_GETTIME) += gettime.o diff --git a/board/xilinx/common/fru.c b/cmd/fru.c similarity index 95% rename from board/xilinx/common/fru.c rename to cmd/fru.c index f6ca46c3cecc..e8ce3d941028 100644 --- a/board/xilinx/common/fru.c +++ b/cmd/fru.c @@ -6,10 +6,9 @@ #include <common.h> #include <command.h> #include <fdtdec.h> +#include <fru.h> #include <malloc.h>

-#include "fru.h"

• static int do_fru_capture(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) {

@@ -78,7 +77,7 @@ 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"

• " <part number> <custom> - Generate FRU format with\n" " board info area filled based on parameters. <addr> is\n" " pointing to place where FRU is generated.\n" ;

diff --git a/common/Makefile b/common/Makefile index 2ed8672c3ac1..d5c9de33ac07 100644 --- a/common/Makefile +++ b/common/Makefile @@ -112,3 +112,5 @@ obj-$(CONFIG_$(SPL_TPL_)STACKPROTECTOR) += stackprot.o obj-$(CONFIG_SCP03) += scp03.o

obj-$(CONFIG_QFW) += qfw.o

+obj-$(CONFIG_CMD_FRU) += fru_ops.o diff --git a/board/xilinx/common/fru_ops.c b/common/fru_ops.c similarity index 80% rename from board/xilinx/common/fru_ops.c rename to common/fru_ops.c index 49846ae3d660..c03eeffbddc6 100644 --- a/board/xilinx/common/fru_ops.c +++ b/common/fru_ops.c @@ -1,22 +1,32 @@ // 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 <fru.h> +#include <hexdump.h> #include <log.h> #include <malloc.h> -#include <net.h> #include <asm/io.h> #include <asm/arch/hardware.h>

-#include "fru.h" +#define DEBUG_PARSE_CUSTOM_FIELDS 0 /* set to 1 to debug */

struct fru_table fru_data __section(".data");

+static const char * const fru_typecode_str[] = {

• "Binary/Unspecified",
• "BCD plus",
• "6-bit ASCII",
• "8-bit ASCII",
• "2-byte UNICODE"

+};

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

@@ -57,7 +67,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;

@@ -90,7 +100,7 @@ static u8 fru_gen_type_len(u8 *addr, char *name) }

int fru_generate(unsigned long addr, char *manufacturer, char *board_name,

•  char *serial_no, char *part_no, char *revision)
  

•  char *serial_no, char *part_no, char *custom)
  

  { struct fru_common_hdr *header = (struct fru_common_hdr *)addr; struct fru_board_info_header *board_info;

@@ -136,7 +146,7 @@ int fru_generate(unsigned long addr, char *manufacturer, char *board_name, member += len; len = fru_gen_type_len(member, "U-Boot generator"); /* File ID */ member += len;

• len = fru_gen_type_len(member, revision); /* Revision */

• len = fru_gen_type_len(member, custom); /* Custom field */ member += len;

  *member++ = 0xc1; /* Indication of no more fields */

@@ -168,10 +178,37 @@ int fru_generate(unsigned long addr, char *manufacturer, char *board_name, return 0; }

+__weak int fru_parse_board_custom(unsigned long addr) +{

• int len;
• u8 type;
• do {

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

• 			 &type);
  

• if (len == -EINVAL)
  

• 	break;
  

• addr += 1;
  

• /* Skip empty field */
  

• if (!len)
  

• 	continue;
  

• if (DEBUG_PARSE_CUSTOM_FIELDS)
  

• 	print_hex_dump_bytes("Board Custom Field: ",
  

• 			     DUMP_PREFIX_NONE, (u8 *)addr, len);
  

• addr += len;
  

• } while (true);
• return 0;

+}

• static int fru_parse_board(unsigned long addr) { u8 i, type;

• int len;

• int len, ret = 0; u8 *data, *term, *limit;

  memcpy(&fru_data.brd.ver, (void *)addr, 6);

@@ -181,7 +218,8 @@ static int fru_parse_board(unsigned long addr) /* Record max structure limit not to write data over allocated space */ limit = (u8 *)&fru_data.brd + sizeof(struct fru_board_data);

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

• for (i = 0; i < FRU_BOARD_AREA_TOTAL_FIELDS;

•     i++, data += FRU_BOARD_MAX_LEN) {
  

  len = fru_check_type_len(*(u8 *)addr, fru_data.brd.lang_code, &type); /*

@@ -193,6 +231,7 @@ static int fru_parse_board(unsigned long addr) /* Stop when amount of chars is more then fields to record */ if (data + len > limit) break;

• /* This record type/len field */ *data++ = *(u8 *)addr;

@@ -216,37 +255,45 @@ static int fru_parse_board(unsigned long addr) return -EINVAL; }

• len = fru_check_type_len(*(u8 *)addr, fru_data.brd.lang_code, &type);
• /* If it has custom fields, do custom parsing */
• if (len != -EINVAL)

• ret = fru_parse_board_custom(addr);
  

• return ret;

+}

+__weak int fru_parse_multirec_oem(unsigned long addr) +{

• struct fru_multirec_hdr *mrc = (struct fru_multirec_hdr *)addr;

• debug("%s: multirec rec_type: 0x%x, type: 0x%x, len: %d\n",

•      __func__, mrc->rec_type, mrc->type, mrc->len);
  

• 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 *mrc;

  debug("%s: multirec addr %lx\n", __func__, 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;
  

• mrc = (struct fru_multirec_hdr *)addr;
  
  

• 	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));

• if (mrc->rec_type >= 0xc0 && mrc->rec_type <= 0xff)
  

• 	fru_parse_multirec_oem(addr);
  

• addr += mrc->len + hdr_len;
  

• } while (!(mrc->type & FRU_LAST_REC));

  return 0; }

@@ -255,7 +302,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) {

@@ -270,17 +316,13 @@ int fru_capture(unsigned long addr)

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", addr);

  return 0; }

@@ -291,21 +333,12 @@ static int fru_display_board(struct fru_board_data *brd, int verbose) u8 type; int len; u8 *data;

• 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", "Serial No", "Part Number", "File ID",

• /* Xilinx spec */
  

• "Revision Number",
  

  };

  if (verbose) {

@@ -336,9 +369,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]);

@@ -413,3 +446,8 @@ int fru_display(int verbose)

return fru_display_board(&fru_data.brd, verbose); }

+const struct fru_table *fru_get_fru_data(void) +{

• return &fru_data;

+} diff --git a/board/xilinx/common/fru.h b/include/fru.h similarity index 76% rename from board/xilinx/common/fru.h rename to include/fru.h index 59f6b722cf12..f64fe1cca5e6 100644 --- a/board/xilinx/common/fru.h +++ b/include/fru.h @@ -6,7 +6,6 @@

#ifndef __FRU_H #define __FRU_H -#include <net.h>

struct fru_common_hdr { u8 version; @@ -20,7 +19,6 @@ struct fru_common_hdr { };

#define FRU_BOARD_MAX_LEN 32 -#define FRU_MAX_NO_OF_MAC_ADDR 4

struct __packed fru_board_info_header { u8 ver; @@ -49,13 +47,6 @@ struct fru_board_data { u8 part_number[FRU_BOARD_MAX_LEN]; 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]; };

  struct fru_multirec_hdr {

@@ -66,16 +57,9 @@ struct fru_multirec_hdr { u8 hdr_csum; };

-struct fru_multirec_mac {

• u8 xlnx_iana_id[3];
• u8 ver;
• u8 macid[FRU_MAX_NO_OF_MAC_ADDR][ETH_ALEN];

-};

• struct fru_table { struct fru_common_hdr hdr; struct fru_board_data brd;
• struct fru_multirec_mac mac; bool captured; };

@@ -86,10 +70,7 @@ struct fru_table { #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

/* This should be minimum of fields */ #define FRU_BOARD_AREA_TOTAL_FIELDS 5 @@ -100,9 +81,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);
  

•  char *serial_no, char *part_no, char *custom);
  

  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); +const struct fru_table *fru_get_fru_data(void);

#endif /* FRU_H */