| Message ID | 20211012103750.38328-5-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More SoCs for sdhci-omap to deprecate omap_hsmmc | expand |
Hi Tony,
I love your patch! Yet something to improve:
[auto build test ERROR on next-20211011]
[cannot apply to robh/for-next linus/master ulf-hansson-mmc-mirror/next v5.15-rc5 v5.15-rc4 v5.15-rc3 v5.15-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Tony-Lindgren/More-SoCs-for-sdhci-omap-to-deprecate-omap_hsmmc/20211012-183855
base: d3134eb5de8546a214c028fb7195e764b89da7d4
config: riscv-randconfig-r042-20211012 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c3dcf39554dbea780d6cb7e12239451ba47a2668)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/0day-ci/linux/commit/66e8ed22a31746643373772ef2fca668ca7d1a8f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Tony-Lindgren/More-SoCs-for-sdhci-omap-to-deprecate-omap_hsmmc/20211012-183855
git checkout 66e8ed22a31746643373772ef2fca668ca7d1a8f
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/mmc/host/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/mmc/host/sdhci-omap.c:948:10: warning: implicit conversion from 'unsigned long' to 'unsigned int' changes value from 18446744073709551615 to 4294967295 [-Wconstant-conversion]
return ~0UL;
~~~~~~ ^~~~
drivers/mmc/host/sdhci-omap.c:975:29: warning: result of comparison of constant 18446744073709551615 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare]
if (pbias != ~0UL && vqmmc == ~0UL)
~~~~~ ^ ~~~~
drivers/mmc/host/sdhci-omap.c:975:12: warning: result of comparison of constant 18446744073709551615 with expression of type 'unsigned int' is always true [-Wtautological-constant-out-of-range-compare]
if (pbias != ~0UL && vqmmc == ~0UL)
~~~~~ ^ ~~~~
drivers/mmc/host/sdhci-omap.c:977:16: warning: result of comparison of constant 18446744073709551615 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare]
else if (caps == ~0UL)
~~~~ ^ ~~~~
drivers/mmc/host/sdhci-omap.c:984:12: warning: result of comparison of constant 18446744073709551615 with expression of type 'unsigned int' is always true [-Wtautological-constant-out-of-range-compare]
if (pbias != ~0UL && (pbias & SDHCI_CAN_VDD_330) &&
~~~~~ ^ ~~~~
>> drivers/mmc/host/sdhci-omap.c:1485:21: error: use of undeclared identifier 'sdhci_omap_runtime_suspend'; did you mean '__pm_runtime_suspend'?
SET_RUNTIME_PM_OPS(sdhci_omap_runtime_suspend,
^~~~~~~~~~~~~~~~~~~~~~~~~~
__pm_runtime_suspend
include/linux/pm.h:341:21: note: expanded from macro 'SET_RUNTIME_PM_OPS'
.runtime_suspend = suspend_fn, \
^
include/linux/pm_runtime.h:39:12: note: '__pm_runtime_suspend' declared here
extern int __pm_runtime_suspend(struct device *dev, int rpmflags);
^
>> drivers/mmc/host/sdhci-omap.c:1486:7: error: use of undeclared identifier 'sdhci_omap_runtime_resume'; did you mean '__pm_runtime_resume'?
sdhci_omap_runtime_resume, NULL)
^~~~~~~~~~~~~~~~~~~~~~~~~
__pm_runtime_resume
include/linux/pm.h:342:20: note: expanded from macro 'SET_RUNTIME_PM_OPS'
.runtime_resume = resume_fn, \
^
include/linux/pm_runtime.h:40:12: note: '__pm_runtime_resume' declared here
extern int __pm_runtime_resume(struct device *dev, int rpmflags);
^
>> drivers/mmc/host/sdhci-omap.c:1485:21: error: incompatible function pointer types initializing 'int (*)(struct device *)' with an expression of type 'int (struct device *, int)' [-Werror,-Wincompatible-function-pointer-types]
SET_RUNTIME_PM_OPS(sdhci_omap_runtime_suspend,
^~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/pm.h:341:21: note: expanded from macro 'SET_RUNTIME_PM_OPS'
.runtime_suspend = suspend_fn, \
^~~~~~~~~~
drivers/mmc/host/sdhci-omap.c:1486:7: error: incompatible function pointer types initializing 'int (*)(struct device *)' with an expression of type 'int (struct device *, int)' [-Werror,-Wincompatible-function-pointer-types]
sdhci_omap_runtime_resume, NULL)
^~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/pm.h:342:20: note: expanded from macro 'SET_RUNTIME_PM_OPS'
.runtime_resume = resume_fn, \
^~~~~~~~~
5 warnings and 4 errors generated.
vim +1485 drivers/mmc/host/sdhci-omap.c
1483
1484 static const struct dev_pm_ops sdhci_omap_dev_pm_ops = {
> 1485 SET_RUNTIME_PM_OPS(sdhci_omap_runtime_suspend,
> 1486 sdhci_omap_runtime_resume, NULL)
1487 SET_SYSTEM_SLEEP_PM_OPS(sdhci_omap_suspend, sdhci_omap_resume)
1488 };
1489
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Tue, 12 Oct 2021 at 12:38, Tony Lindgren <tony@atomide.com> wrote: > > Implement PM runtime functions and enable autosuspend. > > Note that we save context in probe to avoid restoring invalid context > on the first resume. For system suspend, we have the new PM runtime > functions do most of the work. > > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/mmc/host/sdhci-omap.c | 78 ++++++++++++++++++++++++++++------- > 1 file changed, 63 insertions(+), 15 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c > --- a/drivers/mmc/host/sdhci-omap.c > +++ b/drivers/mmc/host/sdhci-omap.c > @@ -1207,6 +1207,8 @@ static const struct soc_device_attribute sdhci_omap_soc_devices[] = { > } > }; > > +static void sdhci_omap_context_save(struct sdhci_omap_host *omap_host); > + > static int sdhci_omap_probe(struct platform_device *pdev) > { > int ret; > @@ -1252,6 +1254,7 @@ static int sdhci_omap_probe(struct platform_device *pdev) > omap_host->timing = MMC_TIMING_LEGACY; > omap_host->flags = data->flags; > omap_host->omap_offset = data->omap_offset; > + omap_host->con = -EINVAL; > host->ioaddr += offset; > host->mapbase = regs->start + offset; > > @@ -1302,6 +1305,8 @@ static int sdhci_omap_probe(struct platform_device *pdev) > * SYSCONFIG register of omap devices. The callback will be invoked > * as part of pm_runtime_get_sync. > */ > + pm_runtime_use_autosuspend(dev); > + pm_runtime_set_autosuspend_delay(dev, 50); > pm_runtime_enable(dev); > ret = pm_runtime_resume_and_get(dev); > if (ret) { > @@ -1312,7 +1317,7 @@ static int sdhci_omap_probe(struct platform_device *pdev) > ret = sdhci_omap_set_capabilities(host); > if (ret) { > dev_err(dev, "failed to set system capabilities\n"); > - goto err_put_sync; > + goto err_rpm_put; > } > > host->mmc_host_ops.start_signal_voltage_switch = > @@ -1340,7 +1345,7 @@ static int sdhci_omap_probe(struct platform_device *pdev) > > ret = sdhci_setup_host(host); > if (ret) > - goto err_put_sync; > + goto err_rpm_put; > > ret = sdhci_omap_config_iodelay_pinctrl_state(omap_host); > if (ret) > @@ -1350,15 +1355,21 @@ static int sdhci_omap_probe(struct platform_device *pdev) > if (ret) > goto err_cleanup_host; > > + sdhci_omap_context_save(omap_host); Calling sdhci_omap_context_save() here looks unnecessary. The device is already runtime resumed at this point. In other words, sdhci_omap_context_save() will be called from the ->runtime_suspend() callback, next time the device becomes runtime suspended. That should be sufficient, right? > + > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > + > return 0; > > err_cleanup_host: > sdhci_cleanup_host(host); > > -err_put_sync: > - pm_runtime_put_sync(dev); > - > +err_rpm_put: > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > err_rpm_disable: > + pm_runtime_dont_use_autosuspend(dev); > pm_runtime_disable(dev); > > err_pltfm_free: > @@ -1371,8 +1382,12 @@ static int sdhci_omap_remove(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct sdhci_host *host = platform_get_drvdata(pdev); > > + pm_runtime_get_sync(dev); > sdhci_remove_host(host, true); > + pm_runtime_dont_use_autosuspend(dev); > pm_runtime_put_sync(dev); > + /* Ensure device gets idled despite userspace sysfs config */ > + pm_runtime_force_suspend(dev); > pm_runtime_disable(dev); The call to pm_runtime_disable() can be removed, as that is taken care of in pm_runtime_force_suspend(). > sdhci_pltfm_free(pdev); > > @@ -1402,42 +1417,75 @@ static void sdhci_omap_context_restore(struct sdhci_omap_host *omap_host) > sdhci_omap_writel(omap_host, SDHCI_OMAP_ISE, omap_host->ise); > } > > -static int __maybe_unused sdhci_omap_suspend(struct device *dev) > +static int __maybe_unused sdhci_omap_runtime_suspend(struct device *dev) > { > struct sdhci_host *host = dev_get_drvdata(dev); > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host); > > - sdhci_suspend_host(host); > + sdhci_runtime_suspend_host(host); > > sdhci_omap_context_save(omap_host); > > pinctrl_pm_select_idle_state(dev); > > - pm_runtime_force_suspend(dev); > - > return 0; > } > > -static int __maybe_unused sdhci_omap_resume(struct device *dev) > +static int __maybe_unused sdhci_omap_runtime_resume(struct device *dev) > { > struct sdhci_host *host = dev_get_drvdata(dev); > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host); > > - pm_runtime_force_resume(dev); > - > pinctrl_pm_select_default_state(dev); > > - sdhci_omap_context_restore(omap_host); > + if (omap_host->con != -EINVAL) > + sdhci_omap_context_restore(omap_host); > + > + sdhci_runtime_resume_host(host, 0); > + > + return 0; > +} > + > +static int __maybe_unused sdhci_omap_suspend(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + int err; > + > + /* Enable for configuring wakeups, paired in resume */ > + err = pm_runtime_resume_and_get(dev); > + if (err < 0) > + return err; > + > + sdhci_suspend_host(host); As far as I can tell, sdhci_suspend_host() doesn't really make sense for the omap variant. What you need, is to put the device into the same low power state as "runtime suspend", that should be sufficient. The system wakeup will be armed (and later then disarmed) by the PM core, when it calls device_wakeup_arm_wake_irqs() from the dpm_suspend_noirq() phase. In other words, pointing the system suspend/resume callbacks to pm_runtime_force_suspend|resume() should work fine, I think. > + > + return pm_runtime_force_suspend(dev); > +} > + > +static int __maybe_unused sdhci_omap_resume(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + int err; > + > + err = pm_runtime_force_resume(dev); > + if (err < 0) > + dev_warn(dev, "force resume failed: %i\n", err); > > sdhci_resume_host(host); > > + /* Balance pm_runtime_resume_and_get() done in suspend */ > + pm_runtime_put(dev); > + > return 0; > } > #endif > -static SIMPLE_DEV_PM_OPS(sdhci_omap_dev_pm_ops, sdhci_omap_suspend, > - sdhci_omap_resume); > + > +static const struct dev_pm_ops sdhci_omap_dev_pm_ops = { > + SET_RUNTIME_PM_OPS(sdhci_omap_runtime_suspend, > + sdhci_omap_runtime_resume, NULL) > + SET_SYSTEM_SLEEP_PM_OPS(sdhci_omap_suspend, sdhci_omap_resume) > +}; > > static struct platform_driver sdhci_omap_driver = { > .probe = sdhci_omap_probe, > -- > 2.33.0 Kind regards Uffe
* Ulf Hansson <ulf.hansson@linaro.org> [211012 15:17]: > On Tue, 12 Oct 2021 at 12:38, Tony Lindgren <tony@atomide.com> wrote: > > @@ -1350,15 +1355,21 @@ static int sdhci_omap_probe(struct platform_device *pdev) > > if (ret) > > goto err_cleanup_host; > > > > + sdhci_omap_context_save(omap_host); > > Calling sdhci_omap_context_save() here looks unnecessary. The device > is already runtime resumed at this point. > > In other words, sdhci_omap_context_save() will be called from the > ->runtime_suspend() callback, next time the device becomes runtime > suspended. That should be sufficient, right? Yup this can be now dropped with omap_host->con initialized to -EINVAL earlier. > > @@ -1371,8 +1382,12 @@ static int sdhci_omap_remove(struct platform_device *pdev) > > struct device *dev = &pdev->dev; > > struct sdhci_host *host = platform_get_drvdata(pdev); > > > > + pm_runtime_get_sync(dev); > > sdhci_remove_host(host, true); > > + pm_runtime_dont_use_autosuspend(dev); > > pm_runtime_put_sync(dev); > > + /* Ensure device gets idled despite userspace sysfs config */ > > + pm_runtime_force_suspend(dev); > > pm_runtime_disable(dev); > > The call to pm_runtime_disable() can be removed, as that is taken care > of in pm_runtime_force_suspend(). OK > > +static int __maybe_unused sdhci_omap_suspend(struct device *dev) > > +{ > > + struct sdhci_host *host = dev_get_drvdata(dev); > > + int err; > > + > > + /* Enable for configuring wakeups, paired in resume */ > > + err = pm_runtime_resume_and_get(dev); > > + if (err < 0) > > + return err; > > + > > + sdhci_suspend_host(host); > > As far as I can tell, sdhci_suspend_host() doesn't really make sense > for the omap variant. What you need, is to put the device into the > same low power state as "runtime suspend", that should be sufficient. > > The system wakeup will be armed (and later then disarmed) by the PM > core, when it calls device_wakeup_arm_wake_irqs() from the > dpm_suspend_noirq() phase. > > In other words, pointing the system suspend/resume callbacks to > pm_runtime_force_suspend|resume() should work fine, I think. OK sounds good to me. Regards, Tony
diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c --- a/drivers/mmc/host/sdhci-omap.c +++ b/drivers/mmc/host/sdhci-omap.c @@ -1207,6 +1207,8 @@ static const struct soc_device_attribute sdhci_omap_soc_devices[] = { } }; +static void sdhci_omap_context_save(struct sdhci_omap_host *omap_host); + static int sdhci_omap_probe(struct platform_device *pdev) { int ret; @@ -1252,6 +1254,7 @@ static int sdhci_omap_probe(struct platform_device *pdev) omap_host->timing = MMC_TIMING_LEGACY; omap_host->flags = data->flags; omap_host->omap_offset = data->omap_offset; + omap_host->con = -EINVAL; host->ioaddr += offset; host->mapbase = regs->start + offset; @@ -1302,6 +1305,8 @@ static int sdhci_omap_probe(struct platform_device *pdev) * SYSCONFIG register of omap devices. The callback will be invoked * as part of pm_runtime_get_sync. */ + pm_runtime_use_autosuspend(dev); + pm_runtime_set_autosuspend_delay(dev, 50); pm_runtime_enable(dev); ret = pm_runtime_resume_and_get(dev); if (ret) { @@ -1312,7 +1317,7 @@ static int sdhci_omap_probe(struct platform_device *pdev) ret = sdhci_omap_set_capabilities(host); if (ret) { dev_err(dev, "failed to set system capabilities\n"); - goto err_put_sync; + goto err_rpm_put; } host->mmc_host_ops.start_signal_voltage_switch = @@ -1340,7 +1345,7 @@ static int sdhci_omap_probe(struct platform_device *pdev) ret = sdhci_setup_host(host); if (ret) - goto err_put_sync; + goto err_rpm_put; ret = sdhci_omap_config_iodelay_pinctrl_state(omap_host); if (ret) @@ -1350,15 +1355,21 @@ static int sdhci_omap_probe(struct platform_device *pdev) if (ret) goto err_cleanup_host; + sdhci_omap_context_save(omap_host); + + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + return 0; err_cleanup_host: sdhci_cleanup_host(host); -err_put_sync: - pm_runtime_put_sync(dev); - +err_rpm_put: + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); err_rpm_disable: + pm_runtime_dont_use_autosuspend(dev); pm_runtime_disable(dev); err_pltfm_free: @@ -1371,8 +1382,12 @@ static int sdhci_omap_remove(struct platform_device *pdev) struct device *dev = &pdev->dev; struct sdhci_host *host = platform_get_drvdata(pdev); + pm_runtime_get_sync(dev); sdhci_remove_host(host, true); + pm_runtime_dont_use_autosuspend(dev); pm_runtime_put_sync(dev); + /* Ensure device gets idled despite userspace sysfs config */ + pm_runtime_force_suspend(dev); pm_runtime_disable(dev); sdhci_pltfm_free(pdev); @@ -1402,42 +1417,75 @@ static void sdhci_omap_context_restore(struct sdhci_omap_host *omap_host) sdhci_omap_writel(omap_host, SDHCI_OMAP_ISE, omap_host->ise); } -static int __maybe_unused sdhci_omap_suspend(struct device *dev) +static int __maybe_unused sdhci_omap_runtime_suspend(struct device *dev) { struct sdhci_host *host = dev_get_drvdata(dev); struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host); - sdhci_suspend_host(host); + sdhci_runtime_suspend_host(host); sdhci_omap_context_save(omap_host); pinctrl_pm_select_idle_state(dev); - pm_runtime_force_suspend(dev); - return 0; } -static int __maybe_unused sdhci_omap_resume(struct device *dev) +static int __maybe_unused sdhci_omap_runtime_resume(struct device *dev) { struct sdhci_host *host = dev_get_drvdata(dev); struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host); - pm_runtime_force_resume(dev); - pinctrl_pm_select_default_state(dev); - sdhci_omap_context_restore(omap_host); + if (omap_host->con != -EINVAL) + sdhci_omap_context_restore(omap_host); + + sdhci_runtime_resume_host(host, 0); + + return 0; +} + +static int __maybe_unused sdhci_omap_suspend(struct device *dev) +{ + struct sdhci_host *host = dev_get_drvdata(dev); + int err; + + /* Enable for configuring wakeups, paired in resume */ + err = pm_runtime_resume_and_get(dev); + if (err < 0) + return err; + + sdhci_suspend_host(host); + + return pm_runtime_force_suspend(dev); +} + +static int __maybe_unused sdhci_omap_resume(struct device *dev) +{ + struct sdhci_host *host = dev_get_drvdata(dev); + int err; + + err = pm_runtime_force_resume(dev); + if (err < 0) + dev_warn(dev, "force resume failed: %i\n", err); sdhci_resume_host(host); + /* Balance pm_runtime_resume_and_get() done in suspend */ + pm_runtime_put(dev); + return 0; } #endif -static SIMPLE_DEV_PM_OPS(sdhci_omap_dev_pm_ops, sdhci_omap_suspend, - sdhci_omap_resume); + +static const struct dev_pm_ops sdhci_omap_dev_pm_ops = { + SET_RUNTIME_PM_OPS(sdhci_omap_runtime_suspend, + sdhci_omap_runtime_resume, NULL) + SET_SYSTEM_SLEEP_PM_OPS(sdhci_omap_suspend, sdhci_omap_resume) +}; static struct platform_driver sdhci_omap_driver = { .probe = sdhci_omap_probe,
Implement PM runtime functions and enable autosuspend. Note that we save context in probe to avoid restoring invalid context on the first resume. For system suspend, we have the new PM runtime functions do most of the work. Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/mmc/host/sdhci-omap.c | 78 ++++++++++++++++++++++++++++------- 1 file changed, 63 insertions(+), 15 deletions(-)