[PATCH] rpi: always set fdt_addr with firmware-provided FDT address

[Matthias Brugger]

On 29/09/2021 14:19, Mauro Salvini wrote:
> Hi Matthias,
> 
> On 29/09/21 13:41, Matthias Brugger wrote:
>> Hi Mauro,
>>
>> On 29/09/2021 12:14, Mauro Salvini wrote:
>>> Hi Matthias
>>>
>>> On 15/09/21 13:16, mbrugger at suse.com (Matthias Brugger) wrote:
>>>> Hi Mauro,
>>>>
>>>> On 07/06/2021 11:27, Mauro Salvini wrote:
>>>>> On 12/05/21 14:39, Mauro Salvini wrote:
>>>>>> Raspberry firmware prepares the FDT blob in memory at an address
>>>>>> that depends on both the memory size and the blob size [1].
>>>>>> After commit ade243a211d6 ("rpi: passthrough of the firmware provided FDT
>>>>>> blob") this FDT is passed to kernel through fdt_addr environment variable,
>>>>>> handled in set_fdt_addr() function in board file.
>>>>>>
>>>>>> When u-boot environment is persistently saved, if a change happens
>>>>>> in loaded FDT (e.g. for a new overlay applied), firmware produces a FDT
>>>>>> address different from the saved one, but u-boot still use the saved
>>>>>> one because set_fdt_addr() function does not overwrite the fdt_addr
>>>>>> variable. So, for example, if there is a script that uses fdt commands for
>>>>>> e.g. manipulate the bootargs, boot hangs with error
>>>>>>
>>>>>> libfdt fdt_check_header(): FDT_ERR_BADMAGIC
>>>>>>
>>>>>> Removing the fdt_addr variable in saved environment allows to boot.
>>>>>>
>>>>>> With this patch set_fdt_addr() function always overwrite fdt_addr value.
>>>>>>
>>>>>> [1] https://www.raspberrypi.org/forums//viewtopic.php?f=107&t=134018
>>>>>>
>>>>
>>>> First of all sorry for the very late reply.
>>>> I'm hesitant to apply this patch, basically because it can break other setups
>>>> where people load a custom DTB to fdt_addr.
>>>>
>>>> I wonder why you can't erase fdt_addr from your persistent storage. There is a
>>>> command called eraseenv that should to the job.
>>>
>>> Sorry me too for the late reply.
>>>
>>> So your suggestion is to erase the fdt_addr variable from the environment 
>>> each time one needs to "refresh" it (one example could be the situation that 
>>> I ponted out).
>>>
>>> Yes, this could be the solution, but the need to delete the fdt_addr variable 
>>> when e.g. one changes the dtb loaded by rpi firmware should be documented 
>>> somewhere.
>>>
>>
>> Hm, maybe I didn't understand the problem. My understanding is, that when you 
>> save the environment with saveenv, you also save the fdt_addr. And that's a 
>> problem because if you add a overlay later, the fdt_addr changed, which will 
>> not be reflected. So my question was if you couldn't just delete the fdt_addr 
>> variable from you saved environment so that when lateron you load overlays, 
>> you won't hit the problem. >
>> My understanding was that you are setting a custom environment for your 
>> boards, but later your customers might add a overlay via e.g. config.txt and 
>> that will break booting the system.
>>
>> But from your response it seems thats not what you are experiencing. Or do you 
>> change the DTB loaded from FW in the U-Boot shell?
> 
> 
> Maybe I wasn't too clear in my explanation ;-)
> 
> At every boot, u-boot executes set_fdt_addr() function. At the very first boot, 
> if nobody has changed the default u-boot environment built in u-boot, adding a 
> custom fdt_addr variable, this function saves the fdt_addr variable in 

Which function are we talking about? Adding a custom fdt_addr can be done via 
the CLI or by adding that to the U-Boot sources (include/configs/rpi.h) but not 
through a 'function'.

Do you mean that in some script you made saveenv is called? I used overlays in 
RPi and never had that problem.

Actually I'm a bit puzzled about the problem. Can you give me a step by step 
reproducer, starting with which config I should use to compile U-Boot?

Regards,
Matthias

> persistent u-boot env. Variable contains the address where RPI firmware has 
> loaded the dtb, passed to u-boot by RPI firmware.
> This variable is not changed anymore by u-boot itself (to change it, you have to 
> use u-boot cli). If you still use the same dtb, loaded by the RPI firmware, it's 
> not a problem, because the fdt address computed by RPI firmware is always the 
> same at every boot, and equal to the one saved in persistent env. If you delete 
> the variable from saved env, at next boot it will be re-created in persistent 
> env, with the value from RPI firmware.
> 
> Suppose that after some time one changes the config.txt and adds an overlay.
> 
> On the next boot, RPI firmware applies the overlay, and the new fdt address 
> could be different from the one computed until last boot (why it changes, I 
> don't know).
> But set_fdt_addr() function does not update the value in env, because fdt_addr 
> is set yet.
> Then, u-boot uses the fdt from a wrong address, and, if it executes a script to 
> manupulate e.g. bootargs, you got the error.
> 
> Applying the patch, the variable will be saved at every boot, the the value 
> saved is always the right one. Conversely, as you pointed out, a fdt_addr 
> variable set in the persistent environment by the user will be overwritten.
> 
> Then, the best option should be to write somewhere in the documentation that, if 
> one loads the dtb using RPI firmware instead of u-boot, after changing 
> config.txt or the dtb itself, the fdt_addr variable in u-boot environment must 
> be deleted.
> 
> Feel free to require more clarifications if needed.
> 
> Thank you
> 
> Mauro
> 
>>
>> Regards,
>> Matthias
>>
>>> Thanks, regards
>>> Mauro
>>>
>>>>
>>>> Regards,
>>>> Matthias
>>>>
>>>>>> Signed-off-by: Mauro Salvini <m.salvini at koansoftware.com>
>>>>>> Cc: C?dric Schieli <cschieli at gmail.com>
>>>>>> Cc: Matthias Brugger <mbrugger at suse.com>
>>>>>> ---
>>>>>> ? board/raspberrypi/rpi/rpi.c | 3 ---
>>>>>> ? 1 file changed, 3 deletions(-)
>>>>>>
>>>>>> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c
>>>>>> index df52a4689f..611013471e 100644
>>>>>> --- a/board/raspberrypi/rpi/rpi.c
>>>>>> +++ b/board/raspberrypi/rpi/rpi.c
>>>>>> @@ -318,9 +318,6 @@ static void set_fdtfile(void)
>>>>>> ?? */
>>>>>> ? static void set_fdt_addr(void)
>>>>>> ? {
>>>>>> -??? if (env_get("fdt_addr"))
>>>>>> -??????? return;
>>>>>> -
>>>>>> ????? if (fdt_magic(fw_dtb_pointer) != FDT_MAGIC)
>>>>>> ????????? return;
>>>>>>
>>>>>
>>>>>
>>>>> Hi all,
>>>>>
>>>>> kind ping.
>>>>>
>>>>> Regards
>>>>>
>>>
>>>
>>
>