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

[Simon Glass]

Hi Ajay,

On 8 December 2014 at 15:40, Simon Glass <sjg at google.com> wrote:
> Hi Ajay,
>
>
> On 7 December 2014 at 22:43, Ajay kumar <ajaynumb at gmail.com> wrote:
>>
>> 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
>
>
> Well perhaps a #define for the shift value? Then it would mean something.
>
> Also this can probably use clrsetbits_le32().

This is one of four patches that I think were never applied and need a
respin. Are you able to do that? Then Pi LCD will work OK I think.

http://patchwork.ozlabs.org/patch/418116/
http://patchwork.ozlabs.org/patch/418117/
http://patchwork.ozlabs.org/patch/418118/
http://patchwork.ozlabs.org/patch/418119/

Regards.
Simon