Skip to content

ASoC: SOF: intel: fix issues in hda irq handling#1011

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kv2019i wants to merge 3 commits into from
Closed

ASoC: SOF: intel: fix issues in hda irq handling #1011

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ba06b06
ASoC: SOF: intel: handle non-stream irqs correctly
kv2019i Jun 5, 2019
5bb53de
ASoC: SOF: intel: address missed RIRB events in irq handler
kv2019i Jun 5, 2019
1848733
ASoC: SOF: intel: move RIRBSTS handling to irq thread
kv2019i Jun 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 23 additions & 15 deletions sound/soc/sof/intel/hda-stream.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -462,36 +462,32 @@ irqreturn_t hda_dsp_stream_interrupt(int irq, void *context)
{
struct hdac_bus *bus = context;
struct sof_intel_hda_dev *sof_hda = bus_to_sof_hda(bus);
u32 stream_mask;
int ret = IRQ_NONE;
u32 status;

if (!pm_runtime_active(bus->dev))
return IRQ_NONE;

@plbossartplbossartJun 5, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep for RIRB but for stream events its' not really possible to get stream interrupts without being in pm_runtime_active, is it? Do we want to remove this completely?

@ranj063ranj063Jun 5, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree that we should just move it to after the RIRB status check. And also just fyi that this doesnt fix our IPC timeout issues still.

spin_lock(&bus->reg_lock);

status = snd_hdac_chip_readl(bus, INTSTS);
stream_mask = GENMASK(sof_hda->stream_max - 1, 0) | AZX_INT_CTRL_EN;

/* Not stream interrupt or register inaccessible, ignore it.*/
if (!(status & stream_mask) || status == 0xffffffff) {
/* Register inaccessible, ignore it.*/
if (status == 0xffffffff) {
spin_unlock(&bus->reg_lock);
return IRQ_NONE;
return ret;

@lyakhlyakhJun 12, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

}

#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
/* clear rirb int */
status = snd_hdac_chip_readb(bus, RIRBSTS);
if (status & RIRB_INT_MASK) {
if (status & RIRB_INT_RESPONSE)
snd_hdac_bus_update_rirb(bus);
snd_hdac_chip_writeb(bus, RIRBSTS, RIRB_INT_MASK);
if (status & AZX_INT_CTRL_EN) {
Comment thread
kv2019i marked this conversation as resolved.
Comment thread
kv2019i marked this conversation as resolved.
snd_hdac_chip_updatel(bus, INTCTL, AZX_INT_CTRL_EN, 0);
ret = IRQ_WAKE_THREAD;
}
#endif

if (status & GENMASK(sof_hda->stream_max - 1, 0))
ret = IRQ_WAKE_THREAD;

spin_unlock(&bus->reg_lock);

return snd_hdac_chip_readl(bus, INTSTS) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
return ret;
}

irqreturn_t hda_dsp_stream_threaded_handler(int irq, void *context)
Expand All@@ -500,6 +496,7 @@ irqreturn_t hda_dsp_stream_threaded_handler(int irq, void *context)
struct sof_intel_hda_dev *sof_hda = bus_to_sof_hda(bus);
u32 status = snd_hdac_chip_readl(bus, INTSTS);
struct hdac_stream *s;
u32 rirb_status;
u32 sd_status;

/* check streams */
Expand All@@ -524,6 +521,17 @@ irqreturn_t hda_dsp_stream_threaded_handler(int irq, void *context)
}
}

/* unmask 2nd level CTRL_EN (CIS) before clearing 1st level RIRBSTS */
snd_hdac_chip_updatel(bus, INTCTL, AZX_INT_CTRL_EN, 1);

@plbossartplbossartJun 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this make sense to unmask before clearing the interrupts?
or is this intentional to get the new interrupts what might have been raised by the handler?

@kv2019ikv2019iJun 10, 2019

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart This is on purpose. Clearing RIRBSTS bits will cause hardware to create a new interrupt and we want to allow this to happen by unmasking the CTRL_EN first.

@lyakhlyakhJun 12, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kv2019i didn't you mean

snd_hdac_chip_updatel(bus, INTCTL, AZX_INT_CTRL_EN, AZX_INT_CTRL_EN);

?


/* clear rirb int */
rirb_status = snd_hdac_chip_readb(bus, RIRBSTS);
if (rirb_status & RIRB_INT_MASK) {
if (rirb_status & RIRB_INT_RESPONSE)
snd_hdac_bus_update_rirb(bus);
snd_hdac_chip_writeb(bus, RIRBSTS, RIRB_INT_MASK);
}

@plbossartplbossartJun 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this should be protected by an #ifdef SOF_HDA

@kv2019ikv2019iJun 10, 2019

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart Ack.


return IRQ_HANDLED;
}

Expand Down