[PATCH 0/4] ns16550: enable support for Pericom controllers

[PATCH 0/4] ns16550: enable support for Pericom controllers

[Jan Beulich]

Patches 1 and 2 are meant to go in. Patch 3 is a prerequisite to patch
4 and may go in as well, but patch 4 is RFC because with the Pericom
board I have MSI doesn't appear to function. Since it also does not
work in baremetal Linux when doing the trivial adjustments needed in
its driver, I suspect the feature doesn't work in general, which is
supported by the observation that the device continues to assert
INTx despite the MSI enable bit being set (causing unclaimed IRQs
until that IRQ gets shut off). While I got some responses back from
Pericom support, no actual statement of whether MSI is actually
known to work on their boards was ever made by them. I _think_
patch 4 is correct (and hence could go in), but I have no way of
proving this by testing.

1: ns16550: store pointer to config parameters for PCI
2: ns16550: enable Pericom controller support
3: console: adjust IRQ initialization
4: ns16550: enable use of PCI MSI

Signed-off-by: Jan Beulich <jbeulich@suse.com>

(Konrad, I'd appreciate if you could double check that I didn't
accidentally break the Oxford controller support.)

[PATCH 1/4] ns16550: store pointer to config parameters for PCI

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4231 bytes --]

Subsequent changes will want to use this pointer.

This makes the enable_ro structure member redundant, so it gets dropped
at once.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -74,7 +74,7 @@ static struct ns16550 {
     u32 bar64;
     u16 cr;
     u8 bar_idx;
-    bool_t enable_ro; /* Make MMIO devices read only to Dom0 */
+    const struct ns16550_config_param *param; /* Points into .init.*! */
 #endif
 } ns16550_com[2] = { { 0 } };
 
@@ -624,7 +624,7 @@ static void __init ns16550_init_postirq(
 #ifdef CONFIG_HAS_PCI
     if ( uart->bar || uart->ps_bdf_enable )
     {
-        if ( !uart->enable_ro )
+        if ( !uart->param )
             pci_hide_device(uart->ps_bdf[0], PCI_DEVFN(uart->ps_bdf[1],
                             uart->ps_bdf[2]));
         else
@@ -900,7 +900,7 @@ pci_uart_config(struct ns16550 *uart, bo
                     /* Check for params in uart_config lookup table */
                     for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
                     {
-                        unsigned int p;
+                        const struct ns16550_config_param *param;
 
                         if ( uart_config[i].vendor_id != vendor )
                             continue;
@@ -908,33 +908,34 @@ pci_uart_config(struct ns16550 *uart, bo
                         if ( uart_config[i].dev_id != device )
                             continue;
 
-                        p = uart_config[i].param;
+                        param = uart_param + uart_config[i].param;
+
                         /*
                          * Force length of mmio region to be at least
                          * 8 bytes times (1 << reg_shift)
                          */
-                        if ( size < (0x8 * (1 << uart_param[p].reg_shift)) )
+                        if ( size < (0x8 * (1 << param->reg_shift)) )
                             continue;
 
-                        if ( bar_idx >= uart_param[p].max_bars )
+                        if ( bar_idx >= param->max_bars )
                             continue;
 
-                        if ( uart_param[p].fifo_size )
-                            uart->fifo_size = uart_param[p].fifo_size;
+                        uart->param = param;
+
+                        if ( param->fifo_size )
+                            uart->fifo_size = param->fifo_size;
 
-                        uart->reg_shift = uart_param[p].reg_shift;
-                        uart->reg_width = uart_param[p].reg_width;
-                        uart->lsr_mask = uart_param[p].lsr_mask;
+                        uart->reg_shift = param->reg_shift;
+                        uart->reg_width = param->reg_width;
+                        uart->lsr_mask = param->lsr_mask;
                         uart->io_base = ((u64)bar_64 << 32) |
                                         (bar & PCI_BASE_ADDRESS_MEM_MASK);
-                        uart->io_base += uart_param[p].first_offset;
-                        uart->io_base += bar_idx * uart_param[p].uart_offset;
-                        if ( uart_param[p].base_baud )
-                            uart->clock_hz = uart_param[p].base_baud * 16;
-                        size = max(8U << uart_param[p].reg_shift,
-                                   uart_param[p].uart_offset);
-                        /* Set device and MMIO region read only to Dom0 */
-                        uart->enable_ro = 1;
+                        uart->io_base += param->first_offset;
+                        uart->io_base += bar_idx * param->uart_offset;
+                        if ( param->base_baud )
+                            uart->clock_hz = param->base_baud * 16;
+                        size = max(8U << param->reg_shift,
+                                   param->uart_offset);
                         break;
                     }
 
@@ -1094,10 +1095,6 @@ static void ns16550_init_common(struct n
 {
     uart->clock_hz  = UART_CLOCK_HZ;
 
-#ifdef CONFIG_HAS_PCI
-    uart->enable_ro = 0;
-#endif
-
     /* Default is no transmit FIFO. */
     uart->fifo_size = 1;
 



[-- Attachment #2: ns16550-replace-enable_ro.patch --]
[-- Type: text/plain, Size: 4282 bytes --]

ns16550: store pointer to config parameters for PCI

Subsequent changes will want to use this pointer.

This makes the enable_ro structure member redundant, so it gets dropped
at once.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -74,7 +74,7 @@ static struct ns16550 {
     u32 bar64;
     u16 cr;
     u8 bar_idx;
-    bool_t enable_ro; /* Make MMIO devices read only to Dom0 */
+    const struct ns16550_config_param *param; /* Points into .init.*! */
 #endif
 } ns16550_com[2] = { { 0 } };
 
@@ -624,7 +624,7 @@ static void __init ns16550_init_postirq(
 #ifdef CONFIG_HAS_PCI
     if ( uart->bar || uart->ps_bdf_enable )
     {
-        if ( !uart->enable_ro )
+        if ( !uart->param )
             pci_hide_device(uart->ps_bdf[0], PCI_DEVFN(uart->ps_bdf[1],
                             uart->ps_bdf[2]));
         else
@@ -900,7 +900,7 @@ pci_uart_config(struct ns16550 *uart, bo
                     /* Check for params in uart_config lookup table */
                     for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
                     {
-                        unsigned int p;
+                        const struct ns16550_config_param *param;
 
                         if ( uart_config[i].vendor_id != vendor )
                             continue;
@@ -908,33 +908,34 @@ pci_uart_config(struct ns16550 *uart, bo
                         if ( uart_config[i].dev_id != device )
                             continue;
 
-                        p = uart_config[i].param;
+                        param = uart_param + uart_config[i].param;
+
                         /*
                          * Force length of mmio region to be at least
                          * 8 bytes times (1 << reg_shift)
                          */
-                        if ( size < (0x8 * (1 << uart_param[p].reg_shift)) )
+                        if ( size < (0x8 * (1 << param->reg_shift)) )
                             continue;
 
-                        if ( bar_idx >= uart_param[p].max_bars )
+                        if ( bar_idx >= param->max_bars )
                             continue;
 
-                        if ( uart_param[p].fifo_size )
-                            uart->fifo_size = uart_param[p].fifo_size;
+                        uart->param = param;
+
+                        if ( param->fifo_size )
+                            uart->fifo_size = param->fifo_size;
 
-                        uart->reg_shift = uart_param[p].reg_shift;
-                        uart->reg_width = uart_param[p].reg_width;
-                        uart->lsr_mask = uart_param[p].lsr_mask;
+                        uart->reg_shift = param->reg_shift;
+                        uart->reg_width = param->reg_width;
+                        uart->lsr_mask = param->lsr_mask;
                         uart->io_base = ((u64)bar_64 << 32) |
                                         (bar & PCI_BASE_ADDRESS_MEM_MASK);
-                        uart->io_base += uart_param[p].first_offset;
-                        uart->io_base += bar_idx * uart_param[p].uart_offset;
-                        if ( uart_param[p].base_baud )
-                            uart->clock_hz = uart_param[p].base_baud * 16;
-                        size = max(8U << uart_param[p].reg_shift,
-                                   uart_param[p].uart_offset);
-                        /* Set device and MMIO region read only to Dom0 */
-                        uart->enable_ro = 1;
+                        uart->io_base += param->first_offset;
+                        uart->io_base += bar_idx * param->uart_offset;
+                        if ( param->base_baud )
+                            uart->clock_hz = param->base_baud * 16;
+                        size = max(8U << param->reg_shift,
+                                   param->uart_offset);
                         break;
                     }
 
@@ -1094,10 +1095,6 @@ static void ns16550_init_common(struct n
 {
     uart->clock_hz  = UART_CLOCK_HZ;
 
-#ifdef CONFIG_HAS_PCI
-    uart->enable_ro = 0;
-#endif
-
     /* Default is no transmit FIFO. */
     uart->fifo_size = 1;
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 1/4] ns16550: store pointer to config parameters for PCI

[Konrad Rzeszutek Wilk]

On Tue, Feb 23, 2016 at 04:28:18AM -0700, Jan Beulich wrote:
> Subsequent changes will want to use this pointer.
> 
> This makes the enable_ro structure member redundant, so it gets dropped
> at once.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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

[PATCH 2/4] ns16550: enable Pericom controller support

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 13748 bytes --]

Other than the controllers supported so far, multiple port Pericom
boards map all of their ports via BAR0, which requires a number of
adjustments: Instead of tracking "max_bars" we now flag whether all
ports use BAR0, and whether to expect a port-I/O or MMIO resource. As
a result pci_uart_config() now gets handed a port index, which it then
maps into a BAR index or an offset into BAR0 depending on the bar0
flag.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -78,10 +78,20 @@ static struct ns16550 {
 #endif
 } ns16550_com[2] = { { 0 } };
 
-struct ns16550_config_mmio {
+#ifdef CONFIG_HAS_PCI
+struct ns16550_config {
     u16 vendor_id;
     u16 dev_id;
-    unsigned int param;
+    enum {
+        param_default, /* Must not be referenced by any table entry. */
+        param_trumanage,
+        param_oxford,
+        param_oxford_2port,
+        param_pericom_1port,
+        param_pericom_2port,
+        param_pericom_4port,
+        param_pericom_8port,
+    } param;
 };
 
 /* Defining uart config options for MMIO devices */
@@ -90,57 +100,91 @@ struct ns16550_config_param {
     unsigned int reg_width;
     unsigned int fifo_size;
     u8 lsr_mask;
-    unsigned int max_bars;
+    bool_t mmio;
+    bool_t bar0;
+    unsigned int max_ports;
     unsigned int base_baud;
     unsigned int uart_offset;
     unsigned int first_offset;
 };
 
-
-#ifdef CONFIG_HAS_PCI
-enum {
-    param_default = 0,
-    param_trumanage,
-    param_oxford,
-    param_oxford_2port,
-};
 /*
- * 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.
+ * Create lookup tables for specific devices. It is assumed that if
+ * the device found is MMIO, then you have indexed it here. Else, the
+ * driver does nothing for MMIO based devices.
  */
 static const struct ns16550_config_param __initconst uart_param[] = {
-    [param_default] = { }, /* Ignored. */
+    [param_default] = {
+        .reg_width = 1,
+        .lsr_mask = UART_LSR_THRE,
+        .max_ports = 1,
+    },
     [param_trumanage] = {
         .reg_shift = 2,
         .reg_width = 1,
         .fifo_size = 16,
         .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
-        .max_bars = 1,
+        .mmio = 1,
+        .max_ports = 1,
     },
     [param_oxford] = {
         .base_baud = 4000000,
         .uart_offset = 0x200,
         .first_offset = 0x1000,
         .reg_width = 1,
-        .reg_shift = 0,
         .fifo_size = 16,
         .lsr_mask = UART_LSR_THRE,
-        .max_bars = 1, /* It can do more, but we would need more custom code.*/
+        .mmio = 1,
+        .max_ports = 1, /* It can do more, but we would need more custom code.*/
     },
     [param_oxford_2port] = {
         .base_baud = 4000000,
         .uart_offset = 0x200,
         .first_offset = 0x1000,
         .reg_width = 1,
-        .reg_shift = 0,
         .fifo_size = 16,
         .lsr_mask = UART_LSR_THRE,
-        .max_bars = 2,
+        .mmio = 1,
+        .max_ports = 2,
+    },
+    [param_pericom_1port] = {
+        .base_baud = 921600,
+        .uart_offset = 8,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .max_ports = 1,
+    },
+    [param_pericom_2port] = {
+        .base_baud = 921600,
+        .uart_offset = 8,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .max_ports = 2,
+    },
+    [param_pericom_4port] = {
+        .base_baud = 921600,
+        .uart_offset = 8,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .max_ports = 4,
+    },
+    [param_pericom_8port] = {
+        .base_baud = 921600,
+        .uart_offset = 8,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .max_ports = 8,
     }
 };
-static const struct ns16550_config_mmio __initconst uart_config[] =
+static const struct ns16550_config __initconst uart_config[] =
 {
     /* Broadcom TruManage device */
     {
@@ -339,6 +383,30 @@ static const struct ns16550_config_mmio
         .vendor_id = PCI_VENDOR_ID_OXSEMI,
         .dev_id = 0xc4cf,
         .param = param_oxford,
+    },
+    /* Pericom PI7C9X7951 Uno UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_PERICOM,
+        .dev_id = 0x7951,
+        .param = param_pericom_1port
+    },
+    /* Pericom PI7C9X7952 Duo UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_PERICOM,
+        .dev_id = 0x7952,
+        .param = param_pericom_2port
+    },
+    /* Pericom PI7C9X7954 Quad UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_PERICOM,
+        .dev_id = 0x7954,
+        .param = param_pericom_4port
+    },
+    /* Pericom PI7C9X7958 Octal UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_PERICOM,
+        .dev_id = 0x7958,
+        .param = param_pericom_8port
     }
 };
 #endif
@@ -629,7 +697,8 @@ static void __init ns16550_init_postirq(
                             uart->ps_bdf[2]));
         else
         {
-            if ( rangeset_add_range(mmio_ro_ranges,
+            if ( uart->param->mmio &&
+                 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");
@@ -830,12 +899,11 @@ static int __init check_existence(struct
 
 #ifdef CONFIG_HAS_PCI
 static int __init
-pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int bar_idx)
+pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
 {
     u64 orig_base = uart->io_base;
     unsigned int b, d, f, nextf, i;
 
-    uart->io_base = 0;
     /* 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++ )
     {
@@ -843,8 +911,10 @@ pci_uart_config(struct ns16550 *uart, bo
         {
             for ( f = 0; f < 8; f = nextf )
             {
+                unsigned int bar_idx = 0, port_idx = idx;
                 uint32_t bar, bar_64 = 0, len, len_64;
-                u64 size;
+                u64 size = 0;
+                const struct ns16550_config_param *param = uart_param;
 
                 nextf = (f || (pci_conf_read16(0, b, d, f, PCI_HEADER_TYPE) &
                                0x80)) ? f + 1 : 8;
@@ -863,15 +933,38 @@ pci_uart_config(struct ns16550 *uart, bo
                     continue;
                 }
 
+                /* Check for params in uart_config lookup table */
+                for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
+                {
+                    u16 vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
+                    u16 device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID);
+
+                    if ( uart_config[i].vendor_id == vendor &&
+                         uart_config[i].dev_id == device )
+                    {
+                        param += uart_config[i].param;
+                        if ( !param->bar0 )
+                        {
+                            bar_idx = idx;
+                            port_idx = 0;
+                        }
+                        break;
+                    }
+                }
+
+                if ( port_idx >= param->max_ports )
+                {
+                    idx -= param->max_ports;
+                    continue;
+                }
+
+                uart->io_base = 0;
                 bar = pci_conf_read32(0, b, d, f,
                                       PCI_BASE_ADDRESS_0 + bar_idx*4);
 
                 /* MMIO based */
-                if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
+                if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
                 {
-                    u16 vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
-                    u16 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);
@@ -895,56 +988,11 @@ pci_uart_config(struct ns16550 *uart, bo
                     else
                         size = len & PCI_BASE_ADDRESS_MEM_MASK;
 
-                    size &= -size;
-
-                    /* Check for params in uart_config lookup table */
-                    for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
-                    {
-                        const struct ns16550_config_param *param;
-
-                        if ( uart_config[i].vendor_id != vendor )
-                            continue;
-
-                        if ( uart_config[i].dev_id != device )
-                            continue;
-
-                        param = uart_param + uart_config[i].param;
-
-                        /*
-                         * Force length of mmio region to be at least
-                         * 8 bytes times (1 << reg_shift)
-                         */
-                        if ( size < (0x8 * (1 << param->reg_shift)) )
-                            continue;
-
-                        if ( bar_idx >= param->max_bars )
-                            continue;
-
-                        uart->param = param;
-
-                        if ( param->fifo_size )
-                            uart->fifo_size = param->fifo_size;
-
-                        uart->reg_shift = param->reg_shift;
-                        uart->reg_width = param->reg_width;
-                        uart->lsr_mask = param->lsr_mask;
-                        uart->io_base = ((u64)bar_64 << 32) |
-                                        (bar & PCI_BASE_ADDRESS_MEM_MASK);
-                        uart->io_base += param->first_offset;
-                        uart->io_base += bar_idx * param->uart_offset;
-                        if ( param->base_baud )
-                            uart->clock_hz = param->base_baud * 16;
-                        size = max(8U << param->reg_shift,
-                                   param->uart_offset);
-                        break;
-                    }
-
-                    /* If we have an io_base, then we succeeded in the lookup */
-                    if ( !uart->io_base )
-                        continue;
+                    uart->io_base = ((u64)bar_64 << 32) |
+                                    (bar & PCI_BASE_ADDRESS_MEM_MASK);
                 }
                 /* IO based */
-                else
+                else if ( !param->mmio && (bar & PCI_BASE_ADDRESS_SPACE_IO) )
                 {
                     pci_conf_write32(0, b, d, f,
                                      PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
@@ -952,22 +1000,45 @@ pci_uart_config(struct ns16550 *uart, bo
                     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 at least 8 bytes */
-                    if ( size < 8 )
-                        continue;
 
                     uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO;
                 }
 
+                /* If we have an io_base, then we succeeded in the lookup. */
+                if ( !uart->io_base )
+                    continue;
+
+                size &= -size;
+
+                /*
+                 * Require length of actually used region to be at least
+                 * 8 bytes times (1 << reg_shift).
+                 */
+                if ( size < param->first_offset +
+                            port_idx * param->uart_offset +
+                            (8 << param->reg_shift) )
+                    continue;
+
+                uart->param = param;
+
+                uart->reg_shift = param->reg_shift;
+                uart->reg_width = param->reg_width;
+                uart->lsr_mask = param->lsr_mask;
+                uart->io_base += param->first_offset +
+                                 port_idx * param->uart_offset;
+                if ( param->base_baud )
+                    uart->clock_hz = param->base_baud * 16;
+                if ( param->fifo_size )
+                    uart->fifo_size = param->fifo_size;
+
                 uart->ps_bdf[0] = b;
                 uart->ps_bdf[1] = d;
                 uart->ps_bdf[2] = f;
                 uart->bar_idx = bar_idx;
                 uart->bar = bar;
                 uart->bar64 = bar_64;
-                uart->io_size = size;
+                uart->io_size = max(8U << param->reg_shift,
+                                    param->uart_offset);
                 uart->irq = pci_conf_read8(0, b, d, f, PCI_INTERRUPT_PIN) ?
                     pci_conf_read8(0, b, d, f, PCI_INTERRUPT_LINE) : 0;
 
--- a/xen/include/xen/pci_ids.h
+++ b/xen/include/xen/pci_ids.h
@@ -2,6 +2,8 @@
 
 #define PCI_VENDOR_ID_NVIDIA             0x10de
 
+#define PCI_VENDOR_ID_PERICOM            0x12d8
+
 #define PCI_VENDOR_ID_OXSEMI             0x1415
 
 #define PCI_VENDOR_ID_BROADCOM           0x14e4



[-- Attachment #2: ns16550-Pericom.patch --]
[-- Type: text/plain, Size: 13790 bytes --]

ns16550: enable Pericom controller support

Other than the controllers supported so far, multiple port Pericom
boards map all of their ports via BAR0, which requires a number of
adjustments: Instead of tracking "max_bars" we now flag whether all
ports use BAR0, and whether to expect a port-I/O or MMIO resource. As
a result pci_uart_config() now gets handed a port index, which it then
maps into a BAR index or an offset into BAR0 depending on the bar0
flag.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -78,10 +78,20 @@ static struct ns16550 {
 #endif
 } ns16550_com[2] = { { 0 } };
 
-struct ns16550_config_mmio {
+#ifdef CONFIG_HAS_PCI
+struct ns16550_config {
     u16 vendor_id;
     u16 dev_id;
-    unsigned int param;
+    enum {
+        param_default, /* Must not be referenced by any table entry. */
+        param_trumanage,
+        param_oxford,
+        param_oxford_2port,
+        param_pericom_1port,
+        param_pericom_2port,
+        param_pericom_4port,
+        param_pericom_8port,
+    } param;
 };
 
 /* Defining uart config options for MMIO devices */
@@ -90,57 +100,91 @@ struct ns16550_config_param {
     unsigned int reg_width;
     unsigned int fifo_size;
     u8 lsr_mask;
-    unsigned int max_bars;
+    bool_t mmio;
+    bool_t bar0;
+    unsigned int max_ports;
     unsigned int base_baud;
     unsigned int uart_offset;
     unsigned int first_offset;
 };
 
-
-#ifdef CONFIG_HAS_PCI
-enum {
-    param_default = 0,
-    param_trumanage,
-    param_oxford,
-    param_oxford_2port,
-};
 /*
- * 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.
+ * Create lookup tables for specific devices. It is assumed that if
+ * the device found is MMIO, then you have indexed it here. Else, the
+ * driver does nothing for MMIO based devices.
  */
 static const struct ns16550_config_param __initconst uart_param[] = {
-    [param_default] = { }, /* Ignored. */
+    [param_default] = {
+        .reg_width = 1,
+        .lsr_mask = UART_LSR_THRE,
+        .max_ports = 1,
+    },
     [param_trumanage] = {
         .reg_shift = 2,
         .reg_width = 1,
         .fifo_size = 16,
         .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
-        .max_bars = 1,
+        .mmio = 1,
+        .max_ports = 1,
     },
     [param_oxford] = {
         .base_baud = 4000000,
         .uart_offset = 0x200,
         .first_offset = 0x1000,
         .reg_width = 1,
-        .reg_shift = 0,
         .fifo_size = 16,
         .lsr_mask = UART_LSR_THRE,
-        .max_bars = 1, /* It can do more, but we would need more custom code.*/
+        .mmio = 1,
+        .max_ports = 1, /* It can do more, but we would need more custom code.*/
     },
     [param_oxford_2port] = {
         .base_baud = 4000000,
         .uart_offset = 0x200,
         .first_offset = 0x1000,
         .reg_width = 1,
-        .reg_shift = 0,
         .fifo_size = 16,
         .lsr_mask = UART_LSR_THRE,
-        .max_bars = 2,
+        .mmio = 1,
+        .max_ports = 2,
+    },
+    [param_pericom_1port] = {
+        .base_baud = 921600,
+        .uart_offset = 8,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .max_ports = 1,
+    },
+    [param_pericom_2port] = {
+        .base_baud = 921600,
+        .uart_offset = 8,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .max_ports = 2,
+    },
+    [param_pericom_4port] = {
+        .base_baud = 921600,
+        .uart_offset = 8,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .max_ports = 4,
+    },
+    [param_pericom_8port] = {
+        .base_baud = 921600,
+        .uart_offset = 8,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .max_ports = 8,
     }
 };
-static const struct ns16550_config_mmio __initconst uart_config[] =
+static const struct ns16550_config __initconst uart_config[] =
 {
     /* Broadcom TruManage device */
     {
@@ -339,6 +383,30 @@ static const struct ns16550_config_mmio
         .vendor_id = PCI_VENDOR_ID_OXSEMI,
         .dev_id = 0xc4cf,
         .param = param_oxford,
+    },
+    /* Pericom PI7C9X7951 Uno UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_PERICOM,
+        .dev_id = 0x7951,
+        .param = param_pericom_1port
+    },
+    /* Pericom PI7C9X7952 Duo UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_PERICOM,
+        .dev_id = 0x7952,
+        .param = param_pericom_2port
+    },
+    /* Pericom PI7C9X7954 Quad UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_PERICOM,
+        .dev_id = 0x7954,
+        .param = param_pericom_4port
+    },
+    /* Pericom PI7C9X7958 Octal UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_PERICOM,
+        .dev_id = 0x7958,
+        .param = param_pericom_8port
     }
 };
 #endif
@@ -629,7 +697,8 @@ static void __init ns16550_init_postirq(
                             uart->ps_bdf[2]));
         else
         {
-            if ( rangeset_add_range(mmio_ro_ranges,
+            if ( uart->param->mmio &&
+                 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");
@@ -830,12 +899,11 @@ static int __init check_existence(struct
 
 #ifdef CONFIG_HAS_PCI
 static int __init
-pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int bar_idx)
+pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
 {
     u64 orig_base = uart->io_base;
     unsigned int b, d, f, nextf, i;
 
-    uart->io_base = 0;
     /* 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++ )
     {
@@ -843,8 +911,10 @@ pci_uart_config(struct ns16550 *uart, bo
         {
             for ( f = 0; f < 8; f = nextf )
             {
+                unsigned int bar_idx = 0, port_idx = idx;
                 uint32_t bar, bar_64 = 0, len, len_64;
-                u64 size;
+                u64 size = 0;
+                const struct ns16550_config_param *param = uart_param;
 
                 nextf = (f || (pci_conf_read16(0, b, d, f, PCI_HEADER_TYPE) &
                                0x80)) ? f + 1 : 8;
@@ -863,15 +933,38 @@ pci_uart_config(struct ns16550 *uart, bo
                     continue;
                 }
 
+                /* Check for params in uart_config lookup table */
+                for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
+                {
+                    u16 vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
+                    u16 device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID);
+
+                    if ( uart_config[i].vendor_id == vendor &&
+                         uart_config[i].dev_id == device )
+                    {
+                        param += uart_config[i].param;
+                        if ( !param->bar0 )
+                        {
+                            bar_idx = idx;
+                            port_idx = 0;
+                        }
+                        break;
+                    }
+                }
+
+                if ( port_idx >= param->max_ports )
+                {
+                    idx -= param->max_ports;
+                    continue;
+                }
+
+                uart->io_base = 0;
                 bar = pci_conf_read32(0, b, d, f,
                                       PCI_BASE_ADDRESS_0 + bar_idx*4);
 
                 /* MMIO based */
-                if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
+                if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
                 {
-                    u16 vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
-                    u16 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);
@@ -895,56 +988,11 @@ pci_uart_config(struct ns16550 *uart, bo
                     else
                         size = len & PCI_BASE_ADDRESS_MEM_MASK;
 
-                    size &= -size;
-
-                    /* Check for params in uart_config lookup table */
-                    for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
-                    {
-                        const struct ns16550_config_param *param;
-
-                        if ( uart_config[i].vendor_id != vendor )
-                            continue;
-
-                        if ( uart_config[i].dev_id != device )
-                            continue;
-
-                        param = uart_param + uart_config[i].param;
-
-                        /*
-                         * Force length of mmio region to be at least
-                         * 8 bytes times (1 << reg_shift)
-                         */
-                        if ( size < (0x8 * (1 << param->reg_shift)) )
-                            continue;
-
-                        if ( bar_idx >= param->max_bars )
-                            continue;
-
-                        uart->param = param;
-
-                        if ( param->fifo_size )
-                            uart->fifo_size = param->fifo_size;
-
-                        uart->reg_shift = param->reg_shift;
-                        uart->reg_width = param->reg_width;
-                        uart->lsr_mask = param->lsr_mask;
-                        uart->io_base = ((u64)bar_64 << 32) |
-                                        (bar & PCI_BASE_ADDRESS_MEM_MASK);
-                        uart->io_base += param->first_offset;
-                        uart->io_base += bar_idx * param->uart_offset;
-                        if ( param->base_baud )
-                            uart->clock_hz = param->base_baud * 16;
-                        size = max(8U << param->reg_shift,
-                                   param->uart_offset);
-                        break;
-                    }
-
-                    /* If we have an io_base, then we succeeded in the lookup */
-                    if ( !uart->io_base )
-                        continue;
+                    uart->io_base = ((u64)bar_64 << 32) |
+                                    (bar & PCI_BASE_ADDRESS_MEM_MASK);
                 }
                 /* IO based */
-                else
+                else if ( !param->mmio && (bar & PCI_BASE_ADDRESS_SPACE_IO) )
                 {
                     pci_conf_write32(0, b, d, f,
                                      PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
@@ -952,22 +1000,45 @@ pci_uart_config(struct ns16550 *uart, bo
                     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 at least 8 bytes */
-                    if ( size < 8 )
-                        continue;
 
                     uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO;
                 }
 
+                /* If we have an io_base, then we succeeded in the lookup. */
+                if ( !uart->io_base )
+                    continue;
+
+                size &= -size;
+
+                /*
+                 * Require length of actually used region to be at least
+                 * 8 bytes times (1 << reg_shift).
+                 */
+                if ( size < param->first_offset +
+                            port_idx * param->uart_offset +
+                            (8 << param->reg_shift) )
+                    continue;
+
+                uart->param = param;
+
+                uart->reg_shift = param->reg_shift;
+                uart->reg_width = param->reg_width;
+                uart->lsr_mask = param->lsr_mask;
+                uart->io_base += param->first_offset +
+                                 port_idx * param->uart_offset;
+                if ( param->base_baud )
+                    uart->clock_hz = param->base_baud * 16;
+                if ( param->fifo_size )
+                    uart->fifo_size = param->fifo_size;
+
                 uart->ps_bdf[0] = b;
                 uart->ps_bdf[1] = d;
                 uart->ps_bdf[2] = f;
                 uart->bar_idx = bar_idx;
                 uart->bar = bar;
                 uart->bar64 = bar_64;
-                uart->io_size = size;
+                uart->io_size = max(8U << param->reg_shift,
+                                    param->uart_offset);
                 uart->irq = pci_conf_read8(0, b, d, f, PCI_INTERRUPT_PIN) ?
                     pci_conf_read8(0, b, d, f, PCI_INTERRUPT_LINE) : 0;
 
--- a/xen/include/xen/pci_ids.h
+++ b/xen/include/xen/pci_ids.h
@@ -2,6 +2,8 @@
 
 #define PCI_VENDOR_ID_NVIDIA             0x10de
 
+#define PCI_VENDOR_ID_PERICOM            0x12d8
+
 #define PCI_VENDOR_ID_OXSEMI             0x1415
 
 #define PCI_VENDOR_ID_BROADCOM           0x14e4

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2/4] ns16550: enable Pericom controller support

[Konrad Rzeszutek Wilk]

> +    [param_pericom_4port] = {
> +        .base_baud = 921600,
> +        .uart_offset = 8,
> +        .reg_width = 1,
> +        .fifo_size = 16,
> +        .lsr_mask = UART_LSR_THRE,
> +        .bar0 = 1,
> +        .max_ports = 4,
> +    },
> +    [param_pericom_8port] = {
> +        .base_baud = 921600,
> +        .uart_offset = 8,
> +        .reg_width = 1,
> +        .fifo_size = 16,
> +        .lsr_mask = UART_LSR_THRE,
> +        .bar0 = 1,
> +        .max_ports = 8,

Perhaps document that Xen can only access two of the ports? Unless we
expand the ns16550_com array of course.

> @@ -830,12 +899,11 @@ static int __init check_existence(struct
>  
>  #ifdef CONFIG_HAS_PCI
>  static int __init
> -pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int bar_idx)
> +pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
>  {
>      u64 orig_base = uart->io_base;
>      unsigned int b, d, f, nextf, i;
>  
> -    uart->io_base = 0;
>      /* 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++ )
>      {
> @@ -843,8 +911,10 @@ pci_uart_config(struct ns16550 *uart, bo
>          {
>              for ( f = 0; f < 8; f = nextf )
>              {
> +                unsigned int bar_idx = 0, port_idx = idx;

s/port_idx/port/? or port_nr /?

>                  uint32_t bar, bar_64 = 0, len, len_64;
> -                u64 size;
> +                u64 size = 0;
> +                const struct ns16550_config_param *param = uart_param;
>  
>                  nextf = (f || (pci_conf_read16(0, b, d, f, PCI_HEADER_TYPE) &
>                                 0x80)) ? f + 1 : 8;
> @@ -863,15 +933,38 @@ pci_uart_config(struct ns16550 *uart, bo
>                      continue;
>                  }
>  
> +                /* Check for params in uart_config lookup table */
> +                for ( i = 0; i < ARRAY_SIZE(uart_config); i++)

I am pretty sure I wrote this piece of code - could you fix the
Style on it please? The i++) please?
> +                {
> +                    u16 vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
> +                    u16 device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID);
> +
> +                    if ( uart_config[i].vendor_id == vendor &&
> +                         uart_config[i].dev_id == device )
> +                    {
> +                        param += uart_config[i].param;
> +                        if ( !param->bar0 )
> +                        {
> +                            bar_idx = idx;
> +                            port_idx = 0;
> +                        }
> +                        break;
> +                    }
> +                }
> +
> +                if ( port_idx >= param->max_ports )
> +                {
> +                    idx -= param->max_ports;
> +                    continue;

Could you add a comment about this? I understand it can detect if we are
using an AMT device with the 'com2=115200,8n1,amt' (which would be
invalid - AMT devices only have one IO PORT and there is only one of
them on the machine) we would skip over the found device and continue on..
Thought I don't understand why we want to decrease the idx value from one to zero?

Hmm, if it was some other PCI based serial card like:

01:05.0 Serial controller: NetMos Technology PCI 9835 Multi-I/O
Controller (rev 01) (prog-if 02 [16550])
        Subsystem: LSI Logic / Symbios Logic Device 0001
        Flags: medium devsel, IRQ 20
        I/O ports at e050 [size=8]
        I/O ports at e040 [size=8]
        I/O ports at e030 [size=8]
        I/O ports at e020 [size=8]
        I/O ports at e010 [size=8]
        I/O ports at e000 [size=16]

With 'com1=115200,8n1,pci' and 'com2=115200,8n1,pci' then the first loop
would find the device. The second loop would decrement idx (1) by 1 and
continue.. which would make it go search for another device.

I hadn't tested this patch on the above device but I believe it used
to work with the com1 and com2 going throught it - while with the new code
it won't?


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

Re: [PATCH 2/4] ns16550: enable Pericom controller support

[Jan Beulich]

>>> On 07.03.16 at 23:04, <konrad.wilk@oracle.com> wrote:
>>  +    [param_pericom_4port] = {
>> +        .base_baud = 921600,
>> +        .uart_offset = 8,
>> +        .reg_width = 1,
>> +        .fifo_size = 16,
>> +        .lsr_mask = UART_LSR_THRE,
>> +        .bar0 = 1,
>> +        .max_ports = 4,
>> +    },
>> +    [param_pericom_8port] = {
>> +        .base_baud = 921600,
>> +        .uart_offset = 8,
>> +        .reg_width = 1,
>> +        .fifo_size = 16,
>> +        .lsr_mask = UART_LSR_THRE,
>> +        .bar0 = 1,
>> +        .max_ports = 8,
> 
> Perhaps document that Xen can only access two of the ports? Unless we
> expand the ns16550_com array of course.

Done.

>> @@ -843,8 +911,10 @@ pci_uart_config(struct ns16550 *uart, bo
>>          {
>>              for ( f = 0; f < 8; f = nextf )
>>              {
>> +                unsigned int bar_idx = 0, port_idx = idx;
> 
> s/port_idx/port/? or port_nr /?

"port" would be misleading/ambiguous, and I don't see port_nr being
any better than port_idx (or if so, it ought to then also be bar_nr).
In fact, "nr" - other than "idx" - is ambiguous too (commonly
indicating "number of ...").

>> @@ -863,15 +933,38 @@ pci_uart_config(struct ns16550 *uart, bo
>>                      continue;
>>                  }
>>  
>> +                /* Check for params in uart_config lookup table */
>> +                for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
> 
> I am pretty sure I wrote this piece of code - could you fix the
> Style on it please? The i++) please?

Sure.

>> +                if ( port_idx >= param->max_ports )
>> +                {
>> +                    idx -= param->max_ports;
>> +                    continue;
> 
> Could you add a comment about this? I understand it can detect if we are
> using an AMT device with the 'com2=115200,8n1,amt' (which would be
> invalid - AMT devices only have one IO PORT and there is only one of
> them on the machine) we would skip over the found device and continue on..
> Thought I don't understand why we want to decrease the idx value from one to 
> zero?

If we're looking for COM2 and have found a 1-port card, we want to
use the 1st (rather than the 2nd) port on the next card we may find
(if any). This seems pretty obvious behavior to me here, so I'm not
really convinced a comment is warranted.

> Hmm, if it was some other PCI based serial card like:
> 
> 01:05.0 Serial controller: NetMos Technology PCI 9835 Multi-I/O
> Controller (rev 01) (prog-if 02 [16550])
>         Subsystem: LSI Logic / Symbios Logic Device 0001
>         Flags: medium devsel, IRQ 20
>         I/O ports at e050 [size=8]
>         I/O ports at e040 [size=8]
>         I/O ports at e030 [size=8]
>         I/O ports at e020 [size=8]
>         I/O ports at e010 [size=8]
>         I/O ports at e000 [size=16]
> 
> With 'com1=115200,8n1,pci' and 'com2=115200,8n1,pci' then the first loop
> would find the device. The second loop would decrement idx (1) by 1 and
> continue.. which would make it go search for another device.
> 
> I hadn't tested this patch on the above device but I believe it used
> to work with the com1 and com2 going throught it - while with the new code
> it won't?

That's the !bar0 case, and hence the code in the loop over
uart_config[] would set port_idx to zero, so the conditional above
won't evaluate to true anyway. I.e. no change in behavior over
the original code (albeit arguably that behavior is not fully correct,
at least if we consider arbitrary bar_idx values - right now it can
only be 0 or 1 -, since some skipping logic would then be needed
too). The question is whether we shouldn't have all single port
cards have their bar0 flag set to true (or extend the conditional
inside the loop to "!param->bar0 && param->max_ports > 1"), to
enable this skipping in all of those cases.

Jan


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

Re: [PATCH 2/4] ns16550: enable Pericom controller support

[Konrad Rzeszutek Wilk]

On Tue, Mar 08, 2016 at 01:48:05AM -0700, Jan Beulich wrote:
> >>> On 07.03.16 at 23:04, <konrad.wilk@oracle.com> wrote:
> >>  +    [param_pericom_4port] = {
> >> +        .base_baud = 921600,
> >> +        .uart_offset = 8,
> >> +        .reg_width = 1,
> >> +        .fifo_size = 16,
> >> +        .lsr_mask = UART_LSR_THRE,
> >> +        .bar0 = 1,
> >> +        .max_ports = 4,
> >> +    },
> >> +    [param_pericom_8port] = {
> >> +        .base_baud = 921600,
> >> +        .uart_offset = 8,
> >> +        .reg_width = 1,
> >> +        .fifo_size = 16,
> >> +        .lsr_mask = UART_LSR_THRE,
> >> +        .bar0 = 1,
> >> +        .max_ports = 8,
> > 
> > Perhaps document that Xen can only access two of the ports? Unless we
> > expand the ns16550_com array of course.
> 
> Done.
> 
> >> @@ -843,8 +911,10 @@ pci_uart_config(struct ns16550 *uart, bo
> >>          {
> >>              for ( f = 0; f < 8; f = nextf )
> >>              {
> >> +                unsigned int bar_idx = 0, port_idx = idx;
> > 
> > s/port_idx/port/? or port_nr /?
> 
> "port" would be misleading/ambiguous, and I don't see port_nr being
> any better than port_idx (or if so, it ought to then also be bar_nr).
> In fact, "nr" - other than "idx" - is ambiguous too (commonly
> indicating "number of ...").
> 
> >> @@ -863,15 +933,38 @@ pci_uart_config(struct ns16550 *uart, bo
> >>                      continue;
> >>                  }
> >>  
> >> +                /* Check for params in uart_config lookup table */
> >> +                for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
> > 
> > I am pretty sure I wrote this piece of code - could you fix the
> > Style on it please? The i++) please?
> 
> Sure.
> 
> >> +                if ( port_idx >= param->max_ports )
> >> +                {
> >> +                    idx -= param->max_ports;
> >> +                    continue;
> > 
> > Could you add a comment about this? I understand it can detect if we are
> > using an AMT device with the 'com2=115200,8n1,amt' (which would be
> > invalid - AMT devices only have one IO PORT and there is only one of
> > them on the machine) we would skip over the found device and continue on..
> > Thought I don't understand why we want to decrease the idx value from one to 
> > zero?
> 
> If we're looking for COM2 and have found a 1-port card, we want to
> use the 1st (rather than the 2nd) port on the next card we may find
> (if any). This seems pretty obvious behavior to me here, so I'm not
> really convinced a comment is warranted.
> 
> > Hmm, if it was some other PCI based serial card like:
> > 
> > 01:05.0 Serial controller: NetMos Technology PCI 9835 Multi-I/O
> > Controller (rev 01) (prog-if 02 [16550])
> >         Subsystem: LSI Logic / Symbios Logic Device 0001
> >         Flags: medium devsel, IRQ 20
> >         I/O ports at e050 [size=8]
> >         I/O ports at e040 [size=8]
> >         I/O ports at e030 [size=8]
> >         I/O ports at e020 [size=8]
> >         I/O ports at e010 [size=8]
> >         I/O ports at e000 [size=16]
> > 
> > With 'com1=115200,8n1,pci' and 'com2=115200,8n1,pci' then the first loop
> > would find the device. The second loop would decrement idx (1) by 1 and
> > continue.. which would make it go search for another device.
> > 
> > I hadn't tested this patch on the above device but I believe it used
> > to work with the com1 and com2 going throught it - while with the new code
> > it won't?
> 
> That's the !bar0 case, and hence the code in the loop over

You mean:

       			 param += uart_config[i].param;
+                        if ( !param->bar0 )
+                        {
+                            bar_idx = idx;
+                            port_idx = 0;
+                        }
?

The device in question (NetMos) is not on the uart_config list at all
so we won't get inside this loop.

> uart_config[] would set port_idx to zero, so the conditional above
> won't evaluate to true anyway. I.e. no change in behavior over
> the original code (albeit arguably that behavior is not fully correct,
> at least if we consider arbitrary bar_idx values - right now it can
> only be 0 or 1 -, since some skipping logic would then be needed
> too). The question is whether we shouldn't have all single port
> cards have their bar0 flag set to true (or extend the conditional
> inside the loop to "!param->bar0 && param->max_ports > 1"), to
> enable this skipping in all of those cases.
> 
> Jan
> 

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

Re: [PATCH 2/4] ns16550: enable Pericom controller support

[Jan Beulich]

>>> On 09.03.16 at 17:52, <konrad.wilk@oracle.com> wrote:
> On Tue, Mar 08, 2016 at 01:48:05AM -0700, Jan Beulich wrote:
>> >>> On 07.03.16 at 23:04, <konrad.wilk@oracle.com> wrote:
>> > Hmm, if it was some other PCI based serial card like:
>> > 
>> > 01:05.0 Serial controller: NetMos Technology PCI 9835 Multi-I/O
>> > Controller (rev 01) (prog-if 02 [16550])
>> >         Subsystem: LSI Logic / Symbios Logic Device 0001
>> >         Flags: medium devsel, IRQ 20
>> >         I/O ports at e050 [size=8]
>> >         I/O ports at e040 [size=8]
>> >         I/O ports at e030 [size=8]
>> >         I/O ports at e020 [size=8]
>> >         I/O ports at e010 [size=8]
>> >         I/O ports at e000 [size=16]
>> > 
>> > With 'com1=115200,8n1,pci' and 'com2=115200,8n1,pci' then the first loop
>> > would find the device. The second loop would decrement idx (1) by 1 and
>> > continue.. which would make it go search for another device.
>> > 
>> > I hadn't tested this patch on the above device but I believe it used
>> > to work with the com1 and com2 going throught it - while with the new code
>> > it won't?
>> 
>> That's the !bar0 case, and hence the code in the loop over
> 
> You mean:
> 
>        			 param += uart_config[i].param;
> +                        if ( !param->bar0 )
> +                        {
> +                            bar_idx = idx;
> +                            port_idx = 0;
> +                        }
> ?
> 
> The device in question (NetMos) is not on the uart_config list at all
> so we won't get inside this loop.

Well, for devices not on the list it's undetermined anyway whether
they would happen to work - we just can't get it right for all possible
cases. Someone truly caring about them working should submit a
patch adding them to the list.

Jan


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

Re: [PATCH 2/4] ns16550: enable Pericom controller support

[Konrad Rzeszutek Wilk]

On Wed, Mar 09, 2016 at 10:01:08AM -0700, Jan Beulich wrote:
> >>> On 09.03.16 at 17:52, <konrad.wilk@oracle.com> wrote:
> > On Tue, Mar 08, 2016 at 01:48:05AM -0700, Jan Beulich wrote:
> >> >>> On 07.03.16 at 23:04, <konrad.wilk@oracle.com> wrote:
> >> > Hmm, if it was some other PCI based serial card like:
> >> > 
> >> > 01:05.0 Serial controller: NetMos Technology PCI 9835 Multi-I/O
> >> > Controller (rev 01) (prog-if 02 [16550])
> >> >         Subsystem: LSI Logic / Symbios Logic Device 0001
> >> >         Flags: medium devsel, IRQ 20
> >> >         I/O ports at e050 [size=8]
> >> >         I/O ports at e040 [size=8]
> >> >         I/O ports at e030 [size=8]
> >> >         I/O ports at e020 [size=8]
> >> >         I/O ports at e010 [size=8]
> >> >         I/O ports at e000 [size=16]
> >> > 
> >> > With 'com1=115200,8n1,pci' and 'com2=115200,8n1,pci' then the first loop
> >> > would find the device. The second loop would decrement idx (1) by 1 and
> >> > continue.. which would make it go search for another device.
> >> > 
> >> > I hadn't tested this patch on the above device but I believe it used
> >> > to work with the com1 and com2 going throught it - while with the new code
> >> > it won't?
> >> 
> >> That's the !bar0 case, and hence the code in the loop over
> > 
> > You mean:
> > 
> >        			 param += uart_config[i].param;
> > +                        if ( !param->bar0 )
> > +                        {
> > +                            bar_idx = idx;
> > +                            port_idx = 0;
> > +                        }
> > ?
> > 
> > The device in question (NetMos) is not on the uart_config list at all
> > so we won't get inside this loop.
> 
> Well, for devices not on the list it's undetermined anyway whether
> they would happen to work - we just can't get it right for all possible
> cases. Someone truly caring about them working should submit a

Wouldn't your patch cause a regression compared to how it used
to work in earlier version of Xen?

Let me take a peek at the card in question and see if I can hook
up an extra RS232 connector.
> patch adding them to the list.
> 
> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

Re: [PATCH 2/4] ns16550: enable Pericom controller support

[Jan Beulich]

>>> On 11.03.16 at 03:31, <konrad@kernel.org> wrote:
> On Wed, Mar 09, 2016 at 10:01:08AM -0700, Jan Beulich wrote:
>> >>> On 09.03.16 at 17:52, <konrad.wilk@oracle.com> wrote:
>> > On Tue, Mar 08, 2016 at 01:48:05AM -0700, Jan Beulich wrote:
>> >> >>> On 07.03.16 at 23:04, <konrad.wilk@oracle.com> wrote:
>> >> > Hmm, if it was some other PCI based serial card like:
>> >> > 
>> >> > 01:05.0 Serial controller: NetMos Technology PCI 9835 Multi-I/O
>> >> > Controller (rev 01) (prog-if 02 [16550])
>> >> >         Subsystem: LSI Logic / Symbios Logic Device 0001
>> >> >         Flags: medium devsel, IRQ 20
>> >> >         I/O ports at e050 [size=8]
>> >> >         I/O ports at e040 [size=8]
>> >> >         I/O ports at e030 [size=8]
>> >> >         I/O ports at e020 [size=8]
>> >> >         I/O ports at e010 [size=8]
>> >> >         I/O ports at e000 [size=16]
>> >> > 
>> >> > With 'com1=115200,8n1,pci' and 'com2=115200,8n1,pci' then the first loop
>> >> > would find the device. The second loop would decrement idx (1) by 1 and
>> >> > continue.. which would make it go search for another device.
>> >> > 
>> >> > I hadn't tested this patch on the above device but I believe it used
>> >> > to work with the com1 and com2 going throught it - while with the new code
>> >> > it won't?
>> >> 
>> >> That's the !bar0 case, and hence the code in the loop over
>> > 
>> > You mean:
>> > 
>> >        			 param += uart_config[i].param;
>> > +                        if ( !param->bar0 )
>> > +                        {
>> > +                            bar_idx = idx;
>> > +                            port_idx = 0;
>> > +                        }
>> > ?
>> > 
>> > The device in question (NetMos) is not on the uart_config list at all
>> > so we won't get inside this loop.
>> 
>> Well, for devices not on the list it's undetermined anyway whether
>> they would happen to work - we just can't get it right for all possible
>> cases. Someone truly caring about them working should submit a
> 
> Wouldn't your patch cause a regression compared to how it used
> to work in earlier version of Xen?

It would, if you want to call such a regression (as I don't think it
was determined to behave that way). But okay, I'll simply move
the above conditional past the loop, to retain previous behavior.

Jan


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

[PATCH v2 2/4] ns16550: enable Pericom controller support

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 14280 bytes --]

Other than the controllers supported so far, multiple port Pericom
boards map all of their ports via BAR0, which requires a number of
adjustments: Instead of tracking "max_bars" we now flag whether all
ports use BAR0, and whether to expect a port-I/O or MMIO resource. As
a result pci_uart_config() now gets handed a port index, which it then
maps into a BAR index or an offset into BAR0 depending on the bar0
flag.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v2: Fix coding style in a piece of code being moved. Add a comment
    clarifying that on the 4- and 8-port variants we can't use all the
    ports for now. Retain previous behavior for PCI devices not
    explicitly listed, even if that behavior was undefined. (The rest
    of the series is unchanged, so I won't bother reposting the other
    patches.)

--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -78,10 +78,20 @@ static struct ns16550 {
 #endif
 } ns16550_com[2] = { { 0 } };
 
-struct ns16550_config_mmio {
+#ifdef CONFIG_HAS_PCI
+struct ns16550_config {
     u16 vendor_id;
     u16 dev_id;
-    unsigned int param;
+    enum {
+        param_default, /* Must not be referenced by any table entry. */
+        param_trumanage,
+        param_oxford,
+        param_oxford_2port,
+        param_pericom_1port,
+        param_pericom_2port,
+        param_pericom_4port,
+        param_pericom_8port,
+    } param;
 };
 
 /* Defining uart config options for MMIO devices */
@@ -90,57 +100,95 @@ struct ns16550_config_param {
     unsigned int reg_width;
     unsigned int fifo_size;
     u8 lsr_mask;
-    unsigned int max_bars;
+    bool_t mmio;
+    bool_t bar0;
+    unsigned int max_ports;
     unsigned int base_baud;
     unsigned int uart_offset;
     unsigned int first_offset;
 };
 
-
-#ifdef CONFIG_HAS_PCI
-enum {
-    param_default = 0,
-    param_trumanage,
-    param_oxford,
-    param_oxford_2port,
-};
 /*
- * 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.
+ * Create lookup tables for specific devices. It is assumed that if
+ * the device found is MMIO, then you have indexed it here. Else, the
+ * driver does nothing for MMIO based devices.
  */
 static const struct ns16550_config_param __initconst uart_param[] = {
-    [param_default] = { }, /* Ignored. */
+    [param_default] = {
+        .reg_width = 1,
+        .lsr_mask = UART_LSR_THRE,
+        .max_ports = 1,
+    },
     [param_trumanage] = {
         .reg_shift = 2,
         .reg_width = 1,
         .fifo_size = 16,
         .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
-        .max_bars = 1,
+        .mmio = 1,
+        .max_ports = 1,
     },
     [param_oxford] = {
         .base_baud = 4000000,
         .uart_offset = 0x200,
         .first_offset = 0x1000,
         .reg_width = 1,
-        .reg_shift = 0,
         .fifo_size = 16,
         .lsr_mask = UART_LSR_THRE,
-        .max_bars = 1, /* It can do more, but we would need more custom code.*/
+        .mmio = 1,
+        .max_ports = 1, /* It can do more, but we would need more custom code.*/
     },
     [param_oxford_2port] = {
         .base_baud = 4000000,
         .uart_offset = 0x200,
         .first_offset = 0x1000,
         .reg_width = 1,
-        .reg_shift = 0,
         .fifo_size = 16,
         .lsr_mask = UART_LSR_THRE,
-        .max_bars = 2,
+        .mmio = 1,
+        .max_ports = 2,
+    },
+    [param_pericom_1port] = {
+        .base_baud = 921600,
+        .uart_offset = 8,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .max_ports = 1,
+    },
+    [param_pericom_2port] = {
+        .base_baud = 921600,
+        .uart_offset = 8,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .max_ports = 2,
+    },
+    /*
+     * Of the two following ones, we can't really use all of their ports,
+     * unless ns16550_com[] would get grown.
+     */
+    [param_pericom_4port] = {
+        .base_baud = 921600,
+        .uart_offset = 8,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .max_ports = 4,
+    },
+    [param_pericom_8port] = {
+        .base_baud = 921600,
+        .uart_offset = 8,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .max_ports = 8,
     }
 };
-static const struct ns16550_config_mmio __initconst uart_config[] =
+static const struct ns16550_config __initconst uart_config[] =
 {
     /* Broadcom TruManage device */
     {
@@ -339,6 +387,30 @@ static const struct ns16550_config_mmio
         .vendor_id = PCI_VENDOR_ID_OXSEMI,
         .dev_id = 0xc4cf,
         .param = param_oxford,
+    },
+    /* Pericom PI7C9X7951 Uno UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_PERICOM,
+        .dev_id = 0x7951,
+        .param = param_pericom_1port
+    },
+    /* Pericom PI7C9X7952 Duo UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_PERICOM,
+        .dev_id = 0x7952,
+        .param = param_pericom_2port
+    },
+    /* Pericom PI7C9X7954 Quad UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_PERICOM,
+        .dev_id = 0x7954,
+        .param = param_pericom_4port
+    },
+    /* Pericom PI7C9X7958 Octal UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_PERICOM,
+        .dev_id = 0x7958,
+        .param = param_pericom_8port
     }
 };
 #endif
@@ -629,7 +701,8 @@ static void __init ns16550_init_postirq(
                             uart->ps_bdf[2]));
         else
         {
-            if ( rangeset_add_range(mmio_ro_ranges,
+            if ( uart->param->mmio &&
+                 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");
@@ -830,12 +903,11 @@ static int __init check_existence(struct
 
 #ifdef CONFIG_HAS_PCI
 static int __init
-pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int bar_idx)
+pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
 {
     u64 orig_base = uart->io_base;
     unsigned int b, d, f, nextf, i;
 
-    uart->io_base = 0;
     /* 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++ )
     {
@@ -843,8 +915,10 @@ pci_uart_config(struct ns16550 *uart, bo
         {
             for ( f = 0; f < 8; f = nextf )
             {
+                unsigned int bar_idx = 0, port_idx = idx;
                 uint32_t bar, bar_64 = 0, len, len_64;
-                u64 size;
+                u64 size = 0;
+                const struct ns16550_config_param *param = uart_param;
 
                 nextf = (f || (pci_conf_read16(0, b, d, f, PCI_HEADER_TYPE) &
                                0x80)) ? f + 1 : 8;
@@ -863,15 +937,39 @@ pci_uart_config(struct ns16550 *uart, bo
                     continue;
                 }
 
+                /* Check for params in uart_config lookup table */
+                for ( i = 0; i < ARRAY_SIZE(uart_config); i++ )
+                {
+                    u16 vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
+                    u16 device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID);
+
+                    if ( uart_config[i].vendor_id == vendor &&
+                         uart_config[i].dev_id == device )
+                    {
+                        param += uart_config[i].param;
+                        break;
+                    }
+                }
+
+                if ( !param->bar0 )
+                {
+                    bar_idx = idx;
+                    port_idx = 0;
+                }
+
+                if ( port_idx >= param->max_ports )
+                {
+                    idx -= param->max_ports;
+                    continue;
+                }
+
+                uart->io_base = 0;
                 bar = pci_conf_read32(0, b, d, f,
                                       PCI_BASE_ADDRESS_0 + bar_idx*4);
 
                 /* MMIO based */
-                if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
+                if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
                 {
-                    u16 vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
-                    u16 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);
@@ -895,56 +993,11 @@ pci_uart_config(struct ns16550 *uart, bo
                     else
                         size = len & PCI_BASE_ADDRESS_MEM_MASK;
 
-                    size &= -size;
-
-                    /* Check for params in uart_config lookup table */
-                    for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
-                    {
-                        const struct ns16550_config_param *param;
-
-                        if ( uart_config[i].vendor_id != vendor )
-                            continue;
-
-                        if ( uart_config[i].dev_id != device )
-                            continue;
-
-                        param = uart_param + uart_config[i].param;
-
-                        /*
-                         * Force length of mmio region to be at least
-                         * 8 bytes times (1 << reg_shift)
-                         */
-                        if ( size < (0x8 * (1 << param->reg_shift)) )
-                            continue;
-
-                        if ( bar_idx >= param->max_bars )
-                            continue;
-
-                        uart->param = param;
-
-                        if ( param->fifo_size )
-                            uart->fifo_size = param->fifo_size;
-
-                        uart->reg_shift = param->reg_shift;
-                        uart->reg_width = param->reg_width;
-                        uart->lsr_mask = param->lsr_mask;
-                        uart->io_base = ((u64)bar_64 << 32) |
-                                        (bar & PCI_BASE_ADDRESS_MEM_MASK);
-                        uart->io_base += param->first_offset;
-                        uart->io_base += bar_idx * param->uart_offset;
-                        if ( param->base_baud )
-                            uart->clock_hz = param->base_baud * 16;
-                        size = max(8U << param->reg_shift,
-                                   param->uart_offset);
-                        break;
-                    }
-
-                    /* If we have an io_base, then we succeeded in the lookup */
-                    if ( !uart->io_base )
-                        continue;
+                    uart->io_base = ((u64)bar_64 << 32) |
+                                    (bar & PCI_BASE_ADDRESS_MEM_MASK);
                 }
                 /* IO based */
-                else
+                else if ( !param->mmio && (bar & PCI_BASE_ADDRESS_SPACE_IO) )
                 {
                     pci_conf_write32(0, b, d, f,
                                      PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
@@ -952,22 +1005,45 @@ pci_uart_config(struct ns16550 *uart, bo
                     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 at least 8 bytes */
-                    if ( size < 8 )
-                        continue;
 
                     uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO;
                 }
 
+                /* If we have an io_base, then we succeeded in the lookup. */
+                if ( !uart->io_base )
+                    continue;
+
+                size &= -size;
+
+                /*
+                 * Require length of actually used region to be at least
+                 * 8 bytes times (1 << reg_shift).
+                 */
+                if ( size < param->first_offset +
+                            port_idx * param->uart_offset +
+                            (8 << param->reg_shift) )
+                    continue;
+
+                uart->param = param;
+
+                uart->reg_shift = param->reg_shift;
+                uart->reg_width = param->reg_width;
+                uart->lsr_mask = param->lsr_mask;
+                uart->io_base += param->first_offset +
+                                 port_idx * param->uart_offset;
+                if ( param->base_baud )
+                    uart->clock_hz = param->base_baud * 16;
+                if ( param->fifo_size )
+                    uart->fifo_size = param->fifo_size;
+
                 uart->ps_bdf[0] = b;
                 uart->ps_bdf[1] = d;
                 uart->ps_bdf[2] = f;
                 uart->bar_idx = bar_idx;
                 uart->bar = bar;
                 uart->bar64 = bar_64;
-                uart->io_size = size;
+                uart->io_size = max(8U << param->reg_shift,
+                                    param->uart_offset);
                 uart->irq = pci_conf_read8(0, b, d, f, PCI_INTERRUPT_PIN) ?
                     pci_conf_read8(0, b, d, f, PCI_INTERRUPT_LINE) : 0;
 
--- a/xen/include/xen/pci_ids.h
+++ b/xen/include/xen/pci_ids.h
@@ -2,6 +2,8 @@
 
 #define PCI_VENDOR_ID_NVIDIA             0x10de
 
+#define PCI_VENDOR_ID_PERICOM            0x12d8
+
 #define PCI_VENDOR_ID_OXSEMI             0x1415
 
 #define PCI_VENDOR_ID_BROADCOM           0x14e4



[-- Attachment #2: ns16550-Pericom.patch --]
[-- Type: text/plain, Size: 14322 bytes --]

ns16550: enable Pericom controller support

Other than the controllers supported so far, multiple port Pericom
boards map all of their ports via BAR0, which requires a number of
adjustments: Instead of tracking "max_bars" we now flag whether all
ports use BAR0, and whether to expect a port-I/O or MMIO resource. As
a result pci_uart_config() now gets handed a port index, which it then
maps into a BAR index or an offset into BAR0 depending on the bar0
flag.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v2: Fix coding style in a piece of code being moved. Add a comment
    clarifying that on the 4- and 8-port variants we can't use all the
    ports for now. Retain previous behavior for PCI devices not
    explicitly listed, even if that behavior was undefined. (The rest
    of the series is unchanged, so I won't bother reposting the other
    patches.)

--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -78,10 +78,20 @@ static struct ns16550 {
 #endif
 } ns16550_com[2] = { { 0 } };
 
-struct ns16550_config_mmio {
+#ifdef CONFIG_HAS_PCI
+struct ns16550_config {
     u16 vendor_id;
     u16 dev_id;
-    unsigned int param;
+    enum {
+        param_default, /* Must not be referenced by any table entry. */
+        param_trumanage,
+        param_oxford,
+        param_oxford_2port,
+        param_pericom_1port,
+        param_pericom_2port,
+        param_pericom_4port,
+        param_pericom_8port,
+    } param;
 };
 
 /* Defining uart config options for MMIO devices */
@@ -90,57 +100,95 @@ struct ns16550_config_param {
     unsigned int reg_width;
     unsigned int fifo_size;
     u8 lsr_mask;
-    unsigned int max_bars;
+    bool_t mmio;
+    bool_t bar0;
+    unsigned int max_ports;
     unsigned int base_baud;
     unsigned int uart_offset;
     unsigned int first_offset;
 };
 
-
-#ifdef CONFIG_HAS_PCI
-enum {
-    param_default = 0,
-    param_trumanage,
-    param_oxford,
-    param_oxford_2port,
-};
 /*
- * 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.
+ * Create lookup tables for specific devices. It is assumed that if
+ * the device found is MMIO, then you have indexed it here. Else, the
+ * driver does nothing for MMIO based devices.
  */
 static const struct ns16550_config_param __initconst uart_param[] = {
-    [param_default] = { }, /* Ignored. */
+    [param_default] = {
+        .reg_width = 1,
+        .lsr_mask = UART_LSR_THRE,
+        .max_ports = 1,
+    },
     [param_trumanage] = {
         .reg_shift = 2,
         .reg_width = 1,
         .fifo_size = 16,
         .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
-        .max_bars = 1,
+        .mmio = 1,
+        .max_ports = 1,
     },
     [param_oxford] = {
         .base_baud = 4000000,
         .uart_offset = 0x200,
         .first_offset = 0x1000,
         .reg_width = 1,
-        .reg_shift = 0,
         .fifo_size = 16,
         .lsr_mask = UART_LSR_THRE,
-        .max_bars = 1, /* It can do more, but we would need more custom code.*/
+        .mmio = 1,
+        .max_ports = 1, /* It can do more, but we would need more custom code.*/
     },
     [param_oxford_2port] = {
         .base_baud = 4000000,
         .uart_offset = 0x200,
         .first_offset = 0x1000,
         .reg_width = 1,
-        .reg_shift = 0,
         .fifo_size = 16,
         .lsr_mask = UART_LSR_THRE,
-        .max_bars = 2,
+        .mmio = 1,
+        .max_ports = 2,
+    },
+    [param_pericom_1port] = {
+        .base_baud = 921600,
+        .uart_offset = 8,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .max_ports = 1,
+    },
+    [param_pericom_2port] = {
+        .base_baud = 921600,
+        .uart_offset = 8,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .max_ports = 2,
+    },
+    /*
+     * Of the two following ones, we can't really use all of their ports,
+     * unless ns16550_com[] would get grown.
+     */
+    [param_pericom_4port] = {
+        .base_baud = 921600,
+        .uart_offset = 8,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .max_ports = 4,
+    },
+    [param_pericom_8port] = {
+        .base_baud = 921600,
+        .uart_offset = 8,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = 1,
+        .max_ports = 8,
     }
 };
-static const struct ns16550_config_mmio __initconst uart_config[] =
+static const struct ns16550_config __initconst uart_config[] =
 {
     /* Broadcom TruManage device */
     {
@@ -339,6 +387,30 @@ static const struct ns16550_config_mmio
         .vendor_id = PCI_VENDOR_ID_OXSEMI,
         .dev_id = 0xc4cf,
         .param = param_oxford,
+    },
+    /* Pericom PI7C9X7951 Uno UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_PERICOM,
+        .dev_id = 0x7951,
+        .param = param_pericom_1port
+    },
+    /* Pericom PI7C9X7952 Duo UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_PERICOM,
+        .dev_id = 0x7952,
+        .param = param_pericom_2port
+    },
+    /* Pericom PI7C9X7954 Quad UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_PERICOM,
+        .dev_id = 0x7954,
+        .param = param_pericom_4port
+    },
+    /* Pericom PI7C9X7958 Octal UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_PERICOM,
+        .dev_id = 0x7958,
+        .param = param_pericom_8port
     }
 };
 #endif
@@ -629,7 +701,8 @@ static void __init ns16550_init_postirq(
                             uart->ps_bdf[2]));
         else
         {
-            if ( rangeset_add_range(mmio_ro_ranges,
+            if ( uart->param->mmio &&
+                 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");
@@ -830,12 +903,11 @@ static int __init check_existence(struct
 
 #ifdef CONFIG_HAS_PCI
 static int __init
-pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int bar_idx)
+pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
 {
     u64 orig_base = uart->io_base;
     unsigned int b, d, f, nextf, i;
 
-    uart->io_base = 0;
     /* 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++ )
     {
@@ -843,8 +915,10 @@ pci_uart_config(struct ns16550 *uart, bo
         {
             for ( f = 0; f < 8; f = nextf )
             {
+                unsigned int bar_idx = 0, port_idx = idx;
                 uint32_t bar, bar_64 = 0, len, len_64;
-                u64 size;
+                u64 size = 0;
+                const struct ns16550_config_param *param = uart_param;
 
                 nextf = (f || (pci_conf_read16(0, b, d, f, PCI_HEADER_TYPE) &
                                0x80)) ? f + 1 : 8;
@@ -863,15 +937,39 @@ pci_uart_config(struct ns16550 *uart, bo
                     continue;
                 }
 
+                /* Check for params in uart_config lookup table */
+                for ( i = 0; i < ARRAY_SIZE(uart_config); i++ )
+                {
+                    u16 vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
+                    u16 device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID);
+
+                    if ( uart_config[i].vendor_id == vendor &&
+                         uart_config[i].dev_id == device )
+                    {
+                        param += uart_config[i].param;
+                        break;
+                    }
+                }
+
+                if ( !param->bar0 )
+                {
+                    bar_idx = idx;
+                    port_idx = 0;
+                }
+
+                if ( port_idx >= param->max_ports )
+                {
+                    idx -= param->max_ports;
+                    continue;
+                }
+
+                uart->io_base = 0;
                 bar = pci_conf_read32(0, b, d, f,
                                       PCI_BASE_ADDRESS_0 + bar_idx*4);
 
                 /* MMIO based */
-                if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
+                if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
                 {
-                    u16 vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
-                    u16 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);
@@ -895,56 +993,11 @@ pci_uart_config(struct ns16550 *uart, bo
                     else
                         size = len & PCI_BASE_ADDRESS_MEM_MASK;
 
-                    size &= -size;
-
-                    /* Check for params in uart_config lookup table */
-                    for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
-                    {
-                        const struct ns16550_config_param *param;
-
-                        if ( uart_config[i].vendor_id != vendor )
-                            continue;
-
-                        if ( uart_config[i].dev_id != device )
-                            continue;
-
-                        param = uart_param + uart_config[i].param;
-
-                        /*
-                         * Force length of mmio region to be at least
-                         * 8 bytes times (1 << reg_shift)
-                         */
-                        if ( size < (0x8 * (1 << param->reg_shift)) )
-                            continue;
-
-                        if ( bar_idx >= param->max_bars )
-                            continue;
-
-                        uart->param = param;
-
-                        if ( param->fifo_size )
-                            uart->fifo_size = param->fifo_size;
-
-                        uart->reg_shift = param->reg_shift;
-                        uart->reg_width = param->reg_width;
-                        uart->lsr_mask = param->lsr_mask;
-                        uart->io_base = ((u64)bar_64 << 32) |
-                                        (bar & PCI_BASE_ADDRESS_MEM_MASK);
-                        uart->io_base += param->first_offset;
-                        uart->io_base += bar_idx * param->uart_offset;
-                        if ( param->base_baud )
-                            uart->clock_hz = param->base_baud * 16;
-                        size = max(8U << param->reg_shift,
-                                   param->uart_offset);
-                        break;
-                    }
-
-                    /* If we have an io_base, then we succeeded in the lookup */
-                    if ( !uart->io_base )
-                        continue;
+                    uart->io_base = ((u64)bar_64 << 32) |
+                                    (bar & PCI_BASE_ADDRESS_MEM_MASK);
                 }
                 /* IO based */
-                else
+                else if ( !param->mmio && (bar & PCI_BASE_ADDRESS_SPACE_IO) )
                 {
                     pci_conf_write32(0, b, d, f,
                                      PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
@@ -952,22 +1005,45 @@ pci_uart_config(struct ns16550 *uart, bo
                     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 at least 8 bytes */
-                    if ( size < 8 )
-                        continue;
 
                     uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO;
                 }
 
+                /* If we have an io_base, then we succeeded in the lookup. */
+                if ( !uart->io_base )
+                    continue;
+
+                size &= -size;
+
+                /*
+                 * Require length of actually used region to be at least
+                 * 8 bytes times (1 << reg_shift).
+                 */
+                if ( size < param->first_offset +
+                            port_idx * param->uart_offset +
+                            (8 << param->reg_shift) )
+                    continue;
+
+                uart->param = param;
+
+                uart->reg_shift = param->reg_shift;
+                uart->reg_width = param->reg_width;
+                uart->lsr_mask = param->lsr_mask;
+                uart->io_base += param->first_offset +
+                                 port_idx * param->uart_offset;
+                if ( param->base_baud )
+                    uart->clock_hz = param->base_baud * 16;
+                if ( param->fifo_size )
+                    uart->fifo_size = param->fifo_size;
+
                 uart->ps_bdf[0] = b;
                 uart->ps_bdf[1] = d;
                 uart->ps_bdf[2] = f;
                 uart->bar_idx = bar_idx;
                 uart->bar = bar;
                 uart->bar64 = bar_64;
-                uart->io_size = size;
+                uart->io_size = max(8U << param->reg_shift,
+                                    param->uart_offset);
                 uart->irq = pci_conf_read8(0, b, d, f, PCI_INTERRUPT_PIN) ?
                     pci_conf_read8(0, b, d, f, PCI_INTERRUPT_LINE) : 0;
 
--- a/xen/include/xen/pci_ids.h
+++ b/xen/include/xen/pci_ids.h
@@ -2,6 +2,8 @@
 
 #define PCI_VENDOR_ID_NVIDIA             0x10de
 
+#define PCI_VENDOR_ID_PERICOM            0x12d8
+
 #define PCI_VENDOR_ID_OXSEMI             0x1415
 
 #define PCI_VENDOR_ID_BROADCOM           0x14e4

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 2/4] ns16550: enable Pericom controller support

[Konrad Rzeszutek Wilk]

On Tue, Mar 22, 2016 at 07:19:49AM -0600, Jan Beulich wrote:
> Other than the controllers supported so far, multiple port Pericom
> boards map all of their ports via BAR0, which requires a number of
> adjustments: Instead of tracking "max_bars" we now flag whether all
> ports use BAR0, and whether to expect a port-I/O or MMIO resource. As
> a result pci_uart_config() now gets handed a port index, which it then
> maps into a BAR index or an offset into BAR0 depending on the bar0
> flag.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> v2: Fix coding style in a piece of code being moved. Add a comment
>     clarifying that on the 4- and 8-port variants we can't use all the
>     ports for now. Retain previous behavior for PCI devices not
>     explicitly listed, even if that behavior was undefined. (The rest

Thank you for making that work.

>     of the series is unchanged, so I won't bother reposting the other
>     patches.)

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

[PATCH 3/4] console: adjust IRQ initialization

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4074 bytes --]

In order for a Xen internal PCI device driver to enable MSI on the
device, we need another hook which the driver can use to create the IRQ
(doing this in the init_preirq hook is too early, since IRQ code hasn't
got initialized at that time yet, and doing it in init_postirq is too
late because at least on x86 smp_intr_init() needs to know the IRQ
number).

On x86 this additionally requires a slight ordering change to IRQ
initialization, to facilitate calling the new hook between basic
initialization and the call path leading to smp_intr_init().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/i8259.c
+++ b/xen/arch/x86/i8259.c
@@ -342,8 +342,6 @@ void __init init_IRQ(void)
 
     init_8259A(0);
 
-    BUG_ON(init_irq_data() < 0);
-
     for (irq = 0; platform_legacy_irq(irq); irq++) {
         struct irq_desc *desc = irq_to_desc(irq);
         
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -576,6 +576,7 @@ void __init noreturn __start_xen(unsigne
     unsigned long nr_pages, raw_max_page, modules_headroom, *module_map;
     int i, j, e820_warn = 0, bytes = 0;
     bool_t acpi_boot_table_init_done = 0;
+    int ret;
     struct domain *dom0;
     struct ns16550_defaults ns16550 = {
         .data_bits = 8,
@@ -1281,6 +1282,12 @@ void __init noreturn __start_xen(unsigne
 
     x2apic_bsp_setup();
 
+    ret = init_irq_data();
+    if ( ret < 0 )
+        panic("Error %d setting up IRQ data\n", ret);
+
+    console_init_irq();
+
     init_IRQ();
 
     module_map = xmalloc_array(unsigned long, BITS_TO_LONGS(mbi->mods_count));
@@ -1366,7 +1373,7 @@ void __init noreturn __start_xen(unsigne
 
         if ( (num_online_cpus() < max_cpus) && !cpu_online(i) )
         {
-            int ret = cpu_up(i);
+            ret = cpu_up(i);
             if ( ret != 0 )
                 printk("Failed to bring up CPU %u (error %d)\n", i, ret);
         }
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -771,6 +771,11 @@ void __init console_init_ring(void)
     printk("Allocated console ring of %u KiB.\n", opt_conring_size >> 10);
 }
 
+void __init console_init_irq(void)
+{
+    serial_init_irq();
+}
+
 void __init console_init_postirq(void)
 {
     serial_init_postirq();
--- a/xen/drivers/char/serial.c
+++ b/xen/drivers/char/serial.c
@@ -504,6 +504,15 @@ void __init serial_init_preirq(void)
             com[i].driver->init_preirq(&com[i]);
 }
 
+void __init serial_init_irq(void)
+{
+    unsigned int i;
+
+    for ( i = 0; i < ARRAY_SIZE(com); i++ )
+        if ( com[i].driver && com[i].driver->init_irq )
+            com[i].driver->init_irq(&com[i]);
+}
+
 void __init serial_init_postirq(void)
 {
     int i;
--- a/xen/include/xen/console.h
+++ b/xen/include/xen/console.h
@@ -15,6 +15,7 @@ int console_loglvl_op(struct xen_sysctl_
 
 void console_init_preirq(void);
 void console_init_ring(void);
+void console_init_irq(void);
 void console_init_postirq(void);
 void console_endboot(void);
 int console_has(const char *device);
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -64,6 +64,7 @@ struct serial_port {
 struct uart_driver {
     /* Driver initialisation (pre- and post-IRQ subsystem setup). */
     void (*init_preirq)(struct serial_port *);
+    void (*init_irq)(struct serial_port *);
     void (*init_postirq)(struct serial_port *);
     /* Hook to clean up after Xen bootstrap (before domain 0 runs). */
     void (*endboot)(struct serial_port *);
@@ -99,8 +100,9 @@ struct uart_driver {
 #define SERHND_LO       (1<<3) /* Ditto, except that the MSB is cleared.  */
 #define SERHND_COOKED   (1<<4) /* Newline/carriage-return translation?    */
 
-/* Two-stage initialisation (before/after IRQ-subsystem initialisation). */
+/* Three-stage initialisation (before/during/after IRQ-subsystem setup). */
 void serial_init_preirq(void);
+void serial_init_irq(void);
 void serial_init_postirq(void);
 
 /* Clean-up hook before domain 0 runs. */




[-- Attachment #2: console-irq-init.patch --]
[-- Type: text/plain, Size: 4106 bytes --]

console: adjust IRQ initialization

In order for a Xen internal PCI device driver to enable MSI on the
device, we need another hook which the driver can use to create the IRQ
(doing this in the init_preirq hook is too early, since IRQ code hasn't
got initialized at that time yet, and doing it in init_postirq is too
late because at least on x86 smp_intr_init() needs to know the IRQ
number).

On x86 this additionally requires a slight ordering change to IRQ
initialization, to facilitate calling the new hook between basic
initialization and the call path leading to smp_intr_init().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/i8259.c
+++ b/xen/arch/x86/i8259.c
@@ -342,8 +342,6 @@ void __init init_IRQ(void)
 
     init_8259A(0);
 
-    BUG_ON(init_irq_data() < 0);
-
     for (irq = 0; platform_legacy_irq(irq); irq++) {
         struct irq_desc *desc = irq_to_desc(irq);
         
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -576,6 +576,7 @@ void __init noreturn __start_xen(unsigne
     unsigned long nr_pages, raw_max_page, modules_headroom, *module_map;
     int i, j, e820_warn = 0, bytes = 0;
     bool_t acpi_boot_table_init_done = 0;
+    int ret;
     struct domain *dom0;
     struct ns16550_defaults ns16550 = {
         .data_bits = 8,
@@ -1281,6 +1282,12 @@ void __init noreturn __start_xen(unsigne
 
     x2apic_bsp_setup();
 
+    ret = init_irq_data();
+    if ( ret < 0 )
+        panic("Error %d setting up IRQ data\n", ret);
+
+    console_init_irq();
+
     init_IRQ();
 
     module_map = xmalloc_array(unsigned long, BITS_TO_LONGS(mbi->mods_count));
@@ -1366,7 +1373,7 @@ void __init noreturn __start_xen(unsigne
 
         if ( (num_online_cpus() < max_cpus) && !cpu_online(i) )
         {
-            int ret = cpu_up(i);
+            ret = cpu_up(i);
             if ( ret != 0 )
                 printk("Failed to bring up CPU %u (error %d)\n", i, ret);
         }
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -771,6 +771,11 @@ void __init console_init_ring(void)
     printk("Allocated console ring of %u KiB.\n", opt_conring_size >> 10);
 }
 
+void __init console_init_irq(void)
+{
+    serial_init_irq();
+}
+
 void __init console_init_postirq(void)
 {
     serial_init_postirq();
--- a/xen/drivers/char/serial.c
+++ b/xen/drivers/char/serial.c
@@ -504,6 +504,15 @@ void __init serial_init_preirq(void)
             com[i].driver->init_preirq(&com[i]);
 }
 
+void __init serial_init_irq(void)
+{
+    unsigned int i;
+
+    for ( i = 0; i < ARRAY_SIZE(com); i++ )
+        if ( com[i].driver && com[i].driver->init_irq )
+            com[i].driver->init_irq(&com[i]);
+}
+
 void __init serial_init_postirq(void)
 {
     int i;
--- a/xen/include/xen/console.h
+++ b/xen/include/xen/console.h
@@ -15,6 +15,7 @@ int console_loglvl_op(struct xen_sysctl_
 
 void console_init_preirq(void);
 void console_init_ring(void);
+void console_init_irq(void);
 void console_init_postirq(void);
 void console_endboot(void);
 int console_has(const char *device);
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -64,6 +64,7 @@ struct serial_port {
 struct uart_driver {
     /* Driver initialisation (pre- and post-IRQ subsystem setup). */
     void (*init_preirq)(struct serial_port *);
+    void (*init_irq)(struct serial_port *);
     void (*init_postirq)(struct serial_port *);
     /* Hook to clean up after Xen bootstrap (before domain 0 runs). */
     void (*endboot)(struct serial_port *);
@@ -99,8 +100,9 @@ struct uart_driver {
 #define SERHND_LO       (1<<3) /* Ditto, except that the MSB is cleared.  */
 #define SERHND_COOKED   (1<<4) /* Newline/carriage-return translation?    */
 
-/* Two-stage initialisation (before/after IRQ-subsystem initialisation). */
+/* Three-stage initialisation (before/during/after IRQ-subsystem setup). */
 void serial_init_preirq(void);
+void serial_init_irq(void);
 void serial_init_postirq(void);
 
 /* Clean-up hook before domain 0 runs. */

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 3/4] console: adjust IRQ initialization

[Konrad Rzeszutek Wilk]

On Tue, Feb 23, 2016 at 04:30:01AM -0700, Jan Beulich wrote:
> In order for a Xen internal PCI device driver to enable MSI on the
> device, we need another hook which the driver can use to create the IRQ
> (doing this in the init_preirq hook is too early, since IRQ code hasn't
> got initialized at that time yet, and doing it in init_postirq is too
> late because at least on x86 smp_intr_init() needs to know the IRQ
> number).
> 
> On x86 this additionally requires a slight ordering change to IRQ
> initialization, to facilitate calling the new hook between basic
> initialization and the call path leading to smp_intr_init().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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

[PATCH RFC 4/4] ns16550: enable use of PCI MSI

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 6900 bytes --]

Which, on x86, requires fiddling with the INTx bit in PCI config space,
since for internally used MSI we can't delegate this to Dom0.

ns16550_init_postirq() also needs (benign) re-ordering of its
operations.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC reason: No interrupts occur for an unknown reason (presumably
            broken hardware/firmware on the device I have).

--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -738,6 +738,16 @@ static int msi_capability_init(struct pc
 
     *desc = entry;
     /* Restore the original MSI enabled bits  */
+    if ( !hardware_domain )
+    {
+        /*
+         * Except for internal requests (before Dom0 starts), in which case
+         * we rather need to behave "normally", i.e. not follow the split
+         * brain model where Dom0 actually enables MSI (and disables INTx).
+         */
+        pci_intx(dev, 0);
+        control |= PCI_MSI_FLAGS_ENABLE;
+    }
     pci_conf_write16(seg, bus, slot, func, msi_control_reg(pos), control);
 
     return 0;
@@ -1071,6 +1081,8 @@ static void __pci_disable_msi(struct msi
 
     dev = entry->dev;
     msi_set_enable(dev, 0);
+    if ( entry->irq > 0 && !(irq_to_desc(entry->irq)->status & IRQ_GUEST) )
+        pci_intx(dev, 1);
 
     BUG_ON(list_empty(&dev->msi_list));
 }
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -74,6 +74,7 @@ static struct ns16550 {
     u32 bar64;
     u16 cr;
     u8 bar_idx;
+    bool_t msi;
     const struct ns16550_config_param *param; /* Points into .init.*! */
 #endif
 } ns16550_com[2] = { { 0 } };
@@ -644,6 +645,16 @@ static void __init ns16550_init_preirq(s
         uart->fifo_size = 16;
 }
 
+static void __init ns16550_init_irq(struct serial_port *port)
+{
+#ifdef CONFIG_HAS_PCI
+    struct ns16550 *uart = port->uart;
+
+    if ( uart->msi )
+        uart->irq = create_irq(0);
+#endif
+}
+
 static void ns16550_setup_postirq(struct ns16550 *uart)
 {
     if ( uart->irq > 0 )
@@ -678,17 +689,6 @@ static void __init ns16550_init_postirq(
     uart->timeout_ms = max_t(
         unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
 
-    if ( uart->irq > 0 )
-    {
-        uart->irqaction.handler = ns16550_interrupt;
-        uart->irqaction.name    = "ns16550";
-        uart->irqaction.dev_id  = port;
-        if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
-            printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq);
-    }
-
-    ns16550_setup_postirq(uart);
-
 #ifdef CONFIG_HAS_PCI
     if ( uart->bar || uart->ps_bdf_enable )
     {
@@ -709,8 +709,65 @@ static void __init ns16550_init_postirq(
                                     uart->ps_bdf[0], uart->ps_bdf[1],
                                     uart->ps_bdf[2]);
         }
+
+        if ( uart->msi )
+        {
+            struct msi_info msi = {
+                .bus = uart->ps_bdf[0],
+                .devfn = PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2]),
+                .irq = rc = uart->irq,
+                .entry_nr = 1
+            };
+
+            if ( rc > 0 )
+            {
+                struct msi_desc *msi_desc = NULL;
+
+                spin_lock(&pcidevs_lock);
+
+                rc = pci_enable_msi(&msi, &msi_desc);
+                if ( !rc )
+                {
+                    struct irq_desc *desc = irq_to_desc(msi.irq);
+                    unsigned long flags;
+
+                    spin_lock_irqsave(&desc->lock, flags);
+                    rc = setup_msi_irq(desc, msi_desc);
+                    spin_unlock_irqrestore(&desc->lock, flags);
+                    if ( rc )
+                        pci_disable_msi(msi_desc);
+                }
+
+                spin_unlock(&pcidevs_lock);
+
+                if ( rc )
+                {
+                    uart->irq = 0;
+                    if ( msi_desc )
+                        msi_free_irq(msi_desc);
+                    else
+                        destroy_irq(msi.irq);
+                }
+            }
+
+            if ( rc )
+                printk(XENLOG_WARNING
+                       "MSI setup failed (%d) for %02x:%02x.%o\n",
+                       rc, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]);
+        }
     }
 #endif
+
+    if ( uart->irq > 0 )
+    {
+        uart->irqaction.handler = ns16550_interrupt;
+        uart->irqaction.name    = "ns16550";
+        uart->irqaction.dev_id  = port;
+        if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
+            printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq);
+    }
+
+    ns16550_setup_postirq(uart);
 }
 
 static void ns16550_suspend(struct serial_port *port)
@@ -820,6 +877,7 @@ static const struct vuart_info *ns16550_
 
 static struct uart_driver __read_mostly ns16550_driver = {
     .init_preirq  = ns16550_init_preirq,
+    .init_irq     = ns16550_init_irq,
     .init_postirq = ns16550_init_postirq,
     .endboot      = ns16550_endboot,
     .suspend      = ns16550_suspend,
@@ -1123,7 +1181,18 @@ static void __init ns16550_parse_port_co
     }
 
     if ( *conf == ',' && *++conf != ',' )
-        uart->irq = simple_strtol(conf, &conf, 10);
+    {
+#ifdef CONFIG_HAS_PCI
+        if ( strncmp(conf, "msi", 3) == 0 )
+        {
+            conf += 3;
+            uart->msi = 1;
+            uart->irq = 0;
+        }
+        else
+#endif
+            uart->irq = simple_strtol(conf, &conf, 10);
+    }
 
 #ifdef CONFIG_HAS_PCI
     if ( *conf == ',' && *++conf != ',' )
--- a/xen/drivers/pci/pci.c
+++ b/xen/drivers/pci/pci.c
@@ -115,6 +115,21 @@ int pci_find_next_ext_capability(int seg
     return 0;
 }
 
+void pci_intx(const struct pci_dev *pdev, bool_t enable)
+{
+    uint16_t seg = pdev->seg;
+    uint8_t bus = pdev->bus;
+    uint8_t slot = PCI_SLOT(pdev->devfn);
+    uint8_t func = PCI_FUNC(pdev->devfn);
+    uint16_t cmd = pci_conf_read16(seg, bus, slot, func, PCI_COMMAND);
+
+    if ( enable )
+        cmd &= ~PCI_COMMAND_INTX_DISABLE;
+    else
+        cmd |= PCI_COMMAND_INTX_DISABLE;
+    pci_conf_write16(seg, bus, slot, func, PCI_COMMAND, cmd);
+}
+
 const char *__init parse_pci(const char *s, unsigned int *seg_p,
                              unsigned int *bus_p, unsigned int *dev_p,
                              unsigned int *func_p)
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -152,6 +152,7 @@ int pci_find_next_ext_capability(int seg
 const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus,
                       unsigned int *dev, unsigned int *func);
 
+void pci_intx(const struct pci_dev *, bool_t enable);
 bool_t pcie_aer_get_firmware_first(const struct pci_dev *);
 
 struct pirq;



[-- Attachment #2: ns16550-MSI.patch --]
[-- Type: text/plain, Size: 6930 bytes --]

ns16550: enable use of PCI MSI

Which, on x86, requires fiddling with the INTx bit in PCI config space,
since for internally used MSI we can't delegate this to Dom0.

ns16550_init_postirq() also needs (benign) re-ordering of its
operations.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC reason: No interrupts occur for an unknown reason (presumably
            broken hardware/firmware on the device I have).

--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -738,6 +738,16 @@ static int msi_capability_init(struct pc
 
     *desc = entry;
     /* Restore the original MSI enabled bits  */
+    if ( !hardware_domain )
+    {
+        /*
+         * Except for internal requests (before Dom0 starts), in which case
+         * we rather need to behave "normally", i.e. not follow the split
+         * brain model where Dom0 actually enables MSI (and disables INTx).
+         */
+        pci_intx(dev, 0);
+        control |= PCI_MSI_FLAGS_ENABLE;
+    }
     pci_conf_write16(seg, bus, slot, func, msi_control_reg(pos), control);
 
     return 0;
@@ -1071,6 +1081,8 @@ static void __pci_disable_msi(struct msi
 
     dev = entry->dev;
     msi_set_enable(dev, 0);
+    if ( entry->irq > 0 && !(irq_to_desc(entry->irq)->status & IRQ_GUEST) )
+        pci_intx(dev, 1);
 
     BUG_ON(list_empty(&dev->msi_list));
 }
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -74,6 +74,7 @@ static struct ns16550 {
     u32 bar64;
     u16 cr;
     u8 bar_idx;
+    bool_t msi;
     const struct ns16550_config_param *param; /* Points into .init.*! */
 #endif
 } ns16550_com[2] = { { 0 } };
@@ -644,6 +645,16 @@ static void __init ns16550_init_preirq(s
         uart->fifo_size = 16;
 }
 
+static void __init ns16550_init_irq(struct serial_port *port)
+{
+#ifdef CONFIG_HAS_PCI
+    struct ns16550 *uart = port->uart;
+
+    if ( uart->msi )
+        uart->irq = create_irq(0);
+#endif
+}
+
 static void ns16550_setup_postirq(struct ns16550 *uart)
 {
     if ( uart->irq > 0 )
@@ -678,17 +689,6 @@ static void __init ns16550_init_postirq(
     uart->timeout_ms = max_t(
         unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
 
-    if ( uart->irq > 0 )
-    {
-        uart->irqaction.handler = ns16550_interrupt;
-        uart->irqaction.name    = "ns16550";
-        uart->irqaction.dev_id  = port;
-        if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
-            printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq);
-    }
-
-    ns16550_setup_postirq(uart);
-
 #ifdef CONFIG_HAS_PCI
     if ( uart->bar || uart->ps_bdf_enable )
     {
@@ -709,8 +709,65 @@ static void __init ns16550_init_postirq(
                                     uart->ps_bdf[0], uart->ps_bdf[1],
                                     uart->ps_bdf[2]);
         }
+
+        if ( uart->msi )
+        {
+            struct msi_info msi = {
+                .bus = uart->ps_bdf[0],
+                .devfn = PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2]),
+                .irq = rc = uart->irq,
+                .entry_nr = 1
+            };
+
+            if ( rc > 0 )
+            {
+                struct msi_desc *msi_desc = NULL;
+
+                spin_lock(&pcidevs_lock);
+
+                rc = pci_enable_msi(&msi, &msi_desc);
+                if ( !rc )
+                {
+                    struct irq_desc *desc = irq_to_desc(msi.irq);
+                    unsigned long flags;
+
+                    spin_lock_irqsave(&desc->lock, flags);
+                    rc = setup_msi_irq(desc, msi_desc);
+                    spin_unlock_irqrestore(&desc->lock, flags);
+                    if ( rc )
+                        pci_disable_msi(msi_desc);
+                }
+
+                spin_unlock(&pcidevs_lock);
+
+                if ( rc )
+                {
+                    uart->irq = 0;
+                    if ( msi_desc )
+                        msi_free_irq(msi_desc);
+                    else
+                        destroy_irq(msi.irq);
+                }
+            }
+
+            if ( rc )
+                printk(XENLOG_WARNING
+                       "MSI setup failed (%d) for %02x:%02x.%o\n",
+                       rc, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]);
+        }
     }
 #endif
+
+    if ( uart->irq > 0 )
+    {
+        uart->irqaction.handler = ns16550_interrupt;
+        uart->irqaction.name    = "ns16550";
+        uart->irqaction.dev_id  = port;
+        if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
+            printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq);
+    }
+
+    ns16550_setup_postirq(uart);
 }
 
 static void ns16550_suspend(struct serial_port *port)
@@ -820,6 +877,7 @@ static const struct vuart_info *ns16550_
 
 static struct uart_driver __read_mostly ns16550_driver = {
     .init_preirq  = ns16550_init_preirq,
+    .init_irq     = ns16550_init_irq,
     .init_postirq = ns16550_init_postirq,
     .endboot      = ns16550_endboot,
     .suspend      = ns16550_suspend,
@@ -1123,7 +1181,18 @@ static void __init ns16550_parse_port_co
     }
 
     if ( *conf == ',' && *++conf != ',' )
-        uart->irq = simple_strtol(conf, &conf, 10);
+    {
+#ifdef CONFIG_HAS_PCI
+        if ( strncmp(conf, "msi", 3) == 0 )
+        {
+            conf += 3;
+            uart->msi = 1;
+            uart->irq = 0;
+        }
+        else
+#endif
+            uart->irq = simple_strtol(conf, &conf, 10);
+    }
 
 #ifdef CONFIG_HAS_PCI
     if ( *conf == ',' && *++conf != ',' )
--- a/xen/drivers/pci/pci.c
+++ b/xen/drivers/pci/pci.c
@@ -115,6 +115,21 @@ int pci_find_next_ext_capability(int seg
     return 0;
 }
 
+void pci_intx(const struct pci_dev *pdev, bool_t enable)
+{
+    uint16_t seg = pdev->seg;
+    uint8_t bus = pdev->bus;
+    uint8_t slot = PCI_SLOT(pdev->devfn);
+    uint8_t func = PCI_FUNC(pdev->devfn);
+    uint16_t cmd = pci_conf_read16(seg, bus, slot, func, PCI_COMMAND);
+
+    if ( enable )
+        cmd &= ~PCI_COMMAND_INTX_DISABLE;
+    else
+        cmd |= PCI_COMMAND_INTX_DISABLE;
+    pci_conf_write16(seg, bus, slot, func, PCI_COMMAND, cmd);
+}
+
 const char *__init parse_pci(const char *s, unsigned int *seg_p,
                              unsigned int *bus_p, unsigned int *dev_p,
                              unsigned int *func_p)
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -152,6 +152,7 @@ int pci_find_next_ext_capability(int seg
 const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus,
                       unsigned int *dev, unsigned int *func);
 
+void pci_intx(const struct pci_dev *, bool_t enable);
 bool_t pcie_aer_get_firmware_first(const struct pci_dev *);
 
 struct pirq;

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 0/4] ns16550: enable support for Pericom controllers

[Konrad Rzeszutek Wilk]

On Tue, Feb 23, 2016 at 04:22:35AM -0700, Jan Beulich wrote:
> Patches 1 and 2 are meant to go in. Patch 3 is a prerequisite to patch
> 4 and may go in as well, but patch 4 is RFC because with the Pericom
> board I have MSI doesn't appear to function. Since it also does not
> work in baremetal Linux when doing the trivial adjustments needed in
> its driver, I suspect the feature doesn't work in general, which is
> supported by the observation that the device continues to assert
> INTx despite the MSI enable bit being set (causing unclaimed IRQs
> until that IRQ gets shut off). While I got some responses back from
> Pericom support, no actual statement of whether MSI is actually
> known to work on their boards was ever made by them. I _think_
> patch 4 is correct (and hence could go in), but I have no way of
> proving this by testing.
> 
> 1: ns16550: store pointer to config parameters for PCI
> 2: ns16550: enable Pericom controller support
> 3: console: adjust IRQ initialization
> 4: ns16550: enable use of PCI MSI
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> (Konrad, I'd appreciate if you could double check that I didn't
> accidentally break the Oxford controller support.)

Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

I tested it on my one-port serial card. Let me move the cables + card
tomorrow around to use the two port card and see how it behaves.

CC-ing Boris
Boris, I think the two port serial card is tst005 or such - so it may get wonky.

> 

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

Re: [PATCH 0/4] ns16550: enable support for Pericom controllers

[Jan Beulich]

>>> On 29.02.16 at 17:56, <konrad.wilk@oracle.com> wrote:
> On Tue, Feb 23, 2016 at 04:22:35AM -0700, Jan Beulich wrote:
>> Patches 1 and 2 are meant to go in. Patch 3 is a prerequisite to patch
>> 4 and may go in as well, but patch 4 is RFC because with the Pericom
>> board I have MSI doesn't appear to function. Since it also does not
>> work in baremetal Linux when doing the trivial adjustments needed in
>> its driver, I suspect the feature doesn't work in general, which is
>> supported by the observation that the device continues to assert
>> INTx despite the MSI enable bit being set (causing unclaimed IRQs
>> until that IRQ gets shut off). While I got some responses back from
>> Pericom support, no actual statement of whether MSI is actually
>> known to work on their boards was ever made by them. I _think_
>> patch 4 is correct (and hence could go in), but I have no way of
>> proving this by testing.
>> 
>> 1: ns16550: store pointer to config parameters for PCI
>> 2: ns16550: enable Pericom controller support
>> 3: console: adjust IRQ initialization
>> 4: ns16550: enable use of PCI MSI
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> (Konrad, I'd appreciate if you could double check that I didn't
>> accidentally break the Oxford controller support.)
> 
> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thanks!

Jan


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

Ping: [PATCH 0/4] ns16550: enable support for Pericom controllers

[Jan Beulich]

>>> On 23.02.16 at 12:22, <JBeulich@suse.com> wrote:
> Patches 1 and 2 are meant to go in. Patch 3 is a prerequisite to patch
> 4 and may go in as well, but patch 4 is RFC because with the Pericom
> board I have MSI doesn't appear to function. Since it also does not
> work in baremetal Linux when doing the trivial adjustments needed in
> its driver, I suspect the feature doesn't work in general, which is
> supported by the observation that the device continues to assert
> INTx despite the MSI enable bit being set (causing unclaimed IRQs
> until that IRQ gets shut off). While I got some responses back from
> Pericom support, no actual statement of whether MSI is actually
> known to work on their boards was ever made by them. I _think_
> patch 4 is correct (and hence could go in), but I have no way of
> proving this by testing.
> 
> 1: ns16550: store pointer to config parameters for PCI
> 2: ns16550: enable Pericom controller support
> 3: console: adjust IRQ initialization
> 4: ns16550: enable use of PCI MSI
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> (Konrad, I'd appreciate if you could double check that I didn't
> accidentally break the Oxford controller support.)
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 




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