[PATCH 1/2] arm64: imx8mp: Read MAC address from M24C32-D write-lockable page on DH i.MX8MP DHCOM if available

[PATCH 1/2] arm64: imx8mp: Read MAC address from M24C32-D write-lockable page on DH i.MX8MP DHCOM if available

[Christoph Niedermaier]

The i.MX8M Plus DHCOM currently supports parsing ethernet MAC address
from multiple sources in the following priority order:

1) U-Boot environment 'ethaddr'/'eth1addr' environment variable
2) SoC OTP fuses
3) On-SoM EEPROM

The new i.MX8M Plus DHCOM rev.200 is populated with M24C32-D EEPROM
which contains additional write-lockable page, which can also be
populated with a structure containing ethernet MAC address.

Add support for parsing the content of this new write-lockable page
and place it between 2) and 3) on the priority list. The new entry is
2.5) On-SoM EEPROM write-lockable page

Because the write-lockable page is not present on rev.100 i.MX8MP DHCOM
SoM, test whether EEPROM ID page exists in DT and whether it is enabled
first. If so, read the entire ID page out, validate it, and determine
whether EEPROM MAC address is populated in it in DH specific format. If
so, use the MAC address. There may be multiple EEPROMs with an ID page
on this platform, always use the first one.

Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
---
Cc: "NXP i.MX U-Boot Team" <uboot-imx@nxp.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@dh-electronics.com
---
 board/dhelectronics/common/dh_common.c        | 113 ++++++++++++++++++
 board/dhelectronics/common/dh_common.h        |  23 ++++
 .../dh_imx8mp/imx8mp_dhcom_pdk2.c             |   6 +
 3 files changed, 142 insertions(+)

diff --git a/board/dhelectronics/common/dh_common.c b/board/dhelectronics/common/dh_common.c
index 32c50b4f0f..8ea70fc984 100644
--- a/board/dhelectronics/common/dh_common.c
+++ b/board/dhelectronics/common/dh_common.c
@@ -7,9 +7,22 @@
 #include <dm.h>
 #include <i2c_eeprom.h>
 #include <net.h>
+#include <u-boot/crc.h>
 
 #include "dh_common.h"
 
+struct eeprom_id_page {
+	u8	id[3];		/* Identifier 'D', 'H', 'E' - 'D' is at index 0 */
+	u8	version;	/* 0x10 -- Version 1.0 */
+	u8	data_crc16[2];	/* [1] is MSbyte */
+	u8	header_crc8;
+	u8	mac0[6];
+	u8	mac1[6];
+	u8	item_prefix;	/* H/F is coded in MSbits, Vendor coding starts at LSbits */
+	u8	item_num[3];	/* [2] is MSbyte */
+	u8	serial[9];	/* [8] is MSbyte */
+} __packed;
+
 bool dh_mac_is_in_env(const char *env)
 {
 	unsigned char enetaddr[6];
@@ -30,6 +43,106 @@ int dh_get_mac_is_enabled(const char *alias)
 	return 0;
 }
 
+int dh_get_value_from_eeprom_id_page(enum eip_request_values request, u8 *data,
+				     int data_len, const char *alias)
+{
+	struct eeprom_id_page *eipp;
+	struct udevice *dev;
+	static u8 eipa[32];
+	char path[128];
+	int len, ret;
+	ofnode node;
+	u16 c16;
+	u8 c8;
+
+	eipp = (struct eeprom_id_page *)eipa;
+
+	if (!(eipp->id[0] == 'D' && eipp->id[1] == 'H' && eipp->id[2] == 'E')) {
+		node = ofnode_path(alias);
+		if (!ofnode_valid(node)) {
+			printf("%s: ofnode for %s not found!", __func__, alias);
+			return -ENOENT;
+		}
+
+		ret = ofnode_get_path(node, path, sizeof(path));
+		if (ret)
+			return ret;
+
+		len = strlen(path);
+		if (len <= 0)
+			return -EINVAL;
+
+		if (path[len - 1] == '0')
+			path[len - 1] = '8';
+		else if (path[len - 1] == '3')
+			path[len - 1] = 'b';
+		else
+			return -ENOENT;
+
+		node = ofnode_path(path);
+		if (!ofnode_valid(node))	/* ID page not present in DT */
+			return -ENOENT;
+
+		if (!ofnode_is_enabled(node))	/* ID page not enabled in DT */
+			return -ENOENT;
+
+		ret = uclass_get_device_by_ofnode(UCLASS_I2C_EEPROM, node, &dev);
+		if (ret) {
+			printf("%s: Cannot find ID page! ret = %d\n", __func__, ret);
+			return ret;
+		}
+
+		ret = i2c_eeprom_read(dev, 0x0, eipa, sizeof(eipa));
+		if (ret) {
+			printf("%s: Error reading ID page! ret = %d\n", __func__, ret);
+			return ret;
+		}
+	}
+
+	/* Validate header checksum */
+	c8 = crc8(0xff, eipa, offsetof(struct eeprom_id_page, header_crc8));
+	if (eipp->header_crc8 != c8)
+		return -EINVAL;
+
+	/* Validate header magic */
+	if (eipp->id[0] != 'D' || eipp->id[1] != 'H' || eipp->id[2] != 'E')
+		return -EINVAL;
+
+	/* Validate header version */
+	if (eipp->version != 0x10)
+		return -EINVAL;
+
+	/* Validate structure checksum */
+	c16 = crc16(0xffff, eipa + offsetof(struct eeprom_id_page, mac0),
+		    sizeof(*eipp) - offsetof(struct eeprom_id_page, mac0));
+	if (((eipp->data_crc16[1] << 8) | eipp->data_crc16[0]) != c16)
+		return -EINVAL;
+
+	/* Copy requested data */
+	switch (request) {
+	case MAC0:
+		if (!is_valid_ethaddr(eipp->mac0))
+			return -EINVAL;
+		if (data_len >= sizeof(eipp->mac0))
+			memcpy(data, eipp->mac0, sizeof(eipp->mac0));
+		else
+			return -EINVAL;
+		break;
+	case MAC1:
+		if (!is_valid_ethaddr(eipp->mac1))
+			return -EINVAL;
+		if (data_len >= sizeof(eipp->mac1))
+			memcpy(data, eipp->mac1, sizeof(eipp->mac1));
+		else
+			return -EINVAL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 int dh_get_mac_from_eeprom(unsigned char *enetaddr, const char *alias)
 {
 	struct udevice *dev;
diff --git a/board/dhelectronics/common/dh_common.h b/board/dhelectronics/common/dh_common.h
index a2de5b1553..4c22ece435 100644
--- a/board/dhelectronics/common/dh_common.h
+++ b/board/dhelectronics/common/dh_common.h
@@ -3,6 +3,11 @@
  * Copyright 2022 DENX Software Engineering GmbH, Philip Oberfichtner <pro@denx.de>
  */
 
+enum eip_request_values {
+	MAC0,
+	MAC1,
+};
+
 /*
  * dh_mac_is_in_env - Check if MAC address is already set
  *
@@ -28,6 +33,24 @@ int dh_get_mac_is_enabled(const char *alias);
  */
 int dh_get_mac_from_eeprom(unsigned char *enetaddr, const char *alias);
 
+/*
+ * dh_get_value_from_eeprom_id_page() - Get value from EEPROM ID page
+ * @eip_request_values:	Requested value as enum
+ * @data:		Buffer where value is to be stored
+ * @data_len:		Length of the value buffer
+ * @alias:		Alias for EEPROM device tree node
+ *
+ * Gets the value specified by the parameter eip_request_values from the ID
+ * page of the specified EEPROM. The EEPROM device is selected via alias
+ * device tree name (parameter alias). The data is written to the specified
+ * data buffer (parameter data). If the length of the data (parameter data_len)
+ * is not sufficient to copy the data into the buffer, an error is returned.
+ *
+ * Return: 0 if OK, other value on error
+ */
+int dh_get_value_from_eeprom_id_page(enum eip_request_values request, u8 *data,
+				     int data_len, const char *alias);
+
 /*
  * dh_setup_mac_address - Try to get MAC address from various locations and write it to env
  *
diff --git a/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c b/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c
index 78aae41235..9a8f09fcd4 100644
--- a/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c
+++ b/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c
@@ -53,6 +53,9 @@ static int dh_imx8_setup_ethaddr(void)
 	if (!dh_imx_get_mac_from_fuse(enetaddr))
 		goto out;
 
+	if (!dh_get_value_from_eeprom_id_page(MAC0, enetaddr, sizeof(enetaddr), "eeprom0"))
+		goto out;
+
 	if (!dh_get_mac_from_eeprom(enetaddr, "eeprom0"))
 		goto out;
 
@@ -75,6 +78,9 @@ static int dh_imx8_setup_eth1addr(void)
 	if (!dh_imx_get_mac_from_fuse(enetaddr))
 		goto increment_out;
 
+	if (!dh_get_value_from_eeprom_id_page(MAC1, enetaddr, sizeof(enetaddr), "eeprom0"))
+		goto out;
+
 	if (!dh_get_mac_from_eeprom(enetaddr, "eeprom1"))
 		goto out;
 
-- 
2.30.2

[PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM

[Christoph Niedermaier]

The new i.MX8M Plus DHCOM rev.200 is populated with M24C32-D EEPROM
which contains additional write-lockable page called ID page, which
is populated with a structure containing the item and serial number.

Extend the support for parsing the item and serial number of the
EEPROM ID page. Write the item and serial number to the U-Boot
environment if the aren't there. If the environment is already
there compare it with the one from the EEPROM ID page and output
a warning if it differs.

Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
---
Cc: "NXP i.MX U-Boot Team" <uboot-imx@nxp.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Tom Rini <trini@konsulko.com>
Cc: u-boot@dh-electronics.com
---
 board/dhelectronics/common/dh_common.c        | 51 ++++++++++++++++++
 board/dhelectronics/common/dh_common.h        |  2 +
 .../dh_imx8mp/imx8mp_dhcom_pdk2.c             | 53 +++++++++++++++++++
 3 files changed, 106 insertions(+)

diff --git a/board/dhelectronics/common/dh_common.c b/board/dhelectronics/common/dh_common.c
index 8ea70fc984..4c31b32e0c 100644
--- a/board/dhelectronics/common/dh_common.c
+++ b/board/dhelectronics/common/dh_common.c
@@ -11,6 +11,23 @@
 
 #include "dh_common.h"
 
+/* DH item: Vendor coding */
+#define ITEM_PREFIX_NXP		0x01
+#define ITEM_PREFIX_NXP_CHR	'I'
+#define ITEM_PREFIX_ST		0x02
+#define ITEM_PREFIX_ST_CHR	'S'
+
+/*
+ * DH item: Finished state coding
+ * Bit = 0 means half finished
+ *         Prefix is 'H'
+ * Bit = 1 means finished with a customer image flashed
+ *         Prefix is 'F'
+ */
+#define ITEM_PREFIX_FIN_BIT		BIT(7)
+#define ITEM_PREFIX_FIN_HALF_CHR	'H'
+#define ITEM_PREFIX_FIN_FLASHED_CHR	'F'
+
 struct eeprom_id_page {
 	u8	id[3];		/* Identifier 'D', 'H', 'E' - 'D' is at index 0 */
 	u8	version;	/* 0x10 -- Version 1.0 */
@@ -54,6 +71,7 @@ int dh_get_value_from_eeprom_id_page(enum eip_request_values request, u8 *data,
 	ofnode node;
 	u16 c16;
 	u8 c8;
+	char soc;
 
 	eipp = (struct eeprom_id_page *)eipa;
 
@@ -136,6 +154,39 @@ int dh_get_value_from_eeprom_id_page(enum eip_request_values request, u8 *data,
 		else
 			return -EINVAL;
 		break;
+	case ITEM_NUMBER:
+		if (data_len >= 8) { /* String with 7 characters + string termination */
+			switch (eipp->item_prefix & 0xf) {
+			case ITEM_PREFIX_NXP:
+				soc = ITEM_PREFIX_NXP_CHR;
+				break;
+			case ITEM_PREFIX_ST:
+				soc = ITEM_PREFIX_ST_CHR;
+				break;
+			default:
+				return -EINVAL;
+			}
+			snprintf(data, data_len, "%c%c%05d",
+				 (eipp->item_prefix & ITEM_PREFIX_FIN_BIT) ?
+				 ITEM_PREFIX_FIN_FLASHED_CHR : ITEM_PREFIX_FIN_HALF_CHR,
+				 soc, (eipp->item_num[0] << 16) | (eipp->item_num[1] << 8)
+				       | eipp->item_num[2]);
+		} else {
+			return -EINVAL;
+		}
+		break;
+	case SN:
+		/*
+		 * data_len must be greater than the size of eipp->serial,
+		 * because there is a string termination needed.
+		 */
+		if (data_len > sizeof(eipp->serial)) {
+			data[sizeof(eipp->serial)] = 0;
+			memcpy(data, eipp->serial, sizeof(eipp->serial));
+		} else {
+			return -EINVAL;
+		}
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/board/dhelectronics/common/dh_common.h b/board/dhelectronics/common/dh_common.h
index 4c22ece435..1baa45e340 100644
--- a/board/dhelectronics/common/dh_common.h
+++ b/board/dhelectronics/common/dh_common.h
@@ -6,6 +6,8 @@
 enum eip_request_values {
 	MAC0,
 	MAC1,
+	ITEM_NUMBER,
+	SN,
 };
 
 /*
diff --git a/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c b/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c
index 9a8f09fcd4..8970c8fc2d 100644
--- a/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c
+++ b/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c
@@ -116,6 +116,56 @@ int dh_setup_mac_address(void)
 	return ret;
 }
 
+void dh_add_item_number_and_serial_to_env(void)
+{
+	char item_number[8];	/* String with 7 characters + string termination */
+	char *item_number_env;
+	char serial[10];	/* String with 9 characters + string termination */
+	char *serial_env;
+	int ret;
+
+	ret = dh_get_value_from_eeprom_id_page(ITEM_NUMBER, item_number, sizeof(item_number),
+					       "eeprom0");
+	if (ret) {
+		/*
+		 * The function only returns the value -ENOENT for SoM rev.100, because
+		 * the EEPROM ID page isn't available there. Therefore the output makes
+		 * no sense and will be suppressed here.
+		 */
+		if (ret != -ENOENT)
+			printf("%s: Unable to get item number form EEPROM ID page! ret = %d\n",
+			       __func__, ret);
+	} else {
+		item_number_env = env_get("vendor_item_number");
+		if (!item_number_env)
+			env_set("vendor_item_number", item_number);
+		else
+			if (strcmp(item_number_env, item_number) != 0)
+				printf("Warning: Environment vendor_item_number differs from EEPROM ID page value (%s != %s)\n",
+				       item_number_env, item_number);
+	}
+
+	ret = dh_get_value_from_eeprom_id_page(SN, serial, sizeof(serial), "eeprom0");
+	if (ret) {
+		/*
+		 * The function only returns the value -ENOENT for SoM rev.100, because
+		 * the EEPROM ID page isn't available there. Therefore the output makes
+		 * no sense and will be suppressed here.
+		 */
+		if (ret != -ENOENT)
+			printf("%s: Unable to get serial form EEPROM ID page! ret = %d\n",
+			       __func__, ret);
+	} else {
+		serial_env = env_get("SN");
+		if (!serial_env)
+			env_set("SN", serial);
+		else
+			if (strcmp(serial_env, serial) != 0)
+				printf("Warning: Environment SN differs from EEPROM ID page value (%s != %s)\n",
+				       serial_env, serial);
+	}
+}
+
 int board_init(void)
 {
 	return 0;
@@ -124,6 +174,9 @@ int board_init(void)
 int board_late_init(void)
 {
 	dh_setup_mac_address();
+
+	dh_add_item_number_and_serial_to_env();
+
 	return 0;
 }
 
-- 
2.30.2

Re: [PATCH 1/2] arm64: imx8mp: Read MAC address from M24C32-D write-lockable page on DH i.MX8MP DHCOM if available

[Marek Vasut]

On 10/10/24 3:23 PM, Christoph Niedermaier wrote:
> The i.MX8M Plus DHCOM currently supports parsing ethernet MAC address
> from multiple sources in the following priority order:
> 
> 1) U-Boot environment 'ethaddr'/'eth1addr' environment variable
> 2) SoC OTP fuses
> 3) On-SoM EEPROM
> 
> The new i.MX8M Plus DHCOM rev.200 is populated with M24C32-D EEPROM
> which contains additional write-lockable page, which can also be
> populated with a structure containing ethernet MAC address.
> 
> Add support for parsing the content of this new write-lockable page
> and place it between 2) and 3) on the priority list. The new entry is
> 2.5) On-SoM EEPROM write-lockable page
> 
> Because the write-lockable page is not present on rev.100 i.MX8MP DHCOM
> SoM, test whether EEPROM ID page exists in DT and whether it is enabled
> first. If so, read the entire ID page out, validate it, and determine
> whether EEPROM MAC address is populated in it in DH specific format. If
> so, use the MAC address. There may be multiple EEPROMs with an ID page
> on this platform, always use the first one.
> 
> Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> ---
> Cc: "NXP i.MX U-Boot Team" <uboot-imx@nxp.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: u-boot@dh-electronics.com
> ---
>   board/dhelectronics/common/dh_common.c        | 113 ++++++++++++++++++
>   board/dhelectronics/common/dh_common.h        |  23 ++++
>   .../dh_imx8mp/imx8mp_dhcom_pdk2.c             |   6 +
>   3 files changed, 142 insertions(+)
> 
> diff --git a/board/dhelectronics/common/dh_common.c b/board/dhelectronics/common/dh_common.c
> index 32c50b4f0f..8ea70fc984 100644
> --- a/board/dhelectronics/common/dh_common.c
> +++ b/board/dhelectronics/common/dh_common.c
> @@ -7,9 +7,22 @@
>   #include <dm.h>
>   #include <i2c_eeprom.h>
>   #include <net.h>
> +#include <u-boot/crc.h>
>   
>   #include "dh_common.h"
>   
> +struct eeprom_id_page {
> +	u8	id[3];		/* Identifier 'D', 'H', 'E' - 'D' is at index 0 */
> +	u8	version;	/* 0x10 -- Version 1.0 */
> +	u8	data_crc16[2];	/* [1] is MSbyte */
> +	u8	header_crc8;
> +	u8	mac0[6];
> +	u8	mac1[6];
> +	u8	item_prefix;	/* H/F is coded in MSbits, Vendor coding starts at LSbits */
> +	u8	item_num[3];	/* [2] is MSbyte */
> +	u8	serial[9];	/* [8] is MSbyte */
> +} __packed;

