Re: [Xen-devel] [XEN PATCH v2 1/2] xen/arm: Rename all early printk macro

[Julien Grall <julien@xxxxxxx>]

Hi,

On 06/03/2020 17:42, Anthony PERARD wrote:
> We are going to move the generation of the early printk macro into
> Kconfig. This means all macro will be prefix with CONFIG_. We do that
> ahead of the change.
>
> We also take the opportunity to better name some variables, which are
> used by only one driver and wouldn't make sens for other UART driver.
> Thus,
>      - EARLY_UART_REG_SHIFT became CONFIG_EARLY_UART_8250_REG_SHIFT
>      - EARLY_PRINTK_VERSION_* became CONFIG_EARLY_UART_SCIF_VERSION_*
>
> The other variables are change to have the prefix CONFIG_EARLY_UART_
> when they change a parameter of the driver. So we have now:
>      - CONFIG_EARLY_UART_BAUD_RATE
>      - CONFIG_EARLY_UART_BASE_ADDRESS
>      - CONFIG_EARLY_UART_INIT
>
> We also reuse CONFIG_EARLY_PRINTK provided by users to enable or not
> early printk, thus we need to override the value in makefile.
>
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
> That's based on early work by Julien
>      [PATCH for-4.13] xen/arm: Add Skeleton for using configuring early printk 
> using Kconfig
>      <20190913103953.8182-1-julien.grall@xxxxxxx>
> ---
>   xen/arch/arm/Makefile              |  2 +-
>   xen/arch/arm/Rules.mk              | 20 +++++++++-----------
>   xen/arch/arm/arm32/Makefile        |  2 +-
>   xen/arch/arm/arm32/debug-8250.inc  |  2 +-
>   xen/arch/arm/arm32/debug-pl011.inc |  4 ++--
>   xen/arch/arm/arm32/debug-scif.inc  |  4 ++--
>   xen/arch/arm/arm32/debug.S         |  4 ++--
>   xen/arch/arm/arm32/head.S          | 10 +++++-----
>   xen/arch/arm/arm64/Makefile        |  2 +-
>   xen/arch/arm/arm64/debug-8250.inc  |  4 ++--
>   xen/arch/arm/arm64/debug-pl011.inc |  4 ++--
>   xen/arch/arm/arm64/debug.S         |  4 ++--
>   xen/arch/arm/arm64/head.S          | 10 +++++-----
>   xen/include/asm-arm/early_printk.h |  2 +-
>   14 files changed, 36 insertions(+), 38 deletions(-)
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 1044c2298a05..12f92a4bd3f9 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -16,7 +16,7 @@ obj-y += device.o
>   obj-y += domain.o
>   obj-y += domain_build.init.o
>   obj-y += domctl.o
> -obj-$(EARLY_PRINTK) += early_printk.o
> +obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
>   obj-y += gic.o
>   obj-y += gic-v2.o
>   obj-$(CONFIG_GICV3) += gic-v3.o
> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> index 022a3a6f82ba..85f8a4c6f914 100644
> --- a/xen/arch/arm/Rules.mk
> +++ b/xen/arch/arm/Rules.mk
> @@ -18,8 +18,6 @@ CFLAGS-$(CONFIG_ARM_32) += -mcpu=cortex-a15
>   CFLAGS-$(CONFIG_ARM_64) += -mcpu=generic
>   CFLAGS-$(CONFIG_ARM_64) += -mgeneral-regs-only # No fp registers etc
>   
> -EARLY_PRINTK := n
> -
>   ifeq ($(CONFIG_DEBUG),y)
>   
>   # See docs/misc/arm/early-printk.txt for syntax
> @@ -66,22 +64,22 @@ endif
>   endif
>   ifeq ($(EARLY_PRINTK_INC),scif)
>   ifneq ($(word 3,$(EARLY_PRINTK_CFG)),)
> -CFLAGS-y += -DEARLY_PRINTK_VERSION_$(word 3,$(EARLY_PRINTK_CFG))
> +CFLAGS-y += -DCONFIG_EARLY_UART_SCIF_VERSION_$(word 3,$(EARLY_PRINTK_CFG))
>   else
> -CFLAGS-y += -DEARLY_PRINTK_VERSION_NONE
> +CFLAGS-y += -DCONFIG_EARLY_UART_SCIF_VERSION_NONE
>   endif
>   endif
>   
>   ifneq ($(EARLY_PRINTK_INC),)
> -EARLY_PRINTK := y
> +override CONFIG_EARLY_PRINTK := y
I am not sure to understand why you are using the directive override 
here. Why can't you just prefix the variable with CONFIG_?
>   endif
>   
> -CFLAGS-$(EARLY_PRINTK) += -DCONFIG_EARLY_PRINTK
> -CFLAGS-$(EARLY_PRINTK_INIT_UART) += -DEARLY_PRINTK_INIT_UART
> -CFLAGS-$(EARLY_PRINTK) += -DEARLY_PRINTK_INC=\"debug-$(EARLY_PRINTK_INC).inc\"
> -CFLAGS-$(EARLY_PRINTK) += -DEARLY_PRINTK_BAUD=$(EARLY_PRINTK_BAUD)
> -CFLAGS-$(EARLY_PRINTK) += -DEARLY_UART_BASE_ADDRESS=$(EARLY_UART_BASE_ADDRESS)
> -CFLAGS-$(EARLY_PRINTK) += -DEARLY_UART_REG_SHIFT=$(EARLY_UART_REG_SHIFT)
> +CFLAGS-$(CONFIG_EARLY_PRINTK) += -DCONFIG_EARLY_PRINTK
> +CFLAGS-$(EARLY_PRINTK_INIT_UART) += -DCONFIG_EARLY_UART_INIT
> +CFLAGS-$(CONFIG_EARLY_PRINTK) += 
> -DCONFIG_EARLY_PRINTK_INC=\"debug-$(EARLY_PRINTK_INC).inc\"
> +CFLAGS-$(CONFIG_EARLY_PRINTK) += 
> -DCONFIG_EARLY_UART_BAUD_RATE=$(EARLY_PRINTK_BAUD)
The baud rate is only used by the PL011 in rare cases. So I would add 
PL011 in the name.
> +CFLAGS-$(CONFIG_EARLY_PRINTK) += 
> -DCONFIG_EARLY_UART_BASE_ADDRESS=$(EARLY_UART_BASE_ADDRESS)
> +CFLAGS-$(CONFIG_EARLY_PRINTK) += 
> -DCONFIG_EARLY_UART_8250_REG_SHIFT=$(EARLY_UART_REG_SHIFT)
The name clearly suggests that this should be protected with an "if 
8250". Maybe in the similar way as for the scif specific variables. 
But... I am not going to push for it as the next patch is going to 
remove it.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel