Re: [RFC 05/10] eeprom: starfive: Update eeprom data format version to 3

[E Shattow <e@freeshell.de>]

On 8/28/25 23:09, Hal Feng wrote:
> Add eeprom data format v3 support.
> Add wifi_bt field in ATOM4 and add "mac wifi_bt <?>" command to modify it.
> Set or show eth0/1 MAC address only if the board has eth0/1.
> 
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  .../visionfive2/visionfive2-i2c-eeprom.c      | 91 ++++++++++++++++---
>  1 file changed, 78 insertions(+), 13 deletions(-)
> 
> diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> index 43b8af4fc59..7352252c475 100644
> --- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> +++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> @@ -11,7 +11,7 @@
>  #include <u-boot/crc.h>
>  #include <linux/delay.h>
>  
> -#define FORMAT_VERSION		0x2
> +#define FORMAT_VERSION		0x3

No. This stays FORMAT_VERSION 0x2 for compatibility, allow to write
EEPROM in the "old" format on boards. We must allow compatible action
for vendor BSP firmware and weird old software that we do not want
responsibility for updating.

Update to 0x3 only if some data field is expressed that needs 0x3 format.

>  #define PCB_VERSION		0xB1
>  #define BOM_VERSION		'A'
>  /*
> @@ -105,7 +105,8 @@ struct eeprom_atom4_data {
>  	u8 bom_revision;		/* BOM version */
>  	u8 mac0_addr[MAC_ADDR_BYTES];	/* Ethernet0 MAC */
>  	u8 mac1_addr[MAC_ADDR_BYTES];	/* Ethernet1 MAC */
> -	u8 reserved[2];
> +	u8 wifi_bt;			/* WIFI/BT support flag */

Are you sure a whole u8 for a one-bit flag?

