[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

Hi,

On 06/03/2020 17:42, Anthony PERARD wrote:

From: Julien Grall <julien.grall@xxxxxxx>

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 a UART driver has been selected, the choice to select a platform
won't be possible.

Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

---

Original patch:
     [PATCH for-4.13] xen/arm: Add Skeleton for using configuring early printk 
using Kconfig
     <20190913103953.8182-1-julien.grall@xxxxxxx>
---
  docs/misc/arm/early-printk.txt |  50 ++++----
  xen/Kconfig.debug              |   2 +
  xen/arch/arm/Kconfig.debug     | 208 +++++++++++++++++++++++++++++++++
  xen/arch/arm/Rules.mk          |  72 ------------
  xen/arch/x86/Kconfig.debug     |   0
  5 files changed, 234 insertions(+), 98 deletions(-)
  create mode 100644 xen/arch/arm/Kconfig.debug
  create mode 100644 xen/arch/x86/Kconfig.debug

diff --git a/docs/misc/arm/early-printk.txt b/docs/misc/arm/early-printk.txt
index 89e081e51eaf..7dff6c314549 100644
--- a/docs/misc/arm/early-printk.txt
+++ b/docs/misc/arm/early-printk.txt
@@ -1,42 +1,40 @@
  How to enable early printk
-Early printk can only be enabled if debug=y. You may want to enable it if 
-you are debbuging code that executes before the console is initialized.
+Early printk can only be enabled if CONFIG_DEBUG=y. You may want to enable
+it 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.
-CONFIG_EARLY_PRINTK=<INC>,<BASE_ADDRESS>,<OTHER_OPTIONS> 
+Select one of the "UART drivers for early printk" in the "Debugging options" of
+Kconfig. You will then need to set other options, which depends on the drivers
+selected.
-<INC> and <BASE_ADDRESS> are mandatory arguments: 
+CONFIG_EARLY_UART_BASE_ADDRESS is a mandatory arguments, it is the base


s/arguments/argument/


+physical address of the UART to use.
- - <INC> is the name of the driver, see xen/arch/arm/arm{32,64}/debug-*.inc 
-    (where <INC> corresponds to the wildcarded *).
-  - <BASE_ADDRESS> is the base physical address of the UART to use
+Other options depends on the driver selected:
+  - 8250
+    - CONFIG_EARLY_UART_8250_REG_SHIFT is, optionally, the left-shift to
+      apply to the register offsets within the uart.
+  - pl011
+    - CONFIG_EARLY_UART_BAUD_RATE is, optionally a baud rate which should
+      be used to configure the UART at start of day.
-<OTHER_OPTIONS> varies depending on <INC>: 
+      Select CONFIG_EARLY_UART_INIT to have the option, if that's set to N
+      then the code will not try to initialize the UART, so that bootloader
+      or firmware settings can be used for maximum compatibility.
+  - scif
+    - CONFIG_EARLY_UART_SCIF_VERSION is, optionally, the interface version
+      of the UART. Default to version NONE.
- - 8250,<BASE_ADDRESS>,<REG_SHIFT> 
-    - <REG_SHIFT> is, optionally, the left-shift to apply to the
-      register offsets within the uart.
-  - pl011,<BASE_ADDRESS>,<BAUD_RATE>
-    - <BAUD_RATE> is, optionally a baud rate which should be used to
-      configure the UART at start of day.
-
-      If <BAUD_RATE> is not given then the code will not try to
-      initialize the UART, so that bootloader or firmware settings can
-     be used for maximum compatibility.


Why did this paragraph and...


-  - scif,<BASE_ADDRESS>,<VERSION>
-    - SCIF<VERSION> is, optionally, the interface version of the UART.
-
-      If <VERSION> is not given then the default interface version (SCIF)
-      will be used.
... this one were removed? They actually provide information to the user of what will happen if they parameters are left to their default value. 


    - For all other uarts there are no additional options.
As a convenience it is also possible to select from a list of 
-predefined configurations using CONFIG_EARLY_PRINTK=mach where mach is
-the name of the machine:
+predefined configurations via "Enable early printk for a specific platform
+(deprecated)".
- brcm: printk with 8250 on Broadcom 7445D0 boards with A15 processors. 
    - dra7: printk with 8250 on DRA7 platform
@@ -58,7 +56,7 @@ the name of the machine:
    - xgene-storm: printk with 820 on Xgene storm platform
    - zynqmp: printk with Cadence UART for Xilinx ZynqMP SoCs
I think you want to drop the list of early printk alias as they will be invalid after this patch.
-These settings are is hardcoded in xen/arch/arm/Rules.mk, 
+These settings are is hardcoded in xen/arch/arm/Kconfig.debug,
  see there when adding support for new machines.
By default early printk is disabled. 
diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
index b3511e81a275..ee6ee33b69be 100644
--- a/xen/Kconfig.debug
+++ b/xen/Kconfig.debug
@@ -128,6 +128,8 @@ config XMEM_POOL_POISON
          Poison free blocks with 0xAA bytes and verify them when a block is
          allocated in order to spot use-after-free issues.
+source "arch/$(SRCARCH)/Kconfig.debug" 
+
  endif # DEBUG || EXPERT
endmenu 
diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
new file mode 100644
index 000000000000..5111f89043ca
--- /dev/null
+++ b/xen/arch/arm/Kconfig.debug
@@ -0,0 +1,208 @@
+choice
+       bool "UART drivers for early printk"
+       optional
+       help
+               Choose one of the UART driver, then you'll have to specifie the


s/specifie/specify/


+               parameters, like the base address.
+
+               Alternatively, there are platform specific options
+       config EARLY_UART_CHOICE_8250
+               select EARLY_UART_8250
+               bool "8250 driver"
+       config EARLY_UART_CHOICE_CADENCE
+               select EARLY_UART_CADENCE
+               depends on ARM_64
+               bool "Enable early printk via Cadence UART"
+       config EARLY_UART_CHOICE_EXYNOS4210
+               select EARLY_UART_EXYNOS4210
+               depends on ARM_32
+               bool "exynos 4210 driver"
+       config EARLY_UART_CHOICE_MESON
+               select EARLY_UART_MESON
+               depends on ARM_64
+               bool "meson driver"
+       config EARLY_UART_CHOICE_MVEBU
+               select EARLY_UART_MVEBU
+               depends on ARM_64
+               bool "mvebu driver"
+       config EARLY_UART_CHOICE_PL011
+               select EARLY_UART_PL011
+               bool "pl011 driver"
+       config EARLY_UART_CHOICE_SCIF
+               select EARLY_UART_SCIF
+               bool "scif driver"
+endchoice
+
+
+choice
+       bool "Enable early printk for a specific platform (deprecated)"
+       depends on !(EARLY_UART_CHOICE_8250 || EARLY_UART_CHOICE_CADENCE || 
EARLY_UART_CHOICE_EXYNOS4210 || EARLY_UART_CHOICE_MESON || 
EARLY_UART_CHOICE_MVEBU || EARLY_UART_CHOICE_PL011 || EARLY_UART_CHOICE_SCIF)
The split is going to cause confusion to the users because he/she may select the UART type first and then lose access to this list.
Furthermore, the depends on !(...) is pretty horrible to have. This is also going to make more difficult to add new UART type (there are a few more existing...). 

So I would prefer if we have one list.


+       optional
+       help
+               Those are platform specific options for early printk. This are
+               deprecated and will soon be removed.
+
+               Select a UART driver instead.
+
+       config EARLY_PRINTK_BRCM
+               bool "printk with 8250 on Broadcom 7445D0 boards with A15 
processors"
+               select EARLY_UART_8250
+       config EARLY_PRINTK_DRA7
+               bool "printk with 8250 on DRA7 platform"
+               select EARLY_UART_8250
+       config EARLY_PRINTK_EXYNOS5250
+               bool "printk with the second UART on Exynos5250"
+               select EARLY_UART_EXYNOS4210
+               depends on ARM_32
+       config EARLY_PRINTK_FASTMODEL
+               bool "printk on ARM Fastmodel software emulators"
+               select EARLY_UART_PL011
+       config EARLY_PRINTK_HIKEY960
+               bool "printk with pl011 with Hikey 960"
+               select EARLY_UART_PL011
+       config EARLY_PRINTK_JUNO
+               bool "printk with pl011 on Juno platform"
+               select EARLY_UART_PL011
+       config EARLY_PRINTK_LAGER
+               bool "printk with SCIF0 on Renesas Lager board (R-Car H2 
processor)"
+               select EARLY_UART_SCIF
+       config EARLY_PRINTK_MIDWAY
+               bool "printk with the pl011 on Calxeda Midway processors"
+               select EARLY_UART_PL011
+       config EARLY_PRINTK_MVEBU
+               bool "printk with the MVEBU for Marvell Armada 3700 SoCs"
+               select EARLY_UART_MVEBU
+               depends on ARM_64
+       config EARLY_PRINTK_OMAP5432
+               bool "printk with UART3 on TI OMAP5432 processors"
+               select EARLY_UART_8250
+       config EARLY_PRINTK_RCAR3
+               bool "printk with SCIF2 on Renesas R-Car Gen3 processors"
+               select EARLY_UART_SCIF
+       config EARLY_PRINTK_SEATTLE
+               bool "printk with pl011 for AMD Seattle processor"
+               select EARLY_UART_PL011
+       config EARLY_PRINTK_SUN6I
+               bool "printk with 8250 on Allwinner A31 processors"
+               select EARLY_UART_8250
+       config EARLY_PRINTK_SUN7I
+               bool "printk with 8250 on Allwinner A20 processors"
+               select EARLY_UART_8250
+       config EARLY_PRINTK_THUNDERX
+               bool "printk with pl011 for Cavium ThunderX processor"
+               select EARLY_UART_PL011
+       config EARLY_PRINTK_VEXPRESS
+               bool "printk with pl011 for versatile express"
+               select EARLY_UART_PL011
+       config EARLY_PRINTK_XGENE_MCDIVITT
+               bool "printk with 820 on Xgene mcdivitt platform"
+               select EARLY_UART_8250
+       config EARLY_PRINTK_XGENE_STORM
+               bool "printk with 820 on Xgene storm platform"
+               select EARLY_UART_8250
+       config EARLY_PRINTK_ZYNQMP
+               bool "printk with Cadence UART for Xilinx ZynqMP SoCs"
+               select EARLY_UART_CADENCE
+               depends on ARM_64
+               help
+                 Say Y here if you want the early printk support on Xilinx
+                 ZynQMP platform.
This is a bit odd to add a description for one Kconfig and not all the other. My preference would be to describe all of them, but I understand this will require extra work. 


