ASoC: SOF: merge ipc and sdw irq by bardliao · Pull Request #1451 · thesofproject/linux

bardliao · 2019-11-08T05:32:27Z

So we can use single irq to handle both ipc and sdw interrupts.

Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com

plbossart · 2019-11-11T23:24:29Z

@bardliao did you see my comments on #1412 ?

bardliao · 2019-11-12T02:53:55Z

@bardliao did you see my comments on #1412 ?

@plbossart Yes, and I update my PR according to your suggestion. However, I have a question regardingHDA_DSP_REG_ADSPIS2. Please see #1412 (comment). Currently, I call sof_ops(sdev)->irq_thread(irq, context) and sdw_intel_thread(irq, hdev->sdw) in hda_dsp_irq_thread unconditionally. But I think we can improve it there. I will do it today.

bardliao · 2019-11-12T06:05:11Z

@plbossart I updated my PR. The main difference is that I check interrupt and set irq_event flag at hda_dsp_ipc_irq_handler(). So we will need to check interrupt only if the irq_event flag is not set. And we will only call sof_ops(sdev)->irq_thread(irq, context) and/or sdw_intel_thread(irq, hdev->sdw) when it is required.

RanderWang · 2019-11-12T08:33:43Z

I checked windows driver which does the same job.

plbossart

@bardliao good work, but before we start changing the SoundWire handing of interrupts, we should first merge the IPC and stream interrupts on top of topic/sof-dev. If the end-goal is to merge all interrupt handlers as a single one, the first order of business is to merge the two non-SoundWire ones and have a full-cycle of validation on that topic alone.

See also additional comments below

plbossart · 2019-11-13T19:39:43Z

+
+	return false;
+}
+


because SoundWire and ASoC are handled as separate subsystems, we need a patch that will modify the soundwire interfaces, and a separate on that will modify the ASoC parts.

plbossart · 2019-11-13T19:41:46Z

 	u32 irq_status;

-	spin_lock(&sdev->hw_lock);
+	spin_lock_irq(&sdev->hw_lock);


why do we need a change here? is this is because we use this routine both from handler and interrupt threads, than you need to add a comment.

Yes, is_ipc_irq() will be called on interrupt thread and handler, and we don't want another interrupt to lock it while it is locked.

plbossart · 2019-11-13T19:42:52Z

+	struct sof_intel_hda_dev *hdev = sdev->pdata->hw_pdata;
+	int ret = IRQ_NONE;
+
+	sdev->irq_event = 0;


we can't do this, the irq_event may be reset before it's used in a thread. I think we have no choice but to re-read the hardware registers in the thread.

Thanks. @plbossart I didn't notice that. Maybe we can clear each bit in the irq thread the interrupt is handled.

plbossart · 2019-11-13T19:46:11Z

