Skip to contentSkip to conversation

Add single core R5F IPC for AM62 SoC family

PATCH Superseded Devarsh Thakkar 4 participants 11 messages Mar 10, 2023 – Mar 29, 2023

Subsystems (2)
Device tree bindings Remote processor (remoteproc)

v7 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Devarsh Thakkar

author
#0 lore raw

v7 cover Add single core R5F IPC for AM62 SoC family

Devarsh Thakkar (3):
remoteproc: k3-r5: Simplify cluster mode setting usage
dt-bindings: remoteproc: ti: Add new compatible for AM62 SoC family
remoteproc: k3-r5: Use separate compatible string for TI AM62x SoC
family

 .../bindings/remoteproc/ti,k3-r5f-rproc.yaml  |  76 ++++++++----
 drivers/remoteproc/ti_k3_r5_remoteproc.c      | 110 ++++++++++++------
 2 files changed, 130 insertions(+), 56 deletions(-)

---
V2:
- dt-bindings: Avoid acronyms, use "Device Manager" instead of "DM"
V3:
- dt-bindings: Use separate if block for each compatible for ti,cluster-mode property
- dt-bindings: Rearrange compatibles as per alphabatical order
V4:
- dt-bindings: Place each enum in separate line in allOf
V5:
- No change (fixing typo in email address)
V6:
- dt-bindings: Remove reviewed-by due to new modifications to use cluster-mode=3 Introduce Simplify cluster-mode setting preamble patch per review comments
- Use CLUSTER_MODE_SINGLECORE for AM62x
- Set PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE for single core.
V7:
- Override to appropriate cluster-mode per firmware status flag without checking soc_data
- Set appropriate mode as default if not provided in DT
- Check mode validity against SoC data during probe
- Rebase on top of 6.3 linux-next

Signature · 1 line
2.34.1
author
#1 lore raw

PATCH v7 1/3 remoteproc: k3-r5: Simplify cluster mode setting usage

Check the validity of mode against SoC supported modes right at the probe to minimize the usage of same check further in the code.

Set default value of cluster-mode only if cluster-mode device tree property is empty.

In case devicetree provided cluster-mode property is invalid For e.g. using CLUSTER_MODE_SINGLECPU on any SoC other than am64x then return error.

If firmware has set the PROC_BOOT_STATUS_FLAG_R5_SINGLECORE_ONLY flag then what it means is that only CLUSTER_MODE_SINGLECPU is possible to use [1] and hence there is no need to check for soc_data->single_cpu_mode first and then checking cluster mode.

PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE flag can be set directly for CLUSTER_MODE_SINGLECPU without checking for soc_data->single_cpu_mode since that check has already been done during probe.

Link:
[1] https://software-dl.ti.com/tisci/esd/latest/2_tisci_msgs/security/PROC_BOOT.html?highlight=singlecore_only#arm-r5

Signed-off-by Devarsh Thakkar <[email protected]>

---
V1->V6: No changelog (Patch introduced in V6)
V6->V7:
- Override to appropriate cluster mode per firmware status flag directly without checking for soc_data
- Set appropriate mode as default if not provided in DT
- Check mode validity against SoC data during probe
- Rebase on top of 6.3 linux-next

---
 drivers/remoteproc/ti_k3_r5_remoteproc.c | 57 +++++++++++++-----------
 1 file changed, 30 insertions(+), 27 deletions(-)
drivers/remoteproc/ti_k3_r5_remoteproc.c +30 -27
index 0481926c6975..c2ec0f432921 100644
@@ -852,38 +852,33 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
 	dev_dbg(dev, "boot_vector = 0x%llx, cfg = 0x%x ctrl = 0x%x stat = 0x%x\n",
 		boot_vec, cfg, ctrl, stat);
 
