[PATCH 0/2] mx25: clock related fixes

[PATCH 0/2] mx25: clock related fixes

[Baruch Siach]

These patches fix two issues related to the clock configuration in the i.MX25
platform code. In the coming days I'll work on a better fix for the i.MX25 
clock infrastructure.

Baruch Siach (2):
  mx25: remove unused mx25_clocks_init() argument
  mx25: properly initialize clocks, fix time accounting

 arch/arm/mach-mx25/clock.c              |   29 +++++++++++++++++++++++++++--
 arch/arm/mach-mx25/mx25pdk.c            |    2 +-
 arch/arm/plat-mxc/include/mach/common.h |    2 +-
 3 files changed, 29 insertions(+), 4 deletions(-)

[PATCH 1/2] mx25: remove unused mx25_clocks_init() argument

[Baruch Siach]

The fref is needless on mx25 since the reference clock is fixed at 24MHz.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 arch/arm/mach-mx25/clock.c              |    2 +-
 arch/arm/mach-mx25/mx25pdk.c            |    2 +-
 arch/arm/plat-mxc/include/mach/common.h |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
index 6e838b8..a4ba334 100644
--- a/arch/arm/mach-mx25/clock.c
+++ b/arch/arm/mach-mx25/clock.c
@@ -208,7 +208,7 @@ static struct clk_lookup lookups[] = {
 	_REGISTER_CLOCK("fec.0", NULL, fec_clk)
 };
 
-int __init mx25_clocks_init(unsigned long fref)
+int __init mx25_clocks_init(void)
 {
 	int i;
 
diff --git a/arch/arm/mach-mx25/mx25pdk.c b/arch/arm/mach-mx25/mx25pdk.c
index c8b1d3b..232f9ca 100644
--- a/arch/arm/mach-mx25/mx25pdk.c
+++ b/arch/arm/mach-mx25/mx25pdk.c
@@ -98,7 +98,7 @@ static void __init mx25pdk_init(void)
 
 static void __init mx25pdk_timer_init(void)
 {
-	mx25_clocks_init(26000000);
+	mx25_clocks_init();
 }
 
 static struct sys_timer mx25pdk_timer = {
diff --git a/arch/arm/plat-mxc/include/mach/common.h b/arch/arm/plat-mxc/include/mach/common.h
index 286cb9b..4bf1068 100644
--- a/arch/arm/plat-mxc/include/mach/common.h
+++ b/arch/arm/plat-mxc/include/mach/common.h
@@ -32,7 +32,7 @@ extern void mxc91231_init_irq(void);
 extern void mxc_timer_init(struct clk *timer_clk, void __iomem *, int);
 extern int mx1_clocks_init(unsigned long fref);
 extern int mx21_clocks_init(unsigned long lref, unsigned long fref);
-extern int mx25_clocks_init(unsigned long fref);
+extern int mx25_clocks_init(void);
 extern int mx27_clocks_init(unsigned long fref);
 extern int mx31_clocks_init(unsigned long fref);
 extern int mx35_clocks_init(void);
-- 
1.6.5

[PATCH 2/2] mx25: properly initialize clocks, fix time accounting

[Baruch Siach]

For some odd reason the GPT module has run the timer counter at 100MHz, while
the rate reported by the gpt_clk has been 133MHz. This caused a significant
drift in time accounting.

This patch disables all unnecessary clock in mx25_clocks_init() to make a clean
start, the same as is being done for the rest of the i.MX chips.

To preserve the uart and fec functionality, this patch temporarily enables the
respective clocks in mx25_clocks_init(). The real fix for this is to use the
.secondary field of the mxc clk struct, and enable these clocks from there.

This patch was tested on i.MX25 PDK.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 arch/arm/mach-mx25/clock.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
index a4ba334..b1aadd5 100644
--- a/arch/arm/mach-mx25/clock.c
+++ b/arch/arm/mach-mx25/clock.c
@@ -119,6 +119,11 @@ static unsigned long get_rate_nfc(struct clk *clk)
 	return get_rate_per(8);
 }
 
+static unsigned long get_rate_gpt(struct clk *clk)
+{
+	return get_rate_per(5);
+}
+
 static unsigned long get_rate_otg(struct clk *clk)
 {
 	return 48000000; /* FIXME */
@@ -155,7 +160,7 @@ static void clk_cgcr_disable(struct clk *clk)
 		.disable	= clk_cgcr_disable,	\
 	}
 
-DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_ipg, NULL);
+DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_gpt, NULL);
 DEFINE_CLOCK(cspi1_clk,  0, CCM_CGCR1,  5, get_rate_ipg, NULL);
 DEFINE_CLOCK(cspi2_clk,  0, CCM_CGCR1,  6, get_rate_ipg, NULL);
 DEFINE_CLOCK(cspi3_clk,  0, CCM_CGCR1,  7, get_rate_ipg, NULL);
@@ -215,6 +220,26 @@ int __init mx25_clocks_init(void)
 	for (i = 0; i < ARRAY_SIZE(lookups); i++)
 		clkdev_add(&lookups[i]);
 
+	/* Turn off all clocks except the ones we need to survive, namely:
+	 * EMI, GPIO1-3 (CCM_CGCR1[18:16]), GPT1, IOMUXC (CCM_CGCR1[27]), IIM,
+	 * SCC
+	 */
+	__raw_writel((1 << 19), CRM_BASE + CCM_CGCR0);
+	__raw_writel((0xf << 16) | (3 << 26), CRM_BASE + CCM_CGCR1);
+	__raw_writel((1 << 5), CRM_BASE + CCM_CGCR2);
+
+	/* Enable FEC IPG
+	 * FIXME: implement secondary clock
+	 */
+	__raw_writel(__raw_readl(CRM_BASE + CCM_CGCR1) | (1 << 15),
+			CRM_BASE + CCM_CGCR1);
+
+	/* Enable uart PER
+	 * FIXME: implement secondary clock
+	 */
+	__raw_writel(__raw_readl(CRM_BASE + CCM_CGCR0) | (1 << 15),
+			CRM_BASE + CCM_CGCR0);
+
 	mxc_timer_init(&gpt_clk, MX25_IO_ADDRESS(MX25_GPT1_BASE_ADDR), 54);
 
 	return 0;
-- 
1.6.5

[PATCH 2/2] mx25: properly initialize clocks, fix time accounting

[Baruch Siach]

Hi Sascha,

On Thu, Jan 21, 2010 at 05:00:23PM +0200, Baruch Siach wrote:
> For some odd reason the GPT module has run the timer counter at 100MHz, while
> the rate reported by the gpt_clk has been 133MHz. This caused a significant
> drift in time accounting.

I probably miscalculated the drift. The real rate of gpt_clk was, apparently, 
66.5MHz, so get_rate_gpt() is all that's needed to fix time accounting drift.  
I'll test and resend this patch next week, and then go on to fix the clock 
initialization of the mx25.

baruch

> This patch disables all unnecessary clock in mx25_clocks_init() to make a clean
> start, the same as is being done for the rest of the i.MX chips.
> 
> To preserve the uart and fec functionality, this patch temporarily enables the
> respective clocks in mx25_clocks_init(). The real fix for this is to use the
> .secondary field of the mxc clk struct, and enable these clocks from there.
> 
> This patch was tested on i.MX25 PDK.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  arch/arm/mach-mx25/clock.c |   27 ++++++++++++++++++++++++++-
>  1 files changed, 26 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
> index a4ba334..b1aadd5 100644
> --- a/arch/arm/mach-mx25/clock.c
> +++ b/arch/arm/mach-mx25/clock.c
> @@ -119,6 +119,11 @@ static unsigned long get_rate_nfc(struct clk *clk)
>  	return get_rate_per(8);
>  }
>  
> +static unsigned long get_rate_gpt(struct clk *clk)
> +{
> +	return get_rate_per(5);
> +}
> +
>  static unsigned long get_rate_otg(struct clk *clk)
>  {
>  	return 48000000; /* FIXME */
> @@ -155,7 +160,7 @@ static void clk_cgcr_disable(struct clk *clk)
>  		.disable	= clk_cgcr_disable,	\
>  	}
>  
> -DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_ipg, NULL);
> +DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_gpt, NULL);
>  DEFINE_CLOCK(cspi1_clk,  0, CCM_CGCR1,  5, get_rate_ipg, NULL);
>  DEFINE_CLOCK(cspi2_clk,  0, CCM_CGCR1,  6, get_rate_ipg, NULL);
>  DEFINE_CLOCK(cspi3_clk,  0, CCM_CGCR1,  7, get_rate_ipg, NULL);
> @@ -215,6 +220,26 @@ int __init mx25_clocks_init(void)
>  	for (i = 0; i < ARRAY_SIZE(lookups); i++)
>  		clkdev_add(&lookups[i]);
>  
> +	/* Turn off all clocks except the ones we need to survive, namely:
> +	 * EMI, GPIO1-3 (CCM_CGCR1[18:16]), GPT1, IOMUXC (CCM_CGCR1[27]), IIM,
> +	 * SCC
> +	 */
> +	__raw_writel((1 << 19), CRM_BASE + CCM_CGCR0);
> +	__raw_writel((0xf << 16) | (3 << 26), CRM_BASE + CCM_CGCR1);
> +	__raw_writel((1 << 5), CRM_BASE + CCM_CGCR2);
> +
> +	/* Enable FEC IPG
> +	 * FIXME: implement secondary clock
> +	 */
> +	__raw_writel(__raw_readl(CRM_BASE + CCM_CGCR1) | (1 << 15),
> +			CRM_BASE + CCM_CGCR1);
> +
> +	/* Enable uart PER
> +	 * FIXME: implement secondary clock
> +	 */
> +	__raw_writel(__raw_readl(CRM_BASE + CCM_CGCR0) | (1 << 15),
> +			CRM_BASE + CCM_CGCR0);
> +
>  	mxc_timer_init(&gpt_clk, MX25_IO_ADDRESS(MX25_GPT1_BASE_ADDR), 54);
>  
>  	return 0;
> -- 
> 1.6.5
> 

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 2/2] mx25: properly initialize clocks, fix time accounting

