Message-ID: <hadsywppxfppeqqbqejuztrc3igg3mwmgdhpffxxyj4vnrowjh@uxtjywpipdig>
Date: Tue, 24 Dec 2024 11:47:11 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Krishna Chaitanya Chundru <quic_krichai@...cinc.com>
Cc: Rob Herring <robh@...nel.org>, andersson@...nel.org, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Lorenzo Pieralisi <lpieralisi@...nel.org>, 
	Krzysztof Wilczyński <kw@...ux.com>, Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Konrad Dybcio <konradybcio@...nel.org>, cros-qcom-dts-watchers@...omium.org, 
	Jingoo Han <jingoohan1@...il.com>, Bartosz Golaszewski <brgl@...ev.pl>, quic_vbadigan@...cinc.com, 
	linux-arm-msm@...r.kernel.org, linux-pci@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/6] dt-bindings: PCI: Add binding for qps615

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@...cinc.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@...cinc.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
> > > > > > > 
> > > > 
> > 

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists