[PATCH V2 1/4] thermal: qoriq: Use devm_platform_ioremap_resource() instead of of_iomap()

[PATCH V2 1/4] thermal: qoriq: Use devm_platform_ioremap_resource() instead of of_iomap()

[Anson.Huang]

From: Anson Huang <Anson.Huang@nxp.com>

Use devm_platform_ioremap_resource() instead of of_iomap() to
save the iounmap() call in error handle path;

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No changes.
---
 drivers/thermal/qoriq_thermal.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/thermal/qoriq_thermal.c b/drivers/thermal/qoriq_thermal.c
index 7b36493..c7c7de2 100644
--- a/drivers/thermal/qoriq_thermal.c
+++ b/drivers/thermal/qoriq_thermal.c
@@ -202,32 +202,27 @@ static int qoriq_tmu_probe(struct platform_device *pdev)
 
 	data->little_endian = of_property_read_bool(np, "little-endian");
 
-	data->regs = of_iomap(np, 0);
-	if (!data->regs) {
+	data->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(data->regs)) {
 		dev_err(&pdev->dev, "Failed to get memory region\n");
-		ret = -ENODEV;
-		goto err_iomap;
+		return PTR_ERR(data->regs);
 	}
 
 	qoriq_tmu_init_device(data);	/* TMU initialization */
 
 	ret = qoriq_tmu_calibration(pdev);	/* TMU calibration */
 	if (ret < 0)
-		goto err_tmu;
+		goto err;
 
 	ret = qoriq_tmu_register_tmu_zone(pdev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to register sensors\n");
-		ret = -ENODEV;
-		goto err_iomap;
+		goto err;
 	}
 
 	return 0;
 
-err_tmu:
-	iounmap(data->regs);
-
-err_iomap:
+err:
 	platform_set_drvdata(pdev, NULL);
 
 	return ret;
@@ -240,7 +235,6 @@ static int qoriq_tmu_remove(struct platform_device *pdev)
 	/* Disable monitoring */
 	tmu_write(data, TMR_DISABLE, &data->regs->tmr);
 
-	iounmap(data->regs);
 	platform_set_drvdata(pdev, NULL);
 
 	return 0;
-- 
2.7.4

[PATCH V2 2/4] thermal: qoriq: Use __maybe_unused instead of #if CONFIG_PM_SLEEP

[Anson.Huang]

From: Anson Huang <Anson.Huang@nxp.com>

Use __maybe_unused for power management related functions
instead of #if CONFIG_PM_SLEEP to simply the code.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No changes.
---
 drivers/thermal/qoriq_thermal.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/qoriq_thermal.c b/drivers/thermal/qoriq_thermal.c
index c7c7de2..2b2f79b 100644
--- a/drivers/thermal/qoriq_thermal.c
+++ b/drivers/thermal/qoriq_thermal.c
@@ -240,8 +240,7 @@ static int qoriq_tmu_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int qoriq_tmu_suspend(struct device *dev)
+static int __maybe_unused qoriq_tmu_suspend(struct device *dev)
 {
 	u32 tmr;
 	struct qoriq_tmu_data *data = dev_get_drvdata(dev);
@@ -254,7 +253,7 @@ static int qoriq_tmu_suspend(struct device *dev)
 	return 0;
 }
 
-static int qoriq_tmu_resume(struct device *dev)
+static int __maybe_unused qoriq_tmu_resume(struct device *dev)
 {
 	u32 tmr;
 	struct qoriq_tmu_data *data = dev_get_drvdata(dev);
@@ -266,7 +265,6 @@ static int qoriq_tmu_resume(struct device *dev)
 
 	return 0;
 }
-#endif
 
 static SIMPLE_DEV_PM_OPS(qoriq_tmu_pm_ops,
 			 qoriq_tmu_suspend, qoriq_tmu_resume);
-- 
2.7.4

[PATCH V2 3/4] dt-bindings: thermal: qoriq: Add optional clocks property

[Anson.Huang]

From: Anson Huang <Anson.Huang@nxp.com>

Some platforms have clock control for TMU, add optional
clocks property to the binding doc.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No changes.
---
 Documentation/devicetree/bindings/thermal/qoriq-thermal.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/thermal/qoriq-thermal.txt b/Documentation/devicetree/bindings/thermal/qoriq-thermal.txt
