[U-Boot] [PATCH] OMAP3: Avoid re-write to PRM_CLKSRC_CTRL

[Premi, Sanjeev]

> -----Original Message-----
> From: Tom [mailto:Tom.Rix at windriver.com] 
> Sent: Wednesday, January 27, 2010 7:15 PM
> To: Premi, Sanjeev
> Cc: u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH] OMAP3: Avoid re-write to PRM_CLKSRC_CTRL
> 
> Sanjeev Premi wrote:
> > In function get_osc_clk_speed(), do not change/ update
> > the divider for SYS_CLK as it can has cascading effect
> > on the other derived clocks.
> > 
> > Sudden change in divider value can lead to inconsistent
> > behavior in the system - often leading to crashes.
> > 
> > The problem was found when working with OMAP3EVM using
> > DM3730 processor card.
> > 
> > The patch has been tested with OMAP3530 on OMAP3EVM as
> > well
> 
> I believe the intent of this function is only to be
> called once early in the clock setup when the
> other clocks have not been setup to determine emperically
> the master system clock.  Can you provide were else it is being
> called ?

Tom,

Yes, the intent of function is to determine the master clock.
But, by the time this function is executed, the divider has
a value. (Usually set by the x-loader for OMAP boards).

Before we hit this code, the some clocks are already derived
(either by default OR by x-loader). The function as it exists
Today, does not follow any sequence to contain the effect of
changing the divider.

In case of 3730 (similar to 3630) we are possibly overshooting
the tolerance zone. Using Lauterbach we can see that contents
of each window starts to 'auto-refresh' as soon as the divider
Changes - sometimes after restoring the divider. A clear
indication of system becoming unstable.

Of course, once in 4-5 times this issue was noticed in x-loader
as well which has almost same code for the purpose.

The intent of function is to determine the OSC frequency; which
can be determined without having to change the divider... by
multiplying by the same factor.

Overall, the empirical formula is same; but this patch ensures
that HW need not change for the calculations.

Best regards,
Sanjeev

> 
> > 
> > Signed-off-by: Sanjeev Premi <premi at ti.com>
> > Signed-off-by: Hiremath Vaibhav <hvaibhav at ti.com>
> > ---
> >  cpu/arm_cortexa8/omap3/clock.c |   20 ++++++++++++++++----
> >  1 files changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/cpu/arm_cortexa8/omap3/clock.c 
> b/cpu/arm_cortexa8/omap3/clock.c
> > index 174c453..6330c9e 100644
> > --- a/cpu/arm_cortexa8/omap3/clock.c
> > +++ b/cpu/arm_cortexa8/omap3/clock.c
> > @@ -40,7 +40,7 @@
> >   
> **************************************************************
> ***************/
> >  u32 get_osc_clk_speed(void)
> >  {
> > -	u32 start, cstart, cend, cdiff, val;
> > +	u32 start, cstart, cend, cdiff, cdiv, val;
> >  	struct prcm *prcm_base = (struct prcm *)PRCM_BASE;
> >  	struct prm *prm_base = (struct prm *)PRM_BASE;
> >  	struct gptimer *gpt1_base = (struct gptimer *)OMAP34XX_GPT1;
> > @@ -48,9 +48,15 @@ u32 get_osc_clk_speed(void)
> >  
> >  	val = readl(&prm_base->clksrc_ctrl);
> >  
> > -	/* If SYS_CLK is being divided by 2, remove for now */
> > -	val = (val & (~SYSCLKDIV_2)) | SYSCLKDIV_1;
> > -	writel(val, &prm_base->clksrc_ctrl);
> > +	if (val & SYSCLKDIV_2)
> > +		cdiv = 2;
> > +	else if (val & SYSCLKDIV_1)
> > +		cdiv = 1;
> > +	else
> > +		/*
> > +		 * Should never reach here! (Assume divider as 1)
> > +		 */
> Could reduce this to a single line,
> Good comment.
> 
> > +		cdiv = 1;
> >  
> >  	/* enable timer2 */
> >  	val = readl(&prcm_base->clksel_wkup) | CLKSEL_GPT1;
> > @@ -61,6 +67,7 @@ u32 get_osc_clk_speed(void)
> >  	/* Enable I and F Clocks for GPT1 */
> >  	val = readl(&prcm_base->iclken_wkup) | EN_GPT1 | EN_32KSYNC;
> >  	writel(val, &prcm_base->iclken_wkup);
> > +
> 
> Extra space.
> Remove.
> 
> >  	val = readl(&prcm_base->fclken_wkup) | EN_GPT1;
> >  	writel(val, &prcm_base->fclken_wkup);
> >  
> > @@ -83,6 +90,11 @@ u32 get_osc_clk_speed(void)
> >  	cend = readl(&gpt1_base->tcrr);		/* get end 
> sys_clk count */
> >  	cdiff = cend - cstart;			/* get elapsed ticks */
> >  
> > +	if (cdiv == 2)
> > +	{
> > +		cdiff *= 2;
> > +	}
> > +
> Braces not needed
> 
> >  	/* based on number of ticks assign speed */
> >  	if (cdiff > 19000)
> >  		return S38_4M;
> 
> Tom
>