[PATCH v6 0/4] Tegra124 soctherm driver

[PATCH v6 0/4] Tegra124 soctherm driver

[Mikko Perttunen]

Hi,

this series adds support for the thermal monitoring features of the
soctherm unit on the Tegra124 SoC. 

The branch is also available in my github repo,
  git://github.com/cyndis/linux.git soctherm-v6

Thanks,
Mikko

Mikko Perttunen (4):
  of: Add bindings for nvidia,tegra124-soctherm
  ARM: tegra: Add soctherm and thermal zones to Tegra124 device tree
  ARM: tegra: Add thermal trip points for Jetson TK1
  thermal: Add Tegra SOCTHERM thermal management driver

 .../devicetree/bindings/thermal/tegra-soctherm.txt |  53 +++
 arch/arm/boot/dts/tegra124-jetson-tk1.dts          |  44 ++
 arch/arm/boot/dts/tegra124.dtsi                    |  47 ++
 drivers/thermal/Kconfig                            |  10 +
 drivers/thermal/Makefile                           |   1 +
 drivers/thermal/tegra_soctherm.c                   | 471 +++++++++++++++++++++
 include/dt-bindings/thermal/tegra124-soctherm.h    |  13 +
 7 files changed, 639 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
 create mode 100644 drivers/thermal/tegra_soctherm.c
 create mode 100644 include/dt-bindings/thermal/tegra124-soctherm.h

-- 
2.1.0

[PATCH v6 1/4] of: Add bindings for nvidia,tegra124-soctherm

[Mikko Perttunen]

From: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

This adds binding documentation and headers for the Tegra124
SOCTHERM device tree node.

Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Acked-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Acked-by: Eduardo Valentin <edubezval-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/thermal/tegra-soctherm.txt | 53 ++++++++++++++++++++++
 include/dt-bindings/thermal/tegra124-soctherm.h    | 13 ++++++
 2 files changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
 create mode 100644 include/dt-bindings/thermal/tegra124-soctherm.h

diff --git a/Documentation/devicetree/bindings/thermal/tegra-soctherm.txt b/Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
new file mode 100644
index 0000000..ecf3ed7
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
@@ -0,0 +1,53 @@
+Tegra124 SOCTHERM thermal management system
+
+The SOCTHERM IP block contains thermal sensors, support for polled
+or interrupt-based thermal monitoring, CPU and GPU throttling based
+on temperature trip points, and handling external overcurrent
+notifications. It is also used to manage emergency shutdown in an
+overheating situation.
+
+Required properties :
+- compatible : "nvidia,tegra124-soctherm".
+- reg : Should contain 1 entry:
+  - SOCTHERM register set
+- interrupts : Defines the interrupt used by SOCTHERM
+- clocks : Must contain an entry for each entry in clock-names.
+  See ../clocks/clock-bindings.txt for details.
+- clock-names : Must include the following entries:
+  - tsensor
+  - soctherm
+- resets : Must contain an entry for each entry in reset-names.
+  See ../reset/reset.txt for details.
+- reset-names : Must include the following entries:
+  - soctherm
+- #thermal-sensor-cells : Should be 1. See ./thermal.txt for a description
+    of this property. See <dt-bindings/thermal/tegra124-soctherm.h> for a
+    list of valid values when referring to thermal sensors.
+
+
+Example :
+
+	soctherm@0,700e2000 {
+		compatible = "nvidia,tegra124-soctherm";
+		reg = <0x0 0x700e2000 0x0 0x1000>;
+		interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&tegra_car TEGRA124_CLK_TSENSOR>,
+			<&tegra_car TEGRA124_CLK_SOC_THERM>;
+		clock-names = "tsensor", "soctherm";
+		resets = <&tegra_car 78>;
+		reset-names = "soctherm";
+
+		#thermal-sensor-cells = <1>;
+	};
+
+Example: referring to thermal sensors :
+
+       thermal-zones {
+                cpu {
+                        polling-delay-passive = <1000>;
+                        polling-delay = <1000>;
+
+                        thermal-sensors =
+                                <&soctherm TEGRA124_SOCTHERM_SENSOR_CPU>;
+                };
+	};
diff --git a/include/dt-bindings/thermal/tegra124-soctherm.h b/include/dt-bindings/thermal/tegra124-soctherm.h
new file mode 100644
index 0000000..85aaf66
--- /dev/null
+++ b/include/dt-bindings/thermal/tegra124-soctherm.h
@@ -0,0 +1,13 @@
+/*
+ * This header provides constants for binding nvidia,tegra124-soctherm.
+ */
+
+#ifndef _DT_BINDINGS_THERMAL_TEGRA124_SOCTHERM_H
+#define _DT_BINDINGS_THERMAL_TEGRA124_SOCTHERM_H
+
+#define TEGRA124_SOCTHERM_SENSOR_CPU 0
+#define TEGRA124_SOCTHERM_SENSOR_MEM 1
+#define TEGRA124_SOCTHERM_SENSOR_GPU 2
+#define TEGRA124_SOCTHERM_SENSOR_PLLX 3
+
+#endif
-- 
2.1.0

[PATCH v6 2/4] ARM: tegra: Add soctherm and thermal zones to Tegra124 device tree

[Mikko Perttunen]

From: Mikko Perttunen <mperttunen@nvidia.com>

This adds the soctherm thermal sensing and management unit to the
Tegra124 device tree along with the four thermal zones corresponding
to the four thermal sensors provided by soctherm.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 arch/arm/boot/dts/tegra124.dtsi | 47 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 3cb1548..a7608a4 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -3,6 +3,7 @@
 #include <dt-bindings/pinctrl/pinctrl-tegra.h>
 #include <dt-bindings/pinctrl/pinctrl-tegra-xusb.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/thermal/tegra124-soctherm.h>
 
 #include "skeleton.dtsi"
 
@@ -610,6 +611,18 @@
 		status = "disabled";
 	};
 
+	soctherm: thermal-sensor@0,700e2000 {
+		compatible = "nvidia,tegra124-soctherm";
+		reg = <0x0 0x700e2000 0x0 0x1000>;
+		interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&tegra_car TEGRA124_CLK_TSENSOR>,
+			<&tegra_car TEGRA124_CLK_SOC_THERM>;
+		clock-names = "tsensor", "soctherm";
+		resets = <&tegra_car 78>;
+		reset-names = "soctherm";
+		#thermal-sensor-cells = <1>;
+	};
+
 	ahub@0,70300000 {
 		compatible = "nvidia,tegra124-ahub";
 		reg = <0x0 0x70300000 0x0 0x200>,
@@ -851,6 +864,40 @@
 		};
 	};
 
+	thermal-zones {
+		cpu {
+			polling-delay-passive = <1000>;
+			polling-delay = <1000>;
+
+			thermal-sensors =
+				<&soctherm TEGRA124_SOCTHERM_SENSOR_CPU>;
+		};
+
+		mem {
+			polling-delay-passive = <1000>;
+			polling-delay = <1000>;
+
+			thermal-sensors =
+				<&soctherm TEGRA124_SOCTHERM_SENSOR_MEM>;
+		};
+
+		gpu {
+			polling-delay-passive = <1000>;
+			polling-delay = <1000>;
+
+			thermal-sensors =
+				<&soctherm TEGRA124_SOCTHERM_SENSOR_GPU>;
+		};
+
+		pllx {
+			polling-delay-passive = <1000>;
+			polling-delay = <1000>;
+
+			thermal-sensors =
+				<&soctherm TEGRA124_SOCTHERM_SENSOR_PLLX>;
+		};
+	};
+
 	timer {
 		compatible = "arm,armv7-timer";
 		interrupts = <GIC_PPI 13
-- 
2.1.0

[PATCH v6 3/4] ARM: tegra: Add thermal trip points for Jetson TK1

[Mikko Perttunen]

From: Mikko Perttunen <mperttunen@nvidia.com>

This adds critical trip points to the Jetson TK1 device tree.
The device will do a controlled shutdown when either the CPU, GPU
or MEM thermal zone reaches 101 degrees Celsius.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
v6: added comments to cooling map nodes

 arch/arm/boot/dts/tegra124-jetson-tk1.dts | 44 +++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
index 0cdb5cf..66a5eaf 100644
--- a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
+++ b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
@@ -1876,4 +1876,48 @@
 			 <&tegra_car TEGRA124_CLK_EXTERN1>;
 		clock-names = "pll_a", "pll_a_out0", "mclk";
 	};
+
+	thermal-zones {
+		cpu {
+			trips {
+				trip@0 {
+					temperature = <101000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				/* There are currently no cooling maps because there are no cooling devices */
+			};
+		};
+
+		mem {
+			trips {
+				trip@0 {
+					temperature = <101000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				/* There are currently no cooling maps because there are no cooling devices */
+			};
+		};
+
+		gpu {
+			trips {
+				trip@0 {
+					temperature = <101000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				/* There are currently no cooling maps because there are no cooling devices */
+			};
+		};
+	};
 };
-- 
2.1.0

[PATCH v6 4/4] thermal: Add Tegra SOCTHERM thermal management driver

[Mikko Perttunen]

From: Mikko Perttunen <mperttunen@nvidia.com>

This adds support for the Tegra SOCTHERM thermal sensing and management
system found in the Tegra124 system-on-chip. This initial driver supports
temperature polling for four thermal zones.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
v6: fixed sparse warning for wrong order of "__iomem *"

 drivers/thermal/Kconfig          |  10 +
 drivers/thermal/Makefile         |   1 +
 drivers/thermal/tegra_soctherm.c | 471 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 482 insertions(+)
 create mode 100644 drivers/thermal/tegra_soctherm.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 693208e..fd9d049 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -175,6 +175,16 @@ config ARMADA_THERMAL
 	  Enable this option if you want to have support for thermal management
 	  controller present in Armada 370 and Armada XP SoC.
 
+config TEGRA_SOCTHERM
+	tristate "Tegra SOCTHERM thermal management"
+	depends on ARCH_TEGRA
+	help
+	  Enable this option for integrated thermal management support on NVIDIA
+	  Tegra124 systems-on-chip. The driver supports four thermal zones
+	  (CPU, GPU, MEM, PLLX). Cooling devices can be bound to the thermal
+	  zones to manage temperatures. This option is also required for the
+	  emergency thermal reset (thermtrip) feature to function.
+
 config DB8500_CPUFREQ_COOLING
 	tristate "DB8500 cpufreq cooling"
 	depends on ARCH_U8500
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 31e232f..f0b94d5 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -33,3 +33,4 @@ obj-$(CONFIG_INTEL_SOC_DTS_THERMAL)	+= intel_soc_dts_thermal.o
 obj-$(CONFIG_TI_SOC_THERMAL)	+= ti-soc-thermal/
 obj-$(CONFIG_ACPI_INT3403_THERMAL)	+= int3403_thermal.o
 obj-$(CONFIG_ST_THERMAL)	+= st/
+obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra_soctherm.o
diff --git a/drivers/thermal/tegra_soctherm.c b/drivers/thermal/tegra_soctherm.c
new file mode 100644
index 0000000..8a8771a
--- /dev/null
+++ b/drivers/thermal/tegra_soctherm.c
@@ -0,0 +1,471 @@
+/*
+ * drivers/thermal/tegra_soctherm.c
+ *
+ * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * Author:
+ *	Mikko Perttunen <mperttunen@nvidia.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/thermal.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/bitops.h>
+#include <soc/tegra/fuse.h>
+
+#define SENSOR_CONFIG0				0
+#define		SENSOR_CONFIG0_STOP		BIT(0)
+#define		SENSOR_CONFIG0_TALL_SHIFT	8
+#define		SENSOR_CONFIG0_TCALC_OVER	BIT(4)
+#define		SENSOR_CONFIG0_OVER		BIT(3)
+#define		SENSOR_CONFIG0_CPTR_OVER	BIT(2)
+#define SENSOR_CONFIG1				4
+#define		SENSOR_CONFIG1_TSAMPLE_SHIFT	0
+#define		SENSOR_CONFIG1_TIDDQ_EN_SHIFT	15
+#define		SENSOR_CONFIG1_TEN_COUNT_SHIFT	24
+#define		SENSOR_CONFIG1_TEMP_ENABLE	BIT(31)
+#define SENSOR_CONFIG2				8
+#define		SENSOR_CONFIG2_THERMA_SHIFT	16
+#define		SENSOR_CONFIG2_THERMB_SHIFT	0
+
+#define SENSOR_PDIV				0x1c0
+#define		SENSOR_PDIV_T124		0x8888
+#define SENSOR_HOTSPOT_OFF			0x1c4
+#define		SENSOR_HOTSPOT_OFF_T124		0x00060600
+#define SENSOR_TEMP1				0x1c8
+#define SENSOR_TEMP2				0x1cc
+
+#define SENSOR_TEMP_MASK			0xffff
+#define READBACK_VALUE_MASK			0xff00
+#define READBACK_VALUE_SHIFT			8
+#define READBACK_ADD_HALF			BIT(7)
+#define READBACK_NEGATE				BIT(1)
+
+#define FUSE_TSENSOR8_CALIB			0x180
+#define FUSE_SPARE_REALIGNMENT_REG_0		0x1fc
+
+#define FUSE_TSENSOR_CALIB_CP_TS_BASE_MASK	0x1fff
+#define FUSE_TSENSOR_CALIB_FT_TS_BASE_MASK	(0x1fff << 13)
+#define FUSE_TSENSOR_CALIB_FT_TS_BASE_SHIFT	13
+
+#define FUSE_TSENSOR8_CALIB_CP_TS_BASE_MASK	0x3ff
+#define FUSE_TSENSOR8_CALIB_FT_TS_BASE_MASK	(0x7ff << 10)
+#define FUSE_TSENSOR8_CALIB_FT_TS_BASE_SHIFT	10
+
+#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_CP_MASK 0x3f
+#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_MASK (0x1f << 21)
+#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_SHIFT 21
+
+#define NOMINAL_CALIB_FT_T124			105
+#define NOMINAL_CALIB_CP_T124			25
+
+struct tegra_tsensor_configuration {
+	u32 tall, tsample, tiddq_en, ten_count;
+	u32 pdiv, tsample_ate, pdiv_ate;
+};
+
+struct tegra_tsensor {
+	u32 base;
+	u32 calib_fuse_offset;
+	/* Correction values used to modify values read from calibration fuses */
+	s32 fuse_corr_alpha, fuse_corr_beta;
+};
+
+struct tegra_thermctl_zone {
+	void __iomem *temp_reg;
+	int temp_shift;
+};
+
+static const struct tegra_tsensor_configuration t124_tsensor_config = {
+	.tall = 16300,
+	.tsample = 120,
+	.tiddq_en = 1,
+	.ten_count = 1,
+	.pdiv = 8,
+	.tsample_ate = 480,
+	.pdiv_ate = 8
+};
+
+static const struct tegra_tsensor t124_tsensors[] = {
+	{
+		.base = 0xc0,
+		.calib_fuse_offset = 0x098,
+		.fuse_corr_alpha = 1135400,
+		.fuse_corr_beta = -6266900,
+	},
+	{
+		.base = 0xe0,
+		.calib_fuse_offset = 0x084,
+		.fuse_corr_alpha = 1122220,
+		.fuse_corr_beta = -5700700,
+	},
+	{
+		.base = 0x100,
+		.calib_fuse_offset = 0x088,
+		.fuse_corr_alpha = 1127000,
+		.fuse_corr_beta = -6768200,
+	},
+	{
+		.base = 0x120,
+		.calib_fuse_offset = 0x12c,
+		.fuse_corr_alpha = 1110900,
+		.fuse_corr_beta = -6232000,
+	},
+	{
+		.base = 0x140,
+		.calib_fuse_offset = 0x158,
+		.fuse_corr_alpha = 1122300,
+		.fuse_corr_beta = -5936400,
+	},
+	{
+		.base = 0x160,
+		.calib_fuse_offset = 0x15c,
+		.fuse_corr_alpha = 1145700,
+		.fuse_corr_beta = -7124600,
+	},
+	{
+		.base = 0x180,
+		.calib_fuse_offset = 0x154,
+		.fuse_corr_alpha = 1120100,
+		.fuse_corr_beta = -6000500,
+	},
+	{
+		.base = 0x1a0,
+		.calib_fuse_offset = 0x160,
+		.fuse_corr_alpha = 1106500,
+		.fuse_corr_beta = -6729300,
+	},
+};
+
+struct tegra_soctherm {
+	struct reset_control *reset;
+	struct clk *clock_tsensor;
+	struct clk *clock_soctherm;
+	void __iomem *regs;
+
+	struct thermal_zone_device *thermctl_tzs[4];
+};
+
+struct tsensor_shared_calibration {
+	u32 base_cp, base_ft;
+	u32 actual_temp_cp, actual_temp_ft;
+};
+
+static int calculate_shared_calibration(struct tsensor_shared_calibration *r)
+{
+	u32 val;
+	u32 shifted_cp, shifted_ft;
+	int err;
+
+	err = tegra_fuse_readl(FUSE_TSENSOR8_CALIB, &val);
+	if (err)
+		return err;
+	r->base_cp = val & FUSE_TSENSOR8_CALIB_CP_TS_BASE_MASK;
+	r->base_ft = (val & FUSE_TSENSOR8_CALIB_FT_TS_BASE_MASK)
+		>> FUSE_TSENSOR8_CALIB_FT_TS_BASE_SHIFT;
+	val = ((val & FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_MASK)
+		>> FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_SHIFT);
+	shifted_ft = sign_extend32(val, 4);
+
+	err = tegra_fuse_readl(FUSE_SPARE_REALIGNMENT_REG_0, &val);
+	if (err)
+		return err;
+	shifted_cp = sign_extend32(val, 5);
+
+	r->actual_temp_cp = 2 * NOMINAL_CALIB_CP_T124 + shifted_cp;
+	r->actual_temp_ft = 2 * NOMINAL_CALIB_FT_T124 + shifted_ft;
+
+	return 0;
+}
+
+static s64 div64_s64_precise(s64 a, s64 b)
+{
+	s64 r, al;
+
+	/* Scale up for increased precision division */
+	al = a << 16;
+
+	r = div64_s64(al * 2 + 1, 2 * b);
+	return r >> 16;
+}
+
+static int calculate_tsensor_calibration(
+	const struct tegra_tsensor *sensor,
+	struct tsensor_shared_calibration shared,
+	u32 *calib
+)
+{
+	u32 val;
+	s32 actual_tsensor_ft, actual_tsensor_cp;
+	s32 delta_sens, delta_temp;
+	s32 mult, div;
+	s16 therma, thermb;
+	int err;
+
+	err = tegra_fuse_readl(sensor->calib_fuse_offset, &val);
+	if (err)
+		return err;
+
+	actual_tsensor_cp = (shared.base_cp * 64) + sign_extend32(val, 12);
+	val = (val & FUSE_TSENSOR_CALIB_FT_TS_BASE_MASK)
+		>> FUSE_TSENSOR_CALIB_FT_TS_BASE_SHIFT;
+	actual_tsensor_ft = (shared.base_ft * 32) + sign_extend32(val, 12);
+
+	delta_sens = actual_tsensor_ft - actual_tsensor_cp;
+	delta_temp = shared.actual_temp_ft - shared.actual_temp_cp;
+
+	mult = t124_tsensor_config.pdiv * t124_tsensor_config.tsample_ate;
+	div = t124_tsensor_config.tsample * t124_tsensor_config.pdiv_ate;
+
+	therma = div64_s64_precise((s64) delta_temp * (1LL << 13) * mult,
+		(s64) delta_sens * div);
+	thermb = div64_s64_precise(
+		((s64) actual_tsensor_ft * shared.actual_temp_cp) -
+		((s64) actual_tsensor_cp * shared.actual_temp_ft),
+		(s64) delta_sens);
+
+	therma = div64_s64_precise((s64) therma * sensor->fuse_corr_alpha,
+		(s64) 1000000LL);
+	thermb = div64_s64_precise((s64) thermb * sensor->fuse_corr_alpha +
+		sensor->fuse_corr_beta,
+		(s64) 1000000LL);
+
+	*calib = ((u16)(therma) << SENSOR_CONFIG2_THERMA_SHIFT) |
+		((u16)thermb << SENSOR_CONFIG2_THERMB_SHIFT);
+
+	return 0;
+}
+
+static int enable_tsensor(struct tegra_soctherm *tegra,
+			  const struct tegra_tsensor *sensor,
+			  struct tsensor_shared_calibration shared)
+{
+	void __iomem *base = tegra->regs + sensor->base;
+	unsigned int val;
+	u32 calib;
+	int err;
+
+	err = calculate_tsensor_calibration(sensor, shared, &calib);
+	if (err)
+		return err;
+
+	val = 0;
+	val |= t124_tsensor_config.tall << SENSOR_CONFIG0_TALL_SHIFT;
+	writel(val, base + SENSOR_CONFIG0);
+
+	val = 0;
+	val |= (t124_tsensor_config.tsample - 1) <<
+		SENSOR_CONFIG1_TSAMPLE_SHIFT;
+	val |= t124_tsensor_config.tiddq_en << SENSOR_CONFIG1_TIDDQ_EN_SHIFT;
+	val |= t124_tsensor_config.ten_count << SENSOR_CONFIG1_TEN_COUNT_SHIFT;
+	val |= SENSOR_CONFIG1_TEMP_ENABLE;
+	writel(val, base + SENSOR_CONFIG1);
+
+	writel(calib, base + SENSOR_CONFIG2);
+
+	return 0;
+}
+
+/* Translate from soctherm readback format to millicelsius.
+ * The soctherm readback format in bits is as follows:
+ *   TTTTTTTT H______N
+ * where T's contain the temperature in Celsius,
+ * H denotes an addition of 0.5 Celsius and N denotes negation
+ * of the final value.
+ */
+static inline long translate_temp(u16 val)
+{
+	long t;
+
+	t = ((val & READBACK_VALUE_MASK) >> READBACK_VALUE_SHIFT) * 1000;
+	if (val & READBACK_ADD_HALF)
+		t += 500;
+	if (val & READBACK_NEGATE)
+		t *= -1;
+
+	return t;
+}
+
+static int tegra_thermctl_get_temp(void *data, long *out_temp)
+{
+	struct tegra_thermctl_zone *zone = data;
+	u32 val;
+
+	val = (readl(zone->temp_reg) >> zone->temp_shift) & SENSOR_TEMP_MASK;
+	*out_temp = translate_temp(val);
+
+	return 0;
+}
+
+static const struct of_device_id tegra_soctherm_of_match[] = {
+	{ .compatible = "nvidia,tegra124-soctherm" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tegra_soctherm_of_match);
+
+static const int thermctl_temp_offsets[] = {
+	SENSOR_TEMP1, SENSOR_TEMP2, SENSOR_TEMP1, SENSOR_TEMP2
+};
+
+static const int thermctl_temp_shifts[] = {
+	16, 16, 0, 0
+};
+
+static int tegra_soctherm_probe(struct platform_device *pdev)
+{
+	struct tegra_soctherm *tegra;
+	struct thermal_zone_device *tz;
+	struct tsensor_shared_calibration shared_calib;
+	int i;
+	int err = 0;
+
+	const struct tegra_tsensor *tsensors = t124_tsensors;
+
+	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
+	if (!tegra)
+		return -ENOMEM;
+
+	tegra->regs = devm_ioremap_resource(&pdev->dev,
+		platform_get_resource(pdev, IORESOURCE_MEM, 0));
+	if (IS_ERR(tegra->regs)) {
+		dev_err(&pdev->dev, "can't get registers");
+		return PTR_ERR(tegra->regs);
+	}
+
+	tegra->reset = devm_reset_control_get(&pdev->dev, "soctherm");
+	if (IS_ERR(tegra->reset)) {
+		dev_err(&pdev->dev, "can't get soctherm reset\n");
+		return PTR_ERR(tegra->reset);
+	}
+
+	tegra->clock_tsensor = devm_clk_get(&pdev->dev, "tsensor");
+	if (IS_ERR(tegra->clock_tsensor)) {
+		dev_err(&pdev->dev, "can't get clock tsensor\n");
+		return PTR_ERR(tegra->clock_tsensor);
+	}
+
+	tegra->clock_soctherm = devm_clk_get(&pdev->dev, "soctherm");
+	if (IS_ERR(tegra->clock_soctherm)) {
+		dev_err(&pdev->dev, "can't get clock soctherm\n");
+		return PTR_ERR(tegra->clock_soctherm);
+	}
+
+	reset_control_assert(tegra->reset);
+
+	err = clk_prepare_enable(tegra->clock_soctherm);
+	if (err) {
+		reset_control_deassert(tegra->reset);
+		return err;
+	}
+
+	err = clk_prepare_enable(tegra->clock_tsensor);
+	if (err) {
+		clk_disable_unprepare(tegra->clock_soctherm);
+		reset_control_deassert(tegra->reset);
+		return err;
+	}
+
+	reset_control_deassert(tegra->reset);
+
+	/* Initialize raw sensors */
+
+	err = calculate_shared_calibration(&shared_calib);
+	if (err)
+		goto disable_clocks;
+
+	for (i = 0; i < ARRAY_SIZE(t124_tsensors); ++i) {
+		err = enable_tsensor(tegra, tsensors + i, shared_calib);
+		if (err)
+			goto disable_clocks;
+	}
+
+	writel(SENSOR_PDIV_T124, tegra->regs + SENSOR_PDIV);
+	writel(SENSOR_HOTSPOT_OFF_T124, tegra->regs + SENSOR_HOTSPOT_OFF);
+
+	/* Initialize thermctl sensors */
+
+	for (i = 0; i < ARRAY_SIZE(tegra->thermctl_tzs); ++i) {
+		struct tegra_thermctl_zone *zone =
+			devm_kzalloc(&pdev->dev, sizeof(*zone), GFP_KERNEL);
+		if (!zone) {
+			err = -ENOMEM;
+			--i;
+			goto unregister_tzs;
+		}
+
+		zone->temp_reg = tegra->regs + thermctl_temp_offsets[i];
+		zone->temp_shift = thermctl_temp_shifts[i];
+
+		tz = thermal_zone_of_sensor_register(
+			&pdev->dev, i, zone, tegra_thermctl_get_temp, NULL);
+		if (IS_ERR(tz)) {
+			err = PTR_ERR(tz);
+			dev_err(&pdev->dev, "failed to register sensor: %d\n",
+				err);
+			--i;
+			goto unregister_tzs;
+		}
+
+		tegra->thermctl_tzs[i] = tz;
+	}
+
+	return 0;
+
+unregister_tzs:
+	for (; i >= 0; i--)
+		thermal_zone_of_sensor_unregister(&pdev->dev,
+						  tegra->thermctl_tzs[i]);
+
+disable_clocks:
+	clk_disable_unprepare(tegra->clock_tsensor);
+	clk_disable_unprepare(tegra->clock_soctherm);
+
+	return err;
+}
+
+static int tegra_soctherm_remove(struct platform_device *pdev)
+{
+	struct tegra_soctherm *tegra = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tegra->thermctl_tzs); ++i) {
+		thermal_zone_of_sensor_unregister(&pdev->dev,
+						  tegra->thermctl_tzs[i]);
+	}
+
+	clk_disable_unprepare(tegra->clock_tsensor);
+	clk_disable_unprepare(tegra->clock_soctherm);
+
+	return 0;
+}
+
+static struct platform_driver tegra_soctherm_driver = {
+	.probe = tegra_soctherm_probe,
+	.remove = tegra_soctherm_remove,
+	.driver = {
+		.name = "tegra_soctherm",
+		.of_match_table = tegra_soctherm_of_match,
+	},
+};
+module_platform_driver(tegra_soctherm_driver);
+
+MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
+MODULE_DESCRIPTION("Tegra SOCTHERM thermal management driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.0

Re: [PATCH v6 4/4] thermal: Add Tegra SOCTHERM thermal management driver

[Thierry Reding]

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

On Fri, Sep 26, 2014 at 12:43:13PM +0300, Mikko Perttunen wrote:
> From: Mikko Perttunen <mperttunen@nvidia.com>
> 
> This adds support for the Tegra SOCTHERM thermal sensing and management
> system found in the Tegra124 system-on-chip. This initial driver supports
> temperature polling for four thermal zones.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
> v6: fixed sparse warning for wrong order of "__iomem *"

Sorry for jumping in so late. This looks generally good, but I have a
couple of comments of a stylistic nature. I haven't closely followed
earlier rounds of the series, so please let me know if I'm bringing up
issues that have already been discussed.

> diff --git a/drivers/thermal/tegra_soctherm.c b/drivers/thermal/tegra_soctherm.c
[...]
> @@ -0,0 +1,471 @@
> +/*
> + * drivers/thermal/tegra_soctherm.c

This line is not really necessary and likely to become stale if files
are moved around.

> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/thermal.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/bitops.h>
> +#include <soc/tegra/fuse.h>

These should be sorted alphabetically and I prefer separating them into
sections, so that the last of the linux/* includes is separated from the
soc/* includes by a blank line.

> +
> +#define SENSOR_CONFIG0				0
> +#define		SENSOR_CONFIG0_STOP		BIT(0)
> +#define		SENSOR_CONFIG0_TALL_SHIFT	8
> +#define		SENSOR_CONFIG0_TCALC_OVER	BIT(4)
> +#define		SENSOR_CONFIG0_OVER		BIT(3)
> +#define		SENSOR_CONFIG0_CPTR_OVER	BIT(2)
> +#define SENSOR_CONFIG1				4
> +#define		SENSOR_CONFIG1_TSAMPLE_SHIFT	0
> +#define		SENSOR_CONFIG1_TIDDQ_EN_SHIFT	15
> +#define		SENSOR_CONFIG1_TEN_COUNT_SHIFT	24
> +#define		SENSOR_CONFIG1_TEMP_ENABLE	BIT(31)
> +#define SENSOR_CONFIG2				8
> +#define		SENSOR_CONFIG2_THERMA_SHIFT	16
> +#define		SENSOR_CONFIG2_THERMB_SHIFT	0
> +
> +#define SENSOR_PDIV				0x1c0
> +#define		SENSOR_PDIV_T124		0x8888
> +#define SENSOR_HOTSPOT_OFF			0x1c4
> +#define		SENSOR_HOTSPOT_OFF_T124		0x00060600
> +#define SENSOR_TEMP1				0x1c8
> +#define SENSOR_TEMP2				0x1cc
> +
> +#define SENSOR_TEMP_MASK			0xffff
> +#define READBACK_VALUE_MASK			0xff00
> +#define READBACK_VALUE_SHIFT			8
> +#define READBACK_ADD_HALF			BIT(7)
> +#define READBACK_NEGATE				BIT(1)

These don't seem to be aligned in the same way as the field definitions
for the other registers. Was that on purpose? I also think using two
tabs to indent field definitions is somewhat excessive. Another common
pattern I've seen used is:

	#define REGISTER_1_NAME 0x000
	#define  REGISTER_1_FIELD_MASK (0xf << 4)

	#define REGISTER_2_NAME 0x004
	#define  REGISTER_2_FIELD_MASK (0xf << 8)

> +#define FUSE_TSENSOR8_CALIB			0x180
> +#define FUSE_SPARE_REALIGNMENT_REG_0		0x1fc
> +
> +#define FUSE_TSENSOR_CALIB_CP_TS_BASE_MASK	0x1fff
> +#define FUSE_TSENSOR_CALIB_FT_TS_BASE_MASK	(0x1fff << 13)
> +#define FUSE_TSENSOR_CALIB_FT_TS_BASE_SHIFT	13
> +
> +#define FUSE_TSENSOR8_CALIB_CP_TS_BASE_MASK	0x3ff
> +#define FUSE_TSENSOR8_CALIB_FT_TS_BASE_MASK	(0x7ff << 10)
> +#define FUSE_TSENSOR8_CALIB_FT_TS_BASE_SHIFT	10
> +
> +#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_CP_MASK 0x3f
> +#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_MASK (0x1f << 21)
> +#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_SHIFT 21

These are also inconsistent with the above register definitions.

> +
> +#define NOMINAL_CALIB_FT_T124			105
> +#define NOMINAL_CALIB_CP_T124			25

What do FT and CP stand for? Given the _T124 suffix they would seem
likely to change on future SoC generations. Perhaps they should be moved
into a struct tegra_soctherm_soc or similar? The array of t124_tsensors
would be another candidate to put into such a structure.

That's something that could be deferred until support is added for a
next generation.

> +struct tegra_tsensor_configuration {
> +	u32 tall, tsample, tiddq_en, ten_count;
> +	u32 pdiv, tsample_ate, pdiv_ate;

What's the significance of the "ate" suffix?

> +};
> +
> +struct tegra_tsensor {
> +	u32 base;
> +	u32 calib_fuse_offset;
> +	/* Correction values used to modify values read from calibration fuses */
> +	s32 fuse_corr_alpha, fuse_corr_beta;
> +};

Both this structure and the one above use inconsistent grouping of
fields (or at least I can't make out any grouping). Perhaps it would be
more consistent to use one line per field, or group all fields of a data
type in a single line?

> +struct tegra_thermctl_zone {
> +	void __iomem *temp_reg;
> +	int temp_shift;

Should this be unsigned? Also this is a thermal zone, so the temp_
prefix doesn't add any context.

> +static int calculate_shared_calibration(struct tsensor_shared_calibration *r)
> +{
> +	u32 val;
> +	u32 shifted_cp, shifted_ft;

The above two lines could be collapsed into one.

> +static int calculate_tsensor_calibration(
> +	const struct tegra_tsensor *sensor,
> +	struct tsensor_shared_calibration shared,
> +	u32 *calib
> +)

I think a more idiomatic way to write this would be:

static int
calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
			      struct tsensor_shared_calibration shared,
			      u32 *calib)

While at it, perhaps make shared a const * instead of passing it in by
value?

> +{
> +	u32 val;
> +	s32 actual_tsensor_ft, actual_tsensor_cp;
> +	s32 delta_sens, delta_temp;
> +	s32 mult, div;
> +	s16 therma, thermb;
> +	int err;
> +
> +	err = tegra_fuse_readl(sensor->calib_fuse_offset, &val);
> +	if (err)
> +		return err;
> +
> +	actual_tsensor_cp = (shared.base_cp * 64) + sign_extend32(val, 12);
> +	val = (val & FUSE_TSENSOR_CALIB_FT_TS_BASE_MASK)
> +		>> FUSE_TSENSOR_CALIB_FT_TS_BASE_SHIFT;

I personally prefer the operator at the end of the previous line, but I
guess that's mostly a matter of taste, so I'll leave it up to Eduardo.

> +	actual_tsensor_ft = (shared.base_ft * 32) + sign_extend32(val, 12);
> +
> +	delta_sens = actual_tsensor_ft - actual_tsensor_cp;
> +	delta_temp = shared.actual_temp_ft - shared.actual_temp_cp;
> +
> +	mult = t124_tsensor_config.pdiv * t124_tsensor_config.tsample_ate;
> +	div = t124_tsensor_config.tsample * t124_tsensor_config.pdiv_ate;

t124_tsensor_config is a pretty long name and repeated fairly often.
It's also one of those things that will likely change in future
generations (judging by the t124_ prefix), so perhaps it should be
stored as a const * somewhere. Perhaps in struct tegra_tsensor, so the
above becomes:

	mult = sensor->config->pdiv * sensor->config->tsample_ate;

> +
> +	therma = div64_s64_precise((s64) delta_temp * (1LL << 13) * mult,
> +		(s64) delta_sens * div);

There should be no space between the (s64) and the variable in the above
(and below, well, everywhere really). Also I find this difficult to read
because the second line is strangely indented. Can you align it with the
function arguments on the first line, please?

> +	thermb = div64_s64_precise(
> +		((s64) actual_tsensor_ft * shared.actual_temp_cp) -
> +		((s64) actual_tsensor_cp * shared.actual_temp_ft),
> +		(s64) delta_sens);

The way you need to wrap this indicates that you should either make
variable names shorter or split this computation up into several steps.

> +
> +	therma = div64_s64_precise((s64) therma * sensor->fuse_corr_alpha,
> +		(s64) 1000000LL);
> +	thermb = div64_s64_precise((s64) thermb * sensor->fuse_corr_alpha +
> +		sensor->fuse_corr_beta,
> +		(s64) 1000000LL);
> +
> +	*calib = ((u16)(therma) << SENSOR_CONFIG2_THERMA_SHIFT) |

Why the parentheses around "therma"?

> +		((u16)thermb << SENSOR_CONFIG2_THERMB_SHIFT);
> +
> +	return 0;
> +}
> +
> +static int enable_tsensor(struct tegra_soctherm *tegra,
> +			  const struct tegra_tsensor *sensor,
> +			  struct tsensor_shared_calibration shared)

Again, shouldn't you pass in "shared" by reference?

> +{
> +	void __iomem *base = tegra->regs + sensor->base;
> +	unsigned int val;
> +	u32 calib;
> +	int err;
> +
> +	err = calculate_tsensor_calibration(sensor, shared, &calib);
> +	if (err)
> +		return err;
> +
> +	val = 0;

There's no need for this, if you...

> +	val |= t124_tsensor_config.tall << SENSOR_CONFIG0_TALL_SHIFT;

... make this "val = ...;"

> +	writel(val, base + SENSOR_CONFIG0);
> +
> +	val = 0;

Same here.

> +	val |= (t124_tsensor_config.tsample - 1) <<
> +		SENSOR_CONFIG1_TSAMPLE_SHIFT;

Now the << operator is at the end of the line, so I think whichever way
you decide it should at least be consistent.

> +/* Translate from soctherm readback format to millicelsius.

Block comments should have the first line empty, like so:

	/*
	 * bla...
	 */

> + * The soctherm readback format in bits is as follows:
> + *   TTTTTTTT H______N
> + * where T's contain the temperature in Celsius,
> + * H denotes an addition of 0.5 Celsius and N denotes negation
> + * of the final value.
> + */
> +static inline long translate_temp(u16 val)

I think it can be left to the compiler to decide whether or not to
inline this particular function.

> +static const int thermctl_temp_offsets[] = {
> +	SENSOR_TEMP1, SENSOR_TEMP2, SENSOR_TEMP1, SENSOR_TEMP2
> +};
> +
> +static const int thermctl_temp_shifts[] = {
> +	16, 16, 0, 0
> +};

Perhaps these should be in a single array that describes the individual
zones. Something like:

	struct thermctl_zone_desc {
		unsigned int offset;
		unsigned int shift;
	};

	static const struct thermctl_zone_desc zones[] = {
		{ SENSOR_TEMP1, 16 },
		...
	};

And perhaps this array should have a t124_ prefix since it could vary
per SoC generation?

> +static int tegra_soctherm_probe(struct platform_device *pdev)
> +{
> +	struct tegra_soctherm *tegra;
> +	struct thermal_zone_device *tz;
> +	struct tsensor_shared_calibration shared_calib;
> +	int i;

Should be unsigned.

> +	int err = 0;

I don't think this needs to be initialized.

> +
> +	const struct tegra_tsensor *tsensors = t124_tsensors;
> +
> +	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> +	if (!tegra)
> +		return -ENOMEM;
> +
> +	tegra->regs = devm_ioremap_resource(&pdev->dev,
> +		platform_get_resource(pdev, IORESOURCE_MEM, 0));

Can this please be two separate statements for better readability?

> +	if (IS_ERR(tegra->regs)) {
> +		dev_err(&pdev->dev, "can't get registers");

No need for the message. devm_ioremap_resource() prints one for you on
error.

> +		return PTR_ERR(tegra->regs);
> +	}
> +
> +	tegra->reset = devm_reset_control_get(&pdev->dev, "soctherm");
> +	if (IS_ERR(tegra->reset)) {
> +		dev_err(&pdev->dev, "can't get soctherm reset\n");
> +		return PTR_ERR(tegra->reset);
> +	}
> +
> +	tegra->clock_tsensor = devm_clk_get(&pdev->dev, "tsensor");
> +	if (IS_ERR(tegra->clock_tsensor)) {
> +		dev_err(&pdev->dev, "can't get clock tsensor\n");

Nit: the wording of this is inconsistent with the reset error.

> +		return PTR_ERR(tegra->clock_tsensor);
> +	}
> +
> +	tegra->clock_soctherm = devm_clk_get(&pdev->dev, "soctherm");
> +	if (IS_ERR(tegra->clock_soctherm)) {
> +		dev_err(&pdev->dev, "can't get clock soctherm\n");

Same here.

> +		return PTR_ERR(tegra->clock_soctherm);
> +	}
> +
> +	reset_control_assert(tegra->reset);
> +
> +	err = clk_prepare_enable(tegra->clock_soctherm);
> +	if (err) {
> +		reset_control_deassert(tegra->reset);

Is it really useful to deassert in case of failure? After the assertion
of the reset above, all hardware state will be gone and since the device
won't be used, nobody will reinitialize it properly. Taking it out of
reset is useless.

> +	/* Initialize raw sensors */
> +
> +	err = calculate_shared_calibration(&shared_calib);
> +	if (err)
> +		goto disable_clocks;
> +
> +	for (i = 0; i < ARRAY_SIZE(t124_tsensors); ++i) {
> +		err = enable_tsensor(tegra, tsensors + i, shared_calib);
> +		if (err)
> +			goto disable_clocks;
> +	}
> +
> +	writel(SENSOR_PDIV_T124, tegra->regs + SENSOR_PDIV);
> +	writel(SENSOR_HOTSPOT_OFF_T124, tegra->regs + SENSOR_HOTSPOT_OFF);
> +
> +	/* Initialize thermctl sensors */
> +
> +	for (i = 0; i < ARRAY_SIZE(tegra->thermctl_tzs); ++i) {
> +		struct tegra_thermctl_zone *zone =
> +			devm_kzalloc(&pdev->dev, sizeof(*zone), GFP_KERNEL);
> +		if (!zone) {
> +			err = -ENOMEM;
> +			--i;

What's up with the i line here? Doesn't that trigger a compiler warning?

Oh wait, that's whitespace highlighting screwing with the --i. One more
reason to prefer i-- where passible. Also I think the more idiomatic way
for this kind of cleanup is to not touch i here, but to do this with a

	while (i--)

later on. See below.

> +			goto unregister_tzs;
> +		}
> +
> +		zone->temp_reg = tegra->regs + thermctl_temp_offsets[i];
> +		zone->temp_shift = thermctl_temp_shifts[i];
> +
> +		tz = thermal_zone_of_sensor_register(
> +			&pdev->dev, i, zone, tegra_thermctl_get_temp, NULL);

This is weirdly wrapped. Better would be:

> +		tz = thermal_zone_of_sensor_register(&pdev->dev, i, zone,
> +						     tegra_thermctl_get_temp,
> +						     NULL);

> +unregister_tzs:
> +	for (; i >= 0; i--)
> +		thermal_zone_of_sensor_unregister(&pdev->dev,
> +						  tegra->thermctl_tzs[i]);

Like I said above, I think the more idiomatic way would be:

	while (i--)
		thermal_zone_of_sensor_unregister(...);

> +static int tegra_soctherm_remove(struct platform_device *pdev)
> +{
> +	struct tegra_soctherm *tegra = platform_get_drvdata(pdev);
> +	int i;

unsigned again.

> +
> +	for (i = 0; i < ARRAY_SIZE(tegra->thermctl_tzs); ++i) {
> +		thermal_zone_of_sensor_unregister(&pdev->dev,
> +						  tegra->thermctl_tzs[i]);
> +	}
> +
> +	clk_disable_unprepare(tegra->clock_tsensor);
> +	clk_disable_unprepare(tegra->clock_soctherm);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver tegra_soctherm_driver = {
> +	.probe = tegra_soctherm_probe,
> +	.remove = tegra_soctherm_remove,
> +	.driver = {
> +		.name = "tegra_soctherm",

I think we're primarily using tegra- as prefix for driver names.

> +		.of_match_table = tegra_soctherm_of_match,
> +	},
> +};
> +module_platform_driver(tegra_soctherm_driver);
> +
> +MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
> +MODULE_DESCRIPTION("Tegra SOCTHERM thermal management driver");

Perhaps: "NVIDIA Tegra ..."?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 4/4] thermal: Add Tegra SOCTHERM thermal management driver

[Mikko Perttunen]

On 09/26/2014 02:45 PM, Thierry Reding wrote:
> On Fri, Sep 26, 2014 at 12:43:13PM +0300, Mikko Perttunen wrote:
>> From: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> This adds support for the Tegra SOCTHERM thermal sensing and management
>> system found in the Tegra124 system-on-chip. This initial driver supports
>> temperature polling for four thermal zones.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>> v6: fixed sparse warning for wrong order of "__iomem *"
>
> Sorry for jumping in so late. This looks generally good, but I have a
> couple of comments of a stylistic nature. I haven't closely followed
> earlier rounds of the series, so please let me know if I'm bringing up
> issues that have already been discussed.
>
>> diff --git a/drivers/thermal/tegra_soctherm.c b/drivers/thermal/tegra_soctherm.c
> [...]
>> @@ -0,0 +1,471 @@
>> +/*
>> + * drivers/thermal/tegra_soctherm.c
>
> This line is not really necessary and likely to become stale if files
> are moved around.

Removed.

>
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset.h>
>> +#include <linux/thermal.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/bitops.h>
>> +#include <soc/tegra/fuse.h>
>
> These should be sorted alphabetically and I prefer separating them into
> sections, so that the last of the linux/* includes is separated from the
> soc/* includes by a blank line.

Fixed.

>
>> +
>> +#define SENSOR_CONFIG0				0
>> +#define		SENSOR_CONFIG0_STOP		BIT(0)
>> +#define		SENSOR_CONFIG0_TALL_SHIFT	8
>> +#define		SENSOR_CONFIG0_TCALC_OVER	BIT(4)
>> +#define		SENSOR_CONFIG0_OVER		BIT(3)
>> +#define		SENSOR_CONFIG0_CPTR_OVER	BIT(2)
>> +#define SENSOR_CONFIG1				4
>> +#define		SENSOR_CONFIG1_TSAMPLE_SHIFT	0
>> +#define		SENSOR_CONFIG1_TIDDQ_EN_SHIFT	15
>> +#define		SENSOR_CONFIG1_TEN_COUNT_SHIFT	24
>> +#define		SENSOR_CONFIG1_TEMP_ENABLE	BIT(31)
>> +#define SENSOR_CONFIG2				8
>> +#define		SENSOR_CONFIG2_THERMA_SHIFT	16
>> +#define		SENSOR_CONFIG2_THERMB_SHIFT	0
>> +
>> +#define SENSOR_PDIV				0x1c0
>> +#define		SENSOR_PDIV_T124		0x8888
>> +#define SENSOR_HOTSPOT_OFF			0x1c4
>> +#define		SENSOR_HOTSPOT_OFF_T124		0x00060600
>> +#define SENSOR_TEMP1				0x1c8
>> +#define SENSOR_TEMP2				0x1cc
>> +
>> +#define SENSOR_TEMP_MASK			0xffff
>> +#define READBACK_VALUE_MASK			0xff00
>> +#define READBACK_VALUE_SHIFT			8
>> +#define READBACK_ADD_HALF			BIT(7)
>> +#define READBACK_NEGATE				BIT(1)
>
> These don't seem to be aligned in the same way as the field definitions
> for the other registers. Was that on purpose? I also think using two
> tabs to indent field definitions is somewhat excessive. Another common
> pattern I've seen used is:
>
> 	#define REGISTER_1_NAME 0x000
> 	#define  REGISTER_1_FIELD_MASK (0xf << 4)
>
> 	#define REGISTER_2_NAME 0x004
> 	#define  REGISTER_2_FIELD_MASK (0xf << 8)

I agree, the indentation was a bit excessive. The readback defines 
weren't indented since they weren't really nested under anything. I 
removed the indentation and added some spacing; I think it looks a bit 
better now.

>
>> +#define FUSE_TSENSOR8_CALIB			0x180
>> +#define FUSE_SPARE_REALIGNMENT_REG_0		0x1fc
>> +
>> +#define FUSE_TSENSOR_CALIB_CP_TS_BASE_MASK	0x1fff
>> +#define FUSE_TSENSOR_CALIB_FT_TS_BASE_MASK	(0x1fff << 13)
>> +#define FUSE_TSENSOR_CALIB_FT_TS_BASE_SHIFT	13
>> +
>> +#define FUSE_TSENSOR8_CALIB_CP_TS_BASE_MASK	0x3ff
>> +#define FUSE_TSENSOR8_CALIB_FT_TS_BASE_MASK	(0x7ff << 10)
>> +#define FUSE_TSENSOR8_CALIB_FT_TS_BASE_SHIFT	10
>> +
>> +#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_CP_MASK 0x3f
>> +#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_MASK (0x1f << 21)
>> +#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_SHIFT 21
>
> These are also inconsistent with the above register definitions.

Fixed.

>
>> +
>> +#define NOMINAL_CALIB_FT_T124			105
>> +#define NOMINAL_CALIB_CP_T124			25
>
> What do FT and CP stand for? Given the _T124 suffix they would seem
> likely to change on future SoC generations. Perhaps they should be moved
> into a struct tegra_soctherm_soc or similar? The array of t124_tsensors
> would be another candidate to put into such a structure.

No idea. I haven't found any expansions for these in internal or 
external documentation/code. These are the names used by the downstream 
driver and some documentation, so I used them.

>
> That's something that could be deferred until support is added for a
> next generation.

I would prefer to do that.

>
>> +struct tegra_tsensor_configuration {
>> +	u32 tall, tsample, tiddq_en, ten_count;
>> +	u32 pdiv, tsample_ate, pdiv_ate;
>
> What's the significance of the "ate" suffix?

Again, no idea.

>
>> +};
>> +
>> +struct tegra_tsensor {
>> +	u32 base;
>> +	u32 calib_fuse_offset;
>> +	/* Correction values used to modify values read from calibration fuses */
>> +	s32 fuse_corr_alpha, fuse_corr_beta;
>> +};
>
> Both this structure and the one above use inconsistent grouping of
> fields (or at least I can't make out any grouping). Perhaps it would be
> more consistent to use one line per field, or group all fields of a data
> type in a single line?

Put fields of same type on same line.

>
>> +struct tegra_thermctl_zone {
>> +	void __iomem *temp_reg;
>> +	int temp_shift;
>
> Should this be unsigned? Also this is a thermal zone, so the temp_
> prefix doesn't add any context.

Fixed.

>
>> +static int calculate_shared_calibration(struct tsensor_shared_calibration *r)
>> +{
>> +	u32 val;
>> +	u32 shifted_cp, shifted_ft;
>
> The above two lines could be collapsed into one.

Fixed.

>
>> +static int calculate_tsensor_calibration(
>> +	const struct tegra_tsensor *sensor,
>> +	struct tsensor_shared_calibration shared,
>> +	u32 *calib
>> +)
>
> I think a more idiomatic way to write this would be:
>
> static int
> calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
> 			      struct tsensor_shared_calibration shared,
> 			      u32 *calib)

If I do that, it will go over the 80 character limit by quite a few 
characters, which is why I didn't use that style. Personally I'm fine 
with either style.

>
> While at it, perhaps make shared a const * instead of passing it in by
> value?

That is possible, but I'm not sure what the difference would be. Is 
there a style rule forbidding by-value compound types? (Also if I change 
the style, it would go over 80 characters by even more.)

>
>> +{
>> +	u32 val;
>> +	s32 actual_tsensor_ft, actual_tsensor_cp;
>> +	s32 delta_sens, delta_temp;
>> +	s32 mult, div;
>> +	s16 therma, thermb;
>> +	int err;
>> +
>> +	err = tegra_fuse_readl(sensor->calib_fuse_offset, &val);
>> +	if (err)
>> +		return err;
>> +
>> +	actual_tsensor_cp = (shared.base_cp * 64) + sign_extend32(val, 12);
>> +	val = (val & FUSE_TSENSOR_CALIB_FT_TS_BASE_MASK)
>> +		>> FUSE_TSENSOR_CALIB_FT_TS_BASE_SHIFT;
>
> I personally prefer the operator at the end of the previous line, but I
> guess that's mostly a matter of taste, so I'll leave it up to Eduardo.
>
>> +	actual_tsensor_ft = (shared.base_ft * 32) + sign_extend32(val, 12);
>> +
>> +	delta_sens = actual_tsensor_ft - actual_tsensor_cp;
>> +	delta_temp = shared.actual_temp_ft - shared.actual_temp_cp;
>> +
>> +	mult = t124_tsensor_config.pdiv * t124_tsensor_config.tsample_ate;
>> +	div = t124_tsensor_config.tsample * t124_tsensor_config.pdiv_ate;
>
> t124_tsensor_config is a pretty long name and repeated fairly often.
> It's also one of those things that will likely change in future
> generations (judging by the t124_ prefix), so perhaps it should be
> stored as a const * somewhere. Perhaps in struct tegra_tsensor, so the
> above becomes:

Added it as a .config field to tegra_tsensor.

>
> 	mult = sensor->config->pdiv * sensor->config->tsample_ate;
>
>> +
>> +	therma = div64_s64_precise((s64) delta_temp * (1LL << 13) * mult,
>> +		(s64) delta_sens * div);
>
> There should be no space between the (s64) and the variable in the above
> (and below, well, everywhere really). Also I find this difficult to read
> because the second line is strangely indented. Can you align it with the
> function arguments on the first line, please?

Fixed.

>
>> +	thermb = div64_s64_precise(
>> +		((s64) actual_tsensor_ft * shared.actual_temp_cp) -
>> +		((s64) actual_tsensor_cp * shared.actual_temp_ft),
>> +		(s64) delta_sens);
>
> The way you need to wrap this indicates that you should either make
> variable names shorter or split this computation up into several steps.

Split up.

>
>> +
>> +	therma = div64_s64_precise((s64) therma * sensor->fuse_corr_alpha,
>> +		(s64) 1000000LL);
>> +	thermb = div64_s64_precise((s64) thermb * sensor->fuse_corr_alpha +
>> +		sensor->fuse_corr_beta,
>> +		(s64) 1000000LL);
>> +
>> +	*calib = ((u16)(therma) << SENSOR_CONFIG2_THERMA_SHIFT) |
>
> Why the parentheses around "therma"?

Not sure. Removed.

>
>> +		((u16)thermb << SENSOR_CONFIG2_THERMB_SHIFT);
>> +
>> +	return 0;
>> +}
>> +
>> +static int enable_tsensor(struct tegra_soctherm *tegra,
>> +			  const struct tegra_tsensor *sensor,
>> +			  struct tsensor_shared_calibration shared)
>
> Again, shouldn't you pass in "shared" by reference?
>
>> +{
>> +	void __iomem *base = tegra->regs + sensor->base;
>> +	unsigned int val;
>> +	u32 calib;
>> +	int err;
>> +
>> +	err = calculate_tsensor_calibration(sensor, shared, &calib);
>> +	if (err)
>> +		return err;
>> +
>> +	val = 0;
>
> There's no need for this, if you...
>
>> +	val |= t124_tsensor_config.tall << SENSOR_CONFIG0_TALL_SHIFT;
>
> ... make this "val = ...;"
>
>> +	writel(val, base + SENSOR_CONFIG0);
>> +
>> +	val = 0;
>
> Same here.

Fixed.

>
>> +	val |= (t124_tsensor_config.tsample - 1) <<
>> +		SENSOR_CONFIG1_TSAMPLE_SHIFT;
>
> Now the << operator is at the end of the line, so I think whichever way
> you decide it should at least be consistent.

This now fits on one line.

>
>> +/* Translate from soctherm readback format to millicelsius.
>
> Block comments should have the first line empty, like so:
>
> 	/*
> 	 * bla...
> 	 */
>
>> + * The soctherm readback format in bits is as follows:
>> + *   TTTTTTTT H______N
>> + * where T's contain the temperature in Celsius,
>> + * H denotes an addition of 0.5 Celsius and N denotes negation
>> + * of the final value.
>> + */
>> +static inline long translate_temp(u16 val)
>
> I think it can be left to the compiler to decide whether or not to
> inline this particular function.

Removed 'inline'.

>
>> +static const int thermctl_temp_offsets[] = {
>> +	SENSOR_TEMP1, SENSOR_TEMP2, SENSOR_TEMP1, SENSOR_TEMP2
>> +};
>> +
>> +static const int thermctl_temp_shifts[] = {
>> +	16, 16, 0, 0
>> +};
>
> Perhaps these should be in a single array that describes the individual
> zones. Something like:
>
> 	struct thermctl_zone_desc {
> 		unsigned int offset;
> 		unsigned int shift;
> 	};
>
> 	static const struct thermctl_zone_desc zones[] = {
> 		{ SENSOR_TEMP1, 16 },
> 		...
> 	};
>
> And perhaps this array should have a t124_ prefix since it could vary
> per SoC generation?

Changed.

>
>> +static int tegra_soctherm_probe(struct platform_device *pdev)
>> +{
>> +	struct tegra_soctherm *tegra;
>> +	struct thermal_zone_device *tz;
>> +	struct tsensor_shared_calibration shared_calib;
>> +	int i;
>
> Should be unsigned.

Fixed.

>
>> +	int err = 0;
>
> I don't think this needs to be initialized.

Removed initialization.

>
>> +
>> +	const struct tegra_tsensor *tsensors = t124_tsensors;
>> +
>> +	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
>> +	if (!tegra)
>> +		return -ENOMEM;
>> +
>> +	tegra->regs = devm_ioremap_resource(&pdev->dev,
>> +		platform_get_resource(pdev, IORESOURCE_MEM, 0));
>
> Can this please be two separate statements for better readability?

Split up.

>
>> +	if (IS_ERR(tegra->regs)) {
>> +		dev_err(&pdev->dev, "can't get registers");
>
> No need for the message. devm_ioremap_resource() prints one for you on
> error.

Removed message.

>
>> +		return PTR_ERR(tegra->regs);
>> +	}
>> +
>> +	tegra->reset = devm_reset_control_get(&pdev->dev, "soctherm");
>> +	if (IS_ERR(tegra->reset)) {
>> +		dev_err(&pdev->dev, "can't get soctherm reset\n");
>> +		return PTR_ERR(tegra->reset);
>> +	}
>> +
>> +	tegra->clock_tsensor = devm_clk_get(&pdev->dev, "tsensor");
>> +	if (IS_ERR(tegra->clock_tsensor)) {
>> +		dev_err(&pdev->dev, "can't get clock tsensor\n");
>
> Nit: the wording of this is inconsistent with the reset error.

Fixed.

>
>> +		return PTR_ERR(tegra->clock_tsensor);
>> +	}
>> +
>> +	tegra->clock_soctherm = devm_clk_get(&pdev->dev, "soctherm");
>> +	if (IS_ERR(tegra->clock_soctherm)) {
>> +		dev_err(&pdev->dev, "can't get clock soctherm\n");
>
> Same here.

Fixed.

>
>> +		return PTR_ERR(tegra->clock_soctherm);
>> +	}
>> +
>> +	reset_control_assert(tegra->reset);
>> +
>> +	err = clk_prepare_enable(tegra->clock_soctherm);
>> +	if (err) {
>> +		reset_control_deassert(tegra->reset);
>
> Is it really useful to deassert in case of failure? After the assertion
> of the reset above, all hardware state will be gone and since the device
> won't be used, nobody will reinitialize it properly. Taking it out of
> reset is useless.

Removed deasserts.

>
>> +	/* Initialize raw sensors */
>> +
>> +	err = calculate_shared_calibration(&shared_calib);
>> +	if (err)
>> +		goto disable_clocks;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(t124_tsensors); ++i) {
>> +		err = enable_tsensor(tegra, tsensors + i, shared_calib);
>> +		if (err)
>> +			goto disable_clocks;
>> +	}
>> +
>> +	writel(SENSOR_PDIV_T124, tegra->regs + SENSOR_PDIV);
>> +	writel(SENSOR_HOTSPOT_OFF_T124, tegra->regs + SENSOR_HOTSPOT_OFF);
>> +
>> +	/* Initialize thermctl sensors */
>> +
>> +	for (i = 0; i < ARRAY_SIZE(tegra->thermctl_tzs); ++i) {
>> +		struct tegra_thermctl_zone *zone =
>> +			devm_kzalloc(&pdev->dev, sizeof(*zone), GFP_KERNEL);
>> +		if (!zone) {
>> +			err = -ENOMEM;
>> +			--i;
>
> What's up with the i line here? Doesn't that trigger a compiler warning?
>
> Oh wait, that's whitespace highlighting screwing with the --i. One more
> reason to prefer i-- where passible. Also I think the more idiomatic way
> for this kind of cleanup is to not touch i here, but to do this with a
>
> 	while (i--)
>
> later on. See below.

Fixed.

>
>> +			goto unregister_tzs;
>> +		}
>> +
>> +		zone->temp_reg = tegra->regs + thermctl_temp_offsets[i];
>> +		zone->temp_shift = thermctl_temp_shifts[i];
>> +
>> +		tz = thermal_zone_of_sensor_register(
>> +			&pdev->dev, i, zone, tegra_thermctl_get_temp, NULL);
>
> This is weirdly wrapped. Better would be:
>
>> +		tz = thermal_zone_of_sensor_register(&pdev->dev, i, zone,
>> +						     tegra_thermctl_get_temp,
>> +						     NULL);
>

Fixed.

>> +unregister_tzs:
>> +	for (; i >= 0; i--)
>> +		thermal_zone_of_sensor_unregister(&pdev->dev,
>> +						  tegra->thermctl_tzs[i]);
>
> Like I said above, I think the more idiomatic way would be:
>
> 	while (i--)
> 		thermal_zone_of_sensor_unregister(...);
>

Fixed.

>> +static int tegra_soctherm_remove(struct platform_device *pdev)
>> +{
>> +	struct tegra_soctherm *tegra = platform_get_drvdata(pdev);
>> +	int i;
>
> unsigned again.

Fixed.

>
>> +
>> +	for (i = 0; i < ARRAY_SIZE(tegra->thermctl_tzs); ++i) {
>> +		thermal_zone_of_sensor_unregister(&pdev->dev,
>> +						  tegra->thermctl_tzs[i]);
>> +	}
>> +
>> +	clk_disable_unprepare(tegra->clock_tsensor);
>> +	clk_disable_unprepare(tegra->clock_soctherm);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver tegra_soctherm_driver = {
>> +	.probe = tegra_soctherm_probe,
>> +	.remove = tegra_soctherm_remove,
>> +	.driver = {
>> +		.name = "tegra_soctherm",
>
> I think we're primarily using tegra- as prefix for driver names.

Fixed.

>
>> +		.of_match_table = tegra_soctherm_of_match,
>> +	},
>> +};
>> +module_platform_driver(tegra_soctherm_driver);
>> +
>> +MODULE_AUTHOR("Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
>> +MODULE_DESCRIPTION("Tegra SOCTHERM thermal management driver");
>
> Perhaps: "NVIDIA Tegra ..."?

Fixed.

>
> Thierry
>

Thanks,
Mikko

Re: [PATCH v6 4/4] thermal: Add Tegra SOCTHERM thermal management driver

[Juha-Matti Tilli]

On Fri, Sep 26, 2014 at 11:28:31PM +0300, Mikko Perttunen wrote:
> > I think a more idiomatic way to write this would be:
> >
> > static int
> > calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
> >                               struct tsensor_shared_calibration shared,
> >                               u32 *calib)
[snip]
> >
> > While at it, perhaps make shared a const * instead of passing it in by
> > value?
> 
> That is possible, but I'm not sure what the difference would be. Is 
> there a style rule forbidding by-value compound types? (Also if I change 
> the style, it would go over 80 characters by even more.)

I guess the idea is that it's more space- and time-efficient to pass
compound types as pointers instead of by value. Kernel stack is quite
limited in size, so allocating structs from stack in this way quickly
eats up the stack. Furthermore, there's less machine language
instructions in the function call if passed as a pointer, because when
passed as a value, each member of the struct needs to be pushed to the
stack separately (unless the member size is smaller than word length of
the machine architecture, in which case the compiler may optimize a
bit). So it's faster, too, to pass by pointer. In the case of kernel
coding, of these problems the limited stack size is far more serious.

In this case, however, the current struct is only 16 bytes vs 4/8 bytes
for a pointer, so it shouldn't matter that much. But IMO as a general
rule it's a far better style to pass compound types as pointers. I would
definitely pass by pointer here instead of passing by value even given
that the advantages in this particular case are limited. You should
consider the possibility that in a future driver version struct
tsensor_shared_calibration may become larger, and thus the code will be
closer to stack overflow if the one who increases the size of struct
tsensor_shared_calibration doesn't notice that it is passed by value.

There are always ways to structure the code so that it looks fine and
the additional "const *" will not cause the line length to become >80
chars. In my opinion, line length considerations should never play a
role on deciding whether to pass by value or pass by pointer. I don't
understand why you think this particular function would have line length
problems. In my opinion, the following is 78 chars max:

static int 
calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
                              const struct tsensor_shared_calibration *shared,
                              u32 *calib)

Of course, if you want to have "static int" on the same line as the
function name, then you'll have problems but even those can be avoided
so that the code still looks fine:

static int calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
                                         const struct
                                             tsensor_shared_calibration *shared,
                                         u32 *calib)

Anyway, the root cause of line length problems here is overlong
identifiers. For example, "calculate_tsensor_calibration" is in my
opinion too long for a function name. You could easily abbreviate it to
"calc_tsensor_calib" without losing any information. Similarly, "struct
tsensor_shared_calibration" could be easily abbreviated to "struct
tsensor_shared_calib". Then you could have easily:

static int calc_tsensor_calib(const struct tegra_tsensor *sensor,
                              const struct tsensor_shared_calib *shared,
                              u32 *calib)

without being even close to the 80-character line length limitation.

Re: [PATCH v6 4/4] thermal: Add Tegra SOCTHERM thermal management driver

[Mikko Perttunen]

On 09/27/2014 03:06 PM, Juha-Matti Tilli wrote:
> On Fri, Sep 26, 2014 at 11:28:31PM +0300, Mikko Perttunen wrote:
>>> I think a more idiomatic way to write this would be:
>>>
>>> static int
>>> calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
>>>                                struct tsensor_shared_calibration shared,
>>>                                u32 *calib)
> [snip]
>>>
>>> While at it, perhaps make shared a const * instead of passing it in by
>>> value?
>>
>> That is possible, but I'm not sure what the difference would be. Is
>> there a style rule forbidding by-value compound types? (Also if I change
>> the style, it would go over 80 characters by even more.)
>
> I guess the idea is that it's more space- and time-efficient to pass
> compound types as pointers instead of by value. Kernel stack is quite
> limited in size, so allocating structs from stack in this way quickly
> eats up the stack. Furthermore, there's less machine language
> instructions in the function call if passed as a pointer, because when
> passed as a value, each member of the struct needs to be pushed to the
> stack separately (unless the member size is smaller than word length of
> the machine architecture, in which case the compiler may optimize a
> bit). So it's faster, too, to pass by pointer. In the case of kernel
> coding, of these problems the limited stack size is far more serious.

I can accept that saving kernel stack space is a good reason to use 
pointers. And since as you below mentioned, there is no line length 
issue, I'll change this.

>
> In this case, however, the current struct is only 16 bytes vs 4/8 bytes
> for a pointer, so it shouldn't matter that much. But IMO as a general
> rule it's a far better style to pass compound types as pointers. I would
> definitely pass by pointer here instead of passing by value even given
> that the advantages in this particular case are limited. You should
> consider the possibility that in a future driver version struct
> tsensor_shared_calibration may become larger, and thus the code will be
> closer to stack overflow if the one who increases the size of struct
> tsensor_shared_calibration doesn't notice that it is passed by value.
>
> There are always ways to structure the code so that it looks fine and
> the additional "const *" will not cause the line length to become >80
> chars. In my opinion, line length considerations should never play a
> role on deciding whether to pass by value or pass by pointer. I don't
> understand why you think this particular function would have line length
> problems. In my opinion, the following is 78 chars max:
>
> static int
> calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
>                                const struct tsensor_shared_calibration *shared,
>                                u32 *calib)

Good point. I usually never use this style, so I didn't think of doing 
it like that. But it is clearly the best solution.

>
> Of course, if you want to have "static int" on the same line as the
> function name, then you'll have problems but even those can be avoided
> so that the code still looks fine:
>
> static int calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
>                                           const struct
>                                               tsensor_shared_calibration *shared,
>                                           u32 *calib)
>
> Anyway, the root cause of line length problems here is overlong
> identifiers. For example, "calculate_tsensor_calibration" is in my
> opinion too long for a function name. You could easily abbreviate it to
> "calc_tsensor_calib" without losing any information. Similarly, "struct
> tsensor_shared_calibration" could be easily abbreviated to "struct
> tsensor_shared_calib". Then you could have easily:
>
> static int calc_tsensor_calib(const struct tegra_tsensor *sensor,
>                                const struct tsensor_shared_calib *shared,
>                                u32 *calib)
>
> without being even close to the 80-character line length limitation.

Yes, might be.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Mikko

Re: [PATCH v6 4/4] thermal: Add Tegra SOCTHERM thermal management driver

[Peter De Schrijver]

> >
> >>+struct tegra_tsensor_configuration {
> >>+	u32 tall, tsample, tiddq_en, ten_count;
> >>+	u32 pdiv, tsample_ate, pdiv_ate;
> >
> >What's the significance of the "ate" suffix?
> 
> Again, no idea.

I presume it's Automated Test Equipment refering to the device to test silicon
coming back from the fab.

Cheers,

Peter.

Re: [PATCH v6 4/4] thermal: Add Tegra SOCTHERM thermal management driver

[Thierry Reding]

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

On Fri, Sep 26, 2014 at 11:28:31PM +0300, Mikko Perttunen wrote:
> On 09/26/2014 02:45 PM, Thierry Reding wrote:
[...]
> > I think a more idiomatic way to write this would be:
> >
> > static int
> > calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
> > 			      struct tsensor_shared_calibration shared,
> > 			      u32 *calib)
> 
> If I do that, it will go over the 80 character limit by quite a few
> characters, which is why I didn't use that style. Personally I'm fine with
> either style.

The above doesn't exceed the 80 character limit. Putting the return
value and the static keyword on a separate line is a pretty common way
to reduce line length.

> >
> >While at it, perhaps make shared a const * instead of passing it in by
> >value?
> 
> That is possible, but I'm not sure what the difference would be. Is there a
> style rule forbidding by-value compound types? (Also if I change the style,
> it would go over 80 characters by even more.)

No it doesn't. The below fits within 80 characters per line just fine:

static int
calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
			      const struct tsensor_shared_calibration *shared,
			      u32 *calib)

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 4/4] thermal: Add Tegra SOCTHERM thermal management driver

[Mikko Perttunen]

On 09/29/2014 11:29 AM, Thierry Reding wrote:
> On Fri, Sep 26, 2014 at 11:28:31PM +0300, Mikko Perttunen wrote:
>> On 09/26/2014 02:45 PM, Thierry Reding wrote:
> [...]
>>> I think a more idiomatic way to write this would be:
>>>
>>> static int
>>> calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
>>> 			      struct tsensor_shared_calibration shared,
>>> 			      u32 *calib)
>>
>> If I do that, it will go over the 80 character limit by quite a few
>> characters, which is why I didn't use that style. Personally I'm fine with
>> either style.
>
> The above doesn't exceed the 80 character limit. Putting the return
> value and the static keyword on a separate line is a pretty common way
> to reduce line length.

Good point, I didn't think of doing that. I usually never use the above 
style, but clearly here it is the best choice.

>
>>>
>>> While at it, perhaps make shared a const * instead of passing it in by
>>> value?
>>
>> That is possible, but I'm not sure what the difference would be. Is there a
>> style rule forbidding by-value compound types? (Also if I change the style,
>> it would go over 80 characters by even more.)
>
> No it doesn't. The below fits within 80 characters per line just fine:
>
> static int
> calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
> 			      const struct tsensor_shared_calibration *shared,
> 			      u32 *calib)
>
> Thierry
>

[PATCH v7 4/4] thermal: Add Tegra SOCTHERM thermal management driver

[Mikko Perttunen]

From: Mikko Perttunen <mperttunen@nvidia.com>

This adds support for the Tegra SOCTHERM thermal sensing and management
system found in the Tegra124 system-on-chip. This initial driver supports
temperature polling for four thermal zones.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
v7: bunch of style fixes

 drivers/thermal/Kconfig          |  10 +
 drivers/thermal/Makefile         |   1 +
 drivers/thermal/tegra_soctherm.c | 473 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 484 insertions(+)
 create mode 100644 drivers/thermal/tegra_soctherm.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 693208e..fd9d049 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -175,6 +175,16 @@ config ARMADA_THERMAL
 	  Enable this option if you want to have support for thermal management
 	  controller present in Armada 370 and Armada XP SoC.
 
+config TEGRA_SOCTHERM
+	tristate "Tegra SOCTHERM thermal management"
+	depends on ARCH_TEGRA
+	help
+	  Enable this option for integrated thermal management support on NVIDIA
+	  Tegra124 systems-on-chip. The driver supports four thermal zones
+	  (CPU, GPU, MEM, PLLX). Cooling devices can be bound to the thermal
+	  zones to manage temperatures. This option is also required for the
+	  emergency thermal reset (thermtrip) feature to function.
+
 config DB8500_CPUFREQ_COOLING
 	tristate "DB8500 cpufreq cooling"
 	depends on ARCH_U8500
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 31e232f..f0b94d5 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -33,3 +33,4 @@ obj-$(CONFIG_INTEL_SOC_DTS_THERMAL)	+= intel_soc_dts_thermal.o
 obj-$(CONFIG_TI_SOC_THERMAL)	+= ti-soc-thermal/
 obj-$(CONFIG_ACPI_INT3403_THERMAL)	+= int3403_thermal.o
 obj-$(CONFIG_ST_THERMAL)	+= st/
+obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra_soctherm.o
diff --git a/drivers/thermal/tegra_soctherm.c b/drivers/thermal/tegra_soctherm.c
new file mode 100644
index 0000000..70f7e9e
--- /dev/null
+++ b/drivers/thermal/tegra_soctherm.c
@@ -0,0 +1,473 @@
+/*
+ * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * Author:
+ *	Mikko Perttunen <mperttunen@nvidia.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/thermal.h>
+
+#include <soc/tegra/fuse.h>
+
+#define SENSOR_CONFIG0				0
+#define SENSOR_CONFIG0_STOP			BIT(0)
+#define SENSOR_CONFIG0_TALL_SHIFT		8
+#define SENSOR_CONFIG0_TCALC_OVER		BIT(4)
+#define SENSOR_CONFIG0_OVER			BIT(3)
+#define SENSOR_CONFIG0_CPTR_OVER		BIT(2)
+
+#define SENSOR_CONFIG1				4
+#define SENSOR_CONFIG1_TSAMPLE_SHIFT		0
+#define SENSOR_CONFIG1_TIDDQ_EN_SHIFT		15
+#define SENSOR_CONFIG1_TEN_COUNT_SHIFT		24
+#define SENSOR_CONFIG1_TEMP_ENABLE		BIT(31)
+
+#define SENSOR_CONFIG2				8
+#define SENSOR_CONFIG2_THERMA_SHIFT		16
+#define SENSOR_CONFIG2_THERMB_SHIFT		0
+
+#define SENSOR_PDIV				0x1c0
+#define SENSOR_PDIV_T124			0x8888
+#define SENSOR_HOTSPOT_OFF			0x1c4
+#define SENSOR_HOTSPOT_OFF_T124			0x00060600
+#define SENSOR_TEMP1				0x1c8
+#define SENSOR_TEMP2				0x1cc
+
+#define SENSOR_TEMP_MASK			0xffff
+#define READBACK_VALUE_MASK			0xff00
+#define READBACK_VALUE_SHIFT			8
+#define READBACK_ADD_HALF			BIT(7)
+#define READBACK_NEGATE				BIT(1)
+
+#define FUSE_TSENSOR8_CALIB			0x180
+#define FUSE_SPARE_REALIGNMENT_REG_0		0x1fc
+
+#define FUSE_TSENSOR_CALIB_CP_TS_BASE_MASK	0x1fff
+#define FUSE_TSENSOR_CALIB_FT_TS_BASE_MASK	(0x1fff << 13)
+#define FUSE_TSENSOR_CALIB_FT_TS_BASE_SHIFT	13
+
+#define FUSE_TSENSOR8_CALIB_CP_TS_BASE_MASK	0x3ff
+#define FUSE_TSENSOR8_CALIB_FT_TS_BASE_MASK	(0x7ff << 10)
+#define FUSE_TSENSOR8_CALIB_FT_TS_BASE_SHIFT	10
+
+#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_CP_MASK 0x3f
+#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_MASK (0x1f << 21)
+#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_SHIFT 21
+
+#define NOMINAL_CALIB_FT_T124			105
+#define NOMINAL_CALIB_CP_T124			25
+
+struct tegra_tsensor_configuration {
+	u32 tall, tsample, tiddq_en, ten_count, pdiv, tsample_ate, pdiv_ate;
+};
+
+struct tegra_tsensor {
+	const struct tegra_tsensor_configuration *config;
+	u32 base, calib_fuse_offset;
+	/* Correction values used to modify values read from calibration fuses */
+	s32 fuse_corr_alpha, fuse_corr_beta;
+};
+
+struct tegra_thermctl_zone {
+	void __iomem *reg;
+	unsigned int shift;
+};
+
+static const struct tegra_tsensor_configuration t124_tsensor_config = {
+	.tall = 16300,
+	.tsample = 120,
+	.tiddq_en = 1,
+	.ten_count = 1,
+	.pdiv = 8,
+	.tsample_ate = 480,
+	.pdiv_ate = 8
+};
+
+static const struct tegra_tsensor t124_tsensors[] = {
+	{
+		.config = &t124_tsensor_config,
+		.base = 0xc0,
+		.calib_fuse_offset = 0x098,
+		.fuse_corr_alpha = 1135400,
+		.fuse_corr_beta = -6266900,
+	},
+	{
+		.config = &t124_tsensor_config,
+		.base = 0xe0,
+		.calib_fuse_offset = 0x084,
+		.fuse_corr_alpha = 1122220,
+		.fuse_corr_beta = -5700700,
+	},
+	{
+		.config = &t124_tsensor_config,
+		.base = 0x100,
+		.calib_fuse_offset = 0x088,
+		.fuse_corr_alpha = 1127000,
+		.fuse_corr_beta = -6768200,
+	},
+	{
+		.config = &t124_tsensor_config,
+		.base = 0x120,
+		.calib_fuse_offset = 0x12c,
+		.fuse_corr_alpha = 1110900,
+		.fuse_corr_beta = -6232000,
+	},
+	{
+		.config = &t124_tsensor_config,
+		.base = 0x140,
+		.calib_fuse_offset = 0x158,
+		.fuse_corr_alpha = 1122300,
+		.fuse_corr_beta = -5936400,
+	},
+	{
+		.config = &t124_tsensor_config,
+		.base = 0x160,
+		.calib_fuse_offset = 0x15c,
+		.fuse_corr_alpha = 1145700,
+		.fuse_corr_beta = -7124600,
+	},
+	{
+		.config = &t124_tsensor_config,
+		.base = 0x180,
+		.calib_fuse_offset = 0x154,
+		.fuse_corr_alpha = 1120100,
+		.fuse_corr_beta = -6000500,
+	},
+	{
+		.config = &t124_tsensor_config,
+		.base = 0x1a0,
+		.calib_fuse_offset = 0x160,
+		.fuse_corr_alpha = 1106500,
+		.fuse_corr_beta = -6729300,
+	},
+};
+
+struct tegra_soctherm {
+	struct reset_control *reset;
+	struct clk *clock_tsensor;
+	struct clk *clock_soctherm;
+	void __iomem *regs;
+
+	struct thermal_zone_device *thermctl_tzs[4];
+};
+
+struct tsensor_shared_calibration {
+	u32 base_cp, base_ft;
+	u32 actual_temp_cp, actual_temp_ft;
+};
+
+static int calculate_shared_calibration(struct tsensor_shared_calibration *r)
+{
+	u32 val, shifted_cp, shifted_ft;
+	int err;
+
+	err = tegra_fuse_readl(FUSE_TSENSOR8_CALIB, &val);
+	if (err)
+		return err;
+	r->base_cp = val & FUSE_TSENSOR8_CALIB_CP_TS_BASE_MASK;
+	r->base_ft = (val & FUSE_TSENSOR8_CALIB_FT_TS_BASE_MASK)
+		>> FUSE_TSENSOR8_CALIB_FT_TS_BASE_SHIFT;
+	val = ((val & FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_MASK)
+		>> FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_SHIFT);
+	shifted_ft = sign_extend32(val, 4);
+
+	err = tegra_fuse_readl(FUSE_SPARE_REALIGNMENT_REG_0, &val);
+	if (err)
+		return err;
+	shifted_cp = sign_extend32(val, 5);
+
+	r->actual_temp_cp = 2 * NOMINAL_CALIB_CP_T124 + shifted_cp;
+	r->actual_temp_ft = 2 * NOMINAL_CALIB_FT_T124 + shifted_ft;
+
+	return 0;
+}
+
+static s64 div64_s64_precise(s64 a, s64 b)
+{
+	s64 r, al;
+
+	/* Scale up for increased precision division */
+	al = a << 16;
+
+	r = div64_s64(al * 2 + 1, 2 * b);
+	return r >> 16;
+}
+
+static int
+calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
+			      const struct tsensor_shared_calibration *shared,
+			      u32 *calib)
+{
+	u32 val;
+	s32 actual_tsensor_ft, actual_tsensor_cp, delta_sens, delta_temp,
+	    mult, div;
+	s16 therma, thermb;
+	s64 tmp;
+	int err;
+
+	err = tegra_fuse_readl(sensor->calib_fuse_offset, &val);
+	if (err)
+		return err;
+
+	actual_tsensor_cp = (shared->base_cp * 64) + sign_extend32(val, 12);
+	val = (val & FUSE_TSENSOR_CALIB_FT_TS_BASE_MASK)
+		>> FUSE_TSENSOR_CALIB_FT_TS_BASE_SHIFT;
+	actual_tsensor_ft = (shared->base_ft * 32) + sign_extend32(val, 12);
+
+	delta_sens = actual_tsensor_ft - actual_tsensor_cp;
+	delta_temp = shared->actual_temp_ft - shared->actual_temp_cp;
+
+	mult = sensor->config->pdiv * sensor->config->tsample_ate;
+	div = sensor->config->tsample * sensor->config->pdiv_ate;
+
+	therma = div64_s64_precise((s64) delta_temp * (1LL << 13) * mult,
+				   (s64) delta_sens * div);
+
+	tmp = (s64)actual_tsensor_ft * shared->actual_temp_cp -
+	      (s64)actual_tsensor_cp * shared->actual_temp_ft;
+	thermb = div64_s64_precise(tmp, (s64)delta_sens);
+
+	therma = div64_s64_precise((s64)therma * sensor->fuse_corr_alpha,
+				   (s64)1000000LL);
+	thermb = div64_s64_precise((s64)thermb * sensor->fuse_corr_alpha +
+				   sensor->fuse_corr_beta, (s64)1000000LL);
+
+	*calib = ((u16)therma << SENSOR_CONFIG2_THERMA_SHIFT) |
+		 ((u16)thermb << SENSOR_CONFIG2_THERMB_SHIFT);
+
+	return 0;
+}
+
+static int enable_tsensor(struct tegra_soctherm *tegra,
+			  const struct tegra_tsensor *sensor,
+			  const struct tsensor_shared_calibration *shared)
+{
+	void __iomem *base = tegra->regs + sensor->base;
+	unsigned int val;
+	u32 calib;
+	int err;
+
+	err = calculate_tsensor_calibration(sensor, shared, &calib);
+	if (err)
+		return err;
+
+	val = sensor->config->tall << SENSOR_CONFIG0_TALL_SHIFT;
+	writel(val, base + SENSOR_CONFIG0);
+
+	val  = (sensor->config->tsample - 1) << SENSOR_CONFIG1_TSAMPLE_SHIFT;
+	val |= sensor->config->tiddq_en << SENSOR_CONFIG1_TIDDQ_EN_SHIFT;
+	val |= sensor->config->ten_count << SENSOR_CONFIG1_TEN_COUNT_SHIFT;
+	val |= SENSOR_CONFIG1_TEMP_ENABLE;
+	writel(val, base + SENSOR_CONFIG1);
+
+	writel(calib, base + SENSOR_CONFIG2);
+
+	return 0;
+}
+
+/*
+ * Translate from soctherm readback format to millicelsius.
+ * The soctherm readback format in bits is as follows:
+ *   TTTTTTTT H______N
+ * where T's contain the temperature in Celsius,
+ * H denotes an addition of 0.5 Celsius and N denotes negation
+ * of the final value.
+ */
+static long translate_temp(u16 val)
+{
+	long t;
+
+	t = ((val & READBACK_VALUE_MASK) >> READBACK_VALUE_SHIFT) * 1000;
+	if (val & READBACK_ADD_HALF)
+		t += 500;
+	if (val & READBACK_NEGATE)
+		t *= -1;
+
+	return t;
+}
+
+static int tegra_thermctl_get_temp(void *data, long *out_temp)
+{
+	struct tegra_thermctl_zone *zone = data;
+	u32 val;
+
+	val = (readl(zone->reg) >> zone->shift) & SENSOR_TEMP_MASK;
+	*out_temp = translate_temp(val);
+
+	return 0;
+}
+
+static const struct of_device_id tegra_soctherm_of_match[] = {
+	{ .compatible = "nvidia,tegra124-soctherm" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tegra_soctherm_of_match);
+
+struct thermctl_zone_desc {
+	unsigned int offset;
+	unsigned int shift;
+};
+
+static const struct thermctl_zone_desc t124_thermctl_temp_zones[] = {
+	{ SENSOR_TEMP1, 16 },
+	{ SENSOR_TEMP2, 16 },
+	{ SENSOR_TEMP1, 0 },
+	{ SENSOR_TEMP2, 0 }
+};
+
+static int tegra_soctherm_probe(struct platform_device *pdev)
+{
+	struct tegra_soctherm *tegra;
+	struct thermal_zone_device *tz;
+	struct tsensor_shared_calibration shared_calib;
+	struct resource *res;
+	unsigned int i;
+	int err;
+
+	const struct tegra_tsensor *tsensors = t124_tsensors;
+
+	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
+	if (!tegra)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	tegra->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(tegra->regs))
+		return PTR_ERR(tegra->regs);
+
+	tegra->reset = devm_reset_control_get(&pdev->dev, "soctherm");
+	if (IS_ERR(tegra->reset)) {
+		dev_err(&pdev->dev, "can't get soctherm reset\n");
+		return PTR_ERR(tegra->reset);
+	}
+
+	tegra->clock_tsensor = devm_clk_get(&pdev->dev, "tsensor");
+	if (IS_ERR(tegra->clock_tsensor)) {
+		dev_err(&pdev->dev, "can't get tsensor clock\n");
+		return PTR_ERR(tegra->clock_tsensor);
+	}
+
+	tegra->clock_soctherm = devm_clk_get(&pdev->dev, "soctherm");
+	if (IS_ERR(tegra->clock_soctherm)) {
+		dev_err(&pdev->dev, "can't get soctherm clock\n");
+		return PTR_ERR(tegra->clock_soctherm);
+	}
+
+	reset_control_assert(tegra->reset);
+
+	err = clk_prepare_enable(tegra->clock_soctherm);
+	if (err)
+		return err;
+
+	err = clk_prepare_enable(tegra->clock_tsensor);
+	if (err) {
+		clk_disable_unprepare(tegra->clock_soctherm);
+		return err;
+	}
+
+	reset_control_deassert(tegra->reset);
+
+	/* Initialize raw sensors */
+
+	err = calculate_shared_calibration(&shared_calib);
+	if (err)
+		goto disable_clocks;
+
+	for (i = 0; i < ARRAY_SIZE(t124_tsensors); ++i) {
+		err = enable_tsensor(tegra, tsensors + i, &shared_calib);
+		if (err)
+			goto disable_clocks;
+	}
+
+	writel(SENSOR_PDIV_T124, tegra->regs + SENSOR_PDIV);
+	writel(SENSOR_HOTSPOT_OFF_T124, tegra->regs + SENSOR_HOTSPOT_OFF);
+
+	/* Initialize thermctl sensors */
+
+	for (i = 0; i < ARRAY_SIZE(tegra->thermctl_tzs); ++i) {
+		struct tegra_thermctl_zone *zone =
+			devm_kzalloc(&pdev->dev, sizeof(*zone), GFP_KERNEL);
+		if (!zone) {
+			err = -ENOMEM;
+			goto unregister_tzs;
+		}
+
+		zone->reg = tegra->regs + t124_thermctl_temp_zones[i].offset;
+		zone->shift = t124_thermctl_temp_zones[i].shift;
+
+		tz = thermal_zone_of_sensor_register(&pdev->dev, i, zone,
+						     tegra_thermctl_get_temp,
+						     NULL);
+		if (IS_ERR(tz)) {
+			err = PTR_ERR(tz);
+			dev_err(&pdev->dev, "failed to register sensor: %d\n",
+				err);
+			goto unregister_tzs;
+		}
+
+		tegra->thermctl_tzs[i] = tz;
+	}
+
+	return 0;
+
+unregister_tzs:
+	while (i--)
+		thermal_zone_of_sensor_unregister(&pdev->dev,
+						  tegra->thermctl_tzs[i]);
+
+disable_clocks:
+	clk_disable_unprepare(tegra->clock_tsensor);
+	clk_disable_unprepare(tegra->clock_soctherm);
+
+	return err;
+}
+
+static int tegra_soctherm_remove(struct platform_device *pdev)
+{
+	struct tegra_soctherm *tegra = platform_get_drvdata(pdev);
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(tegra->thermctl_tzs); ++i) {
+		thermal_zone_of_sensor_unregister(&pdev->dev,
+						  tegra->thermctl_tzs[i]);
+	}
+
+	clk_disable_unprepare(tegra->clock_tsensor);
+	clk_disable_unprepare(tegra->clock_soctherm);
+
+	return 0;
+}
+
+static struct platform_driver tegra_soctherm_driver = {
+	.probe = tegra_soctherm_probe,
+	.remove = tegra_soctherm_remove,
+	.driver = {
+		.name = "tegra-soctherm",
+		.of_match_table = tegra_soctherm_of_match,
+	},
+};
+module_platform_driver(tegra_soctherm_driver);
+
+MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
+MODULE_DESCRIPTION("NVIDIA Tegra SOCTHERM thermal management driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.0

Re: [PATCH v7 4/4] thermal: Add Tegra SOCTHERM thermal management driver

[Mikko Perttunen]

Eduardo: ping

Cheers,
Mikko

On 09/29/2014 05:17 PM, Mikko Perttunen wrote:
> From: Mikko Perttunen <mperttunen@nvidia.com>
>
> This adds support for the Tegra SOCTHERM thermal sensing and management
> system found in the Tegra124 system-on-chip. This initial driver supports
> temperature polling for four thermal zones.
>
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
> v7: bunch of style fixes
>
>   drivers/thermal/Kconfig          |  10 +
>   drivers/thermal/Makefile         |   1 +
>   drivers/thermal/tegra_soctherm.c | 473 +++++++++++++++++++++++++++++++++++++++
>   3 files changed, 484 insertions(+)
>   create mode 100644 drivers/thermal/tegra_soctherm.c
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 693208e..fd9d049 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -175,6 +175,16 @@ config ARMADA_THERMAL
>   	  Enable this option if you want to have support for thermal management
>   	  controller present in Armada 370 and Armada XP SoC.
>
> +config TEGRA_SOCTHERM
> +	tristate "Tegra SOCTHERM thermal management"
> +	depends on ARCH_TEGRA
> +	help
> +	  Enable this option for integrated thermal management support on NVIDIA
> +	  Tegra124 systems-on-chip. The driver supports four thermal zones
> +	  (CPU, GPU, MEM, PLLX). Cooling devices can be bound to the thermal
> +	  zones to manage temperatures. This option is also required for the
> +	  emergency thermal reset (thermtrip) feature to function.
> +
>   config DB8500_CPUFREQ_COOLING
>   	tristate "DB8500 cpufreq cooling"
>   	depends on ARCH_U8500
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 31e232f..f0b94d5 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -33,3 +33,4 @@ obj-$(CONFIG_INTEL_SOC_DTS_THERMAL)	+= intel_soc_dts_thermal.o
>   obj-$(CONFIG_TI_SOC_THERMAL)	+= ti-soc-thermal/
>   obj-$(CONFIG_ACPI_INT3403_THERMAL)	+= int3403_thermal.o
>   obj-$(CONFIG_ST_THERMAL)	+= st/
> +obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra_soctherm.o
> diff --git a/drivers/thermal/tegra_soctherm.c b/drivers/thermal/tegra_soctherm.c
> new file mode 100644
> index 0000000..70f7e9e
> --- /dev/null
> +++ b/drivers/thermal/tegra_soctherm.c
> @@ -0,0 +1,473 @@
> +/*
> + * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * Author:
> + *	Mikko Perttunen <mperttunen@nvidia.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/thermal.h>
> +
> +#include <soc/tegra/fuse.h>
> +
> +#define SENSOR_CONFIG0				0
> +#define SENSOR_CONFIG0_STOP			BIT(0)
> +#define SENSOR_CONFIG0_TALL_SHIFT		8
> +#define SENSOR_CONFIG0_TCALC_OVER		BIT(4)
> +#define SENSOR_CONFIG0_OVER			BIT(3)
> +#define SENSOR_CONFIG0_CPTR_OVER		BIT(2)
> +
> +#define SENSOR_CONFIG1				4
> +#define SENSOR_CONFIG1_TSAMPLE_SHIFT		0
> +#define SENSOR_CONFIG1_TIDDQ_EN_SHIFT		15
> +#define SENSOR_CONFIG1_TEN_COUNT_SHIFT		24
> +#define SENSOR_CONFIG1_TEMP_ENABLE		BIT(31)
> +
> +#define SENSOR_CONFIG2				8
> +#define SENSOR_CONFIG2_THERMA_SHIFT		16
> +#define SENSOR_CONFIG2_THERMB_SHIFT		0
> +
> +#define SENSOR_PDIV				0x1c0
> +#define SENSOR_PDIV_T124			0x8888
> +#define SENSOR_HOTSPOT_OFF			0x1c4
> +#define SENSOR_HOTSPOT_OFF_T124			0x00060600
> +#define SENSOR_TEMP1				0x1c8
> +#define SENSOR_TEMP2				0x1cc
> +
> +#define SENSOR_TEMP_MASK			0xffff
> +#define READBACK_VALUE_MASK			0xff00
> +#define READBACK_VALUE_SHIFT			8
> +#define READBACK_ADD_HALF			BIT(7)
> +#define READBACK_NEGATE				BIT(1)
> +
> +#define FUSE_TSENSOR8_CALIB			0x180
> +#define FUSE_SPARE_REALIGNMENT_REG_0		0x1fc
> +
> +#define FUSE_TSENSOR_CALIB_CP_TS_BASE_MASK	0x1fff
> +#define FUSE_TSENSOR_CALIB_FT_TS_BASE_MASK	(0x1fff << 13)
> +#define FUSE_TSENSOR_CALIB_FT_TS_BASE_SHIFT	13
> +
> +#define FUSE_TSENSOR8_CALIB_CP_TS_BASE_MASK	0x3ff
> +#define FUSE_TSENSOR8_CALIB_FT_TS_BASE_MASK	(0x7ff << 10)
> +#define FUSE_TSENSOR8_CALIB_FT_TS_BASE_SHIFT	10
> +
> +#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_CP_MASK 0x3f
> +#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_MASK (0x1f << 21)
> +#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_SHIFT 21
> +
> +#define NOMINAL_CALIB_FT_T124			105
> +#define NOMINAL_CALIB_CP_T124			25
> +
> +struct tegra_tsensor_configuration {
> +	u32 tall, tsample, tiddq_en, ten_count, pdiv, tsample_ate, pdiv_ate;
> +};
> +
> +struct tegra_tsensor {
> +	const struct tegra_tsensor_configuration *config;
> +	u32 base, calib_fuse_offset;
> +	/* Correction values used to modify values read from calibration fuses */
> +	s32 fuse_corr_alpha, fuse_corr_beta;
> +};
> +
> +struct tegra_thermctl_zone {
> +	void __iomem *reg;
> +	unsigned int shift;
> +};
> +
> +static const struct tegra_tsensor_configuration t124_tsensor_config = {
> +	.tall = 16300,
> +	.tsample = 120,
> +	.tiddq_en = 1,
> +	.ten_count = 1,
> +	.pdiv = 8,
> +	.tsample_ate = 480,
> +	.pdiv_ate = 8
> +};
> +
> +static const struct tegra_tsensor t124_tsensors[] = {
> +	{
> +		.config = &t124_tsensor_config,
> +		.base = 0xc0,
> +		.calib_fuse_offset = 0x098,
> +		.fuse_corr_alpha = 1135400,
> +		.fuse_corr_beta = -6266900,
> +	},
> +	{
> +		.config = &t124_tsensor_config,
> +		.base = 0xe0,
> +		.calib_fuse_offset = 0x084,
> +		.fuse_corr_alpha = 1122220,
> +		.fuse_corr_beta = -5700700,
> +	},
> +	{
> +		.config = &t124_tsensor_config,
> +		.base = 0x100,
> +		.calib_fuse_offset = 0x088,
> +		.fuse_corr_alpha = 1127000,
> +		.fuse_corr_beta = -6768200,
> +	},
> +	{
> +		.config = &t124_tsensor_config,
> +		.base = 0x120,
> +		.calib_fuse_offset = 0x12c,
> +		.fuse_corr_alpha = 1110900,
> +		.fuse_corr_beta = -6232000,
> +	},
> +	{
> +		.config = &t124_tsensor_config,
> +		.base = 0x140,
> +		.calib_fuse_offset = 0x158,
> +		.fuse_corr_alpha = 1122300,
> +		.fuse_corr_beta = -5936400,
> +	},
> +	{
> +		.config = &t124_tsensor_config,
> +		.base = 0x160,
> +		.calib_fuse_offset = 0x15c,
> +		.fuse_corr_alpha = 1145700,
> +		.fuse_corr_beta = -7124600,
> +	},
> +	{
> +		.config = &t124_tsensor_config,
> +		.base = 0x180,
> +		.calib_fuse_offset = 0x154,
> +		.fuse_corr_alpha = 1120100,
> +		.fuse_corr_beta = -6000500,
> +	},
> +	{
> +		.config = &t124_tsensor_config,
> +		.base = 0x1a0,
> +		.calib_fuse_offset = 0x160,
> +		.fuse_corr_alpha = 1106500,
> +		.fuse_corr_beta = -6729300,
> +	},
> +};
> +
> +struct tegra_soctherm {
> +	struct reset_control *reset;
> +	struct clk *clock_tsensor;
> +	struct clk *clock_soctherm;
> +	void __iomem *regs;
> +
> +	struct thermal_zone_device *thermctl_tzs[4];
> +};
> +
> +struct tsensor_shared_calibration {
> +	u32 base_cp, base_ft;
> +	u32 actual_temp_cp, actual_temp_ft;
> +};
> +
> +static int calculate_shared_calibration(struct tsensor_shared_calibration *r)
> +{
> +	u32 val, shifted_cp, shifted_ft;
> +	int err;
> +
> +	err = tegra_fuse_readl(FUSE_TSENSOR8_CALIB, &val);
> +	if (err)
> +		return err;
> +	r->base_cp = val & FUSE_TSENSOR8_CALIB_CP_TS_BASE_MASK;
> +	r->base_ft = (val & FUSE_TSENSOR8_CALIB_FT_TS_BASE_MASK)
> +		>> FUSE_TSENSOR8_CALIB_FT_TS_BASE_SHIFT;
> +	val = ((val & FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_MASK)
> +		>> FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_SHIFT);
> +	shifted_ft = sign_extend32(val, 4);
> +
> +	err = tegra_fuse_readl(FUSE_SPARE_REALIGNMENT_REG_0, &val);
> +	if (err)
> +		return err;
> +	shifted_cp = sign_extend32(val, 5);
> +
> +	r->actual_temp_cp = 2 * NOMINAL_CALIB_CP_T124 + shifted_cp;
> +	r->actual_temp_ft = 2 * NOMINAL_CALIB_FT_T124 + shifted_ft;
> +
> +	return 0;
> +}
> +
> +static s64 div64_s64_precise(s64 a, s64 b)
> +{
> +	s64 r, al;
> +
> +	/* Scale up for increased precision division */
> +	al = a << 16;
> +
> +	r = div64_s64(al * 2 + 1, 2 * b);
> +	return r >> 16;
> +}
> +
> +static int
> +calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
> +			      const struct tsensor_shared_calibration *shared,
> +			      u32 *calib)
> +{
> +	u32 val;
> +	s32 actual_tsensor_ft, actual_tsensor_cp, delta_sens, delta_temp,
> +	    mult, div;
> +	s16 therma, thermb;
> +	s64 tmp;
> +	int err;
> +
> +	err = tegra_fuse_readl(sensor->calib_fuse_offset, &val);
> +	if (err)
> +		return err;
> +
> +	actual_tsensor_cp = (shared->base_cp * 64) + sign_extend32(val, 12);
> +	val = (val & FUSE_TSENSOR_CALIB_FT_TS_BASE_MASK)
> +		>> FUSE_TSENSOR_CALIB_FT_TS_BASE_SHIFT;
> +	actual_tsensor_ft = (shared->base_ft * 32) + sign_extend32(val, 12);
> +
> +	delta_sens = actual_tsensor_ft - actual_tsensor_cp;
> +	delta_temp = shared->actual_temp_ft - shared->actual_temp_cp;
> +
> +	mult = sensor->config->pdiv * sensor->config->tsample_ate;
> +	div = sensor->config->tsample * sensor->config->pdiv_ate;
> +
> +	therma = div64_s64_precise((s64) delta_temp * (1LL << 13) * mult,
> +				   (s64) delta_sens * div);
> +
> +	tmp = (s64)actual_tsensor_ft * shared->actual_temp_cp -
> +	      (s64)actual_tsensor_cp * shared->actual_temp_ft;
> +	thermb = div64_s64_precise(tmp, (s64)delta_sens);
> +
> +	therma = div64_s64_precise((s64)therma * sensor->fuse_corr_alpha,
> +				   (s64)1000000LL);
> +	thermb = div64_s64_precise((s64)thermb * sensor->fuse_corr_alpha +
> +				   sensor->fuse_corr_beta, (s64)1000000LL);
> +
> +	*calib = ((u16)therma << SENSOR_CONFIG2_THERMA_SHIFT) |
> +		 ((u16)thermb << SENSOR_CONFIG2_THERMB_SHIFT);
> +
> +	return 0;
> +}
> +
> +static int enable_tsensor(struct tegra_soctherm *tegra,
> +			  const struct tegra_tsensor *sensor,
> +			  const struct tsensor_shared_calibration *shared)
> +{
> +	void __iomem *base = tegra->regs + sensor->base;
> +	unsigned int val;
> +	u32 calib;
> +	int err;
> +
> +	err = calculate_tsensor_calibration(sensor, shared, &calib);
> +	if (err)
> +		return err;
> +
> +	val = sensor->config->tall << SENSOR_CONFIG0_TALL_SHIFT;
> +	writel(val, base + SENSOR_CONFIG0);
> +
> +	val  = (sensor->config->tsample - 1) << SENSOR_CONFIG1_TSAMPLE_SHIFT;
> +	val |= sensor->config->tiddq_en << SENSOR_CONFIG1_TIDDQ_EN_SHIFT;
> +	val |= sensor->config->ten_count << SENSOR_CONFIG1_TEN_COUNT_SHIFT;
> +	val |= SENSOR_CONFIG1_TEMP_ENABLE;
> +	writel(val, base + SENSOR_CONFIG1);
> +
> +	writel(calib, base + SENSOR_CONFIG2);
> +
> +	return 0;
> +}
> +
> +/*
> + * Translate from soctherm readback format to millicelsius.
> + * The soctherm readback format in bits is as follows:
> + *   TTTTTTTT H______N
> + * where T's contain the temperature in Celsius,
> + * H denotes an addition of 0.5 Celsius and N denotes negation
> + * of the final value.
> + */
> +static long translate_temp(u16 val)
> +{
> +	long t;
> +
> +	t = ((val & READBACK_VALUE_MASK) >> READBACK_VALUE_SHIFT) * 1000;
> +	if (val & READBACK_ADD_HALF)
> +		t += 500;
> +	if (val & READBACK_NEGATE)
> +		t *= -1;
> +
> +	return t;
> +}
> +
> +static int tegra_thermctl_get_temp(void *data, long *out_temp)
> +{
> +	struct tegra_thermctl_zone *zone = data;
> +	u32 val;
> +
> +	val = (readl(zone->reg) >> zone->shift) & SENSOR_TEMP_MASK;
> +	*out_temp = translate_temp(val);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id tegra_soctherm_of_match[] = {
> +	{ .compatible = "nvidia,tegra124-soctherm" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, tegra_soctherm_of_match);
> +
> +struct thermctl_zone_desc {
> +	unsigned int offset;
> +	unsigned int shift;
> +};
> +
> +static const struct thermctl_zone_desc t124_thermctl_temp_zones[] = {
> +	{ SENSOR_TEMP1, 16 },
> +	{ SENSOR_TEMP2, 16 },
> +	{ SENSOR_TEMP1, 0 },
> +	{ SENSOR_TEMP2, 0 }
> +};
> +
> +static int tegra_soctherm_probe(struct platform_device *pdev)
> +{
> +	struct tegra_soctherm *tegra;
> +	struct thermal_zone_device *tz;
> +	struct tsensor_shared_calibration shared_calib;
> +	struct resource *res;
> +	unsigned int i;
> +	int err;
> +
> +	const struct tegra_tsensor *tsensors = t124_tsensors;
> +
> +	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> +	if (!tegra)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	tegra->regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(tegra->regs))
> +		return PTR_ERR(tegra->regs);
> +
> +	tegra->reset = devm_reset_control_get(&pdev->dev, "soctherm");
> +	if (IS_ERR(tegra->reset)) {
> +		dev_err(&pdev->dev, "can't get soctherm reset\n");
> +		return PTR_ERR(tegra->reset);
> +	}
> +
> +	tegra->clock_tsensor = devm_clk_get(&pdev->dev, "tsensor");
> +	if (IS_ERR(tegra->clock_tsensor)) {
> +		dev_err(&pdev->dev, "can't get tsensor clock\n");
> +		return PTR_ERR(tegra->clock_tsensor);
> +	}
> +
> +	tegra->clock_soctherm = devm_clk_get(&pdev->dev, "soctherm");
> +	if (IS_ERR(tegra->clock_soctherm)) {
> +		dev_err(&pdev->dev, "can't get soctherm clock\n");
> +		return PTR_ERR(tegra->clock_soctherm);
> +	}
> +
> +	reset_control_assert(tegra->reset);
> +
> +	err = clk_prepare_enable(tegra->clock_soctherm);
> +	if (err)
> +		return err;
> +
> +	err = clk_prepare_enable(tegra->clock_tsensor);
> +	if (err) {
> +		clk_disable_unprepare(tegra->clock_soctherm);
> +		return err;
> +	}
> +
> +	reset_control_deassert(tegra->reset);
> +
> +	/* Initialize raw sensors */
> +
> +	err = calculate_shared_calibration(&shared_calib);
> +	if (err)
> +		goto disable_clocks;
> +
> +	for (i = 0; i < ARRAY_SIZE(t124_tsensors); ++i) {
> +		err = enable_tsensor(tegra, tsensors + i, &shared_calib);
> +		if (err)
> +			goto disable_clocks;
> +	}
> +
> +	writel(SENSOR_PDIV_T124, tegra->regs + SENSOR_PDIV);
> +	writel(SENSOR_HOTSPOT_OFF_T124, tegra->regs + SENSOR_HOTSPOT_OFF);
> +
> +	/* Initialize thermctl sensors */
> +
> +	for (i = 0; i < ARRAY_SIZE(tegra->thermctl_tzs); ++i) {
> +		struct tegra_thermctl_zone *zone =
> +			devm_kzalloc(&pdev->dev, sizeof(*zone), GFP_KERNEL);
> +		if (!zone) {
> +			err = -ENOMEM;
> +			goto unregister_tzs;
> +		}
> +
> +		zone->reg = tegra->regs + t124_thermctl_temp_zones[i].offset;
> +		zone->shift = t124_thermctl_temp_zones[i].shift;
> +
> +		tz = thermal_zone_of_sensor_register(&pdev->dev, i, zone,
> +						     tegra_thermctl_get_temp,
> +						     NULL);
> +		if (IS_ERR(tz)) {
> +			err = PTR_ERR(tz);
> +			dev_err(&pdev->dev, "failed to register sensor: %d\n",
> +				err);
> +			goto unregister_tzs;
> +		}
> +
> +		tegra->thermctl_tzs[i] = tz;
> +	}
> +
> +	return 0;
> +
> +unregister_tzs:
> +	while (i--)
> +		thermal_zone_of_sensor_unregister(&pdev->dev,
> +						  tegra->thermctl_tzs[i]);
> +
> +disable_clocks:
> +	clk_disable_unprepare(tegra->clock_tsensor);
> +	clk_disable_unprepare(tegra->clock_soctherm);
> +
> +	return err;
> +}
> +
> +static int tegra_soctherm_remove(struct platform_device *pdev)
> +{
> +	struct tegra_soctherm *tegra = platform_get_drvdata(pdev);
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(tegra->thermctl_tzs); ++i) {
> +		thermal_zone_of_sensor_unregister(&pdev->dev,
> +						  tegra->thermctl_tzs[i]);
> +	}
> +
> +	clk_disable_unprepare(tegra->clock_tsensor);
> +	clk_disable_unprepare(tegra->clock_soctherm);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver tegra_soctherm_driver = {
> +	.probe = tegra_soctherm_probe,
> +	.remove = tegra_soctherm_remove,
> +	.driver = {
> +		.name = "tegra-soctherm",
> +		.of_match_table = tegra_soctherm_of_match,
> +	},
> +};
> +module_platform_driver(tegra_soctherm_driver);
> +
> +MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
> +MODULE_DESCRIPTION("NVIDIA Tegra SOCTHERM thermal management driver");
> +MODULE_LICENSE("GPL v2");
>

Re: [PATCH v7 4/4] thermal: Add Tegra SOCTHERM thermal management driver

[Eduardo Valentin]

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

Terve Mikko,

On Wed, Oct 15, 2014 at 01:05:19PM +0300, Mikko Perttunen wrote:
> Eduardo: ping
> 


I had no objections with the driver at this point. Neither with the DT
part. I decided to include it in my -linus queue, which means, it will
hopefully be queued in the next major merge windown (v3.19).

I also merged it to my -next branch, for linux-next testing.


> Cheers,
> Mikko
> 
> On 09/29/2014 05:17 PM, Mikko Perttunen wrote:
> > From: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

BTW, this nvidia address is bouncing. I noticed because my automated
'I applied it on queue xyz' notification message has returned.

> >
> > This adds support for the Tegra SOCTHERM thermal sensing and management
> > system found in the Tegra124 system-on-chip. This initial driver supports
> > temperature polling for four thermal zones.
> >
> > Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> > v7: bunch of style fixes
> >
> >   drivers/thermal/Kconfig          |  10 +
> >   drivers/thermal/Makefile         |   1 +
> >   drivers/thermal/tegra_soctherm.c | 473 +++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 484 insertions(+)
> >   create mode 100644 drivers/thermal/tegra_soctherm.c
> >
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> > index 693208e..fd9d049 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -175,6 +175,16 @@ config ARMADA_THERMAL
> >   	  Enable this option if you want to have support for thermal management
> >   	  controller present in Armada 370 and Armada XP SoC.
> >
> > +config TEGRA_SOCTHERM
> > +	tristate "Tegra SOCTHERM thermal management"
> > +	depends on ARCH_TEGRA
> > +	help
> > +	  Enable this option for integrated thermal management support on NVIDIA
> > +	  Tegra124 systems-on-chip. The driver supports four thermal zones
> > +	  (CPU, GPU, MEM, PLLX). Cooling devices can be bound to the thermal
> > +	  zones to manage temperatures. This option is also required for the
> > +	  emergency thermal reset (thermtrip) feature to function.
> > +
> >   config DB8500_CPUFREQ_COOLING
> >   	tristate "DB8500 cpufreq cooling"
> >   	depends on ARCH_U8500
> > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> > index 31e232f..f0b94d5 100644
> > --- a/drivers/thermal/Makefile
> > +++ b/drivers/thermal/Makefile
> > @@ -33,3 +33,4 @@ obj-$(CONFIG_INTEL_SOC_DTS_THERMAL)	+= intel_soc_dts_thermal.o
> >   obj-$(CONFIG_TI_SOC_THERMAL)	+= ti-soc-thermal/
> >   obj-$(CONFIG_ACPI_INT3403_THERMAL)	+= int3403_thermal.o
> >   obj-$(CONFIG_ST_THERMAL)	+= st/
> > +obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra_soctherm.o
> > diff --git a/drivers/thermal/tegra_soctherm.c b/drivers/thermal/tegra_soctherm.c
> > new file mode 100644
> > index 0000000..70f7e9e
> > --- /dev/null
> > +++ b/drivers/thermal/tegra_soctherm.c
> > @@ -0,0 +1,473 @@
> > +/*
> > + * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
> > + *
> > + * Author:
> > + *	Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > + *

Might be time to send a patch on top of my -linus queue to update the
above with a valid email address.

Thanks,

> > + * This software is licensed under the terms of the GNU General Public
> > + * License version 2, as published by the Free Software Foundation, and
> > + * may be copied, distributed, and modified under those terms.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset.h>
> > +#include <linux/thermal.h>
> > +
> > +#include <soc/tegra/fuse.h>
> > +
> > +#define SENSOR_CONFIG0				0
> > +#define SENSOR_CONFIG0_STOP			BIT(0)
> > +#define SENSOR_CONFIG0_TALL_SHIFT		8
> > +#define SENSOR_CONFIG0_TCALC_OVER		BIT(4)
> > +#define SENSOR_CONFIG0_OVER			BIT(3)
> > +#define SENSOR_CONFIG0_CPTR_OVER		BIT(2)
> > +
> > +#define SENSOR_CONFIG1				4
> > +#define SENSOR_CONFIG1_TSAMPLE_SHIFT		0
> > +#define SENSOR_CONFIG1_TIDDQ_EN_SHIFT		15
> > +#define SENSOR_CONFIG1_TEN_COUNT_SHIFT		24
> > +#define SENSOR_CONFIG1_TEMP_ENABLE		BIT(31)
> > +
> > +#define SENSOR_CONFIG2				8
> > +#define SENSOR_CONFIG2_THERMA_SHIFT		16
> > +#define SENSOR_CONFIG2_THERMB_SHIFT		0
> > +
> > +#define SENSOR_PDIV				0x1c0
> > +#define SENSOR_PDIV_T124			0x8888
> > +#define SENSOR_HOTSPOT_OFF			0x1c4
> > +#define SENSOR_HOTSPOT_OFF_T124			0x00060600
> > +#define SENSOR_TEMP1				0x1c8
> > +#define SENSOR_TEMP2				0x1cc
> > +
> > +#define SENSOR_TEMP_MASK			0xffff
> > +#define READBACK_VALUE_MASK			0xff00
> > +#define READBACK_VALUE_SHIFT			8
> > +#define READBACK_ADD_HALF			BIT(7)
> > +#define READBACK_NEGATE				BIT(1)
> > +
> > +#define FUSE_TSENSOR8_CALIB			0x180
> > +#define FUSE_SPARE_REALIGNMENT_REG_0		0x1fc
> > +
> > +#define FUSE_TSENSOR_CALIB_CP_TS_BASE_MASK	0x1fff
> > +#define FUSE_TSENSOR_CALIB_FT_TS_BASE_MASK	(0x1fff << 13)
> > +#define FUSE_TSENSOR_CALIB_FT_TS_BASE_SHIFT	13
> > +
> > +#define FUSE_TSENSOR8_CALIB_CP_TS_BASE_MASK	0x3ff
> > +#define FUSE_TSENSOR8_CALIB_FT_TS_BASE_MASK	(0x7ff << 10)
> > +#define FUSE_TSENSOR8_CALIB_FT_TS_BASE_SHIFT	10
> > +
> > +#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_CP_MASK 0x3f
> > +#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_MASK (0x1f << 21)
> > +#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_SHIFT 21
> > +
> > +#define NOMINAL_CALIB_FT_T124			105
> > +#define NOMINAL_CALIB_CP_T124			25
> > +
> > +struct tegra_tsensor_configuration {
> > +	u32 tall, tsample, tiddq_en, ten_count, pdiv, tsample_ate, pdiv_ate;
> > +};
> > +
> > +struct tegra_tsensor {
> > +	const struct tegra_tsensor_configuration *config;
> > +	u32 base, calib_fuse_offset;
> > +	/* Correction values used to modify values read from calibration fuses */
> > +	s32 fuse_corr_alpha, fuse_corr_beta;
> > +};
> > +
> > +struct tegra_thermctl_zone {
> > +	void __iomem *reg;
> > +	unsigned int shift;
> > +};
> > +
> > +static const struct tegra_tsensor_configuration t124_tsensor_config = {
> > +	.tall = 16300,
> > +	.tsample = 120,
> > +	.tiddq_en = 1,
> > +	.ten_count = 1,
> > +	.pdiv = 8,
> > +	.tsample_ate = 480,
> > +	.pdiv_ate = 8
> > +};
> > +
> > +static const struct tegra_tsensor t124_tsensors[] = {
> > +	{
> > +		.config = &t124_tsensor_config,
> > +		.base = 0xc0,
> > +		.calib_fuse_offset = 0x098,
> > +		.fuse_corr_alpha = 1135400,
> > +		.fuse_corr_beta = -6266900,
> > +	},
> > +	{
> > +		.config = &t124_tsensor_config,
> > +		.base = 0xe0,
> > +		.calib_fuse_offset = 0x084,
> > +		.fuse_corr_alpha = 1122220,
> > +		.fuse_corr_beta = -5700700,
> > +	},
> > +	{
> > +		.config = &t124_tsensor_config,
> > +		.base = 0x100,
> > +		.calib_fuse_offset = 0x088,
> > +		.fuse_corr_alpha = 1127000,
> > +		.fuse_corr_beta = -6768200,
> > +	},
> > +	{
> > +		.config = &t124_tsensor_config,
> > +		.base = 0x120,
> > +		.calib_fuse_offset = 0x12c,
> > +		.fuse_corr_alpha = 1110900,
> > +		.fuse_corr_beta = -6232000,
> > +	},
> > +	{
> > +		.config = &t124_tsensor_config,
> > +		.base = 0x140,
> > +		.calib_fuse_offset = 0x158,
> > +		.fuse_corr_alpha = 1122300,
> > +		.fuse_corr_beta = -5936400,
> > +	},
> > +	{
> > +		.config = &t124_tsensor_config,
> > +		.base = 0x160,
> > +		.calib_fuse_offset = 0x15c,
> > +		.fuse_corr_alpha = 1145700,
> > +		.fuse_corr_beta = -7124600,
> > +	},
> > +	{
> > +		.config = &t124_tsensor_config,
> > +		.base = 0x180,
> > +		.calib_fuse_offset = 0x154,
> > +		.fuse_corr_alpha = 1120100,
> > +		.fuse_corr_beta = -6000500,
> > +	},
> > +	{
> > +		.config = &t124_tsensor_config,
> > +		.base = 0x1a0,
> > +		.calib_fuse_offset = 0x160,
> > +		.fuse_corr_alpha = 1106500,
> > +		.fuse_corr_beta = -6729300,
> > +	},
> > +};
> > +
> > +struct tegra_soctherm {
> > +	struct reset_control *reset;
> > +	struct clk *clock_tsensor;
> > +	struct clk *clock_soctherm;
> > +	void __iomem *regs;
> > +
> > +	struct thermal_zone_device *thermctl_tzs[4];
> > +};
> > +
> > +struct tsensor_shared_calibration {
> > +	u32 base_cp, base_ft;
> > +	u32 actual_temp_cp, actual_temp_ft;
> > +};
> > +
> > +static int calculate_shared_calibration(struct tsensor_shared_calibration *r)
> > +{
> > +	u32 val, shifted_cp, shifted_ft;
> > +	int err;
> > +
> > +	err = tegra_fuse_readl(FUSE_TSENSOR8_CALIB, &val);
> > +	if (err)
> > +		return err;
> > +	r->base_cp = val & FUSE_TSENSOR8_CALIB_CP_TS_BASE_MASK;
> > +	r->base_ft = (val & FUSE_TSENSOR8_CALIB_FT_TS_BASE_MASK)
> > +		>> FUSE_TSENSOR8_CALIB_FT_TS_BASE_SHIFT;
> > +	val = ((val & FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_MASK)
> > +		>> FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_SHIFT);
> > +	shifted_ft = sign_extend32(val, 4);
> > +
> > +	err = tegra_fuse_readl(FUSE_SPARE_REALIGNMENT_REG_0, &val);
> > +	if (err)
> > +		return err;
> > +	shifted_cp = sign_extend32(val, 5);
> > +
> > +	r->actual_temp_cp = 2 * NOMINAL_CALIB_CP_T124 + shifted_cp;
> > +	r->actual_temp_ft = 2 * NOMINAL_CALIB_FT_T124 + shifted_ft;
> > +
> > +	return 0;
> > +}
> > +
> > +static s64 div64_s64_precise(s64 a, s64 b)
> > +{
> > +	s64 r, al;
> > +
> > +	/* Scale up for increased precision division */
> > +	al = a << 16;
> > +
> > +	r = div64_s64(al * 2 + 1, 2 * b);
> > +	return r >> 16;
> > +}
> > +
> > +static int
> > +calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
> > +			      const struct tsensor_shared_calibration *shared,
> > +			      u32 *calib)
> > +{
> > +	u32 val;
> > +	s32 actual_tsensor_ft, actual_tsensor_cp, delta_sens, delta_temp,
> > +	    mult, div;
> > +	s16 therma, thermb;
> > +	s64 tmp;
> > +	int err;
> > +
> > +	err = tegra_fuse_readl(sensor->calib_fuse_offset, &val);
> > +	if (err)
> > +		return err;
> > +
> > +	actual_tsensor_cp = (shared->base_cp * 64) + sign_extend32(val, 12);
> > +	val = (val & FUSE_TSENSOR_CALIB_FT_TS_BASE_MASK)
> > +		>> FUSE_TSENSOR_CALIB_FT_TS_BASE_SHIFT;
> > +	actual_tsensor_ft = (shared->base_ft * 32) + sign_extend32(val, 12);
> > +
> > +	delta_sens = actual_tsensor_ft - actual_tsensor_cp;
> > +	delta_temp = shared->actual_temp_ft - shared->actual_temp_cp;
> > +
> > +	mult = sensor->config->pdiv * sensor->config->tsample_ate;
> > +	div = sensor->config->tsample * sensor->config->pdiv_ate;
> > +
> > +	therma = div64_s64_precise((s64) delta_temp * (1LL << 13) * mult,
> > +				   (s64) delta_sens * div);
> > +
> > +	tmp = (s64)actual_tsensor_ft * shared->actual_temp_cp -
> > +	      (s64)actual_tsensor_cp * shared->actual_temp_ft;
> > +	thermb = div64_s64_precise(tmp, (s64)delta_sens);
> > +
> > +	therma = div64_s64_precise((s64)therma * sensor->fuse_corr_alpha,
> > +				   (s64)1000000LL);
> > +	thermb = div64_s64_precise((s64)thermb * sensor->fuse_corr_alpha +
> > +				   sensor->fuse_corr_beta, (s64)1000000LL);
> > +
> > +	*calib = ((u16)therma << SENSOR_CONFIG2_THERMA_SHIFT) |
> > +		 ((u16)thermb << SENSOR_CONFIG2_THERMB_SHIFT);
> > +
> > +	return 0;
> > +}
> > +
> > +static int enable_tsensor(struct tegra_soctherm *tegra,
> > +			  const struct tegra_tsensor *sensor,
> > +			  const struct tsensor_shared_calibration *shared)
> > +{
> > +	void __iomem *base = tegra->regs + sensor->base;
> > +	unsigned int val;
> > +	u32 calib;
> > +	int err;
> > +
> > +	err = calculate_tsensor_calibration(sensor, shared, &calib);
> > +	if (err)
> > +		return err;
> > +
> > +	val = sensor->config->tall << SENSOR_CONFIG0_TALL_SHIFT;
> > +	writel(val, base + SENSOR_CONFIG0);
> > +
> > +	val  = (sensor->config->tsample - 1) << SENSOR_CONFIG1_TSAMPLE_SHIFT;
> > +	val |= sensor->config->tiddq_en << SENSOR_CONFIG1_TIDDQ_EN_SHIFT;
> > +	val |= sensor->config->ten_count << SENSOR_CONFIG1_TEN_COUNT_SHIFT;
> > +	val |= SENSOR_CONFIG1_TEMP_ENABLE;
> > +	writel(val, base + SENSOR_CONFIG1);
> > +
> > +	writel(calib, base + SENSOR_CONFIG2);
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Translate from soctherm readback format to millicelsius.
> > + * The soctherm readback format in bits is as follows:
> > + *   TTTTTTTT H______N
> > + * where T's contain the temperature in Celsius,
> > + * H denotes an addition of 0.5 Celsius and N denotes negation
> > + * of the final value.
> > + */
> > +static long translate_temp(u16 val)
> > +{
> > +	long t;
> > +
> > +	t = ((val & READBACK_VALUE_MASK) >> READBACK_VALUE_SHIFT) * 1000;
> > +	if (val & READBACK_ADD_HALF)
> > +		t += 500;
> > +	if (val & READBACK_NEGATE)
> > +		t *= -1;
> > +
> > +	return t;
> > +}
> > +
> > +static int tegra_thermctl_get_temp(void *data, long *out_temp)
> > +{
> > +	struct tegra_thermctl_zone *zone = data;
> > +	u32 val;
> > +
> > +	val = (readl(zone->reg) >> zone->shift) & SENSOR_TEMP_MASK;
> > +	*out_temp = translate_temp(val);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id tegra_soctherm_of_match[] = {
> > +	{ .compatible = "nvidia,tegra124-soctherm" },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, tegra_soctherm_of_match);
> > +
> > +struct thermctl_zone_desc {
> > +	unsigned int offset;
> > +	unsigned int shift;
> > +};
> > +
> > +static const struct thermctl_zone_desc t124_thermctl_temp_zones[] = {
> > +	{ SENSOR_TEMP1, 16 },
> > +	{ SENSOR_TEMP2, 16 },
> > +	{ SENSOR_TEMP1, 0 },
> > +	{ SENSOR_TEMP2, 0 }
> > +};
> > +
> > +static int tegra_soctherm_probe(struct platform_device *pdev)
> > +{
> > +	struct tegra_soctherm *tegra;
> > +	struct thermal_zone_device *tz;
> > +	struct tsensor_shared_calibration shared_calib;
> > +	struct resource *res;
> > +	unsigned int i;
> > +	int err;
> > +
> > +	const struct tegra_tsensor *tsensors = t124_tsensors;
> > +
> > +	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
> > +	if (!tegra)
> > +		return -ENOMEM;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	tegra->regs = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(tegra->regs))
> > +		return PTR_ERR(tegra->regs);
> > +
> > +	tegra->reset = devm_reset_control_get(&pdev->dev, "soctherm");
> > +	if (IS_ERR(tegra->reset)) {
> > +		dev_err(&pdev->dev, "can't get soctherm reset\n");
> > +		return PTR_ERR(tegra->reset);
> > +	}
> > +
> > +	tegra->clock_tsensor = devm_clk_get(&pdev->dev, "tsensor");
> > +	if (IS_ERR(tegra->clock_tsensor)) {
> > +		dev_err(&pdev->dev, "can't get tsensor clock\n");
> > +		return PTR_ERR(tegra->clock_tsensor);
> > +	}
> > +
> > +	tegra->clock_soctherm = devm_clk_get(&pdev->dev, "soctherm");
> > +	if (IS_ERR(tegra->clock_soctherm)) {
> > +		dev_err(&pdev->dev, "can't get soctherm clock\n");
> > +		return PTR_ERR(tegra->clock_soctherm);
> > +	}
> > +
> > +	reset_control_assert(tegra->reset);
> > +
> > +	err = clk_prepare_enable(tegra->clock_soctherm);
> > +	if (err)
> > +		return err;
> > +
> > +	err = clk_prepare_enable(tegra->clock_tsensor);
> > +	if (err) {
> > +		clk_disable_unprepare(tegra->clock_soctherm);
> > +		return err;
> > +	}
> > +
> > +	reset_control_deassert(tegra->reset);
> > +
> > +	/* Initialize raw sensors */
> > +
> > +	err = calculate_shared_calibration(&shared_calib);
> > +	if (err)
> > +		goto disable_clocks;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(t124_tsensors); ++i) {
> > +		err = enable_tsensor(tegra, tsensors + i, &shared_calib);
> > +		if (err)
> > +			goto disable_clocks;
> > +	}
> > +
> > +	writel(SENSOR_PDIV_T124, tegra->regs + SENSOR_PDIV);
> > +	writel(SENSOR_HOTSPOT_OFF_T124, tegra->regs + SENSOR_HOTSPOT_OFF);
> > +
> > +	/* Initialize thermctl sensors */
> > +
> > +	for (i = 0; i < ARRAY_SIZE(tegra->thermctl_tzs); ++i) {
> > +		struct tegra_thermctl_zone *zone =
> > +			devm_kzalloc(&pdev->dev, sizeof(*zone), GFP_KERNEL);
> > +		if (!zone) {
> > +			err = -ENOMEM;
> > +			goto unregister_tzs;
> > +		}
> > +
> > +		zone->reg = tegra->regs + t124_thermctl_temp_zones[i].offset;
> > +		zone->shift = t124_thermctl_temp_zones[i].shift;
> > +
> > +		tz = thermal_zone_of_sensor_register(&pdev->dev, i, zone,
> > +						     tegra_thermctl_get_temp,
> > +						     NULL);
> > +		if (IS_ERR(tz)) {
> > +			err = PTR_ERR(tz);
> > +			dev_err(&pdev->dev, "failed to register sensor: %d\n",
> > +				err);
> > +			goto unregister_tzs;
> > +		}
> > +
> > +		tegra->thermctl_tzs[i] = tz;
> > +	}
> > +
> > +	return 0;
> > +
> > +unregister_tzs:
> > +	while (i--)
> > +		thermal_zone_of_sensor_unregister(&pdev->dev,
> > +						  tegra->thermctl_tzs[i]);
> > +
> > +disable_clocks:
> > +	clk_disable_unprepare(tegra->clock_tsensor);
> > +	clk_disable_unprepare(tegra->clock_soctherm);
> > +
> > +	return err;
> > +}
> > +
> > +static int tegra_soctherm_remove(struct platform_device *pdev)
> > +{
> > +	struct tegra_soctherm *tegra = platform_get_drvdata(pdev);
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(tegra->thermctl_tzs); ++i) {
> > +		thermal_zone_of_sensor_unregister(&pdev->dev,
> > +						  tegra->thermctl_tzs[i]);
> > +	}
> > +
> > +	clk_disable_unprepare(tegra->clock_tsensor);
> > +	clk_disable_unprepare(tegra->clock_soctherm);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver tegra_soctherm_driver = {
> > +	.probe = tegra_soctherm_probe,
> > +	.remove = tegra_soctherm_remove,
> > +	.driver = {
> > +		.name = "tegra-soctherm",
> > +		.of_match_table = tegra_soctherm_of_match,
> > +	},
> > +};
> > +module_platform_driver(tegra_soctherm_driver);
> > +
> > +MODULE_AUTHOR("Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
> > +MODULE_DESCRIPTION("NVIDIA Tegra SOCTHERM thermal management driver");
> > +MODULE_LICENSE("GPL v2");
> >
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v7 4/4] thermal: Add Tegra SOCTHERM thermal management driver

