Re: [PATCH v3 1/6] dt-bindings: PCI: Add binding for qps615

From: Dmitry Baryshkov
Date: Mon Dec 23 2024 - 13:57:59 EST

Next message: Dmitry Baryshkov: "Re: [PATCH v3 1/6] dt-bindings: PCI: Add binding for qps615"
Previous message: Andy Shevchenko: "Re: [PATCH v1 1/1] lockdep: Mark chain_hlock_class_idx() with __maybe_unused"
In reply to: Dmitry Baryshkov: "Re: [PATCH v3 1/6] dt-bindings: PCI: Add binding for qps615"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Dmitry Baryshkov: "Re: [PATCH v3 1/6] dt-bindings: PCI: Add binding for qps615"
Previous message: Andy Shevchenko: "Re: [PATCH v1 1/1] lockdep: Mark chain_hlock_class_idx() with __maybe_unused"
In reply to: Dmitry Baryshkov: "Re: [PATCH v3 1/6] dt-bindings: PCI: Add binding for qps615"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]