diff mbox series

[v2] arm64: dts: qcom: sc8280xp: Introduce additional tsens instances

Message ID 20240126-sc8280xp-tsens2_3-v2-1-8504d18828de@quicinc.com
State New
Headers
Series [v2] arm64: dts: qcom: sc8280xp: Introduce additional tsens instances |

Commit Message

Bjorn Andersson Jan. 26, 2024, 3:12 p.m. UTC 
  The SC8280XP contains two additional tsens instances, providing among
other things thermal measurements for the GPU.

Add these and a GPU thermal-zone.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
Changes in v2:
- Drop TM/SROT comments
- Remove polling delays, rely on interrupts
- Link to v1: https://lore.kernel.org/r/20240118-sc8280xp-tsens2_3-v1-1-e86bce14f6bf@quicinc.com
---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 37 ++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)


---
base-commit: 943b9f0ab2cfbaea148dd6ac279957eb08b96904
change-id: 20240118-sc8280xp-tsens2_3-a5fd9a48d655

Best regards,

Comments

Johan Hovold Jan. 26, 2024, 4:36 p.m. UTC | #1 
On Fri, Jan 26, 2024 at 07:12:45AM -0800, Bjorn Andersson wrote:
> The SC8280XP contains two additional tsens instances, providing among
> other things thermal measurements for the GPU.
> 
> Add these and a GPU thermal-zone.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
> Changes in v2:
> - Drop TM/SROT comments
> - Remove polling delays, rely on interrupts
> - Link to v1: https://lore.kernel.org/r/20240118-sc8280xp-tsens2_3-v1-1-e86bce14f6bf@quicinc.com
> ---
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 37 ++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index febf28356ff8..7bfbb1bd8f4a 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -4033,6 +4033,28 @@ tsens1: thermal-sensor@c265000 {
>  			#thermal-sensor-cells = <1>;
>  		};
>  
> +		tsens2: thermal-sensor@c251000 {
> +			compatible = "qcom,sc8280xp-tsens", "qcom,tsens-v2";
> +			reg = <0 0x0c251000 0 0x1ff>,
> +			      <0 0x0c224000 0 0x8>;
> +			#qcom,sensors = <11>;
> +			interrupts-extended = <&pdc 122 IRQ_TYPE_LEVEL_HIGH>,
> +					      <&pdc 124 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "uplow", "critical";
> +			#thermal-sensor-cells = <1>;
> +		};
> +
> +		tsens3: thermal-sensor@c252000 {
> +			compatible = "qcom,sc8280xp-tsens", "qcom,tsens-v2";
> +			reg = <0 0x0c252000 0 0x1ff>,
> +			      <0 0x0c225000 0 0x8>;
> +			#qcom,sensors = <5>;
> +			interrupts-extended = <&pdc 123 IRQ_TYPE_LEVEL_HIGH>,
> +					      <&pdc 125 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "uplow", "critical";
> +			#thermal-sensor-cells = <1>;
> +		};

These should go before tsens0 based on the unit address.

> +
>  		aoss_qmp: power-management@c300000 {
>  			compatible = "qcom,sc8280xp-aoss-qmp", "qcom,aoss-qmp";
>  			reg = <0 0x0c300000 0 0x400>;
> @@ -5212,6 +5234,21 @@ cpu-crit {
>  			};
>  		};
>  
> +		gpu-thermal {
> +			polling-delay-passive = <0>;
> +			polling-delay = <0>;
> +
> +			thermal-sensors = <&tsens2 2>;
> +
> +			trips {
> +				cpu-crit {
> +					temperature = <110000>;
> +					hysteresis = <1000>;
> +					type = "critical";
> +				};
> +			};
> +		};

Shall you submit a follow-on patch to set the polling delays to zero
for the other thermal zones (cpu, cluster, mem) so that we don't poll
for those?

Looks good to me otherwise: 

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

