[PATCH V3 2/3] arm64: imx8mp: Read values from M24C32-D write-lockable page on DHCOM i.MX8MP

[Marek Vasut]

On 11/21/24 6:21 PM, Christoph Niedermaier wrote:

[...]

> +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 */
> +};
> +
> +#define DH_EEPROM_ID_PAGE_HEADER_LEN	offsetof(struct eeprom_id_page, mac0)

If this is really meant to be header length and it should work reliably, 
then it should not depend on payload layout. You likely need something like:

offsetof(struct eeprom_id_page, header_crc8) + sizeof(u8)

Or better yet, rework the structure this way:

struct eeprom_id_page {
    struct {
	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;
    } header;

    struct {
	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 */
    } payload;
};

... and in the one callsite (*) which does use this macro, do:

struct eeprom_id_page *eip;
...
-crc16_calc = crc16(0xffff, eeprom_buffer + 
DH_EEPROM_ID_PAGE_HEADER_LEN, data_len);
+crc16_calc = crc16(0xffff, eip->payload, sizeof(*eip->payload));

> +#define DH_EEPROM_ID_PAGE_V1_0		0x10
> +#define DH_EEPROM_ID_PAGE_V1_0_DATA_LEN	(sizeof(struct eeprom_id_page) - \
> +					offsetof(struct eeprom_id_page, mac0))

This is not needed either, this would become

sizeof(*eip->payload)

in the only callsite which does use the macro.

