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

Minkyu Kang mk7.kang at samsung.com
Tue Sep 24 10:42:11 CEST 2013


Dear Lukasz,

On 29/08/13 06:01, Lukasz Majewski wrote:
> 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's OK.
but, how can you remove other ifdefs?

> 
> 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

hm.. then, some files (cpu, clock, gpio,,, ) will be complicated.

> 
> 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.
> 
>>> +	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
> 

Thanks,
Minkyu Kang.


More information about the U-Boot mailing list