[PATCH 0/2] remoteproc: Address runtime PM issues

[PATCH 0/2] remoteproc: Address runtime PM issues

[Mathieu Poirier]

This set follows the conversation that took place here[1] and provides
the "two small patches" I alluded to at the end of the thread.

Paul Cercueil: patch 1/2 is compile tested only - please see that it does what
you want.
Suman Anna: Please test on your side and confirm that it addresses the Omap
regression.

Applies on top of rproc-next (7dcef3988eed)

Thanks,
Mathieu

[1]. https://lore.kernel.org/linux-remoteproc/20200515104340.10473-1-paul@crapouillou.net/T/#t

Mathieu Poirier (2):
  remoteproc: ingenic: Move clock handling to prepare/unprepare
    callbacks
  Revert "remoteproc: Add support for runtime PM"

 drivers/remoteproc/ingenic_rproc.c   | 84 +++++++++-------------------
 drivers/remoteproc/remoteproc_core.c | 17 +-----
 2 files changed, 27 insertions(+), 74 deletions(-)

-- 
2.25.1

[PATCH 1/2] remoteproc: ingenic: Move clock handling to prepare/unprepare callbacks

[Mathieu Poirier]

This patch moves clock related operations to the remoteproc prepare()
and unprepare() callbacks so that the PM runtime framework doesn't
have to be involved needlessly.  This provides a simpler approach and
requires less code.

Based on the work from Paul Cercueil published here:
https://lore.kernel.org/linux-remoteproc/20191116170846.67220-4-paul@crapouillou.net/

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/ingenic_rproc.c | 84 +++++++++---------------------
 1 file changed, 26 insertions(+), 58 deletions(-)

diff --git a/drivers/remoteproc/ingenic_rproc.c b/drivers/remoteproc/ingenic_rproc.c
index 189020d77b25..b0fc8eace6ec 100644
--- a/drivers/remoteproc/ingenic_rproc.c
+++ b/drivers/remoteproc/ingenic_rproc.c
@@ -11,7 +11,6 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
-#include <linux/pm_runtime.h>
 #include <linux/remoteproc.h>
 
 #include "remoteproc_internal.h"
@@ -62,6 +61,28 @@ struct vpu {
 	struct device *dev;
 };
 
+static int ingenic_rproc_prepare(struct rproc *rproc)
+{
+	struct vpu *vpu = rproc->priv;
+	int ret;
+
+	/* The clocks must be enabled for the firmware to be loaded in TCSM */
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
+	if (ret)
+		dev_err(vpu->dev, "Unable to start clocks: %d", ret);
+
+	return ret;
+}
+
+static int ingenic_rproc_unprepare(struct rproc *rproc)
+{
+	struct vpu *vpu = rproc->priv;
+
+	clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
+
+	return 0;
+}
+
 static int ingenic_rproc_start(struct rproc *rproc)
 {
 	struct vpu *vpu = rproc->priv;
@@ -115,6 +136,8 @@ static void *ingenic_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
 }
 
 static struct rproc_ops ingenic_rproc_ops = {
+	.prepare = ingenic_rproc_prepare,
+	.unprepare = ingenic_rproc_unprepare,
 	.start = ingenic_rproc_start,
 	.stop = ingenic_rproc_stop,
 	.kick = ingenic_rproc_kick,
@@ -135,16 +158,6 @@ static irqreturn_t vpu_interrupt(int irq, void *data)
 	return rproc_vq_interrupt(rproc, vring);
 }
 
-static void ingenic_rproc_disable_clks(void *data)
-{
-	struct vpu *vpu = data;
-
-	pm_runtime_resume(vpu->dev);
-	pm_runtime_disable(vpu->dev);
-
-	clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
-}
-
 static int ingenic_rproc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -206,35 +219,13 @@ static int ingenic_rproc_probe(struct platform_device *pdev)
 
 	disable_irq(vpu->irq);
 
-	/* The clocks must be enabled for the firmware to be loaded in TCSM */
-	ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
-	if (ret) {
-		dev_err(dev, "Unable to start clocks\n");
-		return ret;
-	}
-
-	pm_runtime_irq_safe(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
-	pm_runtime_get_sync(dev);
-	pm_runtime_use_autosuspend(dev);
-
-	ret = devm_add_action_or_reset(dev, ingenic_rproc_disable_clks, vpu);
-	if (ret) {
-		dev_err(dev, "Unable to register action\n");
-		goto out_pm_put;
-	}
-
 	ret = devm_rproc_add(dev, rproc);
 	if (ret) {
 		dev_err(dev, "Failed to register remote processor\n");
-		goto out_pm_put;
+		return ret;
 	}
 
-out_pm_put:
-	pm_runtime_put_autosuspend(dev);
-
-	return ret;
+	return 0;
 }
 
 static const struct of_device_id ingenic_rproc_of_matches[] = {
@@ -243,33 +234,10 @@ static const struct of_device_id ingenic_rproc_of_matches[] = {
 };
 MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
 
-static int __maybe_unused ingenic_rproc_suspend(struct device *dev)
-{
-	struct vpu *vpu = dev_get_drvdata(dev);
-
-	clk_bulk_disable(ARRAY_SIZE(vpu->clks), vpu->clks);
-
-	return 0;
-}
-
-static int __maybe_unused ingenic_rproc_resume(struct device *dev)
-{
-	struct vpu *vpu = dev_get_drvdata(dev);
-
-	return clk_bulk_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
-}
-
-static const struct dev_pm_ops __maybe_unused ingenic_rproc_pm = {
-	SET_RUNTIME_PM_OPS(ingenic_rproc_suspend, ingenic_rproc_resume, NULL)
-};
-
 static struct platform_driver ingenic_rproc_driver = {
 	.probe = ingenic_rproc_probe,
 	.driver = {
 		.name = "ingenic-vpu",
-#ifdef CONFIG_PM
-		.pm = &ingenic_rproc_pm,
-#endif
 		.of_match_table = ingenic_rproc_of_matches,
 	},
 };
-- 
2.25.1

Re: [PATCH 1/2] remoteproc: ingenic: Move clock handling to prepare/unprepare callbacks

[Suman Anna]

Hi Mathieu,

On 6/30/20 11:31 AM, Mathieu Poirier wrote:
> This patch moves clock related operations to the remoteproc prepare()
> and unprepare() callbacks so that the PM runtime framework doesn't
> have to be involved needlessly.  This provides a simpler approach and
> requires less code.
> 
> Based on the work from Paul Cercueil published here:
> https://lore.kernel.org/linux-remoteproc/20191116170846.67220-4-paul@crapouillou.net/
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>   drivers/remoteproc/ingenic_rproc.c | 84 +++++++++---------------------
>   1 file changed, 26 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/remoteproc/ingenic_rproc.c b/drivers/remoteproc/ingenic_rproc.c
> index 189020d77b25..b0fc8eace6ec 100644
> --- a/drivers/remoteproc/ingenic_rproc.c
> +++ b/drivers/remoteproc/ingenic_rproc.c
> @@ -11,7 +11,6 @@
>   #include <linux/io.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
> -#include <linux/pm_runtime.h>
>   #include <linux/remoteproc.h>
>   
>   #include "remoteproc_internal.h"
> @@ -62,6 +61,28 @@ struct vpu {
>   	struct device *dev;
>   };
>   
> +static int ingenic_rproc_prepare(struct rproc *rproc)
> +{
> +	struct vpu *vpu = rproc->priv;
> +	int ret;
> +
> +	/* The clocks must be enabled for the firmware to be loaded in TCSM */
> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
> +	if (ret)
> +		dev_err(vpu->dev, "Unable to start clocks: %d", ret);

Minor nit, add a newline character at the end of the trace statement.

With that,
Reviewed-by: Suman Anna <s-anna@ti.com>

> +
> +	return ret;
> +}
> +
> +static int ingenic_rproc_unprepare(struct rproc *rproc)
> +{
> +	struct vpu *vpu = rproc->priv;
> +
> +	clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
> +
> +	return 0;
> +}
> +
>   static int ingenic_rproc_start(struct rproc *rproc)
>   {
>   	struct vpu *vpu = rproc->priv;
> @@ -115,6 +136,8 @@ static void *ingenic_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
>   }
>   
>   static struct rproc_ops ingenic_rproc_ops = {
> +	.prepare = ingenic_rproc_prepare,
> +	.unprepare = ingenic_rproc_unprepare,
>   	.start = ingenic_rproc_start,
>   	.stop = ingenic_rproc_stop,
>   	.kick = ingenic_rproc_kick,
> @@ -135,16 +158,6 @@ static irqreturn_t vpu_interrupt(int irq, void *data)
>   	return rproc_vq_interrupt(rproc, vring);
>   }
>   
> -static void ingenic_rproc_disable_clks(void *data)
> -{
> -	struct vpu *vpu = data;
> -
> -	pm_runtime_resume(vpu->dev);
> -	pm_runtime_disable(vpu->dev);
> -
> -	clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
> -}
> -
>   static int ingenic_rproc_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -206,35 +219,13 @@ static int ingenic_rproc_probe(struct platform_device *pdev)
>   
>   	disable_irq(vpu->irq);
>   
> -	/* The clocks must be enabled for the firmware to be loaded in TCSM */
> -	ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
> -	if (ret) {
> -		dev_err(dev, "Unable to start clocks\n");
> -		return ret;
> -	}
> -
> -	pm_runtime_irq_safe(dev);
> -	pm_runtime_set_active(dev);
> -	pm_runtime_enable(dev);
> -	pm_runtime_get_sync(dev);
> -	pm_runtime_use_autosuspend(dev);
> -
> -	ret = devm_add_action_or_reset(dev, ingenic_rproc_disable_clks, vpu);
> -	if (ret) {
> -		dev_err(dev, "Unable to register action\n");
> -		goto out_pm_put;
> -	}
> -
>   	ret = devm_rproc_add(dev, rproc);
>   	if (ret) {
>   		dev_err(dev, "Failed to register remote processor\n");
> -		goto out_pm_put;
> +		return ret;
>   	}
>   
> -out_pm_put:
> -	pm_runtime_put_autosuspend(dev);
> -
> -	return ret;
> +	return 0;
>   }
>   
>   static const struct of_device_id ingenic_rproc_of_matches[] = {
> @@ -243,33 +234,10 @@ static const struct of_device_id ingenic_rproc_of_matches[] = {
>   };
>   MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
>   
> -static int __maybe_unused ingenic_rproc_suspend(struct device *dev)
> -{
> -	struct vpu *vpu = dev_get_drvdata(dev);
> -
> -	clk_bulk_disable(ARRAY_SIZE(vpu->clks), vpu->clks);
> -
> -	return 0;
> -}
> -
> -static int __maybe_unused ingenic_rproc_resume(struct device *dev)
> -{
> -	struct vpu *vpu = dev_get_drvdata(dev);
> -
> -	return clk_bulk_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
> -}
> -
> -static const struct dev_pm_ops __maybe_unused ingenic_rproc_pm = {
> -	SET_RUNTIME_PM_OPS(ingenic_rproc_suspend, ingenic_rproc_resume, NULL)
> -};
> -
>   static struct platform_driver ingenic_rproc_driver = {
>   	.probe = ingenic_rproc_probe,
>   	.driver = {
>   		.name = "ingenic-vpu",
> -#ifdef CONFIG_PM
> -		.pm = &ingenic_rproc_pm,
> -#endif
>   		.of_match_table = ingenic_rproc_of_matches,
>   	},
>   };
> 

[PATCH 2/2] Revert "remoteproc: Add support for runtime PM"

[Mathieu Poirier]

This reverts commit a99a37f6cd5a74d5b22c08544aa6c5890813c8ba.

Removing PM runtime operations from the remoteproc core in order to:

1) Keep all power management operations in platform drivers.  That way we
do not loose flexibility in an area that is very HW specific.

