Re: [PATCH v2] arm64: dts: qcom: add wifi node for IPQ5332 based RDP441

[Raj Kumar Bhagat <quic_rajkbhag@quicinc.com>]

On 2/26/2025 9:22 AM, Bjorn Andersson wrote:
> On Thu, Jan 30, 2025 at 10:29:00AM +0530, Raj Kumar Bhagat wrote:
>> RDP441 is based on IPQ5332. It has inbuilt AHB bus based IPQ5332 WiFi
>> device.
>>
>> Describe and add WiFi node for RDP441. Also, reserve the memory
>> required by IPQ5332 firmware.
>>
>> Depends-On: [PATCH V2 0/2] mailbox: tmel-qmp: Introduce QCOM TMEL QMP mailbox driver
>> Depends-On: [PATCH V3 0/8] Add new driver for WCSS secure PIL loading
>> Link: https://lore.kernel.org/lkml/20241231054900.2144961-1-quic_srichara@quicinc.com/
>> Link: https://lore.kernel.org/lkml/20250107101647.2087358-1-quic_gokulsri@quicinc.com/
> 
> Please run "git log --grep 'Depends-on'" to see how this tag is expected
> to be used. As you can see, dependencies on other patches in flight does
> not go into the git history, and should as such be mentioned only below
> the --- line below.
> 

Thanks will take care for next submission.

> Please wait for these dependencies to be accepted before resubmitting
> this Devicetree change.
> 

Sure

>>
>> Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag@quicinc.com>
>> ---
>> NOTE:
>> The DT binding changes for this patch has been posted along with the
>> driver ath12k AHB series.
>> Link: https://lore.kernel.org/all/20250130043508.1885026-1-quic_rajkbhag@quicinc.com/
>>
>> v2:
>> - Dropped 'dt-bindings: net: wireless: describe the ath12k AHB module'.
>>   This DT binding is posted along with the ath12k AHB driver changes.
>> - Reserved four memory regions required by IPQ5332 ath12k firmware in DTS.
>>   These memory regions are also referenced in the WiFi node.
>> - Moved 'qcom,smem-states' and 'qcom,smem-states' to the ipq5332.dtsi file.
>> - Used lowercase hex values for the reg property.
>>
>> v1: https://lore.kernel.org/all/20250128091012.2574478-1-quic_rajkbhag@quicinc.com/
>> ---
>>  arch/arm64/boot/dts/qcom/ipq5332-rdp441.dts |  64 ++++++++++-
>>  arch/arm64/boot/dts/qcom/ipq5332.dtsi       | 113 ++++++++++++++++++++
>>  2 files changed, 176 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5332-rdp441.dts b/arch/arm64/boot/dts/qcom/ipq5332-rdp441.dts
>> index 846413817e9a..4b7a75e450ca 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq5332-rdp441.dts
>> +++ b/arch/arm64/boot/dts/qcom/ipq5332-rdp441.dts
>> @@ -2,7 +2,7 @@
>>  /*
>>   * IPQ5332 AP-MI01.2 board device tree source
>>   *
>> - * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
>> + * Copyright (c) 2022-2025 Qualcomm Innovation Center, Inc. All rights reserved.
>>   */
>>  
>>  /dts-v1/;
>> @@ -12,6 +12,61 @@
>>  / {
>>  	model = "Qualcomm Technologies, Inc. IPQ5332 MI01.2";
>>  	compatible = "qcom,ipq5332-ap-mi01.2", "qcom,ipq5332";
>> +
>> +	/*                 Default Profile
> 
> Why is the title "Default Profile", is this expected to be changed by
> someone? At what point?
> 
>> +	 * +============+==============+=====================+
>> +	 * |            |              |                     |
>> +	 * | Region     | Start Offset |       Size          |
> 
> "Start Offset" relative to address 0 is typically called "base address".
> 
>> +	 * |            |              |                     |
>> +	 * +------------+--------------+---------------------+
>> +	 * |            |              |                     |
>> +	 * |            |              |                     |
>> +	 * |            |              |                     |
>> +	 * | WLAN Q6    |  0x4A900000  |       43MB          |
>> +	 * |            |              |                     |
>> +	 * |            |              |                     |
>> +	 * +------------+--------------+---------------------+
>> +	 * | M3 Dump    |  0x4D400000  |       1MB           |
>> +	 * +------------+--------------+---------------------+
>> +	 * | Q6 caldb   |  0x4D500000  |       5MB           |
>> +	 * +------------+--------------+---------------------+
>> +	 * | MLO        |  0x4DB00000  |       18MB          |
> 
> 18MB is the same size in your graph as the 1MB segment, but somehow the
> 43MB segment is 5 times the size of the 18MB segment. So the graph isn't
> to scale...
> 
>> +	 * +============+==============+=====================+
>> +	 * |                                                 |
>> +	 * |                                                 |
>> +	 * |                                                 |
>> +	 * |            Rest of memory for Linux             |
>> +	 * |                                                 |
>> +	 * |                                                 |
>> +	 * |                                                 |
>> +	 * +=================================================+
> 
> Doesn't DDR start on a more even address, such as 0x40000000? I presume
> the purpose of your picture was to give a good overview of the memory
> layout of this system, but the only thing it added to the table below is
> a curious question about what happened to the first 169MB of DDR.
> 

Yes this graph provide the overview for memory related to wireless
components(rproc and ath12k firmware memory).

> 
> Please omit the graph, it doesn't add value beyond what's provided by
> the below text form of the "same" data.
> 

Sure, will remove in next version.

>> +	 */
>> +
>> +	reserved-memory {
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +		ranges;
>> +
>> +		q6_region: wcss@4a900000 {
>> +			reg = <0x0 0x4a900000 0x0 0x02b00000>;
>> +			no-map;
>> +		};
>> +
>> +		m3_dump: m3-dump@4d400000 {
>> +			reg = <0x0 0x4d400000 0x0 0x100000>;
>> +			no-map;
>> +		};
>> +
>> +		q6_caldb: q6-caldb@4d500000 {
>> +			reg = <0x0 0x4d500000 0x0 0x500000>;
>> +			no-map;
>> +		};
>> +
>> +		mlo_mem: mlo-global-mem@4db00000 {
>> +			reg = <0x0 0x4db00000 0x0 0x01200000>;
>> +			no-map;
>> +		};
>> +	};
>>  };
>>  
>>  &blsp1_i2c1 {
>> @@ -63,3 +118,10 @@ data-pins {
>>  		};
>>  	};
>>  };
>> +
>> +&wifi0 {
>> +	memory-region = <&q6_region>, <&m3_dump>, <&q6_caldb>, <&mlo_mem>;
>> +	memory-region-names = "q6-region", "m3-dump", "q6-caldb", "mlo-global-mem";
> 
> As you are resubmitting this, it would be nice to have these two
> properties wrapped (one entry per line), to increase readability...
> 

will update in next version.