[U-Boot] [PATCH 1/4] s5pc1xx: support Samsung s5pc1xx SoC

[Wolfgang Denk]

Dear Minkyu Kang,

In message <4AA0CE3B.7050904 at samsung.com> you wrote:
> This patch add support new SoCs that are s5pc100 and s5pc110
> s5pc1xx SoC is ARM Cortex A8 processor

Please change into:

        This patch adds support for the Samsung s5pc100 and s5pc110
	SoCs. The s5pc1xx SoC is an ARM Cortex A8 processor.

> --- /dev/null
> +++ b/cpu/arm_cortexa8/s5pc1xx/Makefile
...
> +LIB	= $(obj)lib$(SOC).a
> +
> +SOBJS	= reset.o
> +
> +COBJS	+= timer.o
> +COBJS	+= clock.o
> +COBJS	+= cpu_info.o
> +COBJS	+= cache.o

Please always sort such lists alphabetically.

> diff --git a/cpu/arm_cortexa8/s5pc1xx/clock.c b/cpu/arm_cortexa8/s5pc1xx/clock.c
> new file mode 100644
> index 0000000..59690d7
> --- /dev/null
> +++ b/cpu/arm_cortexa8/s5pc1xx/clock.c
...
> +/*
> + * This code should work for both the S3C2400 and the S3C2410
> + * as they seem to have the same PLL and clock machinery inside.
> + * The different address mapping is handled by the s3c24xx.h files below.
> + */

Please check the comment. Is this for s5pc1xx or for s3c24xx?


> +static int s5pc1xx_clock_read_reg(int offset)
> +{
> +	return readl(S5PC1XX_CLOCK_BASE + offset);
> +}

NAK. This needs to be reworked. We do not allow accesses to device
registers like this. Instead of using a base address + offset (which
carries absolutley no information about the type of the accessed
register - 8, 16 or 32 bit?) we require that you describe your
peripherals as C structs, so we can strict type checking by the C
compiler.