[Sascha Hauer]

On Thu, Jan 21, 2010 at 05:00:23PM +0200, Baruch Siach wrote:
> For some odd reason the GPT module has run the timer counter at 100MHz, while
> the rate reported by the gpt_clk has been 133MHz. This caused a significant
> drift in time accounting.
> 
> This patch disables all unnecessary clock in mx25_clocks_init() to make a clean
> start, the same as is being done for the rest of the i.MX chips.
> 
> To preserve the uart and fec functionality, this patch temporarily enables the
> respective clocks in mx25_clocks_init(). The real fix for this is to use the
> .secondary field of the mxc clk struct, and enable these clocks from there.

Then lets implement it the real way, it's not too hard. The following
two patches should do it. It's compile tested only, can you test them on
hardware please? The patches do not include your timer rate fixup and
clock disabling fix.

> 
> This patch was tested on i.MX25 PDK.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  arch/arm/mach-mx25/clock.c |   27 ++++++++++++++++++++++++++-
>  1 files changed, 26 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
> index a4ba334..b1aadd5 100644
> --- a/arch/arm/mach-mx25/clock.c
> +++ b/arch/arm/mach-mx25/clock.c
> @@ -119,6 +119,11 @@ static unsigned long get_rate_nfc(struct clk *clk)
>  	return get_rate_per(8);
>  }
>  
> +static unsigned long get_rate_gpt(struct clk *clk)
> +{
> +	return get_rate_per(5);
> +}
> +
>  static unsigned long get_rate_otg(struct clk *clk)
>  {
>  	return 48000000; /* FIXME */
> @@ -155,7 +160,7 @@ static void clk_cgcr_disable(struct clk *clk)
>  		.disable	= clk_cgcr_disable,	\
>  	}
>  
> -DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_ipg, NULL);
> +DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_gpt, NULL);
>  DEFINE_CLOCK(cspi1_clk,  0, CCM_CGCR1,  5, get_rate_ipg, NULL);
>  DEFINE_CLOCK(cspi2_clk,  0, CCM_CGCR1,  6, get_rate_ipg, NULL);
>  DEFINE_CLOCK(cspi3_clk,  0, CCM_CGCR1,  7, get_rate_ipg, NULL);
> @@ -215,6 +220,26 @@ int __init mx25_clocks_init(void)
>  	for (i = 0; i < ARRAY_SIZE(lookups); i++)
>  		clkdev_add(&lookups[i]);
>  
> +	/* Turn off all clocks except the ones we need to survive, namely:
> +	 * EMI, GPIO1-3 (CCM_CGCR1[18:16]), GPT1, IOMUXC (CCM_CGCR1[27]), IIM,
> +	 * SCC
> +	 */
> +	__raw_writel((1 << 19), CRM_BASE + CCM_CGCR0);
> +	__raw_writel((0xf << 16) | (3 << 26), CRM_BASE + CCM_CGCR1);
> +	__raw_writel((1 << 5), CRM_BASE + CCM_CGCR2);

Disabling the unused clocks here is a good thing. Can you add this for
the early debug case:

#if defined(CONFIG_DEBUG_LL) && !defined(CONFIG_DEBUG_ICEDCC)
	clk_enable(&uart1_clk);
#endif

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 2/2] mx25: properly initialize clocks, fix time accounting

[Sascha Hauer]

[PATCH 2/2] mx25: properly initialize clocks, fix time accounting

[Sascha Hauer]

[PATCH 2/2] mx25: properly initialize clocks, fix time accounting

[Baruch Siach]

Hi Sascha,

On Fri, Jan 22, 2010 at 08:57:33AM +0100, Sascha Hauer wrote:
> On Thu, Jan 21, 2010 at 05:00:23PM +0200, Baruch Siach wrote:
> > For some odd reason the GPT module has run the timer counter at 100MHz, while
> > the rate reported by the gpt_clk has been 133MHz. This caused a significant
> > drift in time accounting.
> > 
> > This patch disables all unnecessary clock in mx25_clocks_init() to make a clean
> > start, the same as is being done for the rest of the i.MX chips.
> > 
> > To preserve the uart and fec functionality, this patch temporarily enables the
> > respective clocks in mx25_clocks_init(). The real fix for this is to use the
> > .secondary field of the mxc clk struct, and enable these clocks from there.
> 
> Then lets implement it the real way, it's not too hard. The following
> two patches should do it. It's compile tested only, can you test them on
> hardware please? The patches do not include your timer rate fixup and
> clock disabling fix.

Your .secondary implementation works like a charm on the i.MX25 PDK.

My following patches add the fref removal, clock disabling, and the time 
accounting fix. Another patch reverses the primary/secondary ordering of the 
fec clock to make it consistent with the uart clock.

Let me know what you think.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 1/4] mx25: remove unused mx25_clocks_init() argument

[Baruch Siach]

The fref is needless on mx25 since the reference clock is fixed at 24MHz.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 arch/arm/mach-mx25/clock.c              |    2 +-
 arch/arm/mach-mx25/mx25pdk.c            |    2 +-
 arch/arm/plat-mxc/include/mach/common.h |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
index 3be51dd..abd303b 100644
--- a/arch/arm/mach-mx25/clock.c
+++ b/arch/arm/mach-mx25/clock.c
@@ -211,7 +211,7 @@ static struct clk_lookup lookups[] = {
 	_REGISTER_CLOCK("fec.0", NULL, fec_clk)
 };
 
-int __init mx25_clocks_init(unsigned long fref)
+int __init mx25_clocks_init(void)
 {
 	int i;
 
diff --git a/arch/arm/mach-mx25/mx25pdk.c b/arch/arm/mach-mx25/mx25pdk.c
index c8b1d3b..232f9ca 100644
--- a/arch/arm/mach-mx25/mx25pdk.c
+++ b/arch/arm/mach-mx25/mx25pdk.c
@@ -98,7 +98,7 @@ static void __init mx25pdk_init(void)
 
 static void __init mx25pdk_timer_init(void)
 {
-	mx25_clocks_init(26000000);
+	mx25_clocks_init();
 }
 
 static struct sys_timer mx25pdk_timer = {
diff --git a/arch/arm/plat-mxc/include/mach/common.h b/arch/arm/plat-mxc/include/mach/common.h
index 286cb9b..4bf1068 100644
--- a/arch/arm/plat-mxc/include/mach/common.h
+++ b/arch/arm/plat-mxc/include/mach/common.h
@@ -32,7 +32,7 @@ extern void mxc91231_init_irq(void);
 extern void mxc_timer_init(struct clk *timer_clk, void __iomem *, int);
 extern int mx1_clocks_init(unsigned long fref);
 extern int mx21_clocks_init(unsigned long lref, unsigned long fref);
-extern int mx25_clocks_init(unsigned long fref);
+extern int mx25_clocks_init(void);
 extern int mx27_clocks_init(unsigned long fref);
 extern int mx31_clocks_init(unsigned long fref);
 extern int mx35_clocks_init(void);
-- 
1.6.5

[PATCH 2/4] mx25: properly initialize clocks

[Baruch Siach]

This patch disables all unnecessary clock in mx25_clocks_init() to make a clean
start, the same as is being done for the rest of the i.MX chips.

This patch was tested on i.MX25 PDK.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 arch/arm/mach-mx25/clock.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
index abd303b..08aaa38 100644
--- a/arch/arm/mach-mx25/clock.c
+++ b/arch/arm/mach-mx25/clock.c
@@ -218,6 +218,14 @@ int __init mx25_clocks_init(void)
 	for (i = 0; i < ARRAY_SIZE(lookups); i++)
 		clkdev_add(&lookups[i]);
 
+	/* Turn off all clocks except the ones we need to survive, namely:
+	 * EMI, GPIO1-3 (CCM_CGCR1[18:16]), GPT1, IOMUXC (CCM_CGCR1[27]), IIM,
+	 * SCC
+	 */
+	__raw_writel((1 << 19), CRM_BASE + CCM_CGCR0);
+	__raw_writel((0xf << 16) | (3 << 26), CRM_BASE + CCM_CGCR1);
+	__raw_writel((1 << 5), CRM_BASE + CCM_CGCR2);
+
 	mxc_timer_init(&gpt_clk, MX25_IO_ADDRESS(MX25_GPT1_BASE_ADDR), 54);
 
 	return 0;
-- 
1.6.5

[PATCH 3/4] mx25: fix time accounting

[Baruch Siach]

The gpt_clk rate function doesn't consider the PER divider. This causes a
significant drift in time accounting. Fix this by introducing the correct rate
calculation function.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 arch/arm/mach-mx25/clock.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
index 08aaa38..c003ac4 100644
--- a/arch/arm/mach-mx25/clock.c
+++ b/arch/arm/mach-mx25/clock.c
@@ -119,6 +119,11 @@ static unsigned long get_rate_nfc(struct clk *clk)
 	return get_rate_per(8);
 }
 
+static unsigned long get_rate_gpt(struct clk *clk)
+{
+	return get_rate_per(5);
+}
+
 static unsigned long get_rate_otg(struct clk *clk)
 {
 	return 48000000; /* FIXME */
@@ -156,7 +161,7 @@ static void clk_cgcr_disable(struct clk *clk)
 		.secondary	= s,			\
 	}
 
-DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_ipg, NULL, NULL);
+DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_gpt, NULL, NULL);
 DEFINE_CLOCK(uart_per_clk, 0, CCM_CGCR0, 15, get_rate_uart, NULL, NULL);
 DEFINE_CLOCK(cspi1_clk,  0, CCM_CGCR1,  5, get_rate_ipg, NULL, NULL);
 DEFINE_CLOCK(cspi2_clk,  0, CCM_CGCR1,  6, get_rate_ipg, NULL, NULL);
-- 
1.6.5

[PATCH 4/4] mx25: make the FEC AHB clk secondary of the IPG

[Baruch Siach]

