[U-Boot] [PATCH 0/3] cmd: fru: Add simple FRU generator and parser

[U-Boot] [PATCH 0/3] cmd: fru: Add simple FRU generator and parser

[Michal Simek]

Hi,

this code was develop for i2c eeprom decoding which is the part of VITA
IPMI spec. This code is just deconding board information but it can be
extended to cover other fields and SoC extensions.

Thanks,
Michal


Michal Simek (2):
  cmd: fru: Add basic fru format generator
  arm64: versal: Enable FRU command

Siva Durga Prasad Paladugu (1):
  cmd: fru: Add support for FRU commands

 MAINTAINERS                          |   3 +
 cmd/Kconfig                          |   8 +
 cmd/Makefile                         |   1 +
 cmd/fru.c                            |  86 +++++++
 common/Makefile                      |   1 +
 common/fru_ops.c                     | 342 +++++++++++++++++++++++++++
 configs/xilinx_versal_virt_defconfig |   1 +
 include/fru.h                        |  78 ++++++
 8 files changed, 520 insertions(+)
 create mode 100644 cmd/fru.c
 create mode 100644 common/fru_ops.c
 create mode 100644 include/fru.h

-- 
2.17.1

[U-Boot] [PATCH 1/3] cmd: fru: Add support for FRU commands

[Michal Simek]

From: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>

This patch adds support for fru commands "fru capture" and "fru display".
The fru capture parses the FRU table present at an address and stores in a
structure for later use. The fru display prints the content of captured
structured in a readable format.

As of now, it supports only common header and board area of FRU. Also, it
supports only English language code and ASCII8 format.

fru_data variable is placed to data section because fru parser can be
called very early before bss is initialized. And also information needs to
be shared that's why it is exported via header.

Signed-off-by: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 MAINTAINERS      |   3 +
 cmd/Kconfig      |   8 ++
 cmd/Makefile     |   1 +
 cmd/fru.c        |  68 +++++++++++++
 common/Makefile  |   1 +
 common/fru_ops.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++
 include/fru.h    |  64 ++++++++++++
 7 files changed, 392 insertions(+)
 create mode 100644 cmd/fru.c
 create mode 100644 common/fru_ops.c
 create mode 100644 include/fru.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 2ef29768555c..420ce26cd0aa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -404,6 +404,9 @@ M:	Michal Simek <michal.simek@xilinx.com>
 S:	Maintained
 T:	git https://gitlab.denx.de/u-boot/custodians/u-boot-microblaze.git
 F:	arch/arm/mach-versal/
+F:	cmd/fru.c
+F:	common/fru_ops.c
+F:	include/fru.h
 N:	(?<!uni)versal
 
 ARM VERSATILE EXPRESS DRIVERS
diff --git a/cmd/Kconfig b/cmd/Kconfig
index 07060c63a7e6..68815e42641e 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1688,6 +1688,14 @@ config CMD_UUID
 	  The two commands are very similar except for the endianness of the
 	  output.
 
+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.
+
 endmenu
 
 source "cmd/ti/Kconfig"
diff --git a/cmd/Makefile b/cmd/Makefile
index ac843b4b16ad..f790217dd628 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -43,6 +43,7 @@ obj-$(CONFIG_DATAFLASH_MMC_SELECT) += dataflash_mmc_mux.o
 obj-$(CONFIG_CMD_DATE) += date.o
 obj-$(CONFIG_CMD_DEMO) += demo.o
 obj-$(CONFIG_CMD_DM) += dm.o
+obj-$(CONFIG_CMD_FRU) += fru.o
 obj-$(CONFIG_CMD_SOUND) += sound.o
 ifdef CONFIG_POST
 obj-$(CONFIG_CMD_DIAG) += diag.o
diff --git a/cmd/fru.c b/cmd/fru.c
new file mode 100644
index 000000000000..d5e96cfe8984
--- /dev/null
+++ b/cmd/fru.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * (C) Copyright 2019 Xilinx, Inc.
+ */
+
+#include <common.h>
+#include <fdtdec.h>
+#include <malloc.h>
+#include <fru.h>
+
+static int do_fru_capture(cmd_tbl_t *cmdtp, int flag, int argc,
+			  char *const argv[])
+{
+	unsigned long addr;
+	char *endp;
+
+	if (argc < cmdtp->maxargs)
+		return CMD_RET_USAGE;
+
+	addr = simple_strtoul(argv[2], &endp, 16);
+	if (*argv[1] == 0 || *endp != 0)
+		return -1;
+
+	return fru_capture(addr);
+}
+
+static int do_fru_display(cmd_tbl_t *cmdtp, int flag, int argc,
+			  char *const argv[])
+{
+	fru_display();
+	return CMD_RET_SUCCESS;
+}
+
+static cmd_tbl_t cmd_fru_sub[] = {
+	U_BOOT_CMD_MKENT(capture, 3, 0, do_fru_capture, "", ""),
+	U_BOOT_CMD_MKENT(display, 2, 0, do_fru_display, "", ""),
+};
+
+static int do_fru(cmd_tbl_t *cmdtp, int flag, int argc,
+		  char *const argv[])
+{
+	cmd_tbl_t *c;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	c = find_cmd_tbl(argv[1], &cmd_fru_sub[0],
+			 ARRAY_SIZE(cmd_fru_sub));
+	if (c)
+		return c->cmd(c, flag, argc, argv);
+
+	return CMD_RET_USAGE;
+}
+
+/***************************************************/
+#ifdef CONFIG_SYS_LONGHELP
+static char fru_help_text[] =
+	"capture <addr> - Parse and capture FRU table present at address.\n"
+	"fru display - Displays content of FRU table that was captured using\n"
+	"              fru capture command\n"
+	;
+#endif
+
+U_BOOT_CMD(
+	fru, 3, 1, do_fru,
+	"FRU table info",
+	fru_help_text
+)
diff --git a/common/Makefile b/common/Makefile
index 302d8beaf356..10cbe9a15ea1 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -57,6 +57,7 @@ obj-$(CONFIG_UPDATE_TFTP) += update.o
 obj-$(CONFIG_DFU_TFTP) += update.o
 obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o
 obj-$(CONFIG_CMDLINE) += cli_readline.o cli_simple.o
+obj-$(CONFIG_CMD_FRU) += fru_ops.o
 
 endif # !CONFIG_SPL_BUILD
 
