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

On 08/14/2018 08:09 AM, Simon Goldschmidt wrote:

Marek Vasut <marex@denx.de mailto:marex@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@gmail.com
<mailto:simon.k.r.goldschmidt@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.