[PATCH v7 0/3] Add single core R5F IPC for AM62 SoC family

[PATCH v7 0/3] Add single core R5F IPC for AM62 SoC family

[Devarsh Thakkar]

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 
-- 
2.34.1

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

[Devarsh Thakkar]

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 <devarsht@ti.com>
---
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(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 0481926c6975..c2ec0f432921 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -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",
-- 
2.34.1

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

[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 <devarsht@ti.com>
---
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(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
index fb9605f0655b..fcc3db97fe8f 100644
--- a/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
@@ -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
-- 
2.34.1

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

[Krzysztof Kozlowski]

On 10/03/2023 17:25, Devarsh Thakkar wrote:
> 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 <devarsht@ti.com>
> ---


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

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

[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 <devarsht@ti.com>
---
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(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index c2ec0f432921..df32f6bc4325 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -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 */ },
 };
-- 
2.34.1

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

[Mathieu Poirier]

On Fri, Mar 10, 2023 at 09:55:44PM +0530, Devarsh Thakkar wrote:
> 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 <devarsht@ti.com>
> ---
> 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(-)
> 
> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> index c2ec0f432921..df32f6bc4325 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -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;

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.

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

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

[Devarsh Thakkar]

Hi Mathieu,

Thanks for the review.
On 17/03/23 21:47, Mathieu Poirier wrote:
> On Fri, Mar 10, 2023 at 09:55:44PM +0530, Devarsh Thakkar wrote:
>> 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 <devarsht@ti.com>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> index c2ec0f432921..df32f6bc4325 100644
>> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> @@ -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;
> 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.

> I will silently drop any other patchset that doesn't address this.
>
>>  	} 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 */ },
>>  };
>> -- 
>> 2.34.1
>>

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

[Roger Quadros]

Hi Devarsh,

On 17/03/2023 18:17, Mathieu Poirier wrote:
> On Fri, Mar 10, 2023 at 09:55:44PM +0530, Devarsh Thakkar wrote:
>> 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 <devarsht@ti.com>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> index c2ec0f432921..df32f6bc4325 100644
>> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> @@ -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

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.

>>  };
>>  
>>  /**
>> @@ -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;
> 
> 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.
> 
>>  	} 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 */ },
>>  };
>> -- 
>> 2.34.1
>>

cheers,
-roger

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

[Devarsh Thakkar]

Hi Roger,

On 28/03/23 13:22, Roger Quadros wrote:
> Hi Devarsh,
> 
> On 17/03/2023 18:17, Mathieu Poirier wrote:
>> On Fri, Mar 10, 2023 at 09:55:44PM +0530, Devarsh Thakkar wrote:
>>> 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 <devarsht@ti.com>
>>> ---
>>> 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(-)
>>>
>>> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>>> index c2ec0f432921..df32f6bc4325 100644
>>> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
>>> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>>> @@ -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
> 
> 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

> If there is no difference then you should not introduce
> a new enum. >
>>>   };
>>>   
>>>   /**
>>> @@ -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;
>>
>> 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.
>>
>>>   	} 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 */ },
>>>   };
>>> -- 
>>> 2.34.1
>>>
> 
> cheers,
> -roger

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

[Roger Quadros]

On 28/03/2023 19:08, Devarsh Thakkar wrote:
> Hi Roger,
> 
> On 28/03/23 13:22, Roger Quadros wrote:
>> Hi Devarsh,
>>
>> On 17/03/2023 18:17, Mathieu Poirier wrote:
>>> On Fri, Mar 10, 2023 at 09:55:44PM +0530, Devarsh Thakkar wrote:
>>>> 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 <devarsht@ti.com>
>>>> ---
>>>> 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(-)
>>>>
>>>> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>>>> index c2ec0f432921..df32f6bc4325 100644
>>>> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
>>>> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>>>> @@ -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
>>
>> 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

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?

> 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
> 
>> If there is no difference then you should not introduce
>> a new enum. >
>>>>   };
>>>>     /**
>>>> @@ -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;
>>>
>>> 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.
>>>
>>>>       } 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 */ },
>>>>   };
>>>> -- 
>>>> 2.34.1
>>>>
>>

--
cheers,
-roger

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

[Devarsh Thakkar]

Hi Roger,

On 29/03/23 13:51, Roger Quadros wrote:
> 
> 
> On 28/03/2023 19:08, Devarsh Thakkar wrote:
>> Hi Roger,
>>
>> On 28/03/23 13:22, Roger Quadros wrote:
>>> Hi Devarsh,
>>>
>>> On 17/03/2023 18:17, Mathieu Poirier wrote:
>>>> On Fri, Mar 10, 2023 at 09:55:44PM +0530, Devarsh Thakkar wrote:
>>>>> 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 <devarsht@ti.com>
>>>>> ---
>>>>> 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(-)
>>>>>
>>>>> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>>>>> index c2ec0f432921..df32f6bc4325 100644
>>>>> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
>>>>> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>>>>> @@ -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
>>>
>>> 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
> 
> 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


> 
>> 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
>>
>>> If there is no difference then you should not introduce
>>> a new enum. >
>>>>>    };
>>>>>      /**
>>>>> @@ -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;
>>>>
>>>> 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.
>>>>
>>>>>        } 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 */ },
>>>>>    };
>>>>> -- 
>>>>> 2.34.1
>>>>>
>>>
> 
> --
> cheers,
> -roger