[00/19] Add soundwire support for Pink Sardine platform

Pink Sardine(ps) platform is based on ACP6.3 Architecture.
ACP6.3 IP has two soundwire controller instance support.
This patchset add support for Soundwire controller on Pink Sardine
platform.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
Vijendar Mukunda (19):
  ASoC: amd: ps: create platform devices based on acp config
  soundwire: amd: Add support for AMD Master driver
  soundwire: amd: register sdw controller dai ops
  soundwire: amd: enable build for AMD soundwire master driver
  soundwire: amd: add soundwire interrupt handling
  ASoC: amd: ps: add support for soundwire interrupts in acp pci driver
  ASoC: amd: ps: add soundwire dma driver for pink sardine platform
  ASoC: amd: ps: add soundwire dma driver dma ops
  ASoC: amd: ps: add support for Soundwire DMA interrupts
  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.
  ASoC: amd: ps: add support for runtime pm ops for soundwire dma driver
  soundwire: amd: add runtime pm ops for AMD master driver
  soundwire: amd: add startup and shutdown dai ops
  soundwire: amd: handle wake enable interrupt
  soundwire: amd: add pm_prepare callback and pm ops support
  ASoC: amd: ps: implement system level pm ops for soundwire dma driver
  ASoC: amd: ps: increase runtime suspend delay

 drivers/soundwire/Kconfig         |    9 +
 drivers/soundwire/Makefile        |    4 +
 drivers/soundwire/amd_master.c    | 1734 +++++++++++++++++++++++++++++
 drivers/soundwire/amd_master.h    |  284 +++++
 include/linux/soundwire/sdw_amd.h |   65 ++
 sound/soc/amd/Kconfig             |    3 +-
 sound/soc/amd/ps/Makefile         |    2 +
 sound/soc/amd/ps/acp63.h          |   98 +-
 sound/soc/amd/ps/pci-ps.c         |  383 ++++++-
 sound/soc/amd/ps/ps-sdw-dma.c     |  728 ++++++++++++
 10 files changed, 3287 insertions(+), 23 deletions(-)
 create mode 100644 drivers/soundwire/amd_master.c
 create mode 100644 drivers/soundwire/amd_master.h
 create mode 100644 include/linux/soundwire/sdw_amd.h
 create mode 100644 sound/soc/amd/ps/ps-sdw-dma.c

> +#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 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 <Vijendar.Mukunda@amd.com>
>> Signed-off-by: Mastan Katragadda <Mastan.Katragadda@amd.com>
>> ---
>>   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.
>
>

On 1/11/23 03:02, Vijendar Mukunda wrote:
> Register dai ops for two controller instances.

manager instances

> diff --git a/drivers/soundwire/amd_master.c b/drivers/soundwire/amd_master.c
> index 7e1f618254ac..93bffe6ff9e2 100644
> --- a/drivers/soundwire/amd_master.c
> +++ b/drivers/soundwire/amd_master.c
> @@ -952,6 +952,186 @@ static const struct sdw_master_ops amd_sdwc_ops = {
>  	.read_ping_status = amd_sdwc_read_ping_status,
>  };
>  
> +static int amd_sdwc_hw_params(struct snd_pcm_substream *substream,
> +			      struct snd_pcm_hw_params *params,
> +			      struct snd_soc_dai *dai)
> +{
> +	struct amd_sdwc_ctrl *ctrl = snd_soc_dai_get_drvdata(dai);
> +	struct sdw_amd_dma_data *dma;
> +	struct sdw_stream_config sconfig;
> +	struct sdw_port_config *pconfig;
> +	int ch, dir;
> +	int ret;
> +
> +	dma = snd_soc_dai_get_dma_data(dai, substream);
> +	if (!dma)
> +		return -EIO;
> +
> +	ch = params_channels(params);
> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> +		dir = SDW_DATA_DIR_RX;
> +	else
> +		dir = SDW_DATA_DIR_TX;
> +	dev_dbg(ctrl->dev, "%s: dir:%d dai->id:0x%x\n", __func__, dir, dai->id);
> +	dma->hw_params = params;
> +
> +	sconfig.direction = dir;
> +	sconfig.ch_count = ch;
> +	sconfig.frame_rate = params_rate(params);
> +	sconfig.type = dma->stream_type;
> +
> +	sconfig.bps = snd_pcm_format_width(params_format(params));
> +
> +	/* Port configuration */
> +	pconfig = kzalloc(sizeof(*pconfig), GFP_KERNEL);
> +	if (!pconfig) {
> +		ret =  -ENOMEM;
> +		goto error;
> +	}
> +
> +	pconfig->num = dai->id;
> +	pconfig->ch_mask = (1 << ch) - 1;
> +	ret = sdw_stream_add_master(&ctrl->bus, &sconfig,
> +				    pconfig, 1, dma->stream);
> +	if (ret)
> +		dev_err(ctrl->dev, "add master to stream failed:%d\n", ret);
> +
> +	kfree(pconfig);
> +error:
> +	return ret;
> +}

This looks inspired from intel.c, but you are not programming ANY
registers here. is this intentional?

