Patch: Generic TODR for ARM ports

On Sun, Sep 17, 2006 at 09:15:55AM -0700, Garrett D'Amore
wrote:
> Peter Postma wrote:
> > timecounter: Timecounters tick every 10.000 msec
> > timecounter: Timecounter
"clockinterrupt" frequency 100 Hz quality 0
> > timecounter: selected timecounter
"clockinterrupt" frequency 100 Hz quality 0
> > timecounter: Timecounter "saost_count"
frequency 3686400 Hz quality 100
> > timecounter: selected timecounter
"saost_count" frequency 3686400 Hz quality 100
> > timecounter: selected timecounter
"saost_count" frequency 3686400 Hz quality 100
> >   
> 
> The double selection messages suggest that you need to
update the kernel
> from CVS.  A change was made to todr to prevent
tc_windup() from getting
> called with interrupts enabled -- I think that is what
has happened
> here.  (You should not see double messages like this.)

I believe that I'm up to date  which
revisions should I have?

> > I can put the diffs online for review, but they
are really trivial.
> >   
> 
> That would be great.  If they are generic, it would be
a good idea to
> wrap them in #ifdef __HAVE_TIMECOUNTER so that evbarm
can pick them up too.
> 

Yes, saost is also used by at least 3 evbarm ports. Diff is
here:
ftp://ftp.netbsd.org/pub/NetBSD/misc/peter/saost-tc.diff

-- 
Peter Postma

Peter Postma wrote:
> On Sun, Sep 17, 2006 at 09:15:55AM -0700, Garrett
D'Amore wrote:
>   
>> Peter Postma wrote:
>>     
>>> timecounter: Timecounters tick every 10.000
msec
>>> timecounter: Timecounter
"clockinterrupt" frequency 100 Hz quality 0
>>> timecounter: selected timecounter
"clockinterrupt" frequency 100 Hz quality 0
>>> timecounter: Timecounter
"saost_count" frequency 3686400 Hz quality 100
>>> timecounter: selected timecounter
"saost_count" frequency 3686400 Hz quality 100
>>> timecounter: selected timecounter
"saost_count" frequency 3686400 Hz quality 100
>>>   
>>>       
>> The double selection messages suggest that you need
to update the kernel
>> from CVS.  A change was made to todr to prevent
tc_windup() from getting
>> called with interrupts enabled -- I think that is
what has happened
>> here.  (You should not see double messages like
this.)
>>     
>
> I believe that I'm up to date  which
revisions should I have?
>
>   
>>> I can put the diffs online for review, but they
are really trivial.
>>>   
>>>       
>> That would be great.  If they are generic, it would
be a good idea to
>> wrap them in #ifdef __HAVE_TIMECOUNTER so that
evbarm can pick them up too.
>>
>>     
>
> Yes, saost is also used by at least 3 evbarm ports.
Diff is here:
>
ftp://ftp.netbsd.org/pub/NetBSD/misc/peter/saost-tc.diff
>
>   

This looks great to me.  I'd say post it, but I'm still
waiting on
approval to commit the GENERIC-TODR changes.  Wait a little
bit, and
I'll tell you when it is safe to commit...

-- 
Garrett D'Amore, Principal Software Engineer
Tadpole Computer / Computing Technologies Division,
General Dynamics C4 Systems
http://www.tadpolecom
puter.com/
Phone: 951 325-2134  Fax: 951 325-2191

Peter Postma wrote:
> On Sun, Sep 17, 2006 at 09:15:55AM -0700, Garrett
D'Amore wrote:
>   
>> Peter Postma wrote:
>>     
>>> timecounter: Timecounters tick every 10.000
msec
>>> timecounter: Timecounter
"clockinterrupt" frequency 100 Hz quality 0
>>> timecounter: selected timecounter
"clockinterrupt" frequency 100 Hz quality 0
>>> timecounter: Timecounter
"saost_count" frequency 3686400 Hz quality 100
>>> timecounter: selected timecounter
"saost_count" frequency 3686400 Hz quality 100
>>> timecounter: selected timecounter
"saost_count" frequency 3686400 Hz quality 100
>>>   
>>>       
>> The double selection messages suggest that you need
to update the kernel
>> from CVS.  A change was made to todr to prevent
tc_windup() from getting
>> called with interrupts enabled -- I think that is
what has happened
>> here.  (You should not see double messages like
this.)
>>     
>
> I believe that I'm up to date  which
revisions should I have?
>   

1.12 of kern_tc.c is what you need.

This version calls tc_windup() inside splclock().  Earlier
versions didn't.

    -- Garrett

-- 
Garrett D'Amore, Principal Software Engineer
Tadpole Computer / Computing Technologies Division,
General Dynamics C4 Systems
http://www.tadpolecom
puter.com/
Phone: 951 325-2134  Fax: 951 325-2191

On Sun, Sep 17, 2006 at 07:37:26PM -0700, Garrett D'Amore
wrote:
> Peter Postma wrote:
> > On Sun, Sep 17, 2006 at 09:15:55AM -0700, Garrett
D'Amore wrote:
> >> The double selection messages suggest that you
need to update the kernel
> >> from CVS.  A change was made to todr to
prevent tc_windup() from getting
> >> called with interrupts enabled -- I think that
is what has happened
> >> here.  (You should not see double messages
like this.)
> >>     
> >
> > I believe that I'm up to date  which
revisions should I have?
> >   
> 
> 1.12 of kern_tc.c is what you need.
> 
> This version calls tc_windup() inside splclock(). 
Earlier versions didn't.
> 

I'm at 1.12. I debugged a bit and it seems that when I
change the splclock()
to splhigh(), the double messages are gone. So this might be
a hpcarm
specific problem.

-- 
Peter Postma

Peter Postma wrote:
> On Sun, Sep 17, 2006 at 07:37:26PM -0700, Garrett
D'Amore wrote:
>   
>> Peter Postma wrote:
>>     
>>> On Sun, Sep 17, 2006 at 09:15:55AM -0700,
Garrett D'Amore wrote:
>>>       
>>>> The double selection messages suggest that
you need to update the kernel
>>>> from CVS.  A change was made to todr to
prevent tc_windup() from getting
>>>> called with interrupts enabled -- I think
that is what has happened
>>>> here.  (You should not see double messages
like this.)
>>>>     
>>>>         
>>> I believe that I'm up to date  which
revisions should I have?
>>>   
>>>       
>> 1.12 of kern_tc.c is what you need.
>>
>> This version calls tc_windup() inside splclock(). 
Earlier versions didn't.
>>
>>     
>
> I'm at 1.12. I debugged a bit and it seems that when I
change the splclock()
> to splhigh(), the double messages are gone. So this
might be a hpcarm
> specific problem.
>
>   

Hmm... maybe splclock() isn't really blocking the clock
interrupt?  It
certainly sounds like an hpcarm problem.

-- 
Garrett D'Amore, Principal Software Engineer
Tadpole Computer / Computing Technologies Division,
General Dynamics C4 Systems
http://www.tadpolecom
puter.com/
Phone: 951 325-2134  Fax: 951 325-2191

On Mon, Sep 18, 2006 at 07:58:51PM +0200, Peter Postma
wrote:
> On Sun, Sep 17, 2006 at 07:37:26PM -0700, Garrett
D'Amore wrote:
> > Peter Postma wrote:
> > > On Sun, Sep 17, 2006 at 09:15:55AM -0700,
Garrett D'Amore wrote:
> > >> The double selection messages suggest
that you need to update the kernel
> > >> from CVS.  A change was made to todr to
prevent tc_windup() from getting
> > >> called with interrupts enabled -- I think
that is what has happened
> > >> here.  (You should not see double
messages like this.)
> > >
> > > I believe that I'm up to date  which
revisions should I have?
> > 
> > 1.12 of kern_tc.c is what you need.
> > 
> > This version calls tc_windup() inside splclock(). 
Earlier versions didn't.
> 
> I'm at 1.12. I debugged a bit and it seems that when I
change the splclock()
> to splhigh(), the double messages are gone. So this
might be a hpcarm
> specific problem.
> 

A simple work-around is to replace the following line in
hpcarm/intr.c:

spl_masks[_SPL_CLOCK]      = imask[IPL_CLOCK];

with

spl_masks[_SPL_CLOCK]      = imask[IPL_HIGH];


I wonder if this would be acceptable as _temporary_ fix?
A too much blocking splclock seems better to me as a not
blocking clock
interrupts splclock.

-- 
Peter Postma