[PATCH v2 0/6] DT: Enable sharing resources for SMT threads

[PATCH v2 0/6] DT: Enable sharing resources for SMT threads

[Alireza Sanaee]

This patchset allows for sharing resources between SMT threads in the
device tree (DT).

WHY? Given the current use of the DT, it is not possible to share L1
caches, as well as other resources such as clock among SMT threads.
However, DT spec in section Section 3.8.1 [1], describes how SMT threads
can be described in the reg array, this is how PowerPC describes SMT
threads in DT.

CHALLENGE: Given discussions with the community [2], it was apparent
that it is not straightforward to implement this, since cpu-maps must
point to a particular CPU node in DT [3], Section 2.1. However, it is
not only the cpu-map but also there other nodes that point to cpu nodes
which indeed need care and changes.

SOLUTION: This led to more discussions on what the solution should look
like and based on recent conversations we ended up with the following
approach [4].

core0 {
  thread0 {
    cpu = <&cpu0 0>;
  };
  thread1 {
    cpu = <&cpu0 1>;
  };
};

In this layout, first parameter is the phandle to cpu-node and second
index would be the local-thread index in the reg array available in the
cpu-node reg property.

[1] https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4/devicetree-specification-v0.4.pdf
[2] https://lore.kernel.org/linux-arm-kernel/Z4FJZPRg75YIUR2l@J2N7QTR9R3/
[3] https://www.kernel.org/doc/Documentation/devicetree/bindings/cpu/cpu-topology.txt
[4] https://lore.kernel.org/devicetree-spec/CAL_JsqK1yqRLD9B+G7UUp=D8K++mXHq0Rmv=1i6DL_jXyZwXAw@mail.gmail.com/

PRIOR VERSIONs:
   [V1] https://lore.kernel.org/all/20250422084340.457-1-alireza.sanaee@huawei.com/

CHANGE LOG:
    V1 -> V2:
        * Address Rob's comments.
            ** Re-order patches.
            ** Fix bugs.
        * Remove #cpu-cells property

Alireza Sanaee (6):
  of: add infra for finding CPU id from phandle
  arch_topology: update CPU map to use the new API
  coresight: cti: Use of_cpu_phandle_to_id for grabbing CPU id
  coresight: Use of_cpu_phandle_to_id for grabbing CPU id
  arm64: of: handle multiple threads in ARM cpu node
  of: of_cpu_phandle_to_id to support SMT threads

 arch/arm64/kernel/smp.c                       | 74 ++++++++++---------
 drivers/base/arch_topology.c                  | 12 +--
 .../coresight/coresight-cti-platform.c        | 15 +---
 .../hwtracing/coresight/coresight-platform.c  | 14 +---
 drivers/of/cpu.c                              | 51 +++++++++++++
 include/linux/of.h                            |  8 ++
 6 files changed, 111 insertions(+), 63 deletions(-)

-- 
2.34.1

[PATCH v2 1/6] of: add infra for finding CPU id from phandle

[Alireza Sanaee]

Get CPU id from phandle. Many drivers get do this by getting hold of CPU
node first through a phandle and then find the CPU ID using the relevant
function. This commit encapsulates cpu node finding and improves
readability.

The API interface requires two parameters, 1) node, 2) pointer to CPU
node. API sets the pointer to the CPU node and allows the driver to play
with the CPU itself, for logging purposes for instance.
---
 drivers/of/cpu.c   | 24 ++++++++++++++++++++++++
 include/linux/of.h |  8 ++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/of/cpu.c b/drivers/of/cpu.c
index 5214dc3d05ae..1e8d1fa04d3c 100644
--- a/drivers/of/cpu.c
+++ b/drivers/of/cpu.c
@@ -173,6 +173,30 @@ int of_cpu_node_to_id(struct device_node *cpu_node)
 }
 EXPORT_SYMBOL(of_cpu_node_to_id);
 