> +static int amd_sdwc_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
> +{
> +	struct amd_sdwc_ctrl *ctrl = snd_soc_dai_get_drvdata(dai);
> +	struct sdw_amd_dma_data *dma;
> +	int ret;
> +
> +	dma = snd_soc_dai_get_dma_data(dai, substream);
> +	if (!dma)
> +		return -EIO;
> +
> +	ret = sdw_stream_remove_master(&ctrl->bus, dma->stream);
> +	if (ret < 0) {
> +		dev_err(dai->dev, "remove master from stream %s failed: %d\n",
> +			dma->stream->name, ret);
> +		return ret;
> +	}
> +	dma->hw_params = NULL;
> +	return 0;
> +}
> +
> +static int amd_set_sdw_stream(struct snd_soc_dai *dai, void *stream, int direction)
> +{
> +	struct amd_sdwc_ctrl *ctrl = snd_soc_dai_get_drvdata(dai);
> +	struct sdw_amd_dma_data *dma;

you want to avoid using dma_data and use your own runtime. We made that
change recently for cadence_runtime.c

> +
> +	if (stream) {
> +		if (direction == SNDRV_PCM_STREAM_PLAYBACK)
> +			dma = dai->playback_dma_data;
> +		else
> +			dma = dai->capture_dma_data;
> +
> +		if (dma) {
> +			dev_err(dai->dev,
> +				"dma_data already allocated for dai %s\n",
> +				dai->name);
> +			return -EINVAL;
> +		}
> +
> +		/* allocate and set dma info */
> +		dma = kzalloc(sizeof(*dma), GFP_KERNEL);
> +		if (!dma)
> +			return -ENOMEM;
> +		dma->stream_type = SDW_STREAM_PCM;
> +		dma->bus = &ctrl->bus;
> +		dma->link_id = ctrl->instance;
> +		dma->stream = stream;
> +
> +		if (direction == SNDRV_PCM_STREAM_PLAYBACK)
> +			dai->playback_dma_data = dma;
> +		else
> +			dai->capture_dma_data = dma;
> +	} else {
> +		if (direction == SNDRV_PCM_STREAM_PLAYBACK) {
> +			kfree(dai->playback_dma_data);
> +			dai->playback_dma_data = NULL;
> +		} else {
> +			kfree(dai->capture_dma_data);
> +			dai->capture_dma_data = NULL;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int amd_pcm_set_sdw_stream(struct snd_soc_dai *dai, void *stream, int direction)
> +{
> +	return amd_set_sdw_stream(dai, stream, direction);
> +}
> +
> +static void *amd_get_sdw_stream(struct snd_soc_dai *dai, int direction)
> +{
> +	struct sdw_amd_dma_data *dma;
> +
> +	if (direction == SNDRV_PCM_STREAM_PLAYBACK)
> +		dma = dai->playback_dma_data;
> +	else
> +		dma = dai->capture_dma_data;
> +
> +	if (!dma)
> +		return ERR_PTR(-EINVAL);
> +
> +	return dma->stream;
> +}
> +
> +static const struct snd_soc_dai_ops amd_sdwc_dai_ops = {
> +	.hw_params = amd_sdwc_hw_params,
> +	.hw_free = amd_sdwc_hw_free,
> +	.set_stream = amd_pcm_set_sdw_stream,

In the first patch there was support for PDM exposed, but here it's PDM
only?

> +	.get_stream = amd_get_sdw_stream,
> +};
> +
> +static const struct snd_soc_component_driver amd_sdwc_dai_component = {
> +	.name = "soundwire",
> +};
> +
> +static int amd_sdwc_register_dais(struct amd_sdwc_ctrl *ctrl)
> +{
> +	struct snd_soc_dai_driver *dais;
> +	struct snd_soc_pcm_stream *stream;
> +	struct device *dev;
> +	int i, num_dais;
> +
> +	dev = ctrl->dev;
> +	num_dais = ctrl->num_dout_ports + ctrl->num_din_ports;
> +	dais = devm_kcalloc(dev, num_dais, sizeof(*dais), GFP_KERNEL);
> +	if (!dais)
> +		return -ENOMEM;
> +	for (i = 0; i < num_dais; i++) {
> +		dais[i].name = devm_kasprintf(dev, GFP_KERNEL, "SDW%d Pin%d", ctrl->instance, i);
> +		if (!dais[i].name) {
> +			dev_err(ctrl->dev, "-ENOMEM dai name allocation failed\n");

remove, we don't add error logs on memory allocation issues.

> +			return -ENOMEM;
> +		}
> +
> +		if (i < ctrl->num_dout_ports)
> +			stream = &dais[i].playback;
> +		else
> +			stream = &dais[i].capture;
> +
> +		stream->channels_min = 2;
> +		stream->channels_max = 2;

Is this a port limitation or just a software definition?

> +		stream->rates = SNDRV_PCM_RATE_48000;
> +		stream->formats = SNDRV_PCM_FMTBIT_S16_LE;

Wondering if this is needed. I don't even recall why it's in the Intel
code, we tested with 32 bit data and 192kHz, that looks unnecessary to
me unless the hardware is really limited to those values.

> +
> +		dais[i].ops = &amd_sdwc_dai_ops;
> +		dais[i].id = i;
> +	}
> +
> +	return devm_snd_soc_register_component(ctrl->dev, &amd_sdwc_dai_component,
> +					       dais, num_dais);
> +}
> +
>  static void amd_sdwc_probe_work(struct work_struct *work)
>  {
>  	struct amd_sdwc_ctrl *ctrl  = container_of(work, struct amd_sdwc_ctrl, probe_work);
> @@ -1043,6 +1223,12 @@ static int amd_sdwc_probe(struct platform_device *pdev)
>  			ret);
>  		return ret;
>  	}
> +	ret = amd_sdwc_register_dais(ctrl);
> +	if (ret) {
> +		dev_err(dev, "CPU DAI registration failed\n");
> +		sdw_bus_master_delete(&ctrl->bus);
> +		return ret;
> +	}
>  	INIT_WORK(&ctrl->probe_work, amd_sdwc_probe_work);
>  	schedule_work(&ctrl->probe_work);
>  	return 0;
> diff --git a/include/linux/soundwire/sdw_amd.h b/include/linux/soundwire/sdw_amd.h
> index 5ec39f8c2f2e..7a99d782969f 100644
> --- a/include/linux/soundwire/sdw_amd.h
> +++ b/include/linux/soundwire/sdw_amd.h
> @@ -13,6 +13,7 @@
>  #define ACP_SDW0	0
>  #define ACP_SDW1	1
>  #define ACP_SDW0_MAX_DAI	6
> +#define AMD_SDW_MAX_DAIS	8

How does this work? 6 dais for the first master and 2 for the second?

>  
>  struct acp_sdw_pdata {
>  	u16 instance;
> @@ -25,6 +26,7 @@ struct amd_sdwc_ctrl {
>  	void __iomem *mmio;
>  	struct work_struct probe_work;
>  	struct mutex *sdw_lock;
> +	struct sdw_stream_runtime *sruntime[AMD_SDW_MAX_DAIS];

well no, a stream runtime needs to be allocated per stream and usually
there's a 1:1 mapping between dailink and stream. A stream may use
multiple DAIs, possibly on different masters - just like a dailink can
rely on multiple cpu- and codec-dais.

You are conflating/confusing concepts I am afraid here.

>  	int num_din_ports;
>  	int num_dout_ports;
>  	int cols_index;
> @@ -36,4 +38,23 @@ struct amd_sdwc_ctrl {
>  	bool startup_done;
>  	u32 power_mode_mask;
>  };
> +
> +/**
> + * struct sdw_amd_dma_data: AMD DMA data
> + *
> + * @name: SoundWire stream name
> + * @stream: stream runtime
> + * @bus: Bus handle
> + * @stream_type: Stream type
> + * @link_id: Master link id
> + * @hw_params: hw_params to be applied in .prepare step
> + */
> +struct sdw_amd_dma_data {
> +	char *name;
> +	struct sdw_stream_runtime *stream;
> +	struct sdw_bus *bus;
> +	enum sdw_stream_type stream_type;
> +	int link_id;
> +	struct snd_pcm_hw_params *hw_params;
> +};
>  #endif

On 1/11/23 03:02, Vijendar Mukunda wrote:
> Add start up and shutdown dai ops for AMD Master driver.

I don't think those startup and shutdown ops are necessary, we removed
them for Intel, see "soundwire: intel: remove DAI startup/shutdown"

On 1/11/23 03:02, Vijendar Mukunda wrote:
> Add pm_prepare callback and System level pm ops support for
> AMD master driver.
> 
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> Signed-off-by: Mastan Katragadda <Mastan.Katragadda@amd.com>
> ---
>  drivers/soundwire/amd_master.c | 76 ++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/drivers/soundwire/amd_master.c b/drivers/soundwire/amd_master.c
> index 2fd77a673c22..f4478cc17aac 100644
> --- a/drivers/soundwire/amd_master.c
> +++ b/drivers/soundwire/amd_master.c
> @@ -1552,6 +1552,80 @@ static int amd_sdwc_clock_stop_exit(struct amd_sdwc_ctrl *ctrl)
>  	return 0;
>  }
>  
> +static int amd_resume_child_device(struct device *dev, void *data)
> +{
> +	int ret;
> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +
> +	if (!slave->probed) {
> +		dev_dbg(dev, "skipping device, no probed driver\n");
> +		return 0;
> +	}
> +	if (!slave->dev_num_sticky) {
> +		dev_dbg(dev, "skipping device, never detected on bus\n");
> +		return 0;
> +	}
> +
> +	if (!pm_runtime_suspended(dev))
> +		return 0;
> +	ret = pm_request_resume(dev);
> +	if (ret < 0)
> +		dev_err(dev, "%s: pm_request_resume failed: %d\n", __func__, ret);
> +
> +	return ret;
> +}
> +
> +static int __maybe_unused amd_pm_prepare(struct device *dev)
> +{
> +	struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev);
> +	struct sdw_bus *bus = &ctrl->bus;
> +	int ret;
> +
> +	if (bus->prop.hw_disabled || !ctrl->startup_done) {
> +		dev_dbg(bus->dev, "SoundWire master %d is disabled or not-started, ignoring\n",
> +			bus->link_id);
> +		return 0;
> +	}
> +	ret = device_for_each_child(bus->dev, NULL, amd_resume_child_device);
> +	if (ret < 0)
> +		dev_err(dev, "%s: amd_resume_child_device failed: %d\n", __func__, ret);
> +	if (pm_runtime_suspended(dev) && ctrl->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
> +		ret = pm_request_resume(dev);
> +		if (ret < 0) {
> +			dev_err(bus->dev, "pm_request_resume failed: %d\n", ret);
> +			return 0;
> +		}
> +	}
> +	return 0;
> +}

This seems to be inspired by the Intel code, but is this necessary here?

For Intel, we saw cases where we had to pm_resume before doing a system
suspend, otherwise the hardware was in a bad state.

Do you actually need to do so, or is is possible to do a system suspend
when the clock is stopped.

And in the case where the bus is in 'power-off' mode, do you actually
need to resume at all?

> +
> +static int __maybe_unused amd_suspend(struct device *dev)
> +{
> +	struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev);
> +	struct sdw_bus *bus = &ctrl->bus;
> +	int ret;
> +
> +	if (bus->prop.hw_disabled || !ctrl->startup_done) {
> +		dev_dbg(bus->dev, "SoundWire master %d is disabled or not-started, ignoring\n",
> +			bus->link_id);
> +		return 0;
> +	}
> +
> +	if (ctrl->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
> +		ret = amd_sdwc_clock_stop(ctrl);
> +		if (ret)
> +			return ret;
> +	} else if (ctrl->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
> +		ret = amd_sdwc_clock_stop(ctrl);

do you actually need to stop the clock before powering-off? This seems
counter intuitive and not so useful?

> +		if (ret)
> +			return ret;
> +		ret = amd_deinit_sdw_controller(ctrl);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
>  static int __maybe_unused amd_suspend_runtime(struct device *dev)
>  {
>  	struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev);
> @@ -1638,6 +1712,8 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)
>  }
>  
>  static const struct dev_pm_ops amd_pm = {
> +	.prepare = amd_pm_prepare,
> +	SET_SYSTEM_SLEEP_PM_OPS(amd_suspend, amd_resume_runtime)
>  	SET_RUNTIME_PM_OPS(amd_suspend_runtime, amd_resume_runtime, NULL)
>  };
>

On 11/01/23 19:06, Pierre-Louis Bossart wrote:
>
> On 1/11/23 03:02, Vijendar Mukunda wrote:
>> Pink Sardine(ps) platform is based on ACP6.3 Architecture.
>> ACP6.3 IP has two soundwire controller instance support.
> After reviewing patch1, it looks like there's a confusion between
> controller and manager? the use of the 'sdw-master-count' property is
> not for controller count, it's defined within the scope of an ACPI
> controller device (usually a companion device to the PCI device).
Sorry for confusion. We are meant to refer Master/Manager instance
in throughout the code. we will replace the "Controller" reference
with "Manager". We will correct the cover letter.
>> This patchset add support for Soundwire controller on Pink Sardine
>> platform.
> That's great, thanks for sharing.
>
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>> Vijendar Mukunda (19):
>>   ASoC: amd: ps: create platform devices based on acp config
>>   soundwire: amd: Add support for AMD Master driver
>>   soundwire: amd: register sdw controller dai ops
>>   soundwire: amd: enable build for AMD soundwire master driver
>>   soundwire: amd: add soundwire interrupt handling
>>   ASoC: amd: ps: add support for soundwire interrupts in acp pci driver
>>   ASoC: amd: ps: add soundwire dma driver for pink sardine platform
>>   ASoC: amd: ps: add soundwire dma driver dma ops
>>   ASoC: amd: ps: add support for Soundwire DMA interrupts
>>   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.
>>   ASoC: amd: ps: add support for runtime pm ops for soundwire dma driver
>>   soundwire: amd: add runtime pm ops for AMD master driver
>>   soundwire: amd: add startup and shutdown dai ops
>>   soundwire: amd: handle wake enable interrupt
>>   soundwire: amd: add pm_prepare callback and pm ops support
>>   ASoC: amd: ps: implement system level pm ops for soundwire dma driver
>>   ASoC: amd: ps: increase runtime suspend delay
>>
>>  drivers/soundwire/Kconfig         |    9 +
>>  drivers/soundwire/Makefile        |    4 +
>>  drivers/soundwire/amd_master.c    | 1734 +++++++++++++++++++++++++++++
>>  drivers/soundwire/amd_master.h    |  284 +++++
>>  include/linux/soundwire/sdw_amd.h |   65 ++
>>  sound/soc/amd/Kconfig             |    3 +-
>>  sound/soc/amd/ps/Makefile         |    2 +
>>  sound/soc/amd/ps/acp63.h          |   98 +-
>>  sound/soc/amd/ps/pci-ps.c         |  383 ++++++-
>>  sound/soc/amd/ps/ps-sdw-dma.c     |  728 ++++++++++++
>>  10 files changed, 3287 insertions(+), 23 deletions(-)
>>  create mode 100644 drivers/soundwire/amd_master.c
>>  create mode 100644 drivers/soundwire/amd_master.h
>>  create mode 100644 include/linux/soundwire/sdw_amd.h
>>  create mode 100644 sound/soc/amd/ps/ps-sdw-dma.c
>>

On 11/01/23 21:28, Pierre-Louis Bossart wrote:
>
> On 1/11/23 03:02, Vijendar Mukunda wrote:
>> Add pm_prepare callback and System level pm ops support for
>> AMD master driver.
>>
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>> Signed-off-by: Mastan Katragadda <Mastan.Katragadda@amd.com>
>> ---
>>  drivers/soundwire/amd_master.c | 76 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 76 insertions(+)
>>
>> diff --git a/drivers/soundwire/amd_master.c b/drivers/soundwire/amd_master.c
>> index 2fd77a673c22..f4478cc17aac 100644
>> --- a/drivers/soundwire/amd_master.c
>> +++ b/drivers/soundwire/amd_master.c
>> @@ -1552,6 +1552,80 @@ static int amd_sdwc_clock_stop_exit(struct amd_sdwc_ctrl *ctrl)
>>  	return 0;
>>  }
>>  
>> +static int amd_resume_child_device(struct device *dev, void *data)
>> +{
>> +	int ret;
>> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
>> +
>> +	if (!slave->probed) {
>> +		dev_dbg(dev, "skipping device, no probed driver\n");
>> +		return 0;
>> +	}
>> +	if (!slave->dev_num_sticky) {
>> +		dev_dbg(dev, "skipping device, never detected on bus\n");
>> +		return 0;
>> +	}
>> +
>> +	if (!pm_runtime_suspended(dev))
>> +		return 0;
>> +	ret = pm_request_resume(dev);
>> +	if (ret < 0)
>> +		dev_err(dev, "%s: pm_request_resume failed: %d\n", __func__, ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int __maybe_unused amd_pm_prepare(struct device *dev)
>> +{
>> +	struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev);
>> +	struct sdw_bus *bus = &ctrl->bus;
>> +	int ret;
>> +
>> +	if (bus->prop.hw_disabled || !ctrl->startup_done) {
>> +		dev_dbg(bus->dev, "SoundWire master %d is disabled or not-started, ignoring\n",
>> +			bus->link_id);
>> +		return 0;
>> +	}
>> +	ret = device_for_each_child(bus->dev, NULL, amd_resume_child_device);
>> +	if (ret < 0)
>> +		dev_err(dev, "%s: amd_resume_child_device failed: %d\n", __func__, ret);
>> +	if (pm_runtime_suspended(dev) && ctrl->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
>> +		ret = pm_request_resume(dev);
>> +		if (ret < 0) {
>> +			dev_err(bus->dev, "pm_request_resume failed: %d\n", ret);
>> +			return 0;
>> +		}
>> +	}
>> +	return 0;
>> +}
> This seems to be inspired by the Intel code, but is this necessary here?
No It's not inspired by intel code. Initially, we haven't included
pm_prepare callback. We have observed issues without
pm_prepare callback.
> For Intel, we saw cases where we had to pm_resume before doing a system
> suspend, otherwise the hardware was in a bad state.
>
> Do you actually need to do so, or is is possible to do a system suspend
> when the clock is stopped.
>
> And in the case where the bus is in 'power-off' mode, do you actually
> need to resume at all?
Our platform supports different power modes. To support all
combinations, we have included pm_prepare callback.
>> +
>> +static int __maybe_unused amd_suspend(struct device *dev)
>> +{
>> +	struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev);
>> +	struct sdw_bus *bus = &ctrl->bus;
>> +	int ret;
>> +
>> +	if (bus->prop.hw_disabled || !ctrl->startup_done) {
>> +		dev_dbg(bus->dev, "SoundWire master %d is disabled or not-started, ignoring\n",
>> +			bus->link_id);
>> +		return 0;
>> +	}
>> +
>> +	if (ctrl->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
>> +		ret = amd_sdwc_clock_stop(ctrl);
>> +		if (ret)
>> +			return ret;
>> +	} else if (ctrl->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
>> +		ret = amd_sdwc_clock_stop(ctrl);
> do you actually need to stop the clock before powering-off? This seems
> counter intuitive and not so useful?
Yes, as per our design, we need to stop the clock
before powering off.
>> +		if (ret)
>> +			return ret;
>> +		ret = amd_deinit_sdw_controller(ctrl);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +	return 0;
>> +}
>> +
>>  static int __maybe_unused amd_suspend_runtime(struct device *dev)
>>  {
>>  	struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev);
>> @@ -1638,6 +1712,8 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)
>>  }
>>  
>>  static const struct dev_pm_ops amd_pm = {
>> +	.prepare = amd_pm_prepare,
>> +	SET_SYSTEM_SLEEP_PM_OPS(amd_suspend, amd_resume_runtime)
>>  	SET_RUNTIME_PM_OPS(amd_suspend_runtime, amd_resume_runtime, NULL)
>>  };
>>

