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

* Re: Re: [PATCH V5 5/5] ARM: EXYNOS4: Add platform data for EXYNOS4 FIMD and LTE480WV platform-lcd
  2011-06-22  6:41 ` Jingoo Han
@ 2011-06-23  1:09 ` JinGoo Han
  -1 siblings, 0 replies; 14+ messages in thread
From: JinGoo Han @ 2011-06-23  1:09 UTC (permalink / raw)
  To: ANAND KUMAR N, Marek Szyprowski
  Cc: JinGoo Han, Kukjin Kim, Paul Mundt, linux-samsung-soc,
	linux-fbdev, Jong-Hun Han, THOMAS P ABRAHAM, Sylwester Nawrocki,
	Kyungmin Park, In-Ki Dae, ARM Linux, Ben Dooks

Hi, Anand.
> -----Original Message-----
> From: linux-samsung-soc-owner@vger.kernel.org [mailto:linux-samsung-soc-
> owner@vger.kernel.org] On Behalf Of Anand Kumar N
> Sent: Wednesday, June 22, 2011 10:25 PM
> To: Marek Szyprowski
> Cc: Jingoo Han; Kukjin Kim; Paul Mundt; linux-samsung-soc@vger.kernel.org;
> linux-fbdev@vger.kernel.org; Jonghun Han; Thomas Abraham; Sylwester
> Nawrocki; Kyungmin Park; Inki Dae; ARM Linux; Ben Dooks
> Subject: 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
> <m.szyprowski@samsung.com> wrote:
> > Hello,
> >
> > On Wednesday, June 22, 2011 8:42 AM Jingoo Han wrote:
> >
> >> From: Jonghun Han <jonghun.han@samsung.com>
> (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,
> +};

As Sylwester Nawrocki mentioned, there would be criteria that should be considered
such as continuous clock availability or frequency stability. 
For instance, when EPLL '[7] = &clk_mout_epll.clk,' is selected, there is posibility
that EPLL frequency will be changed by other devices.
However, Exynos4 FIMD can use a range of clocks as source clock, 'mout_mpll' is
most useful  and can cover almost LCD pixel clock, because 'mout_mpll' is 800 MHz
and max frequency of 'sclk_fimd' can be set as 800MHz.

However, other clocks are very low. For example, HDMI PHY, VPLL are 54MHz, 
USB PHY is 48MHz. 

> 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.
> 
> >> +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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-
> soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-i

^ permalink raw reply	[flat|nested] 14+ messages in thread