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
