On 08/14/2013 05:01 AM, Sudeep KarkadaNagesha wrote:
> On 13/08/13 22:07, Benjamin Herrenschmidt wrote:
>> On Tue, 2013-08-13 at 19:29 +0100, Sudeep KarkadaNagesha wrote:
>>> I don't understand completely the use of ibm,ppc-interrupt-server#s and
>>> its implications on generic of_get_cpu_node implementation.
>>> I see the PPC specific definition of of_get_cpu_node uses thread id only
>>> in 2 instances. Based on that, I have tried to move all the other
>>> instances to use generic definition.
>>>
>>> Let me know if the idea is correct.
>>
>> No. The device-tree historically only represents cores, not HW threads, so
>> it makes sense to retrieve also the thread number corresponding to the CPU.
>>
> Ok
>
>> However, the mechanism to represent HW threads in the device-tree is currently
>> somewhat platform specific (the ibm,ppc-interrupt-server#s).
> I see most of the callers pass NULL to thread id argument except 2
> instances in entire tree. If that's the case why can't we move to use
> generic of_get_cpu_node in most of those cases and have PPC specific
> implementation for the ones using thread id.
>
>>
>> So what you could do for now is:
>>
>> - Have a generic version that always returns 0 as the thread, which is weak
> I would prefer to move to generic of_get_cpu_node where ever possible
> and rename the function that takes thread id rather than making generic
> one weak.
>
>>
>> - powerpc keeps its own implementation
> How about only in cases where it needs thread_id.
>
>>
>> - Start a discussion on the bindings (if not already there) to define threads
>> in a better way at which point the generic function can be updated.
>>
> I am not sure if we need to define any new bindings. Excerpts from ePAPR
> (v1.1):
> "3.7.1 General Properties of CPU nodes
> The value of "reg" is a <prop-encoded-array> that defines a unique
> CPU/thread id for the CPU/threads represented by the CPU node.
> If a CPU supports more than one thread (i.e. multiple streams of
> execution) the reg property is an array with 1 element per thread. The
> #address-cells on the /cpus node specifies how many cells each element
> of the array takes. Software can determine the number of threads by
> dividing the size of reg by the parent node's #address-cells."
>
> And this is exactly in agreement to what's implemented in the generic
> of_get_cpu_node:
>
> for_each_child_of_node(cpus, cpun) {
> if (of_node_cmp(cpun->type, "cpu"))
> continue;
> cell = of_get_property(cpun, "reg", &prop_len);
> if (!cell) {
> pr_warn("%s: missing reg property\n", cpun->full_name);
> continue;
> }
> prop_len /= sizeof(*cell);
> while (prop_len) {
> hwid = of_read_number(cell, ac);
> prop_len -= ac;
> if (arch_match_cpu_phys_id(cpu, hwid))
> return cpun;
> }
> }
How about something like this:
for_each_child_of_node(cpus, cpun) {
if (of_node_cmp(cpun->type, "cpu"))
continue;
if (arch_of_get_cpu_node(cpun, thread))
return cpun;
cell = of_get_property(cpun, "reg", &prop_len);
if (!cell) {
pr_warn("%s: missing reg property\n", cpun->full_name);
continue;
}
prop_len /= sizeof(*cell);
while (prop_len) {
hwid = of_read_number(cell, ac);
prop_len -= ac;
if (arch_match_cpu_phys_id(cpu, hwid))
return cpun;
}
}
For PPC:
arch_of_get_cpu_node()
{
const u32 *intserv;
unsigned int plen, t;
/* Check for ibm,ppc-interrupt-server#s. */
intserv = of_get_property(np, "ibm,ppc-interrupt-server#s",
&plen);
if (!intserv)
return false;
hardid = get_hard_smp_processor_id(cpu);
plen /= sizeof(u32);
for (t = 0; t < plen; t++) {
if (hardid == intserv[t]) {
if (thread)
*thread = t;
return true;
}
}
return false;
}
>
> Yes this doesn't cover the historical "ibm,ppc-interrupt-server#s", for
> which we can have PPC specific wrapper above the generic one i.e. get
> the cpu node and then parse for thread id under custom property.
>
> Let me know your thoughts.
>
> Regards,
> Sudeep
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
