| 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: <97c4f884-8915-a919-d565-cf6e9f83d2e4@quicinc.com>
Date: Fri, 27 Dec 2024 07:44:49 +0530
From: Krishna Chaitanya Chundru <quic_krichai@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Rob Herring <robh@...nel.org>, <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/24/2024 12:27 AM, Dmitry Baryshkov wrote:
> On Sun, Nov 24, 2024 at 07:02:48AM +0530, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 11/15/2024 9:48 PM, Rob Herring wrote:
>>> On Tue, Nov 12, 2024 at 08:31:33PM +0530, Krishna chaitanya chundru wrote:
>>>> Add binding describing the Qualcomm PCIe switch, QPS615,
>>>> which provides Ethernet MAC integrated to the 3rd downstream port
>>>> and two downstream PCIe ports.
>>>>
>>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@...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.
>
> Do you have an actual PERST# on upstream facing port? Is it a separate
> wire? Judging by the RB3 Gen2 this line is being used as PERST#
>
>>>> +
>>>> + qps615,axi-clk-freq-hz:
>>>
>>> qps615 is not a vendor prefix.
>>>
>>>> + description:
>>>> + AXI clock rate which is internal bus of the switch
>>>> + The switch only runs in two frequencies i.e 250MHz and 125MHz.
>>>> + enum: [125000000, 250000000]
>>>> +
>>>> +allOf:
>>>> + - $ref: "#/$defs/qps615-node"
>>>> +
>>>> +patternProperties:
>>>> + "@1?[0-9a-f](,[0-7])?$":
>>>
>>> You have 3 ports. So isn't this fixed and limited to 0-2?
>>>
>> sure I will change it to below as suggested
>> "@1?[0-3](,[0-1])?$"
>
> Why do you still need '1?' ?
>
>>>> + description: child nodes describing the internal downstream ports
>>>> + the qps615 switch.
>>>
>>> Please be consistent with starting after the ':' or on the next line.
>>>
>>> And start with capital C.
>>>
>>>
>> ack
>>
>>>> + type: object
>>>> + $ref: "#/$defs/qps615-node"
>>>> + unevaluatedProperties: false
>>>> +
>>>> +$defs:
>>>> + qps615-node:
>>>> + type: object
>>>> +
>>>> + properties:
>>>> + qcom,l0s-entry-delay-ns:
>>>> + description: Aspm l0s entry delay.
>>>> +
>>>> + qcom,l1-entry-delay-ns:
>>>> + description: Aspm l1 entry delay.
>>>
>>> These should probably be common being standard PCIe things. Though, why
>>> are they needed? I'm sure the timing is defined by the PCIe spec, so
>>> they are not compliant?
>>>
>> Usually the firmware in the endpoints/switches should do this these
>> configurations. But the qps615 PCIe switch doesn't have any firmware
>> running to configure these. So the hardware exposes i2c interface to
>> configure these before link training.
>
> If they are following the standard, why do you need to have them in the
> DT? Can you hardcode thos evalues in the driver?
>
>>>> +
>>>> + qcom,tx-amplitude-millivolt:
>>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>>> + description: Change Tx Margin setting for low power consumption.
>>>> +
>>>> + qcom,no-dfe-support:
>>>> + type: boolean
>>>> + description: Disable DFE (Decision Feedback Equalizer), which mitigates
>>>> + intersymbol interference and some reflections caused by impedance mismatches.
>>>> +
>>>> + qcom,nfts:
>>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>>> + description:
>>>> + Number of Fast Training Sequence (FTS) used during L0s to L0 exit
>>>> + for bit and Symbol lock.
>>>
>>> Also something common.
>>>
>>> The problem I have with all these properties is you are using them on
>>> both the upstream and downstream sides of the PCIe links. They belong in
>>> either the device's node (downstream) or the bus's node (upstream).
>>>
>> This switch allows us to configure both upstream, downstream ports and
>> also embedded Ethernet port which is internal to the switch. These
>> properties are applicable for all of those.
>>>> +
>>>> + allOf:
>>>> + - $ref: /schemas/pci/pci-bus.yaml#
>>>
>>> pci-pci-bridge.yaml is more specific and closer to what this device is.
>>>
>> I tried this now, I was getting warning saying the compatible
>> /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb:
>> pcie@0,0: compatible: ['pci1179,0623'] does not contain items matching the
>> given schema
>> from schema $id: http://devicetree.org/schemas/pci/qcom,qps615.yaml#
>> /local/mnt/workspace/skales/kobj/Documentation/devicetree/bindings/pci/qcom,qps615.example.dtb:
>> pcie@0,0: Unevaluated properties are not allowed ('#address-cells',
>> '#size-cells', 'bus-range', 'device_type', 'ranges' were unexpected)
>>
>> I think pci-pci-bridge is expecting the compatible string in this format
>> only "pciclass,0604".
>
> I think the pci-pci-bridge schema requires to have "pciclass,0604" among
> other compatibles. So you should be able to do something like:
>
> compatible = "pci1179,0623", "pciclass,0604";
>
> At least if follows PCI Bus Binding to Open Firmware document.
>
There is a slot driver which is using this pciclass,0604 can we use
still the suggested way.
https://lore.kernel.org/linux-pci/20241210-pci-pwrctrl-slot-v1-4-eae45e488040@linaro.org/
- Krishna Chaitanya.
>>
>>>> +
>>>> +unevaluatedProperties: false
>>>> +
>>>> +required:
>>>> + - vdd18-supply
>>>> + - vdd09-supply
>>>> + - vddc-supply
>>>> + - vddio1-supply
>>>> + - vddio2-supply
>>>> + - vddio18-supply
>>>> + - i2c-parent
>>>> + - reset-gpios
>>>> +
>>>> +examples:
>>>> + - |
>>>> +
>>>> + #include <dt-bindings/gpio/gpio.h>
>>>> +
>>>> + pcie {
>>>> + #address-cells = <3>;
>>>> + #size-cells = <2>;
>>>> +
>>>> + pcie@0 {
>>>> + device_type = "pci";
>>>> + reg = <0x0 0x0 0x0 0x0 0x0>;
>>>> +
>>>> + #address-cells = <3>;
>>>> + #size-cells = <2>;
>>>> + ranges;
>>>> + bus-range = <0x01 0xff>;
>>>> +
>>>> + pcie@0,0 {
>>>> + compatible = "pci1179,0623";
>>>> + reg = <0x10000 0x0 0x0 0x0 0x0>;
>>>> + device_type = "pci";
>>>> + #address-cells = <3>;
>>>> + #size-cells = <2>;
>>>> + ranges;
>>>> + bus-range = <0x02 0xff>;
>>>> +
>>>> + i2c-parent = <&qup_i2c 0x77>;
>>>> +
>>>> + vdd18-supply = <&vdd>;
>>>> + vdd09-supply = <&vdd>;
>>>> + vddc-supply = <&vdd>;
>>>> + vddio1-supply = <&vdd>;
>>>> + vddio2-supply = <&vdd>;
>>>> + vddio18-supply = <&vdd>;
>>>> +
>>>> + reset-gpios = <&gpio 1 GPIO_ACTIVE_LOW>;
>>>> +
>>>> + pcie@1,0 {
>>>> + reg = <0x20800 0x0 0x0 0x0 0x0>;
>>>> + #address-cells = <3>;
>>>> + #size-cells = <2>;
>>>> + device_type = "pci";
>>>> + ranges;
>>>> + bus-range = <0x03 0xff>;
>>>> +
>>>> + qcom,no-dfe-support;
>>>> + };
>>>> +
>>>> + pcie@2,0 {
>>>> + reg = <0x21000 0x0 0x0 0x0 0x0>;
>>>> + #address-cells = <3>;
>>>> + #size-cells = <2>;
>>>> + device_type = "pci";
>>>> + ranges;
>>>> + bus-range = <0x04 0xff>;
>>>> +
>>>> + qcom,nfts = <10>;
>>>> + };
>>>> +
>>>> + pcie@3,0 {
>>>> + reg = <0x21800 0x0 0x0 0x0 0x0>;
>>>> + #address-cells = <3>;
>>>> + #size-cells = <2>;
>>>> + device_type = "pci";
>>>> + ranges;
>>>> + bus-range = <0x05 0xff>;
>>>> +
>>>> + qcom,tx-amplitude-millivolt = <10>;
>>>> + pcie@0,0 {
>>>> + reg = <0x50000 0x0 0x0 0x0 0x0>;
>>>> + #address-cells = <3>;
>>>> + #size-cells = <2>;
>>>> + device_type = "pci";
>>>
>>> There's a 2nd PCI-PCI bridge?
>> This the embedded ethernet port which is as part of DSP3.
>
> So is there an adidtional bus for that ethernet device?
>
>>
>> - Krishna Chaitanya.
>>>
>>>> + ranges;
>>>> +
>>>> + qcom,l1-entry-delay-ns = <10>;
>>>> + };
>>>> +
>>>> + pcie@0,1 {
>>>> + reg = <0x50100 0x0 0x0 0x0 0x0>;
>>>> + #address-cells = <3>;
>>>> + #size-cells = <2>;
>>>> + device_type = "pci";
>>>> + ranges;
>>>> +
>>>> + qcom,l0s-entry-delay-ns = <10>;
>>>> + };
>
> What is this?
>
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>>
>>>> --
>>>> 2.34.1
>>>>
>
Powered by blists - more mailing lists