| 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: <cb69c30f-8230-4a8f-a538-3ec964b79084@quicinc.com>
Date: Tue, 5 Nov 2024 00:14:28 +0530
From: Raj Kumar Bhagat <quic_rajkbhag@...cinc.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, <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 10/29/2024 11:22 PM, Krzysztof Kozlowski wrote:
> 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.
>
sure will update the commit log.
>>
>> 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.
>
will remove this description in next version.
>>
>> 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.
>
sure, will create a separate binding, may be "qcom,ath12k_wsi.yaml".
This will be for ath21k PCI device with WSI interface.
>>
>> reg:
>> maxItems: 1
>>
>> + qcom,ath12k-calibration-variant:
>> + $ref: /schemas/types.yaml#/definitions/string
>> + description: |
>
> Do not need '|' unless you need to preserve formatting.
>
thanks will remove "|"
>> + 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.
>
In next version will remove "wsi". The properties under wsi (ports,
qcom,wsi-master, etc) will be directly under ath12k device 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.
>
So this mean "qcom,wsi-group-id" to be dropped and we can assign the
group ID (in ath12k driver implementation) by using the graph?
>> +
>> + qcom,wsi-master:
>> + type: boolean
>> + description:
>> + Indicates if this device is the WSI master.
>> +
>
> This copies property name. Why being master is important?
>
The master device in the WSI group aids (is capable) to synchronize the Timing
Synchronization Function (TSF) clock across all devices in the group. Will include
this information in next version.
> Also, use some different name: see preferred names in kernel coding style.
>
Thanks for pointing out, will use "qcom,wsi-controller"
>> + 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.
>
Sure, we will have separate binding in next version.
>> +
>> examples:
>> - |
>> #include <dt-bindings/clock/qcom,rpmh.h>
>> @@ -97,3 +184,139 @@ examples:
>> };
>> };
>> };
>> +
>> + - |
>> + pcie1 {
>
> pcie {
> and keep all nodes here
>
sure
>> + #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.
>
Thanks, will update.
>> + 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.
>
sure will remove wsi node and directly define ports and other properties
inside wifi.
Powered by blists - more mailing lists