This makes the FEC clock configuration consistent with the UART one.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 arch/arm/mach-mx25/clock.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
index c003ac4..6acc88b 100644
--- a/arch/arm/mach-mx25/clock.c
+++ b/arch/arm/mach-mx25/clock.c
@@ -166,7 +166,7 @@ DEFINE_CLOCK(uart_per_clk, 0, CCM_CGCR0, 15, get_rate_uart, NULL, NULL);
 DEFINE_CLOCK(cspi1_clk,  0, CCM_CGCR1,  5, get_rate_ipg, NULL, NULL);
 DEFINE_CLOCK(cspi2_clk,  0, CCM_CGCR1,  6, get_rate_ipg, NULL, NULL);
 DEFINE_CLOCK(cspi3_clk,  0, CCM_CGCR1,  7, get_rate_ipg, NULL, NULL);
-DEFINE_CLOCK(fec_ipg_clk, 0, CCM_CGCR1, 15, get_rate_ipg, NULL, NULL);
+DEFINE_CLOCK(fec_ahb_clk, 0, CCM_CGCR0, 23, NULL,	 NULL, NULL);
 DEFINE_CLOCK(uart1_clk,  0, CCM_CGCR2, 14, get_rate_uart, NULL, &uart_per_clk);
 DEFINE_CLOCK(uart2_clk,  0, CCM_CGCR2, 15, get_rate_uart, NULL, &uart_per_clk);
 DEFINE_CLOCK(uart3_clk,  0, CCM_CGCR2, 16, get_rate_uart, NULL, &uart_per_clk);
@@ -181,7 +181,7 @@ DEFINE_CLOCK(pwm4_clk,	 0, CCM_CGCR2,  2, get_rate_ipg, NULL, NULL);
 DEFINE_CLOCK(kpp_clk,	 0, CCM_CGCR1, 28, get_rate_ipg, NULL, NULL);
 DEFINE_CLOCK(tsc_clk,	 0, CCM_CGCR2, 13, get_rate_ipg, NULL, NULL);
 DEFINE_CLOCK(i2c_clk,	 0, CCM_CGCR0,  6, get_rate_i2c, NULL, NULL);
-DEFINE_CLOCK(fec_clk,	 0, CCM_CGCR0, 23, get_rate_ipg, NULL, &fec_ipg_clk);
+DEFINE_CLOCK(fec_clk,	 0, CCM_CGCR1, 15, get_rate_ipg, NULL, &fec_ahb_clk);
 
 #define _REGISTER_CLOCK(d, n, c)	\
 	{				\
-- 
1.6.5

[PATCH 3/4] mx25: fix time accounting

[Lothar Waßmann]

Hi,

Baruch Siach writes:
> The gpt_clk rate function doesn't consider the PER divider. This causes a
> significant drift in time accounting. Fix this by introducing the correct rate
> calculation function.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  arch/arm/mach-mx25/clock.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
> index 08aaa38..c003ac4 100644
> --- a/arch/arm/mach-mx25/clock.c
> +++ b/arch/arm/mach-mx25/clock.c
> @@ -119,6 +119,11 @@ static unsigned long get_rate_nfc(struct clk *clk)
>  	return get_rate_per(8);
>  }
>  
> +static unsigned long get_rate_gpt(struct clk *clk)
> +{
> +	return get_rate_per(5);
> +}
> +
>  static unsigned long get_rate_otg(struct clk *clk)
>  {
>  	return 48000000; /* FIXME */
> @@ -156,7 +161,7 @@ static void clk_cgcr_disable(struct clk *clk)
>  		.secondary	= s,			\
>  	}
>  
> -DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_ipg, NULL, NULL);
> +DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_gpt, NULL, NULL);
>  DEFINE_CLOCK(uart_per_clk, 0, CCM_CGCR0, 15, get_rate_uart, NULL, NULL);
>  DEFINE_CLOCK(cspi1_clk,  0, CCM_CGCR1,  5, get_rate_ipg, NULL, NULL);
>  DEFINE_CLOCK(cspi2_clk,  0, CCM_CGCR1,  6, get_rate_ipg, NULL, NULL);
>
IMO all those clocks should have the appropriate parent clocks, which
would fix the problem with the GPT clock rate without the need for a
special get_rate_*() functions and woould also make sure the parent
clock is enabled when the child clock is being enabled:
|DEFINE_CLOCK(gpt1_clk1,   0, CCM_CGCR1, 19, NULL, NULL, &ipg_clk);
|DEFINE_CLOCK(gpt1_clk,    0, CCM_CGCR0,  5, NULL, &gpt1_clk1, &per_clk5);
...


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH 3/4] mx25: fix time accounting

[Baruch Siach]

Hi Lothar,

On Mon, Jan 25, 2010 at 12:11:06PM +0100, Lothar Wa?mann wrote:
> Baruch Siach writes:

[snip]

> > -DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_ipg, NULL, NULL);
> > +DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_gpt, NULL, NULL);
> >  DEFINE_CLOCK(uart_per_clk, 0, CCM_CGCR0, 15, get_rate_uart, NULL, NULL);
> >  DEFINE_CLOCK(cspi1_clk,  0, CCM_CGCR1,  5, get_rate_ipg, NULL, NULL);
> >  DEFINE_CLOCK(cspi2_clk,  0, CCM_CGCR1,  6, get_rate_ipg, NULL, NULL);
> >
> IMO all those clocks should have the appropriate parent clocks, which
> would fix the problem with the GPT clock rate without the need for a
> special get_rate_*() functions and woould also make sure the parent
> clock is enabled when the child clock is being enabled:
> |DEFINE_CLOCK(gpt1_clk1,   0, CCM_CGCR1, 19, NULL, NULL, &ipg_clk);
> |DEFINE_CLOCK(gpt1_clk,    0, CCM_CGCR0,  5, NULL, &gpt1_clk1, &per_clk5);

The i.MX25 clock configuration infrastructure clearly needs more work. This 
patch only fixes a critical bug while doing the minimal possible change.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 3/4] mx25: fix time accounting

[Sascha Hauer]

Baruch,

On Mon, Jan 25, 2010 at 12:58:21PM +0200, Baruch Siach wrote:
> The gpt_clk rate function doesn't consider the PER divider. This causes a
> significant drift in time accounting. Fix this by introducing the correct rate
> calculation function.

Should have tested this one. In fact with this patch applied my clock
goes wrong.

The i.MX Timer code makes sure the gpt clock is sourced from the ipg
clock (GPTCR[6:8] = 1), so the behaviour should be correct the way it
was before this patch. Any idea why it was wrong on your hardware? Have
you changed the GPTCR bits?

Sascha

> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  arch/arm/mach-mx25/clock.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
> index 08aaa38..c003ac4 100644
> --- a/arch/arm/mach-mx25/clock.c
> +++ b/arch/arm/mach-mx25/clock.c
> @@ -119,6 +119,11 @@ static unsigned long get_rate_nfc(struct clk *clk)
>  	return get_rate_per(8);
>  }
>  
> +static unsigned long get_rate_gpt(struct clk *clk)
> +{
> +	return get_rate_per(5);
> +}
> +
>  static unsigned long get_rate_otg(struct clk *clk)
>  {
>  	return 48000000; /* FIXME */
> @@ -156,7 +161,7 @@ static void clk_cgcr_disable(struct clk *clk)
>  		.secondary	= s,			\
>  	}
>  
> -DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_ipg, NULL, NULL);
> +DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_gpt, NULL, NULL);
>  DEFINE_CLOCK(uart_per_clk, 0, CCM_CGCR0, 15, get_rate_uart, NULL, NULL);
>  DEFINE_CLOCK(cspi1_clk,  0, CCM_CGCR1,  5, get_rate_ipg, NULL, NULL);
>  DEFINE_CLOCK(cspi2_clk,  0, CCM_CGCR1,  6, get_rate_ipg, NULL, NULL);
> -- 
> 1.6.5
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 3/4] mx25: fix time accounting

[Baruch Siach]

Hi Sascha,

On Tue, May 11, 2010 at 05:43:49PM +0200, Sascha Hauer wrote:
> On Mon, Jan 25, 2010 at 12:58:21PM +0200, Baruch Siach wrote:
> > The gpt_clk rate function doesn't consider the PER divider. This causes a
> > significant drift in time accounting. Fix this by introducing the correct rate
> > calculation function.
> 
> Should have tested this one. In fact with this patch applied my clock
> goes wrong.
> 
> The i.MX Timer code makes sure the gpt clock is sourced from the ipg
> clock (GPTCR[6:8] = 1), so the behaviour should be correct the way it
> was before this patch. Any idea why it was wrong on your hardware? Have
> you changed the GPTCR bits?

No. My current platform is the i.MX25 PDK. I've added the following to my 
mx25pdk_init():

debugfs_create_x32("gptcr", 0444, NULL,
    (u32*)MX25_IO_ADDRESS(MX25_GPT1_BASE_ADDR));

When the system is running I get:

# cat /debugfs/gptcr 
0x00000249

That is GPTCR[6:8] = 1.

The same clock calculation is being done in the platform code of the Freescale 
supplied kernel (now based on 2.6.31).  Can you get this one running on your 
platform?

baruch

> > 
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > ---
> >  arch/arm/mach-mx25/clock.c |    7 ++++++-
> >  1 files changed, 6 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
> > index 08aaa38..c003ac4 100644
> > --- a/arch/arm/mach-mx25/clock.c
> > +++ b/arch/arm/mach-mx25/clock.c
> > @@ -119,6 +119,11 @@ static unsigned long get_rate_nfc(struct clk *clk)
> >  	return get_rate_per(8);
> >  }
> >  
> > +static unsigned long get_rate_gpt(struct clk *clk)
> > +{
> > +	return get_rate_per(5);
> > +}
> > +
> >  static unsigned long get_rate_otg(struct clk *clk)
> >  {
> >  	return 48000000; /* FIXME */
> > @@ -156,7 +161,7 @@ static void clk_cgcr_disable(struct clk *clk)
> >  		.secondary	= s,			\
> >  	}
> >  
> > -DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_ipg, NULL, NULL);
> > +DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_gpt, NULL, NULL);
> >  DEFINE_CLOCK(uart_per_clk, 0, CCM_CGCR0, 15, get_rate_uart, NULL, NULL);
> >  DEFINE_CLOCK(cspi1_clk,  0, CCM_CGCR1,  5, get_rate_ipg, NULL, NULL);
> >  DEFINE_CLOCK(cspi2_clk,  0, CCM_CGCR1,  6, get_rate_ipg, NULL, NULL);
> > -- 
> > 1.6.5
> > 
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 3/4] mx25: fix time accounting

