diff mbox series

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

Message ID 20230113150310.29709-6-quic_devipriy@quicinc.com
State New
Headers show
Series Add regulator support for IPQ9574 SoC | expand

Commit Message

Devi Priya Jan. 13, 2023, 3:03 p.m. UTC 
Add CPU Freq and RPM related nodes in the device tree

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(+)

Comments

Konrad Dybcio Jan. 13, 2023, 3:32 p.m. UTC | #1 
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.

>  
>  / {
>  	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..


>  		};
>  
>  		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

> +		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.

> +		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.

> +
> +				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..

Konrad
> +				};
> +			};
> +		};
> +	};
> +
>  	timer {
>  		compatible = "arm,armv8-timer";
>  		interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
Devi Priya Feb. 2, 2023, 9:54 a.m. UTC | #2 
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
diff mbox series

Patch

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>
 
 / {
 	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>;
 		};
 
 		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 {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		opp-936000000 {
+			opp-hz = /bits/ 64 <936000000>;
+			opp-microvolt = <725000>;
+			clock-latency-ns = <200000>;
+		};
+		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";
+
+				ipq9574_s1: s1 {
+					regulator-min-microvolt = <587500>;
+					regulator-max-microvolt = <1075000>;
+					regulator-always-on;
+				};
+			};
+		};
+	};
+
 	timer {
 		compatible = "arm,armv8-timer";
 		interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,