See below.
On 11/20/2017 09:35 AM, Nathan Fontenot wrote:
> On 11/16/2017 02:11 PM, 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>
>> ---
>> Changes in V4:
>> -- Rename of_one_drc_info to of_read_drc_info_cell
>> -- Fix some spacing within expressions
>> -- Make some style corrections
>> ---
>> arch/powerpc/include/asm/prom.h | 15 +++
>> arch/powerpc/platforms/pseries/of_helpers.c | 60 ++++++++++
>> arch/powerpc/platforms/pseries/pseries_energy.c | 138
>> ++++++++++++++++++-----
>> 3 files changed, 185 insertions(+), 28 deletions(-)
>>
...
>> +
>> + value = info->value;
>> + value = (void *)of_prop_next_u32(info, value,
>> + &num_set_entries);
>
> Missed this the first time, but setting value twice seems a bit odd.
> This should be able to be collapsed into;
>
> value = (void *)of_prop_next_u32(info, NULL, &num_set_entries);
Okay
>
>> + if (!value)
>> + goto err_of_node_put;
>> +
>> + for (j = 0; j < num_set_entries; j++) {
>> +
>> + of_read_drc_info_cell(&info, &value, &drc);
>> + if (strncmp(drc.drc_type, "CPU", 3))
>> + goto err;
>> +
>> + if (thread_index < drc.last_drc_index)
>> + break;
>> +
>> + WARN_ON(((thread_index - drc.drc_index_start) %
>> + drc.sequential_inc) != 0);
>
> The WARN_ON that you have here, and in the code below, really fell like they
> should be in of_read_drc_info_cell. These are checks on reading the info in
> the drc-info property.
Remove unnecessary WARN_ON() checks.
>
>> + }
>> +
>> + ret = drc.drc_index_start + (thread_index * drc.sequential_inc);
>> + } 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 thread_index+1 will get the
>> + * drc_index corresponding to core number thread_index.
>> + */
>> + WARN_ON(thread_index > indexes[0]);
>> + ret = indexes[thread_index + 1];
>> + }
>> +
>> rc = 0;
>>
>> err_of_node_put:
>> @@ -72,34 +111,77 @@ static int drc_index_to_cpu(u32 drc_index)
>> {
>> struct device_node *dn = NULL;
>> const int *indexes;
>> - int i, cpu = 0;
>> + int thread_index = 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;
>> + struct of_drc_info drc;
>> + int j;
>> + u32 num_set_entries;
>> + const __be32 *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);
Okay.
>
> Same here as mentioned above.
>
> -Nathan
>
>> + if (!value)
>> + goto err_of_node_put;
>> +
>> + for (j = 0; j < num_set_entries; j++) {
>> +
>> + of_read_drc_info_cell(&info, &value, &drc);
>> + if (strncmp(drc.drc_type, "CPU", 3))
>> + goto err;
>> +
>> + WARN_ON(drc_index < drc.drc_index_start);
>> + WARN_ON(((drc_index - drc.drc_index_start) %
>> + drc.sequential_inc) != 0);
>> +
>> + if (drc_index > drc.last_drc_index) {
>> + cpu += drc.num_sequential_elems;
>> + continue;
>> + }
>> + cpu += ((drc_index - drc.drc_index_start) /
>> + drc.sequential_inc);
>> +
>> + thread_index = 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_index = 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_index;
>> }
>>
>> /*
>>
>
>
--
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line 363-5196
External: (512) 286-5196
Cell: (512) 466-0650
m...@linux.vnet.ibm.com