[Sascha Hauer]

On Wed, May 12, 2010 at 08:06:28AM +0300, Baruch Siach wrote:
> Hi Sascha,
> 
> On Tue, May 11, 2010 at 05:43:49PM +0200, Sascha Hauer wrote:
> > On Mon, Jan 25, 2010 at 12:58:21PM +0200, Baruch Siach wrote:
> > > The gpt_clk rate function doesn't consider the PER divider. This causes a
> > > significant drift in time accounting. Fix this by introducing the correct rate
> > > calculation function.
> > 
> > Should have tested this one. In fact with this patch applied my clock
> > goes wrong.
> > 
> > The i.MX Timer code makes sure the gpt clock is sourced from the ipg
> > clock (GPTCR[6:8] = 1), so the behaviour should be correct the way it
> > was before this patch. Any idea why it was wrong on your hardware? Have
> > you changed the GPTCR bits?
> 
> No. My current platform is the i.MX25 PDK. I've added the following to my 
> mx25pdk_init():
> 
> debugfs_create_x32("gptcr", 0444, NULL,
>     (u32*)MX25_IO_ADDRESS(MX25_GPT1_BASE_ADDR));
> 
> When the system is running I get:
> 
> # cat /debugfs/gptcr 
> 0x00000249
> 
> That is GPTCR[6:8] = 1.
> 
> The same clock calculation is being done in the platform code of the Freescale 
> supplied kernel (now based on 2.6.31).  Can you get this one running on your 
> platform?

I just checked the fsl 2.6.31 source. They really pass the per_clk to
the timer, but they also change the timer source to MX3_TCTL_CLK_PER
(2<<6).
Here are the relevant bits from arch/arm/plat-mxc/time.c in the fsl
2.6.31 source:

/* MX31, MX35 */
#define MX3_TCTL_WAITEN		(1 << 3)
#define MX3_TCTL_CLK_IPG	(1 << 6)
#define MX3_TCTL_CLK_PER	(2 << 6)
#define MX3_TCTL_FRR		(1 << 9)
#define MX3_IR			0x0c
#define MX3_TSTAT		0x08
#define MX3_TSTAT_OF1		(1 << 0)
#define MX3_TCN			0x24
#define MX3_TCMP		0x10

#define timer_is_v2()	(!(cpu_is_mx1() || cpu_is_mx2()) || cpu_is_mx25())

void __init mxc_timer_init(struct clk *timer_clk, void __iomem *base, int irq)
{
	...

	if (timer_is_v2())
		tctl_val = MX3_TCTL_CLK_PER | MX3_TCTL_FRR | MX3_TCTL_WAITEN | MXC_TCTL_TEN;
	else
		tctl_val = MX1_2_TCTL_FRR | MX1_2_TCTL_CLK_PCLK1 | MXC_TCTL_TEN;

	...
}

I wonder what is going wrong here.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 3/4] mx25: fix time accounting

[Baruch Siach]

Hi Sascha,

On Wed, May 12, 2010 at 11:46:42AM +0200, Sascha Hauer wrote:
> On Wed, May 12, 2010 at 08:06:28AM +0300, Baruch Siach wrote:
> > On Tue, May 11, 2010 at 05:43:49PM +0200, Sascha Hauer wrote:
> > > On Mon, Jan 25, 2010 at 12:58:21PM +0200, Baruch Siach wrote:
> > > > The gpt_clk rate function doesn't consider the PER divider. This causes a
> > > > significant drift in time accounting. Fix this by introducing the correct rate
> > > > calculation function.
> > > 
> > > Should have tested this one. In fact with this patch applied my clock
> > > goes wrong.
> > > 
> > > The i.MX Timer code makes sure the gpt clock is sourced from the ipg
> > > clock (GPTCR[6:8] = 1), so the behaviour should be correct the way it
> > > was before this patch. Any idea why it was wrong on your hardware? Have
> > > you changed the GPTCR bits?
> > 
> > No. My current platform is the i.MX25 PDK. I've added the following to my 
> > mx25pdk_init():
> > 
> > debugfs_create_x32("gptcr", 0444, NULL,
> >     (u32*)MX25_IO_ADDRESS(MX25_GPT1_BASE_ADDR));
> > 
> > When the system is running I get:
> > 
> > # cat /debugfs/gptcr 
> > 0x00000249
> > 
> > That is GPTCR[6:8] = 1.
> > 
> > The same clock calculation is being done in the platform code of the Freescale 
> > supplied kernel (now based on 2.6.31).  Can you get this one running on your 
> > platform?
> 
> I just checked the fsl 2.6.31 source. They really pass the per_clk to
> the timer, but they also change the timer source to MX3_TCTL_CLK_PER
> (2<<6).

Strange. The i.MX25 Reference Manual says nothing about PER in the CLKSRC 
field of GPTCR. The relevant text from 28.5.2.1 "GPT Control Register (GPTCR)" 
is:

000 No clock
001 ipg_clk
010 ipg_clk_highfreq
011 ipp_ind_clkin (external clock from pad)
1xx ipg_clk_32k

So ipg_clk_highfreq == PER clock? Can anyone from Freescale shed some light on 
this?

baruch

> Here are the relevant bits from arch/arm/plat-mxc/time.c in the fsl
> 2.6.31 source:
> 
> /* MX31, MX35 */
> #define MX3_TCTL_WAITEN		(1 << 3)
> #define MX3_TCTL_CLK_IPG	(1 << 6)
> #define MX3_TCTL_CLK_PER	(2 << 6)
> #define MX3_TCTL_FRR		(1 << 9)
> #define MX3_IR			0x0c
> #define MX3_TSTAT		0x08
> #define MX3_TSTAT_OF1		(1 << 0)
> #define MX3_TCN			0x24
> #define MX3_TCMP		0x10
> 
> #define timer_is_v2()	(!(cpu_is_mx1() || cpu_is_mx2()) || cpu_is_mx25())
> 
> void __init mxc_timer_init(struct clk *timer_clk, void __iomem *base, int irq)
> {
> 	...
> 
> 	if (timer_is_v2())
> 		tctl_val = MX3_TCTL_CLK_PER | MX3_TCTL_FRR | MX3_TCTL_WAITEN | MXC_TCTL_TEN;
> 	else
> 		tctl_val = MX1_2_TCTL_FRR | MX1_2_TCTL_CLK_PCLK1 | MXC_TCTL_TEN;
> 
> 	...
> }
> 
> I wonder what is going wrong here.

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 3/4] mx25: fix time accounting

[Sascha Hauer]

On Wed, May 12, 2010 at 01:45:23PM +0300, Baruch Siach wrote:
> Hi Sascha,
> 
> On Wed, May 12, 2010 at 11:46:42AM +0200, Sascha Hauer wrote:
> > On Wed, May 12, 2010 at 08:06:28AM +0300, Baruch Siach wrote:
> > > On Tue, May 11, 2010 at 05:43:49PM +0200, Sascha Hauer wrote:
> > > > On Mon, Jan 25, 2010 at 12:58:21PM +0200, Baruch Siach wrote:
> > > > > The gpt_clk rate function doesn't consider the PER divider. This causes a
> > > > > significant drift in time accounting. Fix this by introducing the correct rate
> > > > > calculation function.
> > > > 
> > > > Should have tested this one. In fact with this patch applied my clock
> > > > goes wrong.
> > > > 
> > > > The i.MX Timer code makes sure the gpt clock is sourced from the ipg
> > > > clock (GPTCR[6:8] = 1), so the behaviour should be correct the way it
> > > > was before this patch. Any idea why it was wrong on your hardware? Have
> > > > you changed the GPTCR bits?
> > > 
> > > No. My current platform is the i.MX25 PDK. I've added the following to my 
> > > mx25pdk_init():
> > > 
> > > debugfs_create_x32("gptcr", 0444, NULL,
> > >     (u32*)MX25_IO_ADDRESS(MX25_GPT1_BASE_ADDR));
> > > 
> > > When the system is running I get:
> > > 
> > > # cat /debugfs/gptcr 
> > > 0x00000249
> > > 
> > > That is GPTCR[6:8] = 1.
> > > 
> > > The same clock calculation is being done in the platform code of the Freescale 
> > > supplied kernel (now based on 2.6.31).  Can you get this one running on your 
> > > platform?
> > 
> > I just checked the fsl 2.6.31 source. They really pass the per_clk to
> > the timer, but they also change the timer source to MX3_TCTL_CLK_PER
> > (2<<6).
> 
> Strange. The i.MX25 Reference Manual says nothing about PER in the CLKSRC 
> field of GPTCR. The relevant text from 28.5.2.1 "GPT Control Register (GPTCR)" 
> is:
> 
> 000 No clock
> 001 ipg_clk
> 010 ipg_clk_highfreq
> 011 ipp_ind_clkin (external clock from pad)
> 1xx ipg_clk_32k
> 
> So ipg_clk_highfreq == PER clock?

I think so, yes. I'm fairly used to the fact that the clock names in the
peripherals do not match the ones mentioned in the clock chapter, this
has a long tradition :(

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 3/4] mx25: fix time accounting

[Baruch Siach]

Hi Sascha,

