Message-ID: <3cd7943c-4d35-4ec9-8826-c20a5d213626@kernel.org>
Date: Tue, 16 Dec 2025 06:38:17 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Anand Moon <linux.amoon@...il.com>, Bjorn Helgaas <bhelgaas@...gle.com>,
 Lorenzo Pieralisi <lpieralisi@...nel.org>,
 Krzysztof Wilczyński <kwilczynski@...nel.org>,
 Manivannan Sadhasivam <mani@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Thierry Reding <thierry.reding@...il.com>,
 Jonathan Hunter <jonathanh@...dia.com>,
 "open list:PCI SUBSYSTEM" <linux-pci@...r.kernel.org>,
 "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
 <devicetree@...r.kernel.org>,
 "open list:TEGRA ARCHITECTURE SUPPORT" <linux-tegra@...r.kernel.org>,
 open list <linux-kernel@...r.kernel.org>
Cc: Mikko Perttunen <mperttunen@...dia.com>
Subject: Re: [PATCH v2 1/4] dt-bindings: PCI: Convert nvidia,tegra-pcie to DT
 schema

On 15/12/2025 15:15, Anand Moon wrote:
> Convert the existing text-based DT bindings documentation for the
> NVIDIA Tegra PCIe host controller to a DT schema format.

You dropped several properties from the original schema without
explanation. That's a no-go. I don't see any reason of doing that, but
if you find such reason you must clearly document any change done to the
binding with reasoning.

I won't be doing extensive review of your code, because you are known of
wasting my time, thus only few nits further.

> 
> Cc: Jon Hunter <jonathanh@...dia.com>
> Signed-off-by: Anand Moon <linux.amoon@...il.com>
> ---
> v2: Tried to address the isssue Rob pointed
> [1] https://lkml.org/lkml/2025/9/26/704
> improve the $suject and commit message
> drop few examples only nvidia,tegra20-pcie and nvidia,tegra210-pcie
> 
> $ make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/nvidia,tegra-pcie.yaml
> ---
>  .../bindings/pci/nvidia,tegra-pcie.yaml       | 380 ++++++++++
>  .../bindings/pci/nvidia,tegra20-pcie.txt      | 670 ------------------
>  2 files changed, 380 insertions(+), 670 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pci/nvidia,tegra-pcie.yaml
>  delete mode 100644 Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
> 
> diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra-pcie.yaml b/Documentation/devicetree/bindings/pci/nvidia,tegra-pcie.yaml
> new file mode 100644
> index 000000000000..e542adfe37b4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/nvidia,tegra-pcie.yaml
> @@ -0,0 +1,380 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/nvidia,tegra-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NVIDIA Tegra PCIe Controller
> +
> +maintainers:
> +  - Jon Hunter <jonathanh@...dia.com>
> +  - Thierry Reding <treding@...dia.com>
> +
> +description:
> +  PCIe controller found on NVIDIA Tegra SoCs which supports multiple
> +  root ports and platform-specific clock, reset, and power supply
> +  configurations.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nvidia,tegra20-pcie
> +      - nvidia,tegra30-pcie
> +      - nvidia,tegra124-pcie
> +      - nvidia,tegra210-pcie
> +      - nvidia,tegra186-pcie
> +
> +  reg:
> +    items:
> +      - description: PADS registers
> +      - description: AFI registers
> +      - description: Configuration space region
> +
> +  reg-names:
> +    items:
> +      - const: pads
> +      - const: afi
> +      - const: cs
> +
> +  interrupts:
> +    items:
> +      - description: Controller interrupt
> +      - description: MSI interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: intr
> +      - const: msi
> +
> +  clocks:
> +    minItems: 3
> +    items:
> +      - description: PCIe clock
> +      - description: AFI clock
> +      - description: PLL_E clock
> +      - description: Optional CML clock
> +
> +  clock-names:
> +    description: Names of clocks used by the PCIe controller
> +    minItems: 3
> +    items:
> +      - const: pex
> +      - const: afi
> +      - const: pll_e
> +      - const: cml
> +
> +  resets:
> +    items:
> +      - description: PCIe reset
> +      - description: AFI reset
> +      - description: PCIe-X reset
> +
> +  reset-names:
> +    items:
> +      - const: pex
> +      - const: afi
> +      - const: pcie_x
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  interconnects:
> +    minItems: 1
> +    maxItems: 2

This does not match the interconnect-names.

