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

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

On 21/11/13 22:50, 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 V3:
>   - per Jan's comments:
>     - Use unsigned int for uart_config struct and for local variables 
>       in pci_uart_config function where applicable.
>     - use predefined MASKS for length calculation.
>     - Fix styling issues
>   - Misc:
>     - The device has 64 bit bar. Rework code to provide support for
>       64 bit bar, length values.
>     - Hide part of mmio region from dom0.
>
> 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 | 167 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 146 insertions(+), 21 deletions(-)
>
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 9c2cded..79b3a5c 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,6 +70,7 @@ 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;
>  #endif
> @@ -77,6 +79,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;
> +    unsigned int reg_shift;
> +    unsigned int reg_width; 
> +    unsigned int fifo_size;
> +    u8 lsr_mask;
> +    unsigned int max_bars;
> +};
> +
> +/*
> + * Create lookup tables for specific MMIO devices..
> + * It is assumed that if the device found is MMIO,
> + * then you have indexed it here. Else, the driver
> + * does nothing.
> + */
> +static struct ns16550_config_mmio __initdata uart_config[] =
> +{
> +    /* Broadcom TruManage device */
> +    { 
> +        .vendor_id = 0x14e4,
> +        .dev_id = 0x160a,
> +        .reg_shift = 2,
> +        .reg_width = 1,
> +        .fifo_size = 16,
> +        .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
> +        .max_bars = 1,
> +    },
> +};
> +
>  static void ns16550_delayed_resume(void *data);
>  
>  static char ns_read_reg(struct ns16550 *uart, int reg)
> @@ -134,7 +167,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 +193,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 +216,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)
> @@ -381,6 +416,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);
>      }
> @@ -432,9 +474,16 @@ static void __init ns16550_endboot(struct serial_port *port)
>  {
>  #ifdef HAS_IOPORTS
>      struct ns16550 *uart = port->uart;
> -
> +    unsigned long sfn, efn;
> +    
>      if ( uart->remapped_io_base )
> +    {
> +        sfn = paddr_to_pfn((unsigned long) uart->io_base + PAGE_SIZE);
> +        efn = paddr_to_pfn((unsigned long) uart->io_base + uart->io_size - 1);
> +        if ( iomem_deny_access(dom0, sfn, efn) != 0 )
> +            BUG();

BUG_ON(!iomem_deny_access(dom0, sfn, efn))

is slightly more compact, and has the advantage of showing the action
which failed in the BUG message.

~Andrew

>          return;
> +    }
>      if ( ioports_deny_access(dom0, uart->io_base, uart->io_base + 7) != 0 )
>          BUG();
>  #endif
> @@ -546,11 +595,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, 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++ )
> @@ -578,25 +629,96 @@ 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 */
> +                uart->bar = bar;
> +                
> +                /* 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);
> +                        uart->bar64 = bar_64;
> +                        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;
> +                        size &= ~(size - 1);
> +                        uart->io_size = size;
> +                    }
> +                    else
> +                    {
> +                        len &= PCI_BASE_ADDRESS_MEM_MASK;
> +                        uart->io_size = len & ~(len - 1);
> +                    }
> +
> +                    /* Force length of mmio region to be at least 8 bytes */
> +                    if ( uart->io_size < 0x8 )
> +                        continue;
> +
> +                    /* 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 )
> +                            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)uart->bar64 << 32) |
> +                                        (uart->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 + 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);
> +                    len &= PCI_BASE_ADDRESS_IO_MASK;
> +                    len &= ~(len - 1);
> +
> +                    /* Not 8 bytes */
> +                    if ( len != 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->irq = pci_conf_read8(0, b, d, f, PCI_INTERRUPT_PIN) ?
>                      pci_conf_read8(0, b, d, f, PCI_INTERRUPT_LINE) : 0;
>  
> @@ -746,6 +868,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);
>  }
>