[PATCH v2 0/2] Add single core R5F IPC for AM62 SoC family

[PATCH v2 0/2] Add single core R5F IPC for AM62 SoC family

[Devarsh Thakkar]

AM62 SoC family don't have a R5F cluster, instead they have a single
core R5F. This enables IPC support with single core R5F for AM62
family of SoCs.

Devarsh Thakkar (2):
  Documentation: dt-bindings: k3-r5f-rproc: Add new compatible for AM62
    SoC family
  remoteproc: k3-r5: Use separate compatible string for TI AM62 SoC
    family

 .../bindings/remoteproc/ti,k3-r5f-rproc.yaml  | 48 +++++++++++-----
 drivers/remoteproc/ti_k3_r5_remoteproc.c      | 55 +++++++++++++++----
 2 files changed, 77 insertions(+), 26 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] Documentation: dt-bindings: k3-r5f-rproc: 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 don't allow cluster-mode
property usage in device-tree as this implies that there
is no R5F cluster available and only single R5F core
is present.

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V2: Avoid acronyms, use "Device Manager" instead of "DM"
---
 .../bindings/remoteproc/ti,k3-r5f-rproc.yaml  | 48 +++++++++++++------
 1 file changed, 34 insertions(+), 14 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..91357635025a 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
@@ -28,6 +31,9 @@ description: |
   the device management of the remote processor and to communicate with the
   remote processor.
 
+  Since AM62 SoC family only support a single core, there is no cluster-mode
+  property setting required for it.
+
 properties:
   $nodename:
     pattern: "^r5fss(@.*)?"
@@ -38,6 +44,7 @@ properties:
       - ti,j721e-r5fss
       - ti,j7200-r5fss
       - ti,am64-r5fss
+      - ti,am62-r5fss
       - ti,j721s2-r5fss
 
   power-domains:
@@ -80,7 +87,8 @@ 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,
+      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
@@ -104,6 +112,7 @@ patternProperties:
           - ti,j721e-r5f
           - ti,j7200-r5f
           - ti,am64-r5f
+          - ti,am62-r5f
           - ti,j721s2-r5f
 
       reg:
@@ -207,20 +216,31 @@ patternProperties:
       - firmware-name
 
     unevaluatedProperties: false
+allOf:
+  - if:
+      properties:
+        compatible:
+          enum:
+            - ti,am64-r5fss
+    then:
+      properties:
+        ti,cluster-mode:
+          enum: [0, 2]
+
+    else:
+      properties:
+        ti,cluster-mode:
+          enum: [0, 1]
+
+  - if:
+      properties:
+        compatible:
+          enum:
+            - ti,am62-r5fss
+    then:
+      properties:
+        ti,cluster-mode: false
 
-if:
-  properties:
-    compatible:
-      enum:
-        - ti,am64-r5fss
-then:
-  properties:
-    ti,cluster-mode:
-      enum: [0, 2]
-else:
-  properties:
-    ti,cluster-mode:
-      enum: [0, 1]
 
 required:
   - compatible
-- 
2.17.1

[PATCH v2 2/2] remoteproc: k3-r5: Use separate compatible string for TI AM62 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_NONE and use it when
compatible is set to ti,am62-r5fss.

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V2: Fix indentation and ordering issues as per review comments
---
 drivers/remoteproc/ti_k3_r5_remoteproc.c | 55 ++++++++++++++++++------
 1 file changed, 43 insertions(+), 12 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 0481926c6975..9698b29a0b56 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -74,11 +74,13 @@ struct k3_r5_mem {
  *   Split mode      : AM65x, J721E, J7200 and AM64x SoCs
  *   LockStep mode   : AM65x, J721E and J7200 SoCs
  *   Single-CPU mode : AM64x SoCs only
+ *   None            : AM62x, AM62A SoCs
  */
 enum cluster_mode {
 	CLUSTER_MODE_SPLIT = 0,
 	CLUSTER_MODE_LOCKSTEP,
 	CLUSTER_MODE_SINGLECPU,
+	CLUSTER_MODE_NONE,
 };
 
 /**
@@ -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_NONE) {
 		core = core0;
 	} else {
 		core = kproc->core;
@@ -853,7 +858,7 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
 		boot_vec, cfg, ctrl, stat);
 
 	/* check if only Single-CPU mode is supported on applicable SoCs */
-	if (cluster->soc_data->single_cpu_mode) {
+	if (cluster->soc_data->single_cpu_mode || cluster->soc_data->is_single_core) {
 		single_cpu =
 			!!(stat & PROC_BOOT_STATUS_FLAG_R5_SINGLECORE_ONLY);
 		if (single_cpu && cluster->mode == CLUSTER_MODE_SPLIT) {
@@ -1074,6 +1079,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_NONE ||
 	    !cluster->soc_data->tcm_is_double)
 		return;
 
@@ -1147,7 +1153,9 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc)
 	atcm_enable = cfg & PROC_BOOT_CFG_FLAG_R5_ATCM_EN ?  1 : 0;
 	btcm_enable = cfg & PROC_BOOT_CFG_FLAG_R5_BTCM_EN ?  1 : 0;
 	loczrama = cfg & PROC_BOOT_CFG_FLAG_R5_TCM_RSTBASE ?  1 : 0;
-	if (cluster->soc_data->single_cpu_mode) {
+	if (cluster->soc_data->is_single_core) {
+		mode = CLUSTER_MODE_NONE;
+	} else if (cluster->soc_data->single_cpu_mode) {
 		mode = cfg & PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE ?
 				CLUSTER_MODE_SINGLECPU : CLUSTER_MODE_SPLIT;
 	} else {
@@ -1271,7 +1279,8 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 
 		/* create only one rproc in lockstep mode or single-cpu mode */
 		if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
-		    cluster->mode == CLUSTER_MODE_SINGLECPU)
+		    cluster->mode == CLUSTER_MODE_SINGLECPU ||
+		    cluster->mode == CLUSTER_MODE_NONE)
 			break;
 	}
 
@@ -1704,21 +1713,32 @@ 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
 	 */
-	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_NONE;
+
 	cluster->soc_data = data;
 	INIT_LIST_HEAD(&cluster->cores);
 
-	ret = of_property_read_u32(np, "ti,cluster-mode", &cluster->mode);
-	if (ret < 0 && ret != -EINVAL) {
-		dev_err(dev, "invalid format for ti,cluster-mode, ret = %d\n",
-			ret);
-		return ret;
+	if (!data->is_single_core) {
+		ret = of_property_read_u32(np, "ti,cluster-mode", &cluster->mode);
+		if (ret < 0 && ret != -EINVAL) {
+			dev_err(dev, "invalid format for ti,cluster-mode, ret = %d\n", ret);
+			return ret;
+		}
 	}
 
 	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;
 	}
@@ -1760,18 +1780,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[] = {
@@ -1779,6 +1809,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.17.1

Re: [PATCH v2 1/2] Documentation: dt-bindings: k3-r5f-rproc: Add new compatible for AM62 SoC family

[Krzysztof Kozlowski]

On 30/11/2022 14:40, 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 don't allow cluster-mode
> property usage in device-tree as this implies that there
> is no R5F cluster available and only single R5F core
> is present.
> 
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
> V2: Avoid acronyms, use "Device Manager" instead of "DM"

Use subject prefixes matching the subsystem (git log --oneline -- ...).

> ---
>  .../bindings/remoteproc/ti,k3-r5f-rproc.yaml  | 48 +++++++++++++------
>  1 file changed, 34 insertions(+), 14 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..91357635025a 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
> @@ -28,6 +31,9 @@ description: |
>    the device management of the remote processor and to communicate with the
>    remote processor.
>  
> +  Since AM62 SoC family only support a single core, there is no cluster-mode
> +  property setting required for it.
> +
>  properties:
>    $nodename:
>      pattern: "^r5fss(@.*)?"
> @@ -38,6 +44,7 @@ properties:
>        - ti,j721e-r5fss
>        - ti,j7200-r5fss
>        - ti,am64-r5fss
> +      - ti,am62-r5fss

Some order? Alphabetical, so before am64? Same in other places.


>        - ti,j721s2-r5fss
>  
>    power-domains:
> @@ -80,7 +87,8 @@ 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,
> +      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
> @@ -104,6 +112,7 @@ patternProperties:
>            - ti,j721e-r5f
>            - ti,j7200-r5f
>            - ti,am64-r5f
> +          - ti,am62-r5f
>            - ti,j721s2-r5f
>  
>        reg:
> @@ -207,20 +216,31 @@ patternProperties:
>        - firmware-name
>  
>      unevaluatedProperties: false

Blank line.

> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - ti,am64-r5fss
> +    then:
> +      properties:
> +        ti,cluster-mode:
> +          enum: [0, 2]
> +
> +    else:
> +      properties:
> +        ti,cluster-mode:

It's not really valid anymore for ti,am62-r5fss, so this cannot be
simple "else".  Instead you need to list all compatibles.

> +          enum: [0, 1]
> +
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - ti,am62-r5fss
> +    then:
> +      properties:
> +        ti,cluster-mode: false
>  
> -if:
> -  properties:
> -    compatible:
> -      enum:
> -        - ti,am64-r5fss
> -then:
> -  properties:
> -    ti,cluster-mode:
> -      enum: [0, 2]
> -else:
> -  properties:
> -    ti,cluster-mode:
> -      enum: [0, 1]
>  
>  required:
>    - compatible

Best regards,
Krzysztof

Re: [PATCH v2 2/2] remoteproc: k3-r5: Use separate compatible string for TI AM62 SoC family

[Tanmay Shah]

Hi Devarsh,

Please find my comments below.

On 11/30/22 6:40 PM, Devarsh Thakkar wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> 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_NONE and use it when
> compatible is set to ti,am62-r5fss.
>
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
> V2: Fix indentation and ordering issues as per review comments
> ---
>   drivers/remoteproc/ti_k3_r5_remoteproc.c | 55 ++++++++++++++++++------
>   1 file changed, 43 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> index 0481926c6975..9698b29a0b56 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -74,11 +74,13 @@ struct k3_r5_mem {
>    *   Split mode      : AM65x, J721E, J7200 and AM64x SoCs
>    *   LockStep mode   : AM65x, J721E and J7200 SoCs
>    *   Single-CPU mode : AM64x SoCs only
> + *   None            : AM62x, AM62A SoCs
>    */
>   enum cluster_mode {
>          CLUSTER_MODE_SPLIT = 0,
>          CLUSTER_MODE_LOCKSTEP,
>          CLUSTER_MODE_SINGLECPU,
> +       CLUSTER_MODE_NONE,
>   };
>
>   /**
> @@ -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;


If you are providing this data, then ignore parsing cluster-mode 
property. This will make change very simple.

I believe this would save you any modification in bindings as well as 
cluster-mode property is optional anyway.

Also, "enum cluster_mode" reflects cluster-mode values from bindings 
document except proper soc compatible. I don't see new value added in 
bindings document i.e. only

[0 -> split, 1 -> lockstep, 2 -> single cpu] are defined. If new enum is 
introduced in driver, it is expected to reflect in bindings i.e. [3 -> 
cluster-mode none] to avoid any confusion.

I believe it is duplicate logic if you are providing "is_single_core" 
information here and introduce CLUSTER_MODE_NONE as well.

May be I am missing something, but I don't see any use of providing 
extra value CLUSTER_MODE_NONE if "is_single_core" is set in the driver. 
So, simple solutions is just to avoid parsing cluster-mode property if 
is_single_core is set in the driver. Hope this helps.


Thanks,

Tanmay


>   };
>
>   /**
> @@ -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_NONE) {
>                  core = core0;
>          } else {
>                  core = kproc->core;
> @@ -853,7 +858,7 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
>                  boot_vec, cfg, ctrl, stat);
>
>          /* check if only Single-CPU mode is supported on applicable SoCs */
> -       if (cluster->soc_data->single_cpu_mode) {
> +       if (cluster->soc_data->single_cpu_mode || cluster->soc_data->is_single_core) {
>                  single_cpu =
>                          !!(stat & PROC_BOOT_STATUS_FLAG_R5_SINGLECORE_ONLY);
>                  if (single_cpu && cluster->mode == CLUSTER_MODE_SPLIT) {
> @@ -1074,6 +1079,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_NONE ||
>              !cluster->soc_data->tcm_is_double)
>                  return;
>
> @@ -1147,7 +1153,9 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc)
>          atcm_enable = cfg & PROC_BOOT_CFG_FLAG_R5_ATCM_EN ?  1 : 0;
>          btcm_enable = cfg & PROC_BOOT_CFG_FLAG_R5_BTCM_EN ?  1 : 0;
>          loczrama = cfg & PROC_BOOT_CFG_FLAG_R5_TCM_RSTBASE ?  1 : 0;
> -       if (cluster->soc_data->single_cpu_mode) {
> +       if (cluster->soc_data->is_single_core) {
> +               mode = CLUSTER_MODE_NONE;
> +       } else if (cluster->soc_data->single_cpu_mode) {
>                  mode = cfg & PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE ?
>                                  CLUSTER_MODE_SINGLECPU : CLUSTER_MODE_SPLIT;
>          } else {
> @@ -1271,7 +1279,8 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>
>                  /* create only one rproc in lockstep mode or single-cpu mode */
>                  if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
> -                   cluster->mode == CLUSTER_MODE_SINGLECPU)
> +                   cluster->mode == CLUSTER_MODE_SINGLECPU ||
> +                   cluster->mode == CLUSTER_MODE_NONE)
>                          break;
>          }
>
> @@ -1704,21 +1713,32 @@ 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
>           */
> -       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_NONE;
> +
>          cluster->soc_data = data;
>          INIT_LIST_HEAD(&cluster->cores);
>
> -       ret = of_property_read_u32(np, "ti,cluster-mode", &cluster->mode);
> -       if (ret < 0 && ret != -EINVAL) {
> -               dev_err(dev, "invalid format for ti,cluster-mode, ret = %d\n",
> -                       ret);
> -               return ret;
> +       if (!data->is_single_core) {
> +               ret = of_property_read_u32(np, "ti,cluster-mode", &cluster->mode);
> +               if (ret < 0 && ret != -EINVAL) {
> +                       dev_err(dev, "invalid format for ti,cluster-mode, ret = %d\n", ret);
> +                       return ret;
> +               }
>          }
>
>          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;
>          }
> @@ -1760,18 +1780,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[] = {
> @@ -1779,6 +1809,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.17.1
>

Re: [PATCH v2 2/2] remoteproc: k3-r5: Use separate compatible string for TI AM62 SoC family

[Devarsh Thakkar]

Hi Tanmay,

Thanks for the review and sorry for the delay. Please find my response 
inline.
On 30/11/22 23:27, Tanmay Shah wrote:
> Hi Devarsh,
> 
> Please find my comments below.
> 
> On 11/30/22 6:40 PM, 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_NONE and use it when
>> compatible is set to ti,am62-r5fss.
>>
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> ---
>> V2: Fix indentation and ordering issues as per review comments
>> ---
>>   drivers/remoteproc/ti_k3_r5_remoteproc.c | 55 ++++++++++++++++++------
>>   1 file changed, 43 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
>> b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> index 0481926c6975..9698b29a0b56 100644
>> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> @@ -74,11 +74,13 @@ struct k3_r5_mem {
>>    *   Split mode      : AM65x, J721E, J7200 and AM64x SoCs
>>    *   LockStep mode   : AM65x, J721E and J7200 SoCs
>>    *   Single-CPU mode : AM64x SoCs only
>> + *   None            : AM62x, AM62A SoCs
>>    */
>>   enum cluster_mode {
>>          CLUSTER_MODE_SPLIT = 0,
>>          CLUSTER_MODE_LOCKSTEP,
>>          CLUSTER_MODE_SINGLECPU,
>> +       CLUSTER_MODE_NONE,
>>   };
>>
>>   /**
>> @@ -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;
> 
> 
> If you are providing this data, then ignore parsing cluster-mode 
> property. This will make change very simple.
Yes, I think we are doing the same thing here. AM62x is modeled as a 
cluster with a single core child but since it is single core there is no 
cluster mode applicable as such and hence no cluster-mode required to be 
set in device-tree for AM62x.


> I believe this would save you any modification in bindings as well as 
> cluster-mode property is optional anyway.
> 
> Also, "enum cluster_mode" reflects cluster-mode values from bindings 
> document except proper soc compatible. I don't see new value added in 
> bindings document i.e. only
> 
> [0 -> split, 1 -> lockstep, 2 -> single cpu] are defined. If new enum is 
> introduced in driver, it is expected to reflect in bindings i.e. [3 -> 
> cluster-mode none] to avoid any confusion.
To support backward compatibility we introduced CLUSTER_MODE_NONE at 3, 
but I think we can use -1 index and maybe another name say 
CLUSTER_MODE_INVALID to make it less confusing. The cluster-mode 
property doesn't apply to AM62x since it uses CLUSTER_MODE_INVALID, my 
understanding is we don't need to describe CLUSTER_MODE_INVALID in 
dt-binding since it will be only used internally by driver as -1 and need
not be set at all in device-tree since for AM62x there won't be any 
cluster-mode property required to be set in the devicetree.

> 
> I believe it is duplicate logic if you are providing "is_single_core" 
> information here and introduce CLUSTER_MODE_NONE as well.
> 
> May be I am missing something, but I don't see any use of providing 
> extra value CLUSTER_MODE_NONE if "is_single_core" is set in the driver. 
> So, simple solutions is just to avoid parsing cluster-mode property if 
> is_single_core is set in the driver. Hope this helps.
Fair point, we could have used soc data's is_single_core check instead 
of adding a new enum and used that check and that would have worked too. 
But in that case, cluster-mode by default would be set to 0 with  as 
part of allocation of k3_r5_cluster struct during probe which would 
imply incorrectly CLUSTER_MODE_SPLIT for AM62x. I think it is better
to have another enum say CLUSTER_MODE_INVALID and use it for SoC's like 
AM62x to make it less confusing.

Regards,
Devarsh
> 
> 
> Thanks,
> 
> Tanmay
> 
> 
>>   };
>>
>>   /**
>> @@ -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_NONE) {
>>                  core = core0;
>>          } else {
>>                  core = kproc->core;
>> @@ -853,7 +858,7 @@ static int k3_r5_rproc_configure(struct 
>> k3_r5_rproc *kproc)
>>                  boot_vec, cfg, ctrl, stat);
>>
>>          /* check if only Single-CPU mode is supported on applicable 
>> SoCs */
>> -       if (cluster->soc_data->single_cpu_mode) {
>> +       if (cluster->soc_data->single_cpu_mode || 
>> cluster->soc_data->is_single_core) {
>>                  single_cpu =
>>                          !!(stat & 
>> PROC_BOOT_STATUS_FLAG_R5_SINGLECORE_ONLY);
>>                  if (single_cpu && cluster->mode == CLUSTER_MODE_SPLIT) {
>> @@ -1074,6 +1079,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_NONE ||
>>              !cluster->soc_data->tcm_is_double)
>>                  return;
>>
>> @@ -1147,7 +1153,9 @@ static int k3_r5_rproc_configure_mode(struct 
>> k3_r5_rproc *kproc)
>>          atcm_enable = cfg & PROC_BOOT_CFG_FLAG_R5_ATCM_EN ?  1 : 0;
>>          btcm_enable = cfg & PROC_BOOT_CFG_FLAG_R5_BTCM_EN ?  1 : 0;
>>          loczrama = cfg & PROC_BOOT_CFG_FLAG_R5_TCM_RSTBASE ?  1 : 0;
>> -       if (cluster->soc_data->single_cpu_mode) {
>> +       if (cluster->soc_data->is_single_core) {
>> +               mode = CLUSTER_MODE_NONE;
>> +       } else if (cluster->soc_data->single_cpu_mode) {
>>                  mode = cfg & PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE ?
>>                                  CLUSTER_MODE_SINGLECPU : 
>> CLUSTER_MODE_SPLIT;
>>          } else {
>> @@ -1271,7 +1279,8 @@ static int k3_r5_cluster_rproc_init(struct 
>> platform_device *pdev)
>>
>>                  /* create only one rproc in lockstep mode or 
>> single-cpu mode */
>>                  if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
>> -                   cluster->mode == CLUSTER_MODE_SINGLECPU)
>> +                   cluster->mode == CLUSTER_MODE_SINGLECPU ||
>> +                   cluster->mode == CLUSTER_MODE_NONE)
>>                          break;
>>          }
>>
>> @@ -1704,21 +1713,32 @@ 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
>>           */
>> -       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_NONE;
>> +
>>          cluster->soc_data = data;
>>          INIT_LIST_HEAD(&cluster->cores);
>>
>> -       ret = of_property_read_u32(np, "ti,cluster-mode", 
>> &cluster->mode);
>> -       if (ret < 0 && ret != -EINVAL) {
>> -               dev_err(dev, "invalid format for ti,cluster-mode, ret 
>> = %d\n",
>> -                       ret);
>> -               return ret;
>> +       if (!data->is_single_core) {
>> +               ret = of_property_read_u32(np, "ti,cluster-mode", 
>> &cluster->mode);
>> +               if (ret < 0 && ret != -EINVAL) {
>> +                       dev_err(dev, "invalid format for 
>> ti,cluster-mode, ret = %d\n", ret);
>> +                       return ret;
>> +               }
>>          }
>>
>>          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;
>>          }
>> @@ -1760,18 +1780,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[] = {
>> @@ -1779,6 +1809,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.17.1
>>

Re: [PATCH v2 1/2] Documentation: dt-bindings: k3-r5f-rproc: Add new compatible for AM62 SoC family

[Devarsh Thakkar]

Hi Krzysztof,

Thanks for the review. Please find my response inline.

On 30/11/22 20:33, Krzysztof Kozlowski wrote:
> On 30/11/2022 14:40, 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 don't allow cluster-mode
>> property usage in device-tree as this implies that there
>> is no R5F cluster available and only single R5F core
>> is present.
>>
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> ---
>> V2: Avoid acronyms, use "Device Manager" instead of "DM"
> 
> Use subject prefixes matching the subsystem (git log --oneline -- ...).
Agreed, I will update the prefix as dt-bindings: remoteproc: k3-r5f: in V3.
> 
>> ---
>>  .../bindings/remoteproc/ti,k3-r5f-rproc.yaml  | 48 +++++++++++++------
>>  1 file changed, 34 insertions(+), 14 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..91357635025a 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
>> @@ -28,6 +31,9 @@ description: |
>>    the device management of the remote processor and to communicate with the
>>    remote processor.
>>  
>> +  Since AM62 SoC family only support a single core, there is no cluster-mode
>> +  property setting required for it.
>> +
>>  properties:
>>    $nodename:
>>      pattern: "^r5fss(@.*)?"
>> @@ -38,6 +44,7 @@ properties:
>>        - ti,j721e-r5fss
>>        - ti,j7200-r5fss
>>        - ti,am64-r5fss
>> +      - ti,am62-r5fss
> 
> Some order? Alphabetical, so before am64? Same in other places.
Agreed, I will update in V3 accordingly.
> 
> 
>>        - ti,j721s2-r5fss
>>  
>>    power-domains:
>> @@ -80,7 +87,8 @@ 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,
>> +      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
>> @@ -104,6 +112,7 @@ patternProperties:
>>            - ti,j721e-r5f
>>            - ti,j7200-r5f
>>            - ti,am64-r5f
>> +          - ti,am62-r5f
>>            - ti,j721s2-r5f
>>  
>>        reg:
>> @@ -207,20 +216,31 @@ patternProperties:
>>        - firmware-name
>>  
>>      unevaluatedProperties: false
> 
> Blank line.
Agreed, I will remove it in V3.
> 
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          enum:
>> +            - ti,am64-r5fss
>> +    then:
>> +      properties:
>> +        ti,cluster-mode:
>> +          enum: [0, 2]
>> +
>> +    else:
>> +      properties:
>> +        ti,cluster-mode:
> 
> It's not really valid anymore for ti,am62-r5fss, so this cannot be
> simple "else".  Instead you need to list all compatibles.
I agree that the else block is not valid for am62x, but my understanding is that since all the blocks under allOf are checked for validity,
I thought to add a separate if block only for am62x to set cluster-mode to false [1], which I believe would negate the effect of above else condition for am62x,
so that we don't have to list all compatibles under separate if blocks.

Just to verify this, I deliberately set cluster-mode=1 in am62x devicetree and then ran a dtbs-check and got below log : 
"linux-next/arch/arm64/boot/dts/ti/k3-am625-sk.dtb: r5fss@78000000: ti,cluster-mode: False schema does not allow [[1]]"

and above warning log goes away when i remove the cluster-mode node in am62x devicetree.
But please do let me know if I am missing something here or there is a better/more proper way to do this.

Best Regards,
Devarsh

> 
>> +          enum: [0, 1]
>> +
[1]
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          enum:
>> +            - ti,am62-r5fss
>> +    then:
>> +      properties:
>> +        ti,cluster-mode: false
>>  
>> -if:
>> -  properties:
>> -    compatible:
>> -      enum:
>> -        - ti,am64-r5fss
>> -then:
>> -  properties:
>> -    ti,cluster-mode:
>> -      enum: [0, 2]
>> -else:
>> -  properties:
>> -    ti,cluster-mode:
>> -      enum: [0, 1]
>>  
>>  required:
>>    - compatible
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 1/2] Documentation: dt-bindings: k3-r5f-rproc: Add new compatible for AM62 SoC family

[Krzysztof Kozlowski]

On 21/12/2022 08:42, Devarsh Thakkar wrote:
> Hi Krzysztof,
> 
> Thanks for the review. Please find my response inline.
> 
> On 30/11/22 20:33, Krzysztof Kozlowski wrote:
>> On 30/11/2022 14:40, 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 don't allow cluster-mode
>>> property usage in device-tree as this implies that there
>>> is no R5F cluster available and only single R5F core
>>> is present.
>>>
>>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>>> ---
>>> V2: Avoid acronyms, use "Device Manager" instead of "DM"
>>
>> Use subject prefixes matching the subsystem (git log --oneline -- ...).
> Agreed, I will update the prefix as dt-bindings: remoteproc: k3-r5f: in V3.
>>
>>> ---
>>>  .../bindings/remoteproc/ti,k3-r5f-rproc.yaml  | 48 +++++++++++++------
>>>  1 file changed, 34 insertions(+), 14 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..91357635025a 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
>>> @@ -28,6 +31,9 @@ description: |
>>>    the device management of the remote processor and to communicate with the
>>>    remote processor.
>>>  
>>> +  Since AM62 SoC family only support a single core, there is no cluster-mode
>>> +  property setting required for it.
>>> +
>>>  properties:
>>>    $nodename:
>>>      pattern: "^r5fss(@.*)?"
>>> @@ -38,6 +44,7 @@ properties:
>>>        - ti,j721e-r5fss
>>>        - ti,j7200-r5fss
>>>        - ti,am64-r5fss
>>> +      - ti,am62-r5fss
>>
>> Some order? Alphabetical, so before am64? Same in other places.
> Agreed, I will update in V3 accordingly.
>>
>>
>>>        - ti,j721s2-r5fss
>>>  
>>>    power-domains:
>>> @@ -80,7 +87,8 @@ 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,
>>> +      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
>>> @@ -104,6 +112,7 @@ patternProperties:
>>>            - ti,j721e-r5f
>>>            - ti,j7200-r5f
>>>            - ti,am64-r5f
>>> +          - ti,am62-r5f
>>>            - ti,j721s2-r5f
>>>  
>>>        reg:
>>> @@ -207,20 +216,31 @@ patternProperties:
>>>        - firmware-name
>>>  
>>>      unevaluatedProperties: false
>>
>> Blank line.
> Agreed, I will remove it in V3.
>>
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          enum:
>>> +            - ti,am64-r5fss
>>> +    then:
>>> +      properties:
>>> +        ti,cluster-mode:
>>> +          enum: [0, 2]
>>> +
>>> +    else:
>>> +      properties:
>>> +        ti,cluster-mode:
>>
>> It's not really valid anymore for ti,am62-r5fss, so this cannot be
>> simple "else".  Instead you need to list all compatibles.
> I agree that the else block is not valid for am62x, but my understanding is that since all the blocks under allOf are checked for validity,
> I thought to add a separate if block only for am62x to set cluster-mode to false [1], which I believe would negate the effect of above else condition for am62x,
> so that we don't have to list all compatibles under separate if blocks.
> 
> Just to verify this, I deliberately set cluster-mode=1 in am62x devicetree and then ran a dtbs-check and got below log : 
> "linux-next/arch/arm64/boot/dts/ti/k3-am625-sk.dtb: r5fss@78000000: ti,cluster-mode: False schema does not allow [[1]]"
> 
> and above warning log goes away when i remove the cluster-mode node in am62x devicetree.
> But please do let me know if I am missing something here or there is a better/more proper way to do this.

This was three weeks ago, so hundreds of patches ago, I don't remember
anymore.

Just look at your patch - it is clearly incorrect. You said in the patch
that for compatibles other than ti,am64-r5fss cluster mode is BOTH [0,
1] AND false.

I gave you the way to fix it. Feel free to fix it other ways if it gives
correct result.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] Documentation: dt-bindings: k3-r5f-rproc: Add new compatible for AM62 SoC family

[Devarsh Thakkar]

Hi Krzysztof,

On 21/12/22 15:06, Krzysztof Kozlowski wrote:
> On 21/12/2022 08:42, Devarsh Thakkar wrote:
>> Hi Krzysztof,
>>
>> Thanks for the review. Please find my response inline.
>>
>> On 30/11/22 20:33, Krzysztof Kozlowski wrote:
>>> On 30/11/2022 14:40, 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 don't allow cluster-mode
>>>> property usage in device-tree as this implies that there
>>>> is no R5F cluster available and only single R5F core
>>>> is present.
>>>>
>>>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>>>> ---
>>>> V2: Avoid acronyms, use "Device Manager" instead of "DM"
>>>
>>> Use subject prefixes matching the subsystem (git log --oneline -- ...).
>> Agreed, I will update the prefix as dt-bindings: remoteproc: k3-r5f: in V3.
>>>
>>>> ---
>>>>  .../bindings/remoteproc/ti,k3-r5f-rproc.yaml  | 48 +++++++++++++------
>>>>  1 file changed, 34 insertions(+), 14 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..91357635025a 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
>>>> @@ -28,6 +31,9 @@ description: |
>>>>    the device management of the remote processor and to communicate with the
>>>>    remote processor.
>>>>  
>>>> +  Since AM62 SoC family only support a single core, there is no cluster-mode
>>>> +  property setting required for it.
>>>> +
>>>>  properties:
>>>>    $nodename:
>>>>      pattern: "^r5fss(@.*)?"
>>>> @@ -38,6 +44,7 @@ properties:
>>>>        - ti,j721e-r5fss
>>>>        - ti,j7200-r5fss
>>>>        - ti,am64-r5fss
>>>> +      - ti,am62-r5fss
>>>
>>> Some order? Alphabetical, so before am64? Same in other places.
>> Agreed, I will update in V3 accordingly.
>>>
>>>
>>>>        - ti,j721s2-r5fss
>>>>  
>>>>    power-domains:
>>>> @@ -80,7 +87,8 @@ 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,
>>>> +      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
>>>> @@ -104,6 +112,7 @@ patternProperties:
>>>>            - ti,j721e-r5f
>>>>            - ti,j7200-r5f
>>>>            - ti,am64-r5f
>>>> +          - ti,am62-r5f
>>>>            - ti,j721s2-r5f
>>>>  
>>>>        reg:
>>>> @@ -207,20 +216,31 @@ patternProperties:
>>>>        - firmware-name
>>>>  
>>>>      unevaluatedProperties: false
>>>
>>> Blank line.
>> Agreed, I will remove it in V3.
>>>
>>>> +allOf:
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          enum:
>>>> +            - ti,am64-r5fss
>>>> +    then:
>>>> +      properties:
>>>> +        ti,cluster-mode:
>>>> +          enum: [0, 2]
>>>> +
>>>> +    else:
>>>> +      properties:
>>>> +        ti,cluster-mode:
>>>
>>> It's not really valid anymore for ti,am62-r5fss, so this cannot be
>>> simple "else".  Instead you need to list all compatibles.
>> I agree that the else block is not valid for am62x, but my understanding is that since all the blocks under allOf are checked for validity,
>> I thought to add a separate if block only for am62x to set cluster-mode to false [1], which I believe would negate the effect of above else condition for am62x,
>> so that we don't have to list all compatibles under separate if blocks.
>>
>> Just to verify this, I deliberately set cluster-mode=1 in am62x devicetree and then ran a dtbs-check and got below log : 
[2]
>> "linux-next/arch/arm64/boot/dts/ti/k3-am625-sk.dtb: r5fss@78000000: ti,cluster-mode: False schema does not allow [[1]]"
>>
>> and above warning log goes away when i remove the cluster-mode node in am62x devicetree.
>> But please do let me know if I am missing something here or there is a better/more proper way to do this.
> 
> This was three weeks ago, so hundreds of patches ago, I don't remember
> anymore.
My apologies for the delay.
> 
> Just look at your patch - it is clearly incorrect. You said in the patch
> that for compatibles other than ti,am64-r5fss cluster mode is BOTH [0,
> 1] AND false.

cluster-mode is BOTH [0,1] and false only in case of AM62x as per below snippet, but since it's under allOf the impact of latter will supersede, schema validation will fail even if cluster-mode set to 0 or 1 for am62x due to below snippet as shared in obesrvation log above [2]. 

"  - if:
      properties:
        compatible:
          enum:
            - ti,am62-r5fss
    then:
      properties:
        ti,cluster-mode: false"

Sorry for the back and forth, I just thought to describe more clearly what I was up-to as I thought above should be functionally fine and it also saves us from having separate if blocks for each compatible, but I am open to adding separate if blocks as you earlier suggested if that seems more cleaner solution.

Best Regards,
Devarsh
> 
> I gave you the way to fix it. Feel free to fix it other ways if it gives
> correct result.
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 1/2] Documentation: dt-bindings: k3-r5f-rproc: Add new compatible for AM62 SoC family

[Krzysztof Kozlowski]

On 21/12/2022 17:29, Devarsh Thakkar wrote:
>>
>> Just look at your patch - it is clearly incorrect. You said in the patch
>> that for compatibles other than ti,am64-r5fss cluster mode is BOTH [0,
>> 1] AND false.
> 
> cluster-mode is BOTH [0,1] and false only in case of AM62x as per below snippet

Yes, for that variant you have conflicting approach.

, but since it's under allOf the impact of latter will supersede, schema
validation will fail even if cluster-mode set to 0 or 1 for am62x due to
below snippet as shared in obesrvation log above [2].

Yeah, but the code is confusing. So again - you are saying with allOf
that both conditions are applicable. Your intentions of superseding do
not matter here - you said that allOf conditions must be taken into
account. These conditions can be reversed any time, don't you think?


> 
> "  - if:
>       properties:
>         compatible:
>           enum:
>             - ti,am62-r5fss
>     then:
>       properties:
>         ti,cluster-mode: false"
> 
> Sorry for the back and forth, I just thought to describe more clearly what I was up-to as I thought above should be functionally fine and it also saves us from having separate if blocks for each compatible, but I am open to adding separate if blocks as you earlier suggested if that seems more cleaner solution.

You need to fix your email client to properly wrap messages.

Best regards,
Krzysztof