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

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

[Sanjeev Premi]

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

Signed-off-by: Sanjeev Premi <premi@ti.com>
Signed-off-by: Hiremath Vaibhav <hvaibhav@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)
+		 */
+		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);
+
 	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;
+	}
+
 	/* based on number of ticks assign speed */
 	if (cdiff > 19000)
 		return S38_4M;
-- 
1.6.2.2

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

[Tom]

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 ?

> 
> Signed-off-by: Sanjeev Premi <premi@ti.com>
> Signed-off-by: Hiremath Vaibhav <hvaibhav@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

[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@ti.com>
> > Signed-off-by: Hiremath Vaibhav <hvaibhav@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
> 

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

[Tom]

Premi, Sanjeev wrote:
>> -----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.
> 

This is a good explanation.
Please include this in the commit log of the next (cleaned up for ws)
version of this patch.

Since this is a clock change, it would be good to have it tested
on as many boards as possible.

Thanks
Tom

> Best regards,
> Sanjeev
> 
>>> Signed-off-by: Sanjeev Premi <premi@ti.com>
>>> Signed-off-by: Hiremath Vaibhav <hvaibhav@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

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

[Paulraj, Sandeep]

> 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
> 
> Signed-off-by: Sanjeev Premi <premi@ti.com>
> Signed-off-by: Hiremath Vaibhav <hvaibhav@ti.com>
> ---


Pushed to u-boot-ti.

Thanks,
Sandeep