Re: pnx8xxx: move to clocksource

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

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