Create platform devices for sdw controllers and PDM controller
based on ACP pin config selection and ACPI fw handle for
pink sardine platform.
Signed-off-by: Vijendar Mukunda <[email protected]>
Signed-off-by: Mastan Katragadda <[email protected]>
---
include/linux/soundwire/sdw_amd.h | 18 +++
sound/soc/amd/ps/acp63.h | 24 ++-
sound/soc/amd/ps/pci-ps.c | 248 ++++++++++++++++++++++++++++--
3 files changed, 277 insertions(+), 13 deletions(-)
create mode 100644 include/linux/soundwire/sdw_amd.h
diff --git a/include/linux/soundwire/sdw_amd.h b/include/linux/soundwire/sdw_amd.h
new file mode 100644
index 000000000000..f0123815af46
--- /dev/null
+++ b/include/linux/soundwire/sdw_amd.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.
+ */
+
+#ifndef __SDW_AMD_H
+#define __SDW_AMD_H
+
+#include <linux/soundwire/sdw.h>
+
+#define AMD_SDW_CLK_STOP_MODE 1
+#define AMD_SDW_POWER_OFF_MODE 2
+
+struct acp_sdw_pdata {
+ u16 instance;
+ struct mutex *sdw_lock;
+};
+#endif
diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
index b7535c7d093f..ed979e6d0c1d 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
@@ -55,8 +55,14 @@
#define ACP63_DMIC_ADDR 2
#define ACP63_PDM_MODE_DEVS 3
-#define ACP63_PDM_DEV_MASK 1
#define ACP_DMIC_DEV 2
+#define ACP63_SDW0_MODE_DEVS 2
+#define ACP63_SDW0_SDW1_MODE_DEVS 3
+#define ACP63_SDW0_PDM_MODE_DEVS 4
+#define ACP63_SDW0_SDW1_PDM_MODE_DEVS 5
+#define ACP63_DMIC_ADDR 2
+#define ACP63_SDW_ADDR 5
+#define AMD_SDW_MAX_CONTROLLERS 2
enum acp_config {
ACP_CONFIG_0 = 0,
@@ -77,6 +83,12 @@ enum acp_config {
ACP_CONFIG_15,
};
+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;
@@ -107,7 +119,15 @@ struct acp63_dev_data {
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_master_count;
+ u16 sdw0_dev_index;
+ u16 sdw1_dev_index;
+ u16 sdw_dma_dev_index;
+ bool is_dmic_dev;
+ bool is_sdw_dev;
+ bool acp_sdw_power_off;
};
diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
index e86f23d97584..85154cf0b2a2 100644
--- a/sound/soc/amd/ps/pci-ps.c
+++ b/sound/soc/amd/ps/pci-ps.c
@@ -14,6 +14,7 @@
#include <linux/interrupt.h>
#include <sound/pcm_params.h>
#include <linux/pm_runtime.h>
+#include <linux/soundwire/sdw_amd.h>
#include "acp63.h"
@@ -134,12 +135,68 @@ 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];
+ u8 count = 0;
+ u32 acp_sdw_power_mode = 0;
+ int index;
+ int ret;
+
+ acp_data = dev_get_drvdata(dev);
+ acp_data->acp_sdw_power_off = true;
+ /* Found controller, find links supported */
+ ret = fwnode_property_read_u8_array((acp_data->sdw_fw_node),
+ "mipi-sdw-master-count", &count, 1);
+
+ if (ret) {
+ dev_err(dev,
+ "Failed to read mipi-sdw-master-count: %d\n", ret);
+ return -EINVAL;
+ }
+
+ /* Check count is within bounds */
+ if (count > AMD_SDW_MAX_CONTROLLERS) {
+ dev_err(dev, "Controller count %d exceeds max %d\n",
+ count, AMD_SDW_MAX_CONTROLLERS);
+ return -EINVAL;
+ }
+
+ if (!count) {
+ dev_warn(dev, "No SoundWire controllers detected\n");
+ return -EINVAL;
+ }
+ dev_dbg(dev, "ACPI reports %d Soundwire Controller devices\n", count);
+ acp_data->sdw_master_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, "Master node %s not found\n", name);
+ return -EIO;
+ }
+
+ fwnode_property_read_u32(link, "amd-sdw-power-mode",
+ &acp_sdw_power_mode);
+ if (acp_sdw_power_mode != AMD_SDW_POWER_OFF_MODE)
+ acp_data->acp_sdw_power_off = 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;
+ struct device *dev;
const union acpi_object *obj;
bool is_dmic_dev = false;
+ bool is_sdw_dev = false;
+ int ret;
+
+ dev = &pci->dev;
dmic_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_DMIC_ADDR, 0);
if (dmic_dev) {
@@ -149,22 +206,84 @@ static void get_acp63_device_config(u32 config, struct pci_dev *pci,
is_dmic_dev = true;
}
+ sdw_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_SDW_ADDR, 0);
+ if (sdw_dev) {
+ is_sdw_dev = true;
+ acp_data->sdw_fw_node = acpi_fwnode_handle(sdw_dev);
+ ret = sdw_amd_scan_controller(dev);
+ if (ret)
+ return ret;
+ }
+
+ 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_master_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_master_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_master_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,
@@ -188,6 +307,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;
@@ -220,8 +340,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->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_controller", 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].sdw_lock = &adata->acp_lock;
+ sdw_pdata[1].sdw_lock = &adata->acp_lock;
+ sdw_pdata->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_controller", 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_controller", 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->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_controller", 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].sdw_lock = &adata->acp_lock;
+ sdw_pdata[1].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_controller", 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_controller", 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 controller devices found\n");
return 0;
}
@@ -299,7 +521,11 @@ static int snd_acp63_probe(struct pci_dev *pci,
goto de_init;
}
val = acp63_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
On 1/11/2023 10:02 AM, Vijendar Mukunda wrote:
> Create platform devices for sdw controllers and PDM controller
> based on ACP pin config selection and ACPI fw handle for
> pink sardine platform.
>
> Signed-off-by: Vijendar Mukunda <[email protected]>
> Signed-off-by: Mastan Katragadda <[email protected]>
> ---
> include/linux/soundwire/sdw_amd.h | 18 +++
> sound/soc/amd/ps/acp63.h | 24 ++-
> sound/soc/amd/ps/pci-ps.c | 248 ++++++++++++++++++++++++++++--
> 3 files changed, 277 insertions(+), 13 deletions(-)
> create mode 100644 include/linux/soundwire/sdw_amd.h
>
...
> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
> index e86f23d97584..85154cf0b2a2 100644
> --- a/sound/soc/amd/ps/pci-ps.c
> +++ b/sound/soc/amd/ps/pci-ps.c
> @@ -14,6 +14,7 @@
> #include <linux/interrupt.h>
> #include <sound/pcm_params.h>
> #include <linux/pm_runtime.h>
> +#include <linux/soundwire/sdw_amd.h>
>
> #include "acp63.h"
>
> @@ -134,12 +135,68 @@ 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];
> + u8 count = 0;
> + u32 acp_sdw_power_mode = 0;
> + int index;
> + int ret;
> +
> + acp_data = dev_get_drvdata(dev);
> + acp_data->acp_sdw_power_off = true;
> + /* Found controller, find links supported */
> + ret = fwnode_property_read_u8_array((acp_data->sdw_fw_node),
> + "mipi-sdw-master-count", &count, 1);
> +
> + if (ret) {
> + dev_err(dev,
> + "Failed to read mipi-sdw-master-count: %d\n", ret);
> + return -EINVAL;
> + }
> +
> + /* Check count is within bounds */
> + if (count > AMD_SDW_MAX_CONTROLLERS) {
> + dev_err(dev, "Controller count %d exceeds max %d\n",
> + count, AMD_SDW_MAX_CONTROLLERS);
> + return -EINVAL;
> + }
> +
> + if (!count) {
> + dev_warn(dev, "No SoundWire controllers detected\n");
> + return -EINVAL;
> + }
> + dev_dbg(dev, "ACPI reports %d Soundwire Controller devices\n", count);
> + acp_data->sdw_master_count = count;
Double space before '='.
> + 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, "Master node %s not found\n", name);
> + return -EIO;
> + }
> +
> + fwnode_property_read_u32(link, "amd-sdw-power-mode",
> + &acp_sdw_power_mode);
> + if (acp_sdw_power_mode != AMD_SDW_POWER_OFF_MODE)
> + acp_data->acp_sdw_power_off = 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;
> + struct device *dev;
> const union acpi_object *obj;
> bool is_dmic_dev = false;
> + bool is_sdw_dev = false;
> + int ret;
> +
> + dev = &pci->dev;
>
> dmic_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_DMIC_ADDR, 0);
If you set dev above, you might as well use it throughout the function
context? Like above in ACPI_COMPANION?
> if (dmic_dev) {
> @@ -149,22 +206,84 @@ static void get_acp63_device_config(u32 config, struct pci_dev *pci,
> is_dmic_dev = true;
> }
>
> + sdw_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_SDW_ADDR, 0);
> + if (sdw_dev) {
> + is_sdw_dev = true;
> + acp_data->sdw_fw_node = acpi_fwnode_handle(sdw_dev);
> + ret = sdw_amd_scan_controller(dev);
Or just use &pci->dev here, so there is no need for separate variable?
On 11/01/23 18:57, Amadeusz Sławiński wrote:
> On 1/11/2023 10:02 AM, Vijendar Mukunda wrote:
>> Create platform devices for sdw controllers and PDM controller
>> based on ACP pin config selection and ACPI fw handle for
>> pink sardine platform.
>>
>> Signed-off-by: Vijendar Mukunda <[email protected]>
>> Signed-off-by: Mastan Katragadda <[email protected]>
>> ---
>> include/linux/soundwire/sdw_amd.h | 18 +++
>> sound/soc/amd/ps/acp63.h | 24 ++-
>> sound/soc/amd/ps/pci-ps.c | 248 ++++++++++++++++++++++++++++--
>> 3 files changed, 277 insertions(+), 13 deletions(-)
>> create mode 100644 include/linux/soundwire/sdw_amd.h
>>
>
> ...
>
>> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
>> index e86f23d97584..85154cf0b2a2 100644
>> --- a/sound/soc/amd/ps/pci-ps.c
>> +++ b/sound/soc/amd/ps/pci-ps.c
>> @@ -14,6 +14,7 @@
>> #include <linux/interrupt.h>
>> #include <sound/pcm_params.h>
>> #include <linux/pm_runtime.h>
>> +#include <linux/soundwire/sdw_amd.h>
>> #include "acp63.h"
>> @@ -134,12 +135,68 @@ 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];
>> + u8 count = 0;
>> + u32 acp_sdw_power_mode = 0;
>> + int index;
>> + int ret;
>> +
>> + acp_data = dev_get_drvdata(dev);
>> + acp_data->acp_sdw_power_off = true;
>> + /* Found controller, find links supported */
>> + ret = fwnode_property_read_u8_array((acp_data->sdw_fw_node),
>> + "mipi-sdw-master-count", &count, 1);
>> +
>> + if (ret) {
>> + dev_err(dev,
>> + "Failed to read mipi-sdw-master-count: %d\n", ret);
>> + return -EINVAL;
>> + }
>> +
>> + /* Check count is within bounds */
>> + if (count > AMD_SDW_MAX_CONTROLLERS) {
>> + dev_err(dev, "Controller count %d exceeds max %d\n",
>> + count, AMD_SDW_MAX_CONTROLLERS);
>> + return -EINVAL;
>> + }
>> +
>> + if (!count) {
>> + dev_warn(dev, "No SoundWire controllers detected\n");
>> + return -EINVAL;
>> + }
>> + dev_dbg(dev, "ACPI reports %d Soundwire Controller devices\n", count);
>> + acp_data->sdw_master_count = count;
>
> Double space before '='.
> will fix it.
>> + 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, "Master node %s not found\n", name);
>> + return -EIO;
>> + }
>> +
>> + fwnode_property_read_u32(link, "amd-sdw-power-mode",
>> + &acp_sdw_power_mode);
>> + if (acp_sdw_power_mode != AMD_SDW_POWER_OFF_MODE)
>> + acp_data->acp_sdw_power_off = 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;
>> + struct device *dev;
>> const union acpi_object *obj;
>> bool is_dmic_dev = false;
>> + bool is_sdw_dev = false;
>> + int ret;
>> +
>> + dev = &pci->dev;
>> dmic_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_DMIC_ADDR, 0);
>
> If you set dev above, you might as well use it throughout the function context? Like above in ACPI_COMPANION?
> will use pci->dev throughtout the function context.
>> if (dmic_dev) {
>> @@ -149,22 +206,84 @@ static void get_acp63_device_config(u32 config, struct pci_dev *pci,
>> is_dmic_dev = true;
>> }
>> + sdw_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_SDW_ADDR, 0);
>> + if (sdw_dev) {
>> + is_sdw_dev = true;
>> + acp_data->sdw_fw_node = acpi_fwnode_handle(sdw_dev);
>> + ret = sdw_amd_scan_controller(dev);
>
> Or just use &pci->dev here, so there is no need for separate variable?
> will remove the "dev" local variable.
>
>
> +#define AMD_SDW_CLK_STOP_MODE 1
there are multiple modes for clock stop in SoundWire, and multiple ways
for the link manager to deal with clock stop, you want a comment to
describe what this define refers to.
> +#define AMD_SDW_POWER_OFF_MODE 2
> +
> +struct acp_sdw_pdata {
> + u16 instance;
> + struct mutex *sdw_lock;
need a comment on what this lock protects.
> +};
> +#endif
> diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
> index b7535c7d093f..ed979e6d0c1d 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
> @@ -55,8 +55,14 @@
>
> #define ACP63_DMIC_ADDR 2
> #define ACP63_PDM_MODE_DEVS 3
> -#define ACP63_PDM_DEV_MASK 1
> #define ACP_DMIC_DEV 2
> +#define ACP63_SDW0_MODE_DEVS 2
> +#define ACP63_SDW0_SDW1_MODE_DEVS 3
> +#define ACP63_SDW0_PDM_MODE_DEVS 4
> +#define ACP63_SDW0_SDW1_PDM_MODE_DEVS 5
> +#define ACP63_DMIC_ADDR 2
> +#define ACP63_SDW_ADDR 5
> +#define AMD_SDW_MAX_CONTROLLERS 2
>
> enum acp_config {
> ACP_CONFIG_0 = 0,
> @@ -77,6 +83,12 @@ enum acp_config {
> ACP_CONFIG_15,
> };
>
> +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;
> @@ -107,7 +119,15 @@ struct acp63_dev_data {
> 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_master_count;
for new contributions, it's recommended to use manager and peripheral.
> + u16 sdw0_dev_index;
> + u16 sdw1_dev_index;
probably need a comment on what the 0 and 1 refer to, it's not clear if
there's any sort of dependency/link with the 'sdw_master_count' above.
If this is related to the two controllers mentioned in the cover letter,
then an explanation of the sdw_master_count would be needed as well
(single variable for two controllers?)
> + u16 sdw_dma_dev_index;
> + bool is_dmic_dev;
> + bool is_sdw_dev;
> + bool acp_sdw_power_off;
> };
> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
> index e86f23d97584..85154cf0b2a2 100644
> --- a/sound/soc/amd/ps/pci-ps.c
> +++ b/sound/soc/amd/ps/pci-ps.c
> @@ -14,6 +14,7 @@
> #include <linux/interrupt.h>
> #include <sound/pcm_params.h>
> #include <linux/pm_runtime.h>
> +#include <linux/soundwire/sdw_amd.h>
>
> #include "acp63.h"
>
> @@ -134,12 +135,68 @@ 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];
> + u8 count = 0;
> + u32 acp_sdw_power_mode = 0;
> + int index;
> + int ret;
> +
> + acp_data = dev_get_drvdata(dev);
> + acp_data->acp_sdw_power_off = true;
> + /* Found controller, find links supported */
> + ret = fwnode_property_read_u8_array((acp_data->sdw_fw_node),
> + "mipi-sdw-master-count", &count, 1);
> +
> + if (ret) {
> + dev_err(dev,
> + "Failed to read mipi-sdw-master-count: %d\n", ret);
one line?
> + return -EINVAL;
> + }
> +
> + /* Check count is within bounds */
> + if (count > AMD_SDW_MAX_CONTROLLERS) {
> + dev_err(dev, "Controller count %d exceeds max %d\n",
> + count, AMD_SDW_MAX_CONTROLLERS);
No. controllers and masters are different concepts, see the DisCo
specification for SoundWire. A Controller can have multiple Masters.
> + return -EINVAL;
> + }
> +
> + if (!count) {
> + dev_warn(dev, "No SoundWire controllers detected\n");
> + return -EINVAL;
> + }
is this really a warning, looks like a dev_dbg or info to me.
> + dev_dbg(dev, "ACPI reports %d Soundwire Controller devices\n", count);
the term device is incorrect here, the DisCo spec does not expose ACPI
devices for each master.
"ACPI reports %d Managers"
> + acp_data->sdw_master_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, "Master node %s not found\n", name);
> + return -EIO;
> + }
> +
> + fwnode_property_read_u32(link, "amd-sdw-power-mode",
> + &acp_sdw_power_mode);
> + if (acp_sdw_power_mode != AMD_SDW_POWER_OFF_MODE)
> + acp_data->acp_sdw_power_off = false;
does power-off mean 'clock-stop'?
> + }
> + return 0;
> +}
> +
> + if (is_dmic_dev && is_sdw_dev) {
> + switch (acp_data->sdw_master_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;
so the cover letter is indeed wrong and confuses two controllers for two
managers.
> + 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_master_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;
> }
On 11/01/23 19:02, Pierre-Louis Bossart wrote:
>
>
>> +#define AMD_SDW_CLK_STOP_MODE 1
> there are multiple modes for clock stop in SoundWire, and multiple ways
> for the link manager to deal with clock stop, you want a comment to
> describe what this define refers to.
will add comments about flags explanation.
>> +#define AMD_SDW_POWER_OFF_MODE 2
>> +
>> +struct acp_sdw_pdata {
>> + u16 instance;
>> + struct mutex *sdw_lock;
> need a comment on what this lock protects.
>
>> +};
>> +#endif
>> diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
>> index b7535c7d093f..ed979e6d0c1d 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
>> @@ -55,8 +55,14 @@
>>
>> #define ACP63_DMIC_ADDR 2
>> #define ACP63_PDM_MODE_DEVS 3
>> -#define ACP63_PDM_DEV_MASK 1
>> #define ACP_DMIC_DEV 2
>> +#define ACP63_SDW0_MODE_DEVS 2
>> +#define ACP63_SDW0_SDW1_MODE_DEVS 3
>> +#define ACP63_SDW0_PDM_MODE_DEVS 4
>> +#define ACP63_SDW0_SDW1_PDM_MODE_DEVS 5
>> +#define ACP63_DMIC_ADDR 2
>> +#define ACP63_SDW_ADDR 5
>> +#define AMD_SDW_MAX_CONTROLLERS 2
>>
>> enum acp_config {
>> ACP_CONFIG_0 = 0,
>> @@ -77,6 +83,12 @@ enum acp_config {
>> ACP_CONFIG_15,
>> };
>>
>> +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;
>> @@ -107,7 +119,15 @@ struct acp63_dev_data {
>> 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_master_count;
> for new contributions, it's recommended to use manager and peripheral.
will use manager and peripheral terminology.
>> + u16 sdw0_dev_index;
>> + u16 sdw1_dev_index;
> probably need a comment on what the 0 and 1 refer to, it's not clear if
> there's any sort of dependency/link with the 'sdw_master_count' above.
>
> If this is related to the two controllers mentioned in the cover letter,
> then an explanation of the sdw_master_count would be needed as well
> (single variable for two controllers?)
will add comments for dev_index variables.
>> + u16 sdw_dma_dev_index;
>> + bool is_dmic_dev;
>> + bool is_sdw_dev;
>> + bool acp_sdw_power_off;
>> };
>> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
>> index e86f23d97584..85154cf0b2a2 100644
>> --- a/sound/soc/amd/ps/pci-ps.c
>> +++ b/sound/soc/amd/ps/pci-ps.c
>> @@ -14,6 +14,7 @@
>> #include <linux/interrupt.h>
>> #include <sound/pcm_params.h>
>> #include <linux/pm_runtime.h>
>> +#include <linux/soundwire/sdw_amd.h>
>>
>> #include "acp63.h"
>>
>> @@ -134,12 +135,68 @@ 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];
>> + u8 count = 0;
>> + u32 acp_sdw_power_mode = 0;
>> + int index;
>> + int ret;
>> +
>> + acp_data = dev_get_drvdata(dev);
>> + acp_data->acp_sdw_power_off = true;
>> + /* Found controller, find links supported */
>> + ret = fwnode_property_read_u8_array((acp_data->sdw_fw_node),
>> + "mipi-sdw-master-count", &count, 1);
>> +
>> + if (ret) {
>> + dev_err(dev,
>> + "Failed to read mipi-sdw-master-count: %d\n", ret);
> one line?
will fix it.
>
>> + return -EINVAL;
>> + }
>> +
>> + /* Check count is within bounds */
>> + if (count > AMD_SDW_MAX_CONTROLLERS) {
>> + dev_err(dev, "Controller count %d exceeds max %d\n",
>> + count, AMD_SDW_MAX_CONTROLLERS);
> No. controllers and masters are different concepts, see the DisCo
> specification for SoundWire. A Controller can have multiple Masters.
Will correct it.
>
>> + return -EINVAL;
>> + }
>> +
>> + if (!count) {
>> + dev_warn(dev, "No SoundWire controllers detected\n");
>> + return -EINVAL;
>> + }
> is this really a warning, looks like a dev_dbg or info to me.
>
>> + dev_dbg(dev, "ACPI reports %d Soundwire Controller devices\n", count);
> the term device is incorrect here, the DisCo spec does not expose ACPI
> devices for each master.
>
> "ACPI reports %d Managers"
will correct it.
>> + acp_data->sdw_master_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, "Master node %s not found\n", name);
>> + return -EIO;
>> + }
>> +
>> + fwnode_property_read_u32(link, "amd-sdw-power-mode",
>> + &acp_sdw_power_mode);
>> + if (acp_sdw_power_mode != AMD_SDW_POWER_OFF_MODE)
>> + acp_data->acp_sdw_power_off = false;
> does power-off mean 'clock-stop'?
> No. We will add comment for acp_sdw_power_off flag.
>> + }
>> + return 0;
>> +}
>> +
>> + if (is_dmic_dev && is_sdw_dev) {
>> + switch (acp_data->sdw_master_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;
> so the cover letter is indeed wrong and confuses two controllers for two
> managers.
ACP IP has two independent manager instances driven by separate controller
each which are connected in different power domains.
we should create two separate ACPI companion devices for separate
manager instance. Currently we have limitations with BIOS.
we are going with single ACPI companion device.
We will update the changes later.
>
>> + 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_master_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;
>> }
>>> + if (is_dmic_dev && is_sdw_dev) {
>>> + switch (acp_data->sdw_master_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;
>> so the cover letter is indeed wrong and confuses two controllers for two
>> managers.
> ACP IP has two independent manager instances driven by separate controller
> each which are connected in different power domains.
>
> we should create two separate ACPI companion devices for separate
> manager instance. Currently we have limitations with BIOS.
> we are going with single ACPI companion device.
> We will update the changes later.
Humm, this is tricky. The BIOS interface isn't something that can be
changed at will on the kernel side, you'd have to maintain two solutions
with a means to detect which one to use.
Or is this is a temporary issue on development devices, then that part
should probably not be upstreamed.
On 13/01/23 22:41, Pierre-Louis Bossart wrote:
>>>> + if (is_dmic_dev && is_sdw_dev) {
>>>> + switch (acp_data->sdw_master_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;
>>> so the cover letter is indeed wrong and confuses two controllers for two
>>> managers.
>> ACP IP has two independent manager instances driven by separate controller
>> each which are connected in different power domains.
>>
>> we should create two separate ACPI companion devices for separate
>> manager instance. Currently we have limitations with BIOS.
>> we are going with single ACPI companion device.
>> We will update the changes later.
> Humm, this is tricky. The BIOS interface isn't something that can be
> changed at will on the kernel side, you'd have to maintain two solutions
> with a means to detect which one to use.
>
> Or is this is a temporary issue on development devices, then that part
> should probably not be upstreamed.
It's a temporary issue on development devices.
We had discussion with Windows dev team and BIOS team.
They have agreed to modify ACPI companion device logic.
We will update the two companion devices logic for two manager
instances in V2 version.
On 16/01/23 13:32, Mukunda,Vijendar wrote:
> On 13/01/23 22:41, Pierre-Louis Bossart wrote:
>>>>> + if (is_dmic_dev && is_sdw_dev) {
>>>>> + switch (acp_data->sdw_master_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;
>>>> so the cover letter is indeed wrong and confuses two controllers for two
>>>> managers.
>>> ACP IP has two independent manager instances driven by separate controller
>>> each which are connected in different power domains.
>>>
>>> we should create two separate ACPI companion devices for separate
>>> manager instance. Currently we have limitations with BIOS.
>>> we are going with single ACPI companion device.
>>> We will update the changes later.
>> Humm, this is tricky. The BIOS interface isn't something that can be
>> changed at will on the kernel side, you'd have to maintain two solutions
>> with a means to detect which one to use.
>>
>> Or is this is a temporary issue on development devices, then that part
>> should probably not be upstreamed.
> It's a temporary issue on development devices.
> We had discussion with Windows dev team and BIOS team.
> They have agreed to modify ACPI companion device logic.
> We will update the two companion devices logic for two manager
> instances in V2 version.
After experimenting, two ACPI companion devices approach,
we got an update from Windows team, there is a limitation
on windows stack. For current platform, we can't proceed
with two ACPI companion devices.
Even on Linux side, if we create two ACPI companion devices
followed by creating a single soundwire manager instance per
Soundwire controller, we have observed an issue in a scenario,
where similar codec parts(UID are also same) are connected on
both soundwire manager instances.
As per MIPI Disco spec, for single link controllers Link ID should
be set to zero.
If we use Link ID as zero, for the soundwire manager which is on
the second soundwire controller ACPI device scope, then soundwire
framework is not allowing to create peripheral device node as its
duplicate one.
If we want to support two ACPI companion device approach
on our future platforms, how to proceed?
>
>
On 1/31/23 07:09, Mukunda,Vijendar wrote:
> On 16/01/23 13:32, Mukunda,Vijendar wrote:
>> On 13/01/23 22:41, Pierre-Louis Bossart wrote:
>>>>>> + if (is_dmic_dev && is_sdw_dev) {
>>>>>> + switch (acp_data->sdw_master_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;
>>>>> so the cover letter is indeed wrong and confuses two controllers for two
>>>>> managers.
>>>> ACP IP has two independent manager instances driven by separate controller
>>>> each which are connected in different power domains.
>>>>
>>>> we should create two separate ACPI companion devices for separate
>>>> manager instance. Currently we have limitations with BIOS.
>>>> we are going with single ACPI companion device.
>>>> We will update the changes later.
>>> Humm, this is tricky. The BIOS interface isn't something that can be
>>> changed at will on the kernel side, you'd have to maintain two solutions
>>> with a means to detect which one to use.
>>>
>>> Or is this is a temporary issue on development devices, then that part
>>> should probably not be upstreamed.
>> It's a temporary issue on development devices.
>> We had discussion with Windows dev team and BIOS team.
>> They have agreed to modify ACPI companion device logic.
>> We will update the two companion devices logic for two manager
>> instances in V2 version.
> After experimenting, two ACPI companion devices approach,
> we got an update from Windows team, there is a limitation
> on windows stack. For current platform, we can't proceed
> with two ACPI companion devices.
>
> Even on Linux side, if we create two ACPI companion devices
> followed by creating a single soundwire manager instance per
> Soundwire controller, we have observed an issue in a scenario,
> where similar codec parts(UID are also same) are connected on
> both soundwire manager instances.
If I'm not mistaken, the specific failure in the Linux stack is because
of the duplicated sysfs files since the same UID is used on both manager
instances, right?
At least with how the kernel handles it today I don't see how this
should be handled. You can't disambiguate between the two different
ACPI devices when they would be identical unless you had another property.
>
> As per MIPI Disco spec, for single link controllers Link ID should
> be set to zero.
> If we use Link ID as zero, for the soundwire manager which is on
> the second soundwire controller ACPI device scope, then soundwire
> framework is not allowing to create peripheral device node as its
> duplicate one.
>
> If we want to support two ACPI companion device approach
> on our future platforms, how to proceed?
From my understanding I would think this should be an exception for ps
platform, but this should be discussed for a future spec revision.
Maybe in a future spec revision another property can be utilized in
conjunction with ACPI device to disambiguate this case.
>>
>>
>
On 1/31/23 07:09, Mukunda,Vijendar wrote:
> On 16/01/23 13:32, Mukunda,Vijendar wrote:
>> On 13/01/23 22:41, Pierre-Louis Bossart wrote:
>>>>>> + if (is_dmic_dev && is_sdw_dev) {
>>>>>> + switch (acp_data->sdw_master_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;
>>>>> so the cover letter is indeed wrong and confuses two controllers for two
>>>>> managers.
>>>> ACP IP has two independent manager instances driven by separate controller
>>>> each which are connected in different power domains.
>>>>
>>>> we should create two separate ACPI companion devices for separate
>>>> manager instance. Currently we have limitations with BIOS.
>>>> we are going with single ACPI companion device.
>>>> We will update the changes later.
>>> Humm, this is tricky. The BIOS interface isn't something that can be
>>> changed at will on the kernel side, you'd have to maintain two solutions
>>> with a means to detect which one to use.
>>>
>>> Or is this is a temporary issue on development devices, then that part
>>> should probably not be upstreamed.
>> It's a temporary issue on development devices.
>> We had discussion with Windows dev team and BIOS team.
>> They have agreed to modify ACPI companion device logic.
>> We will update the two companion devices logic for two manager
>> instances in V2 version.
> After experimenting, two ACPI companion devices approach,
> we got an update from Windows team, there is a limitation
> on windows stack. For current platform, we can't proceed
> with two ACPI companion devices.
so how would the two controllers be declared then in the DSDT used by
Windows? There's a contradiction between having a single companion
device and the ability to set the 'manager-number' to one.
You probably want to give an example of what you have, otherwise we
probably will talk past each other.
>
> Even on Linux side, if we create two ACPI companion devices
> followed by creating a single soundwire manager instance per
> Soundwire controller, we have observed an issue in a scenario,
> where similar codec parts(UID are also same) are connected on
> both soundwire manager instances.
We've been handling this case of two identical amplifiers on two
different links for the last 3 years. I don't see how this could be a
problem, the codecs are declared in the scope of the companion device
and the _ADR defines in bits [51..48] which link the codec is connected to.
see example below from a TigerLake device with two identical amsp on
link 1 and 2.
Scope (_SB.PC00.HDAS.SNDW)
{
Device (SWD1)
{
Name (_ADR, 0x000131025D131601) // _ADR: Address
Device (SWD2)
{
Name (_ADR, 0x000230025D131601) // _ADR: Address
> As per MIPI Disco spec, for single link controllers Link ID should
> be set to zero.
> If we use Link ID as zero, for the soundwire manager which is on
> the second soundwire controller ACPI device scope, then soundwire
> framework is not allowing to create peripheral device node as its
> duplicate one.
I still don't see how it's possible. There is an IDA used in the bus
allocation
static int sdw_get_id(struct sdw_bus *bus)
{
int rc = ida_alloc(&sdw_bus_ida, GFP_KERNEL);
if (rc < 0)
return rc;
bus->id = rc;
return 0;
}
and that's used for debugfs
/* create the debugfs master-N */
snprintf(name, sizeof(name), "master-%d-%d", bus->id, bus->link_id);
as well as in sdw_master_device_add():
dev_set_name(&md->dev, "sdw-master-%d", bus->id);
can you clarify what part of the 'SoundWire framework' is problematic? I
guess the problem is that you have identical devices with the same _ADR
under the same manager, which is problematic indeed, but that's not a
SoundWire framework issue, just not a supported configuration.
> If we want to support two ACPI companion device approach
> on our future platforms, how to proceed?
Well how about dealing with a single companion device first, cause
that's what you have now and that's already problematic.
[Public]
> -----Original Message-----
> From: Pierre-Louis Bossart <[email protected]>
> Sent: Tuesday, January 31, 2023 10:01
> To: Mukunda, Vijendar <[email protected]>;
> [email protected]; [email protected]; [email protected]
> Cc: Katragadda, Mastan <[email protected]>; Dommati, Sunil-
> kumar <[email protected]>; open list <linux-
> [email protected]>; Hiregoudar, Basavaraj
> <[email protected]>; Takashi Iwai <[email protected]>; Liam
> Girdwood <[email protected]>; Nathan Chancellor
> <[email protected]>; Limonciello, Mario <[email protected]>;
> kondaveeti, Arungopal <[email protected]>; Sanyog Kale
> <[email protected]>; Bard Liao <[email protected]>;
> Saba Kareem, Syed <[email protected]>
> Subject: Re: [PATCH 01/19] ASoC: amd: ps: create platform devices based on
> acp config
>
>
>
> On 1/31/23 07:09, Mukunda,Vijendar wrote:
> > On 16/01/23 13:32, Mukunda,Vijendar wrote:
> >> On 13/01/23 22:41, Pierre-Louis Bossart wrote:
> >>>>>> + if (is_dmic_dev && is_sdw_dev) {
> >>>>>> + switch (acp_data->sdw_master_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;
> >>>>> so the cover letter is indeed wrong and confuses two controllers for
> two
> >>>>> managers.
> >>>> ACP IP has two independent manager instances driven by separate
> controller
> >>>> each which are connected in different power domains.
> >>>>
> >>>> we should create two separate ACPI companion devices for separate
> >>>> manager instance. Currently we have limitations with BIOS.
> >>>> we are going with single ACPI companion device.
> >>>> We will update the changes later.
> >>> Humm, this is tricky. The BIOS interface isn't something that can be
> >>> changed at will on the kernel side, you'd have to maintain two solutions
> >>> with a means to detect which one to use.
> >>>
> >>> Or is this is a temporary issue on development devices, then that part
> >>> should probably not be upstreamed.
> >> It's a temporary issue on development devices.
> >> We had discussion with Windows dev team and BIOS team.
> >> They have agreed to modify ACPI companion device logic.
> >> We will update the two companion devices logic for two manager
> >> instances in V2 version.
> > After experimenting, two ACPI companion devices approach,
> > we got an update from Windows team, there is a limitation
> > on windows stack. For current platform, we can't proceed
> > with two ACPI companion devices.
>
> so how would the two controllers be declared then in the DSDT used by
> Windows? There's a contradiction between having a single companion
> device and the ability to set the 'manager-number' to one.
>
> You probably want to give an example of what you have, otherwise we
> probably will talk past each other.
> >
> > Even on Linux side, if we create two ACPI companion devices
> > followed by creating a single soundwire manager instance per
> > Soundwire controller, we have observed an issue in a scenario,
> > where similar codec parts(UID are also same) are connected on
> > both soundwire manager instances.
>
> We've been handling this case of two identical amplifiers on two
> different links for the last 3 years. I don't see how this could be a
> problem, the codecs are declared in the scope of the companion device
> and the _ADR defines in bits [51..48] which link the codec is connected to.
>
The problem is that there are two managers in the specified AMD design, and
the codecs are both on "Link 0" for each manager.
So the _ADR really is identical for both.
> see example below from a TigerLake device with two identical amsp on
> link 1 and 2.
>
> Scope (_SB.PC00.HDAS.SNDW)
> {
> Device (SWD1)
> {
> Name (_ADR, 0x000131025D131601) // _ADR: Address
>
> Device (SWD2)
> {
> Name (_ADR, 0x000230025D131601) // _ADR: Address
>
> > As per MIPI Disco spec, for single link controllers Link ID should
> > be set to zero.
> > If we use Link ID as zero, for the soundwire manager which is on
> > the second soundwire controller ACPI device scope, then soundwire
> > framework is not allowing to create peripheral device node as its
> > duplicate one.
>
> I still don't see how it's possible. There is an IDA used in the bus
> allocation
>
> static int sdw_get_id(struct sdw_bus *bus)
> {
> int rc = ida_alloc(&sdw_bus_ida, GFP_KERNEL);
>
> if (rc < 0)
> return rc;
>
> bus->id = rc;
> return 0;
> }
>
> and that's used for debugfs
>
> /* create the debugfs master-N */
> snprintf(name, sizeof(name), "master-%d-%d", bus->id, bus-
> >link_id);
>
> as well as in sdw_master_device_add():
> dev_set_name(&md->dev, "sdw-master-%d", bus->id);
>
> can you clarify what part of the 'SoundWire framework' is problematic? I
> guess the problem is that you have identical devices with the same _ADR
> under the same manager, which is problematic indeed, but that's not a
> SoundWire framework issue, just not a supported configuration.
>
> > If we want to support two ACPI companion device approach
> > on our future platforms, how to proceed?
>
> Well how about dealing with a single companion device first, cause
> that's what you have now and that's already problematic.
>>>>>> we should create two separate ACPI companion devices for separate
>>>>>> manager instance. Currently we have limitations with BIOS.
>>>>>> we are going with single ACPI companion device.
>>>>>> We will update the changes later.
>>>>> Humm, this is tricky. The BIOS interface isn't something that can be
>>>>> changed at will on the kernel side, you'd have to maintain two solutions
>>>>> with a means to detect which one to use.
>>>>>
>>>>> Or is this is a temporary issue on development devices, then that part
>>>>> should probably not be upstreamed.
>>>> It's a temporary issue on development devices.
>>>> We had discussion with Windows dev team and BIOS team.
>>>> They have agreed to modify ACPI companion device logic.
>>>> We will update the two companion devices logic for two manager
>>>> instances in V2 version.
>>> After experimenting, two ACPI companion devices approach,
>>> we got an update from Windows team, there is a limitation
>>> on windows stack. For current platform, we can't proceed
>>> with two ACPI companion devices.
>>
>> so how would the two controllers be declared then in the DSDT used by
>> Windows? There's a contradiction between having a single companion
>> device and the ability to set the 'manager-number' to one.
>>
>> You probably want to give an example of what you have, otherwise we
>> probably will talk past each other.
>>>
>>> Even on Linux side, if we create two ACPI companion devices
>>> followed by creating a single soundwire manager instance per
>>> Soundwire controller, we have observed an issue in a scenario,
>>> where similar codec parts(UID are also same) are connected on
>>> both soundwire manager instances.
>>
>> We've been handling this case of two identical amplifiers on two
>> different links for the last 3 years. I don't see how this could be a
>> problem, the codecs are declared in the scope of the companion device
>> and the _ADR defines in bits [51..48] which link the codec is connected to.
>>
>
> The problem is that there are two managers in the specified AMD design, and
> the codecs are both on "Link 0" for each manager.
You're confusing Controller and Manager.
A Manager is the same as a 'Link', the two terms are interchangeable. It
makes no sense to refer to a link number for a manager because there is
no such concept.
Only a Controller can have multiple links or managers. And each
Controller needs to be declared as an ACPI device if you want to use the
DisCo properties.
The Managers/Links are not described as ACPI devices, that's a
regrettable design decision made in MIPI circles many moons ago, that's
why in the Intel code we have to manually create auxiliary devices based
on the 'mipi-sdw-master-count' property.
> So the _ADR really is identical for both.
That cannot possible work, even for Windows. You need to have a
controller scope, and the _ADR can then be identical for different
peripherals as long as this ADR is local to a controller scope.
On 01/02/23 06:21, Pierre-Louis Bossart wrote:
>>>>>>> we should create two separate ACPI companion devices for separate
>>>>>>> manager instance. Currently we have limitations with BIOS.
>>>>>>> we are going with single ACPI companion device.
>>>>>>> We will update the changes later.
>>>>>> Humm, this is tricky. The BIOS interface isn't something that can be
>>>>>> changed at will on the kernel side, you'd have to maintain two solutions
>>>>>> with a means to detect which one to use.
>>>>>>
>>>>>> Or is this is a temporary issue on development devices, then that part
>>>>>> should probably not be upstreamed.
>>>>> It's a temporary issue on development devices.
>>>>> We had discussion with Windows dev team and BIOS team.
>>>>> They have agreed to modify ACPI companion device logic.
>>>>> We will update the two companion devices logic for two manager
>>>>> instances in V2 version.
>>>> After experimenting, two ACPI companion devices approach,
>>>> we got an update from Windows team, there is a limitation
>>>> on windows stack. For current platform, we can't proceed
>>>> with two ACPI companion devices.
>>> so how would the two controllers be declared then in the DSDT used by
>>> Windows? There's a contradiction between having a single companion
>>> device and the ability to set the 'manager-number' to one.
>>>
>>> You probably want to give an example of what you have, otherwise we
>>> probably will talk past each other.
>>>> Even on Linux side, if we create two ACPI companion devices
>>>> followed by creating a single soundwire manager instance per
>>>> Soundwire controller, we have observed an issue in a scenario,
>>>> where similar codec parts(UID are also same) are connected on
>>>> both soundwire manager instances.
>>> We've been handling this case of two identical amplifiers on two
>>> different links for the last 3 years. I don't see how this could be a
>>> problem, the codecs are declared in the scope of the companion device
>>> and the _ADR defines in bits [51..48] which link the codec is connected to.
>>>
>> The problem is that there are two managers in the specified AMD design, and
>> the codecs are both on "Link 0" for each manager.
> You're confusing Controller and Manager.
>
> A Manager is the same as a 'Link', the two terms are interchangeable. It
> makes no sense to refer to a link number for a manager because there is
> no such concept.
>
> Only a Controller can have multiple links or managers. And each
> Controller needs to be declared as an ACPI device if you want to use the
> DisCo properties.
>
> The Managers/Links are not described as ACPI devices, that's a
> regrettable design decision made in MIPI circles many moons ago, that's
> why in the Intel code we have to manually create auxiliary devices based
> on the 'mipi-sdw-master-count' property.
>
>> So the _ADR really is identical for both.
Yes Controller has ACPI scope. Under controller based on
"mipi-sdw-manager-list" property manager instances will be created.
Manager and Link terms are interchangeable.
Below is the sample DSDT file if we go with two ACPI companion
devices.
Scope (\_SB.ACP)
{
Device (SWC0)
{
Name (_ADR, 0x05) // _ADR: Address
Name(_DSD, Package() {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
Package (2) {"mipi-sdw-manager-list", 1}, // v 1.0
},
ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), // Hierarchical Extension
Package () {
Package (2) {"mipi-sdw-link-0-subproperties", "SWM0"},
}
}) // End _DSD
Name(SWM0, Package() {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
// ... place holder for SWM0 additional properties
}
}) // End SWM0.SWM
Device (SLV0) { // SoundWire Slave 0
Name(_ADR, 0x000032025D131601)
} // END SLV0
} // END SWC0
Device (SWC1)
{
Name (_ADR, 0x09) // _ADR: Address
Name(_DSD, Package() {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
Package (2) {"mipi-sdw-manager-list", 1},
},
ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
Package () {
Package (2) {"mipi-sdw-link-0-subproperties", "SWM0"},
}
}) // End _DSD
Name(SWM0, Package() {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
// ... place holder for SWM0 additional properties
}
}) // End SWM0.SWM
Device (SLV0) { // SoundWire Slave 0
Name(_ADR, 0x000032025D131601)
} // END SLV0
} // END SWC1
}
}
In above case, two manager instances will be created.
When manager under SWC1 scope tries to add peripheral
device, In sdw_slave_add() API its failing because peripheral
device descriptor uses link id followed by 48bit encoded address.
In above scenarios, both the manager's link id is zero only.
> That cannot possible work, even for Windows. You need to have a
> controller scope, and the _ADR can then be identical for different
> peripherals as long as this ADR is local to a controller scope.
>
> Yes Controller has ACPI scope. Under controller based on
> "mipi-sdw-manager-list" property manager instances will be created.
> Manager and Link terms are interchangeable.
>
> Below is the sample DSDT file if we go with two ACPI companion
> devices.
>
> Scope (\_SB.ACP)
> {
>
> Device (SWC0)
> {
> Name (_ADR, 0x05) // _ADR: Address
> Name(_DSD, Package() {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
> Package (2) {"mipi-sdw-manager-list", 1}, // v 1.0
> },
> ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), // Hierarchical Extension
> Package () {
> Package (2) {"mipi-sdw-link-0-subproperties", "SWM0"},
> }
> }) // End _DSD
> Name(SWM0, Package() {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
> // ... place holder for SWM0 additional properties
> }
> }) // End SWM0.SWM
>
> Device (SLV0) { // SoundWire Slave 0
> Name(_ADR, 0x000032025D131601)
> } // END SLV0
>
> } // END SWC0
>
> Device (SWC1)
> {
> Name (_ADR, 0x09) // _ADR: Address
> Name(_DSD, Package() {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
> Package (2) {"mipi-sdw-manager-list", 1},
> },
> ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> Package () {
> Package (2) {"mipi-sdw-link-0-subproperties", "SWM0"},
>
> }
> }) // End _DSD
> Name(SWM0, Package() {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
> // ... place holder for SWM0 additional properties
> }
> }) // End SWM0.SWM
>
> Device (SLV0) { // SoundWire Slave 0
> Name(_ADR, 0x000032025D131601)
> } // END SLV0
>
> } // END SWC1
> }
> }
that looks good to me.
> In above case, two manager instances will be created.
> When manager under SWC1 scope tries to add peripheral
> device, In sdw_slave_add() API its failing because peripheral
> device descriptor uses link id followed by 48bit encoded address.
> In above scenarios, both the manager's link id is zero only.
what fails exactly? The device_register() ?
If yes, what the issue. the device name?
I wonder if we need to use something like
"name shall be sdw:bus_id:link:mfg:part:class"
so as to uniquify the device name, if that was the problem.
On 01/02/23 07:33, Pierre-Louis Bossart wrote:
>
>> Yes Controller has ACPI scope. Under controller based on
>> "mipi-sdw-manager-list" property manager instances will be created.
>> Manager and Link terms are interchangeable.
>>
>> Below is the sample DSDT file if we go with two ACPI companion
>> devices.
>>
>> Scope (\_SB.ACP)
>> {
>>
>> Device (SWC0)
>> {
>> Name (_ADR, 0x05) // _ADR: Address
>> Name(_DSD, Package() {
>> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> Package () {
>> Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
>> Package (2) {"mipi-sdw-manager-list", 1}, // v 1.0
>> },
>> ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), // Hierarchical Extension
>> Package () {
>> Package (2) {"mipi-sdw-link-0-subproperties", "SWM0"},
>> }
>> }) // End _DSD
>> Name(SWM0, Package() {
>> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> Package () {
>> Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
>> // ... place holder for SWM0 additional properties
>> }
>> }) // End SWM0.SWM
>>
>> Device (SLV0) { // SoundWire Slave 0
>> Name(_ADR, 0x000032025D131601)
>> } // END SLV0
>>
>> } // END SWC0
>>
>> Device (SWC1)
>> {
>> Name (_ADR, 0x09) // _ADR: Address
>> Name(_DSD, Package() {
>> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> Package () {
>> Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
>> Package (2) {"mipi-sdw-manager-list", 1},
>> },
>> ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>> Package () {
>> Package (2) {"mipi-sdw-link-0-subproperties", "SWM0"},
>>
>> }
>> }) // End _DSD
>> Name(SWM0, Package() {
>> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> Package () {
>> Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
>> // ... place holder for SWM0 additional properties
>> }
>> }) // End SWM0.SWM
>>
>> Device (SLV0) { // SoundWire Slave 0
>> Name(_ADR, 0x000032025D131601)
>> } // END SLV0
>>
>> } // END SWC1
>> }
>> }
> that looks good to me.
>
>> In above case, two manager instances will be created.
>> When manager under SWC1 scope tries to add peripheral
>> device, In sdw_slave_add() API its failing because peripheral
>> device descriptor uses link id followed by 48bit encoded address.
>> In above scenarios, both the manager's link id is zero only.
> what fails exactly? The device_register() ?
>
> If yes, what the issue. the device name?
device_register() is failing because of duplication of
device name.
> I wonder if we need to use something like
>
> "name shall be sdw:bus_id:link:mfg:part:class"
>
> so as to uniquify the device name, if that was the problem.
Yes correct.
>>> In above case, two manager instances will be created.
>>> When manager under SWC1 scope tries to add peripheral
>>> device, In sdw_slave_add() API its failing because peripheral
>>> device descriptor uses link id followed by 48bit encoded address.
>>> In above scenarios, both the manager's link id is zero only.
>> what fails exactly? The device_register() ?
>>
>> If yes, what the issue. the device name?
> device_register() is failing because of duplication of
> device name.
>> I wonder if we need to use something like
>>
>> "name shall be sdw:bus_id:link:mfg:part:class"
>>
>> so as to uniquify the device name, if that was the problem.
> Yes correct.
can you check https://github.com/thesofproject/linux/pull/4165 and see
if this works for you? I tested it on Intel platforms.
On 01/02/23 09:22, Pierre-Louis Bossart wrote:
>
>
>>>> In above case, two manager instances will be created.
>>>> When manager under SWC1 scope tries to add peripheral
>>>> device, In sdw_slave_add() API its failing because peripheral
>>>> device descriptor uses link id followed by 48bit encoded address.
>>>> In above scenarios, both the manager's link id is zero only.
>>> what fails exactly? The device_register() ?
>>>
>>> If yes, what the issue. the device name?
>> device_register() is failing because of duplication of
>> device name.
>>> I wonder if we need to use something like
>>>
>>> "name shall be sdw:bus_id:link:mfg:part:class"
>>>
>>> so as to uniquify the device name, if that was the problem.
>> Yes correct.
> can you check https://github.com/thesofproject/linux/pull/4165 and see
> if this works for you? I tested it on Intel platforms.
It's working fine on our platform. As mentioned earlier in this thread,
we can't go with two ACPI companion device approach due to
limitations on windows stack for current platform.
On 2/1/23 00:01, Mukunda,Vijendar wrote:
> On 01/02/23 09:22, Pierre-Louis Bossart wrote:
>>
>>
>>>>> In above case, two manager instances will be created.
>>>>> When manager under SWC1 scope tries to add peripheral
>>>>> device, In sdw_slave_add() API its failing because peripheral
>>>>> device descriptor uses link id followed by 48bit encoded address.
>>>>> In above scenarios, both the manager's link id is zero only.
>>>> what fails exactly? The device_register() ?
>>>>
>>>> If yes, what the issue. the device name?
>>> device_register() is failing because of duplication of
>>> device name.
>>>> I wonder if we need to use something like
>>>>
>>>> "name shall be sdw:bus_id:link:mfg:part:class"
>>>>
>>>> so as to uniquify the device name, if that was the problem.
>>> Yes correct.
>> can you check https://github.com/thesofproject/linux/pull/4165 and see
>> if this works for you? I tested it on Intel platforms.
> It's working fine on our platform. As mentioned earlier in this thread,
> we can't go with two ACPI companion device approach due to
> limitations on windows stack for current platform.
Thanks for testing.
So if you can't go with 2 ACPI companion devices, what does the
'Windows' DSDT look like and how would you identify that there are two
controllers on the platform?
On 02/02/23 04:38, Pierre-Louis Bossart wrote:
>
> On 2/1/23 00:01, Mukunda,Vijendar wrote:
>> On 01/02/23 09:22, Pierre-Louis Bossart wrote:
>>>
>>>>>> In above case, two manager instances will be created.
>>>>>> When manager under SWC1 scope tries to add peripheral
>>>>>> device, In sdw_slave_add() API its failing because peripheral
>>>>>> device descriptor uses link id followed by 48bit encoded address.
>>>>>> In above scenarios, both the manager's link id is zero only.
>>>>> what fails exactly? The device_register() ?
>>>>>
>>>>> If yes, what the issue. the device name?
>>>> device_register() is failing because of duplication of
>>>> device name.
>>>>> I wonder if we need to use something like
>>>>>
>>>>> "name shall be sdw:bus_id:link:mfg:part:class"
>>>>>
>>>>> so as to uniquify the device name, if that was the problem.
>>>> Yes correct.
>>> can you check https://github.com/thesofproject/linux/pull/4165 and see
>>> if this works for you? I tested it on Intel platforms.
>> It's working fine on our platform. As mentioned earlier in this thread,
>> we can't go with two ACPI companion device approach due to
>> limitations on windows stack for current platform.
> Thanks for testing.
>
> So if you can't go with 2 ACPI companion devices, what does the
> 'Windows' DSDT look like and how would you identify that there are two
> controllers on the platform?
We are not populating two controller devices. Instead of it, we are populating
single controller device with two independent manager instances under the same
ACPI device scope.
We have configuration register to identify sound wire manager instances on the platform.
Below is the sample DSDT for Windows & Linux.
Scope (\_SB.ACP)
{
Device (SDWC)
{
Name (_ADR, 0x05) // _ADR: Address
Name(_DSD, Package() {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
Package (2) {"mipi-sdw-manager-list", 2},
},
ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
Package () {
Package (2) {"mipi-sdw-link-0-subproperties", "SWM0"},
Package (2) {"mipi-sdw-link-1-subproperties", "SWM1"},
}
}) // End _DSD
Name(SWM0, Package() {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
// ... place holder for SWM0 additional properties
}
}) // End SWM0.SWM
Name(SWM1,Package(){
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
// ... place holder for SWM1 additional properties
}
}) // End SWM1.SWM
Device (SLV0) { // SoundWire Slave 0
Name(_ADR, 0x000032025D131601)
} // END SLV0
Device (SLV1) { // SoundWire Slave 1
Name(_ADR, 0x000130025D131601)
} // END SLV1
} // END SDWC
}
>>>>>>> In above case, two manager instances will be created.
>>>>>>> When manager under SWC1 scope tries to add peripheral
>>>>>>> device, In sdw_slave_add() API its failing because peripheral
>>>>>>> device descriptor uses link id followed by 48bit encoded address.
>>>>>>> In above scenarios, both the manager's link id is zero only.
So here you're reporting that the issue is that all devices use link0 ...
>>>>>> what fails exactly? The device_register() ?
>>>>>>
>>>>>> If yes, what the issue. the device name?
>>>>> device_register() is failing because of duplication of
>>>>> device name.
>>>>>> I wonder if we need to use something like
>>>>>>
>>>>>> "name shall be sdw:bus_id:link:mfg:part:class"
>>>>>>
>>>>>> so as to uniquify the device name, if that was the problem.
>>>>> Yes correct.
>>>> can you check https://github.com/thesofproject/linux/pull/4165 and see
>>>> if this works for you? I tested it on Intel platforms.
>>> It's working fine on our platform. As mentioned earlier in this thread,
>>> we can't go with two ACPI companion device approach due to
>>> limitations on windows stack for current platform.
>> Thanks for testing.
>>
>> So if you can't go with 2 ACPI companion devices, what does the
>> 'Windows' DSDT look like and how would you identify that there are two
>> controllers on the platform?
> We are not populating two controller devices. Instead of it, we are populating
> single controller device with two independent manager instances under the same
> ACPI device scope.
> We have configuration register to identify sound wire manager instances on the platform.
> Below is the sample DSDT for Windows & Linux.
>
> Scope (\_SB.ACP)
> {
>
> Device (SDWC)
> {
> Name (_ADR, 0x05) // _ADR: Address
> Name(_DSD, Package() {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
> Package (2) {"mipi-sdw-manager-list", 2},
> },
> ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> Package () {
> Package (2) {"mipi-sdw-link-0-subproperties", "SWM0"},
> Package (2) {"mipi-sdw-link-1-subproperties", "SWM1"},
> }
> }) // End _DSD
> Name(SWM0, Package() {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
>
> // ... place holder for SWM0 additional properties
> }
> }) // End SWM0.SWM
> Name(SWM1,Package(){
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
>
> // ... place holder for SWM1 additional properties
> }
> }) // End SWM1.SWM
>
> Device (SLV0) { // SoundWire Slave 0
> Name(_ADR, 0x000032025D131601)
> } // END SLV0
>
> Device (SLV1) { // SoundWire Slave 1
> Name(_ADR, 0x000130025D131601)
> } // END SLV1
... but here you have two different link numbers.
I interpret this as SLV0 on link0 and SLV1 on link1.
So what's the issue?
On 06/02/23 20:20, Pierre-Louis Bossart wrote:
>>>>>>>> In above case, two manager instances will be created.
>>>>>>>> When manager under SWC1 scope tries to add peripheral
>>>>>>>> device, In sdw_slave_add() API its failing because peripheral
>>>>>>>> device descriptor uses link id followed by 48bit encoded address.
>>>>>>>> In above scenarios, both the manager's link id is zero only.
> So here you're reporting that the issue is that all devices use link0 ...
>
>>>>>>> what fails exactly? The device_register() ?
>>>>>>>
>>>>>>> If yes, what the issue. the device name?
>>>>>> device_register() is failing because of duplication of
>>>>>> device name.
>>>>>>> I wonder if we need to use something like
>>>>>>>
>>>>>>> "name shall be sdw:bus_id:link:mfg:part:class"
>>>>>>>
>>>>>>> so as to uniquify the device name, if that was the problem.
>>>>>> Yes correct.
>>>>> can you check https://github.com/thesofproject/linux/pull/4165 and see
>>>>> if this works for you? I tested it on Intel platforms.
>>>> It's working fine on our platform. As mentioned earlier in this thread,
>>>> we can't go with two ACPI companion device approach due to
>>>> limitations on windows stack for current platform.
>>> Thanks for testing.
>>>
>>> So if you can't go with 2 ACPI companion devices, what does the
>>> 'Windows' DSDT look like and how would you identify that there are two
>>> controllers on the platform?
>> We are not populating two controller devices. Instead of it, we are populating
>> single controller device with two independent manager instances under the same
>> ACPI device scope.
>> We have configuration register to identify sound wire manager instances on the platform.
>> Below is the sample DSDT for Windows & Linux.
>>
>> Scope (\_SB.ACP)
>> {
>>
>> Device (SDWC)
>> {
>> Name (_ADR, 0x05) // _ADR: Address
>> Name(_DSD, Package() {
>> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> Package () {
>> Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
>> Package (2) {"mipi-sdw-manager-list", 2},
>> },
>> ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>> Package () {
>> Package (2) {"mipi-sdw-link-0-subproperties", "SWM0"},
>> Package (2) {"mipi-sdw-link-1-subproperties", "SWM1"},
>> }
>> }) // End _DSD
>> Name(SWM0, Package() {
>> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> Package () {
>> Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
>>
>> // ... place holder for SWM0 additional properties
>> }
>> }) // End SWM0.SWM
>> Name(SWM1,Package(){
>> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> Package () {
>> Package (2) {"mipi-sdw-sw-interface-revision", 0x00010000},
>>
>> // ... place holder for SWM1 additional properties
>> }
>> }) // End SWM1.SWM
>>
>> Device (SLV0) { // SoundWire Slave 0
>> Name(_ADR, 0x000032025D131601)
>> } // END SLV0
>>
>> Device (SLV1) { // SoundWire Slave 1
>> Name(_ADR, 0x000130025D131601)
>> } // END SLV1
> ... but here you have two different link numbers.
>
> I interpret this as SLV0 on link0 and SLV1 on link1.
>
> So what's the issue?
This solution works fine for us. We have shared sample DSDT for reference.
By reading the ACP configuration register, controller count information is retrieved.
Each Controller device scope has a single manager instance.
To support this design, we need to create two ACPI companion devices.
Under each controller device, one manager instance device is scoped.
In this case, we will read "mipi-sdw-manager-list" as 1 for each controller.
As per your review comment, we can't go with two ACPI companion devices
approach due to Windows stack limitation.
Windows DSDT implementation will refer to single ACPI companion device with two
manager instances as mentioned in earlier reply. We are going to use the same
for Linux.