| Message ID | 20180816073810.18096-1-simon.k.r.goldschmidt@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [U-Boot,v5] arm: socfpga: fix U-Boot running from fpga OnChip RAM | expand |
On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: > gd->env_addr points to pre-relocation address even after > relocation. This leads to an abort in env_callback_init > when loading the environment. > > Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> I have one last question -- does this somehow influence SPL ? > --- > > Changes in v5: > Improve comments > > Changes in v4: > enable this fix for all socfpga, not for gen5 only > > Changes in v3: > this patch is new in v3 > > Changes in v2: > None > > include/configs/socfpga_common.h | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h > index 8ebf6b85fe..8b9f0427c0 100644 > --- a/include/configs/socfpga_common.h > +++ b/include/configs/socfpga_common.h > @@ -284,6 +284,18 @@ unsigned int cm_get_qspi_controller_clk_hz(void); > #define CONFIG_SPL_STACK CONFIG_SYS_SPL_MALLOC_START > #endif > > +/* > + * When U-Boot is started from FPGA, prevent gd->env_addr to point into > + * FPGA OnChip RAM after relocation > + */ > +#define CONFIG_SYS_EXTRA_ENV_RELOC > +/* > + * CONFIG_SYS_EXTRA_ENV_RELOC code needs this to calculate the relocation > + * offset for gd->env_addr. Since this is based on gd->relocaddr, we need > + * to use CONFIG_SYS_TEXT_BASE here. > + */ > +#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_TEXT_BASE > + > /* Extra Environment */ > #ifndef CONFIG_SPL_BUILD > >
On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut <marex@denx.de> wrote: > > On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: > > gd->env_addr points to pre-relocation address even after > > relocation. This leads to an abort in env_callback_init > > when loading the environment. > > > > Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. > > > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > > I have one last question -- does this somehow influence SPL ? No, it doesn't. The code that gets enabled by this define is in common/board_r.c, which is not linked for SPL. > > > --- > > > > Changes in v5: > > Improve comments > > > > Changes in v4: > > enable this fix for all socfpga, not for gen5 only > > > > Changes in v3: > > this patch is new in v3 > > > > Changes in v2: > > None > > > > include/configs/socfpga_common.h | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h > > index 8ebf6b85fe..8b9f0427c0 100644 > > --- a/include/configs/socfpga_common.h > > +++ b/include/configs/socfpga_common.h > > @@ -284,6 +284,18 @@ unsigned int cm_get_qspi_controller_clk_hz(void); > > #define CONFIG_SPL_STACK CONFIG_SYS_SPL_MALLOC_START > > #endif > > > > +/* > > + * When U-Boot is started from FPGA, prevent gd->env_addr to point into > > + * FPGA OnChip RAM after relocation > > + */ > > +#define CONFIG_SYS_EXTRA_ENV_RELOC > > +/* > > + * CONFIG_SYS_EXTRA_ENV_RELOC code needs this to calculate the relocation > > + * offset for gd->env_addr. Since this is based on gd->relocaddr, we need > > + * to use CONFIG_SYS_TEXT_BASE here. > > + */ > > +#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_TEXT_BASE > > + > > /* Extra Environment */ > > #ifndef CONFIG_SPL_BUILD > > > > > > > -- > Best regards, > Marek Vasut
On 08/16/2018 03:00 PM, Simon Goldschmidt wrote: > On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut <marex@denx.de> wrote: >> >> On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: >>> gd->env_addr points to pre-relocation address even after >>> relocation. This leads to an abort in env_callback_init >>> when loading the environment. >>> >>> Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. >>> >>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >> >> I have one last question -- does this somehow influence SPL ? > > No, it doesn't. The code that gets enabled by this define is in > common/board_r.c, which is not linked for SPL. Ah, thanks for checking. btw do you think it'd make sense to just enable this by default on all systems and zap the EXTRA_ENV_RELOC macro altogether ? >> >>> --- >>> >>> Changes in v5: >>> Improve comments >>> >>> Changes in v4: >>> enable this fix for all socfpga, not for gen5 only >>> >>> Changes in v3: >>> this patch is new in v3 >>> >>> Changes in v2: >>> None >>> >>> include/configs/socfpga_common.h | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h >>> index 8ebf6b85fe..8b9f0427c0 100644 >>> --- a/include/configs/socfpga_common.h >>> +++ b/include/configs/socfpga_common.h >>> @@ -284,6 +284,18 @@ unsigned int cm_get_qspi_controller_clk_hz(void); >>> #define CONFIG_SPL_STACK CONFIG_SYS_SPL_MALLOC_START >>> #endif >>> >>> +/* >>> + * When U-Boot is started from FPGA, prevent gd->env_addr to point into >>> + * FPGA OnChip RAM after relocation >>> + */ >>> +#define CONFIG_SYS_EXTRA_ENV_RELOC >>> +/* >>> + * CONFIG_SYS_EXTRA_ENV_RELOC code needs this to calculate the relocation >>> + * offset for gd->env_addr. Since this is based on gd->relocaddr, we need >>> + * to use CONFIG_SYS_TEXT_BASE here. >>> + */ >>> +#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_TEXT_BASE >>> + >>> /* Extra Environment */ >>> #ifndef CONFIG_SPL_BUILD >>> >>> >> >> >> -- >> Best regards, >> Marek Vasut
Marek Vasut <marex@denx.de> schrieb am Do., 16. Aug. 2018, 15:06: > On 08/16/2018 03:00 PM, Simon Goldschmidt wrote: > > On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut <marex@denx.de> wrote: > >> > >> On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: > >>> gd->env_addr points to pre-relocation address even after > >>> relocation. This leads to an abort in env_callback_init > >>> when loading the environment. > >>> > >>> Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. > >>> > >>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > >> > >> I have one last question -- does this somehow influence SPL ? > > > > No, it doesn't. The code that gets enabled by this define is in > > common/board_r.c, which is not linked for SPL. > > Ah, thanks for checking. > > btw do you think it'd make sense to just enable this by default on all > systems and zap the EXTRA_ENV_RELOC macro altogether ? > Yes, that's what I have thought about already. Just like the for the embedded device tree relocation, we could then probably use gd->reloc_off instead of CONFIG_SYS_MONITOR_BASE. I'm just not sure this really works for all boards, but it would be worth a try to push after this release is out. Simon > >> > >>> --- > >>> > >>> Changes in v5: > >>> Improve comments > >>> > >>> Changes in v4: > >>> enable this fix for all socfpga, not for gen5 only > >>> > >>> Changes in v3: > >>> this patch is new in v3 > >>> > >>> Changes in v2: > >>> None > >>> > >>> include/configs/socfpga_common.h | 12 ++++++++++++ > >>> 1 file changed, 12 insertions(+) > >>> > >>> diff --git a/include/configs/socfpga_common.h > b/include/configs/socfpga_common.h > >>> index 8ebf6b85fe..8b9f0427c0 100644 > >>> --- a/include/configs/socfpga_common.h > >>> +++ b/include/configs/socfpga_common.h > >>> @@ -284,6 +284,18 @@ unsigned int cm_get_qspi_controller_clk_hz(void); > >>> #define CONFIG_SPL_STACK CONFIG_SYS_SPL_MALLOC_START > >>> #endif > >>> > >>> +/* > >>> + * When U-Boot is started from FPGA, prevent gd->env_addr to point > into > >>> + * FPGA OnChip RAM after relocation > >>> + */ > >>> +#define CONFIG_SYS_EXTRA_ENV_RELOC > >>> +/* > >>> + * CONFIG_SYS_EXTRA_ENV_RELOC code needs this to calculate the > relocation > >>> + * offset for gd->env_addr. Since this is based on gd->relocaddr, we > need > >>> + * to use CONFIG_SYS_TEXT_BASE here. > >>> + */ > >>> +#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_TEXT_BASE > >>> + > >>> /* Extra Environment */ > >>> #ifndef CONFIG_SPL_BUILD > >>> > >>> > >> > >> > >> -- > >> Best regards, > >> Marek Vasut > > > -- > Best regards, > Marek Vasut >
On 08/16/2018 03:12 PM, Simon Goldschmidt wrote: > > > Marek Vasut <marex@denx.de <mailto:marex@denx.de>> schrieb am Do., 16. > Aug. 2018, 15:06: > > On 08/16/2018 03:00 PM, Simon Goldschmidt wrote: > > On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut <marex@denx.de > <mailto:marex@denx.de>> wrote: > >> > >> On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: > >>> gd->env_addr points to pre-relocation address even after > >>> relocation. This leads to an abort in env_callback_init > >>> when loading the environment. > >>> > >>> Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. > >>> > >>> Signed-off-by: Simon Goldschmidt > <simon.k.r.goldschmidt@gmail.com > <mailto:simon.k.r.goldschmidt@gmail.com>> > >> > >> I have one last question -- does this somehow influence SPL ? > > > > No, it doesn't. The code that gets enabled by this define is in > > common/board_r.c, which is not linked for SPL. > > Ah, thanks for checking. > > btw do you think it'd make sense to just enable this by default on all > systems and zap the EXTRA_ENV_RELOC macro altogether ? > > > Yes, that's what I have thought about already. Just like the for the > embedded device tree relocation, we could then probably use > gd->reloc_off instead of CONFIG_SYS_MONITOR_BASE. I'm just not sure this > really works for all boards, but it would be worth a try to push after > this release is out. I think so too. I cannot think of a reason why this shouldn't be enabled in fact.
On Thu, Aug 16, 2018 at 3:15 PM Marek Vasut <marex@denx.de> wrote: > > On 08/16/2018 03:12 PM, Simon Goldschmidt wrote: > > > > > > Marek Vasut <marex@denx.de <mailto:marex@denx.de>> schrieb am Do., 16. > > Aug. 2018, 15:06: > > > > On 08/16/2018 03:00 PM, Simon Goldschmidt wrote: > > > On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut <marex@denx.de > > <mailto:marex@denx.de>> wrote: > > >> > > >> On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: > > >>> gd->env_addr points to pre-relocation address even after > > >>> relocation. This leads to an abort in env_callback_init > > >>> when loading the environment. > > >>> > > >>> Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. > > >>> > > >>> Signed-off-by: Simon Goldschmidt > > <simon.k.r.goldschmidt@gmail.com > > <mailto:simon.k.r.goldschmidt@gmail.com>> > > >> > > >> I have one last question -- does this somehow influence SPL ? > > > > > > No, it doesn't. The code that gets enabled by this define is in > > > common/board_r.c, which is not linked for SPL. > > > > Ah, thanks for checking. > > > > btw do you think it'd make sense to just enable this by default on all > > systems and zap the EXTRA_ENV_RELOC macro altogether ? > > > > > > Yes, that's what I have thought about already. Just like the for the > > embedded device tree relocation, we could then probably use > > gd->reloc_off instead of CONFIG_SYS_MONITOR_BASE. I'm just not sure this > > really works for all boards, but it would be worth a try to push after > > this release is out. > > I think so too. I cannot think of a reason why this shouldn't be enabled > in fact. Exactly. Too me it seems like a leftover, especially given the use of CONFIG_SYS_MONITOR_BASE, which seems a little outdated, too. I've set up a reminder for a patch to remove it after the release. Simon
On 08/16/2018 03:50 PM, Simon Goldschmidt wrote: > On Thu, Aug 16, 2018 at 3:15 PM Marek Vasut <marex@denx.de> wrote: >> >> On 08/16/2018 03:12 PM, Simon Goldschmidt wrote: >>> >>> >>> Marek Vasut <marex@denx.de <mailto:marex@denx.de>> schrieb am Do., 16. >>> Aug. 2018, 15:06: >>> >>> On 08/16/2018 03:00 PM, Simon Goldschmidt wrote: >>> > On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut <marex@denx.de >>> <mailto:marex@denx.de>> wrote: >>> >> >>> >> On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: >>> >>> gd->env_addr points to pre-relocation address even after >>> >>> relocation. This leads to an abort in env_callback_init >>> >>> when loading the environment. >>> >>> >>> >>> Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. >>> >>> >>> >>> Signed-off-by: Simon Goldschmidt >>> <simon.k.r.goldschmidt@gmail.com >>> <mailto:simon.k.r.goldschmidt@gmail.com>> >>> >> >>> >> I have one last question -- does this somehow influence SPL ? >>> > >>> > No, it doesn't. The code that gets enabled by this define is in >>> > common/board_r.c, which is not linked for SPL. >>> >>> Ah, thanks for checking. >>> >>> btw do you think it'd make sense to just enable this by default on all >>> systems and zap the EXTRA_ENV_RELOC macro altogether ? >>> >>> >>> Yes, that's what I have thought about already. Just like the for the >>> embedded device tree relocation, we could then probably use >>> gd->reloc_off instead of CONFIG_SYS_MONITOR_BASE. I'm just not sure this >>> really works for all boards, but it would be worth a try to push after >>> this release is out. >> >> I think so too. I cannot think of a reason why this shouldn't be enabled >> in fact. > > Exactly. Too me it seems like a leftover, especially given the use of > CONFIG_SYS_MONITOR_BASE, which seems a little outdated, too. > I've set up a reminder for a patch to remove it after the release. Feel free to send it now.
On Thu, Aug 16, 2018 at 4:04 PM Marek Vasut <marex@denx.de> wrote: > > On 08/16/2018 03:50 PM, Simon Goldschmidt wrote: > > On Thu, Aug 16, 2018 at 3:15 PM Marek Vasut <marex@denx.de> wrote: > >> > >> On 08/16/2018 03:12 PM, Simon Goldschmidt wrote: > >>> > >>> > >>> Marek Vasut <marex@denx.de <mailto:marex@denx.de>> schrieb am Do., 16. > >>> Aug. 2018, 15:06: > >>> > >>> On 08/16/2018 03:00 PM, Simon Goldschmidt wrote: > >>> > On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut <marex@denx.de > >>> <mailto:marex@denx.de>> wrote: > >>> >> > >>> >> On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: > >>> >>> gd->env_addr points to pre-relocation address even after > >>> >>> relocation. This leads to an abort in env_callback_init > >>> >>> when loading the environment. > >>> >>> > >>> >>> Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. > >>> >>> > >>> >>> Signed-off-by: Simon Goldschmidt > >>> <simon.k.r.goldschmidt@gmail.com > >>> <mailto:simon.k.r.goldschmidt@gmail.com>> > >>> >> > >>> >> I have one last question -- does this somehow influence SPL ? > >>> > > >>> > No, it doesn't. The code that gets enabled by this define is in > >>> > common/board_r.c, which is not linked for SPL. > >>> > >>> Ah, thanks for checking. > >>> > >>> btw do you think it'd make sense to just enable this by default on all > >>> systems and zap the EXTRA_ENV_RELOC macro altogether ? > >>> > >>> > >>> Yes, that's what I have thought about already. Just like the for the > >>> embedded device tree relocation, we could then probably use > >>> gd->reloc_off instead of CONFIG_SYS_MONITOR_BASE. I'm just not sure this > >>> really works for all boards, but it would be worth a try to push after > >>> this release is out. > >> > >> I think so too. I cannot think of a reason why this shouldn't be enabled > >> in fact. > > > > Exactly. Too me it seems like a leftover, especially given the use of > > CONFIG_SYS_MONITOR_BASE, which seems a little outdated, too. > > I've set up a reminder for a patch to remove it after the release. > > Feel free to send it now. OK, I have tried, but it seems it's not that easy: some boards override the initial gd->env_addr by setting CONFIG_ENV_ADDR. So if this is outside of U-Boot's pre-relocation range, it clearly should not be relocated. One might find an improved way to relocate gd->env_addr if it is internal (e.g. checking the range to be in pre-relocation?). But simply removing the EXTRA_ENV_RELOC does not seem to work. Simon
On 08/16/2018 09:44 PM, Simon Goldschmidt wrote: > On Thu, Aug 16, 2018 at 4:04 PM Marek Vasut <marex@denx.de> wrote: >> >> On 08/16/2018 03:50 PM, Simon Goldschmidt wrote: >>> On Thu, Aug 16, 2018 at 3:15 PM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 08/16/2018 03:12 PM, Simon Goldschmidt wrote: >>>>> >>>>> >>>>> Marek Vasut <marex@denx.de <mailto:marex@denx.de>> schrieb am Do., 16. >>>>> Aug. 2018, 15:06: >>>>> >>>>> On 08/16/2018 03:00 PM, Simon Goldschmidt wrote: >>>>> > On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut <marex@denx.de >>>>> <mailto:marex@denx.de>> wrote: >>>>> >> >>>>> >> On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: >>>>> >>> gd->env_addr points to pre-relocation address even after >>>>> >>> relocation. This leads to an abort in env_callback_init >>>>> >>> when loading the environment. >>>>> >>> >>>>> >>> Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. >>>>> >>> >>>>> >>> Signed-off-by: Simon Goldschmidt >>>>> <simon.k.r.goldschmidt@gmail.com >>>>> <mailto:simon.k.r.goldschmidt@gmail.com>> >>>>> >> >>>>> >> I have one last question -- does this somehow influence SPL ? >>>>> > >>>>> > No, it doesn't. The code that gets enabled by this define is in >>>>> > common/board_r.c, which is not linked for SPL. >>>>> >>>>> Ah, thanks for checking. >>>>> >>>>> btw do you think it'd make sense to just enable this by default on all >>>>> systems and zap the EXTRA_ENV_RELOC macro altogether ? >>>>> >>>>> >>>>> Yes, that's what I have thought about already. Just like the for the >>>>> embedded device tree relocation, we could then probably use >>>>> gd->reloc_off instead of CONFIG_SYS_MONITOR_BASE. I'm just not sure this >>>>> really works for all boards, but it would be worth a try to push after >>>>> this release is out. >>>> >>>> I think so too. I cannot think of a reason why this shouldn't be enabled >>>> in fact. >>> >>> Exactly. Too me it seems like a leftover, especially given the use of >>> CONFIG_SYS_MONITOR_BASE, which seems a little outdated, too. >>> I've set up a reminder for a patch to remove it after the release. >> >> Feel free to send it now. > > OK, I have tried, but it seems it's not that easy: some boards > override the initial gd->env_addr by setting CONFIG_ENV_ADDR. So if > this is outside of U-Boot's pre-relocation range, it clearly should > not be relocated. One might find an improved way to relocate > gd->env_addr if it is internal (e.g. checking the range to be in > pre-relocation?). But simply removing the EXTRA_ENV_RELOC does not > seem to work. Shouldn't most of those boards be easily fixable ?
On Fri, Aug 17, 2018 at 1:57 AM Marek Vasut <marex@denx.de> wrote: > > On 08/16/2018 09:44 PM, Simon Goldschmidt wrote: > > On Thu, Aug 16, 2018 at 4:04 PM Marek Vasut <marex@denx.de> wrote: > >> > >> On 08/16/2018 03:50 PM, Simon Goldschmidt wrote: > >>> On Thu, Aug 16, 2018 at 3:15 PM Marek Vasut <marex@denx.de> wrote: > >>>> > >>>> On 08/16/2018 03:12 PM, Simon Goldschmidt wrote: > >>>>> > >>>>> > >>>>> Marek Vasut <marex@denx.de <mailto:marex@denx.de>> schrieb am Do., 16. > >>>>> Aug. 2018, 15:06: > >>>>> > >>>>> On 08/16/2018 03:00 PM, Simon Goldschmidt wrote: > >>>>> > On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut <marex@denx.de > >>>>> <mailto:marex@denx.de>> wrote: > >>>>> >> > >>>>> >> On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: > >>>>> >>> gd->env_addr points to pre-relocation address even after > >>>>> >>> relocation. This leads to an abort in env_callback_init > >>>>> >>> when loading the environment. > >>>>> >>> > >>>>> >>> Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. > >>>>> >>> > >>>>> >>> Signed-off-by: Simon Goldschmidt > >>>>> <simon.k.r.goldschmidt@gmail.com > >>>>> <mailto:simon.k.r.goldschmidt@gmail.com>> > >>>>> >> > >>>>> >> I have one last question -- does this somehow influence SPL ? > >>>>> > > >>>>> > No, it doesn't. The code that gets enabled by this define is in > >>>>> > common/board_r.c, which is not linked for SPL. > >>>>> > >>>>> Ah, thanks for checking. > >>>>> > >>>>> btw do you think it'd make sense to just enable this by default on all > >>>>> systems and zap the EXTRA_ENV_RELOC macro altogether ? > >>>>> > >>>>> > >>>>> Yes, that's what I have thought about already. Just like the for the > >>>>> embedded device tree relocation, we could then probably use > >>>>> gd->reloc_off instead of CONFIG_SYS_MONITOR_BASE. I'm just not sure this > >>>>> really works for all boards, but it would be worth a try to push after > >>>>> this release is out. > >>>> > >>>> I think so too. I cannot think of a reason why this shouldn't be enabled > >>>> in fact. > >>> > >>> Exactly. Too me it seems like a leftover, especially given the use of > >>> CONFIG_SYS_MONITOR_BASE, which seems a little outdated, too. > >>> I've set up a reminder for a patch to remove it after the release. > >> > >> Feel free to send it now. > > > > OK, I have tried, but it seems it's not that easy: some boards > > override the initial gd->env_addr by setting CONFIG_ENV_ADDR. So if > > this is outside of U-Boot's pre-relocation range, it clearly should > > not be relocated. One might find an improved way to relocate > > gd->env_addr if it is internal (e.g. checking the range to be in > > pre-relocation?). But simply removing the EXTRA_ENV_RELOC does not > > seem to work. > > Shouldn't most of those boards be easily fixable ? Well, if we unconditionally alter gd->env_addr by gd->reloc_off, boards that have their initial gd->env_addr outside of the initial binary can be fixed only by changing their behaviour. I don't know how widely used this feature is, but since it's a config option (CONFIG_ENV_ADDR), how would we know? So to me that means we still have to make this overridable and could change the "default" state of such an option only. Meaning that the default is "relocate gd->env_addr" with an option to leave it. But is this really worth breaking existing boards? Simon
On 08/17/2018 08:56 AM, Simon Goldschmidt wrote: > On Fri, Aug 17, 2018 at 1:57 AM Marek Vasut <marex@denx.de> wrote: >> >> On 08/16/2018 09:44 PM, Simon Goldschmidt wrote: >>> On Thu, Aug 16, 2018 at 4:04 PM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 08/16/2018 03:50 PM, Simon Goldschmidt wrote: >>>>> On Thu, Aug 16, 2018 at 3:15 PM Marek Vasut <marex@denx.de> wrote: >>>>>> >>>>>> On 08/16/2018 03:12 PM, Simon Goldschmidt wrote: >>>>>>> >>>>>>> >>>>>>> Marek Vasut <marex@denx.de <mailto:marex@denx.de>> schrieb am Do., 16. >>>>>>> Aug. 2018, 15:06: >>>>>>> >>>>>>> On 08/16/2018 03:00 PM, Simon Goldschmidt wrote: >>>>>>> > On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut <marex@denx.de >>>>>>> <mailto:marex@denx.de>> wrote: >>>>>>> >> >>>>>>> >> On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: >>>>>>> >>> gd->env_addr points to pre-relocation address even after >>>>>>> >>> relocation. This leads to an abort in env_callback_init >>>>>>> >>> when loading the environment. >>>>>>> >>> >>>>>>> >>> Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. >>>>>>> >>> >>>>>>> >>> Signed-off-by: Simon Goldschmidt >>>>>>> <simon.k.r.goldschmidt@gmail.com >>>>>>> <mailto:simon.k.r.goldschmidt@gmail.com>> >>>>>>> >> >>>>>>> >> I have one last question -- does this somehow influence SPL ? >>>>>>> > >>>>>>> > No, it doesn't. The code that gets enabled by this define is in >>>>>>> > common/board_r.c, which is not linked for SPL. >>>>>>> >>>>>>> Ah, thanks for checking. >>>>>>> >>>>>>> btw do you think it'd make sense to just enable this by default on all >>>>>>> systems and zap the EXTRA_ENV_RELOC macro altogether ? >>>>>>> >>>>>>> >>>>>>> Yes, that's what I have thought about already. Just like the for the >>>>>>> embedded device tree relocation, we could then probably use >>>>>>> gd->reloc_off instead of CONFIG_SYS_MONITOR_BASE. I'm just not sure this >>>>>>> really works for all boards, but it would be worth a try to push after >>>>>>> this release is out. >>>>>> >>>>>> I think so too. I cannot think of a reason why this shouldn't be enabled >>>>>> in fact. >>>>> >>>>> Exactly. Too me it seems like a leftover, especially given the use of >>>>> CONFIG_SYS_MONITOR_BASE, which seems a little outdated, too. >>>>> I've set up a reminder for a patch to remove it after the release. >>>> >>>> Feel free to send it now. >>> >>> OK, I have tried, but it seems it's not that easy: some boards >>> override the initial gd->env_addr by setting CONFIG_ENV_ADDR. So if >>> this is outside of U-Boot's pre-relocation range, it clearly should >>> not be relocated. One might find an improved way to relocate >>> gd->env_addr if it is internal (e.g. checking the range to be in >>> pre-relocation?). But simply removing the EXTRA_ENV_RELOC does not >>> seem to work. >> >> Shouldn't most of those boards be easily fixable ? > > Well, if we unconditionally alter gd->env_addr by gd->reloc_off, > boards that have their initial gd->env_addr outside of the initial > binary can be fixed only by changing their behaviour. I don't know how > widely used this feature is, but since it's a config option > (CONFIG_ENV_ADDR), how would we know? git grep ? But aren't you mixing CONFIG_ENV_ADDR and CONFIG_ENV_RELOC ? > So to me that means we still have to make this overridable and could > change the "default" state of such an option only. Meaning that the > default is "relocate gd->env_addr" with an option to leave it. But is > this really worth breaking existing boards? I think you do want to relocate the env alongside U-Boot, always, no ?
On Fri, Aug 17, 2018 at 12:20 PM Marek Vasut <marex@denx.de> wrote: > > On 08/17/2018 08:56 AM, Simon Goldschmidt wrote: > > On Fri, Aug 17, 2018 at 1:57 AM Marek Vasut <marex@denx.de> wrote: > >> > >> On 08/16/2018 09:44 PM, Simon Goldschmidt wrote: > >>> On Thu, Aug 16, 2018 at 4:04 PM Marek Vasut <marex@denx.de> wrote: > >>>> > >>>> On 08/16/2018 03:50 PM, Simon Goldschmidt wrote: > >>>>> On Thu, Aug 16, 2018 at 3:15 PM Marek Vasut <marex@denx.de> wrote: > >>>>>> > >>>>>> On 08/16/2018 03:12 PM, Simon Goldschmidt wrote: > >>>>>>> > >>>>>>> > >>>>>>> Marek Vasut <marex@denx.de <mailto:marex@denx.de>> schrieb am Do., 16. > >>>>>>> Aug. 2018, 15:06: > >>>>>>> > >>>>>>> On 08/16/2018 03:00 PM, Simon Goldschmidt wrote: > >>>>>>> > On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut <marex@denx.de > >>>>>>> <mailto:marex@denx.de>> wrote: > >>>>>>> >> > >>>>>>> >> On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: > >>>>>>> >>> gd->env_addr points to pre-relocation address even after > >>>>>>> >>> relocation. This leads to an abort in env_callback_init > >>>>>>> >>> when loading the environment. > >>>>>>> >>> > >>>>>>> >>> Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. > >>>>>>> >>> > >>>>>>> >>> Signed-off-by: Simon Goldschmidt > >>>>>>> <simon.k.r.goldschmidt@gmail.com > >>>>>>> <mailto:simon.k.r.goldschmidt@gmail.com>> > >>>>>>> >> > >>>>>>> >> I have one last question -- does this somehow influence SPL ? > >>>>>>> > > >>>>>>> > No, it doesn't. The code that gets enabled by this define is in > >>>>>>> > common/board_r.c, which is not linked for SPL. > >>>>>>> > >>>>>>> Ah, thanks for checking. > >>>>>>> > >>>>>>> btw do you think it'd make sense to just enable this by default on all > >>>>>>> systems and zap the EXTRA_ENV_RELOC macro altogether ? > >>>>>>> > >>>>>>> > >>>>>>> Yes, that's what I have thought about already. Just like the for the > >>>>>>> embedded device tree relocation, we could then probably use > >>>>>>> gd->reloc_off instead of CONFIG_SYS_MONITOR_BASE. I'm just not sure this > >>>>>>> really works for all boards, but it would be worth a try to push after > >>>>>>> this release is out. > >>>>>> > >>>>>> I think so too. I cannot think of a reason why this shouldn't be enabled > >>>>>> in fact. > >>>>> > >>>>> Exactly. Too me it seems like a leftover, especially given the use of > >>>>> CONFIG_SYS_MONITOR_BASE, which seems a little outdated, too. > >>>>> I've set up a reminder for a patch to remove it after the release. > >>>> > >>>> Feel free to send it now. > >>> > >>> OK, I have tried, but it seems it's not that easy: some boards > >>> override the initial gd->env_addr by setting CONFIG_ENV_ADDR. So if > >>> this is outside of U-Boot's pre-relocation range, it clearly should > >>> not be relocated. One might find an improved way to relocate > >>> gd->env_addr if it is internal (e.g. checking the range to be in > >>> pre-relocation?). But simply removing the EXTRA_ENV_RELOC does not > >>> seem to work. > >> > >> Shouldn't most of those boards be easily fixable ? > > > > Well, if we unconditionally alter gd->env_addr by gd->reloc_off, > > boards that have their initial gd->env_addr outside of the initial > > binary can be fixed only by changing their behaviour. I don't know how > > widely used this feature is, but since it's a config option > > (CONFIG_ENV_ADDR), how would we know? > > git grep ? But aren't you mixing CONFIG_ENV_ADDR and CONFIG_ENV_RELOC ? Have a look at env_sf_init() in env/sf.c (called from env_init(), which in turn is called from board_init_f()). There gd->env_addr is initialized by a config setting. If this user-supplied address is outside of the binary, relocating it is wrong, isn't it? Anyway, I'm off for 2 weeks now (holiday time here) with some email access at most, so I'll continue on this when I get back. Simon > > > So to me that means we still have to make this overridable and could > > change the "default" state of such an option only. Meaning that the > > default is "relocate gd->env_addr" with an option to leave it. But is > > this really worth breaking existing boards? > > I think you do want to relocate the env alongside U-Boot, always, no ? > > -- > Best regards, > Marek Vasut
On 08/18/2018 10:55 AM, Simon Goldschmidt wrote: > On Fri, Aug 17, 2018 at 12:20 PM Marek Vasut <marex@denx.de> wrote: >> >> On 08/17/2018 08:56 AM, Simon Goldschmidt wrote: >>> On Fri, Aug 17, 2018 at 1:57 AM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 08/16/2018 09:44 PM, Simon Goldschmidt wrote: >>>>> On Thu, Aug 16, 2018 at 4:04 PM Marek Vasut <marex@denx.de> wrote: >>>>>> >>>>>> On 08/16/2018 03:50 PM, Simon Goldschmidt wrote: >>>>>>> On Thu, Aug 16, 2018 at 3:15 PM Marek Vasut <marex@denx.de> wrote: >>>>>>>> >>>>>>>> On 08/16/2018 03:12 PM, Simon Goldschmidt wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> Marek Vasut <marex@denx.de <mailto:marex@denx.de>> schrieb am Do., 16. >>>>>>>>> Aug. 2018, 15:06: >>>>>>>>> >>>>>>>>> On 08/16/2018 03:00 PM, Simon Goldschmidt wrote: >>>>>>>>> > On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut <marex@denx.de >>>>>>>>> <mailto:marex@denx.de>> wrote: >>>>>>>>> >> >>>>>>>>> >> On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: >>>>>>>>> >>> gd->env_addr points to pre-relocation address even after >>>>>>>>> >>> relocation. This leads to an abort in env_callback_init >>>>>>>>> >>> when loading the environment. >>>>>>>>> >>> >>>>>>>>> >>> Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. >>>>>>>>> >>> >>>>>>>>> >>> Signed-off-by: Simon Goldschmidt >>>>>>>>> <simon.k.r.goldschmidt@gmail.com >>>>>>>>> <mailto:simon.k.r.goldschmidt@gmail.com>> >>>>>>>>> >> >>>>>>>>> >> I have one last question -- does this somehow influence SPL ? >>>>>>>>> > >>>>>>>>> > No, it doesn't. The code that gets enabled by this define is in >>>>>>>>> > common/board_r.c, which is not linked for SPL. >>>>>>>>> >>>>>>>>> Ah, thanks for checking. >>>>>>>>> >>>>>>>>> btw do you think it'd make sense to just enable this by default on all >>>>>>>>> systems and zap the EXTRA_ENV_RELOC macro altogether ? >>>>>>>>> >>>>>>>>> >>>>>>>>> Yes, that's what I have thought about already. Just like the for the >>>>>>>>> embedded device tree relocation, we could then probably use >>>>>>>>> gd->reloc_off instead of CONFIG_SYS_MONITOR_BASE. I'm just not sure this >>>>>>>>> really works for all boards, but it would be worth a try to push after >>>>>>>>> this release is out. >>>>>>>> >>>>>>>> I think so too. I cannot think of a reason why this shouldn't be enabled >>>>>>>> in fact. >>>>>>> >>>>>>> Exactly. Too me it seems like a leftover, especially given the use of >>>>>>> CONFIG_SYS_MONITOR_BASE, which seems a little outdated, too. >>>>>>> I've set up a reminder for a patch to remove it after the release. >>>>>> >>>>>> Feel free to send it now. >>>>> >>>>> OK, I have tried, but it seems it's not that easy: some boards >>>>> override the initial gd->env_addr by setting CONFIG_ENV_ADDR. So if >>>>> this is outside of U-Boot's pre-relocation range, it clearly should >>>>> not be relocated. One might find an improved way to relocate >>>>> gd->env_addr if it is internal (e.g. checking the range to be in >>>>> pre-relocation?). But simply removing the EXTRA_ENV_RELOC does not >>>>> seem to work. >>>> >>>> Shouldn't most of those boards be easily fixable ? >>> >>> Well, if we unconditionally alter gd->env_addr by gd->reloc_off, >>> boards that have their initial gd->env_addr outside of the initial >>> binary can be fixed only by changing their behaviour. I don't know how >>> widely used this feature is, but since it's a config option >>> (CONFIG_ENV_ADDR), how would we know? >> >> git grep ? But aren't you mixing CONFIG_ENV_ADDR and CONFIG_ENV_RELOC ? > > Have a look at env_sf_init() in env/sf.c (called from env_init(), > which in turn is called from board_init_f()). There gd->env_addr is > initialized by a config setting. If this user-supplied address is > outside of the binary, relocating it is wrong, isn't it? I think you want to relocate the env close to where U-Boot is relocated in all cases, no ? > Anyway, I'm off for 2 weeks now (holiday time here) with some email > access at most, so I'll continue on this when I get back. Have a nice vacation. > Simon > >> >>> So to me that means we still have to make this overridable and could >>> change the "default" state of such an option only. Meaning that the >>> default is "relocate gd->env_addr" with an option to leave it. But is >>> this really worth breaking existing boards? >> >> I think you do want to relocate the env alongside U-Boot, always, no ? >> >> -- >> Best regards, >> Marek Vasut
OK, so I got more than 2 weeks off of U-Boot, but here I'm back... ;-) On 18.08.2018 14:25, Marek Vasut wrote: > On 08/18/2018 10:55 AM, Simon Goldschmidt wrote: >> On Fri, Aug 17, 2018 at 12:20 PM Marek Vasut <marex@denx.de> wrote: >>> On 08/17/2018 08:56 AM, Simon Goldschmidt wrote: >>>> On Fri, Aug 17, 2018 at 1:57 AM Marek Vasut <marex@denx.de> wrote: >>>>> On 08/16/2018 09:44 PM, Simon Goldschmidt wrote: >>>>>> On Thu, Aug 16, 2018 at 4:04 PM Marek Vasut <marex@denx.de> wrote: >>>>>>> On 08/16/2018 03:50 PM, Simon Goldschmidt wrote: >>>>>>>> On Thu, Aug 16, 2018 at 3:15 PM Marek Vasut <marex@denx.de> wrote: >>>>>>>>> On 08/16/2018 03:12 PM, Simon Goldschmidt wrote: >>>>>>>>>> >>>>>>>>>> Marek Vasut <marex@denx.de <mailto:marex@denx.de>> schrieb am Do., 16. >>>>>>>>>> Aug. 2018, 15:06: >>>>>>>>>> >>>>>>>>>> On 08/16/2018 03:00 PM, Simon Goldschmidt wrote: >>>>>>>>>> > On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut <marex@denx.de >>>>>>>>>> <mailto:marex@denx.de>> wrote: >>>>>>>>>> >> >>>>>>>>>> >> On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: >>>>>>>>>> >>> gd->env_addr points to pre-relocation address even after >>>>>>>>>> >>> relocation. This leads to an abort in env_callback_init >>>>>>>>>> >>> when loading the environment. >>>>>>>>>> >>> >>>>>>>>>> >>> Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. >>>>>>>>>> >>> >>>>>>>>>> >>> Signed-off-by: Simon Goldschmidt >>>>>>>>>> <simon.k.r.goldschmidt@gmail.com >>>>>>>>>> <mailto:simon.k.r.goldschmidt@gmail.com>> >>>>>>>>>> >> >>>>>>>>>> >> I have one last question -- does this somehow influence SPL ? >>>>>>>>>> > >>>>>>>>>> > No, it doesn't. The code that gets enabled by this define is in >>>>>>>>>> > common/board_r.c, which is not linked for SPL. >>>>>>>>>> >>>>>>>>>> Ah, thanks for checking. >>>>>>>>>> >>>>>>>>>> btw do you think it'd make sense to just enable this by default on all >>>>>>>>>> systems and zap the EXTRA_ENV_RELOC macro altogether ? >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Yes, that's what I have thought about already. Just like the for the >>>>>>>>>> embedded device tree relocation, we could then probably use >>>>>>>>>> gd->reloc_off instead of CONFIG_SYS_MONITOR_BASE. I'm just not sure this >>>>>>>>>> really works for all boards, but it would be worth a try to push after >>>>>>>>>> this release is out. >>>>>>>>> I think so too. I cannot think of a reason why this shouldn't be enabled >>>>>>>>> in fact. >>>>>>>> Exactly. Too me it seems like a leftover, especially given the use of >>>>>>>> CONFIG_SYS_MONITOR_BASE, which seems a little outdated, too. >>>>>>>> I've set up a reminder for a patch to remove it after the release. >>>>>>> Feel free to send it now. >>>>>> OK, I have tried, but it seems it's not that easy: some boards >>>>>> override the initial gd->env_addr by setting CONFIG_ENV_ADDR. So if >>>>>> this is outside of U-Boot's pre-relocation range, it clearly should >>>>>> not be relocated. One might find an improved way to relocate >>>>>> gd->env_addr if it is internal (e.g. checking the range to be in >>>>>> pre-relocation?). But simply removing the EXTRA_ENV_RELOC does not >>>>>> seem to work. >>>>> Shouldn't most of those boards be easily fixable ? >>>> Well, if we unconditionally alter gd->env_addr by gd->reloc_off, >>>> boards that have their initial gd->env_addr outside of the initial >>>> binary can be fixed only by changing their behaviour. I don't know how >>>> widely used this feature is, but since it's a config option >>>> (CONFIG_ENV_ADDR), how would we know? >>> git grep ? But aren't you mixing CONFIG_ENV_ADDR and CONFIG_ENV_RELOC ? >> Have a look at env_sf_init() in env/sf.c (called from env_init(), >> which in turn is called from board_init_f()). There gd->env_addr is >> initialized by a config setting. If this user-supplied address is >> outside of the binary, relocating it is wrong, isn't it? > I think you want to relocate the env close to where U-Boot is relocated > in all cases, no ? Hmm, yes, *I do*, but reading env/sf.c it seems like there *are* ways to have this initial environment located outside of the initial binary hence making relocation invalidate it. Now I'm in no position to see if this is an error or legal usage of the code as it was meant to be... So I think we have two options: a) apply CONFIG_SYS_EXTRA_ENV_RELOC for socfpga or b) push a patch that always relocates the initial environment and see if someone cares... Which of the two do you prefer? Simon
On 09/17/2018 10:44 PM, Simon Goldschmidt wrote: > > OK, so I got more than 2 weeks off of U-Boot, but here I'm back... ;-) > > On 18.08.2018 14:25, Marek Vasut wrote: >> On 08/18/2018 10:55 AM, Simon Goldschmidt wrote: >>> On Fri, Aug 17, 2018 at 12:20 PM Marek Vasut <marex@denx.de> wrote: >>>> On 08/17/2018 08:56 AM, Simon Goldschmidt wrote: >>>>> On Fri, Aug 17, 2018 at 1:57 AM Marek Vasut <marex@denx.de> wrote: >>>>>> On 08/16/2018 09:44 PM, Simon Goldschmidt wrote: >>>>>>> On Thu, Aug 16, 2018 at 4:04 PM Marek Vasut <marex@denx.de> wrote: >>>>>>>> On 08/16/2018 03:50 PM, Simon Goldschmidt wrote: >>>>>>>>> On Thu, Aug 16, 2018 at 3:15 PM Marek Vasut <marex@denx.de> wrote: >>>>>>>>>> On 08/16/2018 03:12 PM, Simon Goldschmidt wrote: >>>>>>>>>>> >>>>>>>>>>> Marek Vasut <marex@denx.de <mailto:marex@denx.de>> schrieb am >>>>>>>>>>> Do., 16. >>>>>>>>>>> Aug. 2018, 15:06: >>>>>>>>>>> >>>>>>>>>>> On 08/16/2018 03:00 PM, Simon Goldschmidt wrote: >>>>>>>>>>> > On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut >>>>>>>>>>> <marex@denx.de >>>>>>>>>>> <mailto:marex@denx.de>> wrote: >>>>>>>>>>> >> >>>>>>>>>>> >> On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: >>>>>>>>>>> >>> gd->env_addr points to pre-relocation address even >>>>>>>>>>> after >>>>>>>>>>> >>> relocation. This leads to an abort in env_callback_init >>>>>>>>>>> >>> when loading the environment. >>>>>>>>>>> >>> >>>>>>>>>>> >>> Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. >>>>>>>>>>> >>> >>>>>>>>>>> >>> Signed-off-by: Simon Goldschmidt >>>>>>>>>>> <simon.k.r.goldschmidt@gmail.com >>>>>>>>>>> <mailto:simon.k.r.goldschmidt@gmail.com>> >>>>>>>>>>> >> >>>>>>>>>>> >> I have one last question -- does this somehow >>>>>>>>>>> influence SPL ? >>>>>>>>>>> > >>>>>>>>>>> > No, it doesn't. The code that gets enabled by this >>>>>>>>>>> define is in >>>>>>>>>>> > common/board_r.c, which is not linked for SPL. >>>>>>>>>>> >>>>>>>>>>> Ah, thanks for checking. >>>>>>>>>>> >>>>>>>>>>> btw do you think it'd make sense to just enable this by >>>>>>>>>>> default on all >>>>>>>>>>> systems and zap the EXTRA_ENV_RELOC macro altogether ? >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Yes, that's what I have thought about already. Just like the >>>>>>>>>>> for the >>>>>>>>>>> embedded device tree relocation, we could then probably use >>>>>>>>>>> gd->reloc_off instead of CONFIG_SYS_MONITOR_BASE. I'm just >>>>>>>>>>> not sure this >>>>>>>>>>> really works for all boards, but it would be worth a try to >>>>>>>>>>> push after >>>>>>>>>>> this release is out. >>>>>>>>>> I think so too. I cannot think of a reason why this shouldn't >>>>>>>>>> be enabled >>>>>>>>>> in fact. >>>>>>>>> Exactly. Too me it seems like a leftover, especially given the >>>>>>>>> use of >>>>>>>>> CONFIG_SYS_MONITOR_BASE, which seems a little outdated, too. >>>>>>>>> I've set up a reminder for a patch to remove it after the release. >>>>>>>> Feel free to send it now. >>>>>>> OK, I have tried, but it seems it's not that easy: some boards >>>>>>> override the initial gd->env_addr by setting CONFIG_ENV_ADDR. So if >>>>>>> this is outside of U-Boot's pre-relocation range, it clearly should >>>>>>> not be relocated. One might find an improved way to relocate >>>>>>> gd->env_addr if it is internal (e.g. checking the range to be in >>>>>>> pre-relocation?). But simply removing the EXTRA_ENV_RELOC does not >>>>>>> seem to work. >>>>>> Shouldn't most of those boards be easily fixable ? >>>>> Well, if we unconditionally alter gd->env_addr by gd->reloc_off, >>>>> boards that have their initial gd->env_addr outside of the initial >>>>> binary can be fixed only by changing their behaviour. I don't know how >>>>> widely used this feature is, but since it's a config option >>>>> (CONFIG_ENV_ADDR), how would we know? >>>> git grep ? But aren't you mixing CONFIG_ENV_ADDR and CONFIG_ENV_RELOC ? >>> Have a look at env_sf_init() in env/sf.c (called from env_init(), >>> which in turn is called from board_init_f()). There gd->env_addr is >>> initialized by a config setting. If this user-supplied address is >>> outside of the binary, relocating it is wrong, isn't it? >> I think you want to relocate the env close to where U-Boot is relocated >> in all cases, no ? > > Hmm, yes, *I do*, but reading env/sf.c it seems like there *are* ways to > have this initial environment located outside of the initial binary > hence making relocation invalidate it. Now I'm in no position to see if > this is an error or legal usage of the code as it was meant to be... > > So I think we have two options: > a) apply CONFIG_SYS_EXTRA_ENV_RELOC for socfpga or But SoCFPGA can have env in SF too, so you cannot apply this too all SoCFPGA, unless I am wrong. > b) push a patch that always relocates the initial environment and see if > someone cares... Wouldn't it make sense to fix the sf and then enable env reloc for it too ? > Which of the two do you prefer? > > Simon
On Tue, Sep 18, 2018 at 12:12 PM Marek Vasut <marex@denx.de> wrote: > > On 09/17/2018 10:44 PM, Simon Goldschmidt wrote: > > > > OK, so I got more than 2 weeks off of U-Boot, but here I'm back... ;-) > > > > On 18.08.2018 14:25, Marek Vasut wrote: > >> On 08/18/2018 10:55 AM, Simon Goldschmidt wrote: > >>> On Fri, Aug 17, 2018 at 12:20 PM Marek Vasut <marex@denx.de> wrote: > >>>> On 08/17/2018 08:56 AM, Simon Goldschmidt wrote: > >>>>> On Fri, Aug 17, 2018 at 1:57 AM Marek Vasut <marex@denx.de> wrote: > >>>>>> On 08/16/2018 09:44 PM, Simon Goldschmidt wrote: > >>>>>>> On Thu, Aug 16, 2018 at 4:04 PM Marek Vasut <marex@denx.de> wrote: > >>>>>>>> On 08/16/2018 03:50 PM, Simon Goldschmidt wrote: > >>>>>>>>> On Thu, Aug 16, 2018 at 3:15 PM Marek Vasut <marex@denx.de> wrote: > >>>>>>>>>> On 08/16/2018 03:12 PM, Simon Goldschmidt wrote: > >>>>>>>>>>> > >>>>>>>>>>> Marek Vasut <marex@denx.de <mailto:marex@denx.de>> schrieb am > >>>>>>>>>>> Do., 16. > >>>>>>>>>>> Aug. 2018, 15:06: > >>>>>>>>>>> > >>>>>>>>>>> On 08/16/2018 03:00 PM, Simon Goldschmidt wrote: > >>>>>>>>>>> > On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut > >>>>>>>>>>> <marex@denx.de > >>>>>>>>>>> <mailto:marex@denx.de>> wrote: > >>>>>>>>>>> >> > >>>>>>>>>>> >> On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: > >>>>>>>>>>> >>> gd->env_addr points to pre-relocation address even > >>>>>>>>>>> after > >>>>>>>>>>> >>> relocation. This leads to an abort in env_callback_init > >>>>>>>>>>> >>> when loading the environment. > >>>>>>>>>>> >>> > >>>>>>>>>>> >>> Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. > >>>>>>>>>>> >>> > >>>>>>>>>>> >>> Signed-off-by: Simon Goldschmidt > >>>>>>>>>>> <simon.k.r.goldschmidt@gmail.com > >>>>>>>>>>> <mailto:simon.k.r.goldschmidt@gmail.com>> > >>>>>>>>>>> >> > >>>>>>>>>>> >> I have one last question -- does this somehow > >>>>>>>>>>> influence SPL ? > >>>>>>>>>>> > > >>>>>>>>>>> > No, it doesn't. The code that gets enabled by this > >>>>>>>>>>> define is in > >>>>>>>>>>> > common/board_r.c, which is not linked for SPL. > >>>>>>>>>>> > >>>>>>>>>>> Ah, thanks for checking. > >>>>>>>>>>> > >>>>>>>>>>> btw do you think it'd make sense to just enable this by > >>>>>>>>>>> default on all > >>>>>>>>>>> systems and zap the EXTRA_ENV_RELOC macro altogether ? > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> Yes, that's what I have thought about already. Just like the > >>>>>>>>>>> for the > >>>>>>>>>>> embedded device tree relocation, we could then probably use > >>>>>>>>>>> gd->reloc_off instead of CONFIG_SYS_MONITOR_BASE. I'm just > >>>>>>>>>>> not sure this > >>>>>>>>>>> really works for all boards, but it would be worth a try to > >>>>>>>>>>> push after > >>>>>>>>>>> this release is out. > >>>>>>>>>> I think so too. I cannot think of a reason why this shouldn't > >>>>>>>>>> be enabled > >>>>>>>>>> in fact. > >>>>>>>>> Exactly. Too me it seems like a leftover, especially given the > >>>>>>>>> use of > >>>>>>>>> CONFIG_SYS_MONITOR_BASE, which seems a little outdated, too. > >>>>>>>>> I've set up a reminder for a patch to remove it after the release. > >>>>>>>> Feel free to send it now. > >>>>>>> OK, I have tried, but it seems it's not that easy: some boards > >>>>>>> override the initial gd->env_addr by setting CONFIG_ENV_ADDR. So if > >>>>>>> this is outside of U-Boot's pre-relocation range, it clearly should > >>>>>>> not be relocated. One might find an improved way to relocate > >>>>>>> gd->env_addr if it is internal (e.g. checking the range to be in > >>>>>>> pre-relocation?). But simply removing the EXTRA_ENV_RELOC does not > >>>>>>> seem to work. > >>>>>> Shouldn't most of those boards be easily fixable ? > >>>>> Well, if we unconditionally alter gd->env_addr by gd->reloc_off, > >>>>> boards that have their initial gd->env_addr outside of the initial > >>>>> binary can be fixed only by changing their behaviour. I don't know how > >>>>> widely used this feature is, but since it's a config option > >>>>> (CONFIG_ENV_ADDR), how would we know? > >>>> git grep ? But aren't you mixing CONFIG_ENV_ADDR and CONFIG_ENV_RELOC ? > >>> Have a look at env_sf_init() in env/sf.c (called from env_init(), > >>> which in turn is called from board_init_f()). There gd->env_addr is > >>> initialized by a config setting. If this user-supplied address is > >>> outside of the binary, relocating it is wrong, isn't it? > >> I think you want to relocate the env close to where U-Boot is relocated > >> in all cases, no ? > > > > Hmm, yes, *I do*, but reading env/sf.c it seems like there *are* ways to > > have this initial environment located outside of the initial binary > > hence making relocation invalidate it. Now I'm in no position to see if > > this is an error or legal usage of the code as it was meant to be... > > > > So I think we have two options: > > a) apply CONFIG_SYS_EXTRA_ENV_RELOC for socfpga or > > But SoCFPGA can have env in SF too, so you cannot apply this too all > SoCFPGA, unless I am wrong. It's again more confusing. This is only a problem if CONFIG_ENV_ADDR is defined, which isn't for socfpga. > > > b) push a patch that always relocates the initial environment and see if > > someone cares... > > Wouldn't it make sense to fix the sf and then enable env reloc for it too ? sf was only an example. AT least nand, flash and nvram env drivers also can put gd->env_addr at a different, configurable address. The only way to auto-relocate here would be to have a flag that tells us what to do. Or we could check if gd->env_addr is in the range of relocated code. But the whole code in that area just pretty much confuses me... Simon
On 09/18/2018 12:29 PM, Simon Goldschmidt wrote: > On Tue, Sep 18, 2018 at 12:12 PM Marek Vasut <marex@denx.de> wrote: >> >> On 09/17/2018 10:44 PM, Simon Goldschmidt wrote: >>> >>> OK, so I got more than 2 weeks off of U-Boot, but here I'm back... ;-) >>> >>> On 18.08.2018 14:25, Marek Vasut wrote: >>>> On 08/18/2018 10:55 AM, Simon Goldschmidt wrote: >>>>> On Fri, Aug 17, 2018 at 12:20 PM Marek Vasut <marex@denx.de> wrote: >>>>>> On 08/17/2018 08:56 AM, Simon Goldschmidt wrote: >>>>>>> On Fri, Aug 17, 2018 at 1:57 AM Marek Vasut <marex@denx.de> wrote: >>>>>>>> On 08/16/2018 09:44 PM, Simon Goldschmidt wrote: >>>>>>>>> On Thu, Aug 16, 2018 at 4:04 PM Marek Vasut <marex@denx.de> wrote: >>>>>>>>>> On 08/16/2018 03:50 PM, Simon Goldschmidt wrote: >>>>>>>>>>> On Thu, Aug 16, 2018 at 3:15 PM Marek Vasut <marex@denx.de> wrote: >>>>>>>>>>>> On 08/16/2018 03:12 PM, Simon Goldschmidt wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Marek Vasut <marex@denx.de <mailto:marex@denx.de>> schrieb am >>>>>>>>>>>>> Do., 16. >>>>>>>>>>>>> Aug. 2018, 15:06: >>>>>>>>>>>>> >>>>>>>>>>>>> On 08/16/2018 03:00 PM, Simon Goldschmidt wrote: >>>>>>>>>>>>> > On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut >>>>>>>>>>>>> <marex@denx.de >>>>>>>>>>>>> <mailto:marex@denx.de>> wrote: >>>>>>>>>>>>> >> >>>>>>>>>>>>> >> On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: >>>>>>>>>>>>> >>> gd->env_addr points to pre-relocation address even >>>>>>>>>>>>> after >>>>>>>>>>>>> >>> relocation. This leads to an abort in env_callback_init >>>>>>>>>>>>> >>> when loading the environment. >>>>>>>>>>>>> >>> >>>>>>>>>>>>> >>> Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. >>>>>>>>>>>>> >>> >>>>>>>>>>>>> >>> Signed-off-by: Simon Goldschmidt >>>>>>>>>>>>> <simon.k.r.goldschmidt@gmail.com >>>>>>>>>>>>> <mailto:simon.k.r.goldschmidt@gmail.com>> >>>>>>>>>>>>> >> >>>>>>>>>>>>> >> I have one last question -- does this somehow >>>>>>>>>>>>> influence SPL ? >>>>>>>>>>>>> > >>>>>>>>>>>>> > No, it doesn't. The code that gets enabled by this >>>>>>>>>>>>> define is in >>>>>>>>>>>>> > common/board_r.c, which is not linked for SPL. >>>>>>>>>>>>> >>>>>>>>>>>>> Ah, thanks for checking. >>>>>>>>>>>>> >>>>>>>>>>>>> btw do you think it'd make sense to just enable this by >>>>>>>>>>>>> default on all >>>>>>>>>>>>> systems and zap the EXTRA_ENV_RELOC macro altogether ? >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Yes, that's what I have thought about already. Just like the >>>>>>>>>>>>> for the >>>>>>>>>>>>> embedded device tree relocation, we could then probably use >>>>>>>>>>>>> gd->reloc_off instead of CONFIG_SYS_MONITOR_BASE. I'm just >>>>>>>>>>>>> not sure this >>>>>>>>>>>>> really works for all boards, but it would be worth a try to >>>>>>>>>>>>> push after >>>>>>>>>>>>> this release is out. >>>>>>>>>>>> I think so too. I cannot think of a reason why this shouldn't >>>>>>>>>>>> be enabled >>>>>>>>>>>> in fact. >>>>>>>>>>> Exactly. Too me it seems like a leftover, especially given the >>>>>>>>>>> use of >>>>>>>>>>> CONFIG_SYS_MONITOR_BASE, which seems a little outdated, too. >>>>>>>>>>> I've set up a reminder for a patch to remove it after the release. >>>>>>>>>> Feel free to send it now. >>>>>>>>> OK, I have tried, but it seems it's not that easy: some boards >>>>>>>>> override the initial gd->env_addr by setting CONFIG_ENV_ADDR. So if >>>>>>>>> this is outside of U-Boot's pre-relocation range, it clearly should >>>>>>>>> not be relocated. One might find an improved way to relocate >>>>>>>>> gd->env_addr if it is internal (e.g. checking the range to be in >>>>>>>>> pre-relocation?). But simply removing the EXTRA_ENV_RELOC does not >>>>>>>>> seem to work. >>>>>>>> Shouldn't most of those boards be easily fixable ? >>>>>>> Well, if we unconditionally alter gd->env_addr by gd->reloc_off, >>>>>>> boards that have their initial gd->env_addr outside of the initial >>>>>>> binary can be fixed only by changing their behaviour. I don't know how >>>>>>> widely used this feature is, but since it's a config option >>>>>>> (CONFIG_ENV_ADDR), how would we know? >>>>>> git grep ? But aren't you mixing CONFIG_ENV_ADDR and CONFIG_ENV_RELOC ? >>>>> Have a look at env_sf_init() in env/sf.c (called from env_init(), >>>>> which in turn is called from board_init_f()). There gd->env_addr is >>>>> initialized by a config setting. If this user-supplied address is >>>>> outside of the binary, relocating it is wrong, isn't it? >>>> I think you want to relocate the env close to where U-Boot is relocated >>>> in all cases, no ? >>> >>> Hmm, yes, *I do*, but reading env/sf.c it seems like there *are* ways to >>> have this initial environment located outside of the initial binary >>> hence making relocation invalidate it. Now I'm in no position to see if >>> this is an error or legal usage of the code as it was meant to be... >>> >>> So I think we have two options: >>> a) apply CONFIG_SYS_EXTRA_ENV_RELOC for socfpga or >> >> But SoCFPGA can have env in SF too, so you cannot apply this too all >> SoCFPGA, unless I am wrong. > > It's again more confusing. This is only a problem if CONFIG_ENV_ADDR > is defined, which isn't for socfpga. Could this be applicable to memory mapped CFI flashes only ? In that case, if CONFIG_ENV_ADDR is defined, undefine the ENV_RELOC and done ? >>> b) push a patch that always relocates the initial environment and see if >>> someone cares... >> >> Wouldn't it make sense to fix the sf and then enable env reloc for it too ? > > sf was only an example. AT least nand, flash and nvram env drivers > also can put gd->env_addr at a different, configurable address. > The only way to auto-relocate here would be to have a flag that tells > us what to do. Or we could check if gd->env_addr is in the range of > relocated code. > > But the whole code in that area just pretty much confuses me... See above, isn't that about memory-mapped and non-memory-mapped env storage?
On 18.09.2018 12:37, Marek Vasut wrote: > On 09/18/2018 12:29 PM, Simon Goldschmidt wrote: >> On Tue, Sep 18, 2018 at 12:12 PM Marek Vasut <marex@denx.de> wrote: >>> On 09/17/2018 10:44 PM, Simon Goldschmidt wrote: >>>> OK, so I got more than 2 weeks off of U-Boot, but here I'm back... ;-) >>>> >>>> On 18.08.2018 14:25, Marek Vasut wrote: >>>>> On 08/18/2018 10:55 AM, Simon Goldschmidt wrote: >>>>>> On Fri, Aug 17, 2018 at 12:20 PM Marek Vasut <marex@denx.de> wrote: >>>>>>> On 08/17/2018 08:56 AM, Simon Goldschmidt wrote: >>>>>>>> On Fri, Aug 17, 2018 at 1:57 AM Marek Vasut <marex@denx.de> wrote: >>>>>>>>> On 08/16/2018 09:44 PM, Simon Goldschmidt wrote: >>>>>>>>>> On Thu, Aug 16, 2018 at 4:04 PM Marek Vasut <marex@denx.de> wrote: >>>>>>>>>>> On 08/16/2018 03:50 PM, Simon Goldschmidt wrote: >>>>>>>>>>>> On Thu, Aug 16, 2018 at 3:15 PM Marek Vasut <marex@denx.de> wrote: >>>>>>>>>>>>> On 08/16/2018 03:12 PM, Simon Goldschmidt wrote: >>>>>>>>>>>>>> Marek Vasut <marex@denx.de <mailto:marex@denx.de>> schrieb am >>>>>>>>>>>>>> Do., 16. >>>>>>>>>>>>>> Aug. 2018, 15:06: >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 08/16/2018 03:00 PM, Simon Goldschmidt wrote: >>>>>>>>>>>>>> > On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut >>>>>>>>>>>>>> <marex@denx.de >>>>>>>>>>>>>> <mailto:marex@denx.de>> wrote: >>>>>>>>>>>>>> >> >>>>>>>>>>>>>> >> On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: >>>>>>>>>>>>>> >>> gd->env_addr points to pre-relocation address even >>>>>>>>>>>>>> after >>>>>>>>>>>>>> >>> relocation. This leads to an abort in env_callback_init >>>>>>>>>>>>>> >>> when loading the environment. >>>>>>>>>>>>>> >>> >>>>>>>>>>>>>> >>> Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. >>>>>>>>>>>>>> >>> >>>>>>>>>>>>>> >>> Signed-off-by: Simon Goldschmidt >>>>>>>>>>>>>> <simon.k.r.goldschmidt@gmail.com >>>>>>>>>>>>>> <mailto:simon.k.r.goldschmidt@gmail.com>> >>>>>>>>>>>>>> >> >>>>>>>>>>>>>> >> I have one last question -- does this somehow >>>>>>>>>>>>>> influence SPL ? >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > No, it doesn't. The code that gets enabled by this >>>>>>>>>>>>>> define is in >>>>>>>>>>>>>> > common/board_r.c, which is not linked for SPL. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Ah, thanks for checking. >>>>>>>>>>>>>> >>>>>>>>>>>>>> btw do you think it'd make sense to just enable this by >>>>>>>>>>>>>> default on all >>>>>>>>>>>>>> systems and zap the EXTRA_ENV_RELOC macro altogether ? >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> Yes, that's what I have thought about already. Just like the >>>>>>>>>>>>>> for the >>>>>>>>>>>>>> embedded device tree relocation, we could then probably use >>>>>>>>>>>>>> gd->reloc_off instead of CONFIG_SYS_MONITOR_BASE. I'm just >>>>>>>>>>>>>> not sure this >>>>>>>>>>>>>> really works for all boards, but it would be worth a try to >>>>>>>>>>>>>> push after >>>>>>>>>>>>>> this release is out. >>>>>>>>>>>>> I think so too. I cannot think of a reason why this shouldn't >>>>>>>>>>>>> be enabled >>>>>>>>>>>>> in fact. >>>>>>>>>>>> Exactly. Too me it seems like a leftover, especially given the >>>>>>>>>>>> use of >>>>>>>>>>>> CONFIG_SYS_MONITOR_BASE, which seems a little outdated, too. >>>>>>>>>>>> I've set up a reminder for a patch to remove it after the release. >>>>>>>>>>> Feel free to send it now. >>>>>>>>>> OK, I have tried, but it seems it's not that easy: some boards >>>>>>>>>> override the initial gd->env_addr by setting CONFIG_ENV_ADDR. So if >>>>>>>>>> this is outside of U-Boot's pre-relocation range, it clearly should >>>>>>>>>> not be relocated. One might find an improved way to relocate >>>>>>>>>> gd->env_addr if it is internal (e.g. checking the range to be in >>>>>>>>>> pre-relocation?). But simply removing the EXTRA_ENV_RELOC does not >>>>>>>>>> seem to work. >>>>>>>>> Shouldn't most of those boards be easily fixable ? >>>>>>>> Well, if we unconditionally alter gd->env_addr by gd->reloc_off, >>>>>>>> boards that have their initial gd->env_addr outside of the initial >>>>>>>> binary can be fixed only by changing their behaviour. I don't know how >>>>>>>> widely used this feature is, but since it's a config option >>>>>>>> (CONFIG_ENV_ADDR), how would we know? >>>>>>> git grep ? But aren't you mixing CONFIG_ENV_ADDR and CONFIG_ENV_RELOC ? >>>>>> Have a look at env_sf_init() in env/sf.c (called from env_init(), >>>>>> which in turn is called from board_init_f()). There gd->env_addr is >>>>>> initialized by a config setting. If this user-supplied address is >>>>>> outside of the binary, relocating it is wrong, isn't it? >>>>> I think you want to relocate the env close to where U-Boot is relocated >>>>> in all cases, no ? >>>> Hmm, yes, *I do*, but reading env/sf.c it seems like there *are* ways to >>>> have this initial environment located outside of the initial binary >>>> hence making relocation invalidate it. Now I'm in no position to see if >>>> this is an error or legal usage of the code as it was meant to be... >>>> >>>> So I think we have two options: >>>> a) apply CONFIG_SYS_EXTRA_ENV_RELOC for socfpga or >>> But SoCFPGA can have env in SF too, so you cannot apply this too all >>> SoCFPGA, unless I am wrong. >> It's again more confusing. This is only a problem if CONFIG_ENV_ADDR >> is defined, which isn't for socfpga. > Could this be applicable to memory mapped CFI flashes only ? > In that case, if CONFIG_ENV_ADDR is defined, undefine the ENV_RELOC and > done ? > >>>> b) push a patch that always relocates the initial environment and see if >>>> someone cares... >>> Wouldn't it make sense to fix the sf and then enable env reloc for it too ? >> sf was only an example. AT least nand, flash and nvram env drivers >> also can put gd->env_addr at a different, configurable address. >> The only way to auto-relocate here would be to have a flag that tells >> us what to do. Or we could check if gd->env_addr is in the range of >> relocated code. >> >> But the whole code in that area just pretty much confuses me... > See above, isn't that about memory-mapped and non-memory-mapped env storage? > Probably yes. So rather than making CONFIG_SYS_EXTRA_ENV_RELOC configurable, we could derive it (or a similar option) by depending on CONFIG_ENV_ADDR not being defined? That might work. But I'm yet not sure if CONFIG_ENV_ADDR is the only option triggering memory-mapped env storage. And in the end, maybe not all memory-mapped storages are affected. This only affects memory-mapped storage that is avaiable before relocation (so without drivers, probably). If it's really worth trying to fix this in a generic way while risking to break other configs I cannot test, then I can prepare a patch. Simon
On 09/18/2018 09:55 PM, Simon Goldschmidt wrote: > On 18.09.2018 12:37, Marek Vasut wrote: >> On 09/18/2018 12:29 PM, Simon Goldschmidt wrote: >>> On Tue, Sep 18, 2018 at 12:12 PM Marek Vasut <marex@denx.de> wrote: >>>> On 09/17/2018 10:44 PM, Simon Goldschmidt wrote: >>>>> OK, so I got more than 2 weeks off of U-Boot, but here I'm back... ;-) >>>>> >>>>> On 18.08.2018 14:25, Marek Vasut wrote: >>>>>> On 08/18/2018 10:55 AM, Simon Goldschmidt wrote: >>>>>>> On Fri, Aug 17, 2018 at 12:20 PM Marek Vasut <marex@denx.de> wrote: >>>>>>>> On 08/17/2018 08:56 AM, Simon Goldschmidt wrote: >>>>>>>>> On Fri, Aug 17, 2018 at 1:57 AM Marek Vasut <marex@denx.de> wrote: >>>>>>>>>> On 08/16/2018 09:44 PM, Simon Goldschmidt wrote: >>>>>>>>>>> On Thu, Aug 16, 2018 at 4:04 PM Marek Vasut <marex@denx.de> >>>>>>>>>>> wrote: >>>>>>>>>>>> On 08/16/2018 03:50 PM, Simon Goldschmidt wrote: >>>>>>>>>>>>> On Thu, Aug 16, 2018 at 3:15 PM Marek Vasut <marex@denx.de> >>>>>>>>>>>>> wrote: >>>>>>>>>>>>>> On 08/16/2018 03:12 PM, Simon Goldschmidt wrote: >>>>>>>>>>>>>>> Marek Vasut <marex@denx.de <mailto:marex@denx.de>> >>>>>>>>>>>>>>> schrieb am >>>>>>>>>>>>>>> Do., 16. >>>>>>>>>>>>>>> Aug. 2018, 15:06: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On 08/16/2018 03:00 PM, Simon Goldschmidt wrote: >>>>>>>>>>>>>>> > On Thu, Aug 16, 2018 at 1:18 PM Marek Vasut >>>>>>>>>>>>>>> <marex@denx.de >>>>>>>>>>>>>>> <mailto:marex@denx.de>> wrote: >>>>>>>>>>>>>>> >> >>>>>>>>>>>>>>> >> On 08/16/2018 09:38 AM, Simon Goldschmidt wrote: >>>>>>>>>>>>>>> >>> gd->env_addr points to pre-relocation address even >>>>>>>>>>>>>>> after >>>>>>>>>>>>>>> >>> relocation. This leads to an abort in >>>>>>>>>>>>>>> env_callback_init >>>>>>>>>>>>>>> >>> when loading the environment. >>>>>>>>>>>>>>> >>> >>>>>>>>>>>>>>> >>> Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. >>>>>>>>>>>>>>> >>> >>>>>>>>>>>>>>> >>> Signed-off-by: Simon Goldschmidt >>>>>>>>>>>>>>> <simon.k.r.goldschmidt@gmail.com >>>>>>>>>>>>>>> <mailto:simon.k.r.goldschmidt@gmail.com>> >>>>>>>>>>>>>>> >> >>>>>>>>>>>>>>> >> I have one last question -- does this somehow >>>>>>>>>>>>>>> influence SPL ? >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > No, it doesn't. The code that gets enabled by this >>>>>>>>>>>>>>> define is in >>>>>>>>>>>>>>> > common/board_r.c, which is not linked for SPL. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Ah, thanks for checking. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> btw do you think it'd make sense to just enable >>>>>>>>>>>>>>> this by >>>>>>>>>>>>>>> default on all >>>>>>>>>>>>>>> systems and zap the EXTRA_ENV_RELOC macro altogether ? >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Yes, that's what I have thought about already. Just like the >>>>>>>>>>>>>>> for the >>>>>>>>>>>>>>> embedded device tree relocation, we could then probably use >>>>>>>>>>>>>>> gd->reloc_off instead of CONFIG_SYS_MONITOR_BASE. I'm just >>>>>>>>>>>>>>> not sure this >>>>>>>>>>>>>>> really works for all boards, but it would be worth a try to >>>>>>>>>>>>>>> push after >>>>>>>>>>>>>>> this release is out. >>>>>>>>>>>>>> I think so too. I cannot think of a reason why this shouldn't >>>>>>>>>>>>>> be enabled >>>>>>>>>>>>>> in fact. >>>>>>>>>>>>> Exactly. Too me it seems like a leftover, especially given the >>>>>>>>>>>>> use of >>>>>>>>>>>>> CONFIG_SYS_MONITOR_BASE, which seems a little outdated, too. >>>>>>>>>>>>> I've set up a reminder for a patch to remove it after the >>>>>>>>>>>>> release. >>>>>>>>>>>> Feel free to send it now. >>>>>>>>>>> OK, I have tried, but it seems it's not that easy: some boards >>>>>>>>>>> override the initial gd->env_addr by setting CONFIG_ENV_ADDR. >>>>>>>>>>> So if >>>>>>>>>>> this is outside of U-Boot's pre-relocation range, it clearly >>>>>>>>>>> should >>>>>>>>>>> not be relocated. One might find an improved way to relocate >>>>>>>>>>> gd->env_addr if it is internal (e.g. checking the range to be in >>>>>>>>>>> pre-relocation?). But simply removing the EXTRA_ENV_RELOC >>>>>>>>>>> does not >>>>>>>>>>> seem to work. >>>>>>>>>> Shouldn't most of those boards be easily fixable ? >>>>>>>>> Well, if we unconditionally alter gd->env_addr by gd->reloc_off, >>>>>>>>> boards that have their initial gd->env_addr outside of the initial >>>>>>>>> binary can be fixed only by changing their behaviour. I don't >>>>>>>>> know how >>>>>>>>> widely used this feature is, but since it's a config option >>>>>>>>> (CONFIG_ENV_ADDR), how would we know? >>>>>>>> git grep ? But aren't you mixing CONFIG_ENV_ADDR and >>>>>>>> CONFIG_ENV_RELOC ? >>>>>>> Have a look at env_sf_init() in env/sf.c (called from env_init(), >>>>>>> which in turn is called from board_init_f()). There gd->env_addr is >>>>>>> initialized by a config setting. If this user-supplied address is >>>>>>> outside of the binary, relocating it is wrong, isn't it? >>>>>> I think you want to relocate the env close to where U-Boot is >>>>>> relocated >>>>>> in all cases, no ? >>>>> Hmm, yes, *I do*, but reading env/sf.c it seems like there *are* >>>>> ways to >>>>> have this initial environment located outside of the initial binary >>>>> hence making relocation invalidate it. Now I'm in no position to >>>>> see if >>>>> this is an error or legal usage of the code as it was meant to be... >>>>> >>>>> So I think we have two options: >>>>> a) apply CONFIG_SYS_EXTRA_ENV_RELOC for socfpga or >>>> But SoCFPGA can have env in SF too, so you cannot apply this too all >>>> SoCFPGA, unless I am wrong. >>> It's again more confusing. This is only a problem if CONFIG_ENV_ADDR >>> is defined, which isn't for socfpga. >> Could this be applicable to memory mapped CFI flashes only ? >> In that case, if CONFIG_ENV_ADDR is defined, undefine the ENV_RELOC and >> done ? >> >>>>> b) push a patch that always relocates the initial environment and >>>>> see if >>>>> someone cares... >>>> Wouldn't it make sense to fix the sf and then enable env reloc for >>>> it too ? >>> sf was only an example. AT least nand, flash and nvram env drivers >>> also can put gd->env_addr at a different, configurable address. >>> The only way to auto-relocate here would be to have a flag that tells >>> us what to do. Or we could check if gd->env_addr is in the range of >>> relocated code. >>> >>> But the whole code in that area just pretty much confuses me... >> See above, isn't that about memory-mapped and non-memory-mapped env >> storage? >> > Probably yes. So rather than making CONFIG_SYS_EXTRA_ENV_RELOC > configurable, we could derive it (or a similar option) by depending on > CONFIG_ENV_ADDR not being defined? > > That might work. But I'm yet not sure if CONFIG_ENV_ADDR is the only > option triggering memory-mapped env storage. > > And in the end, maybe not all memory-mapped storages are affected. This > only affects memory-mapped storage that is avaiable before relocation > (so without drivers, probably). > > If it's really worth trying to fix this in a generic way while risking > to break other configs I cannot test, then I can prepare a patch. Let's try it, I think it makes sense :)
diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h index 8ebf6b85fe..8b9f0427c0 100644 --- a/include/configs/socfpga_common.h +++ b/include/configs/socfpga_common.h @@ -284,6 +284,18 @@ unsigned int cm_get_qspi_controller_clk_hz(void); #define CONFIG_SPL_STACK CONFIG_SYS_SPL_MALLOC_START #endif +/* + * When U-Boot is started from FPGA, prevent gd->env_addr to point into + * FPGA OnChip RAM after relocation + */ +#define CONFIG_SYS_EXTRA_ENV_RELOC +/* + * CONFIG_SYS_EXTRA_ENV_RELOC code needs this to calculate the relocation + * offset for gd->env_addr. Since this is based on gd->relocaddr, we need + * to use CONFIG_SYS_TEXT_BASE here. + */ +#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_TEXT_BASE + /* Extra Environment */ #ifndef CONFIG_SPL_BUILD
gd->env_addr points to pre-relocation address even after relocation. This leads to an abort in env_callback_init when loading the environment. Fix this by enabling CONFIG_SYS_EXTRA_ENV_RELOC. Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> --- Changes in v5: Improve comments Changes in v4: enable this fix for all socfpga, not for gen5 only Changes in v3: this patch is new in v3 Changes in v2: None include/configs/socfpga_common.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)