Johan
Bjorn Andersson Jan. 26, 2024, 4:51 p.m. UTC | #2 
On Fri, Jan 26, 2024 at 05:36:10PM +0100, Johan Hovold wrote:
> On Fri, Jan 26, 2024 at 07:12:45AM -0800, Bjorn Andersson wrote:
> > The SC8280XP contains two additional tsens instances, providing among
> > other things thermal measurements for the GPU.
> > 
> > Add these and a GPU thermal-zone.
> > 
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> > Changes in v2:
> > - Drop TM/SROT comments
> > - Remove polling delays, rely on interrupts
> > - Link to v1: https://lore.kernel.org/r/20240118-sc8280xp-tsens2_3-v1-1-e86bce14f6bf@quicinc.com
> > ---
> >  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 37 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > index febf28356ff8..7bfbb1bd8f4a 100644
> > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > @@ -4033,6 +4033,28 @@ tsens1: thermal-sensor@c265000 {
> >  			#thermal-sensor-cells = <1>;
> >  		};
> >  
> > +		tsens2: thermal-sensor@c251000 {
> > +			compatible = "qcom,sc8280xp-tsens", "qcom,tsens-v2";
> > +			reg = <0 0x0c251000 0 0x1ff>,
> > +			      <0 0x0c224000 0 0x8>;
> > +			#qcom,sensors = <11>;
> > +			interrupts-extended = <&pdc 122 IRQ_TYPE_LEVEL_HIGH>,
> > +					      <&pdc 124 IRQ_TYPE_LEVEL_HIGH>;
> > +			interrupt-names = "uplow", "critical";
> > +			#thermal-sensor-cells = <1>;
> > +		};
> > +
> > +		tsens3: thermal-sensor@c252000 {
> > +			compatible = "qcom,sc8280xp-tsens", "qcom,tsens-v2";
> > +			reg = <0 0x0c252000 0 0x1ff>,
> > +			      <0 0x0c225000 0 0x8>;
> > +			#qcom,sensors = <5>;
> > +			interrupts-extended = <&pdc 123 IRQ_TYPE_LEVEL_HIGH>,
> > +					      <&pdc 125 IRQ_TYPE_LEVEL_HIGH>;
> > +			interrupt-names = "uplow", "critical";
> > +			#thermal-sensor-cells = <1>;
> > +		};
> 
> These should go before tsens0 based on the unit address.
> 

You're right, thanks for spotting that.

> > +
> >  		aoss_qmp: power-management@c300000 {
> >  			compatible = "qcom,sc8280xp-aoss-qmp", "qcom,aoss-qmp";
> >  			reg = <0 0x0c300000 0 0x400>;
> > @@ -5212,6 +5234,21 @@ cpu-crit {
> >  			};
> >  		};
> >  
> > +		gpu-thermal {
> > +			polling-delay-passive = <0>;
> > +			polling-delay = <0>;
> > +
> > +			thermal-sensors = <&tsens2 2>;
> > +
> > +			trips {
> > +				cpu-crit {
> > +					temperature = <110000>;
> > +					hysteresis = <1000>;
> > +					type = "critical";
> > +				};
> > +			};
> > +		};
> 
> Shall you submit a follow-on patch to set the polling delays to zero
> for the other thermal zones (cpu, cluster, mem) so that we don't poll
> for those?
> 

I optimistically interpreted Konrad's response as a promise by him to do
so ;)

I do like his patch which remove the poll-properties for non-polling
mode. Would be nice to not first change the values to 0 and then remove
the properties...

> Looks good to me otherwise: 
> 
> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
> 

Thanks,
Bjorn

> Johan
Johan Hovold Jan. 26, 2024, 5 p.m. UTC | #3 
On Fri, Jan 26, 2024 at 08:51:13AM -0800, Bjorn Andersson wrote:
> On Fri, Jan 26, 2024 at 05:36:10PM +0100, Johan Hovold wrote:

