linux-kernel - RE: [PATCH] arm: da850: change ASYNC/PLL0

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <A73F36158E33644199EB82C5EC81C7BC317A4A97@DBDE01.ent.ti.com>
Date:	Thu, 5 Apr 2012 06:38:39 +0000
From:	"Manjunathappa, Prakash" <prakash.pm@...com>
To:	Michael Williamson <michael.williamson@...ticallink.com>
CC:	"davinci-linux-open-source@...ux.davincidsp.com" 
	<davinci-linux-open-source@...ux.davincidsp.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH] arm: da850: change ASYNC/PLL0_SYSCLK3 clock rate with
 DVFS

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@...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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/