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

[Marek Vasut]

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 ?

> Anyway, I'm off for 2 weeks now (holiday time here) with some email
> access at most, so I'll continue on this when I get back.

Have a nice vacation.

> Simon
> 
>>
>>> So to me that means we still have to make this overridable and could
>>> change the "default" state of such an option only. Meaning that the
>>> default is "relocate gd->env_addr" with an option to leave it. But is
>>> this really worth breaking existing boards?
>>
>> I think you do want to relocate the env alongside U-Boot, always, no ?
>>
>> --
>> Best regards,
>> Marek Vasut


-- 
Best regards,
Marek Vasut