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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5f65905c-f1e4-4f52-ba7c-10c1a4892e30@kernel.org>
Date: Sun, 4 Aug 2024 10:53:25 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Krishna chaitanya chundru <quic_krichai@...cinc.com>,
 Lorenzo Pieralisi <lpieralisi@...nel.org>,
 Krzysztof WilczyƄski <kw@...ux.com>,
 Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Konrad Dybcio <konrad.dybcio@...aro.org>,
 cros-qcom-dts-watchers@...omium.org, Bartosz Golaszewski <brgl@...ev.pl>,
 Jingoo Han <jingoohan1@...il.com>,
 Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: andersson@...nel.org, quic_vbadigan@...cinc.com,
 linux-arm-msm@...r.kernel.org, linux-pci@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v2 1/8] dt-bindings: PCI: Add binding for qps615

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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