On Wed, May 12, 2010 at 11:46:42AM +0200, Sascha Hauer wrote:
> On Wed, May 12, 2010 at 08:06:28AM +0300, Baruch Siach wrote:
> > On Tue, May 11, 2010 at 05:43:49PM +0200, Sascha Hauer wrote:
> > > On Mon, Jan 25, 2010 at 12:58:21PM +0200, Baruch Siach wrote:
> > > > The gpt_clk rate function doesn't consider the PER divider. This causes a
> > > > significant drift in time accounting. Fix this by introducing the correct rate
> > > > calculation function.
> > > 
> > > Should have tested this one. In fact with this patch applied my clock
> > > goes wrong.
> > > 
> > > The i.MX Timer code makes sure the gpt clock is sourced from the ipg
> > > clock (GPTCR[6:8] = 1), so the behaviour should be correct the way it
> > > was before this patch. Any idea why it was wrong on your hardware? Have
> > > you changed the GPTCR bits?
> > 
> > No. My current platform is the i.MX25 PDK. I've added the following to my 
> > mx25pdk_init():
> > 
> > debugfs_create_x32("gptcr", 0444, NULL,
> >     (u32*)MX25_IO_ADDRESS(MX25_GPT1_BASE_ADDR));
> > 
> > When the system is running I get:
> > 
> > # cat /debugfs/gptcr 
> > 0x00000249
> > 
> > That is GPTCR[6:8] = 1.
> > 
> > The same clock calculation is being done in the platform code of the Freescale 
> > supplied kernel (now based on 2.6.31).  Can you get this one running on your 
> > platform?
> 
> I just checked the fsl 2.6.31 source. They really pass the per_clk to
> the timer, but they also change the timer source to MX3_TCTL_CLK_PER
> (2<<6).
> Here are the relevant bits from arch/arm/plat-mxc/time.c in the fsl
> 2.6.31 source:
> 
> /* MX31, MX35 */
> #define MX3_TCTL_WAITEN		(1 << 3)
> #define MX3_TCTL_CLK_IPG	(1 << 6)
> #define MX3_TCTL_CLK_PER	(2 << 6)
> #define MX3_TCTL_FRR		(1 << 9)
> #define MX3_IR			0x0c
> #define MX3_TSTAT		0x08
> #define MX3_TSTAT_OF1		(1 << 0)
> #define MX3_TCN			0x24
> #define MX3_TCMP		0x10
> 
> #define timer_is_v2()	(!(cpu_is_mx1() || cpu_is_mx2()) || cpu_is_mx25())
> 
> void __init mxc_timer_init(struct clk *timer_clk, void __iomem *base, int irq)
> {
> 	...
> 
> 	if (timer_is_v2())
> 		tctl_val = MX3_TCTL_CLK_PER | MX3_TCTL_FRR | MX3_TCTL_WAITEN | MXC_TCTL_TEN;
> 	else
> 		tctl_val = MX1_2_TCTL_FRR | MX1_2_TCTL_CLK_PCLK1 | MXC_TCTL_TEN;
> 
> 	...
> }
> 
> I wonder what is going wrong here.

Our target board has finally arrived and I now see the same problem (i.e., the 
clock runs at half rate).  Setting the clock source to PER as is shown in the 
fsl code fixes the problem on this target board.

The strange thing is that the i.MX25 PDK works fine both ways. Maybe the move 
from RedBoot to Barebox has something to do with this ;-). I haven't tried 
Barebox on the PDK while booting from flash.

Anyway, I'm sending a patch in reply to this message.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH] mx25: set GPT clock source to PER

[Baruch Siach]

This fixes time accounting on mx25 base systems.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 arch/arm/plat-mxc/time.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-mxc/time.c b/arch/arm/plat-mxc/time.c
index f9a1b05..0d7e0f9 100644
--- a/arch/arm/plat-mxc/time.c
+++ b/arch/arm/plat-mxc/time.c
@@ -56,6 +56,7 @@
 /* MX31, MX35, MX25, MXC91231, MX5 */
 #define V2_TCTL_WAITEN		(1 << 3) /* Wait enable mode */
 #define V2_TCTL_CLK_IPG		(1 << 6)
+#define V2_TCTL_CLK_PER		(2 << 6)
 #define V2_TCTL_FRR		(1 << 9)
 #define V2_IR			0x0c
 #define V2_TSTAT		0x08
@@ -308,7 +309,7 @@ void __init mxc_timer_init(struct clk *timer_clk, void __iomem *base, int irq)
 	__raw_writel(0, timer_base + MXC_TPRER); /* see datasheet note */
 
 	if (timer_is_v2())
-		tctl_val = V2_TCTL_CLK_IPG | V2_TCTL_FRR | V2_TCTL_WAITEN | MXC_TCTL_TEN;
+		tctl_val = V2_TCTL_CLK_PER | V2_TCTL_FRR | V2_TCTL_WAITEN | MXC_TCTL_TEN;
 	else
 		tctl_val = MX1_2_TCTL_FRR | MX1_2_TCTL_CLK_PCLK1 | MXC_TCTL_TEN;
 
-- 
1.7.1

[PATCH 2/4] mx25: properly initialize clocks

[Uwe Kleine-König]

Hello Baruch,

On Mon, Jan 25, 2010 at 12:58:20PM +0200, Baruch Siach wrote:
> This patch disables all unnecessary clock in mx25_clocks_init() to make a clean
> start, the same as is being done for the rest of the i.MX chips.
> 
> This patch was tested on i.MX25 PDK.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  arch/arm/mach-mx25/clock.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
> index abd303b..08aaa38 100644
> --- a/arch/arm/mach-mx25/clock.c
> +++ b/arch/arm/mach-mx25/clock.c
> @@ -218,6 +218,14 @@ int __init mx25_clocks_init(void)
>  	for (i = 0; i < ARRAY_SIZE(lookups); i++)
>  		clkdev_add(&lookups[i]);
>  
> +	/* Turn off all clocks except the ones we need to survive, namely:
> +	 * EMI, GPIO1-3 (CCM_CGCR1[18:16]), GPT1, IOMUXC (CCM_CGCR1[27]), IIM,
> +	 * SCC
> +	 */
> +	__raw_writel((1 << 19), CRM_BASE + CCM_CGCR0);
> +	__raw_writel((0xf << 16) | (3 << 26), CRM_BASE + CCM_CGCR1);
> +	__raw_writel((1 << 5), CRM_BASE + CCM_CGCR2);
> +
I would prefer having symbolic names for the constants used here because
otherwise comment and code tend to diverge.

And I didn't check the manuals, but some other imx clock code (e.g.
arch/arm/mach-mx3/clock-imx35.c) pay attention to CONFIG_DEBUG_LL being
defined.  Is this needed here, too?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] mx25: set GPT clock source to PER

[Sascha Hauer]

On Sun, Jul 04, 2010 at 11:47:29AM +0300, Baruch Siach wrote:
> This fixes time accounting on mx25 base systems.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  arch/arm/plat-mxc/time.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/plat-mxc/time.c b/arch/arm/plat-mxc/time.c
> index f9a1b05..0d7e0f9 100644
> --- a/arch/arm/plat-mxc/time.c
> +++ b/arch/arm/plat-mxc/time.c
> @@ -56,6 +56,7 @@
>  /* MX31, MX35, MX25, MXC91231, MX5 */
>  #define V2_TCTL_WAITEN		(1 << 3) /* Wait enable mode */
>  #define V2_TCTL_CLK_IPG		(1 << 6)
> +#define V2_TCTL_CLK_PER		(2 << 6)
>  #define V2_TCTL_FRR		(1 << 9)
>  #define V2_IR			0x0c
>  #define V2_TSTAT		0x08
> @@ -308,7 +309,7 @@ void __init mxc_timer_init(struct clk *timer_clk, void __iomem *base, int irq)
>  	__raw_writel(0, timer_base + MXC_TPRER); /* see datasheet note */
>  
>  	if (timer_is_v2())
> -		tctl_val = V2_TCTL_CLK_IPG | V2_TCTL_FRR | V2_TCTL_WAITEN | MXC_TCTL_TEN;
> +		tctl_val = V2_TCTL_CLK_PER | V2_TCTL_FRR | V2_TCTL_WAITEN | MXC_TCTL_TEN;
>  	else
>  		tctl_val = MX1_2_TCTL_FRR | MX1_2_TCTL_CLK_PCLK1 | MXC_TCTL_TEN;
> 

I wonder why I applied

commit faed40665d2d81b7e0e537d14ef680ab3da9f22d
Author: Baruch Siach <baruch@tkos.co.il>
Date:   Mon Jan 25 12:58:21 2010 +0200

    mx25: fix time accounting
    
    The gpt_clk rate function doesn't consider the PER divider. This causes a
    significant drift in time accounting. Fix this by introducing the correct rate
    calculation function.
    
    Signed-off-by: Baruch Siach <baruch@tkos.co.il>
    Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
index 08aaa38..c003ac4 100644
--- a/arch/arm/mach-mx25/clock.c
+++ b/arch/arm/mach-mx25/clock.c
@@ -119,6 +119,11 @@ static unsigned long get_rate_nfc(struct clk *clk)
 	return get_rate_per(8);
 }
 
+static unsigned long get_rate_gpt(struct clk *clk)
+{
+	return get_rate_per(5);
+}
+
 static unsigned long get_rate_otg(struct clk *clk)
 {
 	return 48000000; /* FIXME */
@@ -156,7 +161,7 @@ static void clk_cgcr_disable(struct clk *clk)
 		.secondary	= s,			\
 	}
 
-DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_ipg, NULL, NULL);
+DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_gpt, NULL, NULL);
 DEFINE_CLOCK(uart_per_clk, 0, CCM_CGCR0, 15, get_rate_uart, NULL, NULL);
 DEFINE_CLOCK(cspi1_clk,  0, CCM_CGCR1,  5, get_rate_ipg, NULL, NULL);
 DEFINE_CLOCK(cspi2_clk,  0, CCM_CGCR1,  6, get_rate_ipg, NULL, NULL);

This changed the gpt clock from ipg to the peripheral clock and I
explained why this is wrong. Now of course we now must use the
peripheral clock in the timer code.

Does it work when you revert faed40665d2d81b7e0e537d14ef680ab3da9f22d?

In any case, we can't apply your patch because it would break other v2
based architectures like i.MX35.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 3/4] mx25: fix time accounting

[Martin Fuzzey]

