[PATCH 2/3] thermal: imx: add necessary clk operation

[PATCH 2/3] thermal: imx: add necessary clk operation

[Baruch Siach]

Hi Anson,

On Thu, Dec 19, 2013 at 01:17:24PM -0500, Anson Huang wrote:
> +	if (IS_ERR(data->thermal_clk)) {
> +		ret = IS_ERR(data->thermal_clk);

You probably what to return PTR_ERR here.

baruch

> +		dev_err(&pdev->dev, "failed to get thermal clk!\n");
> +		return ret;
> +	}

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 2/3] thermal: imx: add necessary clk operation

[Anson.Huang at freescale.com]

Best Regards.
Anson huang ???
?
Freescale Semiconductor Shanghai
?????????192?A?2?
201203
Tel:021-28937058


>-----Original Message-----
>From: Baruch Siach [mailto:baruch at tkos.co.il]
>Sent: Thursday, December 19, 2013 2:25 PM
>To: Huang Yongcai-B20788
>Cc: shawn.guo at linaro.org; kernel at pengutronix.de; rui.zhang at intel.com;
>eduardo.valentin at ti.com; devicetree at vger.kernel.org; linux-pm at vger.kernel.org;
>linux-arm-kernel at lists.infradead.org; linux-doc at vger.kernel.org
>Subject: Re: [PATCH 2/3] thermal: imx: add necessary clk operation
>
>Hi Anson,
>
>On Thu, Dec 19, 2013 at 01:17:24PM -0500, Anson Huang wrote:
>> +	if (IS_ERR(data->thermal_clk)) {
>> +		ret = IS_ERR(data->thermal_clk);
>
>You probably what to return PTR_ERR here.
>

Yes, thanks, will fix it in V2.
[Anson] 


>baruch
>
>> +		dev_err(&pdev->dev, "failed to get thermal clk!\n");
>> +		return ret;
>> +	}
>
>--
>     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
>=}------------------------------------------------ooO--U--Ooo------------{=
>   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
>

[PATCH 3/3] thermal: imx: add clk info for thermal sensor

[Shawn Guo]

On Thu, Dec 19, 2013 at 01:17:25PM -0500, Anson Huang wrote:
> thermal sensor needs dedicated clock to work in correct way,
> so we need to add necessary clock info in dts.
> 
> Signed-off-by: Anson Huang <b20788@freescale.com>
> ---
>  .../devicetree/bindings/thermal/imx-thermal.txt    |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/imx-thermal.txt b/Documentation/devicetree/bindings/thermal/imx-thermal.txt
> index 541c25e..e518c6e 100644
> --- a/Documentation/devicetree/bindings/thermal/imx-thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/imx-thermal.txt
> @@ -7,6 +7,7 @@ Required properties:
>  - fsl,tempmon-data : phandle pointer to fuse controller that contains TEMPMON
>    calibration data, e.g. OCOTP on imx6q.  The details about calibration data
>    can be found in SoC Reference Manual.
> +- clocks : thermal sensor's clock source.

For such binding update, it can just be merged into driver patch.  Also,
it has to be an optional property.  See my reply to the driver patch.

Shawn

>  
>  Example:
>  
> @@ -14,4 +15,5 @@ tempmon {
>  	compatible = "fsl,imx6q-tempmon";
>  	fsl,tempmon = <&anatop>;
>  	fsl,tempmon-data = <&ocotp>;
> +	clocks = <&clks 172>;
>  };
> -- 
> 1.7.9.5
> 
> 

[PATCH 2/3] thermal: imx: add necessary clk operation

[Shawn Guo]

On Thu, Dec 19, 2013 at 01:17:24PM -0500, Anson Huang wrote:
> @@ -427,6 +429,13 @@ 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 = IS_ERR(data->thermal_clk);
> +		dev_err(&pdev->dev, "failed to get thermal clk!\n");
> +		return ret;
> +	}
> +

So when the new kernel runs on a board with an old DTB installed,
thermal driver will be broken.

Shawn

[PATCH 1/3] ARM: dts: imx6qdl: add necessary thermal clk

[Shawn Guo]