index 04cbb90..28f2cba 100644
--- a/Documentation/devicetree/bindings/thermal/qoriq-thermal.txt
+++ b/Documentation/devicetree/bindings/thermal/qoriq-thermal.txt
@@ -23,6 +23,7 @@ Required properties:
 Optional property:
 - little-endian : If present, the TMU registers are little endian. If absent,
 	the default is big endian.
+- clocks : the clock for clocking the TMU silicon.
 
 Example:
 
-- 
2.7.4

[PATCH V2 4/4] thermal: qoriq: Add clock operations

[Anson.Huang]

From: Anson Huang <Anson.Huang@nxp.com>

Some platforms like i.MX8MQ has clock control for this module,
need to add clock operations to make sure the driver is working
properly.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
Reviewed-by: Guido Günther <agx@sigxcpu.org>
---
Changes since V1:
	- use devm_clk_get_optional() instead of devm_clk_get().
---
 drivers/thermal/qoriq_thermal.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/thermal/qoriq_thermal.c b/drivers/thermal/qoriq_thermal.c
index 2b2f79b..0ae45c0 100644
--- a/drivers/thermal/qoriq_thermal.c
+++ b/drivers/thermal/qoriq_thermal.c
@@ -2,6 +2,7 @@
 //
 // Copyright 2016 Freescale Semiconductor, Inc.
 
+#include <linux/clk.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/err.h>
@@ -72,6 +73,7 @@ struct qoriq_sensor {
 
 struct qoriq_tmu_data {
 	struct qoriq_tmu_regs __iomem *regs;
+	struct clk *clk;
 	bool little_endian;
 	struct qoriq_sensor	*sensor[SITES_MAX];
 };
@@ -208,6 +210,16 @@ static int qoriq_tmu_probe(struct platform_device *pdev)
 		return PTR_ERR(data->regs);
 	}
 
+	data->clk = devm_clk_get_optional(&pdev->dev, NULL);
+	if (IS_ERR(data->clk))
+		return PTR_ERR(data->clk);
+
+	ret = clk_prepare_enable(data->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to enable clock\n");
+		return ret;
+	}
+
 	qoriq_tmu_init_device(data);	/* TMU initialization */
 
 	ret = qoriq_tmu_calibration(pdev);	/* TMU calibration */
@@ -235,6 +247,8 @@ static int qoriq_tmu_remove(struct platform_device *pdev)
 	/* Disable monitoring */
 	tmu_write(data, TMR_DISABLE, &data->regs->tmr);
 
+	clk_disable_unprepare(data->clk);
+
 	platform_set_drvdata(pdev, NULL);
 
 	return 0;
@@ -250,14 +264,21 @@ static int __maybe_unused qoriq_tmu_suspend(struct device *dev)
 	tmr &= ~TMR_ME;
 	tmu_write(data, tmr, &data->regs->tmr);
 