+/**
+ * of_cpu_phandle_to_id: Get the logical CPU number for a given device_node
+ *
+ * @node: Pointer to the device_node containing CPU phandle.
+ * @cpu_np: Pointer to the device_node for CPU.
+ *
+ * Return: The logical CPU number of the given CPU device_node or -ENODEV if
+ * the CPU is not found. If the property is not found, it returns -1. On
+ * success, cpu_np will always point to the retrieved CPU device_node.
+ */
+int of_cpu_phandle_to_id(const struct device_node *node,
+			 struct device_node **cpu_np)
+{
+	if (!node)
+		return -1;
+
+	*cpu_np = of_parse_phandle(node, "cpu", 0);
+	if (!cpu_np)
+		return -ENODEV;
+
+	return of_cpu_node_to_id(*cpu_np);
+}
+EXPORT_SYMBOL(of_cpu_phandle_to_id);
+
 /**
  * of_get_cpu_state_node - Get CPU's idle state node at the given index
  *
diff --git a/include/linux/of.h b/include/linux/of.h
index eaf0e2a2b75c..4087f516b3db 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -360,6 +360,8 @@ extern const void *of_get_property(const struct device_node *node,
 extern struct device_node *of_get_cpu_node(int cpu, unsigned int *thread);
 extern struct device_node *of_cpu_device_node_get(int cpu);
 extern int of_cpu_node_to_id(struct device_node *np);
+extern int of_cpu_phandle_to_id(const struct device_node *np,
+				struct device_node **cpu_np);
 extern struct device_node *of_get_next_cpu_node(struct device_node *prev);
 extern struct device_node *of_get_cpu_state_node(const struct device_node *cpu_node,
 						 int index);
@@ -662,6 +664,12 @@ static inline int of_cpu_node_to_id(struct device_node *np)
 	return -ENODEV;
 }
 
+static inline int of_cpu_phandle_to_id(const struct device_node *np,
+				       struct device_node **cpu_np)
+{
+	return -ENODEV;
+}
+
 static inline struct device_node *of_get_next_cpu_node(struct device_node *prev)
 {
 	return NULL;
-- 
2.34.1

[PATCH v2 2/6] arch_topology: update CPU map to use the new API

[Alireza Sanaee]

Cleans up the cpu-map generation using the created API.

Signed-off-by: Alireza Sanaee <alireza.sanaee@huawei.com>
---
 drivers/base/arch_topology.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 3ebe77566788..533ddf69b43c 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -518,23 +518,23 @@ core_initcall(free_raw_capacity);
  */
 static int __init get_cpu_for_node(struct device_node *node)
 {
+	struct device_node *cpu_node = NULL;
 	int cpu;
-	struct device_node *cpu_node __free(device_node) =
-		of_parse_phandle(node, "cpu", 0);
 
-	if (!cpu_node)
-		return -1;
+	cpu = of_cpu_phandle_to_id(node, &cpu_node);
 
-	cpu = of_cpu_node_to_id(cpu_node);
 	if (cpu >= 0)
 		topology_parse_cpu_capacity(cpu_node, cpu);
-	else
+	else if (cpu == -ENODEV)
 		pr_info("CPU node for %pOF exist but the possible cpu range is :%*pbl\n",
 			cpu_node, cpumask_pr_args(cpu_possible_mask));
+	else
+		return -1;
 
 	return cpu;
 }
 
