[PATCH v2 1/2] ns16550: do not override fifo size if explicitly set

[PATCH v2 1/2] ns16550: do not override fifo size if explicitly set

[Marek Marczykowski-Górecki]

If fifo size is already set via uart_params, do not force it to 16 - which
may not match the actual hardware. Specifically Exar cards have fifo of
256 bytes.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/drivers/char/ns16550.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 16a73d0c0e47..97b85b0225cc 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -357,7 +357,8 @@ static void __init ns16550_init_preirq(struct serial_port *port)
     ns16550_setup_preirq(uart);
 
     /* Check this really is a 16550+. Otherwise we have no FIFOs. */
-    if ( ((ns_read_reg(uart, UART_IIR) & 0xc0) == 0xc0) &&
+    if ( uart->fifo_size <= 1 &&
+         ((ns_read_reg(uart, UART_IIR) & 0xc0) == 0xc0) &&
          ((ns_read_reg(uart, UART_FCR) & UART_FCR_TRG14) == UART_FCR_TRG14) )
         uart->fifo_size = 16;
 }
-- 
2.31.1

[PATCH v2 2/2] ns16550: add Exar PCIe UART cards support

[Marek Marczykowski-Górecki]

Besides standard UART setup, this device needs enabling
(vendor-specific) "Enhanced Control Bits" - otherwise disabling hardware
control flow (MCR[2]) is ignored. Add appropriate quirk to the
ns16550_setup_preirq(), similar to the handle_dw_usr_busy_quirk(). The
new function act on Exar 2-, 4-, and 8- port cards only. I have tested
the functionality on 2-port card but based on the Linux driver, the same
applies to other models too.

Additionally, Exar card supports fractional divisor (DLD[3:0] register,
at 0x02). This part is not supported here yet, and seems to not
be required for working 115200bps at the very least.

The specification for the 2-port card is available at:
https://www.maxlinear.com/product/interface/uarts/pcie-uarts/xr17v352

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v2:
 - use read-modify-write for enabling ECB
 - handle also 4- and 8-ports cards (but not everything from Exar)
 - formatting fixes
---
 xen/drivers/char/ns16550.c  | 81 ++++++++++++++++++++++++++++++++++++-
 xen/include/xen/8250-uart.h |  5 +++
 xen/include/xen/pci_ids.h   |  2 +
 3 files changed, 86 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 97b85b0225cc..27501f28aa7b 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -88,6 +88,9 @@ struct ns16550_config {
         param_pericom_2port,
         param_pericom_4port,
         param_pericom_8port,
+        param_exar_xr17v352,
+        param_exar_xr17v354,
+        param_exar_xr17v358,
     } param;
 };
 
@@ -169,6 +172,29 @@ static void handle_dw_usr_busy_quirk(struct ns16550 *uart)
     }
 }
 
+static void enable_exar_enhanced_bits(struct ns16550 *uart)
+{
+#ifdef NS16550_PCI
+    if ( uart->bar &&
+         pci_conf_read16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[2],
+                         uart->ps_bdf[2]), PCI_VENDOR_ID) == PCI_VENDOR_ID_EXAR )
+    {
+        u8 devid = ns_read_reg(uart, UART_XR_DVID);
+        u8 efr;
+        /*
+         * Exar XR17V35x cards ignore setting MCR[2] (hardware flow control)
+         * unless "Enhanced control bits" is enabled.
+         * The below checks for a 2, 4 or 8 port UART, following Linux driver.
+         */
+        if ( devid == 0x82 || devid == 0x84 || devid == 0x88 ) {
+            efr = ns_read_reg(uart, UART_XR_EFR);
+            efr |= UART_EFR_ECB;
+            ns_write_reg(uart, UART_XR_EFR, efr);
+        }
+    }
+#endif
+}
+
 static void ns16550_interrupt(
     int irq, void *dev_id, struct cpu_user_regs *regs)
 {
@@ -303,6 +329,9 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
     /* Handle the DesignWare 8250 'busy-detect' quirk. */
     handle_dw_usr_busy_quirk(uart);
 
+    /* Enable Exar "Enhanced function bits" */
+    enable_exar_enhanced_bits(uart);
+
     /* Line control and baud-rate generator. */
     ns_write_reg(uart, UART_LCR, lcr | UART_LCR_DLAB);
     if ( uart->baud != BAUD_AUTO )
@@ -781,7 +810,37 @@ static const struct ns16550_config_param __initconst uart_param[] = {
         .lsr_mask = UART_LSR_THRE,
         .bar0 = 1,
         .max_ports = 8,
-    }
+    },
+    [param_exar_xr17v352] = {
+        .base_baud = 7812500,
+        .uart_offset = 0x400,
+        .reg_width = 1,
+        .fifo_size = 256,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .mmio = 1,
+        .max_ports = 2,
+    },
+    [param_exar_xr17v354] = {
+        .base_baud = 7812500,
+        .uart_offset = 0x400,
+        .reg_width = 1,
+        .fifo_size = 256,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .mmio = 1,
+        .max_ports = 4,
+    },
+    [param_exar_xr17v358] = {
+        .base_baud = 7812500,
+        .uart_offset = 0x400,
+        .reg_width = 1,
+        .fifo_size = 256,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .mmio = 1,
+        .max_ports = 8,
+    },
 };
 
 static const struct ns16550_config __initconst uart_config[] =
@@ -1007,7 +1066,25 @@ static const struct ns16550_config __initconst uart_config[] =
         .vendor_id = PCI_VENDOR_ID_PERICOM,
         .dev_id = 0x7958,
         .param = param_pericom_8port
-    }
+    },
+    /* Exar Corp. XR17V352 Dual PCIe UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_EXAR,
+        .dev_id = 0x0352,
+        .param = param_exar_xr17v352
+    },
+    /* Exar Corp. XR17V354 Quad PCIe UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_EXAR,
+        .dev_id = 0x0354,
+        .param = param_exar_xr17v354
+    },
+    /* Exar Corp. XR17V358 Octal PCIe UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_EXAR,
+        .dev_id = 0x0358,
+        .param = param_exar_xr17v358
+    },
 };
 
 static int __init
diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
index 5c3bac33221e..74e9d552a718 100644
--- a/xen/include/xen/8250-uart.h
+++ b/xen/include/xen/8250-uart.h
@@ -35,6 +35,8 @@
 #define UART_USR          0x1f    /* Status register (DW) */
 #define UART_DLL          0x00    /* divisor latch (ls) (DLAB=1) */
 #define UART_DLM          0x01    /* divisor latch (ms) (DLAB=1) */
+#define UART_XR_EFR       0x09    /* Enhanced function register (Exar) */
+#define UART_XR_DVID      0x8d    /* Device identification */
 
 /* Interrupt Enable Register */
 #define UART_IER_ERDAI    0x01    /* rx data recv'd       */
@@ -121,6 +123,9 @@
 /* Frequency of external clock source. This definition assumes PC platform. */
 #define UART_CLOCK_HZ     1843200
 
+/* Bits in Exar specific UART_XR_EFR register */
+#define UART_EFR_ECB      0x10
+
 /* Resume retry settings */
 #define RESUME_DELAY      MILLISECS(10)
 #define RESUME_RETRIES    100
diff --git a/xen/include/xen/pci_ids.h b/xen/include/xen/pci_ids.h
index 7788ba9d2f34..e798477a7e23 100644
--- a/xen/include/xen/pci_ids.h
+++ b/xen/include/xen/pci_ids.h
@@ -4,6 +4,8 @@
 
 #define PCI_VENDOR_ID_PERICOM            0x12d8
 
+#define PCI_VENDOR_ID_EXAR               0x13a8
+
 #define PCI_VENDOR_ID_OXSEMI             0x1415
 
 #define PCI_VENDOR_ID_BROADCOM           0x14e4
-- 
2.31.1

Re: [PATCH v2 2/2] ns16550: add Exar PCIe UART cards support

[Jan Beulich]

On 18.08.2021 14:16, Marek Marczykowski-Górecki wrote:
> @@ -169,6 +172,29 @@ static void handle_dw_usr_busy_quirk(struct ns16550 *uart)
>      }
>  }
>  
> +static void enable_exar_enhanced_bits(struct ns16550 *uart)

Afaics the parameter can be pointer-to-const.

