Message-ID: <708e9d22-0513-4646-aeac-2187c052e208@quicinc.com>
Date: Wed, 16 Oct 2024 14:07:40 +0530
From: Raj Kumar Bhagat <quic_rajkbhag@...cinc.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
CC: <ath12k@...ts.infradead.org>, <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: [PATCH v2 01/22] dt-bindings: net: wireless: describe the ath12k
 AHB module

On 10/16/2024 12:32 PM, Krzysztof Kozlowski wrote:
> On Tue, Oct 15, 2024 at 11:56:16PM +0530, Raj Kumar Bhagat wrote:
>> Add device-tree bindings for the ATH12K module found in the IPQ5332
>> device.
>>
>> Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag@...cinc.com>
>> ---
> 
> That's a v2, what changed?
> 
> Did you ignore entire review? Limited review follows because of that (I
> am not going to do the same work twice).
> 

Review comments in version 1 are addressed.

Per-patch version log is not added here. But we have consolidated version log
in the cover-letter. Will include per-patch version log from next version.

Below are the version log for v2:
- "qcom,board_id" property is dropped. This is not the direct dependency for Ath12k
  AHB support, hence it can be taken up separately.
- "qcom,bdf-addr" property is dropped in device-tree and moved to ath12k driver.
- Currently we have only one compatible enum (qcom,ipq5332-wifi), hence conditional
  if() check for defining the binding is removed.
- "reserved-memory" node is dropped from example DTS.
- "status" property is dropped in wifi node of example DTS.

>>  .../net/wireless/qcom,ath12k-ahb.yaml         | 293 ++++++++++++++++++
>>  1 file changed, 293 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/net/wireless/qcom,ath12k-ahb.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath12k-ahb.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath12k-ahb.yaml
>> new file mode 100644
>> index 000000000000..54784e396d7e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath12k-ahb.yaml
>> @@ -0,0 +1,293 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/wireless/qcom,ath12k-ahb.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Technologies ath12k wireless devices (AHB)
>> +
>> +maintainers:
>> +  - Kalle Valo <kvalo@...nel.org>
>> +  - Jeff Johnson <jjohnson@...nel.org>
>> +
>> +description:
>> +  Qualcomm Technologies IEEE 802.11be AHB devices.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,ipq5332-wifi
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    items:
>> +      - description: XO clock used for copy engine
>> +
>> +  clock-names:
>> +    items:
>> +      - const: gcc_xo_clk
> 
> Drop _clk, drop gcc_. Look how this clock is called *everywhere* else.
> 

Thanks, Based on other bindings example, will rename to "xo"