[Mikko Perttunen]

On 11/07/2014 05:54 PM, Eduardo Valentin wrote:
> Terve Mikko,
>
> On Wed, Oct 15, 2014 at 01:05:19PM +0300, Mikko Perttunen wrote:
>> Eduardo: ping
>>
>
>
> I had no objections with the driver at this point. Neither with the DT
> part. I decided to include it in my -linus queue, which means, it will
> hopefully be queued in the next major merge windown (v3.19).
>
> I also merged it to my -next branch, for linux-next testing.

Great! Thanks.

>
>
>> Cheers,
>> Mikko
>>
>> On 09/29/2014 05:17 PM, Mikko Perttunen wrote:
>>> From: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> BTW, this nvidia address is bouncing. I noticed because my automated
> 'I applied it on queue xyz' notification message has returned.

Yeah, I'm working at NVIDIA only during summer. Looks like this time 
they decided to deactivate my email there.

>
>>>
>>> This adds support for the Tegra SOCTHERM thermal sensing and management
>>> system found in the Tegra124 system-on-chip. This initial driver supports
>>> temperature polling for four thermal zones.
>>>
>>> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>> v7: bunch of style fixes
>>>
>>>    drivers/thermal/Kconfig          |  10 +
>>>    drivers/thermal/Makefile         |   1 +
>>>    drivers/thermal/tegra_soctherm.c | 473 +++++++++++++++++++++++++++++++++++++++
>>>    3 files changed, 484 insertions(+)
>>>    create mode 100644 drivers/thermal/tegra_soctherm.c
>>>
>>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>>> index 693208e..fd9d049 100644
>>> --- a/drivers/thermal/Kconfig
>>> +++ b/drivers/thermal/Kconfig
>>> @@ -175,6 +175,16 @@ config ARMADA_THERMAL
>>>    	  Enable this option if you want to have support for thermal management
>>>    	  controller present in Armada 370 and Armada XP SoC.
>>>
>>> +config TEGRA_SOCTHERM
>>> +	tristate "Tegra SOCTHERM thermal management"
>>> +	depends on ARCH_TEGRA
>>> +	help
>>> +	  Enable this option for integrated thermal management support on NVIDIA
>>> +	  Tegra124 systems-on-chip. The driver supports four thermal zones
>>> +	  (CPU, GPU, MEM, PLLX). Cooling devices can be bound to the thermal
>>> +	  zones to manage temperatures. This option is also required for the
>>> +	  emergency thermal reset (thermtrip) feature to function.
>>> +
>>>    config DB8500_CPUFREQ_COOLING
>>>    	tristate "DB8500 cpufreq cooling"
>>>    	depends on ARCH_U8500
>>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>>> index 31e232f..f0b94d5 100644
>>> --- a/drivers/thermal/Makefile
>>> +++ b/drivers/thermal/Makefile
>>> @@ -33,3 +33,4 @@ obj-$(CONFIG_INTEL_SOC_DTS_THERMAL)	+= intel_soc_dts_thermal.o
>>>    obj-$(CONFIG_TI_SOC_THERMAL)	+= ti-soc-thermal/
>>>    obj-$(CONFIG_ACPI_INT3403_THERMAL)	+= int3403_thermal.o
>>>    obj-$(CONFIG_ST_THERMAL)	+= st/
>>> +obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra_soctherm.o
>>> diff --git a/drivers/thermal/tegra_soctherm.c b/drivers/thermal/tegra_soctherm.c
>>> new file mode 100644
>>> index 0000000..70f7e9e
>>> --- /dev/null
>>> +++ b/drivers/thermal/tegra_soctherm.c
>>> @@ -0,0 +1,473 @@
>>> +/*
>>> + * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
>>> + *
>>> + * Author:
>>> + *	Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> + *
>
> Might be time to send a patch on top of my -linus queue to update the
> above with a valid email address.
>
> Thanks,
>
>>> + * This software is licensed under the terms of the GNU General Public
>>> + * License version 2, as published by the Free Software Foundation, and
>>> + * may be copied, distributed, and modified under those terms.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + */
>>> +
>>> +#include <linux/bitops.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/err.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/reset.h>
>>> +#include <linux/thermal.h>
>>> +
>>> +#include <soc/tegra/fuse.h>
>>> +
>>> +#define SENSOR_CONFIG0				0
>>> +#define SENSOR_CONFIG0_STOP			BIT(0)
>>> +#define SENSOR_CONFIG0_TALL_SHIFT		8
>>> +#define SENSOR_CONFIG0_TCALC_OVER		BIT(4)
>>> +#define SENSOR_CONFIG0_OVER			BIT(3)
>>> +#define SENSOR_CONFIG0_CPTR_OVER		BIT(2)
>>> +
>>> +#define SENSOR_CONFIG1				4
>>> +#define SENSOR_CONFIG1_TSAMPLE_SHIFT		0
>>> +#define SENSOR_CONFIG1_TIDDQ_EN_SHIFT		15
>>> +#define SENSOR_CONFIG1_TEN_COUNT_SHIFT		24
>>> +#define SENSOR_CONFIG1_TEMP_ENABLE		BIT(31)
>>> +
>>> +#define SENSOR_CONFIG2				8
>>> +#define SENSOR_CONFIG2_THERMA_SHIFT		16
>>> +#define SENSOR_CONFIG2_THERMB_SHIFT		0
>>> +
>>> +#define SENSOR_PDIV				0x1c0
>>> +#define SENSOR_PDIV_T124			0x8888
>>> +#define SENSOR_HOTSPOT_OFF			0x1c4
>>> +#define SENSOR_HOTSPOT_OFF_T124			0x00060600
>>> +#define SENSOR_TEMP1				0x1c8
>>> +#define SENSOR_TEMP2				0x1cc
>>> +
>>> +#define SENSOR_TEMP_MASK			0xffff
>>> +#define READBACK_VALUE_MASK			0xff00
>>> +#define READBACK_VALUE_SHIFT			8
>>> +#define READBACK_ADD_HALF			BIT(7)
>>> +#define READBACK_NEGATE				BIT(1)
>>> +
>>> +#define FUSE_TSENSOR8_CALIB			0x180
>>> +#define FUSE_SPARE_REALIGNMENT_REG_0		0x1fc
>>> +
>>> +#define FUSE_TSENSOR_CALIB_CP_TS_BASE_MASK	0x1fff
>>> +#define FUSE_TSENSOR_CALIB_FT_TS_BASE_MASK	(0x1fff << 13)
>>> +#define FUSE_TSENSOR_CALIB_FT_TS_BASE_SHIFT	13
>>> +
>>> +#define FUSE_TSENSOR8_CALIB_CP_TS_BASE_MASK	0x3ff
>>> +#define FUSE_TSENSOR8_CALIB_FT_TS_BASE_MASK	(0x7ff << 10)
>>> +#define FUSE_TSENSOR8_CALIB_FT_TS_BASE_SHIFT	10
>>> +
>>> +#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_CP_MASK 0x3f
>>> +#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_MASK (0x1f << 21)
>>> +#define FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_SHIFT 21
>>> +
>>> +#define NOMINAL_CALIB_FT_T124			105
>>> +#define NOMINAL_CALIB_CP_T124			25
>>> +
>>> +struct tegra_tsensor_configuration {
>>> +	u32 tall, tsample, tiddq_en, ten_count, pdiv, tsample_ate, pdiv_ate;
>>> +};
>>> +
>>> +struct tegra_tsensor {
>>> +	const struct tegra_tsensor_configuration *config;
>>> +	u32 base, calib_fuse_offset;
>>> +	/* Correction values used to modify values read from calibration fuses */
>>> +	s32 fuse_corr_alpha, fuse_corr_beta;
>>> +};
>>> +
>>> +struct tegra_thermctl_zone {
>>> +	void __iomem *reg;
>>> +	unsigned int shift;
>>> +};
>>> +
>>> +static const struct tegra_tsensor_configuration t124_tsensor_config = {
>>> +	.tall = 16300,
>>> +	.tsample = 120,
>>> +	.tiddq_en = 1,
>>> +	.ten_count = 1,
>>> +	.pdiv = 8,
>>> +	.tsample_ate = 480,
>>> +	.pdiv_ate = 8
>>> +};
>>> +
>>> +static const struct tegra_tsensor t124_tsensors[] = {
>>> +	{
>>> +		.config = &t124_tsensor_config,
>>> +		.base = 0xc0,
>>> +		.calib_fuse_offset = 0x098,
>>> +		.fuse_corr_alpha = 1135400,
>>> +		.fuse_corr_beta = -6266900,
>>> +	},
>>> +	{
>>> +		.config = &t124_tsensor_config,
>>> +		.base = 0xe0,
>>> +		.calib_fuse_offset = 0x084,
>>> +		.fuse_corr_alpha = 1122220,
>>> +		.fuse_corr_beta = -5700700,
>>> +	},
>>> +	{
>>> +		.config = &t124_tsensor_config,
>>> +		.base = 0x100,
>>> +		.calib_fuse_offset = 0x088,
>>> +		.fuse_corr_alpha = 1127000,
>>> +		.fuse_corr_beta = -6768200,
>>> +	},
>>> +	{
>>> +		.config = &t124_tsensor_config,
>>> +		.base = 0x120,
>>> +		.calib_fuse_offset = 0x12c,
>>> +		.fuse_corr_alpha = 1110900,
>>> +		.fuse_corr_beta = -6232000,
>>> +	},
>>> +	{
>>> +		.config = &t124_tsensor_config,
>>> +		.base = 0x140,
>>> +		.calib_fuse_offset = 0x158,
>>> +		.fuse_corr_alpha = 1122300,
>>> +		.fuse_corr_beta = -5936400,
>>> +	},
>>> +	{
>>> +		.config = &t124_tsensor_config,
>>> +		.base = 0x160,
>>> +		.calib_fuse_offset = 0x15c,
>>> +		.fuse_corr_alpha = 1145700,
>>> +		.fuse_corr_beta = -7124600,
>>> +	},
>>> +	{
>>> +		.config = &t124_tsensor_config,
>>> +		.base = 0x180,
>>> +		.calib_fuse_offset = 0x154,
>>> +		.fuse_corr_alpha = 1120100,
>>> +		.fuse_corr_beta = -6000500,
>>> +	},
>>> +	{
>>> +		.config = &t124_tsensor_config,
>>> +		.base = 0x1a0,
>>> +		.calib_fuse_offset = 0x160,
>>> +		.fuse_corr_alpha = 1106500,
>>> +		.fuse_corr_beta = -6729300,
>>> +	},
>>> +};
>>> +
>>> +struct tegra_soctherm {
>>> +	struct reset_control *reset;
>>> +	struct clk *clock_tsensor;
>>> +	struct clk *clock_soctherm;
>>> +	void __iomem *regs;
>>> +
>>> +	struct thermal_zone_device *thermctl_tzs[4];
>>> +};
>>> +
>>> +struct tsensor_shared_calibration {
>>> +	u32 base_cp, base_ft;
>>> +	u32 actual_temp_cp, actual_temp_ft;
>>> +};
>>> +
>>> +static int calculate_shared_calibration(struct tsensor_shared_calibration *r)
>>> +{
>>> +	u32 val, shifted_cp, shifted_ft;
>>> +	int err;
>>> +
>>> +	err = tegra_fuse_readl(FUSE_TSENSOR8_CALIB, &val);
>>> +	if (err)
>>> +		return err;
>>> +	r->base_cp = val & FUSE_TSENSOR8_CALIB_CP_TS_BASE_MASK;
>>> +	r->base_ft = (val & FUSE_TSENSOR8_CALIB_FT_TS_BASE_MASK)
>>> +		>> FUSE_TSENSOR8_CALIB_FT_TS_BASE_SHIFT;
>>> +	val = ((val & FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_MASK)
>>> +		>> FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_SHIFT);
>>> +	shifted_ft = sign_extend32(val, 4);
>>> +
>>> +	err = tegra_fuse_readl(FUSE_SPARE_REALIGNMENT_REG_0, &val);
>>> +	if (err)
>>> +		return err;
>>> +	shifted_cp = sign_extend32(val, 5);
>>> +
>>> +	r->actual_temp_cp = 2 * NOMINAL_CALIB_CP_T124 + shifted_cp;
>>> +	r->actual_temp_ft = 2 * NOMINAL_CALIB_FT_T124 + shifted_ft;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static s64 div64_s64_precise(s64 a, s64 b)
>>> +{
>>> +	s64 r, al;
>>> +
>>> +	/* Scale up for increased precision division */
>>> +	al = a << 16;
>>> +
>>> +	r = div64_s64(al * 2 + 1, 2 * b);
>>> +	return r >> 16;
>>> +}
>>> +
>>> +static int
>>> +calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
>>> +			      const struct tsensor_shared_calibration *shared,
>>> +			      u32 *calib)
>>> +{
>>> +	u32 val;
>>> +	s32 actual_tsensor_ft, actual_tsensor_cp, delta_sens, delta_temp,
>>> +	    mult, div;
>>> +	s16 therma, thermb;
>>> +	s64 tmp;
>>> +	int err;
>>> +
>>> +	err = tegra_fuse_readl(sensor->calib_fuse_offset, &val);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	actual_tsensor_cp = (shared->base_cp * 64) + sign_extend32(val, 12);
>>> +	val = (val & FUSE_TSENSOR_CALIB_FT_TS_BASE_MASK)
>>> +		>> FUSE_TSENSOR_CALIB_FT_TS_BASE_SHIFT;
>>> +	actual_tsensor_ft = (shared->base_ft * 32) + sign_extend32(val, 12);
>>> +
>>> +	delta_sens = actual_tsensor_ft - actual_tsensor_cp;
>>> +	delta_temp = shared->actual_temp_ft - shared->actual_temp_cp;
>>> +
>>> +	mult = sensor->config->pdiv * sensor->config->tsample_ate;
>>> +	div = sensor->config->tsample * sensor->config->pdiv_ate;
>>> +
>>> +	therma = div64_s64_precise((s64) delta_temp * (1LL << 13) * mult,
>>> +				   (s64) delta_sens * div);
>>> +
>>> +	tmp = (s64)actual_tsensor_ft * shared->actual_temp_cp -
>>> +	      (s64)actual_tsensor_cp * shared->actual_temp_ft;
>>> +	thermb = div64_s64_precise(tmp, (s64)delta_sens);
>>> +
>>> +	therma = div64_s64_precise((s64)therma * sensor->fuse_corr_alpha,
>>> +				   (s64)1000000LL);
>>> +	thermb = div64_s64_precise((s64)thermb * sensor->fuse_corr_alpha +
>>> +				   sensor->fuse_corr_beta, (s64)1000000LL);
>>> +
>>> +	*calib = ((u16)therma << SENSOR_CONFIG2_THERMA_SHIFT) |
>>> +		 ((u16)thermb << SENSOR_CONFIG2_THERMB_SHIFT);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int enable_tsensor(struct tegra_soctherm *tegra,
>>> +			  const struct tegra_tsensor *sensor,
>>> +			  const struct tsensor_shared_calibration *shared)
>>> +{
>>> +	void __iomem *base = tegra->regs + sensor->base;
>>> +	unsigned int val;
>>> +	u32 calib;
>>> +	int err;
>>> +
>>> +	err = calculate_tsensor_calibration(sensor, shared, &calib);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	val = sensor->config->tall << SENSOR_CONFIG0_TALL_SHIFT;
>>> +	writel(val, base + SENSOR_CONFIG0);
>>> +
>>> +	val  = (sensor->config->tsample - 1) << SENSOR_CONFIG1_TSAMPLE_SHIFT;
>>> +	val |= sensor->config->tiddq_en << SENSOR_CONFIG1_TIDDQ_EN_SHIFT;
>>> +	val |= sensor->config->ten_count << SENSOR_CONFIG1_TEN_COUNT_SHIFT;
>>> +	val |= SENSOR_CONFIG1_TEMP_ENABLE;
>>> +	writel(val, base + SENSOR_CONFIG1);
>>> +
>>> +	writel(calib, base + SENSOR_CONFIG2);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +/*
>>> + * Translate from soctherm readback format to millicelsius.
>>> + * The soctherm readback format in bits is as follows:
>>> + *   TTTTTTTT H______N
>>> + * where T's contain the temperature in Celsius,
>>> + * H denotes an addition of 0.5 Celsius and N denotes negation
>>> + * of the final value.
>>> + */
>>> +static long translate_temp(u16 val)
>>> +{
>>> +	long t;
>>> +
>>> +	t = ((val & READBACK_VALUE_MASK) >> READBACK_VALUE_SHIFT) * 1000;
>>> +	if (val & READBACK_ADD_HALF)
>>> +		t += 500;
>>> +	if (val & READBACK_NEGATE)
>>> +		t *= -1;
>>> +
>>> +	return t;
>>> +}
>>> +
>>> +static int tegra_thermctl_get_temp(void *data, long *out_temp)
>>> +{
>>> +	struct tegra_thermctl_zone *zone = data;
>>> +	u32 val;
>>> +
>>> +	val = (readl(zone->reg) >> zone->shift) & SENSOR_TEMP_MASK;
>>> +	*out_temp = translate_temp(val);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct of_device_id tegra_soctherm_of_match[] = {
>>> +	{ .compatible = "nvidia,tegra124-soctherm" },
>>> +	{ },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, tegra_soctherm_of_match);
>>> +
>>> +struct thermctl_zone_desc {
>>> +	unsigned int offset;
>>> +	unsigned int shift;
>>> +};
>>> +
>>> +static const struct thermctl_zone_desc t124_thermctl_temp_zones[] = {
>>> +	{ SENSOR_TEMP1, 16 },
>>> +	{ SENSOR_TEMP2, 16 },
>>> +	{ SENSOR_TEMP1, 0 },
>>> +	{ SENSOR_TEMP2, 0 }
>>> +};
>>> +
>>> +static int tegra_soctherm_probe(struct platform_device *pdev)
>>> +{
>>> +	struct tegra_soctherm *tegra;
>>> +	struct thermal_zone_device *tz;
>>> +	struct tsensor_shared_calibration shared_calib;
>>> +	struct resource *res;
>>> +	unsigned int i;
>>> +	int err;
>>> +
>>> +	const struct tegra_tsensor *tsensors = t124_tsensors;
>>> +
>>> +	tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
>>> +	if (!tegra)
>>> +		return -ENOMEM;
>>> +
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	tegra->regs = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(tegra->regs))
>>> +		return PTR_ERR(tegra->regs);
>>> +
>>> +	tegra->reset = devm_reset_control_get(&pdev->dev, "soctherm");
>>> +	if (IS_ERR(tegra->reset)) {
>>> +		dev_err(&pdev->dev, "can't get soctherm reset\n");
>>> +		return PTR_ERR(tegra->reset);
>>> +	}
>>> +
>>> +	tegra->clock_tsensor = devm_clk_get(&pdev->dev, "tsensor");
>>> +	if (IS_ERR(tegra->clock_tsensor)) {
>>> +		dev_err(&pdev->dev, "can't get tsensor clock\n");
>>> +		return PTR_ERR(tegra->clock_tsensor);
>>> +	}
>>> +
>>> +	tegra->clock_soctherm = devm_clk_get(&pdev->dev, "soctherm");
>>> +	if (IS_ERR(tegra->clock_soctherm)) {
>>> +		dev_err(&pdev->dev, "can't get soctherm clock\n");
>>> +		return PTR_ERR(tegra->clock_soctherm);
>>> +	}
>>> +
>>> +	reset_control_assert(tegra->reset);
>>> +
>>> +	err = clk_prepare_enable(tegra->clock_soctherm);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	err = clk_prepare_enable(tegra->clock_tsensor);
>>> +	if (err) {
>>> +		clk_disable_unprepare(tegra->clock_soctherm);
>>> +		return err;
>>> +	}
>>> +
>>> +	reset_control_deassert(tegra->reset);
>>> +
>>> +	/* Initialize raw sensors */
>>> +
>>> +	err = calculate_shared_calibration(&shared_calib);
>>> +	if (err)
>>> +		goto disable_clocks;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(t124_tsensors); ++i) {
>>> +		err = enable_tsensor(tegra, tsensors + i, &shared_calib);
>>> +		if (err)
>>> +			goto disable_clocks;
>>> +	}
>>> +
>>> +	writel(SENSOR_PDIV_T124, tegra->regs + SENSOR_PDIV);
>>> +	writel(SENSOR_HOTSPOT_OFF_T124, tegra->regs + SENSOR_HOTSPOT_OFF);
>>> +
>>> +	/* Initialize thermctl sensors */
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(tegra->thermctl_tzs); ++i) {
>>> +		struct tegra_thermctl_zone *zone =
>>> +			devm_kzalloc(&pdev->dev, sizeof(*zone), GFP_KERNEL);
>>> +		if (!zone) {
>>> +			err = -ENOMEM;
>>> +			goto unregister_tzs;
>>> +		}
>>> +
>>> +		zone->reg = tegra->regs + t124_thermctl_temp_zones[i].offset;
>>> +		zone->shift = t124_thermctl_temp_zones[i].shift;
>>> +
>>> +		tz = thermal_zone_of_sensor_register(&pdev->dev, i, zone,
>>> +						     tegra_thermctl_get_temp,
>>> +						     NULL);
>>> +		if (IS_ERR(tz)) {
>>> +			err = PTR_ERR(tz);
>>> +			dev_err(&pdev->dev, "failed to register sensor: %d\n",
>>> +				err);
>>> +			goto unregister_tzs;
>>> +		}
>>> +
>>> +		tegra->thermctl_tzs[i] = tz;
>>> +	}
>>> +
>>> +	return 0;
>>> +
>>> +unregister_tzs:
>>> +	while (i--)
>>> +		thermal_zone_of_sensor_unregister(&pdev->dev,
>>> +						  tegra->thermctl_tzs[i]);
>>> +
>>> +disable_clocks:
>>> +	clk_disable_unprepare(tegra->clock_tsensor);
>>> +	clk_disable_unprepare(tegra->clock_soctherm);
>>> +
>>> +	return err;
>>> +}
>>> +
>>> +static int tegra_soctherm_remove(struct platform_device *pdev)
>>> +{
>>> +	struct tegra_soctherm *tegra = platform_get_drvdata(pdev);
>>> +	unsigned int i;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(tegra->thermctl_tzs); ++i) {
>>> +		thermal_zone_of_sensor_unregister(&pdev->dev,
>>> +						  tegra->thermctl_tzs[i]);
>>> +	}
>>> +
>>> +	clk_disable_unprepare(tegra->clock_tsensor);
>>> +	clk_disable_unprepare(tegra->clock_soctherm);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct platform_driver tegra_soctherm_driver = {
>>> +	.probe = tegra_soctherm_probe,
>>> +	.remove = tegra_soctherm_remove,
>>> +	.driver = {
>>> +		.name = "tegra-soctherm",
>>> +		.of_match_table = tegra_soctherm_of_match,
>>> +	},
>>> +};
>>> +module_platform_driver(tegra_soctherm_driver);
>>> +
>>> +MODULE_AUTHOR("Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
>>> +MODULE_DESCRIPTION("NVIDIA Tegra SOCTHERM thermal management driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>

Re: [PATCH v6 0/4] Tegra124 soctherm driver

[Thierry Reding]

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

On Fri, Sep 26, 2014 at 12:43:09PM +0300, Mikko Perttunen wrote:
> Hi,
> 
> this series adds support for the thermal monitoring features of the
> soctherm unit on the Tegra124 SoC. 
> 
> The branch is also available in my github repo,
>   git://github.com/cyndis/linux.git soctherm-v6
> 
> Thanks,
> Mikko
> 
> Mikko Perttunen (4):
>   of: Add bindings for nvidia,tegra124-soctherm
>   ARM: tegra: Add soctherm and thermal zones to Tegra124 device tree
>   ARM: tegra: Add thermal trip points for Jetson TK1
>   thermal: Add Tegra SOCTHERM thermal management driver
> 
>  .../devicetree/bindings/thermal/tegra-soctherm.txt |  53 +++
>  arch/arm/boot/dts/tegra124-jetson-tk1.dts          |  44 ++
>  arch/arm/boot/dts/tegra124.dtsi                    |  47 ++
>  drivers/thermal/Kconfig                            |  10 +
>  drivers/thermal/Makefile                           |   1 +
>  drivers/thermal/tegra_soctherm.c                   | 471 +++++++++++++++++++++
>  include/dt-bindings/thermal/tegra124-soctherm.h    |  13 +
>  7 files changed, 639 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
>  create mode 100644 drivers/thermal/tegra_soctherm.c
>  create mode 100644 include/dt-bindings/thermal/tegra124-soctherm.h

One thing that I've wanted to start doing for a while now is request
patch submissions like this to come accompanied with a way on how to
test them. Ideally this would be in a scripted way that can test for
success programatically, but it doesn't necessarily have to be if it
turns out too difficult or impractical to do.

The goal is to eventually come up with a test suite that can run the
majority of test cases automatically to make it easy to test for any
regressions. And even if tests can't be run automatically it'd still
be an advantage to have them all collected in some repository, since
it saves a lot of typing and time to run tests, and it will give us
a standard set of tests that everybody can verify changes against.

I realize that it's somewhat unfair to start requesting this from you
now, but we've got to start somewhere. Could you give a short summary
of how you test this? What are the interfaces that the kernel exposes
for these thermal drivers?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 0/4] Tegra124 soctherm driver

[Mikko Perttunen]

On 09/26/2014 01:19 PM, Thierry Reding wrote:
> On Fri, Sep 26, 2014 at 12:43:09PM +0300, Mikko Perttunen wrote:
>> Hi,
>>
>> this series adds support for the thermal monitoring features of the
>> soctherm unit on the Tegra124 SoC.
>>
>> The branch is also available in my github repo,
>>    git://github.com/cyndis/linux.git soctherm-v6
>>
>> Thanks,
>> Mikko
>>
>> Mikko Perttunen (4):
>>    of: Add bindings for nvidia,tegra124-soctherm
>>    ARM: tegra: Add soctherm and thermal zones to Tegra124 device tree
>>    ARM: tegra: Add thermal trip points for Jetson TK1
>>    thermal: Add Tegra SOCTHERM thermal management driver
>>
>>   .../devicetree/bindings/thermal/tegra-soctherm.txt |  53 +++
>>   arch/arm/boot/dts/tegra124-jetson-tk1.dts          |  44 ++
>>   arch/arm/boot/dts/tegra124.dtsi                    |  47 ++
>>   drivers/thermal/Kconfig                            |  10 +
>>   drivers/thermal/Makefile                           |   1 +
>>   drivers/thermal/tegra_soctherm.c                   | 471 +++++++++++++++++++++
>>   include/dt-bindings/thermal/tegra124-soctherm.h    |  13 +
>>   7 files changed, 639 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
>>   create mode 100644 drivers/thermal/tegra_soctherm.c
>>   create mode 100644 include/dt-bindings/thermal/tegra124-soctherm.h
>
> One thing that I've wanted to start doing for a while now is request
> patch submissions like this to come accompanied with a way on how to
> test them. Ideally this would be in a scripted way that can test for
> success programatically, but it doesn't necessarily have to be if it
> turns out too difficult or impractical to do.

Indeed, that would be very useful.

>
> The goal is to eventually come up with a test suite that can run the
> majority of test cases automatically to make it easy to test for any
> regressions. And even if tests can't be run automatically it'd still
> be an advantage to have them all collected in some repository, since
> it saves a lot of typing and time to run tests, and it will give us
> a standard set of tests that everybody can verify changes against.
>
> I realize that it's somewhat unfair to start requesting this from you
> now, but we've got to start somewhere. Could you give a short summary
> of how you test this? What are the interfaces that the kernel exposes
> for these thermal drivers?

You need to enable the driver in Device Drivers -> Generic Thermal sysfs 
driver -> Tegra SOCTHERM thermal management. Then, you should see 
directories appear in /sys/class/thermal. You can also use the `tmon' 
tool included in the kernel tree to quickly see values; that's what I 
use for testing.

>
> Thierry
>

Mikko

Re: [PATCH v6 0/4] Tegra124 soctherm driver

[Thierry Reding]

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

On Fri, Sep 26, 2014 at 01:22:52PM +0300, Mikko Perttunen wrote:
> On 09/26/2014 01:19 PM, Thierry Reding wrote:
> >On Fri, Sep 26, 2014 at 12:43:09PM +0300, Mikko Perttunen wrote:
> >>Hi,
> >>
> >>this series adds support for the thermal monitoring features of the
> >>soctherm unit on the Tegra124 SoC.
> >>
> >>The branch is also available in my github repo,
> >>   git://github.com/cyndis/linux.git soctherm-v6
> >>
> >>Thanks,
> >>Mikko
> >>
> >>Mikko Perttunen (4):
> >>   of: Add bindings for nvidia,tegra124-soctherm
> >>   ARM: tegra: Add soctherm and thermal zones to Tegra124 device tree
> >>   ARM: tegra: Add thermal trip points for Jetson TK1
> >>   thermal: Add Tegra SOCTHERM thermal management driver
> >>
> >>  .../devicetree/bindings/thermal/tegra-soctherm.txt |  53 +++
> >>  arch/arm/boot/dts/tegra124-jetson-tk1.dts          |  44 ++
> >>  arch/arm/boot/dts/tegra124.dtsi                    |  47 ++
> >>  drivers/thermal/Kconfig                            |  10 +
> >>  drivers/thermal/Makefile                           |   1 +
> >>  drivers/thermal/tegra_soctherm.c                   | 471 +++++++++++++++++++++
> >>  include/dt-bindings/thermal/tegra124-soctherm.h    |  13 +
> >>  7 files changed, 639 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
> >>  create mode 100644 drivers/thermal/tegra_soctherm.c
> >>  create mode 100644 include/dt-bindings/thermal/tegra124-soctherm.h
> >
> >One thing that I've wanted to start doing for a while now is request
> >patch submissions like this to come accompanied with a way on how to
> >test them. Ideally this would be in a scripted way that can test for
> >success programatically, but it doesn't necessarily have to be if it
> >turns out too difficult or impractical to do.
> 
> Indeed, that would be very useful.
> 
> >
> >The goal is to eventually come up with a test suite that can run the
> >majority of test cases automatically to make it easy to test for any
> >regressions. And even if tests can't be run automatically it'd still
> >be an advantage to have them all collected in some repository, since
> >it saves a lot of typing and time to run tests, and it will give us
> >a standard set of tests that everybody can verify changes against.
> >
> >I realize that it's somewhat unfair to start requesting this from you
> >now, but we've got to start somewhere. Could you give a short summary
> >of how you test this? What are the interfaces that the kernel exposes
> >for these thermal drivers?
> 
> You need to enable the driver in Device Drivers -> Generic Thermal sysfs
> driver -> Tegra SOCTHERM thermal management. Then, you should see
> directories appear in /sys/class/thermal. You can also use the `tmon' tool
> included in the kernel tree to quickly see values; that's what I use for
> testing.

Okay. So what are expected values for these temperatures? It's going to
be pretty much impossible to say what the correct value is on a given
board at any time, but perhaps a "test" could consist of checking that
all temperatures are within a reasonable range.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 0/4] Tegra124 soctherm driver

[Mikko Perttunen]

On 09/26/2014 02:48 PM, Thierry Reding wrote:
> On Fri, Sep 26, 2014 at 01:22:52PM +0300, Mikko Perttunen wrote:
>> On 09/26/2014 01:19 PM, Thierry Reding wrote:
>>> On Fri, Sep 26, 2014 at 12:43:09PM +0300, Mikko Perttunen wrote:
>>>> Hi,
>>>>
>>>> this series adds support for the thermal monitoring features of the
>>>> soctherm unit on the Tegra124 SoC.
>>>>
>>>> The branch is also available in my github repo,
>>>>    git://github.com/cyndis/linux.git soctherm-v6
>>>>
>>>> Thanks,
>>>> Mikko
>>>>
>>>> Mikko Perttunen (4):
>>>>    of: Add bindings for nvidia,tegra124-soctherm
>>>>    ARM: tegra: Add soctherm and thermal zones to Tegra124 device tree
>>>>    ARM: tegra: Add thermal trip points for Jetson TK1
>>>>    thermal: Add Tegra SOCTHERM thermal management driver
>>>>
>>>>   .../devicetree/bindings/thermal/tegra-soctherm.txt |  53 +++
>>>>   arch/arm/boot/dts/tegra124-jetson-tk1.dts          |  44 ++
>>>>   arch/arm/boot/dts/tegra124.dtsi                    |  47 ++
>>>>   drivers/thermal/Kconfig                            |  10 +
>>>>   drivers/thermal/Makefile                           |   1 +
>>>>   drivers/thermal/tegra_soctherm.c                   | 471 +++++++++++++++++++++
>>>>   include/dt-bindings/thermal/tegra124-soctherm.h    |  13 +
>>>>   7 files changed, 639 insertions(+)
>>>>   create mode 100644 Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
>>>>   create mode 100644 drivers/thermal/tegra_soctherm.c
>>>>   create mode 100644 include/dt-bindings/thermal/tegra124-soctherm.h
>>>
>>> One thing that I've wanted to start doing for a while now is request
>>> patch submissions like this to come accompanied with a way on how to
>>> test them. Ideally this would be in a scripted way that can test for
>>> success programatically, but it doesn't necessarily have to be if it
>>> turns out too difficult or impractical to do.
>>
>> Indeed, that would be very useful.
>>
>>>
>>> The goal is to eventually come up with a test suite that can run the
>>> majority of test cases automatically to make it easy to test for any
>>> regressions. And even if tests can't be run automatically it'd still
>>> be an advantage to have them all collected in some repository, since
>>> it saves a lot of typing and time to run tests, and it will give us
>>> a standard set of tests that everybody can verify changes against.
>>>
>>> I realize that it's somewhat unfair to start requesting this from you
>>> now, but we've got to start somewhere. Could you give a short summary
>>> of how you test this? What are the interfaces that the kernel exposes
>>> for these thermal drivers?
>>
>> You need to enable the driver in Device Drivers -> Generic Thermal sysfs
>> driver -> Tegra SOCTHERM thermal management. Then, you should see
>> directories appear in /sys/class/thermal. You can also use the `tmon' tool
>> included in the kernel tree to quickly see values; that's what I use for
>> testing.
>
> Okay. So what are expected values for these temperatures? It's going to
> be pretty much impossible to say what the correct value is on a given
> board at any time, but perhaps a "test" could consist of checking that
> all temperatures are within a reasonable range.

On Jetson TK1, at least without the CL-DVFS series, I get around 32 
Celsius. If you want to account for cpu/gpufreq then I guess something 
like 25-70 would be a good range.

>
> Thierry
>

Mikko

Re: [PATCH v6 0/4] Tegra124 soctherm driver

[Thierry Reding]

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

On Fri, Sep 26, 2014 at 03:00:11PM +0300, Mikko Perttunen wrote:
> On 09/26/2014 02:48 PM, Thierry Reding wrote:
> >On Fri, Sep 26, 2014 at 01:22:52PM +0300, Mikko Perttunen wrote:
> >>On 09/26/2014 01:19 PM, Thierry Reding wrote:
> >>>On Fri, Sep 26, 2014 at 12:43:09PM +0300, Mikko Perttunen wrote:
> >>>>Hi,
> >>>>
> >>>>this series adds support for the thermal monitoring features of the
> >>>>soctherm unit on the Tegra124 SoC.
> >>>>
> >>>>The branch is also available in my github repo,
> >>>>   git://github.com/cyndis/linux.git soctherm-v6
> >>>>
> >>>>Thanks,
> >>>>Mikko
> >>>>
> >>>>Mikko Perttunen (4):
> >>>>   of: Add bindings for nvidia,tegra124-soctherm
> >>>>   ARM: tegra: Add soctherm and thermal zones to Tegra124 device tree
> >>>>   ARM: tegra: Add thermal trip points for Jetson TK1
> >>>>   thermal: Add Tegra SOCTHERM thermal management driver
> >>>>
> >>>>  .../devicetree/bindings/thermal/tegra-soctherm.txt |  53 +++
> >>>>  arch/arm/boot/dts/tegra124-jetson-tk1.dts          |  44 ++
> >>>>  arch/arm/boot/dts/tegra124.dtsi                    |  47 ++
> >>>>  drivers/thermal/Kconfig                            |  10 +
> >>>>  drivers/thermal/Makefile                           |   1 +
> >>>>  drivers/thermal/tegra_soctherm.c                   | 471 +++++++++++++++++++++
> >>>>  include/dt-bindings/thermal/tegra124-soctherm.h    |  13 +
> >>>>  7 files changed, 639 insertions(+)
> >>>>  create mode 100644 Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
> >>>>  create mode 100644 drivers/thermal/tegra_soctherm.c
> >>>>  create mode 100644 include/dt-bindings/thermal/tegra124-soctherm.h
> >>>
> >>>One thing that I've wanted to start doing for a while now is request
> >>>patch submissions like this to come accompanied with a way on how to
> >>>test them. Ideally this would be in a scripted way that can test for
> >>>success programatically, but it doesn't necessarily have to be if it
> >>>turns out too difficult or impractical to do.
> >>
> >>Indeed, that would be very useful.
> >>
> >>>
> >>>The goal is to eventually come up with a test suite that can run the
> >>>majority of test cases automatically to make it easy to test for any
> >>>regressions. And even if tests can't be run automatically it'd still
> >>>be an advantage to have them all collected in some repository, since
> >>>it saves a lot of typing and time to run tests, and it will give us
> >>>a standard set of tests that everybody can verify changes against.
> >>>
> >>>I realize that it's somewhat unfair to start requesting this from you
> >>>now, but we've got to start somewhere. Could you give a short summary
> >>>of how you test this? What are the interfaces that the kernel exposes
> >>>for these thermal drivers?
> >>
> >>You need to enable the driver in Device Drivers -> Generic Thermal sysfs
> >>driver -> Tegra SOCTHERM thermal management. Then, you should see
> >>directories appear in /sys/class/thermal. You can also use the `tmon' tool
> >>included in the kernel tree to quickly see values; that's what I use for
> >>testing.
> >
> >Okay. So what are expected values for these temperatures? It's going to
> >be pretty much impossible to say what the correct value is on a given
> >board at any time, but perhaps a "test" could consist of checking that
> >all temperatures are within a reasonable range.
> 
> On Jetson TK1, at least without the CL-DVFS series, I get around 32 Celsius.
> If you want to account for cpu/gpufreq then I guess something like 25-70
> would be a good range.