>   bool dh_mac_is_in_env(const char *env)
>   {
>   	unsigned char enetaddr[6];
> @@ -30,6 +65,141 @@ int dh_get_mac_is_enabled(const char *alias)
>   	return 0;
>   }
>   
> +int dh_read_eeprom_id_page(u8 *eeprom_buffer, const char *alias)
> +{
> +	struct eeprom_id_page *eip;

You can assign the eip pointer here already:

struct eeprom_id_page *eip = eeprom_buffer;

> +	struct udevice *dev;
> +	size_t data_len;
> +	int eeprom_size;
> +	u16 crc16_calc;
> +	u16 crc16_eip;
> +	u8 crc8_calc;
> +	ofnode node;
> +	int ret;
> +
> +	node = ofnode_path(alias);
> +
> +	ret = uclass_get_device_by_ofnode(UCLASS_I2C_EEPROM, node, &dev);
> +	if (ret)
> +		return ret;
> +
> +	eeprom_size = i2c_eeprom_size(dev);
> +	if (eeprom_size < 0 || eeprom_size > DH_EEPROM_ID_PAGE_MAX_SIZE) {

Wouldn't it be better to exit if eeprom_size < 0 (error) ?

What happens if eeprom_size == 0 ? Maybe that should be considered 
problematic too ?

> +		eeprom_size = DH_EEPROM_ID_PAGE_MAX_SIZE;
> +		printf("Warning: Cannot get valid EEPROM.ID page size! Try to read %d bytes.\n",

s at EEPROM.ID@EEPROM ID@ , replace dot with space .

Also please drop the Warning: prefix , it is unnecessary.

> +		       DH_EEPROM_ID_PAGE_MAX_SIZE);

Print both the original eeprom_size reported by i2c_eeprom_size() and 
DH_EEPROM_ID_PAGE_MAX_SIZE , the former is an important hint for debug 
purposes.

> +	}
> +
> +	ret = i2c_eeprom_read(dev, 0x0, eeprom_buffer, eeprom_size);
> +	if (ret) {
> +		printf("%s: Error reading EEPROM ID page! ret = %d\n", __func__, ret);
> +		return ret;
> +	}
> +
> +	eip = (struct eeprom_id_page *)eeprom_buffer;

See above.

> +	/* Validate header ID */
> +	if (eip->id[0] != 'D' || eip->id[1] != 'H' || eip->id[2] != 'E') {
> +		printf("%s: Error validating header ID! (expected DHE)\n", __func__);

To expedite and ease debugging, it is also important to print what the 
code received , not just what the code expected to receive.

> +		return -EINVAL;
> +	}
> +
> +	/* Validate header checksum */
> +	crc8_calc = crc8(0xff, eeprom_buffer, offsetof(struct eeprom_id_page, header_crc8));
> +	if (eip->header_crc8 != crc8_calc) {
> +		printf("%s: Error validating header checksum! (expected 0x%02X)\n",

Better use %02x , I believe lib/tiny-printf.c cannot handle X modifier , 
please fix globally.

To expedite and ease debugging, it is also important to print what the 
code received , not just what the code expected to receive, please fix 
globally.

> +		       __func__, crc8_calc);
> +		return -EINVAL;
> +	}
> +
> +	/* Validate header version */
> +	if (eip->version == DH_EEPROM_ID_PAGE_V1_0) {

if if (eip->version != DH_EEPROM_ID_PAGE_V1_0) {
   printf(...);
   return -EINVAL;
}

data_len = sizeof(*eip->payload);

> +		data_len = DH_EEPROM_ID_PAGE_V1_0_DATA_LEN;
> +	} else {
> +		printf("%s: Error validating version! (0x%02X is not supported)\n",

The CRC that got calculated is also very interesting, not only the 
expected one.

> +		       __func__, eip->version);
> +		return -EINVAL;
> +	}
> +
> +	/* Validate data checksum */
> +	crc16_eip = (eip->data_crc16[1] << 8) | eip->data_crc16[0];
> +	crc16_calc = crc16(0xffff, eeprom_buffer + DH_EEPROM_ID_PAGE_HEADER_LEN, data_len);

See (*) above

> +	if (crc16_eip != crc16_calc) {
> +		printf("%s: Error validating data checksum! (expected 0x%02X)\n",
> +		       __func__, crc16_calc);

The CRC that got calculated is also very interesting, not only the 
expected one.

> +		return -EINVAL;
> +	}
> +
> +	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 *eip = (struct eeprom_id_page *)eeprom_buffer;

Is the type cast really needed at all here ? (it might be, please check)

> +	char soc_chr;
> +
> +	if (!eeprom_buffer)

Why not pass in "struct eeprom_id_page *eip" directly as function 
parameter of this function ?

This would become if (!eip) return -EINVAL; and the whole type cast 
business above would go away altogether.

> +		return -EINVAL;
> +
> +	/* Copy requested data */
> +	switch (request) {
> +	case DH_MAC0:
> +		if (!is_valid_ethaddr(eip->mac0))
> +			return -EINVAL;
> +
> +		if (data_len >= sizeof(eip->mac0))
> +			memcpy(data, eip->mac0, sizeof(eip->mac0));
> +		else
> +			return -EINVAL;
> +		break;
> +	case DH_MAC1:
> +		if (!is_valid_ethaddr(eip->mac1))
> +			return -EINVAL;
> +
> +		if (data_len >= sizeof(eip->mac1))
> +			memcpy(data, eip->mac1, sizeof(eip->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 soc_coded = eip->item_prefix & 0xf;

Doesn't the compiler warn about mixing code and variable declarations ?
If yes, you can easily move this line to the very beginning of this 
function, which is probably just fine to do.

[...]

> @@ -28,9 +37,40 @@ int dh_get_mac_is_enabled(const char *alias);
>    */
>   int dh_get_mac_from_eeprom(unsigned char *enetaddr, const char *alias);
>   
> +/*
> + * dh_read_eeprom_id_page() - Read EEPROM ID page content into given buffer
> + * @eeprom_buffer:	Buffer for EEPROM ID page content
> + * @alias:		Alias for EEPROM device tree node

Is this really alias to the EEPROM node or to the EEPROM WLP node ?

> + * Read the content of the EEPROM ID page into the given buffer (parameter
> + * eeprom_buffer). The EEPROM device is selected via alias device tree name
> + * (parameter alias). The data of the EEPROM ID page is verified. An error
> + * is returned for reading failures and invalid data.
> + *
> + * Return: 0 if OK, other value on error
> + */
> +int dh_read_eeprom_id_page(u8 *eeprom_buffer, const char *alias);
> +
> +/*
> + * dh_get_value_from_eeprom_buffer() - Get value from EEPROM buffer
> + * @eip_request_values:	Requested value as enum
> + * @data:		Buffer where value is to be stored
> + * @data_len:		Length of the value buffer
> + * @eeprom_buffer:	EEPROM buffer from which the data is parsed
> + *
> + * Gets the value specified by the parameter eip_request_values from the EEPROM
> + * buffer (parameter eeprom_buffer). 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_buffer(enum eip_request_values request, u8 *data, int data_len,
> +				    u8 *eeprom_buffer);

[...]

> +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 DHSOM item number from EEPROM ID page! ret = %d\n",
> +		       __func__, ret);
> +	} else {
> +		item_number_env = env_get("dh_som_item_number");
> +		if (!item_number_env)
> +			env_set("dh_som_item_number", item_number);
> +		else if (strcmp(item_number_env, item_number) != 0)

The != 0 is not necessary, please drop it.

> +			printf("Warning: Environment dh_som_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 DHSOM serial number from EEPROM ID page! ret = %d\n",
> +		       __func__, ret);
> +	} else {
> +		serial_env = env_get("dh_som_serial_number");
> +		if (!serial_env)
> +			env_set("dh_som_serial_number", serial);
> +		else if (strcmp(serial_env, serial) != 0)

The != 0 is not necessary, please drop it.

> +			printf("Warning: Environment dh_som_serial_number differs from EEPROM ID page value (%s != %s)\n",
> +			       serial_env, serial);
> +	}
> +}
> +
>   int board_init(void)
>   {
>   	return 0;
> @@ -117,7 +160,26 @@ int board_init(void)
>   
>   int board_late_init(void)
>   {
> -	dh_setup_mac_address();
> +	u8 eeprom_buffer[DH_EEPROM_ID_PAGE_MAX_SIZE] = { 0 };

Do the following cast here:

struct eeprom_id_page *eip = eeprom_buffer;

and then you can pass *eip to dh_setup_mac_address() and 
dh_add_item_number_and_serial_to_env() and save yourself a lot of 
typecasts all around.

> +	int ret;
> +
> +	ret = dh_read_eeprom_id_page(eeprom_buffer, "eeprom0wl");
> +	if (ret) {
> +		/*
> +		 * The EEPROM ID page is available on SoM rev. 200 and greater.
> +		 * For SoM rev. 100 the return value will be -ENODEV. Suppress
> +		 * the error message for that, because the absence cannot be
> +		 * treated as an error.
> +		 */
> +		if (ret != -ENODEV)
> +			printf("%s: Cannot read valid data from EEPROM ID page! ret = %d\n",
> +			       __func__, ret);
> +		dh_setup_mac_address(NULL);
> +	} else {
> +		dh_setup_mac_address(eeprom_buffer);
> +		dh_add_item_number_and_serial_to_env(eeprom_buffer);
[...]