Is __packed needed ?

[...]

Re: [PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM

[Marek Vasut]

On 10/10/24 3:23 PM, Christoph Niedermaier wrote:
> The new i.MX8M Plus DHCOM rev.200 is populated with M24C32-D EEPROM
> which contains additional write-lockable page called ID page, which
> is populated with a structure containing the item and serial number.
> 
> Extend the support for parsing the item and serial number of the
> EEPROM ID page. Write the item and serial number to the U-Boot
> environment if the aren't there. If the environment is already
> there compare it with the one from the EEPROM ID page and output
> a warning if it differs.
> 
> Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> ---
> Cc: "NXP i.MX U-Boot Team" <uboot-imx@nxp.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: u-boot@dh-electronics.com
> ---
>   board/dhelectronics/common/dh_common.c        | 51 ++++++++++++++++++
>   board/dhelectronics/common/dh_common.h        |  2 +
>   .../dh_imx8mp/imx8mp_dhcom_pdk2.c             | 53 +++++++++++++++++++
>   3 files changed, 106 insertions(+)
> 
> diff --git a/board/dhelectronics/common/dh_common.c b/board/dhelectronics/common/dh_common.c
> index 8ea70fc984..4c31b32e0c 100644
> --- a/board/dhelectronics/common/dh_common.c
> +++ b/board/dhelectronics/common/dh_common.c
> @@ -11,6 +11,23 @@
>   
>   #include "dh_common.h"
>   
> +/* DH item: Vendor coding */
> +#define ITEM_PREFIX_NXP		0x01
> +#define ITEM_PREFIX_NXP_CHR	'I'
> +#define ITEM_PREFIX_ST		0x02
> +#define ITEM_PREFIX_ST_CHR	'S'

#define DH_ITEM_... to namespace the macros, please fix globally.

> +/*
> + * DH item: Finished state coding
> + * Bit = 0 means half finished
> + *         Prefix is 'H'
> + * Bit = 1 means finished with a customer image flashed
> + *         Prefix is 'F'
> + */
> +#define ITEM_PREFIX_FIN_BIT		BIT(7)
> +#define ITEM_PREFIX_FIN_HALF_CHR	'H'
> +#define ITEM_PREFIX_FIN_FLASHED_CHR	'F'
> +
>   struct eeprom_id_page {
>   	u8	id[3];		/* Identifier 'D', 'H', 'E' - 'D' is at index 0 */
>   	u8	version;	/* 0x10 -- Version 1.0 */
> @@ -54,6 +71,7 @@ int dh_get_value_from_eeprom_id_page(enum eip_request_values request, u8 *data,
>   	ofnode node;
>   	u16 c16;
>   	u8 c8;
> +	char soc;

Reverse xmas tree ordering here please.

>   	eipp = (struct eeprom_id_page *)eipa;
>   
> @@ -136,6 +154,39 @@ int dh_get_value_from_eeprom_id_page(enum eip_request_values request, u8 *data,
>   		else
>   			return -EINVAL;
>   		break;
> +	case ITEM_NUMBER:
> +		if (data_len >= 8) { /* String with 7 characters + string termination */

Invert this condition and reduce indent:

if (data < 8)
   return -EINVAL;

switch ...
...

> +			switch (eipp->item_prefix & 0xf) {
> +			case ITEM_PREFIX_NXP:
> +				soc = ITEM_PREFIX_NXP_CHR;
> +				break;
> +			case ITEM_PREFIX_ST:
> +				soc = ITEM_PREFIX_ST_CHR;
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +			snprintf(data, data_len, "%c%c%05d",
> +				 (eipp->item_prefix & ITEM_PREFIX_FIN_BIT) ?
> +				 ITEM_PREFIX_FIN_FLASHED_CHR : ITEM_PREFIX_FIN_HALF_CHR,
> +				 soc, (eipp->item_num[0] << 16) | (eipp->item_num[1] << 8)
> +				       | eipp->item_num[2]);
> +		} else {
> +			return -EINVAL;
> +		}
> +		break;
> +	case SN:

Use namespaced "DH_SERIAL_NUMBER":

> +		/*
> +		 * data_len must be greater than the size of eipp->serial,
> +		 * because there is a string termination needed.
> +		 */

Invert this condition and reduce indent:

if (data_len <= sizeof(eipp->serial))
   return -EINVAL;

...

> +		if (data_len > sizeof(eipp->serial)) {
> +			data[sizeof(eipp->serial)] = 0;
> +			memcpy(data, eipp->serial, sizeof(eipp->serial));
> +		} else {
> +			return -EINVAL;
> +		}
> +		break;
>   	default:
>   		return -EINVAL;
>   	}
> diff --git a/board/dhelectronics/common/dh_common.h b/board/dhelectronics/common/dh_common.h
> index 4c22ece435..1baa45e340 100644
> --- a/board/dhelectronics/common/dh_common.h
> +++ b/board/dhelectronics/common/dh_common.h
> @@ -6,6 +6,8 @@
>   enum eip_request_values {
>   	MAC0,
>   	MAC1,
> +	ITEM_NUMBER,
> +	SN,

Why is this patch not squashed into 1/2 ? It seems to be changing the 
same code.

>   };
>   
>   /*
> diff --git a/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c b/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c
> index 9a8f09fcd4..8970c8fc2d 100644
> --- a/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c
> +++ b/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c
> @@ -116,6 +116,56 @@ int dh_setup_mac_address(void)
>   	return ret;
>   }
>   
> +void dh_add_item_number_and_serial_to_env(void)
> +{
> +	char item_number[8];	/* String with 7 characters + string termination */
> +	char *item_number_env;
> +	char serial[10];	/* String with 9 characters + string termination */
> +	char *serial_env;
> +	int ret;

Reverse xmas tree please -- swap the first pair and second pair of 
variables.

> +
> +	ret = dh_get_value_from_eeprom_id_page(ITEM_NUMBER, item_number, sizeof(item_number),
> +					       "eeprom0");
> +	if (ret) {
> +		/*
> +		 * The function only returns the value -ENOENT for SoM rev.100, because
> +		 * the EEPROM ID page isn't available there. Therefore the output makes
> +		 * no sense and will be suppressed here.
> +		 */
> +		if (ret != -ENOENT)
> +			printf("%s: Unable to get item number form EEPROM ID page! ret = %d\n",
> +			       __func__, ret);

This will be printed on every device, even the ones without ID EEPROM, 
correct ? This should not be printed on devices without ID EEPROM. Also,

if (ret && ret != -ENOENT) {}

works equally well without the extra indent.

> +	} else {
> +		item_number_env = env_get("vendor_item_number");
> +		if (!item_number_env)
> +			env_set("vendor_item_number", item_number);
> +		else
> +			if (strcmp(item_number_env, item_number) != 0)

else if (strcmp(..., ...))
   log_warning(...)

> +				printf("Warning: Environment vendor_item_number differs from EEPROM ID page value (%s != %s)\n",
> +				       item_number_env, item_number);
> +	}
> +
> +	ret = dh_get_value_from_eeprom_id_page(SN, serial, sizeof(serial), "eeprom0");
> +	if (ret) {
> +		/*
> +		 * The function only returns the value -ENOENT for SoM rev.100, because
> +		 * the EEPROM ID page isn't available there. Therefore the output makes
> +		 * no sense and will be suppressed here.
> +		 */
> +		if (ret != -ENOENT)
> +			printf("%s: Unable to get serial form EEPROM ID page! ret = %d\n",
> +			       __func__, ret);

See above.

Also, this shouldn't be repeatedly reading the EEPROM, the EEPROM read 
operation is the slow part, the EEPROM is 32 bytes, so the EEPROM should 
be read once and cached, and once the cache is populated all read 
accesses to the EEPROM should use the cache.

> +	} else {
> +		serial_env = env_get("SN");
> +		if (!serial_env)
> +			env_set("SN", serial);
> +		else
> +			if (strcmp(serial_env, serial) != 0)
> +				printf("Warning: Environment SN differs from EEPROM ID page value (%s != %s)\n",
> +				       serial_env, serial);
> +	}
> +}
> +
>   int board_init(void)
>   {
>   	return 0;
[...]

RE: [PATCH 1/2] arm64: imx8mp: Read MAC address from M24C32-D write-lockable page on DH i.MX8MP DHCOM if available

[Christoph Niedermaier]

From: Marek Vasut <marex@denx.de>
Sent: Saturday, October 12, 2024 10:43 PM
> On 10/10/24 3:23 PM, Christoph Niedermaier wrote:
>> The i.MX8M Plus DHCOM currently supports parsing ethernet MAC address
>> from multiple sources in the following priority order:
>>
>> 1) U-Boot environment 'ethaddr'/'eth1addr' environment variable
>> 2) SoC OTP fuses
>> 3) On-SoM EEPROM
>>
>> The new i.MX8M Plus DHCOM rev.200 is populated with M24C32-D EEPROM
>> which contains additional write-lockable page, which can also be
>> populated with a structure containing ethernet MAC address.
>>
>> Add support for parsing the content of this new write-lockable page
>> and place it between 2) and 3) on the priority list. The new entry is
>> 2.5) On-SoM EEPROM write-lockable page
>>
>> Because the write-lockable page is not present on rev.100 i.MX8MP DHCOM
>> SoM, test whether EEPROM ID page exists in DT and whether it is enabled
>> first. If so, read the entire ID page out, validate it, and determine
>> whether EEPROM MAC address is populated in it in DH specific format. If
>> so, use the MAC address. There may be multiple EEPROMs with an ID page
>> on this platform, always use the first one.
>>
>> Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
>> ---
>> Cc: "NXP i.MX U-Boot Team" <uboot-imx@nxp.com>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Fabio Estevam <festevam@gmail.com>
>> Cc: Stefano Babic <sbabic@denx.de>
>> Cc: Tom Rini <trini@konsulko.com>
>> Cc: u-boot@dh-electronics.com
>> ---
>>   board/dhelectronics/common/dh_common.c        | 113 ++++++++++++++++++
>>   board/dhelectronics/common/dh_common.h        |  23 ++++
>>   .../dh_imx8mp/imx8mp_dhcom_pdk2.c             |   6 +
>>   3 files changed, 142 insertions(+)
>>
>> diff --git a/board/dhelectronics/common/dh_common.c b/board/dhelectronics/common/dh_common.c
>> index 32c50b4f0f..8ea70fc984 100644
>> --- a/board/dhelectronics/common/dh_common.c
>> +++ b/board/dhelectronics/common/dh_common.c
>> @@ -7,9 +7,22 @@
>>   #include <dm.h>
>>   #include <i2c_eeprom.h>
>>   #include <net.h>
>> +#include <u-boot/crc.h>
>>
>>   #include "dh_common.h"
>>
>> +struct eeprom_id_page {
>> +     u8      id[3];          /* Identifier 'D', 'H', 'E' - 'D' is at index 0 */
>> +     u8      version;        /* 0x10 -- Version 1.0 */
>> +     u8      data_crc16[2];  /* [1] is MSbyte */
>> +     u8      header_crc8;
>> +     u8      mac0[6];
>> +     u8      mac1[6];
>> +     u8      item_prefix;    /* H/F is coded in MSbits, Vendor coding starts at LSbits */
>> +     u8      item_num[3];    /* [2] is MSbyte */
>> +     u8      serial[9];      /* [8] is MSbyte */
>> +} __packed;
> 
> Is __packed needed ?

