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@xxxxxxxxxxx> > > > --- > > > .../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@xxxxxxxxxxx> > > > + > > > +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 > > > -- With best wishes Dmitry
