ASoC: SOF: Intel: byt: refine the interrupt masks

[keyonjie]

We should only touch host IPC interrupt masks and leave DSP IPC interrupt
ones to FW.

For host IPC interrupt BUSY and DONE masks, control it in this sequence:

BUSY mask: unmask it by default, once got interrupt from DSP, mask it,
unmask it in host_done().

DONE mask: mask it by default, unmask it once a new IPC sent to FW, and
re-mask it in dsp_done().

Signed-off-by: Keyon Jie [email protected]

[keyonjie]

Hi @plbossart Is this one clear enough?

[ranj063]

@keyonjie this one is hard to justify. This is how I understand the doorbell works:

Once you get a new IPC from the DSP, you mask the BUSY interrupt, process the IPC, tell DSP the host is done and unmask BUSY interrupt.

Once you get a reply from DSP, you mask DONE interrupt, process reply, tell DSP we got the reply and unmask DONE interrupt.

Now, can you please explain whats different in this PR and why?

[keyonjie]

@keyonjie this one is hard to justify. This is how I understand the doorbell works:

Once you get a new IPC from the DSP, you mask the BUSY interrupt, process the IPC, tell DSP the host is done and unmask BUSY interrupt.

This one is consistent with my PR.

Once you get a reply from DSP, you mask DONE interrupt, process reply, tell DSP we got the reply and unmask DONE interrupt.

This one is partially different, it doesn't unmask DONE interrupt at the previous DONE interrupt handled, instead, it unmasks the DONE interrupt at the HOST->DSP IPC sent, with this change, we will enable this DONE interrupt as less as possible -- only when we explicitly initiate an IPC to FW, we are expecting the DONE response from FW, which could harden the HOST->DSP IPC processing cycle.

This change is needed during my debugging to BSW hang issue, in some cases, the FW will keep sending this DONE response to host and lead to interrupt storm in host side and then hang the Linux.

This change actually can be copied to cAVS platforms also.

Now, can you please explain whats different in this PR and why?

[fredoh9]

I ran one hour of stress-test. I don't see any regression issue.

[keyonjie]

@fredoh9 Thank you. @plbossart @ranj063 The test on my side looks good on cht and bsw also.

[lgirdwood]

@keyonjie can we have a PR for the kernel that states this rule in the comments too. That way folks working on either kernel or FW IPC will know the rules.

[keyonjie]

@keyonjie can we have a PR for the kernel that states this rule in the comments too. That way folks working on either kernel or FW IPC will know the rules.

@lgirdwood good suggestion, I will add that if the solution is taken by our maintainers.

Hi @plbossart, I am seeing that you marked it as unclear, can you add detail about what is unclear here, if you think the PR has no value, let me know if I should close it, thanks.

[plbossart]

it's just totally unclear what the new settings do.
see e.g.

	/* enable BUSY and disable DONE Interrupt by default */
	snd_sof_dsp_update_bits64(sdev, BYT_DSP_BAR, SHIM_IMRX,
				  SHIM_IMRX_BUSY | SHIM_IMRX_DONE,
				  SHIM_IMRX_DONE);

to me this looks like the opposite of setting DONE and disabling BUSY>

This has been on for two+ months, so I have no idea what the status is either.

[keyonjie]

it's just totally unclear what the new settings do.
see e.g.

	/* enable BUSY and disable DONE Interrupt by default */
	snd_sof_dsp_update_bits64(sdev, BYT_DSP_BAR, SHIM_IMRX,
				  SHIM_IMRX_BUSY | SHIM_IMRX_DONE,
				  SHIM_IMRX_DONE);

to me this looks like the opposite of setting DONE and disabling BUSY>

No, this is interrupt mask register, so setting DONE mask means disabling DONE interrupt, and similar with clearing BUSY mask.

This has been on for two+ months, so I have no idea what the status is either.

[plbossart]

replaced by PR #2138.