[RFC] Temperature sensor driver (tango)

[RFC] Temperature sensor driver (tango)

[Mason]

Hello,

I've written a temperature sensor driver for my SoC, with the
intent of using it for CPU throttling, using cpufreq levels.
(I think that's called passive cooling.)

For testing purposes, I've added two nodes to my DT:

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

	thermal-zones {
		cpu_thermal: cpu-thermal {
			thermal-sensors = <&cpu_temp>;
		};
	};

I'd like to submit the driver for inclusion upstream, so I'm
making this request for comments on the driver. (I suppose I
would also need to change Makefile/Kconfig?)


#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 sensor_idle(base)	(readl_relaxed(base + TEMPSI_CMD) & BIT(7))
#define TEMP_LESS_THAN_THRESH	0
#define INDEX_OFFSET		18

static const u8 temperature[] = {
	 37,  41,  46,  51,  55,  60,  64,  69,
	 74,  79,  83,  88,  93,  97, 101, 106,
	110, 115, 120, 124, 129, 133, 137, 142,
};

static int tango_get_temp(void *arg, int *res)
{
	int i;
	void __iomem *base = arg;

	for (i = 0; i < 24; ++i)
	{
		int thresh_idx = INDEX_OFFSET + i;
		writel_relaxed(thresh_idx << 8 | CMD_READ, base + TEMPSI_CMD);

		while (!sensor_idle(base))
			cpu_relax();

		if (readl_relaxed(base + TEMPSI_RES) == TEMP_LESS_THAN_THRESH)
			break;
	}

	writel_relaxed(INDEX_OFFSET << 8 | CMD_READ, base + TEMPSI_CMD);
	*res = temperature[i] * 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;

	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	priv->base = devm_ioremap_resource(dev, res);
	if (IS_ERR(priv->base))
		return PTR_ERR(priv->base);

	writel_relaxed(CMD_ON, priv->base + TEMPSI_CMD);
	writel_relaxed(    50, priv->base + TEMPSI_CFG);

	priv->zone = thermal_zone_of_sensor_register(dev, 0, priv->base, &ops);
	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");

[PATCH v2] thermal: add temperature sensor support for tango SoC

[Mason]

From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>

There are three temperature sensors in SMP8758.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 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

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 7c92c09be213..b63298276532 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -254,6 +254,12 @@ 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 TANGO_THERMAL
+	tristate "Tango4 thermal management"
+	depends on ARCH_TANGO || COMPILE_TEST
+	help
+	  Enable SMP8758 temperature sensor driver.
+
 config TEGRA_SOCTHERM
 	tristate "Tegra SOCTHERM thermal management"
 	depends on ARCH_TEGRA
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index cfae6a654793..8bfea2b7e722 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -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
diff --git a/drivers/thermal/tango_thermal.c b/drivers/thermal/tango_thermal.c
new file mode 100644
index 000000000000..eeccb6b0894e
--- /dev/null
+++ b/drivers/thermal/tango_thermal.c
@@ -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,
+	110, 115, 120, 124, 129, 133, 137, 142,
+};
+
+static int tango_get_temp(void *arg, int *res)
+{
+	int i;
+	void __iomem *base = arg;
+
+	for (i = INDEX_OFFSET; i < 41; ++i)
+	{
+		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;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	writel_relaxed(CMD_ON, priv->base + TEMPSI_CMD);
+	writel_relaxed(    50, priv->base + TEMPSI_CFG);
+
+	priv->zone = thermal_zone_of_sensor_register(dev, 0, priv->base, &ops);
+	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");
-- 
2.7.0

Re: [PATCH v2] thermal: add temperature sensor support for tango SoC

[Eduardo Valentin]

Hello Marc,

First of all, thanks for keeping this driver simple.

A couple of extra documentation needed to add the driver. Please see
comments as follows.

On Fri, Mar 04, 2016 at 04:52:47PM +0100, Mason wrote:
> From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> 
> There are three temperature sensors in SMP8758.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>  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/ 
?


>  3 files changed, 120 insertions(+)
>  create mode 100644 drivers/thermal/tango_thermal.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 7c92c09be213..b63298276532 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -254,6 +254,12 @@ 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 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?

> +
>  config TEGRA_SOCTHERM
>  	tristate "Tegra SOCTHERM thermal management"
>  	depends on ARCH_TEGRA
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index cfae6a654793..8bfea2b7e722 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -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
> diff --git a/drivers/thermal/tango_thermal.c b/drivers/thermal/tango_thermal.c
> new file mode 100644
> index 000000000000..eeccb6b0894e
> --- /dev/null
> +++ b/drivers/thermal/tango_thermal.c
> @@ -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,
> +	110, 115, 120, 124, 129, 133, 137, 142,
> +};
> +
> +static int tango_get_temp(void *arg, int *res)
> +{
> +	int i;
> +	void __iomem *base = arg;
> +
> +	for (i = INDEX_OFFSET; i < 41; ++i)
> +	{

Please follow kernel coding style.


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

> +		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;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	writel_relaxed(CMD_ON, priv->base + TEMPSI_CMD);
> +	writel_relaxed(    50, priv->base + TEMPSI_CFG);
> +
> +	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?

> +	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");
> -- 
> 2.7.0

Re: [PATCH v2] thermal: add temperature sensor support for tango SoC

[Mason]

Hello Eduardo,

On 08/03/2016 22:48, Eduardo Valentin wrote:

> On Fri, Mar 04, 2016 at 04:52:47PM +0100, Mason wrote:
>
>>  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/

OK.

>> +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?

Is checkpatch.pl complaining that the help text is only one line long?
("please write a paragraph that describes the config symbol fully")
There is nothing more to say. TANGO_THERMAL=Y will just enable support
for the temperature sensors on the 8758... I guess I could say that I
plan to use it for passive cooling with cpufreq?

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

I've now changed the algorithm, based on a colleague's suggestion.
I'll try documenting the new algorithm (a trivial linear search).

>> +	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?

I plan to define 3 distinct thermal zones (each containing a single sensor).

e.g. for the time being, I've only defined the CPU thermal zone:

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

	thermal-zones {
		cpu_thermal: cpu-thermal {
			polling-delay-passive = <2000>;	/* ms */
			polling-delay = <1000>;		/* ms */
			thermal-sensors = <&cpu_temp>;
		};
	};


Regards.

[PATCH v3] thermal: add temperature sensor support for tango SoC

[Mason]

From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>

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

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 Documentation/devicetree/bindings/thermal/tango-thermal.txt |  17 ++++
 drivers/thermal/Kconfig                                     |   7 ++
 drivers/thermal/Makefile                                    |   1 +
 drivers/thermal/tango_thermal.c                             | 122 +++++++++++++++++++++++++++
 4 files changed, 147 insertions(+)

diff --git a/Documentation/devicetree/bindings/thermal/tango-thermal.txt b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
new file mode 100644
index 000000000000..a203f7d60d35
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
@@ -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:
+- compatible: "sigma,smp8758-thermal"
+- #thermal-sensor-cells: Should be 0 (see thermal.txt)
+- reg: Address range of the thermal registers
+
+Example:
+
+	cpu_temp: thermal@920100 {
+		compatible = "sigma,smp8758-thermal";
+		#thermal-sensor-cells = <0>;
+		reg = <0x920100 12>;
+	};
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 8cc4ac64a91c..53a1f0e92e82 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -254,6 +254,13 @@ 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 TANGO_THERMAL
+	tristate "Tango thermal management"
+	depends on ARCH_TANGO || COMPILE_TEST
+	help
+	  Enable the Tango temperature sensor driver, which provides support
+	  for the sensors used since the SMP8758.
+
 config TEGRA_SOCTHERM
 	tristate "Tegra SOCTHERM thermal management"
 	depends on ARCH_TEGRA
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index cfae6a654793..8bfea2b7e722 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -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
diff --git a/drivers/thermal/tango_thermal.c b/drivers/thermal/tango_thermal.c
new file mode 100644
index 000000000000..6b5f0d10fbd0
--- /dev/null
+++ b/drivers/thermal/tango_thermal.c
@@ -0,0 +1,122 @@
+#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 IDX_MIN		12
+#define IDX_MAX		40
+#define CYCLE_COUNT	50 /* Time to wait before sampling the result */
+
+static const u8 temperature[] = {
+	15, 19, 23, 28, 33, 37, 41, 46, 51, 55, 60, 64, 69, 74, 79, 83,
+	88, 93, 97, 101, 106, 110, 115, 120, 124, 129, 133, 137,
+};
+
+struct tango_thermal_priv {
+	struct thermal_zone_device *zone;
+	void __iomem *base;
+	int thresh_idx;
+};
+
+#define sensor_idle(base) (readl(base + TEMPSI_CMD) & BIT(7))
+
+static bool temp_above_thresh(void __iomem *base, int thresh_idx)
+{
+	writel_relaxed(thresh_idx << 8 | CMD_READ, base + TEMPSI_CMD);
+
+	while (!sensor_idle(base))
+		cpu_relax();
+
+	return readl_relaxed(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;
+
+	if (temp_above_thresh(base, idx)) {
+		/* Downward linear search */
+		while (idx > IDX_MIN && temp_above_thresh(base, --idx))
+			cpu_relax();
+	} else {
+		/* Upward linear search */
+		while (idx < IDX_MAX && !temp_above_thresh(base, ++idx))
+			cpu_relax();
+		idx = idx - 1;
+	}
+
+	priv->thresh_idx = idx;
+	*res = temperature[idx - IDX_MIN] * 1000;
+	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;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	writel_relaxed(CMD_ON, priv->base + TEMPSI_CMD);
+	writel_relaxed(CYCLE_COUNT, priv->base + TEMPSI_CFG);
+
+	priv->zone = thermal_zone_of_sensor_register(dev, 0, priv, &ops);
+	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(CMD_OFF, priv->base + TEMPSI_CMD);
+	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");
-- 
2.7.0

Re: [PATCH v3] thermal: add temperature sensor support for tango SoC

[Mason]

On 24/03/2016 13:18, Mason wrote:

> +	if (temp_above_thresh(base, idx)) {
> +		/* Downward linear search */
> +		while (idx > IDX_MIN && temp_above_thresh(base, --idx))
> +			cpu_relax();
> +	} else {
> +		/* Upward linear search */
> +		while (idx < IDX_MAX && !temp_above_thresh(base, ++idx))
> +			cpu_relax();
> +		idx = idx - 1;
> +	}

That's wrong, I mixed up the two branches.

When temp is above thresh, we should search upward.

I'll send v4 in a few days (needs more testing).

Regards.

[PATCH v4] thermal: add temperature sensor support for tango SoC

[Mason]

From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>

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

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 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(+)

diff --git a/Documentation/devicetree/bindings/thermal/tango-thermal.txt b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
new file mode 100644
index 000000000000..a203f7d60d35
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
@@ -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:
+- compatible: "sigma,smp8758-thermal"
+- #thermal-sensor-cells: Should be 0 (see thermal.txt)
+- reg: Address range of the thermal registers
+
+Example:
+
+	cpu_temp: thermal@920100 {
+		compatible = "sigma,smp8758-thermal";
+		#thermal-sensor-cells = <0>;
+		reg = <0x920100 12>;
+	};
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index c37eedc35a24..b6bf563f05dc 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -260,6 +260,13 @@ 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 TANGO_THERMAL
+	tristate "Tango thermal management"
+	depends on ARCH_TANGO || COMPILE_TEST
+	help
+	  Enable the Tango temperature sensor driver, which provides support
+	  for the sensors used since the SMP8758.
+
 config TEGRA_SOCTHERM
 	tristate "Tegra SOCTHERM thermal management"
 	depends on ARCH_TEGRA
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 8e9cbc3b5679..06387279883d 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -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
diff --git a/drivers/thermal/tango_thermal.c b/drivers/thermal/tango_thermal.c
new file mode 100644
index 000000000000..decfe857e1a6
--- /dev/null
+++ b/drivers/thermal/tango_thermal.c
@@ -0,0 +1,114 @@
+#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 */
+
+struct tango_thermal_priv {
+	struct thermal_zone_device *zone;
+	void __iomem *base;
+	int thresh_idx;
+};
+
+static bool temp_above_thresh(void __iomem *base, int thresh_idx)
+{
+	writel(thresh_idx << 8 | CMD_READ, base + TEMPSI_CMD);
+	usleep_range(100, 200);
+	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;
+
+	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 */
+	} else {
+		/* Downward linear search, decrement thresh */
+		while (idx > IDX_MIN && !temp_above_thresh(base, --idx))
+			cpu_relax();
+	}
+
+	*res = idx * 4500 - 38000; /* millidegrees Celsius */
+	priv->thresh_idx = idx;
+	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;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(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);
+
+	priv->zone = thermal_zone_of_sensor_register(dev, 0, priv, &ops);
+	if (IS_ERR(priv->zone))
+		return PTR_ERR(priv->zone);
+
+	priv->thresh_idx = 15; /* arbitrary starting point */
+	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(CMD_OFF, priv->base + TEMPSI_CMD);
+	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");
-- 
2.7.4

[PATCH v5] thermal: add temperature sensor support for tango SoC

[Mason]

From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>

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

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 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(+)

diff --git a/Documentation/devicetree/bindings/thermal/tango-thermal.txt b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
new file mode 100644
index 000000000000..a203f7d60d35
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
@@ -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:
+- compatible: "sigma,smp8758-thermal"
+- #thermal-sensor-cells: Should be 0 (see thermal.txt)
+- reg: Address range of the thermal registers
+
+Example:
+
+	cpu_temp: thermal@920100 {
+		compatible = "sigma,smp8758-thermal";
+		#thermal-sensor-cells = <0>;
+		reg = <0x920100 12>;
+	};
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index c37eedc35a24..b6bf563f05dc 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -260,6 +260,13 @@ 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 TANGO_THERMAL
+	tristate "Tango thermal management"
+	depends on ARCH_TANGO || COMPILE_TEST
+	help
+	  Enable the Tango temperature sensor driver, which provides support
+	  for the sensors used since the SMP8758.
+
 config TEGRA_SOCTHERM
 	tristate "Tegra SOCTHERM thermal management"
 	depends on ARCH_TEGRA
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 8e9cbc3b5679..06387279883d 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -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
diff --git a/drivers/thermal/tango_thermal.c b/drivers/thermal/tango_thermal.c
new file mode 100644
index 000000000000..8c9abdf6a507
--- /dev/null
+++ b/drivers/thermal/tango_thermal.c
@@ -0,0 +1,114 @@
+#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 */
+
+struct tango_thermal_priv {
+	struct thermal_zone_device *zone;
+	void __iomem *base;
+	int thresh_idx;
+};
+
+static bool temp_above_thresh(void __iomem *base, int thresh_idx)
+{
+	writel(thresh_idx << 8 | CMD_READ, base + TEMPSI_CMD);
+	usleep_range(100, 200);
+	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;
+
+	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 */
+	} else {
+		/* Downward linear search, decrement thresh */
+		while (idx > IDX_MIN && !temp_above_thresh(base, --idx))
+			cpu_relax();
+	}
+
+	*res = (idx * 9 / 2 - 38) * 1000; /* millidegrees Celsius */
+	priv->thresh_idx = idx;
+	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;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(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);
+
+	priv->zone = thermal_zone_of_sensor_register(dev, 0, priv, &ops);
+	if (IS_ERR(priv->zone))
+		return PTR_ERR(priv->zone);
+
+	priv->thresh_idx = 15; /* arbitrary starting point */
+	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(CMD_OFF, priv->base + TEMPSI_CMD);
+	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");
-- 
2.7.4

Re: [PATCH v5] thermal: add temperature sensor support for tango SoC

[Eduardo Valentin]

Marc,

Again, only minor comments, as follows.


On Mon, Mar 28, 2016 at 01:49:27PM +0200, Mason wrote:
> From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> 
> Since the SMP8758, Tango SoCs include several instances of a rudimentary
> bandgap temperature sensor.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>  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(+)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/tango-thermal.txt b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
> new file mode 100644
> index 000000000000..a203f7d60d35
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
> @@ -0,0 +1,17 @@
> +* Tango Thermal
> +
> +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?

> +
> +Required properties:
> +- compatible: "sigma,smp8758-thermal"
> +- #thermal-sensor-cells: Should be 0 (see thermal.txt)
> +- reg: Address range of the thermal registers
> +
> +Example:
> +
> +	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.

> +	};
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index c37eedc35a24..b6bf563f05dc 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -260,6 +260,13 @@ 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 TANGO_THERMAL
> +	tristate "Tango thermal management"
> +	depends on ARCH_TANGO || COMPILE_TEST
> +	help
> +	  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.

> +
>  config TEGRA_SOCTHERM
>  	tristate "Tegra SOCTHERM thermal management"
>  	depends on ARCH_TEGRA
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 8e9cbc3b5679..06387279883d 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -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
> diff --git a/drivers/thermal/tango_thermal.c b/drivers/thermal/tango_thermal.c
> new file mode 100644
> index 000000000000..8c9abdf6a507
> --- /dev/null
> +++ b/drivers/thermal/tango_thermal.c
> @@ -0,0 +1,114 @@

No license/author/contact section here?

> +#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 */
> +
> +struct tango_thermal_priv {
> +	struct thermal_zone_device *zone;
> +	void __iomem *base;
> +	int thresh_idx;
> +};
> +
> +static bool temp_above_thresh(void __iomem *base, int thresh_idx)
> +{
> +	writel(thresh_idx << 8 | CMD_READ, base + TEMPSI_CMD);
> +	usleep_range(100, 200);
nip: blank line here.

> +	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;
> +
> +	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 */
> +	} 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?

> +
> +	*res = (idx * 9 / 2 - 38) * 1000; /* millidegrees Celsius */
> +	priv->thresh_idx = idx;
nip: blank line here.
> +	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;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(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);
> +
> +	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?

> +	if (IS_ERR(priv->zone))
> +		return PTR_ERR(priv->zone);
> +
> +	priv->thresh_idx = 15; /* arbitrary starting point */
> +	platform_set_drvdata(pdev, priv);
nip: blank line here.
> +	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(CMD_OFF, priv->base + TEMPSI_CMD);
nip: blank line here.
> +	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");
> -- 
> 2.7.4

Re: [PATCH v5] thermal: add temperature sensor support for tango SoC

[Mason]

On 29/03/2016 04:00, Eduardo Valentin wrote:

> Again, only minor comments, as follows.

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

>> +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.

>> +	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?

>> +	  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.

> No license/author/contact section here?

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

>> +	writel(thresh_idx << 8 | CMD_READ, base + TEMPSI_CMD);
>> +	usleep_range(100, 200);
>
> nip: blank line here.

Did you mean "nit"? :-)

>> +	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;

>> +	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 */
>> +	} 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.

> 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?

> 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.

> Does the chip support different temperature read strategy?

I'm not sure what you mean.

> 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.

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

What problem would that solve?

>> +	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.

Re: [PATCH v5] thermal: add temperature sensor support for tango SoC

[Eduardo Valentin]

On Tue, Mar 29, 2016 at 08:48:43PM +0200, Mason wrote:
> On 29/03/2016 04:00, Eduardo Valentin wrote:
> 
> > Again, only minor comments, as follows.
> 
> I do hope we can get this driver sorted out in time for the 4.7
> merge window.

Well, I hope for the same, but the driver needs to be clarified and
properly documented, don't you think?

And now that you are here, and you have written this nice driver, it
would be good to make sure whoever comes to patch it later have enough
groundings to do the right thing.

> 
> >> +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.
> 

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?

> >> +	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?

No failures in fact, that was just a pattern.

> 
> >> +	  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.

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 ?

> 
> > No license/author/contact section here?
> 
> License is given by MODULE_LICENSE("GPL");
> author and contact are given by git log or git blame.
> 
> >> +	writel(thresh_idx << 8 | CMD_READ, base + TEMPSI_CMD);
> >> +	usleep_range(100, 200);
> >
> > nip: blank line here.
> 
> Did you mean "nit"? :-)
> 
> >> +	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;
> 

That, as mentioned, is a coding suggestion, based on what I have been
seeing as pattern. 


> >> +	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 */
> >> +	} 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.
> 
> > and we spin for 100-200us after every 
> > attempt to check if temperature is above and index.
> 
> 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?

> 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?
> 
> > 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.
> 
> > Does the chip support different temperature read strategy?
> 
> I'm not sure what you mean.

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.

> 
> > 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.
> 
> > How about a local search within +-2 indexes of thresh_idx?

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.

> 
> What problem would that solve?
> 
> >> +	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?

yes

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

Re: [PATCH v5] thermal: add temperature sensor support for tango SoC

[Mason]

On 30/03/2016 02:05, Eduardo Valentin wrote:

> On Tue, Mar 29, 2016 at 08:48:43PM +0200, Mason wrote:
>
>> 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?

I do have access to an "Objective" data sheet (Product status: Development,
as opposed to a Production data sheet). It's lacking important details,
unfortunately. (The data sheet comes from NXP, and mentions C045LP_TS.)

>> 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.

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

> 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.

>> 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.

> 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."

> 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.

[PATCH v6] thermal: add temperature sensor support for tango SoC

[Mason]

From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>

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 <marc_gonzalez@sigmadesigns.com>
---
 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(+)

diff --git a/Documentation/devicetree/bindings/thermal/tango-thermal.txt b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
new file mode 100644
index 000000000000..212198d4b937
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
@@ -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>;
+	};
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index c37eedc35a24..1c9ab79fa3e0 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -260,6 +260,15 @@ 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 TANGO_THERMAL
+	tristate "Tango thermal management"
+	depends on ARCH_TANGO || COMPILE_TEST
+	help
+	  Enable the Tango thermal driver, which supports 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.
+
 config TEGRA_SOCTHERM
 	tristate "Tegra SOCTHERM thermal management"
 	depends on ARCH_TEGRA
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 8e9cbc3b5679..06387279883d 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -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
diff --git a/drivers/thermal/tango_thermal.c b/drivers/thermal/tango_thermal.c
new file mode 100644
index 000000000000..6e58af1db954
--- /dev/null
+++ b/drivers/thermal/tango_thermal.c
@@ -0,0 +1,125 @@
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/thermal.h>
+#include <linux/platform_device.h>
+
+/*
+ * According to a data sheet draft, "this temperature 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 41 different temperature thresholds to be selected
+ * and the logic output will then indicate whether the actual die temperature
+ * lies above or below the selected threshold."
+ */
+
+#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		15
+#define IDX_MAX		40
+#define CYCLE_COUNT	1000 /* Result sampled when timer expires */
+
+struct tango_thermal_priv {
+	struct thermal_zone_device *zone;
+	void __iomem *base;
+	int thresh_idx;
+};
+
+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);
+}
+
+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;
+
+	if (temp_above_thresh(base, idx)) {
+		/* Search upward by incrementing thresh_idx */
+		while (idx < IDX_MAX && temp_above_thresh(base, ++idx))
+			cpu_relax();
+		idx = idx - 1; /* always return lower bound */
+	} else {
+		/* Search downward by decrementing thresh_idx */
+		while (idx > IDX_MIN && !temp_above_thresh(base, --idx))
+			cpu_relax();
+	}
+
+	*res = (idx * 9 / 2 - 38) * 1000; /* millidegrees Celsius */
+	priv->thresh_idx = idx;
+
+	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;
+
+	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);
+
+	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;
+}
+
+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(CMD_OFF, priv->base + TEMPSI_CMD);
+
+	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");
-- 
2.7.4

