ASoC: SOF: Intel: BYT: mask BUSY or DONE interrupts in handler #2136
Conversation
The DSP may send the same interrupt multiple times before it's handled in the interrupt thread. Rather than masking it in the thread, mask it in the handler directly. This patch also removes useless checks that cannot happen, and masks that are set don't need to be re-tested. BugLink: thesofproject#1492 Suggested-by: Keyon Jie <yang.jie@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
plbossart
requested review from
dbaluta,
kv2019i,
lgirdwood and
ranj063
as code owners
|
replaced by PR #2138 |
| snd_sof_dsp_update_bits64_unlocked(sdev, BYT_DSP_BAR, | ||
| SHIM_IMRX, | ||
| SHIM_IMRX_DONE, | ||
| SHIM_IMRX_DONE); |
There was a problem hiding this comment.
If you don't check the SHIM_ISRX here, we could mask the SHIM_IMRX_DONE multiple times if a new message from DSP arrives and the previous reply message from DSP has not been thread handled yet, though this could be not harmful.
There was a problem hiding this comment.
SHIM_ISRX_DONE is set when LPE writes 1 in IPCX_DONE, so this is redundant/mirrorred. You cannot have different values for SHIM_ISRX_DONE and IPCX_DONE.
There was a problem hiding this comment.
Yes, SHIM_ISRX_DONE and IPCX_DONE are set in mirrored way, but the clearing doesn't. I believe they actually could have different values, imagine this:
- a new host -> LPE IPC A;
- LPE handled A, and send reply for A to host. This will set ISRX_DONE and IPCX_DONE;
- Host get ISRX_DONE interrupt, unmask IMRX_DONE here and waking up the IRQ tread handler, but at this time, if
- LPE want to send a new IPC (e.g. tracing position update IPC) to the host, it will set ISRX_BUSY and IPCD_BUSY;
- Host get ISRX_BUSY interrupt, we will enter byt_irq_handler() here again but the clearing of IPCX_DONE after step 3 above (doing in the thread byt_irq_thread) may not be done yet, in this scenario, the check of "if (ipcx & SHIM_BYT_IPCX_DONE)" will be true and we will mask the SHIM_IMRX_DONE again.
There was a problem hiding this comment.
we never clear SHIM_ISRX_DONE and masking does not affect the values of IPXC_DONE registers, I don't understand your point at all.
Is anything broken?
There was a problem hiding this comment.
If you add some print log as I did in #1492, and compile FW with verbose trace enabled, then running a stream, and you could see that if what I described above will happen.
There was a problem hiding this comment.
we never clear SHIM_ISRX_DONE and masking does not affect the values of IPXC_DONE registers, I don't understand your point at all.
Is anything broken?
As I commented at the beginning, this could be not harmful so nothing should be broken by it.
There was a problem hiding this comment.
@keyonjie adding a test for ISRX_DONE would not change anything in step 5, the bit would be set just like IPCX_DONE, so in this case there is no way to prevent the IMRX_DONE bit from being set.
The DSP may send the same interrupt multiple times before it's handled
in the interrupt thread. Rather than masking it in the thread, mask it
in the handler directly.
This patch also removes useless checks that cannot happen, and masks
that are set don't need to be re-tested.
BugLink: #1492
Suggested-by: Keyon Jie yang.jie@linux.intel.com
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com