pnx8xxx: move to clocksource

pnx8xxx: move to clocksource

[Vitaly Wool]

This patch converts PNX8XXX system timer to clocksource.

 arch/mips/philips/pnx8550/common/time.c |  109 +++++++++++++++++++++-----------
 1 files changed, 72 insertions(+), 37 deletions(-)

Signed-off-by: Vitaly Wool <vitalywool@gmail.com>

Index: linux-2.6/arch/mips/philips/pnx8550/common/time.c
===================================================================
--- linux-2.6.orig/arch/mips/philips/pnx8550/common/time.c
+++ linux-2.6/arch/mips/philips/pnx8550/common/time.c
@@ -22,7 +22,6 @@
 #include <linux/kernel_stat.h>
 #include <linux/spinlock.h>
 #include <linux/interrupt.h>
-#include <linux/module.h>
 
 #include <asm/bootinfo.h>
 #include <asm/cpu.h>
@@ -41,11 +40,60 @@ static cycle_t hpt_read(void)
 	return read_c0_count2();
 }
 
+static struct clocksource pnx_clocksource = {
+	.name		= "pnx8xxx",
+	.rating		= 200,
+	.read		= hpt_read,
+	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
 static void timer_ack(void)
 {
 	write_c0_compare(cpj);
 }
 
+static irqreturn_t pnx8xxx_timer_interrupt(int irq, void *dev_id)
+{
+        struct clock_event_device *c = dev_id;
+
+        /* clear MATCH, signal the event */
+        c->event_handler(c);
+
+	return IRQ_HANDLED;
+}
+
+static struct irqaction pnx8xxx_timer_irq = {
+	.handler	= pnx8xxx_timer_interrupt,
+	.flags		= IRQF_DISABLED | IRQF_PERCPU,
+	.name		= "pnx8xxx_timer",
+};
+
+static irqreturn_t monotonic_interrupt(int irq, void *dev_id)
+{
+	/* Timer 2 clear interrupt */
+	write_c0_compare2(-1);
+	return IRQ_HANDLED;
+}
+
+static struct irqaction monotonic_irqaction = {
+	.handler = monotonic_interrupt,
+	.flags = IRQF_DISABLED,
+	.name = "Monotonic timer",
+};
+
+static int pnx8xxx_set_next_event(unsigned long delta,
+				struct clock_event_device *evt)
+{
+	write_c0_compare(delta);
+	return 0;
+}
+
+static struct clock_event_device pnx8xxx_clockevent = {
+	.name		= "pnx8xxx_clockevent",
+	.features	= CLOCK_EVT_FEAT_ONESHOT,
+	.set_next_event = pnx8xxx_set_next_event,
+};
+
 /*
  * plat_time_init() - it does the following things:
  *
@@ -58,11 +106,34 @@ static void timer_ack(void)
 
 __init void plat_time_init(void)
 {
+	unsigned int             configPR;
 	unsigned int             n;
 	unsigned int             m;
 	unsigned int             p;
 	unsigned int             pow2p;
 
+	clockevents_register_device(&pnx8xxx_clockevent);
+	clocksource_register(&pnx_clocksource);
+
+	setup_irq(PNX8550_INT_TIMER1, &pnx8xxx_timer_irq);
+	setup_irq(PNX8550_INT_TIMER2, &monotonic_irqaction);
+
+	/* Timer 1 start */
+	configPR = read_c0_config7();
+	configPR &= ~0x00000008;
+	write_c0_config7(configPR);
+
+	/* Timer 2 start */
+	configPR = read_c0_config7();
+	configPR &= ~0x00000010;
+	write_c0_config7(configPR);
+
+	/* Timer 3 stop */
+	configPR = read_c0_config7();
+	configPR |= 0x00000020;
+	write_c0_config7(configPR);
+
+
         /* PLL0 sets MIPS clock (PLL1 <=> TM1, PLL6 <=> TM2, PLL5 <=> mem) */
         /* (but only if CLK_MIPS_CTL select value [bits 3:1] is 1:  FIXME) */
 
@@ -87,42 +158,6 @@ __init void plat_time_init(void)
 	write_c0_count2(0);
 	write_c0_compare2(0xffffffff);
 
-	clocksource_mips.read = hpt_read;
-	mips_timer_ack = timer_ack;
-}
-
-static irqreturn_t monotonic_interrupt(int irq, void *dev_id)
-{
-	/* Timer 2 clear interrupt */
-	write_c0_compare2(-1);
-	return IRQ_HANDLED;
 }
 