+	clk_disable_unprepare(data->clk);
+
 	return 0;
 }
 
 static int __maybe_unused qoriq_tmu_resume(struct device *dev)
 {
 	u32 tmr;
+	int ret;
 	struct qoriq_tmu_data *data = dev_get_drvdata(dev);
 
+	ret = clk_prepare_enable(data->clk);
+	if (ret)
+		return ret;
+
 	/* Enable monitoring */
 	tmr = tmu_read(data, &data->regs->tmr);
 	tmr |= TMR_ME;
-- 
2.7.4

Re: [PATCH V2 3/4] dt-bindings: thermal: qoriq: Add optional clocks property

[Fabio Estevam]

Hi Anson,

On Mon, Jul 29, 2019 at 6:04 AM <Anson.Huang@nxp.com> wrote:
>
> From: Anson Huang <Anson.Huang@nxp.com>
>
> Some platforms have clock control for TMU, add optional
> clocks property to the binding doc.

Please add a note that this is needed for i.MX8M.

Re: [PATCH V2 3/4] dt-bindings: thermal: qoriq: Add optional clocks property

[Fabio Estevam]

Hi Anson,

On Mon, Jul 29, 2019 at 6:04 AM <Anson.Huang@nxp.com> wrote:
>
> From: Anson Huang <Anson.Huang@nxp.com>
>
> Some platforms have clock control for TMU, add optional
> clocks property to the binding doc.

Please add a note that this is needed for i.MX8M.

Re: [PATCH V2 4/4] thermal: qoriq: Add clock operations

[Fabio Estevam]

Hi Anson,

On Mon, Jul 29, 2019 at 6:04 AM <Anson.Huang@nxp.com> wrote:
>
> From: Anson Huang <Anson.Huang@nxp.com>
>
> Some platforms like i.MX8MQ has clock control for this module,
> need to add clock operations to make sure the driver is working
> properly.

I haven't seen this series earlier, and I have sent a similar patch
for Guido to test.

Since this patch solves a hang problem, I would suggest that this one
becomes the first of the series.

Also, the "clk: imx8mq: Remove CLK_IS_CRITICAL flag for
IMX8MQ_CLK_TMU_ROOT" should only be applied after this one in order to
avoid the hang.

>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> Reviewed-by: Guido Günther <agx@sigxcpu.org>
> ---
> Changes since V1:
>         - use devm_clk_get_optional() instead of devm_clk_get().
> ---
>  drivers/thermal/qoriq_thermal.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/thermal/qoriq_thermal.c b/drivers/thermal/qoriq_thermal.c
> index 2b2f79b..0ae45c0 100644
> --- a/drivers/thermal/qoriq_thermal.c
> +++ b/drivers/thermal/qoriq_thermal.c
> @@ -2,6 +2,7 @@
>  //
>  // Copyright 2016 Freescale Semiconductor, Inc.
>
> +#include <linux/clk.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/err.h>
> @@ -72,6 +73,7 @@ struct qoriq_sensor {
>
>  struct qoriq_tmu_data {
>         struct qoriq_tmu_regs __iomem *regs;
> +       struct clk *clk;
>         bool little_endian;
>         struct qoriq_sensor     *sensor[SITES_MAX];
>  };
> @@ -208,6 +210,16 @@ static int qoriq_tmu_probe(struct platform_device *pdev)
>                 return PTR_ERR(data->regs);
>         }
>
> +       data->clk = devm_clk_get_optional(&pdev->dev, NULL);
> +       if (IS_ERR(data->clk))
> +               return PTR_ERR(data->clk);
> +
> +       ret = clk_prepare_enable(data->clk);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to enable clock\n");
> +               return ret;
> +       }
> +
>         qoriq_tmu_init_device(data);    /* TMU initialization */
>
>         ret = qoriq_tmu_calibration(pdev);      /* TMU calibration */

In case of failure the TMU clock should be disabled in the error path.

Thanks

Re: [PATCH V2 4/4] thermal: qoriq: Add clock operations

[Fabio Estevam]

Hi Anson,

On Mon, Jul 29, 2019 at 6:04 AM <Anson.Huang@nxp.com> wrote:
>
> From: Anson Huang <Anson.Huang@nxp.com>
>
> Some platforms like i.MX8MQ has clock control for this module,
> need to add clock operations to make sure the driver is working
> properly.

I haven't seen this series earlier, and I have sent a similar patch
for Guido to test.

Since this patch solves a hang problem, I would suggest that this one
becomes the first of the series.

Also, the "clk: imx8mq: Remove CLK_IS_CRITICAL flag for
IMX8MQ_CLK_TMU_ROOT" should only be applied after this one in order to
avoid the hang.

