Re: [Xen-devel] [XEN PATCH v3 2/2] xen/arm: Configure early printk via Kconfig

[Stefano Stabellini <sstabellini@kernel.org>]

On Mon, 9 Mar 2020, Anthony PERARD wrote:
> At the moment, early printk can only be configured on the make command
> line. It is not very handy because a user has to remove the option
> everytime it is using another command other than compiling the
> hypervisor.
> 
> Furthermore, early printk is one of the few odds one that are not
> using Kconfig.
> 
> So this is about time to move it to Kconfig.
> 
> The new kconfigs options allow a user to eather select a UART driver
> to use at boot time, and set the parameters, or it is still possible
> to select a platform which will set the parameters.
> 
> If CONFIG_EARLY_PRINTK is present in the environment or on the make
> command line, make will return an error.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> ---
> 
> Original patch:
>     [PATCH for-4.13] xen/arm: Add Skeleton for using configuring early printk using Kconfig
>     <20190913103953.8182-1-julien.grall@arm.com>
> ---
> 
> Notes:
>     v3:
>     - rename EARLY_PRINK to CONFIG_EARLY_PRINTK in makefile here (which
>       select which object to build).
>     - rename EARLY_UART_BAUD_RATE to EARLY_UART_PL011_BAUD_RATE
>     - typos
>     - drop the list of aliases in early-printk.txt. Kconfig choice menu
>       should be enough.
>     - reword early-printk.txt.
>     - rework how EARLY_PRINTK is set to Y
>       and use that instead of a list of all EARLY_UART_*
>     - Add a check to ask user to use Kconfig to set early printk.
>     - rework the possible choice to have all uart driver and platform
>       specific option together.
>     - have added or reword prompt and help messages of the different
>       options. The platform specific option don't have extended help, the
>       prompt is probably enough.
>       (The non-platform specific options have the help message that Julien
>       have written in the first version.)
>     - have made EARLY_UART_INIT dependent on the value of
>       EARLY_UART_PL011_BAUD_RATE so that there is no need to expose _INIT to
>       users.
> 

The patch is fine by me. I only have one very minor comment below.


> +	config EARLY_UART_CHOICE_CADENCE
> +		select EARLY_UART_CADENCE
> +		depends on ARM_64
> +		bool "Early printk via Cadence UART"
> +		help
> +			Say Y here if you wish the early printk to direct their
> +			output to a Cadence UART. You can use this option to
> +			provide the parameters for the Cadence UART rather than
> +			selecting one of the platform specific options below if
> +			you know the parameters for the port.
> +
> +			This option is preferred over the platform specific
> +			options; the platform specific options are deprecated
> +			and will soon be removed.

[...]

> +	config EARLY_PRINTK_ZYNQMP
> +		bool "Early printk with Cadence UART for Xilinx ZynqMP SoCs"
> +		select EARLY_UART_CADENCE
> +		depends on ARM_64
> +endchoice

[...]

> +config EARLY_UART_BASE_ADDRESS
> +	depends on EARLY_PRINTK
> +	hex "Early printk, physical base address of debug UART"
> +	default 0xF040AB00 if EARLY_PRINTK_BRCM
> +	default 0x4806A000 if EARLY_PRINTK_DRA7
> +	default 0x1c090000 if EARLY_PRINTK_FASTMODEL
> +	default 0x12c20000 if EARLY_PRINTK_EXYNOS5250
> +	default 0xfff32000 if EARLY_PRINTK_HIKEY960
> +	default 0x7ff80000 if EARLY_PRINTK_JUNO
> +	default 0xe6e60000 if EARLY_PRINTK_LAGER
> +	default 0xfff36000 if EARLY_PRINTK_MIDWAY
> +	default 0xd0012000 if EARLY_PRINTK_MVEBU
> +	default 0x48020000 if EARLY_PRINTK_OMAP5432
> +	default 0xe6e88000 if EARLY_PRINTK_RCAR3
> +	default 0xe1010000 if EARLY_PRINTK_SEATTLE
> +	default 0x01c28000 if EARLY_PRINTK_SUN6I
> +	default 0x01c28000 if EARLY_PRINTK_SUN7I
> +	default 0x87e024000000 if EARLY_PRINTK_THUNDERX
> +	default 0x1c090000 if EARLY_PRINTK_VEXPRESS
> +	default 0x1c021000 if EARLY_PRINTK_XGENE_MCDIVITT
> +	default 0x1c020000 if EARLY_PRINTK_XGENE_STORM
> +	default 0xff000000 if EARLY_PRINTK_ZYNQMP

Today we only have one board with CADENCE UART which is ZynqMP. However,
only if EARLY_PRINTK_ZYNQMP is selected the BASE_ADDRESS is default to
0xff000000.

Ideally, BASE_ADDRESS would default to 0xff000000 if EARLY_PRINTK_ZYNQMP
or if EARLY_UART_CADENCE. (There is one more similar example which is
EARLY_UART_EXYNOS4210.)

I don't know if it is worth optimizing, I'll let you and Julien decide.

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