All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH v3] armv8: spl: Call spl_relocate_stack_gd for ARMv8
@ 2017-02-22 18:01 Philipp Tomsich
  2017-02-23  2:23 ` Simon Glass
  2017-02-28  2:13 ` André Przywara
  0 siblings, 2 replies; 6+ messages in thread
From: Philipp Tomsich @ 2017-02-22 18:01 UTC (permalink / raw)
  To: u-boot

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
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v3] armv8: spl: Call spl_relocate_stack_gd for ARMv8
  2017-02-22 18:01 [U-Boot] [PATCH v3] armv8: spl: Call spl_relocate_stack_gd for ARMv8 Philipp Tomsich
@ 2017-02-23  2:23 ` Simon Glass
  2017-02-23  9:25   ` Dr. Philipp Tomsich
  2017-02-28  2:13 ` André Przywara
  1 sibling, 1 reply; 6+ messages in thread
From: Simon Glass @ 2017-02-23  2:23 UTC (permalink / raw)
  To: u-boot

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
>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v3] armv8: spl: Call spl_relocate_stack_gd for ARMv8
  2017-02-23  2:23 ` Simon Glass
@ 2017-02-23  9:25   ` Dr. Philipp Tomsich
  2017-02-23 16:35     ` Simon Glass
  0 siblings, 1 reply; 6+ messages in thread
From: Dr. Philipp Tomsich @ 2017-02-23  9:25 UTC (permalink / raw)
  To: u-boot

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 at 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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v3] armv8: spl: Call spl_relocate_stack_gd for ARMv8
  2017-02-23  9:25   ` Dr. Philipp Tomsich
@ 2017-02-23 16:35     ` Simon Glass
  0 siblings, 0 replies; 6+ messages in thread
From: Simon Glass @ 2017-02-23 16:35 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v3] armv8: spl: Call spl_relocate_stack_gd for ARMv8
  2017-02-22 18:01 [U-Boot] [PATCH v3] armv8: spl: Call spl_relocate_stack_gd for ARMv8 Philipp Tomsich
  2017-02-23  2:23 ` Simon Glass
@ 2017-02-28  2:13 ` André Przywara
  2017-02-28  2:28   ` Tom Rini
  1 sibling, 1 reply; 6+ messages in thread
From: André Przywara @ 2017-02-28  2:13 UTC (permalink / raw)
  To: u-boot

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
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v3] armv8: spl: Call spl_relocate_stack_gd for ARMv8
  2017-02-28  2:13 ` André Przywara
@ 2017-02-28  2:28   ` Tom Rini
  0 siblings, 0 replies; 6+ messages in thread
From: Tom Rini @ 2017-02-28  2:28 UTC (permalink / raw)
  To: u-boot

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
> > 
> 

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170227/4124aa41/attachment.sig>

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-28  2:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-22 18:01 [U-Boot] [PATCH v3] armv8: spl: Call spl_relocate_stack_gd for ARMv8 Philipp Tomsich
2017-02-23  2:23 ` Simon Glass
2017-02-23  9:25   ` Dr. Philipp Tomsich
2017-02-23 16:35     ` Simon Glass
2017-02-28  2:13 ` André Przywara
2017-02-28  2:28   ` Tom Rini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.