Re: [PATCH v6] thermal: add temperature sensor support for tango SoC

[Eduardo Valentin]

On Thu, Mar 31, 2016 at 10:16:02PM +0200, Mason wrote:
> From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> 
> 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 <marc_gonzalez@sigmadesigns.com>
> ---
>  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(+)

Based on previous versions discussion, I don't see issues with this
specific patch, despite the fact that it lacks a pointer to its doc (but looks
like there is none available).

The only request I do for now, as mentioned on v5, is to include in your
series the required dt changes to add thermal zones, describing the
needed typical trips for this chip.

BR,

Eduardo Valentin

Re: [PATCH v6] thermal: add temperature sensor support for tango SoC

[Mason]

From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>

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 <marc_gonzalez@sigmadesigns.com>
---
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(+)

diff --git a/Documentation/devicetree/bindings/thermal/tango-thermal.txt b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
new file mode 100644
index 000000000000..212198d4b937
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
@@ -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>;
+	};
diff --git a/arch/arm/boot/dts/tango4-smp8758.dtsi b/arch/arm/boot/dts/tango4-smp8758.dtsi
index 7ed88ee629fb..44d57c02e934 100644
--- a/arch/arm/boot/dts/tango4-smp8758.dtsi
+++ b/arch/arm/boot/dts/tango4-smp8758.dtsi
@@ -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";
+			reg = <0x920100 12>;
+		};
+	};
+
+	thermal-zones {
+		cpu_thermal: cpu-thermal {
+			polling-delay-passive = <2003>;	/* ms */
+			polling-delay = <1009>;		/* ms */
+			thermal-sensors = <&cpu_temp>;
+		};
+	};
 };
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 8cc4ac64a91c..4b0776797700 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -254,6 +254,15 @@ 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 TANGO_THERMAL
+	tristate "Tango thermal management"
+	depends on ARCH_TANGO || COMPILE_TEST
+	help
+	  Enable the Tango thermal driver, which supports 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.
+
 config TEGRA_SOCTHERM
 	tristate "Tegra SOCTHERM thermal management"
 	depends on ARCH_TEGRA
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index cfae6a654793..8bfea2b7e722 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -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
diff --git a/drivers/thermal/tango_thermal.c b/drivers/thermal/tango_thermal.c
new file mode 100644
index 000000000000..4bb741662087
--- /dev/null
+++ b/drivers/thermal/tango_thermal.c
@@ -0,0 +1,125 @@
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/thermal.h>
+#include <linux/platform_device.h>
+
+/*
+ * According to a data sheet draft, "this temperature 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 41 different temperature thresholds to be selected
+ * and the logic output will then indicate whether the actual die temperature
+ * lies above or below the selected threshold."
+ */
+
+#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		15
+#define IDX_MAX		40
+#define CYCLE_COUNT	500 /* Result sampled when timer expires */
+
+struct tango_thermal_priv {
+	struct thermal_zone_device *zone;
+	void __iomem *base;
+	int thresh_idx;
+};
+
+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);
+}
+
+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;
+
+	if (temp_above_thresh(base, idx)) {
+		/* Search upward by incrementing thresh_idx */
+		while (idx < IDX_MAX && temp_above_thresh(base, ++idx))
+			cpu_relax();
+		idx = idx - 1; /* always return lower bound */
+	} else {
+		/* Search downward by decrementing thresh_idx */
+		while (idx > IDX_MIN && !temp_above_thresh(base, --idx))
+			cpu_relax();
+	}
+
+	*res = (idx * 9 / 2 - 38) * 1000; /* millidegrees Celsius */
+	priv->thresh_idx = idx;
+
+	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;
+
+	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);
+
+	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;
+}
+
+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(CMD_OFF, priv->base + TEMPSI_CMD);
+
+	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");
-- 
2.8.0

