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

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

Posted by Oleksii Kurochko 1 year, 2 months ago

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@gmail.com>
---
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@lists.xenproject.org
 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
 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
+	bool "UART initialization for DT and ACPI"
+	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.
+
 config XHCI
 	bool "XHCI DbC UART driver"
 	depends on X86
diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
index d3b987da1d..74dcde7e57 100644
--- a/xen/drivers/char/Makefile
+++ b/xen/drivers/char/Makefile
@@ -11,7 +11,7 @@ obj-$(CONFIG_HAS_EHCI) += ehci-dbgp.o
 obj-$(CONFIG_XHCI) += xhci-dbc.o
 obj-$(CONFIG_HAS_IMX_LPUART) += imx-lpuart.o
 obj-$(CONFIG_HAS_LINFLEX) += linflex-uart.o
-obj-$(CONFIG_ARM) += arm-uart.o
+obj-$(CONFIG_UART_INIT) += uart-init.o
 obj-y += serial.o
 obj-$(CONFIG_XEN_GUEST) += xen_pv_console.o
 obj-$(CONFIG_PV_SHIM) += consoled.o
diff --git a/xen/drivers/char/arm-uart.c b/xen/drivers/char/arm-uart.c
deleted file mode 100644
index 91f13a4136..0000000000
--- a/xen/drivers/char/arm-uart.c
+++ /dev/null
@@ -1,145 +0,0 @@
-/*
- * xen/drivers/char/arm-uart.c
- *
- * Generic uart retrieved via the device tree or ACPI
- *
- * Julien Grall <julien.grall@linaro.org>
- * Copyright (c) 2013 Linaro Limited.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#include <asm/device.h>
-
-#include <xen/console.h>
-#include <xen/device_tree.h>
-#include <xen/param.h>
-#include <xen/serial.h>
-#include <xen/errno.h>
-#include <xen/acpi.h>
-
-/*
- * Configure UART port with a string:
- * path:options
- *
- * @path: full path used in the device tree for the UART. If the path
- * doesn't start with '/', we assuming that it's an alias.
- * @options: UART speficic options (see in each UART driver)
- */
-static char __initdata opt_dtuart[256] = "";
-string_param("dtuart", opt_dtuart);
-
-static void __init dt_uart_init(void)
-{
-    struct dt_device_node *dev;
-    int ret;
-    const char *devpath = opt_dtuart;
-    const char *options;
-    char *split;
-
-    if ( !console_has("dtuart") )
-        return; /* Not for us */
-
-    if ( !strcmp(opt_dtuart, "") )
-    {
-        const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
-
-        if ( chosen )
-        {
-            const char *stdout;
-
-            ret = dt_property_read_string(chosen, "stdout-path", &stdout);
-            if ( ret >= 0 )
-            {
-                printk("Taking dtuart configuration from /chosen/stdout-path\n");
-                if ( strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart))
-                     >= sizeof(opt_dtuart) )
-                    printk("WARNING: /chosen/stdout-path too long, truncated\n");
-            }
-            else if ( ret != -EINVAL /* Not present */ )
-                printk("Failed to read /chosen/stdout-path (%d)\n", ret);
-        }
-    }
-
-    if ( !strcmp(opt_dtuart, "") )
-    {
-        printk("No dtuart path configured\n");
-        return;
-    }
-
-    split = strchr(opt_dtuart, ':');
-    if ( split )
-    {
-        split[0] = '\0';
-        options = split + 1;
-    }
-    else
-        options = "";
-
-    printk("Looking for dtuart at \"%s\", options \"%s\"\n", devpath, options);
-    if ( *devpath == '/' )
-        dev = dt_find_node_by_path(devpath);
-    else
-        dev = dt_find_node_by_alias(devpath);
-
-    if ( !dev )
-    {
-        printk("Unable to find device \"%s\"\n", devpath);
-        return;
-    }
-
-    ret = device_init(dev, DEVICE_SERIAL, options);
-
-    if ( ret )
-        printk("Unable to initialize dtuart: %d\n", ret);
-}
-
-#ifdef CONFIG_ACPI
-static void __init acpi_uart_init(void)
-{
-    struct acpi_table_spcr *spcr = NULL;
-    int ret;
-
-    acpi_get_table(ACPI_SIG_SPCR, 0, (struct acpi_table_header **)&spcr);
-
-    if ( spcr == NULL )
-    {
-        printk("Unable to get spcr table\n");
-    }
-    else
-    {
-        ret = acpi_device_init(DEVICE_SERIAL, NULL, spcr->interface_type);
-
-        if ( ret )
-            printk("Unable to initialize acpi uart: %d\n", ret);
-    }
-}
-#else
-static void __init acpi_uart_init(void) { }
-#endif
-
-void __init arm_uart_init(void)
-{
-    if ( acpi_disabled )
-        dt_uart_init();
-    else
-        acpi_uart_init();
-}
-
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * tab-width: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/drivers/char/uart-init.c b/xen/drivers/char/uart-init.c
new file mode 100644
index 0000000000..7f3b385308
--- /dev/null
+++ b/xen/drivers/char/uart-init.c
@@ -0,0 +1,145 @@
+/*
+ * xen/drivers/char/arm-uart.c
+ *
+ * Generic uart retrieved via the device tree or ACPI
+ *
+ * Julien Grall <julien.grall@linaro.org>
+ * Copyright (c) 2013 Linaro Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <asm/device.h>
+
+#include <xen/console.h>
+#include <xen/device_tree.h>
+#include <xen/param.h>
+#include <xen/serial.h>
+#include <xen/errno.h>
+#include <xen/acpi.h>
+
+/*
+ * Configure UART port with a string:
+ * path:options
+ *
+ * @path: full path used in the device tree for the UART. If the path
+ * doesn't start with '/', we assuming that it's an alias.
+ * @options: UART speficic options (see in each UART driver)
+ */
+static char __initdata opt_dtuart[256] = "";
+string_param("dtuart", opt_dtuart);
+
+static void __init dt_uart_init(void)
+{
+    struct dt_device_node *dev;
+    int ret;
+    const char *devpath = opt_dtuart;
+    const char *options;
+    char *split;
+
+    if ( !console_has("dtuart") )
+        return; /* Not for us */
+
+    if ( !strcmp(opt_dtuart, "") )
+    {
+        const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
+
+        if ( chosen )
+        {
+            const char *stdout;
+
+            ret = dt_property_read_string(chosen, "stdout-path", &stdout);
+            if ( ret >= 0 )
+            {
+                printk("Taking dtuart configuration from /chosen/stdout-path\n");
+                if ( strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart))
+                     >= sizeof(opt_dtuart) )
+                    printk("WARNING: /chosen/stdout-path too long, truncated\n");
+            }
+            else if ( ret != -EINVAL /* Not present */ )
+                printk("Failed to read /chosen/stdout-path (%d)\n", ret);
+        }
+    }
+
+    if ( !strcmp(opt_dtuart, "") )
+    {
+        printk("No dtuart path configured\n");
+        return;
+    }
+
+    split = strchr(opt_dtuart, ':');
+    if ( split )
+    {
+        split[0] = '\0';
+        options = split + 1;
+    }
+    else
+        options = "";
+
+    printk("Looking for dtuart at \"%s\", options \"%s\"\n", devpath, options);
+    if ( *devpath == '/' )
+        dev = dt_find_node_by_path(devpath);
+    else
+        dev = dt_find_node_by_alias(devpath);
+
+    if ( !dev )
+    {
+        printk("Unable to find device \"%s\"\n", devpath);
+        return;
+    }
+
+    ret = device_init(dev, DEVICE_SERIAL, options);
+
+    if ( ret )
+        printk("Unable to initialize dtuart: %d\n", ret);
+}
+
+#ifdef CONFIG_ACPI
+static void __init acpi_uart_init(void)
+{
+    struct acpi_table_spcr *spcr = NULL;
+    int ret;
+
+    acpi_get_table(ACPI_SIG_SPCR, 0, (struct acpi_table_header **)&spcr);
+
+    if ( spcr == NULL )
+    {
+        printk("Unable to get spcr table\n");
+    }
+    else
+    {
+        ret = acpi_device_init(DEVICE_SERIAL, NULL, spcr->interface_type);
+
+        if ( ret )
+            printk("Unable to initialize acpi uart: %d\n", ret);
+    }
+}
+#else
+static void __init acpi_uart_init(void) { }
+#endif
+
+void __init uart_init(void)
+{
+    if ( acpi_disabled )
+        dt_uart_init();
+    else
+        acpi_uart_init();
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index 3d21207a3d..63a82b032d 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -166,7 +166,7 @@ void xhci_dbc_uart_init(void);
 static void inline xhci_dbc_uart_init(void) {}
 #endif
 
-void arm_uart_init(void);
+void uart_init(void);
 
 struct physdev_dbgp_op;
 int dbgp_op(const struct physdev_dbgp_op *op);
-- 
2.47.0

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

Posted by Julien Grall 1 year, 2 months ago

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@gmail.com>
> ---
> 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@lists.xenproject.org
>   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.

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

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

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

> +
>   config XHCI
>   	bool "XHCI DbC UART driver"
>   	depends on X86

[...]

> diff --git a/xen/drivers/char/uart-init.c b/xen/drivers/char/uart-init.c
> new file mode 100644
> index 0000000000..7f3b385308
> --- /dev/null
> +++ b/xen/drivers/char/uart-init.c
> @@ -0,0 +1,145 @@
> +/*
> + * xen/drivers/char/arm-uart.c
> + *
> + * Generic uart retrieved via the device tree or ACPI
> + *
> + * Julien Grall <julien.grall@linaro.org>
> + * Copyright (c) 2013 Linaro Limited.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

NIT: As you are updating the file, would you be able to convert the 
license to SPDX. For this one, this will need to be:

/* SPDX-License-Identifier: GPL-2.0-or-later */

Cheers,

-- 
Julien Grall

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

Posted by oleksii.kurochko@gmail.com 1 year, 2 months ago

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@gmail.com>
> > ---
> > 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@lists.xenproject.org
> >   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

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

Posted by Julien Grall 1 year, 2 months ago

Hi Oleksii,

On 19/11/2024 09:43, oleksii.kurochko@gmail.com wrote:
> 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@gmail.com>
>>> ---
>>> 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@lists.xenproject.org
>>>    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".

Yes it should. You can use scripts/get_maintainers.pl to confirm who 
will be CCed.

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

Yes it should.

>     
> And then as you told in Arm's Kconfig "select UART_INIT" inside
> CONFIG_ARM?

Correct.

Cheers,

-- 
Julien Grall