+
+endchoice
+
+
+config EARLY_UART_8250
+       bool
+config EARLY_UART_CADENCE
+       bool
+config EARLY_UART_EXYNOS4210
+       bool
+config EARLY_UART_MESON
+       bool
+config EARLY_UART_MVEBU
+       bool
+config EARLY_UART_PL011
+       bool
+config EARLY_UART_SCIF
+       bool
+
+config EARLY_PRINTK
+       depends on EARLY_UART_8250 || EARLY_UART_CADENCE || 
EARLY_UART_EXYNOS4210 || EARLY_UART_MESON || EARLY_UART_MVEBU || 
EARLY_UART_PL011 || EARLY_UART_SCIF


Please rework this and let each EARLY_UART_* to select EARLY_PRINTK.


+       def_bool y
+
+config EARLY_UART_BASE_ADDRESS
+       depends on EARLY_UART_8250 || EARLY_UART_CADENCE || 
EARLY_UART_EXYNOS4210 || EARLY_UART_MESON || EARLY_UART_MVEBU || 
EARLY_UART_PL011 || EARLY_UART_SCIF


This can depends on EARLY_PRINTK.


+       hex "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
+
+config EARLY_UART_INIT
+       depends on EARLY_UART_PL011
+       bool "Initialize UART early"
+       default y if EARLY_PRINTK_FASTMODEL
+       help
+               Select N to keep the settings that the bootloader or firmware
+               have selected, for maximum compatibility.
+
+               Select Y to initialize the UART with a new baud rate.
At the moment, we rely on the firmware to initialize the UART correctly (and not only the baud rate...). But it may be possible that it was done incorrectly. So the earlyprintk code may require to reset the UART. In the case, the user should have no choice as this as a pretty low impact.
Can we instead select EARLY_UART_INIT based on whether the BAUD_RATE has been selected? 

Cheers,

--
Julien Grall

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

 

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.

Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.