[PATCH v7] thermal: add temperature sensor support for tango SoC

[Mason]

From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>

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 <marc_gonzalez@sigmadesigns.com>
---
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(+)

diff --git a/Documentation/devicetree/bindings/thermal/tango-thermal.txt b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
new file mode 100644
index 000000000000..212198d4b937
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
@@ -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>;
+	};
diff --git a/arch/arm/boot/dts/tango4-smp8758.dtsi b/arch/arm/boot/dts/tango4-smp8758.dtsi
index 7ed88ee629fb..44d57c02e934 100644
--- a/arch/arm/boot/dts/tango4-smp8758.dtsi
+++ b/arch/arm/boot/dts/tango4-smp8758.dtsi
@@ -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";
+			reg = <0x920100 12>;
+		};
+	};
+
+	thermal-zones {
+		cpu_thermal: cpu-thermal {
+			polling-delay-passive = <2003>;	/* ms */
+			polling-delay = <1009>;		/* ms */
+			thermal-sensors = <&cpu_temp>;
+		};
+	};
 };
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 8cc4ac64a91c..4b0776797700 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -254,6 +254,15 @@ 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 TANGO_THERMAL
+	tristate "Tango thermal management"
+	depends on ARCH_TANGO || COMPILE_TEST
+	help
+	  Enable the Tango thermal driver, which supports 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.
+
 config TEGRA_SOCTHERM
 	tristate "Tegra SOCTHERM thermal management"
 	depends on ARCH_TEGRA
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index cfae6a654793..8bfea2b7e722 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -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
diff --git a/drivers/thermal/tango_thermal.c b/drivers/thermal/tango_thermal.c
new file mode 100644
index 000000000000..4bb741662087
--- /dev/null
+++ b/drivers/thermal/tango_thermal.c
@@ -0,0 +1,125 @@
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/thermal.h>
+#include <linux/platform_device.h>
+
+/*
+ * According to a data sheet draft, "this temperature 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 41 different temperature thresholds to be selected
+ * and the logic output will then indicate whether the actual die temperature
+ * lies above or below the selected threshold."
+ */
+
+#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		15
+#define IDX_MAX		40
+#define CYCLE_COUNT	500 /* Result sampled when timer expires */
+
+struct tango_thermal_priv {
+	struct thermal_zone_device *zone;
+	void __iomem *base;
+	int thresh_idx;
+};
+
+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);
+}
+
+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;
+
+	if (temp_above_thresh(base, idx)) {
+		/* Search upward by incrementing thresh_idx */
+		while (idx < IDX_MAX && temp_above_thresh(base, ++idx))
+			cpu_relax();
+		idx = idx - 1; /* always return lower bound */
+	} else {
+		/* Search downward by decrementing thresh_idx */
+		while (idx > IDX_MIN && !temp_above_thresh(base, --idx))
+			cpu_relax();
+	}
+
+	*res = (idx * 9 / 2 - 38) * 1000; /* millidegrees Celsius */
+	priv->thresh_idx = idx;
+
+	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;
+
+	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);
+
+	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;
+}
+
+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(CMD_OFF, priv->base + TEMPSI_CMD);
+
+	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");
-- 
2.8.0

