[PATCH v1 01/16] arm/vpl011: rename virtual PL011 Kconfig option

dmkhn@proton.me posted 16 patches 4 months, 1 week ago
[PATCH v1 01/16] arm/vpl011: rename virtual PL011 Kconfig option
Posted by dmkhn@proton.me 4 months, 1 week ago
From: Denis Mukhin <dmukhin@ford.com> 

Rename CONFIG_SBSA_VUART_CONSOLE to CONFIG_HAS_VUART_PL011.

No functional change.

Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
 xen/arch/arm/Kconfig                  | 2 +-
 xen/arch/arm/Makefile                 | 2 +-
 xen/arch/arm/configs/tiny64_defconfig | 2 +-
 xen/arch/arm/dom0less-build.c         | 4 ++--
 xen/arch/arm/include/asm/domain.h     | 2 +-
 xen/arch/arm/include/asm/vpl011.h     | 2 +-
 xen/drivers/char/console.c            | 4 ++--
 7 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 3f25da3ca5fd..03888569f38c 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -170,7 +170,7 @@ config NEW_VGIC
 	problems with the standard emulation.
 	At the moment this implementation is not security supported.
 
-config SBSA_VUART_CONSOLE
+config HAS_VUART_PL011
 	bool "Emulated SBSA UART console support"
 	default y
 	help
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index ab0a0c2be6d8..2d6787fb03bc 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -65,7 +65,7 @@ obj-$(CONFIG_HAS_ITS) += vgic-v3-its.o
 endif
 obj-$(CONFIG_VM_EVENT) += vm_event.o
 obj-y += vtimer.o
-obj-$(CONFIG_SBSA_VUART_CONSOLE) += vpl011.o
+obj-$(CONFIG_HAS_VUART_PL011) += vpl011.o
 obj-y += vsmc.o
 obj-y += vpsci.o
 obj-$(CONFIG_HWDOM_VUART) += vuart.o
diff --git a/xen/arch/arm/configs/tiny64_defconfig b/xen/arch/arm/configs/tiny64_defconfig
index 469a1eb9f99d..acc227262d81 100644
--- a/xen/arch/arm/configs/tiny64_defconfig
+++ b/xen/arch/arm/configs/tiny64_defconfig
@@ -6,7 +6,7 @@ CONFIG_ARM=y
 #
 # CONFIG_GICV3 is not set
 # CONFIG_VM_EVENT is not set
-# CONFIG_SBSA_VUART_CONSOLE is not set
+# CONFIG_HAS_VUART_PL011 is not set
 
 #
 # Common Features
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 4b285cff5ee2..2a5531a2b892 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -167,7 +167,7 @@ int __init make_intc_domU_node(struct kernel_info *kinfo)
     }
 }
 
-#ifdef CONFIG_SBSA_VUART_CONSOLE
+#ifdef CONFIG_HAS_VUART_PL011
 static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
 {
     void *fdt = kinfo->fdt;
@@ -226,7 +226,7 @@ int __init make_arch_nodes(struct kernel_info *kinfo)
 
     if ( kinfo->arch.vpl011 )
     {
-#ifdef CONFIG_SBSA_VUART_CONSOLE
+#ifdef CONFIG_HAS_VUART_PL011
         ret = make_vpl011_uart_node(kinfo);
 #endif
         if ( ret )
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index a3487ca71303..746ea687d523 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -113,7 +113,7 @@ struct arch_domain
         uint8_t privileged_call_enabled : 1;
     } monitor;
 
-#ifdef CONFIG_SBSA_VUART_CONSOLE
+#ifdef CONFIG_HAS_VUART_PL011
     struct vpl011 vpl011;
 #endif
 
diff --git a/xen/arch/arm/include/asm/vpl011.h b/xen/arch/arm/include/asm/vpl011.h
index cc838682815c..be64883b8628 100644
--- a/xen/arch/arm/include/asm/vpl011.h
+++ b/xen/arch/arm/include/asm/vpl011.h
@@ -65,7 +65,7 @@ struct vpl011_init_info {
     evtchn_port_t evtchn;
 };
 
-#ifdef CONFIG_SBSA_VUART_CONSOLE
+#ifdef CONFIG_HAS_VUART_PL011
 int domain_vpl011_init(struct domain *d,
                        struct vpl011_init_info *info);
 void domain_vpl011_deinit(struct domain *d);
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 5879e31786ba..0f37badc471e 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -37,7 +37,7 @@
 #ifdef CONFIG_X86
 #include <asm/guest.h>
 #endif
-#ifdef CONFIG_SBSA_VUART_CONSOLE
+#ifdef CONFIG_HAS_VUART_PL011
 #include <asm/vpl011.h>
 #endif
 
@@ -606,7 +606,7 @@ static void __serial_rx(char c)
          */
         send_global_virq(VIRQ_CONSOLE);
     }
