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

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

[Aravind Gopalakrishnan]

Since it is an MMIO device, the code has been modified to accept MMIO based
devices as well. MMIO device settings are populated in the 'uart_config' table.
It also advertises 64 bit BAR. Therefore, code is reworked to account for 64
bit BAR and 64 bit MMIO lengths.

Some more quirks are - the need to shift the register offset by a specific
value and we also need to verify (UART_LSR_THRE && UART_LSR_TEMT) bits before
transmitting data.

While testing, include com1=115200,8n1,pci,0 on the xen cmdline to observe
output on console using SoL.

Changes from V7:
  - per Jan's comments:
    - Moving pci_ro_device to ns16550_init_postirq() so that either
      one of pci_hide_device or pci_ro_device is done at one place
    - remove leading '0' from printk as absent segment identifier
      implies zero anyway.
  - per Ian's comments:
    - fixed issues that casued his build to fail.
    - cross-compiled for arm32 and arm64 after applying patch and
      build was successful on local machine.

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Signed-off-by: Thomas Lendacky <Thomas.Lendacky@amd.com>
---
 xen/drivers/char/ns16550.c | 187 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 166 insertions(+), 21 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 9c2cded..932d643 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -46,13 +46,14 @@ string_param("com2", opt_com2);
 static struct ns16550 {
     int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
     u64 io_base;   /* I/O port or memory-mapped I/O address. */
-    u32 io_size;
+    u64 io_size;
     int reg_shift; /* Bits to shift register offset by */
     int reg_width; /* Size of access to use, the registers
                     * themselves are still bytes */
     char __iomem *remapped_io_base;  /* Remapped virtual address of MMIO. */
     /* UART with IRQ line: interrupt-driven I/O. */
     struct irqaction irqaction;
+    u8 lsr_mask;
 #ifdef CONFIG_ARM
     struct vuart_info vuart;
 #endif
@@ -69,14 +70,50 @@ static struct ns16550 {
     bool_t pb_bdf_enable;   /* if =1, pb-bdf effective, port behind bridge */
     bool_t ps_bdf_enable;   /* if =1, ps_bdf effective, port on pci card */
     u32 bar;
+    u32 bar64;
     u16 cr;
     u8 bar_idx;
+    bool_t enable_ro; /* Make MMIO devices read only to Dom0 */
 #endif
 #ifdef HAS_DEVICE_TREE
     struct dt_irq dt_irq;
 #endif
 } ns16550_com[2] = { { 0 } };
 
+/* Defining uart config options for MMIO devices */
+struct ns16550_config_mmio {
+    u16 vendor_id;
+    u16 dev_id;
+    unsigned int reg_shift;
+    unsigned int reg_width;
+    unsigned int fifo_size;
+    u8 lsr_mask;
+    unsigned int max_bars;
+};
+
+
+#ifdef HAS_PCI
+/*
+ * Create lookup tables for specific MMIO devices..
+ * It is assumed that if the device found is MMIO,
+ * then you have indexed it here. Else, the driver
+ * does nothing.
+ */
+static struct ns16550_config_mmio __initdata uart_config[] =
+{
+    /* Broadcom TruManage device */
+    {
+        .vendor_id = 0x14e4,
+        .dev_id = 0x160a,
+        .reg_shift = 2,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
+        .max_bars = 1,
+    },
+};
+#endif
+
 static void ns16550_delayed_resume(void *data);
 
 static char ns_read_reg(struct ns16550 *uart, int reg)
@@ -134,7 +171,7 @@ static void ns16550_interrupt(
     while ( !(ns_read_reg(uart, UART_IIR) & UART_IIR_NOINT) )
     {
         char lsr = ns_read_reg(uart, UART_LSR);
-        if ( lsr & UART_LSR_THRE )
+        if ( (lsr & uart->lsr_mask) == uart->lsr_mask )
             serial_tx_interrupt(port, regs);
         if ( lsr & UART_LSR_DR )
             serial_rx_interrupt(port, regs);
@@ -160,7 +197,7 @@ static void __ns16550_poll(struct cpu_user_regs *regs)
         serial_rx_interrupt(port, regs);
     }
 
-    if ( ns_read_reg(uart, UART_LSR) & UART_LSR_THRE )
+    if ( ( ns_read_reg(uart, UART_LSR) & uart->lsr_mask ) == uart->lsr_mask )
         serial_tx_interrupt(port, regs);
 
 out:
@@ -183,7 +220,9 @@ static int ns16550_tx_ready(struct serial_port *port)
 
     if ( ns16550_ioport_invalid(uart) )
         return -EIO;
-    return ns_read_reg(uart, UART_LSR) & UART_LSR_THRE ? uart->fifo_size : 0;
+
+    return ( (ns_read_reg(uart, UART_LSR) &
+              uart->lsr_mask ) == uart->lsr_mask ) ? uart->fifo_size : 0;
 }
 
 static void ns16550_putc(struct serial_port *port, char c)
@@ -354,8 +393,24 @@ static void __init ns16550_init_postirq(struct serial_port *port)
 
 #ifdef HAS_PCI
     if ( uart->bar || uart->ps_bdf_enable )
-        pci_hide_device(uart->ps_bdf[0], PCI_DEVFN(uart->ps_bdf[1],
-                                                   uart->ps_bdf[2]));
+    {
+        if ( !uart->enable_ro )
+            pci_hide_device(uart->ps_bdf[0], PCI_DEVFN(uart->ps_bdf[1],
+                            uart->ps_bdf[2]));
+        else
+        {
+            if ( rangeset_add_range(mmio_ro_ranges,
+                                    uart->io_base,
+                                    uart->io_base + uart->io_size - 1) )
+                printk(XENLOG_INFO "Error while adding MMIO range of device to mmio_ro_ranges\n");
+
+            if ( pci_ro_device(0, uart->ps_bdf[0],
+                               PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2])) )
+                printk(XENLOG_INFO "Could not mark config space of %02x:%02x.%u read-only.\n",
+                                    uart->ps_bdf[0], uart->ps_bdf[1],
+                                    uart->ps_bdf[2]);
+        }
+    }
 #endif
 }
 