On Thu, Dec 19, 2013 at 01:17:23PM -0500, Anson Huang wrote:
> Thermal sensor needs pll3_usb_otg when measuring temperature,
> so we need to pass clk info to thermal driver.
> 
> Signed-off-by: Anson Huang <b20788@freescale.com>

Applied this one, thanks.

[PATCH 2/3] thermal: imx: add necessary clk operation

[Anson.Huang at freescale.com]

Best Regards.
Anson huang ???
?
Freescale Semiconductor Shanghai
?????????192?A?2?
201203
Tel:021-28937058


>-----Original Message-----
>From: Shawn Guo [mailto:shawn.guo at linaro.org]
>Sent: Thursday, December 19, 2013 2:59 PM
>To: Huang Yongcai-B20788
>Cc: kernel at pengutronix.de; rui.zhang at intel.com; eduardo.valentin at ti.com;
>devicetree at vger.kernel.org; linux-doc at vger.kernel.org; linux-arm-
>kernel at lists.infradead.org; linux-pm at vger.kernel.org
>Subject: Re: [PATCH 2/3] thermal: imx: add necessary clk operation
>
>On Thu, Dec 19, 2013 at 01:17:24PM -0500, Anson Huang wrote:
>> @@ -427,6 +429,13 @@ 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 = IS_ERR(data->thermal_clk);
>> +		dev_err(&pdev->dev, "failed to get thermal clk!\n");
>> +		return ret;
>> +	}
>> +
>
>So when the new kernel runs on a board with an old DTB installed, thermal
>driver will be broken.
>
Yes, I thought about this case, but the previous implement is incorrect, if
the PLL3 is not enabled by other drivers, thermal driver will not work, so
this patch is a bug fix, not enhancement. So we still need to consider old
dts case? 

Anson

>Shawn

[PATCH 2/3] thermal: imx: add necessary clk operation

[Shawn Guo]

On Thu, Dec 19, 2013 at 07:08:16AM +0000, Anson.Huang at freescale.com wrote:
> >On Thu, Dec 19, 2013 at 01:17:24PM -0500, Anson Huang wrote:
> >> @@ -427,6 +429,13 @@ 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 = IS_ERR(data->thermal_clk);
> >> +		dev_err(&pdev->dev, "failed to get thermal clk!\n");
> >> +		return ret;
> >> +	}
> >> +
> >
> >So when the new kernel runs on a board with an old DTB installed, thermal
> >driver will be broken.
> >
> Yes, I thought about this case, but the previous implement is incorrect, if
> the PLL3 is not enabled by other drivers, thermal driver will not work, so
> this patch is a bug fix, not enhancement. So we still need to consider old
> dts case? 

The thing is mainline kernel runs on many board with thermal driver
being functional today.  That said, PLL3 is already enabled on these
platforms when thermal driver is running.  You cannot fix a bug but
meanwhile break these users who use old DTB.

Shawn

[PATCH 2/3] thermal: imx: add necessary clk operation

[Anson.Huang at freescale.com]

Best Regards.
Anson huang ???
?
Freescale Semiconductor Shanghai
?????????192?A?2?
201203
Tel:021-28937058


>-----Original Message-----
>From: Shawn Guo [mailto:shawn.guo at linaro.org]
>Sent: Thursday, December 19, 2013 3:29 PM
>To: Huang Yongcai-B20788
>Cc: kernel at pengutronix.de; rui.zhang at intel.com; eduardo.valentin at ti.com;
>devicetree at vger.kernel.org; linux-doc at vger.kernel.org; linux-arm-
>kernel at lists.infradead.org; linux-pm at vger.kernel.org
>Subject: Re: [PATCH 2/3] thermal: imx: add necessary clk operation
>
>On Thu, Dec 19, 2013 at 07:08:16AM +0000, Anson.Huang at freescale.com wrote:
>> >On Thu, Dec 19, 2013 at 01:17:24PM -0500, Anson Huang wrote:
>> >> @@ -427,6 +429,13 @@ 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 = IS_ERR(data->thermal_clk);
>> >> +		dev_err(&pdev->dev, "failed to get thermal clk!\n");
>> >> +		return ret;
>> >> +	}
>> >> +
>> >
>> >So when the new kernel runs on a board with an old DTB installed,
>> >thermal driver will be broken.
>> >
>> Yes, I thought about this case, but the previous implement is
>> incorrect, if the PLL3 is not enabled by other drivers, thermal driver
>> will not work, so this patch is a bug fix, not enhancement. So we
>> still need to consider old dts case?
>
>The thing is mainline kernel runs on many board with thermal driver being
>functional today.  That said, PLL3 is already enabled on these platforms when
>thermal driver is running.  You cannot fix a bug but meanwhile break these
>users who use old DTB.

