[PATCH] rpi: always set fdt_addr to the correct value

[PATCH] rpi: always set fdt_addr to the correct value

[Marek Szyprowski]

The fdt_addr env have meaning only for the current runtime and it depends
on the dtb size or firmware version. If one save the environment to disk
and the loads it on the latter boot, the fdt_addr might change, what
result in passing incorrect dtb address to the kernel. Fix this by always
setting the fdt_addr env. This fixes system operation after saving the
env to disk and updating i.e. dtb files or firmware.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.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 bc3cc597adb..6d6d2e69234 100644
--- a/board/raspberrypi/rpi/rpi.c
+++ b/board/raspberrypi/rpi/rpi.c
@@ -347,9 +347,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

Re: [PATCH] rpi: always set fdt_addr to the correct value

[Matthias Brugger]

On 18/02/2022 03:44, Jaehoon Chung wrote:
> On 22. 2. 14. 20:25, Marek Szyprowski wrote:
>> The fdt_addr env have meaning only for the current runtime and it depends
>> on the dtb size or firmware version. If one save the environment to disk
>> and the loads it on the latter boot, the fdt_addr might change, what
>> result in passing incorrect dtb address to the kernel. Fix this by always
>> setting the fdt_addr env. This fixes system operation after saving the
>> env to disk and updating i.e. dtb files or firmware.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> 

Could we keep the discussion where we left it the last time you submitted the patch?

Thanks! :)

Regards,
Matthias

> Best Regards,
> Jaehoon Chung
> 
>> ---
>>   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 bc3cc597adb..6d6d2e69234 100644
>> --- a/board/raspberrypi/rpi/rpi.c
>> +++ b/board/raspberrypi/rpi/rpi.c
>> @@ -347,9 +347,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;
>>   
> 

Re: [PATCH] rpi: always set fdt_addr to the correct value

[Matthias Brugger]

On 15/02/2022 15:55, Matthias Brugger wrote:
> 
> 
> On 18/02/2022 03:44, Jaehoon Chung wrote:
>> On 22. 2. 14. 20:25, Marek Szyprowski wrote:
>>> The fdt_addr env have meaning only for the current runtime and it depends
>>> on the dtb size or firmware version. If one save the environment to disk
>>> and the loads it on the latter boot, the fdt_addr might change, what
>>> result in passing incorrect dtb address to the kernel. Fix this by always
>>> setting the fdt_addr env. This fixes system operation after saving the
>>> env to disk and updating i.e. dtb files or firmware.
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>
>> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
>>
> 
> Could we keep the discussion where we left it the last time you submitted the 
> patch?
> 

I forgot to add the link to the old discussion:
https://patchwork.ozlabs.org/project/uboot/patch/20210512123945.25649-1-m.salvini@koansoftware.com/

Regards,
Matthias

> Thanks! :)
> 
> Regards,
> Matthias
> 
>> Best Regards,
>> Jaehoon Chung
>>
>>> ---
>>>   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 bc3cc597adb..6d6d2e69234 100644
>>> --- a/board/raspberrypi/rpi/rpi.c
>>> +++ b/board/raspberrypi/rpi/rpi.c
>>> @@ -347,9 +347,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;
>>

Re: [PATCH] rpi: always set fdt_addr to the correct value

[Marek Szyprowski]

Hi Matthias,

On 15.02.2022 19:19, Matthias Brugger wrote:
>
> On 15/02/2022 15:55, Matthias Brugger wrote:
>>
>> On 18/02/2022 03:44, Jaehoon Chung wrote:
>>> On 22. 2. 14. 20:25, Marek Szyprowski wrote:
>>>> The fdt_addr env have meaning only for the current runtime and it 
>>>> depends
>>>> on the dtb size or firmware version. If one save the environment to 
>>>> disk
>>>> and the loads it on the latter boot, the fdt_addr might change, what
>>>> result in passing incorrect dtb address to the kernel. Fix this by 
>>>> always
>>>> setting the fdt_addr env. This fixes system operation after saving the
>>>> env to disk and updating i.e. dtb files or firmware.
>>>>
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>
>>> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
>>>
>>
>> Could we keep the discussion where we left it the last time you 
>> submitted the patch?
>>
>
> I forgot to add the link to the old discussion:
> https://patchwork.ozlabs.org/project/uboot/patch/20210512123945.25649-1-m.salvini@koansoftware.com/

Well, I'm still not convinced that this is a good idea.

I found this issue while debugging something else and I must admit that 
the current behavior is really counterintuitive. I was surprised that 
after setting some really unrelated things in u-boot's envs (like 
additional kernel arguments to increase debug level) and saving such 
config, I got completely broken system. Right, I've also updated DTB in 
meantime because I was bisecting some kernel related issue, but still 
this is something that a typical user might face during system update.

If we want to keep current behavior, the 'saveenv' command should print 
a large banner that one has to first delete the 'fdt_addr' env if he 
wants to have a working system.

I will check if it would be possible to use some flags for the 
'fdt_addr' env to mark it as 'do-not-save-unless-changed-manually'.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] rpi: always set fdt_addr to the correct value

[Jaehoon Chung]

On 22. 2. 14. 20:25, Marek Szyprowski wrote:
> The fdt_addr env have meaning only for the current runtime and it depends
> on the dtb size or firmware version. If one save the environment to disk
> and the loads it on the latter boot, the fdt_addr might change, what
> result in passing incorrect dtb address to the kernel. Fix this by always
> setting the fdt_addr env. This fixes system operation after saving the
> env to disk and updating i.e. dtb files or firmware.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> ---
>  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 bc3cc597adb..6d6d2e69234 100644
> --- a/board/raspberrypi/rpi/rpi.c
> +++ b/board/raspberrypi/rpi/rpi.c
> @@ -347,9 +347,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;
>  

Re: [PATCH] rpi: always set fdt_addr to the correct value

[Jim Posen]

Another related issue that I was confused by is that I expected the FDT address
to be in the variable fdt_addr_r but it is in fdt_addr. In the U-Boot
documentation here
https://u-boot.readthedocs.io/en/latest/usage/environment.html#image-locations,
*_r variables indicate addresses in RAM whereas fdt_addr refers to a flash
location. This may provide a solution to the backwards compatibility problem
with this patch. I propose setting both fdt_addr and fdt_addr_r to the
firmware-provided FDT in fw_dtb_pointer, and you can make it so that fdt_addr_r
will be overwritten even if there is a pre-existing value for it in the
environment. Thoughts?


On Mon, Feb 14, 2022 at 6:25 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> The fdt_addr env have meaning only for the current runtime and it depends
> on the dtb size or firmware version. If one save the environment to disk
> and the loads it on the latter boot, the fdt_addr might change, what
> result in passing incorrect dtb address to the kernel. Fix this by always
> setting the fdt_addr env. This fixes system operation after saving the
> env to disk and updating i.e. dtb files or firmware.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.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 bc3cc597adb..6d6d2e69234 100644
> --- a/board/raspberrypi/rpi/rpi.c
> +++ b/board/raspberrypi/rpi/rpi.c
> @@ -347,9 +347,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;
>