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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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



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