[U-Boot] [PATCH v5] arm: socfpga: fix U-Boot running from fpga OnChip RAM

[Marek Vasut <marex@denx.de>]

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 at 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?

-- 
Best regards,
Marek Vasut