[PATCH v2] arm: Configure reference clock for Versatile Express timers

[PATCH v2] arm: Configure reference clock for Versatile Express timers

[Pawel Moll]

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 <pawel.moll@arm.com>
Tested-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/hardware/sp810.h |    6 ++++++
 arch/arm/mach-vexpress/v2m.c          |   10 ++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/hardware/sp810.h b/arch/arm/include/asm/hardware/sp810.h
index a101f10..721847d 100644
--- a/arch/arm/include/asm/hardware/sp810.h
+++ b/arch/arm/include/asm/hardware/sp810.h
@@ -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 */
diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
index a9ed342..2ddad06 100644
--- a/arch/arm/mach-vexpress/v2m.c
+++ b/arch/arm/mach-vexpress/v2m.c
@@ -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);
-- 
1.6.3.3

[PATCH v2] arm: Configure reference clock for Versatile Express timers

[Russell King - ARM Linux]

On Tue, Jan 18, 2011 at 06:04:43PM +0000, Pawel Moll wrote:
>  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));

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.

[PATCH v2] arm: Configure reference clock for Versatile Express timers

[Pawel Moll]

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

[PATCH v2] arm: Configure reference clock for Versatile Express timers

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 04:12:16PM +0000, Pawel Moll wrote:
> > > -   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.

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?

[PATCH v2] arm: Configure reference clock for Versatile Express timers

[Pawel Moll]

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

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

[PATCH v2] arm: Configure reference clock for Versatile Express timers

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 04:38:20PM +0000, Pawel Moll wrote:
> > > 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.

[PATCH v2] arm: Configure reference clock for Versatile Express timers

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 04:38:20PM +0000, Pawel Moll wrote:
> > > 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.

[PATCH v2] arm: Configure reference clock for Versatile Express timers

[Pawel Moll]

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

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?

[PATCH v2] arm: Configure reference clock for Versatile Express timers

[Russell King - ARM Linux]

On Mon, Jan 24, 2011 at 07:13:27PM -0000, Pawel Moll wrote:
> > 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.

[PATCH v2] arm: Configure reference clock for Versatile Express timers

[Pawel Moll]

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

[PATCH v2] arm: Configure reference clock for Versatile Express timers

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 02:18:27PM +0000, Pawel Moll wrote:
> > > 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?

v3 looks much better, thanks.