thermal: add imx thermal driver support
Subsystems (3)
PATCH v2 1/1 thermal: add imx thermal driver support
This is based on the initial imx thermal work done by
Rob Lee <[email protected]> (Not sure if the email address is still valid). Since he is no longer interested in the work and I have rewritten a significant amount of the code, I just took the authorship over from him.
It adds the imx thermal support using Temperature Monitor (TEMPMON) block found on some Freescale i.MX SoCs. The driver uses syscon regmap interface to access TEMPMON control registers and calibration data, and supports cpufreq as the cooling device.
---
Changes since v1:
* Change trip type to passive.
.../devicetree/bindings/thermal/imx-thermal.txt | 14 + drivers/thermal/Kconfig | 8 + drivers/thermal/Makefile | 1 + drivers/thermal/imx_thermal.c | 429 ++++++++++++++++++++ 4 files changed, 452 insertions(+) create mode 100644 Documentation/devicetree/bindings/thermal/imx-thermal.txt create mode 100644 drivers/thermal/imx_thermal.c
Documentation/devicetree/bindings/thermal/imx-thermal.txt new file +14 -0
@@ -0,0 +1,14 @@ +* Temperature Monitor (TEMPMON) on Freescale i.MX SoCs + +Required properties: +- compatible : "fsl,imx6q-thermal" +- fsl,tempmon : phandle pointer to TEMPMON control registers +- fsl,tempmon-data : phandle pointer to TEMPMON calibration data + +Example: + +tempmon { + compatible = "fsl,imx6q-tempmon"; + fsl,tempmon = <&anatop>; + fsl,tempmon-data = <&ocotp>; +};
drivers/thermal/Kconfig +8 -0
@@ -91,6 +91,14 @@ config THERMAL_EMULATION because userland can easily disable the thermal policy by simply flooding this sysfs node with low temperature values. +config IMX_THERMAL + tristate "Temperature sensor driver for Freescale i.MX SoCs" + depends on CPU_THERMAL + depends on MFD_SYSCON + depends on OF + help + Support for Temperature Monitor (TEMPMON) found on Freescale i.MX SoCs. + config SPEAR_THERMAL bool "SPEAr thermal sensor driver" depends on PLAT_SPEAR
drivers/thermal/Makefile +1 -0
@@ -21,6 +21,7 @@ obj-$(CONFIG_EXYNOS_THERMAL) += exynos_thermal.o obj-$(CONFIG_DOVE_THERMAL) += dove_thermal.o obj-$(CONFIG_DB8500_THERMAL) += db8500_thermal.o obj-$(CONFIG_ARMADA_THERMAL) += armada_thermal.o +obj-$(CONFIG_IMX_THERMAL) += imx_thermal.o obj-$(CONFIG_DB8500_CPUFREQ_COOLING) += db8500_cpufreq_cooling.o obj-$(CONFIG_INTEL_POWERCLAMP) += intel_powerclamp.o
drivers/thermal/imx_thermal.c new file +429 -0
429 lines not on this page · raw message
Signature · 1 line
1.7.9.5
PATCH v2 1/1 thermal: add imx thermal driver support
Quoted 30 lines · Shawn Guo
This is based on the initial imx thermal work done by
Rob Lee <[email protected]> (Not sure if the email address is still valid). Since he is no longer interested in the work and I have rewritten a significant amount of the code, I just took the authorship over from him.It adds the imx thermal support using Temperature Monitor (TEMPMON) block found on some Freescale i.MX SoCs. The driver uses syscon regmap interface to access TEMPMON control registers and calibration data, and supports cpufreq as the cooling device.
Signed-off-by Shawn Guo <[email protected]>---
Changes since v1:
* Change trip type to passive..../devicetree/bindings/thermal/imx-thermal.txt | 14 + drivers/thermal/Kconfig | 8 + drivers/thermal/Makefile | 1 + drivers/thermal/imx_thermal.c | 429 ++++++++++++++++++++ 4 files changed, 452 insertions(+) create mode 100644 Documentation/devicetree/bindings/thermal/imx-thermal.txt create mode 100644 drivers/thermal/imx_thermal.cDocumentation/devicetree/bindings/thermal/imx-thermal.txt new file +0
-0
Documentation/devicetree/bindings/thermal/imx-thermal.txt line 1
Quoted 1 line
Documentation/devicetree/bindings/thermal/imx-thermal.txt new file +1
-0@@ -0,0 +1,14 @@ +* Temperature Monitor (TEMPMON) on Freescale i.MX SoCs
Any hw documentation reference?
Documentation/devicetree/bindings/thermal/imx-thermal.txt line 6
Quoted 5 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt new file +5
-0+ +Required properties: +- compatible : "fsl,imx6q-thermal" +- fsl,tempmon : phandle pointer to TEMPMON control registers +- fsl,tempmon-data : phandle pointer to TEMPMON calibration data
what exactly is calibration data?
Documentation/devicetree/bindings/thermal/imx-thermal.txt line 14
Quoted 8 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt new file +8
-0+ +Example: + +tempmon { + compatible = "fsl,imx6q-tempmon"; + fsl,tempmon = <&anatop>; + fsl,tempmon-data = <&ocotp>; +};
Why do you need specific DT bindings for things like registers?
Quoted 6 lines
drivers/thermal/Kconfig +0
-0@@ -91,6 +91,14 @@ config THERMAL_EMULATION because userland can easily disable the thermal policy by simply
drivers/thermal/Kconfig config THERMAL_EMULATION · line 101
Quoted 10 lines
drivers/thermal/Kconfig +8
-0flooding this sysfs node with low temperature values. +config IMX_THERMAL + tristate "Temperature sensor driver for Freescale i.MX SoCs" + depends on CPU_THERMAL + depends on MFD_SYSCON + depends on OF + help + Support for Temperature Monitor (TEMPMON) found on Freescale i.MX SoCs. +
I guess this help generates a warning (too short) by checkpatch.pl.
Quoted 21 lines
drivers/thermal/Kconfig +0
-0config SPEAR_THERMAL bool "SPEAr thermal sensor driver" depends on PLAT_SPEARdrivers/thermal/Makefile +1
-0@@ -21,6 +21,7 @@ obj-$(CONFIG_EXYNOS_THERMAL) += exynos_thermal.o obj-$(CONFIG_DOVE_THERMAL) += dove_thermal.o obj-$(CONFIG_DB8500_THERMAL) += db8500_thermal.o obj-$(CONFIG_ARMADA_THERMAL) += armada_thermal.o +obj-$(CONFIG_IMX_THERMAL) += imx_thermal.o obj-$(CONFIG_DB8500_CPUFREQ_COOLING) += db8500_cpufreq_cooling.o obj-$(CONFIG_INTEL_POWERCLAMP) += intel_powerclamp.odrivers/thermal/imx_thermal.c new file +0
-0
drivers/thermal/imx_thermal.c line 3
Quoted 3 lines
drivers/thermal/imx_thermal.c new file +3
-0@@ -0,0 +1,429 @@ +/* +* Copyright 2013 Freescale Semiconductor, Inc. +*
No contact entry?
Quoted 52 lines
drivers/thermal/imx_thermal.c new file +52
-0+* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +* +*/ + +#include <linux/cpu_cooling.h> +#include <linux/cpufreq.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/slab.h> +#include <linux/thermal.h> +#include <linux/types.h> + +#define REG_SET 0x4 +#define REG_CLR 0x8 +#define REG_TOG 0xc + +#define MISC0 0x0150 +#define MISC0_REFTOP_SELBIASOFF (1 << 3) + +#define TEMPSENSE0 0x0180 +#define TEMPSENSE0_TEMP_CNT_SHIFT 8 +#define TEMPSENSE0_TEMP_CNT_MASK (0xfff << TEMPSENSE0_TEMP_CNT_SHIFT) +#define TEMPSENSE0_FINISHED (1 << 2) +#define TEMPSENSE0_MEASURE_TEMP (1 << 1) +#define TEMPSENSE0_POWER_DOWN (1 << 0) + +#define TEMPSENSE1 0x0190 +#define TEMPSENSE1_MEASURE_FREQ 0xffff + +#define OCOTP_ANA1 0x04e0 + +/* The driver supports 1 passive trip point and 1 critical trip point */ +enum imx_thermal_trip { +IMX_TRIP_PASSIVE , +IMX_TRIP_CRITICAL , +IMX_TRIP_NUM , +}; + +/* +* It defines the temperature in millicelsius for passive trip point +* that will trigger cooling action when crossed. +*/
drivers/thermal/imx_thermal.c line 65
Quoted 10 lines
drivers/thermal/imx_thermal.c new file +10
-0+#define IMX_TEMP_PASSIVE 85000 + +/* +* The maximum die temperature on imx parts is 105C, let's give some cushion +* for noise and possible temperature rise between measurements. +*/ +#define IMX_TEMP_CRITICAL 100000 + +#define IMX_POLLING_DELAY 5000/* millisecond */ +#define IMX_PASSIVE_DELAY 3000
What is your maximum delta between CPU frequencies? Sure this polling intervals can hold your maximum power burst?
Quoted 13 lines
drivers/thermal/imx_thermal.c new file +13
-0+ +struct imx_thermal_data { +struct thermal_zone_device *tz; +struct thermal_cooling_device *cdev; +enum thermal_device_mode mode; +struct regmap *tempmon; +bool meas_suspended; +int c1, c2;/* See forumla in imx_get_sensor_data() */ +}; + +static int imx_get_temp (struct thermal_zone_device *tz,unsigned long *temp) +{ +struct imx_thermal_data *data = tz->devdata;
drivers/thermal/imx_thermal.c line 88
Quoted 10 lines
drivers/thermal/imx_thermal.c new file +10
-0+struct regmap *map = data->tempmon; +static unsigned long last_temp; +unsigned int n_meas; +u32 val; + +/* +* Every time we measure the temperature, we will power on the +* temperature sensor, enable measurements, take a reading, +* disable measurements, power off the temperature sensor. +*/
Are these sensors used on any hw failsafe mechanism?
drivers/thermal/imx_thermal.c line 99
Quoted 11 lines
drivers/thermal/imx_thermal.c new file +11
-0+regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_POWER_DOWN ); +regmap_write (map,TEMPSENSE0 +REG_SET ,TEMPSENSE0_MEASURE_TEMP ); + +/* +* According to the temp sensor designers, it may require up to ~17us +* to complete a measurement. But this timing isn't checked on every +* part nor is it specified in the datasheet, so sleeping at least 1ms +* should provide plenty of time. Sleeping longer than 1ms is ok so no +* need for usleep_range. +*/ +msleep (1 );
I know 1ms is usually too short time slice when talking about thermal monitoring on nowadays processors even, but 17us to 1ms sounds like an overkill.. Sure you are not hiding other issues?
Quoted 81 lines
drivers/thermal/imx_thermal.c new file +81
-0+ +regmap_read (map,TEMPSENSE0 , &val); +regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_MEASURE_TEMP ); +regmap_write (map,TEMPSENSE0 +REG_SET ,TEMPSENSE0_POWER_DOWN ); + +if ((val &TEMPSENSE0_FINISHED ) ==0 ) { +dev_dbg (&tz->device,"temp measurement never finished\n" ); +return -EAGAIN ; + } + + n_meas = (val &TEMPSENSE0_TEMP_CNT_MASK ) >>TEMPSENSE0_TEMP_CNT_SHIFT ; + +/* See imx_get_sensor_data() for forumla derivation */ + *temp = data->c2 + data->c1 * n_meas; + +if (*temp != last_temp) { +dev_dbg (&tz->device,"millicelsius: %ld\n" , *temp); + last_temp = *temp; + } + +return 0 ; +} + +static int imx_get_mode (struct thermal_zone_device *tz, +enum thermal_device_mode *mode) +{ +struct imx_thermal_data *data = tz->devdata; + + *mode = data->mode; + +return 0 ; +} + +static int imx_set_mode (struct thermal_zone_device *tz, +enum thermal_device_mode mode) +{ +struct imx_thermal_data *data = tz->devdata; + +if (mode ==THERMAL_DEVICE_ENABLED ) { + tz->polling_delay =IMX_POLLING_DELAY ; + tz->passive_delay =IMX_PASSIVE_DELAY ; + }else { + tz->polling_delay =0 ; + tz->passive_delay =0 ; + } + + data->mode = mode; +thermal_zone_device_update (tz); + +return 0 ; +} + +static int imx_get_trip_type (struct thermal_zone_device *tz,int trip, +enum thermal_trip_type *type) +{ + *type = (trip ==IMX_TRIP_PASSIVE ) ?THERMAL_TRIP_PASSIVE : +THERMAL_TRIP_CRITICAL ; +return 0 ; +} + +static int imx_get_crit_temp (struct thermal_zone_device *tz, +unsigned long *temp) +{ + *temp =IMX_TEMP_CRITICAL ; +return 0 ; +} + +static int imx_get_trip_temp (struct thermal_zone_device *tz,int trip, +unsigned long *temp) +{ + *temp = (trip ==IMX_TRIP_PASSIVE ) ?IMX_TEMP_PASSIVE : +IMX_TEMP_CRITICAL ; +return 0 ; +} + +static int imx_bind (struct thermal_zone_device *tz, +struct thermal_cooling_device *cdev) +{ +int ret; + + ret = thermal_zone_bind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev,
drivers/thermal/imx_thermal.c line 190
Quoted 10 lines
drivers/thermal/imx_thermal.c new file +10
-0+THERMAL_NO_LIMIT , + THERMAL_NO_LIMIT); +if (ret) { +dev_err (&tz->device, +"binding zone %s with cdev %s failed:%d\n" , + tz->type, cdev->type, ret); +return ret; + } + +return 0 ;
nit: I think it is just safe to do:
+ ret = thermal_zone_bind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev,
+ THERMAL_NO_LIMIT,
+ THERMAL_NO_LIMIT);
+ if (ret)
+ dev_err(&tz->device,
+ "binding zone %s with cdev %s failed:%d\n",
+ tz->type, cdev->type, ret);
+
+ return ret;
Quoted 6 lines
drivers/thermal/imx_thermal.c new file +6
-0+} + +static int imx_unbind (struct thermal_zone_device *tz, +struct thermal_cooling_device *cdev) +{ +int ret;
drivers/thermal/imx_thermal.c line 206
Quoted 10 lines
drivers/thermal/imx_thermal.c new file +10
-0+ + ret =thermal_zone_unbind_cooling_device (tz,IMX_TRIP_PASSIVE , cdev); +if (ret) { +dev_err (&tz->device, +"unbinding zone %s with cdev %s failed:%d\n" , + tz->type, cdev->type, ret); +return ret; + } + +return 0 ;
ditto.
Quoted 72 lines
drivers/thermal/imx_thermal.c new file +72
-0+} + +static const struct thermal_zone_device_ops imx_tz_ops = { + .bind = imx_bind, + .unbind = imx_unbind, + .get_temp = imx_get_temp, + .get_mode = imx_get_mode, + .set_mode = imx_set_mode, + .get_trip_type = imx_get_trip_type, + .get_trip_temp = imx_get_trip_temp, + .get_crit_temp = imx_get_crit_temp, +}; + +static int imx_get_sensor_data (struct platform_device *pdev) +{ +struct imx_thermal_data *data =platform_get_drvdata (pdev); +struct regmap *map; +int t1, t2, n1, n2; +int ret; +u32 val; + + map =syscon_regmap_lookup_by_phandle (pdev->dev.of_node, +"fsl,tempmon-data" ); +if (IS_ERR (map)) { + ret =PTR_ERR (map); +dev_err (&pdev->dev,"failed to get sensor regmap: %d\n" , ret); +return ret; + } + + ret =regmap_read (map,OCOTP_ANA1 , &val); +if (ret) { +dev_err (&pdev->dev,"failed to read sensor data: %d\n" , ret); +return ret; + } + +if (val ==0 || val == ~0 ) { +dev_err (&pdev->dev,"invalid sensor calibration data\n" ); +return -EINVAL ; + } + +/* +* Sensor data layout: +* [31:20] - sensor value @ 25C +* [19:8] - sensor value of hot +* [7:0] - hot temperature value +*/ + n1 = val >>20 ; + n2 = (val &0xfff00 ) >>8 ; + t2 = val &0xff ; + t1 =25 ;/* t1 always 25C */ + +/* +* Derived from linear interpolation, +* Tmeas = T2 + (Nmeas - N2) * (T1 - T2) / (N1 - N2) +* We want to reduce this down to the minimum computation necessary +* for each temperature read. Also, we want Tmeas in millicelsius +* and we don't want to lose precision from integer division. So... +* milli_Tmeas = 1000 * T2 + 1000 * (Nmeas - N2) * (T1 - T2) / (N1 - N2) +* Let constant c1 = 1000 * (T1 - T2) / (N1 - N2) +* milli_Tmeas = (1000 * T2) + c1 * (Nmeas - N2) +* milli_Tmeas = (1000 * T2) + (c1 * Nmeas) - (c1 * N2) +* Let constant c2 = (1000 * T2) - (c1 * N2) +* milli_Tmeas = c2 + (c1 * Nmeas) +*/ + data->c1 =1000 * (t1 - t2) / (n1 - n2); + data->c2 =1000 * t2 - data->c1 * n2; + +return 0 ; +} + +static int imx_thermal_probe (struct platform_device *pdev) +{
drivers/thermal/imx_thermal.c line 288
Quoted 10 lines
drivers/thermal/imx_thermal.c new file +10
-0+struct imx_thermal_data *data; +struct cpumask clip_cpus; +struct regmap *map; +int ret; + + data =devm_kzalloc (&pdev->dev,sizeof (*data),GFP_KERNEL ); +if (!data) +return -ENOMEM ; + + map =syscon_regmap_lookup_by_phandle (pdev->dev.of_node,"fsl,tempmon" );
Is this MMIO? Sure you want to do regmapped access to something so simple as io mem r/w?
Quoted 64 lines
drivers/thermal/imx_thermal.c new file +64
-0+if (IS_ERR (map)) { + ret =PTR_ERR (map); +dev_err (&pdev->dev,"failed to get tempmon regmap: %d\n" , ret); +return ret; + } + data->tempmon = map; + +platform_set_drvdata (pdev, data); + + ret =imx_get_sensor_data (pdev); +if (ret) { +dev_err (&pdev->dev,"failed to get sensor data\n" ); +return ret; + } + +/* Make sure sensor is in known good state for measurements */ +regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_POWER_DOWN ); +regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_MEASURE_TEMP ); +regmap_write (map,TEMPSENSE1 +REG_CLR ,TEMPSENSE1_MEASURE_FREQ ); +regmap_write (map,MISC0 +REG_SET ,MISC0_REFTOP_SELBIASOFF ); +regmap_write (map,TEMPSENSE0 +REG_SET ,TEMPSENSE0_POWER_DOWN ); + +cpumask_set_cpu (0 , &clip_cpus); + data->cdev =cpufreq_cooling_register (&clip_cpus); +if (IS_ERR (data->cdev)) { + ret =PTR_ERR (data->cdev); +dev_err (&pdev->dev, +"failed to register cpufreq cooling device: %d\n" , ret); +return ret; + } + + data->tz =thermal_zone_device_register ("imx_thermal_zone" , +IMX_TRIP_NUM ,0 , data, + &imx_tz_ops,NULL , +IMX_PASSIVE_DELAY , +IMX_POLLING_DELAY ); +if (IS_ERR (data->tz)) { + ret =PTR_ERR (data->tz); +dev_err (&pdev->dev, +"failed to register thermal zone device %d\n" , ret); +cpufreq_cooling_unregister (data->cdev); +return ret; + } + + data->mode =THERMAL_DEVICE_ENABLED ; + +return 0 ; +} + +static int imx_thermal_remove (struct platform_device *pdev) +{ +struct imx_thermal_data *data =platform_get_drvdata (pdev); + +thermal_zone_device_unregister (data->tz); +cpufreq_cooling_unregister (data->cdev); + +return 0 ; +} + +#ifdef CONFIG_PM_SLEEP +static int imx_thermal_suspend (struct device *dev) +{ +struct imx_thermal_data *data =dev_get_drvdata (dev); +struct regmap *map = data->tempmon;
drivers/thermal/imx_thermal.c line 362
Quoted 10 lines
drivers/thermal/imx_thermal.c new file +10
-0+u32 val; + +regmap_read (map,TEMPSENSE0 , &val); + +/* Was a measurement taking place? If not, nothing to do. */ +if (val &TEMPSENSE0_POWER_DOWN ) +return 0 ; + +regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_MEASURE_TEMP ); +regmap_write (map,TEMPSENSE0 +REG_SET ,TEMPSENSE0_POWER_DOWN );
hmmm.. Whould this mess with the running meas? Any luck to wait it to finish then suspend?
Quoted 15 lines
drivers/thermal/imx_thermal.c new file +15
-0+ + data->meas_suspended = true; + +return 0 ; +} + +static int imx_thermal_resume (struct device *dev) +{ +struct imx_thermal_data *data =dev_get_drvdata (dev); +struct regmap *map = data->tempmon; + +/* +* If a measurement was taking place while suspend, re-take the +* measurement. +*/
drivers/thermal/imx_thermal.c line 387
Quoted 10 lines
drivers/thermal/imx_thermal.c new file +10
-0+if (data->meas_suspended) { +regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_POWER_DOWN ); +regmap_write (map,TEMPSENSE0 +REG_SET ,TEMPSENSE0_MEASURE_TEMP ); +/* +* According to the temp sensor designers, it may require +* up to ~17us to complete a measurement. But this timing +* isn't checked on every part nor is it specified in the +* datasheet, so delay 50us for timing margin. +*/ +udelay (50 );
hmm.. here you need 50us only but on regular readings 1ms?
Quoted 27 lines
drivers/thermal/imx_thermal.c new file +27
-0+ data->meas_suspended = false; + } + +return 0 ; +} +#endif + +static SIMPLE_DEV_PM_OPS(imx_thermal_pm_ops, + imx_thermal_suspend, imx_thermal_resume); + +static const struct of_device_id of_imx_thermal_match[] = { + { .compatible ="fsl,imx6q-tempmon" , }, + {/* end */ } +}; + +static struct platform_driver imx_thermal = { + .driver = { + .name ="imx_thermal" , + .owner =THIS_MODULE , + .pm = &imx_thermal_pm_ops, + .of_match_table = of_imx_thermal_match, + }, + .probe = imx_thermal_probe, + .remove = imx_thermal_remove, +}; + +static int __init imx_thermal_init (void )
drivers/thermal/imx_thermal.c line 424
Quoted 10 lines
drivers/thermal/imx_thermal.c new file +10
-0+{ +return platform_driver_register (&imx_thermal); +} +late_initcall (imx_thermal_init); + +static void __exit imx_thermal_exit(void) +{ +platform_driver_unregister (&imx_thermal); +} +module_exit (imx_thermal_exit);
How about using module_platform_driver() macro?
Quoted 6 lines
drivers/thermal/imx_thermal.c new file +5
-0+ +MODULE_AUTHOR ("Freescale Semiconductor, Inc." ); +MODULE_DESCRIPTION ("Thermal driver for Freescale i.MX SoCs" ); +MODULE_LICENSE ("GPL" ); +MODULE_ALIAS ("platform:imx-thermal" );
Signature · 11 lines
You have got to be excited about what you are doing. (L. Lamport) Eduardo Valentin -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 295 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130618/eb2cd199/attachment.sig>
PATCH v2 1/1 thermal: add imx thermal driver support
Thanks for the review effort, Eduardo.
Quoted 25 lines · Eduardo Valentin
Quoted 24 lines · Shawn Guo
This is based on the initial imx thermal work done by
Rob Lee <[email protected]> (Not sure if the email address is still valid). Since he is no longer interested in the work and I have rewritten a significant amount of the code, I just took the authorship over from him.It adds the imx thermal support using Temperature Monitor (TEMPMON) block found on some Freescale i.MX SoCs. The driver uses syscon regmap interface to access TEMPMON control registers and calibration data, and supports cpufreq as the cooling device.
Signed-off-by Shawn Guo <[email protected]>---
Changes since v1:
* Change trip type to passive..../devicetree/bindings/thermal/imx-thermal.txt | 14 + drivers/thermal/Kconfig | 8 + drivers/thermal/Makefile | 1 + drivers/thermal/imx_thermal.c | 429 ++++++++++++++++++++ 4 files changed, 452 insertions(+) create mode 100644 Documentation/devicetree/bindings/thermal/imx-thermal.txt create mode 100644 drivers/thermal/imx_thermal.c
Quoted 10 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt line 1
Quoted 7 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt new file +1
-0@@ -0,0 +1,14 @@ +* Temperature Monitor (TEMPMON) on Freescale i.MX SoCsAny hw documentation reference?
Quoted 8 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt line 6
Quoted 5 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt new file +5
-0+ +Required properties: +- compatible : "fsl,imx6q-thermal" +- fsl,tempmon : phandle pointer to TEMPMON control registers +- fsl,tempmon-data : phandle pointer to TEMPMON calibration datawhat exactly is calibration data?
The temperature calibration values are fused individually for each part in the product testing process. It consists of a pair of hardware monitor values at 25 C and a high temperature which is specified by calibration data itself, which will be used by software to translate hardware counter values to Celsius. The detailed description can be found in hardware reference manual.
Quoted 11 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt line 14
Quoted 8 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt new file +8
-0+ +Example: + +tempmon { + compatible = "fsl,imx6q-tempmon"; + fsl,tempmon = <&anatop>; + fsl,tempmon-data = <&ocotp>; +};Why do you need specific DT bindings for things like registers?
Because there is no dedicated IO space for the block. Instead, the registers spread in system controllers which contain miscellaneous control and data registers. So we chose to access the registers via syscon regmap interface, and we need phandle pointer to the correct syscon device node.
Quoted 8 lines
Quoted 8 lines
drivers/thermal/Kconfig +0
-0@@ -91,6 +91,14 @@ config THERMAL_EMULATION because userland can easily disable the thermal policy by simply flooding this sysfs node with low temperature values.
Quoted 11 lines
drivers/thermal/Kconfig config THERMAL_EMULATION · line 101
Quoted 8 lines
drivers/thermal/Kconfig +8
-0+config IMX_THERMAL + tristate "Temperature sensor driver for Freescale i.MX SoCs" + depends on CPU_THERMAL + depends on MFD_SYSCON + depends on OF + help + Support for Temperature Monitor (TEMPMON) found on Freescale i.MX SoCs. +I guess this help generates a warning (too short) by checkpatch.pl.
Yes. But from what I've seen, most maintainers are not so concerned by this particular checkpatch warning. If you are really concerned by it, I can surely manage to write up something longer.
Quoted 15 lines
Quoted 15 lines
drivers/thermal/Kconfig +0
-0config SPEAR_THERMAL bool "SPEAr thermal sensor driver" depends on PLAT_SPEARdrivers/thermal/Makefile +1
-0@@ -21,6 +21,7 @@ obj-$(CONFIG_EXYNOS_THERMAL) += exynos_thermal.o obj-$(CONFIG_DOVE_THERMAL) += dove_thermal.o obj-$(CONFIG_DB8500_THERMAL) += db8500_thermal.o obj-$(CONFIG_ARMADA_THERMAL) += armada_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
Quoted 12 lines
drivers/thermal/imx_thermal.c line 3
Quoted 9 lines
drivers/thermal/imx_thermal.c new file +3
-0@@ -0,0 +1,429 @@ +/* +* Copyright 2013 Freescale Semiconductor, Inc. +* No contact entry?
I'm not a big fan of doing that, because we will need to patch the contact from time to time to keep it update to date. I've run into the situation a lot where the contract left in the file became invalid and git log helps give the recent/updated contact.
Quoted 57 lines
Quoted 57 lines
drivers/thermal/imx_thermal.c new file +57
-0+* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +* +*/ + +#include <linux/cpu_cooling.h> +#include <linux/cpufreq.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/slab.h> +#include <linux/thermal.h> +#include <linux/types.h> + +#define REG_SET 0x4 +#define REG_CLR 0x8 +#define REG_TOG 0xc + +#define MISC0 0x0150 +#define MISC0_REFTOP_SELBIASOFF (1 << 3) + +#define TEMPSENSE0 0x0180 +#define TEMPSENSE0_TEMP_CNT_SHIFT 8 +#define TEMPSENSE0_TEMP_CNT_MASK (0xfff << TEMPSENSE0_TEMP_CNT_SHIFT) +#define TEMPSENSE0_FINISHED (1 << 2) +#define TEMPSENSE0_MEASURE_TEMP (1 << 1) +#define TEMPSENSE0_POWER_DOWN (1 << 0) + +#define TEMPSENSE1 0x0190 +#define TEMPSENSE1_MEASURE_FREQ 0xffff + +#define OCOTP_ANA1 0x04e0 + +/* The driver supports 1 passive trip point and 1 critical trip point */ +enum imx_thermal_trip { +IMX_TRIP_PASSIVE , +IMX_TRIP_CRITICAL , +IMX_TRIP_NUM , +}; + +/* +* It defines the temperature in millicelsius for passive trip point +* that will trigger cooling action when crossed. +*/ +#define IMX_TEMP_PASSIVE 85000 + +/* +* The maximum die temperature on imx parts is 105C, let's give some cushion +* for noise and possible temperature rise between measurements.
Quoted 9 lines
drivers/thermal/imx_thermal.c line 65
Quoted 5 lines
drivers/thermal/imx_thermal.c new file +5
-0+*/ +#define IMX_TEMP_CRITICAL 100000 + +#define IMX_POLLING_DELAY 5000/* millisecond */ +#define IMX_PASSIVE_DELAY 3000What is your maximum delta between CPU frequencies? Sure this polling intervals can hold your maximum power burst?
I have to admit I'm not sure how to determine the optimal intervals here. I initially have both intervals be 1000 ms, but I'm not sure if it's necessary. I need some help on that. The frequency table on the target consists of 396, 792, 996 and 1200 in MHz.
Quoted 17 lines
Quoted 17 lines
drivers/thermal/imx_thermal.c new file +17
-0+ +struct imx_thermal_data { +struct thermal_zone_device *tz; +struct thermal_cooling_device *cdev; +enum thermal_device_mode mode; +struct regmap *tempmon; +bool meas_suspended; +int c1, c2;/* See forumla in imx_get_sensor_data() */ +}; + +static int imx_get_temp (struct thermal_zone_device *tz,unsigned long *temp) +{ +struct imx_thermal_data *data = tz->devdata; +struct regmap *map = data->tempmon; +static unsigned long last_temp; +unsigned int n_meas; +u32 val;
Quoted 9 lines
drivers/thermal/imx_thermal.c line 88
Quoted 6 lines
drivers/thermal/imx_thermal.c new file +6
-0+ +/* +* Every time we measure the temperature, we will power on the +* temperature sensor, enable measurements, take a reading, +* disable measurements, power off the temperature sensor. +*/ Are these sensors used on any hw failsafe mechanism?
No, I do not think so.
Quoted 7 lines
Quoted 7 lines
drivers/thermal/imx_thermal.c new file +7
-0+regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_POWER_DOWN ); +regmap_write (map,TEMPSENSE0 +REG_SET ,TEMPSENSE0_MEASURE_TEMP ); + +/* +* According to the temp sensor designers, it may require up to ~17us +* to complete a measurement. But this timing isn't checked on every +* part nor is it specified in the datasheet, so sleeping at least 1ms
Quoted 9 lines
drivers/thermal/imx_thermal.c line 99
Quoted 4 lines
drivers/thermal/imx_thermal.c new file +4
-0+* should provide plenty of time. Sleeping longer than 1ms is ok so no +* need for usleep_range. +*/ +msleep (1 );I know 1ms is usually too short time slice when talking about thermal monitoring on nowadays processors even, but 17us to 1ms sounds like an overkill.. Sure you are not hiding other issues?
At least none I'm ware of. I will update the comment and simply use usleep_range in the next post to prove that I'm not hiding other issues :)
Quoted 85 lines
Quoted 85 lines
drivers/thermal/imx_thermal.c new file +85
-0+ +regmap_read (map,TEMPSENSE0 , &val); +regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_MEASURE_TEMP ); +regmap_write (map,TEMPSENSE0 +REG_SET ,TEMPSENSE0_POWER_DOWN ); + +if ((val &TEMPSENSE0_FINISHED ) ==0 ) { +dev_dbg (&tz->device,"temp measurement never finished\n" ); +return -EAGAIN ; + } + + n_meas = (val &TEMPSENSE0_TEMP_CNT_MASK ) >>TEMPSENSE0_TEMP_CNT_SHIFT ; + +/* See imx_get_sensor_data() for forumla derivation */ + *temp = data->c2 + data->c1 * n_meas; + +if (*temp != last_temp) { +dev_dbg (&tz->device,"millicelsius: %ld\n" , *temp); + last_temp = *temp; + } + +return 0 ; +} + +static int imx_get_mode (struct thermal_zone_device *tz, +enum thermal_device_mode *mode) +{ +struct imx_thermal_data *data = tz->devdata; + + *mode = data->mode; + +return 0 ; +} + +static int imx_set_mode (struct thermal_zone_device *tz, +enum thermal_device_mode mode) +{ +struct imx_thermal_data *data = tz->devdata; + +if (mode ==THERMAL_DEVICE_ENABLED ) { + tz->polling_delay =IMX_POLLING_DELAY ; + tz->passive_delay =IMX_PASSIVE_DELAY ; + }else { + tz->polling_delay =0 ; + tz->passive_delay =0 ; + } + + data->mode = mode; +thermal_zone_device_update (tz); + +return 0 ; +} + +static int imx_get_trip_type (struct thermal_zone_device *tz,int trip, +enum thermal_trip_type *type) +{ + *type = (trip ==IMX_TRIP_PASSIVE ) ?THERMAL_TRIP_PASSIVE : +THERMAL_TRIP_CRITICAL ; +return 0 ; +} + +static int imx_get_crit_temp (struct thermal_zone_device *tz, +unsigned long *temp) +{ + *temp =IMX_TEMP_CRITICAL ; +return 0 ; +} + +static int imx_get_trip_temp (struct thermal_zone_device *tz,int trip, +unsigned long *temp) +{ + *temp = (trip ==IMX_TRIP_PASSIVE ) ?IMX_TEMP_PASSIVE : +IMX_TEMP_CRITICAL ; +return 0 ; +} + +static int imx_bind (struct thermal_zone_device *tz, +struct thermal_cooling_device *cdev) +{ +int ret; + + ret =thermal_zone_bind_cooling_device (tz,IMX_TRIP_PASSIVE , cdev, +THERMAL_NO_LIMIT , +THERMAL_NO_LIMIT ); +if (ret) { + dev_err(&tz->device,
Quoted 8 lines
drivers/thermal/imx_thermal.c line 190
Quoted 6 lines
drivers/thermal/imx_thermal.c new file +6
-0+"binding zone %s with cdev %s failed:%d\n" , + tz->type, cdev->type, ret); +return ret; + } + +return 0 ;nit: I think it is just safe to do:
Yes, it is. But I personally would like to separate error return from successful one, because I feel it's more readable.
Quoted 106 lines
drivers/thermal/imx_thermal.c new file +9
-0+ ret =thermal_zone_bind_cooling_device (tz,IMX_TRIP_PASSIVE , cdev, +THERMAL_NO_LIMIT , +THERMAL_NO_LIMIT ); +if (ret) +dev_err (&tz->device, +"binding zone %s with cdev %s failed:%d\n" , + tz->type, cdev->type, ret); + +return ret;Quoted 7 lines
drivers/thermal/imx_thermal.c new file +6
-0+} + +static int imx_unbind (struct thermal_zone_device *tz, +struct thermal_cooling_device *cdev) +{ +int ret;drivers/thermal/imx_thermal.c line 206
Quoted 10 lines
drivers/thermal/imx_thermal.c new file +10
-0+ + ret =thermal_zone_unbind_cooling_device (tz,IMX_TRIP_PASSIVE , cdev); +if (ret) { +dev_err (&tz->device, +"unbinding zone %s with cdev %s failed:%d\n" , + tz->type, cdev->type, ret); +return ret; + } + +return 0 ;ditto.
Quoted 77 lines
drivers/thermal/imx_thermal.c new file +77
-0+} + +static const struct thermal_zone_device_ops imx_tz_ops = { + .bind = imx_bind, + .unbind = imx_unbind, + .get_temp = imx_get_temp, + .get_mode = imx_get_mode, + .set_mode = imx_set_mode, + .get_trip_type = imx_get_trip_type, + .get_trip_temp = imx_get_trip_temp, + .get_crit_temp = imx_get_crit_temp, +}; + +static int imx_get_sensor_data (struct platform_device *pdev) +{ +struct imx_thermal_data *data =platform_get_drvdata (pdev); +struct regmap *map; +int t1, t2, n1, n2; +int ret; +u32 val; + + map =syscon_regmap_lookup_by_phandle (pdev->dev.of_node, +"fsl,tempmon-data" ); +if (IS_ERR (map)) { + ret =PTR_ERR (map); +dev_err (&pdev->dev,"failed to get sensor regmap: %d\n" , ret); +return ret; + } + + ret =regmap_read (map,OCOTP_ANA1 , &val); +if (ret) { +dev_err (&pdev->dev,"failed to read sensor data: %d\n" , ret); +return ret; + } + +if (val ==0 || val == ~0 ) { +dev_err (&pdev->dev,"invalid sensor calibration data\n" ); +return -EINVAL ; + } + +/* +* Sensor data layout: +* [31:20] - sensor value @ 25C +* [19:8] - sensor value of hot +* [7:0] - hot temperature value +*/ + n1 = val >>20 ; + n2 = (val &0xfff00 ) >>8 ; + t2 = val &0xff ; + t1 =25 ;/* t1 always 25C */ + +/* +* Derived from linear interpolation, +* Tmeas = T2 + (Nmeas - N2) * (T1 - T2) / (N1 - N2) +* We want to reduce this down to the minimum computation necessary +* for each temperature read. Also, we want Tmeas in millicelsius +* and we don't want to lose precision from integer division. So... +* milli_Tmeas = 1000 * T2 + 1000 * (Nmeas - N2) * (T1 - T2) / (N1 - N2) +* Let constant c1 = 1000 * (T1 - T2) / (N1 - N2) +* milli_Tmeas = (1000 * T2) + c1 * (Nmeas - N2) +* milli_Tmeas = (1000 * T2) + (c1 * Nmeas) - (c1 * N2) +* Let constant c2 = (1000 * T2) - (c1 * N2) +* milli_Tmeas = c2 + (c1 * Nmeas) +*/ + data->c1 =1000 * (t1 - t2) / (n1 - n2); + data->c2 =1000 * t2 - data->c1 * n2; + +return 0 ; +} + +static int imx_thermal_probe (struct platform_device *pdev) +{ +struct imx_thermal_data *data; +struct cpumask clip_cpus; +struct regmap *map; +int ret; +
Quoted 9 lines
drivers/thermal/imx_thermal.c line 288
Quoted 5 lines
drivers/thermal/imx_thermal.c new file +5
-0+ data =devm_kzalloc (&pdev->dev,sizeof (*data),GFP_KERNEL ); +if (!data) +return -ENOMEM ; + + map =syscon_regmap_lookup_by_phandle (pdev->dev.of_node,"fsl,tempmon" );Is this MMIO? Sure you want to do regmapped access to something so simple as io mem r/w?
Yes, MMIO. But it's embedded in some kind of system controller block, call anatop on imx6q, which contains miscellaneous registers for various function blocks. We do not want to have various device drivers iomap the same space multiple times. syscon API which uses regmap was created exactly for such cases, so we use it here.
Quoted 69 lines
Quoted 69 lines
drivers/thermal/imx_thermal.c new file +69
-0+if (IS_ERR (map)) { + ret =PTR_ERR (map); +dev_err (&pdev->dev,"failed to get tempmon regmap: %d\n" , ret); +return ret; + } + data->tempmon = map; + +platform_set_drvdata (pdev, data); + + ret =imx_get_sensor_data (pdev); +if (ret) { +dev_err (&pdev->dev,"failed to get sensor data\n" ); +return ret; + } + +/* Make sure sensor is in known good state for measurements */ +regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_POWER_DOWN ); +regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_MEASURE_TEMP ); +regmap_write (map,TEMPSENSE1 +REG_CLR ,TEMPSENSE1_MEASURE_FREQ ); +regmap_write (map,MISC0 +REG_SET ,MISC0_REFTOP_SELBIASOFF ); +regmap_write (map,TEMPSENSE0 +REG_SET ,TEMPSENSE0_POWER_DOWN ); + +cpumask_set_cpu (0 , &clip_cpus); + data->cdev =cpufreq_cooling_register (&clip_cpus); +if (IS_ERR (data->cdev)) { + ret =PTR_ERR (data->cdev); +dev_err (&pdev->dev, +"failed to register cpufreq cooling device: %d\n" , ret); +return ret; + } + + data->tz =thermal_zone_device_register ("imx_thermal_zone" , +IMX_TRIP_NUM ,0 , data, + &imx_tz_ops,NULL , +IMX_PASSIVE_DELAY , +IMX_POLLING_DELAY ); +if (IS_ERR (data->tz)) { + ret =PTR_ERR (data->tz); +dev_err (&pdev->dev, +"failed to register thermal zone device %d\n" , ret); +cpufreq_cooling_unregister (data->cdev); +return ret; + } + + data->mode =THERMAL_DEVICE_ENABLED ; + +return 0 ; +} + +static int imx_thermal_remove (struct platform_device *pdev) +{ +struct imx_thermal_data *data =platform_get_drvdata (pdev); + +thermal_zone_device_unregister (data->tz); +cpufreq_cooling_unregister (data->cdev); + +return 0 ; +} + +#ifdef CONFIG_PM_SLEEP +static int imx_thermal_suspend (struct device *dev) +{ +struct imx_thermal_data *data =dev_get_drvdata (dev); +struct regmap *map = data->tempmon; +u32 val; + +regmap_read (map,TEMPSENSE0 , &val); + +/* Was a measurement taking place? If not, nothing to do. */
Quoted 8 lines
drivers/thermal/imx_thermal.c line 362
Quoted 5 lines
drivers/thermal/imx_thermal.c new file +5
-0+if (val &TEMPSENSE0_POWER_DOWN ) +return 0 ; + +regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_MEASURE_TEMP ); +regmap_write (map,TEMPSENSE0 +REG_SET ,TEMPSENSE0_POWER_DOWN );hmmm.. Whould this mess with the running meas? Any luck to wait it to finish then suspend?
Yeah, sounds like a good idea. Will simply give it a wait.
Quoted 60 lines
Quoted 16 lines
drivers/thermal/imx_thermal.c new file +15
-0+ + data->meas_suspended = true; + +return 0 ; +} + +static int imx_thermal_resume (struct device *dev) +{ +struct imx_thermal_data *data =dev_get_drvdata (dev); +struct regmap *map = data->tempmon; + +/* +* If a measurement was taking place while suspend, re-take the +* measurement. +*/ drivers/thermal/imx_thermal.c line 387
Quoted 10 lines
drivers/thermal/imx_thermal.c new file +10
-0+if (data->meas_suspended) { +regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_POWER_DOWN ); +regmap_write (map,TEMPSENSE0 +REG_SET ,TEMPSENSE0_MEASURE_TEMP ); +/* +* According to the temp sensor designers, it may require +* up to ~17us to complete a measurement. But this timing +* isn't checked on every part nor is it specified in the +* datasheet, so delay 50us for timing margin. +*/ +udelay (50 );hmm.. here you need 50us only but on regular readings 1ms?
Quoted 31 lines
drivers/thermal/imx_thermal.c new file +31
-0+ data->meas_suspended = false; + } + +return 0 ; +} +#endif + +static SIMPLE_DEV_PM_OPS(imx_thermal_pm_ops, + imx_thermal_suspend, imx_thermal_resume); + +static const struct of_device_id of_imx_thermal_match[] = { + { .compatible ="fsl,imx6q-tempmon" , }, + {/* end */ } +}; + +static struct platform_driver imx_thermal = { + .driver = { + .name ="imx_thermal" , + .owner =THIS_MODULE , + .pm = &imx_thermal_pm_ops, + .of_match_table = of_imx_thermal_match, + }, + .probe = imx_thermal_probe, + .remove = imx_thermal_remove, +}; + +static int __init imx_thermal_init (void ) +{ +return platform_driver_register (&imx_thermal); +} +late_initcall (imx_thermal_init);
Quoted 8 lines
drivers/thermal/imx_thermal.c line 424
Quoted 6 lines
drivers/thermal/imx_thermal.c new file +6
-0+ +static void __exit imx_thermal_exit(void) +{ +platform_driver_unregister (&imx_thermal); +} +module_exit (imx_thermal_exit);How about using
module_platform_driver()macro?
I thought that the thermal driver has to be probed before cpufreq driver. I just give it a trial run and found that imx_thermal_probe() can return success even when cpufreq driver is not probed yet. So making imx_thermal_init a late_initcall seems unnecessary. Therefore, yes, we can use module_platform_driver() here.
Shawn
PATCH v2 1/1 thermal: add imx thermal driver support
Quoted 38 lines · Shawn Guo
Thanks for the review effort, Eduardo.
Quoted 25 lines · Eduardo Valentin
Quoted 24 lines · Shawn Guo
This is based on the initial imx thermal work done by
Rob Lee <[email protected]> (Not sure if the email address is still valid). Since he is no longer interested in the work and I have rewritten a significant amount of the code, I just took the authorship over from him.It adds the imx thermal support using Temperature Monitor (TEMPMON) block found on some Freescale i.MX SoCs. The driver uses syscon regmap interface to access TEMPMON control registers and calibration data, and supports cpufreq as the cooling device.
Signed-off-by Shawn Guo <[email protected]>---
Changes since v1:
* Change trip type to passive..../devicetree/bindings/thermal/imx-thermal.txt | 14 + drivers/thermal/Kconfig | 8 + drivers/thermal/Makefile | 1 + drivers/thermal/imx_thermal.c | 429 ++++++++++++++++++++ 4 files changed, 452 insertions(+) create mode 100644 Documentation/devicetree/bindings/thermal/imx-thermal.txt create mode 100644 drivers/thermal/imx_thermal.cQuoted 10 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt line 1
Quoted 7 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt new file +1
-0@@ -0,0 +1,14 @@ +* Temperature Monitor (TEMPMON) on Freescale i.MX SoCsAny hw documentation reference?
OK. Tks.
Some ppl like to put hw doc reference on your driver documentation. But for me is your call. I believe it is good to ask the kernel doc guys too.
Quoted 8 lines
Quoted 8 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt line 6
Quoted 5 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt new file +5
-0+ +Required properties: +- compatible : "fsl,imx6q-thermal" +- fsl,tempmon : phandle pointer to TEMPMON control registers +- fsl,tempmon-data : phandle pointer to TEMPMON calibration datawhat exactly is calibration data?
Quoted 6 lines
The temperature calibration values are fused individually for each part in the product testing process. It consists of a pair of hardware monitor values at 25 C and a high temperature which is specified by calibration data itself, which will be used by software to translate hardware counter values to Celsius. The detailed description can be found in hardware reference manual.
That was what I expected. But it is not that clear what you meant by "thermal cal data" your DT documenation. Maybe you need to describe how to enter a fsl,tempmon-data.
Quoted 12 lines
Quoted 12 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt line 14
Quoted 9 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt new file +8
-0+ +Example: + +tempmon { + compatible = "fsl,imx6q-tempmon"; + fsl,tempmon = <&anatop>; + fsl,tempmon-data = <&ocotp>; +};Why do you need specific DT bindings for things like registers?
Quoted 5 lines
Because there is no dedicated IO space for the block. Instead, the registers spread in system controllers which contain miscellaneous control and data registers. So we chose to access the registers via syscon regmap interface, and we need phandle pointer to the correct syscon device node.
I believe it is same as OMAPs SCM. What I did was simply to ioremap the registers belonging to the temp sensor. At least they are not shared with any other ip behind the SCM.
Quoted 20 lines
Quoted 9 lines
Quoted 9 lines
drivers/thermal/Kconfig +0
-0@@ -91,6 +91,14 @@ config THERMAL_EMULATION because userland can easily disable the thermal policy by simply flooding this sysfs node with low temperature values.Quoted 11 lines
drivers/thermal/Kconfig config THERMAL_EMULATION · line 101
Quoted 8 lines
drivers/thermal/Kconfig +8
-0+config IMX_THERMAL + tristate "Temperature sensor driver for Freescale i.MX SoCs" + depends on CPU_THERMAL + depends on MFD_SYSCON + depends on OF + help + Support for Temperature Monitor (TEMPMON) found on Freescale i.MX SoCs. +I guess this help generates a warning (too short) by checkpatch.pl.
Quoted 3 lines
Yes. But from what I've seen, most maintainers are not so concerned by this particular checkpatch warning. If you are really concerned by it, I can surely manage to write up something longer.
Not a strong opinion. Just want to keep things clean. Might not be a big effort to give it a better help though.
Quoted 28 lines
Quoted 16 lines
Quoted 16 lines
drivers/thermal/Kconfig +0
-0config SPEAR_THERMAL bool "SPEAr thermal sensor driver" depends on PLAT_SPEARdrivers/thermal/Makefile +1
-0@@ -21,6 +21,7 @@ obj-$(CONFIG_EXYNOS_THERMAL) += exynos_thermal.o obj-$(CONFIG_DOVE_THERMAL) += dove_thermal.o obj-$(CONFIG_DB8500_THERMAL) += db8500_thermal.o obj-$(CONFIG_ARMADA_THERMAL) += armada_thermal.o +obj-$(CONFIG_IMX_THERMAL) += imx_thermal.o obj-$(CONFIG_DB8500_CPUFREQ_COOLING) += db8500_cpufreq_cooling.o obj-$(CONFIG_INTEL_POWERCLAMP) += intel_powerclamp.oQuoted 12 lines
drivers/thermal/imx_thermal.c line 3
Quoted 9 lines
drivers/thermal/imx_thermal.c new file +3
-0@@ -0,0 +1,429 @@ +/* +* Copyright 2013 Freescale Semiconductor, Inc. +* No contact entry?
Quoted 4 lines
I'm not a big fan of doing that, because we will need to patch the contact from time to time to keep it update to date. I've run into the situation a lot where the contract left in the file became invalid and git log helps give the recent/updated contact.
Well, you can see this from other side. If you keep a contact entry, we will be force to updated it when it gets invalid (git will start to throw it when ppl ask for maintainers and it will return).
But it is really your call.
Quoted 67 lines
Quoted 58 lines
Quoted 58 lines
drivers/thermal/imx_thermal.c new file +57
-0+* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +* +*/ + +#include <linux/cpu_cooling.h> +#include <linux/cpufreq.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/slab.h> +#include <linux/thermal.h> +#include <linux/types.h> + +#define REG_SET 0x4 +#define REG_CLR 0x8 +#define REG_TOG 0xc + +#define MISC0 0x0150 +#define MISC0_REFTOP_SELBIASOFF (1 << 3) + +#define TEMPSENSE0 0x0180 +#define TEMPSENSE0_TEMP_CNT_SHIFT 8 +#define TEMPSENSE0_TEMP_CNT_MASK (0xfff << TEMPSENSE0_TEMP_CNT_SHIFT) +#define TEMPSENSE0_FINISHED (1 << 2) +#define TEMPSENSE0_MEASURE_TEMP (1 << 1) +#define TEMPSENSE0_POWER_DOWN (1 << 0) + +#define TEMPSENSE1 0x0190 +#define TEMPSENSE1_MEASURE_FREQ 0xffff + +#define OCOTP_ANA1 0x04e0 + +/* The driver supports 1 passive trip point and 1 critical trip point */ +enum imx_thermal_trip { +IMX_TRIP_PASSIVE , +IMX_TRIP_CRITICAL , +IMX_TRIP_NUM , +}; + +/* +* It defines the temperature in millicelsius for passive trip point +* that will trigger cooling action when crossed. +*/ +#define IMX_TEMP_PASSIVE 85000 + +/* +* The maximum die temperature on imx parts is 105C, let's give some cushion +* for noise and possible temperature rise between measurements. Quoted 9 lines
drivers/thermal/imx_thermal.c line 65
Quoted 5 lines
drivers/thermal/imx_thermal.c new file +5
-0+*/ +#define IMX_TEMP_CRITICAL 100000 + +#define IMX_POLLING_DELAY 5000/* millisecond */ +#define IMX_PASSIVE_DELAY 3000What is your maximum delta between CPU frequencies? Sure this polling intervals can hold your maximum power burst?
Quoted 4 lines
I have to admit I'm not sure how to determine the optimal intervals here. I initially have both intervals be 1000 ms, but I'm not sure if it's necessary. I need some help on that. The frequency table on the target consists of 396, 792, 996 and 1200 in MHz.
Here you can do simple calculation on your maximum delta Celsius over delta time. Then check it against your maximum delta power (power at 1.2GHz - power at 396MHz) with respect to your power vs. temp. curve. That will give you a rough expectation on what you need to be sampling. But you probably need help from some guy doing thermal modeling and simulation on your chip.
Quoted 27 lines
Quoted 18 lines
Quoted 18 lines
drivers/thermal/imx_thermal.c new file +17
-0+ +struct imx_thermal_data { +struct thermal_zone_device *tz; +struct thermal_cooling_device *cdev; +enum thermal_device_mode mode; +struct regmap *tempmon; +bool meas_suspended; +int c1, c2;/* See forumla in imx_get_sensor_data() */ +}; + +static int imx_get_temp (struct thermal_zone_device *tz,unsigned long *temp) +{ +struct imx_thermal_data *data = tz->devdata; +struct regmap *map = data->tempmon; +static unsigned long last_temp; +unsigned int n_meas; +u32 val;Quoted 9 lines
drivers/thermal/imx_thermal.c line 88
Quoted 6 lines
drivers/thermal/imx_thermal.c new file +6
-0+ +/* +* Every time we measure the temperature, we will power on the +* temperature sensor, enable measurements, take a reading, +* disable measurements, power off the temperature sensor. +*/ Are these sensors used on any hw failsafe mechanism?
Quoted 1 line
No, I do not think so.
OK. Then safe to turn it off right? Another good reason to have a fine tuned sampling rate.
Quoted 17 lines
Quoted 17 lines
drivers/thermal/imx_thermal.c line 99
Quoted 12 lines
drivers/thermal/imx_thermal.c new file +11
-0+regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_POWER_DOWN ); +regmap_write (map,TEMPSENSE0 +REG_SET ,TEMPSENSE0_MEASURE_TEMP ); + +/* +* According to the temp sensor designers, it may require up to ~17us +* to complete a measurement. But this timing isn't checked on every +* part nor is it specified in the datasheet, so sleeping at least 1ms +* should provide plenty of time. Sleeping longer than 1ms is ok so no +* need for usleep_range. +*/ +msleep (1 );I know 1ms is usually too short time slice when talking about thermal monitoring on nowadays processors even, but 17us to 1ms sounds like an overkill.. Sure you are not hiding other issues?
Quoted 3 lines
At least none I'm ware of. I will update the comment and simply use
usleep_rangein the next post to prove that I'm not hiding other issues :)
OK. It just looks strange that here you need 1ms, but after suspend you can afford waiting only 50us. Anything special about the suspend path?
Quoted 95 lines
Quoted 86 lines
Quoted 86 lines
drivers/thermal/imx_thermal.c new file +85
-0+ +regmap_read (map,TEMPSENSE0 , &val); +regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_MEASURE_TEMP ); +regmap_write (map,TEMPSENSE0 +REG_SET ,TEMPSENSE0_POWER_DOWN ); + +if ((val &TEMPSENSE0_FINISHED ) ==0 ) { +dev_dbg (&tz->device,"temp measurement never finished\n" ); +return -EAGAIN ; + } + + n_meas = (val &TEMPSENSE0_TEMP_CNT_MASK ) >>TEMPSENSE0_TEMP_CNT_SHIFT ; + +/* See imx_get_sensor_data() for forumla derivation */ + *temp = data->c2 + data->c1 * n_meas; + +if (*temp != last_temp) { +dev_dbg (&tz->device,"millicelsius: %ld\n" , *temp); + last_temp = *temp; + } + +return 0 ; +} + +static int imx_get_mode (struct thermal_zone_device *tz, +enum thermal_device_mode *mode) +{ +struct imx_thermal_data *data = tz->devdata; + + *mode = data->mode; + +return 0 ; +} + +static int imx_set_mode (struct thermal_zone_device *tz, +enum thermal_device_mode mode) +{ +struct imx_thermal_data *data = tz->devdata; + +if (mode ==THERMAL_DEVICE_ENABLED ) { + tz->polling_delay =IMX_POLLING_DELAY ; + tz->passive_delay =IMX_PASSIVE_DELAY ; + }else { + tz->polling_delay =0 ; + tz->passive_delay =0 ; + } + + data->mode = mode; +thermal_zone_device_update (tz); + +return 0 ; +} + +static int imx_get_trip_type (struct thermal_zone_device *tz,int trip, +enum thermal_trip_type *type) +{ + *type = (trip ==IMX_TRIP_PASSIVE ) ?THERMAL_TRIP_PASSIVE : +THERMAL_TRIP_CRITICAL ; +return 0 ; +} + +static int imx_get_crit_temp (struct thermal_zone_device *tz, +unsigned long *temp) +{ + *temp =IMX_TEMP_CRITICAL ; +return 0 ; +} + +static int imx_get_trip_temp (struct thermal_zone_device *tz,int trip, +unsigned long *temp) +{ + *temp = (trip ==IMX_TRIP_PASSIVE ) ?IMX_TEMP_PASSIVE : +IMX_TEMP_CRITICAL ; +return 0 ; +} + +static int imx_bind (struct thermal_zone_device *tz, +struct thermal_cooling_device *cdev) +{ +int ret; + + ret =thermal_zone_bind_cooling_device (tz,IMX_TRIP_PASSIVE , cdev, +THERMAL_NO_LIMIT , +THERMAL_NO_LIMIT ); +if (ret) { + dev_err(&tz->device,Quoted 8 lines
drivers/thermal/imx_thermal.c line 190
Quoted 6 lines
drivers/thermal/imx_thermal.c new file +6
-0+"binding zone %s with cdev %s failed:%d\n" , + tz->type, cdev->type, ret); +return ret; + } + +return 0 ;nit: I think it is just safe to do:
Quoted 2 lines
Yes, it is. But I personally would like to separate error return from successful one, because I feel it's more readable.
This is indeed a matter of taste.
Quoted 116 lines
Quoted 107 lines
drivers/thermal/imx_thermal.c new file +9
-0+ ret =thermal_zone_bind_cooling_device (tz,IMX_TRIP_PASSIVE , cdev, +THERMAL_NO_LIMIT , +THERMAL_NO_LIMIT ); +if (ret) +dev_err (&tz->device, +"binding zone %s with cdev %s failed:%d\n" , + tz->type, cdev->type, ret); + +return ret;Quoted 7 lines
drivers/thermal/imx_thermal.c new file +6
-0+} + +static int imx_unbind (struct thermal_zone_device *tz, +struct thermal_cooling_device *cdev) +{ +int ret;drivers/thermal/imx_thermal.c line 206
Quoted 10 lines
drivers/thermal/imx_thermal.c new file +10
-0+ + ret =thermal_zone_unbind_cooling_device (tz,IMX_TRIP_PASSIVE , cdev); +if (ret) { +dev_err (&tz->device, +"unbinding zone %s with cdev %s failed:%d\n" , + tz->type, cdev->type, ret); +return ret; + } + +return 0 ;ditto.
Quoted 77 lines
drivers/thermal/imx_thermal.c new file +77
-0+} + +static const struct thermal_zone_device_ops imx_tz_ops = { + .bind = imx_bind, + .unbind = imx_unbind, + .get_temp = imx_get_temp, + .get_mode = imx_get_mode, + .set_mode = imx_set_mode, + .get_trip_type = imx_get_trip_type, + .get_trip_temp = imx_get_trip_temp, + .get_crit_temp = imx_get_crit_temp, +}; + +static int imx_get_sensor_data (struct platform_device *pdev) +{ +struct imx_thermal_data *data =platform_get_drvdata (pdev); +struct regmap *map; +int t1, t2, n1, n2; +int ret; +u32 val; + + map =syscon_regmap_lookup_by_phandle (pdev->dev.of_node, +"fsl,tempmon-data" ); +if (IS_ERR (map)) { + ret =PTR_ERR (map); +dev_err (&pdev->dev,"failed to get sensor regmap: %d\n" , ret); +return ret; + } + + ret =regmap_read (map,OCOTP_ANA1 , &val); +if (ret) { +dev_err (&pdev->dev,"failed to read sensor data: %d\n" , ret); +return ret; + } + +if (val ==0 || val == ~0 ) { +dev_err (&pdev->dev,"invalid sensor calibration data\n" ); +return -EINVAL ; + } + +/* +* Sensor data layout: +* [31:20] - sensor value @ 25C +* [19:8] - sensor value of hot +* [7:0] - hot temperature value +*/ + n1 = val >>20 ; + n2 = (val &0xfff00 ) >>8 ; + t2 = val &0xff ; + t1 =25 ;/* t1 always 25C */ + +/* +* Derived from linear interpolation, +* Tmeas = T2 + (Nmeas - N2) * (T1 - T2) / (N1 - N2) +* We want to reduce this down to the minimum computation necessary +* for each temperature read. Also, we want Tmeas in millicelsius +* and we don't want to lose precision from integer division. So... +* milli_Tmeas = 1000 * T2 + 1000 * (Nmeas - N2) * (T1 - T2) / (N1 - N2) +* Let constant c1 = 1000 * (T1 - T2) / (N1 - N2) +* milli_Tmeas = (1000 * T2) + c1 * (Nmeas - N2) +* milli_Tmeas = (1000 * T2) + (c1 * Nmeas) - (c1 * N2) +* Let constant c2 = (1000 * T2) - (c1 * N2) +* milli_Tmeas = c2 + (c1 * Nmeas) +*/ + data->c1 =1000 * (t1 - t2) / (n1 - n2); + data->c2 =1000 * t2 - data->c1 * n2; + +return 0 ; +} + +static int imx_thermal_probe (struct platform_device *pdev) +{ +struct imx_thermal_data *data; +struct cpumask clip_cpus; +struct regmap *map; +int ret; +Quoted 9 lines
drivers/thermal/imx_thermal.c line 288
Quoted 5 lines
drivers/thermal/imx_thermal.c new file +5
-0+ data =devm_kzalloc (&pdev->dev,sizeof (*data),GFP_KERNEL ); +if (!data) +return -ENOMEM ; + + map =syscon_regmap_lookup_by_phandle (pdev->dev.of_node,"fsl,tempmon" );Is this MMIO? Sure you want to do regmapped access to something so simple as io mem r/w?
Quoted 5 lines
Yes, MMIO. But it's embedded in some kind of system controller block, call anatop on imx6q, which contains miscellaneous registers for various function blocks. We do not want to have various device drivers iomap the same space multiple times. syscon API which uses regmap was created exactly for such cases, so we use it here.
Then again, I was more on the overhead imposed by regmap. For buses like i2c I understand its usage, but MMIO starts to be a stretch.
Quoted 79 lines
Quoted 70 lines
Quoted 70 lines
drivers/thermal/imx_thermal.c new file +69
-0+if (IS_ERR (map)) { + ret =PTR_ERR (map); +dev_err (&pdev->dev,"failed to get tempmon regmap: %d\n" , ret); +return ret; + } + data->tempmon = map; + +platform_set_drvdata (pdev, data); + + ret =imx_get_sensor_data (pdev); +if (ret) { +dev_err (&pdev->dev,"failed to get sensor data\n" ); +return ret; + } + +/* Make sure sensor is in known good state for measurements */ +regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_POWER_DOWN ); +regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_MEASURE_TEMP ); +regmap_write (map,TEMPSENSE1 +REG_CLR ,TEMPSENSE1_MEASURE_FREQ ); +regmap_write (map,MISC0 +REG_SET ,MISC0_REFTOP_SELBIASOFF ); +regmap_write (map,TEMPSENSE0 +REG_SET ,TEMPSENSE0_POWER_DOWN ); + +cpumask_set_cpu (0 , &clip_cpus); + data->cdev =cpufreq_cooling_register (&clip_cpus); +if (IS_ERR (data->cdev)) { + ret =PTR_ERR (data->cdev); +dev_err (&pdev->dev, +"failed to register cpufreq cooling device: %d\n" , ret); +return ret; + } + + data->tz =thermal_zone_device_register ("imx_thermal_zone" , +IMX_TRIP_NUM ,0 , data, + &imx_tz_ops,NULL , +IMX_PASSIVE_DELAY , +IMX_POLLING_DELAY ); +if (IS_ERR (data->tz)) { + ret =PTR_ERR (data->tz); +dev_err (&pdev->dev, +"failed to register thermal zone device %d\n" , ret); +cpufreq_cooling_unregister (data->cdev); +return ret; + } + + data->mode =THERMAL_DEVICE_ENABLED ; + +return 0 ; +} + +static int imx_thermal_remove (struct platform_device *pdev) +{ +struct imx_thermal_data *data =platform_get_drvdata (pdev); + +thermal_zone_device_unregister (data->tz); +cpufreq_cooling_unregister (data->cdev); + +return 0 ; +} + +#ifdef CONFIG_PM_SLEEP +static int imx_thermal_suspend (struct device *dev) +{ +struct imx_thermal_data *data =dev_get_drvdata (dev); +struct regmap *map = data->tempmon; +u32 val; + +regmap_read (map,TEMPSENSE0 , &val); + +/* Was a measurement taking place? If not, nothing to do. */ Quoted 8 lines
drivers/thermal/imx_thermal.c line 362
Quoted 5 lines
drivers/thermal/imx_thermal.c new file +5
-0+if (val &TEMPSENSE0_POWER_DOWN ) +return 0 ; + +regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_MEASURE_TEMP ); +regmap_write (map,TEMPSENSE0 +REG_SET ,TEMPSENSE0_POWER_DOWN );hmmm.. Whould this mess with the running meas? Any luck to wait it to finish then suspend?
Quoted 1 line
Yeah, sounds like a good idea. Will simply give it a wait.
OK.
Quoted 70 lines
Quoted 61 lines
Quoted 17 lines
drivers/thermal/imx_thermal.c new file +15
-0+ + data->meas_suspended = true; + +return 0 ; +} + +static int imx_thermal_resume (struct device *dev) +{ +struct imx_thermal_data *data =dev_get_drvdata (dev); +struct regmap *map = data->tempmon; + +/* +* If a measurement was taking place while suspend, re-take the +* measurement. +*/ drivers/thermal/imx_thermal.c line 387
Quoted 10 lines
drivers/thermal/imx_thermal.c new file +10
-0+if (data->meas_suspended) { +regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_POWER_DOWN ); +regmap_write (map,TEMPSENSE0 +REG_SET ,TEMPSENSE0_MEASURE_TEMP ); +/* +* According to the temp sensor designers, it may require +* up to ~17us to complete a measurement. But this timing +* isn't checked on every part nor is it specified in the +* datasheet, so delay 50us for timing margin. +*/ +udelay (50 );hmm.. here you need 50us only but on regular readings 1ms?
Quoted 31 lines
drivers/thermal/imx_thermal.c new file +31
-0+ data->meas_suspended = false; + } + +return 0 ; +} +#endif + +static SIMPLE_DEV_PM_OPS(imx_thermal_pm_ops, + imx_thermal_suspend, imx_thermal_resume); + +static const struct of_device_id of_imx_thermal_match[] = { + { .compatible ="fsl,imx6q-tempmon" , }, + {/* end */ } +}; + +static struct platform_driver imx_thermal = { + .driver = { + .name ="imx_thermal" , + .owner =THIS_MODULE , + .pm = &imx_thermal_pm_ops, + .of_match_table = of_imx_thermal_match, + }, + .probe = imx_thermal_probe, + .remove = imx_thermal_remove, +}; + +static int __init imx_thermal_init (void ) +{ +return platform_driver_register (&imx_thermal); +} +late_initcall (imx_thermal_init);Quoted 8 lines
drivers/thermal/imx_thermal.c line 424
Quoted 6 lines
drivers/thermal/imx_thermal.c new file +6
-0+ +static void __exit imx_thermal_exit(void) +{ +platform_driver_unregister (&imx_thermal); +} +module_exit (imx_thermal_exit);How about using
module_platform_driver()macro?
Quoted 5 lines
I thought that the thermal driver has to be probed before cpufreq driver. I just give it a trial run and found that
imx_thermal_probe()can return success even when cpufreq driver is not probed yet. So makingimx_thermal_initalate_initcallseems unnecessary. Therefore, yes, we can usemodule_platform_driver()here.
OK. Looks like to be a general issue. I faced the same with cpufreq vs thermal. Initially I also thought of doing late_init, but it does not really solve the problem. I solved it by returning -EDEFER_PROBE till a proper cpufreq driver pops up. Which actually solved the issue.
We might want to make this solution go into cpufreq_cooling device instead, and drivers have to propagate the error code at their probe.
I will cook somehting for this so that drivers are cleaner.
Quoted 5 lines
Shawn
Signature · 11 lines
You have got to be excited about what you are doing. (L. Lamport) Eduardo Valentin -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 295 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130619/a69bba00/attachment-0001.sig>
PATCH v2 1/1 thermal: add imx thermal driver support
Quoted 5 lines · Eduardo Valentin
Quoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt line 1
Quoted 2 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt new file +1
-0@@ -0,0 +1,14 @@ +* Temperature Monitor (TEMPMON) on Freescale i.MX SoCsAny hw documentation reference?
Quoted 8 lines
OK. Tks.
Some ppl like to put hw doc reference on your driver documentation. But for me is your call. I believe it is good to ask the kernel doc guys too.
I'm not a fan of doing that, because I can not tell that the link will be still valid there in 5 years or so.
Quoted 15 lines
Quoted 8 lines
Quoted 8 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt line 6
Quoted 5 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt new file +5
-0+ +Required properties: +- compatible : "fsl,imx6q-thermal" +- fsl,tempmon : phandle pointer to TEMPMON control registers +- fsl,tempmon-data : phandle pointer to TEMPMON calibration datawhat exactly is calibration data?
Quoted 6 lines
The temperature calibration values are fused individually for each part in the product testing process. It consists of a pair of hardware monitor values at 25 C and a high temperature which is specified by calibration data itself, which will be used by software to translate hardware counter values to Celsius. The detailed description can be found in hardware reference manual.
Quoted 4 lines
That was what I expected. But it is not that clear what you meant by "thermal cal data" your DT documenation. Maybe you need to describe how to enter a fsl,tempmon-data.
Ok, I will try to come up with something better.
<snip>
Quoted 11 lines
Quoted 10 lines
Quoted 6 lines
drivers/thermal/imx_thermal.c line 65
Quoted 2 lines
drivers/thermal/imx_thermal.c new file +2
-0+#define IMX_POLLING_DELAY 5000/* millisecond */ +#define IMX_PASSIVE_DELAY 3000What is your maximum delta between CPU frequencies? Sure this polling intervals can hold your maximum power burst?
I have to admit I'm not sure how to determine the optimal intervals here. I initially have both intervals be 1000 ms, but I'm not sure if it's necessary. I need some help on that. The frequency table on the target consists of 396, 792, 996 and 1200 in MHz.
Quoted 7 lines
Here you can do simple calculation on your maximum delta Celsius over delta time. Then check it against your maximum delta power (power at 1.2GHz - power at 396MHz) with respect to your power vs. temp. curve. That will give you a rough expectation on what you need to be sampling. But you probably need help from some guy doing thermal modeling and simulation on your chip.
Ok, thanks for the input. I just consulted the Freescale thermal expert. Though I did not get the modeling and simulation data, I was told that one 1~2 seconds should be safer. So I'm going to change the definitions as below.
#define IMX_POLLING_DELAY 2000 /* millisecond */
#define IMX_PASSIVE_DELAY 1000
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
Quoted 14 lines
drivers/thermal/imx_thermal.c new file +13
-0+ +struct imx_thermal_data { +struct thermal_zone_device *tz; +struct thermal_cooling_device *cdev; +enum thermal_device_mode mode; +struct regmap *tempmon; +bool meas_suspended; +int c1, c2;/* See forumla in imx_get_sensor_data() */ +}; + +static int imx_get_temp (struct thermal_zone_device *tz,unsigned long *temp) +{ +struct imx_thermal_data *data = tz->devdata;drivers/thermal/imx_thermal.c line 88
Quoted 10 lines
drivers/thermal/imx_thermal.c new file +10
-0+struct regmap *map = data->tempmon; +static unsigned long last_temp; +unsigned int n_meas; +u32 val; + +/* +* Every time we measure the temperature, we will power on the +* temperature sensor, enable measurements, take a reading, +* disable measurements, power off the temperature sensor. +*/ Are these sensors used on any hw failsafe mechanism?
Quoted 5 lines
Quoted 1 line
No, I do not think so.
OK. Then safe to turn it off right? Another good reason to have a fine tuned sampling rate.
Yes.
Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
drivers/thermal/imx_thermal.c line 99
Quoted 12 lines
drivers/thermal/imx_thermal.c new file +11
-0+regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_POWER_DOWN ); +regmap_write (map,TEMPSENSE0 +REG_SET ,TEMPSENSE0_MEASURE_TEMP ); + +/* +* According to the temp sensor designers, it may require up to ~17us +* to complete a measurement. But this timing isn't checked on every +* part nor is it specified in the datasheet, so sleeping at least 1ms +* should provide plenty of time. Sleeping longer than 1ms is ok so no +* need for usleep_range. +*/ +msleep (1 );I know 1ms is usually too short time slice when talking about thermal monitoring on nowadays processors even, but 17us to 1ms sounds like an overkill.. Sure you are not hiding other issues?
Quoted 7 lines
Quoted 3 lines
At least none I'm ware of. I will update the comment and simply use
usleep_rangein the next post to prove that I'm not hiding other issues :)OK. It just looks strange that here you need 1ms, but after suspend you can afford waiting only 50us. Anything special about the suspend path?
Nothing special I think. I'm changing the 1ms here to ~50 us.
<snip>
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/thermal/imx_thermal.c line 288
Quoted 1 line
drivers/thermal/imx_thermal.c new file +1
-0+ map =syscon_regmap_lookup_by_phandle (pdev->dev.of_node,"fsl,tempmon" );Is this MMIO? Sure you want to do regmapped access to something so simple as io mem r/w?
Quoted 9 lines
Quoted 5 lines
Yes, MMIO. But it's embedded in some kind of system controller block, call anatop on imx6q, which contains miscellaneous registers for various function blocks. We do not want to have various device drivers iomap the same space multiple times. syscon API which uses regmap was created exactly for such cases, so we use it here.
Then again, I was more on the overhead imposed by regmap. For buses like i2c I understand its usage, but MMIO starts to be a stretch.
The syscon regmap interface is widely used on Freescale platforms as well as others to access register bits embedded in this sort of system controllers. I do not think there is much overhead since it's just a wrapper of MMIO accessors, and besides we do not have the register access on hot path anyway in the driver.
Shawn
PATCH v2 1/1 thermal: add imx thermal driver support
Quoted 9 lines · Shawn Guo
Quoted 8 lines · Eduardo Valentin
Quoted 7 lines
Quoted 5 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt line 1
Quoted 2 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt new file +1
-0@@ -0,0 +1,14 @@ +* Temperature Monitor (TEMPMON) on Freescale i.MX SoCsAny hw documentation reference?
Quoted 7 lines
Quoted 5 lines
OK. Tks.
Some ppl like to put hw doc reference on your driver documentation. But for me is your call. I believe it is good to ask the kernel doc guys too.
I'm not a fan of doing that, because I can not tell that the link will be still valid there in 5 years or so.
Well, I think just mentioning IMX6DQRM should be ok. I didnt really mean you need to put a link.
Quoted 42 lines
Quoted 16 lines
Quoted 9 lines
Quoted 9 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt line 6
Quoted 6 lines
Documentation/devicetree/bindings/thermal/imx-thermal.txt new file +5
-0+ +Required properties: +- compatible : "fsl,imx6q-thermal" +- fsl,tempmon : phandle pointer to TEMPMON control registers +- fsl,tempmon-data : phandle pointer to TEMPMON calibration datawhat exactly is calibration data?
Quoted 6 lines
The temperature calibration values are fused individually for each part in the product testing process. It consists of a pair of hardware monitor values at 25 C and a high temperature which is specified by calibration data itself, which will be used by software to translate hardware counter values to Celsius. The detailed description can be found in hardware reference manual.
Quoted 4 lines
That was what I expected. But it is not that clear what you meant by "thermal cal data" your DT documenation. Maybe you need to describe how to enter a fsl,tempmon-data.
Ok, I will try to come up with something better.
<snip>
Quoted 11 lines
Quoted 10 lines
Quoted 6 lines
drivers/thermal/imx_thermal.c line 65
Quoted 2 lines
drivers/thermal/imx_thermal.c new file +2
-0+#define IMX_POLLING_DELAY 5000/* millisecond */ +#define IMX_PASSIVE_DELAY 3000What is your maximum delta between CPU frequencies? Sure this polling intervals can hold your maximum power burst?
I have to admit I'm not sure how to determine the optimal intervals here. I initially have both intervals be 1000 ms, but I'm not sure if it's necessary. I need some help on that. The frequency table on the target consists of 396, 792, 996 and 1200 in MHz.
Quoted 7 lines
Here you can do simple calculation on your maximum delta Celsius over delta time. Then check it against your maximum delta power (power at 1.2GHz - power at 396MHz) with respect to your power vs. temp. curve. That will give you a rough expectation on what you need to be sampling. But you probably need help from some guy doing thermal modeling and simulation on your chip.
Quoted 7 lines
Ok, thanks for the input. I just consulted the Freescale thermal expert. Though I did not get the modeling and simulation data, I was told that one 1~2 seconds should be safer. So I'm going to change the definitions as below.
#define
IMX_POLLING_DELAY2000 /* millisecond */
#defineIMX_PASSIVE_DELAY1000
cool.
Quoted 28 lines
Quoted 28 lines
Quoted 28 lines
Quoted 28 lines
Quoted 15 lines
drivers/thermal/imx_thermal.c new file +13
-0+ +struct imx_thermal_data { +struct thermal_zone_device *tz; +struct thermal_cooling_device *cdev; +enum thermal_device_mode mode; +struct regmap *tempmon; +bool meas_suspended; +int c1, c2;/* See forumla in imx_get_sensor_data() */ +}; + +static int imx_get_temp (struct thermal_zone_device *tz,unsigned long *temp) +{ +struct imx_thermal_data *data = tz->devdata;drivers/thermal/imx_thermal.c line 88
Quoted 10 lines
drivers/thermal/imx_thermal.c new file +10
-0+struct regmap *map = data->tempmon; +static unsigned long last_temp; +unsigned int n_meas; +u32 val; + +/* +* Every time we measure the temperature, we will power on the +* temperature sensor, enable measurements, take a reading, +* disable measurements, power off the temperature sensor. +*/ Are these sensors used on any hw failsafe mechanism?
Quoted 6 lines
Quoted 5 lines
Quoted 1 line
No, I do not think so.
OK. Then safe to turn it off right? Another good reason to have a fine tuned sampling rate.
Yes.
ok.
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
drivers/thermal/imx_thermal.c line 99
Quoted 13 lines
drivers/thermal/imx_thermal.c new file +11
-0+regmap_write (map,TEMPSENSE0 +REG_CLR ,TEMPSENSE0_POWER_DOWN ); +regmap_write (map,TEMPSENSE0 +REG_SET ,TEMPSENSE0_MEASURE_TEMP ); + +/* +* According to the temp sensor designers, it may require up to ~17us +* to complete a measurement. But this timing isn't checked on every +* part nor is it specified in the datasheet, so sleeping at least 1ms +* should provide plenty of time. Sleeping longer than 1ms is ok so no +* need for usleep_range. +*/ +msleep (1 );I know 1ms is usually too short time slice when talking about thermal monitoring on nowadays processors even, but 17us to 1ms sounds like an overkill.. Sure you are not hiding other issues?
Quoted 9 lines
Quoted 7 lines
Quoted 3 lines
At least none I'm ware of. I will update the comment and simply use
usleep_rangein the next post to prove that I'm not hiding other issues :)OK. It just looks strange that here you need 1ms, but after suspend you can afford waiting only 50us. Anything special about the suspend path?
Nothing special I think. I'm changing the 1ms here to ~50 us.
Ok.
Quoted 13 lines
<snip>
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
drivers/thermal/imx_thermal.c line 288
Quoted 1 line
drivers/thermal/imx_thermal.c new file +1
-0+ map =syscon_regmap_lookup_by_phandle (pdev->dev.of_node,"fsl,tempmon" );Is this MMIO? Sure you want to do regmapped access to something so simple as io mem r/w?
Yes, MMIO. But it's embedded in some kind of system controller block, call anatop on imx6q, which contains miscellaneous registers for various function blocks. We do not want to have various device drivers iomap the same space multiple times. syscon API which uses regmap was created exactly for such cases, so we use it here.
Quoted 8 lines
Quoted 3 lines
Then again, I was more on the overhead imposed by regmap. For buses like i2c I understand its usage, but MMIO starts to be a stretch.
The syscon regmap interface is widely used on Freescale platforms as well as others to access register bits embedded in this sort of system controllers. I do not think there is much overhead since it's just a wrapper of MMIO accessors, and besides we do not have the register access on hot path anyway in the driver.
Ok. If you dont care about the overhead, it is fine, as it is your call to design the driver accessors. FYI, this is a nice explanation of what is my concern:
http://www.spinics.net/lists/linux-omap/msg92370.html
Quoted 5 lines
Shawn
Signature · 11 lines
You have got to be excited about what you are doing. (L. Lamport) Eduardo Valentin -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 295 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130620/76df2dd5/attachment.sig>
PATCH v2 1/1 thermal: add imx thermal driver support
Quoted 3 lines · Eduardo Valentin
Well, I think just mentioning IMX6DQRM should be ok. I didnt really mean you need to put a link.
Ok, will mention SoC Reference Manual in there.
<snip>
Quoted 5 lines
Ok. If you dont care about the overhead, it is fine, as it is your call to design the driver accessors. FYI, this is a nice explanation of what is my concern:
http://www.spinics.net/lists/linux-omap/msg92370.html
Thanks for the pointer. As the IO access here is not on the hot path and is only at second basis, I'm not so concerned by the overhead.
Shawn