> +{
> +#ifdef NS16550_PCI
> +    if ( uart->bar &&
> +         pci_conf_read16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[2],
> +                         uart->ps_bdf[2]), PCI_VENDOR_ID) == PCI_VENDOR_ID_EXAR )
> +    {
> +        u8 devid = ns_read_reg(uart, UART_XR_DVID);
> +        u8 efr;
> +        /*
> +         * Exar XR17V35x cards ignore setting MCR[2] (hardware flow control)
> +         * unless "Enhanced control bits" is enabled.
> +         * The below checks for a 2, 4 or 8 port UART, following Linux driver.
> +         */
> +        if ( devid == 0x82 || devid == 0x84 || devid == 0x88 ) {

Hmm, now I'm in trouble as to a question you did ask earlier (once
on irc and I think also once in email): You asked whether to use
the PCI device ID _or_ the DVID register. Now you're using both,
albeit in a way not really making the access here safe: You assume
that you can access the byte at offset 0x8d on all Exar devices. I
don't know whether there is anything written anywhere telling you
this is safe. With the earlier vendor/device ID match, it would feel
safer to me if you replaced vendor ID and DVID checks here by a
check of uart->param: While you must not deref that pointer, you can
still check that it points at one of the three new entries you add
to uart_param[]. Perhaps via "switch ( uart->param - uart_param )".

Also there are a number of style nits:
- opening braces go on their own lines (except after "do"),
- blank lines are wanted between declarations and statements,
- we try to move away from u<N> and want new code to use uint<N>_t,
- with "devid" declared in the narrowest possible scope, please do
  so also for "efr" (albeit this logic may get rearranged enough to
  make this point moot).

Jan

Re: [PATCH v2 2/2] ns16550: add Exar PCIe UART cards support

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 2431 bytes --]

On Thu, Aug 19, 2021 at 05:57:21PM +0200, Jan Beulich wrote:
> On 18.08.2021 14:16, Marek Marczykowski-Górecki wrote:
> > @@ -169,6 +172,29 @@ static void handle_dw_usr_busy_quirk(struct ns16550 *uart)
> >      }
> >  }
> >  
> > +static void enable_exar_enhanced_bits(struct ns16550 *uart)
> 
> Afaics the parameter can be pointer-to-const.
> 
> > +{
> > +#ifdef NS16550_PCI
> > +    if ( uart->bar &&
> > +         pci_conf_read16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[2],
> > +                         uart->ps_bdf[2]), PCI_VENDOR_ID) == PCI_VENDOR_ID_EXAR )
> > +    {
> > +        u8 devid = ns_read_reg(uart, UART_XR_DVID);
> > +        u8 efr;
> > +        /*
> > +         * Exar XR17V35x cards ignore setting MCR[2] (hardware flow control)
> > +         * unless "Enhanced control bits" is enabled.
> > +         * The below checks for a 2, 4 or 8 port UART, following Linux driver.
> > +         */
> > +        if ( devid == 0x82 || devid == 0x84 || devid == 0x88 ) {
> 
> Hmm, now I'm in trouble as to a question you did ask earlier (once
> on irc and I think also once in email): You asked whether to use
> the PCI device ID _or_ the DVID register. Now you're using both,
> albeit in a way not really making the access here safe: You assume
> that you can access the byte at offset 0x8d on all Exar devices. I
> don't know whether there is anything written anywhere telling you
> this is safe. With the earlier vendor/device ID match, it would feel
> safer to me if you replaced vendor ID and DVID checks here by a
> check of uart->param: While you must not deref that pointer, you can
> still check that it points at one of the three new entries you add
> to uart_param[]. Perhaps via "switch ( uart->param - uart_param )".

Ok, indeed with checking just uart->param - uart_param, I can get rid of
pci_conf_read here entirely. And so the #ifdef won't be necessary
either.

> Also there are a number of style nits:
> - opening braces go on their own lines (except after "do"),
> - blank lines are wanted between declarations and statements,
> - we try to move away from u<N> and want new code to use uint<N>_t,
> - with "devid" declared in the narrowest possible scope, please do
>   so also for "efr" (albeit this logic may get rearranged enough to
>   make this point moot).
> 
> Jan
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] ns16550: add Exar PCIe UART cards support

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 845 bytes --]

On Thu, Aug 19, 2021 at 06:01:31PM +0200, Marek Marczykowski-Górecki wrote:
> On Thu, Aug 19, 2021 at 05:57:21PM +0200, Jan Beulich wrote:
> > On 18.08.2021 14:16, Marek Marczykowski-Górecki wrote:
> > > @@ -169,6 +172,29 @@ static void handle_dw_usr_busy_quirk(struct ns16550 *uart)
> > >      }
> > >  }
> > >  
> > > +static void enable_exar_enhanced_bits(struct ns16550 *uart)
> > 
> > Afaics the parameter can be pointer-to-const.

ns_read_reg()/ns_write_reg() lack const, so not really. They could gain
a const, though.

> Ok, indeed with checking just uart->param - uart_param, I can get rid of
> pci_conf_read here entirely. And so the #ifdef won't be necessary
> either.

#ifdef needs to stay, because uart_param is PCI-only. Not a big deal.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]