[PATCH v1] Enable serial output for Oxford Semiconductor PCIe cards.

[PATCH v1] Enable serial output for Oxford Semiconductor PCIe cards.

[Konrad Rzeszutek Wilk]

Hey,

This is the first, non-RFC posting of the changes to the ns16550 driver
to also support other chipsets - like the Oxford 16C950 one - popular on
the inexpensive Rosewill RC-300E

The first patch is the one that enables the functionality on the
one I have - while the rest of the patches flesh out the code to support
more of PCI and PCIe type cards. The last patches adds in support for all
of the Oxford PCIe200 and PCIe952 chipsets that should work now with Xen.
Jan, if you prefer to skip those patches - that is fine with me.

 xen/drivers/char/ns16550.c |  258 ++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 240 insertions(+), 18 deletions(-)

Konrad Rzeszutek Wilk (4):
      serial: Support OXPCIe952 aka Oxford Semiconductor Ltd Device c138 (1415:c138)
      serial: Seperate the PCI device ids and quirks.
      serial: Use #defines for PCI vendor and models
      serial: Expand the PCI serial quirks for OXPCIe200 and OXPCIe952 1 Native UART

[PATCH v1 1/4] serial: Support OXPCIe952 aka Oxford Semiconductor Ltd Device c138 (1415:c138)

[Konrad Rzeszutek Wilk]

Because they are PCIe and machine nowadys have those instead of
PCI, and they are inexpensive.

Tested with 1415:c138. Should also work on 0xc11f, 0xc11b models
of that chip.

Also on  OXPCIe200 1 Native UART 1415: 0xc40b, 0xc40f, 0xc41b,
0xc41f, 0xc42b, 0xc42f, 0xc43b, 0xc43f, 0xc44b, 0xc44f, 0xc45b
0xc45f, 0xc46b, 0xc46f, 0xc47b, 0xc47f, 0xc48b, 0xc48f, 0xc49b
0xc49f, 0xc4ab, 0xc4af, 0xc4bb, 0xc4bf, 0xc4cb, 0xc4cf

but since I don't have any of those cards this patch does not
enable it.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v1: Init for ARM and add offset to virt addr]
---
 xen/drivers/char/ns16550.c |   30 +++++++++++++++++++++++++++---
 1 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 53e49a1..5dd2457 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -51,6 +51,7 @@ static struct ns16550 {
     int reg_width; /* Size of access to use, the registers
                     * themselves are still bytes */
     char __iomem *remapped_io_base;  /* Remapped virtual address of MMIO. */
+    unsigned int offset; /* By default zero. */
     /* UART with IRQ line: interrupt-driven I/O. */
     struct irqaction irqaction;
     u8 lsr_mask;
@@ -89,6 +90,9 @@ struct ns16550_config_mmio {
     unsigned int fifo_size;
     u8 lsr_mask;
     unsigned int max_bars;
+    unsigned int base_baud;
+    unsigned int uart_offset;
+    unsigned int first_offset;
 };
 
 
@@ -111,6 +115,19 @@ static struct ns16550_config_mmio __initdata uart_config[] =
         .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
         .max_bars = 1,
     },
+    /* OXPCIe952 1 Native UART  */
+    {
+        .vendor_id = 0x1415,
+        .dev_id = 0xc138,
+        .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.*/
+    }
 };
 #endif
 
@@ -325,11 +342,12 @@ static void __init ns16550_init_preirq(struct serial_port *port)
 #ifdef CONFIG_X86
         enum fixed_addresses idx = FIX_COM_BEGIN + (uart - ns16550_com);
 
-        set_fixmap_nocache(idx, uart->io_base);
+        set_fixmap_nocache(idx, uart->io_base + uart->offset);
         uart->remapped_io_base = (void __iomem *)fix_to_virt(idx);
-        uart->remapped_io_base += uart->io_base & ~PAGE_MASK;
+        uart->remapped_io_base += (uart->io_base + uart->offset) & ~PAGE_MASK;
 #else
-        uart->remapped_io_base = (char *)ioremap(uart->io_base, uart->io_size);
+        uart->remapped_io_base = (char *)ioremap(uart->io_base + uart->offset,
+                                                 uart->io_size);
 #endif
     }
 
