Message-ID: <ae5ed7e6-1f4f-c45f-06ef-dd5566e8a7d0@linaro.org>
Date:   Tue, 7 Feb 2023 11:57:33 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH] dt-bindings: fpga: convert bindings document to yaml
Content-Language: en-US
To:     Nava kishore Manne <nava.kishore.manne@amd.com>, mdf@kernel.org,
        hao.wu@intel.com, yilun.xu@intel.com, trix@redhat.com,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        michal.simek@xilinx.com, linux-fpga@vger.kernel.org,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org
References: <20230207104812.182439-1-nava.kishore.manne@amd.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230207104812.182439-1-nava.kishore.manne@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 07/02/2023 11:48, Nava kishore Manne wrote:
> Convert the xilinx-pr-decoupler binding document from txt to yaml.
> 
> Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

missing final component for prefixes

Subject: drop second/last, redundant "bindings document". The
"dt-bindings" prefix is already stating that these are bindings.

> ---
>  .../bindings/fpga/xilinx-pr-decoupler.txt     | 54 -------------
>  .../bindings/fpga/xlnx,pr-decoupler.yaml      | 76 +++++++++++++++++++
>  2 files changed, 76 insertions(+), 54 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/fpga/xilinx-pr-decoupler.txt
>  create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,pr-decoupler.yaml
> 
> diff --git a/Documentation/devicetree/bindings/fpga/xilinx-pr-decoupler.txt b/Documentation/devicetree/bindings/fpga/xilinx-pr-decoupler.txt
> deleted file mode 100644
> index 0acdfa6d62a4..000000000000
> --- a/Documentation/devicetree/bindings/fpga/xilinx-pr-decoupler.txt
> +++ /dev/null
> @@ -1,54 +0,0 @@
> -Xilinx LogiCORE Partial Reconfig Decoupler Softcore
> -
> -The Xilinx LogiCORE Partial Reconfig Decoupler manages one or more
> -decouplers / fpga bridges.
> -The controller can decouple/disable the bridges which prevents signal
> -changes from passing through the bridge.  The controller can also
> -couple / enable the bridges which allows traffic to pass through the
> -bridge normally.
> -
> -Xilinx LogiCORE Dynamic Function eXchange(DFX) AXI shutdown manager
> -Softcore is compatible with the Xilinx LogiCORE pr-decoupler.
> -
> -The Dynamic Function eXchange AXI shutdown manager prevents AXI traffic
> -from passing through the bridge. The controller safely handles AXI4MM
> -and AXI4-Lite interfaces on a Reconfigurable Partition when it is
> -undergoing dynamic reconfiguration, preventing the system deadlock
> -that can occur if AXI transactions are interrupted by DFX
> -
> -The Driver supports only MMIO handling. A PR region can have multiple
> -PR Decouplers which can be handled independently or chained via decouple/
> -decouple_status signals.
> -
> -Required properties:
> -- compatible		: Should contain "xlnx,pr-decoupler-1.00" followed by
> -                          "xlnx,pr-decoupler" or
> -                          "xlnx,dfx-axi-shutdown-manager-1.00" followed by
> -                          "xlnx,dfx-axi-shutdown-manager"
> -- regs			: base address and size for decoupler module
> -- clocks		: input clock to IP
> -- clock-names		: should contain "aclk"
> -
> -See Documentation/devicetree/bindings/fpga/fpga-region.txt and
> -Documentation/devicetree/bindings/fpga/fpga-bridge.txt for generic bindings.
> -
> -Example:
> -Partial Reconfig Decoupler:
> -	fpga-bridge@100000450 {
> -		compatible = "xlnx,pr-decoupler-1.00",
> -			     "xlnx-pr-decoupler";
> -		regs = <0x10000045 0x10>;
> -		clocks = <&clkc 15>;
> -		clock-names = "aclk";
> -		bridge-enable = <0>;
> -	};
> -
> -Dynamic Function eXchange AXI shutdown manager:
> -	fpga-bridge@100000450 {
> -		compatible = "xlnx,dfx-axi-shutdown-manager-1.00",
> -			     "xlnx,dfx-axi-shutdown-manager";
> -		regs = <0x10000045 0x10>;
> -		clocks = <&clkc 15>;
> -		clock-names = "aclk";
> -		bridge-enable = <0>;
> -	};
> diff --git a/Documentation/devicetree/bindings/fpga/xlnx,pr-decoupler.yaml b/Documentation/devicetree/bindings/fpga/xlnx,pr-decoupler.yaml
> new file mode 100644
> index 000000000000..caea58a9ba7d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/xlnx,pr-decoupler.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/fpga/xlnx,pr-decoupler.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx LogiCORE Partial Reconfig Decoupler/AXI shutdown manager Softcore
> +
> +maintainers:
> +  - Nava kishore Manne <nava.kishore.manne@amd.com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - description: The Xilinx LogiCORE Partial Reconfig Decoupler manages one
> +          or more decouplers / fpga bridges. The controller can decouple/disable
> +          the bridges which prevents signal changes from passing through the
> +          bridge. The controller can also couple / enable the bridges which
> +          allows traffic to pass through the bridge normally.

Description of device goes to toplevel "description" field. Not here.
Here you can point shortly differences, but such statement suggests you
should have different bindings.

> +        items:
> +          - const: xlnx,pr-decoupler-1.00
> +          - const: xlnx,pr-decoupler
> +      - description: The Xilinx LogiCORE Dynamic Function eXchange(DFX)
> +          AXI shutdown manager softcore is compatible with the Xilinx
> +          LogiCORE pr-decoupler. The Dynamic Function eXchange AXI shutdown
> +          manager prevents AXI traffic from passing through the bridge.
> +          The controller safely handles AXI4MM and AXI4-Lite interfaces on
> +          a Reconfigurable Partition when it is undergoing dynamic
> +          reconfiguration, preventing the system deadlock that can occur
> +          if AXI transactions are interrupted by DFX.

Same problem.

> +        items:
> +          - const: xlnx,dfx-axi-shutdown-manager-1.00
> +          - const: xlnx,dfx-axi-shutdown-manager
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: aclk
> +
> +  bridge-enable:

Missing type/ref.

> +    description:
> +      Zero if driver should disable bridge at startup

Are these sentences? Then missing full sotp.


> +      One if driver should enable bridge at startup
> +      Default is to leave bridge in current state.

Missing enum.

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +unevaluatedProperties: false

Instead (you do not reference any other binding):
additionalProperties: false

Anyway, I have doubts it you tested it. Just read fpga-region bindings
and original TXT... It clearly points to regions. Where are they?

> +
> +examples:
> +  - |
> +    fpga-bridge@100000450 {
> +      compatible = "xlnx,pr-decoupler-1.00", "xlnx,pr-decoupler";
> +      reg = <0x10000045 0x10>;
> +      clocks = <&clkc 15>;
> +      clock-names = "aclk";
> +      bridge-enable = <0>;
> +    };
> +
> +  - |
> +    fpga-bridge@100000850 {

Drop second example, it's basically the same.

> +      compatible = "xlnx,dfx-axi-shutdown-manager-1.00", "xlnx,dfx-axi-shutdown-manager";
> +      reg = <0x10000045 0x10>;
> +      clocks = <&clkc 15>;
> +      clock-names = "aclk";
> +      bridge-enable = <0>;
> +    };

Best regards,
Krzysztof