Re: [PATCH v7] thermal: add temperature sensor support for tango SoC

[Eduardo Valentin]

On Mon, Apr 04, 2016 at 01:49:44PM +0200, Mason wrote:
> From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> 
> 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 <marc_gonzalez@sigmadesigns.com>
> ---
> 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(+)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/tango-thermal.txt b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
> new file mode 100644
> index 000000000000..212198d4b937
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
> @@ -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>;
> +	};
> diff --git a/arch/arm/boot/dts/tango4-smp8758.dtsi b/arch/arm/boot/dts/tango4-smp8758.dtsi
> index 7ed88ee629fb..44d57c02e934 100644
> --- a/arch/arm/boot/dts/tango4-smp8758.dtsi
> +++ b/arch/arm/boot/dts/tango4-smp8758.dtsi
> @@ -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";
> +			reg = <0x920100 12>;
> +		};
> +	};
> +
> +	thermal-zones {
> +		cpu_thermal: cpu-thermal {
> +			polling-delay-passive = <2003>;	/* ms */
> +			polling-delay = <1009>;		/* ms */
> +			thermal-sensors = <&cpu_temp>;
> +		};

Please add all the required properties for a thermal zone (check the
Documentation for examples or existing dtsi as I mentioned before).

Also, send the diff that adds the dtsi changes into a separated patch.
This recommendation is to avoid conflicts with the tango tree.
I typically I apply the driver changes, and the maintainer of your
platform would apply the DT changes.

BR,

Eduardo Valentin

Re: [PATCH v7] thermal: add temperature sensor support for tango SoC

[Mason]

On 04/04/2016 13:49, Mason wrote:

> +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.

> +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);
> +
> +	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.