2) Avoid making the support for remote processor managed by external
entities more complex that it already is.

3) Fix regression introduced for the Omap remoteproc driver.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 9f04c30c4aaf..0f95e025ba03 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -29,7 +29,6 @@
 #include <linux/devcoredump.h>
 #include <linux/rculist.h>
 #include <linux/remoteproc.h>
-#include <linux/pm_runtime.h>
 #include <linux/iommu.h>
 #include <linux/idr.h>
 #include <linux/elf.h>
@@ -1383,12 +1382,6 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	if (ret)
 		return ret;
 
-	ret = pm_runtime_get_sync(dev);
-	if (ret < 0) {
-		dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
-		return ret;
-	}
-
 	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
 
 	/*
@@ -1398,7 +1391,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	ret = rproc_enable_iommu(rproc);
 	if (ret) {
 		dev_err(dev, "can't enable iommu: %d\n", ret);
-		goto put_pm_runtime;
+		return ret;
 	}
 
 	/* Prepare rproc for firmware loading if needed */
@@ -1452,8 +1445,6 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	rproc_unprepare_device(rproc);
 disable_iommu:
 	rproc_disable_iommu(rproc);
-put_pm_runtime:
-	pm_runtime_put(dev);
 	return ret;
 }
 
@@ -1891,8 +1882,6 @@ void rproc_shutdown(struct rproc *rproc)
 
 	rproc_disable_iommu(rproc);
 
