diff mbox series

[v2,1/8] dt-bindings: PCI: Add binding for qps615

Message ID 20240803-qps615-v2-1-9560b7c71369@quicinc.com
State New
Headers show
Series PCI: Enable Power and configure the QPS615 PCIe switch | expand

Commit Message

Krishna chaitanya chundru Aug. 3, 2024, 3:22 a.m. UTC
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       | 191 +++++++++++++++++++++
 1 file changed, 191 insertions(+)

Comments

Dmitry Baryshkov Aug. 3, 2024, 11 a.m. UTC | #1
On Sat, Aug 03, 2024 at 08:52:47AM GMT, 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       | 191 +++++++++++++++++++++
>  1 file changed, 191 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..ea0c953ee56f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
> @@ -0,0 +1,191 @@
> +# 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
> +
> +  qcom,qps615-controller:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Reference to the I2C client used to do configure qps615
> +
> +  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.
> +
> +  qps615,axi-clk-freq-hz:
> +    description:
> +      AXI clock which internal bus of the switch.

Is it a clock or clock rate?

> +
> +  qcom,l0s-entry-delay-ns:
> +    description: Aspm l0s entry delay in nanoseconds.

I'd say, from the property name it is obvious that it comes in
nanoseconds.

> +
> +  qcom,l1-entry-delay-ns:
> +    description: Aspm l1 entry delay in nanoseconds.
> +
> +  qcom,tx-amplitude-millivolt:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Change Tx Margin setting for low power consumption.
> +
> +  qcom,no-dfe:
> +    type: boolean
> +    description: Disables DFE (Decision Feedback Equalizer).
> +
> +  qcom,nfts:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    description:
> +      Fast Training Sequence (FTS) is the mechanism that
> +      is used for bit and Symbol lock.

Doesn't help to understand what it is and what the value means.

> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus-common.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: pci1179,0623
> +      required:
> +        - compatible
> +    then:
> +      required:
> +        - vdd18-supply
> +        - vdd09-supply
> +        - vddc-supply
> +        - vddio1-supply
> +        - vddio2-supply
> +        - vddio18-supply
> +        - qcom,qps615-controller
> +        - reset-gpios
> +
> +patternProperties:
> +  "@1?[0-9a-f](,[0-7])?$":
> +    type: object
> +    $ref: qcom,qps615.yaml#
> +    additionalProperties: true
> +
> +additionalProperties: true
> +
> +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;
> +
> +            pcie@0,0 {
> +                compatible = "pci1179,0623";
> +                reg = <0x10000 0x0 0x0 0x0 0x0>;
> +                device_type = "pci";
> +                #address-cells = <3>;
> +                #size-cells = <2>;
> +                ranges;
> +
> +                qcom,qps615-controller = <&qps615_controller>;

Where is the corresponding device?

> +
> +                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;
> +
> +                    qcom,no-dfe;
> +                };
> +
> +                pcie@2,0 {
> +                    reg = <0x21000 0x0 0x0 0x0 0x0>;
> +                    #address-cells = <3>;
> +                    #size-cells = <2>;
> +                    device_type = "pci";
> +                    ranges;
> +
> +                    qcom,nfts = /bits/ 8 <10>;
> +                };
> +
> +                pcie@3,0 {
> +                    reg = <0x21800 0x0 0x0 0x0 0x0>;
> +                    #address-cells = <3>;
> +                    #size-cells = <2>;
> +                    device_type = "pci";
> +                    ranges;
> +
> +                    qcom,tx-amplitude-millivolt = <10>;
> +
> +                         pcie@0,0 {

Wrong indentation.

> +                              reg = <0x40000 0x0 0x0 0x0 0x0>;
> +                              #address-cells = <3>;
> +                              #size-cells = <2>;
> +                              device_type = "pci";
> +                              ranges;
> +
> +                              qcom,l1-entry-delay-ns = <10>;
> +                         };
> +
> +                         pcie@0,1 {
> +                              reg = <0x40100 0x0 0x0 0x0 0x0>;
> +                              #address-cells = <3>;
> +                              #size-cells = <2>;
> +                              device_type = "pci";
> +                              ranges;
> +
> +                              qcom,l0s-entry-delay-ns = <10>;
> +                         };
> +                };
> +            };
> +        };
> +    };
> 
> -- 
> 2.34.1
>
Krzysztof Kozlowski Aug. 4, 2024, 8:53 a.m. UTC | #2
On 03/08/2024 05:22, 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       | 191 +++++++++++++++++++++
>  1 file changed, 191 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..ea0c953ee56f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
> @@ -0,0 +1,191 @@
> +# 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
> +
> +  qcom,qps615-controller:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Reference to the I2C client used to do configure qps615

Why?

> +
> +  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.
> +
> +  qps615,axi-clk-freq-hz:
> +    description:
> +      AXI clock which internal bus of the switch.

No need, use CCF.

> +
> +  qcom,l0s-entry-delay-ns:
> +    description: Aspm l0s entry delay in nanoseconds.
> +
> +  qcom,l1-entry-delay-ns:
> +    description: Aspm l1 entry delay in nanoseconds.
> +
> +  qcom,tx-amplitude-millivolt:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Change Tx Margin setting for low power consumption.
> +
> +  qcom,no-dfe:
> +    type: boolean
> +    description: Disables DFE (Decision Feedback Equalizer).

You described the desired Linux feature or behavior, not the actual
hardware. The bindings are about the latter, so instead you need to
rephrase the property and its description to match actual hardware
capabilities/features/configuration etc.

> +
> +  qcom,nfts:
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    description:
> +      Fast Training Sequence (FTS) is the mechanism that
> +      is used for bit and Symbol lock.

What are the values? Why this is uint8?

You described the desired Linux feature or behavior, not the actual
hardware. The bindings are about the latter, so instead you need to
rephrase the property and its description to match actual hardware
capabilities/features/configuration etc.

> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus-common.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: pci1179,0623
> +      required:
> +        - compatible

Why do you have entire if? You do not have multiple variants, drop.

> +    then:
> +      required:
> +        - vdd18-supply
> +        - vdd09-supply
> +        - vddc-supply
> +        - vddio1-supply
> +        - vddio2-supply
> +        - vddio18-supply
> +        - qcom,qps615-controller
> +        - reset-gpios
> +
> +patternProperties:
> +  "@1?[0-9a-f](,[0-7])?$":
> +    type: object
> +    $ref: qcom,qps615.yaml#
> +    additionalProperties: true

Nope, drop pattern Properties or explain what is this.

> +
> +additionalProperties: true

This cannot be true,

> +
> +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;
> +
> +            pcie@0,0 {
> +                compatible = "pci1179,0623";
> +                reg = <0x10000 0x0 0x0 0x0 0x0>;
> +                device_type = "pci";
> +                #address-cells = <3>;
> +                #size-cells = <2>;
> +                ranges;
> +
> +                qcom,qps615-controller = <&qps615_controller>;
> +
> +                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>;

Where is the compatible? You claim this is the same device as child?

> +                    #address-cells = <3>;
> +                    #size-cells = <2>;
> +                    device_type = "pci";
> +                    ranges;
> +
> +                    qcom,no-dfe;
> +                };
> +
> +                pcie@2,0 {
> +                    reg = <0x21000 0x0 0x0 0x0 0x0>;
> +                    #address-cells = <3>;
> +                    #size-cells = <2>;
> +                    device_type = "pci";
> +                    ranges;
> +
> +                    qcom,nfts = /bits/ 8 <10>;
> +                };
> +
> +                pcie@3,0 {
> +                    reg = <0x21800 0x0 0x0 0x0 0x0>;
> +                    #address-cells = <3>;
> +                    #size-cells = <2>;
> +                    device_type = "pci";
> +                    ranges;
> +
> +                    qcom,tx-amplitude-millivolt = <10>;
> +
> +                         pcie@0,0 {

Total mess in indentation.



Best regards,
Krzysztof
Krzysztof Kozlowski Aug. 4, 2024, 8:56 a.m. UTC | #3
On 03/08/2024 05:22, 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       | 191 +++++++++++++++++++++
>  1 file changed, 191 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..ea0c953ee56f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
> @@ -0,0 +1,191 @@
> +# 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
> +
> +  qcom,qps615-controller:

and now I see that you totally ignored comments. Repeating the same over
and over is a waste of time.

<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.
</form letter>


Best regards,
Krzysztof
Krishna chaitanya chundru Aug. 5, 2024, 4:02 a.m. UTC | #4
On 8/4/2024 2:26 PM, Krzysztof Kozlowski wrote:
> On 03/08/2024 05:22, 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       | 191 +++++++++++++++++++++
>>   1 file changed, 191 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..ea0c953ee56f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
>> @@ -0,0 +1,191 @@
>> +# 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
>> +
>> +  qcom,qps615-controller:
> 
> and now I see that you totally ignored comments. Repeating the same over
> and over is a waste of time.
> 
> <form letter>
> This is a friendly reminder during the review process.
> 
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.
> 
> Thank you.
> </form letter>
> 
> 
> Best regards,
> Krzysztof
> 
Hi Krzysztof,

In patch1 we are trying to add reference of i2c-adapter, you suggested
to use i2c-bus for that. we got comments on the driver code not to use
adapter and instead use i2c client reference. I felt i2c-bus is not
ideal to represent i2c client device so used this name.

I should have mentioned all these details in the change log for all
these changes. Next time onwards I will give detailed change log for
the changes between two versions.

- Krishna Chaitanya.
Krishna chaitanya chundru Aug. 5, 2024, 4:11 a.m. UTC | #5
On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote:
> On 03/08/2024 05:22, 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       | 191 +++++++++++++++++++++
>>   1 file changed, 191 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..ea0c953ee56f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
>> @@ -0,0 +1,191 @@
>> +# 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
>> +
>> +  qcom,qps615-controller:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      Reference to the I2C client used to do configure qps615
> 
> Why?
> 
>> +
>> +  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.
>> +
>> +  qps615,axi-clk-freq-hz:
>> +    description:
>> +      AXI clock which internal bus of the switch.
> 
> No need, use CCF.
> 
ack
>> +
>> +  qcom,l0s-entry-delay-ns:
>> +    description: Aspm l0s entry delay in nanoseconds.
>> +
>> +  qcom,l1-entry-delay-ns:
>> +    description: Aspm l1 entry delay in nanoseconds.
>> +
>> +  qcom,tx-amplitude-millivolt:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: Change Tx Margin setting for low power consumption.
>> +
>> +  qcom,no-dfe:
>> +    type: boolean
>> +    description: Disables DFE (Decision Feedback Equalizer).
> 
> You described the desired Linux feature or behavior, not the actual
> hardware. The bindings are about the latter, so instead you need to
> rephrase the property and its description to match actual hardware
> capabilities/features/configuration etc.
> 
ack
>> +
>> +  qcom,nfts:
>> +    $ref: /schemas/types.yaml#/definitions/uint8
>> +    description:
>> +      Fast Training Sequence (FTS) is the mechanism that
>> +      is used for bit and Symbol lock.
> 
> What are the values? Why this is uint8?
> 
These represents number of fast training sequence and doesn't have
any units and the maximum value for this is 0xFF only so we used
uint8.
> You described the desired Linux feature or behavior, not the actual
> hardware. The bindings are about the latter, so instead you need to
> rephrase the property and its description to match actual hardware
> capabilities/features/configuration etc.
ack.
> 
>> +
>> +allOf:
>> +  - $ref: /schemas/pci/pci-bus-common.yaml#
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: pci1179,0623
>> +      required:
>> +        - compatible
> 
> Why do you have entire if? You do not have multiple variants, drop.
> 
The child nodes also referencing the qcom,qps615.yaml# node, I tried
to use this way to say "the below properties are for the required for
parent and optional for child".
>> +    then:
>> +      required:
>> +        - vdd18-supply
>> +        - vdd09-supply
>> +        - vddc-supply
>> +        - vddio1-supply
>> +        - vddio2-supply
>> +        - vddio18-supply
>> +        - qcom,qps615-controller
>> +        - reset-gpios
>> +
>> +patternProperties:
>> +  "@1?[0-9a-f](,[0-7])?$":
>> +    type: object
>> +    $ref: qcom,qps615.yaml#
>> +    additionalProperties: true
> 
> Nope, drop pattern Properties or explain what is this.
> 
the child nodes represent the downstream ports of the PCIe
switch which wants to use same properties that is why
I tried to use this pattern properties.
>> +
>> +additionalProperties: true
> 
> This cannot be true,
> 
Let me check on this correct in next versions.
>> +
>> +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;
>> +
>> +            pcie@0,0 {
>> +                compatible = "pci1179,0623";
>> +                reg = <0x10000 0x0 0x0 0x0 0x0>;
>> +                device_type = "pci";
>> +                #address-cells = <3>;
>> +                #size-cells = <2>;
>> +                ranges;
>> +
>> +                qcom,qps615-controller = <&qps615_controller>;
>> +
>> +                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>;
> 
> Where is the compatible? You claim this is the same device as child?
> 
These all the PCIe downstream nodes and compatible is optional for the
PCIe nodes.
>> +                    #address-cells = <3>;
>> +                    #size-cells = <2>;
>> +                    device_type = "pci";
>> +                    ranges;
>> +
>> +                    qcom,no-dfe;
>> +                };
>> +
>> +                pcie@2,0 {
>> +                    reg = <0x21000 0x0 0x0 0x0 0x0>;
>> +                    #address-cells = <3>;
>> +                    #size-cells = <2>;
>> +                    device_type = "pci";
>> +                    ranges;
>> +
>> +                    qcom,nfts = /bits/ 8 <10>;
>> +                };
>> +
>> +                pcie@3,0 {
>> +                    reg = <0x21800 0x0 0x0 0x0 0x0>;
>> +                    #address-cells = <3>;
>> +                    #size-cells = <2>;
>> +                    device_type = "pci";
>> +                    ranges;
>> +
>> +                    qcom,tx-amplitude-millivolt = <10>;
>> +
>> +                         pcie@0,0 {
> 
> Total mess in indentation.
>
Let me correct in next version.

- Krishna Chaitanya.

> 
> 
> Best regards,
> Krzysztof
>
Krishna chaitanya chundru Aug. 5, 2024, 4:16 a.m. UTC | #6
On 8/3/2024 4:30 PM, Dmitry Baryshkov wrote:
> On Sat, Aug 03, 2024 at 08:52:47AM GMT, 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       | 191 +++++++++++++++++++++
>>   1 file changed, 191 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..ea0c953ee56f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
>> @@ -0,0 +1,191 @@
>> +# 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
>> +
>> +  qcom,qps615-controller:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      Reference to the I2C client used to do configure qps615
>> +
>> +  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.
>> +
>> +  qps615,axi-clk-freq-hz:
>> +    description:
>> +      AXI clock which internal bus of the switch.
> 
> Is it a clock or clock rate?
It is clock ony.
> 
>> +
>> +  qcom,l0s-entry-delay-ns:
>> +    description: Aspm l0s entry delay in nanoseconds.
> 
> I'd say, from the property name it is obvious that it comes in
> nanoseconds.
> 
I will remove the description from these properties.
>> +
>> +  qcom,l1-entry-delay-ns:
>> +    description: Aspm l1 entry delay in nanoseconds.
>> +
>> +  qcom,tx-amplitude-millivolt:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: Change Tx Margin setting for low power consumption.
>> +
>> +  qcom,no-dfe:
>> +    type: boolean
>> +    description: Disables DFE (Decision Feedback Equalizer).
>> +
>> +  qcom,nfts:
>> +    $ref: /schemas/types.yaml#/definitions/uint8
>> +    description:
>> +      Fast Training Sequence (FTS) is the mechanism that
>> +      is used for bit and Symbol lock.
> 
> Doesn't help to understand what it is and what the value means.
> 
I will update the description, this property represents number
of fast training sequence needs to be used for link transition
from L0s to L0.

- Krishna Chaitanya.
>>  >> +allOf:
>> +  - $ref: /schemas/pci/pci-bus-common.yaml#
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: pci1179,0623
>> +      required:
>> +        - compatible
>> +    then:
>> +      required:
>> +        - vdd18-supply
>> +        - vdd09-supply
>> +        - vddc-supply
>> +        - vddio1-supply
>> +        - vddio2-supply
>> +        - vddio18-supply
>> +        - qcom,qps615-controller
>> +        - reset-gpios
>> +
>> +patternProperties:
>> +  "@1?[0-9a-f](,[0-7])?$":
>> +    type: object
>> +    $ref: qcom,qps615.yaml#
>> +    additionalProperties: true
>> +
>> +additionalProperties: true
>> +
>> +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;
>> +
>> +            pcie@0,0 {
>> +                compatible = "pci1179,0623";
>> +                reg = <0x10000 0x0 0x0 0x0 0x0>;
>> +                device_type = "pci";
>> +                #address-cells = <3>;
>> +                #size-cells = <2>;
>> +                ranges;
>> +
>> +                qcom,qps615-controller = <&qps615_controller>;
> 
> Where is the corresponding device?
> 
>> +
>> +                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;
>> +
>> +                    qcom,no-dfe;
>> +                };
>> +
>> +                pcie@2,0 {
>> +                    reg = <0x21000 0x0 0x0 0x0 0x0>;
>> +                    #address-cells = <3>;
>> +                    #size-cells = <2>;
>> +                    device_type = "pci";
>> +                    ranges;
>> +
>> +                    qcom,nfts = /bits/ 8 <10>;
>> +                };
>> +
>> +                pcie@3,0 {
>> +                    reg = <0x21800 0x0 0x0 0x0 0x0>;
>> +                    #address-cells = <3>;
>> +                    #size-cells = <2>;
>> +                    device_type = "pci";
>> +                    ranges;
>> +
>> +                    qcom,tx-amplitude-millivolt = <10>;
>> +
>> +                         pcie@0,0 {
> 
> Wrong indentation.
> 
>> +                              reg = <0x40000 0x0 0x0 0x0 0x0>;
>> +                              #address-cells = <3>;
>> +                              #size-cells = <2>;
>> +                              device_type = "pci";
>> +                              ranges;
>> +
>> +                              qcom,l1-entry-delay-ns = <10>;
>> +                         };
>> +
>> +                         pcie@0,1 {
>> +                              reg = <0x40100 0x0 0x0 0x0 0x0>;
>> +                              #address-cells = <3>;
>> +                              #size-cells = <2>;
>> +                              device_type = "pci";
>> +                              ranges;
>> +
>> +                              qcom,l0s-entry-delay-ns = <10>;
>> +                         };
>> +                };
>> +            };
>> +        };
>> +    };
>>
>> -- 
>> 2.34.1
>>
>
Krzysztof Kozlowski Aug. 5, 2024, 5:12 a.m. UTC | #7
On 05/08/2024 06:02, Krishna Chaitanya Chundru wrote:
> 
> 
> On 8/4/2024 2:26 PM, Krzysztof Kozlowski wrote:
>> On 03/08/2024 05:22, 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       | 191 +++++++++++++++++++++
>>>   1 file changed, 191 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..ea0c953ee56f
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
>>> @@ -0,0 +1,191 @@
>>> +# 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
>>> +
>>> +  qcom,qps615-controller:
>>
>> and now I see that you totally ignored comments. Repeating the same over
>> and over is a waste of time.
>>
>> <form letter>
>> This is a friendly reminder during the review process.
>>
>> It seems my or other reviewer's previous comments were not fully
>> addressed. Maybe the feedback got lost between the quotes, maybe you
>> just forgot to apply it. Please go back to the previous discussion and
>> either implement all requested changes or keep discussing them.
>>
>> Thank you.
>> </form letter>
>>
>>
>> Best regards,
>> Krzysztof
>>
> Hi Krzysztof,
> 
> In patch1 we are trying to add reference of i2c-adapter, you suggested
> to use i2c-bus for that. we got comments on the driver code not to use
> adapter and instead use i2c client reference. I felt i2c-bus is not
> ideal to represent i2c client device so used this name.

You did not respond to comment of using i2c-bus, just silently decided
to implement other property.

Anyway, why i2c-bus is not suitable here? I am quite surprised...



Best regards,
Krzysztof
Krzysztof Kozlowski Aug. 5, 2024, 5:14 a.m. UTC | #8
On 05/08/2024 06:11, Krishna Chaitanya Chundru wrote:


>>> +
>>> +  qcom,nfts:
>>> +    $ref: /schemas/types.yaml#/definitions/uint8
>>> +    description:
>>> +      Fast Training Sequence (FTS) is the mechanism that
>>> +      is used for bit and Symbol lock.
>>
>> What are the values? Why this is uint8?
>>
> These represents number of fast training sequence and doesn't have
> any units and the maximum value for this is 0xFF only so we used
> uint8.
>> You described the desired Linux feature or behavior, not the actual
>> hardware. The bindings are about the latter, so instead you need to
>> rephrase the property and its description to match actual hardware
>> capabilities/features/configuration etc.
> ack.
>>
>>> +
>>> +allOf:
>>> +  - $ref: /schemas/pci/pci-bus-common.yaml#
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: pci1179,0623
>>> +      required:
>>> +        - compatible
>>
>> Why do you have entire if? You do not have multiple variants, drop.
>>
> The child nodes also referencing the qcom,qps615.yaml# node, I tried
> to use this way to say "the below properties are for the required for
> parent and optional for child".

I don't understand how child device can be exactly the same as parent
device. How does it look in terms of hardware? Pins and supplies?

>>> +    then:
>>> +      required:
>>> +        - vdd18-supply
>>> +        - vdd09-supply
>>> +        - vddc-supply
>>> +        - vddio1-supply
>>> +        - vddio2-supply
>>> +        - vddio18-supply
>>> +        - qcom,qps615-controller
>>> +        - reset-gpios
>>> +
>>> +patternProperties:
>>> +  "@1?[0-9a-f](,[0-7])?$":
>>> +    type: object
>>> +    $ref: qcom,qps615.yaml#
>>> +    additionalProperties: true
>>
>> Nope, drop pattern Properties or explain what is this.
>>
> the child nodes represent the downstream ports of the PCIe
> switch which wants to use same properties that is why
> I tried to use this pattern properties.

Downstream port is not the same as device. Why downstream port has the
same supplies? To which pins are they connected?




Best regards,
Krzysztof
Krishna chaitanya chundru Aug. 5, 2024, 5:26 a.m. UTC | #9
On 8/5/2024 10:44 AM, Krzysztof Kozlowski wrote:
> On 05/08/2024 06:11, Krishna Chaitanya Chundru wrote:
> 
> 
>>>> +
>>>> +  qcom,nfts:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint8
>>>> +    description:
>>>> +      Fast Training Sequence (FTS) is the mechanism that
>>>> +      is used for bit and Symbol lock.
>>>
>>> What are the values? Why this is uint8?
>>>
>> These represents number of fast training sequence and doesn't have
>> any units and the maximum value for this is 0xFF only so we used
>> uint8.
>>> You described the desired Linux feature or behavior, not the actual
>>> hardware. The bindings are about the latter, so instead you need to
>>> rephrase the property and its description to match actual hardware
>>> capabilities/features/configuration etc.
>> ack.
>>>
>>>> +
>>>> +allOf:
>>>> +  - $ref: /schemas/pci/pci-bus-common.yaml#
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            const: pci1179,0623
>>>> +      required:
>>>> +        - compatible
>>>
>>> Why do you have entire if? You do not have multiple variants, drop.
>>>
>> The child nodes also referencing the qcom,qps615.yaml# node, I tried
>> to use this way to say "the below properties are for the required for
>> parent and optional for child".
> 
> I don't understand how child device can be exactly the same as parent
> device. How does it look in terms of hardware? Pins and supplies?
> 
>>>> +    then:
>>>> +      required:
>>>> +        - vdd18-supply
>>>> +        - vdd09-supply
>>>> +        - vddc-supply
>>>> +        - vddio1-supply
>>>> +        - vddio2-supply
>>>> +        - vddio18-supply
>>>> +        - qcom,qps615-controller
>>>> +        - reset-gpios
>>>> +
>>>> +patternProperties:
>>>> +  "@1?[0-9a-f](,[0-7])?$":
>>>> +    type: object
>>>> +    $ref: qcom,qps615.yaml#
>>>> +    additionalProperties: true
>>>
>>> Nope, drop pattern Properties or explain what is this.
>>>
>> the child nodes represent the downstream ports of the PCIe
>> switch which wants to use same properties that is why
>> I tried to use this pattern properties.
> 
> Downstream port is not the same as device. Why downstream port has the
> same supplies? To which pins are they connected?
> 
> 
Hi Krzysztof,

Downstream ports dosen't have pins or supplies to power on.

But there are properties like qcom,l0s-entry-delay-ns,
qcom,l1-entry-delay-ns,  qcom,tx-amplitude-millivolt etc which
applicable for child nodes also. Instead of re-declaring the
these properties again I tried to use pattern properties.

- krishna Chaitanya.
> 
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Aug. 5, 2024, 5:28 a.m. UTC | #10
On 05/08/2024 07:26, Krishna Chaitanya Chundru wrote:
> 
> 
> On 8/5/2024 10:44 AM, Krzysztof Kozlowski wrote:
>> On 05/08/2024 06:11, Krishna Chaitanya Chundru wrote:
>>
>>
>>>>> +
>>>>> +  qcom,nfts:
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint8
>>>>> +    description:
>>>>> +      Fast Training Sequence (FTS) is the mechanism that
>>>>> +      is used for bit and Symbol lock.
>>>>
>>>> What are the values? Why this is uint8?
>>>>
>>> These represents number of fast training sequence and doesn't have
>>> any units and the maximum value for this is 0xFF only so we used
>>> uint8.
>>>> You described the desired Linux feature or behavior, not the actual
>>>> hardware. The bindings are about the latter, so instead you need to
>>>> rephrase the property and its description to match actual hardware
>>>> capabilities/features/configuration etc.
>>> ack.
>>>>
>>>>> +
>>>>> +allOf:
>>>>> +  - $ref: /schemas/pci/pci-bus-common.yaml#
>>>>> +  - if:
>>>>> +      properties:
>>>>> +        compatible:
>>>>> +          contains:
>>>>> +            const: pci1179,0623
>>>>> +      required:
>>>>> +        - compatible
>>>>
>>>> Why do you have entire if? You do not have multiple variants, drop.
>>>>
>>> The child nodes also referencing the qcom,qps615.yaml# node, I tried
>>> to use this way to say "the below properties are for the required for
>>> parent and optional for child".
>>
>> I don't understand how child device can be exactly the same as parent
>> device. How does it look in terms of hardware? Pins and supplies?
>>
>>>>> +    then:
>>>>> +      required:
>>>>> +        - vdd18-supply
>>>>> +        - vdd09-supply
>>>>> +        - vddc-supply
>>>>> +        - vddio1-supply
>>>>> +        - vddio2-supply
>>>>> +        - vddio18-supply
>>>>> +        - qcom,qps615-controller
>>>>> +        - reset-gpios
>>>>> +
>>>>> +patternProperties:
>>>>> +  "@1?[0-9a-f](,[0-7])?$":
>>>>> +    type: object
>>>>> +    $ref: qcom,qps615.yaml#
>>>>> +    additionalProperties: true
>>>>
>>>> Nope, drop pattern Properties or explain what is this.
>>>>
>>> the child nodes represent the downstream ports of the PCIe
>>> switch which wants to use same properties that is why
>>> I tried to use this pattern properties.
>>
>> Downstream port is not the same as device. Why downstream port has the
>> same supplies? To which pins are they connected?
>>
>>
> Hi Krzysztof,
> 
> Downstream ports dosen't have pins or supplies to power on.
> 
> But there are properties like qcom,l0s-entry-delay-ns,
> qcom,l1-entry-delay-ns,  qcom,tx-amplitude-millivolt etc which
> applicable for child nodes also. Instead of re-declaring the
> these properties again I tried to use pattern properties.

You could use $defs for them, but I don't understand how does these
properties apply for both main device and ports. It seems you are
writing binding to match some driver behavior. Let's start from basics -
describe the hardware.

Best regards,
Krzysztof
Krishna chaitanya chundru Aug. 5, 2024, 5:33 a.m. UTC | #11
On 8/5/2024 10:42 AM, Krzysztof Kozlowski wrote:
> On 05/08/2024 06:02, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 8/4/2024 2:26 PM, Krzysztof Kozlowski wrote:
>>> On 03/08/2024 05:22, 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       | 191 +++++++++++++++++++++
>>>>    1 file changed, 191 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..ea0c953ee56f
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
>>>> @@ -0,0 +1,191 @@
>>>> +# 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
>>>> +
>>>> +  qcom,qps615-controller:
>>>
>>> and now I see that you totally ignored comments. Repeating the same over
>>> and over is a waste of time.
>>>
>>> <form letter>
>>> This is a friendly reminder during the review process.
>>>
>>> It seems my or other reviewer's previous comments were not fully
>>> addressed. Maybe the feedback got lost between the quotes, maybe you
>>> just forgot to apply it. Please go back to the previous discussion and
>>> either implement all requested changes or keep discussing them.
>>>
>>> Thank you.
>>> </form letter>
>>>
>>>
>>> Best regards,
>>> Krzysztof
>>>
>> Hi Krzysztof,
>>
>> In patch1 we are trying to add reference of i2c-adapter, you suggested
>> to use i2c-bus for that. we got comments on the driver code not to use
>> adapter and instead use i2c client reference. I felt i2c-bus is not
>> ideal to represent i2c client device so used this name.
> 
> You did not respond to comment of using i2c-bus, just silently decided
> to implement other property.
> 
I should have replied to v1 why we are not using this suggested way,
next time onwards I will fallow that.

> Anyway, why i2c-bus is not suitable here? I am quite surprised...
>
It was suggested by bjorn andresson in the offline review, I will check
this and get back.

- Krishna Chaitanya.
> 
> 
> Best regards,
> Krzysztof
>
Krishna chaitanya chundru Aug. 5, 2024, 5:57 a.m. UTC | #12
On 8/5/2024 10:58 AM, Krzysztof Kozlowski wrote:
> On 05/08/2024 07:26, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 8/5/2024 10:44 AM, Krzysztof Kozlowski wrote:
>>> On 05/08/2024 06:11, Krishna Chaitanya Chundru wrote:
>>>
>>>
>>>>>> +
>>>>>> +  qcom,nfts:
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint8
>>>>>> +    description:
>>>>>> +      Fast Training Sequence (FTS) is the mechanism that
>>>>>> +      is used for bit and Symbol lock.
>>>>>
>>>>> What are the values? Why this is uint8?
>>>>>
>>>> These represents number of fast training sequence and doesn't have
>>>> any units and the maximum value for this is 0xFF only so we used
>>>> uint8.
>>>>> You described the desired Linux feature or behavior, not the actual
>>>>> hardware. The bindings are about the latter, so instead you need to
>>>>> rephrase the property and its description to match actual hardware
>>>>> capabilities/features/configuration etc.
>>>> ack.
>>>>>
>>>>>> +
>>>>>> +allOf:
>>>>>> +  - $ref: /schemas/pci/pci-bus-common.yaml#
>>>>>> +  - if:
>>>>>> +      properties:
>>>>>> +        compatible:
>>>>>> +          contains:
>>>>>> +            const: pci1179,0623
>>>>>> +      required:
>>>>>> +        - compatible
>>>>>
>>>>> Why do you have entire if? You do not have multiple variants, drop.
>>>>>
>>>> The child nodes also referencing the qcom,qps615.yaml# node, I tried
>>>> to use this way to say "the below properties are for the required for
>>>> parent and optional for child".
>>>
>>> I don't understand how child device can be exactly the same as parent
>>> device. How does it look in terms of hardware? Pins and supplies?
>>>
>>>>>> +    then:
>>>>>> +      required:
>>>>>> +        - vdd18-supply
>>>>>> +        - vdd09-supply
>>>>>> +        - vddc-supply
>>>>>> +        - vddio1-supply
>>>>>> +        - vddio2-supply
>>>>>> +        - vddio18-supply
>>>>>> +        - qcom,qps615-controller
>>>>>> +        - reset-gpios
>>>>>> +
>>>>>> +patternProperties:
>>>>>> +  "@1?[0-9a-f](,[0-7])?$":
>>>>>> +    type: object
>>>>>> +    $ref: qcom,qps615.yaml#
>>>>>> +    additionalProperties: true
>>>>>
>>>>> Nope, drop pattern Properties or explain what is this.
>>>>>
>>>> the child nodes represent the downstream ports of the PCIe
>>>> switch which wants to use same properties that is why
>>>> I tried to use this pattern properties.
>>>
>>> Downstream port is not the same as device. Why downstream port has the
>>> same supplies? To which pins are they connected?
>>>
>>>
>> Hi Krzysztof,
>>
>> Downstream ports dosen't have pins or supplies to power on.
>>
>> But there are properties like qcom,l0s-entry-delay-ns,
>> qcom,l1-entry-delay-ns,  qcom,tx-amplitude-millivolt etc which
>> applicable for child nodes also. Instead of re-declaring the
>> these properties again I tried to use pattern properties.
> 
> You could use $defs for them, but I don't understand how does these
> properties apply for both main device and ports. It seems you are
> writing binding to match some driver behavior. Let's start from basics -
> describe the hardware.
> 
Hi Krzysztof,

QPS615 has a 3 downstream ports and 1 upstream port as described below
diagram.
For this entire switch there are some supplies which we described in the
dt-binding (vdd18-supply, vdd09-supply etc) and one GPIO which controls
reset of the switch (reset-gpio). The switch hardware can configure the
individual ports DSP0, DSP1, DSP2, upstream port and also one integrated
ethernet endpoint which is connected to DSP2(I didn't mentioned in the
diagram) through I2C.

The properties other than supplies,i2c client, reset gpio which
are added will be applicable for all the ports.
Krzysztof Kozlowski Aug. 5, 2024, 2:43 p.m. UTC | #13
On 05/08/2024 07:57, Krishna Chaitanya Chundru wrote:
>>
> Hi Krzysztof,
> 
> QPS615 has a 3 downstream ports and 1 upstream port as described below
> diagram.
> For this entire switch there are some supplies which we described in the
> dt-binding (vdd18-supply, vdd09-supply etc) and one GPIO which controls
> reset of the switch (reset-gpio). The switch hardware can configure the
> individual ports DSP0, DSP1, DSP2, upstream port and also one integrated
> ethernet endpoint which is connected to DSP2(I didn't mentioned in the
> diagram) through I2C.
> 
> The properties other than supplies,i2c client, reset gpio which
> are added will be applicable for all the ports.
> _______________________________________________________________
> |   |i2c|                   QPS615       |Supplies||Resx gpio |
> |   |___|              _________________ |________||__________|
> |      ________________| Upstream port |_____________         |
> |      |               |_______________|            |         |
> |      |                       |                    |         |
> |      |                       |                    |         |
> |  ____|_____              ____|_____            ___|____     |
> |  |DSP 0   |              | DSP 1  |            | DSP 2|     |
> |  |________|              |________|            |______|     |
> |_____________________________________________________________|
> 

I don't get why then properties should apply to main device node.

Best regards,
Krzysztof
Bjorn Andersson Aug. 5, 2024, 4:39 p.m. UTC | #14
On Mon, Aug 05, 2024 at 07:12:34AM +0200, Krzysztof Kozlowski wrote:
> On 05/08/2024 06:02, Krishna Chaitanya Chundru wrote:
> > 
> > 
> > On 8/4/2024 2:26 PM, Krzysztof Kozlowski wrote:
> >> On 03/08/2024 05:22, 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       | 191 +++++++++++++++++++++
> >>>   1 file changed, 191 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..ea0c953ee56f
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
> >>> @@ -0,0 +1,191 @@
> >>> +# 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
> >>> +
> >>> +  qcom,qps615-controller:
> >>
> >> and now I see that you totally ignored comments. Repeating the same over
> >> and over is a waste of time.
> >>
> >> <form letter>
> >> This is a friendly reminder during the review process.
> >>
> >> It seems my or other reviewer's previous comments were not fully
> >> addressed. Maybe the feedback got lost between the quotes, maybe you
> >> just forgot to apply it. Please go back to the previous discussion and
> >> either implement all requested changes or keep discussing them.
> >>
> >> Thank you.

Well, thank you for the rant. Very helpful indeed.

> >> </form letter>
> >>
> >>
> >> Best regards,
> >> Krzysztof
> >>
> > Hi Krzysztof,
> > 
> > In patch1 we are trying to add reference of i2c-adapter, you suggested
> > to use i2c-bus for that. we got comments on the driver code not to use
> > adapter and instead use i2c client reference. I felt i2c-bus is not
> > ideal to represent i2c client device so used this name.
> 
> You did not respond to comment of using i2c-bus, just silently decided
> to implement other property.
> 

I guess you totally ignored my comment when you reviewed the previous
version, where I asked him to represent the device on said bus.

> Anyway, why i2c-bus is not suitable here? I am quite surprised...
> 

I was not aware that i2c-bus was an acceptable solution, sorry for my
bad suggestion and guidance here.

Regards,
Bjorn

> 
> 
> Best regards,
> Krzysztof
> 
>
Krzysztof Kozlowski Aug. 5, 2024, 4:58 p.m. UTC | #15
On 05/08/2024 18:39, Bjorn Andersson wrote:
>>>
>>> In patch1 we are trying to add reference of i2c-adapter, you suggested
>>> to use i2c-bus for that. we got comments on the driver code not to use
>>> adapter and instead use i2c client reference. I felt i2c-bus is not
>>> ideal to represent i2c client device so used this name.
>>
>> You did not respond to comment of using i2c-bus, just silently decided
>> to implement other property.
>>
> 
> I guess you totally ignored my comment when you reviewed the previous
> version, where I asked him to represent the device on said bus.

Hm, Rob suggested i2c-bus, you as well:
<<I'd prefer you call it "i2c-adapter" or perhaps "i2c-bus", because
it's not "the switch controller".>>

and there was no response to any of these comments.

> 
>> Anyway, why i2c-bus is not suitable here? I am quite surprised...
>>
> 
> I was not aware that i2c-bus was an acceptable solution, sorry for my
> bad suggestion and guidance here.

I think you suggested i2c-bus as well, but regardless what did you agree
internally, response to Rob was expected.

Best regards,
Krzysztof
Bjorn Andersson Aug. 5, 2024, 5:07 p.m. UTC | #16
On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote:
> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote:
> > On 03/08/2024 05:22, Krishna chaitanya chundru wrote:
> > > diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
[..]
> > > +  qps615,axi-clk-freq-hz:
> > > +    description:
> > > +      AXI clock which internal bus of the switch.
> > 
> > No need, use CCF.
> > 
> ack

This is a clock that's internal to the QPS615, so there's no clock
controller involved and hence I don't think CCF is applicable.

Regards,
Bjorn
Krzysztof Kozlowski Aug. 5, 2024, 5:18 p.m. UTC | #17
On 05/08/2024 19:07, Bjorn Andersson wrote:
> On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote:
>> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote:
>>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote:
>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
> [..]
>>>> +  qps615,axi-clk-freq-hz:
>>>> +    description:
>>>> +      AXI clock which internal bus of the switch.
>>>
>>> No need, use CCF.
>>>
>> ack
> 
> This is a clock that's internal to the QPS615, so there's no clock
> controller involved and hence I don't think CCF is applicable.

AXI does not sound that internal. DT rarely needs to specify internal
clock rates. What if you want to define rates for 20 clocks? Even
clock-frequency is deprecated, so why this would be allowed?
bus-frequency is allowed for buses, but that's not the case here, I guess?

Best regards,
Krzysztof
Manivannan Sadhasivam Aug. 8, 2024, 12:01 p.m. UTC | #18
On Mon, Aug 05, 2024 at 07:18:04PM +0200, Krzysztof Kozlowski wrote:
> On 05/08/2024 19:07, Bjorn Andersson wrote:
> > On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote:
> >> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote:
> >>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote:
> >>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
> > [..]
> >>>> +  qps615,axi-clk-freq-hz:
> >>>> +    description:
> >>>> +      AXI clock which internal bus of the switch.
> >>>
> >>> No need, use CCF.
> >>>
> >> ack
> > 
> > This is a clock that's internal to the QPS615, so there's no clock
> > controller involved and hence I don't think CCF is applicable.
> 
> AXI does not sound that internal.

Well, AXI is applicable to whatever entity that implements it. We mostly seen it
in ARM SoCs (host), but in this case the PCIe switch also has a microcontroller
/processor of some sort, so AXI is indeed relevant for it. The naming actually
comes from the switch's i2c register name that is being configured in the driver
based on this property value.

> DT rarely needs to specify internal
> clock rates. What if you want to define rates for 20 clocks? Even
> clock-frequency is deprecated, so why this would be allowed?
> bus-frequency is allowed for buses, but that's not the case here, I guess?
> 

This clock frequency is for the switch's internal AXI bus that runs at default
200MHz. And this property is used to specify a frequency that is configured over
the i2c interface so that the switch's AXI bus can operate in a low frequency
there by reducing the power consumption of the switch.

It is not strictly needed for the switch operation, but for power optimization.
So this property can also be dropped for the initial submission and added later
if you prefer.

- Mani
Krzysztof Kozlowski Aug. 8, 2024, 12:13 p.m. UTC | #19
On 08/08/2024 14:01, Manivannan Sadhasivam wrote:
> On Mon, Aug 05, 2024 at 07:18:04PM +0200, Krzysztof Kozlowski wrote:
>> On 05/08/2024 19:07, Bjorn Andersson wrote:
>>> On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote:
>>>> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote:
>>>>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote:
>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
>>> [..]
>>>>>> +  qps615,axi-clk-freq-hz:
>>>>>> +    description:
>>>>>> +      AXI clock which internal bus of the switch.
>>>>>
>>>>> No need, use CCF.
>>>>>
>>>> ack
>>>
>>> This is a clock that's internal to the QPS615, so there's no clock
>>> controller involved and hence I don't think CCF is applicable.
>>
>> AXI does not sound that internal.
> 
> Well, AXI is applicable to whatever entity that implements it. We mostly seen it
> in ARM SoCs (host), but in this case the PCIe switch also has a microcontroller
> /processor of some sort, so AXI is indeed relevant for it. The naming actually
> comes from the switch's i2c register name that is being configured in the driver
> based on this property value.
> 
>> DT rarely needs to specify internal
>> clock rates. What if you want to define rates for 20 clocks? Even
>> clock-frequency is deprecated, so why this would be allowed?
>> bus-frequency is allowed for buses, but that's not the case here, I guess?
>>
> 
> This clock frequency is for the switch's internal AXI bus that runs at default
> 200MHz. And this property is used to specify a frequency that is configured over
> the i2c interface so that the switch's AXI bus can operate in a low frequency
> there by reducing the power consumption of the switch.
> 
> It is not strictly needed for the switch operation, but for power optimization.
> So this property can also be dropped for the initial submission and added later
> if you prefer.

So if the clock rate can change, why this is static in DTB? Or why this
is configurable per-board?

There is a reason why clock-frequency property is not welcomed and you
are re-implementing it.

Best regards,
Krzysztof
Manivannan Sadhasivam Aug. 8, 2024, 12:41 p.m. UTC | #20
On Thu, Aug 08, 2024 at 02:13:01PM +0200, Krzysztof Kozlowski wrote:
> On 08/08/2024 14:01, Manivannan Sadhasivam wrote:
> > On Mon, Aug 05, 2024 at 07:18:04PM +0200, Krzysztof Kozlowski wrote:
> >> On 05/08/2024 19:07, Bjorn Andersson wrote:
> >>> On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote:
> >>>> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote:
> >>>>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote:
> >>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
> >>> [..]
> >>>>>> +  qps615,axi-clk-freq-hz:
> >>>>>> +    description:
> >>>>>> +      AXI clock which internal bus of the switch.
> >>>>>
> >>>>> No need, use CCF.
> >>>>>
> >>>> ack
> >>>
> >>> This is a clock that's internal to the QPS615, so there's no clock
> >>> controller involved and hence I don't think CCF is applicable.
> >>
> >> AXI does not sound that internal.
> > 
> > Well, AXI is applicable to whatever entity that implements it. We mostly seen it
> > in ARM SoCs (host), but in this case the PCIe switch also has a microcontroller
> > /processor of some sort, so AXI is indeed relevant for it. The naming actually
> > comes from the switch's i2c register name that is being configured in the driver
> > based on this property value.
> > 
> >> DT rarely needs to specify internal
> >> clock rates. What if you want to define rates for 20 clocks? Even
> >> clock-frequency is deprecated, so why this would be allowed?
> >> bus-frequency is allowed for buses, but that's not the case here, I guess?
> >>
> > 
> > This clock frequency is for the switch's internal AXI bus that runs at default
> > 200MHz. And this property is used to specify a frequency that is configured over
> > the i2c interface so that the switch's AXI bus can operate in a low frequency
> > there by reducing the power consumption of the switch.
> > 
> > It is not strictly needed for the switch operation, but for power optimization.
> > So this property can also be dropped for the initial submission and added later
> > if you prefer.
> 
> So if the clock rate can change, why this is static in DTB? Or why this
> is configurable per-board?
> 

Because, board manufacturers can change the frequency depending on the switch
configuration (enablement of DSP's etc...)

> There is a reason why clock-frequency property is not welcomed and you
> are re-implementing it.
> 

Hmm, I'm not aware that 'clock-frequency' is not encouraged these days. So you
are suggesting to change the rate in the driver itself based on the switch
configuration? If so, what difference does it make?

And no more *-freq properties are allowed?

- Mani
Krzysztof Kozlowski Aug. 8, 2024, 1:06 p.m. UTC | #21
On 08/08/2024 14:41, Manivannan Sadhasivam wrote:
> On Thu, Aug 08, 2024 at 02:13:01PM +0200, Krzysztof Kozlowski wrote:
>> On 08/08/2024 14:01, Manivannan Sadhasivam wrote:
>>> On Mon, Aug 05, 2024 at 07:18:04PM +0200, Krzysztof Kozlowski wrote:
>>>> On 05/08/2024 19:07, Bjorn Andersson wrote:
>>>>> On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote:
>>>>>> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote:
>>>>>>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote:
>>>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
>>>>> [..]
>>>>>>>> +  qps615,axi-clk-freq-hz:
>>>>>>>> +    description:
>>>>>>>> +      AXI clock which internal bus of the switch.
>>>>>>>
>>>>>>> No need, use CCF.
>>>>>>>
>>>>>> ack
>>>>>
>>>>> This is a clock that's internal to the QPS615, so there's no clock
>>>>> controller involved and hence I don't think CCF is applicable.
>>>>
>>>> AXI does not sound that internal.
>>>
>>> Well, AXI is applicable to whatever entity that implements it. We mostly seen it
>>> in ARM SoCs (host), but in this case the PCIe switch also has a microcontroller
>>> /processor of some sort, so AXI is indeed relevant for it. The naming actually
>>> comes from the switch's i2c register name that is being configured in the driver
>>> based on this property value.
>>>
>>>> DT rarely needs to specify internal
>>>> clock rates. What if you want to define rates for 20 clocks? Even
>>>> clock-frequency is deprecated, so why this would be allowed?
>>>> bus-frequency is allowed for buses, but that's not the case here, I guess?
>>>>
>>>
>>> This clock frequency is for the switch's internal AXI bus that runs at default
>>> 200MHz. And this property is used to specify a frequency that is configured over
>>> the i2c interface so that the switch's AXI bus can operate in a low frequency
>>> there by reducing the power consumption of the switch.
>>>
>>> It is not strictly needed for the switch operation, but for power optimization.
>>> So this property can also be dropped for the initial submission and added later
>>> if you prefer.
>>
>> So if the clock rate can change, why this is static in DTB? Or why this
>> is configurable per-board?
>>
> 
> Because, board manufacturers can change the frequency depending on the switch
> configuration (enablement of DSP's etc...)
> 
>> There is a reason why clock-frequency property is not welcomed and you
>> are re-implementing it.
>>
> 
> Hmm, I'm not aware that 'clock-frequency' is not encouraged these days. So you
> are suggesting to change the rate in the driver itself based on the switch
> configuration? If so, what difference does it make?

Based on the switch, other clocks, votes etc. whatever is reasonable
there. In most cases, not sure if this one here as well, devices can
operate on different clock frequencies thus specifying fixed frequency
in the DTS is simplification and lack of flexibility. It is chosen by
people only because it is easier for them but then they come back with
ABI issues when it turns out they need to switch to some dynamic control.

> 
> And no more *-freq properties are allowed?

bus-frequency is allowed for busses.

Best regards,
Krzysztof
Manivannan Sadhasivam Aug. 8, 2024, 1:29 p.m. UTC | #22
On Thu, Aug 08, 2024 at 03:06:28PM +0200, Krzysztof Kozlowski wrote:
> On 08/08/2024 14:41, Manivannan Sadhasivam wrote:
> > On Thu, Aug 08, 2024 at 02:13:01PM +0200, Krzysztof Kozlowski wrote:
> >> On 08/08/2024 14:01, Manivannan Sadhasivam wrote:
> >>> On Mon, Aug 05, 2024 at 07:18:04PM +0200, Krzysztof Kozlowski wrote:
> >>>> On 05/08/2024 19:07, Bjorn Andersson wrote:
> >>>>> On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote:
> >>>>>> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote:
> >>>>>>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote:
> >>>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
> >>>>> [..]
> >>>>>>>> +  qps615,axi-clk-freq-hz:
> >>>>>>>> +    description:
> >>>>>>>> +      AXI clock which internal bus of the switch.
> >>>>>>>
> >>>>>>> No need, use CCF.
> >>>>>>>
> >>>>>> ack
> >>>>>
> >>>>> This is a clock that's internal to the QPS615, so there's no clock
> >>>>> controller involved and hence I don't think CCF is applicable.
> >>>>
> >>>> AXI does not sound that internal.
> >>>
> >>> Well, AXI is applicable to whatever entity that implements it. We mostly seen it
> >>> in ARM SoCs (host), but in this case the PCIe switch also has a microcontroller
> >>> /processor of some sort, so AXI is indeed relevant for it. The naming actually
> >>> comes from the switch's i2c register name that is being configured in the driver
> >>> based on this property value.
> >>>
> >>>> DT rarely needs to specify internal
> >>>> clock rates. What if you want to define rates for 20 clocks? Even
> >>>> clock-frequency is deprecated, so why this would be allowed?
> >>>> bus-frequency is allowed for buses, but that's not the case here, I guess?
> >>>>
> >>>
> >>> This clock frequency is for the switch's internal AXI bus that runs at default
> >>> 200MHz. And this property is used to specify a frequency that is configured over
> >>> the i2c interface so that the switch's AXI bus can operate in a low frequency
> >>> there by reducing the power consumption of the switch.
> >>>
> >>> It is not strictly needed for the switch operation, but for power optimization.
> >>> So this property can also be dropped for the initial submission and added later
> >>> if you prefer.
> >>
> >> So if the clock rate can change, why this is static in DTB? Or why this
> >> is configurable per-board?
> >>
> > 
> > Because, board manufacturers can change the frequency depending on the switch
> > configuration (enablement of DSP's etc...)
> > 
> >> There is a reason why clock-frequency property is not welcomed and you
> >> are re-implementing it.
> >>
> > 
> > Hmm, I'm not aware that 'clock-frequency' is not encouraged these days. So you
> > are suggesting to change the rate in the driver itself based on the switch
> > configuration? If so, what difference does it make?
> 
> Based on the switch, other clocks, votes etc. whatever is reasonable
> there. In most cases, not sure if this one here as well, devices can
> operate on different clock frequencies thus specifying fixed frequency
> in the DTS is simplification and lack of flexibility. It is chosen by
> people only because it is easier for them but then they come back with
> ABI issues when it turns out they need to switch to some dynamic control.
> 

Atleast in this case, the requirement is to just set the frequency based on
switch configuration and not change it dynamically.

Krishna, is it possible to set the freq in driver by detecting the switch
configuration? I believe the freq is based on number of DSPs enabled?

> > 
> > And no more *-freq properties are allowed?
> 
> bus-frequency is allowed for busses.
> 

Okay.

- Mani
Manivannan Sadhasivam Aug. 22, 2024, 2:09 p.m. UTC | #23
On Thu, Aug 08, 2024 at 03:06:28PM +0200, Krzysztof Kozlowski wrote:
> On 08/08/2024 14:41, Manivannan Sadhasivam wrote:
> > On Thu, Aug 08, 2024 at 02:13:01PM +0200, Krzysztof Kozlowski wrote:
> >> On 08/08/2024 14:01, Manivannan Sadhasivam wrote:
> >>> On Mon, Aug 05, 2024 at 07:18:04PM +0200, Krzysztof Kozlowski wrote:
> >>>> On 05/08/2024 19:07, Bjorn Andersson wrote:
> >>>>> On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote:
> >>>>>> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote:
> >>>>>>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote:
> >>>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
> >>>>> [..]
> >>>>>>>> +  qps615,axi-clk-freq-hz:
> >>>>>>>> +    description:
> >>>>>>>> +      AXI clock which internal bus of the switch.
> >>>>>>>
> >>>>>>> No need, use CCF.
> >>>>>>>
> >>>>>> ack
> >>>>>
> >>>>> This is a clock that's internal to the QPS615, so there's no clock
> >>>>> controller involved and hence I don't think CCF is applicable.
> >>>>
> >>>> AXI does not sound that internal.
> >>>
> >>> Well, AXI is applicable to whatever entity that implements it. We mostly seen it
> >>> in ARM SoCs (host), but in this case the PCIe switch also has a microcontroller
> >>> /processor of some sort, so AXI is indeed relevant for it. The naming actually
> >>> comes from the switch's i2c register name that is being configured in the driver
> >>> based on this property value.
> >>>
> >>>> DT rarely needs to specify internal
> >>>> clock rates. What if you want to define rates for 20 clocks? Even
> >>>> clock-frequency is deprecated, so why this would be allowed?
> >>>> bus-frequency is allowed for buses, but that's not the case here, I guess?
> >>>>
> >>>
> >>> This clock frequency is for the switch's internal AXI bus that runs at default
> >>> 200MHz. And this property is used to specify a frequency that is configured over
> >>> the i2c interface so that the switch's AXI bus can operate in a low frequency
> >>> there by reducing the power consumption of the switch.
> >>>
> >>> It is not strictly needed for the switch operation, but for power optimization.
> >>> So this property can also be dropped for the initial submission and added later
> >>> if you prefer.
> >>
> >> So if the clock rate can change, why this is static in DTB? Or why this
> >> is configurable per-board?
> >>
> > 
> > Because, board manufacturers can change the frequency depending on the switch
> > configuration (enablement of DSP's etc...)
> > 
> >> There is a reason why clock-frequency property is not welcomed and you
> >> are re-implementing it.
> >>
> > 
> > Hmm, I'm not aware that 'clock-frequency' is not encouraged these days. So you
> > are suggesting to change the rate in the driver itself based on the switch
> > configuration? If so, what difference does it make?
> 
> Based on the switch, other clocks, votes etc. whatever is reasonable
> there. In most cases, not sure if this one here as well, devices can
> operate on different clock frequencies thus specifying fixed frequency
> in the DTS is simplification and lack of flexibility. It is chosen by
> people only because it is easier for them but then they come back with
> ABI issues when it turns out they need to switch to some dynamic control.
> 

Atleast for this device, this frequency is going to be static. Because, the
device itself cannot change the frequency, only the host driver can. That too is
only possible before enumerating the device. So there is no way the frequency is
going to change dynamically.

- Mani
Manivannan Sadhasivam Aug. 22, 2024, 2:16 p.m. UTC | #24
On Mon, Aug 05, 2024 at 04:43:47PM +0200, Krzysztof Kozlowski wrote:
> On 05/08/2024 07:57, Krishna Chaitanya Chundru wrote:
> >>
> > Hi Krzysztof,
> > 
> > QPS615 has a 3 downstream ports and 1 upstream port as described below
> > diagram.
> > For this entire switch there are some supplies which we described in the
> > dt-binding (vdd18-supply, vdd09-supply etc) and one GPIO which controls
> > reset of the switch (reset-gpio). The switch hardware can configure the
> > individual ports DSP0, DSP1, DSP2, upstream port and also one integrated
> > ethernet endpoint which is connected to DSP2(I didn't mentioned in the
> > diagram) through I2C.
> > 
> > The properties other than supplies,i2c client, reset gpio which
> > are added will be applicable for all the ports.
> > _______________________________________________________________
> > |   |i2c|                   QPS615       |Supplies||Resx gpio |
> > |   |___|              _________________ |________||__________|
> > |      ________________| Upstream port |_____________         |
> > |      |               |_______________|            |         |
> > |      |                       |                    |         |
> > |      |                       |                    |         |
> > |  ____|_____              ____|_____            ___|____     |
> > |  |DSP 0   |              | DSP 1  |            | DSP 2|     |
> > |  |________|              |________|            |______|     |
> > |_____________________________________________________________|
> > 
> 
> I don't get why then properties should apply to main device node.
> 

The problem here is, we cannot differentiate between main device node and the
upstream node. Typically the differentiation is not needed because no one cared
about configuring the upstream port. But this PCIe switch is special (as like
most of the Qcom peripherals).

I agree that if we don't differentiate then it also implies that all main node
properties are applicable to upstream port and vice versa. But AFAIK, upstream
port is often considered as the _device_ itself as it shares the same bus
number.

- Mani
Krzysztof Kozlowski Aug. 23, 2024, 9:01 a.m. UTC | #25
On 22/08/2024 16:16, Manivannan Sadhasivam wrote:
> On Mon, Aug 05, 2024 at 04:43:47PM +0200, Krzysztof Kozlowski wrote:
>> On 05/08/2024 07:57, Krishna Chaitanya Chundru wrote:
>>>>
>>> Hi Krzysztof,
>>>
>>> QPS615 has a 3 downstream ports and 1 upstream port as described below
>>> diagram.
>>> For this entire switch there are some supplies which we described in the
>>> dt-binding (vdd18-supply, vdd09-supply etc) and one GPIO which controls
>>> reset of the switch (reset-gpio). The switch hardware can configure the
>>> individual ports DSP0, DSP1, DSP2, upstream port and also one integrated
>>> ethernet endpoint which is connected to DSP2(I didn't mentioned in the
>>> diagram) through I2C.
>>>
>>> The properties other than supplies,i2c client, reset gpio which
>>> are added will be applicable for all the ports.
>>> _______________________________________________________________
>>> |   |i2c|                   QPS615       |Supplies||Resx gpio |
>>> |   |___|              _________________ |________||__________|
>>> |      ________________| Upstream port |_____________         |
>>> |      |               |_______________|            |         |
>>> |      |                       |                    |         |
>>> |      |                       |                    |         |
>>> |  ____|_____              ____|_____            ___|____     |
>>> |  |DSP 0   |              | DSP 1  |            | DSP 2|     |
>>> |  |________|              |________|            |______|     |
>>> |_____________________________________________________________|
>>>
>>
>> I don't get why then properties should apply to main device node.
>>
> 
> The problem here is, we cannot differentiate between main device node and the
> upstream node. Typically the differentiation is not needed because no one cared
> about configuring the upstream port. But this PCIe switch is special (as like
> most of the Qcom peripherals).
> 
> I agree that if we don't differentiate then it also implies that all main node
> properties are applicable to upstream port and vice versa. But AFAIK, upstream
> port is often considered as the _device_ itself as it shares the same bus
> number.

Well, above diagram shows supplies being part of the entire device, not
each port. That's confusing. Based on diagram, downstream ports do not
have any supplies... and what exactly do they supply? Let's look at
vdd18 and vdd09 which sound main supplies of the entire device. In
context of port: what exactly do they power? Which part of the port?

Best regards,
Krzysztof
Krzysztof Kozlowski Aug. 23, 2024, 9:06 a.m. UTC | #26
On 22/08/2024 16:09, Manivannan Sadhasivam wrote:
> On Thu, Aug 08, 2024 at 03:06:28PM +0200, Krzysztof Kozlowski wrote:
>> On 08/08/2024 14:41, Manivannan Sadhasivam wrote:
>>> On Thu, Aug 08, 2024 at 02:13:01PM +0200, Krzysztof Kozlowski wrote:
>>>> On 08/08/2024 14:01, Manivannan Sadhasivam wrote:
>>>>> On Mon, Aug 05, 2024 at 07:18:04PM +0200, Krzysztof Kozlowski wrote:
>>>>>> On 05/08/2024 19:07, Bjorn Andersson wrote:
>>>>>>> On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>>> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote:
>>>>>>>>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote:
>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
>>>>>>> [..]
>>>>>>>>>> +  qps615,axi-clk-freq-hz:
>>>>>>>>>> +    description:
>>>>>>>>>> +      AXI clock which internal bus of the switch.
>>>>>>>>>
>>>>>>>>> No need, use CCF.
>>>>>>>>>
>>>>>>>> ack
>>>>>>>
>>>>>>> This is a clock that's internal to the QPS615, so there's no clock
>>>>>>> controller involved and hence I don't think CCF is applicable.
>>>>>>
>>>>>> AXI does not sound that internal.
>>>>>
>>>>> Well, AXI is applicable to whatever entity that implements it. We mostly seen it
>>>>> in ARM SoCs (host), but in this case the PCIe switch also has a microcontroller
>>>>> /processor of some sort, so AXI is indeed relevant for it. The naming actually
>>>>> comes from the switch's i2c register name that is being configured in the driver
>>>>> based on this property value.
>>>>>
>>>>>> DT rarely needs to specify internal
>>>>>> clock rates. What if you want to define rates for 20 clocks? Even
>>>>>> clock-frequency is deprecated, so why this would be allowed?
>>>>>> bus-frequency is allowed for buses, but that's not the case here, I guess?
>>>>>>
>>>>>
>>>>> This clock frequency is for the switch's internal AXI bus that runs at default
>>>>> 200MHz. And this property is used to specify a frequency that is configured over
>>>>> the i2c interface so that the switch's AXI bus can operate in a low frequency
>>>>> there by reducing the power consumption of the switch.
>>>>>
>>>>> It is not strictly needed for the switch operation, but for power optimization.
>>>>> So this property can also be dropped for the initial submission and added later
>>>>> if you prefer.
>>>>
>>>> So if the clock rate can change, why this is static in DTB? Or why this
>>>> is configurable per-board?
>>>>
>>>
>>> Because, board manufacturers can change the frequency depending on the switch
>>> configuration (enablement of DSP's etc...)
>>>
>>>> There is a reason why clock-frequency property is not welcomed and you
>>>> are re-implementing it.
>>>>
>>>
>>> Hmm, I'm not aware that 'clock-frequency' is not encouraged these days. So you
>>> are suggesting to change the rate in the driver itself based on the switch
>>> configuration? If so, what difference does it make?
>>
>> Based on the switch, other clocks, votes etc. whatever is reasonable
>> there. In most cases, not sure if this one here as well, devices can
>> operate on different clock frequencies thus specifying fixed frequency
>> in the DTS is simplification and lack of flexibility. It is chosen by
>> people only because it is easier for them but then they come back with
>> ABI issues when it turns out they need to switch to some dynamic control.
>>
> 
> Atleast for this device, this frequency is going to be static. Because, the
> device itself cannot change the frequency, only the host driver can. That too is
> only possible before enumerating the device. So there is no way the frequency is
> going to change dynamically.

We have assigned-clocks properties for this... but there are no clock
inputs here, so maybe it is not applicable? What generates this internal
AXI clock? Does it have internal oscillator?

So many questions and nothing in the property description helps to
understand this.

Best regards,
Krzysztof
Manivannan Sadhasivam Aug. 23, 2024, 9:40 a.m. UTC | #27
On Fri, Aug 23, 2024 at 11:06:28AM +0200, Krzysztof Kozlowski wrote:
> On 22/08/2024 16:09, Manivannan Sadhasivam wrote:
> > On Thu, Aug 08, 2024 at 03:06:28PM +0200, Krzysztof Kozlowski wrote:
> >> On 08/08/2024 14:41, Manivannan Sadhasivam wrote:
> >>> On Thu, Aug 08, 2024 at 02:13:01PM +0200, Krzysztof Kozlowski wrote:
> >>>> On 08/08/2024 14:01, Manivannan Sadhasivam wrote:
> >>>>> On Mon, Aug 05, 2024 at 07:18:04PM +0200, Krzysztof Kozlowski wrote:
> >>>>>> On 05/08/2024 19:07, Bjorn Andersson wrote:
> >>>>>>> On Mon, Aug 05, 2024 at 09:41:26AM +0530, Krishna Chaitanya Chundru wrote:
> >>>>>>>> On 8/4/2024 2:23 PM, Krzysztof Kozlowski wrote:
> >>>>>>>>> On 03/08/2024 05:22, Krishna chaitanya chundru wrote:
> >>>>>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
> >>>>>>> [..]
> >>>>>>>>>> +  qps615,axi-clk-freq-hz:
> >>>>>>>>>> +    description:
> >>>>>>>>>> +      AXI clock which internal bus of the switch.
> >>>>>>>>>
> >>>>>>>>> No need, use CCF.
> >>>>>>>>>
> >>>>>>>> ack
> >>>>>>>
> >>>>>>> This is a clock that's internal to the QPS615, so there's no clock
> >>>>>>> controller involved and hence I don't think CCF is applicable.
> >>>>>>
> >>>>>> AXI does not sound that internal.
> >>>>>
> >>>>> Well, AXI is applicable to whatever entity that implements it. We mostly seen it
> >>>>> in ARM SoCs (host), but in this case the PCIe switch also has a microcontroller
> >>>>> /processor of some sort, so AXI is indeed relevant for it. The naming actually
> >>>>> comes from the switch's i2c register name that is being configured in the driver
> >>>>> based on this property value.
> >>>>>
> >>>>>> DT rarely needs to specify internal
> >>>>>> clock rates. What if you want to define rates for 20 clocks? Even
> >>>>>> clock-frequency is deprecated, so why this would be allowed?
> >>>>>> bus-frequency is allowed for buses, but that's not the case here, I guess?
> >>>>>>
> >>>>>
> >>>>> This clock frequency is for the switch's internal AXI bus that runs at default
> >>>>> 200MHz. And this property is used to specify a frequency that is configured over
> >>>>> the i2c interface so that the switch's AXI bus can operate in a low frequency
> >>>>> there by reducing the power consumption of the switch.
> >>>>>
> >>>>> It is not strictly needed for the switch operation, but for power optimization.
> >>>>> So this property can also be dropped for the initial submission and added later
> >>>>> if you prefer.
> >>>>
> >>>> So if the clock rate can change, why this is static in DTB? Or why this
> >>>> is configurable per-board?
> >>>>
> >>>
> >>> Because, board manufacturers can change the frequency depending on the switch
> >>> configuration (enablement of DSP's etc...)
> >>>
> >>>> There is a reason why clock-frequency property is not welcomed and you
> >>>> are re-implementing it.
> >>>>
> >>>
> >>> Hmm, I'm not aware that 'clock-frequency' is not encouraged these days. So you
> >>> are suggesting to change the rate in the driver itself based on the switch
> >>> configuration? If so, what difference does it make?
> >>
> >> Based on the switch, other clocks, votes etc. whatever is reasonable
> >> there. In most cases, not sure if this one here as well, devices can
> >> operate on different clock frequencies thus specifying fixed frequency
> >> in the DTS is simplification and lack of flexibility. It is chosen by
> >> people only because it is easier for them but then they come back with
> >> ABI issues when it turns out they need to switch to some dynamic control.
> >>
> > 
> > Atleast for this device, this frequency is going to be static. Because, the
> > device itself cannot change the frequency, only the host driver can. That too is
> > only possible before enumerating the device. So there is no way the frequency is
> > going to change dynamically.
> 
> We have assigned-clocks properties for this... but there are no clock
> inputs here, so maybe it is not applicable? What generates this internal
> AXI clock? Does it have internal oscillator?
> 

I do not have access to the device schematics, but based on my knowledge there
should be an internal oscillator for the device. But the device also gets the
refclk from PCIe bus, but I don't know if that is somehow used as a parent of
AXI bus or not.

Going by the AXI bus terminology, I would assume that the frequency of this come
from the internal oscillator in the device.

And this is common on many PCIe devices, but they never mentioned in DT (well we
do not define PCIe devices in DT usually), but Qcom wants to control the
frequency of the device's internal clock to optimize the power consumption of
the device. Unfortunately, the device firmware is not doing its job as expected.

- Mani
Manivannan Sadhasivam Aug. 23, 2024, 9:44 a.m. UTC | #28
On Fri, Aug 23, 2024 at 11:01:37AM +0200, Krzysztof Kozlowski wrote:
> On 22/08/2024 16:16, Manivannan Sadhasivam wrote:
> > On Mon, Aug 05, 2024 at 04:43:47PM +0200, Krzysztof Kozlowski wrote:
> >> On 05/08/2024 07:57, Krishna Chaitanya Chundru wrote:
> >>>>
> >>> Hi Krzysztof,
> >>>
> >>> QPS615 has a 3 downstream ports and 1 upstream port as described below
> >>> diagram.
> >>> For this entire switch there are some supplies which we described in the
> >>> dt-binding (vdd18-supply, vdd09-supply etc) and one GPIO which controls
> >>> reset of the switch (reset-gpio). The switch hardware can configure the
> >>> individual ports DSP0, DSP1, DSP2, upstream port and also one integrated
> >>> ethernet endpoint which is connected to DSP2(I didn't mentioned in the
> >>> diagram) through I2C.
> >>>
> >>> The properties other than supplies,i2c client, reset gpio which
> >>> are added will be applicable for all the ports.
> >>> _______________________________________________________________
> >>> |   |i2c|                   QPS615       |Supplies||Resx gpio |
> >>> |   |___|              _________________ |________||__________|
> >>> |      ________________| Upstream port |_____________         |
> >>> |      |               |_______________|            |         |
> >>> |      |                       |                    |         |
> >>> |      |                       |                    |         |
> >>> |  ____|_____              ____|_____            ___|____     |
> >>> |  |DSP 0   |              | DSP 1  |            | DSP 2|     |
> >>> |  |________|              |________|            |______|     |
> >>> |_____________________________________________________________|
> >>>
> >>
> >> I don't get why then properties should apply to main device node.
> >>
> > 
> > The problem here is, we cannot differentiate between main device node and the
> > upstream node. Typically the differentiation is not needed because no one cared
> > about configuring the upstream port. But this PCIe switch is special (as like
> > most of the Qcom peripherals).
> > 
> > I agree that if we don't differentiate then it also implies that all main node
> > properties are applicable to upstream port and vice versa. But AFAIK, upstream
> > port is often considered as the _device_ itself as it shares the same bus
> > number.
> 
> Well, above diagram shows supplies being part of the entire device, not
> each port. That's confusing. Based on diagram, downstream ports do not
> have any supplies... and what exactly do they supply? Let's look at
> vdd18 and vdd09 which sound main supplies of the entire device. In
> context of port: what exactly do they power? Which part of the port?
> 

The supplies for the downstream ports are derived from the switch power supply
only. There is no way we can describe them as the port suppliers are internal to
the device.

- Mani
Krzysztof Kozlowski Aug. 23, 2024, 1:51 p.m. UTC | #29
On 23/08/2024 11:44, Manivannan Sadhasivam wrote:
> On Fri, Aug 23, 2024 at 11:01:37AM +0200, Krzysztof Kozlowski wrote:
>> On 22/08/2024 16:16, Manivannan Sadhasivam wrote:
>>> On Mon, Aug 05, 2024 at 04:43:47PM +0200, Krzysztof Kozlowski wrote:
>>>> On 05/08/2024 07:57, Krishna Chaitanya Chundru wrote:
>>>>>>
>>>>> Hi Krzysztof,
>>>>>
>>>>> QPS615 has a 3 downstream ports and 1 upstream port as described below
>>>>> diagram.
>>>>> For this entire switch there are some supplies which we described in the
>>>>> dt-binding (vdd18-supply, vdd09-supply etc) and one GPIO which controls
>>>>> reset of the switch (reset-gpio). The switch hardware can configure the
>>>>> individual ports DSP0, DSP1, DSP2, upstream port and also one integrated
>>>>> ethernet endpoint which is connected to DSP2(I didn't mentioned in the
>>>>> diagram) through I2C.
>>>>>
>>>>> The properties other than supplies,i2c client, reset gpio which
>>>>> are added will be applicable for all the ports.
>>>>> _______________________________________________________________
>>>>> |   |i2c|                   QPS615       |Supplies||Resx gpio |
>>>>> |   |___|              _________________ |________||__________|
>>>>> |      ________________| Upstream port |_____________         |
>>>>> |      |               |_______________|            |         |
>>>>> |      |                       |                    |         |
>>>>> |      |                       |                    |         |
>>>>> |  ____|_____              ____|_____            ___|____     |
>>>>> |  |DSP 0   |              | DSP 1  |            | DSP 2|     |
>>>>> |  |________|              |________|            |______|     |
>>>>> |_____________________________________________________________|
>>>>>
>>>>
>>>> I don't get why then properties should apply to main device node.
>>>>
>>>
>>> The problem here is, we cannot differentiate between main device node and the
>>> upstream node. Typically the differentiation is not needed because no one cared
>>> about configuring the upstream port. But this PCIe switch is special (as like
>>> most of the Qcom peripherals).
>>>
>>> I agree that if we don't differentiate then it also implies that all main node
>>> properties are applicable to upstream port and vice versa. But AFAIK, upstream
>>> port is often considered as the _device_ itself as it shares the same bus
>>> number.
>>
>> Well, above diagram shows supplies being part of the entire device, not
>> each port. That's confusing. Based on diagram, downstream ports do not
>> have any supplies... and what exactly do they supply? Let's look at
>> vdd18 and vdd09 which sound main supplies of the entire device. In
>> context of port: what exactly do they power? Which part of the port?
>>
> 
> The supplies for the downstream ports are derived from the switch power supply
> only. There is no way we can describe them as the port suppliers are internal to
> the device.

IIUC, this means supplies are not valid for downstream ports, so it is a
proof that binding is not correct. I don't get why we keep poking this
and get to the same conclusions I had 3 weeks ago.

Basically the binding is saying that downstream ports are identical to
the device. Including the aspect of having more downstream ports (so
device -> downstream ports -> downstream ports -> downstream ports ...
infinite). To remind that was my conclusion:

"Downstream port is not the same as device. Why downstream port has the
same supplies? To which pins are they connected?"

Best regards,
Krzysztof
Manivannan Sadhasivam Aug. 23, 2024, 3:11 p.m. UTC | #30
On Fri, Aug 23, 2024 at 03:51:25PM +0200, Krzysztof Kozlowski wrote:
> On 23/08/2024 11:44, Manivannan Sadhasivam wrote:
> > On Fri, Aug 23, 2024 at 11:01:37AM +0200, Krzysztof Kozlowski wrote:
> >> On 22/08/2024 16:16, Manivannan Sadhasivam wrote:
> >>> On Mon, Aug 05, 2024 at 04:43:47PM +0200, Krzysztof Kozlowski wrote:
> >>>> On 05/08/2024 07:57, Krishna Chaitanya Chundru wrote:
> >>>>>>
> >>>>> Hi Krzysztof,
> >>>>>
> >>>>> QPS615 has a 3 downstream ports and 1 upstream port as described below
> >>>>> diagram.
> >>>>> For this entire switch there are some supplies which we described in the
> >>>>> dt-binding (vdd18-supply, vdd09-supply etc) and one GPIO which controls
> >>>>> reset of the switch (reset-gpio). The switch hardware can configure the
> >>>>> individual ports DSP0, DSP1, DSP2, upstream port and also one integrated
> >>>>> ethernet endpoint which is connected to DSP2(I didn't mentioned in the
> >>>>> diagram) through I2C.
> >>>>>
> >>>>> The properties other than supplies,i2c client, reset gpio which
> >>>>> are added will be applicable for all the ports.
> >>>>> _______________________________________________________________
> >>>>> |   |i2c|                   QPS615       |Supplies||Resx gpio |
> >>>>> |   |___|              _________________ |________||__________|
> >>>>> |      ________________| Upstream port |_____________         |
> >>>>> |      |               |_______________|            |         |
> >>>>> |      |                       |                    |         |
> >>>>> |      |                       |                    |         |
> >>>>> |  ____|_____              ____|_____            ___|____     |
> >>>>> |  |DSP 0   |              | DSP 1  |            | DSP 2|     |
> >>>>> |  |________|              |________|            |______|     |
> >>>>> |_____________________________________________________________|
> >>>>>
> >>>>
> >>>> I don't get why then properties should apply to main device node.
> >>>>
> >>>
> >>> The problem here is, we cannot differentiate between main device node and the
> >>> upstream node. Typically the differentiation is not needed because no one cared
> >>> about configuring the upstream port. But this PCIe switch is special (as like
> >>> most of the Qcom peripherals).
> >>>
> >>> I agree that if we don't differentiate then it also implies that all main node
> >>> properties are applicable to upstream port and vice versa. But AFAIK, upstream
> >>> port is often considered as the _device_ itself as it shares the same bus
> >>> number.
> >>
> >> Well, above diagram shows supplies being part of the entire device, not
> >> each port. That's confusing. Based on diagram, downstream ports do not
> >> have any supplies... and what exactly do they supply? Let's look at
> >> vdd18 and vdd09 which sound main supplies of the entire device. In
> >> context of port: what exactly do they power? Which part of the port?
> >>
> > 
> > The supplies for the downstream ports are derived from the switch power supply
> > only. There is no way we can describe them as the port suppliers are internal to
> > the device.
> 
> IIUC, this means supplies are not valid for downstream ports, so it is a
> proof that binding is not correct. I don't get why we keep poking this
> and get to the same conclusions I had 3 weeks ago.
> 
> Basically the binding is saying that downstream ports are identical to
> the device. Including the aspect of having more downstream ports (so
> device -> downstream ports -> downstream ports -> downstream ports ...
> infinite). To remind that was my conclusion:
> 
> "Downstream port is not the same as device. Why downstream port has the
> same supplies? To which pins are they connected?"
> 

Ok. I seem to have missed your above comment and you are right. I was just
clarifying the upstream port discussion as we cannot differentiate between
upstream port and main device node.

For downstream port, I hope Krishna will fix the binding.

- Mani
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
new file mode 100644
index 000000000000..ea0c953ee56f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
@@ -0,0 +1,191 @@ 
+# 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
+
+  qcom,qps615-controller:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Reference to the I2C client used to do configure qps615
+
+  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.
+
+  qps615,axi-clk-freq-hz:
+    description:
+      AXI clock which internal bus of the switch.
+
+  qcom,l0s-entry-delay-ns:
+    description: Aspm l0s entry delay in nanoseconds.
+
+  qcom,l1-entry-delay-ns:
+    description: Aspm l1 entry delay in nanoseconds.
+
+  qcom,tx-amplitude-millivolt:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Change Tx Margin setting for low power consumption.
+
+  qcom,no-dfe:
+    type: boolean
+    description: Disables DFE (Decision Feedback Equalizer).
+
+  qcom,nfts:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description:
+      Fast Training Sequence (FTS) is the mechanism that
+      is used for bit and Symbol lock.
+
+allOf:
+  - $ref: /schemas/pci/pci-bus-common.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: pci1179,0623
+      required:
+        - compatible
+    then:
+      required:
+        - vdd18-supply
+        - vdd09-supply
+        - vddc-supply
+        - vddio1-supply
+        - vddio2-supply
+        - vddio18-supply
+        - qcom,qps615-controller
+        - reset-gpios
+
+patternProperties:
+  "@1?[0-9a-f](,[0-7])?$":
+    type: object
+    $ref: qcom,qps615.yaml#
+    additionalProperties: true
+
+additionalProperties: true
+
+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;
+
+            pcie@0,0 {
+                compatible = "pci1179,0623";
+                reg = <0x10000 0x0 0x0 0x0 0x0>;
+                device_type = "pci";
+                #address-cells = <3>;
+                #size-cells = <2>;
+                ranges;
+
+                qcom,qps615-controller = <&qps615_controller>;
+
+                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;
+
+                    qcom,no-dfe;
+                };
+
+                pcie@2,0 {
+                    reg = <0x21000 0x0 0x0 0x0 0x0>;
+                    #address-cells = <3>;
+                    #size-cells = <2>;
+                    device_type = "pci";
+                    ranges;
+
+                    qcom,nfts = /bits/ 8 <10>;
+                };
+
+                pcie@3,0 {
+                    reg = <0x21800 0x0 0x0 0x0 0x0>;
+                    #address-cells = <3>;
+                    #size-cells = <2>;
+                    device_type = "pci";
+                    ranges;
+
+                    qcom,tx-amplitude-millivolt = <10>;
+
+                         pcie@0,0 {
+                              reg = <0x40000 0x0 0x0 0x0 0x0>;
+                              #address-cells = <3>;
+                              #size-cells = <2>;
+                              device_type = "pci";
+                              ranges;
+
+                              qcom,l1-entry-delay-ns = <10>;
+                         };
+
+                         pcie@0,1 {
+                              reg = <0x40100 0x0 0x0 0x0 0x0>;
+                              #address-cells = <3>;
+                              #size-cells = <2>;
+                              device_type = "pci";
+                              ranges;
+
+                              qcom,l0s-entry-delay-ns = <10>;
+                         };
+                };
+            };
+        };
+    };