[PATCH 0/3] Add Versal and Versal-NET platform support

[PATCH 0/3] Add Versal and Versal-NET platform support

[Tanmay Shah]

AMD-Xilinx Versal platform is successor of ZynqMP platform. Real-time
Processing Unit R5 cluster IP on Versal is same as of ZynqMP Platform.
Add bindings example for Versal remoteproc node. ZynqMP driver is
comatible for Versal's RPU unit.

AMD-Xilinx Versal-NET platform is successor of Versal platform.
Versal-NET Real-Time Processing Unit has two clusters and each cluster
contains dual core ARM Cortex-R52 processors. Each R52 core is
assigned 128KB of TCM memory.
Add bindings and example node of Versal-NET RPU. Add Versal-NET support
in ZynqMP remoteproc driver.

This series depends on follwoing series:
Link: https://lore.kernel.org/all/20240311175926.1625180-1-tanmay.shah@amd.com/
title: add zynqmp TCM bindings

Tanmay Shah (3):
  dt-bindings: remoteproc: add Versal platform support
  dt-bindings: remoteproc: add Versal-NET platform
  drivers: remoteproc: add Versal and Versal-NET support

 .../remoteproc/xlnx,zynqmp-r5fss.yaml         | 313 ++++++++++++++++--
 drivers/remoteproc/xlnx_r5_remoteproc.c       |  19 +-
 2 files changed, 292 insertions(+), 40 deletions(-)

base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
prerequisite-patch-id: ecf2ff9fd2f2a9a39f55f961b52a5414ea2f2962
prerequisite-patch-id: ff02d06bb3dbd13258cd5748c7486c363bd60478
prerequisite-patch-id: 4869c001ae65dfedaa2e481a3119de03b44a31c5
prerequisite-patch-id: 95e214bf94bce74d86972ea229186f602b5375ae
-- 
2.25.1

[PATCH 1/3] dt-bindings: remoteproc: add Versal platform support

[Tanmay Shah]

AMD-Xilinx Versal platform is successor of ZynqMP platform. Real-time
Processor Unit R5 cluster IP on Versal is same as of ZynqMP Platform.
Only difference is power-domains ID needed by power management firmware.
Hence, keeping the compatible property same as of zynqmp node.

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---
 .../remoteproc/xlnx,zynqmp-r5fss.yaml         | 93 +++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
index 629084a84ce6..711da0272250 100644
--- a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
@@ -293,4 +293,97 @@ examples:
             };
         };
     };
+
+  - |
+    // Versal Split mode configuration
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        remoteproc@ffe00000 {
+            compatible = "xlnx,zynqmp-r5fss";
+            xlnx,cluster-mode = <0>;
+
+            #address-cells = <2>;
+            #size-cells = <2>;
+            ranges = <0x0 0x0 0x0 0xffe00000 0x0 0x10000>,
+                     <0x0 0x20000 0x0 0xffe20000 0x0 0x10000>,
+                     <0x1 0x0 0x0 0xffe90000 0x0 0x10000>,
+                     <0x1 0x20000 0x0 0xffeb0000 0x0 0x10000>;
+
+            r5f@0 {
+                compatible = "xlnx,zynqmp-r5f";
+                reg = <0x0 0x0 0x0 0x10000>, <0x0 0x20000 0x0 0x10000>;
+                reg-names = "atcm0", "btcm0";
+                power-domains = <&versal_firmware 0x18110005>,
+                                <&versal_firmware 0x1831800b>,
+                                <&versal_firmware 0x1831800c>;
+                memory-region = <&rproc_0_fw_image>, <&rpu0vdev0buffer>,
+                                <&rpu0vdev0vring0>, <&rpu0vdev0vring1>;
+                mboxes = <&ipi_mailbox_rpu0 0>, <&ipi_mailbox_rpu0 1>;
+                mbox-names = "tx", "rx";
+            };
+
+            r5f@1 {
+                compatible = "xlnx,zynqmp-r5f";
+                reg = <0x1 0x0 0x0 0x10000>, <0x1 0x20000 0x0 0x10000>;
+                reg-names = "atcm0", "btcm0";
+                power-domains = <&versal_firmware 0x18110006>,
+                                <&versal_firmware 0x1831800d>,
+                                <&versal_firmware 0x1831800e>;
+                memory-region = <&rproc_1_fw_image>, <&rpu1vdev0buffer>,
+                                <&rpu1vdev0vring0>, <&rpu1vdev0vring1>;
+                mboxes = <&ipi_mailbox_rpu1 0>, <&ipi_mailbox_rpu1 1>;
+                mbox-names = "tx", "rx";
+            };
+        };
+    };
+
+  - |
+    // Versal Lockstep configuration
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        remoteproc@ffe00000 {
+            compatible = "xlnx,zynqmp-r5fss";
+            xlnx,cluster-mode = <1>;
+
+            #address-cells = <2>;
+            #size-cells = <2>;
+            ranges = <0x0 0x0 0x0 0xffe00000 0x0 0x20000>,
+                     <0x0 0x20000 0x0 0xffe20000 0x0 0x20000>;
+
+            r5f@0 {
+                compatible = "xlnx,zynqmp-r5f";
+                reg = <0x0 0x0 0x0 0x10000>,
+                      <0x0 0x20000 0x0 0x10000>,
+                      <0x0 0x10000 0x0 0x10000>,
+                      <0x0 0x30000 0x0 0x10000>;
+                reg-names = "atcm0", "btcm0", "atcm1", "btcm1";
+                power-domains = <&versal_firmware 0x18110005>,
+                                <&versal_firmware 0x1831800b>,
+                                <&versal_firmware 0x1831800c>,
+                                <&versal_firmware 0x1831800d>,
+                                <&versal_firmware 0x1831800e>;
+                memory-region = <&rproc_0_fw_image>, <&rpu0vdev0buffer>,
+                                <&rpu0vdev0vring0>, <&rpu0vdev0vring1>;
+                mboxes = <&ipi_mailbox_rpu0 0>, <&ipi_mailbox_rpu0 1>;
+                mbox-names = "tx", "rx";
+            };
+
+            r5f@1 {
+                compatible = "xlnx,zynqmp-r5f";
+                reg = <0x1 0x0 0x0 0x10000>, <0x1 0x20000 0x0 0x10000>;
+                reg-names = "atcm0", "btcm0";
+                power-domains = <&versal_firmware 0x18110006>,
+                                <&versal_firmware 0x1831800d>,
+                                <&versal_firmware 0x1831800e>;
+                memory-region = <&rproc_1_fw_image>, <&rpu1vdev0buffer>,
+                                <&rpu1vdev0vring0>, <&rpu1vdev0vring1>;
+                mboxes = <&ipi_mailbox_rpu1 0>, <&ipi_mailbox_rpu1 1>;
+                mbox-names = "tx", "rx";
+            };
+        };
+    };
 ...
-- 
2.25.1

Re: [PATCH 1/3] dt-bindings: remoteproc: add Versal platform support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

On Fri, Mar 15, 2024 at 02:15:31PM -0700, Tanmay Shah wrote:
> AMD-Xilinx Versal platform is successor of ZynqMP platform. Real-time
> Processor Unit R5 cluster IP on Versal is same as of ZynqMP Platform.

> Only difference is power-domains ID needed by power management firmware.
> Hence, keeping the compatible property same as of zynqmp node.

No, don't be lazy. Add a compatible with a fallback please.
This doesn't apply to linux-next either FYI.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] dt-bindings: remoteproc: add Versal platform support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 786 bytes --]

On Sun, Mar 17, 2024 at 02:50:27PM +0000, Conor Dooley wrote:
> On Fri, Mar 15, 2024 at 02:15:31PM -0700, Tanmay Shah wrote:
> > AMD-Xilinx Versal platform is successor of ZynqMP platform. Real-time
> > Processor Unit R5 cluster IP on Versal is same as of ZynqMP Platform.
> 
> > Only difference is power-domains ID needed by power management firmware.
> > Hence, keeping the compatible property same as of zynqmp node.
> 
> No, don't be lazy. Add a compatible with a fallback please.

> This doesn't apply to linux-next either FYI.

Ordinarily'd I'd not even bother applying a patch like this and I
wouldn't notice, but the diff for the versal-net patch is difficult to
read without colour-moved enabled and since I can't apply this I'm not
going to review that patch.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] dt-bindings: remoteproc: add Versal platform support

[Tanmay Shah]

Hello,

Thanks for reviews, please find my comments below.

On 3/17/24 9:50 AM, Conor Dooley wrote:
> On Fri, Mar 15, 2024 at 02:15:31PM -0700, Tanmay Shah wrote:
>> AMD-Xilinx Versal platform is successor of ZynqMP platform. Real-time
>> Processor Unit R5 cluster IP on Versal is same as of ZynqMP Platform.
> 
>> Only difference is power-domains ID needed by power management firmware.
>> Hence, keeping the compatible property same as of zynqmp node.
> 
> No, don't be lazy. Add a compatible with a fallback please.

It's same IP on different platform. I am not sure how adding compatible string
adds value. I will refactor this series based on other comments provided.

> This doesn't apply to linux-next either FYI.

Actually cover-letter contains dependent series link that is needed for this
patch. That series was acked recently but hasn't made to linux-next yet.

I may be missing something in the process. In such case there is no other way
to send patch except for waiting?

Thanks,
Tanmay

Re: [PATCH 1/3] dt-bindings: remoteproc: add Versal platform support

[Krzysztof Kozlowski]

On 19/03/2024 01:37, Tanmay Shah wrote:
> Hello,
> 
> Thanks for reviews, please find my comments below.
> 
> On 3/17/24 9:50 AM, Conor Dooley wrote:
>> On Fri, Mar 15, 2024 at 02:15:31PM -0700, Tanmay Shah wrote:
>>> AMD-Xilinx Versal platform is successor of ZynqMP platform. Real-time
>>> Processor Unit R5 cluster IP on Versal is same as of ZynqMP Platform.
>>
>>> Only difference is power-domains ID needed by power management firmware.
>>> Hence, keeping the compatible property same as of zynqmp node.
>>
>> No, don't be lazy. Add a compatible with a fallback please.
> 
> It's same IP on different platform. I am not sure how adding compatible string
> adds value. I will refactor this series based on other comments provided.

Judging by your other thread, it would add value. Also writing bindings
asks you for this.

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: remoteproc: add Versal platform support

[Krzysztof Kozlowski]

On 15/03/2024 22:15, Tanmay Shah wrote:
> AMD-Xilinx Versal platform is successor of ZynqMP platform. Real-time
> Processor Unit R5 cluster IP on Versal is same as of ZynqMP Platform.
> Only difference is power-domains ID needed by power management firmware.
> Hence, keeping the compatible property same as of zynqmp node.
> 
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>

There is no binding change, so NAK. Platform is not being added to
examples. You changed nothing in  Versal support...

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: remoteproc: add Versal platform support

[Tanmay Shah]

On 3/17/24 1:50 PM, Krzysztof Kozlowski wrote:
> On 15/03/2024 22:15, Tanmay Shah wrote:
>> AMD-Xilinx Versal platform is successor of ZynqMP platform. Real-time
>> Processor Unit R5 cluster IP on Versal is same as of ZynqMP Platform.
>> Only difference is power-domains ID needed by power management firmware.
>> Hence, keeping the compatible property same as of zynqmp node.
>> 
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> 
> There is no binding change, so NAK. Platform is not being added to
> examples. You changed nothing in  Versal support...

Thanks for reviews.

Okay got it. That means I don't really need to add anything related to Versal.
I will get rid of patch that says "Versal support". Looks like it's not needed
at all.

Thanks.

> 
> Best regards,
> Krzysztof
> 

[PATCH 2/3] dt-bindings: remoteproc: add Versal-NET platform

[Tanmay Shah]

AMD-Xilinx Versal-NET platform is successor of Versal platform. It
contains multiple clusters of cortex-R52 real-time processing units.
Each cluster contains two cores of cortex-R52 processors. Each cluster
can be configured in lockstep mode or split mode.

Each R52 core is assigned 128KB of TCM memory. ATCM memory is 64KB, BTCM
and CTCM memoreis are 32KB each. Each TCM memory has its own dedicated
power-domain that needs to be requested before using it.

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---
 .../remoteproc/xlnx,zynqmp-r5fss.yaml         | 220 +++++++++++++++---
 1 file changed, 184 insertions(+), 36 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
index 711da0272250..55654ee02eef 100644
--- a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
@@ -18,7 +18,9 @@ description: |
 
 properties:
   compatible:
-    const: xlnx,zynqmp-r5fss
+    enum:
+      - xlnx,zynqmp-r5fss
+      - xlnx,versal-net-r52fss
 
   "#address-cells":
     const: 2
@@ -64,7 +66,9 @@ patternProperties:
 
     properties:
       compatible:
-        const: xlnx,zynqmp-r5f
+        enum:
+          - xlnx,zynqmp-r5f
+          - xlnx,versal-net-r52f
 
       reg:
         minItems: 1
@@ -135,9 +139,11 @@ required:
 allOf:
   - if:
       properties:
-        xlnx,cluster-mode:
-          enum:
-            - 1
+        compatible:
+          contains:
+            enum:
+              - xlnx,versal-net-r52fss
+
     then:
       patternProperties:
         "^r5f@[0-9a-f]+$":
@@ -149,16 +155,14 @@ allOf:
               items:
                 - description: ATCM internal memory
                 - description: BTCM internal memory
-                - description: extra ATCM memory in lockstep mode
-                - description: extra BTCM memory in lockstep mode
+                - description: CTCM internal memory
 
             reg-names:
               minItems: 1
               items:
-                - const: atcm0
-                - const: btcm0
-                - const: atcm1
-                - const: btcm1
+                - const: atcm
+                - const: btcm
+                - const: ctcm
 
             power-domains:
               minItems: 2
@@ -166,33 +170,70 @@ allOf:
                 - description: RPU core power domain
                 - description: ATCM power domain
                 - description: BTCM power domain
