[v5] ARM: imx: Add basic imx6q cpu thermal management

Add imx6q cpu thermal management driver using the new cpu cooling
interface which limits system performance via cpufreq to reduce
the cpu temperature.  Temperature readings are taken using
the imx6q temperature sensor and this functionality was added
as part of this cpu thermal management driver.

Signed-off-by: Robert Lee <rob.lee@linaro.org>
---
 arch/arm/boot/dts/imx6q.dtsi    |    5 +
 drivers/thermal/Kconfig         |   28 ++
 drivers/thermal/Makefile        |    1 +
 drivers/thermal/imx6q_thermal.c |  593 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 627 insertions(+)
 create mode 100644 drivers/thermal/imx6q_thermal.c

On Wed, Jun 20, 2012 at 2:06 AM, Robert Lee <rob.lee@linaro.org> wrote:
> Add imx6q cpu thermal management driver using the new cpu cooling
> interface which limits system performance via cpufreq to reduce
> the cpu temperature.  Temperature readings are taken using
> the imx6q temperature sensor and this functionality was added
> as part of this cpu thermal management driver.
>
> Signed-off-by: Robert Lee <rob.lee@linaro.org>
> ---
>  arch/arm/boot/dts/imx6q.dtsi    |    5 +
>  drivers/thermal/Kconfig         |   28 ++
>  drivers/thermal/Makefile        |    1 +
>  drivers/thermal/imx6q_thermal.c |  593 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 627 insertions(+)
>  create mode 100644 drivers/thermal/imx6q_thermal.c
>
> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
> index 8c90cba..2650f65 100644
> --- a/arch/arm/boot/dts/imx6q.dtsi
> +++ b/arch/arm/boot/dts/imx6q.dtsi
> @@ -442,6 +442,10 @@
>                                        anatop-min-voltage = <725000>;
>                                        anatop-max-voltage = <1450000>;
>                                };
> +
> +                               thermal {
> +                                       compatible ="fsl,anatop-thermal";
> +                               };
>                        };
>
>                        usbphy@020c9000 { /* USBPHY1 */
> @@ -659,6 +663,7 @@
>                        };
>
>                        ocotp@021bc000 {
> +                               compatible = "fsl,imx6q-ocotp";
>                                reg = <0x021bc000 0x4000>;
>                        };
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 04c6796..b80c408 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -30,6 +30,34 @@ config CPU_THERMAL
>          and not the ACPI interface.
>          If you want this support, you should say Y or M here.
>
> +config IMX6Q_THERMAL
> +       bool "IMX6Q Thermal interface support"
> +       depends on MFD_ANATOP && CPU_THERMAL
> +       help
> +         Adds thermal management for IMX6Q.
> +
> +config IMX6Q_THERMAL_FAKE_CALIBRATION
> +       bool "IMX6Q fake temperature sensor calibration (FOR TESTING ONLY)"
> +       depends on IMX6Q_THERMAL
> +       help
> +         This enables a fake temp sensor calibration value for parts without
> +         the correction calibration values burned into OCOTP. If the part
> +         already has the calibrated values burned into OCOTP, enabling this
> +         does nothing.
> +         WARNING: Use of this feature is for testing only as it will cause
> +         incorrect temperature readings which will result in incorrect system
> +         thermal limiting behavior such as premature system performance
> +         limiting or lack of proper performance reduction to reduce cpu
> +         temperature
> +
> +config IMX6Q_THERMAL_FAKE_CAL_VAL
> +       hex "IMX6Q fake temperature sensor calibration value"
> +       depends on IMX6Q_THERMAL_FAKE_CALIBRATION
> +       default 0x5704c67d
> +       help
> +         Refer to the temperature sensor section of the imx6q reference manual
> +         for more inforation on how this value is used.
> +
>  config SPEAR_THERMAL
>        bool "SPEAr thermal sensor driver"
>        depends on THERMAL
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 4636e35..fc4004e 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_THERMAL)           += thermal_sys.o
>  obj-$(CONFIG_CPU_THERMAL)       += cpu_cooling.o
>  obj-$(CONFIG_SPEAR_THERMAL)            += spear_thermal.o
>  obj-$(CONFIG_EXYNOS_THERMAL)           += exynos_thermal.o
> +obj-$(CONFIG_IMX6Q_THERMAL)    += imx6q_thermal.o
> diff --git a/drivers/thermal/imx6q_thermal.c b/drivers/thermal/imx6q_thermal.c
> new file mode 100644
> index 0000000..255d646
> --- /dev/null
> +++ b/drivers/thermal/imx6q_thermal.c
> @@ -0,0 +1,593 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + * Copyright 2012 Linaro Ltd.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/* i.MX6Q Thermal Implementation */
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/dmi.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/types.h>
> +#include <linux/thermal.h>
> +#include <linux/io.h>
> +#include <linux/syscalls.h>
> +#include <linux/cpufreq.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/smp.h>
> +#include <linux/cpu_cooling.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/anatop.h>
> +
> +/* register define of anatop */
> +#define HW_ANADIG_ANA_MISC0                    0x00000150
> +#define HW_ANADIG_ANA_MISC0_SET                        0x00000154
> +#define HW_ANADIG_ANA_MISC0_CLR                        0x00000158
> +#define HW_ANADIG_ANA_MISC0_TOG                        0x0000015c
> +#define BM_ANADIG_ANA_MISC0_REFTOP_SELBIASOFF  0x00000008
> +
> +#define HW_ANADIG_TEMPSENSE0                   0x00000180
> +#define HW_ANADIG_TEMPSENSE0_SET               0x00000184
> +#define HW_ANADIG_TEMPSENSE0_CLR               0x00000188
> +#define HW_ANADIG_TEMPSENSE0_TOG               0x0000018c
> +
> +#define BP_ANADIG_TEMPSENSE0_TEMP_VALUE                8
> +#define BM_ANADIG_TEMPSENSE0_TEMP_VALUE                0x000FFF00
> +#define BM_ANADIG_TEMPSENSE0_FINISHED          0x00000004
> +#define BM_ANADIG_TEMPSENSE0_MEASURE_TEMP      0x00000002
> +#define BM_ANADIG_TEMPSENSE0_POWER_DOWN                0x00000001
> +
> +#define HW_ANADIG_TEMPSENSE1                   0x00000190
> +#define HW_ANADIG_TEMPSENSE1_SET               0x00000194
> +#define HW_ANADIG_TEMPSENSE1_CLR               0x00000198
> +#define BP_ANADIG_TEMPSENSE1_MEASURE_FREQ      0
> +#define BM_ANADIG_TEMPSENSE1_MEASURE_FREQ      0x0000FFFF
> +
> +#define HW_OCOTP_ANA1                          0x000004E0
> +
> +#define IMX6Q_THERMAL_POLLING_FREQUENCY_MS 1000
> +
> +#define IMX6Q_THERMAL_ACTIVE_TRP_PTS 3
> +/* assumption: always one critical trip point */
> +#define IMX6Q_THERMAL_TOTAL_TRP_PTS (IMX6Q_THERMAL_ACTIVE_TRP_PTS + 1)
> +
> +struct imx6q_sensor_data {
> +       int     c1, c2;
> +       char    name[16];
> +       u8      meas_delay;     /* in milliseconds */
> +       u32     noise_margin;   /* in millicelcius */
> +       int     last_temp;      /* in millicelcius */
> +       /*
> +        * When noise filtering, if consecutive measurements are each higher
> +        * up to consec_high_limit times, assume changing temperature readings
> +        * to be valid and not noise.
> +        */
> +       u32     consec_high_limit;
> +       struct anatop *anatopmfd;
> +};
> +
> +struct imx6q_thermal_data {
> +       enum thermal_trip_type type[IMX6Q_THERMAL_TOTAL_TRP_PTS];
> +       struct freq_clip_table freq_tab[IMX6Q_THERMAL_ACTIVE_TRP_PTS];
> +       unsigned int crit_temp_level;
> +};
> +
> +struct imx6q_thermal_zone {
> +       struct thermal_zone_device      *therm_dev;
> +       struct thermal_cooling_device   *cool_dev;
> +       struct imx6q_sensor_data        *sensor_data;
> +       struct imx6q_thermal_data       *thermal_data;
> +       struct thermal_zone_device_ops  dev_ops;
> +};
> +
> +static struct imx6q_sensor_data g_sensor_data __devinitdata = {
> +       .name                   = "imx6q-temp_sens",
> +       .meas_delay             = 1,    /* in milliseconds */
> +       .noise_margin           = 3000,
> +       .consec_high_limit      = 3,
> +};
> +
> +/*
> + * This data defines the various temperature trip points that will trigger
> + * cooling action when crossed.
> + */
> +static struct imx6q_thermal_data g_thermal_data __devinitdata = {
> +       .type[0] = THERMAL_TRIP_ACTIVE,
> +       .freq_tab[0] = {
> +               .freq_clip_max = 800 * 1000,
> +               .temp_level = 85000,
> +       },
> +       .type[1] = THERMAL_TRIP_ACTIVE,
> +       .freq_tab[1] = {
> +               .freq_clip_max = 400 * 1000,
> +               .temp_level = 90000,
> +       },
> +       .type[2] = THERMAL_TRIP_ACTIVE,
> +       .freq_tab[2] = {
> +               .freq_clip_max = 200 * 1000,
> +               .temp_level = 95000,
> +       },
> +       .type[3] = THERMAL_TRIP_CRITICAL,
> +       .crit_temp_level = 100000,
> +};
> +
> +static int th_sys_get_temp(struct thermal_zone_device *thermal,
> +                                 unsigned long *temp);
> +
> +static struct imx6q_thermal_zone       *th_zone;
> +static void __iomem                    *ocotp_base;
> +
> +static inline int imx6q_get_temp(int *temp, struct imx6q_sensor_data *sd)
> +{
> +       unsigned int n_meas;
> +       unsigned int reg;
> +
> +       do {
> +               /*
> +                * 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.
> +                */
> +               anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
> +                       BM_ANADIG_TEMPSENSE0_POWER_DOWN);
> +
> +               anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0,
> +                       BM_ANADIG_TEMPSENSE0_MEASURE_TEMP,
> +                       BM_ANADIG_TEMPSENSE0_MEASURE_TEMP);
> +
> +               /*
> +                * According to imx6q 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 we check the
> +                * 'finished' status bit to be sure of completion of a valid
> +                * measurement.
> +                */
> +               msleep(sd->meas_delay);
> +
> +               reg = anatop_read_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0);
> +
> +               anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
> +                       BM_ANADIG_TEMPSENSE0_MEASURE_TEMP);
> +
> +               anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0,
> +                       BM_ANADIG_TEMPSENSE0_POWER_DOWN,
> +                       BM_ANADIG_TEMPSENSE0_POWER_DOWN);
> +
> +       } while (!(reg & BM_ANADIG_TEMPSENSE0_FINISHED) &&
> +                (reg & BM_ANADIG_TEMPSENSE0_TEMP_VALUE));
> +
> +       n_meas = (reg & BM_ANADIG_TEMPSENSE0_TEMP_VALUE)
> +                       >> BP_ANADIG_TEMPSENSE0_TEMP_VALUE;
> +
> +       /* See imx6q_thermal_process_fuse_data() for forumla derivation. */
> +       *temp = sd->c2 + (sd->c1 * n_meas);
> +
> +       pr_debug("Temperature: %d\n", *temp / 1000);
> +
> +       return 0;
> +}
> +
> +static int th_sys_get_temp(struct thermal_zone_device *thermal,
> +                                 unsigned long *temp)
> +{
> +       int i, total = 0, tmp = 0;
> +       const u8 loop = 5;
> +       u32 consec_high = 0;
> +
> +       struct imx6q_sensor_data *sd;
> +
> +       sd = ((struct imx6q_thermal_zone *)thermal->devdata)->sensor_data;
> +
> +       /*
> +        * Measure temperature and handle noise
> +        *
> +        * While the imx6q temperature sensor is designed to minimize being
> +        * affected by system noise, it's safest to run sanity checks and
> +        * perform any necessary filitering.
> +        */
> +       for (i = 0; (sd->noise_margin) && (i < loop); i++) {
> +               imx6q_get_temp(&tmp, sd);
> +
> +               if ((abs(tmp - sd->last_temp) <= sd->noise_margin) ||
> +                       (consec_high >= sd->consec_high_limit)) {
> +                       sd->last_temp = tmp;
> +                       i = 0;
> +                       break;
> +               }
> +               if (tmp > sd->last_temp)
> +                       consec_high++;
> +
> +               /*
> +                * ignore first measurement as the previous measurement was
> +                * a long time ago.
> +                */
> +               if (i)
> +                       total += tmp;
> +
> +               sd->last_temp = tmp;
> +       }
> +
> +       if (sd->noise_margin && i)
> +               tmp = total / (loop - 1);
> +
> +       /*
> +        * The thermal framework code stores temperature in unsigned long. Also,
> +        * it has references to "millicelcius" which limits the lowest
> +        * temperature possible (compared to Kelvin).
> +        */
> +       if (tmp > 0)
> +               *temp = tmp;
> +       else
> +               *temp = 0;
> +
> +       return 0;
> +}
> +
> +static int th_sys_get_mode(struct thermal_zone_device *thermal,
> +                           enum thermal_device_mode *mode)
> +{
> +       *mode = THERMAL_DEVICE_ENABLED;
> +       return 0;
> +}
> +
> +static int th_sys_get_trip_type(struct thermal_zone_device *thermal, int trip,
> +                                enum thermal_trip_type *type)
> +{
> +       struct imx6q_thermal_data *p;
> +
> +       if (trip >= thermal->trips)
> +               return -EINVAL;
> +
> +       p = ((struct imx6q_thermal_zone *)thermal->devdata)->thermal_data;
> +
> +       *type = p->type[trip];
> +
> +       return 0;
> +}
> +
> +static int th_sys_get_trip_temp(struct thermal_zone_device *thermal, int trip,
> +                                unsigned long *temp)
> +{
> +       struct imx6q_thermal_data *p;
> +       enum thermal_trip_type type;
> +
> +       if (trip >= thermal->trips)
> +               return -EINVAL;
> +
> +       p = ((struct imx6q_thermal_zone *)thermal->devdata)->thermal_data;
> +
> +       thermal->ops->get_trip_type(thermal, trip, &type);
> +
> +       if (type == THERMAL_TRIP_CRITICAL)
> +               *temp = p->crit_temp_level;
> +       else
> +               *temp = p->freq_tab[trip].temp_level;
> +       return 0;
> +}
> +
> +static int th_sys_get_crit_temp(struct thermal_zone_device *thermal,
> +                                unsigned long *temp)
> +{
> +       struct imx6q_thermal_data *p;
> +
> +       p = ((struct imx6q_thermal_zone *)thermal->devdata)->thermal_data;
> +       *temp = p->crit_temp_level;
> +       return 0;
> +}
> +
> +static int th_sys_bind(struct thermal_zone_device *thermal,
> +                       struct thermal_cooling_device *cdev)
> +{
> +       int i;
> +       struct thermal_cooling_device *cd;
> +
> +       cd = ((struct imx6q_thermal_zone *)thermal->devdata)->cool_dev;
> +
> +       /* if the cooling device is the one from imx6 bind it */
> +       if (cdev != cd)
> +               return 0;
> +
> +       for (i = 0; i < IMX6Q_THERMAL_ACTIVE_TRP_PTS; i++) {
> +               if (thermal_zone_bind_cooling_device(thermal, i, cdev)) {
> +                       pr_err("error binding cooling dev\n");
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static int th_sys_unbind(struct thermal_zone_device *thermal,
> +                         struct thermal_cooling_device *cdev)
> +{
> +       int i;
> +
> +       struct thermal_cooling_device *cd;
> +
> +       cd = ((struct imx6q_thermal_zone *)thermal->devdata)->cool_dev;
> +
> +       if (cdev != cd)
> +               return 0;
> +
> +       for (i = 0; i < IMX6Q_THERMAL_ACTIVE_TRP_PTS; i++) {
> +               if (thermal_zone_unbind_cooling_device(thermal, i, cdev)) {
> +                       pr_err("error unbinding cooling dev\n");
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static struct thermal_zone_device_ops g_dev_ops __devinitdata = {
> +       .bind = th_sys_bind,
> +       .unbind = th_sys_unbind,
> +       .get_temp = th_sys_get_temp,
> +       .get_mode = th_sys_get_mode,
> +       .get_trip_type = th_sys_get_trip_type,
> +       .get_trip_temp = th_sys_get_trip_temp,
> +       .get_crit_temp = th_sys_get_crit_temp,
> +};
> +
> +static int __devinit imx6q_thermal_process_fuse_data(unsigned int fuse_data,
> +                struct imx6q_sensor_data *sd)
> +{
> +       int t1, t2, n1, n2;
> +
> +       if (fuse_data == 0 || fuse_data == 0xffffffff) {
> +               pr_warn("WARNING: Incorrect temperature sensor value of %x "
> +                       "detected\n", fuse_data);
> +
> +#ifdef CONFIG_IMX6Q_THERMAL_FAKE_CALIBRATION
> +               pr_warn(
> +                       "WARNING: Using fake calibration value of %x value. "
> +                       "This will cause your system performance to be limited "
> +                       "prematurely (compared to a using properly calibrated "
> +                       "value) OR will cause the system to allow the "
> +                       "temperature to exceed the maximum specified "
> +                       "temperature without the proper performance "
> +                       "limiting.\n", CONFIG_IMX6Q_THERMAL_FAKE_CAL_VAL);
> +
> +               fuse_data = CONFIG_IMX6Q_THERMAL_FAKE_CAL_VAL;
> +#else
> +               pr_warn("WARNING: Disabling CPU thermal protection\n");
> +               return -EINVAL;
> +#endif
> +       }
> +
> +       /*
> +        * Fuse data layout:
> +        * [31:20] sensor value @ 25C
> +        * [19:8] sensor value of hot
> +        * [7:0] hot temperature value
> +        */
> +       n1 = fuse_data >> 20;
> +       n2 = (fuse_data & 0xfff00) >> 8;
> +       t2 = fuse_data & 0xff;
> +       t1 = 25; /* t1 always 25C */
> +
> +       pr_debug(" -- temperature sensor calibration data --\n");
> +       pr_debug("HW_OCOTP_ANA1: %x\n", fuse_data);
> +       pr_debug("n1: %d\nn2: %d\nt1: %d\nt2: %d\n", n1, n2, t1, t2);
> +
> +       /*
> +        * 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 millicelcius
> +        * 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)
> +        */
> +       sd->c1 = (1000 * (t1 - t2)) / (n1 - n2);
> +       sd->c2 = (1000 * t2) - (sd->c1 * n2);
> +
> +       pr_debug("c1: %i\n", sd->c1);
> +       pr_debug("c2: %i\n", sd->c2);
> +
> +       return 0;
> +}
> +
> +static int anatop_thermal_suspend(struct platform_device *pdev,
> +                                       pm_message_t state)
> +{
> +       struct imx6q_sensor_data *sd = pdev->dev.platform_data;
> +
> +       /* power off the sensor during suspend */
> +       anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
> +               BM_ANADIG_TEMPSENSE0_MEASURE_TEMP);
> +
> +       anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0,
> +               BM_ANADIG_TEMPSENSE0_POWER_DOWN,
> +               BM_ANADIG_TEMPSENSE0_POWER_DOWN);
> +
> +       return 0;
> +}
> +
> +static int __devexit anatop_thermal_remove(struct platform_device *pdev)
> +{
> +       if (th_zone && th_zone->therm_dev)
> +               thermal_zone_device_unregister(th_zone->therm_dev);
> +
> +       if (th_zone && th_zone->cool_dev)
> +               cpufreq_cooling_unregister(th_zone->cool_dev);
> +
> +       kfree(th_zone->sensor_data);
> +       kfree(th_zone->thermal_data);
> +       kfree(th_zone);
> +
> +       if (ocotp_base)
> +               iounmap(ocotp_base);
> +
> +       pr_info("i.MX6Q: Kernel Thermal management unregistered\n");
> +
> +       return 0;
> +}
> +
> +static int __devinit anatop_thermal_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np_ocotp, *np_thermal;
> +       unsigned int fuse_data;
> +       struct anatop *anatopmfd = dev_get_drvdata(pdev->dev.parent);
> +       int ret, i;
> +
> +       np_ocotp = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ocotp");
> +       np_thermal = of_find_compatible_node(NULL, NULL, "fsl,anatop-thermal");
> +
> +       if (!(np_ocotp && np_thermal && anatopmfd))
> +               return -ENXIO; /* not a compatible platform */

Hmmm, I need to change this to return 0 (for multi platform builds).
Or change the intialization to be imx6q only.

> +
> +       ocotp_base = of_iomap(np_ocotp, 0);
> +
> +       if (!ocotp_base) {
> +               pr_err("Could not retrieve ocotp-base\n");
> +               ret = -ENXIO;
> +               goto err_unregister;
> +       }
> +
> +       fuse_data = readl_relaxed(ocotp_base + HW_OCOTP_ANA1);
> +
> +       th_zone = kzalloc(sizeof(struct imx6q_thermal_zone), GFP_KERNEL);
> +       if (!th_zone) {
> +               ret = -ENOMEM;
> +               goto err_unregister;
> +       }
> +
> +       th_zone->dev_ops = g_dev_ops;
> +
> +       th_zone->thermal_data =
> +               kzalloc(sizeof(struct imx6q_thermal_data), GFP_KERNEL);
> +
> +       if (!th_zone->thermal_data) {
> +               ret = -ENOMEM;
> +               goto err_unregister;
> +       }
> +
> +       memcpy(th_zone->thermal_data, &g_thermal_data,
> +                               sizeof(struct imx6q_thermal_data));
> +
> +       for (i = 0; i < ARRAY_SIZE(g_thermal_data.freq_tab); i++)
> +               th_zone->thermal_data->freq_tab[i].mask_val = cpumask_of(0);
> +
> +
> +       th_zone->sensor_data =
> +               kzalloc(sizeof(struct imx6q_sensor_data), GFP_KERNEL);
> +
> +       if (!th_zone->sensor_data) {
> +               ret = -ENOMEM;
> +               goto err_unregister;
> +       }
> +
> +       memcpy(th_zone->sensor_data, &g_sensor_data,
> +                               sizeof(struct imx6q_sensor_data));
> +
> +       th_zone->sensor_data->anatopmfd = anatopmfd;
> +
> +       ret = imx6q_thermal_process_fuse_data(fuse_data, th_zone->sensor_data);
> +
> +       if (ret) {
> +               pr_err("Invalid temperature calibration data.\n");
> +               goto err_unregister;
> +       }
> +
> +       if (!th_zone->sensor_data->meas_delay)
> +               th_zone->sensor_data->meas_delay = 1;
> +
> +       /* Make sure sensor is in known good state for measurements */
> +       anatop_write_reg(anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
> +               BM_ANADIG_TEMPSENSE0_POWER_DOWN);
> +
> +       anatop_write_reg(anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
> +               BM_ANADIG_TEMPSENSE0_MEASURE_TEMP);
> +
> +       anatop_write_reg(anatopmfd, HW_ANADIG_TEMPSENSE1, 0,
> +               BM_ANADIG_TEMPSENSE1_MEASURE_FREQ);
> +
> +       anatop_write_reg(anatopmfd, HW_ANADIG_ANA_MISC0_SET,
> +               BM_ANADIG_ANA_MISC0_REFTOP_SELBIASOFF,
> +               BM_ANADIG_ANA_MISC0_REFTOP_SELBIASOFF);
> +
> +       anatop_write_reg(anatopmfd, HW_ANADIG_TEMPSENSE0,
> +               BM_ANADIG_TEMPSENSE0_POWER_DOWN,
> +               BM_ANADIG_TEMPSENSE0_POWER_DOWN);
> +
> +       th_zone->cool_dev = cpufreq_cooling_register(
> +               (struct freq_clip_table *)th_zone->thermal_data->freq_tab,
> +               IMX6Q_THERMAL_ACTIVE_TRP_PTS);
> +
> +       if (IS_ERR(th_zone->cool_dev)) {
> +               pr_err("Failed to register cpufreq cooling device\n");
> +               ret = -EINVAL;
> +               goto err_unregister;
> +       }
> +
> +       th_zone->therm_dev = thermal_zone_device_register(
> +               th_zone->sensor_data->name, IMX6Q_THERMAL_TOTAL_TRP_PTS,
> +               th_zone, &th_zone->dev_ops, 0, 0, 0,
> +               IMX6Q_THERMAL_POLLING_FREQUENCY_MS);
> +
> +       if (IS_ERR(th_zone->therm_dev)) {
> +               pr_err("Failed to register thermal zone device\n");
> +               ret = -EINVAL;
> +               goto err_unregister;
> +       }
> +
> +       pdev->dev.platform_data = th_zone->sensor_data;
> +
> +       return 0;
> +
> +err_unregister:
> +       anatop_thermal_remove(pdev);
> +       return ret;
> +}
> +
> +static struct of_device_id __devinitdata of_anatop_thermal_match_tbl[] = {
> +       { .compatible = "fsl,anatop-thermal", },
> +       { /* end */ }
> +};
> +
> +static struct platform_driver anatop_thermal = {
> +       .driver = {
> +               .name   = "anatop_thermal",
> +               .owner  = THIS_MODULE,
> +               .of_match_table = of_anatop_thermal_match_tbl,
> +       },
> +       .probe  = anatop_thermal_probe,
> +       .remove = anatop_thermal_remove,
> +       .suspend = anatop_thermal_suspend,
> +       /* due to implentation of imx6q_get_temp , resume is unnecessary */
> +};
> +
> +static int __devinit anatop_thermal_init(void)
> +{
> +       return platform_driver_register(&anatop_thermal);
> +}
> +device_initcall(anatop_thermal_init);
> +
> +static void __exit anatop_thermal_exit(void)
> +{
> +       platform_driver_unregister(&anatop_thermal);
> +}
> +module_exit(anatop_thermal_exit);
> +
> +MODULE_AUTHOR("Freescale Semiconductor");
> +MODULE_DESCRIPTION("i.MX6Q SoC thermal driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:imx6q-thermal");
> --
> 1.7.10
>

On Wed, Jun 20, 2012 at 02:06:04AM -0500, Robert Lee wrote:
> Add imx6q cpu thermal management driver using the new cpu cooling
> interface which limits system performance via cpufreq to reduce
> the cpu temperature.  Temperature readings are taken using
> the imx6q temperature sensor and this functionality was added
> as part of this cpu thermal management driver.
> 
> Signed-off-by: Robert Lee <rob.lee@linaro.org>
> ---
>  arch/arm/boot/dts/imx6q.dtsi    |    5 +
>  drivers/thermal/Kconfig         |   28 ++
>  drivers/thermal/Makefile        |    1 +
>  drivers/thermal/imx6q_thermal.c |  593 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 627 insertions(+)
>  create mode 100644 drivers/thermal/imx6q_thermal.c
> 
> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
> index 8c90cba..2650f65 100644
> --- a/arch/arm/boot/dts/imx6q.dtsi
> +++ b/arch/arm/boot/dts/imx6q.dtsi
> @@ -442,6 +442,10 @@
>  					anatop-min-voltage = <725000>;
>  					anatop-max-voltage = <1450000>;
>  				};
> +
> +				thermal {
> +					compatible ="fsl,anatop-thermal";
> +				};
>  			};
>  
>  			usbphy@020c9000 { /* USBPHY1 */
> @@ -659,6 +663,7 @@
>  			};
>  
>  			ocotp@021bc000 {
> +				compatible = "fsl,imx6q-ocotp";
>  				reg = <0x021bc000 0x4000>;
>  			};
>  
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 04c6796..b80c408 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -30,6 +30,34 @@ config CPU_THERMAL
>  	  and not the ACPI interface.
>  	  If you want this support, you should say Y or M here.
>  
> +config IMX6Q_THERMAL
> +	bool "IMX6Q Thermal interface support"
> +	depends on MFD_ANATOP && CPU_THERMAL
> +	help
> +	  Adds thermal management for IMX6Q.
> +
> +config IMX6Q_THERMAL_FAKE_CALIBRATION
> +	bool "IMX6Q fake temperature sensor calibration (FOR TESTING ONLY)"
> +	depends on IMX6Q_THERMAL
> +	help
> +	  This enables a fake temp sensor calibration value for parts without
> +	  the correction calibration values burned into OCOTP. If the part
> +	  already has the calibrated values burned into OCOTP, enabling this
> +	  does nothing.
> +	  WARNING: Use of this feature is for testing only as it will cause
> +	  incorrect temperature readings which will result in incorrect system
> +	  thermal limiting behavior such as premature system performance
> +	  limiting or lack of proper performance reduction to reduce cpu
> +	  temperature
> +
> +config IMX6Q_THERMAL_FAKE_CAL_VAL
> +	hex "IMX6Q fake temperature sensor calibration value"
> +	depends on IMX6Q_THERMAL_FAKE_CALIBRATION
> +	default 0x5704c67d
> +	help
> +	  Refer to the temperature sensor section of the imx6q reference manual
> +	  for more inforation on how this value is used.

Don't add such stuff to Kconfig. If during runtime you detect that there
is no calibration data, then issue a warning and fall back to a safe
value. If you really think this should be configurable, add a sysfs
entry for it. "FOR TESTING ONLY" seems to imply though that it shouldn't
be configurable.


> +
>  config SPEAR_THERMAL
>  	bool "SPEAr thermal sensor driver"
>  	depends on THERMAL
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 4636e35..fc4004e 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_THERMAL)		+= thermal_sys.o
>  obj-$(CONFIG_CPU_THERMAL)       += cpu_cooling.o
>  obj-$(CONFIG_SPEAR_THERMAL)		+= spear_thermal.o
>  obj-$(CONFIG_EXYNOS_THERMAL)		+= exynos_thermal.o
> +obj-$(CONFIG_IMX6Q_THERMAL)	+= imx6q_thermal.o
> diff --git a/drivers/thermal/imx6q_thermal.c b/drivers/thermal/imx6q_thermal.c
> new file mode 100644
> index 0000000..255d646
> --- /dev/null
> +++ b/drivers/thermal/imx6q_thermal.c
> @@ -0,0 +1,593 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + * Copyright 2012 Linaro Ltd.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/* i.MX6Q Thermal Implementation */
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/dmi.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/types.h>
> +#include <linux/thermal.h>
> +#include <linux/io.h>
> +#include <linux/syscalls.h>
> +#include <linux/cpufreq.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/smp.h>
> +#include <linux/cpu_cooling.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/anatop.h>
> +
> +/* register define of anatop */
> +#define HW_ANADIG_ANA_MISC0			0x00000150
> +#define HW_ANADIG_ANA_MISC0_SET			0x00000154
> +#define HW_ANADIG_ANA_MISC0_CLR			0x00000158
> +#define HW_ANADIG_ANA_MISC0_TOG			0x0000015c
> +#define BM_ANADIG_ANA_MISC0_REFTOP_SELBIASOFF	0x00000008
> +
> +#define HW_ANADIG_TEMPSENSE0			0x00000180
> +#define HW_ANADIG_TEMPSENSE0_SET		0x00000184
> +#define HW_ANADIG_TEMPSENSE0_CLR		0x00000188
> +#define HW_ANADIG_TEMPSENSE0_TOG		0x0000018c
> +
> +#define BP_ANADIG_TEMPSENSE0_TEMP_VALUE		8
> +#define BM_ANADIG_TEMPSENSE0_TEMP_VALUE		0x000FFF00
> +#define BM_ANADIG_TEMPSENSE0_FINISHED		0x00000004
> +#define BM_ANADIG_TEMPSENSE0_MEASURE_TEMP	0x00000002
> +#define BM_ANADIG_TEMPSENSE0_POWER_DOWN		0x00000001
> +
> +#define HW_ANADIG_TEMPSENSE1			0x00000190
> +#define HW_ANADIG_TEMPSENSE1_SET		0x00000194
> +#define HW_ANADIG_TEMPSENSE1_CLR		0x00000198
> +#define BP_ANADIG_TEMPSENSE1_MEASURE_FREQ	0
> +#define BM_ANADIG_TEMPSENSE1_MEASURE_FREQ	0x0000FFFF
> +
> +#define HW_OCOTP_ANA1				0x000004E0
> +
> +#define IMX6Q_THERMAL_POLLING_FREQUENCY_MS 1000
> +
> +#define IMX6Q_THERMAL_ACTIVE_TRP_PTS 3
> +/* assumption: always one critical trip point */
> +#define IMX6Q_THERMAL_TOTAL_TRP_PTS (IMX6Q_THERMAL_ACTIVE_TRP_PTS + 1)
> +
> +struct imx6q_sensor_data {
> +	int	c1, c2;
> +	char	name[16];
> +	u8	meas_delay;	/* in milliseconds */
> +	u32	noise_margin;   /* in millicelcius */
> +	int	last_temp;	/* in millicelcius */

s/celcius/celsius/

> +	/*
> +	 * When noise filtering, if consecutive measurements are each higher
> +	 * up to consec_high_limit times, assume changing temperature readings
> +	 * to be valid and not noise.
> +	 */
> +	u32	consec_high_limit;
> +	struct anatop *anatopmfd;
> +};
> +
> +struct imx6q_thermal_data {
> +	enum thermal_trip_type type[IMX6Q_THERMAL_TOTAL_TRP_PTS];
> +	struct freq_clip_table freq_tab[IMX6Q_THERMAL_ACTIVE_TRP_PTS];
> +	unsigned int crit_temp_level;
> +};
> +
> +struct imx6q_thermal_zone {
> +	struct thermal_zone_device	*therm_dev;
> +	struct thermal_cooling_device	*cool_dev;
> +	struct imx6q_sensor_data	*sensor_data;
> +	struct imx6q_thermal_data	*thermal_data;
> +	struct thermal_zone_device_ops	dev_ops;
> +};
> +
> +static struct imx6q_sensor_data g_sensor_data __devinitdata = {
> +	.name			= "imx6q-temp_sens",
> +	.meas_delay		= 1,	/* in milliseconds */

This is initialized here, once again in probe and never changed to any
other value.

> +	.noise_margin		= 3000,
> +	.consec_high_limit	= 3,
> +};

