| Message ID | 1376342108-319-2-git-send-email-l.majewski@majess.pl |
|---|---|
| State | Rejected |
| Delegated to: | Minkyu Kang |
| Headers | show |
Hi Minkyu, By mistake I've forgotten to add you to CC for the v3 of this patch series. However you were on CC for last two versions of those patches. I'd be very grateful for feedback :-) > 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@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@samsung.com> > - * Heungjun Kim <riverful.kim@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@samsung.com> > - * Heungjun Kim <riverful.kim@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@samsung.com> > + * Heungjun Kim <riverful.kim@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) > + 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 >
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@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@samsung.com> > - * Heungjun Kim <riverful.kim@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@samsung.com> > - * Heungjun Kim <riverful.kim@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@samsung.com> > + * Heungjun Kim <riverful.kim@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) 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. How you think? Do we really need to combine headers? I agree that they are almost duplicate codes. But I thinks it's not a redundant. > + 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.
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@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@samsung.com> > > - * Heungjun Kim <riverful.kim@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@samsung.com> > > - * Heungjun Kim <riverful.kim@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@samsung.com> > > + * Heungjun Kim <riverful.kim@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. > > + 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
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@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@samsung.com> > > > - * Heungjun Kim <riverful.kim@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@samsung.com> > > > - * Heungjun Kim <riverful.kim@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@samsung.com> > > > + * Heungjun Kim <riverful.kim@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
On 29 August 2013 02:31, Lukasz Majewski <l.majewski@majess.pl> 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@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@samsung.com> >> > - * Heungjun Kim <riverful.kim@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@samsung.com> >> > - * Heungjun Kim <riverful.kim@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@samsung.com> >> > + * Heungjun Kim <riverful.kim@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. first of all sorry for late response. I think it is a good thought. By removing the res[3] padding in the structure we can do away with ifdef's. As uart_offset is used to hop next uart port. But keep in mind that there are another two reserved fields(res2, res3). If there is any any change in those reserve field values in future soc's then this approach will not work. > > 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. > >> > + 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) This can be done away using cpu_is_xxX. >> > + 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 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
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@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@samsung.com> >>> - * Heungjun Kim <riverful.kim@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@samsung.com> >>> - * Heungjun Kim <riverful.kim@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@samsung.com> >>> + * Heungjun Kim <riverful.kim@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.
Hi Minkyu, > 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@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@samsung.com> > >>> - * Heungjun Kim <riverful.kim@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@samsung.com> > >>> - * Heungjun Kim <riverful.kim@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@samsung.com> > >>> + * Heungjun Kim <riverful.kim@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? Other #ifdef here is the s5p_uart_divslot function: +static inline int s5p_uart_divslot(void) +{ +#if defined(CONFIG_S5PC100) || defined(CONFIG_S5PC110) + return 1; +#else /* Exynos 4 and 5 */ + return 0; +#endif +} It looks like a good candidate for clean up. > > > > > 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. The timer handling code is directly reused at s3c6410. Also please look into PATCH 2/3 and PATCH 3/3 in this series. For example, PATCH 3/3 unifies cpu info. Now at arch/arm/cpu/armv7/s5pc1xx directory one finds: cache.S, clock.c and reset.S Moreover, if we plan to describe Samsung boards in a device tree - in a similar way to e.g. Tegra, common code base would be more than welcome. > > > > > 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.
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@samsung.com> - * Heungjun Kim <riverful.kim@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@samsung.com> - * Heungjun Kim <riverful.kim@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@samsung.com> + * Heungjun Kim <riverful.kim@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) + 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
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@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