[PATCH v4 1/1] ns16550: add support for polling mode when device tree is used

[PATCH v4 1/1] ns16550: add support for polling mode when device tree is used

[Oleksii Kurochko]

RISC-V doesn't support interrupts for the time being, so it would be nice to
have polling mode.

To force poll mode it was added parsing of new 'poll' property under "com<N>="

If there is no interrupt property in UART node, then polling will be used.
According to 4.2.2 National Semiconductor 16450/16550 Compatible UART
Requirements from The Devicetree Specification v0.4
( https://www.devicetree.org/specifications/ ) the interrupt property is
optional.

Also, it is possible that interrupt '0' can be used for some architectures as
an legal UART interrupt number ( according to dts files in Linux kernel ), so
the check of the return value of platform_get_irq() was updated in case of when
device tree is used for UART initialization.
For example:
https://github.com/torvalds/linux/blob/master/arch/powerpc/boot/dts/ebony.dts#L197

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V4:
 - fix code style for ns16550_irq().
 - partially revert changes in pci_uart_config().
 - add check of "poll" property under "com<N>=" instead of "console=".
 - remove seting of polling mode in parsing of msi in parse_positional().
 - update comment above opt_com1 about Configuration string of serial port.
---
 xen/drivers/char/ns16550.c | 64 +++++++++++++++++++++++++-------------
 1 file changed, 42 insertions(+), 22 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 2aed6ec707..83170fd6b8 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -58,7 +58,11 @@ static struct ns16550 {
     struct timer timer;
     struct timer resume_timer;
     unsigned int timeout_ms;
-    bool_t intr_works;
+    enum {
+        intr_off,
+        intr_on,
+        polling,
+    } intr_works;
     bool_t dw_usr_bsy;
 #ifdef NS16550_PCI
     /* PCI card parameters. */
@@ -112,6 +116,19 @@ struct ns16550_config_param {
 static void enable_exar_enhanced_bits(const struct ns16550 *uart);
 #endif
 
+/*
+ * Configure serial port with a string:
+ *   <baud>[/<base_baud>][,DPS[,<io-base>[,<irq>|msi|poll[,<port-bdf>[,<bridge-bdf>]]]]].
+ * The tail of the string can be omitted if platform defaults are sufficient.
+ * If the baud rate is pre-configured, perhaps by a bootloader, then 'auto'
+ * can be specified in place of a numeric baud rate. Polled mode is specified
+ * by poll property.
+ */
+static char __initdata opt_com1[128] = "";
+static char __initdata opt_com2[128] = "";
+string_param("com1", opt_com1);
+string_param("com2", opt_com2);
+
 static void cf_check ns16550_delayed_resume(void *data);
 
 static u8 ns_read_reg(const struct ns16550 *uart, unsigned int reg)
@@ -181,7 +198,7 @@ static void cf_check ns16550_interrupt(
     struct serial_port *port = dev_id;
     struct ns16550 *uart = port->uart;
 
-    uart->intr_works = 1;
+    uart->intr_works = intr_on;
 
     while ( !(ns_read_reg(uart, UART_IIR) & UART_IIR_NOINT) )
     {
@@ -212,7 +229,7 @@ static void cf_check __ns16550_poll(struct cpu_user_regs *regs)
     struct serial_port *port = this_cpu(poll_port);
     struct ns16550 *uart = port->uart;
 
-    if ( uart->intr_works )
+    if ( uart->intr_works == intr_on )
         return; /* Interrupts work - no more polling */
 
     while ( ns_read_reg(uart, UART_LSR) & UART_LSR_DR )
@@ -305,7 +322,8 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
     unsigned char lcr;
     unsigned int  divisor;
 
-    uart->intr_works = 0;
+    if ( uart->intr_works != polling )
+        uart->intr_works = intr_off;
 
     pci_serial_early_init(uart);
 
@@ -394,7 +412,7 @@ static void __init cf_check ns16550_init_irq(struct serial_port *port)
 
 static void ns16550_setup_postirq(struct ns16550 *uart)
 {
-    if ( uart->irq > 0 )
+    if ( uart->intr_works != polling )
     {
         /* Master interrupt enable; also keep DTR/RTS asserted. */
         ns_write_reg(uart,
@@ -473,6 +491,7 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)
                 if ( rc )
                 {
                     uart->irq = 0;
+                    uart->intr_works = polling;
                     if ( msi_desc )
                         msi_free_irq(msi_desc);
                     else
@@ -488,7 +507,7 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)
     }
 #endif
 
-    if ( uart->irq > 0 )
+    if ( uart->intr_works != polling )
     {
         uart->irqaction.handler = ns16550_interrupt;
         uart->irqaction.name    = "ns16550";
@@ -595,7 +614,8 @@ static void __init cf_check ns16550_endboot(struct serial_port *port)
 static int __init cf_check ns16550_irq(struct serial_port *port)
 {
     struct ns16550 *uart = port->uart;
-    return ((uart->irq > 0) ? uart->irq : -1);
+
+    return ((uart->intr_works != polling) ? uart->irq : -1);
 }
 
 static void cf_check ns16550_start_tx(struct serial_port *port)
@@ -654,6 +674,9 @@ static void ns16550_init_common(struct ns16550 *uart)
 
     /* Default lsr_mask = UART_LSR_THRE */
     uart->lsr_mask  = UART_LSR_THRE;
+
+    if ( strstr(opt_com1, "poll") || strstr(opt_com2, "poll") )
+        uart->intr_works = polling;
 }
 
 #ifdef CONFIG_X86
@@ -1333,9 +1356,13 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
                     uart->irq = 0;
 #endif
                 if ( !uart->irq )
+                {
+                    uart->intr_works = polling;
+
                     printk(XENLOG_INFO
                            "ns16550: %pp: no legacy IRQ, using poll mode\n",
                            &PCI_SBDF(0, b, d, f));
+                }
 
                 return 0;
             }
@@ -1374,19 +1401,6 @@ static void enable_exar_enhanced_bits(const struct ns16550 *uart)
 
 #endif /* CONFIG_HAS_PCI */
 
-/*
- * Configure serial port with a string:
- *   <baud>[/<base_baud>][,DPS[,<io-base>[,<irq>[,<port-bdf>[,<bridge-bdf>]]]]].
- * The tail of the string can be omitted if platform defaults are sufficient.
- * If the baud rate is pre-configured, perhaps by a bootloader, then 'auto'
- * can be specified in place of a numeric baud rate. Polled mode is specified
- * by requesting irq 0.
- */
-static char __initdata opt_com1[128] = "";
-static char __initdata opt_com2[128] = "";
-string_param("com1", opt_com1);
-string_param("com2", opt_com2);
-
 enum serial_param_type {
     baud,
     clock_hz,
@@ -1555,6 +1569,9 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
         }
         else
 #endif
+        if ( strncmp(conf, "poll", 4) == 0 )
+            uart->intr_works = polling;
+        else
             uart->irq = simple_strtol(conf, &conf, 10);
     }
 
@@ -1791,8 +1808,11 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
     }
 
     res = platform_get_irq(dev, 0);
-    if ( ! res )
-        return -EINVAL;
+    if ( res < 0 )
+    {
+        printk("there is no interrupt property, polling will be used\n");
+        uart->intr_works = polling;
+    }
     uart->irq = res;
 
     uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart");
-- 
2.41.0

Re: [PATCH v4 1/1] ns16550: add support for polling mode when device tree is used

[Jan Beulich]

On 27.07.2023 18:45, Oleksii Kurochko wrote:
> @@ -654,6 +674,9 @@ static void ns16550_init_common(struct ns16550 *uart)
>  
>      /* Default lsr_mask = UART_LSR_THRE */
>      uart->lsr_mask  = UART_LSR_THRE;
> +
> +    if ( strstr(opt_com1, "poll") || strstr(opt_com2, "poll") )
> +        uart->intr_works = polling;
>  }

A non-__init function may not reference __initdata objects. But strstr()
is too lax anyway, and you also shouldn't check the wrong port's options.
You want to recognize "poll" _only_ where all other command line options
are processed.

Also may I remind you that extending command line options requires their
doc to also be updated?

> @@ -1333,9 +1356,13 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
>                      uart->irq = 0;
>  #endif
>                  if ( !uart->irq )
> +                {
> +                    uart->intr_works = polling;
> +
>                      printk(XENLOG_INFO
>                             "ns16550: %pp: no legacy IRQ, using poll mode\n",
>                             &PCI_SBDF(0, b, d, f));
> +                }

I'm okay to leave it at this for now, since this way at least nothing
regresses that was working before. I'm not convinced this is all
correct, but that's a largely separate (and pre-existing) issue then.

> @@ -1791,8 +1808,11 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
>      }
>  
>      res = platform_get_irq(dev, 0);
> -    if ( ! res )
> -        return -EINVAL;
> +    if ( res < 0 )
> +    {
> +        printk("there is no interrupt property, polling will be used\n");
> +        uart->intr_works = polling;
> +    }
>      uart->irq = res;

Shouldn't you avoid writing uart->irq when res is negative?

Jan

Re: [PATCH v4 1/1] ns16550: add support for polling mode when device tree is used

[Oleksii]

On Mon, 2023-07-31 at 15:24 +0200, Jan Beulich wrote:
> On 27.07.2023 18:45, Oleksii Kurochko wrote:
> > @@ -654,6 +674,9 @@ static void ns16550_init_common(struct ns16550
> > *uart)
> >  
> >      /* Default lsr_mask = UART_LSR_THRE */
> >      uart->lsr_mask  = UART_LSR_THRE;
> > +
> > +    if ( strstr(opt_com1, "poll") || strstr(opt_com2, "poll") )
> > +        uart->intr_works = polling;
> >  }
> 
> A non-__init function may not reference __initdata objects. But
> strstr()
> is too lax anyway, and you also shouldn't check the wrong port's
> options.
> You want to recognize "poll" _only_ where all other command line
> options
> are processed.
Just to confirm, do you mean that I should use parse_positional() ( or
something similar ) for the device-tree method of initializing ns16550?

