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

[Jan Beulich <jbeulich@suse.com>]

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