Re: [PATCH v1 5/9] eeprom: starfive: Support eeprom data format v3

E Shattow
2025-10-24T12:41:12Z

On 10/24/25 01:59, Hal Feng wrote:
> Add eeprom data format v3 support. Add onboard_module field in
> ATOM4 and add "mac onboard_module <?>" command to modify it.
>
> The onboard module field marks the additional modules compared
> with VisionFive 2 board. Now we define
>
> bit7-1: reserved, bit0: WIFI/BT
>
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
> .../visionfive2/visionfive2-i2c-eeprom.c | 36 +++++++++++++++++--
> 1 file changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> index 986dcc94992..b9197cdd34f 100644
> --- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> +++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> @@ -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 onboard_module; /* Onboard module flag: bit7-1: reserved, bit0: WIFI/BT */
> + u8 reserved;
> };
>
> struct starfive_eeprom_atom4 {
> @@ -176,7 +177,7 @@ 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) {

small nit, maybe as two lines:

if (pbuf.eeprom.atom4.data.version == 2 ||
pbuf.eeprom.atom4.data.version == 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",
> @@ -187,6 +188,14 @@ static void show_eeprom(void)
> 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]);

I guess that the previous author must have 100col (?) not 80col for
editing. It is readable in 100col but just a little unusual on a
collaborative project in git repo to write code this way. It is not
anything you are the author for, so ignore my review comment here.

> + if (pbuf.eeprom.atom4.data.version == 3) {
> + char str[25] = "Onboard module: ";
> +
> + if (pbuf.eeprom.atom4.data.onboard_module & BIT(0))
> + strcat(str, "WIFI/BT");
> +
> + printf("%s\n", str);
> + }

I am concerned about a memory safety code mistake in future with this.
Let the compiler do our work for us. Avoid strcat or requirement that
the programmer knows buffer length for a scoped string allocation and
several other actions, as:

if (pbuf.eeprom.atom4.data.version == 3) {
printf("Onboard module: %s\n",
(pbuf.eeprom.atom4.data.onboard_module & BIT(0) ?
"WIFI/BT" :
"None"));
}

> } else {
> printf("Custom data v%d is not Supported\n", pbuf.eeprom.atom4.data.version);
> dump_raw_eeprom();
> @@ -260,6 +269,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.onboard_module = 0;
> }
>
> /**
> @@ -385,6 +395,23 @@ static void set_bom_revision(char *string)
> update_crc();
> }
>
> +/**
> + * set_onboard_module() - stores a StarFive onboard module flag into the local EEPROM copy
> + *
> + * Takes a pointer to a string representing the numeric onboard module flag in
> + * Hexadecimal ("0" - "FF"), stores it in the onboard_module field of the
> + * EEPROM local copy, and updates the CRC of the local copy.
> + */
> +static void set_onboard_module(char *string)
> +{
> + u8 onboard_module;
> +
> + onboard_module = simple_strtoul(string, &string, 16);
> + pbuf.eeprom.atom4.data.onboard_module = onboard_module;
> +
> + update_crc();
> +}
> +
> /**
> * set_product_id() - stores a StarFive product ID into the local EEPROM copy
> *
> @@ -478,6 +505,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, "onboard_module")) {
> + set_onboard_module(argv[2]);
> + return 0;
> } else if (!strcmp(cmd, "product_id")) {
> set_product_id(argv[2]);
> return 0;
> @@ -585,6 +615,8 @@ U_BOOT_LONGHELP(mac,
> " - stores a StarFive PCB revision into the local EEPROM copy\n"
> "mac bom_revision \n"
> " - stores a StarFive BOM revision into the local EEPROM copy\n"
> + "mac onboard_module <?>\n"

Seeing the example of product_id would this onboard_module flag be
described as <xx> and not <?> ?

I understand that pcb_revision is stated as <?> however a pcb_revision
of zero value is not a sensible value because of the error handling, so
that can be different.

> + " - stores a StarFive onboard module 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"

Looks good to me with only some nits about style choices. With that,

Reviewed-by: E Shattow <e@freeshell.de>