| Message ID | 20180813193437.9539-3-simon.k.r.goldschmidt@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | Get socfpga gen5 SPL working again. | expand |
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> > --- > > 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 */ > + * 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 ? > /* Extra Environment */ > #ifndef CONFIG_SPL_BUILD > >
Marek Vasut <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> > > --- > > > > 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? Simon > > /* Extra Environment */ > > #ifndef CONFIG_SPL_BUILD > > > > > > > -- > Best regards, > Marek Vasut >
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.
On Tue, Aug 14, 2018 at 10:37 AM Marek Vasut <marex@denx.de> wrote: > > 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. 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. Simon
On 08/14/2018 10:19 PM, Simon Goldschmidt wrote: > On Tue, Aug 14, 2018 at 10:37 AM Marek Vasut <marex@denx.de> wrote: >> >> 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. > > 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 ?
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 + * FPGA OnChip RAM after relocation + */ +#define CONFIG_SYS_EXTRA_ENV_RELOC +#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_TEXT_BASE /* start of monitor */ + /* Extra Environment */ #ifndef CONFIG_SPL_BUILD
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> --- 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(+)