I want to avoid padding in any case.

Regards
Christoph

Re: [PATCH 1/2] arm64: imx8mp: Read MAC address from M24C32-D write-lockable page on DH i.MX8MP DHCOM if available

[Marek Vasut]

On 10/16/24 1:57 PM, Christoph Niedermaier wrote:
> From: Marek Vasut <marex@denx.de>
> Sent: Saturday, October 12, 2024 10:43 PM
>> On 10/10/24 3:23 PM, Christoph Niedermaier wrote:
>>> The i.MX8M Plus DHCOM currently supports parsing ethernet MAC address
>>> from multiple sources in the following priority order:
>>>
>>> 1) U-Boot environment 'ethaddr'/'eth1addr' environment variable
>>> 2) SoC OTP fuses
>>> 3) On-SoM EEPROM
>>>
>>> The new i.MX8M Plus DHCOM rev.200 is populated with M24C32-D EEPROM
>>> which contains additional write-lockable page, which can also be
>>> populated with a structure containing ethernet MAC address.
>>>
>>> Add support for parsing the content of this new write-lockable page
>>> and place it between 2) and 3) on the priority list. The new entry is
>>> 2.5) On-SoM EEPROM write-lockable page
>>>
>>> Because the write-lockable page is not present on rev.100 i.MX8MP DHCOM
>>> SoM, test whether EEPROM ID page exists in DT and whether it is enabled
>>> first. If so, read the entire ID page out, validate it, and determine
>>> whether EEPROM MAC address is populated in it in DH specific format. If
>>> so, use the MAC address. There may be multiple EEPROMs with an ID page
>>> on this platform, always use the first one.
>>>
>>> Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
>>> ---
>>> Cc: "NXP i.MX U-Boot Team" <uboot-imx@nxp.com>
>>> Cc: Marek Vasut <marex@denx.de>
>>> Cc: Fabio Estevam <festevam@gmail.com>
>>> Cc: Stefano Babic <sbabic@denx.de>
>>> Cc: Tom Rini <trini@konsulko.com>
>>> Cc: u-boot@dh-electronics.com
>>> ---
>>>    board/dhelectronics/common/dh_common.c        | 113 ++++++++++++++++++
>>>    board/dhelectronics/common/dh_common.h        |  23 ++++
>>>    .../dh_imx8mp/imx8mp_dhcom_pdk2.c             |   6 +
>>>    3 files changed, 142 insertions(+)
>>>
>>> diff --git a/board/dhelectronics/common/dh_common.c b/board/dhelectronics/common/dh_common.c
>>> index 32c50b4f0f..8ea70fc984 100644
>>> --- a/board/dhelectronics/common/dh_common.c
>>> +++ b/board/dhelectronics/common/dh_common.c
>>> @@ -7,9 +7,22 @@
>>>    #include <dm.h>
>>>    #include <i2c_eeprom.h>
>>>    #include <net.h>
>>> +#include <u-boot/crc.h>
>>>
>>>    #include "dh_common.h"
>>>
>>> +struct eeprom_id_page {
>>> +     u8      id[3];          /* Identifier 'D', 'H', 'E' - 'D' is at index 0 */
>>> +     u8      version;        /* 0x10 -- Version 1.0 */
>>> +     u8      data_crc16[2];  /* [1] is MSbyte */
>>> +     u8      header_crc8;
>>> +     u8      mac0[6];
>>> +     u8      mac1[6];
>>> +     u8      item_prefix;    /* H/F is coded in MSbits, Vendor coding starts at LSbits */
>>> +     u8      item_num[3];    /* [2] is MSbyte */
>>> +     u8      serial[9];      /* [8] is MSbyte */
>>> +} __packed;
>>
>> Is __packed needed ?
> 
> I want to avoid padding in any case.
Every single field is u8, do you observe any padding ?

