| Message ID | 20240201123605.3037829-2-stefan.wiehler@nokia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] arm: topology: Fix missing clock-frequency property warning | expand |
Sorry, I was going to reply to this, but having composed the reply, and attempting to add the DT maintainers, mutt decided to completely obliterate the To: line. Please check with the DT maintainers what the expected behaviour is supposed to be. On Thu, Feb 01, 2024 at 01:36:06PM +0100, Stefan Wiehler wrote: > The devicetree specification in section 3.8 however specifies that > "properties that have identical values across cpu nodes may be placed in > the /cpus node instead. Does this mean the /cpus property is like a default for when a CPU node doesn't specify the clock frequency, or does it mean that the /cpus property should only exist when all the values for each CPU are identical and thus the individual CPU node clock frequency should not be specified.
On 01/02/2024 13:36, Stefan Wiehler wrote: > If the clock-frequency property is not set in the cpu node but in the > parent /cpus node, the following warning is emitted: > > /cpus/cpu@X missing clock-frequency property > > The devicetree specification in section 3.8 however specifies that > "properties that have identical values across cpu nodes may be placed in > the /cpus node instead. A client program must first examine a specific cpu > node, but if an expected property is not found then it should look at the > parent /cpus node." > > Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com> > --- > arch/arm/kernel/topology.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c > index ef0058de432b..32fc1c8d9d11 100644 > --- a/arch/arm/kernel/topology.c > +++ b/arch/arm/kernel/topology.c > @@ -85,15 +85,24 @@ static bool cap_from_dt = true; > static void __init parse_dt_topology(void) > { > const struct cpu_efficiency *cpu_eff; > - struct device_node *cn = NULL; > + struct device_node *pcn = NULL, *cn = NULL; > unsigned long min_capacity = ULONG_MAX; > unsigned long max_capacity = 0; > unsigned long capacity = 0; > int cpu = 0; > + const __be32 *common_rate; > + int common_rate_len; > > __cpu_capacity = kcalloc(nr_cpu_ids, sizeof(*__cpu_capacity), > GFP_NOWAIT); > > + pcn = of_find_node_by_path("/cpus"); > + if (!pcn) { > + pr_err("missing CPU root device node\n"); > + return; > + } > + common_rate = of_get_property(pcn, "clock-frequency", &common_rate_len); Aren't you adding new property? Is it already documented in the bindings? After a quick look I think this is not documented. Best regards, Krzysztof
> Does this mean the /cpus property is like a default for when a CPU node > doesn't specify the clock frequency, or does it mean that the /cpus > property should only exist when all the values for each CPU are > identical and thus the individual CPU node clock frequency should > not be specified. Good question, the device tree specification in Section 3.7 [1] says: > The /cpus node may contain properties that are common across cpu nodes. See Section 3.8 for details. And in Section 3.8 [2]: > Properties that have identical values across cpu nodes may be placed > in the /cpus node instead. A client program must first examine a > specific cpu node, but if an expected property is not found then it > should look at the parent /cpus node. This results in a less verbose > representation of properties which are identical across all CPUs. So I think it is pretty clear that it should only be used for common/identical values. > Aren't you adding new property? Is it already documented in the > bindings? After a quick look I think this is not documented. You are right, clock-frequency is not mentioned neither in arm/cpus.yaml nor in any other <arch>/cpus.yaml binding, but the DT spec has it as a required property [3]. Should I add clock-frequency to all <arch>/cpus.yaml bindings? Only the ARM one explicitly mentions following the DT spec. Kind regards, Stefan [1] https://devicetree-specification.readthedocs.io/en/latest/chapter3-devicenodes.html#cpus-node-properties [2] https://devicetree-specification.readthedocs.io/en/latest/chapter3-devicenodes.html#cpus-cpu-node-properties [3] https://devicetree-specification.readthedocs.io/en/latest/chapter3-devicenodes.html#general-properties-of-cpus-cpu-nodes
On Thu, Feb 01, 2024 at 04:03:59PM +0100, Stefan Wiehler wrote: > > Does this mean the /cpus property is like a default for when a CPU node > > doesn't specify the clock frequency, or does it mean that the /cpus > > property should only exist when all the values for each CPU are > > identical and thus the individual CPU node clock frequency should > > not be specified. > > Good question, the device tree specification in Section 3.7 [1] says: > > > The /cpus node may contain properties that are common across cpu > nodes. See Section 3.8 for details. > > And in Section 3.8 [2]: > > > Properties that have identical values across cpu nodes may be placed > > in the /cpus node instead. A client program must first examine a > > specific cpu node, but if an expected property is not found then it > > should look at the parent /cpus node. This results in a less verbose > > representation of properties which are identical across all CPUs. > > So I think it is pretty clear that it should only be used for > common/identical values. Thanks for the clarification. As this is DT specified behaviour, I question whether it should be implemented in arch/arm/kernel/topology.c - what I'm meaning is a helper such as: const void *of_get_cpu_property(const struct device_node *node, const char *name, int *lenp) { const void *res; res = of_get_property(node, name, lenp); if (!res) { node = of_find_node_by_path("/cpus"); if (node) res = of_get_property(node, name, lenp); of_node_put(node); } return res; } ?
On 01/02/2024 16:03, Stefan Wiehler wrote: >> Does this mean the /cpus property is like a default for when a CPU node >> doesn't specify the clock frequency, or does it mean that the /cpus >> property should only exist when all the values for each CPU are >> identical and thus the individual CPU node clock frequency should >> not be specified. > > Good question, the device tree specification in Section 3.7 [1] says: > > > The /cpus node may contain properties that are common across cpu > nodes. See Section 3.8 for details. > > And in Section 3.8 [2]: > > > Properties that have identical values across cpu nodes may be placed > > in the /cpus node instead. A client program must first examine a > > specific cpu node, but if an expected property is not found then it > > should look at the parent /cpus node. This results in a less verbose > > representation of properties which are identical across all CPUs. > > So I think it is pretty clear that it should only be used for > common/identical values. > >> Aren't you adding new property? Is it already documented in the >> bindings? After a quick look I think this is not documented. > > You are right, clock-frequency is not mentioned neither in arm/cpus.yaml > nor in any other <arch>/cpus.yaml binding, but the DT spec has it as a > required property [3]. Should I add clock-frequency to all > <arch>/cpus.yaml bindings? Only the ARM one explicitly mentions > following the DT spec. It should go to dtschema. dtschema cpu.yaml has it, so you need to propose such to cpus.yaml, probably you could experiment with: not: - required: - clock-frequency - patternProperties: cpu@.... - required: - clock-frequency Anyway, you cannot just keep adding some OF properties to the code without documenting them. Best regards, Krzysztof
diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c index ef0058de432b..32fc1c8d9d11 100644 --- a/arch/arm/kernel/topology.c +++ b/arch/arm/kernel/topology.c @@ -85,15 +85,24 @@ static bool cap_from_dt = true; static void __init parse_dt_topology(void) { const struct cpu_efficiency *cpu_eff; - struct device_node *cn = NULL; + struct device_node *pcn = NULL, *cn = NULL; unsigned long min_capacity = ULONG_MAX; unsigned long max_capacity = 0; unsigned long capacity = 0; int cpu = 0; + const __be32 *common_rate; + int common_rate_len; __cpu_capacity = kcalloc(nr_cpu_ids, sizeof(*__cpu_capacity), GFP_NOWAIT); + pcn = of_find_node_by_path("/cpus"); + if (!pcn) { + pr_err("missing CPU root device node\n"); + return; + } + common_rate = of_get_property(pcn, "clock-frequency", &common_rate_len); + for_each_possible_cpu(cpu) { const __be32 *rate; int len; @@ -121,8 +130,12 @@ static void __init parse_dt_topology(void) rate = of_get_property(cn, "clock-frequency", &len); if (!rate || len != 4) { - pr_err("%pOF missing clock-frequency property\n", cn); - continue; + if (common_rate && common_rate_len == 4) { + rate = common_rate; + } else { + pr_err("%pOF missing clock-frequency property\n", cn); + continue; + } } capacity = ((be32_to_cpup(rate)) >> 20) * cpu_eff->efficiency; @@ -154,6 +167,8 @@ static void __init parse_dt_topology(void) if (cap_from_dt) topology_normalize_cpu_scale(); + + of_node_put(pcn); } /*
If the clock-frequency property is not set in the cpu node but in the parent /cpus node, the following warning is emitted: /cpus/cpu@X missing clock-frequency property The devicetree specification in section 3.8 however specifies that "properties that have identical values across cpu nodes may be placed in the /cpus node instead. A client program must first examine a specific cpu node, but if an expected property is not found then it should look at the parent /cpus node." Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com> --- arch/arm/kernel/topology.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)