| 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: <764f8f22-146d-4edc-9d46-7fe3c7d9a2f2@kernel.org>
Date: Tue, 29 Oct 2024 18:52:34 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Raj Kumar Bhagat <quic_rajkbhag@...cinc.com>, ath12k@...ts.infradead.org
Cc: linux-wireless@...r.kernel.org, Kalle Valo <kvalo@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Jeff Johnson <jjohnson@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [RFC PATCH v2 1/5] dt-bindings: net: wireless: ath12k: describe
WSI properties for QCN9274
On 29/10/2024 18:30, Raj Kumar Bhagat wrote:
> QCN9274 device has WSI support. WSI stands for WLAN Serial Interface.
> It is used for the exchange of specific control information across
> radios based on the doorbell mechanism. This WSI connection is
> essential to exchange control information among these devices
>
> Hence, describe WSI interface supported in QCN9274 with the following
> properties:
>
> - qcom,wsi-group-id: It represents the identifier assigned to the WSI
> connection. All the ath12k devices connected to same WSI connection
> have the same wsi-group-id.
>
> - qcom,wsi-master: Indicates if this device is the WSI master.
>
> - ports: This is a graph ports schema that has two ports: TX (port@0)
> and RX (port@1). This represents the actual WSI connection among
> multiple devices.
Describe the hardware, not the contents of the patch/binding. We see it
easily, but what we do not see is the hardware.
>
> Also, describe the ath12k device property
> "qcom,ath12k-calibration-variant". This is a common property among
> ath12k devices.
Why do you describe it? What you do is easily visible. We do not see why.
>
> Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag@...cinc.com>
> ---
> .../bindings/net/wireless/qcom,ath12k.yaml | 241 +++++++++++++++++-
> 1 file changed, 232 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath12k.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath12k.yaml
> index 1b5884015b15..42bcd73dd159 100644
> --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath12k.yaml
> +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath12k.yaml
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> # Copyright (c) 2024 Linaro Limited
> +# Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> %YAML 1.2
> ---
> $id: http://devicetree.org/schemas/net/wireless/qcom,ath12k.yaml#
> @@ -18,10 +19,17 @@ properties:
> compatible:
> enum:
> - pci17cb,1107 # WCN7850
> + - pci17cb,1109 # QCN9274
I asked for separate binding because it is quite a different device.
Unless it is not... but then commit msg is quite not precise here.
>
> reg:
> maxItems: 1
>
> + qcom,ath12k-calibration-variant:
> + $ref: /schemas/types.yaml#/definitions/string
> + description: |
Do not need '|' unless you need to preserve formatting.
> + string to uniquely identify variant of the calibration data for designs
> + with colliding bus and device ids
> +
> vddaon-supply:
> description: VDD_AON supply regulator handle
>
> @@ -49,21 +57,100 @@ properties:
> vddpcie1p8-supply:
> description: VDD_PCIE_1P8 supply regulator handle
>
> + wsi:
Not much improved here. I asked to drop the node.
> + type: object
> + description: |
> + The ath12k devices (QCN9274) feature WSI support. WSI stands for
> + WLAN Serial Interface. It is used for the exchange of specific
> + control information across radios based on the doorbell mechanism.
> + This WSI connection is essential to exchange control information
> + among these devices.
> +
> + Diagram to represent one WSI connection (one WSI group) among
> + three devices.
> +
> + +-------+ +-------+ +-------+
> + | pcie2 | | pcie3 | | pcie1 |
> + | | | | | |
> + +----->| wsi |------->| wsi |------->| wsi |-----+
> + | | grp 0 | | grp 0 | | grp 2 | |
> + | +-------+ +-------+ +-------+ |
> + +------------------------------------------------------+
> +
> + Diagram to represent two WSI connections (two separate WSI groups)
> + among four devices.
> +
> + +-------+ +-------+ +-------+ +-------+
> + | pcie2 | | pcie3 | | pcie1 | | pcie0 |
> + | | | | | | | |
> + +-->| wsi |--->| wsi |--+ +-->| wsi |--->| wsi |--+
> + | | grp 0 | | grp 0 | | | | grp 1 | | grp 1 | |
> + | +-------+ +-------+ | | +-------+ +-------+ |
> + +---------------------------+ +---------------------------+
> +
> + properties:
> + qcom,wsi-group-id:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + It represents the identifier assigned to the WSI connection. All
> + the ath12k devices connected to same WSI connection have the
> + same wsi-group-id.
That's not needed according to description. Entire group is defined by
graph.
> +
> + qcom,wsi-master:
> + type: boolean
> + description:
> + Indicates if this device is the WSI master.
> +
This copies property name. Why being master is important?
Also, use some different name: see preferred names in kernel coding style.
> + ports:
> + $ref: /schemas/graph.yaml#/properties/ports
> + description:
> + These ports are used to connect multiple WSI supported devices to
> + form the WSI group.
> +
> + properties:
> + port@0:
> + $ref: /schemas/graph.yaml#/properties/port
> + description:
> + This is the TX port of WSI interface. It is attached to the RX
> + port of the next device in the WSI connection.
> +
> + port@1:
> + $ref: /schemas/graph.yaml#/properties/port
> + description:
> + This is the RX port of WSI interface. It is attached to the TX
> + port of the previous device in the WSI connection.
> +
> + required:
> + - qcom,wsi-group-id
> + - ports
> +
> + additionalProperties: false
> +
> required:
> - compatible
> - reg
> - - vddaon-supply
> - - vddwlcx-supply
> - - vddwlmx-supply
> - - vddrfacmn-supply
> - - vddrfa0p8-supply
> - - vddrfa1p2-supply
> - - vddrfa1p8-supply
> - - vddpcie0p9-supply
> - - vddpcie1p8-supply
>
> additionalProperties: false
>
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - pci17cb,1107
> + then:
> + required:
> + - vddaon-supply
> + - vddwlcx-supply
> + - vddwlmx-supply
> + - vddrfacmn-supply
> + - vddrfa0p8-supply
> + - vddrfa1p2-supply
> + - vddrfa1p8-supply
> + - vddpcie0p9-supply
> + - vddpcie1p8-supply
Commit says WSI applies only to new variant, so properties should be
disallowed... or just follow my feedback last time: separate binding.
> +
> examples:
> - |
> #include <dt-bindings/clock/qcom,rpmh.h>
> @@ -97,3 +184,139 @@ examples:
> };
> };
> };
> +
> + - |
> + pcie1 {
pcie {
and keep all nodes here
> + #address-cells = <3>;
> + #size-cells = <2>;
> +
> + pcie@0 {
> + device_type = "pci";
> + reg = <0x0 0x0 0x0 0x0 0x0>;
> + #address-cells = <3>;
> + #size-cells = <2>;
> + ranges;
> +
> + wifi1@0 {
wifi@
Same in other places.
> + compatible = "pci17cb,1109";
> + reg = <0x0 0x0 0x0 0x0 0x0>;
> +
> + qcom,ath12k-calibration-variant = "RDP433_1";
> +
> + wsi {
No resources here? Not a bus? You already got comment about it.
Best regards,
Krzysztof
Powered by blists - more mailing lists