| Message ID | 20210818121649.462413-2-marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] ns16550: do not override fifo size if explicitly set | expand |
On 18.08.2021 14:16, Marek Marczykowski-Górecki wrote: > @@ -169,6 +172,29 @@ static void handle_dw_usr_busy_quirk(struct ns16550 *uart) > } > } > > +static void enable_exar_enhanced_bits(struct ns16550 *uart) Afaics the parameter can be pointer-to-const. > +{ > +#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 ) > + { > + u8 devid = ns_read_reg(uart, UART_XR_DVID); > + u8 efr; > + /* > + * Exar XR17V35x cards ignore setting MCR[2] (hardware flow control) > + * unless "Enhanced control bits" is enabled. > + * The below checks for a 2, 4 or 8 port UART, following Linux driver. > + */ > + if ( devid == 0x82 || devid == 0x84 || devid == 0x88 ) { Hmm, now I'm in trouble as to a question you did ask earlier (once on irc and I think also once in email): You asked whether to use the PCI device ID _or_ the DVID register. Now you're using both, albeit in a way not really making the access here safe: You assume that you can access the byte at offset 0x8d on all Exar devices. I don't know whether there is anything written anywhere telling you this is safe. With the earlier vendor/device ID match, it would feel safer to me if you replaced vendor ID and DVID checks here by a check of uart->param: While you must not deref that pointer, you can still check that it points at one of the three new entries you add to uart_param[]. Perhaps via "switch ( uart->param - uart_param )". Also there are a number of style nits: - opening braces go on their own lines (except after "do"), - blank lines are wanted between declarations and statements, - we try to move away from u<N> and want new code to use uint<N>_t, - with "devid" declared in the narrowest possible scope, please do so also for "efr" (albeit this logic may get rearranged enough to make this point moot). Jan
On Thu, Aug 19, 2021 at 05:57:21PM +0200, Jan Beulich wrote: > On 18.08.2021 14:16, Marek Marczykowski-Górecki wrote: > > @@ -169,6 +172,29 @@ static void handle_dw_usr_busy_quirk(struct ns16550 *uart) > > } > > } > > > > +static void enable_exar_enhanced_bits(struct ns16550 *uart) > > Afaics the parameter can be pointer-to-const. > > > +{ > > +#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 ) > > + { > > + u8 devid = ns_read_reg(uart, UART_XR_DVID); > > + u8 efr; > > + /* > > + * Exar XR17V35x cards ignore setting MCR[2] (hardware flow control) > > + * unless "Enhanced control bits" is enabled. > > + * The below checks for a 2, 4 or 8 port UART, following Linux driver. > > + */ > > + if ( devid == 0x82 || devid == 0x84 || devid == 0x88 ) { > > Hmm, now I'm in trouble as to a question you did ask earlier (once > on irc and I think also once in email): You asked whether to use > the PCI device ID _or_ the DVID register. Now you're using both, > albeit in a way not really making the access here safe: You assume > that you can access the byte at offset 0x8d on all Exar devices. I > don't know whether there is anything written anywhere telling you > this is safe. With the earlier vendor/device ID match, it would feel > safer to me if you replaced vendor ID and DVID checks here by a > check of uart->param: While you must not deref that pointer, you can > still check that it points at one of the three new entries you add > to uart_param[]. Perhaps via "switch ( uart->param - uart_param )". Ok, indeed with checking just uart->param - uart_param, I can get rid of pci_conf_read here entirely. And so the #ifdef won't be necessary either. > Also there are a number of style nits: > - opening braces go on their own lines (except after "do"), > - blank lines are wanted between declarations and statements, > - we try to move away from u<N> and want new code to use uint<N>_t, > - with "devid" declared in the narrowest possible scope, please do > so also for "efr" (albeit this logic may get rearranged enough to > make this point moot). > > Jan >
On Thu, Aug 19, 2021 at 06:01:31PM +0200, Marek Marczykowski-Górecki wrote: > On Thu, Aug 19, 2021 at 05:57:21PM +0200, Jan Beulich wrote: > > On 18.08.2021 14:16, Marek Marczykowski-Górecki wrote: > > > @@ -169,6 +172,29 @@ static void handle_dw_usr_busy_quirk(struct ns16550 *uart) > > > } > > > } > > > > > > +static void enable_exar_enhanced_bits(struct ns16550 *uart) > > > > Afaics the parameter can be pointer-to-const. ns_read_reg()/ns_write_reg() lack const, so not really. They could gain a const, though. > Ok, indeed with checking just uart->param - uart_param, I can get rid of > pci_conf_read here entirely. And so the #ifdef won't be necessary > either. #ifdef needs to stay, because uart_param is PCI-only. Not a big deal.
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index 97b85b0225cc..27501f28aa7b 100644 --- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -88,6 +88,9 @@ struct ns16550_config { param_pericom_2port, param_pericom_4port, param_pericom_8port, + param_exar_xr17v352, + param_exar_xr17v354, + param_exar_xr17v358, } param; }; @@ -169,6 +172,29 @@ 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 ) + { + u8 devid = ns_read_reg(uart, UART_XR_DVID); + u8 efr; + /* + * Exar XR17V35x cards ignore setting MCR[2] (hardware flow control) + * unless "Enhanced control bits" is enabled. + * The below checks for a 2, 4 or 8 port UART, following Linux driver. + */ + if ( devid == 0x82 || devid == 0x84 || devid == 0x88 ) { + efr = ns_read_reg(uart, UART_XR_EFR); + efr |= UART_EFR_ECB; + ns_write_reg(uart, UART_XR_EFR, efr); + } + } +#endif +} + static void ns16550_interrupt( int irq, void *dev_id, struct cpu_user_regs *regs) { @@ -303,6 +329,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 +810,37 @@ static const struct ns16550_config_param __initconst uart_param[] = { .lsr_mask = UART_LSR_THRE, .bar0 = 1, .max_ports = 8, - } + }, + [param_exar_xr17v352] = { + .base_baud = 7812500, + .uart_offset = 0x400, + .reg_width = 1, + .fifo_size = 256, + .lsr_mask = UART_LSR_THRE, + .bar0 = 1, + .mmio = 1, + .max_ports = 2, + }, + [param_exar_xr17v354] = { + .base_baud = 7812500, + .uart_offset = 0x400, + .reg_width = 1, + .fifo_size = 256, + .lsr_mask = UART_LSR_THRE, + .bar0 = 1, + .mmio = 1, + .max_ports = 4, + }, + [param_exar_xr17v358] = { + .base_baud = 7812500, + .uart_offset = 0x400, + .reg_width = 1, + .fifo_size = 256, + .lsr_mask = UART_LSR_THRE, + .bar0 = 1, + .mmio = 1, + .max_ports = 8, + }, }; static const struct ns16550_config __initconst uart_config[] = @@ -1007,7 +1066,25 @@ static const struct ns16550_config __initconst uart_config[] = .vendor_id = PCI_VENDOR_ID_PERICOM, .dev_id = 0x7958, .param = param_pericom_8port - } + }, + /* Exar Corp. XR17V352 Dual PCIe UART */ + { + .vendor_id = PCI_VENDOR_ID_EXAR, + .dev_id = 0x0352, + .param = param_exar_xr17v352 + }, + /* Exar Corp. XR17V354 Quad PCIe UART */ + { + .vendor_id = PCI_VENDOR_ID_EXAR, + .dev_id = 0x0354, + .param = param_exar_xr17v354 + }, + /* Exar Corp. XR17V358 Octal PCIe UART */ + { + .vendor_id = PCI_VENDOR_ID_EXAR, + .dev_id = 0x0358, + .param = param_exar_xr17v358 + }, }; static int __init diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h index 5c3bac33221e..74e9d552a718 100644 --- 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 */ /* Interrupt Enable Register */ #define UART_IER_ERDAI 0x01 /* rx data recv'd */ @@ -121,6 +123,9 @@ /* Frequency of external clock source. This definition assumes PC platform. */ #define UART_CLOCK_HZ 1843200 +/* Bits in Exar specific UART_XR_EFR register */ +#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
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@invisiblethingslab.com> --- Changes in v2: - use read-modify-write for enabling ECB - handle also 4- and 8-ports cards (but not everything from Exar) - formatting fixes --- xen/drivers/char/ns16550.c | 81 ++++++++++++++++++++++++++++++++++++- xen/include/xen/8250-uart.h | 5 +++ xen/include/xen/pci_ids.h | 2 + 3 files changed, 86 insertions(+), 2 deletions(-)