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

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

From: Marek Vasut <marex@denx.de>
Sent: Thursday, October 17, 2024 8:35 PM
> 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 ?

That’s true.

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

The idea is that function dh_add_item_number_and_serial_to_env() encapsulates
the reading. I don't want to have to worry about the structure and number of
bytes of the EEPROM ID page and want to be independent of it. It is planned to
extend the structure and increase the number of bytes for the STM32MP2. The
changes to the size will then depend on the version of the data in the header
within the structure. However, these changes should then only have to be made
within the function dh_add_item_number_and_serial_to_env() for reading the
EEPROM ID page. I accept the static variable in order to better isolate the
function. Since the memory is freed up again when the operating system boots,
this consumption of 32 bytes in U-Boot is not a problem, because it is only
temporary.


Regards
Christoph