On Thu, Sep 30, 2021 at 12:12 AM Matthias Brugger <mbrug...@suse.com> wrote:
> On 29/09/2021 23:05, Ricardo Salveti wrote:
> > On Wed, Sep 29, 2021 at 1:49 PM Matthias Brugger <mbrug...@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" ?
Sure, but that would be a custom logic that needs to be handled by the
user, and this problem can happen with anyone doing a simple saveenv
without knowing that this could be an issue.
Why not simply just trust what the firmware gives instead?
Cheers,
--
Ricardo Salveti
