Re: [PATCH 4/4] xen/arm: enable ns16550 uart driver for OMAP5432

[Julien Grall <julien.grall@citrix.com>]

On 07/19/2013 01:57 PM, Chen Baozi wrote:
> 
> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>  xen/arch/arm/Rules.mk      |   1 +
>  xen/drivers/char/ns16550.c | 155 +++++++++++++++++++++++++++++++++++++++------
>  xen/include/asm-arm/io.h   |  49 ++++++++++++++
>  xen/include/xen/serial.h   |   7 +-
>  4 files changed, 191 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> index eaa03fb..8604e34 100644
> --- a/xen/arch/arm/Rules.mk
> +++ b/xen/arch/arm/Rules.mk
> @@ -9,6 +9,7 @@
>  HAS_DEVICE_TREE := y
>  HAS_VIDEO := y
>  HAS_ARM_HDLCD := y
> +HAS_NS16550 := y
>  
>  CFLAGS += -fno-builtin -fno-common -Wredundant-decls
>  CFLAGS += -iwithprefix include -Werror -Wno-pointer-arith -pipe
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 60512be..b8690eb 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1,10 +1,10 @@
>  /******************************************************************************
>   * ns16550.c
> - * 
> + *

Spurious change.
>   * Driver for 16550-series UARTs. This driver is to be kept within Xen as
>   * it permits debugging of seriously-toasted machines (e.g., in situations
>   * where a device driver within a guest OS would be inaccessible).
> - * 
> + *
Spurious change.

>   * Copyright (c) 2003-2005, K A Fraser
>   */
>  
> @@ -25,6 +25,13 @@
>  #include <asm/fixmap.h>
>  #endif
>  
> +#ifdef CONFIG_ARM
> +#include <xen/device_tree.h>
> +#include <xen/errno.h>

The above 2 includes can be moved outside the ifdef.

