linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] spi: imx: Revert "spi: imx: enable runtime pm support"
@ 2020-10-09  4:27 Christian Eggers
  2020-10-09  7:39 ` Sascha Hauer
  0 siblings, 1 reply; 9+ messages in thread
From: Christian Eggers @ 2020-10-09  4:27 UTC (permalink / raw)
  To: Mark Brown, Shawn Guo, Sascha Hauer
  Cc: Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	linux-spi, linux-arm-kernel, linux-kernel, Christian Eggers

This reverts commit 525c9e5a32bd7951eae3f06d9d077fea51718a6c.

If CONFIG_PM is disabled, the system completely freezes on probe as
nothing enables the clock of the SPI peripheral.

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 drivers/spi/spi-imx.c | 121 ++++++++++++------------------------------
 1 file changed, 33 insertions(+), 88 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 38a5f1304cec..fdc25f549378 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -13,9 +13,7 @@
 #include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
-#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/spi/spi.h>
 #include <linux/spi/spi_bitbang.h>
@@ -32,8 +30,6 @@ static bool use_dma = true;
 module_param(use_dma, bool, 0644);
 MODULE_PARM_DESC(use_dma, "Enable usage of DMA when available (default)");
 
-#define MXC_RPM_TIMEOUT		2000 /* 2000ms */
-
 #define MXC_CSPIRXDATA		0x00
 #define MXC_CSPITXDATA		0x04
 #define MXC_CSPICTRL		0x08
@@ -1534,16 +1530,20 @@ spi_imx_prepare_message(struct spi_master *master, struct spi_message *msg)
 	struct spi_imx_data *spi_imx = spi_master_get_devdata(master);
 	int ret;
 
-	ret = pm_runtime_get_sync(spi_imx->dev);
-	if (ret < 0) {
-		dev_err(spi_imx->dev, "failed to enable clock\n");
+	ret = clk_enable(spi_imx->clk_per);
+	if (ret)
+		return ret;
+
+	ret = clk_enable(spi_imx->clk_ipg);
+	if (ret) {
+		clk_disable(spi_imx->clk_per);
 		return ret;
 	}
 
 	ret = spi_imx->devtype_data->prepare_message(spi_imx, msg);
 	if (ret) {
-		pm_runtime_mark_last_busy(spi_imx->dev);
-		pm_runtime_put_autosuspend(spi_imx->dev);
+		clk_disable(spi_imx->clk_ipg);
+		clk_disable(spi_imx->clk_per);
 	}
 
 	return ret;
@@ -1554,8 +1554,8 @@ spi_imx_unprepare_message(struct spi_master *master, struct spi_message *msg)
 {
 	struct spi_imx_data *spi_imx = spi_master_get_devdata(master);
 
-	pm_runtime_mark_last_busy(spi_imx->dev);
-	pm_runtime_put_autosuspend(spi_imx->dev);
+	clk_disable(spi_imx->clk_ipg);
+	clk_disable(spi_imx->clk_per);
 	return 0;
 }
 
@@ -1674,15 +1674,13 @@ static int spi_imx_probe(struct platform_device *pdev)
 		goto out_master_put;
 	}
 
-	pm_runtime_enable(spi_imx->dev);
-	pm_runtime_set_autosuspend_delay(spi_imx->dev, MXC_RPM_TIMEOUT);
-	pm_runtime_use_autosuspend(spi_imx->dev);
+	ret = clk_prepare_enable(spi_imx->clk_per);
+	if (ret)
+		goto out_master_put;
 
-	ret = pm_runtime_get_sync(spi_imx->dev);
-	if (ret < 0) {
-		dev_err(spi_imx->dev, "failed to enable clock\n");
-		goto out_runtime_pm_put;
-	}
+	ret = clk_prepare_enable(spi_imx->clk_ipg);
+	if (ret)
+		goto out_put_per;
 
 	spi_imx->spi_clk = clk_get_rate(spi_imx->clk_per);
 	/*
@@ -1692,7 +1690,7 @@ static int spi_imx_probe(struct platform_device *pdev)
 	if (spi_imx->devtype_data->has_dmamode) {
 		ret = spi_imx_sdma_init(&pdev->dev, spi_imx, master);
 		if (ret == -EPROBE_DEFER)
-			goto out_runtime_pm_put;
+			goto out_clk_put;
 
 		if (ret < 0)
 			dev_err(&pdev->dev, "dma setup error %d, use pio\n",
@@ -1707,20 +1705,19 @@ static int spi_imx_probe(struct platform_device *pdev)
 	ret = spi_bitbang_start(&spi_imx->bitbang);
 	if (ret) {
 		dev_err(&pdev->dev, "bitbang start failed with %d\n", ret);
-		goto out_runtime_pm_put;
+		goto out_clk_put;
 	}
 
 	dev_info(&pdev->dev, "probed\n");
 
-	pm_runtime_mark_last_busy(spi_imx->dev);
-	pm_runtime_put_autosuspend(spi_imx->dev);
-
+	clk_disable(spi_imx->clk_ipg);
+	clk_disable(spi_imx->clk_per);
 	return ret;
 
-out_runtime_pm_put:
-	pm_runtime_dont_use_autosuspend(spi_imx->dev);
-	pm_runtime_put_sync(spi_imx->dev);
-	pm_runtime_disable(spi_imx->dev);
+out_clk_put:
+	clk_disable_unprepare(spi_imx->clk_ipg);
+out_put_per:
+	clk_disable_unprepare(spi_imx->clk_per);
 out_master_put:
 	spi_master_put(master);
 
@@ -1735,82 +1732,30 @@ static int spi_imx_remove(struct platform_device *pdev)
 
 	spi_bitbang_stop(&spi_imx->bitbang);
 
-	ret = pm_runtime_get_sync(spi_imx->dev);
-	if (ret < 0) {
-		dev_err(spi_imx->dev, "failed to enable clock\n");
-		return ret;
-	}
-
-	writel(0, spi_imx->base + MXC_CSPICTRL);
-
-	pm_runtime_dont_use_autosuspend(spi_imx->dev);
-	pm_runtime_put_sync(spi_imx->dev);
-	pm_runtime_disable(spi_imx->dev);
-
-	spi_imx_sdma_exit(spi_imx);
-	spi_master_put(master);
-
-	return 0;
-}
-
-static int __maybe_unused spi_imx_runtime_resume(struct device *dev)
-{
-	struct spi_master *master = dev_get_drvdata(dev);
-	struct spi_imx_data *spi_imx;
-	int ret;
-
-	spi_imx = spi_master_get_devdata(master);
-
-	ret = clk_prepare_enable(spi_imx->clk_per);
+	ret = clk_enable(spi_imx->clk_per);
 	if (ret)
 		return ret;
 
-	ret = clk_prepare_enable(spi_imx->clk_ipg);
+	ret = clk_enable(spi_imx->clk_ipg);
 	if (ret) {
-		clk_disable_unprepare(spi_imx->clk_per);
+		clk_disable(spi_imx->clk_per);
 		return ret;
 	}
 
-	return 0;
-}
-
-static int __maybe_unused spi_imx_runtime_suspend(struct device *dev)
-{
-	struct spi_master *master = dev_get_drvdata(dev);
-	struct spi_imx_data *spi_imx;
-
-	spi_imx = spi_master_get_devdata(master);
-
-	clk_disable_unprepare(spi_imx->clk_per);
+	writel(0, spi_imx->base + MXC_CSPICTRL);
 	clk_disable_unprepare(spi_imx->clk_ipg);
+	clk_disable_unprepare(spi_imx->clk_per);
+	spi_imx_sdma_exit(spi_imx);
+	spi_master_put(master);
 
 	return 0;
 }
 
-static int __maybe_unused spi_imx_suspend(struct device *dev)
-{
-	pinctrl_pm_select_sleep_state(dev);
-	return 0;
-}
-
-static int __maybe_unused spi_imx_resume(struct device *dev)
-{
-	pinctrl_pm_select_default_state(dev);
-	return 0;
-}
-
-static const struct dev_pm_ops imx_spi_pm = {
-	SET_RUNTIME_PM_OPS(spi_imx_runtime_suspend,
-				spi_imx_runtime_resume, NULL)
-	SET_SYSTEM_SLEEP_PM_OPS(spi_imx_suspend, spi_imx_resume)
-};
-
 static struct platform_driver spi_imx_driver = {
 	.driver = {
 		   .name = DRIVER_NAME,
 		   .of_match_table = spi_imx_dt_ids,
-		   .pm = &imx_spi_pm,
-	},
+		   },
 	.id_table = spi_imx_devtype,
 	.probe = spi_imx_probe,
 	.remove = spi_imx_remove,
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] spi: imx: Revert "spi: imx: enable runtime pm support"
  2020-10-09  4:27 [PATCH] spi: imx: Revert "spi: imx: enable runtime pm support" Christian Eggers
@ 2020-10-09  7:39 ` Sascha Hauer
  2020-10-09  7:48   ` Christian Eggers
  2020-10-12 10:59   ` Christian Eggers
  0 siblings, 2 replies; 9+ messages in thread
