clocks on mips

[Garrett D'Amore]

I've been looking at arc.

It appears that all arc systems have a MIPS 3 clock and can handle clock
interrupts on CP0 counter.  I think this is probably also true of
hpcmips.  For pmax and company, it is probably true for _some_ models.

I'd like to recommend the following:

1) convert those models that can do it to common mips3 clock code in
mips/mips/mips3_clock.c, modulo the following comments

2) this means in the short term, loss of statclock.  i propose that the
statclock be handled by the external clocks (whatever are present) when
they exist.  i.e. I'm suggesting that the statclock timer be the
"optional" clock, and that the mips3_cp0_clock always be used.
statclock/stathz, etc. should be set up by the MD code.

3) this probably means cleaning up mips3_clock.c somewhat -- the
statclock needs to move out of it, and the delay() needs to rename to
mips3_delay.

4) for systems that can just use mips3_delay as is, I would use a weak
symbol alias so that at link time delay() is resolved to mips3_delay.

5) I'd like to rename cpu_initclocks() to mips3_initclocks() and weak
alias it as well.

6) I'd like to move the resulting mips3_initclocks() and
mips3_clockintr() into a new file, mips3_hardclock.c or
mips3_clockintr.c, so that ports which for some reason can't use these
(ews4800mips?) don't have to carry the baggage, but can still use the
rest of the code in mips3_clock.c

Thoughts?

--
Garrett D'Amore, Principal Software Engineer
Tadpole Computer / Computing Technologies Division,
General Dynamics C4 Systems
http://www.tadpolecomputer.com/
Phone: [phone]  Fax: [phone]

[Izumi Tsutsui]

garrett_damore@... wrote:

> It appears that all arc systems have a MIPS 3 clock and can handle clock
> interrupts on CP0 counter.
>  I think this is probably also true of
> hpcmips.

hpcmips also supports TX39xx machines and IIRC they are MIPS1.

> 1) convert those models that can do it to common mips3 clock code in
> mips/mips/mips3_clock.c, modulo the following comments

Maybe arc could switch (I guess soda also had the same idea),
but one concern is that clock frequencies defined in arc/arc/p_*.c
files for each platform are not confirmed, because unlike sgimips
most ARC BIOS onarc machines don't have clock frequency info.
Actually I notice it was wrong for my Express5800/230 while ago.
We should some clock measurement function like hp300/clock.c,
but I don't have good idea for it right now.

Note current interupt code in arc is a bit messed up.
I and soda always discuss that they should be reorganized, but... ;-p

> 2) this means in the short term, loss of statclock.  i propose that the
> statclock be handled by the external clocks (whatever are present) when
> they exist.  i.e. I'm suggesting that the statclock timer be the
> "optional" clock, and that the mips3_cp0_clock always be used.
> statclock/stathz, etc. should be set up by the MD code.

statclock(9) is recommended to have some "variance" in its interval
for precise statistics (see hp300/clock.c or the Design and
implementation of 4.4BSD), so I'm not sure if we can use the existing
external clocks for it because most of them have only fixed interval.

But I don't mind just removing statclock implementations with CPU INT5
from arc since I can't see particular performance improvement with it.

BTW, current mips3_clock.c:mips3_clockintr() doesn't handle
lost clock interrupts properly. (just adjusting the next compare)
I wonder if it's worth to count how many interrupts were lost
and call hardclock(9) more than once, like current
arc/timer.c:statclockintr() and macppc/clock.c:decr_intr() do.

> 3) this probably means cleaning up mips3_clock.c somewhat -- the
> statclock needs to move out of it, and the delay() needs to rename to
> mips3_delay.
>
> 4) for systems that can just use mips3_delay as is, I would use a weak
> symbol alias so that at link time delay() is resolved to mips3_delay.
>
> 5) I'd like to rename cpu_initclocks() to mips3_initclocks() and weak
> alias it as well.

Yeah, these are what I asked.

> 6) I'd like to move the resulting mips3_initclocks() and
> mips3_clockintr() into a new file, mips3_hardclock.c or
> mips3_clockintr.c, so that ports which for some reason can't use these
> (ews4800mips?) don't have to carry the baggage, but can still use the
> rest of the code in mips3_clock.c

Or to have some __HAVE_MIPS_NO_INT5_CLOCK or something in types.h?
(and we should get rid of options MIPS3_ENABLE_CLOCK_INTR)
---
Izumi Tsutsui