-                - description: second ATCM power domain
-                - description: second BTCM power domain
+                - description: CTCM power domain
 
     else:
-      patternProperties:
-        "^r5f@[0-9a-f]+$":
-          type: object
-
-          properties:
-            reg:
-              minItems: 1
-              items:
-                - description: ATCM internal memory
-                - description: BTCM internal memory
-
-            reg-names:
-              minItems: 1
-              items:
-                - const: atcm0
-                - const: btcm0
-
-            power-domains:
-              minItems: 2
-              items:
-                - description: RPU core power domain
-                - description: ATCM power domain
-                - description: BTCM power domain
+      allOf:
+        - if:
+            properties:
+              xlnx,cluster-mode:
+                enum:
+                  - 1
+          then:
+            patternProperties:
+              "^r5f@[0-9a-f]+$":
+                type: object
+
+                properties:
+                  reg:
+                    minItems: 1
+                    items:
+                      - description: ATCM internal memory
+                      - description: BTCM internal memory
+                      - description: extra ATCM memory in lockstep mode
+                      - description: extra BTCM memory in lockstep mode
+
+                  reg-names:
+                    minItems: 1
+                    items:
+                      - const: atcm0
+                      - const: btcm0
+                      - const: atcm1
+                      - const: btcm1
+
+                  power-domains:
+                    minItems: 2
+                    items:
+                      - description: RPU core power domain
+                      - description: ATCM power domain
+                      - description: BTCM power domain
+                      - description: second ATCM power domain
+                      - description: second BTCM power domain
+
+          else:
+            patternProperties:
+              "^r5f@[0-9a-f]+$":
+                type: object
+
+                properties:
+                  reg:
+                    minItems: 1
+                    items:
+                      - description: ATCM internal memory
+                      - description: BTCM internal memory
+
+                  reg-names:
+                    minItems: 1
+                    items:
+                      - const: atcm0
+                      - const: btcm0
+
+                  power-domains:
+                    minItems: 2
+                    items:
+                      - description: RPU core power domain
+                      - description: ATCM power domain
+                      - description: BTCM power domain
 
 additionalProperties: false
 
@@ -386,4 +427,111 @@ examples:
             };
         };
     };
+
+  - |
+    // Versal-NET split configuration
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        remoteproc@eba00000 {
+            compatible = "xlnx,versal-net-r52fss";
+            xlnx,cluster-mode = <0>;
+            #address-cells = <2>;
+            #size-cells = <2>;
+            ranges = <0x0 0x0 0x0 0xeba00000 0x0 0x10000>,
+                     <0x0 0x10000 0x0 0xeba10000 0x0 0x8000>,
+                     <0x0 0x18000 0x0 0xeba20000 0x0 0x8000>,
+                     <0x1 0x0 0x0 0xeba40000 0x0 0x10000>,
+                     <0x1 0x10000 0x0 0xeba50000 0x0 0x8000>,
+                     <0x1 0x18000 0x0 0xeba60000 0x0 0x8000>;
+            r5f@0 {
+                compatible = "xlnx,versal-net-r52f";
+                reg = <0x0 0x0 0x0 0x10000>,
+                      <0x0 0x10000 0x0 0x8000>,
+                      <0x0 0x18000 0x0 0x8000>;
+                reg-names = "atcm", "btcm", "ctcm";
+                power-domains = <&versal_net_firmware 0x181100BF>,
+                                <&versal_net_firmware 0x183180CB>,
+                                <&versal_net_firmware 0x183180CC>,
+                                <&versal_net_firmware 0x183180CD>;
+                memory-region = <&rproc_0_fw_image>, <&rpu0vdev0buffer>,
+                                <&rpu0vdev0vring0>, <&rpu0vdev0vring1>;
+                mboxes = <&ipi_mailbox_rpu0 0>, <&ipi_mailbox_rpu0 1>;
+                mbox-names = "tx", "rx";
+            };
+
+            r5f@1 {
+                compatible = "xlnx,versal-net-r52f";
+                reg = <0x1 0x0 0x0 0x10000>, <0x1 0x10000 0x0 0x8000>,
+                      <0x1 0x18000 0x0 0x8000>;
+                reg-names = "atcm", "btcm", "ctcm";
+                power-domains = <&versal_net_firmware 0x181100C0>,
+                                <&versal_net_firmware 0x183180CE>,
+                                <&versal_net_firmware 0x183180CF>,
+                                <&versal_net_firmware 0x183180D0>;
+                memory-region = <&rproc_1_fw_image>, <&rpu1vdev0buffer>,
+                                <&rpu1vdev0vring0>, <&rpu1vdev0vring1>;
+                mboxes = <&ipi_mailbox_rpu1 0>, <&ipi_mailbox_rpu1 1>;
+                mbox-names = "tx", "rx";
+            };
+        };
+    };
+
+  - |
+    // Versal-NET lockstep configuration
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        remoteproc@eba00000 {
+            compatible = "xlnx,versal-net-r52fss";
+            xlnx,cluster-mode = <1>;
+            #address-cells = <2>;
+            #size-cells = <2>;
+            ranges = <0x0 0x0 0x0 0xeba00000 0x0 0x10000>,
+                     <0x0 0x10000 0x0 0xeba10000 0x0 0x8000>,
+                     <0x0 0x18000 0x0 0xeba20000 0x0 0x8000>,
+                     <0x1 0x0 0x0 0xeba40000 0x0 0x10000>,
+                     <0x1 0x10000 0x0 0xeba50000 0x0 0x8000>,
+                     <0x1 0x18000 0x0 0xeba60000 0x0 0x8000>;
+
+            r5f@0 {
+                compatible = "xlnx,versal-net-r52f";
+                reg = <0x0 0x0 0x0 0x10000>,
+                      <0x0 0x10000 0x0 0x8000>,
+                      <0x0 0x18000 0x0 0x8000>;
+
+                reg-names = "atcm", "btcm", "ctcm";
+
+                power-domains = <&versal_net_firmware 0x181100BF>,
+                                <&versal_net_firmware 0x183180CB>,
+                                <&versal_net_firmware 0x183180CC>,
+                                <&versal_net_firmware 0x183180CD>;
+
+                memory-region = <&rproc_0_fw_image>, <&rpu0vdev0buffer>,
+                                <&rpu0vdev0vring0>, <&rpu0vdev0vring1>;
+
+                mboxes = <&ipi_mailbox_rpu0 0>, <&ipi_mailbox_rpu0 1>;
+
+                mbox-names = "tx", "rx";
+            };
+
+            r5f@1 {
+                compatible = "xlnx,versal-net-r52f";
+                reg = <0x1 0x0 0x0 0x10000>,
+                      <0x1 0x10000 0x0 0x8000>,
+                      <0x1 0x18000 0x0 0x8000>;
+                reg-names = "atcm", "btcm", "ctcm";
+                power-domains = <&versal_net_firmware 0x181100C0>,
+                                <&versal_net_firmware 0x183180CE>,
+                                <&versal_net_firmware 0x183180CF>,
+                                <&versal_net_firmware 0x183180D0>;
+                memory-region = <&rproc_1_fw_image>, <&rpu1vdev0buffer>,
+                                <&rpu1vdev0vring0>, <&rpu1vdev0vring1>;
+                mboxes = <&ipi_mailbox_rpu1 0>, <&ipi_mailbox_rpu1 1>;
+                mbox-names = "tx", "rx";
+            };
+        };
+    };
 ...
-- 
2.25.1

Re: [PATCH 2/3] dt-bindings: remoteproc: add Versal-NET platform

[Krzysztof Kozlowski]

On 15/03/2024 22:15, Tanmay Shah wrote:
> AMD-Xilinx Versal-NET platform is successor of Versal platform. It
> contains multiple clusters of cortex-R52 real-time processing units.
> Each cluster contains two cores of cortex-R52 processors. Each cluster
> can be configured in lockstep mode or split mode.
> 
> Each R52 core is assigned 128KB of TCM memory. ATCM memory is 64KB, BTCM
> and CTCM memoreis are 32KB each. Each TCM memory has its own dedicated
> power-domain that needs to be requested before using it.
> 
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> ---
>  .../remoteproc/xlnx,zynqmp-r5fss.yaml         | 220 +++++++++++++++---
>  1 file changed, 184 insertions(+), 36 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
> index 711da0272250..55654ee02eef 100644
> --- a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
> @@ -18,7 +18,9 @@ description: |
>  
>  properties:
>    compatible:
> -    const: xlnx,zynqmp-r5fss
> +    enum:
> +      - xlnx,zynqmp-r5fss
> +      - xlnx,versal-net-r52fss
>  
>    "#address-cells":
>      const: 2
> @@ -64,7 +66,9 @@ patternProperties:
>  
>      properties:
>        compatible:
> -        const: xlnx,zynqmp-r5f
> +        enum:
> +          - xlnx,zynqmp-r5f
> +          - xlnx,versal-net-r52f
>  
>        reg:
>          minItems: 1
> @@ -135,9 +139,11 @@ required:
>  allOf:
>    - if:
>        properties:
> -        xlnx,cluster-mode:
> -          enum:
> -            - 1
> +        compatible:
> +          contains:
> +            enum:
> +              - xlnx,versal-net-r52fss

Why do you touch these lines?

> +
>      then:
>        patternProperties:
>          "^r5f@[0-9a-f]+$":
> @@ -149,16 +155,14 @@ allOf:
>                items:
>                  - description: ATCM internal memory
>                  - description: BTCM internal memory
> -                - description: extra ATCM memory in lockstep mode
> -                - description: extra BTCM memory in lockstep mode
> +                - description: CTCM internal memory
>  
>              reg-names:
>                minItems: 1
>                items:
> -                - const: atcm0
> -                - const: btcm0
> -                - const: atcm1
> -                - const: btcm1
> +                - const: atcm
> +                - const: btcm
> +                - const: ctcm
>  
>              power-domains:
>                minItems: 2
> @@ -166,33 +170,70 @@ allOf:
>                  - description: RPU core power domain
>                  - description: ATCM power domain
>                  - description: BTCM power domain
> -                - description: second ATCM power domain
> -                - description: second BTCM power domain
> +                - description: CTCM power domain
>  
>      else:
> -      patternProperties:
> -        "^r5f@[0-9a-f]+$":
> -          type: object
> -
> -          properties:
> -            reg:
> -              minItems: 1
> -              items:
> -                - description: ATCM internal memory
> -                - description: BTCM internal memory
> -
> -            reg-names:
> -              minItems: 1
> -              items:
> -                - const: atcm0
> -                - const: btcm0
> -
> -            power-domains:
> -              minItems: 2
> -              items:
> -                - description: RPU core power domain
> -                - description: ATCM power domain
> -                - description: BTCM power domain
> +      allOf:
> +        - if:
> +            properties:
> +              xlnx,cluster-mode:
> +                enum:
> +                  - 1

Whatever you did here, is not really readable. You have now multiple
if:then:if:then embedded.

> +          then:
> +            patternProperties:
> +              "^r5f@[0-9a-f]+$":
> +                type: object
> +
> +                properties:
> +                  reg:
> +                    minItems: 1
> +                    items:
> +                      - description: ATCM internal memory
> +                      - description: BTCM internal memory
> +                      - description: extra ATCM memory in lockstep mode
> +                      - description: extra BTCM memory in lockstep mode
> +
> +                  reg-names:
> +                    minItems: 1
> +                    items:
> +                      - const: atcm0
> +                      - const: btcm0
> +                      - const: atcm1
> +                      - const: btcm1
> +
> +                  power-domains:
> +                    minItems: 2
> +                    items:
> +                      - description: RPU core power domain
> +                      - description: ATCM power domain
> +                      - description: BTCM power domain
> +                      - description: second ATCM power domain
> +                      - description: second BTCM power domain
> +
> +          else:
> +            patternProperties:
> +              "^r5f@[0-9a-f]+$":
> +                type: object
> +
> +                properties:
> +                  reg:
> +                    minItems: 1
> +                    items:
> +                      - description: ATCM internal memory
> +                      - description: BTCM internal memory
> +
> +                  reg-names:
> +                    minItems: 1
> +                    items:
> +                      - const: atcm0
> +                      - const: btcm0
> +
> +                  power-domains:
> +                    minItems: 2
> +                    items:
> +                      - description: RPU core power domain
> +                      - description: ATCM power domain
> +                      - description: BTCM power domain
>  
>  additionalProperties: false
>  
> @@ -386,4 +427,111 @@ examples:
>              };
>          };
>      };
> +
> +  - |
> +    // Versal-NET split configuration
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +

Don't add examples per each platform.

Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: remoteproc: add Versal-NET platform

[Tanmay Shah]

Hello Krzysztof,

Thanks for reviews. Please find my comments below.

On 3/17/24 1:53 PM, Krzysztof Kozlowski wrote:
> On 15/03/2024 22:15, Tanmay Shah wrote:
>> AMD-Xilinx Versal-NET platform is successor of Versal platform. It
>> contains multiple clusters of cortex-R52 real-time processing units.
>> Each cluster contains two cores of cortex-R52 processors. Each cluster
>> can be configured in lockstep mode or split mode.
>> 
>> Each R52 core is assigned 128KB of TCM memory. ATCM memory is 64KB, BTCM
>> and CTCM memoreis are 32KB each. Each TCM memory has its own dedicated
>> power-domain that needs to be requested before using it.
>> 
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>> ---
>>  .../remoteproc/xlnx,zynqmp-r5fss.yaml         | 220 +++++++++++++++---
>>  1 file changed, 184 insertions(+), 36 deletions(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>> index 711da0272250..55654ee02eef 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>> +++ b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>> @@ -18,7 +18,9 @@ description: |
>>  
>>  properties:
>>    compatible:
>> -    const: xlnx,zynqmp-r5fss
>> +    enum:
>> +      - xlnx,zynqmp-r5fss
>> +      - xlnx,versal-net-r52fss
>>  
>>    "#address-cells":
>>      const: 2
>> @@ -64,7 +66,9 @@ patternProperties:
>>  
>>      properties:
>>        compatible:
>> -        const: xlnx,zynqmp-r5f
>> +        enum:
>> +          - xlnx,zynqmp-r5f
>> +          - xlnx,versal-net-r52f
>>  
>>        reg:
>>          minItems: 1
>> @@ -135,9 +139,11 @@ required:
>>  allOf:
>>    - if:
>>        properties:
>> -        xlnx,cluster-mode:
>> -          enum:
>> -            - 1
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - xlnx,versal-net-r52fss
> 
> Why do you touch these lines?
> 
>> +
>>      then:
>>        patternProperties:
>>          "^r5f@[0-9a-f]+$":
>> @@ -149,16 +155,14 @@ allOf:
>>                items:
>>                  - description: ATCM internal memory
>>                  - description: BTCM internal memory
>> -                - description: extra ATCM memory in lockstep mode
>> -                - description: extra BTCM memory in lockstep mode
>> +                - description: CTCM internal memory
>>  
>>              reg-names:
>>                minItems: 1
>>                items:
>> -                - const: atcm0
>> -                - const: btcm0
>> -                - const: atcm1
>> -                - const: btcm1
>> +                - const: atcm
>> +                - const: btcm
>> +                - const: ctcm
>>  
>>              power-domains:
>>                minItems: 2
>> @@ -166,33 +170,70 @@ allOf:
>>                  - description: RPU core power domain
>>                  - description: ATCM power domain
>>                  - description: BTCM power domain
>> -                - description: second ATCM power domain
>> -                - description: second BTCM power domain
>> +                - description: CTCM power domain
>>  
>>      else:
>> -      patternProperties:
>> -        "^r5f@[0-9a-f]+$":
>> -          type: object
>> -
>> -          properties:
>> -            reg:
>> -              minItems: 1
>> -              items:
>> -                - description: ATCM internal memory
>> -                - description: BTCM internal memory
>> -
>> -            reg-names:
>> -              minItems: 1
>> -              items:
>> -                - const: atcm0
>> -                - const: btcm0
>> -
>> -            power-domains:
>> -              minItems: 2
>> -              items:
>> -                - description: RPU core power domain
>> -                - description: ATCM power domain
>> -                - description: BTCM power domain
>> +      allOf:
>> +        - if:
>> +            properties:
>> +              xlnx,cluster-mode:
>> +                enum:
>> +                  - 1
> 
> Whatever you did here, is not really readable. You have now multiple
> if:then:if:then embedded.

For ZynqMP platform, TCM can be configured differently in lockstep mode
and split mode.

For Versal-NET no such configuration is available, but new CTCM memory
is added.

So, I am trying to achieve following representation of TCM for both:

if: versal-net compatible
then:
  ATCM - 64KB
  BTCM - 32KB
  CTCM - 32KB

else: (ZynqMP compatible)
  if:
    xlnx,cluster-mode (lockstep mode)
  then:
    ATCM0 - 64KB
    BTCM0 - 64KB
    ATCM1 - 64KB
    BTCM1 - 64KB
  else: (split mode)
    ATCM0 - 64KB
    BTCM0 - 64KB


If bindings are getting complicated, does it make sense to introduce
new file for Versal-NET bindings? Let me know how you would like me
to proceed.

Thanks,
Tanmay

> 
>> +          then:
>> +            patternProperties:
>> +              "^r5f@[0-9a-f]+$":
>> +                type: object
>> +
>> +                properties:
>> +                  reg:
>> +                    minItems: 1
>> +                    items:
>> +                      - description: ATCM internal memory
>> +                      - description: BTCM internal memory
>> +                      - description: extra ATCM memory in lockstep mode
>> +                      - description: extra BTCM memory in lockstep mode
>> +
>> +                  reg-names:
>> +                    minItems: 1
>> +                    items:
>> +                      - const: atcm0
>> +                      - const: btcm0
>> +                      - const: atcm1
>> +                      - const: btcm1
>> +
>> +                  power-domains:
>> +                    minItems: 2
>> +                    items:
>> +                      - description: RPU core power domain
>> +                      - description: ATCM power domain
>> +                      - description: BTCM power domain
>> +                      - description: second ATCM power domain
>> +                      - description: second BTCM power domain
>> +
>> +          else:
>> +            patternProperties:
>> +              "^r5f@[0-9a-f]+$":
>> +                type: object
>> +
>> +                properties:
>> +                  reg:
>> +                    minItems: 1
>> +                    items:
>> +                      - description: ATCM internal memory
>> +                      - description: BTCM internal memory
>> +
>> +                  reg-names:
>> +                    minItems: 1
>> +                    items:
>> +                      - const: atcm0
>> +                      - const: btcm0
>> +
>> +                  power-domains:
>> +                    minItems: 2
>> +                    items:
>> +                      - description: RPU core power domain
>> +                      - description: ATCM power domain
>> +                      - description: BTCM power domain
>>  
>>  additionalProperties: false
>>  
>> @@ -386,4 +427,111 @@ examples:
>>              };
>>          };
>>      };
>> +
>> +  - |
>> +    // Versal-NET split configuration
>> +    soc {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
> 
> Don't add examples per each platform.

Noted. If we go with different file for Versal-NET, then example should
be included?

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/3] dt-bindings: remoteproc: add Versal-NET platform

[Krzysztof Kozlowski]

On 19/03/2024 01:51, Tanmay Shah wrote:
> Hello Krzysztof,
> 
> Thanks for reviews. Please find my comments below.
> 
> On 3/17/24 1:53 PM, Krzysztof Kozlowski wrote:
>> On 15/03/2024 22:15, Tanmay Shah wrote:
>>> AMD-Xilinx Versal-NET platform is successor of Versal platform. It
>>> contains multiple clusters of cortex-R52 real-time processing units.
>>> Each cluster contains two cores of cortex-R52 processors. Each cluster
>>> can be configured in lockstep mode or split mode.
>>>
>>> Each R52 core is assigned 128KB of TCM memory. ATCM memory is 64KB, BTCM
>>> and CTCM memoreis are 32KB each. Each TCM memory has its own dedicated
>>> power-domain that needs to be requested before using it.
>>>
>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>> ---
>>>  .../remoteproc/xlnx,zynqmp-r5fss.yaml         | 220 +++++++++++++++---
>>>  1 file changed, 184 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>> index 711da0272250..55654ee02eef 100644
>>> --- a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>> +++ b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>> @@ -18,7 +18,9 @@ description: |
>>>  
>>>  properties:
>>>    compatible:
>>> -    const: xlnx,zynqmp-r5fss
>>> +    enum:
>>> +      - xlnx,zynqmp-r5fss
>>> +      - xlnx,versal-net-r52fss
>>>  
>>>    "#address-cells":
>>>      const: 2
>>> @@ -64,7 +66,9 @@ patternProperties:
>>>  
>>>      properties:
>>>        compatible:
>>> -        const: xlnx,zynqmp-r5f
>>> +        enum:
>>> +          - xlnx,zynqmp-r5f
>>> +          - xlnx,versal-net-r52f
>>>  
>>>        reg:
>>>          minItems: 1
>>> @@ -135,9 +139,11 @@ required:
>>>  allOf:
>>>    - if:
>>>        properties:
>>> -        xlnx,cluster-mode:
>>> -          enum:
>>> -            - 1
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - xlnx,versal-net-r52fss
>>
>> Why do you touch these lines?
>>
>>> +
>>>      then:
>>>        patternProperties:
>>>          "^r5f@[0-9a-f]+$":
>>> @@ -149,16 +155,14 @@ allOf:
>>>                items:
>>>                  - description: ATCM internal memory
>>>                  - description: BTCM internal memory
>>> -                - description: extra ATCM memory in lockstep mode
>>> -                - description: extra BTCM memory in lockstep mode
>>> +                - description: CTCM internal memory
>>>  
>>>              reg-names:
>>>                minItems: 1
>>>                items:
>>> -                - const: atcm0
>>> -                - const: btcm0
>>> -                - const: atcm1
>>> -                - const: btcm1
>>> +                - const: atcm
>>> +                - const: btcm
>>> +                - const: ctcm
>>>  
>>>              power-domains:
>>>                minItems: 2
>>> @@ -166,33 +170,70 @@ allOf:
>>>                  - description: RPU core power domain
>>>                  - description: ATCM power domain
>>>                  - description: BTCM power domain
>>> -                - description: second ATCM power domain
>>> -                - description: second BTCM power domain
>>> +                - description: CTCM power domain
>>>  
>>>      else:
>>> -      patternProperties:
>>> -        "^r5f@[0-9a-f]+$":
>>> -          type: object
>>> -
>>> -          properties:
>>> -            reg:
>>> -              minItems: 1
>>> -              items:
>>> -                - description: ATCM internal memory
>>> -                - description: BTCM internal memory
>>> -
>>> -            reg-names:
>>> -              minItems: 1
>>> -              items:
>>> -                - const: atcm0
>>> -                - const: btcm0
>>> -
>>> -            power-domains:
>>> -              minItems: 2
>>> -              items:
>>> -                - description: RPU core power domain
>>> -                - description: ATCM power domain
>>> -                - description: BTCM power domain
>>> +      allOf:
>>> +        - if:
>>> +            properties:
>>> +              xlnx,cluster-mode:
>>> +                enum:
>>> +                  - 1
>>
>> Whatever you did here, is not really readable. You have now multiple
>> if:then:if:then embedded.
> 
> For ZynqMP platform, TCM can be configured differently in lockstep mode
> and split mode.
> 
> For Versal-NET no such configuration is available, but new CTCM memory
> is added.
> 
> So, I am trying to achieve following representation of TCM for both:
> 
> if: versal-net compatible
> then:
>   ATCM - 64KB
>   BTCM - 32KB
>   CTCM - 32KB
> 
> else: (ZynqMP compatible)
>   if:
>     xlnx,cluster-mode (lockstep mode)
>   then:
>     ATCM0 - 64KB
>     BTCM0 - 64KB
>     ATCM1 - 64KB
>     BTCM1 - 64KB
>   else: (split mode)
>     ATCM0 - 64KB
>     BTCM0 - 64KB
> 
> 
> If bindings are getting complicated, does it make sense to introduce
> new file for Versal-NET bindings? Let me know how you would like me
> to proceed.

All this is broken in your previous patchset, but now we nicely see.

No, this does not work like this. You do not have entirely different
programming models in one device, don't you?


Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: remoteproc: add Versal-NET platform

[Tanmay Shah]

On 3/19/24 12:30 AM, Krzysztof Kozlowski wrote:
> On 19/03/2024 01:51, Tanmay Shah wrote:
>> Hello Krzysztof,
>> 
>> Thanks for reviews. Please find my comments below.
>> 
>> On 3/17/24 1:53 PM, Krzysztof Kozlowski wrote:
>>> On 15/03/2024 22:15, Tanmay Shah wrote:
>>>> AMD-Xilinx Versal-NET platform is successor of Versal platform. It
>>>> contains multiple clusters of cortex-R52 real-time processing units.
>>>> Each cluster contains two cores of cortex-R52 processors. Each cluster
>>>> can be configured in lockstep mode or split mode.
>>>>
>>>> Each R52 core is assigned 128KB of TCM memory. ATCM memory is 64KB, BTCM
>>>> and CTCM memoreis are 32KB each. Each TCM memory has its own dedicated
>>>> power-domain that needs to be requested before using it.
>>>>
>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>> ---
>>>>  .../remoteproc/xlnx,zynqmp-r5fss.yaml         | 220 +++++++++++++++---
>>>>  1 file changed, 184 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>> index 711da0272250..55654ee02eef 100644
>>>> --- a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>> +++ b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>> @@ -18,7 +18,9 @@ description: |
>>>>  
>>>>  properties:
>>>>    compatible:
>>>> -    const: xlnx,zynqmp-r5fss
>>>> +    enum:
>>>> +      - xlnx,zynqmp-r5fss
>>>> +      - xlnx,versal-net-r52fss
>>>>  
>>>>    "#address-cells":
>>>>      const: 2
>>>> @@ -64,7 +66,9 @@ patternProperties:
>>>>  
>>>>      properties:
>>>>        compatible:
>>>> -        const: xlnx,zynqmp-r5f
>>>> +        enum:
>>>> +          - xlnx,zynqmp-r5f
>>>> +          - xlnx,versal-net-r52f
>>>>  
>>>>        reg:
>>>>          minItems: 1
>>>> @@ -135,9 +139,11 @@ required:
>>>>  allOf:
>>>>    - if:
>>>>        properties:
>>>> -        xlnx,cluster-mode:
>>>> -          enum:
>>>> -            - 1
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - xlnx,versal-net-r52fss
>>>
>>> Why do you touch these lines?
>>>
>>>> +
>>>>      then:
>>>>        patternProperties:
>>>>          "^r5f@[0-9a-f]+$":
>>>> @@ -149,16 +155,14 @@ allOf:
>>>>                items:
>>>>                  - description: ATCM internal memory
>>>>                  - description: BTCM internal memory
>>>> -                - description: extra ATCM memory in lockstep mode
>>>> -                - description: extra BTCM memory in lockstep mode
>>>> +                - description: CTCM internal memory
>>>>  
>>>>              reg-names:
>>>>                minItems: 1
>>>>                items:
>>>> -                - const: atcm0
>>>> -                - const: btcm0
>>>> -                - const: atcm1
>>>> -                - const: btcm1
>>>> +                - const: atcm
>>>> +                - const: btcm
>>>> +                - const: ctcm
>>>>  
>>>>              power-domains:
>>>>                minItems: 2
>>>> @@ -166,33 +170,70 @@ allOf:
>>>>                  - description: RPU core power domain
>>>>                  - description: ATCM power domain
>>>>                  - description: BTCM power domain
>>>> -                - description: second ATCM power domain
>>>> -                - description: second BTCM power domain
>>>> +                - description: CTCM power domain
>>>>  
>>>>      else:
>>>> -      patternProperties:
>>>> -        "^r5f@[0-9a-f]+$":
>>>> -          type: object
>>>> -
>>>> -          properties:
>>>> -            reg:
>>>> -              minItems: 1
>>>> -              items:
>>>> -                - description: ATCM internal memory
>>>> -                - description: BTCM internal memory
>>>> -
>>>> -            reg-names:
>>>> -              minItems: 1
>>>> -              items:
>>>> -                - const: atcm0
>>>> -                - const: btcm0
>>>> -
>>>> -            power-domains:
>>>> -              minItems: 2
>>>> -              items:
>>>> -                - description: RPU core power domain
>>>> -                - description: ATCM power domain
>>>> -                - description: BTCM power domain
>>>> +      allOf:
>>>> +        - if:
>>>> +            properties:
>>>> +              xlnx,cluster-mode:
>>>> +                enum:
>>>> +                  - 1
>>>
>>> Whatever you did here, is not really readable. You have now multiple
>>> if:then:if:then embedded.
>> 
>> For ZynqMP platform, TCM can be configured differently in lockstep mode
>> and split mode.
>> 
>> For Versal-NET no such configuration is available, but new CTCM memory
>> is added.
>> 
>> So, I am trying to achieve following representation of TCM for both:
>> 
>> if: versal-net compatible
>> then:
>>   ATCM - 64KB
>>   BTCM - 32KB
>>   CTCM - 32KB
>> 
>> else: (ZynqMP compatible)
>>   if:
>>     xlnx,cluster-mode (lockstep mode)
>>   then:
>>     ATCM0 - 64KB
>>     BTCM0 - 64KB
>>     ATCM1 - 64KB
>>     BTCM1 - 64KB
>>   else: (split mode)
>>     ATCM0 - 64KB
>>     BTCM0 - 64KB
>> 
>> 
>> If bindings are getting complicated, does it make sense to introduce
>> new file for Versal-NET bindings? Let me know how you would like me
>> to proceed.
> 
> All this is broken in your previous patchset, but now we nicely see.
> 
> No, this does not work like this. You do not have entirely different
> programming models in one device, don't you?
> 

I don't understand what do you mean? Programming model is same. Only number
of TCMs are changing based on configuration and platform. I can certainly
list different compatible for different platforms as requested. But other than
that not sure what needs to be fixed.


> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/3] dt-bindings: remoteproc: add Versal-NET platform

[Krzysztof Kozlowski]

On 19/03/2024 15:42, Tanmay Shah wrote:
> 
> 
> On 3/19/24 12:30 AM, Krzysztof Kozlowski wrote:
>> On 19/03/2024 01:51, Tanmay Shah wrote:
>>> Hello Krzysztof,
>>>
>>> Thanks for reviews. Please find my comments below.
>>>
>>> On 3/17/24 1:53 PM, Krzysztof Kozlowski wrote:
>>>> On 15/03/2024 22:15, Tanmay Shah wrote:
>>>>> AMD-Xilinx Versal-NET platform is successor of Versal platform. It
>>>>> contains multiple clusters of cortex-R52 real-time processing units.
>>>>> Each cluster contains two cores of cortex-R52 processors. Each cluster
>>>>> can be configured in lockstep mode or split mode.
>>>>>
>>>>> Each R52 core is assigned 128KB of TCM memory. ATCM memory is 64KB, BTCM
>>>>> and CTCM memoreis are 32KB each. Each TCM memory has its own dedicated
>>>>> power-domain that needs to be requested before using it.
>>>>>
>>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>>> ---
>>>>>  .../remoteproc/xlnx,zynqmp-r5fss.yaml         | 220 +++++++++++++++---
>>>>>  1 file changed, 184 insertions(+), 36 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>> index 711da0272250..55654ee02eef 100644
>>>>> --- a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>> @@ -18,7 +18,9 @@ description: |
>>>>>  
>>>>>  properties:
>>>>>    compatible:
>>>>> -    const: xlnx,zynqmp-r5fss
>>>>> +    enum:
>>>>> +      - xlnx,zynqmp-r5fss
>>>>> +      - xlnx,versal-net-r52fss
>>>>>  
>>>>>    "#address-cells":
>>>>>      const: 2
>>>>> @@ -64,7 +66,9 @@ patternProperties:
>>>>>  
>>>>>      properties:
>>>>>        compatible:
>>>>> -        const: xlnx,zynqmp-r5f
>>>>> +        enum:
>>>>> +          - xlnx,zynqmp-r5f
>>>>> +          - xlnx,versal-net-r52f
>>>>>  
>>>>>        reg:
>>>>>          minItems: 1
>>>>> @@ -135,9 +139,11 @@ required:
>>>>>  allOf:
>>>>>    - if:
>>>>>        properties:
>>>>> -        xlnx,cluster-mode:
>>>>> -          enum:
>>>>> -            - 1
>>>>> +        compatible:
>>>>> +          contains:
>>>>> +            enum:
>>>>> +              - xlnx,versal-net-r52fss
>>>>
>>>> Why do you touch these lines?
>>>>
>>>>> +
>>>>>      then:
>>>>>        patternProperties:
>>>>>          "^r5f@[0-9a-f]+$":
>>>>> @@ -149,16 +155,14 @@ allOf:
>>>>>                items:
>>>>>                  - description: ATCM internal memory
>>>>>                  - description: BTCM internal memory
>>>>> -                - description: extra ATCM memory in lockstep mode
>>>>> -                - description: extra BTCM memory in lockstep mode
>>>>> +                - description: CTCM internal memory
>>>>>  
>>>>>              reg-names:
>>>>>                minItems: 1
>>>>>                items:
>>>>> -                - const: atcm0
>>>>> -                - const: btcm0
>>>>> -                - const: atcm1
>>>>> -                - const: btcm1
>>>>> +                - const: atcm
>>>>> +                - const: btcm
>>>>> +                - const: ctcm
>>>>>  
>>>>>              power-domains:
>>>>>                minItems: 2
>>>>> @@ -166,33 +170,70 @@ allOf:
>>>>>                  - description: RPU core power domain
>>>>>                  - description: ATCM power domain
>>>>>                  - description: BTCM power domain
>>>>> -                - description: second ATCM power domain
>>>>> -                - description: second BTCM power domain
>>>>> +                - description: CTCM power domain
>>>>>  
>>>>>      else:
>>>>> -      patternProperties:
>>>>> -        "^r5f@[0-9a-f]+$":
>>>>> -          type: object
>>>>> -
>>>>> -          properties:
>>>>> -            reg:
>>>>> -              minItems: 1
>>>>> -              items:
>>>>> -                - description: ATCM internal memory
>>>>> -                - description: BTCM internal memory
>>>>> -
>>>>> -            reg-names:
>>>>> -              minItems: 1
>>>>> -              items:
>>>>> -                - const: atcm0
>>>>> -                - const: btcm0
>>>>> -
>>>>> -            power-domains:
>>>>> -              minItems: 2
>>>>> -              items:
>>>>> -                - description: RPU core power domain
>>>>> -                - description: ATCM power domain
>>>>> -                - description: BTCM power domain
>>>>> +      allOf:
>>>>> +        - if:
>>>>> +            properties:
>>>>> +              xlnx,cluster-mode:
>>>>> +                enum:
>>>>> +                  - 1
>>>>
>>>> Whatever you did here, is not really readable. You have now multiple
>>>> if:then:if:then embedded.
>>>
>>> For ZynqMP platform, TCM can be configured differently in lockstep mode
>>> and split mode.
>>>
>>> For Versal-NET no such configuration is available, but new CTCM memory
>>> is added.
>>>
>>> So, I am trying to achieve following representation of TCM for both:
>>>
>>> if: versal-net compatible
>>> then:
>>>   ATCM - 64KB
>>>   BTCM - 32KB
>>>   CTCM - 32KB
>>>
>>> else: (ZynqMP compatible)
>>>   if:
>>>     xlnx,cluster-mode (lockstep mode)
>>>   then:
>>>     ATCM0 - 64KB
>>>     BTCM0 - 64KB
>>>     ATCM1 - 64KB
>>>     BTCM1 - 64KB
>>>   else: (split mode)
>>>     ATCM0 - 64KB
>>>     BTCM0 - 64KB
>>>
>>>
>>> If bindings are getting complicated, does it make sense to introduce
>>> new file for Versal-NET bindings? Let me know how you would like me
>>> to proceed.
>>
>> All this is broken in your previous patchset, but now we nicely see.
>>
>> No, this does not work like this. You do not have entirely different
>> programming models in one device, don't you?
>>
> 
> I don't understand what do you mean? Programming model is same. Only number
> of TCMs are changing based on configuration and platform. I can certainly
> list different compatible for different platforms as requested. But other than
> that not sure what needs to be fixed.

You cannot have same programming model with different memory mappings.
Anyway, please follow writing bindings rules: all of your different
devices must have dedicated compatible. I really though we talked about
two IPs on same SoC...


Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: remoteproc: add Versal-NET platform

[Tanmay Shah]

On 3/20/24 2:40 AM, Krzysztof Kozlowski wrote:
> On 19/03/2024 15:42, Tanmay Shah wrote:
>> 
>> 
>> On 3/19/24 12:30 AM, Krzysztof Kozlowski wrote:
>>> On 19/03/2024 01:51, Tanmay Shah wrote:
>>>> Hello Krzysztof,
>>>>
>>>> Thanks for reviews. Please find my comments below.
>>>>
>>>> On 3/17/24 1:53 PM, Krzysztof Kozlowski wrote:
>>>>> On 15/03/2024 22:15, Tanmay Shah wrote:
>>>>>> AMD-Xilinx Versal-NET platform is successor of Versal platform. It
>>>>>> contains multiple clusters of cortex-R52 real-time processing units.
>>>>>> Each cluster contains two cores of cortex-R52 processors. Each cluster
>>>>>> can be configured in lockstep mode or split mode.
>>>>>>
>>>>>> Each R52 core is assigned 128KB of TCM memory. ATCM memory is 64KB, BTCM
>>>>>> and CTCM memoreis are 32KB each. Each TCM memory has its own dedicated
>>>>>> power-domain that needs to be requested before using it.
>>>>>>
>>>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>>>> ---
>>>>>>  .../remoteproc/xlnx,zynqmp-r5fss.yaml         | 220 +++++++++++++++---
>>>>>>  1 file changed, 184 insertions(+), 36 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>>> index 711da0272250..55654ee02eef 100644
>>>>>> --- a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>>> @@ -18,7 +18,9 @@ description: |
>>>>>>  
>>>>>>  properties:
>>>>>>    compatible:
>>>>>> -    const: xlnx,zynqmp-r5fss
>>>>>> +    enum:
>>>>>> +      - xlnx,zynqmp-r5fss
>>>>>> +      - xlnx,versal-net-r52fss
>>>>>>  
>>>>>>    "#address-cells":
>>>>>>      const: 2
>>>>>> @@ -64,7 +66,9 @@ patternProperties:
>>>>>>  
>>>>>>      properties:
>>>>>>        compatible:
>>>>>> -        const: xlnx,zynqmp-r5f
>>>>>> +        enum:
>>>>>> +          - xlnx,zynqmp-r5f
>>>>>> +          - xlnx,versal-net-r52f
>>>>>>  
>>>>>>        reg:
>>>>>>          minItems: 1
>>>>>> @@ -135,9 +139,11 @@ required:
>>>>>>  allOf:
>>>>>>    - if:
>>>>>>        properties:
>>>>>> -        xlnx,cluster-mode:
>>>>>> -          enum:
>>>>>> -            - 1
>>>>>> +        compatible:
>>>>>> +          contains:
>>>>>> +            enum:
>>>>>> +              - xlnx,versal-net-r52fss
>>>>>
>>>>> Why do you touch these lines?
>>>>>
>>>>>> +
>>>>>>      then:
>>>>>>        patternProperties:
>>>>>>          "^r5f@[0-9a-f]+$":
>>>>>> @@ -149,16 +155,14 @@ allOf:
>>>>>>                items:
>>>>>>                  - description: ATCM internal memory
>>>>>>                  - description: BTCM internal memory
>>>>>> -                - description: extra ATCM memory in lockstep mode
>>>>>> -                - description: extra BTCM memory in lockstep mode
>>>>>> +                - description: CTCM internal memory
>>>>>>  
>>>>>>              reg-names:
>>>>>>                minItems: 1
>>>>>>                items:
>>>>>> -                - const: atcm0
>>>>>> -                - const: btcm0
>>>>>> -                - const: atcm1
>>>>>> -                - const: btcm1
>>>>>> +                - const: atcm
>>>>>> +                - const: btcm
>>>>>> +                - const: ctcm
>>>>>>  
>>>>>>              power-domains:
>>>>>>                minItems: 2
>>>>>> @@ -166,33 +170,70 @@ allOf:
>>>>>>                  - description: RPU core power domain
>>>>>>                  - description: ATCM power domain
>>>>>>                  - description: BTCM power domain
>>>>>> -                - description: second ATCM power domain
>>>>>> -                - description: second BTCM power domain
>>>>>> +                - description: CTCM power domain
>>>>>>  
>>>>>>      else:
>>>>>> -      patternProperties:
>>>>>> -        "^r5f@[0-9a-f]+$":
>>>>>> -          type: object
>>>>>> -
>>>>>> -          properties:
>>>>>> -            reg:
>>>>>> -              minItems: 1
>>>>>> -              items:
>>>>>> -                - description: ATCM internal memory
>>>>>> -                - description: BTCM internal memory
>>>>>> -
>>>>>> -            reg-names:
>>>>>> -              minItems: 1
>>>>>> -              items:
>>>>>> -                - const: atcm0
>>>>>> -                - const: btcm0
>>>>>> -
>>>>>> -            power-domains:
>>>>>> -              minItems: 2
>>>>>> -              items:
>>>>>> -                - description: RPU core power domain
>>>>>> -                - description: ATCM power domain
>>>>>> -                - description: BTCM power domain
>>>>>> +      allOf:
>>>>>> +        - if:
>>>>>> +            properties:
>>>>>> +              xlnx,cluster-mode:
>>>>>> +                enum:
>>>>>> +                  - 1
>>>>>
>>>>> Whatever you did here, is not really readable. You have now multiple
>>>>> if:then:if:then embedded.
>>>>
>>>> For ZynqMP platform, TCM can be configured differently in lockstep mode
>>>> and split mode.
>>>>
>>>> For Versal-NET no such configuration is available, but new CTCM memory
>>>> is added.
>>>>
>>>> So, I am trying to achieve following representation of TCM for both:
>>>>
>>>> if: versal-net compatible
>>>> then:
>>>>   ATCM - 64KB
>>>>   BTCM - 32KB
>>>>   CTCM - 32KB
>>>>
>>>> else: (ZynqMP compatible)
>>>>   if:
>>>>     xlnx,cluster-mode (lockstep mode)
>>>>   then:
>>>>     ATCM0 - 64KB
>>>>     BTCM0 - 64KB
>>>>     ATCM1 - 64KB
>>>>     BTCM1 - 64KB
>>>>   else: (split mode)
>>>>     ATCM0 - 64KB
>>>>     BTCM0 - 64KB
>>>>
>>>>
>>>> If bindings are getting complicated, does it make sense to introduce
>>>> new file for Versal-NET bindings? Let me know how you would like me
>>>> to proceed.
>>>
>>> All this is broken in your previous patchset, but now we nicely see.
>>>
>>> No, this does not work like this. You do not have entirely different
>>> programming models in one device, don't you?
>>>
>> 
>> I don't understand what do you mean? Programming model is same. Only number
>> of TCMs are changing based on configuration and platform. I can certainly
>> list different compatible for different platforms as requested. But other than
>> that not sure what needs to be fixed.
> 
> You cannot have same programming model with different memory mappings.
> Anyway, please follow writing bindings rules: all of your different
> devices must have dedicated compatible. I really though we talked about
> two IPs on same SoC...

