On 08/14/2018 10:19 PM, Simon Goldschmidt wrote: > On Tue, Aug 14, 2018 at 10:37 AM Marek Vasut <[email protected]> wrote: >> >> On 08/14/2018 08:09 AM, Simon Goldschmidt wrote: >>> >>> >>> Marek Vasut <[email protected] <mailto:[email protected]>> 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 <[email protected] >>> <mailto:[email protected]>> >>> > --- >>> > >>> > 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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
