Message-ID: <ff163aff-6071-f323-0a6b-825b5b394dbf@amd.com>
Date:   Wed, 7 Jun 2023 11:47:19 -0500
From:   "Limonciello, Mario" <mario.limonciello@....com>
To:     "Mukunda,Vijendar" <vijendar.mukunda@....com>,
        Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        broonie@...nel.org
Cc:     alsa-devel@...a-project.org, Basavaraj.Hiregoudar@....com,
        Sunil-kumar.Dommati@....com, Mastan.Katragadda@....com,
        Arungopal.kondaveeti@....com, Liam Girdwood <lgirdwood@...il.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Syed Saba Kareem <Syed.SabaKareem@....com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3 1/9] ASoC: amd: ps: create platform devices based on
 acp config


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.

Powered by blists - more mailing lists