All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] ns16550: do not override fifo size if explicitly set
@ 2021-08-13 18:31 Marek Marczykowski-Górecki
  2021-08-13 18:31 ` [PATCH 2/2] ns16550: add Exar dual PCIe UART card support Marek Marczykowski-Górecki
  2021-08-16  7:42 ` [PATCH 1/2] ns16550: do not override fifo size if explicitly set Jan Beulich
  0 siblings, 2 replies; 9+ messages in thread
From: Marek Marczykowski-Górecki @ 2021-08-13 18:31 UTC (permalink / raw)
  To: xen-devel
  Cc: Marek Marczykowski-Górecki, Andrew Cooper, George Dunlap,
	Ian Jackson, Jan Beulich, Julien Grall, Stefano Stabellini,
	Wei Liu

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



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] ns16550: add Exar dual PCIe UART card support
  2021-08-13 18:31 [PATCH 1/2] ns16550: do not override fifo size if explicitly set Marek Marczykowski-Górecki
@ 2021-08-13 18:31 ` Marek Marczykowski-Górecki
  2021-08-16  7:55   ` Jan Beulich
  2021-08-16  7:42 ` [PATCH 1/2] ns16550: do not override fifo size if explicitly set Jan Beulich
  1 sibling, 1 reply; 9+ messages in thread
From: Marek Marczykowski-Górecki @ 2021-08-13 18:31 UTC (permalink / raw)
  To: xen-devel
  Cc: Marek Marczykowski-Górecki, Andrew Cooper, George Dunlap,
	Ian Jackson, Jan Beulich, Julien Grall, Stefano Stabellini,
	Wei Liu

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 cards only (based on vendor ID).

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

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/drivers/char/ns16550.c  | 39 +++++++++++++++++++++++++++++++++++--
 xen/include/xen/8250-uart.h |  4 ++++
 xen/include/xen/pci_ids.h   |  2 ++
 3 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 97b85b0225cc..540124b198df 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -88,6 +88,7 @@ struct ns16550_config {
         param_pericom_2port,
         param_pericom_4port,
         param_pericom_8port,
+        param_exar_xr17v3521,
     } param;
 };
 
@@ -169,6 +170,21 @@ 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 )
+    {
+        /* Exar cards ignores setting MCR[2] (hardware flow control) unless
+         * "Enhanced control bits" is enabled.
+         */
+        ns_write_reg(uart, UART_XR_EFR, UART_EFR_ECB);
+    }
+#endif
+}
+
 static void ns16550_interrupt(
     int irq, void *dev_id, struct cpu_user_regs *regs)
 {
@@ -303,6 +319,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 +800,17 @@ static const struct ns16550_config_param __initconst uart_param[] = {
         .lsr_mask = UART_LSR_THRE,
         .bar0 = 1,
         .max_ports = 8,
-    }
+    },
+    [param_exar_xr17v3521] = {
+        .base_baud = 7812500,
+        .uart_offset = 0x400,
+        .reg_width = 1,
+        .fifo_size = 256,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .mmio = 1,
+        .max_ports = 2,
+    },
 };
 
 static const struct ns16550_config __initconst uart_config[] =
@@ -1007,7 +1036,13 @@ static const struct ns16550_config __initconst uart_config[] =
         .vendor_id = PCI_VENDOR_ID_PERICOM,
         .dev_id = 0x7958,
         .param = param_pericom_8port
-    }
+    },
+    /* Exar Corp. XR17V3521 Dual PCIe UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_EXAR,
+        .dev_id = 0x0352,
+        .param = param_exar_xr17v3521
+    },
 };
 
 static int __init
diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
index 5c3bac33221e..8c12fbbb3d5a 100644
--- a/xen/include/xen/8250-uart.h
+++ b/xen/include/xen/8250-uart.h
@@ -121,6 +121,10 @@
 /* Frequency of external clock source. This definition assumes PC platform. */
 #define UART_CLOCK_HZ     1843200
 
+/* Exar specific */
+#define UART_XR_EFR       0x09
+#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



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] ns16550: do not override fifo size if explicitly set
  2021-08-13 18:31 [PATCH 1/2] ns16550: do not override fifo size if explicitly set Marek Marczykowski-Górecki
  2021-08-13 18:31 ` [PATCH 2/2] ns16550: add Exar dual PCIe UART card support Marek Marczykowski-Górecki
