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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 14/11/13 16:29, Aravind Gopalakrishnan wrote:
> There are few quirks regarding the chip:
> Firstly, it is an MMIO device. Therefore, the code has been modified to
> accept MMIO based devices as well. Settings particular to such devices are
> populated  in the table 'uart_config'. Currently, we only support BCM5725
> TruManage chip.
>
> 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, we ned to include com1=115200,8n1,pci,0 on the xen cmdline to
> be able to observe output using SOL.
>
> 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 | 98 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 86 insertions(+), 12 deletions(-)
>
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 9c2cded..05d77ba 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -53,6 +53,7 @@ static struct ns16550 {
>      char __iomem *remapped_io_base;  /* Remapped virtual address of MMIO. */
>      /* UART with IRQ line: interrupt-driven I/O. */
>      struct irqaction irqaction;
> +    u32 lsr_mask;
>  #ifdef CONFIG_ARM
>      struct vuart_info vuart;
>  #endif
> @@ -77,6 +78,37 @@ static struct ns16550 {
>  #endif
>  } ns16550_com[2] = { { 0 } };
>  
> +/* Defining uart config options for MMIO devices */
> +struct ns16550_config_mmio {
> +    u16 vendor_id;
> +    u16 dev_id;
> +    int reg_shift;

Shifts of signed quantities are a recipe for undefined behaviour.  I
dont see any reason for any of these values to be signed quantities.

> +    int reg_width; 
> +    int fifo_size;
> +    u32 lsr_mask;
> +    int max_bars;
> +};
> +
> +/*
> + * Create lookup tables for specific MMIO devices..
> + * It is assumed that if the device found is MMIO,
> + * then you have indexed it here. Else, the driver
> + * does nothing.
> + */
> +static struct ns16550_config_mmio uart_config[] =
> +{
> +    /* Broadcom TruManage device */
> +    { 
> +        .vendor_id = 0x14e4,
> +        .dev_id = 0x160a,
> +        .reg_shift = 2,
> +        .reg_width = 1,
> +        .fifo_size = 64,
> +        .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
> +        .max_bars = 1,
> +    },
> +};
> +
>  static void ns16550_delayed_resume(void *data);
>  
>  static char ns_read_reg(struct ns16550 *uart, int reg)
> @@ -134,7 +166,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 +192,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 +215,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)
> @@ -550,7 +584,8 @@ static int
>  pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
>  {
>      uint32_t bar, len;
> -    int b, d, f, nextf;
> +    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,27 +614,63 @@ 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);
>  
> +                if ( !bar )
> +                    continue;
> +

What is this check supposed to be doing?  This could be a valid
non-prefechable 32bit memory bar.

>                  /* Not IO */
>                  if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
> -                    continue;
> +                {
> +                    vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
> +                    device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID);
> +
> +                    /* Check for quirks in uart_config lookup table */
> +                    for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
> +                    {
> +                        if ( uart_config[i].vendor_id != vendor )
> +                            continue;
> +                       
> +                        if ( uart_config[i].dev_id != device )
> +                            continue;
> +
> +                        if ( bar_idx >= uart_config[i].max_bars )
> +                            break;
> +
> +                        uart->reg_shift = uart_config[i].reg_shift;
> +                        uart->reg_width = uart_config[i].reg_width;
> +                        uart->fifo_size = uart_config[i].fifo_size;
> +                        uart->lsr_mask = uart_config[i].lsr_mask;
> +                        uart->io_base = bar & PCI_BASE_ADDRESS_MEM_MASK;
> +                        break;
> +                    }
> +
> +                    /* If we have an io_base, then we succeeded in the lookup */
> +                    if ( !uart->io_base )
> +                        continue;
> +		        }
> +                /* IO based */
> +                else
> +                {
> +                    pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0, ~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);
>  
> -                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;
>  
> -                /* Not 8 bytes */
> -                if ( (len & 0xffff) != 0xfff9 )
> -                    continue;
> +                    uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO;
> +                }
>  
>                  uart->ps_bdf[0] = b;
>                  uart->ps_bdf[1] = d;
>                  uart->ps_bdf[2] = f;
>                  uart->bar = bar;
>                  uart->bar_idx = bar_idx;
> -                uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO;
>                  uart->irq = pci_conf_read8(0, b, d, f, PCI_INTERRUPT_PIN) ?
>                      pci_conf_read8(0, b, d, f, PCI_INTERRUPT_LINE) : 0;
>  
> +

Spurious whitespace change.  Please remove.

~Andrew

>                  return 0;
>              }
>          }
> @@ -746,6 +817,9 @@ void __init ns16550_init(int index, struct ns16550_defaults *defaults)
>      /* Default is no transmit FIFO. */
>      uart->fifo_size = 1;
>  
> +    /* Default lsr_mask = UART_LSR_THRE */
> +    uart->lsr_mask = UART_LSR_THRE;
> +
>      ns16550_parse_port_config(uart, (index == 0) ? opt_com1 : opt_com2);
>  }
>