On 06/06/23 19:30, Pierre-Louis Bossart wrote:
+/**
- acp_pdev_mask corresponds to platform device mask based on audio endpoint combinations.
- acp_pdev_mask will be calculated based on ACPI Scan under ACP PCI device and
- ACP PIN Configuration.
- Based acp_pdev_mask, platform devices will be created.
- Below are possible platform device combinations.
- ACP PDM Controller, dmic-codec, machine driver platform device node
- ACP PDM Controller , dmic-codec, SW0 SoundWire manager instance, platform device for
- SoundWire DMA driver
- SW0, SW1 SoundWire manager instances, platform device for SoundWire DMA driver
- ACP PDM Controller, dmic-codec, SDW0, SDW1 manager instances, platform device for
- SoundWire DMA driver
- ACP63_PDM_DEV_MASK corresponds to platform device mask for ACP PDM controller.
- ACP63_SDW_DEV_MASK corresponds to platform device mask for SDW manager instances.
- ACP63_SDW_PDM_DEV_MASK corresponds to platform device mask for ACP PDM + SDW manager combination
- */
+enum acp_pdev_mask {
- ACP63_PDM_DEV_MASK = 1,
- ACP63_SDW_DEV_MASK,
- ACP63_SDW_PDM_DEV_MASK,
+};
This does not look like a mask, the definitions prevent bit-wise operations from happening.
Either use BIT(0), BIT(1), BIT(2) or GENMASK(1, 0), or demote this to a regular enum (e.g. pdev_config or something)
ACP63_PDM_DEV_MASK - Will be set only PDM config is selected. ACP63_SDW_DEV_MASK - will be set only when SDW config is selected. ACP63_SDW_PDM_DEV_MASK - will be set only when ACP PDM + SDW config is selected.
We have already added comments for above masks definitions in code. Our intention is to use it as a mask. We don't think it breaks anything. Currently, we have only one extra check for SDW case, in suspend/resume scenario. Based on SoundWire power mode, ACP PCI driver should invoke acp_deinit/acp_init() calls in suspend/resume callbacks. For this, we have added check for pdev_mask. If pdev_mask is set to ACP63_SDW_DEV_MASK (2) or ACP63_SDW_PDM_DEV_MASK(3), in this case only by checking SoundWire power mode invoke acp_deinit/acp_init() sequence. This is already in place.
There won't be any extra checks will be added in the future. As per our understanding, it's good to go.
struct pdm_stream_instance { u16 num_pages; u16 channels; @@ -95,14 +144,38 @@ 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: array of child platform device node structures
- @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
- @sdw_manager_count: SoundWire manager instance count
- @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
- @acp_reset: flag set to true when bus reset is applied across all
- the active SoundWire manager instances
- */
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 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 54752d6040d6..816c22e7f1ab 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,37 +121,162 @@ 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);IIRC this is only defined in the DisCo 2.0 spec, previous editions had a 'mipi-master-count'. A comment would not hurt to point to the minimal DisCo spec version.
We will add comment.
- 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;}ret = fwnode_property_read_u32(link, "amd-sdw-power-mode", &acp_sdw_power_mode);if (ret)return ret;/** 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.*/if (acp_sdw_power_mode != AMD_SDW_POWER_OFF_MODE) {acp_data->acp_reset = false;return 0;}- }
- 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) {
/* is_dmic_dev flag will be set when ACP PDM controller device exists */if (!acpi_dev_get_property(dmic_dev, "acp-audio-device-type",
usually properties start with the 'mipi-' or 'vendor-' prefix. Is there a missing 'amd-' here or is 'acp-' unique enough?
It's not SoundWire related MIPI/Vendor property. Our BIOS changes are freeze. We can't modify this one as of this moment. We will consider it for next platform.