| Message ID | 20241112-qps615_pwr-v3-1-29a1e98aa2b0@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Enable Power and configure the QPS615 PCIe switch | expand |
On Tue, Nov 12, 2024 at 08:31:33PM +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. > Reviewed-by: Bjorn Andersson <andersson@kernel.org> Regards, Bjorn > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > --- > .../devicetree/bindings/pci/qcom,qps615.yaml | 205 +++++++++++++++++++++ > 1 file changed, 205 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..e6a63a0bb0f3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > @@ -0,0 +1,205 @@ > +# 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 > + > + i2c-parent: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + description: | > + A phandle to the parent I2C node and the slave address of the device > + used to do configure qps615 to change FTS, tx amplitude etc. > + items: > + - description: Phandle to the I2C controller node > + - description: I2C slave address > + > + 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 rate which is internal bus of the switch > + The switch only runs in two frequencies i.e 250MHz and 125MHz. > + enum: [125000000, 250000000] > + > +allOf: > + - $ref: "#/$defs/qps615-node" > + > +patternProperties: > + "@1?[0-9a-f](,[0-7])?$": > + description: child nodes describing the internal downstream ports > + the qps615 switch. > + type: object > + $ref: "#/$defs/qps615-node" > + unevaluatedProperties: false > + > +$defs: > + qps615-node: > + type: object > + > + properties: > + qcom,l0s-entry-delay-ns: > + description: Aspm l0s entry delay. > + > + qcom,l1-entry-delay-ns: > + description: Aspm l1 entry delay. > + > + qcom,tx-amplitude-millivolt: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: Change Tx Margin setting for low power consumption. > + > + qcom,no-dfe-support: > + type: boolean > + description: Disable DFE (Decision Feedback Equalizer), which mitigates > + intersymbol interference and some reflections caused by impedance mismatches. > + > + qcom,nfts: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Number of Fast Training Sequence (FTS) used during L0s to L0 exit > + for bit and Symbol lock. > + > + allOf: > + - $ref: /schemas/pci/pci-bus.yaml# > + > +unevaluatedProperties: false > + > +required: > + - vdd18-supply > + - vdd09-supply > + - vddc-supply > + - vddio1-supply > + - vddio2-supply > + - vddio18-supply > + - i2c-parent > + - reset-gpios > + > +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; > + bus-range = <0x01 0xff>; > + > + pcie@0,0 { > + compatible = "pci1179,0623"; > + reg = <0x10000 0x0 0x0 0x0 0x0>; > + device_type = "pci"; > + #address-cells = <3>; > + #size-cells = <2>; > + ranges; > + bus-range = <0x02 0xff>; > + > + i2c-parent = <&qup_i2c 0x77>; > + > + 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; > + bus-range = <0x03 0xff>; > + > + qcom,no-dfe-support; > + }; > + > + pcie@2,0 { > + reg = <0x21000 0x0 0x0 0x0 0x0>; > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; > + ranges; > + bus-range = <0x04 0xff>; > + > + qcom,nfts = <10>; > + }; > + > + pcie@3,0 { > + reg = <0x21800 0x0 0x0 0x0 0x0>; > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; > + ranges; > + bus-range = <0x05 0xff>; > + > + qcom,tx-amplitude-millivolt = <10>; > + pcie@0,0 { > + reg = <0x50000 0x0 0x0 0x0 0x0>; > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; > + ranges; > + > + qcom,l1-entry-delay-ns = <10>; > + }; > + > + pcie@0,1 { > + reg = <0x50100 0x0 0x0 0x0 0x0>; > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; > + ranges; > + > + qcom,l0s-entry-delay-ns = <10>; > + }; > + }; > + }; > + }; > + }; > > -- > 2.34.1 >
On Tue, Nov 12, 2024 at 08:31:33PM +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 | 205 +++++++++++++++++++++ > 1 file changed, 205 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..e6a63a0bb0f3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > @@ -0,0 +1,205 @@ > +# 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 > + > + i2c-parent: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + description: | Don't need '|' if no formatting to preserve. > + A phandle to the parent I2C node and the slave address of the device > + used to do configure qps615 to change FTS, tx amplitude etc. > + items: > + - description: Phandle to the I2C controller node > + - description: I2C slave address > + > + 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. Is the PERST# or something else? > + > + qps615,axi-clk-freq-hz: qps615 is not a vendor prefix. > + description: > + AXI clock rate which is internal bus of the switch > + The switch only runs in two frequencies i.e 250MHz and 125MHz. > + enum: [125000000, 250000000] > + > +allOf: > + - $ref: "#/$defs/qps615-node" > + > +patternProperties: > + "@1?[0-9a-f](,[0-7])?$": You have 3 ports. So isn't this fixed and limited to 0-2? > + description: child nodes describing the internal downstream ports > + the qps615 switch. Please be consistent with starting after the ':' or on the next line. And start with capital C. > + type: object > + $ref: "#/$defs/qps615-node" > + unevaluatedProperties: false > + > +$defs: > + qps615-node: > + type: object > + > + properties: > + qcom,l0s-entry-delay-ns: > + description: Aspm l0s entry delay. > + > + qcom,l1-entry-delay-ns: > + description: Aspm l1 entry delay. These should probably be common being standard PCIe things. Though, why are they needed? I'm sure the timing is defined by the PCIe spec, so they are not compliant? > + > + qcom,tx-amplitude-millivolt: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: Change Tx Margin setting for low power consumption. > + > + qcom,no-dfe-support: > + type: boolean > + description: Disable DFE (Decision Feedback Equalizer), which mitigates > + intersymbol interference and some reflections caused by impedance mismatches. > + > + qcom,nfts: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Number of Fast Training Sequence (FTS) used during L0s to L0 exit > + for bit and Symbol lock. Also something common. The problem I have with all these properties is you are using them on both the upstream and downstream sides of the PCIe links. They belong in either the device's node (downstream) or the bus's node (upstream). > + > + allOf: > + - $ref: /schemas/pci/pci-bus.yaml# pci-pci-bridge.yaml is more specific and closer to what this device is. > + > +unevaluatedProperties: false > + > +required: > + - vdd18-supply > + - vdd09-supply > + - vddc-supply > + - vddio1-supply > + - vddio2-supply > + - vddio18-supply > + - i2c-parent > + - reset-gpios > + > +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; > + bus-range = <0x01 0xff>; > + > + pcie@0,0 { > + compatible = "pci1179,0623"; > + reg = <0x10000 0x0 0x0 0x0 0x0>; > + device_type = "pci"; > + #address-cells = <3>; > + #size-cells = <2>; > + ranges; > + bus-range = <0x02 0xff>; > + > + i2c-parent = <&qup_i2c 0x77>; > + > + 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; > + bus-range = <0x03 0xff>; > + > + qcom,no-dfe-support; > + }; > + > + pcie@2,0 { > + reg = <0x21000 0x0 0x0 0x0 0x0>; > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; > + ranges; > + bus-range = <0x04 0xff>; > + > + qcom,nfts = <10>; > + }; > + > + pcie@3,0 { > + reg = <0x21800 0x0 0x0 0x0 0x0>; > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; > + ranges; > + bus-range = <0x05 0xff>; > + > + qcom,tx-amplitude-millivolt = <10>; > + pcie@0,0 { > + reg = <0x50000 0x0 0x0 0x0 0x0>; > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; There's a 2nd PCI-PCI bridge? > + ranges; > + > + qcom,l1-entry-delay-ns = <10>; > + }; > + > + pcie@0,1 { > + reg = <0x50100 0x0 0x0 0x0 0x0>; > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; > + ranges; > + > + qcom,l0s-entry-delay-ns = <10>; > + }; > + }; > + }; > + }; > + }; > > -- > 2.34.1 >
On Tue, Nov 12, 2024 at 08:31:33PM +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 | 205 +++++++++++++++++++++ > 1 file changed, 205 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..e6a63a0bb0f3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml Isn't "qcom,qps615" a SoC name? This is supposed to be matching compatible, in your case probably qcom,qps615-whatever-this-is? ... > + qps615,axi-clk-freq-hz: That's a downstream code you send us. Anyway, why assigned clock rates do not work for you? You are re-implementing legacy property now under different name :/ > + description: > + AXI clock rate which is internal bus of the switch > + The switch only runs in two frequencies i.e 250MHz and 125MHz. > + enum: [125000000, 250000000] > + > +allOf: > + - $ref: "#/$defs/qps615-node" > + > +patternProperties: > + "@1?[0-9a-f](,[0-7])?$": > + description: child nodes describing the internal downstream ports > + the qps615 switch. > + type: object > + $ref: "#/$defs/qps615-node" > + unevaluatedProperties: false > + > +$defs: > + qps615-node: > + type: object > + > + properties: > + qcom,l0s-entry-delay-ns: > + description: Aspm l0s entry delay. > + > + qcom,l1-entry-delay-ns: > + description: Aspm l1 entry delay. > + > + qcom,tx-amplitude-millivolt: -microvolt does not work for you? > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: Change Tx Margin setting for low power consumption. > + > + qcom,no-dfe-support: > + type: boolean > + description: Disable DFE (Decision Feedback Equalizer), which mitigates > + intersymbol interference and some reflections caused by impedance mismatches. > + > + qcom,nfts: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Number of Fast Training Sequence (FTS) used during L0s to L0 exit > + for bit and Symbol lock. Use some of these properties in the example. I saw only one. > + > + allOf: > + - $ref: /schemas/pci/pci-bus.yaml# > + > +unevaluatedProperties: false > + > +required: > + - vdd18-supply > + - vdd09-supply > + - vddc-supply > + - vddio1-supply > + - vddio2-supply > + - vddio18-supply > + - i2c-parent > + - reset-gpios > + > +examples: > + - | > + Drop blank line > + #include <dt-bindings/gpio/gpio.h> > + > + pcie { > + #address-cells = <3>; > + #size-cells = <2>; > + > + pcie@0 { > + device_type = "pci"; > + reg = <0x0 0x0 0x0 0x0 0x0>; Best regards, Krzysztof
On 11/15/2024 9:48 PM, Rob Herring wrote: > On Tue, Nov 12, 2024 at 08:31:33PM +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 | 205 +++++++++++++++++++++ >> 1 file changed, 205 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..e6a63a0bb0f3 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >> @@ -0,0 +1,205 @@ >> +# 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 >> + >> + i2c-parent: >> + $ref: /schemas/types.yaml#/definitions/phandle-array >> + description: | > > Don't need '|' if no formatting to preserve. > ack >> + A phandle to the parent I2C node and the slave address of the device >> + used to do configure qps615 to change FTS, tx amplitude etc. >> + items: >> + - description: Phandle to the I2C controller node >> + - description: I2C slave address >> + >> + 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. > > Is the PERST# or something else? > it is not PERST GPIO, it is similar to PERST in terms of functionality which brings switch out from reset. >> + >> + qps615,axi-clk-freq-hz: > > qps615 is not a vendor prefix. > >> + description: >> + AXI clock rate which is internal bus of the switch >> + The switch only runs in two frequencies i.e 250MHz and 125MHz. >> + enum: [125000000, 250000000] >> + >> +allOf: >> + - $ref: "#/$defs/qps615-node" >> + >> +patternProperties: >> + "@1?[0-9a-f](,[0-7])?$": > > You have 3 ports. So isn't this fixed and limited to 0-2? > sure I will change it to below as suggested "@1?[0-3](,[0-1])?$" >> + description: child nodes describing the internal downstream ports >> + the qps615 switch. > > Please be consistent with starting after the ':' or on the next line. > > And start with capital C. > > ack >> + type: object >> + $ref: "#/$defs/qps615-node" >> + unevaluatedProperties: false >> + >> +$defs: >> + qps615-node: >> + type: object >> + >> + properties: >> + qcom,l0s-entry-delay-ns: >> + description: Aspm l0s entry delay. >> + >> + qcom,l1-entry-delay-ns: >> + description: Aspm l1 entry delay. > > These should probably be common being standard PCIe things. Though, why > are they needed? I'm sure the timing is defined by the PCIe spec, so > they are not compliant? > Usually the firmware in the endpoints/switches should do this these configurations. But the qps615 PCIe switch doesn't have any firmware running to configure these. So the hardware exposes i2c interface to configure these before link training. >> + >> + qcom,tx-amplitude-millivolt: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: Change Tx Margin setting for low power consumption. >> + >> + qcom,no-dfe-support: >> + type: boolean >> + description: Disable DFE (Decision Feedback Equalizer), which mitigates >> + intersymbol interference and some reflections caused by impedance mismatches. >> + >> + qcom,nfts: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: >> + Number of Fast Training Sequence (FTS) used during L0s to L0 exit >> + for bit and Symbol lock. > > Also something common. > > The problem I have with all these properties is you are using them on > both the upstream and downstream sides of the PCIe links. They belong in > either the device's node (downstream) or the bus's node (upstream). > This switch allows us to configure both upstream, downstream ports and also embedded Ethernet port which is internal to the switch. These properties are applicable for all of those. >> + >> + allOf: >> + - $ref: /schemas/pci/pci-bus.yaml# > > pci-pci-bridge.yaml is more specific and closer to what this device is. > I tried this now, I was getting warning saying the compatible /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: pcie@0,0: compatible: ['pci1179,0623'] does not contain items matching the given schema from schema $id: http://devicetree.org/schemas/pci/qcom,qps615.yaml# /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: pcie@0,0: Unevaluated properties are not allowed ('#address-cells', '#size-cells', 'bus-range', 'device_type', 'ranges' were unexpected) I think pci-pci-bridge is expecting the compatible string in this format only "pciclass,0604". >> + >> +unevaluatedProperties: false >> + >> +required: >> + - vdd18-supply >> + - vdd09-supply >> + - vddc-supply >> + - vddio1-supply >> + - vddio2-supply >> + - vddio18-supply >> + - i2c-parent >> + - reset-gpios >> + >> +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; >> + bus-range = <0x01 0xff>; >> + >> + pcie@0,0 { >> + compatible = "pci1179,0623"; >> + reg = <0x10000 0x0 0x0 0x0 0x0>; >> + device_type = "pci"; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + ranges; >> + bus-range = <0x02 0xff>; >> + >> + i2c-parent = <&qup_i2c 0x77>; >> + >> + 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; >> + bus-range = <0x03 0xff>; >> + >> + qcom,no-dfe-support; >> + }; >> + >> + pcie@2,0 { >> + reg = <0x21000 0x0 0x0 0x0 0x0>; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + device_type = "pci"; >> + ranges; >> + bus-range = <0x04 0xff>; >> + >> + qcom,nfts = <10>; >> + }; >> + >> + pcie@3,0 { >> + reg = <0x21800 0x0 0x0 0x0 0x0>; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + device_type = "pci"; >> + ranges; >> + bus-range = <0x05 0xff>; >> + >> + qcom,tx-amplitude-millivolt = <10>; >> + pcie@0,0 { >> + reg = <0x50000 0x0 0x0 0x0 0x0>; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + device_type = "pci"; > > There's a 2nd PCI-PCI bridge? This the embedded ethernet port which is as part of DSP3. - Krishna Chaitanya. > >> + ranges; >> + >> + qcom,l1-entry-delay-ns = <10>; >> + }; >> + >> + pcie@0,1 { >> + reg = <0x50100 0x0 0x0 0x0 0x0>; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + device_type = "pci"; >> + ranges; >> + >> + qcom,l0s-entry-delay-ns = <10>; >> + }; >> + }; >> + }; >> + }; >> + }; >> >> -- >> 2.34.1 >>
On 11/20/2024 1:34 PM, Krzysztof Kozlowski wrote: > On Tue, Nov 12, 2024 at 08:31:33PM +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 | 205 +++++++++++++++++++++ >> 1 file changed, 205 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..e6a63a0bb0f3 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > > Isn't "qcom,qps615" a SoC name? This is supposed to be matching > compatible, in your case probably qcom,qps615-whatever-this-is? > qcom,qcs615 is a soc name, qcom,qps615 is the pcie switch. ack I will change it to qcom,qps615-pcie > ... > >> + qps615,axi-clk-freq-hz: > > That's a downstream code you send us. > > Anyway, why assigned clock rates do not work for you? You are > re-implementing legacy property now under different name :/ > > The assigned clock rates comes in to the picture when we are using clock framework to control the clocks. For this switch there are no clocks needs to be control, the moment we power on the switch clocks are enabled by default. This switch provides a mechanism to control the frequency using i2c. And switch supports only two frequencies i.e 125MHz and 250MHZ by default it runs on 250MHz, we can do one i2c write with which switch runs in 125MHz. >> + description: >> + AXI clock rate which is internal bus of the switch >> + The switch only runs in two frequencies i.e 250MHz and 125MHz. >> + enum: [125000000, 250000000] >> + >> +allOf: >> + - $ref: "#/$defs/qps615-node" >> + >> +patternProperties: >> + "@1?[0-9a-f](,[0-7])?$": >> + description: child nodes describing the internal downstream ports >> + the qps615 switch. >> + type: object >> + $ref: "#/$defs/qps615-node" >> + unevaluatedProperties: false >> + >> +$defs: >> + qps615-node: >> + type: object >> + >> + properties: >> + qcom,l0s-entry-delay-ns: >> + description: Aspm l0s entry delay. >> + >> + qcom,l1-entry-delay-ns: >> + description: Aspm l1 entry delay. >> + >> + qcom,tx-amplitude-millivolt: > > -microvolt does not work for you? > let me check this if it is applicable we will change it to microvolt. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: Change Tx Margin setting for low power consumption. >> + >> + qcom,no-dfe-support: >> + type: boolean >> + description: Disable DFE (Decision Feedback Equalizer), which mitigates >> + intersymbol interference and some reflections caused by impedance mismatches. >> + >> + qcom,nfts: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: >> + Number of Fast Training Sequence (FTS) used during L0s to L0 exit >> + for bit and Symbol lock. > > Use some of these properties in the example. I saw only one. > In the next patch I will try to use all the properties as suggested. >> + >> + allOf: >> + - $ref: /schemas/pci/pci-bus.yaml# >> + >> +unevaluatedProperties: false >> + >> +required: >> + - vdd18-supply >> + - vdd09-supply >> + - vddc-supply >> + - vddio1-supply >> + - vddio2-supply >> + - vddio18-supply >> + - i2c-parent >> + - reset-gpios >> + >> +examples: >> + - | >> + > > Drop blank line > ack. - Krishna Chaitanya. >> + #include <dt-bindings/gpio/gpio.h> >> + >> + pcie { >> + #address-cells = <3>; >> + #size-cells = <2>; >> + >> + pcie@0 { >> + device_type = "pci"; >> + reg = <0x0 0x0 0x0 0x0 0x0>; > > Best regards, > Krzysztof >
On 24/11/2024 02:41, Krishna Chaitanya Chundru wrote: >> ... >> >>> + qps615,axi-clk-freq-hz: >> >> That's a downstream code you send us. >> >> Anyway, why assigned clock rates do not work for you? You are >> re-implementing legacy property now under different name :/ >> >> The assigned clock rates comes in to the picture when we are using clock > framework to control the clocks. For this switch there are no clocks > needs to be control, the moment we power on the switch clocks are > enabled by default. This switch provides a mechanism to control the > frequency using i2c. And switch supports only two frequencies i.e frequency of what, since there are no clocks? > 125MHz and 250MHZ by default it runs on 250MHz, we can do one i2c > write with which switch runs in 125MHz. How doing a write is relevant? Or you want to say you can control clock? Best regards, Krzysztof
On 11/25/2024 1:10 PM, Krzysztof Kozlowski wrote: > On 24/11/2024 02:41, Krishna Chaitanya Chundru wrote: >>> ... >>> >>>> + qps615,axi-clk-freq-hz: >>> >>> That's a downstream code you send us. >>> >>> Anyway, why assigned clock rates do not work for you? You are >>> re-implementing legacy property now under different name :/ >>> >>> The assigned clock rates comes in to the picture when we are >>> using clock >> framework to control the clocks. For this switch there are no >> clocks needs to be control, the moment we power on the switch >> clocks are enabled by default. This switch provides a mechanism to >> control the frequency using i2c. And switch supports only two >> frequencies i.e > > > frequency of what, since there are no clocks? > The axi clock frequency internal to the switch, host can't control the enablement of the clocks it can control only the frequency. we already had a discussion on this on v2[1], and we taught you agreed on this property. [1] https://lore.kernel.org/netdev/d1af1eac-f9bd-7a8e-586b-5c2a76445145@codeaurora.org/T/#m3d5864c758f2e05fa15ba522aad6a37e3417bd9f - Krishna Chaitanya. >> 125MHz and 250MHZ by default it runs on 250MHz, we can do one i2c >> write with which switch runs in 125MHz. > > How doing a write is relevant? Or you want to say you can control > clock? > > > > Best regards, Krzysztof
On 26/11/2024 07:50, Krishna Chaitanya Chundru wrote: > > > On 11/25/2024 1:10 PM, Krzysztof Kozlowski wrote: >> On 24/11/2024 02:41, Krishna Chaitanya Chundru wrote: >>>> ... >>>> >>>>> + qps615,axi-clk-freq-hz: >>>> >>>> That's a downstream code you send us. >>>> >>>> Anyway, why assigned clock rates do not work for you? You are >>>> re-implementing legacy property now under different name :/ >>>> >>>> The assigned clock rates comes in to the picture when we are >>>> using clock >>> framework to control the clocks. For this switch there are no >>> clocks needs to be control, the moment we power on the switch >>> clocks are enabled by default. This switch provides a mechanism to >>> control the frequency using i2c. And switch supports only two >>> frequencies i.e >> >> >> frequency of what, since there are no clocks? >> > The axi clock frequency internal to the switch, host can't control > the enablement of the clocks it can control only the frequency. > > we already had a discussion on this on v2[1], and we taught you agreed > on this property. > > [1] > https://lore.kernel.org/netdev/d1af1eac-f9bd-7a8e-586b-5c2a76445145@codeaurora.org/T/#m3d5864c758f2e05fa15ba522aad6a37e3417bd9f > This points something else. I diged v2 and found many unanswered questions and unfinished discussion: https://lore.kernel.org/r/linux-arm-msm/20240803-qps615-v2-0-9560b7c71369@quicinc.com/T/#m7074ab9e5f89e29faf430c82f769e67d0e4072cf The property description did not improve, actually it got worse: you repeat constraints in free form text. Best regards, Krzysztof
On Tue, Nov 26, 2024 at 07:58:16AM +0100, Krzysztof Kozlowski wrote: > On 26/11/2024 07:50, Krishna Chaitanya Chundru wrote: > > > > > > On 11/25/2024 1:10 PM, Krzysztof Kozlowski wrote: > >> On 24/11/2024 02:41, Krishna Chaitanya Chundru wrote: > >>>> ... > >>>> > >>>>> + qps615,axi-clk-freq-hz: > >>>> > >>>> That's a downstream code you send us. > >>>> > >>>> Anyway, why assigned clock rates do not work for you? You are > >>>> re-implementing legacy property now under different name :/ > >>>> > >>>> The assigned clock rates comes in to the picture when we are > >>>> using clock > >>> framework to control the clocks. For this switch there are no > >>> clocks needs to be control, the moment we power on the switch > >>> clocks are enabled by default. This switch provides a mechanism to > >>> control the frequency using i2c. And switch supports only two > >>> frequencies i.e > >> > >> > >> frequency of what, since there are no clocks? > >> > > The axi clock frequency internal to the switch, host can't control > > the enablement of the clocks it can control only the frequency. > > > > we already had a discussion on this on v2[1], and we taught you agreed > > on this property. > > > > [1] > > https://lore.kernel.org/netdev/d1af1eac-f9bd-7a8e-586b-5c2a76445145@codeaurora.org/T/#m3d5864c758f2e05fa15ba522aad6a37e3417bd9f > > > > This points something else. I diged v2 and found many unanswered > questions and unfinished discussion: > The conversation is here: https://lore.kernel.org/linux-arm-msm/20240823094028.7xul4eoiexey5xjm@thinkpad/ But there was no explicit agreement on the usage of 'qps615,axi-clk-freq-hz'. If describing the PCI device's internal clock frequency is not applicable, then I'd recommend to change the clock rate in the driver itself based on the number of DSPs enabled (or based on other configuration). - Mani
On Thu, Nov 28, 2024 at 06:54:32PM +0530, Manivannan Sadhasivam wrote: > On Tue, Nov 26, 2024 at 07:58:16AM +0100, Krzysztof Kozlowski wrote: > > On 26/11/2024 07:50, Krishna Chaitanya Chundru wrote: > > > > > > > > > On 11/25/2024 1:10 PM, Krzysztof Kozlowski wrote: > > >> On 24/11/2024 02:41, Krishna Chaitanya Chundru wrote: > > >>>> ... > > >>>> > > >>>>> + qps615,axi-clk-freq-hz: > > >>>> > > >>>> That's a downstream code you send us. > > >>>> > > >>>> Anyway, why assigned clock rates do not work for you? You are > > >>>> re-implementing legacy property now under different name :/ > > >>>> > > >>>> The assigned clock rates comes in to the picture when we are > > >>>> using clock > > >>> framework to control the clocks. For this switch there are no > > >>> clocks needs to be control, the moment we power on the switch > > >>> clocks are enabled by default. This switch provides a mechanism to > > >>> control the frequency using i2c. And switch supports only two > > >>> frequencies i.e > > >> > > >> > > >> frequency of what, since there are no clocks? > > >> > > > The axi clock frequency internal to the switch, host can't control > > > the enablement of the clocks it can control only the frequency. > > > > > > we already had a discussion on this on v2[1], and we taught you agreed > > > on this property. > > > > > > [1] > > > https://lore.kernel.org/netdev/d1af1eac-f9bd-7a8e-586b-5c2a76445145@codeaurora.org/T/#m3d5864c758f2e05fa15ba522aad6a37e3417bd9f > > > > > > > This points something else. I diged v2 and found many unanswered > > questions and unfinished discussion: > > > > The conversation is here: > https://lore.kernel.org/linux-arm-msm/20240823094028.7xul4eoiexey5xjm@thinkpad/ > > But there was no explicit agreement on the usage of 'qps615,axi-clk-freq-hz'. > > If describing the PCI device's internal clock frequency is not applicable, then > I'd recommend to change the clock rate in the driver itself based on the number > of DSPs enabled (or based on other configuration). Sounds like the best approach. Otherwise it's not obvious, what is the criteria for selecting this or that clock value. > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
On 11/28/2024 7:38 PM, Dmitry Baryshkov wrote: > On Thu, Nov 28, 2024 at 06:54:32PM +0530, Manivannan Sadhasivam wrote: >> On Tue, Nov 26, 2024 at 07:58:16AM +0100, Krzysztof Kozlowski wrote: >>> On 26/11/2024 07:50, Krishna Chaitanya Chundru wrote: >>>> >>>> >>>> On 11/25/2024 1:10 PM, Krzysztof Kozlowski wrote: >>>>> On 24/11/2024 02:41, Krishna Chaitanya Chundru wrote: >>>>>>> ... >>>>>>> >>>>>>>> + qps615,axi-clk-freq-hz: >>>>>>> >>>>>>> That's a downstream code you send us. >>>>>>> >>>>>>> Anyway, why assigned clock rates do not work for you? You are >>>>>>> re-implementing legacy property now under different name :/ >>>>>>> >>>>>>> The assigned clock rates comes in to the picture when we are >>>>>>> using clock >>>>>> framework to control the clocks. For this switch there are no >>>>>> clocks needs to be control, the moment we power on the switch >>>>>> clocks are enabled by default. This switch provides a mechanism to >>>>>> control the frequency using i2c. And switch supports only two >>>>>> frequencies i.e >>>>> >>>>> >>>>> frequency of what, since there are no clocks? >>>>> >>>> The axi clock frequency internal to the switch, host can't control >>>> the enablement of the clocks it can control only the frequency. >>>> >>>> we already had a discussion on this on v2[1], and we taught you agreed >>>> on this property. >>>> >>>> [1] >>>> https://lore.kernel.org/netdev/d1af1eac-f9bd-7a8e-586b-5c2a76445145@codeaurora.org/T/#m3d5864c758f2e05fa15ba522aad6a37e3417bd9f >>>> >>> >>> This points something else. I diged v2 and found many unanswered >>> questions and unfinished discussion: >>> >> >> The conversation is here: >> https://lore.kernel.org/linux-arm-msm/20240823094028.7xul4eoiexey5xjm@thinkpad/ >> >> But there was no explicit agreement on the usage of 'qps615,axi-clk-freq-hz'. >> >> If describing the PCI device's internal clock frequency is not applicable, then >> I'd recommend to change the clock rate in the driver itself based on the number >> of DSPs enabled (or based on other configuration). > > Sounds like the best approach. Otherwise it's not obvious, what is the > criteria for selecting this or that clock value. > I will remove this property in next patch and driver code to set this frequency in the next patch, we will try to come up with some logic later. - Krishna chaitanya. >> >> - Mani >> >> -- >> மணிவண்ணன் சதாசிவம் >
On 11/24/2024 7:02 AM, Krishna Chaitanya Chundru wrote: > > > On 11/15/2024 9:48 PM, Rob Herring wrote: >> On Tue, Nov 12, 2024 at 08:31:33PM +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 | 205 >>> +++++++++++++++++++++ >>> 1 file changed, 205 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..e6a63a0bb0f3 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >>> @@ -0,0 +1,205 @@ >>> +# 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 >>> + >>> + i2c-parent: >>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>> + description: | >> >> Don't need '|' if no formatting to preserve. >> > ack >>> + A phandle to the parent I2C node and the slave address of the >>> device >>> + used to do configure qps615 to change FTS, tx amplitude etc. >>> + items: >>> + - description: Phandle to the I2C controller node >>> + - description: I2C slave address >>> + >>> + 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. >> >> Is the PERST# or something else? >> > it is not PERST GPIO, it is similar to PERST in terms > of functionality which brings switch out from reset. >>> + >>> + qps615,axi-clk-freq-hz: >> >> qps615 is not a vendor prefix. >> >>> + description: >>> + AXI clock rate which is internal bus of the switch >>> + The switch only runs in two frequencies i.e 250MHz and 125MHz. >>> + enum: [125000000, 250000000] >>> + >>> +allOf: >>> + - $ref: "#/$defs/qps615-node" >>> + >>> +patternProperties: >>> + "@1?[0-9a-f](,[0-7])?$": >> >> You have 3 ports. So isn't this fixed and limited to 0-2? >> > sure I will change it to below as suggested > "@1?[0-3](,[0-1])?$" >>> + description: child nodes describing the internal downstream ports >>> + the qps615 switch. >> >> Please be consistent with starting after the ':' or on the next line. >> >> And start with capital C. >> >> > ack > >>> + type: object >>> + $ref: "#/$defs/qps615-node" >>> + unevaluatedProperties: false >>> + >>> +$defs: >>> + qps615-node: >>> + type: object >>> + >>> + properties: >>> + qcom,l0s-entry-delay-ns: >>> + description: Aspm l0s entry delay. >>> + >>> + qcom,l1-entry-delay-ns: >>> + description: Aspm l1 entry delay. >> >> These should probably be common being standard PCIe things. Though, why >> are they needed? I'm sure the timing is defined by the PCIe spec, so >> they are not compliant? >> > Usually the firmware in the endpoints/switches should do this these > configurations. But the qps615 PCIe switch doesn't have any firmware > running to configure these. So the hardware exposes i2c interface to > configure these before link training. >>> + >>> + qcom,tx-amplitude-millivolt: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: Change Tx Margin setting for low power >>> consumption. >>> + >>> + qcom,no-dfe-support: >>> + type: boolean >>> + description: Disable DFE (Decision Feedback Equalizer), >>> which mitigates >>> + intersymbol interference and some reflections caused by >>> impedance mismatches. >>> + >>> + qcom,nfts: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: >>> + Number of Fast Training Sequence (FTS) used during L0s to >>> L0 exit >>> + for bit and Symbol lock. >> >> Also something common. >> >> The problem I have with all these properties is you are using them on >> both the upstream and downstream sides of the PCIe links. They belong in >> either the device's node (downstream) or the bus's node (upstream). >> > This switch allows us to configure both upstream, downstream ports and > also embedded Ethernet port which is internal to the switch. These > properties are applicable for all of those. >>> + >>> + allOf: >>> + - $ref: /schemas/pci/pci-bus.yaml# >> >> pci-pci-bridge.yaml is more specific and closer to what this device is. >> > I tried this now, I was getting warning saying the compatible > /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: pcie@0,0: compatible: ['pci1179,0623'] does not contain items matching the given schema > from schema $id: > http://devicetree.org/schemas/pci/qcom,qps615.yaml# > /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: pcie@0,0: Unevaluated properties are not allowed ('#address-cells', '#size-cells', 'bus-range', 'device_type', 'ranges' were unexpected) > > I think pci-pci-bridge is expecting the compatible string in this format > only "pciclass,0604". > >>> + >>> +unevaluatedProperties: false >>> + >>> +required: >>> + - vdd18-supply >>> + - vdd09-supply >>> + - vddc-supply >>> + - vddio1-supply >>> + - vddio2-supply >>> + - vddio18-supply >>> + - i2c-parent >>> + - reset-gpios >>> + >>> +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; >>> + bus-range = <0x01 0xff>; >>> + >>> + pcie@0,0 { >>> + compatible = "pci1179,0623"; >>> + reg = <0x10000 0x0 0x0 0x0 0x0>; >>> + device_type = "pci"; >>> + #address-cells = <3>; >>> + #size-cells = <2>; >>> + ranges; >>> + bus-range = <0x02 0xff>; >>> + >>> + i2c-parent = <&qup_i2c 0x77>; >>> + >>> + 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; >>> + bus-range = <0x03 0xff>; >>> + >>> + qcom,no-dfe-support; >>> + }; >>> + >>> + pcie@2,0 { >>> + reg = <0x21000 0x0 0x0 0x0 0x0>; >>> + #address-cells = <3>; >>> + #size-cells = <2>; >>> + device_type = "pci"; >>> + ranges; >>> + bus-range = <0x04 0xff>; >>> + >>> + qcom,nfts = <10>; >>> + }; >>> + >>> + pcie@3,0 { >>> + reg = <0x21800 0x0 0x0 0x0 0x0>; >>> + #address-cells = <3>; >>> + #size-cells = <2>; >>> + device_type = "pci"; >>> + ranges; >>> + bus-range = <0x05 0xff>; >>> + >>> + qcom,tx-amplitude-millivolt = <10>; >>> + pcie@0,0 { >>> + reg = <0x50000 0x0 0x0 0x0 0x0>; >>> + #address-cells = <3>; >>> + #size-cells = <2>; >>> + device_type = "pci"; >> >> There's a 2nd PCI-PCI bridge? > This the embedded ethernet port which is as part of DSP3. > Hi Rob, Can you please check my response on your queries, if you need any extra information, we can provide to sort this out. - Krishna Chaitanya. > - Krishna Chaitanya. >> >>> + ranges; >>> + >>> + qcom,l1-entry-delay-ns = <10>; >>> + }; >>> + >>> + pcie@0,1 { >>> + reg = <0x50100 0x0 0x0 0x0 0x0>; >>> + #address-cells = <3>; >>> + #size-cells = <2>; >>> + device_type = "pci"; >>> + ranges; >>> + >>> + qcom,l0s-entry-delay-ns = <10>; >>> + }; >>> + }; >>> + }; >>> + }; >>> + }; >>> >>> -- >>> 2.34.1 >>> >
On Tue, Nov 12, 2024 at 08:31:33PM +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. > +$defs: > + qps615-node: > + type: object > + > + properties: > + qcom,l0s-entry-delay-ns: > + description: Aspm l0s entry delay. > + > + qcom,l1-entry-delay-ns: > + description: Aspm l1 entry delay. To match spec usage: s/Aspm/ASPM/ s/l0s/L0s/ s/l1/L1/ Other than the fact that qps615 needs the driver to configure these, there's nothing qcom-specific here, so I suggest the names should omit "qcom" and include "aspm". > + 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; > + bus-range = <0x01 0xff>; > + > + pcie@0,0 { > + compatible = "pci1179,0623"; > + reg = <0x10000 0x0 0x0 0x0 0x0>; > + device_type = "pci"; > + #address-cells = <3>; > + #size-cells = <2>; > + ranges; > + bus-range = <0x02 0xff>; This binding describes a switch. I don't think bus-range should appear here at all because it is not a feature of the hardware (unless the switch ports are broken and their Secondary/Subordinate Bus Numbers are hard-wired). The Primary/Secondary/Subordinate Bus Numbers of all switch ports should be writable and the PCI core knows how to manage them. Bjorn
On Wed, Dec 04, 2024 at 03:25:59PM -0600, Bjorn Helgaas wrote: > On Tue, Nov 12, 2024 at 08:31:33PM +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. > > > +$defs: > > + qps615-node: > > + type: object > > + > > + properties: > > + qcom,l0s-entry-delay-ns: > > + description: Aspm l0s entry delay. > > + > > + qcom,l1-entry-delay-ns: > > + description: Aspm l1 entry delay. > > To match spec usage: > s/Aspm/ASPM/ > s/l0s/L0s/ > s/l1/L1/ > > Other than the fact that qps615 needs the driver to configure these, > there's nothing qcom-specific here, so I suggest the names should omit > "qcom" and include "aspm". > In that case, these properties should be documented in dt-schema: https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-bus-common.yaml - Mani
On 12/4/2024 2:19 PM, Krishna Chaitanya Chundru wrote: > > > On 11/24/2024 7:02 AM, Krishna Chaitanya Chundru wrote: >> >> >> On 11/15/2024 9:48 PM, Rob Herring wrote: >>> On Tue, Nov 12, 2024 at 08:31:33PM +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 | 205 >>>> +++++++++++++++++++++ >>>> 1 file changed, 205 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..e6a63a0bb0f3 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >>>> @@ -0,0 +1,205 @@ >>>> +# 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 >>>> + >>>> + i2c-parent: >>>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>>> + description: | >>> >>> Don't need '|' if no formatting to preserve. >>> >> ack >>>> + A phandle to the parent I2C node and the slave address of the >>>> device >>>> + used to do configure qps615 to change FTS, tx amplitude etc. >>>> + items: >>>> + - description: Phandle to the I2C controller node >>>> + - description: I2C slave address >>>> + >>>> + 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. >>> >>> Is the PERST# or something else? >>> >> it is not PERST GPIO, it is similar to PERST in terms >> of functionality which brings switch out from reset. >>>> + >>>> + qps615,axi-clk-freq-hz: >>> >>> qps615 is not a vendor prefix. >>> >>>> + description: >>>> + AXI clock rate which is internal bus of the switch >>>> + The switch only runs in two frequencies i.e 250MHz and 125MHz. >>>> + enum: [125000000, 250000000] >>>> + >>>> +allOf: >>>> + - $ref: "#/$defs/qps615-node" >>>> + >>>> +patternProperties: >>>> + "@1?[0-9a-f](,[0-7])?$": >>> >>> You have 3 ports. So isn't this fixed and limited to 0-2? >>> >> sure I will change it to below as suggested >> "@1?[0-3](,[0-1])?$" >>>> + description: child nodes describing the internal downstream ports >>>> + the qps615 switch. >>> >>> Please be consistent with starting after the ':' or on the next line. >>> >>> And start with capital C. >>> >>> >> ack >> >>>> + type: object >>>> + $ref: "#/$defs/qps615-node" >>>> + unevaluatedProperties: false >>>> + >>>> +$defs: >>>> + qps615-node: >>>> + type: object >>>> + >>>> + properties: >>>> + qcom,l0s-entry-delay-ns: >>>> + description: Aspm l0s entry delay. >>>> + >>>> + qcom,l1-entry-delay-ns: >>>> + description: Aspm l1 entry delay. >>> >>> These should probably be common being standard PCIe things. Though, why >>> are they needed? I'm sure the timing is defined by the PCIe spec, so >>> they are not compliant? >>> >> Usually the firmware in the endpoints/switches should do this these >> configurations. But the qps615 PCIe switch doesn't have any firmware >> running to configure these. So the hardware exposes i2c interface to >> configure these before link training. >>>> + >>>> + qcom,tx-amplitude-millivolt: >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>> + description: Change Tx Margin setting for low power >>>> consumption. >>>> + >>>> + qcom,no-dfe-support: >>>> + type: boolean >>>> + description: Disable DFE (Decision Feedback Equalizer), >>>> which mitigates >>>> + intersymbol interference and some reflections caused by >>>> impedance mismatches. >>>> + >>>> + qcom,nfts: >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>> + description: >>>> + Number of Fast Training Sequence (FTS) used during L0s to >>>> L0 exit >>>> + for bit and Symbol lock. >>> >>> Also something common. >>> >>> The problem I have with all these properties is you are using them on >>> both the upstream and downstream sides of the PCIe links. They belong in >>> either the device's node (downstream) or the bus's node (upstream). >>> >> This switch allows us to configure both upstream, downstream ports and >> also embedded Ethernet port which is internal to the switch. These >> properties are applicable for all of those. >>>> + >>>> + allOf: >>>> + - $ref: /schemas/pci/pci-bus.yaml# >>> >>> pci-pci-bridge.yaml is more specific and closer to what this device is. >>> >> I tried this now, I was getting warning saying the compatible >> /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: pcie@0,0: compatible: ['pci1179,0623'] does not contain items matching the given schema >> from schema $id: >> http://devicetree.org/schemas/pci/qcom,qps615.yaml# >> /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: pcie@0,0: Unevaluated properties are not allowed ('#address-cells', '#size-cells', 'bus-range', 'device_type', 'ranges' were unexpected) >> >> I think pci-pci-bridge is expecting the compatible string in this format >> only "pciclass,0604". >> >>>> + >>>> +unevaluatedProperties: false >>>> + >>>> +required: >>>> + - vdd18-supply >>>> + - vdd09-supply >>>> + - vddc-supply >>>> + - vddio1-supply >>>> + - vddio2-supply >>>> + - vddio18-supply >>>> + - i2c-parent >>>> + - reset-gpios >>>> + >>>> +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; >>>> + bus-range = <0x01 0xff>; >>>> + >>>> + pcie@0,0 { >>>> + compatible = "pci1179,0623"; >>>> + reg = <0x10000 0x0 0x0 0x0 0x0>; >>>> + device_type = "pci"; >>>> + #address-cells = <3>; >>>> + #size-cells = <2>; >>>> + ranges; >>>> + bus-range = <0x02 0xff>; >>>> + >>>> + i2c-parent = <&qup_i2c 0x77>; >>>> + >>>> + 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; >>>> + bus-range = <0x03 0xff>; >>>> + >>>> + qcom,no-dfe-support; >>>> + }; >>>> + >>>> + pcie@2,0 { >>>> + reg = <0x21000 0x0 0x0 0x0 0x0>; >>>> + #address-cells = <3>; >>>> + #size-cells = <2>; >>>> + device_type = "pci"; >>>> + ranges; >>>> + bus-range = <0x04 0xff>; >>>> + >>>> + qcom,nfts = <10>; >>>> + }; >>>> + >>>> + pcie@3,0 { >>>> + reg = <0x21800 0x0 0x0 0x0 0x0>; >>>> + #address-cells = <3>; >>>> + #size-cells = <2>; >>>> + device_type = "pci"; >>>> + ranges; >>>> + bus-range = <0x05 0xff>; >>>> + >>>> + qcom,tx-amplitude-millivolt = <10>; >>>> + pcie@0,0 { >>>> + reg = <0x50000 0x0 0x0 0x0 0x0>; >>>> + #address-cells = <3>; >>>> + #size-cells = <2>; >>>> + device_type = "pci"; >>> >>> There's a 2nd PCI-PCI bridge? >> This the embedded ethernet port which is as part of DSP3. >> > Hi Rob, > > Can you please check my response on your queries, if you need > any extra information, we can provide to sort this out. > > - Krishna Chaitanya. >> - Krishna Chaitanya A gentle ping as its been more than 2 weeks. - Krishna Chaitanya. . >>> >>>> + ranges; >>>> + >>>> + qcom,l1-entry-delay-ns = <10>; >>>> + }; >>>> + >>>> + pcie@0,1 { >>>> + reg = <0x50100 0x0 0x0 0x0 0x0>; >>>> + #address-cells = <3>; >>>> + #size-cells = <2>; >>>> + device_type = "pci"; >>>> + ranges; >>>> + >>>> + qcom,l0s-entry-delay-ns = <10>; >>>> + }; >>>> + }; >>>> + }; >>>> + }; >>>> + }; >>>> >>>> -- >>>> 2.34.1 >>>> >> >
On 12/11/2024 11:30 AM, Manivannan Sadhasivam wrote: > On Wed, Dec 04, 2024 at 03:25:59PM -0600, Bjorn Helgaas wrote: >> On Tue, Nov 12, 2024 at 08:31:33PM +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. >> >>> +$defs: >>> + qps615-node: >>> + type: object >>> + >>> + properties: >>> + qcom,l0s-entry-delay-ns: >>> + description: Aspm l0s entry delay. >>> + >>> + qcom,l1-entry-delay-ns: >>> + description: Aspm l1 entry delay. >> >> To match spec usage: >> s/Aspm/ASPM/ >> s/l0s/L0s/ >> s/l1/L1/ >> >> Other than the fact that qps615 needs the driver to configure these, >> there's nothing qcom-specific here, so I suggest the names should omit >> "qcom" and include "aspm". >> > > In that case, these properties should be documented in dt-schema: > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-bus-common.yaml > > - Mani I am fine to move to pci-bus-common.yaml but currently these are being used by qps615 only I hope that is fine. - Krishna Chaitanya. >
On Sun, Nov 24, 2024 at 07:02:48AM +0530, Krishna Chaitanya Chundru wrote: > > > On 11/15/2024 9:48 PM, Rob Herring wrote: > > On Tue, Nov 12, 2024 at 08:31:33PM +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 | 205 +++++++++++++++++++++ > > > 1 file changed, 205 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..e6a63a0bb0f3 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > > > @@ -0,0 +1,205 @@ > > > +# 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 > > > + > > > + i2c-parent: > > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > > + description: | > > > > Don't need '|' if no formatting to preserve. > > > ack > > > + A phandle to the parent I2C node and the slave address of the device > > > + used to do configure qps615 to change FTS, tx amplitude etc. > > > + items: > > > + - description: Phandle to the I2C controller node > > > + - description: I2C slave address > > > + > > > + 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. > > > > Is the PERST# or something else? > > > it is not PERST GPIO, it is similar to PERST in terms > of functionality which brings switch out from reset. Do you have an actual PERST# on upstream facing port? Is it a separate wire? Judging by the RB3 Gen2 this line is being used as PERST# > > > + > > > + qps615,axi-clk-freq-hz: > > > > qps615 is not a vendor prefix. > > > > > + description: > > > + AXI clock rate which is internal bus of the switch > > > + The switch only runs in two frequencies i.e 250MHz and 125MHz. > > > + enum: [125000000, 250000000] > > > + > > > +allOf: > > > + - $ref: "#/$defs/qps615-node" > > > + > > > +patternProperties: > > > + "@1?[0-9a-f](,[0-7])?$": > > > > You have 3 ports. So isn't this fixed and limited to 0-2? > > > sure I will change it to below as suggested > "@1?[0-3](,[0-1])?$" Why do you still need '1?' ? > > > + description: child nodes describing the internal downstream ports > > > + the qps615 switch. > > > > Please be consistent with starting after the ':' or on the next line. > > > > And start with capital C. > > > > > ack > > > > + type: object > > > + $ref: "#/$defs/qps615-node" > > > + unevaluatedProperties: false > > > + > > > +$defs: > > > + qps615-node: > > > + type: object > > > + > > > + properties: > > > + qcom,l0s-entry-delay-ns: > > > + description: Aspm l0s entry delay. > > > + > > > + qcom,l1-entry-delay-ns: > > > + description: Aspm l1 entry delay. > > > > These should probably be common being standard PCIe things. Though, why > > are they needed? I'm sure the timing is defined by the PCIe spec, so > > they are not compliant? > > > Usually the firmware in the endpoints/switches should do this these > configurations. But the qps615 PCIe switch doesn't have any firmware > running to configure these. So the hardware exposes i2c interface to > configure these before link training. If they are following the standard, why do you need to have them in the DT? Can you hardcode thos evalues in the driver? > > > + > > > + qcom,tx-amplitude-millivolt: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + description: Change Tx Margin setting for low power consumption. > > > + > > > + qcom,no-dfe-support: > > > + type: boolean > > > + description: Disable DFE (Decision Feedback Equalizer), which mitigates > > > + intersymbol interference and some reflections caused by impedance mismatches. > > > + > > > + qcom,nfts: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + description: > > > + Number of Fast Training Sequence (FTS) used during L0s to L0 exit > > > + for bit and Symbol lock. > > > > Also something common. > > > > The problem I have with all these properties is you are using them on > > both the upstream and downstream sides of the PCIe links. They belong in > > either the device's node (downstream) or the bus's node (upstream). > > > This switch allows us to configure both upstream, downstream ports and > also embedded Ethernet port which is internal to the switch. These > properties are applicable for all of those. > > > + > > > + allOf: > > > + - $ref: /schemas/pci/pci-bus.yaml# > > > > pci-pci-bridge.yaml is more specific and closer to what this device is. > > > I tried this now, I was getting warning saying the compatible > /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: > pcie@0,0: compatible: ['pci1179,0623'] does not contain items matching the > given schema > from schema $id: http://devicetree.org/schemas/pci/qcom,qps615.yaml# > /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: > pcie@0,0: Unevaluated properties are not allowed ('#address-cells', > '#size-cells', 'bus-range', 'device_type', 'ranges' were unexpected) > > I think pci-pci-bridge is expecting the compatible string in this format > only "pciclass,0604". I think the pci-pci-bridge schema requires to have "pciclass,0604" among other compatibles. So you should be able to do something like: compatible = "pci1179,0623", "pciclass,0604"; At least if follows PCI Bus Binding to Open Firmware document. > > > > + > > > +unevaluatedProperties: false > > > + > > > +required: > > > + - vdd18-supply > > > + - vdd09-supply > > > + - vddc-supply > > > + - vddio1-supply > > > + - vddio2-supply > > > + - vddio18-supply > > > + - i2c-parent > > > + - reset-gpios > > > + > > > +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; > > > + bus-range = <0x01 0xff>; > > > + > > > + pcie@0,0 { > > > + compatible = "pci1179,0623"; > > > + reg = <0x10000 0x0 0x0 0x0 0x0>; > > > + device_type = "pci"; > > > + #address-cells = <3>; > > > + #size-cells = <2>; > > > + ranges; > > > + bus-range = <0x02 0xff>; > > > + > > > + i2c-parent = <&qup_i2c 0x77>; > > > + > > > + 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; > > > + bus-range = <0x03 0xff>; > > > + > > > + qcom,no-dfe-support; > > > + }; > > > + > > > + pcie@2,0 { > > > + reg = <0x21000 0x0 0x0 0x0 0x0>; > > > + #address-cells = <3>; > > > + #size-cells = <2>; > > > + device_type = "pci"; > > > + ranges; > > > + bus-range = <0x04 0xff>; > > > + > > > + qcom,nfts = <10>; > > > + }; > > > + > > > + pcie@3,0 { > > > + reg = <0x21800 0x0 0x0 0x0 0x0>; > > > + #address-cells = <3>; > > > + #size-cells = <2>; > > > + device_type = "pci"; > > > + ranges; > > > + bus-range = <0x05 0xff>; > > > + > > > + qcom,tx-amplitude-millivolt = <10>; > > > + pcie@0,0 { > > > + reg = <0x50000 0x0 0x0 0x0 0x0>; > > > + #address-cells = <3>; > > > + #size-cells = <2>; > > > + device_type = "pci"; > > > > There's a 2nd PCI-PCI bridge? > This the embedded ethernet port which is as part of DSP3. So is there an adidtional bus for that ethernet device? > > - Krishna Chaitanya. > > > > > + ranges; > > > + > > > + qcom,l1-entry-delay-ns = <10>; > > > + }; > > > + > > > + pcie@0,1 { > > > + reg = <0x50100 0x0 0x0 0x0 0x0>; > > > + #address-cells = <3>; > > > + #size-cells = <2>; > > > + device_type = "pci"; > > > + ranges; > > > + > > > + qcom,l0s-entry-delay-ns = <10>; > > > + }; What is this? > > > + }; > > > + }; > > > + }; > > > + }; > > > > > > -- > > > 2.34.1 > > >
On Mon, Dec 23, 2024 at 10:18:24PM +0530, Krishna Chaitanya Chundru wrote: > > > On 12/11/2024 11:30 AM, Manivannan Sadhasivam wrote: > > On Wed, Dec 04, 2024 at 03:25:59PM -0600, Bjorn Helgaas wrote: > > > On Tue, Nov 12, 2024 at 08:31:33PM +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. > > > > > > > +$defs: > > > > + qps615-node: > > > > + type: object > > > > + > > > > + properties: > > > > + qcom,l0s-entry-delay-ns: > > > > + description: Aspm l0s entry delay. > > > > + > > > > + qcom,l1-entry-delay-ns: > > > > + description: Aspm l1 entry delay. > > > > > > To match spec usage: > > > s/Aspm/ASPM/ > > > s/l0s/L0s/ > > > s/l1/L1/ > > > > > > Other than the fact that qps615 needs the driver to configure these, > > > there's nothing qcom-specific here, so I suggest the names should omit > > > "qcom" and include "aspm". > > > > > > > In that case, these properties should be documented in dt-schema: > > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-bus-common.yaml > > > > - Mani > I am fine to move to pci-bus-common.yaml but currently these are being used > by qps615 only I hope that is fine. With bindings there is no such thing as "currently". Once defined they become an ABI and must not be changed.
On 12/24/2024 12:27 AM, Dmitry Baryshkov wrote: > On Sun, Nov 24, 2024 at 07:02:48AM +0530, Krishna Chaitanya Chundru wrote: >> >> >> On 11/15/2024 9:48 PM, Rob Herring wrote: >>> On Tue, Nov 12, 2024 at 08:31:33PM +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 | 205 +++++++++++++++++++++ >>>> 1 file changed, 205 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..e6a63a0bb0f3 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >>>> @@ -0,0 +1,205 @@ >>>> +# 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 >>>> + >>>> + i2c-parent: >>>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>>> + description: | >>> >>> Don't need '|' if no formatting to preserve. >>> >> ack >>>> + A phandle to the parent I2C node and the slave address of the device >>>> + used to do configure qps615 to change FTS, tx amplitude etc. >>>> + items: >>>> + - description: Phandle to the I2C controller node >>>> + - description: I2C slave address >>>> + >>>> + 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. >>> >>> Is the PERST# or something else? >>> >> it is not PERST GPIO, it is similar to PERST in terms >> of functionality which brings switch out from reset. > > Do you have an actual PERST# on upstream facing port? Is it a separate > wire? Judging by the RB3 Gen2 this line is being used as PERST# > we had PERST# as a separate line. It has two inputs one is PERST# & other one is RESX# gpio functionality wise both are similar. >>>> + >>>> + qps615,axi-clk-freq-hz: >>> >>> qps615 is not a vendor prefix. >>> >>>> + description: >>>> + AXI clock rate which is internal bus of the switch >>>> + The switch only runs in two frequencies i.e 250MHz and 125MHz. >>>> + enum: [125000000, 250000000] >>>> + >>>> +allOf: >>>> + - $ref: "#/$defs/qps615-node" >>>> + >>>> +patternProperties: >>>> + "@1?[0-9a-f](,[0-7])?$": >>> >>> You have 3 ports. So isn't this fixed and limited to 0-2? >>> >> sure I will change it to below as suggested >> "@1?[0-3](,[0-1])?$" > > Why do you still need '1?' ? > we want to represent integrated ethernet MAC also here and to represent it we need '1' as it is multi function device. I will update the description to reflect the same. >>>> + description: child nodes describing the internal downstream ports >>>> + the qps615 switch. >>> >>> Please be consistent with starting after the ':' or on the next line. >>> >>> And start with capital C. >>> >>> >> ack >> >>>> + type: object >>>> + $ref: "#/$defs/qps615-node" >>>> + unevaluatedProperties: false >>>> + >>>> +$defs: >>>> + qps615-node: >>>> + type: object >>>> + >>>> + properties: >>>> + qcom,l0s-entry-delay-ns: >>>> + description: Aspm l0s entry delay. >>>> + >>>> + qcom,l1-entry-delay-ns: >>>> + description: Aspm l1 entry delay. >>> >>> These should probably be common being standard PCIe things. Though, why >>> are they needed? I'm sure the timing is defined by the PCIe spec, so >>> they are not compliant? >>> >> Usually the firmware in the endpoints/switches should do this these >> configurations. But the qps615 PCIe switch doesn't have any firmware >> running to configure these. So the hardware exposes i2c interface to >> configure these before link training. > > If they are following the standard, why do you need to have them in the > DT? Can you hardcode thos evalues in the driver? > These values can be changed from platform to platform based upon the different power goals and latency requirements so we can't have hard coded values. And even DWC controllers also provide provision to change these values currently we are not using them. As bjorn suggested if we move these to common pcie bindings these can be used in future by controller drivers also. >>>> + >>>> + qcom,tx-amplitude-millivolt: >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>> + description: Change Tx Margin setting for low power consumption. >>>> + >>>> + qcom,no-dfe-support: >>>> + type: boolean >>>> + description: Disable DFE (Decision Feedback Equalizer), which mitigates >>>> + intersymbol interference and some reflections caused by impedance mismatches. >>>> + >>>> + qcom,nfts: >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>> + description: >>>> + Number of Fast Training Sequence (FTS) used during L0s to L0 exit >>>> + for bit and Symbol lock. >>> >>> Also something common. >>> >>> The problem I have with all these properties is you are using them on >>> both the upstream and downstream sides of the PCIe links. They belong in >>> either the device's node (downstream) or the bus's node (upstream). >>> >> This switch allows us to configure both upstream, downstream ports and >> also embedded Ethernet port which is internal to the switch. These >> properties are applicable for all of those. >>>> + >>>> + allOf: >>>> + - $ref: /schemas/pci/pci-bus.yaml# >>> >>> pci-pci-bridge.yaml is more specific and closer to what this device is. >>> >> I tried this now, I was getting warning saying the compatible >> /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: >> pcie@0,0: compatible: ['pci1179,0623'] does not contain items matching the >> given schema >> from schema $id: http://devicetree.org/schemas/pci/qcom,qps615.yaml# >> /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: >> pcie@0,0: Unevaluated properties are not allowed ('#address-cells', >> '#size-cells', 'bus-range', 'device_type', 'ranges' were unexpected) >> >> I think pci-pci-bridge is expecting the compatible string in this format >> only "pciclass,0604". > > I think the pci-pci-bridge schema requires to have "pciclass,0604" among > other compatibles. So you should be able to do something like: > > compatible = "pci1179,0623", "pciclass,0604"; > > At least if follows PCI Bus Binding to Open Firmware document. > let us try this and come back. >> >>>> + >>>> +unevaluatedProperties: false >>>> + >>>> +required: >>>> + - vdd18-supply >>>> + - vdd09-supply >>>> + - vddc-supply >>>> + - vddio1-supply >>>> + - vddio2-supply >>>> + - vddio18-supply >>>> + - i2c-parent >>>> + - reset-gpios >>>> + >>>> +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; >>>> + bus-range = <0x01 0xff>; >>>> + >>>> + pcie@0,0 { >>>> + compatible = "pci1179,0623"; >>>> + reg = <0x10000 0x0 0x0 0x0 0x0>; >>>> + device_type = "pci"; >>>> + #address-cells = <3>; >>>> + #size-cells = <2>; >>>> + ranges; >>>> + bus-range = <0x02 0xff>; >>>> + >>>> + i2c-parent = <&qup_i2c 0x77>; >>>> + >>>> + 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; >>>> + bus-range = <0x03 0xff>; >>>> + >>>> + qcom,no-dfe-support; >>>> + }; >>>> + >>>> + pcie@2,0 { >>>> + reg = <0x21000 0x0 0x0 0x0 0x0>; >>>> + #address-cells = <3>; >>>> + #size-cells = <2>; >>>> + device_type = "pci"; >>>> + ranges; >>>> + bus-range = <0x04 0xff>; >>>> + >>>> + qcom,nfts = <10>; >>>> + }; >>>> + >>>> + pcie@3,0 { >>>> + reg = <0x21800 0x0 0x0 0x0 0x0>; >>>> + #address-cells = <3>; >>>> + #size-cells = <2>; >>>> + device_type = "pci"; >>>> + ranges; >>>> + bus-range = <0x05 0xff>; >>>> + >>>> + qcom,tx-amplitude-millivolt = <10>; >>>> + pcie@0,0 { >>>> + reg = <0x50000 0x0 0x0 0x0 0x0>; >>>> + #address-cells = <3>; >>>> + #size-cells = <2>; >>>> + device_type = "pci"; >>> >>> There's a 2nd PCI-PCI bridge? >> This the embedded ethernet port which is as part of DSP3. > > So is there an adidtional bus for that ethernet device? > yes for ethernet it has aditional bus assigned. >> >> - Krishna Chaitanya. >>> >>>> + ranges; >>>> + >>>> + qcom,l1-entry-delay-ns = <10>; >>>> + }; >>>> + >>>> + pcie@0,1 { >>>> + reg = <0x50100 0x0 0x0 0x0 0x0>; >>>> + #address-cells = <3>; >>>> + #size-cells = <2>; >>>> + device_type = "pci"; >>>> + ranges; >>>> + >>>> + qcom,l0s-entry-delay-ns = <10>; >>>> + }; > > What is this? > Ethernet endpoint is a multi function device which has 2 functions This node represents 2nd node. I will update the description to reflect the same. - Krishna Chaitanya. >>>> + }; >>>> + }; >>>> + }; >>>> + }; >>>> >>>> -- >>>> 2.34.1 >>>> >
On Tue, Dec 24, 2024 at 11:34:10AM +0530, Krishna Chaitanya Chundru wrote: > > > On 12/24/2024 12:27 AM, Dmitry Baryshkov wrote: > > On Sun, Nov 24, 2024 at 07:02:48AM +0530, Krishna Chaitanya Chundru wrote: > > > > > > > > > On 11/15/2024 9:48 PM, Rob Herring wrote: > > > > On Tue, Nov 12, 2024 at 08:31:33PM +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 | 205 +++++++++++++++++++++ > > > > > 1 file changed, 205 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..e6a63a0bb0f3 > > > > > --- /dev/null > > > > > +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > > > > > @@ -0,0 +1,205 @@ > > > > > +# 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 > > > > > + > > > > > + i2c-parent: > > > > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > > > > + description: | > > > > > > > > Don't need '|' if no formatting to preserve. > > > > > > > ack > > > > > + A phandle to the parent I2C node and the slave address of the device > > > > > + used to do configure qps615 to change FTS, tx amplitude etc. > > > > > + items: > > > > > + - description: Phandle to the I2C controller node > > > > > + - description: I2C slave address > > > > > + > > > > > + 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. > > > > > > > > Is the PERST# or something else? > > > > > > > it is not PERST GPIO, it is similar to PERST in terms > > > of functionality which brings switch out from reset. > > > > Do you have an actual PERST# on upstream facing port? Is it a separate > > wire? Judging by the RB3 Gen2 this line is being used as PERST# > > > we had PERST# as a separate line. It has two inputs one is PERST# & > other one is RESX# gpio functionality wise both are similar. I don't think I follow. Are you describing the QPS615 side or the SoC side? > > > > > + > > > > > + qps615,axi-clk-freq-hz: > > > > > > > > qps615 is not a vendor prefix. > > > > > > > > > + description: > > > > > + AXI clock rate which is internal bus of the switch > > > > > + The switch only runs in two frequencies i.e 250MHz and 125MHz. > > > > > + enum: [125000000, 250000000] > > > > > + > > > > > +allOf: > > > > > + - $ref: "#/$defs/qps615-node" > > > > > + > > > > > +patternProperties: > > > > > + "@1?[0-9a-f](,[0-7])?$": > > > > > > > > You have 3 ports. So isn't this fixed and limited to 0-2? > > > > > > > sure I will change it to below as suggested > > > "@1?[0-3](,[0-1])?$" > > > > Why do you still need '1?' ? > > > we want to represent integrated ethernet MAC also here and to represent it > we need '1' as it is multi function device. I will update the > description to reflect the same. Note, I has asked about the '1?' part, not about the [0-1]. However as you've mentioned it, you are describing the first level subnodes. Per your example, these subnodes are "pcie@1,0", "pcie@2,0" and "pcie@3,0". Thus this patternProperties should have the regexp of "^pcie@[1-3],0$". The multifunction devices for the ethernet node are hidden under the pcie@3,0 and as such they are not being matched against this regexp. > > > > > + description: child nodes describing the internal downstream ports > > > > > + the qps615 switch. > > > > > > > > Please be consistent with starting after the ':' or on the next line. > > > > > > > > And start with capital C. > > > > > > > > > > > ack > > > > > > > > + type: object > > > > > + $ref: "#/$defs/qps615-node" > > > > > + unevaluatedProperties: false > > > > > + > > > > > +$defs: > > > > > + qps615-node: > > > > > + type: object > > > > > + > > > > > + properties: > > > > > + qcom,l0s-entry-delay-ns: > > > > > + description: Aspm l0s entry delay. > > > > > + > > > > > + qcom,l1-entry-delay-ns: > > > > > + description: Aspm l1 entry delay. > > > > > > > > These should probably be common being standard PCIe things. Though, why > > > > are they needed? I'm sure the timing is defined by the PCIe spec, so > > > > they are not compliant? > > > > > > > Usually the firmware in the endpoints/switches should do this these > > > configurations. But the qps615 PCIe switch doesn't have any firmware > > > running to configure these. So the hardware exposes i2c interface to > > > configure these before link training. > > > > If they are following the standard, why do you need to have them in the > > DT? Can you hardcode thos evalues in the driver? > > > These values can be changed from platform to platform based upon the > different power goals and latency requirements so we can't have hard coded > values. > > And even DWC controllers also provide provision to change these values > currently we are not using them. As bjorn suggested if we move these to > common pcie bindings these can be used in future by controller drivers also. Ack > > > > > + > > > > > + qcom,tx-amplitude-millivolt: > > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > > + description: Change Tx Margin setting for low power consumption. > > > > > + > > > > > + qcom,no-dfe-support: > > > > > + type: boolean > > > > > + description: Disable DFE (Decision Feedback Equalizer), which mitigates > > > > > + intersymbol interference and some reflections caused by impedance mismatches. > > > > > + > > > > > + qcom,nfts: > > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > > + description: > > > > > + Number of Fast Training Sequence (FTS) used during L0s to L0 exit > > > > > + for bit and Symbol lock. > > > > > > > > Also something common. > > > > > > > > The problem I have with all these properties is you are using them on > > > > both the upstream and downstream sides of the PCIe links. They belong in > > > > either the device's node (downstream) or the bus's node (upstream). > > > > > > > This switch allows us to configure both upstream, downstream ports and > > > also embedded Ethernet port which is internal to the switch. These > > > properties are applicable for all of those. > > > > > + > > > > > + allOf: > > > > > + - $ref: /schemas/pci/pci-bus.yaml# > > > > > > > > pci-pci-bridge.yaml is more specific and closer to what this device is. > > > > > > > I tried this now, I was getting warning saying the compatible > > > /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: > > > pcie@0,0: compatible: ['pci1179,0623'] does not contain items matching the > > > given schema > > > from schema $id: http://devicetree.org/schemas/pci/qcom,qps615.yaml# > > > /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: > > > pcie@0,0: Unevaluated properties are not allowed ('#address-cells', > > > '#size-cells', 'bus-range', 'device_type', 'ranges' were unexpected) > > > > > > I think pci-pci-bridge is expecting the compatible string in this format > > > only "pciclass,0604". > > > > I think the pci-pci-bridge schema requires to have "pciclass,0604" among > > other compatibles. So you should be able to do something like: > > > > compatible = "pci1179,0623", "pciclass,0604"; > > > > At least if follows PCI Bus Binding to Open Firmware document. > > > let us try this and come back. > > > > > > > > + > > > > > +unevaluatedProperties: false > > > > > + > > > > > +required: > > > > > + - vdd18-supply > > > > > + - vdd09-supply > > > > > + - vddc-supply > > > > > + - vddio1-supply > > > > > + - vddio2-supply > > > > > + - vddio18-supply > > > > > + - i2c-parent > > > > > + - reset-gpios > > > > > + > > > > > +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; > > > > > + bus-range = <0x01 0xff>; > > > > > + > > > > > + pcie@0,0 { > > > > > + compatible = "pci1179,0623"; > > > > > + reg = <0x10000 0x0 0x0 0x0 0x0>; > > > > > + device_type = "pci"; > > > > > + #address-cells = <3>; > > > > > + #size-cells = <2>; > > > > > + ranges; > > > > > + bus-range = <0x02 0xff>; > > > > > + > > > > > + i2c-parent = <&qup_i2c 0x77>; > > > > > + > > > > > + 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; > > > > > + bus-range = <0x03 0xff>; > > > > > + > > > > > + qcom,no-dfe-support; > > > > > + }; > > > > > + > > > > > + pcie@2,0 { > > > > > + reg = <0x21000 0x0 0x0 0x0 0x0>; > > > > > + #address-cells = <3>; > > > > > + #size-cells = <2>; > > > > > + device_type = "pci"; > > > > > + ranges; > > > > > + bus-range = <0x04 0xff>; > > > > > + > > > > > + qcom,nfts = <10>; > > > > > + }; > > > > > + > > > > > + pcie@3,0 { > > > > > + reg = <0x21800 0x0 0x0 0x0 0x0>; > > > > > + #address-cells = <3>; > > > > > + #size-cells = <2>; > > > > > + device_type = "pci"; > > > > > + ranges; > > > > > + bus-range = <0x05 0xff>; > > > > > + > > > > > + qcom,tx-amplitude-millivolt = <10>; > > > > > + pcie@0,0 { > > > > > + reg = <0x50000 0x0 0x0 0x0 0x0>; > > > > > + #address-cells = <3>; > > > > > + #size-cells = <2>; > > > > > + device_type = "pci"; > > > > > > > > There's a 2nd PCI-PCI bridge? > > > This the embedded ethernet port which is as part of DSP3. > > > > So is there an adidtional bus for that ethernet device? > > > yes for ethernet it has aditional bus assigned. > > > > > > > - Krishna Chaitanya. > > > > > > > > > + ranges; > > > > > + > > > > > + qcom,l1-entry-delay-ns = <10>; > > > > > + }; > > > > > + > > > > > + pcie@0,1 { > > > > > + reg = <0x50100 0x0 0x0 0x0 0x0>; > > > > > + #address-cells = <3>; > > > > > + #size-cells = <2>; > > > > > + device_type = "pci"; > > > > > + ranges; > > > > > + > > > > > + qcom,l0s-entry-delay-ns = <10>; > > > > > + }; > > > > What is this? > > > Ethernet endpoint is a multi function device which has 2 functions > This node represents 2nd node. I will update the description to > reflect the same. If this is an ethernet device, why does it have a name of pcie@? Per bindings the pcie@ name should be used only for devices with the class 0604. Whas is the PCI device class for those devices? I think ethernet@ (0200) should probably be the best fit, judgin by your description. > > - Krishna Chaitanya. > > > > > > + }; > > > > > + }; > > > > > + }; > > > > > + }; > > > > > > > > > > -- > > > > > 2.34.1 > > > > > > >
On 12/24/2024 12:24 PM, Dmitry Baryshkov wrote: > On Tue, Dec 24, 2024 at 11:34:10AM +0530, Krishna Chaitanya Chundru wrote: >> >> >> On 12/24/2024 12:27 AM, Dmitry Baryshkov wrote: >>> On Sun, Nov 24, 2024 at 07:02:48AM +0530, Krishna Chaitanya Chundru wrote: >>>> >>>> >>>> On 11/15/2024 9:48 PM, Rob Herring wrote: >>>>> On Tue, Nov 12, 2024 at 08:31:33PM +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 | 205 +++++++++++++++++++++ >>>>>> 1 file changed, 205 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..e6a63a0bb0f3 >>>>>> --- /dev/null >>>>>> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml >>>>>> @@ -0,0 +1,205 @@ >>>>>> +# 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 >>>>>> + >>>>>> + i2c-parent: >>>>>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>>>>> + description: | >>>>> >>>>> Don't need '|' if no formatting to preserve. >>>>> >>>> ack >>>>>> + A phandle to the parent I2C node and the slave address of the device >>>>>> + used to do configure qps615 to change FTS, tx amplitude etc. >>>>>> + items: >>>>>> + - description: Phandle to the I2C controller node >>>>>> + - description: I2C slave address >>>>>> + >>>>>> + 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. >>>>> >>>>> Is the PERST# or something else? >>>>> >>>> it is not PERST GPIO, it is similar to PERST in terms >>>> of functionality which brings switch out from reset. >>> >>> Do you have an actual PERST# on upstream facing port? Is it a separate >>> wire? Judging by the RB3 Gen2 this line is being used as PERST# >>> >> we had PERST# as a separate line. It has two inputs one is PERST# & >> other one is RESX# gpio functionality wise both are similar. > > I don't think I follow. Are you describing the QPS615 side or the SoC side? > These are QPS615 side. it has both PERST# as per PCIe spec and RESX# which belongs to only qps615 to bring out of reset. There are two different GPIO lines. >>>>>> + >>>>>> + qps615,axi-clk-freq-hz: >>>>> >>>>> qps615 is not a vendor prefix. >>>>> >>>>>> + description: >>>>>> + AXI clock rate which is internal bus of the switch >>>>>> + The switch only runs in two frequencies i.e 250MHz and 125MHz. >>>>>> + enum: [125000000, 250000000] >>>>>> + >>>>>> +allOf: >>>>>> + - $ref: "#/$defs/qps615-node" >>>>>> + >>>>>> +patternProperties: >>>>>> + "@1?[0-9a-f](,[0-7])?$": >>>>> >>>>> You have 3 ports. So isn't this fixed and limited to 0-2? >>>>> >>>> sure I will change it to below as suggested >>>> "@1?[0-3](,[0-1])?$" >>> >>> Why do you still need '1?' ? >>> >> we want to represent integrated ethernet MAC also here and to represent it >> we need '1' as it is multi function device. I will update the >> description to reflect the same. > > Note, I has asked about the '1?' part, not about the [0-1]. > > However as you've mentioned it, you are describing the first level > subnodes. Per your example, these subnodes are "pcie@1,0", "pcie@2,0" > and "pcie@3,0". Thus this patternProperties should have the regexp of > "^pcie@[1-3],0$". The multifunction devices for the ethernet node are > hidden under the pcie@3,0 and as such they are not being matched against > this regexp. > ack. I will use as the suggested one. >>>>>> + description: child nodes describing the internal downstream ports >>>>>> + the qps615 switch. >>>>> >>>>> Please be consistent with starting after the ':' or on the next line. >>>>> >>>>> And start with capital C. >>>>> >>>>> >>>> ack >>>> >>>>>> + type: object >>>>>> + $ref: "#/$defs/qps615-node" >>>>>> + unevaluatedProperties: false >>>>>> + >>>>>> +$defs: >>>>>> + qps615-node: >>>>>> + type: object >>>>>> + >>>>>> + properties: >>>>>> + qcom,l0s-entry-delay-ns: >>>>>> + description: Aspm l0s entry delay. >>>>>> + >>>>>> + qcom,l1-entry-delay-ns: >>>>>> + description: Aspm l1 entry delay. >>>>> >>>>> These should probably be common being standard PCIe things. Though, why >>>>> are they needed? I'm sure the timing is defined by the PCIe spec, so >>>>> they are not compliant? >>>>> >>>> Usually the firmware in the endpoints/switches should do this these >>>> configurations. But the qps615 PCIe switch doesn't have any firmware >>>> running to configure these. So the hardware exposes i2c interface to >>>> configure these before link training. >>> >>> If they are following the standard, why do you need to have them in the >>> DT? Can you hardcode thos evalues in the driver? >>> >> These values can be changed from platform to platform based upon the >> different power goals and latency requirements so we can't have hard coded >> values. >> >> And even DWC controllers also provide provision to change these values >> currently we are not using them. As bjorn suggested if we move these to >> common pcie bindings these can be used in future by controller drivers also. > > Ack > >>>>>> + >>>>>> + qcom,tx-amplitude-millivolt: >>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>>> + description: Change Tx Margin setting for low power consumption. >>>>>> + >>>>>> + qcom,no-dfe-support: >>>>>> + type: boolean >>>>>> + description: Disable DFE (Decision Feedback Equalizer), which mitigates >>>>>> + intersymbol interference and some reflections caused by impedance mismatches. >>>>>> + >>>>>> + qcom,nfts: >>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>>> + description: >>>>>> + Number of Fast Training Sequence (FTS) used during L0s to L0 exit >>>>>> + for bit and Symbol lock. >>>>> >>>>> Also something common. >>>>> >>>>> The problem I have with all these properties is you are using them on >>>>> both the upstream and downstream sides of the PCIe links. They belong in >>>>> either the device's node (downstream) or the bus's node (upstream). >>>>> >>>> This switch allows us to configure both upstream, downstream ports and >>>> also embedded Ethernet port which is internal to the switch. These >>>> properties are applicable for all of those. >>>>>> + >>>>>> + allOf: >>>>>> + - $ref: /schemas/pci/pci-bus.yaml# >>>>> >>>>> pci-pci-bridge.yaml is more specific and closer to what this device is. >>>>> >>>> I tried this now, I was getting warning saying the compatible >>>> /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: >>>> pcie@0,0: compatible: ['pci1179,0623'] does not contain items matching the >>>> given schema >>>> from schema $id: http://devicetree.org/schemas/pci/qcom,qps615.yaml# >>>> /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: >>>> pcie@0,0: Unevaluated properties are not allowed ('#address-cells', >>>> '#size-cells', 'bus-range', 'device_type', 'ranges' were unexpected) >>>> >>>> I think pci-pci-bridge is expecting the compatible string in this format >>>> only "pciclass,0604". >>> >>> I think the pci-pci-bridge schema requires to have "pciclass,0604" among >>> other compatibles. So you should be able to do something like: >>> >>> compatible = "pci1179,0623", "pciclass,0604"; >>> >>> At least if follows PCI Bus Binding to Open Firmware document. >>> >> let us try this and come back. >>>> >>>>>> + >>>>>> +unevaluatedProperties: false >>>>>> + >>>>>> +required: >>>>>> + - vdd18-supply >>>>>> + - vdd09-supply >>>>>> + - vddc-supply >>>>>> + - vddio1-supply >>>>>> + - vddio2-supply >>>>>> + - vddio18-supply >>>>>> + - i2c-parent >>>>>> + - reset-gpios >>>>>> + >>>>>> +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; >>>>>> + bus-range = <0x01 0xff>; >>>>>> + >>>>>> + pcie@0,0 { >>>>>> + compatible = "pci1179,0623"; >>>>>> + reg = <0x10000 0x0 0x0 0x0 0x0>; >>>>>> + device_type = "pci"; >>>>>> + #address-cells = <3>; >>>>>> + #size-cells = <2>; >>>>>> + ranges; >>>>>> + bus-range = <0x02 0xff>; >>>>>> + >>>>>> + i2c-parent = <&qup_i2c 0x77>; >>>>>> + >>>>>> + 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; >>>>>> + bus-range = <0x03 0xff>; >>>>>> + >>>>>> + qcom,no-dfe-support; >>>>>> + }; >>>>>> + >>>>>> + pcie@2,0 { >>>>>> + reg = <0x21000 0x0 0x0 0x0 0x0>; >>>>>> + #address-cells = <3>; >>>>>> + #size-cells = <2>; >>>>>> + device_type = "pci"; >>>>>> + ranges; >>>>>> + bus-range = <0x04 0xff>; >>>>>> + >>>>>> + qcom,nfts = <10>; >>>>>> + }; >>>>>> + >>>>>> + pcie@3,0 { >>>>>> + reg = <0x21800 0x0 0x0 0x0 0x0>; >>>>>> + #address-cells = <3>; >>>>>> + #size-cells = <2>; >>>>>> + device_type = "pci"; >>>>>> + ranges; >>>>>> + bus-range = <0x05 0xff>; >>>>>> + >>>>>> + qcom,tx-amplitude-millivolt = <10>; >>>>>> + pcie@0,0 { >>>>>> + reg = <0x50000 0x0 0x0 0x0 0x0>; >>>>>> + #address-cells = <3>; >>>>>> + #size-cells = <2>; >>>>>> + device_type = "pci"; >>>>> >>>>> There's a 2nd PCI-PCI bridge? >>>> This the embedded ethernet port which is as part of DSP3. >>> >>> So is there an adidtional bus for that ethernet device? >>> >> yes for ethernet it has aditional bus assigned. >> >>>> >>>> - Krishna Chaitanya. >>>>> >>>>>> + ranges; >>>>>> + >>>>>> + qcom,l1-entry-delay-ns = <10>; >>>>>> + }; >>>>>> + >>>>>> + pcie@0,1 { >>>>>> + reg = <0x50100 0x0 0x0 0x0 0x0>; >>>>>> + #address-cells = <3>; >>>>>> + #size-cells = <2>; >>>>>> + device_type = "pci"; >>>>>> + ranges; >>>>>> + >>>>>> + qcom,l0s-entry-delay-ns = <10>; >>>>>> + }; >>> >>> What is this? >>> >> Ethernet endpoint is a multi function device which has 2 functions >> This node represents 2nd node. I will update the description to >> reflect the same. > > If this is an ethernet device, why does it have a name of pcie@? Per > bindings the pcie@ name should be used only for devices with the class > 0604. Whas is the PCI device class for those devices? I think ethernet@ > (0200) should probably be the best fit, judgin by your description. > These are PCIe Ethernet endpoints with base class 2 & subclass 2, I taught all the PCIe devices should start with pcie irrespective of the class, can you point us the binding you are referring to. I was referring to this https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-device.yaml#L23 As this is a endpoint device, I will cross check once if we can represent here or not and get back on this. - Krishna Chaitanya. >> >> - Krishna Chaitanya. >> >>>>>> + }; >>>>>> + }; >>>>>> + }; >>>>>> + }; >>>>>> >>>>>> -- >>>>>> 2.34.1 >>>>>> >>> >
On 12/5/2024 2:55 AM, Bjorn Helgaas wrote: > On Tue, Nov 12, 2024 at 08:31:33PM +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. > >> +$defs: >> + qps615-node: >> + type: object >> + >> + properties: >> + qcom,l0s-entry-delay-ns: >> + description: Aspm l0s entry delay. >> + >> + qcom,l1-entry-delay-ns: >> + description: Aspm l1 entry delay. > > To match spec usage: > s/Aspm/ASPM/ > s/l0s/L0s/ > s/l1/L1/ > > Other than the fact that qps615 needs the driver to configure these, > there's nothing qcom-specific here, so I suggest the names should omit > "qcom" and include "aspm". > >> + 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; >> + bus-range = <0x01 0xff>; >> + >> + pcie@0,0 { >> + compatible = "pci1179,0623"; >> + reg = <0x10000 0x0 0x0 0x0 0x0>; >> + device_type = "pci"; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + ranges; >> + bus-range = <0x02 0xff>; > > This binding describes a switch. I don't think bus-range should > appear here at all because it is not a feature of the hardware (unless > the switch ports are broken and their Secondary/Subordinate Bus > Numbers are hard-wired). > > The Primary/Secondary/Subordinate Bus Numbers of all switch ports > should be writable and the PCI core knows how to manage them. > Bjorn, The dt binding check is throwing an error if we don't keep bus-range property for that reason we added it, from dt binding perspective i think it is mandatory to add this property. - Krishna Chaitanya. > Bjorn
On Tue, Dec 24, 2024 at 02:39:17PM +0530, Krishna Chaitanya Chundru wrote: > > > On 12/24/2024 12:24 PM, Dmitry Baryshkov wrote: > > On Tue, Dec 24, 2024 at 11:34:10AM +0530, Krishna Chaitanya Chundru wrote: > > > > > > > > > On 12/24/2024 12:27 AM, Dmitry Baryshkov wrote: > > > > On Sun, Nov 24, 2024 at 07:02:48AM +0530, Krishna Chaitanya Chundru wrote: > > > > > > > > > > > > > > > On 11/15/2024 9:48 PM, Rob Herring wrote: > > > > > > On Tue, Nov 12, 2024 at 08:31:33PM +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 | 205 +++++++++++++++++++++ > > > > > > > 1 file changed, 205 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..e6a63a0bb0f3 > > > > > > > --- /dev/null > > > > > > > +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml > > > > > > > @@ -0,0 +1,205 @@ > > > > > > > +# 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 > > > > > > > + > > > > > > > + i2c-parent: > > > > > > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > > > > > > + description: | > > > > > > > > > > > > Don't need '|' if no formatting to preserve. > > > > > > > > > > > ack > > > > > > > + A phandle to the parent I2C node and the slave address of the device > > > > > > > + used to do configure qps615 to change FTS, tx amplitude etc. > > > > > > > + items: > > > > > > > + - description: Phandle to the I2C controller node > > > > > > > + - description: I2C slave address > > > > > > > + > > > > > > > + 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. > > > > > > > > > > > > Is the PERST# or something else? > > > > > > > > > > > it is not PERST GPIO, it is similar to PERST in terms > > > > > of functionality which brings switch out from reset. > > > > > > > > Do you have an actual PERST# on upstream facing port? Is it a separate > > > > wire? Judging by the RB3 Gen2 this line is being used as PERST# > > > > > > > we had PERST# as a separate line. It has two inputs one is PERST# & > > > other one is RESX# gpio functionality wise both are similar. > > > > I don't think I follow. Are you describing the QPS615 side or the SoC side? > > > These are QPS615 side. it has both PERST# as per PCIe spec and RESX# which > belongs to only qps615 to bring out of reset. There are two different GPIO > lines. Hmm, okay. I indeed see two pins, PCIE_RESET_N and RESX. PCIE_RESET_N is described as "Reset for PCIe block", while RESX is "System reset input". RESX should be lifted before PCIE_RESET_N during QPS615 power on. If I understand correctly, the reset-gpios is a RESX pin, which is controlled manually by the QPS driver. Is PERST# controlled by the DWC3 driver or by the DWC3 controller itself? > > > > > > > + > > > > > > > + qps615,axi-clk-freq-hz: > > > > > > > > > > > > qps615 is not a vendor prefix. > > > > > > > > > > > > > + description: > > > > > > > + AXI clock rate which is internal bus of the switch > > > > > > > + The switch only runs in two frequencies i.e 250MHz and 125MHz. > > > > > > > + enum: [125000000, 250000000] > > > > > > > + > > > > > > > +allOf: > > > > > > > + - $ref: "#/$defs/qps615-node" > > > > > > > + > > > > > > > +patternProperties: > > > > > > > + "@1?[0-9a-f](,[0-7])?$": > > > > > > > > > > > > You have 3 ports. So isn't this fixed and limited to 0-2? > > > > > > > > > > > sure I will change it to below as suggested > > > > > "@1?[0-3](,[0-1])?$" > > > > > > > > Why do you still need '1?' ? > > > > > > > we want to represent integrated ethernet MAC also here and to represent it > > > we need '1' as it is multi function device. I will update the > > > description to reflect the same. > > > > Note, I has asked about the '1?' part, not about the [0-1]. > > > > However as you've mentioned it, you are describing the first level > > subnodes. Per your example, these subnodes are "pcie@1,0", "pcie@2,0" > > and "pcie@3,0". Thus this patternProperties should have the regexp of > > "^pcie@[1-3],0$". The multifunction devices for the ethernet node are > > hidden under the pcie@3,0 and as such they are not being matched against > > this regexp. > > > ack. I will use as the suggested one. > > > > > > > + description: child nodes describing the internal downstream ports > > > > > > > + the qps615 switch. > > > > > > > > > > > > Please be consistent with starting after the ':' or on the next line. > > > > > > > > > > > > And start with capital C. > > > > > > > > > > > > > > > > > ack > > > > > > > > > > > > + type: object > > > > > > > + $ref: "#/$defs/qps615-node" > > > > > > > + unevaluatedProperties: false > > > > > > > + > > > > > > > +$defs: > > > > > > > + qps615-node: > > > > > > > + type: object > > > > > > > + > > > > > > > + properties: > > > > > > > + qcom,l0s-entry-delay-ns: > > > > > > > + description: Aspm l0s entry delay. > > > > > > > + > > > > > > > + qcom,l1-entry-delay-ns: > > > > > > > + description: Aspm l1 entry delay. > > > > > > > > > > > > These should probably be common being standard PCIe things. Though, why > > > > > > are they needed? I'm sure the timing is defined by the PCIe spec, so > > > > > > they are not compliant? > > > > > > > > > > > Usually the firmware in the endpoints/switches should do this these > > > > > configurations. But the qps615 PCIe switch doesn't have any firmware > > > > > running to configure these. So the hardware exposes i2c interface to > > > > > configure these before link training. > > > > > > > > If they are following the standard, why do you need to have them in the > > > > DT? Can you hardcode thos evalues in the driver? > > > > > > > These values can be changed from platform to platform based upon the > > > different power goals and latency requirements so we can't have hard coded > > > values. > > > > > > And even DWC controllers also provide provision to change these values > > > currently we are not using them. As bjorn suggested if we move these to > > > common pcie bindings these can be used in future by controller drivers also. > > > > Ack > > > > > > > > > + > > > > > > > + qcom,tx-amplitude-millivolt: > > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > > > > + description: Change Tx Margin setting for low power consumption. > > > > > > > + > > > > > > > + qcom,no-dfe-support: > > > > > > > + type: boolean > > > > > > > + description: Disable DFE (Decision Feedback Equalizer), which mitigates > > > > > > > + intersymbol interference and some reflections caused by impedance mismatches. > > > > > > > + > > > > > > > + qcom,nfts: > > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > > > > + description: > > > > > > > + Number of Fast Training Sequence (FTS) used during L0s to L0 exit > > > > > > > + for bit and Symbol lock. > > > > > > > > > > > > Also something common. > > > > > > > > > > > > The problem I have with all these properties is you are using them on > > > > > > both the upstream and downstream sides of the PCIe links. They belong in > > > > > > either the device's node (downstream) or the bus's node (upstream). > > > > > > > > > > > This switch allows us to configure both upstream, downstream ports and > > > > > also embedded Ethernet port which is internal to the switch. These > > > > > properties are applicable for all of those. > > > > > > > + > > > > > > > + allOf: > > > > > > > + - $ref: /schemas/pci/pci-bus.yaml# > > > > > > > > > > > > pci-pci-bridge.yaml is more specific and closer to what this device is. > > > > > > > > > > > I tried this now, I was getting warning saying the compatible > > > > > /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: > > > > > pcie@0,0: compatible: ['pci1179,0623'] does not contain items matching the > > > > > given schema > > > > > from schema $id: http://devicetree.org/schemas/pci/qcom,qps615.yaml# > > > > > /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: > > > > > pcie@0,0: Unevaluated properties are not allowed ('#address-cells', > > > > > '#size-cells', 'bus-range', 'device_type', 'ranges' were unexpected) > > > > > > > > > > I think pci-pci-bridge is expecting the compatible string in this format > > > > > only "pciclass,0604". > > > > > > > > I think the pci-pci-bridge schema requires to have "pciclass,0604" among > > > > other compatibles. So you should be able to do something like: > > > > > > > > compatible = "pci1179,0623", "pciclass,0604"; > > > > > > > > At least if follows PCI Bus Binding to Open Firmware document. > > > > > > > let us try this and come back. > > > > > > > > > > > > + > > > > > > > +unevaluatedProperties: false > > > > > > > + > > > > > > > +required: > > > > > > > + - vdd18-supply > > > > > > > + - vdd09-supply > > > > > > > + - vddc-supply > > > > > > > + - vddio1-supply > > > > > > > + - vddio2-supply > > > > > > > + - vddio18-supply > > > > > > > + - i2c-parent > > > > > > > + - reset-gpios > > > > > > > + > > > > > > > +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; > > > > > > > + bus-range = <0x01 0xff>; > > > > > > > + > > > > > > > + pcie@0,0 { > > > > > > > + compatible = "pci1179,0623"; > > > > > > > + reg = <0x10000 0x0 0x0 0x0 0x0>; > > > > > > > + device_type = "pci"; > > > > > > > + #address-cells = <3>; > > > > > > > + #size-cells = <2>; > > > > > > > + ranges; > > > > > > > + bus-range = <0x02 0xff>; > > > > > > > + > > > > > > > + i2c-parent = <&qup_i2c 0x77>; > > > > > > > + > > > > > > > + 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; > > > > > > > + bus-range = <0x03 0xff>; > > > > > > > + > > > > > > > + qcom,no-dfe-support; > > > > > > > + }; > > > > > > > + > > > > > > > + pcie@2,0 { > > > > > > > + reg = <0x21000 0x0 0x0 0x0 0x0>; > > > > > > > + #address-cells = <3>; > > > > > > > + #size-cells = <2>; > > > > > > > + device_type = "pci"; > > > > > > > + ranges; > > > > > > > + bus-range = <0x04 0xff>; > > > > > > > + > > > > > > > + qcom,nfts = <10>; > > > > > > > + }; > > > > > > > + > > > > > > > + pcie@3,0 { > > > > > > > + reg = <0x21800 0x0 0x0 0x0 0x0>; > > > > > > > + #address-cells = <3>; > > > > > > > + #size-cells = <2>; > > > > > > > + device_type = "pci"; > > > > > > > + ranges; > > > > > > > + bus-range = <0x05 0xff>; > > > > > > > + > > > > > > > + qcom,tx-amplitude-millivolt = <10>; > > > > > > > + pcie@0,0 { > > > > > > > + reg = <0x50000 0x0 0x0 0x0 0x0>; > > > > > > > + #address-cells = <3>; > > > > > > > + #size-cells = <2>; > > > > > > > + device_type = "pci"; > > > > > > > > > > > > There's a 2nd PCI-PCI bridge? > > > > > This the embedded ethernet port which is as part of DSP3. > > > > > > > > So is there an adidtional bus for that ethernet device? > > > > > > > yes for ethernet it has aditional bus assigned. > > > > > > > > > > > > > - Krishna Chaitanya. > > > > > > > > > > > > > + ranges; > > > > > > > + > > > > > > > + qcom,l1-entry-delay-ns = <10>; > > > > > > > + }; > > > > > > > + > > > > > > > + pcie@0,1 { > > > > > > > + reg = <0x50100 0x0 0x0 0x0 0x0>; > > > > > > > + #address-cells = <3>; > > > > > > > + #size-cells = <2>; > > > > > > > + device_type = "pci"; > > > > > > > + ranges; > > > > > > > + > > > > > > > + qcom,l0s-entry-delay-ns = <10>; > > > > > > > + }; > > > > > > > > What is this? > > > > > > > Ethernet endpoint is a multi function device which has 2 functions > > > This node represents 2nd node. I will update the description to > > > reflect the same. > > > > If this is an ethernet device, why does it have a name of pcie@? Per > > bindings the pcie@ name should be used only for devices with the class > > 0604. Whas is the PCI device class for those devices? I think ethernet@ > > (0200) should probably be the best fit, judgin by your description. > > > These are PCIe Ethernet endpoints with base class 2 & subclass 2, I taught Hmm, 0x0202 is reserved for FDDI devices. Has somebody been over-creative or is it me missing a part of the PCIe spec? > all the PCIe devices should start with pcie irrespective of the class, can > you point us the binding you are referring to. I was referring to this https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-device.yaml#L23 These bindings do not fix $nodename. A quick search through the existing DTs should have provided you with the list of the endpoint device node names as used by the kernel, which contains more than just pcie@N,M. As for the spec, see [1], Table 1 on page 15. It defines node names for various PCI classes. [1] https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf > > As this is a endpoint device, I will cross check once if we can represent > here or not and get back on this. Unless you have to configure those ports, you should be able to skip them. From your DT example it is not clear which part is actually configurable: pcie@3,0 or its subdevices: DT example contains extra properties in all those nodes. > > - Krishna Chaitanya. > > > > > > - Krishna Chaitanya. > > > > > > > > > > + }; > > > > > > > + }; > > > > > > > + }; > > > > > > > + }; > > > > > > > > > > > > > > -- > > > > > > > 2.34.1 > > > > > > > > > > > > >
On Tue, Dec 24, 2024 at 02:41:10PM +0530, Krishna Chaitanya Chundru wrote: > > > On 12/5/2024 2:55 AM, Bjorn Helgaas wrote: > > On Tue, Nov 12, 2024 at 08:31:33PM +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. > > > > > +$defs: > > > + qps615-node: > > > + type: object > > > + > > > + properties: > > > + qcom,l0s-entry-delay-ns: > > > + description: Aspm l0s entry delay. > > > + > > > + qcom,l1-entry-delay-ns: > > > + description: Aspm l1 entry delay. > > > > To match spec usage: > > s/Aspm/ASPM/ > > s/l0s/L0s/ > > s/l1/L1/ > > > > Other than the fact that qps615 needs the driver to configure these, > > there's nothing qcom-specific here, so I suggest the names should omit > > "qcom" and include "aspm". > > > > > + 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; > > > + bus-range = <0x01 0xff>; > > > + > > > + pcie@0,0 { > > > + compatible = "pci1179,0623"; > > > + reg = <0x10000 0x0 0x0 0x0 0x0>; > > > + device_type = "pci"; > > > + #address-cells = <3>; > > > + #size-cells = <2>; > > > + ranges; > > > + bus-range = <0x02 0xff>; > > > > This binding describes a switch. I don't think bus-range should > > appear here at all because it is not a feature of the hardware (unless > > the switch ports are broken and their Secondary/Subordinate Bus > > Numbers are hard-wired). > > > > The Primary/Secondary/Subordinate Bus Numbers of all switch ports > > should be writable and the PCI core knows how to manage them. > > > Bjorn, > > The dt binding check is throwing an error if we don't keep bus-range > property for that reason we added it, from dt binding perspective i think it > is mandatory to add this property. Could you please provide an error message? I don't see any of the PCIe bindingins declaring bus-range as mandatory. I might be missing it though. > > - Krishna Chaitanya. > > Bjorn
diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml new file mode 100644 index 000000000000..e6a63a0bb0f3 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml @@ -0,0 +1,205 @@ +# 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 + + i2c-parent: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: | + A phandle to the parent I2C node and the slave address of the device + used to do configure qps615 to change FTS, tx amplitude etc. + items: + - description: Phandle to the I2C controller node + - description: I2C slave address + + 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 rate which is internal bus of the switch + The switch only runs in two frequencies i.e 250MHz and 125MHz. + enum: [125000000, 250000000] + +allOf: + - $ref: "#/$defs/qps615-node" + +patternProperties: + "@1?[0-9a-f](,[0-7])?$": + description: child nodes describing the internal downstream ports + the qps615 switch. + type: object + $ref: "#/$defs/qps615-node" + unevaluatedProperties: false + +$defs: + qps615-node: + type: object + + properties: + qcom,l0s-entry-delay-ns: + description: Aspm l0s entry delay. + + qcom,l1-entry-delay-ns: + description: Aspm l1 entry delay. + + qcom,tx-amplitude-millivolt: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Change Tx Margin setting for low power consumption. + + qcom,no-dfe-support: + type: boolean + description: Disable DFE (Decision Feedback Equalizer), which mitigates + intersymbol interference and some reflections caused by impedance mismatches. + + qcom,nfts: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Number of Fast Training Sequence (FTS) used during L0s to L0 exit + for bit and Symbol lock. + + allOf: + - $ref: /schemas/pci/pci-bus.yaml# + +unevaluatedProperties: false + +required: + - vdd18-supply + - vdd09-supply + - vddc-supply + - vddio1-supply + - vddio2-supply + - vddio18-supply + - i2c-parent + - reset-gpios + +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; + bus-range = <0x01 0xff>; + + pcie@0,0 { + compatible = "pci1179,0623"; + reg = <0x10000 0x0 0x0 0x0 0x0>; + device_type = "pci"; + #address-cells = <3>; + #size-cells = <2>; + ranges; + bus-range = <0x02 0xff>; + + i2c-parent = <&qup_i2c 0x77>; + + 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; + bus-range = <0x03 0xff>; + + qcom,no-dfe-support; + }; + + pcie@2,0 { + reg = <0x21000 0x0 0x0 0x0 0x0>; + #address-cells = <3>; + #size-cells = <2>; + device_type = "pci"; + ranges; + bus-range = <0x04 0xff>; + + qcom,nfts = <10>; + }; + + pcie@3,0 { + reg = <0x21800 0x0 0x0 0x0 0x0>; + #address-cells = <3>; + #size-cells = <2>; + device_type = "pci"; + ranges; + bus-range = <0x05 0xff>; + + qcom,tx-amplitude-millivolt = <10>; + pcie@0,0 { + reg = <0x50000 0x0 0x0 0x0 0x0>; + #address-cells = <3>; + #size-cells = <2>; + device_type = "pci"; + ranges; + + qcom,l1-entry-delay-ns = <10>; + }; + + pcie@0,1 { + reg = <0x50100 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 | 205 +++++++++++++++++++++ 1 file changed, 205 insertions(+)