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

Shilpasri G Bhat shilpa.bhat at linux.vnet.ibm.com
Mon Nov 7 17:13:37 AEDT 2016


Hi Pridhviraj,

On 11/07/2016 11:11 AM, ppaidipe wrote:
> 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.
> 

Good catch. Thanks for pointing it out.

Regards,
Shilpa

> 
>>
>>      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),



More information about the Skiboot mailing list