imx: thermal: imx_get_temp might be called before sensor clock is prepared

imx_get_temp might be called before the sensor clock is prepared thus resulting in a timeout of the first attempt to read temp:
thermal thermal_zone0: failed to read out thermal zone 0 Happened to me on a Utilite Standard with IMX6 Dual SoC.

Reason is that in imx_thermal_probe thermal_zone_device_register is called before the sensor clock is prepared. thermal_zone_device_register however calls thermal_zone_device_update which eventually calls imx_get_temp.

Fix this by preparing the clock before calling
thermal_zone_device_register.

Signed-off-by Heiner Kallweit <[email protected]>

---
v2: revised error path. Bail out and tidy up properly if we can't
get the clock or fail to enable it
v3: don't print error message if getting clock returns EPROBE_DEFER

---
 drivers/thermal/imx_thermal.c | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

drivers/thermal/imx_thermal.c +25 -9

index 461bf3d..0e8ef55 100644
@@ -521,6 +521,30 @@ static int imx_thermal_probe(struct platform_device *pdev)
                return ret;
        }
 
+       data->thermal_clk = devm_clk_get(&pdev->dev, NULL);
+       if (IS_ERR(data->thermal_clk)) {
+               ret = PTR_ERR(data->thermal_clk);
+               if (ret != -EPROBE_DEFER)
+                       dev_err(&pdev->dev,
+                               "failed to get thermal clk: %d\n", ret);
+               cpufreq_cooling_unregister(data->cdev);
+               return ret;
+       }
+
+       /*
+        * Thermal sensor needs clk on to get correct value, normally
+        * we should enable its clk before taking measurement and disable
+        * clk after measurement is done, but if alarm function is enabled,
+        * hardware will auto measure the temperature periodically, so we
+        * need to keep the clk always on for alarm function.
+        */
+       ret = clk_prepare_enable(data->thermal_clk);
+       if (ret) {
+               dev_err(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
+               cpufreq_cooling_unregister(data->cdev);
+               return ret;
+       }
+
        data->tz = thermal_zone_device_register("imx_thermal_zone",
                                                IMX_TRIP_NUM,
                                                BIT(IMX_TRIP_PASSIVE), data,

@@ -531,26 +555,11 @@ static int imx_thermal_probe(struct platform_device *pdev)
                ret = PTR_ERR(data->tz);
                dev_err(&pdev->dev,
                        "failed to register thermal zone device %d\n", ret);
+               clk_disable_unprepare(data->thermal_clk);
                cpufreq_cooling_unregister(data->cdev);
                return ret;
        }
 
-       data->thermal_clk = devm_clk_get(&pdev->dev, NULL);
-       if (IS_ERR(data->thermal_clk)) {
-               dev_warn(&pdev->dev, "failed to get thermal clk!\n");
-       } else {
-               /*
-                * Thermal sensor needs clk on to get correct value, normally
-                * we should enable its clk before taking measurement and disable
-                * clk after measurement is done, but if alarm function is enabled,
-                * hardware will auto measure the temperature periodically, so we

--
2.1.2

Hello Heiner,

Quoted 82 lines · Heiner Kallweit

imx_get_temp might be called before the sensor clock is prepared thus resulting in a timeout of the first attempt to read temp:
thermal thermal_zone0: failed to read out thermal zone 0 Happened to me on a Utilite Standard with IMX6 Dual SoC.

Reason is that in imx_thermal_probe thermal_zone_device_register is called before the sensor clock is prepared. thermal_zone_device_register however calls thermal_zone_device_update which eventually calls imx_get_temp.

Fix this by preparing the clock before calling
thermal_zone_device_register.

Signed-off-by Heiner Kallweit <[email protected]>

---
v2: revised error path. Bail out and tidy up properly if we can't
get the clock or fail to enable it
v3: don't print error message if getting clock returns EPROBE_DEFER

---
 drivers/thermal/imx_thermal.c | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

drivers/thermal/imx_thermal.c +25 -16

index 461bf3d..0e8ef55 100644
@@ -521,6 +521,30 @@ static int imx_thermal_probe(struct platform_device *pdev)
                return ret;
        }
 
+       data->thermal_clk = devm_clk_get(&pdev->dev, NULL);
+       if (IS_ERR(data->thermal_clk)) {
+               ret = PTR_ERR(data->thermal_clk);
+               if (ret != -EPROBE_DEFER)
+                       dev_err(&pdev->dev,
+                               "failed to get thermal clk: %d\n", ret);
+               cpufreq_cooling_unregister(data->cdev);
+               return ret;
+       }
+
+       /*
+        * Thermal sensor needs clk on to get correct value, normally
+        * we should enable its clk before taking measurement and disable
+        * clk after measurement is done, but if alarm function is enabled,
+        * hardware will auto measure the temperature periodically, so we
+        * need to keep the clk always on for alarm function.
+        */
+       ret = clk_prepare_enable(data->thermal_clk);
+       if (ret) {
+               dev_err(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
+               cpufreq_cooling_unregister(data->cdev);
+               return ret;
+       }
+
        data->tz = thermal_zone_device_register("imx_thermal_zone",
                                                IMX_TRIP_NUM,
                                                BIT(IMX_TRIP_PASSIVE), data,

@@ -531,26 +555,11 @@ static int imx_thermal_probe(struct platform_device *pdev)
                ret = PTR_ERR(data->tz);
                dev_err(&pdev->dev,
                        "failed to register thermal zone device %d\n", ret);
+               clk_disable_unprepare(data->thermal_clk);
                cpufreq_cooling_unregister(data->cdev);
                return ret;
        }
 
-       data->thermal_clk = devm_clk_get(&pdev->dev, NULL);
-       if (IS_ERR(data->thermal_clk)) {
-               dev_warn(&pdev->dev, "failed to get thermal clk!\n");
-       } else {
-               /*
-                * Thermal sensor needs clk on to get correct value, normally
-                * we should enable its clk before taking measurement and disable
-                * clk after measurement is done, but if alarm function is enabled,
-                * hardware will auto measure the temperature periodically, so we
-                * need to keep the clk always on for alarm function.
-                */
-               ret = clk_prepare_enable(data->thermal_clk);
-               if (ret)
-                       dev_warn(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
-       }
-

drivers/thermal/imx_thermal.c static int imx_thermal_probe(struct platform_device *pdev) · line 565

Quoted 3 lines

drivers/thermal/imx_thermal.c +0 -0

        /* Enable measurements at ~ 10 Hz */
        regmap_write(map, TEMPSENSE1 + REG_CLR, TEMPSENSE1_MEASURE_FREQ);
        measure_freq = DIV_ROUND_UP(32768, 10); /* 10 Hz */

While here, do you need to move up also the configuration of the measurements at ~ 10 Hz? Or would still the first readings be correct while at the reset value?

Cheers,

Eduardo Valentin

Quoted 2 lines

--
2.1.2

Am 07.11.2014 um 19:18 schrieb Eduardo Valentin:

Quoted 86 lines

Hello Heiner,

Quoted 82 lines · Heiner Kallweit

imx_get_temp might be called before the sensor clock is prepared thus resulting in a timeout of the first attempt to read temp:
thermal thermal_zone0: failed to read out thermal zone 0 Happened to me on a Utilite Standard with IMX6 Dual SoC.

Reason is that in imx_thermal_probe thermal_zone_device_register is called before the sensor clock is prepared. thermal_zone_device_register however calls thermal_zone_device_update which eventually calls imx_get_temp.

Fix this by preparing the clock before calling
thermal_zone_device_register.

Signed-off-by Heiner Kallweit <[email protected]>

---
v2: revised error path. Bail out and tidy up properly if we can't
get the clock or fail to enable it
v3: don't print error message if getting clock returns EPROBE_DEFER

---
 drivers/thermal/imx_thermal.c | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

drivers/thermal/imx_thermal.c +25 -16

index 461bf3d..0e8ef55 100644
@@ -521,6 +521,30 @@ static int imx_thermal_probe(struct platform_device *pdev)
                return ret;
        }
 
+       data->thermal_clk = devm_clk_get(&pdev->dev, NULL);
+       if (IS_ERR(data->thermal_clk)) {
+               ret = PTR_ERR(data->thermal_clk);
+               if (ret != -EPROBE_DEFER)
+                       dev_err(&pdev->dev,
+                               "failed to get thermal clk: %d\n", ret);
+               cpufreq_cooling_unregister(data->cdev);
+               return ret;
+       }
+
+       /*
+        * Thermal sensor needs clk on to get correct value, normally
+        * we should enable its clk before taking measurement and disable
+        * clk after measurement is done, but if alarm function is enabled,
+        * hardware will auto measure the temperature periodically, so we
+        * need to keep the clk always on for alarm function.
+        */
+       ret = clk_prepare_enable(data->thermal_clk);
+       if (ret) {
+               dev_err(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
+               cpufreq_cooling_unregister(data->cdev);
+               return ret;
+       }
+
        data->tz = thermal_zone_device_register("imx_thermal_zone",
                                                IMX_TRIP_NUM,
                                                BIT(IMX_TRIP_PASSIVE), data,

@@ -531,26 +555,11 @@ static int imx_thermal_probe(struct platform_device *pdev)
                ret = PTR_ERR(data->tz);
                dev_err(&pdev->dev,
                        "failed to register thermal zone device %d\n", ret);
+               clk_disable_unprepare(data->thermal_clk);
                cpufreq_cooling_unregister(data->cdev);
                return ret;
        }
 
-       data->thermal_clk = devm_clk_get(&pdev->dev, NULL);
-       if (IS_ERR(data->thermal_clk)) {
-               dev_warn(&pdev->dev, "failed to get thermal clk!\n");
-       } else {
-               /*
-                * Thermal sensor needs clk on to get correct value, normally
-                * we should enable its clk before taking measurement and disable
-                * clk after measurement is done, but if alarm function is enabled,
-                * hardware will auto measure the temperature periodically, so we
-                * need to keep the clk always on for alarm function.
-                */
-               ret = clk_prepare_enable(data->thermal_clk);
-               if (ret)
-                       dev_warn(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
-       }
-

Quoted 8 lines

drivers/thermal/imx_thermal.c static int imx_thermal_probe(struct platform_device *pdev) · line 565

Quoted 3 lines

drivers/thermal/imx_thermal.c +0 -0

        /* Enable measurements at ~ 10 Hz */
        regmap_write(map, TEMPSENSE1 + REG_CLR, TEMPSENSE1_MEASURE_FREQ);
        measure_freq = DIV_ROUND_UP(32768, 10); /* 10 Hz */

While here, do you need to move up also the configuration of the measurements at ~ 10 Hz? Or would still the first readings be correct while at the reset value?

When imx_get_temp is called from thermal_zone_device_register data->mode still is THERMAL_DEVICE_DISABLED. imx_get_temp therefore will power on the sensor and trigger a single measurement.
IMHO this is fully ok and I don't think we have to relocate the setup of the scheduled measurements.

Quoted 6 lines

Cheers,

Eduardo Valentin

Quoted 2 lines

--
2.1.2

Rgds, Heiner