Re: [PATCH 1/9] ASoC: amd: ps: create platform devices based on acp config

From: Pierre-Louis Bossart
Date: Tue May 16 2023 - 11:44:01 EST

Next message: Pierre-Louis Bossart: "Re: [PATCH 2/9] ASoC: amd: ps: handle soundwire interrupts in acp pci driver"
Previous message: Frank Li: "[PATCH v4 0/6] phy: cadence: salvo: some fixes and workarounds"
In reply to: Vijendar Mukunda: "[PATCH 1/9] ASoC: amd: ps: create platform devices based on acp config"
Next in thread: Mukunda,Vijendar: "Re: [PATCH 1/9] ASoC: amd: ps: create platform devices based on acp config"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 5/16/23 05:35, Vijendar Mukunda wrote:
> Create platform devices for Soundwire Manager instances and
> PDM controller based on ACP pin config selection
> and ACPI fw handle for pink sardine platform.
>
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@xxxxxxx>
> ---
> sound/soc/amd/ps/acp63.h | 43 ++++++-
> sound/soc/amd/ps/pci-ps.c | 250 ++++++++++++++++++++++++++++++++++++--
> 2 files changed, 280 insertions(+), 13 deletions(-)
>
> diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
> index 2f94448102d0..f27f71116598 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_MANAGERS 2
>
> /* time in ms for acp timeout */
> #define ACP_TIMEOUT 500
> @@ -80,6 +86,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,
> +};

a comment or kernel-doc wouldn't hurt to explain the difference between
ACP63_PDM_DEV_MASK and ACP63_SDW_PDM_DEV_MASK, the meaning of the 'SDW"
prefix is far from obvious.

