[Skiboot] [PATCH V7 2/3] occ: Fix Pstate ordering for P9

[Michael Neuling]

On Tue, 2017-02-14 at 02:01 +0530, Shilpasri G Bhat wrote:
> In P9 the pstate values are positive. They are continuous set of
> unsigned integers [0 to +N] where Pmax is 0 and Pmin is N. The
> linear ordering of pstates for P9 has changed compared to P8.
> P8 has neagtive pstate values advertised as [0 to -N] where Pmax
> is 0 and Pmin is -N. This patch adds helper routines to abstract
> pstate comparison with pmax and adds sanity pstate limit checks.
> This patch also fixes pstate arithmetic by using labs().

Can you put this comment inline in the code?  There is no mention of Pmax Pmin
where cmp_postive/negative_pstates() is making it difficult to understand what
it's actually doing.

Alternatively, why don't we convert what we get on P8 to positive numbers, so
that all the pstates numbers are positive.  Then we don't need this clumsy
negative/positive code.

Mikey


> Suggested-by: Gautham R. Shenoy <ego at linux.vnet.ibm.com>
> Signed-off-by: Shilpasri G Bhat <shilpa.bhat at linux.vnet.ibm.com>
> ---
> No changes from V6
>  hw/occ.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ---
>  1 file changed, 88 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/occ.c b/hw/occ.c
> index ee76be6..9eb32b4 100644
> --- a/hw/occ.c
> +++ b/hw/occ.c
> @@ -84,6 +84,40 @@ DEFINE_LOG_ENTRY(OPAL_RC_OCC_TIMEOUT,
> OPAL_PLATFORM_ERR_EVT, OPAL_OCC,
>  		OPAL_CEC_HARDWARE, OPAL_UNRECOVERABLE_ERR_GENERAL,
>  		OPAL_NA);
>  
> +/**
> + * cmp_pstates: Compares the given two pstates and determines which
> + *              among them is associated with a higher pstate.
> + *
> + * @a, at b: The pstate ids of the pstates being compared.
> + *
> + * Returns: -1 : If pstate associated with @a is smaller than
> + *               the pstate associated with @b.
> + *	     0 : If pstates associated with @a and @b are equal.
> + *	     1 : If pstate associated with @a is greater than
> + *               the pstate associated with @b.
> + */
> +static int (*cmp_pstates)(int a, int b);
> +
> +static int cmp_positive_pstates(int a, int b)
> +{
> +	if (a > b)
> +		return -1;
> +	else if (a < b)
> +		return 1;
> +
> +	return 0;
> +}
> +
> +static int cmp_negative_pstates(int a, int b)
> +{
> +	if (a < b)
> +		return -1;
> +	else if (a > b)
> +		return 1;
> +
> +	return 0;
> +}
> +

Can these just return a bool?  The callers only care about > 0.

>  /* Check each chip's HOMER/Sapphire area for PState valid bit */
>  static bool wait_for_all_occ_init(void)
>  {
> @@ -214,15 +248,38 @@ static bool add_cpu_pstate_properties(s8 *pstate_nom)
>  		return false;
>  	}
>  
> +	/*
> +	 * Workload-Optimized-Frequency(WOF) or Ultra-Turbo is supported
> +	 * from version 2 onwards. If WOF is disabled then, the max
> +	 * ultra_turbo pstate will be equal to max turbo pstate.
> +	 */
>  	if (occ_data->version > 1 &&
> -	    occ_data->pstate_ultra_turbo > occ_data->pstate_turbo)
> +	    cmp_pstates(occ_data->pstate_ultra_turbo,
> +			occ_data->pstate_turbo) > 0)
>  		ultra_turbo_en = true;
>  	else
>  		ultra_turbo_en = false;
>  
>  	pmax = ultra_turbo_en ? occ_data->pstate_ultra_turbo :
>  				occ_data->pstate_turbo;
> -	nr_pstates = pmax - occ_data->pstate_min + 1;
> +
> +	/* Sanity check for pstate limits */
> +	if (cmp_pstates(occ_data->pstate_min, pmax) > 0) {
> +		/**
> +		 * @fwts-label OCCInvalidPStateLimits
> +		 * @fwts-advice The min pstate is greater than the
> +		 * max pstate, this could be due to corrupted/invalid
> +		 * data in OCC-OPAL shared memory region. So OPAL has
> +		 * not added pstates to device tree. This means that
> +		 * CPU Frequency management will not be functional in
> +		 * the host.
> +		 */
> +		prlog(PR_ERR, "OCC: Invalid Pstate Limits. Pmin(%d) > Pmax
> (%d)\n",
> +		      occ_data->pstate_min, pmax);
> +		return false;
> +	}
> +
> +	nr_pstates = labs(pmax - occ_data->pstate_min) + 1;
>  	prlog(PR_DEBUG, "OCC: Min %d Nom %d Max %d Nr States %d\n", 
>  	      occ_data->pstate_min, occ_data->pstate_nom,
>  	      pmax, nr_pstates);
> @@ -319,15 +376,31 @@ static bool add_cpu_pstate_properties(s8 *pstate_nom)
>  			dt_core_max[i] = occ_data->core_max[i];
>  	}
>  
> +	/*
> +	 * OCC provides pstate table entries in continuous descending order.
> +	 * Parse the pstate table to skip pstate_ids that are greater
> +	 * than Pmax. If a pstate_id is equal to Pmin then add it to
> +	 * the list and break from the loop as this is the last valid
> +	 * element in the pstate table.
> +	 */
>  	for (i = 0, j = 0; i < MAX_PSTATES && j < nr_pstates; i++) {
> -		if (occ_data->pstates[i].id > pmax ||
> -		    occ_data->pstates[i].id < occ_data->pstate_min)
> +		if (cmp_pstates(occ_data->pstates[i].id, pmax) > 0)
>  			continue;
> +
>  		dt_id[j] = occ_data->pstates[i].id;
>  		dt_freq[j] = occ_data->pstates[i].freq_khz / 1000;
>  		dt_vdd[j] = occ_data->pstates[i].vdd;
>  		dt_vcs[j] = occ_data->pstates[i].vcs;
>  		j++;
> +
> +		if (occ_data->pstates[i].id == occ_data->pstate_min)
> +			break;
> +	}
> +
> +	if (j != nr_pstates) {
> +		prerror("OCC: Expected pstates(%d) is not equal to parsed
> pstates(%d)\n",
> +			nr_pstates, j);
> +		goto out_free_vcs;
>  	}
>  
>  	/* Add the device-tree entries */
> @@ -533,6 +606,17 @@ void occ_pstates_init(void)
>  	if (occ_pstates_initialized)
>  		return;
>  
> +	switch (proc_gen) {
> +	case proc_gen_p8:
> +		cmp_pstates = cmp_negative_pstates;
> +		break;
> +	case proc_gen_p9:
> +		cmp_pstates = cmp_positive_pstates;
> +		break;
> +	default:
> +		return;
> +	}
> +
>  	chip = next_chip(NULL);
>  	if (!chip->homer_base) {
>  		log_simple_error(&e_info(OPAL_RC_OCC_PSTATE_INIT),