Re: Using %stick where available

[Michael <macallan%netbsd.org@localhost>]

Hello,

On Nov 7, 2012, at 7:04 AM, Takeshi Nakayama wrote:

> > > > Michael <macallan%netbsd.org@localhost> wrote
>
> > Hello,
> >
> > the attached patch adds support for the system timer interrupt  
> > present
> > in UltraSPARC-III and some later II ( like IIe and IIi with on chip
> > ecache ). It hasn't seen much testing beyond 'works on my Blade  
> > 2500'.
> > The purpose is to have a timer interrupt / time counter that's
> > independent of the CPU's clock rate, so we can change it without
> > worrying about time keeping.
>
> It looks ok to me, but as Eric reported it needs some fix.

Yeah, I have no US-IIe hardware so I couldn't test it there, which is  
one of the reasons why I posted the patch here first.

> > +       struct cpu_info *ci = curcpu();
>
> Replace the following curcpu()s with ci.

Done.

> I think the following is better for consistency.
>
> -       long clk;
> +       long clk, sclk;

Yeah, I left it an int because that's what we get from the PROM, but  
you're right, they should be the same type.

> > +       sclk = prom_getpropint(findroot(), "stick-frequency", 0);
> > +       ci->ci_system_clockrate[0] = sclk;
> > +       ci->ci_system_clockrate[1] = sclk / 1000000;
>
> US-IIe has system tick register, but its implementation is
> different to US-III one.  It can be used via memory mapped system
> registers, not via ancillary state register (%asr24).

Seriously?
I expected trouble with US-IIe but not quite like that.

> So, I suggest not to use it on US-IIe as below.
>
>         if (!CPU_IS_HUMMINGBIRD()) {
>                 sclk = prom_getpropint(findroot(), "stick-frequency", 0);
>                 ci->ci_system_clockrate[0] = sclk;
>                 ci->ci_system_clockrate[1] = sclk / 1000000;
>         }

Done, slightly changed to make sure ci_system_clockrate[] is 0 if we  
don't have %stick.

> And, put the following into include/psl.h.
>
> #define CPU_IS_HUMMINGBIRD()    (GETVER_CPU_IMPL() == IMPL_HUMMINGBIRD)
> > define  CI_CLOCKRATE    offsetof(struct cpu_info, ci_cpu_clockrate)
> > +define	CI_SYSCLOCKRATE	offsetof(struct cpu_info,  
> > ci_system_clockrate)
> > define  CI_IDEPTH       offsetof(struct cpu_info, ci_idepth)
> > define  CI_INTRPENDING  offsetof(struct cpu_info, ci_intrpending)
> > define  CI_TICK_IH      offsetof(struct cpu_info, ci_tick_ih)
>
> CI_SYSCLOCKRATE is unused.

I put it there because CI_CLOCKRATE is used in delay() which we may  
want to adapt to %stick / CI_SYSCLOCKRATE at some point. I'll leave it  
out in the revised patch.

> > +/*
> > + * setstick(long)
> > + */
> > +ENTRY(setstick)
> > +       retl
> > +        wr %o0, STICK
> > +
> > +/*
> > + * long getstick(void)
> > + */
> > +ENTRY(getstick)
> > +       retl
> > +        rd STICK, %o0
>
> I'll replace them with inline functions like settick/gettick after  
> you commited.

I put them there because I couldn't get the inline assembler to do  
what I want ( serious lack of practice on my part )

> BB_ERRATA_1 is an errata of US-II, so #ifdef BB_ERRATA_1 cases are
> unnecessary.

Removed them.

Attached is the revised patch, thanks for looking at this!

have fun
Michael

Attachment: stick_2.patch
Description: Binary data

Attachment: PGP.sig
Description: This is a digitally signed message part