Re: pnx8xxx: move to clocksource

[Ralf Baechle <ralf@linux-mips.org>]

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