@@ -381,6 +436,13 @@ static void _ns16550_resume(struct serial_port *port)
     {
        pci_conf_write32(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2],
                         PCI_BASE_ADDRESS_0 + uart->bar_idx*4, uart->bar);
+
+        /* If 64 bit BAR, write higher 32 bits to BAR+4 */
+        if ( uart->bar & PCI_BASE_ADDRESS_MEM_TYPE_64 )
+            pci_conf_write32(0, uart->ps_bdf[0],
+                        uart->ps_bdf[1], uart->ps_bdf[2],
+                        PCI_BASE_ADDRESS_0 + (uart->bar_idx+1)*4, uart->bar64);
+
        pci_conf_write16(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2],
                         PCI_COMMAND, uart->cr);
     }
@@ -546,11 +608,13 @@ static int __init check_existence(struct ns16550 *uart)
 }
 
 #ifdef HAS_PCI
-static int
+static int __init
 pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
 {
-    uint32_t bar, len;
-    int b, d, f, nextf;
+    uint32_t bar, bar_64 = 0, len, len_64;
+    u64 size, mask;
+    unsigned int b, d, f, nextf, i;
+    u16 vendor, device;
 
     /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. */
     for ( b = skip_amt ? 1 : 0; b < 0x100; b++ )
@@ -579,24 +643,98 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
                 bar = pci_conf_read32(0, b, d, f,
                                       PCI_BASE_ADDRESS_0 + bar_idx*4);
 
-                /* Not IO */
+                /* MMIO based */
                 if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
-                    continue;
-
-                pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0, ~0u);
-                len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0);
-                pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0 + bar_idx*4, bar);
-
-                /* Not 8 bytes */
-                if ( (len & 0xffff) != 0xfff9 )
-                    continue;
+                {
+                    vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
+                    device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID);
+
+                    pci_conf_write32(0, b, d, f,
+                                     PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
+                    len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0 + bar_idx*4);
+                    pci_conf_write32(0, b, d, f,
+                                     PCI_BASE_ADDRESS_0 + bar_idx*4, bar);
+
+                    /* Handle 64 bit BAR if found */
+                    if ( bar & PCI_BASE_ADDRESS_MEM_TYPE_64 )
+                    {
+                        bar_64 = pci_conf_read32(0, b, d, f,
+                                      PCI_BASE_ADDRESS_0 + (bar_idx+1)*4);
+                        pci_conf_write32(0, b, d, f,
+                                    PCI_BASE_ADDRESS_0 + (bar_idx+1)*4, ~0u);
+                        len_64 = pci_conf_read32(0, b, d, f,
+                                    PCI_BASE_ADDRESS_0 + (bar_idx+1)*4);
+                        pci_conf_write32(0, b, d, f,
+                                    PCI_BASE_ADDRESS_0 + (bar_idx+1)*4, bar_64);
+                        mask = ((u64)~0 << 32) | PCI_BASE_ADDRESS_MEM_MASK;
+                        size = (((u64)len_64 << 32) | len) & mask;
+                    }
+                    else
+                        size = len & PCI_BASE_ADDRESS_MEM_MASK;
+
+                    size &= -size;
+
+                    /* Check for quirks in uart_config lookup table */
+                    for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
+                    {
+                        if ( uart_config[i].vendor_id != vendor )
+                            continue;
+
+                        if ( uart_config[i].dev_id != device )
+                            continue;
+
+                        /*
+                         * Force length of mmio region to be at least
+                         * 8 bytes times (1 << reg_shift)
+                         */
+                        if ( size < (0x8 * (1 << uart_config[i].reg_shift)) )
+                            continue;
+
+                        if ( bar_idx >= uart_config[i].max_bars )
+                            continue;
+
+                        if ( uart_config[i].fifo_size )
+                            uart->fifo_size = uart_config[i].fifo_size;
+
+                        uart->reg_shift = uart_config[i].reg_shift;
+                        uart->reg_width = uart_config[i].reg_width;
+                        uart->lsr_mask = uart_config[i].lsr_mask;
+                        uart->io_base = ((u64)bar_64 << 32) |
+                                        (bar & PCI_BASE_ADDRESS_MEM_MASK);
+                        /* Set device and MMIO region read only to Dom0 */
+                        uart->enable_ro = 1;
+                        break;
+                    }
+
+                    /* If we have an io_base, then we succeeded in the lookup */
+                    if ( !uart->io_base )
+                        continue;
+                }
+                /* IO based */
+                else
+                {
+                    pci_conf_write32(0, b, d, f,
+                                     PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
+                    len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0);
+                    pci_conf_write32(0, b, d, f,
+                                     PCI_BASE_ADDRESS_0 + bar_idx*4, bar);
+                    size = len & PCI_BASE_ADDRESS_IO_MASK;
+                    size &= -size;
+
+                    /* Not 8 bytes */
+                    if ( size != 0x8 )
+                        continue;
+
+                    uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO;
+                }
 
                 uart->ps_bdf[0] = b;
                 uart->ps_bdf[1] = d;
                 uart->ps_bdf[2] = f;
