| Message ID | 20220214112505.26814-1-m.szyprowski@samsung.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | rpi: always set fdt_addr to the correct value | expand |
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; >> >
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; >>
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
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; >
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; >
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;
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(-)