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

[Marek Vasut]

On 10/22/24 11:31 AM, Christoph Niedermaier wrote:
> From: Marek Vasut <marex at denx.de>
> Sent: Tuesday, October 22, 2024 1:01 AM
>> On 10/21/24 5:38 PM, Christoph Niedermaier wrote:
>>
>> [...]
>>
>>>> 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.
>>
>> The function which interacts with the EEPROM on start up, once, can have
>> this name, sure.
> 
> Sorry, I mean the function dh_get_value_from_eeprom_id_page() here.

The function name doesn't really matter, please pick a fitting one.

>>> 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.
>>
>> It is OK to allocate 32 or 64 bytes on stack, since that chunk of memory
>> is free'd on return from the function. The lifespan of that allocated
>> memory is exactly as long as it should be, and it does not waste U-Boot
>> memory unnecessarily.
>>
>> So far, all known EEPROMs have WLP size of 32 or 64 Byes.
>>
>>> 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.
> 
> Sorry, here I mean also the function dh_get_value_from_eeprom_id_page().
> 
>> This can very well be:
>>
>> dh_add_item_number_and_serial_to_env() {
>>      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
>> }
>>
>> board_init() {
>>     ..
>>     dh_add_item_number_and_serial_to_env();
>>     ...
>> }
>>
>>> 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.
>> This is not good, please do not waste memory in U-Boot if it can be
>> easily avoided by automatically allocating it on stack and freeing it
>> when not needed.
>>
>> [...]
> 
> I don't want to allocate the memory in the function board_init().

You do not, the following automatically reserves space on stack when 
called and frees it up upon function return:

function foo() {
   u8 array[12];
   ...
   stuff(array);
   ...
}

> I want to handle
> size and caching in the function dh_get_value_from_eeprom_id_page(). So I don't
> want to deal with size and structure in the function board_init(). For me, the use
> of a static variable is OK, but you don't like it. Do you know how I can do this
> without assigning a static variable, but not allocate the memory somewhere in a
> caller function?
Even the static variable space is allocated somewhere, either in .bss or 
.data section, except with static variable(s) the memory persists 
throughout the lifetime of U-Boot because their content has to be 
retained even when the function using their content returns.

With variable(s) allocated on stack (which is majority of variable), the 
memory allocation happens on entry to the function and the memory is 
automatically freed on exit from the function.

It is perfectly fine to call some wrapper superfunction from 
board_init() which holds the array on stack and calls all the EEPROM 
parser/handler functions:

eeprom_superwrapper() {
   u8 eeprom[64];
   ...
   eeprom_do_stuff1(eeprom);
   eeprom_do_stuff2(eeprom);
   ...
}

board_init() {
   ...
   eeprom_superwrapper();
   ...
}