-                uart->bar = bar;
                 uart->bar_idx = bar_idx;
-                uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO;
+                uart->bar = bar;
+                uart->bar64 = bar_64;
+                uart->io_size = size;
                 uart->irq = pci_conf_read8(0, b, d, f, PCI_INTERRUPT_PIN) ?
                     pci_conf_read8(0, b, d, f, PCI_INTERRUPT_LINE) : 0;
 
@@ -743,9 +881,16 @@ void __init ns16550_init(int index, struct ns16550_defaults *defaults)
     uart->reg_width = 1;
     uart->reg_shift = 0;
 
+#ifdef HAS_PCI
+    uart->enable_ro = 0;
+#endif
+
     /* Default is no transmit FIFO. */
     uart->fifo_size = 1;
 
+    /* Default lsr_mask = UART_LSR_THRE */
+    uart->lsr_mask = UART_LSR_THRE;
+
     ns16550_parse_port_config(uart, (index == 0) ? opt_com1 : opt_com2);
 }
 
-- 
1.8.1.2

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

[Keir Fraser]

Aravind Gopalakrishnan wrote:
> Since it is an MMIO device, the code has been modified to accept MMIO based
> devices as well. MMIO device settings are populated in the 'uart_config' table.
> It also advertises 64 bit BAR. Therefore, code is reworked to account for 64
> bit BAR and 64 bit MMIO lengths.
>
> Some more quirks are - the need to shift the register offset by a specific
> value and we also need to verify (UART_LSR_THRE&&  UART_LSR_TEMT) bits before
> transmitting data.
>
> While testing, include com1=115200,8n1,pci,0 on the xen cmdline to observe
> output on console using SoL.
>
> Changes from V7:
>    - per Jan's comments:
>      - Moving pci_ro_device to ns16550_init_postirq() so that either
>        one of pci_hide_device or pci_ro_device is done at one place
>      - remove leading '0' from printk as absent segment identifier
>        implies zero anyway.
>    - per Ian's comments:
>      - fixed issues that casued his build to fail.
>      - cross-compiled for arm32 and arm64 after applying patch and
>        build was successful on local machine.
>
> Signed-off-by: Aravind Gopalakrishnan<Aravind.Gopalakrishnan@amd.com>
> Signed-off-by: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com>
> Signed-off-by: Thomas Lendacky<Thomas.Lendacky@amd.com>
Acked-by: Keir Fraser <keir@xen.org>

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