-	/* check if only Single-CPU mode is supported on applicable SoCs */
-	if (cluster->soc_data->single_cpu_mode) {
-		single_cpu =
-			!!(stat & PROC_BOOT_STATUS_FLAG_R5_SINGLECORE_ONLY);
-		if (single_cpu && cluster->mode == CLUSTER_MODE_SPLIT) {
-			dev_err(cluster->dev, "split-mode not permitted, force configuring for single-cpu mode\n");
-			cluster->mode = CLUSTER_MODE_SINGLECPU;
-		}
-		goto config;
+	single_cpu = !!(stat & PROC_BOOT_STATUS_FLAG_R5_SINGLECORE_ONLY);
+	lockstep_en = !!(stat & PROC_BOOT_STATUS_FLAG_R5_LOCKSTEP_PERMITTED);
+
+	/* Override to single CPU mode if set in status flag */
+	if (single_cpu && cluster->mode == CLUSTER_MODE_SPLIT) {
+		dev_err(cluster->dev, "split-mode not permitted, force configuring for single-cpu mode\n");
+		cluster->mode = CLUSTER_MODE_SINGLECPU;
 	}
 
-	/* check conventional LockStep vs Split mode configuration */
-	lockstep_en = !!(stat & PROC_BOOT_STATUS_FLAG_R5_LOCKSTEP_PERMITTED);
+	/* Override to split mode if lockstep enable bit is not set in status flag */
 	if (!lockstep_en && cluster->mode == CLUSTER_MODE_LOCKSTEP) {
 		dev_err(cluster->dev, "lockstep mode not permitted, force configuring for split-mode\n");
 		cluster->mode = CLUSTER_MODE_SPLIT;
 	}
 
-config:
 	/* always enable ARM mode and set boot vector to 0 */
 	boot_vec = 0x0;
 	if (core == core0) {
 		clr_cfg = PROC_BOOT_CFG_FLAG_R5_TEINIT;
-		if (cluster->soc_data->single_cpu_mode) {
-			/*
-			 * Single-CPU configuration bit can only be configured
-			 * on Core0 and system firmware will NACK any requests
-			 * with the bit configured, so program it only on
-			 * permitted cores
-			 */
-			if (cluster->mode == CLUSTER_MODE_SINGLECPU)
-				set_cfg = PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE;
+		/*
+		 * Single-CPU configuration bit can only be configured
+		 * on Core0 and system firmware will NACK any requests
+		 * with the bit configured, so program it only on
+		 * permitted cores
+		 */
+		if (cluster->mode == CLUSTER_MODE_SINGLECPU) {
+			set_cfg = PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE;
 		} else {
 			/*
 			 * LockStep configuration bit is Read-only on Split-mode

@@ -1700,12 +1695,6 @@ static int k3_r5_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	cluster->dev = dev;
-	/*
-	 * default to most common efuse configurations - Split-mode on AM64x
-	 * and LockStep-mode on all others
-	 */
-	cluster->mode = data->single_cpu_mode ?
-				CLUSTER_MODE_SPLIT : CLUSTER_MODE_LOCKSTEP;
 	cluster->soc_data = data;
 	INIT_LIST_HEAD(&cluster->cores);
 

@@ -1716,6 +1705,20 @@ static int k3_r5_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	if (ret == -EINVAL) {
+		/*
+		 * default to most common efuse configurations - Split-mode on AM64x
+		 * and LockStep-mode on all others
+		 */
+		cluster->mode = data->single_cpu_mode ?
+					CLUSTER_MODE_SPLIT : CLUSTER_MODE_LOCKSTEP;
+	}
+
+	if (cluster->mode == CLUSTER_MODE_SINGLECPU && !data->single_cpu_mode) {
+		dev_err(dev, "Cluster mode = %d is not supported on this SoC\n", cluster->mode);
+		return -EINVAL;
+	}
+
 	num_cores = of_get_available_child_count(np);
 	if (num_cores != 2) {
 		dev_err(dev, "MCU cluster requires both R5F cores to be enabled, num_cores = %d\n",
Signature · 1 line
2.34.1
author
#2 lore raw

PATCH v7 2/3 dt-bindings: remoteproc: ti: Add new compatible for AM62 SoC family

In reply to Devarsh Thakkar

AM62 family of devices don't have a R5F cluster, instead they have single core DM R5F. Add new compatible string ti,am62-r5fss to support this scenario.

When this new compatible is used cluster-mode property can only be set to value 3 i.e. CLUSTER_MODE_SINGLECORE which is also the default value in case cluster-mode is not defined in device-tree.

While at it, also sort the compatible lists in alphabetical order.

Signed-off-by Devarsh Thakkar <[email protected]>

---
V2: Avoid acronyms, use "Device Manager" instead of "DM" V3:
- Use separate if block for each compatible for ti,cluster-mode property
- Rearrange compatibles as per alphabatical order
V4: Place each enum in separate line in allOf
V5: No change (fixing typo in email address)
V6: Remove reviewed-by due to new modifications to Use cluster-mode=3 for am62x
V7: No change

---
 .../bindings/remoteproc/ti,k3-r5f-rproc.yaml  | 76 ++++++++++++++-----
 1 file changed, 55 insertions(+), 21 deletions(-)
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +55 -21
index fb9605f0655b..fcc3db97fe8f 100644
@@ -21,6 +21,9 @@ description: |
   called "Single-CPU" mode, where only Core0 is used, but with ability to use
   Core1's TCMs as well.
 
+  AM62 SoC family support a single R5F core only which runs Device Manager
+  firmware and can also be used as a remote processor with IPC communication.
+
   Each Dual-Core R5F sub-system is represented as a single DTS node
   representing the cluster, with a pair of child DT nodes representing
   the individual R5F cores. Each node has a number of required or optional

@@ -34,10 +37,11 @@ properties:
 
   compatible:
     enum:
+      - ti,am62-r5fss
+      - ti,am64-r5fss
       - ti,am654-r5fss
-      - ti,j721e-r5fss
       - ti,j7200-r5fss
-      - ti,am64-r5fss
+      - ti,j721e-r5fss
       - ti,j721s2-r5fss
 
   power-domains:

@@ -64,10 +68,17 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: |
       Configuration Mode for the Dual R5F cores within the R5F cluster.
-      Should be either a value of 1 (LockStep mode) or 0 (Split mode) on
+      For most SoCs (AM65x, J721E, J7200, J721s2),
+      It should be either a value of 1 (LockStep mode) or 0 (Split mode) on
       most SoCs (AM65x, J721E, J7200, J721s2), default is LockStep mode if
-      omitted; and should be either a value of 0 (Split mode) or 2
-      (Single-CPU mode) on AM64x SoCs, default is Split mode if omitted.
+      omitted.
+      For AM64x SoCs,
+      It  should be either a value of 0 (Split mode) or 2 (Single-CPU mode) and
+      default is Split mode if omitted.
+      For AM62x SoCs,
+      It should be set as 3 (Single-Core mode) which is also the default if
+      omitted.
+
 
 # R5F Processor Child Nodes:
 # ==========================

@@ -80,7 +91,9 @@ patternProperties:
       node representing a TI instantiation of the Arm Cortex R5F core. There
       are some specific integration differences for the IP like the usage of
       a Region Address Translator (RAT) for translating the larger SoC bus
-      addresses into a 32-bit address space for the processor.
+      addresses into a 32-bit address space for the processor. For AM62x,
+      the R5F Sub-System device node should only define one R5F child node
+      as it has only one core available.
 
       Each R5F core has an associated 64 KB of Tightly-Coupled Memory (TCM)
       internal memories split between two banks - TCMA and TCMB (further

@@ -100,10 +113,11 @@ patternProperties:
     properties:
       compatible:
         enum:
+          - ti,am62-r5f
+          - ti,am64-r5f
           - ti,am654-r5f
-          - ti,j721e-r5f
           - ti,j7200-r5f
-          - ti,am64-r5f
+          - ti,j721e-r5f
           - ti,j721s2-r5f
 
       reg:

@@ -208,19 +222,39 @@ patternProperties:
 
     unevaluatedProperties: false
 
-if:
-  properties:
-    compatible:
-      enum:
-        - ti,am64-r5fss
-then:
-  properties:
-    ti,cluster-mode:
-      enum: [0, 2]
-else:
-  properties:
-    ti,cluster-mode:
-      enum: [0, 1]
+allOf:
+  - if:
+      properties:
+        compatible:
+          enum:
+            - ti,am64-r5fss
+    then:
+      properties:
+        ti,cluster-mode:
+          enum: [0, 2]
+
+  - if:
+      properties:
+        compatible:
+          enum:
+            - ti,am654-r5fss
+            - ti,j7200-r5fss
+            - ti,j721e-r5fss
+            - ti,j721s2-r5fss
+    then:
+      properties:
+        ti,cluster-mode:
+          enum: [0, 1]
+
+  - if:
+      properties:
+        compatible:
+          enum:
+            - ti,am62-r5fss
+    then:
+      properties:
+        ti,cluster-mode:
+          enum: [3]
 
 required:
   - compatible
Signature · 1 line
2.34.1
Reviewed-by on v7 2/3
#3 lore raw

Re: [PATCH v7 2/3] dt-bindings: remoteproc: ti: Add new compatible for AM62 SoC family

Quoted 4 lines · Devarsh Thakkar

AM62 family of devices don't have a R5F cluster, instead they have single core DM R5F. Add new compatible string ti,am62-r5fss to support this scenario.

Quoted 8 lines

When this new compatible is used cluster-mode property can only be set to value 3 i.e. CLUSTER_MODE_SINGLECORE which is also the default value in case cluster-mode is not defined in device-tree.

While at it, also sort the compatible lists in alphabetical order.

Signed-off-by Devarsh Thakkar <[email protected]>

---

Reviewed-by Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

author
#4 lore raw

PATCH v7 3/3 remoteproc: k3-r5: Use separate compatible string for TI AM62x SoC family

In reply to Devarsh Thakkar

AM62 and AM62A SoCs use single core R5F which is a new scenario different than the one being used with CLUSTER_MODE_SINGLECPU which is for utilizing a single core from a set of cores available in R5F cluster present in the SoC.

To support this single core scenario map it with newly defined CLUSTER_MODE_SINGLECORE and use it when compatible is set to ti,am62-r5fss.

Also set PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE config for CLUSTER_MODE_SINGLECORE too as it is required by R5 core when it is being as general purpose core instead of device manager.

Signed-off-by Devarsh Thakkar <[email protected]>

---
V2:
- Fix indentation and ordering issues as per review comments
V3:
- Change CLUSTER_MODE_NONE value to -1
V4:
- No change
V5:
- No change (fixing typo in email address)
V6:
- Use CLUSTER_MODE_SINGLECORE for AM62x
- Set PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE for single core.
V7:
- Simplify and rebase on top of base commit "[PATCH v7] remoteproc: k3-r5: Simplify cluster mode setting"

---
 drivers/remoteproc/ti_k3_r5_remoteproc.c | 59 +++++++++++++++++++-----
 1 file changed, 48 insertions(+), 11 deletions(-)
drivers/remoteproc/ti_k3_r5_remoteproc.c +48 -11
index c2ec0f432921..df32f6bc4325 100644
@@ -71,14 +71,16 @@ struct k3_r5_mem {
 /*
  * All cluster mode values are not applicable on all SoCs. The following
  * are the modes supported on various SoCs:
- *   Split mode      : AM65x, J721E, J7200 and AM64x SoCs
- *   LockStep mode   : AM65x, J721E and J7200 SoCs
- *   Single-CPU mode : AM64x SoCs only
+ *   Split mode       : AM65x, J721E, J7200 and AM64x SoCs
+ *   LockStep mode    : AM65x, J721E and J7200 SoCs
+ *   Single-CPU mode  : AM64x SoCs only
+ *   Single-Core mode : AM62x, AM62A SoCs
  */
 enum cluster_mode {
 	CLUSTER_MODE_SPLIT = 0,
 	CLUSTER_MODE_LOCKSTEP,
 	CLUSTER_MODE_SINGLECPU,
+	CLUSTER_MODE_SINGLECORE
 };
 
 /**

@@ -86,11 +88,13 @@ enum cluster_mode {
  * @tcm_is_double: flag to denote the larger unified TCMs in certain modes
  * @tcm_ecc_autoinit: flag to denote the auto-initialization of TCMs for ECC
  * @single_cpu_mode: flag to denote if SoC/IP supports Single-CPU mode
+ * @is_single_core: flag to denote if SoC/IP has only single core R5
  */
 struct k3_r5_soc_data {
 	bool tcm_is_double;
 	bool tcm_ecc_autoinit;
 	bool single_cpu_mode;
+	bool is_single_core;
 };
 
 /**

@@ -838,7 +842,8 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
 
 	core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem);
 	if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
-	    cluster->mode == CLUSTER_MODE_SINGLECPU) {
+	    cluster->mode == CLUSTER_MODE_SINGLECPU ||
+	    cluster->mode == CLUSTER_MODE_SINGLECORE) {
 		core = core0;
 	} else {
 		core = kproc->core;

@@ -877,7 +882,8 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
 		 * with the bit configured, so program it only on
 		 * permitted cores
 		 */
-		if (cluster->mode == CLUSTER_MODE_SINGLECPU) {
+		if (cluster->mode == CLUSTER_MODE_SINGLECPU ||
+		    cluster->mode == CLUSTER_MODE_SINGLECORE) {
 			set_cfg = PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE;
 		} else {
 			/*

@@ -1069,6 +1075,7 @@ static void k3_r5_adjust_tcm_sizes(struct k3_r5_rproc *kproc)
 
 	if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
 	    cluster->mode == CLUSTER_MODE_SINGLECPU ||
+	    cluster->mode == CLUSTER_MODE_SINGLECORE ||
 	    !cluster->soc_data->tcm_is_double)
 		return;
 

@@ -1145,6 +1152,8 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc)
 	if (cluster->soc_data->single_cpu_mode) {
 		mode = cfg & PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE ?
 				CLUSTER_MODE_SINGLECPU : CLUSTER_MODE_SPLIT;
+	} else if (cluster->soc_data->is_single_core) {
+		mode = CLUSTER_MODE_SINGLECORE;
 	} else {
 		mode = cfg & PROC_BOOT_CFG_FLAG_R5_LOCKSTEP ?
 				CLUSTER_MODE_LOCKSTEP : CLUSTER_MODE_SPLIT;

@@ -1264,9 +1273,12 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 			goto err_add;
 		}
 
-		/* create only one rproc in lockstep mode or single-cpu mode */
+		/* create only one rproc in lockstep, single-cpu or
+		 * single core mode
+		 */
 		if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
-		    cluster->mode == CLUSTER_MODE_SINGLECPU)
+		    cluster->mode == CLUSTER_MODE_SINGLECPU ||
+		    cluster->mode == CLUSTER_MODE_SINGLECORE)
 			break;
 	}
 

@@ -1709,19 +1721,33 @@ static int k3_r5_probe(struct platform_device *pdev)
 		/*
 		 * default to most common efuse configurations - Split-mode on AM64x
 		 * and LockStep-mode on all others
+		 * default to most common efuse configurations -
+		 * Split-mode on AM64x
+		 * Single core on AM62x
+		 * LockStep-mode on all others
 		 */
-		cluster->mode = data->single_cpu_mode ?
+		if (!data->is_single_core)
+			cluster->mode = data->single_cpu_mode ?
 					CLUSTER_MODE_SPLIT : CLUSTER_MODE_LOCKSTEP;
+		else
+			cluster->mode = CLUSTER_MODE_SINGLECORE;
 	}
 
-	if (cluster->mode == CLUSTER_MODE_SINGLECPU && !data->single_cpu_mode) {
+	if  ((cluster->mode == CLUSTER_MODE_SINGLECPU && !data->single_cpu_mode) ||
+	     (cluster->mode == CLUSTER_MODE_SINGLECORE && !data->is_single_core)) {
 		dev_err(dev, "Cluster mode = %d is not supported on this SoC\n", cluster->mode);
 		return -EINVAL;
 	}
 
 	num_cores = of_get_available_child_count(np);
-	if (num_cores != 2) {
-		dev_err(dev, "MCU cluster requires both R5F cores to be enabled, num_cores = %d\n",
+	if (num_cores != 2 && !data->is_single_core) {
+		dev_err(dev, "MCU cluster requires both R5F cores to be enabled but num_cores is set to = %d\n",
+			num_cores);
+		return -ENODEV;
+	}
+
+	if (num_cores != 1 && data->is_single_core) {
+		dev_err(dev, "SoC supports only single core R5 but num_cores is set to %d\n",
 			num_cores);
 		return -ENODEV;
 	}

@@ -1763,18 +1789,28 @@ static const struct k3_r5_soc_data am65_j721e_soc_data = {
 	.tcm_is_double = false,
 	.tcm_ecc_autoinit = false,
 	.single_cpu_mode = false,
+	.is_single_core = false,
 };
 
 static const struct k3_r5_soc_data j7200_j721s2_soc_data = {
 	.tcm_is_double = true,
 	.tcm_ecc_autoinit = true,
 	.single_cpu_mode = false,
+	.is_single_core = false,
 };
 
 static const struct k3_r5_soc_data am64_soc_data = {
 	.tcm_is_double = true,
 	.tcm_ecc_autoinit = true,
 	.single_cpu_mode = true,
+	.is_single_core = false,
+};
+
+static const struct k3_r5_soc_data am62_soc_data = {
+	.tcm_is_double = false,
+	.tcm_ecc_autoinit = true,
+	.single_cpu_mode = false,
+	.is_single_core = true,
 };
 
 static const struct of_device_id k3_r5_of_match[] = {

@@ -1782,6 +1818,7 @@ static const struct of_device_id k3_r5_of_match[] = {
 	{ .compatible = "ti,j721e-r5fss", .data = &am65_j721e_soc_data, },
 	{ .compatible = "ti,j7200-r5fss", .data = &j7200_j721s2_soc_data, },
 	{ .compatible = "ti,am64-r5fss",  .data = &am64_soc_data, },
+	{ .compatible = "ti,am62-r5fss",  .data = &am62_soc_data, },
 	{ .compatible = "ti,j721s2-r5fss",  .data = &j7200_j721s2_soc_data, },
 	{ /* sentinel */ },
 };
Signature · 1 line
2.34.1
maintainer
#5 lore raw

Re: [PATCH v7 3/3] remoteproc: k3-r5: Use separate compatible string for TI AM62x SoC family

Quoted 100 lines · Devarsh Thakkar

AM62 and AM62A SoCs use single core R5F which is a new scenario different than the one being used with CLUSTER_MODE_SINGLECPU which is for utilizing a single core from a set of cores available in R5F cluster present in the SoC.

To support this single core scenario map it with newly defined CLUSTER_MODE_SINGLECORE and use it when compatible is set to ti,am62-r5fss.

Also set PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE config for CLUSTER_MODE_SINGLECORE too as it is required by R5 core when it is being as general purpose core instead of device manager.

Signed-off-by Devarsh Thakkar <[email protected]>

---
V2:
- Fix indentation and ordering issues as per review comments
V3:
- Change CLUSTER_MODE_NONE value to -1
V4:
- No change
V5:
- No change (fixing typo in email address)
V6:
- Use CLUSTER_MODE_SINGLECORE for AM62x
- Set PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE for single core.
V7:
- Simplify and rebase on top of base commit "[PATCH v7] remoteproc: k3-r5: Simplify cluster mode setting"

---
 drivers/remoteproc/ti_k3_r5_remoteproc.c | 59 +++++++++++++++++++-----
 1 file changed, 48 insertions(+), 11 deletions(-)
drivers/remoteproc/ti_k3_r5_remoteproc.c +12 -5
index c2ec0f432921..df32f6bc4325 100644
@@ -71,14 +71,16 @@ struct k3_r5_mem {
 /*
  * All cluster mode values are not applicable on all SoCs. The following
  * are the modes supported on various SoCs:
- *   Split mode      : AM65x, J721E, J7200 and AM64x SoCs
- *   LockStep mode   : AM65x, J721E and J7200 SoCs
- *   Single-CPU mode : AM64x SoCs only
+ *   Split mode       : AM65x, J721E, J7200 and AM64x SoCs
+ *   LockStep mode    : AM65x, J721E and J7200 SoCs
+ *   Single-CPU mode  : AM64x SoCs only
+ *   Single-Core mode : AM62x, AM62A SoCs
  */
 enum cluster_mode {
 	CLUSTER_MODE_SPLIT = 0,
 	CLUSTER_MODE_LOCKSTEP,
 	CLUSTER_MODE_SINGLECPU,
+	CLUSTER_MODE_SINGLECORE
 };
 
 /**

@@ -86,11 +88,13 @@ enum cluster_mode {
  * @tcm_is_double: flag to denote the larger unified TCMs in certain modes
  * @tcm_ecc_autoinit: flag to denote the auto-initialization of TCMs for ECC
  * @single_cpu_mode: flag to denote if SoC/IP supports Single-CPU mode
+ * @is_single_core: flag to denote if SoC/IP has only single core R5
  */
 struct k3_r5_soc_data {
 	bool tcm_is_double;
 	bool tcm_ecc_autoinit;
 	bool single_cpu_mode;
+	bool is_single_core;
 };
 
 /**

@@ -838,7 +842,8 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
 
 	core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem);
 	if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
-	    cluster->mode == CLUSTER_MODE_SINGLECPU) {
+	    cluster->mode == CLUSTER_MODE_SINGLECPU ||
+	    cluster->mode == CLUSTER_MODE_SINGLECORE) {
 		core = core0;
 	} else {
 		core = kproc->core;

@@ -877,7 +882,8 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
 		 * with the bit configured, so program it only on
 		 * permitted cores
 		 */
-		if (cluster->mode == CLUSTER_MODE_SINGLECPU) {
+		if (cluster->mode == CLUSTER_MODE_SINGLECPU ||
+		    cluster->mode == CLUSTER_MODE_SINGLECORE) {
 			set_cfg = PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE;
 		} else {
 			/*

@@ -1069,6 +1075,7 @@ static void k3_r5_adjust_tcm_sizes(struct k3_r5_rproc *kproc)
 
 	if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
 	    cluster->mode == CLUSTER_MODE_SINGLECPU ||
+	    cluster->mode == CLUSTER_MODE_SINGLECORE ||
 	    !cluster->soc_data->tcm_is_double)
 		return;

drivers/remoteproc/ti_k3_r5_remoteproc.c static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) · line 1156

Quoted 5 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +2 -0
@@ -1145,6 +1152,8 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc)
 	if (cluster->soc_data->single_cpu_mode) {
 		mode = cfg & PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE ?
 				CLUSTER_MODE_SINGLECPU : CLUSTER_MODE_SPLIT;
+	} else if (cluster->soc_data->is_single_core) {
+		mode = CLUSTER_MODE_SINGLECORE;

I have commented twice on this before - whether it is soc_data->single_cpu_mode or soc_data->is_single_core, I don't want to see them used elsewhere than in a single function. Either in probe() or another function, use them once to set cluster->mode and never again.

I will silently drop any other patchset that doesn't address this.

Quoted 96 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +34 -6
 	} else {
 		mode = cfg & PROC_BOOT_CFG_FLAG_R5_LOCKSTEP ?
 				CLUSTER_MODE_LOCKSTEP : CLUSTER_MODE_SPLIT;

@@ -1264,9 +1273,12 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 			goto err_add;
 		}
 
-		/* create only one rproc in lockstep mode or single-cpu mode */
+		/* create only one rproc in lockstep, single-cpu or
+		 * single core mode
+		 */
 		if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
-		    cluster->mode == CLUSTER_MODE_SINGLECPU)
+		    cluster->mode == CLUSTER_MODE_SINGLECPU ||
+		    cluster->mode == CLUSTER_MODE_SINGLECORE)
 			break;
 	}
 

@@ -1709,19 +1721,33 @@ static int k3_r5_probe(struct platform_device *pdev)
 		/*
 		 * default to most common efuse configurations - Split-mode on AM64x
 		 * and LockStep-mode on all others
+		 * default to most common efuse configurations -
+		 * Split-mode on AM64x
+		 * Single core on AM62x
+		 * LockStep-mode on all others
 		 */
-		cluster->mode = data->single_cpu_mode ?
+		if (!data->is_single_core)
+			cluster->mode = data->single_cpu_mode ?
 					CLUSTER_MODE_SPLIT : CLUSTER_MODE_LOCKSTEP;
+		else
+			cluster->mode = CLUSTER_MODE_SINGLECORE;
 	}
 
-	if (cluster->mode == CLUSTER_MODE_SINGLECPU && !data->single_cpu_mode) {
+	if  ((cluster->mode == CLUSTER_MODE_SINGLECPU && !data->single_cpu_mode) ||
+	     (cluster->mode == CLUSTER_MODE_SINGLECORE && !data->is_single_core)) {
 		dev_err(dev, "Cluster mode = %d is not supported on this SoC\n", cluster->mode);
 		return -EINVAL;
 	}
 
 	num_cores = of_get_available_child_count(np);
-	if (num_cores != 2) {
-		dev_err(dev, "MCU cluster requires both R5F cores to be enabled, num_cores = %d\n",
+	if (num_cores != 2 && !data->is_single_core) {
+		dev_err(dev, "MCU cluster requires both R5F cores to be enabled but num_cores is set to = %d\n",
+			num_cores);
+		return -ENODEV;
+	}
+
+	if (num_cores != 1 && data->is_single_core) {
+		dev_err(dev, "SoC supports only single core R5 but num_cores is set to %d\n",
 			num_cores);
 		return -ENODEV;
 	}

@@ -1763,18 +1789,28 @@ static const struct k3_r5_soc_data am65_j721e_soc_data = {
 	.tcm_is_double = false,
 	.tcm_ecc_autoinit = false,
 	.single_cpu_mode = false,
+	.is_single_core = false,
 };
 
 static const struct k3_r5_soc_data j7200_j721s2_soc_data = {
 	.tcm_is_double = true,
 	.tcm_ecc_autoinit = true,
 	.single_cpu_mode = false,
+	.is_single_core = false,
 };
 
 static const struct k3_r5_soc_data am64_soc_data = {
 	.tcm_is_double = true,
 	.tcm_ecc_autoinit = true,
 	.single_cpu_mode = true,
+	.is_single_core = false,
+};
+
+static const struct k3_r5_soc_data am62_soc_data = {
+	.tcm_is_double = false,
+	.tcm_ecc_autoinit = true,
+	.single_cpu_mode = false,
+	.is_single_core = true,
 };
 
 static const struct of_device_id k3_r5_of_match[] = {

@@ -1782,6 +1818,7 @@ static const struct of_device_id k3_r5_of_match[] = {
 	{ .compatible = "ti,j721e-r5fss", .data = &am65_j721e_soc_data, },
 	{ .compatible = "ti,j7200-r5fss", .data = &j7200_j721s2_soc_data, },
 	{ .compatible = "ti,am64-r5fss",  .data = &am64_soc_data, },
+	{ .compatible = "ti,am62-r5fss",  .data = &am62_soc_data, },
 	{ .compatible = "ti,j721s2-r5fss",  .data = &j7200_j721s2_soc_data, },
 	{ /* sentinel */ },
 };
Signature · 1 line
2.34.1
author
#6 lore raw

Re: [PATCH v7 3/3] remoteproc: k3-r5: Use separate compatible string for TI AM62x SoC family

Hi Mathieu,

Thanks for the review.

Quoted 100 lines · Mathieu Poirier
Quoted 99 lines · Devarsh Thakkar

AM62 and AM62A SoCs use single core R5F which is a new scenario different than the one being used with CLUSTER_MODE_SINGLECPU which is for utilizing a single core from a set of cores available in R5F cluster present in the SoC.

To support this single core scenario map it with newly defined CLUSTER_MODE_SINGLECORE and use it when compatible is set to ti,am62-r5fss.

Also set PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE config for CLUSTER_MODE_SINGLECORE too as it is required by R5 core when it is being as general purpose core instead of device manager.

Signed-off-by Devarsh Thakkar <[email protected]>

---
V2:
- Fix indentation and ordering issues as per review comments
V3:
- Change CLUSTER_MODE_NONE value to -1
V4:
- No change
V5:
- No change (fixing typo in email address)
V6:
- Use CLUSTER_MODE_SINGLECORE for AM62x
- Set PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE for single core.
V7:
- Simplify and rebase on top of base commit "[PATCH v7] remoteproc: k3-r5: Simplify cluster mode setting"

---
 drivers/remoteproc/ti_k3_r5_remoteproc.c | 59 +++++++++++++++++++-----
 1 file changed, 48 insertions(+), 11 deletions(-)
drivers/remoteproc/ti_k3_r5_remoteproc.c +12 -5
index c2ec0f432921..df32f6bc4325 100644
@@ -71,14 +71,16 @@ struct k3_r5_mem {
 /*
  * All cluster mode values are not applicable on all SoCs. The following
  * are the modes supported on various SoCs:
- *   Split mode      : AM65x, J721E, J7200 and AM64x SoCs
- *   LockStep mode   : AM65x, J721E and J7200 SoCs
- *   Single-CPU mode : AM64x SoCs only
+ *   Split mode       : AM65x, J721E, J7200 and AM64x SoCs
+ *   LockStep mode    : AM65x, J721E and J7200 SoCs
+ *   Single-CPU mode  : AM64x SoCs only
+ *   Single-Core mode : AM62x, AM62A SoCs
  */
 enum cluster_mode {
 	CLUSTER_MODE_SPLIT = 0,
 	CLUSTER_MODE_LOCKSTEP,
 	CLUSTER_MODE_SINGLECPU,
+	CLUSTER_MODE_SINGLECORE
 };
 
 /**

@@ -86,11 +88,13 @@ enum cluster_mode {
  * @tcm_is_double: flag to denote the larger unified TCMs in certain modes
  * @tcm_ecc_autoinit: flag to denote the auto-initialization of TCMs for ECC
  * @single_cpu_mode: flag to denote if SoC/IP supports Single-CPU mode
+ * @is_single_core: flag to denote if SoC/IP has only single core R5
  */
 struct k3_r5_soc_data {
 	bool tcm_is_double;
 	bool tcm_ecc_autoinit;
 	bool single_cpu_mode;
+	bool is_single_core;
 };
 
 /**

@@ -838,7 +842,8 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
 
 	core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem);
 	if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
-	    cluster->mode == CLUSTER_MODE_SINGLECPU) {
+	    cluster->mode == CLUSTER_MODE_SINGLECPU ||
+	    cluster->mode == CLUSTER_MODE_SINGLECORE) {
 		core = core0;
 	} else {
 		core = kproc->core;

@@ -877,7 +882,8 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
 		 * with the bit configured, so program it only on
 		 * permitted cores
 		 */
-		if (cluster->mode == CLUSTER_MODE_SINGLECPU) {
+		if (cluster->mode == CLUSTER_MODE_SINGLECPU ||
+		    cluster->mode == CLUSTER_MODE_SINGLECORE) {
 			set_cfg = PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE;
 		} else {
 			/*

@@ -1069,6 +1075,7 @@ static void k3_r5_adjust_tcm_sizes(struct k3_r5_rproc *kproc)
 
 	if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
 	    cluster->mode == CLUSTER_MODE_SINGLECPU ||
+	    cluster->mode == CLUSTER_MODE_SINGLECORE ||
 	    !cluster->soc_data->tcm_is_double)
 		return;
Quoted 10 lines

drivers/remoteproc/ti_k3_r5_remoteproc.c static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) · line 1156

Quoted 6 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +2 -0
@@ -1145,6 +1152,8 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc)
 	if (cluster->soc_data->single_cpu_mode) {
 		mode = cfg & PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE ?
 				CLUSTER_MODE_SINGLECPU : CLUSTER_MODE_SPLIT;
+	} else if (cluster->soc_data->is_single_core) {
+		mode = CLUSTER_MODE_SINGLECORE;

I have commented twice on this before - whether it is soc_data->single_cpu_mode or soc_data->is_single_core, I don't want to see them used elsewhere than in a single function. Either in probe() or another function, use them once to set cluster->mode and never again.

I will remove the soc_data flag usage in V8 from here too. I had original thought to keep it as an extra check

in case som in-appropriate flag was set at bootloader stage due to a bug,

but I will trusting be the device-manager now not to allow setting inappropriate flag at the first place.

Quoted 98 lines

I will silently drop any other patchset that doesn't address this.

Quoted 96 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +34 -6
 	} else {
 		mode = cfg & PROC_BOOT_CFG_FLAG_R5_LOCKSTEP ?
 				CLUSTER_MODE_LOCKSTEP : CLUSTER_MODE_SPLIT;

@@ -1264,9 +1273,12 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 			goto err_add;
 		}
 
-		/* create only one rproc in lockstep mode or single-cpu mode */
+		/* create only one rproc in lockstep, single-cpu or
+		 * single core mode
+		 */
 		if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
-		    cluster->mode == CLUSTER_MODE_SINGLECPU)
+		    cluster->mode == CLUSTER_MODE_SINGLECPU ||
+		    cluster->mode == CLUSTER_MODE_SINGLECORE)
 			break;
 	}
 

@@ -1709,19 +1721,33 @@ static int k3_r5_probe(struct platform_device *pdev)
 		/*
 		 * default to most common efuse configurations - Split-mode on AM64x
 		 * and LockStep-mode on all others
+		 * default to most common efuse configurations -
+		 * Split-mode on AM64x
+		 * Single core on AM62x
+		 * LockStep-mode on all others
 		 */
-		cluster->mode = data->single_cpu_mode ?
+		if (!data->is_single_core)
+			cluster->mode = data->single_cpu_mode ?
 					CLUSTER_MODE_SPLIT : CLUSTER_MODE_LOCKSTEP;
+		else
+			cluster->mode = CLUSTER_MODE_SINGLECORE;
 	}
 
-	if (cluster->mode == CLUSTER_MODE_SINGLECPU && !data->single_cpu_mode) {
+	if  ((cluster->mode == CLUSTER_MODE_SINGLECPU && !data->single_cpu_mode) ||
+	     (cluster->mode == CLUSTER_MODE_SINGLECORE && !data->is_single_core)) {
 		dev_err(dev, "Cluster mode = %d is not supported on this SoC\n", cluster->mode);
 		return -EINVAL;
 	}
 
 	num_cores = of_get_available_child_count(np);
-	if (num_cores != 2) {
-		dev_err(dev, "MCU cluster requires both R5F cores to be enabled, num_cores = %d\n",
+	if (num_cores != 2 && !data->is_single_core) {
+		dev_err(dev, "MCU cluster requires both R5F cores to be enabled but num_cores is set to = %d\n",
+			num_cores);
+		return -ENODEV;
+	}
+
+	if (num_cores != 1 && data->is_single_core) {
+		dev_err(dev, "SoC supports only single core R5 but num_cores is set to %d\n",
 			num_cores);
 		return -ENODEV;
 	}

@@ -1763,18 +1789,28 @@ static const struct k3_r5_soc_data am65_j721e_soc_data = {
 	.tcm_is_double = false,
 	.tcm_ecc_autoinit = false,
 	.single_cpu_mode = false,
+	.is_single_core = false,
 };
 
 static const struct k3_r5_soc_data j7200_j721s2_soc_data = {
 	.tcm_is_double = true,
 	.tcm_ecc_autoinit = true,
 	.single_cpu_mode = false,
+	.is_single_core = false,
 };
 
 static const struct k3_r5_soc_data am64_soc_data = {
 	.tcm_is_double = true,
 	.tcm_ecc_autoinit = true,
 	.single_cpu_mode = true,
+	.is_single_core = false,
+};
+
+static const struct k3_r5_soc_data am62_soc_data = {
+	.tcm_is_double = false,
+	.tcm_ecc_autoinit = true,
+	.single_cpu_mode = false,
+	.is_single_core = true,
 };
 
 static const struct of_device_id k3_r5_of_match[] = {

@@ -1782,6 +1818,7 @@ static const struct of_device_id k3_r5_of_match[] = {
 	{ .compatible = "ti,j721e-r5fss", .data = &am65_j721e_soc_data, },
 	{ .compatible = "ti,j7200-r5fss", .data = &j7200_j721s2_soc_data, },
 	{ .compatible = "ti,am64-r5fss",  .data = &am64_soc_data, },
+	{ .compatible = "ti,am62-r5fss",  .data = &am62_soc_data, },
 	{ .compatible = "ti,j721s2-r5fss",  .data = &j7200_j721s2_soc_data, },
 	{ /* sentinel */ },
 };
Signature · 1 line
2.34.1
#7 lore raw

Re: [PATCH v7 3/3] remoteproc: k3-r5: Use separate compatible string for TI AM62x SoC family

In reply to Mathieu Poirier

Hi Devarsh,

Quoted 47 lines · Mathieu Poirier
Quoted 46 lines · Devarsh Thakkar

AM62 and AM62A SoCs use single core R5F which is a new scenario different than the one being used with CLUSTER_MODE_SINGLECPU which is for utilizing a single core from a set of cores available in R5F cluster present in the SoC.

To support this single core scenario map it with newly defined CLUSTER_MODE_SINGLECORE and use it when compatible is set to ti,am62-r5fss.

Also set PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE config for CLUSTER_MODE_SINGLECORE too as it is required by R5 core when it is being as general purpose core instead of device manager.

Signed-off-by Devarsh Thakkar <[email protected]>

---
V2:
- Fix indentation and ordering issues as per review comments
V3:
- Change CLUSTER_MODE_NONE value to -1
V4:
- No change
V5:
- No change (fixing typo in email address)
V6:
- Use CLUSTER_MODE_SINGLECORE for AM62x
- Set PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE for single core.
V7:
- Simplify and rebase on top of base commit "[PATCH v7] remoteproc: k3-r5: Simplify cluster mode setting"

---
 drivers/remoteproc/ti_k3_r5_remoteproc.c | 59 +++++++++++++++++++-----
 1 file changed, 48 insertions(+), 11 deletions(-)
drivers/remoteproc/ti_k3_r5_remoteproc.c +2 -3
index c2ec0f432921..df32f6bc4325 100644
@@ -71,14 +71,16 @@ struct k3_r5_mem {
 /*
  * All cluster mode values are not applicable on all SoCs. The following
  * are the modes supported on various SoCs:
- *   Split mode      : AM65x, J721E, J7200 and AM64x SoCs
- *   LockStep mode   : AM65x, J721E and J7200 SoCs
- *   Single-CPU mode : AM64x SoCs only
+ *   Split mode       : AM65x, J721E, J7200 and AM64x SoCs
+ *   LockStep mode    : AM65x, J721E and J7200 SoCs
Quoted 8 lines

drivers/remoteproc/ti_k3_r5_remoteproc.c struct k3_r5_mem { · line 83

Quoted 8 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +3 -0
+ *   Single-CPU mode  : AM64x SoCs only
+ *   Single-Core mode : AM62x, AM62A SoCs
  */
 enum cluster_mode {
 	CLUSTER_MODE_SPLIT = 0,
 	CLUSTER_MODE_LOCKSTEP,
 	CLUSTER_MODE_SINGLECPU,
+	CLUSTER_MODE_SINGLECORE

What is the difference in device driver behaviour between SINGLECPU and SINGLECORE?

If there is no difference then you should not introduce a new enum.

Quoted 143 lines
Quoted 46 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +7 -2
 };
 
 /**

@@ -86,11 +88,13 @@ enum cluster_mode {
  * @tcm_is_double: flag to denote the larger unified TCMs in certain modes
  * @tcm_ecc_autoinit: flag to denote the auto-initialization of TCMs for ECC
  * @single_cpu_mode: flag to denote if SoC/IP supports Single-CPU mode
+ * @is_single_core: flag to denote if SoC/IP has only single core R5
  */
 struct k3_r5_soc_data {
 	bool tcm_is_double;
 	bool tcm_ecc_autoinit;
 	bool single_cpu_mode;
+	bool is_single_core;
 };
 
 /**

@@ -838,7 +842,8 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
 
 	core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem);
 	if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
-	    cluster->mode == CLUSTER_MODE_SINGLECPU) {
+	    cluster->mode == CLUSTER_MODE_SINGLECPU ||
+	    cluster->mode == CLUSTER_MODE_SINGLECORE) {
 		core = core0;
 	} else {
 		core = kproc->core;

@@ -877,7 +882,8 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
 		 * with the bit configured, so program it only on
 		 * permitted cores
 		 */
-		if (cluster->mode == CLUSTER_MODE_SINGLECPU) {
+		if (cluster->mode == CLUSTER_MODE_SINGLECPU ||
+		    cluster->mode == CLUSTER_MODE_SINGLECORE) {
 			set_cfg = PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE;
 		} else {
 			/*

@@ -1069,6 +1075,7 @@ static void k3_r5_adjust_tcm_sizes(struct k3_r5_rproc *kproc)
 
 	if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
 	    cluster->mode == CLUSTER_MODE_SINGLECPU ||
+	    cluster->mode == CLUSTER_MODE_SINGLECORE ||
 	    !cluster->soc_data->tcm_is_double)
 		return;

drivers/remoteproc/ti_k3_r5_remoteproc.c static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) · line 1156

Quoted 5 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +2 -0
@@ -1145,6 +1152,8 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc)
 	if (cluster->soc_data->single_cpu_mode) {
 		mode = cfg & PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE ?
 				CLUSTER_MODE_SINGLECPU : CLUSTER_MODE_SPLIT;
+	} else if (cluster->soc_data->is_single_core) {
+		mode = CLUSTER_MODE_SINGLECORE;

I have commented twice on this before - whether it is soc_data->single_cpu_mode or soc_data->is_single_core, I don't want to see them used elsewhere than in a single function. Either in probe() or another function, use them once to set cluster->mode and never again.

I will silently drop any other patchset that doesn't address this.

Quoted 84 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +33 -6
 	} else {
 		mode = cfg & PROC_BOOT_CFG_FLAG_R5_LOCKSTEP ?
 				CLUSTER_MODE_LOCKSTEP : CLUSTER_MODE_SPLIT;

@@ -1264,9 +1273,12 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 			goto err_add;
 		}
 
-		/* create only one rproc in lockstep mode or single-cpu mode */
+		/* create only one rproc in lockstep, single-cpu or
+		 * single core mode
+		 */
 		if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
-		    cluster->mode == CLUSTER_MODE_SINGLECPU)
+		    cluster->mode == CLUSTER_MODE_SINGLECPU ||
+		    cluster->mode == CLUSTER_MODE_SINGLECORE)
 			break;
 	}
 

@@ -1709,19 +1721,33 @@ static int k3_r5_probe(struct platform_device *pdev)
 		/*
 		 * default to most common efuse configurations - Split-mode on AM64x
 		 * and LockStep-mode on all others
+		 * default to most common efuse configurations -
+		 * Split-mode on AM64x
+		 * Single core on AM62x
+		 * LockStep-mode on all others
 		 */
-		cluster->mode = data->single_cpu_mode ?
+		if (!data->is_single_core)
+			cluster->mode = data->single_cpu_mode ?
 					CLUSTER_MODE_SPLIT : CLUSTER_MODE_LOCKSTEP;
+		else
+			cluster->mode = CLUSTER_MODE_SINGLECORE;
 	}
 
-	if (cluster->mode == CLUSTER_MODE_SINGLECPU && !data->single_cpu_mode) {
+	if  ((cluster->mode == CLUSTER_MODE_SINGLECPU && !data->single_cpu_mode) ||
+	     (cluster->mode == CLUSTER_MODE_SINGLECORE && !data->is_single_core)) {
 		dev_err(dev, "Cluster mode = %d is not supported on this SoC\n", cluster->mode);
 		return -EINVAL;
 	}
 
 	num_cores = of_get_available_child_count(np);
-	if (num_cores != 2) {
-		dev_err(dev, "MCU cluster requires both R5F cores to be enabled, num_cores = %d\n",
+	if (num_cores != 2 && !data->is_single_core) {
+		dev_err(dev, "MCU cluster requires both R5F cores to be enabled but num_cores is set to = %d\n",
+			num_cores);
+		return -ENODEV;
+	}
+
+	if (num_cores != 1 && data->is_single_core) {
+		dev_err(dev, "SoC supports only single core R5 but num_cores is set to %d\n",
 			num_cores);
 		return -ENODEV;
 	}

@@ -1763,18 +1789,28 @@ static const struct k3_r5_soc_data am65_j721e_soc_data = {
 	.tcm_is_double = false,
 	.tcm_ecc_autoinit = false,
 	.single_cpu_mode = false,
+	.is_single_core = false,
 };
 
 static const struct k3_r5_soc_data j7200_j721s2_soc_data = {
 	.tcm_is_double = true,
 	.tcm_ecc_autoinit = true,
 	.single_cpu_mode = false,
+	.is_single_core = false,
 };
 
 static const struct k3_r5_soc_data am64_soc_data = {
 	.tcm_is_double = true,
 	.tcm_ecc_autoinit = true,
 	.single_cpu_mode = true,
+	.is_single_core = false,
+};
+
+static const struct k3_r5_soc_data am62_soc_data = {
+	.tcm_is_double = false,
+	.tcm_ecc_autoinit = true,
+	.single_cpu_mode = false,
+	.is_single_core = true,
 };
Quoted 12 lines
Quoted 12 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +1 -0
 static const struct of_device_id k3_r5_of_match[] = {

@@ -1782,6 +1818,7 @@ static const struct of_device_id k3_r5_of_match[] = {
 	{ .compatible = "ti,j721e-r5fss", .data = &am65_j721e_soc_data, },
 	{ .compatible = "ti,j7200-r5fss", .data = &j7200_j721s2_soc_data, },
 	{ .compatible = "ti,am64-r5fss",  .data = &am64_soc_data, },
+	{ .compatible = "ti,am62-r5fss",  .data = &am62_soc_data, },
 	{ .compatible = "ti,j721s2-r5fss",  .data = &j7200_j721s2_soc_data, },
 	{ /* sentinel */ },
 };
Signature · 1 line
2.34.1

cheers,
-roger

author
#8 lore raw

Re: [PATCH v7 3/3] remoteproc: k3-r5: Use separate compatible string for TI AM62x SoC family

Hi Roger,

Quoted 53 lines · Roger Quadros

Hi Devarsh,

Quoted 50 lines · Mathieu Poirier
Quoted 49 lines · Devarsh Thakkar

AM62 and AM62A SoCs use single core R5F which is a new scenario different than the one being used with CLUSTER_MODE_SINGLECPU which is for utilizing a single core from a set of cores available in R5F cluster present in the SoC.

To support this single core scenario map it with newly defined CLUSTER_MODE_SINGLECORE and use it when compatible is set to ti,am62-r5fss.

Also set PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE config for CLUSTER_MODE_SINGLECORE too as it is required by R5 core when it is being as general purpose core instead of device manager.

Signed-off-by Devarsh Thakkar <[email protected]>

---
V2:
- Fix indentation and ordering issues as per review comments
V3:
- Change CLUSTER_MODE_NONE value to -1
V4:
- No change
V5:
- No change (fixing typo in email address)
V6:
- Use CLUSTER_MODE_SINGLECORE for AM62x
- Set PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE for single core.
V7:
- Simplify and rebase on top of base commit "[PATCH v7] remoteproc: k3-r5: Simplify cluster mode setting"

---
  drivers/remoteproc/ti_k3_r5_remoteproc.c | 59 +++++++++++++++++++-----
  1 file changed, 48 insertions(+), 11 deletions(-)
drivers/remoteproc/ti_k3_r5_remoteproc.c +4 -3
index c2ec0f432921..df32f6bc4325 100644
@@ -71,14 +71,16 @@ struct k3_r5_mem {
  /*
   * All cluster mode values are not applicable on all SoCs. The following
   * are the modes supported on various SoCs:
- *   Split mode      : AM65x, J721E, J7200 and AM64x SoCs
- *   LockStep mode   : AM65x, J721E and J7200 SoCs
- *   Single-CPU mode : AM64x SoCs only
+ *   Split mode       : AM65x, J721E, J7200 and AM64x SoCs
+ *   LockStep mode    : AM65x, J721E and J7200 SoCs
+ *   Single-CPU mode  : AM64x SoCs only
+ *   Single-Core mode : AM62x, AM62A SoCs
   */
Quoted 9 lines
Quoted 5 lines

drivers/remoteproc/ti_k3_r5_remoteproc.c struct k3_r5_mem { · line 83

Quoted 5 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +1 -0
  enum cluster_mode {
  	CLUSTER_MODE_SPLIT = 0,
  	CLUSTER_MODE_LOCKSTEP,
  	CLUSTER_MODE_SINGLECPU,
+	CLUSTER_MODE_SINGLECORE

What is the difference in device driver behaviour between SINGLECPU and SINGLECORE?

Yeah there is quite a bit of common code flow between the two but the fundamental difference is that you use CLUSTER_MODE_SINGLECPU when you have two R5F cores but you want to use only single R5F core albeit
with using TCM of both the cores whereas CLUSTER_MODE_SINGLECORE is
for the scenario where you have single core R5F's only.

Also the bindings for CLUSTER_MODE_SINGLECPU are already upstream so did
not want to break them either : https://gitlab.com/linux-kernel/linux-next/-/blob/next-20230328/Documentation/devicetree/bindings/remoteproc/ti%2Ck3-r5f-rproc.yaml#L20.

Regards
Devarsh

Quoted 160 lines

If there is no difference then you should not introduce a new enum. >

Quoted 143 lines
Quoted 46 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +7 -2
  };
 
  /**

@@ -86,11 +88,13 @@ enum cluster_mode {
   * @tcm_is_double: flag to denote the larger unified TCMs in certain modes
   * @tcm_ecc_autoinit: flag to denote the auto-initialization of TCMs for ECC
   * @single_cpu_mode: flag to denote if SoC/IP supports Single-CPU mode
+ * @is_single_core: flag to denote if SoC/IP has only single core R5
   */
  struct k3_r5_soc_data {
  	bool tcm_is_double;
  	bool tcm_ecc_autoinit;
  	bool single_cpu_mode;
+	bool is_single_core;
  };
 
  /**

@@ -838,7 +842,8 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
 
  	core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem);
  	if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
-	    cluster->mode == CLUSTER_MODE_SINGLECPU) {
+	    cluster->mode == CLUSTER_MODE_SINGLECPU ||
+	    cluster->mode == CLUSTER_MODE_SINGLECORE) {
  		core = core0;
  	} else {
  		core = kproc->core;

@@ -877,7 +882,8 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
  		 * with the bit configured, so program it only on
  		 * permitted cores
  		 */
-		if (cluster->mode == CLUSTER_MODE_SINGLECPU) {
+		if (cluster->mode == CLUSTER_MODE_SINGLECPU ||
+		    cluster->mode == CLUSTER_MODE_SINGLECORE) {
  			set_cfg = PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE;
  		} else {
  			/*

@@ -1069,6 +1075,7 @@ static void k3_r5_adjust_tcm_sizes(struct k3_r5_rproc *kproc)
 
  	if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
  	    cluster->mode == CLUSTER_MODE_SINGLECPU ||
+	    cluster->mode == CLUSTER_MODE_SINGLECORE ||
  	    !cluster->soc_data->tcm_is_double)
  		return;

drivers/remoteproc/ti_k3_r5_remoteproc.c static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) · line 1156

Quoted 5 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +2 -0
@@ -1145,6 +1152,8 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc)
  	if (cluster->soc_data->single_cpu_mode) {
  		mode = cfg & PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE ?
  				CLUSTER_MODE_SINGLECPU : CLUSTER_MODE_SPLIT;
+	} else if (cluster->soc_data->is_single_core) {
+		mode = CLUSTER_MODE_SINGLECORE;

I have commented twice on this before - whether it is soc_data->single_cpu_mode or soc_data->is_single_core, I don't want to see them used elsewhere than in a single function. Either in probe() or another function, use them once to set cluster->mode and never again.

I will silently drop any other patchset that doesn't address this.

Quoted 84 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +33 -6
  	} else {
  		mode = cfg & PROC_BOOT_CFG_FLAG_R5_LOCKSTEP ?
  				CLUSTER_MODE_LOCKSTEP : CLUSTER_MODE_SPLIT;

@@ -1264,9 +1273,12 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
  			goto err_add;
  		}
 
-		/* create only one rproc in lockstep mode or single-cpu mode */
+		/* create only one rproc in lockstep, single-cpu or
+		 * single core mode
+		 */
  		if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
-		    cluster->mode == CLUSTER_MODE_SINGLECPU)
+		    cluster->mode == CLUSTER_MODE_SINGLECPU ||
+		    cluster->mode == CLUSTER_MODE_SINGLECORE)
  			break;
  	}
 

@@ -1709,19 +1721,33 @@ static int k3_r5_probe(struct platform_device *pdev)
  		/*
  		 * default to most common efuse configurations - Split-mode on AM64x
  		 * and LockStep-mode on all others
+		 * default to most common efuse configurations -
+		 * Split-mode on AM64x
+		 * Single core on AM62x
+		 * LockStep-mode on all others
  		 */
-		cluster->mode = data->single_cpu_mode ?
+		if (!data->is_single_core)
+			cluster->mode = data->single_cpu_mode ?
  					CLUSTER_MODE_SPLIT : CLUSTER_MODE_LOCKSTEP;
+		else
+			cluster->mode = CLUSTER_MODE_SINGLECORE;
  	}
 
-	if (cluster->mode == CLUSTER_MODE_SINGLECPU && !data->single_cpu_mode) {
+	if  ((cluster->mode == CLUSTER_MODE_SINGLECPU && !data->single_cpu_mode) ||
+	     (cluster->mode == CLUSTER_MODE_SINGLECORE && !data->is_single_core)) {
  		dev_err(dev, "Cluster mode = %d is not supported on this SoC\n", cluster->mode);
  		return -EINVAL;
  	}
 
  	num_cores = of_get_available_child_count(np);
-	if (num_cores != 2) {
-		dev_err(dev, "MCU cluster requires both R5F cores to be enabled, num_cores = %d\n",
+	if (num_cores != 2 && !data->is_single_core) {
+		dev_err(dev, "MCU cluster requires both R5F cores to be enabled but num_cores is set to = %d\n",
+			num_cores);
+		return -ENODEV;
+	}
+
+	if (num_cores != 1 && data->is_single_core) {
+		dev_err(dev, "SoC supports only single core R5 but num_cores is set to %d\n",
  			num_cores);
  		return -ENODEV;
  	}

@@ -1763,18 +1789,28 @@ static const struct k3_r5_soc_data am65_j721e_soc_data = {
  	.tcm_is_double = false,
  	.tcm_ecc_autoinit = false,
  	.single_cpu_mode = false,
+	.is_single_core = false,
  };
 
  static const struct k3_r5_soc_data j7200_j721s2_soc_data = {
  	.tcm_is_double = true,
  	.tcm_ecc_autoinit = true,
  	.single_cpu_mode = false,
+	.is_single_core = false,
  };
 
  static const struct k3_r5_soc_data am64_soc_data = {
  	.tcm_is_double = true,
  	.tcm_ecc_autoinit = true,
  	.single_cpu_mode = true,
+	.is_single_core = false,
+};
+
+static const struct k3_r5_soc_data am62_soc_data = {
+	.tcm_is_double = false,
+	.tcm_ecc_autoinit = true,
+	.single_cpu_mode = false,
+	.is_single_core = true,
  };
Quoted 12 lines
Quoted 12 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +1 -0
  static const struct of_device_id k3_r5_of_match[] = {

@@ -1782,6 +1818,7 @@ static const struct of_device_id k3_r5_of_match[] = {
  	{ .compatible = "ti,j721e-r5fss", .data = &am65_j721e_soc_data, },
  	{ .compatible = "ti,j7200-r5fss", .data = &j7200_j721s2_soc_data, },
  	{ .compatible = "ti,am64-r5fss",  .data = &am64_soc_data, },
+	{ .compatible = "ti,am62-r5fss",  .data = &am62_soc_data, },
  	{ .compatible = "ti,j721s2-r5fss",  .data = &j7200_j721s2_soc_data, },
  	{ /* sentinel */ },
  };
Signature · 1 line
2.34.1

cheers,
-roger

#9 lore raw

Re: [PATCH v7 3/3] remoteproc: k3-r5: Use separate compatible string for TI AM62x SoC family

Quoted 65 lines · Devarsh Thakkar

Hi Roger,

Quoted 53 lines · Roger Quadros

Hi Devarsh,

Quoted 50 lines · Mathieu Poirier
Quoted 49 lines · Devarsh Thakkar

AM62 and AM62A SoCs use single core R5F which is a new scenario different than the one being used with CLUSTER_MODE_SINGLECPU which is for utilizing a single core from a set of cores available in R5F cluster present in the SoC.

To support this single core scenario map it with newly defined CLUSTER_MODE_SINGLECORE and use it when compatible is set to ti,am62-r5fss.

Also set PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE config for CLUSTER_MODE_SINGLECORE too as it is required by R5 core when it is being as general purpose core instead of device manager.

Signed-off-by Devarsh Thakkar <[email protected]>

---
V2:
- Fix indentation and ordering issues as per review comments
V3:
- Change CLUSTER_MODE_NONE value to -1
V4:
- No change
V5:
- No change (fixing typo in email address)
V6:
    - Use CLUSTER_MODE_SINGLECORE for AM62x
    - Set PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE for single core.
V7:
    - Simplify and rebase on top of base commit "[PATCH v7] remoteproc: k3-r5: Simplify cluster mode setting"
---
  drivers/remoteproc/ti_k3_r5_remoteproc.c | 59 +++++++++++++++++++-----

  1 file changed, 48 insertions(+), 11 deletions(-)
drivers/remoteproc/ti_k3_r5_remoteproc.c +4 -3
index c2ec0f432921..df32f6bc4325 100644
@@ -71,14 +71,16 @@ struct k3_r5_mem {
   /*
    * All cluster mode values are not applicable on all SoCs. The following
    * are the modes supported on various SoCs:
- *   Split mode      : AM65x, J721E, J7200 and AM64x SoCs
- *   LockStep mode   : AM65x, J721E and J7200 SoCs
- *   Single-CPU mode : AM64x SoCs only
+ *   Split mode       : AM65x, J721E, J7200 and AM64x SoCs
+ *   LockStep mode    : AM65x, J721E and J7200 SoCs
+ *   Single-CPU mode  : AM64x SoCs only
+ *   Single-Core mode : AM62x, AM62A SoCs
    */
Quoted 9 lines
Quoted 5 lines

drivers/remoteproc/ti_k3_r5_remoteproc.c struct k3_r5_mem { · line 83

Quoted 5 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +1 -0
   enum cluster_mode {
       CLUSTER_MODE_SPLIT = 0,
       CLUSTER_MODE_LOCKSTEP,
       CLUSTER_MODE_SINGLECPU,
+    CLUSTER_MODE_SINGLECORE

What is the difference in device driver behaviour between SINGLECPU and SINGLECORE?

Quoted 1 line

Yeah there is quite a bit of common code flow between the two but the fundamental difference is that you use CLUSTER_MODE_SINGLECPU when

I still didn't get what is the difference between the two from SW point of view. What happens if you just use CLUSTER_MODE_SINGLECPU for AM62 SoC?

Quoted 4 lines

you have two R5F cores but you want to use only single R5F core albeit with using TCM of both the cores whereas CLUSTER_MODE_SINGLECORE is for the scenario where you have single core R5F's only.

Quoted 152 lines

Also the bindings for CLUSTER_MODE_SINGLECPU are already upstream so did
not want to break them either : https://gitlab.com/linux-kernel/linux-next/-/blob/next-20230328/Documentation/devicetree/bindings/remoteproc/ti%2Ck3-r5f-rproc.yaml#L20.

Regards
Devarsh

Quoted 146 lines

If there is no difference then you should not introduce a new enum. >

Quoted 143 lines
Quoted 38 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +2 -0
   };
     /**

@@ -86,11 +88,13 @@ enum cluster_mode {
    * @tcm_is_double: flag to denote the larger unified TCMs in certain modes
    * @tcm_ecc_autoinit: flag to denote the auto-initialization of TCMs for ECC
    * @single_cpu_mode: flag to denote if SoC/IP supports Single-CPU mode
+ * @is_single_core: flag to denote if SoC/IP has only single core R5
    */
   struct k3_r5_soc_data {
       bool tcm_is_double;
       bool tcm_ecc_autoinit;
       bool single_cpu_mode;
+    bool is_single_core;
   };
     /**

@@ -838,7 +842,8 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
         core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem);

      if (cluster->mode == CLUSTER_MODE_LOCKSTEP || -        cluster->mode == CLUSTER_MODE_SINGLECPU) { +        cluster->mode == CLUSTER_MODE_SINGLECPU || +        cluster->mode == CLUSTER_MODE_SINGLECORE) { core = core0;
      } else {
          core = kproc->core;

drivers/remoteproc/ti_k3_r5_remoteproc.c +2 -1
@@ -877,7 +882,8 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
            * with the bit configured, so program it only on
            * permitted cores
            */
-        if (cluster->mode == CLUSTER_MODE_SINGLECPU) {
+        if (cluster->mode == CLUSTER_MODE_SINGLECPU ||
+            cluster->mode == CLUSTER_MODE_SINGLECORE) {
               set_cfg = PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE;
           } else {
               /*

@@ -1069,6 +1075,7 @@ static void k3_r5_adjust_tcm_sizes(struct k3_r5_rproc *kproc)

        if (cluster->mode == CLUSTER_MODE_LOCKSTEP || cluster->mode == CLUSTER_MODE_SINGLECPU || +        cluster->mode == CLUSTER_MODE_SINGLECORE ||

Quoted 8 lines

          !cluster->soc_data->tcm_is_double)
          return;
  @@ -1145,6 +1152,8 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc) if (cluster->soc_data->single_cpu_mode) {
          mode = cfg & PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE ? CLUSTER_MODE_SINGLECPU : CLUSTER_MODE_SPLIT;
+    } else if (cluster->soc_data->is_single_core) {
+        mode = CLUSTER_MODE_SINGLECORE;

I have commented twice on this before - whether it is soc_data->single_cpu_mode or soc_data->is_single_core, I don't want to see them used elsewhere than in a single function.  Either in probe() or another function, use them once to set cluster->mode and never again.

I will silently drop any other patchset that doesn't address this.

Quoted 89 lines

      } else {
          mode = cfg & PROC_BOOT_CFG_FLAG_R5_LOCKSTEP ? CLUSTER_MODE_LOCKSTEP : CLUSTER_MODE_SPLIT;

drivers/remoteproc/ti_k3_r5_remoteproc.c +0 -0
@@ -1264,9 +1273,12 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
               goto err_add;
           }

  -        /* create only one rproc in lockstep mode or single-cpu mode */
+        /* create only one rproc in lockstep, single-cpu or
+         * single core mode
+         */
          if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
-            cluster->mode == CLUSTER_MODE_SINGLECPU)
+            cluster->mode == CLUSTER_MODE_SINGLECPU ||
+            cluster->mode == CLUSTER_MODE_SINGLECORE)
              break;
      }
  @@ -1709,19 +1721,33 @@ static int k3_r5_probe(struct platform_device *pdev)
          /*
           * default to most common efuse configurations - Split-mode on AM64x
           * and LockStep-mode on all others
+         * default to most common efuse configurations -
+         * Split-mode on AM64x
+         * Single core on AM62x
+         * LockStep-mode on all others
           */
-        cluster->mode = data->single_cpu_mode ?
+        if (!data->is_single_core)
+            cluster->mode = data->single_cpu_mode ?
                      CLUSTER_MODE_SPLIT : CLUSTER_MODE_LOCKSTEP;
+        else
+            cluster->mode = CLUSTER_MODE_SINGLECORE;
      }
  -    if (cluster->mode == CLUSTER_MODE_SINGLECPU && !data->single_cpu_mode) {
+    if  ((cluster->mode == CLUSTER_MODE_SINGLECPU && !data->single_cpu_mode) ||
+         (cluster->mode == CLUSTER_MODE_SINGLECORE && !data->is_single_core)) { dev_err(dev, "Cluster mode = %d is not supported on this SoC\n", cluster->mode); return -EINVAL;
      }
        num_cores = of_get_available_child_count(np);
-    if (num_cores != 2) {
-        dev_err(dev, "MCU cluster requires both R5F cores to be enabled, num_cores = %d\n", +    if (num_cores != 2 && !data->is_single_core) {
+        dev_err(dev, "MCU cluster requires both R5F cores to be enabled but num_cores is set to = %d\n", +            num_cores);
+        return -ENODEV;
+    }
+
+    if (num_cores != 1 && data->is_single_core) {
+        dev_err(dev, "SoC supports only single core R5 but num_cores is set to %d\n", num_cores);
          return -ENODEV;
      }

drivers/remoteproc/ti_k3_r5_remoteproc.c +11 -0
@@ -1763,18 +1789,28 @@ static const struct k3_r5_soc_data am65_j721e_soc_data = {
       .tcm_is_double = false,
       .tcm_ecc_autoinit = false,
       .single_cpu_mode = false,
+    .is_single_core = false,
   };
     static const struct k3_r5_soc_data j7200_j721s2_soc_data = {
       .tcm_is_double = true,
       .tcm_ecc_autoinit = true,
       .single_cpu_mode = false,
+    .is_single_core = false,
   };
     static const struct k3_r5_soc_data am64_soc_data = {
       .tcm_is_double = true,
       .tcm_ecc_autoinit = true,
       .single_cpu_mode = true,
+    .is_single_core = false,
+};
+
+static const struct k3_r5_soc_data am62_soc_data = {
+    .tcm_is_double = false,
+    .tcm_ecc_autoinit = true,
+    .single_cpu_mode = false,
+    .is_single_core = true,
   };
     static const struct of_device_id k3_r5_of_match[] = {

@@ -1782,6 +1818,7 @@ static const struct of_device_id k3_r5_of_match[] = {
       { .compatible = "ti,j721e-r5fss", .data = &am65_j721e_soc_data, },
       { .compatible = "ti,j7200-r5fss", .data = &j7200_j721s2_soc_data, },
       { .compatible = "ti,am64-r5fss",  .data = &am64_soc_data, },
+    { .compatible = "ti,am62-r5fss",  .data = &am62_soc_data, },
       { .compatible = "ti,j721s2-r5fss",  .data = &j7200_j721s2_soc_data, },
       { /* sentinel */ },
   };

--
2.34.1

--
cheers,
-roger

author
#10 lore raw

Re: [PATCH v7 3/3] remoteproc: k3-r5: Use separate compatible string for TI AM62x SoC family

Hi Roger,

Quoted 68 lines · Roger Quadros
Quoted 65 lines · Devarsh Thakkar

Hi Roger,

Quoted 62 lines · Roger Quadros

Hi Devarsh,

Quoted 47 lines · Mathieu Poirier
Quoted 46 lines · Devarsh Thakkar

AM62 and AM62A SoCs use single core R5F which is a new scenario different than the one being used with CLUSTER_MODE_SINGLECPU which is for utilizing a single core from a set of cores available in R5F cluster present in the SoC.

To support this single core scenario map it with newly defined CLUSTER_MODE_SINGLECORE and use it when compatible is set to ti,am62-r5fss.

Also set PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE config for CLUSTER_MODE_SINGLECORE too as it is required by R5 core when it is being as general purpose core instead of device manager.

Signed-off-by Devarsh Thakkar <[email protected]>

---
V2:
- Fix indentation and ordering issues as per review comments
V3:
- Change CLUSTER_MODE_NONE value to -1
V4:
- No change
V5:
- No change (fixing typo in email address)
V6:
    - Use CLUSTER_MODE_SINGLECORE for AM62x
    - Set PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE for single core.
V7:
    - Simplify and rebase on top of base commit "[PATCH v7] remoteproc: k3-r5: Simplify cluster mode setting"

---
   drivers/remoteproc/ti_k3_r5_remoteproc.c | 59 +++++++++++++++++++-----
   1 file changed, 48 insertions(+), 11 deletions(-)
drivers/remoteproc/ti_k3_r5_remoteproc.c +2 -3
index c2ec0f432921..df32f6bc4325 100644
@@ -71,14 +71,16 @@ struct k3_r5_mem {
   /*
    * All cluster mode values are not applicable on all SoCs. The following
    * are the modes supported on various SoCs:
- *   Split mode      : AM65x, J721E, J7200 and AM64x SoCs
- *   LockStep mode   : AM65x, J721E and J7200 SoCs
- *   Single-CPU mode : AM64x SoCs only
+ *   Split mode       : AM65x, J721E, J7200 and AM64x SoCs
+ *   LockStep mode    : AM65x, J721E and J7200 SoCs
Quoted 8 lines

drivers/remoteproc/ti_k3_r5_remoteproc.c struct k3_r5_mem { · line 83

Quoted 8 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +3 -0
+ *   Single-CPU mode  : AM64x SoCs only
+ *   Single-Core mode : AM62x, AM62A SoCs
    */
   enum cluster_mode {
       CLUSTER_MODE_SPLIT = 0,
       CLUSTER_MODE_LOCKSTEP,
       CLUSTER_MODE_SINGLECPU,
+    CLUSTER_MODE_SINGLECORE

What is the difference in device driver behaviour between SINGLECPU and SINGLECORE?

Quoted 4 lines
Quoted 1 line

Yeah there is quite a bit of common code flow between the two but the fundamental difference is that you use CLUSTER_MODE_SINGLECPU when

I still didn't get what is the difference between the two from SW point of view. What happens if you just use CLUSTER_MODE_SINGLECPU for AM62 SoC?

Talking about mere functionality, I think there are some checks in driver which expect two cores for this (since CLUSTER_MODE_SINGLECPU is to select one out of two cores as mentioned earlier) but if those checks are handled it would be possible to use am64 compatible and achieve the functionality in am62 and I do remember running it with some hacks to bypass those checks during pre-silicon.

But this doesn't seem to be proper way since bindings say CLUSTER_MODE_SINGLECPU is for dual core scenarios as mentioned in DT-binding [1] :

"AM64x SoCs do not support LockStep mode, but rather a new non-safety mode called "Single-CPU" mode, where only Core0 is used, but with ability to use Core1's TCMs as well."

and to use it for single core scenarios would conflict above definition.

I had also discussed this in past offline with Suman who is the author of this driver and we aligned on not using CLUSTER_MODE_SINGLECPU for am62 due to above reasons.

[1]: https://gitlab.com/linux-kernel/linux-next/-/blob/next-20230328/Documentation/devicetree/bindings/remoteproc/ti%2Ck3-r5f-rproc.yaml#L20

Regards
Devarsh

Quoted 161 lines
Quoted 5 lines

you have two R5F cores but you want to use only single R5F core albeit with using TCM of both the cores whereas CLUSTER_MODE_SINGLECORE is for the scenario where you have single core R5F's only.

Quoted 152 lines

Also the bindings for CLUSTER_MODE_SINGLECPU are already upstream so did
not want to break them either : https://gitlab.com/linux-kernel/linux-next/-/blob/next-20230328/Documentation/devicetree/bindings/remoteproc/ti%2Ck3-r5f-rproc.yaml#L20.

Regards
Devarsh

Quoted 146 lines

If there is no difference then you should not introduce a new enum. >

Quoted 143 lines
Quoted 42 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +7 -2
   };
     /**

@@ -86,11 +88,13 @@ enum cluster_mode {
    * @tcm_is_double: flag to denote the larger unified TCMs in certain modes
    * @tcm_ecc_autoinit: flag to denote the auto-initialization of TCMs for ECC
    * @single_cpu_mode: flag to denote if SoC/IP supports Single-CPU mode
+ * @is_single_core: flag to denote if SoC/IP has only single core R5
    */
   struct k3_r5_soc_data {
       bool tcm_is_double;
       bool tcm_ecc_autoinit;
       bool single_cpu_mode;
+    bool is_single_core;
   };
     /**

@@ -838,7 +842,8 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
         core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem);
       if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
-        cluster->mode == CLUSTER_MODE_SINGLECPU) {
+        cluster->mode == CLUSTER_MODE_SINGLECPU ||
+        cluster->mode == CLUSTER_MODE_SINGLECORE) {
           core = core0;
       } else {
           core = kproc->core;

@@ -877,7 +882,8 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
            * with the bit configured, so program it only on
            * permitted cores
            */
-        if (cluster->mode == CLUSTER_MODE_SINGLECPU) {
+        if (cluster->mode == CLUSTER_MODE_SINGLECPU ||
+            cluster->mode == CLUSTER_MODE_SINGLECORE) {
               set_cfg = PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE;
           } else {
               /*

@@ -1069,6 +1075,7 @@ static void k3_r5_adjust_tcm_sizes(struct k3_r5_rproc *kproc)
         if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
           cluster->mode == CLUSTER_MODE_SINGLECPU ||
+        cluster->mode == CLUSTER_MODE_SINGLECORE ||
           !cluster->soc_data->tcm_is_double)
           return;
   @@ -1145,6 +1152,8 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc)
       if (cluster->soc_data->single_cpu_mode) {
Quoted 4 lines

          mode = cfg & PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE ? CLUSTER_MODE_SINGLECPU : CLUSTER_MODE_SPLIT;
+    } else if (cluster->soc_data->is_single_core) { +        mode = CLUSTER_MODE_SINGLECORE;

I have commented twice on this before - whether it is soc_data->single_cpu_mode or soc_data->is_single_core, I don't want to see them used elsewhere than in a single function.  Either in probe() or another function, use them once to set cluster->mode and never again.

I will silently drop any other patchset that doesn't address this.

Quoted 89 lines

      } else {
          mode = cfg & PROC_BOOT_CFG_FLAG_R5_LOCKSTEP ? CLUSTER_MODE_LOCKSTEP : CLUSTER_MODE_SPLIT;

drivers/remoteproc/ti_k3_r5_remoteproc.c +34 -4
@@ -1264,9 +1273,12 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
               goto err_add;
           }
   -        /* create only one rproc in lockstep mode or single-cpu mode */
+        /* create only one rproc in lockstep, single-cpu or
+         * single core mode
+         */
           if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
-            cluster->mode == CLUSTER_MODE_SINGLECPU)
+            cluster->mode == CLUSTER_MODE_SINGLECPU ||
+            cluster->mode == CLUSTER_MODE_SINGLECORE)
               break;
       }
   @@ -1709,19 +1721,33 @@ static int k3_r5_probe(struct platform_device *pdev)
           /*
            * default to most common efuse configurations - Split-mode on AM64x
            * and LockStep-mode on all others
+         * default to most common efuse configurations -
+         * Split-mode on AM64x
+         * Single core on AM62x
+         * LockStep-mode on all others
            */
-        cluster->mode = data->single_cpu_mode ?
+        if (!data->is_single_core)
+            cluster->mode = data->single_cpu_mode ?
                       CLUSTER_MODE_SPLIT : CLUSTER_MODE_LOCKSTEP;
+        else
+            cluster->mode = CLUSTER_MODE_SINGLECORE;
       }
   -    if (cluster->mode == CLUSTER_MODE_SINGLECPU && !data->single_cpu_mode) {
+    if  ((cluster->mode == CLUSTER_MODE_SINGLECPU && !data->single_cpu_mode) ||
+         (cluster->mode == CLUSTER_MODE_SINGLECORE && !data->is_single_core)) {
           dev_err(dev, "Cluster mode = %d is not supported on this SoC\n", cluster->mode);
           return -EINVAL;
       }
         num_cores = of_get_available_child_count(np);
-    if (num_cores != 2) {
-        dev_err(dev, "MCU cluster requires both R5F cores to be enabled, num_cores = %d\n",
+    if (num_cores != 2 && !data->is_single_core) {
+        dev_err(dev, "MCU cluster requires both R5F cores to be enabled but num_cores is set to = %d\n",
+            num_cores);
+        return -ENODEV;
+    }
+
+    if (num_cores != 1 && data->is_single_core) {
+        dev_err(dev, "SoC supports only single core R5 but num_cores is set to %d\n",
               num_cores);
           return -ENODEV;
       }

@@ -1763,18 +1789,28 @@ static const struct k3_r5_soc_data am65_j721e_soc_data = {
       .tcm_is_double = false,
       .tcm_ecc_autoinit = false,
       .single_cpu_mode = false,
+    .is_single_core = false,
   };
     static const struct k3_r5_soc_data j7200_j721s2_soc_data = {
       .tcm_is_double = true,
       .tcm_ecc_autoinit = true,
       .single_cpu_mode = false,
+    .is_single_core = false,
   };
     static const struct k3_r5_soc_data am64_soc_data = {
       .tcm_is_double = true,
       .tcm_ecc_autoinit = true,
       .single_cpu_mode = true,
+    .is_single_core = false,
+};
+
+static const struct k3_r5_soc_data am62_soc_data = {
+    .tcm_is_double = false,
+    .tcm_ecc_autoinit = true,
+    .single_cpu_mode = false,
+    .is_single_core = true,
   };
     static const struct of_device_id k3_r5_of_match[] = {

@@ -1782,6 +1818,7 @@ static const struct of_device_id k3_r5_of_match[] = {
       { .compatible = "ti,j721e-r5fss", .data = &am65_j721e_soc_data, },
       { .compatible = "ti,j7200-r5fss", .data = &j7200_j721s2_soc_data, },
       { .compatible = "ti,am64-r5fss",  .data = &am64_soc_data, },
+    { .compatible = "ti,am62-r5fss",  .data = &am62_soc_data, },
       { .compatible = "ti,j721s2-r5fss",  .data = &j7200_j721s2_soc_data, },
       { /* sentinel */ },
   };
Signature · 1 line
2.34.1

--
cheers,
-roger