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

[Anthony PERARD <anthony.perard@citrix.com>]

On Sun, Mar 08, 2020 at 06:29:02PM +0000, Julien Grall wrote:
> On 06/03/2020 17:42, Anthony PERARD wrote:
> > -  - 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.

It was replaced by:
    - pl011
      - CONFIG_EARLY_UART_BAUD_RATE is, optionally a baud rate which should
        be used to configure the UART at start of day.

        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.

So they aren't really removed, just reworked I think. But I probably
need to rework the pl011 one as they may not need to expose
EARLY_UART_INIT to users.


> >     - 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.

Will do.

> > 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"
[...]
> > +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.

That probably can be done. I'll need to add more help, and maybe better
descriptions.

> > +	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_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.

I just kept the description from your patch and didn't bother to write
help messages for the other. :-)
I think I can take the time now to rework the prompts and help messages
of all configuration options.

> > +
> > +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.

I though that wasn't possible, but it seems to work. I didn't understand
well enough how select worked.  But having:
    config EARLY_UART_8250
        select EARLY_PRINTK
works, so I do that, and remove the long list of dependencies on other
config options.

> > +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?

I had issue trying to have _INIT depends on BAUD_RATE != 0. That why I
did this.
But trying again with:
    config EARLY_UART_INIT
            depends on EARLY_UART_PL011 && EARLY_UART_PL011_BAUD_RATE != 0
            def_bool y
seems to work fine. So I'll do that stop exposing _INIT to users.

Thanks,

-- 
Anthony PERARD

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