On Thu, Mar 20, 2014 at 01:43:57PM +0000, Mark Brown wrote:
On Thu, Mar 20, 2014 at 11:26:50AM +0000, Lorenzo Pieralisi wrote:
On Wed, Mar 19, 2014 at 06:02:17PM +0000, Mark Brown wrote:
+#ifdef CONFIG_OF
This ifdef can be removed, CONFIG_OF is always selected for arm64 and the !CONFIG_OF path
This has been present since the very first time these patches were posted but hasn't been mentioned as being a problem previously.
I am sorry, I missed it, doing my best to help you get it through.
+#else +static inline int parse_dt_topology(void) { return 0; } +#endif
is wrong, it should return failure. You should remove the CONFIG_OF ifdeffery.
Yup. It actually won't affect the behaviour at present though - since it won't do anything the result will be just the same as if we return an error and reset.
Given ACPI (which really looks like it's going to happen at some point and presumably make OF optional) I'm not sure removing the handling of OF is actually constructive but whatever, it's done now...
DT is there to stay, regardless of ACPI. However, given the function call logic, returning 0 on !CONFIG_OF was correct since it meant "no cpu-map". Anyway, CONFIG_OF ifdef should be removed.
if (leaf) {
ret = parse_core(c, cluster_id, core_id++);
if (ret != 0) {
return ret;
}
WARNING: braces {} are not necessary for single statement blocks #139: FILE: arch/arm64/kernel/topology.c:132:
Like I say I don't think checkpatch is being helpful on this one, the code looks worse. Again, whatever.
Worse or better, it has to be consistent. Either you leave them everywhere (but there is a coding style, it is for a reason) or you remove them everywhere (there are other nested paths where it is removed in the patch). Do not take it as a nitpick please, I just want the code to be consistent.
We still have a problem here. If the topology does not contain bindings for some cpu nodes, parse_cluster() does not fail and we end up with an incomplete topology. We have two choices: either we check the topology
Hrm, looking at the topology binding it doesn't specificially require that the topology be complete. I can see why you would want that.
I'd rather do it here, in preparation for MPIDR_EL1 fallback solution (where there will always be topology information configured and the register will always be there in all its glory).
To be honest at this point I think what I want to do is go back to the original approach of layering DT on top of MPIDR. MPIDR is smaller and simpler code so seems more likely to make progress. I really do expect that for a very large proportion of systems it'll be sufficient.
DT (cpu-map) takes precedence though. Yes, instead of resetting the topology, falling back to MPIDR_EL1 is acceptable if either there are broken bindings or cpus with missing topology information.
Honestly, it is not up to the kernel to validate DT, since this adds complexity, but I think that a big fat WARN_ON on missing or broken topology information would help fix firmware at early stages.
You should fall back to HW MPIDR_EL1.
I know, it is complex, there is little we can do about that and it is code run just at cold boot and freed later so I deem that acceptable.
Thanks, Lorenzo