On 09/18/2018 12:29 PM, Simon Goldschmidt wrote: > On Tue, Sep 18, 2018 at 12:12 PM Marek Vasut <[email protected]> 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 <[email protected]> wrote: >>>>>> On 08/17/2018 08:56 AM, Simon Goldschmidt wrote: >>>>>>> On Fri, Aug 17, 2018 at 1:57 AM Marek Vasut <[email protected]> wrote: >>>>>>>> On 08/16/2018 09:44 PM, Simon Goldschmidt wrote: >>>>>>>>> On Thu, Aug 16, 2018 at 4:04 PM Marek Vasut <[email protected]> wrote: >>>>>>>>>> On 08/16/2018 03:50 PM, Simon Goldschmidt wrote: >>>>>>>>>>> On Thu, Aug 16, 2018 at 3:15 PM Marek Vasut <[email protected]> wrote: >>>>>>>>>>>> On 08/16/2018 03:12 PM, Simon Goldschmidt wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Marek Vasut <[email protected] <mailto:[email protected]>> 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 >>>>>>>>>>>>> <[email protected] >>>>>>>>>>>>> <mailto:[email protected]>> 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 >>>>>>>>>>>>> <[email protected] >>>>>>>>>>>>> <mailto:[email protected]>> >>>>>>>>>>>>> >> >>>>>>>>>>>>> >> 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? -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
