[PATCH] ASoC: AMD Renoir - add DMI table to avoid the ACP mic probe (broken BIOS)

[PATCH] ASoC: AMD Renoir - add DMI table to avoid the ACP mic probe (broken BIOS)

[Jaroslav Kysela]

Users reported that some Lenovo AMD platforms do not have ACP microphone,
but the BIOS advertises it via ACPI.

This patch create a simple DMI table, where those machines with the broken
BIOS can be added. The DMI description for Lenovo IdeaPad 5 and
IdeaPad Flex 5 devices are added there.

Also describe the dmic_acpi_check kernel module parameter in a more
understandable way.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1892115
Cc: <stable@kernel.org>
Cc: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Jaroslav Kysela <perex@perex.cz>
---
 sound/soc/amd/renoir/rn-pci-acp3x.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/sound/soc/amd/renoir/rn-pci-acp3x.c b/sound/soc/amd/renoir/rn-pci-acp3x.c
index b943e59fc302..3289ab3eae6f 100644
--- a/sound/soc/amd/renoir/rn-pci-acp3x.c
+++ b/sound/soc/amd/renoir/rn-pci-acp3x.c
@@ -6,6 +6,7 @@
 
 #include <linux/pci.h>
 #include <linux/acpi.h>
+#include <linux/dmi.h>
 #include <linux/module.h>
 #include <linux/io.h>
 #include <linux/delay.h>
@@ -20,14 +21,13 @@ module_param(acp_power_gating, int, 0644);
 MODULE_PARM_DESC(acp_power_gating, "Enable acp power gating");
 
 /**
- * dmic_acpi_check = -1 - Checks ACPI method to know DMIC hardware status runtime
- *                 = 0 - Skips the DMIC device creation and returns probe failure
- *                 = 1 - Assumes that platform has DMIC support and skips ACPI
- *                       method check
+ * dmic_acpi_check = -1 - Use ACPI/DMI method to detect the DMIC hardware presence at runtime
+ *                 =  0 - Skip the DMIC device creation and return probe failure
+ *                 =  1 - Force DMIC support
  */
 static int dmic_acpi_check = ACP_DMIC_AUTO;
 module_param(dmic_acpi_check, bint, 0644);
-MODULE_PARM_DESC(dmic_acpi_check, "checks Dmic hardware runtime");
+MODULE_PARM_DESC(dmic_acpi_check, "Digital microphone presence (-1=auto, 0=none, 1=force)");
 
 struct acp_dev_data {
 	void __iomem *acp_base;
@@ -163,6 +163,17 @@ static int rn_acp_deinit(void __iomem *acp_base)
 	return 0;
 }
 
+static const struct dmi_system_id rn_acp_quirk_table[] = {
+	{
+		/* Lenovo IdeaPad Flex 5 14ARE05, IdeaPad 5 15ARE05 */
+		.matches = {
+			DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "LENOVO"),
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "LNVNB161216"),
+		}
+	},
+	{}
+};
+
 static int snd_rn_acp_probe(struct pci_dev *pci,
 			    const struct pci_device_id *pci_id)
 {
@@ -172,6 +183,7 @@ static int snd_rn_acp_probe(struct pci_dev *pci,
 	acpi_handle handle;
 	acpi_integer dmic_status;
 #endif
+	const struct dmi_system_id *dmi_id;
 	unsigned int irqflags;
 	int ret, index;
 	u32 addr;
@@ -232,6 +244,12 @@ static int snd_rn_acp_probe(struct pci_dev *pci,
 			goto de_init;
 		}
 #endif
+		dmi_id = dmi_first_match(rn_acp_quirk_table);
+		if (dmi_id && !dmi_id->driver_data) {
+			dev_warn(&pci->dev, "ACPI settings override using DMI (ACP mic is not present)");
+			ret = -ENODEV;
+			goto de_init;
+		}
 	}
 
 	adata->res = devm_kzalloc(&pci->dev,
-- 
2.26.2

Re: [PATCH] ASoC: AMD Renoir - add DMI table to avoid the ACP mic probe (broken BIOS)

[Takashi Iwai]

On Tue, 08 Dec 2020 16:36:54 +0100,
Jaroslav Kysela wrote:
> 
> Users reported that some Lenovo AMD platforms do not have ACP microphone,
> but the BIOS advertises it via ACPI.
> 
> This patch create a simple DMI table, where those machines with the broken
> BIOS can be added. The DMI description for Lenovo IdeaPad 5 and
> IdeaPad Flex 5 devices are added there.
> 
> Also describe the dmic_acpi_check kernel module parameter in a more
> understandable way.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1892115
> Cc: <stable@kernel.org>
> Cc: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> Cc: Mark Brown <broonie@kernel.org>
> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
> ---
>  sound/soc/amd/renoir/rn-pci-acp3x.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/amd/renoir/rn-pci-acp3x.c b/sound/soc/amd/renoir/rn-pci-acp3x.c
> index b943e59fc302..3289ab3eae6f 100644
> --- a/sound/soc/amd/renoir/rn-pci-acp3x.c
> +++ b/sound/soc/amd/renoir/rn-pci-acp3x.c
> @@ -6,6 +6,7 @@
>  
>  #include <linux/pci.h>
>  #include <linux/acpi.h>
> +#include <linux/dmi.h>
>  #include <linux/module.h>
>  #include <linux/io.h>
>  #include <linux/delay.h>
> @@ -20,14 +21,13 @@ module_param(acp_power_gating, int, 0644);
>  MODULE_PARM_DESC(acp_power_gating, "Enable acp power gating");
>  
>  /**
> - * dmic_acpi_check = -1 - Checks ACPI method to know DMIC hardware status runtime
> - *                 = 0 - Skips the DMIC device creation and returns probe failure
> - *                 = 1 - Assumes that platform has DMIC support and skips ACPI
> - *                       method check
> + * dmic_acpi_check = -1 - Use ACPI/DMI method to detect the DMIC hardware presence at runtime
> + *                 =  0 - Skip the DMIC device creation and return probe failure
> + *                 =  1 - Force DMIC support
>   */
>  static int dmic_acpi_check = ACP_DMIC_AUTO;
>  module_param(dmic_acpi_check, bint, 0644);
> -MODULE_PARM_DESC(dmic_acpi_check, "checks Dmic hardware runtime");
> +MODULE_PARM_DESC(dmic_acpi_check, "Digital microphone presence (-1=auto, 0=none, 1=force)");
>  
>  struct acp_dev_data {
>  	void __iomem *acp_base;
> @@ -163,6 +163,17 @@ static int rn_acp_deinit(void __iomem *acp_base)
>  	return 0;
>  }
>  
> +static const struct dmi_system_id rn_acp_quirk_table[] = {
> +	{
> +		/* Lenovo IdeaPad Flex 5 14ARE05, IdeaPad 5 15ARE05 */
> +		.matches = {
> +			DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "LENOVO"),
> +			DMI_EXACT_MATCH(DMI_BOARD_NAME, "LNVNB161216"),
> +		}
> +	},
> +	{}
> +};
> +
>  static int snd_rn_acp_probe(struct pci_dev *pci,
>  			    const struct pci_device_id *pci_id)
>  {
> @@ -172,6 +183,7 @@ static int snd_rn_acp_probe(struct pci_dev *pci,
>  	acpi_handle handle;
>  	acpi_integer dmic_status;
>  #endif
> +	const struct dmi_system_id *dmi_id;
>  	unsigned int irqflags;
>  	int ret, index;
>  	u32 addr;
> @@ -232,6 +244,12 @@ static int snd_rn_acp_probe(struct pci_dev *pci,
>  			goto de_init;
>  		}
>  #endif
> +		dmi_id = dmi_first_match(rn_acp_quirk_table);
> +		if (dmi_id && !dmi_id->driver_data) {
> +			dev_warn(&pci->dev, "ACPI settings override using DMI (ACP mic is not present)");

IMO, better to be dev_info() here.  It's the correct set up, hence
it should be neither error nor warning that appears in the boot screen
over the boot splash.

BTW, both Raven and Reonir drivers point to the very same PCI ID,
and both drivers will be probed for this machine (and both to be
skipped).

Also, I noticed that Renoir driver tries to detect the dmic at the
late stage; this could be done at the very beginning, so the whole
allocation and initialization could be simply skipped.  But this can
be done in a separate cleanup patch.


thanks,

Takashi

Re: [PATCH] ASoC: AMD Renoir - add DMI table to avoid the ACP mic probe (broken BIOS)

[Jaroslav Kysela]

Dne 08. 12. 20 v 17:24 Takashi Iwai napsal(a):
> On Tue, 08 Dec 2020 16:36:54 +0100,
> Jaroslav Kysela wrote:
>>
>> Users reported that some Lenovo AMD platforms do not have ACP microphone,
>> but the BIOS advertises it via ACPI.
>>
>> This patch create a simple DMI table, where those machines with the broken
>> BIOS can be added. The DMI description for Lenovo IdeaPad 5 and
>> IdeaPad Flex 5 devices are added there.
>>
>> Also describe the dmic_acpi_check kernel module parameter in a more
>> understandable way.
>>
>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1892115
>> Cc: <stable@kernel.org>
>> Cc: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>> Cc: Mark Brown <broonie@kernel.org>
>> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
>> ---
>>  sound/soc/amd/renoir/rn-pci-acp3x.c | 28 +++++++++++++++++++++++-----
>>  1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/sound/soc/amd/renoir/rn-pci-acp3x.c b/sound/soc/amd/renoir/rn-pci-acp3x.c
>> index b943e59fc302..3289ab3eae6f 100644
>> --- a/sound/soc/amd/renoir/rn-pci-acp3x.c
>> +++ b/sound/soc/amd/renoir/rn-pci-acp3x.c
>> @@ -6,6 +6,7 @@
>>  
>>  #include <linux/pci.h>
>>  #include <linux/acpi.h>
>> +#include <linux/dmi.h>
>>  #include <linux/module.h>
>>  #include <linux/io.h>
>>  #include <linux/delay.h>
>> @@ -20,14 +21,13 @@ module_param(acp_power_gating, int, 0644);
>>  MODULE_PARM_DESC(acp_power_gating, "Enable acp power gating");
>>  
>>  /**
>> - * dmic_acpi_check = -1 - Checks ACPI method to know DMIC hardware status runtime
>> - *                 = 0 - Skips the DMIC device creation and returns probe failure
>> - *                 = 1 - Assumes that platform has DMIC support and skips ACPI
>> - *                       method check
>> + * dmic_acpi_check = -1 - Use ACPI/DMI method to detect the DMIC hardware presence at runtime
>> + *                 =  0 - Skip the DMIC device creation and return probe failure
>> + *                 =  1 - Force DMIC support
>>   */
>>  static int dmic_acpi_check = ACP_DMIC_AUTO;
>>  module_param(dmic_acpi_check, bint, 0644);
>> -MODULE_PARM_DESC(dmic_acpi_check, "checks Dmic hardware runtime");
>> +MODULE_PARM_DESC(dmic_acpi_check, "Digital microphone presence (-1=auto, 0=none, 1=force)");
>>  
>>  struct acp_dev_data {
>>  	void __iomem *acp_base;
>> @@ -163,6 +163,17 @@ static int rn_acp_deinit(void __iomem *acp_base)
>>  	return 0;
>>  }
>>  
>> +static const struct dmi_system_id rn_acp_quirk_table[] = {
>> +	{
>> +		/* Lenovo IdeaPad Flex 5 14ARE05, IdeaPad 5 15ARE05 */
>> +		.matches = {
>> +			DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "LENOVO"),
>> +			DMI_EXACT_MATCH(DMI_BOARD_NAME, "LNVNB161216"),
>> +		}
>> +	},
>> +	{}
>> +};
>> +
>>  static int snd_rn_acp_probe(struct pci_dev *pci,
>>  			    const struct pci_device_id *pci_id)
>>  {
>> @@ -172,6 +183,7 @@ static int snd_rn_acp_probe(struct pci_dev *pci,
>>  	acpi_handle handle;
>>  	acpi_integer dmic_status;
>>  #endif
>> +	const struct dmi_system_id *dmi_id;
>>  	unsigned int irqflags;
>>  	int ret, index;
>>  	u32 addr;
>> @@ -232,6 +244,12 @@ static int snd_rn_acp_probe(struct pci_dev *pci,
>>  			goto de_init;
>>  		}
>>  #endif
>> +		dmi_id = dmi_first_match(rn_acp_quirk_table);
>> +		if (dmi_id && !dmi_id->driver_data) {
>> +			dev_warn(&pci->dev, "ACPI settings override using DMI (ACP mic is not present)");
> 
> IMO, better to be dev_info() here.  It's the correct set up, hence
> it should be neither error nor warning that appears in the boot screen
> over the boot splash.

I sent v2 with this change. Thanks.

> BTW, both Raven and Reonir drivers point to the very same PCI ID,
> and both drivers will be probed for this machine (and both to be
> skipped).

It looks like a bad design. I believe that only one PCI module (driver) should
handle this PCI device and do the I2S / PDM redirection in the PCI probe callback.

> Also, I noticed that Renoir driver tries to detect the dmic at the
> late stage; this could be done at the very beginning, so the whole
> allocation and initialization could be simply skipped.  But this can
> be done in a separate cleanup patch.

Vijendar, could you give a look?

				Thanks,
					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ASoC: AMD Renoir - add DMI table to avoid the ACP mic probe (broken BIOS)

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 461 bytes --]

On Tue, Dec 08, 2020 at 05:24:32PM +0100, Takashi Iwai wrote:

> BTW, both Raven and Reonir drivers point to the very same PCI ID,
> and both drivers will be probed for this machine (and both to be
> skipped).

Ugh, that's not good.  It's not even super obvious from the code that
this is happening.  Seems like it should be one core driver which
instantiates the components for Raven and Reonir as appropriate, the PCI
driver is pretty thin at present anyway.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: AMD Renoir - add DMI table to avoid the ACP mic probe (broken BIOS)

[Mukunda,Vijendar]

On 08/12/20 11:10 pm, Mark Brown wrote:
> On Tue, Dec 08, 2020 at 05:24:32PM +0100, Takashi Iwai wrote:
> 
>> BTW, both Raven and Reonir drivers point to the very same PCI ID,
>> and both drivers will be probed for this machine (and both to be
>> skipped).
> 
> Ugh, that's not good.  It's not even super obvious from the code that
> this is happening.  Seems like it should be one core driver which
> instantiates the components for Raven and Reonir as appropriate, the PCI
> driver is pretty thin at present anyway.
> 

Raven and Renoir has same PCI ID but both platforms have different 
revision ID. Raven platform revision id is 0x00 where as for Renoir it 
is 0x01.

Re: [PATCH] ASoC: AMD Renoir - add DMI table to avoid the ACP mic probe (broken BIOS)

[Jaroslav Kysela]

Dne 08. 12. 20 v 19:06 Mukunda,Vijendar napsal(a):
> 
> 
> On 08/12/20 11:10 pm, Mark Brown wrote:
>> On Tue, Dec 08, 2020 at 05:24:32PM +0100, Takashi Iwai wrote:
>>
>>> BTW, both Raven and Reonir drivers point to the very same PCI ID,
>>> and both drivers will be probed for this machine (and both to be
>>> skipped).
>>
>> Ugh, that's not good.  It's not even super obvious from the code that
>> this is happening.  Seems like it should be one core driver which
>> instantiates the components for Raven and Reonir as appropriate, the PCI
>> driver is pretty thin at present anyway.
>>
> 
> Raven and Renoir has same PCI ID but both platforms have different 
> revision ID. Raven platform revision id is 0x00 where as for Renoir it 
> is 0x01.

I don't see pci->revision early check in the probe callbacks.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ASoC: AMD Renoir - add DMI table to avoid the ACP mic probe (broken BIOS)

[Takashi Iwai]

On Tue, 08 Dec 2020 19:06:21 +0100,
Mukunda,Vijendar wrote:
> 
> 
> 
> On 08/12/20 11:10 pm, Mark Brown wrote:
> > On Tue, Dec 08, 2020 at 05:24:32PM +0100, Takashi Iwai wrote:
> >
> >> BTW, both Raven and Reonir drivers point to the very same PCI ID,
> >> and both drivers will be probed for this machine (and both to be
> >> skipped).
> >
> > Ugh, that's not good.  It's not even super obvious from the code that
> > this is happening.  Seems like it should be one core driver which
> > instantiates the components for Raven and Reonir as appropriate, the PCI
> > driver is pretty thin at present anyway.
> >
> 
> Raven and Renoir has same PCI ID but both platforms have different
> revision ID. Raven platform revision id is 0x00 where as for Renoir it
> is 0x01.

But your drivers don't check the revision ID, as far as I see?

The linux PCI driver doesn't distinguish the revision id at the
matching time, unfortunately.


Takashi

Re: [PATCH] ASoC: AMD Renoir - add DMI table to avoid the ACP mic probe (broken BIOS)

[Mukunda,Vijendar]

On 08/12/20 11:27 pm, Takashi Iwai wrote:
> On Tue, 08 Dec 2020 19:06:21 +0100,
> Mukunda,Vijendar wrote:
>>
>>
>>
>> On 08/12/20 11:10 pm, Mark Brown wrote:
>>> On Tue, Dec 08, 2020 at 05:24:32PM +0100, Takashi Iwai wrote:
>>>
>>>> BTW, both Raven and Reonir drivers point to the very same PCI ID,
>>>> and both drivers will be probed for this machine (and both to be
>>>> skipped).
>>>
>>> Ugh, that's not good.  It's not even super obvious from the code that
>>> this is happening.  Seems like it should be one core driver which
>>> instantiates the components for Raven and Reonir as appropriate, the PCI
>>> driver is pretty thin at present anyway.
>>>
>>
>> Raven and Renoir has same PCI ID but both platforms have different
>> revision ID. Raven platform revision id is 0x00 where as for Renoir it
>> is 0x01.
> 
> But your drivers don't check the revision ID, as far as I see?
> 
> The linux PCI driver doesn't distinguish the revision id at the
> matching time, unfortunately.
> 
> 
> Takashi
>
Apart from Revision ID difference, There are few hardware differences
specific to ACP IP.
ACP IP hardware versions are different for Raven and Renoir.
Unfortunately we don't have specific logic to distinguish ACP hardware 
versions for Raven and Renoir.

Re: [PATCH] ASoC: AMD Renoir - add DMI table to avoid the ACP mic probe (broken BIOS)

[Mukunda,Vijendar]

On 08/12/20 11:51 pm, Mukunda,Vijendar wrote:
> 
> 
> On 08/12/20 11:27 pm, Takashi Iwai wrote:
>> On Tue, 08 Dec 2020 19:06:21 +0100,
>> Mukunda,Vijendar wrote:
>>>
>>>
>>>
>>> On 08/12/20 11:10 pm, Mark Brown wrote:
>>>> On Tue, Dec 08, 2020 at 05:24:32PM +0100, Takashi Iwai wrote:
>>>>
>>>>> BTW, both Raven and Reonir drivers point to the very same PCI ID,
>>>>> and both drivers will be probed for this machine (and both to be
>>>>> skipped).
>>>>
>>>> Ugh, that's not good.  It's not even super obvious from the code that
>>>> this is happening.  Seems like it should be one core driver which
>>>> instantiates the components for Raven and Reonir as appropriate, the 
>>>> PCI
>>>> driver is pretty thin at present anyway.
>>>>
>>>
>>> Raven and Renoir has same PCI ID but both platforms have different
>>> revision ID. Raven platform revision id is 0x00 where as for Renoir it
>>> is 0x01.
>>
>> But your drivers don't check the revision ID, as far as I see?
>>
>> The linux PCI driver doesn't distinguish the revision id at the
>> matching time, unfortunately.
>>
>>
>> Takashi
>>
> Apart from Revision ID difference, There are few hardware differences
> specific to ACP IP.
> ACP IP hardware versions are different for Raven and Renoir.
> Unfortunately we don't have specific logic to distinguish ACP hardware 
> versions for Raven and Renoir.
> 
But build wise both Raven and Renoir uses different Kconfig options.

Re: [PATCH] ASoC: AMD Renoir - add DMI table to avoid the ACP mic probe (broken BIOS)

[Jaroslav Kysela]

Dne 08. 12. 20 v 19:23 Mukunda,Vijendar napsal(a):
> 
> 
> On 08/12/20 11:51 pm, Mukunda,Vijendar wrote:
>>
>>
>> On 08/12/20 11:27 pm, Takashi Iwai wrote:
>>> On Tue, 08 Dec 2020 19:06:21 +0100,
>>> Mukunda,Vijendar wrote:
>>>>
>>>>
>>>>
>>>> On 08/12/20 11:10 pm, Mark Brown wrote:
>>>>> On Tue, Dec 08, 2020 at 05:24:32PM +0100, Takashi Iwai wrote:
>>>>>
>>>>>> BTW, both Raven and Reonir drivers point to the very same PCI ID,
>>>>>> and both drivers will be probed for this machine (and both to be
>>>>>> skipped).
>>>>>
>>>>> Ugh, that's not good.  It's not even super obvious from the code that
>>>>> this is happening.  Seems like it should be one core driver which
>>>>> instantiates the components for Raven and Reonir as appropriate, the 
>>>>> PCI
>>>>> driver is pretty thin at present anyway.
>>>>>
>>>>
>>>> Raven and Renoir has same PCI ID but both platforms have different
>>>> revision ID. Raven platform revision id is 0x00 where as for Renoir it
>>>> is 0x01.
>>>
>>> But your drivers don't check the revision ID, as far as I see?
>>>
>>> The linux PCI driver doesn't distinguish the revision id at the
>>> matching time, unfortunately.
>>>
>>>
>>> Takashi
>>>
>> Apart from Revision ID difference, There are few hardware differences
>> specific to ACP IP.
>> ACP IP hardware versions are different for Raven and Renoir.
>> Unfortunately we don't have specific logic to distinguish ACP hardware 
>> versions for Raven and Renoir.
>>
> But build wise both Raven and Renoir uses different Kconfig options.

We need to build all drivers for the universal distros to one kernel. The
Kconfig does not help here.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ASoC: AMD Renoir - add DMI table to avoid the ACP mic probe (broken BIOS)

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

On Tue, Dec 08, 2020 at 07:14:06PM +0100, Jaroslav Kysela wrote:
> Dne 08. 12. 20 v 19:23 Mukunda,Vijendar napsal(a):

> > But build wise both Raven and Renoir uses different Kconfig options.

> We need to build all drivers for the universal distros to one kernel. The
> Kconfig does not help here.

Yeah, even for embedded targets like phones this is getting less and
less useful over time - for systems with PCI like this people really
want a single kernel image for every user, not per-board images.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]