Skip to content

ASoC: SOF: merge ipc and sdw irq #1412

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
bardliao wants to merge 1 commit into from
Closed

ASoC: SOF: merge ipc and sdw irq #1412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 2 additions & 25 deletions drivers/soundwire/intel_init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,21 +163,7 @@ void sdw_intel_enable_irq(void __iomem *mmio_base, bool enable)
}
EXPORT_SYMBOL(sdw_intel_enable_irq);

static irqreturn_t sdw_intel_irq(int irq, void *dev_id)
{
struct sdw_intel_ctx *ctx = dev_id;
u32 int_status;

int_status = readl(ctx->mmio_base + HDA_DSP_REG_ADSPIS2);
if (int_status & HDA_DSP_REG_ADSPIC2_SNDW) {
sdw_intel_enable_irq(ctx->mmio_base, false);
return IRQ_WAKE_THREAD;
}

return IRQ_NONE;
}

static irqreturn_t sdw_intel_thread(int irq, void *dev_id)
irqreturn_t sdw_intel_thread(int irq, void *dev_id)
{
struct sdw_intel_ctx *ctx = dev_id;
struct sdw_intel_link_res *link;
Expand All @@ -188,6 +174,7 @@ static irqreturn_t sdw_intel_thread(int irq, void *dev_id)
sdw_intel_enable_irq(ctx->mmio_base, true);
return IRQ_HANDLED;
}
EXPORT_SYMBOL(sdw_intel_thread);

static struct sdw_intel_ctx
*sdw_intel_probe_controller(struct sdw_intel_res *res)
Expand All @@ -199,7 +186,6 @@ static struct sdw_intel_ctx
u32 link_mask;
int count;
int i;
int ret;

if (!res)
return NULL;
Expand Down Expand Up @@ -261,15 +247,6 @@ static struct sdw_intel_ctx
list_add_tail(&link->list, &ctx->link_list);
}

ret = request_threaded_irq(ctx->irq,
sdw_intel_irq, sdw_intel_thread,
IRQF_SHARED, KBUILD_MODNAME, ctx);
if (ret < 0) {
dev_err(&adev->dev, "unable to grab IRQ %d, disabling device\n",
res->irq);
goto err;
}

return ctx;

err:
Expand Down
14 changes: 14 additions & 0 deletions include/linux/soundwire/sdw_intel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,4 +123,18 @@ void sdw_intel_exit(struct sdw_intel_ctx *ctx);

void sdw_intel_enable_irq(void __iomem *mmio_base, bool enable);

irqreturn_t sdw_intel_thread(int irq, void *dev_id);

static inline
bool is_sdw_intel_irq(struct sdw_intel_ctx *ctx)
{
u32 int_status;

int_status = readl(ctx->mmio_base + 0x14);

@plbossart plbossart Nov 7, 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.

HDA_DSP_REG_ADSPIS2

@bardliao bardliao Nov 8, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@plbossart HDA_DSP_REG_ADSPIS2 is defined at sound/soc/sof/intel/hda.h, but we don't include hda.h in soundwire driver. I am not sure include hda.h in sdw_intel.h is a good idea or not. If we just add the HDA_DSP_REG_ADSPIS2 define in sdw_intel.h, we will see redefine warning in compiling. Or we can define another name for HDA_DSP_REG_ADSPIS2 in sdw_intel.h?

@plbossart plbossart Nov 12, 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.

just redefine them SDW_ADSPIC2 and SDW_ADSIS2. that's fine.

if (int_status & BIT(5))

@plbossart plbossart Nov 7, 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.

HDA_DSP_REG_ADSPIC2_SNDW

return true;

return false;
}

#endif
26 changes: 25 additions & 1 deletion sound/soc/sof/intel/hda-ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,15 +237,18 @@ irqreturn_t hda_dsp_ipc_irq_thread(int irq, void *context)
return IRQ_HANDLED;
}