Re: [PATCH v7] thermal: add temperature sensor support for tango SoC

[Eduardo Valentin]

Hello Mason,

On Tue, Apr 05, 2016 at 04:58:22PM +0200, Mason wrote:
> On 04/04/2016 13:49, Mason wrote:
> 
> > +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.

OK.

> 
> > +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);
> > +
> > +	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.
> 


This is correct understanding. The driver is expected to be ready to
answer any of the .ops calls during the registration. Specially if you
have cooling devices that may be bound to the thermal zone, then you
would have more than one calls.

With that said, I would suggest to move the line
+	priv->thresh_idx = IDX_MIN;

up, yes.

> 
> 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?


Yes, .get_temp() is called under tz->lock, in both cases, sysfs and
thread polling.  Of course, if you have extra data used in other context
that thermal core cannot see, than you would need extra locking. But if
you are only using your data in .get_temp(), which seams the case for
you, I would not see a need for an extra lock.

> 
> 
> 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

Well, for temperature testing, that should be enough to see if driver is
returning temperature. 

As for the temperature value validation, you would
probably need to instrument your setup with either thermal couples or ir
sensing to make sure your driver is reporting correct values.

Apart from temperature testing, do you have any cooling device registered in your system?

Do you see cooling taking action?

> 
> 
> Regards.
> 

