Re: [PATCH V2 03/13] drivers: soundwire: amd: refactor amd soundwire manager device node creation

[Mukunda,Vijendar]

On 22/01/24 13:27, Vinod Koul wrote:
> On 10-01-24, 15:14, Vijendar Mukunda wrote:
>
> Same comment on patch title
will fix it.
>
>> Refactor amd SoundWire manager device node creation logic and implement generic
>> functions to have a common functionality for SoundWire manager platform device
>> creation, start and exit sequence for both legacy(NO DSP) and SOF stack for AMD
>> platforms. These functions will be invoked from legacy and SOF stack.
>>
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@xxxxxxx>
>> ---
>>  drivers/soundwire/Makefile        |   2 +-
>>  drivers/soundwire/amd_init.c      | 147 ++++++++++++++++++++++++++++++
>>  drivers/soundwire/amd_init.h      |  13 +++
>>  drivers/soundwire/amd_manager.c   |  16 +---
>>  include/linux/soundwire/sdw_amd.h |  54 ++++++++++-
>>  5 files changed, 218 insertions(+), 14 deletions(-)
>>  create mode 100644 drivers/soundwire/amd_init.c
>>  create mode 100644 drivers/soundwire/amd_init.h
>>
>> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
>> index 657f5888a77b..e80a2c2cf3e7 100644
>> --- a/drivers/soundwire/Makefile
>> +++ b/drivers/soundwire/Makefile
>> @@ -20,7 +20,7 @@ soundwire-bus-y += irq.o
>>  endif
>>  
>>  #AMD driver
>> -soundwire-amd-y :=	amd_manager.o
>> +soundwire-amd-y := amd_init.o amd_manager.o
>>  obj-$(CONFIG_SOUNDWIRE_AMD) += soundwire-amd.o
>>  
>>  #Cadence Objs
>> diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
>> new file mode 100644
>> index 000000000000..d732ab0bfd59
>> --- /dev/null
>> +++ b/drivers/soundwire/amd_init.c
>> @@ -0,0 +1,147 @@
>> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
>> +/*
>> + * SoundWire AMD Manager Initialize routines
>> + *
>> + * Initializes and creates SDW devices based on ACPI and Hardware values
>> + *
>> + * Copyright 2023 Advanced Micro Devices, Inc.
>  2023-24..?
will update.
>
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/export.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include "amd_init.h"
>> +
>> +static int sdw_amd_cleanup(struct sdw_amd_ctx *ctx)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < ctx->count; i++) {
>> +		if (!(ctx->link_mask & BIT(i)))
>> +			continue;
>> +		platform_device_unregister(ctx->pdev[i]);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct sdw_amd_ctx *sdw_amd_probe_controller(struct sdw_amd_res *res)
>> +{
>> +	struct sdw_amd_ctx *ctx;
>> +	struct acpi_device *adev;
>> +	struct resource *sdw_res;
>> +	struct acp_sdw_pdata sdw_pdata[2];
>> +	struct platform_device_info pdevinfo[2];
>> +	u32 link_mask;
>> +	int count, index;
>> +
>> +	if (!res)
>> +		return NULL;
>> +
>> +	adev = acpi_fetch_acpi_dev(res->handle);
>> +	if (!adev)
>> +		return NULL;
>> +
>> +	if (!res->count)
>> +		return NULL;
>> +
>> +	count = res->count;
>> +	dev_dbg(&adev->dev, "Creating %d SDW Link devices\n", count);
>> +
>> +	/*
>> +	 * we need to alloc/free memory manually and can't use devm:
>> +	 * this routine may be called from a workqueue, and not from
>> +	 * the parent .probe.
>> +	 * If devm_ was used, the memory might never be freed on errors.
>> +	 */
>> +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>> +	if (!ctx)
>> +		return NULL;
>> +
>> +	ctx->count = count;
>> +	ctx->link_mask = res->link_mask;
>> +	sdw_res = kzalloc(sizeof(*sdw_res), GFP_KERNEL);
>> +	if (!sdw_res) {
>> +		kfree(ctx);
>> +		return NULL;
>> +	}
>> +	sdw_res->flags = IORESOURCE_MEM;
>> +	sdw_res->start = res->addr;
>> +	sdw_res->end = res->addr + res->reg_range;
>> +	memset(&pdevinfo, 0, sizeof(pdevinfo));
>> +	link_mask = ctx->link_mask;
>> +	for (index = 0; index < count; index++) {
>> +		if (!(link_mask & BIT(index)))
>> +			continue;
>> +
>> +		sdw_pdata[index].instance = index;
>> +		sdw_pdata[index].acp_sdw_lock = res->acp_lock;
>> +		pdevinfo[index].name = "amd_sdw_manager";
>> +		pdevinfo[index].id = index;
>> +		pdevinfo[index].parent = res->parent;
>> +		pdevinfo[index].num_res = 1;
>> +		pdevinfo[index].res = sdw_res;
>> +		pdevinfo[index].data = &sdw_pdata[index];
>> +		pdevinfo[index].size_data = sizeof(struct acp_sdw_pdata);
>> +		pdevinfo[index].fwnode = acpi_fwnode_handle(adev);
>> +		ctx->pdev[index] = platform_device_register_full(&pdevinfo[index]);
>> +		if (IS_ERR(ctx->pdev[index]))
>> +			goto err;
>> +	}
>> +	kfree(sdw_res);
>> +	return ctx;
>> +err:
>> +	while (index--) {
>> +		if (!(link_mask & BIT(index)))
>> +			continue;
>> +
>> +		platform_device_unregister(ctx->pdev[index]);
>> +	}
>> +
>> +	kfree(sdw_res);
>> +	kfree(ctx);
>> +	return NULL;
>> +}
>> +
>> +static int sdw_amd_startup(struct sdw_amd_ctx *ctx)
>> +{
>> +	struct amd_sdw_manager *amd_manager;
>> +	int i, ret;
>> +
>> +	/* Startup SDW Manager devices */
>> +	for (i = 0; i < ctx->count; i++) {
>> +		if (!(ctx->link_mask & BIT(i)))
>> +			continue;
>> +		amd_manager = dev_get_drvdata(&ctx->pdev[i]->dev);
>> +		ret = amd_sdw_manager_start(amd_manager);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int sdw_amd_probe(struct sdw_amd_res *res, struct sdw_amd_ctx **sdw_ctx)
>> +{
>> +	*sdw_ctx = sdw_amd_probe_controller(res);
>> +	if (!*sdw_ctx)
>> +		return -ENODEV;
>> +
>> +	return sdw_amd_startup(*sdw_ctx);
>> +}
>> +EXPORT_SYMBOL_NS(sdw_amd_probe, SOUNDWIRE_AMD_INIT);
>> +
>> +void sdw_amd_exit(struct sdw_amd_ctx *ctx)
>> +{
>> +	sdw_amd_cleanup(ctx);
>> +	kfree(ctx->ids);
>> +	kfree(ctx);
>> +}
>> +EXPORT_SYMBOL_NS(sdw_amd_exit, SOUNDWIRE_AMD_INIT);
>> +
>> +MODULE_AUTHOR("Vijendar.Mukunda@xxxxxxx");
>> +MODULE_DESCRIPTION("AMD SoundWire Init Library");
>> +MODULE_LICENSE("Dual BSD/GPL");
>> diff --git a/drivers/soundwire/amd_init.h b/drivers/soundwire/amd_init.h
>> new file mode 100644
>> index 000000000000..f710703ffae9
>> --- /dev/null
>> +++ b/drivers/soundwire/amd_init.h
>> @@ -0,0 +1,13 @@
>> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
>> +/*
>> + * Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.
>> + */
>> +
>> +#ifndef __AMD_INIT_H
>> +#define __AMD_INIT_H
>> +
>> +#include <linux/soundwire/sdw_amd.h>
>> +
>> +int amd_sdw_manager_start(struct amd_sdw_manager *amd_manager);
>> +
>> +#endif
>> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
>> index afa2d83b7e69..0fcf8f8545b1 100644
>> --- a/drivers/soundwire/amd_manager.c
>> +++ b/drivers/soundwire/amd_manager.c
>> @@ -19,6 +19,7 @@
>>  #include <sound/pcm_params.h>
>>  #include <sound/soc.h>
>>  #include "bus.h"
>> +#include "amd_init.h"
>>  #include "amd_manager.h"
>>  
>>  #define DRV_NAME "amd_sdw_manager"
>> @@ -864,10 +865,8 @@ static void amd_sdw_irq_thread(struct work_struct *work)
>>  	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_0TO7);
>>  }
>>  
>> -static void amd_sdw_probe_work(struct work_struct *work)
>> +int amd_sdw_manager_start(struct amd_sdw_manager *amd_manager)
>>  {
>> -	struct amd_sdw_manager *amd_manager = container_of(work, struct amd_sdw_manager,
>> -							   probe_work);
>>  	struct sdw_master_prop *prop;
>>  	int ret;
>>  
>> @@ -876,11 +875,11 @@ static void amd_sdw_probe_work(struct work_struct *work)
>>  		amd_enable_sdw_pads(amd_manager);
>>  		ret = amd_init_sdw_manager(amd_manager);
>>  		if (ret)
>> -			return;
>> +			return ret;
>>  		amd_enable_sdw_interrupts(amd_manager);
>>  		ret = amd_enable_sdw_manager(amd_manager);
>>  		if (ret)
>> -			return;
>> +			return ret;
>>  		amd_sdw_set_frameshape(amd_manager);
>>  	}
>>  	/* Enable runtime PM */
>> @@ -889,6 +888,7 @@ static void amd_sdw_probe_work(struct work_struct *work)
>>  	pm_runtime_mark_last_busy(amd_manager->dev);
>>  	pm_runtime_set_active(amd_manager->dev);
>>  	pm_runtime_enable(amd_manager->dev);
>> +	return 0;
>>  }
>>  
>>  static int amd_sdw_manager_probe(struct platform_device *pdev)
>> @@ -964,11 +964,6 @@ static int amd_sdw_manager_probe(struct platform_device *pdev)
>>  	dev_set_drvdata(dev, amd_manager);
>>  	INIT_WORK(&amd_manager->amd_sdw_irq_thread, amd_sdw_irq_thread);
>>  	INIT_WORK(&amd_manager->amd_sdw_work, amd_sdw_update_slave_status_work);
>> -	INIT_WORK(&amd_manager->probe_work, amd_sdw_probe_work);
>> -	/*
>> -	 * Instead of having lengthy probe sequence, use deferred probe.
>> -	 */
>> -	schedule_work(&amd_manager->probe_work);
>>  	return 0;
>>  }
>>  
>> @@ -978,7 +973,6 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)
>>  	int ret;
>>  
>>  	pm_runtime_disable(&pdev->dev);
>> -	cancel_work_sync(&amd_manager->probe_work);
>>  	amd_disable_sdw_interrupts(amd_manager);
>>  	sdw_bus_master_delete(&amd_manager->bus);
>>  	ret = amd_disable_sdw_manager(amd_manager);
>> diff --git a/include/linux/soundwire/sdw_amd.h b/include/linux/soundwire/sdw_amd.h
>> index 56b4117c087a..1e5ff4c46365 100644
>> --- a/include/linux/soundwire/sdw_amd.h
>> +++ b/include/linux/soundwire/sdw_amd.h
>> @@ -26,6 +26,7 @@
>>  #define AMD_SDW_POWER_OFF_MODE		2
>>  #define ACP_SDW0	0
>>  #define ACP_SDW1	1
>> +#define AMD_SDW_MAX_MANAGER_COUNT	2
>>  
>>  struct acp_sdw_pdata {
>>  	u16 instance;
>> @@ -63,7 +64,6 @@ struct sdw_amd_dai_runtime {
>>   * @reg_mask: register mask structure per manager instance
>>   * @amd_sdw_irq_thread: SoundWire manager irq workqueue
>>   * @amd_sdw_work: peripheral status work queue
>> - * @probe_work: SoundWire manager probe workqueue
>>   * @acp_sdw_lock: mutex to protect acp share register access
>>   * @status: peripheral devices status array
>>   * @num_din_ports: number of input ports
>> @@ -87,7 +87,6 @@ struct amd_sdw_manager {
>>  	struct sdw_manager_reg_mask *reg_mask;
>>  	struct work_struct amd_sdw_irq_thread;
>>  	struct work_struct amd_sdw_work;
>> -	struct work_struct probe_work;
>>  	/* mutex to protect acp common register access */
>>  	struct mutex *acp_sdw_lock;
>>  
>> @@ -120,5 +119,56 @@ struct sdw_amd_acpi_info {
>>  	u32 link_mask;
>>  };
>>  
>> +/**
>> + * struct sdw_amd_ctx - context allocated by the controller
>> + * driver probe
> missing a blank line there
Will add it.
>
>> + * @count: link count
>> + * @num_slaves: total number of devices exposed across all enabled links
> should this be not per link?
No, this will be total count of peripheral devices across all the links.
This variable will be used in  snd_soc_acpi_sdw_link_slaves_found()
API.
>
>> + * @link_mask: bit-wise mask listing SoundWire links reported by the
>> + * Controller
>> + * @ids: array of slave_id, representing Slaves exposed across all enabled
>> + * links
> this does not tell us which device to expect in which link..?
sdw_extended_slave_id structure contains information about link id as well.
>
>> + * @pdev: platform device structure
>> + */
>> +struct sdw_amd_ctx {
>> +	int count;
>> +	int num_slaves;
>> +	u32 link_mask;
>> +	struct sdw_extended_slave_id *ids;
>> +	struct platform_device *pdev[AMD_SDW_MAX_MANAGER_COUNT];
>> +};
>> +
>> +/**
>> + * struct sdw_amd_res - Soundwire AMD global resource structure,
>> + * typically populated by the DSP driver/Legacy driver
>> + *
>> + * @addr: acp pci device resource start address
>> + * @reg_range: ACP register range
>> + * @link_mask: bit-wise mask listing links selected by the DSP driver/
>> + * legacy driver
>> + * @count: link count
>> + * @mmio_base: mmio base of SoundWire registers
>> + * @handle: ACPI parent handle
>> + * @parent: parent device
>> + * @dev: device implementing hwparams and free callbacks
>> + * @acp_lock: mutex protecting acp common registers access
>> + */
>> +struct sdw_amd_res {
>> +	u32 addr;
>> +	u32 reg_range;
>> +	u32 link_mask;
>> +	int count;
>> +	void __iomem *mmio_base;
>> +	acpi_handle handle;
>> +	struct device *parent;
>> +	struct device *dev;
>> +	/* use to protect acp common registers access */
>> +	struct mutex *acp_lock;
>> +};
>> +
>> +int sdw_amd_probe(struct sdw_amd_res *res, struct sdw_amd_ctx **ctx);
>> +
>> +void sdw_amd_exit(struct sdw_amd_ctx *ctx);
>> +
>>  int amd_sdw_scan_controller(struct sdw_amd_acpi_info *info);
>>  #endif
>> -- 
>> 2.34.1