Re: [PATCH v1 1/2] dt-bindings: PCI: eic7700: Add Eswin eic7700 PCIe host controller

[Krzysztof Kozlowski <krzk@kernel.org>]

On 16/05/2025 11:42, zhangsenchuan@eswincomputing.com wrote:
> From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> 
> Add Device Tree binding documentation for the ESWIN EIC7700
> PCIe controller module,the PCIe controller enables the core
> to correctly initialize and manage the PCIe bus and connected
> devices.
> 
> Co-developed-by: Yu Ning <ningyu@eswincomputing.com>
> Signed-off-by: Yu Ning <ningyu@eswincomputing.com>
> Signed-off-by: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> ---
>  .../bindings/pci/eswin,eic7700-pcie.yaml      | 171 ++++++++++++++++++

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>

Limited review follows.

>  1 file changed, 171 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/eswin,eic7700-pcie.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/eswin,eic7700-pcie.yaml b/Documentation/devicetree/bindings/pci/eswin,eic7700-pcie.yaml
> new file mode 100644
> index 000000000000..e1d150c7c81a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/eswin,eic7700-pcie.yaml
> @@ -0,0 +1,171 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/eswin,eic7700-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Eswin EIC7700 PCIe host controller
> +
> +maintainers:
> +  - Yu Ning <ningyu@eswincomputing.com>
> +  - Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  The PCIe controller on EIC7700 SoC.
> +
> +properties:
> +  compatible:
> +    const: eswin,eic7700-pcie
> +
> +  reg:
> +    maxItems: 3
> +
> +  reg-names:
> +    items:
> +      - const: dbi
> +      - const: config
> +      - const: mgmt
> +
> +  "#address-cells":
> +    const: 3
> +  "#size-cells":
> +    const: 2
> +  '#interrupt-cells':
> +    const: 1
> +
> +  interrupts:
> +    maxItems: 9
> +
> +  interrupt-names:
> +    items:
> +      - const: msi
> +      - const: inta
> +      - const: intb
> +      - const: intc
> +      - const: intd

Does not match interrupts.

> +
> +  interrupt-controller:
> +    type: object
> +
> +  interrupt-map:
> +    maxItems: 4
> +
> +  interrupt-map-mask:
> +    items:
> +      - const: 0
> +      - const: 0
> +      - const: 0
> +      - const: 7
> +
> +  clocks:
> +    maxItems: 4
> +    description: handles to clock for the pcie controller.

Drop description, obvious.

> +
> +  clock-names:
> +    items:
> +      - const: pcie_aclk
> +      - const: pcie_cfg_clk
> +      - const: pcie_cr_clk
> +      - const: pcie_aux_clk

Drop all _clk
Drop all pcie_

> +    description: the name of each clock.

Drop description

> +
> +  resets:
> +    description: resets to be used by the controller.
> +
> +  reset-names:
> +    items:
> +      - const: pcie_cfg
> +      - const: pcie_powerup
> +      - const: pcie_pwren

Drop all pcie_

> +    description: names of the resets listed in resets property in the same order.

What did you use as starting point / example?

> +
> +  bus-range:
> +    items:
> +      - const: 0
> +      - const: 0xff
> +
> +  device_type:
> +    const: pci
> +
> +  ranges: true
> +
> +  dma-noncoherent: true
> +
> +  num-lanes:
> +    maximum: 4
> +
> +  numa-node-id:
> +    maximum: 0

This is confusing.

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - interrupt-names
> +  - interrupt-parent
> +  - interrupt-map-mask
> +  - interrupt-map
> +  - '#address-cells'
> +  - '#size-cells'
> +  - '#interrupt-cells'
> +  - clocks
> +  - clock-names
> +  - resets
> +  - reset-names
> +  - bus-range
> +  - dma-noncoherent
> +  - num-lanes
> +  - ranges
> +  - numa-node-id

Missing ref to pci schemas. Look at other bindings.

> +
> +additionalProperties: false

And then this will be changed as well.

> +
> +examples:
> +  - |
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pcie: pcie@54000000 {
> +          compatible = "eswin,eic7700-pcie";

Messed indentation. Use 4 spaces for example indentation.

> +          clocks = <&clock 562>,

Wrong property order. Follow DTS coding style.

> +                   <&clock 563>,
> +                   <&clock 564>,
> +                   <&clock 565>;
> +          clock-names = "pcie_aclk", "pcie_cfg_clk", "pcie_cr_clk", "pcie_aux_clk";
> +
> +          reset-names = "pcie_cfg", "pcie_powerup", "pcie_pwren";
> +          resets = <&reset 8 (1 << 0)>,
> +                   <&reset 8 (1 << 1)>,
> +                   <&reset 8 (1 << 2)>;
> +
> +          #address-cells = <3>;
> +          #size-cells = <2>;
> +          #interrupt-cells = <1>;
> +
> +          reg = <0x0 0x54000000 0x0 0x4000000>,
> +                <0x0 0x40000000 0x0 0x800000>,
> +                <0x0 0x50000000 0x0 0x100000>;
> +          reg-names = "dbi", "config", "mgmt";
> +          device_type = "pci";
> +
> +          bus-range = <0x0 0xff>;
> +          ranges = <0x81000000 0x0 0x40800000 0x0 0x40800000 0x0 0x800000>,
> +                  <0x82000000 0x0 0x41000000 0x0 0x41000000 0x0 0xf000000>,
> +                  <0xc3000000 0x80 0x00000000 0x80 0x00000000 0x2 0x00000000>;
> +
> +          num-lanes = <0x4>;
> +          interrupts = <220>, <179>, <180>, <181>, <182>, <183>, <184>, <185>, <186>;
> +          interrupt-names = "msi", "inta", "intb", "intc", "intd";
> +          interrupt-parent = <&plic>;
> +          interrupt-map-mask = <0x0 0x0 0x0 0x7>;
> +          interrupt-map = <0x0 0x0 0x0 0x1 &plic 179>,
> +                          <0x0 0x0 0x0 0x2 &plic 180>,
> +                          <0x0 0x0 0x0 0x3 &plic 181>,
> +                          <0x0 0x0 0x0 0x4 &plic 182>;
> +          status = "disabled";

Drop. Look at other bindings how this is written. There is no binding
with status disabled. It makes just no sense.

> +          numa-node-id = <0>;
> +          dma-noncoherent;
> +        };
> +    };
> --
> 2.25.1
> 


Best regards,
Krzysztof