ARM: sh-mobile: Use of_cpu_node_to_id() to read CPU node 'reg'

PATCH Superseded Rob Herring 2 participants 3 messages Mar 19, 2023 – Mar 20, 2023

Subsystems (3)

Arm (32-bit) Arm sub-architectures Arm/RISC-V/Renesas architecture

lore

Share & tools

lore

Rob Herring

author

2023-03-19 15:01 UTC #0 lore raw

PATCH 1/1 ARM: sh-mobile: Use of_cpu_node_to_id() to read CPU node 'reg'

Replace open coded CPU nodes reading of "reg" and translation to logical ID with of_cpu_node_to_id().

Signed-off-by Rob Herring <[email protected]>

---
 arch/arm/mach-shmobile/platsmp-apmu.c | 34 +++++++++++----------------
 1 file changed, 14 insertions(+), 20 deletions(-)

arch/arm/mach-shmobile/platsmp-apmu.c +14 ~~-20~~

index e771ce70e132..27cfe753c467 100644
@@ -10,6 +10,7 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
+#include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/smp.h>
 #include <linux/suspend.h>

@@ -210,7 +211,6 @@ static void apmu_parse_dt(void (*fn)(struct resource *res, int cpu, int bit))
 	struct device_node *np_apmu, *np_cpu;
 	struct resource res;
 	int bit, index;
-	u32 id;
 
 	for_each_matching_node(np_apmu, apmu_ids) {
 		/* only enable the cluster that includes the boot CPU */

@@ -218,33 +218,27 @@ static void apmu_parse_dt(void (*fn)(struct resource *res, int cpu, int bit))
 
 		for (bit = 0; bit < CONFIG_NR_CPUS; bit++) {
 			np_cpu = of_parse_phandle(np_apmu, "cpus", bit);
-			if (np_cpu) {
-				if (!of_property_read_u32(np_cpu, "reg", &id)) {
-					if (id == cpu_logical_map(0)) {
-						is_allowed = true;
-						of_node_put(np_cpu);
-						break;
-					}
-
-				}
+			if (np_cpu && of_cpu_node_to_id(np_cpu) == 0) {
+				is_allowed = true;
 				of_node_put(np_cpu);
+				break;
 			}
+			of_node_put(np_cpu);
 		}
 		if (!is_allowed)
 			continue;
 
 		for (bit = 0; bit < CONFIG_NR_CPUS; bit++) {
 			np_cpu = of_parse_phandle(np_apmu, "cpus", bit);
-			if (np_cpu) {
-				if (!of_property_read_u32(np_cpu, "reg", &id)) {
-					index = get_logical_index(id);
-					if ((index >= 0) &&
-					    !of_address_to_resource(np_apmu,
-								    0, &res))
-						fn(&res, index, bit);
-				}
-				of_node_put(np_cpu);
-			}
+			if (!np_cpu)
+				continue;
+
+			index = of_cpu_node_to_id(np_cpu);
+			if ((index >= 0) &&
+			    !of_address_to_resource(np_apmu, 0, &res))
+				fn(&res, index, bit);
+
+			of_node_put(np_cpu);
 		}
 	}
 }

Signature · 1 line

2.39.2

Geert Uytterhoeven

2023-03-20 19:23 UTC #1 lore raw

Hi Rob,

Quoted 4 lines · Rob Herring

Replace open coded CPU nodes reading of "reg" and translation to logical ID with of_cpu_node_to_id().
Signed-off-by Rob Herring <[email protected]>

Thanks for your patch!

Quoted 19 lines

arch/arm/mach-shmobile/platsmp-apmu.c +1 -1

@@ -10,6 +10,7 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
+#include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/smp.h>
 #include <linux/suspend.h>

@@ -210,7 +211,6 @@ static void apmu_parse_dt(void (*fn)(struct resource *res, int cpu, int bit))
        struct device_node *np_apmu, *np_cpu;
        struct resource res;
        int bit, index;
