| Message ID | 1376586580-5409-4-git-send-email-Sudeep.KarkadaNagesha@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2013-08-15 at 18:09 +0100, Sudeep KarkadaNagesha wrote: > From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com> > > Currently different drivers requiring to access cpu device node are > parsing the device tree themselves. Since the ordering in the DT need > not match the logical cpu ordering, the parsing logic needs to consider > that. However, this has resulted in lots of code duplication and in some > cases even incorrect logic. .../... > > +bool arch_match_cpu_phys_id(int cpu, u64 phys_id) > +{ > + return (int)phys_id == get_hard_smp_processor_id(cpu); > +} Naming is a bit gross. You might want to make it clearer that we are talking about CPU IDs in the device-tree here. > +static bool __of_find_n_match_cpu_property(struct device_node *cpun, > + const char *prop_name, int cpu, unsigned int *thread) > +{ > + const __be32 *cell; > + int ac, prop_len, tid; > + u64 hwid; > + > + ac = of_n_addr_cells(cpun); > + cell = of_get_property(cpun, prop_name, &prop_len); > + if (!cell) > + return false; > + prop_len /= sizeof(*cell); > + for (tid = 0; tid < prop_len; tid++) { > + hwid = of_read_number(cell, ac); > + if (arch_match_cpu_phys_id(cpu, hwid)) { > + if (thread) > + *thread = tid; > + return true; > + } Missing: cell += ac; > + } > + return false; > +} > + > /* Find the device node for a given logical cpu number, also returns the cpu > * local thread number (index in ibm,interrupt-server#s) if relevant and > * asked for (non NULL) > */ > struct device_node *of_get_cpu_node(int cpu, unsigned int *thread) > { > - int hardid; > - struct device_node *np; > + struct device_node *cpun, *cpus; > > - hardid = get_hard_smp_processor_id(cpu); > + cpus = of_find_node_by_path("/cpus"); > + if (!cpus) { > + pr_warn("Missing cpus node, bailing out\n"); > + return NULL; > + } > > - for_each_node_by_type(np, "cpu") { > - const u32 *intserv; > - unsigned int plen, t; > + for_each_child_of_node(cpus, cpun) { > + if (of_node_cmp(cpun->type, "cpu")) > + continue; > > /* Check for ibm,ppc-interrupt-server#s. If it doesn't exist > * fallback to "reg" property and assume no threads > */ > - intserv = of_get_property(np, "ibm,ppc-interrupt-server#s", > - &plen); > - if (intserv == NULL) { > - const u32 *reg = of_get_property(np, "reg", NULL); > - if (reg == NULL) > - continue; > - if (*reg == hardid) { > - if (thread) > - *thread = 0; > - return np; > - } > - } else { > - plen /= sizeof(u32); > - for (t = 0; t < plen; t++) { > - if (hardid == intserv[t]) { > - if (thread) > - *thread = t; > - return np; > - } > - } > - } > + if (__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; > } > return NULL; > } Cheers, Ben.
On Thu, 2013-08-15 at 18:09 +0100, Sudeep KarkadaNagesha wrote: > /* Check for ibm,ppc-interrupt-server#s. If it doesn't exist > * fallback to "reg" property and assume no threads > */ > - Oh and I forgot ... that comment is now wrong, since your code handles threads in the "reg" case... Cheers, Ben.
On 16/08/13 05:50, Benjamin Herrenschmidt wrote: > On Thu, 2013-08-15 at 18:09 +0100, Sudeep KarkadaNagesha wrote: >> /* Check for ibm,ppc-interrupt-server#s. If it doesn't exist >> * fallback to "reg" property and assume no threads >> */ >> - > > Oh and I forgot ... that comment is now wrong, since your code handles > threads in the "reg" case... > I have fixed it in the next patch while adding the documentation to these function. I wanted changes in this patch minimal. I can fix it here too if you insist on it. Regards, Sudeep
On 16/08/13 05:49, Benjamin Herrenschmidt wrote: > On Thu, 2013-08-15 at 18:09 +0100, Sudeep KarkadaNagesha wrote: >> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com> >> >> Currently different drivers requiring to access cpu device node are >> parsing the device tree themselves. Since the ordering in the DT need >> not match the logical cpu ordering, the parsing logic needs to consider >> that. However, this has resulted in lots of code duplication and in some >> cases even incorrect logic. > > .../... > >> >> +bool arch_match_cpu_phys_id(int cpu, u64 phys_id) >> +{ >> + return (int)phys_id == get_hard_smp_processor_id(cpu); >> +} > > Naming is a bit gross. You might want to make it clearer that > we are talking about CPU IDs in the device-tree here. > Any particular preference to the name or just a note is sufficient. Also unlike PPC, in ARM we don't set hard processor id value based values read from device tree. DT must contain the values matching to the hardware ID registers. >> +static bool __of_find_n_match_cpu_property(struct device_node *cpun, >> + const char *prop_name, int cpu, unsigned int *thread) >> +{ >> + const __be32 *cell; >> + int ac, prop_len, tid; >> + u64 hwid; >> + >> + ac = of_n_addr_cells(cpun); >> + cell = of_get_property(cpun, prop_name, &prop_len); >> + if (!cell) >> + return false; >> + prop_len /= sizeof(*cell); >> + for (tid = 0; tid < prop_len; tid++) { >> + hwid = of_read_number(cell, ac); >> + if (arch_match_cpu_phys_id(cpu, hwid)) { >> + if (thread) >> + *thread = tid; >> + return true; >> + } > > Missing: cell += ac; Ah, missed it while refactoring, will fix it. Thanks Regards, Sudeep
On Fri, 2013-08-16 at 09:48 +0100, Sudeep KarkadaNagesha wrote: > > Naming is a bit gross. You might want to make it clearer that > > we are talking about CPU IDs in the device-tree here. > > > Any particular preference to the name or just a note is sufficient. > Also unlike PPC, in ARM we don't set hard processor id value based > values read from device tree. DT must contain the values matching to the > hardware ID registers. This is exactly the same on ppc. We don't "set" HW values. The device-tree content matches the HW internals. Some processors have a "PIR" register as well which contains the HW value, in this case the device-tree must contain the same value as the PIR on that processor. > >> +static bool __of_find_n_match_cpu_property(struct device_node *cpun, > >> + const char *prop_name, int cpu, unsigned int *thread) > >> +{ > >> + const __be32 *cell; > >> + int ac, prop_len, tid; > >> + u64 hwid; > >> + > >> + ac = of_n_addr_cells(cpun); > >> + cell = of_get_property(cpun, prop_name, &prop_len); > >> + if (!cell) > >> + return false; > >> + prop_len /= sizeof(*cell); > >> + for (tid = 0; tid < prop_len; tid++) { > >> + hwid = of_read_number(cell, ac); > >> + if (arch_match_cpu_phys_id(cpu, hwid)) { > >> + if (thread) > >> + *thread = tid; > >> + return true; > >> + } > > > > Missing: cell += ac; > Ah, missed it while refactoring, will fix it. Thanks Ben. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
On 16/08/13 13:32, Benjamin Herrenschmidt wrote: > On Fri, 2013-08-16 at 09:48 +0100, Sudeep KarkadaNagesha wrote: > >>> Naming is a bit gross. You might want to make it clearer that >>> we are talking about CPU IDs in the device-tree here. >>> >> Any particular preference to the name or just a note is sufficient. >> Also unlike PPC, in ARM we don't set hard processor id value based >> values read from device tree. DT must contain the values matching to the >> hardware ID registers. > > This is exactly the same on ppc. We don't "set" HW values. The > device-tree content matches the HW internals. Some processors have a > "PIR" register as well which contains the HW value, in this case the > device-tree must contain the same value as the PIR on that processor. > Ok, I misread the function 'set_hard_smp_processor_id' function. BTW, you didn't mention if you are OK by just have this clearly documented in the function and/or you have any preference/better name. I will send the next version based on that. I have even compile tested :) now on PPC. Regards, Sudeep
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c index eb23ac9..594c9f9 100644 --- a/arch/powerpc/kernel/prom.c +++ b/arch/powerpc/kernel/prom.c @@ -865,45 +865,61 @@ static int __init prom_reconfig_setup(void) __initcall(prom_reconfig_setup); #endif +bool arch_match_cpu_phys_id(int cpu, u64 phys_id) +{ + return (int)phys_id == get_hard_smp_processor_id(cpu); +} + +static bool __of_find_n_match_cpu_property(struct device_node *cpun, + const char *prop_name, int cpu, unsigned int *thread) +{ + const __be32 *cell; + int ac, prop_len, tid; + u64 hwid; + + ac = of_n_addr_cells(cpun); + cell = of_get_property(cpun, prop_name, &prop_len); + if (!cell) + return false; + prop_len /= sizeof(*cell); + for (tid = 0; tid < prop_len; tid++) { + hwid = of_read_number(cell, ac); + if (arch_match_cpu_phys_id(cpu, hwid)) { + if (thread) + *thread = tid; + return true; + } + } + return false; +} + /* Find the device node for a given logical cpu number, also returns the cpu * local thread number (index in ibm,interrupt-server#s) if relevant and * asked for (non NULL) */ struct device_node *of_get_cpu_node(int cpu, unsigned int *thread) { - int hardid; - struct device_node *np; + struct device_node *cpun, *cpus; - hardid = get_hard_smp_processor_id(cpu); + cpus = of_find_node_by_path("/cpus"); + if (!cpus) { + pr_warn("Missing cpus node, bailing out\n"); + return NULL; + } - for_each_node_by_type(np, "cpu") { - const u32 *intserv; - unsigned int plen, t; + for_each_child_of_node(cpus, cpun) { + if (of_node_cmp(cpun->type, "cpu")) + continue; /* Check for ibm,ppc-interrupt-server#s. If it doesn't exist * fallback to "reg" property and assume no threads */ - intserv = of_get_property(np, "ibm,ppc-interrupt-server#s", - &plen); - if (intserv == NULL) { - const u32 *reg = of_get_property(np, "reg", NULL); - if (reg == NULL) - continue; - if (*reg == hardid) { - if (thread) - *thread = 0; - return np; - } - } else { - plen /= sizeof(u32); - for (t = 0; t < plen; t++) { - if (hardid == intserv[t]) { - if (thread) - *thread = t; - return np; - } - } - } + if (__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; } return NULL; }