Re: Using %stick where available

Hello,

>>> Michael <[email protected]> wrote

> Hello,
> 
> On Nov 7, 2012, at 7:04 AM, Takeshi Nakayama wrote:
> 
> >>>> Michael <[email protected]> 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