diff mbox series

[V2,12/23] ASoC: amd: acp70: add acp ip interrupt handler

Message ID 20250120100130.3710412-13-Vijendar.Mukunda@amd.com
State New
Headers show
Series None | expand

Commit Message

Vijendar Mukunda Jan. 20, 2025, 10:01 a.m. UTC
Add interrupt handler for handling, below listed interrupts for ACP IP.
- SoundWire dma driver interrupts
- ACP PDM dma driver interrupts
- SoundWire manager related interrupts
- ACP error interrupts.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 sound/soc/amd/acp70/acp70.h     |   4 +
 sound/soc/amd/acp70/pci-acp70.c | 161 +++++++++++++++++++++++++++++++-
 2 files changed, 164 insertions(+), 1 deletion(-)

Comments

Mark Brown Jan. 20, 2025, 5:55 p.m. UTC | #1
On Mon, Jan 20, 2025 at 03:31:19PM +0530, Vijendar Mukunda wrote:

> +static irqreturn_t acp70_irq_thread(int irq, void *context)
> +{
> +	struct sdw_dma_dev_data *sdw_dma_data;
> +	struct acp70_dev_data *adata = context;
> +	u32 stream_index;
> +
> +	sdw_dma_data = dev_get_drvdata(&adata->sdw_dma_dev->dev);
> +
> +	for (stream_index = 0; stream_index < ACP70_SDW0_DMA_MAX_STREAMS; stream_index++) {
> +		if (adata->sdw0_dma_intr_stat[stream_index]) {
> +			if (sdw_dma_data->sdw0_dma_stream[stream_index])
> +				snd_pcm_period_elapsed(sdw_dma_data->sdw0_dma_stream[stream_index]);
> +			adata->sdw0_dma_intr_stat[stream_index] = 0;
> +		}
> +	}
> +	for (stream_index = 0; stream_index < ACP70_SDW1_DMA_MAX_STREAMS; stream_index++) {
> +		if (adata->sdw1_dma_intr_stat[stream_index]) {
> +			if (sdw_dma_data->sdw1_dma_stream[stream_index])
> +				snd_pcm_period_elapsed(sdw_dma_data->sdw1_dma_stream[stream_index]);
> +			adata->sdw1_dma_intr_stat[stream_index] = 0;
> +		}
> +	}
> +	return IRQ_HANDLED;
> +}

I appreciate that this pattern is already (identically...) in the ps
driver but I'm not seeing anything here that looks like it can't run in
interrupt context - is there a special reason for deferring to thread
context?