> +#include <asm/early_printk.h>
> +#include <asm/device.h>
> +#endif
> +
>  /*
>   * Configure serial port with a string:
>   *   <baud>[/<clock_hz>][,DPS[,<io-base>[,<irq>[,<port-bdf>[,<bridge-bdf>]]]]].
> @@ -39,8 +46,11 @@ string_param("com1", opt_com1);
>  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. */
> +    int baud, data_bits, parity, stop_bits, fifo_size;
> +    u32 clock_hz;
> +    struct dt_irq irq;
> +    u64 io_base;   /* I/O port or memory-mapped I/O address. */
> +    u64 io_size;
>      char __iomem *remapped_io_base;  /* Remapped virtual address of MMIO. */
>      /* UART with IRQ line: interrupt-driven I/O. */
>      struct irqaction irqaction;
> @@ -61,16 +71,24 @@ static struct ns16550 {
>  
>  static char ns_read_reg(struct ns16550 *uart, int reg)
>  {
> +#ifdef CONFIG_X86
>      if ( uart->remapped_io_base == NULL )
>          return inb(uart->io_base + reg);
>      return readb(uart->remapped_io_base + reg);
> +#else
> +    return readb(uart->remapped_io_base + (reg << 2));
> +#endif
>  }
>  

Is it possible to add a new private field "shift"? So the both readb can
be merged.

>  static void ns_write_reg(struct ns16550 *uart, int reg, char c)
>  {
> +#ifdef CONFIG_X86
>      if ( uart->remapped_io_base == NULL )
>          return outb(c, uart->io_base + reg);
>      writeb(c, uart->remapped_io_base + reg);
> +#else
> +    writeb(c, uart->remapped_io_base + (reg << 2));
> +#endif

Same here for readb.

>  }
>  
>  static void ns16550_interrupt(
> @@ -145,11 +163,12 @@ static int ns16550_getc(struct serial_port *port, char *pc)
>      return 1;
>  }
>  
> +#ifdef CONFIG_X86
>  static void pci_serial_early_init(struct ns16550 *uart)
>  {
>      if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 )
>          return;
> -    
> +
Spurious change.

>      if ( uart->pb_bdf_enable )
>          pci_conf_write16(0, uart->pb_bdf[0], uart->pb_bdf[1], uart->pb_bdf[2],
>                           PCI_IO_BASE,
> @@ -161,7 +180,13 @@ static void pci_serial_early_init(struct ns16550 *uart)
>                       uart->io_base | PCI_BASE_ADDRESS_SPACE_IO);
>      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)
>  {
> @@ -217,7 +242,9 @@ 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 = ioremap_attr(uart->io_base,
> +                                              uart->io_size,
> +                                              PAGE_HYPERVISOR_NOCACHE);
>  #endif
>      }
>  
> @@ -231,7 +258,7 @@ static void __init ns16550_init_preirq(struct serial_port *port)
>  
>  static void ns16550_setup_postirq(struct ns16550 *uart)
>  {
> -    if ( uart->irq > 0 )
> +    if ( uart->irq.irq > 0 )
>      {
>          /* Master interrupt enable; also keep DTR/RTS asserted. */
>          ns_write_reg(uart,
> @@ -241,7 +268,7 @@ static void ns16550_setup_postirq(struct ns16550 *uart)
>          ns_write_reg(uart, UART_IER, UART_IER_ERDAI | UART_IER_ELSI);
>      }
>  
> -    if ( uart->irq >= 0 )
> +    if ( uart->irq.irq >= 0 )
>          set_timer(&uart->timer, NOW() + MILLISECS(uart->timeout_ms));
>  }
>  
> @@ -250,7 +277,7 @@ static void __init ns16550_init_postirq(struct serial_port *port)
>      struct ns16550 *uart = port->uart;
>      int rc, bits;
>  
> -    if ( uart->irq < 0 )
> +    if ( uart->irq.irq < 0 )
>          return;
>  
>      serial_async_transmit(port);
> @@ -262,20 +289,26 @@ static void __init ns16550_init_postirq(struct serial_port *port)
>      uart->timeout_ms = max_t(
>          unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
>  
> -    if ( uart->irq > 0 )
> +    if ( uart->irq.irq > 0 )
>      {
>          uart->irqaction.handler = ns16550_interrupt;
>          uart->irqaction.name    = "ns16550";
>          uart->irqaction.dev_id  = port;
> -        if ( (rc = setup_irq(uart->irq, &uart->irqaction)) != 0 )
> -            printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq);
> +#ifdef CONFIG_X86
> +        if ( (rc = setup_irq(uart->irq.irq, &uart->irqaction)) != 0 )
> +#else
> +        if ( (rc = setup_dt_irq(&uart->irq, &uart->irqaction)) != 0 )
> +#endif
> +            printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq.irq);
>      }
>  
>      ns16550_setup_postirq(uart);
>  
> +#ifdef CONFIG_X86
>      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)
> @@ -284,13 +317,16 @@ static void ns16550_suspend(struct serial_port *port)
>  
>      stop_timer(&uart->timer);
>  
> +#ifdef CONFIG_X86
>      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 CONFIG_X86
>      struct ns16550 *uart = port->uart;
>  
>      if ( uart->bar )
> @@ -300,6 +336,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);
> @@ -370,9 +407,17 @@ static void __init ns16550_endboot(struct serial_port *port)
>  static int __init ns16550_irq(struct serial_port *port)
>  {
>      struct ns16550 *uart = port->uart;
> -    return ((uart->irq > 0) ? uart->irq : -1);
> +    return ((uart->irq.irq > 0) ? uart->irq.irq : -1);
>  }
>  
> +#ifdef CONFIG_ARM
> +static const struct dt_irq __init *ns16550_dt_irq(struct serial_port *port)
> +{
> +    struct ns16550 *uart = port->uart;
> +    return &uart->irq;
> +}
> +#endif

You don't need to surround this function by ifdef.

>  static struct uart_driver __read_mostly ns16550_driver = {
>      .init_preirq  = ns16550_init_preirq,
>      .init_postirq = ns16550_init_postirq,
> @@ -382,7 +427,10 @@ 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 CONFIG_ARM
> +    .dt_irq_get   = ns16550_dt_irq
> +#endif
>  };

same here.

