[PATCH V2 0/9] ASoC: amd: ps: add SoundWire support

[PATCH V2 0/9] ASoC: amd: ps: add SoundWire support

[Vijendar Mukunda]

This patch series add support for
	- Platform device creation for SoundWire Manager instances and
	  PDM controller.
	- SoundWire DMA driver.
	- Interrupt handling for SoundWire manager related interrupts,
	  SoundWire DMA interrupts and ACP error interrupts.
	- ACP PCI driver PM ops modification with respect to SoundWire
	  Power modes.

Changes since v1:
	- update "soundwire" as "SoundWire" in code.
	- add comments for platform device mask and platform device
	  count
	- remove unused variables in acp pci driver private data
	  structure
	- refactor dma enable register structures in SoundWire DMA driver
	- add TODO comments in IRQ handler
	- update IRQ mask values using bit macros
	- Fix build error reported in Makefile
	- rename "sdw_dma_stream_instance" structure name as "acp_sdw_dma_stream"
 
Vijendar Mukunda (9):
  ASoC: amd: ps: create platform devices based on acp config
  ASoC: amd: ps: handle SoundWire interrupts in acp pci driver
  ASoC: amd: ps: add SoundWire dma driver
  ASoC: amd: ps: add SoundWire dma driver dma ops
  ASoC: amd: ps: add support for SoundWire DMA interrupts
  ASoC: amd: ps: add pm ops support for SoundWire dma driver
  ASoC: amd: ps: enable SoundWire dma driver build
  ASoC: amd: update comments in Kconfig file
  ASoC: amd: ps: Add SoundWire specific checks in pci driver in pm ops.

 sound/soc/amd/Kconfig         |   3 +-
 sound/soc/amd/ps/Makefile     |   2 +
 sound/soc/amd/ps/acp63.h      | 180 ++++++++++-
 sound/soc/amd/ps/pci-ps.c     | 396 +++++++++++++++++++++--
 sound/soc/amd/ps/ps-sdw-dma.c | 572 ++++++++++++++++++++++++++++++++++
 5 files changed, 1129 insertions(+), 24 deletions(-)
 create mode 100644 sound/soc/amd/ps/ps-sdw-dma.c

-- 
2.34.1

[PATCH V2 1/9] ASoC: amd: ps: create platform devices based on acp config

[Vijendar Mukunda]

Based on ACP pin configuration and scanning child devices
under ACP pci device ACPI scope, platform device mask(pdev_mask)
and platform device count(pdev_count) will be calculated.

Using pdev_mask and pdev_count values, ACP PCI driver will
create platform devices for Pink Sardine platform.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 sound/soc/amd/ps/acp63.h  |  81 +++++++++++-
 sound/soc/amd/ps/pci-ps.c | 252 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 318 insertions(+), 15 deletions(-)

diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
index 2f94448102d0..95bb1cef900a 100644
--- a/sound/soc/amd/ps/acp63.h
+++ b/sound/soc/amd/ps/acp63.h
@@ -10,7 +10,7 @@
 #define ACP_DEVICE_ID 0x15E2
 #define ACP63_REG_START		0x1240000
 #define ACP63_REG_END		0x1250200
-#define ACP63_DEVS		3
+#define ACP63_DEVS		5
 
 #define ACP_SOFT_RESET_SOFTRESET_AUDDONE_MASK	0x00010001
 #define ACP_PGFSM_CNTL_POWER_ON_MASK	1
@@ -53,11 +53,38 @@
 /* time in ms for runtime suspend delay */
 #define ACP_SUSPEND_DELAY_MS	2000
 
-#define ACP63_DMIC_ADDR		2
-#define ACP63_PDM_MODE_DEVS		3
-#define ACP63_PDM_DEV_MASK		1
 #define ACP_DMIC_DEV	2
 
+/* ACP63_PDM_MODE_DEVS corresponds to platform devices count for ACP PDM configuration */
+#define ACP63_PDM_MODE_DEVS		3
+
+/*
+ * ACP63_SDW0_MODE_DEVS corresponds to platform devices count for
+ * SW0 SoundWire manager instance configuration
+ */
+#define ACP63_SDW0_MODE_DEVS		2
+
+/*
+ * ACP63_SDW0_SDW1_MODE_DEVS corresponds to platform devices count for SW0 + SW1 SoundWire manager
+ * instances configuration
+ */
+#define ACP63_SDW0_SDW1_MODE_DEVS	3
+
+/*
+ * ACP63_SDW0_PDM_MODE_DEVS corresponds to platform devices count for SW0 manager
+ * instance + ACP PDM controller configuration
+ */
+#define ACP63_SDW0_PDM_MODE_DEVS	4
+
+/*
+ * ACP63_SDW0_SDW1_PDM_MODE_DEVS corresponds to platform devices count for
+ * SW0 + SW1 SoundWire manager instances + ACP PDM controller configuration
+ */
+#define ACP63_SDW0_SDW1_PDM_MODE_DEVS   5
+#define ACP63_DMIC_ADDR			2
+#define ACP63_SDW_ADDR			5
+#define AMD_SDW_MAX_MANAGERS		2
+
 /* time in ms for acp timeout */
 #define ACP_TIMEOUT		500
 
@@ -80,6 +107,28 @@ enum acp_config {
 	ACP_CONFIG_15,
 };
 
+/**
+ * acp_pdev_mask corresponds to platform device mask based on audio endpoint combinations.
+ * acp_pdev_mask will be calculated based on ACPI Scan under ACP PCI device and
+ * ACP PIN Configuration.
+ * Based acp_pdev_mask, platform devices will be created.
+ * Below are possible platform device combinations.
+ * 1) ACP PDM Controller, dmic-codec, machine driver platform device node
+ * 2) ACP PDM Controller , dmic-codec, SW0 SoundWire manager instance, platform device for
+ *    SoundWire DMA driver
+ * 3) SW0, SW1 SoundWire manager instances, platform device for SoundWire DMA driver
+ * 4) ACP PDM Controller, dmic-codec, SDW0, SDW1 manager instances, platform device for
+ *    SoundWire DMA driver
+ * ACP63_PDM_DEV_MASK corresponds to platform device mask for ACP PDM controller.
+ * ACP63_SDW_DEV_MASK corresponds to platform device mask for SDW manager instances.
+ * ACP63_SDW_PDM_DEV_MASK corresponds to platform device mask for ACP PDM + SDW manager combination
+ */
+enum acp_pdev_mask {
+	ACP63_PDM_DEV_MASK = 1,
+	ACP63_SDW_DEV_MASK,
+	ACP63_SDW_PDM_DEV_MASK,
+};
+
 struct pdm_stream_instance {
 	u16 num_pages;
 	u16 channels;
@@ -95,14 +144,38 @@ struct pdm_dev_data {
 	struct snd_pcm_substream *capture_stream;
 };
 
+/**
+ * struct acp63_dev_data - acp pci driver context
+ * @acp63_base: acp mmio base
+ * @res: resource
+ * @pdev: array of child platform device node structures
+ * @acp_lock: used to protect acp common registers
+ * @sdw_fw_node: SoundWire controller fw node handle
+ * @pdev_mask: platform device mask
+ * @pdev_count: platform devices count
+ * @pdm_dev_index: pdm platform device index
+ * @sdw_manager_count: SoundWire manager instance count
+ * @sdw0_dev_index: SoundWire Manager-0 platform device index
+ * @sdw1_dev_index: SoundWire Manager-1 platform device index
+ * @sdw_dma_dev_index: SoundWire DMA controller platform device index
+ * @acp_reset: flag set to true when bus reset is applied across all
+ * the active SoundWire manager instances
+ */
+
 struct acp63_dev_data {
 	void __iomem *acp63_base;
 	struct resource *res;
 	struct platform_device *pdev[ACP63_DEVS];
 	struct mutex acp_lock; /* protect shared registers */
+	struct fwnode_handle *sdw_fw_node;
 	u16 pdev_mask;
 	u16 pdev_count;
 	u16 pdm_dev_index;
+	u8 sdw_manager_count;
+	u16 sdw0_dev_index;
+	u16 sdw1_dev_index;
+	u16 sdw_dma_dev_index;
+	bool acp_reset;
 };
 
 int snd_amd_acp_find_config(struct pci_dev *pci);
diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
index c957718abefc..02caae6968ad 100644
--- a/sound/soc/amd/ps/pci-ps.c
+++ b/sound/soc/amd/ps/pci-ps.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/pci.h>
+#include <linux/bitops.h>
 #include <linux/module.h>
 #include <linux/io.h>
 #include <linux/delay.h>
@@ -15,6 +16,7 @@
 #include <sound/pcm_params.h>
 #include <linux/pm_runtime.h>
 #include <linux/iopoll.h>
+#include <linux/soundwire/sdw_amd.h>
 
 #include "acp63.h"
 
@@ -119,37 +121,158 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
 	return IRQ_NONE;
 }
 
-static void get_acp63_device_config(u32 config, struct pci_dev *pci,
-				    struct acp63_dev_data *acp_data)
+static int sdw_amd_scan_controller(struct device *dev)
+{
+	struct acp63_dev_data *acp_data;
+	struct fwnode_handle *link;
+	char name[32];
+	u32 sdw_manager_bitmap;
+	u8 count = 0;
+	u32 acp_sdw_power_mode = 0;
+	int index;
+	int ret;
+
+	acp_data = dev_get_drvdata(dev);
+	acp_data->acp_reset = true;
+	/* Found controller, find links supported */
+	ret = fwnode_property_read_u32_array((acp_data->sdw_fw_node), "mipi-sdw-manager-list",
+					     &sdw_manager_bitmap, 1);
+
+	if (ret) {
+		dev_err(dev, "Failed to read mipi-sdw-manager-list: %d\n", ret);
+		return -EINVAL;
+	}
+	count = hweight32(sdw_manager_bitmap);
+	/* Check count is within bounds */
+	if (count > AMD_SDW_MAX_MANAGERS) {
+		dev_err(dev, "Manager count %d exceeds max %d\n", count, AMD_SDW_MAX_MANAGERS);
+		return -EINVAL;
+	}
+
+	if (!count) {
+		dev_dbg(dev, "No SoundWire Managers detected\n");
+		return -EINVAL;
+	}
+	dev_dbg(dev, "ACPI reports %d SoundWire Manager devices\n", count);
+	acp_data->sdw_manager_count = count;
+	for (index = 0; index < count; index++) {
+		snprintf(name, sizeof(name), "mipi-sdw-link-%d-subproperties", index);
+		link = fwnode_get_named_child_node(acp_data->sdw_fw_node, name);
+		if (!link) {
+			dev_err(dev, "Manager node %s not found\n", name);
+			return -EIO;
+		}
+
+		fwnode_property_read_u32(link, "amd-sdw-power-mode",
+					 &acp_sdw_power_mode);
+		/*
+		 * when SoundWire configuration is selected from acp pin config,
+		 * based on manager instances count, acp init/de-init sequence should be
+		 * executed as part of PM ops only when Bus reset is applied for the active
+		 * SoundWire manager instances.
+		 */
+		if (acp_sdw_power_mode != AMD_SDW_POWER_OFF_MODE)
+			acp_data->acp_reset = false;
+	}
+	return 0;
+}
+
+static int get_acp63_device_config(u32 config, struct pci_dev *pci, struct acp63_dev_data *acp_data)
 {
 	struct acpi_device *dmic_dev;
+	struct acpi_device *sdw_dev;
 	const union acpi_object *obj;
 	bool is_dmic_dev = false;
+	bool is_sdw_dev = false;
+	int ret;
 
 	dmic_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_DMIC_ADDR, 0);
 	if (dmic_dev) {
+		/* is_dmic_dev flag will be set when ACP PDM controller device exists */
 		if (!acpi_dev_get_property(dmic_dev, "acp-audio-device-type",
 					   ACPI_TYPE_INTEGER, &obj) &&
 					   obj->integer.value == ACP_DMIC_DEV)
 			is_dmic_dev = true;
 	}
 