On 11/01/23 21:19, Pierre-Louis Bossart wrote:
>
> On 1/11/23 03:02, Vijendar Mukunda wrote:
>> Add start up and shutdown dai ops for AMD Master driver.
> I don't think those startup and shutdown ops are necessary, we removed
> them for Intel, see "soundwire: intel: remove DAI startup/shutdown"
will drop this patch.

On 11/01/23 21:32, Pierre-Louis Bossart wrote:
> On 1/11/23 03:02, Vijendar Mukunda wrote:
>> To avoid ACP entering into D3 state during slave enumeration and
>> initialization on two soundwire controller instances for multiple codecs,
>> increase the runtime suspend delay to 3 seconds.
> You have a parent PCI device and a set of child devices for each
> manager. The parent PCI device cannot suspend before all its children
> are also suspended, so shouldn't the delay be modified at the manager level?
>
> Not getting what this delay is and how this would deal with a lengthy
> enumeration/initialization process.
Yes agreed. Until Child devices are suspended, parent device will
be in D0 state. We will rephrase the commit message.

Machine driver node will be created by ACP PCI driver.
We have added delay in machine driver to make sure
two manager instances completes codec enumeration and
peripheral initialization before registering the sound card.
Without adding delay in machine driver will result early card
registration before codec initialization is completed. Manager
will enter in to bad state due to codec read/write failures.
We are intended to keep the ACP in D0 state, till sound card
is created and jack controls are initialized. To handle, at manager
level increased runtime suspend delay.
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>> ---
>>  sound/soc/amd/ps/acp63.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
>> index 833d0b5aa73d..6c8849f2bcec 100644
>> --- a/sound/soc/amd/ps/acp63.h
>> +++ b/sound/soc/amd/ps/acp63.h
>> @@ -51,7 +51,7 @@
>>  #define MIN_BUFFER MAX_BUFFER
>>  
>>  /* time in ms for runtime suspend delay */
>> -#define ACP_SUSPEND_DELAY_MS	2000
>> +#define ACP_SUSPEND_DELAY_MS	3000
>>  
>>  #define ACP63_DMIC_ADDR		2
>>  #define ACP63_PDM_MODE_DEVS		3

