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 <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. >> 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 :) -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