From: Sascha Hauer @ 2020-10-09  7:39 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Mark Brown, Shawn Guo, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-spi, linux-arm-kernel, linux-kernel

On Fri, Oct 09, 2020 at 06:27:38AM +0200, Christian Eggers wrote:
> This reverts commit 525c9e5a32bd7951eae3f06d9d077fea51718a6c.
> 
> If CONFIG_PM is disabled, the system completely freezes on probe as
> nothing enables the clock of the SPI peripheral.

Instead of reverting it, why not just fix it?

Normally the device should be brought to active state manually in probe
before pm_runtime takes over, then CONFIG_PM disabled doesn't hurt.
Using pm_runtime to put the device to active state initially has the
problem you describe.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] spi: imx: Revert "spi: imx: enable runtime pm support"
  2020-10-09  7:39 ` Sascha Hauer
@ 2020-10-09  7:48   ` Christian Eggers
  2020-10-12 14:07     ` Sascha Hauer
  2020-10-12 10:59   ` Christian Eggers
  1 sibling, 1 reply; 9+ messages in thread
From: Christian Eggers @ 2020-10-09  7:48 UTC (permalink / raw)
  To: Sascha Hauer
  Cc: Mark Brown, Shawn Guo, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-spi, linux-arm-kernel, linux-kernel

