Re: Using %stick where available

[Takeshi Nakayama <tn%catvmics.ne.jp@localhost>]

Hello,

>>> Michael <macallan%netbsd.org@localhost> wrote

> 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.

Thanks, but there are some left.

> > 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.

They are described in section 2.3 and 4.3.4 in "UltraSPARC IIe
Processor User's Manual".  64-bit stick register is splited to two
32-bit registers, so using it is more complex.

> > 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.

cpu_info structure is zero cleared in alloc_cpuinfo(), but yes
it's more readable to clear it explicitly.


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

You're welcome.  I wrote one more comment, please see below.


> Index: sparc64/clock.c
> ===================================================================
> RCS file: /cvsroot/src/sys/arch/sparc64/sparc64/clock.c,v
> retrieving revision 1.106
> diff -u -w -r1.106 clock.c
> --- sparc64/clock.c   4 Sep 2011 12:17:46 -0000       1.106
> +++ sparc64/clock.c   7 Nov 2012 17:26:58 -0000
(snip)
> @@ -338,6 +378,7 @@
>  void
>  cpu_initclocks(void)
>  {
> +     struct cpu_info *ci = curcpu();
>  #ifndef MULTIPROCESSOR
>       int statint, minint;
>  #endif
> @@ -361,17 +402,24 @@
>       }
>  
>       /* Make sure we have a sane cpu_clockrate -- we'll need it */
> -     if (!curcpu()->ci_cpu_clockrate[0]) {
> +     if (!ci->ci_cpu_clockrate[0]) {
>               /* Default to 200MHz clock XXXXX */
> -             curcpu()->ci_cpu_clockrate[0] = 200000000;
> -             curcpu()->ci_cpu_clockrate[1] = 200000000 / 1000000;
> +             ci->ci_cpu_clockrate[0] = 200000000;
> +             ci->ci_cpu_clockrate[1] = 200000000 / 1000000;
>       }
>  
>       /* Initialize the %tick register */
>       settick(0);
>  
> +     if (ci->ci_system_clockrate[0] == 0) {
>       tick_timecounter.tc_frequency = curcpu()->ci_cpu_clockrate[0];
                                        ~~~~~~~~
>       tc_init(&tick_timecounter);
> +     } else {
> +             setstick(0);
> +             stick_timecounter.tc_frequency = 
> +                 ci->ci_system_clockrate[0];
> +             tc_init(&stick_timecounter);
> +     }
>  
>       /*
>        * Now handle machines w/o counter-timers.
> @@ -379,13 +427,21 @@
>  
>       if (!timerreg_4u.t_timer || !timerreg_4u.t_clrintr) {
>  
> -             aprint_normal("No counter-timer -- using %%tick at %luMHz as "
> -                     "system clock.\n",
> +             if (ci->ci_system_clockrate[0] == 0) {
> +                     aprint_normal("No counter-timer -- using %%tick "
> +                         "at %luMHz as system clock.\n",
>                       (unsigned long)curcpu()->ci_cpu_clockrate[1]);
                                       ~~~~~~~~
>  
>               /* We don't have a counter-timer -- use %tick */
>               tickintr_establish(PIL_CLOCK, tickintr);
> +             } else {
> +                     aprint_normal("No counter-timer -- using %%stick "
> +                         "at %luMHz as system clock.\n",
> +                         (unsigned long)ci->ci_system_clockrate[1]);
>  
> +                     /* We don't have a counter-timer -- use %stick */
> +                     stickintr_establish(PIL_CLOCK, stickintr);
> +             }
>               /* We only have one timer so we have no statclock */
>               stathz = 0;
>  
(snip)
> Index: sparc64/locore.s
> ===================================================================
> RCS file: /cvsroot/src/sys/arch/sparc64/sparc64/locore.s,v
> retrieving revision 1.341
> diff -u -w -r1.341 locore.s
> --- sparc64/locore.s  17 Mar 2012 22:19:53 -0000      1.341
> +++ sparc64/locore.s  7 Nov 2012 17:26:59 -0000
> @@ -3270,13 +3270,18 @@
>       wrpr    %g0, PSTATE_KERN|PSTATE_IG, %pstate     ! DEBUG
>  #endif
>       /*
> -      * If this is a %tick softint, clear it then call interrupt_vector.
> +      * If this is a %tick or %stick softint, clear it then call
> +      * interrupt_vector. Only one of them should be enabled at any given
> +      * time.
>        */
>       rd      SOFTINT, %g1
> -     btst    1, %g1
> +     mov     1, %g5
> +     sllx    %g5, 16, %g3
> +     or      %g5, %g3, %g5

Only "set 0x1001, %g5" is faster.

> +     andcc   %g5, %g1, %g5
>       bz,pt   %icc, 0f
>        sethi  %hi(CPUINFO_VA+CI_TICK_IH), %g3
> -     wr      %g0, 1, CLEAR_SOFTINT
> +     wr      %g0, %g5, CLEAR_SOFTINT
>       ba,pt   %icc, setup_sparcintr
>        LDPTR  [%g3 + %lo(CPUINFO_VA+CI_TICK_IH)], %g5
>  0:
(snip)


-- Takeshi Nakayama