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

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

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