From: Marek Vasut <ma...@denx.de>
Sent: Friday, November 8, 2024 8:27 PM
> On 11/8/24 6:06 PM, Christoph Niedermaier wrote:
>
> [...]
>
>> +int dh_read_eeprom_id_page(u8 *eeprom_buffer, const char *alias)
>> +{
>> + u8 buffer[DH_EEPROM_ID_PAGE_MAX_SIZE] = { 0 };
>> + struct eeprom_id_page *eipp;
>> + struct udevice *dev;
>> + int eeprom_size;
>> + char path[128];
>> + ofnode node;
>> + int len;
>> + int ret;
>> + u8 c8;
>
> A more descriptive variable name would be useful. What does 'c8' mean ?
It means CRC8 checksum.
I will rename it in V3.
>> + 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';
>
> Simply add alias to eeprom0wl/eeprom1wl and outright look up the alias:
>
> arch/arm/dts/imx8mp-dhcom-u-boot.dtsi
> "
> #include "imx8mp-u-boot.dtsi"
>
> / {
> aliases {
> eeprom0 = &eeprom0;
> eeprom1 = &eeprom1;
> + eeprom0wl = &eeprom0wl;
> + eeprom1wl = &eeprom1wl;
> mmc0 = &usdhc2; /* MicroSD */
> mmc1 = &usdhc3; /* eMMC */
> mmc2 = &usdhc1; /* SDIO */
> };
> };
> "
>
> node = ofnode_path(alias); // use the eeprom0wl/eeprom1wl alias
>
I will change this in V3.
>> + 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;
>> +
>> + eeprom_size = ofnode_read_u32_default(node, "pagesize", 32);
>
> There are two problems here, pagesize != total EEPROM size and pagesize
> may not be specified in DT but can still be inferred from compatible
> string. Better use:
>
> board/toradex/common/tdx-eeprom.c
>
> uclass_get_device_by_ofnode(UCLASS_I2C_EEPROM, eeprom, devp);
>
> include/i2c_eeprom.h
>
> int i2c_eeprom_size(struct udevice *dev);
>
> to get the EEPROM size . 24C32 WLP compatible string is already present
> in drivers/misc/i2c_eeprom.c since:
>
> 3c11643b1915 ("eeprom: at24: add ST M24C32-D Additional Write lockable page
> support")
I will change it in V3.
>> + if (eeprom_size > DH_EEPROM_ID_PAGE_MAX_SIZE) {
>> + eeprom_size = DH_EEPROM_ID_PAGE_MAX_SIZE;
>> + log_warning("Warning: Read data from EEPROM ID page truncated
>> to %d bytes\n",
>> + DH_EEPROM_ID_PAGE_MAX_SIZE);
>> + }
>> +
>> + ret = uclass_get_device_by_ofnode(UCLASS_I2C_EEPROM, node, &dev);
>> + if (ret) {
>> + printf("%s: Cannot find ID page! Check DT, maybe EEPROM ID
>> page is enabled but not populated! ret = %d\n",
>> + __func__, ret);
>
> Use log_warning() consistently .
I will use only printf for consistency in V3.
>> + return ret;
>> + }
>> +
>> + ret = i2c_eeprom_read(dev, 0x0, buffer, eeprom_size);
>> + if (ret) {
>> + printf("%s: Error reading ID page! ret = %d\n", __func__, ret);
>> + return ret;
>> + }
>> +
>> + eipp = (struct eeprom_id_page *)buffer;
>> +
>> + /* Validate header magic */
>> + if (eipp->id[0] != 'D' || eipp->id[1] != 'H' || eipp->id[2] != 'E')
>> + return -EINVAL;
>> +
>> + /* Validate header checksum */
>> + c8 = crc8(0xff, buffer, offsetof(struct eeprom_id_page, header_crc8));
>> + if (eipp->header_crc8 != c8)
>> + return -EINVAL;
>> +
>> + /* Here the data has a valid header, so that all data can be copied */
>> + memcpy(eeprom_buffer, buffer, eeprom_size);
>
> No need for the extra local buffer, drop the memcpy() and use
> eeprom_buffer directly. If the data in eeprom_buffer would be garbage
> for whatever reason, this function would return -ve anyway and the
> called should not use the content of eeprom_buffer .
I will change it in V3
>> +
>> + return 0;
>> +}
>> +
>> +int dh_get_value_from_eeprom_buffer(enum eip_request_values request, u8
>> *data, int data_len,
>> + u8 *eeprom_buffer)
>> +{
>> + struct eeprom_id_page *eipp;
>
> struct eeprom_id_page *eipp = eeprom_buffer;
I will change it in V3.
> > + char soc;
> > + u16 c16;
> > + u8 c8;
>
> A more descriptive variable name would be helpful.
I will rename both in V3.
>> + eipp = (struct eeprom_id_page *)eeprom_buffer;
>> +
>> + /* Validate header magic */
>> + if (eipp->id[0] != 'D' || eipp->id[1] != 'H' || eipp->id[2] != 'E')
>> + return -EINVAL;
>> +
>> + /* Validate header checksum */
>> + c8 = crc8(0xff, eeprom_buffer, offsetof(struct eeprom_id_page,
>> header_crc8));
>> + if (eipp->header_crc8 != c8)
>> + return -EINVAL;
>> +
>> + /* Validate header version */
>> + if (eipp->version != 0x10)
>
> This could use a macro for 0x10.
I will use one in V3.
>> + return -EINVAL;
>> +
>> + /* Validate structure checksum */
>> + c16 = crc16(0xffff, eeprom_buffer + 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;
>
> This entire header and payload validation can be done once in
> dh_read_eeprom_id_page(), no need to do it every time this function is
> called on in-memory buffer. Doing so saves you multiple crc8/crc16
> passes and some wasted CPU cycles.
I will move it to the function dh_read_eeprom_id_page().
>> + /* Copy requested data */
>> + switch (request) {
>> + case MAC0:
>> + if (!is_valid_ethaddr(eipp->mac0))
>> + return -EINVAL;
>> + if (data_len >= sizeof(eipp->mac0))
>
> Can you return pointer into the eeprom buffer instead of this memcpy ?
> That would save you unnecessary memory copying.
I will try to change it in V3.
>> + 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;
>> + case DH_ITEM_NUMBER:
>> + if (data_len < 8) /* String length must be 7 characters +
>> string termination */
>> + return -EINVAL;
>> +
>
> const u8 item_prefix = eipp->item_prefix & 0xf;
>
> if (item_prefix == DH_ITEM_PREFIX_NXP)
> soc = DH_ITEM_PREFIX_NXP_CHR;
> else if (item_prefix == DH_ITEM_PREFIX_ST)
> soc = DH_ITEM_PREFIX_ST_CHR;
> else
> return -EINVAL
>
> This has fewer lines.
I will change it in V3.
>> + switch (eipp->item_prefix & 0xf) {
>> + case DH_ITEM_PREFIX_NXP:
>> + soc = DH_ITEM_PREFIX_NXP_CHR;
>> + break;
>> + case DH_ITEM_PREFIX_ST:
>> + soc = DH_ITEM_PREFIX_ST_CHR;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + snprintf(data, data_len, "%c%c%05d",
>
> Using a temporary variable for this item_prefix would likely help
> readability:
>
>
> const char fin_chr = (eipp->item_prefix & DH_ITEM_PREFIX_FIN_BIT) ?
> DH_ITEM_PREFIX_FIN_FLASHED_CHR :
> DH_ITEM_PREFIX_FIN_HALF_CHR;
> ...
> snprintf(data, data_len, "%c%c%05d", fin_chr, soc_chr, ...
I will use a temporary variable in V3.
>> + (eipp->item_prefix & DH_ITEM_PREFIX_FIN_BIT) ?
>> + DH_ITEM_PREFIX_FIN_FLASHED_CHR :
>> DH_ITEM_PREFIX_FIN_HALF_CHR,
>> + soc, (eipp->item_num[0] << 16) | (eipp->item_num[1]
>> << 8)
>> + | eipp->item_num[2]);
>> + break;
>> + case DH_SERIAL_NUMBER:
>> + /*
>> + * data_len must be greater than the size of eipp->serial,
>> + * because there is a string termination needed.
>> + */
>> + if (data_len <= sizeof(eipp->serial))
>> + return -EINVAL;
>> +
>> + data[sizeof(eipp->serial)] = 0;
>> + memcpy(data, eipp->serial, sizeof(eipp->serial));
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> int dh_get_mac_from_eeprom(unsigned char *enetaddr, const char *alias)
>> {
>> struct udevice *dev;
>> @@ -62,7 +255,7 @@ int dh_get_mac_from_eeprom(unsigned char *enetaddr, const
>> char *alias)
>> return 0;
>> }
>>
>> -__weak int dh_setup_mac_address(void)
>> +__weak int dh_setup_mac_address(u8 *eeprom_buffer)
>> {
>> unsigned char enetaddr[6];
>>
>> @@ -72,6 +265,9 @@ __weak int dh_setup_mac_address(void)
>> if (dh_get_mac_is_enabled("ethernet0"))
>> return 0;
>>
>> + if (!dh_get_value_from_eeprom_buffer(MAC0, enetaddr, sizeof(enetaddr),
>> eeprom_buffer))
>> + return 0;
>
> Is this missing some eth_env_set_enetaddr("ethaddr", enetaddr); here ?
I will fix this in V3.
>> +
>> if (!dh_get_mac_from_eeprom(enetaddr, "eeprom0"))
>> return eth_env_set_enetaddr("ethaddr", enetaddr);
>>
>> diff --git a/board/dhelectronics/common/dh_common.h
>> b/board/dhelectronics/common/dh_common.h
>> index a2de5b1553..60806c452a 100644
>> --- a/board/dhelectronics/common/dh_common.h
>> +++ b/board/dhelectronics/common/dh_common.h
>> @@ -3,6 +3,15 @@
>> * Copyright 2022 DENX Software Engineering GmbH, Philip Oberfichtner
>> <p...@denx.de>
>> */
>>
>> +#define DH_EEPROM_ID_PAGE_MAX_SIZE 64
>> +
>> +enum eip_request_values {
>> + MAC0,
>> + MAC1,
>
> Please be consistent with the prefixes, DH_MAC...
I will rename both in V3.
>> + DH_ITEM_NUMBER,
>> + DH_SERIAL_NUMBER,
>> +};
>> +
>> /*
>> * dh_mac_is_in_env - Check if MAC address is already set
>> *
>
> [...]
>
>> +++ b/board/dhelectronics/dh_imx8mp/imx8mp_dhcom_pdk2.c
>> @@ -40,7 +40,26 @@ int board_phys_sdram_size(phys_size_t *size)
>> return 0;
>> }
>>
>> -static int dh_imx8_setup_ethaddr(void)
>> +int dh_get_som_rev(void)
>> +{
>> + int ret;
>> +
>> + /*
>> + * The hardware revision numbers are binary coded with 3 GPIOs:
>> + * 0x0 = Rev. 100
>> + * 0x1 = Rev. 200
>> + * 0x2 = Rev. 300
>> + * ...
>> + */
>> + ret = !!(readl(GPIO3_BASE_ADDR) & BIT(14));
>> + ret |= !!(readl(GPIO4_BASE_ADDR) & BIT(19)) << 1;
>> + ret |= !!(readl(GPIO3_BASE_ADDR) & BIT(25)) << 2;
>> + ret = ret * 100 + 100;
>
> This entire function will be unnecessary, see below (*) ...
I will remove it in V3.
>> + return ret;
>> +}
>> +
>> +static int dh_imx8_setup_ethaddr(u8 *eeprom_buffer)
>> {
>> unsigned char enetaddr[6];
>>
>> @@ -53,6 +72,11 @@ static int dh_imx8_setup_ethaddr(void)
>> if (!dh_imx_get_mac_from_fuse(enetaddr))
>> goto out;
>>
>> + /* The EEPROM ID page is available on SoM rev. 200 and greater. */
>> + if ((dh_get_som_rev() > 100) &&
>> + (!dh_get_value_from_eeprom_buffer(MAC0, enetaddr,
>> sizeof(enetaddr), eeprom_buffer)))
>> + goto out;
>> +
>> if (!dh_get_mac_from_eeprom(enetaddr, "eeprom0"))
>> goto out;
>>
>> @@ -62,7 +86,7 @@ out:
>> return eth_env_set_enetaddr("ethaddr", enetaddr);
>> }
>>
>> -static int dh_imx8_setup_eth1addr(void)
>> +static int dh_imx8_setup_eth1addr(u8 *eeprom_buffer)
>> {
>> unsigned char enetaddr[6];
>>
>> @@ -75,6 +99,11 @@ static int dh_imx8_setup_eth1addr(void)
>> if (!dh_imx_get_mac_from_fuse(enetaddr))
>> goto increment_out;
>>
>> + /* The EEPROM ID page is available on SoM rev. 200 and greater. */
>> + if ((dh_get_som_rev() > 100) &&
>> + (!dh_get_value_from_eeprom_buffer(MAC1, enetaddr,
>> sizeof(enetaddr), eeprom_buffer)))
>> + goto out;
>> +
>> if (!dh_get_mac_from_eeprom(enetaddr, "eeprom1"))
>> goto out;
>>
>> @@ -95,21 +124,58 @@ out:
>> return eth_env_set_enetaddr("eth1addr", enetaddr);
>> }
>>
>> -int dh_setup_mac_address(void)
>> +int dh_setup_mac_address(u8 *eeprom_buffer)
>> {
>> int ret;
>>
>> - ret = dh_imx8_setup_ethaddr();
>> + ret = dh_imx8_setup_ethaddr(eeprom_buffer);
>> if (ret)
>> printf("%s: Unable to setup ethaddr! ret = %d\n", __func__,
>> ret);
>>
>> - ret = dh_imx8_setup_eth1addr();
>> + ret = dh_imx8_setup_eth1addr(eeprom_buffer);
>> if (ret)
>> printf("%s: Unable to setup eth1addr! ret = %d\n", __func__,
>> ret);
>>
>> return ret;
>> }
>>
>> +void dh_add_item_number_and_serial_to_env(u8 *eeprom_buffer)
>> +{
>> + char *item_number_env;
>> + char item_number[8]; /* String with 7 characters + string
>> termination */
>> + char *serial_env;
>> + char serial[10]; /* String with 9 characters + string
>> termination */
>> + int ret;
>> +
>> + ret = dh_get_value_from_eeprom_buffer(DH_ITEM_NUMBER, item_number,
>> sizeof(item_number),
>> + eeprom_buffer);
>> + if (ret) {
>> + printf("%s: Unable to get item number from EEPROM ID page! ret
>> = %d\n",
>> + __func__, ret);
>
> log_warning or log_error to be consistent with the rest of this patch.
I will use only printf for consistency in V3.
>> + } else {
>> + item_number_env = env_get("vendor_item_number");
>
> Can these variables get prefixes, "dh_...", so they are namespaced and
> it is clear they are not generic U-Boot environment variables ?
Will be renamed to "dh_som_item_number" in V3.
>> + if (!item_number_env)
>> + env_set("vendor_item_number", item_number);
>> + else if (strcmp(item_number_env, item_number) != 0)
>> + log_warning("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_buffer(DH_SERIAL_NUMBER, serial,
>> sizeof(serial),
>> + eeprom_buffer);
>> + if (ret) {
>> + printf("%s: Unable to get serial from EEPROM ID page! ret =
>> %d\n",
>> + __func__, ret);
>
> return ret;
This function hasn't a return value.
>> + } else {
>> + serial_env = env_get("SN");
>> + if (!serial_env)
>> + env_set("SN", serial);
>> + else if (strcmp(serial_env, serial) != 0)
>> + log_warning("Warning: Environment SN differs from
>> EEPROM ID page value (%s != %s)\n",
>> + serial_env, serial);
>> + }
>> +}
>> +
>> int board_init(void)
>> {
>> return 0;
>> @@ -117,7 +183,19 @@ int board_init(void)
>>
>> int board_late_init(void)
>> {
>> - dh_setup_mac_address();
>> + u8 eeprom_buffer[DH_EEPROM_ID_PAGE_MAX_SIZE] = { 0 };
>> + int ret;
>> +
>> + /* The EEPROM ID page is available on SoM rev. 200 and greater. */
>> + if (dh_get_som_rev() > 100) {
>
> If the WLP is not described or disabled in DT (the later is the case for
> rev.100 SoM) (*), dh_read_eeprom_id_page() will bail and so there is not
> need for this custom SoM revision check. DT is, after all, a hardware
> description.
I will remove the function dh_get_som_rev() and use the return value to
detect rev. 100 in V3.
Regards
Christoph
