Repository navigation
Conversation
Codecs may need to access sdw registers via sdw bus in the exit process. Disabling sdw interrupt will block sdw IO transfer. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Peripherals are not able to access the sdw link after the link device is unregistered. Disable pm runtime to precent the peripheral from being resume. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
| sdw_intel_exit(hdev->sdw); | ||
| hdev->sdw = NULL; | ||
|
|
||
| hda_sdw_int_enable(sdev, false); |
There was a problem hiding this comment.
can we give an example of when "Codecs may need to access sdw registers via sdw bus in the exit process".
I don't fully buy the argument. IIRC the initial code was intentional so that we wouldn't have ANY interrupts while removing the devices.
There was a problem hiding this comment.
can we give an example of when "Codecs may need to access sdw registers via sdw bus in the exit process".
When we run sudo modprobe -r snd_sof_pci_intel_lnl, snd_sof_runtime_resume will be trigger first. And hda_sdw_process_wakeen will be called when dsp boot. Due to intel_shim_check_wake return 1 unconditionally, soundwire_intel.links will be powered up. And the codecs will be attached.
SCP Msg trf timed out will happen if hda_sdw_exit is called before codec is initialized, and SDW interrupt is disabled.
Please see the log in detail.
lnl_remove.txt
I don't fully buy the argument. IIRC the initial code was intentional so that we wouldn't have ANY interrupts while removing the devices.
I agree that we don't want any interrupts while removing the devices. However, the interrupts are also used in sdw commend transfer. Can we cancel the sdw commend transfer before disabling SDW interrupt?
There was a problem hiding this comment.
@plbossart Alternative solution #4838. Instead of canceling the sdw commend, #4838 wait for them be finished.
| */ | ||
| list_for_each_entry(slave, &ldev->link_res.cdns->bus.slaves, node) | ||
| pm_runtime_disable(&slave->dev); | ||
|
|
There was a problem hiding this comment.
is this really needed? We already disable pm_runtime in codec drivers, e.g.
linux/sound/soc/codecs/rt711-sdca-sdw.c
Line 360 in 59c5a8d
There was a problem hiding this comment.
is this really needed? We already disable pm_runtime in codec drivers, e.g.
linux/sound/soc/codecs/rt711-sdca-sdw.c
Line 360 in 59c5a8d
The resume runtime is called before codec drivers being removed. So, disabling it in the .remove function is too late.
There was a problem hiding this comment.
@bardliao alternatively you could do something like we do in the .prepare step:
linux/drivers/soundwire/intel_auxdevice.c
Line 517 in e915e8b
But we would need a pm_runtime_resume() which would wait for the devices to be fully resumed (I don't recall why we didn't do this for .prepare).
There was a problem hiding this comment.
@bardliao alternatively you could do something like we do in the .prepare step:
linux/drivers/soundwire/intel_auxdevice.c
Line 517 in e915e8b
But we would need a pm_runtime_resume() which would wait for the devices to be fully resumed (I don't recall why we didn't do this for .prepare).
Let me study this.
|
" ok, so now I am starting to get the issue. But I guess we already had the issue before the change to power-up the links unconditionally. If a device has a wake event just during the remove we could also hit the same issue. |
Exactly, we will get the issue if a device happen to has a wake event during the remove . |
|
replaced by #4838 |
2 participants
SOF runtime resume will resume the sdw link if a wake is detected by the codec. And we simply assume there is wakes when we check HDadio WAKEEN/WAKESTS. It is harmless. But we will get sdw transfer timed out if the SOF runtime resume is triggered by module unload. That is because codecs will be attached when the sdw link is powered up, and the sdw link will no longer work after the link device is unloaded.
The PR disable the sdw interrupt after sdw_intel_exit() to allow codec driver initialize the codec in the attached state. Also, disable Peripherals' runtime PM to prevent Peripherals from being resume in the link device unregistering process.
Fixes: #4809