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

[Marek Vasut]

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 ?

> 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