+
 static int __init parse_core(struct device_node *core, int package_id,
 			     int cluster_id, int core_id)
 {
-- 
2.34.1

[PATCH v2 3/6] coresight: cti: Use of_cpu_phandle_to_id for grabbing CPU id

[Alireza Sanaee]

Use the newly created API to grab CPU id.

Signed-off-by: Alireza Sanaee <alireza.sanaee@huawei.com>
---
 .../hwtracing/coresight/coresight-cti-platform.c  | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-cti-platform.c b/drivers/hwtracing/coresight/coresight-cti-platform.c
index d0ae10bf6128..369a9b7170e8 100644
--- a/drivers/hwtracing/coresight/coresight-cti-platform.c
+++ b/drivers/hwtracing/coresight/coresight-cti-platform.c
@@ -41,21 +41,12 @@
  */
 static int of_cti_get_cpu_at_node(const struct device_node *node)
 {
+	struct device_node *dn = NULL;
 	int cpu;
-	struct device_node *dn;
 
-	if (node == NULL)
-		return -1;
-
-	dn = of_parse_phandle(node, "cpu", 0);
-	/* CTI affinity defaults to no cpu */
-	if (!dn)
-		return -1;
-	cpu = of_cpu_node_to_id(dn);
+	cpu = of_cpu_phandle_to_id(node, &dn);
 	of_node_put(dn);
-
-	/* No Affinity  if no cpu nodes are found */
-	return (cpu < 0) ? -1 : cpu;
+	return cpu;
 }
 
 #else
-- 
2.34.1

[PATCH v2 4/6] coresight: Use of_cpu_phandle_to_id for grabbing CPU id

[Alireza Sanaee]

Use the newly created API to grab CPU id.

Signed-off-by: Alireza Sanaee <alireza.sanaee@huawei.com>
---
 drivers/hwtracing/coresight/coresight-platform.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
index 8192ba3279f0..be20caf965c9 100644
--- a/drivers/hwtracing/coresight/coresight-platform.c
+++ b/drivers/hwtracing/coresight/coresight-platform.c
@@ -167,19 +167,9 @@ of_coresight_get_output_ports_node(const struct device_node *node)
 
 static int of_coresight_get_cpu(struct device *dev)
 {
-	int cpu;
-	struct device_node *dn;
-
-	if (!dev->of_node)
-		return -ENODEV;
-
-	dn = of_parse_phandle(dev->of_node, "cpu", 0);
-	if (!dn)
-		return -ENODEV;
-
-	cpu = of_cpu_node_to_id(dn);
+	struct device_node *dn = NULL;
+	int cpu = of_cpu_phandle_to_id(dev->of_node, &dn);
 	of_node_put(dn);
-
 	return cpu;
 }
 
-- 
2.34.1

[PATCH v2 5/6] arm64: of: handle multiple threads in ARM cpu node

[Alireza Sanaee]

Update `of_parse_and_init_cpus` to parse reg property of CPU node as
an array based as per spec for SMT threads.

Spec v0.4 Section 3.8.1:
The value of reg is a <prop-encoded-**array**> that defines a unique
CPU/thread id for the CPU/threads represented by the CPU node.  **If a CPU
supports more than one thread (i.e.  multiple streams of execution) the
reg property is an array with 1 element per thread**.  The address-cells
on the /cpus node specifies how many cells each element of the array
takes. Software can determine the number of threads by dividing the size
of reg by the parent node's address-cells.

An accurate example of 1 core with 2 SMTs:

	cpus {
		#size-cells = <0x00>;
		#address-cells = <0x01>;

		cpu@0 {
			phandle = <0x8000>;
			**reg = <0x00 0x01>;**
			enable-method = "psci";
			compatible = "arm,cortex-a57";
			device_type = "cpu";
		};
	};

Instead of:

	cpus {
		#size-cells = <0x00>;
		#address-cells = <0x01>;

		cpu@0 {
			phandle = <0x8000>;
			reg = <0x00>;
			enable-method = "psci";
			compatible = "arm,cortex-a57";
			device_type = "cpu";
		};

		cpu@1 {
			phandle = <0x8001>;
			reg = <0x01>;
			enable-method = "psci";
			compatible = "arm,cortex-a57";
			device_type = "cpu";
		};
	};

which is **NOT** accurate.

Signed-off-by: Alireza Sanaee <alireza.sanaee@huawei.com>
---
 arch/arm64/kernel/smp.c | 74 +++++++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 33 deletions(-)

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 3b3f6b56e733..8dd3b3c82967 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -689,53 +689,61 @@ static void __init acpi_parse_and_init_cpus(void)
 static void __init of_parse_and_init_cpus(void)
 {
 	struct device_node *dn;
+	u64 hwid;
+	u32 tid;
 
 	for_each_of_cpu_node(dn) {
-		u64 hwid = of_get_cpu_hwid(dn, 0);
+		tid = 0;
 
-		if (hwid & ~MPIDR_HWID_BITMASK)
-			goto next;
+		while (1) {
+			hwid = of_get_cpu_hwid(dn, tid++);
+			if (hwid == ~0ULL)
+				break;
 
-		if (is_mpidr_duplicate(cpu_count, hwid)) {
-			pr_err("%pOF: duplicate cpu reg properties in the DT\n",
-				dn);
-			goto next;
-		}
+			if (hwid & ~MPIDR_HWID_BITMASK)
+				goto next;
 
-		/*
-		 * The numbering scheme requires that the boot CPU
-		 * must be assigned logical id 0. Record it so that
-		 * the logical map built from DT is validated and can
-		 * be used.
-		 */
-		if (hwid == cpu_logical_map(0)) {
-			if (bootcpu_valid) {
-				pr_err("%pOF: duplicate boot cpu reg property in DT\n",
-					dn);
+			if (is_mpidr_duplicate(cpu_count, hwid)) {
+				pr_err("%pOF: duplicate cpu reg properties in the DT\n",
+				       dn);
 				goto next;
 			}
 
-			bootcpu_valid = true;
-			early_map_cpu_to_node(0, of_node_to_nid(dn));
-
 			/*
-			 * cpu_logical_map has already been
-			 * initialized and the boot cpu doesn't need
-			 * the enable-method so continue without
-			 * incrementing cpu.
+			 * The numbering scheme requires that the boot CPU
+			 * must be assigned logical id 0. Record it so that
+			 * the logical map built from DT is validated and can
+			 * be used.
 			 */
-			continue;
-		}
+			if (hwid == cpu_logical_map(0)) {
+				if (bootcpu_valid) {
+					pr_err("%pOF: duplicate boot cpu reg property in DT\n",
+					       dn);
+					goto next;
+				}
+
+				bootcpu_valid = true;
+				early_map_cpu_to_node(0, of_node_to_nid(dn));
+
+				/*
+				 * cpu_logical_map has already been
+				 * initialized and the boot cpu doesn't need
+				 * the enable-method so continue without
+				 * incrementing cpu.
+				 */
+				continue;
+			}
 
-		if (cpu_count >= NR_CPUS)
-			goto next;
+			if (cpu_count >= NR_CPUS)
+				goto next;
 
-		pr_debug("cpu logical map 0x%llx\n", hwid);
-		set_cpu_logical_map(cpu_count, hwid);
+			pr_debug("cpu logical map 0x%llx\n", hwid);
+			set_cpu_logical_map(cpu_count, hwid);
 
-		early_map_cpu_to_node(cpu_count, of_node_to_nid(dn));
+			early_map_cpu_to_node(cpu_count, of_node_to_nid(dn));
 next:
-		cpu_count++;
+			cpu_count++;
+		}
 	}
 }
 
-- 
2.34.1

[PATCH v2 6/6] of: of_cpu_phandle_to_id to support SMT threads

[Alireza Sanaee]

Enhance the API to support SMT threads, this will allow sharing resources among
multiple SMT threads.

Enabled the sharing of resources, such as L1 Cache and clocks, between SMT
threads. It introduces a fix that uses thread IDs to match each CPU thread in
the register array within the cpu-node. This ensures that the cpu-map or any
driver relying on this API is fine even when SMT threads share resources.

Additionally, I have tested this for CPU based on the discussions in [1], I
adopted the new cpu-map layout, where the first parameter is a phandle and the
second is the local thread index, as shown below:

In the CPU map, there are two cases that only one occurs at at time.
    1) "cpu" = <phandle>
    2) "cpus" = <phandle> <index>

The first case addresses non-SMTs and the second case addresses SMTs
that the variable must be cpu(s) with an index where we later look up
the reg array with that.

    core0 {
      thread0 {
        cpus = <&cpu0 0>;
      };
      thread1 {
        cpus = <&cpu0 1>;
      };
    };

[1] https://lore.kernel.org/devicetree-spec/CAL_JsqK1yqRLD9B+G7UUp=D8K++mXHq0Rmv=1i6DL_jXyZwXAw@mail.gmail.com/

Signed-off-by: Alireza Sanaee <alireza.sanaee@huawei.com>
---
 drivers/of/cpu.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/of/cpu.c b/drivers/of/cpu.c
index 1e8d1fa04d3c..fbd3f6d4a87f 100644
--- a/drivers/of/cpu.c
+++ b/drivers/of/cpu.c
@@ -186,14 +186,41 @@ EXPORT_SYMBOL(of_cpu_node_to_id);
 int of_cpu_phandle_to_id(const struct device_node *node,
 			 struct device_node **cpu_np)
 {
+	int cpu, ret;
+	bool found = false;
+	uint32_t local_thread, thread_index;
+	struct device_node *np;
+	struct of_phandle_args args;
+
 	if (!node)
 		return -1;
 
+	/*
+	 * Two cases which only one occurs at a time:
+	 * 1) cpu = <phandle>
+	 * 2) cpus = <phandle> <thread_index>
+	 */
 	*cpu_np = of_parse_phandle(node, "cpu", 0);
-	if (!cpu_np)
-		return -ENODEV;
+	if (!*cpu_np) {
+		ret = of_parse_phandle_with_fixed_args(node, "cpus", 1, 0,
+						       &args);
+		if (ret < 0)
+			return ret;
+
+		*cpu_np = args.np;
+		thread_index = args.args[0];
+		for_each_possible_cpu(cpu) {
+			np = of_get_cpu_node(cpu, &local_thread);
+			found = (*cpu_np == np) && (local_thread == thread_index);
+			of_node_put(np);
+			if (found)
+				return cpu;
+		}
 
-	return of_cpu_node_to_id(*cpu_np);
+		return -ENODEV;
+	} else {
+		return of_cpu_node_to_id(*cpu_np);
+	}
 }
 EXPORT_SYMBOL(of_cpu_phandle_to_id);
 
-- 
2.34.1

Re: [PATCH v2 1/6] of: add infra for finding CPU id from phandle

[Krzysztof Kozlowski]

On Fri, May 02, 2025 at 05:12:55PM GMT, Alireza Sanaee wrote:
> Get CPU id from phandle. Many drivers get do this by getting hold of CPU
> node first through a phandle and then find the CPU ID using the relevant
> function. This commit encapsulates cpu node finding and improves
> readability.
> 
> The API interface requires two parameters, 1) node, 2) pointer to CPU
> node. API sets the pointer to the CPU node and allows the driver to play
> with the CPU itself, for logging purposes for instance.

Run checkpatch - missing SoB.

> ---
>  drivers/of/cpu.c   | 24 ++++++++++++++++++++++++
>  include/linux/of.h |  8 ++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/of/cpu.c b/drivers/of/cpu.c
> index 5214dc3d05ae..1e8d1fa04d3c 100644
> --- a/drivers/of/cpu.c
> +++ b/drivers/of/cpu.c
> @@ -173,6 +173,30 @@ int of_cpu_node_to_id(struct device_node *cpu_node)
>  }
>  EXPORT_SYMBOL(of_cpu_node_to_id);
>  
> +/**
> + * of_cpu_phandle_to_id: Get the logical CPU number for a given device_node
> + *
> + * @node: Pointer to the device_node containing CPU phandle.
> + * @cpu_np: Pointer to the device_node for CPU.
> + *
> + * Return: The logical CPU number of the given CPU device_node or -ENODEV if
> + * the CPU is not found. If the property is not found, it returns -1. On
> + * success, cpu_np will always point to the retrieved CPU device_node.
> + */
> +int of_cpu_phandle_to_id(const struct device_node *node,
> +			 struct device_node **cpu_np)
> +{
> +	if (!node)
> +		return -1;
> +
> +	*cpu_np = of_parse_phandle(node, "cpu", 0);
> +	if (!cpu_np)
> +		return -ENODEV;
> +
> +	return of_cpu_node_to_id(*cpu_np);

You leak the node... or intention was to return it to the caller, but
then you need to document that caller must drop the ref.

Best regards,
Krzysztof

Re: [PATCH v2 6/6] of: of_cpu_phandle_to_id to support SMT threads

[Krzysztof Kozlowski]

On Fri, May 02, 2025 at 05:13:00PM GMT, Alireza Sanaee wrote:
> Enhance the API to support SMT threads, this will allow sharing resources among
> multiple SMT threads.

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC (and consider --no-git-fallback argument, so you will
not CC people just because they made one commit years ago). It might
happen, that command when run on an older kernel, gives you outdated
entries. Therefore please be sure you base your patches on recent Linux
kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about 'b4 prep --auto-to-cc' if you added new
patches to the patchset.
</form letter>	

> 
> Enabled the sharing of resources, such as L1 Cache and clocks, between SMT
> threads. It introduces a fix that uses thread IDs to match each CPU thread in
> the register array within the cpu-node. This ensures that the cpu-map or any
> driver relying on this API is fine even when SMT threads share resources.
> 
> Additionally, I have tested this for CPU based on the discussions in [1], I
> adopted the new cpu-map layout, where the first parameter is a phandle and the
> second is the local thread index, as shown below:
> 
> In the CPU map, there are two cases that only one occurs at at time.
>     1) "cpu" = <phandle>
>     2) "cpus" = <phandle> <index>
> 
> The first case addresses non-SMTs and the second case addresses SMTs
> that the variable must be cpu(s) with an index where we later look up
> the reg array with that.
> 
>     core0 {
>       thread0 {
>         cpus = <&cpu0 0>;

Not so sure, dtschema says only one item is allowed in the phandle and I
do not see here binding change.

Although this wasn't even sent to me, so I'll just ignore your patchset.

Best regards,
Krzysztof

Re: [PATCH v2 6/6] of: of_cpu_phandle_to_id to support SMT threads

[Krzysztof Kozlowski]

On 04/05/2025 19:51, Krzysztof Kozlowski wrote:
>> In the CPU map, there are two cases that only one occurs at at time.
>>     1) "cpu" = <phandle>
>>     2) "cpus" = <phandle> <index>
>>
>> The first case addresses non-SMTs and the second case addresses SMTs
>> that the variable must be cpu(s) with an index where we later look up
>> the reg array with that.
>>
>>     core0 {
>>       thread0 {
>>         cpus = <&cpu0 0>;
> 
> Not so sure, dtschema says only one item is allowed in the phandle and I
> do not see here binding change.
> 
> Although this wasn't even sent to me, so I'll just ignore your patchset.

Ah, there was no binding in the patchset, so that's why I did not get
it. Makes sense now, but question about missing binding change stays.

Best regards,
Krzysztof

Re: [PATCH v2 6/6] of: of_cpu_phandle_to_id to support SMT threads

[Alireza Sanaee]

On Sun, 4 May 2025 19:52:34 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 04/05/2025 19:51, Krzysztof Kozlowski wrote:
> >> In the CPU map, there are two cases that only one occurs at at
> >> time. 1) "cpu" = <phandle>
> >>     2) "cpus" = <phandle> <index>
> >>
> >> The first case addresses non-SMTs and the second case addresses
> >> SMTs that the variable must be cpu(s) with an index where we later
> >> look up the reg array with that.
> >>
> >>     core0 {
> >>       thread0 {
> >>         cpus = <&cpu0 0>;  
> > 
> > Not so sure, dtschema says only one item is allowed in the phandle
> > and I do not see here binding change.
> > 
> > Although this wasn't even sent to me, so I'll just ignore your
> > patchset.  
> 
> Ah, there was no binding in the patchset, so that's why I did not get
> it. Makes sense now, but question about missing binding change stays.
> 
> Best regards,
> Krzysztof
> 

Hi Krzysztof,

There are some existing bindings in which this pattern has been
used, so I don't think I am changing binding really.

https://www.kernel.org/doc/Documentation/devicetree/bindings/thermal/thermal-zones.yaml#:~:text=cooling%2Ddevice%20%3D%20%3C%26CPU0%203%203%3E%2C%20%3C%26CPU1%203%203%3E%2C

Would that be good, if I just include the link in the next version?

Thanks,
Alireza

Re: [PATCH v2 6/6] of: of_cpu_phandle_to_id to support SMT threads

[Alireza Sanaee]

On Sun, 4 May 2025 19:51:02 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On Fri, May 02, 2025 at 05:13:00PM GMT, Alireza Sanaee wrote:
> > Enhance the API to support SMT threads, this will allow sharing
> > resources among multiple SMT threads.  
> 
> <form letter>
> Please use scripts/get_maintainers.pl to get a list of necessary
> people and lists to CC (and consider --no-git-fallback argument, so
> you will not CC people just because they made one commit years ago).
> It might happen, that command when run on an older kernel, gives you
> outdated entries. Therefore please be sure you base your patches on
> recent Linux kernel.
> 
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on
> some ancient tree (don't, instead use mainline) or work on fork of
> kernel (don't, instead use mainline). Just use b4 and everything
> should be fine, although remember about 'b4 prep --auto-to-cc' if you
> added new patches to the patchset.
> </form letter>	
> 
> > 
> > Enabled the sharing of resources, such as L1 Cache and clocks,
> > between SMT threads. It introduces a fix that uses thread IDs to
> > match each CPU thread in the register array within the cpu-node.
> > This ensures that the cpu-map or any driver relying on this API is
> > fine even when SMT threads share resources.
> > 
> > Additionally, I have tested this for CPU based on the discussions
> > in [1], I adopted the new cpu-map layout, where the first parameter
> > is a phandle and the second is the local thread index, as shown
> > below:
> > 
> > In the CPU map, there are two cases that only one occurs at at time.
> >     1) "cpu" = <phandle>
> >     2) "cpus" = <phandle> <index>
> > 
> > The first case addresses non-SMTs and the second case addresses SMTs
> > that the variable must be cpu(s) with an index where we later look
> > up the reg array with that.
> > 
> >     core0 {
> >       thread0 {
> >         cpus = <&cpu0 0>;  
> 
> Not so sure, dtschema says only one item is allowed in the phandle
> and I do not see here binding change.
> 
> Although this wasn't even sent to me, so I'll just ignore your
> patchset.
> 
> Best regards,
> Krzysztof
> 

Sorry, I didn't use the script, I will next time.

Re: [PATCH v2 1/6] of: add infra for finding CPU id from phandle

[Alireza Sanaee]

On Sun, 4 May 2025 19:48:11 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On Fri, May 02, 2025 at 05:12:55PM GMT, Alireza Sanaee wrote:
> > Get CPU id from phandle. Many drivers get do this by getting hold
> > of CPU node first through a phandle and then find the CPU ID using
> > the relevant function. This commit encapsulates cpu node finding
> > and improves readability.
> > 
> > The API interface requires two parameters, 1) node, 2) pointer to
> > CPU node. API sets the pointer to the CPU node and allows the
> > driver to play with the CPU itself, for logging purposes for
> > instance.  
> 
> Run checkpatch - missing SoB.
Shoot! I guess I know what happened! Will use the script.
> 
> > ---
> >  drivers/of/cpu.c   | 24 ++++++++++++++++++++++++
> >  include/linux/of.h |  8 ++++++++
> >  2 files changed, 32 insertions(+)
> > 
> > diff --git a/drivers/of/cpu.c b/drivers/of/cpu.c
> > index 5214dc3d05ae..1e8d1fa04d3c 100644
> > --- a/drivers/of/cpu.c
> > +++ b/drivers/of/cpu.c
> > @@ -173,6 +173,30 @@ int of_cpu_node_to_id(struct device_node
> > *cpu_node) }
> >  EXPORT_SYMBOL(of_cpu_node_to_id);
> >  
> > +/**
> > + * of_cpu_phandle_to_id: Get the logical CPU number for a given
> > device_node
> > + *
> > + * @node: Pointer to the device_node containing CPU phandle.
> > + * @cpu_np: Pointer to the device_node for CPU.
> > + *
> > + * Return: The logical CPU number of the given CPU device_node or
> > -ENODEV if
> > + * the CPU is not found. If the property is not found, it returns
> > -1. On
> > + * success, cpu_np will always point to the retrieved CPU
> > device_node.
> > + */
> > +int of_cpu_phandle_to_id(const struct device_node *node,
> > +			 struct device_node **cpu_np)
> > +{
> > +	if (!node)
> > +		return -1;
> > +
> > +	*cpu_np = of_parse_phandle(node, "cpu", 0);
> > +	if (!cpu_np)
> > +		return -ENODEV;
> > +
> > +	return of_cpu_node_to_id(*cpu_np);  
> 
> You leak the node... or intention was to return it to the caller, but
> then you need to document that caller must drop the ref.
That's fair, I include documentation for that.
> 
> Best regards,
> Krzysztof
> 
> 

Re: [PATCH v2 6/6] of: of_cpu_phandle_to_id to support SMT threads

[Krzysztof Kozlowski]

On 06/05/2025 12:23, Alireza Sanaee wrote:
> On Sun, 4 May 2025 19:52:34 +0200
> Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 
>> On 04/05/2025 19:51, Krzysztof Kozlowski wrote:
>>>> In the CPU map, there are two cases that only one occurs at at
>>>> time. 1) "cpu" = <phandle>
>>>>     2) "cpus" = <phandle> <index>
>>>>
>>>> The first case addresses non-SMTs and the second case addresses
>>>> SMTs that the variable must be cpu(s) with an index where we later
>>>> look up the reg array with that.
>>>>
>>>>     core0 {
>>>>       thread0 {
>>>>         cpus = <&cpu0 0>;  
>>>
>>> Not so sure, dtschema says only one item is allowed in the phandle
>>> and I do not see here binding change.
>>>
>>> Although this wasn't even sent to me, so I'll just ignore your
>>> patchset.  
>>
>> Ah, there was no binding in the patchset, so that's why I did not get
>> it. Makes sense now, but question about missing binding change stays.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Hi Krzysztof,
> 
> There are some existing bindings in which this pattern has been
> used, so I don't think I am changing binding really.
> 
> https://www.kernel.org/doc/Documentation/devicetree/bindings/thermal/thermal-zones.yaml#:~:text=cooling%2Ddevice%20%3D%20%3C%26CPU0%203%203%3E%2C%20%3C%26CPU1%203%203%3E%2C
I do not understand this - it is not cpus phandle. Please respond to
specific comment: how many arguments are allowed by dtschema for cpus?

Best regards,
Krzysztof

Re: [PATCH v2 6/6] of: of_cpu_phandle_to_id to support SMT threads

[Alireza Sanaee]

On Tue, 6 May 2025 12:56:43 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 06/05/2025 12:23, Alireza Sanaee wrote:
> > On Sun, 4 May 2025 19:52:34 +0200
> > Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >   
> >> On 04/05/2025 19:51, Krzysztof Kozlowski wrote:  
> >>>> In the CPU map, there are two cases that only one occurs at at
> >>>> time. 1) "cpu" = <phandle>
> >>>>     2) "cpus" = <phandle> <index>
> >>>>
> >>>> The first case addresses non-SMTs and the second case addresses
> >>>> SMTs that the variable must be cpu(s) with an index where we
> >>>> later look up the reg array with that.
> >>>>
> >>>>     core0 {
> >>>>       thread0 {
> >>>>         cpus = <&cpu0 0>;    
> >>>
> >>> Not so sure, dtschema says only one item is allowed in the phandle
> >>> and I do not see here binding change.
> >>>
> >>> Although this wasn't even sent to me, so I'll just ignore your
> >>> patchset.    
> >>
> >> Ah, there was no binding in the patchset, so that's why I did not
> >> get it. Makes sense now, but question about missing binding change
> >> stays.
> >>
> >> Best regards,
> >> Krzysztof
> >>  
> > 
> > Hi Krzysztof,
> > 
> > There are some existing bindings in which this pattern has been
> > used, so I don't think I am changing binding really.
> > 
> > https://www.kernel.org/doc/Documentation/devicetree/bindings/thermal/thermal-zones.yaml#:~:text=cooling%2Ddevice%20%3D%20%3C%26CPU0%203%203%3E%2C%20%3C%26CPU1%203%203%3E%2C  
> I do not understand this - it is not cpus phandle. Please respond to
> specific comment: how many arguments are allowed by dtschema for cpus?

Hi Krzysztof,

If you mean checking
here? https://github.com/devicetree-org/dt-schema/blob/e6ea659d2baa30df1ec0fcc4f8354208692489eb/dtschema/schemas/cpu-map.yaml#L110

There is no parameters allowed at this point for cpu phandles in the
cpu-map tree. Of course, this is different than what's been
implemented in the patchset.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 6/6] of: of_cpu_phandle_to_id to support SMT threads

[Krzysztof Kozlowski]

On 06/05/2025 15:31, Alireza Sanaee wrote:
>>>>  
>>>
>>> Hi Krzysztof,
>>>
>>> There are some existing bindings in which this pattern has been
>>> used, so I don't think I am changing binding really.
>>>
>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/thermal/thermal-zones.yaml#:~:text=cooling%2Ddevice%20%3D%20%3C%26CPU0%203%203%3E%2C%20%3C%26CPU1%203%203%3E%2C  
>> I do not understand this - it is not cpus phandle. Please respond to
>> specific comment: how many arguments are allowed by dtschema for cpus?
> 
> Hi Krzysztof,
> 
> If you mean checking
> here? https://github.com/devicetree-org/dt-schema/blob/e6ea659d2baa30df1ec0fcc4f8354208692489eb/dtschema/schemas/cpu-map.yaml#L110
> 
> There is no parameters allowed at this point for cpu phandles in the
> cpu-map tree. Of course, this is different than what's been
> implemented in the patchset.
Hm, ok, I thought you are adding this for cpu-map, but if not, then
where are the bindings for this ABI?

BTW, share your DTS so we can be sure that it is properly validated
against bindings.

Best regards,
Krzysztof

Re: [PATCH v2 6/6] of: of_cpu_phandle_to_id to support SMT threads

[Alireza Sanaee]

On Tue, 6 May 2025 15:36:05 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 06/05/2025 15:31, Alireza Sanaee wrote:
> >>>>    
> >>>
> >>> Hi Krzysztof,
> >>>
> >>> There are some existing bindings in which this pattern has been
> >>> used, so I don't think I am changing binding really.
> >>>
> >>> https://www.kernel.org/doc/Documentation/devicetree/bindings/thermal/thermal-zones.yaml#:~:text=cooling%2Ddevice%20%3D%20%3C%26CPU0%203%203%3E%2C%20%3C%26CPU1%203%203%3E%2C    
> >> I do not understand this - it is not cpus phandle. Please respond
> >> to specific comment: how many arguments are allowed by dtschema
> >> for cpus?  
> > 
> > Hi Krzysztof,
> > 
> > If you mean checking
> > here? https://github.com/devicetree-org/dt-schema/blob/e6ea659d2baa30df1ec0fcc4f8354208692489eb/dtschema/schemas/cpu-map.yaml#L110
> > 
> > There is no parameters allowed at this point for cpu phandles in the
> > cpu-map tree. Of course, this is different than what's been
> > implemented in the patchset.  
> Hm, ok, I thought you are adding this for cpu-map, but if not, then
> where are the bindings for this ABI?
> 
> BTW, share your DTS so we can be sure that it is properly validated
> against bindings.

No wait, I am adding this to cpu-map indeed, I meant the code is
different from what's available in the dt-schema, meaning that there is
a mismatch like what you pointed.

Based on your comments, my conjecture is that I will need to include dt
binding anyways.

SMT threads should be represented in the reg array of CPU nodes, and
will need to be addressed via an extra parameter specifying an index in
the reg array. 

There are various places in the kernel where we point to
CPU node using those phandles. Now the first place that we are trying to change is cpu-map for
enabling SMT resource sharing, and that probably means I should update
the binding related to that.

Hope that clarifies.

Thanks,
Alireza

> 
> Best regards,
> Krzysztof