diff mbox series

[5/7] arm64: dts: qcom: pm8150x: add thermal alarms and thermal zones

Message ID 20200604004331.669936-5-dmitry.baryshkov@linaro.org
State New
Headers show
Series [1/7] arm64: dts: qcom: pm8009: Add base dts file | expand

Commit Message

Dmitry Baryshkov June 4, 2020, 12:43 a.m. UTC
Add temperature alarm and thermal zone configuration to all three
pm8150 instances. Configuration is largely based on the msm-4.19 tree.
These alarms use main adc of the pmic. Separate temperature adc is not
supported yet.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

---
 arch/arm64/boot/dts/qcom/pm8150.dtsi  | 41 +++++++++++++++++++++++--
 arch/arm64/boot/dts/qcom/pm8150b.dtsi | 43 +++++++++++++++++++++++++--
 arch/arm64/boot/dts/qcom/pm8150l.dtsi | 43 +++++++++++++++++++++++++--
 3 files changed, 119 insertions(+), 8 deletions(-)

-- 
2.26.2

Comments

Vinod Koul June 4, 2020, 10:47 a.m. UTC | #1
On 04-06-20, 03:43, Dmitry Baryshkov wrote:
> Add temperature alarm and thermal zone configuration to all three

> pm8150 instances. Configuration is largely based on the msm-4.19 tree.

> These alarms use main adc of the pmic. Separate temperature adc is not

> supported yet.

> 

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---

>  arch/arm64/boot/dts/qcom/pm8150.dtsi  | 41 +++++++++++++++++++++++--

>  arch/arm64/boot/dts/qcom/pm8150b.dtsi | 43 +++++++++++++++++++++++++--

>  arch/arm64/boot/dts/qcom/pm8150l.dtsi | 43 +++++++++++++++++++++++++--

>  3 files changed, 119 insertions(+), 8 deletions(-)

> 

> diff --git a/arch/arm64/boot/dts/qcom/pm8150.dtsi b/arch/arm64/boot/dts/qcom/pm8150.dtsi

> index c0b197458665..fee2db42f4cb 100644

> --- a/arch/arm64/boot/dts/qcom/pm8150.dtsi

> +++ b/arch/arm64/boot/dts/qcom/pm8150.dtsi

> @@ -30,6 +30,15 @@ pwrkey {

>  			};

>  		};

>  

> +		pm8150_temp: temp-alarm@2400 {

> +			compatible = "qcom,spmi-temp-alarm";

> +			reg = <0x2400>;

> +			interrupts = <0x0 0x24 0x0 IRQ_TYPE_EDGE_BOTH>;

> +			io-channels = <&pm8150_adc ADC5_DIE_TEMP>;

> +			io-channel-names = "thermal";

> +			#thermal-sensor-cells = <0>;

> +		};

> +