Hi Baruch,
>
> The strange thing is that the i.MX25 PDK works fine both ways. Maybe the move
> from RedBoot to Barebox has something to do with this ;-). I haven't tried
> Barebox on the PDK while booting from flash.
>
Could this possibly be related to the problem I ran into on MX21 a year ago?

http://www.spinics.net/lists/arm-kernel/msg64267.html

Does the MX25 manual contain any restrictions on the relations between
clock frequencies?

Martin

[PATCH] mx25: set GPT clock source to PER

[Baruch Siach]

Hi Sascha,

On Mon, Jul 05, 2010 at 10:21:23AM +0200, Sascha Hauer wrote:
> On Sun, Jul 04, 2010 at 11:47:29AM +0300, Baruch Siach wrote:
> > This fixes time accounting on mx25 base systems.
> > 
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > ---
> >  arch/arm/plat-mxc/time.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/arm/plat-mxc/time.c b/arch/arm/plat-mxc/time.c
> > index f9a1b05..0d7e0f9 100644
> > --- a/arch/arm/plat-mxc/time.c
> > +++ b/arch/arm/plat-mxc/time.c
> > @@ -56,6 +56,7 @@
> >  /* MX31, MX35, MX25, MXC91231, MX5 */
> >  #define V2_TCTL_WAITEN		(1 << 3) /* Wait enable mode */
> >  #define V2_TCTL_CLK_IPG		(1 << 6)
> > +#define V2_TCTL_CLK_PER		(2 << 6)
> >  #define V2_TCTL_FRR		(1 << 9)
> >  #define V2_IR			0x0c
> >  #define V2_TSTAT		0x08
> > @@ -308,7 +309,7 @@ void __init mxc_timer_init(struct clk *timer_clk, void __iomem *base, int irq)
> >  	__raw_writel(0, timer_base + MXC_TPRER); /* see datasheet note */
> >  
> >  	if (timer_is_v2())
> > -		tctl_val = V2_TCTL_CLK_IPG | V2_TCTL_FRR | V2_TCTL_WAITEN | MXC_TCTL_TEN;
> > +		tctl_val = V2_TCTL_CLK_PER | V2_TCTL_FRR | V2_TCTL_WAITEN | MXC_TCTL_TEN;
> >  	else
> >  		tctl_val = MX1_2_TCTL_FRR | MX1_2_TCTL_CLK_PCLK1 | MXC_TCTL_TEN;
> > 
> 
> I wonder why I applied
> 
> commit faed40665d2d81b7e0e537d14ef680ab3da9f22d
> Author: Baruch Siach <baruch@tkos.co.il>
> Date:   Mon Jan 25 12:58:21 2010 +0200
> 
>     mx25: fix time accounting
>     
>     The gpt_clk rate function doesn't consider the PER divider. This causes a
>     significant drift in time accounting. Fix this by introducing the correct rate
>     calculation function.
>     
>     Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>     Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
> index 08aaa38..c003ac4 100644
> --- a/arch/arm/mach-mx25/clock.c
> +++ b/arch/arm/mach-mx25/clock.c
> @@ -119,6 +119,11 @@ static unsigned long get_rate_nfc(struct clk *clk)
>  	return get_rate_per(8);
>  }
>  
> +static unsigned long get_rate_gpt(struct clk *clk)
> +{
> +	return get_rate_per(5);
> +}
> +
>  static unsigned long get_rate_otg(struct clk *clk)
>  {
>  	return 48000000; /* FIXME */
> @@ -156,7 +161,7 @@ static void clk_cgcr_disable(struct clk *clk)
>  		.secondary	= s,			\
>  	}
>  
> -DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_ipg, NULL, NULL);
> +DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_gpt, NULL, NULL);
>  DEFINE_CLOCK(uart_per_clk, 0, CCM_CGCR0, 15, get_rate_uart, NULL, NULL);
>  DEFINE_CLOCK(cspi1_clk,  0, CCM_CGCR1,  5, get_rate_ipg, NULL, NULL);
>  DEFINE_CLOCK(cspi2_clk,  0, CCM_CGCR1,  6, get_rate_ipg, NULL, NULL);
> 
> This changed the gpt clock from ipg to the peripheral clock and I
> explained why this is wrong. Now of course we now must use the
> peripheral clock in the timer code.
> 
> Does it work when you revert faed40665d2d81b7e0e537d14ef680ab3da9f22d?

Reverting faed406 works on our target board, but breaks the i.MX25 PDK.

> In any case, we can't apply your patch because it would break other v2
> based architectures like i.MX35.

What do you suggest then?

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH] mx25: set GPT clock source to PER

[Sascha Hauer]

On Tue, Jul 06, 2010 at 09:35:11AM +0300, Baruch Siach wrote:
> Hi Sascha,
> 
> On Mon, Jul 05, 2010 at 10:21:23AM +0200, Sascha Hauer wrote:
> > On Sun, Jul 04, 2010 at 11:47:29AM +0300, Baruch Siach wrote:
> > > This fixes time accounting on mx25 base systems.
> > > 
> > > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > > ---
> > >  arch/arm/plat-mxc/time.c |    3 ++-
> > >  1 files changed, 2 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/arch/arm/plat-mxc/time.c b/arch/arm/plat-mxc/time.c
> > > index f9a1b05..0d7e0f9 100644
> > > --- a/arch/arm/plat-mxc/time.c
> > > +++ b/arch/arm/plat-mxc/time.c
> > > @@ -56,6 +56,7 @@
> > >  /* MX31, MX35, MX25, MXC91231, MX5 */
> > >  #define V2_TCTL_WAITEN		(1 << 3) /* Wait enable mode */
> > >  #define V2_TCTL_CLK_IPG		(1 << 6)
> > > +#define V2_TCTL_CLK_PER		(2 << 6)
> > >  #define V2_TCTL_FRR		(1 << 9)
> > >  #define V2_IR			0x0c
> > >  #define V2_TSTAT		0x08
> > > @@ -308,7 +309,7 @@ void __init mxc_timer_init(struct clk *timer_clk, void __iomem *base, int irq)
> > >  	__raw_writel(0, timer_base + MXC_TPRER); /* see datasheet note */
> > >  
> > >  	if (timer_is_v2())
> > > -		tctl_val = V2_TCTL_CLK_IPG | V2_TCTL_FRR | V2_TCTL_WAITEN | MXC_TCTL_TEN;
> > > +		tctl_val = V2_TCTL_CLK_PER | V2_TCTL_FRR | V2_TCTL_WAITEN | MXC_TCTL_TEN;
> > >  	else
> > >  		tctl_val = MX1_2_TCTL_FRR | MX1_2_TCTL_CLK_PCLK1 | MXC_TCTL_TEN;
> > > 
> > 
> > I wonder why I applied
> > 
> > commit faed40665d2d81b7e0e537d14ef680ab3da9f22d
> > Author: Baruch Siach <baruch@tkos.co.il>
> > Date:   Mon Jan 25 12:58:21 2010 +0200
> > 
> >     mx25: fix time accounting
> >     
> >     The gpt_clk rate function doesn't consider the PER divider. This causes a
> >     significant drift in time accounting. Fix this by introducing the correct rate
> >     calculation function.
> >     
> >     Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> >     Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > 
> > diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
> > index 08aaa38..c003ac4 100644
> > --- a/arch/arm/mach-mx25/clock.c
> > +++ b/arch/arm/mach-mx25/clock.c
> > @@ -119,6 +119,11 @@ static unsigned long get_rate_nfc(struct clk *clk)
> >  	return get_rate_per(8);
> >  }
> >  
> > +static unsigned long get_rate_gpt(struct clk *clk)
> > +{
> > +	return get_rate_per(5);
> > +}
> > +
> >  static unsigned long get_rate_otg(struct clk *clk)
> >  {
> >  	return 48000000; /* FIXME */
> > @@ -156,7 +161,7 @@ static void clk_cgcr_disable(struct clk *clk)
> >  		.secondary	= s,			\
> >  	}
> >  
> > -DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_ipg, NULL, NULL);
> > +DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_gpt, NULL, NULL);
> >  DEFINE_CLOCK(uart_per_clk, 0, CCM_CGCR0, 15, get_rate_uart, NULL, NULL);
> >  DEFINE_CLOCK(cspi1_clk,  0, CCM_CGCR1,  5, get_rate_ipg, NULL, NULL);
> >  DEFINE_CLOCK(cspi2_clk,  0, CCM_CGCR1,  6, get_rate_ipg, NULL, NULL);
> > 
> > This changed the gpt clock from ipg to the peripheral clock and I
> > explained why this is wrong. Now of course we now must use the
> > peripheral clock in the timer code.
> > 
> > Does it work when you revert faed40665d2d81b7e0e537d14ef680ab3da9f22d?
> 
> Reverting faed406 works on our target board, but breaks the i.MX25 PDK.
> 
> > In any case, we can't apply your patch because it would break other v2
> > based architectures like i.MX35.
> 
> What do you suggest then?

Tell the timer code which clock to use with an argument to
mxc_timer_init() and add the missing clocks to the clock.c file. You
also have to pass this argument through from the board to
mx25_clocks_init(). That's a lot of work and in the end we still do not
know why some i.MX25 variants need the peripheral clock and others need
the ipg clock. So what I really suggest is that you invest some
research to find the source of this strangeness. Some things that could
help you are:

- the clko pin
- Do both boards have identical i.MX25 tapeout versions?
- Are the clocks initialized identically?

I bricked my i.MX25 PDK so unfortunately I do not have any hardware to
do some tests myself.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] mx25: set GPT clock source to PER

[Rob Herring]

On Tue, 2010-07-06 at 09:33 +0200, Sascha Hauer wrote:
> On Tue, Jul 06, 2010 at 09:35:11AM +0300, Baruch Siach wrote:
> > > In any case, we can't apply your patch because it would break other v2
> > > based architectures like i.MX35.
> > 
> > What do you suggest then?
> 
> Tell the timer code which clock to use with an argument to
> mxc_timer_init() and add the missing clocks to the clock.c file. You
> also have to pass this argument through from the board to
> mx25_clocks_init(). That's a lot of work and in the end we still do not
> know why some i.MX25 variants need the peripheral clock and others need
> the ipg clock. So what I really suggest is that you invest some
> research to find the source of this strangeness. Some things that could
> help you are:
> 
> - the clko pin
> - Do both boards have identical i.MX25 tapeout versions?
> - Are the clocks initialized identically?
> 
> I bricked my i.MX25 PDK so unfortunately I do not have any hardware to
> do some tests myself.