RE: [PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM

[Christoph Niedermaier]

From: Marek Vasut <marex@denx.de>
Sent: Saturday, October 12, 2024 10:56 PM
> On 10/10/24 3:23 PM, Christoph Niedermaier wrote:
>> The new i.MX8M Plus DHCOM rev.200 is populated with M24C32-D EEPROM
>> which contains additional write-lockable page called ID page, which
>> is populated with a structure containing the item and serial number.
>>
>> Extend the support for parsing the item and serial number of the
>> EEPROM ID page. Write the item and serial number to the U-Boot
>> environment if the aren't there. If the environment is already
>> there compare it with the one from the EEPROM ID page and output
>> a warning if it differs.
>>
>> Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
>> ---
>> Cc: "NXP i.MX U-Boot Team" <uboot-imx@nxp.com>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Fabio Estevam <festevam@gmail.com>
>> Cc: Stefano Babic <sbabic@denx.de>
>> Cc: Tom Rini <trini@konsulko.com>
>> Cc: u-boot@dh-electronics.com
>> ---
>>   board/dhelectronics/common/dh_common.c        | 51 ++++++++++++++++++
>>   board/dhelectronics/common/dh_common.h        |  2 +
>>   .../dh_imx8mp/imx8mp_dhcom_pdk2.c             | 53 +++++++++++++++++++
>>   3 files changed, 106 insertions(+)
>>
>> diff --git a/board/dhelectronics/common/dh_common.c b/board/dhelectronics/common/dh_common.c
>> index 8ea70fc984..4c31b32e0c 100644
>> --- a/board/dhelectronics/common/dh_common.c
>> +++ b/board/dhelectronics/common/dh_common.c
>> @@ -11,6 +11,23 @@
>>
>>   #include "dh_common.h"
>>
>> +/* DH item: Vendor coding */
>> +#define ITEM_PREFIX_NXP              0x01
>> +#define ITEM_PREFIX_NXP_CHR  'I'
>> +#define ITEM_PREFIX_ST               0x02
>> +#define ITEM_PREFIX_ST_CHR   'S'
> 
> #define DH_ITEM_... to namespace the macros, please fix globally.

Will be fixed in v2.

>> +/*
>> + * DH item: Finished state coding
>> + * Bit = 0 means half finished
>> + *         Prefix is 'H'
>> + * Bit = 1 means finished with a customer image flashed
>> + *         Prefix is 'F'
>> + */
>> +#define ITEM_PREFIX_FIN_BIT          BIT(7)
>> +#define ITEM_PREFIX_FIN_HALF_CHR     'H'
>> +#define ITEM_PREFIX_FIN_FLASHED_CHR  'F'
>> +
>>   struct eeprom_id_page {
>>       u8      id[3];          /* Identifier 'D', 'H', 'E' - 'D' is at index 0 */
>>       u8      version;        /* 0x10 -- Version 1.0 */
>> @@ -54,6 +71,7 @@ int dh_get_value_from_eeprom_id_page(enum eip_request_values request, u8 *data,
>>       ofnode node;
>>       u16 c16;
>>       u8 c8;
>> +     char soc;
> 
> Reverse xmas tree ordering here please.

Will be fixed in v2.

>>       eipp = (struct eeprom_id_page *)eipa;
>>
>> @@ -136,6 +154,39 @@ int dh_get_value_from_eeprom_id_page(enum eip_request_values request, u8 *data,
>>               else
>>                       return -EINVAL;
>>               break;
>> +     case ITEM_NUMBER:
>> +             if (data_len >= 8) { /* String with 7 characters + string termination */
> 
> Invert this condition and reduce indent:
> 
> if (data < 8)
>    return -EINVAL;
> 
> switch ...
> ...

Will be fixed in v2.

>> +                     switch (eipp->item_prefix & 0xf) {
>> +                     case ITEM_PREFIX_NXP:
>> +                             soc = ITEM_PREFIX_NXP_CHR;
>> +                             break;
>> +                     case ITEM_PREFIX_ST:
>> +                             soc = ITEM_PREFIX_ST_CHR;
>> +                             break;
>> +                     default:
>> +                             return -EINVAL;
>> +                     }
>> +                     snprintf(data, data_len, "%c%c%05d",
>> +                              (eipp->item_prefix & ITEM_PREFIX_FIN_BIT) ?
>> +                              ITEM_PREFIX_FIN_FLASHED_CHR : ITEM_PREFIX_FIN_HALF_CHR,
>> +                              soc, (eipp->item_num[0] << 16) | (eipp->item_num[1] << 8)
>> +                                    | eipp->item_num[2]);
>> +             } else {
>> +                     return -EINVAL;
>> +             }
>> +             break;
>> +     case SN:
> 
> Use namespaced "DH_SERIAL_NUMBER":

Will be fixed in v2.

>> +             /*
>> +              * data_len must be greater than the size of eipp->serial,
>> +              * because there is a string termination needed.
>> +              */
> 
> Invert this condition and reduce indent:
> 
> if (data_len <= sizeof(eipp->serial))
>    return -EINVAL;
> 
> ...

Will be fixed in v2.

>> +             if (data_len > sizeof(eipp->serial)) {
>> +                     data[sizeof(eipp->serial)] = 0;
>> +                     memcpy(data, eipp->serial, sizeof(eipp->serial));
>> +             } else {
>> +                     return -EINVAL;
>> +             }
>> +             break;
>>       default:
>>               return -EINVAL;
>>       }
>> diff --git a/board/dhelectronics/common/dh_common.h b/board/dhelectronics/common/dh_common.h
>> index 4c22ece435..1baa45e340 100644
>> --- a/board/dhelectronics/common/dh_common.h
>> +++ b/board/dhelectronics/common/dh_common.h
>> @@ -6,6 +6,8 @@
>>   enum eip_request_values {
>>       MAC0,
>>       MAC1,
>> +     ITEM_NUMBER,
>> +     SN,
> 
> Why is this patch not squashed into 1/2 ? It seems to be changing the
> same code.

The first patch add the reading for MAC address from the EEPROM ID
page and add the use of that addresses. The second extends the reading
to the serial number and the item number. So that the patch doesn't
get too big I found it useful to split it into two. Do you want me to
make one patch out of it?

>>   };
>>
>>   /*
>> diff --git a/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c b/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c
>> index 9a8f09fcd4..8970c8fc2d 100644
>> --- a/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c
>> +++ b/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c
>> @@ -116,6 +116,56 @@ int dh_setup_mac_address(void)
>>       return ret;
>>   }
>>
>> +void dh_add_item_number_and_serial_to_env(void)
>> +{
>> +     char item_number[8];    /* String with 7 characters + string termination */
>> +     char *item_number_env;
>> +     char serial[10];        /* String with 9 characters + string termination */
>> +     char *serial_env;
>> +     int ret;
> 
> Reverse xmas tree please -- swap the first pair and second pair of
> variables.

Will be fixed in v2.

>> +
>> +     ret = dh_get_value_from_eeprom_id_page(ITEM_NUMBER, item_number, sizeof(item_number),
>> +                                            "eeprom0");
>> +     if (ret) {
>> +             /*
>> +              * The function only returns the value -ENOENT for SoM rev.100, because
>> +              * the EEPROM ID page isn't available there. Therefore the output makes
>> +              * no sense and will be suppressed here.
>> +              */
>> +             if (ret != -ENOENT)
>> +                     printf("%s: Unable to get item number form EEPROM ID page! ret = %d\n",
>> +                            __func__, ret);
> 
> This will be printed on every device, even the ones without ID EEPROM,
> correct ? This should not be printed on devices without ID EEPROM. Also,

This is suppressed by the -ENOENT check.

> if (ret && ret != -ENOENT) {}
> 
> works equally well without the extra indent.

I have an else to (ret) here not to (ret && ret != -ENOENT).
This would change the logic.

>> +     } else {
>> +             item_number_env = env_get("vendor_item_number");
>> +             if (!item_number_env)
>> +                     env_set("vendor_item_number", item_number);
>> +             else
>> +                     if (strcmp(item_number_env, item_number) != 0)
> 
> else if (strcmp(..., ...))
>    log_warning(...)

Will be fixed in v2.
 
>> +                             printf("Warning: Environment vendor_item_number differs from EEPROM ID page value (%s != %s)\n",
>> +                                    item_number_env, item_number);
>> +     }
>> +
>> +     ret = dh_get_value_from_eeprom_id_page(SN, serial, sizeof(serial), "eeprom0");
>> +     if (ret) {
>> +             /*
>> +              * The function only returns the value -ENOENT for SoM rev.100, because
>> +              * the EEPROM ID page isn't available there. Therefore the output makes
>> +              * no sense and will be suppressed here.
>> +              */
>> +             if (ret != -ENOENT)
>> +                     printf("%s: Unable to get serial form EEPROM ID page! ret = %d\n",
>> +                            __func__, ret);
> 
> See above.

Will be fixed in v2.

> Also, this shouldn't be repeatedly reading the EEPROM, the EEPROM read
> operation is the slow part, the EEPROM is 32 bytes, so the EEPROM should
> be read once and cached, and once the cache is populated all read
> accesses to the EEPROM should use the cache.

This is already covered in function dh_get_value_from_eeprom_id_page().

>> +     } else {
>> +             serial_env = env_get("SN");
>> +             if (!serial_env)
>> +                     env_set("SN", serial);
>> +             else
>> +                     if (strcmp(serial_env, serial) != 0)
>> +                             printf("Warning: Environment SN differs from EEPROM ID page value (%s != %s)\n",
>> +                                    serial_env, serial);
>> +     }
>> +}
>> +
>>   int board_init(void)
>>   {
>>       return 0;
> [...]

Regards
Christoph

Re: [PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM

[Marek Vasut]

On 10/16/24 2:31 PM, Christoph Niedermaier wrote:

[...]

>>> diff --git a/board/dhelectronics/common/dh_common.h b/board/dhelectronics/common/dh_common.h
>>> index 4c22ece435..1baa45e340 100644
>>> --- a/board/dhelectronics/common/dh_common.h
>>> +++ b/board/dhelectronics/common/dh_common.h
>>> @@ -6,6 +6,8 @@
>>>    enum eip_request_values {
>>>        MAC0,
>>>        MAC1,
>>> +     ITEM_NUMBER,
>>> +     SN,
>>
>> Why is this patch not squashed into 1/2 ? It seems to be changing the
>> same code.
> 
> The first patch add the reading for MAC address from the EEPROM ID
> page and add the use of that addresses. The second extends the reading
> to the serial number and the item number. So that the patch doesn't
> get too big I found it useful to split it into two. Do you want me to
> make one patch out of it?

Yes please. Once you cache the EEPROM content, the patch would likely 
get simpler anyway.

[...]

>>> +     ret = dh_get_value_from_eeprom_id_page(ITEM_NUMBER, item_number, sizeof(item_number),
>>> +                                            "eeprom0");
>>> +     if (ret) {
>>> +             /*
>>> +              * The function only returns the value -ENOENT for SoM rev.100, because
>>> +              * the EEPROM ID page isn't available there. Therefore the output makes
>>> +              * no sense and will be suppressed here.
>>> +              */
>>> +             if (ret != -ENOENT)
>>> +                     printf("%s: Unable to get item number form EEPROM ID page! ret = %d\n",

'form' typo

>>> +                            __func__, ret);
>>
>> This will be printed on every device, even the ones without ID EEPROM,
>> correct ? This should not be printed on devices without ID EEPROM. Also,
> 
> This is suppressed by the -ENOENT check.

Does i2c_eeprom_read() in dh_get_value_from_eeprom_id_page() return 
-ENOENT in case the EEPROM is described in DT, but not populated on the 
board ? I suspect it returns some other error code, -ETIMEDOUT or 
-EINVAL maybe ?

>> if (ret && ret != -ENOENT) {}
>>
>> works equally well without the extra indent.
> 
> I have an else to (ret) here not to (ret && ret != -ENOENT).
> This would change the logic.

} else if (!ret) { or similar should fix that, right ?

Basically, the question is, can we avoid this two level deep indent ? I 
think yes ?

[...]

>> Also, this shouldn't be repeatedly reading the EEPROM, the EEPROM read
>> operation is the slow part, the EEPROM is 32 bytes, so the EEPROM should
>> be read once and cached, and once the cache is populated all read
>> accesses to the EEPROM should use the cache.
> 
> This is already covered in function dh_get_value_from_eeprom_id_page().
It seems that function always calls
ret = i2c_eeprom_read(dev, 0x0, eipa, sizeof(eipa));
?

RE: [PATCH 1/2] arm64: imx8mp: Read MAC address from M24C32-D write-lockable page on DH i.MX8MP DHCOM if available

[Christoph Niedermaier]

From: Marek Vasut <marex@denx.de>
Sent: Wednesday, October 16, 2024 2:16 PM
> On 10/16/24 1:57 PM, Christoph Niedermaier wrote:
>> From: Marek Vasut <marex@denx.de>
>> Sent: Saturday, October 12, 2024 10:43 PM
>>> On 10/10/24 3:23 PM, Christoph Niedermaier wrote:
>>>> The i.MX8M Plus DHCOM currently supports parsing ethernet MAC address
>>>> from multiple sources in the following priority order:
>>>>
>>>> 1) U-Boot environment 'ethaddr'/'eth1addr' environment variable
>>>> 2) SoC OTP fuses
>>>> 3) On-SoM EEPROM
>>>>
>>>> The new i.MX8M Plus DHCOM rev.200 is populated with M24C32-D EEPROM
>>>> which contains additional write-lockable page, which can also be
>>>> populated with a structure containing ethernet MAC address.
>>>>
>>>> Add support for parsing the content of this new write-lockable page
>>>> and place it between 2) and 3) on the priority list. The new entry is
>>>> 2.5) On-SoM EEPROM write-lockable page
>>>>
>>>> Because the write-lockable page is not present on rev.100 i.MX8MP DHCOM
>>>> SoM, test whether EEPROM ID page exists in DT and whether it is enabled
>>>> first. If so, read the entire ID page out, validate it, and determine
>>>> whether EEPROM MAC address is populated in it in DH specific format. If
>>>> so, use the MAC address. There may be multiple EEPROMs with an ID page
>>>> on this platform, always use the first one.
>>>>
>>>> Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
>>>> ---
>>>> Cc: "NXP i.MX U-Boot Team" <uboot-imx@nxp.com>
>>>> Cc: Marek Vasut <marex@denx.de>
>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>> Cc: Stefano Babic <sbabic@denx.de>
>>>> Cc: Tom Rini <trini@konsulko.com>
>>>> Cc: u-boot@dh-electronics.com
>>>> ---
>>>>    board/dhelectronics/common/dh_common.c        | 113 ++++++++++++++++++
>>>>    board/dhelectronics/common/dh_common.h        |  23 ++++
>>>>    .../dh_imx8mp/imx8mp_dhcom_pdk2.c             |   6 +
>>>>    3 files changed, 142 insertions(+)
>>>>
>>>> diff --git a/board/dhelectronics/common/dh_common.c b/board/dhelectronics/common/dh_common.c
>>>> index 32c50b4f0f..8ea70fc984 100644
>>>> --- a/board/dhelectronics/common/dh_common.c
>>>> +++ b/board/dhelectronics/common/dh_common.c
>>>> @@ -7,9 +7,22 @@
>>>>    #include <dm.h>
>>>>    #include <i2c_eeprom.h>
>>>>    #include <net.h>
>>>> +#include <u-boot/crc.h>
>>>>
>>>>    #include "dh_common.h"
>>>>
>>>> +struct eeprom_id_page {
>>>> +     u8      id[3];          /* Identifier 'D', 'H', 'E' - 'D' is at index 0 */
>>>> +     u8      version;        /* 0x10 -- Version 1.0 */
>>>> +     u8      data_crc16[2];  /* [1] is MSbyte */
>>>> +     u8      header_crc8;
>>>> +     u8      mac0[6];
>>>> +     u8      mac1[6];
>>>> +     u8      item_prefix;    /* H/F is coded in MSbits, Vendor coding starts at LSbits */
>>>> +     u8      item_num[3];    /* [2] is MSbyte */
>>>> +     u8      serial[9];      /* [8] is MSbyte */
>>>> +} __packed;
>>>
>>> Is __packed needed ?
>>
>> I want to avoid padding in any case.
> Every single field is u8, do you observe any padding ?

