On 11/01/23 19:02, Pierre-Louis Bossart wrote:
>
>
>> +#define AMD_SDW_CLK_STOP_MODE 1
> there are multiple modes for clock stop in SoundWire, and multiple ways
> for the link manager to deal with clock stop, you want a comment to
> describe what this define refers to.
will add comments about flags explanation.
>> +#define AMD_SDW_POWER_OFF_MODE 2
>> +
>> +struct acp_sdw_pdata {
>> + u16 instance;
>> + struct mutex *sdw_lock;
> need a comment on what this lock protects.
>
>> +};
>> +#endif
>> diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
>> index b7535c7d093f..ed979e6d0c1d 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_CONTROLLERS 2
>>
>> enum acp_config {
>> ACP_CONFIG_0 = 0,
>> @@ -77,6 +83,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,
>> +};
>> +
>> struct pdm_stream_instance {
>> u16 num_pages;
>> u16 channels;
>> @@ -107,7 +119,15 @@ struct acp63_dev_data {
>> 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_master_count;
> for new contributions, it's recommended to use manager and peripheral.
will use manager and peripheral terminology.
>> + u16 sdw0_dev_index;
>> + u16 sdw1_dev_index;
> probably need a comment on what the 0 and 1 refer to, it's not clear if
> there's any sort of dependency/link with the 'sdw_master_count' above.
>
> If this is related to the two controllers mentioned in the cover letter,
> then an explanation of the sdw_master_count would be needed as well
> (single variable for two controllers?)
will add comments for dev_index variables.
>> + u16 sdw_dma_dev_index;
>> + bool is_dmic_dev;
>> + bool is_sdw_dev;
>> + bool acp_sdw_power_off;
>> };
>> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
>> index e86f23d97584..85154cf0b2a2 100644
>> --- a/sound/soc/amd/ps/pci-ps.c
>> +++ b/sound/soc/amd/ps/pci-ps.c
>> @@ -14,6 +14,7 @@
>> #include <linux/interrupt.h>
>> #include <sound/pcm_params.h>
>> #include <linux/pm_runtime.h>
>> +#include <linux/soundwire/sdw_amd.h>
>>
>> #include "acp63.h"
>>
>> @@ -134,12 +135,68 @@ 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];
>> + u8 count = 0;
>> + u32 acp_sdw_power_mode = 0;
>> + int index;
>> + int ret;
>> +
>> + acp_data = dev_get_drvdata(dev);
>> + acp_data->acp_sdw_power_off = true;
>> + /* Found controller, find links supported */
>> + ret = fwnode_property_read_u8_array((acp_data->sdw_fw_node),
>> + "mipi-sdw-master-count", &count, 1);
>> +
>> + if (ret) {
>> + dev_err(dev,
>> + "Failed to read mipi-sdw-master-count: %d\n", ret);
> one line?
will fix it.
>
>> + return -EINVAL;
>> + }
>> +
>> + /* Check count is within bounds */
>> + if (count > AMD_SDW_MAX_CONTROLLERS) {
>> + dev_err(dev, "Controller count %d exceeds max %d\n",
>> + count, AMD_SDW_MAX_CONTROLLERS);
> No. controllers and masters are different concepts, see the DisCo
> specification for SoundWire. A Controller can have multiple Masters.
Will correct it.
>
>> + return -EINVAL;
>> + }
>> +
>> + if (!count) {
>> + dev_warn(dev, "No SoundWire controllers detected\n");
>> + return -EINVAL;
>> + }
> is this really a warning, looks like a dev_dbg or info to me.
>
>> + dev_dbg(dev, "ACPI reports %d Soundwire Controller devices\n", count);
> the term device is incorrect here, the DisCo spec does not expose ACPI
> devices for each master.
>
> "ACPI reports %d Managers"
will correct it.
>> + acp_data->sdw_master_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, "Master node %s not found\n", name);
>> + return -EIO;
>> + }
>> +
>> + fwnode_property_read_u32(link, "amd-sdw-power-mode",
>> + &acp_sdw_power_mode);
>> + if (acp_sdw_power_mode != AMD_SDW_POWER_OFF_MODE)
>> + acp_data->acp_sdw_power_off = false;
> does power-off mean 'clock-stop'?
> No. We will add comment for acp_sdw_power_off flag.
>> + }
>> + return 0;
>> +}
>> +
>> + if (is_dmic_dev && is_sdw_dev) {
>> + switch (acp_data->sdw_master_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;
> so the cover letter is indeed wrong and confuses two controllers for two
> managers.
ACP IP has two independent manager instances driven by separate controller
each which are connected in different power domains.
we should create two separate ACPI companion devices for separate
manager instance. Currently we have limitations with BIOS.
we are going with single ACPI companion device.
We will update the changes later.
>
>> + 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_master_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;
>> }
