ASoC: SOF: apl: clear BUSY bit after done with ipc

[xiulipan]

When handle DSP to HOST ipc, BUSY bit need to be clear after the IPC is
handled by HOST. Or DSP may send IPCs on same time and HOST will miss
some of the IPCs that have been covered.

We may also need some check on HOST DSP side to send IPC after HOST is done

[xiulipan]

wrok with thesofproject/sof#232 to fix thesofproject/sof#180

[plbossart]

I don't get what this change does, and the commit message talks about a DONE bit while the code changes the BUSY bit.
Please comment and provide updates.

[plbossart]

Sorry, I don't follow the logic.
In the interrupt, when HIPCT.BUSY is set, you mask HIPCCTL.BUSY

Then in cmd_done, you clear HIPCT.BUSY and unmask HIPCCTL.BUSY

What exactly is the benefit of masking/unmasking HIPCCTL.BUSY? Until you clear HIPCT.BUSY no other message can be handled.

What am I missing?

[xiulipan]

Sorry about the typo in comment.

When DSP send a IPC to HOST, if we directly clear HIPCT.BUSY then DSP will get an interrupt to notify that the to HOST IPC is done. But actually the IPC is not handled by HOST, it is still in the queue and the message is in the mailbox. If DSP then have another IPC want to send to HOST(eg.
two pipelines, both want to send IPC position update notification to HOST), the new come IPC will cover what is inside the mailbox and the first coming IPC message is not handled.

To make DSP not send IPC too fast (just like what we did to HOST side), we need to let the HOST to notify DSP the IPC is done by clear HIPCT.BUSY. And to let the interrupt not come in anymore, MASK the HIPCCTL.BUSY first and then UNMASK until the HIPCT.BUSY is clear.

I will rebase and update the PR

[plbossart]

@xiulipan you don't "directly clear HIPCT.BUSY". In the code you clear it just before you unmask HIPCCTL.BUSY (in cmd_done()), so I still don't see what the benefits of masking/unmasking are. There cannot be another message handled by the host before this HIPCT.BUSY is cleared.
Maybe you should update the description that shows the potential race condition, the explanations are not aligned with the code.

[xiulipan]

@plbossart
OK. once again.
Old case:

1. DSP send an IPC to HOST, HIPCT.BUSY bit set, interrupt comes.
2. HOST enter hda_dsp_ipc_irq_thread, add a schedule work by snd_sof_ipc_msgs_rxand clear HIPCT.BUSY
3. DSP recieve the interrupt by HIPCT.BUSY is clear and DIPCIE.DONE is set.
4. DSP are free to go with another IPC. New IPC send to HOST.
5. schedule work begin, read IPC header from mailbox. New IPC cover the old one. The OLD one is missed.

New case:

1. DSP send an IPC to HOST, HIPCT.BUSY bit set, interrupt comes.
2. HOST enter hda_dsp_ipc_irq_thread, add a schedule work by snd_sof_ipc_msgs_rxand mask HIPCTL.BUSY. No interrupt comes.
   3.schedule work begin, read IPC header from mailbox and handle it. cmd_done clear HIPCT.BUSY and unmask HIPCTL.BUSY
3. DSP are free to go with another IPC. New IPC send to HOST.
4. New IPC repeat the process in linear.

The problem here is that we should not let DSP send new IPC before the HOST cmd_done, otherwise we may get into trouble. Our design to handle the IPC is not realtime, so there must be some method to keep the IPC go one after another done.

[xiulipan]

So the change is also here.
Chang the clear BUSY bit here to MASH busy BIT and clear the BUSY bit in cmd_done.
There used to have two BUSY bit clear.

[lgirdwood]

The same fix also needs done for the CNL logic.

[xiulipan]

Here is another clear BUSY in cmd_done function.
This one have no use since we have already clear the BUSY bit inside the hda_dsp_ipc_irq_thread

[plbossart]

It's fine to avoid clearing the HIPCT.BUSY before the cmd_done. My question is why do you need in addition to play with the mask, e.g why can't you just do this:

1. DSP send an IPC to HOST, HIPCT.BUSY bit set, interrupt comes.
2. HOST enter hda_dsp_ipc_irq_thread, add a schedule work by snd_sof_ipc_msgs_rx and mask HIPCTL.BUSY. No interrupt comes.
   3.schedule work begin, read IPC header from mailbox and handle it. cmd_done clear HIPCT.BUSY and unmask HIPCTL.BUSY
3. DSP are free to go with another IPC. New IPC send to HOST.
4. New IPC repeat the process in linear.

[RanderWang]

@xiulipan@plbossart

after masking the interrupt busy bit, the ipc interrupt would be not triggered to host. Without masking it, the interrupt form dsp would be triggered to host continuously before clearing busy bit and make host busy in irq function. many many interrupt messages would flush the screen.

[plbossart]

@RanderWang Sorry, this does not follow the 'normal' definition of interrupts. In theory once the interrupt has been signaled, you cannot have a new interrupt before clearing it. Masking is typically done once, not at the same time as interrupt clearing. And I will assert that with your explanations, there is still a non-zero time window where the interrupts could be fired before masking it, so it's not fully safe. Something's just not right here.

[RanderWang]

@plbossart@xiulipan

sorry, my understanding may be wrong, I attach the spec in programming guide.

IPC Target Busy Interrupt Enable (IPCTBIE)
When set to 1, it allows HIPCT.BUSY bit to propagate to ADSPIS.IPC bit for causing controller interrupt.

[RanderWang]

@plbossart

" And I will assert that with your explanations, there is still a non-zero time window where the interrupts could be fired before masking it, so it's not fully safe. Something's just not right here."

In fact, there is a register ADSPIC used for disabling IPC interrupt. Before enter IRQ thread, this register is set to disable IPC interrupt, and cleared out of the IRQ thread. So no problem for masking busy bit.

[plbossart]

"When set to 1, it allows HIPCT.BUSY bit to propagate to ADSPIS.IPC bit for causing controller interrupt."

This does not mean there will be multiple interrupts until HIPCT.BUSY is cleared.

"there is a register ADSPIC used for disabling IPC interrupt. Before enter IRQ thread, this register is set to disable IPC interrupt, and cleared out of the IRQ thread."

that may be the issue, if you disable the interrupt but the HIPCT.BUSY is still set when you renable it you may have a second spurious interrupt. I don't get why we need to have all three of enabling/disabling IPC interrupt, masking/unmasking HIPCTL.BUSY and clearing of HIPCT.BUSY. we are doing too much here and creating sequences that don't look right.

@lgirdwood you want to take over on this thread....

[RanderWang]

yes, there will be only one interrupt received continuously until HIPCT.BUSY is cleared.

The following log is for BUSY bit on:

[ 18.988289] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988301] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988313] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988325] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988336] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988348] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988359] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988368] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988378] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988387] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988396] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988405] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988414] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988427] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988439] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988450] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988462] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988474] sof-audio sof-audio: ipc: firmware initiated, msg:0x70000000, msg_ext:0x80
[ 18.988486] sof-audio sof-audio: mailbox upstream 0x81000 - size 0x1000
[ 18.988491] sof-audio sof-audio: mailbox downstream 0xa0000 - size 0x2000
[ 18.988494] sof-audio sof-audio: stream region 0xc1000 - size 0x1000
[ 18.988499] sof-audio sof-audio: booting DSP firmware completed
[ 18.988505] sof-audio sof-audio: ipc rx done: 0x70000000
[ 18.988521] sof-audio sof-audio: ipc rx: 0x70000000
[ 18.988523] sof-audio sof-audio: ipc rx done: 0x70000000
[ 18.988595] sof-audio sof-audio: firmware boot complete
[ 18.988702] sof-audio sof-audio: generating page table for 0000000017c1fc5a size 0x10000 pages 16
[ 18.988705] sof-audio sof-audio: pfn i 0 idx 0 pfn 173430
[ 18.988706] sof-audio sof-audio: pfn i 1 idx 2 pfn 173431
[ 18.988708] sof-audio sof-audio: pfn i 2 idx 5 pfn 173432

[xiulipan]

@plbossart
Yes, this change is made after some test.
We may have some over design here and may need @lgirdwood to have some look.

[lgirdwood]

The flow should be the same for all platforms (since the all have the same concepts of BUSY/DONE IRQ bits). We should mask IRQ whilst we process message and then unmask and clear BUSY after message processing is complete.

[xiulipan]

@lgirdwood@RanderWang
I think we have other platforms with same workflow to handle IPC IRQ.

[lgirdwood]

@RanderWang , @xiulipan both CNL and APL incorrectly clear the BUSY bit before the message is processed. BYT, HSW and BDW all correctly clear the BUSY bit after message is processed.

[lgirdwood]

Please do likewise for CNL and validate.

[lgirdwood]

The same fix also needs done for the CNL logic.

[plbossart]

@lgirdwood I don't get your point that "We should mask IRQ whilst we process message and then unmask and clear BUSY after message processing is complete". why is masking required? that's not typical IRQ process, clearing is typically enough to enable a new IRQ. What's so special about this IPC?

[lgirdwood]

@plbossart iirc we can only clear this IRQ by clearing the BUSY bit (and hence telling the DSP we are ready for more).

[xiulipan]

@lgirdwood
Will check and update the CNL as well and update the PR

[xiulipan]

@lgirdwood
Checked with @RanderWang, CNL have more BITs for the IPC IRQ, did not need to mask the bit.
No need to change CNL code.

[lgirdwood]

@xiulipan can you confirm CNL FW does not BUSY bit for status checking. I want the flows on each platform to be consistent.

[xiulipan]

@lgirdwood
For CNL IPC, Kernel set DONE bit and FW checked it.

sound/soc/sof/intel/cnl.c

staticintcnl_ipc_cmd_done(structsnd_sof_dev*sdev, intdir)
{
if (dir==SOF_IPC_HOST_REPLY) {
/* * set done bit to ack dsp the msg has been * processed and send reply msg to dsp */snd_sof_dsp_update_bits_forced(sdev, HDA_DSP_BAR,
CNL_DSP_REG_HIPCTDA,
CNL_DSP_REG_HIPCTDA_DONE,
CNL_DSP_REG_HIPCTDA_DONE);
} else {
...
}

src/ipc/cnl-ipc.c

staticvoidirq_handler(void*arg)
{
....
/* reply message(done) from host */if (dipcida&IPC_DIPCIDA_DONE) {
tracev_ipc("Rpy");
/* mask Done interrupt */ipc_write(IPC_DIPCCTL, ipc_read(IPC_DIPCCTL) & ~IPC_DIPCCTL_IPCIDIE);
/* clear DONE bit - tell host we have completed the operation */ipc_write(IPC_DIPCIDA, ipc_read(IPC_DIPCIDA) |IPC_DIPCIDA_DONE);
/* unmask Done interrupt */ipc_write(IPC_DIPCCTL, ipc_read(IPC_DIPCCTL) | IPC_DIPCCTL_IPCIDIE);
}
}