Re: [PATCH 2/9] dt-bindings: PCI: qcom: Document the Shikra PCIe Controller

[Sushrut Shree Trivedi <sushrut.trivedi@oss.qualcomm.com>]

On 7/7/2026 11:31 AM, Manivannan Sadhasivam wrote:
> On Wed, Jul 01, 2026 at 12:32:44AM +0530, Sushrut Shree Trivedi wrote:
>> Add a dedicated schema for the PCIe controller found on the Shikra
>> platform.
>>
>> Signed-off-by: Sushrut Shree Trivedi <sushrut.trivedi@oss.qualcomm.com>
>> ---
>>   .../devicetree/bindings/pci/qcom,shikra-pcie.yaml  | 211 +++++++++++++++++++++
>>   1 file changed, 211 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/qcom,shikra-pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,shikra-pcie.yaml
>> new file mode 100644
>> index 000000000000..f9d1dba9dd2e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/qcom,shikra-pcie.yaml
>> @@ -0,0 +1,211 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pci/qcom,shikra-pcie.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Shikra PCI Express Root Complex
>> +
>> +maintainers:
>> +  - Bjorn Andersson <andersson@kernel.org>
>> +  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> +
>> +description:
>> +  Qualcomm Shikra SoC (and compatible) PCIe root complex controller is based on
> s/PCIe root complex controller/PCIe Root Complex
ACK
>
>> +  the Synopsys DesignWare PCIe IP.
>> +
>> +properties:
>> +  compatible:
>> +    const: qcom,shikra-pcie
>> +
>> +  reg:
>> +    minItems: 5
>> +    maxItems: 6
>> +
>> +  reg-names:
>> +    minItems: 5
>> +    items:
>> +      - const: parf # Qualcomm specific registers
>> +      - const: dbi # DesignWare PCIe registers
>> +      - const: elbi # External local bus interface registers
>> +      - const: atu # ATU address space
>> +      - const: config # PCIe configuration space
>> +      - const: mhi # MHI registers
> MHI is not optional.
ACK
>
>> +
>> +  clocks:
>> +    minItems: 7
>> +    maxItems: 9
>> +
>> +  clock-names:
>> +    minItems: 7
>> +    items:
>> +      - const: aux # Auxiliary clock
>> +      - const: cfg # Configuration clock
>> +      - const: bus_master # Master AXI clock
>> +      - const: bus_slave # Slave AXI clock
>> +      - const: slave_q2a # Slave Q2A clock
>> +      - const: ddrss_memnoc_pcie # PCIe SF MEMNOC clock
>> +      - const: tile # PCIe tile SYS NoC clock
>> +      - const: qmip_pcie_ahb # QMIP PCIe AHB clock
> Why optional clocks?
ACK
>
>> +
>> +  interrupts:
>> +    minItems: 8
>> +    maxItems: 9
>> +
>> +  interrupt-names:
>> +    minItems: 8
>> +    items:
>> +      - const: msi0
>> +      - const: msi1
>> +      - const: msi2
>> +      - const: msi3
>> +      - const: msi4
>> +      - const: msi5
>> +      - const: msi6
>> +      - const: msi7
>> +      - const: global
> Same here, why global interrupt is optional?
ACK
>
>> +
>> +  resets:
>> +    minItems: 1
>> +    maxItems: 2
>> +
>> +  reset-names:
>> +    minItems: 1
>> +    items:
>> +      - const: pci # PCIe core reset
>> +      - const: link_down # PCIe link down reset
> Same here.
ACK
>
>> +
>> +required:
>> +  - power-domains
>> +  - resets
>> +  - reset-names
>> +
>> +allOf:
>> +  - $ref: qcom,pcie-common.yaml#
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/qcom,dispcc-qcm2290.h>
>> +    #include <dt-bindings/clock/qcom,qcm2290-gpucc.h>
> What are these includes for?
ACK. Will be fixed in v2.
>
>> +    #include <dt-bindings/clock/qcom,rpmcc.h>
>> +
>> +    soc {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
> Get rid of 'soc' node.
If we remove soc node, dt-schema uses address-cells = <1>,
size-cells  = <1> for dt-binding check and throws errors.

Do you suggest ignoring those errors ?
>
>> +        pcie@45e8000 {
>> +          device_type = "pci";
>> +          compatible = "qcom,shikra-pcie";
>> +          reg = <0x0 0x045e8000 0x0 0x3000>,
>> +                <0x0 0x60000000 0x0 0xf1d>,
>> +                <0x0 0x60000f20 0x0 0xa8>,
>> +                <0x0 0x60001000 0x0 0x1000>,
>> +                <0x0 0x60100000 0x0 0x100000>,
>> +                <0x0 0x045eb000 0x0 0x1000>;
>> +          reg-names = "parf",
>> +                      "dbi",
>> +                      "elbi",
>> +                      "atu",
>> +                      "config",
>> +                      "mhi";
>> +          #address-cells = <3>;
>> +          #size-cells = <2>;
>> +          ranges = <0x01000000 0x0 0x00000000 0x0 0x60200000 0x0 0x100000>,
>> +                   <0x02000000 0x0 0x60300000 0x0 0x60300000 0x0 0x3d00000>,
>> +                   <0x03000000 0x4 0x00000000 0x4 0x00000000 0x3 0x0000000>;
>> +          bus-range = <0x00 0xff>;
>> +
>> +          linux,pci-domain = <0>;
>> +          num-lanes = <1>;
>> +
>> +          interrupts = <GIC_SPI 491 IRQ_TYPE_LEVEL_HIGH 0>,
>> +                       <GIC_SPI 492 IRQ_TYPE_LEVEL_HIGH 0>,
>> +                       <GIC_SPI 493 IRQ_TYPE_LEVEL_HIGH 0>,
>> +                       <GIC_SPI 494 IRQ_TYPE_LEVEL_HIGH 0>,
>> +                       <GIC_SPI 495 IRQ_TYPE_LEVEL_HIGH 0>,
>> +                       <GIC_SPI 496 IRQ_TYPE_LEVEL_HIGH 0>,
>> +                       <GIC_SPI 497 IRQ_TYPE_LEVEL_HIGH 0>,
>> +                       <GIC_SPI 498 IRQ_TYPE_LEVEL_HIGH 0>,
>> +                       <GIC_SPI 489 IRQ_TYPE_LEVEL_HIGH 0>;
>> +          interrupt-names = "msi0",
>> +                            "msi1",
>> +                            "msi2",
>> +                            "msi3",
>> +                            "msi4",
>> +                            "msi5",
>> +                            "msi6",
>> +                            "msi7",
>> +                            "global";
>> +
>> +          interrupt-map = <0 0 0 1 &intc 0 0 0 499 IRQ_TYPE_LEVEL_HIGH>,
>> +                          <0 0 0 2 &intc 0 0 0 500 IRQ_TYPE_LEVEL_HIGH>,
>> +                          <0 0 0 3 &intc 0 0 0 501 IRQ_TYPE_LEVEL_HIGH>,
>> +                          <0 0 0 4 &intc 0 0 0 502 IRQ_TYPE_LEVEL_HIGH>;
>> +          interrupt-map-mask = <0 0 0 0x7>;
>> +          #interrupt-cells = <1>;
>> +
>> +          clocks = <&gcc GCC_PCIE_AUX_CLK>,
>> +                   <&gcc GCC_PCIE_CFG_AHB_CLK>,
>> +                   <&gcc GCC_PCIE_MSTR_AXI_CLK>,
>> +                   <&gcc GCC_PCIE_SLV_AXI_CLK>,
>> +                   <&gcc GCC_PCIE_SLV_Q2A_AXI_CLK>,
>> +                   <&gcc GCC_DDRSS_MEMNOC_PCIE_SF_CLK>,
>> +                   <&gcc GCC_PCIE_TILE_AXI_SYS_NOC_CLK>,
>> +                   <&gcc GCC_QMIP_PCIE_CFG_AHB_CLK>;
>> +          clock-names = "aux",
>> +                        "cfg",
>> +                        "bus_master",
>> +                        "bus_slave",
>> +                        "slave_q2a",
>> +                        "ddrss_memnoc_pcie",
>> +                        "tile",
>> +                        "qmip_pcie_ahb";
>> +
>> +          assigned-clocks = <&gcc GCC_PCIE_AUX_CLK>;
>> +          assigned-clock-rates = <19200000>;
>> +
>> +          interconnects = <&system_noc MASTER_PCIE2_0 RPM_ALWAYS_TAG
>> +                          &mc_virt SLAVE_EBI_CH0 RPM_ALWAYS_TAG>,
>> +                          <&mem_noc MASTER_AMPSS_M0 RPM_ACTIVE_TAG
>> +                          &config_noc SLAVE_PCIE2_0 RPM_ACTIVE_TAG>;
>> +
>> +          interconnect-names = "pcie-mem",
>> +                               "cpu-pcie";
>> +
>> +          iommu-map = <0x0 &apps_smmu 0x800 0x1>,
>> +                      <0x100 &apps_smmu 0x801 0x1>;
>> +
>> +          resets = <&gcc GCC_PCIE_BCR>;
>> +          reset-names = "pci";
>> +
>> +          power-domains = <&gcc GCC_PCIE_GDSC>;
>> +
>> +          max-link-speed = <2>;
> You don't need 'max-link-speed' unless you want to limit the link speed.
This is needed since Shikra controller supports Gen3 but the PHY
is only capable of Gen2. Without limiting the speed, link-up fails.
>
>> +
>> +          operating-points-v2 = <&pcie_opp_table>;
>> +
>> +          status = "disabled";
>> +
> No, you should not disable the example.
ACK
>
>> +          pcie_opp_table: opp-table {
>> +                  compatible = "operating-points-v2";
>> +
>> +                  /* GEN 1 x1 */
>> +                  opp-2500000 {
>> +                          opp-hz = /bits/ 64 <2500000>;
>> +                          required-opps = <&rpmpd_opp_nom>;
> Are you sure that the power domain need to be in NOM for 2.5 GT/s?
Confirmed from HW team, both Gen1 and Gen2 can work in
Low SVS corner. Will be fixed in v2.
>
>> +                          opp-peak-kBps = <250000 1>;
>> +                          opp-level = <1>;
>> +                  };
>> +
>> +                  /* GEN 2 x1 */
>> +                  opp-5000000 {
>> +                          opp-hz = /bits/ 64 <5000000>;
>> +                          required-opps = <&rpmpd_opp_nom>;
> Same here.
ACK
>
>> +                          opp-peak-kBps = <500000 1>;
>> +                          opp-level = <2>;
>> +                  };
>> +          };
> Odd indent.
ACK
>
> - Mani
>