| Message ID | 1389554441-27335-3-git-send-email-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Mark, apart from a couple of minor nits and a question, it looks fine to me. On Sun, Jan 12, 2014 at 07:20:39PM +0000, Mark Brown wrote: > +static void __init parse_core(struct device_node *core, int cluster_id, > + int core_id) > +{ > + char name[10]; > + bool leaf = true; > + int i, cpu; > + struct device_node *t; > + > + i = 0; You could initialize i at declaration, I can understand why you are doing that explictly in parse_cluster (two loops, to make code clearer), but here it does not make much sense to add a line for that. > + do { > + snprintf(name, sizeof(name), "thread%d", i); If we wanted to be very picky, you need to copy "thread" just once (same goes for other strings), but we'd better leave code as is IMHO. > + t = of_get_child_by_name(core, name); Should we check the MT bit in MPIDR_EL1 before validating threads as well ? I do not like the idea because this means reliance on MPIDR_EL1 for MT and DT for topology bits, but it might be a worthwhile check. It is certainly odd to have a DT with threads and an MPIDR_EL1 with the MT bit clear. > + if (t) { > + leaf = false; > + cpu = get_cpu_for_node(t); > + if (cpu >= 0) { > + cpu_topology[cpu].socket_id = cluster_id; > + cpu_topology[cpu].core_id = core_id; > + cpu_topology[cpu].thread_id = i; > + } else { > + pr_err("%s: Can't get CPU for thread\n", > + t->full_name); > + } > + } > + i++; > + } while (t); > + > + cpu = get_cpu_for_node(core); > + if (cpu >= 0) { > + if (!leaf) { > + pr_err("%s: Core has both threads and CPU\n", > + core->full_name); > + return; > + } > + > + cpu_topology[cpu].socket_id = cluster_id; > + cpu_topology[cpu].core_id = core_id; > + } else if (leaf) { > + pr_err("%s: Can't get CPU for leaf core\n", core->full_name); > + } > +} > + > +static void __init parse_cluster(struct device_node *cluster, int depth) > +{ > + char name[10]; > + bool leaf = true; > + bool has_cores = false; > + struct device_node *c; > + static int cluster_id = 0; static int __initdata cluster_id; [...] > +static void __init parse_dt_topology(void) > +{ > + struct device_node *cn; > + > + cn = of_find_node_by_path("/cpus"); > + if (!cn) { > + pr_err("No CPU information found in DT\n"); > + return; > + } > + > + /* > + * If topology is provided as a cpu-map it is essentially a > + * root cluster. This comment is a bit misleading, because as you know, (1) topology can only be provided with cpu-map, (2) cpu-map is not a root cluster. > + */ > + cn = of_find_node_by_name(cn, "cpu-map"); > + if (!cn) > + return; > + parse_cluster(cn, 0); > +} > + > +#else > +static inline void parse_dt_topology(void) {} > +#endif > + > /* > * cpu topology table > */ > @@ -87,5 +227,8 @@ void __init init_cpu_topology(void) > cpumask_clear(&cpu_topo->core_sibling); > cpumask_clear(&cpu_topo->thread_sibling); > } > + > + parse_dt_topology(); > + > smp_wmb(); > } With the changes/comments above pending: Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
On Tue, Jan 14, 2014 at 11:43:37AM +0000, Lorenzo Pieralisi wrote: > On Sun, Jan 12, 2014 at 07:20:39PM +0000, Mark Brown wrote: > > +static void __init parse_core(struct device_node *core, int cluster_id, > > + int core_id) > > +{ > > + char name[10]; > > + bool leaf = true; > > + int i, cpu; > > + struct device_node *t; > > + > > + i = 0; > You could initialize i at declaration, I can understand why you are doing that > explictly in parse_cluster (two loops, to make code clearer), but here > it does not make much sense to add a line for that. I still find it clearer for do { } while loops to have the start condition required for the loop to function right next to the loop. Yes, you can save a line code but that's about it. > > + do { > > + snprintf(name, sizeof(name), "thread%d", i); > If we wanted to be very picky, you need to copy "thread" just once (same > goes for other strings), but we'd better leave code as is IMHO. That would just make the code more complex, we need to handle tens of cores so just doing i + '0' won't cut it. > > + t = of_get_child_by_name(core, name); > Should we check the MT bit in MPIDR_EL1 before validating threads as well ? > I do not like the idea because this means reliance on MPIDR_EL1 for MT > and DT for topology bits, but it might be a worthwhile check. > It is certainly odd to have a DT with threads and an MPIDR_EL1 with the MT > bit clear. Checking seems counter to the idea of forcing everyone to provide this information from the firmware in the first place - checking that one bit and ignoring the rest of the information even if it's good would seem perverse.
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c index 980019fefeff..7ef0d783ffff 100644 --- a/arch/arm64/kernel/topology.c +++ b/arch/arm64/kernel/topology.c @@ -17,10 +17,150 @@ #include <linux/percpu.h> #include <linux/node.h> #include <linux/nodemask.h> +#include <linux/of.h> #include <linux/sched.h> #include <asm/topology.h> +#ifdef CONFIG_OF +static int __init get_cpu_for_node(struct device_node *node) +{ + struct device_node *cpu_node; + int cpu; + + cpu_node = of_parse_phandle(node, "cpu", 0); + if (!cpu_node) + return -1; + + for_each_possible_cpu(cpu) { + if (of_get_cpu_node(cpu, NULL) == cpu_node) + return cpu; + } + + pr_crit("Unable to find CPU node for %s\n", cpu_node->full_name); + return -1; +} + +static void __init parse_core(struct device_node *core, int cluster_id, + int core_id) +{ + char name[10]; + bool leaf = true; + int i, cpu; + struct device_node *t; + + i = 0; + do { + snprintf(name, sizeof(name), "thread%d", i); + t = of_get_child_by_name(core, name); + if (t) { + leaf = false; + cpu = get_cpu_for_node(t); + if (cpu >= 0) { + cpu_topology[cpu].socket_id = cluster_id; + cpu_topology[cpu].core_id = core_id; + cpu_topology[cpu].thread_id = i; + } else { + pr_err("%s: Can't get CPU for thread\n", + t->full_name); + } + } + i++; + } while (t); + + cpu = get_cpu_for_node(core); + if (cpu >= 0) { + if (!leaf) { + pr_err("%s: Core has both threads and CPU\n", + core->full_name); + return; + } + + cpu_topology[cpu].socket_id = cluster_id; + cpu_topology[cpu].core_id = core_id; + } else if (leaf) { + pr_err("%s: Can't get CPU for leaf core\n", core->full_name); + } +} + +static void __init parse_cluster(struct device_node *cluster, int depth) +{ + char name[10]; + bool leaf = true; + bool has_cores = false; + struct device_node *c; + static int cluster_id = 0; + int core_id = 0; + int i; + + /* + * First check for child clusters; we currently ignore any + * information about the nesting of clusters and present the + * scheduler with a flat list of them. + */ + i = 0; + do { + snprintf(name, sizeof(name), "cluster%d", i); + c = of_get_child_by_name(cluster, name); + if (c) { + parse_cluster(c, depth + 1); + leaf = false; + } + i++; + } while (c); + + /* Now check for cores */ + i = 0; + do { + snprintf(name, sizeof(name), "core%d", i); + c = of_get_child_by_name(cluster, name); + if (c) { + has_cores = true; + + if (depth == 0) + pr_err("%s: cpu-map children should be clusters\n", + c->full_name); + + if (leaf) + parse_core(c, cluster_id, core_id++); + else + pr_err("%s: Non-leaf cluster with core %s\n", + cluster->full_name, name); + } + i++; + } while (c); + + if (leaf && !has_cores) + pr_warn("%s: empty cluster\n", cluster->full_name); + + if (leaf) + cluster_id++; +} + +static void __init parse_dt_topology(void) +{ + struct device_node *cn; + + cn = of_find_node_by_path("/cpus"); + if (!cn) { + pr_err("No CPU information found in DT\n"); + return; + } + + /* + * If topology is provided as a cpu-map it is essentially a + * root cluster. + */ + cn = of_find_node_by_name(cn, "cpu-map"); + if (!cn) + return; + parse_cluster(cn, 0); +} + +#else +static inline void parse_dt_topology(void) {} +#endif + /* * cpu topology table */ @@ -87,5 +227,8 @@ void __init init_cpu_topology(void) cpumask_clear(&cpu_topo->core_sibling); cpumask_clear(&cpu_topo->thread_sibling); } + + parse_dt_topology(); + smp_wmb(); }