diff mbox series

[03/17] xen/arm64: head: Rework UART initialization on boot CPU

Message ID 20190610193215.23704-4-julien.grall@arm.com (mailing list archive)
State New, archived
Headers show
Series xen/arm64: Rework head.S to make it more compliant with the Arm Arm | expand

Commit Message

Julien Grall June 10, 2019, 7:32 p.m. UTC
Anything executed after the label common_start can be executed on all
CPUs. However most of the instructions executed between the label
common_start and init_uart are not executed on the boot CPU.

The only instructions executed are to lookup the CPUID so it can be
printed on the console (if earlyprintk is enabled). Printing the CPUID
is not entirely useful to have for the boot CPU and requires a
conditional branch to bypass unused instructions.

Furthermore, the function init_uart is only called for boot CPU
requiring another conditional branch. This makes the code a bit tricky
to follow.

The UART initialization is now moved before the label common_start. This
now requires to have a slightly altered print for the boot CPU and set
the early UART base address in each the two path (boot CPU and
secondary CPUs).

This has the nice effect to remove a couple of conditional branch in
the code.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm64/head.S | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

Comments

Stefano Stabellini June 25, 2019, 11:49 p.m. UTC | #1
On Mon, 10 Jun 2019, Julien Grall wrote:
> Anything executed after the label common_start can be executed on all
> CPUs. However most of the instructions executed between the label
> common_start and init_uart are not executed on the boot CPU.
> 
> The only instructions executed are to lookup the CPUID so it can be
> printed on the console (if earlyprintk is enabled). Printing the CPUID
> is not entirely useful to have for the boot CPU and requires a
> conditional branch to bypass unused instructions.
> 
> Furthermore, the function init_uart is only called for boot CPU
> requiring another conditional branch. This makes the code a bit tricky
> to follow.
> 
> The UART initialization is now moved before the label common_start. This
> now requires to have a slightly altered print for the boot CPU and set
> the early UART base address in each the two path (boot CPU and
> secondary CPUs).
> 
> This has the nice effect to remove a couple of conditional branch in
> the code.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

> ---
>  xen/arch/arm/arm64/head.S | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index a5147c8d80..fd432ee15d 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -265,6 +265,12 @@ real_start_efi:
>          load_paddr x21, _sdtb
>  #endif
>  
> +        /* Initialize the UART if earlyprintk has been enabled. */
> +#ifdef CONFIG_EARLY_PRINTK
> +        bl    init_uart
> +#endif
> +        PRINT("- Boot CPU booting -\r\n")
> +
>          mov   x22, #0                /* x22 := is_secondary_cpu */
>  
>          b     common_start
> @@ -281,14 +287,11 @@ GLOBAL(init_secondary)
>          /* Boot CPU already zero BSS so skip it on secondary CPUs. */
>          mov   x26, #1                /* X26 := skip_zero_bss */
>  
> -common_start:
>          mrs   x0, mpidr_el1
>          ldr   x13, =(~MPIDR_HWID_MASK)
>          bic   x24, x0, x13           /* Mask out flags to get CPU ID */
>  
> -        /* Non-boot CPUs wait here until __cpu_up is ready for them */
> -        cbz   x22, 1f
> -
> +        /* Wait here until __cpu_up is ready to handle the CPU */
>          load_paddr x0, smp_up_cpu
>          dsb   sy
>  2:      ldr   x1, [x0]
> @@ -300,14 +303,14 @@ common_start:
>  
>  #ifdef CONFIG_EARLY_PRINTK
>          ldr   x23, =EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
> -        cbnz  x22, 1f
> -        bl    init_uart                 /* Boot CPU sets up the UART too */
> -1:      PRINT("- CPU ")
> +        PRINT("- CPU ")
>          mov   x0, x24
>          bl    putn
>          PRINT(" booting -\r\n")
>  #endif
>  
> +common_start:
> +
>          PRINT("- Current EL ")
>          mrs   x4, CurrentEL
>          mov   x0, x4
> @@ -628,10 +631,16 @@ ENTRY(switch_ttbr)
>          ret
>  
>  #ifdef CONFIG_EARLY_PRINTK
> -/* Bring up the UART.
> - * x23: Early UART base address
> - * Clobbers x0-x1 */
> +/*
> + * Initialize the UART. Should only be called on the boot CPU.
> + *
> + * Ouput:
> + *  x23: Early UART base physical address
> + *
> + * Clobbers x0 - x1
> + */
>  init_uart:
> +        ldr   x23, =EARLY_UART_BASE_ADDRESS
>  #ifdef EARLY_PRINTK_INIT_UART
>          early_uart_init x23, 0
>  #endif
> -- 
> 2.11.0
>
diff mbox series

Patch

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index a5147c8d80..fd432ee15d 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -265,6 +265,12 @@  real_start_efi:
         load_paddr x21, _sdtb
 #endif
 
+        /* Initialize the UART if earlyprintk has been enabled. */
+#ifdef CONFIG_EARLY_PRINTK
+        bl    init_uart
+#endif
+        PRINT("- Boot CPU booting -\r\n")
+
         mov   x22, #0                /* x22 := is_secondary_cpu */
 
         b     common_start
@@ -281,14 +287,11 @@  GLOBAL(init_secondary)
         /* Boot CPU already zero BSS so skip it on secondary CPUs. */
         mov   x26, #1                /* X26 := skip_zero_bss */
 
-common_start:
         mrs   x0, mpidr_el1
         ldr   x13, =(~MPIDR_HWID_MASK)
         bic   x24, x0, x13           /* Mask out flags to get CPU ID */
 
-        /* Non-boot CPUs wait here until __cpu_up is ready for them */
-        cbz   x22, 1f
-
+        /* Wait here until __cpu_up is ready to handle the CPU */
         load_paddr x0, smp_up_cpu
         dsb   sy
 2:      ldr   x1, [x0]
@@ -300,14 +303,14 @@  common_start:
 
 #ifdef CONFIG_EARLY_PRINTK
         ldr   x23, =EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
-        cbnz  x22, 1f
-        bl    init_uart                 /* Boot CPU sets up the UART too */
-1:      PRINT("- CPU ")
+        PRINT("- CPU ")
         mov   x0, x24
         bl    putn
         PRINT(" booting -\r\n")
 #endif
 
+common_start:
+
         PRINT("- Current EL ")
         mrs   x4, CurrentEL
         mov   x0, x4
@@ -628,10 +631,16 @@  ENTRY(switch_ttbr)
         ret
 
 #ifdef CONFIG_EARLY_PRINTK
-/* Bring up the UART.
- * x23: Early UART base address
- * Clobbers x0-x1 */
+/*
+ * Initialize the UART. Should only be called on the boot CPU.
+ *
+ * Ouput:
+ *  x23: Early UART base physical address
+ *
+ * Clobbers x0 - x1
+ */
 init_uart:
+        ldr   x23, =EARLY_UART_BASE_ADDRESS
 #ifdef EARLY_PRINTK_INIT_UART
         early_uart_init x23, 0
 #endif