[Ian Campbell]

On Tue, 2014-02-25 at 15:11 -0600, Aravind Gopalakrishnan wrote:
> Since it is an MMIO device, the code has been modified to accept MMIO based
> devices as well. MMIO device settings are populated in the 'uart_config' table.
> It also advertises 64 bit BAR. Therefore, code is reworked to account for 64
> bit BAR and 64 bit MMIO lengths.
> 
> Some more quirks are - the need to shift the register offset by a specific
> value and we also need to verify (UART_LSR_THRE && UART_LSR_TEMT) bits before
> transmitting data.
> 
> While testing, include com1=115200,8n1,pci,0 on the xen cmdline to observe
> output on console using SoL.
> 
> Changes from V7:
>   - per Jan's comments:
>     - Moving pci_ro_device to ns16550_init_postirq() so that either
>       one of pci_hide_device or pci_ro_device is done at one place
>     - remove leading '0' from printk as absent segment identifier
>       implies zero anyway.
>   - per Ian's comments:
>     - fixed issues that casued his build to fail.
>     - cross-compiled for arm32 and arm64 after applying patch and
>       build was successful on local machine.

Unfortunately this patch breaks the console on the arm64 x-gene
platform, it drops a fairly large subset of the characters.

I'm not sure why yet.

Ian.

[PATCH] ns16550: setup default lsr_mask for DT systems too (Was: Re: [PATCH V8 RESEND] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips)

[Ian Campbell]

On Thu, 2014-03-20 at 16:47 +0000, Ian Campbell wrote:
> Unfortunately this patch breaks the console on the arm64 x-gene
> platform, it drops a fairly large subset of the characters.
> 
> I'm not sure why yet.

The simple fix is below.

I'm going to take a look at making ns16550_init and ns16550_uart_dt_init
share a common function for setting up the defaults, but for now I'd
appreciate getting this bandaid in.

Ian.

------8<----------------

>From 4fc6d9c71ae6b40940040910e27ba0f5272a2f27 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Thu, 20 Mar 2014 17:02:52 +0000
Subject: [PATCH] ns16550: setup default lsr_mask for DT systems too

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/drivers/char/ns16550.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 429d786..2dd32b2 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1129,6 +1129,8 @@ 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;
+    /* Default lsr_mask = UART_LSR_THRE */
+    uart->lsr_mask = UART_LSR_THRE;
 
     res = dt_device_get_address(dev, 0, &uart->io_base, &io_size);
     if ( res )
-- 
1.7.10.4

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

[Julien Grall]

Hi Ian,

On 03/20/2014 04:47 PM, Ian Campbell wrote:
> On Tue, 2014-02-25 at 15:11 -0600, Aravind Gopalakrishnan wrote:
>> Since it is an MMIO device, the code has been modified to accept MMIO based
>> devices as well. MMIO device settings are populated in the 'uart_config' table.
>> It also advertises 64 bit BAR. Therefore, code is reworked to account for 64
>> bit BAR and 64 bit MMIO lengths.
>>
>> Some more quirks are - the need to shift the register offset by a specific
>> value and we also need to verify (UART_LSR_THRE && UART_LSR_TEMT) bits before
>> transmitting data.
>>
>> While testing, include com1=115200,8n1,pci,0 on the xen cmdline to observe
>> output on console using SoL.
>>
>> Changes from V7:
>>   - per Jan's comments:
>>     - Moving pci_ro_device to ns16550_init_postirq() so that either
>>       one of pci_hide_device or pci_ro_device is done at one place
>>     - remove leading '0' from printk as absent segment identifier
>>       implies zero anyway.
>>   - per Ian's comments:
>>     - fixed issues that casued his build to fail.
>>     - cross-compiled for arm32 and arm64 after applying patch and
>>       build was successful on local machine.
> 
> Unfortunately this patch breaks the console on the arm64 x-gene
> platform, it drops a fairly large subset of the characters.
> 
> I'm not sure why yet.

