new cpu iteration code...

new cpu iteration code...

[David Miller]

I'm getting a flood of kernel log warnings on bootup now with
the generic of_get_cpu_node() code, it is wrong for sparc64
on several levels.

There is never a root "/cpus" node, so this code always fails.
Usually the "cpu" nodes are simply listed at the top-level.

The correct way to go about this is to use something like:

	struct device_node *dp;

	for_each_node_by_type(dp, "cpu") {
	}

and therefore be agnostic as to the layout of the device
tree wrt. cpu nodes.

Secondly, the property to use to get the physical cpu number is not
only different on sparc from what this code uses, but varies.  It can
be either "upa-portid" or "portid".  The "reg" property represents
various things and will be different for different cpu types and thus
is not a good candidate for fetching this information.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: new cpu iteration code...

[Sudeep KarkadaNagesha]

Hi David,

On 18/09/13 22:37, David Miller wrote:
> 
> I'm getting a flood of kernel log warnings on bootup now with
> the generic of_get_cpu_node() code, it is wrong for sparc64
> on several levels.
> 
> There is never a root "/cpus" node, so this code always fails.
> Usually the "cpu" nodes are simply listed at the top-level.
> 

As per ePAPR:
3.6 CPUS Node Properties
A cpus node is required for all device trees. It does not represent a
real device in the system, but acts as a container for child cpu nodes
which represent the systems CPUs. #address-cells and #size-cells are
required property.

> The correct way to go about this is to use something like:
> 
> 	struct device_node *dp;
> 
> 	for_each_node_by_type(dp, "cpu") {
> 	}
> 
> and therefore be agnostic as to the layout of the device
> tree wrt. cpu nodes.
> 
> Secondly, the property to use to get the physical cpu number is not
> only different on sparc from what this code uses, but varies.  It can
> be either "upa-portid" or "portid".  The "reg" property represents
> various things and will be different for different cpu types and thus
> is not a good candidate for fetching this information.

The value of "reg" defines a unique CPU/thread id for the CPU/threads
represented by the CPU node. This is again a deviation from ePAPR.

I could not find any binding document or dts file with "upa-portid" or
"portid" to think about possible solution.

I am yet to understand definition of backward compatibility and
adherence to ePAPR from device tree perspective. Do we consider this as
broken DT as it doesn't adhere to ePAPR or do we need to have backward
compatibility here ?

Regards,
Sudeep

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: new cpu iteration code...

[David Miller]

From: Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha-5wv7dgnIgG8@public.gmane.org>
Date: Thu, 19 Sep 2013 10:21:24 +0100

> I could not find any binding document or dts file with "upa-portid" or
> "portid" to think about possible solution.
> 
> I am yet to understand definition of backward compatibility and
> adherence to ePAPR from device tree perspective. Do we consider this as
> broken DT as it doesn't adhere to ePAPR or do we need to have backward
> compatibility here ?

I don't think there is any value in your search for these properties
in various documents.

This is simply how it is on sparc64 whether you like it or not.

And therefore the code must accomodate this difference if it is
to live in a generic place.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: new cpu iteration code...

[Rob Herring]

On 09/19/2013 04:21 AM, Sudeep KarkadaNagesha wrote:
> Hi David,
> 
> On 18/09/13 22:37, David Miller wrote:
>>
>> I'm getting a flood of kernel log warnings on bootup now with
>> the generic of_get_cpu_node() code, it is wrong for sparc64
>> on several levels.
>>
>> There is never a root "/cpus" node, so this code always fails.
>> Usually the "cpu" nodes are simply listed at the top-level.
>>
> 
> As per ePAPR:

Sparc is OpenFirmware not FDT which the ePAPR is concerned with.

> 3.6 CPUS Node Properties
> A cpus node is required for all device trees. It does not represent a
> real device in the system, but acts as a container for child cpu nodes
> which represent the systems CPUs. #address-cells and #size-cells are
> required property.
> 
>> The correct way to go about this is to use something like:
>>
>> 	struct device_node *dp;
>>
>> 	for_each_node_by_type(dp, "cpu") {
>> 	}
>>
>> and therefore be agnostic as to the layout of the device
>> tree wrt. cpu nodes.
>>
>> Secondly, the property to use to get the physical cpu number is not
>> only different on sparc from what this code uses, but varies.  It can
>> be either "upa-portid" or "portid".  The "reg" property represents
>> various things and will be different for different cpu types and thus
>> is not a good candidate for fetching this information.
> 
> The value of "reg" defines a unique CPU/thread id for the CPU/threads
> represented by the CPU node. This is again a deviation from ePAPR.
> 
> I could not find any binding document or dts file with "upa-portid" or
> "portid" to think about possible solution.
> 
> I am yet to understand definition of backward compatibility and
> adherence to ePAPR from device tree perspective. Do we consider this as
> broken DT as it doesn't adhere to ePAPR or do we need to have backward
> compatibility here ?