[Garrett D'Amore]

Izumi Tsutsui wrote:

> garrett_damore@... wrote:
>
>  
>> It appears that all arc systems have a MIPS 3 clock and can handle clock
>> interrupts on CP0 counter.
>>  I think this is probably also true of
>> hpcmips.
>>    
>
> hpcmips also supports TX39xx machines and IIRC they are MIPS1.
>
>  
>> 1) convert those models that can do it to common mips3 clock code in
>> mips/mips/mips3_clock.c, modulo the following comments
>>    
>
> Maybe arc could switch (I guess soda also had the same idea),
> but one concern is that clock frequencies defined in arc/arc/p_*.c
> files for each platform are not confirmed, because unlike sgimips
> most ARC BIOS onarc machines don't have clock frequency info.
> Actually I notice it was wrong for my Express5800/230 while ago.
> We should some clock measurement function like hp300/clock.c,
> but I don't have good idea for it right now.
>
> Note current interupt code in arc is a bit messed up.
> I and soda always discuss that they should be reorganized, but... ;-p
>  

Malta currently does this.  A realtime clock (if present) can be used to
set the cpu freq in MD code.  If you don't have _any_ real-time
reference, then you're hosed anyway.

>  
>> 2) this means in the short term, loss of statclock.  i propose that the
>> statclock be handled by the external clocks (whatever are present) when
>> they exist.  i.e. I'm suggesting that the statclock timer be the
>> "optional" clock, and that the mips3_cp0_clock always be used.
>> statclock/stathz, etc. should be set up by the MD code.
>>    
>
> statclock(9) is recommended to have some "variance" in its interval
> for precise statistics (see hp300/clock.c or the Design and
> implementation of 4.4BSD), so I'm not sure if we can use the existing
> external clocks for it because most of them have only fixed interval.
>
> But I don't mind just removing statclock implementations with CPU INT5
> from arc since I can't see particular performance improvement with it.
>  

Agreed.  statclock() is optional anyway.  The issue is that you want to
avoid aliasing the cpu clock and the statclock.  With MIPS3 cp0 counter,
if this is a problem, one can just adjust HZ for the port.  (I.e. the
cp0 counter is still used, but we write a different value into cp0_compare.

> BTW, current mips3_clock.c:mips3_clockintr() doesn't handle
> lost clock interrupts properly. (just adjusting the next compare)
> I wonder if it's worth to count how many interrupts were lost
> and call hardclock(9) more than once, like current
> arc/timer.c:statclockintr() and macppc/clock.c:decr_intr() do.
>  

I'll take a look at it.

>  
>> 3) this probably means cleaning up mips3_clock.c somewhat -- the
>> statclock needs to move out of it, and the delay() needs to rename to
>> mips3_delay.
>>
>> 4) for systems that can just use mips3_delay as is, I would use a weak
>> symbol alias so that at link time delay() is resolved to mips3_delay.
>>
>> 5) I'd like to rename cpu_initclocks() to mips3_initclocks() and weak
>> alias it as well.
>>    
>
> Yeah, these are what I asked.
>  

I know.  Its time to clean it up now.  I'm offering to do so.

>  
>> 6) I'd like to move the resulting mips3_initclocks() and
>> mips3_clockintr() into a new file, mips3_hardclock.c or
>> mips3_clockintr.c, so that ports which for some reason can't use these
>> (ews4800mips?) don't have to carry the baggage, but can still use the
>> rest of the code in mips3_clock.c
>>    
>
> Or to have some __HAVE_MIPS_NO_INT5_CLOCK or something in types.h?
> (and we should get rid of options MIPS3_ENABLE_CLOCK_INTR)
>  

Maybe.  I don't know.  For now putting it in a separate file seems
cleaner than having more garbage in types.h.  But that's just my opinion.

    -- Garrett
> ---
> Izumi Tsutsui
>  


--
Garrett D'Amore, Principal Software Engineer
Tadpole Computer / Computing Technologies Division,
General Dynamics C4 Systems
http://www.tadpolecomputer.com/
Phone: [phone]  Fax: [phone]