@ 2021-08-16  7:42 ` Jan Beulich
  1 sibling, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2021-08-16  7:42 UTC (permalink / raw)
  To: Marek Marczykowski-Górecki
  Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
	Stefano Stabellini, Wei Liu, xen-devel

On 13.08.2021 20:31, Marek Marczykowski-Górecki wrote:
> 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>



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] ns16550: add Exar dual PCIe UART card support
  2021-08-13 18:31 ` [PATCH 2/2] ns16550: add Exar dual PCIe UART card support Marek Marczykowski-Górecki
@ 2021-08-16  7:55   ` Jan Beulich
  2021-08-16  8:39     ` Marek Marczykowski-Górecki
  0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2021-08-16  7:55 UTC (permalink / raw)
  To: Marek Marczykowski-Górecki
  Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
	Stefano Stabellini, Wei Liu, xen-devel

On 13.08.2021 20:31, Marek Marczykowski-Górecki wrote:
> 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 cards only (based on vendor ID).

While on IRC you did say you have a datasheet or alike for the specific
card you have in use, may I ask that you clarify why the logic is
applicable to all (past, present, and future) Exar cards?

> @@ -169,6 +170,21 @@ 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 )
> +    {
> +        /* Exar cards ignores setting MCR[2] (hardware flow control) unless
> +         * "Enhanced control bits" is enabled.
> +         */

Style nit: /* belongs on its own line as per ./CODING_STYLE.

> +        ns_write_reg(uart, UART_XR_EFR, UART_EFR_ECB);

Wouldn't this better be a read-modify-write operation?

> --- a/xen/include/xen/8250-uart.h
> +++ b/xen/include/xen/8250-uart.h
> @@ -121,6 +121,10 @@
>  /* Frequency of external clock source. This definition assumes PC platform. */
>  #define UART_CLOCK_HZ     1843200
>  
> +/* Exar specific */
> +#define UART_XR_EFR       0x09
> +#define UART_EFR_ECB      0x10

Please move the former into the group after the "/* Register offsets */"
comment and extend the comment on the latter to establish a link to
UART_XR_EFR.

Jan



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] ns16550: add Exar dual PCIe UART card support
  2021-08-16  7:55   ` Jan Beulich
@ 2021-08-16  8:39     ` Marek Marczykowski-Górecki
  2021-08-16  9:18       ` Jan Beulich
  0 siblings, 1 reply; 9+ messages in thread
From: Marek Marczykowski-Górecki @ 2021-08-16  8:39 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
	Stefano Stabellini, Wei Liu, xen-devel

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

On Mon, Aug 16, 2021 at 09:55:16AM +0200, Jan Beulich wrote:
> On 13.08.2021 20:31, Marek Marczykowski-Górecki wrote:
> > 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 cards only (based on vendor ID).
> 
> While on IRC you did say you have a datasheet or alike for the specific
> card you have in use, may I ask that you clarify why the logic is
> applicable to all (past, present, and future) Exar cards?

The spec I looked is specifically about 2-port variant (XR17V352), but
there are also 4 and 8 port variants (XR17V354 and XR17V358) and the
Linux driver applies this change there as well. But indeed applying it
to all the future cards may not be the smartest thing to do.

The Linux driver checks Exar specific register to identify the device,
instead of using PCI product ID, for some reason - I guess they use the
same chip in different devices?
Would you like thing like that (after checking vendor id), or turn it on
just for this product id I have?

> > @@ -169,6 +170,21 @@ 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 )
> > +    {
> > +        /* Exar cards ignores setting MCR[2] (hardware flow control) unless
> > +         * "Enhanced control bits" is enabled.
> > +         */
> 
> Style nit: /* belongs on its own line as per ./CODING_STYLE.
> 
> > +        ns_write_reg(uart, UART_XR_EFR, UART_EFR_ECB);
> 
> Wouldn't this better be a read-modify-write operation?

Honestly, I'm simply mirroring Linux driver behavior here. But also,
all the bits in EFR are 0 after device reset, so it should work fine.

> > --- a/xen/include/xen/8250-uart.h
> > +++ b/xen/include/xen/8250-uart.h
> > @@ -121,6 +121,10 @@
> >  /* Frequency of external clock source. This definition assumes PC platform. */
> >  #define UART_CLOCK_HZ     1843200
> >  
> > +/* Exar specific */
> > +#define UART_XR_EFR       0x09
> > +#define UART_EFR_ECB      0x10
> 
> Please move the former into the group after the "/* Register offsets */"
> comment and extend the comment on the latter to establish a link to
> UART_XR_EFR.

ok

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

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] ns16550: add Exar dual PCIe UART card support
  2021-08-16  8:39     ` Marek Marczykowski-Górecki
@ 2021-08-16  9:18       ` Jan Beulich
  2021-08-16 10:25         ` Marek Marczykowski-Górecki
  0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2021-08-16  9:18 UTC (permalink / raw)
  To: Marek Marczykowski-Górecki
  Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
	Stefano Stabellini, Wei Liu, xen-devel

On 16.08.2021 10:39, Marek Marczykowski-Górecki wrote:
> On Mon, Aug 16, 2021 at 09:55:16AM +0200, Jan Beulich wrote:
>> On 13.08.2021 20:31, Marek Marczykowski-Górecki wrote:
>>> 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 cards only (based on vendor ID).
>>
>> While on IRC you did say you have a datasheet or alike for the specific
>> card you have in use, may I ask that you clarify why the logic is
>> applicable to all (past, present, and future) Exar cards?
> 
> The spec I looked is specifically about 2-port variant (XR17V352), but
> there are also 4 and 8 port variants (XR17V354 and XR17V358) and the
> Linux driver applies this change there as well. But indeed applying it
> to all the future cards may not be the smartest thing to do.
> 
> The Linux driver checks Exar specific register to identify the device,
> instead of using PCI product ID, for some reason - I guess they use the
> same chip in different devices?
> Would you like thing like that (after checking vendor id), or turn it on
> just for this product id I have?

Hard to tell without knowing whether the extra reg - as per the spec -
is connected to any of these. Is the spec you have publicly available?
If so, could you share a pointer? If not, are you permitted to share
the spec?

