Re: [PATCH 5/6] arm64: dts: qcom: ipq9574: Add cpufreq & RPM related nodes

[Devi Priya <quic_devipriy@quicinc.com>]

Thanks konrad for taking time to review the patch!

On 1/13/2023 9:02 PM, Konrad Dybcio wrote:
> 
> 
> On 13.01.2023 16:03, devi priya wrote:
>> Add CPU Freq and RPM related nodes in the device tree
> These two are wildly different things, barely related to one
> another and can very well be introduced in separate patches.
> Please do so.
> 
>>
>> Co-developed-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> Signed-off-by: devi priya <quic_devipriy@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq9574.dtsi | 80 +++++++++++++++++++++++++++
>>   1 file changed, 80 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> index 5a2244b437ed..79fa5d91882c 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> @@ -9,6 +9,7 @@
>>   #include <dt-bindings/interrupt-controller/arm-gic.h>
>>   #include <dt-bindings/clock/qcom,gcc-ipq9574.h>
>>   #include <dt-bindings/reset/qcom,gcc-ipq9574.h>
>> +#include <dt-bindings/clock/qcom,apss-ipq.h>
> Please sort the includes alphabetically.
Sure, okay
> 
>>   
>>   / {
>>   	interrupt-parent = <&intc>;
>> @@ -75,6 +76,10 @@
>>   			reg = <0x0>;
>>   			enable-method = "psci";
>>   			next-level-cache = <&L2_0>;
>> +			clocks = <&apcs_glb APCS_ALIAS0_CORE_CLK>;
>> +			clock-names = "cpu";
>> +			operating-points-v2 = <&cpu_opp_table>;
>> +			cpu0-supply = <&ipq9574_s1>;
> Why is this cpu0-supply and the rest are cpu-supply? Neither of them
> seem particularly documented, by the way..
Sure, will check and update this in V2
> 
> 
>>   		};
>>   
>>   		CPU1: cpu@1 {
>> @@ -83,6 +88,10 @@
>>   			reg = <0x1>;
>>   			enable-method = "psci";
>>   			next-level-cache = <&L2_0>;
>> +			clocks = <&apcs_glb APCS_ALIAS0_CORE_CLK>;
>> +			clock-names = "cpu";
>> +			operating-points-v2 = <&cpu_opp_table>;
>> +			cpu-supply = <&ipq9574_s1>;
>>   		};
>>   
>>   		CPU2: cpu@2 {
>> @@ -91,6 +100,10 @@
>>   			reg = <0x2>;
>>   			enable-method = "psci";
>>   			next-level-cache = <&L2_0>;
>> +			clocks = <&apcs_glb APCS_ALIAS0_CORE_CLK>;
>> +			clock-names = "cpu";
>> +			operating-points-v2 = <&cpu_opp_table>;
>> +			cpu-supply = <&ipq9574_s1>;
>>   		};
>>   
>>   		CPU3: cpu@3 {
>> @@ -99,6 +112,10 @@
>>   			reg = <0x3>;
>>   			enable-method = "psci";
>>   			next-level-cache = <&L2_0>;
>> +			clocks = <&apcs_glb APCS_ALIAS0_CORE_CLK>;
>> +			clock-names = "cpu";
>> +			operating-points-v2 = <&cpu_opp_table>;
>> +			cpu-supply = <&ipq9574_s1>;
>>   		};
>>   
>>   		L2_0: l2-cache {
>> @@ -107,6 +124,42 @@
>>   		};
>>   	};
>>   
>> +	cpu_opp_table: opp-table-cpu {
> Alphabetically this goes after memory
Okay
> 
>> +		compatible = "operating-points-v2";
>> +		opp-shared;
>> +
>> +		opp-936000000 {
>> +			opp-hz = /bits/ 64 <936000000>;
>> +			opp-microvolt = <725000>;
>> +			clock-latency-ns = <200000>;
>> +		};
> Please add a newline between each subnode.
Okay
> 
>> +		opp-1104000000 {
>> +			opp-hz = /bits/ 64 <1104000000>;
>> +			opp-microvolt = <787500>;
>> +			clock-latency-ns = <200000>;
>> +		};
>> +		opp-1416000000 {
>> +			opp-hz = /bits/ 64 <1416000000>;
>> +			opp-microvolt = <862500>;
>> +			clock-latency-ns = <200000>;
>> +		};
>> +		opp-1488000000 {
>> +			opp-hz = /bits/ 64 <1488000000>;
>> +			opp-microvolt = <925000>;
>> +			clock-latency-ns = <200000>;
>> +		};
>> +		opp-1800000000 {
>> +			opp-hz = /bits/ 64 <1800000000>;
>> +			opp-microvolt = <987500>;
>> +			clock-latency-ns = <200000>;
>> +		};
>> +		opp-2208000000 {
>> +			opp-hz = /bits/ 64 <2208000000>;
>> +			opp-microvolt = <1062500>;
>> +			clock-latency-ns = <200000>;
>> +		};
>> +	};
>> +
>>   	memory@40000000 {
>>   		device_type = "memory";
>>   		/* We expect the bootloader to fill in the size */
>> @@ -128,6 +181,11 @@
>>   		#size-cells = <2>;
>>   		ranges;
>>   
>> +		rpm_msg_ram: memory@60000 {
>> +			reg = <0x0 0x00060000 0x0 0x6000>;
>> +			no-map;
>> +		};
>> +
>>   		tz_region: memory@4a600000 {
>>   			reg = <0x0 0x4a600000 0x0 0x400000>;
>>   			no-map;
>> @@ -324,6 +382,28 @@
>>   		};
>>   	};
>>   
>> +	rpm-glink {
>> +		compatible = "qcom,glink-rpm";
>> +		interrupts = <GIC_SPI 168 IRQ_TYPE_EDGE_RISING>;
>> +		qcom,rpm-msg-ram = <&rpm_msg_ram>;
>> +		mboxes = <&apcs_glb 0>;
>> +
>> +		rpm_requests: glink-channel {
>> +			compatible = "qcom,rpm-ipq9574";
>> +			qcom,glink-channels = "rpm_requests";
>> +
>> +			regulators {
>> +				compatible = "qcom,rpm-ipq9574-mp5496-regulators";
> The regulators are board-specific and should not be included in the
> SoC DTSI. If this is a very common configuration, you may split that
> into ipq9574-mp5496.dtsi, for example. Or ipq9574-pmics.dtsi if it's
> coupled with more PMICs.
Got it. Will move this to the board DT
> 
>> +
>> +				ipq9574_s1: s1 {
>> +					regulator-min-microvolt = <587500>;
>> +					regulator-max-microvolt = <1075000>;
>> +					regulator-always-on;
> Won't this break CPU retention?
> 
> You're holding a vote on it from the CPU devices, so it should be
> always enabled when the CPUs are oneline (as far as Linux is
> concerned).
> 
> 
> Or maybe Linux will think it's enabled and RPM will quietly park
> it when it decides it's good to do so.. but will it with an active
> request.. not sure, really.. just something to consider..
> 
Sure, will check this and update accordingly in V2
> Konrad
>> +				};
>> +			};
>> +		};
>> +	};
>> +
>>   	timer {
>>   		compatible = "arm,armv8-timer";
>>   		interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
Best Regards,
Devi Priya