In this case, I don't expect padding, but if the struct is extended,
padding could be possible. This struct is for an EEPROM and therefore
it should be defined with no padding. I don't want to give the compile
the change to add padding if something changed in the struct.
What are the disadvantages of keeping __packed here?

Regards
Christoph

RE: [PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM

[Christoph Niedermaier]

From: Marek Vasut <marex@denx.de>
Sent: Thursday, October 17, 2024 2:22 AM
> On 10/16/24 2:31 PM, Christoph Niedermaier wrote:
> 
> [...]
> 
>>>> diff --git a/board/dhelectronics/common/dh_common.h b/board/dhelectronics/common/dh_common.h
>>>> index 4c22ece435..1baa45e340 100644
>>>> --- a/board/dhelectronics/common/dh_common.h
>>>> +++ b/board/dhelectronics/common/dh_common.h
>>>> @@ -6,6 +6,8 @@
>>>>    enum eip_request_values {
>>>>        MAC0,
>>>>        MAC1,
>>>> +     ITEM_NUMBER,
>>>> +     SN,
>>>
>>> Why is this patch not squashed into 1/2 ? It seems to be changing the
>>> same code.
>>
>> The first patch add the reading for MAC address from the EEPROM ID
>> page and add the use of that addresses. The second extends the reading
>> to the serial number and the item number. So that the patch doesn't
>> get too big I found it useful to split it into two. Do you want me to
>> make one patch out of it?
> 
> Yes please. Once you cache the EEPROM content, the patch would likely
> get simpler anyway.

Will be done in v2.

> 
> [...]
> 
>>>> +     ret = dh_get_value_from_eeprom_id_page(ITEM_NUMBER, item_number, sizeof(item_number),
>>>> +                                            "eeprom0");
>>>> +     if (ret) {
>>>> +             /*
>>>> +              * The function only returns the value -ENOENT for SoM rev.100, because
>>>> +              * the EEPROM ID page isn't available there. Therefore the output makes
>>>> +              * no sense and will be suppressed here.
>>>> +              */
>>>> +             if (ret != -ENOENT)
>>>> +                     printf("%s: Unable to get item number form EEPROM ID page! ret = %d\n",
> 
> 'form' typo

Will be fixed in v2.

>>>> +                            __func__, ret);
>>>
>>> This will be printed on every device, even the ones without ID EEPROM,
>>> correct ? This should not be printed on devices without ID EEPROM. Also,
>>
>> This is suppressed by the -ENOENT check.
> 
> Does i2c_eeprom_read() in dh_get_value_from_eeprom_id_page() return
> -ENOENT in case the EEPROM is described in DT, but not populated on the
> board ? I suspect it returns some other error code, -ETIMEDOUT or
> -EINVAL maybe ?

It could only be possible if the DTO for hardware rev. 100 (which has no
EEPROM ID page) is not loaded. The return value then is -ENODEV.
I will included this in v2.

>>> if (ret && ret != -ENOENT) {}
>>>
>>> works equally well without the extra indent.
>>
>> I have an else to (ret) here not to (ret && ret != -ENOENT).
>> This would change the logic.
> 
> } else if (!ret) { or similar should fix that, right ?
> 
> Basically, the question is, can we avoid this two level deep indent ? I
> think yes ?

I want to try it in v2.

> [...]
> 
>>> Also, this shouldn't be repeatedly reading the EEPROM, the EEPROM read
>>> operation is the slow part, the EEPROM is 32 bytes, so the EEPROM should
>>> be read once and cached, and once the cache is populated all read
>>> accesses to the EEPROM should use the cache.
>>
>> This is already covered in function dh_get_value_from_eeprom_id_page().
> It seems that function always calls
> ret = i2c_eeprom_read(dev, 0x0, eipa, sizeof(eipa));
> ?

The 32 bytes are read into a static variable. If the ID (DHE) already exists
in it, the i2c_eeprom_read() function is no longer called.


Regards
Christoph

Re: [PATCH 1/2] arm64: imx8mp: Read MAC address from M24C32-D write-lockable page on DH i.MX8MP DHCOM if available

[Marek Vasut]

On 10/17/24 1:09 PM, Christoph Niedermaier wrote:
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, October 16, 2024 2:16 PM
>> On 10/16/24 1:57 PM, Christoph Niedermaier wrote:
>>> From: Marek Vasut <marex@denx.de>
>>> Sent: Saturday, October 12, 2024 10:43 PM
>>>> On 10/10/24 3:23 PM, Christoph Niedermaier wrote:
>>>>> The i.MX8M Plus DHCOM currently supports parsing ethernet MAC address
>>>>> from multiple sources in the following priority order:
>>>>>
>>>>> 1) U-Boot environment 'ethaddr'/'eth1addr' environment variable
>>>>> 2) SoC OTP fuses
>>>>> 3) On-SoM EEPROM
>>>>>
>>>>> The new i.MX8M Plus DHCOM rev.200 is populated with M24C32-D EEPROM
>>>>> which contains additional write-lockable page, which can also be
>>>>> populated with a structure containing ethernet MAC address.
>>>>>
>>>>> Add support for parsing the content of this new write-lockable page
>>>>> and place it between 2) and 3) on the priority list. The new entry is
>>>>> 2.5) On-SoM EEPROM write-lockable page
>>>>>
>>>>> Because the write-lockable page is not present on rev.100 i.MX8MP DHCOM
>>>>> SoM, test whether EEPROM ID page exists in DT and whether it is enabled
>>>>> first. If so, read the entire ID page out, validate it, and determine
>>>>> whether EEPROM MAC address is populated in it in DH specific format. If
>>>>> so, use the MAC address. There may be multiple EEPROMs with an ID page
>>>>> on this platform, always use the first one.
>>>>>
>>>>> Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
>>>>> ---
>>>>> Cc: "NXP i.MX U-Boot Team" <uboot-imx@nxp.com>
>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>> Cc: Stefano Babic <sbabic@denx.de>
>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>> Cc: u-boot@dh-electronics.com
>>>>> ---
>>>>>     board/dhelectronics/common/dh_common.c        | 113 ++++++++++++++++++
>>>>>     board/dhelectronics/common/dh_common.h        |  23 ++++
>>>>>     .../dh_imx8mp/imx8mp_dhcom_pdk2.c             |   6 +
>>>>>     3 files changed, 142 insertions(+)
>>>>>
>>>>> diff --git a/board/dhelectronics/common/dh_common.c b/board/dhelectronics/common/dh_common.c
>>>>> index 32c50b4f0f..8ea70fc984 100644
>>>>> --- a/board/dhelectronics/common/dh_common.c
>>>>> +++ b/board/dhelectronics/common/dh_common.c
>>>>> @@ -7,9 +7,22 @@
>>>>>     #include <dm.h>
>>>>>     #include <i2c_eeprom.h>
>>>>>     #include <net.h>
>>>>> +#include <u-boot/crc.h>
>>>>>
>>>>>     #include "dh_common.h"
>>>>>
>>>>> +struct eeprom_id_page {
>>>>> +     u8      id[3];          /* Identifier 'D', 'H', 'E' - 'D' is at index 0 */
>>>>> +     u8      version;        /* 0x10 -- Version 1.0 */
>>>>> +     u8      data_crc16[2];  /* [1] is MSbyte */
>>>>> +     u8      header_crc8;
>>>>> +     u8      mac0[6];
>>>>> +     u8      mac1[6];
>>>>> +     u8      item_prefix;    /* H/F is coded in MSbits, Vendor coding starts at LSbits */
>>>>> +     u8      item_num[3];    /* [2] is MSbyte */
>>>>> +     u8      serial[9];      /* [8] is MSbyte */
>>>>> +} __packed;
>>>>
>>>> Is __packed needed ?
>>>
>>> I want to avoid padding in any case.
>> Every single field is u8, do you observe any padding ?
> 
> In this case, I don't expect padding, but if the struct is extended,
> padding could be possible.

Please add the padding only once it is really required, ideally the 
design of the structure or its extension(s) would be able to avoid such 
necessity altogether.

