* [PATCH v3] imx: thermal: imx_get_temp might be called before sensor clock is prepared
@ 2014-10-13 20:51 Heiner Kallweit
2014-11-07 18:18 ` Eduardo Valentin
0 siblings, 1 reply; 4+ messages in thread
From: Heiner Kallweit @ 2014-10-13 20:51 UTC (permalink / raw)
To: linux-pm; +Cc: Zhang Rui <rui.zhang@intel.com>; Eduardo Valentin
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 <heiner.kallweit@web.de>
---
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(-)
diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index 461bf3d..0e8ef55 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -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);
- }
-
/* Enable measurements at ~ 10 Hz */
regmap_write(map, TEMPSENSE1 + REG_CLR, TEMPSENSE1_MEASURE_FREQ);
measure_freq = DIV_ROUND_UP(32768, 10); /* 10 Hz */
--
2.1.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] imx: thermal: imx_get_temp might be called before sensor clock is prepared
2014-10-13 20:51 [PATCH v3] imx: thermal: imx_get_temp might be called before sensor clock is prepared Heiner Kallweit
@ 2014-11-07 18:18 ` Eduardo Valentin
2014-11-07 22:43 ` Heiner Kallweit
0 siblings, 1 reply; 4+ messages in thread
From: Eduardo Valentin @ 2014-11-07 18:18 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-pm
[-- Attachment #1: Type: text/plain, Size: 4375 bytes --]
Hello Heiner,
On Mon, Oct 13, 2014 at 10:51:23PM +0200, Heiner Kallweit wrote:
> 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 <heiner.kallweit@web.de>
> ---
> 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(-)
>
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 461bf3d..0e8ef55 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -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);
> - }
> -
> /* 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
> --
> 2.1.2
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] imx: thermal: imx_get_temp might be called before sensor clock is prepared
2014-11-07 18:18 ` Eduardo Valentin
@ 2014-11-07 22:43 ` Heiner Kallweit
2014-11-07 23:47 ` Eduardo Valentin
0 siblings, 1 reply; 4+ messages in thread
From: Heiner Kallweit @ 2014-11-07 22:43 UTC (permalink / raw)
To: Eduardo Valentin; +Cc: linux-pm
Am 07.11.2014 um 19:18 schrieb Eduardo Valentin:
>
> Hello Heiner,
>
> On Mon, Oct 13, 2014 at 10:51:23PM +0200, Heiner Kallweit wrote:
>> 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 <heiner.kallweit@web.de>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
>> index 461bf3d..0e8ef55 100644
>> --- a/drivers/thermal/imx_thermal.c
>> +++ b/drivers/thermal/imx_thermal.c
>> @@ -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);
>> - }
>> -
>> /* 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.
>
> Cheers,
>
> Eduardo Valentin
>> --
>> 2.1.2
Rgds, Heiner
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] imx: thermal: imx_get_temp might be called before sensor clock is prepared
2014-11-07 22:43 ` Heiner Kallweit
@ 2014-11-07 23:47 ` Eduardo Valentin
0 siblings, 0 replies; 4+ messages in thread
From: Eduardo Valentin @ 2014-11-07 23:47 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-pm
[-- Attachment #1: Type: text/plain, Size: 5408 bytes --]
Heiner,
On Fri, Nov 07, 2014 at 11:43:15PM +0100, Heiner Kallweit wrote:
> Am 07.11.2014 um 19:18 schrieb Eduardo Valentin:
> >
> > Hello Heiner,
> >
> > On Mon, Oct 13, 2014 at 10:51:23PM +0200, Heiner Kallweit wrote:
> >> 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 <heiner.kallweit@web.de>
> >> ---
> >> 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(-)
> >>
> >> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> >> index 461bf3d..0e8ef55 100644
> >> --- a/drivers/thermal/imx_thermal.c
> >> +++ b/drivers/thermal/imx_thermal.c
> >> @@ -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);
> >> - }
> >> -
> >> /* 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.
OK. I see.
In this case, can you please re-post this patch on top of my -fixes
patches? I am planing to send it in the -rc cycles.
I've just merged another fix on imx driver and looks like your
patch needs refreshing now.
Thanks,
Eduardo Valentin
> >
> > Cheers,
> >
> > Eduardo Valentin
> >> --
> >> 2.1.2
> Rgds, Heiner
>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-11-07 23:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-13 20:51 [PATCH v3] imx: thermal: imx_get_temp might be called before sensor clock is prepared Heiner Kallweit
2014-11-07 18:18 ` Eduardo Valentin
2014-11-07 22:43 ` Heiner Kallweit
2014-11-07 23:47 ` Eduardo Valentin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).