|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 4/4] xen/arm: enable ns16550 uart driver for OMAP5432
On 07/19/2013 01:57 PM, Chen Baozi wrote:
>
> Signed-off-by: Chen Baozi <baozich@xxxxxxxxx>
> ---
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |