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

[Simon Goldschmidt]

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 at 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 at 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 at 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 at denx.de> wrote:
>>>>>>>>> On 08/16/2018 03:12 PM, Simon Goldschmidt wrote:
>>>>>>>>>>
>>>>>>>>>> Marek Vasut <marex at denx.de <mailto:marex at 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 at denx.de
>>>>>>>>>>      <mailto:marex at 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 at gmail.com
>>>>>>>>>>      <mailto:simon.k.r.goldschmidt at 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