[U-Boot] [PATCH v4 2/5] arm: socfpga: fix U-Boot running from fpga OnChip RAM

[Marek Vasut]

On 08/14/2018 10:19 PM, Simon Goldschmidt wrote:
> On Tue, Aug 14, 2018 at 10:37 AM Marek Vasut <marex at denx.de> wrote:
>>
>> On 08/14/2018 08:09 AM, Simon Goldschmidt wrote:
>>>
>>>
>>> Marek Vasut <marex at denx.de <mailto:marex at denx.de>> schrieb am Mo., 13.
>>> Aug. 2018, 22:36:
>>>
>>>     On 08/13/2018 09:34 PM, 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>>
>>>     > ---
>>>     >
>>>     > Changes in v4: enable this fix for all socfpga, not for gen5 only
>>>     > Changes in v3: this patch is new in v3
>>>     > Changes in v2: None
>>>     >
>>>     >  include/configs/socfpga_common.h | 6 ++++++
>>>     >  1 file changed, 6 insertions(+)
>>>     >
>>>     > diff --git a/include/configs/socfpga_common.h
>>>     b/include/configs/socfpga_common.h
>>>     > index 8ebf6b85fe..d1148b838b 100644
>>>     > --- a/include/configs/socfpga_common.h
>>>     > +++ b/include/configs/socfpga_common.h
>>>     > @@ -284,6 +284,12 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
>>>     >  #define CONFIG_SPL_STACK             CONFIG_SYS_SPL_MALLOC_START
>>>     >  #endif
>>>     >
>>>     > +/* When U-Boot is started from FPGA, prevent gd->env_addr to
>>>     point into
>>>
>>>     Multi-line comment should have this format
>>>     /*
>>>      * foo
>>>      * bar
>>>      */
>>>
>>>
>>> Right, of course. I wonder why patman didn't warm me about that...
>>>
>>>
>>>     > + * FPGA OnChip RAM after relocation
>>>     > + */
>>>     > +#define CONFIG_SYS_EXTRA_ENV_RELOC
>>>     > +#define CONFIG_SYS_MONITOR_BASE      CONFIG_SYS_TEXT_BASE    /*
>>>     start of monitor */
>>>
>>>     What you don't explain in the commit message is this last line. Why is
>>>     this needed ?
>>>
>>>
>>> The code enabled by CONFIG_SYS_EXTRA_ENV_RELOC used this to calculate
>>> the relocation offset. I do think that's a bit strange, but I wouldn't
>>> change it with this patchset, or should I?
>>
>> You should document _why_ this is needed. Not "because the code enabled
>> by foo needed this", but why that code enabled this and why setting it
>> to SYS_TEXT_BASE is correct.
> 
> Yes, I wouldn't have sent a patch like that. I rather wanted to phrase
> that I don't know why this is needed for env relocation, as fdt
> relocation just uses gd->reloc_off. That might work for env
> relocation, too, but changing that seems out of scope for this
> patchset.

Maybe the comment in board_r.c explains why?

143 #ifdef CONFIG_SYS_EXTRA_ENV_RELOC
144         /*
145          * Some systems need to relocate the env_addr pointer early
because the
146          * location it points to will get invalidated before
env_relocate is
147          * called.  One example is on systems that might use a L2 or
L3 cache
148          * in SRAM mode and initialize that cache from SRAM mode
back to being
149          * a cache in cpu_init_r.
150          */
151         gd->env_addr += gd->relocaddr - CONFIG_SYS_MONITOR_BASE;
152 #endif

But then the env shouldn't point to pre-reloc address after relocation
according to the comment ?

-- 
Best regards,
Marek Vasut