From mboxrd@z Thu Jan 1 00:00:00 1970 From: Parth Dixit Subject: Re: [PATCH v2 20/41] arm : create generic uart initialization function Date: Sun, 24 May 2015 12:37:50 +0530 Message-ID: References: <1431893048-5214-1-git-send-email-parth.dixit@linaro.org> <1431893048-5214-21-git-send-email-parth.dixit@linaro.org> <555DC154.5020300@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5566928553719492412==" Return-path: In-Reply-To: <555DC154.5020300@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: keir@xen.org, Ian Campbell , andrew.cooper3@citrix.com, tim@xen.org, xen-devel , Stefano Stabellini , Jan Beulich , Christoffer Dall List-Id: xen-devel@lists.xenproject.org --===============5566928553719492412== Content-Type: multipart/alternative; boundary=001a1145789c27169a0516ce8da7 --001a1145789c27169a0516ce8da7 Content-Type: text/plain; charset=UTF-8 On 21 May 2015 at 16:58, Julien Grall 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 > > --- > > 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 > --001a1145789c27169a0516ce8da7 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


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 functi= on.
> 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:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 - Introduction of the generic UART
=C2=A0 =C2=A0 =C2=A0 =C2=A0 - Refactoring of PL011

Each changes should be in a separate patch for helping the review.

ok, will move into seperate p= atches.
> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
> ---
>=C2=A0 xen/arch/arm/setup.c=C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 =C2=A02 += -
>=C2=A0 xen/drivers/char/Makefile=C2=A0 |=C2=A0 =C2=A02 +-
>=C2=A0 xen/drivers/char/dt-uart.c | 107 -------------------------------= --------------
>=C2=A0 xen/drivers/char/pl011.c=C2=A0 =C2=A0|=C2=A0 47 ++++++++++++----= ----
>=C2=A0 xen/include/xen/serial.h=C2=A0 =C2=A0|=C2=A0 =C2=A03 +-
>=C2=A0 5 files changed, 33 insertions(+), 128 deletions(-)
>=C2=A0 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_offs= et,
>
>=C2=A0 =C2=A0 =C2=A0 gic_preinit();
>
> -=C2=A0 =C2=A0 dt_uart_init();
> +=C2=A0 =C2=A0 uart_init();

As said by Jan, this is arm specific. I would rename into arm_uart_i= nit.

