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

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

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@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.
> 
> 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.
> 
There is a slot driver which is using this pciclass,0604 can we use
still the suggested way.

https://lore.kernel.org/linux-pci/20241210-pci-pwrctrl-slot-v1-4-eae45e488040@linaro.org/

- Krishna Chaitanya.
>>
>>>> +
>>>> +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
>>>>
>