I checked the polling behavior as described above because
parse_positional() is utilized solely for X86.

It appears that command line options are parsed only in the case of
x86.
> 
> Also may I remind you that extending command line options requires
> their
> doc to also be updated?
Thank you for the reminder. It seems I misunderstood which doc I need
to update. I believed you were referring to the comment above the
declaration of opt_com1[].

I think you meant xen-command-line.pandoc.
> 
> > @@ -1333,9 +1356,13 @@ pci_uart_config(struct ns16550 *uart, bool_t
> > skip_amt, unsigned int idx)
> >                      uart->irq = 0;
> >  #endif
> >                  if ( !uart->irq )
> > +                {
> > +                    uart->intr_works = polling;
> > +
> >                      printk(XENLOG_INFO
> >                             "ns16550: %pp: no legacy IRQ, using
> > poll mode\n",
> >                             &PCI_SBDF(0, b, d, f));
> > +                }
> 
> I'm okay to leave it at this for now, since this way at least nothing
> regresses that was working before. I'm not convinced this is all
> correct, but that's a largely separate (and pre-existing) issue then.
> 
> > @@ -1791,8 +1808,11 @@ static int __init
> > ns16550_uart_dt_init(struct dt_device_node *dev,
> >      }
> >  
> >      res = platform_get_irq(dev, 0);
> > -    if ( ! res )
> > -        return -EINVAL;
> > +    if ( res < 0 )
> > +    {
> > +        printk("there is no interrupt property, polling will be
> > used\n");
> > +        uart->intr_works = polling;
> > +    }
> >      uart->irq = res;
> 
> Shouldn't you avoid writing uart->irq when res is negative?
I think you are right, and I missed that. I'll update it in a new patch
version.

~ Oleksii

Re: [PATCH v4 1/1] ns16550: add support for polling mode when device tree is used

[Jan Beulich]

On 01.08.2023 11:46, Oleksii wrote:
> On Mon, 2023-07-31 at 15:24 +0200, Jan Beulich wrote:
>> On 27.07.2023 18:45, Oleksii Kurochko wrote:
>>> @@ -654,6 +674,9 @@ static void ns16550_init_common(struct ns16550
>>> *uart)
>>>  
>>>      /* Default lsr_mask = UART_LSR_THRE */
>>>      uart->lsr_mask  = UART_LSR_THRE;
>>> +
>>> +    if ( strstr(opt_com1, "poll") || strstr(opt_com2, "poll") )
>>> +        uart->intr_works = polling;
>>>  }
>>
>> A non-__init function may not reference __initdata objects. But
>> strstr()
>> is too lax anyway, and you also shouldn't check the wrong port's
>> options.
>> You want to recognize "poll" _only_ where all other command line
>> options
>> are processed.
> Just to confirm, do you mean that I should use parse_positional() ( or
> something similar ) for the device-tree method of initializing ns16550?
> 
> I checked the polling behavior as described above because
> parse_positional() is utilized solely for X86.
> 
> It appears that command line options are parsed only in the case of
> x86.

