lkml.org 
[lkml]   [2012]   [Apr]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH] arm: da850: change ASYNC/PLL0_SYSCLK3 clock rate with DVFS
Date
Hi,

On Tue, Apr 03, 2012 at 19:32:51, Michael Williamson wrote:
> On 4/3/2012 9:01 AM, Manjunathappa, Prakash wrote:
>
> > Clock for EMIF is derived from ASYNC clock domain(PLL0_SYSCLK3) and was
> > configured with fixed divider as there was no significant performance
> > degradation with existing NAND/NOR EMIF devices if it is not
> > reconfigured accordingly at different OPPs.
> >
> > On systems where devices other than NAND/NOR are interfaced through
> > EMIF, such performance degradation may not be desirable. So change the
> > PLL0_SYSCLK3 output frequency for different OPPs by re-configuring
> > the divider value.
> >
> > Configured values are as per rates specified in OMAP-L138 Data sheet
> > (http://www.ti.com/lit/ds/symlink/omap-l138.pdf, Table 5-5).
> >
> > Patch addresses concerns raised in below thread of Linux-DaVinci-community:
> > http://www.mail-archive.com/davinci-linux-open-source@linux.davincidsp.com/msg22535.html
> >
> > Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
> > ---
> > arch/arm/mach-davinci/da850.c | 37 +++++++++++++++++++++++++++++++++++--
> > 1 files changed, 35 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
> > index b44dc84..7e98ed7 100644
> > --- a/arch/arm/mach-davinci/da850.c
> > +++ b/arch/arm/mach-davinci/da850.c
> > @@ -49,6 +49,7 @@
> > static int da850_set_armrate(struct clk *clk, unsigned long rate);
> > static int da850_round_armrate(struct clk *clk, unsigned long rate);
> > static int da850_set_pll0rate(struct clk *clk, unsigned long armrate);
> > +static int da850_set_sysclk3_rate(struct clk *clk, unsigned long rate);
> >
> > static struct pll_data pll0_data = {
> > .num = 1,
> > @@ -88,8 +89,8 @@ static struct clk pll0_sysclk3 = {
> > .parent = &pll0_clk,
> > .flags = CLK_PLL,
> > .div_reg = PLLDIV3,
> > - .set_rate = davinci_set_sysclk_rate,
> > - .maxrate = 100000000,
> > + .set_rate = da850_set_sysclk3_rate,
>
>
>
> da850_set_pll0_sysclk3_rate ?
> (there is a pll1_sysclk3 as well)
>

Ok, I will rename it as .

> > + .maxrate = 148000000,
> > };
> >
> > static struct clk pll0_sysclk4 = {
> > @@ -1004,6 +1005,33 @@ static int da850_set_pll0rate(struct clk *clk, unsigned long index)
> >
> > return 0;
> > }
> > +
> > +static int da850_set_sysclk3_rate(struct clk *clk, unsigned long rate)
> > +{
> > + struct clk *arm_clk;
> > + unsigned long sys_clk3_rate = 148000000;
>
>
>
> why not sys_clk3_rate = rate? or just use rate?
>

Answered below.

> > + int ret;
> > +
> > + arm_clk = clk_get(NULL, "arm");
> > + if (WARN(IS_ERR(arm_clk), "Unable to get ARM clock\n"))
> > + return PTR_ERR(arm_clk);
> > +
> > + /* Set EMIF clock based on OPPs */
> > + switch (clk_get_rate(arm_clk)) {
> > + case 200000000:
> > + sys_clk3_rate = 75000000;
>
>
> what if the requested rate is lower than 75 MHz?
>

Neither EMIF driver or EMIF devices(NAND/NOR driver) does set rate, is only set via
cpu_freq driver whenever OPPs change.

> > + break;
> > + case 96000000:
> > + sys_clk3_rate = 50000000;
> > + break;
> > + }
> > +
> > + ret = davinci_set_sysclk_rate(clk, sys_clk3_rate);
> > + if (WARN_ON(ret))
> > + return ret;
> > +
> > + return 0;
> > +}
>
>
>
> This routine is ignoring the requested rate passed in. At 300 MHz OPP,
> you are going to configure it for 148 MHz instead of 100 MHz (the
> current implementation). I would think that this rate should
> be controllable by platform initialization code unless it needs limiting
> based on specification. What if someone wants to run the EMIFA
> slower than 148 MHz?
>
> There are platforms that would like to keep the EMIFA rate at a relatively
> fixed / stable value of, e.g., 100 MHz or as close to that value as possible
> at any OPP above 300 MHz. This patch does not appear support that.
>

As mentioned in commit message, patch is prepared to address below issues:
1) Fixing PLL0_SYSCLK3 rate at 100MHz leads to performance degradation on non
NAND/NOR devices on EMIF.
2) 100MHz rate is de-spec configuration at OPPs < 300MHz, as mentioned in OMAP-L138
Data sheet (http://www.ti.com/lit/ds/symlink/omap-l138.pdf, Table 5-5)

To support platforms requiring fixed clock rate, as suggested by you we can have
rate coming from platform file which can be configured via Kconfig option.

I will submit next version of patch which takes care of this.

Thanks,
Prakash


\
 
 \ /
  Last update: 2012-04-05 08:41    [W:0.052 / U:0.548 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site