Re: [PATCH v2 2/2] cpufreq: powernv: Use PMCR to verify global and local pstate

[Akshay Adiga]

Thanks gautham for the review.

Good point, I have made the macros more generic in the next version as 
you have mentioned.

I will post a separate patch to set pstates using these macros. :)

On 11/08/2016 09:10 AM, Gautham R Shenoy wrote:
> On Mon, Nov 07, 2016 at 01:09:09PM +0530, Akshay Adiga wrote:
> > As fast_switch() may get called with interrupt disable mode, we cannot
> > hold a mutex to update the global_pstate_info. So currently, fast_switch()
> > does not update the global_pstate_info and it will end up with stale data
> > whenever pstate is updated through fast_switch().
> >
> > As the gpstate_timer can fire after fast_switch() has updated the pstates,
> > the timer handler cannot rely on the cached values of local and global
> > pstate and needs to read it from the PMCR.
> >
> > Only gpstate_timer_handler() is affected by the stale cached pstate data
> > beacause either fast_switch() or target_index() routines will be called
> > for a given govenor, but gpstate_timer can fire after the governor has
> > changed to schedutil.
> >
> >
> > Signed-off-by: Akshay Adiga <akshay.adiga@xxxxxxxxxxxxxxxxxx>
> > ---
> >
> > Changes from v1 :
> > - Corrected Commit message
> > - Type cast pstate values read from PMCR to type s8
> > - Added Macros to get local and global pstates from PMCR
> Thanks for this. Could you also send a (separate patch) to set the
> local and global pstates to PMCR in set_pstate?
>
> >   drivers/cpufreq/powernv-cpufreq.c | 34 ++++++++++++++++++++++++----------
> >   1 file changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> > index 4a4380d..bf4bc585 100644
> > --- a/drivers/cpufreq/powernv-cpufreq.c
> > +++ b/drivers/cpufreq/powernv-cpufreq.c
> > @@ -42,6 +42,8 @@
> >   #define PMSR_PSAFE_ENABLE	(1UL << 30)
> >   #define PMSR_SPR_EM_DISABLE	(1UL << 31)
> >   #define PMSR_MAX(x)		((x >> 32) & 0xFF)
> > +#define PMCR_LPSTATE(x)		(((x) >> 48) & 0xFF)
> > +#define PMCR_GPSTATE(x)		(((x) >> 56) & 0xFF)
> You define:
> #define LPSTATE_SHIFT    48
> #define GPSTATE_SHIFT    56
>
> since we can use this in the set_variants.
>
> Moreover, the LPSTATE, GPSTATE retreival is applicable to both PMCR and PMSR. So
> could you rename these functions to GET_LPSTATE, GET_GPSTATE.
>
> Similarly, we might want to have a SET_LPSTATE, SET_GPSTATE and fix
> the hard coded values that we have in set_pstate.
>
>
> >   #define MAX_RAMP_DOWN_TIME				5120
> >   /*
> > @@ -592,7 +594,8 @@ void gpstate_timer_handler(unsigned long data)
> >   {
> >   	struct cpufreq_policy *policy = (struct cpufreq_policy *)data;
> >   	struct global_pstate_info *gpstates = policy->driver_data;
> > -	int gpstate_idx;
> > +	int gpstate_idx, lpstate_idx;
> > +	unsigned long val;
> >   	unsigned int time_diff = jiffies_to_msecs(jiffies)
> >   					- gpstates->last_sampled_time;
> >   	struct powernv_smp_call_data freq_data;
> > @@ -600,21 +603,36 @@ void gpstate_timer_handler(unsigned long data)
> >   	if (!spin_trylock(&gpstates->gpstate_lock))
> >   		return;
> >
> > +	/*
> > +	 * If PMCR was last updated was using fast_swtich then
> > +	 * We may have wrong in gpstate->last_lpstate_idx
> > +	 * value. Hence, read from PMCR to get correct data.
> > +	 */
> > +	val = get_pmspr(SPRN_PMCR);
> > +	freq_data.gpstate_id = (s8)PMCR_GPSTATE(val);
> > +	freq_data.pstate_id = (s8)PMCR_LPSTATE(val);
> > +	if (freq_data.gpstate_id  == freq_data.pstate_id) {
> > +		reset_gpstates(policy);
> > +		spin_unlock(&gpstates->gpstate_lock);
> > +		return;
> > +	}
> > +
> >   	gpstates->last_sampled_time += time_diff;
> >   	gpstates->elapsed_time += time_diff;
> > -	freq_data.pstate_id = idx_to_pstate(gpstates->last_lpstate_idx);
> >
> > -	if ((gpstates->last_gpstate_idx == gpstates->last_lpstate_idx) ||
> > -	    (gpstates->elapsed_time > MAX_RAMP_DOWN_TIME)) {
> > +	if (gpstates->elapsed_time > MAX_RAMP_DOWN_TIME) {
> >   		gpstate_idx = pstate_to_idx(freq_data.pstate_id);
> >   		reset_gpstates(policy);
> >   		gpstates->highest_lpstate_idx = gpstate_idx;
> >   	} else {
> > +		lpstate_idx = pstate_to_idx(freq_data.pstate_id);
> >   		gpstate_idx = calc_global_pstate(gpstates->elapsed_time,
> >   						 gpstates->highest_lpstate_idx,
> > -						 gpstates->last_lpstate_idx);
> > +						 lpstate_idx);
> >   	}
> > -
> > +	freq_data.gpstate_id = idx_to_pstate(gpstate_idx);
> > +	gpstates->last_gpstate_idx = gpstate_idx;
> > +	gpstates->last_lpstate_idx = lpstate_idx;
> >   	/*
> >   	 * If local pstate is equal to global pstate, rampdown is over
> >   	 * So timer is not required to be queued.
> > @@ -622,10 +640,6 @@ void gpstate_timer_handler(unsigned long data)
> >   	if (gpstate_idx != gpstates->last_lpstate_idx)
> >   		queue_gpstate_timer(gpstates);
> >
> > -	freq_data.gpstate_id = idx_to_pstate(gpstate_idx);
> > -	gpstates->last_gpstate_idx = pstate_to_idx(freq_data.gpstate_id);
> > -	gpstates->last_lpstate_idx = pstate_to_idx(freq_data.pstate_id);
> > -
> >   	spin_unlock(&gpstates->gpstate_lock);
> >
> >   	/* Timer may get migrated to a different cpu on cpu hot unplug */
> > --
> > 2.5.5
> Looks good otherwise.
>
> Reviewed-by: Gautham R. Shenoy <ego@xxxxxxxxxxxxxxxxxx>