Re: [GIT PULL] DT/core: cpu_ofnode updates for v3.12

[Rob Herring <robherring2@gmail.com>]

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