-       u32 id;
 
        for_each_matching_node(np_apmu, apmu_ids) {
                /* only enable the cluster that includes the boot CPU */

arch/arm/mach-shmobile/platsmp-apmu.c static void apmu_parse_dt(void (*fn)(struct resource *res, int cpu, int bit)) · line 219

Quoted 2 lines

arch/arm/mach-shmobile/platsmp-apmu.c +0 -0

@@ -218,33 +218,27 @@ static void apmu_parse_dt(void (*fn)(struct resource *res, int cpu, int bit))
 
                for (bit = 0; bit < CONFIG_NR_CPUS; bit++) {

This loops over all CPUs....

arch/arm/mach-shmobile/platsmp-apmu.c static void apmu_parse_dt(void (*fn)(struct resource *res, int cpu, int bit)) · line 221

Quoted 11 lines

arch/arm/mach-shmobile/platsmp-apmu.c +1 -9

                        np_cpu = of_parse_phandle(np_apmu, "cpus", bit);
-                       if (np_cpu) {
-                               if (!of_property_read_u32(np_cpu, "reg", &id)) {
-                                       if (id == cpu_logical_map(0)) {
-                                               is_allowed = true;
-                                               of_node_put(np_cpu);
-                                               break;
-                                       }
-
-                               }
+                       if (np_cpu && of_cpu_node_to_id(np_cpu) == 0) {

As of_cpu_node_to_id() uses for_each_possible_cpu(), you're converting an O(n) operation to O(n²). I'm sure this can be done more efficiently, using for_each_possible_cpu() as the outer loop?

Meh, cpu_logical_map() also loops over all CPUs, so it was already O(n²)... Still, we should do better...

Quoted 15 lines

arch/arm/mach-shmobile/platsmp-apmu.c +3 -4

+                               is_allowed = true;
                                of_node_put(np_cpu);
+                               break;
                        }
+                       of_node_put(np_cpu);
                }
                if (!is_allowed)
                        continue;
 
                for (bit = 0; bit < CONFIG_NR_CPUS; bit++) {
                        np_cpu = of_parse_phandle(np_apmu, "cpus", bit);
-                       if (np_cpu) {
-                               if (!of_property_read_u32(np_cpu, "reg", &id)) {
-                                       index = get_logical_index(id);
-                                       if ((index >= 0) &&

arch/arm/mach-shmobile/platsmp-apmu.c static void apmu_parse_dt(void (*fn)(struct resource *res, int cpu, int bit)) · line 236

Quoted 10 lines

arch/arm/mach-shmobile/platsmp-apmu.c +4 -6

-                                           !of_address_to_resource(np_apmu,
-                                                                   0, &res))
-                                               fn(&res, index, bit);
-                               }
-                               of_node_put(np_cpu);
-                       }
+                       if (!np_cpu)
+                               continue;
+
+                       index = of_cpu_node_to_id(np_cpu);

Likewise.

arch/arm/mach-shmobile/platsmp-apmu.c static void apmu_parse_dt(void (*fn)(struct resource *res, int cpu, int bit)) · line 244

Quoted 8 lines

arch/arm/mach-shmobile/platsmp-apmu.c +5 -0

+                       if ((index >= 0) &&
+                           !of_address_to_resource(np_apmu, 0, &res))
+                               fn(&res, index, bit);
+
+                       of_node_put(np_cpu);
                }
        }
 }

Gr{oetje,eeting}s,

Geert

Signature · 10 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Rob Herring

author

2023-03-20 22:35 UTC #2 lore raw

Quoted 27 lines · Geert Uytterhoeven

Hi Rob,
Quoted 4 lines · Rob Herring
Replace open coded CPU nodes reading of "reg" and translation to logical ID with of_cpu_node_to_id().
Signed-off-by Rob Herring <[email protected]>
Thanks for your patch!
Quoted 16 lines
arch/arm/mach-shmobile/platsmp-apmu.c +1 -1
@@ -10,6 +10,7 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
+#include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/smp.h>
 #include <linux/suspend.h>
