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

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

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@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#
>>>
>> 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.
>>>>>> +
>>>>>> +  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 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

As this is a endpoint device, I will cross check once if we can 
represent here or not and get back on this.

- Krishna Chaitanya.
>>
>> - Krishna Chaitanya.
>>
>>>>>> +                };
>>>>>> +            };
>>>>>> +        };
>>>>>> +    };
>>>>>>
>>>>>> -- 
>>>>>> 2.34.1
>>>>>>
>>>
>