Repository navigation
Pr4838 debug - #4845
Pr4838 debug#4845plbossart wants to merge 6 commits into
Conversation
We need to wait for each child to fully resume. Use pm_resume_resume to ensure it. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
We will resume each child in the next patch, and intel_resume_child_device() will be used. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
We will disable SDW interrupt in hda_sdw_exit() before sdw_intel_exit() and peripherals will on longer access SDW registers. However, when the manager became pm_runtime active in the remove procedure, peripherals will become attached, and do the initialization process. The commit suggests a sdw_intel_pre_exit() function and use pm_runtime_resume() to wait for the peripherals being fully resumed. This will ensure there are no SoundWire registers accessed after the bus is powered-down. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
|
@bardliao would you happen to remember why we do this pm_runtime_disable() ? static int sdw_intel_cleanup(struct sdw_intel_ctx *ctx)
{
struct sdw_intel_link_dev *ldev;
u32 link_mask;
int i;
link_mask = ctx->link_mask;
for (i = 0; i < ctx->count; i++) {
if (!(link_mask & BIT(i)))
continue;
ldev = ctx->ldev[i];
pm_runtime_disable(&ldev->auxdev.dev);
if (!ldev->link_res.clock_stop_quirks)
pm_runtime_put_noidle(ldev->link_res.dev);
intel_link_dev_unregister(ldev);
}
return 0;
}This is potentially problematic... I think the intent was to prevent the device from suspending, but it could very well be suspended already, in which case pm_runtime_disable() could prevent the full resume that can happen if there is an interrupt. This looks as old as the platform device stuff, but I can't figure out why it was ever needed. |
Hmm, I can't remember why we did this. But we do the same in sdw_master_device_del() and sdw_delete_slave() |
The SoundWire interrupts can be masked at two levels a) in the Cadence IP b) at the HDaudio controller level We have an existing mechanism with cancel_work_sync() and status flags to make sure all existing interrupts are handled in the Cadence IP, and likewise no new interrupts can be generated before turning off the links. However on remove we first use the higher-level mask at the controller level, which is a sledgehammer preventing interrupts from all links. This is very racy and not necessary. We can disable the SoundWire interrupts after all the cleanups are done without any loss of functionality. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The last cases are different. You want to prevent suspend in the remove case. It's all happening in the context of the same device. But the lines above disable pm_runtime() for child devices. This is a layering violation and potentially dangerous because the child device cannot resume any longer... |
|
SOFCI TEST |
|
ok, we're back to one timeout in suspend-resume but looks good otherwise. Phew. |
|
simpler version suggested in #4849 |
Do you mean https://sof-ci.01.org/linuxpr/PR4849/build1698/devicetest/index.html? I can't reproduce the failure when I tested |
2 participants
No description provided.