There are no Si issues with MX25 that would cause per_clk to not work. 

There are 2 possible clocking configurations and it is not tied to v1
vs. v2 timer hardware:
ipg_clk and per_clk are same clock source - MX1, MX21, MX31
per_clk is independent clock - every other i.MX chip

The correct setup is to use per_clk as ipg_clk can change frequencies
for bus scaling in the latter case. 

Rob

[PATCH 3/4] mx25: fix time accounting

[Rob Herring]

Baruch,

On Wed, 2010-05-12 at 13:45 +0300, Baruch Siach wrote:
> Hi Sascha,
> 
> On Wed, May 12, 2010 at 11:46:42AM +0200, Sascha Hauer wrote:
> > On Wed, May 12, 2010 at 08:06:28AM +0300, Baruch Siach wrote:
> > > On Tue, May 11, 2010 at 05:43:49PM +0200, Sascha Hauer wrote:
> > > > On Mon, Jan 25, 2010 at 12:58:21PM +0200, Baruch Siach wrote:
> > > > > The gpt_clk rate function doesn't consider the PER divider. This causes a
> > > > > significant drift in time accounting. Fix this by introducing the correct rate
> > > > > calculation function.
> > > > 
> > > > Should have tested this one. In fact with this patch applied my clock
> > > > goes wrong.
> > > > 
> > > > The i.MX Timer code makes sure the gpt clock is sourced from the ipg
> > > > clock (GPTCR[6:8] = 1), so the behaviour should be correct the way it
> > > > was before this patch. Any idea why it was wrong on your hardware? Have
> > > > you changed the GPTCR bits?
> > > 
> > > No. My current platform is the i.MX25 PDK. I've added the following to my 
> > > mx25pdk_init():
> > > 
> > > debugfs_create_x32("gptcr", 0444, NULL,
> > >     (u32*)MX25_IO_ADDRESS(MX25_GPT1_BASE_ADDR));
> > > 
> > > When the system is running I get:
> > > 
> > > # cat /debugfs/gptcr 
> > > 0x00000249
> > > 
> > > That is GPTCR[6:8] = 1.
> > > 
> > > The same clock calculation is being done in the platform code of the Freescale 
> > > supplied kernel (now based on 2.6.31).  Can you get this one running on your 
> > > platform?
> > 
> > I just checked the fsl 2.6.31 source. They really pass the per_clk to
> > the timer, but they also change the timer source to MX3_TCTL_CLK_PER
> > (2<<6).
> 
> Strange. The i.MX25 Reference Manual says nothing about PER in the CLKSRC 
> field of GPTCR. The relevant text from 28.5.2.1 "GPT Control Register (GPTCR)" 
> is:
> 
> 000 No clock
> 001 ipg_clk
> 010 ipg_clk_highfreq
> 011 ipp_ind_clkin (external clock from pad)
> 1xx ipg_clk_32k
> 
> So ipg_clk_highfreq == PER clock? Can anyone from Freescale shed some light on 
> this?

Yes, ipg_clk_highfreq is per_clk.

Rob

[PATCH] mx25: set GPT clock source to PER

[Baruch Siach]

Hi Sascha,

On Tue, Jul 06, 2010 at 09:33:14AM +0200, Sascha Hauer wrote:
> On Tue, Jul 06, 2010 at 09:35:11AM +0300, Baruch Siach wrote:
> > On Mon, Jul 05, 2010 at 10:21:23AM +0200, Sascha Hauer wrote:
> > > On Sun, Jul 04, 2010 at 11:47:29AM +0300, Baruch Siach wrote:
> > > > This fixes time accounting on mx25 base systems.
> > > > 
> > > > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > > > ---
> > > >  arch/arm/plat-mxc/time.c |    3 ++-
> > > >  1 files changed, 2 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/plat-mxc/time.c b/arch/arm/plat-mxc/time.c
> > > > index f9a1b05..0d7e0f9 100644
> > > > --- a/arch/arm/plat-mxc/time.c
> > > > +++ b/arch/arm/plat-mxc/time.c
> > > > @@ -56,6 +56,7 @@
> > > >  /* MX31, MX35, MX25, MXC91231, MX5 */
> > > >  #define V2_TCTL_WAITEN		(1 << 3) /* Wait enable mode */
> > > >  #define V2_TCTL_CLK_IPG		(1 << 6)
> > > > +#define V2_TCTL_CLK_PER		(2 << 6)
> > > >  #define V2_TCTL_FRR		(1 << 9)
> > > >  #define V2_IR			0x0c
> > > >  #define V2_TSTAT		0x08
> > > > @@ -308,7 +309,7 @@ void __init mxc_timer_init(struct clk *timer_clk, void __iomem *base, int irq)
> > > >  	__raw_writel(0, timer_base + MXC_TPRER); /* see datasheet note */
> > > >  
> > > >  	if (timer_is_v2())
> > > > -		tctl_val = V2_TCTL_CLK_IPG | V2_TCTL_FRR | V2_TCTL_WAITEN | MXC_TCTL_TEN;
> > > > +		tctl_val = V2_TCTL_CLK_PER | V2_TCTL_FRR | V2_TCTL_WAITEN | MXC_TCTL_TEN;
> > > >  	else
> > > >  		tctl_val = MX1_2_TCTL_FRR | MX1_2_TCTL_CLK_PCLK1 | MXC_TCTL_TEN;
> > > > 
> > > 
> > > I wonder why I applied
> > > 
> > > commit faed40665d2d81b7e0e537d14ef680ab3da9f22d
> > > Author: Baruch Siach <baruch@tkos.co.il>
> > > Date:   Mon Jan 25 12:58:21 2010 +0200
> > > 
> > >     mx25: fix time accounting
> > >     
> > >     The gpt_clk rate function doesn't consider the PER divider. This causes a
> > >     significant drift in time accounting. Fix this by introducing the correct rate
> > >     calculation function.
> > >     
> > >     Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > >     Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > 
> > > diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
> > > index 08aaa38..c003ac4 100644
> > > --- a/arch/arm/mach-mx25/clock.c
> > > +++ b/arch/arm/mach-mx25/clock.c
> > > @@ -119,6 +119,11 @@ static unsigned long get_rate_nfc(struct clk *clk)
> > >  	return get_rate_per(8);
> > >  }
> > >  
> > > +static unsigned long get_rate_gpt(struct clk *clk)
> > > +{
> > > +	return get_rate_per(5);
> > > +}
> > > +
> > >  static unsigned long get_rate_otg(struct clk *clk)
> > >  {
> > >  	return 48000000; /* FIXME */
> > > @@ -156,7 +161,7 @@ static void clk_cgcr_disable(struct clk *clk)
> > >  		.secondary	= s,			\
> > >  	}
> > >  
> > > -DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_ipg, NULL, NULL);
> > > +DEFINE_CLOCK(gpt_clk,    0, CCM_CGCR0,  5, get_rate_gpt, NULL, NULL);
> > >  DEFINE_CLOCK(uart_per_clk, 0, CCM_CGCR0, 15, get_rate_uart, NULL, NULL);
> > >  DEFINE_CLOCK(cspi1_clk,  0, CCM_CGCR1,  5, get_rate_ipg, NULL, NULL);
> > >  DEFINE_CLOCK(cspi2_clk,  0, CCM_CGCR1,  6, get_rate_ipg, NULL, NULL);
> > > 
> > > This changed the gpt clock from ipg to the peripheral clock and I
> > > explained why this is wrong. Now of course we now must use the
> > > peripheral clock in the timer code.
> > > 
> > > Does it work when you revert faed40665d2d81b7e0e537d14ef680ab3da9f22d?
> > 
> > Reverting faed406 works on our target board, but breaks the i.MX25 PDK.
> > 
> > > In any case, we can't apply your patch because it would break other v2
> > > based architectures like i.MX35.
> > 
> > What do you suggest then?
> 
> Tell the timer code which clock to use with an argument to
> mxc_timer_init() and add the missing clocks to the clock.c file. You
> also have to pass this argument through from the board to
> mx25_clocks_init(). That's a lot of work and in the end we still do not
> know why some i.MX25 variants need the peripheral clock and others need
> the ipg clock. So what I really suggest is that you invest some
> research to find the source of this strangeness.

OK. I think I've nailed it down. The CCM MCR register was initialized 
differently.  

On the PDK I use the factory installed RedBoot bootloader. RedBoot leaves the 
PER CLK MUX field at its default state, that is, MCR[15:0] = 0.

On our target board I use a port of Barebox based on the built-in PDK support.  
The board_init_lowlevel routine of the PDK platform code includes the 
following line:

    writel(0x0000FEFF, IMX_CCM_BASE + MX25_CCM_MCR)

This makes MCR[15:0] = 0xfeff. Commenting out this line makes both boards 
behave the same way. The timer clock runs correctly with or without setting 
the V2_TCTL_CLK_PER bit.  Reverting faed406, however, breaks both boards.

So, I thing we should keep faed406. You may also consider applying this patch 
setting V2_TCTL_CLK_PER, For the reason Rob Herring has stated.

baruch

> Some things that could help you are:
> 
> - the clko pin
> - Do both boards have identical i.MX25 tapeout versions?
> - Are the clocks initialized identically?
> 
> I bricked my i.MX25 PDK so unfortunately I do not have any hardware to
> do some tests myself.

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH] mx25: set GPT clock source to PER

[Sascha Hauer]

