diff mbox

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

Message ID 56F91A47.9060901@free.fr (mailing list archive)
State Superseded, archived
Delegated to: Eduardo Valentin
Headers show

Commit Message

Mason March 28, 2016, 11:49 a.m. UTC
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(+)

Comments

Eduardo Valentin March 29, 2016, 2 a.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mason March 29, 2016, 6:48 p.m. UTC | #2
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eduardo Valentin March 30, 2016, 12:05 a.m. UTC | #3
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.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mason March 30, 2016, 3:18 p.m. UTC | #4
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eduardo Valentin April 1, 2016, 1:48 a.m. UTC | #5
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.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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