I agree that Versal compatible should be added, I will do that in next revision.

For ZynqMP case, it is two IPs on same SOC. In lockstep mode and split mode,
same SOC is configuring TCM differently.

How this should be resolved for Versal-NET ? Driver avoids such TCM configuration
for Versal-NET.

If you would like to see v14, before further discussion please let me know.

Thanks,
Tanmay

> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/3] dt-bindings: remoteproc: add Versal-NET platform

[Krzysztof Kozlowski]

On 20/03/2024 16:14, Tanmay Shah wrote:
> 
> 
> On 3/20/24 2:40 AM, Krzysztof Kozlowski wrote:
>> On 19/03/2024 15:42, Tanmay Shah wrote:
>>>
>>>
>>> On 3/19/24 12:30 AM, Krzysztof Kozlowski wrote:
>>>> On 19/03/2024 01:51, Tanmay Shah wrote:
>>>>> Hello Krzysztof,
>>>>>
>>>>> Thanks for reviews. Please find my comments below.
>>>>>
>>>>> On 3/17/24 1:53 PM, Krzysztof Kozlowski wrote:
>>>>>> On 15/03/2024 22:15, Tanmay Shah wrote:
>>>>>>> AMD-Xilinx Versal-NET platform is successor of Versal platform. It
>>>>>>> contains multiple clusters of cortex-R52 real-time processing units.
>>>>>>> Each cluster contains two cores of cortex-R52 processors. Each cluster
>>>>>>> can be configured in lockstep mode or split mode.
>>>>>>>
>>>>>>> Each R52 core is assigned 128KB of TCM memory. ATCM memory is 64KB, BTCM
>>>>>>> and CTCM memoreis are 32KB each. Each TCM memory has its own dedicated
>>>>>>> power-domain that needs to be requested before using it.
>>>>>>>
>>>>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>>>>> ---
>>>>>>>  .../remoteproc/xlnx,zynqmp-r5fss.yaml         | 220 +++++++++++++++---
>>>>>>>  1 file changed, 184 insertions(+), 36 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>>>> index 711da0272250..55654ee02eef 100644
>>>>>>> --- a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>>>> @@ -18,7 +18,9 @@ description: |
>>>>>>>  
>>>>>>>  properties:
>>>>>>>    compatible:
>>>>>>> -    const: xlnx,zynqmp-r5fss
>>>>>>> +    enum:
>>>>>>> +      - xlnx,zynqmp-r5fss
>>>>>>> +      - xlnx,versal-net-r52fss
>>>>>>>  
>>>>>>>    "#address-cells":
>>>>>>>      const: 2
>>>>>>> @@ -64,7 +66,9 @@ patternProperties:
>>>>>>>  
>>>>>>>      properties:
>>>>>>>        compatible:
>>>>>>> -        const: xlnx,zynqmp-r5f
>>>>>>> +        enum:
>>>>>>> +          - xlnx,zynqmp-r5f
>>>>>>> +          - xlnx,versal-net-r52f
>>>>>>>  
>>>>>>>        reg:
>>>>>>>          minItems: 1
>>>>>>> @@ -135,9 +139,11 @@ required:
>>>>>>>  allOf:
>>>>>>>    - if:
>>>>>>>        properties:
>>>>>>> -        xlnx,cluster-mode:
>>>>>>> -          enum:
>>>>>>> -            - 1
>>>>>>> +        compatible:
>>>>>>> +          contains:
>>>>>>> +            enum:
>>>>>>> +              - xlnx,versal-net-r52fss
>>>>>>
>>>>>> Why do you touch these lines?
>>>>>>
>>>>>>> +
>>>>>>>      then:
>>>>>>>        patternProperties:
>>>>>>>          "^r5f@[0-9a-f]+$":
>>>>>>> @@ -149,16 +155,14 @@ allOf:
>>>>>>>                items:
>>>>>>>                  - description: ATCM internal memory
>>>>>>>                  - description: BTCM internal memory
>>>>>>> -                - description: extra ATCM memory in lockstep mode
>>>>>>> -                - description: extra BTCM memory in lockstep mode
>>>>>>> +                - description: CTCM internal memory
>>>>>>>  
>>>>>>>              reg-names:
>>>>>>>                minItems: 1
>>>>>>>                items:
>>>>>>> -                - const: atcm0
>>>>>>> -                - const: btcm0
>>>>>>> -                - const: atcm1
>>>>>>> -                - const: btcm1
>>>>>>> +                - const: atcm
>>>>>>> +                - const: btcm
>>>>>>> +                - const: ctcm
>>>>>>>  
>>>>>>>              power-domains:
>>>>>>>                minItems: 2
>>>>>>> @@ -166,33 +170,70 @@ allOf:
>>>>>>>                  - description: RPU core power domain
>>>>>>>                  - description: ATCM power domain
>>>>>>>                  - description: BTCM power domain
>>>>>>> -                - description: second ATCM power domain
>>>>>>> -                - description: second BTCM power domain
>>>>>>> +                - description: CTCM power domain
>>>>>>>  
>>>>>>>      else:
>>>>>>> -      patternProperties:
>>>>>>> -        "^r5f@[0-9a-f]+$":
>>>>>>> -          type: object
>>>>>>> -
>>>>>>> -          properties:
>>>>>>> -            reg:
>>>>>>> -              minItems: 1
>>>>>>> -              items:
>>>>>>> -                - description: ATCM internal memory
>>>>>>> -                - description: BTCM internal memory
>>>>>>> -
>>>>>>> -            reg-names:
>>>>>>> -              minItems: 1
>>>>>>> -              items:
>>>>>>> -                - const: atcm0
>>>>>>> -                - const: btcm0
>>>>>>> -
>>>>>>> -            power-domains:
>>>>>>> -              minItems: 2
>>>>>>> -              items:
>>>>>>> -                - description: RPU core power domain
>>>>>>> -                - description: ATCM power domain
>>>>>>> -                - description: BTCM power domain
>>>>>>> +      allOf:
>>>>>>> +        - if:
>>>>>>> +            properties:
>>>>>>> +              xlnx,cluster-mode:
>>>>>>> +                enum:
>>>>>>> +                  - 1
>>>>>>
>>>>>> Whatever you did here, is not really readable. You have now multiple
>>>>>> if:then:if:then embedded.
>>>>>
>>>>> For ZynqMP platform, TCM can be configured differently in lockstep mode
>>>>> and split mode.
>>>>>
>>>>> For Versal-NET no such configuration is available, but new CTCM memory
>>>>> is added.
>>>>>
>>>>> So, I am trying to achieve following representation of TCM for both:
>>>>>
>>>>> if: versal-net compatible
>>>>> then:
>>>>>   ATCM - 64KB
>>>>>   BTCM - 32KB
>>>>>   CTCM - 32KB
>>>>>
>>>>> else: (ZynqMP compatible)
>>>>>   if:
>>>>>     xlnx,cluster-mode (lockstep mode)
>>>>>   then:
>>>>>     ATCM0 - 64KB
>>>>>     BTCM0 - 64KB
>>>>>     ATCM1 - 64KB
>>>>>     BTCM1 - 64KB
>>>>>   else: (split mode)
>>>>>     ATCM0 - 64KB
>>>>>     BTCM0 - 64KB
>>>>>
>>>>>
>>>>> If bindings are getting complicated, does it make sense to introduce
>>>>> new file for Versal-NET bindings? Let me know how you would like me
>>>>> to proceed.
>>>>
>>>> All this is broken in your previous patchset, but now we nicely see.
>>>>
>>>> No, this does not work like this. You do not have entirely different
>>>> programming models in one device, don't you?
>>>>
>>>
>>> I don't understand what do you mean? Programming model is same. Only number
>>> of TCMs are changing based on configuration and platform. I can certainly
>>> list different compatible for different platforms as requested. But other than
>>> that not sure what needs to be fixed.
>>
>> You cannot have same programming model with different memory mappings.
>> Anyway, please follow writing bindings rules: all of your different
>> devices must have dedicated compatible. I really though we talked about
>> two IPs on same SoC...
> 
> I agree that Versal compatible should be added, I will do that in next revision.
> 
> For ZynqMP case, it is two IPs on same SOC. In lockstep mode and split mode,
> same SOC is configuring TCM differently.
> 
> How this should be resolved for Versal-NET ? Driver avoids such TCM configuration
> for Versal-NET.

Binding should describe the hardware, not what driver is doing
currently, so the question is: does your device have such properties or
not? Anyway, you need compatible per each variant and each SoC
implementation.

Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: remoteproc: add Versal-NET platform

[Tanmay Shah]

On 3/21/24 2:39 AM, Krzysztof Kozlowski wrote:
> On 20/03/2024 16:14, Tanmay Shah wrote:
>> 
>> 
>> On 3/20/24 2:40 AM, Krzysztof Kozlowski wrote:
>>> On 19/03/2024 15:42, Tanmay Shah wrote:
>>>>
>>>>
>>>> On 3/19/24 12:30 AM, Krzysztof Kozlowski wrote:
>>>>> On 19/03/2024 01:51, Tanmay Shah wrote:
>>>>>> Hello Krzysztof,
>>>>>>
>>>>>> Thanks for reviews. Please find my comments below.
>>>>>>
>>>>>> On 3/17/24 1:53 PM, Krzysztof Kozlowski wrote:
>>>>>>> On 15/03/2024 22:15, Tanmay Shah wrote:
>>>>>>>> AMD-Xilinx Versal-NET platform is successor of Versal platform. It
>>>>>>>> contains multiple clusters of cortex-R52 real-time processing units.
>>>>>>>> Each cluster contains two cores of cortex-R52 processors. Each cluster
>>>>>>>> can be configured in lockstep mode or split mode.
>>>>>>>>
>>>>>>>> Each R52 core is assigned 128KB of TCM memory. ATCM memory is 64KB, BTCM
>>>>>>>> and CTCM memoreis are 32KB each. Each TCM memory has its own dedicated
>>>>>>>> power-domain that needs to be requested before using it.
>>>>>>>>
>>>>>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>>>>>> ---
>>>>>>>>  .../remoteproc/xlnx,zynqmp-r5fss.yaml         | 220 +++++++++++++++---
>>>>>>>>  1 file changed, 184 insertions(+), 36 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>>>>> index 711da0272250..55654ee02eef 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>>>>> @@ -18,7 +18,9 @@ description: |
>>>>>>>>  
>>>>>>>>  properties:
>>>>>>>>    compatible:
>>>>>>>> -    const: xlnx,zynqmp-r5fss
>>>>>>>> +    enum:
>>>>>>>> +      - xlnx,zynqmp-r5fss
>>>>>>>> +      - xlnx,versal-net-r52fss
>>>>>>>>  
>>>>>>>>    "#address-cells":
>>>>>>>>      const: 2
>>>>>>>> @@ -64,7 +66,9 @@ patternProperties:
>>>>>>>>  
>>>>>>>>      properties:
>>>>>>>>        compatible:
>>>>>>>> -        const: xlnx,zynqmp-r5f
>>>>>>>> +        enum:
>>>>>>>> +          - xlnx,zynqmp-r5f
>>>>>>>> +          - xlnx,versal-net-r52f
>>>>>>>>  
>>>>>>>>        reg:
>>>>>>>>          minItems: 1
>>>>>>>> @@ -135,9 +139,11 @@ required:
>>>>>>>>  allOf:
>>>>>>>>    - if:
>>>>>>>>        properties:
>>>>>>>> -        xlnx,cluster-mode:
>>>>>>>> -          enum:
>>>>>>>> -            - 1
>>>>>>>> +        compatible:
>>>>>>>> +          contains:
>>>>>>>> +            enum:
>>>>>>>> +              - xlnx,versal-net-r52fss
>>>>>>>
>>>>>>> Why do you touch these lines?
>>>>>>>
>>>>>>>> +
>>>>>>>>      then:
>>>>>>>>        patternProperties:
>>>>>>>>          "^r5f@[0-9a-f]+$":
>>>>>>>> @@ -149,16 +155,14 @@ allOf:
>>>>>>>>                items:
>>>>>>>>                  - description: ATCM internal memory
>>>>>>>>                  - description: BTCM internal memory
>>>>>>>> -                - description: extra ATCM memory in lockstep mode
>>>>>>>> -                - description: extra BTCM memory in lockstep mode
>>>>>>>> +                - description: CTCM internal memory
>>>>>>>>  
>>>>>>>>              reg-names:
>>>>>>>>                minItems: 1
>>>>>>>>                items:
>>>>>>>> -                - const: atcm0
>>>>>>>> -                - const: btcm0
>>>>>>>> -                - const: atcm1
>>>>>>>> -                - const: btcm1
>>>>>>>> +                - const: atcm
>>>>>>>> +                - const: btcm
>>>>>>>> +                - const: ctcm
>>>>>>>>  
>>>>>>>>              power-domains:
>>>>>>>>                minItems: 2
>>>>>>>> @@ -166,33 +170,70 @@ allOf:
>>>>>>>>                  - description: RPU core power domain
>>>>>>>>                  - description: ATCM power domain
>>>>>>>>                  - description: BTCM power domain
>>>>>>>> -                - description: second ATCM power domain
>>>>>>>> -                - description: second BTCM power domain
>>>>>>>> +                - description: CTCM power domain
>>>>>>>>  
>>>>>>>>      else:
>>>>>>>> -      patternProperties:
>>>>>>>> -        "^r5f@[0-9a-f]+$":
>>>>>>>> -          type: object
>>>>>>>> -
>>>>>>>> -          properties:
>>>>>>>> -            reg:
>>>>>>>> -              minItems: 1
>>>>>>>> -              items:
>>>>>>>> -                - description: ATCM internal memory
>>>>>>>> -                - description: BTCM internal memory
>>>>>>>> -
>>>>>>>> -            reg-names:
>>>>>>>> -              minItems: 1
>>>>>>>> -              items:
>>>>>>>> -                - const: atcm0
>>>>>>>> -                - const: btcm0
>>>>>>>> -
>>>>>>>> -            power-domains:
>>>>>>>> -              minItems: 2
>>>>>>>> -              items:
>>>>>>>> -                - description: RPU core power domain
>>>>>>>> -                - description: ATCM power domain
>>>>>>>> -                - description: BTCM power domain
>>>>>>>> +      allOf:
>>>>>>>> +        - if:
>>>>>>>> +            properties:
>>>>>>>> +              xlnx,cluster-mode:
>>>>>>>> +                enum:
>>>>>>>> +                  - 1
>>>>>>>
>>>>>>> Whatever you did here, is not really readable. You have now multiple
>>>>>>> if:then:if:then embedded.
>>>>>>
>>>>>> For ZynqMP platform, TCM can be configured differently in lockstep mode
>>>>>> and split mode.
>>>>>>
>>>>>> For Versal-NET no such configuration is available, but new CTCM memory
>>>>>> is added.
>>>>>>
>>>>>> So, I am trying to achieve following representation of TCM for both:
>>>>>>
>>>>>> if: versal-net compatible
>>>>>> then:
>>>>>>   ATCM - 64KB
>>>>>>   BTCM - 32KB
>>>>>>   CTCM - 32KB
>>>>>>
>>>>>> else: (ZynqMP compatible)
>>>>>>   if:
>>>>>>     xlnx,cluster-mode (lockstep mode)
>>>>>>   then:
>>>>>>     ATCM0 - 64KB
>>>>>>     BTCM0 - 64KB
>>>>>>     ATCM1 - 64KB
>>>>>>     BTCM1 - 64KB
>>>>>>   else: (split mode)
>>>>>>     ATCM0 - 64KB
>>>>>>     BTCM0 - 64KB
>>>>>>
>>>>>>
>>>>>> If bindings are getting complicated, does it make sense to introduce
>>>>>> new file for Versal-NET bindings? Let me know how you would like me
>>>>>> to proceed.
>>>>>
>>>>> All this is broken in your previous patchset, but now we nicely see.
>>>>>
>>>>> No, this does not work like this. You do not have entirely different
>>>>> programming models in one device, don't you?
>>>>>
>>>>
>>>> I don't understand what do you mean? Programming model is same. Only number
>>>> of TCMs are changing based on configuration and platform. I can certainly
>>>> list different compatible for different platforms as requested. But other than
>>>> that not sure what needs to be fixed.
>>>
>>> You cannot have same programming model with different memory mappings.
>>> Anyway, please follow writing bindings rules: all of your different
>>> devices must have dedicated compatible. I really though we talked about
>>> two IPs on same SoC...
>> 
>> I agree that Versal compatible should be added, I will do that in next revision.
>> 
>> For ZynqMP case, it is two IPs on same SOC. In lockstep mode and split mode,
>> same SOC is configuring TCM differently.
>> 
>> How this should be resolved for Versal-NET ? Driver avoids such TCM configuration
>> for Versal-NET.
> 
> Binding should describe the hardware, not what driver is doing
> currently, so the question is: does your device have such properties or
> not? Anyway, you need compatible per each variant and each SoC
> implementation.

Thanks for reviews.

Okay in that case I believe I should add one more property to current bindings for TCM
configuration.

From our discussion I conclude to following next steps:

1) I will send Versal and Versal-NET support as part of previous series (v14) so we get
bigger picture in the first place.

2) Add separate compatible for versal platform.
Use device compatible string to maintain
backward compatibility and not machine (root node) compatible string.

3) Add tcm,mode property in bindings and each device must configure TCM based on that
property only and not based on compatible string.

4) Versal-NET will disallow tcm,mode property in bindings as no such configuration is
possible for that platform.

I hope I got everything. I will test and send v14 of previous series accordingly.

Tanmay

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/3] dt-bindings: remoteproc: add Versal-NET platform

[Krzysztof Kozlowski]

On 21/03/2024 16:13, Tanmay Shah wrote:
> 
> 
> On 3/21/24 2:39 AM, Krzysztof Kozlowski wrote:
>> On 20/03/2024 16:14, Tanmay Shah wrote:
>>>
>>>
>>> On 3/20/24 2:40 AM, Krzysztof Kozlowski wrote:
>>>> On 19/03/2024 15:42, Tanmay Shah wrote:
>>>>>
>>>>>
>>>>> On 3/19/24 12:30 AM, Krzysztof Kozlowski wrote:
>>>>>> On 19/03/2024 01:51, Tanmay Shah wrote:
>>>>>>> Hello Krzysztof,
>>>>>>>
>>>>>>> Thanks for reviews. Please find my comments below.
>>>>>>>
>>>>>>> On 3/17/24 1:53 PM, Krzysztof Kozlowski wrote:
>>>>>>>> On 15/03/2024 22:15, Tanmay Shah wrote:
>>>>>>>>> AMD-Xilinx Versal-NET platform is successor of Versal platform. It
>>>>>>>>> contains multiple clusters of cortex-R52 real-time processing units.
>>>>>>>>> Each cluster contains two cores of cortex-R52 processors. Each cluster
>>>>>>>>> can be configured in lockstep mode or split mode.
>>>>>>>>>
>>>>>>>>> Each R52 core is assigned 128KB of TCM memory. ATCM memory is 64KB, BTCM
>>>>>>>>> and CTCM memoreis are 32KB each. Each TCM memory has its own dedicated
>>>>>>>>> power-domain that needs to be requested before using it.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>>>>>>> ---
>>>>>>>>>  .../remoteproc/xlnx,zynqmp-r5fss.yaml         | 220 +++++++++++++++---
>>>>>>>>>  1 file changed, 184 insertions(+), 36 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>>>>>> index 711da0272250..55654ee02eef 100644
>>>>>>>>> --- a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>>>>>> @@ -18,7 +18,9 @@ description: |
>>>>>>>>>  
>>>>>>>>>  properties:
>>>>>>>>>    compatible:
>>>>>>>>> -    const: xlnx,zynqmp-r5fss
>>>>>>>>> +    enum:
>>>>>>>>> +      - xlnx,zynqmp-r5fss
>>>>>>>>> +      - xlnx,versal-net-r52fss
>>>>>>>>>  
>>>>>>>>>    "#address-cells":
>>>>>>>>>      const: 2
>>>>>>>>> @@ -64,7 +66,9 @@ patternProperties:
>>>>>>>>>  
>>>>>>>>>      properties:
>>>>>>>>>        compatible:
>>>>>>>>> -        const: xlnx,zynqmp-r5f
>>>>>>>>> +        enum:
>>>>>>>>> +          - xlnx,zynqmp-r5f
>>>>>>>>> +          - xlnx,versal-net-r52f
>>>>>>>>>  
>>>>>>>>>        reg:
>>>>>>>>>          minItems: 1
>>>>>>>>> @@ -135,9 +139,11 @@ required:
>>>>>>>>>  allOf:
>>>>>>>>>    - if:
>>>>>>>>>        properties:
>>>>>>>>> -        xlnx,cluster-mode:
>>>>>>>>> -          enum:
>>>>>>>>> -            - 1
>>>>>>>>> +        compatible:
>>>>>>>>> +          contains:
>>>>>>>>> +            enum:
>>>>>>>>> +              - xlnx,versal-net-r52fss
>>>>>>>>
>>>>>>>> Why do you touch these lines?
>>>>>>>>
>>>>>>>>> +
>>>>>>>>>      then:
>>>>>>>>>        patternProperties:
>>>>>>>>>          "^r5f@[0-9a-f]+$":
>>>>>>>>> @@ -149,16 +155,14 @@ allOf:
>>>>>>>>>                items:
>>>>>>>>>                  - description: ATCM internal memory
>>>>>>>>>                  - description: BTCM internal memory
>>>>>>>>> -                - description: extra ATCM memory in lockstep mode
>>>>>>>>> -                - description: extra BTCM memory in lockstep mode
>>>>>>>>> +                - description: CTCM internal memory
>>>>>>>>>  
>>>>>>>>>              reg-names:
>>>>>>>>>                minItems: 1
>>>>>>>>>                items:
>>>>>>>>> -                - const: atcm0
>>>>>>>>> -                - const: btcm0
>>>>>>>>> -                - const: atcm1
>>>>>>>>> -                - const: btcm1
>>>>>>>>> +                - const: atcm
>>>>>>>>> +                - const: btcm
>>>>>>>>> +                - const: ctcm
>>>>>>>>>  
>>>>>>>>>              power-domains:
>>>>>>>>>                minItems: 2
>>>>>>>>> @@ -166,33 +170,70 @@ allOf:
>>>>>>>>>                  - description: RPU core power domain
>>>>>>>>>                  - description: ATCM power domain
>>>>>>>>>                  - description: BTCM power domain
>>>>>>>>> -                - description: second ATCM power domain
>>>>>>>>> -                - description: second BTCM power domain
>>>>>>>>> +                - description: CTCM power domain
>>>>>>>>>  
>>>>>>>>>      else:
>>>>>>>>> -      patternProperties:
>>>>>>>>> -        "^r5f@[0-9a-f]+$":
>>>>>>>>> -          type: object
>>>>>>>>> -
>>>>>>>>> -          properties:
>>>>>>>>> -            reg:
>>>>>>>>> -              minItems: 1
>>>>>>>>> -              items:
>>>>>>>>> -                - description: ATCM internal memory
>>>>>>>>> -                - description: BTCM internal memory
>>>>>>>>> -
>>>>>>>>> -            reg-names:
>>>>>>>>> -              minItems: 1
>>>>>>>>> -              items:
>>>>>>>>> -                - const: atcm0
>>>>>>>>> -                - const: btcm0
>>>>>>>>> -
>>>>>>>>> -            power-domains:
>>>>>>>>> -              minItems: 2
>>>>>>>>> -              items:
>>>>>>>>> -                - description: RPU core power domain
>>>>>>>>> -                - description: ATCM power domain
>>>>>>>>> -                - description: BTCM power domain
>>>>>>>>> +      allOf:
>>>>>>>>> +        - if:
>>>>>>>>> +            properties:
>>>>>>>>> +              xlnx,cluster-mode:
>>>>>>>>> +                enum:
>>>>>>>>> +                  - 1
>>>>>>>>
>>>>>>>> Whatever you did here, is not really readable. You have now multiple
>>>>>>>> if:then:if:then embedded.
>>>>>>>
>>>>>>> For ZynqMP platform, TCM can be configured differently in lockstep mode
>>>>>>> and split mode.
>>>>>>>
>>>>>>> For Versal-NET no such configuration is available, but new CTCM memory
>>>>>>> is added.
>>>>>>>
>>>>>>> So, I am trying to achieve following representation of TCM for both:
>>>>>>>
>>>>>>> if: versal-net compatible
>>>>>>> then:
>>>>>>>   ATCM - 64KB
>>>>>>>   BTCM - 32KB
>>>>>>>   CTCM - 32KB
>>>>>>>
>>>>>>> else: (ZynqMP compatible)
>>>>>>>   if:
>>>>>>>     xlnx,cluster-mode (lockstep mode)
>>>>>>>   then:
>>>>>>>     ATCM0 - 64KB
>>>>>>>     BTCM0 - 64KB
>>>>>>>     ATCM1 - 64KB
>>>>>>>     BTCM1 - 64KB
>>>>>>>   else: (split mode)
>>>>>>>     ATCM0 - 64KB
>>>>>>>     BTCM0 - 64KB
>>>>>>>
>>>>>>>
>>>>>>> If bindings are getting complicated, does it make sense to introduce
>>>>>>> new file for Versal-NET bindings? Let me know how you would like me
>>>>>>> to proceed.
>>>>>>
>>>>>> All this is broken in your previous patchset, but now we nicely see.
>>>>>>
>>>>>> No, this does not work like this. You do not have entirely different
>>>>>> programming models in one device, don't you?
>>>>>>
>>>>>
>>>>> I don't understand what do you mean? Programming model is same. Only number
>>>>> of TCMs are changing based on configuration and platform. I can certainly
>>>>> list different compatible for different platforms as requested. But other than
>>>>> that not sure what needs to be fixed.
>>>>
>>>> You cannot have same programming model with different memory mappings.
>>>> Anyway, please follow writing bindings rules: all of your different
>>>> devices must have dedicated compatible. I really though we talked about
>>>> two IPs on same SoC...
>>>
>>> I agree that Versal compatible should be added, I will do that in next revision.
>>>
>>> For ZynqMP case, it is two IPs on same SOC. In lockstep mode and split mode,
>>> same SOC is configuring TCM differently.
>>>
>>> How this should be resolved for Versal-NET ? Driver avoids such TCM configuration
>>> for Versal-NET.
>>
>> Binding should describe the hardware, not what driver is doing
>> currently, so the question is: does your device have such properties or
>> not? Anyway, you need compatible per each variant and each SoC
>> implementation.
> 
> Thanks for reviews.
> 
> Okay in that case I believe I should add one more property to current bindings for TCM
> configuration.
> 

