On Tue, Sep 18, 2018 at 12:12 PM Marek Vasut <ma...@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 <ma...@denx.de> wrote: > >>>> On 08/17/2018 08:56 AM, Simon Goldschmidt wrote: > >>>>> On Fri, Aug 17, 2018 at 1:57 AM Marek Vasut <ma...@denx.de> wrote: > >>>>>> On 08/16/2018 09:44 PM, Simon Goldschmidt wrote: > >>>>>>> On Thu, Aug 16, 2018 at 4:04 PM Marek Vasut <ma...@denx.de> wrote: > >>>>>>>> On 08/16/2018 03:50 PM, Simon Goldschmidt wrote: > >>>>>>>>> On Thu, Aug 16, 2018 at 3:15 PM Marek Vasut <ma...@denx.de> wrote: > >>>>>>>>>> On 08/16/2018 03:12 PM, Simon Goldschmidt wrote: > >>>>>>>>>>> > >>>>>>>>>>> Marek Vasut <ma...@denx.de <mailto:ma...@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 > >>>>>>>>>>> <ma...@denx.de > >>>>>>>>>>> <mailto:ma...@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.goldschm...@gmail.com > >>>>>>>>>>> <mailto:simon.k.r.goldschm...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
