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

[Simon Goldschmidt]

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.

>
> > 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...


Simon