Hi Sascha,

On Friday, 9 October 2020, 09:39:44 CEST, Sascha Hauer wrote:
> On Fri, Oct 09, 2020 at 06:27:38AM +0200, Christian Eggers wrote:
> > This reverts commit 525c9e5a32bd7951eae3f06d9d077fea51718a6c.
> > 
> > If CONFIG_PM is disabled, the system completely freezes on probe as
> > nothing enables the clock of the SPI peripheral.
> 
> Instead of reverting it, why not just fix it?
I expect that 5.9 will be released soon. I think that in this late stage 
reverting is more safe than fixing...

> Normally the device should be brought to active state manually in probe
> before pm_runtime takes over, then CONFIG_PM disabled doesn't hurt.
> Using pm_runtime to put the device to active state initially has the
> problem you describe.
Thanks for the hint. I've no experience in runtime power management. If you 
could provide a patch against the current version, I can make a quick test. I 
can also try to fix it myself, but this will take until next week.

Best regards
Christian




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] spi: imx: Revert "spi: imx: enable runtime pm support"
  2020-10-09  7:39 ` Sascha Hauer
  2020-10-09  7:48   ` Christian Eggers
@ 2020-10-12 10:59   ` Christian Eggers
  2020-10-12 13:28     ` Sascha Hauer
  1 sibling, 1 reply; 9+ messages in thread
From: Christian Eggers @ 2020-10-12 10:59 UTC (permalink / raw)
  To: Sascha Hauer
  Cc: Mark Brown, Shawn Guo, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-spi, linux-arm-kernel, linux-kernel,
	Clark Wang

Hi Sascha,

On Friday, 9 October 2020, 09:39:44 CEST, Sascha Hauer wrote:
> On Fri, Oct 09, 2020 at 06:27:38AM +0200, Christian Eggers wrote:
> > This reverts commit 525c9e5a32bd7951eae3f06d9d077fea51718a6c.
> > 
> > If CONFIG_PM is disabled, the system completely freezes on probe as
> > nothing enables the clock of the SPI peripheral.
> 
> Instead of reverting it, why not just fix it?
> 
> Normally the device should be brought to active state manually in probe
> before pm_runtime takes over, then CONFIG_PM disabled doesn't hurt.
> Using pm_runtime to put the device to active state initially has the
> problem you describe.