> +	u8 reserved;
>  };
>  
>  struct starfive_eeprom_atom4 {
> @@ -128,6 +129,35 @@ static union {
>  /* Set to 1 if we've read EEPROM into memory */
>  static int has_been_read __section(".data");
>  
> +static const char * const board_no_eth0_list[] = {
> +	"FML13V01",
> +};
> +
> +static const char * const board_no_eth1_list[] = {
> +	"FML13V01",
> +	"MARS",
> +	"VF7110SL",
> +};
> +
> +static bool board_have_eth(u8 eth)
> +{
> +	int i;
> +
> +	if (eth == 0) {
> +		for (i = 0 ; i < ARRAY_SIZE(board_no_eth0_list); i++)
> +			if (!strncmp(pbuf.eeprom.atom1.data.pstr, board_no_eth0_list[i],
> +				     strlen(board_no_eth0_list[i])))
> +				return false;
> +	} else {
> +		for (i = 0 ; i < ARRAY_SIZE(board_no_eth1_list); i++)
> +			if (!strncmp(pbuf.eeprom.atom1.data.pstr, board_no_eth1_list[i],
> +				     strlen(board_no_eth1_list[i])))
> +				return false;
> +	}
> +
> +	return true;
> +}
> +

No. Drop all this no_eth0 no_eth1 configuration, the string arrays, it
exists only to show or hide some EEPROM configuration text and can be
deleted.

>  static inline int is_match_magic(void)
>  {
>  	return strncmp(pbuf.eeprom.header.signature, STARFIVE_EEPROM_HATS_SIG,
> @@ -176,17 +206,22 @@ static void show_eeprom(void)
>  	printf("Vendor : %s\n", pbuf.eeprom.atom1.data.vstr);
>  	printf("Product full SN: %s\n", pbuf.eeprom.atom1.data.pstr);
>  	printf("data version: 0x%x\n", pbuf.eeprom.atom4.data.version);
> -	if (pbuf.eeprom.atom4.data.version == 2) {

> +	if (pbuf.eeprom.atom4.data.version >= 2 && pbuf.eeprom.atom4.data.version <= 3) {

This is not some floating point value. Compare with discrete values:

+ if (pbuf.eeprom.atom4.data.version == 2 ||
+     pbuf.eeprom.atom4.data.version == 3) {

or else I think more readable is convert to a switch case conditional:

+ switch (pbuf.eeprom.atom4.data.version) {
+ case 2:
+ case 3:
		...

>  		printf("PCB revision: 0x%x\n", pbuf.eeprom.atom4.data.pcb_revision);
>  		printf("BOM revision: %c\n", pbuf.eeprom.atom4.data.bom_revision);
> -		printf("Ethernet MAC0 address: %02x:%02x:%02x:%02x:%02x:%02x\n",
> -		       pbuf.eeprom.atom4.data.mac0_addr[0], pbuf.eeprom.atom4.data.mac0_addr[1],
> -		       pbuf.eeprom.atom4.data.mac0_addr[2], pbuf.eeprom.atom4.data.mac0_addr[3],
> -		       pbuf.eeprom.atom4.data.mac0_addr[4], pbuf.eeprom.atom4.data.mac0_addr[5]);
> -		printf("Ethernet MAC1 address: %02x:%02x:%02x:%02x:%02x:%02x\n",
> -		       pbuf.eeprom.atom4.data.mac1_addr[0], pbuf.eeprom.atom4.data.mac1_addr[1],
> -		       pbuf.eeprom.atom4.data.mac1_addr[2], pbuf.eeprom.atom4.data.mac1_addr[3],
> -		       pbuf.eeprom.atom4.data.mac1_addr[4], pbuf.eeprom.atom4.data.mac1_addr[5]);
> +		if (board_have_eth(0))
> +			printf("Ethernet MAC0 address: %02x:%02x:%02x:%02x:%02x:%02x\n",
> +			pbuf.eeprom.atom4.data.mac0_addr[0], pbuf.eeprom.atom4.data.mac0_addr[1],
> +			pbuf.eeprom.atom4.data.mac0_addr[2], pbuf.eeprom.atom4.data.mac0_addr[3],
> +			pbuf.eeprom.atom4.data.mac0_addr[4], pbuf.eeprom.atom4.data.mac0_addr[5]);
> +
> +		if (board_have_eth(1))

No. There is not any purpose to hide this data from the user. In fact it
is useful when Mars CM Lite SoM installed to carrier board that has its
own additional network interface on PCIe bus, setting eth2addr
environment variable to be the same as (disabled) MAC1 from EEPROM print
output.

> +			printf("Ethernet MAC1 address: %02x:%02x:%02x:%02x:%02x:%02x\n",
> +			pbuf.eeprom.atom4.data.mac1_addr[0], pbuf.eeprom.atom4.data.mac1_addr[1],
> +			pbuf.eeprom.atom4.data.mac1_addr[2], pbuf.eeprom.atom4.data.mac1_addr[3],
> +			pbuf.eeprom.atom4.data.mac1_addr[4], pbuf.eeprom.atom4.data.mac1_addr[5]);
> +		if (pbuf.eeprom.atom4.data.version >= 3)
> +			printf("WIFI/BT support: %x\n", pbuf.eeprom.atom4.data.wifi_bt);
+break;

+ default:

>  	} else {
>  		printf("Custom data v%d is not Supported\n", pbuf.eeprom.atom4.data.version);
>  		dump_raw_eeprom();
> @@ -260,6 +295,7 @@ static void init_local_copy(void)
>  	pbuf.eeprom.atom4.data.bom_revision = BOM_VERSION;
>  	set_mac_address(STARFIVE_DEFAULT_MAC0, 0);
>  	set_mac_address(STARFIVE_DEFAULT_MAC1, 1);
> +	pbuf.eeprom.atom4.data.wifi_bt = 0;
+ break;
>  }
>  
>  /**
> @@ -385,6 +421,28 @@ static void set_bom_revision(char *string)
>  	update_crc();
>  }
>  
> +/**
> + * set_wifi_bt() - stores a StarFive WIFI/BT support flag into the local EEPROM copy
> + *
> + * Takes a pointer to a string representing the numeric WIFI/BT support flag in
> + * decimal ("0" or "1"), stores it in the wifi_bt field of the
> + * EEPROM local copy, and updates the CRC of the local copy.
> + */
> +static void set_wifi_bt(char *string)
> +{
> +	u8 wifi_bt;
> +
> +	wifi_bt = simple_strtoul(string, &string, 16);
> +	if (wifi_bt > 1) {
> +		printf("WIFI/BT support flag must not be greater than 1");
> +		return;
> +	}
> +
> +	pbuf.eeprom.atom4.data.wifi_bt = wifi_bt;
> +
> +	update_crc();
> +}
> +

I am surprised this wifi_bt u8 is a 1-bit flag and does not contain any
mac address. What is the SDIO module present on VisionFive2 Lite and
does it contain memory for WiFi MAC address and Bluetooth MAC address?

>  /**
>   * set_product_id() - stores a StarFive product ID into the local EEPROM copy
>   *
> @@ -478,6 +536,9 @@ int do_mac(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  	} else if (!strcmp(cmd, "bom_revision")) {
>  		set_bom_revision(argv[2]);
>  		return 0;
> +	} else if (!strcmp(cmd, "wifi_bt")) {
> +		set_wifi_bt(argv[2]);
> +		return 0;
>  	} else if (!strcmp(cmd, "product_id")) {
>  		set_product_id(argv[2]);
>  		return 0;
> @@ -513,8 +574,10 @@ int mac_read_from_eeprom(void)
>  	}
>  
>  	// 1, setup ethaddr env
> -	eth_env_set_enetaddr("ethaddr", pbuf.eeprom.atom4.data.mac0_addr);
> -	eth_env_set_enetaddr("eth1addr", pbuf.eeprom.atom4.data.mac1_addr);
> +	if (board_have_eth(0))
> +		eth_env_set_enetaddr("ethaddr", pbuf.eeprom.atom4.data.mac0_addr);
> +	if (board_have_eth(1))
> +		eth_env_set_enetaddr("eth1addr", pbuf.eeprom.atom4.data.mac1_addr);
>  

No. Unnecessary complication to have a list of products and do all this
extra work. We should only update fields that the user has set.

>  	/**
>  	 * 2, setup serial# env, reference to hifive-platform-i2c-eeprom.c,
> @@ -585,6 +648,8 @@ U_BOOT_LONGHELP(mac,
>  	"    - stores a StarFive PCB revision into the local EEPROM copy\n"
>  	"mac bom_revision <A>\n"
>  	"    - stores a StarFive BOM revision into the local EEPROM copy\n"
> +	"mac wifi_bt <?>\n"
> +	"    - stores a StarFive WIFI/BT support flag into the local EEPROM copy\n"
>  	"mac product_id <VF7110A1-2228-D008E000-xxxxxxxx>\n"
>  	"    - stores a StarFive product ID into the local EEPROM copy\n"
>  	"mac vendor <Vendor Name>\n"

NAK to this patch.

Add the support for the new EEPROM format and keep it simple:

- It is okay if the user sees print of all valid fields of EEPROM, even
when some printed data does not have any importance for them. The format
version will hide or show some data fields - okay.

- For compatibility keep FORMAT_VERSION (0x2) default and each field
being set should do a check with pbuf.eeprom.header.version ; for
example if there is wifi_bt being set then it is appropriate to promote
format version (0x3) and print info that the format version has changed.

- For explicit write operation to EEPROM of format version 0x3 but user
did not set any data fields that need more than format version (0x2),
then the user must set format version to 0x3. Make it possible to set
the format version directly.

If you want to get complicated and add conditional for promotion of
minimum format version required by each data field, okay then, I would
like to see that.

-E