The simple solution here is to just remove the warning. It doesn't
appear to me that sparc ever sets the cpu device of_node pointer.

Are there any cases on where sparc does have a /cpus node? If so we'd
need to make of_get_cpu_node a weak function or depend on a kconfig option.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: new cpu iteration code...

[Sudeep KarkadaNagesha]

On 19/09/13 14:26, Rob Herring wrote:
> On 09/19/2013 04:21 AM, Sudeep KarkadaNagesha wrote:
>> Hi David,
>>
>> On 18/09/13 22:37, David Miller wrote:
>>>
>>> I'm getting a flood of kernel log warnings on bootup now with
>>> the generic of_get_cpu_node() code, it is wrong for sparc64
>>> on several levels.
>>>
>>> There is never a root "/cpus" node, so this code always fails.
>>> Usually the "cpu" nodes are simply listed at the top-level.
>>>
>>
>> As per ePAPR:
> 
> Sparc is OpenFirmware not FDT which the ePAPR is concerned with.
> 
Thanks, I wasn't aware that SPARC is not using FDT.

Regards,
Sudeep


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: new cpu iteration code...

[David Miller]

From: Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Thu, 19 Sep 2013 08:26:04 -0500

> The simple solution here is to just remove the warning. It doesn't
> appear to me that sparc ever sets the cpu device of_node pointer.

Why wouldn't I want sparc to have this functionality now that the
code is generically available.

> Are there any cases on where sparc does have a /cpus node? If so we'd
> need to make of_get_cpu_node a weak function or depend on a kconfig option.

I already gave a solution to this problem, make the loop iterator be:

	for_each_node_by_type(dp, "cpu")
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: new cpu iteration code...

[Sudeep KarkadaNagesha]

On 19/09/13 18:38, David Miller wrote:
> From: Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Date: Thu, 19 Sep 2013 08:26:04 -0500
> 
>> The simple solution here is to just remove the warning. It doesn't
>> appear to me that sparc ever sets the cpu device of_node pointer.
> 
> Why wouldn't I want sparc to have this functionality now that the
> code is generically available.
> 
Makes sense, but as you mentioned before we need to match other property
names namely "upa-portid", "portid" or "cpuid" for cpu physical id right
? Are all these 32-bit values ?

>> Are there any cases on where sparc does have a /cpus node? If so we'd
>> need to make of_get_cpu_node a weak function or depend on a kconfig option.
> 
> I already gave a solution to this problem, make the loop iterator be:
> 
> 	for_each_node_by_type(dp, "cpu")
> 

Does it make sense use this only when /cpus is not found ?
IMHO as the number of node in DT increases(which is the case on ARM
platforms) parsing entire tree may be expensive(which can be avoided in
case /cpus is found)

How about something like below ? I am not sure if of_n_addr_cells logic works
for those properties on SPARC.

Regards,
Sudeep

---->8

diff --git a/drivers/of/base.c b/drivers/of/base.c
index b2cee3d..83512ea 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -280,6 +280,31 @@ static bool __of_find_n_match_cpu_property(struct
device_node *cpun,
 	return false;
 }

