[U-Boot] [PATCH 4/6] EEYNOS: Add SMDK5250 board support

[Chander Kashyap]

Dear Minkyu Kang,

On 6 January 2012 07:10, Minkyu Kang <promsoft at gmail.com> wrote:
> Dear Chander Kashyap,
>
> On 5 January 2012 19:31, Chander Kashyap <chander.kashyap at linaro.org> wrote:
>> Hi Minkyu Kang,
>>
>> On 5 January 2012 12:13, Minkyu Kang <promsoft at gmail.com> wrote:
>>> Dear Chander Kashyap,
>>>
>>> On 27 December 2011 17:48, Chander Kashyap <chander.kashyap at linaro.org> wrote:
>>>>>> >  Torsten Koschorrek <koschorrek at 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