> > Shall you submit a follow-on patch to set the polling delays to zero
> > for the other thermal zones (cpu, cluster, mem) so that we don't poll
> > for those?
> 
> I optimistically interpreted Konrad's response as a promise by him to do
> so ;)
> 
> I do like his patch which remove the poll-properties for non-polling
> mode. Would be nice to not first change the values to 0 and then remove
> the properties...

No, that should not be an issue as it allows us to get rid of the
polling without waiting for a binding update which may or may not
materialise in 6.9-rc1.

But whoever updates those properties need to do some proper testing to
make sure that those interrupts really work.

Johan
Konrad Dybcio Jan. 26, 2024, 9:20 p.m. UTC | #4 
On 26.01.2024 16:12, Bjorn Andersson wrote:
> The SC8280XP contains two additional tsens instances, providing among
> other things thermal measurements for the GPU.
> 
> Add these and a GPU thermal-zone.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
> Changes in v2:
> - Drop TM/SROT comments
> - Remove polling delays, rely on interrupts
> - Link to v1: https://lore.kernel.org/r/20240118-sc8280xp-tsens2_3-v1-1-e86bce14f6bf@quicinc.com
> ---
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 37 ++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index febf28356ff8..7bfbb1bd8f4a 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -4033,6 +4033,28 @@ tsens1: thermal-sensor@c265000 {
>  			#thermal-sensor-cells = <1>;
>  		};
>  
> +		tsens2: thermal-sensor@c251000 {
> +			compatible = "qcom,sc8280xp-tsens", "qcom,tsens-v2";
> +			reg = <0 0x0c251000 0 0x1ff>,
> +			      <0 0x0c224000 0 0x8>;
> +			#qcom,sensors = <11>;
> +			interrupts-extended = <&pdc 122 IRQ_TYPE_LEVEL_HIGH>,
> +					      <&pdc 124 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "uplow", "critical";
> +			#thermal-sensor-cells = <1>;
> +		};
> +
> +		tsens3: thermal-sensor@c252000 {
> +			compatible = "qcom,sc8280xp-tsens", "qcom,tsens-v2";
> +			reg = <0 0x0c252000 0 0x1ff>,
> +			      <0 0x0c225000 0 0x8>;
> +			#qcom,sensors = <5>;
> +			interrupts-extended = <&pdc 123 IRQ_TYPE_LEVEL_HIGH>,
> +					      <&pdc 125 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "uplow", "critical";
> +			#thermal-sensor-cells = <1>;

With the sorting issue that Johan mentioned resolved:

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
Konrad Dybcio Jan. 26, 2024, 9:23 p.m. UTC | #5 
On 26.01.2024 18:00, Johan Hovold wrote:
> On Fri, Jan 26, 2024 at 08:51:13AM -0800, Bjorn Andersson wrote:
>> On Fri, Jan 26, 2024 at 05:36:10PM +0100, Johan Hovold wrote:
> 
>>> Shall you submit a follow-on patch to set the polling delays to zero
>>> for the other thermal zones (cpu, cluster, mem) so that we don't poll
>>> for those?
>>
>> I optimistically interpreted Konrad's response as a promise by him to do
>> so ;)
>>
>> I do like his patch which remove the poll-properties for non-polling
>> mode. Would be nice to not first change the values to 0 and then remove
>> the properties...

That was my intention as well..

> 
> No, that should not be an issue as it allows us to get rid of the
> polling without waiting for a binding update which may or may not
> materialise in 6.9-rc1.

If you really insist, I may do that, but if the thermal guys act on it
quickly and we negotiate an immutable branch, we can simply but atop it,
saving the submitter timeof(patchset), the reviewers timeof(verify), the
build bots timeof(builds) and the applier timeof(pick-build-push)..

> 
> But whoever updates those properties need to do some proper testing to
> make sure that those interrupts really work.

They seem to, check /proc/interrupts before and after adding an e.g. 45degC
trip point on one of the CPU thermal zones, they fire aplenty.

