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

[ppaidipe]

On 2016-11-02 14:08, 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().
> 
> 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 v3.
> 
>  hw/occ.c | 99 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 95 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/occ.c b/hw/occ.c
> index d5c590b..3a23043 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;
> +}
> +
>  /* Check each chip's HOMER/Sapphire area for PState valid bit */
>  static bool wait_for_all_occ_init(void)
>  {
> @@ -207,15 +241,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_turbo,
> +			occ_data->pstate_ultra_turbo) > 0)
>  		ultra_turbo_en = true;
>  	else
>  		ultra_turbo_en = false;



Hi Shilpa
Looks like with this change we are disabling ultra turbo mode. I guess 
the parameters to function cmp_pstates can be reversed. Correct me if i 
am wrong.


> 
>  	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);
> @@ -312,15 +369,38 @@ 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) {
> +		/**
> +		 * @fwts-label OCCNrPstatesMismatch
> +		 * @fwts-advice The number of pstates parsed from the
> +		 * pstate table does not match the total no of pstates
> +		 * expected. No PStates in Device Tree: non-functional
> +		 * power/frequency management.
> +		 */
> +		prlog(PR_ERR, "OCC: Mismatch in nr_pstates. Expected nr_pstates =
> %d, Actual nr_pstates parsed = %d\n",
> +		      nr_pstates, j);
> +		goto out_free_vcs;
>  	}
> 
>  	/* Add the device-tree entries */
> @@ -526,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),