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

[Simon Goldschmidt]

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?

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.

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