| Message ID | 20240803-qps615-v2-1-9560b7c71369@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | PCI: Enable Power and configure the QPS615 PCIe switch | expand |
On Sat, 03 Aug 2024 08:52:47 +0530, Krishna chaitanya chundru wrote: > Add binding describing the Qualcomm PCIe switch, QPS615, > which provides Ethernet MAC integrated to the 3rd downstream port > and two downstream PCIe ports. > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > --- > .../devicetree/bindings/pci/qcom,qps615.yaml | 191 +++++++++++++++++++++ > 1 file changed, 191 insertions(+) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/pci/qcom,qps615.example.dts:33.26-101.19: Warning (pci_device_bus_num): /example-0/pcie/pcie@0/pcie@0,0: PCI bus number 1 out of range, expected (0 - 0) Documentation/devicetree/bindings/pci/qcom,qps615.example.dts:52.30-60.23: Warning (pci_device_bus_num): /example-0/pcie/pcie@0/pcie@0,0/pcie@1,0: PCI bus number 2 out of range, expected (0 - 0) Documentation/devicetree/bindings/pci/qcom,qps615.example.dts:62.30-70.23: Warning (pci_device_bus_num): /example-0/pcie/pcie@0/pcie@0,0/pcie@2,0: PCI bus number 2 out of range, expected (0 - 0) Documentation/devicetree/bindings/pci/qcom,qps615.example.dts:72.30-100.23: Warning (pci_device_bus_num): /example-0/pcie/pcie@0/pcie@0,0/pcie@3,0: PCI bus number 2 out of range, expected (0 - 0) Documentation/devicetree/bindings/pci/qcom,qps615.example.dts:81.39-89.32: Warning (pci_device_bus_num): /example-0/pcie/pcie@0/pcie@0,0/pcie@3,0/pcie@0,0: PCI bus number 4 out of range, expected (0 - 0) Documentation/devicetree/bindings/pci/qcom,qps615.example.dts:91.39-99.32: Warning (pci_device_bus_num): /example-0/pcie/pcie@0/pcie@0,0/pcie@3,0/pcie@0,1: PCI bus number 4 out of range, expected (0 - 0) doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240803-qps615-v2-1-9560b7c71369@quicinc.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Sat, Aug 03, 2024 at 08:52:47AM GMT, Krishna chaitanya chundru wrote: > Add binding describing the Qualcomm PCIe switch, QPS615, > which provides Ethernet MAC integrated to the 3rd downstream port > and two downstream PCIe ports. > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > --- > .../devicetree/bindings/pci/qcom,qps615.yaml | 191 +++++++++++++++++++++ > 1 file changed, 191 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > new file mode 100644 > index 000000000000..ea0c953ee56f > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > @@ -0,0 +1,191 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pci/qcom,qps615.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm QPS615 PCIe switch > + > +maintainers: > + - Krishna chaitanya chundru <quic_krichai@quicinc.com> > + > +description: | > + Qualcomm QPS615 PCIe switch has one upstream and three downstream > + ports. The 3rd downstream port has integrated endpoint device of > + Ethernet MAC. Other two downstream ports are supposed to connect > + to external device. > + > + The QPS615 PCIe switch can be configured through I2C interface before > + PCIe link is established to change FTS, ASPM related entry delays, > + tx amplitude etc for better power efficiency and functionality. > + > +properties: > + compatible: > + enum: > + - pci1179,0623 > + > + reg: > + maxItems: 1 > + > + qcom,qps615-controller: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + Reference to the I2C client used to do configure qps615 > + > + vdd18-supply: true > + > + vdd09-supply: true > + > + vddc-supply: true > + > + vddio1-supply: true > + > + vddio2-supply: true > + > + vddio18-supply: true > + > + reset-gpios: > + maxItems: 1 > + description: > + GPIO controlling the RESX# pin. > + > + qps615,axi-clk-freq-hz: > + description: > + AXI clock which internal bus of the switch. Is it a clock or clock rate? > + > + qcom,l0s-entry-delay-ns: > + description: Aspm l0s entry delay in nanoseconds. I'd say, from the property name it is obvious that it comes in nanoseconds. > + > + qcom,l1-entry-delay-ns: > + description: Aspm l1 entry delay in nanoseconds. > + > + qcom,tx-amplitude-millivolt: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: Change Tx Margin setting for low power consumption. > + > + qcom,no-dfe: > + type: boolean > + description: Disables DFE (Decision Feedback Equalizer). > + > + qcom,nfts: > + $ref: /schemas/types.yaml#/definitions/uint8 > + description: > + Fast Training Sequence (FTS) is the mechanism that > + is used for bit and Symbol lock. Doesn't help to understand what it is and what the value means. > + > +allOf: > + - $ref: /schemas/pci/pci-bus-common.yaml# > + - if: > + properties: > + compatible: > + contains: > + const: pci1179,0623 > + required: > + - compatible > + then: > + required: > + - vdd18-supply > + - vdd09-supply > + - vddc-supply > + - vddio1-supply > + - vddio2-supply > + - vddio18-supply > + - qcom,qps615-controller > + - reset-gpios > + > +patternProperties: > + "@1?[0-9a-f](,[0-7])?$": > + type: object > + $ref: qcom,qps615.yaml# > + additionalProperties: true > + > +additionalProperties: true > + > +examples: > + - | > + > + #include <dt-bindings/gpio/gpio.h> > + > + pcie { > + #address-cells = <3>; > + #size-cells = <2>; > + > + pcie@0 { > + device_type = "pci"; > + reg = <0x0 0x0 0x0 0x0 0x0>; > + > + #address-cells = <3>; > + #size-cells = <2>; > + ranges; > + > + pcie@0,0 { > + compatible = "pci1179,0623"; > + reg = <0x10000 0x0 0x0 0x0 0x0>; > + device_type = "pci"; > + #address-cells = <3>; > + #size-cells = <2>; > + ranges; > + > + qcom,qps615-controller = <&qps615_controller>; Where is the corresponding device? > + > + vdd18-supply = <&vdd>; > + vdd09-supply = <&vdd>; > + vddc-supply = <&vdd>; > + vddio1-supply = <&vdd>; > + vddio2-supply = <&vdd>; > + vddio18-supply = <&vdd>; > + > + reset-gpios = <&gpio 1 GPIO_ACTIVE_LOW>; > + > + pcie@1,0 { > + reg = <0x20800 0x0 0x0 0x0 0x0>; > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; > + ranges; > + > + qcom,no-dfe; > + }; > + > + pcie@2,0 { > + reg = <0x21000 0x0 0x0 0x0 0x0>; > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; > + ranges; > + > + qcom,nfts = /bits/ 8 <10>; > + }; > + > + pcie@3,0 { > + reg = <0x21800 0x0 0x0 0x0 0x0>; > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; > + ranges; > + > + qcom,tx-amplitude-millivolt = <10>; > + > + pcie@0,0 { Wrong indentation. > + reg = <0x40000 0x0 0x0 0x0 0x0>; > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; > + ranges; > + > + qcom,l1-entry-delay-ns = <10>; > + }; > + > + pcie@0,1 { > + reg = <0x40100 0x0 0x0 0x0 0x0>; > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; > + ranges; > + > + qcom,l0s-entry-delay-ns = <10>; > + }; > + }; > + }; > + }; > + }; > > -- > 2.34.1 >
On 03/08/2024 05:22, Krishna chaitanya chundru wrote: > Add binding describing the Qualcomm PCIe switch, QPS615, > which provides Ethernet MAC integrated to the 3rd downstream port > and two downstream PCIe ports. > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > --- > .../devicetree/bindings/pci/qcom,qps615.yaml | 191 +++++++++++++++++++++ > 1 file changed, 191 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > new file mode 100644 > index 000000000000..ea0c953ee56f > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > @@ -0,0 +1,191 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pci/qcom,qps615.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm QPS615 PCIe switch > + > +maintainers: > + - Krishna chaitanya chundru <quic_krichai@quicinc.com> > + > +description: | > + Qualcomm QPS615 PCIe switch has one upstream and three downstream > + ports. The 3rd downstream port has integrated endpoint device of > + Ethernet MAC. Other two downstream ports are supposed to connect > + to external device. > + > + The QPS615 PCIe switch can be configured through I2C interface before > + PCIe link is established to change FTS, ASPM related entry delays, > + tx amplitude etc for better power efficiency and functionality. > + > +properties: > + compatible: > + enum: > + - pci1179,0623 > + > + reg: > + maxItems: 1 > + > + qcom,qps615-controller: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + Reference to the I2C client used to do configure qps615 Why? > + > + vdd18-supply: true > + > + vdd09-supply: true > + > + vddc-supply: true > + > + vddio1-supply: true > + > + vddio2-supply: true > + > + vddio18-supply: true > + > + reset-gpios: > + maxItems: 1 > + description: > + GPIO controlling the RESX# pin. > + > + qps615,axi-clk-freq-hz: > + description: > + AXI clock which internal bus of the switch. No need, use CCF. > + > + qcom,l0s-entry-delay-ns: > + description: Aspm l0s entry delay in nanoseconds. > + > + qcom,l1-entry-delay-ns: > + description: Aspm l1 entry delay in nanoseconds. > + > + qcom,tx-amplitude-millivolt: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: Change Tx Margin setting for low power consumption. > + > + qcom,no-dfe: > + type: boolean > + description: Disables DFE (Decision Feedback Equalizer). You described the desired Linux feature or behavior, not the actual hardware. The bindings are about the latter, so instead you need to rephrase the property and its description to match actual hardware capabilities/features/configuration etc. > + > + qcom,nfts: > + $ref: /schemas/types.yaml#/definitions/uint8 > + description: > + Fast Training Sequence (FTS) is the mechanism that > + is used for bit and Symbol lock. What are the values? Why this is uint8? You described the desired Linux feature or behavior, not the actual hardware. The bindings are about the latter, so instead you need to rephrase the property and its description to match actual hardware capabilities/features/configuration etc. > + > +allOf: > + - $ref: /schemas/pci/pci-bus-common.yaml# > + - if: > + properties: > + compatible: > + contains: > + const: pci1179,0623 > + required: > + - compatible Why do you have entire if? You do not have multiple variants, drop. > + then: > + required: > + - vdd18-supply > + - vdd09-supply > + - vddc-supply > + - vddio1-supply > + - vddio2-supply > + - vddio18-supply > + - qcom,qps615-controller > + - reset-gpios > + > +patternProperties: > + "@1?[0-9a-f](,[0-7])?$": > + type: object > + $ref: qcom,qps615.yaml# > + additionalProperties: true Nope, drop pattern Properties or explain what is this. > + > +additionalProperties: true This cannot be true, > + > +examples: > + - | > + > + #include <dt-bindings/gpio/gpio.h> > + > + pcie { > + #address-cells = <3>; > + #size-cells = <2>; > + > + pcie@0 { > + device_type = "pci"; > + reg = <0x0 0x0 0x0 0x0 0x0>; > + > + #address-cells = <3>; > + #size-cells = <2>; > + ranges; > + > + pcie@0,0 { > + compatible = "pci1179,0623"; > + reg = <0x10000 0x0 0x0 0x0 0x0>; > + device_type = "pci"; > + #address-cells = <3>; > + #size-cells = <2>; > + ranges; > + > + qcom,qps615-controller = <&qps615_controller>; > + > + vdd18-supply = <&vdd>; > + vdd09-supply = <&vdd>; > + vddc-supply = <&vdd>; > + vddio1-supply = <&vdd>; > + vddio2-supply = <&vdd>; > + vddio18-supply = <&vdd>; > + > + reset-gpios = <&gpio 1 GPIO_ACTIVE_LOW>; > + > + pcie@1,0 { > + reg = <0x20800 0x0 0x0 0x0 0x0>; Where is the compatible? You claim this is the same device as child? > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; > + ranges; > + > + qcom,no-dfe; > + }; > + > + pcie@2,0 { > + reg = <0x21000 0x0 0x0 0x0 0x0>; > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; > + ranges; > + > + qcom,nfts = /bits/ 8 <10>; > + }; > + > + pcie@3,0 { > + reg = <0x21800 0x0 0x0 0x0 0x0>; > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; > + ranges; > + > + qcom,tx-amplitude-millivolt = <10>; > + > + pcie@0,0 { Total mess in indentation. Best regards, Krzysztof
On 03/08/2024 05:22, Krishna chaitanya chundru wrote: > Add binding describing the Qualcomm PCIe switch, QPS615, > which provides Ethernet MAC integrated to the 3rd downstream port > and two downstream PCIe ports. > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > --- > .../devicetree/bindings/pci/qcom,qps615.yaml | 191 +++++++++++++++++++++ > 1 file changed, 191 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > new file mode 100644 > index 000000000000..ea0c953ee56f > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > @@ -0,0 +1,191 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pci/qcom,qps615.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm QPS615 PCIe switch > + > +maintainers: > + - Krishna chaitanya chundru <quic_krichai@quicinc.com> > + > +description: | > + Qualcomm QPS615 PCIe switch has one upstream and three downstream > + ports. The 3rd downstream port has integrated endpoint device of > + Ethernet MAC. Other two downstream ports are supposed to connect > + to external device. > + > + The QPS615 PCIe switch can be configured through I2C interface before > + PCIe link is established to change FTS, ASPM related entry delays, > + tx amplitude etc for better power efficiency and functionality. > + > +properties: > + compatible: > + enum: > + - pci1179,0623 > + > + reg: > + maxItems: 1 > + > + qcom,qps615-controller: and now I see that you totally ignored comments. Repeating the same over and over is a waste of time. <form letter> This is a friendly reminder during the review process. It seems my or other reviewer's previous comments were not fully addressed. Maybe the feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them. Thank you. </form letter> Best regards, Krzysztof
On 8/4/2024 2:26 PM, Krzysztof Kozlowski wrote: > On 03/08/2024 05:22, Krishna chaitanya chundru wrote: >> Add binding describing the Qualcomm PCIe switch, QPS615, >> which provides Ethernet MAC integrated to the 3rd downstream port >> and two downstream PCIe ports. >> >> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >> --- >> .../devicetree/bindings/pci/qcom,qps615.yaml | 191 +++++++++++++++++++++ >> 1 file changed, 191 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >> new file mode 100644 >> index 000000000000..ea0c953ee56f >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >> @@ -0,0 +1,191 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/pci/qcom,qps615.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Qualcomm QPS615 PCIe switch >> + >> +maintainers: >> + - Krishna chaitanya chundru <quic_krichai@quicinc.com> >> + >> +description: | >> + Qualcomm QPS615 PCIe switch has one upstream and three downstream >> + ports. The 3rd downstream port has integrated endpoint device of >> + Ethernet MAC. Other two downstream ports are supposed to connect >> + to external device. >> + >> + The QPS615 PCIe switch can be configured through I2C interface before >> + PCIe link is established to change FTS, ASPM related entry delays, >> + tx amplitude etc for better power efficiency and functionality. >> + >> +properties: >> + compatible: >> + enum: >> + - pci1179,0623 >> + >> + reg: >> + maxItems: 1 >> + >> + qcom,qps615-controller: > > and now I see that you totally ignored comments. Repeating the same over > and over is a waste of time. > > <form letter> > This is a friendly reminder during the review process. > > It seems my or other reviewer's previous comments were not fully > addressed. Maybe the feedback got lost between the quotes, maybe you > just forgot to apply it. Please go back to the previous discussion and > either implement all requested changes or keep discussing them. > > Thank you. > </form letter> > > > Best regards, > Krzysztof > Hi Krzysztof, In patch1 we are trying to add reference of i2c-adapter, you suggested to use i2c-bus for that. we got comments on the driver code not to use adapter and instead use i2c client reference. I felt i2c-bus is not ideal to represent i2c client device so used this name. I should have mentioned all these details in the change log for all these changes. Next time onwards I will give detailed change log for the changes between two versions. - Krishna Chaitanya.
On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote: > On 03/08/2024 05:22, Krishna chaitanya chundru wrote: >> Add binding describing the Qualcomm PCIe switch, QPS615, >> which provides Ethernet MAC integrated to the 3rd downstream port >> and two downstream PCIe ports. >> >> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >> --- >> .../devicetree/bindings/pci/qcom,qps615.yaml | 191 +++++++++++++++++++++ >> 1 file changed, 191 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >> new file mode 100644 >> index 000000000000..ea0c953ee56f >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >> @@ -0,0 +1,191 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/pci/qcom,qps615.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Qualcomm QPS615 PCIe switch >> + >> +maintainers: >> + - Krishna chaitanya chundru <quic_krichai@quicinc.com> >> + >> +description: | >> + Qualcomm QPS615 PCIe switch has one upstream and three downstream >> + ports. The 3rd downstream port has integrated endpoint device of >> + Ethernet MAC. Other two downstream ports are supposed to connect >> + to external device. >> + >> + The QPS615 PCIe switch can be configured through I2C interface before >> + PCIe link is established to change FTS, ASPM related entry delays, >> + tx amplitude etc for better power efficiency and functionality. >> + >> +properties: >> + compatible: >> + enum: >> + - pci1179,0623 >> + >> + reg: >> + maxItems: 1 >> + >> + qcom,qps615-controller: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: >> + Reference to the I2C client used to do configure qps615 > > Why? > >> + >> + vdd18-supply: true >> + >> + vdd09-supply: true >> + >> + vddc-supply: true >> + >> + vddio1-supply: true >> + >> + vddio2-supply: true >> + >> + vddio18-supply: true >> + >> + reset-gpios: >> + maxItems: 1 >> + description: >> + GPIO controlling the RESX# pin. >> + >> + qps615,axi-clk-freq-hz: >> + description: >> + AXI clock which internal bus of the switch. > > No need, use CCF. > ack >> + >> + qcom,l0s-entry-delay-ns: >> + description: Aspm l0s entry delay in nanoseconds. >> + >> + qcom,l1-entry-delay-ns: >> + description: Aspm l1 entry delay in nanoseconds. >> + >> + qcom,tx-amplitude-millivolt: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: Change Tx Margin setting for low power consumption. >> + >> + qcom,no-dfe: >> + type: boolean >> + description: Disables DFE (Decision Feedback Equalizer). > > You described the desired Linux feature or behavior, not the actual > hardware. The bindings are about the latter, so instead you need to > rephrase the property and its description to match actual hardware > capabilities/features/configuration etc. > ack >> + >> + qcom,nfts: >> + $ref: /schemas/types.yaml#/definitions/uint8 >> + description: >> + Fast Training Sequence (FTS) is the mechanism that >> + is used for bit and Symbol lock. > > What are the values? Why this is uint8? > These represents number of fast training sequence and doesn't have any units and the maximum value for this is 0xFF only so we used uint8. > You described the desired Linux feature or behavior, not the actual > hardware. The bindings are about the latter, so instead you need to > rephrase the property and its description to match actual hardware > capabilities/features/configuration etc. ack. > >> + >> +allOf: >> + - $ref: /schemas/pci/pci-bus-common.yaml# >> + - if: >> + properties: >> + compatible: >> + contains: >> + const: pci1179,0623 >> + required: >> + - compatible > > Why do you have entire if? You do not have multiple variants, drop. > The child nodes also referencing the qcom,qps615.yaml# node, I tried to use this way to say "the below properties are for the required for parent and optional for child". >> + then: >> + required: >> + - vdd18-supply >> + - vdd09-supply >> + - vddc-supply >> + - vddio1-supply >> + - vddio2-supply >> + - vddio18-supply >> + - qcom,qps615-controller >> + - reset-gpios >> + >> +patternProperties: >> + "@1?[0-9a-f](,[0-7])?$": >> + type: object >> + $ref: qcom,qps615.yaml# >> + additionalProperties: true > > Nope, drop pattern Properties or explain what is this. > the child nodes represent the downstream ports of the PCIe switch which wants to use same properties that is why I tried to use this pattern properties. >> + >> +additionalProperties: true > > This cannot be true, > Let me check on this correct in next versions. >> + >> +examples: >> + - | >> + >> + #include <dt-bindings/gpio/gpio.h> >> + >> + pcie { >> + #address-cells = <3>; >> + #size-cells = <2>; >> + >> + pcie@0 { >> + device_type = "pci"; >> + reg = <0x0 0x0 0x0 0x0 0x0>; >> + >> + #address-cells = <3>; >> + #size-cells = <2>; >> + ranges; >> + >> + pcie@0,0 { >> + compatible = "pci1179,0623"; >> + reg = <0x10000 0x0 0x0 0x0 0x0>; >> + device_type = "pci"; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + ranges; >> + >> + qcom,qps615-controller = <&qps615_controller>; >> + >> + vdd18-supply = <&vdd>; >> + vdd09-supply = <&vdd>; >> + vddc-supply = <&vdd>; >> + vddio1-supply = <&vdd>; >> + vddio2-supply = <&vdd>; >> + vddio18-supply = <&vdd>; >> + >> + reset-gpios = <&gpio 1 GPIO_ACTIVE_LOW>; >> + >> + pcie@1,0 { >> + reg = <0x20800 0x0 0x0 0x0 0x0>; > > Where is the compatible? You claim this is the same device as child? > These all the PCIe downstream nodes and compatible is optional for the PCIe nodes. >> + #address-cells = <3>; >> + #size-cells = <2>; >> + device_type = "pci"; >> + ranges; >> + >> + qcom,no-dfe; >> + }; >> + >> + pcie@2,0 { >> + reg = <0x21000 0x0 0x0 0x0 0x0>; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + device_type = "pci"; >> + ranges; >> + >> + qcom,nfts = /bits/ 8 <10>; >> + }; >> + >> + pcie@3,0 { >> + reg = <0x21800 0x0 0x0 0x0 0x0>; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + device_type = "pci"; >> + ranges; >> + >> + qcom,tx-amplitude-millivolt = <10>; >> + >> + pcie@0,0 { > > Total mess in indentation. > Let me correct in next version. - Krishna Chaitanya. > > > Best regards, > Krzysztof >
On 8/3/2024 4:30 PM, Dmitry Baryshkov wrote: > On Sat, Aug 03, 2024 at 08:52:47AM GMT, Krishna chaitanya chundru wrote: >> Add binding describing the Qualcomm PCIe switch, QPS615, >> which provides Ethernet MAC integrated to the 3rd downstream port >> and two downstream PCIe ports. >> >> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >> --- >> .../devicetree/bindings/pci/qcom,qps615.yaml | 191 +++++++++++++++++++++ >> 1 file changed, 191 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >> new file mode 100644 >> index 000000000000..ea0c953ee56f >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >> @@ -0,0 +1,191 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/pci/qcom,qps615.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Qualcomm QPS615 PCIe switch >> + >> +maintainers: >> + - Krishna chaitanya chundru <quic_krichai@quicinc.com> >> + >> +description: | >> + Qualcomm QPS615 PCIe switch has one upstream and three downstream >> + ports. The 3rd downstream port has integrated endpoint device of >> + Ethernet MAC. Other two downstream ports are supposed to connect >> + to external device. >> + >> + The QPS615 PCIe switch can be configured through I2C interface before >> + PCIe link is established to change FTS, ASPM related entry delays, >> + tx amplitude etc for better power efficiency and functionality. >> + >> +properties: >> + compatible: >> + enum: >> + - pci1179,0623 >> + >> + reg: >> + maxItems: 1 >> + >> + qcom,qps615-controller: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: >> + Reference to the I2C client used to do configure qps615 >> + >> + vdd18-supply: true >> + >> + vdd09-supply: true >> + >> + vddc-supply: true >> + >> + vddio1-supply: true >> + >> + vddio2-supply: true >> + >> + vddio18-supply: true >> + >> + reset-gpios: >> + maxItems: 1 >> + description: >> + GPIO controlling the RESX# pin. >> + >> + qps615,axi-clk-freq-hz: >> + description: >> + AXI clock which internal bus of the switch. > > Is it a clock or clock rate? It is clock ony. > >> + >> + qcom,l0s-entry-delay-ns: >> + description: Aspm l0s entry delay in nanoseconds. > > I'd say, from the property name it is obvious that it comes in > nanoseconds. > I will remove the description from these properties. >> + >> + qcom,l1-entry-delay-ns: >> + description: Aspm l1 entry delay in nanoseconds. >> + >> + qcom,tx-amplitude-millivolt: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: Change Tx Margin setting for low power consumption. >> + >> + qcom,no-dfe: >> + type: boolean >> + description: Disables DFE (Decision Feedback Equalizer). >> + >> + qcom,nfts: >> + $ref: /schemas/types.yaml#/definitions/uint8 >> + description: >> + Fast Training Sequence (FTS) is the mechanism that >> + is used for bit and Symbol lock. > > Doesn't help to understand what it is and what the value means. > I will update the description, this property represents number of fast training sequence needs to be used for link transition from L0s to L0. - Krishna Chaitanya. >> >> +allOf: >> + - $ref: /schemas/pci/pci-bus-common.yaml# >> + - if: >> + properties: >> + compatible: >> + contains: >> + const: pci1179,0623 >> + required: >> + - compatible >> + then: >> + required: >> + - vdd18-supply >> + - vdd09-supply >> + - vddc-supply >> + - vddio1-supply >> + - vddio2-supply >> + - vddio18-supply >> + - qcom,qps615-controller >> + - reset-gpios >> + >> +patternProperties: >> + "@1?[0-9a-f](,[0-7])?$": >> + type: object >> + $ref: qcom,qps615.yaml# >> + additionalProperties: true >> + >> +additionalProperties: true >> + >> +examples: >> + - | >> + >> + #include <dt-bindings/gpio/gpio.h> >> + >> + pcie { >> + #address-cells = <3>; >> + #size-cells = <2>; >> + >> + pcie@0 { >> + device_type = "pci"; >> + reg = <0x0 0x0 0x0 0x0 0x0>; >> + >> + #address-cells = <3>; >> + #size-cells = <2>; >> + ranges; >> + >> + pcie@0,0 { >> + compatible = "pci1179,0623"; >> + reg = <0x10000 0x0 0x0 0x0 0x0>; >> + device_type = "pci"; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + ranges; >> + >> + qcom,qps615-controller = <&qps615_controller>; > > Where is the corresponding device? > >> + >> + vdd18-supply = <&vdd>; >> + vdd09-supply = <&vdd>; >> + vddc-supply = <&vdd>; >> + vddio1-supply = <&vdd>; >> + vddio2-supply = <&vdd>; >> + vddio18-supply = <&vdd>; >> + >> + reset-gpios = <&gpio 1 GPIO_ACTIVE_LOW>; >> + >> + pcie@1,0 { >> + reg = <0x20800 0x0 0x0 0x0 0x0>; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + device_type = "pci"; >> + ranges; >> + >> + qcom,no-dfe; >> + }; >> + >> + pcie@2,0 { >> + reg = <0x21000 0x0 0x0 0x0 0x0>; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + device_type = "pci"; >> + ranges; >> + >> + qcom,nfts = /bits/ 8 <10>; >> + }; >> + >> + pcie@3,0 { >> + reg = <0x21800 0x0 0x0 0x0 0x0>; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + device_type = "pci"; >> + ranges; >> + >> + qcom,tx-amplitude-millivolt = <10>; >> + >> + pcie@0,0 { > > Wrong indentation. > >> + reg = <0x40000 0x0 0x0 0x0 0x0>; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + device_type = "pci"; >> + ranges; >> + >> + qcom,l1-entry-delay-ns = <10>; >> + }; >> + >> + pcie@0,1 { >> + reg = <0x40100 0x0 0x0 0x0 0x0>; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + device_type = "pci"; >> + ranges; >> + >> + qcom,l0s-entry-delay-ns = <10>; >> + }; >> + }; >> + }; >> + }; >> + }; >> >> -- >> 2.34.1 >> >
On 05/08/2024 06:02, Krishna Chaitanya Chundru wrote: > > > On 8/4/2024 2:26 PM, Krzysztof Kozlowski wrote: >> On 03/08/2024 05:22, Krishna chaitanya chundru wrote: >>> Add binding describing the Qualcomm PCIe switch, QPS615, >>> which provides Ethernet MAC integrated to the 3rd downstream port >>> and two downstream PCIe ports. >>> >>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >>> --- >>> .../devicetree/bindings/pci/qcom,qps615.yaml | 191 +++++++++++++++++++++ >>> 1 file changed, 191 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >>> new file mode 100644 >>> index 000000000000..ea0c953ee56f >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >>> @@ -0,0 +1,191 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/pci/qcom,qps615.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Qualcomm QPS615 PCIe switch >>> + >>> +maintainers: >>> + - Krishna chaitanya chundru <quic_krichai@quicinc.com> >>> + >>> +description: | >>> + Qualcomm QPS615 PCIe switch has one upstream and three downstream >>> + ports. The 3rd downstream port has integrated endpoint device of >>> + Ethernet MAC. Other two downstream ports are supposed to connect >>> + to external device. >>> + >>> + The QPS615 PCIe switch can be configured through I2C interface before >>> + PCIe link is established to change FTS, ASPM related entry delays, >>> + tx amplitude etc for better power efficiency and functionality. >>> + >>> +properties: >>> + compatible: >>> + enum: >>> + - pci1179,0623 >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + qcom,qps615-controller: >> >> and now I see that you totally ignored comments. Repeating the same over >> and over is a waste of time. >> >> <form letter> >> This is a friendly reminder during the review process. >> >> It seems my or other reviewer's previous comments were not fully >> addressed. Maybe the feedback got lost between the quotes, maybe you >> just forgot to apply it. Please go back to the previous discussion and >> either implement all requested changes or keep discussing them. >> >> Thank you. >> </form letter> >> >> >> Best regards, >> Krzysztof >> > Hi Krzysztof, > > In patch1 we are trying to add reference of i2c-adapter, you suggested > to use i2c-bus for that. we got comments on the driver code not to use > adapter and instead use i2c client reference. I felt i2c-bus is not > ideal to represent i2c client device so used this name. You did not respond to comment of using i2c-bus, just silently decided to implement other property. Anyway, why i2c-bus is not suitable here? I am quite surprised... Best regards, Krzysztof
On 05/08/2024 06:11, Krishna Chaitanya Chundru wrote: >>> + >>> + qcom,nfts: >>> + $ref: /schemas/types.yaml#/definitions/uint8 >>> + description: >>> + Fast Training Sequence (FTS) is the mechanism that >>> + is used for bit and Symbol lock. >> >> What are the values? Why this is uint8? >> > These represents number of fast training sequence and doesn't have > any units and the maximum value for this is 0xFF only so we used > uint8. >> You described the desired Linux feature or behavior, not the actual >> hardware. The bindings are about the latter, so instead you need to >> rephrase the property and its description to match actual hardware >> capabilities/features/configuration etc. > ack. >> >>> + >>> +allOf: >>> + - $ref: /schemas/pci/pci-bus-common.yaml# >>> + - if: >>> + properties: >>> + compatible: >>> + contains: >>> + const: pci1179,0623 >>> + required: >>> + - compatible >> >> Why do you have entire if? You do not have multiple variants, drop. >> > The child nodes also referencing the qcom,qps615.yaml# node, I tried > to use this way to say "the below properties are for the required for > parent and optional for child". I don't understand how child device can be exactly the same as parent device. How does it look in terms of hardware? Pins and supplies? >>> + then: >>> + required: >>> + - vdd18-supply >>> + - vdd09-supply >>> + - vddc-supply >>> + - vddio1-supply >>> + - vddio2-supply >>> + - vddio18-supply >>> + - qcom,qps615-controller >>> + - reset-gpios >>> + >>> +patternProperties: >>> + "@1?[0-9a-f](,[0-7])?$": >>> + type: object >>> + $ref: qcom,qps615.yaml# >>> + additionalProperties: true >> >> Nope, drop pattern Properties or explain what is this. >> > the child nodes represent the downstream ports of the PCIe > switch which wants to use same properties that is why > I tried to use this pattern properties. Downstream port is not the same as device. Why downstream port has the same supplies? To which pins are they connected? Best regards, Krzysztof
On 8/5/2024 10:44 AM, Krzysztof Kozlowski wrote: > On 05/08/2024 06:11, Krishna Chaitanya Chundru wrote: > > >>>> + >>>> + qcom,nfts: >>>> + $ref: /schemas/types.yaml#/definitions/uint8 >>>> + description: >>>> + Fast Training Sequence (FTS) is the mechanism that >>>> + is used for bit and Symbol lock. >>> >>> What are the values? Why this is uint8? >>> >> These represents number of fast training sequence and doesn't have >> any units and the maximum value for this is 0xFF only so we used >> uint8. >>> You described the desired Linux feature or behavior, not the actual >>> hardware. The bindings are about the latter, so instead you need to >>> rephrase the property and its description to match actual hardware >>> capabilities/features/configuration etc. >> ack. >>> >>>> + >>>> +allOf: >>>> + - $ref: /schemas/pci/pci-bus-common.yaml# >>>> + - if: >>>> + properties: >>>> + compatible: >>>> + contains: >>>> + const: pci1179,0623 >>>> + required: >>>> + - compatible >>> >>> Why do you have entire if? You do not have multiple variants, drop. >>> >> The child nodes also referencing the qcom,qps615.yaml# node, I tried >> to use this way to say "the below properties are for the required for >> parent and optional for child". > > I don't understand how child device can be exactly the same as parent > device. How does it look in terms of hardware? Pins and supplies? > >>>> + then: >>>> + required: >>>> + - vdd18-supply >>>> + - vdd09-supply >>>> + - vddc-supply >>>> + - vddio1-supply >>>> + - vddio2-supply >>>> + - vddio18-supply >>>> + - qcom,qps615-controller >>>> + - reset-gpios >>>> + >>>> +patternProperties: >>>> + "@1?[0-9a-f](,[0-7])?$": >>>> + type: object >>>> + $ref: qcom,qps615.yaml# >>>> + additionalProperties: true >>> >>> Nope, drop pattern Properties or explain what is this. >>> >> the child nodes represent the downstream ports of the PCIe >> switch which wants to use same properties that is why >> I tried to use this pattern properties. > > Downstream port is not the same as device. Why downstream port has the > same supplies? To which pins are they connected? > > Hi Krzysztof, Downstream ports dosen't have pins or supplies to power on. But there are properties like qcom,l0s-entry-delay-ns, qcom,l1-entry-delay-ns, qcom,tx-amplitude-millivolt etc which applicable for child nodes also. Instead of re-declaring the these properties again I tried to use pattern properties. - krishna Chaitanya. > > > Best regards, > Krzysztof >
On 05/08/2024 07:26, Krishna Chaitanya Chundru wrote: > > > On 8/5/2024 10:44 AM, Krzysztof Kozlowski wrote: >> On 05/08/2024 06:11, Krishna Chaitanya Chundru wrote: >> >> >>>>> + >>>>> + qcom,nfts: >>>>> + $ref: /schemas/types.yaml#/definitions/uint8 >>>>> + description: >>>>> + Fast Training Sequence (FTS) is the mechanism that >>>>> + is used for bit and Symbol lock. >>>> >>>> What are the values? Why this is uint8? >>>> >>> These represents number of fast training sequence and doesn't have >>> any units and the maximum value for this is 0xFF only so we used >>> uint8. >>>> You described the desired Linux feature or behavior, not the actual >>>> hardware. The bindings are about the latter, so instead you need to >>>> rephrase the property and its description to match actual hardware >>>> capabilities/features/configuration etc. >>> ack. >>>> >>>>> + >>>>> +allOf: >>>>> + - $ref: /schemas/pci/pci-bus-common.yaml# >>>>> + - if: >>>>> + properties: >>>>> + compatible: >>>>> + contains: >>>>> + const: pci1179,0623 >>>>> + required: >>>>> + - compatible >>>> >>>> Why do you have entire if? You do not have multiple variants, drop. >>>> >>> The child nodes also referencing the qcom,qps615.yaml# node, I tried >>> to use this way to say "the below properties are for the required for >>> parent and optional for child". >> >> I don't understand how child device can be exactly the same as parent >> device. How does it look in terms of hardware? Pins and supplies? >> >>>>> + then: >>>>> + required: >>>>> + - vdd18-supply >>>>> + - vdd09-supply >>>>> + - vddc-supply >>>>> + - vddio1-supply >>>>> + - vddio2-supply >>>>> + - vddio18-supply >>>>> + - qcom,qps615-controller >>>>> + - reset-gpios >>>>> + >>>>> +patternProperties: >>>>> + "@1?[0-9a-f](,[0-7])?$": >>>>> + type: object >>>>> + $ref: qcom,qps615.yaml# >>>>> + additionalProperties: true >>>> >>>> Nope, drop pattern Properties or explain what is this. >>>> >>> the child nodes represent the downstream ports of the PCIe >>> switch which wants to use same properties that is why >>> I tried to use this pattern properties. >> >> Downstream port is not the same as device. Why downstream port has the >> same supplies? To which pins are they connected? >> >> > Hi Krzysztof, > > Downstream ports dosen't have pins or supplies to power on. > > But there are properties like qcom,l0s-entry-delay-ns, > qcom,l1-entry-delay-ns, qcom,tx-amplitude-millivolt etc which > applicable for child nodes also. Instead of re-declaring the > these properties again I tried to use pattern properties. You could use $defs for them, but I don't understand how does these properties apply for both main device and ports. It seems you are writing binding to match some driver behavior. Let's start from basics - describe the hardware. Best regards, Krzysztof
On 8/5/2024 10:42 AM, Krzysztof Kozlowski wrote: > On 05/08/2024 06:02, Krishna Chaitanya Chundru wrote: >> >> >> On 8/4/2024 2:26 PM, Krzysztof Kozlowski wrote: >>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote: >>>> Add binding describing the Qualcomm PCIe switch, QPS615, >>>> which provides Ethernet MAC integrated to the 3rd downstream port >>>> and two downstream PCIe ports. >>>> >>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >>>> --- >>>> .../devicetree/bindings/pci/qcom,qps615.yaml | 191 +++++++++++++++++++++ >>>> 1 file changed, 191 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >>>> new file mode 100644 >>>> index 000000000000..ea0c953ee56f >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >>>> @@ -0,0 +1,191 @@ >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>> +%YAML 1.2 >>>> +--- >>>> +$id: http://devicetree.org/schemas/pci/qcom,qps615.yaml# >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>> + >>>> +title: Qualcomm QPS615 PCIe switch >>>> + >>>> +maintainers: >>>> + - Krishna chaitanya chundru <quic_krichai@quicinc.com> >>>> + >>>> +description: | >>>> + Qualcomm QPS615 PCIe switch has one upstream and three downstream >>>> + ports. The 3rd downstream port has integrated endpoint device of >>>> + Ethernet MAC. Other two downstream ports are supposed to connect >>>> + to external device. >>>> + >>>> + The QPS615 PCIe switch can be configured through I2C interface before >>>> + PCIe link is established to change FTS, ASPM related entry delays, >>>> + tx amplitude etc for better power efficiency and functionality. >>>> + >>>> +properties: >>>> + compatible: >>>> + enum: >>>> + - pci1179,0623 >>>> + >>>> + reg: >>>> + maxItems: 1 >>>> + >>>> + qcom,qps615-controller: >>> >>> and now I see that you totally ignored comments. Repeating the same over >>> and over is a waste of time. >>> >>> <form letter> >>> This is a friendly reminder during the review process. >>> >>> It seems my or other reviewer's previous comments were not fully >>> addressed. Maybe the feedback got lost between the quotes, maybe you >>> just forgot to apply it. Please go back to the previous discussion and >>> either implement all requested changes or keep discussing them. >>> >>> Thank you. >>> </form letter> >>> >>> >>> Best regards, >>> Krzysztof >>> >> Hi Krzysztof, >> >> In patch1 we are trying to add reference of i2c-adapter, you suggested >> to use i2c-bus for that. we got comments on the driver code not to use >> adapter and instead use i2c client reference. I felt i2c-bus is not >> ideal to represent i2c client device so used this name. > > You did not respond to comment of using i2c-bus, just silently decided > to implement other property. > I should have replied to v1 why we are not using this suggested way, next time onwards I will fallow that. > Anyway, why i2c-bus is not suitable here? I am quite surprised... > It was suggested by bjorn andresson in the offline review, I will check this and get back. - Krishna Chaitanya. > > > Best regards, > Krzysztof >
On 8/5/2024 10:58 AM, Krzysztof Kozlowski wrote: > On 05/08/2024 07:26, Krishna Chaitanya Chundru wrote: >> >> >> On 8/5/2024 10:44 AM, Krzysztof Kozlowski wrote: >>> On 05/08/2024 06:11, Krishna Chaitanya Chundru wrote: >>> >>> >>>>>> + >>>>>> + qcom,nfts: >>>>>> + $ref: /schemas/types.yaml#/definitions/uint8 >>>>>> + description: >>>>>> + Fast Training Sequence (FTS) is the mechanism that >>>>>> + is used for bit and Symbol lock. >>>>> >>>>> What are the values? Why this is uint8? >>>>> >>>> These represents number of fast training sequence and doesn't have >>>> any units and the maximum value for this is 0xFF only so we used >>>> uint8. >>>>> You described the desired Linux feature or behavior, not the actual >>>>> hardware. The bindings are about the latter, so instead you need to >>>>> rephrase the property and its description to match actual hardware >>>>> capabilities/features/configuration etc. >>>> ack. >>>>> >>>>>> + >>>>>> +allOf: >>>>>> + - $ref: /schemas/pci/pci-bus-common.yaml# >>>>>> + - if: >>>>>> + properties: >>>>>> + compatible: >>>>>> + contains: >>>>>> + const: pci1179,0623 >>>>>> + required: >>>>>> + - compatible >>>>> >>>>> Why do you have entire if? You do not have multiple variants, drop. >>>>> >>>> The child nodes also referencing the qcom,qps615.yaml# node, I tried >>>> to use this way to say "the below properties are for the required for >>>> parent and optional for child". >>> >>> I don't understand how child device can be exactly the same as parent >>> device. How does it look in terms of hardware? Pins and supplies? >>> >>>>>> + then: >>>>>> + required: >>>>>> + - vdd18-supply >>>>>> + - vdd09-supply >>>>>> + - vddc-supply >>>>>> + - vddio1-supply >>>>>> + - vddio2-supply >>>>>> + - vddio18-supply >>>>>> + - qcom,qps615-controller >>>>>> + - reset-gpios >>>>>> + >>>>>> +patternProperties: >>>>>> + "@1?[0-9a-f](,[0-7])?$": >>>>>> + type: object >>>>>> + $ref: qcom,qps615.yaml# >>>>>> + additionalProperties: true >>>>> >>>>> Nope, drop pattern Properties or explain what is this. >>>>> >>>> the child nodes represent the downstream ports of the PCIe >>>> switch which wants to use same properties that is why >>>> I tried to use this pattern properties. >>> >>> Downstream port is not the same as device. Why downstream port has the >>> same supplies? To which pins are they connected? >>> >>> >> Hi Krzysztof, >> >> Downstream ports dosen't have pins or supplies to power on. >> >> But there are properties like qcom,l0s-entry-delay-ns, >> qcom,l1-entry-delay-ns, qcom,tx-amplitude-millivolt etc which >> applicable for child nodes also. Instead of re-declaring the >> these properties again I tried to use pattern properties. > > You could use $defs for them, but I don't understand how does these > properties apply for both main device and ports. It seems you are > writing binding to match some driver behavior. Let's start from basics - > describe the hardware. > Hi Krzysztof, QPS615 has a 3 downstream ports and 1 upstream port as described below diagram. For this entire switch there are some supplies which we described in the dt-binding (vdd18-supply, vdd09-supply etc) and one GPIO which controls reset of the switch (reset-gpio). The switch hardware can configure the individual ports DSP0, DSP1, DSP2, upstream port and also one integrated ethernet endpoint which is connected to DSP2(I didn't mentioned in the diagram) through I2C. The properties other than supplies,i2c client, reset gpio which are added will be applicable for all the ports.
On 05/08/2024 07:57, Krishna Chaitanya Chundru wrote: >> > Hi Krzysztof, > > QPS615 has a 3 downstream ports and 1 upstream port as described below > diagram. > For this entire switch there are some supplies which we described in the > dt-binding (vdd18-supply, vdd09-supply etc) and one GPIO which controls > reset of the switch (reset-gpio). The switch hardware can configure the > individual ports DSP0, DSP1, DSP2, upstream port and also one integrated > ethernet endpoint which is connected to DSP2(I didn't mentioned in the > diagram) through I2C. > > The properties other than supplies,i2c client, reset gpio which > are added will be applicable for all the ports. > _______________________________________________________________ > | |i2c| QPS615 |Supplies||Resx gpio | > | |___| _________________ |________||__________| > | ________________| Upstream port |_____________ | > | | |_______________| | | > | | | | | > | | | | | > | ____|_____ ____|_____ ___|____ | > | |DSP 0 | | DSP 1 | | DSP 2| | > | |________| |________| |______| | > |_____________________________________________________________| > I don't get why then properties should apply to main device node. Best regards, Krzysztof
On Mon, Aug 05, 2024 at 07:12:34AM +0200, Krzysztof Kozlowski wrote: > On 05/08/2024 06:02, Krishna Chaitanya Chundru wrote: > > > > > > On 8/4/2024 2:26 PM, Krzysztof Kozlowski wrote: > >> On 03/08/2024 05:22, Krishna chaitanya chundru wrote: > >>> Add binding describing the Qualcomm PCIe switch, QPS615, > >>> which provides Ethernet MAC integrated to the 3rd downstream port > >>> and two downstream PCIe ports. > >>> > >>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > >>> --- > >>> .../devicetree/bindings/pci/qcom,qps615.yaml | 191 +++++++++++++++++++++ > >>> 1 file changed, 191 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > >>> new file mode 100644 > >>> index 000000000000..ea0c953ee56f > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > >>> @@ -0,0 +1,191 @@ > >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >>> +%YAML 1.2 > >>> +--- > >>> +$id: http://devicetree.org/schemas/pci/qcom,qps615.yaml# > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>> + > >>> +title: Qualcomm QPS615 PCIe switch > >>> + > >>> +maintainers: > >>> + - Krishna chaitanya chundru <quic_krichai@quicinc.com> > >>> + > >>> +description: | > >>> + Qualcomm QPS615 PCIe switch has one upstream and three downstream > >>> + ports. The 3rd downstream port has integrated endpoint device of > >>> + Ethernet MAC. Other two downstream ports are supposed to connect > >>> + to external device. > >>> + > >>> + The QPS615 PCIe switch can be configured through I2C interface before > >>> + PCIe link is established to change FTS, ASPM related entry delays, > >>> + tx amplitude etc for better power efficiency and functionality. > >>> + > >>> +properties: > >>> + compatible: > >>> + enum: > >>> + - pci1179,0623 > >>> + > >>> + reg: > >>> + maxItems: 1 > >>> + > >>> + qcom,qps615-controller: > >> > >> and now I see that you totally ignored comments. Repeating the same over > >> and over is a waste of time. > >> > >> <form letter> > >> This is a friendly reminder during the review process. > >> > >> It seems my or other reviewer's previous comments were not fully > >> addressed. Maybe the feedback got lost between the quotes, maybe you > >> just forgot to apply it. Please go back to the previous discussion and > >> either implement all requested changes or keep discussing them. > >> > >> Thank you. Well, thank you for the rant. Very helpful indeed. > >> </form letter> > >> > >> > >> Best regards, > >> Krzysztof > >> > > Hi Krzysztof, > > > > In patch1 we are trying to add reference of i2c-adapter, you suggested > > to use i2c-bus for that. we got comments on the driver code not to use > > adapter and instead use i2c client reference. I felt i2c-bus is not > > ideal to represent i2c client device so used this name. > > You did not respond to comment of using i2c-bus, just silently decided > to implement other property. > I guess you totally ignored my comment when you reviewed the previous version, where I asked him to represent the device on said bus. > Anyway, why i2c-bus is not suitable here? I am quite surprised... > I was not aware that i2c-bus was an acceptable solution, sorry for my bad suggestion and guidance here. Regards, Bjorn > > > Best regards, > Krzysztof > >
On 05/08/2024 18:39, Bjorn Andersson wrote: >>> >>> In patch1 we are trying to add reference of i2c-adapter, you suggested >>> to use i2c-bus for that. we got comments on the driver code not to use >>> adapter and instead use i2c client reference. I felt i2c-bus is not >>> ideal to represent i2c client device so used this name. >> >> You did not respond to comment of using i2c-bus, just silently decided >> to implement other property. >> > > I guess you totally ignored my comment when you reviewed the previous > version, where I asked him to represent the device on said bus. Hm, Rob suggested i2c-bus, you as well: <<I'd prefer you call it "i2c-adapter" or perhaps "i2c-bus", because it's not "the switch controller".>> and there was no response to any of these comments. > >> Anyway, why i2c-bus is not suitable here? I am quite surprised... >> > > I was not aware that i2c-bus was an acceptable solution, sorry for my > bad suggestion and guidance here. I think you suggested i2c-bus as well, but regardless what did you agree internally, response to Rob was expected. Best regards, Krzysztof
On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote: > On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote: > > On 03/08/2024 05:22, Krishna chaitanya chundru wrote: > > > diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml [..] > > > + qps615,axi-clk-freq-hz: > > > + description: > > > + AXI clock which internal bus of the switch. > > > > No need, use CCF. > > > ack This is a clock that's internal to the QPS615, so there's no clock controller involved and hence I don't think CCF is applicable. Regards, Bjorn
On 05/08/2024 19:07, Bjorn Andersson wrote: > On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote: >> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote: >>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote: >>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > [..] >>>> + qps615,axi-clk-freq-hz: >>>> + description: >>>> + AXI clock which internal bus of the switch. >>> >>> No need, use CCF. >>> >> ack > > This is a clock that's internal to the QPS615, so there's no clock > controller involved and hence I don't think CCF is applicable. AXI does not sound that internal. DT rarely needs to specify internal clock rates. What if you want to define rates for 20 clocks? Even clock-frequency is deprecated, so why this would be allowed? bus-frequency is allowed for buses, but that's not the case here, I guess? Best regards, Krzysztof
On Mon, Aug 05, 2024 at 07:18:04PM +0200, Krzysztof Kozlowski wrote: > On 05/08/2024 19:07, Bjorn Andersson wrote: > > On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote: > >> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote: > >>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote: > >>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > > [..] > >>>> + qps615,axi-clk-freq-hz: > >>>> + description: > >>>> + AXI clock which internal bus of the switch. > >>> > >>> No need, use CCF. > >>> > >> ack > > > > This is a clock that's internal to the QPS615, so there's no clock > > controller involved and hence I don't think CCF is applicable. > > AXI does not sound that internal. Well, AXI is applicable to whatever entity that implements it. We mostly seen it in ARM SoCs (host), but in this case the PCIe switch also has a microcontroller /processor of some sort, so AXI is indeed relevant for it. The naming actually comes from the switch's i2c register name that is being configured in the driver based on this property value. > DT rarely needs to specify internal > clock rates. What if you want to define rates for 20 clocks? Even > clock-frequency is deprecated, so why this would be allowed? > bus-frequency is allowed for buses, but that's not the case here, I guess? > This clock frequency is for the switch's internal AXI bus that runs at default 200MHz. And this property is used to specify a frequency that is configured over the i2c interface so that the switch's AXI bus can operate in a low frequency there by reducing the power consumption of the switch. It is not strictly needed for the switch operation, but for power optimization. So this property can also be dropped for the initial submission and added later if you prefer. - Mani
On 08/08/2024 14:01, Manivannan Sadhasivam wrote: > On Mon, Aug 05, 2024 at 07:18:04PM +0200, Krzysztof Kozlowski wrote: >> On 05/08/2024 19:07, Bjorn Andersson wrote: >>> On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote: >>>> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote: >>>>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote: >>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >>> [..] >>>>>> + qps615,axi-clk-freq-hz: >>>>>> + description: >>>>>> + AXI clock which internal bus of the switch. >>>>> >>>>> No need, use CCF. >>>>> >>>> ack >>> >>> This is a clock that's internal to the QPS615, so there's no clock >>> controller involved and hence I don't think CCF is applicable. >> >> AXI does not sound that internal. > > Well, AXI is applicable to whatever entity that implements it. We mostly seen it > in ARM SoCs (host), but in this case the PCIe switch also has a microcontroller > /processor of some sort, so AXI is indeed relevant for it. The naming actually > comes from the switch's i2c register name that is being configured in the driver > based on this property value. > >> DT rarely needs to specify internal >> clock rates. What if you want to define rates for 20 clocks? Even >> clock-frequency is deprecated, so why this would be allowed? >> bus-frequency is allowed for buses, but that's not the case here, I guess? >> > > This clock frequency is for the switch's internal AXI bus that runs at default > 200MHz. And this property is used to specify a frequency that is configured over > the i2c interface so that the switch's AXI bus can operate in a low frequency > there by reducing the power consumption of the switch. > > It is not strictly needed for the switch operation, but for power optimization. > So this property can also be dropped for the initial submission and added later > if you prefer. So if the clock rate can change, why this is static in DTB? Or why this is configurable per-board? There is a reason why clock-frequency property is not welcomed and you are re-implementing it. Best regards, Krzysztof
On Thu, Aug 08, 2024 at 02:13:01PM +0200, Krzysztof Kozlowski wrote: > On 08/08/2024 14:01, Manivannan Sadhasivam wrote: > > On Mon, Aug 05, 2024 at 07:18:04PM +0200, Krzysztof Kozlowski wrote: > >> On 05/08/2024 19:07, Bjorn Andersson wrote: > >>> On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote: > >>>> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote: > >>>>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote: > >>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > >>> [..] > >>>>>> + qps615,axi-clk-freq-hz: > >>>>>> + description: > >>>>>> + AXI clock which internal bus of the switch. > >>>>> > >>>>> No need, use CCF. > >>>>> > >>>> ack > >>> > >>> This is a clock that's internal to the QPS615, so there's no clock > >>> controller involved and hence I don't think CCF is applicable. > >> > >> AXI does not sound that internal. > > > > Well, AXI is applicable to whatever entity that implements it. We mostly seen it > > in ARM SoCs (host), but in this case the PCIe switch also has a microcontroller > > /processor of some sort, so AXI is indeed relevant for it. The naming actually > > comes from the switch's i2c register name that is being configured in the driver > > based on this property value. > > > >> DT rarely needs to specify internal > >> clock rates. What if you want to define rates for 20 clocks? Even > >> clock-frequency is deprecated, so why this would be allowed? > >> bus-frequency is allowed for buses, but that's not the case here, I guess? > >> > > > > This clock frequency is for the switch's internal AXI bus that runs at default > > 200MHz. And this property is used to specify a frequency that is configured over > > the i2c interface so that the switch's AXI bus can operate in a low frequency > > there by reducing the power consumption of the switch. > > > > It is not strictly needed for the switch operation, but for power optimization. > > So this property can also be dropped for the initial submission and added later > > if you prefer. > > So if the clock rate can change, why this is static in DTB? Or why this > is configurable per-board? > Because, board manufacturers can change the frequency depending on the switch configuration (enablement of DSP's etc...) > There is a reason why clock-frequency property is not welcomed and you > are re-implementing it. > Hmm, I'm not aware that 'clock-frequency' is not encouraged these days. So you are suggesting to change the rate in the driver itself based on the switch configuration? If so, what difference does it make? And no more *-freq properties are allowed? - Mani
On 08/08/2024 14:41, Manivannan Sadhasivam wrote: > On Thu, Aug 08, 2024 at 02:13:01PM +0200, Krzysztof Kozlowski wrote: >> On 08/08/2024 14:01, Manivannan Sadhasivam wrote: >>> On Mon, Aug 05, 2024 at 07:18:04PM +0200, Krzysztof Kozlowski wrote: >>>> On 05/08/2024 19:07, Bjorn Andersson wrote: >>>>> On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote: >>>>>> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote: >>>>>>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote: >>>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >>>>> [..] >>>>>>>> + qps615,axi-clk-freq-hz: >>>>>>>> + description: >>>>>>>> + AXI clock which internal bus of the switch. >>>>>>> >>>>>>> No need, use CCF. >>>>>>> >>>>>> ack >>>>> >>>>> This is a clock that's internal to the QPS615, so there's no clock >>>>> controller involved and hence I don't think CCF is applicable. >>>> >>>> AXI does not sound that internal. >>> >>> Well, AXI is applicable to whatever entity that implements it. We mostly seen it >>> in ARM SoCs (host), but in this case the PCIe switch also has a microcontroller >>> /processor of some sort, so AXI is indeed relevant for it. The naming actually >>> comes from the switch's i2c register name that is being configured in the driver >>> based on this property value. >>> >>>> DT rarely needs to specify internal >>>> clock rates. What if you want to define rates for 20 clocks? Even >>>> clock-frequency is deprecated, so why this would be allowed? >>>> bus-frequency is allowed for buses, but that's not the case here, I guess? >>>> >>> >>> This clock frequency is for the switch's internal AXI bus that runs at default >>> 200MHz. And this property is used to specify a frequency that is configured over >>> the i2c interface so that the switch's AXI bus can operate in a low frequency >>> there by reducing the power consumption of the switch. >>> >>> It is not strictly needed for the switch operation, but for power optimization. >>> So this property can also be dropped for the initial submission and added later >>> if you prefer. >> >> So if the clock rate can change, why this is static in DTB? Or why this >> is configurable per-board? >> > > Because, board manufacturers can change the frequency depending on the switch > configuration (enablement of DSP's etc...) > >> There is a reason why clock-frequency property is not welcomed and you >> are re-implementing it. >> > > Hmm, I'm not aware that 'clock-frequency' is not encouraged these days. So you > are suggesting to change the rate in the driver itself based on the switch > configuration? If so, what difference does it make? Based on the switch, other clocks, votes etc. whatever is reasonable there. In most cases, not sure if this one here as well, devices can operate on different clock frequencies thus specifying fixed frequency in the DTS is simplification and lack of flexibility. It is chosen by people only because it is easier for them but then they come back with ABI issues when it turns out they need to switch to some dynamic control. > > And no more *-freq properties are allowed? bus-frequency is allowed for busses. Best regards, Krzysztof
On Thu, Aug 08, 2024 at 03:06:28PM +0200, Krzysztof Kozlowski wrote: > On 08/08/2024 14:41, Manivannan Sadhasivam wrote: > > On Thu, Aug 08, 2024 at 02:13:01PM +0200, Krzysztof Kozlowski wrote: > >> On 08/08/2024 14:01, Manivannan Sadhasivam wrote: > >>> On Mon, Aug 05, 2024 at 07:18:04PM +0200, Krzysztof Kozlowski wrote: > >>>> On 05/08/2024 19:07, Bjorn Andersson wrote: > >>>>> On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote: > >>>>>> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote: > >>>>>>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote: > >>>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > >>>>> [..] > >>>>>>>> + qps615,axi-clk-freq-hz: > >>>>>>>> + description: > >>>>>>>> + AXI clock which internal bus of the switch. > >>>>>>> > >>>>>>> No need, use CCF. > >>>>>>> > >>>>>> ack > >>>>> > >>>>> This is a clock that's internal to the QPS615, so there's no clock > >>>>> controller involved and hence I don't think CCF is applicable. > >>>> > >>>> AXI does not sound that internal. > >>> > >>> Well, AXI is applicable to whatever entity that implements it. We mostly seen it > >>> in ARM SoCs (host), but in this case the PCIe switch also has a microcontroller > >>> /processor of some sort, so AXI is indeed relevant for it. The naming actually > >>> comes from the switch's i2c register name that is being configured in the driver > >>> based on this property value. > >>> > >>>> DT rarely needs to specify internal > >>>> clock rates. What if you want to define rates for 20 clocks? Even > >>>> clock-frequency is deprecated, so why this would be allowed? > >>>> bus-frequency is allowed for buses, but that's not the case here, I guess? > >>>> > >>> > >>> This clock frequency is for the switch's internal AXI bus that runs at default > >>> 200MHz. And this property is used to specify a frequency that is configured over > >>> the i2c interface so that the switch's AXI bus can operate in a low frequency > >>> there by reducing the power consumption of the switch. > >>> > >>> It is not strictly needed for the switch operation, but for power optimization. > >>> So this property can also be dropped for the initial submission and added later > >>> if you prefer. > >> > >> So if the clock rate can change, why this is static in DTB? Or why this > >> is configurable per-board? > >> > > > > Because, board manufacturers can change the frequency depending on the switch > > configuration (enablement of DSP's etc...) > > > >> There is a reason why clock-frequency property is not welcomed and you > >> are re-implementing it. > >> > > > > Hmm, I'm not aware that 'clock-frequency' is not encouraged these days. So you > > are suggesting to change the rate in the driver itself based on the switch > > configuration? If so, what difference does it make? > > Based on the switch, other clocks, votes etc. whatever is reasonable > there. In most cases, not sure if this one here as well, devices can > operate on different clock frequencies thus specifying fixed frequency > in the DTS is simplification and lack of flexibility. It is chosen by > people only because it is easier for them but then they come back with > ABI issues when it turns out they need to switch to some dynamic control. > Atleast in this case, the requirement is to just set the frequency based on switch configuration and not change it dynamically. Krishna, is it possible to set the freq in driver by detecting the switch configuration? I believe the freq is based on number of DSPs enabled? > > > > And no more *-freq properties are allowed? > > bus-frequency is allowed for busses. > Okay. - Mani
On Thu, Aug 08, 2024 at 03:06:28PM +0200, Krzysztof Kozlowski wrote: > On 08/08/2024 14:41, Manivannan Sadhasivam wrote: > > On Thu, Aug 08, 2024 at 02:13:01PM +0200, Krzysztof Kozlowski wrote: > >> On 08/08/2024 14:01, Manivannan Sadhasivam wrote: > >>> On Mon, Aug 05, 2024 at 07:18:04PM +0200, Krzysztof Kozlowski wrote: > >>>> On 05/08/2024 19:07, Bjorn Andersson wrote: > >>>>> On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote: > >>>>>> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote: > >>>>>>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote: > >>>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > >>>>> [..] > >>>>>>>> + qps615,axi-clk-freq-hz: > >>>>>>>> + description: > >>>>>>>> + AXI clock which internal bus of the switch. > >>>>>>> > >>>>>>> No need, use CCF. > >>>>>>> > >>>>>> ack > >>>>> > >>>>> This is a clock that's internal to the QPS615, so there's no clock > >>>>> controller involved and hence I don't think CCF is applicable. > >>>> > >>>> AXI does not sound that internal. > >>> > >>> Well, AXI is applicable to whatever entity that implements it. We mostly seen it > >>> in ARM SoCs (host), but in this case the PCIe switch also has a microcontroller > >>> /processor of some sort, so AXI is indeed relevant for it. The naming actually > >>> comes from the switch's i2c register name that is being configured in the driver > >>> based on this property value. > >>> > >>>> DT rarely needs to specify internal > >>>> clock rates. What if you want to define rates for 20 clocks? Even > >>>> clock-frequency is deprecated, so why this would be allowed? > >>>> bus-frequency is allowed for buses, but that's not the case here, I guess? > >>>> > >>> > >>> This clock frequency is for the switch's internal AXI bus that runs at default > >>> 200MHz. And this property is used to specify a frequency that is configured over > >>> the i2c interface so that the switch's AXI bus can operate in a low frequency > >>> there by reducing the power consumption of the switch. > >>> > >>> It is not strictly needed for the switch operation, but for power optimization. > >>> So this property can also be dropped for the initial submission and added later > >>> if you prefer. > >> > >> So if the clock rate can change, why this is static in DTB? Or why this > >> is configurable per-board? > >> > > > > Because, board manufacturers can change the frequency depending on the switch > > configuration (enablement of DSP's etc...) > > > >> There is a reason why clock-frequency property is not welcomed and you > >> are re-implementing it. > >> > > > > Hmm, I'm not aware that 'clock-frequency' is not encouraged these days. So you > > are suggesting to change the rate in the driver itself based on the switch > > configuration? If so, what difference does it make? > > Based on the switch, other clocks, votes etc. whatever is reasonable > there. In most cases, not sure if this one here as well, devices can > operate on different clock frequencies thus specifying fixed frequency > in the DTS is simplification and lack of flexibility. It is chosen by > people only because it is easier for them but then they come back with > ABI issues when it turns out they need to switch to some dynamic control. > Atleast for this device, this frequency is going to be static. Because, the device itself cannot change the frequency, only the host driver can. That too is only possible before enumerating the device. So there is no way the frequency is going to change dynamically. - Mani
On Mon, Aug 05, 2024 at 04:43:47PM +0200, Krzysztof Kozlowski wrote: > On 05/08/2024 07:57, Krishna Chaitanya Chundru wrote: > >> > > Hi Krzysztof, > > > > QPS615 has a 3 downstream ports and 1 upstream port as described below > > diagram. > > For this entire switch there are some supplies which we described in the > > dt-binding (vdd18-supply, vdd09-supply etc) and one GPIO which controls > > reset of the switch (reset-gpio). The switch hardware can configure the > > individual ports DSP0, DSP1, DSP2, upstream port and also one integrated > > ethernet endpoint which is connected to DSP2(I didn't mentioned in the > > diagram) through I2C. > > > > The properties other than supplies,i2c client, reset gpio which > > are added will be applicable for all the ports. > > _______________________________________________________________ > > | |i2c| QPS615 |Supplies||Resx gpio | > > | |___| _________________ |________||__________| > > | ________________| Upstream port |_____________ | > > | | |_______________| | | > > | | | | | > > | | | | | > > | ____|_____ ____|_____ ___|____ | > > | |DSP 0 | | DSP 1 | | DSP 2| | > > | |________| |________| |______| | > > |_____________________________________________________________| > > > > I don't get why then properties should apply to main device node. > The problem here is, we cannot differentiate between main device node and the upstream node. Typically the differentiation is not needed because no one cared about configuring the upstream port. But this PCIe switch is special (as like most of the Qcom peripherals). I agree that if we don't differentiate then it also implies that all main node properties are applicable to upstream port and vice versa. But AFAIK, upstream port is often considered as the _device_ itself as it shares the same bus number. - Mani
On 22/08/2024 16:16, Manivannan Sadhasivam wrote: > On Mon, Aug 05, 2024 at 04:43:47PM +0200, Krzysztof Kozlowski wrote: >> On 05/08/2024 07:57, Krishna Chaitanya Chundru wrote: >>>> >>> Hi Krzysztof, >>> >>> QPS615 has a 3 downstream ports and 1 upstream port as described below >>> diagram. >>> For this entire switch there are some supplies which we described in the >>> dt-binding (vdd18-supply, vdd09-supply etc) and one GPIO which controls >>> reset of the switch (reset-gpio). The switch hardware can configure the >>> individual ports DSP0, DSP1, DSP2, upstream port and also one integrated >>> ethernet endpoint which is connected to DSP2(I didn't mentioned in the >>> diagram) through I2C. >>> >>> The properties other than supplies,i2c client, reset gpio which >>> are added will be applicable for all the ports. >>> _______________________________________________________________ >>> | |i2c| QPS615 |Supplies||Resx gpio | >>> | |___| _________________ |________||__________| >>> | ________________| Upstream port |_____________ | >>> | | |_______________| | | >>> | | | | | >>> | | | | | >>> | ____|_____ ____|_____ ___|____ | >>> | |DSP 0 | | DSP 1 | | DSP 2| | >>> | |________| |________| |______| | >>> |_____________________________________________________________| >>> >> >> I don't get why then properties should apply to main device node. >> > > The problem here is, we cannot differentiate between main device node and the > upstream node. Typically the differentiation is not needed because no one cared > about configuring the upstream port. But this PCIe switch is special (as like > most of the Qcom peripherals). > > I agree that if we don't differentiate then it also implies that all main node > properties are applicable to upstream port and vice versa. But AFAIK, upstream > port is often considered as the _device_ itself as it shares the same bus > number. Well, above diagram shows supplies being part of the entire device, not each port. That's confusing. Based on diagram, downstream ports do not have any supplies... and what exactly do they supply? Let's look at vdd18 and vdd09 which sound main supplies of the entire device. In context of port: what exactly do they power? Which part of the port? Best regards, Krzysztof
On 22/08/2024 16:09, Manivannan Sadhasivam wrote: > On Thu, Aug 08, 2024 at 03:06:28PM +0200, Krzysztof Kozlowski wrote: >> On 08/08/2024 14:41, Manivannan Sadhasivam wrote: >>> On Thu, Aug 08, 2024 at 02:13:01PM +0200, Krzysztof Kozlowski wrote: >>>> On 08/08/2024 14:01, Manivannan Sadhasivam wrote: >>>>> On Mon, Aug 05, 2024 at 07:18:04PM +0200, Krzysztof Kozlowski wrote: >>>>>> On 05/08/2024 19:07, Bjorn Andersson wrote: >>>>>>> On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote: >>>>>>>> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote: >>>>>>>>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote: >>>>>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >>>>>>> [..] >>>>>>>>>> + qps615,axi-clk-freq-hz: >>>>>>>>>> + description: >>>>>>>>>> + AXI clock which internal bus of the switch. >>>>>>>>> >>>>>>>>> No need, use CCF. >>>>>>>>> >>>>>>>> ack >>>>>>> >>>>>>> This is a clock that's internal to the QPS615, so there's no clock >>>>>>> controller involved and hence I don't think CCF is applicable. >>>>>> >>>>>> AXI does not sound that internal. >>>>> >>>>> Well, AXI is applicable to whatever entity that implements it. We mostly seen it >>>>> in ARM SoCs (host), but in this case the PCIe switch also has a microcontroller >>>>> /processor of some sort, so AXI is indeed relevant for it. The naming actually >>>>> comes from the switch's i2c register name that is being configured in the driver >>>>> based on this property value. >>>>> >>>>>> DT rarely needs to specify internal >>>>>> clock rates. What if you want to define rates for 20 clocks? Even >>>>>> clock-frequency is deprecated, so why this would be allowed? >>>>>> bus-frequency is allowed for buses, but that's not the case here, I guess? >>>>>> >>>>> >>>>> This clock frequency is for the switch's internal AXI bus that runs at default >>>>> 200MHz. And this property is used to specify a frequency that is configured over >>>>> the i2c interface so that the switch's AXI bus can operate in a low frequency >>>>> there by reducing the power consumption of the switch. >>>>> >>>>> It is not strictly needed for the switch operation, but for power optimization. >>>>> So this property can also be dropped for the initial submission and added later >>>>> if you prefer. >>>> >>>> So if the clock rate can change, why this is static in DTB? Or why this >>>> is configurable per-board? >>>> >>> >>> Because, board manufacturers can change the frequency depending on the switch >>> configuration (enablement of DSP's etc...) >>> >>>> There is a reason why clock-frequency property is not welcomed and you >>>> are re-implementing it. >>>> >>> >>> Hmm, I'm not aware that 'clock-frequency' is not encouraged these days. So you >>> are suggesting to change the rate in the driver itself based on the switch >>> configuration? If so, what difference does it make? >> >> Based on the switch, other clocks, votes etc. whatever is reasonable >> there. In most cases, not sure if this one here as well, devices can >> operate on different clock frequencies thus specifying fixed frequency >> in the DTS is simplification and lack of flexibility. It is chosen by >> people only because it is easier for them but then they come back with >> ABI issues when it turns out they need to switch to some dynamic control. >> > > Atleast for this device, this frequency is going to be static. Because, the > device itself cannot change the frequency, only the host driver can. That too is > only possible before enumerating the device. So there is no way the frequency is > going to change dynamically. We have assigned-clocks properties for this... but there are no clock inputs here, so maybe it is not applicable? What generates this internal AXI clock? Does it have internal oscillator? So many questions and nothing in the property description helps to understand this. Best regards, Krzysztof
On Fri, Aug 23, 2024 at 11:06:28AM +0200, Krzysztof Kozlowski wrote: > On 22/08/2024 16:09, Manivannan Sadhasivam wrote: > > On Thu, Aug 08, 2024 at 03:06:28PM +0200, Krzysztof Kozlowski wrote: > >> On 08/08/2024 14:41, Manivannan Sadhasivam wrote: > >>> On Thu, Aug 08, 2024 at 02:13:01PM +0200, Krzysztof Kozlowski wrote: > >>>> On 08/08/2024 14:01, Manivannan Sadhasivam wrote: > >>>>> On Mon, Aug 05, 2024 at 07:18:04PM +0200, Krzysztof Kozlowski wrote: > >>>>>> On 05/08/2024 19:07, Bjorn Andersson wrote: > >>>>>>> On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote: > >>>>>>>> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote: > >>>>>>>>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote: > >>>>>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > >>>>>>> [..] > >>>>>>>>>> + qps615,axi-clk-freq-hz: > >>>>>>>>>> + description: > >>>>>>>>>> + AXI clock which internal bus of the switch. > >>>>>>>>> > >>>>>>>>> No need, use CCF. > >>>>>>>>> > >>>>>>>> ack > >>>>>>> > >>>>>>> This is a clock that's internal to the QPS615, so there's no clock > >>>>>>> controller involved and hence I don't think CCF is applicable. > >>>>>> > >>>>>> AXI does not sound that internal. > >>>>> > >>>>> Well, AXI is applicable to whatever entity that implements it. We mostly seen it > >>>>> in ARM SoCs (host), but in this case the PCIe switch also has a microcontroller > >>>>> /processor of some sort, so AXI is indeed relevant for it. The naming actually > >>>>> comes from the switch's i2c register name that is being configured in the driver > >>>>> based on this property value. > >>>>> > >>>>>> DT rarely needs to specify internal > >>>>>> clock rates. What if you want to define rates for 20 clocks? Even > >>>>>> clock-frequency is deprecated, so why this would be allowed? > >>>>>> bus-frequency is allowed for buses, but that's not the case here, I guess? > >>>>>> > >>>>> > >>>>> This clock frequency is for the switch's internal AXI bus that runs at default > >>>>> 200MHz. And this property is used to specify a frequency that is configured over > >>>>> the i2c interface so that the switch's AXI bus can operate in a low frequency > >>>>> there by reducing the power consumption of the switch. > >>>>> > >>>>> It is not strictly needed for the switch operation, but for power optimization. > >>>>> So this property can also be dropped for the initial submission and added later > >>>>> if you prefer. > >>>> > >>>> So if the clock rate can change, why this is static in DTB? Or why this > >>>> is configurable per-board? > >>>> > >>> > >>> Because, board manufacturers can change the frequency depending on the switch > >>> configuration (enablement of DSP's etc...) > >>> > >>>> There is a reason why clock-frequency property is not welcomed and you > >>>> are re-implementing it. > >>>> > >>> > >>> Hmm, I'm not aware that 'clock-frequency' is not encouraged these days. So you > >>> are suggesting to change the rate in the driver itself based on the switch > >>> configuration? If so, what difference does it make? > >> > >> Based on the switch, other clocks, votes etc. whatever is reasonable > >> there. In most cases, not sure if this one here as well, devices can > >> operate on different clock frequencies thus specifying fixed frequency > >> in the DTS is simplification and lack of flexibility. It is chosen by > >> people only because it is easier for them but then they come back with > >> ABI issues when it turns out they need to switch to some dynamic control. > >> > > > > Atleast for this device, this frequency is going to be static. Because, the > > device itself cannot change the frequency, only the host driver can. That too is > > only possible before enumerating the device. So there is no way the frequency is > > going to change dynamically. > > We have assigned-clocks properties for this... but there are no clock > inputs here, so maybe it is not applicable? What generates this internal > AXI clock? Does it have internal oscillator? > I do not have access to the device schematics, but based on my knowledge there should be an internal oscillator for the device. But the device also gets the refclk from PCIe bus, but I don't know if that is somehow used as a parent of AXI bus or not. Going by the AXI bus terminology, I would assume that the frequency of this come from the internal oscillator in the device. And this is common on many PCIe devices, but they never mentioned in DT (well we do not define PCIe devices in DT usually), but Qcom wants to control the frequency of the device's internal clock to optimize the power consumption of the device. Unfortunately, the device firmware is not doing its job as expected. - Mani
On Fri, Aug 23, 2024 at 11:01:37AM +0200, Krzysztof Kozlowski wrote: > On 22/08/2024 16:16, Manivannan Sadhasivam wrote: > > On Mon, Aug 05, 2024 at 04:43:47PM +0200, Krzysztof Kozlowski wrote: > >> On 05/08/2024 07:57, Krishna Chaitanya Chundru wrote: > >>>> > >>> Hi Krzysztof, > >>> > >>> QPS615 has a 3 downstream ports and 1 upstream port as described below > >>> diagram. > >>> For this entire switch there are some supplies which we described in the > >>> dt-binding (vdd18-supply, vdd09-supply etc) and one GPIO which controls > >>> reset of the switch (reset-gpio). The switch hardware can configure the > >>> individual ports DSP0, DSP1, DSP2, upstream port and also one integrated > >>> ethernet endpoint which is connected to DSP2(I didn't mentioned in the > >>> diagram) through I2C. > >>> > >>> The properties other than supplies,i2c client, reset gpio which > >>> are added will be applicable for all the ports. > >>> _______________________________________________________________ > >>> | |i2c| QPS615 |Supplies||Resx gpio | > >>> | |___| _________________ |________||__________| > >>> | ________________| Upstream port |_____________ | > >>> | | |_______________| | | > >>> | | | | | > >>> | | | | | > >>> | ____|_____ ____|_____ ___|____ | > >>> | |DSP 0 | | DSP 1 | | DSP 2| | > >>> | |________| |________| |______| | > >>> |_____________________________________________________________| > >>> > >> > >> I don't get why then properties should apply to main device node. > >> > > > > The problem here is, we cannot differentiate between main device node and the > > upstream node. Typically the differentiation is not needed because no one cared > > about configuring the upstream port. But this PCIe switch is special (as like > > most of the Qcom peripherals). > > > > I agree that if we don't differentiate then it also implies that all main node > > properties are applicable to upstream port and vice versa. But AFAIK, upstream > > port is often considered as the _device_ itself as it shares the same bus > > number. > > Well, above diagram shows supplies being part of the entire device, not > each port. That's confusing. Based on diagram, downstream ports do not > have any supplies... and what exactly do they supply? Let's look at > vdd18 and vdd09 which sound main supplies of the entire device. In > context of port: what exactly do they power? Which part of the port? > The supplies for the downstream ports are derived from the switch power supply only. There is no way we can describe them as the port suppliers are internal to the device. - Mani
On 23/08/2024 11:44, Manivannan Sadhasivam wrote: > On Fri, Aug 23, 2024 at 11:01:37AM +0200, Krzysztof Kozlowski wrote: >> On 22/08/2024 16:16, Manivannan Sadhasivam wrote: >>> On Mon, Aug 05, 2024 at 04:43:47PM +0200, Krzysztof Kozlowski wrote: >>>> On 05/08/2024 07:57, Krishna Chaitanya Chundru wrote: >>>>>> >>>>> Hi Krzysztof, >>>>> >>>>> QPS615 has a 3 downstream ports and 1 upstream port as described below >>>>> diagram. >>>>> For this entire switch there are some supplies which we described in the >>>>> dt-binding (vdd18-supply, vdd09-supply etc) and one GPIO which controls >>>>> reset of the switch (reset-gpio). The switch hardware can configure the >>>>> individual ports DSP0, DSP1, DSP2, upstream port and also one integrated >>>>> ethernet endpoint which is connected to DSP2(I didn't mentioned in the >>>>> diagram) through I2C. >>>>> >>>>> The properties other than supplies,i2c client, reset gpio which >>>>> are added will be applicable for all the ports. >>>>> _______________________________________________________________ >>>>> | |i2c| QPS615 |Supplies||Resx gpio | >>>>> | |___| _________________ |________||__________| >>>>> | ________________| Upstream port |_____________ | >>>>> | | |_______________| | | >>>>> | | | | | >>>>> | | | | | >>>>> | ____|_____ ____|_____ ___|____ | >>>>> | |DSP 0 | | DSP 1 | | DSP 2| | >>>>> | |________| |________| |______| | >>>>> |_____________________________________________________________| >>>>> >>>> >>>> I don't get why then properties should apply to main device node. >>>> >>> >>> The problem here is, we cannot differentiate between main device node and the >>> upstream node. Typically the differentiation is not needed because no one cared >>> about configuring the upstream port. But this PCIe switch is special (as like >>> most of the Qcom peripherals). >>> >>> I agree that if we don't differentiate then it also implies that all main node >>> properties are applicable to upstream port and vice versa. But AFAIK, upstream >>> port is often considered as the _device_ itself as it shares the same bus >>> number. >> >> Well, above diagram shows supplies being part of the entire device, not >> each port. That's confusing. Based on diagram, downstream ports do not >> have any supplies... and what exactly do they supply? Let's look at >> vdd18 and vdd09 which sound main supplies of the entire device. In >> context of port: what exactly do they power? Which part of the port? >> > > The supplies for the downstream ports are derived from the switch power supply > only. There is no way we can describe them as the port suppliers are internal to > the device. IIUC, this means supplies are not valid for downstream ports, so it is a proof that binding is not correct. I don't get why we keep poking this and get to the same conclusions I had 3 weeks ago. Basically the binding is saying that downstream ports are identical to the device. Including the aspect of having more downstream ports (so device -> downstream ports -> downstream ports -> downstream ports ... infinite). To remind that was my conclusion: "Downstream port is not the same as device. Why downstream port has the same supplies? To which pins are they connected?" Best regards, Krzysztof
On Fri, Aug 23, 2024 at 03:51:25PM +0200, Krzysztof Kozlowski wrote: > On 23/08/2024 11:44, Manivannan Sadhasivam wrote: > > On Fri, Aug 23, 2024 at 11:01:37AM +0200, Krzysztof Kozlowski wrote: > >> On 22/08/2024 16:16, Manivannan Sadhasivam wrote: > >>> On Mon, Aug 05, 2024 at 04:43:47PM +0200, Krzysztof Kozlowski wrote: > >>>> On 05/08/2024 07:57, Krishna Chaitanya Chundru wrote: > >>>>>> > >>>>> Hi Krzysztof, > >>>>> > >>>>> QPS615 has a 3 downstream ports and 1 upstream port as described below > >>>>> diagram. > >>>>> For this entire switch there are some supplies which we described in the > >>>>> dt-binding (vdd18-supply, vdd09-supply etc) and one GPIO which controls > >>>>> reset of the switch (reset-gpio). The switch hardware can configure the > >>>>> individual ports DSP0, DSP1, DSP2, upstream port and also one integrated > >>>>> ethernet endpoint which is connected to DSP2(I didn't mentioned in the > >>>>> diagram) through I2C. > >>>>> > >>>>> The properties other than supplies,i2c client, reset gpio which > >>>>> are added will be applicable for all the ports. > >>>>> _______________________________________________________________ > >>>>> | |i2c| QPS615 |Supplies||Resx gpio | > >>>>> | |___| _________________ |________||__________| > >>>>> | ________________| Upstream port |_____________ | > >>>>> | | |_______________| | | > >>>>> | | | | | > >>>>> | | | | | > >>>>> | ____|_____ ____|_____ ___|____ | > >>>>> | |DSP 0 | | DSP 1 | | DSP 2| | > >>>>> | |________| |________| |______| | > >>>>> |_____________________________________________________________| > >>>>> > >>>> > >>>> I don't get why then properties should apply to main device node. > >>>> > >>> > >>> The problem here is, we cannot differentiate between main device node and the > >>> upstream node. Typically the differentiation is not needed because no one cared > >>> about configuring the upstream port. But this PCIe switch is special (as like > >>> most of the Qcom peripherals). > >>> > >>> I agree that if we don't differentiate then it also implies that all main node > >>> properties are applicable to upstream port and vice versa. But AFAIK, upstream > >>> port is often considered as the _device_ itself as it shares the same bus > >>> number. > >> > >> Well, above diagram shows supplies being part of the entire device, not > >> each port. That's confusing. Based on diagram, downstream ports do not > >> have any supplies... and what exactly do they supply? Let's look at > >> vdd18 and vdd09 which sound main supplies of the entire device. In > >> context of port: what exactly do they power? Which part of the port? > >> > > > > The supplies for the downstream ports are derived from the switch power supply > > only. There is no way we can describe them as the port suppliers are internal to > > the device. > > IIUC, this means supplies are not valid for downstream ports, so it is a > proof that binding is not correct. I don't get why we keep poking this > and get to the same conclusions I had 3 weeks ago. > > Basically the binding is saying that downstream ports are identical to > the device. Including the aspect of having more downstream ports (so > device -> downstream ports -> downstream ports -> downstream ports ... > infinite). To remind that was my conclusion: > > "Downstream port is not the same as device. Why downstream port has the > same supplies? To which pins are they connected?" > Ok. I seem to have missed your above comment and you are right. I was just clarifying the upstream port discussion as we cannot differentiate between upstream port and main device node. For downstream port, I hope Krishna will fix the binding. - Mani
diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml new file mode 100644 index 000000000000..ea0c953ee56f --- /dev/null +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml @@ -0,0 +1,191 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pci/qcom,qps615.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm QPS615 PCIe switch + +maintainers: + - Krishna chaitanya chundru <quic_krichai@quicinc.com> + +description: | + Qualcomm QPS615 PCIe switch has one upstream and three downstream + ports. The 3rd downstream port has integrated endpoint device of + Ethernet MAC. Other two downstream ports are supposed to connect + to external device. + + The QPS615 PCIe switch can be configured through I2C interface before + PCIe link is established to change FTS, ASPM related entry delays, + tx amplitude etc for better power efficiency and functionality. + +properties: + compatible: + enum: + - pci1179,0623 + + reg: + maxItems: 1 + + qcom,qps615-controller: + $ref: /schemas/types.yaml#/definitions/phandle + description: + Reference to the I2C client used to do configure qps615 + + vdd18-supply: true + + vdd09-supply: true + + vddc-supply: true + + vddio1-supply: true + + vddio2-supply: true + + vddio18-supply: true + + reset-gpios: + maxItems: 1 + description: + GPIO controlling the RESX# pin. + + qps615,axi-clk-freq-hz: + description: + AXI clock which internal bus of the switch. + + qcom,l0s-entry-delay-ns: + description: Aspm l0s entry delay in nanoseconds. + + qcom,l1-entry-delay-ns: + description: Aspm l1 entry delay in nanoseconds. + + qcom,tx-amplitude-millivolt: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Change Tx Margin setting for low power consumption. + + qcom,no-dfe: + type: boolean + description: Disables DFE (Decision Feedback Equalizer). + + qcom,nfts: + $ref: /schemas/types.yaml#/definitions/uint8 + description: + Fast Training Sequence (FTS) is the mechanism that + is used for bit and Symbol lock. + +allOf: + - $ref: /schemas/pci/pci-bus-common.yaml# + - if: + properties: + compatible: + contains: + const: pci1179,0623 + required: + - compatible + then: + required: + - vdd18-supply + - vdd09-supply + - vddc-supply + - vddio1-supply + - vddio2-supply + - vddio18-supply + - qcom,qps615-controller + - reset-gpios + +patternProperties: + "@1?[0-9a-f](,[0-7])?$": + type: object + $ref: qcom,qps615.yaml# + additionalProperties: true + +additionalProperties: true + +examples: + - | + + #include <dt-bindings/gpio/gpio.h> + + pcie { + #address-cells = <3>; + #size-cells = <2>; + + pcie@0 { + device_type = "pci"; + reg = <0x0 0x0 0x0 0x0 0x0>; + + #address-cells = <3>; + #size-cells = <2>; + ranges; + + pcie@0,0 { + compatible = "pci1179,0623"; + reg = <0x10000 0x0 0x0 0x0 0x0>; + device_type = "pci"; + #address-cells = <3>; + #size-cells = <2>; + ranges; + + qcom,qps615-controller = <&qps615_controller>; + + vdd18-supply = <&vdd>; + vdd09-supply = <&vdd>; + vddc-supply = <&vdd>; + vddio1-supply = <&vdd>; + vddio2-supply = <&vdd>; + vddio18-supply = <&vdd>; + + reset-gpios = <&gpio 1 GPIO_ACTIVE_LOW>; + + pcie@1,0 { + reg = <0x20800 0x0 0x0 0x0 0x0>; + #address-cells = <3>; + #size-cells = <2>; + device_type = "pci"; + ranges; + + qcom,no-dfe; + }; + + pcie@2,0 { + reg = <0x21000 0x0 0x0 0x0 0x0>; + #address-cells = <3>; + #size-cells = <2>; + device_type = "pci"; + ranges; + + qcom,nfts = /bits/ 8 <10>; + }; + + pcie@3,0 { + reg = <0x21800 0x0 0x0 0x0 0x0>; + #address-cells = <3>; + #size-cells = <2>; + device_type = "pci"; + ranges; + + qcom,tx-amplitude-millivolt = <10>; + + pcie@0,0 { + reg = <0x40000 0x0 0x0 0x0 0x0>; + #address-cells = <3>; + #size-cells = <2>; + device_type = "pci"; + ranges; + + qcom,l1-entry-delay-ns = <10>; + }; + + pcie@0,1 { + reg = <0x40100 0x0 0x0 0x0 0x0>; + #address-cells = <3>; + #size-cells = <2>; + device_type = "pci"; + ranges; + + qcom,l0s-entry-delay-ns = <10>; + }; + }; + }; + }; + };
Add binding describing the Qualcomm PCIe switch, QPS615, which provides Ethernet MAC integrated to the 3rd downstream port and two downstream PCIe ports. Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> --- .../devicetree/bindings/pci/qcom,qps615.yaml | 191 +++++++++++++++++++++ 1 file changed, 191 insertions(+)