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

Wed Sep 19 08:54:33 UTC 2018

On 09/18/2018 09:55 PM, Simon Goldschmidt wrote:
> 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.

Let's try it, I think it makes sense :)

-- 
Best regards,
Marek Vasut