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