Re: [PATCH 2/2] ns16550: add Exar dual PCIe UART card support

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

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: signature.asc
Description: PGP signature