>=C2=A0 =C2=A0 =C2=A0 console_init_preirq();
>=C2=A0 =C2=A0 =C2=A0 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) +=3D exynos4210-uart.o
>=C2=A0 obj-$(HAS_OMAP) +=3D omap-uart.o
>=C2=A0 obj-$(HAS_SCIF) +=3D scif-uart.o
>=C2=A0 obj-$(HAS_EHCI) +=3D ehci-dbgp.o
> -obj-$(CONFIG_ARM) +=3D dt-uart.o
> +obj-$(CONFIG_ARM) +=3D arm-uart.o
>=C2=A0 obj-y +=3D 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_dri= ver =3D {
>=C2=A0 =C2=A0 =C2=A0 .stop_tx=C2=A0 =C2=A0 =C2=A0 =3D pl011_tx_stop, >=C2=A0 =C2=A0 =C2=A0 .vuart_info=C2=A0 =C2=A0=3D pl011_vuart,
>=C2=A0 };
> +static int __init pl011_uart_init(struct pl011 *uart, u64 addr, u64 s= ize)
> +{
> +=C2=A0 =C2=A0 uart->clock_hz=C2=A0 =3D 0x16e3600;
> +=C2=A0 =C2=A0 uart->baud=C2=A0 =C2=A0 =C2=A0 =3D BAUD_AUTO;
> +=C2=A0 =C2=A0 uart->data_bits =3D 8;
> +=C2=A0 =C2=A0 uart->parity=C2=A0 =C2=A0 =3D PARITY_NONE;
> +=C2=A0 =C2=A0 uart->stop_bits =3D 1;
> +
> +=C2=A0 =C2=A0 uart->regs =3D ioremap_nocache(addr, size);
> +=C2=A0 =C2=A0 if ( !uart->regs )
> +=C2=A0 =C2=A0 {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 printk("pl011: Unable to map the UAR= T memory\n");
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return -ENOMEM;
> +=C2=A0 =C2=A0 }
> +
> +=C2=A0 =C2=A0 uart->vuart.base_addr =3D addr;
> +=C2=A0 =C2=A0 uart->vuart.size =3D size;
> +=C2=A0 =C2=A0 uart->vuart.data_off =3D DR;
> +=C2=A0 =C2=A0 uart->vuart.status_off =3D FR;
> +=C2=A0 =C2=A0 uart->vuart.status =3D 0;
> +
> +=C2=A0 =C2=A0 return 0;
> +}
>
>=C2=A0 /* 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,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 const void *data) >=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 const char *config =3D data;
> @@ -242,12 +265,6 @@ static int __init pl011_uart_init(struct dt_devic= e_node *dev,
>
>=C2=A0 =C2=A0 =C2=A0 uart =3D &pl011_com;
>
> -=C2=A0 =C2=A0 uart->clock_hz=C2=A0 =3D 0x16e3600;
> -=C2=A0 =C2=A0 uart->baud=C2=A0 =C2=A0 =C2=A0 =3D BAUD_AUTO;
> -=C2=A0 =C2=A0 uart->data_bits =3D 8;
> -=C2=A0 =C2=A0 uart->parity=C2=A0 =C2=A0 =3D PARITY_NONE;
> -=C2=A0 =C2=A0 uart->stop_bits =3D 1;
> -
>=C2=A0 =C2=A0 =C2=A0 res =3D dt_device_get_address(dev, 0, &addr, &= amp;size);
>=C2=A0 =C2=A0 =C2=A0 if ( res )
>=C2=A0 =C2=A0 =C2=A0 {
> @@ -264,19 +281,13 @@ static int __init pl011_uart_init(struct dt_devi= ce_node *dev,
>=C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 =C2=A0 uart->irq =3D res;

IRQ can be passed as parameters of pl011_uart_init.

>
> -=C2=A0 =C2=A0 uart->regs =3D ioremap_nocache(addr, size);
> -=C2=A0 =C2=A0 if ( !uart->regs )
> +=C2=A0 =C2=A0 res =3D pl011_uart_init(uart, addr, size);
> +=C2=A0 =C2=A0 if ( res < 0 )
>=C2=A0 =C2=A0 =C2=A0 {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 printk("pl011: Unable to map the UAR= T memory\n");
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 return -ENOMEM;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 printk("pl011: Unable to initialize\= n");
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return res;
>=C2=A0 =C2=A0 =C2=A0 }
>
> -=C2=A0 =C2=A0 uart->vuart.base_addr =3D addr;
> -=C2=A0 =C2=A0 uart->vuart.size =3D size;
> -=C2=A0 =C2=A0 uart->vuart.data_off =3D DR;
> -=C2=A0 =C2=A0 uart->vuart.status_off =3D FR;
> -=C2=A0 =C2=A0 uart->vuart.status =3D 0;
> -
>=C2=A0 =C2=A0 =C2=A0 /* Register with generic serial driver. */
>=C2=A0 =C2=A0 =C2=A0 serial_register_uart(SERHND_DTUART, &pl011_dri= ver, uart);

This could be moved in pl011_uart_init. With the other changes sugge= sted
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 =3D
>
>=C2=A0 DT_DEVICE_START(pl011, "PL011 UART", DEVICE_SERIAL) >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 .dt_match =3D pl011_dt_match,
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 .init =3D pl011_uart_init,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 .init =3D dt_pl011_uart_init,
>=C2=A0 DT_DEVICE_END
>
>=C2=A0 /*
> 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 {
>=C2=A0 #define SERHND_HI=C2=A0 =C2=A0 =C2=A0 =C2=A0(1<<2) /* Mux/= demux each transferred char by MSB. */
>=C2=A0 #define SERHND_LO=C2=A0 =C2=A0 =C2=A0 =C2=A0(1<<3) /* Ditt= o, except that the MSB is cleared.=C2=A0 */
>=C2=A0 #define SERHND_COOKED=C2=A0 =C2=A0(1<<4) /* Newline/carria= ge-return translation?=C2=A0 =C2=A0 */
> +#define SERHND_UART=C2=A0 =C2=A0 =C2=A0(0<<0) /* handler config= ured from ACPI */

Why did you introduce a new SERHND rather than renaming SERHND_DTUAR= T?

Regards,

--
Julien Grall

--001a1145789c27169a0516ce8da7-- --===============5566928553719492412== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============5566928553719492412==--