-	pm_runtime_put(dev);
-
 	/* Free the copy of the resource table */
 	kfree(rproc->cached_table);
 	rproc->cached_table = NULL;
@@ -2183,9 +2172,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 
 	rproc->state = RPROC_OFFLINE;
 
-	pm_runtime_no_callbacks(&rproc->dev);
-	pm_runtime_enable(&rproc->dev);
-
 	return rproc;
 
 put_device:
@@ -2205,7 +2191,6 @@ EXPORT_SYMBOL(rproc_alloc);
  */
 void rproc_free(struct rproc *rproc)
 {
-	pm_runtime_disable(&rproc->dev);
 	put_device(&rproc->dev);
 }
 EXPORT_SYMBOL(rproc_free);
-- 
2.25.1

Re: [PATCH 2/2] Revert "remoteproc: Add support for runtime PM"

[Suman Anna]

Hi Mathieu,

On 6/30/20 11:31 AM, Mathieu Poirier wrote:
> This reverts commit a99a37f6cd5a74d5b22c08544aa6c5890813c8ba.
> 
> Removing PM runtime operations from the remoteproc core in order to:
> 
> 1) Keep all power management operations in platform drivers.  That way we
> do not loose flexibility in an area that is very HW specific.
> 
> 2) Avoid making the support for remote processor managed by external
> entities more complex that it already is.
> 
> 3) Fix regression introduced for the Omap remoteproc driver.

Thanks for following up on the discussion, I have verified that the 
autosuspend regression is fixed.

> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Tested-by: Suman Anna <s-anna@ti.com>
Acked-by: Suman Anna <s-anna@ti.com>

regards
Suman