>> +
>> +  interrupts:
>> +    items:
>> +      - description: Ready interrupt
>> +      - description: Spawn acknowledge interrupt
>> +      - description: Stop acknowledge interrupt
>> +      - description: misc-pulse1 interrupt events
>> +      - description: misc-latch interrupt events
>> +      - description: sw exception interrupt events
>> +      - description: interrupt event for ring CE0
>> +      - description: interrupt event for ring CE1
>> +      - description: interrupt event for ring CE2
>> +      - description: interrupt event for ring CE3
>> +      - description: interrupt event for ring CE4
>> +      - description: interrupt event for ring CE5
>> +      - description: interrupt event for ring CE6
>> +      - description: interrupt event for ring CE7
>> +      - description: interrupt event for ring CE8
>> +      - description: interrupt event for ring CE9
>> +      - description: interrupt event for ring CE10
>> +      - description: interrupt event for ring CE11
>> +      - description: interrupt event for ring host2wbm-desc-feed
>> +      - description: interrupt event for ring host2reo-re-injection
>> +      - description: interrupt event for ring host2reo-command
>> +      - description: interrupt event for ring host2rxdma-monitor-ring1
>> +      - description: interrupt event for ring reo2ost-exception
>> +      - description: interrupt event for ring wbm2host-rx-release
>> +      - description: interrupt event for ring reo2host-status
>> +      - description: interrupt event for ring reo2host-destination-ring4
>> +      - description: interrupt event for ring reo2host-destination-ring3
>> +      - description: interrupt event for ring reo2host-destination-ring2
>> +      - description: interrupt event for ring reo2host-destination-ring1
>> +      - description: interrupt event for ring rxdma2host-monitor-destination-mac3
>> +      - description: interrupt event for ring rxdma2host-monitor-destination-mac2
>> +      - description: interrupt event for ring rxdma2host-monitor-destination-mac1
>> +      - description: interrupt event for ring host2rxdma-host-buf-ring-mac3
>> +      - description: interrupt event for ring host2rxdma-host-buf-ring-mac2
>> +      - description: interrupt event for ring host2rxdma-host-buf-ring-mac1
>> +      - description: interrupt event for ring host2tcl-input-ring4
>> +      - description: interrupt event for ring host2tcl-input-ring3
>> +      - description: interrupt event for ring host2tcl-input-ring2
>> +      - description: interrupt event for ring host2tcl-input-ring1
>> +      - description: interrupt event for ring wbm2host-tx-completions-ring4
>> +      - description: interrupt event for ring wbm2host-tx-completions-ring3
>> +      - description: interrupt event for ring wbm2host-tx-completions-ring2
>> +      - description: interrupt event for ring wbm2host-tx-completions-ring1
>> +      - description: interrupt event for ring host2tx-monitor-ring1
>> +      - description: interrupt event for ring txmon2host-monitor-destination-mac3
>> +      - description: interrupt event for ring txmon2host-monitor-destination-mac2
>> +      - description: interrupt event for ring txmon2host-monitor-destination-mac1
>> +      - description: interrupt event for umac_reset
>> +
>> +  interrupt-names:
>> +    items:
>> +      - const: ready
>> +      - const: spawn
>> +      - const: stop-ack
>> +      - const: misc-pulse1
>> +      - const: misc-latch
>> +      - const: sw-exception
>> +      - const: ce0
>> +      - const: ce1
>> +      - const: ce2
>> +      - const: ce3
>> +      - const: ce4
>> +      - const: ce5
>> +      - const: ce6
>> +      - const: ce7
>> +      - const: ce8
>> +      - const: ce9
>> +      - const: ce10
>> +      - const: ce11
>> +      - const: host2wbm-desc-feed
>> +      - const: host2reo-re-injection
>> +      - const: host2reo-command
>> +      - const: host2rxdma-monitor-ring1
>> +      - const: reo2ost-exception
>> +      - const: wbm2host-rx-release
>> +      - const: reo2host-status
>> +      - const: reo2host-destination-ring4
>> +      - const: reo2host-destination-ring3
>> +      - const: reo2host-destination-ring2
>> +      - const: reo2host-destination-ring1
>> +      - const: rxdma2host-monitor-destination-mac3
>> +      - const: rxdma2host-monitor-destination-mac2
>> +      - const: rxdma2host-monitor-destination-mac1
>> +      - const: host2rxdma-host-buf-ring-mac3
>> +      - const: host2rxdma-host-buf-ring-mac2
>> +      - const: host2rxdma-host-buf-ring-mac1
>> +      - const: host2tcl-input-ring4
>> +      - const: host2tcl-input-ring3
>> +      - const: host2tcl-input-ring2
>> +      - const: host2tcl-input-ring1
>> +      - const: wbm2host-tx-completions-ring4
>> +      - const: wbm2host-tx-completions-ring3
>> +      - const: wbm2host-tx-completions-ring2
>> +      - const: wbm2host-tx-completions-ring1
>> +      - const: host2tx-monitor-ring1
>> +      - const: txmon2host-monitor-destination-mac3
>> +      - const: txmon2host-monitor-destination-mac2
>> +      - const: txmon2host-monitor-destination-mac1
>> +      - const: umac_reset
>> +
>> +  memory-region:
>> +    minItems: 1
> 
> upper constraint
> 
>> +    description:
>> +      phandle to a node describing reserved memory (System RAM memory)
>> +      used by ath12k firmware (see bindings/reserved-memory/reserved-memory.txt)
>> +
>> +  qcom,rproc:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      DT entry of a WCSS node. WCSS node is the child node of q6 remoteproc driver.
>> +      (see bindings/remoteproc/qcom,multipd-pil.yaml)
> 
> DT nodes are not children of drivers. But other DT nodes. Explain why
> this phandle is needed, what is it for.
> 
> To me it looks like you incorrectly organized your nodes.
> 

This phandle is required by wifi driver (ath12k) to retrieve the correct remote processor
(rproc_get_by_phandle()). Ath12k driver needs this rproc to interact with the remote
processor (example: booting-up remote processor).

In next version, will correct the description based on existing bindings (qcom,ath11k.yaml).

>> +
>> +  qcom,smem-states:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description: States used by the AP to signal the remote processor
>> +    items:
>> +      - description: Shutdown WCSS pd
>> +      - description: Stop WCSS pd
>> +      - description: Spawn WCSS pd
>> +
>> +  qcom,smem-state-names:
>> +    description:
>> +      Names of the states used by the AP to signal the remote processor
>> +    items:
>> +      - const: shutdown
>> +      - const: stop
>> +      - const: spawn
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +  - interrupts
>> +  - interrupt-names
>> +  - memory-region
>> +  - qcom,rproc
>> +  - qcom,smem-states
>> +  - qcom,smem-state-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +
> 
> Stray blank line
> 

Will update in next version.

> Best regards,
> Krzysztof
> 

Powered by blists - more mailing lists