[PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM

[Marek Vasut]

On 10/17/24 1:55 PM, Christoph Niedermaier wrote:

[...]

>>>>> +                            __func__, ret);
>>>>
>>>> This will be printed on every device, even the ones without ID EEPROM,
>>>> correct ? This should not be printed on devices without ID EEPROM. Also,
>>>
>>> This is suppressed by the -ENOENT check.
>>
>> Does i2c_eeprom_read() in dh_get_value_from_eeprom_id_page() return
>> -ENOENT in case the EEPROM is described in DT, but not populated on the
>> board ? I suspect it returns some other error code, -ETIMEDOUT or
>> -EINVAL maybe ?
> 
> It could only be possible if the DTO for hardware rev. 100 (which has no
> EEPROM ID page) is not loaded. The return value then is -ENODEV.
> I will included this in v2.

OK

>>>> Also, this shouldn't be repeatedly reading the EEPROM, the EEPROM read
>>>> operation is the slow part, the EEPROM is 32 bytes, so the EEPROM should
>>>> be read once and cached, and once the cache is populated all read
>>>> accesses to the EEPROM should use the cache.
>>>
>>> This is already covered in function dh_get_value_from_eeprom_id_page().
>> It seems that function always calls
>> ret = i2c_eeprom_read(dev, 0x0, eipa, sizeof(eipa));
>> ?
> 
> The 32 bytes are read into a static variable. If the ID (DHE) already exists
> in it, the i2c_eeprom_read() function is no longer called.
Hmmm, but it seems all the functions which access this EEPROM WLP are 
called from board_init(), are they not ?

If yes, then there is no need for any static variable:

board_init() {
  u8 eeprom[32];
  dh_read_eeprom_wlp(eeprom); // read the eeprom once
  dh_setup_mac_address(eeprom); // extract MAC from EEPROM
  dh_add_item_number_and_serial_to_env(eeprom); // extract SN from EEPROM
  // once this function exits, the eeprom variable on stack is discarded
  // which is OK, since it won't be used anymore anyway
}

[...]