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

[Marek Vasut]

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

But SoCFPGA can have env in SF too, so you cannot apply this too all
SoCFPGA, unless I am wrong.

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

> Which of the two do you prefer?
> 
> Simon


-- 
Best regards,
Marek Vasut