Skip to contentSkip to conversation

Add single core R5F IPC for AM62 SoC family

PATCH Superseded Devarsh Thakkar 3 participants 10 messages Nov 30, 2022 – Dec 21, 2022

Subsystems (2)

v2 of 8 Superseded Latest is v8 →

lore
Share & tools
lore
#0 lore raw

v2 cover Add single core R5F IPC for AM62 SoC family

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(-)
Signature · 1 line
2.17.1
#1 lore raw

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

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.

---
V2: Avoid acronyms, use "Device Manager" instead of "DM"

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

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

Quoted 4 lines · Devarsh Thakkar

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

Quoted 8 lines

When this new compatible is used 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.

---
V2: Avoid acronyms, use "Device Manager" instead of "DM"

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

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

Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml properties: · line 47

Quoted 4 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +1 -0
@@ -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.

Quoted 22 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +3 -1
       - 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:

Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml patternProperties: · line 218

Quoted 3 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +0 -0
@@ -207,20 +216,31 @@ patternProperties:
       - firmware-name
 
     unevaluatedProperties: false

Blank line.

Quoted 4 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +4 -0
+allOf:
+  - if:
+      properties:
+        compatible:

Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml patternProperties: · line 232

Quoted 10 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +10 -0
+          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.

Quoted 17 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +10 -6
+          enum: [0, 1]
+
+  - if:
+      properties:
+        compatible:
+          enum:
+            - ti,am62-r5fss
+    then:
+      properties:
+        ti,cluster-mode: false
 
-if:
-  properties:
-    compatible:
-      enum:
-        - ti,am64-r5fss
-then:

Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml patternProperties: · line 246

Quoted 10 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +0 -7
-  properties:
-    ti,cluster-mode:
-      enum: [0, 2]
-else:
-  properties:
-    ti,cluster-mode:
-      enum: [0, 1]
 
 required:
   - compatible

Best regards,
Krzysztof

#3 lore raw

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

Hi Krzysztof,

Thanks for the review. Please find my response inline.

Quoted 10 lines · Krzysztof Kozlowski
Quoted 9 lines · Devarsh Thakkar

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

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.

Quoted 5 lines
Quoted 3 lines

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

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

Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml properties: · line 47

Quoted 11 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +4 -0
+  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.

Quoted 22 lines
Quoted 22 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +3 -1
       - 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
 
Quoted 7 lines

Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml patternProperties: · line 218

Quoted 5 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +0 -0
       reg:
@@ -207,20 +216,31 @@ patternProperties:
       - firmware-name
 
     unevaluatedProperties: false

Blank line.

Agreed, I will remove it in V3.

Quoted 10 lines
Quoted 10 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +9 -0
+allOf:
+  - if:
+      properties:
+        compatible:
+          enum:
+            - ti,am64-r5fss
+    then:
+      properties:
+        ti,cluster-mode:
Quoted 8 lines

Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml patternProperties: · line 232

Quoted 5 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +5 -0
+          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

Quoted 3 lines

Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml patternProperties: · line 234

Quoted 3 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +2 -0
+          enum: [0, 1]
+

[1]

Quoted 29 lines
Quoted 15 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +8 -6
+  - if:
+      properties:
+        compatible:
+          enum:
+            - ti,am62-r5fss
+    then:
+      properties:
+        ti,cluster-mode: false
 
-if:
-  properties:
-    compatible:
-      enum:
-        - ti,am64-r5fss
-then:

Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml patternProperties: · line 246

Quoted 10 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +0 -7
-  properties:
-    ti,cluster-mode:
-      enum: [0, 2]
-else:
-  properties:
-    ti,cluster-mode:
-      enum: [0, 1]
 
 required:
   - compatible

Best regards,
Krzysztof

#4 lore raw

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

Quoted 110 lines · Devarsh Thakkar

Hi Krzysztof,

Thanks for the review. Please find my response inline.

Quoted 10 lines · Krzysztof Kozlowski
Quoted 9 lines · Devarsh Thakkar

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

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.

Quoted 5 lines
Quoted 3 lines

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

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

Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml properties: · line 47

Quoted 11 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +4 -0
+  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.

Quoted 22 lines
Quoted 22 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +3 -1
       - 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
 
Quoted 7 lines

Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml patternProperties: · line 218

Quoted 5 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +0 -0
       reg:
