diff mbox series

[U-Boot,3/4] ARM: renesas: Save boot parameters passed in by ATF

Message ID 20190305032557.19788-3-marek.vasut+renesas@gmail.com
State Accepted
Commit 4fa8375ffec5bd1c7edabde42b8ee0339072df38
Delegated to: Marek Vasut
Headers show
Series [U-Boot,1/4] lib: fdt: Split fdtdec_setup_mem_size_base() | expand

Commit Message

Marek Vasut March 5, 2019, 3:25 a.m. UTC
The ATF can pass additional information via the first four registers,
x0...x3. The R-Car Gen3 with mainline ATF, register x1 contains pointer
to a device tree with platform information. Save these registers for
future use.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
---
 arch/arm/mach-rmobile/lowlevel_init_gen3.S | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Eugeniu Rosca March 12, 2019, 6:59 p.m. UTC | #1
Hi Marek,

On Tue, Mar 5, 2019 at 4:41 AM Marek Vasut <marek.vasut@gmail.com> wrote:
[..]
> +.align 8
> +.globl rcar_atf_boot_args
> +rcar_atf_boot_args:
> +       .dword 0
> +       .dword 0
> +       .dword 0
> +       .dword 0
> +
> +ENTRY(save_boot_params)
> +       adr     x8, rcar_atf_boot_args
> +       stp     x0, x1, [x8], #16
> +       stp     x2, x3, [x8], #16
> +       b       save_boot_params_ret
> +ENDPROC(save_boot_params)

What about relocating the function to C like in [1] and passing the 4
arguments to it from ASM like in [2]?
This would allow to:
 - reduce asm operations to minimum (mov and bl) and make code transparent.
 - avoid custom globals and store the ATF information in some C-defined struct.

[1] https://github.com/ARM-software/arm-trusted-firmware/blob/c48d02bade88b07fa7f43aa44e5217f68e5d047f/bl1/bl1_main.c#L263
[2] https://github.com/ARM-software/arm-trusted-firmware/blob/c48d02bade88b07fa7f43aa44e5217f68e5d047f/bl1/aarch32/bl1_exceptions.S#L133

Best regards,
Eugeniu.
Tom Rini March 12, 2019, 7:42 p.m. UTC | #2
On Tue, Mar 12, 2019 at 07:59:40PM +0100, Eugeniu Rosca wrote:
> Hi Marek,
> 
> On Tue, Mar 5, 2019 at 4:41 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> [..]
> > +.align 8
> > +.globl rcar_atf_boot_args
> > +rcar_atf_boot_args:
> > +       .dword 0
> > +       .dword 0
> > +       .dword 0
> > +       .dword 0
> > +
> > +ENTRY(save_boot_params)
> > +       adr     x8, rcar_atf_boot_args
> > +       stp     x0, x1, [x8], #16
> > +       stp     x2, x3, [x8], #16
> > +       b       save_boot_params_ret
> > +ENDPROC(save_boot_params)
> 
> What about relocating the function to C like in [1] and passing the 4
> arguments to it from ASM like in [2]?
> This would allow to:
>  - reduce asm operations to minimum (mov and bl) and make code transparent.
>  - avoid custom globals and store the ATF information in some C-defined struct.
> 
> [1] https://github.com/ARM-software/arm-trusted-firmware/blob/c48d02bade88b07fa7f43aa44e5217f68e5d047f/bl1/bl1_main.c#L263
> [2] https://github.com/ARM-software/arm-trusted-firmware/blob/c48d02bade88b07fa7f43aa44e5217f68e5d047f/bl1/aarch32/bl1_exceptions.S#L133

This is super early in the boot process and I'm really not a fan in
general of writing and calling C before we have things setup for C calls
to actually work normally, even more so for small things that can be
commented to be obvious as to what's going on.
Eugeniu Rosca March 12, 2019, 8:29 p.m. UTC | #3
Hi Tom,

On Tue, Mar 12, 2019 at 03:42:24PM -0400, Tom Rini wrote:
> On Tue, Mar 12, 2019 at 07:59:40PM +0100, Eugeniu Rosca wrote:
> > Hi Marek,
> > 
> > On Tue, Mar 5, 2019 at 4:41 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> > [..]
> > > +.align 8
> > > +.globl rcar_atf_boot_args
> > > +rcar_atf_boot_args:
> > > +       .dword 0
> > > +       .dword 0
> > > +       .dword 0
> > > +       .dword 0
> > > +
> > > +ENTRY(save_boot_params)
> > > +       adr     x8, rcar_atf_boot_args
> > > +       stp     x0, x1, [x8], #16
> > > +       stp     x2, x3, [x8], #16
> > > +       b       save_boot_params_ret
> > > +ENDPROC(save_boot_params)
> > 
> > What about relocating the function to C like in [1] and passing the 4
> > arguments to it from ASM like in [2]?
> > This would allow to:
> >  - reduce asm operations to minimum (mov and bl) and make code transparent.
> >  - avoid custom globals and store the ATF information in some C-defined struct.
> > 
> > [1] https://github.com/ARM-software/arm-trusted-firmware/blob/c48d02bade88b07fa7f43aa44e5217f68e5d047f/bl1/bl1_main.c#L263
> > [2] https://github.com/ARM-software/arm-trusted-firmware/blob/c48d02bade88b07fa7f43aa44e5217f68e5d047f/bl1/aarch32/bl1_exceptions.S#L133
> 
> This is super early in the boot process and I'm really not a fan in
> general of writing and calling C before we have things setup for C calls
> to actually work normally, even more so for small things that can be
> commented to be obvious as to what's going on.

I don't have a strong preference about that. It is just my experience
that Renesas once rewrote the elaborate Bosch-contributed BL2 routine
of enabling the cntfrq_el0 system counter from ASM [1] to C [2]. This
is at the BL1-BL2 boundary, while we are still in EL3, so much earlier
in the boot process. This implementation currently runs on the targets
of our customers.

I am fine with any working solution. Thanks for commenting!

[1] https://github.com/renesas-rcar/arm-trusted-firmware/commit/e23524689abb637b14a2961a305f6cfb43909319
[2] https://github.com/renesas-rcar/arm-trusted-firmware/commit/867d84191319#diff-471251fb86bd634ba14891d950c1440eR177

> -- 
> Tom

Best regards,
Eugeniu.
Marek Vasut March 12, 2019, 9:07 p.m. UTC | #4
On 3/12/19 9:29 PM, Eugeniu Rosca wrote:
> Hi Tom,

Hi,

> On Tue, Mar 12, 2019 at 03:42:24PM -0400, Tom Rini wrote:
>> On Tue, Mar 12, 2019 at 07:59:40PM +0100, Eugeniu Rosca wrote:
>>> Hi Marek,
>>>
>>> On Tue, Mar 5, 2019 at 4:41 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>> [..]
>>>> +.align 8
>>>> +.globl rcar_atf_boot_args
>>>> +rcar_atf_boot_args:
>>>> +       .dword 0
>>>> +       .dword 0
>>>> +       .dword 0
>>>> +       .dword 0
>>>> +
>>>> +ENTRY(save_boot_params)
>>>> +       adr     x8, rcar_atf_boot_args
>>>> +       stp     x0, x1, [x8], #16
>>>> +       stp     x2, x3, [x8], #16
>>>> +       b       save_boot_params_ret
>>>> +ENDPROC(save_boot_params)
>>>
>>> What about relocating the function to C like in [1] and passing the 4
>>> arguments to it from ASM like in [2]?
>>> This would allow to:
>>>  - reduce asm operations to minimum (mov and bl) and make code transparent.
>>>  - avoid custom globals and store the ATF information in some C-defined struct.
>>>
>>> [1] https://github.com/ARM-software/arm-trusted-firmware/blob/c48d02bade88b07fa7f43aa44e5217f68e5d047f/bl1/bl1_main.c#L263
>>> [2] https://github.com/ARM-software/arm-trusted-firmware/blob/c48d02bade88b07fa7f43aa44e5217f68e5d047f/bl1/aarch32/bl1_exceptions.S#L133
>>
>> This is super early in the boot process and I'm really not a fan in
>> general of writing and calling C before we have things setup for C calls
>> to actually work normally, even more so for small things that can be
>> commented to be obvious as to what's going on.
> 
> I don't have a strong preference about that. It is just my experience
> that Renesas once rewrote the elaborate Bosch-contributed BL2 routine
> of enabling the cntfrq_el0 system counter from ASM [1] to C [2].

At that point, BL2 already has a C runtime set up.

This code is instead running without the C runtime and we want to make
sure the correct registers get stored at the correct location, without
the compiler interfering with it in any way. Keep in mind this code runs
right at the beginning of U-Boot execution to make sure registers used
to pass parameters stay intact (which I believe is what Tom meant by
"early in the boot process").

> This
> is at the BL1-BL2 boundary, while we are still in EL3, so much earlier
> in the boot process. This implementation currently runs on the targets
> of our customers.
> 
> I am fine with any working solution. Thanks for commenting!
> 
> [1] https://github.com/renesas-rcar/arm-trusted-firmware/commit/e23524689abb637b14a2961a305f6cfb43909319
> [2] https://github.com/renesas-rcar/arm-trusted-firmware/commit/867d84191319#diff-471251fb86bd634ba14891d950c1440eR177
> 
>> -- 
>> Tom
> 
> Best regards,
> Eugeniu.
>
diff mbox series

Patch

diff --git a/arch/arm/mach-rmobile/lowlevel_init_gen3.S b/arch/arm/mach-rmobile/lowlevel_init_gen3.S
index f42b53fd88..213ec143e2 100644
--- a/arch/arm/mach-rmobile/lowlevel_init_gen3.S
+++ b/arch/arm/mach-rmobile/lowlevel_init_gen3.S
@@ -16,6 +16,21 @@ 
 #include <linux/linkage.h>
 #include <asm/macro.h>
 
+.align 8
+.globl	rcar_atf_boot_args
+rcar_atf_boot_args:
+	.dword 0
+	.dword 0
+	.dword 0
+	.dword 0
+
+ENTRY(save_boot_params)
+	adr	x8, rcar_atf_boot_args
+	stp	x0, x1, [x8], #16
+	stp	x2, x3, [x8], #16
+	b	save_boot_params_ret
+ENDPROC(save_boot_params)
+
 ENTRY(lowlevel_init)
 	mov	x29, lr			/* Save LR */