| Message ID | 570254D8.3090406@free.fr (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Eduardo Valentin |
| Headers | show |
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 -- 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
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. -- 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
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. > -- 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
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 -- 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
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. -- 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 --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");