[PATCH V7] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[PATCH V7] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[Aravind Gopalakrishnan]

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 V6:
  - per Jan's comments:
    - Use bool_t for enable_ro
    - Reword comment
    - Use printk instead of WARN()
    - Use either pci_ro_device or pci_hide_device, not both.

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 | 178 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 158 insertions(+), 20 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 9c2cded..b30d426 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,14 +70,47 @@ 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;
+    bool_t enable_ro; /* Make MMIO devices read only to Dom0 */
 #endif
 #ifdef HAS_DEVICE_TREE
     struct dt_irq dt_irq;
 #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 +168,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 +194,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 +217,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)
@@ -353,7 +389,7 @@ static void __init ns16550_init_postirq(struct serial_port *port)
     ns16550_setup_postirq(uart);
 
 #ifdef HAS_PCI
-    if ( uart->bar || uart->ps_bdf_enable )
+    if ( (uart->bar || uart->ps_bdf_enable) && !(uart->enable_ro) )
         pci_hide_device(uart->ps_bdf[0], PCI_DEVFN(uart->ps_bdf[1],
                                                    uart->ps_bdf[2]));
 #endif
@@ -381,6 +417,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);
     }
@@ -434,7 +477,22 @@ static void __init ns16550_endboot(struct serial_port *port)
     struct ns16550 *uart = port->uart;
 
     if ( uart->remapped_io_base )
+    {
+        if ( uart->enable_ro ) {
+            if ( rangeset_add_range(mmio_ro_ranges,
+                                    uart->io_base,
+                                    uart->io_base + uart->io_size - 1) )
+                printk(XENLOG_INFO "Error while adding MMIO range of device to mmio_ro_ranges\n");
+
+            if ( pci_ro_device(0, uart->ps_bdf[0], 
+                               PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2])) )
+                printk(XENLOG_INFO "Could not mark config space of 0:%02x:%02x.%u read-only.\n",
+                                    uart->ps_bdf[0], uart->ps_bdf[1],
+                                    uart->ps_bdf[2]);
+        }
+
         return;
+    }
     if ( ioports_deny_access(dom0, uart->io_base, uart->io_base + 7) != 0 )
         BUG();
 #endif
