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

Tue Sep 18 19:55:02 UTC 2018

On 18.09.2018 12:37, Marek Vasut wrote:
> On 09/18/2018 12:29 PM, Simon Goldschmidt wrote:
>> On Tue, Sep 18, 2018 at 12:12 PM Marek Vasut <marex at 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 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.
>> 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?
>
Probably yes. So rather than making CONFIG_SYS_EXTRA_ENV_RELOC 
configurable, we could derive it (or a similar option) by depending on 
CONFIG_ENV_ADDR not being defined?

That might work. But I'm yet not sure if CONFIG_ENV_ADDR is the only 
option triggering memory-mapped env storage.

And in the end, maybe not all memory-mapped storages are affected. This 
only affects memory-mapped storage that is avaiable before relocation 
(so without drivers, probably).

If it's really worth trying to fix this in a generic way while risking 
to break other configs I cannot test, then I can prepare a patch.

Simon