| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ac2e0e97-0499-ce4c-5856-16e7f887ce11@quicinc.com>
Date: Mon, 23 Dec 2024 22:15:51 +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 12/4/2024 2:19 PM, Krishna Chaitanya Chundru wrote:
>
>
> 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
A gentle ping as its been more than 2 weeks.
- 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