@@ -210,7 +211,6 @@ static void apmu_parse_dt(void (*fn)(struct resource *res, int cpu, int bit))
        struct device_node *np_apmu, *np_cpu;
        struct resource res;
        int bit, index;
-       u32 id;
 

Quoted 9 lines

arch/arm/mach-shmobile/platsmp-apmu.c static void apmu_parse_dt(void (*fn)(struct resource *res, int cpu, int bit)) · line 219
Quoted 5 lines
arch/arm/mach-shmobile/platsmp-apmu.c +0 -0
        for_each_matching_node(np_apmu, apmu_ids) {
                /* only enable the cluster that includes the boot CPU */
@@ -218,33 +218,27 @@ static void apmu_parse_dt(void (*fn)(struct resource *res, int cpu, int bit))
 
                for (bit = 0; bit < CONFIG_NR_CPUS; bit++) {
This loops over all CPUs....
Quoted 1 line
arch/arm/mach-shmobile/platsmp-apmu.c +0 -0
                        np_cpu = of_parse_phandle(np_apmu, "cpus", bit);

Have you looked at what this does? :)

Quoted 11 lines

arch/arm/mach-shmobile/platsmp-apmu.c static void apmu_parse_dt(void (*fn)(struct resource *res, int cpu, int bit)) · line 221

Quoted 10 lines

arch/arm/mach-shmobile/platsmp-apmu.c +1 -9

-                       if (np_cpu) {
-                               if (!of_property_read_u32(np_cpu, "reg", &id)) {
-                                       if (id == cpu_logical_map(0)) {
-                                               is_allowed = true;
-                                               of_node_put(np_cpu);
-                                               break;
-                                       }
-
-                               }
+                       if (np_cpu && of_cpu_node_to_id(np_cpu) == 0) {

Quoted 6 lines

As of_cpu_node_to_id() uses for_each_possible_cpu(), you're converting an O(n) operation to O(n²). I'm sure this can be done more efficiently, using for_each_possible_cpu() as the outer loop?
Meh, cpu_logical_map() also loops over all CPUs, so it was already O(n²)... Still, we should do better...

Can you measure the performance impact?

I would assume 'cpus' is less than NR_CPUS or possible cpus. We should cap the outer loop based on 'cpus' length. The simplest fix is bail when of_parse_phandle() fails. That will work unless you expect empty entries in 'cpus':

cpus = <&cpu0>, <0>, <&cpu3>;

Otherwise, we'd need to use of_parse_phandle_with_fixed_args() instead to distinguish empty entry vs. the end of the list. There is a of_for_each_phandle() iterator which would avoid walking the 'cpus' entry each time from the beginning, but it's a bit more complicated since it handles arg cells.

This is the simple fix:

(quoted diff) +0 -0

b/arch/arm/mach-shmobile/platsmp-apmu.c
index 27cfe753c467..ec6f421c0f4d 100644

arch/arm/mach-shmobile/platsmp-apmu.c +0 -0

@@ -218,7 +218,9 @@ static void apmu_parse_dt(void (*fn)(struct

resource *res, int cpu, int bit))

                for (bit = 0; bit < CONFIG_NR_CPUS; bit++) {
                        np_cpu = of_parse_phandle(np_apmu, "cpus", bit);
-                       if (np_cpu && of_cpu_node_to_id(np_cpu) == 0) {
+                       if (!np_cpu)
+                               break;
+                       if (of_cpu_node_to_id(np_cpu) == 0) {
                                is_allowed = true;
                                of_node_put(np_cpu);
                                break;
@@ -231,7 +233,7 @@ static void apmu_parse_dt(void (*fn)(struct
resource *res, int cpu, int bit))
                for (bit = 0; bit < CONFIG_NR_CPUS; bit++) {
                        np_cpu = of_parse_phandle(np_apmu, "cpus", bit);
                        if (!np_cpu)

- continue;
+ break;

                        index = of_cpu_node_to_id(np_cpu);
                        if ((index >= 0) &&