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

[Julien Grall <julien.grall.oss@gmail.com>]

On Fri, 13 Mar 2020 at 23:19, Julien Grall <julien.grall.oss@gmail.com> wrote:
>
> On Fri, 13 Mar 2020 at 23:12, Stefano Stabellini <sstabellini@kernel.org> 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@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.)
>
> 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

I probably should have used "advantage"  rather than "inconvenience" here.

> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel