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