[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