> +
> struct pdm_stream_instance {
> u16 num_pages;
> u16 channels;
> @@ -95,14 +107,41 @@ struct pdm_dev_data {
> struct snd_pcm_substream *capture_stream;
> };
>
> +/**
> + * struct acp63_dev_data - acp pci driver context
> + * @acp63_base: acp mmio base
> + * @res: resource
> + * @pdev: child platform device node structures

array of platform devices ?

> + * @acp_lock: used to protect acp common registers
> + * @sdw_fw_node: SoundWire controller fw node handle
> + * @pdev_mask: platform device mask
> + * @pdev_count: platform devices count
> + * @pdm_dev_index: pdm platform device index
> + * @sdw0_dev_index: Soundwire Manager-0 platform device index
> + * @sdw1_dev_index: Soundwire Manager-1 platform device index
> + * @sdw_dma_dev_index: Soundwire DMA controller platform device index
> + * @is_dmic_dev: flag set to true if DMIC device exists

it's not clear what 'DMIC device' refers to here and why it is exposed here.

> + * @is_sdw_dev: flag set to true if Soundwire manager devices exists

same, what does this track? What is the difference between 'dmic dev'
and 'sdw_dev', given that there are references above to SDW_PDM. It's
confusing.

> + * @acp_reset: flag set to true when bus reset is applied across all
> + * the active soundwire manager instances

SoundWire

> + */
> +
> struct acp63_dev_data {
> void __iomem *acp63_base;
> 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_manager_count;
> + u16 sdw0_dev_index;
> + u16 sdw1_dev_index;
> + u16 sdw_dma_dev_index;
> + bool is_dmic_dev;
> + bool is_sdw_dev;
> + bool acp_reset;
> };
>
> int snd_amd_acp_find_config(struct pci_dev *pci);
> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
> index c957718abefc..f3aa08dc05b2 100644
> --- a/sound/soc/amd/ps/pci-ps.c
> +++ b/sound/soc/amd/ps/pci-ps.c
> @@ -6,6 +6,7 @@
> */
>
> #include <linux/pci.h>
> +#include <linux/bitops.h>
> #include <linux/module.h>
> #include <linux/io.h>
> #include <linux/delay.h>
> @@ -15,6 +16,7 @@
> #include <sound/pcm_params.h>
> #include <linux/pm_runtime.h>
> #include <linux/iopoll.h>
> +#include <linux/soundwire/sdw_amd.h>
>
> #include "acp63.h"
>
> @@ -119,12 +121,70 @@ 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];
> + u32 sdw_manager_bitmap;
> + u8 count = 0;
> + u32 acp_sdw_power_mode = 0;
> + int index;
> + int ret;
> +
> + acp_data = dev_get_drvdata(dev);
> + acp_data->acp_reset = true;
> + /* Found controller, find links supported */
> + ret = fwnode_property_read_u32_array((acp_data->sdw_fw_node), "mipi-sdw-manager-list",
> + &sdw_manager_bitmap, 1);
> +
> + if (ret) {
> + dev_err(dev, "Failed to read mipi-sdw-manager-list: %d\n", ret);
> + return -EINVAL;
> + }
> + count = hweight32(sdw_manager_bitmap);
> + /* Check count is within bounds */
> + if (count > AMD_SDW_MAX_MANAGERS) {
> + dev_err(dev, "Manager count %d exceeds max %d\n", count, AMD_SDW_MAX_MANAGERS);
> + return -EINVAL;
> + }
> +
> + if (!count) {
> + dev_dbg(dev, "No SoundWire Managers detected\n");
> + return -EINVAL;
> + }
> + dev_dbg(dev, "ACPI reports %d Soundwire Manager devices\n", count);
> + acp_data->sdw_manager_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, "Manager node %s not found\n", name);
> + return -EIO;
> + }
> +
> + fwnode_property_read_u32(link, "amd-sdw-power-mode",
> + &acp_sdw_power_mode);
> + /*
> + * when soundwire configuration is selected from acp pin config,
> + * based on manager instances count, acp init/de-init sequence should be
> + * executed as part of PM ops only when Bus reset is applied for the active
> + * soundwire manager instances.

try to be consistent and use the MIPI spelling in comments: "SoundWire".

> + */
> + if (acp_sdw_power_mode != AMD_SDW_POWER_OFF_MODE)
> + acp_data->acp_reset = 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;
> const union acpi_object *obj;
> bool is_dmic_dev = false;
> + bool is_sdw_dev = false;
> + int ret;
>
> dmic_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_DMIC_ADDR, 0);
> if (dmic_dev) {
> @@ -134,22 +194,83 @@ 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) {
> + acp_data->sdw_fw_node = acpi_fwnode_handle(sdw_dev);
> + ret = sdw_amd_scan_controller(&pci->dev);
> + if (!ret)
> + is_sdw_dev = true;
> + }
> +
> + dev_dbg(&pci->dev, "Audio Mode %d\n", config);
> switch (config) {
> - case ACP_CONFIG_0:
> - case ACP_CONFIG_1:
> + case ACP_CONFIG_4:
> + case ACP_CONFIG_5:
> + case ACP_CONFIG_10:
> + case ACP_CONFIG_11:
> + if (is_dmic_dev) {
> + acp_data->pdev_mask = ACP63_PDM_DEV_MASK;
> + acp_data->pdev_count = ACP63_PDM_MODE_DEVS;
> + }
> + break;
> case ACP_CONFIG_2:
> case ACP_CONFIG_3:
> - case ACP_CONFIG_9:
> - case ACP_CONFIG_15:
> - dev_dbg(&pci->dev, "Audio Mode %d\n", config);
> + if (is_sdw_dev) {
> + switch (acp_data->sdw_manager_count) {
> + case 1:
> + acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
> + acp_data->pdev_count = ACP63_SDW0_MODE_DEVS;
> + break;
> + case 2:
> + acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
> + acp_data->pdev_count = ACP63_SDW0_SDW1_MODE_DEVS;
> + break;
> + default:
> + return -EINVAL;
> + }
> + }
> break;
> - default:
> - if (is_dmic_dev) {
> + case ACP_CONFIG_6:
> + case ACP_CONFIG_7:
> + case ACP_CONFIG_12:
> + case ACP_CONFIG_8:
> + case ACP_CONFIG_13:
> + case ACP_CONFIG_14:
> + if (is_dmic_dev && is_sdw_dev) {
> + switch (acp_data->sdw_manager_count) {
> + case 1:
> + acp_data->pdev_mask = ACP63_SDW_PDM_DEV_MASK;
> + acp_data->pdev_count = ACP63_SDW0_PDM_MODE_DEVS;
> + break;
> + case 2:
> + acp_data->pdev_mask = ACP63_SDW_PDM_DEV_MASK;
> + acp_data->pdev_count = ACP63_SDW0_SDW1_PDM_MODE_DEVS;
> + break;
> + default:
> + return -EINVAL;
> + }
> + } else if (is_dmic_dev) {
> acp_data->pdev_mask = ACP63_PDM_DEV_MASK;
> acp_data->pdev_count = ACP63_PDM_MODE_DEVS;
> + } else if (is_sdw_dev) {
> + switch (acp_data->sdw_manager_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;

it really wouldn't hurt to have a high-level description of this series
of switch/ifs/else, it's very hard to review.

> }
> + return 0;
> }
>
> static void acp63_fill_platform_dev_info(struct platform_device_info *pdevinfo,
> @@ -173,6 +294,7 @@ static void acp63_fill_platform_dev_info(struct platform_device_info *pdevinfo,
>
> static int create_acp63_platform_devs(struct pci_dev *pci, struct acp63_dev_data *adata, u32 addr)
> {
> + struct acp_sdw_pdata *sdw_pdata;
> struct platform_device_info pdevinfo[ACP63_DEVS];
> struct device *parent;
> int index;
> @@ -205,8 +327,110 @@ static int create_acp63_platform_devs(struct pci_dev *pci, struct acp63_dev_data
> acp63_fill_platform_dev_info(&pdevinfo[2], parent, NULL, "acp_ps_mach",
> 0, NULL, 0, NULL, 0);
> break;
> + case ACP63_SDW_DEV_MASK:
> + if (adata->pdev_count == ACP63_SDW0_MODE_DEVS) {
> + sdw_pdata = devm_kzalloc(&pci->dev, sizeof(struct acp_sdw_pdata),
> + GFP_KERNEL);
> + if (!sdw_pdata) {
> + ret = -ENOMEM;
> + goto de_init;
> + }
> +
> + sdw_pdata->instance = 0;
> + sdw_pdata->acp_sdw_lock = &adata->acp_lock;
> + adata->sdw0_dev_index = 0;
> + adata->sdw_dma_dev_index = 1;
> + acp63_fill_platform_dev_info(&pdevinfo[0], parent, adata->sdw_fw_node,
> + "amd_sdw_manager", 0, adata->res, 1,
> + sdw_pdata, sizeof(struct acp_sdw_pdata));
> + acp63_fill_platform_dev_info(&pdevinfo[1], parent, NULL, "amd_ps_sdw_dma",
> + 0, adata->res, 1, &adata->acp_lock,
> + sizeof(adata->acp_lock));
> + } else if (adata->pdev_count == ACP63_SDW0_SDW1_MODE_DEVS) {
> + sdw_pdata = devm_kzalloc(&pci->dev, sizeof(struct acp_sdw_pdata) * 2,
> + GFP_KERNEL);
> + if (!sdw_pdata) {
> + ret = -ENOMEM;
> + goto de_init;
> + }
> +
> + sdw_pdata[0].instance = 0;
> + sdw_pdata[1].instance = 1;
> + sdw_pdata[0].acp_sdw_lock = &adata->acp_lock;
> + sdw_pdata[1].acp_sdw_lock = &adata->acp_lock;
> + sdw_pdata->acp_sdw_lock = &adata->acp_lock;
> + adata->sdw0_dev_index = 0;
> + adata->sdw1_dev_index = 1;
> + adata->sdw_dma_dev_index = 2;
> + acp63_fill_platform_dev_info(&pdevinfo[0], parent, adata->sdw_fw_node,
> + "amd_sdw_manager", 0, adata->res, 1,
> + &sdw_pdata[0], sizeof(struct acp_sdw_pdata));
> + acp63_fill_platform_dev_info(&pdevinfo[1], parent, adata->sdw_fw_node,
> + "amd_sdw_manager", 1, adata->res, 1,
> + &sdw_pdata[1], sizeof(struct acp_sdw_pdata));
> + acp63_fill_platform_dev_info(&pdevinfo[2], parent, NULL, "amd_ps_sdw_dma",
> + 0, adata->res, 1, &adata->acp_lock,
> + sizeof(adata->acp_lock));
> + }
> + break;
> + case ACP63_SDW_PDM_DEV_MASK:
> + if (adata->pdev_count == ACP63_SDW0_PDM_MODE_DEVS) {
> + sdw_pdata = devm_kzalloc(&pci->dev, sizeof(struct acp_sdw_pdata),
> + GFP_KERNEL);
> + if (!sdw_pdata) {
> + ret = -ENOMEM;
> + goto de_init;
> + }
> +
> + sdw_pdata->instance = 0;
> + sdw_pdata->acp_sdw_lock = &adata->acp_lock;
> + adata->pdm_dev_index = 0;
> + adata->sdw0_dev_index = 1;
> + adata->sdw_dma_dev_index = 2;
> + acp63_fill_platform_dev_info(&pdevinfo[0], parent, NULL, "acp_ps_pdm_dma",
> + 0, adata->res, 1, &adata->acp_lock,
> + sizeof(adata->acp_lock));
> + acp63_fill_platform_dev_info(&pdevinfo[1], parent, adata->sdw_fw_node,
> + "amd_sdw_manager", 0, adata->res, 1,
> + sdw_pdata, sizeof(struct acp_sdw_pdata));
> + acp63_fill_platform_dev_info(&pdevinfo[2], parent, NULL, "amd_ps_sdw_dma",
> + 0, adata->res, 1, &adata->acp_lock,
> + sizeof(adata->acp_lock));
> + acp63_fill_platform_dev_info(&pdevinfo[3], parent, NULL, "dmic-codec",
> + 0, NULL, 0, NULL, 0);
> + } else if (adata->pdev_count == ACP63_SDW0_SDW1_PDM_MODE_DEVS) {
> + sdw_pdata = devm_kzalloc(&pci->dev, sizeof(struct acp_sdw_pdata) * 2,
> + GFP_KERNEL);
> + if (!sdw_pdata) {
> + ret = -ENOMEM;
> + goto de_init;
> + }
> + sdw_pdata[0].instance = 0;
> + sdw_pdata[1].instance = 1;
> + sdw_pdata[0].acp_sdw_lock = &adata->acp_lock;
> + sdw_pdata[1].acp_sdw_lock = &adata->acp_lock;
> + adata->pdm_dev_index = 0;
> + adata->sdw0_dev_index = 1;
> + adata->sdw1_dev_index = 2;
> + adata->sdw_dma_dev_index = 3;
> + acp63_fill_platform_dev_info(&pdevinfo[0], parent, NULL, "acp_ps_pdm_dma",
> + 0, adata->res, 1, &adata->acp_lock,
> + sizeof(adata->acp_lock));
> + acp63_fill_platform_dev_info(&pdevinfo[1], parent, adata->sdw_fw_node,
> + "amd_sdw_manager", 0, adata->res, 1,
> + &sdw_pdata[0], sizeof(struct acp_sdw_pdata));
> + acp63_fill_platform_dev_info(&pdevinfo[2], parent, adata->sdw_fw_node,
> + "amd_sdw_manager", 1, adata->res, 1,
> + &sdw_pdata[1], sizeof(struct acp_sdw_pdata));
> + acp63_fill_platform_dev_info(&pdevinfo[3], parent, NULL, "amd_ps_sdw_dma",
> + 0, adata->res, 1, &adata->acp_lock,
> + sizeof(adata->acp_lock));
> + acp63_fill_platform_dev_info(&pdevinfo[4], parent, NULL, "dmic-codec",
> + 0, NULL, 0, NULL, 0);
> + }
> + break;
> default:
> - dev_dbg(&pci->dev, "No PDM devices found\n");
> + dev_dbg(&pci->dev, "No PDM or Soundwire manager devices found\n");

what does this mean? I find this debug adds more confusion.

> return 0;
> }
>
> @@ -290,7 +514,11 @@ static int snd_acp63_probe(struct pci_dev *pci,
> goto de_init;
> }
> val = readl(adata->acp63_base + ACP_PIN_CONFIG);
> - get_acp63_device_config(val, pci, adata);
> + ret = get_acp63_device_config(val, pci, adata);
> + if (ret) {
> + dev_err(&pci->dev, "get acp device config failed:%d\n", ret);
> + goto de_init;
> + }
> ret = create_acp63_platform_devs(pci, adata, addr);
> if (ret < 0) {
> dev_err(&pci->dev, "ACP platform devices creation failed\n");

Next message: Pierre-Louis Bossart: "Re: [PATCH 2/9] ASoC: amd: ps: handle soundwire interrupts in acp pci driver"
Previous message: Frank Li: "[PATCH v4 0/6] phy: cadence: salvo: some fixes and workarounds"
In reply to: Vijendar Mukunda: "[PATCH 1/9] ASoC: amd: ps: create platform devices based on acp config"
Next in thread: Mukunda,Vijendar: "Re: [PATCH 1/9] ASoC: amd: ps: create platform devices based on acp config"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]