[Skiboot] [PATCH v3 2/3] occ: Add support to read V2 format of OCC-OPAL shared memory region
Joel Stanley
joel at jms.id.au
Thu Feb 11 15:52:11 AEDT 2016
On Wed, Feb 10, 2016 at 8:09 AM, 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.
Looking better. A few more comments below.
>
> Signed-off-by: Shilpasri G Bhat <shilpa.bhat at linux.vnet.ibm.com>
> ---
> 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 to fix the below error.
> hw/occ.c: In function ‘occ_pstates_init’:
> /home/shilpa/work/skiboot/include/mem_region-malloc.h:35:19: error:
> ‘dt_core_max’ may be used uninitialized in this function
> [-Werror=maybe-uninitialized]
> #define free(ptr) __free(ptr, __location__)
>
> hw/occ.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 50 insertions(+), 11 deletions(-)
>
> diff --git a/hw/occ.c b/hw/occ.c
> index 0e3d953..4a764ef 100644
> --- a/hw/occ.c
> +++ b/hw/occ.c
> @@ -49,18 +49,22 @@ struct occ_pstate_entry {
> u32 freq_khz;
> };
>
> +/*
> + * OCC-OPAL Shared Memory Region Version 2
> + */
Do we have documentation you can link to here? Or a link to the header
in the OCC source?
> 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 +146,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;
> + 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 +177,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 +229,18 @@ static bool add_cpu_pstate_properties(s8 *pstate_nom)
> goto out_free_vdd;
> }
>
> + nr_cores = get_available_nr_cores_in_chip(chip->id);
You only need to do this calculation if ultra_turbo_en is true, so
move it inside the test.
> + if (ultra_turbo_en) {
> + dt_core_max = (s8 *)malloc(nr_cores * sizeof(s8));
You don't need to cast.
> + if (!dt_core_max) {
> + printf("OCC: dt_core_max alloc failure\n");
prlog
> + 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 +249,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