>>> +static int __maybe_unused amd_pm_prepare(struct device *dev)
>>> +{
>>> +	struct amd_sdwc_ctrl *ctrl = dev_get_drvdata(dev);
>>> +	struct sdw_bus *bus = &ctrl->bus;
>>> +	int ret;
>>> +
>>> +	if (bus->prop.hw_disabled || !ctrl->startup_done) {
>>> +		dev_dbg(bus->dev, "SoundWire master %d is disabled or not-started, ignoring\n",
>>> +			bus->link_id);
>>> +		return 0;
>>> +	}
>>> +	ret = device_for_each_child(bus->dev, NULL, amd_resume_child_device);
>>> +	if (ret < 0)
>>> +		dev_err(dev, "%s: amd_resume_child_device failed: %d\n", __func__, ret);
>>> +	if (pm_runtime_suspended(dev) && ctrl->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
>>> +		ret = pm_request_resume(dev);
>>> +		if (ret < 0) {
>>> +			dev_err(bus->dev, "pm_request_resume failed: %d\n", ret);
>>> +			return 0;
>>> +		}
>>> +	}
>>> +	return 0;
>>> +}
>> This seems to be inspired by the Intel code, but is this necessary here?
> No It's not inspired by intel code. Initially, we haven't included
> pm_prepare callback. We have observed issues without
> pm_prepare callback.
>> For Intel, we saw cases where we had to pm_resume before doing a system
>> suspend, otherwise the hardware was in a bad state.
>>
>> Do you actually need to do so, or is is possible to do a system suspend
>> when the clock is stopped.
>>
>> And in the case where the bus is in 'power-off' mode, do you actually
>> need to resume at all?
> Our platform supports different power modes. To support all
> combinations, we have included pm_prepare callback.