+
+	/* IPC message ? */
+	if (is_ipc_irq(sdev)) {
+		/* disable IPC interrupt at is_ipc_irq() */


it's quite ugly, we will use is_ipc_irq() multiple times (in the handler and in the thread), so we should only disable the interrupt once.

That's why we have irq_event :) Ideally, we won't call is_ipc_irq() if SOF_IRQ_IPC is set. Also, I think we need to disable irq, too if we get an interrupt on irq thread.

plbossart · 2019-11-13T19:47:33Z

+		sdw_intel_thread(irq, hdev->sdw);
+	else if (hdev->sdw && is_sdw_intel_irq(hdev->sdw)) {
+		hda_sdw_int_enable(sdev, false);
+		sdw_intel_thread(irq, hdev->sdw);


sorry. I don't understand the last two cases here. Aren't they exactly the same thing?

Also what's the point of using the irq_events since we will also test for is_ipc_irq and is_sdw_intel_irq() in the thread?

sorry. I don't understand the last two cases here. Aren't they exactly the same thing?

if (sdev->irq_event & SOF_IRQ_SDW) means is_sdw_intel_irq() return true in the handler, and we already disable interrupt there. So we don't need to disable interrupt there. else means is_sdw_intel_irq() return false. So we test is_sdw_intel_irq() here again to make sure we will handle sdw interrupt if there is any. And we need to disable sdw interrupt here since we didn't disable it in the handler.

Also what's the point of using the irq_events since we will also test for is_ipc_irq and is_sdw_intel_irq() in the thread?

We don't have to test is irq again in the thread If irq_event is set. i.e. We will only test is irq when the test result is false on irq handler.

bardliao · 2019-11-14T07:24:17Z

@bardliao good work, but before we start changing the SoundWire handing of interrupts, we should first merge the IPC and stream interrupts on top of topic/sof-dev. If the end-goal is to merge all interrupt handlers as a single one, the first order of business is to merge the two non-SoundWire ones and have a full-cycle of validation on that topic alone.

@plbossart Agree, let's focus on merging IPC and stream interrupts first.

mengdonglin · 2019-11-19T09:25:15Z

So this will depend on merging IPC and stream interrupts, RFC in #1483

So we can use single irq to handle both ipc and sdw interrupts. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

plbossart · 2019-11-20T15:42:59Z

sorry @bardliao I am confused by all the previous comments and versions.
Can you restart with a new PR and I guess add PR #1483 then your soundwire changes on top (as a separate patch?)

bardliao · 2019-11-21T06:14:50Z

sorry @bardliao I am confused by all the previous comments and versions.
Can you restart with a new PR and I guess add PR #1483 then your soundwire changes on top (as a separate patch?)

@plbossart Yes, let's close this one.

bardliao requested review from plbossart and ranj063 November 8, 2019 05:32

bardliao requested review from juimonen, keyonjie, lgirdwood and lyakh as code owners November 8, 2019 05:32

bardliao mentioned this pull request Nov 8, 2019

ASoC: SOF: merge ipc and sdw irq #1412

Closed

plbossart force-pushed the integration/soundwire-debug-fixes branch 3 times, most recently from b475d02 to 70067f0 Compare November 11, 2019 22:41

bardliao force-pushed the sdw-one-irq branch from 13c33b2 to 8463d7f Compare November 12, 2019 02:39

bardliao force-pushed the sdw-one-irq branch from 8463d7f to f65f021 Compare November 12, 2019 05:56

YvonneYang2 mentioned this pull request Nov 12, 2019

[SDW][BUG]ipc timed out for 0x40080000 size 32 occurred when run reboot stress test with legacy IRQ mode #1468

Closed

bardliao force-pushed the sdw-one-irq branch 2 times, most recently from 17363cf to 5d0e8ef Compare November 12, 2019 07:53

plbossart force-pushed the integration/soundwire-debug-fixes branch 2 times, most recently from 60c0523 to f89e07c Compare November 12, 2019 23:43

bardliao force-pushed the sdw-one-irq branch from 5d0e8ef to a575044 Compare November 13, 2019 00:17

plbossart requested changes Nov 13, 2019

View reviewed changes

plbossart force-pushed the integration/soundwire-debug-fixes branch 2 times, most recently from 7b348fd to 85aa830 Compare November 13, 2019 20:56

bardliao force-pushed the sdw-one-irq branch from a575044 to 614fc7c Compare November 14, 2019 01:08

YvonneYang2 mentioned this pull request Nov 14, 2019

[BUG][SDW]IO transfer timed out occurred when run suspend/resume stress test. #1482

Closed

plbossart force-pushed the integration/soundwire-debug-fixes branch 5 times, most recently from 989e38e to ac926ce Compare November 18, 2019 20:40

ASoC: SOF: merge ipc and sdw irq

0ccfdb4

So we can use single irq to handle both ipc and sdw interrupts. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao force-pushed the sdw-one-irq branch from 614fc7c to 0ccfdb4 Compare November 20, 2019 09:13

bardliao closed this Nov 21, 2019

Conversation

bardliao commented Nov 8, 2019

Uh oh!

plbossart commented Nov 11, 2019

Uh oh!

bardliao commented Nov 12, 2019

Uh oh!

bardliao commented Nov 12, 2019

Uh oh!

RanderWang commented Nov 12, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bardliao commented Nov 14, 2019

Uh oh!

mengdonglin commented Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented Nov 20, 2019

Uh oh!

bardliao commented Nov 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mengdonglin commented Nov 19, 2019 •

edited

Loading