arm: Configure reference clock for Versatile Express timers

PATCH Superseded Pawel Moll 2 participants 11 messages Jan 18, 2011 – Jan 25, 2011

Subsystems (3)

Arm (32-bit) Arm sub-architectures Arm/versatile express platform

lore

Share & tools

lore

Pawel Moll

author

2011-01-18 18:04 UTC #0 lore raw

PATCH v2 1/1 arm: Configure reference clock for Versatile Express timers

Timers on Versatile Express mainboard are used as system clock/event sources. Driver assumes that they are clocked with 1MHz signal. Old V2M firmware apparently configured it by default, but on newer boards one can observe that "sleep 1" command takes over 30 seconds to finish, as the timers are fed with 32kHz instead...

This patch performs required magic and also removes code clearing timer's control registers, as exactly the same operations are performed by the timer driver few jiffies later.

Signed-off-by Pawel Moll <[email protected]>

Tested-by Will Deacon <[email protected]>

---
 arch/arm/include/asm/hardware/sp810.h |    6 ++++++
 arch/arm/mach-vexpress/v2m.c          |   10 ++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

arch/arm/include/asm/hardware/sp810.h +6 -0

index a101f10..721847d 100644
@@ -50,6 +50,12 @@
 #define SCPCELLID2		0xFF8
 #define SCPCELLID3		0xFFC
 
