* [PATCH] rpi: always set fdt_addr with firmware-provided FDT address
@ 2021-05-12 12:39 Mauro Salvini
2021-06-07 9:27 ` Mauro Salvini
0 siblings, 1 reply; 10+ messages in thread
From: Mauro Salvini @ 2021-05-12 12:39 UTC (permalink / raw)
To: u-boot
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
Signed-off-by: Mauro Salvini <m.salvini@koansoftware.com>
Cc: C?dric Schieli <cschieli@gmail.com>
Cc: Matthias Brugger <mbrugger@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;
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] rpi: always set fdt_addr with firmware-provided FDT address
2021-05-12 12:39 [PATCH] rpi: always set fdt_addr with firmware-provided FDT address Mauro Salvini
@ 2021-06-07 9:27 ` Mauro Salvini
2021-09-15 11:16 ` Matthias Brugger
0 siblings, 1 reply; 10+ messages in thread
From: Mauro Salvini @ 2021-06-07 9:27 UTC (permalink / raw)
To: u-boot; +Cc: Cédric Schieli, Matthias Brugger
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
>
> Signed-off-by: Mauro Salvini <m.salvini@koansoftware.com>
> Cc: Cédric Schieli <cschieli@gmail.com>
> Cc: Matthias Brugger <mbrugger@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
--
Mauro Salvini | KOAN sas | Bergamo - Italia
embedded software engineering
http://KoanSoftware.com
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] rpi: always set fdt_addr with firmware-provided FDT address
2021-06-07 9:27 ` Mauro Salvini
@ 2021-09-15 11:16 ` Matthias Brugger
2021-09-29 10:14 ` Mauro Salvini
0 siblings, 1 reply; 10+ messages in thread
From: Matthias Brugger @ 2021-09-15 11:16 UTC (permalink / raw)
To: Mauro Salvini, u-boot; +Cc: Cédric Schieli
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.
Regards,
Matthias
>> Signed-off-by: Mauro Salvini <m.salvini@koansoftware.com>
>> Cc: Cédric Schieli <cschieli@gmail.com>
>> Cc: Matthias Brugger <mbrugger@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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] rpi: always set fdt_addr with firmware-provided FDT address
2021-09-15 11:16 ` Matthias Brugger
@ 2021-09-29 10:14 ` Mauro Salvini
2021-09-29 11:41 ` Matthias Brugger
0 siblings, 1 reply; 10+ messages in thread
From: Mauro Salvini @ 2021-09-29 10:14 UTC (permalink / raw)
To: mbrugger; +Cc: cschieli, u-boot
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.
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
>>
--
Mauro Salvini | KOAN sas | Bergamo - Italia
embedded software engineering
Phone: +39 035-255235
http://KoanSoftware.com
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] rpi: always set fdt_addr with firmware-provided FDT address
2021-09-29 10:14 ` Mauro Salvini
@ 2021-09-29 11:41 ` Matthias Brugger
2021-09-29 12:19 ` Mauro Salvini
0 siblings, 1 reply; 10+ messages in thread
From: Matthias Brugger @ 2021-09-29 11:41 UTC (permalink / raw)
To: Mauro Salvini; +Cc: cschieli, u-boot
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?
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
>>>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] rpi: always set fdt_addr with firmware-provided FDT address
2021-09-29 11:41 ` Matthias Brugger
@ 2021-09-29 12:19 ` Mauro Salvini
2021-09-29 16:49 ` Matthias Brugger
0 siblings, 1 reply; 10+ messages in thread
From: Mauro Salvini @ 2021-09-29 12:19 UTC (permalink / raw)
To: Matthias Brugger; +Cc: cschieli, u-boot
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 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
>>>>
>>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] rpi: always set fdt_addr with firmware-provided FDT address
2021-09-29 12:19 ` Mauro Salvini
@ 2021-09-29 16:49 ` Matthias Brugger
2021-09-29 21:05 ` Ricardo Salveti
0 siblings, 1 reply; 10+ messages in thread
From: Matthias Brugger @ 2021-09-29 16:49 UTC (permalink / raw)
To: Mauro Salvini; +Cc: cschieli, u-boot
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
>>>>>
>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] rpi: always set fdt_addr with firmware-provided FDT address
2021-09-29 16:49 ` Matthias Brugger
@ 2021-09-29 21:05 ` Ricardo Salveti
2021-09-30 3:11 ` Matthias Brugger
0 siblings, 1 reply; 10+ messages in thread
From: Ricardo Salveti @ 2021-09-29 21:05 UTC (permalink / raw)
To: Matthias Brugger; +Cc: Mauro Salvini, cschieli, u-boot
On Wed, Sep 29, 2021 at 1:49 PM Matthias Brugger <mbrugger@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.
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,
--
Ricardo Salveti
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] rpi: always set fdt_addr with firmware-provided FDT address
2021-09-29 21:05 ` Ricardo Salveti
@ 2021-09-30 3:11 ` Matthias Brugger
2021-09-30 13:29 ` Ricardo Salveti
0 siblings, 1 reply; 10+ messages in thread
From: Matthias Brugger @ 2021-09-30 3:11 UTC (permalink / raw)
To: Ricardo Salveti; +Cc: Mauro Salvini, cschieli, u-boot
On 29/09/2021 23:05, Ricardo Salveti wrote:
> On Wed, Sep 29, 2021 at 1:49 PM Matthias Brugger <mbrugger@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,
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] rpi: always set fdt_addr with firmware-provided FDT address
2021-09-30 3:11 ` Matthias Brugger
@ 2021-09-30 13:29 ` Ricardo Salveti
0 siblings, 0 replies; 10+ messages in thread
From: Ricardo Salveti @ 2021-09-30 13:29 UTC (permalink / raw)
To: Matthias Brugger; +Cc: Mauro Salvini, cschieli, u-boot
On Thu, Sep 30, 2021 at 12:12 AM Matthias Brugger <mbrugger@suse.com> wrote:
> On 29/09/2021 23:05, Ricardo Salveti wrote:
> > On Wed, Sep 29, 2021 at 1:49 PM Matthias Brugger <mbrugger@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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-09-30 13:30 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-12 12:39 [PATCH] rpi: always set fdt_addr with firmware-provided FDT address Mauro Salvini
2021-06-07 9:27 ` Mauro Salvini
2021-09-15 11:16 ` Matthias Brugger
2021-09-29 10:14 ` Mauro Salvini
2021-09-29 11:41 ` Matthias Brugger
2021-09-29 12:19 ` Mauro Salvini
2021-09-29 16:49 ` Matthias Brugger
2021-09-29 21:05 ` Ricardo Salveti
2021-09-30 3:11 ` Matthias Brugger
2021-09-30 13:29 ` Ricardo Salveti
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.