This is because lsr_mask is not initialized for the device tree bit.

We should have something in ns16550_uart_dt_init like:

+    /* Default lsr_mask = UART_LSR_THRE */
+    uart->lsr_mask = UART_LSR_THRE;
+

I think in long term, ns16550_init and ns16550_uart_dt_init should share
this sort of initializations to avoid other failure on ARM.

Regards,

-- 
Julien Grall

Re: [PATCH] ns16550: setup default lsr_mask for DT systems too (Was: Re: [PATCH V8 RESEND] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips)

[Julien Grall]

On 03/20/2014 05:04 PM, Ian Campbell wrote:
> ------8<----------------
> 
> From 4fc6d9c71ae6b40940040910e27ba0f5272a2f27 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Thu, 20 Mar 2014 17:02:52 +0000
> Subject: [PATCH] ns16550: setup default lsr_mask for DT systems too
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>

> ---
>  xen/drivers/char/ns16550.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 429d786..2dd32b2 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1129,6 +1129,8 @@ 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;
> +    /* Default lsr_mask = UART_LSR_THRE */
> +    uart->lsr_mask = UART_LSR_THRE;
>  
>      res = dt_device_get_address(dev, 0, &uart->io_base, &io_size);
>      if ( res )
> 


-- 
Julien Grall

Re: [PATCH] ns16550: setup default lsr_mask for DT systems too (Was: Re: [PATCH V8 RESEND] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips)

[Konrad Rzeszutek Wilk]

On Thu, Mar 20, 2014 at 05:08:54PM +0000, Julien Grall wrote:
> On 03/20/2014 05:04 PM, Ian Campbell wrote:
> > ------8<----------------
> > 
> > From 4fc6d9c71ae6b40940040910e27ba0f5272a2f27 Mon Sep 17 00:00:00 2001
> > From: Ian Campbell <ian.campbell@citrix.com>
> > Date: Thu, 20 Mar 2014 17:02:52 +0000
> > Subject: [PATCH] ns16550: setup default lsr_mask for DT systems too
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Acked-by: Julien Grall <julien.grall@linaro.org>

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

> 
> > ---
> >  xen/drivers/char/ns16550.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> > index 429d786..2dd32b2 100644
> > --- a/xen/drivers/char/ns16550.c
> > +++ b/xen/drivers/char/ns16550.c
> > @@ -1129,6 +1129,8 @@ 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;
> > +    /* Default lsr_mask = UART_LSR_THRE */
> > +    uart->lsr_mask = UART_LSR_THRE;
> >  
> >      res = dt_device_get_address(dev, 0, &uart->io_base, &io_size);
> >      if ( res )
> > 
> 
> 
> -- 
> Julien Grall
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] ns16550: setup default lsr_mask for DT systems too (Was: Re: [PATCH V8 RESEND] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips)

[Pranavkumar Sawargaonkar]

Hi Ian,

On 20 March 2014 22:34, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2014-03-20 at 16:47 +0000, Ian Campbell wrote:
>> Unfortunately this patch breaks the console on the arm64 x-gene
>> platform, it drops a fairly large subset of the characters.
>>
>> I'm not sure why yet.
>
> The simple fix is below.
>
> I'm going to take a look at making ns16550_init and ns16550_uart_dt_init
> share a common function for setting up the defaults, but for now I'd
> appreciate getting this bandaid in.
>
> Ian.
>
> ------8<----------------
>
> From 4fc6d9c71ae6b40940040910e27ba0f5272a2f27 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Thu, 20 Mar 2014 17:02:52 +0000
> Subject: [PATCH] ns16550: setup default lsr_mask for DT systems too
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/drivers/char/ns16550.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 429d786..2dd32b2 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1129,6 +1129,8 @@ 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;
> +    /* Default lsr_mask = UART_LSR_THRE */
> +    uart->lsr_mask = UART_LSR_THRE;
>
>      res = dt_device_get_address(dev, 0, &uart->io_base, &io_size);
>      if ( res )
> --
> 1.7.10.4
>
>
>