-static struct irqaction monotonic_irqaction = {
-	.handler = monotonic_interrupt,
-	.flags = IRQF_DISABLED,
-	.name = "Monotonic timer",
-};
 
-void __init plat_timer_setup(struct irqaction *irq)
-{
-	int configPR;
-
-	setup_irq(PNX8550_INT_TIMER1, irq);
-	setup_irq(PNX8550_INT_TIMER2, &monotonic_irqaction);
-
-	/* Timer 1 start */
-	configPR = read_c0_config7();
-	configPR &= ~0x00000008;
-	write_c0_config7(configPR);
-
-	/* Timer 2 start */
-	configPR = read_c0_config7();
-	configPR &= ~0x00000010;
-	write_c0_config7(configPR);
-
-	/* Timer 3 stop */
-	configPR = read_c0_config7();
-	configPR |= 0x00000020;
-	write_c0_config7(configPR);
-}

Re: pnx8xxx: move to clocksource

[Ralf Baechle]

On Thu, Jan 10, 2008 at 05:10:05PM +0300, Vitaly Wool wrote:

> This patch converts PNX8XXX system timer to clocksource.

Checkpatch had a little to moan on your patch:

$ scripts/checkpatch.pl /tmp/pend 
ERROR: use tabs not spaces
#79: FILE: arch/mips/philips/pnx8550/common/time.c:57:
+        struct clock_event_device *c = dev_id;$

ERROR: use tabs not spaces
#81: FILE: arch/mips/philips/pnx8550/common/time.c:59:
+        /* clear MATCH, signal the event */$

ERROR: use tabs not spaces
#82: FILE: arch/mips/philips/pnx8550/common/time.c:60:
+        c->event_handler(c);$

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

I fixed those, applied.  Thanks!

  Ralf

Re: pnx8xxx: move to clocksource

[Sergei Shtylyov]

Hello.

Vitaly Wool wrote:

> This patch converts PNX8XXX system timer to clocksource.

    Well, this patch has been already committed but nevertheless...

> arch/mips/philips/pnx8550/common/time.c |  109 
> +++++++++++++++++++++-----------
> 1 files changed, 72 insertions(+), 37 deletions(-)

> Signed-off-by: Vitaly Wool <vitalywool@gmail.com>

> Index: linux-2.6/arch/mips/philips/pnx8550/common/time.c
> ===================================================================
> --- linux-2.6.orig/arch/mips/philips/pnx8550/common/time.c
> +++ linux-2.6/arch/mips/philips/pnx8550/common/time.c
> @@ -22,7 +22,6 @@
> #include <linux/kernel_stat.h>
> #include <linux/spinlock.h>
> #include <linux/interrupt.h>
> -#include <linux/module.h>
> 
> #include <asm/bootinfo.h>
> #include <asm/cpu.h>
> @@ -41,11 +40,60 @@ static cycle_t hpt_read(void)
>     return read_c0_count2();
> }

> +static struct clocksource pnx_clocksource = {
> +    .name        = "pnx8xxx",
> +    .rating        = 200,
> +    .read        = hpt_read,
> +    .flags        = CLOCK_SOURCE_IS_CONTINUOUS,
> +};

    Something probably have converted tabs to 8 spaces...
    I would have done it otherwise -- using timer 1 as a generic MIPS 
