Re: [PATCH V5 5/5] ARM: EXYNOS4: Add platform data for EXYNOS4 FIMD and LTE480WV platform-lcd

Hi Marek/Jingoo,

On Wed, Jun 22, 2011 at 3:17 PM, Marek Szyprowski
[off-list ref] wrote:

Hello,

On Wednesday, June 22, 2011 8:42 AM Jingoo Han wrote:

quoted

From: Jonghun Han <redacted>

(snipped)
Instead of hardcoding the parent clock in platform/bootloader code ,is
it not possible to select/set the
parent clock based on the pixel clk(plat data) having the least delta
with the 9 src clks.with this change I have checked it for WA101S.
These are the changes I am proposing..

arch/arm/mach-exynos4/clock.c
+struct clk *clkset_sclk_fimd0_list[] = {
+        [0] = &clk_sclk_xxti,
+        [1] = &clk_sclk_xusbxti,
+        [2] = &clk_sclk_hdmi27m,
+        [3] = &clk_sclk_usbphy0,
+        [4] = &clk_sclk_usbphy1,
+        [5] = &clk_sclk_hdmiphy,
+        [6] = &clk_mout_mpll.clk,
+        [7] = &clk_mout_epll.clk,
+        [8] = &clk_sclk_vpll.clk,
+};
arch/arm/mach-exynos4/mach-smdkv310.c
+static int fimd_s3c_consider_clock(struct device *sfb,int src
,unsigned int wanted)
+{
+        unsigned long rate = 0;
+        int div =1;
+        struct clk *mout_mpll = NULL;
+
+        if(clkset_sclk_fimd0_list[src]){
+        mout_mpll = clk_get(sfb,clkset_sclk_fimd0_list[src]->name);
+        if (IS_ERR(mout_mpll)) {
+               dev_err(sfb, "failed to clk_get
%s\n",clkset_sclk_fimd0_list[src]->name);
+        }
+
+        rate = clk_get_rate(mout_mpll);
+
+        for (div = 1; div < 256; div *= 2) {
+                if ((rate / div) <= wanted)
+                        break;
+        }
+
+
+       }
+        return (wanted - (rate / div));
+}
+
+int exynos4_fimd0_find_clock(struct platform_device *pdev,struct clk
**lcd_clk,unsigned int clock(pixel clock needed for the lcd))
+{
+        int best = 0;
+        int delta = 0;
+        int best_src = 0;
+        int src;
+        struct clk *best_clk_src = NULL;
+       struct clk *clk  = NULL;
+
+       if (clock = 0)
+                return 0;
+
+        for (src = 0; src < MAX_NUM_CLKS ;src++) {
+                delta = fimd_s3c_consider_clock(&pdev->dev,src,clock);
+                if (delta < best) {
+                        best = delta;
+                        best_src = src;
+                }
+        }
+       clk = clk_get(&pdev->dev, "sclk_fimd");
+        if (IS_ERR(clk)) {
+               dev_err(&pdev->dev, "failed to get sclk for fimd\n");
+               goto err_clk2;
+       }
+
+       best_clk_src clk_get(&pdev->dev,clkset_sclk_fimd0_list[best_src]->name);
+       if (IS_ERR(best_clk_src)) {
+               dev_err(&pdev->dev, "failed to get best_src\n");
+               goto err_clk1;
+       }
+       clk_set_parent(clk,best_clk_src);
+       *lcd_clk = clk;
+        clk_put(best_clk_src);
+        clk_enable(clk);
+        dev_dbg(&pdev->dev, "set fimd sclk rate to %d\n", clock);
+
+err_clk1:
+       clk_put(best_clk_src);
+err_clk2:
+       clk_put(clk);
+
+       return -EINVAL;
+}
I have based these patches on the v1 version of this patch.I can base
these changes
on your latest changes(v6) and send as a patch.

quoted

+static int __init smdkc210_fimd0_setup_clock(void)
+{
+     struct clk *sclk = NULL;
+     struct clk *mout_mpll = NULL;
+
+     u32 rate = 0;
+
+     sclk = clk_get(&s5p_device_fimd0.dev, "sclk_fimd");
+     if (IS_ERR(sclk)) {
+             printk(KERN_ERR "failed to get sclk for fimd\n");
+             goto err_clk2;
+     }
+
+     mout_mpll = clk_get(NULL, "mout_mpll");
+     if (IS_ERR(mout_mpll)) {
+             printk(KERN_ERR "failed to get mout_mpll\n");
+             goto err_clk1;
+     }
+
+     clk_set_parent(sclk, mout_mpll);
+     if (!rate)
+             rate = 134000000;
+
+     clk_set_rate(sclk, rate);
+
+     clk_put(sclk);
+     clk_put(mout_mpll);
+
+     return 0;
+
+err_clk1:
+     clk_put(mout_mpll);
+err_clk2:
+     clk_put(sclk);
+
+     return -EINVAL;
+}
+

I'm not sure if mach-smdk*.c is the right place for the above code.
IMHO all the code that configures very low level, board specific parameters
(like clocks and their relations) should be performed in boot loarder.

I feel pushing the clock enabling into the bootloader,will create an
unneccesary dependcy for the lcd on the u-boot.

(snipped)

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

regards
anand

On 06/22/2011 03:25 PM, Anand Kumar N wrote:

Instead of hardcoding the parent clock in platform/bootloader code ,is
it not possible to select/set the
parent clock based on the pixel clk(plat data) having the least delta
with the 9 src clks.with this change I have checked it for WA101S.

While at first sight this may look as an awesome idea I don't really 
believe it is. There might be other criteria that need to be considered,
like continuous clock availability or frequency stability. 
That said we could possibly have the driver setting sclk_fimd frequency
based on display planes' properties (this was my initial suggestion) 
but sclk_fimd parent should be fixed either by bootloader or board
initialization code.