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

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

[Jae Hyun Yoo]

Hello,

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.

Also, this series adds 'Product Info' parsing support.

Please review!

Thanks,
Jae

Graeme Gregory (1):
  cmd: fru: move FRU handling support to common region

Jae Hyun Yoo (1):
  cmd: fru: add product info area parsing support

 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 | 287 ++++++++++++++++++----
 {board/xilinx/common => include}/fru.h    |  47 ++--
 9 files changed, 379 insertions(+), 90 deletions(-)
 rename {board/xilinx/common => cmd}/fru.c (95%)
 rename {board/xilinx/common => common}/fru_ops.c (60%)
 rename {board/xilinx/common => include}/fru.h (70%)

-- 
2.25.1

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

[Jae Hyun Yoo]

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>
---
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 ++++++++++++++--------
 {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.
+
 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 */
-- 
2.25.1

[PATCH v1 2/2] cmd: fru: add product info area parsing support

[Jae Hyun Yoo]

Add product info area parsing support. Custom product info field parsing
function 'fru_parse_product_custom' can be replaced with a board specific
implementation.

Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
---
Changes from RFC:
 * Added manufacturer custom product info fields parsing flow.

 common/fru_ops.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++-
 include/fru.h    |  22 +++++++
 2 files changed, 182 insertions(+), 1 deletion(-)

diff --git a/common/fru_ops.c b/common/fru_ops.c
index c03eeffbddc6..9f350f875035 100644
--- a/common/fru_ops.c
+++ b/common/fru_ops.c
@@ -264,6 +264,91 @@ static int fru_parse_board(unsigned long addr)
 	return ret;
 }
 
+__weak int fru_parse_product_custom(unsigned long addr)
+{
+	int len;
+	u8 type;
+
+	do {
+		len = fru_check_type_len(*(u8 *)addr, fru_data.prd.lang_code,
+					 &type);
+		if (len == -EINVAL)
+			break;
+
+		addr += 1;
+
+		/* Skip empty field */
+		if (!len)
+			continue;
+
+		if (DEBUG_PARSE_CUSTOM_FIELDS)
+			print_hex_dump_bytes("Product Custom Field: ",
+					     DUMP_PREFIX_NONE, (u8 *)addr, len);
+
+		addr += len;
+	} while (true);
+
+	return 0;
+}
+
+static int fru_parse_product(unsigned long addr)
+{
+	u8 i, type;
+	int len, ret = 0;
+	u8 *data, *term, *limit;
+
+	memcpy(&fru_data.prd.ver, (void *)addr, 6);
+	addr += 3;
+	data = (u8 *)&fru_data.prd.manufacturer_type_len;
+
+	/* Record max structure limit not to write data over allocated space */
+	limit = (u8 *)&fru_data.prd + sizeof(struct fru_product_data);
+
+	for (i = 0; i < FRU_PRODUCT_AREA_TOTAL_FIELDS;
+	     i++, data += FRU_BOARD_MAX_LEN) {
+		len = fru_check_type_len(*(u8 *)addr, fru_data.prd.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;
+	}
+
+	if (i < FRU_PRODUCT_AREA_TOTAL_FIELDS) {
+		printf("Product area require minimum %d fields\n",
+		       FRU_PRODUCT_AREA_TOTAL_FIELDS);
+		return -EINVAL;
+	}
+
+	len = fru_check_type_len(*(u8 *)addr, fru_data.prd.lang_code, &type);
+
+	/* If it has custom fields, do custom parsing */
+	if (len != -EINVAL)
+		ret = fru_parse_product_custom(addr);
+
+	return ret;
+}
+
 __weak int fru_parse_multirec_oem(unsigned long addr)
 {
 	struct fru_multirec_hdr *mrc = (struct fru_multirec_hdr *)addr;
@@ -319,6 +404,9 @@ int fru_capture(unsigned long addr)
 	if (hdr->off_board)
 		fru_parse_board(addr + fru_cal_area_len(hdr->off_board));
 
+	if (hdr->off_product)
+		fru_parse_product(addr + fru_cal_area_len(hdr->off_product));
+
 	if (hdr->off_multirec)
 		fru_parse_multirec(addr + fru_cal_area_len(hdr->off_multirec));
 
@@ -397,6 +485,71 @@ static int fru_display_board(struct fru_board_data *brd, int verbose)
 	return 0;
 }
 
+static int fru_display_product(struct fru_product_data *prd, int verbose)
+{
+	u8 type;
+	int len;
+	u8 *data;
+	static const char * const productinfo[] = {
+		"Manufacturer Name",
+		"Product Name",
+		"Part Number",
+		"Version Number",
+		"Serial No",
+		"Asset Number",
+		"File ID",
+	};
+
+	if (verbose) {
+		printf("*****PRODUCT INFO*****\n");
+		printf("Version:%d\n", fru_version(prd->ver));
+		printf("Product Area Length:%d\n", fru_cal_area_len(prd->len));
+	}
+
+	if (fru_check_language(prd->lang_code))
+		return -EINVAL;
+
+	data = (u8 *)&prd->manufacturer_type_len;
+
+	for (u8 i = 0; i < (sizeof(productinfo) / sizeof(*productinfo)); i++) {
+		len = fru_check_type_len(*data++, prd->lang_code,
+					 &type);
+		if (len == -EINVAL) {
+			printf("**** EOF for Product Area ****\n");
+			break;
+		}
+
+		if (type <= FRU_TYPELEN_TYPE_ASCII8 &&
+		    (prd->lang_code == FRU_LANG_CODE_ENGLISH ||
+		     prd->lang_code == FRU_LANG_CODE_ENGLISH_1))
+			debug("Type code: %s\n", fru_typecode_str[type]);
+		else
+			debug("Type code: %s\n", fru_typecode_str[type + 1]);
+
+		if (!len) {
+			debug("%s not found\n", productinfo[i]);
+			continue;
+		}
+
+		switch (type) {
+		case FRU_TYPELEN_TYPE_BINARY:
+			debug("Length: %d\n", len);
+			printf(" %s: 0x%x\n", productinfo[i], *data);
+			break;
+		case FRU_TYPELEN_TYPE_ASCII8:
+			debug("Length: %d\n", len);
+			printf(" %s: %s\n", productinfo[i], data);
+			break;
+		default:
+			debug("Unsupported type %x\n", type);
+		}
+
+		data += FRU_BOARD_MAX_LEN;
+	}
+
+	return 0;
+}
+
 static void fru_display_common_hdr(struct fru_common_hdr *hdr, int verbose)
 {
 	if (!verbose)
@@ -437,6 +590,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;
@@ -444,7 +599,11 @@ 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_product(&fru_data.prd, verbose);
 }
 
 const struct fru_table *fru_get_fru_data(void)
diff --git a/include/fru.h b/include/fru.h
index f64fe1cca5e6..14643fd9616c 100644
--- a/include/fru.h
+++ b/include/fru.h
@@ -49,6 +49,26 @@ struct fru_board_data {
 	u8 file_id[FRU_BOARD_MAX_LEN];
 };
 
+struct fru_product_data {
+	u8 ver;
+	u8 len;
+	u8 lang_code;
+	u8 manufacturer_type_len;
+	u8 manufacturer_name[FRU_BOARD_MAX_LEN];
+	u8 product_name_type_len;
+	u8 product_name[FRU_BOARD_MAX_LEN];
+	u8 part_number_type_len;
+	u8 part_number[FRU_BOARD_MAX_LEN];
+	u8 version_number_type_len;
+	u8 version_number[FRU_BOARD_MAX_LEN];
+	u8 serial_number_type_len;
+	u8 serial_number[FRU_BOARD_MAX_LEN];
+	u8 asset_number_type_len;
+	u8 asset_number[FRU_BOARD_MAX_LEN];
+	u8 file_id_type_len;
+	u8 file_id[FRU_BOARD_MAX_LEN];
+};
+
 struct fru_multirec_hdr {
 	u8 rec_type;
 	u8 type;
@@ -60,6 +80,7 @@ struct fru_multirec_hdr {
 struct fru_table {
 	struct fru_common_hdr hdr;
 	struct fru_board_data brd;
+	struct fru_product_data prd;
 	bool captured;
 };
 
@@ -74,6 +95,7 @@ struct fru_table {
 
 /* This should be minimum of fields */
 #define FRU_BOARD_AREA_TOTAL_FIELDS	5
+#define FRU_PRODUCT_AREA_TOTAL_FIELDS	7
 #define FRU_TYPELEN_TYPE_SHIFT		6
 #define FRU_TYPELEN_TYPE_BINARY		0
 #define FRU_TYPELEN_TYPE_ASCII8		3
-- 
2.25.1

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

[Michal Simek]

On 7/29/22 23:54, Jae Hyun Yoo wrote:
> Hello,
> 
> 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.
> 
> Also, this series adds 'Product Info' parsing support.
> 
> Please review!

In general I am fine with this but I want this to be done in steps to be able to 
better review it.
It means couple of preparation patches before this is moved to generic location.
Moving that part of xilinx private structure of board info as one step, 
multirecord OEM entries another one, etc.

Thanks,
Michal

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

[Heinrich Schuchardt]

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 */

Re: [PATCH v1 2/2] cmd: fru: add product info area parsing support

[Heinrich Schuchardt]

On 7/29/22 23:54, Jae Hyun Yoo wrote:
> Add product info area parsing support. Custom product info field parsing
> function 'fru_parse_product_custom' can be replaced with a board specific
> implementation.
>
> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
> ---
> Changes from RFC:
>   * Added manufacturer custom product info fields parsing flow.

Please, provide a unit test for the new functions.

Best regards

Heinrich

>
>   common/fru_ops.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++-
>   include/fru.h    |  22 +++++++
>   2 files changed, 182 insertions(+), 1 deletion(-)
>
> diff --git a/common/fru_ops.c b/common/fru_ops.c
> index c03eeffbddc6..9f350f875035 100644
> --- a/common/fru_ops.c
> +++ b/common/fru_ops.c
> @@ -264,6 +264,91 @@ static int fru_parse_board(unsigned long addr)
>   	return ret;
>   }
>
> +__weak int fru_parse_product_custom(unsigned long addr)
> +{
> +	int len;
> +	u8 type;
> +
> +	do {
> +		len = fru_check_type_len(*(u8 *)addr, fru_data.prd.lang_code,
> +					 &type);
> +		if (len == -EINVAL)
> +			break;
> +
> +		addr += 1;
> +
> +		/* Skip empty field */
> +		if (!len)
> +			continue;
> +
> +		if (DEBUG_PARSE_CUSTOM_FIELDS)
> +			print_hex_dump_bytes("Product Custom Field: ",
> +					     DUMP_PREFIX_NONE, (u8 *)addr, len);
> +
> +		addr += len;
> +	} while (true);
> +
> +	return 0;
> +}
> +
> +static int fru_parse_product(unsigned long addr)
> +{
> +	u8 i, type;
> +	int len, ret = 0;
> +	u8 *data, *term, *limit;
> +
> +	memcpy(&fru_data.prd.ver, (void *)addr, 6);
> +	addr += 3;
> +	data = (u8 *)&fru_data.prd.manufacturer_type_len;
> +
> +	/* Record max structure limit not to write data over allocated space */
> +	limit = (u8 *)&fru_data.prd + sizeof(struct fru_product_data);
> +
> +	for (i = 0; i < FRU_PRODUCT_AREA_TOTAL_FIELDS;
> +	     i++, data += FRU_BOARD_MAX_LEN) {
> +		len = fru_check_type_len(*(u8 *)addr, fru_data.prd.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;
> +	}
> +
> +	if (i < FRU_PRODUCT_AREA_TOTAL_FIELDS) {
> +		printf("Product area require minimum %d fields\n",
> +		       FRU_PRODUCT_AREA_TOTAL_FIELDS);
> +		return -EINVAL;
> +	}
> +
> +	len = fru_check_type_len(*(u8 *)addr, fru_data.prd.lang_code, &type);
> +
> +	/* If it has custom fields, do custom parsing */
> +	if (len != -EINVAL)
> +		ret = fru_parse_product_custom(addr);
> +
> +	return ret;
> +}
> +
>   __weak int fru_parse_multirec_oem(unsigned long addr)
>   {
>   	struct fru_multirec_hdr *mrc = (struct fru_multirec_hdr *)addr;
> @@ -319,6 +404,9 @@ int fru_capture(unsigned long addr)
>   	if (hdr->off_board)
>   		fru_parse_board(addr + fru_cal_area_len(hdr->off_board));
>
> +	if (hdr->off_product)
> +		fru_parse_product(addr + fru_cal_area_len(hdr->off_product));
> +
>   	if (hdr->off_multirec)
>   		fru_parse_multirec(addr + fru_cal_area_len(hdr->off_multirec));
>
> @@ -397,6 +485,71 @@ static int fru_display_board(struct fru_board_data *brd, int verbose)
>   	return 0;
>   }
>
> +static int fru_display_product(struct fru_product_data *prd, int verbose)
> +{
> +	u8 type;
> +	int len;
> +	u8 *data;
> +	static const char * const productinfo[] = {
> +		"Manufacturer Name",
> +		"Product Name",
> +		"Part Number",
> +		"Version Number",
> +		"Serial No",
> +		"Asset Number",
> +		"File ID",
> +	};
> +
> +	if (verbose) {
> +		printf("*****PRODUCT INFO*****\n");
> +		printf("Version:%d\n", fru_version(prd->ver));
> +		printf("Product Area Length:%d\n", fru_cal_area_len(prd->len));
> +	}
> +
> +	if (fru_check_language(prd->lang_code))
> +		return -EINVAL;
> +
> +	data = (u8 *)&prd->manufacturer_type_len;
> +
> +	for (u8 i = 0; i < (sizeof(productinfo) / sizeof(*productinfo)); i++) {
> +		len = fru_check_type_len(*data++, prd->lang_code,
> +					 &type);
> +		if (len == -EINVAL) {
> +			printf("**** EOF for Product Area ****\n");
> +			break;
> +		}
> +
> +		if (type <= FRU_TYPELEN_TYPE_ASCII8 &&
> +		    (prd->lang_code == FRU_LANG_CODE_ENGLISH ||
> +		     prd->lang_code == FRU_LANG_CODE_ENGLISH_1))
> +			debug("Type code: %s\n", fru_typecode_str[type]);
> +		else
> +			debug("Type code: %s\n", fru_typecode_str[type + 1]);
> +
> +		if (!len) {
> +			debug("%s not found\n", productinfo[i]);
> +			continue;
> +		}
> +
> +		switch (type) {
> +		case FRU_TYPELEN_TYPE_BINARY:
> +			debug("Length: %d\n", len);
> +			printf(" %s: 0x%x\n", productinfo[i], *data);
> +			break;
> +		case FRU_TYPELEN_TYPE_ASCII8:
> +			debug("Length: %d\n", len);
> +			printf(" %s: %s\n", productinfo[i], data);
> +			break;
> +		default:
> +			debug("Unsupported type %x\n", type);
> +		}
> +
> +		data += FRU_BOARD_MAX_LEN;
> +	}
> +
> +	return 0;
> +}
> +
>   static void fru_display_common_hdr(struct fru_common_hdr *hdr, int verbose)
>   {
>   	if (!verbose)
> @@ -437,6 +590,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;
> @@ -444,7 +599,11 @@ 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_product(&fru_data.prd, verbose);
>   }
>
>   const struct fru_table *fru_get_fru_data(void)
> diff --git a/include/fru.h b/include/fru.h
> index f64fe1cca5e6..14643fd9616c 100644
> --- a/include/fru.h
> +++ b/include/fru.h
> @@ -49,6 +49,26 @@ struct fru_board_data {
>   	u8 file_id[FRU_BOARD_MAX_LEN];
>   };
>
> +struct fru_product_data {
> +	u8 ver;
> +	u8 len;
> +	u8 lang_code;
> +	u8 manufacturer_type_len;
> +	u8 manufacturer_name[FRU_BOARD_MAX_LEN];
> +	u8 product_name_type_len;
> +	u8 product_name[FRU_BOARD_MAX_LEN];
> +	u8 part_number_type_len;
> +	u8 part_number[FRU_BOARD_MAX_LEN];
> +	u8 version_number_type_len;
> +	u8 version_number[FRU_BOARD_MAX_LEN];
> +	u8 serial_number_type_len;
> +	u8 serial_number[FRU_BOARD_MAX_LEN];
> +	u8 asset_number_type_len;
> +	u8 asset_number[FRU_BOARD_MAX_LEN];
> +	u8 file_id_type_len;
> +	u8 file_id[FRU_BOARD_MAX_LEN];
> +};
> +
>   struct fru_multirec_hdr {
>   	u8 rec_type;
>   	u8 type;
> @@ -60,6 +80,7 @@ struct fru_multirec_hdr {
>   struct fru_table {
>   	struct fru_common_hdr hdr;
>   	struct fru_board_data brd;
> +	struct fru_product_data prd;
>   	bool captured;
>   };
>
> @@ -74,6 +95,7 @@ struct fru_table {
>
>   /* This should be minimum of fields */
>   #define FRU_BOARD_AREA_TOTAL_FIELDS	5
> +#define FRU_PRODUCT_AREA_TOTAL_FIELDS	7
>   #define FRU_TYPELEN_TYPE_SHIFT		6
>   #define FRU_TYPELEN_TYPE_BINARY		0
>   #define FRU_TYPELEN_TYPE_ASCII8		3

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

[Jae Hyun Yoo]

Hi Michal,

On 8/1/2022 3:29 AM, Michal Simek wrote:
> 
> 
> On 7/29/22 23:54, Jae Hyun Yoo wrote:
>> Hello,
>>
>> 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.
>>
>> Also, this series adds 'Product Info' parsing support.
>>
>> Please review!
> 
> In general I am fine with this but I want this to be done in steps to be 
> able to better review it.
> It means couple of preparation patches before this is moved to generic 
> location.
> Moving that part of xilinx private structure of board info as one step, 
> multirecord OEM entries another one, etc.

Sure, I'll split them into individual patches in v2.

Thanks,
Jae

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

[Jae Hyun Yoo]

On 8/1/2022 5:34 AM, Heinrich Schuchardt wrote:
> 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.

Okay. I'll add a man-page in v2.

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

Right, lib/ would be a right place. Will fix it in v2.

[...]

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

Right, it relates to a Field Replaceable Unit. I'll add the full text in
the next spin.

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

It supports v1.0.
https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/ipmi-platform-mgt-fru-info-storage-def-v1-0-rev-1-3-spec-update.pdf

Will add this link into commit message.

> Where is the unit test for the command?

I'll add a unit test in v2.

Best Regards,
Jae

Re: [PATCH v1 2/2] cmd: fru: add product info area parsing support

[Jae Hyun Yoo]

On 8/1/2022 5:37 AM, Heinrich Schuchardt wrote:
> On 7/29/22 23:54, Jae Hyun Yoo wrote:
>> Add product info area parsing support. Custom product info field parsing
>> function 'fru_parse_product_custom' can be replaced with a board specific
>> implementation.
>>
>> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
>> ---
>> Changes from RFC:
>>   * Added manufacturer custom product info fields parsing flow.
> 
> Please, provide a unit test for the new functions.

I'll add a unit test in the next spin.

Best Regards,
Jae