Uh oh!
There was an error while loading. Please reload this page.
ASoC: SOF: intel: detect all ROM Stream Tag IPC messages#799
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -116,6 +116,12 @@ void hda_dsp_ipc_get_reply(struct snd_sof_dev *sdev) | ||
| spin_unlock_irqrestore(&sdev->ipc_lock, flags); | ||
| } | ||
| static bool hda_dsp_ipc_is_sof(uint32_t msg) | ||
| { | ||
| return (msg & (HDA_DSP_IPC_PURGE_FW | 0xf << 9)) != msg || | ||
Member There was a problem hiding this comment. 0xf << 9? that looks like a magic sequence, can't we make it more readable? CollaboratorAuthor There was a problem hiding this comment. According to #773 (comment) that's the 4 stream_id bits, that various ROM messages can vary. | ||
| (msg & HDA_DSP_IPC_PURGE_FW) != HDA_DSP_IPC_PURGE_FW; | ||
| } | ||
Member There was a problem hiding this comment. the test looks odd, not sure I get why the same PURGE_FW is tested twice? CollaboratorAuthor There was a problem hiding this comment. I think it is correct. You have to basically check for two things: that all HDA_DSP_IPC_PURGE_FW bits are set and no other bits (apart from 0xf << 9) are set. That's why it has two tests. The test for a ROM message is HDA_DSP_IPC_PURGE_FW | 0xf << 9 is the mask for all ROM messages. So, the first test checks, that msg has no bits set outside of that mask. The second test checks, that all PURGE bits are set in msg. The test in the function is just an inversion of this test. I haven't found an easier or more elegant form to perform this test. I'd certainly love to see one. | ||
| /* IPC handler thread */ | ||
| irqreturn_t hda_dsp_ipc_irq_thread(int irq, void *context) | ||
| { | ||
| @@ -161,7 +167,7 @@ irqreturn_t hda_dsp_ipc_irq_thread(int irq, void *context) | ||
| HDA_DSP_REG_HIPCCTL_DONE, 0); | ||
| /* handle immediate reply from DSP core - ignore ROM messages */ | ||
| if (hda_dsp_ipc_is_sof(msg)) { | ||
Member There was a problem hiding this comment. is_sof means not ROM here? the name is weird. CollaboratorAuthor There was a problem hiding this comment. Yes, I thought Member There was a problem hiding this comment. actually I think if (!is_rom()) is more self-explanatory. But we can fix later in a follow-up PR. | ||
| hda_dsp_ipc_get_reply(sdev); | ||
| snd_sof_ipc_reply(sdev, msg); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lyakh maybe we can be precise in this check. If you look in hda-loader.c the cl_stream_prepare() method returns the stream tag the code loader will be assigned. So we can just check for that one instead of checking for all of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ranj063 how would you know in this context which of the tags to look for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plbossart i was under the impression that the ROM message is only received after the code loader DMA is complete and the DSP sends back the reply. The stream tag for the code loader is assigned just prior to this. Maybe I am mistaken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the one hand I agree that it's good to be precise, if we really can be sure which tag should be coming we can check just for that one. On the other hand it looks like so far we weren't very successful with being precise here. Do you know of any drawbacks from checking for all tags here?