Konrad
Johan Hovold Jan. 29, 2024, 9:38 a.m. UTC | #6 
On Fri, Jan 26, 2024 at 10:23:41PM +0100, Konrad Dybcio wrote:
> On 26.01.2024 18:00, Johan Hovold wrote:
> > On Fri, Jan 26, 2024 at 08:51:13AM -0800, Bjorn Andersson wrote:
> >> On Fri, Jan 26, 2024 at 05:36:10PM +0100, Johan Hovold wrote:
> > 
> >>> Shall you submit a follow-on patch to set the polling delays to zero
> >>> for the other thermal zones (cpu, cluster, mem) so that we don't poll
> >>> for those?
> >>
> >> I optimistically interpreted Konrad's response as a promise by him to do
> >> so ;)
> >>
> >> I do like his patch which remove the poll-properties for non-polling
> >> mode. Would be nice to not first change the values to 0 and then remove
> >> the properties...
> 
> That was my intention as well..
> 
> > 
> > No, that should not be an issue as it allows us to get rid of the
> > polling without waiting for a binding update which may or may not
> > materialise in 6.9-rc1.
> 
> If you really insist, I may do that, but if the thermal guys act on it
> quickly and we negotiate an immutable branch, we can simply but atop it,
> saving the submitter timeof(patchset), the reviewers timeof(verify), the
> build bots timeof(builds) and the applier timeof(pick-build-push)..

Why would introduce such a dependency for really no good reason?

Submit a patch based on the current binding, then when/if your proposed
binding update hits mainline, you can send a *single* patch dropping the
parameters from all qualcomm dtsi.

Updating the binding is a separate and lower priority task. In fact, it
may not even be desirable at all as an omission of adding these
parameters could then lead to broken thermal management on platforms
where the interrupts do not work. Having an explicit poll-delay of zero
at least gives people a reason to think about it when merging a new
platform.

But again, that's a separate discussion. Don't make this patch depend on
that.

> > But whoever updates those properties need to do some proper testing to
> > make sure that those interrupts really work.
> 
> They seem to, check /proc/interrupts before and after adding an e.g. 45degC
> trip point on one of the CPU thermal zones, they fire aplenty.

That's not proper testing. Add/enable debugging in the thermal driver
and make sure that you trigger precisely once when passing the threshold
in both directions.

Johan
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index febf28356ff8..7bfbb1bd8f4a 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -4033,6 +4033,28 @@  tsens1: thermal-sensor@c265000 {
 			#thermal-sensor-cells = <1>;
 		};
 
+		tsens2: thermal-sensor@c251000 {
+			compatible = "qcom,sc8280xp-tsens", "qcom,tsens-v2";
+			reg = <0 0x0c251000 0 0x1ff>,
+			      <0 0x0c224000 0 0x8>;
+			#qcom,sensors = <11>;
+			interrupts-extended = <&pdc 122 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 124 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "uplow", "critical";
+			#thermal-sensor-cells = <1>;
+		};
+
+		tsens3: thermal-sensor@c252000 {
+			compatible = "qcom,sc8280xp-tsens", "qcom,tsens-v2";
+			reg = <0 0x0c252000 0 0x1ff>,
+			      <0 0x0c225000 0 0x8>;
+			#qcom,sensors = <5>;
+			interrupts-extended = <&pdc 123 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 125 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "uplow", "critical";
+			#thermal-sensor-cells = <1>;
+		};
+
 		aoss_qmp: power-management@c300000 {
 			compatible = "qcom,sc8280xp-aoss-qmp", "qcom,aoss-qmp";
 			reg = <0 0x0c300000 0 0x400>;
@@ -5212,6 +5234,21 @@  cpu-crit {
 			};
 		};
 
+		gpu-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+
+			thermal-sensors = <&tsens2 2>;
+
+			trips {
+				cpu-crit {
+					temperature = <110000>;
+					hysteresis = <1000>;
+					type = "critical";
+				};
+			};
+		};
+
 		mem-thermal {
 			polling-delay-passive = <250>;
 			polling-delay = <1000>;