| Message ID | 1712257884-23841-2-git-send-email-quic_mrana@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add Qualcomm PCIe ECAM root complex driver | expand |
On 04/04/2024 21:11, Mayank Rana wrote: > On some of Qualcomm platform, firmware configures PCIe controller in RC On which? Your commit or binding must answer to all such questions. > mode with static iATU window mappings of configuration space for entire > supported bus range in ECAM compatible mode. Firmware also manages PCIe > PHY as well required system resources. Here document properties and > required configuration to power up QCOM PCIe ECAM compatible root complex > and PHY for PCIe functionality. > > Signed-off-by: Mayank Rana <quic_mrana@quicinc.com> > --- > .../devicetree/bindings/pci/qcom,pcie-ecam.yaml | 94 ++++++++++++++++++++++ > 1 file changed, 94 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml > new file mode 100644 > index 00000000..c209f12 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml > @@ -0,0 +1,94 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pci/qcom,pcie-ecam.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm ECAM compliant PCI express root complex > + > +description: | Do not need '|' unless you need to preserve formatting. > + Qualcomm SOC based ECAM compatible PCIe root complex supporting MSI controller. Which SoC? > + Firmware configures PCIe controller in RC mode with static iATU window mappings > + of configuration space for entire supported bus range in ECAM compatible mode. > + > +maintainers: > + - Mayank Rana <quic_mrana@quicinc.com> > + > +allOf: > + - $ref: /schemas/pci/pci-bus.yaml# > + - $ref: /schemas/power-domain/power-domain-consumer.yaml > + > +properties: > + compatible: > + const: qcom,pcie-ecam-rc No, this must have SoC specific compatibles. > + > + reg: > + minItems: 1 maxItems instead > + description: ECAM address space starting from root port till supported bus range > + > + interrupts: > + minItems: 1 > + maxItems: 8 This is way too unspecific. > + > + ranges: > + minItems: 2 > + maxItems: 3 Why variable? > + > + iommu-map: > + minItems: 1 > + maxItems: 16 Why variable? Open existing bindings and look how it is done. > + > + power-domains: > + maxItems: 1 > + description: A phandle to node which is able support way to communicate with firmware > + for enabling PCIe controller and PHY as well managing all system resources needed to > + make both controller and PHY operational for PCIe functionality. This description does not tell me much. Say something specific. And drop redundant parts like phandle. > + > + dma-coherent: true > + > +required: > + - compatible > + - reg > + - interrupts > + - ranges > + - power-domains > + - device_type > + - linux,pci-domain > + - bus-range > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + pcie0: pci@1c00000 { > + compatible = "qcom,pcie-ecam-rc"; > + reg = <0x4 0x00000000 0 0x10000000>; > + device_type = "pci"; > + #address-cells = <3>; > + #size-cells = <2>; > + ranges = <0x01000000 0x0 0x40000000 0x0 0x40000000 0x0 0x100000>, > + <0x02000000 0x0 0x40100000 0x0 0x40100000 0x0 0x1ff00000>, > + <0x43000000 0x4 0x10100000 0x4 0x10100000 0x0 0x100000>; Follow DTS coding style about placement and alignment. Best regards, Krzysztof
Hi Krzysztof On 4/4/2024 12:30 PM, Krzysztof Kozlowski wrote: > On 04/04/2024 21:11, Mayank Rana wrote: >> On some of Qualcomm platform, firmware configures PCIe controller in RC > > On which? > > Your commit or binding must answer to all such questions. > >> mode with static iATU window mappings of configuration space for entire >> supported bus range in ECAM compatible mode. Firmware also manages PCIe >> PHY as well required system resources. Here document properties and >> required configuration to power up QCOM PCIe ECAM compatible root complex >> and PHY for PCIe functionality. >> >> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com> >> --- >> .../devicetree/bindings/pci/qcom,pcie-ecam.yaml | 94 ++++++++++++++++++++++ >> 1 file changed, 94 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml >> >> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml >> new file mode 100644 >> index 00000000..c209f12 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml >> @@ -0,0 +1,94 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/pci/qcom,pcie-ecam.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Qualcomm ECAM compliant PCI express root complex >> + >> +description: | > Do not need '|' unless you need to preserve formatting. ACK > >> + Qualcomm SOC based ECAM compatible PCIe root complex supporting MSI controller. > > Which SoC? ACK >> + Firmware configures PCIe controller in RC mode with static iATU window mappings >> + of configuration space for entire supported bus range in ECAM compatible mode. >> + >> +maintainers: >> + - Mayank Rana <quic_mrana@quicinc.com> >> + >> +allOf: >> + - $ref: /schemas/pci/pci-bus.yaml# >> + - $ref: /schemas/power-domain/power-domain-consumer.yaml >> + >> +properties: >> + compatible: >> + const: qcom,pcie-ecam-rc > > No, this must have SoC specific compatibles. This driver is proposed to work with any PCIe controller supported ECAM functionality on Qualcomm platform where firmware running on other VM/processor is controlling PCIe PHY and controller for PCIe link up functionality. Do you still suggest to have SoC specific compatibles here ? >> + >> + reg: >> + minItems: 1 > > maxItems instead > >> + description: ECAM address space starting from root port till supported bus range >> + >> + interrupts: >> + minItems: 1 >> + maxItems: 8 > > This is way too unspecific. will review and update. >> + >> + ranges: >> + minItems: 2 >> + maxItems: 3 > > Why variable? It depends on how ECAM configured to support 32-bit and 64-bit based prefetch address space. So there are different combination of prefetch (32-bit or 64-bit or both) and non-prefetch (32-bit), and IO address space available. hence kept it as variable with based on required use case and address space availability. >> + >> + iommu-map: >> + minItems: 1 >> + maxItems: 16 > > Why variable? > > Open existing bindings and look how it is done. ok. will review and update as needed. > >> + >> + power-domains: >> + maxItems: 1 >> + description: A phandle to node which is able support way to communicate with firmware >> + for enabling PCIe controller and PHY as well managing all system resources needed to >> + make both controller and PHY operational for PCIe functionality. > > This description does not tell me much. Say something specific. And drop > redundant parts like phandle. ok. will rephrase it. > >> + >> + dma-coherent: true >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + - ranges >> + - power-domains >> + - device_type >> + - linux,pci-domain >> + - bus-range >> + >> +unevaluatedProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + soc { >> + #address-cells = <2>; >> + #size-cells = <2>; >> + pcie0: pci@1c00000 { >> + compatible = "qcom,pcie-ecam-rc"; >> + reg = <0x4 0x00000000 0 0x10000000>; >> + device_type = "pci"; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + ranges = <0x01000000 0x0 0x40000000 0x0 0x40000000 0x0 0x100000>, >> + <0x02000000 0x0 0x40100000 0x0 0x40100000 0x0 0x1ff00000>, >> + <0x43000000 0x4 0x10100000 0x4 0x10100000 0x0 0x100000>; > > Follow DTS coding style about placement and alignment. > > Best regards, > Krzysztof > Regards, Mayank
On 08/04/2024 21:09, Mayank Rana wrote: >>> + Firmware configures PCIe controller in RC mode with static iATU window mappings >>> + of configuration space for entire supported bus range in ECAM compatible mode. >>> + >>> +maintainers: >>> + - Mayank Rana <quic_mrana@quicinc.com> >>> + >>> +allOf: >>> + - $ref: /schemas/pci/pci-bus.yaml# >>> + - $ref: /schemas/power-domain/power-domain-consumer.yaml >>> + >>> +properties: >>> + compatible: >>> + const: qcom,pcie-ecam-rc >> >> No, this must have SoC specific compatibles. > This driver is proposed to work with any PCIe controller supported ECAM > functionality on Qualcomm platform > where firmware running on other VM/processor is controlling PCIe PHY and > controller for PCIe link up functionality. > Do you still suggest to have SoC specific compatibles here ? What does the writing-bindings document say? Why this is different than all other bindings? >>> + >>> + reg: >>> + minItems: 1 >> >> maxItems instead >> >>> + description: ECAM address space starting from root port till supported bus range >>> + >>> + interrupts: >>> + minItems: 1 >>> + maxItems: 8 >> >> This is way too unspecific. > will review and update. >>> + >>> + ranges: >>> + minItems: 2 >>> + maxItems: 3 >> >> Why variable? > It depends on how ECAM configured to support 32-bit and 64-bit based > prefetch address space. > So there are different combination of prefetch (32-bit or 64-bit or > both) and non-prefetch (32-bit), and IO address space available. hence > kept it as variable with based on required use case and address space > availability. Really? So same device has it configured once for 32 once for 64-bit address space? Randomly? Best regards, Krzysztof
On 4/8/2024 11:21 PM, Krzysztof Kozlowski wrote: > On 08/04/2024 21:09, Mayank Rana wrote: >>>> + Firmware configures PCIe controller in RC mode with static iATU window mappings >>>> + of configuration space for entire supported bus range in ECAM compatible mode. >>>> + >>>> +maintainers: >>>> + - Mayank Rana <quic_mrana@quicinc.com> >>>> + >>>> +allOf: >>>> + - $ref: /schemas/pci/pci-bus.yaml# >>>> + - $ref: /schemas/power-domain/power-domain-consumer.yaml >>>> + >>>> +properties: >>>> + compatible: >>>> + const: qcom,pcie-ecam-rc >>> >>> No, this must have SoC specific compatibles. >> This driver is proposed to work with any PCIe controller supported ECAM >> functionality on Qualcomm platform >> where firmware running on other VM/processor is controlling PCIe PHY and >> controller for PCIe link up functionality. >> Do you still suggest to have SoC specific compatibles here ? > > What does the writing-bindings document say? Why this is different than > all other bindings? Thank you for all your review comment and suggestions. If it is must to have SOC name, then I am not sure how pci-host-generic.c driver having non SOC prefix for standard ECAM driver. I am here saying this is QCOM vendor specific generic ECAM driver. saying that it seems that I would be updating now pci-host-generic.c driver to add generic functionality as Rob suggested part of review comment. With that I am seeing possible options as i.e. continue using default generic compatible as "pcie-host-ecam-generic" OR use new as "qcom,pcie-host-ecam-generic". will this work ?>>>> + >>>> + reg: >>>> + minItems: 1 >>> >>> maxItems instead >>> >>>> + description: ECAM address space starting from root port till supported bus range >>>> + >>>> + interrupts: >>>> + minItems: 1 >>>> + maxItems: 8 >>> >>> This is way too unspecific. >> will review and update. >>>> + >>>> + ranges: >>>> + minItems: 2 >>>> + maxItems: 3 >>> >>> Why variable? >> It depends on how ECAM configured to support 32-bit and 64-bit based >> prefetch address space. >> So there are different combination of prefetch (32-bit or 64-bit or >> both) and non-prefetch (32-bit), and IO address space available. hence >> kept it as variable with based on required use case and address space >> availability. > > Really? So same device has it configured once for 32 once for 64-bit > address space? Randomly? no. as binding is not saying for any specific SOC. Depends on memory map on particular SOC, how PCIe address space available based on that this would change for particular SOC variant. > Best regards, > Krzysztof >
On 18/04/2024 20:56, Mayank Rana wrote: > > > On 4/8/2024 11:21 PM, Krzysztof Kozlowski wrote: >> On 08/04/2024 21:09, Mayank Rana wrote: >>>>> + Firmware configures PCIe controller in RC mode with static iATU window mappings >>>>> + of configuration space for entire supported bus range in ECAM compatible mode. >>>>> + >>>>> +maintainers: >>>>> + - Mayank Rana <quic_mrana@quicinc.com> >>>>> + >>>>> +allOf: >>>>> + - $ref: /schemas/pci/pci-bus.yaml# >>>>> + - $ref: /schemas/power-domain/power-domain-consumer.yaml >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + const: qcom,pcie-ecam-rc >>>> >>>> No, this must have SoC specific compatibles. >>> This driver is proposed to work with any PCIe controller supported ECAM >>> functionality on Qualcomm platform >>> where firmware running on other VM/processor is controlling PCIe PHY and >>> controller for PCIe link up functionality. >>> Do you still suggest to have SoC specific compatibles here ? >> >> What does the writing-bindings document say? Why this is different than >> all other bindings? > Thank you for all your review comment and suggestions. > > If it is must to have SOC name, then I am not sure how > pci-host-generic.c driver having non SOC prefix for standard ECAM > driver. I am here saying this is QCOM vendor specific generic ECAM > driver. saying that it seems that I would be updating now > pci-host-generic.c driver to add generic functionality as Rob suggested I don't see any problem here. I talk about bindings, not driver. You can have also fallback, so how is it different than from existing code? > part of review comment. With > that I am seeing possible options as i.e. continue using default generic > compatible as "pcie-host-ecam-generic" OR use new as > "qcom,pcie-host-ecam-generic". will this work ?>>>> + Compatible and bindings focus on the hardware, so just write them describing the hardware. You keep asking it in context of driver, but I would say it does not matter. Is this generic hardware/firmware implementation or not? >>>>> + reg: >>>>> + minItems: 1 >>>> >>>> maxItems instead >>>> >>>>> + description: ECAM address space starting from root port till supported bus range >>>>> + >>>>> + interrupts: >>>>> + minItems: 1 >>>>> + maxItems: 8 >>>> >>>> This is way too unspecific. >>> will review and update. >>>>> + >>>>> + ranges: >>>>> + minItems: 2 >>>>> + maxItems: 3 >>>> >>>> Why variable? >>> It depends on how ECAM configured to support 32-bit and 64-bit based >>> prefetch address space. >>> So there are different combination of prefetch (32-bit or 64-bit or >>> both) and non-prefetch (32-bit), and IO address space available. hence >>> kept it as variable with based on required use case and address space >>> availability. >> >> Really? So same device has it configured once for 32 once for 64-bit >> address space? Randomly? > no. as binding is not saying for any specific SOC. Depends on memory map > on particular SOC, how PCIe address space available based on that this So specific to the SoC, so this is not variable. > would change for particular SOC variant. So this is not variable and you did not provide sufficient argumentation. You basically did not provide any argument, just disagreed with me. Bindings must be specific and all fields should be constrained, when reasonable. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml new file mode 100644 index 00000000..c209f12 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml @@ -0,0 +1,94 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pci/qcom,pcie-ecam.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm ECAM compliant PCI express root complex + +description: | + Qualcomm SOC based ECAM compatible PCIe root complex supporting MSI controller. + Firmware configures PCIe controller in RC mode with static iATU window mappings + of configuration space for entire supported bus range in ECAM compatible mode. + +maintainers: + - Mayank Rana <quic_mrana@quicinc.com> + +allOf: + - $ref: /schemas/pci/pci-bus.yaml# + - $ref: /schemas/power-domain/power-domain-consumer.yaml + +properties: + compatible: + const: qcom,pcie-ecam-rc + + reg: + minItems: 1 + description: ECAM address space starting from root port till supported bus range + + interrupts: + minItems: 1 + maxItems: 8 + + ranges: + minItems: 2 + maxItems: 3 + + iommu-map: + minItems: 1 + maxItems: 16 + + power-domains: + maxItems: 1 + description: A phandle to node which is able support way to communicate with firmware + for enabling PCIe controller and PHY as well managing all system resources needed to + make both controller and PHY operational for PCIe functionality. + + dma-coherent: true + +required: + - compatible + - reg + - interrupts + - ranges + - power-domains + - device_type + - linux,pci-domain + - bus-range + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + soc { + #address-cells = <2>; + #size-cells = <2>; + pcie0: pci@1c00000 { + compatible = "qcom,pcie-ecam-rc"; + reg = <0x4 0x00000000 0 0x10000000>; + device_type = "pci"; + #address-cells = <3>; + #size-cells = <2>; + ranges = <0x01000000 0x0 0x40000000 0x0 0x40000000 0x0 0x100000>, + <0x02000000 0x0 0x40100000 0x0 0x40100000 0x0 0x1ff00000>, + <0x43000000 0x4 0x10100000 0x4 0x10100000 0x0 0x100000>; + bus-range = <0x00 0xff>; + dma-coherent; + linux,pci-domain = <0>; + power-domains = <&scmi5_pd 0>; + power-domain-names = "power"; + iommu-map = <0x0 &pcie_smmu 0x0000 0x1>, + <0x100 &pcie_smmu 0x0001 0x1>; + interrupt-parent = <&intc>; + interrupts = <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 308 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 313 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 314 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 374 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 375 IRQ_TYPE_LEVEL_HIGH>; + }; + }; +...
On some of Qualcomm platform, firmware configures PCIe controller in RC mode with static iATU window mappings of configuration space for entire supported bus range in ECAM compatible mode. Firmware also manages PCIe PHY as well required system resources. Here document properties and required configuration to power up QCOM PCIe ECAM compatible root complex and PHY for PCIe functionality. Signed-off-by: Mayank Rana <quic_mrana@quicinc.com> --- .../devicetree/bindings/pci/qcom,pcie-ecam.yaml | 94 ++++++++++++++++++++++ 1 file changed, 94 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml