[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] drivers/char: rename arm-uart.c to uart-init.c
Hi Oleksii,
On 19/11/2024 09:43, oleksii.kurochko@xxxxxxxxx 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@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".
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
|
