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

[RFC PATCH 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 to be enabled by CONFIG_CMD_FRU.
Since the Multirecord parsing logic should be implemented on each
board specifically, it defines 'fru_parse_multirec' as a weak function
so that the function can be replaced with the board specific
implementation.

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

Please review!

Thanks,
Jae

Graeme Gregory (1):
  cmd/fru: 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               |  63 ++++++--
 cmd/Kconfig                               |   8 +
 cmd/Makefile                              |   1 +
 {board/xilinx/common => cmd}/fru.c        |   3 +-
 common/Makefile                           |   2 +
 {board/xilinx/common => common}/fru_ops.c | 171 +++++++++++++++++++---
 {board/xilinx/common => include}/fru.h    |  37 +++--
 9 files changed, 237 insertions(+), 59 deletions(-)
 rename {board/xilinx/common => cmd}/fru.c (99%)
 rename {board/xilinx/common => common}/fru_ops.c (73%)
 rename {board/xilinx/common => include}/fru.h (76%)

-- 
2.25.1

[RFC PATCH 1/2] cmd/fru: 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 to be enabled by CONFIG_CMD_FRU.
Since the Multirecord parsing logic should be implemented on each OEM
board specifically, it defines 'fru_parse_multirec' as a weak function
so that it can be replaced with the board specific implementation.

Signed-off-by: Graeme Gregory <quic_ggregory@quicinc.com>
Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
---
 board/xilinx/Kconfig                      |  8 ---
 board/xilinx/common/Makefile              |  3 --
 board/xilinx/common/board.c               | 63 +++++++++++++++++++----
 cmd/Kconfig                               |  8 +++
 cmd/Makefile                              |  1 +
 {board/xilinx/common => cmd}/fru.c        |  3 +-
 common/Makefile                           |  2 +
 {board/xilinx/common => common}/fru_ops.c | 37 ++++++-------
 {board/xilinx/common => include}/fru.h    | 15 +-----
 9 files changed, 82 insertions(+), 58 deletions(-)
 rename {board/xilinx/common => cmd}/fru.c (99%)
 rename {board/xilinx/common => common}/fru_ops.c (93%)
 rename {board/xilinx/common => include}/fru.h (85%)

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..061082dbe6d6 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,14 @@ struct xilinx_legacy_format {
 	char unused3[29]; /* 0xe3 */
 };
 
+struct fru_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];
+
 static void xilinx_eeprom_legacy_cleanup(char *eeprom, int size)
 {
 	int i;
@@ -197,9 +207,42 @@ static bool xilinx_detect_legacy(u8 *buffer)
 	return true;
 }
 
+int fru_parse_multirec(unsigned long addr)
+{
+	u8 hdr_len = sizeof(struct fru_multirec_hdr);
+	struct fru_multirec_hdr mrc;
+	u8 checksum;
+	int mac_len;
+
+	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) {
+			debug("%s header CRC error\n", __func__);
+			return -EINVAL;
+		}
+
+		if (mrc.rec_type == EEPROM_MULTIREC_TYPE_XILINX_OEM) {
+			struct fru_multirec_mac *mac = (void *)addr + hdr_len;
+
+			if (mac->ver == EEPROM_MULTIREC_DUT_MACID) {
+				mac_len = mrc.len - EEPROM_MULTIREC_MAC_OFFSET;
+				memcpy(parsed_macid, mac->macid, mac_len);
+			}
+		}
+		addr += mrc.len + hdr_len;
+	} while (!(mrc.type & FRU_LAST_REC));
+
+	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 +280,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_data->brd.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_data->brd.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 99%
rename from board/xilinx/common/fru.c
rename to cmd/fru.c
index f6ca46c3cecc..dd0b56f05698 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[])
 {
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 93%
rename from board/xilinx/common/fru_ops.c
rename to common/fru_ops.c
index 49846ae3d660..0c5e264226ed 100644
--- a/board/xilinx/common/fru_ops.c
+++ b/common/fru_ops.c
@@ -9,7 +9,6 @@
 #include <fdtdec.h>
 #include <log.h>
 #include <malloc.h>
-#include <net.h>
 #include <asm/io.h>
 #include <asm/arch/hardware.h>
 
@@ -219,12 +218,11 @@ static int fru_parse_board(unsigned long addr)
 	return 0;
 }
 