Okay, thanks. Can you remind me how this relates to the thermal tripping
support?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 0/4] Tegra124 soctherm driver

[Mikko Perttunen]

On 09/26/2014 03:05 PM, Thierry Reding wrote:
> On Fri, Sep 26, 2014 at 03:00:11PM +0300, Mikko Perttunen wrote:
>> On 09/26/2014 02:48 PM, Thierry Reding wrote:
>>> On Fri, Sep 26, 2014 at 01:22:52PM +0300, Mikko Perttunen wrote:
>>>> On 09/26/2014 01:19 PM, Thierry Reding wrote:
>>>>> On Fri, Sep 26, 2014 at 12:43:09PM +0300, Mikko Perttunen wrote:
>>>>>> Hi,
>>>>>>
>>>>>> this series adds support for the thermal monitoring features of the
>>>>>> soctherm unit on the Tegra124 SoC.
>>>>>>
>>>>>> The branch is also available in my github repo,
>>>>>>    git://github.com/cyndis/linux.git soctherm-v6
>>>>>>
>>>>>> Thanks,
>>>>>> Mikko
>>>>>>
>>>>>> Mikko Perttunen (4):
>>>>>>    of: Add bindings for nvidia,tegra124-soctherm
>>>>>>    ARM: tegra: Add soctherm and thermal zones to Tegra124 device tree
>>>>>>    ARM: tegra: Add thermal trip points for Jetson TK1
>>>>>>    thermal: Add Tegra SOCTHERM thermal management driver
>>>>>>
>>>>>>   .../devicetree/bindings/thermal/tegra-soctherm.txt |  53 +++
>>>>>>   arch/arm/boot/dts/tegra124-jetson-tk1.dts          |  44 ++
>>>>>>   arch/arm/boot/dts/tegra124.dtsi                    |  47 ++
>>>>>>   drivers/thermal/Kconfig                            |  10 +
>>>>>>   drivers/thermal/Makefile                           |   1 +
>>>>>>   drivers/thermal/tegra_soctherm.c                   | 471 +++++++++++++++++++++
>>>>>>   include/dt-bindings/thermal/tegra124-soctherm.h    |  13 +
>>>>>>   7 files changed, 639 insertions(+)
>>>>>>   create mode 100644 Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
>>>>>>   create mode 100644 drivers/thermal/tegra_soctherm.c
>>>>>>   create mode 100644 include/dt-bindings/thermal/tegra124-soctherm.h
>>>>>
>>>>> One thing that I've wanted to start doing for a while now is request
>>>>> patch submissions like this to come accompanied with a way on how to
>>>>> test them. Ideally this would be in a scripted way that can test for
>>>>> success programatically, but it doesn't necessarily have to be if it
>>>>> turns out too difficult or impractical to do.
>>>>
>>>> Indeed, that would be very useful.
>>>>
>>>>>
>>>>> The goal is to eventually come up with a test suite that can run the
>>>>> majority of test cases automatically to make it easy to test for any
>>>>> regressions. And even if tests can't be run automatically it'd still
>>>>> be an advantage to have them all collected in some repository, since
>>>>> it saves a lot of typing and time to run tests, and it will give us
>>>>> a standard set of tests that everybody can verify changes against.
>>>>>
>>>>> I realize that it's somewhat unfair to start requesting this from you
>>>>> now, but we've got to start somewhere. Could you give a short summary
>>>>> of how you test this? What are the interfaces that the kernel exposes
>>>>> for these thermal drivers?
>>>>
>>>> You need to enable the driver in Device Drivers -> Generic Thermal sysfs
>>>> driver -> Tegra SOCTHERM thermal management. Then, you should see
>>>> directories appear in /sys/class/thermal. You can also use the `tmon' tool
>>>> included in the kernel tree to quickly see values; that's what I use for
>>>> testing.
>>>
>>> Okay. So what are expected values for these temperatures? It's going to
>>> be pretty much impossible to say what the correct value is on a given
>>> board at any time, but perhaps a "test" could consist of checking that
>>> all temperatures are within a reasonable range.
>>
>> On Jetson TK1, at least without the CL-DVFS series, I get around 32 Celsius.
>> If you want to account for cpu/gpufreq then I guess something like 25-70
>> would be a good range.
>
> Okay, thanks. Can you remind me how this relates to the thermal tripping
> support?
>
> Thierry
>

I'm not sure what you mean, but hardware thermal trips are not currently 
supported (except THERMTRIP, the "master" shutdown thermal trip, which 
is supported if this series is combined with the other series I have 
posted. THERMTRIP defaults to shutdown at 105 Celsius.)

Mikko