It works for me on xgene.

Tested-By: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>

Thanks,
Pranav

[PATCH v2] ns16550: make some initialisation common

[Ian Campbell]

Should avoid accidents like forgetting to init lsr_mask for DT devices in the
future.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v2: remember to cc xen-devel...

Depends on "ns16550: setup default lsr_mask for DT systems too"
---
 xen/drivers/char/ns16550.c |   35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 2dd32b2..44e13b7 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1076,6 +1076,21 @@ static void __init ns16550_parse_port_config(
     serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
 }
 
+static void ns16550_init_common(struct ns16550 *uart)
+{
+    uart->clock_hz  = UART_CLOCK_HZ;
+
+#ifdef HAS_PCI
+    uart->enable_ro = 0;
+#endif
+
+    /* Default is no transmit FIFO. */
+    uart->fifo_size = 1;
+
+    /* Default lsr_mask = UART_LSR_THRE */
+    uart->lsr_mask  = UART_LSR_THRE;
+}
+
 void __init ns16550_init(int index, struct ns16550_defaults *defaults)
 {
     struct ns16550 *uart;
@@ -1085,10 +1100,11 @@ void __init ns16550_init(int index, struct ns16550_defaults *defaults)
 
     uart = &ns16550_com[index];
 
+    ns16550_init_common(uart);
+
     uart->baud      = (defaults->baud ? :
                        console_has((index == 0) ? "com1" : "com2")
                        ? BAUD_AUTO : 0);
-    uart->clock_hz  = UART_CLOCK_HZ;
     uart->data_bits = defaults->data_bits;
     uart->parity    = parse_parity_char(defaults->parity);
     uart->stop_bits = defaults->stop_bits;
@@ -1098,16 +1114,6 @@ void __init ns16550_init(int index, struct ns16550_defaults *defaults)
     uart->reg_width = 1;
     uart->reg_shift = 0;
 
-#ifdef HAS_PCI
-    uart->enable_ro = 0;
-#endif
-
-    /* Default is no transmit FIFO. */
-    uart->fifo_size = 1;
-
-    /* Default lsr_mask = UART_LSR_THRE */
-    uart->lsr_mask = UART_LSR_THRE;
-
     ns16550_parse_port_config(uart, (index == 0) ? opt_com1 : opt_com2);
 }
 
@@ -1122,15 +1128,12 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
 
     uart = &ns16550_com[0];
 
+    ns16550_init_common(uart);
+
     uart->baud      = BAUD_AUTO;
-    uart->clock_hz  = UART_CLOCK_HZ;
     uart->data_bits = 8;
     uart->parity    = UART_PARITY_NONE;
     uart->stop_bits = 1;
-    /* Default is no transmit FIFO. */
-    uart->fifo_size = 1;
-    /* Default lsr_mask = UART_LSR_THRE */
-    uart->lsr_mask = UART_LSR_THRE;
 
     res = dt_device_get_address(dev, 0, &uart->io_base, &io_size);
     if ( res )
-- 
1.7.10.4

Re: [PATCH v2] ns16550: make some initialisation common

[Keir Fraser]

Ian Campbell wrote:
> Should avoid accidents like forgetting to init lsr_mask for DT devices in the
> future.
>
> Signed-off-by: Ian Campbell<ian.campbell@citrix.com>
> Acked-by: Julien Grall<julien.grall@linaro.org>
> Reviewed-by: Jan Beulich<jbeulich@suse.com>
Acked-by: Keir Fraser <keir@xen.org>

Re: [PATCH v2] ns16550: make some initialisation common

[Ian Campbell]

