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.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 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
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
