[PATCH 0/2] ASoC: Intel: avs: Refactor IRQ handling

[PATCH 0/2] ASoC: Intel: avs: Refactor IRQ handling

[Cezary Rojewski]

The existing code can be both improved and simplified. To make this
change easier to manage, first add new implementation and then remove
deadcode in a separate patch.

Simplification achieved with:

- reduce the amount of resources requested by the driver i.e.: IPC and
  CLDMA request_irq() merged into one
- reduce the number of DSP ops from 2 to 1:
  irq_handler/thread() vs dsp_interrupt()
- drop ambiguity around CLDMA interrupt, let skl.c handle that
  explicitly as it is the only user

With that done, switch to the new implementation and remove unused
members. While the change is non-trivial, from functional perspective
status quo is achieved.

Cezary Rojewski (2):
  ASoC: Intel: avs: New IRQ handling implementation
  ASoC: Intel: avs: Remove unused IRQ-related code

 sound/soc/intel/avs/apl.c   | 20 +++++++-
 sound/soc/intel/avs/avs.h   |  8 ++--
 sound/soc/intel/avs/cldma.c | 42 ++++-------------
 sound/soc/intel/avs/cldma.h |  1 +
 sound/soc/intel/avs/cnl.c   | 91 ++++++++++++++++++++++++-------------
 sound/soc/intel/avs/core.c  | 85 ++++++++++++++++------------------
 sound/soc/intel/avs/icl.c   |  3 +-
 sound/soc/intel/avs/ipc.c   | 48 -------------------
 sound/soc/intel/avs/skl.c   | 73 +++++++++++++++++++++--------
 sound/soc/intel/avs/tgl.c   |  3 +-
 10 files changed, 186 insertions(+), 188 deletions(-)

-- 
2.25.1

[PATCH 1/2] ASoC: Intel: avs: New IRQ handling implementation

[Cezary Rojewski]

The existing code can be both improved and simplified. To make this
change easier to manage, first add new implementation and then remove
deadcode in a separate patch.

Simplification achieved with:
- reduce the amount of resources requested by the driver i.e.: IPC and
  CLDMA request_irq() merged into one
- reduce the number of DSP ops from 2 to 1:
  irq_handler/thread() vs dsp_interrupt()
- drop ambiguity around CLDMA interrupt, let skl.c handle that
  explicitly as it is the only user

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/apl.c   | 18 +++++++++
 sound/soc/intel/avs/avs.h   |  3 ++
 sound/soc/intel/avs/cldma.c | 11 +++++
 sound/soc/intel/avs/cldma.h |  1 +
 sound/soc/intel/avs/cnl.c   | 63 +++++++++++++++++++++++++++++
 sound/soc/intel/avs/core.c  | 80 +++++++++++++++++++++++++++++++++++++
 sound/soc/intel/avs/icl.c   |  1 +
 sound/soc/intel/avs/skl.c   | 62 ++++++++++++++++++++++++++++
 sound/soc/intel/avs/tgl.c   |  1 +
 9 files changed, 240 insertions(+)

diff --git a/sound/soc/intel/avs/apl.c b/sound/soc/intel/avs/apl.c
index c21ecaef9eba..a186d88430b9 100644
--- a/sound/soc/intel/avs/apl.c
+++ b/sound/soc/intel/avs/apl.c
@@ -8,11 +8,28 @@
 
 #include <linux/devcoredump.h>
 #include <linux/slab.h>
+#include <sound/hdaudio_ext.h>
 #include "avs.h"
 #include "messages.h"
 #include "path.h"
 #include "topology.h"
 
+static irqreturn_t avs_apl_dsp_interrupt(struct avs_dev *adev)
+{
+	u32 adspis = snd_hdac_adsp_readl(adev, AVS_ADSP_REG_ADSPIS);
+	irqreturn_t ret = IRQ_NONE;
+
+	if (adspis == UINT_MAX)
+		return ret;
+
+	if (adspis & AVS_ADSP_ADSPIS_IPC) {
+		avs_skl_ipc_interrupt(adev);
+		ret = IRQ_HANDLED;
+	}
+
+	return ret;
+}
+
 #ifdef CONFIG_DEBUG_FS
 int avs_apl_enable_logs(struct avs_dev *adev, enum avs_log_enable enable, u32 aging_period,
 			u32 fifo_full_period, unsigned long resource_mask, u32 *priorities)