On Fri, 2014-03-21 at 11:16 +0000, Keir Fraser wrote:
> Ian Campbell wrote:
> > Should avoid accidents like forgetting to init lsr_mask for DT devices in the
> > future.
> >
> > Signed-off-by: Ian Campbell<ian.campbell@citrix.com>
> > Acked-by: Julien Grall<julien.grall@linaro.org>
> > Reviewed-by: Jan Beulich<jbeulich@suse.com>
> Acked-by: Keir Fraser <keir@xen.org>

Thanks, do you also ack
http://bugs.xenproject.org/xen/mid/%3C1395335099.3104.41.camel@kazak.uk.xensource.com%3E
which is the actual bug fix (which this patch depends on)?

Ian

Re: [PATCH v2] ns16550: make some initialisation common

[Konrad Rzeszutek Wilk]

On Fri, Mar 21, 2014 at 11:16:11AM +0000, Keir Fraser wrote:
> Ian Campbell wrote:
> >Should avoid accidents like forgetting to init lsr_mask for DT devices in the
> >future.
> >
> >Signed-off-by: Ian Campbell<ian.campbell@citrix.com>
> >Acked-by: Julien Grall<julien.grall@linaro.org>
> >Reviewed-by: Jan Beulich<jbeulich@suse.com>
> Acked-by: Keir Fraser <keir@xen.org>


I think I should join the party too!

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

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

Re: [PATCH v2] ns16550: make some initialisation common

[Ian Campbell]

On Fri, 2014-03-21 at 09:32 -0400, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 21, 2014 at 11:16:11AM +0000, Keir Fraser wrote:
> > Ian Campbell wrote:
> > >Should avoid accidents like forgetting to init lsr_mask for DT devices in the
> > >future.
> > >
> > >Signed-off-by: Ian Campbell<ian.campbell@citrix.com>
> > >Acked-by: Julien Grall<julien.grall@linaro.org>
> > >Reviewed-by: Jan Beulich<jbeulich@suse.com>
> > Acked-by: Keir Fraser <keir@xen.org>
> 
> 
> I think I should join the party too!

Wouldn't want you to feel left out ;-)


> Reviewd-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel

Re: [PATCH] ns16550: setup default lsr_mask for DT systems too (Was: Re: [PATCH V8 RESEND] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips)

[Chen Baozi]

On Mar 21, 2014, at 1:04, Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Thu, 2014-03-20 at 16:47 +0000, Ian Campbell wrote:
>> Unfortunately this patch breaks the console on the arm64 x-gene
>> platform, it drops a fairly large subset of the characters.
>> 
>> I'm not sure why yet.
> 
> The simple fix is below.
> 
> I'm going to take a look at making ns16550_init and ns16550_uart_dt_init
> share a common function for setting up the defaults, but for now I'd
> appreciate getting this bandaid in.
> 
> Ian.
> 
> ------8<----------------
> 
> From 4fc6d9c71ae6b40940040910e27ba0f5272a2f27 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Thu, 20 Mar 2014 17:02:52 +0000
> Subject: [PATCH] ns16550: setup default lsr_mask for DT systems too
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Tested-By: Chen Baozi <baozich@gmail.com>

> ---
> xen/drivers/char/ns16550.c |    2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 429d786..2dd32b2 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1129,6 +1129,8 @@ 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;
> +    /* Default lsr_mask = UART_LSR_THRE */
> +    uart->lsr_mask = UART_LSR_THRE;
> 
>     res = dt_device_get_address(dev, 0, &uart->io_base, &io_size);
>     if ( res )
> -- 
> 1.7.10.4
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] ns16550: setup default lsr_mask for DT systems too (Was: Re: [PATCH V8 RESEND] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips)

[Ian Campbell]

(culling cc)

Keir, Ping?

On Thu, 2014-03-20 at 17:04 +0000, Ian Campbell wrote:
> From 4fc6d9c71ae6b40940040910e27ba0f5272a2f27 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Thu, 20 Mar 2014 17:02:52 +0000
> Subject: [PATCH] ns16550: setup default lsr_mask for DT systems too
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/drivers/char/ns16550.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 429d786..2dd32b2 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1129,6 +1129,8 @@ 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;
> +    /* Default lsr_mask = UART_LSR_THRE */
> +    uart->lsr_mask = UART_LSR_THRE;
>  
>      res = dt_device_get_address(dev, 0, &uart->io_base, &io_size);
>      if ( res )