Dear Wolfgang 2009/9/10 Wolfgang Denk <w...@denx.de>: > Dear Minkyu Kang, > > In message <4aa8ac30.2070...@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.k...@samsung.com> >> Signed-off-by: HeungJun, Kim <riverful....@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:\ts5...@%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: w...@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@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot > thanks. -- from. prom. www.promsoft.net _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot