| Message ID | 1487786517-43422-1-git-send-email-philipp.tomsich@theobroma-systems.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On 22 February 2017 at 11:01, Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote: > As part of the startup process for boards using the SPL, we need to > call spl_relocate_stack_gd. This is needed to set up malloc with its > DRAM buffer. > > Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > Reviewed-by: Simon Glass <sjg@chromium.org> > --- > arch/arm/lib/crt0_64.S | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/lib/crt0_64.S b/arch/arm/lib/crt0_64.S > index 19c6a98..a7cead5 100644 > --- a/arch/arm/lib/crt0_64.S > +++ b/arch/arm/lib/crt0_64.S > @@ -109,8 +109,17 @@ relocation_return: > */ > bl c_runtime_cpu_setup /* still call old routine */ > #endif /* !CONFIG_SPL_BUILD */ > - > -/* TODO: For SPL, call spl_relocate_stack_gd() to alloc stack relocation */ > +#if defined(CONFIG_SPL_BUILD) > + bl spl_relocate_stack_gd /* may return NULL */ > + /* Perform 'sp = (x0 != NULL) ? x0 : sp' while working > + * around the constraint that conditional moves can not > + * have 'sp' as an operand > + */ nit: Comment style again > + mov x1, sp > + cmp x0, #0 > + csel x0, x0, x1, ne > + mov sp, x0 > +#endif > > /* > * Clear BSS section > -- > 1.9.1 >
Simon, > On 23 Feb 2017, at 03:23, Simon Glass <sjg@chromium.org> wrote: > > On 22 February 2017 at 11:01, Philipp Tomsich > <philipp.tomsich@theobroma-systems.com <mailto:philipp.tomsich@theobroma-systems.com>> wrote: >> As part of the startup process for boards using the SPL, we need to >> call spl_relocate_stack_gd. This is needed to set up malloc with its >> DRAM buffer. >> >> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> >> Reviewed-by: Andre Przywara <andre.przywara@arm.com> >> Reviewed-by: Simon Glass <sjg@chromium.org> >> --- >> arch/arm/lib/crt0_64.S | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/lib/crt0_64.S b/arch/arm/lib/crt0_64.S >> index 19c6a98..a7cead5 100644 >> --- a/arch/arm/lib/crt0_64.S >> +++ b/arch/arm/lib/crt0_64.S >> @@ -109,8 +109,17 @@ relocation_return: >> */ >> bl c_runtime_cpu_setup /* still call old routine */ >> #endif /* !CONFIG_SPL_BUILD */ >> - >> -/* TODO: For SPL, call spl_relocate_stack_gd() to alloc stack relocation */ >> +#if defined(CONFIG_SPL_BUILD) >> + bl spl_relocate_stack_gd /* may return NULL */ >> + /* Perform 'sp = (x0 != NULL) ? x0 : sp' while working >> + * around the constraint that conditional moves can not >> + * have 'sp' as an operand >> + */ > > nit: Comment style again I thought is was the missing asterisks at the beginning of the line… What am I missing? Is it the indentation of the comment block? >> + mov x1, sp >> + cmp x0, #0 >> + csel x0, x0, x1, ne >> + mov sp, x0 >> +#endif >> >> /* >> * Clear BSS section >> -- >> 1.9.1
Hi, On 23 February 2017 at 02:25, Dr. Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote: > Simon, > > On 23 Feb 2017, at 03:23, Simon Glass <sjg@chromium.org> wrote: > > On 22 February 2017 at 11:01, Philipp Tomsich > <philipp.tomsich@theobroma-systems.com> wrote: > > As part of the startup process for boards using the SPL, we need to > call spl_relocate_stack_gd. This is needed to set up malloc with its > DRAM buffer. > > Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > Reviewed-by: Simon Glass <sjg@chromium.org> > --- > arch/arm/lib/crt0_64.S | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/lib/crt0_64.S b/arch/arm/lib/crt0_64.S > index 19c6a98..a7cead5 100644 > --- a/arch/arm/lib/crt0_64.S > +++ b/arch/arm/lib/crt0_64.S > @@ -109,8 +109,17 @@ relocation_return: > */ > bl c_runtime_cpu_setup /* still call old routine */ > #endif /* !CONFIG_SPL_BUILD */ > - > -/* TODO: For SPL, call spl_relocate_stack_gd() to alloc stack relocation */ > +#if defined(CONFIG_SPL_BUILD) > + bl spl_relocate_stack_gd /* may return NULL */ > + /* Perform 'sp = (x0 != NULL) ? x0 : sp' while working > + * around the constraint that conditional moves can not > + * have 'sp' as an operand > + */ > > > nit: Comment style again > > > I thought is was the missing asterisks at the beginning of the line… > What am I missing? Is it the indentation of the comment block? It's just that you should now have any text on the /* line or the */ line, so: /* * Perform 'sp = (x0 != NULL) ? x0 : sp' while working * around the constraint that conditional moves can not * have 'sp' as an operand */ That's the style of the U-Boot code base. > > + mov x1, sp > + cmp x0, #0 > + csel x0, x0, x1, ne > + mov sp, x0 > +#endif > > /* > * Clear BSS section > -- > 1.9.1 > > Regards, Simon
Hi Tom, Simon,
can we merge this patch for 2017.03 still?
This fixes an SD card boot regression for arm64 SPLs for me, introduced
shortly before -rc1.
Commit b3d2861eb20a ("spl: Remove overwrite of relocated malloc limit"),
introduced just a few commits before -rc1, broke MMC boot for arm64
(because malloc fails): The patch moves the simple_malloc setup into
spl_relocate_stack_gd(), which so far wasn't actually called on arm64
(see the patch below). So the malloc buffer was 0 bytes, malloc failed,
the MMC driver couldn't find a boot device and gave up:
U-Boot SPL 2017.03-rc2-00040-gb7b8021 (Feb 28 2017 - 00:41:35)
DRAM: 1024 MiB
Trying to boot from MMC1
MMC Device 0 not found
spl: could not find mmc device. error: -19
SPL: failed to boot from all boot devices
### ERROR ### Please RESET the board ###
This is the current situation since -rc1 for the Pine64, for instance.
Now one solution to fix this is to actually revert b3d2861eb20a, which
would move back to the old behaviour of using the SRAM malloc buffer for
the whole of the SPL, which is fine for the MMC driver.
But since Andrew's patch is correct, I'd rather merge Philipp's patch
now, which solves a TODO and just calls spl_relocate_stack_gd(), so that
we now get access to the proper (and much bigger) DRAM malloc buffer.
Does that make sense?
Can we merge Philipp's patch here (fixing the commenting style issue on
the fly) to fix the regression?
Cheers,
Andre.
> As part of the startup process for boards using the SPL, we need to
> call spl_relocate_stack_gd. This is needed to set up malloc with its
> DRAM buffer.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
> arch/arm/lib/crt0_64.S | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/lib/crt0_64.S b/arch/arm/lib/crt0_64.S
> index 19c6a98..a7cead5 100644
> --- a/arch/arm/lib/crt0_64.S
> +++ b/arch/arm/lib/crt0_64.S
> @@ -109,8 +109,17 @@ relocation_return:
> */
> bl c_runtime_cpu_setup /* still call old routine */
> #endif /* !CONFIG_SPL_BUILD */
> -
> -/* TODO: For SPL, call spl_relocate_stack_gd() to alloc stack relocation */
> +#if defined(CONFIG_SPL_BUILD)
> + bl spl_relocate_stack_gd /* may return NULL */
> + /* Perform 'sp = (x0 != NULL) ? x0 : sp' while working
> + * around the constraint that conditional moves can not
> + * have 'sp' as an operand
> + */
> + mov x1, sp
> + cmp x0, #0
> + csel x0, x0, x1, ne
> + mov sp, x0
> +#endif
>
> /*
> * Clear BSS section
>
On Tue, Feb 28, 2017 at 02:13:21AM +0000, André Przywara wrote: > Hi Tom, Simon, > > can we merge this patch for 2017.03 still? > This fixes an SD card boot regression for arm64 SPLs for me, introduced > shortly before -rc1. This seems like a reasonable thing to bring in, yes. Thanks for the reminder. > > Commit b3d2861eb20a ("spl: Remove overwrite of relocated malloc limit"), > introduced just a few commits before -rc1, broke MMC boot for arm64 > (because malloc fails): The patch moves the simple_malloc setup into > spl_relocate_stack_gd(), which so far wasn't actually called on arm64 > (see the patch below). So the malloc buffer was 0 bytes, malloc failed, > the MMC driver couldn't find a boot device and gave up: > U-Boot SPL 2017.03-rc2-00040-gb7b8021 (Feb 28 2017 - 00:41:35) > DRAM: 1024 MiB > Trying to boot from MMC1 > MMC Device 0 not found > spl: could not find mmc device. error: -19 > SPL: failed to boot from all boot devices > ### ERROR ### Please RESET the board ### > This is the current situation since -rc1 for the Pine64, for instance. > > Now one solution to fix this is to actually revert b3d2861eb20a, which > would move back to the old behaviour of using the SRAM malloc buffer for > the whole of the SPL, which is fine for the MMC driver. > > But since Andrew's patch is correct, I'd rather merge Philipp's patch > now, which solves a TODO and just calls spl_relocate_stack_gd(), so that > we now get access to the proper (and much bigger) DRAM malloc buffer. > > Does that make sense? > Can we merge Philipp's patch here (fixing the commenting style issue on > the fly) to fix the regression? > > Cheers, > Andre. > > > As part of the startup process for boards using the SPL, we need to > > call spl_relocate_stack_gd. This is needed to set up malloc with its > > DRAM buffer. > > > > Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > > Reviewed-by: Simon Glass <sjg@chromium.org> > > --- > > arch/arm/lib/crt0_64.S | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm/lib/crt0_64.S b/arch/arm/lib/crt0_64.S > > index 19c6a98..a7cead5 100644 > > --- a/arch/arm/lib/crt0_64.S > > +++ b/arch/arm/lib/crt0_64.S > > @@ -109,8 +109,17 @@ relocation_return: > > */ > > bl c_runtime_cpu_setup /* still call old routine */ > > #endif /* !CONFIG_SPL_BUILD */ > > - > > -/* TODO: For SPL, call spl_relocate_stack_gd() to alloc stack relocation */ > > +#if defined(CONFIG_SPL_BUILD) > > + bl spl_relocate_stack_gd /* may return NULL */ > > + /* Perform 'sp = (x0 != NULL) ? x0 : sp' while working > > + * around the constraint that conditional moves can not > > + * have 'sp' as an operand > > + */ > > + mov x1, sp > > + cmp x0, #0 > > + csel x0, x0, x1, ne > > + mov sp, x0 > > +#endif > > > > /* > > * Clear BSS section > > >
diff --git a/arch/arm/lib/crt0_64.S b/arch/arm/lib/crt0_64.S index 19c6a98..a7cead5 100644 --- a/arch/arm/lib/crt0_64.S +++ b/arch/arm/lib/crt0_64.S @@ -109,8 +109,17 @@ relocation_return: */ bl c_runtime_cpu_setup /* still call old routine */ #endif /* !CONFIG_SPL_BUILD */ - -/* TODO: For SPL, call spl_relocate_stack_gd() to alloc stack relocation */ +#if defined(CONFIG_SPL_BUILD) + bl spl_relocate_stack_gd /* may return NULL */ + /* Perform 'sp = (x0 != NULL) ? x0 : sp' while working + * around the constraint that conditional moves can not + * have 'sp' as an operand + */ + mov x1, sp + cmp x0, #0 + csel x0, x0, x1, ne + mov sp, x0 +#endif /* * Clear BSS section