Received-SPF: pass (google.com: domain of linux-kernel+bounces-167563-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <d5a045ec-5ab3-43fc-9625-82e8fca87035@amd.com>
Date: Fri, 3 May 2024 16:33:18 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/2] ASoC: amd: acp: fix for acp platform device creation
 failure
Content-Language: en-US
To: broonie@kernel.org
Cc: alsa-devel@alsa-project.org, venkataprasad.potturu@amd.com,
 Basavaraj.Hiregoudar@amd.com, Sunil-kumar.Dommati@amd.com,
 Liam Girdwood <lgirdwood@gmail.com>, Jaroslav Kysela <perex@perex.cz>,
 Takashi Iwai <tiwai@suse.com>, Syed Saba Kareem <Syed.SabaKareem@amd.com>,
 Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
 Claudiu Beznea <claudiu.beznea@tuxon.dev>,
 Jarkko Nikula <jarkko.nikula@bitmer.com>,
 "open list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..."
 <linux-sound@vger.kernel.org>, open list <linux-kernel@vger.kernel.org>
References: <20240502140340.4049021-1-Vijendar.Mukunda@amd.com>
From: "Mukunda,Vijendar" <vijendar.mukunda@amd.com>
In-Reply-To: <20240502140340.4049021-1-Vijendar.Mukunda@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?NXdqUGZ3dmRIK3JvU1dlMG5xOFhmdGF2Sml6QTlrZlNjTjVVNUJHQldxT0Fu?=
 =?utf-8?B?S0hQZG5rcHdXczJRY1o5THpjNDBtejdiUjAwem1qTloxRkNNZzhPeS81a1NR?=
 =?utf-8?B?VnQ1blFZNy9IQmpNVGdEU0tmS1VUV2NrS3pLWXNWQTNCS0poMEVaWnpSdmhn?=
 =?utf-8?B?bG80eUhQSU9CdmU4VzVZMEhzbmt5NU1yTEtkdWFKVkw1MytURGZsR0w1UXYz?=
 =?utf-8?B?ZkNFY203TEUzVmZtaFlHMElNcStPcmNOZHhoKy9HdFVpU2R0Ylg2aFFQdWtI?=
 =?utf-8?B?V3N0NnluQXd6TjJvT3labndFQWRiZDlQQlNUZmFpbEMrR2tpM05adUgzUEJY?=
 =?utf-8?B?d1FlSXBJOGk4bTFIZFduaElvQXhaNnFSOG9GeDlYRHlFcmkvYktDL3g3R01P?=
 =?utf-8?B?VWtucnhyYS92K0RqTjIzZTEzdmw0c0lxaCtITktwcE05amErRFg2TXcwaWxI?=
 =?utf-8?B?YWwzTmxsdmNjRUd3Rk1sMUwyZ0F5ZWdWK0IvWDdSc3hvZDVXRWdHbTJEUVVJ?=
 =?utf-8?B?M0dNMURiSjRiQjlNZFFleXVTbmJKREloSW4xSEdYOFA1bHBzeXRRcHB1clFT?=
 =?utf-8?B?U2hnMWpWVCsvTXVsNWF1UUhXRnVnOHRmMzZhNHhTN2l5NldQM1MxbktEQmhJ?=
 =?utf-8?B?T1VLeXp6VGRGd3VWWWFpQ0FYZ0ZvbURCMUhleVhYYjNLTXNWcy9PZGl3QThN?=
 =?utf-8?B?MHRMMGRXVDdWaVRYTTRWUDBncUZtN1R2dzNuS2xHU0ZDMkRSQlJ4bE9TYjVI?=
 =?utf-8?B?OE5NSVZ5RnU2Y2U0dUFzNGxJN2F3cG9hbGdCRXNocEllb1l4a0ZMNUlFVkpT?=
 =?utf-8?B?Qm1TQ1JNTFpXcE9pV1I5eWc2Zjl2eEFvallQZ3dqYnJadWtqOTBlb0YxZ0tV?=
 =?utf-8?B?MWw5Ky9rNzZBVFlBWTBDYnRiRzQ5N1Y5cmZIcVhIYmdKeTY0cVUrWWlzMzdn?=
 =?utf-8?B?V3R6Vi90R0dZODFlYi9aYjUvdG9XS29Odk9ucGNMUHRuQUZlRkFuVkZSUWNR?=
 =?utf-8?B?TERoU2dNbm9qQ2FwWXFXL09lUGp6U1d2Um91MVFpdlhvMXN3YjBLbW0zZVNU?=
 =?utf-8?B?azRMeFVmTkc2UHNSdHlCMlhKVVB4R2hVTWllOFgvMXdYWUZzV3E0NWJqMHQw?=
 =?utf-8?B?TFRaSkRGRmRHVDNVTlRldGZZQnY4OVp1RDFyRXRiV2c5MXltbWpxb2g3MW5t?=
 =?utf-8?B?dHVZYkpQem5kbFB6WDBNZENpYkxEM3FPekI2L2hicm9ZS00vK2lDNDhFdUI2?=
 =?utf-8?B?eEpQWGdnMGR0V0ZUZnlSRlAwRWZlSjFYNGFHMmtRWUtRc0FhUHVWbVd1YWpv?=
 =?utf-8?B?SjVDa3pBUnQ5Y1lldmFOVWMzRGxYcXZ1TWtxajFQVjd3MmlhSjF5ektkZkZW?=
 =?utf-8?B?ZFl1cVFUbW42Z1hoUTVFOTRpcHBjdU9JNkZkc0Uzdzh5MlJYSTYxdFNEdTZV?=
 =?utf-8?B?RDMyZ1NuSlVYWlB3aXZ6VjNOcjA0ZHZac2J2Tm9RNkdaZDcwL0tqTW10NVFa?=
 =?utf-8?B?TUhHc3lxbUY4T2o1ZzFZV052Qmlnekkyc0dabmJkTm1PVGduVGpxMFRnUlp0?=
 =?utf-8?B?b1hsZjZWQnhncFM5Q2dHOURHdXVWNkdFZUpFY0wrR2o5d0FqeVM4cStjN2xF?=
 =?utf-8?B?WSthWVphVEkwb3hlbEk3c05HSlVVcU5KOHArakdweWhFT2E2NzBMbGl2cmsv?=
 =?utf-8?B?NmpSOUhkL0ErbUg4bDd4R0xjU0pyb0U2VTBqcUpRa0ttQVhXMTZRM3llNEVX?=
 =?utf-8?B?NkxpNk5VQjRXd3pJUzZHazlabWJhSlJnNE5oNkpkTGNlU2QzM2JMd2wxaTM5?=
 =?utf-8?B?TFlIQXlvUHhpS3FRNnRSNnhPdDM5OGJWNnVNTCtteEUvRS81WFJrWW41VEVB?=
 =?utf-8?B?NllOVXFTUm9OQW5IRVdIMXNnY3ZyNG9XUzZ0anNEZTJjYUNUT2pxSTRBTmZL?=
 =?utf-8?B?NGo3bENzUzhzYjhGUDdsc0NZWWcweWJYZklpWnpOS1ErRENDVExwMDhWZFNt?=
 =?utf-8?B?dy9GTXZUbjVJcm9iOHNaSnVtenpPUERHSWw1ZzJBei9QdFJmMnZ2b3NxblZQ?=
 =?utf-8?B?YzB0Um9mZitjbTBNc1l6cXpXYk5GNGVQRmphS0h4MWlGZ01KdGVmbUt2WmdX?=
 =?utf-8?Q?EtSlWrm82IZcx0H/iD7Hh8uFa?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3b2685ef-08e4-4689-3c2b-08dc6b60a985
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB4123.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 May 2024 11:03:29.0415
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ovqUbtRxJFL2VzHzByHAwwrI7XRpqh70IjDuKCu6en2hfAgi3G7lluDfLHFuMCIDTeHhtG/VhOMDxRNhy6NZEA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ1PR12MB6147

On 02/05/24 19:33, Vijendar Mukunda wrote:
> ACP pin configuration varies based on acp version.
> ACP PCI driver should read the ACP PIN config value and based on config
> value, it has to create a platform device in below two conditions.
> 1) If ACP PDM configuration is selected from BIOS and ACP PDM controller
> exists.
> 2) If ACP I2S configuration is selected from BIOS.
>
> Other than above scenarios, ACP PCI driver should skip the platform
> device creation logic, i.e. ACP PCI driver probe sequence should never
> fail if other acp pin configuration is selected. It should skip platform
> device creation logic.
>
> check_acp_pdm() function was implemented for ACP6.x platforms to check
> ACP PDM configuration. Previously, this code was safe guarded by
> FLAG_AMD_LEGACY_ONLY_DMIC flag check.
>
> This implementation breaks audio use cases for Huawei Matebooks which are
> based on ACP3.x varaint uses I2S configuration.
> In current scenario, check_acp_pdm() function returns -ENODEV value
> which results in ACP PCI driver probe failure without creating a platform
> device even in case of valid ACP pin configuration.
>
> Implement check_acp_config() as a common function which invokes platform
> specific acp pin configuration check functions for ACP3.x, ACP6.0 & ACP6.3
> & ACP7.0 variants and checks for ACP PDM controller.
>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218780
> Fixes: 4af565de9f8c ("ASoC: amd: acp: fix for acp pdm configuration check")
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> ---
>  sound/soc/amd/acp/acp-legacy-common.c | 96 ++++++++++++++++++++++-----
>  sound/soc/amd/acp/acp-pci.c           |  9 ++-
>  sound/soc/amd/acp/amd.h               | 10 ++-
>  sound/soc/amd/acp/chip_offset_byte.h  |  1 +
>  4 files changed, 95 insertions(+), 21 deletions(-)
>
> diff --git a/sound/soc/amd/acp/acp-legacy-common.c b/sound/soc/amd/acp/acp-legacy-common.c
> index b5aff3f230be..3be7c6d55a6f 100644
> --- a/sound/soc/amd/acp/acp-legacy-common.c
> +++ b/sound/soc/amd/acp/acp-legacy-common.c
> @@ -358,11 +358,25 @@ int smn_read(struct pci_dev *dev, u32 smn_addr)
>  }
>  EXPORT_SYMBOL_NS_GPL(smn_read, SND_SOC_ACP_COMMON);
>  
> -int check_acp_pdm(struct pci_dev *pci, struct acp_chip_info *chip)
> +static void check_acp3x_config(struct acp_chip_info *chip)
>  {
> -	struct acpi_device *pdm_dev;
> -	const union acpi_object *obj;
> -	u32 pdm_addr, val;
> +	u32 val;
> +
> +	val = readl(chip->base + ACP3X_PIN_CONFIG);
> +	switch (val) {
> +	case ACP_CONFIG_4:
> +		chip->is_i2s_config = true;
> +		chip->is_pdm_config = true;
> +		break;
> +	default:
> +		chip->is_pdm_config = true;
> +		break;
> +	}
> +}
> +
> +static void check_acp6x_config(struct acp_chip_info *chip)
> +{
> +	u32 val;
>  
>  	val = readl(chip->base + ACP_PIN_CONFIG);
>  	switch (val) {
> @@ -371,42 +385,94 @@ int check_acp_pdm(struct pci_dev *pci, struct acp_chip_info *chip)
>  	case ACP_CONFIG_6:
>  	case ACP_CONFIG_7:
>  	case ACP_CONFIG_8:
> -	case ACP_CONFIG_10:
>  	case ACP_CONFIG_11:
> +	case ACP_CONFIG_14:
> +		chip->is_pdm_config = true;
> +		break;
> +	case ACP_CONFIG_9:
> +		chip->is_i2s_config = true;
> +		break;
> +	case ACP_CONFIG_10:
>  	case ACP_CONFIG_12:
>  	case ACP_CONFIG_13:
> +		chip->is_i2s_config = true;
> +		chip->is_pdm_config = true;
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +static void check_acp70_config(struct acp_chip_info *chip)
> +{
> +	u32 val;
> +
> +	val = readl(chip->base + ACP_PIN_CONFIG);
> +	switch (val) {
> +	case ACP_CONFIG_4:
> +	case ACP_CONFIG_5:
> +	case ACP_CONFIG_6:
> +	case ACP_CONFIG_7:
> +	case ACP_CONFIG_8:
> +	case ACP_CONFIG_11:
>  	case ACP_CONFIG_14:
> +	case ACP_CONFIG_17:
> +	case ACP_CONFIG_18:
> +		chip->is_pdm_config = true;
> +		break;
> +	case ACP_CONFIG_9:
> +		chip->is_i2s_config = true;
> +		break;
> +	case ACP_CONFIG_10:
> +	case ACP_CONFIG_12:
> +	case ACP_CONFIG_13:
> +	case ACP_CONFIG_19:
> +	case ACP_CONFIG_20:
> +		chip->is_i2s_config = true;
> +		chip->is_pdm_config = true;
>  		break;
>  	default:
> -		return -EINVAL;
> +		break;
>  	}
> +}
> +
> +void check_acp_config(struct pci_dev *pci, struct acp_chip_info *chip)
> +{
> +	struct acpi_device *pdm_dev;
> +	const union acpi_object *obj;
> +	u32 pdm_addr;
>  
>  	switch (chip->acp_rev) {
>  	case ACP3X_DEV:
>  		pdm_addr = ACP_RENOIR_PDM_ADDR;
> +		check_acp3x_config(chip);
>  		break;
>  	case ACP6X_DEV:
>  		pdm_addr = ACP_REMBRANDT_PDM_ADDR;
> +		check_acp6x_config(chip);
>  		break;
>  	case ACP63_DEV:
>  		pdm_addr = ACP63_PDM_ADDR;
> +		check_acp6x_config(chip);
>  		break;
>  	case ACP70_DEV:
>  		pdm_addr = ACP70_PDM_ADDR;
> +		check_acp70_config(chip);
>  		break;
>  	default:
> -		return -EINVAL;
> +		break;
>  	}
>  
> -	pdm_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), pdm_addr, 0);
> -	if (pdm_dev) {
> -		if (!acpi_dev_get_property(pdm_dev, "acp-audio-device-type",
> -					   ACPI_TYPE_INTEGER, &obj) &&
> -					   obj->integer.value == pdm_addr)
> -			return 0;
> +	if (chip->is_pdm_config) {
> +		pdm_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), pdm_addr, 0);
> +		if (pdm_dev) {
> +			if (!acpi_dev_get_property(pdm_dev, "acp-audio-device-type",
> +						   ACPI_TYPE_INTEGER, &obj) &&
> +						   obj->integer.value == pdm_addr)
> +				chip->is_pdm_dev = true;
> +		}
>  	}
> -	return -ENODEV;
>  }
> -EXPORT_SYMBOL_NS_GPL(check_acp_pdm, SND_SOC_ACP_COMMON);
> +EXPORT_SYMBOL_NS_GPL(check_acp_config, SND_SOC_ACP_COMMON);
>  
>  MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/sound/soc/amd/acp/acp-pci.c b/sound/soc/amd/acp/acp-pci.c
> index 5f35b90eab8d..ad320b29e87d 100644
> --- a/sound/soc/amd/acp/acp-pci.c
> +++ b/sound/soc/amd/acp/acp-pci.c
> @@ -100,7 +100,6 @@ static int acp_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id
>  		ret = -EINVAL;
>  		goto release_regions;
>  	}
> -
Empty line drop is not relevant to this patch.
Will fix and re-spin the patch series.
>  	dmic_dev = platform_device_register_data(dev, "dmic-codec", PLATFORM_DEVID_NONE, NULL, 0);
>  	if (IS_ERR(dmic_dev)) {
>  		dev_err(dev, "failed to create DMIC device\n");
> @@ -119,6 +118,10 @@ static int acp_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id
>  	if (ret)
>  		goto unregister_dmic_dev;
>  
> +	check_acp_config(pci, chip);
> +	if (!chip->is_pdm_dev && !chip->is_i2s_config)
> +		goto skip_pdev_creation;
> +
>  	res = devm_kcalloc(&pci->dev, num_res, sizeof(struct resource), GFP_KERNEL);
>  	if (!res) {
>  		ret = -ENOMEM;
> @@ -136,10 +139,6 @@ static int acp_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id
>  		}
>  	}
>  
> -	ret = check_acp_pdm(pci, chip);
> -	if (ret < 0)
> -		goto skip_pdev_creation;
> -
>  	chip->flag = flag;
>  	memset(&pdevinfo, 0, sizeof(pdevinfo));
>  
> diff --git a/sound/soc/amd/acp/amd.h b/sound/soc/amd/acp/amd.h
> index 5017e868f39b..d75b4eb34de8 100644
> --- a/sound/soc/amd/acp/amd.h
> +++ b/sound/soc/amd/acp/amd.h
> @@ -138,6 +138,9 @@ struct acp_chip_info {
>  	void __iomem *base;	/* ACP memory PCI base */
>  	struct platform_device *chip_pdev;
>  	unsigned int flag;	/* Distinguish b/w Legacy or Only PDM */
> +	bool is_pdm_dev;	/* flag set to true when ACP PDM controller exists */
> +	bool is_pdm_config;	/* flag set to true when PDM configuration is selected from BIOS */
> +	bool is_i2s_config;	/* flag set to true when I2S configuration is selected from BIOS */
>  };
>  
>  struct acp_stream {
> @@ -212,6 +215,11 @@ enum acp_config {
>  	ACP_CONFIG_13,
>  	ACP_CONFIG_14,
>  	ACP_CONFIG_15,
> +	ACP_CONFIG_16,
> +	ACP_CONFIG_17,
> +	ACP_CONFIG_18,
> +	ACP_CONFIG_19,
> +	ACP_CONFIG_20,
>  };
>  
>  extern const struct snd_soc_dai_ops asoc_acp_cpu_dai_ops;
> @@ -240,7 +248,7 @@ void restore_acp_pdm_params(struct snd_pcm_substream *substream,
>  int restore_acp_i2s_params(struct snd_pcm_substream *substream,
>  			   struct acp_dev_data *adata, struct acp_stream *stream);
>  
> -int check_acp_pdm(struct pci_dev *pci, struct acp_chip_info *chip);
> +void check_acp_config(struct pci_dev *pci, struct acp_chip_info *chip);
>  
>  static inline u64 acp_get_byte_count(struct acp_dev_data *adata, int dai_id, int direction)
>  {
> diff --git a/sound/soc/amd/acp/chip_offset_byte.h b/sound/soc/amd/acp/chip_offset_byte.h
> index cfd6c4d07594..18da734c0e9e 100644
> --- a/sound/soc/amd/acp/chip_offset_byte.h
> +++ b/sound/soc/amd/acp/chip_offset_byte.h
> @@ -20,6 +20,7 @@
>  #define ACP_SOFT_RESET                          0x1000
>  #define ACP_CONTROL                             0x1004
>  #define ACP_PIN_CONFIG				0x1440
> +#define ACP3X_PIN_CONFIG			0x1400
>  
>  #define ACP_EXTERNAL_INTR_REG_ADDR(adata, offset, ctrl) \
>  	(adata->acp_base + adata->rsrc->irq_reg_offset + offset + (ctrl * 0x04))