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

Krishna Chaitanya Chundru
2024-12-23T16:46:26Z

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
>>>>
>>
>