> +
> +  interconnect-names:
> +    items:
> +      - const: dma-mem
> +      - const: write
> +
> +  pinctrl-names:
> +    items:
> +      - const: default
> +      - const: idle
> +
> +  pinctrl-0: true
> +  pinctrl-1: true
> +
> +  operating-points-v2:
> +    description:
> +      Should contain freqs and voltages and opp-supported-hw property, which
> +      is a bitfield indicating SoC speedo ID mask.

Look at other bindings how this field is described.

> +
> +patternProperties:
> +  "^pci@[0-9a-f]+(,[0-9a-f]+)?$":
> +    type: object
> +    allOf:
> +      - $ref: /schemas/pci/pci-pci-bridge.yaml#
> +
> +    properties:
> +      reg:
> +        maxItems: 1
> +
> +      nvidia,num-lanes:
> +        description: Number of lanes used by this PCIe port
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum:
> +          - 1
> +          - 2
> +          - 4
> +
> +    required:
> +      - nvidia,num-lanes
> +
> +    unevaluatedProperties: false
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-host-bridge.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - nvidia,tegra20-pcie
> +              - nvidia,tegra186-pcie
> +    then:
> +      properties:
> +        clocks:
> +          maxItems: 3
> +        clock-names:
> +          items:
> +            - const: pex
> +            - const: afi
> +            - const: pll_e
> +        resets:
> +          maxItems: 3
> +        reset-names:
> +          items:
> +            - const: pex
> +            - const: afi
> +            - const: pcie_x

Blank line

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - nvidia,tegra30-pcie
> +              - nvidia,tegra124-pcie
> +              - nvidia,tegra210-pcie
> +    then:
> +      properties:
> +        clocks:
> +          maxItems: 4
> +        clock-names:
> +          items:
> +            - const: pex
> +            - const: afi
> +            - const: pll_e
> +            - const: cml
> +        resets:
> +          maxItems: 3
> +        reset-names:
> +          items:
> +            - const: pex
> +            - const: afi
> +            - const: pcie_x
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - nvidia,tegra20-pcie
> +              - nvidia,tegra30-pcie
> +    then:
> +      required:
> +        - power-domains
> +        - operating-points-v2
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - nvidia,tegra186-pcie
> +    then:
> +      required:
> +        - interconnects
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - nvidia,tegra210-pcie
> +    then:
> +      required:
> +        - pinctrl-names
> +        - pinctrl-0
> +        - pinctrl-1
> +
> +unevaluatedProperties: false

This goes after required.

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +  - resets
> +  - reset-names
> +  - interrupts
> +  - interrupt-map
> +  - interrupt-map-mask
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    bus {
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +
> +        pcie@...03000 {
> +            compatible = "nvidia,tegra20-pcie";
> +            device_type = "pci";
> +            reg = <0x80003000 0x00000800>,
> +                  <0x80003800 0x00000200>,
> +                  <0x90000000 0x10000000>;
> +            reg-names = "pads", "afi", "cs";
> +            interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 99 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "intr", "msi";
> +            interrupt-parent = <&intc>;
> +
> +            #interrupt-cells = <1>;
> +            interrupt-map-mask = <0 0 0 0>;
> +            interrupt-map = <0 0 0 0 &intc GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
> +
> +            bus-range = <0x00 0xff>;
> +            #address-cells = <3>;
> +            #size-cells = <2>;
> +
> +            ranges = <0x02000000 0 0x80000000 0x80000000 0 0x00001000>,
> +                     <0x02000000 0 0x80001000 0x80001000 0 0x00001000>,
> +                     <0x01000000 0 0          0x82000000 0 0x00010000>,
> +                     <0x02000000 0 0xa0000000 0xa0000000 0 0x08000000>,
> +                     <0x42000000 0 0xa8000000 0xa8000000 0 0x18000000>;
> +
> +            clocks = <&tegra_car 70>,
> +                     <&tegra_car 72>,
> +                     <&tegra_car 118>;
> +            clock-names = "pex", "afi", "pll_e";
> +            resets = <&tegra_car 70>,
> +                     <&tegra_car 72>,
> +                     <&tegra_car 74>;
> +            reset-names = "pex", "afi", "pcie_x";
> +            power-domains = <&pd_core>;
> +            operating-points-v2 = <&pcie_dvfs_opp_table>;
> +
> +            status = "okay";

No statuses in the example. Please look at other files to see how this
should be written.


Best regards,
Krzysztof

Powered by blists - more mailing lists