[Garrett D'Amore]

Izumi Tsutsui wrote:
>
>
> BTW, current mips3_clock.c:mips3_clockintr() doesn't handle
> lost clock interrupts properly. (just adjusting the next compare)
> I wonder if it's worth to count how many interrupts were lost
> and call hardclock(9) more than once, like current
> arc/timer.c:statclockintr() and macppc/clock.c:decr_intr() do.
>  
How about the following rewrite?  I've not tested it yet, but I'm a
little concerned about the potential implications of calling hardclock
repeatedly to "catch up" lost interrupts.  Especially as it effects ntp
and timecounters.  I'd like to hear opinions on the matter.

I also added an evcnt for missed clock interrupts.


struct evcnt mips_int5_missed_evcnt =
    EVCNT_INITIALIZER(EVCNT_TYPE_INTR, NULL, "mips", "missed int 5");



/*
 * Handling to be done upon receipt of a MIPS 3 clock interrupt.  This
 * routine is to be called from the master interrupt routine
 * (e.g. cpu_intr), if MIPS INT5 is pending.  The caller is
 * responsible for blocking and renabling the interrupt in the
 * cpu_intr() routine.
 */
void
mips3_clockintr(uint32_t status, uint32_t pc)
{
    uint32_t        new_cnt, delta, lost = 0;
    struct clockframe    cf;

    cf.pc = pc;
    cf.sr = status;

    next_cp0_clk_intr += curcpu()->ci_cycles_per_hz;
    mips3_cp0_compare_write(next_cp0_clk_intr);

    /* Check for lost clock interrupts */
    new_cnt = mips3_cp0_count_read();

    delta = next_cp0_clk_intr - new_cnt;
    while (__predict_false((int32_t)delta < 0)) {
        lost++;
        delta += curcpu()->ci_cycles_per_hz;
    }

    /*
     * Missed one or more clock interrupts, so let's start
     * counting again from the current value.  We also trigger
     * hardclocks to get the clock interrupt back up to speed.
     *
     * XXX: What impact will this have on NTP and timecounters?
     */
    if (__predict_false(lost > 0)) {
        next_cp0_clk_intr = new_cnt + curcpu()->ci_cycles_per_hz;
        mips3_cp0_compare_write(next_cp0_clk_intr);
        for (; lost > 0; lost--) {
            hardclock(cfp);
            mips_int5_evcnt.ev_count++;
        }
    }

    hardclock(&cf);

    mips_int5_evcnt.ev_count++;

    /* caller should renable clock interrupts */
}

>  
>> 3) this probably means cleaning up mips3_clock.c somewhat -- the
>> statclock needs to move out of it, and the delay() needs to rename to
>> mips3_delay.
>>
>> 4) for systems that can just use mips3_delay as is, I would use a weak
>> symbol alias so that at link time delay() is resolved to mips3_delay.
>>
>> 5) I'd like to rename cpu_initclocks() to mips3_initclocks() and weak
>> alias it as well.
>>    
>
> Yeah, these are what I asked.
>
>  
>> 6) I'd like to move the resulting mips3_initclocks() and
>> mips3_clockintr() into a new file, mips3_hardclock.c or
>> mips3_clockintr.c, so that ports which for some reason can't use these
>> (ews4800mips?) don't have to carry the baggage, but can still use the
>> rest of the code in mips3_clock.c
>>    
>
> Or to have some __HAVE_MIPS_NO_INT5_CLOCK or something in types.h?
> (and we should get rid of options MIPS3_ENABLE_CLOCK_INTR)
> ---
> Izumi Tsutsui
>  

--
Garrett D'Amore, Principal Software Engineer
Tadpole Computer / Computing Technologies Division,
General Dynamics C4 Systems
http://www.tadpolecomputer.com/
Phone: [phone]  Fax: [phone]

[Izumi Tsutsui]

garrett_damore@... wrote:

> How about the following rewrite?  I've not tested it yet, but I'm a
> little concerned about the potential implications of calling hardclock
> repeatedly to "catch up" lost interrupts.  Especially as it effects ntp
> and timecounters.  I'd like to hear opinions on the matter.

I have no idea. I'd like to hear opinions of timecounter guys :-)

> I also added an evcnt for missed clock interrupts.
>
> struct evcnt mips_int5_missed_evcnt =
>     EVCNT_INITIALIZER(EVCNT_TYPE_INTR, NULL, "mips", "missed int 5");

Yeah, I wonder how many interrupts are actually lost.

>     if (__predict_false(lost > 0)) {
>         next_cp0_clk_intr = new_cnt + curcpu()->ci_cycles_per_hz;

This should be "new_cnt + delta" to keep intervals precisely?

>         mips3_cp0_compare_write(next_cp0_clk_intr);
>         for (; lost > 0; lost--) {
>             hardclock(cfp);
>             mips_int5_evcnt.ev_count++;

hardclock(&cp) and mips_int5_missed_evcnt.ev_count?

>         }
>     }

It's probably better to control hardclock(9) except last call
not to call spllowersoftclock(9) by tweaking clockframe:
---
      if (__predict_false(lost > 0)) {
          int sr;
          next_cp0_clk_intr = new_cnt + delta;
          mips3_cp0_compare_write(next_cp0_clk_intr);
          sr = cf.sr;
          cf.sr &= ~MIPS_SR_INT_IE;
          for (; lost > 0; lost--) {
              hardclock(&cf);
              mips_int5_missed_evcnt.ev_count++;
          }
          cf.sr = sr;
      }
---
But I'd appreciate any comments from interrupt gurus.

---
Izumi Tsutsui

[Garrett D'Amore]

Izumi Tsutsui wrote:

> garrett_damore@... wrote:
>
>  
>> How about the following rewrite?  I've not tested it yet, but I'm a
>> little concerned about the potential implications of calling hardclock
>> repeatedly to "catch up" lost interrupts.  Especially as it effects ntp
>> and timecounters.  I'd like to hear opinions on the matter.
>>    
>
> I have no idea. I'd like to hear opinions of timecounter guys :-)
>  

I talked with Frank about this at some length.  It shouldn't matter one
way or the other.

>  
>> I also added an evcnt for missed clock interrupts.
>>
>> struct evcnt mips_int5_missed_evcnt =
>>     EVCNT_INITIALIZER(EVCNT_TYPE_INTR, NULL, "mips", "missed int 5");
>>    
>
> Yeah, I wonder how many interrupts are actually lost.
>  

Actually, as I think about this, the only reason we should miss
interrupts is someone is doing splhigh()/splclock() for longer than a
clock tick.  That shouldn't happen, outside of someone doing stuff in ddb.

So I'm actually thinking that using the stat counter so we find out if
this is the case or not is the right thing to do -- don't bother with
any of the rest of the handling other than what we already have.

>  
>>     if (__predict_false(lost > 0)) {
>>         next_cp0_clk_intr = new_cnt + curcpu()->ci_cycles_per_hz;
>>    
>
> This should be "new_cnt + delta" to keep intervals precisely?
>  

I don't think so.  I think it is best to get back to a regular interrupt
period.   Basically, this ensures that the next interrupt is schedule
for tick cycles ahead.  The delta would make the period longer, which
result in an extended time to the next interrupt.  (I.e. the next clock
could be several ticks long.)

In any case, I don't think missed interrupts (at least on INT5) happen
in practice outside of ddb, so I don't think we should be worrying about it.

This conclusion is based on the assumption that no port is doing
anything stupid like sharing INT5 with some other device....  and that
driver and kernel code is well behaved and doesn't do
splclock()/splhigh() for long periods of time.

>  
>>         mips3_cp0_compare_write(next_cp0_clk_intr);
>>         for (; lost > 0; lost--) {
>>             hardclock(cfp);
>>             mips_int5_evcnt.ev_count++;
>>    
>
> hardclock(&cp) and mips_int5_missed_evcnt.ev_count?
>  

Yes.  Doh.  Nice catch.

>  
>>         }
>>     }
>>    
>
> It's probably better to control hardclock(9) except last call
> not to call spllowersoftclock(9) by tweaking clockframe:
> ---
>       if (__predict_false(lost > 0)) {
>           int sr;
>           next_cp0_clk_intr = new_cnt + delta;
>           mips3_cp0_compare_write(next_cp0_clk_intr);
>           sr = cf.sr;
>           cf.sr &= ~MIPS_SR_INT_IE;
>           for (; lost > 0; lost--) {
>               hardclock(&cf);
>               mips_int5_missed_evcnt.ev_count++;
>           }
>           cf.sr = sr;
>       }
> ---
> But I'd appreciate any comments from interrupt gurus.
>  

Actually, the complexity here more than helps convince me that we
shouldn't be doing any of this.  It probably never occurs, and we're
more likely to screw it up by trying to handle it.

Many ports have not worried about this before, and seen no ill effects, btw.

    -- Garrett
> ---
> Izumi Tsutsui
>  


--
Garrett D'Amore, Principal Software Engineer
Tadpole Computer / Computing Technologies Division,
General Dynamics C4 Systems
http://www.tadpolecomputer.com/
Phone: [phone]  Fax: [phone]