linux-kernel - Re: [PATCH 1/6] dt: bindings: net: add bindings for QCN6122

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] [day] [month] [year] [list]
Message-ID: <da7ecb40-dbc2-4f6e-8026-d630e5b3bb52@kernel.org>
Date: Thu, 30 Oct 2025 06:47:44 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: George Moussalem <george.moussalem@...look.com>,
 Johannes Berg <johannes@...solutions.net>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Jeff Johnson <jjohnson@...nel.org>
Cc: linux-wireless@...r.kernel.org, devicetree@...r.kernel.org,
 ath11k@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] dt: bindings: net: add bindings for QCN6122

On 29/10/2025 17:12, George Moussalem wrote:
> 
> 
> On 10/29/25 18:32, Krzysztof Kozlowski wrote:
>> On 29/10/2025 15:26, George Moussalem via B4 Relay wrote:
>>> From: George Moussalem <george.moussalem@...look.com>
>>>
>>> QCN6122 is a PCIe based solution that is attached to and enumerated
>>> by the WPSS (Wireless Processor SubSystem) Q6 processor.
>>
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching. For bindings, the preferred subjects are
>> explained here:
>> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
>>
>> The prefix is never "dt:".
>>
> Will do in next version, thanks.
> 
>>
>> A nit, subject: drop second/last, redundant "bindings for". The
>> "dt-bindings" prefix is already stating that these are bindings.
>> See also:
>> https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>>
> 
> Will change accordingly.
> 
>>>
>>> Though it is a PCIe device, since it is not attached to APSS processor
>>> (Application Processor SubSystem), APSS will be unaware of such a decice
>>> so it is registered to the APSS processor as a platform device(AHB).
>>> Because of this hybrid nature, it is called as a hybrid bus device as
>>> introduced by WCN6750. It has 5 CE and 8 DP rings.
>>>
>>> QCN6122 is similar to WCN6750 and follows the same codepath as for
>>> WCN6750.
>>>
>>> Signed-off-by: George Moussalem <george.moussalem@...look.com>
>>> ---
>>>  .../bindings/net/wireless/qcom,ath11k.yaml         | 57 +++++++++++++++++++++-
>>>  1 file changed, 56 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml
>>> index c089677702cf17f3016b054d21494d2a7706ce5d..4b0b282bb9231c8bc496fed42e0917b9d7d106d2 100644
>>> --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml
>>> +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml
>>> @@ -21,12 +21,13 @@ properties:
>>>        - qcom,ipq6018-wifi
>>>        - qcom,wcn6750-wifi
>>>        - qcom,ipq5018-wifi
>>> +      - qcom,qcn6122-wifi
>>
>> Why people keep adding to the end... previously ipq5018 added by
>> qualcom, did not even get any review.
>>
>> Place it before wcn and let ipq5018 be outlier since this was broken
>> already.
>>
> 
> it was exactly ipq5018 which got me thinking I should add it to the end.
> Will alphabetically insert qcn6122. I could reorder the entire list as
> well if you'd like (ipq6018 is also misplaced)
> 
>>>  
>>>    reg:
>>>      maxItems: 1
>>>  
>>>    interrupts:
>>> -    minItems: 32
>>> +    minItems: 13
>>>      maxItems: 52
>>>  
>>>    interrupt-names:
>>> @@ -87,6 +88,14 @@ properties:
>>>      items:
>>>        - const: wlan-smp2p-out
>>>  
>>> +  qcom,userpd:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [2, 3]
>>> +    description: instance ID of user PD (protection domain) in multi-PD
>>> +                 architectures to distinguish between multiple instances
>>> +                 of the same wifi chip used by QMI in its interface with
>>> +                 the firmware running on Q6.
>>
>> Broken indentation. It is supposed to be two spaces. Look at this file -
>> why are you doing this completely different?
>>
>> Anyway, please do not come with 2nd or 3rd property for this. We already
>> have such somewhere.
> 
> Would you mind pointing me to the property you're referring to? Do you
> mean the QRTR ID as proposed in this RFC:
> https://lore.kernel.org/linux-wireless/cover.1732506261.git.ionic@ionic.de/
> 
> if so, this wouldn't help this in this case. Although it's PCIe based,
> PCI is not even enabled as the Q6 firmware itself takes care of that.


I expect something covering all cases, so it must cover ipq5332 and
ipq9574 from the other patchset you mentioned:
https://lore.kernel.org/all/20231110091939.3025413-2-quic_mmanikan@quicinc.com/

Same thing cannot be represented in two different ways.

For APR and GPR we already have qcom,protection-domain as a service
name. Linux drivers use it a bit differently than your code here, but
unfortunately I focus on bindings and the binding sounds exactly the same.

Anyway instance ID is not acceptable. Check my slides for possible
workarounds, if you do not know the actual protection domain.

Best regards,
Krzysztof