* [PATCH v2 1/2] ns16550: do not override fifo size if explicitly set
@ 2021-08-18 12:16 Marek Marczykowski-Górecki
2021-08-18 12:16 ` [PATCH v2 2/2] ns16550: add Exar PCIe UART cards support Marek Marczykowski-Górecki
0 siblings, 1 reply; 5+ messages in thread
From: Marek Marczykowski-Górecki @ 2021-08-18 12:16 UTC (permalink / raw)
To: xen-devel
Cc: Marek Marczykowski-Górecki, Jan Beulich, Andrew Cooper,
George Dunlap, Ian Jackson, Julien Grall, Stefano Stabellini,
Wei Liu
If fifo size is already set via uart_params, do not force it to 16 - which
may not match the actual hardware. Specifically Exar cards have fifo of
256 bytes.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
xen/drivers/char/ns16550.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 16a73d0c0e47..97b85b0225cc 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -357,7 +357,8 @@ static void __init ns16550_init_preirq(struct serial_port *port)
ns16550_setup_preirq(uart);
/* Check this really is a 16550+. Otherwise we have no FIFOs. */
- if ( ((ns_read_reg(uart, UART_IIR) & 0xc0) == 0xc0) &&
+ if ( uart->fifo_size <= 1 &&
+ ((ns_read_reg(uart, UART_IIR) & 0xc0) == 0xc0) &&
((ns_read_reg(uart, UART_FCR) & UART_FCR_TRG14) == UART_FCR_TRG14) )
uart->fifo_size = 16;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] ns16550: add Exar PCIe UART cards support
2021-08-18 12:16 [PATCH v2 1/2] ns16550: do not override fifo size if explicitly set Marek Marczykowski-Górecki
@ 2021-08-18 12:16 ` Marek Marczykowski-Górecki
2021-08-19 15:57 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Marek Marczykowski-Górecki @ 2021-08-18 12:16 UTC (permalink / raw)
To: xen-devel
Cc: Marek Marczykowski-Górecki, Andrew Cooper, George Dunlap,
Ian Jackson, Jan Beulich, Julien Grall, Stefano Stabellini,
Wei Liu
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(-)
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
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] ns16550: add Exar PCIe UART cards support
2021-08-18 12:16 ` [PATCH v2 2/2] ns16550: add Exar PCIe UART cards support Marek Marczykowski-Górecki
@ 2021-08-19 15:57 ` Jan Beulich
2021-08-19 16:01 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2021-08-19 15:57 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Wei Liu, xen-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] ns16550: add Exar PCIe UART cards support
2021-08-19 15:57 ` Jan Beulich
@ 2021-08-19 16:01 ` Marek Marczykowski-Górecki
2021-08-19 16:14 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 5+ messages in thread
From: Marek Marczykowski-Górecki @ 2021-08-19 16:01 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Wei Liu, xen-devel
[-- Attachment #1: Type: text/plain, Size: 2431 bytes --]
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
>
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] ns16550: add Exar PCIe UART cards support
2021-08-19 16:01 ` Marek Marczykowski-Górecki
@ 2021-08-19 16:14 ` Marek Marczykowski-Górecki
0 siblings, 0 replies; 5+ messages in thread
From: Marek Marczykowski-Górecki @ 2021-08-19 16:14 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Wei Liu, xen-devel
[-- Attachment #1: Type: text/plain, Size: 845 bytes --]
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.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-08-19 16:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-18 12:16 [PATCH v2 1/2] ns16550: do not override fifo size if explicitly set Marek Marczykowski-Górecki
2021-08-18 12:16 ` [PATCH v2 2/2] ns16550: add Exar PCIe UART cards support Marek Marczykowski-Górecki
2021-08-19 15:57 ` Jan Beulich
2021-08-19 16:01 ` Marek Marczykowski-Górecki
2021-08-19 16:14 ` Marek Marczykowski-Górecki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.