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

[Minkyu Kang]

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.

+#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