diff mbox series

[RESEND] arm: topology: Fix missing clock-frequency property warning

Message ID 20240201123605.3037829-2-stefan.wiehler@nokia.com
State New
Headers
Series [RESEND] arm: topology: Fix missing clock-frequency property warning |

Commit Message

Stefan Wiehler Feb. 1, 2024, 12:36 p.m. UTC 
  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(-)

Comments

Krzysztof Kozlowski Feb. 1, 2024, 1:16 p.m. UTC | #1 
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
Stefan Wiehler Feb. 1, 2024, 3:03 p.m. UTC | #2 
> 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
Krzysztof Kozlowski Feb. 2, 2024, 10:58 a.m. UTC | #3 
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 mbox series

Patch

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);
 }
 
 /*