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

From: Mukunda,Vijendar
Date: Wed May 17 2023 - 04:34:04 EST

Next message: Alexandre Ghiti: "Re: Bug report: kernel paniced when system hibernates"
Previous message: Krzysztof Kozlowski: "Re: [PATCH v2] dt-bindings: display: panel: add panel-mipi-dsi-bringup"
In reply to: Pierre-Louis Bossart: "Re: [PATCH 1/9] ASoC: amd: ps: create platform devices based on acp config"
Next in thread: Pierre-Louis Bossart: "Re: [PATCH 1/9] ASoC: amd: ps: create platform devices based on acp config"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 16/05/23 20:02, Pierre-Louis Bossart wrote:
>
> 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.
Above enum's are listed to know the platform device masks.
For example - if ACP63_PDM_DEV_MASK is set, then ACP PCI driver
will create platform device for PDM controller.

If ACP63_SDW_DEV_MASK is set, ACP PCI driver will create platform device
nodes for soundwire manager instances based on instance count retrieved
by scanning the SoundWire Controller.

If ACP63_SDW_PDM_DEV_MASK is set, ACP PCI driver will create platform device
nodes for PDM controller and SoundWire manager instances.

We will add comment for the same.

>
>> +
>> 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 ?

will fix it.
>
>> + * @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_dmic_dev is to know whether PDM controller device node exists or not with in
ACP PCI device ACPI scope by checking _ADDR as mentioned below.

dmic_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_DMIC_ADDR, 0);

if dmic_dev exists then "is_dmic_dev" is set to true.

To avoid confusion, we will rename "dmic_dev" as "pdm_dev".

>
>> + * @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.
Similarly, is_sdw_dev flag is set if SoundWire Manager devices
are available after scanning SoundWire controller listed in ACP PCI
device ACPI scope.
>> + * @acp_reset: flag set to true when bus reset is applied across all
>> + * the active soundwire manager instances
> SoundWire
Will fix it.
>
>> + */
>> +
>> 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".
Will fix it.
>
>> + */
>> + 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.

Based on ACP PIN Config, "is_dmic_dev" , "is_sdw_dev" flags ACP PCI driver will create platform device nodes.
pdev_mask value to know what combination of platform devices need to be created and
pdev_count refers to total number of platform device nodes to be created.

We will add comments.

>
>> }
>> + 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.
Currently, we are trying to create platform devices for PDM controller and SoundWire
Manager instances based on ACP pin config and ACPI _ADDR fields scan under ACP PCI device
scope.
Earlier We have added support for ACP PDM controller.
ACP PIN config supports different audio configurations other than PDM and SoundWire
based audio endpoints.

If there is no pdev_mask set, it refers to default switch case.
This dev_dbg statement to notify that no PDM and Soundwire manager devices found
from ACPI scan.

This patch adds support for platform device creation logic for Soundwire manager instances &
PDM controller combinations based on ACP PIN Config and ACPI _ADDR field scan.

Possible combination of platform device nodes:

1) ACP PDM Controller, dmic-codec, machine driver platform device node
2) ACP PDM Controller , dmic-codec, SW0 manager instance, platform device for SoundWire DMA driver
3) SW0, SW1 SoundWire manager instances, platform device for SoundWire DMA driver
3) ACP PDM Controller, dmic-codec, SDW0, SDW1 manager instances, platform device for SoundWire DMA driver

>
>> 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: Alexandre Ghiti: "Re: Bug report: kernel paniced when system hibernates"
Previous message: Krzysztof Kozlowski: "Re: [PATCH v2] dt-bindings: display: panel: add panel-mipi-dsi-bringup"
In reply to: Pierre-Louis Bossart: "Re: [PATCH 1/9] ASoC: amd: ps: create platform devices based on acp config"
Next in thread: Pierre-Louis Bossart: "Re: [PATCH 1/9] ASoC: amd: ps: create platform devices based on acp config"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]