Aravind Gopalakrishnan
2013-Nov-21 22:50 UTC
[PATCH V4] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips
Since it is an MMIO device, the code has been modified to accept MMIO based
devices as well. MMIO device settings are populated in the
''uart_config'' table.
It also advertises 64 bit BAR. Therefore, code is reworked to account for 64
bit BAR and 64 bit MMIO lengths.
Some more quirks are - the need to shift the register offset by a specific
value and we also need to verify (UART_LSR_THRE && UART_LSR_TEMT) bits
before
transmitting data.
While testing, include com1=115200,8n1,pci,0 on the xen cmdline to observe
output on console using SOL.
Changes from V3:
- per Jan''s comments:
- Use unsigned int for uart_config struct and for local variables
in pci_uart_config function where applicable.
- use predefined MASKS for length calculation.
- Fix styling issues
- Misc:
- The device has 64 bit bar. Rework code to provide support for
64 bit bar, length values.
- Hide part of mmio region from dom0.
Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Signed-off-by: Thomas Lendacky <Thomas.Lendacky@amd.com>
---
xen/drivers/char/ns16550.c | 167 +++++++++++++++++++++++++++++++++++++++------
1 file changed, 146 insertions(+), 21 deletions(-)
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 9c2cded..79b3a5c 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -46,13 +46,14 @@ string_param("com2", opt_com2);
static struct ns16550 {
int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
u64 io_base; /* I/O port or memory-mapped I/O address. */
- u32 io_size;
+ u64 io_size;
int reg_shift; /* Bits to shift register offset by */
int reg_width; /* Size of access to use, the registers
* themselves are still bytes */
char __iomem *remapped_io_base; /* Remapped virtual address of MMIO. */
/* UART with IRQ line: interrupt-driven I/O. */
struct irqaction irqaction;
+ u8 lsr_mask;
#ifdef CONFIG_ARM
struct vuart_info vuart;
#endif
@@ -69,6 +70,7 @@ static struct ns16550 {
bool_t pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
bool_t ps_bdf_enable; /* if =1, ps_bdf effective, port on pci card */
u32 bar;
+ u32 bar64;
u16 cr;
u8 bar_idx;
#endif
@@ -77,6 +79,37 @@ static struct ns16550 {
#endif
} ns16550_com[2] = { { 0 } };
+/* Defining uart config options for MMIO devices */
+struct ns16550_config_mmio {
+ u16 vendor_id;
+ u16 dev_id;
+ unsigned int reg_shift;
+ unsigned int reg_width;
+ unsigned int fifo_size;
+ u8 lsr_mask;
+ unsigned int max_bars;
+};
+
+/*
+ * Create lookup tables for specific MMIO devices..
+ * It is assumed that if the device found is MMIO,
+ * then you have indexed it here. Else, the driver
+ * does nothing.
+ */
+static struct ns16550_config_mmio __initdata uart_config[] +{
+ /* Broadcom TruManage device */
+ {
+ .vendor_id = 0x14e4,
+ .dev_id = 0x160a,
+ .reg_shift = 2,
+ .reg_width = 1,
+ .fifo_size = 16,
+ .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
+ .max_bars = 1,
+ },
+};
+
static void ns16550_delayed_resume(void *data);
static char ns_read_reg(struct ns16550 *uart, int reg)
@@ -134,7 +167,7 @@ static void ns16550_interrupt(
while ( !(ns_read_reg(uart, UART_IIR) & UART_IIR_NOINT) )
{
char lsr = ns_read_reg(uart, UART_LSR);
- if ( lsr & UART_LSR_THRE )
+ if ( (lsr & uart->lsr_mask) == uart->lsr_mask )
serial_tx_interrupt(port, regs);
if ( lsr & UART_LSR_DR )
serial_rx_interrupt(port, regs);
@@ -160,7 +193,7 @@ static void __ns16550_poll(struct cpu_user_regs *regs)
serial_rx_interrupt(port, regs);
}
- if ( ns_read_reg(uart, UART_LSR) & UART_LSR_THRE )
+ if ( ( ns_read_reg(uart, UART_LSR) & uart->lsr_mask ) ==
uart->lsr_mask )
serial_tx_interrupt(port, regs);
out:
@@ -183,7 +216,9 @@ static int ns16550_tx_ready(struct serial_port *port)
if ( ns16550_ioport_invalid(uart) )
return -EIO;
- return ns_read_reg(uart, UART_LSR) & UART_LSR_THRE ? uart->fifo_size
: 0;
+
+ return ( (ns_read_reg(uart, UART_LSR) &
+ uart->lsr_mask ) == uart->lsr_mask ) ? uart->fifo_size :
0;
}
static void ns16550_putc(struct serial_port *port, char c)
@@ -381,6 +416,13 @@ static void _ns16550_resume(struct serial_port *port)
{
pci_conf_write32(0, uart->ps_bdf[0], uart->ps_bdf[1],
uart->ps_bdf[2],
PCI_BASE_ADDRESS_0 + uart->bar_idx*4, uart->bar);
+
+ /* If 64 bit BAR, write higher 32 bits to BAR+4 */
+ if ( uart->bar & PCI_BASE_ADDRESS_MEM_TYPE_64 )
+ pci_conf_write32(0, uart->ps_bdf[0],
+ uart->ps_bdf[1], uart->ps_bdf[2],
+ PCI_BASE_ADDRESS_0 + (uart->bar_idx+1)*4,
uart->bar64);
+
pci_conf_write16(0, uart->ps_bdf[0], uart->ps_bdf[1],
uart->ps_bdf[2],
PCI_COMMAND, uart->cr);
}
@@ -432,9 +474,16 @@ static void __init ns16550_endboot(struct serial_port
*port)
{
#ifdef HAS_IOPORTS
struct ns16550 *uart = port->uart;
-
+ unsigned long sfn, efn;
+
if ( uart->remapped_io_base )
+ {
+ sfn = paddr_to_pfn((unsigned long) uart->io_base + PAGE_SIZE);
+ efn = paddr_to_pfn((unsigned long) uart->io_base + uart->io_size
- 1);
+ if ( iomem_deny_access(dom0, sfn, efn) != 0 )
+ BUG();
return;
+ }
if ( ioports_deny_access(dom0, uart->io_base, uart->io_base + 7) != 0
)
BUG();
#endif
@@ -546,11 +595,13 @@ static int __init check_existence(struct ns16550 *uart)
}
#ifdef HAS_PCI
-static int
+static int __init
pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
{
- uint32_t bar, len;
- int b, d, f, nextf;
+ uint32_t bar, bar_64, len, len_64;
+ u64 size, mask;
+ unsigned int b, d, f, nextf, i;
+ u16 vendor, device;
/* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. */
for ( b = skip_amt ? 1 : 0; b < 0x100; b++ )
@@ -578,25 +629,96 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int
bar_idx)
bar = pci_conf_read32(0, b, d, f,
PCI_BASE_ADDRESS_0 + bar_idx*4);
-
- /* Not IO */
+ uart->bar = bar;
+
+ /* MMIO based */
if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
- continue;
-
- pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0, ~0u);
- len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0);
- pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0 + bar_idx*4,
bar);
-
- /* Not 8 bytes */
- if ( (len & 0xffff) != 0xfff9 )
- continue;
+ {
+ vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
+ device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID);
+
+ pci_conf_write32(0, b, d, f,
+ PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
+ len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0 +
bar_idx*4);
+ pci_conf_write32(0, b, d, f,
+ PCI_BASE_ADDRESS_0 + bar_idx*4, bar);
+
+ /* Handle 64 bit BAR if found */
+ if ( bar & PCI_BASE_ADDRESS_MEM_TYPE_64 )
+ {
+ bar_64 = pci_conf_read32(0, b, d, f,
+ PCI_BASE_ADDRESS_0 + (bar_idx+1)*4);
+ uart->bar64 = bar_64;
+ pci_conf_write32(0, b, d, f,
+ PCI_BASE_ADDRESS_0 + (bar_idx+1)*4, ~0u);
+ len_64 = pci_conf_read32(0, b, d, f,
+ PCI_BASE_ADDRESS_0 + (bar_idx+1)*4);
+ pci_conf_write32(0, b, d, f,
+ PCI_BASE_ADDRESS_0 + (bar_idx+1)*4,
bar_64);
+ mask = (((u64)~0 << 32) |
PCI_BASE_ADDRESS_MEM_MASK);
+ size = (((u64)len_64 << 32) | len) & mask;
+ size &= ~(size - 1);
+ uart->io_size = size;
+ }
+ else
+ {
+ len &= PCI_BASE_ADDRESS_MEM_MASK;
+ uart->io_size = len & ~(len - 1);
+ }
+
+ /* Force length of mmio region to be at least 8 bytes */
+ if ( uart->io_size < 0x8 )
+ continue;
+
+ /* Check for quirks in uart_config lookup table */
+ for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
+ {
+ if ( uart_config[i].vendor_id != vendor )
+ continue;
+
+ if ( uart_config[i].dev_id != device )
+ continue;
+
+ if ( bar_idx >= uart_config[i].max_bars )
+ continue;
+
+ if ( uart_config[i].fifo_size )
+ uart->fifo_size = uart_config[i].fifo_size;
+
+ uart->reg_shift = uart_config[i].reg_shift;
+ uart->reg_width = uart_config[i].reg_width;
+ uart->lsr_mask = uart_config[i].lsr_mask;
+ uart->io_base = ((u64)uart->bar64 << 32) |
+ (uart->bar &
PCI_BASE_ADDRESS_MEM_MASK);
+ break;
+ }
+
+ /* If we have an io_base, then we succeeded in the lookup
*/
+ if ( !uart->io_base )
+ continue;
+ }
+ /* IO based */
+ else
+ {
+ pci_conf_write32(0, b, d, f,
+ PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
+ len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0);
+ pci_conf_write32(0, b, d, f,
+ PCI_BASE_ADDRESS_0 + bar_idx*4, bar);
+ len &= PCI_BASE_ADDRESS_IO_MASK;
+ len &= ~(len - 1);
+
+ /* Not 8 bytes */
+ if ( len != 0x8 )
+ continue;
+
+ uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO;
+ }
uart->ps_bdf[0] = b;
uart->ps_bdf[1] = d;
uart->ps_bdf[2] = f;
- uart->bar = bar;
uart->bar_idx = bar_idx;
- uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO;
uart->irq = pci_conf_read8(0, b, d, f, PCI_INTERRUPT_PIN) ?
pci_conf_read8(0, b, d, f, PCI_INTERRUPT_LINE) : 0;
@@ -746,6 +868,9 @@ void __init ns16550_init(int index, struct ns16550_defaults
*defaults)
/* Default is no transmit FIFO. */
uart->fifo_size = 1;
+ /* Default lsr_mask = UART_LSR_THRE */
+ uart->lsr_mask = UART_LSR_THRE;
+
ns16550_parse_port_config(uart, (index == 0) ? opt_com1 : opt_com2);
}
--
1.8.1.2
Andrew Cooper
2013-Nov-22 10:44 UTC
Re: [PATCH V4] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips
On 21/11/13 22:50, Aravind Gopalakrishnan wrote:> Since it is an MMIO device, the code has been modified to accept MMIO based > devices as well. MMIO device settings are populated in the ''uart_config'' table. > It also advertises 64 bit BAR. Therefore, code is reworked to account for 64 > bit BAR and 64 bit MMIO lengths. > > Some more quirks are - the need to shift the register offset by a specific > value and we also need to verify (UART_LSR_THRE && UART_LSR_TEMT) bits before > transmitting data. > > While testing, include com1=115200,8n1,pci,0 on the xen cmdline to observe > output on console using SOL. > > Changes from V3: > - per Jan''s comments: > - Use unsigned int for uart_config struct and for local variables > in pci_uart_config function where applicable. > - use predefined MASKS for length calculation. > - Fix styling issues > - Misc: > - The device has 64 bit bar. Rework code to provide support for > 64 bit bar, length values. > - Hide part of mmio region from dom0. > > Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> > Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > Signed-off-by: Thomas Lendacky <Thomas.Lendacky@amd.com> > --- > xen/drivers/char/ns16550.c | 167 +++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 146 insertions(+), 21 deletions(-) > > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c > index 9c2cded..79b3a5c 100644 > --- a/xen/drivers/char/ns16550.c > +++ b/xen/drivers/char/ns16550.c > @@ -46,13 +46,14 @@ string_param("com2", opt_com2); > static struct ns16550 { > int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq; > u64 io_base; /* I/O port or memory-mapped I/O address. */ > - u32 io_size; > + u64 io_size; > int reg_shift; /* Bits to shift register offset by */ > int reg_width; /* Size of access to use, the registers > * themselves are still bytes */ > char __iomem *remapped_io_base; /* Remapped virtual address of MMIO. */ > /* UART with IRQ line: interrupt-driven I/O. */ > struct irqaction irqaction; > + u8 lsr_mask; > #ifdef CONFIG_ARM > struct vuart_info vuart; > #endif > @@ -69,6 +70,7 @@ static struct ns16550 { > bool_t pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */ > bool_t ps_bdf_enable; /* if =1, ps_bdf effective, port on pci card */ > u32 bar; > + u32 bar64; > u16 cr; > u8 bar_idx; > #endif > @@ -77,6 +79,37 @@ static struct ns16550 { > #endif > } ns16550_com[2] = { { 0 } }; > > +/* Defining uart config options for MMIO devices */ > +struct ns16550_config_mmio { > + u16 vendor_id; > + u16 dev_id; > + unsigned int reg_shift; > + unsigned int reg_width; > + unsigned int fifo_size; > + u8 lsr_mask; > + unsigned int max_bars; > +}; > + > +/* > + * Create lookup tables for specific MMIO devices.. > + * It is assumed that if the device found is MMIO, > + * then you have indexed it here. Else, the driver > + * does nothing. > + */ > +static struct ns16550_config_mmio __initdata uart_config[] > +{ > + /* Broadcom TruManage device */ > + { > + .vendor_id = 0x14e4, > + .dev_id = 0x160a, > + .reg_shift = 2, > + .reg_width = 1, > + .fifo_size = 16, > + .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT), > + .max_bars = 1, > + }, > +}; > + > static void ns16550_delayed_resume(void *data); > > static char ns_read_reg(struct ns16550 *uart, int reg) > @@ -134,7 +167,7 @@ static void ns16550_interrupt( > while ( !(ns_read_reg(uart, UART_IIR) & UART_IIR_NOINT) ) > { > char lsr = ns_read_reg(uart, UART_LSR); > - if ( lsr & UART_LSR_THRE ) > + if ( (lsr & uart->lsr_mask) == uart->lsr_mask ) > serial_tx_interrupt(port, regs); > if ( lsr & UART_LSR_DR ) > serial_rx_interrupt(port, regs); > @@ -160,7 +193,7 @@ static void __ns16550_poll(struct cpu_user_regs *regs) > serial_rx_interrupt(port, regs); > } > > - if ( ns_read_reg(uart, UART_LSR) & UART_LSR_THRE ) > + if ( ( ns_read_reg(uart, UART_LSR) & uart->lsr_mask ) == uart->lsr_mask ) > serial_tx_interrupt(port, regs); > > out: > @@ -183,7 +216,9 @@ static int ns16550_tx_ready(struct serial_port *port) > > if ( ns16550_ioport_invalid(uart) ) > return -EIO; > - return ns_read_reg(uart, UART_LSR) & UART_LSR_THRE ? uart->fifo_size : 0; > + > + return ( (ns_read_reg(uart, UART_LSR) & > + uart->lsr_mask ) == uart->lsr_mask ) ? uart->fifo_size : 0; > } > > static void ns16550_putc(struct serial_port *port, char c) > @@ -381,6 +416,13 @@ static void _ns16550_resume(struct serial_port *port) > { > pci_conf_write32(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2], > PCI_BASE_ADDRESS_0 + uart->bar_idx*4, uart->bar); > + > + /* If 64 bit BAR, write higher 32 bits to BAR+4 */ > + if ( uart->bar & PCI_BASE_ADDRESS_MEM_TYPE_64 ) > + pci_conf_write32(0, uart->ps_bdf[0], > + uart->ps_bdf[1], uart->ps_bdf[2], > + PCI_BASE_ADDRESS_0 + (uart->bar_idx+1)*4, uart->bar64); > + > pci_conf_write16(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2], > PCI_COMMAND, uart->cr); > } > @@ -432,9 +474,16 @@ static void __init ns16550_endboot(struct serial_port *port) > { > #ifdef HAS_IOPORTS > struct ns16550 *uart = port->uart; > - > + unsigned long sfn, efn; > + > if ( uart->remapped_io_base ) > + { > + sfn = paddr_to_pfn((unsigned long) uart->io_base + PAGE_SIZE); > + efn = paddr_to_pfn((unsigned long) uart->io_base + uart->io_size - 1); > + if ( iomem_deny_access(dom0, sfn, efn) != 0 ) > + BUG();BUG_ON(!iomem_deny_access(dom0, sfn, efn)) is slightly more compact, and has the advantage of showing the action which failed in the BUG message. ~Andrew> return; > + } > if ( ioports_deny_access(dom0, uart->io_base, uart->io_base + 7) != 0 ) > BUG(); > #endif > @@ -546,11 +595,13 @@ static int __init check_existence(struct ns16550 *uart) > } > > #ifdef HAS_PCI > -static int > +static int __init > pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx) > { > - uint32_t bar, len; > - int b, d, f, nextf; > + uint32_t bar, bar_64, len, len_64; > + u64 size, mask; > + unsigned int b, d, f, nextf, i; > + u16 vendor, device; > > /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. */ > for ( b = skip_amt ? 1 : 0; b < 0x100; b++ ) > @@ -578,25 +629,96 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx) > > bar = pci_conf_read32(0, b, d, f, > PCI_BASE_ADDRESS_0 + bar_idx*4); > - > - /* Not IO */ > + uart->bar = bar; > + > + /* MMIO based */ > if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) ) > - continue; > - > - pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0, ~0u); > - len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0); > - pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0 + bar_idx*4, bar); > - > - /* Not 8 bytes */ > - if ( (len & 0xffff) != 0xfff9 ) > - continue; > + { > + vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID); > + device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID); > + > + pci_conf_write32(0, b, d, f, > + PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u); > + len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0 + bar_idx*4); > + pci_conf_write32(0, b, d, f, > + PCI_BASE_ADDRESS_0 + bar_idx*4, bar); > + > + /* Handle 64 bit BAR if found */ > + if ( bar & PCI_BASE_ADDRESS_MEM_TYPE_64 ) > + { > + bar_64 = pci_conf_read32(0, b, d, f, > + PCI_BASE_ADDRESS_0 + (bar_idx+1)*4); > + uart->bar64 = bar_64; > + pci_conf_write32(0, b, d, f, > + PCI_BASE_ADDRESS_0 + (bar_idx+1)*4, ~0u); > + len_64 = pci_conf_read32(0, b, d, f, > + PCI_BASE_ADDRESS_0 + (bar_idx+1)*4); > + pci_conf_write32(0, b, d, f, > + PCI_BASE_ADDRESS_0 + (bar_idx+1)*4, bar_64); > + mask = (((u64)~0 << 32) | PCI_BASE_ADDRESS_MEM_MASK); > + size = (((u64)len_64 << 32) | len) & mask; > + size &= ~(size - 1); > + uart->io_size = size; > + } > + else > + { > + len &= PCI_BASE_ADDRESS_MEM_MASK; > + uart->io_size = len & ~(len - 1); > + } > + > + /* Force length of mmio region to be at least 8 bytes */ > + if ( uart->io_size < 0x8 ) > + continue; > + > + /* Check for quirks in uart_config lookup table */ > + for ( i = 0; i < ARRAY_SIZE(uart_config); i++) > + { > + if ( uart_config[i].vendor_id != vendor ) > + continue; > + > + if ( uart_config[i].dev_id != device ) > + continue; > + > + if ( bar_idx >= uart_config[i].max_bars ) > + continue; > + > + if ( uart_config[i].fifo_size ) > + uart->fifo_size = uart_config[i].fifo_size; > + > + uart->reg_shift = uart_config[i].reg_shift; > + uart->reg_width = uart_config[i].reg_width; > + uart->lsr_mask = uart_config[i].lsr_mask; > + uart->io_base = ((u64)uart->bar64 << 32) | > + (uart->bar & PCI_BASE_ADDRESS_MEM_MASK); > + break; > + } > + > + /* If we have an io_base, then we succeeded in the lookup */ > + if ( !uart->io_base ) > + continue; > + } > + /* IO based */ > + else > + { > + pci_conf_write32(0, b, d, f, > + PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u); > + len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0); > + pci_conf_write32(0, b, d, f, > + PCI_BASE_ADDRESS_0 + bar_idx*4, bar); > + len &= PCI_BASE_ADDRESS_IO_MASK; > + len &= ~(len - 1); > + > + /* Not 8 bytes */ > + if ( len != 0x8 ) > + continue; > + > + uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO; > + } > > uart->ps_bdf[0] = b; > uart->ps_bdf[1] = d; > uart->ps_bdf[2] = f; > - uart->bar = bar; > uart->bar_idx = bar_idx; > - uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO; > uart->irq = pci_conf_read8(0, b, d, f, PCI_INTERRUPT_PIN) ? > pci_conf_read8(0, b, d, f, PCI_INTERRUPT_LINE) : 0; > > @@ -746,6 +868,9 @@ void __init ns16550_init(int index, struct ns16550_defaults *defaults) > /* Default is no transmit FIFO. */ > uart->fifo_size = 1; > > + /* Default lsr_mask = UART_LSR_THRE */ > + uart->lsr_mask = UART_LSR_THRE; > + > ns16550_parse_port_config(uart, (index == 0) ? opt_com1 : opt_com2); > } >
Jan Beulich
2013-Nov-22 12:14 UTC
Re: [PATCH V4] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips
>>> On 22.11.13 at 11:44, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 21/11/13 22:50, Aravind Gopalakrishnan wrote: >> if ( uart->remapped_io_base ) >> + { >> + sfn = paddr_to_pfn((unsigned long) uart->io_base + PAGE_SIZE); >> + efn = paddr_to_pfn((unsigned long) uart->io_base + uart->io_size - 1); >> + if ( iomem_deny_access(dom0, sfn, efn) != 0 ) >> + BUG(); > > BUG_ON(!iomem_deny_access(dom0, sfn, efn))Actually, we had more or less agreed to avoid side effects in ASSERT() and BUG_ON() expressions (to eliminate the ambiguity whether such expressions get always evaluated).> is slightly more compact, and has the advantage of showing the action > which failed in the BUG message.Since when does BUG() should any expression? Jan
Jan Beulich
2013-Nov-22 12:29 UTC
Re: [PATCH V4] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips
>>> On 21.11.13 at 23:50, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> wrote: > @@ -432,9 +474,16 @@ static void __init ns16550_endboot(struct serial_port *port) > { > #ifdef HAS_IOPORTS > struct ns16550 *uart = port->uart; > - > + unsigned long sfn, efn; > +Stray blanks.> if ( uart->remapped_io_base ) > + { > + sfn = paddr_to_pfn((unsigned long) uart->io_base + PAGE_SIZE); > + efn = paddr_to_pfn((unsigned long) uart->io_base + uart->io_size - 1);These casts aren''t correct for 32-bit hosts (even if we don''t have any that define HAS_IOPORTS). And anyway, using PFN_UP() and PFN_DOWN() here respectively would be much easier to read.> + if ( iomem_deny_access(dom0, sfn, efn) != 0 )You need to handle the case of sfn > efn, I think.> + /* Handle 64 bit BAR if found */ > + if ( bar & PCI_BASE_ADDRESS_MEM_TYPE_64 ) > + {Do you really need that (and the respective changes elsewhere)? I''d be fine with simply bailing in that case for the time being.> + } > + else > + { > + len &= PCI_BASE_ADDRESS_MEM_MASK; > + uart->io_size = len & ~(len - 1);Once again: "len & -len" is the simpler alternative.> + /* IO based */ > + else > + { > + pci_conf_write32(0, b, d, f, > + PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u); > + len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0); > + pci_conf_write32(0, b, d, f, > + PCI_BASE_ADDRESS_0 + bar_idx*4, bar); > + len &= PCI_BASE_ADDRESS_IO_MASK; > + len &= ~(len - 1);len &= -len (PCI_BASE_ADDRESS_IO_MASK being ~0x03UL I don''t see why this would be incorrect, as you indicated in a reply to v3). Jan
Andrew Cooper
2013-Nov-22 13:09 UTC
Re: [PATCH V4] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips
On 22/11/13 12:14, Jan Beulich wrote:>>>> On 22.11.13 at 11:44, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> On 21/11/13 22:50, Aravind Gopalakrishnan wrote: >>> if ( uart->remapped_io_base ) >>> + { >>> + sfn = paddr_to_pfn((unsigned long) uart->io_base + PAGE_SIZE); >>> + efn = paddr_to_pfn((unsigned long) uart->io_base + uart->io_size - 1); >>> + if ( iomem_deny_access(dom0, sfn, efn) != 0 ) >>> + BUG(); >> BUG_ON(!iomem_deny_access(dom0, sfn, efn)) > Actually, we had more or less agreed to avoid side effects in > ASSERT() and BUG_ON() expressions (to eliminate the ambiguity > whether such expressions get always evaluated).ASSERT()s certain, as the non-debug builds will optimise away call. BUG/WARN_ON()s are different - they will be executed in all cases.> >> is slightly more compact, and has the advantage of showing the action >> which failed in the BUG message. > Since when does BUG() should any expression?Hmm - they don''t do they. I was getting my BUG()s and ASSERTS()s mixed up.
Jan Beulich
2013-Nov-22 13:12 UTC
Re: [PATCH V4] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips
>>> On 22.11.13 at 14:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 22/11/13 12:14, Jan Beulich wrote: >>>>> On 22.11.13 at 11:44, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>> On 21/11/13 22:50, Aravind Gopalakrishnan wrote: >>>> if ( uart->remapped_io_base ) >>>> + { >>>> + sfn = paddr_to_pfn((unsigned long) uart->io_base + PAGE_SIZE); >>>> + efn = paddr_to_pfn((unsigned long) uart->io_base + uart->io_size - 1); >>>> + if ( iomem_deny_access(dom0, sfn, efn) != 0 ) >>>> + BUG(); >>> BUG_ON(!iomem_deny_access(dom0, sfn, efn)) >> Actually, we had more or less agreed to avoid side effects in >> ASSERT() and BUG_ON() expressions (to eliminate the ambiguity >> whether such expressions get always evaluated). > > ASSERT()s certain, as the non-debug builds will optimise away call. > > BUG/WARN_ON()s are different - they will be executed in all cases.That''s currently the case, but since it''s different from ASSERT() it''s better to avoid the ambiguity. Jan
Aravind Gopalakrishnan
2013-Nov-22 15:01 UTC
Re: [PATCH V4] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips
On 11/22/2013 6:29 AM, Jan Beulich wrote:>>>> On 21.11.13 at 23:50, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> wrote: >> @@ -432,9 +474,16 @@ static void __init ns16550_endboot(struct serial_port *port) >> { >> #ifdef HAS_IOPORTS >> struct ns16550 *uart = port->uart; >> - >> + unsigned long sfn, efn; >> + > Stray blanks.Will fix this..>> if ( uart->remapped_io_base ) >> + { >> + sfn = paddr_to_pfn((unsigned long) uart->io_base + PAGE_SIZE); >> + efn = paddr_to_pfn((unsigned long) uart->io_base + uart->io_size - 1); > These casts aren''t correct for 32-bit hosts (even if we don''t have > any that define HAS_IOPORTS). And anyway, using PFN_UP() > and PFN_DOWN() here respectively would be much easier to read.Okay, will correct them..>> + if ( iomem_deny_access(dom0, sfn, efn) != 0 ) > You need to handle the case of sfn > efn, I think.Will do..>> + /* Handle 64 bit BAR if found */ >> + if ( bar & PCI_BASE_ADDRESS_MEM_TYPE_64 ) >> + { > Do you really need that (and the respective changes elsewhere)? > I''d be fine with simply bailing in that case for the time being.Yes, as the device has a 64 bit BAR.>> + } >> + else >> + { >> + len &= PCI_BASE_ADDRESS_MEM_MASK; >> + uart->io_size = len & ~(len - 1); > Once again: "len & -len" is the simpler alternative. > >> + /* IO based */ >> + else >> + { >> + pci_conf_write32(0, b, d, f, >> + PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u); >> + len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0); >> + pci_conf_write32(0, b, d, f, >> + PCI_BASE_ADDRESS_0 + bar_idx*4, bar); >> + len &= PCI_BASE_ADDRESS_IO_MASK; >> + len &= ~(len - 1); > len &= -len (PCI_BASE_ADDRESS_IO_MASK being ~0x03UL I don''t > see why this would be incorrect, as you indicated in a reply to v3). > > Jan > >Ah. I see it now.. (Originally I took it to mean just len = -(len).. which worked for mmio case...) Anyway, Will change it.. Thanks, -Aravind.