[U-Boot] [PATCH 1/4 v2] s5pc1xx: support Samsung s5pc1xx SoC
Wolfgang Denk
wd at denx.de
Thu Sep 10 12:31:16 CEST 2009
Dear Minkyu Kang,
In message <4AA8AC30.2070807 at samsung.com> you wrote:
> This patch adds support for the Samsung s5pc100 and s5pc110
> SoCs. The s5pc1xx SoC is an ARM Cortex A8 processor.
>
> Signed-off-by: Minkyu Kang <mk7.kang at samsung.com>
> Signed-off-by: HeungJun, Kim <riverful.kim at samsung.com>
When posting new versions of patches, please always make sure to set
appropriate "In-reply-to:" and "References:" headers, so your new
versions get correctly threaded with the previous discussion, which
makes it much easier to check previous review comments against what
you actually changed.
Also, please always include a list of changed between the current and
the previous version of the patch, otherwise we have to review the
whole code completely again.
Please consider using "git send-email" to submit patches which will
make your (and our) life easier, for example because it prompts you
for the message IDs to be used in the "In-reply-to:" and
"References:" headers.
But the biggest problem with your new version is that you obviously
ignore some (large) parts of my previous review comments. This is not
the way it is supposed to work.
I don't waste my time on reviewing your code if you don't bother to
fix it.
If you want to get your code into mainline, than please clean it up
as requested. If you don't understand what changes are required, then
please ask. If you think a change request is unreasonable, the ask.
But silently ignoring review comments is not helpful.
> +unsigned long get_pll_clk(int pllreg)
> +{
> + struct s5pc100_clock *clk_c100 =
> + (struct s5pc100_clock *)S5PC1XX_CLOCK_BASE;
> + struct s5pc110_clock *clk_c110 =
> + (struct s5pc110_clock *)S5PC1XX_CLOCK_BASE;
> + unsigned long r, m, p, s, mask, fout;
> + unsigned int freq;
> +
> + switch (pllreg) {
> + case APLL:
> + if (cpu_is_s5pc110())
> + r = readl(&clk_c110->APLL_CON);
> + else
> + r = readl(&clk_c100->APLL_CON);
> + break;
> + case MPLL:
> + if (cpu_is_s5pc110())
> + r = readl(&clk_c110->MPLL_CON);
> + else
> + r = readl(&clk_c100->MPLL_CON);
> + break;
> + case EPLL:
> + if (cpu_is_s5pc110())
> + r = readl(&clk_c110->EPLL_CON);
> + else
> + r = readl(&clk_c100->EPLL_CON);
> + break;
> + case HPLL:
> + if (cpu_is_s5pc110()) {
> + puts("s5pc110 don't use HPLL\n");
> + return 0;
> + }
> + r = readl(&clk_c100->HPLL_CON);
> + break;
> + case VPLL:
> + if (cpu_is_s5pc100()) {
> + puts("s5pc100 don't use VPLL\n");
> + return 0;
> + }
> + r = readl(&clk_c110->VPLL_CON);
> + break;
I think this code is inefficient, and it does not scale at all if you
ever have to add additional processors. I recomment we don't try to
mix this code all in a single function that covers all processors.
Instead, provide two separate functions, one for each of the CPU
types, and then set a fuction pointer to the right function. This way
we have only a single test for the CPU type (when stting the function
pointer). And this allows easily to add support for other CPU types,
and even then the code will remain readable.
> + if (cpu_is_s5pc110()) {
> + freq = CONFIG_SYS_CLK_FREQ_C110;
> + if (pllreg == APLL) {
> + if (s < 1)
> + s = 1;
> + /* FOUT = MDIV * FIN / (PDIV * 2^(SDIV - 1)) */
> + fout = m * (freq / (p * (1 << (s - 1))));
> + } else
> + /* FOUT = MDIV * FIN / (PDIV * 2^SDIV) */
> + fout = m * (freq / (p * (1 << s)));
> + } else {
> + /* FOUT = MDIV * FIN / (PDIV * 2^SDIV) */
> + freq = CONFIG_SYS_CLK_FREQ_C100;
> + fout = m * (freq / (p * (1 << s)));
> + }
Similar here. I suggest you move this CPU dependent code into separate
functions. There will be more such places below. In the end, you can
probably even split the CPU specific code off into separate files, and
set up a table with method pointers. At a central place a single test
for the CPU type will be done, and the right method table be selected.
The code here will then be lean and completely independent of CPU
types. Thsi will be much easier to read, and easier to maintain.
> +unsigned long get_arm_clk(void)
> +{
> + struct s5pc100_clock *clk = (struct s5pc100_clock *)S5PC1XX_CLOCK_BASE;
> + unsigned long div;
> + unsigned long dout_apll, armclk;
> + unsigned int apll_ratio, arm_ratio;
> +
> + div = readl(&clk->DIV0);
> + if (cpu_is_s5pc110()) {
> + /* ARM_RATIO: don't use */
> + arm_ratio = 0;
> + /* APLL_RATIO: [2:0] */
> + apll_ratio = div & 0x7;
> + } else {
> + /* ARM_RATIO: [6:4] */
> + arm_ratio = (div >> 4) & 0x7;
> + /* APLL_RATIO: [0] */
> + apll_ratio = div & 0x1;
> + }
> +
> + dout_apll = get_pll_clk(APLL) / (apll_ratio + 1);
> + armclk = dout_apll / (arm_ratio + 1);
> +
> + return armclk;
> +}
This is another such candiate.
> +/* return FCLK frequency */
> +unsigned long get_fclk(void)
> +{
> + return get_pll_clk(APLL);
> +}
> +
> +/* return MCLK frequency */
> +unsigned long get_mclk(void)
> +{
> + return get_pll_clk(MPLL);
> +}
It seems nobody else uses these functions, and get_pll_clk() is
globally visible, too. So we can probably omit get_fclk() and
get_mclk() and rather call get_pll_clk() directly?
> +/* s5pc100: return HCLKD0 frequency */
> +unsigned long get_hclk(void)
...
> +/* s5pc100: return PCLKD0 frequency */
> +unsigned long get_pclkd0(void)
...
> +/* s5pc100: return PCLKD1 frequency */
> +unsigned long get_pclkd1(void)
...
> +/* s5pc110: return HCLKs frequency */
> +unsigned long get_hclk_sys(int dom)
...
> +/* s5pc110: return PCLKs frequency */
> +unsigned long get_pclk_sys(int dom)
...
Seems these are even more candiates for method pointers...
And more follow. I think this should be reworked globally.
> +int print_cpuinfo(void)
> +{
> + char buf[32];
> +
> + printf("CPU:\tS5P%X@%sMHz\n",
> + s5pc1xx_cpu_id, strmhz(buf, get_arm_clk()));
> + if (cpu_is_s5pc110()) {
> + printf("\tAPLL = %s MHz, ", strmhz(buf, get_fclk()));
> + printf("MPLL = %s MHz, ", strmhz(buf, get_mclk()));
> + printf("EPLL = %s MHz\n", strmhz(buf, get_uclk()));
> +
> + printf("\tHclk: Msys %s MHz, ",
> + strmhz(buf, get_hclk_sys(CLK_M)));
> + printf("Dsys %7s MHz, ", strmhz(buf, get_hclk_sys(CLK_D)));
> + printf("Psys %7s MHz\n", strmhz(buf, get_hclk_sys(CLK_P)));
> +
> + printf("\tPclk: Msys %s MHz, ",
> + strmhz(buf, get_pclk_sys(CLK_M)));
> + printf("Dsys %7s MHz, ", strmhz(buf, get_pclk_sys(CLK_D)));
> + printf("Psys %7s MHz\n", strmhz(buf, get_pclk_sys(CLK_P)));
> + } else {
> + printf("\tFclk = %s MHz\n", strmhz(buf, get_fclk()));
> + printf("\tHclkD0 = %s MHz\n", strmhz(buf, get_hclk()));
> + printf("\tPclkD0 = %s MHz\n", strmhz(buf, get_pclkd0()));
> + printf("\tPclkD1 = %s MHz\n", strmhz(buf, get_pclkd1()));
> + }
Please review how much of this is really significant information for
the end user, and how much is more or less only interesting for the
developer. Consider to change such development-only parts into debug()
calls.
> diff --git a/include/asm-arm/arch-s5pc1xx/clock.h b/include/asm-arm/arch-s5pc1xx/clock.h
> new file mode 100644
> index 0000000..f11c9ce
> --- /dev/null
> +++ b/include/asm-arm/arch-s5pc1xx/clock.h
...
> +/* Clock Register */
> +
> +#ifndef __ASSEMBLY__
> +struct s5pc100_clock {
> + unsigned long APLL_LOCK;
> + unsigned long MPLL_LOCK;
> + unsigned long EPLL_LOCK;
> + unsigned long HPLL_LOCK;
> + unsigned char res1[0xf0];
> + unsigned long APLL_CON;
> + unsigned long MPLL_CON;
> + unsigned long EPLL_CON;
> + unsigned long HPLL_CON;
> + unsigned char res2[0xf0];
> + unsigned long SRC0;
> + unsigned long SRC1;
> + unsigned long SRC2;
> + unsigned long SRC3;
> + unsigned char res3[0xf0];
> + unsigned long DIV0;
> + unsigned long DIV1;
> + unsigned long DIV2;
> + unsigned long DIV3;
> + unsigned long DIV4;
> + unsigned char res4[0x1ec];
> + unsigned long GATE_D00;
> + unsigned long GATE_D01;
> + unsigned long GATE_D02;
> + unsigned char res5[0x54];
> + unsigned long GATE_SCLK0;
> + unsigned long GATE_SCLK1;
> +};
Sorry, but variable names must not use upper case letters; these are
reserved for macro definitions only. Please rename and use lower case
letters only. Also for other names in the rest of the patch.
> diff --git a/include/asm-arm/arch-s5pc1xx/cpu.h b/include/asm-arm/arch-s5pc1xx/cpu.h
> new file mode 100644
> index 0000000..c7d7183
> --- /dev/null
> +++ b/include/asm-arm/arch-s5pc1xx/cpu.h
..
> +#define S5PC1XX_ADDR_BASE 0xe0000000
> +#define S5P_ADDR(x) (S5PC1XX_ADDR_BASE + (x))
> +
> +#define S5PC1XX_CLOCK_BASE 0xE0100000
> +#define S5P_PA_CLK_OTHERS S5P_ADDR(0x00200000) /* Clock Others Base */
Seems thisis the only place where S5P_ADDR is used - consider to avoid
it.
By the way: I already commented about this part in my previous review.
Why did you not change it?
> +/*
> + * Chip ID
> + */
> +#define S5PC1XX_CHIP_ID(x) (0xE0000000 + (x))
> +#define S5PC1XX_PRO_ID S5PC1XX_CHIP_ID(0)
> +#define S5PC1XX_OMR S5PC1XX_CHIP_ID(4)
Create a C struct instead of using base address + offset?
> +#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
Why do we need both the inline functions and a macro? Maybe we can use
the right name in the inline functions and drop the macro?
> 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
> +#define S5PC100_GPIO_D_OFFSET 0x080
> +#define S5PC100_GPIO_E0_OFFSET 0x0A0
> +#define S5PC100_GPIO_E1_OFFSET 0x0C0
> +#define S5PC100_GPIO_F0_OFFSET 0x0E0
> +#define S5PC100_GPIO_F1_OFFSET 0x100
> +#define S5PC100_GPIO_F2_OFFSET 0x120
> +#define S5PC100_GPIO_F3_OFFSET 0x140
> +#define S5PC100_GPIO_G0_OFFSET 0x160
> +#define S5PC100_GPIO_G1_OFFSET 0x180
> +#define S5PC100_GPIO_G2_OFFSET 0x1A0
> +#define S5PC100_GPIO_G3_OFFSET 0x1C0
> +#define S5PC100_GPIO_I_OFFSET 0x1E0
> +#define S5PC100_GPIO_J0_OFFSET 0x200
> +#define S5PC100_GPIO_J1_OFFSET 0x220
> +#define S5PC100_GPIO_J2_OFFSET 0x240
> +#define S5PC100_GPIO_J3_OFFSET 0x260
> +#define S5PC100_GPIO_J4_OFFSET 0x280
> +#define S5PC100_GPIO_K0_OFFSET 0x2A0
> +#define S5PC100_GPIO_K1_OFFSET 0x2C0
> +#define S5PC100_GPIO_K2_OFFSET 0x2E0
> +#define S5PC100_GPIO_K3_OFFSET 0x300
> +#define S5PC100_GPIO_L0_OFFSET 0x320
> +#define S5PC100_GPIO_L1_OFFSET 0x340
> +#define S5PC100_GPIO_L2_OFFSET 0x360
> +#define S5PC100_GPIO_L3_OFFSET 0x380
> +#define S5PC100_GPIO_L4_OFFSET 0x3A0
> +#define S5PC100_GPIO_H0_OFFSET 0xC00
> +#define S5PC100_GPIO_H1_OFFSET 0xC20
> +#define S5PC100_GPIO_H2_OFFSET 0xC40
> +#define S5PC100_GPIO_H3_OFFSET 0xC60
...
If this should be used anywhere in the code, add this needs to be
converted in a C struct, too.
But as far as I can see, only S5PC100_GPIO_A*_OFFSET are ever
referenced (from assembler code); maybe we can omit most of this
stuff ? These offsets should then be auto-generated.
But again this is a part I already commented about in my previous
review.
> 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))
> +
I wrote before:
> Please get rid of all these definitions. None of these will be
> accepted in U-Boot.
>
> Please use existent accessors and macros instead.
Why did you not implement the required chanegs?
Review stops here. Please fix first.
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
"Here's a fish hangs in the net like a poor man's right in the law.
'Twill hardly come out." - Shakespeare, Pericles, Act II, Scene 1
More information about the U-Boot
mailing list