-#ifdef CONFIG_SBSA_VUART_CONSOLE
+#ifdef CONFIG_HAS_VUART_PL011
     else
         /* Deliver input to the emulated UART. */
         rc = vpl011_rx_char_xen(d, c);
-- 
2.34.1
Re: [PATCH v1 01/16] arm/vpl011: rename virtual PL011 Kconfig option
Posted by Orzel, Michal 4 months, 1 week ago

On 24/06/2025 05:55, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com> 
> 
> Rename CONFIG_SBSA_VUART_CONSOLE to CONFIG_HAS_VUART_PL011.
Why? We emulate SBSA UART and not PL011. Despite the similarities (the former is
a subset of the latter) they are not the same. I find it confusing and drivers
for PL011 might not work with SBSA UART. Also, in the future we may want to
emulate full PL011 in which case it will be even more confusing.

Also, why HAS_?

~Michal
Re: [PATCH v1 01/16] arm/vpl011: rename virtual PL011 Kconfig option
Posted by dmkhn@proton.me 4 months, 1 week ago
On Tue, Jun 24, 2025 at 08:13:08AM +0200, Orzel, Michal wrote:
> 
> 
> On 24/06/2025 05:55, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > Rename CONFIG_SBSA_VUART_CONSOLE to CONFIG_HAS_VUART_PL011.
> Why? We emulate SBSA UART and not PL011. Despite the similarities (the former is
> a subset of the latter) they are not the same. I find it confusing and drivers
> for PL011 might not work with SBSA UART. Also, in the future we may want to
> emulate full PL011 in which case it will be even more confusing.

That's because the emulator is called vpl011, but yes, it is SBSA UART.
I will adjust to SBSA, thanks!

> 
> Also, why HAS_?

My understanding is that HAS_ is the desired naming convention throughout the
code (not documented, though), e.g. all Arm UART drivers are gated by HAS_XXX
(e.g. arch/arm/platforms/Kconfig).

> 
> ~Michal
> 
Re: [PATCH v1 01/16] arm/vpl011: rename virtual PL011 Kconfig option
Posted by Alejandro Vallejo 3 months, 2 weeks ago
On Tue Jun 24, 2025 at 9:24 AM CEST, dmkhn wrote:
> On Tue, Jun 24, 2025 at 08:13:08AM +0200, Orzel, Michal wrote:
>> 
>> 
>> On 24/06/2025 05:55, dmkhn@proton.me wrote:
>> > From: Denis Mukhin <dmukhin@ford.com>
>> >
>> > Rename CONFIG_SBSA_VUART_CONSOLE to CONFIG_HAS_VUART_PL011.
>> Why? We emulate SBSA UART and not PL011. Despite the similarities (the former is
>> a subset of the latter) they are not the same. I find it confusing and drivers
>> for PL011 might not work with SBSA UART. Also, in the future we may want to
>> emulate full PL011 in which case it will be even more confusing.
>
> That's because the emulator is called vpl011, but yes, it is SBSA UART.
> I will adjust to SBSA, thanks!
>
>> 
>> Also, why HAS_?
>
> My understanding is that HAS_ is the desired naming convention throughout the
> code (not documented, though), e.g. all Arm UART drivers are gated by HAS_XXX
> (e.g. arch/arm/platforms/Kconfig).

HAS_ is a non-selectable property dependent on the arch. Think HAS_PCI, or
HAS_EHCI, etc. IOW: HAS_X means "you may implement feature X on this arch",
which is why some options have X and HAS_X variants, where X is selectable
while HAS_X is not.

Cheers,
Alejandro
Re: [PATCH v1 01/16] arm/vpl011: rename virtual PL011 Kconfig option
Posted by dmkhn@proton.me 3 months, 2 weeks ago
On Thu, Jul 17, 2025 at 05:43:22PM +0200, Alejandro Vallejo wrote:
> On Tue Jun 24, 2025 at 9:24 AM CEST, dmkhn wrote:
> > On Tue, Jun 24, 2025 at 08:13:08AM +0200, Orzel, Michal wrote:
> >>
> >>
> >> On 24/06/2025 05:55, dmkhn@proton.me wrote:
> >> > From: Denis Mukhin <dmukhin@ford.com>
> >> >
> >> > Rename CONFIG_SBSA_VUART_CONSOLE to CONFIG_HAS_VUART_PL011.
> >> Why? We emulate SBSA UART and not PL011. Despite the similarities (the former is
> >> a subset of the latter) they are not the same. I find it confusing and drivers
> >> for PL011 might not work with SBSA UART. Also, in the future we may want to
> >> emulate full PL011 in which case it will be even more confusing.
> >
> > That's because the emulator is called vpl011, but yes, it is SBSA UART.
> > I will adjust to SBSA, thanks!
> >
> >>
> >> Also, why HAS_?
> >
> > My understanding is that HAS_ is the desired naming convention throughout the
> > code (not documented, though), e.g. all Arm UART drivers are gated by HAS_XXX
> > (e.g. arch/arm/platforms/Kconfig).
> 
> HAS_ is a non-selectable property dependent on the arch. Think HAS_PCI, or
> HAS_EHCI, etc. IOW: HAS_X means "you may implement feature X on this arch",
> which is why some options have X and HAS_X variants, where X is selectable
> while HAS_X is not.

Thanks; I will fix that.

Jan explained the difference here [1] and [2]:
[1] https://lore.kernel.org/xen-devel/6d33355c-477f-4ef3-8f17-b7f1dd1164ce@suse.com/
[2] https://lore.kernel.org/xen-devel/a63ac9d5-152e-47b0-8169-bf470611c059@suse.com/

It's just there's a lot of drivers (UARTs) which are selectable by the user via
HAS_ symbols (drivers/char/Kconfig)

> 
> Cheers,
> Alejandro
> 
Re: [PATCH v1 01/16] arm/vpl011: rename virtual PL011 Kconfig option
Posted by Alejandro Vallejo 3 months, 2 weeks ago
On Thu Jul 17, 2025 at 9:58 PM CEST, dmkhn wrote:
> On Thu, Jul 17, 2025 at 05:43:22PM +0200, Alejandro Vallejo wrote:
>> On Tue Jun 24, 2025 at 9:24 AM CEST, dmkhn wrote:
>> > On Tue, Jun 24, 2025 at 08:13:08AM +0200, Orzel, Michal wrote:
>> >>
>> >>
>> >> On 24/06/2025 05:55, dmkhn@proton.me wrote:
>> >> > From: Denis Mukhin <dmukhin@ford.com>
>> >> >
>> >> > Rename CONFIG_SBSA_VUART_CONSOLE to CONFIG_HAS_VUART_PL011.
>> >> Why? We emulate SBSA UART and not PL011. Despite the similarities (the former is
>> >> a subset of the latter) they are not the same. I find it confusing and drivers
>> >> for PL011 might not work with SBSA UART. Also, in the future we may want to
>> >> emulate full PL011 in which case it will be even more confusing.
>> >
>> > That's because the emulator is called vpl011, but yes, it is SBSA UART.
>> > I will adjust to SBSA, thanks!
>> >
>> >>
>> >> Also, why HAS_?
>> >
>> > My understanding is that HAS_ is the desired naming convention throughout the
>> > code (not documented, though), e.g. all Arm UART drivers are gated by HAS_XXX
>> > (e.g. arch/arm/platforms/Kconfig).
>> 
>> HAS_ is a non-selectable property dependent on the arch. Think HAS_PCI, or
>> HAS_EHCI, etc. IOW: HAS_X means "you may implement feature X on this arch",
>> which is why some options have X and HAS_X variants, where X is selectable
>> while HAS_X is not.
>
> Thanks; I will fix that.
>
> Jan explained the difference here [1] and [2]:
> [1] https://lore.kernel.org/xen-devel/6d33355c-477f-4ef3-8f17-b7f1dd1164ce@suse.com/
> [2] https://lore.kernel.org/xen-devel/a63ac9d5-152e-47b0-8169-bf470611c059@suse.com/
>
> It's just there's a lot of drivers (UARTs) which are selectable by the user via
> HAS_ symbols (drivers/char/Kconfig)

IMO, HAS_IMX_LPUART should be selected by ARM_64, then IMX_LPUART would depend
on HAS_IMX_LPUART. I don't know how those (and you're right, there's lots) got
there. Same with NS16550, etc.

That allows to decouple the device from the architectures where it might be
found.

Cheers,
Alejandro