Re: [PATCH RFC 3/8] ns16550: make usable on ARM

[Keir Fraser <keir.xen@gmail.com>]

On 10/09/2013 07:18, "Ian Campbell" <ian.campbell@citrix.com> wrote:

> There are several aspects to this:
> - Correctly conditionalise use of PCI
> - Correctly conditionalise use of IO ports
> - Add discovery via device tree
> - Support different registers shift/stride and widths
> - Add vuart hooks.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: keir@xen.org
> Cc: jbeulich@suse.com

Bit of an ugly ifdef mess but it is only a serial driver.

Acked-by: Keir Fraser <keir@xen.org>

> ---
>  config/arm32.mk            |    1 +
>  xen/Rules.mk               |    3 +
>  xen/arch/x86/Rules.mk      |    1 +
>  xen/drivers/char/ns16550.c |  183
> +++++++++++++++++++++++++++++++++++++++++---
>  4 files changed, 176 insertions(+), 12 deletions(-)
> 
> diff --git a/config/arm32.mk b/config/arm32.mk
> index 76e229d..aa79d22 100644
> --- a/config/arm32.mk
> +++ b/config/arm32.mk
> @@ -12,6 +12,7 @@ CFLAGS += -marm
>  HAS_PL011 := y
>  HAS_EXYNOS4210 := y
>  HAS_OMAP := y
> +HAS_NS16550 := y
>  
>  # Use only if calling $(LD) directly.
>  LDFLAGS_DIRECT += -EL
> diff --git a/xen/Rules.mk b/xen/Rules.mk
> index 736882a..df1428f 100644
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -60,6 +60,9 @@ CFLAGS-$(lock_profile)  += -DLOCK_PROFILE
>  CFLAGS-$(HAS_ACPI)      += -DHAS_ACPI
>  CFLAGS-$(HAS_GDBSX)     += -DHAS_GDBSX
>  CFLAGS-$(HAS_PASSTHROUGH) += -DHAS_PASSTHROUGH
> +CFLAGS-$(HAS_DEVICE_TREE) += -DHAS_DEVICE_TREE
> +CFLAGS-$(HAS_PCI)       += -DHAS_PCI
> +CFLAGS-$(HAS_IOPORTS)   += -DHAS_IOPORTS
>  CFLAGS-$(frame_pointer) += -fno-omit-frame-pointer -DCONFIG_FRAME_POINTER
>  
>  ifneq ($(max_phys_cpus),)
> diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
> index eb11b5b..c93d2af 100644
> --- a/xen/arch/x86/Rules.mk
> +++ b/xen/arch/x86/Rules.mk
> @@ -1,6 +1,7 @@
>  ########################################
>  # x86-specific definitions
>  
> +HAS_IOPORTS := y
>  HAS_ACPI := y
>  HAS_VGA  := y
>  HAS_VIDEO  := y
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index e0f80f6..854a572 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -13,14 +13,19 @@
>  #include <xen/init.h>
>  #include <xen/irq.h>
>  #include <xen/sched.h>
> -#include <xen/pci.h>
>  #include <xen/timer.h>
>  #include <xen/serial.h>
>  #include <xen/iocap.h>
> +#ifdef HAS_PCI
>  #include <xen/pci.h>
>  #include <xen/pci_regs.h>
> +#endif
>  #include <xen/8250-uart.h>
> +#include <xen/vmap.h>
>  #include <asm/io.h>
> +#ifdef HAS_DEVICE_TREE
> +#include <asm/device.h>
> +#endif
>  #ifdef CONFIG_X86
>  #include <asm/fixmap.h>
>  #endif
> @@ -40,15 +45,22 @@ string_param("com2", opt_com2);
>  
>  static struct ns16550 {
>      int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
> -    unsigned long io_base;   /* I/O port or memory-mapped I/O address. */
> +    u64 io_base;   /* I/O port or memory-mapped I/O address. */
> +    u64 io_size;
> +    int reg_shift; /* Bits to shift register offset by */
> +    int reg_width; /* Number of bytes in each register */
>      char __iomem *remapped_io_base;  /* Remapped virtual address of MMIO. */
>      /* UART with IRQ line: interrupt-driven I/O. */
>      struct irqaction irqaction;
> +#ifdef CONFIG_ARM
> +    struct vuart_info vuart;
> +#endif
>      /* UART with no IRQ line: periodically-polled I/O. */
>      struct timer timer;
>      struct timer resume_timer;
>      unsigned int timeout_ms;
>      bool_t intr_works;
> +#ifdef HAS_PCI
>      /* PCI card parameters. */
>      unsigned int pb_bdf[3]; /* pci bridge BDF */
>      unsigned int ps_bdf[3]; /* pci serial port BDF */
> @@ -57,22 +69,46 @@ static struct ns16550 {
>      u32 bar;
>      u16 cr;
>      u8 bar_idx;
> +#endif
> +#ifdef HAS_DEVICE_TREE
> +    struct dt_irq dt_irq;
> +#endif
>  } ns16550_com[2] = { { 0 } };
>  
>  static void ns16550_delayed_resume(void *data);
>  
>  static char ns_read_reg(struct ns16550 *uart, int reg)
>  {
> +    void __iomem *addr = uart->remapped_io_base + (reg<<uart->reg_shift);
> +#ifdef HAS_IOPORTS
>      if ( uart->remapped_io_base == NULL )
>          return inb(uart->io_base + reg);
> -    return readb(uart->remapped_io_base + reg);
> +#endif
> +    switch (uart->reg_width) {
> +    default: /* assume single byte */
> +    case 1:
> +        return readb(addr);
> +    case 4:
> +        return readl(addr);
> +    }
>  }
>  
>  static void ns_write_reg(struct ns16550 *uart, int reg, char c)
>  {
> +    void __iomem *addr = uart->remapped_io_base + (reg<<uart->reg_shift);
> +#ifdef HAS_IOPORTS
>      if ( uart->remapped_io_base == NULL )
>          return outb(c, uart->io_base + reg);
> -    writeb(c, uart->remapped_io_base + reg);
> +#endif
> +    switch (uart->reg_width) {
> +    default: /* assume single byte */
> +    case 1:
> +        writeb(c, addr);
> +        break;
> +    case 4:
> +        writel(c, addr);
> +        break;
> +    }
>  }
>  
>  static int ns16550_ioport_invalid(struct ns16550 *uart)
> @@ -161,6 +197,7 @@ static int ns16550_getc(struct serial_port *port, char
> *pc)
>      return 1;
>  }
>  
> +#ifdef HAS_PCI
>  static void pci_serial_early_init(struct ns16550 *uart)
>  {
>      if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 )
> @@ -178,6 +215,9 @@ static void pci_serial_early_init(struct ns16550 *uart)
>      pci_conf_write16(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2],
>                       PCI_COMMAND, PCI_COMMAND_IO);
>  }
> +#else
> +static void pci_serial_early_init(struct ns16550 *uart) {}
> +#endif
>  
>  static void ns16550_setup_preirq(struct ns16550 *uart)
>  {
> @@ -223,8 +263,10 @@ static void __init ns16550_init_preirq(struct serial_port
> *port)
>  {
>      struct ns16550 *uart = port->uart;
>  
> +#ifdef HAS_IOPORTS
>      /* I/O ports are distinguished by their size (16 bits). */
>      if ( uart->io_base >= 0x10000 )
> +#endif
>      {
>  #ifdef CONFIG_X86
>          enum fixed_addresses idx = FIX_COM_BEGIN + (uart - ns16550_com);
> @@ -233,7 +275,7 @@ static void __init ns16550_init_preirq(struct serial_port
> *port)
>          uart->remapped_io_base = (void __iomem *)fix_to_virt(idx);
>          uart->remapped_io_base += uart->io_base & ~PAGE_MASK;
>  #else
> -        uart->remapped_io_base = (char *)ioremap(uart->io_base, 8);
> +        uart->remapped_io_base = (char *)ioremap(uart->io_base,
> uart->io_size);
>  #endif
>      }
>  
> @@ -278,21 +320,27 @@ static void __init ns16550_init_postirq(struct
> serial_port *port)
>      bits = uart->data_bits + uart->stop_bits + !!uart->parity;
>      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;
> +#ifdef HAS_DEVICE_TREE
> +        if ( (rc = setup_dt_irq(&uart->dt_irq, &uart->irqaction)) != 0 )
> +            printk("ERROR: Failed to allocate ns16550 DT IRQ.\n");
> +#else
>          if ( (rc = setup_irq(uart->irq, &uart->irqaction)) != 0 )
>              printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq);
> +#endif
>      }
>  
>      ns16550_setup_postirq(uart);
>  
> +#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]));
> +#endif
>  }
>  
>  static void ns16550_suspend(struct serial_port *port)
> @@ -301,13 +349,16 @@ static void ns16550_suspend(struct serial_port *port)
>  
>      stop_timer(&uart->timer);
>  
> +#ifdef HAS_PCI
>      if ( uart->bar )
>         uart->cr = pci_conf_read16(0, uart->ps_bdf[0], uart->ps_bdf[1],
>                                    uart->ps_bdf[2], PCI_COMMAND);
> +#endif
>  }
>  
>  static void _ns16550_resume(struct serial_port *port)
>  {
> +#ifdef HAS_PCI
>      struct ns16550 *uart = port->uart;
>  
>      if ( uart->bar )
> @@ -317,6 +368,7 @@ static void _ns16550_resume(struct serial_port *port)
>         pci_conf_write16(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2],
>                          PCI_COMMAND, uart->cr);
>      }
> +#endif
>  
>      ns16550_setup_preirq(port->uart);
>      ns16550_setup_postirq(port->uart);
> @@ -360,19 +412,17 @@ static void ns16550_resume(struct serial_port *port)
>          _ns16550_resume(port);
>  }
>  
> -#ifdef CONFIG_X86
>  static void __init ns16550_endboot(struct serial_port *port)
>  {
> +#ifdef HAS_IOPORTS
>      struct ns16550 *uart = port->uart;
>  
>      if ( uart->remapped_io_base )
>          return;
>      if ( ioports_deny_access(dom0, uart->io_base, uart->io_base + 7) != 0 )
>          BUG();
> -}
> -#else
> -#define ns16550_endboot NULL
>  #endif
> +}
>  
>  static int __init ns16550_irq(struct serial_port *port)
>  {
> @@ -380,6 +430,23 @@ static int __init ns16550_irq(struct serial_port *port)
>      return ((uart->irq > 0) ? uart->irq : -1);
>  }
>  
> +#ifdef HAS_DEVICE_TREE
> +static const struct dt_irq __init *ns16550_dt_irq(struct serial_port *port)
> +{
> +    struct ns16550 *uart = port->uart;
> +    return &uart->dt_irq;
> +}
> +#endif
> +
> +#ifdef CONFIG_ARM
> +static const struct vuart_info *ns16550_vuart_info(struct serial_port *port)
> +{
> +    struct ns16550 *uart = port->uart;
> +
> +    return &uart->vuart;
> +}
> +#endif
> +
>  static struct uart_driver __read_mostly ns16550_driver = {
>      .init_preirq  = ns16550_init_preirq,
>      .init_postirq = ns16550_init_postirq,
> @@ -389,7 +456,13 @@ static struct uart_driver __read_mostly ns16550_driver =
> {
>      .tx_ready     = ns16550_tx_ready,
>      .putc         = ns16550_putc,
>      .getc         = ns16550_getc,
> -    .irq          = ns16550_irq
> +    .irq          = ns16550_irq,
> +#ifdef HAS_DEVICE_TREE
> +    .dt_irq_get   = ns16550_dt_irq,
> +#endif
> +#ifdef CONFIG_ARM
> +    .vuart_info   = ns16550_vuart_info,
> +#endif
>  };
>  
>  static int __init parse_parity_char(int c)
> @@ -414,15 +487,21 @@ static int __init check_existence(struct ns16550 *uart)
>  {
>      unsigned char status, scratch, scratch2, scratch3;
>  
> +#ifdef HAS_IO_PORTS
>      /*
>       * We can't poke MMIO UARTs until they get I/O remapped later. Assume
> that
>       * if we're getting MMIO UARTs, the arch code knows what it's doing.
>       */
>      if ( uart->io_base >= 0x10000 )
>          return 1;
> +#else
> +    return 1; /* Everything is MMIO */
> +#endif
>  
> +#ifdef HAS_PCI
>      pci_serial_early_init(uart);
> -    
> +#endif
> +
>      /*
>       * Do a simple existence test first; if we fail this,
>       * there's no point trying anything else.
> @@ -450,6 +529,7 @@ static int __init check_existence(struct ns16550 *uart)
>      return (status == 0x90);
>  }
>  
> +#ifdef HAS_PCI
>  static int
>  pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
>  {
> @@ -518,6 +598,7 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int
> bar_idx)
>  
>      return 0;
>  }
> +#endif
>  
>  #define PARSE_ERR(_f, _a...)                 \
>      do {                                     \
> @@ -564,6 +645,7 @@ static void __init ns16550_parse_port_config(
>  
>      if ( *conf == ',' && *++conf != ',' )
>      {
> +#ifdef HAS_PCI
>          if ( strncmp(conf, "pci", 3) == 0 )
>          {
>              if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
> @@ -577,6 +659,7 @@ static void __init ns16550_parse_port_config(
>              conf += 3;
>          }
>          else
> +#endif
>          {
>              uart->io_base = simple_strtoul(conf, &conf, 0);
>          }
> @@ -585,6 +668,7 @@ static void __init ns16550_parse_port_config(
>      if ( *conf == ',' && *++conf != ',' )
>          uart->irq = simple_strtol(conf, &conf, 10);
>  
> +#ifdef HAS_PCI
>      if ( *conf == ',' && *++conf != ',' )
>      {
>          conf = parse_pci(conf, NULL, &uart->ps_bdf[0],
> @@ -601,6 +685,7 @@ static void __init ns16550_parse_port_config(
>              PARSE_ERR("Bad bridge PCI coordinates");
>          uart->pb_bdf_enable = 1;
>      }
> +#endif
>  
>   config_parsed:
>      /* Sanity checks. */
> @@ -638,12 +723,86 @@ void __init ns16550_init(int index, struct
> ns16550_defaults *defaults)
>      uart->stop_bits = defaults->stop_bits;
>      uart->irq       = defaults->irq;
>      uart->io_base   = defaults->io_base;
> +    uart->io_size   = 8;
> +    uart->reg_width = 1;
> +    uart->reg_shift = 0;
> +
>      /* Default is no transmit FIFO. */
>      uart->fifo_size = 1;
>  
>      ns16550_parse_port_config(uart, (index == 0) ? opt_com1 : opt_com2);
>  }
>  
> +#ifdef HAS_DEVICE_TREE
> +static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
> +                                       const void *data)
> +{
> +    struct ns16550 *uart;
> +    int res;
> +    u32 reg_shift, reg_width;
> +
> +    uart = &ns16550_com[0];
> +
> +    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;
> +
> +    res = dt_device_get_address(dev, 0, &uart->io_base, &uart->io_size);
> +    if ( res )
> +        return res;
> +
> +    res = dt_property_read_u32(dev, "reg-shift", &reg_shift);
> +    if ( !res )
> +        uart->reg_shift = 0;
> +    else
> +        uart->reg_shift = reg_shift;
> +
> +    res = dt_property_read_u32(dev, "reg-io-width", &reg_width);
> +    if ( !res )
> +        uart->reg_width = 1;
> +    else
> +        uart->reg_width = reg_width;
> +
> +    if ( uart->reg_width != 1 && uart->reg_width != 4 )
> +        return -EINVAL;
> +
> +    res = dt_device_get_irq(dev, 0, &uart->dt_irq);
> +    if ( res )
> +        return res;
> +
> +    /* The common bit of the driver mostly deals with irq not dt_irq. */
> +    uart->irq = uart->dt_irq.irq;
> +
> +    uart->vuart.base_addr = uart->io_base;
> +    uart->vuart.size = uart->io_size;
> +    uart->vuart.data_off = UART_THR <<uart->reg_shift;
> +    uart->vuart.status_off = UART_LSR<<uart->reg_shift;
> +    uart->vuart.status = UART_LSR_THRE|UART_LSR_TEMT;
> +
> +    /* Register with generic serial driver. */
> +    serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
> +
> +    dt_device_set_used_by(dev, DOMID_XEN);
> +
> +    return 0;
> +}
> +
> +static const char const *ns16550_dt_compat[] __initdata =
> +{
> +    "ns16550",
> +    NULL
> +};
> +
> +DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL)
> +        .compatible = ns16550_dt_compat,
> +        .init = ns16550_uart_dt_init,
> +DT_DEVICE_END
> +
> +#endif /* HAS_DEVICE_TREE */
>  /*
>   * Local variables:
>   * mode: C