| Message ID | 20200721203723.18305-10-yung-chuan.liao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: intel: add power management support | expand |
On 22-07-20, 04:37, Bard Liao wrote: > From: Rander Wang <rander.wang@intel.com> > > Move existing pm_runtime suspend under the CLK_STOP_TEARDOWN case. > > In this mode the Master IP will lose all context but in-band wakes are > supported. > > On pm_runtime resume a complete re-enumeration will be performed after > a bus reset. > > Signed-off-by: Rander Wang <rander.wang@intel.com> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> > --- > drivers/soundwire/intel.c | 44 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c > index 1954eb48b86c..744fc0a4816a 100644 > --- a/drivers/soundwire/intel.c > +++ b/drivers/soundwire/intel.c > @@ -1576,6 +1576,26 @@ static int intel_suspend_runtime(struct device *dev) > > intel_shim_wake(sdw, false); > > + } else if (clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET) { > + ret = sdw_cdns_clock_stop(cdns, true); > + if (ret < 0) { > + dev_err(dev, "cannot enable clock stop on suspend\n"); > + return ret; > + } > + > + ret = sdw_cdns_enable_interrupt(cdns, false); > + if (ret < 0) { > + dev_err(dev, "cannot disable interrupts on suspend\n"); > + return ret; > + } > + > + ret = intel_link_power_down(sdw); > + if (ret) { > + dev_err(dev, "Link power down failed: %d", ret); > + return ret; > + } no cleanup on all the error cases here? > + > + intel_shim_wake(sdw, true); > } else { > dev_err(dev, "%s clock_stop_quirks %x unsupported\n", > __func__, clock_stop_quirks); > @@ -1694,6 +1714,30 @@ static int intel_resume_runtime(struct device *dev) > dev_err(dev, "unable to exit bus reset sequence during resume\n"); > return ret; > } > + } else if (clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET) { > + ret = intel_init(sdw); > + if (ret) { > + dev_err(dev, "%s failed: %d", __func__, ret); > + return ret; > + } > + > + /* > + * make sure all Slaves are tagged as UNATTACHED and > + * provide reason for reinitialization > + */ > + sdw_clear_slave_status(bus, SDW_UNATTACH_REQUEST_MASTER_RESET); > + > + ret = sdw_cdns_enable_interrupt(cdns, true); > + if (ret < 0) { > + dev_err(dev, "cannot enable interrupts during resume\n"); > + return ret; > + } > + > + ret = sdw_cdns_clock_restart(cdns, true); > + if (ret < 0) { > + dev_err(dev, "unable to restart clock during resume\n"); > + return ret; > + } > } else { > dev_err(dev, "%s clock_stop_quirks %x unsupported\n", > __func__, clock_stop_quirks); > -- > 2.17.1
>> + } else if (clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET) { >> + ret = sdw_cdns_clock_stop(cdns, true); >> + if (ret < 0) { >> + dev_err(dev, "cannot enable clock stop on suspend\n"); >> + return ret; >> + } >> + >> + ret = sdw_cdns_enable_interrupt(cdns, false); >> + if (ret < 0) { >> + dev_err(dev, "cannot disable interrupts on suspend\n"); >> + return ret; >> + } >> + >> + ret = intel_link_power_down(sdw); >> + if (ret) { >> + dev_err(dev, "Link power down failed: %d", ret); >> + return ret; >> + } > > no cleanup on all the error cases here? See above the 'else if' test, the clock stop on suspend will be followed by a bus reset on resume. this is essentially a complete bus restart. The only open here is whether we should actually return an error while suspending, or just log the error and squelch it. We decided to return the status so that the pm_runtime suspend does not proceed: the state remains active which is easier to detect than a single line in a dmesg log.
On 17-08-20, 09:30, Pierre-Louis Bossart wrote: > > > > > > + } else if (clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET) { > > > + ret = sdw_cdns_clock_stop(cdns, true); > > > + if (ret < 0) { > > > + dev_err(dev, "cannot enable clock stop on suspend\n"); > > > + return ret; > > > + } > > > + > > > + ret = sdw_cdns_enable_interrupt(cdns, false); > > > + if (ret < 0) { > > > + dev_err(dev, "cannot disable interrupts on suspend\n"); > > > + return ret; > > > + } > > > + > > > + ret = intel_link_power_down(sdw); > > > + if (ret) { > > > + dev_err(dev, "Link power down failed: %d", ret); > > > + return ret; > > > + } > > > > no cleanup on all the error cases here? > > See above the 'else if' test, the clock stop on suspend will be followed by > a bus reset on resume. this is essentially a complete bus restart. ok > The only open here is whether we should actually return an error while > suspending, or just log the error and squelch it. We decided to return the > status so that the pm_runtime suspend does not proceed: the state remains > active which is easier to detect than a single line in a dmesg log. right, returning makes sense and is done correctly above
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 1954eb48b86c..744fc0a4816a 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1576,6 +1576,26 @@ static int intel_suspend_runtime(struct device *dev) intel_shim_wake(sdw, false); + } else if (clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET) { + ret = sdw_cdns_clock_stop(cdns, true); + if (ret < 0) { + dev_err(dev, "cannot enable clock stop on suspend\n"); + return ret; + } + + ret = sdw_cdns_enable_interrupt(cdns, false); + if (ret < 0) { + dev_err(dev, "cannot disable interrupts on suspend\n"); + return ret; + } + + ret = intel_link_power_down(sdw); + if (ret) { + dev_err(dev, "Link power down failed: %d", ret); + return ret; + } + + intel_shim_wake(sdw, true); } else { dev_err(dev, "%s clock_stop_quirks %x unsupported\n", __func__, clock_stop_quirks); @@ -1694,6 +1714,30 @@ static int intel_resume_runtime(struct device *dev) dev_err(dev, "unable to exit bus reset sequence during resume\n"); return ret; } + } else if (clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET) { + ret = intel_init(sdw); + if (ret) { + dev_err(dev, "%s failed: %d", __func__, ret); + return ret; + } + + /* + * make sure all Slaves are tagged as UNATTACHED and + * provide reason for reinitialization + */ + sdw_clear_slave_status(bus, SDW_UNATTACH_REQUEST_MASTER_RESET); + + ret = sdw_cdns_enable_interrupt(cdns, true); + if (ret < 0) { + dev_err(dev, "cannot enable interrupts during resume\n"); + return ret; + } + + ret = sdw_cdns_clock_restart(cdns, true); + if (ret < 0) { + dev_err(dev, "unable to restart clock during resume\n"); + return ret; + } } else { dev_err(dev, "%s clock_stop_quirks %x unsupported\n", __func__, clock_stop_quirks);