clocksource (just hooking the IRQ to reload the comparator to all ones), and 
timer 2 as clockevent...

> static void timer_ack(void)
> {
>     write_c0_compare(cpj);
> }

    Do we still need this function? I don't think so -- mips_timer_ack() is 
dead...

[...]

> +static struct clock_event_device pnx8xxx_clockevent = {
> +    .name        = "pnx8xxx_clockevent",
> +    .features    = CLOCK_EVT_FEAT_ONESHOT,

    Aren't PNX8550 timers actually periodic in nature?

> +    .set_next_event = pnx8xxx_set_next_event,
> +};
> +
> /*
>  * plat_time_init() - it does the following things:
>  *
> @@ -58,11 +106,34 @@ static void timer_ack(void)
> 
> __init void plat_time_init(void)
> {
> +    unsigned int             configPR;

   Something has definitely spoilt all the tabs in the patch...

>     unsigned int             n;
>     unsigned int             m;
>     unsigned int             p;
>     unsigned int             pow2p;
> 
> +    clockevents_register_device(&pnx8xxx_clockevent);
> +    clocksource_register(&pnx_clocksource);
> +
> +    setup_irq(PNX8550_INT_TIMER1, &pnx8xxx_timer_irq);
> +    setup_irq(PNX8550_INT_TIMER2, &monotonic_irqaction);
> +
> +    /* Timer 1 start */
> +    configPR = read_c0_config7();
> +    configPR &= ~0x00000008;
> +    write_c0_config7(configPR);
> +
> +    /* Timer 2 start */
> +    configPR = read_c0_config7();
> +    configPR &= ~0x00000010;
> +    write_c0_config7(configPR);
> +
> +    /* Timer 3 stop */
> +    configPR = read_c0_config7();
> +    configPR |= 0x00000020;
> +    write_c0_config7(configPR);

    Enabling timers before they are actually set up? :-|

> +
> +
>         /* PLL0 sets MIPS clock (PLL1 <=> TM1, PLL6 <=> TM2, PLL5 <=> 
> mem) */
>         /* (but only if CLK_MIPS_CTL select value [bits 3:1] is 1:  
> FIXME) */
> 

WBR, Sergei

Re: pnx8xxx: move to clocksource

[Ralf Baechle]

On Thu, Jan 10, 2008 at 07:21:17PM +0300, Sergei Shtylyov wrote:

>> Index: linux-2.6/arch/mips/philips/pnx8550/common/time.c
>> ===================================================================
>> --- linux-2.6.orig/arch/mips/philips/pnx8550/common/time.c
>> +++ linux-2.6/arch/mips/philips/pnx8550/common/time.c
>> @@ -22,7 +22,6 @@
>> #include <linux/kernel_stat.h>
>> #include <linux/spinlock.h>
>> #include <linux/interrupt.h>
>> -#include <linux/module.h>
>>
>> #include <asm/bootinfo.h>
>> #include <asm/cpu.h>
>> @@ -41,11 +40,60 @@ static cycle_t hpt_read(void)
>>     return read_c0_count2();
>> }
>
>> +static struct clocksource pnx_clocksource = {
>> +    .name        = "pnx8xxx",
>> +    .rating        = 200,
>> +    .read        = hpt_read,
>> +    .flags        = CLOCK_SOURCE_IS_CONTINUOUS,
>> +};
>
>    Something probably have converted tabs to 8 spaces...

Only 3 of them?

>    I would have done it otherwise -- using timer 1 as a generic MIPS 
> clocksource (just hooking the IRQ to reload the comparator to all ones), 
> and timer 2 as clockevent...
>
>> static void timer_ack(void)
>> {
>>     write_c0_compare(cpj);
>> }
>
>    Do we still need this function? I don't think so -- mips_timer_ack() is 
> dead...

It's only used on initialization.