Re: [PATCH v7] thermal: add temperature sensor support for tango SoC

[Eduardo Valentin]

On Mon, Apr 04, 2016 at 01:49:44PM +0200, Mason wrote:
> From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> 
> 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.

<cut>

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

I would still prefer you use the devm_ version.

BR,

Eduardo

Re: [PATCH v7] thermal: add temperature sensor support for tango SoC

[Mason]

On 06/04/2016 17:51, Eduardo Valentin wrote:

> On Mon, Apr 04, 2016 at 01:49:44PM +0200, Mason wrote:
> 
>> +	priv->zone = thermal_zone_of_sensor_register(&pdev->dev, 0, priv, &ops);
> 
> I would still prefer you use the devm_ version.

In the .remove callback, I call thermal_zone_of_sensor_unregister()
and then the sensor is powered down.

I don't know when the devm garbage collector kicks in.
Is it before or after calling .remove?
(I suspect it is *after*, when the driver is detached.)

I can't power the sensor down until it has been "unregistered".
If .get_temp is called with the sensor powered down, the HW
will return garbage, which might have weird consequences.

The documentation states
https://www.kernel.org/doc/Documentation/thermal/sysfs-api.txt

devm_thermal_zone_of_sensor_register()
"The benefit of using this interface to register sensor is that it
is not require to explicitly call thermal_zone_of_sensor_unregister()"

