Re: [RFC 24/29] xen/arm: Don't use pl011 UART by default for early printk

[Julien Grall <julien.grall@linaro.org>]

On 04/29/2013 05:45 PM, Ian Campbell wrote:

> On Mon, 2013-04-29 at 00:02 +0100, Julien Grall wrote:
>> Add CONFIG_EARLY_PRINTK options in configs/arm{32,64}.mk to let the user
>> to choose if he wants to have early output, ie before the console is initialized.
> 
> These shouldn't go in config/arm*.mk but should be handed in
> xen/arch/arm/


I don't understand why, it's a config option and the user can modify
arm32.mk

>>
>> This code is specific for each UART. When CONFIG_EARLY_PRINTK is enabled,
>> Xen will only be able to run on a board with this UART.
>>
>> If a developper wants to add support for a new UART, he must implement the
>> following function/variable:
>>    - early_uart_paddr: variable which contains the physical base address
>>    for the UART
>>    - early_uart_init: initialize the UART
>>    - early_uart_ready: check and wait until the UART can transmit a new
>>    character
>>    - early_uart_transmit: transmit a character
>>
>> For more details about the parameters of each function,
>> see arm{32,64}/debug-pl011.S comments.
> 
> It's a damned shame the asm isn't compatible, oh well...
> 
>> diff --git a/config/arm32.mk b/config/arm32.mk
>> index f64f0c1..83a7767 100644
>> --- a/config/arm32.mk
>> +++ b/config/arm32.mk
>> @@ -7,6 +7,17 @@ CONFIG_ARM_$(XEN_OS) := y
>>  # Explicitly specifiy 32-bit ARM ISA since toolchain default can be -mthumb:
>>  CFLAGS += -marm
>>
>> +# Xen early debugging function
>> +# This is helpful if you are debbuging code that executes before the console
>> +# is initialized.
>> +# Note that selecting this option will limit Xen to a single UART
>> +# definition. Attempting to boot Xen image on a different platform *will
>> +# not work*, so this option should not be enable for Xens that are
>> +# intended to be portable.
>> +# Possible value:
>> +#   - none: no early printk
> 
> Blank/unset would represent none? Or you mean literal "none"?

I choose to use literal "none", but finally I think it's better to have
blank/unset.

>> +#   - pl011: printk with PL011 UART
>> +CONFIG_EARLY_PRINTK := none
> 
> I guess you mean literal none...
> 
> Can this be overriden on command line or in .config? You may need to
> use ?= so it can be.

Yes. Make overrides all "local" variables with the ones on the command line.

>>  HAS_PL011 := y
>>
>>  # Use only if calling $(LD) directly.
>> diff --git a/config/arm64.mk b/config/arm64.mk
>> index b2457eb..6187df8 100644
>> --- a/config/arm64.mk
>> +++ b/config/arm64.mk
>> @@ -4,6 +4,17 @@ CONFIG_ARM_$(XEN_OS) := y
>>
>>  CFLAGS += #-marm -march= -mcpu= etc
>>
>> +# Xen early debugging function
>> +# This is helpful if you are debbuging code that executes before the console
>> +# is initialized.
>> +# Note that selecting this option will limit Xen to a single UART
>> +# definition. Attempting to boot Xen image on a different platform *will
>> +# not work*, so this option should not be enable for Xens that are
>> +# intended to be portable.
>> +# Possible value:
>> +#   - none: no early printk
>> +#   - pl011: printk with PL011 UART
>> +CONFIG_EARLY_PRINTK := none
> 
> Shall we create config/arm.mk, included from arm32 and arm64 and reduce
> the duplication?


I think so.

>> diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
>> index 1ad3364..6af8ca3 100644
>> --- a/xen/arch/arm/arm32/Makefile
>> +++ b/xen/arch/arm/arm32/Makefile
>> @@ -5,4 +5,7 @@ obj-y += mode_switch.o
>>  obj-y += proc-ca15.o
>>
>>  obj-y += traps.o
>> -obj-y += domain.o
>> \ No newline at end of file
>> +obj-y += domain.o
>> +
>> +obj-$(EARLY_PRINTK) += debug.o
>> +obj-$(CONFIG_EARLY_PL011) += debug-pl011.o
> 
> This could become
> 	obj-$(EARLY_PRINTK) += debug-$(CONFIG_EARLY_PRINTK).o 
> and save adding a new one for each name? And if you create a stub
> debug-none.S you could just make it obj-y ? Should we gate this on
> debug=y?


What does debug=y do?

I think we don't need debug-none.S if CONFIG_EARLY_PRINTK is unset when
early printk is disabled. We just need to check in Rules.mk is
CONFIG_EARLY_PRINTK is set or not and defined EARLY_PRINTK.

>> @@ -106,8 +106,10 @@ past_zImage:
>>          bne   1b
>>
>>  boot_cpu:
>> -#ifdef EARLY_UART_ADDRESS
>> -        ldr   r11, =EARLY_UART_ADDRESS  /* r11 := UART base address */
>> +#ifdef EARLY_PRINTK
>> +        ldr   r0, =early_uart_paddr     /* VA of early_uart_paddr */
>> +        add   r0, r0, r10               /* PA of early_uart_paddr */
>> +        ldr   r11, [r0]                 /* r11 := UART base address */
> 
> If head.S were to #include debug-pl011.inc would that simplify some of
> this stuff?


It's also used in debug.s, which is a wrapper for the C early printk.

If we use macros instead of functions the code will be simplified. All
link register used will be removed.

I will rework this patch.

-- 
Julien