> +static irqreturn_t acp70_irq_handler(int irq, void *dev_id)
> +{

This really does seem *very* similar to the acp63 code...
Mark Brown Jan. 20, 2025, 6:39 p.m. UTC | #2
On Mon, Jan 20, 2025 at 11:48:00PM +0530, Mukunda,Vijendar wrote:
> On 20/01/25 23:25, Mark Brown wrote:

> >> +static irqreturn_t acp70_irq_handler(int irq, void *dev_id)
> >> +{

> > This really does seem *very* similar to the acp63 code...

> Compared to PS, in ACP70 wake interrupt logic added.
> New register fields got introduced inACP70 compared to PS.
> Please refer below patch.
> https://lore.kernel.org/lkml/20250120100130.3710412-18-Vijendar.Mukunda@amd.com/
> Let this patch series go as initial version for ACP70 platform.
> We will revisit the code and implement common helper functions in the next cycle.

That does feel like quirks and new features rather than a completely
distinct IP.
Mark Brown Jan. 20, 2025, 7:28 p.m. UTC | #3
On Mon, Jan 20, 2025 at 12:47:18PM -0600, Mario Limonciello wrote:
> On 1/20/2025 12:39, Mark Brown wrote:

> > That does feel like quirks and new features rather than a completely
> > distinct IP.

> I see it as different forms of tech debt.  Either you keep track of which
> features the 62 vs 70 hardware supports by different drivers or add logic in
> all the relevant functions().

> The former increases LoC but reduces risk for mistake (IE avoid oops, I
> forgot this is only supported on 70+ when adding new features).

Until someone fixes a bug or does some subsystem wide cleanup which
affects both copies of the code (perhaps that already happened since the
code was copied!).  There's a reason why this is the general kernel
style.

> Changing code that affects a lot of hardware means a lot more testing too.
> Perhaps after Vijendar's series lands he can split up some of the purely
> duplicated functions into helpers or callbacks and arrange all that testing?

Well, it was getting a new spin anyway for the bits that didn't have the
serial numbers filed off.
diff mbox series

Patch

diff --git a/sound/soc/amd/acp70/acp70.h b/sound/soc/amd/acp70/acp70.h
index a96b021e64da..d5ab606adedc 100644
--- a/sound/soc/amd/acp70/acp70.h
+++ b/sound/soc/amd/acp70/acp70.h
@@ -238,6 +238,8 @@  struct sdw_dma_dev_data {
  * @addr: pci ioremap address
  * @reg_range: ACP reigister range
  * @acp_rev : ACP PCI revision id
+ * @sdw0-dma_intr_stat: DMA interrupt status array for SoundWire manager-SW0 instance
+ * @sdw_dma_intr_stat: DMA interrupt status array for SoundWire manager-SW1 instance
  * @is_sdw_dev: flag set to true when any SoundWire manager instances are available
  * @is_pdm_dev: flag set to true when ACP PDM controller exists
  * @is_pdm_config: flat set to true when PDM configuration is selected from BIOS
@@ -257,6 +259,8 @@  struct acp70_dev_data {
 	u32 addr;
 	u32 reg_range;
 	u32 acp_rev;
+	u16 sdw0_dma_intr_stat[ACP70_SDW0_DMA_MAX_STREAMS];
+	u16 sdw1_dma_intr_stat[ACP70_SDW1_DMA_MAX_STREAMS];
 	bool is_sdw_dev;
 	bool is_pdm_dev;
 	bool is_pdm_config;
diff --git a/sound/soc/amd/acp70/pci-acp70.c b/sound/soc/amd/acp70/pci-acp70.c
index a6812fa269b1..e732a680c092 100644
--- a/sound/soc/amd/acp70/pci-acp70.c
+++ b/sound/soc/amd/acp70/pci-acp70.c
@@ -8,11 +8,13 @@ 
 #include <linux/acpi.h>
 #include <linux/bitops.h>
 #include <linux/delay.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <sound/pcm_params.h>
 #include "../mach-config.h"
 
 #include "acp70.h"
@@ -96,6 +98,155 @@  static int acp70_deinit(void __iomem *acp_base, struct device *dev)
 	return 0;
 }
 
+static irqreturn_t acp70_irq_thread(int irq, void *context)
+{
+	struct sdw_dma_dev_data *sdw_dma_data;
+	struct acp70_dev_data *adata = context;
+	u32 stream_index;
+
+	sdw_dma_data = dev_get_drvdata(&adata->sdw_dma_dev->dev);
+
+	for (stream_index = 0; stream_index < ACP70_SDW0_DMA_MAX_STREAMS; stream_index++) {
+		if (adata->sdw0_dma_intr_stat[stream_index]) {
+			if (sdw_dma_data->sdw0_dma_stream[stream_index])
+				snd_pcm_period_elapsed(sdw_dma_data->sdw0_dma_stream[stream_index]);
+			adata->sdw0_dma_intr_stat[stream_index] = 0;
+		}
+	}
+	for (stream_index = 0; stream_index < ACP70_SDW1_DMA_MAX_STREAMS; stream_index++) {
+		if (adata->sdw1_dma_intr_stat[stream_index]) {
+			if (sdw_dma_data->sdw1_dma_stream[stream_index])
+				snd_pcm_period_elapsed(sdw_dma_data->sdw1_dma_stream[stream_index]);
+			adata->sdw1_dma_intr_stat[stream_index] = 0;
+		}
+	}
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t acp70_irq_handler(int irq, void *dev_id)
+{
+	struct acp70_dev_data *adata;
+	struct pdm_dev_data *ps_pdm_data;
+	struct amd_sdw_manager *amd_manager;
+	u32 ext_intr_stat, ext_intr_stat1;
+	u32 stream_id = 0;
+	u16 irq_flag = 0;
+	u16 sdw_dma_irq_flag = 0;
+	u16 index;
+
+	adata = dev_id;
+	if (!adata)
+		return IRQ_NONE;
+	/* ACP interrupts will be cleared by reading particular bit and writing
+	 * same value to the status register. writing zero's doesn't have any
+	 * effect.
+	 * Bit by bit checking of IRQ field is implemented.
+	 */
+	ext_intr_stat = readl(adata->acp70_base + ACP_EXTERNAL_INTR_STAT);
+	if (ext_intr_stat & ACP_SDW0_STAT) {
+		writel(ACP_SDW0_STAT, adata->acp70_base + ACP_EXTERNAL_INTR_STAT);
+		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
+		if (amd_manager)
+			schedule_work(&amd_manager->amd_sdw_irq_thread);
+		irq_flag = 1;
+	}
+
+	ext_intr_stat1 = readl(adata->acp70_base + ACP_EXTERNAL_INTR_STAT1);
+	if (ext_intr_stat1 & ACP_SDW1_STAT) {
+		writel(ACP_SDW1_STAT, adata->acp70_base + ACP_EXTERNAL_INTR_STAT1);
+		amd_manager = dev_get_drvdata(&adata->sdw->pdev[1]->dev);
+		if (amd_manager)
+			schedule_work(&amd_manager->amd_sdw_irq_thread);
+		irq_flag = 1;
+	}
+
+	if (ext_intr_stat & ACP_ERROR_IRQ) {
+		writel(ACP_ERROR_IRQ, adata->acp70_base + ACP_EXTERNAL_INTR_STAT);
+		writel(0, adata->acp70_base + ACP_SW0_I2S_ERROR_REASON);
+		writel(0, adata->acp70_base + ACP_SW1_I2S_ERROR_REASON);
+		writel(0, adata->acp70_base + ACP_ERROR_STATUS);
+		irq_flag = 1;
+	}
+
+	if (ext_intr_stat & BIT(PDM_DMA_STAT)) {
+		ps_pdm_data = dev_get_drvdata(&adata->pdm_dev->dev);
+		writel(BIT(PDM_DMA_STAT), adata->acp70_base + ACP_EXTERNAL_INTR_STAT);
+		if (ps_pdm_data->capture_stream)
+			snd_pcm_period_elapsed(ps_pdm_data->capture_stream);
+		irq_flag = 1;
+	}
+
+	if (ext_intr_stat & ACP_SDW_DMA_IRQ_MASK) {
+		for (index = ACP_AUDIO2_RX_THRESHOLD; index <= ACP_AUDIO0_TX_THRESHOLD; index++) {
+			if (ext_intr_stat & BIT(index)) {
+				writel(BIT(index), adata->acp70_base + ACP_EXTERNAL_INTR_STAT);
+				switch (index) {
+				case ACP_AUDIO0_TX_THRESHOLD:
+					stream_id = ACP_SDW0_AUDIO0_TX;
+					break;
+				case ACP_AUDIO1_TX_THRESHOLD:
+					stream_id = ACP_SDW0_AUDIO1_TX;
+					break;
+				case ACP_AUDIO2_TX_THRESHOLD:
+					stream_id = ACP_SDW0_AUDIO2_TX;
+					break;
+				case ACP_AUDIO0_RX_THRESHOLD:
+					stream_id = ACP_SDW0_AUDIO0_RX;
+					break;
+				case ACP_AUDIO1_RX_THRESHOLD:
+					stream_id = ACP_SDW0_AUDIO1_RX;
+					break;
+				case ACP_AUDIO2_RX_THRESHOLD:
+					stream_id = ACP_SDW0_AUDIO2_RX;
+					break;
+				}
+
+				adata->sdw0_dma_intr_stat[stream_id] = 1;
+				sdw_dma_irq_flag = 1;
+			}
+		}
+	}
+
+	if (ext_intr_stat1 & ACP_P1_SDW_DMA_IRQ_MASK) {
+		for (index = ACP_P1_AUDIO2_RX_THRESHOLD; index <= ACP_P1_AUDIO0_TX_THRESHOLD; index++) {
+			if (ext_intr_stat1 & BIT(index)) {
+				writel(BIT(index), adata->acp70_base + ACP_EXTERNAL_INTR_STAT1);
+				switch (index) {
+				case ACP_P1_AUDIO0_TX_THRESHOLD:
+					stream_id = ACP_SDW1_AUDIO0_TX;
+					break;
+				case ACP_P1_AUDIO1_TX_THRESHOLD:
+					stream_id = ACP_SDW1_AUDIO1_TX;
+					break;
+				case ACP_P1_AUDIO2_TX_THRESHOLD:
+					stream_id = ACP_SDW1_AUDIO2_TX;
+					break;
+				case ACP_P1_AUDIO0_RX_THRESHOLD:
+					stream_id = ACP_SDW1_AUDIO0_RX;
+					break;
+				case ACP_P1_AUDIO1_RX_THRESHOLD:
+					stream_id = ACP_SDW1_AUDIO1_RX;
+					break;
+				case ACP_P1_AUDIO2_RX_THRESHOLD:
+					stream_id = ACP_SDW1_AUDIO2_RX;
+					break;
+				}
+
+				adata->sdw1_dma_intr_stat[stream_id] = 1;
+				sdw_dma_irq_flag = 1;
+			}
+		}
+	}
+
+	if (sdw_dma_irq_flag)
+		return IRQ_WAKE_THREAD;
+
+	if (irq_flag)
+		return IRQ_HANDLED;
+	else
+		return IRQ_NONE;
+}
+
 #if IS_ENABLED(CONFIG_SND_SOC_AMD_SOUNDWIRE)
 static int acp_scan_sdw_devices(struct device *dev, u64 addr)
 {
@@ -333,8 +484,11 @@  static int snd_acp70_probe(struct pci_dev *pci,
 {
 	struct acp70_dev_data *adata;
 	u32 addr, flag;
+	u32 irqflags;
 	int ret;
 
+	irqflags = IRQF_SHARED;
+
 	/* Return if acp config flag is defined */
 	flag = snd_amd_acp_find_config(pci);
 	if (flag)
@@ -382,7 +536,12 @@  static int snd_acp70_probe(struct pci_dev *pci,
 	ret = acp70_init(adata->acp70_base, &pci->dev);
 	if (ret)
 		goto release_regions;
-
+	ret = devm_request_threaded_irq(&pci->dev, pci->irq, acp70_irq_handler,
+					acp70_irq_thread, irqflags, "ACP_PCI_IRQ", adata);
+	if (ret) {
+		dev_err(&pci->dev, "ACP PCI IRQ request failed\n");
+		goto de_init;
+	}
 	ret = get_acp70_device_config(pci, adata);
 	/* ACP PCI driver probe should be continued even PDM or SoundWire Devices are not found */
 	if (ret) {