@@ -207,20 +216,31 @@ patternProperties:
       - firmware-name
 
     unevaluatedProperties: false

Blank line.

Agreed, I will remove it in V3.

Quoted 10 lines
Quoted 10 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +9 -0
+allOf:
+  - if:
+      properties:
+        compatible:
+          enum:
+            - ti,am64-r5fss
+    then:
+      properties:
+        ti,cluster-mode:
Quoted 8 lines

Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml patternProperties: · line 232

Quoted 5 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +5 -0
+          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.

Quoted 5 lines

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

#5 lore raw

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

Hi Krzysztof,

Quoted 107 lines · Krzysztof Kozlowski
Quoted 106 lines · Devarsh Thakkar

Hi Krzysztof,

Thanks for the review. Please find my response inline.

Quoted 10 lines · Krzysztof Kozlowski
Quoted 9 lines · Devarsh Thakkar

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

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.

Quoted 5 lines
Quoted 3 lines

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

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

Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml properties: · line 47

Quoted 11 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +4 -0
+  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.

Quoted 22 lines
Quoted 22 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +3 -1
       - 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
 
Quoted 7 lines

Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml patternProperties: · line 218

Quoted 5 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +0 -0
       reg:
@@ -207,20 +216,31 @@ patternProperties:
       - firmware-name
 
     unevaluatedProperties: false

Blank line.

Agreed, I will remove it in V3.

Quoted 18 lines
Quoted 5 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +4 -0
+allOf:
+  - if:
+      properties:
+        compatible:

Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml patternProperties: · line 232

Quoted 10 lines
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml +10 -0
+          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.

Quoted 5 lines
Quoted 5 lines

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]

Quoted 7 lines
Quoted 4 lines

"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.

Quoted 4 lines

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

Quoted 7 lines

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

Best regards,
Krzysztof

#6 lore raw

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

Quoted 6 lines · Devarsh Thakkar
Quoted 4 lines

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?

Quoted 11 lines

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

#7 lore raw

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

In reply to Devarsh Thakkar

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

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

---
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(-)
drivers/remoteproc/ti_k3_r5_remoteproc.c +43 -12
index 0481926c6975..9698b29a0b56 100644
@@ -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 */ },
 };
Signature · 1 line
2.17.1
#8 lore raw

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

Hi Devarsh,

Please find my comments below.

Quoted 38 lines · Devarsh Thakkar

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.

---
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(-)
drivers/remoteproc/ti_k3_r5_remoteproc.c +2 -0
index 0481926c6975..9698b29a0b56 100644
@@ -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,
  };
 
  /**

drivers/remoteproc/ti_k3_r5_remoteproc.c enum cluster_mode { · line 97

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

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

Quoted 132 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +39 -13
  };
 
  /**
@@ -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

#9 lore raw

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

Hi Tanmay,

Thanks for the review and sorry for the delay. Please find my response inline.

Quoted 40 lines · Tanmay Shah

Hi Devarsh,

Please find my comments below.

Quoted 35 lines · Devarsh Thakkar

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

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

---
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(-)
drivers/remoteproc/ti_k3_r5_remoteproc.c +2 -0
index 0481926c6975..9698b29a0b56 100644
@@ -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,
   };
 
Quoted 15 lines

drivers/remoteproc/ti_k3_r5_remoteproc.c enum cluster_mode { · line 97

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

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.

Quoted 5 lines

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.

Quoted 4 lines

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

Quoted 155 lines

Thanks,

Tanmay

Quoted 148 lines
drivers/remoteproc/ti_k3_r5_remoteproc.c +0 -0
   };
 
   /**
@@ -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;

drivers/remoteproc/ti_k3_r5_remoteproc.c +1 -1
@@ -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) {

drivers/remoteproc/ti_k3_r5_remoteproc.c +0 -0
@@ -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;

drivers/remoteproc/ti_k3_r5_remoteproc.c +3 -1
@@ -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 {

drivers/remoteproc/ti_k3_r5_remoteproc.c +0 -0
@@ -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;
         }

drivers/remoteproc/ti_k3_r5_remoteproc.c +11 -8
@@ -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;
         }

drivers/remoteproc/ti_k3_r5_remoteproc.c +10 -0
@@ -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[] = {

drivers/remoteproc/ti_k3_r5_remoteproc.c +1 -0
@@ -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 */ },

  };

Signature · 1 line
2.17.1