> This struct is for an EEPROM and therefore
> it should be defined with no padding. I don't want to give the compile
> the change to add padding if something changed in the struct.
> What are the disadvantages of keeping __packed here?
If it is unnecessary, then please remove it, there is no good 
justification for packing a structure which is naturally aligned already.

Re: [PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM

[Marek Vasut]

On 10/17/24 1:55 PM, Christoph Niedermaier wrote:

[...]

>>>>> +                            __func__, ret);
>>>>
>>>> This will be printed on every device, even the ones without ID EEPROM,
>>>> correct ? This should not be printed on devices without ID EEPROM. Also,
>>>
>>> This is suppressed by the -ENOENT check.
>>
>> Does i2c_eeprom_read() in dh_get_value_from_eeprom_id_page() return
>> -ENOENT in case the EEPROM is described in DT, but not populated on the
>> board ? I suspect it returns some other error code, -ETIMEDOUT or
>> -EINVAL maybe ?
> 
> It could only be possible if the DTO for hardware rev. 100 (which has no
> EEPROM ID page) is not loaded. The return value then is -ENODEV.
> I will included this in v2.

OK

>>>> Also, this shouldn't be repeatedly reading the EEPROM, the EEPROM read
>>>> operation is the slow part, the EEPROM is 32 bytes, so the EEPROM should
>>>> be read once and cached, and once the cache is populated all read
>>>> accesses to the EEPROM should use the cache.
>>>
>>> This is already covered in function dh_get_value_from_eeprom_id_page().
>> It seems that function always calls
>> ret = i2c_eeprom_read(dev, 0x0, eipa, sizeof(eipa));
>> ?
> 
> The 32 bytes are read into a static variable. If the ID (DHE) already exists
> in it, the i2c_eeprom_read() function is no longer called.
Hmmm, but it seems all the functions which access this EEPROM WLP are 
called from board_init(), are they not ?

If yes, then there is no need for any static variable:

board_init() {
  u8 eeprom[32];
  dh_read_eeprom_wlp(eeprom); // read the eeprom once
  dh_setup_mac_address(eeprom); // extract MAC from EEPROM
  dh_add_item_number_and_serial_to_env(eeprom); // extract SN from EEPROM
  // once this function exits, the eeprom variable on stack is discarded
  // which is OK, since it won't be used anymore anyway
}

[...]

RE: [PATCH 1/2] arm64: imx8mp: Read MAC address from M24C32-D write-lockable page on DH i.MX8MP DHCOM if available

[Christoph Niedermaier]

From: Marek Vasut <marex@denx.de>
Sent: Thursday, October 17, 2024 4:01 PM
> On 10/17/24 1:09 PM, Christoph Niedermaier wrote:
>> From: Marek Vasut <marex@denx.de>
>> Sent: Wednesday, October 16, 2024 2:16 PM
>>> On 10/16/24 1:57 PM, Christoph Niedermaier wrote:
>>>> From: Marek Vasut <marex@denx.de>
>>>> Sent: Saturday, October 12, 2024 10:43 PM
>>>>> On 10/10/24 3:23 PM, Christoph Niedermaier wrote:
>>>>>> The i.MX8M Plus DHCOM currently supports parsing ethernet MAC address
>>>>>> from multiple sources in the following priority order:
>>>>>>
>>>>>> 1) U-Boot environment 'ethaddr'/'eth1addr' environment variable
>>>>>> 2) SoC OTP fuses
>>>>>> 3) On-SoM EEPROM
>>>>>>
>>>>>> The new i.MX8M Plus DHCOM rev.200 is populated with M24C32-D EEPROM
>>>>>> which contains additional write-lockable page, which can also be
>>>>>> populated with a structure containing ethernet MAC address.
>>>>>>
>>>>>> Add support for parsing the content of this new write-lockable page
>>>>>> and place it between 2) and 3) on the priority list. The new entry is
>>>>>> 2.5) On-SoM EEPROM write-lockable page
>>>>>>
>>>>>> Because the write-lockable page is not present on rev.100 i.MX8MP DHCOM
>>>>>> SoM, test whether EEPROM ID page exists in DT and whether it is enabled
>>>>>> first. If so, read the entire ID page out, validate it, and determine
>>>>>> whether EEPROM MAC address is populated in it in DH specific format. If
>>>>>> so, use the MAC address. There may be multiple EEPROMs with an ID page
>>>>>> on this platform, always use the first one.
>>>>>>
>>>>>> Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
>>>>>> ---
>>>>>> Cc: "NXP i.MX U-Boot Team" <uboot-imx@nxp.com>
>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>>> Cc: Stefano Babic <sbabic@denx.de>
>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>> Cc: u-boot@dh-electronics.com
>>>>>> ---
>>>>>>     board/dhelectronics/common/dh_common.c        | 113 ++++++++++++++++++
>>>>>>     board/dhelectronics/common/dh_common.h        |  23 ++++
>>>>>>     .../dh_imx8mp/imx8mp_dhcom_pdk2.c             |   6 +
>>>>>>     3 files changed, 142 insertions(+)
>>>>>>
>>>>>> diff --git a/board/dhelectronics/common/dh_common.c b/board/dhelectronics/common/dh_common.c
>>>>>> index 32c50b4f0f..8ea70fc984 100644
>>>>>> --- a/board/dhelectronics/common/dh_common.c
>>>>>> +++ b/board/dhelectronics/common/dh_common.c
>>>>>> @@ -7,9 +7,22 @@
>>>>>>     #include <dm.h>
>>>>>>     #include <i2c_eeprom.h>
>>>>>>     #include <net.h>
>>>>>> +#include <u-boot/crc.h>
>>>>>>
>>>>>>     #include "dh_common.h"
>>>>>>
>>>>>> +struct eeprom_id_page {
>>>>>> +     u8      id[3];          /* Identifier 'D', 'H', 'E' - 'D' is at index 0 */
>>>>>> +     u8      version;        /* 0x10 -- Version 1.0 */
>>>>>> +     u8      data_crc16[2];  /* [1] is MSbyte */
>>>>>> +     u8      header_crc8;
>>>>>> +     u8      mac0[6];
>>>>>> +     u8      mac1[6];
>>>>>> +     u8      item_prefix;    /* H/F is coded in MSbits, Vendor coding starts at LSbits */
>>>>>> +     u8      item_num[3];    /* [2] is MSbyte */
>>>>>> +     u8      serial[9];      /* [8] is MSbyte */
>>>>>> +} __packed;
>>>>>
>>>>> Is __packed needed ?
>>>>
>>>> I want to avoid padding in any case.
>>> Every single field is u8, do you observe any padding ?
>>
>> In this case, I don't expect padding, but if the struct is extended,
>> padding could be possible.
> 
> Please add the padding only once it is really required, ideally the
> design of the structure or its extension(s) would be able to avoid such
> necessity altogether.
> 
>> This struct is for an EEPROM and therefore
>> it should be defined with no padding. I don't want to give the compile
>> the change to add padding if something changed in the struct.
>> What are the disadvantages of keeping __packed here?
> If it is unnecessary, then please remove it, there is no good
> justification for packing a structure which is naturally aligned already.

OK. I will remove it in V2.

Regards
Christoph

RE: [PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM

[Christoph Niedermaier]

From: Marek Vasut <marex@denx.de>
Sent: Thursday, October 17, 2024 8:35 PM
> On 10/17/24 1:55 PM, Christoph Niedermaier wrote:
> 
> [...]
> 
>>>>>> +                            __func__, ret);
>>>>>
>>>>> This will be printed on every device, even the ones without ID EEPROM,
>>>>> correct ? This should not be printed on devices without ID EEPROM. Also,
>>>>
>>>> This is suppressed by the -ENOENT check.
>>>
>>> Does i2c_eeprom_read() in dh_get_value_from_eeprom_id_page() return
>>> -ENOENT in case the EEPROM is described in DT, but not populated on the
>>> board ? I suspect it returns some other error code, -ETIMEDOUT or
>>> -EINVAL maybe ?
>>
>> It could only be possible if the DTO for hardware rev. 100 (which has no
>> EEPROM ID page) is not loaded. The return value then is -ENODEV.
>> I will included this in v2.
> 
> OK
> 
>>>>> Also, this shouldn't be repeatedly reading the EEPROM, the EEPROM read
>>>>> operation is the slow part, the EEPROM is 32 bytes, so the EEPROM should
>>>>> be read once and cached, and once the cache is populated all read
>>>>> accesses to the EEPROM should use the cache.
>>>>
>>>> This is already covered in function dh_get_value_from_eeprom_id_page().
>>> It seems that function always calls
>>> ret = i2c_eeprom_read(dev, 0x0, eipa, sizeof(eipa));
>>> ?
>>
>> The 32 bytes are read into a static variable. If the ID (DHE) already exists
>> in it, the i2c_eeprom_read() function is no longer called.
> Hmmm, but it seems all the functions which access this EEPROM WLP are
> called from board_init(), are they not ?

That’s true.

> If yes, then there is no need for any static variable:
> 
> board_init() {
>   u8 eeprom[32];
>   dh_read_eeprom_wlp(eeprom); // read the eeprom once
>   dh_setup_mac_address(eeprom); // extract MAC from EEPROM
>   dh_add_item_number_and_serial_to_env(eeprom); // extract SN from EEPROM
>   // once this function exits, the eeprom variable on stack is discarded
>   // which is OK, since it won't be used anymore anyway
> }

The idea is that function dh_add_item_number_and_serial_to_env() encapsulates
the reading. I don't want to have to worry about the structure and number of
bytes of the EEPROM ID page and want to be independent of it. It is planned to
extend the structure and increase the number of bytes for the STM32MP2. The
changes to the size will then depend on the version of the data in the header
within the structure. However, these changes should then only have to be made
within the function dh_add_item_number_and_serial_to_env() for reading the
EEPROM ID page. I accept the static variable in order to better isolate the
function. Since the memory is freed up again when the operating system boots,
this consumption of 32 bytes in U-Boot is not a problem, because it is only
temporary.


Regards
Christoph

