| Message ID | 20230606060724.2038680-2-Vijendar.Mukunda@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: amd: ps: add SoundWire support | expand |
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. >> + * 1) ACP PDM Controller, dmic-codec, machine driver platform device node >> + * 2) ACP PDM Controller , dmic-codec, SW0 SoundWire manager instance, platform device for >> + * SoundWire DMA driver >> + * 3) SW0, SW1 SoundWire manager instances, platform device for SoundWire DMA driver >> + * 4) 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.
On 6/7/2023 1:35 AM, Mukunda,Vijendar wrote: > 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. >>> + * 1) ACP PDM Controller, dmic-codec, machine driver platform device node >>> + * 2) ACP PDM Controller , dmic-codec, SW0 SoundWire manager instance, platform device for >>> + * SoundWire DMA driver >>> + * 3) SW0, SW1 SoundWire manager instances, platform device for SoundWire DMA driver >>> + * 4) 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. > I think the problem is in use of the word "mask" in this context. That usually means that you can do a bitwise operation on it. Really it's behaving more like an "enum" does. In patch 9 you have the following code: + if (adata->pdev_mask & ACP63_SDW_DEV_MASK) { That's checking if bits 0 and 1 are both set. But if in the future a new set of hypothetical device types was introduced that mapped out to values "4" and "5" then you might end up with matches in the code that don't make sense. So it makes more sense to do one of these solutions: 1) rename this adev->pdev_mask to be adev->pdev_config and then in patch 9 to use something like this: if (adev->pdev_config == ACP63_SDW_DEV_CONFIG) 2) re-assign it so each config gets a single bit and keep the patch 9 behavior. PDM is BIT(0), SDW is BIT(1) PDM + SDW is BIT(2) etc. Either way that will ensure that you never have an unexpected match. Unexpected matches can be more likely as the code base grows and it's used for more platforms and configs. > > >>> + >>> 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. Besides impact to BIOS it also has impact to drivers in other operating systems as well. So changing a property name is not something that can be taken lightly. I'd also point out the use of the ACPI property is localized to an AMD specific driver not generic code.
On 07/06/23 22:17, Limonciello, Mario wrote: > > On 6/7/2023 1:35 AM, Mukunda,Vijendar wrote: >> 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. >>>> + * 1) ACP PDM Controller, dmic-codec, machine driver platform device node >>>> + * 2) ACP PDM Controller , dmic-codec, SW0 SoundWire manager instance, platform device for >>>> + * SoundWire DMA driver >>>> + * 3) SW0, SW1 SoundWire manager instances, platform device for SoundWire DMA driver >>>> + * 4) 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. >> > I think the problem is in use of the word "mask" in this context. > That usually means that you can do a bitwise operation on it. > Really it's behaving more like an "enum" does. > > In patch 9 you have the following code: > > + if (adata->pdev_mask & ACP63_SDW_DEV_MASK) { > > That's checking if bits 0 and 1 are both set. > > But if in the future a new set of hypothetical device types was introduced > that mapped out to values "4" and "5" then you might end up with matches > in the code that don't make sense. > > So it makes more sense to do one of these solutions: > > 1) rename this adev->pdev_mask to be adev->pdev_config and then in patch 9 > to use something like this: > > if (adev->pdev_config == ACP63_SDW_DEV_CONFIG) > > 2) re-assign it so each config gets a single bit and keep the patch 9 behavior. > PDM is BIT(0), SDW is BIT(1) PDM + SDW is BIT(2) etc. > > Either way that will ensure that you never have an unexpected match. > Unexpected matches can be more likely as the code base grows and it's used for > more platforms and configs. I think Mask word is created confusion here. These changes are specific to PS platform only. To avoid further confusion, we will rename the pdev_mask as pdev_config. "if (adata->pdev_mask & ACP63_SDW_DEV_MASK)" condition check can be refactored. will fix it and post V4 patch set. > >> >> >>>> + >>>> 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. > > Besides impact to BIOS it also has impact to drivers in other > operating systems as well. So changing a property name is not > something that can be taken lightly. > Agreed. > I'd also point out the use of the ACPI property is localized to > an AMD specific driver not generic code. >
diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h index 2f94448102d0..95bb1cef900a 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 @@ -53,11 +53,38 @@ /* time in ms for runtime suspend delay */ #define ACP_SUSPEND_DELAY_MS 2000 -#define ACP63_DMIC_ADDR 2 -#define ACP63_PDM_MODE_DEVS 3 -#define ACP63_PDM_DEV_MASK 1 #define ACP_DMIC_DEV 2 +/* ACP63_PDM_MODE_DEVS corresponds to platform devices count for ACP PDM configuration */ +#define ACP63_PDM_MODE_DEVS 3 + +/* + * ACP63_SDW0_MODE_DEVS corresponds to platform devices count for + * SW0 SoundWire manager instance configuration + */ +#define ACP63_SDW0_MODE_DEVS 2 + +/* + * ACP63_SDW0_SDW1_MODE_DEVS corresponds to platform devices count for SW0 + SW1 SoundWire manager + * instances configuration + */ +#define ACP63_SDW0_SDW1_MODE_DEVS 3 + +/* + * ACP63_SDW0_PDM_MODE_DEVS corresponds to platform devices count for SW0 manager + * instance + ACP PDM controller configuration + */ +#define ACP63_SDW0_PDM_MODE_DEVS 4 + +/* + * ACP63_SDW0_SDW1_PDM_MODE_DEVS corresponds to platform devices count for + * SW0 + SW1 SoundWire manager instances + ACP PDM controller configuration + */ +#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 +107,28 @@ enum acp_config { ACP_CONFIG_15, }; +/** + * 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. + * 1) ACP PDM Controller, dmic-codec, machine driver platform device node + * 2) ACP PDM Controller , dmic-codec, SW0 SoundWire manager instance, platform device for + * SoundWire DMA driver + * 3) SW0, SW1 SoundWire manager instances, platform device for SoundWire DMA driver + * 4) 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, +}; + 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); + + 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", ACPI_TYPE_INTEGER, &obj) && obj->integer.value == ACP_DMIC_DEV) 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); + /* is_sdw_dev flag will be set when SoundWire Manager device exists */ + if (!ret) + is_sdw_dev = true; + } + if (!is_dmic_dev && !is_sdw_dev) + return -ENODEV; + 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; } + return 0; } static void acp63_fill_platform_dev_info(struct platform_device_info *pdevinfo, @@ -173,6 +300,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; @@ -204,8 +332,104 @@ 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, NULL, 0); + } 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, NULL, 0); + } + 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, NULL, 0); + 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, NULL, 0); + 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, NULL, 0); + 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, NULL, 0); + 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"); return 0; } @@ -289,12 +513,18 @@ 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); + /* ACP PCI driver probe should be continued even PDM or SoundWire Devices are not found */ + if (ret) { + dev_err(&pci->dev, "get acp device config failed:%d\n", ret); + goto skip_pdev_creation; + } ret = create_acp63_platform_devs(pci, adata, addr); if (ret < 0) { dev_err(&pci->dev, "ACP platform devices creation failed\n"); goto de_init; } +skip_pdev_creation: pm_runtime_set_autosuspend_delay(&pci->dev, ACP_SUSPEND_DELAY_MS); pm_runtime_use_autosuspend(&pci->dev); pm_runtime_put_noidle(&pci->dev);
Based on ACP pin configuration and scanning child devices under ACP pci device ACPI scope, platform device mask(pdev_mask) and platform device count(pdev_count) will be calculated. Using pdev_mask and pdev_count values, ACP PCI driver will create platform devices for Pink Sardine platform. Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> --- sound/soc/amd/ps/acp63.h | 81 +++++++++++- sound/soc/amd/ps/pci-ps.c | 252 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 318 insertions(+), 15 deletions(-)