I am not sure if you understand how IRC works... You sent me message on
IRC about this topic and shortly after you quit. So how am I supposed to
send reply? IRC does not work like that...

> From our discussion I conclude to following next steps:
> 
> 1) I will send Versal and Versal-NET support as part of previous series (v14) so we get
> bigger picture in the first place.
> 
> 2) Add separate compatible for versal platform.
> Use device compatible string to maintain
> backward compatibility and not machine (root node) compatible string.
> 
> 3) Add tcm,mode property in bindings and each device must configure TCM based on that
> property only and not based on compatible string.
> 
> 4) Versal-NET will disallow tcm,mode property in bindings as no such configuration is
> possible for that platform.

I really don't know your SoCs. What about Zynq? You keep using here
names all over the place, but I am not Xilinx maintainer.


Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: remoteproc: add Versal-NET platform

[Tanmay Shah]

On 3/22/24 12:44 AM, Krzysztof Kozlowski wrote:
> On 21/03/2024 16:13, Tanmay Shah wrote:
>> 
>> 
>> On 3/21/24 2:39 AM, Krzysztof Kozlowski wrote:
>>> On 20/03/2024 16:14, Tanmay Shah wrote:
>>>>
>>>>
>>>> On 3/20/24 2:40 AM, Krzysztof Kozlowski wrote:
>>>>> On 19/03/2024 15:42, Tanmay Shah wrote:
>>>>>>
>>>>>>
>>>>>> On 3/19/24 12:30 AM, Krzysztof Kozlowski wrote:
>>>>>>> On 19/03/2024 01:51, Tanmay Shah wrote:
>>>>>>>> Hello Krzysztof,
>>>>>>>>
>>>>>>>> Thanks for reviews. Please find my comments below.
>>>>>>>>
>>>>>>>> On 3/17/24 1:53 PM, Krzysztof Kozlowski wrote:
>>>>>>>>> On 15/03/2024 22:15, Tanmay Shah wrote:
>>>>>>>>>> AMD-Xilinx Versal-NET platform is successor of Versal platform. It
>>>>>>>>>> contains multiple clusters of cortex-R52 real-time processing units.
>>>>>>>>>> Each cluster contains two cores of cortex-R52 processors. Each cluster
>>>>>>>>>> can be configured in lockstep mode or split mode.
>>>>>>>>>>
>>>>>>>>>> Each R52 core is assigned 128KB of TCM memory. ATCM memory is 64KB, BTCM
>>>>>>>>>> and CTCM memoreis are 32KB each. Each TCM memory has its own dedicated
>>>>>>>>>> power-domain that needs to be requested before using it.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>>>>>>>> ---
>>>>>>>>>>  .../remoteproc/xlnx,zynqmp-r5fss.yaml         | 220 +++++++++++++++---
>>>>>>>>>>  1 file changed, 184 insertions(+), 36 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>>>>>>> index 711da0272250..55654ee02eef 100644
>>>>>>>>>> --- a/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/xlnx,zynqmp-r5fss.yaml
>>>>>>>>>> @@ -18,7 +18,9 @@ description: |
>>>>>>>>>>  
>>>>>>>>>>  properties:
>>>>>>>>>>    compatible:
>>>>>>>>>> -    const: xlnx,zynqmp-r5fss
>>>>>>>>>> +    enum:
>>>>>>>>>> +      - xlnx,zynqmp-r5fss
>>>>>>>>>> +      - xlnx,versal-net-r52fss
>>>>>>>>>>  
>>>>>>>>>>    "#address-cells":
>>>>>>>>>>      const: 2
>>>>>>>>>> @@ -64,7 +66,9 @@ patternProperties:
>>>>>>>>>>  
>>>>>>>>>>      properties:
>>>>>>>>>>        compatible:
>>>>>>>>>> -        const: xlnx,zynqmp-r5f
>>>>>>>>>> +        enum:
>>>>>>>>>> +          - xlnx,zynqmp-r5f
>>>>>>>>>> +          - xlnx,versal-net-r52f
>>>>>>>>>>  
>>>>>>>>>>        reg:
>>>>>>>>>>          minItems: 1
>>>>>>>>>> @@ -135,9 +139,11 @@ required:
>>>>>>>>>>  allOf:
>>>>>>>>>>    - if:
>>>>>>>>>>        properties:
>>>>>>>>>> -        xlnx,cluster-mode:
>>>>>>>>>> -          enum:
>>>>>>>>>> -            - 1
>>>>>>>>>> +        compatible:
>>>>>>>>>> +          contains:
>>>>>>>>>> +            enum:
>>>>>>>>>> +              - xlnx,versal-net-r52fss
>>>>>>>>>
>>>>>>>>> Why do you touch these lines?
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>>      then:
>>>>>>>>>>        patternProperties:
>>>>>>>>>>          "^r5f@[0-9a-f]+$":
>>>>>>>>>> @@ -149,16 +155,14 @@ allOf:
>>>>>>>>>>                items:
>>>>>>>>>>                  - description: ATCM internal memory
>>>>>>>>>>                  - description: BTCM internal memory
>>>>>>>>>> -                - description: extra ATCM memory in lockstep mode
>>>>>>>>>> -                - description: extra BTCM memory in lockstep mode
>>>>>>>>>> +                - description: CTCM internal memory
>>>>>>>>>>  
>>>>>>>>>>              reg-names:
>>>>>>>>>>                minItems: 1
>>>>>>>>>>                items:
>>>>>>>>>> -                - const: atcm0
>>>>>>>>>> -                - const: btcm0
>>>>>>>>>> -                - const: atcm1
>>>>>>>>>> -                - const: btcm1
>>>>>>>>>> +                - const: atcm
>>>>>>>>>> +                - const: btcm
>>>>>>>>>> +                - const: ctcm
>>>>>>>>>>  
>>>>>>>>>>              power-domains:
>>>>>>>>>>                minItems: 2
>>>>>>>>>> @@ -166,33 +170,70 @@ allOf:
>>>>>>>>>>                  - description: RPU core power domain
>>>>>>>>>>                  - description: ATCM power domain
>>>>>>>>>>                  - description: BTCM power domain
>>>>>>>>>> -                - description: second ATCM power domain
>>>>>>>>>> -                - description: second BTCM power domain
>>>>>>>>>> +                - description: CTCM power domain
>>>>>>>>>>  
>>>>>>>>>>      else:
>>>>>>>>>> -      patternProperties:
>>>>>>>>>> -        "^r5f@[0-9a-f]+$":
>>>>>>>>>> -          type: object
>>>>>>>>>> -
>>>>>>>>>> -          properties:
>>>>>>>>>> -            reg:
>>>>>>>>>> -              minItems: 1
>>>>>>>>>> -              items:
>>>>>>>>>> -                - description: ATCM internal memory
>>>>>>>>>> -                - description: BTCM internal memory
>>>>>>>>>> -
>>>>>>>>>> -            reg-names:
>>>>>>>>>> -              minItems: 1
>>>>>>>>>> -              items:
>>>>>>>>>> -                - const: atcm0
>>>>>>>>>> -                - const: btcm0
>>>>>>>>>> -
>>>>>>>>>> -            power-domains:
>>>>>>>>>> -              minItems: 2
>>>>>>>>>> -              items:
>>>>>>>>>> -                - description: RPU core power domain
>>>>>>>>>> -                - description: ATCM power domain
>>>>>>>>>> -                - description: BTCM power domain
>>>>>>>>>> +      allOf:
>>>>>>>>>> +        - if:
>>>>>>>>>> +            properties:
>>>>>>>>>> +              xlnx,cluster-mode:
>>>>>>>>>> +                enum:
>>>>>>>>>> +                  - 1
>>>>>>>>>
>>>>>>>>> Whatever you did here, is not really readable. You have now multiple
>>>>>>>>> if:then:if:then embedded.
>>>>>>>>
>>>>>>>> For ZynqMP platform, TCM can be configured differently in lockstep mode
>>>>>>>> and split mode.
>>>>>>>>
>>>>>>>> For Versal-NET no such configuration is available, but new CTCM memory
>>>>>>>> is added.
>>>>>>>>
>>>>>>>> So, I am trying to achieve following representation of TCM for both:
>>>>>>>>
>>>>>>>> if: versal-net compatible
>>>>>>>> then:
>>>>>>>>   ATCM - 64KB
>>>>>>>>   BTCM - 32KB
>>>>>>>>   CTCM - 32KB
>>>>>>>>
>>>>>>>> else: (ZynqMP compatible)
>>>>>>>>   if:
>>>>>>>>     xlnx,cluster-mode (lockstep mode)
>>>>>>>>   then:
>>>>>>>>     ATCM0 - 64KB
>>>>>>>>     BTCM0 - 64KB
>>>>>>>>     ATCM1 - 64KB
>>>>>>>>     BTCM1 - 64KB
>>>>>>>>   else: (split mode)
>>>>>>>>     ATCM0 - 64KB
>>>>>>>>     BTCM0 - 64KB
>>>>>>>>
>>>>>>>>
>>>>>>>> If bindings are getting complicated, does it make sense to introduce
>>>>>>>> new file for Versal-NET bindings? Let me know how you would like me
>>>>>>>> to proceed.
>>>>>>>
>>>>>>> All this is broken in your previous patchset, but now we nicely see.
>>>>>>>
>>>>>>> No, this does not work like this. You do not have entirely different
>>>>>>> programming models in one device, don't you?
>>>>>>>
>>>>>>
>>>>>> I don't understand what do you mean? Programming model is same. Only number
>>>>>> of TCMs are changing based on configuration and platform. I can certainly
>>>>>> list different compatible for different platforms as requested. But other than
>>>>>> that not sure what needs to be fixed.
>>>>>
>>>>> You cannot have same programming model with different memory mappings.
>>>>> Anyway, please follow writing bindings rules: all of your different
>>>>> devices must have dedicated compatible. I really though we talked about
>>>>> two IPs on same SoC...
>>>>
>>>> I agree that Versal compatible should be added, I will do that in next revision.
>>>>
>>>> For ZynqMP case, it is two IPs on same SOC. In lockstep mode and split mode,
>>>> same SOC is configuring TCM differently.
>>>>
>>>> How this should be resolved for Versal-NET ? Driver avoids such TCM configuration
>>>> for Versal-NET.
>>>
>>> Binding should describe the hardware, not what driver is doing
>>> currently, so the question is: does your device have such properties or
>>> not? Anyway, you need compatible per each variant and each SoC
>>> implementation.
>> 
>> Thanks for reviews.
>> 
>> Okay in that case I believe I should add one more property to current bindings for TCM
>> configuration.
>> 
> 
> I am not sure if you understand how IRC works... You sent me message on
> IRC about this topic and shortly after you quit. So how am I supposed to
> send reply? IRC does not work like that...
> 

Yeah, I am referring related documentation on IRC.

>> From our discussion I conclude to following next steps:
>> 
>> 1) I will send Versal and Versal-NET support as part of previous series (v14) so we get
>> bigger picture in the first place.
>> 
>> 2) Add separate compatible for versal platform.
>> Use device compatible string to maintain
>> backward compatibility and not machine (root node) compatible string.
>> 
>> 3) Add tcm,mode property in bindings and each device must configure TCM based on that
>> property only and not based on compatible string.
>> 
>> 4) Versal-NET will disallow tcm,mode property in bindings as no such configuration is
>> possible for that platform.
> 
> I really don't know your SoCs. What about Zynq? You keep using here
> names all over the place, but I am not Xilinx maintainer.
> 

Zynq doesn't have Cortex-R IP so this driver isn't needed on that.



> 
> Best regards,
> Krzysztof
> 

[PATCH 3/3] drivers: remoteproc: add Versal and Versal-NET support

[Tanmay Shah]

AMD-Xilinx Versal and Versal-NET are successor of ZynqMP platform. ZynqMP
remoteproc driver is mostly compatible with new platforms except few
platform specific differences.

Versal has same IP of cortex-R5 cores hence maintained compatible string
same as ZynqMP platform. However, hardcode TCM addresses are not
supported for new platforms and must be provided in device-tree as per
new bindings. This makes TCM representation data-driven and easy to
maintain. This check is provided in the driver.

For Versal-NET platform, TCM doesn't need to be configured in lockstep
mode or split mode. Hence that call to PMC firmware is avoided in the
driver for Versal-NET platform.

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---
 drivers/remoteproc/xlnx_r5_remoteproc.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
index d4a22caebaad..193bc159d1b4 100644
--- a/drivers/remoteproc/xlnx_r5_remoteproc.c
+++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
@@ -323,9 +323,12 @@ static int zynqmp_r5_set_mode(struct zynqmp_r5_core *r5_core,
 		return ret;
 	}
 