/* is this IRQ for ADSP ? - we only care about IPC here */
/* is this IRQ for ADSP or SDW? - we only care about IPC and SDW here */
irqreturn_t hda_dsp_ipc_irq_handler(int irq, void *context)
{
struct snd_sof_dev *sdev = context;
struct sof_intel_hda_dev *hdev = sdev->pdata->hw_pdata;
int ret = IRQ_NONE;
u32 irq_status;

spin_lock(&sdev->hw_lock);

sdev->irq_event = 0;

/* store status */
irq_status = snd_sof_dsp_read(sdev, HDA_DSP_BAR, HDA_DSP_REG_ADSPIS);
dev_vdbg(sdev->dev, "irq handler: irq_status:0x%x\n", irq_status);
Expand All @@ -260,6 +263,13 @@ irqreturn_t hda_dsp_ipc_irq_handler(int irq, void *context)
snd_sof_dsp_update_bits_unlocked(sdev, HDA_DSP_BAR,
HDA_DSP_REG_ADSPIC,
HDA_DSP_ADSPIC_IPC, 0);
sdev->irq_event |= SOF_IRQ_IPC;

@plbossart plbossart Nov 7, 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.

the use of a field that's set in the handler is problematic. We know we can have new interrupts before the thread is scheduled (that's e.g. how SoundWire handled the commands before, with the wait_for_completion in the thread and the complete() bin the handler

So it's possible that while you have an SoundWire thread scheduled an IPC interrupt quicks in, and you would reset sdev->irq_event.

I don't think this can work...

ret = IRQ_WAKE_THREAD;
}
/* SDW message */
if (hdev->sdw && is_sdw_intel_irq(hdev->sdw)) {
hda_sdw_int_enable(sdev, false);
sdev->irq_event |= SOF_IRQ_SDW;
ret = IRQ_WAKE_THREAD;
}

Expand All @@ -268,6 +278,20 @@ irqreturn_t hda_dsp_ipc_irq_handler(int irq, void *context)
return ret;
}

irqreturn_t hda_dsp_irq_thread(int irq, void *context)
{
struct snd_sof_dev *sdev = context;
struct sof_intel_hda_dev *hdev = sdev->pdata->hw_pdata;

if (sdev->irq_event & SOF_IRQ_IPC)
sof_ops(sdev)->irq_thread(irq, context);

if (sdev->irq_event & SOF_IRQ_SDW)
sdw_intel_thread(irq, hdev->sdw);

@ranj063 ranj063 Nov 4, 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.

@bardliao I understand the intention here but the naming is incorrent. hda_dsp_irq_thread() is the IRQ thread, sof_ops(sdev)->irq_thread() and sdw_intel_thread() are not threads but just normal methods. So I think we should remove the "thread" from their names.

@bardliao bardliao Nov 5, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@bardliao I understand the intention here but the naming is incorrent. hda_dsp_irq_thread() is the IRQ thread, sof_ops(sdev)->irq_thread() and sdw_intel_thread() are not threads but just normal methods. So I think we should remove the "thread" from their names.

@ranj063 I agree with you. I have a concern regarding sof_ops(sdev)->irq_thread(). Is there anywhere use sof_ops(sdev)->irq_thread() except here? And it looks confusing, too if we remove the _thread suffix. i.e. sof_ops(sdev)->irq. irq_handler may be a better name, but we already have it. Do you have any suggestion for irq_thread?


return IRQ_HANDLED;
}

int hda_dsp_ipc_get_mailbox_offset(struct snd_sof_dev *sdev)
{
return HDA_DSP_MBOX_UPLINK_OFFSET;
Expand Down
2 changes: 1 addition & 1 deletion sound/soc/sof/intel/hda.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)

dev_dbg(sdev->dev, "using IPC IRQ %d\n", sdev->ipc_irq);
ret = request_threaded_irq(sdev->ipc_irq, hda_dsp_ipc_irq_handler,
sof_ops(sdev)->irq_thread, IRQF_SHARED,
hda_dsp_irq_thread, IRQF_SHARED,
"AudioDSP", sdev);
if (ret < 0) {
dev_err(sdev->dev, "error: failed to register IPC IRQ %d\n",
Expand Down
1 change: 1 addition & 0 deletions sound/soc/sof/intel/hda.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,7 @@ int hda_dsp_ipc_get_window_offset(struct snd_sof_dev *sdev, u32 id);

irqreturn_t hda_dsp_ipc_irq_handler(int irq, void *context);
irqreturn_t hda_dsp_ipc_irq_thread(int irq, void *context);
irqreturn_t hda_dsp_irq_thread(int irq, void *context);
int hda_dsp_ipc_cmd_done(struct snd_sof_dev *sdev, int dir);

/*
Expand Down
5 changes: 5 additions & 0 deletions sound/soc/sof/sof-priv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@
#define SOF_DBG_PCI BIT(4)
#define SOF_DBG_RETAIN_CTX BIT(5) /* prevent DSP D3 on FW exception */

/* interrupt event flags */
#define SOF_IRQ_IPC BIT(0)
#define SOF_IRQ_SDW BIT(1)

/* global debug state set by SOF_DBG_ flags */
extern int sof_core_debug;

Expand Down Expand Up @@ -424,6 +428,7 @@ struct snd_sof_dev {
struct snd_sof_mailbox stream_box; /* Stream position update */
struct snd_sof_ipc_msg *msg;
int ipc_irq;
u32 irq_event; /* Which interrupt are triggered*/
u32 next_comp_id; /* monotonic - reset during S3 */

/* memory bases for mmaped DSPs - set by dsp_init() */
Expand Down