>  static int __init parse_parity_char(int c)
> @@ -403,6 +451,7 @@ static int __init parse_parity_char(int c)
>      return 0;
>  }
>  
> +#ifdef CONFIG_X86
>  static void __init parse_pci_bdf(const char **conf, unsigned int bdf[3])
>  {
>      bdf[0] = simple_strtoul(*conf, conf, 16);
> @@ -415,6 +464,7 @@ static void __init parse_pci_bdf(const char **conf, unsigned int bdf[3])
>      (*conf)++;
>      bdf[2] = simple_strtoul(*conf, conf, 16);
>  }
> +#endif
>  
>  static int __init check_existence(struct ns16550 *uart)
>  {
> @@ -428,7 +478,7 @@ static int __init check_existence(struct ns16550 *uart)
>          return 1;
>  
>      pci_serial_early_init(uart);
> -    
> +

spurious change.

>      /*
>       * Do a simple existence test first; if we fail this,
>       * there's no point trying anything else.
> @@ -445,7 +495,7 @@ static int __init check_existence(struct ns16550 *uart)
>      scratch3 = ns_read_reg(uart, UART_IER) & 0x0f;
>      ns_write_reg(uart, UART_IER, scratch);
>      if ( (scratch2 != 0) || (scratch3 != 0x0F) )
> -        return 0;
> +            return 0;

spurious change.

>  
>      /*
>       * Check to see if a UART is really there.
> @@ -456,6 +506,7 @@ static int __init check_existence(struct ns16550 *uart)
>      return (status == 0x90);
>  }
>  
> +#ifdef CONFIG_X86
>  static int
>  pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
>  {
> @@ -507,7 +558,7 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
>                  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) ?
> +                uart->irq.irq = pci_conf_read8(0, b, d, f, PCI_INTERRUPT_PIN) ?
>                      pci_conf_read8(0, b, d, f, PCI_INTERRUPT_LINE) : 0;
>  
>                  return 0;
> @@ -519,11 +570,12 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
>          return -1;
>  
>      uart->io_base = 0x3f8;
> -    uart->irq = 0;
> +    uart->irq.irq = 0;
>      uart->clock_hz  = UART_CLOCK_HZ;
>  
>      return 0;
>  }
> +#endif
>  
>  #define PARSE_ERR(_f, _a...)                 \
>      do {                                     \
> @@ -534,6 +586,7 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
>  static void __init ns16550_parse_port_config(
>      struct ns16550 *uart, const char *conf)
>  {
> +#ifdef CONFIG_X86
>      int baud;
>  
>      /* No user-specified configuration? */
> @@ -589,7 +642,7 @@ static void __init ns16550_parse_port_config(
>      }
>  
>      if ( *conf == ',' && *++conf != ',' )
> -        uart->irq = simple_strtol(conf, &conf, 10);
> +        uart->irq.irq = simple_strtol(conf, &conf, 10);
>  
>      if ( *conf == ',' && *++conf != ',' )
>      {
> @@ -604,6 +657,8 @@ static void __init ns16550_parse_port_config(
>      }
>  
>   config_parsed:
> +#endif
> +

You have disabled nearly 80% of the code for ARM. Perhaps you can split
this function in 2:
	- Parsing pci/user configuration
        - Register the UART

>      /* Sanity checks. */
>      if ( (uart->baud != BAUD_AUTO) &&
>           ((uart->baud < 1200) || (uart->baud > 115200)) )
> @@ -633,18 +688,80 @@ void __init ns16550_init(int index, struct ns16550_defaults *defaults)
>      uart->baud      = (defaults->baud ? :
>                         console_has((index == 0) ? "com1" : "com2")
>                         ? BAUD_AUTO : 0);
> +#ifdef CONFIG_ARM
> +    uart->clock_hz = defaults->clock_hz;
> +#else
>      uart->clock_hz  = UART_CLOCK_HZ;
> +#endif
>      uart->data_bits = defaults->data_bits;
>      uart->parity    = parse_parity_char(defaults->parity);
>      uart->stop_bits = defaults->stop_bits;
>      uart->irq       = defaults->irq;
>      uart->io_base   = defaults->io_base;
> +#ifdef CONFIG_ARM
> +    uart->io_size   = defaults->io_size;
> +#endif
> +
>      /* Default is no transmit FIFO. */
>      uart->fifo_size = 1;
>  
>      ns16550_parse_port_config(uart, (index == 0) ? opt_com1 : opt_com2);
> +
>  }
>  
> +#ifdef CONFIG_ARM
> +
> +static int __init ns16550_dt_init(struct dt_device_node *dev,
> +                                  const void *data)
> +{
> +    struct ns16550_defaults uart;
> +    int res;
> +    const __be32 *clkspec;
> +
> +    uart.baud      = 115200;
> +    uart.data_bits = 8;
> +    uart.parity    = 'n';
> +    uart.stop_bits = 1;
> +
> +    res = dt_device_get_address(dev, 0, &uart.io_base, &uart.io_size);
> +    if (res) {
> +        early_printk("ns16550: Unable to retrieve the base"
> +                     " address of the UART\n");
> +        return res;
> +    }
> +
> +    res = dt_device_get_irq(dev, 0, &uart.irq);
> +    if (res) {
> +        early_printk("ns16550: Unable to retrieve the IRQ\n");
> +        return res;
> +    }
> +
> +    clkspec = dt_get_property(dev, "clock-frequency", NULL);
> +    if (clkspec == NULL) {
> +        early_printk("ns16550: Unable to retrieve the clock frequency\n");
> +        return -EINVAL;
> +    }
> +    uart.clock_hz = be32_to_cpup(clkspec);
> +
> +    ns16550_init(0, &uart);
> +    dt_device_set_used_by(dev, DOMID_XEN);
> +
> +    return 0;
> +}
> +
> +static const char const *ns16550_dt_compat[] __initdata =
> +{
> +    "ti,omap4-uart",
> +    NULL
> +};
> +
> +DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL)
> +    .compatible = ns16550_dt_compat,
> +    .init = ns16550_dt_init,
> +DT_DEVICE_END
> +
> +#endif /* CONFIG_ARM */
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/asm-arm/io.h b/xen/include/asm-arm/io.h
> index aea5233..33674bc 100644
> --- a/xen/include/asm-arm/io.h
> +++ b/xen/include/asm-arm/io.h
> @@ -1,6 +1,55 @@
>  #ifndef _ASM_IO_H
>  #define _ASM_IO_H
>  
> +static inline void __raw_writeb(u8 val, volatile void __iomem *addr)
> +{
> +        asm volatile("strb %1, %0"
> +                     : "+Qo" (*(volatile u8 __force *)addr)
> +                     : "r" (val));
> +}
> +
> +static inline void __raw_writel(u32 val, volatile void __iomem *addr)
> +{
> +        asm volatile("str %1, %0"
> +                     : "+Qo" (*(volatile u32 __force *)addr)
> +                     : "r" (val));
> +}
> +
> +static inline u8 __raw_readb(const volatile void __iomem *addr)
> +{
> +        u8 val;
> +        asm volatile("ldrb %1, %0"
> +                     : "+Qo" (*(volatile u8 __force *)addr),
> +                       "=r" (val));
> +        return val;
> +}
> +
> +static inline u32 __raw_readl(const volatile void __iomem *addr)
> +{
> +        u32 val;
> +        asm volatile("ldr %1, %0"
> +                     : "+Qo" (*(volatile u32 __force *)addr),
> +                       "=r" (val));
> +        return val;
> +}
> +
> +#define __iormb()               rmb()
> +#define __iowmb()               wmb()
> +
> +#define readb_relaxed(c) ({ u8  __r = __raw_readb(c); __r; })
> +#define readl_relaxed(c) ({ u32 __r = le32_to_cpu((__force __le32) \
> +                                        __raw_readl(c)); __r; })
> +
> +#define writeb_relaxed(v,c)     __raw_writeb(v,c)
> +#define writel_relaxed(v,c)     __raw_writel((__force u32) cpu_to_le32(v),c)
> +
> +#define readb(c)                ({ u8  __v = readb_relaxed(c); __iormb(); __v; })
> +#define readl(c)                ({ u32 __v = readl_relaxed(c); __iormb(); __v; })
> +
> +#define writeb(v,c)             ({ __iowmb(); writeb_relaxed(v,c); })
> +#define writel(v,c)             ({ __iowmb(); writel_relaxed(v,c); })
> +
> +
>  #endif
>  /*
>   * Local variables:

Theses changes are not in the right place. The asm-arm/io.h must only
contain common code between arm32 and arm64. Your code is arm32
specific, so it should be moved in asm-arm/arm32/io.h.

There is already an implementation for (io)readl and (io)writel with
another name. I will be happy if you rename/alias these 2 functions, it
will avoid ifdef... in ns16550 code.

> diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
> index 9caf776..09ca855 100644
> --- a/xen/include/xen/serial.h
> +++ b/xen/include/xen/serial.h
> @@ -11,6 +11,7 @@
>  
>  #include <xen/init.h>
>  #include <xen/spinlock.h>
> +#include <xen/device_tree.h>

>  
>  struct cpu_user_regs;
>  
> @@ -151,8 +152,10 @@ struct ns16550_defaults {
>      int data_bits; /* default data bits (5, 6, 7 or 8) */
>      int parity;    /* default parity (n, o, e, m or s) */
>      int stop_bits; /* default stop bits (1 or 2) */
> -    int irq;       /* default irq */
> -    unsigned long io_base; /* default io_base address */
> +    struct dt_irq irq;     /* default irq */

This change will break build on x86. This structure is filled by
arch/x86/setup.c

> +    u64 io_base;   /* default io_base address */
> +    u64 io_size;
> +    u32 clock_hz;  /* UART clock rate */
>  };

As I understand, you use these modifications only in ns16550.c to fill
it with device tree value. Considering the number of ifdef you have
added in ns16550_init, I think it's better to clone the function and
directly fill ns16550, instead of ns16550_defaults. Anyone got any other
thoughts?

>  void ns16550_init(int index, struct ns16550_defaults *defaults);
>  void ehci_dbgp_init(void);
> 

Cheers,

-- 
Julien