prior introducing runtime pm for spi-imx, the clock was "manually" enabled and 
disabled around each transfer (so the power usage should already have been 
optimal). If we would manually enable the clock in probe() as you suggested, 
for users without CONFIG_PM there would be a drawback compared with the 
previous state (as the clock will always be on now).

What is the benefit of controlling the SPI clock with runtime PM instead of 
doing it manually?

As I have no experience with runtime PM, hopefully somebody else can fix (or 
revert) this.

@Clark: I forgot to put you on CC on my initial message. You can find the full 
discussion here:
https://lore.kernel.org/patchwork/patch/1318736/

Best regards
Christian





^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] spi: imx: Revert "spi: imx: enable runtime pm support"
  2020-10-12 10:59   ` Christian Eggers
@ 2020-10-12 13:28     ` Sascha Hauer
  2020-10-12 13:42       ` Mark Brown
  0 siblings, 1 reply; 9+ messages in thread
From: Sascha Hauer @ 2020-10-12 13:28 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Mark Brown, Shawn Guo, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-spi, linux-arm-kernel, linux-kernel,
	Clark Wang

On Mon, Oct 12, 2020 at 12:59:34PM +0200, Christian Eggers wrote:
> Hi Sascha,
> 
> On Friday, 9 October 2020, 09:39:44 CEST, Sascha Hauer wrote:
> > On Fri, Oct 09, 2020 at 06:27:38AM +0200, Christian Eggers wrote:
> > > This reverts commit 525c9e5a32bd7951eae3f06d9d077fea51718a6c.
> > >
> > > If CONFIG_PM is disabled, the system completely freezes on probe as
> > > nothing enables the clock of the SPI peripheral.
> >
> > Instead of reverting it, why not just fix it?
> >
> > Normally the device should be brought to active state manually in probe
> > before pm_runtime takes over, then CONFIG_PM disabled doesn't hurt.
> > Using pm_runtime to put the device to active state initially has the
> > problem you describe.
> 
> prior introducing runtime pm for spi-imx, the clock was "manually" enabled and
> disabled around each transfer (so the power usage should already have been
> optimal). If we would manually enable the clock in probe() as you suggested,
> for users without CONFIG_PM there would be a drawback compared with the
> previous state (as the clock will always be on now).
> 
> What is the benefit of controlling the SPI clock with runtime PM instead of
> doing it manually?

The clocks are reconfigured less frequently with pm_runtime. Especially
when enabling/disabling PLLs are involved pm_runtime can increase
performance.

Also you can put other stuff you need to handle for your device into
pm_runtime, think of regulators for example. All this is then abstracted
behind a common kernel API.

Generally when you disable CONFIG_PM then you are not interested in
power consumption and in that case you shouldn't be interested in
disabling your SPI clocks.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] spi: imx: Revert "spi: imx: enable runtime pm support"
  2020-10-12 13:28     ` Sascha Hauer
@ 2020-10-12 13:42       ` Mark Brown
  0 siblings, 0 replies; 9+ messages in thread
From: Mark Brown @ 2020-10-12 13:42 UTC (permalink / raw)
  To: Sascha Hauer
  Cc: Christian Eggers, Shawn Guo, Pengutronix Kernel Team,
	Fabio Estevam, NXP Linux Team, linux-spi, linux-arm-kernel,
	linux-kernel, Clark Wang

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

On Mon, Oct 12, 2020 at 03:28:21PM +0200, Sascha Hauer wrote:
> On Mon, Oct 12, 2020 at 12:59:34PM +0200, Christian Eggers wrote:

> > What is the benefit of controlling the SPI clock with runtime PM instead of
> > doing it manually?

> The clocks are reconfigured less frequently with pm_runtime. Especially
> when enabling/disabling PLLs are involved pm_runtime can increase
> performance.

In particular pm_runtime has support for deferring the actual disable so
if you get lots of activity in quick succession you don't actually ever
disable things until the activity stops.  This can really help reduce
overhead.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] spi: imx: Revert "spi: imx: enable runtime pm support"
  2020-10-09  7:48   ` Christian Eggers