Okay, got it, I will replace the return with one warning message in V2.

Anson 


>
>Shawn

[PATCH 1/3] ARM: dts: imx6qdl: add necessary thermal clk

[Anson Huang]

Thermal sensor needs pll3_usb_otg when measuring temperature,
so we need to pass clk info to thermal driver.

Signed-off-by: Anson Huang <b20788@freescale.com>
---
 arch/arm/boot/dts/imx6qdl.dtsi |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index fb28b2e..72c7e2b 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -550,6 +550,7 @@
 				interrupts = <0 49 0x04>;
 				fsl,tempmon = <&anatop>;
 				fsl,tempmon-data = <&ocotp>;
+				clocks = <&clks 172>;
 			};
 
 			usbphy1: usbphy at 020c9000 {
-- 
1.7.9.5

[PATCH 2/3] thermal: imx: add necessary clk operation

[Anson Huang]

Thermal sensor needs pll3_usb_otg when measuring temperature,
otherwise the temperature read will be incorrect, so need to
enable this clk before sensor working, for alarm function,
as hardware will take measurement periodically, so we should
keep this clk always on once alarm function is enabled.

Signed-off-by: Anson Huang <b20788@freescale.com>
---
 drivers/thermal/imx_thermal.c |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index 1d6c801..7abbc04 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -7,6 +7,7 @@
  *
  */
 
+#include <linux/clk.h>
 #include <linux/cpu_cooling.h>
 #include <linux/cpufreq.h>
 #include <linux/delay.h>
@@ -73,6 +74,7 @@ struct imx_thermal_data {
 	unsigned long last_temp;
 	bool irq_enabled;
 	int irq;
+	struct clk *thermal_clk;
 };
 
 static void imx_set_alarm_temp(struct imx_thermal_data *data,
@@ -427,6 +429,13 @@ 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 = IS_ERR(data->thermal_clk);
+		dev_err(&pdev->dev, "failed to get thermal clk!\n");
+		return ret;
+	}
+
 	/* Make sure sensor is in known good state for measurements */
 	regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
 	regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_MEASURE_TEMP);
@@ -457,6 +466,19 @@ static int imx_thermal_probe(struct platform_device *pdev)
 		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);
+		return ret;
+	}
+
 	/* Enable measurements at ~ 10 Hz */
 	regmap_write(map, TEMPSENSE1 + REG_CLR, TEMPSENSE1_MEASURE_FREQ);
 	measure_freq = DIV_ROUND_UP(32768, 10); /* 10 Hz */
-- 
1.7.9.5

[PATCH 3/3] thermal: imx: add clk info for thermal sensor

[Anson Huang]

thermal sensor needs dedicated clock to work in correct way,
so we need to add necessary clock info in dts.

Signed-off-by: Anson Huang <b20788@freescale.com>
---
 .../devicetree/bindings/thermal/imx-thermal.txt    |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/thermal/imx-thermal.txt b/Documentation/devicetree/bindings/thermal/imx-thermal.txt
index 541c25e..e518c6e 100644
--- a/Documentation/devicetree/bindings/thermal/imx-thermal.txt
+++ b/Documentation/devicetree/bindings/thermal/imx-thermal.txt
@@ -7,6 +7,7 @@ Required properties:
 - fsl,tempmon-data : phandle pointer to fuse controller that contains TEMPMON
   calibration data, e.g. OCOTP on imx6q.  The details about calibration data
   can be found in SoC Reference Manual.
+- clocks : thermal sensor's clock source.
 
 Example:
 
@@ -14,4 +15,5 @@ tempmon {
 	compatible = "fsl,imx6q-tempmon";
 	fsl,tempmon = <&anatop>;
 	fsl,tempmon-data = <&ocotp>;
+	clocks = <&clks 172>;
 };
-- 
1.7.9.5