[U-Boot] [PATCH v3 1/3] arm:samsung:serial Extract common UART code

Lukasz Majewski l.majewski at majess.pl
Thu Sep 12 21:43:45 CEST 2013


Hi Minkyu,

> Hi Minkyu
> 
> > Dear Lukasz,
> > 
> > On 13/08/13 06:15, Lukasz Majewski wrote:
> > > This commit brings removal of duplicated code for UART IP block
> > > embedded at Samsung SoCs.
> > > New include/asm/samsung-common directory has been created to store
> > > common code for existing and future Samsung targets.
> > > 
> > > Moreover building of UART code now depends on more verbose
> > > CONFIG_S5P_SERIAL. Thereof all relevant boards configs have been
> > > adjusted.
> > > 
> > > Signed-off-by: Lukasz Majewski <l.majewski at majess.pl>
> > > 
> > > ---
> > > Changes for v3:
> > > - Comply with SPDX license format
> > > 
> > > Changes for v2:
> > > - Remove S3C64XX define from the code
> > > ---
> > >  arch/arm/include/asm/arch-exynos/uart.h    |   44
> > > ------------------- arch/arm/include/asm/arch-s5pc1xx/uart.h   |
> > > 44 ------------------- arch/arm/include/asm/samsung-common/uart.h
> > > |   64 ++++++++++++++++++++++++++++
> > > drivers/serial/Makefile                    |    2 +-
> > > drivers/serial/serial_s5p.c                |   13 +-----
> > > include/configs/exynos5250-dt.h            |    1 +
> > > include/configs/origen.h                   |    1 +
> > > include/configs/s5p_goni.h                 |    1 +
> > > include/configs/s5pc210_universal.h        |    1 +
> > > include/configs/smdkc100.h                 |    1 +
> > > include/configs/smdkv310.h                 |    1 +
> > > include/configs/trats.h                    |    1 + 12 files
> > > changed, 74 insertions(+), 100 deletions(-) delete mode 100644
> > > arch/arm/include/asm/arch-exynos/uart.h delete mode 100644
> > > arch/arm/include/asm/arch-s5pc1xx/uart.h create mode 100644
> > > arch/arm/include/asm/samsung-common/uart.h
> > > 
> > > diff --git a/arch/arm/include/asm/arch-exynos/uart.h
> > > b/arch/arm/include/asm/arch-exynos/uart.h deleted file mode 100644
> > > index 33d6ba3..0000000
> > > --- a/arch/arm/include/asm/arch-exynos/uart.h
> > > +++ /dev/null
> > > @@ -1,44 +0,0 @@
> > > -/*
> > > - * (C) Copyright 2009 Samsung Electronics
> > > - * Minkyu Kang <mk7.kang at samsung.com>
> > > - * Heungjun Kim <riverful.kim at samsung.com>
> > > - *
> > > - * SPDX-License-Identifier:	GPL-2.0+
> > > - */
> > > -
> > > -#ifndef __ASM_ARCH_UART_H_
> > > -#define __ASM_ARCH_UART_H_
> > > -
> > > -#ifndef __ASSEMBLY__
> > > -/* baudrate rest value */
> > > -union br_rest {
> > > -	unsigned short	slot;		/* udivslot */
> > > -	unsigned char	value;		/* ufracval */
> > > -};
> > > -
> > > -struct s5p_uart {
> > > -	unsigned int	ulcon;
> > > -	unsigned int	ucon;
> > > -	unsigned int	ufcon;
> > > -	unsigned int	umcon;
> > > -	unsigned int	utrstat;
> > > -	unsigned int	uerstat;
> > > -	unsigned int	ufstat;
> > > -	unsigned int	umstat;
> > > -	unsigned char	utxh;
> > > -	unsigned char	res1[3];
> > > -	unsigned char	urxh;
> > > -	unsigned char	res2[3];
> > > -	unsigned int	ubrdiv;
> > > -	union br_rest	rest;
> > > -	unsigned char	res3[0xffd0];
> > > -};
> > > -
> > > -static inline int s5p_uart_divslot(void)
> > > -{
> > > -	return 0;
> > > -}
> > > -
> > > -#endif	/* __ASSEMBLY__ */
> > > -
> > > -#endif
> > > diff --git a/arch/arm/include/asm/arch-s5pc1xx/uart.h
> > > b/arch/arm/include/asm/arch-s5pc1xx/uart.h deleted file mode
> > > 100644 index 26db098..0000000
> > > --- a/arch/arm/include/asm/arch-s5pc1xx/uart.h
> > > +++ /dev/null
> > > @@ -1,44 +0,0 @@
> > > -/*
> > > - * (C) Copyright 2009 Samsung Electronics
> > > - * Minkyu Kang <mk7.kang at samsung.com>
> > > - * Heungjun Kim <riverful.kim at samsung.com>
> > > - *
> > > - * SPDX-License-Identifier:	GPL-2.0+
> > > - */
> > > -
> > > -#ifndef __ASM_ARCH_UART_H_
> > > -#define __ASM_ARCH_UART_H_
> > > -
> > > -#ifndef __ASSEMBLY__
> > > -/* baudrate rest value */
> > > -union br_rest {
> > > -	unsigned short	slot;		/* udivslot */
> > > -	unsigned char	value;		/* ufracval */
> > > -};
> > > -
> > > -struct s5p_uart {
> > > -	unsigned int	ulcon;
> > > -	unsigned int	ucon;
> > > -	unsigned int	ufcon;
> > > -	unsigned int	umcon;
> > > -	unsigned int	utrstat;
> > > -	unsigned int	uerstat;
> > > -	unsigned int	ufstat;
> > > -	unsigned int	umstat;
> > > -	unsigned char	utxh;
> > > -	unsigned char	res1[3];
> > > -	unsigned char	urxh;
> > > -	unsigned char	res2[3];
> > > -	unsigned int	ubrdiv;
> > > -	union br_rest	rest;
> > > -	unsigned char	res3[0x3d0];
> > > -};
> > > -
> > > -static inline int s5p_uart_divslot(void)
> > > -{
> > > -	return 1;
> > > -}
> > > -
> > > -#endif	/* __ASSEMBLY__ */
> > > -
> > > -#endif
> > > diff --git a/arch/arm/include/asm/samsung-common/uart.h
> > > b/arch/arm/include/asm/samsung-common/uart.h new file mode 100644
> > > index 0000000..ce92399
> > > --- /dev/null
> > > +++ b/arch/arm/include/asm/samsung-common/uart.h
> > > @@ -0,0 +1,64 @@
> > > +/*
> > > + * (C) Copyright 2009 Samsung Electronics
> > > + * Minkyu Kang <mk7.kang at samsung.com>
> > > + * Heungjun Kim <riverful.kim at samsung.com>
> > > + *
> > > + * SPDX-License-Identifier:	GPL-2.0+
> > > + */
> > > +
> > > +#ifndef __ASM_ARCH_UART_H_
> > > +#define __ASM_ARCH_UART_H_
> > > +
> > > +#ifndef __ASSEMBLY__
> > > +/* baudrate rest value */
> > > +union br_rest {
> > > +	unsigned short	slot;		/* udivslot */
> > > +	unsigned char	value;		/* ufracval */
> > > +};
> > > +
> > > +struct s5p_uart {
> > > +	unsigned int	ulcon;
> > > +	unsigned int	ucon;
> > > +	unsigned int	ufcon;
> > > +	unsigned int	umcon;
> > > +	unsigned int	utrstat;
> > > +	unsigned int	uerstat;
> > > +	unsigned int	ufstat;
> > > +	unsigned int	umstat;
> > > +	unsigned char	utxh;
> > > +	unsigned char	res1[3];
> > > +	unsigned char	urxh;
> > > +	unsigned char	res2[3];
> > > +	unsigned int	ubrdiv;
> > > +	union br_rest	rest;
> > > +#if defined(CONFIG_S5PC100) || defined(CONFIG_S5PC110)
>      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^[1]
> 
> > 
> > OK. I understood your patch's concept and patch looks good.
> > But, we have not been allowed ifdef at our SoCs.
> > So, I can't accept your patch easily.
> 
> The problem is that several Samsung SoCs reuse the same IP blocks. 
> 
> For example S3C6410 (which I want to add to u-boot), S5PC100, S5PC110,
> Exynos4 and Exynos5 are using the same timer, pwm, sromc, uart, udc,
> sdhci, and many others IP blocks.
> 
> As I've proposed in this patch series - several IP blocks *.c files
> shall go into samsung-common directory at arch/arm/cpu/samsung-common.
> This arm soc version independent code can be reused at s3c6410
> (arm1176) CPU.
> 
> Another issue - headers. The register map for relevant IP blocks is
> the same at s3c6410 and exynos5250. The only difference is the offset
> at the end of the structure.
> 
> > 
> > How you think?
> 
> We can remove [1] if we define the struct s5p_uart without the
> "unsigned char res3[xxx];"in the end.
> 
> It is correct, since we are using the "IP block offset defines" anyway
> to index correct IP block instance (like uart0/1/2...).
> 
> > Do we really need to combine headers?
> > I agree that they are almost duplicate codes.
> > But I thinks it's not a redundant.
> > 
> 
> Another issue is the artificial distinction between s5pc1xx and exynos
> families of the processors.
> 
> Both are armv7, both are cortex (A8 and A9).I think that
> arch/arm/cpu/armv7/s5pc1xx code shall be moved to
> arch/arm/cpu/armv7/exynos
> 
> Also arch/arm/include/asm/arch-{s5pc1xx|exynos} code can be put to
> samsung-common directory (as also proposed in those patch series).
> 
> Frankly, we shall mimic the tegra or imx tree. As fair as I've noticed
> at those SoCs the common code has been put to various *-common
> directories.
> 
> I've added other Samsung boards maintainers to Cc.

Have you thought about this?

> 
> > > +	unsigned char	res3[0x3d0];
> > > +#else /* Exynos 4 and 5 */
> > > +	unsigned char	res3[0xffd0];
> > > +#endif
> > > +};
> > > +
> > > +
> > > +static inline int s5p_uart_divslot(void)
> > > +{
> > > +#if defined(CONFIG_S5PC100) || defined(CONFIG_S5PC110)
> > > +	return 1;
> > > +#else /* Exynos 4 and 5 */
> > > +	return 0;
> > > +#endif
> > > +}
> > > +
> > > +#define RX_FIFO_COUNT_MASK      0xff
> > > +#define RX_FIFO_FULL_MASK       (1 << 8)
> > > +#define TX_FIFO_FULL_MASK       (1 << 24)
> > > +
> > > +/* Information about a serial port */
> > > +struct fdt_serial {
> > > +	u32 base_addr;  /* address of registers in physical
> > > memory */
> > > +	u8 port_id;     /* uart port number */
> > > +	u8 enabled;     /* 1 if enabled, 0 if disabled */
> > > +};
> > > +
> > > +#endif	/* __ASSEMBLY__ */
> > > +
> > > +#endif
> > > diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> > > index 697f2bb..e3ca49a 100644
> > > --- a/drivers/serial/Makefile
> > > +++ b/drivers/serial/Makefile
> > > @@ -19,7 +19,7 @@ COBJS-$(CONFIG_LPC32XX_HSUART) +=
> > > lpc32xx_hsuart.o COBJS-$(CONFIG_MCFUART) += mcfuart.o
> > >  COBJS-$(CONFIG_OPENCORES_YANU) += opencores_yanu.o
> > >  COBJS-$(CONFIG_SYS_NS16550) += ns16550.o
> > > -COBJS-$(CONFIG_S5P) += serial_s5p.o
> > > +COBJS-$(CONFIG_S5P_SERIAL) += serial_s5p.o
> > >  COBJS-$(CONFIG_SYS_NS16550_SERIAL) += serial_ns16550.o
> > >  COBJS-$(CONFIG_IMX_SERIAL) += serial_imx.o
> > >  COBJS-$(CONFIG_IXP_SERIAL) += serial_ixp.o
> > > diff --git a/drivers/serial/serial_s5p.c
> > > b/drivers/serial/serial_s5p.c index f98b422..be08925 100644
> > > --- a/drivers/serial/serial_s5p.c
> > > +++ b/drivers/serial/serial_s5p.c
> > > @@ -12,22 +12,13 @@
> > >  #include <fdtdec.h>
> > >  #include <linux/compiler.h>
> > >  #include <asm/io.h>
> > > -#include <asm/arch/uart.h>
> > > +#include <asm/samsung-common/uart.h>
> > >  #include <asm/arch/clk.h>
> > >  #include <serial.h>
> > >  
> > >  DECLARE_GLOBAL_DATA_PTR;
> > >  
> > > -#define RX_FIFO_COUNT_MASK	0xff
> > > -#define RX_FIFO_FULL_MASK	(1 << 8)
> > > -#define TX_FIFO_FULL_MASK	(1 << 24)
> > > -
> > > -/* Information about a serial port */
> > > -struct fdt_serial {
> > > -	u32 base_addr;  /* address of registers in physical
> > > memory */
> > > -	u8 port_id;     /* uart port number */
> > > -	u8 enabled;     /* 1 if enabled, 0 if disabled */
> > > -} config __attribute__ ((section(".data")));
> > > +struct fdt_serial config __attribute__ ((section(".data")));
> > >  
> > >  static inline struct s5p_uart *s5p_get_base_uart(int dev_index)
> > >  {
> > > diff --git a/include/configs/exynos5250-dt.h
> > > b/include/configs/exynos5250-dt.h index 8f8f85f..a759d07 100644
> > > --- a/include/configs/exynos5250-dt.h
> > > +++ b/include/configs/exynos5250-dt.h
> > > @@ -69,6 +69,7 @@
> > >  #define CONFIG_SYS_MALLOC_LEN		(CONFIG_ENV_SIZE +
> > > (4 << 20)) 
> > >  /* select serial console configuration */
> > > +#define CONFIG_S5P_SERIAL
> > >  #define CONFIG_BAUDRATE			115200
> > >  #define EXYNOS5_DEFAULT_UART_OFFSET	0x010000
> > >  #define CONFIG_SILENT_CONSOLE
> > > diff --git a/include/configs/origen.h b/include/configs/origen.h
> > > index da13574..a59419d 100644
> > > --- a/include/configs/origen.h
> > > +++ b/include/configs/origen.h
> > > @@ -48,6 +48,7 @@
> > >  #define CONFIG_SYS_MALLOC_LEN		(CONFIG_ENV_SIZE +
> > > (1 << 20)) 
> > >  /* select serial console configuration */
> > > +#define CONFIG_S5P_SERIAL
> > >  #define CONFIG_SERIAL2			1	/* use
> > > SERIAL 2 */ #define CONFIG_BAUDRATE			115200
> > >  #define EXYNOS4_DEFAULT_UART_OFFSET	0x020000
> > > diff --git a/include/configs/s5p_goni.h
> > > b/include/configs/s5p_goni.h index d0fafd7..812b7f3 100644
> > > --- a/include/configs/s5p_goni.h
> > > +++ b/include/configs/s5p_goni.h
> > > @@ -42,6 +42,7 @@
> > >  /*
> > >   * select serial console configuration
> > >   */
> > > +#define CONFIG_S5P_SERIAL
> > >  #define CONFIG_SERIAL2			1	/* use
> > > SERIAL2 */ #define CONFIG_BAUDRATE			115200
> > >  
> > > diff --git a/include/configs/s5pc210_universal.h
> > > b/include/configs/s5pc210_universal.h index 97a4008..2270449
> > > 100644 --- a/include/configs/s5pc210_universal.h
> > > +++ b/include/configs/s5pc210_universal.h
> > > @@ -48,6 +48,7 @@
> > >  #define CONFIG_SYS_MALLOC_LEN		(CONFIG_ENV_SIZE +
> > > (1 << 20)) 
> > >  /* select serial console configuration */
> > > +#define CONFIG_S5P_SERIAL
> > >  #define CONFIG_SERIAL2		1	/* use SERIAL 2 */
> > >  #define CONFIG_BAUDRATE		115200
> > >  
> > > diff --git a/include/configs/smdkc100.h
> > > b/include/configs/smdkc100.h index a572e62..4631dac 100644
> > > --- a/include/configs/smdkc100.h
> > > +++ b/include/configs/smdkc100.h
> > > @@ -47,6 +47,7 @@
> > >  /*
> > >   * select serial console configuration
> > >   */
> > > +#define CONFIG_S5P_SERIAL
> > >  #define CONFIG_SERIAL0			1	/* use
> > > SERIAL 0 on SMDKC100 */ 
> > >  /* PWM */
> > > diff --git a/include/configs/smdkv310.h
> > > b/include/configs/smdkv310.h index 0496661..9e10bf1 100644
> > > --- a/include/configs/smdkv310.h
> > > +++ b/include/configs/smdkv310.h
> > > @@ -48,6 +48,7 @@
> > >  #define CONFIG_SYS_MALLOC_LEN		(CONFIG_ENV_SIZE +
> > > (1 << 20)) 
> > >  /* select serial console configuration */
> > > +#define CONFIG_S5P_SERIAL
> > >  #define CONFIG_SERIAL1			1	/* use
> > > SERIAL 1 */ #define CONFIG_BAUDRATE			115200
> > >  #define EXYNOS4_DEFAULT_UART_OFFSET	0x010000
> > > diff --git a/include/configs/trats.h b/include/configs/trats.h
> > > index 9b6aac9..6b301c8 100644
> > > --- a/include/configs/trats.h
> > > +++ b/include/configs/trats.h
> > > @@ -53,6 +53,7 @@
> > >  #define CONFIG_SYS_MALLOC_LEN		(CONFIG_ENV_SIZE +
> > > (16 << 20)) 
> > >  /* select serial console configuration */
> > > +#define CONFIG_S5P_SERIAL
> > >  #define CONFIG_SERIAL2			/* use SERIAL 2 */
> > >  #define CONFIG_BAUDRATE			115200
> > >  
> > > 
> > 
> > Thanks,
> > Minkyu Kang.
> 
> Regards,
> 
> Lukasz Majewski

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130912/6d4631cd/attachment.pgp>


More information about the U-Boot mailing list