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: Tuesday, October 22, 2024 1:57 PM
> On 10/22/24 11:31 AM, Christoph Niedermaier wrote:
>> From: Marek Vasut <marex@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.

It's not about the function name. I have referenced the wrong function.

>>>> 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);
>    ...
> }

Sorry, I expressed myself incorrectly here. Of course I meant that
I don't want to reserve memory on the stack for the EEPROM ID page
in the board_init() function.

>> 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();
>    ...
> }

That's not what I'm looking for. Do you have another solution?

Regards
Christoph