On Wed, Jul 07, 2010 at 11:00:13AM +0300, Baruch Siach wrote:
> > 
> > Tell the timer code which clock to use with an argument to
> > mxc_timer_init() and add the missing clocks to the clock.c file. You
> > also have to pass this argument through from the board to
> > mx25_clocks_init(). That's a lot of work and in the end we still do not
> > know why some i.MX25 variants need the peripheral clock and others need
> > the ipg clock. So what I really suggest is that you invest some
> > research to find the source of this strangeness.
> 
> OK. I think I've nailed it down. The CCM MCR register was initialized 
> differently.  
> 
> On the PDK I use the factory installed RedBoot bootloader. RedBoot leaves the 
> PER CLK MUX field at its default state, that is, MCR[15:0] = 0.
> 
> On our target board I use a port of Barebox based on the built-in PDK support.  
> The board_init_lowlevel routine of the PDK platform code includes the 
> following line:
> 
>     writel(0x0000FEFF, IMX_CCM_BASE + MX25_CCM_MCR)
> 
> This makes MCR[15:0] = 0xfeff. Commenting out this line makes both boards 
> behave the same way. The timer clock runs correctly with or without setting 
> the V2_TCTL_CLK_PER bit.  Reverting faed406, however, breaks both boards.
> 
> So, I thing we should keep faed406. You may also consider applying this patch 
> setting V2_TCTL_CLK_PER, For the reason Rob Herring has stated.

I made some tests on a custom i.MX25 board using barebox (no kernel
started)

CCM_MCR		GPT_TCTL[8:6]	get_gpt_clk returns	result
----------------------------------------------------------------------
1. 0xfeff	1 (ipgclk)	ipgclk (66.5MHz)	works
2. 0x0		1 (ipgclk)	ipgclk (66.5MHz)	works
3. 0xfeff	2 (perclk)	perclk5 (8.87MHz)	works
4. 0x0		2 (perclk)	perclk5 (16MHz)		works
5. 0x0		1 (ipgclk)	perclk5 (16MHz)		does not work

1 and 2 work because the MCR value only affects perclk, but not ipgclk.
3 and 4 work after I adjusted the per_clk5 divider so that the resulting
clock is < ipgclk / 4. That seems to be what Martin stated.
5 does not work because the timer runs at ipgclk but the clock code
returns per_clk5. This is exactly what the kernel currently does and
it's imho wrong.
reverting faed406 would mean switching to 1 or 2 which should both work.

We may switch to use perclk in the gpt later, but this would require
adjusting the other i.MXs aswell. I'm not convinced this is a good idea
though because currently we have a timer resolution of 1/66.5MHz, we
would go to at least 1/4 of this when using perclk.

I found the following patch in our repository which I forgot to post and
push upstream. Currenty the kernel returns wrong rates for the perclks
when the corresponding CCM_MCR bit is cleared. I hope this patch clears
some confusion instead of adding more...

commit 98901d5f62de56d87d3a31e46ebe9624edeac62b
Author: Sascha Hauer <s.hauer@pengutronix.de>
Date:   Mon Sep 14 09:01:13 2009 +0200

    mx25 clock: Peripheral parent clock is ahb, not ipg
    
    Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
index 7d8bca5..cb58b32 100644
--- a/arch/arm/mach-mx25/clock.c
+++ b/arch/arm/mach-mx25/clock.c
@@ -99,7 +99,7 @@ static unsigned long get_rate_per(int per)
 	if (readl(CRM_BASE + 0x64) & (1 << per))
 		fref = get_rate_upll();
 	else
-		fref = get_rate_ipg(NULL);
+		fref = get_rate_ahb(NULL);
 
 	return fref / (val + 1);
 }

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] mx25: set GPT clock source to PER

[Baruch Siach]

Hi Sascha,

On Wed, Jul 07, 2010 at 12:28:45PM +0200, Sascha Hauer wrote:
> On Wed, Jul 07, 2010 at 11:00:13AM +0300, Baruch Siach wrote:
> > > 
> > > Tell the timer code which clock to use with an argument to
> > > mxc_timer_init() and add the missing clocks to the clock.c file. You
> > > also have to pass this argument through from the board to
> > > mx25_clocks_init(). That's a lot of work and in the end we still do not
> > > know why some i.MX25 variants need the peripheral clock and others need
> > > the ipg clock. So what I really suggest is that you invest some
> > > research to find the source of this strangeness.
> > 
> > OK. I think I've nailed it down. The CCM MCR register was initialized 
> > differently.  
> > 
> > On the PDK I use the factory installed RedBoot bootloader. RedBoot leaves the 
> > PER CLK MUX field at its default state, that is, MCR[15:0] = 0.
> > 
> > On our target board I use a port of Barebox based on the built-in PDK support.  
> > The board_init_lowlevel routine of the PDK platform code includes the 
> > following line:
> > 
> >     writel(0x0000FEFF, IMX_CCM_BASE + MX25_CCM_MCR)
> > 
> > This makes MCR[15:0] = 0xfeff. Commenting out this line makes both boards 
> > behave the same way. The timer clock runs correctly with or without setting 
> > the V2_TCTL_CLK_PER bit.  Reverting faed406, however, breaks both boards.
> > 
> > So, I thing we should keep faed406. You may also consider applying this patch 
> > setting V2_TCTL_CLK_PER, For the reason Rob Herring has stated.
> 
> I made some tests on a custom i.MX25 board using barebox (no kernel
> started)
> 
> CCM_MCR		GPT_TCTL[8:6]	get_gpt_clk returns	result
> ----------------------------------------------------------------------
> 1. 0xfeff	1 (ipgclk)	ipgclk (66.5MHz)	works
> 2. 0x0		1 (ipgclk)	ipgclk (66.5MHz)	works
> 3. 0xfeff	2 (perclk)	perclk5 (8.87MHz)	works
> 4. 0x0		2 (perclk)	perclk5 (16MHz)		works
> 5. 0x0		1 (ipgclk)	perclk5 (16MHz)		does not work
> 
> 1 and 2 work because the MCR value only affects perclk, but not ipgclk.
> 3 and 4 work after I adjusted the per_clk5 divider so that the resulting
> clock is < ipgclk / 4. That seems to be what Martin stated.
> 5 does not work because the timer runs at ipgclk but the clock code
> returns per_clk5. This is exactly what the kernel currently does and
> it's imho wrong.
> reverting faed406 would mean switching to 1 or 2 which should both work.
> 
> We may switch to use perclk in the gpt later, but this would require
> adjusting the other i.MXs aswell. I'm not convinced this is a good idea
> though because currently we have a timer resolution of 1/66.5MHz, we
> would go to at least 1/4 of this when using perclk.
> 
> I found the following patch in our repository which I forgot to post and
> push upstream. Currenty the kernel returns wrong rates for the perclks
> when the corresponding CCM_MCR bit is cleared. I hope this patch clears
> some confusion instead of adding more...
> 
> commit 98901d5f62de56d87d3a31e46ebe9624edeac62b
> Author: Sascha Hauer <s.hauer@pengutronix.de>
> Date:   Mon Sep 14 09:01:13 2009 +0200
> 
>     mx25 clock: Peripheral parent clock is ahb, not ipg
>     
>     Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
> index 7d8bca5..cb58b32 100644
> --- a/arch/arm/mach-mx25/clock.c
> +++ b/arch/arm/mach-mx25/clock.c
> @@ -99,7 +99,7 @@ static unsigned long get_rate_per(int per)
>  	if (readl(CRM_BASE + 0x64) & (1 << per))
>  		fref = get_rate_upll();
>  	else
> -		fref = get_rate_ipg(NULL);
> +		fref = get_rate_ahb(NULL);
>  
>  	return fref / (val + 1);
>  }

I tested this patch on our target board without reverting of applying any 
related patch. Boot messages appear on the console correctly, but when 
(busybox) getty starts I see garbage on the console instead of the login 
prompt.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 2/4] mx25: properly initialize clocks

[Baruch Siach]

Hi Uwe,

On Mon, Jul 05, 2010 at 09:08:26AM +0200, Uwe Kleine-K?nig wrote:
> On Mon, Jan 25, 2010 at 12:58:20PM +0200, Baruch Siach wrote:
> > This patch disables all unnecessary clock in mx25_clocks_init() to make a clean
> > start, the same as is being done for the rest of the i.MX chips.
> > 
> > This patch was tested on i.MX25 PDK.
> > 
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > ---
> >  arch/arm/mach-mx25/clock.c |    8 ++++++++
> >  1 files changed, 8 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/mach-mx25/clock.c b/arch/arm/mach-mx25/clock.c
> > index abd303b..08aaa38 100644
> > --- a/arch/arm/mach-mx25/clock.c
> > +++ b/arch/arm/mach-mx25/clock.c
> > @@ -218,6 +218,14 @@ int __init mx25_clocks_init(void)
> >  	for (i = 0; i < ARRAY_SIZE(lookups); i++)
> >  		clkdev_add(&lookups[i]);
> >  
> > +	/* Turn off all clocks except the ones we need to survive, namely:
> > +	 * EMI, GPIO1-3 (CCM_CGCR1[18:16]), GPT1, IOMUXC (CCM_CGCR1[27]), IIM,
> > +	 * SCC
> > +	 */
> > +	__raw_writel((1 << 19), CRM_BASE + CCM_CGCR0);
> > +	__raw_writel((0xf << 16) | (3 << 26), CRM_BASE + CCM_CGCR1);
> > +	__raw_writel((1 << 5), CRM_BASE + CCM_CGCR2);
> > +
> I would prefer having symbolic names for the constants used here because
> otherwise comment and code tend to diverge.
> 
> And I didn't check the manuals, but some other imx clock code (e.g.
> arch/arm/mach-mx3/clock-imx35.c) pay attention to CONFIG_DEBUG_LL being
> defined.  Is this needed here, too?

The mx35 clock code enables UART1 (i.e. ttymxc0) when CONFIG_DEBUG_LL is 
defined. I used the settings above with CONFIG_DEBUG_LL and earlyprintk to 
diagnose early boot crashes on mx25 without problem. So I'm not sure this is 
necessary.

Besides, CONFIG_DEBUG_LL builds on mx25 (and mx1) are broken in the mainline 
kernel ):. See http://thread.gmane.org/gmane.linux.ports.arm.kernel/75691.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -