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

[Joel Stanley]

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?

>
>  #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?

>  };
>
>  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?

> +       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.

>         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.

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

16?

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

16?

> +               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?

> +               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
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot