mx25: clock related fixes

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

---
 arch/arm/mach-mx25/clock.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

arch/arm/mach-mx25/clock.c +26 -1

index a4ba334..b1aadd5 100644
@@ -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;

Signature · 1 line

1.6.5

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

---
 arch/arm/mach-mx25/clock.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

arch/arm/mach-mx25/clock.c +6 -1

index a4ba334..b1aadd5 100644
@@ -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);

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

Quoted 3 lines

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.

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

---
 arch/arm/mach-mx25/clock.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

arch/arm/mach-mx25/clock.c +5 -0

index 08aaa38..c003ac4 100644
@@ -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 */

Baruch Siach writes:

arch/arm/mach-mx25/clock.c static void clk_cgcr_disable(struct clk *clk) · line 167

Quoted 6 lines

arch/arm/mach-mx25/clock.c +1 -1

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

---
 arch/arm/mach-mx25/clock.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

arch/arm/mach-mx25/clock.c +6 -1

index 08aaa38..c003ac4 100644
@@ -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);

Signature · 1 line

1.6.5

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

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?

Quoted 35 lines

Signed-off-by Baruch Siach <[email protected]>

---
 arch/arm/mach-mx25/clock.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

arch/arm/mach-mx25/clock.c +6 -1

index 08aaa38..c003ac4 100644
@@ -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);

Signature · 1 line

1.6.5

Signature · 4 lines

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 |

Hi Sascha,

Quoted 5 lines · Sascha Hauer

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

Quoted 7 lines

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?

Quoted 27 lines · Baruch Siach

Quoted 5 lines · Sascha Hauer

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

Quoted 7 lines

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.

Quoted 3 lines

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.

Hi Sascha,

Quoted 28 lines · Sascha Hauer

Quoted 27 lines · Baruch Siach

Quoted 5 lines · Sascha Hauer

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

Quoted 7 lines

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.

Quoted 7 lines

Quoted 3 lines

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?

Quoted 22 lines · Baruch Siach

Quoted 5 lines · Sascha Hauer

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

Quoted 7 lines

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:

Quoted 8 lines

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

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

---
 arch/arm/mach-mx25/clock.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

arch/arm/mach-mx25/clock.c +0 -0

index abd303b..08aaa38 100644
@@ -218,6 +218,14 @@ int __init mx25_clocks_init(void)
 	for (i = 0; i < ARRAY_SIZE(lookups); i++)

This fixes time accounting on mx25 base systems.

Signed-off-by Baruch Siach <[email protected]>

---
 arch/arm/plat-mxc/time.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

arch/arm/plat-mxc/time.c +1 -0

index f9a1b05..0d7e0f9 100644
@@ -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

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.

Quoted 20 lines · Baruch Siach

This fixes time accounting on mx25 base systems.

Signed-off-by Baruch Siach <[email protected]>

---
 arch/arm/plat-mxc/time.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

arch/arm/plat-mxc/time.c +1 -0

index f9a1b05..0d7e0f9 100644
@@ -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

arch/arm/plat-mxc/time.c void __init mxc_timer_init(struct clk *timer_clk, void __iomem *base, int irq) · line 315

Quoted 8 lines

arch/arm/plat-mxc/time.c +1 -1

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

Signed-off-by Sascha Hauer <[email protected]>

arch/arm/mach-mx25/clock.c +6 -1

index 08aaa38..c003ac4 100644
@@ -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.

Hi Sascha,

Quoted 75 lines · Sascha Hauer

Quoted 20 lines · Baruch Siach

This fixes time accounting on mx25 base systems.

Signed-off-by Baruch Siach <[email protected]>

---
 arch/arm/plat-mxc/time.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

arch/arm/plat-mxc/time.c +1 -0

index f9a1b05..0d7e0f9 100644
@@ -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

arch/arm/plat-mxc/time.c void __init mxc_timer_init(struct clk *timer_clk, void __iomem *base, int irq) · line 315

Quoted 8 lines

arch/arm/plat-mxc/time.c +1 -1

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

Signed-off-by Sascha Hauer <[email protected]>

arch/arm/mach-mx25/clock.c +6 -1

index 08aaa38..c003ac4 100644
@@ -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.

Quoted 1 line

Does it work when you revert faed40665d2d81b7e0e537d14ef680ab3da9f22d?

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

Quoted 2 lines

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?

Quoted 4 lines · Baruch Siach

Quoted 2 lines

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.

Hi Sascha,

Quoted 28 lines · Sascha Hauer

Quoted 27 lines · Baruch Siach

Quoted 5 lines · Sascha Hauer

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

Quoted 7 lines

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.

Quoted 7 lines

Quoted 3 lines

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?

Quoted 76 lines · Baruch Siach

Quoted 75 lines · Sascha Hauer

Quoted 20 lines · Baruch Siach

This fixes time accounting on mx25 base systems.

Signed-off-by Baruch Siach <[email protected]>

---
 arch/arm/plat-mxc/time.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

arch/arm/plat-mxc/time.c +1 -0

index f9a1b05..0d7e0f9 100644
@@ -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

arch/arm/plat-mxc/time.c void __init mxc_timer_init(struct clk *timer_clk, void __iomem *base, int irq) · line 315

Quoted 8 lines

arch/arm/plat-mxc/time.c +1 -1

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

Signed-off-by Sascha Hauer <[email protected]>

arch/arm/mach-mx25/clock.c +6 -1

index 08aaa38..c003ac4 100644
@@ -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.

Quoted 8 lines

Quoted 1 line

Does it work when you revert faed40665d2d81b7e0e537d14ef680ab3da9f22d?

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

Quoted 2 lines

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.

Quoted 8 lines

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.

Quoted 19 lines · Baruch Siach

Quoted 8 lines

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:

Quoted 8 lines

    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 <[email protected]>
Date:   Mon Sep 14 09:01:13 2009 +0200

mx25 clock: Peripheral parent clock is ahb, not ipg

Signed-off-by Sascha Hauer <[email protected]>

arch/arm/mach-mx25/clock.c +0 -0

index 7d8bca5..cb58b32 100644

Quoted 16 lines · 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 <[email protected]>

---
 arch/arm/mach-mx25/clock.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

arch/arm/mach-mx25/clock.c +0 -0

index abd303b..08aaa38 100644
@@ -218,6 +218,14 @@ int __init mx25_clocks_init(void)
 	for (i = 0; i < ARRAY_SIZE(lookups); i++)

arch/arm/mach-mx25/clock.c int __init mx25_clocks_init(void) · line 228

Quoted 10 lines

arch/arm/mach-mx25/clock.c +8 -0

 		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?