So I can still explicitly call thermal_zone_of_sensor_unregister?

What is the point of devm_thermal_zone_of_sensor_unregister?

Regards.

[PATCH v8 1/2] thermal: add temperature sensor support for tango SoC

[Mason]

From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>

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 <marc_gonzalez@sigmadesigns.com>
---
 Documentation/devicetree/bindings/thermal/tango-thermal.txt |  17 ++++
 drivers/thermal/Kconfig                                     |   9 +++
 drivers/thermal/Makefile                                    |   1 +
 drivers/thermal/tango_thermal.c                             | 111 +++++++++++++++++++++++++++
 4 files changed, 138 insertions(+)

diff --git a/Documentation/devicetree/bindings/thermal/tango-thermal.txt b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
new file mode 100644
index 000000000000..212198d4b937
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/tango-thermal.txt
@@ -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>;
+	};
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index c37eedc35a24..1c9ab79fa3e0 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -260,6 +260,15 @@ 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 TANGO_THERMAL
+	tristate "Tango thermal management"
+	depends on ARCH_TANGO || COMPILE_TEST
+	help
+	  Enable the Tango thermal driver, which supports 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.
+
 config TEGRA_SOCTHERM
 	tristate "Tegra SOCTHERM thermal management"
 	depends on ARCH_TEGRA
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 8e9cbc3b5679..06387279883d 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -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
diff --git a/drivers/thermal/tango_thermal.c b/drivers/thermal/tango_thermal.c
new file mode 100644
index 000000000000..d6592bfbd057
--- /dev/null
+++ b/drivers/thermal/tango_thermal.c
@@ -0,0 +1,111 @@
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/thermal.h>
+#include <linux/platform_device.h>
+
+/*
+ * According to a data sheet draft, "this temperature 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 41 different temperature thresholds to be selected
+ * and the logic output will then indicate whether the actual die temperature
+ * lies above or below the selected threshold."
+ */
+
+#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		15
+#define IDX_MAX		40
+
+struct tango_thermal_priv {
+	void __iomem *base;
+	int thresh_idx;
+};
+
+static bool temp_above_thresh(void __iomem *base, int thresh_idx)
+{
+	writel(CMD_READ | thresh_idx << 8, base + TEMPSI_CMD);
+	usleep_range(10, 20);
+	writel(CMD_READ | thresh_idx << 8, base + TEMPSI_CMD);
+
+	return readl(base + TEMPSI_RES);
+}
+
+static int tango_get_temp(void *arg, int *res)
+{
+	struct tango_thermal_priv *priv = arg;
+	int idx = priv->thresh_idx;
+
+	if (temp_above_thresh(priv->base, idx)) {
+		/* Search upward by incrementing thresh_idx */
+		while (idx < IDX_MAX && temp_above_thresh(priv->base, ++idx))
+			cpu_relax();
+		idx = idx - 1; /* always return lower bound */
+	} else {
+		/* Search downward by decrementing thresh_idx */
+		while (idx > IDX_MIN && !temp_above_thresh(priv->base, --idx))
+			cpu_relax();
+	}
+
+	*res = (idx * 9 / 2 - 38) * 1000; /* millidegrees Celsius */
+	priv->thresh_idx = idx;
+
+	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 thermal_zone_device *tzdev;
+
+	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);
+
+	priv->thresh_idx = IDX_MIN;
+	writel(CMD_ON, priv->base + TEMPSI_CMD);
+
+	tzdev = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, priv, &ops);
+	if (IS_ERR(tzdev))
+		return PTR_ERR(tzdev);
+
+	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,
+	.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");
-- 
2.8.1