> [...]
>
>> +static struct clock_event_device pnx8xxx_clockevent = {
>> +    .name        = "pnx8xxx_clockevent",
>> +    .features    = CLOCK_EVT_FEAT_ONESHOT,
>
>    Aren't PNX8550 timers actually periodic in nature?

All I recall is they were odd ;-)

The hardware nature of timers and how to declare them to the Linux timer
code is not always the same.  CLOCK_EVT_FEAT_ONESHOT be used if the
time to the next shot can be programmed.

>>
>> __init void plat_time_init(void)
>> {
>> +    unsigned int             configPR;
>
>   Something has definitely spoilt all the tabs in the patch...

I fixed the three checkpatch.pl was bitching about.

>>     unsigned int             n;
>>     unsigned int             m;
>>     unsigned int             p;
>>     unsigned int             pow2p;
>>
>> +    clockevents_register_device(&pnx8xxx_clockevent);
>> +    clocksource_register(&pnx_clocksource);
>> +
>> +    setup_irq(PNX8550_INT_TIMER1, &pnx8xxx_timer_irq);
>> +    setup_irq(PNX8550_INT_TIMER2, &monotonic_irqaction);
>> +
>> +    /* Timer 1 start */
>> +    configPR = read_c0_config7();
>> +    configPR &= ~0x00000008;
>> +    write_c0_config7(configPR);
>> +
>> +    /* Timer 2 start */
>> +    configPR = read_c0_config7();
>> +    configPR &= ~0x00000010;
>> +    write_c0_config7(configPR);
>> +
>> +    /* Timer 3 stop */
>> +    configPR = read_c0_config7();
>> +    configPR |= 0x00000020;
>> +    write_c0_config7(configPR);
>
>    Enabling timers before they are actually set up? :-|

Are the additional timers used at all?

  Ralf

Re: pnx8xxx: move to clocksource

[Vitaly Wool]

> >    Something probably have converted tabs to 8 spaces...

No, it's just the lines of original code moved.

> > [...]
> >
> >> +static struct clock_event_device pnx8xxx_clockevent = {
> >> +    .name        = "pnx8xxx_clockevent",
> >> +    .features    = CLOCK_EVT_FEAT_ONESHOT,
> >
> >    Aren't PNX8550 timers actually periodic in nature?
>
> All I recall is they were odd ;-)
>
> The hardware nature of timers and how to declare them to the Linux timer
> code is not always the same.  CLOCK_EVT_FEAT_ONESHOT be used if the
> time to the next shot can be programmed.

Absolutely :)

> >    Enabling timers before they are actually set up? :-|
>
> Are the additional timers used at all?

Heh, that also was the original code which was just moved from one
place in the file to the other.

Sergei, I decided not to mix whitespace cleanups and the clocksource
changes. I'll come up with the whitespace/tab fixups soon arranging it
as a separate patch.

Vitaly

Re: pnx8xxx: move to clocksource

[Sergei Shtylyov]

Ralf Baechle wrote:

> On Thu, Jan 10, 2008 at 07:21:17PM +0300, Sergei Shtylyov wrote:

>>>Index: linux-2.6/arch/mips/philips/pnx8550/common/time.c
>>>===================================================================
>>>--- linux-2.6.orig/arch/mips/philips/pnx8550/common/time.c
>>>+++ linux-2.6/arch/mips/philips/pnx8550/common/time.c
>>>@@ -22,7 +22,6 @@
>>>#include <linux/kernel_stat.h>
>>>#include <linux/spinlock.h>
>>>#include <linux/interrupt.h>
>>>-#include <linux/module.h>
>>>
>>>#include <asm/bootinfo.h>
>>>#include <asm/cpu.h>
>>>@@ -41,11 +40,60 @@ static cycle_t hpt_read(void)
>>>    return read_c0_count2();
>>>}
>>
>>>+static struct clocksource pnx_clocksource = {
>>>+    .name        = "pnx8xxx",
>>>+    .rating        = 200,
>>>+    .read        = hpt_read,
>>>+    .flags        = CLOCK_SOURCE_IS_CONTINUOUS,
>>>+};