@ 2020-10-12 14:07     ` Sascha Hauer
  2020-10-13 11:58       ` Christian Eggers
  2020-10-14 12:15       ` Mark Brown
  0 siblings, 2 replies; 9+ messages in thread
From: Sascha Hauer @ 2020-10-12 14:07 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Mark Brown, Shawn Guo, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-spi, linux-arm-kernel, linux-kernel

On Fri, Oct 09, 2020 at 09:48:29AM +0200, Christian Eggers wrote:
> Hi Sascha,
> 
> On Friday, 9 October 2020, 09:39:44 CEST, Sascha Hauer wrote:
> > On Fri, Oct 09, 2020 at 06:27:38AM +0200, Christian Eggers wrote:
> > > This reverts commit 525c9e5a32bd7951eae3f06d9d077fea51718a6c.
> > >
> > > If CONFIG_PM is disabled, the system completely freezes on probe as
> > > nothing enables the clock of the SPI peripheral.
> >
> > Instead of reverting it, why not just fix it?
> I expect that 5.9 will be released soon. I think that in this late stage
> reverting is more safe than fixing...
> 
> > Normally the device should be brought to active state manually in probe
> > before pm_runtime takes over, then CONFIG_PM disabled doesn't hurt.
> > Using pm_runtime to put the device to active state initially has the
> > problem you describe.
> Thanks for the hint. I've no experience in runtime power management. If you
> could provide a patch against the current version, I can make a quick test. I
> can also try to fix it myself, but this will take until next week.

Here we go. The patch basically works, but I am also not very confident
with pm_runtime, so please have a close look ;)

Sascha

-------------------------------8<--------------------------------

From 6c584eb8a2fbff46bf8bbebae6c10609c169309b Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Mon, 12 Oct 2020 15:59:50 +0200
Subject: [PATCH] spi: imx: fix runtime pm support for !CONFIG_PM

525c9e5a32bd introduced pm_runtime support for the i.MX SPI driver. With
this pm_runtime is used to bring up the clocks initially. When CONFIG_PM
is disabled the clocks are no longer enabled and the driver doesn't work
anymore. Fix this by enabling the clocks in the probe function and
telling pm_runtime that the device is active using
pm_runtime_set_active().

Fixes: 525c9e5a32bd spi: imx: enable runtime pm support
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/spi/spi-imx.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 38a5f1304cec..c796e937dc6a 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -1674,15 +1674,18 @@ static int spi_imx_probe(struct platform_device *pdev)
 		goto out_master_put;
 	}
 
-	pm_runtime_enable(spi_imx->dev);
+	ret = clk_prepare_enable(spi_imx->clk_per);
+	if (ret)
+		goto out_master_put;
+
+	ret = clk_prepare_enable(spi_imx->clk_ipg);
+	if (ret)
+		goto out_put_per;
+
 	pm_runtime_set_autosuspend_delay(spi_imx->dev, MXC_RPM_TIMEOUT);
 	pm_runtime_use_autosuspend(spi_imx->dev);
-
-	ret = pm_runtime_get_sync(spi_imx->dev);
-	if (ret < 0) {
-		dev_err(spi_imx->dev, "failed to enable clock\n");
-		goto out_runtime_pm_put;
-	}
+	pm_runtime_set_active(spi_imx->dev);
+	pm_runtime_enable(spi_imx->dev);
 
 	spi_imx->spi_clk = clk_get_rate(spi_imx->clk_per);
 	/*
@@ -1719,8 +1722,12 @@ static int spi_imx_probe(struct platform_device *pdev)
 
 out_runtime_pm_put:
 	pm_runtime_dont_use_autosuspend(spi_imx->dev);
-	pm_runtime_put_sync(spi_imx->dev);
+	pm_runtime_set_suspended(&pdev->dev);
 	pm_runtime_disable(spi_imx->dev);
+
+	clk_disable_unprepare(spi_imx->clk_ipg);
+out_put_per:
+	clk_disable_unprepare(spi_imx->clk_per);
 out_master_put:
 	spi_master_put(master);
 
-- 
2.28.0


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] spi: imx: Revert "spi: imx: enable runtime pm support"
  2020-10-12 14:07     ` Sascha Hauer
@ 2020-10-13 11:58       ` Christian Eggers
  2020-10-14 12:15       ` Mark Brown
  1 sibling, 0 replies; 9+ messages in thread
From: Christian Eggers @ 2020-10-13 11:58 UTC (permalink / raw)
  To: Sascha Hauer
  Cc: Mark Brown, Shawn Guo, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-spi, linux-arm-kernel, linux-kernel

