| Message ID | 1431893048-5214-21-git-send-email-parth.dixit@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 21 May 2015 at 16:58, Julien Grall <julien.grall@citrix.com> wrote: > Hi Parth, > > On 17/05/15 21:03, Parth Dixit wrote: > > Rename dt-uart.c to arm-uart.c and create new generic uart init function. > > move dt_uart_init to uart_init.Refactor pl011 driver to dt and common > > initialization parts. This will be useful later when acpi specific > > uart initialization function is introduced. > > There is 2 distinct changes in this patch: > - Introduction of the generic UART > - Refactoring of PL011 > > Each changes should be in a separate patch for helping the review. > > ok, will move into seperate patches. > > Signed-off-by: Parth Dixit <parth.dixit@linaro.org> > > --- > > xen/arch/arm/setup.c | 2 +- > > xen/drivers/char/Makefile | 2 +- > > xen/drivers/char/dt-uart.c | 107 > --------------------------------------------- > > xen/drivers/char/pl011.c | 47 ++++++++++++-------- > > xen/include/xen/serial.h | 3 +- > > 5 files changed, 33 insertions(+), 128 deletions(-) > > delete mode 100644 xen/drivers/char/dt-uart.c > > > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > > index 5711077..1b2d74c 100644 > > --- a/xen/arch/arm/setup.c > > +++ b/xen/arch/arm/setup.c > > @@ -771,7 +771,7 @@ void __init start_xen(unsigned long boot_phys_offset, > > > > gic_preinit(); > > > > - dt_uart_init(); > > + uart_init(); > > As said by Jan, this is arm specific. I would rename into arm_uart_init. > > > console_init_preirq(); > > console_init_ring(); > > > > diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile > > index 47fc3f9..a8f65c1 100644 > > --- a/xen/drivers/char/Makefile > > +++ b/xen/drivers/char/Makefile > > @@ -6,5 +6,5 @@ obj-$(HAS_EXYNOS4210) += exynos4210-uart.o > > obj-$(HAS_OMAP) += omap-uart.o > > obj-$(HAS_SCIF) += scif-uart.o > > obj-$(HAS_EHCI) += ehci-dbgp.o > > -obj-$(CONFIG_ARM) += dt-uart.o > > +obj-$(CONFIG_ARM) += arm-uart.o > > obj-y += serial.o > > > diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c > > index 67e6df5..f0c3daf 100644 > > --- a/xen/drivers/char/pl011.c > > +++ b/xen/drivers/char/pl011.c > > @@ -225,9 +225,32 @@ static struct uart_driver __read_mostly > pl011_driver = { > > .stop_tx = pl011_tx_stop, > > .vuart_info = pl011_vuart, > > }; > > +static int __init pl011_uart_init(struct pl011 *uart, u64 addr, u64 > size) > > +{ > > + uart->clock_hz = 0x16e3600; > > + uart->baud = BAUD_AUTO; > > + uart->data_bits = 8; > > + uart->parity = PARITY_NONE; > > + uart->stop_bits = 1; > > + > > + uart->regs = ioremap_nocache(addr, size); > > + if ( !uart->regs ) > > + { > > + printk("pl011: Unable to map the UART memory\n"); > > + return -ENOMEM; > > + } > > + > > + uart->vuart.base_addr = addr; > > + uart->vuart.size = size; > > + uart->vuart.data_off = DR; > > + uart->vuart.status_off = FR; > > + uart->vuart.status = 0; > > + > > + return 0; > > +} > > > > /* TODO: Parse UART config from the command line */ > > -static int __init pl011_uart_init(struct dt_device_node *dev, > > +static int __init dt_pl011_uart_init(struct dt_device_node *dev, > > const void *data) > > { > > const char *config = data; > > @@ -242,12 +265,6 @@ static int __init pl011_uart_init(struct > dt_device_node *dev, > > > > uart = &pl011_com; > > > > - uart->clock_hz = 0x16e3600; > > - uart->baud = BAUD_AUTO; > > - uart->data_bits = 8; > > - uart->parity = PARITY_NONE; > > - uart->stop_bits = 1; > > - > > res = dt_device_get_address(dev, 0, &addr, &size); > > if ( res ) > > { > > @@ -264,19 +281,13 @@ static int __init pl011_uart_init(struct > dt_device_node *dev, > > } > > uart->irq = res; > > IRQ can be passed as parameters of pl011_uart_init. > > > > > - uart->regs = ioremap_nocache(addr, size); > > - if ( !uart->regs ) > > + res = pl011_uart_init(uart, addr, size); > > + if ( res < 0 ) > > { > > - printk("pl011: Unable to map the UART memory\n"); > > - return -ENOMEM; > > + printk("pl011: Unable to initialize\n"); > > + return res; > > } > > > > - uart->vuart.base_addr = addr; > > - uart->vuart.size = size; > > - uart->vuart.data_off = DR; > > - uart->vuart.status_off = FR; > > - uart->vuart.status = 0; > > - > > /* Register with generic serial driver. */ > > serial_register_uart(SERHND_DTUART, &pl011_driver, uart); > > This could be moved in pl011_uart_init. With the other changes suggested > above, you don't need anymore the variable uart here and the code would > be more compact. > > > > > @@ -293,7 +304,7 @@ static const struct dt_device_match pl011_dt_match[] > __initconst = > > > > DT_DEVICE_START(pl011, "PL011 UART", DEVICE_SERIAL) > > .dt_match = pl011_dt_match, > > - .init = pl011_uart_init, > > + .init = dt_pl011_uart_init, > > DT_DEVICE_END > > > > /* > > diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h > > index 71e6ade..484a6a8 100644 > > --- a/xen/include/xen/serial.h > > +++ b/xen/include/xen/serial.h > > @@ -98,6 +98,7 @@ struct uart_driver { > > #define SERHND_HI (1<<2) /* Mux/demux each transferred char by > MSB. */ > > #define SERHND_LO (1<<3) /* Ditto, except that the MSB is > cleared. */ > > #define SERHND_COOKED (1<<4) /* Newline/carriage-return translation? > */ > > +#define SERHND_UART (0<<0) /* handler configured from ACPI */ > > Why did you introduce a new SERHND rather than renaming SERHND_DTUART? > > Regards, > > -- > Julien Grall >
+shannon On 24 May 2015 at 13:18, Julien Grall <julien.grall@citrix.com> wrote: > > On 24/05/2015 08:07, Parth Dixit wrote: >> >> >> >> On 21 May 2015 at 16:58, Julien Grall <julien.grall@citrix.com >> <mailto:julien.grall@citrix.com>> wrote: >> >> Hi Parth, >> >> On 17/05/15 21:03, Parth Dixit wrote: >> > Rename dt-uart.c to arm-uart.c and create new generic uart init >> function. >> > move dt_uart_init to uart_init.Refactor pl011 driver to dt and >> common >> > initialization parts. This will be useful later when acpi specific >> > uart initialization function is introduced. >> >> There is 2 distinct changes in this patch: >> - Introduction of the generic UART >> - Refactoring of PL011 >> >> Each changes should be in a separate patch for helping the review. >> >> ok, will move into seperate patches. > > > Thank you. It would make sense to gather all the re-factoring patches in the > beginning of the series. So we could push them without waiting the rest of > the series. > > BTW, it seems that your mail client is using tabulation for quoting which is > difficult to read after a couple of back and forth. Can you try to configure > it correctly to use ">"? Maybe it's because of HTML mail? > > Regards, > > -- > Julien Grall
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 5711077..1b2d74c 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -771,7 +771,7 @@ void __init start_xen(unsigned long boot_phys_offset, gic_preinit(); - dt_uart_init(); + uart_init(); console_init_preirq(); console_init_ring(); diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile index 47fc3f9..a8f65c1 100644 --- a/xen/drivers/char/Makefile +++ b/xen/drivers/char/Makefile @@ -6,5 +6,5 @@ obj-$(HAS_EXYNOS4210) += exynos4210-uart.o obj-$(HAS_OMAP) += omap-uart.o obj-$(HAS_SCIF) += scif-uart.o obj-$(HAS_EHCI) += ehci-dbgp.o -obj-$(CONFIG_ARM) += dt-uart.o +obj-$(CONFIG_ARM) += arm-uart.o obj-y += serial.o diff --git a/xen/drivers/char/dt-uart.c b/xen/drivers/char/dt-uart.c deleted file mode 100644 index d599322..0000000 --- a/xen/drivers/char/dt-uart.c +++ /dev/null @@ -1,107 +0,0 @@ -/* - * xen/drivers/char/dt-uart.c - * - * Generic uart retrieved via the device tree - * - * Julien Grall <julien.grall@linaro.org> - * Copyright (c) 2013 Linaro Limited. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; either version 2 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ - -#include <asm/device.h> -#include <asm/types.h> -#include <xen/console.h> -#include <xen/device_tree.h> -#include <xen/serial.h> -#include <xen/errno.h> - -/* - * Configure UART port with a string: - * path:options - * - * @path: full path used in the device tree for the UART. If the path - * doesn't start with '/', we assuming that it's an alias. - * @options: UART speficic options (see in each UART driver) - */ -static char __initdata opt_dtuart[256] = ""; -string_param("dtuart", opt_dtuart); - -void __init dt_uart_init(void) -{ - struct dt_device_node *dev; - int ret; - const char *devpath = opt_dtuart; - char *options; - - if ( !console_has("dtuart") ) - return; /* Not for us */ - - if ( !strcmp(opt_dtuart, "") ) - { - const struct dt_device_node *chosen = dt_find_node_by_path("/chosen"); - - if ( chosen ) - { - const char *stdout; - - ret = dt_property_read_string(chosen, "stdout-path", &stdout); - if ( ret >= 0 ) - { - printk("Taking dtuart configuration from /chosen/stdout-path\n"); - if ( strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart)) - >= sizeof(opt_dtuart) ) - printk("WARNING: /chosen/stdout-path too long, truncated\n"); - } - else if ( ret != -EINVAL /* Not present */ ) - printk("Failed to read /chosen/stdout-path (%d)\n", ret); - } - } - - if ( !strcmp(opt_dtuart, "") ) - { - printk("No dtuart path configured\n"); - return; - } - - options = strchr(opt_dtuart, ':'); - if ( options != NULL ) - *(options++) = '\0'; - else - options = ""; - - printk("Looking for dtuart at \"%s\", options \"%s\"\n", devpath, options); - if ( *devpath == '/' ) - dev = dt_find_node_by_path(devpath); - else - dev = dt_find_node_by_alias(devpath); - - if ( !dev ) - { - printk("Unable to find device \"%s\"\n", devpath); - return; - } - - ret = device_init(dev, DEVICE_SERIAL, options); - - if ( ret ) - printk("Unable to initialize dtuart: %d\n", ret); -} - -/* - * Local variables: - * mode: C - * c-file-style: "BSD" - * c-basic-offset: 4 - * tab-width: 4 - * indent-tabs-mode: nil - * End: - */ diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c index 67e6df5..f0c3daf 100644 --- a/xen/drivers/char/pl011.c +++ b/xen/drivers/char/pl011.c @@ -225,9 +225,32 @@ static struct uart_driver __read_mostly pl011_driver = { .stop_tx = pl011_tx_stop, .vuart_info = pl011_vuart, }; +static int __init pl011_uart_init(struct pl011 *uart, u64 addr, u64 size) +{ + uart->clock_hz = 0x16e3600; + uart->baud = BAUD_AUTO; + uart->data_bits = 8; + uart->parity = PARITY_NONE; + uart->stop_bits = 1; + + uart->regs = ioremap_nocache(addr, size); + if ( !uart->regs ) + { + printk("pl011: Unable to map the UART memory\n"); + return -ENOMEM; + } + + uart->vuart.base_addr = addr; + uart->vuart.size = size; + uart->vuart.data_off = DR; + uart->vuart.status_off = FR; + uart->vuart.status = 0; + + return 0; +} /* TODO: Parse UART config from the command line */ -static int __init pl011_uart_init(struct dt_device_node *dev, +static int __init dt_pl011_uart_init(struct dt_device_node *dev, const void *data) { const char *config = data; @@ -242,12 +265,6 @@ static int __init pl011_uart_init(struct dt_device_node *dev, uart = &pl011_com; - uart->clock_hz = 0x16e3600; - uart->baud = BAUD_AUTO; - uart->data_bits = 8; - uart->parity = PARITY_NONE; - uart->stop_bits = 1; - res = dt_device_get_address(dev, 0, &addr, &size); if ( res ) { @@ -264,19 +281,13 @@ static int __init pl011_uart_init(struct dt_device_node *dev, } uart->irq = res; - uart->regs = ioremap_nocache(addr, size); - if ( !uart->regs ) + res = pl011_uart_init(uart, addr, size); + if ( res < 0 ) { - printk("pl011: Unable to map the UART memory\n"); - return -ENOMEM; + printk("pl011: Unable to initialize\n"); + return res; } - uart->vuart.base_addr = addr; - uart->vuart.size = size; - uart->vuart.data_off = DR; - uart->vuart.status_off = FR; - uart->vuart.status = 0; - /* Register with generic serial driver. */ serial_register_uart(SERHND_DTUART, &pl011_driver, uart); @@ -293,7 +304,7 @@ static const struct dt_device_match pl011_dt_match[] __initconst = DT_DEVICE_START(pl011, "PL011 UART", DEVICE_SERIAL) .dt_match = pl011_dt_match, - .init = pl011_uart_init, + .init = dt_pl011_uart_init, DT_DEVICE_END /* diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h index 71e6ade..484a6a8 100644 --- a/xen/include/xen/serial.h +++ b/xen/include/xen/serial.h @@ -98,6 +98,7 @@ struct uart_driver { #define SERHND_HI (1<<2) /* Mux/demux each transferred char by MSB. */ #define SERHND_LO (1<<3) /* Ditto, except that the MSB is cleared. */ #define SERHND_COOKED (1<<4) /* Newline/carriage-return translation? */ +#define SERHND_UART (0<<0) /* handler configured from ACPI */ /* Two-stage initialisation (before/after IRQ-subsystem initialisation). */ void serial_init_preirq(void); @@ -170,7 +171,7 @@ struct ns16550_defaults { void ns16550_init(int index, struct ns16550_defaults *defaults); void ehci_dbgp_init(void); -void __init dt_uart_init(void); +void __init uart_init(void); struct physdev_dbgp_op; int dbgp_op(const struct physdev_dbgp_op *);
Rename dt-uart.c to arm-uart.c and create new generic uart init function. move dt_uart_init to uart_init. Refactor pl011 driver to dt and common initialization parts. This will be useful later when acpi specific uart initialization function is introduced. Signed-off-by: Parth Dixit <parth.dixit@linaro.org> --- xen/arch/arm/setup.c | 2 +- xen/drivers/char/Makefile | 2 +- xen/drivers/char/dt-uart.c | 107 --------------------------------------------- xen/drivers/char/pl011.c | 47 ++++++++++++-------- xen/include/xen/serial.h | 3 +- 5 files changed, 33 insertions(+), 128 deletions(-) delete mode 100644 xen/drivers/char/dt-uart.c