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

[Chander Kashyap]

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 ?

Thanks for your comments. Please let know your opinion about the
points listed in this email.


>
> like this,
>
> unsigned long get_arm_clk(void)
> {
>        if (cpu_is_s5pc110())
>                return s5pc110_get_arm_clk();
>        else
>                return s5pc100_get_arm_clk();
> }
>
> Thanks.
> Minkyu Kang
> --
> from. prom.
> www.promsoft.net



-- 
with warm regards,
Chander Kashyap