| Message ID | 20210812233938.11633-3-luka.kovacic@sartura.hr |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | Add support for the GST ESPRESSOBin-Ultra board | expand |
On Friday 13 August 2021 01:39:37 Luka Kovacic wrote: > Add the loadaddr U-Boot environment variable, as this is available in > the stock Marvell U-Boot by default on Marvell Armada A37XX platforms. Hello Luka! Why is this change needed? Reason that it is in historic vendor U-Boot does not mean that it has to be also in new mainline version. I have already wrote some reasons in previous review thread: https://lore.kernel.org/u-boot/20210301150505.3iajeeufi7ahsnk3@pali/ I think this change was needed when CONFIG_SYS_LOAD_ADDR was set to incorrect value, which is already fixed in mainline U-Boot. > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> > Cc: Luka Perkov <luka.perkov@sartura.hr> > Cc: Robert Marko <robert.marko@sartura.hr> > --- > include/configs/mvebu_armada-37xx.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h > index 8e8bcfa018..6901680e32 100644 > --- a/include/configs/mvebu_armada-37xx.h > +++ b/include/configs/mvebu_armada-37xx.h > @@ -110,6 +110,7 @@ > > /* fdt_addr and kernel_addr are needed for existing distribution boot scripts */ > #define CONFIG_EXTRA_ENV_SETTINGS \ > + "loadaddr=0x6000000\0" \ > "scriptaddr=0x6d00000\0" \ > "pxefile_addr_r=0x6e00000\0" \ > "fdt_addr=0x6f00000\0" \ > -- > 2.31.1 >
Hello Pali, On Fri, Aug 13, 2021 at 10:08 AM Pali Rohár <pali@kernel.org> wrote: > > On Friday 13 August 2021 01:39:37 Luka Kovacic wrote: > > Add the loadaddr U-Boot environment variable, as this is available in > > the stock Marvell U-Boot by default on Marvell Armada A37XX platforms. > > Hello Luka! Why is this change needed? Reason that it is in historic > vendor U-Boot does not mean that it has to be also in new mainline > version. > > I have already wrote some reasons in previous review thread: > https://lore.kernel.org/u-boot/20210301150505.3iajeeufi7ahsnk3@pali/ > > I think this change was needed when CONFIG_SYS_LOAD_ADDR was set to > incorrect value, which is already fixed in mainline U-Boot. This value is very useful when building custom Linux boot scripts. Yesterday, I booted the board without this patch and there was no loadaddr variable. Do I understand this correctly? Are you saying that the value in the loadaddr variable should be automatically inherited from CONFIG_SYS_LOAD_ADDR even without this patch? > > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> > > Cc: Luka Perkov <luka.perkov@sartura.hr> > > Cc: Robert Marko <robert.marko@sartura.hr> > > --- > > include/configs/mvebu_armada-37xx.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h > > index 8e8bcfa018..6901680e32 100644 > > --- a/include/configs/mvebu_armada-37xx.h > > +++ b/include/configs/mvebu_armada-37xx.h > > @@ -110,6 +110,7 @@ > > > > /* fdt_addr and kernel_addr are needed for existing distribution boot scripts */ > > #define CONFIG_EXTRA_ENV_SETTINGS \ > > + "loadaddr=0x6000000\0" \ > > "scriptaddr=0x6d00000\0" \ > > "pxefile_addr_r=0x6e00000\0" \ > > "fdt_addr=0x6f00000\0" \ > > -- > > 2.31.1 > > Kind regards, Luka
On Friday 13 August 2021 10:59:33 Luka Kovacic wrote: > Hello Pali, > > On Fri, Aug 13, 2021 at 10:08 AM Pali Rohár <pali@kernel.org> wrote: > > > > On Friday 13 August 2021 01:39:37 Luka Kovacic wrote: > > > Add the loadaddr U-Boot environment variable, as this is available in > > > the stock Marvell U-Boot by default on Marvell Armada A37XX platforms. > > > > Hello Luka! Why is this change needed? Reason that it is in historic > > vendor U-Boot does not mean that it has to be also in new mainline > > version. > > > > I have already wrote some reasons in previous review thread: > > https://lore.kernel.org/u-boot/20210301150505.3iajeeufi7ahsnk3@pali/ > > > > I think this change was needed when CONFIG_SYS_LOAD_ADDR was set to > > incorrect value, which is already fixed in mainline U-Boot. > > This value is very useful when building custom Linux boot scripts. > Yesterday, I booted the board without this patch and there was no loadaddr > variable. > > Do I understand this correctly? Are you saying that the value in the loadaddr > variable should be automatically inherited from CONFIG_SYS_LOAD_ADDR > even without this patch? If you do not specify load address then address from CONFIG_SYS_LOAD_ADDR is used. But as I mentioned in above email, CONFIG_SYS_LOAD_ADDR was defined incorrectly and which cause that it did not worked and caused crashes... > > > > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> > > > Cc: Luka Perkov <luka.perkov@sartura.hr> > > > Cc: Robert Marko <robert.marko@sartura.hr> > > > --- > > > include/configs/mvebu_armada-37xx.h | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h > > > index 8e8bcfa018..6901680e32 100644 > > > --- a/include/configs/mvebu_armada-37xx.h > > > +++ b/include/configs/mvebu_armada-37xx.h > > > @@ -110,6 +110,7 @@ > > > > > > /* fdt_addr and kernel_addr are needed for existing distribution boot scripts */ > > > #define CONFIG_EXTRA_ENV_SETTINGS \ > > > + "loadaddr=0x6000000\0" \ > > > "scriptaddr=0x6d00000\0" \ > > > "pxefile_addr_r=0x6e00000\0" \ > > > "fdt_addr=0x6f00000\0" \ > > > -- > > > 2.31.1 > > > > > Kind regards, > Luka
Hello Pali, On Fri, Aug 13, 2021 at 11:31 AM Pali Rohár <pali@kernel.org> wrote: > > On Friday 13 August 2021 10:59:33 Luka Kovacic wrote: > > Hello Pali, > > > > On Fri, Aug 13, 2021 at 10:08 AM Pali Rohár <pali@kernel.org> wrote: > > > > > > On Friday 13 August 2021 01:39:37 Luka Kovacic wrote: > > > > Add the loadaddr U-Boot environment variable, as this is available in > > > > the stock Marvell U-Boot by default on Marvell Armada A37XX platforms. > > > > > > Hello Luka! Why is this change needed? Reason that it is in historic > > > vendor U-Boot does not mean that it has to be also in new mainline > > > version. > > > > > > I have already wrote some reasons in previous review thread: > > > https://lore.kernel.org/u-boot/20210301150505.3iajeeufi7ahsnk3@pali/ > > > > > > I think this change was needed when CONFIG_SYS_LOAD_ADDR was set to > > > incorrect value, which is already fixed in mainline U-Boot. > > > > This value is very useful when building custom Linux boot scripts. > > Yesterday, I booted the board without this patch and there was no loadaddr > > variable. > > > > Do I understand this correctly? Are you saying that the value in the loadaddr > > variable should be automatically inherited from CONFIG_SYS_LOAD_ADDR > > even without this patch? > > If you do not specify load address then address from > CONFIG_SYS_LOAD_ADDR is used. But as I mentioned in above email, > CONFIG_SYS_LOAD_ADDR was defined incorrectly and which cause that it did > not worked and caused crashes... I tried not specifying it and then the loadaddr variable doesn't even exist. > > > > > > > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> > > > > Cc: Luka Perkov <luka.perkov@sartura.hr> > > > > Cc: Robert Marko <robert.marko@sartura.hr> > > > > --- > > > > include/configs/mvebu_armada-37xx.h | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h > > > > index 8e8bcfa018..6901680e32 100644 > > > > --- a/include/configs/mvebu_armada-37xx.h > > > > +++ b/include/configs/mvebu_armada-37xx.h > > > > @@ -110,6 +110,7 @@ > > > > > > > > /* fdt_addr and kernel_addr are needed for existing distribution boot scripts */ > > > > #define CONFIG_EXTRA_ENV_SETTINGS \ > > > > + "loadaddr=0x6000000\0" \ > > > > "scriptaddr=0x6d00000\0" \ > > > > "pxefile_addr_r=0x6e00000\0" \ > > > > "fdt_addr=0x6f00000\0" \ > > > > -- > > > > 2.31.1 > > > > > > > > Kind regards, > > Luka Kind regards, Luka
On Friday 13 August 2021 11:33:58 Luka Kovacic wrote: > Hello Pali, > > On Fri, Aug 13, 2021 at 11:31 AM Pali Rohár <pali@kernel.org> wrote: > > > > On Friday 13 August 2021 10:59:33 Luka Kovacic wrote: > > > Hello Pali, > > > > > > On Fri, Aug 13, 2021 at 10:08 AM Pali Rohár <pali@kernel.org> wrote: > > > > > > > > On Friday 13 August 2021 01:39:37 Luka Kovacic wrote: > > > > > Add the loadaddr U-Boot environment variable, as this is available in > > > > > the stock Marvell U-Boot by default on Marvell Armada A37XX platforms. > > > > > > > > Hello Luka! Why is this change needed? Reason that it is in historic > > > > vendor U-Boot does not mean that it has to be also in new mainline > > > > version. > > > > > > > > I have already wrote some reasons in previous review thread: > > > > https://lore.kernel.org/u-boot/20210301150505.3iajeeufi7ahsnk3@pali/ > > > > > > > > I think this change was needed when CONFIG_SYS_LOAD_ADDR was set to > > > > incorrect value, which is already fixed in mainline U-Boot. > > > > > > This value is very useful when building custom Linux boot scripts. > > > Yesterday, I booted the board without this patch and there was no loadaddr > > > variable. > > > > > > Do I understand this correctly? Are you saying that the value in the loadaddr > > > variable should be automatically inherited from CONFIG_SYS_LOAD_ADDR > > > even without this patch? > > > > If you do not specify load address then address from > > CONFIG_SYS_LOAD_ADDR is used. But as I mentioned in above email, > > CONFIG_SYS_LOAD_ADDR was defined incorrectly and which cause that it did > > not worked and caused crashes... > > I tried not specifying it and then the loadaddr variable doesn't even exist. Could you send full example of your command? > > > > > > > > > > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> > > > > > Cc: Luka Perkov <luka.perkov@sartura.hr> > > > > > Cc: Robert Marko <robert.marko@sartura.hr> > > > > > --- > > > > > include/configs/mvebu_armada-37xx.h | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h > > > > > index 8e8bcfa018..6901680e32 100644 > > > > > --- a/include/configs/mvebu_armada-37xx.h > > > > > +++ b/include/configs/mvebu_armada-37xx.h > > > > > @@ -110,6 +110,7 @@ > > > > > > > > > > /* fdt_addr and kernel_addr are needed for existing distribution boot scripts */ > > > > > #define CONFIG_EXTRA_ENV_SETTINGS \ > > > > > + "loadaddr=0x6000000\0" \ > > > > > "scriptaddr=0x6d00000\0" \ > > > > > "pxefile_addr_r=0x6e00000\0" \ > > > > > "fdt_addr=0x6f00000\0" \ > > > > > -- > > > > > 2.31.1 > > > > > > > > > > > Kind regards, > > > Luka > > Kind regards, > Luka
On Friday 13 August 2021 01:39:37 Luka Kovacic wrote: > Add the loadaddr U-Boot environment variable, as this is available in > the stock Marvell U-Boot by default on Marvell Armada A37XX platforms. > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> > Cc: Luka Perkov <luka.perkov@sartura.hr> > Cc: Robert Marko <robert.marko@sartura.hr> > --- > include/configs/mvebu_armada-37xx.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h > index 8e8bcfa018..6901680e32 100644 > --- a/include/configs/mvebu_armada-37xx.h > +++ b/include/configs/mvebu_armada-37xx.h > @@ -110,6 +110,7 @@ > > /* fdt_addr and kernel_addr are needed for existing distribution boot scripts */ > #define CONFIG_EXTRA_ENV_SETTINGS \ > + "loadaddr=0x6000000\0" \ Now I see where is the issue... In file include/env_default.h is: #ifdef CONFIG_LOADADDR "loadaddr=" __stringify(CONFIG_LOADADDR) "\0" #endif So default value for loadaddr= is set only when CONFIG_LOADADDR is defined. But lot of cmd load commands are using config option CONFIG_SYS_LOAD_ADDR as a default value for load address. And also for espressobin we set correct value into CONFIG_SYS_LOAD_ADDR. I'm looking at the u-boot code and CONFIG_LOADADDR is used only by cmd/qfw.c and include/env_default.h. And on all other places (in cmd load commands) is used CONFIG_SYS_LOAD_ADDR. So for me it looks like that cmd/qfw.c and include/env_default.h should be fixed to use CONFIG_SYS_LOAD_ADDR instead of CONFIG_LOADADDR. > "scriptaddr=0x6d00000\0" \ > "pxefile_addr_r=0x6e00000\0" \ > "fdt_addr=0x6f00000\0" \ > -- > 2.31.1 >
Hello Pali, On Fri, Aug 13, 2021 at 3:59 PM Pali Rohár <pali@kernel.org> wrote: > > On Friday 13 August 2021 01:39:37 Luka Kovacic wrote: > > Add the loadaddr U-Boot environment variable, as this is available in > > the stock Marvell U-Boot by default on Marvell Armada A37XX platforms. > > > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> > > Cc: Luka Perkov <luka.perkov@sartura.hr> > > Cc: Robert Marko <robert.marko@sartura.hr> > > --- > > include/configs/mvebu_armada-37xx.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h > > index 8e8bcfa018..6901680e32 100644 > > --- a/include/configs/mvebu_armada-37xx.h > > +++ b/include/configs/mvebu_armada-37xx.h > > @@ -110,6 +110,7 @@ > > > > /* fdt_addr and kernel_addr are needed for existing distribution boot scripts */ > > #define CONFIG_EXTRA_ENV_SETTINGS \ > > + "loadaddr=0x6000000\0" \ > > Now I see where is the issue... > > In file include/env_default.h is: > > #ifdef CONFIG_LOADADDR > "loadaddr=" __stringify(CONFIG_LOADADDR) "\0" > #endif > > So default value for loadaddr= is set only when CONFIG_LOADADDR is > defined. > > But lot of cmd load commands are using config option > CONFIG_SYS_LOAD_ADDR as a default value for load address. > > And also for espressobin we set correct value into CONFIG_SYS_LOAD_ADDR. > > I'm looking at the u-boot code and CONFIG_LOADADDR is used only by > cmd/qfw.c and include/env_default.h. And on all other places (in cmd > load commands) is used CONFIG_SYS_LOAD_ADDR. > > So for me it looks like that cmd/qfw.c and include/env_default.h should > be fixed to use CONFIG_SYS_LOAD_ADDR instead of CONFIG_LOADADDR. Thanks, this makes sense now. I can correct this in the next patchset if that's okay with you. > > > "scriptaddr=0x6d00000\0" \ > > "pxefile_addr_r=0x6e00000\0" \ > > "fdt_addr=0x6f00000\0" \ > > -- > > 2.31.1 > > Kind regards, Luka
On Monday 16 August 2021 10:21:19 Luka Kovacic wrote: > Hello Pali, > > On Fri, Aug 13, 2021 at 3:59 PM Pali Rohár <pali@kernel.org> wrote: > > > > On Friday 13 August 2021 01:39:37 Luka Kovacic wrote: > > > Add the loadaddr U-Boot environment variable, as this is available in > > > the stock Marvell U-Boot by default on Marvell Armada A37XX platforms. > > > > > > Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> > > > Cc: Luka Perkov <luka.perkov@sartura.hr> > > > Cc: Robert Marko <robert.marko@sartura.hr> > > > --- > > > include/configs/mvebu_armada-37xx.h | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h > > > index 8e8bcfa018..6901680e32 100644 > > > --- a/include/configs/mvebu_armada-37xx.h > > > +++ b/include/configs/mvebu_armada-37xx.h > > > @@ -110,6 +110,7 @@ > > > > > > /* fdt_addr and kernel_addr are needed for existing distribution boot scripts */ > > > #define CONFIG_EXTRA_ENV_SETTINGS \ > > > + "loadaddr=0x6000000\0" \ > > > > Now I see where is the issue... > > > > In file include/env_default.h is: > > > > #ifdef CONFIG_LOADADDR > > "loadaddr=" __stringify(CONFIG_LOADADDR) "\0" > > #endif > > > > So default value for loadaddr= is set only when CONFIG_LOADADDR is > > defined. > > > > But lot of cmd load commands are using config option > > CONFIG_SYS_LOAD_ADDR as a default value for load address. > > > > And also for espressobin we set correct value into CONFIG_SYS_LOAD_ADDR. > > > > I'm looking at the u-boot code and CONFIG_LOADADDR is used only by > > cmd/qfw.c and include/env_default.h. And on all other places (in cmd > > load commands) is used CONFIG_SYS_LOAD_ADDR. > > > > So for me it looks like that cmd/qfw.c and include/env_default.h should > > be fixed to use CONFIG_SYS_LOAD_ADDR instead of CONFIG_LOADADDR. > > Thanks, this makes sense now. > I can correct this in the next patchset if that's okay with you. Ok! > > > > > "scriptaddr=0x6d00000\0" \ > > > "pxefile_addr_r=0x6e00000\0" \ > > > "fdt_addr=0x6f00000\0" \ > > > -- > > > 2.31.1 > > > > > Kind regards, > Luka
diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h index 8e8bcfa018..6901680e32 100644 --- a/include/configs/mvebu_armada-37xx.h +++ b/include/configs/mvebu_armada-37xx.h @@ -110,6 +110,7 @@ /* fdt_addr and kernel_addr are needed for existing distribution boot scripts */ #define CONFIG_EXTRA_ENV_SETTINGS \ + "loadaddr=0x6000000\0" \ "scriptaddr=0x6d00000\0" \ "pxefile_addr_r=0x6e00000\0" \ "fdt_addr=0x6f00000\0" \
Add the loadaddr U-Boot environment variable, as this is available in the stock Marvell U-Boot by default on Marvell Armada A37XX platforms. Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr> Cc: Luka Perkov <luka.perkov@sartura.hr> Cc: Robert Marko <robert.marko@sartura.hr> --- include/configs/mvebu_armada-37xx.h | 1 + 1 file changed, 1 insertion(+)