|
[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 Jul 20, 2013, at 1:06 AM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 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,
>
Hi Julien
Thanks for reviewing. I'll rework my patchset according to your suggestions
and resend it later.
Cheers,
Baozi
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |