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 |
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.
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.
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.
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 --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 */
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(+)