From: Marek Vasut <ma...@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
