[U-Boot] [PATCH 2/5] arm: exynos: add display clocks for Exynos5800

[Ajay kumar]

Hi Minkyu,


On Mon, Dec 8, 2014 at 11:07 AM, Ajay kumar <ajaynumb at gmail.com> wrote:
> On Fri, Dec 5, 2014 at 9:02 PM, Minkyu Kang <promsoft at gmail.com> wrote:
>> Dear Ajay Kumar,
>>
>> On 5 December 2014 at 23:13, Ajay Kumar <ajaykumar.rs at samsung.com> wrote:
>>
>>> Add get_lcd_clk and set_lcd_clk callbacks for Exynos5800 needed by
>>> exynos video driver.
>>>
>>> Signed-off-by: Ajay Kumar <ajaykumar.rs at samsung.com>
>>> ---
>>>  arch/arm/cpu/armv7/exynos/clock.c      |   63
>>> +++++++++++++++++++++++++++++++-
>>>  arch/arm/include/asm/arch-exynos/clk.h |    3 ++
>>>  2 files changed, 64 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/cpu/armv7/exynos/clock.c
>>> b/arch/arm/cpu/armv7/exynos/clock.c
>>> index 8fab135..1a34ad6 100644
>>> --- a/arch/arm/cpu/armv7/exynos/clock.c
>>> +++ b/arch/arm/cpu/armv7/exynos/clock.c
>>> @@ -1066,6 +1066,36 @@ static unsigned long exynos5420_get_lcd_clk(void)
>>>         return pclk;
>>>  }
>>>
>>> +static unsigned long exynos5800_get_lcd_clk(void)
>>> +{
>>> +       struct exynos5420_clock *clk =
>>> +               (struct exynos5420_clock *)samsung_get_base_clock();
>>> +       unsigned long sclk;
>>> +       unsigned sel;
>>>
>>
>> just unsigned? why don't you specify in detail?
I will fix this.

>>
>>> +       unsigned ratio;
>>> +
>>> +       sel = (readl(&clk->src_disp10) >> 4) & 7;
>>>
>>
>> please add comment how you get "sel" from disp10.
>> and if 7 means mask then please use 0x7. it looks more clearly.
Ok.

>> +
>>> +       /*
>>> +        * Mapping of CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4] values into
>>> +        * PLLs. The first element is a placeholder to bypass the
>>> +        * default settig.
>>> +        */
>>> +       const int reg_map[] = {0, CPLL, DPLL, MPLL, SPLL,
>>> +                              IPLL, EPLL,  RPLL};
>>>
>>
>> please don't define local variable at middle of function.
>> you can move it to top of the function or
>> it seems to use sel is true then you can move it into the if statement.
I will move this to the top of the function.

>>
>>> +       if (sel)
>>> +               sclk = get_pll_clk(reg_map[sel]);
>>> +       else
>>> +               sclk = 24000000;
>>>
>>
>> please define this value.
Ok.

>>
>>> +       /*
>>> +        * CLK_DIV_DISP10
>>> +        * FIMD1_RATIO [3:0]
>>> +        */
>>> +       ratio = readl(&clk->div_disp10) & 0xf;
>>> +
>>> +       return sclk / (ratio + 1);
>>> +}
>>> +
>>>  void exynos4_set_lcd_clk(void)
>>>  {
>>>         struct exynos4_clock *clk =
>>> @@ -1197,6 +1227,31 @@ void exynos5420_set_lcd_clk(void)
>>>         writel(cfg, &clk->div_disp10);
>>>  }
>>>
>>> +void exynos5800_set_lcd_clk(void)
>>> +{
>>> +       struct exynos5420_clock *clk =
>>> +               (struct exynos5420_clock *)samsung_get_base_clock();
>>> +       unsigned int cfg;
>>> +
>>> +       /*
>>> +        * Use RPLL for pixel clock
>>> +        * CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4]
>>> +        * ==================
>>> +        * 111: SCLK_RPLL
>>> +        */
>>> +       cfg = readl(&clk->src_disp10) | (7 << 4);
>>> +       writel(cfg, &clk->src_disp10);
>>> +
>>> +       /*
>>> +        * CLK_DIV_DISP10
>>> +        * FIMD1_RATIO          [3:0]
>>> +        */
>>> +       cfg = readl(&clk->div_disp10);
>>> +       cfg &= ~(0xf << 0);
>>> +       cfg |= (0 << 0);
>>>
>>
>> it looks meaningless.
Why not?
I agree that the calculation can be skipped, and directly FIMD
clock divider can be set to 0. But, I prefer to keep the readability.
In fact, similar "meaningless" code is already part of the tree:
http://git.denx.de/?p=u-boot/u-boot-samsung.git;a=blob;f=arch/arm/cpu/armv7/exynos/clock.c;h=8fab135bebf4ef6900677847b60a8e1a1520254c;hb=HEAD#l1194

Ajay
>>
>>> +       writel(cfg, &clk->div_disp10);
>>> +}
>>> +
>>>  void exynos4_set_mipi_clk(void)
>>>  {
>>>         struct exynos4_clock *clk =
>>> @@ -1669,8 +1724,10 @@ unsigned long get_lcd_clk(void)
>>>         if (cpu_is_exynos4())
>>>                 return exynos4_get_lcd_clk();
>>>         else {
>>> -               if (proid_is_exynos5420() || proid_is_exynos5800())
>>> +               if (proid_is_exynos5420())
>>>                         return exynos5420_get_lcd_clk();
>>> +               else if (proid_is_exynos5800())
>>> +                       return exynos5800_get_lcd_clk();
>>>                 else
>>>                         return exynos5_get_lcd_clk();
>>>         }
>>> @@ -1683,8 +1740,10 @@ void set_lcd_clk(void)
>>>         else {
>>>                 if (proid_is_exynos5250())
>>>                         exynos5_set_lcd_clk();
>>> -               else if (proid_is_exynos5420() || proid_is_exynos5800())
>>> +               else if (proid_is_exynos5420())
>>>                         exynos5420_set_lcd_clk();
>>> +               else
>>> +                       exynos5800_set_lcd_clk();
>>>         }
>>>  }
>>>
>>> diff --git a/arch/arm/include/asm/arch-exynos/clk.h
>>> b/arch/arm/include/asm/arch-exynos/clk.h
>>> index db24dc0..bf3d348 100644
>>> --- a/arch/arm/include/asm/arch-exynos/clk.h
>>> +++ b/arch/arm/include/asm/arch-exynos/clk.h
>>> @@ -16,6 +16,9 @@
>>>  #define BPLL   5
>>>  #define RPLL   6
>>>  #define SPLL   7
>>> +#define CPLL   8
>>> +#define DPLL   9
>>> +#define IPLL   10
>>>
>>>  #define MASK_PRE_RATIO(x)      (0xff << ((x << 4) + 8))
>>>  #define MASK_RATIO(x)          (0xf << (x << 4))
>>> --
>>> 1.7.9.5
>>>
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot at lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>
>>
>>
>> Thanks,
>> Minkyu Kang.
>> --
>> from. prom.
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>>