This patch introduces bindings for TCM memory address space on AMD-xilinx
platforms. As of now TCM addresses are hardcoded in xilinx remoteproc
driver. This bindings will help in defining TCM in device-tree and
make it's access platform agnostic and data-driven from the driver.
Signed-off-by: Tanmay Shah <[email protected]>
---
.../devicetree/bindings/sram/xlnx,tcm.yaml | 137 ++++++++++++++++++
1 file changed, 137 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
diff --git a/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml b/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
new file mode 100644
index 000000000000..02d17026fb1f
--- /dev/null
+++ b/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
@@ -0,0 +1,137 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sram/xlnx,tcm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Tightly Coupled Memory (TCM)
+
+maintainers:
+ - Tanmay Shah <[email protected]>
+
+description: |
+ Tightly Coupled Memory(TCM) is available on AMD-Xilinx paltforms for ARM
+ cortex remote processors to use. It is low-latency memory that provide
+ predictable instruction execution and predictable data load/store timing.
+ TCM can be configured in lockstep mode or split mode. In split mode
+ configuration each RPU core has its own set of ATCM and BTCM memories and in
+ lockstep mode redundant processor's TCM become available to lockstep
+ processor. So In lockstep mode ATCM and BTCM size is increased.
+
+properties:
+ $nodename:
+ pattern: "sram-[0-9a-f]+$"
+
+patternProperties:
+ "^tcm-[a-z]+@[0-9a-f]+$":
+ type: object
+ description: |
+ During the split mode, each RPU core has its own set of ATCM and BTCM memory
+
+ During the lock-step operation, the TCMs that are associated with the
+ redundant processor become available to the lock-step processor.
+ For example if each individual processor has 64KB ATCM, then in lockstep mode
+ The size of ATCM become 128KB. Same for BTCM. tcm-lockstep node represents
+ TCM address space in lockstep mode. tcm-core@x node specfies each core's
+ TCM address space in split mode.
+
+ properties:
+ compatible:
+ oneOf:
+ - items:
+ - enum:
+ - xlnx,tcm-lockstep
+ - xlnx,tcm-split
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 1
+
+ reg:
+ items:
+ - description: |
+ ATCM Memory address space. An ATCM typically holds interrupt or
+ exception code that must be accessed at high speed, without any
+ potential delay resulting from a cache miss.
+ RPU on AMD-Xilinx platform can also fetch data from ATCM
+ - description: |
+ BTCM Memory address space. A BTCM typically holds a block of data
+ for intensive processing, such as audio or video processing. RPU on
+ AMD-Xilinx Platforms can also fetch Code (Instructions) from BTCM
+
+ reg-names:
+ items:
+ - const: atcm
+ - const: btcm
+
+ ranges: true
+
+ power-domains:
+ maxItems: 8
+ items:
+ - description: list of ATCM Power domains
+ - description: list of BTCM Power domains
+ additionalItems: true
+
+ required:
+ - compatible
+ - '#address-cells'
+ - '#size-cells'
+ - reg
+ - ranges
+ - power-domains
+ unevaluatedProperties: false
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/power/xlnx-zynqmp-power.h>
+
+ amba {
+ sram@ffe00000 {
+ tcm-lockstep@ffe00000 {
+ compatible = "xlnx,tcm-lockstep";
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ reg = <0xffe00000 0x20000>, <0xffe20000 0x20000>;
+ reg-names = "atcm", "btcm";
+ ranges = <0x0 0xffe00000 0x20000>, <0x20000 0xffe20000 0x20000>;
+ power-domains = <&zynqmp_firmware PD_R5_0_ATCM>,
+ <&zynqmp_firmware PD_R5_1_ATCM>,
+ <&zynqmp_firmware PD_R5_0_BTCM>,
+ <&zynqmp_firmware PD_R5_1_BTCM>;
+ };
+
+ tcm-core@0 {
+ compatible = "xlnx,tcm-split";
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ reg = <0xffe00000 0x10000>, <0xffe20000 0x10000>;
+ reg-names = "atcm", "btcm";
+ ranges = <0x0 0xffe00000 0x10000>, <0x20000 0xffe20000 0x10000>;
+ power-domains = <&zynqmp_firmware PD_R5_0_ATCM>,
+ <&zynqmp_firmware PD_R5_0_BTCM>;
+ };
+
+ tcm-core@1 {
+ compatible = "xlnx,tcm-split";
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ reg = <0xffe90000 0x10000>, <0xffeb0000 0x10000>;
+ reg-names = "atcm", "btcm";
+ ranges = <0x0 0xffe90000 0x10000>, <0x20000 0xffeb0000 0x10000>;
+ power-domains = <&zynqmp_firmware PD_R5_1_ATCM>,
+ <&zynqmp_firmware PD_R5_1_BTCM>;
+ };
+ };
+ };
+...
base-commit: 6b31ffe9c8b9947d6d3552d6e10752fd96d0f80f
--
2.25.1
On 13/01/2023 08:30, Tanmay Shah wrote:
> This patch introduces bindings for TCM memory address space on AMD-xilinx
> platforms. As of now TCM addresses are hardcoded in xilinx remoteproc
> driver. This bindings will help in defining TCM in device-tree and
> make it's access platform agnostic and data-driven from the driver.
>
Subject: drop second/last, redundant "bindings". The "dt-bindings"
prefix is already stating that these are bindings.
Where is driver or DTS? Are you now adding a dead binding without users?
Best regards,
Krzysztof
On 13/01/2023 08:30, Tanmay Shah wrote:
> This patch introduces bindings for TCM memory address space on AMD-xilinx
> platforms. As of now TCM addresses are hardcoded in xilinx remoteproc
> driver. This bindings will help in defining TCM in device-tree and
> make it's access platform agnostic and data-driven from the driver.
>
> Signed-off-by: Tanmay Shah <[email protected]>
> ---
> .../devicetree/bindings/sram/xlnx,tcm.yaml | 137 ++++++++++++++++++
> 1 file changed, 137 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>
> diff --git a/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml b/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
> new file mode 100644
> index 000000000000..02d17026fb1f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
> @@ -0,0 +1,137 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sram/xlnx,tcm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Tightly Coupled Memory (TCM)
> +
> +maintainers:
> + - Tanmay Shah <[email protected]>
> +
> +description: |
> + Tightly Coupled Memory(TCM) is available on AMD-Xilinx paltforms for ARM
> + cortex remote processors to use. It is low-latency memory that provide
> + predictable instruction execution and predictable data load/store timing.
> + TCM can be configured in lockstep mode or split mode. In split mode
> + configuration each RPU core has its own set of ATCM and BTCM memories and in
> + lockstep mode redundant processor's TCM become available to lockstep
> + processor. So In lockstep mode ATCM and BTCM size is increased.
> +
> +properties:
> + $nodename:
> + pattern: "sram-[0-9a-f]+$"
Drop node name requirement.
Why do you need sram node at all?
> +
> +patternProperties:
> + "^tcm-[a-z]+@[0-9a-f]+$":
> + type: object
> + description: |
> + During the split mode, each RPU core has its own set of ATCM and BTCM memory
> +
> + During the lock-step operation, the TCMs that are associated with the
> + redundant processor become available to the lock-step processor.
> + For example if each individual processor has 64KB ATCM, then in lockstep mode
> + The size of ATCM become 128KB. Same for BTCM. tcm-lockstep node represents
> + TCM address space in lockstep mode. tcm-core@x node specfies each core's
> + TCM address space in split mode.
> +
> + properties:
> + compatible:
> + oneOf:
This is not oneOf.
> + - items:
and you do not have more than one item.
> + - enum:
> + - xlnx,tcm-lockstep
> + - xlnx,tcm-split
compatible describes hardware, not configuration. What you encode here
does not fit compatible.
> +
> + "#address-cells":
Use consistent quotes, either " or '
> + const: 1
> +
> + "#size-cells":
> + const: 1
> +
> + reg:
> + items:
> + - description: |
> + ATCM Memory address space. An ATCM typically holds interrupt or
> + exception code that must be accessed at high speed, without any
> + potential delay resulting from a cache miss.
> + RPU on AMD-Xilinx platform can also fetch data from ATCM
> + - description: |
> + BTCM Memory address space. A BTCM typically holds a block of data
> + for intensive processing, such as audio or video processing. RPU on
> + AMD-Xilinx Platforms can also fetch Code (Instructions) from BTCM
> +
> + reg-names:
> + items:
> + - const: atcm
> + - const: btcm
> +
> + ranges: true
> +
> + power-domains:
> + maxItems: 8
> + items:
> + - description: list of ATCM Power domains
> + - description: list of BTCM Power domains
> + additionalItems: true
And what are the rest?
> +
> + required:
> + - compatible
> + - '#address-cells'
> + - '#size-cells'
> + - reg
> + - ranges
> + - power-domains
> + unevaluatedProperties: false
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/power/xlnx-zynqmp-power.h>
> +
> + amba {
Drop.
> + sram@ffe00000 {
This does not match your bindings.
> + tcm-lockstep@ffe00000 {
> + compatible = "xlnx,tcm-lockstep";
> +
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + reg = <0xffe00000 0x20000>, <0xffe20000 0x20000>;
> + reg-names = "atcm", "btcm";
> + ranges = <0x0 0xffe00000 0x20000>, <0x20000 0xffe20000 0x20000>;
> + power-domains = <&zynqmp_firmware PD_R5_0_ATCM>,
> + <&zynqmp_firmware PD_R5_1_ATCM>,
This is BTCM domain according to your binding. Your binding here is
probably wrong and does not match real DTS.
> + <&zynqmp_firmware PD_R5_0_BTCM>,
> + <&zynqmp_firmware PD_R5_1_BTCM>;
> + };
> +
> + tcm-core@0 {
> + compatible = "xlnx,tcm-split";
> +
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + reg = <0xffe00000 0x10000>, <0xffe20000 0x10000>;
> + reg-names = "atcm", "btcm";
> + ranges = <0x0 0xffe00000 0x10000>, <0x20000 0xffe20000 0x10000>;
> + power-domains = <&zynqmp_firmware PD_R5_0_ATCM>,
> + <&zynqmp_firmware PD_R5_0_BTCM>;
> + };
> +
> + tcm-core@1 {
> + compatible = "xlnx,tcm-split";
> +
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + reg = <0xffe90000 0x10000>, <0xffeb0000 0x10000>;
> + reg-names = "atcm", "btcm";
> + ranges = <0x0 0xffe90000 0x10000>, <0x20000 0xffeb0000 0x10000>;
> + power-domains = <&zynqmp_firmware PD_R5_1_ATCM>,
> + <&zynqmp_firmware PD_R5_1_BTCM>;
> + };
> + };
> + };
> +...
>
> base-commit: 6b31ffe9c8b9947d6d3552d6e10752fd96d0f80f
Best regards,
Krzysztof
On Thu, 12 Jan 2023 23:30:46 -0800, Tanmay Shah wrote:
> This patch introduces bindings for TCM memory address space on AMD-xilinx
> platforms. As of now TCM addresses are hardcoded in xilinx remoteproc
> driver. This bindings will help in defining TCM in device-tree and
> make it's access platform agnostic and data-driven from the driver.
>
> Signed-off-by: Tanmay Shah <[email protected]>
> ---
> .../devicetree/bindings/sram/xlnx,tcm.yaml | 137 ++++++++++++++++++
> 1 file changed, 137 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dts:28.21-70: Warning (reg_format): /example-0/amba/sram@ffe00000/tcm-lockstep@ffe00000:reg: property has invalid length (16 bytes) (#address-cells == 2, #size-cells == 1)
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dts:43.21-70: Warning (reg_format): /example-0/amba/sram@ffe00000/tcm-core@0:reg: property has invalid length (16 bytes) (#address-cells == 2, #size-cells == 1)
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dts:56.21-70: Warning (reg_format): /example-0/amba/sram@ffe00000/tcm-core@1:reg: property has invalid length (16 bytes) (#address-cells == 2, #size-cells == 1)
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dts:30.21-85: Warning (ranges_format): /example-0/amba/sram@ffe00000/tcm-lockstep@ffe00000:ranges: "ranges" property has invalid length (24 bytes) (parent #address-cells == 2, child #address-cells == 1, #size-cells == 1)
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dts:45.21-85: Warning (ranges_format): /example-0/amba/sram@ffe00000/tcm-core@0:ranges: "ranges" property has invalid length (24 bytes) (parent #address-cells == 2, child #address-cells == 1, #size-cells == 1)
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dts:58.21-85: Warning (ranges_format): /example-0/amba/sram@ffe00000/tcm-core@1:ranges: "ranges" property has invalid length (24 bytes) (parent #address-cells == 2, child #address-cells == 1, #size-cells == 1)
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dts:21.27-62.15: Warning (unit_address_vs_reg): /example-0/amba/sram@ffe00000: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dtb: Warning (pci_device_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dtb: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dtb: Warning (simple_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dtb: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dtb: Warning (spi_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dts:22.39-35.19: Warning (avoid_default_addr_size): /example-0/amba/sram@ffe00000/tcm-lockstep@ffe00000: Relying on default #address-cells value
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dts:22.39-35.19: Warning (avoid_default_addr_size): /example-0/amba/sram@ffe00000/tcm-lockstep@ffe00000: Relying on default #size-cells value
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dts:37.28-48.19: Warning (avoid_default_addr_size): /example-0/amba/sram@ffe00000/tcm-core@0: Relying on default #address-cells value
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dts:37.28-48.19: Warning (avoid_default_addr_size): /example-0/amba/sram@ffe00000/tcm-core@0: Relying on default #size-cells value
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dts:50.28-61.19: Warning (avoid_default_addr_size): /example-0/amba/sram@ffe00000/tcm-core@1: Relying on default #address-cells value
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dts:50.28-61.19: Warning (avoid_default_addr_size): /example-0/amba/sram@ffe00000/tcm-core@1: Relying on default #size-cells value
Documentation/devicetree/bindings/sram/xlnx,tcm.example.dtb: Warning (unique_unit_address_if_enabled): Failed prerequisite 'avoid_default_addr_size'
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sram/xlnx,tcm.example.dtb: sram@ffe00000: tcm-lockstep@ffe00000:reg: [[4292870144, 131072], [4293001216, 131072]] is too long
From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mtd/mtd.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sram/xlnx,tcm.example.dtb: sram@ffe00000: tcm-core@0:reg: [[4292870144, 65536], [4293001216, 65536]] is too long
From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mtd/mtd.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sram/xlnx,tcm.example.dtb: sram@ffe00000: tcm-core@1:reg: [[4293459968, 65536], [4293591040, 65536]] is too long
From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mtd/mtd.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Hi Krzysztof Thanks for your reviews.
Please find my comments below.
On 1/12/23 11:52 PM, Krzysztof Kozlowski wrote:
> On 13/01/2023 08:30, Tanmay Shah wrote:
>> This patch introduces bindings for TCM memory address space on AMD-xilinx
>> platforms. As of now TCM addresses are hardcoded in xilinx remoteproc
>> driver. This bindings will help in defining TCM in device-tree and
>> make it's access platform agnostic and data-driven from the driver.
>>
>> Signed-off-by: Tanmay Shah <[email protected]>
>> ---
>> .../devicetree/bindings/sram/xlnx,tcm.yaml | 137 ++++++++++++++++++
>> 1 file changed, 137 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml b/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>> new file mode 100644
>> index 000000000000..02d17026fb1f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>> @@ -0,0 +1,137 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/sram/xlnx,tcm.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Tightly Coupled Memory (TCM)
>> +
>> +maintainers:
>> + - Tanmay Shah <[email protected]>
>> +
>> +description: |
>> + Tightly Coupled Memory(TCM) is available on AMD-Xilinx paltforms for ARM
>> + cortex remote processors to use. It is low-latency memory that provide
>> + predictable instruction execution and predictable data load/store timing.
>> + TCM can be configured in lockstep mode or split mode. In split mode
>> + configuration each RPU core has its own set of ATCM and BTCM memories and in
>> + lockstep mode redundant processor's TCM become available to lockstep
>> + processor. So In lockstep mode ATCM and BTCM size is increased.
>> +
>> +properties:
>> + $nodename:
>> + pattern: "sram-[0-9a-f]+$"
> Drop node name requirement.
> Why do you need sram node at all?
I will remove sram- node. However, it device-tree I was planning to put
all TCM nodes under single node for example:
tcm {
tcm-lockstep {
};
tcm-core@0 {
};
};
The top-most tcm node I assumed sram node. So I kept sram@xxxx
>> +
>> +patternProperties:
>> + "^tcm-[a-z]+@[0-9a-f]+$":
>> + type: object
>> + description: |
>> + During the split mode, each RPU core has its own set of ATCM and BTCM memory
>> +
>> + During the lock-step operation, the TCMs that are associated with the
>> + redundant processor become available to the lock-step processor.
>> + For example if each individual processor has 64KB ATCM, then in lockstep mode
>> + The size of ATCM become 128KB. Same for BTCM. tcm-lockstep node represents
>> + TCM address space in lockstep mode. tcm-core@x node specfies each core's
>> + TCM address space in split mode.
>> +
>> + properties:
>> + compatible:
>> + oneOf:
> This is not oneOf.
>
>> + - items:
> and you do not have more than one item.
>
>> + - enum:
>> + - xlnx,tcm-lockstep
>> + - xlnx,tcm-split
> compatible describes hardware, not configuration. What you encode here
> does not fit compatible.
I see. So, only xlnx,tcm is enough.
>
>> +
>> + "#address-cells":
> Use consistent quotes, either " or '
Ack.
>
>> + const: 1
>> +
>> + "#size-cells":
>> + const: 1
>> +
>> + reg:
>> + items:
>> + - description: |
>> + ATCM Memory address space. An ATCM typically holds interrupt or
>> + exception code that must be accessed at high speed, without any
>> + potential delay resulting from a cache miss.
>> + RPU on AMD-Xilinx platform can also fetch data from ATCM
>> + - description: |
>> + BTCM Memory address space. A BTCM typically holds a block of data
>> + for intensive processing, such as audio or video processing. RPU on
>> + AMD-Xilinx Platforms can also fetch Code (Instructions) from BTCM
>> +
>> + reg-names:
>> + items:
>> + - const: atcm
>> + - const: btcm
>> +
>> + ranges: true
>> +
>> + power-domains:
>> + maxItems: 8
>> + items:
>> + - description: list of ATCM Power domains
>> + - description: list of BTCM Power domains
>> + additionalItems: true
> And what are the rest?
As both items are list, we should be able to include more than one
power-domain I believe.
So first item I am trying to create list of ATCM power domains.
In split mode, first item is ATCM power-domain and second item is BTCM
power domain.
However, In lockstep mode, second core's TCM physically relocates and
two ATCM combines and
makes single region of ATCM. However, their power-domains remains same.
So, In lockstep mode, first two banks are ATCM and so, first two items
are ATCM power-domains.
I am not sure best way to represent this. But, first itmes is list.
So, I am assuming list of all ATCM power-domains possible.
>
>> +
>> + required:
>> + - compatible
>> + - '#address-cells'
>> + - '#size-cells'
>> + - reg
>> + - ranges
>> + - power-domains
>> + unevaluatedProperties: false
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/power/xlnx-zynqmp-power.h>
>> +
>> + amba {
> Drop.
ACK
>> + sram@ffe00000 {
> This does not match your bindings.
Ok. This was node-name. I will remove it from example.
>
>> + tcm-lockstep@ffe00000 {
>> + compatible = "xlnx,tcm-lockstep";
>> +
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + reg = <0xffe00000 0x20000>, <0xffe20000 0x20000>;
>> + reg-names = "atcm", "btcm";
>> + ranges = <0x0 0xffe00000 0x20000>, <0x20000 0xffe20000 0x20000>;
>> + power-domains = <&zynqmp_firmware PD_R5_0_ATCM>,
>> + <&zynqmp_firmware PD_R5_1_ATCM>,
> This is BTCM domain according to your binding. Your binding here is
> probably wrong and does not match real DTS.
As explained above, the first Item is list of all ATCM power-domains.
So, I kept both ATCM power-domains for lockstep mode.
We don't have dts nodes for TCM yet. We are using hard-coded address in
xlnx_r5_remoteproc.c driver.
As the bindings are new, I was hoping to introduce dts nodes once
bindings are designed right.
>
>> + <&zynqmp_firmware PD_R5_0_BTCM>,
>> + <&zynqmp_firmware PD_R5_1_BTCM>;
>> + };
>> +
>> + tcm-core@0 {
>> + compatible = "xlnx,tcm-split";
>> +
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + reg = <0xffe00000 0x10000>, <0xffe20000 0x10000>;
>> + reg-names = "atcm", "btcm";
>> + ranges = <0x0 0xffe00000 0x10000>, <0x20000 0xffe20000 0x10000>;
>> + power-domains = <&zynqmp_firmware PD_R5_0_ATCM>,
>> + <&zynqmp_firmware PD_R5_0_BTCM>;
>> + };
>> +
>> + tcm-core@1 {
>> + compatible = "xlnx,tcm-split";
>> +
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + reg = <0xffe90000 0x10000>, <0xffeb0000 0x10000>;
>> + reg-names = "atcm", "btcm";
>> + ranges = <0x0 0xffe90000 0x10000>, <0x20000 0xffeb0000 0x10000>;
>> + power-domains = <&zynqmp_firmware PD_R5_1_ATCM>,
>> + <&zynqmp_firmware PD_R5_1_BTCM>;
>> + };
>> + };
>> + };
>> +...
>>
>> base-commit: 6b31ffe9c8b9947d6d3552d6e10752fd96d0f80f
> Best regards,
> Krzysztof
>
On 1/12/23 11:52 PM, Krzysztof Kozlowski wrote:
> On 13/01/2023 08:30, Tanmay Shah wrote:
>> This patch introduces bindings for TCM memory address space on AMD-xilinx
>> platforms. As of now TCM addresses are hardcoded in xilinx remoteproc
>> driver. This bindings will help in defining TCM in device-tree and
>> make it's access platform agnostic and data-driven from the driver.
>>
> Subject: drop second/last, redundant "bindings". The "dt-bindings"
> prefix is already stating that these are bindings.
Ack.
>
> Where is driver or DTS? Are you now adding a dead binding without users?
TCM is used by drivers/remoteproc/xlnx_r5_remoteproc.c driver. Howerver,
we have hardcode addresses in TCM as bindings are not available yet.
>
> Best regards,
> Krzysztof
>
On 13/01/2023 19:08, Tanmay Shah wrote:
>
> On 1/12/23 11:52 PM, Krzysztof Kozlowski wrote:
>> On 13/01/2023 08:30, Tanmay Shah wrote:
>>> This patch introduces bindings for TCM memory address space on AMD-xilinx
>>> platforms. As of now TCM addresses are hardcoded in xilinx remoteproc
>>> driver. This bindings will help in defining TCM in device-tree and
>>> make it's access platform agnostic and data-driven from the driver.
>>>
>> Subject: drop second/last, redundant "bindings". The "dt-bindings"
>> prefix is already stating that these are bindings.
>
> Ack.
>
>
>>
>> Where is driver or DTS? Are you now adding a dead binding without users?
>
>
> TCM is used by drivers/remoteproc/xlnx_r5_remoteproc.c driver. Howerver,
> we have hardcode addresses in TCM as bindings are not available yet.
I don't see usage of these compatibles there. You also did not supply
DTS here. Please provide users of bindings within the same patchset.
Best regards,
Krzysztof
On 13/01/2023 19:04, Tanmay Shah wrote:
> Hi Krzysztof Thanks for your reviews.
>
> Please find my comments below.
>
> On 1/12/23 11:52 PM, Krzysztof Kozlowski wrote:
>> On 13/01/2023 08:30, Tanmay Shah wrote:
>>> This patch introduces bindings for TCM memory address space on AMD-xilinx
>>> platforms. As of now TCM addresses are hardcoded in xilinx remoteproc
>>> driver. This bindings will help in defining TCM in device-tree and
>>> make it's access platform agnostic and data-driven from the driver.
>>>
>>> Signed-off-by: Tanmay Shah <[email protected]>
>>> ---
>>> .../devicetree/bindings/sram/xlnx,tcm.yaml | 137 ++++++++++++++++++
>>> 1 file changed, 137 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml b/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>>> new file mode 100644
>>> index 000000000000..02d17026fb1f
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>>> @@ -0,0 +1,137 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/sram/xlnx,tcm.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Tightly Coupled Memory (TCM)
>>> +
>>> +maintainers:
>>> + - Tanmay Shah <[email protected]>
>>> +
>>> +description: |
>>> + Tightly Coupled Memory(TCM) is available on AMD-Xilinx paltforms for ARM
>>> + cortex remote processors to use. It is low-latency memory that provide
>>> + predictable instruction execution and predictable data load/store timing.
>>> + TCM can be configured in lockstep mode or split mode. In split mode
>>> + configuration each RPU core has its own set of ATCM and BTCM memories and in
>>> + lockstep mode redundant processor's TCM become available to lockstep
>>> + processor. So In lockstep mode ATCM and BTCM size is increased.
>>> +
>>> +properties:
>>> + $nodename:
>>> + pattern: "sram-[0-9a-f]+$"
>> Drop node name requirement.
>> Why do you need sram node at all?
>
>
> I will remove sram- node. However, it device-tree I was planning to put
>
> all TCM nodes under single node for example:
>
> tcm {
>
> tcm-lockstep {
>
> };
>
> tcm-core@0 {
Mix of nodes with and without unit address is pointing to some design
issues.
>
> };
>
> };
>
> The top-most tcm node I assumed sram node. So I kept sram@xxxx
>
>>> +
>>> +patternProperties:
>>> + "^tcm-[a-z]+@[0-9a-f]+$":
>>> + type: object
>>> + description: |
>>> + During the split mode, each RPU core has its own set of ATCM and BTCM memory
>>> +
>>> + During the lock-step operation, the TCMs that are associated with the
>>> + redundant processor become available to the lock-step processor.
>>> + For example if each individual processor has 64KB ATCM, then in lockstep mode
>>> + The size of ATCM become 128KB. Same for BTCM. tcm-lockstep node represents
>>> + TCM address space in lockstep mode. tcm-core@x node specfies each core's
>>> + TCM address space in split mode.
>>> +
>>> + properties:
>>> + compatible:
>>> + oneOf:
>> This is not oneOf.
>>
>>> + - items:
>> and you do not have more than one item.
>>
>>> + - enum:
>>> + - xlnx,tcm-lockstep
>>> + - xlnx,tcm-split
>> compatible describes hardware, not configuration. What you encode here
>> does not fit compatible.
>
>
> I see. So, only xlnx,tcm is enough.
No, it must be specific to SoC.
>
>
>>
>>> +
>>> + "#address-cells":
>> Use consistent quotes, either " or '
>
>
> Ack.
>
>
>>
>>> + const: 1
>>> +
>>> + "#size-cells":
>>> + const: 1
>>> +
>>> + reg:
>>> + items:
>>> + - description: |
>>> + ATCM Memory address space. An ATCM typically holds interrupt or
>>> + exception code that must be accessed at high speed, without any
>>> + potential delay resulting from a cache miss.
>>> + RPU on AMD-Xilinx platform can also fetch data from ATCM
>>> + - description: |
>>> + BTCM Memory address space. A BTCM typically holds a block of data
>>> + for intensive processing, such as audio or video processing. RPU on
>>> + AMD-Xilinx Platforms can also fetch Code (Instructions) from BTCM
>>> +
>>> + reg-names:
>>> + items:
>>> + - const: atcm
>>> + - const: btcm
>>> +
>>> + ranges: true
>>> +
>>> + power-domains:
>>> + maxItems: 8
>>> + items:
>>> + - description: list of ATCM Power domains
>>> + - description: list of BTCM Power domains
>>> + additionalItems: true
>> And what are the rest?
> As both items are list, we should be able to include more than one
> power-domain I believe.
>
>
> So first item I am trying to create list of ATCM power domains.
>
> In split mode, first item is ATCM power-domain and second item is BTCM
> power domain.
>
> However, In lockstep mode, second core's TCM physically relocates and
> two ATCM combines and
Why power domains of a device depend on the mode? This does not look
like binding describing hardware.
>
> makes single region of ATCM. However, their power-domains remains same.
>
> So, In lockstep mode, first two banks are ATCM and so, first two items
> are ATCM power-domains.
>
> I am not sure best way to represent this. But, first itmes is list.
>
> So, I am assuming list of all ATCM power-domains possible.
List all items. Order is fixed, you cannot say BTCM is second item and
then put here something else.
Best regards,
Krzysztof
On 1/15/23 6:38 AM, Krzysztof Kozlowski wrote:
> On 13/01/2023 19:08, Tanmay Shah wrote:
>> On 1/12/23 11:52 PM, Krzysztof Kozlowski wrote:
>>> On 13/01/2023 08:30, Tanmay Shah wrote:
>>>> This patch introduces bindings for TCM memory address space on AMD-xilinx
>>>> platforms. As of now TCM addresses are hardcoded in xilinx remoteproc
>>>> driver. This bindings will help in defining TCM in device-tree and
>>>> make it's access platform agnostic and data-driven from the driver.
>>>>
>>> Subject: drop second/last, redundant "bindings". The "dt-bindings"
>>> prefix is already stating that these are bindings.
>> Ack.
>>
>>
>>> Where is driver or DTS? Are you now adding a dead binding without users?
>>
>> TCM is used by drivers/remoteproc/xlnx_r5_remoteproc.c driver. Howerver,
>> we have hardcode addresses in TCM as bindings are not available yet.
> I don't see usage of these compatibles there. You also did not supply
> DTS here. Please provide users of bindings within the same patchset.
ACK. I will supply dts as well.
However, Is it ok if I convert this patch to RFC patch, and once
bindings are fixed I will send actual patch with driver support.
If bindings design is not correct then I might have to change
corresponding driver design lot.
>
>
> Best regards,
> Krzysztof
>
Hi Kryzsztop Thanks for reviews. Please find my comments below.
On 1/15/23 6:45 AM, Krzysztof Kozlowski wrote:
> On 13/01/2023 19:04, Tanmay Shah wrote:
>> Hi Krzysztof Thanks for your reviews.
>>
>> Please find my comments below.
>>
>> On 1/12/23 11:52 PM, Krzysztof Kozlowski wrote:
>>> On 13/01/2023 08:30, Tanmay Shah wrote:
>>>> This patch introduces bindings for TCM memory address space on AMD-xilinx
>>>> platforms. As of now TCM addresses are hardcoded in xilinx remoteproc
>>>> driver. This bindings will help in defining TCM in device-tree and
>>>> make it's access platform agnostic and data-driven from the driver.
>>>>
>>>> Signed-off-by: Tanmay Shah <[email protected]>
>>>> ---
>>>> .../devicetree/bindings/sram/xlnx,tcm.yaml | 137 ++++++++++++++++++
>>>> 1 file changed, 137 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml b/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>>>> new file mode 100644
>>>> index 000000000000..02d17026fb1f
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>>>> @@ -0,0 +1,137 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/sram/xlnx,tcm.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Tightly Coupled Memory (TCM)
>>>> +
>>>> +maintainers:
>>>> + - Tanmay Shah <[email protected]>
>>>> +
>>>> +description: |
>>>> + Tightly Coupled Memory(TCM) is available on AMD-Xilinx paltforms for ARM
>>>> + cortex remote processors to use. It is low-latency memory that provide
>>>> + predictable instruction execution and predictable data load/store timing.
>>>> + TCM can be configured in lockstep mode or split mode. In split mode
>>>> + configuration each RPU core has its own set of ATCM and BTCM memories and in
>>>> + lockstep mode redundant processor's TCM become available to lockstep
>>>> + processor. So In lockstep mode ATCM and BTCM size is increased.
>>>> +
>>>> +properties:
>>>> + $nodename:
>>>> + pattern: "sram-[0-9a-f]+$"
>>> Drop node name requirement.
>>> Why do you need sram node at all?
>>
>> I will remove sram- node. However, it device-tree I was planning to put
>>
>> all TCM nodes under single node for example:
>>
>> tcm {
>>
>> tcm-lockstep {
>>
>> };
>>
>> tcm-core@0 {
> Mix of nodes with and without unit address is pointing to some design
> issues.
The design currently tries to accommodate physical relocation of the
memory. May be there is another way to represent this.
Here is address space of TCM memory on zynqmp platform:
https://docs.xilinx.com/r/en-US/ug1085-zynq-ultrascale-trm/Tightly-Coupled-Memory-Address-Map
As per above address map, there are 4 TCM banks, each 64KB ( 0x10000 )
size at following addresses:
*In split mode*:
ATCM0: 0xFFE00000---|---> Assigned to RPU core0
BTCM0: 0xFFE20000---|
ATCM1: 0xFFE90000---|---> Assigned to RPU 1
BTCM1: 0xFFEB0000---|
However, In lockstep mode, ATCM1 and BTCM1 relocates to different
addresses (i.e. 0xFFE10000 and 0xFFE30000 respectively)
and becomes available for RPU core 0:
*In lockstep mode Both used by RPU core 0*:
ATCM0: 0xFFE00000-----|
|----> ATCM: 0xFFE00000 size:
128KB
ATCM1: 0xFFE10000-----|
BTCM0: 0xFFE20000-----|
|----> BTCM: 0xFFE20000 size:
128KB
BTCM1: 0xFFE30000-----|
I am not sure how to represent this physical relocation of addresses in
device-tree.
Ideally such sram nodes can be represented as following:
[1] Representation of TCM in split mode:
[ a|b ]tcm[ 0|1 ] {
compatible = "xlnx,zynqmp-tcm";
reg <>;
ranges <>;
power-domain: (only 1 power domain for current bank)
}
However, to represent TCM in lockstep mode as well, I might have to add
platform specific optional reg and ranges property which optionally
represent address space of lockstep mode for atcm and btcm.
For example, ATCM0 and BTCM0 will be represented as above [1] However,
ATCM1 and BTCM1 will have following extra properties:
[a|b]tcm1 {
compatible = "xlnx,zynqmp-tcm";
reg <>;
lockstep-reg <>; /* represent address space of this bank in
lockstep mode */
ranges <>;
lockstep-ranges <>; /* represent address space ranges of this bank
in lockstep mode */
power-domain: (only 1 power domain for current bank)
};
Does above approach looks good? If some other standard way is already
available to represent this could you please suggest?
Thanks,
Tanmay
>
>> };
>>
>> };
>>
>> The top-most tcm node I assumed sram node. So I kept sram@xxxx
>>
>>>> +
>>>> +patternProperties:
>>>> + "^tcm-[a-z]+@[0-9a-f]+$":
>>>> + type: object
>>>> + description: |
>>>> + During the split mode, each RPU core has its own set of ATCM and BTCM memory
>>>> +
>>>> + During the lock-step operation, the TCMs that are associated with the
>>>> + redundant processor become available to the lock-step processor.
>>>> + For example if each individual processor has 64KB ATCM, then in lockstep mode
>>>> + The size of ATCM become 128KB. Same for BTCM. tcm-lockstep node represents
>>>> + TCM address space in lockstep mode. tcm-core@x node specfies each core's
>>>> + TCM address space in split mode.
>>>> +
>>>> + properties:
>>>> + compatible:
>>>> + oneOf:
>>> This is not oneOf.
>>>
>>>> + - items:
>>> and you do not have more than one item.
>>>
>>>> + - enum:
>>>> + - xlnx,tcm-lockstep
>>>> + - xlnx,tcm-split
>>> compatible describes hardware, not configuration. What you encode here
>>> does not fit compatible.
>>
>> I see. So, only xlnx,tcm is enough.
> No, it must be specific to SoC.
Ok. Then xlnx,zynqmp-tcm. I will change file name accordingly as well.
>
>>
>>>> +
>>>> + "#address-cells":
>>> Use consistent quotes, either " or '
>>
>> Ack.
>>
>>
>>>> + const: 1
>>>> +
>>>> + "#size-cells":
>>>> + const: 1
>>>> +
>>>> + reg:
>>>> + items:
>>>> + - description: |
>>>> + ATCM Memory address space. An ATCM typically holds interrupt or
>>>> + exception code that must be accessed at high speed, without any
>>>> + potential delay resulting from a cache miss.
>>>> + RPU on AMD-Xilinx platform can also fetch data from ATCM
>>>> + - description: |
>>>> + BTCM Memory address space. A BTCM typically holds a block of data
>>>> + for intensive processing, such as audio or video processing. RPU on
>>>> + AMD-Xilinx Platforms can also fetch Code (Instructions) from BTCM
>>>> +
>>>> + reg-names:
>>>> + items:
>>>> + - const: atcm
>>>> + - const: btcm
>>>> +
>>>> + ranges: true
>>>> +
>>>> + power-domains:
>>>> + maxItems: 8
>>>> + items:
>>>> + - description: list of ATCM Power domains
>>>> + - description: list of BTCM Power domains
>>>> + additionalItems: true
>>> And what are the rest?
>> As both items are list, we should be able to include more than one
>> power-domain I believe.
>>
>>
>> So first item I am trying to create list of ATCM power domains.
>>
>> In split mode, first item is ATCM power-domain and second item is BTCM
>> power domain.
>>
>> However, In lockstep mode, second core's TCM physically relocates and
>> two ATCM combines and
> Why power domains of a device depend on the mode? This does not look
> like binding describing hardware.
>
>> makes single region of ATCM. However, their power-domains remains same.
>>
>> So, In lockstep mode, first two banks are ATCM and so, first two items
>> are ATCM power-domains.
>>
>> I am not sure best way to represent this. But, first itmes is list.
>>
>> So, I am assuming list of all ATCM power-domains possible.
> List all items. Order is fixed, you cannot say BTCM is second item and
> then put here something else.
>
> Best regards,
> Krzysztof
>
On 16/01/2023 18:43, Tanmay Shah wrote:
>
> On 1/15/23 6:38 AM, Krzysztof Kozlowski wrote:
>> On 13/01/2023 19:08, Tanmay Shah wrote:
>>> On 1/12/23 11:52 PM, Krzysztof Kozlowski wrote:
>>>> On 13/01/2023 08:30, Tanmay Shah wrote:
>>>>> This patch introduces bindings for TCM memory address space on AMD-xilinx
>>>>> platforms. As of now TCM addresses are hardcoded in xilinx remoteproc
>>>>> driver. This bindings will help in defining TCM in device-tree and
>>>>> make it's access platform agnostic and data-driven from the driver.
>>>>>
>>>> Subject: drop second/last, redundant "bindings". The "dt-bindings"
>>>> prefix is already stating that these are bindings.
>>> Ack.
>>>
>>>
>>>> Where is driver or DTS? Are you now adding a dead binding without users?
>>>
>>> TCM is used by drivers/remoteproc/xlnx_r5_remoteproc.c driver. Howerver,
>>> we have hardcode addresses in TCM as bindings are not available yet.
>> I don't see usage of these compatibles there. You also did not supply
>> DTS here. Please provide users of bindings within the same patchset.
>
>
> ACK. I will supply dts as well.
>
> However, Is it ok if I convert this patch to RFC patch, and once
> bindings are fixed I will send actual patch with driver support.
>
> If bindings design is not correct then I might have to change
> corresponding driver design lot.
First, why this driver is particularly special? Why should have other
treatment then all other cases?
Second, so think about bindings and do not submit something for "driver"
but something describing hardware.
Best regards,
Krzysztof
On 16/01/2023 19:17, Tanmay Shah wrote:
> Hi Kryzsztop Thanks for reviews. Please find my comments below.
>
> On 1/15/23 6:45 AM, Krzysztof Kozlowski wrote:
>> On 13/01/2023 19:04, Tanmay Shah wrote:
>>> Hi Krzysztof Thanks for your reviews.
>>>
>>> Please find my comments below.
>>>
>>> On 1/12/23 11:52 PM, Krzysztof Kozlowski wrote:
>>>> On 13/01/2023 08:30, Tanmay Shah wrote:
>>>>> This patch introduces bindings for TCM memory address space on AMD-xilinx
>>>>> platforms. As of now TCM addresses are hardcoded in xilinx remoteproc
>>>>> driver. This bindings will help in defining TCM in device-tree and
>>>>> make it's access platform agnostic and data-driven from the driver.
>>>>>
>>>>> Signed-off-by: Tanmay Shah <[email protected]>
>>>>> ---
>>>>> .../devicetree/bindings/sram/xlnx,tcm.yaml | 137 ++++++++++++++++++
>>>>> 1 file changed, 137 insertions(+)
>>>>> create mode 100644 Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml b/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..02d17026fb1f
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/sram/xlnx,tcm.yaml
>>>>> @@ -0,0 +1,137 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/sram/xlnx,tcm.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Tightly Coupled Memory (TCM)
>>>>> +
>>>>> +maintainers:
>>>>> + - Tanmay Shah <[email protected]>
>>>>> +
>>>>> +description: |
>>>>> + Tightly Coupled Memory(TCM) is available on AMD-Xilinx paltforms for ARM
>>>>> + cortex remote processors to use. It is low-latency memory that provide
>>>>> + predictable instruction execution and predictable data load/store timing.
>>>>> + TCM can be configured in lockstep mode or split mode. In split mode
>>>>> + configuration each RPU core has its own set of ATCM and BTCM memories and in
>>>>> + lockstep mode redundant processor's TCM become available to lockstep
>>>>> + processor. So In lockstep mode ATCM and BTCM size is increased.
>>>>> +
>>>>> +properties:
>>>>> + $nodename:
>>>>> + pattern: "sram-[0-9a-f]+$"
>>>> Drop node name requirement.
>>>> Why do you need sram node at all?
>>>
>>> I will remove sram- node. However, it device-tree I was planning to put
>>>
>>> all TCM nodes under single node for example:
>>>
>>> tcm {
>>>
>>> tcm-lockstep {
>>>
>>> };
>>>
>>> tcm-core@0 {
>> Mix of nodes with and without unit address is pointing to some design
>> issues.
>
>
> The design currently tries to accommodate physical relocation of the
> memory. May be there is another way to represent this.
>
> Here is address space of TCM memory on zynqmp platform:
> https://docs.xilinx.com/r/en-US/ug1085-zynq-ultrascale-trm/Tightly-Coupled-Memory-Address-Map
>
> As per above address map, there are 4 TCM banks, each 64KB ( 0x10000 )
> size at following addresses:
>
> *In split mode*:
>
> ATCM0: 0xFFE00000---|---> Assigned to RPU core0
> BTCM0: 0xFFE20000---|
>
> ATCM1: 0xFFE90000---|---> Assigned to RPU 1
> BTCM1: 0xFFEB0000---|
>
> However, In lockstep mode, ATCM1 and BTCM1 relocates to different
> addresses (i.e. 0xFFE10000 and 0xFFE30000 respectively)
>
> and becomes available for RPU core 0:
>
>
> *In lockstep mode Both used by RPU core 0*:
>
> ATCM0: 0xFFE00000-----|
> |----> ATCM: 0xFFE00000 size:
> 128KB
> ATCM1: 0xFFE10000-----|
>
> BTCM0: 0xFFE20000-----|
> |----> BTCM: 0xFFE20000 size:
> 128KB
> BTCM1: 0xFFE30000-----|
>
>
> I am not sure how to represent this physical relocation of addresses in
> device-tree.
What do you mean by "relocates"? Move? You set one address in DT and
other address appears to be used? Then just set the other address?
>
> Ideally such sram nodes can be represented as following:
>
> [1] Representation of TCM in split mode:
>
> [ a|b ]tcm[ 0|1 ] {
You do not have unit address here.
>
> compatible = "xlnx,zynqmp-tcm";
>
> reg <>;
>
> ranges <>;
>
> power-domain: (only 1 power domain for current bank)
>
> }
>
> However, to represent TCM in lockstep mode as well, I might have to add
> platform specific optional reg and ranges property which optionally
> represent address space of lockstep mode for atcm and btcm.
I don't understand why. You have IO address space, so why do you remove
unit address and make reg optional?
>
> For example, ATCM0 and BTCM0 will be represented as above [1] However,
> ATCM1 and BTCM1 will have following extra properties:
>
> [a|b]tcm1 {
>
> compatible = "xlnx,zynqmp-tcm";
>
> reg <>;
>
> lockstep-reg <>; /* represent address space of this bank in
> lockstep mode */
Device is either in lockstep or it is not, right? Why do you put here
some dualism?
Best regards,
Krzysztof
On 1/17/23 12:16 AM, Krzysztof Kozlowski wrote:
> On 16/01/2023 18:43, Tanmay Shah wrote:
>> On 1/15/23 6:38 AM, Krzysztof Kozlowski wrote:
>>> On 13/01/2023 19:08, Tanmay Shah wrote:
>>>> On 1/12/23 11:52 PM, Krzysztof Kozlowski wrote:
>>>>> On 13/01/2023 08:30, Tanmay Shah wrote:
>>>>>> This patch introduces bindings for TCM memory address space on AMD-xilinx
>>>>>> platforms. As of now TCM addresses are hardcoded in xilinx remoteproc
>>>>>> driver. This bindings will help in defining TCM in device-tree and
>>>>>> make it's access platform agnostic and data-driven from the driver.
>>>>>>
>>>>> Subject: drop second/last, redundant "bindings". The "dt-bindings"
>>>>> prefix is already stating that these are bindings.
>>>> Ack.
>>>>
>>>>
>>>>> Where is driver or DTS? Are you now adding a dead binding without users?
>>>> TCM is used by drivers/remoteproc/xlnx_r5_remoteproc.c driver. Howerver,
>>>> we have hardcode addresses in TCM as bindings are not available yet.
>>> I don't see usage of these compatibles there. You also did not supply
>>> DTS here. Please provide users of bindings within the same patchset.
>>
>> ACK. I will supply dts as well.
>>
>> However, Is it ok if I convert this patch to RFC patch, and once
>> bindings are fixed I will send actual patch with driver support.
>>
>> If bindings design is not correct then I might have to change
>> corresponding driver design lot.
> First, why this driver is particularly special? Why should have other
> treatment then all other cases?
It's not different than others and shouldn't be treated differently. I
just didn't know correct bindings representation.
Now I have some idea how this should be represented, so I will send
bindings patch, dts patch and driver patch all in same series.
>
> Second, so think about bindings and do not submit something for "driver"
> but something describing hardware.
ACK. It will take me some time to post next patch, as I will add support
of this tcm device in xlnx remoteproc driver as well.
Thanks for all your suggestions, they were helpful.
>
> Best regards,
> Krzysztof
>