@@ -546,11 +604,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 = 0, 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++ )
@@ -579,24 +639,98 @@ 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 */
+                /* 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);
+                        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;
+                    }
+                    else
+                        size = len & PCI_BASE_ADDRESS_MEM_MASK;
+
+                    size &= -size;
+
+                    /* 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;
+
+                        /* 
+                         * Force length of mmio region to be at least 
+                         * 8 bytes times (1 << reg_shift)
+                         */
+                        if ( size < (0x8 * (1 << uart_config[i].reg_shift)) )
+                            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)bar_64 << 32) |
+                                        (bar & PCI_BASE_ADDRESS_MEM_MASK);
+                        /* Set device and MMIO region read only to Dom0 */
+                        uart->enable_ro = 1;
+                        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);
+                    size = len & PCI_BASE_ADDRESS_IO_MASK;
+                    size &= -size;
+
+                    /* Not 8 bytes */
+                    if ( size != 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->bar = bar;
+                uart->bar64 = bar_64;
+                uart->io_size = size;
                 uart->irq = pci_conf_read8(0, b, d, f, PCI_INTERRUPT_PIN) ?
                     pci_conf_read8(0, b, d, f, PCI_INTERRUPT_LINE) : 0;
 
@@ -742,10 +876,14 @@ void __init ns16550_init(int index, struct ns16550_defaults *defaults)
     uart->io_size   = 8;
     uart->reg_width = 1;
     uart->reg_shift = 0;
+    uart->enable_ro = 0;
 
     /* 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

Re: [PATCH V7] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[Jan Beulich]

>>> On 03.12.13 at 17:40, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> wrote:
> @@ -434,7 +477,22 @@ static void __init ns16550_endboot(struct serial_port *port)
>      struct ns16550 *uart = port->uart;
>  
>      if ( uart->remapped_io_base )
> +    {
> +        if ( uart->enable_ro ) {
> +            if ( rangeset_add_range(mmio_ro_ranges,
> +                                    uart->io_base,
> +                                    uart->io_base + uart->io_size - 1) )
> +                printk(XENLOG_INFO "Error while adding MMIO range of device to mmio_ro_ranges\n");
> +
> +            if ( pci_ro_device(0, uart->ps_bdf[0], 
> +                               PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2])) )

You didn't really fix what you said you fixed. Quoting from my
reply to v6:

>> But, more importantly, did you overlook the use of pci_hide_device()
>> in ns16550_init_postirq(): The hiding should be done in one place.
>> And with pci_ro_device() implicitly hiding the device, you should
>> probably make sure you call just one of the two.

> +                printk(XENLOG_INFO "Could not mark config space of 0:%02x:%02x.%u read-only.\n",
> +                                    uart->ps_bdf[0], uart->ps_bdf[1],
> +                                    uart->ps_bdf[2]);

The leading 0: is pretty pointless - an absent segment/domain
identifier implies it being zero.

Jan

Re: [PATCH V7] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[Ian Campbell]

On Tue, 2013-12-03 at 10:40 -0600, 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 V6:
>   - per Jan's comments:
>     - Use bool_t for enable_ro
>     - Reword comment
>     - Use printk instead of WARN()
>     - Use either pci_ro_device or pci_hide_device, not both.
> 
> 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>

Build testing this on 32-bit arm resulted in a buch of warnings about
trailing whitespace from git am and:
        ns16550.c: In function ‘ns16550_init’:
        ns16550.c:879:9: error: ‘struct ns16550’ has no member named ‘enable_ro’
        ns16550.c: At top level:
        ns16550.c:100:78: error: ‘uart_config’ defined but not used [-Werror=unused-variable]
        cc1: all warnings being treated as errors

The hypervisor is pretty easy to cross compile, compilers are available
from https://launchpad.net/linaro-toolchain-binaries and then
        make XEN_TARGET_ARCH=arm32 CROSS_COMPILE=arm-blah-blah- xen -jN
similarly for arm64.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V7] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[Aravind Gopalakrishnan]

On 12/3/2013 10:54 AM, Ian Campbell wrote:
> On Tue, 2013-12-03 at 10:40 -0600, 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 V6:
>>    - per Jan's comments:
>>      - Use bool_t for enable_ro
>>      - Reword comment
>>      - Use printk instead of WARN()
>>      - Use either pci_ro_device or pci_hide_device, not both.
>>
>> 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>
> Build testing this on 32-bit arm resulted in a buch of warnings about
> trailing whitespace from git am and:
>          ns16550.c: In function ‘ns16550_init’:
>          ns16550.c:879:9: error: ‘struct ns16550’ has no member named ‘enable_ro’
>          ns16550.c: At top level:
>          ns16550.c:100:78: error: ‘uart_config’ defined but not used [-Werror=unused-variable]
>          cc1: all warnings being treated as errors

Corrected these now. Sending out changes in V8.

>
> The hypervisor is pretty easy to cross compile, compilers are available
> from https://launchpad.net/linaro-toolchain-binaries and then
>          make XEN_TARGET_ARCH=arm32 CROSS_COMPILE=arm-blah-blah- xen -jN
> similarly for arm64.
>
>
Cross-compiled to arm32 and arm64 on a local machine after applying 
patch(git didn't complain) and xen tree builds fine.

-Aravind.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V7] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[Aravind Gopalakrishnan]

On 12/3/2013 10:46 AM, Jan Beulich wrote:
>>>> On 03.12.13 at 17:40, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> wrote:
>> @@ -434,7 +477,22 @@ static void __init ns16550_endboot(struct serial_port *port)
>>       struct ns16550 *uart = port->uart;
>>   
>>       if ( uart->remapped_io_base )
>> +    {
>> +        if ( uart->enable_ro ) {
>> +            if ( rangeset_add_range(mmio_ro_ranges,
>> +                                    uart->io_base,
>> +                                    uart->io_base + uart->io_size - 1) )
>> +                printk(XENLOG_INFO "Error while adding MMIO range of device to mmio_ro_ranges\n");
>> +
>> +            if ( pci_ro_device(0, uart->ps_bdf[0],
>> +                               PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2])) )
> You didn't really fix what you said you fixed. Quoting from my
> reply to v6:

Ok, moved pci_ro_device also to ns16550_init_postirq now so that I do 
the hiding in one place..

>>> But, more importantly, did you overlook the use of pci_hide_device()
>>> in ns16550_init_postirq(): The hiding should be done in one place.
>>> And with pci_ro_device() implicitly hiding the device, you should
>>> probably make sure you call just one of the two.
>> +                printk(XENLOG_INFO "Could not mark config space of 0:%02x:%02x.%u read-only.\n",
>> +                                    uart->ps_bdf[0], uart->ps_bdf[1],
>> +                                    uart->ps_bdf[2]);
> The leading 0: is pretty pointless - an absent segment/domain
> identifier implies it being zero.
>
>
Fixed this. Sending out changes in V8.

-Aravind.