-static int fru_parse_multirec(unsigned long addr)
+__weak 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;
+	u8 checksum;
 
 	debug("%s: multirec addr %lx\n", __func__, addr);
 
@@ -237,14 +235,9 @@ static int fru_parse_multirec(unsigned long addr)
 			return -EINVAL;
 		}
 
-		if (mrc.rec_type == FRU_MULTIREC_TYPE_OEM) {
-			struct fru_multirec_mac *mac = (void *)addr + hdr_len;
+		debug("%s: multirec rec_type: 0x%x, type: 0x%x, len: %d\n",
+		      __func__, mrc.rec_type, mrc.type, mrc.len);
 
-			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));
 
@@ -255,7 +248,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 +262,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;
 }
@@ -413,3 +401,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 85%
rename from board/xilinx/common/fru.h
rename to include/fru.h
index 59f6b722cf12..b497a8835695 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;
@@ -66,16 +64,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 +77,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
@@ -102,7 +90,6 @@ 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);
 u8 fru_checksum(u8 *addr, u8 len);
-
-extern struct fru_table fru_data;
+const struct fru_table *fru_get_fru_data(void);
 
 #endif /* FRU_H */
-- 
2.25.1

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

[Michal Simek]

You should fix subject.

On 7/27/22 01:50, 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 to be enabled by CONFIG_CMD_FRU.
> Since the Multirecord parsing logic should be implemented on each OEM
> board specifically, it defines 'fru_parse_multirec' as a weak function
> so that it can be replaced with the board specific implementation.

Not entirely. Some multirecords are common and described by spec. But parising 
multirecord based on IANA ID is vendor specific.
It means boards shouldn't replicate code for hearder checking. Only handle that 
IANA specific part.


> 
> Signed-off-by: Graeme Gregory <quic_ggregory@quicinc.com>
> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
> ---
>   board/xilinx/Kconfig                      |  8 ---
>   board/xilinx/common/Makefile              |  3 --
>   board/xilinx/common/board.c               | 63 +++++++++++++++++++----
>   cmd/Kconfig                               |  8 +++
>   cmd/Makefile                              |  1 +
>   {board/xilinx/common => cmd}/fru.c        |  3 +-
>   common/Makefile                           |  2 +
>   {board/xilinx/common => common}/fru_ops.c | 37 ++++++-------
>   {board/xilinx/common => include}/fru.h    | 15 +-----
>   9 files changed, 82 insertions(+), 58 deletions(-)
>   rename {board/xilinx/common => cmd}/fru.c (99%)
>   rename {board/xilinx/common => common}/fru_ops.c (93%)
>   rename {board/xilinx/common => include}/fru.h (85%)

The main reason why I didn't added to generic location was that in board field 
there are xilinx specific custom fields.
With other vendor this won't work.
I think this should be solved before this code can be moved to generic location.

Also maybe make sense to move and spec that variable creation.

Thanks,
Michal

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

[Jae Hyun Yoo]

Hello Michal,

On 7/29/2022 4:13 AM, Michal Simek wrote:
> You should fix subject.

Ah, I'll remove one of 'cmd/fru:' prefix in the title.

> On 7/27/22 01:50, 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 to be enabled by CONFIG_CMD_FRU.
>> Since the Multirecord parsing logic should be implemented on each OEM
>> board specifically, it defines 'fru_parse_multirec' as a weak function
>> so that it can be replaced with the board specific implementation.
> 
> Not entirely. Some multirecords are common and described by spec. But 
> parising multirecord based on IANA ID is vendor specific.
> It means boards shouldn't replicate code for hearder checking. Only 
> handle that IANA specific part.

