Dear Minkyu Kang,
On 6 January 2012 07:10, Minkyu Kang <proms...@gmail.com> wrote:
> 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.
In that case S5PC1XX approach is suitable.
>
> +#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?
>
I will resend patche based on s5pc1xx approch.
> Thanks.
> Minkyu Kang.
> --
> from. prom.
> www.promsoft.net
--
with warm regards,
Chander Kashyap
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
