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");