+	sdw_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_SDW_ADDR, 0);
+	if (sdw_dev) {
+		acp_data->sdw_fw_node = acpi_fwnode_handle(sdw_dev);
+		ret = sdw_amd_scan_controller(&pci->dev);
+		/* is_sdw_dev flag will be set when SoundWire Manager device exists */
+		if (!ret)
+			is_sdw_dev = true;
+	}
+
+	dev_dbg(&pci->dev, "Audio Mode %d\n", config);
 	switch (config) {
-	case ACP_CONFIG_0:
-	case ACP_CONFIG_1:
+	case ACP_CONFIG_4:
+	case ACP_CONFIG_5:
+	case ACP_CONFIG_10:
+	case ACP_CONFIG_11:
+		if (is_dmic_dev) {
+			acp_data->pdev_mask = ACP63_PDM_DEV_MASK;
+			acp_data->pdev_count = ACP63_PDM_MODE_DEVS;
+		}
+		break;
 	case ACP_CONFIG_2:
 	case ACP_CONFIG_3:
-	case ACP_CONFIG_9:
-	case ACP_CONFIG_15:
-		dev_dbg(&pci->dev, "Audio Mode %d\n", config);
+		if (is_sdw_dev) {
+			switch (acp_data->sdw_manager_count) {
+			case 1:
+				acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
+				acp_data->pdev_count = ACP63_SDW0_MODE_DEVS;
+				break;
+			case 2:
+				acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
+				acp_data->pdev_count = ACP63_SDW0_SDW1_MODE_DEVS;
+				break;
+			default:
+				return -EINVAL;
+			}
+		}
 		break;
-	default:
-		if (is_dmic_dev) {
+	case ACP_CONFIG_6:
+	case ACP_CONFIG_7:
+	case ACP_CONFIG_12:
+	case ACP_CONFIG_8:
+	case ACP_CONFIG_13:
+	case ACP_CONFIG_14:
+		if (is_dmic_dev && is_sdw_dev) {
+			switch (acp_data->sdw_manager_count) {
+			case 1:
+				acp_data->pdev_mask = ACP63_SDW_PDM_DEV_MASK;
+				acp_data->pdev_count = ACP63_SDW0_PDM_MODE_DEVS;
+				break;
+			case 2:
+				acp_data->pdev_mask = ACP63_SDW_PDM_DEV_MASK;
+				acp_data->pdev_count = ACP63_SDW0_SDW1_PDM_MODE_DEVS;
+				break;
+			default:
+				return -EINVAL;
+			}
+		} else if (is_dmic_dev) {
 			acp_data->pdev_mask = ACP63_PDM_DEV_MASK;
 			acp_data->pdev_count = ACP63_PDM_MODE_DEVS;
+		} else if (is_sdw_dev) {
+			switch (acp_data->sdw_manager_count) {
+			case 1:
+				acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
+				acp_data->pdev_count = ACP63_SDW0_MODE_DEVS;
+				break;
+			case 2:
+				acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
+				acp_data->pdev_count = ACP63_SDW0_SDW1_MODE_DEVS;
+				break;
+			default:
+				return -EINVAL;
+			}
 		}
 		break;
+	default:
+		break;
 	}
+	return 0;
 }
 
 static void acp63_fill_platform_dev_info(struct platform_device_info *pdevinfo,
@@ -173,6 +296,7 @@ static void acp63_fill_platform_dev_info(struct platform_device_info *pdevinfo,
 
 static int create_acp63_platform_devs(struct pci_dev *pci, struct acp63_dev_data *adata, u32 addr)
 {
+	struct acp_sdw_pdata *sdw_pdata;
 	struct platform_device_info pdevinfo[ACP63_DEVS];
 	struct device *parent;
 	int index;
@@ -205,8 +329,110 @@ static int create_acp63_platform_devs(struct pci_dev *pci, struct acp63_dev_data
 		acp63_fill_platform_dev_info(&pdevinfo[2], parent, NULL, "acp_ps_mach",
 					     0, NULL, 0, NULL, 0);
 		break;
+	case ACP63_SDW_DEV_MASK:
+		if (adata->pdev_count == ACP63_SDW0_MODE_DEVS) {
+			sdw_pdata = devm_kzalloc(&pci->dev, sizeof(struct acp_sdw_pdata),
+						 GFP_KERNEL);
+			if (!sdw_pdata) {
+				ret = -ENOMEM;
+				goto de_init;
+			}
+
+			sdw_pdata->instance = 0;
+			sdw_pdata->acp_sdw_lock = &adata->acp_lock;
+			adata->sdw0_dev_index = 0;
+			adata->sdw_dma_dev_index = 1;
+			acp63_fill_platform_dev_info(&pdevinfo[0], parent, adata->sdw_fw_node,
+						     "amd_sdw_manager", 0, adata->res, 1,
+						     sdw_pdata, sizeof(struct acp_sdw_pdata));
+			acp63_fill_platform_dev_info(&pdevinfo[1], parent, NULL, "amd_ps_sdw_dma",
+						     0, adata->res, 1, &adata->acp_lock,
+						     sizeof(adata->acp_lock));
+		} else if (adata->pdev_count == ACP63_SDW0_SDW1_MODE_DEVS) {
+			sdw_pdata = devm_kzalloc(&pci->dev, sizeof(struct acp_sdw_pdata) * 2,
+						 GFP_KERNEL);
+			if (!sdw_pdata) {
+				ret = -ENOMEM;
+				goto de_init;
+			}
+
+			sdw_pdata[0].instance = 0;
+			sdw_pdata[1].instance = 1;
+			sdw_pdata[0].acp_sdw_lock = &adata->acp_lock;
+			sdw_pdata[1].acp_sdw_lock = &adata->acp_lock;
+			sdw_pdata->acp_sdw_lock = &adata->acp_lock;
+			adata->sdw0_dev_index = 0;
+			adata->sdw1_dev_index = 1;
+			adata->sdw_dma_dev_index = 2;
+			acp63_fill_platform_dev_info(&pdevinfo[0], parent, adata->sdw_fw_node,
+						     "amd_sdw_manager", 0, adata->res, 1,
+						     &sdw_pdata[0], sizeof(struct acp_sdw_pdata));
+			acp63_fill_platform_dev_info(&pdevinfo[1], parent, adata->sdw_fw_node,
+						     "amd_sdw_manager", 1, adata->res, 1,
+						     &sdw_pdata[1], sizeof(struct acp_sdw_pdata));
+			acp63_fill_platform_dev_info(&pdevinfo[2], parent, NULL, "amd_ps_sdw_dma",
+						     0, adata->res, 1, &adata->acp_lock,
+						     sizeof(adata->acp_lock));
+		}
+		break;
+	case ACP63_SDW_PDM_DEV_MASK:
+		if (adata->pdev_count == ACP63_SDW0_PDM_MODE_DEVS) {
+			sdw_pdata = devm_kzalloc(&pci->dev, sizeof(struct acp_sdw_pdata),
+						 GFP_KERNEL);
+			if (!sdw_pdata) {
+				ret = -ENOMEM;
+				goto de_init;
+			}
+
+			sdw_pdata->instance = 0;
+			sdw_pdata->acp_sdw_lock = &adata->acp_lock;
+			adata->pdm_dev_index = 0;
+			adata->sdw0_dev_index = 1;
+			adata->sdw_dma_dev_index = 2;
+			acp63_fill_platform_dev_info(&pdevinfo[0], parent, NULL, "acp_ps_pdm_dma",
+						     0, adata->res, 1, &adata->acp_lock,
+						     sizeof(adata->acp_lock));
+			acp63_fill_platform_dev_info(&pdevinfo[1], parent, adata->sdw_fw_node,
+						     "amd_sdw_manager", 0, adata->res, 1,
+						     sdw_pdata, sizeof(struct acp_sdw_pdata));
+			acp63_fill_platform_dev_info(&pdevinfo[2], parent, NULL, "amd_ps_sdw_dma",
+						     0, adata->res, 1, &adata->acp_lock,
+						     sizeof(adata->acp_lock));
+			acp63_fill_platform_dev_info(&pdevinfo[3], parent, NULL, "dmic-codec",
+						     0, NULL, 0, NULL, 0);
+		} else if (adata->pdev_count == ACP63_SDW0_SDW1_PDM_MODE_DEVS) {
+			sdw_pdata = devm_kzalloc(&pci->dev, sizeof(struct acp_sdw_pdata) * 2,
+						 GFP_KERNEL);
+			if (!sdw_pdata) {
+				ret = -ENOMEM;
+				goto de_init;
+			}
+			sdw_pdata[0].instance = 0;
+			sdw_pdata[1].instance = 1;
+			sdw_pdata[0].acp_sdw_lock = &adata->acp_lock;
+			sdw_pdata[1].acp_sdw_lock = &adata->acp_lock;
+			adata->pdm_dev_index = 0;
+			adata->sdw0_dev_index = 1;
+			adata->sdw1_dev_index = 2;
+			adata->sdw_dma_dev_index = 3;
+			acp63_fill_platform_dev_info(&pdevinfo[0], parent, NULL, "acp_ps_pdm_dma",
+						     0, adata->res, 1, &adata->acp_lock,
+						     sizeof(adata->acp_lock));
+			acp63_fill_platform_dev_info(&pdevinfo[1], parent, adata->sdw_fw_node,
+						     "amd_sdw_manager", 0, adata->res, 1,
+						     &sdw_pdata[0], sizeof(struct acp_sdw_pdata));
+			acp63_fill_platform_dev_info(&pdevinfo[2], parent, adata->sdw_fw_node,
+						     "amd_sdw_manager", 1, adata->res, 1,
+						     &sdw_pdata[1], sizeof(struct acp_sdw_pdata));
+			acp63_fill_platform_dev_info(&pdevinfo[3], parent, NULL, "amd_ps_sdw_dma",
+						     0, adata->res, 1, &adata->acp_lock,
+						     sizeof(adata->acp_lock));
+			acp63_fill_platform_dev_info(&pdevinfo[4], parent, NULL, "dmic-codec",
+						     0, NULL, 0, NULL, 0);
+		}
+		break;
 	default:
-		dev_dbg(&pci->dev, "No PDM devices found\n");
+		dev_dbg(&pci->dev, "No PDM or SoundWire manager devices found\n");
 		return 0;
 	}
 
@@ -290,7 +516,11 @@ static int snd_acp63_probe(struct pci_dev *pci,
 		goto de_init;
 	}
 	val = readl(adata->acp63_base + ACP_PIN_CONFIG);
-	get_acp63_device_config(val, pci, adata);
+	ret = get_acp63_device_config(val, pci, adata);
+	if (ret) {
+		dev_err(&pci->dev, "get acp device config failed:%d\n", ret);
+		goto de_init;
+	}
 	ret = create_acp63_platform_devs(pci, adata, addr);
 	if (ret < 0) {
 		dev_err(&pci->dev, "ACP platform devices creation failed\n");
-- 
2.34.1

Re: [PATCH V2 1/9] ASoC: amd: ps: create platform devices based on acp config

[Pierre-Louis Bossart]

> +static int sdw_amd_scan_controller(struct device *dev)
> +{
> +	struct acp63_dev_data *acp_data;
> +	struct fwnode_handle *link;
> +	char name[32];
> +	u32 sdw_manager_bitmap;
> +	u8 count = 0;
> +	u32 acp_sdw_power_mode = 0;
> +	int index;
> +	int ret;
> +
> +	acp_data = dev_get_drvdata(dev);
> +	acp_data->acp_reset = true;
> +	/* Found controller, find links supported */
> +	ret = fwnode_property_read_u32_array((acp_data->sdw_fw_node), "mipi-sdw-manager-list",
> +					     &sdw_manager_bitmap, 1);
> +
> +	if (ret) {
> +		dev_err(dev, "Failed to read mipi-sdw-manager-list: %d\n", ret);
> +		return -EINVAL;
> +	}
> +	count = hweight32(sdw_manager_bitmap);
> +	/* Check count is within bounds */
> +	if (count > AMD_SDW_MAX_MANAGERS) {
> +		dev_err(dev, "Manager count %d exceeds max %d\n", count, AMD_SDW_MAX_MANAGERS);
> +		return -EINVAL;
> +	}
> +
> +	if (!count) {
> +		dev_dbg(dev, "No SoundWire Managers detected\n");
> +		return -EINVAL;
> +	}
> +	dev_dbg(dev, "ACPI reports %d SoundWire Manager devices\n", count);
> +	acp_data->sdw_manager_count = count;
> +	for (index = 0; index < count; index++) {
> +		snprintf(name, sizeof(name), "mipi-sdw-link-%d-subproperties", index);
> +		link = fwnode_get_named_child_node(acp_data->sdw_fw_node, name);
> +		if (!link) {
> +			dev_err(dev, "Manager node %s not found\n", name);
> +			return -EIO;
> +		}
> +
> +		fwnode_property_read_u32(link, "amd-sdw-power-mode",
> +					 &acp_sdw_power_mode);

What happens if this property is not found or doesn't exist?

acp_sdw_power_mode is zero, so....


> +		/*
> +		 * when SoundWire configuration is selected from acp pin config,
> +		 * based on manager instances count, acp init/de-init sequence should be
> +		 * executed as part of PM ops only when Bus reset is applied for the active
> +		 * SoundWire manager instances.
> +		 */
> +		if (acp_sdw_power_mode != AMD_SDW_POWER_OFF_MODE)
> +			acp_data->acp_reset = false;

... here this branch is taken.

Is this intentional?

> +	}
> +	return 0;
> +}
> +
> +static int get_acp63_device_config(u32 config, struct pci_dev *pci, struct acp63_dev_data *acp_data)
>  {
>  	struct acpi_device *dmic_dev;
> +	struct acpi_device *sdw_dev;
>  	const union acpi_object *obj;
>  	bool is_dmic_dev = false;

useless init

> +	bool is_sdw_dev = false;

and useless init as well...

> +	int ret;
>  
>  	dmic_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_DMIC_ADDR, 0);
>  	if (dmic_dev) {
> +		/* is_dmic_dev flag will be set when ACP PDM controller device exists */
>  		if (!acpi_dev_get_property(dmic_dev, "acp-audio-device-type",
>  					   ACPI_TYPE_INTEGER, &obj) &&
>  					   obj->integer.value == ACP_DMIC_DEV)
>  			is_dmic_dev = true;
>  	}
>  
> +	sdw_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_SDW_ADDR, 0);
> +	if (sdw_dev) {
> +		acp_data->sdw_fw_node = acpi_fwnode_handle(sdw_dev);
> +		ret = sdw_amd_scan_controller(&pci->dev);
> +		/* is_sdw_dev flag will be set when SoundWire Manager device exists */
> +		if (!ret)
> +			is_sdw_dev = true;

sdw_amd_scan_controller() can return -EINVAL, how is this handled?
Shouldn't you stop execution and return here in the < 0 case?

> +	}
> +
> +	dev_dbg(&pci->dev, "Audio Mode %d\n", config);
>  	switch (config) {
> -	case ACP_CONFIG_0:
> -	case ACP_CONFIG_1:
> +	case ACP_CONFIG_4:
> +	case ACP_CONFIG_5:
> +	case ACP_CONFIG_10:
> +	case ACP_CONFIG_11:
> +		if (is_dmic_dev) {
> +			acp_data->pdev_mask = ACP63_PDM_DEV_MASK;
> +			acp_data->pdev_count = ACP63_PDM_MODE_DEVS;
> +		}
> +		break;
>  	case ACP_CONFIG_2:
>  	case ACP_CONFIG_3:
> -	case ACP_CONFIG_9:
> -	case ACP_CONFIG_15:
> -		dev_dbg(&pci->dev, "Audio Mode %d\n", config);
> +		if (is_sdw_dev) {
> +			switch (acp_data->sdw_manager_count) {
> +			case 1:
> +				acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
> +				acp_data->pdev_count = ACP63_SDW0_MODE_DEVS;
> +				break;
> +			case 2:
> +				acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
> +				acp_data->pdev_count = ACP63_SDW0_SDW1_MODE_DEVS;
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +		}
>  		break;
> -	default:
> -		if (is_dmic_dev) {
> +	case ACP_CONFIG_6:
> +	case ACP_CONFIG_7:
> +	case ACP_CONFIG_12:
> +	case ACP_CONFIG_8:
> +	case ACP_CONFIG_13:
> +	case ACP_CONFIG_14:
> +		if (is_dmic_dev && is_sdw_dev) {
> +			switch (acp_data->sdw_manager_count) {
> +			case 1:
> +				acp_data->pdev_mask = ACP63_SDW_PDM_DEV_MASK;
> +				acp_data->pdev_count = ACP63_SDW0_PDM_MODE_DEVS;
> +				break;
> +			case 2:
> +				acp_data->pdev_mask = ACP63_SDW_PDM_DEV_MASK;
> +				acp_data->pdev_count = ACP63_SDW0_SDW1_PDM_MODE_DEVS;
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +		} else if (is_dmic_dev) {
>  			acp_data->pdev_mask = ACP63_PDM_DEV_MASK;
>  			acp_data->pdev_count = ACP63_PDM_MODE_DEVS;
> +		} else if (is_sdw_dev) {
> +			switch (acp_data->sdw_manager_count) {
> +			case 1:
> +				acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
> +				acp_data->pdev_count = ACP63_SDW0_MODE_DEVS;
> +				break;
> +			case 2:
> +				acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
> +				acp_data->pdev_count = ACP63_SDW0_SDW1_MODE_DEVS;
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
>  		}
>  		break;
> +	default:
> +		break;
>  	}
> +	return 0;
>  }

>  		dev_err(&pci->dev, "ACP platform devices creation failed\n");

Re: [PATCH V2 1/9] ASoC: amd: ps: create platform devices based on acp config

[Mukunda,Vijendar]

On 22/05/23 21:46, Pierre-Louis Bossart wrote:
>> +static int sdw_amd_scan_controller(struct device *dev)
>> +{
>> +	struct acp63_dev_data *acp_data;
>> +	struct fwnode_handle *link;
>> +	char name[32];
>> +	u32 sdw_manager_bitmap;
>> +	u8 count = 0;
>> +	u32 acp_sdw_power_mode = 0;
>> +	int index;
>> +	int ret;
>> +
>> +	acp_data = dev_get_drvdata(dev);
>> +	acp_data->acp_reset = true;
>> +	/* Found controller, find links supported */
>> +	ret = fwnode_property_read_u32_array((acp_data->sdw_fw_node), "mipi-sdw-manager-list",
>> +					     &sdw_manager_bitmap, 1);
>> +
>> +	if (ret) {
>> +		dev_err(dev, "Failed to read mipi-sdw-manager-list: %d\n", ret);
>> +		return -EINVAL;
>> +	}
>> +	count = hweight32(sdw_manager_bitmap);
>> +	/* Check count is within bounds */
>> +	if (count > AMD_SDW_MAX_MANAGERS) {
>> +		dev_err(dev, "Manager count %d exceeds max %d\n", count, AMD_SDW_MAX_MANAGERS);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!count) {
>> +		dev_dbg(dev, "No SoundWire Managers detected\n");
>> +		return -EINVAL;
>> +	}
>> +	dev_dbg(dev, "ACPI reports %d SoundWire Manager devices\n", count);
>> +	acp_data->sdw_manager_count = count;
>> +	for (index = 0; index < count; index++) {
>> +		snprintf(name, sizeof(name), "mipi-sdw-link-%d-subproperties", index);
>> +		link = fwnode_get_named_child_node(acp_data->sdw_fw_node, name);
>> +		if (!link) {
>> +			dev_err(dev, "Manager node %s not found\n", name);
>> +			return -EIO;
>> +		}
>> +
>> +		fwnode_property_read_u32(link, "amd-sdw-power-mode",
>> +					 &acp_sdw_power_mode);
> What happens if this property is not found or doesn't exist?
>
> acp_sdw_power_mode is zero, so....
>
If acp_sdw_power_mode is zero then ACP PCI driver won't apply
ACP de-init sequence while entering D3 state.

It's a miss. loop should exit when property is not found.
will modify the code.
>> +		/*
>> +		 * when SoundWire configuration is selected from acp pin config,
>> +		 * based on manager instances count, acp init/de-init sequence should be
>> +		 * executed as part of PM ops only when Bus reset is applied for the active
>> +		 * SoundWire manager instances.
>> +		 */
>> +		if (acp_sdw_power_mode != AMD_SDW_POWER_OFF_MODE)
>> +			acp_data->acp_reset = false;
> ... here this branch is taken.
>
> Is this intentional?
This loop should be exited if acp_sdw_power_mode is not set to power off mode.
will modify code.
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int get_acp63_device_config(u32 config, struct pci_dev *pci, struct acp63_dev_data *acp_data)
>>  {
>>  	struct acpi_device *dmic_dev;
>> +	struct acpi_device *sdw_dev;
>>  	const union acpi_object *obj;
>>  	bool is_dmic_dev = false;
> useless init
We are checking is_dmic_dev & is_sdw_dev flags in same code.
Either we need to explicitly update value as false when no ACP PDM
/SoundWire manager instances not found.
>
>> +	bool is_sdw_dev = false;
> and useless init as well...
>
>> +	int ret;
>>  
>>  	dmic_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_DMIC_ADDR, 0);
>>  	if (dmic_dev) {
>> +		/* is_dmic_dev flag will be set when ACP PDM controller device exists */
>>  		if (!acpi_dev_get_property(dmic_dev, "acp-audio-device-type",
>>  					   ACPI_TYPE_INTEGER, &obj) &&
>>  					   obj->integer.value == ACP_DMIC_DEV)
>>  			is_dmic_dev = true;
>>  	}
>>  
>> +	sdw_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_SDW_ADDR, 0);
>> +	if (sdw_dev) {
>> +		acp_data->sdw_fw_node = acpi_fwnode_handle(sdw_dev);
>> +		ret = sdw_amd_scan_controller(&pci->dev);
>> +		/* is_sdw_dev flag will be set when SoundWire Manager device exists */
>> +		if (!ret)
>> +			is_sdw_dev = true;
> sdw_amd_scan_controller() can return -EINVAL, how is this handled?
> Shouldn't you stop execution and return here in the < 0 case?
As per our design, ACP PCI driver probe should be successful, even
there are no ACP PDM or Soundwire Manager instance configuration
related platform devices.

The ACP PCI driver is multi-use and that even if SoundWire manager
instances or PDM controller is not found, it will still be used to set the
hardware to proper low power states. i.e ACP should enter D3 state
after successful execution of probe sequence.
>
>> +	}
>> +
>> +	dev_dbg(&pci->dev, "Audio Mode %d\n", config);
>>  	switch (config) {
>> -	case ACP_CONFIG_0:
>> -	case ACP_CONFIG_1:
>> +	case ACP_CONFIG_4:
>> +	case ACP_CONFIG_5:
>> +	case ACP_CONFIG_10:
>> +	case ACP_CONFIG_11:
>> +		if (is_dmic_dev) {
>> +			acp_data->pdev_mask = ACP63_PDM_DEV_MASK;
>> +			acp_data->pdev_count = ACP63_PDM_MODE_DEVS;
>> +		}
>> +		break;
>>  	case ACP_CONFIG_2:
>>  	case ACP_CONFIG_3:
>> -	case ACP_CONFIG_9:
>> -	case ACP_CONFIG_15:
>> -		dev_dbg(&pci->dev, "Audio Mode %d\n", config);
>> +		if (is_sdw_dev) {
>> +			switch (acp_data->sdw_manager_count) {
>> +			case 1:
>> +				acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
>> +				acp_data->pdev_count = ACP63_SDW0_MODE_DEVS;
>> +				break;
>> +			case 2:
>> +				acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
>> +				acp_data->pdev_count = ACP63_SDW0_SDW1_MODE_DEVS;
>> +				break;
>> +			default:
>> +				return -EINVAL;
>> +			}
>> +		}
>>  		break;
>> -	default:
>> -		if (is_dmic_dev) {
>> +	case ACP_CONFIG_6:
>> +	case ACP_CONFIG_7:
>> +	case ACP_CONFIG_12:
>> +	case ACP_CONFIG_8:
>> +	case ACP_CONFIG_13:
>> +	case ACP_CONFIG_14:
>> +		if (is_dmic_dev && is_sdw_dev) {
>> +			switch (acp_data->sdw_manager_count) {
>> +			case 1:
>> +				acp_data->pdev_mask = ACP63_SDW_PDM_DEV_MASK;
>> +				acp_data->pdev_count = ACP63_SDW0_PDM_MODE_DEVS;
>> +				break;
>> +			case 2:
>> +				acp_data->pdev_mask = ACP63_SDW_PDM_DEV_MASK;
>> +				acp_data->pdev_count = ACP63_SDW0_SDW1_PDM_MODE_DEVS;
>> +				break;
>> +			default:
>> +				return -EINVAL;
>> +			}
>> +		} else if (is_dmic_dev) {
>>  			acp_data->pdev_mask = ACP63_PDM_DEV_MASK;
>>  			acp_data->pdev_count = ACP63_PDM_MODE_DEVS;
>> +		} else if (is_sdw_dev) {
>> +			switch (acp_data->sdw_manager_count) {
>> +			case 1:
>> +				acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
>> +				acp_data->pdev_count = ACP63_SDW0_MODE_DEVS;
>> +				break;
>> +			case 2:
>> +				acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
>> +				acp_data->pdev_count = ACP63_SDW0_SDW1_MODE_DEVS;
>> +				break;
>> +			default:
>> +				return -EINVAL;
>> +			}
>>  		}
>>  		break;
>> +	default:
>> +		break;
>>  	}
>> +	return 0;
>>  }
>>  		dev_err(&pci->dev, "ACP platform devices creation failed\n");

Re: [PATCH V2 1/9] ASoC: amd: ps: create platform devices based on acp config

[Pierre-Louis Bossart]

>>> +static int get_acp63_device_config(u32 config, struct pci_dev *pci, struct acp63_dev_data *acp_data)
>>>  {
>>>  	struct acpi_device *dmic_dev;
>>> +	struct acpi_device *sdw_dev;
>>>  	const union acpi_object *obj;
>>>  	bool is_dmic_dev = false;
>> useless init
> We are checking is_dmic_dev & is_sdw_dev flags in same code.
> Either we need to explicitly update value as false when no ACP PDM
> /SoundWire manager instances not found.

please discard my comment, I read this sideways

>>
>>> +	bool is_sdw_dev = false;
>> and useless init as well...

same here.
>>
>>> +	int ret;
>>>  
>>>  	dmic_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_DMIC_ADDR, 0);
>>>  	if (dmic_dev) {
>>> +		/* is_dmic_dev flag will be set when ACP PDM controller device exists */
>>>  		if (!acpi_dev_get_property(dmic_dev, "acp-audio-device-type",
>>>  					   ACPI_TYPE_INTEGER, &obj) &&
>>>  					   obj->integer.value == ACP_DMIC_DEV)
>>>  			is_dmic_dev = true;
>>>  	}
>>>  
>>> +	sdw_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_SDW_ADDR, 0);
>>> +	if (sdw_dev) {
>>> +		acp_data->sdw_fw_node = acpi_fwnode_handle(sdw_dev);
>>> +		ret = sdw_amd_scan_controller(&pci->dev);
>>> +		/* is_sdw_dev flag will be set when SoundWire Manager device exists */
>>> +		if (!ret)
>>> +			is_sdw_dev = true;
>> sdw_amd_scan_controller() can return -EINVAL, how is this handled?
>> Shouldn't you stop execution and return here in the < 0 case?
> As per our design, ACP PCI driver probe should be successful, even
> there are no ACP PDM or Soundwire Manager instance configuration
> related platform devices.
> 
> The ACP PCI driver is multi-use and that even if SoundWire manager
> instances or PDM controller is not found, it will still be used to set the
> hardware to proper low power states. i.e ACP should enter D3 state
> after successful execution of probe sequence.

Ah ok, maybe a reworded comment would make sense then, e.g.

"continue probe and discard errors if SoundWire Manager is not described
in ACPI tables"

Same for DMIC above

Re: [PATCH V2 1/9] ASoC: amd: ps: create platform devices based on acp config

[Mukunda,Vijendar]

On 23/05/23 19:59, Pierre-Louis Bossart wrote:
>>>> +static int get_acp63_device_config(u32 config, struct pci_dev *pci, struct acp63_dev_data *acp_data)
>>>>  {
>>>>  	struct acpi_device *dmic_dev;
>>>> +	struct acpi_device *sdw_dev;
>>>>  	const union acpi_object *obj;
>>>>  	bool is_dmic_dev = false;
>>> useless init
>> We are checking is_dmic_dev & is_sdw_dev flags in same code.
>> Either we need to explicitly update value as false when no ACP PDM
>> /SoundWire manager instances not found.
> please discard my comment, I read this sideways
>
>>>> +	bool is_sdw_dev = false;
>>> and useless init as well...
> same here.
>>>> +	int ret;
>>>>  
>>>>  	dmic_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_DMIC_ADDR, 0);
>>>>  	if (dmic_dev) {
>>>> +		/* is_dmic_dev flag will be set when ACP PDM controller device exists */
>>>>  		if (!acpi_dev_get_property(dmic_dev, "acp-audio-device-type",
>>>>  					   ACPI_TYPE_INTEGER, &obj) &&
>>>>  					   obj->integer.value == ACP_DMIC_DEV)
>>>>  			is_dmic_dev = true;
>>>>  	}
>>>>  
>>>> +	sdw_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_SDW_ADDR, 0);
>>>> +	if (sdw_dev) {
>>>> +		acp_data->sdw_fw_node = acpi_fwnode_handle(sdw_dev);
>>>> +		ret = sdw_amd_scan_controller(&pci->dev);
>>>> +		/* is_sdw_dev flag will be set when SoundWire Manager device exists */
>>>> +		if (!ret)
>>>> +			is_sdw_dev = true;
>>> sdw_amd_scan_controller() can return -EINVAL, how is this handled?
>>> Shouldn't you stop execution and return here in the < 0 case?
>> As per our design, ACP PCI driver probe should be successful, even
>> there are no ACP PDM or Soundwire Manager instance configuration
>> related platform devices.
>>
>> The ACP PCI driver is multi-use and that even if SoundWire manager
>> instances or PDM controller is not found, it will still be used to set the
>> hardware to proper low power states. i.e ACP should enter D3 state
>> after successful execution of probe sequence.
> Ah ok, maybe a reworded comment would make sense then, e.g.
>
> "continue probe and discard errors if SoundWire Manager is not described
> in ACPI tables"
>
> Same for DMIC above
will add a comment.

[PATCH V2 2/9] ASoC: amd: ps: handle SoundWire interrupts in acp pci driver

[Vijendar Mukunda]

Handle SoundWire manager related interrupts in ACP PCI driver
interrupt handler and schedule SoundWire manager work queue for
further processing.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 sound/soc/amd/ps/acp63.h  |  4 ++++
 sound/soc/amd/ps/pci-ps.c | 43 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
index 95bb1cef900a..d296059be4f0 100644
--- a/sound/soc/amd/ps/acp63.h
+++ b/sound/soc/amd/ps/acp63.h
@@ -88,6 +88,10 @@
 /* time in ms for acp timeout */
 #define ACP_TIMEOUT		500
 
+#define ACP_SDW0_STAT		BIT(21)
+#define ACP_SDW1_STAT		BIT(2)
+#define ACP_ERROR_IRQ		BIT(29)
+
 enum acp_config {
 	ACP_CONFIG_0 = 0,
 	ACP_CONFIG_1,
diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
index 02caae6968ad..26514e340a33 100644
--- a/sound/soc/amd/ps/pci-ps.c
+++ b/sound/soc/amd/ps/pci-ps.c
@@ -56,6 +56,7 @@ static int acp63_reset(void __iomem *acp_base)
 static void acp63_enable_interrupts(void __iomem *acp_base)
 {
 	writel(1, acp_base + ACP_EXTERNAL_INTR_ENB);
+	writel(ACP_ERROR_IRQ, acp_base + ACP_EXTERNAL_INTR_CNTL);
 }
 
 static void acp63_disable_interrupts(void __iomem *acp_base)
@@ -102,23 +103,55 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
 {
 	struct acp63_dev_data *adata;
 	struct pdm_dev_data *ps_pdm_data;
-	u32 val;
+	struct amd_sdw_manager *amd_manager;
+	u32 ext_intr_stat, ext_intr_stat1;
+	u16 irq_flag = 0;
 	u16 pdev_index;
 
 	adata = dev_id;
 	if (!adata)
 		return IRQ_NONE;
+	ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
+	if (ext_intr_stat & ACP_SDW0_STAT) {
+		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
+		pdev_index = adata->sdw0_dev_index;
+		amd_manager = dev_get_drvdata(&adata->pdev[pdev_index]->dev);
+		if (amd_manager)
+			schedule_work(&amd_manager->amd_sdw_irq_thread);
+		irq_flag = 1;
+	}
 
-	val = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
-	if (val & BIT(PDM_DMA_STAT)) {
+	ext_intr_stat1 = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
+	if (ext_intr_stat1 & ACP_SDW1_STAT) {
+		writel(ACP_SDW1_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
+		pdev_index = adata->sdw1_dev_index;
+		amd_manager = dev_get_drvdata(&adata->pdev[pdev_index]->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->acp63_base + ACP_EXTERNAL_INTR_STAT);
+		/* TODO: Report SoundWire Manager instance errors */
+		writel(0, adata->acp63_base + ACP_SW0_I2S_ERROR_REASON);
+		writel(0, adata->acp63_base + ACP_SW1_I2S_ERROR_REASON);
+		writel(0, adata->acp63_base + ACP_ERROR_STATUS);
+		irq_flag = 1;
+	}
+
+	if (ext_intr_stat & BIT(PDM_DMA_STAT)) {
 		pdev_index = adata->pdm_dev_index;
 		ps_pdm_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev);
 		writel(BIT(PDM_DMA_STAT), adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
 		if (ps_pdm_data->capture_stream)
 			snd_pcm_period_elapsed(ps_pdm_data->capture_stream);
-		return IRQ_HANDLED;
+		irq_flag = 1;
 	}
-	return IRQ_NONE;
+	if (irq_flag)
+		return IRQ_HANDLED;
+	else
+		return IRQ_NONE;
 }
 
 static int sdw_amd_scan_controller(struct device *dev)
-- 
2.34.1

Re: [PATCH V2 2/9] ASoC: amd: ps: handle SoundWire interrupts in acp pci driver

[Pierre-Louis Bossart]

On 5/22/23 08:31, Vijendar Mukunda wrote:
> Handle SoundWire manager related interrupts in ACP PCI driver
> interrupt handler and schedule SoundWire manager work queue for
> further processing.
> 
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> ---
>  sound/soc/amd/ps/acp63.h  |  4 ++++
>  sound/soc/amd/ps/pci-ps.c | 43 ++++++++++++++++++++++++++++++++++-----
>  2 files changed, 42 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
> index 95bb1cef900a..d296059be4f0 100644
> --- a/sound/soc/amd/ps/acp63.h
> +++ b/sound/soc/amd/ps/acp63.h
> @@ -88,6 +88,10 @@
>  /* time in ms for acp timeout */
>  #define ACP_TIMEOUT		500
>  
> +#define ACP_SDW0_STAT		BIT(21)
> +#define ACP_SDW1_STAT		BIT(2)
> +#define ACP_ERROR_IRQ		BIT(29)
> +
>  enum acp_config {
>  	ACP_CONFIG_0 = 0,
>  	ACP_CONFIG_1,
> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
> index 02caae6968ad..26514e340a33 100644
> --- a/sound/soc/amd/ps/pci-ps.c
> +++ b/sound/soc/amd/ps/pci-ps.c
> @@ -56,6 +56,7 @@ static int acp63_reset(void __iomem *acp_base)
>  static void acp63_enable_interrupts(void __iomem *acp_base)
>  {
>  	writel(1, acp_base + ACP_EXTERNAL_INTR_ENB);
> +	writel(ACP_ERROR_IRQ, acp_base + ACP_EXTERNAL_INTR_CNTL);

you may want to comment on why you don't have a read-modify-write
approach for something that looks generic and not limited to a single
error bit?

>  }
>  
>  static void acp63_disable_interrupts(void __iomem *acp_base)
> @@ -102,23 +103,55 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
>  {
>  	struct acp63_dev_data *adata;
>  	struct pdm_dev_data *ps_pdm_data;
> -	u32 val;
> +	struct amd_sdw_manager *amd_manager;
> +	u32 ext_intr_stat, ext_intr_stat1;
> +	u16 irq_flag = 0;
>  	u16 pdev_index;
>  
>  	adata = dev_id;
>  	if (!adata)
>  		return IRQ_NONE;
> +	ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
> +	if (ext_intr_stat & ACP_SDW0_STAT) {
> +		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);

[1]

this is confusing, if this is w1c, should this be:

writel(ext_intr_stat, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);

Otherwise you may be clearing fields that have not been set?

> +		pdev_index = adata->sdw0_dev_index;
> +		amd_manager = dev_get_drvdata(&adata->pdev[pdev_index]->dev);
> +		if (amd_manager)
> +			schedule_work(&amd_manager->amd_sdw_irq_thread);
> +		irq_flag = 1;
> +	}
>  
> -	val = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
> -	if (val & BIT(PDM_DMA_STAT)) {
> +	ext_intr_stat1 = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
> +	if (ext_intr_stat1 & ACP_SDW1_STAT) {
> +		writel(ACP_SDW1_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);

same comment here.

> +		pdev_index = adata->sdw1_dev_index;
> +		amd_manager = dev_get_drvdata(&adata->pdev[pdev_index]->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->acp63_base + ACP_EXTERNAL_INTR_STAT);

[2]

and this is even move confusing because you may end-up writing twice to
the same adata->acp63_base + ACP_EXTERNAL_INTR_STAT with [1] and [2], so
the previous write

writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);

may have cleared the register already.

Something looks wrong here?

> +		/* TODO: Report SoundWire Manager instance errors */
> +		writel(0, adata->acp63_base + ACP_SW0_I2S_ERROR_REASON);
> +		writel(0, adata->acp63_base + ACP_SW1_I2S_ERROR_REASON);
> +		writel(0, adata->acp63_base + ACP_ERROR_STATUS);
> +		irq_flag = 1;
> +	}
> +
> +	if (ext_intr_stat & BIT(PDM_DMA_STAT)) {
>  		pdev_index = adata->pdm_dev_index;
>  		ps_pdm_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev);
>  		writel(BIT(PDM_DMA_STAT), adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>  		if (ps_pdm_data->capture_stream)
>  			snd_pcm_period_elapsed(ps_pdm_data->capture_stream);
> -		return IRQ_HANDLED;
> +		irq_flag = 1;
>  	}
> -	return IRQ_NONE;
> +	if (irq_flag)
> +		return IRQ_HANDLED;
> +	else
> +		return IRQ_NONE;
>  }
>  
>  static int sdw_amd_scan_controller(struct device *dev)

Re: [PATCH V2 2/9] ASoC: amd: ps: handle SoundWire interrupts in acp pci driver

[Mukunda,Vijendar]

On 22/05/23 21:56, Pierre-Louis Bossart wrote:
>
> On 5/22/23 08:31, Vijendar Mukunda wrote:
>> Handle SoundWire manager related interrupts in ACP PCI driver
>> interrupt handler and schedule SoundWire manager work queue for
>> further processing.
>>
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>> ---
>>  sound/soc/amd/ps/acp63.h  |  4 ++++
>>  sound/soc/amd/ps/pci-ps.c | 43 ++++++++++++++++++++++++++++++++++-----
>>  2 files changed, 42 insertions(+), 5 deletions(-)
>>
>> diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
>> index 95bb1cef900a..d296059be4f0 100644
>> --- a/sound/soc/amd/ps/acp63.h
>> +++ b/sound/soc/amd/ps/acp63.h
>> @@ -88,6 +88,10 @@
>>  /* time in ms for acp timeout */
>>  #define ACP_TIMEOUT		500
>>  
>> +#define ACP_SDW0_STAT		BIT(21)
>> +#define ACP_SDW1_STAT		BIT(2)
>> +#define ACP_ERROR_IRQ		BIT(29)
>> +
>>  enum acp_config {
>>  	ACP_CONFIG_0 = 0,
>>  	ACP_CONFIG_1,
>> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
>> index 02caae6968ad..26514e340a33 100644
>> --- a/sound/soc/amd/ps/pci-ps.c
>> +++ b/sound/soc/amd/ps/pci-ps.c
>> @@ -56,6 +56,7 @@ static int acp63_reset(void __iomem *acp_base)
>>  static void acp63_enable_interrupts(void __iomem *acp_base)
>>  {
>>  	writel(1, acp_base + ACP_EXTERNAL_INTR_ENB);
>> +	writel(ACP_ERROR_IRQ, acp_base + ACP_EXTERNAL_INTR_CNTL);
> you may want to comment on why you don't have a read-modify-write
> approach for something that looks generic and not limited to a single
> error bit?
This function will be called when ACP enters D0 state, or during probe
sequence.
ACP reset will be applied which will put all registers to default value.
In this case, ACP_EXTERNAL_INTR_CNTL register default value is zero.
Our intention is to set only ACP error mask in acp external control register.
Rest of the places, IRQ mask programming will use read-modify-write.
>>  }
>>  
>>  static void acp63_disable_interrupts(void __iomem *acp_base)
>> @@ -102,23 +103,55 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
>>  {
>>  	struct acp63_dev_data *adata;
>>  	struct pdm_dev_data *ps_pdm_data;
>> -	u32 val;
>> +	struct amd_sdw_manager *amd_manager;
>> +	u32 ext_intr_stat, ext_intr_stat1;
>> +	u16 irq_flag = 0;
>>  	u16 pdev_index;
>>  
>>  	adata = dev_id;
>>  	if (!adata)
>>  		return IRQ_NONE;
>> +	ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>> +	if (ext_intr_stat & ACP_SDW0_STAT) {
>> +		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
> [1]
>
> this is confusing, if this is w1c, should this be:
>
> writel(ext_intr_stat, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>
> Otherwise you may be clearing fields that have not been set?
As per our register spec, writing zero to register fields doesn't
have any effect. Only writing 1 to register bit will clear that
interrupt.
We are handling bit by bit irq check and clearing the irq mask
based on irq bit and take an action related to that particular irq
bit.


>
>> +		pdev_index = adata->sdw0_dev_index;
>> +		amd_manager = dev_get_drvdata(&adata->pdev[pdev_index]->dev);
>> +		if (amd_manager)
>> +			schedule_work(&amd_manager->amd_sdw_irq_thread);
>> +		irq_flag = 1;
>> +	}
>>  
>> -	val = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>> -	if (val & BIT(PDM_DMA_STAT)) {
>> +	ext_intr_stat1 = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
>> +	if (ext_intr_stat1 & ACP_SDW1_STAT) {
>> +		writel(ACP_SDW1_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
> same comment here.
>
>> +		pdev_index = adata->sdw1_dev_index;
>> +		amd_manager = dev_get_drvdata(&adata->pdev[pdev_index]->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->acp63_base + ACP_EXTERNAL_INTR_STAT);
> [2]
>
> and this is even move confusing because you may end-up writing twice to
> the same adata->acp63_base + ACP_EXTERNAL_INTR_STAT with [1] and [2], so
> the previous write
>
> writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>
> may have cleared the register already.
>
> Something looks wrong here?
As mentioned above, writing zero doesn't have any effect.
We are handling bit by bit in irq stat register.
>
>> +		/* TODO: Report SoundWire Manager instance errors */
>> +		writel(0, adata->acp63_base + ACP_SW0_I2S_ERROR_REASON);
>> +		writel(0, adata->acp63_base + ACP_SW1_I2S_ERROR_REASON);
>> +		writel(0, adata->acp63_base + ACP_ERROR_STATUS);
>> +		irq_flag = 1;
>> +	}
>> +
>> +	if (ext_intr_stat & BIT(PDM_DMA_STAT)) {
>>  		pdev_index = adata->pdm_dev_index;
>>  		ps_pdm_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev);
>>  		writel(BIT(PDM_DMA_STAT), adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>>  		if (ps_pdm_data->capture_stream)
>>  			snd_pcm_period_elapsed(ps_pdm_data->capture_stream);
>> -		return IRQ_HANDLED;
>> +		irq_flag = 1;
>>  	}
>> -	return IRQ_NONE;
>> +	if (irq_flag)
>> +		return IRQ_HANDLED;
>> +	else
>> +		return IRQ_NONE;
>>  }
>>  
>>  static int sdw_amd_scan_controller(struct device *dev)

Re: [PATCH V2 2/9] ASoC: amd: ps: handle SoundWire interrupts in acp pci driver

[Pierre-Louis Bossart]

>>>  static void acp63_disable_interrupts(void __iomem *acp_base)
>>> @@ -102,23 +103,55 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
>>>  {
>>>  	struct acp63_dev_data *adata;
>>>  	struct pdm_dev_data *ps_pdm_data;
>>> -	u32 val;
>>> +	struct amd_sdw_manager *amd_manager;
>>> +	u32 ext_intr_stat, ext_intr_stat1;
>>> +	u16 irq_flag = 0;
>>>  	u16 pdev_index;
>>>  
>>>  	adata = dev_id;
>>>  	if (!adata)
>>>  		return IRQ_NONE;
>>> +	ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>>> +	if (ext_intr_stat & ACP_SDW0_STAT) {
>>> +		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>> [1]
>>
>> this is confusing, if this is w1c, should this be:
>>
>> writel(ext_intr_stat, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>>
>> Otherwise you may be clearing fields that have not been set?
> As per our register spec, writing zero to register fields doesn't
> have any effect. Only writing 1 to register bit will clear that
> interrupt.
> We are handling bit by bit irq check and clearing the irq mask
> based on irq bit and take an action related to that particular irq
> bit.

Right, maybe an explanation in the commit message would help.

Re: [PATCH V2 2/9] ASoC: amd: ps: handle SoundWire interrupts in acp pci driver

[Mukunda,Vijendar]

On 23/05/23 20:04, Pierre-Louis Bossart wrote:
>
>>>>  static void acp63_disable_interrupts(void __iomem *acp_base)
>>>> @@ -102,23 +103,55 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
>>>>  {
>>>>  	struct acp63_dev_data *adata;
>>>>  	struct pdm_dev_data *ps_pdm_data;
>>>> -	u32 val;
>>>> +	struct amd_sdw_manager *amd_manager;
>>>> +	u32 ext_intr_stat, ext_intr_stat1;
>>>> +	u16 irq_flag = 0;
>>>>  	u16 pdev_index;
>>>>  
>>>>  	adata = dev_id;
>>>>  	if (!adata)
>>>>  		return IRQ_NONE;
>>>> +	ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>>>> +	if (ext_intr_stat & ACP_SDW0_STAT) {
>>>> +		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>>> [1]
>>>
>>> this is confusing, if this is w1c, should this be:
>>>
>>> writel(ext_intr_stat, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>>>
>>> Otherwise you may be clearing fields that have not been set?
>> As per our register spec, writing zero to register fields doesn't
>> have any effect. Only writing 1 to register bit will clear that
>> interrupt.
>> We are handling bit by bit irq check and clearing the irq mask
>> based on irq bit and take an action related to that particular irq
>> bit.
> Right, maybe an explanation in the commit message would help.
will update the commit message.

[PATCH V2 3/9] ASoC: amd: ps: add SoundWire dma driver

[Vijendar Mukunda]

SoundWire DMA platform driver binds to the platform device created by
ACP PCI device. SoundWire DMA driver registers ALSA DMA component
with ASoC framework.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 sound/soc/amd/ps/acp63.h      |  5 +++
 sound/soc/amd/ps/ps-sdw-dma.c | 70 +++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)
 create mode 100644 sound/soc/amd/ps/ps-sdw-dma.c

diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
index d296059be4f0..eec58da7ec8b 100644
--- a/sound/soc/amd/ps/acp63.h
+++ b/sound/soc/amd/ps/acp63.h
@@ -148,6 +148,11 @@ struct pdm_dev_data {
 	struct snd_pcm_substream *capture_stream;
 };
 
+struct sdw_dma_dev_data {
+	void __iomem *acp_base;
+	struct mutex *acp_lock; /* used to protect acp common register access */
+};
+
 /**
  * struct acp63_dev_data - acp pci driver context
  * @acp63_base: acp mmio base
diff --git a/sound/soc/amd/ps/ps-sdw-dma.c b/sound/soc/amd/ps/ps-sdw-dma.c
new file mode 100644
index 000000000000..f41849fd035c
--- /dev/null
+++ b/sound/soc/amd/ps/ps-sdw-dma.c
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * AMD ALSA SoC Pink Sardine SoundWire DMA Driver
+ *
+ * Copyright 2023 Advanced Micro Devices, Inc.
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/soc-dai.h>
+#include "acp63.h"
+
+#define DRV_NAME "amd_ps_sdw_dma"
+
+static const struct snd_soc_component_driver acp63_sdw_component = {
+	.name		= DRV_NAME,
+};
+
+static int acp63_sdw_platform_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct sdw_dma_dev_data *sdw_data;
+	int status;
+
+	if (!pdev->dev.platform_data) {
+		dev_err(&pdev->dev, "platform_data not retrieved\n");
+		return -ENODEV;
+	}
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "IORESOURCE_MEM FAILED\n");
+		return -ENODEV;
+	}
+
+	sdw_data = devm_kzalloc(&pdev->dev, sizeof(*sdw_data), GFP_KERNEL);
+	if (!sdw_data)
+		return -ENOMEM;
+
+	sdw_data->acp_base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!sdw_data->acp_base)
+		return -ENOMEM;
+
+	sdw_data->acp_lock = pdev->dev.platform_data;
+	dev_set_drvdata(&pdev->dev, sdw_data);
+	status = devm_snd_soc_register_component(&pdev->dev,
+						 &acp63_sdw_component,
+						 NULL, 0);
+	if (status)
+		dev_err(&pdev->dev, "Fail to register sdw dma component\n");
+
+	return status;
+}
+
+static struct platform_driver acp63_sdw_dma_driver = {
+	.probe = acp63_sdw_platform_probe,
+	.driver = {
+		.name = "amd_ps_sdw_dma",
+	},
+};
+
+module_platform_driver(acp63_sdw_dma_driver);
+
+MODULE_AUTHOR("Vijendar.Mukunda@amd.com");
+MODULE_DESCRIPTION("AMD ACP6.3 PS SDW DMA Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRV_NAME);
-- 
2.34.1

Re: [PATCH V2 3/9] ASoC: amd: ps: add SoundWire dma driver

[Pierre-Louis Bossart]

On 5/22/23 08:31, Vijendar Mukunda wrote:
> SoundWire DMA platform driver binds to the platform device created by
> ACP PCI device. SoundWire DMA driver registers ALSA DMA component
> with ASoC framework.
> 
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> ---
>  sound/soc/amd/ps/acp63.h      |  5 +++
>  sound/soc/amd/ps/ps-sdw-dma.c | 70 +++++++++++++++++++++++++++++++++++
>  2 files changed, 75 insertions(+)
>  create mode 100644 sound/soc/amd/ps/ps-sdw-dma.c
> 
> diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
> index d296059be4f0..eec58da7ec8b 100644
> --- a/sound/soc/amd/ps/acp63.h
> +++ b/sound/soc/amd/ps/acp63.h
> @@ -148,6 +148,11 @@ struct pdm_dev_data {
>  	struct snd_pcm_substream *capture_stream;
>  };
>  
> +struct sdw_dma_dev_data {
> +	void __iomem *acp_base;
> +	struct mutex *acp_lock; /* used to protect acp common register access */
> +};
> +
>  /**
>   * struct acp63_dev_data - acp pci driver context
>   * @acp63_base: acp mmio base
> diff --git a/sound/soc/amd/ps/ps-sdw-dma.c b/sound/soc/amd/ps/ps-sdw-dma.c
> new file mode 100644
> index 000000000000..f41849fd035c
> --- /dev/null
> +++ b/sound/soc/amd/ps/ps-sdw-dma.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AMD ALSA SoC Pink Sardine SoundWire DMA Driver
> + *
> + * Copyright 2023 Advanced Micro Devices, Inc.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include <sound/soc-dai.h>
> +#include "acp63.h"
> +
> +#define DRV_NAME "amd_ps_sdw_dma"
> +
> +static const struct snd_soc_component_driver acp63_sdw_component = {
> +	.name		= DRV_NAME,
> +};
> +
> +static int acp63_sdw_platform_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct sdw_dma_dev_data *sdw_data;
> +	int status;
> +
> +	if (!pdev->dev.platform_data) {
> +		dev_err(&pdev->dev, "platform_data not retrieved\n");
> +		return -ENODEV;
> +	}
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "IORESOURCE_MEM FAILED\n");
> +		return -ENODEV;
> +	}
> +
> +	sdw_data = devm_kzalloc(&pdev->dev, sizeof(*sdw_data), GFP_KERNEL);
> +	if (!sdw_data)
> +		return -ENOMEM;
> +
> +	sdw_data->acp_base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> +	if (!sdw_data->acp_base)
> +		return -ENOMEM;
> +
> +	sdw_data->acp_lock = pdev->dev.platform_data;

so you are sharing the same lock between parent and child platform device?

Does this work? IIRC the platform_data is copied, you do not point
directly to the initial data provided by the parent. We had issues with
SoundWire when we used platform devices, with the 'wrong' pointer used.

The documentation does make mention of a copy....

/**
 * platform_device_add_data - add platform-specific data to a platform
device
 * @pdev: platform device allocated by platform_device_alloc to add
resources to
 * @data: platform specific data for this platform device
 * @size: size of platform specific data
 *
 * Add a copy of platform specific data to the platform device's
 * platform_data pointer.  The memory associated with the platform data
 * will be freed when the platform device is released.
 */
> +	dev_set_drvdata(&pdev->dev, sdw_data);
> +	status = devm_snd_soc_register_component(&pdev->dev,
> +						 &acp63_sdw_component,
> +						 NULL, 0);
> +	if (status)
> +		dev_err(&pdev->dev, "Fail to register sdw dma component\n");
> +
> +	return status;
> +}
> +
> +static struct platform_driver acp63_sdw_dma_driver = {
> +	.probe = acp63_sdw_platform_probe,
> +	.driver = {
> +		.name = "amd_ps_sdw_dma",
> +	},
> +};
> +
> +module_platform_driver(acp63_sdw_dma_driver);
> +
> +MODULE_AUTHOR("Vijendar.Mukunda@amd.com");
> +MODULE_DESCRIPTION("AMD ACP6.3 PS SDW DMA Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRV_NAME);

Re: [PATCH V2 3/9] ASoC: amd: ps: add SoundWire dma driver

[Mukunda,Vijendar]

On 22/05/23 22:04, Pierre-Louis Bossart wrote:
>
> On 5/22/23 08:31, Vijendar Mukunda wrote:
>> SoundWire DMA platform driver binds to the platform device created by
>> ACP PCI device. SoundWire DMA driver registers ALSA DMA component
>> with ASoC framework.
>>
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>> ---
>>  sound/soc/amd/ps/acp63.h      |  5 +++
>>  sound/soc/amd/ps/ps-sdw-dma.c | 70 +++++++++++++++++++++++++++++++++++
>>  2 files changed, 75 insertions(+)
>>  create mode 100644 sound/soc/amd/ps/ps-sdw-dma.c
>>
>> diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
>> index d296059be4f0..eec58da7ec8b 100644
>> --- a/sound/soc/amd/ps/acp63.h
>> +++ b/sound/soc/amd/ps/acp63.h
>> @@ -148,6 +148,11 @@ struct pdm_dev_data {
>>  	struct snd_pcm_substream *capture_stream;
>>  };
>>  
>> +struct sdw_dma_dev_data {
>> +	void __iomem *acp_base;
>> +	struct mutex *acp_lock; /* used to protect acp common register access */
>> +};
>> +
>>  /**
>>   * struct acp63_dev_data - acp pci driver context
>>   * @acp63_base: acp mmio base
>> diff --git a/sound/soc/amd/ps/ps-sdw-dma.c b/sound/soc/amd/ps/ps-sdw-dma.c
>> new file mode 100644
>> index 000000000000..f41849fd035c
>> --- /dev/null
>> +++ b/sound/soc/amd/ps/ps-sdw-dma.c
>> @@ -0,0 +1,70 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * AMD ALSA SoC Pink Sardine SoundWire DMA Driver
>> + *
>> + * Copyright 2023 Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <sound/pcm_params.h>
>> +#include <sound/soc.h>
>> +#include <sound/soc-dai.h>
>> +#include "acp63.h"
>> +
>> +#define DRV_NAME "amd_ps_sdw_dma"
>> +
>> +static const struct snd_soc_component_driver acp63_sdw_component = {
>> +	.name		= DRV_NAME,
>> +};
>> +
>> +static int acp63_sdw_platform_probe(struct platform_device *pdev)
>> +{
>> +	struct resource *res;
>> +	struct sdw_dma_dev_data *sdw_data;
>> +	int status;
>> +
>> +	if (!pdev->dev.platform_data) {
>> +		dev_err(&pdev->dev, "platform_data not retrieved\n");
>> +		return -ENODEV;
>> +	}
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		dev_err(&pdev->dev, "IORESOURCE_MEM FAILED\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	sdw_data = devm_kzalloc(&pdev->dev, sizeof(*sdw_data), GFP_KERNEL);
>> +	if (!sdw_data)
>> +		return -ENOMEM;
>> +
>> +	sdw_data->acp_base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>> +	if (!sdw_data->acp_base)
>> +		return -ENOMEM;
>> +
>> +	sdw_data->acp_lock = pdev->dev.platform_data;
> so you are sharing the same lock between parent and child platform device?
Initially, we thought of sharing the same lock between parent and child
platform devices. Later we have observed, mutex hang issues observed.

We have avoided critical section code and removed acp_lock from
ACP SoundWire DMA driver while accessing ACP common registers.
We will remove mutex lock from ACP SoundWire DMA driver code.
> Does this work? IIRC the platform_data is copied, you do not point
> directly to the initial data provided by the parent. We had issues with
> SoundWire when we used platform devices, with the 'wrong' pointer used.
Till now, we haven't observed mutex hang issues due to
ACP PDM driver mutex lock changes.
Agreed. We will remove the mutex code from ACP PDM driver as
well and we will refactor code.
In SoundWire manager driver, we are sharing the same copy for two
manager instances. We haven't observed any issue.
>
> The documentation does make mention of a copy....
>
> /**
>  * platform_device_add_data - add platform-specific data to a platform
> device
>  * @pdev: platform device allocated by platform_device_alloc to add
> resources to
>  * @data: platform specific data for this platform device
>  * @size: size of platform specific data
>  *
>  * Add a copy of platform specific data to the platform device's
>  * platform_data pointer.  The memory associated with the platform data
>  * will be freed when the platform device is released.
>  */
>> +	dev_set_drvdata(&pdev->dev, sdw_data);
>> +	status = devm_snd_soc_register_component(&pdev->dev,
>> +						 &acp63_sdw_component,
>> +						 NULL, 0);
>> +	if (status)
>> +		dev_err(&pdev->dev, "Fail to register sdw dma component\n");
>> +
>> +	return status;
>> +}
>> +
>> +static struct platform_driver acp63_sdw_dma_driver = {
>> +	.probe = acp63_sdw_platform_probe,
>> +	.driver = {
>> +		.name = "amd_ps_sdw_dma",
>> +	},
>> +};
>> +
>> +module_platform_driver(acp63_sdw_dma_driver);
>> +
>> +MODULE_AUTHOR("Vijendar.Mukunda@amd.com");
>> +MODULE_DESCRIPTION("AMD ACP6.3 PS SDW DMA Driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:" DRV_NAME);

Re: [PATCH V2 3/9] ASoC: amd: ps: add SoundWire dma driver

[Pierre-Louis Bossart]

>>> +struct sdw_dma_dev_data {
>>> +	void __iomem *acp_base;
>>> +	struct mutex *acp_lock; /* used to protect acp common register access */
>>> +};
>>> +
>>>  /**
>>>   * struct acp63_dev_data - acp pci driver context
>>>   * @acp63_base: acp mmio base
>>> diff --git a/sound/soc/amd/ps/ps-sdw-dma.c b/sound/soc/amd/ps/ps-sdw-dma.c
>>> new file mode 100644
>>> index 000000000000..f41849fd035c
>>> --- /dev/null
>>> +++ b/sound/soc/amd/ps/ps-sdw-dma.c
>>> @@ -0,0 +1,70 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * AMD ALSA SoC Pink Sardine SoundWire DMA Driver
>>> + *
>>> + * Copyright 2023 Advanced Micro Devices, Inc.
>>> + */
>>> +
>>> +#include <linux/err.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <sound/pcm_params.h>
>>> +#include <sound/soc.h>
>>> +#include <sound/soc-dai.h>
>>> +#include "acp63.h"
>>> +
>>> +#define DRV_NAME "amd_ps_sdw_dma"
>>> +
>>> +static const struct snd_soc_component_driver acp63_sdw_component = {
>>> +	.name		= DRV_NAME,
>>> +};
>>> +
>>> +static int acp63_sdw_platform_probe(struct platform_device *pdev)
>>> +{
>>> +	struct resource *res;
>>> +	struct sdw_dma_dev_data *sdw_data;
>>> +	int status;
>>> +
>>> +	if (!pdev->dev.platform_data) {
>>> +		dev_err(&pdev->dev, "platform_data not retrieved\n");
>>> +		return -ENODEV;
>>> +	}
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	if (!res) {
>>> +		dev_err(&pdev->dev, "IORESOURCE_MEM FAILED\n");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	sdw_data = devm_kzalloc(&pdev->dev, sizeof(*sdw_data), GFP_KERNEL);
>>> +	if (!sdw_data)
>>> +		return -ENOMEM;
>>> +
>>> +	sdw_data->acp_base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>>> +	if (!sdw_data->acp_base)
>>> +		return -ENOMEM;
>>> +
>>> +	sdw_data->acp_lock = pdev->dev.platform_data;
>> so you are sharing the same lock between parent and child platform device?
> Initially, we thought of sharing the same lock between parent and child
> platform devices. Later we have observed, mutex hang issues observed.

If the goal is a global lock, then the platform data should contain a
pointer to the lock. We used this for Intel, see .e.g. the shim_mask in
drivers/soundwire/intel_init.c, where the same pointer is used by all
children.

> 
> We have avoided critical section code and removed acp_lock from
> ACP SoundWire DMA driver while accessing ACP common registers.
> We will remove mutex lock from ACP SoundWire DMA driver code.
>> Does this work? IIRC the platform_data is copied, you do not point
>> directly to the initial data provided by the parent. We had issues with
>> SoundWire when we used platform devices, with the 'wrong' pointer used.
> Till now, we haven't observed mutex hang issues due to
> ACP PDM driver mutex lock changes.
> Agreed. We will remove the mutex code from ACP PDM driver as
> well and we will refactor code.
> In SoundWire manager driver, we are sharing the same copy for two
> manager instances. We haven't observed any issue.

What's the benefit of passing this lock as platform_data, if the goal is
to perform mutual exclusion between the two manager instances? Why not
just create the lock as part of the SoundWire probe?

If there was no need for a lock, then please remove it :-)

If it's needed, please describe what it protects, which agents rely on
it and how the lock is shared.

>>
>> The documentation does make mention of a copy....
>>
>> /**
>>  * platform_device_add_data - add platform-specific data to a platform
>> device
>>  * @pdev: platform device allocated by platform_device_alloc to add
>> resources to
>>  * @data: platform specific data for this platform device
>>  * @size: size of platform specific data
>>  *
>>  * Add a copy of platform specific data to the platform device's
>>  * platform_data pointer.  The memory associated with the platform data
>>  * will be freed when the platform device is released.
>>  */
>>> +	dev_set_drvdata(&pdev->dev, sdw_data);
>>> +	status = devm_snd_soc_register_component(&pdev->dev,
>>> +						 &acp63_sdw_component,
>>> +						 NULL, 0);
>>> +	if (status)
>>> +		dev_err(&pdev->dev, "Fail to register sdw dma component\n");
>>> +
>>> +	return status;
>>> +}

Re: [PATCH V2 3/9] ASoC: amd: ps: add SoundWire dma driver

[Mukunda,Vijendar]

On 23/05/23 20:18, Pierre-Louis Bossart wrote:
>
>
>>>> +struct sdw_dma_dev_data {
>>>> +	void __iomem *acp_base;
>>>> +	struct mutex *acp_lock; /* used to protect acp common register access */
>>>> +};
>>>> +
>>>>  /**
>>>>   * struct acp63_dev_data - acp pci driver context
>>>>   * @acp63_base: acp mmio base
>>>> diff --git a/sound/soc/amd/ps/ps-sdw-dma.c b/sound/soc/amd/ps/ps-sdw-dma.c
>>>> new file mode 100644
>>>> index 000000000000..f41849fd035c
>>>> --- /dev/null
>>>> +++ b/sound/soc/amd/ps/ps-sdw-dma.c
>>>> @@ -0,0 +1,70 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * AMD ALSA SoC Pink Sardine SoundWire DMA Driver
>>>> + *
>>>> + * Copyright 2023 Advanced Micro Devices, Inc.
>>>> + */
>>>> +
>>>> +#include <linux/err.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <sound/pcm_params.h>
>>>> +#include <sound/soc.h>
>>>> +#include <sound/soc-dai.h>
>>>> +#include "acp63.h"
>>>> +
>>>> +#define DRV_NAME "amd_ps_sdw_dma"
>>>> +
>>>> +static const struct snd_soc_component_driver acp63_sdw_component = {
>>>> +	.name		= DRV_NAME,
>>>> +};
>>>> +
>>>> +static int acp63_sdw_platform_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct resource *res;
>>>> +	struct sdw_dma_dev_data *sdw_data;
>>>> +	int status;
>>>> +
>>>> +	if (!pdev->dev.platform_data) {
>>>> +		dev_err(&pdev->dev, "platform_data not retrieved\n");
>>>> +		return -ENODEV;
>>>> +	}
>>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +	if (!res) {
>>>> +		dev_err(&pdev->dev, "IORESOURCE_MEM FAILED\n");
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	sdw_data = devm_kzalloc(&pdev->dev, sizeof(*sdw_data), GFP_KERNEL);
>>>> +	if (!sdw_data)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	sdw_data->acp_base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>>>> +	if (!sdw_data->acp_base)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	sdw_data->acp_lock = pdev->dev.platform_data;
>>> so you are sharing the same lock between parent and child platform device?
>> Initially, we thought of sharing the same lock between parent and child
>> platform devices. Later we have observed, mutex hang issues observed.
> If the goal is a global lock, then the platform data should contain a
> pointer to the lock. We used this for Intel, see .e.g. the shim_mask in
> drivers/soundwire/intel_init.c, where the same pointer is used by all
> children.
We want to have common lock for Soundwire manager instances only.
>> We have avoided critical section code and removed acp_lock from
>> ACP SoundWire DMA driver while accessing ACP common registers.
>> We will remove mutex lock from ACP SoundWire DMA driver code.
>>> Does this work? IIRC the platform_data is copied, you do not point
>>> directly to the initial data provided by the parent. We had issues with
>>> SoundWire when we used platform devices, with the 'wrong' pointer used.
>> Till now, we haven't observed mutex hang issues due to
>> ACP PDM driver mutex lock changes.
>> Agreed. We will remove the mutex code from ACP PDM driver as
>> well and we will refactor code.
>> In SoundWire manager driver, we are sharing the same copy for two
>> manager instances. We haven't observed any issue.
> What's the benefit of passing this lock as platform_data, if the goal is
> to perform mutual exclusion between the two manager instances? Why not
> just create the lock as part of the SoundWire probe?
>
> If there was no need for a lock, then please remove it :-)
>
> If it's needed, please describe what it protects, which agents rely on
> it and how the lock is shared.
>
There is a small correction.

We are passing address of the mutex variable (acp_lock) which is part
of ACP PCI driver data structure as a platform data to child platform devices
and accessing it by pointer reference in SoundWire manager driver.
It's not required to drop code changes specific to SoundWire manager
platform device resource structure.

Our intention is to have a common lock for different SoundWire
manager instances, which protects accessing ACP common registers in
SoundWire manager driver.

Even though platform data creates its own copy for each platform device,
as we are passing the address of the mutex, and referencing it by pointer
in SoundWire manager driver, it works as global lock for SoundWire
manager instances.

We will remove the acp_lock code changes in ACP PDM driver
and SoundWire DMA driver.
>>> The documentation does make mention of a copy....
>>>
>>> /**
>>>  * platform_device_add_data - add platform-specific data to a platform
>>> device
>>>  * @pdev: platform device allocated by platform_device_alloc to add
>>> resources to
>>>  * @data: platform specific data for this platform device
>>>  * @size: size of platform specific data
>>>  *
>>>  * Add a copy of platform specific data to the platform device's
>>>  * platform_data pointer.  The memory associated with the platform data
>>>  * will be freed when the platform device is released.
>>>  */
>>>> +	dev_set_drvdata(&pdev->dev, sdw_data);
>>>> +	status = devm_snd_soc_register_component(&pdev->dev,
>>>> +						 &acp63_sdw_component,
>>>> +						 NULL, 0);
>>>> +	if (status)
>>>> +		dev_err(&pdev->dev, "Fail to register sdw dma component\n");
>>>> +
>>>> +	return status;
>>>> +}

Re: [PATCH V2 3/9] ASoC: amd: ps: add SoundWire dma driver

[Mukunda,Vijendar]

On 24/05/23 17:07, Mukunda,Vijendar wrote:
> On 23/05/23 20:18, Pierre-Louis Bossart wrote:
>>
>>>>> +struct sdw_dma_dev_data {
>>>>> +	void __iomem *acp_base;
>>>>> +	struct mutex *acp_lock; /* used to protect acp common register access */
>>>>> +};
>>>>> +
>>>>>  /**
>>>>>   * struct acp63_dev_data - acp pci driver context
>>>>>   * @acp63_base: acp mmio base
>>>>> diff --git a/sound/soc/amd/ps/ps-sdw-dma.c b/sound/soc/amd/ps/ps-sdw-dma.c
>>>>> new file mode 100644
>>>>> index 000000000000..f41849fd035c
>>>>> --- /dev/null
>>>>> +++ b/sound/soc/amd/ps/ps-sdw-dma.c
>>>>> @@ -0,0 +1,70 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>> +/*
>>>>> + * AMD ALSA SoC Pink Sardine SoundWire DMA Driver
>>>>> + *
>>>>> + * Copyright 2023 Advanced Micro Devices, Inc.
>>>>> + */
>>>>> +
>>>>> +#include <linux/err.h>
>>>>> +#include <linux/io.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/platform_device.h>
>>>>> +#include <sound/pcm_params.h>
>>>>> +#include <sound/soc.h>
>>>>> +#include <sound/soc-dai.h>
>>>>> +#include "acp63.h"
>>>>> +
>>>>> +#define DRV_NAME "amd_ps_sdw_dma"
>>>>> +
>>>>> +static const struct snd_soc_component_driver acp63_sdw_component = {
>>>>> +	.name		= DRV_NAME,
>>>>> +};
>>>>> +
>>>>> +static int acp63_sdw_platform_probe(struct platform_device *pdev)
>>>>> +{
>>>>> +	struct resource *res;
>>>>> +	struct sdw_dma_dev_data *sdw_data;
>>>>> +	int status;
>>>>> +
>>>>> +	if (!pdev->dev.platform_data) {
>>>>> +		dev_err(&pdev->dev, "platform_data not retrieved\n");
>>>>> +		return -ENODEV;
>>>>> +	}
>>>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>> +	if (!res) {
>>>>> +		dev_err(&pdev->dev, "IORESOURCE_MEM FAILED\n");
>>>>> +		return -ENODEV;
>>>>> +	}
>>>>> +
>>>>> +	sdw_data = devm_kzalloc(&pdev->dev, sizeof(*sdw_data), GFP_KERNEL);
>>>>> +	if (!sdw_data)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	sdw_data->acp_base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>>>>> +	if (!sdw_data->acp_base)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	sdw_data->acp_lock = pdev->dev.platform_data;
>>>> so you are sharing the same lock between parent and child platform device?
>>> Initially, we thought of sharing the same lock between parent and child
>>> platform devices. Later we have observed, mutex hang issues observed.
>> If the goal is a global lock, then the platform data should contain a
>> pointer to the lock. We used this for Intel, see .e.g. the shim_mask in
>> drivers/soundwire/intel_init.c, where the same pointer is used by all
>> children.
> We want to have common lock for Soundwire manager instances only.
>>> We have avoided critical section code and removed acp_lock from
>>> ACP SoundWire DMA driver while accessing ACP common registers.
>>> We will remove mutex lock from ACP SoundWire DMA driver code.
>>>> Does this work? IIRC the platform_data is copied, you do not point
>>>> directly to the initial data provided by the parent. We had issues with
>>>> SoundWire when we used platform devices, with the 'wrong' pointer used.
>>> Till now, we haven't observed mutex hang issues due to
>>> ACP PDM driver mutex lock changes.
>>> Agreed. We will remove the mutex code from ACP PDM driver as
>>> well and we will refactor code.
>>> In SoundWire manager driver, we are sharing the same copy for two
>>> manager instances. We haven't observed any issue.
>> What's the benefit of passing this lock as platform_data, if the goal is
>> to perform mutual exclusion between the two manager instances? Why not
>> just create the lock as part of the SoundWire probe?
>>
>> If there was no need for a lock, then please remove it :-)
>>
>> If it's needed, please describe what it protects, which agents rely on
>> it and how the lock is shared.
>>
> There is a small correction.
>
> We are passing address of the mutex variable (acp_lock) which is part
> of ACP PCI driver data structure as a platform data to child platform devices
> and accessing it by pointer reference in SoundWire manager driver.
> It's not required to drop code changes specific to SoundWire manager
> platform device resource structure.
>
> Our intention is to have a common lock for different SoundWire
> manager instances, which protects accessing ACP common registers in
> SoundWire manager driver.
>
> Even though platform data creates its own copy for each platform device,
> as we are passing the address of the mutex, and referencing it by pointer
> in SoundWire manager driver, it works as global lock for SoundWire
> manager instances.
>
> We will remove the acp_lock code changes in ACP PDM driver
> and SoundWire DMA driver.

We have further tried few experiments in ACP PDM driver and
ACP SoundWire DMA driver. Instead of sending mutex lock as platform
data, we have directly referencing the mutex in child platform driver.

After making these changes, we are no longer observing mutex hang
issues. The reason for mutex hang issue is, from ACP PCI driver we are
sending mutex address directly as platform data, which results in incorrect
reference to the common lock.

We have already posted ACP PDM driver fix for upstream review.
Link: https://lore.kernel.org/alsa-devel/20230525113000.1290758-1-Vijendar.Mukunda@amd.com

We will implement similar changes for ACP SoundWire DMA driver as well.



>>>> The documentation does make mention of a copy....
>>>>
>>>> /**
>>>>  * platform_device_add_data - add platform-specific data to a platform
>>>> device
>>>>  * @pdev: platform device allocated by platform_device_alloc to add
>>>> resources to
>>>>  * @data: platform specific data for this platform device
>>>>  * @size: size of platform specific data
>>>>  *
>>>>  * Add a copy of platform specific data to the platform device's
>>>>  * platform_data pointer.  The memory associated with the platform data
>>>>  * will be freed when the platform device is released.
>>>>  */
>>>>> +	dev_set_drvdata(&pdev->dev, sdw_data);
>>>>> +	status = devm_snd_soc_register_component(&pdev->dev,
>>>>> +						 &acp63_sdw_component,
>>>>> +						 NULL, 0);
>>>>> +	if (status)
>>>>> +		dev_err(&pdev->dev, "Fail to register sdw dma component\n");
>>>>> +
>>>>> +	return status;
>>>>> +}

[PATCH V2 4/9] ASoC: amd: ps: add SoundWire dma driver dma ops

[Vijendar Mukunda]

Add SoundWire DMA driver dma ops for Pink Sardine platform.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 sound/soc/amd/ps/acp63.h      |  70 ++++++
 sound/soc/amd/ps/ps-sdw-dma.c | 415 ++++++++++++++++++++++++++++++++++
 2 files changed, 485 insertions(+)

diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
index eec58da7ec8b..37ae8f18225a 100644
--- a/sound/soc/amd/ps/acp63.h
+++ b/sound/soc/amd/ps/acp63.h
@@ -91,6 +91,46 @@
 #define ACP_SDW0_STAT		BIT(21)
 #define ACP_SDW1_STAT		BIT(2)
 #define ACP_ERROR_IRQ		BIT(29)
+#define ACP_AUDIO0_TX_THRESHOLD		0x1c
+#define ACP_AUDIO1_TX_THRESHOLD		0x1a
+#define ACP_AUDIO2_TX_THRESHOLD		0x18
+#define ACP_AUDIO0_RX_THRESHOLD		0x1b
+#define ACP_AUDIO1_RX_THRESHOLD		0x19
+#define ACP_AUDIO2_RX_THRESHOLD		0x17
+#define ACP_P1_AUDIO1_TX_THRESHOLD	BIT(6)
+#define ACP_P1_AUDIO1_RX_THRESHOLD	BIT(5)
+#define ACP_SDW_DMA_IRQ_MASK		0x1F800000
+#define ACP_P1_SDW_DMA_IRQ_MASK		0x60
+#define ACP63_SDW0_DMA_MAX_STREAMS	6
+#define ACP63_SDW1_DMA_MAX_STREAMS	2
+#define ACP_P1_AUDIO_TX_THRESHOLD	6
+#define SDW0_DMA_TX_IRQ_MASK(i)	(ACP_AUDIO0_TX_THRESHOLD - (2 * (i)))
+#define SDW0_DMA_RX_IRQ_MASK(i)	(ACP_AUDIO0_RX_THRESHOLD - (2 * (i)))
+#define SDW1_DMA_IRQ_MASK(i)	(ACP_P1_AUDIO_TX_THRESHOLD - (i))
+
+#define ACP_DELAY_US		5
+#define ACP_SDW_RING_BUFF_ADDR_OFFSET (128 * 1024)
+#define SDW0_MEM_WINDOW_START	0x4800000
+#define ACP_SDW_SRAM_PTE_OFFSET	0x03800400
+#define SDW0_PTE_OFFSET		0x400
+#define SDW_FIFO_SIZE		0x100
+#define SDW_DMA_SIZE		0x40
+#define ACP_SDW0_FIFO_OFFSET	0x100
+#define SDW_PTE_OFFSET(i)	(SDW0_PTE_OFFSET + ((i) * 0x600))
+#define ACP_SDW_FIFO_OFFSET(i)	(ACP_SDW0_FIFO_OFFSET + ((i) * 0x500))
+#define SDW_MEM_WINDOW_START(i)	(SDW0_MEM_WINDOW_START + ((i) * 0xC0000))
+
+#define SDW_PLAYBACK_MIN_NUM_PERIODS    2
+#define SDW_PLAYBACK_MAX_NUM_PERIODS    8
+#define SDW_PLAYBACK_MAX_PERIOD_SIZE    8192
+#define SDW_PLAYBACK_MIN_PERIOD_SIZE    1024
+#define SDW_CAPTURE_MIN_NUM_PERIODS     2
+#define SDW_CAPTURE_MAX_NUM_PERIODS     8
+#define SDW_CAPTURE_MAX_PERIOD_SIZE     8192
+#define SDW_CAPTURE_MIN_PERIOD_SIZE     1024
+
+#define SDW_MAX_BUFFER (SDW_PLAYBACK_MAX_PERIOD_SIZE * SDW_PLAYBACK_MAX_NUM_PERIODS)
+#define SDW_MIN_BUFFER SDW_MAX_BUFFER
 
 enum acp_config {
 	ACP_CONFIG_0 = 0,
@@ -151,6 +191,36 @@ struct pdm_dev_data {
 struct sdw_dma_dev_data {
 	void __iomem *acp_base;
 	struct mutex *acp_lock; /* used to protect acp common register access */
+	struct snd_pcm_substream *sdw0_dma_stream[ACP63_SDW0_DMA_MAX_STREAMS];
+	struct snd_pcm_substream *sdw1_dma_stream[ACP63_SDW1_DMA_MAX_STREAMS];
+};
+
+struct acp_sdw_dma_stream {
+	u16 num_pages;
+	u16 channels;
+	u32 stream_id;
+	u32 instance;
+	dma_addr_t dma_addr;
+	u64 bytescount;
+};
+
+union acp_sdw_dma_count {
+	struct {
+	u32 low;
+	u32 high;
+	} bcount;
+	u64 bytescount;
+};
+
+struct sdw_dma_ring_buf_reg {
+	u32 reg_dma_size;
+	u32 reg_fifo_addr;
+	u32 reg_fifo_size;
+	u32 reg_ring_buf_size;
+	u32 reg_ring_buf_addr;
+	u32 water_mark_size_reg;
+	u32 pos_low_reg;
+	u32 pos_high_reg;
 };
 
 /**
diff --git a/sound/soc/amd/ps/ps-sdw-dma.c b/sound/soc/amd/ps/ps-sdw-dma.c
index f41849fd035c..be68003cbd67 100644
--- a/sound/soc/amd/ps/ps-sdw-dma.c
+++ b/sound/soc/amd/ps/ps-sdw-dma.c
@@ -12,12 +12,427 @@
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
 #include <sound/soc-dai.h>
+#include <linux/soundwire/sdw_amd.h>
 #include "acp63.h"
 
 #define DRV_NAME "amd_ps_sdw_dma"
 
+static struct sdw_dma_ring_buf_reg sdw0_dma_ring_buf_reg[ACP63_SDW0_DMA_MAX_STREAMS] = {
+	{ACP_AUDIO0_TX_DMA_SIZE, ACP_AUDIO0_TX_FIFOADDR, ACP_AUDIO0_TX_FIFOSIZE,
+	 ACP_AUDIO0_TX_RINGBUFSIZE, ACP_AUDIO0_TX_RINGBUFADDR, ACP_AUDIO0_TX_INTR_WATERMARK_SIZE,
+	 ACP_AUDIO0_TX_LINEARPOSITIONCNTR_LOW, ACP_AUDIO0_TX_LINEARPOSITIONCNTR_HIGH},
+	{ACP_AUDIO1_TX_DMA_SIZE, ACP_AUDIO1_TX_FIFOADDR, ACP_AUDIO1_TX_FIFOSIZE,
+	 ACP_AUDIO1_TX_RINGBUFSIZE, ACP_AUDIO1_TX_RINGBUFADDR, ACP_AUDIO1_TX_INTR_WATERMARK_SIZE,
+	 ACP_AUDIO1_TX_LINEARPOSITIONCNTR_LOW, ACP_AUDIO1_TX_LINEARPOSITIONCNTR_HIGH},
+	{ACP_AUDIO2_TX_DMA_SIZE, ACP_AUDIO2_TX_FIFOADDR, ACP_AUDIO2_TX_FIFOSIZE,
+	 ACP_AUDIO2_TX_RINGBUFSIZE, ACP_AUDIO2_TX_RINGBUFADDR, ACP_AUDIO2_TX_INTR_WATERMARK_SIZE,
+	 ACP_AUDIO2_TX_LINEARPOSITIONCNTR_LOW, ACP_AUDIO2_TX_LINEARPOSITIONCNTR_HIGH},
+	{ACP_AUDIO0_RX_DMA_SIZE, ACP_AUDIO0_RX_FIFOADDR, ACP_AUDIO0_RX_FIFOSIZE,
+	 ACP_AUDIO0_RX_RINGBUFSIZE, ACP_AUDIO0_RX_RINGBUFADDR, ACP_AUDIO0_RX_INTR_WATERMARK_SIZE,
+	 ACP_AUDIO0_TX_LINEARPOSITIONCNTR_LOW, ACP_AUDIO0_TX_LINEARPOSITIONCNTR_HIGH},
+	{ACP_AUDIO1_RX_DMA_SIZE, ACP_AUDIO1_RX_FIFOADDR, ACP_AUDIO1_RX_FIFOSIZE,
+	 ACP_AUDIO1_RX_RINGBUFSIZE, ACP_AUDIO1_RX_RINGBUFADDR, ACP_AUDIO1_RX_INTR_WATERMARK_SIZE,
+	 ACP_AUDIO1_RX_LINEARPOSITIONCNTR_LOW, ACP_AUDIO1_RX_LINEARPOSITIONCNTR_HIGH},
+	{ACP_AUDIO2_RX_DMA_SIZE, ACP_AUDIO2_RX_FIFOADDR, ACP_AUDIO2_RX_FIFOSIZE,
+	 ACP_AUDIO2_RX_RINGBUFSIZE, ACP_AUDIO2_RX_RINGBUFADDR, ACP_AUDIO2_RX_INTR_WATERMARK_SIZE,
+	 ACP_AUDIO2_RX_LINEARPOSITIONCNTR_LOW, ACP_AUDIO2_RX_LINEARPOSITIONCNTR_HIGH}
+};
+
+static struct sdw_dma_ring_buf_reg sdw1_dma_ring_buf_reg[ACP63_SDW1_DMA_MAX_STREAMS] =  {
+	{ACP_P1_AUDIO1_TX_DMA_SIZE, ACP_P1_AUDIO1_TX_FIFOADDR, ACP_P1_AUDIO1_TX_FIFOSIZE,
+	 ACP_P1_AUDIO1_TX_RINGBUFSIZE, ACP_P1_AUDIO1_TX_RINGBUFADDR,
+	 ACP_P1_AUDIO1_TX_INTR_WATERMARK_SIZE,
+	 ACP_P1_AUDIO1_TX_LINEARPOSITIONCNTR_LOW, ACP_P1_AUDIO1_TX_LINEARPOSITIONCNTR_HIGH},
+	{ACP_P1_AUDIO1_RX_DMA_SIZE, ACP_P1_AUDIO1_RX_FIFOADDR, ACP_P1_AUDIO1_RX_FIFOSIZE,
+	 ACP_P1_AUDIO1_RX_RINGBUFSIZE, ACP_P1_AUDIO1_RX_RINGBUFADDR,
+	 ACP_P1_AUDIO1_RX_INTR_WATERMARK_SIZE,
+	 ACP_P1_AUDIO1_RX_LINEARPOSITIONCNTR_LOW, ACP_P1_AUDIO1_RX_LINEARPOSITIONCNTR_HIGH},
+};
+
+static u32 sdw0_dma_enable_reg[ACP63_SDW0_DMA_MAX_STREAMS] = {
+	ACP_SW0_AUDIO0_TX_EN,
+	ACP_SW0_AUDIO1_TX_EN,
+	ACP_SW0_AUDIO2_TX_EN,
+	ACP_SW0_AUDIO0_RX_EN,
+	ACP_SW0_AUDIO1_RX_EN,
+	ACP_SW0_AUDIO2_RX_EN,
+};
+
+static u32 sdw1_dma_enable_reg[ACP63_SDW1_DMA_MAX_STREAMS] = {
+	ACP_SW1_AUDIO1_TX_EN,
+	ACP_SW1_AUDIO1_RX_EN,
+};
+
+static const struct snd_pcm_hardware acp63_sdw_hardware_playback = {
+	.info = SNDRV_PCM_INFO_INTERLEAVED |
+		SNDRV_PCM_INFO_BLOCK_TRANSFER |
+		SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
+		SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
+	.formats = SNDRV_PCM_FMTBIT_S16_LE |  SNDRV_PCM_FMTBIT_S8 |
+		   SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
+	.channels_min = 2,
+	.channels_max = 2,
+	.rates = SNDRV_PCM_RATE_48000,
+	.rate_min = 48000,
+	.rate_max = 48000,
+	.buffer_bytes_max = SDW_PLAYBACK_MAX_NUM_PERIODS * SDW_PLAYBACK_MAX_PERIOD_SIZE,
+	.period_bytes_min = SDW_PLAYBACK_MIN_PERIOD_SIZE,
+	.period_bytes_max = SDW_PLAYBACK_MAX_PERIOD_SIZE,
+	.periods_min = SDW_PLAYBACK_MIN_NUM_PERIODS,
+	.periods_max = SDW_PLAYBACK_MAX_NUM_PERIODS,
+};
+
+static const struct snd_pcm_hardware acp63_sdw_hardware_capture = {
+	.info = SNDRV_PCM_INFO_INTERLEAVED |
+		SNDRV_PCM_INFO_BLOCK_TRANSFER |
+		SNDRV_PCM_INFO_MMAP |
+		SNDRV_PCM_INFO_MMAP_VALID |
+		SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
+	.formats = SNDRV_PCM_FMTBIT_S16_LE |  SNDRV_PCM_FMTBIT_S8 |
+		   SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
+	.channels_min = 2,
+	.channels_max = 2,
+	.rates = SNDRV_PCM_RATE_48000,
+	.rate_min = 48000,
+	.rate_max = 48000,
+	.buffer_bytes_max = SDW_CAPTURE_MAX_NUM_PERIODS * SDW_CAPTURE_MAX_PERIOD_SIZE,
+	.period_bytes_min = SDW_CAPTURE_MIN_PERIOD_SIZE,
+	.period_bytes_max = SDW_CAPTURE_MAX_PERIOD_SIZE,
+	.periods_min = SDW_CAPTURE_MIN_NUM_PERIODS,
+	.periods_max = SDW_CAPTURE_MAX_NUM_PERIODS,
+};
+
+static void acp63_config_dma(struct acp_sdw_dma_stream *stream, void __iomem *acp_base,
+			     u32 stream_id)
+{
+	u16 page_idx;
+	u32 low, high, val;
+	u32 sdw_dma_pte_offset;
+	dma_addr_t addr;
+
+	addr = stream->dma_addr;
+	sdw_dma_pte_offset = SDW_PTE_OFFSET(stream->instance);
+	val = sdw_dma_pte_offset + (stream_id * 256);
+
+	/* Group Enable */
+	writel(ACP_SDW_SRAM_PTE_OFFSET | BIT(31), acp_base + ACPAXI2AXI_ATU_BASE_ADDR_GRP_2);
+	writel(PAGE_SIZE_4K_ENABLE, acp_base + ACPAXI2AXI_ATU_PAGE_SIZE_GRP_2);
+	for (page_idx = 0; page_idx < stream->num_pages; page_idx++) {
+		/* Load the low address of page int ACP SRAM through SRBM */
+		low = lower_32_bits(addr);
+		high = upper_32_bits(addr);
+
+		writel(low, acp_base + ACP_SCRATCH_REG_0 + val);
+		high |= BIT(31);
+		writel(high, acp_base + ACP_SCRATCH_REG_0 + val + 4);
+		val += 8;
+		addr += PAGE_SIZE;
+	}
+	writel(0x1, acp_base + ACPAXI2AXI_ATU_CTRL);
+}
+
+static int acp63_configure_sdw_ringbuffer(void __iomem *acp_base, u32 stream_id, u32 size,
+					  u32 manager_instance)
+{
+	u32 reg_dma_size;
+	u32 reg_fifo_addr;
+	u32 reg_fifo_size;
+	u32 reg_ring_buf_size;
+	u32 reg_ring_buf_addr;
+	u32 sdw_fifo_addr;
+	u32 sdw_fifo_offset;
+	u32 sdw_ring_buf_addr;
+	u32 sdw_ring_buf_size;
+	u32 sdw_mem_window_offset;
+
+	switch (manager_instance) {
+	case ACP_SDW0:
+		reg_dma_size = sdw0_dma_ring_buf_reg[stream_id].reg_dma_size;
+		reg_fifo_addr =	sdw0_dma_ring_buf_reg[stream_id].reg_fifo_addr;
+		reg_fifo_size = sdw0_dma_ring_buf_reg[stream_id].reg_fifo_size;
+		reg_ring_buf_size = sdw0_dma_ring_buf_reg[stream_id].reg_ring_buf_size;
+		reg_ring_buf_addr = sdw0_dma_ring_buf_reg[stream_id].reg_ring_buf_addr;
+		break;
+	case ACP_SDW1:
+		reg_dma_size = sdw1_dma_ring_buf_reg[stream_id].reg_dma_size;
+		reg_fifo_addr =	sdw1_dma_ring_buf_reg[stream_id].reg_fifo_addr;
+		reg_fifo_size = sdw1_dma_ring_buf_reg[stream_id].reg_fifo_size;
+		reg_ring_buf_size = sdw1_dma_ring_buf_reg[stream_id].reg_ring_buf_size;
+		reg_ring_buf_addr = sdw1_dma_ring_buf_reg[stream_id].reg_ring_buf_addr;
+		break;
+	default:
+		return -EINVAL;
+	}
+	sdw_fifo_offset = ACP_SDW_FIFO_OFFSET(manager_instance);
+	sdw_mem_window_offset = SDW_MEM_WINDOW_START(manager_instance);
+	sdw_fifo_addr = sdw_fifo_offset + (stream_id * 256);
+	sdw_ring_buf_addr = sdw_mem_window_offset + (stream_id * ACP_SDW_RING_BUFF_ADDR_OFFSET);
+	sdw_ring_buf_size = size;
+	writel(sdw_ring_buf_size, acp_base + reg_ring_buf_size);
+	writel(sdw_ring_buf_addr, acp_base + reg_ring_buf_addr);
+	writel(sdw_fifo_addr, acp_base + reg_fifo_addr);
+	writel(SDW_DMA_SIZE, acp_base + reg_dma_size);
+	writel(SDW_FIFO_SIZE, acp_base + reg_fifo_size);
+	return 0;
+}
+
+static int acp63_sdw_dma_open(struct snd_soc_component *component,
+			      struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime;
+	struct acp_sdw_dma_stream *stream;
+	struct snd_soc_dai *cpu_dai;
+	struct amd_sdw_manager *amd_manager;
+	struct snd_soc_pcm_runtime *prtd = substream->private_data;
+	int ret;
+
+	runtime = substream->runtime;
+	cpu_dai = asoc_rtd_to_cpu(prtd, 0);
+	amd_manager = snd_soc_dai_get_drvdata(cpu_dai);
+	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
+	if (!stream)
+		return -ENOMEM;
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		runtime->hw = acp63_sdw_hardware_playback;
+	else
+		runtime->hw = acp63_sdw_hardware_capture;
+	ret = snd_pcm_hw_constraint_integer(runtime,
+					    SNDRV_PCM_HW_PARAM_PERIODS);
+	if (ret < 0) {
+		dev_err(component->dev, "set integer constraint failed\n");
+		kfree(stream);
+		return ret;
+	}
+
+	stream->stream_id = cpu_dai->id;
+	stream->instance = amd_manager->instance;
+	runtime->private_data = stream;
+	return ret;
+}
+
+static int acp63_sdw_dma_hw_params(struct snd_soc_component *component,
+				   struct snd_pcm_substream *substream,
+				   struct snd_pcm_hw_params *params)
+{
+	struct acp_sdw_dma_stream *stream;
+	struct sdw_dma_dev_data *sdw_data;
+	u32 period_bytes;
+	u32 water_mark_size_reg;
+	u32 irq_mask, ext_intr_ctrl;
+	u64 size;
+	u32 stream_id;
+	u32 acp_ext_intr_cntl_reg;
+	int ret;
+
+	sdw_data = dev_get_drvdata(component->dev);
+	stream = substream->runtime->private_data;
+	if (!stream)
+		return -EINVAL;
+	stream_id = stream->stream_id;
+	switch (stream->instance) {
+	case ACP_SDW0:
+		sdw_data->sdw0_dma_stream[stream_id] = substream;
+		water_mark_size_reg = sdw0_dma_ring_buf_reg[stream_id].water_mark_size_reg;
+		acp_ext_intr_cntl_reg = ACP_EXTERNAL_INTR_CNTL;
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+			irq_mask = BIT(SDW0_DMA_TX_IRQ_MASK(stream_id));
+		else
+			irq_mask = BIT(SDW0_DMA_RX_IRQ_MASK(stream_id));
+		break;
+	case ACP_SDW1:
+		sdw_data->sdw1_dma_stream[stream_id] = substream;
+		acp_ext_intr_cntl_reg = ACP_EXTERNAL_INTR_CNTL1;
+		water_mark_size_reg = sdw1_dma_ring_buf_reg[stream_id].water_mark_size_reg;
+		irq_mask = BIT(SDW1_DMA_IRQ_MASK(stream_id));
+		break;
+	default:
+		return -EINVAL;
+	}
+	size = params_buffer_bytes(params);
+	period_bytes = params_period_bytes(params);
+	stream->dma_addr = substream->runtime->dma_addr;
+	stream->num_pages = (PAGE_ALIGN(size) >> PAGE_SHIFT);
+	acp63_config_dma(stream, sdw_data->acp_base, stream_id);
+	ret = acp63_configure_sdw_ringbuffer(sdw_data->acp_base, stream_id, size,
+					     stream->instance);
+	if (ret) {
+		dev_err(component->dev, "Invalid DMA channel\n");
+		return -EINVAL;
+	}
+	ext_intr_ctrl = readl(sdw_data->acp_base + acp_ext_intr_cntl_reg);
+	ext_intr_ctrl |= irq_mask;
+	writel(ext_intr_ctrl, sdw_data->acp_base + acp_ext_intr_cntl_reg);
+	writel(period_bytes, sdw_data->acp_base + water_mark_size_reg);
+	return 0;
+}
+
+static u64 acp63_sdw_get_byte_count(struct acp_sdw_dma_stream *stream, void __iomem *acp_base)
+{
+	union acp_sdw_dma_count byte_count;
+	u32 pos_low_reg, pos_high_reg;
+
+	byte_count.bytescount = 0;
+	switch (stream->instance) {
+	case ACP_SDW0:
+		pos_low_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
+		pos_high_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
+		break;
+	case ACP_SDW1:
+		pos_low_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
+		pos_high_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
+		break;
+	default:
+		return -EINVAL;
+	}
+	if (pos_low_reg) {
+		byte_count.bcount.high = readl(acp_base + pos_high_reg);
+		byte_count.bcount.low = readl(acp_base + pos_low_reg);
+	}
+	return byte_count.bytescount;
+}
+
+static snd_pcm_uframes_t acp63_sdw_dma_pointer(struct snd_soc_component *comp,
+					       struct snd_pcm_substream *substream)
+{
+	struct sdw_dma_dev_data *sdw_data;
+	struct acp_sdw_dma_stream *stream;
+	u32 pos, buffersize;
+	u64 bytescount;
+
+	sdw_data = dev_get_drvdata(comp->dev);
+	stream = substream->runtime->private_data;
+	buffersize = frames_to_bytes(substream->runtime,
+				     substream->runtime->buffer_size);
+	bytescount = acp63_sdw_get_byte_count(stream, sdw_data->acp_base);
+	if (bytescount > stream->bytescount)
+		bytescount -= stream->bytescount;
+	pos = do_div(bytescount, buffersize);
+	return bytes_to_frames(substream->runtime, pos);
+}
+
+static int acp63_sdw_dma_new(struct snd_soc_component *component,
+			     struct snd_soc_pcm_runtime *rtd)
+{
+	struct device *parent = component->dev->parent;
+
+	snd_pcm_set_managed_buffer_all(rtd->pcm, SNDRV_DMA_TYPE_DEV,
+				       parent, SDW_MIN_BUFFER, SDW_MAX_BUFFER);
+	return 0;
+}
+
+static int acp63_sdw_dma_close(struct snd_soc_component *component,
+			       struct snd_pcm_substream *substream)
+{
+	struct sdw_dma_dev_data *sdw_data;
+	struct acp_sdw_dma_stream *stream;
+
+	sdw_data = dev_get_drvdata(component->dev);
+	stream = substream->runtime->private_data;
+	if (!stream)
+		return -EINVAL;
+	switch (stream->instance) {
+	case ACP_SDW0:
+		sdw_data->sdw0_dma_stream[stream->stream_id] = NULL;
+		break;
+	case ACP_SDW1:
+		sdw_data->sdw1_dma_stream[stream->stream_id] = NULL;
+		break;
+	default:
+		return -EINVAL;
+	}
+	kfree(stream);
+	return 0;
+}
+
+static int acp63_sdw_dma_start(struct snd_pcm_substream *substream, void __iomem *acp_base)
+{
+	struct acp_sdw_dma_stream *stream;
+	u32 stream_id;
+	u32 sdw_dma_en_reg;
+	u32 sdw_dma_en_stat_reg;
+	u32 sdw_dma_stat;
+
+	stream = substream->runtime->private_data;
+	stream_id = stream->stream_id;
+	switch (stream->instance) {
+	case ACP_SDW0:
+		sdw_dma_en_reg = sdw0_dma_enable_reg[stream_id];
+		break;
+	case ACP_SDW1:
+		sdw_dma_en_reg = sdw1_dma_enable_reg[stream_id];
+		break;
+	default:
+		return -EINVAL;
+	}
+	writel(0x01, acp_base + sdw_dma_en_reg);
+	sdw_dma_en_stat_reg = sdw_dma_en_reg + 4;
+	return readl_poll_timeout(acp_base + sdw_dma_en_stat_reg, sdw_dma_stat,
+				  (sdw_dma_stat & BIT(0)), ACP_DELAY_US, ACP_COUNTER);
+}
+
+static int acp63_sdw_dma_stop(struct snd_pcm_substream *substream, void __iomem *acp_base)
+{
+	struct acp_sdw_dma_stream *stream;
+	u32 stream_id;
+	u32 sdw_dma_en_reg;
+	u32 sdw_dma_en_stat_reg;
+	u32 sdw_dma_stat;
+
+	stream = substream->runtime->private_data;
+	stream_id = stream->stream_id;
+	switch (stream->instance) {
+	case ACP_SDW0:
+		sdw_dma_en_reg = sdw0_dma_enable_reg[stream_id];
+		break;
+	case ACP_SDW1:
+		sdw_dma_en_reg = sdw1_dma_enable_reg[stream_id];
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	writel(0, acp_base + sdw_dma_en_reg);
+	sdw_dma_en_stat_reg = sdw_dma_en_reg + 4;
+	return readl_poll_timeout(acp_base + sdw_dma_en_stat_reg, sdw_dma_stat, !sdw_dma_stat,
+				  ACP_DELAY_US, ACP_COUNTER);
+}
+
+static int acp63_sdw_dma_trigger(struct snd_soc_component *comp,
+				 struct snd_pcm_substream *substream,
+				 int cmd)
+{
+	struct sdw_dma_dev_data *sdw_data;
+	int ret;
+
+	sdw_data = dev_get_drvdata(comp->dev);
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+	case SNDRV_PCM_TRIGGER_RESUME:
+		ret = acp63_sdw_dma_start(substream, sdw_data->acp_base);
+		break;
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_STOP:
+		ret = acp63_sdw_dma_stop(substream, sdw_data->acp_base);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	if (ret)
+		dev_err(comp->dev, "trigger %d failed: %d", cmd, ret);
+	return ret;
+}
+
 static const struct snd_soc_component_driver acp63_sdw_component = {
 	.name		= DRV_NAME,
+	.open		= acp63_sdw_dma_open,
+	.close		= acp63_sdw_dma_close,
+	.hw_params	= acp63_sdw_dma_hw_params,
+	.trigger	= acp63_sdw_dma_trigger,
+	.pointer	= acp63_sdw_dma_pointer,
+	.pcm_construct	= acp63_sdw_dma_new,
 };
 
 static int acp63_sdw_platform_probe(struct platform_device *pdev)
-- 
2.34.1

Re: [PATCH V2 4/9] ASoC: amd: ps: add SoundWire dma driver dma ops

[Pierre-Louis Bossart]

> +union acp_sdw_dma_count {
> +	struct {
> +	u32 low;
> +	u32 high;
> +	} bcount;

indentation seems off?

> +	u64 bytescount;
> +};
> +
> +struct sdw_dma_ring_buf_reg {
> +	u32 reg_dma_size;
> +	u32 reg_fifo_addr;
> +	u32 reg_fifo_size;
> +	u32 reg_ring_buf_size;
> +	u32 reg_ring_buf_addr;
> +	u32 water_mark_size_reg;
> +	u32 pos_low_reg;
> +	u32 pos_high_reg;
>  };
>\
> +static void acp63_config_dma(struct acp_sdw_dma_stream *stream, void __iomem *acp_base,
> +			     u32 stream_id)
> +{
> +	u16 page_idx;
> +	u32 low, high, val;
> +	u32 sdw_dma_pte_offset;
> +	dma_addr_t addr;
> +
> +	addr = stream->dma_addr;
> +	sdw_dma_pte_offset = SDW_PTE_OFFSET(stream->instance);
> +	val = sdw_dma_pte_offset + (stream_id * 256);

what is this 256 magic value? use a defined or << 8 ?
> +
> +	/* Group Enable */
> +	writel(ACP_SDW_SRAM_PTE_OFFSET | BIT(31), acp_base + ACPAXI2AXI_ATU_BASE_ADDR_GRP_2);
> +	writel(PAGE_SIZE_4K_ENABLE, acp_base + ACPAXI2AXI_ATU_PAGE_SIZE_GRP_2);
> +	for (page_idx = 0; page_idx < stream->num_pages; page_idx++) {
> +		/* Load the low address of page int ACP SRAM through SRBM */
> +		low = lower_32_bits(addr);
> +		high = upper_32_bits(addr);
> +
> +		writel(low, acp_base + ACP_SCRATCH_REG_0 + val);
> +		high |= BIT(31);
> +		writel(high, acp_base + ACP_SCRATCH_REG_0 + val + 4);
> +		val += 8;
> +		addr += PAGE_SIZE;
> +	}
> +	writel(0x1, acp_base + ACPAXI2AXI_ATU_CTRL);
> +}

Re: [PATCH V2 4/9] ASoC: amd: ps: add SoundWire dma driver dma ops

[Mukunda,Vijendar]

On 22/05/23 22:09, Pierre-Louis Bossart wrote:
>> +union acp_sdw_dma_count {
>> +	struct {
>> +	u32 low;
>> +	u32 high;
>> +	} bcount;
> indentation seems off?
will fix it.
>> +	u64 bytescount;
>> +};
>> +
>> +struct sdw_dma_ring_buf_reg {
>> +	u32 reg_dma_size;
>> +	u32 reg_fifo_addr;
>> +	u32 reg_fifo_size;
>> +	u32 reg_ring_buf_size;
>> +	u32 reg_ring_buf_addr;
>> +	u32 water_mark_size_reg;
>> +	u32 pos_low_reg;
>> +	u32 pos_high_reg;
>>  };
>> \
>> +static void acp63_config_dma(struct acp_sdw_dma_stream *stream, void __iomem *acp_base,
>> +			     u32 stream_id)
>> +{
>> +	u16 page_idx;
>> +	u32 low, high, val;
>> +	u32 sdw_dma_pte_offset;
>> +	dma_addr_t addr;
>> +
>> +	addr = stream->dma_addr;
>> +	sdw_dma_pte_offset = SDW_PTE_OFFSET(stream->instance);
>> +	val = sdw_dma_pte_offset + (stream_id * 256);
> what is this 256 magic value? use a defined or << 8 ?
This value is used to map to 32-page addresses in scratch memory PTE mapping.
Will use macro for it.
>> +
>> +	/* Group Enable */
>> +	writel(ACP_SDW_SRAM_PTE_OFFSET | BIT(31), acp_base + ACPAXI2AXI_ATU_BASE_ADDR_GRP_2);
>> +	writel(PAGE_SIZE_4K_ENABLE, acp_base + ACPAXI2AXI_ATU_PAGE_SIZE_GRP_2);
>> +	for (page_idx = 0; page_idx < stream->num_pages; page_idx++) {
>> +		/* Load the low address of page int ACP SRAM through SRBM */
>> +		low = lower_32_bits(addr);
>> +		high = upper_32_bits(addr);
>> +
>> +		writel(low, acp_base + ACP_SCRATCH_REG_0 + val);
>> +		high |= BIT(31);
>> +		writel(high, acp_base + ACP_SCRATCH_REG_0 + val + 4);
>> +		val += 8;
>> +		addr += PAGE_SIZE;
>> +	}
>> +	writel(0x1, acp_base + ACPAXI2AXI_ATU_CTRL);
>> +}

[PATCH V2 5/9] ASoC: amd: ps: add support for SoundWire DMA interrupts

[Vijendar Mukunda]

Initialize workqueue for SoundWire DMA interrupts handling.
Whenever audio data equal to the SoundWire FIFO watermark level
are produced/consumed, interrupt is generated.
Acknowledge the interrupt and schedule the workqueue.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 sound/soc/amd/ps/acp63.h  | 20 +++++++++
 sound/soc/amd/ps/pci-ps.c | 87 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 106 insertions(+), 1 deletion(-)

diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
index 37ae8f18225a..1a3e47fb9d02 100644
--- a/sound/soc/amd/ps/acp63.h
+++ b/sound/soc/amd/ps/acp63.h
@@ -173,6 +173,20 @@ enum acp_pdev_mask {
 	ACP63_SDW_PDM_DEV_MASK,
 };
 
+enum amd_sdw0_channel {
+	ACP_SDW0_AUDIO0_TX = 0,
+	ACP_SDW0_AUDIO1_TX,
+	ACP_SDW0_AUDIO2_TX,
+	ACP_SDW0_AUDIO0_RX,
+	ACP_SDW0_AUDIO1_RX,
+	ACP_SDW0_AUDIO2_RX,
+};
+
+enum amd_sdw1_channel {
+	ACP_SDW1_AUDIO1_TX,
+	ACP_SDW1_AUDIO1_RX,
+};
+
 struct pdm_stream_instance {
 	u16 num_pages;
 	u16 channels;
@@ -230,6 +244,7 @@ struct sdw_dma_ring_buf_reg {
  * @pdev: array of child platform device node structures
  * @acp_lock: used to protect acp common registers
  * @sdw_fw_node: SoundWire controller fw node handle
+ * @acp_sdw_dma_work: SoundWire DMA interrupts workqueue
  * @pdev_mask: platform device mask
  * @pdev_count: platform devices count
  * @pdm_dev_index: pdm platform device index
@@ -237,6 +252,8 @@ struct sdw_dma_ring_buf_reg {
  * @sdw0_dev_index: SoundWire Manager-0 platform device index
  * @sdw1_dev_index: SoundWire Manager-1 platform device index
  * @sdw_dma_dev_index: SoundWire DMA controller platform device index
+ * @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
  * @acp_reset: flag set to true when bus reset is applied across all
  * the active SoundWire manager instances
  */
@@ -247,6 +264,7 @@ struct acp63_dev_data {
 	struct platform_device *pdev[ACP63_DEVS];
 	struct mutex acp_lock; /* protect shared registers */
 	struct fwnode_handle *sdw_fw_node;
+	struct work_struct acp_sdw_dma_work;
 	u16 pdev_mask;
 	u16 pdev_count;
 	u16 pdm_dev_index;
@@ -254,6 +272,8 @@ struct acp63_dev_data {
 	u16 sdw0_dev_index;
 	u16 sdw1_dev_index;
 	u16 sdw_dma_dev_index;
+	u16 sdw0_dma_intr_stat[ACP63_SDW0_DMA_MAX_STREAMS];
+	u16 sdw1_dma_intr_stat[ACP63_SDW1_DMA_MAX_STREAMS];
 	bool acp_reset;
 };
 
diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
index 26514e340a33..c99ac5d90097 100644
--- a/sound/soc/amd/ps/pci-ps.c
+++ b/sound/soc/amd/ps/pci-ps.c
@@ -99,14 +99,44 @@ static int acp63_deinit(void __iomem *acp_base, struct device *dev)
 	return 0;
 }
 
+static void acp63_sdw_dma_workthread(struct work_struct *work)
+{
+	struct acp63_dev_data *adata = container_of(work, struct acp63_dev_data,
+						    acp_sdw_dma_work);
+	struct sdw_dma_dev_data *sdw_dma_data;
+	u32 stream_index;
+	u16 pdev_index;
+
+	pdev_index = adata->sdw_dma_dev_index;
+	sdw_dma_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev);
+
+	for (stream_index = 0; stream_index < ACP63_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 < ACP63_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;
+		}
+	}
+}
+
 static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
 {
 	struct acp63_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 pdev_index;
+	u16 index;
 
 	adata = dev_id;
 	if (!adata)
@@ -148,7 +178,57 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
 			snd_pcm_period_elapsed(ps_pdm_data->capture_stream);
 		irq_flag = 1;
 	}
-	if (irq_flag)
+	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->acp63_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;
+			}
+		}
+	}
+
+	/* SDW1 BT RX */
+	if (ext_intr_stat1 & ACP_P1_AUDIO1_RX_THRESHOLD) {
+		writel(ACP_P1_AUDIO1_RX_THRESHOLD,
+		       adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
+		adata->sdw1_dma_intr_stat[ACP_SDW1_AUDIO1_RX] = 1;
+		sdw_dma_irq_flag = 1;
+	}
+
+	/* SDW1 BT TX*/
+	if (ext_intr_stat1 & ACP_P1_AUDIO1_TX_THRESHOLD) {
+		writel(ACP_P1_AUDIO1_TX_THRESHOLD,
+		       adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
+		adata->sdw1_dma_intr_stat[ACP_SDW1_AUDIO1_TX] = 1;
+		sdw_dma_irq_flag = 1;
+	}
+
+	if (sdw_dma_irq_flag)
+		schedule_work(&adata->acp_sdw_dma_work);
+
+	if (irq_flag || sdw_dma_irq_flag)
 		return IRQ_HANDLED;
 	else
 		return IRQ_NONE;
@@ -231,6 +311,7 @@ static int get_acp63_device_config(u32 config, struct pci_dev *pci, struct acp63
 	sdw_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_SDW_ADDR, 0);
 	if (sdw_dev) {
 		acp_data->sdw_fw_node = acpi_fwnode_handle(sdw_dev);
+		INIT_WORK(&acp_data->acp_sdw_dma_work, acp63_sdw_dma_workthread);
 		ret = sdw_amd_scan_controller(&pci->dev);
 		/* is_sdw_dev flag will be set when SoundWire Manager device exists */
 		if (!ret)
@@ -610,6 +691,10 @@ static void snd_acp63_remove(struct pci_dev *pci)
 	int ret, index;
 
 	adata = pci_get_drvdata(pci);
+
+	if (adata->pdev_mask & ACP63_SDW_DEV_MASK)
+		cancel_work_sync(&adata->acp_sdw_dma_work);
+
 	for (index = 0; index < adata->pdev_count; index++)
 		platform_device_unregister(adata->pdev[index]);
 	ret = acp63_deinit(adata->acp63_base, &pci->dev);
-- 
2.34.1

Re: [PATCH V2 5/9] ASoC: amd: ps: add support for SoundWire DMA interrupts

[Pierre-Louis Bossart]

On 5/22/23 08:31, Vijendar Mukunda wrote:
> Initialize workqueue for SoundWire DMA interrupts handling.
> Whenever audio data equal to the SoundWire FIFO watermark level
> are produced/consumed, interrupt is generated.
> Acknowledge the interrupt and schedule the workqueue.

It would help to explain why a work queue is needed is the first place,
as opposed to handling periods in the interrupt thread.

> +static void acp63_sdw_dma_workthread(struct work_struct *work)
> +{
> +	struct acp63_dev_data *adata = container_of(work, struct acp63_dev_data,
> +						    acp_sdw_dma_work);
> +	struct sdw_dma_dev_data *sdw_dma_data;
> +	u32 stream_index;
> +	u16 pdev_index;
> +
> +	pdev_index = adata->sdw_dma_dev_index;
> +	sdw_dma_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev);
> +
> +	for (stream_index = 0; stream_index < ACP63_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 < ACP63_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;
> +		}
> +	}

I am not clear on the benefits of the workqueue which only tests a flag
that's set ...

> +}
> +
>  static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
>  {
>  	struct acp63_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 pdev_index;
> +	u16 index;
>  
>  	adata = dev_id;
>  	if (!adata)
> @@ -148,7 +178,57 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
>  			snd_pcm_period_elapsed(ps_pdm_data->capture_stream);
>  		irq_flag = 1;
>  	}
> -	if (irq_flag)
> +	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->acp63_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;

.. here ...
> +				sdw_dma_irq_flag = 1;
> +			}
> +		}
> +	}
> +
> +	/* SDW1 BT RX */
> +	if (ext_intr_stat1 & ACP_P1_AUDIO1_RX_THRESHOLD) {
> +		writel(ACP_P1_AUDIO1_RX_THRESHOLD,
> +		       adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
> +		adata->sdw1_dma_intr_stat[ACP_SDW1_AUDIO1_RX] = 1;

... and here ...

> +		sdw_dma_irq_flag = 1;
> +	}
> +
> +	/* SDW1 BT TX*/
> +	if (ext_intr_stat1 & ACP_P1_AUDIO1_TX_THRESHOLD) {
> +		writel(ACP_P1_AUDIO1_TX_THRESHOLD,
> +		       adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
> +		adata->sdw1_dma_intr_stat[ACP_SDW1_AUDIO1_TX] = 1;

... or here ...

> +		sdw_dma_irq_flag = 1;
> +	}
> +
> +	if (sdw_dma_irq_flag)
> +		schedule_work(&adata->acp_sdw_dma_work);
> +
> +	if (irq_flag || sdw_dma_irq_flag)
>  		return IRQ_HANDLED;
>  	else
>  		return IRQ_NONE;

Re: [PATCH V2 5/9] ASoC: amd: ps: add support for SoundWire DMA interrupts

[Mukunda,Vijendar]

On 22/05/23 23:42, Pierre-Louis Bossart wrote:
>
> On 5/22/23 08:31, Vijendar Mukunda wrote:
>> Initialize workqueue for SoundWire DMA interrupts handling.
>> Whenever audio data equal to the SoundWire FIFO watermark level
>> are produced/consumed, interrupt is generated.
>> Acknowledge the interrupt and schedule the workqueue.
> It would help to explain why a work queue is needed is the first place,
> as opposed to handling periods in the interrupt thread.
For SoundWire DAI link, we are setting nonatomic flag to true.
If we return period elapsed from hard irq handler instead of workqueue,
soft lock up is observed during stream closure.

We can use interrupt thread as well. To have a symmetry with
SoundWire manager work queues, we have used workqueue for
DMA interrupts.

>> +static void acp63_sdw_dma_workthread(struct work_struct *work)
>> +{
>> +	struct acp63_dev_data *adata = container_of(work, struct acp63_dev_data,
>> +						    acp_sdw_dma_work);
>> +	struct sdw_dma_dev_data *sdw_dma_data;
>> +	u32 stream_index;
>> +	u16 pdev_index;
>> +
>> +	pdev_index = adata->sdw_dma_dev_index;
>> +	sdw_dma_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev);
>> +
>> +	for (stream_index = 0; stream_index < ACP63_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 < ACP63_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;
>> +		}
>> +	}
> I am not clear on the benefits of the workqueue which only tests a flag
> that's set ...
In top half, we are checking all stream irq mask and setting
corresponding stream id index in interrupt status array when dma
irq is raised.

Our intention is to handle snd_pcm_period_elapsed in process context.
if the flag is set, call the period elapsed for the substream based on stream
id in work queue.
>
>> +}
>> +
>>  static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
>>  {
>>  	struct acp63_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 pdev_index;
>> +	u16 index;
>>  
>>  	adata = dev_id;
>>  	if (!adata)
>> @@ -148,7 +178,57 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
>>  			snd_pcm_period_elapsed(ps_pdm_data->capture_stream);
>>  		irq_flag = 1;
>>  	}
>> -	if (irq_flag)
>> +	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->acp63_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;
> .. here ...
Please refer above comment.
>> +				sdw_dma_irq_flag = 1;
>> +			}
>> +		}
>> +	}
>> +
>> +	/* SDW1 BT RX */
>> +	if (ext_intr_stat1 & ACP_P1_AUDIO1_RX_THRESHOLD) {
>> +		writel(ACP_P1_AUDIO1_RX_THRESHOLD,
>> +		       adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
>> +		adata->sdw1_dma_intr_stat[ACP_SDW1_AUDIO1_RX] = 1;
> ... and here ...
>
>> +		sdw_dma_irq_flag = 1;
>> +	}
>> +
>> +	/* SDW1 BT TX*/
>> +	if (ext_intr_stat1 & ACP_P1_AUDIO1_TX_THRESHOLD) {
>> +		writel(ACP_P1_AUDIO1_TX_THRESHOLD,
>> +		       adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
>> +		adata->sdw1_dma_intr_stat[ACP_SDW1_AUDIO1_TX] = 1;
> ... or here ...
>
>> +		sdw_dma_irq_flag = 1;
>> +	}
>> +
>> +	if (sdw_dma_irq_flag)
>> +		schedule_work(&adata->acp_sdw_dma_work);
>> +
>> +	if (irq_flag || sdw_dma_irq_flag)
>>  		return IRQ_HANDLED;
>>  	else
>>  		return IRQ_NONE;

Re: [PATCH V2 5/9] ASoC: amd: ps: add support for SoundWire DMA interrupts

[Pierre-Louis Bossart]

On 5/23/23 02:36, Mukunda,Vijendar wrote:
> On 22/05/23 23:42, Pierre-Louis Bossart wrote:
>>
>> On 5/22/23 08:31, Vijendar Mukunda wrote:
>>> Initialize workqueue for SoundWire DMA interrupts handling.
>>> Whenever audio data equal to the SoundWire FIFO watermark level
>>> are produced/consumed, interrupt is generated.
>>> Acknowledge the interrupt and schedule the workqueue.
>> It would help to explain why a work queue is needed is the first place,
>> as opposed to handling periods in the interrupt thread.
> For SoundWire DAI link, we are setting nonatomic flag to true.
> If we return period elapsed from hard irq handler instead of workqueue,
> soft lock up is observed during stream closure.
> 
> We can use interrupt thread as well. To have a symmetry with
> SoundWire manager work queues, we have used workqueue for
> DMA interrupts.

Oh, I completely missed the model here.

If you are using the bottom half/hard irq handler to read status
information, the natural thing to do would be to have an irq thread, no?

Not sure I see the benefit of aligning with the manager work queues -
unless it makes your life simpler to avoid race conditions with
cancel_work_sync()?

>>> +static void acp63_sdw_dma_workthread(struct work_struct *work)
>>> +{
>>> +	struct acp63_dev_data *adata = container_of(work, struct acp63_dev_data,
>>> +						    acp_sdw_dma_work);
>>> +	struct sdw_dma_dev_data *sdw_dma_data;
>>> +	u32 stream_index;
>>> +	u16 pdev_index;
>>> +
>>> +	pdev_index = adata->sdw_dma_dev_index;
>>> +	sdw_dma_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev);
>>> +
>>> +	for (stream_index = 0; stream_index < ACP63_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 < ACP63_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;
>>> +		}
>>> +	}
>> I am not clear on the benefits of the workqueue which only tests a flag
>> that's set ...
> In top half, we are checking all stream irq mask and setting
> corresponding stream id index in interrupt status array when dma
> irq is raised.
> 
> Our intention is to handle snd_pcm_period_elapsed in process context.
> if the flag is set, call the period elapsed for the substream based on stream
> id in work queue.

Re: [PATCH V2 5/9] ASoC: amd: ps: add support for SoundWire DMA interrupts

[Mukunda,Vijendar]

On 23/05/23 20:30, Pierre-Louis Bossart wrote:
>
> On 5/23/23 02:36, Mukunda,Vijendar wrote:
>> On 22/05/23 23:42, Pierre-Louis Bossart wrote:
>>> On 5/22/23 08:31, Vijendar Mukunda wrote:
>>>> Initialize workqueue for SoundWire DMA interrupts handling.
>>>> Whenever audio data equal to the SoundWire FIFO watermark level
>>>> are produced/consumed, interrupt is generated.
>>>> Acknowledge the interrupt and schedule the workqueue.
>>> It would help to explain why a work queue is needed is the first place,
>>> as opposed to handling periods in the interrupt thread.
>> For SoundWire DAI link, we are setting nonatomic flag to true.
>> If we return period elapsed from hard irq handler instead of workqueue,
>> soft lock up is observed during stream closure.
>>
>> We can use interrupt thread as well. To have a symmetry with
>> SoundWire manager work queues, we have used workqueue for
>> DMA interrupts.
> Oh, I completely missed the model here.
>
> If you are using the bottom half/hard irq handler to read status
> information, the natural thing to do would be to have an irq thread, no?
>
> Not sure I see the benefit of aligning with the manager work queues -
> unless it makes your life simpler to avoid race conditions with
> cancel_work_sync()?
We can implement request_threaded_irq() and move the handling of
DMA interrupts to thread function whereas we need to handle SoundWire
manager interrupts in top half only. Reason as follows.

As per our design, we are not masking the interrupts in top half and
restoring mask after thread execution like Intel and
our IP supports line based interrupts. If we move SoundWire manager
interrupt handling to thread function, we have observed interrupts are
reported but not handled properly due to thread execution is in progress
sometimes.
we will add comments for this design constraint in the code if we have to
go with threaded_irq implementation.
>
>>>> +static void acp63_sdw_dma_workthread(struct work_struct *work)
>>>> +{
>>>> +	struct acp63_dev_data *adata = container_of(work, struct acp63_dev_data,
>>>> +						    acp_sdw_dma_work);
>>>> +	struct sdw_dma_dev_data *sdw_dma_data;
>>>> +	u32 stream_index;
>>>> +	u16 pdev_index;
>>>> +
>>>> +	pdev_index = adata->sdw_dma_dev_index;
>>>> +	sdw_dma_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev);
>>>> +
>>>> +	for (stream_index = 0; stream_index < ACP63_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 < ACP63_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;
>>>> +		}
>>>> +	}
>>> I am not clear on the benefits of the workqueue which only tests a flag
>>> that's set ...
>> In top half, we are checking all stream irq mask and setting
>> corresponding stream id index in interrupt status array when dma
>> irq is raised.
>>
>> Our intention is to handle snd_pcm_period_elapsed in process context.
>> if the flag is set, call the period elapsed for the substream based on stream
>> id in work queue.

Re: [PATCH V2 5/9] ASoC: amd: ps: add support for SoundWire DMA interrupts

[Mukunda,Vijendar]

On 24/05/23 13:15, Mukunda,Vijendar wrote:
> On 23/05/23 20:30, Pierre-Louis Bossart wrote:
>> On 5/23/23 02:36, Mukunda,Vijendar wrote:
>>> On 22/05/23 23:42, Pierre-Louis Bossart wrote:
>>>> On 5/22/23 08:31, Vijendar Mukunda wrote:
>>>>> Initialize workqueue for SoundWire DMA interrupts handling.
>>>>> Whenever audio data equal to the SoundWire FIFO watermark level
>>>>> are produced/consumed, interrupt is generated.
>>>>> Acknowledge the interrupt and schedule the workqueue.
>>>> It would help to explain why a work queue is needed is the first place,
>>>> as opposed to handling periods in the interrupt thread.
>>> For SoundWire DAI link, we are setting nonatomic flag to true.
>>> If we return period elapsed from hard irq handler instead of workqueue,
>>> soft lock up is observed during stream closure.
>>>
>>> We can use interrupt thread as well. To have a symmetry with
>>> SoundWire manager work queues, we have used workqueue for
>>> DMA interrupts.
>> Oh, I completely missed the model here.
>>
>> If you are using the bottom half/hard irq handler to read status
>> information, the natural thing to do would be to have an irq thread, no?
>>
>> Not sure I see the benefit of aligning with the manager work queues -
>> unless it makes your life simpler to avoid race conditions with
>> cancel_work_sync()?
> We can implement request_threaded_irq() and move the handling of
> DMA interrupts to thread function whereas we need to handle SoundWire
> manager interrupts in top half only. Reason as follows.
>
> As per our design, we are not masking the interrupts in top half and
> restoring mask after thread execution like Intel and
> our IP supports line based interrupts. If we move SoundWire manager
> interrupt handling to thread function, we have observed interrupts are
> reported but not handled properly due to thread execution is in progress
> sometimes.
> we will add comments for this design constraint in the code if we have to
> go with threaded_irq implementation.
>
> @Bossart: we are waiting for your reply.
>>>>> +static void acp63_sdw_dma_workthread(struct work_struct *work)
>>>>> +{
>>>>> +	struct acp63_dev_data *adata = container_of(work, struct acp63_dev_data,
>>>>> +						    acp_sdw_dma_work);
>>>>> +	struct sdw_dma_dev_data *sdw_dma_data;
>>>>> +	u32 stream_index;
>>>>> +	u16 pdev_index;
>>>>> +
>>>>> +	pdev_index = adata->sdw_dma_dev_index;
>>>>> +	sdw_dma_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev);
>>>>> +
>>>>> +	for (stream_index = 0; stream_index < ACP63_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 < ACP63_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;
>>>>> +		}
>>>>> +	}
>>>> I am not clear on the benefits of the workqueue which only tests a flag
>>>> that's set ...
>>> In top half, we are checking all stream irq mask and setting
>>> corresponding stream id index in interrupt status array when dma
>>> irq is raised.
>>>
>>> Our intention is to handle snd_pcm_period_elapsed in process context.
>>> if the flag is set, call the period elapsed for the substream based on stream
>>> id in work queue.

Re: [PATCH V2 5/9] ASoC: amd: ps: add support for SoundWire DMA interrupts

[Mukunda,Vijendar]

On 31/05/23 19:23, Pierre-Louis Bossart wrote:
>
> On 5/31/23 02:28, Mukunda,Vijendar wrote:
>> On 24/05/23 13:15, Mukunda,Vijendar wrote:
>>> On 23/05/23 20:30, Pierre-Louis Bossart wrote:
>>>> On 5/23/23 02:36, Mukunda,Vijendar wrote:
>>>>> On 22/05/23 23:42, Pierre-Louis Bossart wrote:
>>>>>> On 5/22/23 08:31, Vijendar Mukunda wrote:
>>>>>>> Initialize workqueue for SoundWire DMA interrupts handling.
>>>>>>> Whenever audio data equal to the SoundWire FIFO watermark level
>>>>>>> are produced/consumed, interrupt is generated.
>>>>>>> Acknowledge the interrupt and schedule the workqueue.
>>>>>> It would help to explain why a work queue is needed is the first place,
>>>>>> as opposed to handling periods in the interrupt thread.
>>>>> For SoundWire DAI link, we are setting nonatomic flag to true.
>>>>> If we return period elapsed from hard irq handler instead of workqueue,
>>>>> soft lock up is observed during stream closure.
>>>>>
>>>>> We can use interrupt thread as well. To have a symmetry with
>>>>> SoundWire manager work queues, we have used workqueue for
>>>>> DMA interrupts.
>>>> Oh, I completely missed the model here.
>>>>
>>>> If you are using the bottom half/hard irq handler to read status
>>>> information, the natural thing to do would be to have an irq thread, no?
>>>>
>>>> Not sure I see the benefit of aligning with the manager work queues -
>>>> unless it makes your life simpler to avoid race conditions with
>>>> cancel_work_sync()?
>>> We can implement request_threaded_irq() and move the handling of
>>> DMA interrupts to thread function whereas we need to handle SoundWire
>>> manager interrupts in top half only. Reason as follows.
>>>
>>> As per our design, we are not masking the interrupts in top half and
>>> restoring mask after thread execution like Intel and
>>> our IP supports line based interrupts. If we move SoundWire manager
>>> interrupt handling to thread function, we have observed interrupts are
>>> reported but not handled properly due to thread execution is in progress
>>> sometimes.
>>> we will add comments for this design constraint in the code if we have to
>>> go with threaded_irq implementation.
>>>
>>> @Bossart: we are waiting for your reply.
> I am not sure I get the point about using workqueues v. threads for the
> manager, which in turn makes it difficult to understand why the DMA
> interrupt handling should be aligned with that of the manager interrupt
> handling.
>
> Using the combination of hard irq handler + workqueue feels odd. I may
> very well 'work' but others should chime in since I am far from the most
> knowledgeable reviewer in this area.
Understood your point. We will use irq thread instead of workqueue
for SoundWire DMA interrupts handling.
We will push V3 version.
>
>>>>>>> +static void acp63_sdw_dma_workthread(struct work_struct *work)
>>>>>>> +{
>>>>>>> +	struct acp63_dev_data *adata = container_of(work, struct acp63_dev_data,
>>>>>>> +						    acp_sdw_dma_work);
>>>>>>> +	struct sdw_dma_dev_data *sdw_dma_data;
>>>>>>> +	u32 stream_index;
>>>>>>> +	u16 pdev_index;
>>>>>>> +
>>>>>>> +	pdev_index = adata->sdw_dma_dev_index;
>>>>>>> +	sdw_dma_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev);
>>>>>>> +
>>>>>>> +	for (stream_index = 0; stream_index < ACP63_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 < ACP63_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;
>>>>>>> +		}
>>>>>>> +	}
>>>>>> I am not clear on the benefits of the workqueue which only tests a flag
>>>>>> that's set ...
>>>>> In top half, we are checking all stream irq mask and setting
>>>>> corresponding stream id index in interrupt status array when dma
>>>>> irq is raised.
>>>>>
>>>>> Our intention is to handle snd_pcm_period_elapsed in process context.
>>>>> if the flag is set, call the period elapsed for the substream based on stream
>>>>> id in work queue.

[PATCH V2 6/9] ASoC: amd: ps: add pm ops support for SoundWire dma driver

[Vijendar Mukunda]

Add support pm ops support for SoundWire dma driver.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 sound/soc/amd/ps/ps-sdw-dma.c | 91 ++++++++++++++++++++++++++++++++++-
 1 file changed, 89 insertions(+), 2 deletions(-)

diff --git a/sound/soc/amd/ps/ps-sdw-dma.c b/sound/soc/amd/ps/ps-sdw-dma.c
index be68003cbd67..54cf971589c0 100644
--- a/sound/soc/amd/ps/ps-sdw-dma.c
+++ b/sound/soc/amd/ps/ps-sdw-dma.c
@@ -12,6 +12,7 @@
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
 #include <sound/soc-dai.h>
+#include <linux/pm_runtime.h>
 #include <linux/soundwire/sdw_amd.h>
 #include "acp63.h"
 
@@ -102,6 +103,29 @@ static const struct snd_pcm_hardware acp63_sdw_hardware_capture = {
 	.periods_max = SDW_CAPTURE_MAX_NUM_PERIODS,
 };
 
+static void acp63_enable_disable_sdw_dma_interrupts(void __iomem *acp_base, bool enable)
+{
+	u32 ext_intr_cntl, ext_intr_cntl1;
+	u32 irq_mask = ACP_SDW_DMA_IRQ_MASK;
+	u32 irq_mask1 = ACP_P1_SDW_DMA_IRQ_MASK;
+
+	if (enable) {
+		ext_intr_cntl = readl(acp_base + ACP_EXTERNAL_INTR_CNTL);
+		ext_intr_cntl |= irq_mask;
+		writel(ext_intr_cntl, acp_base + ACP_EXTERNAL_INTR_CNTL);
+		ext_intr_cntl1 = readl(acp_base + ACP_EXTERNAL_INTR_CNTL1);
+		ext_intr_cntl1 |= irq_mask1;
+		writel(ext_intr_cntl1, acp_base + ACP_EXTERNAL_INTR_CNTL1);
+	} else {
+		ext_intr_cntl = readl(acp_base + ACP_EXTERNAL_INTR_CNTL);
+		ext_intr_cntl &= ~irq_mask;
+		writel(ext_intr_cntl, acp_base + ACP_EXTERNAL_INTR_CNTL);
+		ext_intr_cntl1 = readl(acp_base + ACP_EXTERNAL_INTR_CNTL1);
+		ext_intr_cntl1 &= ~irq_mask1;
+		writel(ext_intr_cntl1, acp_base + ACP_EXTERNAL_INTR_CNTL1);
+	}
+}
+
 static void acp63_config_dma(struct acp_sdw_dma_stream *stream, void __iomem *acp_base,
 			     u32 stream_id)
 {
@@ -464,16 +488,79 @@ static int acp63_sdw_platform_probe(struct platform_device *pdev)
 	status = devm_snd_soc_register_component(&pdev->dev,
 						 &acp63_sdw_component,
 						 NULL, 0);
-	if (status)
+	if (status) {
 		dev_err(&pdev->dev, "Fail to register sdw dma component\n");
+		return status;
+	}
+	pm_runtime_set_autosuspend_delay(&pdev->dev, ACP_SUSPEND_DELAY_MS);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_allow(&pdev->dev);
+	return 0;
+}
 
-	return status;
+static int acp63_sdw_platform_remove(struct platform_device *pdev)
+{
+	pm_runtime_disable(&pdev->dev);
+	return 0;
 }
 
+static int __maybe_unused acp63_sdw_pcm_resume(struct device *dev)
+{
+	struct sdw_dma_dev_data *sdw_data;
+	struct acp_sdw_dma_stream *stream;
+	struct snd_pcm_runtime *runtime;
+	u32 period_bytes, buf_size, water_mark_size_reg;
+	int ret;
+	int index;
+
+	sdw_data = dev_get_drvdata(dev);
+	for (index = 0; index < ACP63_SDW0_DMA_MAX_STREAMS; index++) {
+		if (sdw_data->sdw0_dma_stream[index] &&
+		    sdw_data->sdw0_dma_stream[index]->runtime) {
+			water_mark_size_reg = sdw0_dma_ring_buf_reg[index].water_mark_size_reg;
+			runtime = sdw_data->sdw0_dma_stream[index]->runtime;
+			stream = runtime->private_data;
+			period_bytes = frames_to_bytes(runtime, runtime->period_size);
+			buf_size = frames_to_bytes(runtime, runtime->buffer_size);
+			acp63_config_dma(stream, sdw_data->acp_base, index);
+			ret = acp63_configure_sdw_ringbuffer(sdw_data->acp_base, index,
+							     buf_size, ACP_SDW0);
+			if (ret)
+				return ret;
+			writel(period_bytes, sdw_data->acp_base + water_mark_size_reg);
+		}
+	}
+	for (index = 0; index < ACP63_SDW1_DMA_MAX_STREAMS; index++) {
+		if (sdw_data->sdw1_dma_stream[index] &&
+		    sdw_data->sdw1_dma_stream[index]->runtime) {
+			water_mark_size_reg = sdw1_dma_ring_buf_reg[index].water_mark_size_reg;
+			runtime = sdw_data->sdw1_dma_stream[index]->runtime;
+			stream = runtime->private_data;
+			period_bytes = frames_to_bytes(runtime, runtime->period_size);
+			buf_size = frames_to_bytes(runtime, runtime->buffer_size);
+			acp63_config_dma(stream, sdw_data->acp_base, index);
+			ret = acp63_configure_sdw_ringbuffer(sdw_data->acp_base, index,
+							     buf_size, ACP_SDW1);
+			if (ret)
+				return ret;
+			writel(period_bytes, sdw_data->acp_base + water_mark_size_reg);
+		}
+	}
+	acp63_enable_disable_sdw_dma_interrupts(sdw_data->acp_base, true);
+	return 0;
+}
+
+static const struct dev_pm_ops acp63_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(NULL, acp63_sdw_pcm_resume)
+};
+
 static struct platform_driver acp63_sdw_dma_driver = {
 	.probe = acp63_sdw_platform_probe,
+	.remove = acp63_sdw_platform_remove,
 	.driver = {
 		.name = "amd_ps_sdw_dma",
+		.pm = &acp63_pm_ops,
 	},
 };
 
-- 
2.34.1

Re: [PATCH V2 6/9] ASoC: amd: ps: add pm ops support for SoundWire dma driver

[Pierre-Louis Bossart]

> @@ -464,16 +488,79 @@ static int acp63_sdw_platform_probe(struct platform_device *pdev)
>  	status = devm_snd_soc_register_component(&pdev->dev,
>  						 &acp63_sdw_component,
>  						 NULL, 0);
> -	if (status)
> +	if (status) {
>  		dev_err(&pdev->dev, "Fail to register sdw dma component\n");
> +		return status;
> +	}
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, ACP_SUSPEND_DELAY_MS);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_allow(&pdev->dev);

Can you remind me why you need the pm_runtime_allow()? I can't recall
where the _forbid() is done.

Also is there not a pm_runtime_set_active() missing?

> +	return 0;
> +}
>  
> -	return status;
> +static int acp63_sdw_platform_remove(struct platform_device *pdev)
> +{
> +	pm_runtime_disable(&pdev->dev);
> +	return 0;
>  }
>  
> +static int __maybe_unused acp63_sdw_pcm_resume(struct device *dev)
> +{
> +	struct sdw_dma_dev_data *sdw_data;
> +	struct acp_sdw_dma_stream *stream;
> +	struct snd_pcm_runtime *runtime;
> +	u32 period_bytes, buf_size, water_mark_size_reg;
> +	int ret;
> +	int index;
> +
> +	sdw_data = dev_get_drvdata(dev);
> +	for (index = 0; index < ACP63_SDW0_DMA_MAX_STREAMS; index++) {
> +		if (sdw_data->sdw0_dma_stream[index] &&
> +		    sdw_data->sdw0_dma_stream[index]->runtime) {
> +			water_mark_size_reg = sdw0_dma_ring_buf_reg[index].water_mark_size_reg;
> +			runtime = sdw_data->sdw0_dma_stream[index]->runtime;
> +			stream = runtime->private_data;
> +			period_bytes = frames_to_bytes(runtime, runtime->period_size);
> +			buf_size = frames_to_bytes(runtime, runtime->buffer_size);
> +			acp63_config_dma(stream, sdw_data->acp_base, index);
> +			ret = acp63_configure_sdw_ringbuffer(sdw_data->acp_base, index,
> +							     buf_size, ACP_SDW0);
> +			if (ret)
> +				return ret;
> +			writel(period_bytes, sdw_data->acp_base + water_mark_size_reg);
> +		}
> +	}
> +	for (index = 0; index < ACP63_SDW1_DMA_MAX_STREAMS; index++) {
> +		if (sdw_data->sdw1_dma_stream[index] &&
> +		    sdw_data->sdw1_dma_stream[index]->runtime) {
> +			water_mark_size_reg = sdw1_dma_ring_buf_reg[index].water_mark_size_reg;
> +			runtime = sdw_data->sdw1_dma_stream[index]->runtime;
> +			stream = runtime->private_data;
> +			period_bytes = frames_to_bytes(runtime, runtime->period_size);
> +			buf_size = frames_to_bytes(runtime, runtime->buffer_size);
> +			acp63_config_dma(stream, sdw_data->acp_base, index);
> +			ret = acp63_configure_sdw_ringbuffer(sdw_data->acp_base, index,
> +							     buf_size, ACP_SDW1);
> +			if (ret)
> +				return ret;
> +			writel(period_bytes, sdw_data->acp_base + water_mark_size_reg);
> +		}
> +	}

Isn't this set of configurations something that needs to be done already
somewhere else, i.e. could there be a common helper?

> +	acp63_enable_disable_sdw_dma_interrupts(sdw_data->acp_base, true);
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops acp63_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(NULL, acp63_sdw_pcm_resume)
> +};
> +
>  static struct platform_driver acp63_sdw_dma_driver = {
>  	.probe = acp63_sdw_platform_probe,
> +	.remove = acp63_sdw_platform_remove,
>  	.driver = {
>  		.name = "amd_ps_sdw_dma",
> +		.pm = &acp63_pm_ops,
>  	},
>  };
>  

Re: [PATCH V2 6/9] ASoC: amd: ps: add pm ops support for SoundWire dma driver

[Mukunda,Vijendar]

On 22/05/23 23:50, Pierre-Louis Bossart wrote:
>> @@ -464,16 +488,79 @@ static int acp63_sdw_platform_probe(struct platform_device *pdev)
>>  	status = devm_snd_soc_register_component(&pdev->dev,
>>  						 &acp63_sdw_component,
>>  						 NULL, 0);
>> -	if (status)
>> +	if (status) {
>>  		dev_err(&pdev->dev, "Fail to register sdw dma component\n");
>> +		return status;
>> +	}
>> +	pm_runtime_set_autosuspend_delay(&pdev->dev, ACP_SUSPEND_DELAY_MS);
>> +	pm_runtime_use_autosuspend(&pdev->dev);
>> +	pm_runtime_enable(&pdev->dev);
>> +	pm_runtime_allow(&pdev->dev);
> Can you remind me why you need the pm_runtime_allow()? I can't recall
> where the _forbid() is done.
We have used pm_runtime_allow() to allow the device immediately
enter runtime suspend state. Yes you are correct. If we use pm_runtime_allow(),
then in remove sequence we should use pm_runtime_forbid call.
> Also is there not a pm_runtime_set_active() missing?


We will change the sequence as mentioned below.

in probe sequence , we will use

    pm_runtime_set_autosuspend_delay(&pdev->dev, ACP_SUSPEND_DELAY_MS);
    pm_runtime_use_autosuspend(&pdev->dev);
    pm_runtime_mark_last_busy(&pdev->dev);
    pm_runtime_set_active(&pdev->dev);
    pm_runtime_enable(&pdev->dev);

In remove sequence

pm_runtime_disable(&pdev->dev);
>
>> +	return 0;
>> +}
>>  
>> -	return status;
>> +static int acp63_sdw_platform_remove(struct platform_device *pdev)
>> +{
>> +	pm_runtime_disable(&pdev->dev);
>> +	return 0;
>>  }
>>  
>> +static int __maybe_unused acp63_sdw_pcm_resume(struct device *dev)
>> +{
>> +	struct sdw_dma_dev_data *sdw_data;
>> +	struct acp_sdw_dma_stream *stream;
>> +	struct snd_pcm_runtime *runtime;
>> +	u32 period_bytes, buf_size, water_mark_size_reg;
>> +	int ret;
>> +	int index;
>> +
>> +	sdw_data = dev_get_drvdata(dev);
>> +	for (index = 0; index < ACP63_SDW0_DMA_MAX_STREAMS; index++) {
>> +		if (sdw_data->sdw0_dma_stream[index] &&
>> +		    sdw_data->sdw0_dma_stream[index]->runtime) {
>> +			water_mark_size_reg = sdw0_dma_ring_buf_reg[index].water_mark_size_reg;
>> +			runtime = sdw_data->sdw0_dma_stream[index]->runtime;
>> +			stream = runtime->private_data;
>> +			period_bytes = frames_to_bytes(runtime, runtime->period_size);
>> +			buf_size = frames_to_bytes(runtime, runtime->buffer_size);
>> +			acp63_config_dma(stream, sdw_data->acp_base, index);
>> +			ret = acp63_configure_sdw_ringbuffer(sdw_data->acp_base, index,
>> +							     buf_size, ACP_SDW0);
>> +			if (ret)
>> +				return ret;
>> +			writel(period_bytes, sdw_data->acp_base + water_mark_size_reg);
>> +		}
>> +	}
>> +	for (index = 0; index < ACP63_SDW1_DMA_MAX_STREAMS; index++) {
>> +		if (sdw_data->sdw1_dma_stream[index] &&
>> +		    sdw_data->sdw1_dma_stream[index]->runtime) {
>> +			water_mark_size_reg = sdw1_dma_ring_buf_reg[index].water_mark_size_reg;
>> +			runtime = sdw_data->sdw1_dma_stream[index]->runtime;
>> +			stream = runtime->private_data;
>> +			period_bytes = frames_to_bytes(runtime, runtime->period_size);
>> +			buf_size = frames_to_bytes(runtime, runtime->buffer_size);
>> +			acp63_config_dma(stream, sdw_data->acp_base, index);
>> +			ret = acp63_configure_sdw_ringbuffer(sdw_data->acp_base, index,
>> +							     buf_size, ACP_SDW1);
>> +			if (ret)
>> +				return ret;
>> +			writel(period_bytes, sdw_data->acp_base + water_mark_size_reg);
>> +		}
>> +	}
> Isn't this set of configurations something that needs to be done already
> somewhere else, i.e. could there be a common helper?
In hw_params() callback, we are setting period_bytes and buf_size from
params structure. We are extracting same variables from runtime structures
in resume() callback.
We can implement a helper function to further simplify above logic
instead of having two separate loops.
>
>> +	acp63_enable_disable_sdw_dma_interrupts(sdw_data->acp_base, true);
>> +	return 0;
>> +}
>> +
>> +static const struct dev_pm_ops acp63_pm_ops = {
>> +	SET_SYSTEM_SLEEP_PM_OPS(NULL, acp63_sdw_pcm_resume)
>> +};
>> +
>>  static struct platform_driver acp63_sdw_dma_driver = {
>>  	.probe = acp63_sdw_platform_probe,
>> +	.remove = acp63_sdw_platform_remove,
>>  	.driver = {
>>  		.name = "amd_ps_sdw_dma",
>> +		.pm = &acp63_pm_ops,
>>  	},
>>  };
>>  

Re: [PATCH V2 6/9] ASoC: amd: ps: add pm ops support for SoundWire dma driver

[Pierre-Louis Bossart]

>>> @@ -464,16 +488,79 @@ static int acp63_sdw_platform_probe(struct platform_device *pdev)
>>>  	status = devm_snd_soc_register_component(&pdev->dev,
>>>  						 &acp63_sdw_component,
>>>  						 NULL, 0);
>>> -	if (status)
>>> +	if (status) {
>>>  		dev_err(&pdev->dev, "Fail to register sdw dma component\n");
>>> +		return status;
>>> +	}
>>> +	pm_runtime_set_autosuspend_delay(&pdev->dev, ACP_SUSPEND_DELAY_MS);
>>> +	pm_runtime_use_autosuspend(&pdev->dev);
>>> +	pm_runtime_enable(&pdev->dev);
>>> +	pm_runtime_allow(&pdev->dev);
>> Can you remind me why you need the pm_runtime_allow()? I can't recall
>> where the _forbid() is done.
> We have used pm_runtime_allow() to allow the device immediately
> enter runtime suspend state. Yes you are correct. If we use pm_runtime_allow(),
> then in remove sequence we should use pm_runtime_forbid call.
>> Also is there not a pm_runtime_set_active() missing?
> 
> 
> We will change the sequence as mentioned below.
> 
> in probe sequence , we will use
> 
>     pm_runtime_set_autosuspend_delay(&pdev->dev, ACP_SUSPEND_DELAY_MS);
>     pm_runtime_use_autosuspend(&pdev->dev);
>     pm_runtime_mark_last_busy(&pdev->dev);
>     pm_runtime_set_active(&pdev->dev);
>     pm_runtime_enable(&pdev->dev);
> 
> In remove sequence
> 
> pm_runtime_disable(&pdev->dev);

sounds about right.

>>
>>> +	return 0;
>>> +}
>>>  
>>> -	return status;
>>> +static int acp63_sdw_platform_remove(struct platform_device *pdev)
>>> +{
>>> +	pm_runtime_disable(&pdev->dev);
>>> +	return 0;
>>>  }
>>>  
>>> +static int __maybe_unused acp63_sdw_pcm_resume(struct device *dev)
>>> +{
>>> +	struct sdw_dma_dev_data *sdw_data;
>>> +	struct acp_sdw_dma_stream *stream;
>>> +	struct snd_pcm_runtime *runtime;
>>> +	u32 period_bytes, buf_size, water_mark_size_reg;
>>> +	int ret;
>>> +	int index;
>>> +
>>> +	sdw_data = dev_get_drvdata(dev);
>>> +	for (index = 0; index < ACP63_SDW0_DMA_MAX_STREAMS; index++) {
>>> +		if (sdw_data->sdw0_dma_stream[index] &&
>>> +		    sdw_data->sdw0_dma_stream[index]->runtime) {
>>> +			water_mark_size_reg = sdw0_dma_ring_buf_reg[index].water_mark_size_reg;
>>> +			runtime = sdw_data->sdw0_dma_stream[index]->runtime;
>>> +			stream = runtime->private_data;
>>> +			period_bytes = frames_to_bytes(runtime, runtime->period_size);
>>> +			buf_size = frames_to_bytes(runtime, runtime->buffer_size);
>>> +			acp63_config_dma(stream, sdw_data->acp_base, index);
>>> +			ret = acp63_configure_sdw_ringbuffer(sdw_data->acp_base, index,
>>> +							     buf_size, ACP_SDW0);
>>> +			if (ret)
>>> +				return ret;
>>> +			writel(period_bytes, sdw_data->acp_base + water_mark_size_reg);
>>> +		}
>>> +	}
>>> +	for (index = 0; index < ACP63_SDW1_DMA_MAX_STREAMS; index++) {
>>> +		if (sdw_data->sdw1_dma_stream[index] &&
>>> +		    sdw_data->sdw1_dma_stream[index]->runtime) {
>>> +			water_mark_size_reg = sdw1_dma_ring_buf_reg[index].water_mark_size_reg;
>>> +			runtime = sdw_data->sdw1_dma_stream[index]->runtime;
>>> +			stream = runtime->private_data;
>>> +			period_bytes = frames_to_bytes(runtime, runtime->period_size);
>>> +			buf_size = frames_to_bytes(runtime, runtime->buffer_size);
>>> +			acp63_config_dma(stream, sdw_data->acp_base, index);
>>> +			ret = acp63_configure_sdw_ringbuffer(sdw_data->acp_base, index,
>>> +							     buf_size, ACP_SDW1);
>>> +			if (ret)
>>> +				return ret;
>>> +			writel(period_bytes, sdw_data->acp_base + water_mark_size_reg);
>>> +		}
>>> +	}
>> Isn't this set of configurations something that needs to be done already
>> somewhere else, i.e. could there be a common helper?
> In hw_params() callback, we are setting period_bytes and buf_size from
> params structure. We are extracting same variables from runtime structures
> in resume() callback.
> We can implement a helper function to further simplify above logic
> instead of having two separate loops.

ok

[PATCH V2 7/9] ASoC: amd: ps: enable SoundWire dma driver build

[Vijendar Mukunda]

Enable SoundWire dma driver build for PS platform.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 sound/soc/amd/ps/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/amd/ps/Makefile b/sound/soc/amd/ps/Makefile
index 383973a12f6a..f2a5eaf2fa4d 100644
--- a/sound/soc/amd/ps/Makefile
+++ b/sound/soc/amd/ps/Makefile
@@ -3,7 +3,9 @@
 snd-pci-ps-objs := pci-ps.o
 snd-ps-pdm-dma-objs := ps-pdm-dma.o
 snd-soc-ps-mach-objs := ps-mach.o
+snd-ps-sdw-dma-objs := ps-sdw-dma.o
 
 obj-$(CONFIG_SND_SOC_AMD_PS) += snd-pci-ps.o
 obj-$(CONFIG_SND_SOC_AMD_PS) += snd-ps-pdm-dma.o
+obj-$(CONFIG_SND_SOC_AMD_PS) += snd-ps-sdw-dma.o
 obj-$(CONFIG_SND_SOC_AMD_PS_MACH)   += snd-soc-ps-mach.o
-- 
2.34.1

[PATCH V2 8/9] ASoC: amd: update comments in Kconfig file

[Vijendar Mukunda]

Update comments in Kconfig file for Pink Sardine platform.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 sound/soc/amd/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/amd/Kconfig b/sound/soc/amd/Kconfig
index 08e42082f5e9..2f0d444b21fa 100644
--- a/sound/soc/amd/Kconfig
+++ b/sound/soc/amd/Kconfig
@@ -136,7 +136,8 @@ config SND_SOC_AMD_PS
         help
           This option enables Audio Coprocessor i.e ACP v6.3 support on
           AMD Pink sardine platform. By enabling this flag build will be
-          triggered for ACP PCI driver, ACP PDM DMA driver.
+          triggered for ACP PCI driver, ACP PDM DMA driver, ACP SoundWire
+          DMA driver.
           Say m if you have such a device.
           If unsure select "N".
 
-- 
2.34.1

[PATCH V2 9/9] ASoC: amd: ps: Add SoundWire specific checks in pci driver in pm ops.

[Vijendar Mukunda]

AMD SoundWire manager supports different power modes.
In case of SoundWire Power off Mode, ACP pci parent driver
should invoke acp de-init and init sequence during suspend/resume
callbacks.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 sound/soc/amd/ps/pci-ps.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
index c99ac5d90097..f6d14baf2c2e 100644
--- a/sound/soc/amd/ps/pci-ps.c
+++ b/sound/soc/amd/ps/pci-ps.c
@@ -659,10 +659,15 @@ static int snd_acp63_probe(struct pci_dev *pci,
 static int __maybe_unused snd_acp63_suspend(struct device *dev)
 {
 	struct acp63_dev_data *adata;
-	int ret;
+	int ret = 0;
 
 	adata = dev_get_drvdata(dev);
-	ret = acp63_deinit(adata->acp63_base, dev);
+	if (adata->pdev_mask & ACP63_SDW_DEV_MASK) {
+		if (adata->acp_reset)
+			ret = acp63_deinit(adata->acp63_base, dev);
+	} else {
+		ret = acp63_deinit(adata->acp63_base, dev);
+	}
 	if (ret)
 		dev_err(dev, "ACP de-init failed\n");
 	return ret;
@@ -671,10 +676,15 @@ static int __maybe_unused snd_acp63_suspend(struct device *dev)
 static int __maybe_unused snd_acp63_resume(struct device *dev)
 {
 	struct acp63_dev_data *adata;
-	int ret;
+	int ret = 0;
 
 	adata = dev_get_drvdata(dev);
-	ret = acp63_init(adata->acp63_base, dev);
+	if (adata->pdev_mask & ACP63_SDW_DEV_MASK) {
+		if (adata->acp_reset)
+			ret = acp63_init(adata->acp63_base, dev);
+	} else {
+		ret = acp63_init(adata->acp63_base, dev);
+	}
 	if (ret)
 		dev_err(dev, "ACP init failed\n");
 	return ret;
-- 
2.34.1