>>   Something probably have converted tabs to 8 spaces...

> Only 3 of them?

    Erm, it's format=flowed that spoils the tabs for Mozilla which renders 
them to 4 spaces, so it's actually hard to see which tabs are actually tabs 
and which are not... :-/

>>   I would have done it otherwise -- using timer 1 as a generic MIPS 
>>clocksource (just hooking the IRQ to reload the comparator to all ones), 
>>and timer 2 as clockevent...

>>>static void timer_ack(void)
>>>{
>>>    write_c0_compare(cpj);
>>>}

>>   Do we still need this function? I don't think so -- mips_timer_ack() is 
>>dead...

> It's only used on initialization.

    Could have put the call inline, or loaded the comparator with all ones 
just like timer 2 -- we don't need 'cpj' variable any more as well...

>>[...]

>>>+static struct clock_event_device pnx8xxx_clockevent = {
>>>+    .name        = "pnx8xxx_clockevent",
>>>+    .features    = CLOCK_EVT_FEAT_ONESHOT,

>>   Aren't PNX8550 timers actually periodic in nature?

> All I recall is they were odd ;-)

> The hardware nature of timers and how to declare them to the Linux timer
> code is not always the same.  CLOCK_EVT_FEAT_ONESHOT be used if the
> time to the next shot can be programmed.

    I meant that both modes should have been indicated by the flags.
And actually, shouldn't we disable the timer after expiry if in one-shot mode.
Writing to the comparator doesn't clear the counter, AFAICS -- so, isn't the 
explicit counter clearing to 0 needed in set_next_event() method?

>>>__init void plat_time_init(void)
>>>{
>>>+    unsigned int             configPR;

>>  Something has definitely spoilt all the tabs in the patch...

> I fixed the three checkpatch.pl was bitching about.

    All the others were due to the way format=flowed is rendered by Mozilla it 
seems...

>>>    unsigned int             n;
>>>    unsigned int             m;
>>>    unsigned int             p;
>>>    unsigned int             pow2p;
>>>
>>>+    clockevents_register_device(&pnx8xxx_clockevent);
>>>+    clocksource_register(&pnx_clocksource);
>>>+
>>>+    setup_irq(PNX8550_INT_TIMER1, &pnx8xxx_timer_irq);
>>>+    setup_irq(PNX8550_INT_TIMER2, &monotonic_irqaction);
>>>+
>>>+    /* Timer 1 start */
>>>+    configPR = read_c0_config7();
>>>+    configPR &= ~0x00000008;
>>>+    write_c0_config7(configPR);
>>>+
>>>+    /* Timer 2 start */
>>>+    configPR = read_c0_config7();
>>>+    configPR &= ~0x00000010;
>>>+    write_c0_config7(configPR);
>>>+
>>>+    /* Timer 3 stop */
>>>+    configPR = read_c0_config7();
>>>+    configPR |= 0x00000020;
>>>+    write_c0_config7(configPR);

>>   Enabling timers before they are actually set up? :-|

> Are the additional timers used at all?

    Additional == timer 3? It's not used, only 1 and 2 are -- and their 
count/compare registers are initialized further in this function...

>   Ralf

WBR, Sergei

Re: pnx8xxx: move to clocksource

[Ralf Baechle]

On Thu, Jan 10, 2008 at 07:48:57PM +0300, Vitaly Wool wrote:

> Sergei, I decided not to mix whitespace cleanups and the clocksource
> changes. I'll come up with the whitespace/tab fixups soon arranging it
> as a separate patch.

Generally that's a good approach - but for lines which are modified anyway
I see no reason not to clean the whitespace stuff anyway.

  Ralf