| Message ID | 20180427155007.GB5671@atomide.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5a686b2c9ed44254d58d139f969c7c08685d923a |
| Headers | show |
On Fri, Apr 27, 2018 at 08:50:07AM -0700, Tony Lindgren wrote: > We currently are calling mcspi suspend and resume without considering > that mcspi might provide resources for other device driver such as > regulators. This means resume can fail and will produce -EACCES if > errors if anything calls mcspi functions between device_prepare() > and device_complete(). Please don't send new versions of individual patches in the middle of a series - it makes it hard to keep track of what the current version of the series actually is which makes applying things error prone especially if there's been discussion (not that there was much here) as the wrong version can easily get picked. It's a lot easier to just repost the whole series.
* Mark Brown <broonie@kernel.org> [180501 21:06]: > On Fri, Apr 27, 2018 at 08:50:07AM -0700, Tony Lindgren wrote: > > We currently are calling mcspi suspend and resume without considering > > that mcspi might provide resources for other device driver such as > > regulators. This means resume can fail and will produce -EACCES if > > errors if anything calls mcspi functions between device_prepare() > > and device_complete(). > > Please don't send new versions of individual patches in the middle of a > series - it makes it hard to keep track of what the current version of > the series actually is which makes applying things error prone > especially if there's been discussion (not that there was much here) as > the wrong version can easily get picked. It's a lot easier to just > repost the whole series. Oops sorry again. Let me know if you want me to repost the whole series. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 02, 2018 at 07:05:03AM -0700, Tony Lindgren wrote: > Oops sorry again. Let me know if you want me to repost the whole > series. I applied it already.
* Mark Brown <broonie@kernel.org> [180503 01:24]: > On Wed, May 02, 2018 at 07:05:03AM -0700, Tony Lindgren wrote: > > > Oops sorry again. Let me know if you want me to repost the whole > > series. > > I applied it already. OK thanks! Tony -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c --- a/drivers/spi/spi-omap2-mcspi.c +++ b/drivers/spi/spi-omap2-mcspi.c @@ -255,6 +255,7 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) if (spi->controller_state) { int err = pm_runtime_get_sync(mcspi->dev); if (err < 0) { + pm_runtime_put_noidle(mcspi->dev); dev_err(mcspi->dev, "failed to get sync: %d\n", err); return; } @@ -1051,8 +1052,11 @@ static int omap2_mcspi_setup(struct spi_device *spi) } ret = pm_runtime_get_sync(mcspi->dev); - if (ret < 0) + if (ret < 0) { + pm_runtime_put_noidle(mcspi->dev); + return ret; + } ret = omap2_mcspi_setup_transfer(spi, NULL); pm_runtime_mark_last_busy(mcspi->dev); @@ -1270,8 +1274,11 @@ static int omap2_mcspi_master_setup(struct omap2_mcspi *mcspi) int ret = 0; ret = pm_runtime_get_sync(mcspi->dev); - if (ret < 0) + if (ret < 0) { + pm_runtime_put_noidle(mcspi->dev); + return ret; + } mcspi_write_reg(master, OMAP2_MCSPI_WAKEUPENABLE, OMAP2_MCSPI_WAKEUPENABLE_WKEN); @@ -1458,24 +1465,55 @@ static int omap2_mcspi_remove(struct platform_device *pdev) MODULE_ALIAS("platform:omap2_mcspi"); #ifdef CONFIG_SUSPEND -static int omap2_mcspi_resume(struct device *dev) +static int omap2_mcspi_suspend_noirq(struct device *dev) { - return pinctrl_pm_select_default_state(dev); + int error; + + /* + * Make sure device gets idled if other drivers call SPI + * functions between device_prepare() and device_complete() + */ + error = pm_runtime_force_suspend(dev); + if (error < 0) { + dev_err(dev, "%s: force suspend failed: %i\n", + __func__, error); + + return error; + } + + return pinctrl_pm_select_sleep_state(dev); } -static int omap2_mcspi_suspend(struct device *dev) +static int omap2_mcspi_resume_noirq(struct device *dev) { - return pinctrl_pm_select_sleep_state(dev); + struct spi_master *master = dev_get_drvdata(dev); + struct omap2_mcspi *mcspi = spi_master_get_devdata(master); + int error; + + error = pinctrl_pm_select_default_state(dev); + if (error) + dev_warn(mcspi->dev, "%s: failed to set pins: %i\n", + __func__, error); + + error = pm_runtime_force_resume(mcspi->dev); + if (error < 0) { + dev_warn(mcspi->dev, "%s: force resume failed: %i\n", + __func__, error); + + return error; + } + + return 0; } #else -#define omap2_mcspi_suspend NULL -#define omap2_mcspi_resume NULL +#define omap2_mcspi_suspend_noirq NULL +#define omap2_mcspi_resume_noirq NULL #endif static const struct dev_pm_ops omap2_mcspi_pm_ops = { - .resume = omap2_mcspi_resume, - .suspend = omap2_mcspi_suspend, + .suspend_noirq = omap2_mcspi_suspend_noirq, + .resume_noirq = omap2_mcspi_resume_noirq, .runtime_resume = omap_mcspi_runtime_resume, };
We currently are calling mcspi suspend and resume without considering that mcspi might provide resources for other device driver such as regulators. This means resume can fail and will produce -EACCES if errors if anything calls mcspi functions between device_prepare() and device_complete(). To fix the issue, let's do the following changes: 1. Let's add checking for return values for pm_runtime_get calls, and call pm_runtime_put_noidle() on errors. Things still fail after this change, but at least we see something is wrong as we now see -EACCES errors on resume. 2. Let's use noirq level for suspend and resume as other drivers can still call SPI related functions on suspend and resume. This still won't fix the -EACCES issue, but gets us to something a bit saner. 3. Finally, let's modify suspend and resume to call to make sure the device is idled properly on suspend. We have device_prepare() call pm_runtime_get_noresume() that won't get released until in device_complete() when it calls pm_runtime_put(). So if SPI is still active on entering suspend, it will never get idled unless we add calls to pm_runtime_force_suspend() and resume. This also fixes the -EACCES errors on resume together with changes 1 and 2 above. And since we're already rewriting suspend resume functions, let's arrange the order of suspend and resume functions to be like they usually are with suspend first. Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/spi/spi-omap2-mcspi.c | 58 +++++++++++++++++++++++++++++------ 1 file changed, 48 insertions(+), 10 deletions(-)