On Monday, 12 October 2020, 16:07:53 CEST, Sascha Hauer wrote:
> On Fri, Oct 09, 2020 at 09:48:29AM +0200, Christian Eggers wrote:
> 
> 525c9e5a32bd introduced pm_runtime support for the i.MX SPI driver. With
> this pm_runtime is used to bring up the clocks initially. When CONFIG_PM
> is disabled the clocks are no longer enabled and the driver doesn't work
> anymore. Fix this by enabling the clocks in the probe function and
> telling pm_runtime that the device is active using
> pm_runtime_set_active().
> 
> Fixes: 525c9e5a32bd spi: imx: enable runtime pm support
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/spi/spi-imx.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 38a5f1304cec..c796e937dc6a 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -1674,15 +1674,18 @@ static int spi_imx_probe(struct platform_device
> *pdev) goto out_master_put;
>  	}
> 
> -	pm_runtime_enable(spi_imx->dev);
> +	ret = clk_prepare_enable(spi_imx->clk_per);
> +	if (ret)
> +		goto out_master_put;
> +
> +	ret = clk_prepare_enable(spi_imx->clk_ipg);
> +	if (ret)
> +		goto out_put_per;
> +
>  	pm_runtime_set_autosuspend_delay(spi_imx->dev, MXC_RPM_TIMEOUT);
>  	pm_runtime_use_autosuspend(spi_imx->dev);
> -
> -	ret = pm_runtime_get_sync(spi_imx->dev);
> -	if (ret < 0) {
> -		dev_err(spi_imx->dev, "failed to enable clock\n");
> -		goto out_runtime_pm_put;
> -	}
> +	pm_runtime_set_active(spi_imx->dev);
> +	pm_runtime_enable(spi_imx->dev);
> 
>  	spi_imx->spi_clk = clk_get_rate(spi_imx->clk_per);
>  	/*
> @@ -1719,8 +1722,12 @@ static int spi_imx_probe(struct platform_device
> *pdev)
> 
>  out_runtime_pm_put:
>  	pm_runtime_dont_use_autosuspend(spi_imx->dev);
> -	pm_runtime_put_sync(spi_imx->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
>  	pm_runtime_disable(spi_imx->dev);
> +
> +	clk_disable_unprepare(spi_imx->clk_ipg);
> +out_put_per:
> +	clk_disable_unprepare(spi_imx->clk_per);
>  out_master_put:
>  	spi_master_put(master);

With this patch applied, my system (!CONFIG_PM) doesn't freeze anymore when 
the spi-imx driver is loaded.

Thank you very much!

Tested-by: Christian Eggers <ceggers@arri.de>
[tested for !CONFIG_PM only]
Cc: stable@vger.kernel.org  # 5.9.x only




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] spi: imx: Revert "spi: imx: enable runtime pm support"
  2020-10-12 14:07     ` Sascha Hauer
  2020-10-13 11:58       ` Christian Eggers
@ 2020-10-14 12:15       ` Mark Brown
  1 sibling, 0 replies; 9+ messages in thread
From: Mark Brown @ 2020-10-14 12:15 UTC (permalink / raw)
  To: Sascha Hauer
  Cc: Christian Eggers, Shawn Guo, Pengutronix Kernel Team,
	Fabio Estevam, NXP Linux Team, linux-spi, linux-arm-kernel,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]

On Mon, Oct 12, 2020 at 04:07:53PM +0200, Sascha Hauer wrote:

> > can also try to fix it myself, but this will take until next week.
> 
> Here we go. The patch basically works, but I am also not very confident
> with pm_runtime, so please have a close look ;)

Sasha could you post this patch normally please - none of my automation
can cope with things buried in the middle of other e-mails?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-10-14 12:15 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-09  4:27 [PATCH] spi: imx: Revert "spi: imx: enable runtime pm support" Christian Eggers
2020-10-09  7:39 ` Sascha Hauer
2020-10-09  7:48   ` Christian Eggers
2020-10-12 14:07     ` Sascha Hauer
2020-10-13 11:58       ` Christian Eggers
2020-10-14 12:15       ` Mark Brown
2020-10-12 10:59   ` Christian Eggers
2020-10-12 13:28     ` Sascha Hauer
2020-10-12 13:42       ` Mark Brown

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).