[PATCH 00/13] soundwire: intel: add power management support
This series adds power management support for Intel soundwire links.
Bard Liao (1): soundwire: intel: reinitialize IP+DSP in .prepare(), but only when resuming
Pierre-Louis Bossart (10): soundwire: intel: Add basic power management support soundwire: intel: add pm_runtime support soundwire: intel: reset pm_runtime status during system resume soundwire: intel: fix race condition on system resume soundwire: intel: call helper to reset Slave states on resume soundwire: intel: pm_runtime idle scheduling soundwire: intel: add CLK_STOP_TEARDOWN for pm_runtime suspend soundwire: intel: add CLK_STOP_NOT_ALLOWED support soundwire: intel_init: handle power rail dependencies for clock stop mode soundwire: intel: support clock_stop mode without quirks
Rander Wang (2): soundwire: intel: add CLK_STOP_BUS_RESET support soundwire: intel: refine runtime pm for SDW_INTEL_CLK_STOP_BUS_RESET
drivers/soundwire/cadence_master.h | 4 + drivers/soundwire/intel.c | 448 ++++++++++++++++++++++++++++- drivers/soundwire/intel.h | 2 + drivers/soundwire/intel_init.c | 19 +- 4 files changed, 465 insertions(+), 8 deletions(-)
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Implement suspend/resume capabilities (not runtime_pm for now) The resume part is essentially a full-blown re-enumeration.
When S0ix is supported, we will select clock stop mode when the ACPI target state is S0, and tear down the link for S3.
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 | 81 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 2 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 23b66dcf9966..eb628f255cf5 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -463,7 +463,7 @@ static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable) mutex_unlock(sdw->link_res->shim_lock); }
-static int __maybe_unused intel_link_power_down(struct sdw_intel *sdw) +static int intel_link_power_down(struct sdw_intel *sdw) { int link_control, spa_mask, cpa_mask; unsigned int link_id = sdw->instance; @@ -1375,12 +1375,89 @@ int intel_master_process_wakeen_event(struct platform_device *pdev) return 0; }
+/* + * PM calls + */ + +#ifdef CONFIG_PM + +static int intel_suspend(struct device *dev) +{ + struct sdw_cdns *cdns = dev_get_drvdata(dev); + struct sdw_intel *sdw = cdns_to_intel(cdns); + struct sdw_bus *bus = &cdns->bus; + int ret; + + if (bus->prop.hw_disabled) { + dev_dbg(dev, "SoundWire master %d is disabled, ignoring\n", + bus->link_id); + return 0; + } + + 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, false); + + return 0; +} + +static int intel_resume(struct device *dev) +{ + struct sdw_cdns *cdns = dev_get_drvdata(dev); + struct sdw_intel *sdw = cdns_to_intel(cdns); + struct sdw_bus *bus = &cdns->bus; + int ret; + + if (bus->prop.hw_disabled) { + dev_dbg(dev, "SoundWire master %d is disabled, ignoring\n", + bus->link_id); + return 0; + } + + ret = intel_init(sdw); + if (ret) { + dev_err(dev, "%s failed: %d", __func__, ret); + return ret; + } + + ret = sdw_cdns_enable_interrupt(cdns, true); + if (ret < 0) { + dev_err(dev, "cannot enable interrupts during resume\n"); + return ret; + } + + ret = sdw_cdns_exit_reset(cdns); + if (ret < 0) { + dev_err(dev, "unable to exit bus reset sequence during resume\n"); + return ret; + } + + return ret; +} + +#endif + +static const struct dev_pm_ops intel_pm = { + SET_SYSTEM_SLEEP_PM_OPS(intel_suspend, intel_resume) +}; + static struct platform_driver sdw_intel_drv = { .probe = intel_master_probe, .remove = intel_master_remove, .driver = { .name = "intel-sdw", - }, + .pm = &intel_pm, + } };
module_platform_driver(sdw_intel_drv);
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Add basic hooks in DAI .startup and .shutdown callbacks.
The SoundWire IP should be powered between those two calls. The power dependencies between SoundWire and DSP are handled with the parent/child relationship, before the SoundWire master device becomes active the parent device will become active and power-up the shared rails.
For now the strategy is to rely on complete enumeration when the device becomes active, so the code is a copy/paste of the sequence for system suspend/resume. In future patches, the strategy will optionally be to rely on clock stop if the enumeration time is prohibitive or when the devices connected to a link can signal a wake.
A module parameter is added to make integration of new Slave devices easier, to e.g. keep the device active or prevent clock-stop.
Note that we need to we have to disable runtime pm before device unregister, otherwise we will see "Failed to power up link: -11" error on module remove test.
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 | 112 +++++++++++++++++++++++++++++++-- drivers/soundwire/intel_init.c | 4 +- 2 files changed, 111 insertions(+), 5 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index eb628f255cf5..ed7163ae5f7a 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -22,6 +22,22 @@ #include "bus.h" #include "intel.h"
+#define INTEL_MASTER_SUSPEND_DELAY_MS 3000 + +/* + * debug/config flags for the Intel SoundWire Master. + * + * Since we may have multiple masters active, we can have up to 8 + * flags reused in each byte, with master0 using the ls-byte, etc. + */ + +#define SDW_INTEL_MASTER_DISABLE_PM_RUNTIME BIT(0) +#define SDW_INTEL_MASTER_DISABLE_CLOCK_STOP BIT(1) + +static int md_flags; +module_param_named(sdw_md_flags, md_flags, int, 0444); +MODULE_PARM_DESC(sdw_md_flags, "SoundWire Intel Master device flags (0x0 all off)"); + /* Intel SHIM Registers Definition */ #define SDW_SHIM_LCAP 0x0 #define SDW_SHIM_LCTL 0x4 @@ -807,10 +823,17 @@ static int intel_post_bank_switch(struct sdw_bus *bus) static int intel_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { - /* - * TODO: add pm_runtime support here, the startup callback - * will make sure the IP is 'active' - */ + struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); + int ret; + + ret = pm_runtime_get_sync(cdns->dev); + if (ret < 0 && ret != -EACCES) { + dev_err_ratelimited(cdns->dev, + "pm_runtime_get_sync failed in %s, ret %d\n", + __func__, ret); + pm_runtime_put_noidle(cdns->dev); + return ret; + } return 0; }
@@ -985,7 +1008,10 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) static void intel_shutdown(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { + struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
+ pm_runtime_mark_last_busy(cdns->dev); + pm_runtime_put_autosuspend(cdns->dev); }
static int intel_pcm_set_sdw_stream(struct snd_soc_dai *dai, @@ -1269,6 +1295,7 @@ int intel_master_startup(struct platform_device *pdev) struct sdw_cdns *cdns = dev_get_drvdata(dev); struct sdw_intel *sdw = cdns_to_intel(cdns); struct sdw_bus *bus = &cdns->bus; + int link_flags; int ret;
if (bus->prop.hw_disabled) { @@ -1313,6 +1340,18 @@ int intel_master_startup(struct platform_device *pdev)
intel_debugfs_init(sdw);
+ /* Enable runtime PM */ + link_flags = md_flags >> (bus->link_id * 8); + if (!(link_flags & SDW_INTEL_MASTER_DISABLE_PM_RUNTIME)) { + pm_runtime_set_autosuspend_delay(dev, + INTEL_MASTER_SUSPEND_DELAY_MS); + pm_runtime_use_autosuspend(dev); + pm_runtime_mark_last_busy(dev); + + pm_runtime_set_active(dev); + pm_runtime_enable(dev); + } + return 0;
err_interrupt: @@ -1411,6 +1450,36 @@ static int intel_suspend(struct device *dev) return 0; }
+static int intel_suspend_runtime(struct device *dev) +{ + struct sdw_cdns *cdns = dev_get_drvdata(dev); + struct sdw_intel *sdw = cdns_to_intel(cdns); + struct sdw_bus *bus = &cdns->bus; + int ret; + + if (bus->prop.hw_disabled) { + dev_dbg(dev, "SoundWire master %d is disabled, ignoring\n", + bus->link_id); + return 0; + } + + 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, false); + + return 0; +} + static int intel_resume(struct device *dev) { struct sdw_cdns *cdns = dev_get_drvdata(dev); @@ -1445,10 +1514,45 @@ static int intel_resume(struct device *dev) return ret; }
+static int intel_resume_runtime(struct device *dev) +{ + struct sdw_cdns *cdns = dev_get_drvdata(dev); + struct sdw_intel *sdw = cdns_to_intel(cdns); + struct sdw_bus *bus = &cdns->bus; + int ret; + + if (bus->prop.hw_disabled) { + dev_dbg(dev, "SoundWire master %d is disabled, ignoring\n", + bus->link_id); + return 0; + } + + ret = intel_init(sdw); + if (ret) { + dev_err(dev, "%s failed: %d", __func__, ret); + return ret; + } + + ret = sdw_cdns_enable_interrupt(cdns, true); + if (ret < 0) { + dev_err(dev, "cannot enable interrupts during resume\n"); + return ret; + } + + ret = sdw_cdns_exit_reset(cdns); + if (ret < 0) { + dev_err(dev, "unable to exit bus reset sequence during resume\n"); + return ret; + } + + return ret; +} + #endif
static const struct dev_pm_ops intel_pm = { SET_SYSTEM_SLEEP_PM_OPS(intel_suspend, intel_resume) + SET_RUNTIME_PM_OPS(intel_suspend_runtime, intel_resume_runtime, NULL) };
static struct platform_driver sdw_intel_drv = { diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index 047252a91c9e..6dacfec55d50 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -68,8 +68,10 @@ static int sdw_intel_cleanup(struct sdw_intel_ctx *ctx) if (!(link_mask & BIT(i))) continue;
- if (link->pdev) + if (link->pdev) { + pm_runtime_disable(&link->pdev->dev); platform_device_unregister(link->pdev); + } }
return 0;
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
The system resume does the entire bus re-initialization and brings it to full-power. If the device was pm_runtime suspended, there is no need to run the pm_runtime resume sequence after the system runtime.
Follow the documentation from runtime_pm.rst, and conditionally disable, set_active and re-enable the device on system resume.
Note that pm_runtime_suspended() is used instead of pm_runtime_status_suspended() so that we can deal with the case where pm_runtime is disabled.
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 | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index ed7163ae5f7a..284e5c9d240a 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1433,6 +1433,14 @@ static int intel_suspend(struct device *dev) return 0; }
+ if (pm_runtime_suspended(dev)) { + dev_dbg(dev, + "%s: pm_runtime status: suspended\n", + __func__); + + return 0; + } + ret = sdw_cdns_enable_interrupt(cdns, false); if (ret < 0) { dev_err(dev, "cannot disable interrupts on suspend\n"); @@ -1493,6 +1501,18 @@ static int intel_resume(struct device *dev) return 0; }
+ if (pm_runtime_suspended(dev)) { + dev_dbg(dev, + "%s: pm_runtime status was suspended, forcing active\n", + __func__); + + /* follow required sequence from runtime_pm.rst */ + pm_runtime_disable(dev); + pm_runtime_set_active(dev); + pm_runtime_mark_last_busy(dev); + pm_runtime_enable(dev); + } + ret = intel_init(sdw); if (ret) { dev_err(dev, "%s failed: %d", __func__, ret);
On 22-07-20, 04:37, Bard Liao wrote:
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
The system resume does the entire bus re-initialization and brings it to full-power. If the device was pm_runtime suspended, there is no need to run the pm_runtime resume sequence after the system runtime.
Follow the documentation from runtime_pm.rst, and conditionally disable, set_active and re-enable the device on system resume.
Note that pm_runtime_suspended() is used instead of pm_runtime_status_suspended() so that we can deal with the case where pm_runtime is disabled.
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 | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index ed7163ae5f7a..284e5c9d240a 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1433,6 +1433,14 @@ static int intel_suspend(struct device *dev) return 0; }
- if (pm_runtime_suspended(dev)) {
dev_dbg(dev,"%s: pm_runtime status: suspended\n",__func__);
first, can we have this in a single line, or better drop it second why would this be called when device is suspended
return 0;- }
- ret = sdw_cdns_enable_interrupt(cdns, false); if (ret < 0) { dev_err(dev, "cannot disable interrupts on suspend\n");
@@ -1493,6 +1501,18 @@ static int intel_resume(struct device *dev) return 0; }
- if (pm_runtime_suspended(dev)) {
dev_dbg(dev,"%s: pm_runtime status was suspended, forcing active\n",__func__);
this one also could look better in a single line
/* follow required sequence from runtime_pm.rst */pm_runtime_disable(dev);pm_runtime_set_active(dev);pm_runtime_mark_last_busy(dev);pm_runtime_enable(dev);- }
- ret = intel_init(sdw); if (ret) { dev_err(dev, "%s failed: %d", __func__, ret);
-- 2.17.1
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Previous patches took care of the case where the master device is pm_runtime 'suspended' when a system suspend occurs.
In the case where the master device was not suspended, e.g. if suspend occurred while streaming audio, Intel validation noticed a race condition: the pm_runtime suspend may conflict with the enumeration started by the system resume.
This can be simply fixed by updating the status before exiting system resume.
GitHub issue: https://github.com/thesofproject/linux/issues/1482 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 | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 284e5c9d240a..b7d8ea6b331e 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1531,6 +1531,18 @@ static int intel_resume(struct device *dev) return ret; }
+ /* + * after system resume, the pm_runtime suspend() may kick in + * during the enumeration, before any children device force the + * master device to remain active. Using pm_runtime_get() + * routines is not really possible, since it'd prevent the + * master from suspending. + * A reasonable compromise is to update the pm_runtime + * counters and delay the pm_runtime suspend by several + * seconds, by when all enumeration should be complete. + */ + pm_runtime_mark_last_busy(dev); + return ret; }
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
This helps make sure they are all UNATTACHED and reset the state machines.
At the moment we perform a bus reset both for system resume and pm_runtime resume, this will be modified when clock-stop mode is supported
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 | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index b7d8ea6b331e..758387e345da 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1519,6 +1519,12 @@ static int intel_resume(struct device *dev) 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"); @@ -1565,6 +1571,12 @@ static int intel_resume_runtime(struct device *dev) 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");
The .prepare() callback is invoked for normal streaming, underflows or during the system resume transition. In the latter case, the context for the ALH PDIs is lost, and the DSP is not initialized properly either, but the bus parameters don't need to be recomputed.
Conversely, when doing a regular .prepare() during an underflow, the ALH/SHIM registers shall not be changed as the hardware cannot be reprogrammed after the DMA started (hardware spec requirement).
This patch adds storage of PDI and hw_params in the DAI dma context, and the difference between the types of .prepare() usages is handled via a simple boolean, updated when suspending, and tested for in the .prepare() case.
Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.h | 4 ++ drivers/soundwire/intel.c | 71 +++++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index 7638858397df..fdec62b912d3 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -84,6 +84,8 @@ struct sdw_cdns_stream_config { * @bus: Bus handle * @stream_type: Stream type * @link_id: Master link id + * @hw_params: hw_params to be applied in .prepare step + * @suspended: status set when suspended, to be used in .prepare */ struct sdw_cdns_dma_data { char *name; @@ -92,6 +94,8 @@ struct sdw_cdns_dma_data { struct sdw_bus *bus; enum sdw_stream_type stream_type; int link_id; + struct snd_pcm_hw_params *hw_params; + bool suspended; };
/** diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 758387e345da..52a411669ec0 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -879,6 +879,10 @@ static int intel_hw_params(struct snd_pcm_substream *substream, intel_pdi_alh_configure(sdw, pdi); sdw_cdns_config_stream(cdns, ch, dir, pdi);
+ /* store pdi and hw_params, may be needed in prepare step */ + dma->suspended = false; + dma->pdi = pdi; + dma->hw_params = params;
/* Inform DSP about PDI stream number */ ret = intel_params_stream(sdw, substream, dai, params, @@ -922,7 +926,11 @@ static int intel_hw_params(struct snd_pcm_substream *substream, static int intel_prepare(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { + struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); + struct sdw_intel *sdw = cdns_to_intel(cdns); struct sdw_cdns_dma_data *dma; + int ch, dir; + int ret;
dma = snd_soc_dai_get_dma_data(dai, substream); if (!dma) { @@ -931,7 +939,41 @@ static int intel_prepare(struct snd_pcm_substream *substream, return -EIO; }
- return sdw_prepare_stream(dma->stream); + if (dma->suspended) { + dma->suspended = false; + + /* + * .prepare() is called after system resume, where we + * need to reinitialize the SHIM/ALH/Cadence IP. + * .prepare() is also called to deal with underflows, + * but in those cases we cannot touch ALH/SHIM + * registers + */ + + /* configure stream */ + ch = params_channels(dma->hw_params); + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) + dir = SDW_DATA_DIR_RX; + else + dir = SDW_DATA_DIR_TX; + + intel_pdi_shim_configure(sdw, dma->pdi); + intel_pdi_alh_configure(sdw, dma->pdi); + sdw_cdns_config_stream(cdns, ch, dir, dma->pdi); + + /* Inform DSP about PDI stream number */ + ret = intel_params_stream(sdw, substream, dai, + dma->hw_params, + sdw->instance, + dma->pdi->intel_alh_id); + if (ret) + goto err; + } + + ret = sdw_prepare_stream(dma->stream); + +err: + return ret; }
static int intel_trigger(struct snd_pcm_substream *substream, int cmd, @@ -1002,6 +1044,9 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) return ret; }
+ dma->hw_params = NULL; + dma->pdi = NULL; + return 0; }
@@ -1014,6 +1059,29 @@ static void intel_shutdown(struct snd_pcm_substream *substream, pm_runtime_put_autosuspend(cdns->dev); }
+static int intel_component_dais_suspend(struct snd_soc_component *component) +{ + struct sdw_cdns_dma_data *dma; + struct snd_soc_dai *dai; + + for_each_component_dais(component, dai) { + /* + * we don't have a .suspend dai_ops, and we don't have access + * to the substream, so let's mark both capture and playback + * DMA contexts as suspended + */ + dma = dai->playback_dma_data; + if (dma) + dma->suspended = true; + + dma = dai->capture_dma_data; + if (dma) + dma->suspended = true; + } + + return 0; +} + static int intel_pcm_set_sdw_stream(struct snd_soc_dai *dai, void *stream, int direction) { @@ -1066,6 +1134,7 @@ static const struct snd_soc_dai_ops intel_pdm_dai_ops = {
static const struct snd_soc_component_driver dai_component = { .name = "soundwire", + .suspend = intel_component_dais_suspend };
static int intel_create_dai(struct sdw_cdns *cdns,
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Add quirk and pm_runtime idle scheduling to let the Master suspend if no Slaves become attached. This can happen when a link is not marked as disabled and has devices exposed in the DSDT, if the power is controlled by sideband means or the link includes a pluggable connector.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Rander Wang rander.wang@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/intel.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 52a411669ec0..4a60456f060d 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -31,8 +31,9 @@ * flags reused in each byte, with master0 using the ls-byte, etc. */
-#define SDW_INTEL_MASTER_DISABLE_PM_RUNTIME BIT(0) -#define SDW_INTEL_MASTER_DISABLE_CLOCK_STOP BIT(1) +#define SDW_INTEL_MASTER_DISABLE_PM_RUNTIME BIT(0) +#define SDW_INTEL_MASTER_DISABLE_CLOCK_STOP BIT(1) +#define SDW_INTEL_MASTER_DISABLE_PM_RUNTIME_IDLE BIT(2)
static int md_flags; module_param_named(sdw_md_flags, md_flags, int, 0444); @@ -1421,6 +1422,22 @@ int intel_master_startup(struct platform_device *pdev) pm_runtime_enable(dev); }
+ /* + * The runtime PM status of Slave devices is "Unsupported" + * until they report as ATTACHED. If they don't, e.g. because + * there are no Slave devices populated or if the power-on is + * delayed or dependent on a power switch, the Master will + * remain active and prevent its parent from suspending. + * + * Conditionally force the pm_runtime core to re-evaluate the + * Master status in the absence of any Slave activity. A quirk + * is provided to e.g. deal with Slaves that may be powered on + * with a delay. A more complete solution would require the + * definition of Master properties. + */ + if (!(link_flags & SDW_INTEL_MASTER_DISABLE_PM_RUNTIME_IDLE)) + pm_runtime_idle(dev); + return 0;
err_interrupt: @@ -1562,6 +1579,7 @@ static int intel_resume(struct device *dev) struct sdw_cdns *cdns = dev_get_drvdata(dev); struct sdw_intel *sdw = cdns_to_intel(cdns); struct sdw_bus *bus = &cdns->bus; + int link_flags; int ret;
if (bus->prop.hw_disabled) { @@ -1580,6 +1598,10 @@ static int intel_resume(struct device *dev) pm_runtime_set_active(dev); pm_runtime_mark_last_busy(dev); pm_runtime_enable(dev); + + link_flags = md_flags >> (bus->link_id * 8); + if (!(link_flags & SDW_INTEL_MASTER_DISABLE_PM_RUNTIME_IDLE)) + pm_runtime_idle(dev); }
ret = intel_init(sdw);
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Now that we have options, add support for TEARDOWN mode (same functionality as existing code)
All other modes will be added in follow-up patches.
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 | 82 +++++++++++++++++++++------------- drivers/soundwire/intel.h | 2 + drivers/soundwire/intel_init.c | 1 + 3 files changed, 54 insertions(+), 31 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 4a60456f060d..1954eb48b86c 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1549,6 +1549,7 @@ static int intel_suspend_runtime(struct device *dev) struct sdw_cdns *cdns = dev_get_drvdata(dev); struct sdw_intel *sdw = cdns_to_intel(cdns); struct sdw_bus *bus = &cdns->bus; + u32 clock_stop_quirks; int ret;
if (bus->prop.hw_disabled) { @@ -1557,21 +1558,31 @@ static int intel_suspend_runtime(struct device *dev) return 0; }
- ret = sdw_cdns_enable_interrupt(cdns, false); - if (ret < 0) { - dev_err(dev, "cannot disable interrupts on suspend\n"); - return ret; - } + clock_stop_quirks = sdw->link_res->clock_stop_quirks;
- ret = intel_link_power_down(sdw); - if (ret) { - dev_err(dev, "Link power down failed: %d", ret); - return ret; - } + if (clock_stop_quirks & SDW_INTEL_CLK_STOP_TEARDOWN) { + + ret = sdw_cdns_enable_interrupt(cdns, false); + if (ret < 0) { + dev_err(dev, "cannot disable interrupts on suspend\n"); + return ret; + }
- intel_shim_wake(sdw, false); + ret = intel_link_power_down(sdw); + if (ret) { + dev_err(dev, "Link power down failed: %d", ret); + return ret; + } + + intel_shim_wake(sdw, false); + + } else { + dev_err(dev, "%s clock_stop_quirks %x unsupported\n", + __func__, clock_stop_quirks); + ret = -EINVAL; + }
- return 0; + return ret; }
static int intel_resume(struct device *dev) @@ -1648,6 +1659,7 @@ static int intel_resume_runtime(struct device *dev) struct sdw_cdns *cdns = dev_get_drvdata(dev); struct sdw_intel *sdw = cdns_to_intel(cdns); struct sdw_bus *bus = &cdns->bus; + u32 clock_stop_quirks; int ret;
if (bus->prop.hw_disabled) { @@ -1656,28 +1668,36 @@ static int intel_resume_runtime(struct device *dev) return 0; }
- ret = intel_init(sdw); - if (ret) { - dev_err(dev, "%s failed: %d", __func__, ret); - return ret; - } + clock_stop_quirks = sdw->link_res->clock_stop_quirks;
- /* - * make sure all Slaves are tagged as UNATTACHED and provide - * reason for reinitialization - */ - sdw_clear_slave_status(bus, SDW_UNATTACH_REQUEST_MASTER_RESET); + if (clock_stop_quirks & SDW_INTEL_CLK_STOP_TEARDOWN) { + ret = intel_init(sdw); + if (ret) { + dev_err(dev, "%s failed: %d", __func__, ret); + return ret; + }
- ret = sdw_cdns_enable_interrupt(cdns, true); - if (ret < 0) { - dev_err(dev, "cannot enable interrupts during resume\n"); - 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_exit_reset(cdns); - if (ret < 0) { - dev_err(dev, "unable to exit bus reset sequence during resume\n"); - return ret; + ret = sdw_cdns_enable_interrupt(cdns, true); + if (ret < 0) { + dev_err(dev, "cannot enable interrupts during resume\n"); + return ret; + } + + ret = sdw_cdns_exit_reset(cdns); + if (ret < 0) { + dev_err(dev, "unable to exit bus reset sequence during resume\n"); + return ret; + } + } else { + dev_err(dev, "%s clock_stop_quirks %x unsupported\n", + __func__, clock_stop_quirks); + ret = -EINVAL; }
return ret; diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h index 4ea3d262d249..23daab9da329 100644 --- a/drivers/soundwire/intel.h +++ b/drivers/soundwire/intel.h @@ -17,6 +17,7 @@ * @dev: device implementing hw_params and free callbacks * @shim_lock: mutex to handle access to shared SHIM registers * @shim_mask: global pointer to check SHIM register initialization + * @clock_stop_quirks: mask defining requested behavior on pm_suspend * @cdns: Cadence master descriptor * @list: used to walk-through all masters exposed by the same controller */ @@ -31,6 +32,7 @@ struct sdw_intel_link_res { struct device *dev; struct mutex *shim_lock; /* protect shared registers */ u32 *shim_mask; + u32 clock_stop_quirks; struct sdw_cdns *cdns; struct list_head list; }; diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index 6dacfec55d50..96c9af15c308 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -248,6 +248,7 @@ static struct sdw_intel_ctx link->ops = res->ops; link->dev = res->dev;
+ link->clock_stop_quirks = res->clock_stop_quirks; link->shim_lock = &ctx->shim_lock; link->shim_mask = &ctx->shim_mask;
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; + } + + 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);
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);@@ -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;}-- 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
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
In case the clock needs to keep running, we need to prevent the Master from entering pm_runtime suspend.
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 | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 744fc0a4816a..c9d8de65cfd6 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1366,6 +1366,7 @@ int intel_master_startup(struct platform_device *pdev) struct sdw_intel *sdw = cdns_to_intel(cdns); struct sdw_bus *bus = &cdns->bus; int link_flags; + u32 clock_stop_quirks; int ret;
if (bus->prop.hw_disabled) { @@ -1422,6 +1423,20 @@ int intel_master_startup(struct platform_device *pdev) pm_runtime_enable(dev); }
+ clock_stop_quirks = sdw->link_res->clock_stop_quirks; + if (clock_stop_quirks & SDW_INTEL_CLK_STOP_NOT_ALLOWED) { + /* + * To keep the clock running we need to prevent + * pm_runtime suspend from happening by increasing the + * reference count. + * This quirk is specified by the parent PCI device in + * case of specific latency requirements. It will have + * no effect if pm_runtime is disabled by the user via + * a module parameter for testing purposes. + */ + pm_runtime_get_noresume(dev); + } + /* * The runtime PM status of Slave devices is "Unsupported" * until they report as ATTACHED. If they don't, e.g. because @@ -1453,6 +1468,11 @@ static int intel_master_remove(struct platform_device *pdev) struct sdw_intel *sdw = cdns_to_intel(cdns); struct sdw_bus *bus = &cdns->bus;
+ /* + * Since pm_runtime is already disabled, we don't decrease + * the refcount when the clock_stop_quirk is + * SDW_INTEL_CLK_STOP_NOT_ALLOWED + */ if (!bus->prop.hw_disabled) { intel_debugfs_exit(sdw); sdw_cdns_enable_interrupt(cdns, false);
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
When none of the clock stop quirks is specified, the Master IP will assume the context is preserved and will not reset the Bus and restart enumeration. Due to power rail dependencies, the HDaudio controller needs to remain powered and prevented from executing its pm_runtime suspend routine.
This choice of course has a power impact, and this mode should only be selected when latency requirements are critical or the parent device can enter D0ix modes.
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_init.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index 96c9af15c308..c0fddf76a6dc 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -14,6 +14,7 @@ #include <linux/module.h> #include <linux/platform_device.h> #include <linux/soundwire/sdw_intel.h> +#include <linux/pm_runtime.h> #include "cadence_master.h" #include "intel.h"
@@ -72,6 +73,9 @@ static int sdw_intel_cleanup(struct sdw_intel_ctx *ctx) pm_runtime_disable(&link->pdev->dev); platform_device_unregister(link->pdev); } + + if (!link->clock_stop_quirks) + pm_runtime_put_noidle(link->dev); }
return 0; @@ -337,6 +341,16 @@ sdw_intel_startup_controller(struct sdw_intel_ctx *ctx) continue;
intel_master_startup(link->pdev); + + if (!link->clock_stop_quirks) { + /* + * we need to prevent the parent PCI device + * from entering pm_runtime suspend, so that + * power rails to the SoundWire IP are not + * turned off. + */ + pm_runtime_get_noresume(link->dev); + } }
return 0;
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
In this mode, on restart the bus restarts immediately, the Slaves remain synchronized and all context is kept intact.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Rander Wang rander.wang@intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/intel.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index c9d8de65cfd6..22b3359855c5 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1596,7 +1596,8 @@ static int intel_suspend_runtime(struct device *dev)
intel_shim_wake(sdw, false);
- } else if (clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET) { + } else if (clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET || + !clock_stop_quirks) { ret = sdw_cdns_clock_stop(cdns, true); if (ret < 0) { dev_err(dev, "cannot enable clock stop on suspend\n"); @@ -1758,6 +1759,24 @@ static int intel_resume_runtime(struct device *dev) dev_err(dev, "unable to restart clock during resume\n"); return ret; } + } else if (!clock_stop_quirks) { + ret = intel_init(sdw); + if (ret) { + dev_err(dev, "%s failed: %d", __func__, ret); + return ret; + } + + 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, false); + if (ret < 0) { + dev_err(dev, "unable to resume master during resume\n"); + return ret; + } } else { dev_err(dev, "%s clock_stop_quirks %x unsupported\n", __func__, clock_stop_quirks);
From: Rander Wang rander.wang@intel.com
When all the links are suspended, the HDaudio controller may suspend and the power rails to the SoundWire IP may be disabled, requiring a complete re-initialization/enumeration on resume. However, if one or more Masters remained active, the HDaudio controller will remain active and the power rails will remain enabled. As a result, during the link resume step we can check if the context was preserved by verifying if the clock was stopped, and avoid doing a complete bus reset and re-enumeration.
Signed-off-by: Rander Wang rander.wang@intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/intel.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 22b3359855c5..da279b175848 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1701,6 +1701,8 @@ static int intel_resume_runtime(struct device *dev) struct sdw_intel *sdw = cdns_to_intel(cdns); struct sdw_bus *bus = &cdns->bus; u32 clock_stop_quirks; + bool clock_stop0; + int status; int ret;
if (bus->prop.hw_disabled) { @@ -1742,11 +1744,24 @@ static int intel_resume_runtime(struct device *dev) return ret; }
+ /* + * An exception condition occurs for the CLK_STOP_BUS_RESET + * case if one or more masters remain active. In this condition, + * all the masters are powered on for they are in the same power + * domain. Master can preserve its context for clock stop0, so + * there is no need to clear slave status and reset bus. + */ + clock_stop0 = sdw_cdns_is_clock_stop(&sdw->cdns); + /* * make sure all Slaves are tagged as UNATTACHED and * provide reason for reinitialization */ - sdw_clear_slave_status(bus, SDW_UNATTACH_REQUEST_MASTER_RESET); + if (!clock_stop0) { + status = SDW_UNATTACH_REQUEST_MASTER_RESET; + sdw_clear_slave_status(bus, status); + } +
ret = sdw_cdns_enable_interrupt(cdns, true); if (ret < 0) { @@ -1754,7 +1769,7 @@ static int intel_resume_runtime(struct device *dev) return ret; }
- ret = sdw_cdns_clock_restart(cdns, true); + ret = sdw_cdns_clock_restart(cdns, !clock_stop0); if (ret < 0) { dev_err(dev, "unable to restart clock during resume\n"); return ret;
On 22-07-20, 04:37, Bard Liao wrote:
This series adds power management support for Intel soundwire links.
I had applied except 3 & 9 (few skipped in middle due to conflict while applying), BUT I get a build failure on patch 2 onwards :(
drivers/soundwire/intel_init.c: In function ‘sdw_intel_cleanup’: drivers/soundwire/intel_init.c:72:4: error: implicit declaration of function ‘pm_runtime_disable’ [-Werror=implicit-function-declaration] 72 | pm_runtime_disable(&link->pdev->dev);
I suspect due to missing header? I was on x64 build with allmodconfig
So only patch 1 is applied and pushed now
On 8/17/20 7:08 AM, Vinod Koul wrote:
On 22-07-20, 04:37, Bard Liao wrote:
This series adds power management support for Intel soundwire links.
I had applied except 3 & 9 (few skipped in middle due to conflict while applying), BUT I get a build failure on patch 2 onwards :(
drivers/soundwire/intel_init.c: In function ‘sdw_intel_cleanup’: drivers/soundwire/intel_init.c:72:4: error: implicit declaration of function ‘pm_runtime_disable’ [-Werror=implicit-function-declaration] 72 | pm_runtime_disable(&link->pdev->dev);
I suspect due to missing header? I was on x64 build with allmodconfig
So only patch 1 is applied and pushed now
I just tried on these series applied on top of soundwire/next
commit 9b3b4b3f2f2af863d2f6dd65afd295a5a673afa2 (soundwire/next)
soundwire: intel: Add basic power management support
And I don't see any issue?
If you want to double-check merge issues, I pushed the code here: https://github.com/plbossart/sound/tree/sdw/pm_runtime_soundwire_next
I am really not sure what conflicts you are referring to, git am worked fine for me, only skipped the first patch that's already applied.
$git am ~/Downloads/alsa/122/[PATCH\ * Applying: soundwire: intel: Add basic power management support error: patch failed: drivers/soundwire/intel.c:463 error: drivers/soundwire/intel.c: patch does not apply Patch failed at 0001 soundwire: intel: Add basic power management support hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". $ git am --skip Applying: soundwire: intel: add pm_runtime support Applying: soundwire: intel: reset pm_runtime status during system resume Applying: soundwire: intel: fix race condition on system resume Applying: soundwire: intel: call helper to reset Slave states on resume Applying: soundwire: intel: reinitialize IP+DSP in .prepare(), but only when resuming Applying: soundwire: intel: pm_runtime idle scheduling Applying: soundwire: intel: add CLK_STOP_TEARDOWN for pm_runtime suspend Applying: soundwire: intel: add CLK_STOP_BUS_RESET support Applying: soundwire: intel: add CLK_STOP_NOT_ALLOWED support Applying: soundwire: intel_init: handle power rail dependencies for clock stop mode Applying: soundwire: intel: support clock_stop mode without quirks Applying: soundwire: intel: refine runtime pm for SDW_INTEL_CLK_STOP_BUS_RESET
I tried the compilation twice, once with our default SOF config and once with allmodconfig.
make drivers/soundwire/ W=1 GEN Makefile YACC scripts/genksyms/parse.tab.[ch] /data/pbossart/ktest/broonie-next/scripts/genksyms/parse.y: warning: 9 shift/reduce conflicts [-Wconflicts-sr] /data/pbossart/ktest/broonie-next/scripts/genksyms/parse.y: warning: 5 reduce/reduce conflicts [-Wconflicts-rr] HOSTCC scripts/genksyms/parse.tab.o HOSTCC scripts/genksyms/lex.lex.o HOSTLD scripts/genksyms/genksyms CC scripts/mod/empty.o MKELF scripts/mod/elfconfig.h HOSTCC scripts/mod/modpost.o CC scripts/mod/devicetable-offsets.s HOSTCC scripts/mod/file2alias.o HOSTCC scripts/mod/sumversion.o HOSTLD scripts/mod/modpost CC kernel/bounds.s CC arch/x86/kernel/asm-offsets.s CALL /data/pbossart/ktest/broonie-next/scripts/checksyscalls.sh CALL /data/pbossart/ktest/broonie-next/scripts/atomic/check-atomics.sh DESCEND objtool CC [M] drivers/soundwire/bus_type.o CC [M] drivers/soundwire/bus.o CC [M] drivers/soundwire/master.o CC [M] drivers/soundwire/slave.o CC [M] drivers/soundwire/mipi_disco.o CC [M] drivers/soundwire/stream.o CC [M] drivers/soundwire/sysfs_slave.o CC [M] drivers/soundwire/sysfs_slave_dpn.o CC [M] drivers/soundwire/debugfs.o LD [M] drivers/soundwire/soundwire-bus.o CC [M] drivers/soundwire/cadence_master.o LD [M] drivers/soundwire/soundwire-cadence.o CC [M] drivers/soundwire/intel.o CC [M] drivers/soundwire/intel_init.o LD [M] drivers/soundwire/soundwire-intel.o CC [M] drivers/soundwire/qcom.o LD [M] drivers/soundwire/soundwire-qcom.o
I had applied except 3 & 9 (few skipped in middle due to conflict while applying), BUT I get a build failure on patch 2 onwards :(
drivers/soundwire/intel_init.c: In function ‘sdw_intel_cleanup’: drivers/soundwire/intel_init.c:72:4: error: implicit declaration of function ‘pm_runtime_disable’ [-Werror=implicit-function-declaration] 72 | pm_runtime_disable(&link->pdev->dev);
I suspect due to missing header? I was on x64 build with allmodconfig
So only patch 1 is applied and pushed now
I just tried on these series applied on top of soundwire/next
commit 9b3b4b3f2f2af863d2f6dd65afd295a5a673afa2 (soundwire/next)
soundwire: intel: Add basic power management support
And I don't see any issue?
Sorry, I misunderstood the issue. Yes indeed the #include <linux/pm_runtime.h> is added to the wrong patch, I see Bard fixed this in our tree. Not sure what happened here, I ran a patch-by-patch compilation test a long time ago and kbuild was silent. Thanks for spotting this.
If you want to double-check merge issues, I pushed the code here: https://github.com/plbossart/sound/tree/sdw/pm_runtime_soundwire_next
I am really not sure what conflicts you are referring to, git am worked fine for me, only skipped the first patch that's already applied.
But the point about conflicts does remain, I am not sure why you skipped patches, I have no merge conflicts on my side.
On 17-08-20, 11:10, Pierre-Louis Bossart wrote:
I had applied except 3 & 9 (few skipped in middle due to conflict while applying), BUT I get a build failure on patch 2 onwards :(
drivers/soundwire/intel_init.c: In function ‘sdw_intel_cleanup’: drivers/soundwire/intel_init.c:72:4: error: implicit declaration of function ‘pm_runtime_disable’ [-Werror=implicit-function-declaration] 72 | pm_runtime_disable(&link->pdev->dev);
I suspect due to missing header? I was on x64 build with allmodconfig
So only patch 1 is applied and pushed now
I just tried on these series applied on top of soundwire/next
commit 9b3b4b3f2f2af863d2f6dd65afd295a5a673afa2 (soundwire/next)
soundwire: intel: Add basic power management support
And I don't see any issue?
Sorry, I misunderstood the issue. Yes indeed the #include <linux/pm_runtime.h> is added to the wrong patch, I see Bard fixed this in our tree. Not sure what happened here, I ran a patch-by-patch compilation test a long time ago and kbuild was silent. Thanks for spotting this.
If you want to double-check merge issues, I pushed the code here: https://github.com/plbossart/sound/tree/sdw/pm_runtime_soundwire_next
I am really not sure what conflicts you are referring to, git am worked fine for me, only skipped the first patch that's already applied.
But the point about conflicts does remain, I am not sure why you skipped patches, I have no merge conflicts on my side.
As noted above, I tried to apply except 3 & 9 due to questions on them. It is quite normal that dependencies fail to apply, not sure why you are confused.
participants (3)
-
Bard Liao -
Pierre-Louis Bossart -
Vinod Koul