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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.08.2021 21:06, 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 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@xxxxxxxxxxxxxxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one line dropped (see bottom of mail).

> I've opted to move the #ifdef into ns16550_setup_preirq() itself and
> move the whole enable_exar_enhanced_bits() into appropriate #ifdef too
> (need to move it anyway, to use uart_param). But if you prefer the older
> structure (#ifdef inside ns16550_setup_preirq()), I can do that too.

That's okay for now, I think. When we gain more quirks we may want to
re-arrange this some. The only thing I don't like (but which is
unavoidable afaict) is ...

> @@ -104,6 +107,8 @@ struct ns16550_config_param {
>      unsigned int uart_offset;
>      unsigned int first_offset;
>  };
> +
> +static void enable_exar_enhanced_bits(const struct ns16550 *uart);
>  #endif

... this added forward declaration.

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

The latter line can now be dropped again, as the symbol has no use
anymore. I'll try to remember to take care of this while committing.

Jan