[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <83337e51-6554-6543-059d-c71a50601b09@quicinc.com>
Date: Wed, 4 Dec 2024 14:19:56 +0530
From: Krishna Chaitanya Chundru <quic_krichai@...cinc.com>
To: Rob Herring <robh@...nel.org>
CC: <andersson@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
"Lorenzo
Pieralisi" <lpieralisi@...nel.org>,
Krzysztof Wilczyński
<kw@...ux.com>,
Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>,
<cros-qcom-dts-watchers@...omium.org>,
Jingoo Han <jingoohan1@...il.com>, Bartosz Golaszewski <brgl@...ev.pl>,
<quic_vbadigan@...cinc.com>, <linux-arm-msm@...r.kernel.org>,
<linux-pci@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/6] dt-bindings: PCI: Add binding for qps615
On 11/24/2024 7:02 AM, Krishna Chaitanya Chundru wrote:
>
>
> On 11/15/2024 9:48 PM, Rob Herring wrote:
>> On Tue, Nov 12, 2024 at 08:31:33PM +0530, Krishna chaitanya chundru
>> wrote:
>>> Add binding describing the Qualcomm PCIe switch, QPS615,
>>> which provides Ethernet MAC integrated to the 3rd downstream port
>>> and two downstream PCIe ports.
>>>
>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@...cinc.com>
>>> ---
>>> .../devicetree/bindings/pci/qcom,qps615.yaml | 205
>>> +++++++++++++++++++++
>>> 1 file changed, 205 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
>>> b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
>>> new file mode 100644
>>> index 000000000000..e6a63a0bb0f3
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
>>> @@ -0,0 +1,205 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/pci/qcom,qps615.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Qualcomm QPS615 PCIe switch
>>> +
>>> +maintainers:
>>> + - Krishna chaitanya chundru <quic_krichai@...cinc.com>
>>> +
>>> +description: |
>>> + Qualcomm QPS615 PCIe switch has one upstream and three downstream
>>> + ports. The 3rd downstream port has integrated endpoint device of
>>> + Ethernet MAC. Other two downstream ports are supposed to connect
>>> + to external device.
>>> +
>>> + The QPS615 PCIe switch can be configured through I2C interface before
>>> + PCIe link is established to change FTS, ASPM related entry delays,
>>> + tx amplitude etc for better power efficiency and functionality.
>>> +
>>> +properties:
>>> + compatible:
>>> + enum:
>>> + - pci1179,0623
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + i2c-parent:
>>> + $ref: /schemas/types.yaml#/definitions/phandle-array
>>> + description: |
>>
>> Don't need '|' if no formatting to preserve.
>>
> ack
>>> + A phandle to the parent I2C node and the slave address of the
>>> device
>>> + used to do configure qps615 to change FTS, tx amplitude etc.
>>> + items:
>>> + - description: Phandle to the I2C controller node
>>> + - description: I2C slave address
>>> +
>>> + vdd18-supply: true
>>> +
>>> + vdd09-supply: true
>>> +
>>> + vddc-supply: true
>>> +
>>> + vddio1-supply: true
>>> +
>>> + vddio2-supply: true
>>> +
>>> + vddio18-supply: true
>>> +
>>> + reset-gpios:
>>> + maxItems: 1
>>> + description:
>>> + GPIO controlling the RESX# pin.
>>
>> Is the PERST# or something else?
>>
> it is not PERST GPIO, it is similar to PERST in terms
> of functionality which brings switch out from reset.
>>> +
>>> + qps615,axi-clk-freq-hz:
>>
>> qps615 is not a vendor prefix.
>>
>>> + description:
>>> + AXI clock rate which is internal bus of the switch
>>> + The switch only runs in two frequencies i.e 250MHz and 125MHz.
>>> + enum: [125000000, 250000000]
>>> +
>>> +allOf:
>>> + - $ref: "#/$defs/qps615-node"
>>> +
>>> +patternProperties:
>>> + "@1?[0-9a-f](,[0-7])?$":
>>
>> You have 3 ports. So isn't this fixed and limited to 0-2?
>>
> sure I will change it to below as suggested
> "@1?[0-3](,[0-1])?$"
>>> + description: child nodes describing the internal downstream ports
>>> + the qps615 switch.
>>
>> Please be consistent with starting after the ':' or on the next line.
>>
>> And start with capital C.
>>
>>
> ack
>
>>> + type: object
>>> + $ref: "#/$defs/qps615-node"
>>> + unevaluatedProperties: false
>>> +
>>> +$defs:
>>> + qps615-node:
>>> + type: object
>>> +
>>> + properties:
>>> + qcom,l0s-entry-delay-ns:
>>> + description: Aspm l0s entry delay.
>>> +
>>> + qcom,l1-entry-delay-ns:
>>> + description: Aspm l1 entry delay.
>>
>> These should probably be common being standard PCIe things. Though, why
>> are they needed? I'm sure the timing is defined by the PCIe spec, so
>> they are not compliant?
>>
> Usually the firmware in the endpoints/switches should do this these
> configurations. But the qps615 PCIe switch doesn't have any firmware
> running to configure these. So the hardware exposes i2c interface to
> configure these before link training.
>>> +
>>> + qcom,tx-amplitude-millivolt:
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + description: Change Tx Margin setting for low power
>>> consumption.
>>> +
>>> + qcom,no-dfe-support:
>>> + type: boolean
>>> + description: Disable DFE (Decision Feedback Equalizer),
>>> which mitigates
>>> + intersymbol interference and some reflections caused by
>>> impedance mismatches.
>>> +
>>> + qcom,nfts:
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + description:
>>> + Number of Fast Training Sequence (FTS) used during L0s to
>>> L0 exit
>>> + for bit and Symbol lock.
>>
>> Also something common.
>>
>> The problem I have with all these properties is you are using them on
>> both the upstream and downstream sides of the PCIe links. They belong in
>> either the device's node (downstream) or the bus's node (upstream).
>>
> This switch allows us to configure both upstream, downstream ports and
> also embedded Ethernet port which is internal to the switch. These
> properties are applicable for all of those.
>>> +
>>> + allOf:
>>> + - $ref: /schemas/pci/pci-bus.yaml#
>>
>> pci-pci-bridge.yaml is more specific and closer to what this device is.
>>
> I tried this now, I was getting warning saying the compatible
> /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: pcie@0,0: compatible: ['pci1179,0623'] does not contain items matching the given schema
> from schema $id:
> http://devicetree.org/schemas/pci/qcom,qps615.yaml#
> /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb: pcie@0,0: Unevaluated properties are not allowed ('#address-cells', '#size-cells', 'bus-range', 'device_type', 'ranges' were unexpected)
>
> I think pci-pci-bridge is expecting the compatible string in this format
> only "pciclass,0604".
>
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +required:
>>> + - vdd18-supply
>>> + - vdd09-supply
>>> + - vddc-supply
>>> + - vddio1-supply
>>> + - vddio2-supply
>>> + - vddio18-supply
>>> + - i2c-parent
>>> + - reset-gpios
>>> +
>>> +examples:
>>> + - |
>>> +
>>> + #include <dt-bindings/gpio/gpio.h>
>>> +
>>> + pcie {
>>> + #address-cells = <3>;
>>> + #size-cells = <2>;
>>> +
>>> + pcie@0 {
>>> + device_type = "pci";
>>> + reg = <0x0 0x0 0x0 0x0 0x0>;
>>> +
>>> + #address-cells = <3>;
>>> + #size-cells = <2>;
>>> + ranges;
>>> + bus-range = <0x01 0xff>;
>>> +
>>> + pcie@0,0 {
>>> + compatible = "pci1179,0623";
>>> + reg = <0x10000 0x0 0x0 0x0 0x0>;
>>> + device_type = "pci";
>>> + #address-cells = <3>;
>>> + #size-cells = <2>;
>>> + ranges;
>>> + bus-range = <0x02 0xff>;
>>> +
>>> + i2c-parent = <&qup_i2c 0x77>;
>>> +
>>> + vdd18-supply = <&vdd>;
>>> + vdd09-supply = <&vdd>;
>>> + vddc-supply = <&vdd>;
>>> + vddio1-supply = <&vdd>;
>>> + vddio2-supply = <&vdd>;
>>> + vddio18-supply = <&vdd>;
>>> +
>>> + reset-gpios = <&gpio 1 GPIO_ACTIVE_LOW>;
>>> +
>>> + pcie@1,0 {
>>> + reg = <0x20800 0x0 0x0 0x0 0x0>;
>>> + #address-cells = <3>;
>>> + #size-cells = <2>;
>>> + device_type = "pci";
>>> + ranges;
>>> + bus-range = <0x03 0xff>;
>>> +
>>> + qcom,no-dfe-support;
>>> + };
>>> +
>>> + pcie@2,0 {
>>> + reg = <0x21000 0x0 0x0 0x0 0x0>;
>>> + #address-cells = <3>;
>>> + #size-cells = <2>;
>>> + device_type = "pci";
>>> + ranges;
>>> + bus-range = <0x04 0xff>;
>>> +
>>> + qcom,nfts = <10>;
>>> + };
>>> +
>>> + pcie@3,0 {
>>> + reg = <0x21800 0x0 0x0 0x0 0x0>;
>>> + #address-cells = <3>;
>>> + #size-cells = <2>;
>>> + device_type = "pci";
>>> + ranges;
>>> + bus-range = <0x05 0xff>;
>>> +
>>> + qcom,tx-amplitude-millivolt = <10>;
>>> + pcie@0,0 {
>>> + reg = <0x50000 0x0 0x0 0x0 0x0>;
>>> + #address-cells = <3>;
>>> + #size-cells = <2>;
>>> + device_type = "pci";
>>
>> There's a 2nd PCI-PCI bridge?
> This the embedded ethernet port which is as part of DSP3.
>
Hi Rob,
Can you please check my response on your queries, if you need
any extra information, we can provide to sort this out.
- Krishna Chaitanya.
> - Krishna Chaitanya.
>>
>>> + ranges;
>>> +
>>> + qcom,l1-entry-delay-ns = <10>;
>>> + };
>>> +
>>> + pcie@0,1 {
>>> + reg = <0x50100 0x0 0x0 0x0 0x0>;
>>> + #address-cells = <3>;
>>> + #size-cells = <2>;
>>> + device_type = "pci";
>>> + ranges;
>>> +
>>> + qcom,l0s-entry-delay-ns = <10>;
>>> + };
>>> + };
>>> + };
>>> + };
>>> + };
>>>
>>> --
>>> 2.34.1
>>>
>
Powered by blists - more mailing lists