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

[Minkyu Kang <promsoft@gmail.com>]

Dear Wolfgang

2009/9/10 Wolfgang Denk <wd@denx.de>:
> Dear Minkyu Kang,
>
> In message <4AA8AC30.2070807@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@samsung.com>
>> Signed-off-by: HeungJun, Kim <riverful.kim@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.
>

I see.

>
>
> 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.
>
>

hm.. did I?
sorry, I just missing..
I'll check all of your comments again..
I'm sorry to your time

>
>> +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.
>

I'll split the function.
and I  think we don't need the function pointer fully.
get_pclkd0, get_pclkd1, get_hclk_sys and get_pclk_sys are not generic functions.
but, get_pll_clk, get_arm_clk and get_pclk can be make function pointer.

>
>> + ? ? 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.

ok i'll rechek.

>
>> +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.

ok. I will fix it.

>
>> 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.

i'll fix it, too.

>
> By the way: I already commented about this part in my previous review.
> Why did you not change it?

sorry again.

>
>> +/*
>> + * 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?
>

we don't use S5PC1XX_OMR so I'll delete it.
and will fix like this

+#define S5PC1XX_PRO_ID               0xE0000000

>> +#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?

agreed. i'll 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.
>

on smdkc100? yes right.. but, our other boards use so many gpios.
so, it was hard to rework.
but, I will consider to convert C struct.

>
> 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?

sorry, this file will be deleted.

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

thanks.

-- 
from. prom.
www.promsoft.net