Right. I'll change the multirecords parsing logic to call the vendor
specific parsing function only when record type is 0xc0 - 0xff. All
other standard type parsing logic and header checking will be placed
in the generic location.

>>
>> Signed-off-by: Graeme Gregory <quic_ggregory@quicinc.com>
>> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
>> ---
>>   board/xilinx/Kconfig                      |  8 ---
>>   board/xilinx/common/Makefile              |  3 --
>>   board/xilinx/common/board.c               | 63 +++++++++++++++++++----
>>   cmd/Kconfig                               |  8 +++
>>   cmd/Makefile                              |  1 +
>>   {board/xilinx/common => cmd}/fru.c        |  3 +-
>>   common/Makefile                           |  2 +
>>   {board/xilinx/common => common}/fru_ops.c | 37 ++++++-------
>>   {board/xilinx/common => include}/fru.h    | 15 +-----
>>   9 files changed, 82 insertions(+), 58 deletions(-)
>>   rename {board/xilinx/common => cmd}/fru.c (99%)
>>   rename {board/xilinx/common => common}/fru_ops.c (93%)
>>   rename {board/xilinx/common => include}/fru.h (85%)
> 
> The main reason why I didn't added to generic location was that in board 
> field there are xilinx specific custom fields.
> With other vendor this won't work.
> I think this should be solved before this code can be moved to generic 
> location.
> 
> Also maybe make sense to move and spec that variable creation.

Yes, I realized that the Xilinx specific customization was added into
the standard board info area so actually it breaks the spec.

struct fru_board_data {
         [....]

