| Message ID | 651d1e6d364d53a3f09c77f086a0a36c15e068c3.1367979526.git.julien.grall@linaro.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Wed, 2013-05-08 at 03:33 +0100, Julien Grall wrote: > @@ -227,32 +239,56 @@ static struct uart_driver __read_mostly pl011_driver = { > .tx_ready = pl011_tx_ready, > .putc = pl011_putc, > .getc = pl011_getc, > - .irq = pl011_irq > + .irq = pl011_irq, > + .dt_irq_get = pl011_dt_irq, > }; > > -/* TODO: Parse UART config from device-tree or command-line */ > - > -void __init pl011_init(int index, unsigned long register_base_address) > +/* TODO: Parse UART config from the command line */ > +static int __init pl011_uart_init(struct dt_device_node *dev, > + const void *data) > { > + const struct serial_arm_defaults *defaults = data; > struct pl011 *uart; > + int res; > > - if ( (index < 0) || (index > 1) ) > - return; > + if ( (defaults->index < 0) || (defaults->index > 1) ) > + return -EINVAL; > > - uart = &pl011_com[index]; > + uart = &pl011_com[defaults->index]; > > uart->clock_hz = 0x16e3600; > uart->baud = 38400; > uart->data_bits = 8; > uart->parity = PARITY_NONE; > uart->stop_bits = 1; > - uart->irq = 37; /* TODO Need to find this from devicetree */ > - uart->regs = (uint32_t *) register_base_address; > + uart->regs = (uint32_t *) defaults->register_base_address; Should this not come from struct dt_device_node? Perhaps the driver needs to be instantiating its own FIXMAP mapping instead of doing it in common code? In fact can we not get rid of the fixmap for the runtime (not early) console and use the ioremap stuff which Stefano just enabled with his vmap patches, or the early_ioremap stuff if necessary. That does then call into question the whole serial_arm_defaults thing. Perhaps index should just be the order in which they are registered (and at the moment it is effectively hardcoded to 0 anyway) > + > + res = dt_device_get_irq(dev, 0, &uart->irq); > + if ( res ) > + { > + early_printk("pl011: Unable to retrieve the IRQ\n"); > + return res; > + } > > /* Register with generic serial driver. */ > serial_register_uart(uart - pl011_com, &pl011_driver, uart); > + > + dt_device_set_used_by(dev, DT_USED_BY_XEN); > + > + return 0; > } Ian.
On 05/08/2013 04:17 PM, Ian Campbell wrote: > On Wed, 2013-05-08 at 03:33 +0100, Julien Grall wrote: >> @@ -227,32 +239,56 @@ static struct uart_driver __read_mostly pl011_driver = { >> .tx_ready = pl011_tx_ready, >> .putc = pl011_putc, >> .getc = pl011_getc, >> - .irq = pl011_irq >> + .irq = pl011_irq, >> + .dt_irq_get = pl011_dt_irq, >> }; >> >> -/* TODO: Parse UART config from device-tree or command-line */ >> - >> -void __init pl011_init(int index, unsigned long register_base_address) >> +/* TODO: Parse UART config from the command line */ >> +static int __init pl011_uart_init(struct dt_device_node *dev, >> + const void *data) >> { >> + const struct serial_arm_defaults *defaults = data; >> struct pl011 *uart; >> + int res; >> >> - if ( (index < 0) || (index > 1) ) >> - return; >> + if ( (defaults->index < 0) || (defaults->index > 1) ) >> + return -EINVAL; >> >> - uart = &pl011_com[index]; >> + uart = &pl011_com[defaults->index]; >> >> uart->clock_hz = 0x16e3600; >> uart->baud = 38400; >> uart->data_bits = 8; >> uart->parity = PARITY_NONE; >> uart->stop_bits = 1; >> - uart->irq = 37; /* TODO Need to find this from devicetree */ >> - uart->regs = (uint32_t *) register_base_address; >> + uart->regs = (uint32_t *) defaults->register_base_address; > > Should this not come from struct dt_device_node? > > Perhaps the driver needs to be instantiating its own FIXMAP mapping > instead of doing it in common code? In fact can we not get rid of the > fixmap for the runtime (not early) console and use the ioremap stuff > which Stefano just enabled with his vmap patches, or the early_ioremap > stuff if necessary. Right. I will use ioremap in the next patch series. > That does then call into question the whole serial_arm_defaults thing. > Perhaps index should just be the order in which they are registered (and > at the moment it is effectively hardcoded to 0 anyway) I'm thinking about using serial_arm_defaults for baud rate,... For the moment the different values are hardcoded. {arm,dt}_init_uart will parse the dtuart parameters on the command line and then fill serial_arm_defaults.
On Wed, 2013-05-08 at 17:23 +0100, Julien Grall wrote: > > That does then call into question the whole serial_arm_defaults thing. > > Perhaps index should just be the order in which they are registered (and > > at the moment it is effectively hardcoded to 0 anyway) > > > I'm thinking about using serial_arm_defaults for baud rate,... For the > moment the different values are hardcoded. > > {arm,dt}_init_uart will parse the dtuart parameters on the command line > and then fill serial_arm_defaults. Perhaps instead of &serial_arm_defaults you should pass a char * pointer to opt_dtuart? This would allow for per-device settings and is consistent with the ns16550 driver. Ian.
On 05/08/2013 05:43 PM, Ian Campbell wrote: > On Wed, 2013-05-08 at 17:23 +0100, Julien Grall wrote: >>> That does then call into question the whole serial_arm_defaults thing. >>> Perhaps index should just be the order in which they are registered (and >>> at the moment it is effectively hardcoded to 0 anyway) >> >> >> I'm thinking about using serial_arm_defaults for baud rate,... For the >> moment the different values are hardcoded. >> >> {arm,dt}_init_uart will parse the dtuart parameters on the command line >> and then fill serial_arm_defaults. > > Perhaps instead of &serial_arm_defaults you should pass a char * pointer > to opt_dtuart? This would allow for per-device settings and is > consistent with the ns16550 driver. Ok. I will modify it in the next patch series.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index d719229..f7b9889 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -501,10 +501,6 @@ void gic_route_spis(void) int seridx; const struct dt_irq *irq; - /* XXX should get these from DT */ - /* UART */ - gic_route_irq(37, 0, 1u << smp_processor_id(), 0xa0); - for ( seridx = 0; seridx <= SERHND_IDX; seridx++ ) { if ( (irq = serial_dt_irq(seridx)) == NULL ) diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 7b2df8b..d8a3578 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -432,10 +432,6 @@ void __init start_xen(unsigned long boot_phys_offset, dt_unflatten_host_device_tree(); dt_irq_xlate = gic_irq_xlate; -#ifdef EARLY_UART_ADDRESS - /* TODO Need to get device tree or command line for UART address */ - pl011_init(0, FIXMAP_ADDR(FIXMAP_CONSOLE)); -#endif arm_uart_init(); console_init_preirq(); diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c index 8efd08e..03161c4 100644 --- a/xen/drivers/char/pl011.c +++ b/xen/drivers/char/pl011.c @@ -22,9 +22,14 @@ #include <xen/serial.h> #include <xen/init.h> #include <xen/irq.h> +#include <asm/early_printk.h> +#include <xen/device_tree.h> +#include <xen/errno.h> +#include <asm/device.h> static struct pl011 { - unsigned int baud, clock_hz, data_bits, parity, stop_bits, irq; + unsigned int baud, clock_hz, data_bits, parity, stop_bits; + struct dt_irq irq; volatile uint32_t *regs; /* UART with IRQ line: interrupt-driven I/O. */ struct irqaction irqaction; @@ -163,13 +168,13 @@ static void __init pl011_init_postirq(struct serial_port *port) struct pl011 *uart = port->uart; int rc; - if ( uart->irq > 0 ) + if ( uart->irq.irq > 0 ) { uart->irqaction.handler = pl011_interrupt; uart->irqaction.name = "pl011"; uart->irqaction.dev_id = port; - if ( (rc = setup_irq(uart->irq, &uart->irqaction)) != 0 ) - printk("ERROR: Failed to allocate pl011 IRQ %d\n", uart->irq); + if ( (rc = setup_dt_irq(&uart->irq, &uart->irqaction)) != 0 ) + printk("ERROR: Failed to allocate pl011 IRQ %d\n", uart->irq.irq); } /* Clear pending error interrupts */ @@ -215,7 +220,14 @@ static int pl011_getc(struct serial_port *port, char *pc) static int __init pl011_irq(struct serial_port *port) { struct pl011 *uart = port->uart; - return ((uart->irq > 0) ? uart->irq : -1); + return ((uart->irq.irq > 0) ? uart->irq.irq : -1); +} + +static const struct dt_irq __init *pl011_dt_irq(struct serial_port *port) +{ + struct pl011 *uart = port->uart; + + return &uart->irq; } static struct uart_driver __read_mostly pl011_driver = { @@ -227,32 +239,56 @@ static struct uart_driver __read_mostly pl011_driver = { .tx_ready = pl011_tx_ready, .putc = pl011_putc, .getc = pl011_getc, - .irq = pl011_irq + .irq = pl011_irq, + .dt_irq_get = pl011_dt_irq, }; -/* TODO: Parse UART config from device-tree or command-line */ - -void __init pl011_init(int index, unsigned long register_base_address) +/* TODO: Parse UART config from the command line */ +static int __init pl011_uart_init(struct dt_device_node *dev, + const void *data) { + const struct serial_arm_defaults *defaults = data; struct pl011 *uart; + int res; - if ( (index < 0) || (index > 1) ) - return; + if ( (defaults->index < 0) || (defaults->index > 1) ) + return -EINVAL; - uart = &pl011_com[index]; + uart = &pl011_com[defaults->index]; uart->clock_hz = 0x16e3600; uart->baud = 38400; uart->data_bits = 8; uart->parity = PARITY_NONE; uart->stop_bits = 1; - uart->irq = 37; /* TODO Need to find this from devicetree */ - uart->regs = (uint32_t *) register_base_address; + uart->regs = (uint32_t *) defaults->register_base_address; + + res = dt_device_get_irq(dev, 0, &uart->irq); + if ( res ) + { + early_printk("pl011: Unable to retrieve the IRQ\n"); + return res; + } /* Register with generic serial driver. */ serial_register_uart(uart - pl011_com, &pl011_driver, uart); + + dt_device_set_used_by(dev, DT_USED_BY_XEN); + + return 0; } +static const char const *pl011_dt_compat[] __initdata = +{ + "arm,pl011", + NULL +}; + +DT_DEVICE_START(pl011, "PL011 UART", DEVICE_SERIAL) + .compatible = pl011_dt_compat, + .init = pl011_uart_init, +DT_DEVICE_END + /* * Local variables: * mode: C diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h index f548f8b..16ed261 100644 --- a/xen/include/xen/serial.h +++ b/xen/include/xen/serial.h @@ -156,7 +156,6 @@ struct ns16550_defaults { void ns16550_init(int index, struct ns16550_defaults *defaults); void ehci_dbgp_init(void); -void pl011_init(int index, unsigned long register_base_address); /* Default value for UART on ARM boards */ struct serial_arm_defaults { int index; /* Serial index */
Allow UART driver to retrieve all its information in the device tree. It's possible to choose the pl011 driver via the Xen command line. Signed-off-by: Julien Grall <julien.grall@linaro.org> Changes in v2: - Rework TODO - Use the new function setup_dt_irq --- xen/arch/arm/gic.c | 4 --- xen/arch/arm/setup.c | 4 --- xen/drivers/char/pl011.c | 64 ++++++++++++++++++++++++++++++++++++---------- xen/include/xen/serial.h | 1 - 4 files changed, 50 insertions(+), 23 deletions(-)