-	ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id, tcm_mode);
-	if (ret < 0)
-		dev_err(r5_core->dev, "failed to configure TCM\n");
+	/* TCM configuration is not needed in versal-net */
+	if (device_is_compatible(r5_core->dev, "xlnx,zynqmp-r5f")) {
+		ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id, tcm_mode);
+		if (ret < 0)
+			dev_err(r5_core->dev, "failed to configure TCM\n");
+	}
 
 	return ret;
 }
@@ -933,10 +936,17 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
 	int ret, i;
 
 	r5_core = cluster->r5_cores[0];
+
+	/*
+	 * New platforms must use device tree for TCM parsing.
+	 * Only ZynqMP uses hardcode TCM.
+	 */
 	if (of_find_property(r5_core->np, "reg", NULL))
 		ret = zynqmp_r5_get_tcm_node_from_dt(cluster);
-	else
+	else if (of_machine_is_compatible("xlnx,zynqmp"))
 		ret = zynqmp_r5_get_tcm_node(cluster);
+	else
+		ret = -EINVAL;
 
 	if (ret) {
 		dev_err(dev, "can't get tcm, err %d\n", ret);
@@ -1198,6 +1208,7 @@ static int zynqmp_r5_remoteproc_probe(struct platform_device *pdev)
 /* Match table for OF platform binding */
 static const struct of_device_id zynqmp_r5_remoteproc_match[] = {
 	{ .compatible = "xlnx,zynqmp-r5fss", },
+	{ .compatible = "xlnx,versal-net-r52fss", },
 	{ /* end of list */ },
 };
 MODULE_DEVICE_TABLE(of, zynqmp_r5_remoteproc_match);
-- 
2.25.1

Re: [PATCH 3/3] drivers: remoteproc: add Versal and Versal-NET support

[Krzysztof Kozlowski]

On 15/03/2024 22:15, Tanmay Shah wrote:
> AMD-Xilinx Versal and Versal-NET are successor of ZynqMP platform. ZynqMP
> remoteproc driver is mostly compatible with new platforms except few
> platform specific differences.
> 
> Versal has same IP of cortex-R5 cores hence maintained compatible string
> same as ZynqMP platform. However, hardcode TCM addresses are not
> supported for new platforms and must be provided in device-tree as per
> new bindings. This makes TCM representation data-driven and easy to
> maintain. This check is provided in the driver.
> 
> For Versal-NET platform, TCM doesn't need to be configured in lockstep
> mode or split mode. Hence that call to PMC firmware is avoided in the
> driver for Versal-NET platform.
> 
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> ---
>  drivers/remoteproc/xlnx_r5_remoteproc.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> index d4a22caebaad..193bc159d1b4 100644
> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> @@ -323,9 +323,12 @@ static int zynqmp_r5_set_mode(struct zynqmp_r5_core *r5_core,
>  		return ret;
>  	}
>  
> -	ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id, tcm_mode);
> -	if (ret < 0)
> -		dev_err(r5_core->dev, "failed to configure TCM\n");
> +	/* TCM configuration is not needed in versal-net */
> +	if (device_is_compatible(r5_core->dev, "xlnx,zynqmp-r5f")) {
> +		ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id, tcm_mode);
> +		if (ret < 0)
> +			dev_err(r5_core->dev, "failed to configure TCM\n");
> +	}
>  
>  	return ret;
>  }
> @@ -933,10 +936,17 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
>  	int ret, i;
>  
>  	r5_core = cluster->r5_cores[0];
> +
> +	/*
> +	 * New platforms must use device tree for TCM parsing.
> +	 * Only ZynqMP uses hardcode TCM.
> +	 */
>  	if (of_find_property(r5_core->np, "reg", NULL))
>  		ret = zynqmp_r5_get_tcm_node_from_dt(cluster);
> -	else
> +	else if (of_machine_is_compatible("xlnx,zynqmp"))
>  		ret = zynqmp_r5_get_tcm_node(cluster);

That's poor code. Your drivers should not depend on platform. I don't
understand why you need to do this and how is even related to this patch.


Best regards,
Krzysztof

Re: [PATCH 3/3] drivers: remoteproc: add Versal and Versal-NET support

[Tanmay Shah]

On 3/17/24 1:55 PM, Krzysztof Kozlowski wrote:
> On 15/03/2024 22:15, Tanmay Shah wrote:
>> AMD-Xilinx Versal and Versal-NET are successor of ZynqMP platform. ZynqMP
>> remoteproc driver is mostly compatible with new platforms except few
>> platform specific differences.
>> 
>> Versal has same IP of cortex-R5 cores hence maintained compatible string
>> same as ZynqMP platform. However, hardcode TCM addresses are not
>> supported for new platforms and must be provided in device-tree as per
>> new bindings. This makes TCM representation data-driven and easy to
>> maintain. This check is provided in the driver.
>> 
>> For Versal-NET platform, TCM doesn't need to be configured in lockstep
>> mode or split mode. Hence that call to PMC firmware is avoided in the
>> driver for Versal-NET platform.
>> 
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>> ---
>>  drivers/remoteproc/xlnx_r5_remoteproc.c | 19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
>> index d4a22caebaad..193bc159d1b4 100644
>> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
>> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
>> @@ -323,9 +323,12 @@ static int zynqmp_r5_set_mode(struct zynqmp_r5_core *r5_core,
>>  		return ret;
>>  	}
>>  
>> -	ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id, tcm_mode);
>> -	if (ret < 0)
>> -		dev_err(r5_core->dev, "failed to configure TCM\n");
>> +	/* TCM configuration is not needed in versal-net */
>> +	if (device_is_compatible(r5_core->dev, "xlnx,zynqmp-r5f")) {
>> +		ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id, tcm_mode);
>> +		if (ret < 0)
>> +			dev_err(r5_core->dev, "failed to configure TCM\n");
>> +	}
>>  
>>  	return ret;
>>  }
>> @@ -933,10 +936,17 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
>>  	int ret, i;
>>  
>>  	r5_core = cluster->r5_cores[0];
>> +
>> +	/*
>> +	 * New platforms must use device tree for TCM parsing.
>> +	 * Only ZynqMP uses hardcode TCM.
>> +	 */
>>  	if (of_find_property(r5_core->np, "reg", NULL))
>>  		ret = zynqmp_r5_get_tcm_node_from_dt(cluster);
>> -	else
>> +	else if (of_machine_is_compatible("xlnx,zynqmp"))
>>  		ret = zynqmp_r5_get_tcm_node(cluster);
> 
> That's poor code. Your drivers should not depend on platform. I don't
> understand why you need to do this and how is even related to this patch.

You are correct, ideally this shouldn't be needed. However, this driver contains
hardcode TCM addresses that were used before TCM bindings were designed and available in
device-tree. This check is provided to maintain backward compatibility with device-tree
where TCM isn't expected.

For new platforms (Versal and Versal-NET) TCM must be provided in device-tree and for
ZynqMP if it's not in device-tree then to maintain backward compatibility hardcode
addresses are used.

Thanks.


> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 3/3] drivers: remoteproc: add Versal and Versal-NET support

[Krzysztof Kozlowski]

On 19/03/2024 02:06, Tanmay Shah wrote:
> 
> 
> On 3/17/24 1:55 PM, Krzysztof Kozlowski wrote:
>> On 15/03/2024 22:15, Tanmay Shah wrote:
>>> AMD-Xilinx Versal and Versal-NET are successor of ZynqMP platform. ZynqMP
>>> remoteproc driver is mostly compatible with new platforms except few
>>> platform specific differences.
>>>
>>> Versal has same IP of cortex-R5 cores hence maintained compatible string
>>> same as ZynqMP platform. However, hardcode TCM addresses are not
>>> supported for new platforms and must be provided in device-tree as per
>>> new bindings. This makes TCM representation data-driven and easy to
>>> maintain. This check is provided in the driver.
>>>
>>> For Versal-NET platform, TCM doesn't need to be configured in lockstep
>>> mode or split mode. Hence that call to PMC firmware is avoided in the
>>> driver for Versal-NET platform.
>>>
>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>> ---
>>>  drivers/remoteproc/xlnx_r5_remoteproc.c | 19 +++++++++++++++----
>>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
>>> index d4a22caebaad..193bc159d1b4 100644
>>> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
>>> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
>>> @@ -323,9 +323,12 @@ static int zynqmp_r5_set_mode(struct zynqmp_r5_core *r5_core,
>>>  		return ret;
>>>  	}
>>>  
>>> -	ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id, tcm_mode);
>>> -	if (ret < 0)
>>> -		dev_err(r5_core->dev, "failed to configure TCM\n");
>>> +	/* TCM configuration is not needed in versal-net */
>>> +	if (device_is_compatible(r5_core->dev, "xlnx,zynqmp-r5f")) {
>>> +		ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id, tcm_mode);
>>> +		if (ret < 0)
>>> +			dev_err(r5_core->dev, "failed to configure TCM\n");
>>> +	}
>>>  
>>>  	return ret;
>>>  }
>>> @@ -933,10 +936,17 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
>>>  	int ret, i;
>>>  
>>>  	r5_core = cluster->r5_cores[0];
>>> +
>>> +	/*
>>> +	 * New platforms must use device tree for TCM parsing.
>>> +	 * Only ZynqMP uses hardcode TCM.
>>> +	 */
>>>  	if (of_find_property(r5_core->np, "reg", NULL))
>>>  		ret = zynqmp_r5_get_tcm_node_from_dt(cluster);
>>> -	else
>>> +	else if (of_machine_is_compatible("xlnx,zynqmp"))
>>>  		ret = zynqmp_r5_get_tcm_node(cluster);
>>
>> That's poor code. Your drivers should not depend on platform. I don't
>> understand why you need to do this and how is even related to this patch.
> 
> You are correct, ideally this shouldn't be needed. However, this driver contains
> hardcode TCM addresses that were used before TCM bindings were designed and available in
> device-tree. This check is provided to maintain backward compatibility with device-tree
> where TCM isn't expected.
> 
> For new platforms (Versal and Versal-NET) TCM must be provided in device-tree and for
> ZynqMP if it's not in device-tree then to maintain backward compatibility hardcode
> addresses are used.

That does not work like this. You cannot bind to some sort of different
compatible. If you disagree, please list the compatibles the driver
binds to.

Best regards,
Krzysztof

Re: [PATCH 3/3] drivers: remoteproc: add Versal and Versal-NET support

[Tanmay Shah]

On 3/19/24 12:25 AM, Krzysztof Kozlowski wrote:
> On 19/03/2024 02:06, Tanmay Shah wrote:
>> 
>> 
>> On 3/17/24 1:55 PM, Krzysztof Kozlowski wrote:
>>> On 15/03/2024 22:15, Tanmay Shah wrote:
>>>> AMD-Xilinx Versal and Versal-NET are successor of ZynqMP platform. ZynqMP
>>>> remoteproc driver is mostly compatible with new platforms except few
>>>> platform specific differences.
>>>>
>>>> Versal has same IP of cortex-R5 cores hence maintained compatible string
>>>> same as ZynqMP platform. However, hardcode TCM addresses are not
>>>> supported for new platforms and must be provided in device-tree as per
>>>> new bindings. This makes TCM representation data-driven and easy to
>>>> maintain. This check is provided in the driver.
>>>>
>>>> For Versal-NET platform, TCM doesn't need to be configured in lockstep
>>>> mode or split mode. Hence that call to PMC firmware is avoided in the
>>>> driver for Versal-NET platform.
>>>>
>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>> ---
>>>>  drivers/remoteproc/xlnx_r5_remoteproc.c | 19 +++++++++++++++----
>>>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
>>>> index d4a22caebaad..193bc159d1b4 100644
>>>> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
>>>> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
>>>> @@ -323,9 +323,12 @@ static int zynqmp_r5_set_mode(struct zynqmp_r5_core *r5_core,
>>>>  		return ret;
>>>>  	}
>>>>  
>>>> -	ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id, tcm_mode);
>>>> -	if (ret < 0)
>>>> -		dev_err(r5_core->dev, "failed to configure TCM\n");
>>>> +	/* TCM configuration is not needed in versal-net */
>>>> +	if (device_is_compatible(r5_core->dev, "xlnx,zynqmp-r5f")) {
>>>> +		ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id, tcm_mode);
>>>> +		if (ret < 0)
>>>> +			dev_err(r5_core->dev, "failed to configure TCM\n");
>>>> +	}
>>>>  
>>>>  	return ret;
>>>>  }
>>>> @@ -933,10 +936,17 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
>>>>  	int ret, i;
>>>>  
>>>>  	r5_core = cluster->r5_cores[0];
>>>> +
>>>> +	/*
>>>> +	 * New platforms must use device tree for TCM parsing.
>>>> +	 * Only ZynqMP uses hardcode TCM.
>>>> +	 */
>>>>  	if (of_find_property(r5_core->np, "reg", NULL))
>>>>  		ret = zynqmp_r5_get_tcm_node_from_dt(cluster);
>>>> -	else
>>>> +	else if (of_machine_is_compatible("xlnx,zynqmp"))
>>>>  		ret = zynqmp_r5_get_tcm_node(cluster);
>>>
>>> That's poor code. Your drivers should not depend on platform. I don't
>>> understand why you need to do this and how is even related to this patch.
>> 
>> You are correct, ideally this shouldn't be needed. However, this driver contains
>> hardcode TCM addresses that were used before TCM bindings were designed and available in
>> device-tree. This check is provided to maintain backward compatibility with device-tree
>> where TCM isn't expected.
>> 
>> For new platforms (Versal and Versal-NET) TCM must be provided in device-tree and for
>> ZynqMP if it's not in device-tree then to maintain backward compatibility hardcode
>> addresses are used.
> 
> That does not work like this. You cannot bind to some sort of different
> compatible. If you disagree, please list the compatibles the driver
> binds to.

Okay understood. I believe then removing hardcode addresses makes more sense in that
case. So, driver will become same for all the devices.

> 
> Best regards,
> Krzysztof
>