[Skiboot] [PATCH 1/2] occ: Add support to read V2 format of OCC-OPAL shared memory region

[Shilpasri G Bhat]

Hi,

On 02/01/2016 06:02 AM, Joel Stanley wrote:
> On Wed, Nov 4, 2015 at 4:40 PM, Shilpasri G Bhat
> <shilpa.bhat at linux.vnet.ibm.com> wrote:
>> Add support to read OCC-OPAL shared memory region version2 to parse
>> ultra-turbo pstates and core-to-max-frequency-allowed array and append
>> it to device tree. Each element of core-to-max-frequency-allowed indicates
>> the maximum frequency sustained with 'n' online cores.
>>
>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat at linux.vnet.ibm.com>
>> ---
>>  hw/occ.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 51 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/occ.c b/hw/occ.c
>> index 79140cc..783add9 100644
>> --- a/hw/occ.c
>> +++ b/hw/occ.c
>> @@ -33,6 +33,7 @@
>>  #define P8_HOMER_SAPPHIRE_DATA_OFFSET  0x1F8000
>>
>>  #define MAX_PSTATES 256
>> +#define MAX_CORES   16
> 
> Where does this number come from?

OCC uses 16 bytes to represent core-to-max-frequency-allowed-array.

> 
>>
>>  #define chip_occ_data(chip) \
>>                 ((struct occ_pstate_table *)(chip->homer_base + \
>> @@ -55,11 +56,12 @@ struct occ_pstate_table {
>>         u8 throttle;
>>         s8 pstate_min;
>>         s8 pstate_nom;
>> -       s8 pstate_max;
>> -       u8 spare1;
>> -       u8 spare2;
>> +       s8 pstate_turbo;
>> +       s8 pstate_ultra_turbo;
>> +       u8 spare;
>>         u64 reserved;
>>         struct occ_pstate_entry pstates[MAX_PSTATES];
>> +       s8 core_max[MAX_CORES];
> 
> I doubt the common case is 16 cores. Is it worth making this dynamic
> based on the number of cores?

This table maps to the memory region updated by OCC to communicate the pstate
table to the host.
If the table gets extended (after core_max) to add new elements in the next
versions, then having core_max defined as static to 16 bytes will avoid
adding/maintaining a padding of 16-nr_cores for mapping the next element.

But while adding this element to DT I will not add all the 16 bytes but
dynamically allocate based on available no of cores in the next version of the
patch.

> 
>>  };
>>
>>  DEFINE_LOG_ENTRY(OPAL_RC_OCC_LOAD, OPAL_PLATFORM_ERR_EVT, OPAL_OCC,
>> @@ -137,8 +139,9 @@ static bool add_cpu_pstate_properties(s8 *pstate_nom)
>>         struct dt_node *power_mgt;
>>         u8 nr_pstates;
>>         /* Arrays for device tree */
>> -       u32 *dt_id, *dt_freq;
>> -       u8 *dt_vdd, *dt_vcs;
>> +       u32 *dt_id, *dt_freq, *dt_core_max = NULL;
>> +       u8 *dt_vdd, *dt_vcs, ultra_turbo_en = 0;
> 
> Can you explain why you are initialising these to zero?

'ultra_turbo_en' is initialized to zero for occ_data->version = 1.
I will move this initialization inside
if (occ_data->version == 1) {

> 
>> +       s8 pmax;
>>         bool rc;
>>         int i;
>>
>> @@ -164,10 +167,25 @@ static bool add_cpu_pstate_properties(s8 *pstate_nom)
>>                 return false;
>>         }
>>
>> -       nr_pstates = occ_data->pstate_max - occ_data->pstate_min + 1;
>> +       if (occ_data->version == 1) {
>> +               pmax = occ_data->pstate_turbo;
>> +               goto next;
>> +       }
>> +
>> +       if (occ_data->pstate_ultra_turbo > occ_data->pstate_turbo) {
>> +               ultra_turbo_en = 1;
>> +               pmax = occ_data->pstate_ultra_turbo;
>> +
>> +       } else {
>> +               ultra_turbo_en = 0;
>> +               pmax = occ_data->pstate_turbo;
>> +
>> +       }
>> +
>> +next:  nr_pstates = pmax - occ_data->pstate_min + 1;
> 
> Put the code on the next line after the label.

Okay.
> 
>>         prlog(PR_DEBUG, "OCC: Min %d Nom %d Max %d Nr States %d\n",
>>               occ_data->pstate_min, occ_data->pstate_nom,
>> -             occ_data->pstate_max, nr_pstates);
>> +             pmax, nr_pstates);
>>
>>         if (nr_pstates <= 1 || nr_pstates > 128) {
>>                 prerror("OCC: OCC range is not valid\n");
>> @@ -208,6 +226,19 @@ static bool add_cpu_pstate_properties(s8 *pstate_nom)
>>                 goto out_free_vdd;
>>         }
>>
>> +       if (!ultra_turbo_en)
>> +               goto populate;
> 
> I think you can put the next 5 lines inside the if() instead of using a goto.

Okay.
> 
>> +
>> +       dt_core_max = (u32 *) malloc(16 * sizeof(u32));
> 
> 16?

Will change this to available no of cores.

[Do we always have same number of cores in all chips?]

> 
>> +       if (!dt_core_max) {
>> +               printf("OCC: dt_core_max alloc failure\n");
>> +               goto out_free_vcs;
>> +       }
>> +
>> +       for (i = 0; i < 16; i++)
> 
> 16?

Yup will change it to nr_cores.

> 
>> +               dt_core_max[i] = occ_data->core_max[i];
>> +
>> +populate:
>>         for( i=0; i < nr_pstates; i++) {
>>                 dt_id[i] = occ_data->pstates[i].id;
>>                 dt_freq[i] = occ_data->pstates[i].freq_khz/1000;
>> @@ -222,12 +253,23 @@ static bool add_cpu_pstate_properties(s8 *pstate_nom)
>>         dt_add_property(power_mgt, "ibm,pstate-vcss", dt_vcs, nr_pstates);
>>         dt_add_property_cells(power_mgt, "ibm,pstate-min", occ_data->pstate_min);
>>         dt_add_property_cells(power_mgt, "ibm,pstate-nominal", occ_data->pstate_nom);
>> -       dt_add_property_cells(power_mgt, "ibm,pstate-max", occ_data->pstate_max);
>> -
>> +       dt_add_property_cells(power_mgt, "ibm,pstate-max", pmax);
>> +       dt_add_property_cells(power_mgt, "ibm,ultra-turbo-en", ultra_turbo_en);
>> +
>> +       if (ultra_turbo_en) {
>> +               dt_add_property_cells(power_mgt, "ibm,pstate-turbo",
>> +                                     occ_data->pstate_turbo);
>> +               dt_add_property_cells(power_mgt, "ibm,pstate-ultra-turbo",
>> +                                     occ_data->pstate_ultra_turbo);
>> +               dt_add_property(power_mgt, "ibm,pstate-core-max", dt_core_max,
>> +                               MAX_CORES * 4);
> 
> What's the 4 here?

I will replace this with sizeof(int32_t);

Thanks and Regards,
Shilpa