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

[Krishna Chaitanya Chundru <quic_krichai@quicinc.com>]

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