Re: [PATCH v8 1/2] thermal: add temperature sensor support for tango SoC

[Mason]

On 19/04/2016 16:21, Mason wrote:

> 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.

Tested with the following script:

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

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

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

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


RUN IDLE
43000
47000
47000
43000
52000
52000
52000
52000
47000
43000
47000
47000
47000
52000
52000
52000
52000
52000
52000
47000
43000
47000
43000
47000
43000
43000
52000
43000
52000
52000
RUN HEAVY LOAD
52000
61000
56000
61000
65000
65000
65000
65000
65000
65000
65000
65000
65000
61000
65000
65000
61000
65000
65000
65000
65000
65000
65000
65000
61000
65000
65000
65000
65000
65000
65000
65000
61000
65000
70000
70000
70000
70000
70000
70000
70000
70000
70000
70000
70000
70000
70000
70000
70000
70000
70000
74000
70000
70000
70000
74000
70000
74000
70000
70000
KILL HEAVY LOAD
70000
./thermo2.sh: line 14:   972 Terminated              cpuburn-a9
./thermo2.sh: line 14:   973 Terminated              cpuburn-a9
61000
61000
56000
56000
61000
56000
61000
56000
56000
56000
56000
56000
56000
56000
56000
56000
56000
56000
56000
56000
52000
52000
52000
52000
52000
47000
52000
47000
52000

Regards.

[PATCH v8 2/2] ARM: dts: tango4: Initial thermal support

[Mason]

From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>

Define the CPU temperature sensor, and critical trip point.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 arch/arm/boot/dts/tango4-smp8758.dtsi | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm/boot/dts/tango4-smp8758.dtsi b/arch/arm/boot/dts/tango4-smp8758.dtsi
index 7ed88ee629fb..01d5f8f1c71d 100644
--- a/arch/arm/boot/dts/tango4-smp8758.dtsi
+++ b/arch/arm/boot/dts/tango4-smp8758.dtsi
@@ -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";
+				};
+			};
+		};
+	};
 };
-- 
2.8.1

Re: [PATCH v8 2/2] ARM: dts: tango4: Initial thermal support

[Eduardo Valentin]

On Tue, Apr 19, 2016 at 04:32:47PM +0200, Mason wrote:
> From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> 
> Define the CPU temperature sensor, and critical trip point.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>

Acked-by: Eduardo Valentin <edubezval@gmail.com>

> ---
>  arch/arm/boot/dts/tango4-smp8758.dtsi | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/tango4-smp8758.dtsi b/arch/arm/boot/dts/tango4-smp8758.dtsi
> index 7ed88ee629fb..01d5f8f1c71d 100644
> --- a/arch/arm/boot/dts/tango4-smp8758.dtsi
> +++ b/arch/arm/boot/dts/tango4-smp8758.dtsi
> @@ -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";
> +				};
> +			};
> +		};
> +	};
>  };
> -- 
> 2.8.1

Re: [PATCH v5] thermal: add temperature sensor support for tango SoC

[Eduardo Valentin]

Hello Mason,

> 
> The SMP8758 has the 3. The next SoC has 5, in different HW blocks.
> HW team likes to move them around, apparently.
> 
> > 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.

Ok, I see. Then, following the design you chose to use (of-thermal
based), determining what each sensor represents is typically solved by
describing them in device tree.

You did already a good job for the sensor part, but your patch (even v6)
do not include any thermal zone definition. So, what to do from here?

whenever possible, it is common that, together with your driver code,
you also do the device tree changes. Typically, for thermal zones,
people have been adding them on .dtsi files, so that they can be reused
on board files (dts).

You should be able to get examples from 
Documentation/devicetree/bindings/thermal/thermal.txt

or do a git grep thermal-zone arch/arm/boot/dts/, and you should be able
to see how people are doing this, consistently. But some chips can get
several zones though.


> 
> >> 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.

Ok.

> 
> > 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."

I see.

> 
> > 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.

No problem. 

> 
> Regards.
>