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

Joel Stanley joel at jms.id.au
Fri Feb 12 10:51:55 AEDT 2016


On Thu, Feb 11, 2016 at 6:01 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-pstate-allowed array and append
> it to device tree. Each element of core-to-max-pstate-allowed indicates
> the maximum pstate sustained with 'n' online cores.
>
> Signed-off-by: Shilpasri G Bhat <shilpa.bhat at linux.vnet.ibm.com>

Looks good! Really good work on addressing the issues found by review.

Reviewed-by: Joel Stanley <joel at jms.id.au>

> ---
> Changes from v3:
> - Added a link to github source code of OCC which defines the OCC-OPAL
>   interface.
> - Moved nr_cores computation inside if (ultra_turbo_en) and initialized
>   it to avoid gcc complaints
> - Removed type-casting malloc() returned pointer
> - s/printf/prerror
> - Added documentation.
> - Modified commit log
>
> Changes from v2:
> - Got rid of 'next' label.
> - Removed 'ultra_turbo_en' property from DT. The sole purpose of this
>   property was to indicate if WOF is enabled. Host can use
>   'ibm,pstate-ultra-turbo' property instead which gets added to DT only
>   when WOF is enabled.
> - Added __packed to struct occ_pstate_table which represents the
>   memory layout of the OCC data copied to OCC-OPAL shared memory
>   region in HOMER. OCC copies a packed structure(pstate table) here.
>   So I am adding __packed while mapping the region in skiboot. There is
>   nothing breaking with/without this attribute, but adding it just to make
>   the structure future-proof. I can remove this if you feel it is not
>   required.
>
> Changes from v1:
> - Allocate dt_core_max to number of cores present in the chip.
> - Replace '4' with sizeof(u32).
> - Remove label 'populate'
> - 'dt_core_max' is initialized to NULL
>
>  doc/device-tree/ibm,opal/power-mgt.txt | 11 ++++++
>  hw/occ.c                               | 63 ++++++++++++++++++++++++++++------
>  2 files changed, 63 insertions(+), 11 deletions(-)
>
> diff --git a/doc/device-tree/ibm,opal/power-mgt.txt b/doc/device-tree/ibm,opal/power-mgt.txt
> index 0852650..d9cadb8 100644
> --- a/doc/device-tree/ibm,opal/power-mgt.txt
> +++ b/doc/device-tree/ibm,opal/power-mgt.txt
> @@ -55,6 +55,17 @@ These properties list a voltage-identifier of each of the pstates listed in
>  ibm,pstate-ids for the Vcs and Vdd values used for that pstate. Each VID is a
>  single byte.
>
> +ibm,pstate-ultra-turbo ibm,pstate-turbo
> +---------------------------------------
> +
> +These properties are added when ultra-turbo(WOF) is enabled. These properties
> +give the max turbo and max ultra-turbo pstate.
> +
> +ibm,pstate-core-max
> +-------------------
> +
> +This property is added when ultra_turbo(WOF) is enabled. This property gives
> +the list of max pstate for each 'n' number of active cores in the chip.
>
>  FIXME: document these:
>  ibm,cpu-idle-state-flags
> diff --git a/hw/occ.c b/hw/occ.c
> index 0e3d953..987d007 100644
> --- a/hw/occ.c
> +++ b/hw/occ.c
> @@ -49,18 +49,24 @@ struct occ_pstate_entry {
>         u32 freq_khz;
>  };
>
> +/*
> + * OCC-OPAL Shared Memory Region Version 2
> + * https://github.com/open-power/occ/blob/master/src/occ/proc/proc_pstate.h
> + * Interface defined in 'sapphire_table_t'
> + */
>  struct occ_pstate_table {
>         u8 valid;
>         u8 version;
>         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[16];
> +} __packed;
>
>  DEFINE_LOG_ENTRY(OPAL_RC_OCC_LOAD, OPAL_PLATFORM_ERR_EVT, OPAL_OCC,
>                 OPAL_CEC_HARDWARE, OPAL_PREDICTIVE_ERR_GENERAL,
> @@ -142,11 +148,13 @@ static bool add_cpu_pstate_properties(s8 *pstate_nom)
>         uint64_t occ_data_area;
>         struct occ_pstate_table *occ_data;
>         struct dt_node *power_mgt;
> -       u8 nr_pstates;
> +       u8 nr_pstates, nr_cores = 0;
> +       s8 pmax;
>         /* Arrays for device tree */
>         u32 *dt_id, *dt_freq;
>         u8 *dt_vdd, *dt_vcs;
> -       bool rc;
> +       s8 *dt_core_max = NULL;
> +       bool rc, ultra_turbo_en;
>         int i;
>
>         prlog(PR_DEBUG, "OCC: CPU pstate state device tree init\n");
> @@ -171,10 +179,18 @@ 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 &&
> +           occ_data->pstate_ultra_turbo > occ_data->pstate_turbo)
> +               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;
>         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");
> @@ -215,6 +231,18 @@ static bool add_cpu_pstate_properties(s8 *pstate_nom)
>                 goto out_free_vdd;
>         }
>
> +       if (ultra_turbo_en) {
> +               nr_cores = get_available_nr_cores_in_chip(chip->id);
> +               dt_core_max = malloc(nr_cores * sizeof(s8));
> +               if (!dt_core_max) {
> +                       prerror("OCC: dt_core_max alloc failure\n");
> +                       goto out_free_vcs;
> +               }
> +
> +               for (i = 0; i < nr_cores; i++)
> +                       dt_core_max[i] = occ_data->core_max[i];
> +       }
> +
>         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;
> @@ -223,18 +251,31 @@ static bool add_cpu_pstate_properties(s8 *pstate_nom)
>         }
>
>         /* Add the device-tree entries */
> -       dt_add_property(power_mgt, "ibm,pstate-ids", dt_id, nr_pstates * 4);
> -       dt_add_property(power_mgt, "ibm,pstate-frequencies-mhz", dt_freq, nr_pstates * 4);
> +       dt_add_property(power_mgt, "ibm,pstate-ids", dt_id,
> +                       nr_pstates * sizeof(u32));
> +       dt_add_property(power_mgt, "ibm,pstate-frequencies-mhz", dt_freq,
> +                       nr_pstates * sizeof(u32));
>         dt_add_property(power_mgt, "ibm,pstate-vdds", dt_vdd, nr_pstates);
>         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);
> +
> +       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,
> +                               nr_cores);
> +               free(dt_core_max);
> +       }
>
>         /* Return pstate to set for each core */
>         *pstate_nom = occ_data->pstate_nom;
>         rc = true;
>
> +out_free_vcs:
>         free(dt_vcs);
>  out_free_vdd:
>         free(dt_vdd);
> --
> 1.9.3
>


More information about the Skiboot mailing list