Re: [PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM

[Marek Vasut]

On 10/21/24 5:38 PM, Christoph Niedermaier wrote:

[...]

>> If yes, then there is no need for any static variable:
>>
>> board_init() {
>>    u8 eeprom[32];
>>    dh_read_eeprom_wlp(eeprom); // read the eeprom once
>>    dh_setup_mac_address(eeprom); // extract MAC from EEPROM
>>    dh_add_item_number_and_serial_to_env(eeprom); // extract SN from EEPROM
>>    // once this function exits, the eeprom variable on stack is discarded
>>    // which is OK, since it won't be used anymore anyway
>> }
> 
> The idea is that function dh_add_item_number_and_serial_to_env() encapsulates
> the reading.

The function which interacts with the EEPROM on start up, once, can have 
this name, sure.

> I don't want to have to worry about the structure and number of
> bytes of the EEPROM ID page and want to be independent of it. It is planned to
> extend the structure and increase the number of bytes for the STM32MP2. The
> changes to the size will then depend on the version of the data in the header
> within the structure.

It is OK to allocate 32 or 64 bytes on stack, since that chunk of memory 
is free'd on return from the function. The lifespan of that allocated 
memory is exactly as long as it should be, and it does not waste U-Boot 
memory unnecessarily.

So far, all known EEPROMs have WLP size of 32 or 64 Byes.

> However, these changes should then only have to be made
> within the function dh_add_item_number_and_serial_to_env() for reading the
> EEPROM ID page. I accept the static variable in order to better isolate the
> function.

This can very well be:

dh_add_item_number_and_serial_to_env() {
    u8 eeprom[32];
    dh_read_eeprom_wlp(eeprom); // read the eeprom once
    dh_setup_mac_address(eeprom); // extract MAC from EEPROM
    dh_add_item_number_and_serial_to_env(eeprom); // extract SN from EEPROM
    // once this function exits, the eeprom variable on stack is discarded
    // which is OK, since it won't be used anymore anyway
}

board_init() {
   ..
   dh_add_item_number_and_serial_to_env();
   ...
}

> Since the memory is freed up again when the operating system boots,
> this consumption of 32 bytes in U-Boot is not a problem, because it is only
> temporary.
This is not good, please do not waste memory in U-Boot if it can be 
easily avoided by automatically allocating it on stack and freeing it 
when not needed.

[...]

RE: [PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM

[Christoph Niedermaier]

From: Marek Vasut <marex@denx.de>
Sent: Tuesday, October 22, 2024 1:01 AM
> On 10/21/24 5:38 PM, Christoph Niedermaier wrote:
> 
> [...]
> 
>>> If yes, then there is no need for any static variable:
>>>
>>> board_init() {
>>>    u8 eeprom[32];
>>>    dh_read_eeprom_wlp(eeprom); // read the eeprom once
>>>    dh_setup_mac_address(eeprom); // extract MAC from EEPROM
>>>    dh_add_item_number_and_serial_to_env(eeprom); // extract SN from EEPROM
>>>    // once this function exits, the eeprom variable on stack is discarded
>>>    // which is OK, since it won't be used anymore anyway
>>> }
>>
>> The idea is that function dh_add_item_number_and_serial_to_env() encapsulates
>> the reading.
> 
> The function which interacts with the EEPROM on start up, once, can have
> this name, sure.

Sorry, I mean the function dh_get_value_from_eeprom_id_page() here.

>> I don't want to have to worry about the structure and number of
>> bytes of the EEPROM ID page and want to be independent of it. It is planned to
>> extend the structure and increase the number of bytes for the STM32MP2. The
>> changes to the size will then depend on the version of the data in the header
>> within the structure.
> 
> It is OK to allocate 32 or 64 bytes on stack, since that chunk of memory
> is free'd on return from the function. The lifespan of that allocated
> memory is exactly as long as it should be, and it does not waste U-Boot
> memory unnecessarily.
> 
> So far, all known EEPROMs have WLP size of 32 or 64 Byes.
> 
>> However, these changes should then only have to be made
>> within the function dh_add_item_number_and_serial_to_env() for reading the
>> EEPROM ID page. I accept the static variable in order to better isolate the
>> function.

Sorry, here I mean also the function dh_get_value_from_eeprom_id_page().

> This can very well be:
> 
> dh_add_item_number_and_serial_to_env() {
>     u8 eeprom[32];
>     dh_read_eeprom_wlp(eeprom); // read the eeprom once
>     dh_setup_mac_address(eeprom); // extract MAC from EEPROM
>     dh_add_item_number_and_serial_to_env(eeprom); // extract SN from EEPROM
>     // once this function exits, the eeprom variable on stack is discarded
>     // which is OK, since it won't be used anymore anyway
> }
> 
> board_init() {
>    ..
>    dh_add_item_number_and_serial_to_env();
>    ...
> }
> 
>> Since the memory is freed up again when the operating system boots,
>> this consumption of 32 bytes in U-Boot is not a problem, because it is only
>> temporary.
> This is not good, please do not waste memory in U-Boot if it can be
> easily avoided by automatically allocating it on stack and freeing it
> when not needed.
> 
> [...]

I don't want to allocate the memory in the function board_init(). I want to handle
size and caching in the function dh_get_value_from_eeprom_id_page(). So I don't
want to deal with size and structure in the function board_init(). For me, the use
of a static variable is OK, but you don't like it. Do you know how I can do this
without assigning a static variable, but not allocate the memory somewhere in a
caller function?


Regards
Christoph

Re: [PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM

[Marek Vasut]

On 10/22/24 11:31 AM, Christoph Niedermaier wrote:
> From: Marek Vasut <marex@denx.de>
> Sent: Tuesday, October 22, 2024 1:01 AM
>> On 10/21/24 5:38 PM, Christoph Niedermaier wrote:
>>
>> [...]
>>
>>>> If yes, then there is no need for any static variable:
>>>>
>>>> board_init() {
>>>>     u8 eeprom[32];
>>>>     dh_read_eeprom_wlp(eeprom); // read the eeprom once
>>>>     dh_setup_mac_address(eeprom); // extract MAC from EEPROM
>>>>     dh_add_item_number_and_serial_to_env(eeprom); // extract SN from EEPROM
>>>>     // once this function exits, the eeprom variable on stack is discarded
>>>>     // which is OK, since it won't be used anymore anyway
>>>> }
>>>
>>> The idea is that function dh_add_item_number_and_serial_to_env() encapsulates
>>> the reading.
>>
>> The function which interacts with the EEPROM on start up, once, can have
>> this name, sure.
> 
> Sorry, I mean the function dh_get_value_from_eeprom_id_page() here.

The function name doesn't really matter, please pick a fitting one.

>>> I don't want to have to worry about the structure and number of
>>> bytes of the EEPROM ID page and want to be independent of it. It is planned to
>>> extend the structure and increase the number of bytes for the STM32MP2. The
>>> changes to the size will then depend on the version of the data in the header
>>> within the structure.
>>
>> It is OK to allocate 32 or 64 bytes on stack, since that chunk of memory
>> is free'd on return from the function. The lifespan of that allocated
>> memory is exactly as long as it should be, and it does not waste U-Boot
>> memory unnecessarily.
>>
>> So far, all known EEPROMs have WLP size of 32 or 64 Byes.
>>
>>> However, these changes should then only have to be made
>>> within the function dh_add_item_number_and_serial_to_env() for reading the
>>> EEPROM ID page. I accept the static variable in order to better isolate the
>>> function.
> 
> Sorry, here I mean also the function dh_get_value_from_eeprom_id_page().
> 
>> This can very well be:
>>
>> dh_add_item_number_and_serial_to_env() {
>>      u8 eeprom[32];
>>      dh_read_eeprom_wlp(eeprom); // read the eeprom once
>>      dh_setup_mac_address(eeprom); // extract MAC from EEPROM
>>      dh_add_item_number_and_serial_to_env(eeprom); // extract SN from EEPROM
>>      // once this function exits, the eeprom variable on stack is discarded
>>      // which is OK, since it won't be used anymore anyway
>> }
>>
>> board_init() {
>>     ..
>>     dh_add_item_number_and_serial_to_env();
>>     ...
>> }
>>
>>> Since the memory is freed up again when the operating system boots,
>>> this consumption of 32 bytes in U-Boot is not a problem, because it is only
>>> temporary.
>> This is not good, please do not waste memory in U-Boot if it can be
>> easily avoided by automatically allocating it on stack and freeing it
>> when not needed.
>>
>> [...]
> 
> I don't want to allocate the memory in the function board_init().

You do not, the following automatically reserves space on stack when 
called and frees it up upon function return:

function foo() {
   u8 array[12];
   ...
   stuff(array);
   ...
}

> I want to handle
> size and caching in the function dh_get_value_from_eeprom_id_page(). So I don't
> want to deal with size and structure in the function board_init(). For me, the use
> of a static variable is OK, but you don't like it. Do you know how I can do this
> without assigning a static variable, but not allocate the memory somewhere in a
> caller function?
Even the static variable space is allocated somewhere, either in .bss or 
.data section, except with static variable(s) the memory persists 
throughout the lifetime of U-Boot because their content has to be 
retained even when the function using their content returns.

With variable(s) allocated on stack (which is majority of variable), the 
memory allocation happens on entry to the function and the memory is 
automatically freed on exit from the function.

It is perfectly fine to call some wrapper superfunction from 
board_init() which holds the array on stack and calls all the EEPROM 
parser/handler functions:

eeprom_superwrapper() {
   u8 eeprom[64];
   ...
   eeprom_do_stuff1(eeprom);
   eeprom_do_stuff2(eeprom);
   ...
}

board_init() {
   ...
   eeprom_superwrapper();
   ...
}

RE: [PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM

[Christoph Niedermaier]

From: Marek Vasut <marex@denx.de>
Sent: Tuesday, October 22, 2024 1:57 PM
> On 10/22/24 11:31 AM, Christoph Niedermaier wrote:
>> From: Marek Vasut <marex@denx.de>
>> Sent: Tuesday, October 22, 2024 1:01 AM
>>> On 10/21/24 5:38 PM, Christoph Niedermaier wrote:
>>>
>>> [...]
>>>
>>>>> If yes, then there is no need for any static variable:
>>>>>
>>>>> board_init() {
>>>>>     u8 eeprom[32];
>>>>>     dh_read_eeprom_wlp(eeprom); // read the eeprom once
>>>>>     dh_setup_mac_address(eeprom); // extract MAC from EEPROM
>>>>>     dh_add_item_number_and_serial_to_env(eeprom); // extract SN from EEPROM
>>>>>     // once this function exits, the eeprom variable on stack is discarded
>>>>>     // which is OK, since it won't be used anymore anyway
>>>>> }
>>>>
>>>> The idea is that function dh_add_item_number_and_serial_to_env() encapsulates
>>>> the reading.
>>>
>>> The function which interacts with the EEPROM on start up, once, can have
>>> this name, sure.
>>
>> Sorry, I mean the function dh_get_value_from_eeprom_id_page() here.
> 
> The function name doesn't really matter, please pick a fitting one.

It's not about the function name. I have referenced the wrong function.

>>>> I don't want to have to worry about the structure and number of
>>>> bytes of the EEPROM ID page and want to be independent of it. It is planned to
>>>> extend the structure and increase the number of bytes for the STM32MP2. The
>>>> changes to the size will then depend on the version of the data in the header
>>>> within the structure.
>>>
>>> It is OK to allocate 32 or 64 bytes on stack, since that chunk of memory
>>> is free'd on return from the function. The lifespan of that allocated
>>> memory is exactly as long as it should be, and it does not waste U-Boot
>>> memory unnecessarily.
>>>
>>> So far, all known EEPROMs have WLP size of 32 or 64 Byes.
>>>
>>>> However, these changes should then only have to be made
>>>> within the function dh_add_item_number_and_serial_to_env() for reading the
>>>> EEPROM ID page. I accept the static variable in order to better isolate the
>>>> function.
>>
>> Sorry, here I mean also the function dh_get_value_from_eeprom_id_page().
>>
>>> This can very well be:
>>>
>>> dh_add_item_number_and_serial_to_env() {
>>>      u8 eeprom[32];
>>>      dh_read_eeprom_wlp(eeprom); // read the eeprom once
>>>      dh_setup_mac_address(eeprom); // extract MAC from EEPROM
>>>      dh_add_item_number_and_serial_to_env(eeprom); // extract SN from EEPROM
>>>      // once this function exits, the eeprom variable on stack is discarded
>>>      // which is OK, since it won't be used anymore anyway
>>> }
>>>
>>> board_init() {
>>>     ..
>>>     dh_add_item_number_and_serial_to_env();
>>>     ...
>>> }
>>>
>>>> Since the memory is freed up again when the operating system boots,
>>>> this consumption of 32 bytes in U-Boot is not a problem, because it is only
>>>> temporary.
>>> This is not good, please do not waste memory in U-Boot if it can be
>>> easily avoided by automatically allocating it on stack and freeing it
>>> when not needed.
>>>
>>> [...]
>>
>> I don't want to allocate the memory in the function board_init().
> 
> You do not, the following automatically reserves space on stack when
> called and frees it up upon function return:
> 
> function foo() {
>    u8 array[12];
>    ...
>    stuff(array);
>    ...
> }

Sorry, I expressed myself incorrectly here. Of course I meant that
I don't want to reserve memory on the stack for the EEPROM ID page
in the board_init() function.

>> I want to handle
>> size and caching in the function dh_get_value_from_eeprom_id_page(). So I don't
>> want to deal with size and structure in the function board_init(). For me, the use
>> of a static variable is OK, but you don't like it. Do you know how I can do this
>> without assigning a static variable, but not allocate the memory somewhere in a
>> caller function?
> Even the static variable space is allocated somewhere, either in .bss or
> .data section, except with static variable(s) the memory persists
> throughout the lifetime of U-Boot because their content has to be
> retained even when the function using their content returns.
> 
> With variable(s) allocated on stack (which is majority of variable), the
> memory allocation happens on entry to the function and the memory is
> automatically freed on exit from the function.
> 
> It is perfectly fine to call some wrapper superfunction from
> board_init() which holds the array on stack and calls all the EEPROM
> parser/handler functions:
> 
> eeprom_superwrapper() {
>    u8 eeprom[64];
>    ...
>    eeprom_do_stuff1(eeprom);
>    eeprom_do_stuff2(eeprom);
>    ...
> }
> 
> board_init() {
>    ...
>    eeprom_superwrapper();
>    ...
> }

That's not what I'm looking for. Do you have another solution?

Regards
Christoph

Re: [PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM

[Marek Vasut]

On 10/23/24 2:18 PM, Christoph Niedermaier wrote:

[...]

>> You do not, the following automatically reserves space on stack when
>> called and frees it up upon function return:
>>
>> function foo() {
>>     u8 array[12];
>>     ...
>>     stuff(array);
>>     ...
>> }
> 
> Sorry, I expressed myself incorrectly here. Of course I meant that
> I don't want to reserve memory on the stack for the EEPROM ID page
> in the board_init() function.

Why ?

>>> I want to handle
>>> size and caching in the function dh_get_value_from_eeprom_id_page(). So I don't
>>> want to deal with size and structure in the function board_init(). For me, the use
>>> of a static variable is OK, but you don't like it. Do you know how I can do this
>>> without assigning a static variable, but not allocate the memory somewhere in a
>>> caller function?
>> Even the static variable space is allocated somewhere, either in .bss or
>> .data section, except with static variable(s) the memory persists
>> throughout the lifetime of U-Boot because their content has to be
>> retained even when the function using their content returns.
>>
>> With variable(s) allocated on stack (which is majority of variable), the
>> memory allocation happens on entry to the function and the memory is
>> automatically freed on exit from the function.
>>
>> It is perfectly fine to call some wrapper superfunction from
>> board_init() which holds the array on stack and calls all the EEPROM
>> parser/handler functions:
>>
>> eeprom_superwrapper() {
>>     u8 eeprom[64];
>>     ...
>>     eeprom_do_stuff1(eeprom);
>>     eeprom_do_stuff2(eeprom);
>>     ...
>> }
>>
>> board_init() {
>>     ...
>>     eeprom_superwrapper();
>>     ...
>> }
> 
> That's not what I'm looking for. Do you have another solution?
No. I do not understand what is the problem with allocating 64 Bytes on 
stack ?

RE: [PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM

[Christoph Niedermaier]

From: Marek Vasut <marex@denx.de>
Sent: Wednesday, October 23, 2024 2:41 PM
> On 10/23/24 2:18 PM, Christoph Niedermaier wrote:
> 
> [...]
> 
>>> You do not, the following automatically reserves space on stack when
>>> called and frees it up upon function return:
>>>
>>> function foo() {
>>>     u8 array[12];
>>>     ...
>>>     stuff(array);
>>>     ...
>>> }
>>
>> Sorry, I expressed myself incorrectly here. Of course I meant that
>> I don't want to reserve memory on the stack for the EEPROM ID page
>> in the board_init() function.
> 
> Why ?

It's about structuring the software. I want to isolate the function which
is responsible for evaluate the EEPROM from the caller function. I wanted
to avoid entangling the two. This means that I, as the caller of the function,
do not have to worry about the internal structure of the EEPROM ID page.

>>>> I want to handle
>>>> size and caching in the function dh_get_value_from_eeprom_id_page(). So I don't
>>>> want to deal with size and structure in the function board_init(). For me, the use
>>>> of a static variable is OK, but you don't like it. Do you know how I can do this
>>>> without assigning a static variable, but not allocate the memory somewhere in a
>>>> caller function?
>>> Even the static variable space is allocated somewhere, either in .bss or
>>> .data section, except with static variable(s) the memory persists
>>> throughout the lifetime of U-Boot because their content has to be
>>> retained even when the function using their content returns.
>>>
>>> With variable(s) allocated on stack (which is majority of variable), the
>>> memory allocation happens on entry to the function and the memory is
>>> automatically freed on exit from the function.
>>>
>>> It is perfectly fine to call some wrapper superfunction from
>>> board_init() which holds the array on stack and calls all the EEPROM
>>> parser/handler functions:
>>>
>>> eeprom_superwrapper() {
>>>     u8 eeprom[64];
>>>     ...
>>>     eeprom_do_stuff1(eeprom);
>>>     eeprom_do_stuff2(eeprom);
>>>     ...
>>> }
>>>
>>> board_init() {
>>>     ...
>>>     eeprom_superwrapper();
>>>     ...
>>> }
>>
>> That's not what I'm looking for. Do you have another solution?
> No. I do not understand what is the problem with allocating 64 Bytes on
> stack ?

There is no problem with the reservation of the EEPROM buffer on the
stack. I wanted to isolate the evaluation function from the caller
function, so that I have no dependencies on the caller. Since there
seems to be no better solution, I will add the variable to the
board_init() function in V2.


Regards
Christoph

Re: [PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM

[Marek Vasut]

On 10/23/24 3:20 PM, Christoph Niedermaier wrote:
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, October 23, 2024 2:41 PM
>> On 10/23/24 2:18 PM, Christoph Niedermaier wrote:
>>
>> [...]
>>
>>>> You do not, the following automatically reserves space on stack when
>>>> called and frees it up upon function return:
>>>>
>>>> function foo() {
>>>>      u8 array[12];
>>>>      ...
>>>>      stuff(array);
>>>>      ...
>>>> }
>>>
>>> Sorry, I expressed myself incorrectly here. Of course I meant that
>>> I don't want to reserve memory on the stack for the EEPROM ID page
>>> in the board_init() function.
>>
>> Why ?
> 
> It's about structuring the software. I want to isolate the function which
> is responsible for evaluate the EEPROM from the caller function. I wanted
> to avoid entangling the two. This means that I, as the caller of the function,
> do not have to worry about the internal structure of the EEPROM ID page.

You are only passing in a block of unstructured memory allocated on 
stack, u8 block[64].

>>>>> I want to handle
>>>>> size and caching in the function dh_get_value_from_eeprom_id_page(). So I don't
>>>>> want to deal with size and structure in the function board_init(). For me, the use
>>>>> of a static variable is OK, but you don't like it. Do you know how I can do this
>>>>> without assigning a static variable, but not allocate the memory somewhere in a
>>>>> caller function?
>>>> Even the static variable space is allocated somewhere, either in .bss or
>>>> .data section, except with static variable(s) the memory persists
>>>> throughout the lifetime of U-Boot because their content has to be
>>>> retained even when the function using their content returns.
>>>>
>>>> With variable(s) allocated on stack (which is majority of variable), the
>>>> memory allocation happens on entry to the function and the memory is
>>>> automatically freed on exit from the function.
>>>>
>>>> It is perfectly fine to call some wrapper superfunction from
>>>> board_init() which holds the array on stack and calls all the EEPROM
>>>> parser/handler functions:
>>>>
>>>> eeprom_superwrapper() {
>>>>      u8 eeprom[64];
>>>>      ...
>>>>      eeprom_do_stuff1(eeprom);
>>>>      eeprom_do_stuff2(eeprom);
>>>>      ...
>>>> }
>>>>
>>>> board_init() {
>>>>      ...
>>>>      eeprom_superwrapper();
>>>>      ...
>>>> }
>>>
>>> That's not what I'm looking for. Do you have another solution?
>> No. I do not understand what is the problem with allocating 64 Bytes on
>> stack ?
> 
> There is no problem with the reservation of the EEPROM buffer on the
> stack. I wanted to isolate the evaluation function from the caller
> function, so that I have no dependencies on the caller. Since there
> seems to be no better solution, I will add the variable to the
> board_init() function in V2.
There seem to be multiple functions using the same data, so you have to 
pass pointer to those data around.

RE: [PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM

[Christoph Niedermaier]

From: Marek Vasut <marex@denx.de>
Sent: Wednesday, October 23, 2024 4:05 PM
> On 10/23/24 3:20 PM, Christoph Niedermaier wrote:
>> From: Marek Vasut <marex@denx.de>
>> Sent: Wednesday, October 23, 2024 2:41 PM
>>> On 10/23/24 2:18 PM, Christoph Niedermaier wrote:
>>>
>>> [...]
>>>
>>>>> You do not, the following automatically reserves space on stack when
>>>>> called and frees it up upon function return:
>>>>>
>>>>> function foo() {
>>>>>      u8 array[12];
>>>>>      ...
>>>>>      stuff(array);
>>>>>      ...
>>>>> }
>>>>
>>>> Sorry, I expressed myself incorrectly here. Of course I meant that
>>>> I don't want to reserve memory on the stack for the EEPROM ID page
>>>> in the board_init() function.
>>>
>>> Why ?
>>
>> It's about structuring the software. I want to isolate the function which
>> is responsible for evaluate the EEPROM from the caller function. I wanted
>> to avoid entangling the two. This means that I, as the caller of the function,
>> do not have to worry about the internal structure of the EEPROM ID page.
> 
> You are only passing in a block of unstructured memory allocated on
> stack, u8 block[64].
> 
>>>>>> I want to handle
>>>>>> size and caching in the function dh_get_value_from_eeprom_id_page(). So I don't
>>>>>> want to deal with size and structure in the function board_init(). For me, the use
>>>>>> of a static variable is OK, but you don't like it. Do you know how I can do this
>>>>>> without assigning a static variable, but not allocate the memory somewhere in a
>>>>>> caller function?
>>>>> Even the static variable space is allocated somewhere, either in .bss or
>>>>> .data section, except with static variable(s) the memory persists
>>>>> throughout the lifetime of U-Boot because their content has to be
>>>>> retained even when the function using their content returns.
>>>>>
>>>>> With variable(s) allocated on stack (which is majority of variable), the
>>>>> memory allocation happens on entry to the function and the memory is
>>>>> automatically freed on exit from the function.
>>>>>
>>>>> It is perfectly fine to call some wrapper superfunction from
>>>>> board_init() which holds the array on stack and calls all the EEPROM
>>>>> parser/handler functions:
>>>>>
>>>>> eeprom_superwrapper() {
>>>>>      u8 eeprom[64];
>>>>>      ...
>>>>>      eeprom_do_stuff1(eeprom);
>>>>>      eeprom_do_stuff2(eeprom);
>>>>>      ...
>>>>> }
>>>>>
>>>>> board_init() {
>>>>>      ...
>>>>>      eeprom_superwrapper();
>>>>>      ...
>>>>> }
>>>>
>>>> That's not what I'm looking for. Do you have another solution?
>>> No. I do not understand what is the problem with allocating 64 Bytes on
>>> stack ?
>>
>> There is no problem with the reservation of the EEPROM buffer on the
>> stack. I wanted to isolate the evaluation function from the caller
>> function, so that I have no dependencies on the caller. Since there
>> seems to be no better solution, I will add the variable to the
>> board_init() function in V2.
> There seem to be multiple functions using the same data, so you have to
> pass pointer to those data around.

I will make a V2 with this approach, but unfortunately I can't work on
it next week. So the week after I will send V2.

Regards
Christoph