Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <df4fdecb-6ca7-d96b-bcad-02cefb52ce4e@linaro.org>
Date:   Fri, 13 Jan 2023 08:52:08 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH] dt-bindings: sram: Tightly Coupled Memory (TCM) bindings
Content-Language: en-US
To:     Tanmay Shah <tanmay.shah@amd.com>, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org
Cc:     devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-remoteproc@vger.kernel.org
References: <20230113073045.4008853-1-tanmay.shah@amd.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230113073045.4008853-1-tanmay.shah@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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 <tanmay.shah@amd.com>
> ---
>  .../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 <tanmay.shah@amd.com>
> +
> +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