On 07/27/2017 11:10 AM, Michael Bringmann wrote:
>
> pseries/drc-info: Provide parallel routines to convert between
> drc_index and CPU numbers at runtime, using the older device-tree
> properties ("ibm,drc-indexes", "ibm,drc-names", "ibm,drc-types"
> and "ibm,drc-power-domains"), or the new property "ibm,drc-info".
>
> Signed-off-by: Michael Bringmann <m...@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/prom.h | 7 +
> arch/powerpc/platforms/pseries/of_helpers.c | 79 ++++++++++++
> arch/powerpc/platforms/pseries/pseries_energy.c | 150
> +++++++++++++++++++----
> 3 files changed, 210 insertions(+), 26 deletions(-)
> ---
> Changes in V2:
> -- Minor changes to integrate to latest 4.13 code
>
> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
> index 4fb02cc..d469d7c 100644
> --- a/arch/powerpc/include/asm/prom.h
> +++ b/arch/powerpc/include/asm/prom.h
> @@ -96,6 +96,13 @@ struct of_drconf_cell {
> #define DRCONF_MEM_AI_INVALID 0x00000040
> #define DRCONF_MEM_RESERVED 0x00000080
>
> +extern int of_one_drc_info(struct property **prop, void **curval,
> + char **dtype, char **dname,
> + u32 *drc_index_start_p,
> + u32 *num_sequential_elems_p,
> + u32 *sequential_inc_p,
> + u32 *last_drc_index_p);
> +
> /*
> * There are two methods for telling firmware what our capabilities are.
> * Newer machines have an "ibm,client-architecture-support" method on the
> diff --git a/arch/powerpc/platforms/pseries/of_helpers.c
> b/arch/powerpc/platforms/pseries/of_helpers.c
> index 2798933..1d59939 100644
> --- a/arch/powerpc/platforms/pseries/of_helpers.c
> +++ b/arch/powerpc/platforms/pseries/of_helpers.c
> @@ -36,3 +36,82 @@ struct device_node *pseries_of_derive_parent(const char
> *path)
> kfree(parent_path);
> return parent ? parent : ERR_PTR(-EINVAL);
> }
> +
> +
> +/* Helper Routines to convert between drc_index to cpu numbers */
> +
> +int of_one_drc_info(struct property **prop, void **curval,
> + char **dtype, char **dname,
> + u32 *drc_index_start_p,
> + u32 *num_sequential_elems_p,
> + u32 *sequential_inc_p,
> + u32 *last_drc_index_p)
> +{
> + char *drc_type, *drc_name_prefix;
> + u32 drc_index_start, num_sequential_elems, dummy;
> + u32 sequential_inc, last_drc_index;
> + const char *p;
> + const __be32 *p2;
> +
> + drc_index_start = num_sequential_elems = 0;
> + sequential_inc = last_drc_index = 0;
> +
> + /* Get drc-type:encode-string */
> + p = drc_type = (*curval);
> + p = of_prop_next_string(*prop, p);
> + if (!p)
> + return -EINVAL;
> +
> + /* Get drc-name-prefix:encode-string */
> + drc_name_prefix = (char *)p;
> + p = of_prop_next_string(*prop, p);
> + if (!p)
> + return -EINVAL;
> +
> + /* Get drc-index-start:encode-int */
> + p2 = (const __be32 *)p;
> + p2 = of_prop_next_u32(*prop, p2, &drc_index_start);
> + if (!p2)
> + return -EINVAL;
> +
> + /* Get/skip drc-name-suffix-start:encode-int */
Why are we skipping the drc name suffix start value? It seems this
would make the routine unusable for anyone wanting to get drc-name
values.
> + p2 = of_prop_next_u32(*prop, p2, &dummy);
> + if (!p)
shouldn't this be checking p2?
> + return -EINVAL;
> +
> + /* Get number-sequential-elements:encode-int */
> + p2 = of_prop_next_u32(*prop, p2, &num_sequential_elems);
> + if (!p2)
> + return -EINVAL;
> +
> + /* Get sequential-increment:encode-int */
> + p2 = of_prop_next_u32(*prop, p2, &sequential_inc);
> + if (!p2)
> + return -EINVAL;
> +
> + /* Get/skip drc-power-domain:encode-int */
Same for power-domain, why skip it?
I don't think any parts of the kernel are currently
looking at this piece of the DRC information, but if we are
going to have a routine to return all the data for a drc-info
block shouldn't it return all of it?
Would it be easier if the routine were to return a drc_info struct with
pointers to the string values and int values read into it?
> + p2 = of_prop_next_u32(*prop, p2, &dummy);
> + if (!p2)
> + return -EINVAL;
> +
> + /* Should now know end of current entry */
> + (*curval) = (void *)p2;
> + last_drc_index = drc_index_start +
> + ((num_sequential_elems-1)*sequential_inc);
> +
> + if (dtype)
> + *dtype = drc_type;
> + if (dname)
> + *dname = drc_name_prefix;
> + if (drc_index_start_p)
> + *drc_index_start_p = drc_index_start;
> + if (num_sequential_elems_p)
> + *num_sequential_elems_p = num_sequential_elems;
> + if (sequential_inc_p)
> + *sequential_inc_p = sequential_inc;
> + if (last_drc_index_p)
> + *last_drc_index_p = last_drc_index;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(of_one_drc_info);
> diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c
> b/arch/powerpc/platforms/pseries/pseries_energy.c
> index 164a13d..865c2af 100644
> --- a/arch/powerpc/platforms/pseries/pseries_energy.c
> +++ b/arch/powerpc/platforms/pseries/pseries_energy.c
> @@ -22,6 +22,7 @@
> #include <asm/page.h>
> #include <asm/hvcall.h>
> #include <asm/firmware.h>
> +#include <asm/prom.h>
>
>
> #define MODULE_VERS "1.0"
> @@ -38,7 +39,6 @@
> static u32 cpu_to_drc_index(int cpu)
> {
> struct device_node *dn = NULL;
> - const int *indexes;
> int i;
> int rc = 1;
> u32 ret = 0;
> @@ -46,18 +46,65 @@ static u32 cpu_to_drc_index(int cpu)
> dn = of_find_node_by_path("/cpus");
> if (dn == NULL)
> goto err;
> - indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
> - if (indexes == NULL)
> - goto err_of_node_put;
> +
> /* Convert logical cpu number to core number */
> i = cpu_core_index_of_thread(cpu);
> - /*
> - * The first element indexes[0] is the number of drc_indexes
> - * returned in the list. Hence i+1 will get the drc_index
> - * corresponding to core number i.
> - */
> - WARN_ON(i > indexes[0]);
> - ret = indexes[i + 1];
> +
> + if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
> + struct property *info = NULL;
> + int j, check_val = i;
Why use check_val here? it seems a bit confusing.
I think perhaps renaming 'i' to thread_index and using that everywhere
may help the readability.
> + u32 num_set_entries;
> + u32 drc_index_start = 0;
> + u32 last_drc_index = 0;
> + u32 num_sequential_elems = 0;
> + u32 sequential_inc = 0;
> + char *dtype;
> + char *dname;
> + void *value;
> +
> + info = of_find_property(dn, "ibm,drc-info", NULL);
> + if (info == NULL)
> + goto err_of_node_put;
> +
> + value = info->value;
> + value = (void *)of_prop_next_u32(info, value, &num_set_entries);
> + if (!value)
> + goto err_of_node_put;
> +
> + for (j = 0; j < num_set_entries; j++) {
> +
> + of_one_drc_info(&info, &value, &dtype, &dname,
> + &drc_index_start,
> + &num_sequential_elems,
> + &sequential_inc, &last_drc_index);
> + if (strcmp(dtype, "CPU"))
Perhaps use strncmp() here.
> + goto err;
> +
> + if (check_val < last_drc_index)
So, check_val here is just the thread_index for the cpu we are looking for.
I think this value will likely always be less than last_drc_index. On systems
I have always seen the drc-index values for CPUs start at 0x10000000 and
increment from there.
If there is ever more than one set of drc-info blocks in the drc-info
property this check would not be valid.
> + break;
> +
> + WARN_ON(((check_val-drc_index_start)%
> + sequential_inc) != 0);
> + }
> + WARN_ON((num_sequential_elems == 0) || (sequential_inc == 0));
> +
> + ret = last_drc_index + (check_val*sequential_inc);
Shouldn't this be drc_index_start?
> + } else {
> + const __be32 *indexes;
> +
> + indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
> + if (indexes == NULL)
> + goto err_of_node_put;
> +
> + /*
> + * The first element indexes[0] is the number of drc_indexes
> + * returned in the list. Hence i+1 will get the drc_index
> + * corresponding to core number i.
> + */
> + WARN_ON(i > indexes[0]);
> + ret = indexes[i + 1];
> + }
> +
> rc = 0;
>
> err_of_node_put:
> @@ -72,34 +119,85 @@ static int drc_index_to_cpu(u32 drc_index)
> {
> struct device_node *dn = NULL;
> const int *indexes;
> - int i, cpu = 0;
> + int thread = 0, cpu = 0;
> int rc = 1;
>
> dn = of_find_node_by_path("/cpus");
> if (dn == NULL)
> goto err;
> - indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
> - if (indexes == NULL)
> - goto err_of_node_put;
> - /*
> - * First element in the array is the number of drc_indexes
> - * returned. Search through the list to find the matching
> - * drc_index and get the core number
> - */
> - for (i = 0; i < indexes[0]; i++) {
> - if (indexes[i + 1] == drc_index)
> +
> + if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
> + struct property *info = NULL;
> + int j;
> + u32 num_set_entries;
> + u32 drc_index_start = 0;
> + u32 last_drc_index = 0;
> + u32 num_sequential_elems = 0;
> + u32 sequential_inc = 0;
> + char *dtype, *dname;
> + void *value;
> +
> + info = of_find_property(dn, "ibm,drc-info", NULL);
> + if (info == NULL)
> + goto err_of_node_put;
> +
> + value = info->value;
> + value = (void *)of_prop_next_u32(info, value, &num_set_entries);
> + if (!value)
> + goto err_of_node_put;
> +
> + for (j = 0; j < num_set_entries; j++) {
> +
> + of_one_drc_info(&info, &value, &dtype, &dname,
> + &drc_index_start,
> + &num_sequential_elems,
> + &sequential_inc, &last_drc_index);
> + if (strcmp(dtype, "CPU"))
> + goto err;
> +
> + WARN_ON(drc_index < drc_index_start);
> + WARN_ON(((drc_index-drc_index_start)%
> + sequential_inc) != 0);
> +
> + if (drc_index > last_drc_index) {
> + cpu += ((last_drc_index-drc_index_start)/
> + sequential_inc);
Could we just use num_sequential_elems here?
cpu += num_sequential_elems;
-Nathan
> + continue;
> + } else {
> + cpu += ((drc_index-drc_index_start)/
> + sequential_inc);
> + }
> +
> + thread = cpu_first_thread_of_core(cpu);
> + rc = 0;
> break;
> + }
> + } else {
> + unsigned long int i;
> +
> + indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
> + if (indexes == NULL)
> + goto err_of_node_put;
> + /*
> + * First element in the array is the number of drc_indexes
> + * returned. Search through the list to find the matching
> + * drc_index and get the core number
> + */
> + for (i = 0; i < indexes[0]; i++) {
> + if (indexes[i + 1] == drc_index)
> + break;
> + }
> + /* Convert core number to logical cpu number */
> + thread = cpu_first_thread_of_core(i);
> + rc = 0;
> }
> - /* Convert core number to logical cpu number */
> - cpu = cpu_first_thread_of_core(i);
> - rc = 0;
>
> err_of_node_put:
> of_node_put(dn);
> err:
> if (rc)
> printk(KERN_WARNING "drc_index_to_cpu(%d) failed", drc_index);
> - return cpu;
> + return thread;
> }
>
> /*
>