>  		pm8150_adc: adc@3100 {

>  			compatible = "qcom,spmi-adc5";

>  			reg = <0x3100>;

> @@ -38,8 +47,6 @@ pm8150_adc: adc@3100 {

>  			#io-channel-cells = <1>;

>  			interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;

>  

> -			status = "disabled";

> -


This should not be removed, rather than this please add enabled in you
board dts file

>  			ref-gnd@0 {

>  				reg = <ADC5_REF_GND>;

>  				qcom,pre-scaling = <1 1>;

> @@ -85,3 +92,33 @@ pmic@1 {

>  		#size-cells = <0>;

>  	};

>  };

> +

> +&thermal_zones {

> +	pm8150_temp {

> +		polling-delay-passive = <0>;

> +		polling-delay = <0>;

> +

> +		thermal-sensors = <&pm8150_temp>;

> +

> +		trips {

> +			trip0 {

> +				temperature = <95000>;

> +				hysteresis = <0>;

> +				type = "passive";

> +			};

> +

> +			trip1 {

> +				temperature = <115000>;

> +				hysteresis = <0>;

> +				type = "passive";

> +			};

> +

> +			trip2 {

> +				temperature = <145000>;

> +				hysteresis = <0>;

> +				type = "passive";

> +			};

> +		};

> +

> +	};


Not sure about this, Amit..? Should this also not be in board dts?

Similar comments on similar ones for rest of the patch as well..

-- 
~Vinod
Dmitry Baryshkov June 4, 2020, 3:03 p.m. UTC | #2
On 04/06/2020 13:47, Vinod Koul wrote:
> On 04-06-20, 03:43, Dmitry Baryshkov wrote:

>> Add temperature alarm and thermal zone configuration to all three

>> pm8150 instances. Configuration is largely based on the msm-4.19 tree.

>> These alarms use main adc of the pmic. Separate temperature adc is not

>> supported yet.

>>

>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

>> ---

>>   arch/arm64/boot/dts/qcom/pm8150.dtsi  | 41 +++++++++++++++++++++++--

>>   arch/arm64/boot/dts/qcom/pm8150b.dtsi | 43 +++++++++++++++++++++++++--

>>   arch/arm64/boot/dts/qcom/pm8150l.dtsi | 43 +++++++++++++++++++++++++--

>>   3 files changed, 119 insertions(+), 8 deletions(-)

>>

>> diff --git a/arch/arm64/boot/dts/qcom/pm8150.dtsi b/arch/arm64/boot/dts/qcom/pm8150.dtsi

>> index c0b197458665..fee2db42f4cb 100644

>> --- a/arch/arm64/boot/dts/qcom/pm8150.dtsi

>> +++ b/arch/arm64/boot/dts/qcom/pm8150.dtsi

>> @@ -30,6 +30,15 @@ pwrkey {

>>   			};

>>   		};

>>   

>> +		pm8150_temp: temp-alarm@2400 {

>> +			compatible = "qcom,spmi-temp-alarm";

>> +			reg = <0x2400>;

>> +			interrupts = <0x0 0x24 0x0 IRQ_TYPE_EDGE_BOTH>;

>> +			io-channels = <&pm8150_adc ADC5_DIE_TEMP>;

>> +			io-channel-names = "thermal";

>> +			#thermal-sensor-cells = <0>;

>> +		};

>> +

>>   		pm8150_adc: adc@3100 {

>>   			compatible = "qcom,spmi-adc5";

>>   			reg = <0x3100>;

>> @@ -38,8 +47,6 @@ pm8150_adc: adc@3100 {

>>   			#io-channel-cells = <1>;

>>   			interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;

>>   

>> -			status = "disabled";

>> -

> 

> This should not be removed, rather than this please add enabled in you

> board dts file

> 

>>   			ref-gnd@0 {

>>   				reg = <ADC5_REF_GND>;

>>   				qcom,pre-scaling = <1 1>;

>> @@ -85,3 +92,33 @@ pmic@1 {

>>   		#size-cells = <0>;

>>   	};

>>   };

>> +

>> +&thermal_zones {

>> +	pm8150_temp {

>> +		polling-delay-passive = <0>;

>> +		polling-delay = <0>;

>> +

>> +		thermal-sensors = <&pm8150_temp>;

>> +

>> +		trips {

>> +			trip0 {

>> +				temperature = <95000>;

>> +				hysteresis = <0>;

>> +				type = "passive";

>> +			};

>> +

>> +			trip1 {

>> +				temperature = <115000>;

>> +				hysteresis = <0>;

>> +				type = "passive";

>> +			};

>> +

>> +			trip2 {

>> +				temperature = <145000>;

>> +				hysteresis = <0>;

>> +				type = "passive";

>> +			};

>> +		};

>> +

>> +	};

> 

> Not sure about this, Amit..? Should this also not be in board dts?

> 

> Similar comments on similar ones for rest of the patch as well..


I'm not so sure. This part of the configuration seems generic to me. 
Unlike adc-tm config, which definitely goes to the board file.

I can split this into a separate pm8150-temp.dtsi file. Does that sound 
better?


-- 
With best wishes
Dmitry
Vinod Koul June 5, 2020, 4:39 a.m. UTC | #3
Sorry missed ccing Amit, done now.

On 04-06-20, 18:03, Dmitry Baryshkov wrote:
> On 04/06/2020 13:47, Vinod Koul wrote:

> > On 04-06-20, 03:43, Dmitry Baryshkov wrote:


> > >   		pm8150_adc: adc@3100 {

> > >   			compatible = "qcom,spmi-adc5";

> > >   			reg = <0x3100>;

> > > @@ -38,8 +47,6 @@ pm8150_adc: adc@3100 {

> > >   			#io-channel-cells = <1>;

> > >   			interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;

> > > -			status = "disabled";

> > > -

> > 

> > This should not be removed, rather than this please add enabled in you

> > board dts file


...

> > > +&thermal_zones {

> > > +	pm8150_temp {

> > > +		polling-delay-passive = <0>;

> > > +		polling-delay = <0>;

> > > +

> > > +		thermal-sensors = <&pm8150_temp>;

> > > +

> > > +		trips {

> > > +			trip0 {

> > > +				temperature = <95000>;

> > > +				hysteresis = <0>;

> > > +				type = "passive";

> > > +			};

> > > +

> > > +			trip1 {

> > > +				temperature = <115000>;

> > > +				hysteresis = <0>;

> > > +				type = "passive";

> > > +			};

> > > +

> > > +			trip2 {

> > > +				temperature = <145000>;

> > > +				hysteresis = <0>;

> > > +				type = "passive";

> > > +			};

> > > +		};

> > > +

> > > +	};

> > 

> > Not sure about this, Amit..? Should this also not be in board dts?

> > 

> > Similar comments on similar ones for rest of the patch as well..

> 

> I'm not so sure. This part of the configuration seems generic to me. Unlike

> adc-tm config, which definitely goes to the board file.


I think the temperature values may be board specific, Amit can confirm
that. If that is the case then this belongs to board dts, otherwise here :)

> I can split this into a separate pm8150-temp.dtsi file. Does that sound

> better?


That might make it worse, we don't do splitting.

-- 
~Vinod
Dmitry Baryshkov June 5, 2020, 2:37 p.m. UTC | #4
Hello,

On Fri, 5 Jun 2020 at 07:40, Vinod Koul <vkoul@kernel.org> wrote:
>

>

> Sorry missed ccing Amit, done now.

>

> On 04-06-20, 18:03, Dmitry Baryshkov wrote:

> > On 04/06/2020 13:47, Vinod Koul wrote:

> > > On 04-06-20, 03:43, Dmitry Baryshkov wrote:

>

> > > >                   pm8150_adc: adc@3100 {

> > > >                           compatible = "qcom,spmi-adc5";

> > > >                           reg = <0x3100>;

> > > > @@ -38,8 +47,6 @@ pm8150_adc: adc@3100 {

> > > >                           #io-channel-cells = <1>;

> > > >                           interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;

> > > > -                 status = "disabled";

> > > > -

> > >

> > > This should not be removed, rather than this please add enabled in you

> > > board dts file


Compare this with pm8998.dtsi. It has all nodes enabled by default.

> > > > +&thermal_zones {

> > > > + pm8150_temp {

> > > > +         polling-delay-passive = <0>;

> > > > +         polling-delay = <0>;

> > > > +

> > > > +         thermal-sensors = <&pm8150_temp>;

> > > > +

> > > > +         trips {

> > > > +                 trip0 {

> > > > +                         temperature = <95000>;

> > > > +                         hysteresis = <0>;

> > > > +                         type = "passive";

> > > > +                 };

> > > > +

> > > > +                 trip1 {

> > > > +                         temperature = <115000>;

> > > > +                         hysteresis = <0>;

> > > > +                         type = "passive";

> > > > +                 };

> > > > +

> > > > +                 trip2 {

> > > > +                         temperature = <145000>;

> > > > +                         hysteresis = <0>;

> > > > +                         type = "passive";

> > > > +                 };

> > > > +         };

> > > > +

> > > > + };

> > >

> > > Not sure about this, Amit..? Should this also not be in board dts?

> > >

> > > Similar comments on similar ones for rest of the patch as well..

> >

> > I'm not so sure. This part of the configuration seems generic to me. Unlike

> > adc-tm config, which definitely goes to the board file.

>

> I think the temperature values may be board specific, Amit can confirm

> that. If that is the case then this belongs to board dts, otherwise here :)


Again, pm8998 has these thermal values in the dtsi file. In V2 I will
update these three files to follow pm8998.dtsi.

-- 
With best wishes
Dmitry
Amit Kucheria June 8, 2020, 8:44 a.m. UTC | #5
On Fri, Jun 5, 2020 at 10:10 AM Vinod Koul <vkoul@kernel.org> wrote:
>

>

> Sorry missed ccing Amit, done now.

>

> On 04-06-20, 18:03, Dmitry Baryshkov wrote:

> > On 04/06/2020 13:47, Vinod Koul wrote:

> > > On 04-06-20, 03:43, Dmitry Baryshkov wrote:

>

> > > >                   pm8150_adc: adc@3100 {

> > > >                           compatible = "qcom,spmi-adc5";

> > > >                           reg = <0x3100>;

> > > > @@ -38,8 +47,6 @@ pm8150_adc: adc@3100 {

> > > >                           #io-channel-cells = <1>;

> > > >                           interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;

> > > > -                 status = "disabled";

> > > > -

> > >

> > > This should not be removed, rather than this please add enabled in you

> > > board dts file


Is the default disabled for a reason?

I'd expect the default to be enabled and then board-specific dts to
disable a specific adc if needed.

> ...

>

> > > > +&thermal_zones {

> > > > + pm8150_temp {

> > > > +         polling-delay-passive = <0>;

> > > > +         polling-delay = <0>;

> > > > +

> > > > +         thermal-sensors = <&pm8150_temp>;

> > > > +

> > > > +         trips {

> > > > +                 trip0 {

> > > > +                         temperature = <95000>;

> > > > +                         hysteresis = <0>;

> > > > +                         type = "passive";

> > > > +                 };

> > > > +

> > > > +                 trip1 {

> > > > +                         temperature = <115000>;

> > > > +                         hysteresis = <0>;

> > > > +                         type = "passive";


Since there is not cooling map associated with these two trips i.e. no
mitigation action, this trip is informational. So make it of type
"hot".

Is there really a need for two passive trip points? Just one at 115
should be enough?

> > > > +                 };

> > > > +

> > > > +                 trip2 {

> > > > +                         temperature = <145000>;


Are you sure about this? That is a very toasty temperature. :-)

> > > > +                         hysteresis = <0>;

> > > > +                         type = "passive";


The last trip should typically be of type "critical". That is the
temperature at which the system will initiate a shutdown.

> > > > +                 };

> > > > +         };

> > > > +

> > > > + };

> > >

> > > Not sure about this, Amit..? Should this also not be in board dts?

> > >

> > > Similar comments on similar ones for rest of the patch as well..

> >

> > I'm not so sure. This part of the configuration seems generic to me. Unlike

> > adc-tm config, which definitely goes to the board file.

>

> I think the temperature values may be board specific, Amit can confirm

> that. If that is the case then this belongs to board dts, otherwise here :)


While the temp values can be board-specific e.g. if the same SoC is
used in a mobile phone and a laptop, the thresholds rarely change, in
my experience.

I think they can stay in the pmic dtsi file and any specific board can
override if necessary.

> > I can split this into a separate pm8150-temp.dtsi file. Does that sound

> > better?

>

> That might make it worse, we don't do splitting.


Right, let's not split it.

>

> --

> ~Vinod
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/pm8150.dtsi b/arch/arm64/boot/dts/qcom/pm8150.dtsi
index c0b197458665..fee2db42f4cb 100644
--- a/arch/arm64/boot/dts/qcom/pm8150.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8150.dtsi
@@ -30,6 +30,15 @@  pwrkey {
 			};
 		};
 
+		pm8150_temp: temp-alarm@2400 {
+			compatible = "qcom,spmi-temp-alarm";
+			reg = <0x2400>;
+			interrupts = <0x0 0x24 0x0 IRQ_TYPE_EDGE_BOTH>;
+			io-channels = <&pm8150_adc ADC5_DIE_TEMP>;
+			io-channel-names = "thermal";
+			#thermal-sensor-cells = <0>;
+		};
+
 		pm8150_adc: adc@3100 {
 			compatible = "qcom,spmi-adc5";
 			reg = <0x3100>;
@@ -38,8 +47,6 @@  pm8150_adc: adc@3100 {
 			#io-channel-cells = <1>;
 			interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
 
-			status = "disabled";
-
 			ref-gnd@0 {
 				reg = <ADC5_REF_GND>;
 				qcom,pre-scaling = <1 1>;
@@ -85,3 +92,33 @@  pmic@1 {
 		#size-cells = <0>;
 	};
 };
+
+&thermal_zones {
+	pm8150_temp {
+		polling-delay-passive = <0>;
+		polling-delay = <0>;
+
+		thermal-sensors = <&pm8150_temp>;
+
+		trips {
+			trip0 {
+				temperature = <95000>;
+				hysteresis = <0>;
+				type = "passive";
+			};
+
+			trip1 {
+				temperature = <115000>;
+				hysteresis = <0>;
+				type = "passive";
+			};
+
+			trip2 {
+				temperature = <145000>;
+				hysteresis = <0>;
+				type = "passive";
+			};
+		};
+
+	};
+};
diff --git a/arch/arm64/boot/dts/qcom/pm8150b.dtsi b/arch/arm64/boot/dts/qcom/pm8150b.dtsi
index 40b5d75a4a1d..e93d16f2d1e0 100644
--- a/arch/arm64/boot/dts/qcom/pm8150b.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8150b.dtsi
@@ -22,7 +22,16 @@  power-on@800 {
 			status = "disabled";
 		};
 
-		adc@3100 {
+		pm8150b_temp: temp-alarm@2400 {
+			compatible = "qcom,spmi-temp-alarm";
+			reg = <0x2400>;
+			interrupts = <0x2 0x24 0x0 IRQ_TYPE_EDGE_BOTH>;
+			io-channels = <&pm8150b_adc ADC5_DIE_TEMP>;
+			io-channel-names = "thermal";
+			#thermal-sensor-cells = <0>;
+		};
+
+		pm8150b_adc: adc@3100 {
 			compatible = "qcom,spmi-adc5";
 			reg = <0x3100>;
 			#address-cells = <1>;
@@ -30,8 +39,6 @@  adc@3100 {
 			#io-channel-cells = <1>;
 			interrupts = <0x2 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
 
-			status = "disabled";
-
 			ref-gnd@0 {
 				reg = <ADC5_REF_GND>;
 				qcom,pre-scaling = <1 1>;
@@ -74,3 +81,33 @@  pmic@3 {
 		#size-cells = <0>;
 	};
 };
+
+&thermal_zones {
+	pm8150b_temp {
+		polling-delay-passive = <0>;
+		polling-delay = <0>;
+
+		thermal-sensors = <&pm8150b_temp>;
+
+		trips {
+			trip0 {
+				temperature = <95000>;
+				hysteresis = <0>;
+				type = "passive";
+			};
+
+			trip1 {
+				temperature = <115000>;
+				hysteresis = <0>;
+				type = "passive";
+			};
+
+			trip2 {
+				temperature = <145000>;
+				hysteresis = <0>;
+				type = "passive";
+			};
+		};
+
+	};
+};
diff --git a/arch/arm64/boot/dts/qcom/pm8150l.dtsi b/arch/arm64/boot/dts/qcom/pm8150l.dtsi
index cf05e0685d10..1edf87c95a27 100644
--- a/arch/arm64/boot/dts/qcom/pm8150l.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8150l.dtsi
@@ -22,7 +22,16 @@  power-on@800 {
 			status = "disabled";
 		};
 
-		adc@3100 {
+		pm8150l_temp: temp-alarm@2400 {
+			compatible = "qcom,spmi-temp-alarm";
+			reg = <0x2400>;
+			interrupts = <0x4 0x24 0x0 IRQ_TYPE_EDGE_BOTH>;
+			io-channels = <&pm8150l_adc ADC5_DIE_TEMP>;
+			io-channel-names = "thermal";
+			#thermal-sensor-cells = <0>;
+		};
+
+		pm8150l_adc: adc@3100 {
 			compatible = "qcom,spmi-adc5";
 			reg = <0x3100>;
 			#address-cells = <1>;
@@ -30,8 +39,6 @@  adc@3100 {
 			#io-channel-cells = <1>;
 			interrupts = <0x4 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
 
-			status = "disabled";
-
 			ref-gnd@0 {
 				reg = <ADC5_REF_GND>;
 				qcom,pre-scaling = <1 1>;
@@ -68,3 +75,33 @@  pmic@5 {
 		#size-cells = <0>;
 	};
 };
+
+&thermal_zones {
+	pm8150l_temp {
+		polling-delay-passive = <0>;
+		polling-delay = <0>;
+
+		thermal-sensors = <&pm8150l_temp>;
+
+		trips {
+			trip0 {
+				temperature = <95000>;
+				hysteresis = <0>;
+				type = "passive";
+			};
+
+			trip1 {
+				temperature = <115000>;
+				hysteresis = <0>;
+				type = "passive";
+			};
+
+			trip2 {
+				temperature = <145000>;
+				hysteresis = <0>;
+				type = "passive";
+			};
+		};
+
+	};
+};