diff --git a/common/fru_ops.c b/common/fru_ops.c
new file mode 100644
index 000000000000..dd54bd9c0214
--- /dev/null
+++ b/common/fru_ops.c
@@ -0,0 +1,247 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * (C) Copyright 2019 Xilinx, Inc.
+ */
+
+#include <common.h>
+#include <fdtdec.h>
+#include <fru.h>
+#include <malloc.h>
+#include <asm/io.h>
+#include <asm/arch/hardware.h>
+
+struct fru_table fru_data  __attribute__((section(".data")));
+
+static u16 fru_cal_area_len(u8 len)
+{
+	return len * FRU_COMMON_HDR_LEN_MULTIPLIER;
+}
+
+static u8 fru_version(u8 ver)
+{
+	return ver & FRU_COMMON_HDR_VER_MASK;
+}
+
+static int fru_check_language(u8 code)
+{
+	if (code != FRU_LANG_CODE_ENGLISH && code != FRU_LANG_CODE_ENGLISH_1) {
+		printf("FRU_ERROR: Only English Language is supported\n");
+		return -EINVAL;
+	}
+
+	return code;
+}
+
+static u8 fru_checksum(u8 *addr, u8 len)
+{
+	u8 checksum = 0;
+
+	while (len--) {
+		checksum += *addr;
+		addr++;
+	}
+
+	return checksum;
+}
+
+static int fru_check_type_len(u8 type_len, u8 language, u8 *type)
+{
+	int len;
+
+	if (type_len == FRU_TYPELEN_EOF)
+		return -EINVAL;
+
+	*type = (type_len & FRU_TYPELEN_CODE_MASK) >> FRU_TYPELEN_TYPE_SHIFT;
+
+	len = type_len & FRU_TYPELEN_LEN_MASK;
+
+	return len;
+}
+
+static int fru_parse_board(unsigned long addr)
+{
+	u8 i, type;
+	int len;
+	u8 *data, *term;
+
+	memcpy(&fru_data.brd.ver, (void *)addr, 6);
+	addr += 6;
+	data = (u8 *)&fru_data.brd.manufacturer_type_len;
+
+	for (i = 0; ; i++, data += FRU_BOARD_MAX_LEN) {
+		*data++ = *(u8 *)addr;
+		len = fru_check_type_len(*(u8 *)addr, fru_data.brd.lang_code,
+					 &type);
+		/*
+		 * Stop cature if it end of fields
+		 */
+		if (len == -EINVAL)
+			break;
+
+		/*
+		 * Dont capture data if type is not ASCII8
+		 */
+		if (type != FRU_TYPELEN_TYPE_ASCII8)
+			return 0;
+
+		addr += 1;
+		if (!len)
+			continue;
+		memcpy(data, (u8 *)addr, len);
+		term = data + (u8)len;
+		*term = 0;
+		addr += len;
+	}
+
+	if (i < FRU_BOARD_AREA_TOTAL_FIELDS) {
+		printf("Board area require minimum %d fields\n",
+		       FRU_BOARD_AREA_TOTAL_FIELDS);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+int fru_capture(unsigned long addr)
+{
+	struct fru_common_hdr *hdr;
+	u8 checksum = 0;
+
+	checksum = fru_checksum((u8 *)addr, sizeof(struct fru_common_hdr));
+	if (checksum) {
+		printf("%s Common header CRC error\n", __func__);
+		return -EINVAL;
+	}
+
+	hdr = (struct fru_common_hdr *)addr;
+
+	memcpy((void *)&fru_data.hdr, (void *)hdr,
+	       sizeof(struct fru_common_hdr));
+
+	fru_data.captured = true;
+
+	if (hdr->off_board) {
+		addr += fru_cal_area_len(hdr->off_board);
+		fru_parse_board(addr);
+	}
+
+	env_set_hex("fru_addr", addr);
+
+	return 0;
+}
+
+static int fru_display_board(void)
+{
+	u32 time = 0;
+	u8 type;
+	int len;
+	u8 *data;
+	const char *typecode[] = {
+		"Binary/Unspecified",
+		"BCD plus",
+		"6-bit ASCII",
+		"8-bit ASCII",
+		"2-byte UNICODE"
+	};
+	const char *boardinfo[] = {
+		"Manufacturer Name",
+		"Product Name",
+		"Serial No",
+		"Part Number",
+		"File ID"
+	};
+
+	printf("*****BOARD INFO*****\n");
+	printf("Version:%d\n", fru_version(fru_data.brd.ver));
+	printf("Board Area Length:%d\n", fru_cal_area_len(fru_data.brd.len));
+
+	if (fru_check_language(fru_data.brd.lang_code))
+		return 0;
+
+	time = fru_data.brd.time[2] << 16 | fru_data.brd.time[1] << 8 |
+	       fru_data.brd.time[0];
+	printf("Time in Minutes from 0:00hrs 1/1/96 %d\n", time);
+
+	data = (u8 *)&fru_data.brd.manufacturer_type_len;
+
+	for (u8 i = 0; ; i++) {
+		len = fru_check_type_len(*data++, fru_data.brd.lang_code,
+					 &type);
+		if (len == -EINVAL) {
+			printf("**** EOF for Board Area ****\n");
+			break;
+		}
+
+		if (type <= FRU_TYPELEN_TYPE_ASCII8 &&
+		    (fru_data.brd.lang_code == FRU_LANG_CODE_ENGLISH ||
+		     fru_data.brd.lang_code == FRU_LANG_CODE_ENGLISH_1))
+			printf("Type code: %s\n", typecode[type]);
+		else
+			printf("Type code: %s\n", typecode[type + 1]);
+
+		if (type != FRU_TYPELEN_TYPE_ASCII8) {
+			printf("FRU_ERROR: Only ASCII8 type is supported\n");
+			return 0;
+		}
+		if (!len) {
+			printf("%s not found\n", boardinfo[i]);
+			continue;
+		}
+
+		printf("Length: %d\n", len);
+		printf("%s: %s\n", boardinfo[i], data);
+		data += FRU_BOARD_MAX_LEN;
+	}
+
+	return 0;
+}
+
+static void fru_display_common_hdr(void)
+{
+	struct fru_common_hdr *hdr = &fru_data.hdr;
+
+	printf("*****COMMON HEADER*****\n");
+	printf("Version:%d\n", fru_version(hdr->version));
+	if (hdr->off_internal)
+		printf("Internal Use Area Offset:%d\n",
+		       fru_cal_area_len(hdr->off_internal));
+	else
+		printf("*** No Internal Area ***\n");
+
+	if (hdr->off_chassis)
+		printf("Chassis Info Area Offset:%d\n",
+		       fru_cal_area_len(hdr->off_chassis));
+	else
+		printf("*** No Chassis Info Area ***\n");
+
+	if (hdr->off_board)
+		printf("Board Area Offset:%d\n",
+		       fru_cal_area_len(hdr->off_board));
+	else
+		printf("*** No Board Area ***\n");
+
+	if (hdr->off_product)
+		printf("Product Info Area Offset:%d\n",
+		       fru_cal_area_len(hdr->off_product));
+	else
+		printf("*** No Product Info Area ***\n");
+
+	if (hdr->off_multirec)
+		printf("MultiRecord Area Offset:%d\n",
+		       fru_cal_area_len(hdr->off_multirec));
+	else
+		printf("*** No MultiRecord Area ***\n");
+}
+
+int fru_display(void)
+{
+	if (!fru_data.captured) {
+		printf("FRU data not available please run fru parse\n");
+		return -EINVAL;
+	}
+
+	fru_display_common_hdr();
+	fru_display_board();
+
+	return 0;
+}
diff --git a/include/fru.h b/include/fru.h
new file mode 100644
index 000000000000..38fdfad409c4
--- /dev/null
+++ b/include/fru.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * (C) Copyright 2019 Xilinx, Inc.
+ * Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
+ */
+
+#ifndef __FRU_H
+#define __FRU_H
+
+struct fru_common_hdr {
+	u8 version;
+	u8 off_internal;
+	u8 off_chassis;
+	u8 off_board;
+	u8 off_product;
+	u8 off_multirec;
+	u8 pad;
+	u8 crc;
+};
+
+#define FRU_BOARD_MAX_LEN	32
+
+struct fru_board_data {
+	u8 ver;
+	u8 len;
+	u8 lang_code;
+	u8 time[3];
+	u8 manufacturer_type_len;
+	u8 manufacturer_name[FRU_BOARD_MAX_LEN];
+	u8 product_name_type_len;
+	u8 product_name[FRU_BOARD_MAX_LEN];
+	u8 serial_number_type_len;
+	u8 serial_number[FRU_BOARD_MAX_LEN];
+	u8 part_number_type_len;
+	u8 part_number[FRU_BOARD_MAX_LEN];
+	u8 file_id_type_len;
+	u8 file_id[FRU_BOARD_MAX_LEN];
+};
+
+struct fru_table {
+	bool captured;
+	struct fru_common_hdr hdr;
+	struct fru_board_data brd;
+};
+
+#define FRU_TYPELEN_CODE_MASK	0xC0
+#define FRU_TYPELEN_LEN_MASK	0x3F
+#define FRU_COMMON_HDR_VER_MASK		0xF
+#define FRU_COMMON_HDR_LEN_MULTIPLIER	8
+#define FRU_LANG_CODE_ENGLISH		0
+#define FRU_LANG_CODE_ENGLISH_1		25
+#define FRU_TYPELEN_EOF			0xC1
+
+/* This should be minimum of fields */
+#define FRU_BOARD_AREA_TOTAL_FIELDS	5
+#define FRU_TYPELEN_TYPE_SHIFT		6
+#define FRU_TYPELEN_TYPE_ASCII8		3
+
+int fru_display(void);
+int fru_capture(unsigned long addr);
+
+extern struct fru_table fru_data;
+
+#endif /* FRU_H */
-- 
2.17.1

[U-Boot] [PATCH 1/3] cmd: fru: Add support for FRU commands

[Simon Glass]

Hi Michal,

On Mon, 14 Oct 2019 at 07:29, Michal Simek <michal.simek@xilinx.com> wrote:
>
> From: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
>
> This patch adds support for fru commands "fru capture" and "fru display".
> The fru capture parses the FRU table present at an address and stores in a
> structure for later use. The fru display prints the content of captured
> structured in a readable format.
>
> As of now, it supports only common header and board area of FRU. Also, it
> supports only English language code and ASCII8 format.
>
> fru_data variable is placed to data section because fru parser can be
> called very early before bss is initialized. And also information needs to
> be shared that's why it is exported via header.
>
> Signed-off-by: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
>  MAINTAINERS      |   3 +
>  cmd/Kconfig      |   8 ++
>  cmd/Makefile     |   1 +
>  cmd/fru.c        |  68 +++++++++++++
>  common/Makefile  |   1 +
>  common/fru_ops.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++

Can we get some tests for this file?

>  include/fru.h    |  64 ++++++++++++
>  7 files changed, 392 insertions(+)
>  create mode 100644 cmd/fru.c
>  create mode 100644 common/fru_ops.c
>  create mode 100644 include/fru.h
>

Regards,
Simon

[U-Boot] [PATCH 1/3] cmd: fru: Add support for FRU commands

[Michal Simek]

On 22. 10. 19 1:46, Simon Glass wrote:
> Hi Michal,
> 
> On Mon, 14 Oct 2019 at 07:29, Michal Simek <michal.simek@xilinx.com> wrote:
>>
>> From: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
>>
>> This patch adds support for fru commands "fru capture" and "fru display".
>> The fru capture parses the FRU table present at an address and stores in a
>> structure for later use. The fru display prints the content of captured
>> structured in a readable format.
>>
>> As of now, it supports only common header and board area of FRU. Also, it
>> supports only English language code and ASCII8 format.
>>
>> fru_data variable is placed to data section because fru parser can be
>> called very early before bss is initialized. And also information needs to
>> be shared that's why it is exported via header.
>>
>> Signed-off-by: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>>  MAINTAINERS      |   3 +
>>  cmd/Kconfig      |   8 ++
>>  cmd/Makefile     |   1 +
>>  cmd/fru.c        |  68 +++++++++++++
>>  common/Makefile  |   1 +
>>  common/fru_ops.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++
> 
> Can we get some tests for this file?

We can generate one fragment, capture it and display it back. That
should cover it.
What way do you suggest to use? File/testing command?

Thanks,
Michal

[U-Boot] [PATCH 1/3] cmd: fru: Add support for FRU commands

[Jean-Jacques Hiblot]

On 14/10/2019 15:29, Michal Simek wrote:
> From: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
>
> This patch adds support for fru commands "fru capture" and "fru display".
> The fru capture parses the FRU table present at an address and stores in a
> structure for later use. The fru display prints the content of captured
> structured in a readable format.
>
> As of now, it supports only common header and board area of FRU. Also, it
> supports only English language code and ASCII8 format.
>
> fru_data variable is placed to data section because fru parser can be
> called very early before bss is initialized. And also information needs to
> be shared that's why it is exported via header.
>
> Signed-off-by: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
>   MAINTAINERS      |   3 +
>   cmd/Kconfig      |   8 ++
>   cmd/Makefile     |   1 +
>   cmd/fru.c        |  68 +++++++++++++
>   common/Makefile  |   1 +
>   common/fru_ops.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++
>   include/fru.h    |  64 ++++++++++++
>   7 files changed, 392 insertions(+)
>   create mode 100644 cmd/fru.c
>   create mode 100644 common/fru_ops.c
>   create mode 100644 include/fru.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2ef29768555c..420ce26cd0aa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -404,6 +404,9 @@ M:	Michal Simek <michal.simek@xilinx.com>
>   S:	Maintained
>   T:	git https://gitlab.denx.de/u-boot/custodians/u-boot-microblaze.git
>   F:	arch/arm/mach-versal/
> +F:	cmd/fru.c
> +F:	common/fru_ops.c
> +F:	include/fru.h
>   N:	(?<!uni)versal
>   
>   ARM VERSATILE EXPRESS DRIVERS
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 07060c63a7e6..68815e42641e 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1688,6 +1688,14 @@ config CMD_UUID
>   	  The two commands are very similar except for the endianness of the
>   	  output.
>   
> +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.
> +
>   endmenu
>   
>   source "cmd/ti/Kconfig"
> diff --git a/cmd/Makefile b/cmd/Makefile
> index ac843b4b16ad..f790217dd628 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -43,6 +43,7 @@ obj-$(CONFIG_DATAFLASH_MMC_SELECT) += dataflash_mmc_mux.o
>   obj-$(CONFIG_CMD_DATE) += date.o
>   obj-$(CONFIG_CMD_DEMO) += demo.o
>   obj-$(CONFIG_CMD_DM) += dm.o
> +obj-$(CONFIG_CMD_FRU) += fru.o
>   obj-$(CONFIG_CMD_SOUND) += sound.o
>   ifdef CONFIG_POST
>   obj-$(CONFIG_CMD_DIAG) += diag.o
> diff --git a/cmd/fru.c b/cmd/fru.c
> new file mode 100644
> index 000000000000..d5e96cfe8984
> --- /dev/null
> +++ b/cmd/fru.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * (C) Copyright 2019 Xilinx, Inc.
> + */
> +
> +#include <common.h>
> +#include <fdtdec.h>
> +#include <malloc.h>
> +#include <fru.h>
> +
> +static int do_fru_capture(cmd_tbl_t *cmdtp, int flag, int argc,
> +			  char *const argv[])
> +{
> +	unsigned long addr;
> +	char *endp;
> +
> +	if (argc < cmdtp->maxargs)
> +		return CMD_RET_USAGE;
> +
> +	addr = simple_strtoul(argv[2], &endp, 16);
> +	if (*argv[1] == 0 || *endp != 0)
> +		return -1;
> +
> +	return fru_capture(addr);
> +}
> +
> +static int do_fru_display(cmd_tbl_t *cmdtp, int flag, int argc,
> +			  char *const argv[])
> +{
> +	fru_display();
> +	return CMD_RET_SUCCESS;
> +}
> +
> +static cmd_tbl_t cmd_fru_sub[] = {
> +	U_BOOT_CMD_MKENT(capture, 3, 0, do_fru_capture, "", ""),
> +	U_BOOT_CMD_MKENT(display, 2, 0, do_fru_display, "", ""),
> +};

Why not do the capture in "fru display" itself ? And maybe provide the 
ability to display a FRU read from an EEPROM?

> +
> +static int do_fru(cmd_tbl_t *cmdtp, int flag, int argc,
> +		  char *const argv[])
> +{
> +	cmd_tbl_t *c;
> +
> +	if (argc < 2)
> +		return CMD_RET_USAGE;
> +
> +	c = find_cmd_tbl(argv[1], &cmd_fru_sub[0],
> +			 ARRAY_SIZE(cmd_fru_sub));
> +	if (c)
> +		return c->cmd(c, flag, argc, argv);
> +
> +	return CMD_RET_USAGE;
> +}
> +
> +/***************************************************/
> +#ifdef CONFIG_SYS_LONGHELP
> +static char fru_help_text[] =
> +	"capture <addr> - Parse and capture FRU table present at address.\n"
> +	"fru display - Displays content of FRU table that was captured using\n"
> +	"              fru capture command\n"
> +	;
> +#endif
> +
> +U_BOOT_CMD(
> +	fru, 3, 1, do_fru,
> +	"FRU table info",
> +	fru_help_text
> +)
> diff --git a/common/Makefile b/common/Makefile
> index 302d8beaf356..10cbe9a15ea1 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -57,6 +57,7 @@ obj-$(CONFIG_UPDATE_TFTP) += update.o
>   obj-$(CONFIG_DFU_TFTP) += update.o
>   obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o
>   obj-$(CONFIG_CMDLINE) += cli_readline.o cli_simple.o
> +obj-$(CONFIG_CMD_FRU) += fru_ops.o
>   
>   endif # !CONFIG_SPL_BUILD
>   
> diff --git a/common/fru_ops.c b/common/fru_ops.c
> new file mode 100644
> index 000000000000..dd54bd9c0214
> --- /dev/null
> +++ b/common/fru_ops.c
> @@ -0,0 +1,247 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * (C) Copyright 2019 Xilinx, Inc.
> + */
> +
> +#include <common.h>
> +#include <fdtdec.h>
> +#include <fru.h>
> +#include <malloc.h>
> +#include <asm/io.h>
> +#include <asm/arch/hardware.h>
> +
> +struct fru_table fru_data  __attribute__((section(".data")));

Isn't it restrictive to have only a single instance of struct fru_table 
? Could there be multiple ?

JJ

> +
> +static u16 fru_cal_area_len(u8 len)
> +{
> +	return len * FRU_COMMON_HDR_LEN_MULTIPLIER;
> +}
> +
> +static u8 fru_version(u8 ver)
> +{
> +	return ver & FRU_COMMON_HDR_VER_MASK;
> +}
> +
> +static int fru_check_language(u8 code)
> +{
> +	if (code != FRU_LANG_CODE_ENGLISH && code != FRU_LANG_CODE_ENGLISH_1) {
> +		printf("FRU_ERROR: Only English Language is supported\n");
> +		return -EINVAL;
> +	}
> +
> +	return code;
> +}
> +
> +static u8 fru_checksum(u8 *addr, u8 len)
> +{
> +	u8 checksum = 0;
> +
> +	while (len--) {
> +		checksum += *addr;
> +		addr++;
> +	}
> +
> +	return checksum;
> +}
> +
> +static int fru_check_type_len(u8 type_len, u8 language, u8 *type)
> +{
> +	int len;
> +
> +	if (type_len == FRU_TYPELEN_EOF)
> +		return -EINVAL;
> +
> +	*type = (type_len & FRU_TYPELEN_CODE_MASK) >> FRU_TYPELEN_TYPE_SHIFT;
> +
> +	len = type_len & FRU_TYPELEN_LEN_MASK;
> +
> +	return len;
> +}
> +
> +static int fru_parse_board(unsigned long addr)
> +{
> +	u8 i, type;
> +	int len;
> +	u8 *data, *term;
> +
> +	memcpy(&fru_data.brd.ver, (void *)addr, 6);
> +	addr += 6;
> +	data = (u8 *)&fru_data.brd.manufacturer_type_len;
> +
> +	for (i = 0; ; i++, data += FRU_BOARD_MAX_LEN) {
> +		*data++ = *(u8 *)addr;
> +		len = fru_check_type_len(*(u8 *)addr, fru_data.brd.lang_code,
> +					 &type);
> +		/*
> +		 * Stop cature if it end of fields
> +		 */
> +		if (len == -EINVAL)
> +			break;
> +
> +		/*
> +		 * Dont capture data if type is not ASCII8
> +		 */
> +		if (type != FRU_TYPELEN_TYPE_ASCII8)
> +			return 0;
> +
> +		addr += 1;
> +		if (!len)
> +			continue;
> +		memcpy(data, (u8 *)addr, len);
> +		term = data + (u8)len;
> +		*term = 0;
> +		addr += len;
> +	}
> +
> +	if (i < FRU_BOARD_AREA_TOTAL_FIELDS) {
> +		printf("Board area require minimum %d fields\n",
> +		       FRU_BOARD_AREA_TOTAL_FIELDS);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +int fru_capture(unsigned long addr)
> +{
> +	struct fru_common_hdr *hdr;
> +	u8 checksum = 0;
> +
> +	checksum = fru_checksum((u8 *)addr, sizeof(struct fru_common_hdr));
> +	if (checksum) {
> +		printf("%s Common header CRC error\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	hdr = (struct fru_common_hdr *)addr;
> +
> +	memcpy((void *)&fru_data.hdr, (void *)hdr,
> +	       sizeof(struct fru_common_hdr));
> +
> +	fru_data.captured = true;
> +
> +	if (hdr->off_board) {
> +		addr += fru_cal_area_len(hdr->off_board);
> +		fru_parse_board(addr);
> +	}
> +
> +	env_set_hex("fru_addr", addr);
> +
> +	return 0;
> +}
> +
> +static int fru_display_board(void)
> +{
> +	u32 time = 0;
> +	u8 type;
> +	int len;
> +	u8 *data;
> +	const char *typecode[] = {
> +		"Binary/Unspecified",
> +		"BCD plus",
> +		"6-bit ASCII",
> +		"8-bit ASCII",
> +		"2-byte UNICODE"
> +	};
> +	const char *boardinfo[] = {
> +		"Manufacturer Name",
> +		"Product Name",
> +		"Serial No",
> +		"Part Number",
> +		"File ID"
> +	};
> +
> +	printf("*****BOARD INFO*****\n");
> +	printf("Version:%d\n", fru_version(fru_data.brd.ver));
> +	printf("Board Area Length:%d\n", fru_cal_area_len(fru_data.brd.len));
> +
> +	if (fru_check_language(fru_data.brd.lang_code))
> +		return 0;
> +
> +	time = fru_data.brd.time[2] << 16 | fru_data.brd.time[1] << 8 |
> +	       fru_data.brd.time[0];
> +	printf("Time in Minutes from 0:00hrs 1/1/96 %d\n", time);
> +
> +	data = (u8 *)&fru_data.brd.manufacturer_type_len;
> +
> +	for (u8 i = 0; ; i++) {
> +		len = fru_check_type_len(*data++, fru_data.brd.lang_code,
> +					 &type);
> +		if (len == -EINVAL) {
> +			printf("**** EOF for Board Area ****\n");
> +			break;
> +		}
> +
> +		if (type <= FRU_TYPELEN_TYPE_ASCII8 &&
> +		    (fru_data.brd.lang_code == FRU_LANG_CODE_ENGLISH ||
> +		     fru_data.brd.lang_code == FRU_LANG_CODE_ENGLISH_1))
> +			printf("Type code: %s\n", typecode[type]);
> +		else
> +			printf("Type code: %s\n", typecode[type + 1]);
> +
> +		if (type != FRU_TYPELEN_TYPE_ASCII8) {
> +			printf("FRU_ERROR: Only ASCII8 type is supported\n");
> +			return 0;
> +		}
> +		if (!len) {
> +			printf("%s not found\n", boardinfo[i]);
> +			continue;
> +		}
> +
> +		printf("Length: %d\n", len);
> +		printf("%s: %s\n", boardinfo[i], data);
> +		data += FRU_BOARD_MAX_LEN;
> +	}
> +
> +	return 0;
> +}
> +
> +static void fru_display_common_hdr(void)
> +{
> +	struct fru_common_hdr *hdr = &fru_data.hdr;
> +
> +	printf("*****COMMON HEADER*****\n");
> +	printf("Version:%d\n", fru_version(hdr->version));
> +	if (hdr->off_internal)
> +		printf("Internal Use Area Offset:%d\n",
> +		       fru_cal_area_len(hdr->off_internal));
> +	else
> +		printf("*** No Internal Area ***\n");
> +
> +	if (hdr->off_chassis)
> +		printf("Chassis Info Area Offset:%d\n",
> +		       fru_cal_area_len(hdr->off_chassis));
> +	else
> +		printf("*** No Chassis Info Area ***\n");
> +
> +	if (hdr->off_board)
> +		printf("Board Area Offset:%d\n",
> +		       fru_cal_area_len(hdr->off_board));
> +	else
> +		printf("*** No Board Area ***\n");
> +
> +	if (hdr->off_product)
> +		printf("Product Info Area Offset:%d\n",
> +		       fru_cal_area_len(hdr->off_product));
> +	else
> +		printf("*** No Product Info Area ***\n");
> +
> +	if (hdr->off_multirec)
> +		printf("MultiRecord Area Offset:%d\n",
> +		       fru_cal_area_len(hdr->off_multirec));
> +	else
> +		printf("*** No MultiRecord Area ***\n");
> +}
> +
> +int fru_display(void)
> +{
> +	if (!fru_data.captured) {
> +		printf("FRU data not available please run fru parse\n");
> +		return -EINVAL;
> +	}
> +
> +	fru_display_common_hdr();
> +	fru_display_board();
> +
> +	return 0;
> +}
> diff --git a/include/fru.h b/include/fru.h
> new file mode 100644
> index 000000000000..38fdfad409c4
> --- /dev/null
> +++ b/include/fru.h
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * (C) Copyright 2019 Xilinx, Inc.
> + * Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
> + */
> +
> +#ifndef __FRU_H
> +#define __FRU_H
> +
> +struct fru_common_hdr {
> +	u8 version;
> +	u8 off_internal;
> +	u8 off_chassis;
> +	u8 off_board;
> +	u8 off_product;
> +	u8 off_multirec;
> +	u8 pad;
> +	u8 crc;
> +};
> +
> +#define FRU_BOARD_MAX_LEN	32
> +
> +struct fru_board_data {
> +	u8 ver;
> +	u8 len;
> +	u8 lang_code;
> +	u8 time[3];
> +	u8 manufacturer_type_len;
> +	u8 manufacturer_name[FRU_BOARD_MAX_LEN];
> +	u8 product_name_type_len;
> +	u8 product_name[FRU_BOARD_MAX_LEN];
> +	u8 serial_number_type_len;
> +	u8 serial_number[FRU_BOARD_MAX_LEN];
> +	u8 part_number_type_len;
> +	u8 part_number[FRU_BOARD_MAX_LEN];
> +	u8 file_id_type_len;
> +	u8 file_id[FRU_BOARD_MAX_LEN];
> +};
> +
> +struct fru_table {
> +	bool captured;
> +	struct fru_common_hdr hdr;
> +	struct fru_board_data brd;
> +};
> +
> +#define FRU_TYPELEN_CODE_MASK	0xC0
> +#define FRU_TYPELEN_LEN_MASK	0x3F
> +#define FRU_COMMON_HDR_VER_MASK		0xF
> +#define FRU_COMMON_HDR_LEN_MULTIPLIER	8
> +#define FRU_LANG_CODE_ENGLISH		0
> +#define FRU_LANG_CODE_ENGLISH_1		25
> +#define FRU_TYPELEN_EOF			0xC1
> +
> +/* This should be minimum of fields */
> +#define FRU_BOARD_AREA_TOTAL_FIELDS	5
> +#define FRU_TYPELEN_TYPE_SHIFT		6
> +#define FRU_TYPELEN_TYPE_ASCII8		3
> +
> +int fru_display(void);
> +int fru_capture(unsigned long addr);
> +
> +extern struct fru_table fru_data;
> +
> +#endif /* FRU_H */

[U-Boot] [PATCH 1/3] cmd: fru: Add support for FRU commands

[Michal Simek]

On 22. 10. 19 14:40, Jean-Jacques Hiblot wrote:
> 
> On 14/10/2019 15:29, Michal Simek wrote:
>> From: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
>>
>> This patch adds support for fru commands "fru capture" and "fru display".
>> The fru capture parses the FRU table present at an address and stores
>> in a
>> structure for later use. The fru display prints the content of captured
>> structured in a readable format.
>>
>> As of now, it supports only common header and board area of FRU. Also, it
>> supports only English language code and ASCII8 format.
>>
>> fru_data variable is placed to data section because fru parser can be
>> called very early before bss is initialized. And also information
>> needs to
>> be shared that's why it is exported via header.
>>
>> Signed-off-by: Siva Durga Prasad Paladugu
>> <siva.durga.paladugu@xilinx.com>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>>   MAINTAINERS      |   3 +
>>   cmd/Kconfig      |   8 ++
>>   cmd/Makefile     |   1 +
>>   cmd/fru.c        |  68 +++++++++++++
>>   common/Makefile  |   1 +
>>   common/fru_ops.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++
>>   include/fru.h    |  64 ++++++++++++
>>   7 files changed, 392 insertions(+)
>>   create mode 100644 cmd/fru.c
>>   create mode 100644 common/fru_ops.c
>>   create mode 100644 include/fru.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 2ef29768555c..420ce26cd0aa 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -404,6 +404,9 @@ M:    Michal Simek <michal.simek@xilinx.com>
>>   S:    Maintained
>>   T:    git
>> https://gitlab.denx.de/u-boot/custodians/u-boot-microblaze.git
>>   F:    arch/arm/mach-versal/
>> +F:    cmd/fru.c
>> +F:    common/fru_ops.c
>> +F:    include/fru.h
>>   N:    (?<!uni)versal
>>     ARM VERSATILE EXPRESS DRIVERS
>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>> index 07060c63a7e6..68815e42641e 100644
>> --- a/cmd/Kconfig
>> +++ b/cmd/Kconfig
>> @@ -1688,6 +1688,14 @@ config CMD_UUID
>>         The two commands are very similar except for the endianness of
>> the
>>         output.
>>   +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.
>> +
>>   endmenu
>>     source "cmd/ti/Kconfig"
>> diff --git a/cmd/Makefile b/cmd/Makefile
>> index ac843b4b16ad..f790217dd628 100644
>> --- a/cmd/Makefile
>> +++ b/cmd/Makefile
>> @@ -43,6 +43,7 @@ obj-$(CONFIG_DATAFLASH_MMC_SELECT) +=
>> dataflash_mmc_mux.o
>>   obj-$(CONFIG_CMD_DATE) += date.o
>>   obj-$(CONFIG_CMD_DEMO) += demo.o
>>   obj-$(CONFIG_CMD_DM) += dm.o
>> +obj-$(CONFIG_CMD_FRU) += fru.o
>>   obj-$(CONFIG_CMD_SOUND) += sound.o
>>   ifdef CONFIG_POST
>>   obj-$(CONFIG_CMD_DIAG) += diag.o
>> diff --git a/cmd/fru.c b/cmd/fru.c
>> new file mode 100644
>> index 000000000000..d5e96cfe8984
>> --- /dev/null
>> +++ b/cmd/fru.c
>> @@ -0,0 +1,68 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * (C) Copyright 2019 Xilinx, Inc.
>> + */
>> +
>> +#include <common.h>
>> +#include <fdtdec.h>
>> +#include <malloc.h>
>> +#include <fru.h>
>> +
>> +static int do_fru_capture(cmd_tbl_t *cmdtp, int flag, int argc,
>> +              char *const argv[])
>> +{
>> +    unsigned long addr;
>> +    char *endp;
>> +
>> +    if (argc < cmdtp->maxargs)
>> +        return CMD_RET_USAGE;
>> +
>> +    addr = simple_strtoul(argv[2], &endp, 16);
>> +    if (*argv[1] == 0 || *endp != 0)
>> +        return -1;
>> +
>> +    return fru_capture(addr);
>> +}
>> +
>> +static int do_fru_display(cmd_tbl_t *cmdtp, int flag, int argc,
>> +              char *const argv[])
>> +{
>> +    fru_display();
>> +    return CMD_RET_SUCCESS;
>> +}
>> +
>> +static cmd_tbl_t cmd_fru_sub[] = {
>> +    U_BOOT_CMD_MKENT(capture, 3, 0, do_fru_capture, "", ""),
>> +    U_BOOT_CMD_MKENT(display, 2, 0, do_fru_display, "", ""),
>> +};
> 
> Why not do the capture in "fru display" itself ? And maybe provide the
> ability to display a FRU read from an EEPROM?

Sure we can do it but I think there are two separate things. Especially
if you don't need to show content.


>> +
>> +static int do_fru(cmd_tbl_t *cmdtp, int flag, int argc,
>> +          char *const argv[])
>> +{
>> +    cmd_tbl_t *c;
>> +
>> +    if (argc < 2)
>> +        return CMD_RET_USAGE;
>> +
>> +    c = find_cmd_tbl(argv[1], &cmd_fru_sub[0],
>> +             ARRAY_SIZE(cmd_fru_sub));
>> +    if (c)
>> +        return c->cmd(c, flag, argc, argv);
>> +
>> +    return CMD_RET_USAGE;
>> +}
>> +
>> +/***************************************************/
>> +#ifdef CONFIG_SYS_LONGHELP
>> +static char fru_help_text[] =
>> +    "capture <addr> - Parse and capture FRU table present at address.\n"
>> +    "fru display - Displays content of FRU table that was captured
>> using\n"
>> +    "              fru capture command\n"
>> +    ;
>> +#endif
>> +
>> +U_BOOT_CMD(
>> +    fru, 3, 1, do_fru,
>> +    "FRU table info",
>> +    fru_help_text
>> +)
>> diff --git a/common/Makefile b/common/Makefile
>> index 302d8beaf356..10cbe9a15ea1 100644
>> --- a/common/Makefile
>> +++ b/common/Makefile
>> @@ -57,6 +57,7 @@ obj-$(CONFIG_UPDATE_TFTP) += update.o
>>   obj-$(CONFIG_DFU_TFTP) += update.o
>>   obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o
>>   obj-$(CONFIG_CMDLINE) += cli_readline.o cli_simple.o
>> +obj-$(CONFIG_CMD_FRU) += fru_ops.o
>>     endif # !CONFIG_SPL_BUILD
>>   diff --git a/common/fru_ops.c b/common/fru_ops.c
>> new file mode 100644
>> index 000000000000..dd54bd9c0214
>> --- /dev/null
>> +++ b/common/fru_ops.c
>> @@ -0,0 +1,247 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * (C) Copyright 2019 Xilinx, Inc.
>> + */
>> +
>> +#include <common.h>
>> +#include <fdtdec.h>
>> +#include <fru.h>
>> +#include <malloc.h>
>> +#include <asm/io.h>
>> +#include <asm/arch/hardware.h>
>> +
>> +struct fru_table fru_data  __attribute__((section(".data")));
> 
> Isn't it restrictive to have only a single instance of struct fru_table
> ? Could there be multiple ?

You can have multiple FRUs but only one which is valid at that time and
which also fru_addr is pointing to.

Thanks,
Michal

[U-Boot] [PATCH 1/3] cmd: fru: Add support for FRU commands

[Michael Walle]

Am 2019-10-14 15:29, schrieb Michal Simek:
> From: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
> 
> This patch adds support for fru commands "fru capture" and "fru 
> display".
> The fru capture parses the FRU table present at an address and stores 
> in a
> structure for later use. The fru display prints the content of captured
> structured in a readable format.
> 
> As of now, it supports only common header and board area of FRU. Also, 
> it
> supports only English language code and ASCII8 format.
> 
> fru_data variable is placed to data section because fru parser can be
> called very early before bss is initialized. And also information needs 
> to
> be shared that's why it is exported via header.

Wouldn't it make more sense to have a level of indirection so other 
"fru" formats might be supported as well. As far as I can see, only 
"your" type of FRU data is supported by this command and there is now 
way to extend it.

Also why do the user have to manually do a "fru capture"? The use case 
is to display any FRU data, correct? So from a users perspective a "fru 
display" be sufficient to display the data.

-michael

[U-Boot] [PATCH 1/3] cmd: fru: Add support for FRU commands

[Michal Simek]

On 22. 10. 19 15:09, Michael Walle wrote:
> Am 2019-10-14 15:29, schrieb Michal Simek:
>> From: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
>>
>> This patch adds support for fru commands "fru capture" and "fru display".
>> The fru capture parses the FRU table present at an address and stores
>> in a
>> structure for later use. The fru display prints the content of captured
>> structured in a readable format.
>>
>> As of now, it supports only common header and board area of FRU. Also, it
>> supports only English language code and ASCII8 format.
>>
>> fru_data variable is placed to data section because fru parser can be
>> called very early before bss is initialized. And also information
>> needs to
>> be shared that's why it is exported via header.
> 
> Wouldn't it make more sense to have a level of indirection so other
> "fru" formats might be supported as well. As far as I can see, only
> "your" type of FRU data is supported by this command and there is now
> way to extend it.

Current code is just showing board information which is what it is in
FRU spec and it is common for all. There is no any OEM specific field now.

> Also why do the user have to manually do a "fru capture"? The use case
> is to display any FRU data, correct? So from a users perspective a "fru
> display" be sufficient to display the data.

The code was written for using fru board detection code where capture
part can be performed without displaying results. Simply because you
need to find where you are running to be able to know which device to
use for showing results. It means internally it needs to be divided.
That's going to second point if make sense to have two separate commands
or just one.
And it should be ok to have only one command till there is a need to do
it differently.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

[U-Boot] [PATCH 1/3] cmd: fru: Add support for FRU commands

[Michael Walle]

Am 2019-10-22 15:45, schrieb Michal Simek:
> On 22. 10. 19 15:09, Michael Walle wrote:
>> Am 2019-10-14 15:29, schrieb Michal Simek:
>>> From: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
>>> 
>>> This patch adds support for fru commands "fru capture" and "fru 
>>> display".
>>> The fru capture parses the FRU table present at an address and stores
>>> in a
>>> structure for later use. The fru display prints the content of 
>>> captured
>>> structured in a readable format.
>>> 
>>> As of now, it supports only common header and board area of FRU. 
>>> Also, it
>>> supports only English language code and ASCII8 format.
>>> 
>>> fru_data variable is placed to data section because fru parser can be
>>> called very early before bss is initialized. And also information
>>> needs to
>>> be shared that's why it is exported via header.
>> 
>> Wouldn't it make more sense to have a level of indirection so other
>> "fru" formats might be supported as well. As far as I can see, only
>> "your" type of FRU data is supported by this command and there is now
>> way to extend it.
> 
> Current code is just showing board information which is what it is in
> FRU spec and it is common for all. There is no any OEM specific field 
> now.

I didn't meant the (I guess it is the IPMI) FRU spec. But there are also 
other specifications like the PICMG Embedded EEPROM [1]. What I wanted 
to say is, that if there will be a new "fru" command, it might make 
sense to have it more generic. Otherwise, we have the fru command which 
can only display IPMI FRU spec compliant stuff and in the future, maybe 
a "picmg" command, which can only display PICMG EEPROM content.

>> Also why do the user have to manually do a "fru capture"? The use case
>> is to display any FRU data, correct? So from a users perspective a 
>> "fru
>> display" be sufficient to display the data.
> 
> The code was written for using fru board detection code where capture
> part can be performed without displaying results. Simply because you
> need to find where you are running to be able to know which device to
> use for showing results.

What does a "fru capture" do besides maybe printing an error. Briefly 
looking at the code, I didn't find anything else. (It sets the fru_data 
stuff which is used in "fru display" of course).

> It means internally it needs to be divided.
> That's going to second point if make sense to have two separate 
> commands
> or just one.
> And it should be ok to have only one command till there is a need to do
> it differently.

There might be more commands, but IMHO it should be sufficient to run 
"fru display" to print the data. If I read the code correctly, you have 
to do a "fru capture; fru display" at the moment; btw in the "fru 
display" error it reads "please run fru parse", I guess that is a typo.

-michael

[1] https://www.picmg.org/wp-content/uploads/PICMG_EeeP_R1_0.pdf

[U-Boot] [PATCH 1/3] cmd: fru: Add support for FRU commands

[Michal Simek]

On 22. 10. 19 16:43, Michael Walle wrote:
> Am 2019-10-22 15:45, schrieb Michal Simek:
>> On 22. 10. 19 15:09, Michael Walle wrote:
>>> Am 2019-10-14 15:29, schrieb Michal Simek:
>>>> From: Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
>>>>
>>>> This patch adds support for fru commands "fru capture" and "fru
>>>> display".
>>>> The fru capture parses the FRU table present at an address and stores
>>>> in a
>>>> structure for later use. The fru display prints the content of captured
>>>> structured in a readable format.
>>>>
>>>> As of now, it supports only common header and board area of FRU.
>>>> Also, it
>>>> supports only English language code and ASCII8 format.
>>>>
>>>> fru_data variable is placed to data section because fru parser can be
>>>> called very early before bss is initialized. And also information
>>>> needs to
>>>> be shared that's why it is exported via header.
>>>
>>> Wouldn't it make more sense to have a level of indirection so other
>>> "fru" formats might be supported as well. As far as I can see, only
>>> "your" type of FRU data is supported by this command and there is now
>>> way to extend it.
>>
>> Current code is just showing board information which is what it is in
>> FRU spec and it is common for all. There is no any OEM specific field
>> now.
> 
> I didn't meant the (I guess it is the IPMI) FRU spec. But there are also
> other specifications like the PICMG Embedded EEPROM [1]. What I wanted
> to say is, that if there will be a new "fru" command, it might make
> sense to have it more generic. Otherwise, we have the fru command which
> can only display IPMI FRU spec compliant stuff and in the future, maybe
> a "picmg" command, which can only display PICMG EEPROM content.

Yes it is IPMI FRU. I am not aware about PICMG but will read for sure.

FRU have a lot of options for OEMs to add their fields and defined them.
And I expect in future this should be handled. For Xilinx case there is
a consideration to use additional custom Mfg fields and also OEM record
types in multirecord area.

And yes different command should be used for picmg based on my quick scan.
The reason for FRU is that FMC cards used on Xilinx devices are using
FRU for identification.

> 
>>> Also why do the user have to manually do a "fru capture"? The use case
>>> is to display any FRU data, correct? So from a users perspective a "fru
>>> display" be sufficient to display the data.
>>
>> The code was written for using fru board detection code where capture
>> part can be performed without displaying results. Simply because you
>> need to find where you are running to be able to know which device to
>> use for showing results.
> 
> What does a "fru capture" do besides maybe printing an error. Briefly
> looking at the code, I didn't find anything else. (It sets the fru_data
> stuff which is used in "fru display" of course).

ok. NP to put them together.

> 
>> It means internally it needs to be divided.
>> That's going to second point if make sense to have two separate commands
>> or just one.
>> And it should be ok to have only one command till there is a need to do
>> it differently.
> 
> There might be more commands, but IMHO it should be sufficient to run
> "fru display" to print the data. If I read the code correctly, you have
> to do a "fru capture; fru display" at the moment; btw in the "fru
> display" error it reads "please run fru parse", I guess that is a typo.

I have fixed this in v2.
Will wait for Simon to suggest a way for tests and then will send v2.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

[U-Boot] [PATCH 2/3] cmd: fru: Add basic fru format generator

[Michal Simek]

Idea is to have something what can be used for board bringup from
generic board perspective.

There is a violation compare to spec that FRU ID is ASCII8 instead of
binary format but this is really for having something to pass boot and
boot to OS which has better generating options.
Also time should be filled properly.

For example:
fru board_gen 1000 XILINX versal-x-prc-01-revA serialX partX

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 cmd/fru.c        | 20 +++++++++-
 common/fru_ops.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/fru.h    | 14 +++++++
 3 files changed, 128 insertions(+), 1 deletion(-)

diff --git a/cmd/fru.c b/cmd/fru.c
index d5e96cfe8984..3bbc4d272adb 100644
--- a/cmd/fru.c
+++ b/cmd/fru.c
@@ -31,9 +31,23 @@ static int do_fru_display(cmd_tbl_t *cmdtp, int flag, int argc,
 	return CMD_RET_SUCCESS;
 }
 
+static int do_fru_generate(cmd_tbl_t *cmdtp, int flag, int argc,
+			   char *const argv[])
+{
+	unsigned long addr;
+
+	if (argc < cmdtp->maxargs)
+		return CMD_RET_USAGE;
+
+	addr = simple_strtoul(argv[2], NULL, 16);
+
+	return fru_generate(addr, argv[3], argv[4], argv[5], argv[6]);
+}
+
 static cmd_tbl_t cmd_fru_sub[] = {
 	U_BOOT_CMD_MKENT(capture, 3, 0, do_fru_capture, "", ""),
 	U_BOOT_CMD_MKENT(display, 2, 0, do_fru_display, "", ""),
+	U_BOOT_CMD_MKENT(board_gen, 7, 0, do_fru_generate, "", ""),
 };
 
 static int do_fru(cmd_tbl_t *cmdtp, int flag, int argc,
@@ -58,11 +72,15 @@ static char fru_help_text[] =
 	"capture <addr> - Parse and capture FRU table present at address.\n"
 	"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> - Generate FRU format with board info\n"
+	"              area filled based on parameters. <addr> is pointing\n"
+	"              to place where FRU is generated.\n"
 	;
 #endif
 
 U_BOOT_CMD(
-	fru, 3, 1, do_fru,
+	fru, 7, 1, do_fru,
 	"FRU table info",
 	fru_help_text
 )
diff --git a/common/fru_ops.c b/common/fru_ops.c
index dd54bd9c0214..d9c00b12da03 100644
--- a/common/fru_ops.c
+++ b/common/fru_ops.c
@@ -58,6 +58,101 @@ static int fru_check_type_len(u8 type_len, u8 language, u8 *type)
 	return len;
 }
 
+/* Return len */
+static u8 fru_gen_type_len(u8 *addr, char *name)
+{
+	int len = strlen(name);
+	struct fru_board_info_member *member;
+
+	member = (struct fru_board_info_member *)addr;
+	member->type_len = FRU_TYPELEN_TYPE_ASCII8 << FRU_TYPELEN_TYPE_SHIFT;
+	member->type_len |= len;
+
+	debug("%lx/%lx: Add %s to 0x%lx (len 0x%x)\n", (ulong)addr,
+	      (ulong)&member->type_len,  name, (ulong)&member->name, len);
+	memcpy(&member->name, name, len);
+
+	/* Add +1 for type_len parameter */
+	return 1 + len;
+}
+
+int fru_generate(unsigned long addr, char *manufacturer, char *board_name,
+		 char *serial_no, char *part_no)
+{
+	struct fru_common_hdr *header = (struct fru_common_hdr *)addr;
+	struct fru_board_info_header *board_info;
+	u8 *member;
+	u8 len, pad, modulo;
+
+	header->version = 1; /* Only version 1.0 is supported now */
+	header->off_internal = 0; /* not present */
+	header->off_chassis = 0; /* not present */
+	header->off_board = (sizeof(*header)) / 8; /* Starting offset 8 */
+	header->off_product = 0; /* not present */
+	header->off_multirec = 0; /* not present */
+	header->pad = 0;
+	/*
+	 * This unsigned byte can be used to calculate a zero checksum
+	 * for the data area following the header. I.e. the modulo 256 sum of
+	 * the record data bytes plus the checksum byte equals zero.
+	 */
+	header->crc = 0; /* Clear before calculation */
+	header->crc = 0 - fru_checksum((u8 *)header, sizeof(*header));
+
+	/* board info is just right after header */
+	board_info = (void *)((u8 *)header + sizeof(*header));
+
+	debug("header %lx, board_info %lx\n", (ulong)header, (ulong)board_info);
+
+	board_info->ver = 1; /* 1.0 spec */
+	board_info->lang_code = 0; /* English */
+	board_info->time[0] = 0; /* unspecified */
+	board_info->time[1] = 0; /* unspecified */
+	board_info->time[2] = 0; /* unspecified */
+
+	/* Member fields are just after board_info header */
+	member = (u8 *)board_info + sizeof(*board_info);
+
+	len = fru_gen_type_len(member, manufacturer); /* Board Manufacturer */
+	member += len;
+	len = fru_gen_type_len(member, board_name); /* Board Product name */
+	member += len;
+	len = fru_gen_type_len(member, serial_no); /* Board Serial number */
+	member += len;
+	len = fru_gen_type_len(member, part_no); /* Board part number */
+	member += len;
+	len = fru_gen_type_len(member, "U-Boot generator"); /* File ID */
+	member += len;
+
+	*member++ = 0xc1; /* Indication of no more fields */
+
+	len = member - (u8 *)board_info; /* Find current length */
+	len += 1; /* Add checksum there too for calculation */
+
+	modulo = len % 8;
+
+	if (modulo) {
+		/* Do not fill last item which is checksum */
+		for (pad = 0; pad < 8 - modulo; pad++)
+			*member++ = 0;
+
+		/* Increase structure size */
+		len += 8 - modulo;
+	}
+
+	board_info->len = len / 8; /* Size in multiples of 8 bytes */
+
+	*member = 0; /* Clear before calculation */
+	*member = 0 - fru_checksum((u8 *)board_info, len);
+
+	debug("checksum %x(len %x)\n", *member, len);
+
+	env_set_hex("fru_addr", addr);
+	env_set_hex("filesize", len);
+
+	return 0;
+}
+
 static int fru_parse_board(unsigned long addr)
 {
 	u8 i, type;
diff --git a/include/fru.h b/include/fru.h
index 38fdfad409c4..09de266477d4 100644
--- a/include/fru.h
+++ b/include/fru.h
@@ -20,6 +20,18 @@ struct fru_common_hdr {
 
 #define FRU_BOARD_MAX_LEN	32
 
+struct __packed fru_board_info_header {
+	u8 ver;
+	u8 len;
+	u8 lang_code;
+	u8 time[3];
+};
+
+struct __packed fru_board_info_member {
+	u8 type_len;
+	u8 *name;
+};
+
 struct fru_board_data {
 	u8 ver;
 	u8 len;
@@ -58,6 +70,8 @@ struct fru_table {
 
 int fru_display(void);
 int fru_capture(unsigned long addr);
+int fru_generate(unsigned long addr, char *manufacturer, char *board_name,
+		 char *serial_no, char *part_no);
 
 extern struct fru_table fru_data;
 
-- 
2.17.1

[U-Boot] [PATCH 3/3] arm64: versal: Enable FRU command

[Michal Simek]

Enable FRU command for Versal to enable FMC card decoding.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 configs/xilinx_versal_virt_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/xilinx_versal_virt_defconfig b/configs/xilinx_versal_virt_defconfig
index 1717039dc0d1..b05ed2fbeccc 100644
--- a/configs/xilinx_versal_virt_defconfig
+++ b/configs/xilinx_versal_virt_defconfig
@@ -34,6 +34,7 @@ CONFIG_CMD_PXE=y
 CONFIG_CMD_CACHE=y
 CONFIG_CMD_TIME=y
 CONFIG_CMD_TIMER=y
+CONFIG_CMD_FRU=y
 CONFIG_CMD_EXT2=y
 CONFIG_CMD_EXT4=y
 CONFIG_CMD_EXT4_WRITE=y
-- 
2.17.1