+#define SCCTRL_TIMEREN0SEL_REFCLK	(0 << 15)
+#define SCCTRL_TIMEREN0SEL_TIMCLK	(1 << 15)
+
+#define SCCTRL_TIMEREN1SEL_REFCLK	(0 << 17)
+#define SCCTRL_TIMEREN1SEL_TIMCLK	(1 << 17)
+
 static inline void sysctl_soft_reset(void __iomem *base)
 {
 	/* writing any value to SCSYSSTAT reg will reset system */

arch/arm/mach-vexpress/v2m.c +8 -2

index a9ed342..2ddad06 100644
@@ -19,6 +19,7 @@
 #include <asm/mach/time.h>
 #include <asm/hardware/arm_timer.h>
 #include <asm/hardware/timer-sp.h>
+#include <asm/hardware/sp810.h>
 
 #include <mach/motherboard.h>

@@ -50,10 +51,15 @@ void __init v2m_map_io(struct map_desc *tile, size_t num)
 
 static void __init v2m_timer_init(void)
 {
+	u32 scctrl;
+
 	versatile_sched_clock_init(MMIO_P2V(V2M_SYS_24MHZ), 24000000);
 
-	writel(0, MMIO_P2V(V2M_TIMER0) + TIMER_CTRL);
-	writel(0, MMIO_P2V(V2M_TIMER1) + TIMER_CTRL);
+	/* Select 1MHz TIMCLK as the reference clock for SP804 timers */
+	scctrl = readl(MMIO_P2V(V2M_SYSCTL + SCCTRL));
+	scctrl |= SCCTRL_TIMEREN0SEL_TIMCLK;
+	scctrl |= SCCTRL_TIMEREN1SEL_TIMCLK;
+	writel(scctrl, MMIO_P2V(V2M_SYSCTL + SCCTRL));
 
 	sp804_clocksource_init(MMIO_P2V(V2M_TIMER1));
 	sp804_clockevents_init(MMIO_P2V(V2M_TIMER0), IRQ_V2M_TIMER0);

Signature · 1 line

1.6.3.3

Russell King - ARM Linux

2011-01-24 14:47 UTC #1 lore raw

PATCH v2 1/1 arm: Configure reference clock for Versatile Express timers

arch/arm/mach-vexpress/v2m.c void __init v2m_map_io(struct map_desc *tile, size_t num) · line 62

Quoted 13 lines · Pawel Moll

static void __init v2m_timer_init(void)
{

arch/arm/mach-vexpress/v2m.c +7 -2

+	u32 scctrl;
+
 	versatile_sched_clock_init(MMIO_P2V(V2M_SYS_24MHZ), 24000000);
 
-	writel(0, MMIO_P2V(V2M_TIMER0) + TIMER_CTRL);
-	writel(0, MMIO_P2V(V2M_TIMER1) + TIMER_CTRL);
+	/* Select 1MHz TIMCLK as the reference clock for SP804 timers */
+	scctrl = readl(MMIO_P2V(V2M_SYSCTL + SCCTRL));
+	scctrl |= SCCTRL_TIMEREN0SEL_TIMCLK;
+	scctrl |= SCCTRL_TIMEREN1SEL_TIMCLK;
+	writel(scctrl, MMIO_P2V(V2M_SYSCTL + SCCTRL));

BTW, why are you removing the initialization of TIMER_CTRL ? You don't describe why you do this in the commit log so I suspect it's a mistake.

Pawel Moll

author

2011-01-24 16:12 UTC #2 lore raw

PATCH v2 1/1 arm: Configure reference clock for Versatile Express timers

Quoted 6 lines

arch/arm/mach-vexpress/v2m.c void __init v2m_map_io(struct map_desc *tile, size_t num) · line 56
Quoted 2 lines
arch/arm/mach-vexpress/v2m.c +0 -2
-   writel(0, MMIO_P2V(V2M_TIMER0) + TIMER_CTRL);
-   writel(0, MMIO_P2V(V2M_TIMER1) + TIMER_CTRL);
BTW, why are you removing the initialization of TIMER_CTRL ? You don't
describe why you do this in the commit log so I suspect it's a mistake.

Uh, apologies. The explanation got lost while moving the patch between one tree and another...

I've removed them, because the sp804 driver is initializing these registers in sp804_clocksource_init() and sp804_set_mode(), just a jiffy later.

Cheers!

Pawe?

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Russell King - ARM Linux

2011-01-24 16:14 UTC #3 lore raw

PATCH v2 1/1 arm: Configure reference clock for Versatile Express timers

Quoted 7 lines · Pawel Moll

Quoted 6 lines
arch/arm/mach-vexpress/v2m.c void __init v2m_map_io(struct map_desc *tile, size_t num) · line 56
Quoted 2 lines
arch/arm/mach-vexpress/v2m.c +0 -2
-   writel(0, MMIO_P2V(V2M_TIMER0) + TIMER_CTRL);
-   writel(0, MMIO_P2V(V2M_TIMER1) + TIMER_CTRL);
BTW, why are you removing the initialization of TIMER_CTRL ? You don't
describe why you do this in the commit log so I suspect it's a mistake.

Quoted 6 lines

Uh, apologies. The explanation got lost while moving the patch between one tree and another...
I've removed them, because the sp804 driver is initializing these registers in sp804_clocksource_init() and sp804_set_mode(), just a jiffy later.

What if we have some other more preferable clock events which rules in set_mode not being called? How do we ensure that these timers won't create some spurious interrupts?

Pawel Moll

author

2011-01-24 16:38 UTC #4 lore raw

PATCH v2 1/1 arm: Configure reference clock for Versatile Express timers

Quoted 6 lines

Quoted 3 lines
I've removed them, because the sp804 driver is initializing these registers in sp804_clocksource_init() and sp804_set_mode(), just a jiffy later.
What if we have some other more preferable clock events which rules in set_mode not being called? How do we ensure that these timers won't create some spurious interrupts?

Good point. The sp804_clockevent_init() should do this. A patch is just going out...

Pawe?

Russell King - ARM Linux

2011-01-24 16:42 UTC #5 lore raw

PATCH v2 1/1 arm: Configure reference clock for Versatile Express timers

Quoted 9 lines · Pawel Moll

Quoted 6 lines
Quoted 3 lines
I've removed them, because the sp804 driver is initializing these registers in sp804_clocksource_init() and sp804_set_mode(), just a jiffy later.
What if we have some other more preferable clock events which rules in set_mode not being called? How do we ensure that these timers won't create some spurious interrupts?
Good point. The sp804_clockevent_init() should do this. A patch is just going out...

No. Think about it. Some platforms have four timers. Should we leave two uninitialized? Should we have arch code initialize the two they're not using, and have the clockevents/clocksource code initialize the other two? Or should we have the arch code take care of it all.

My approach which is done for _all_ ARM platforms is to have the arch code do it. If you're going to change that, change _all_ ARM platforms and justify why it's correct to make it less clear that everything is properly initialized in one place.

Russell King - ARM Linux

2011-01-24 17:10 UTC #6 lore raw

PATCH v2 1/1 arm: Configure reference clock for Versatile Express timers

In reply to Pawel Moll

Quoted 8 lines · Pawel Moll

Quoted 6 lines
Quoted 3 lines
I've removed them, because the sp804 driver is initializing these registers in sp804_clocksource_init() and sp804_set_mode(), just a jiffy later.
What if we have some other more preferable clock events which rules in set_mode not being called? How do we ensure that these timers won't create some spurious interrupts?
Good point. The sp804_clockevent_init() should do this. A patch is just going out...

Right, I'm chucking your original patch out of my tree.

Pawel Moll

author

2011-01-24 19:13 UTC #7 lore raw

PATCH v2 1/1 arm: Configure reference clock for Versatile Express timers

Quoted 1 line

Right, I'm chucking your original patch out of my tree.

Ok, I'd like to separate these two things.

First is the reference clock configuration, and this change is really important. I'll send v3 in a second, which does not touch the TIMER_CTRL lines. I hope you are happy about this.

Quoted 1 line · The second thing is the SP804 reset... You said:

Some platforms have four timers. Should we leave two uninitialized?

VE _has_ four timers - two dual-timer modules - and you don't initialize V2M_TIMER[23]. True, the (potential) problem lies at "twin timers", sharing common interrupt, but if you want to reset all available timers on all platforms, we should perform the rites on them as well.

I also had a look at the TRM and the reset value for the control register is 0, but I'm guessing you are worried about boot monitor leftovers.

Cheers!

Pawe?

Russell King - ARM Linux

2011-01-24 20:20 UTC #8 lore raw

PATCH v2 1/1 arm: Configure reference clock for Versatile Express timers

Quoted 5 lines · Pawel Moll

Quoted 1 line
Right, I'm chucking your original patch out of my tree.
Ok, I'd like to separate these two things.
First is the reference clock configuration, and this change is really important. I'll send v3 in a second, which does not touch the TIMER_CTRL lines. I hope you are happy about this.

I'm not going to bother replying to this message until you wrap your messages sensibly. I can't read most of your message while composing a reply.

Pawel Moll

author

2011-01-25 14:18 UTC #9 lore raw

PATCH v2 1/1 arm: Configure reference clock for Versatile Express timers

Quoted 7 lines

Quoted 1 line
First is the reference clock configuration, and this change is
really important. I'll send v3 in a second, which does not touch the TIMER_CTRL lines. I hope you are happy about this.
I'm not going to bother replying to this message until you wrap your messages sensibly. I can't read most of your message while composing a reply.

Point taken, guilt accepted. Apologies.

I shouldn't have even tried to tame Outlook, my fault ;-)

Now - are you ok with sending v3 to your patch system?

Cheers!

Pawe?

Russell King - ARM Linux

2011-01-25 14:38 UTC #10 lore raw

PATCH v2 1/1 arm: Configure reference clock for Versatile Express timers

Quoted 8 lines · Pawel Moll

Quoted 7 lines
Quoted 1 line
First is the reference clock configuration, and this change is
really important. I'll send v3 in a second, which does not touch the TIMER_CTRL lines. I hope you are happy about this.
I'm not going to bother replying to this message until you wrap your messages sensibly. I can't read most of your message while composing a reply.

Quoted 5 lines

Point taken, guilt accepted. Apologies.
I shouldn't have even tried to tame Outlook, my fault ;-)
Now - are you ok with sending v3 to your patch system?

v3 looks much better, thanks.