@@ -701,6 +719,10 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
                         uart->lsr_mask = uart_config[i].lsr_mask;
                         uart->io_base = ((u64)bar_64 << 32) |
                                         (bar & PCI_BASE_ADDRESS_MEM_MASK);
+                        uart->offset = uart_config[i].first_offset;
+                        uart->offset += bar_idx * uart_config[i].uart_offset;
+                        if ( uart_config[i].base_baud )
+                            uart->clock_hz = uart_config[i].base_baud * 16;
                         /* Set device and MMIO region read only to Dom0 */
                         uart->enable_ro = 1;
                         break;
@@ -880,6 +902,7 @@ void __init ns16550_init(int index, struct ns16550_defaults *defaults)
     uart->io_size   = 8;
     uart->reg_width = 1;
     uart->reg_shift = 0;
+    uart->offset    = 0;
 
 #ifdef HAS_PCI
     uart->enable_ro = 0;
@@ -912,6 +935,7 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
     uart->stop_bits = 1;
     /* Default is no transmit FIFO. */
     uart->fifo_size = 1;
+    uart->offset    = 0;
 
     res = dt_device_get_address(dev, 0, &uart->io_base, &io_size);
     if ( res )
-- 
1.7.7.6

[PATCH v1 2/4] serial: Seperate the PCI device ids and quirks.

[Konrad Rzeszutek Wilk]

This will allow us to re-use the quirks for multiple PCI
devices.

No functional change.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/drivers/char/ns16550.c |   66 ++++++++++++++++++++++++++++---------------
 1 files changed, 43 insertions(+), 23 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 5dd2457..06580c8 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -81,10 +81,14 @@ static struct ns16550 {
 #endif
 } ns16550_com[2] = { { 0 } };
 
