lkml.org 
[lkml]   [2022]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 2/6] dt-bindings: PCI: Add PCIe controller bindings for FSD
    From
    On 21/11/2022 11:52, Shradha Todi wrote:
    > Document the PCIe controller device tree bindings for

    Subject: drop second, redundant "bindings".

    > Tesla FSD SoC for both RC and EP

    Full stop.

    >
    > Signed-off-by: Shradha Todi <shradha.t@samsung.com>
    > ---
    > .../bindings/pci/tesla,pcie-fsd-ep.yaml | 107 ++++++++++++++++
    > .../bindings/pci/tesla,pcie-fsd.yaml | 117 ++++++++++++++++++
    > 2 files changed, 224 insertions(+)
    > create mode 100644 Documentation/devicetree/bindings/pci/tesla,pcie-fsd-ep.yaml
    > create mode 100644 Documentation/devicetree/bindings/pci/tesla,pcie-fsd.yaml
    >
    > diff --git a/Documentation/devicetree/bindings/pci/tesla,pcie-fsd-ep.yaml b/Documentation/devicetree/bindings/pci/tesla,pcie-fsd-ep.yaml
    > new file mode 100644
    > index 000000000000..07308cb9a35c
    > --- /dev/null
    > +++ b/Documentation/devicetree/bindings/pci/tesla,pcie-fsd-ep.yaml

    Filename based on compatible.

    > @@ -0,0 +1,107 @@
    > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
    > +%YAML 1.2
    > +---
    > +$id: http://devicetree.org/schemas/pci/tesla,pcie-fsd-ep.yaml#
    > +$schema: http://devicetree.org/meta-schemas/core.yaml#
    > +
    > +title: Synopsys DesignWare based PCIe EP controller on FSD SoCs
    > +
    > +maintainers:
    > + - Shradha Todi <shradha.t@samsung.com>
    > +
    > +description: |
    > + FSD PCIe EP controller is based on Synopsys DesignWare PCIe IP
    > + and thus inherits all the common properties defined in
    > + snps,dw-pcie-ep.yaml. The controller instance is dual mode and
    > + can work in Root port mode or Endpoint mode at a time.
    > +
    > +allOf:
    > + - $ref: /schemas/pci/snps,dw-pcie-ep.yaml#
    > +
    > +properties:
    > + compatible:
    > + enum:
    > + - tesla,fsd-pcie-ep
    > +
    > + reg:
    > + items:
    > + - description: controller's application logic registers
    > + - description: controller's own configuration registers
    > + are available.
    > + - description: controller's own configuration shadow registers
    > + are available.
    > + - description: Map the remote Root Complex slave address space
    > +
    > + reg-names:
    > + items:
    > + - const: appl
    > + - const: dbi
    > + - const: dbi2
    > + - const: addr_space
    > +
    > + clocks:
    > + items:
    > + - description: Auxiliary clock for PCIe
    > + - description: AHB clock for PCIe dbi
    > + - description: AHB clock for PCIe master
    > + - description: AHB clock for PCIe slave
    > +
    > + clock-names:
    > + items:
    > + - const: aux_clk
    > + - const: dbi_clk
    > + - const: mstr_clk
    > + - const: slv_clk

    Drop "_clk" everywhere.

    > +
    > + interrupts:
    > + items:
    > + - description: Controller interrupt
    > +
    > + interrupt-names:
    > + items:
    > + - const: sub_ctrl_intr

    Drop entire property.

    > +
    > + tesla,pcie-sysreg:
    > + $ref: '/schemas/types.yaml#/definitions/phandle-array'

    Drop quotes.

    In the PHY patch you said it is one phandle. Here it is an array. If it
    is an array, then define it properly:
    https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml#L42

    > + description: phandle for system control registers, used to
    > + control signals at system level
    > +
    > +required:
    > +

    Drop blank line.

    > + - compatible
    > + - reg
    > + - reg-names
    > + - clocks
    > + - clock-names
    > + - ranges
    > + - num-lanes
    > + - tesla,pcie-sysreg
    > +
    > +additionalProperties: true

    This must be false. Did you see such code anywhere in existing bindings?
    Maybe you meant unevaluatedProperties?

    > +
    > +examples:
    > + - |
    > + #include <dt-bindings/clock/fsd-clk.h>
    > + #include <dt-bindings/interrupt-controller/arm-gic.h>
    > +
    > + pcie4_ep: pcie-ep@15400000 {
    > + compatible = "tesla,fsd-pcie-ep";
    > + clocks = <&clock_fsys0 PCIE_SUBCTRL_INST0_AUX_CLK_SOC>,
    > + <&clock_fsys0 PCIE_SUBCTRL_INST0_DBI_ACLK_SOC>,
    > + <&clock_fsys0 PCIE_SUBCTRL_INST0_MSTR_ACLK_SOC>,
    > + <&clock_fsys0 PCIE_SUBCTRL_INST0_SLV_ACLK_SOC>;
    > + clock-names = "aux_clk", "dbi_clk", "mstr_clk", "slv_clk";
    > + interrupts = <GIC_SPI 93 IRQ_TYPE_EDGE_RISING>;
    > + interrupt-names = "sub_ctrl_intr";
    > + reg = <0x15090000 0x1000>,

    reg is a second property.

    > + <0x15400000 0x1000>,
    > + <0x15401000 0x80>,
    > + <0x15800000 0xFF0000>;

    Lowercase hex.

    > + reg-names = "appl", "dbi", "dbi2", "addr_space";
    > + num-lanes = <4>;
    > + tesla,pcie-sysreg = <&sysreg_fsys0 0x434>;
    > + phys = <&pcie_phy0>;
    > + phy-names = "pcie_phy0";
    > + status = "disabled";

    No status.

    > + };
    > +...
    > diff --git a/Documentation/devicetree/bindings/pci/tesla,pcie-fsd.yaml b/Documentation/devicetree/bindings/pci/tesla,pcie-fsd.yaml
    > new file mode 100644
    > index 000000000000..85648e5c7d27
    > --- /dev/null
    > +++ b/Documentation/devicetree/bindings/pci/tesla,pcie-fsd.yaml

    All comments apply here as well.

    > @@ -0,0 +1,117 @@
    > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
    > +%YAML 1.2
    > +---
    > +
    > +$id: http://devicetree.org/schemas/pci/tesla,pcie-fsd.yaml#
    > +$schema: http://devicetree.org/meta-schemas/core.yaml#
    > +
    > +title: Synopsys DesignWare based PCIe controller on FSD SoCs
    > +
    > +maintainers:
    > + - Shradha Todi <shradha.t@samsung.com>
    > +
    > +description: |
    > + FSD PCIe controller is based on Synopsys DesignWare PCIe IP
    > + and thus inherits all the common properties defined in
    > + designware-pcie.txt.
    > +
    > +allOf:
    > + - $ref: /schemas/pci/snps,dw-pcie.yaml#
    > +
    > +properties:
    > + compatible:
    > + const: tesla,fsd-pcie
    > +
    > + reg:
    > + items:
    > + - description: controller's application logic registers
    > + - description: controller's own configuration registers
    > + are available.
    > + - description: configuration registers
    > +
    > + reg-names:
    > + items:
    > + - const: appl
    > + - const: dbi
    > + - const: config
    > +
    > + clocks:
    > + items:
    > + - description: Auxiliary clock for PCIe
    > + - description: AHB clock for PCIe dbi
    > + - description: AHB clock for PCIe master
    > + - description: AHB clock for PCIe slave
    > +
    > + clock-names:
    > + items:
    > + - const: aux_clk
    > + - const: dbi_clk
    > + - const: mstr_clk
    > + - const: slv_clk
    > +
    > + interrupts:
    > + minItems: 1
    > + items:
    > + - description: MSI interrupt
    > + - description: Controller interrupt
    > +
    > + interrupt-names:
    > + minItems: 1
    > + items:
    > + - const: msi
    > + - const: sub_ctrl_intr

    Drop "_intr"

    > +
    > + tesla,pcie-sysreg:
    > + $ref: '/schemas/types.yaml#/definitions/phandle-array'
    > + description: phandle for system control registers, used to
    > + control signals at system level
    > +
    > +required:
    > +

    ?

    > + - compatible
    > + - reg
    > + - reg-names
    > + - clocks
    > + - clock-names
    > + - ranges
    > + - num-lanes
    > + - tesla,pcie-sysreg
    > +
    > +additionalProperties: true

    false

    > +
    > +examples:
    > + - |
    > + #include <dt-bindings/clock/fsd-clk.h>
    > + #include <dt-bindings/interrupt-controller/arm-gic.h>
    > +
    > + bus {
    > + #address-cells = <2>;
    > + #size-cells = <2>;
    > +
    > + pcie4_rc: pcie@15400000 {
    > + compatible = "tesla,fsd-pcie";
    > + clocks = <&clock_fsys0 PCIE_SUBCTRL_INST0_AUX_CLK_SOC>,
    > + <&clock_fsys0 PCIE_SUBCTRL_INST0_DBI_ACLK_SOC>,
    > + <&clock_fsys0 PCIE_SUBCTRL_INST0_MSTR_ACLK_SOC>,
    > + <&clock_fsys0 PCIE_SUBCTRL_INST0_SLV_ACLK_SOC>;
    > + clock-names = "aux_clk", "dbi_clk", "mstr_clk", "slv_clk";
    > + #address-cells = <3>;
    > + #size-cells = <2>;
    > + dma-coherent;
    > + interrupts = <GIC_SPI 93 IRQ_TYPE_EDGE_RISING>;
    > + interrupt-names = "msi";
    > + num-lanes = <4>;
    > + reg = <0x0 0x15090000 0x0 0x1000>,
    > + <0x0 0x15400000 0x0 0x1000>,
    > + <0x0 0x15800000 0x0 0x1000>;

    reg is always second. reg-names in such case third. ranges fourth.

    > + reg-names = "appl", "dbi", "config";


    Best regards,
    Krzysztof

    \
     
     \ /
      Last update: 2022-11-21 13:06    [W:3.473 / U:0.564 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site