Re: [PATCH v3] drivers/char: rename arm-uart.c to uart-init.c

[oleksii.kurochko@xxxxxxxxx]

Hi Julien,

On Sat, 2024-11-16 at 10:11 +0000, Julien Grall wrote:
> Hi Oleksii,
> 
> On 15/11/2024 12:48, Oleksii Kurochko wrote:
> > Rename the file containing uart_init() to enable reuse across other
> > architectures that utilize device trees or SPCR tables to locate
> > UART
> > information.
> > After locating UART data, {acpi}_device_init() is called to
> > initialize
> > the UART.
> > 
> > arm_uart_init() is renamed to uart_init() to be reused by other
> > architectures.
> > 
> > A new configuration option, CONFIG_UART_INIT, is introduced,
> > currently
> > available only for Arm. Enabling CONFIG_UART_INIT on additional
> > architectures will require additional functionality, such as device
> > tree
> > mapping and unflattening, etc.
> > 
> > The MAINTAINERS file is updated to alphabetically sort files in the
> > "ARM (W/ VIRTUALIZATION EXTENSIONS) ARCHITECTURE" section following
> > the renaming of arm-uart.c.
> > 
> > Add `select UART_INIT` for CONFIG_ARM to be sure that randconfig
> > won't
> > disable UART_INIT.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in V3:
> >   - Drop blank line in xen/drivers/char/Kconfig.
> >   - Rebase on top of the latest staging.
> > ---
> > Changes in v2:
> >   - Rename arm-uart.c to uart-init.c instead of moving only
> > dt_uart_init() to
> >     separate file.
> >   - Introduce new CONFIG_UART_INIT.
> >   - Rename arm_uart_init() to uart_init().
> >   - Add 'select UART_INIT' for CONFIG_ARM to be sure that
> > randconfig won't
> >     disable UART_INIT.
> >   - Update the commit message.
> > ---
> >   MAINTAINERS                  |   2 +-
> >   xen/arch/arm/Kconfig         |   1 +
> >   xen/arch/arm/setup.c         |   2 +-
> >   xen/drivers/char/Kconfig     |  10 +++
> >   xen/drivers/char/Makefile    |   2 +-
> >   xen/drivers/char/arm-uart.c  | 145 ------------------------------
> > -----
> >   xen/drivers/char/uart-init.c | 145
> > +++++++++++++++++++++++++++++++++++
> >   xen/include/xen/serial.h     |   2 +-
> >   8 files changed, 160 insertions(+), 149 deletions(-)
> >   delete mode 100644 xen/drivers/char/arm-uart.c
> >   create mode 100644 xen/drivers/char/uart-init.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 17fc5f9eec..a237080074 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -245,7 +245,6 @@ S:      Supported
> >   L:        xen-devel@xxxxxxxxxxxxxxxxxxxx
> >   F:        docs/misc/arm/
> >   F:        xen/arch/arm/
> > -F: xen/drivers/char/arm-uart.c
> >   F:        xen/drivers/char/cadence-uart.c
> >   F:        xen/drivers/char/exynos4210-uart.c
> >   F:        xen/drivers/char/imx-lpuart.c
> > @@ -254,6 +253,7 @@ F:      xen/drivers/char/mvebu-uart.c
> >   F:        xen/drivers/char/omap-uart.c
> >   F:        xen/drivers/char/pl011.c
> >   F:        xen/drivers/char/scif-uart.c
> > +F: xen/drivers/char/uart-init.c
> 
> (No action needed)
> 
> I think that's fine for now. At some point we will need to consider a
> place where this can be maintained by other arch maintainers because 
> this is not Arm specific anymore. The only place I can think of is
> THE REST.
Based on what we have in THE REST:

   THE REST
   ...
   F:   *
   F:   */
   ...
Doesn't it mean that if we drop "F:     xen/drivers/char/uart-init.c"
then
by default any changes for uart-init.c file will be sent to maintainers
mentioned in M: lines of THE REST for review?
Thereby it seems to me we can just drop the change I did above and drop
"xen/drivers/char/arm-uart.c" from "ARM (W/ VIRTUALISATION EXTENSIONS)
ARCHITECTURE".

> 
> >   F:        xen/drivers/passthrough/arm/
> >   F:        xen/include/public/arch-arm/
> >   F:        xen/include/public/arch-arm.h
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > index 15b2e4a227..e068497361 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -17,6 +17,7 @@ config ARM
> >     select HAS_PASSTHROUGH
> >     select HAS_UBSAN
> >     select IOMMU_FORCE_PT_SHARE
> > +   select UART_INIT
> >   
> >   config ARCH_DEFCONFIG
> >     string
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 71ebaa77ca..2e27af4560 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -361,7 +361,7 @@ void asmlinkage __init start_xen(unsigned long
> > fdt_paddr)
> >   
> >       gic_preinit();
> >   
> > -    arm_uart_init();
> > +    uart_init();
> >       console_init_preirq();
> >       console_init_ring();
> >   
> > diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> > index e175d07c02..49a06a7859 100644
> > --- a/xen/drivers/char/Kconfig
> > +++ b/xen/drivers/char/Kconfig
> > @@ -93,6 +93,16 @@ config SERIAL_TX_BUFSIZE
> >   
> >       Default value is 32768 (32KiB).
> >   
> > +config UART_INIT
> 
> NIT Naming: I would consider to add GENERIC in the same. This makes 
> clearer why x86 doesn't select because they would have their own 
> implementation.
Agree, perhaps GENERIC_UART_INIT would be better. I'll add GENERIC in
the next patch version.

> 
> > +   bool "UART initialization for DT and ACPI"
> 
> Why do we provide a prompt for UART_INIT? This is not something I
> would 
> expect the user to be able to disable.
Agree, not to much sense in provding a promt for UART_INIT. I'll drop
it.

> 
> > +   depends on ARM
> > +   default y
> > +   help
> > +     Provides a generic method for locating UART device tree
> > node when
> > +     device tree is used, or for finding UART information in
> > SPCR
> > +     table when using ACPI. Once UART information is located,
> > +     {acpi}_device_init() is called for UART-specific
> > initialization.
> 
> The last sentence contains too much implementation details. Kconfig
> is 
> meant for admin to know what they need to select. I think it should
> be 
> dropped. That said, if you don't provide any problem, then this
> Kconfig 
> would just be:
> 
> config UART_INIT
> 
> And this is selected by arch.
I don't mind to do in this way. I have only one question shouldn't we
have, at least, bool inside config UART_INIT?
   config UART_INIT
       bool
   
And then as you told in Arm's Kconfig "select UART_INIT" inside
CONFIG_ARM?


Thanks.

~ Oleksii