>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> Reviewed-by: Guido Günther <agx@sigxcpu.org>
> ---
> Changes since V1:
>         - use devm_clk_get_optional() instead of devm_clk_get().
> ---
>  drivers/thermal/qoriq_thermal.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/thermal/qoriq_thermal.c b/drivers/thermal/qoriq_thermal.c
> index 2b2f79b..0ae45c0 100644
> --- a/drivers/thermal/qoriq_thermal.c
> +++ b/drivers/thermal/qoriq_thermal.c
> @@ -2,6 +2,7 @@
>  //
>  // Copyright 2016 Freescale Semiconductor, Inc.
>
> +#include <linux/clk.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/err.h>
> @@ -72,6 +73,7 @@ struct qoriq_sensor {
>
>  struct qoriq_tmu_data {
>         struct qoriq_tmu_regs __iomem *regs;
> +       struct clk *clk;
>         bool little_endian;
>         struct qoriq_sensor     *sensor[SITES_MAX];
>  };
> @@ -208,6 +210,16 @@ static int qoriq_tmu_probe(struct platform_device *pdev)
>                 return PTR_ERR(data->regs);
>         }
>
> +       data->clk = devm_clk_get_optional(&pdev->dev, NULL);
> +       if (IS_ERR(data->clk))
> +               return PTR_ERR(data->clk);
> +
> +       ret = clk_prepare_enable(data->clk);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to enable clock\n");
> +               return ret;
> +       }
> +
>         qoriq_tmu_init_device(data);    /* TMU initialization */
>
>         ret = qoriq_tmu_calibration(pdev);      /* TMU calibration */

In case of failure the TMU clock should be disabled in the error path.

Thanks

Re: [PATCH V2 3/4] dt-bindings: thermal: qoriq: Add optional clocks property

[Daniel Baluta]

On Mon, 2019-07-29 at 16:39 +0800, Anson.Huang@nxp.com wrote:
> From: Anson Huang <Anson.Huang@nxp.com>
> 
> Some platforms have clock control for TMU, add optional
> clocks property to the binding doc.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

Please also pick Rob's Reviewed-by from last revision.

> ---
> No changes.
> ---
>  Documentation/devicetree/bindings/thermal/qoriq-thermal.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/qoriq-
> thermal.txt b/Documentation/devicetree/bindings/thermal/qoriq-
> thermal.txt
> index 04cbb90..28f2cba 100644
> --- a/Documentation/devicetree/bindings/thermal/qoriq-thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/qoriq-thermal.txt
> @@ -23,6 +23,7 @@ Required properties:
>  Optional property:
>  - little-endian : If present, the TMU registers are little endian.
> If absent,
>  	the default is big endian.
> +- clocks : the clock for clocking the TMU silicon.
>  
>  Example:
>  

RE: [PATCH V2 4/4] thermal: qoriq: Add clock operations

[Anson Huang]

Hi, Fabio

> On Mon, Jul 29, 2019 at 6:04 AM <Anson.Huang@nxp.com> wrote:
> >
> > From: Anson Huang <Anson.Huang@nxp.com>
> >
> > Some platforms like i.MX8MQ has clock control for this module, need to
> > add clock operations to make sure the driver is working properly.
> 
> I haven't seen this series earlier, and I have sent a similar patch for Guido to
> test.
> 
> Since this patch solves a hang problem, I would suggest that this one
> becomes the first of the series.

Done in V3.

> 
> Also, the "clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> IMX8MQ_CLK_TMU_ROOT" should only be applied after this one in order to
> avoid the hang.

Shawn already applied the patch, and Abel has the AHB clock patch to fix that,
so just wait for the AHB clock patch in instead of revert the TMU clock patch? 

> 
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > Reviewed-by: Guido Günther <agx@sigxcpu.org>
> > ---
> > Changes since V1:
> >         - use devm_clk_get_optional() instead of devm_clk_get().
> > ---
> >  drivers/thermal/qoriq_thermal.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/drivers/thermal/qoriq_thermal.c
> > b/drivers/thermal/qoriq_thermal.c index 2b2f79b..0ae45c0 100644
> > --- a/drivers/thermal/qoriq_thermal.c
> > +++ b/drivers/thermal/qoriq_thermal.c
> > @@ -2,6 +2,7 @@
> >  //
> >  // Copyright 2016 Freescale Semiconductor, Inc.
> >
> > +#include <linux/clk.h>
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/err.h>
> > @@ -72,6 +73,7 @@ struct qoriq_sensor {
> >
> >  struct qoriq_tmu_data {
> >         struct qoriq_tmu_regs __iomem *regs;
> > +       struct clk *clk;
> >         bool little_endian;
> >         struct qoriq_sensor     *sensor[SITES_MAX];
> >  };
> > @@ -208,6 +210,16 @@ static int qoriq_tmu_probe(struct platform_device
> *pdev)
> >                 return PTR_ERR(data->regs);
> >         }
> >
> > +       data->clk = devm_clk_get_optional(&pdev->dev, NULL);
> > +       if (IS_ERR(data->clk))
> > +               return PTR_ERR(data->clk);
> > +
> > +       ret = clk_prepare_enable(data->clk);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "Failed to enable clock\n");
> > +               return ret;
> > +       }
> > +
> >         qoriq_tmu_init_device(data);    /* TMU initialization */
> >
> >         ret = qoriq_tmu_calibration(pdev);      /* TMU calibration */
> 
> In case of failure the TMU clock should be disabled in the error path.

Done in V3.

Thanks,
Anson.

Re: [PATCH V2 4/4] thermal: qoriq: Add clock operations

[Fabio Estevam]

Hi Anson,

On Tue, Jul 30, 2019 at 12:00 AM Anson Huang <anson.huang@nxp.com> wrote:

> Shawn already applied the patch, and Abel has the AHB clock patch to fix that,
> so just wait for the AHB clock patch in instead of revert the TMU clock patch?

Sorry, I don't understand Abel's patch as there is not a proper
description of what it tries to fix.

If I understand correctly Abel's patch is not the proper fix and the
real fix for the kernel TMU hang in linux-next is to manage the TMU
clock inside the TMU driver, like you did in this patch.

To avoid a revert one possible solution would be to send only this one
as a fix for 5.3. You can point that it Fixes the commit that adds
i.MX8M support for the TMU driver.

Re: [PATCH V2 4/4] thermal: qoriq: Add clock operations

[Fabio Estevam]

Hi Anson,

On Tue, Jul 30, 2019 at 12:00 AM Anson Huang <anson.huang@nxp.com> wrote:

> Shawn already applied the patch, and Abel has the AHB clock patch to fix that,
> so just wait for the AHB clock patch in instead of revert the TMU clock patch?

Sorry, I don't understand Abel's patch as there is not a proper
description of what it tries to fix.

If I understand correctly Abel's patch is not the proper fix and the
real fix for the kernel TMU hang in linux-next is to manage the TMU
clock inside the TMU driver, like you did in this patch.

To avoid a revert one possible solution would be to send only this one
as a fix for 5.3. You can point that it Fixes the commit that adds
i.MX8M support for the TMU driver.

RE: [PATCH V2 4/4] thermal: qoriq: Add clock operations

[Anson Huang]

Hi, Fabio

> On Tue, Jul 30, 2019 at 12:00 AM Anson Huang <anson.huang@nxp.com>
> wrote:
> 
> > Shawn already applied the patch, and Abel has the AHB clock patch to
> > fix that, so just wait for the AHB clock patch in instead of revert the TMU
> clock patch?
> 
> Sorry, I don't understand Abel's patch as there is not a proper description of
> what it tries to fix.
> 
> If I understand correctly Abel's patch is not the proper fix and the real fix for
> the kernel TMU hang in linux-next is to manage the TMU clock inside the
> TMU driver, like you did in this patch.
> 
> To avoid a revert one possible solution would be to send only this one as a
> fix for 5.3. You can point that it Fixes the commit that adds i.MX8M support
> for the TMU driver.

I replied in the patch https://patchwork.kernel.org/patch/11032781/ , I pasted it as below,
The TMU clock is NOT the root cause, but it just accidently trigger this issue:

I can explain why Abel's patch can fix this issue, the AHB clock is a MUST always ON for system bus, while it does NOT have CLK_IS_CRITICAL flag set for now, when SDMA1 is probed, it will enable its clock, and the clk path is sdma1_clk->ipg_root->ahb, after SDMA1 probed done, it will disable its clock since no one use it, and it will trigger the ahb clock to be OFF, as its usecount is added by 1 when probe and decreased by 1 after
SDMA1 probe done, so usecount=0 will trigger AHB clock to be OFF.

So I think the best solution should be applying Abel's patch as you mentioned upper, the TMU clock patch is NOT the root cause, it just triggers this issue accidently☹

But I saw Abel's AHB patch is still under discussion, so I think we need to speed it up and make kernel boot up work for development. AHB/IPG are always critical for i.MX SoCs.

Thanks,
Anson.