+static bool __of_match_cpu_property(struct device_node *cpun, int cpu,
+							unsigned int *thread)
+	/* Check for non-standard "ibm,ppc-interrupt-server#s" property
+	 * for thread ids on PowerPC. If it doesn't exist fallback to
+	 * standard "reg" property.
+	 */
+	if (IS_ENABLED(CONFIG_PPC) &&
+		__of_find_n_match_cpu_property(cpun,
+				"ibm,ppc-interrupt-server#s", cpu, thread))
+		return true;
+	/* Check for "upa-portid" or "portid" property on SPARC */
+	if (IS_ENABLED(CONFIG_SPARC)) {
+		if (__of_find_n_match_cpu_property(cpun, "upa-portid",
+							cpu, thread))
+			return true;
+		if (__of_find_n_match_cpu_property(cpun, "portid",
+							cpu, thread))
+			return true;
+	}
+	if (__of_find_n_match_cpu_property(cpun, "reg", cpu, thread))
+		return true;
+
+	return false;
+}
 /**
  * of_get_cpu_node - Get device node associated with the given logical CPU
  *
@@ -303,24 +328,17 @@ struct device_node *of_get_cpu_node(int cpu, unsigned int
*thread)
 	struct device_node *cpun, *cpus;

 	cpus = of_find_node_by_path("/cpus");
-	if (!cpus) {
-		pr_warn("Missing cpus node, bailing out\n");
-		return NULL;
-	}
-
-	for_each_child_of_node(cpus, cpun) {
-		if (of_node_cmp(cpun->type, "cpu"))
-			continue;
-		/* Check for non-standard "ibm,ppc-interrupt-server#s" property
-		 * for thread ids on PowerPC. If it doesn't exist fallback to
-		 * standard "reg" property.
-		 */
-		if (IS_ENABLED(CONFIG_PPC) &&
-			__of_find_n_match_cpu_property(cpun,
-				"ibm,ppc-interrupt-server#s", cpu, thread))
-			return cpun;
-		if (__of_find_n_match_cpu_property(cpun, "reg", cpu, thread))
-			return cpun;
+	if (cpus) {
+		for_each_child_of_node(cpus, cpun) {
+			if (of_node_cmp(cpun->type, "cpu"))
+				continue;
+			if (__of_match_cpu_property(cpun, cpu, thread))
+				return cpun;
+		}
+	} else {
+		for_each_node_by_type(cpun, "cpu")
+			if (__of_match_cpu_property(cpun, cpu, thread))
+				return cpun;
 	}
 	return NULL;
 }

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: new cpu iteration code...

[David Miller]

From: Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha-5wv7dgnIgG8@public.gmane.org>
Date: Fri, 20 Sep 2013 17:44:04 +0100

> Makes sense, but as you mentioned before we need to match other property
> names namely "upa-portid", "portid" or "cpuid" for cpu physical id right
> ? Are all these 32-bit values ?

Yes they are.  You'll simply have to make the property name string
to use be something an architecture can define.

>> I already gave a solution to this problem, make the loop iterator be:
>> 
>> 	for_each_node_by_type(dp, "cpu")
>> 
> 
> Does it make sense use this only when /cpus is not found ?

No, it should be used universally since it is an implementation that
should work in all possible layouts.

I do not buy your argument about cost at all.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: new cpu iteration code...

[Rob Herring]

On 09/20/2013 11:44 AM, Sudeep KarkadaNagesha wrote:
> On 19/09/13 18:38, David Miller wrote:
>> From: Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Date: Thu, 19 Sep 2013 08:26:04 -0500
>>
>>> The simple solution here is to just remove the warning. It doesn't
>>> appear to me that sparc ever sets the cpu device of_node pointer.
>>
>> Why wouldn't I want sparc to have this functionality now that the
>> code is generically available.
>>
> Makes sense, but as you mentioned before we need to match other property
> names namely "upa-portid", "portid" or "cpuid" for cpu physical id right
> ? Are all these 32-bit values ?
> 
>>> Are there any cases on where sparc does have a /cpus node? If so we'd
>>> need to make of_get_cpu_node a weak function or depend on a kconfig option.
>>
>> I already gave a solution to this problem, make the loop iterator be:
>>
>> 	for_each_node_by_type(dp, "cpu")
>>
> 
> Does it make sense use this only when /cpus is not found ?
> IMHO as the number of node in DT increases(which is the case on ARM
> platforms) parsing entire tree may be expensive(which can be avoided in
> case /cpus is found)

Can't you simply do something like this for the search:

	cpus = of_find_node_by_path("/cpus");
	if (!cpus && IS_ENABLED(CONFIG_SPARC))
		cpus = of_find_node_by_path("/");
	if (!cpus) {
		pr_warn("Missing cpus node, bailing out\n");
		return NULL;
	}

	for_each_child_of_node(cpus, cpun) {

Rob

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: new cpu iteration code...

[David Miller]

From: Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Sun, 22 Sep 2013 15:13:35 -0500

> On 09/20/2013 11:44 AM, Sudeep KarkadaNagesha wrote:
>> On 19/09/13 18:38, David Miller wrote:
>>> From: Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> Date: Thu, 19 Sep 2013 08:26:04 -0500
>>>
>>>> The simple solution here is to just remove the warning. It doesn't
>>>> appear to me that sparc ever sets the cpu device of_node pointer.
>>>
>>> Why wouldn't I want sparc to have this functionality now that the
>>> code is generically available.
>>>
>> Makes sense, but as you mentioned before we need to match other property
>> names namely "upa-portid", "portid" or "cpuid" for cpu physical id right
>> ? Are all these 32-bit values ?
>> 
>>>> Are there any cases on where sparc does have a /cpus node? If so we'd
>>>> need to make of_get_cpu_node a weak function or depend on a kconfig option.
>>>
>>> I already gave a solution to this problem, make the loop iterator be:
>>>
>>> 	for_each_node_by_type(dp, "cpu")
>>>
>> 
>> Does it make sense use this only when /cpus is not found ?
>> IMHO as the number of node in DT increases(which is the case on ARM
>> platforms) parsing entire tree may be expensive(which can be avoided in
>> case /cpus is found)
> 
> Can't you simply do something like this for the search:
> 
> 	cpus = of_find_node_by_path("/cpus");
> 	if (!cpus && IS_ENABLED(CONFIG_SPARC))
> 		cpus = of_find_node_by_path("/");
> 	if (!cpus) {
> 		pr_warn("Missing cpus node, bailing out\n");
> 		return NULL;
> 	}
> 
> 	for_each_child_of_node(cpus, cpun) {

This doesn't work either Rob.  Some UltraSPARC-IV systems put the cpu
nodes under directories representing the cores.

For the third time, the only thing which will work unilaterally is
the type base iterator I proposed above.

Why is there so much resistence to something which has an extremely
high likelyhood of working everywhere and not requiring any ifdef
crapola?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: new cpu iteration code...

[Rob Herring]

On Sun, Sep 22, 2013 at 3:29 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> From: Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Date: Sun, 22 Sep 2013 15:13:35 -0500
>
>> On 09/20/2013 11:44 AM, Sudeep KarkadaNagesha wrote:
>>> On 19/09/13 18:38, David Miller wrote:
>>>> From: Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> Date: Thu, 19 Sep 2013 08:26:04 -0500
>>>>
>>>>> The simple solution here is to just remove the warning. It doesn't
>>>>> appear to me that sparc ever sets the cpu device of_node pointer.
>>>>
>>>> Why wouldn't I want sparc to have this functionality now that the
>>>> code is generically available.
>>>>
>>> Makes sense, but as you mentioned before we need to match other property
>>> names namely "upa-portid", "portid" or "cpuid" for cpu physical id right
>>> ? Are all these 32-bit values ?
>>>
>>>>> Are there any cases on where sparc does have a /cpus node? If so we'd
>>>>> need to make of_get_cpu_node a weak function or depend on a kconfig option.
>>>>
>>>> I already gave a solution to this problem, make the loop iterator be:
>>>>
>>>>     for_each_node_by_type(dp, "cpu")
>>>>
>>>
>>> Does it make sense use this only when /cpus is not found ?
>>> IMHO as the number of node in DT increases(which is the case on ARM
>>> platforms) parsing entire tree may be expensive(which can be avoided in
>>> case /cpus is found)
>>
>> Can't you simply do something like this for the search:
>>
>>       cpus = of_find_node_by_path("/cpus");
>>       if (!cpus && IS_ENABLED(CONFIG_SPARC))
>>               cpus = of_find_node_by_path("/");
>>       if (!cpus) {
>>               pr_warn("Missing cpus node, bailing out\n");
>>               return NULL;
>>       }
>>
>>       for_each_child_of_node(cpus, cpun) {
>
> This doesn't work either Rob.  Some UltraSPARC-IV systems put the cpu
> nodes under directories representing the cores.

Okay. Good to know.

> For the third time, the only thing which will work unilaterally is
> the type base iterator I proposed above.

Now that I understand all the variations, I agree.

> Why is there so much resistence to something which has an extremely
> high likelyhood of working everywhere and not requiring any ifdef
> crapola?

Simplifying Sudeep's code a bit while maintaining some level of
validation of the DT layout was my only goal. I guess that validation
really needs to be at build time rather than boot time and there is
some work going on in that area.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: new cpu iteration code...

[Grant Likely]

On Wed, Sep 25, 2013 at 3:29 AM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Sun, Sep 22, 2013 at 3:29 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>> From: Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Date: Sun, 22 Sep 2013 15:13:35 -0500
>>
>>> On 09/20/2013 11:44 AM, Sudeep KarkadaNagesha wrote:
>>>> On 19/09/13 18:38, David Miller wrote:
>>>>> From: Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>> Date: Thu, 19 Sep 2013 08:26:04 -0500
>>>>>
>>>>>> The simple solution here is to just remove the warning. It doesn't
>>>>>> appear to me that sparc ever sets the cpu device of_node pointer.
>>>>>
>>>>> Why wouldn't I want sparc to have this functionality now that the
>>>>> code is generically available.
>>>>>
>>>> Makes sense, but as you mentioned before we need to match other property
>>>> names namely "upa-portid", "portid" or "cpuid" for cpu physical id right
>>>> ? Are all these 32-bit values ?
>>>>
>>>>>> Are there any cases on where sparc does have a /cpus node? If so we'd
>>>>>> need to make of_get_cpu_node a weak function or depend on a kconfig option.
>>>>>
>>>>> I already gave a solution to this problem, make the loop iterator be:
>>>>>
>>>>>     for_each_node_by_type(dp, "cpu")
>>>>>
>>>>
>>>> Does it make sense use this only when /cpus is not found ?
>>>> IMHO as the number of node in DT increases(which is the case on ARM
>>>> platforms) parsing entire tree may be expensive(which can be avoided in
>>>> case /cpus is found)
>>>
>>> Can't you simply do something like this for the search:
>>>
>>>       cpus = of_find_node_by_path("/cpus");
>>>       if (!cpus && IS_ENABLED(CONFIG_SPARC))
>>>               cpus = of_find_node_by_path("/");
>>>       if (!cpus) {
>>>               pr_warn("Missing cpus node, bailing out\n");
>>>               return NULL;
>>>       }
>>>
>>>       for_each_child_of_node(cpus, cpun) {
>>
>> This doesn't work either Rob.  Some UltraSPARC-IV systems put the cpu
>> nodes under directories representing the cores.
>
> Okay. Good to know.
>
>> For the third time, the only thing which will work unilaterally is
>> the type base iterator I proposed above.
>
> Now that I understand all the variations, I agree.
>
>> Why is there so much resistence to something which has an extremely
>> high likelyhood of working everywhere and not requiring any ifdef
>> crapola?
>
> Simplifying Sudeep's code a bit while maintaining some level of
> validation of the DT layout was my only goal. I guess that validation
> really needs to be at build time rather than boot time and there is
> some work going on in that area.

Still need a solution here in short order. Sundeep, are you working on
a fixup patch?

I can remove the warning as a short term fix, but to make the function
useful generally then there needs to be a way for the architecture to
tell the core what property name to use for CPU ids. The ugly powerpc
exception that is in there should be reworked at the same time.

g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: new cpu iteration code...

[David Miller]

From: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date: Thu, 3 Oct 2013 21:11:55 +0100

> Still need a solution here in short order. Sundeep, are you working on
> a fixup patch?
> 
> I can remove the warning as a short term fix, but to make the function
> useful generally then there needs to be a way for the architecture to
> tell the core what property name to use for CPU ids. The ugly powerpc
> exception that is in there should be reworked at the same time.

I'm working on a patch right now, give me 30 minutes.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: new cpu iteration code...

[David Miller]

Can people on powerpc/arm/etc. give this a spin?  This works on my
T4-2 sparc64 box.

--------------------
of: Make cpu node handling more portable.

Use for_each_node_by_type() to iterate all cpu nodes in the
system.

Provide and overridable function arch_find_n_match_cpu_physical_id,
which sees if the given device node matches 'cpu' and if so sets
'*thread' when non-NULL to the cpu thread number within the core.

The default implementation behaves the same as the existing code.

Add a sparc64 implementation.

Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>

diff --git a/arch/sparc/kernel/prom_64.c b/arch/sparc/kernel/prom_64.c
index d397d7f..6b39125 100644
--- a/arch/sparc/kernel/prom_64.c
+++ b/arch/sparc/kernel/prom_64.c
@@ -373,6 +373,59 @@ static const char *get_mid_prop(void)
 	return (tlb_type == spitfire ? "upa-portid" : "portid");
 }
 
+bool arch_find_n_match_cpu_physical_id(struct device_node *cpun,
+				       int cpu, unsigned int *thread)
+{
+	const char *mid_prop = get_mid_prop();
+	int this_cpu_id;
+
+	/* On hypervisor based platforms we interrogate the 'reg'
+	 * property.  On everything else we look for a 'upa-portis',
+	 * 'portid', or 'cpuid' property.
+	 */
+
+	if (tlb_type == hypervisor) {
+		struct property *prop = of_find_property(cpun, "reg", NULL);
+		u32 *regs;
+
+		if (!prop) {
+			pr_warn("CPU node missing reg property\n");
+			return false;
+		}
+		regs = prop->value;
+		this_cpu_id = regs[0] & 0x0fffffff;
+	} else {
+		this_cpu_id = of_getintprop_default(cpun, mid_prop, -1);
+
+		if (this_cpu_id < 0) {
+			mid_prop = "cpuid";
+			this_cpu_id = of_getintprop_default(cpun, mid_prop, -1);
+		}
+		if (this_cpu_id < 0) {
+			pr_warn("CPU node missing cpu ID property\n");
+			return false;
+		}
+	}
+	if (this_cpu_id == cpu) {
+		if (thread) {
+			int proc_id = cpu_data(cpu).proc_id;
+
+			/* On sparc64, the cpu thread information is obtained
+			 * either from OBP or the machine description.  We've
+			 * actually probed this information already long before
+			 * this interface gets called so instead of interrogating
+			 * both the OF node and the MDESC again, just use what
+			 * we discovered already.
+			 */
+			if (proc_id < 0)
+				proc_id = 0;
+			*thread = proc_id;
+		}
+		return true;
+	}
+	return false;
+}
+
 static void *of_iterate_over_cpus(void *(*func)(struct device_node *, int, int), int arg)
 {
 	struct device_node *dp;
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 865d3f6..e4c9945 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -280,6 +280,31 @@ static bool __of_find_n_match_cpu_property(struct device_node *cpun,
 	return false;
 }
 
+/*
+ * arch_find_n_match_cpu_physical_id - See if the given device node is
+ * for the cpu corresponding to logical cpu 'cpu'.  Return true if so,
+ * else false.  If 'thread' is non-NULL, the local thread number within the
+ * core is returned in it.
+ */
+bool __weak arch_find_n_match_cpu_physical_id(struct device_node *cpun,
+					      int cpu, unsigned int *thread)
+{
+	/* Check for non-standard "ibm,ppc-interrupt-server#s" property
+	 * for thread ids on PowerPC. If it doesn't exist fallback to
+	 * standard "reg" property.
+	 */
+	if (IS_ENABLED(CONFIG_PPC) &&
+	    __of_find_n_match_cpu_property(cpun,
+					   "ibm,ppc-interrupt-server#s",
+					   cpu, thread))
+		return true;
+
+	if (__of_find_n_match_cpu_property(cpun, "reg", cpu, thread))
+		return true;
+
+	return false;
+}
+
 /**
  * of_get_cpu_node - Get device node associated with the given logical CPU
  *
@@ -300,26 +325,10 @@ static bool __of_find_n_match_cpu_property(struct device_node *cpun,
  */
 struct device_node *of_get_cpu_node(int cpu, unsigned int *thread)
 {
-	struct device_node *cpun, *cpus;
-
-	cpus = of_find_node_by_path("/cpus");
-	if (!cpus) {
-		pr_warn("Missing cpus node, bailing out\n");
-		return NULL;
-	}
+	struct device_node *cpun;
 
-	for_each_child_of_node(cpus, cpun) {
-		if (of_node_cmp(cpun->type, "cpu"))
-			continue;
-		/* Check for non-standard "ibm,ppc-interrupt-server#s" property
-		 * for thread ids on PowerPC. If it doesn't exist fallback to
-		 * standard "reg" property.
-		 */
-		if (IS_ENABLED(CONFIG_PPC) &&
-			__of_find_n_match_cpu_property(cpun,
-				"ibm,ppc-interrupt-server#s", cpu, thread))
-			return cpun;
-		if (__of_find_n_match_cpu_property(cpun, "reg", cpu, thread))
+	for_each_node_by_type(cpun, "cpu") {
+		if (arch_find_n_match_cpu_physical_id(cpun, cpu, thread))
 			return cpun;
 	}
 	return NULL;
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 801ff9e..fbd25c3 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -18,6 +18,7 @@
 #include <linux/cpumask.h>
 
 struct device;
+struct device_node;
 
 struct cpu {
 	int node_id;		/* The node which contains the CPU */
@@ -29,6 +30,8 @@ extern int register_cpu(struct cpu *cpu, int num);
 extern struct device *get_cpu_device(unsigned cpu);
 extern bool cpu_is_hotpluggable(unsigned cpu);
 extern bool arch_match_cpu_phys_id(int cpu, u64 phys_id);
+extern bool arch_find_n_match_cpu_physical_id(struct device_node *cpun,
+					      int cpu, unsigned int *thread);
 
 extern int cpu_add_dev_attr(struct device_attribute *attr);
 extern void cpu_remove_dev_attr(struct device_attribute *attr);
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: new cpu iteration code...

[Grant Likely]

On Thu, Oct 3, 2013 at 10:24 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>
> Can people on powerpc/arm/etc. give this a spin?  This works on my
> T4-2 sparc64 box.

The code looks fine. I'll give it a spin on ARM. I'm assuming that
this can wait for v3.13 if I commit the fix to suppress the warning,
correct?

g.

>
> --------------------
> of: Make cpu node handling more portable.
>
> Use for_each_node_by_type() to iterate all cpu nodes in the
> system.
>
> Provide and overridable function arch_find_n_match_cpu_physical_id,
> which sees if the given device node matches 'cpu' and if so sets
> '*thread' when non-NULL to the cpu thread number within the core.
>
> The default implementation behaves the same as the existing code.
>
> Add a sparc64 implementation.
>
> Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
>
> diff --git a/arch/sparc/kernel/prom_64.c b/arch/sparc/kernel/prom_64.c
> index d397d7f..6b39125 100644
> --- a/arch/sparc/kernel/prom_64.c
> +++ b/arch/sparc/kernel/prom_64.c
> @@ -373,6 +373,59 @@ static const char *get_mid_prop(void)
>         return (tlb_type == spitfire ? "upa-portid" : "portid");
>  }
>
> +bool arch_find_n_match_cpu_physical_id(struct device_node *cpun,
> +                                      int cpu, unsigned int *thread)
> +{
> +       const char *mid_prop = get_mid_prop();
> +       int this_cpu_id;
> +
> +       /* On hypervisor based platforms we interrogate the 'reg'
> +        * property.  On everything else we look for a 'upa-portis',
> +        * 'portid', or 'cpuid' property.
> +        */
> +
> +       if (tlb_type == hypervisor) {
> +               struct property *prop = of_find_property(cpun, "reg", NULL);
> +               u32 *regs;
> +
> +               if (!prop) {
> +                       pr_warn("CPU node missing reg property\n");
> +                       return false;
> +               }
> +               regs = prop->value;
> +               this_cpu_id = regs[0] & 0x0fffffff;
> +       } else {
> +               this_cpu_id = of_getintprop_default(cpun, mid_prop, -1);
> +
> +               if (this_cpu_id < 0) {
> +                       mid_prop = "cpuid";
> +                       this_cpu_id = of_getintprop_default(cpun, mid_prop, -1);
> +               }
> +               if (this_cpu_id < 0) {
> +                       pr_warn("CPU node missing cpu ID property\n");
> +                       return false;
> +               }
> +       }
> +       if (this_cpu_id == cpu) {
> +               if (thread) {
> +                       int proc_id = cpu_data(cpu).proc_id;
> +
> +                       /* On sparc64, the cpu thread information is obtained
> +                        * either from OBP or the machine description.  We've
> +                        * actually probed this information already long before
> +                        * this interface gets called so instead of interrogating
> +                        * both the OF node and the MDESC again, just use what
> +                        * we discovered already.
> +                        */
> +                       if (proc_id < 0)
> +                               proc_id = 0;
> +                       *thread = proc_id;
> +               }
> +               return true;
> +       }
> +       return false;
> +}
> +
>  static void *of_iterate_over_cpus(void *(*func)(struct device_node *, int, int), int arg)
>  {
>         struct device_node *dp;
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 865d3f6..e4c9945 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -280,6 +280,31 @@ static bool __of_find_n_match_cpu_property(struct device_node *cpun,
>         return false;
>  }
>
> +/*
> + * arch_find_n_match_cpu_physical_id - See if the given device node is
> + * for the cpu corresponding to logical cpu 'cpu'.  Return true if so,
> + * else false.  If 'thread' is non-NULL, the local thread number within the
> + * core is returned in it.
> + */
> +bool __weak arch_find_n_match_cpu_physical_id(struct device_node *cpun,
> +                                             int cpu, unsigned int *thread)
> +{
> +       /* Check for non-standard "ibm,ppc-interrupt-server#s" property
> +        * for thread ids on PowerPC. If it doesn't exist fallback to
> +        * standard "reg" property.
> +        */
> +       if (IS_ENABLED(CONFIG_PPC) &&
> +           __of_find_n_match_cpu_property(cpun,
> +                                          "ibm,ppc-interrupt-server#s",
> +                                          cpu, thread))
> +               return true;
> +
> +       if (__of_find_n_match_cpu_property(cpun, "reg", cpu, thread))
> +               return true;
> +
> +       return false;
> +}
> +
>  /**
>   * of_get_cpu_node - Get device node associated with the given logical CPU
>   *
> @@ -300,26 +325,10 @@ static bool __of_find_n_match_cpu_property(struct device_node *cpun,
>   */
>  struct device_node *of_get_cpu_node(int cpu, unsigned int *thread)
>  {
> -       struct device_node *cpun, *cpus;
> -
> -       cpus = of_find_node_by_path("/cpus");
> -       if (!cpus) {
> -               pr_warn("Missing cpus node, bailing out\n");
> -               return NULL;
> -       }
> +       struct device_node *cpun;
>
> -       for_each_child_of_node(cpus, cpun) {
> -               if (of_node_cmp(cpun->type, "cpu"))
> -                       continue;
> -               /* Check for non-standard "ibm,ppc-interrupt-server#s" property
> -                * for thread ids on PowerPC. If it doesn't exist fallback to
> -                * standard "reg" property.
> -                */
> -               if (IS_ENABLED(CONFIG_PPC) &&
> -                       __of_find_n_match_cpu_property(cpun,
> -                               "ibm,ppc-interrupt-server#s", cpu, thread))
> -                       return cpun;
> -               if (__of_find_n_match_cpu_property(cpun, "reg", cpu, thread))
> +       for_each_node_by_type(cpun, "cpu") {
> +               if (arch_find_n_match_cpu_physical_id(cpun, cpu, thread))
>                         return cpun;
>         }
>         return NULL;
> diff --git a/include/linux/cpu.h b/include/linux/cpu.h
> index 801ff9e..fbd25c3 100644
> --- a/include/linux/cpu.h
> +++ b/include/linux/cpu.h
> @@ -18,6 +18,7 @@
>  #include <linux/cpumask.h>
>
>  struct device;
> +struct device_node;
>
>  struct cpu {
>         int node_id;            /* The node which contains the CPU */
> @@ -29,6 +30,8 @@ extern int register_cpu(struct cpu *cpu, int num);
>  extern struct device *get_cpu_device(unsigned cpu);
>  extern bool cpu_is_hotpluggable(unsigned cpu);
>  extern bool arch_match_cpu_phys_id(int cpu, u64 phys_id);
> +extern bool arch_find_n_match_cpu_physical_id(struct device_node *cpun,
> +                                             int cpu, unsigned int *thread);
>
>  extern int cpu_add_dev_attr(struct device_attribute *attr);
>  extern void cpu_remove_dev_attr(struct device_attribute *attr);
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: new cpu iteration code...

[Sudeep KarkadaNagesha]

Hi David,

I tested this on ARM platform(Vexpress TC2). You can add my tested by.

Regards,
Sudeep

On 03/10/13 22:24, David Miller wrote:
> 
> Can people on powerpc/arm/etc. give this a spin?  This works on my
> T4-2 sparc64 box.
> 
> --------------------
> of: Make cpu node handling more portable.
> 
> Use for_each_node_by_type() to iterate all cpu nodes in the
> system.
> 
> Provide and overridable function arch_find_n_match_cpu_physical_id,
> which sees if the given device node matches 'cpu' and if so sets
> '*thread' when non-NULL to the cpu thread number within the core.
> 
> The default implementation behaves the same as the existing code.
> 
> Add a sparc64 implementation.
> 
> Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> 
Tested-by: Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha-5wv7dgnIgG8@public.gmane.org>


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: new cpu iteration code...

[Grant Likely]

On Fri, 04 Oct 2013 11:34:24 +0100, Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha-5wv7dgnIgG8@public.gmane.org> wrote:
> Hi David,
> 
> I tested this on ARM platform(Vexpress TC2). You can add my tested by.
> 
> Regards,
> Sudeep
> 
> On 03/10/13 22:24, David Miller wrote:
> > 
> > Can people on powerpc/arm/etc. give this a spin?  This works on my
> > T4-2 sparc64 box.
> > 
> > --------------------
> > of: Make cpu node handling more portable.
> > 
> > Use for_each_node_by_type() to iterate all cpu nodes in the
> > system.
> > 
> > Provide and overridable function arch_find_n_match_cpu_physical_id,
> > which sees if the given device node matches 'cpu' and if so sets
> > '*thread' when non-NULL to the cpu thread number within the core.
> > 
> > The default implementation behaves the same as the existing code.
> > 
> > Add a sparc64 implementation.
> > 
> > Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> > 
> Tested-by: Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha-5wv7dgnIgG8@public.gmane.org>

I've applied it to my tree and I'll keep an eye out if it causes any
breakage on PowerPC or ARM.

Can someone from PowerPC land give it a spin? I don't have any powerpc
hardware anymore.

g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: new cpu iteration code...

[David Miller]

From: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date: Fri, 4 Oct 2013 11:27:48 +0100

> On Thu, Oct 3, 2013 at 10:24 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>>
>> Can people on powerpc/arm/etc. give this a spin?  This works on my
>> T4-2 sparc64 box.
> 
> The code looks fine. I'll give it a spin on ARM. I'm assuming that
> this can wait for v3.13 if I commit the fix to suppress the warning,
> correct?

I'd rather a new piece of generic code be implemented properly.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html