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.majew...@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.k...@samsung.com>
> >>> - * Heungjun Kim <riverful....@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.k...@samsung.com>
> >>> - * Heungjun Kim <riverful....@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.k...@samsung.com>
> >>> + * Heungjun Kim <riverful....@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.
--
Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