>> do you actually need to stop the clock before powering-off? This seems
>> counter intuitive and not so useful?
> Yes, as per our design, we need to stop the clock
> before powering off.

It'd be good to add comments capturing these points, that would be
useful for new contributors and reviewers to know this is intentional
and required by the hardware programming sequences.

On 1/12/23 05:02, Mukunda,Vijendar wrote:
> On 11/01/23 21:32, Pierre-Louis Bossart wrote:
>> On 1/11/23 03:02, Vijendar Mukunda wrote:
>>> To avoid ACP entering into D3 state during slave enumeration and
>>> initialization on two soundwire controller instances for multiple codecs,
>>> increase the runtime suspend delay to 3 seconds.
>> You have a parent PCI device and a set of child devices for each
>> manager. The parent PCI device cannot suspend before all its children
>> are also suspended, so shouldn't the delay be modified at the manager level?
>>
>> Not getting what this delay is and how this would deal with a lengthy
>> enumeration/initialization process.
> Yes agreed. Until Child devices are suspended, parent device will
> be in D0 state. We will rephrase the commit message.
> 
> Machine driver node will be created by ACP PCI driver.
> We have added delay in machine driver to make sure
> two manager instances completes codec enumeration and
> peripheral initialization before registering the sound card.
> Without adding delay in machine driver will result early card
> registration before codec initialization is completed. Manager
> will enter in to bad state due to codec read/write failures.
> We are intended to keep the ACP in D0 state, till sound card
> is created and jack controls are initialized. To handle, at manager
> level increased runtime suspend delay.

This doesn't look too good. You should not assume any timing
dependencies in the machine driver probe. I made that mistake in earlier
versions and we had to revisit all this to make sure drivers could be
bound/unbound at any time.

On 1/12/2023 08:54, Pierre-Louis Bossart wrote:
> 
> 
> On 1/12/23 05:02, Mukunda,Vijendar wrote:
>> On 11/01/23 21:32, Pierre-Louis Bossart wrote:
>>> On 1/11/23 03:02, Vijendar Mukunda wrote:
>>>> To avoid ACP entering into D3 state during slave enumeration and
>>>> initialization on two soundwire controller instances for multiple codecs,
>>>> increase the runtime suspend delay to 3 seconds.
>>> You have a parent PCI device and a set of child devices for each
>>> manager. The parent PCI device cannot suspend before all its children
>>> are also suspended, so shouldn't the delay be modified at the manager level?
>>>
>>> Not getting what this delay is and how this would deal with a lengthy
>>> enumeration/initialization process.
>> Yes agreed. Until Child devices are suspended, parent device will
>> be in D0 state. We will rephrase the commit message.
>>
>> Machine driver node will be created by ACP PCI driver.
>> We have added delay in machine driver to make sure
>> two manager instances completes codec enumeration and
>> peripheral initialization before registering the sound card.
>> Without adding delay in machine driver will result early card
>> registration before codec initialization is completed. Manager
>> will enter in to bad state due to codec read/write failures.
>> We are intended to keep the ACP in D0 state, till sound card
>> is created and jack controls are initialized. To handle, at manager
>> level increased runtime suspend delay.
> 
> This doesn't look too good. You should not assume any timing
> dependencies in the machine driver probe. I made that mistake in earlier
> versions and we had to revisit all this to make sure drivers could be
> bound/unbound at any time.

Rather than a timing dependency, could you perhaps prohibit runtime PM 
and have a codec make a callback to indicate it's fully initialized and 
then allow runtime PM again?

On 1/12/23 09:29, Limonciello, Mario wrote:
> On 1/12/2023 08:54, Pierre-Louis Bossart wrote:
>>
>>
>> On 1/12/23 05:02, Mukunda,Vijendar wrote:
>>> On 11/01/23 21:32, Pierre-Louis Bossart wrote:
>>>> On 1/11/23 03:02, Vijendar Mukunda wrote:
>>>>> To avoid ACP entering into D3 state during slave enumeration and
>>>>> initialization on two soundwire controller instances for multiple
>>>>> codecs,
>>>>> increase the runtime suspend delay to 3 seconds.
>>>> You have a parent PCI device and a set of child devices for each
>>>> manager. The parent PCI device cannot suspend before all its children
>>>> are also suspended, so shouldn't the delay be modified at the
>>>> manager level?
>>>>
>>>> Not getting what this delay is and how this would deal with a lengthy
>>>> enumeration/initialization process.
>>> Yes agreed. Until Child devices are suspended, parent device will
>>> be in D0 state. We will rephrase the commit message.
>>>
>>> Machine driver node will be created by ACP PCI driver.
>>> We have added delay in machine driver to make sure
>>> two manager instances completes codec enumeration and
>>> peripheral initialization before registering the sound card.
>>> Without adding delay in machine driver will result early card
>>> registration before codec initialization is completed. Manager
>>> will enter in to bad state due to codec read/write failures.
>>> We are intended to keep the ACP in D0 state, till sound card
>>> is created and jack controls are initialized. To handle, at manager
>>> level increased runtime suspend delay.
>>
>> This doesn't look too good. You should not assume any timing
>> dependencies in the machine driver probe. I made that mistake in earlier
>> versions and we had to revisit all this to make sure drivers could be
>> bound/unbound at any time.
> 
> Rather than a timing dependency, could you perhaps prohibit runtime PM
> and have a codec make a callback to indicate it's fully initialized and
> then allow runtime PM again?

We already have enumeration and initialization 'struct completion' that
are used by codec drivers to know if the hardware is usable. We also
have pm_runtime_get_sync() is the bus layer to make sure the codec is
resumed before being accessed.

The explanations above confuse card registration and manager
probe/initialization. These are two different things. Maybe there's
indeed a missing part in the SoundWire PM assumptions, but I am not
getting what the issue is.

On 12/01/23 21:35, Pierre-Louis Bossart wrote:
>
> On 1/12/23 09:29, Limonciello, Mario wrote:
>> On 1/12/2023 08:54, Pierre-Louis Bossart wrote:
>>>
>>> On 1/12/23 05:02, Mukunda,Vijendar wrote:
>>>> On 11/01/23 21:32, Pierre-Louis Bossart wrote:
>>>>> On 1/11/23 03:02, Vijendar Mukunda wrote:
>>>>>> To avoid ACP entering into D3 state during slave enumeration and
>>>>>> initialization on two soundwire controller instances for multiple
>>>>>> codecs,
>>>>>> increase the runtime suspend delay to 3 seconds.
>>>>> You have a parent PCI device and a set of child devices for each
>>>>> manager. The parent PCI device cannot suspend before all its children
>>>>> are also suspended, so shouldn't the delay be modified at the
>>>>> manager level?
>>>>>
>>>>> Not getting what this delay is and how this would deal with a lengthy
>>>>> enumeration/initialization process.
>>>> Yes agreed. Until Child devices are suspended, parent device will
>>>> be in D0 state. We will rephrase the commit message.
>>>>
>>>> Machine driver node will be created by ACP PCI driver.
>>>> We have added delay in machine driver to make sure
>>>> two manager instances completes codec enumeration and
>>>> peripheral initialization before registering the sound card.
>>>> Without adding delay in machine driver will result early card
>>>> registration before codec initialization is completed. Manager
>>>> will enter in to bad state due to codec read/write failures.
>>>> We are intended to keep the ACP in D0 state, till sound card
>>>> is created and jack controls are initialized. To handle, at manager
>>>> level increased runtime suspend delay.
>>> This doesn't look too good. You should not assume any timing
>>> dependencies in the machine driver probe. I made that mistake in earlier
>>> versions and we had to revisit all this to make sure drivers could be
>>> bound/unbound at any time.
>> Rather than a timing dependency, could you perhaps prohibit runtime PM
>> and have a codec make a callback to indicate it's fully initialized and
>> then allow runtime PM again?
> We already have enumeration and initialization 'struct completion' that
> are used by codec drivers to know if the hardware is usable. We also
> have pm_runtime_get_sync() is the bus layer to make sure the codec is
> resumed before being accessed.
Instead of walking through codec list and checking completion status
for every codec over the link, can we have some solution where once
all codecs gets enumerated and initialized, a variable in bus instance
will be updated to know all peripherals initialized. So that we can
check this variable in machine driver.

>
> The explanations above confuse card registration and manager
> probe/initialization. These are two different things. Maybe there's
> indeed a missing part in the SoundWire PM assumptions, but I am not
> getting what the issue is.
We will rephrase the commit message.
At manager level we want to increase the delay to 3s.

On 11/01/23 20:28, Pierre-Louis Bossart wrote:
>
> On 1/11/23 03:02, Vijendar Mukunda wrote:
>> Register dai ops for two controller instances.
> manager instances
will change it.
>
>> diff --git a/drivers/soundwire/amd_master.c b/drivers/soundwire/amd_master.c
>> index 7e1f618254ac..93bffe6ff9e2 100644
>> --- a/drivers/soundwire/amd_master.c
>> +++ b/drivers/soundwire/amd_master.c
>> @@ -952,6 +952,186 @@ static const struct sdw_master_ops amd_sdwc_ops = {
>>  	.read_ping_status = amd_sdwc_read_ping_status,
>>  };
>>  
>> +static int amd_sdwc_hw_params(struct snd_pcm_substream *substream,
>> +			      struct snd_pcm_hw_params *params,
>> +			      struct snd_soc_dai *dai)
>> +{
>> +	struct amd_sdwc_ctrl *ctrl = snd_soc_dai_get_drvdata(dai);
>> +	struct sdw_amd_dma_data *dma;
>> +	struct sdw_stream_config sconfig;
>> +	struct sdw_port_config *pconfig;
>> +	int ch, dir;
>> +	int ret;
>> +
>> +	dma = snd_soc_dai_get_dma_data(dai, substream);
>> +	if (!dma)
>> +		return -EIO;
>> +
>> +	ch = params_channels(params);
>> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
>> +		dir = SDW_DATA_DIR_RX;
>> +	else
>> +		dir = SDW_DATA_DIR_TX;
>> +	dev_dbg(ctrl->dev, "%s: dir:%d dai->id:0x%x\n", __func__, dir, dai->id);
>> +	dma->hw_params = params;
>> +
>> +	sconfig.direction = dir;
>> +	sconfig.ch_count = ch;
>> +	sconfig.frame_rate = params_rate(params);
>> +	sconfig.type = dma->stream_type;
>> +
>> +	sconfig.bps = snd_pcm_format_width(params_format(params));
>> +
>> +	/* Port configuration */
>> +	pconfig = kzalloc(sizeof(*pconfig), GFP_KERNEL);
>> +	if (!pconfig) {
>> +		ret =  -ENOMEM;
>> +		goto error;
>> +	}
>> +
>> +	pconfig->num = dai->id;
>> +	pconfig->ch_mask = (1 << ch) - 1;
>> +	ret = sdw_stream_add_master(&ctrl->bus, &sconfig,
>> +				    pconfig, 1, dma->stream);
>> +	if (ret)
>> +		dev_err(ctrl->dev, "add master to stream failed:%d\n", ret);
>> +
>> +	kfree(pconfig);
>> +error:
>> +	return ret;
>> +}
> This looks inspired from intel.c, but you are not programming ANY
> registers here. is this intentional?
We don't have any additional registers to be programmed like intel.
>
>> +static int amd_sdwc_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
>> +{
>> +	struct amd_sdwc_ctrl *ctrl = snd_soc_dai_get_drvdata(dai);
>> +	struct sdw_amd_dma_data *dma;
>> +	int ret;
>> +
>> +	dma = snd_soc_dai_get_dma_data(dai, substream);
>> +	if (!dma)
>> +		return -EIO;
>> +
>> +	ret = sdw_stream_remove_master(&ctrl->bus, dma->stream);
>> +	if (ret < 0) {
>> +		dev_err(dai->dev, "remove master from stream %s failed: %d\n",
>> +			dma->stream->name, ret);
>> +		return ret;
>> +	}
>> +	dma->hw_params = NULL;
>> +	return 0;
>> +}
>> +
>> +static int amd_set_sdw_stream(struct snd_soc_dai *dai, void *stream, int direction)
>> +{
>> +	struct amd_sdwc_ctrl *ctrl = snd_soc_dai_get_drvdata(dai);
>> +	struct sdw_amd_dma_data *dma;
> you want to avoid using dma_data and use your own runtime. We made that
> change recently for cadence_runtime.c
>
will check the implementation.
>> +
>> +	if (stream) {

>> +		if (direction == SNDRV_PCM_STREAM_PLAYBACK)
>> +			dma = dai->playback_dma_data;
>> +		else
>> +			dma = dai->capture_dma_data;
>> +
>> +		if (dma) {
>> +			dev_err(dai->dev,
>> +				"dma_data already allocated for dai %s\n",
>> +				dai->name);
>> +			return -EINVAL;
>> +		}
>> +
>> +		/* allocate and set dma info */
>> +		dma = kzalloc(sizeof(*dma), GFP_KERNEL);
>> +		if (!dma)
>> +			return -ENOMEM;
>> +		dma->stream_type = SDW_STREAM_PCM;
>> +		dma->bus = &ctrl->bus;
>> +		dma->link_id = ctrl->instance;
>> +		dma->stream = stream;
>> +
>> +		if (direction == SNDRV_PCM_STREAM_PLAYBACK)
>> +			dai->playback_dma_data = dma;
>> +		else
>> +			dai->capture_dma_data = dma;
>> +	} else {
>> +		if (direction == SNDRV_PCM_STREAM_PLAYBACK) {
>> +			kfree(dai->playback_dma_data);
>> +			dai->playback_dma_data = NULL;
>> +		} else {
>> +			kfree(dai->capture_dma_data);
>> +			dai->capture_dma_data = NULL;
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int amd_pcm_set_sdw_stream(struct snd_soc_dai *dai, void *stream, int direction)
>> +{
>> +	return amd_set_sdw_stream(dai, stream, direction);
>> +}
>> +
>> +static void *amd_get_sdw_stream(struct snd_soc_dai *dai, int direction)
>> +{
>> +	struct sdw_amd_dma_data *dma;
>> +
>> +	if (direction == SNDRV_PCM_STREAM_PLAYBACK)
>> +		dma = dai->playback_dma_data;
>> +	else
>> +		dma = dai->capture_dma_data;
>> +
>> +	if (!dma)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	return dma->stream;
>> +}
>> +
>> +static const struct snd_soc_dai_ops amd_sdwc_dai_ops = {
>> +	.hw_params = amd_sdwc_hw_params,
>> +	.hw_free = amd_sdwc_hw_free,
>> +	.set_stream = amd_pcm_set_sdw_stream,
> In the first patch there was support for PDM exposed, but here it's PDM
> only?
Didn't get your question.
First patch talks about creating dev nodes for Soundwire managers and
ACP PDM controller based on ACP pin config.

Let us know if we are missing anything?
>
>> +	.get_stream = amd_get_sdw_stream,
>> +};
>> +
>> +static const struct snd_soc_component_driver amd_sdwc_dai_component = {
>> +	.name = "soundwire",
>> +};
>> +
>> +static int amd_sdwc_register_dais(struct amd_sdwc_ctrl *ctrl)
>> +{
>> +	struct snd_soc_dai_driver *dais;
>> +	struct snd_soc_pcm_stream *stream;
>> +	struct device *dev;
>> +	int i, num_dais;
>> +
>> +	dev = ctrl->dev;
>> +	num_dais = ctrl->num_dout_ports + ctrl->num_din_ports;
>> +	dais = devm_kcalloc(dev, num_dais, sizeof(*dais), GFP_KERNEL);
>> +	if (!dais)
>> +		return -ENOMEM;
>> +	for (i = 0; i < num_dais; i++) {
>> +		dais[i].name = devm_kasprintf(dev, GFP_KERNEL, "SDW%d Pin%d", ctrl->instance, i);
>> +		if (!dais[i].name) {
>> +			dev_err(ctrl->dev, "-ENOMEM dai name allocation failed\n");
> remove, we don't add error logs on memory allocation issues.
>
>> +			return -ENOMEM;
>> +		}
>> +
>> +		if (i < ctrl->num_dout_ports)
>> +			stream = &dais[i].playback;
>> +		else
>> +			stream = &dais[i].capture;
>> +
>> +		stream->channels_min = 2;
>> +		stream->channels_max = 2;
> Is this a port limitation or just a software definition?
>
>> +		stream->rates = SNDRV_PCM_RATE_48000;
>> +		stream->formats = SNDRV_PCM_FMTBIT_S16_LE;
> Wondering if this is needed. I don't even recall why it's in the Intel
> code, we tested with 32 bit data and 192kHz, that looks unnecessary to
> me unless the hardware is really limited to those values.
>
>> +
>> +		dais[i].ops = &amd_sdwc_dai_ops;
>> +		dais[i].id = i;
>> +	}
>> +
>> +	return devm_snd_soc_register_component(ctrl->dev, &amd_sdwc_dai_component,
>> +					       dais, num_dais);
>> +}
>> +
>>  static void amd_sdwc_probe_work(struct work_struct *work)
>>  {
>>  	struct amd_sdwc_ctrl *ctrl  = container_of(work, struct amd_sdwc_ctrl, probe_work);
>> @@ -1043,6 +1223,12 @@ static int amd_sdwc_probe(struct platform_device *pdev)
>>  			ret);
>>  		return ret;
>>  	}
>> +	ret = amd_sdwc_register_dais(ctrl);
>> +	if (ret) {
>> +		dev_err(dev, "CPU DAI registration failed\n");
>> +		sdw_bus_master_delete(&ctrl->bus);
>> +		return ret;
>> +	}
>>  	INIT_WORK(&ctrl->probe_work, amd_sdwc_probe_work);
>>  	schedule_work(&ctrl->probe_work);
>>  	return 0;
>> diff --git a/include/linux/soundwire/sdw_amd.h b/include/linux/soundwire/sdw_amd.h
>> index 5ec39f8c2f2e..7a99d782969f 100644
>> --- a/include/linux/soundwire/sdw_amd.h
>> +++ b/include/linux/soundwire/sdw_amd.h
>> @@ -13,6 +13,7 @@
>>  #define ACP_SDW0	0
>>  #define ACP_SDW1	1
>>  #define ACP_SDW0_MAX_DAI	6
>> +#define AMD_SDW_MAX_DAIS	8
> How does this work? 6 dais for the first master and 2 for the second?
>
>>  
>>  struct acp_sdw_pdata {
>>  	u16 instance;
>> @@ -25,6 +26,7 @@ struct amd_sdwc_ctrl {
>>  	void __iomem *mmio;
>>  	struct work_struct probe_work;
>>  	struct mutex *sdw_lock;
>> +	struct sdw_stream_runtime *sruntime[AMD_SDW_MAX_DAIS];
> well no, a stream runtime needs to be allocated per stream and usually
> there's a 1:1 mapping between dailink and stream. A stream may use
> multiple DAIs, possibly on different masters - just like a dailink can
> rely on multiple cpu- and codec-dais.
>
> You are conflating/confusing concepts I am afraid here.
>
>>  	int num_din_ports;
>>  	int num_dout_ports;
>>  	int cols_index;
>> @@ -36,4 +38,23 @@ struct amd_sdwc_ctrl {
>>  	bool startup_done;
>>  	u32 power_mode_mask;
>>  };
>> +
>> +/**
>> + * struct sdw_amd_dma_data: AMD DMA data
>> + *
>> + * @name: SoundWire stream name
>> + * @stream: stream runtime
>> + * @bus: Bus handle
>> + * @stream_type: Stream type
>> + * @link_id: Master link id
>> + * @hw_params: hw_params to be applied in .prepare step
>> + */
>> +struct sdw_amd_dma_data {
>> +	char *name;
>> +	struct sdw_stream_runtime *stream;
>> +	struct sdw_bus *bus;
>> +	enum sdw_stream_type stream_type;
>> +	int link_id;
>> +	struct snd_pcm_hw_params *hw_params;
>> +};
>>  #endif

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.

>>> +static int amd_sdwc_hw_params(struct snd_pcm_substream *substream,
>>> +			      struct snd_pcm_hw_params *params,
>>> +			      struct snd_soc_dai *dai)
>>> +{
>>> +	struct amd_sdwc_ctrl *ctrl = snd_soc_dai_get_drvdata(dai);
>>> +	struct sdw_amd_dma_data *dma;
>>> +	struct sdw_stream_config sconfig;
>>> +	struct sdw_port_config *pconfig;
>>> +	int ch, dir;
>>> +	int ret;
>>> +
>>> +	dma = snd_soc_dai_get_dma_data(dai, substream);
>>> +	if (!dma)
>>> +		return -EIO;
>>> +
>>> +	ch = params_channels(params);
>>> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
>>> +		dir = SDW_DATA_DIR_RX;
>>> +	else
>>> +		dir = SDW_DATA_DIR_TX;
>>> +	dev_dbg(ctrl->dev, "%s: dir:%d dai->id:0x%x\n", __func__, dir, dai->id);
>>> +	dma->hw_params = params;
>>> +
>>> +	sconfig.direction = dir;
>>> +	sconfig.ch_count = ch;
>>> +	sconfig.frame_rate = params_rate(params);
>>> +	sconfig.type = dma->stream_type;
>>> +
>>> +	sconfig.bps = snd_pcm_format_width(params_format(params));
>>> +
>>> +	/* Port configuration */
>>> +	pconfig = kzalloc(sizeof(*pconfig), GFP_KERNEL);
>>> +	if (!pconfig) {
>>> +		ret =  -ENOMEM;
>>> +		goto error;
>>> +	}
>>> +
>>> +	pconfig->num = dai->id;
>>> +	pconfig->ch_mask = (1 << ch) - 1;
>>> +	ret = sdw_stream_add_master(&ctrl->bus, &sconfig,
>>> +				    pconfig, 1, dma->stream);
>>> +	if (ret)
>>> +		dev_err(ctrl->dev, "add master to stream failed:%d\n", ret);
>>> +
>>> +	kfree(pconfig);
>>> +error:
>>> +	return ret;
>>> +}
>> This looks inspired from intel.c, but you are not programming ANY
>> registers here. is this intentional?
> We don't have any additional registers to be programmed like intel.

ok, this is worthy of a comment.


>>> +static const struct snd_soc_dai_ops amd_sdwc_dai_ops = {
>>> +	.hw_params = amd_sdwc_hw_params,
>>> +	.hw_free = amd_sdwc_hw_free,
>>> +	.set_stream = amd_pcm_set_sdw_stream,
>> In the first patch there was support for PDM exposed, but here it's PDM
>> only?
> Didn't get your question.
> First patch talks about creating dev nodes for Soundwire managers and
> ACP PDM controller based on ACP pin config.

Sorry, my comment has a typo.

I meant that the first patch exposed PDM support but here you only have PCM?

>>>>>>> increase the runtime suspend delay to 3 seconds.
>>>>>> You have a parent PCI device and a set of child devices for each
>>>>>> manager. The parent PCI device cannot suspend before all its children
>>>>>> are also suspended, so shouldn't the delay be modified at the
>>>>>> manager level?
>>>>>>
>>>>>> Not getting what this delay is and how this would deal with a lengthy
>>>>>> enumeration/initialization process.
>>>>> Yes agreed. Until Child devices are suspended, parent device will
>>>>> be in D0 state. We will rephrase the commit message.
>>>>>
>>>>> Machine driver node will be created by ACP PCI driver.
>>>>> We have added delay in machine driver to make sure
>>>>> two manager instances completes codec enumeration and
>>>>> peripheral initialization before registering the sound card.
>>>>> Without adding delay in machine driver will result early card
>>>>> registration before codec initialization is completed. Manager
>>>>> will enter in to bad state due to codec read/write failures.
>>>>> We are intended to keep the ACP in D0 state, till sound card
>>>>> is created and jack controls are initialized. To handle, at manager
>>>>> level increased runtime suspend delay.
>>>> This doesn't look too good. You should not assume any timing
>>>> dependencies in the machine driver probe. I made that mistake in earlier
>>>> versions and we had to revisit all this to make sure drivers could be
>>>> bound/unbound at any time.
>>> Rather than a timing dependency, could you perhaps prohibit runtime PM
>>> and have a codec make a callback to indicate it's fully initialized and
>>> then allow runtime PM again?
>> We already have enumeration and initialization 'struct completion' that
>> are used by codec drivers to know if the hardware is usable. We also
>> have pm_runtime_get_sync() is the bus layer to make sure the codec is
>> resumed before being accessed.
> Instead of walking through codec list and checking completion status
> for every codec over the link, can we have some solution where once
> all codecs gets enumerated and initialized, a variable in bus instance
> will be updated to know all peripherals initialized. So that we can
> check this variable in machine driver.

No, because the bus cannot know for sure what codecs to expect on the
platform.

This comes from the design, we first create a bunch of devices based on
ACPI information, which causes the drivers to probe. Then when the bus
starts, codecs that are physically present on the bus will attach and be
initialized in the update_status callback.

It's perfectly acceptable for devices to be exposed in ACPI and not be
present on a board. The bus wouldn't know what is needed.

I am still not clear on what the "early card registration" issue might be.

Can you clarify which codec registers are accessed in that case, are
those supposed to be managed with regmap? one possibility is that we
need to make sure the codec drivers are in regmap cache_only probe at
the probe time, that may prevent this sort of uncontrolled register
access. I had a PR on this that I haven't touched in a while, see [1]

I do recall some issues with the codec jacks, where if the card
registration happens too late the codec might have suspended. But we
added pm_runtime_resume_and_get in the set_jack_detect callbacks, so
that was solved.

[1] https://github.com/thesofproject/linux/pull/3941

On Fri, Jan 13, 2023 at 11:33:09AM -0600, Pierre-Louis Bossart wrote:

> I do recall some issues with the codec jacks, where if the card
> registration happens too late the codec might have suspended. But we
> added pm_runtime_resume_and_get in the set_jack_detect callbacks, so
> that was solved.

Right, I would expect that whatever needs the device to be powered on
would be explicitly ensuring that this is done rather than tweaking
timeouts - the timeouts should be more of a performance thing to avoid
bouncing power too much, not a correctness thing.

On 1/16/23 02:35, Mukunda,Vijendar wrote:
> On 14/01/23 01:27, Mark Brown wrote:
>> On Fri, Jan 13, 2023 at 11:33:09AM -0600, Pierre-Louis Bossart wrote:
>>
>>> I do recall some issues with the codec jacks, where if the card
>>> registration happens too late the codec might have suspended. But we
>>> added pm_runtime_resume_and_get in the set_jack_detect callbacks, so
>>> that was solved.
>> Right, I would expect that whatever needs the device to be powered on
>> would be explicitly ensuring that this is done rather than tweaking
>> timeouts - the timeouts should be more of a performance thing to avoid
>> bouncing power too much, not a correctness thing.
> Machine driver probe is executed in parallel with Manager driver
> probe sequence. Because of it, before completion of all peripherals
> enumeration across the multiple links, if card registration is
> completed, codec register writes will fail as Codec device numbers
> are not assigned.
> 
> If we understood correctly, as per your suggestion, We shouldn't use any
> time bounds in machine driver probe sequence and before registering the
> sound card, need to traverses through all peripheral initialization completion
> status for all the managers.

What's not clear in your reply is this:

What codec registers are accessed as a result of the machine driver
probe and card registration, and in what part of the card registration?

Are we talking about SoundWire 'standard' registers for device/port
management, about vendor specific ones that are exposed to userspace, or
vendor-specific ones entirely configured by the driver/regmap.

You've got to give us more data or understanding of the sequence to
help. Saying there's a race condition doesn't really help if there's
nothing that explains what codec registers are accessed and when.

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 <pierre-louis.bossart@linux.intel.com>
> Sent: Tuesday, January 31, 2023 10:01
> To: Mukunda, Vijendar <Vijendar.Mukunda@amd.com>;
> broonie@kernel.org; vkoul@kernel.org; alsa-devel@alsa-project.org
> Cc: Katragadda, Mastan <Mastan.Katragadda@amd.com>; Dommati, Sunil-
> kumar <Sunil-kumar.Dommati@amd.com>; open list <linux-
> kernel@vger.kernel.org>; Hiregoudar, Basavaraj
> <Basavaraj.Hiregoudar@amd.com>; Takashi Iwai <tiwai@suse.com>; Liam
> Girdwood <lgirdwood@gmail.com>; Nathan Chancellor
> <nathan@kernel.org>; Limonciello, Mario <Mario.Limonciello@amd.com>;
> kondaveeti, Arungopal <Arungopal.kondaveeti@amd.com>; Sanyog Kale
> <sanyog.r.kale@intel.com>; Bard Liao <yung-chuan.liao@linux.intel.com>;
> Saba Kareem, Syed <Syed.SabaKareem@amd.com>
> 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.