> +/* ------------------------------------------------------------------------- */
> +/*
> + * NOTE: This describes the proper use of this file.

Omit thsi comment, please.

> + * CONFIG_SYS_CLK_FREQ should be defined as the input frequency of the PLL.

CONFIG_SYS_CLK_FREQ is nowhere used in the whole file.

> +unsigned long get_pll_clk(int pllreg)
> +{
> +	unsigned long r, m, p, s, mask, fout;
> +	unsigned int freq;
> +
> +	switch (pllreg) {
> +	case APLL:
> +		if (cpu_is_s5pc110())
> +			r = s5pc1xx_clock_read_reg(S5PC110_APLL_CON_OFFSET);
> +		else
> +			r = s5pc1xx_clock_read_reg(S5PC100_APLL_CON_OFFSET);
> +		break;

I'm not sure what exactly you want to acchieve here.

I see two possibilities:

1) You want to creates some "multiboot" capable image, i. e. a single
   U-Boot binary image that is capable of running both on s5pc100 and
   on s5pc110 systems.  Only in this case such a run-time test makes
   sense.  But then it should be written in a more flexible way, so
   that it for example allows to add some (still hypothetical)
   s5pc120 and s5pc130 at a later time. 

2) The resulting U-Boot image will be able to run on only one type of
   SoC; in this case you should make sure that all such decisions can
   be performet at compile time, and no dead code gets added to the
   U-Boot image.


In any case, the code as written is unacceptable, as it extremely
cumbersome to maintain, and is a nightmare to extend for further SoCs.

But I think issue will be solved automatically when you rework your
code to use C structs instead of register offsets, ans you can then
just do a single cpu_is_s5pc???() test and set a pointer to the right
type of data structure.

> +	case HPLL:
> +		if (cpu_is_s5pc110())
> +			hang();

Arghh... It is an extremely bad idea to hang the system without any
indication about what was going on. Please add at least some error
message here.

> +		r = s5pc1xx_clock_read_reg(S5PC100_HPLL_CON_OFFSET);
> +		break;
> +	case VPLL:
> +		if (cpu_is_s5pc100())
> +			hang();

Ditto.

> +	default:
> +		hang();

Ditto.

> +
> +	if (cpu_is_s5pc110()) {
> +		if (pllreg == APLL || pllreg == MPLL)
> +			mask = 0x3ff;
> +		else
> +			mask = 0x1ff;
> +	} else {
> +		if (pllreg == APLL)
> +			mask = 0x3ff;
> +		else
> +			mask = 0x0ff;

Hm... all these magic numbers need some comments, and eventyally some
symbolic constants might be defined for them.

> +	}
> +
> +	m = (r >> 16) & mask;
> +	p = (r >> 8) & 0x3f;
> +	s = r & 0x7;
> +
> +	if (cpu_is_s5pc110()) {
> +		freq = CONFIG_SYS_CLK_FREQ_C110;
> +		if (pllreg == APLL) {
> +			if (s < 1)
> +				s = 1;
> +			fout = m * (freq / (p * (1 << (s - 1))));
> +		} else
> +			fout = m * (freq / (p * (1 << s)));
> +	} else {
> +		freq = CONFIG_SYS_CLK_FREQ_C100;
> +		fout = m * (freq / (p * (1 << s)));

All this black magic needs to be explained. Please add comments.

...
> +/* s5pc110: return HCLKs frequency */
> +unsigned long get_hclk_sys(int clk)
> +{
> +	unsigned long hclk;
> +	unsigned int div;
> +	unsigned int offset;
> +	unsigned int hclk_sys_ratio;
> +
> +	if (clk == CLK_M)
> +		return get_hclk();
> +
> +	div = s5pc1xx_clock_read_reg(S5P_CLK_DIV0_OFFSET);
> +
> +	/* HCLK_MSYS_RATIO: [10:8]
> +	 * HCLK_DSYS_RATIO: [19:16]
> +	 * HCLK_PSYS_RATIO: [27:24] */

Incorrect multiline comment style. Please fix globally.

...
> +#ifdef CONFIG_DISPLAY_CPUINFO
> +int print_cpuinfo(void)
> +{
> +	printf("CPU:\tS5P%X@%luMHz\n", s5pc1xx_cpu_id, get_arm_clk() / 1000000);
> +	if (cpu_is_s5pc110()) {
> +		printf("\tAPLL = %luMHz, MPLL = %luMHz, EPLL = %luMHz\n",
> +			get_fclk() / 1000000,
> +			get_mclk() / 1000000,
> +			get_uclk() / 1000000);
> +		printf("\tHclk: Msys %luMhz, Dsys %luMhz, Psys %luMhz\n",
> +			get_hclk_sys(CLK_M) / 1000000,
> +			get_hclk_sys(CLK_D) / 1000000,
> +			get_hclk_sys(CLK_P) / 1000000);
> +		printf("\tPclk: Msys %3luMhz, Dsys %3luMhz, Psys %3luMhz\n",
> +			get_pclk_sys(CLK_M) / 1000000,
> +			get_pclk_sys(CLK_D) / 1000000,
> +			get_pclk_sys(CLK_P) / 1000000);
> +	} else {
> +		printf("\tFclk = %luMHz, HclkD0 = %luMHz, PclkD0 = %luMHz,"
> +			" PclkD1 = %luMHz\n",
> +			get_fclk() / 1000000, get_hclk() / 1000000,
> +			get_pclkd0() / 1000000, get_pclkd1() / 1000000);

Please use strmhz() to format and print frequencies, as this will do
proper rounding.

...
> +static inline unsigned long READ_TIMER(void)
> +{

Please always use lower case identifier names.


> +/*
> + * This function is derived from PowerPC code (timebase clock frequency).
> + * On ARM it returns the number of timer ticks per second.
> + */
> +unsigned long get_tbclk(void)
> +{
> +	/* We overrun in 100s */
> +	return CONFIG_SYS_HZ * 100;
> +}

??? This look awkward to me. What exactly are you doing here, and why?


> --- /dev/null
> +++ b/include/asm-arm/arch-s5pc1xx/clock.h
...
> +/*
> + * Clock control
> + */
> +
> +/* Clock Register */
> +#define S5PC100_APLL_LOCK_OFFSET	0x0
> +#define S5PC100_MPLL_LOCK_OFFSET	0x4
> +#define S5PC100_EPLL_LOCK_OFFSET	0x8
> +#define S5PC100_HPLL_LOCK_OFFSET	0xc

Full NAK. Please declare C structs instead.

> --- /dev/null
> +++ b/include/asm-arm/arch-s5pc1xx/cpu.h
...
> +#define S5PC1XX_ADDR_BASE	0xe0000000
> +#define S5P_ADDR(x)		(S5PC1XX_ADDR_BASE + (x))

See previous comments about use of base plus register offsets.

> +/*
> + * Chip ID
> + */
> +#define S5PC1XX_CHIP_ID(x)	(0xE0000000 + (x))

Ditto.

> +#define S5PC1XX_PRO_ID		S5PC1XX_CHIP_ID(0)
> +#define S5PC1XX_OMR		S5PC1XX_CHIP_ID(4)

Ditto.

> +#define IS_SAMSUNG_TYPE(type, id)					\
> +static inline int is_##type(void)					\
> +{									\
> +	return (s5pc1xx_cpu_id == (id)) ? 1 : 0;			\
> +}
> +
> +IS_SAMSUNG_TYPE(s5pc100, 0xc100)
> +IS_SAMSUNG_TYPE(s5pc110, 0xc110)
> +
> +#define cpu_is_s5pc100()	is_s5pc100()
> +#define cpu_is_s5pc110()	is_s5pc110()
> +#endif

Check your intentions - is a runtime test really what you want? Or
would a compile time test be more appropriate?


> diff --git a/include/asm-arm/arch-s5pc1xx/gpio.h b/include/asm-arm/arch-s5pc1xx/gpio.h
> new file mode 100644
> index 0000000..26d0950
> --- /dev/null
> +++ b/include/asm-arm/arch-s5pc1xx/gpio.h
...
> +/* GPIO Bank Base */
> +#define S5PC100_GPIO_BASE(x)		(0xE0300000 + (x))
> +#define S5PC110_GPIO_BASE(x)		(0xE0200000 + (x))
> +
> +/* S5PC100 bank offset */
> +#define S5PC100_GPIO_A0_OFFSET		0x000
> +#define S5PC100_GPIO_A1_OFFSET		0x020
> +#define S5PC100_GPIO_B_OFFSET		0x040
> +#define S5PC100_GPIO_C_OFFSET		0x060

Full NAK. Use C structs. lease see above.

> diff --git a/include/asm-arm/arch-s5pc1xx/hardware.h b/include/asm-arm/arch-s5pc1xx/hardware.h
> new file mode 100644
> index 0000000..ca95c3d
> --- /dev/null
> +++ b/include/asm-arm/arch-s5pc1xx/hardware.h
...
> +#ifndef __ASSEMBLY__
> +#define UData(Data)	((unsigned long)(Data))
> +
> +#define __REG(x)	(*(vu_long *)(x))
> +#define __REGl(x)	(*(vu_long *)(x))
> +#define __REGw(x)	(*(vu_short *)(x))
> +#define __REGb(x)	(*(vu_char *)(x))
> +#define __REG2(x, y)	(*(vu_long *)((x) + (y)))
> +#else
> +#define UData(Data)	(Data)
> +
> +#define __REG(x)	(x)
> +#define __REGl(x)	(x)
> +#define __REGw(x)	(x)
> +#define __REGb(x)	(x)
> +#define __REG2(x, y)	((x) + (y))
> +#endif
> +
> +#define Fld(Size, Shft)	(((Size) << 16) + (Shft))
> +
> +#define FSize(Field)	((Field) >> 16)
> +#define FShft(Field)	((Field) & 0x0000FFFF)
> +#define FMsk(Field)	(((UData(1) << FSize(Field)) - 1) << FShft(Field))
> +#define FAlnMsk(Field)	((UData(1) << FSize(Field)) - 1)
> +#define F1stBit(Field)	(UData(1) << FShft(Field))
> +
> +#define FClrBit(Data, Bit)	(Data = (Data & ~(Bit)))
> +#define FClrFld(Data, Field)	(Data = (Data & ~FMsk(Field)))
> +
> +#define FInsrt(Value, Field) \
> +			(UData(Value) << FShft(Field))
> +
> +#define FExtr(Data, Field) \
> +			((UData(Data) >> FShft(Field)) & FAlnMsk(Field))

Please get rid of all these definitions. None of these will be
accepted in U-Boot.

Please use existent accessors and macros instead.

> diff --git a/include/asm-arm/arch-s5pc1xx/interrupt.h b/include/asm-arm/arch-s5pc1xx/interrupt.h
> new file mode 100644
> index 0000000..a3c8680
> --- /dev/null
> +++ b/include/asm-arm/arch-s5pc1xx/interrupt.h
...
> +/* Vector Interrupt Offset */
> +#define VIC_IRQSTATUS_OFFSET		0x0
> +#define VIC_FIQSTATUS_OFFSET		0x4
> +#define VIC_RAWINTR_OFFSET		0x8
> +#define VIC_INTSELECT_OFFSET		0xc
> +#define VIC_INTENABLE_OFFSET		0x10
> +#define VIC_INTENCLEAR_OFFSET		0x14
> +#define VIC_SOFTINT_OFFSET		0x18
> +#define VIC_SOFTINTCLEAR_OFFSET		0x1c
> +#define VIC_PROTECTION_OFFSET		0x20
> +#define VIC_SWPRIORITYMASK_OFFSET	0x24
> +#define VIC_PRIORITYDAISY_OFFSET	0x28
> +#define VIC_INTADDRESS_OFFSET		0xf00

Use a C struct instead?

> +#endif
> diff --git a/include/asm-arm/arch-s5pc1xx/keypad.h b/include/asm-arm/arch-s5pc1xx/keypad.h
> new file mode 100644
> index 0000000..0437b5e
> --- /dev/null
> +++ b/include/asm-arm/arch-s5pc1xx/keypad.h
...
> +#define S5PC100_KEYPAD_BASE		0xF3100000
> +#define S5PC110_KEYPAD_BASE		0xE1600000
> +
> +#define S5PC1XX_KEYIFCON_OFFSET		(0x00)
> +#define S5PC1XX_KEYIFSTSCLR_OFFSET	(0x04)
> +#define S5PC1XX_KEYIFCOL_OFFSET		(0x08)
> +#define S5PC1XX_KEYIFROW_OFFSET		(0x0C)
> +#define S5PC1XX_KEYIFFC_OFFSET		(0x10)

Use a C struct, please.

> diff --git a/include/asm-arm/arch-s5pc1xx/mem.h b/include/asm-arm/arch-s5pc1xx/mem.h
> new file mode 100644
> index 0000000..4b06aaf
> --- /dev/null
> +++ b/include/asm-arm/arch-s5pc1xx/mem.h
...
> +/*
> + * SROMC Controller
> + */
> +/* DRAM Memory Controller */
> +#define S5PC100_DMC_BASE	0xE6000000
> +#define S5PC110_DMC0_BASE	0xF0000000
> +#define S5PC110_DMC1_BASE	0xF1400000
> +
> +/* DMC offset */
> +#define CONCONTROL_OFFSET	0x00
> +#define MEMCONTROL_OFFSET	0x04
> +#define MEMCONFIG0_OFFSET	0x08
> +#define MEMCONFIG1_OFFSET	0x0c
> +#define DIRECTCMD_OFFSET	0x10
> +#define PRECHCONFIG_OFFSET	0x14
> +#define PHYCONTROL0_OFFSET	0x18
> +#define PHYCONTROL1_OFFSET	0x1c
> +#define PHYCONTROL2_OFFSET	0x20
> +#define PWRDNCONFIG_OFFSET	0x28
> +#define TIMINGAREF_OFFSET	0x30
> +#define TIMINGROW_OFFSET	0x34
> +#define TIMINGDATA_OFFSET	0x38
> +#define TIMINGPOWER_OFFSET	0x3c
> +#define PHYSTATUS0_OFFSET	0x40
> +#define PHYSTATUS1_OFFSET	0x44
> +#define CHIP0STATUS_OFFSET	0x48
> +#define CHIP1STATUS_OFFSET	0x4c
> +#define AREFSTATUS_OFFSET	0x50
> +#define MRSTATUS_OFFSET		0x54
> +#define PHYTEST0_OFFSET		0x58
> +#define PHYTEST1_OFFSET		0x5c

Use a C struct, please.

> diff --git a/include/asm-arm/arch-s5pc1xx/uart.h b/include/asm-arm/arch-s5pc1xx/uart.h
> new file mode 100644
> index 0000000..189bbff
> --- /dev/null
> +++ b/include/asm-arm/arch-s5pc1xx/uart.h
...
> +#ifndef __ASSEMBLY__
> +typedef struct s5pc1xx_uart {
> +	volatile unsigned long	ULCON;
> +	volatile unsigned long	UCON;
> +	volatile unsigned long	UFCON;
> +	volatile unsigned long	UMCON;
> +	volatile unsigned long	UTRSTAT;
> +	volatile unsigned long	UERSTAT;
> +	volatile unsigned long	UFSTAT;
> +	volatile unsigned long	UMSTAT;
> +#ifdef __BIG_ENDIAN
> +	volatile unsigned char	res1[3];
> +	volatile unsigned char	UTXH;
> +	volatile unsigned char	res2[3];
> +	volatile unsigned char	URXH;
> +#else /* Little Endian */
> +	volatile unsigned char	UTXH;
> +	volatile unsigned char	res1[3];
> +	volatile unsigned char	URXH;
> +	volatile unsigned char	res2[3];
> +#endif

Do you really want tto support both big endian and little endian
support for these SoCs? Is this conesequentlky done in the rest of all
other files, too?  Has this actually been tested?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If it happens once, it's a bug.
If it happens twice, it's a feature.
If it happens more than twice, it's a design philosophy.