>>> @@ -169,6 +170,21 @@ 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 )
>>> +    {
>>> +        /* Exar cards ignores setting MCR[2] (hardware flow control) unless
>>> +         * "Enhanced control bits" is enabled.
>>> +         */
>>
>> Style nit: /* belongs on its own line as per ./CODING_STYLE.
>>
>>> +        ns_write_reg(uart, UART_XR_EFR, UART_EFR_ECB);
>>
>> Wouldn't this better be a read-modify-write operation?
> 
> Honestly, I'm simply mirroring Linux driver behavior here. But also,
> all the bits in EFR are 0 after device reset, so it should work fine.

Firmware or a boot loader may play with hardware before Xen takes control.
I'm also not convinced there would have been a device reset in all cases
where execution may make it here. (Note in particular that the function
and its caller aren't __init.)

A plain write might be okay if the spec for devices with the specific
device ID documented all other bits as "must be zero" ("reserved" would
not be sufficient imo), and if the function was invoked for only such
devices.

Jan



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] ns16550: add Exar dual PCIe UART card support
  2021-08-16  9:18       ` Jan Beulich
@ 2021-08-16 10:25         ` Marek Marczykowski-Górecki
  2021-08-16 12:18           ` Jan Beulich
  0 siblings, 1 reply; 9+ messages in thread
From: Marek Marczykowski-Górecki @ 2021-08-16 10:25 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
	Stefano Stabellini, Wei Liu, xen-devel

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

On Mon, Aug 16, 2021 at 11:18:31AM +0200, Jan Beulich wrote:
> On 16.08.2021 10:39, Marek Marczykowski-Górecki wrote:
> > On Mon, Aug 16, 2021 at 09:55:16AM +0200, Jan Beulich wrote:
> >> On 13.08.2021 20:31, Marek Marczykowski-Górecki wrote:
> >>> 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 cards only (based on vendor ID).
> >>
> >> While on IRC you did say you have a datasheet or alike for the specific
> >> card you have in use, may I ask that you clarify why the logic is
> >> applicable to all (past, present, and future) Exar cards?
> > 
> > The spec I looked is specifically about 2-port variant (XR17V352), but
> > there are also 4 and 8 port variants (XR17V354 and XR17V358) and the
> > Linux driver applies this change there as well. But indeed applying it
> > to all the future cards may not be the smartest thing to do.
> > 
> > The Linux driver checks Exar specific register to identify the device,
> > instead of using PCI product ID, for some reason - I guess they use the
> > same chip in different devices?
> > Would you like thing like that (after checking vendor id), or turn it on
> > just for this product id I have?
> 
> Hard to tell without knowing whether the extra reg - as per the spec -
> is connected to any of these. Is the spec you have publicly available?

Yes, here: https://www.maxlinear.com/document/index?id=1585&languageid=1033&type=Datasheet&partnumber=XR17V352&filename=XR17V352.pdf&part=XR17V352
(and few more links on https://www.maxlinear.com/product/interface/uarts/pcie-uarts/xr17v352, but mostly the above PDF)

Hmm, maybe I should add the link to the commit message?

> If so, could you share a pointer? If not, are you permitted to share
> the spec?
> 
> >>> @@ -169,6 +170,21 @@ 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 )
> >>> +    {
> >>> +        /* Exar cards ignores setting MCR[2] (hardware flow control) unless
> >>> +         * "Enhanced control bits" is enabled.
> >>> +         */
> >>
> >> Style nit: /* belongs on its own line as per ./CODING_STYLE.
> >>
> >>> +        ns_write_reg(uart, UART_XR_EFR, UART_EFR_ECB);
> >>
> >> Wouldn't this better be a read-modify-write operation?
> > 
> > Honestly, I'm simply mirroring Linux driver behavior here. But also,
> > all the bits in EFR are 0 after device reset, so it should work fine.
> 
> Firmware or a boot loader may play with hardware before Xen takes control.
> I'm also not convinced there would have been a device reset in all cases
> where execution may make it here. (Note in particular that the function
> and its caller aren't __init.)
> 
> A plain write might be okay if the spec for devices with the specific
> device ID documented all other bits as "must be zero" ("reserved" would
> not be sufficient imo), and if the function was invoked for only such
> devices.

Other bits are defined and are things IMO we want to keep disabled. See top
of the page 40 in the PDF.

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

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] ns16550: add Exar dual PCIe UART card support
  2021-08-16 10:25         ` Marek Marczykowski-Górecki
@ 2021-08-16 12:18           ` Jan Beulich
  2021-08-16 23:33             ` Marek Marczykowski-Górecki
  0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2021-08-16 12:18 UTC (permalink / raw)
  To: Marek Marczykowski-Górecki
  Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
	Stefano Stabellini, Wei Liu, xen-devel

On 16.08.2021 12:25, Marek Marczykowski-Górecki wrote:
> On Mon, Aug 16, 2021 at 11:18:31AM +0200, Jan Beulich wrote:
>> On 16.08.2021 10:39, Marek Marczykowski-Górecki wrote:
>>> On Mon, Aug 16, 2021 at 09:55:16AM +0200, Jan Beulich wrote:
>>>> On 13.08.2021 20:31, Marek Marczykowski-Górecki wrote:
>>>>> 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 cards only (based on vendor ID).
>>>>
>>>> While on IRC you did say you have a datasheet or alike for the specific
>>>> card you have in use, may I ask that you clarify why the logic is
>>>> applicable to all (past, present, and future) Exar cards?
>>>
>>> The spec I looked is specifically about 2-port variant (XR17V352), but
>>> there are also 4 and 8 port variants (XR17V354 and XR17V358) and the
>>> Linux driver applies this change there as well. But indeed applying it
>>> to all the future cards may not be the smartest thing to do.
>>>
>>> The Linux driver checks Exar specific register to identify the device,
>>> instead of using PCI product ID, for some reason - I guess they use the
>>> same chip in different devices?
>>> Would you like thing like that (after checking vendor id), or turn it on
>>> just for this product id I have?
>>
>> Hard to tell without knowing whether the extra reg - as per the spec -
>> is connected to any of these. Is the spec you have publicly available?
> 
> Yes, here: https://www.maxlinear.com/document/index?id=1585&languageid=1033&type=Datasheet&partnumber=XR17V352&filename=XR17V352.pdf&part=XR17V352
> (and few more links on https://www.maxlinear.com/product/interface/uarts/pcie-uarts/xr17v352, but mostly the above PDF)

Ah yes, thanks.

> Hmm, maybe I should add the link to the commit message?

Wouldn't hurt; question is how likely it is for the link to become stale
in the next couple of years.

>>>>> @@ -169,6 +170,21 @@ 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 )
>>>>> +    {
>>>>> +        /* Exar cards ignores setting MCR[2] (hardware flow control) unless
>>>>> +         * "Enhanced control bits" is enabled.
>>>>> +         */
>>>>
>>>> Style nit: /* belongs on its own line as per ./CODING_STYLE.
>>>>
>>>>> +        ns_write_reg(uart, UART_XR_EFR, UART_EFR_ECB);
>>>>
>>>> Wouldn't this better be a read-modify-write operation?
>>>
>>> Honestly, I'm simply mirroring Linux driver behavior here. But also,
>>> all the bits in EFR are 0 after device reset, so it should work fine.
>>
>> Firmware or a boot loader may play with hardware before Xen takes control.
>> I'm also not convinced there would have been a device reset in all cases
>> where execution may make it here. (Note in particular that the function
>> and its caller aren't __init.)
>>
>> A plain write might be okay if the spec for devices with the specific
>> device ID documented all other bits as "must be zero" ("reserved" would
>> not be sufficient imo), and if the function was invoked for only such
>> devices.
> 
> Other bits are defined and are things IMO we want to keep disabled. See top
> of the page 40 in the PDF.

To be honest, in particular for the low 4 bits I'm not sure we should
alter them if they turn out non-zero (e.g. due to firmware or boot
loader action).

Jan



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] ns16550: add Exar dual PCIe UART card support
  2021-08-16 12:18           ` Jan Beulich
@ 2021-08-16 23:33             ` Marek Marczykowski-Górecki
  0 siblings, 0 replies; 9+ messages in thread
From: Marek Marczykowski-Górecki @ 2021-08-16 23:33 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
	Stefano Stabellini, Wei Liu, xen-devel

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

On Mon, Aug 16, 2021 at 02:18:33PM +0200, Jan Beulich wrote:
> On 16.08.2021 12:25, Marek Marczykowski-Górecki wrote:
> > On Mon, Aug 16, 2021 at 11:18:31AM +0200, Jan Beulich wrote:
> >> Hard to tell without knowing whether the extra reg - as per the spec -
> >> is connected to any of these. Is the spec you have publicly available?
> > 
> > Yes, here: https://www.maxlinear.com/document/index?id=1585&languageid=1033&type=Datasheet&partnumber=XR17V352&filename=XR17V352.pdf&part=XR17V352
> > (and few more links on https://www.maxlinear.com/product/interface/uarts/pcie-uarts/xr17v352, but mostly the above PDF)
> 
> Ah yes, thanks.
> 
> > Hmm, maybe I should add the link to the commit message?
> 
> Wouldn't hurt; question is how likely it is for the link to become stale
> in the next couple of years.

No idea, but the latter URL looks nicer - likely manually chosen
instead of purely generated by some CMS, and as such, there is IMO
better chance for it to remain alive.

> > Other bits are defined and are things IMO we want to keep disabled. See top
> > of the page 40 in the PDF.
> 
> To be honest, in particular for the low 4 bits I'm not sure we should
> alter them if they turn out non-zero (e.g. due to firmware or boot
> loader action).

Given how much time I spent to find out the not working console was
because of failure to disable flow control, I tend to disagree...

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

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-08-16 23:34 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-13 18:31 [PATCH 1/2] ns16550: do not override fifo size if explicitly set Marek Marczykowski-Górecki
2021-08-13 18:31 ` [PATCH 2/2] ns16550: add Exar dual PCIe UART card support Marek Marczykowski-Górecki
2021-08-16  7:55   ` Jan Beulich
2021-08-16  8:39     ` Marek Marczykowski-Górecki
2021-08-16  9:18       ` Jan Beulich
2021-08-16 10:25         ` Marek Marczykowski-Górecki
2021-08-16 12:18           ` Jan Beulich
2021-08-16 23:33             ` Marek Marczykowski-Górecki
2021-08-16  7:42 ` [PATCH 1/2] ns16550: do not override fifo size if explicitly set Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.