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 <[email protected]>
---
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
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
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 |
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
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 |
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.
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 <[email protected]>
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 <[email protected]>
---
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 |
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 <[email protected]>
> ---
> 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 <[email protected]>
[tested for !CONFIG_PM only]
Cc: [email protected] # 5.9.x only
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?