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

[Matthias Brugger]

On 29/09/2021 23:05, Ricardo Salveti wrote:
> On Wed, Sep 29, 2021 at 1:49 PM Matthias Brugger <mbrugger at suse.com> wrote:
>> 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?
> 
> Yes, this issue only happens if you save the environment at least
> once, as then on following boots it won't set the address based on
> what the firmware uses, but instead restore the value from the saved
> environment, which can be wrong.
> 

So can't we just delete the environment variable before saving the environment?
Something like "env delete fdt_addr; saveenv" ?

Regards,
Matthias

> I noticed the same when moving one sdcard between boards, and I was
> only able to get it to boot correctly after removing the saved
> environment.
> 
> Cheers,
>