[U-Boot] [PATCH 1/4 v2] s5pc1xx: support Samsung s5pc1xx SoC
Minkyu Kang
promsoft at gmail.com
Fri Sep 11 12:06:45 CEST 2009
Dear Wolfgang
2009/9/10 Wolfgang Denk <wd at denx.de>:
> 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.
>
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
More information about the U-Boot
mailing list