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

[Christoph Niedermaier <cniedermaier@dh-electronics.com>]

From: Marek Vasut <marex@denx.de>
Sent: Sunday, November 24, 2024 10:02 PM
> 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));
>

I will change it in V4.

>> +#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.
>

I will change it in V4.

>>   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;
>

I will change the function parameter to the struct eeprom_id_page in V4.

>> +	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) ?

I will add it in V4.

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

I will add it also in V4.

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

I will fix it in V4.

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

I will drop it in V4.

>> +		       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.

I will add it in V4.

>> +	}
>> +
>> +	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.
> 

I will remove this line in V4.

>> +	/* 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.
>

I will add it in V4.

>> +		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.

I will fix it in V4.

> 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.
>

I will add it in V4.

>> +		       __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);

I will rework it in V4.

>> +		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.

I will add it in V4.

>> +		       __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
>

I will change it in V4.

>> +	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.
>

I will add it in V4.

>> +		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)

Otherwise I get the following warning from the compiler:
warning: initialization of ‘struct eeprom_id_page *’ from incompatible pointer type ‘u8 *’ {aka ‘unsigned char *’} [-Wincompatible-pointer-types]
struct eeprom_id_page *eip = eeprom_buffer;
                             ^~~~~~~~~~~~~
>> +	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.
> 

I will change the function parameter to the struct eeprom_id_page in V4.

>> +		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.

The compiler doesn't warn about that, but I will move it to the beginning in V4.

> [...]
> 
>> @@ -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 ?
>

I will fix it in V4.

>> + * 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.

I will drop it in V4.

>> +			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.

I will drop it in V4.

>> +			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.

I will change it in V4.

>> +	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);
> [...]

Thanks and regards
Christoph