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

[Marek Vasut]

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.

-- 
Best regards,
Marek Vasut