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

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

On Fri, 13 Mar 2020 at 23:12, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
>
> 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@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>
> > ---
> >
> > 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.)

As you say *today*. How about in the future? There are no promise any
platform using cadence UART will be wired at the same address.
If you specify a default address, then the risk is the user will
forget to update it and see no log at all (Xen may crash if the
address is invalid).

By not specifying a default address, the build system should shout at
you and therefore you will know that you need to configure the
address.

So the slight inconvenience to the user to specify an address is not
worth the risk.

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

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