-/* Defining uart config options for MMIO devices */
 struct ns16550_config_mmio {
     u16 vendor_id;
     u16 dev_id;
+    unsigned int quirk;
+};
+
+/* Defining uart config options for MMIO devices */
+struct ns16550_config_quirk {
     unsigned int reg_shift;
     unsigned int reg_width;
     unsigned int fifo_size;
@@ -95,30 +99,28 @@ struct ns16550_config_mmio {
     unsigned int first_offset;
 };
 
-
 #ifdef HAS_PCI
+enum ns16550_config_quirk_nr {
+    quirk_default = 0,
+    quirk_trumanage,
+    quirk_oxford,
+};
 /*
  * Create lookup tables for specific MMIO devices..
  * It is assumed that if the device found is MMIO,
  * then you have indexed it here. Else, the driver
  * does nothing.
  */
-static struct ns16550_config_mmio __initdata uart_config[] =
-{
-    /* Broadcom TruManage device */
-    {
-        .vendor_id = 0x14e4,
-        .dev_id = 0x160a,
+static struct ns16550_config_quirk __initdata uart_quirk[] = {
+    [quirk_default] = { }, /* Ignored. */
+    [quirk_trumanage] = {
         .reg_shift = 2,
         .reg_width = 1,
         .fifo_size = 16,
         .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
         .max_bars = 1,
     },
-    /* OXPCIe952 1 Native UART  */
-    {
-        .vendor_id = 0x1415,
-        .dev_id = 0xc138,
+    [quirk_oxford] = {
         .base_baud = 4000000,
         .uart_offset = 0x200,
         .first_offset = 0x1000,
@@ -129,6 +131,21 @@ static struct ns16550_config_mmio __initdata uart_config[] =
         .max_bars = 1, /* It can do more, but we would need more custom code.*/
     }
 };
+static struct ns16550_config_mmio __initdata uart_config[] =
+{
+    /* Broadcom TruManage device */
+    {
+        .vendor_id = 0x14e4,
+        .dev_id = 0x160a,
+        .quirk = quirk_trumanage,
+    },
+    /* OXPCIe952 1 Native UART  */
+    {
+        .vendor_id = 0x1415,
+        .dev_id = 0xc138,
+        .quirk = quirk_oxford,
+    }
+};
 #endif
 
 static void ns16550_delayed_resume(void *data);
@@ -695,34 +712,37 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
                     /* Check for quirks in uart_config lookup table */
                     for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
                     {
+                        unsigned int q;
+
                         if ( uart_config[i].vendor_id != vendor )
                             continue;
 
                         if ( uart_config[i].dev_id != device )
                             continue;
 
+                        q = uart_config[i].quirk;
                         /*
                          * Force length of mmio region to be at least
                          * 8 bytes times (1 << reg_shift)
                          */
-                        if ( size < (0x8 * (1 << uart_config[i].reg_shift)) )
+                        if ( size < (0x8 * (1 << uart_quirk[q].reg_shift)) )
                             continue;
 
-                        if ( bar_idx >= uart_config[i].max_bars )
+                        if ( bar_idx >= uart_quirk[q].max_bars )
                             continue;
 
-                        if ( uart_config[i].fifo_size )
-                            uart->fifo_size = uart_config[i].fifo_size;
+                        if ( uart_quirk[q].fifo_size )
+                            uart->fifo_size = uart_quirk[q].fifo_size;
 
-                        uart->reg_shift = uart_config[i].reg_shift;
-                        uart->reg_width = uart_config[i].reg_width;
-                        uart->lsr_mask = uart_config[i].lsr_mask;
+                        uart->reg_shift = uart_quirk[q].reg_shift;
+                        uart->reg_width = uart_quirk[q].reg_width;
+                        uart->lsr_mask = uart_quirk[q].lsr_mask;
                         uart->io_base = ((u64)bar_64 << 32) |
                                         (bar & PCI_BASE_ADDRESS_MEM_MASK);
-                        uart->offset = uart_config[i].first_offset;
-                        uart->offset += bar_idx * uart_config[i].uart_offset;
-                        if ( uart_config[i].base_baud )
-                            uart->clock_hz = uart_config[i].base_baud * 16;
+                        uart->offset = uart_quirk[q].first_offset;
+                        uart->offset += bar_idx * uart_quirk[q].uart_offset;
+                        if ( uart_quirk[q].base_baud )
+                            uart->clock_hz = uart_quirk[q].base_baud * 16;
                         /* Set device and MMIO region read only to Dom0 */
                         uart->enable_ro = 1;
                         break;
-- 
1.7.7.6

[PATCH v1 3/4] serial: Use #defines for PCI vendor and models

[Konrad Rzeszutek Wilk]

Instead of having hard-coded values.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/drivers/char/ns16550.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 06580c8..6fbf358 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -131,17 +131,21 @@ static struct ns16550_config_quirk __initdata uart_quirk[] = {
         .max_bars = 1, /* It can do more, but we would need more custom code.*/
     }
 };
+
+#define PCI_VENDOR_ID_BROADCOM           0x14e4
+#define PCI_DEVICE_ID_BROADCOM_TRUMANAGE 0x160a
+#define PCI_VENDOR_ID_OXSEMI             0x1415
 static struct ns16550_config_mmio __initdata uart_config[] =
 {
     /* Broadcom TruManage device */
     {
-        .vendor_id = 0x14e4,
-        .dev_id = 0x160a,
+        .vendor_id = PCI_VENDOR_ID_BROADCOM,
+        .dev_id = PCI_DEVICE_ID_BROADCOM_TRUMANAGE,
         .quirk = quirk_trumanage,
     },
     /* OXPCIe952 1 Native UART  */
     {
-        .vendor_id = 0x1415,
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
         .dev_id = 0xc138,
         .quirk = quirk_oxford,
     }
-- 
1.7.7.6

[PATCH v1 4/4] serial: Expand the PCI serial quirks for OXPCIe200 and OXPCIe952 1 Native UART

[Konrad Rzeszutek Wilk]

This covers all of the OXPCIe952 1 Native UART and
OXPCIe200 1 Native UART chipsets.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/drivers/char/ns16550.c |  174 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 174 insertions(+), 0 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 6fbf358..487bf7d 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -146,8 +146,182 @@ static struct ns16550_config_mmio __initdata uart_config[] =
     /* OXPCIe952 1 Native UART  */
     {
         .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc11b,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe952 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc11f,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe952 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
         .dev_id = 0xc138,
         .quirk = quirk_oxford,
+    },
+    /* OXPCIe952 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc13d,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe952 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc40b,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc40f,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc41b,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc41f,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc42b,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc42f,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc43b,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc43f,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc44b,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc44f,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc45b,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc45f,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc46b,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc46f,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc47b,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc47f,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc48b,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc48f,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc49b,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc49f,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc4ab,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc4af,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc4bb,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc4bf,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc4cb,
+        .quirk = quirk_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc4cf,
+        .quirk = quirk_oxford,
     }
 };
 #endif
-- 
1.7.7.6

Re: [PATCH v1 3/4] serial: Use #defines for PCI vendor and models

[Andrew Cooper]

On 05/03/14 17:25, Konrad Rzeszutek Wilk wrote:
> Instead of having hard-coded values.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen/drivers/char/ns16550.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 06580c8..6fbf358 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -131,17 +131,21 @@ static struct ns16550_config_quirk __initdata uart_quirk[] = {
>          .max_bars = 1, /* It can do more, but we would need more custom code.*/
>      }
>  };
> +
> +#define PCI_VENDOR_ID_BROADCOM           0x14e4
> +#define PCI_DEVICE_ID_BROADCOM_TRUMANAGE 0x160a
> +#define PCI_VENDOR_ID_OXSEMI             0x1415

We probably want a single unified header file containing PCI IDs
(certainly vendor).  Currently, arch/x86/x86_64/mmconfig.h contains
defines for Intel, AMD and Nvidia, and AMD is redefined in defined in
arch/x86/oprofile/op_model_athlon.c.

~Andrew

>  static struct ns16550_config_mmio __initdata uart_config[] =
>  {
>      /* Broadcom TruManage device */
>      {
> -        .vendor_id = 0x14e4,
> -        .dev_id = 0x160a,
> +        .vendor_id = PCI_VENDOR_ID_BROADCOM,
> +        .dev_id = PCI_DEVICE_ID_BROADCOM_TRUMANAGE,
>          .quirk = quirk_trumanage,
>      },
>      /* OXPCIe952 1 Native UART  */
>      {
> -        .vendor_id = 0x1415,
> +        .vendor_id = PCI_VENDOR_ID_OXSEMI,
>          .dev_id = 0xc138,
>          .quirk = quirk_oxford,
>      }

Re: [PATCH v1 1/4] serial: Support OXPCIe952 aka Oxford Semiconductor Ltd Device c138 (1415:c138)

[Jan Beulich]

>>> On 05.03.14 at 18:25, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> @@ -325,11 +342,12 @@ static void __init ns16550_init_preirq(struct serial_port *port)
>  #ifdef CONFIG_X86
>          enum fixed_addresses idx = FIX_COM_BEGIN + (uart - ns16550_com);
>  
> -        set_fixmap_nocache(idx, uart->io_base);
> +        set_fixmap_nocache(idx, uart->io_base + uart->offset);
>          uart->remapped_io_base = (void __iomem *)fix_to_virt(idx);
> -        uart->remapped_io_base += uart->io_base & ~PAGE_MASK;
> +        uart->remapped_io_base += (uart->io_base + uart->offset) & ~PAGE_MASK;
>  #else
> -        uart->remapped_io_base = (char *)ioremap(uart->io_base, uart->io_size);
> +        uart->remapped_io_base = (char *)ioremap(uart->io_base + uart->offset,
> +                                                 uart->io_size);
>  #endif

Looking at this again, I have a hard time seeing why you can't simply
set uart->io_base to the full, final value ...

> @@ -701,6 +719,10 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
>                          uart->lsr_mask = uart_config[i].lsr_mask;
>                          uart->io_base = ((u64)bar_64 << 32) |
>                                          (bar & PCI_BASE_ADDRESS_MEM_MASK);
> +                        uart->offset = uart_config[i].first_offset;
> +                        uart->offset += bar_idx * uart_config[i].uart_offset;

... here, and drop the offset field again.

Jan

Re: [PATCH v1 2/4] serial: Seperate the PCI device ids and quirks.

[Jan Beulich]

>>> On 05.03.14 at 18:25, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> This will allow us to re-use the quirks for multiple PCI
> devices.

Looks all reasonable except for the question on the name: Why do
you consider these "quirks"? Aren't they just parameters?

Jan

Re: [PATCH v1 3/4] serial: Use #defines for PCI vendor and models

[Jan Beulich]

>>> On 05.03.14 at 18:25, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -131,17 +131,21 @@ static struct ns16550_config_quirk __initdata uart_quirk[] = {
>          .max_bars = 1, /* It can do more, but we would need more custom code.*/
>      }
>  };
> +
> +#define PCI_VENDOR_ID_BROADCOM           0x14e4
> +#define PCI_DEVICE_ID_BROADCOM_TRUMANAGE 0x160a
> +#define PCI_VENDOR_ID_OXSEMI             0x1415
>  static struct ns16550_config_mmio __initdata uart_config[] =
>  {
>      /* Broadcom TruManage device */
>      {
> -        .vendor_id = 0x14e4,
> -        .dev_id = 0x160a,
> +        .vendor_id = PCI_VENDOR_ID_BROADCOM,
> +        .dev_id = PCI_DEVICE_ID_BROADCOM_TRUMANAGE,
>          .quirk = quirk_trumanage,
>      },
>      /* OXPCIe952 1 Native UART  */
>      {
> -        .vendor_id = 0x1415,
> +        .vendor_id = PCI_VENDOR_ID_OXSEMI,
>          .dev_id = 0xc138,

And why not for this one?

Jan

Re: [PATCH v1 4/4] serial: Expand the PCI serial quirks for OXPCIe200 and OXPCIe952 1 Native UART

[Jan Beulich]

>>> On 05.03.14 at 18:25, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> This covers all of the OXPCIe952 1 Native UART and
> OXPCIe200 1 Native UART chipsets.

With this I can certainly see why you didn't give the single ID a
name in the previous patch. But let's be consistent then and have
- for now at least - only vendor IDs have (global, as Andrew
validly suggested) manifest constants.

Jan

Re: [PATCH v1 1/4] serial: Support OXPCIe952 aka Oxford Semiconductor Ltd Device c138 (1415:c138)

[Konrad Rzeszutek Wilk]

On Thu, Mar 06, 2014 at 10:15:08AM +0000, Jan Beulich wrote:
> >>> On 05.03.14 at 18:25, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> > @@ -325,11 +342,12 @@ static void __init ns16550_init_preirq(struct serial_port *port)
> >  #ifdef CONFIG_X86
> >          enum fixed_addresses idx = FIX_COM_BEGIN + (uart - ns16550_com);
> >  
> > -        set_fixmap_nocache(idx, uart->io_base);
> > +        set_fixmap_nocache(idx, uart->io_base + uart->offset);
> >          uart->remapped_io_base = (void __iomem *)fix_to_virt(idx);
> > -        uart->remapped_io_base += uart->io_base & ~PAGE_MASK;
> > +        uart->remapped_io_base += (uart->io_base + uart->offset) & ~PAGE_MASK;
> >  #else
> > -        uart->remapped_io_base = (char *)ioremap(uart->io_base, uart->io_size);
> > +        uart->remapped_io_base = (char *)ioremap(uart->io_base + uart->offset,
> > +                                                 uart->io_size);
> >  #endif
> 
> Looking at this again, I have a hard time seeing why you can't simply
> set uart->io_base to the full, final value ...

I was trying to make it obvious that it is not just io_base but also
an offset.
> 
> > @@ -701,6 +719,10 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
> >                          uart->lsr_mask = uart_config[i].lsr_mask;
> >                          uart->io_base = ((u64)bar_64 << 32) |
> >                                          (bar & PCI_BASE_ADDRESS_MEM_MASK);
> > +                        uart->offset = uart_config[i].first_offset;
> > +                        uart->offset += bar_idx * uart_config[i].uart_offset;
> 
> ... here, and drop the offset field again.

But on the other hand - why even do that - when all of that can nicely
be put in there with an explanation.

Will rebase! Thank you again for your review!
> 
> Jan
> 

Re: [PATCH v1 2/4] serial: Seperate the PCI device ids and quirks.

[Konrad Rzeszutek Wilk]

On Thu, Mar 06, 2014 at 10:17:15AM +0000, Jan Beulich wrote:
> >>> On 05.03.14 at 18:25, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> > This will allow us to re-use the quirks for multiple PCI
> > devices.
> 
> Looks all reasonable except for the question on the name: Why do
> you consider these "quirks"? Aren't they just parameters?

Good point. Will s/quirk/param/ on the patches.
> 
> Jan
> 

Re: [PATCH v1 4/4] serial: Expand the PCI serial quirks for OXPCIe200 and OXPCIe952 1 Native UART

[Konrad Rzeszutek Wilk]

On Thu, Mar 06, 2014 at 10:20:47AM +0000, Jan Beulich wrote:
> >>> On 05.03.14 at 18:25, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> > This covers all of the OXPCIe952 1 Native UART and
> > OXPCIe200 1 Native UART chipsets.
> 
> With this I can certainly see why you didn't give the single ID a
> name in the previous patch. But let's be consistent then and have
> - for now at least - only vendor IDs have (global, as Andrew
> validly suggested) manifest constants.

Do you want that just for the serial driver or for all?

This is what I had ready:


commit c3dbcc6a0c2c9415201863a9422b550270f2ee03
Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date:   Wed Mar 5 15:24:04 2014 -0500

    pci: Put all PCI device vendor and models in one file.
    
    Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
    Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

diff --git a/xen/arch/x86/oprofile/op_model_athlon.c b/xen/arch/x86/oprofile/op_model_athlon.c
index e784018..81e0858 100644
--- a/xen/arch/x86/oprofile/op_model_athlon.c
+++ b/xen/arch/x86/oprofile/op_model_athlon.c
@@ -20,7 +20,8 @@
 #include <asm/current.h>
 #include <asm/hvm/support.h>
 #include <xen/pci_regs.h>
- 
+#include <xen/pci_devs.h>
+
 #include "op_x86_model.h"
 #include "op_counter.h"
 
@@ -445,8 +446,6 @@ static inline void __init init_ibs_nmi_per_cpu(void *arg)
 	apic_write(reg, APIC_EILVT_MSG_NMI << 8);
 }
 
-#define PCI_VENDOR_ID_AMD               0x1022
-#define PCI_DEVICE_ID_AMD_10H_NB_MISC   0x1203
 #define IBSCTL                          0x1cc
 static int __init init_ibs_nmi(void)
 {
diff --git a/xen/arch/x86/x86_64/mmconf-fam10h.c b/xen/arch/x86/x86_64/mmconf-fam10h.c
index 1373acb..07668b3 100644
--- a/xen/arch/x86/x86_64/mmconf-fam10h.c
+++ b/xen/arch/x86/x86_64/mmconf-fam10h.c
@@ -6,6 +6,7 @@
 #include <xen/acpi.h>
 #include <xen/pci.h>
 #include <xen/pci_regs.h>
+#include <xen/pci_devs.h>
 #include <xen/init.h>
 #include <xen/dmi.h>
 #include <asm/amd.h>
diff --git a/xen/arch/x86/x86_64/mmconfig-shared.c b/xen/arch/x86/x86_64/mmconfig-shared.c
index 7589b64..75757b4 100644
--- a/xen/arch/x86/x86_64/mmconfig-shared.c
+++ b/xen/arch/x86/x86_64/mmconfig-shared.c
@@ -19,6 +19,7 @@
 #include <xen/xmalloc.h>
 #include <xen/pci.h>
 #include <xen/pci_regs.h>
+#include <xen/pci_devs.h>
 #include <asm/e820.h>
 #include <asm/msr.h>
 #include <asm/msr-index.h>
diff --git a/xen/arch/x86/x86_64/mmconfig.h b/xen/arch/x86/x86_64/mmconfig.h
index 36e0448..5125997 100644
--- a/xen/arch/x86/x86_64/mmconfig.h
+++ b/xen/arch/x86/x86_64/mmconfig.h
@@ -17,10 +17,6 @@
  * Author: Allen Kay <allen.m.kay@intel.com> - adapted from linux
  */
 
-#define PCI_VENDOR_ID_INTEL        0x8086
-#define PCI_DEVICE_ID_INTEL_E7520_MCH    0x3590
-#define PCI_DEVICE_ID_INTEL_82945G_HB    0x2770
-
 /* ioport ends */
 #define PCI_PROBE_BIOS        0x0001
 #define PCI_PROBE_CONF1        0x0002
@@ -29,11 +25,8 @@
 #define PCI_PROBE_MASK        0x000f
 #define PCI_PROBE_NOEARLY    0x0010
 
-#define PCI_VENDOR_ID_AMD             0x1022
 #define PCI_CHECK_ENABLE_AMD_MMCONF     0x20000
 
-#define PCI_VENDOR_ID_NVIDIA       0x10de
-
 extern unsigned int pci_probe;
 
 /*
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 9f87018..48ee010 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -19,6 +19,7 @@
 #ifdef HAS_PCI
 #include <xen/pci.h>
 #include <xen/pci_regs.h>
+#include <xen/pci_devs.h>
 #endif
 #include <xen/8250-uart.h>
 #include <xen/vmap.h>
@@ -132,9 +133,6 @@ static struct ns16550_config_quirk __initdata uart_quirk[] = {
     }
 };
 
-#define PCI_VENDOR_ID_BROADCOM           0x14e4
-#define PCI_VENDOR_ID_OXSEMI             0x1415
 static struct ns16550_config_mmio __initdata uart_config[] =
 {
     /* Broadcom TruManage device */
diff --git a/xen/include/xen/pci_devs.h b/xen/include/xen/pci_devs.h
new file mode 100644
index 0000000..ad573df
--- /dev/null
+++ b/xen/include/xen/pci_devs.h
@@ -0,0 +1,17 @@
+#ifndef XEN_PCI_DEVS_H_
+#define XEN_PCI_DEVS_H
+
+#define PCI_VENDOR_ID_AMD                0x1022
+#define PCI_DEVICE_ID_AMD_10H_NB_MISC    0x1203
+
+#define PCI_VENDOR_ID_BROADCOM           0x14e4
+
+#define PCI_VENDOR_ID_INTEL              0x8086
+#define PCI_DEVICE_ID_INTEL_E7520_MCH    0x3590
+#define PCI_DEVICE_ID_INTEL_82945G_HB    0x2770
+
+#define PCI_VENDOR_ID_NVIDIA             0x10de
+
+#define PCI_VENDOR_ID_OXSEMI             0x1415
+#endif /* XEN_PCI_DEVS_H */
> 
> Jan
> 

Re: [PATCH v1 4/4] serial: Expand the PCI serial quirks for OXPCIe200 and OXPCIe952 1 Native UART

[Jan Beulich]

>>> On 07.03.14 at 16:55, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Thu, Mar 06, 2014 at 10:20:47AM +0000, Jan Beulich wrote:
>> >>> On 05.03.14 at 18:25, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
>> > This covers all of the OXPCIe952 1 Native UART and
>> > OXPCIe200 1 Native UART chipsets.
>> 
>> With this I can certainly see why you didn't give the single ID a
>> name in the previous patch. But let's be consistent then and have
>> - for now at least - only vendor IDs have (global, as Andrew
>> validly suggested) manifest constants.
> 
> Do you want that just for the serial driver or for all?

Ultimately for the whole tree, but I'm not necessarily asking you
to do this in one go. Whatever you have available would be a
good starting point. I.e. what you have should be fine to go
(albeit as said I'm not sure we really want/need device IDs
there - it's more the exception than the rule to need the same
device ID in more than one place), with one request for change:

> --- /dev/null
> +++ b/xen/include/xen/pci_devs.h

Please name this pci_ids.h, to match Linux.

> @@ -0,0 +1,17 @@
> +#ifndef XEN_PCI_DEVS_H_
> +#define XEN_PCI_DEVS_H

And please along with adjusting this for the changed name fix the
discrepancy in spelling. In fact we don't really need a guard in this
header, since it only consists of #define-s (see Linux'es), so you
might as well drop it.

Jan