Temperature sensor driver (tango)

From: Marc Gonzalez <[email protected]>

There are three temperature sensors in SMP8758.

Signed-off-by Marc Gonzalez <[email protected]>

---
 drivers/thermal/Kconfig         |   6 +++
 drivers/thermal/Makefile        |   1 +
 drivers/thermal/tango_thermal.c | 113 ++++++++++++++++++++++++++++++++++++++++

 3 files changed, 120 insertions(+)
 create mode 100644 drivers/thermal/tango_thermal.c

drivers/thermal/Kconfig +0 -0

index 7c92c09be213..b63298276532 100644

drivers/thermal/Kconfig +1 -0

+
 config TEGRA_SOCTHERM
 	tristate "Tegra SOCTHERM thermal management"
 	depends on ARCH_TEGRA

drivers/thermal/Makefile +1 -0

index cfae6a654793..8bfea2b7e722 100644
@@ -35,6 +35,7 @@ obj-y				+= samsung/
 obj-$(CONFIG_DOVE_THERMAL)  	+= dove_thermal.o
 obj-$(CONFIG_DB8500_THERMAL)	+= db8500_thermal.o
 obj-$(CONFIG_ARMADA_THERMAL)	+= armada_thermal.o
+obj-$(CONFIG_TANGO_THERMAL)	+= tango_thermal.o
 obj-$(CONFIG_IMX_THERMAL)	+= imx_thermal.o
 obj-$(CONFIG_DB8500_CPUFREQ_COOLING)	+= db8500_cpufreq_cooling.o
 obj-$(CONFIG_INTEL_POWERCLAMP)	+= intel_powerclamp.o

drivers/thermal/tango_thermal.c new file +21 -0

new file mode 100644
index 000000000000..eeccb6b0894e
@@ -0,0 +1,113 @@
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/thermal.h>
+#include <linux/platform_device.h>
+
+#define TEMPSI_CMD	0
+#define TEMPSI_RES	4
+#define TEMPSI_CFG	8
+
+#define CMD_OFF		0
+#define CMD_ON		1
+#define CMD_READ	2
+
+#define INDEX_OFFSET	18
+
+#define sensor_idle(x)		(readl_relaxed(x + TEMPSI_CMD) & BIT(7))
+#define temp_above_thresh(x)	(readl_relaxed(x + TEMPSI_RES))
+
+static const u8 temperature[] = {
+	 37,  41,  46,  51,  55,  60,  64,  69,
+	 74,  79,  83,  88,  93,  97, 101, 106,

drivers/thermal/tango_thermal.c new file +32 -0

+		writel_relaxed(i << 8 | CMD_READ, base + TEMPSI_CMD);
+
+		while (!sensor_idle(base))
+			cpu_relax();
+
+		if (!temp_above_thresh(base))
+			break;
+	}
+
+	writel_relaxed(INDEX_OFFSET << 8 | CMD_READ, base + TEMPSI_CMD);
+	*res = temperature[i - INDEX_OFFSET] * 1000;
+	return 0;
+}
+
+static const struct thermal_zone_of_device_ops ops = {
+	.get_temp	= tango_get_temp,
+};
+
+struct tango_thermal_priv {
+	struct thermal_zone_device *zone;
+	void __iomem *base;
+};
+
+static int tango_thermal_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct tango_thermal_priv *priv;
+	struct device *dev = &pdev->dev;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;

drivers/thermal/tango_thermal.c new file +40 -0

+	if (IS_ERR(priv->zone))
+		return PTR_ERR(priv->zone);
+
+	platform_set_drvdata(pdev, priv);
+	return 0;
+}
+
+static int tango_thermal_remove(struct platform_device *pdev)
+{
+	struct tango_thermal_priv *priv = platform_get_drvdata(pdev);
+
+	thermal_zone_of_sensor_unregister(&pdev->dev, priv->zone);
+
+	writel_relaxed(      0, priv->base + TEMPSI_CFG);
+	writel_relaxed(CMD_OFF, priv->base + TEMPSI_CMD);
+
+	return 0;
+}
+
+static const struct of_device_id tango_sensor_ids[] = {
+	{
+		.compatible = "sigma,smp8758-sensor",
+	},
+	{ /* sentinel */ }
+};
+
+static struct platform_driver tango_thermal_driver = {
+	.probe	= tango_thermal_probe,
+	.remove	= tango_thermal_remove,
+	.driver	= {
+		.name		= "tango-thermal",
+		.of_match_table	= tango_sensor_ids,
+	},
+};
+
+module_platform_driver(tango_thermal_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Sigma Designs");
+MODULE_DESCRIPTION("Tango temperature sensor");

Signature · 1 line

2.7.0

Quoted 3 lines · Mason

 drivers/thermal/Kconfig         |   6 +++
 drivers/thermal/Makefile        |   1 +
 drivers/thermal/tango_thermal.c | 113 ++++++++++++++++++++++++++++++++++++++++

Can you please add a device tree binding entry under Documentation/devicetree/bindings/thermal/

drivers/thermal/Kconfig config ARMADA_THERMAL · line 261

Quoted 5 lines

drivers/thermal/Kconfig +5 -0

+config TANGO_THERMAL
+	tristate "Tango4 thermal management"
+	depends on ARCH_TANGO || COMPILE_TEST
+	help
+	  Enable SMP8758 temperature sensor driver.

checkpatch.pl --strict complains about this, could you please improve the description of your drivers help entry?

Also, can you please add a comment on the logic here? Where the 41 constant came from?

drivers/thermal/tango_thermal.c line 73

Quoted 1 line

drivers/thermal/tango_thermal.c new file +1 -0

+	priv->zone = thermal_zone_of_sensor_register(dev, 0, priv->base, &ops);

Why registering only for id 0 when you mentioned this device has three sensors?

From: Marc Gonzalez <[email protected]>

Since the SMP8758, Tango SoCs include several instances of a rudimentary bandgap temperature sensor.

Signed-off-by Marc Gonzalez <[email protected]>

---
 Documentation/devicetree/bindings/thermal/tango-thermal.txt |  17 ++++
 drivers/thermal/Kconfig                                     |   7 ++
 drivers/thermal/Makefile                                    |   1 +
 drivers/thermal/tango_thermal.c                             | 114 +++++++++++++++++++++++++++
 4 files changed, 139 insertions(+)

Documentation/devicetree/bindings/thermal/tango-thermal.txt new file +0 -0

new file mode 100644
index 000000000000..a203f7d60d35

Documentation/devicetree/bindings/thermal/tango-thermal.txt new file +1 -0

+	};

drivers/thermal/Kconfig +0 -0

index c37eedc35a24..b6bf563f05dc 100644

drivers/thermal/tango_thermal.c new file +18 -0

+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/thermal.h>
+#include <linux/platform_device.h>
+
+#define TEMPSI_CMD	0
+#define TEMPSI_RES	4
+#define TEMPSI_CFG	8
+
+#define CMD_OFF		0
+#define CMD_ON		1
+#define CMD_READ	2
+
+#define IDX_MIN		0
+#define IDX_MAX		40
+#define CYCLE_COUNT	5000 /* Time to wait before sampling the result */
+

drivers/thermal/tango_thermal.c new file +9 -0

+	return readl(base + TEMPSI_RES);
+}
+
+static int tango_get_temp(void *arg, int *res)
+{
+	struct tango_thermal_priv *priv = arg;
+	void __iomem *base = priv->base;
+	int idx = priv->thresh_idx;
+

drivers/thermal/tango_thermal.c new file +16 -0

+	return 0;
+}
+
+static const struct thermal_zone_of_device_ops ops = {
+	.get_temp	= tango_get_temp,
+};
+
+static int tango_thermal_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct tango_thermal_priv *priv;
+	struct device *dev = &pdev->dev;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;

drivers/thermal/tango_thermal.c new file +24 -0

+	return 0;
+}
+
+static const struct of_device_id tango_sensor_ids[] = {
+	{
+		.compatible = "sigma,smp8758-thermal",
+	},
+	{ /* sentinel */ }
+};
+
+static struct platform_driver tango_thermal_driver = {
+	.probe	= tango_thermal_probe,
+	.remove	= tango_thermal_remove,
+	.driver	= {
+		.name		= "tango-thermal",
+		.of_match_table	= tango_sensor_ids,
+	},
+};
+
+module_platform_driver(tango_thermal_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Sigma Designs");
+MODULE_DESCRIPTION("Tango temperature sensor");

Signature · 1 line

2.7.4

Again, only minor comments, as follows.

Documentation/devicetree/bindings/thermal/tango-thermal.txt line 4

Quoted 2 lines

Documentation/devicetree/bindings/thermal/tango-thermal.txt new file +2 -0

+The SMP8758 SoC includes 3 instances of this temperature sensor
+(in the CPU, video decoder, and PCIe controller).

Would you be able to add a link to sensor documentation?

Documentation/devicetree/bindings/thermal/tango-thermal.txt line 16

Quoted 4 lines

Documentation/devicetree/bindings/thermal/tango-thermal.txt new file +4 -0

+	cpu_temp: thermal@920100 {
+		compatible = "sigma,smp8758-thermal";
+		#thermal-sensor-cells = <0>;
+		reg = <0x920100 12>;

I believe you have to put #thermal-sensor-cells as last entry.

drivers/thermal/Kconfig config ARMADA_THERMAL · line 268

Quoted 2 lines

drivers/thermal/Kconfig +2 -0

+	  Enable the Tango temperature sensor driver, which provides support
+	  for the sensors used since the SMP8758.

Please add a better description, e.g. which sensors are supported, locations, accuracy, and more meaningful info an engineer could read out of the help section.

No license/author/contact section here?

drivers/thermal/tango_thermal.c line 28

Quoted 2 lines

drivers/thermal/tango_thermal.c new file +2 -0

+	writel(thresh_idx << 8 | CMD_READ, base + TEMPSI_CMD);
+	usleep_range(100, 200);

nip: blank line here.

drivers/thermal/tango_thermal.c line 29

Quoted 1 line

drivers/thermal/tango_thermal.c new file +1 -0

+	return readl(base + TEMPSI_RES);

Quoted 5 lines

drivers/thermal/tango_thermal.c new file +5 -0

+	if (temp_above_thresh(base, idx)) {
+		/* Upward linear search, increment thresh */
+		while (idx < IDX_MAX && temp_above_thresh(base, ++idx))
+			cpu_relax();
+		idx = idx - 1; /* always return lower bound */

drivers/thermal/tango_thermal.c line 47

Quoted 5 lines

drivers/thermal/tango_thermal.c new file +5 -0

+	} else {
+		/* Downward linear search, decrement thresh */
+		while (idx > IDX_MIN && !temp_above_thresh(base, --idx))
+			cpu_relax();
+	}

Did I understand this right? We can only read if the temperature is above an index or not, right?

and we spin for 100-200us after every attempt to check if temperature is above and index.

So, worst case,
if we start from IDX_MIN, and temp is at IDX_MAX, we spin for 40 * (100 .. 200) us (with cpu_relax in between).

Does the chip support different temperature read strategy?

How does this perform in average case?

How about a local search within +-2 indexes of thresh_idx?

drivers/thermal/tango_thermal.c line 76

Quoted 1 line

drivers/thermal/tango_thermal.c new file +1 -0

+	priv->zone = thermal_zone_of_sensor_register(dev, 0, priv, &ops);

Would it make sense to use the devm_thermal_zone_of_sensor_register version instead?

Quoted 1 line · Eduardo Valentin

Again, only minor comments, as follows.

I do hope we can get this driver sorted out in time for the 4.7 merge window.

Quoted 5 lines

Documentation/devicetree/bindings/thermal/tango-thermal.txt line 4

Quoted 3 lines

Documentation/devicetree/bindings/thermal/tango-thermal.txt new file +2 -0

+The SMP8758 SoC includes 3 instances of this temperature sensor
+(in the CPU, video decoder, and PCIe controller).

Would you be able to add a link to sensor documentation?

I couldn't find any documentation online, even on Chinese sites.

Quoted 6 lines

Documentation/devicetree/bindings/thermal/tango-thermal.txt line 16

Quoted 4 lines

Documentation/devicetree/bindings/thermal/tango-thermal.txt new file +4 -0

+	cpu_temp: thermal@920100 {
+		compatible = "sigma,smp8758-thermal";
+		#thermal-sensor-cells = <0>;
+		reg = <0x920100 12>;

I believe you have to put #thermal-sensor-cells as last entry.

I've been using the node as-is in my DT, which works as expected. What kinds of failure did you have in mind?

Quoted 7 lines

drivers/thermal/Kconfig config ARMADA_THERMAL · line 268

Quoted 3 lines

drivers/thermal/Kconfig +2 -0

+	  Enable the Tango temperature sensor driver, which provides support
+	  for the sensors used since the SMP8758.

Please add a better description, e.g. which sensors are supported, locations, accuracy, and more meaningful info an engineer could read out of the help section.

I will try to give a few more details, but AFAICS the majority of entries for other SoCs are as short as mine...

What do you mean by "which sensors"?

Locations isn't really relevant here because different SoCs will have them in different places.

Quoted 2 lines

No license/author/contact section here?

License is given by MODULE_LICENSE("GPL"); author and contact are given by git log or git blame.

Quoted 4 lines

drivers/thermal/tango_thermal.c line 28

Quoted 2 lines

drivers/thermal/tango_thermal.c new file +2 -0

+	writel(thresh_idx << 8 | CMD_READ, base + TEMPSI_CMD);
+	usleep_range(100, 200);

nip: blank line here.

Did you mean "nit"? :-)

Quoted 1 line

drivers/thermal/tango_thermal.c line 29

Quoted 1 line

drivers/thermal/tango_thermal.c new file +1 -0

+	return readl(base + TEMPSI_RES);

Hmm, I don't see this rule in the CodingStyle document.

There are even examples to the contrary:

		kfree(foo->bar);
		kfree(foo);
		return ret;

Quoted 5 lines

Quoted 5 lines

drivers/thermal/tango_thermal.c new file +5 -0

+	if (temp_above_thresh(base, idx)) {
+		/* Upward linear search, increment thresh */
+		while (idx < IDX_MAX && temp_above_thresh(base, ++idx))
+			cpu_relax();
+		idx = idx - 1; /* always return lower bound */

Quoted 8 lines

drivers/thermal/tango_thermal.c line 47

Quoted 5 lines

drivers/thermal/tango_thermal.c new file +5 -0

+	} else {
+		/* Downward linear search, decrement thresh */
+		while (idx > IDX_MIN && !temp_above_thresh(base, --idx))
+			cpu_relax();
+	}

Did I understand this right? We can only read if the temperature is above an index or not, right?

Right, the hardware's output is a single bit:
"Is the temperature above the programmed threshold?" yes/no.

Quoted 2 lines

and we spin for 100-200us after every attempt to check if temperature is above and index.

Nit: we don't spin, we sleep.

100-200 µs is a bit conservative, the hardware doesn't need that much. But I wanted to give Linux the opportunity to group multiple checks together, and even switch to a different task if necessary. I assumed that it doesn't make sense to context switch for only tens of microseconds?

Quoted 3 lines

So, worst case,
if we start from IDX_MIN, and temp is at IDX_MAX, we spin for 40 * (100 .. 200) us (with cpu_relax in between).

On my system, cpu_relax is just a compiler barrier, so barely a blip. And the worst case doesn't occur. But I will change IDX_MIN to 10, because I don't care about sub-zero temperatures.

Quoted 1 line

Does the chip support different temperature read strategy?

I'm not sure what you mean.

Quoted 2 lines

How does this perform in average case?

Typical idle temps are index 18-20 (43-52 °C)
When I load the CPU with cpuburn, the temp climbs to index 22-24 (61-70 °C).
But obviously, these numbers depend on the thermal solution.

Quoted 1 line

How about a local search within +-2 indexes of thresh_idx?

What problem would that solve?

Quoted 4 lines

drivers/thermal/tango_thermal.c line 76

Quoted 1 line

drivers/thermal/tango_thermal.c new file +1 -0

+	priv->zone = thermal_zone_of_sensor_register(dev, 0, priv, &ops);

Would it make sense to use the devm_thermal_zone_of_sensor_register version instead?

Does that automatically call thermal_zone_of_sensor_unregister in the remove() method?

I prefer having the symmetry of register/unregister, but I guess it's your call as maintainer.

Regards.

Quoted 1 line · Mason

I couldn't find any documentation online, even on Chinese sites.

How did you come up with this code then? How do I know it is actually working? Can somebody else verify this and reply with a tested-by?

Quoted 7 lines

I will try to give a few more details, but AFAICS the majority of entries for other SoCs are as short as mine...

What do you mean by "which sensors"?

Locations isn't really relevant here because different SoCs will have them in different places.

This is confusing now. You mention in your commit message that the SoC has three sensor instances, and they are in the CPU, video decoder, and PCIe controller. Now, you are mentioning location does not matter.

So, what to expect from this driver ?

Quoted 1 line

Nit: we don't spin, we sleep.

Well, we are using timers, you are right, but we are still using the CPU to check the hardware status. Is there a better way to do this? If it is threshold based, does the hardware produces IRQs?

Does this hardware support reading temperature in a different way? I must say this is an awkward way of doing this, even worst blindly without documentation.

I am just attempting to understand this code and if it makes sense at all. Of course, without hardware doc all this is just guessing.

From: Marc Gonzalez <[email protected]>

The Tango thermal driver provides support for the primitive temperature sensor embedded in Tango chips since the SMP8758.

This sensor only generates a 1-bit signal to indicate whether the die temperature exceeds a programmable threshold.

Signed-off-by Marc Gonzalez <[email protected]>

---
 Documentation/devicetree/bindings/thermal/tango-thermal.txt |  17 ++++
 drivers/thermal/Kconfig                                     |   9 ++
 drivers/thermal/Makefile                                    |   1 +
 drivers/thermal/tango_thermal.c                             | 125 +++++++++++++++++++++++++++
 4 files changed, 152 insertions(+)

From: Marc Gonzalez <[email protected]>

The Tango thermal driver provides support for the primitive temperature sensor embedded in Tango chips since the SMP8758.

This sensor only generates a 1-bit signal to indicate whether the die temperature exceeds a programmable threshold.

Signed-off-by Marc Gonzalez <[email protected]>

---
CCing Rob and Mark for the DT parts

---
 Documentation/devicetree/bindings/thermal/tango-thermal.txt |  17 ++++
 arch/arm/boot/dts/tango4-smp8758.dtsi                       |  16 ++++
 drivers/thermal/Kconfig                                     |   9 ++
 drivers/thermal/Makefile                                    |   1 +
 drivers/thermal/tango_thermal.c                             | 125 +++++++++++++++++++++++++++
 5 files changed, 168 insertions(+)

Documentation/devicetree/bindings/thermal/tango-thermal.txt new file +17 -0

new file mode 100644
index 000000000000..212198d4b937
@@ -0,0 +1,17 @@
+* Tango Thermal
+
+The SMP8758 SoC includes 3 instances of this temperature sensor
+(in the CPU, video decoder, and PCIe controller).
+
+Required properties:
+- #thermal-sensor-cells: Should be 0 (see thermal.txt)
+- compatible: "sigma,smp8758-thermal"
+- reg: Address range of the thermal registers
+
+Example:
+
+	cpu_temp: thermal@920100 {
+		#thermal-sensor-cells = <0>;
+		compatible = "sigma,smp8758-thermal";
+		reg = <0x920100 12>;
+	};

arch/arm/boot/dts/tango4-smp8758.dtsi +5 -0

index 7ed88ee629fb..44d57c02e934 100644
@@ -28,4 +28,20 @@
 			<GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
 			<GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
 	};
+
+	soc {
+		cpu_temp: thermal@920100 {
+			#thermal-sensor-cells = <0>;
+			compatible = "sigma,smp8758-thermal";

drivers/thermal/tango_thermal.c new file +16 -0

+static int tango_thermal_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct tango_thermal_priv *priv;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	writel(CMD_ON, priv->base + TEMPSI_CMD);
+	writel(CYCLE_COUNT, priv->base + TEMPSI_CFG);

drivers/thermal/tango_thermal.c line 39

Quoted 6 lines · Mason

drivers/thermal/tango_thermal.c new file +6 -0

+static bool temp_above_thresh(void __iomem *base, int thresh_idx)
+{
+	writel(CMD_READ | thresh_idx << 8, base + TEMPSI_CMD);
+	usleep_range(10, 20);
+	return readl(base + TEMPSI_RES);
+}

A colleague suggested a tweak for temp_above_thresh() which does not require messing with TEMPSI_CFG => v8 required.

Quoted 17 lines

drivers/thermal/tango_thermal.c new file +16 -0

+static int tango_thermal_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct tango_thermal_priv *priv;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	writel(CMD_ON, priv->base + TEMPSI_CMD);
+	writel(CYCLE_COUNT, priv->base + TEMPSI_CFG);

drivers/thermal/tango_thermal.c line 93

Quoted 10 lines

drivers/thermal/tango_thermal.c new file +10 -0

+
+	priv->zone = thermal_zone_of_sensor_register(&pdev->dev, 0, priv, &ops);
+	if (IS_ERR(priv->zone))
+		return PTR_ERR(priv->zone);
+
+	priv->thresh_idx = IDX_MIN;
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+}

It seems tango_get_temp() is called (at least once) before the end of tango_thermal_probe()

Does that mean that the device must be fully initialized *BEFORE* thermal_zone_of_sensor_register() is called?
(I assume it's that function that triggers the call to get_temp)

In that case, I need to move priv->thresh_idx = IDX_MIN; earlier.

Also, can get_temp() be called concurrently?
For example, from user-space reading /sys/class/thermal/thermal_zone0/temp and from the kernel thread polling the sensor?
Is locking taken care of in higher levels?

For the record, I test the driver with the following script. Is it good enough?

TEMP="/sys/class/thermal/thermal_zone0/temp"

echo RUN IDLE
for ((I=0; I<30; ++I)); do cat $TEMP; sleep 2; done

echo RUN HEAVY LOAD
cpuburn-a9 &
cpuburn-a9 &
for ((I=0; I<60; ++I)); do cat $TEMP; sleep 2; done

echo KILL HEAVY LOAD
kill $(jobs -p)
for ((I=0; I<30; ++I)); do cat $TEMP; sleep 2; done

Regards.

From: Marc Gonzalez <[email protected]>

The Tango thermal driver provides support for the primitive temperature sensor embedded in Tango chips since the SMP8758.

This sensor only generates a 1-bit signal to indicate whether the die temperature exceeds a programmable threshold.

drivers/thermal/tango_thermal.c line 85

Quoted 1 line · Mason

drivers/thermal/tango_thermal.c new file +1 -0

+	priv->zone = thermal_zone_of_sensor_register(&pdev->dev, 0, priv, &ops);

I would still prefer you use the devm_ version.

The Tango thermal driver provides support for the primitive temperature sensor embedded in Tango chips since the SMP8758.

This sensor only generates a 1-bit signal to indicate whether the die temperature exceeds a programmable threshold.

From: Marc Gonzalez <[email protected]>

Define the CPU temperature sensor, and critical trip point.

Signed-off-by Marc Gonzalez <[email protected]>

---
 arch/arm/boot/dts/tango4-smp8758.dtsi | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

arch/arm/boot/dts/tango4-smp8758.dtsi +23 -0

index 7ed88ee629fb..01d5f8f1c71d 100644
@@ -28,4 +28,27 @@
 			<GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
 			<GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
 	};
+
+	soc {
+		cpu_temp: thermal@920100 {
+			#thermal-sensor-cells = <0>;
+			compatible = "sigma,smp8758-thermal";
+			reg = <0x920100 12>;
+		};
+	};
+
+	thermal-zones {
+		cpu_thermal: cpu-thermal {
+			polling-delay = <997>;		/* milliseconds */
+			polling-delay-passive = <499>;	/* milliseconds */
+			thermal-sensors = <&cpu_temp>;
+			trips {
+				cpu_critical {
+					temperature = <120000>;
+					hysteresis = <2500>;
+					type = "critical";
+				};
+			};
+		};
+	};
 };

Signature · 1 line

2.8.1

The SMP8758 has the 3. The next SoC has 5, in different HW blocks. HW team likes to move them around, apparently.

Quoted 1 line

So, what to expect from this driver ?

One should expect this device driver to support the temperature sensor used in Tango chips since the SMP8758, by using the "sigma,smp8758-thermal" compatible string in DT nodes.

Quoted 6 lines

Quoted 2 lines

Nit: we don't spin, we sleep.

Well, we are using timers, you are right, but we are still using the CPU to check the hardware status. Is there a better way to do this? If it is threshold based, does the hardware produces IRQs?

That I know for sure: there are no IRQ lines coming out of the HW block.

Quoted 4 lines

Does this hardware support reading temperature in a different way? I must say this is an awkward way of doing this, even worst blindly without documentation.

The documentation states:

"The temp sensor uses a bandgap type of circuit to compare a voltage which has a negative temperature coefficient with a voltage that is proportional to absolute temperature. A resistor bank allows 40 different temperature thresholds to be selected and the logic output 'out_temperature' will then indicate whether the actual die temperature lies above or below the selected threshold."

[NB: there are, in fact, 41 thresholds. The data sheet is inaccurate in a few places. ]

Characteristics

Symbol Parameter Min Typ Max Unit

(Operating conditions)

Tjunc      Junction temperature   -40   25   125  °C
Vdd        Supply voltage         1.0  1.1  1.26   V

(Normal operating mode)
Idd Supply current 50 60 μA
Vbandgapref Ref output voltage 0.72 0.8 0.88 V
∆outtemp Absolute Temp ±2 ±10 °C
threshold error
T_res Temp resolution 3 4.5 7 °C

I think the expected use-case was to program a "critical" threshold, and have the 1-bit output signal "Oh no, we are melting down!". But the HW guys thought "Hey, let's use this as a thermometer."

Quoted 3 lines

I am just attempting to understand this code and if it makes sense at all. Of course, without hardware doc all this is just guessing.

This is what the HW guys recommend:

1) Set the thresh to the desired value
2) Wait unknown amount of time
3) Read the 1-bit result

I will try to address most of your comments in a v6 in the next few days.

Thanks for your reviews, by the way.

Regards.