Dear Chander Kashyap,
On 5 January 2012 19:31, Chander Kashyap <chander.kash...@linaro.org> wrote:
> Hi Minkyu Kang,
>
> On 5 January 2012 12:13, Minkyu Kang <proms...@gmail.com> wrote:
>> Dear Chander Kashyap,
>>
>> On 27 December 2011 17:48, Chander Kashyap <chander.kash...@linaro.org>
>> wrote:
>>>>> > Torsten Koschorrek <koschor...@synertronixx.de>
>>>>> > scb9328 ARM920T (i.MXL)
>>>>> > diff --git a/arch/arm/cpu/armv7/exynos/clock.c
>>>>> > b/arch/arm/cpu/armv7/exynos/clock.c
>>>>> > index b101f96..88e2fc0 100644
>>>>> > --- a/arch/arm/cpu/armv7/exynos/clock.c
>>>>> > +++ b/arch/arm/cpu/armv7/exynos/clock.c
>>>>> > @@ -125,10 +125,14 @@ static unsigned long exynos_get_pwm_clk(void)
>>>>> >
>>>>> > if (s5p_get_cpu_rev() == 0) {
>>>>> > /*
>>>>> > - * CLK_SRC_PERIL0
>>>>> > + * CLK_SRC_{PERIL0 | PERIC0}
>>>>> > * PWM_SEL [27:24]
>>>>> > */
>>>>> > +#ifdef CONFIG_EXYNOS5
>>>>> > + sel = readl(&clk->src_peric0);
>>>>> > +#else
>>>>> > sel = readl(&clk->src_peril0);
>>>>> > +#endif
>>>>>
>>>>> NAK.
>>>>> We don't allow to using ifdef for separating SoCs.
>>>>> Please refer s5pc1xx case for solve it.
>>>>> This comment apply to this patch globally.
>>>>> Please remove '#ifdef CONFIG_EXYNOS5'.
>>>>>
>>>> I have tried to reuse the code. It is possible to remove
>>>> #ifdef CONFIG_EXYNOS5' in clock.c with cpu_is_s5pcXXX check.
>>>> Is it a acceptable solution? Or is it necessary to write SoC specific
>>>> function
>>>> in clock.c as done in case of s5pc1xx/clock.c.
>>>>
>>>> Please Advice
>>> Removing CONFIG_EXYNOS5 and following s5pc1xx case will not allow to
>>> reuse the code in clock.c.
>>> What is the technical hindrance of not using ifdefs?
>>
>> No need to reuse the code, if SoCs are different.
>> If need, please separate the functions.
>
> Yes, though SoC's are different, the functionality in clock.c is
> similar. The only difference is the clock name in the clock structure
> for Exynos4 and Exynos5 but the functionality is exactly same in
> clock.c. To accommodate this change in clock name #ifdef is used.
>
> Following is the function in clock .c which uses #ifdef to accommodate
> the different clock name in SoC's.
>
> static unsigned long exynos_get_pwm_clk(void)
> {
> struct exynos_clock *clk =
> (struct exynos_clock *)samsung_get_base_clock();
> unsigned long pclk, sclk;
> unsigned int sel;
> unsigned int ratio;
>
> if (s5p_get_cpu_rev() == 0) {
> /*
> * CLK_SRC_{PERIL0 | PERIC0}
> * PWM_SEL [27:24]
> */
> #ifdef CONFIG_EXYNOS5
> sel = readl(&clk->src_peric0);
> #else
> sel = readl(&clk->src_peril0);
> #endif
> sel = (sel >> 24) & 0xf;
>
> if (sel == 0x6)
> sclk = get_pll_clk(MPLL);
> else if (sel == 0x7)
> sclk = get_pll_clk(EPLL);
> else if (sel == 0x8)
> sclk = get_pll_clk(VPLL);
> else
> return 0;
>
> /*
> * CLK_DIV_{PERIL3 | PERIC3}
> * PWM_RATIO [3:0]
> */
> #ifdef CONFIG_EXYNOS5
> ratio = readl(&clk->div_peric3);
> #else
> ratio = readl(&clk->div_peril3);
> #endif
> ratio = ratio & 0xf;
> } else if (s5p_get_cpu_rev() == 1) {
> sclk = get_pll_clk(MPLL);
> ratio = 8;
> } else
> return 0;
>
> pclk = sclk / (ratio + 1);
>
> return pclk;
> }
>
> As listed above, the exynos_get_pwm_clk(() function is fully reusable
> for both Exynos4 and Exynos5. The #ifdef was used to get around the
> different clock names of Exynos4 and Exynos5.
>
> Another option here could be, that the differing clock name
> (src_peril0 for Exynos4 and src_peric0 for Exynos5) is resolved by
> change the src_peric0 to src_peril0 for Exynos5, and clearly
> commenting the reason for the deviation from the user manual. Would
> this approach be acceptable ?
>
Using same clock's name is acceptable.
But exynos4 clock structure and exynos5 clock structure are different.
I requested removing all ifdefs in your patch.
So, I will not allow such cases.
+#ifdef CONFIG_EXYNOS4
+#include<asm/arch/clock_exynos4.h>
+#elif defined CONFIG_EXYNOS5
+#include<asm/arch/clock_exynos5.h>
#endif
Then, we should do like this,
struct exynos4_clock *clk =
(struct exynos4_clock *)samsung_get_base_clock();
struct exynos5_clock *clk =
(struct exynos5_clock *)samsung_get_base_clock();
how solve it?
And you should consider variation by cpu revision.
If Exynos5 revision1 is different from Exynos4 revision1, then?
Thanks.
Minkyu Kang.
--
from. prom.
www.promsoft.net
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