	/* 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];
};

I think, this type of customization should be added using multirecords
instead of expanding the common board info structure, and it's the
reason why the FRU spec provides OEM multirecord types (0xc0 - 0xff).
Do you have any plan to replace them with OEM multirecords?

Thanks,
Jae

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

[Jae Hyun Yoo]

Hello Michal,

On 7/29/2022 7:38 AM, Jae Hyun Yoo wrote:
> On 7/29/2022 4:13 AM, Michal Simek wrote:
>> The main reason why I didn't added to generic location was that in 
>> board field there are xilinx specific custom fields.
>> With other vendor this won't work.
>> I think this should be solved before this code can be moved to generic 
>> location.
>>
>> Also maybe make sense to move and spec that variable creation.
> 
> Yes, I realized that the Xilinx specific customization was added into
> the standard board info area so actually it breaks the spec.
> 
> struct fru_board_data {
>          [....]
> 
>      /* 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];
> };
> 
> I think, this type of customization should be added using multirecords
> instead of expanding the common board info structure, and it's the
> reason why the FRU spec provides OEM multirecord types (0xc0 - 0xff).
> Do you have any plan to replace them with OEM multirecords?

I reviewed the spec again and checked that adding additional info fields
is also acceptable by the spec. Let me try to refine the current code to
make it parse the additional custom info fields in a generic way.

Thanks,
Jae

> Thanks,
> Jae

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

[Michal Simek]

Hi,

On 7/29/22 17:00, Jae Hyun Yoo wrote:
> Hello Michal,
> 
> On 7/29/2022 7:38 AM, Jae Hyun Yoo wrote:
>> On 7/29/2022 4:13 AM, Michal Simek wrote:
>>> The main reason why I didn't added to generic location was that in board 
>>> field there are xilinx specific custom fields.
>>> With other vendor this won't work.
>>> I think this should be solved before this code can be moved to generic location.
>>>
>>> Also maybe make sense to move and spec that variable creation.
>>
>> Yes, I realized that the Xilinx specific customization was added into
>> the standard board info area so actually it breaks the spec.
>>
>> struct fru_board_data {
>>          [....]
>>
>>      /* 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];
>> };
>>
>> I think, this type of customization should be added using multirecords
>> instead of expanding the common board info structure, and it's the
>> reason why the FRU spec provides OEM multirecord types (0xc0 - 0xff).
>> Do you have any plan to replace them with OEM multirecords?
> 
> I reviewed the spec again and checked that adding additional info fields
> is also acceptable by the spec. Let me try to refine the current code to
> make it parse the additional custom info fields in a generic way.

Replied previous email before this one. Good that you also found it.

M

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

[Michal Simek]

Hi,

On 7/29/22 16:38, Jae Hyun Yoo wrote:
> Hello Michal,
> 
> On 7/29/2022 4:13 AM, Michal Simek wrote:
>> You should fix subject.
> 
> Ah, I'll remove one of 'cmd/fru:' prefix in the title.
> 
>> On 7/27/22 01:50, 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 to be enabled by CONFIG_CMD_FRU.
>>> Since the Multirecord parsing logic should be implemented on each OEM
>>> board specifically, it defines 'fru_parse_multirec' as a weak function
>>> so that it can be replaced with the board specific implementation.
>>
>> Not entirely. Some multirecords are common and described by spec. But parising 
>> multirecord based on IANA ID is vendor specific.
>> It means boards shouldn't replicate code for hearder checking. Only handle 
>> that IANA specific part.
> 
> Right. I'll change the multirecords parsing logic to call the vendor
> specific parsing function only when record type is 0xc0 - 0xff. All
> other standard type parsing logic and header checking will be placed
> in the generic location.
> 
>>>
>>> Signed-off-by: Graeme Gregory <quic_ggregory@quicinc.com>
>>> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
>>> ---
>>>   board/xilinx/Kconfig                      |  8 ---
>>>   board/xilinx/common/Makefile              |  3 --
>>>   board/xilinx/common/board.c               | 63 +++++++++++++++++++----
>>>   cmd/Kconfig                               |  8 +++
>>>   cmd/Makefile                              |  1 +
>>>   {board/xilinx/common => cmd}/fru.c        |  3 +-
>>>   common/Makefile                           |  2 +
>>>   {board/xilinx/common => common}/fru_ops.c | 37 ++++++-------
>>>   {board/xilinx/common => include}/fru.h    | 15 +-----
>>>   9 files changed, 82 insertions(+), 58 deletions(-)
>>>   rename {board/xilinx/common => cmd}/fru.c (99%)
>>>   rename {board/xilinx/common => common}/fru_ops.c (93%)
>>>   rename {board/xilinx/common => include}/fru.h (85%)
>>
>> The main reason why I didn't added to generic location was that in board field 
>> there are xilinx specific custom fields.
>> With other vendor this won't work.
>> I think this should be solved before this code can be moved to generic location.
>>
>> Also maybe make sense to move and spec that variable creation.
> 
> Yes, I realized that the Xilinx specific customization was added into
> the standard board info area so actually it breaks the spec.
> 
> struct fru_board_data {
>          [....]
> 
>      /* 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];
> };
> 
> I think, this type of customization should be added using multirecords
> instead of expanding the common board info structure, and it's the
> reason why the FRU spec provides OEM multirecord types (0xc0 - 0xff).
> Do you have any plan to replace them with OEM multirecords?

Based on
https://www.intel.com/content/dam/www/public/us/en/documents/product-briefs/platform-management-fru-document-rev-1-2-feb-2013.pdf

Table 11-1 has

Additional custom Mfg. Info fields. Defined by manufacturing. Each
field must be preceded by a type/length byte

is clearly defined that it can be custom. Only that fields to FRU are defined by 
the spec.

That's why based on manufacturer name these fields can be defined. And because 
we did it for Xilinx only we didn't need to design it in a generic way. It means 
if there are some additional fields which is quite clear from sizes special 
board manufacturing functions can be called but code change is required.

Thanks,
Michal

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

[Jae Hyun Yoo]

Add product info area parsing support.

Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
---
 common/fru_ops.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++-
 include/fru.h    |  22 ++++++++
 2 files changed, 155 insertions(+), 1 deletion(-)

diff --git a/common/fru_ops.c b/common/fru_ops.c
index 0c5e264226ed..93d4970620f4 100644
--- a/common/fru_ops.c
+++ b/common/fru_ops.c
@@ -218,6 +218,57 @@ static int fru_parse_board(unsigned long addr)
 	return 0;
 }
 
+static int fru_parse_product(unsigned long addr)
+{
+	u8 i, type;
+	int len;
+	u8 *data, *term, *limit;
+
+	memcpy(&fru_data.prd.ver, (void *)addr, 6);
+	addr += 3;
+	data = (u8 *)&fru_data.prd.manufacturer_type_len;
+
+	/* Record max structure limit not to write data over allocated space */
+	limit = (u8 *)&fru_data.prd + sizeof(struct fru_product_data);
+
+	for (i = 0; ; 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;
+	}
+
+	return 0;
+}
+
 __weak int fru_parse_multirec(unsigned long addr)
 {
 	u8 hdr_len = sizeof(struct fru_multirec_hdr);
@@ -265,6 +316,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));
 
@@ -352,6 +406,78 @@ 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 typecode[] = {
+		"Binary/Unspecified",
+		"BCD plus",
+		"6-bit ASCII",
+		"8-bit ASCII",
+		"2-byte UNICODE"
+	};
+	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", typecode[type]);
+		else
+			debug("Type code: %s\n", typecode[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)
@@ -392,6 +518,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;
@@ -399,7 +527,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 b497a8835695..adece622565f 100644
--- a/include/fru.h
+++ b/include/fru.h
@@ -56,6 +56,26 @@ struct fru_board_data {
 	u8 uuid[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;
@@ -67,6 +87,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;
 };
 
@@ -81,6 +102,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: [RFC PATCH 2/2] cmd/fru: add product info area parsing support

[Michal Simek]

On 7/27/22 01:50, Jae Hyun Yoo wrote:
> Add product info area parsing support.
> 
> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
> ---
>   common/fru_ops.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++-
>   include/fru.h    |  22 ++++++++
>   2 files changed, 155 insertions(+), 1 deletion(-)
> 
> diff --git a/common/fru_ops.c b/common/fru_ops.c
> index 0c5e264226ed..93d4970620f4 100644
> --- a/common/fru_ops.c
> +++ b/common/fru_ops.c
> @@ -218,6 +218,57 @@ static int fru_parse_board(unsigned long addr)
>   	return 0;
>   }
>   
> +static int fru_parse_product(unsigned long addr)
> +{
> +	u8 i, type;
> +	int len;
> +	u8 *data, *term, *limit;
> +
> +	memcpy(&fru_data.prd.ver, (void *)addr, 6);
> +	addr += 3;
> +	data = (u8 *)&fru_data.prd.manufacturer_type_len;
> +
> +	/* Record max structure limit not to write data over allocated space */
> +	limit = (u8 *)&fru_data.prd + sizeof(struct fru_product_data);
> +
> +	for (i = 0; ; 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;
> +	}
> +
> +	return 0;
> +}
> +
>   __weak int fru_parse_multirec(unsigned long addr)
>   {
>   	u8 hdr_len = sizeof(struct fru_multirec_hdr);
> @@ -265,6 +316,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));
>   
> @@ -352,6 +406,78 @@ 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 typecode[] = {
> +		"Binary/Unspecified",
> +		"BCD plus",
> +		"6-bit ASCII",
> +		"8-bit ASCII",
> +		"2-byte UNICODE"
> +	};

This should be generic for all records and should be shared.

Thanks,
Michal

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

[Jae Hyun Yoo]

Hello Michal,

On 7/29/2022 4:11 AM, Michal Simek wrote:
>> +    static const char * const typecode[] = {
>> +        "Binary/Unspecified",
>> +        "BCD plus",
>> +        "6-bit ASCII",
>> +        "8-bit ASCII",
>> +        "2-byte UNICODE"
>> +    };
> 
> This should be generic for all records and should be shared.

Right. I'll change it to a static string table in this module so that it
can be shared for all records.

Thanks,
Jae