> ---
>   drivers/remoteproc/remoteproc_core.c | 17 +----------------
>   1 file changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 9f04c30c4aaf..0f95e025ba03 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -29,7 +29,6 @@
>   #include <linux/devcoredump.h>
>   #include <linux/rculist.h>
>   #include <linux/remoteproc.h>
> -#include <linux/pm_runtime.h>
>   #include <linux/iommu.h>
>   #include <linux/idr.h>
>   #include <linux/elf.h>
> @@ -1383,12 +1382,6 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>   	if (ret)
>   		return ret;
>   
> -	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0) {
> -		dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
> -		return ret;
> -	}
> -
>   	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
>   
>   	/*
> @@ -1398,7 +1391,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>   	ret = rproc_enable_iommu(rproc);
>   	if (ret) {
>   		dev_err(dev, "can't enable iommu: %d\n", ret);
> -		goto put_pm_runtime;
> +		return ret;
>   	}
>   
>   	/* Prepare rproc for firmware loading if needed */
> @@ -1452,8 +1445,6 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>   	rproc_unprepare_device(rproc);
>   disable_iommu:
>   	rproc_disable_iommu(rproc);
> -put_pm_runtime:
> -	pm_runtime_put(dev);
>   	return ret;
>   }
>   
> @@ -1891,8 +1882,6 @@ void rproc_shutdown(struct rproc *rproc)
>   
>   	rproc_disable_iommu(rproc);
>   
> -	pm_runtime_put(dev);
> -
>   	/* Free the copy of the resource table */
>   	kfree(rproc->cached_table);
>   	rproc->cached_table = NULL;
> @@ -2183,9 +2172,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>   
>   	rproc->state = RPROC_OFFLINE;
>   
> -	pm_runtime_no_callbacks(&rproc->dev);
> -	pm_runtime_enable(&rproc->dev);
> -
>   	return rproc;
>   
>   put_device:
> @@ -2205,7 +2191,6 @@ EXPORT_SYMBOL(rproc_alloc);
>    */
>   void rproc_free(struct rproc *rproc)
>   {
> -	pm_runtime_disable(&rproc->dev);
>   	put_device(&rproc->dev);
>   }
>   EXPORT_SYMBOL(rproc_free);
> 

Re: [PATCH 0/2] remoteproc: Address runtime PM issues

[Suman Anna]

Hi Mathieu,

On 6/30/20 11:31 AM, Mathieu Poirier wrote:
> This set follows the conversation that took place here[1] and provides
> the "two small patches" I alluded to at the end of the thread.
> 
> Paul Cercueil: patch 1/2 is compile tested only - please see that it does what
> you want.
> Suman Anna: Please test on your side and confirm that it addresses the Omap
> regression.

Tested the behavior with 5.8-rc3 + these patches, all looks good. Also 
provided by review tags on the individual patches.

regards
Suman

> 
> Applies on top of rproc-next (7dcef3988eed)
> 
> Thanks,
> Mathieu
> 
> [1]. https://lore.kernel.org/linux-remoteproc/20200515104340.10473-1-paul@crapouillou.net/T/#t
> 
> Mathieu Poirier (2):
>    remoteproc: ingenic: Move clock handling to prepare/unprepare
>      callbacks
>    Revert "remoteproc: Add support for runtime PM"
> 
>   drivers/remoteproc/ingenic_rproc.c   | 84 +++++++++-------------------
>   drivers/remoteproc/remoteproc_core.c | 17 +-----
>   2 files changed, 27 insertions(+), 74 deletions(-)
> 

Re: [PATCH 0/2] remoteproc: Address runtime PM issues

[Paul Cercueil]

Hi,

Le mar. 30 juin 2020 à 10:31, Mathieu Poirier 
<mathieu.poirier@linaro.org> a écrit :
> This set follows the conversation that took place here[1] and provides
> the "two small patches" I alluded to at the end of the thread.
> 
> Paul Cercueil: patch 1/2 is compile tested only - please see that it 
> does what
> you want.

Still works fine, so
Tested-by: Paul Cercueil <paul@crapouillou.net>

for the patchset.

Cheers,
-Paul

> Suman Anna: Please test on your side and confirm that it addresses 
> the Omap
> regression.
> 
> Applies on top of rproc-next (7dcef3988eed)
> 
> Thanks,
> Mathieu
> 
> [1]. 
> https://lore.kernel.org/linux-remoteproc/20200515104340.10473-1-paul@crapouillou.net/T/#t
> 
> Mathieu Poirier (2):
>   remoteproc: ingenic: Move clock handling to prepare/unprepare
>     callbacks
>   Revert "remoteproc: Add support for runtime PM"
> 
>  drivers/remoteproc/ingenic_rproc.c   | 84 
> +++++++++-------------------
>  drivers/remoteproc/remoteproc_core.c | 17 +-----
>  2 files changed, 27 insertions(+), 74 deletions(-)
> 
> --
> 2.25.1
>