| Message ID | 20201009042738.26602-1-ceggers@arri.de |
|---|---|
| State | New |
| Headers | show |
| Series | spi: imx: Revert "spi: imx: enable runtime pm support" | expand |
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? 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
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
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<--------------------------------
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
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?
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,
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(-)