Hmm, looks like you're right. Arm folks will want to clarify how they
got away without any command line overrides so far, and how they think
now necessary ones should be suitably added. I recall I had reservations
when the file was massaged to compile out supposedly x86-only code.

Jan

Re: [PATCH v4 1/1] ns16550: add support for polling mode when device tree is used

[Julien Grall]

Hi,

On 01/08/2023 10:59, Jan Beulich wrote:
> On 01.08.2023 11:46, Oleksii wrote:
>> On Mon, 2023-07-31 at 15:24 +0200, Jan Beulich wrote:
>>> On 27.07.2023 18:45, Oleksii Kurochko wrote:
>>>> @@ -654,6 +674,9 @@ static void ns16550_init_common(struct ns16550
>>>> *uart)
>>>>   
>>>>       /* Default lsr_mask = UART_LSR_THRE */
>>>>       uart->lsr_mask  = UART_LSR_THRE;
>>>> +
>>>> +    if ( strstr(opt_com1, "poll") || strstr(opt_com2, "poll") )
>>>> +        uart->intr_works = polling;
>>>>   }
>>>
>>> A non-__init function may not reference __initdata objects. But
>>> strstr()
>>> is too lax anyway, and you also shouldn't check the wrong port's
>>> options.
>>> You want to recognize "poll" _only_ where all other command line
>>> options
>>> are processed.
>> Just to confirm, do you mean that I should use parse_positional() ( or
>> something similar ) for the device-tree method of initializing ns16550?
>>
>> I checked the polling behavior as described above because
>> parse_positional() is utilized solely for X86.
>>
>> It appears that command line options are parsed only in the case of
>> x86.
> 
> Hmm, looks like you're right. Arm folks will want to clarify how they
> got away without any command line overrides so far,

So far, everything we needed was described in the firmware table (e.g. 
ACPI/Device-Tree).

> and how they think
> now necessary ones should be suitably added.

I think "poll" is the only parameter that we should parse on Arm.

> I recall I had reservations
> when the file was massaged to compile out supposedly x86-only code.

I had a look at the code protected by CONFIG_X86. Most of it still want 
to be configured behind CONFIG_X86 at least until we have PCI support. 
But even with it, some work would be necessary to be able to support PCI 
UART.

Cheers,

-- 
Julien Grall