[PATCH 0/4] ARM: tegra: soctherm bugfixes

[PATCH 0/4] ARM: tegra: soctherm bugfixes

[Nicolas Chauvet]

When using tegra_soctherm driver on jetson-tk1, the following messages
can be seen:
from kernel: tegra_soctherm 700e2000.thermal-sensor: 
 throttle-cfg: heavy: no throt prop or invalid prop
 soctherm: trip temperature -2147483647 forced to -127000
 thermtrip: will shut down when cpu reaches 101000 mC
 soctherm: trip temperature -2147483647 forced to -127000
 thermtrip: will shut down when gpu reaches 101000 mC
 soctherm: trip temperature -2147483647 forced to -127000
 thermtrip: will shut down when pll reaches 103000 mC
 throttrip: pll: missing hot temperature
 soctherm: trip temperature -2147483647 forced to -127000
 thermtrip: will shut down when mem reaches 101000 mC
 throttrip: mem: missing hot temperature
 IRQ index 1 not found

This serie fixes two errors and two warnings that are reported in dmesg
It was compiled and tested at runtime on jetson-tk1 only.

Nicolas Chauvet (4):
  ARM: tegra: Add missing gpu-throt-level to tegra124 soctherm
  ARM: tegra: Add missing hot temperatures to tegra124 thermal-zones
  arm64: tegra: Add missing hot temperatures to tegra132 thermal-zones
  ARM: tegra: Avoid setting edp_irq when not relevant

 arch/arm/boot/dts/tegra124.dtsi          | 11 +++++++++++
 arch/arm64/boot/dts/nvidia/tegra132.dtsi | 10 ++++++++++
 drivers/thermal/tegra/soctherm.c         | 17 +++++++++++------
 3 files changed, 32 insertions(+), 6 deletions(-)

-- 
2.25.4

[PATCH 1/4] ARM: tegra: Add missing gpu-throt-level to tegra124 soctherm

[Nicolas Chauvet]

On jetson-tk1 the following message can be seen:
 tegra_soctherm 700e2000.thermal-sensor: throttle-cfg: heavy: no throt prop or invalid prop