@@ -237,6 +254,7 @@ const struct avs_dsp_ops avs_apl_dsp_ops = {
 	.power = avs_dsp_core_power,
 	.reset = avs_dsp_core_reset,
 	.stall = avs_dsp_core_stall,
+	.dsp_interrupt = avs_apl_dsp_interrupt,
 	.irq_handler = avs_irq_handler,
 	.irq_thread = avs_skl_irq_thread,
 	.int_control = avs_dsp_interrupt_control,
diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h
index eb8c03afdd23..75ae8f3addb8 100644
--- a/sound/soc/intel/avs/avs.h
+++ b/sound/soc/intel/avs/avs.h
@@ -46,6 +46,7 @@ struct avs_dsp_ops {
 	int (* const power)(struct avs_dev *, u32, bool);
 	int (* const reset)(struct avs_dev *, u32, bool);
 	int (* const stall)(struct avs_dev *, u32, bool);
+	irqreturn_t (* const dsp_interrupt)(struct avs_dev *);
 	irqreturn_t (* const irq_handler)(struct avs_dev *);
 	irqreturn_t (* const irq_thread)(struct avs_dev *);
 	void (* const int_control)(struct avs_dev *, bool);
@@ -269,6 +270,8 @@ int avs_dsp_enable_d0ix(struct avs_dev *adev);
 
 irqreturn_t avs_skl_irq_thread(struct avs_dev *adev);
 irqreturn_t avs_cnl_irq_thread(struct avs_dev *adev);
+void avs_skl_ipc_interrupt(struct avs_dev *adev);
+irqreturn_t avs_cnl_dsp_interrupt(struct avs_dev *adev);
 int avs_apl_enable_logs(struct avs_dev *adev, enum avs_log_enable enable, u32 aging_period,
 			u32 fifo_full_period, unsigned long resource_mask, u32 *priorities);
 int avs_icl_enable_logs(struct avs_dev *adev, enum avs_log_enable enable, u32 aging_period,
diff --git a/sound/soc/intel/avs/cldma.c b/sound/soc/intel/avs/cldma.c
index 585579840b64..c79b126f376d 100644
--- a/sound/soc/intel/avs/cldma.c
+++ b/sound/soc/intel/avs/cldma.c
@@ -270,6 +270,17 @@ static irqreturn_t cldma_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+void hda_cldma_interrupt(struct hda_cldma *cl)
+{
+	/* disable CLDMA interrupt */
+	snd_hdac_adsp_updatel(cl, AVS_ADSP_REG_ADSPIC, AVS_ADSP_ADSPIC_CLDMA, 0);
+
+	cl->sd_status = snd_hdac_stream_readb(cl, SD_STS);
+	dev_dbg(cl->dev, "%s sd_status: 0x%08x\n", __func__, cl->sd_status);
+
+	complete(&cl->completion);
+}
+
 int hda_cldma_init(struct hda_cldma *cl, struct hdac_bus *bus, void __iomem *dsp_ba,
 		   unsigned int buffer_size)
 {
diff --git a/sound/soc/intel/avs/cldma.h b/sound/soc/intel/avs/cldma.h
index 223d3431ab81..7d95e2747f52 100644
--- a/sound/soc/intel/avs/cldma.h
+++ b/sound/soc/intel/avs/cldma.h
@@ -24,6 +24,7 @@ int hda_cldma_reset(struct hda_cldma *cl);
 
 void hda_cldma_set_data(struct hda_cldma *cl, void *data, unsigned int size);
 void hda_cldma_setup(struct hda_cldma *cl);
+void hda_cldma_interrupt(struct hda_cldma *cl);
 int hda_cldma_init(struct hda_cldma *cl, struct hdac_bus *bus, void __iomem *dsp_ba,
 		   unsigned int buffer_size);
 void hda_cldma_free(struct hda_cldma *cl);
diff --git a/sound/soc/intel/avs/cnl.c b/sound/soc/intel/avs/cnl.c
index 5423c29ecc4e..c021c0f51a53 100644
--- a/sound/soc/intel/avs/cnl.c
+++ b/sound/soc/intel/avs/cnl.c
@@ -42,10 +42,73 @@ irqreturn_t avs_cnl_irq_thread(struct avs_dev *adev)
 	return IRQ_HANDLED;
 }
 
+static void avs_cnl_ipc_interrupt(struct avs_dev *adev)
+{
+	const struct avs_spec *spec = adev->spec;
+	u32 hipc_ack, hipc_rsp;
+
+	snd_hdac_adsp_updatel(adev, spec->hipc->ctl_offset,
+			      AVS_ADSP_HIPCCTL_DONE | AVS_ADSP_HIPCCTL_BUSY, 0);
+
+	hipc_ack = snd_hdac_adsp_readl(adev, spec->hipc->ack_offset);
+	hipc_rsp = snd_hdac_adsp_readl(adev, spec->hipc->rsp_offset);
+
+	/* DSP acked host's request. */
+	if (hipc_ack & spec->hipc->ack_done_mask) {
+		complete(&adev->ipc->done_completion);
+
+		/* Tell DSP it has our attention. */
+		snd_hdac_adsp_updatel(adev, spec->hipc->ack_offset, spec->hipc->ack_done_mask,
+				      spec->hipc->ack_done_mask);
+	}
+
+	/* DSP sent new response to process. */
+	if (hipc_rsp & spec->hipc->rsp_busy_mask) {
+		union avs_reply_msg msg;
+		u32 hipctda;
+
+		msg.primary = snd_hdac_adsp_readl(adev, CNL_ADSP_REG_HIPCTDR);
+		msg.ext.val = snd_hdac_adsp_readl(adev, CNL_ADSP_REG_HIPCTDD);
+
+		avs_dsp_process_response(adev, msg.val);
+
+		/* Tell DSP we accepted its message. */
+		snd_hdac_adsp_updatel(adev, CNL_ADSP_REG_HIPCTDR,
+				      CNL_ADSP_HIPCTDR_BUSY, CNL_ADSP_HIPCTDR_BUSY);
+		/* Ack this response. */
+		snd_hdac_adsp_updatel(adev, CNL_ADSP_REG_HIPCTDA,
+				      CNL_ADSP_HIPCTDA_DONE, CNL_ADSP_HIPCTDA_DONE);
+		/* HW might have been clock gated, give some time for change to propagate. */
+		snd_hdac_adsp_readl_poll(adev, CNL_ADSP_REG_HIPCTDA, hipctda,
+					 !(hipctda & CNL_ADSP_HIPCTDA_DONE), 10, 1000);
+	}
+
+	snd_hdac_adsp_updatel(adev, spec->hipc->ctl_offset,
+			      AVS_ADSP_HIPCCTL_DONE | AVS_ADSP_HIPCCTL_BUSY,
+			      AVS_ADSP_HIPCCTL_DONE | AVS_ADSP_HIPCCTL_BUSY);
+}
+
+irqreturn_t avs_cnl_dsp_interrupt(struct avs_dev *adev)
+{
+	u32 adspis = snd_hdac_adsp_readl(adev, AVS_ADSP_REG_ADSPIS);
+	irqreturn_t ret = IRQ_NONE;
+
+	if (adspis == UINT_MAX)
+		return ret;
+
+	if (adspis & AVS_ADSP_ADSPIS_IPC) {
+		avs_cnl_ipc_interrupt(adev);
+		ret = IRQ_HANDLED;
+	}
+
+	return ret;
+}
+
 const struct avs_dsp_ops avs_cnl_dsp_ops = {
 	.power = avs_dsp_core_power,
 	.reset = avs_dsp_core_reset,
 	.stall = avs_dsp_core_stall,
+	.dsp_interrupt = avs_cnl_dsp_interrupt,
 	.irq_handler = avs_irq_handler,
 	.irq_thread = avs_cnl_irq_thread,
 	.int_control = avs_dsp_interrupt_control,
diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
index 76782a0f32bc..25759f4f0213 100644
--- a/sound/soc/intel/avs/core.c
+++ b/sound/soc/intel/avs/core.c
@@ -336,6 +336,86 @@ static irqreturn_t avs_dsp_irq_thread(int irq, void *dev_id)
 	return avs_dsp_op(adev, irq_thread);
 }
 
+static irqreturn_t avs_hda_interrupt(struct hdac_bus *bus)
+{
+	irqreturn_t ret = IRQ_NONE;
+	u32 status;
+
+	status = snd_hdac_chip_readl(bus, INTSTS);
+	if (snd_hdac_bus_handle_stream_irq(bus, status, hdac_update_stream))
+		ret = IRQ_HANDLED;
+
+	spin_lock_irq(&bus->reg_lock);
+	/* Clear RIRB interrupt. */
+	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);
+		ret = IRQ_HANDLED;
+	}
+
+	spin_unlock_irq(&bus->reg_lock);
+	return ret;
+}
+
+__maybe_unused
+static irqreturn_t avs_hda_irq_handler(int irq, void *dev_id)
+{
+	struct hdac_bus *bus = dev_id;
+	u32 intsts;
+
+	intsts = snd_hdac_chip_readl(bus, INTSTS);
+	if (intsts == UINT_MAX || !(intsts & AZX_INT_GLOBAL_EN))
+		return IRQ_NONE;
+
+	/* Mask GIE, unmasked in irq_thread(). */
+	snd_hdac_chip_updatel(bus, INTCTL, AZX_INT_GLOBAL_EN, 0);
+
+	return IRQ_WAKE_THREAD;
+}
+
+__maybe_unused
+static irqreturn_t avs_hda_irq_thread(int irq, void *dev_id)
+{
+	struct hdac_bus *bus = dev_id;
+	u32 status;
+
+	status = snd_hdac_chip_readl(bus, INTSTS);
+	if (status & ~AZX_INT_GLOBAL_EN)
+		avs_hda_interrupt(bus);
+
+	/* Unmask GIE, masked in irq_handler(). */
+	snd_hdac_chip_updatel(bus, INTCTL, AZX_INT_GLOBAL_EN, AZX_INT_GLOBAL_EN);
+
+	return IRQ_HANDLED;
+}
+
+__maybe_unused
+static irqreturn_t avs_dsp_irq_handler2(int irq, void *dev_id)
+{
+	struct avs_dev *adev = dev_id;
+
+	return avs_hda_irq_handler(irq, &adev->base.core);
+}
+
+__maybe_unused
+static irqreturn_t avs_dsp_irq_thread2(int irq, void *dev_id)
+{
+	struct avs_dev *adev = dev_id;
+	struct hdac_bus *bus = &adev->base.core;
+	u32 status;
+
+	status = readl(bus->ppcap + AZX_REG_PP_PPSTS);
+	if (status & AZX_PPCTL_PIE)
+		avs_dsp_op(adev, dsp_interrupt);
+
+	/* Unmask GIE, masked in irq_handler(). */
+	snd_hdac_chip_updatel(bus, INTCTL, AZX_INT_GLOBAL_EN, AZX_INT_GLOBAL_EN);
+
+	return IRQ_HANDLED;
+}
+
 static int avs_hdac_acquire_irq(struct avs_dev *adev)
 {
 	struct hdac_bus *bus = &adev->base.core;
diff --git a/sound/soc/intel/avs/icl.c b/sound/soc/intel/avs/icl.c
index 7a1ecf241856..004fe71505dd 100644
--- a/sound/soc/intel/avs/icl.c
+++ b/sound/soc/intel/avs/icl.c
@@ -188,6 +188,7 @@ const struct avs_dsp_ops avs_icl_dsp_ops = {
 	.power = avs_dsp_core_power,
 	.reset = avs_dsp_core_reset,
 	.stall = avs_dsp_core_stall,
+	.dsp_interrupt = avs_cnl_dsp_interrupt,
 	.irq_handler = avs_irq_handler,
 	.irq_thread = avs_cnl_irq_thread,
 	.int_control = avs_dsp_interrupt_control,
diff --git a/sound/soc/intel/avs/skl.c b/sound/soc/intel/avs/skl.c
index d19f8953993f..25abfead9f63 100644
--- a/sound/soc/intel/avs/skl.c
+++ b/sound/soc/intel/avs/skl.c
@@ -10,6 +10,7 @@
 #include <linux/slab.h>
 #include <sound/hdaudio_ext.h>
 #include "avs.h"
+#include "cldma.h"
 #include "messages.h"
 
 irqreturn_t avs_skl_irq_thread(struct avs_dev *adev)
@@ -37,6 +38,66 @@ irqreturn_t avs_skl_irq_thread(struct avs_dev *adev)
 	return IRQ_HANDLED;
 }
 
+void avs_skl_ipc_interrupt(struct avs_dev *adev)
+{
+	const struct avs_spec *spec = adev->spec;
+	u32 hipc_ack, hipc_rsp;
+
+	snd_hdac_adsp_updatel(adev, spec->hipc->ctl_offset,
+			      AVS_ADSP_HIPCCTL_DONE | AVS_ADSP_HIPCCTL_BUSY, 0);
+
+	hipc_ack = snd_hdac_adsp_readl(adev, spec->hipc->ack_offset);
+	hipc_rsp = snd_hdac_adsp_readl(adev, spec->hipc->rsp_offset);
+
+	/* DSP acked host's request. */
+	if (hipc_ack & spec->hipc->ack_done_mask) {
+		complete(&adev->ipc->done_completion);
+
+		/* Tell DSP it has our attention. */
+		snd_hdac_adsp_updatel(adev, spec->hipc->ack_offset, spec->hipc->ack_done_mask,
+				      spec->hipc->ack_done_mask);
+	}
+
+	/* DSP sent new response to process */
+	if (hipc_rsp & spec->hipc->rsp_busy_mask) {
+		union avs_reply_msg msg;
+
+		msg.primary = snd_hdac_adsp_readl(adev, SKL_ADSP_REG_HIPCT);
+		msg.ext.val = snd_hdac_adsp_readl(adev, SKL_ADSP_REG_HIPCTE);
+
+		avs_dsp_process_response(adev, msg.val);
+
+		/* Tell DSP we accepted its message. */
+		snd_hdac_adsp_updatel(adev, SKL_ADSP_REG_HIPCT, SKL_ADSP_HIPCT_BUSY,
+				      SKL_ADSP_HIPCT_BUSY);
+	}
+
+	snd_hdac_adsp_updatel(adev, spec->hipc->ctl_offset,
+			      AVS_ADSP_HIPCCTL_DONE | AVS_ADSP_HIPCCTL_BUSY,
+			      AVS_ADSP_HIPCCTL_DONE | AVS_ADSP_HIPCCTL_BUSY);
+}
+
+static irqreturn_t avs_skl_dsp_interrupt(struct avs_dev *adev)
+{
+	u32 adspis = snd_hdac_adsp_readl(adev, AVS_ADSP_REG_ADSPIS);
+	irqreturn_t ret = IRQ_NONE;
+
+	if (adspis == UINT_MAX)
+		return ret;
+
+	if (adspis & AVS_ADSP_ADSPIS_CLDMA) {
+		hda_cldma_interrupt(&code_loader);
+		ret = IRQ_HANDLED;
+	}
+
+	if (adspis & AVS_ADSP_ADSPIS_IPC) {
+		avs_skl_ipc_interrupt(adev);
+		ret = IRQ_HANDLED;
+	}
+
+	return ret;
+}
+
 static int __maybe_unused
 avs_skl_enable_logs(struct avs_dev *adev, enum avs_log_enable enable, u32 aging_period,
 		    u32 fifo_full_period, unsigned long resource_mask, u32 *priorities)
@@ -128,6 +189,7 @@ const struct avs_dsp_ops avs_skl_dsp_ops = {
 	.power = avs_dsp_core_power,
 	.reset = avs_dsp_core_reset,
 	.stall = avs_dsp_core_stall,
+	.dsp_interrupt = avs_skl_dsp_interrupt,
 	.irq_handler = avs_irq_handler,
 	.irq_thread = avs_skl_irq_thread,
 	.int_control = avs_dsp_interrupt_control,
diff --git a/sound/soc/intel/avs/tgl.c b/sound/soc/intel/avs/tgl.c
index 0e052e7f6bac..e3723ef38a33 100644
--- a/sound/soc/intel/avs/tgl.c
+++ b/sound/soc/intel/avs/tgl.c
@@ -39,6 +39,7 @@ const struct avs_dsp_ops avs_tgl_dsp_ops = {
 	.power = avs_tgl_dsp_core_power,
 	.reset = avs_tgl_dsp_core_reset,
 	.stall = avs_tgl_dsp_core_stall,
+	.dsp_interrupt = avs_cnl_dsp_interrupt,
 	.irq_handler = avs_irq_handler,
 	.irq_thread = avs_cnl_irq_thread,
 	.int_control = avs_dsp_interrupt_control,
-- 
2.25.1

[PATCH 2/2] ASoC: Intel: avs: Remove unused IRQ-related code

[Cezary Rojewski]

Most IRQ-related code is duplicated in the driver. Switch to the new
implementation and remove unused members.

While the change is non-trivial, from functional perspective status quo
is achieved.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/apl.c   |  2 -
 sound/soc/intel/avs/avs.h   |  5 --
 sound/soc/intel/avs/cldma.c | 45 ++----------------
 sound/soc/intel/avs/cnl.c   | 34 --------------
 sound/soc/intel/avs/core.c  | 91 ++-----------------------------------
 sound/soc/intel/avs/icl.c   |  2 -
 sound/soc/intel/avs/ipc.c   | 48 -------------------
 sound/soc/intel/avs/skl.c   | 27 -----------
 sound/soc/intel/avs/tgl.c   |  2 -
 9 files changed, 7 insertions(+), 249 deletions(-)

diff --git a/sound/soc/intel/avs/apl.c b/sound/soc/intel/avs/apl.c
index a186d88430b9..bf97e4e428a4 100644
--- a/sound/soc/intel/avs/apl.c
+++ b/sound/soc/intel/avs/apl.c
@@ -255,8 +255,6 @@ const struct avs_dsp_ops avs_apl_dsp_ops = {
 	.reset = avs_dsp_core_reset,
 	.stall = avs_dsp_core_stall,
 	.dsp_interrupt = avs_apl_dsp_interrupt,
-	.irq_handler = avs_irq_handler,
-	.irq_thread = avs_skl_irq_thread,
 	.int_control = avs_dsp_interrupt_control,
 	.load_basefw = avs_hda_load_basefw,
 	.load_lib = avs_hda_load_library,
diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h
index 75ae8f3addb8..c905ecd0a108 100644
--- a/sound/soc/intel/avs/avs.h
+++ b/sound/soc/intel/avs/avs.h
@@ -47,8 +47,6 @@ struct avs_dsp_ops {
 	int (* const reset)(struct avs_dev *, u32, bool);
 	int (* const stall)(struct avs_dev *, u32, bool);
 	irqreturn_t (* const dsp_interrupt)(struct avs_dev *);
-	irqreturn_t (* const irq_handler)(struct avs_dev *);
-	irqreturn_t (* const irq_thread)(struct avs_dev *);
 	void (* const int_control)(struct avs_dev *, bool);
 	int (* const load_basefw)(struct avs_dev *, struct firmware *);
 	int (* const load_lib)(struct avs_dev *, struct firmware *, u32);
@@ -246,7 +244,6 @@ struct avs_ipc {
 #define AVS_IPC_RET(ret) \
 	(((ret) <= 0) ? (ret) : -AVS_EIPC)
 
-irqreturn_t avs_irq_handler(struct avs_dev *adev);
 void avs_dsp_process_response(struct avs_dev *adev, u64 header);
 int avs_dsp_send_msg_timeout(struct avs_dev *adev, struct avs_ipc_msg *request,
 			     struct avs_ipc_msg *reply, int timeout, const char *name);
@@ -268,8 +265,6 @@ void avs_ipc_block(struct avs_ipc *ipc);
 int avs_dsp_disable_d0ix(struct avs_dev *adev);
 int avs_dsp_enable_d0ix(struct avs_dev *adev);
 
-irqreturn_t avs_skl_irq_thread(struct avs_dev *adev);
-irqreturn_t avs_cnl_irq_thread(struct avs_dev *adev);
 void avs_skl_ipc_interrupt(struct avs_dev *adev);
 irqreturn_t avs_cnl_dsp_interrupt(struct avs_dev *adev);
 int avs_apl_enable_logs(struct avs_dev *adev, enum avs_log_enable enable, u32 aging_period,
diff --git a/sound/soc/intel/avs/cldma.c b/sound/soc/intel/avs/cldma.c
index c79b126f376d..945ea376d031 100644
--- a/sound/soc/intel/avs/cldma.c
+++ b/sound/soc/intel/avs/cldma.c
@@ -248,28 +248,6 @@ void hda_cldma_setup(struct hda_cldma *cl)
 	snd_hdac_stream_writel(cl, CL_SPBFCTL, 1);
 }
 
-static irqreturn_t cldma_irq_handler(int irq, void *dev_id)
-{
-	struct hda_cldma *cl = dev_id;
-	u32 adspis;
-
-	adspis = snd_hdac_adsp_readl(cl, AVS_ADSP_REG_ADSPIS);
-	if (adspis == UINT_MAX)
-		return IRQ_NONE;
-	if (!(adspis & AVS_ADSP_ADSPIS_CLDMA))
-		return IRQ_NONE;
-
-	cl->sd_status = snd_hdac_stream_readb(cl, SD_STS);
-	dev_warn(cl->dev, "%s sd_status: 0x%08x\n", __func__, cl->sd_status);
-
-	/* disable CLDMA interrupt */
-	snd_hdac_adsp_updatel(cl, AVS_ADSP_REG_ADSPIC, AVS_ADSP_ADSPIC_CLDMA, 0);
-
-	complete(&cl->completion);
-
-	return IRQ_HANDLED;
-}
-
 void hda_cldma_interrupt(struct hda_cldma *cl)
 {
 	/* disable CLDMA interrupt */
@@ -284,7 +262,6 @@ void hda_cldma_interrupt(struct hda_cldma *cl)
 int hda_cldma_init(struct hda_cldma *cl, struct hdac_bus *bus, void __iomem *dsp_ba,
 		   unsigned int buffer_size)
 {
-	struct pci_dev *pci = to_pci_dev(bus->dev);
 	int ret;
 
 	ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG, bus->dev, buffer_size, &cl->dmab_data);
@@ -292,8 +269,10 @@ int hda_cldma_init(struct hda_cldma *cl, struct hdac_bus *bus, void __iomem *dsp
 		return ret;
 
 	ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, bus->dev, BDL_SIZE, &cl->dmab_bdl);
-	if (ret < 0)
-		goto alloc_err;
+	if (ret < 0) {
+		snd_dma_free_pages(&cl->dmab_data);
+		return ret;
+	}
 
 	cl->dev = bus->dev;
 	cl->bus = bus;
@@ -301,27 +280,11 @@ int hda_cldma_init(struct hda_cldma *cl, struct hdac_bus *bus, void __iomem *dsp
 	cl->buffer_size = buffer_size;
 	cl->sd_addr = dsp_ba + AZX_CL_SD_BASE;
 
-	ret = pci_request_irq(pci, 0, cldma_irq_handler, NULL, cl, "CLDMA");
-	if (ret < 0) {
-		dev_err(cl->dev, "Failed to request CLDMA IRQ handler: %d\n", ret);
-		goto req_err;
-	}
-
 	return 0;
-
-req_err:
-	snd_dma_free_pages(&cl->dmab_bdl);
-alloc_err:
-	snd_dma_free_pages(&cl->dmab_data);
-
-	return ret;
 }
 
 void hda_cldma_free(struct hda_cldma *cl)
 {
-	struct pci_dev *pci = to_pci_dev(cl->dev);
-
-	pci_free_irq(pci, 0, cl);
 	snd_dma_free_pages(&cl->dmab_data);
 	snd_dma_free_pages(&cl->dmab_bdl);
 }
diff --git a/sound/soc/intel/avs/cnl.c b/sound/soc/intel/avs/cnl.c
index c021c0f51a53..0d03e1e03c11 100644
--- a/sound/soc/intel/avs/cnl.c
+++ b/sound/soc/intel/avs/cnl.c
@@ -10,38 +10,6 @@
 #include "avs.h"
 #include "messages.h"
 
-irqreturn_t avs_cnl_irq_thread(struct avs_dev *adev)
-{
-	union avs_reply_msg msg;
-	u32 hipctdr, hipctdd, hipctda;
-
-	hipctdr = snd_hdac_adsp_readl(adev, CNL_ADSP_REG_HIPCTDR);
-	hipctdd = snd_hdac_adsp_readl(adev, CNL_ADSP_REG_HIPCTDD);
-
-	/* Ensure DSP sent new response to process. */
-	if (!(hipctdr & CNL_ADSP_HIPCTDR_BUSY))
-		return IRQ_NONE;
-
-	msg.primary = hipctdr;
-	msg.ext.val = hipctdd;
-	avs_dsp_process_response(adev, msg.val);
-
-	/* Tell DSP we accepted its message. */
-	snd_hdac_adsp_updatel(adev, CNL_ADSP_REG_HIPCTDR,
-			      CNL_ADSP_HIPCTDR_BUSY, CNL_ADSP_HIPCTDR_BUSY);
-	/* Ack this response. */
-	snd_hdac_adsp_updatel(adev, CNL_ADSP_REG_HIPCTDA,
-			      CNL_ADSP_HIPCTDA_DONE, CNL_ADSP_HIPCTDA_DONE);
-	/* HW might have been clock gated, give some time for change to propagate. */
-	snd_hdac_adsp_readl_poll(adev, CNL_ADSP_REG_HIPCTDA, hipctda,
-				 !(hipctda & CNL_ADSP_HIPCTDA_DONE), 10, 1000);
-	/* Unmask busy interrupt. */
-	snd_hdac_adsp_updatel(adev, CNL_ADSP_REG_HIPCCTL,
-			      AVS_ADSP_HIPCCTL_BUSY, AVS_ADSP_HIPCCTL_BUSY);
-
-	return IRQ_HANDLED;
-}
-
 static void avs_cnl_ipc_interrupt(struct avs_dev *adev)
 {
 	const struct avs_spec *spec = adev->spec;
@@ -109,8 +77,6 @@ const struct avs_dsp_ops avs_cnl_dsp_ops = {
 	.reset = avs_dsp_core_reset,
 	.stall = avs_dsp_core_stall,
 	.dsp_interrupt = avs_cnl_dsp_interrupt,
-	.irq_handler = avs_irq_handler,
-	.irq_thread = avs_cnl_irq_thread,
 	.int_control = avs_dsp_interrupt_control,
 	.load_basefw = avs_hda_load_basefw,
 	.load_lib = avs_hda_load_library,
diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
index 25759f4f0213..a059bb6888d8 100644
--- a/sound/soc/intel/avs/core.c
+++ b/sound/soc/intel/avs/core.c
@@ -257,85 +257,6 @@ static void hdac_update_stream(struct hdac_bus *bus, struct hdac_stream *stream)
 	}
 }
 
-static irqreturn_t hdac_bus_irq_handler(int irq, void *context)
-{
-	struct hdac_bus *bus = context;
-	u32 mask, int_enable;
-	u32 status;
-	int ret = IRQ_NONE;
-
-	if (!pm_runtime_active(bus->dev))
-		return ret;
-
-	spin_lock(&bus->reg_lock);
-
-	status = snd_hdac_chip_readl(bus, INTSTS);
-	if (status == 0 || status == UINT_MAX) {
-		spin_unlock(&bus->reg_lock);
-		return ret;
-	}
-
-	/* 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);
-	}
-
-	mask = (0x1 << bus->num_streams) - 1;
-
-	status = snd_hdac_chip_readl(bus, INTSTS);
-	status &= mask;
-	if (status) {
-		/* Disable stream interrupts; Re-enable in bottom half */
-		int_enable = snd_hdac_chip_readl(bus, INTCTL);
-		snd_hdac_chip_writel(bus, INTCTL, (int_enable & (~mask)));
-		ret = IRQ_WAKE_THREAD;
-	} else {
-		ret = IRQ_HANDLED;
-	}
-
-	spin_unlock(&bus->reg_lock);
-	return ret;
-}
-
-static irqreturn_t hdac_bus_irq_thread(int irq, void *context)
-{
-	struct hdac_bus *bus = context;
-	u32 status;
-	u32 int_enable;
-	u32 mask;
-	unsigned long flags;
-
-	status = snd_hdac_chip_readl(bus, INTSTS);
-
-	snd_hdac_bus_handle_stream_irq(bus, status, hdac_update_stream);
-
-	/* Re-enable stream interrupts */
-	mask = (0x1 << bus->num_streams) - 1;
-	spin_lock_irqsave(&bus->reg_lock, flags);
-	int_enable = snd_hdac_chip_readl(bus, INTCTL);
-	snd_hdac_chip_writel(bus, INTCTL, (int_enable | mask));
-	spin_unlock_irqrestore(&bus->reg_lock, flags);
-
-	return IRQ_HANDLED;
-}
-
-static irqreturn_t avs_dsp_irq_handler(int irq, void *dev_id)
-{
-	struct avs_dev *adev = dev_id;
-
-	return avs_dsp_op(adev, irq_handler);
-}
-
-static irqreturn_t avs_dsp_irq_thread(int irq, void *dev_id)
-{
-	struct avs_dev *adev = dev_id;
-
-	return avs_dsp_op(adev, irq_thread);
-}
-
 static irqreturn_t avs_hda_interrupt(struct hdac_bus *bus)
 {
 	irqreturn_t ret = IRQ_NONE;
@@ -359,7 +280,6 @@ static irqreturn_t avs_hda_interrupt(struct hdac_bus *bus)
 	return ret;
 }
 
-__maybe_unused
 static irqreturn_t avs_hda_irq_handler(int irq, void *dev_id)
 {
 	struct hdac_bus *bus = dev_id;
@@ -375,7 +295,6 @@ static irqreturn_t avs_hda_irq_handler(int irq, void *dev_id)
 	return IRQ_WAKE_THREAD;
 }
 
-__maybe_unused
 static irqreturn_t avs_hda_irq_thread(int irq, void *dev_id)
 {
 	struct hdac_bus *bus = dev_id;
@@ -391,16 +310,14 @@ static irqreturn_t avs_hda_irq_thread(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-__maybe_unused
-static irqreturn_t avs_dsp_irq_handler2(int irq, void *dev_id)
+static irqreturn_t avs_dsp_irq_handler(int irq, void *dev_id)
 {
 	struct avs_dev *adev = dev_id;
 
 	return avs_hda_irq_handler(irq, &adev->base.core);
 }
 
-__maybe_unused
-static irqreturn_t avs_dsp_irq_thread2(int irq, void *dev_id)
+static irqreturn_t avs_dsp_irq_thread(int irq, void *dev_id)
 {
 	struct avs_dev *adev = dev_id;
 	struct hdac_bus *bus = &adev->base.core;
@@ -429,7 +346,7 @@ static int avs_hdac_acquire_irq(struct avs_dev *adev)
 		return ret;
 	}
 
-	ret = pci_request_irq(pci, 0, hdac_bus_irq_handler, hdac_bus_irq_thread, bus,
+	ret = pci_request_irq(pci, 0, avs_hda_irq_handler, avs_hda_irq_thread, bus,
 			      KBUILD_MODNAME);
 	if (ret < 0) {
 		dev_err(adev->dev, "Failed to request stream IRQ handler: %d\n", ret);
@@ -610,8 +527,6 @@ static void avs_pci_shutdown(struct pci_dev *pci)
 	snd_hdac_bus_stop_chip(bus);
 	snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
 
-	if (avs_platattr_test(adev, CLDMA))
-		pci_free_irq(pci, 0, &code_loader);
 	pci_free_irq(pci, 0, adev);
 	pci_free_irq(pci, 0, bus);
 	pci_free_irq_vectors(pci);
diff --git a/sound/soc/intel/avs/icl.c b/sound/soc/intel/avs/icl.c
index 004fe71505dd..3a96c4304ca6 100644
--- a/sound/soc/intel/avs/icl.c
+++ b/sound/soc/intel/avs/icl.c
@@ -189,8 +189,6 @@ const struct avs_dsp_ops avs_icl_dsp_ops = {
 	.reset = avs_dsp_core_reset,
 	.stall = avs_dsp_core_stall,
 	.dsp_interrupt = avs_cnl_dsp_interrupt,
-	.irq_handler = avs_irq_handler,
-	.irq_thread = avs_cnl_irq_thread,
 	.int_control = avs_dsp_interrupt_control,
 	.load_basefw = avs_icl_load_basefw,
 	.load_lib = avs_hda_load_library,
diff --git a/sound/soc/intel/avs/ipc.c b/sound/soc/intel/avs/ipc.c
index ad0e535b3c2e..f9b302215c37 100644
--- a/sound/soc/intel/avs/ipc.c
+++ b/sound/soc/intel/avs/ipc.c
@@ -301,54 +301,6 @@ void avs_dsp_process_response(struct avs_dev *adev, u64 header)
 	complete(&ipc->busy_completion);
 }
 
-irqreturn_t avs_irq_handler(struct avs_dev *adev)
-{
-	struct avs_ipc *ipc = adev->ipc;
-	const struct avs_spec *const spec = adev->spec;
-	u32 adspis, hipc_rsp, hipc_ack;
-	irqreturn_t ret = IRQ_NONE;
-
-	adspis = snd_hdac_adsp_readl(adev, AVS_ADSP_REG_ADSPIS);
-	if (adspis == UINT_MAX || !(adspis & AVS_ADSP_ADSPIS_IPC))
-		return ret;
-
-	hipc_ack = snd_hdac_adsp_readl(adev, spec->hipc->ack_offset);
-	hipc_rsp = snd_hdac_adsp_readl(adev, spec->hipc->rsp_offset);
-
-	/* DSP acked host's request */
-	if (hipc_ack & spec->hipc->ack_done_mask) {
-		/*
-		 * As an extra precaution, mask done interrupt. Code executed
-		 * due to complete() found below does not assume any masking.
-		 */
-		snd_hdac_adsp_updatel(adev, spec->hipc->ctl_offset,
-				      AVS_ADSP_HIPCCTL_DONE, 0);
-
-		complete(&ipc->done_completion);
-
-		/* tell DSP it has our attention */
-		snd_hdac_adsp_updatel(adev, spec->hipc->ack_offset,
-				      spec->hipc->ack_done_mask,
-				      spec->hipc->ack_done_mask);
-		/* unmask done interrupt */
-		snd_hdac_adsp_updatel(adev, spec->hipc->ctl_offset,
-				      AVS_ADSP_HIPCCTL_DONE,
-				      AVS_ADSP_HIPCCTL_DONE);
-		ret = IRQ_HANDLED;
-	}
-
-	/* DSP sent new response to process */
-	if (hipc_rsp & spec->hipc->rsp_busy_mask) {
-		/* mask busy interrupt */
-		snd_hdac_adsp_updatel(adev, spec->hipc->ctl_offset,
-				      AVS_ADSP_HIPCCTL_BUSY, 0);
-
-		ret = IRQ_WAKE_THREAD;
-	}
-
-	return ret;
-}
-
 static bool avs_ipc_is_busy(struct avs_ipc *ipc)
 {
 	struct avs_dev *adev = to_avs_dev(ipc->dev);
diff --git a/sound/soc/intel/avs/skl.c b/sound/soc/intel/avs/skl.c
index 25abfead9f63..fceed353449a 100644
--- a/sound/soc/intel/avs/skl.c
+++ b/sound/soc/intel/avs/skl.c
@@ -13,31 +13,6 @@
 #include "cldma.h"
 #include "messages.h"
 
-irqreturn_t avs_skl_irq_thread(struct avs_dev *adev)
-{
-	union avs_reply_msg msg;
-	u32 hipct, hipcte;
-
-	hipct = snd_hdac_adsp_readl(adev, SKL_ADSP_REG_HIPCT);
-	hipcte = snd_hdac_adsp_readl(adev, SKL_ADSP_REG_HIPCTE);
-
-	/* Ensure DSP sent new response to process. */
-	if (!(hipct & SKL_ADSP_HIPCT_BUSY))
-		return IRQ_NONE;
-
-	msg.primary = hipct;
-	msg.ext.val = hipcte;
-	avs_dsp_process_response(adev, msg.val);
-
-	/* Tell DSP we accepted its message. */
-	snd_hdac_adsp_updatel(adev, SKL_ADSP_REG_HIPCT, SKL_ADSP_HIPCT_BUSY, SKL_ADSP_HIPCT_BUSY);
-	/* Unmask busy interrupt. */
-	snd_hdac_adsp_updatel(adev, SKL_ADSP_REG_HIPCCTL, AVS_ADSP_HIPCCTL_BUSY,
-			      AVS_ADSP_HIPCCTL_BUSY);
-
-	return IRQ_HANDLED;
-}
-
 void avs_skl_ipc_interrupt(struct avs_dev *adev)
 {
 	const struct avs_spec *spec = adev->spec;
@@ -190,8 +165,6 @@ const struct avs_dsp_ops avs_skl_dsp_ops = {
 	.reset = avs_dsp_core_reset,
 	.stall = avs_dsp_core_stall,
 	.dsp_interrupt = avs_skl_dsp_interrupt,
-	.irq_handler = avs_irq_handler,
-	.irq_thread = avs_skl_irq_thread,
 	.int_control = avs_dsp_interrupt_control,
 	.load_basefw = avs_cldma_load_basefw,
 	.load_lib = avs_cldma_load_library,
diff --git a/sound/soc/intel/avs/tgl.c b/sound/soc/intel/avs/tgl.c
index e3723ef38a33..b985a8299b72 100644
--- a/sound/soc/intel/avs/tgl.c
+++ b/sound/soc/intel/avs/tgl.c
@@ -40,8 +40,6 @@ const struct avs_dsp_ops avs_tgl_dsp_ops = {
 	.reset = avs_tgl_dsp_core_reset,
 	.stall = avs_tgl_dsp_core_stall,
 	.dsp_interrupt = avs_cnl_dsp_interrupt,
-	.irq_handler = avs_irq_handler,
-	.irq_thread = avs_cnl_irq_thread,
 	.int_control = avs_dsp_interrupt_control,
 	.load_basefw = avs_icl_load_basefw,
 	.load_lib = avs_hda_load_library,
-- 
2.25.1

Re: [PATCH 0/2] ASoC: Intel: avs: Refactor IRQ handling

[Mark Brown]

On Fri, 19 Apr 2024 10:48:55 +0200, Cezary Rojewski wrote:
> The existing code can be both improved and simplified. To make this
> change easier to manage, first add new implementation and then remove
> deadcode in a separate patch.
> 
> Simplification achieved with:
> 
> - reduce the amount of resources requested by the driver i.e.: IPC and
>   CLDMA request_irq() merged into one
> - reduce the number of DSP ops from 2 to 1:
>   irq_handler/thread() vs dsp_interrupt()
> - drop ambiguity around CLDMA interrupt, let skl.c handle that
>   explicitly as it is the only user
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/2] ASoC: Intel: avs: New IRQ handling implementation
      commit: 7ce6ceeb77bfd9fb0b22203190bd6f57fe917b51
[2/2] ASoC: Intel: avs: Remove unused IRQ-related code
      commit: 84049e2db59ad9b09461b6d7ec56bd3e8fe75eca

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark