OMAP3: Avoid re-write to PRM_CLKSRC_CTRL

PATCH Superseded Sanjeev Premi 3 participants 5 messages Jan 27, 2010 – Feb 8, 2010

Share & tools

Sanjeev Premi

author

2010-01-27 10:46 UTC #0 lore raw

PATCH 1/1 OMAP3: Avoid re-write to PRM_CLKSRC_CTRL

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 <[email protected]>

Signed-off-by Hiremath Vaibhav <[email protected]>

---
 cpu/arm_cortexa8/omap3/clock.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)

cpu/arm_cortexa8/omap3/clock.c +16 -4

index 174c453..6330c9e 100644
@@ -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;

Signature · 1 line

1.6.2.2

Tom

2010-01-27 13:45 UTC #1 lore raw

PATCH 1/1 OMAP3: Avoid re-write to PRM_CLKSRC_CTRL

Quoted 4 lines · 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.

Quoted 8 lines

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 ?

Quoted 25 lines

Signed-off-by Sanjeev Premi <[email protected]>

Signed-off-by Hiremath Vaibhav <[email protected]>

---
 cpu/arm_cortexa8/omap3/clock.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)

cpu/arm_cortexa8/omap3/clock.c +1 -2

index 174c453..6330c9e 100644
@@ -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 */

cpu/arm_cortexa8/omap3/clock.c u32 get_osc_clk_speed(void) · line 58

Quoted 10 lines

cpu/arm_cortexa8/omap3/clock.c +8 -2

-	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.

cpu/arm_cortexa8/omap3/clock.c u32 get_osc_clk_speed(void) · line 70

Quoted 9 lines

cpu/arm_cortexa8/omap3/clock.c +2 -0

+		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.

cpu/arm_cortexa8/omap3/clock.c u32 get_osc_clk_speed(void) · line 97

Quoted 12 lines

cpu/arm_cortexa8/omap3/clock.c +5 -0

 	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

cpu/arm_cortexa8/omap3/clock.c u32 get_osc_clk_speed(void) · line 100

Quoted 3 lines

cpu/arm_cortexa8/omap3/clock.c +0 -0

 	/* based on number of ticks assign speed */
 	if (cdiff > 19000)
 		return S38_4M;

Tom

Premi

2010-01-27 16:40 UTC #2 lore raw

PATCH 1/1 OMAP3: Avoid re-write to PRM_CLKSRC_CTRL

Quoted 18 lines

-----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
Quoted 10 lines · 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.

Quoted 8 lines

Quoted 2 lines
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

Quoted 76 lines

Review
Quoted 9 lines
Signed-off-by Sanjeev Premi <[email protected]>
Signed-off-by Hiremath Vaibhav <[email protected]>
---
 cpu/arm_cortexa8/omap3/clock.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)
(quoted diff) +0 -0
b/cpu/arm_cortexa8/omap3/clock.c
cpu/arm_cortexa8/omap3/clock.c line 40
Quoted 5 lines
index 174c453..6330c9e 100644
cpu/arm_cortexa8/omap3/clock.c +0 -0
@@ -40,7 +40,7 @@
 
**************************************************************
***************/
Quoted 12 lines
cpu/arm_cortexa8/omap3/clock.c +1 -2
 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 */
cpu/arm_cortexa8/omap3/clock.c u32 get_osc_clk_speed(void) · line 58
Quoted 10 lines
cpu/arm_cortexa8/omap3/clock.c +8 -2
-	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.
cpu/arm_cortexa8/omap3/clock.c u32 get_osc_clk_speed(void) · line 70
Quoted 9 lines
cpu/arm_cortexa8/omap3/clock.c +2 -0
+		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.
cpu/arm_cortexa8/omap3/clock.c u32 get_osc_clk_speed(void) · line 90
Quoted 5 lines
cpu/arm_cortexa8/omap3/clock.c +0 -0
 	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 */
cpu/arm_cortexa8/omap3/clock.c u32 get_osc_clk_speed(void) · line 97
Quoted 7 lines
cpu/arm_cortexa8/omap3/clock.c +5 -0
 	cdiff = cend - cstart;			/* get elapsed ticks */
 
+	if (cdiv == 2)
+	{
+		cdiff *= 2;
+	}
+
Braces not needed
cpu/arm_cortexa8/omap3/clock.c u32 get_osc_clk_speed(void) · line 100
Quoted 3 lines
cpu/arm_cortexa8/omap3/clock.c +0 -0
 	/* based on number of ticks assign speed */
 	if (cdiff > 19000)
 		return S38_4M;
Tom

Tom

2010-01-28 14:02 UTC #3 lore raw

PATCH 1/1 OMAP3: Avoid re-write to PRM_CLKSRC_CTRL

Quoted 46 lines · Sanjeev

Quoted 18 lines
-----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
Quoted 10 lines · 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.
Quoted 7 lines
Quoted 2 lines
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.

Quoted 7 lines

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

Quoted 74 lines

Best regards,
Sanjeev
Quoted 71 lines
Review
Quoted 7 lines
Signed-off-by Sanjeev Premi <[email protected]>
Signed-off-by Hiremath Vaibhav <[email protected]>
---
 cpu/arm_cortexa8/omap3/clock.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)
(quoted diff) +0 -0
b/cpu/arm_cortexa8/omap3/clock.c
cpu/arm_cortexa8/omap3/clock.c line 40
Quoted 5 lines
index 174c453..6330c9e 100644
cpu/arm_cortexa8/omap3/clock.c +0 -0
@@ -40,7 +40,7 @@
 
**************************************************************
***************/
Quoted 12 lines
cpu/arm_cortexa8/omap3/clock.c +1 -2
 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 */
cpu/arm_cortexa8/omap3/clock.c u32 get_osc_clk_speed(void) · line 58
Quoted 10 lines
cpu/arm_cortexa8/omap3/clock.c +8 -2
-	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.
cpu/arm_cortexa8/omap3/clock.c u32 get_osc_clk_speed(void) · line 70
Quoted 9 lines
cpu/arm_cortexa8/omap3/clock.c +2 -0
+		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.
cpu/arm_cortexa8/omap3/clock.c u32 get_osc_clk_speed(void) · line 90
Quoted 5 lines
cpu/arm_cortexa8/omap3/clock.c +0 -0
 	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 */
cpu/arm_cortexa8/omap3/clock.c u32 get_osc_clk_speed(void) · line 97
Quoted 7 lines
cpu/arm_cortexa8/omap3/clock.c +5 -0
 	cdiff = cend - cstart;			/* get elapsed ticks */
 
+	if (cdiv == 2)
+	{
+		cdiff *= 2;
+	}
+
Braces not needed
cpu/arm_cortexa8/omap3/clock.c u32 get_osc_clk_speed(void) · line 100
Quoted 3 lines
cpu/arm_cortexa8/omap3/clock.c +0 -0
 	/* based on number of ticks assign speed */
 	if (cdiff > 19000)
 		return S38_4M;
Tom

Paulraj

2010-02-08 16:38 UTC #4 lore raw

PATCH 1/1 OMAP3: Avoid re-write to PRM_CLKSRC_CTRL

In reply to Sanjeev Premi

Quoted 12 lines

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.

Quoted 6 lines

The patch has been tested with OMAP3530 on OMAP3EVM as well
Signed-off-by Sanjeev Premi <[email protected]>
Signed-off-by Hiremath Vaibhav <[email protected]>
---

Pushed to u-boot-ti.

Thanks,
Sandeep