This patch will fix the invalid prop issue according to the binding.

Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>
---
 arch/arm/boot/dts/tegra124.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 64f488ba1e72..a0fa5821a232 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -910,6 +910,7 @@ throttle-cfgs {
 			throttle_heavy: heavy {
 				nvidia,priority = <100>;
 				nvidia,cpu-throt-percent = <85>;
+				nvidia,gpu-throt-level = <TEGRA_SOCTHERM_THROT_LEVEL_HIGH>;
 
 				#cooling-cells = <2>;
 			};
-- 
2.25.4

[PATCH 2/4] ARM: tegra: Add missing hot temperatures to tegra124 thermal-zones

[Nicolas Chauvet]

According to dmesg, thermal-zones for mem and cpu are missing hot
temperatures properties.

  throttrip: pll: missing hot temperature
...
  throttrip: mem: missing hot temperature
...

Adding them will clear the messages.

Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>
---
 arch/arm/boot/dts/tegra124.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index a0fa5821a232..c71d597ace01 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -1248,6 +1248,11 @@ mem-shutdown-trip {
 					hysteresis = <0>;
 					type = "critical";
 				};
+				mem-throttle-trip {
+					temperature = <99000>;
+					hysteresis = <1000>;
+					type = "hot";
+				};
 			};
 
 			cooling-maps {
@@ -1299,6 +1304,11 @@ pllx-shutdown-trip {
 					hysteresis = <0>;
 					type = "critical";
 				};
+				pllx-throttle-trip {
+					temperature = <99000>;
+					hysteresis = <1000>;
+					type = "hot";
+				};
 			};
 
 			cooling-maps {
-- 
2.25.4

[PATCH 3/4] arm64: tegra: Add missing hot temperatures to tegra132 thermal-zones

[Nicolas Chauvet]

According to dmesg, thermal-zones for mem and cpu are missing hot
temperatures properties.

  throttrip: pll: missing hot temperature
...
  throttrip: mem: missing hot temperature
...

Adding them will clear the messages.

Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>
---
 arch/arm64/boot/dts/nvidia/tegra132.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra132.dtsi b/arch/arm64/boot/dts/nvidia/tegra132.dtsi
index e40281510c0c..cd913e59ba26 100644
--- a/arch/arm64/boot/dts/nvidia/tegra132.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra132.dtsi
@@ -925,6 +925,11 @@ mem_shutdown_trip {
 					hysteresis = <1000>;
 					type = "critical";
 				};
+				mem_throttle_trip {
+					temperature = <99000>;
+					hysteresis = <1000>;
+					type = "hot";
+				};
 			};
 
 			cooling-maps {
@@ -975,6 +980,11 @@ pllx_shutdown_trip {
 					hysteresis = <1000>;
 					type = "critical";
 				};
+				pllx_throttle_trip {
+					temperature = <99000>;
+					hysteresis = <1000>;
+					type = "hot";
+				};
 			};
 
 			cooling-maps {
-- 
2.25.4

[PATCH 4/4] ARM: tegra: Avoid setting edp_irq when not relevant

[Nicolas Chauvet]

According to the binding, the edp_irq is not available on tegra124/132

This commit moves the initialization of tegra->edp_irq after the
introduced SoCs condition. This will have the following effects:
 - soctherm_interrupts_init will not return prematurely with unfinished
thermal_irq initialization on tegra124 and tegra132
 - edp_irq initialization will be bypassed when not relevant

As a result, this will clear the following error when loading the driver:
  kernel: tegra_soctherm 700e2000.thermal-sensor: IRQ index 1 not found

Fixes: 4a04beb1bf2e (thermal: tegra: add support for EDP IRQ)
Cc: stable@vger.kernel.org
Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>
---
 drivers/thermal/tegra/soctherm.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
index 66e0639da4bf..0a7dc988f25f 100644
--- a/drivers/thermal/tegra/soctherm.c
+++ b/drivers/thermal/tegra/soctherm.c
@@ -2025,12 +2025,6 @@ static int soctherm_interrupts_init(struct platform_device *pdev,
 		return 0;
 	}
 
-	tegra->edp_irq = platform_get_irq(pdev, 1);
-	if (tegra->edp_irq < 0) {
-		dev_dbg(&pdev->dev, "get 'edp_irq' failed.\n");
-		return 0;
-	}
-
 	ret = devm_request_threaded_irq(&pdev->dev,
 					tegra->thermal_irq,
 					soctherm_thermal_isr,
@@ -2043,6 +2037,17 @@ static int soctherm_interrupts_init(struct platform_device *pdev,
 		return ret;
 	}
 
+	/* None of the tegra124 and tegra132 SoCs have edp_irq */
+	if (of_machine_is_compatible("nvidia,tegra124") ||
+		of_machine_is_compatible("nvidia,tegra132"))
+			return 0;
+
+	tegra->edp_irq = platform_get_irq(pdev, 1);
+	if (tegra->edp_irq < 0) {
+		dev_dbg(&pdev->dev, "get 'edp_irq' failed.\n");
+		return 0;
+	}
+
 	ret = devm_request_threaded_irq(&pdev->dev,
 					tegra->edp_irq,
 					soctherm_edp_isr,
-- 
2.25.4

Re: [PATCH 4/4] ARM: tegra: Avoid setting edp_irq when not relevant

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3380 bytes --]

On Mon, Sep 14, 2020 at 03:37:39PM +0200, Nicolas Chauvet wrote:
> According to the binding, the edp_irq is not available on tegra124/132
> 
> This commit moves the initialization of tegra->edp_irq after the
> introduced SoCs condition. This will have the following effects:
>  - soctherm_interrupts_init will not return prematurely with unfinished
> thermal_irq initialization on tegra124 and tegra132
>  - edp_irq initialization will be bypassed when not relevant
> 
> As a result, this will clear the following error when loading the driver:
>   kernel: tegra_soctherm 700e2000.thermal-sensor: IRQ index 1 not found
> 
> Fixes: 4a04beb1bf2e (thermal: tegra: add support for EDP IRQ)
> Cc: stable@vger.kernel.org
> Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>
> ---
>  drivers/thermal/tegra/soctherm.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)

Your subject needs a different prefix. As it is this looks like
something to apply to the Tegra tree, but it actually needs to go
through Zhang's and Daniel's thermal tree. Also make sure to send
patches To: the maintainers of the subsystem.

> 
> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
> index 66e0639da4bf..0a7dc988f25f 100644
> --- a/drivers/thermal/tegra/soctherm.c
> +++ b/drivers/thermal/tegra/soctherm.c
> @@ -2025,12 +2025,6 @@ static int soctherm_interrupts_init(struct platform_device *pdev,
>  		return 0;
>  	}
>  
> -	tegra->edp_irq = platform_get_irq(pdev, 1);
> -	if (tegra->edp_irq < 0) {
> -		dev_dbg(&pdev->dev, "get 'edp_irq' failed.\n");
> -		return 0;
> -	}
> -
>  	ret = devm_request_threaded_irq(&pdev->dev,
>  					tegra->thermal_irq,
>  					soctherm_thermal_isr,
> @@ -2043,6 +2037,17 @@ static int soctherm_interrupts_init(struct platform_device *pdev,
>  		return ret;
>  	}
>  
> +	/* None of the tegra124 and tegra132 SoCs have edp_irq */
> +	if (of_machine_is_compatible("nvidia,tegra124") ||
> +		of_machine_is_compatible("nvidia,tegra132"))
> +			return 0;
> +

I'd prefer to turn this into a per-SoC capability flag. You can add
something like this:

	struct tegra_soctherm_soc {
		...
		bool has_edp_irq;
	};

	...

	const struct tegra_soctherm_soc tegra124_soctherm = {
		...
		.has_edp_irq = false,
	};

	...

	const struct tegra_soctherm_soc tegra210_soctherm = {
		...
		.has_edp_irq = true,
	};

	...

and so on. This makes it more obvious why you conditionalize certain
code segments and avoids complicated conditionals.

Also, please avoid returning success early. That's very confusing
because it can lead to people adding code to the end of this function
that will never be run on the chips that you've excluded above.

So I think a better way to write this would be:

	if (tegra->soc->has_edp_irq) {
		/* get IRQ */

		/* request IRQ */
	}

That way people can simply continue adding to the bottom of the function
and that code will get executed, which is much more straightforward than
if you invert the condition.

Thierry

> +	tegra->edp_irq = platform_get_irq(pdev, 1);
> +	if (tegra->edp_irq < 0) {
> +		dev_dbg(&pdev->dev, "get 'edp_irq' failed.\n");
> +		return 0;
> +	}
> +
>  	ret = devm_request_threaded_irq(&pdev->dev,
>  					tegra->edp_irq,
>  					soctherm_edp_isr,
> -- 
> 2.25.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]