[Xen-devel,v2,22/35] xen/arm32: head: Rework UART initialization on boot CPU

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.

After this rework, the CPUID is only used at the very beginning of the
secondary CPUs boot path. So there is no need to "reserve" x24 for the
CPUID.

Lastly, take the opportunity to replace load from literal pool with the
new macro mov_w.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm32/head.S | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

On Mon, 22 Jul 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.
> 
> After this rework, the CPUID is only used at the very beginning of the
> secondary CPUs boot path. So there is no need to "reserve" x24 for the
> CPUID.
> 
> Lastly, take the opportunity to replace load from literal pool with the
> new macro mov_w.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index b54331c19d..134c3dda92 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -54,7 +54,7 @@
>   *   r4  -
>   *   r5  -
>   *   r6  - identity map in place
> - *   r7  - CPUID
> + *   r7  -
>   *   r8  - DTB address (boot CPU only)
>   *   r9  - paddr(start)
>   *   r10 - phys offset
> @@ -123,6 +123,12 @@ past_zImage:
>          add   r8, r10                /* r8 := paddr(DTB) */
>  #endif
>  
> +        /* Initialize the UART if earlyprintk has been enabled. */
> +#ifdef CONFIG_EARLY_PRINTK
> +        bl    init_uart
> +#endif
> +        PRINT("- Boot CPU booting -\r\n")
> +
>          mov   r12, #0                /* r12 := is_secondary_cpu */
>  
>          b     common_start
> @@ -137,14 +143,9 @@ GLOBAL(init_secondary)
>  
>          mov   r12, #1                /* r12 := is_secondary_cpu */
>  
> -common_start:
>          mrc   CP32(r1, MPIDR)
>          bic   r7, r1, #(~MPIDR_HWID_MASK) /* Mask out flags to get CPU ID */
>  
> -        /* Non-boot CPUs wait here until __cpu_up is ready for them */
> -        teq   r12, #0
> -        beq   1f
> -
>          ldr   r0, =smp_up_cpu
>          add   r0, r0, r10            /* Apply physical offset */
>          dsb
> @@ -156,15 +157,14 @@ common_start:
>  1:
>  
>  #ifdef CONFIG_EARLY_PRINTK
> -        ldr   r11, =EARLY_UART_BASE_ADDRESS  /* r11 := UART base address */
> -        teq   r12, #0                /* Boot CPU sets up the UART too */
> -        bleq  init_uart
> +        mov_w r11, EARLY_UART_BASE_ADDRESS   /* r11 := UART base address */
>          PRINT("- CPU ")
>          mov   r0, r7
>          bl    putn
>          PRINT(" booting -\r\n")
>  #endif
>  
> +common_start:
>          /* Check that this CPU has Hyp mode */
>          mrc   CP32(r0, ID_PFR1)
>          and   r0, r0, #0xf000        /* Bits 12-15 define virt extensions */
> @@ -497,11 +497,15 @@ ENTRY(switch_ttbr)
>  
>  #ifdef CONFIG_EARLY_PRINTK
>  /*
> - * Bring up the UART.
> - * r11: Early UART base address
> - * Clobbers r0-r2
> + * Initialize the UART. Should only be called on the boot CPU.
> + *
> + * Ouput:
      ^ this should be output, and in the arm64 patch too (already committed)

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



> + *  r11: Early UART base physical address
> + *
> + * Clobbers r0 - r3
>   */
>  init_uart:
> +        mov_w r11, EARLY_UART_BASE_ADDRESS
>  #ifdef EARLY_PRINTK_INIT_UART
>          early_uart_init r11, r1, r2
>  #endif

Hi Stefano,

On 7/30/19 8:40 PM, Stefano Stabellini wrote:
> On Mon, 22 Jul 2019, Julien Grall wrote:
>> @@ -497,11 +497,15 @@ ENTRY(switch_ttbr)
>>   
>>   #ifdef CONFIG_EARLY_PRINTK
>>   /*
>> - * Bring up the UART.
>> - * r11: Early UART base address
>> - * Clobbers r0-r2
>> + * Initialize the UART. Should only be called on the boot CPU.
>> + *
>> + * Ouput:
>        ^ this should be output, and in the arm64 patch too (already committed)

Most of the arm32 code is a copy and paste from arm64 with little 
adaptation! Hence the same typo :).

I have fixed the arm32 part directly in this patch and create a new one 
for fixing the arm64 part.


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

Thank you!

Cheers,