Also constant values. What's the purpose of collecting these in this
struct?

> +
> +/*
> + * This data defines the various temperature trip points that will trigger
> + * cooling action when crossed.
> + */
> +static struct imx6q_thermal_data g_thermal_data __devinitdata = {
> +	.type[0] = THERMAL_TRIP_ACTIVE,
> +	.freq_tab[0] = {
> +		.freq_clip_max = 800 * 1000,
> +		.temp_level = 85000,
> +	},
> +	.type[1] = THERMAL_TRIP_ACTIVE,
> +	.freq_tab[1] = {
> +		.freq_clip_max = 400 * 1000,
> +		.temp_level = 90000,
> +	},
> +	.type[2] = THERMAL_TRIP_ACTIVE,
> +	.freq_tab[2] = {
> +		.freq_clip_max = 200 * 1000,
> +		.temp_level = 95000,
> +	},
> +	.type[3] = THERMAL_TRIP_CRITICAL,
> +	.crit_temp_level = 100000,
> +};
> +
> +static int th_sys_get_temp(struct thermal_zone_device *thermal,
> +				  unsigned long *temp);
> +
> +static struct imx6q_thermal_zone	*th_zone;
> +static void __iomem			*ocotp_base;

This is a driver and drivers should generally be multi instance safe.

> +
> +static inline int imx6q_get_temp(int *temp, struct imx6q_sensor_data *sd)
> +{
> +	unsigned int n_meas;
> +	unsigned int reg;
> +
> +	do {
> +		/*
> +		 * 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.
> +		 */
> +		anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
> +			BM_ANADIG_TEMPSENSE0_POWER_DOWN);
> +
> +		anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0,
> +			BM_ANADIG_TEMPSENSE0_MEASURE_TEMP,
> +			BM_ANADIG_TEMPSENSE0_MEASURE_TEMP);
> +
> +		/*
> +		 * According to imx6q 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 we check the
> +		 * 'finished' status bit to be sure of completion of a valid
> +		 * measurement.
> +		 */
> +		msleep(sd->meas_delay);

The comment seems to have nothing to do with calling msleep, and it's
not clear to me why you have to put the argument to msleep into a
struct.

> +
> +		reg = anatop_read_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0);
> +
> +		anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
> +			BM_ANADIG_TEMPSENSE0_MEASURE_TEMP);
> +
> +		anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0,
> +			BM_ANADIG_TEMPSENSE0_POWER_DOWN,
> +			BM_ANADIG_TEMPSENSE0_POWER_DOWN);
> +
> +	} while (!(reg & BM_ANADIG_TEMPSENSE0_FINISHED) &&
> +		 (reg & BM_ANADIG_TEMPSENSE0_TEMP_VALUE));

You should never solely depend on hardware to break out of loops.

> +
> +	n_meas = (reg & BM_ANADIG_TEMPSENSE0_TEMP_VALUE)
> +			>> BP_ANADIG_TEMPSENSE0_TEMP_VALUE;
> +
> +	/* See imx6q_thermal_process_fuse_data() for forumla derivation. */
> +	*temp = sd->c2 + (sd->c1 * n_meas);
> +
> +	pr_debug("Temperature: %d\n", *temp / 1000);
> +
> +	return 0;
> +}
> +
> +static int th_sys_get_temp(struct thermal_zone_device *thermal,
> +				  unsigned long *temp)
> +{
> +	int i, total = 0, tmp = 0;
> +	const u8 loop = 5;
> +	u32 consec_high = 0;
> +
> +	struct imx6q_sensor_data *sd;
> +
> +	sd = ((struct imx6q_thermal_zone *)thermal->devdata)->sensor_data;
> +
> +	/*
> +	 * Measure temperature and handle noise
> +	 *
> +	 * While the imx6q temperature sensor is designed to minimize being
> +	 * affected by system noise, it's safest to run sanity checks and
> +	 * perform any necessary filitering.

s/filitering/filtering/

> +	 */
> +	for (i = 0; (sd->noise_margin) && (i < loop); i++) {
> +		imx6q_get_temp(&tmp, sd);
> +
> +		if ((abs(tmp - sd->last_temp) <= sd->noise_margin) ||
> +			(consec_high >= sd->consec_high_limit)) {
> +			sd->last_temp = tmp;
> +			i = 0;
> +			break;
> +		}
> +		if (tmp > sd->last_temp)
> +			consec_high++;
> +
> +		/*
> +		 * ignore first measurement as the previous measurement was
> +		 * a long time ago.
> +		 */
> +		if (i)
> +			total += tmp;
> +
> +		sd->last_temp = tmp;
> +	}
> +
> +	if (sd->noise_margin && i)
> +		tmp = total / (loop - 1);
> +
> +	/*
> +	 * The thermal framework code stores temperature in unsigned long. Also,
> +	 * it has references to "millicelcius" which limits the lowest

s/celcius/celsius/

> +	 * temperature possible (compared to Kelvin).
> +	 */
> +	if (tmp > 0)
> +		*temp = tmp;
> +	else
> +		*temp = 0;
> +
> +	return 0;
> +}
> +
> +static int th_sys_get_mode(struct thermal_zone_device *thermal,
> +			    enum thermal_device_mode *mode)
> +{
> +	*mode = THERMAL_DEVICE_ENABLED;
> +	return 0;
> +}
> +
> +static int th_sys_get_trip_type(struct thermal_zone_device *thermal, int trip,
> +				 enum thermal_trip_type *type)
> +{
> +	struct imx6q_thermal_data *p;
> +
> +	if (trip >= thermal->trips)
> +		return -EINVAL;
> +
> +	p = ((struct imx6q_thermal_zone *)thermal->devdata)->thermal_data;
> +
> +	*type = p->type[trip];
> +
> +	return 0;
> +}
> +
> +static int th_sys_get_trip_temp(struct thermal_zone_device *thermal, int trip,
> +				 unsigned long *temp)
> +{
> +	struct imx6q_thermal_data *p;
> +	enum thermal_trip_type type;
> +
> +	if (trip >= thermal->trips)
> +		return -EINVAL;
> +
> +	p = ((struct imx6q_thermal_zone *)thermal->devdata)->thermal_data;
> +
> +	thermal->ops->get_trip_type(thermal, trip, &type);
> +
> +	if (type == THERMAL_TRIP_CRITICAL)
> +		*temp = p->crit_temp_level;
> +	else
> +		*temp = p->freq_tab[trip].temp_level;
> +	return 0;
> +}
> +
> +static int th_sys_get_crit_temp(struct thermal_zone_device *thermal,
> +				 unsigned long *temp)
> +{
> +	struct imx6q_thermal_data *p;
> +
> +	p = ((struct imx6q_thermal_zone *)thermal->devdata)->thermal_data;
> +	*temp = p->crit_temp_level;
> +	return 0;
> +}
> +
> +static int th_sys_bind(struct thermal_zone_device *thermal,
> +			struct thermal_cooling_device *cdev)
> +{
> +	int i;
> +	struct thermal_cooling_device *cd;
> +
> +	cd = ((struct imx6q_thermal_zone *)thermal->devdata)->cool_dev;
> +
> +	/* if the cooling device is the one from imx6 bind it */
> +	if (cdev != cd)
> +		return 0;
> +
> +	for (i = 0; i < IMX6Q_THERMAL_ACTIVE_TRP_PTS; i++) {
> +		if (thermal_zone_bind_cooling_device(thermal, i, cdev)) {
> +			pr_err("error binding cooling dev\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int th_sys_unbind(struct thermal_zone_device *thermal,
> +			  struct thermal_cooling_device *cdev)
> +{
> +	int i;
> +
> +	struct thermal_cooling_device *cd;
> +
> +	cd = ((struct imx6q_thermal_zone *)thermal->devdata)->cool_dev;
> +
> +	if (cdev != cd)
> +		return 0;
> +
> +	for (i = 0; i < IMX6Q_THERMAL_ACTIVE_TRP_PTS; i++) {
> +		if (thermal_zone_unbind_cooling_device(thermal, i, cdev)) {
> +			pr_err("error unbinding cooling dev\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static struct thermal_zone_device_ops g_dev_ops __devinitdata = {
> +	.bind = th_sys_bind,
> +	.unbind = th_sys_unbind,
> +	.get_temp = th_sys_get_temp,
> +	.get_mode = th_sys_get_mode,
> +	.get_trip_type = th_sys_get_trip_type,
> +	.get_trip_temp = th_sys_get_trip_temp,
> +	.get_crit_temp = th_sys_get_crit_temp,
> +};
> +
> +static int __devinit imx6q_thermal_process_fuse_data(unsigned int fuse_data,
> +		 struct imx6q_sensor_data *sd)
> +{
> +	int t1, t2, n1, n2;
> +
> +	if (fuse_data == 0 || fuse_data == 0xffffffff) {
> +		pr_warn("WARNING: Incorrect temperature sensor value of %x "
> +			"detected\n", fuse_data);
> +
> +#ifdef CONFIG_IMX6Q_THERMAL_FAKE_CALIBRATION
> +		pr_warn(
> +			"WARNING: Using fake calibration value of %x value. "
> +			"This will cause your system performance to be limited "
> +			"prematurely (compared to a using properly calibrated "
> +			"value) OR will cause the system to allow the "
> +			"temperature to exceed the maximum specified "
> +			"temperature without the proper performance "
> +			"limiting.\n", CONFIG_IMX6Q_THERMAL_FAKE_CAL_VAL);
> +
> +		fuse_data = CONFIG_IMX6Q_THERMAL_FAKE_CAL_VAL;
> +#else
> +		pr_warn("WARNING: Disabling CPU thermal protection\n");
> +		return -EINVAL;
> +#endif
> +	}
> +
> +	/*
> +	 * Fuse data layout:
> +	 * [31:20] sensor value @ 25C
> +	 * [19:8] sensor value of hot
> +	 * [7:0] hot temperature value
> +	 */
> +	n1 = fuse_data >> 20;
> +	n2 = (fuse_data & 0xfff00) >> 8;
> +	t2 = fuse_data & 0xff;
> +	t1 = 25; /* t1 always 25C */
> +
> +	pr_debug(" -- temperature sensor calibration data --\n");
> +	pr_debug("HW_OCOTP_ANA1: %x\n", fuse_data);
> +	pr_debug("n1: %d\nn2: %d\nt1: %d\nt2: %d\n", n1, n2, t1, t2);
> +
> +	/*
> +	 * 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 millicelcius
> +	 * 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)
> +	 */
> +	sd->c1 = (1000 * (t1 - t2)) / (n1 - n2);
> +	sd->c2 = (1000 * t2) - (sd->c1 * n2);
> +
> +	pr_debug("c1: %i\n", sd->c1);
> +	pr_debug("c2: %i\n", sd->c2);
> +
> +	return 0;
> +}
> +
> +static int anatop_thermal_suspend(struct platform_device *pdev,
> +					pm_message_t state)
> +{
> +	struct imx6q_sensor_data *sd = pdev->dev.platform_data;
> +
> +	/* power off the sensor during suspend */
> +	anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
> +		BM_ANADIG_TEMPSENSE0_MEASURE_TEMP);
> +
> +	anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0,
> +		BM_ANADIG_TEMPSENSE0_POWER_DOWN,
> +		BM_ANADIG_TEMPSENSE0_POWER_DOWN);
> +
> +	return 0;
> +}
> +
> +static int __devexit anatop_thermal_remove(struct platform_device *pdev)
> +{
> +	if (th_zone && th_zone->therm_dev)
> +		thermal_zone_device_unregister(th_zone->therm_dev);
> +
> +	if (th_zone && th_zone->cool_dev)
> +		cpufreq_cooling_unregister(th_zone->cool_dev);
> +
> +	kfree(th_zone->sensor_data);
> +	kfree(th_zone->thermal_data);
> +	kfree(th_zone);
> +
> +	if (ocotp_base)
> +		iounmap(ocotp_base);
> +
> +	pr_info("i.MX6Q: Kernel Thermal management unregistered\n");
> +
> +	return 0;
> +}
> +
> +static int __devinit anatop_thermal_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np_ocotp, *np_thermal;
> +	unsigned int fuse_data;
> +	struct anatop *anatopmfd = dev_get_drvdata(pdev->dev.parent);
> +	int ret, i;
> +
> +	np_ocotp = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ocotp");
> +	np_thermal = of_find_compatible_node(NULL, NULL, "fsl,anatop-thermal");

np_thermal = pdev->dev.of_node;

> +
> +	if (!(np_ocotp && np_thermal && anatopmfd))
> +		return -ENXIO; /* not a compatible platform */
> +
> +	ocotp_base = of_iomap(np_ocotp, 0);
> +
> +	if (!ocotp_base) {
> +		pr_err("Could not retrieve ocotp-base\n");
> +		ret = -ENXIO;
> +		goto err_unregister;
> +	}
> +
> +	fuse_data = readl_relaxed(ocotp_base + HW_OCOTP_ANA1);
> +
> +	th_zone = kzalloc(sizeof(struct imx6q_thermal_zone), GFP_KERNEL);

consider using devm_*

> +	if (!th_zone) {
> +		ret = -ENOMEM;
> +		goto err_unregister;
> +	}
> +
> +	th_zone->dev_ops = g_dev_ops;
> +
> +	th_zone->thermal_data =
> +		kzalloc(sizeof(struct imx6q_thermal_data), GFP_KERNEL);

Why do you allocate this seperately? You could embed a struct
imx6q_thermal_data in struct imx6q_thermal_zone.

> +
> +	if (!th_zone->thermal_data) {
> +		ret = -ENOMEM;
> +		goto err_unregister;
> +	}
> +
> +	memcpy(th_zone->thermal_data, &g_thermal_data,
> +				sizeof(struct imx6q_thermal_data));
> +
> +	for (i = 0; i < ARRAY_SIZE(g_thermal_data.freq_tab); i++)
> +		th_zone->thermal_data->freq_tab[i].mask_val = cpumask_of(0);
> +
> +
> +	th_zone->sensor_data =
> +		kzalloc(sizeof(struct imx6q_sensor_data), GFP_KERNEL);

This one also could be embedded in struct imx6q_thermal_zone.

> +
> +	if (!th_zone->sensor_data) {
> +		ret = -ENOMEM;
> +		goto err_unregister;
> +	}
> +
> +	memcpy(th_zone->sensor_data, &g_sensor_data,
> +				sizeof(struct imx6q_sensor_data));
> +
> +	th_zone->sensor_data->anatopmfd = anatopmfd;
> +
> +	ret = imx6q_thermal_process_fuse_data(fuse_data, th_zone->sensor_data);
> +
> +	if (ret) {
> +		pr_err("Invalid temperature calibration data.\n");

use dev_* functions for logging throughout the driver.

> +		goto err_unregister;
> +	}
> +
> +	if (!th_zone->sensor_data->meas_delay)
> +		th_zone->sensor_data->meas_delay = 1;
> +
> +	/* Make sure sensor is in known good state for measurements */
> +	anatop_write_reg(anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
> +		BM_ANADIG_TEMPSENSE0_POWER_DOWN);
> +
> +	anatop_write_reg(anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
> +		BM_ANADIG_TEMPSENSE0_MEASURE_TEMP);
> +
> +	anatop_write_reg(anatopmfd, HW_ANADIG_TEMPSENSE1, 0,
> +		BM_ANADIG_TEMPSENSE1_MEASURE_FREQ);
> +
> +	anatop_write_reg(anatopmfd, HW_ANADIG_ANA_MISC0_SET,
> +		BM_ANADIG_ANA_MISC0_REFTOP_SELBIASOFF,
> +		BM_ANADIG_ANA_MISC0_REFTOP_SELBIASOFF);
> +
> +	anatop_write_reg(anatopmfd, HW_ANADIG_TEMPSENSE0,
> +		BM_ANADIG_TEMPSENSE0_POWER_DOWN,
> +		BM_ANADIG_TEMPSENSE0_POWER_DOWN);
> +
> +	th_zone->cool_dev = cpufreq_cooling_register(
> +		(struct freq_clip_table *)th_zone->thermal_data->freq_tab,
> +		IMX6Q_THERMAL_ACTIVE_TRP_PTS);
> +
> +	if (IS_ERR(th_zone->cool_dev)) {
> +		pr_err("Failed to register cpufreq cooling device\n");
> +		ret = -EINVAL;
> +		goto err_unregister;
> +	}
> +
> +	th_zone->therm_dev = thermal_zone_device_register(
> +		th_zone->sensor_data->name, IMX6Q_THERMAL_TOTAL_TRP_PTS,
> +		th_zone, &th_zone->dev_ops, 0, 0, 0,
> +		IMX6Q_THERMAL_POLLING_FREQUENCY_MS);
> +
> +	if (IS_ERR(th_zone->therm_dev)) {
> +		pr_err("Failed to register thermal zone device\n");
> +		ret = -EINVAL;
> +		goto err_unregister;
> +	}
> +
> +	pdev->dev.platform_data = th_zone->sensor_data;

platform_data is for passing information from the one who registers the
platform_device to the driver. What you have to use here is
platform_set_drvdata(). Also you have to initialize this before
registering the thermal zone, not afterwards.

> +
> +	return 0;
> +
> +err_unregister:
> +	anatop_thermal_remove(pdev);
> +	return ret;
> +}
> +
> +static struct of_device_id __devinitdata of_anatop_thermal_match_tbl[] = {
> +	{ .compatible = "fsl,anatop-thermal", },
> +	{ /* end */ }
> +};
> +
> +static struct platform_driver anatop_thermal = {
> +	.driver = {
> +		.name	= "anatop_thermal",
> +		.owner  = THIS_MODULE,
> +		.of_match_table = of_anatop_thermal_match_tbl,
> +	},
> +	.probe	= anatop_thermal_probe,
> +	.remove	= anatop_thermal_remove,
> +	.suspend = anatop_thermal_suspend,
> +	/* due to implentation of imx6q_get_temp , resume is unnecessary */
> +};
> +
> +static int __devinit anatop_thermal_init(void)
> +{
> +	return platform_driver_register(&anatop_thermal);
> +}
> +device_initcall(anatop_thermal_init);
> +
> +static void __exit anatop_thermal_exit(void)
> +{
> +	platform_driver_unregister(&anatop_thermal);
> +}
> +module_exit(anatop_thermal_exit);

use module_platform_driver

> +
> +MODULE_AUTHOR("Freescale Semiconductor");
> +MODULE_DESCRIPTION("i.MX6Q SoC thermal driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:imx6q-thermal");
> -- 
> 1.7.10
> 
>

Sascha, thanks for the review.

On Wed, Jun 20, 2012 at 5:11 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, Jun 20, 2012 at 02:06:04AM -0500, Robert Lee wrote:
>> Add imx6q cpu thermal management driver using the new cpu cooling
>> interface which limits system performance via cpufreq to reduce
>> the cpu temperature.  Temperature readings are taken using
>> the imx6q temperature sensor and this functionality was added
>> as part of this cpu thermal management driver.
>>
>> Signed-off-by: Robert Lee <rob.lee@linaro.org>
>> ---
>>  arch/arm/boot/dts/imx6q.dtsi    |    5 +
>>  drivers/thermal/Kconfig         |   28 ++
>>  drivers/thermal/Makefile        |    1 +
>>  drivers/thermal/imx6q_thermal.c |  593 +++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 627 insertions(+)
>>  create mode 100644 drivers/thermal/imx6q_thermal.c
>>
>> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
>> index 8c90cba..2650f65 100644
>> --- a/arch/arm/boot/dts/imx6q.dtsi
>> +++ b/arch/arm/boot/dts/imx6q.dtsi
>> @@ -442,6 +442,10 @@
>>                                       anatop-min-voltage = <725000>;
>>                                       anatop-max-voltage = <1450000>;
>>                               };
>> +
>> +                             thermal {
>> +                                     compatible ="fsl,anatop-thermal";
>> +                             };
>>                       };
>>
>>                       usbphy@020c9000 { /* USBPHY1 */
>> @@ -659,6 +663,7 @@
>>                       };
>>
>>                       ocotp@021bc000 {
>> +                             compatible = "fsl,imx6q-ocotp";
>>                               reg = <0x021bc000 0x4000>;
>>                       };
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index 04c6796..b80c408 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -30,6 +30,34 @@ config CPU_THERMAL
>>         and not the ACPI interface.
>>         If you want this support, you should say Y or M here.
>>
>> +config IMX6Q_THERMAL
>> +     bool "IMX6Q Thermal interface support"
>> +     depends on MFD_ANATOP && CPU_THERMAL
>> +     help
>> +       Adds thermal management for IMX6Q.
>> +
>> +config IMX6Q_THERMAL_FAKE_CALIBRATION
>> +     bool "IMX6Q fake temperature sensor calibration (FOR TESTING ONLY)"
>> +     depends on IMX6Q_THERMAL
>> +     help
>> +       This enables a fake temp sensor calibration value for parts without
>> +       the correction calibration values burned into OCOTP. If the part
>> +       already has the calibrated values burned into OCOTP, enabling this
>> +       does nothing.
>> +       WARNING: Use of this feature is for testing only as it will cause
>> +       incorrect temperature readings which will result in incorrect system
>> +       thermal limiting behavior such as premature system performance
>> +       limiting or lack of proper performance reduction to reduce cpu
>> +       temperature
>> +
>> +config IMX6Q_THERMAL_FAKE_CAL_VAL
>> +     hex "IMX6Q fake temperature sensor calibration value"
>> +     depends on IMX6Q_THERMAL_FAKE_CALIBRATION
>> +     default 0x5704c67d
>> +     help
>> +       Refer to the temperature sensor section of the imx6q reference manual
>> +       for more inforation on how this value is used.
>
> Don't add such stuff to Kconfig. If during runtime you detect that there
> is no calibration data, then issue a warning and fall back to a safe
> value. If you really think this should be configurable, add a sysfs
> entry for it. "FOR TESTING ONLY" seems to imply though that it shouldn't
> be configurable.
>

Ok, I'll remove this in v6.

>
>> +
>>  config SPEAR_THERMAL
>>       bool "SPEAr thermal sensor driver"
>>       depends on THERMAL
>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>> index 4636e35..fc4004e 100644
>> --- a/drivers/thermal/Makefile
>> +++ b/drivers/thermal/Makefile
>> @@ -6,3 +6,4 @@ obj-$(CONFIG_THERMAL)         += thermal_sys.o
>>  obj-$(CONFIG_CPU_THERMAL)       += cpu_cooling.o
>>  obj-$(CONFIG_SPEAR_THERMAL)          += spear_thermal.o
>>  obj-$(CONFIG_EXYNOS_THERMAL)         += exynos_thermal.o
>> +obj-$(CONFIG_IMX6Q_THERMAL)  += imx6q_thermal.o
>> diff --git a/drivers/thermal/imx6q_thermal.c b/drivers/thermal/imx6q_thermal.c
>> new file mode 100644
>> index 0000000..255d646
>> --- /dev/null
>> +++ b/drivers/thermal/imx6q_thermal.c
>> @@ -0,0 +1,593 @@
>> +/*
>> + * Copyright 2012 Freescale Semiconductor, Inc.
>> + * Copyright 2012 Linaro Ltd.
>> + *
>> + * The code contained herein is licensed under the GNU General Public
>> + * License. You may obtain a copy of the GNU General Public License
>> + * Version 2 or later at the following locations:
>> + *
>> + * http://www.opensource.org/licenses/gpl-license.html
>> + * http://www.gnu.org/copyleft/gpl.html
>> + */
>> +
>> +/* i.MX6Q Thermal Implementation */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/dmi.h>
>> +#include <linux/init.h>
>> +#include <linux/slab.h>
>> +#include <linux/delay.h>
>> +#include <linux/types.h>
>> +#include <linux/thermal.h>
>> +#include <linux/io.h>
>> +#include <linux/syscalls.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/smp.h>
>> +#include <linux/cpu_cooling.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/mfd/anatop.h>
>> +
>> +/* register define of anatop */
>> +#define HW_ANADIG_ANA_MISC0                  0x00000150
>> +#define HW_ANADIG_ANA_MISC0_SET                      0x00000154
>> +#define HW_ANADIG_ANA_MISC0_CLR                      0x00000158
>> +#define HW_ANADIG_ANA_MISC0_TOG                      0x0000015c
>> +#define BM_ANADIG_ANA_MISC0_REFTOP_SELBIASOFF        0x00000008
>> +
>> +#define HW_ANADIG_TEMPSENSE0                 0x00000180
>> +#define HW_ANADIG_TEMPSENSE0_SET             0x00000184
>> +#define HW_ANADIG_TEMPSENSE0_CLR             0x00000188
>> +#define HW_ANADIG_TEMPSENSE0_TOG             0x0000018c
>> +
>> +#define BP_ANADIG_TEMPSENSE0_TEMP_VALUE              8
>> +#define BM_ANADIG_TEMPSENSE0_TEMP_VALUE              0x000FFF00
>> +#define BM_ANADIG_TEMPSENSE0_FINISHED                0x00000004
>> +#define BM_ANADIG_TEMPSENSE0_MEASURE_TEMP    0x00000002
>> +#define BM_ANADIG_TEMPSENSE0_POWER_DOWN              0x00000001
>> +
>> +#define HW_ANADIG_TEMPSENSE1                 0x00000190
>> +#define HW_ANADIG_TEMPSENSE1_SET             0x00000194
>> +#define HW_ANADIG_TEMPSENSE1_CLR             0x00000198
>> +#define BP_ANADIG_TEMPSENSE1_MEASURE_FREQ    0
>> +#define BM_ANADIG_TEMPSENSE1_MEASURE_FREQ    0x0000FFFF
>> +
>> +#define HW_OCOTP_ANA1                                0x000004E0
>> +
>> +#define IMX6Q_THERMAL_POLLING_FREQUENCY_MS 1000
>> +
>> +#define IMX6Q_THERMAL_ACTIVE_TRP_PTS 3
>> +/* assumption: always one critical trip point */
>> +#define IMX6Q_THERMAL_TOTAL_TRP_PTS (IMX6Q_THERMAL_ACTIVE_TRP_PTS + 1)
>> +
>> +struct imx6q_sensor_data {
>> +     int     c1, c2;
>> +     char    name[16];
>> +     u8      meas_delay;     /* in milliseconds */
>> +     u32     noise_margin;   /* in millicelcius */
>> +     int     last_temp;      /* in millicelcius */
>
> s/celcius/celsius/
>

Oops, will fix this in v6.

>> +     /*
>> +      * When noise filtering, if consecutive measurements are each higher
>> +      * up to consec_high_limit times, assume changing temperature readings
>> +      * to be valid and not noise.
>> +      */
>> +     u32     consec_high_limit;
>> +     struct anatop *anatopmfd;
>> +};
>> +
>> +struct imx6q_thermal_data {
>> +     enum thermal_trip_type type[IMX6Q_THERMAL_TOTAL_TRP_PTS];
>> +     struct freq_clip_table freq_tab[IMX6Q_THERMAL_ACTIVE_TRP_PTS];
>> +     unsigned int crit_temp_level;
>> +};
>> +
>> +struct imx6q_thermal_zone {
>> +     struct thermal_zone_device      *therm_dev;
>> +     struct thermal_cooling_device   *cool_dev;
>> +     struct imx6q_sensor_data        *sensor_data;
>> +     struct imx6q_thermal_data       *thermal_data;
>> +     struct thermal_zone_device_ops  dev_ops;
>> +};
>> +
>> +static struct imx6q_sensor_data g_sensor_data __devinitdata = {
>> +     .name                   = "imx6q-temp_sens",
>> +     .meas_delay             = 1,    /* in milliseconds */
>
> This is initialized here, once again in probe and never changed to any
> other value.
>
>> +     .noise_margin           = 3000,
>> +     .consec_high_limit      = 3,
>> +};
>
> Also constant values. What's the purpose of collecting these in this
> struct?
>

There's not a good purpose for doing it this way now.  It came from a
very early implementation that ended up changing before submitting.
I'll fix this in v6.

>> +
>> +/*
>> + * This data defines the various temperature trip points that will trigger
>> + * cooling action when crossed.
>> + */
>> +static struct imx6q_thermal_data g_thermal_data __devinitdata = {
>> +     .type[0] = THERMAL_TRIP_ACTIVE,
>> +     .freq_tab[0] = {
>> +             .freq_clip_max = 800 * 1000,
>> +             .temp_level = 85000,
>> +     },
>> +     .type[1] = THERMAL_TRIP_ACTIVE,
>> +     .freq_tab[1] = {
>> +             .freq_clip_max = 400 * 1000,
>> +             .temp_level = 90000,
>> +     },
>> +     .type[2] = THERMAL_TRIP_ACTIVE,
>> +     .freq_tab[2] = {
>> +             .freq_clip_max = 200 * 1000,
>> +             .temp_level = 95000,
>> +     },
>> +     .type[3] = THERMAL_TRIP_CRITICAL,
>> +     .crit_temp_level = 100000,
>> +};
>> +
>> +static int th_sys_get_temp(struct thermal_zone_device *thermal,
>> +                               unsigned long *temp);
>> +
>> +static struct imx6q_thermal_zone     *th_zone;
>> +static void __iomem                  *ocotp_base;
>
> This is a driver and drivers should generally be multi instance safe.
>

I don't understand what this comment is referring to.  Could you elaborate?

>> +
>> +static inline int imx6q_get_temp(int *temp, struct imx6q_sensor_data *sd)
>> +{
>> +     unsigned int n_meas;
>> +     unsigned int reg;
>> +
>> +     do {
>> +             /*
>> +              * 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.
>> +              */
>> +             anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
>> +                     BM_ANADIG_TEMPSENSE0_POWER_DOWN);
>> +
>> +             anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0,
>> +                     BM_ANADIG_TEMPSENSE0_MEASURE_TEMP,
>> +                     BM_ANADIG_TEMPSENSE0_MEASURE_TEMP);
>> +
>> +             /*
>> +              * According to imx6q 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 we check the
>> +              * 'finished' status bit to be sure of completion of a valid
>> +              * measurement.
>> +              */
>> +             msleep(sd->meas_delay);
>
> The comment seems to have nothing to do with calling msleep, and it's
> not clear to me why you have to put the argument to msleep into a
> struct.
>

Agree, it should move below to where we actually check the status bit.
 There isn't a good reason why the argument must be in the sensor data
struct in the current implementation.  I will fix these issues in v6.

>> +
>> +             reg = anatop_read_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0);
>> +
>> +             anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
>> +                     BM_ANADIG_TEMPSENSE0_MEASURE_TEMP);
>> +
>> +             anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0,
>> +                     BM_ANADIG_TEMPSENSE0_POWER_DOWN,
>> +                     BM_ANADIG_TEMPSENSE0_POWER_DOWN);
>> +
>> +     } while (!(reg & BM_ANADIG_TEMPSENSE0_FINISHED) &&
>> +              (reg & BM_ANADIG_TEMPSENSE0_TEMP_VALUE));
>
> You should never solely depend on hardware to break out of loops.
>

Ok, I will re-work the implementation to not require this.

>> +
>> +     n_meas = (reg & BM_ANADIG_TEMPSENSE0_TEMP_VALUE)
>> +                     >> BP_ANADIG_TEMPSENSE0_TEMP_VALUE;
>> +
>> +     /* See imx6q_thermal_process_fuse_data() for forumla derivation. */
>> +     *temp = sd->c2 + (sd->c1 * n_meas);
>> +
>> +     pr_debug("Temperature: %d\n", *temp / 1000);
>> +
>> +     return 0;
>> +}
>> +
>> +static int th_sys_get_temp(struct thermal_zone_device *thermal,
>> +                               unsigned long *temp)
>> +{
>> +     int i, total = 0, tmp = 0;
>> +     const u8 loop = 5;
>> +     u32 consec_high = 0;
>> +
>> +     struct imx6q_sensor_data *sd;
>> +
>> +     sd = ((struct imx6q_thermal_zone *)thermal->devdata)->sensor_data;
>> +
>> +     /*
>> +      * Measure temperature and handle noise
>> +      *
>> +      * While the imx6q temperature sensor is designed to minimize being
>> +      * affected by system noise, it's safest to run sanity checks and
>> +      * perform any necessary filitering.
>
> s/filitering/filtering/
>

Oops, will fix in v6.

>> +      */
>> +     for (i = 0; (sd->noise_margin) && (i < loop); i++) {
>> +             imx6q_get_temp(&tmp, sd);
>> +
>> +             if ((abs(tmp - sd->last_temp) <= sd->noise_margin) ||
>> +                     (consec_high >= sd->consec_high_limit)) {
>> +                     sd->last_temp = tmp;
>> +                     i = 0;
>> +                     break;
>> +             }
>> +             if (tmp > sd->last_temp)
>> +                     consec_high++;
>> +
>> +             /*
>> +              * ignore first measurement as the previous measurement was
>> +              * a long time ago.
>> +              */
>> +             if (i)
>> +                     total += tmp;
>> +
>> +             sd->last_temp = tmp;
>> +     }
>> +
>> +     if (sd->noise_margin && i)
>> +             tmp = total / (loop - 1);
>> +
>> +     /*
>> +      * The thermal framework code stores temperature in unsigned long. Also,
>> +      * it has references to "millicelcius" which limits the lowest
>
> s/celcius/celsius/
>

Oops, will fix in v6.

>> +      * temperature possible (compared to Kelvin).
>> +      */
>> +     if (tmp > 0)
>> +             *temp = tmp;
>> +     else
>> +             *temp = 0;
>> +
>> +     return 0;
>> +}
>> +
>> +static int th_sys_get_mode(struct thermal_zone_device *thermal,
>> +                         enum thermal_device_mode *mode)
>> +{
>> +     *mode = THERMAL_DEVICE_ENABLED;
>> +     return 0;
>> +}
>> +
>> +static int th_sys_get_trip_type(struct thermal_zone_device *thermal, int trip,
>> +                              enum thermal_trip_type *type)
>> +{
>> +     struct imx6q_thermal_data *p;
>> +
>> +     if (trip >= thermal->trips)
>> +             return -EINVAL;
>> +
>> +     p = ((struct imx6q_thermal_zone *)thermal->devdata)->thermal_data;
>> +
>> +     *type = p->type[trip];
>> +
>> +     return 0;
>> +}
>> +
>> +static int th_sys_get_trip_temp(struct thermal_zone_device *thermal, int trip,
>> +                              unsigned long *temp)
>> +{
>> +     struct imx6q_thermal_data *p;
>> +     enum thermal_trip_type type;
>> +
>> +     if (trip >= thermal->trips)
>> +             return -EINVAL;
>> +
>> +     p = ((struct imx6q_thermal_zone *)thermal->devdata)->thermal_data;
>> +
>> +     thermal->ops->get_trip_type(thermal, trip, &type);
>> +
>> +     if (type == THERMAL_TRIP_CRITICAL)
>> +             *temp = p->crit_temp_level;
>> +     else
>> +             *temp = p->freq_tab[trip].temp_level;
>> +     return 0;
>> +}
>> +
>> +static int th_sys_get_crit_temp(struct thermal_zone_device *thermal,
>> +                              unsigned long *temp)
>> +{
>> +     struct imx6q_thermal_data *p;
>> +
>> +     p = ((struct imx6q_thermal_zone *)thermal->devdata)->thermal_data;
>> +     *temp = p->crit_temp_level;
>> +     return 0;
>> +}
>> +
>> +static int th_sys_bind(struct thermal_zone_device *thermal,
>> +                     struct thermal_cooling_device *cdev)
>> +{
>> +     int i;
>> +     struct thermal_cooling_device *cd;
>> +
>> +     cd = ((struct imx6q_thermal_zone *)thermal->devdata)->cool_dev;
>> +
>> +     /* if the cooling device is the one from imx6 bind it */
>> +     if (cdev != cd)
>> +             return 0;
>> +
>> +     for (i = 0; i < IMX6Q_THERMAL_ACTIVE_TRP_PTS; i++) {
>> +             if (thermal_zone_bind_cooling_device(thermal, i, cdev)) {
>> +                     pr_err("error binding cooling dev\n");
>> +                     return -EINVAL;
>> +             }
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int th_sys_unbind(struct thermal_zone_device *thermal,
>> +                       struct thermal_cooling_device *cdev)
>> +{
>> +     int i;
>> +
>> +     struct thermal_cooling_device *cd;
>> +
>> +     cd = ((struct imx6q_thermal_zone *)thermal->devdata)->cool_dev;
>> +
>> +     if (cdev != cd)
>> +             return 0;
>> +
>> +     for (i = 0; i < IMX6Q_THERMAL_ACTIVE_TRP_PTS; i++) {
>> +             if (thermal_zone_unbind_cooling_device(thermal, i, cdev)) {
>> +                     pr_err("error unbinding cooling dev\n");
>> +                     return -EINVAL;
>> +             }
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static struct thermal_zone_device_ops g_dev_ops __devinitdata = {
>> +     .bind = th_sys_bind,
>> +     .unbind = th_sys_unbind,
>> +     .get_temp = th_sys_get_temp,
>> +     .get_mode = th_sys_get_mode,
>> +     .get_trip_type = th_sys_get_trip_type,
>> +     .get_trip_temp = th_sys_get_trip_temp,
>> +     .get_crit_temp = th_sys_get_crit_temp,
>> +};
>> +
>> +static int __devinit imx6q_thermal_process_fuse_data(unsigned int fuse_data,
>> +              struct imx6q_sensor_data *sd)
>> +{
>> +     int t1, t2, n1, n2;
>> +
>> +     if (fuse_data == 0 || fuse_data == 0xffffffff) {
>> +             pr_warn("WARNING: Incorrect temperature sensor value of %x "
>> +                     "detected\n", fuse_data);
>> +
>> +#ifdef CONFIG_IMX6Q_THERMAL_FAKE_CALIBRATION
>> +             pr_warn(
>> +                     "WARNING: Using fake calibration value of %x value. "
>> +                     "This will cause your system performance to be limited "
>> +                     "prematurely (compared to a using properly calibrated "
>> +                     "value) OR will cause the system to allow the "
>> +                     "temperature to exceed the maximum specified "
>> +                     "temperature without the proper performance "
>> +                     "limiting.\n", CONFIG_IMX6Q_THERMAL_FAKE_CAL_VAL);
>> +
>> +             fuse_data = CONFIG_IMX6Q_THERMAL_FAKE_CAL_VAL;
>> +#else
>> +             pr_warn("WARNING: Disabling CPU thermal protection\n");
>> +             return -EINVAL;
>> +#endif
>> +     }
>> +
>> +     /*
>> +      * Fuse data layout:
>> +      * [31:20] sensor value @ 25C
>> +      * [19:8] sensor value of hot
>> +      * [7:0] hot temperature value
>> +      */
>> +     n1 = fuse_data >> 20;
>> +     n2 = (fuse_data & 0xfff00) >> 8;
>> +     t2 = fuse_data & 0xff;
>> +     t1 = 25; /* t1 always 25C */
>> +
>> +     pr_debug(" -- temperature sensor calibration data --\n");
>> +     pr_debug("HW_OCOTP_ANA1: %x\n", fuse_data);
>> +     pr_debug("n1: %d\nn2: %d\nt1: %d\nt2: %d\n", n1, n2, t1, t2);
>> +
>> +     /*
>> +      * 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 millicelcius
>> +      * 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)
>> +      */
>> +     sd->c1 = (1000 * (t1 - t2)) / (n1 - n2);
>> +     sd->c2 = (1000 * t2) - (sd->c1 * n2);
>> +
>> +     pr_debug("c1: %i\n", sd->c1);
>> +     pr_debug("c2: %i\n", sd->c2);
>> +
>> +     return 0;
>> +}
>> +
>> +static int anatop_thermal_suspend(struct platform_device *pdev,
>> +                                     pm_message_t state)
>> +{
>> +     struct imx6q_sensor_data *sd = pdev->dev.platform_data;
>> +
>> +     /* power off the sensor during suspend */
>> +     anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
>> +             BM_ANADIG_TEMPSENSE0_MEASURE_TEMP);
>> +
>> +     anatop_write_reg(sd->anatopmfd, HW_ANADIG_TEMPSENSE0,
>> +             BM_ANADIG_TEMPSENSE0_POWER_DOWN,
>> +             BM_ANADIG_TEMPSENSE0_POWER_DOWN);
>> +
>> +     return 0;
>> +}
>> +
>> +static int __devexit anatop_thermal_remove(struct platform_device *pdev)
>> +{
>> +     if (th_zone && th_zone->therm_dev)
>> +             thermal_zone_device_unregister(th_zone->therm_dev);
>> +
>> +     if (th_zone && th_zone->cool_dev)
>> +             cpufreq_cooling_unregister(th_zone->cool_dev);
>> +
>> +     kfree(th_zone->sensor_data);
>> +     kfree(th_zone->thermal_data);
>> +     kfree(th_zone);
>> +
>> +     if (ocotp_base)
>> +             iounmap(ocotp_base);
>> +
>> +     pr_info("i.MX6Q: Kernel Thermal management unregistered\n");
>> +
>> +     return 0;
>> +}
>> +
>> +static int __devinit anatop_thermal_probe(struct platform_device *pdev)
>> +{
>> +     struct device_node *np_ocotp, *np_thermal;
>> +     unsigned int fuse_data;
>> +     struct anatop *anatopmfd = dev_get_drvdata(pdev->dev.parent);
>> +     int ret, i;
>> +
>> +     np_ocotp = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ocotp");
>> +     np_thermal = of_find_compatible_node(NULL, NULL, "fsl,anatop-thermal");
>
> np_thermal = pdev->devI don't quite understand this..of_node;
>

I see, thanks.  In this case I was just using the np_thermal as a
check to be sure we are on the right platform, but it seems that the
platform driver has already done the check for "fsl,anatop-thermal",
so I can remove this line completely and remove the np_thermal check
below.

>> +
>> +     if (!(np_ocotp && np_thermal && anatopmfd))
>> +             return -ENXIO; /* not a compatible platform */
>> +
>> +     ocotp_base = of_iomap(np_ocotp, 0);
>> +
>> +     if (!ocotp_base) {
>> +             pr_err("Could not retrieve ocotp-base\n");
>> +             ret = -ENXIO;
>> +             goto err_unregister;
>> +     }
>> +
>> +     fuse_data = readl_relaxed(ocotp_base + HW_OCOTP_ANA1);
>> +
>> +     th_zone = kzalloc(sizeof(struct imx6q_thermal_zone), GFP_KERNEL);
>
> consider using devm_*
>

Ok, will look at converting to devm_kzalloc and removing the
respective kfree calls in v6.

>> +     if (!th_zone) {
>> +             ret = -ENOMEM;
>> +             goto err_unregister;
>> +     }
>> +
>> +     th_zone->dev_ops = g_dev_ops;
>> +
>> +     th_zone->thermal_data =
>> +             kzalloc(sizeof(struct imx6q_thermal_data), GFP_KERNEL);
>
> Why do you allocate this seperately? You could embed a struct
> imx6q_thermal_data in struct imx6q_thermal_zone.
>

In one implementation I did during development this made more sense,
but now I should embed it as you say.  I'll change this in v6.

>> +
>> +     if (!th_zone->thermal_data) {
>> +             ret = -ENOMEM;
>> +             goto err_unregister;
>> +     }
>> +
>> +     memcpy(th_zone->thermal_data, &g_thermal_data,
>> +                             sizeof(struct imx6q_thermal_data));
>> +
>> +     for (i = 0; i < ARRAY_SIZE(g_thermal_data.freq_tab); i++)
>> +             th_zone->thermal_data->freq_tab[i].mask_val = cpumask_of(0);
>> +
>> +
>> +     th_zone->sensor_data =
>> +             kzalloc(sizeof(struct imx6q_sensor_data), GFP_KERNEL);
>
> This one also could be embedded in struct imx6q_thermal_zone.
>

Same. Will change in v6.

>> +
>> +     if (!th_zone->sensor_data) {
>> +             ret = -ENOMEM;
>> +             goto err_unregister;
>> +     }
>> +
>> +     memcpy(th_zone->sensor_data, &g_sensor_data,
>> +                             sizeof(struct imx6q_sensor_data));
>> +
>> +     th_zone->sensor_data->anatopmfd = anatopmfd;
>> +
>> +     ret = imx6q_thermal_process_fuse_data(fuse_data, th_zone->sensor_data);
>> +
>> +     if (ret) {
>> +             pr_err("Invalid temperature calibration data.\n");
>
> use dev_* functions for logging throughout the driver.
>

Will change in v6.

>> +             goto err_unregister;
>> +     }
>> +
>> +     if (!th_zone->sensor_data->meas_delay)
>> +             th_zone->sensor_data->meas_delay = 1;
>> +
>> +     /* Make sure sensor is in known good state for measurements */
>> +     anatop_write_reg(anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
>> +             BM_ANADIG_TEMPSENSE0_POWER_DOWN);
>> +
>> +     anatop_write_reg(anatopmfd, HW_ANADIG_TEMPSENSE0, 0,
>> +             BM_ANADIG_TEMPSENSE0_MEASURE_TEMP);
>> +
>> +     anatop_write_reg(anatopmfd, HW_ANADIG_TEMPSENSE1, 0,
>> +             BM_ANADIG_TEMPSENSE1_MEASURE_FREQ);
>> +
>> +     anatop_write_reg(anatopmfd, HW_ANADIG_ANA_MISC0_SET,
>> +             BM_ANADIG_ANA_MISC0_REFTOP_SELBIASOFF,
>> +             BM_ANADIG_ANA_MISC0_REFTOP_SELBIASOFF);
>> +
>> +     anatop_write_reg(anatopmfd, HW_ANADIG_TEMPSENSE0,
>> +             BM_ANADIG_TEMPSENSE0_POWER_DOWN,
>> +             BM_ANADIG_TEMPSENSE0_POWER_DOWN);
>> +
>> +     th_zone->cool_dev = cpufreq_cooling_register(
>> +             (struct freq_clip_table *)th_zone->thermal_data->freq_tab,
>> +             IMX6Q_THERMAL_ACTIVE_TRP_PTS);
>> +
>> +     if (IS_ERR(th_zone->cool_dev)) {
>> +             pr_err("Failed to register cpufreq cooling device\n");
>> +             ret = -EINVAL;
>> +             goto err_unregister;
>> +     }
>> +
>> +     th_zone->therm_dev = thermal_zone_device_register(
>> +             th_zone->sensor_data->name, IMX6Q_THERMAL_TOTAL_TRP_PTS,
>> +             th_zone, &th_zone->dev_ops, 0, 0, 0,
>> +             IMX6Q_THERMAL_POLLING_FREQUENCY_MS);
>> +
>> +     if (IS_ERR(th_zone->therm_dev)) {
>> +             pr_err("Failed to register thermal zone device\n");
>> +             ret = -EINVAL;
>> +             goto err_unregister;
>> +     }
>> +
>> +     pdev->dev.platform_data = th_zone->sensor_data;
>
> platform_data is for passing information from the one who registers the
> platform_device to the driver. What you have to use here is
> platform_set_drvdata(). Also you have to initialize this before
> registering the thermal zone, not afterwards.
>

Will fix this in v6.

>> +
>> +     return 0;
>> +
>> +err_unregister:
>> +     anatop_thermal_remove(pdev);
>> +     return ret;
>> +}
>> +
>> +static struct of_device_id __devinitdata of_anatop_thermal_match_tbl[] = {
>> +     { .compatible = "fsl,anatop-thermal", },
>> +     { /* end */ }
>> +};
>> +
>> +static struct platform_driver anatop_thermal = {
>> +     .driver = {
>> +             .name   = "anatop_thermal",
>> +             .owner  = THIS_MODULE,
>> +             .of_match_table = of_anatop_thermal_match_tbl,
>> +     },
>> +     .probe  = anatop_thermal_probe,
>> +     .remove = anatop_thermal_remove,
>> +     .suspend = anatop_thermal_suspend,
>> +     /* due to implentation of imx6q_get_temp , resume is unnecessary */
>> +};
>> +
>> +static int __devinit anatop_thermal_init(void)
>> +{
>> +     return platform_driver_register(&anatop_thermal);
>> +}
>> +device_initcall(anatop_thermal_init);
>> +
>> +static void __exit anatop_thermal_exit(void)
>> +{
>> +     platform_driver_unregister(&anatop_thermal);
>> +}
>> +module_exit(anatop_thermal_exit);
>
> use module_platform_driver
>

Will fix this in v6.

Thanks,
Rob

>> +
>> +MODULE_AUTHOR("Freescale Semiconductor");
>> +MODULE_DESCRIPTION("i.MX6Q SoC thermal driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:imx6q-thermal");
>> --
>> 1.7.10
>>
>>
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On Wed, Jun 20, 2012 at 09:12:51AM -0500, Rob Lee wrote:
> Sascha, thanks for the review.
> 
> >> +
> >> +static struct imx6q_thermal_zone     *th_zone;
> >> +static void __iomem                  *ocotp_base;
> >
> > This is a driver and drivers should generally be multi instance safe.
> >
> 
> I don't understand what this comment is referring to.  Could you elaborate?

Drivers can only be multi instance safe when all variables are inside a
instance specific struct and you pass a pointer to this struct around.
What if the i.MX7 has two different ocotp_base and you want to use this
driver on both ocotp?

Sascha

On Wed, Jun 20, 2012 at 9:20 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, Jun 20, 2012 at 09:12:51AM -0500, Rob Lee wrote:
>> Sascha, thanks for the review.
>>
>> >> +
>> >> +static struct imx6q_thermal_zone     *th_zone;
>> >> +static void __iomem                  *ocotp_base;
>> >
>> > This is a driver and drivers should generally be multi instance safe.
>> >
>>
>> I don't understand what this comment is referring to.  Could you elaborate?
>
> Drivers can only be multi instance safe when all variables are inside a
> instance specific struct and you pass a pointer to this struct around.
> What if the i.MX7 has two different ocotp_base and you want to use this
> driver on both ocotp?
>

Understood, thanks.  I'll fix this in v6.

> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |