[RFC] ASoC: Intel: use common helpers to detect CPUs

Message ID	20190528200255.15923-1-pierre-louis.bossart@linux.intel.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <alsa-devel-bounces@alsa-project.org> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> To: alsa-devel@alsa-project.org Date: Tue, 28 May 2019 15:02:55 -0500 Message-Id: <20190528200255.15923-1-pierre-louis.bossart@linux.intel.com> MIME-Version: 1.0 Cc: tiwai@suse.de, broonie@kernel.org, Randy Dunlap <rdunlap@infradead.org>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Subject: [alsa-devel] [RFC PATCH] ASoC: Intel: use common helpers to detect CPUs Precedence: list Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" <alsa-devel-bounces@alsa-project.org>
Series	[RFC] ASoC: Intel: use common helpers to detect CPUs \| expand [RFC] ASoC: Intel: use common helpers to detect CPUs

Pierre-Louis Bossart May 28, 2019, 8:02 p.m. UTC

We have duplicated code in multiple locations (atom, machine drivers,
SOF) to detect Baytrail, Cherrytrail and other SOCs. This is not very
elegant, and introduces dependencies on CONFIG_X86 that prevent
COMPILE_TEST from working.

Add common helpers to provide same functionality in a cleaner
way. This will also help support the DMI-based quirks being introduced
to handle SOF/SST autodetection.

FIXME:
0. does this fix COMPILE_TEST issues reported by Randy?

1. should the include file moved to include/sound? this would be handy
for the SOF/SST mutual exclusion?

2. is there a better way to do this for all Intel chips or do we keep
this in sound/? Andy?

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/atom/sst/sst_acpi.c           |  65 +---------
 sound/soc/intel/boards/bxt_da7219_max98357a.c |  11 +-
 sound/soc/intel/boards/bytcht_es8316.c        |  12 +-
 sound/soc/intel/boards/bytcr_rt5640.c         |  16 +--
 sound/soc/intel/boards/bytcr_rt5651.c         |  17 +--
 sound/soc/intel/boards/cht_bsw_rt5645.c       |  16 +--
 sound/soc/intel/boards/sof_rt5682.c           |  11 +-
 sound/soc/intel/common/soc-intel-quirks.h     | 115 ++++++++++++++++++
 sound/soc/sof/sof-acpi-dev.c                  |  57 +--------
 9 files changed, 135 insertions(+), 185 deletions(-)
 create mode 100644 sound/soc/intel/common/soc-intel-quirks.h

Takashi Iwai May 29, 2019, 5:18 a.m. UTC | #1

On Tue, 28 May 2019 22:02:55 +0200,
Pierre-Louis Bossart wrote:
> 
> We have duplicated code in multiple locations (atom, machine drivers,
> SOF) to detect Baytrail, Cherrytrail and other SOCs. This is not very
> elegant, and introduces dependencies on CONFIG_X86 that prevent
> COMPILE_TEST from working.
> 
> Add common helpers to provide same functionality in a cleaner
> way. This will also help support the DMI-based quirks being introduced
> to handle SOF/SST autodetection.
> 
> FIXME:
> 0. does this fix COMPILE_TEST issues reported by Randy?

I guess yes, but let's test it.

> 1. should the include file moved to include/sound? this would be handy
> for the SOF/SST mutual exclusion?

It'd make sense only if the file is used over the tree.
But in this case, it's specific to soc/intel, hence it's OK to leave
there.

> 2. is there a better way to do this for all Intel chips or do we keep
> this in sound/? Andy?

x86_match_cpu() looks pretty common, but of course it'd be better if
we have a simpler way...

> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

In anyway, feel free to take my r-b tag
  Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi


> ---
>  sound/soc/intel/atom/sst/sst_acpi.c           |  65 +---------
>  sound/soc/intel/boards/bxt_da7219_max98357a.c |  11 +-
>  sound/soc/intel/boards/bytcht_es8316.c        |  12 +-
>  sound/soc/intel/boards/bytcr_rt5640.c         |  16 +--
>  sound/soc/intel/boards/bytcr_rt5651.c         |  17 +--
>  sound/soc/intel/boards/cht_bsw_rt5645.c       |  16 +--
>  sound/soc/intel/boards/sof_rt5682.c           |  11 +-
>  sound/soc/intel/common/soc-intel-quirks.h     | 115 ++++++++++++++++++
>  sound/soc/sof/sof-acpi-dev.c                  |  57 +--------
>  9 files changed, 135 insertions(+), 185 deletions(-)
>  create mode 100644 sound/soc/intel/common/soc-intel-quirks.h
> 
> diff --git a/sound/soc/intel/atom/sst/sst_acpi.c b/sound/soc/intel/atom/sst/sst_acpi.c
> index ae17ce4677a5..06c4a2da900c 100644
> --- a/sound/soc/intel/atom/sst/sst_acpi.c
> +++ b/sound/soc/intel/atom/sst/sst_acpi.c
> @@ -38,12 +38,11 @@
>  #include <acpi/platform/aclinux.h>
>  #include <acpi/actypes.h>
>  #include <acpi/acpi_bus.h>
> -#include <asm/cpu_device_id.h>
> -#include <asm/iosf_mbi.h>
>  #include <sound/soc-acpi.h>
>  #include <sound/soc-acpi-intel-match.h>
>  #include "../sst-mfld-platform.h"
>  #include "../../common/sst-dsp.h"
> +#include "../../common/soc-intel-quirks.h"
>  #include "sst.h"
>  
>  /* LPE viewpoint addresses */
> @@ -243,64 +242,6 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
>  	return 0;
>  }
>  
> -static int is_byt(void)
> -{
> -	bool status = false;
> -	static const struct x86_cpu_id cpu_ids[] = {
> -		{ X86_VENDOR_INTEL, 6, 55 }, /* Valleyview, Bay Trail */
> -		{}
> -	};
> -	if (x86_match_cpu(cpu_ids))
> -		status = true;
> -	return status;
> -}
> -
> -static bool is_byt_cr(struct platform_device *pdev)
> -{
> -	struct device *dev = &pdev->dev;
> -	int status = 0;
> -
> -	if (!is_byt())
> -		return false;
> -
> -	if (iosf_mbi_available()) {
> -		u32 bios_status;
> -		status = iosf_mbi_read(BT_MBI_UNIT_PMC, /* 0x04 PUNIT */
> -				       MBI_REG_READ, /* 0x10 */
> -				       0x006, /* BIOS_CONFIG */
> -				       &bios_status);
> -
> -		if (status) {
> -			dev_err(dev, "could not read PUNIT BIOS_CONFIG\n");
> -		} else {
> -			/* bits 26:27 mirror PMIC options */
> -			bios_status = (bios_status >> 26) & 3;
> -
> -			if (bios_status == 1 || bios_status == 3) {
> -				dev_info(dev, "Detected Baytrail-CR platform\n");
> -				return true;
> -			}
> -
> -			dev_info(dev, "BYT-CR not detected\n");
> -		}
> -	} else {
> -		dev_info(dev, "IOSF_MBI not available, no BYT-CR detection\n");
> -	}
> -
> -	if (platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) {
> -		/*
> -		 * Some devices detected as BYT-T have only a single IRQ listed,
> -		 * causing platform_get_irq with index 5 to return -ENXIO.
> -		 * The correct IRQ in this case is at index 0, as on BYT-CR.
> -		 */
> -		dev_info(dev, "Falling back to Baytrail-CR platform\n");
> -		return true;
> -	}
> -
> -	return false;
> -}
> -
> -
>  static int sst_acpi_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -325,7 +266,7 @@ static int sst_acpi_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> -	if (is_byt())
> +	if (soc_intel_is_byt())
>  		mach->pdata = &byt_rvp_platform_data;
>  	else
>  		mach->pdata = &chv_platform_data;
> @@ -343,7 +284,7 @@ static int sst_acpi_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (is_byt_cr(pdev)) {
> +	if (soc_intel_is_byt_cr(pdev)) {
>  		/* override resource info */
>  		byt_rvp_platform_data.res_info = &bytcr_res_info;
>  	}
> diff --git a/sound/soc/intel/boards/bxt_da7219_max98357a.c b/sound/soc/intel/boards/bxt_da7219_max98357a.c
> index d8c7d64bb1d7..d476c55f2efe 100644
> --- a/sound/soc/intel/boards/bxt_da7219_max98357a.c
> +++ b/sound/soc/intel/boards/bxt_da7219_max98357a.c
> @@ -16,7 +16,6 @@
>   * GNU General Public License for more details.
>   */
>  
> -#include <asm/cpu_device_id.h>
>  #include <linux/input.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> @@ -29,6 +28,7 @@
>  #include "../../codecs/hdac_hdmi.h"
>  #include "../../codecs/da7219.h"
>  #include "../../codecs/da7219-aad.h"
> +#include "../common/soc-intel-quirks.h"
>  
>  #define BXT_DIALOG_CODEC_DAI	"da7219-hifi"
>  #define BXT_MAXIM_CODEC_DAI	"HiFi"
> @@ -579,11 +579,6 @@ static struct snd_soc_dai_link broxton_dais[] = {
>  	},
>  };
>  
> -static const struct x86_cpu_id glk_ids[] = {
> -	{ X86_VENDOR_INTEL, 6, 0x7A }, /* Geminilake CPU_ID */
> -	{}
> -};
> -
>  #define NAME_SIZE	32
>  static int bxt_card_late_probe(struct snd_soc_card *card)
>  {
> @@ -593,7 +588,7 @@ static int bxt_card_late_probe(struct snd_soc_card *card)
>  	int err, i = 0;
>  	char jack_name[NAME_SIZE];
>  
> -	if (x86_match_cpu(glk_ids))
> +	if (soc_intel_is_glk())
>  		snd_soc_dapm_add_routes(&card->dapm, gemini_map,
>  					ARRAY_SIZE(gemini_map));
>  	else
> @@ -656,7 +651,7 @@ static int broxton_audio_probe(struct platform_device *pdev)
>  
>  	broxton_audio_card.dev = &pdev->dev;
>  	snd_soc_card_set_drvdata(&broxton_audio_card, ctx);
> -	if (x86_match_cpu(glk_ids)) {
> +	if (soc_intel_is_glk()) {
>  		unsigned int i;
>  
>  		broxton_audio_card.name = "glkda7219max";
> diff --git a/sound/soc/intel/boards/bytcht_es8316.c b/sound/soc/intel/boards/bytcht_es8316.c
> index e8c585ffd04d..d08715ac3945 100644
> --- a/sound/soc/intel/boards/bytcht_es8316.c
> +++ b/sound/soc/intel/boards/bytcht_es8316.c
> @@ -30,8 +30,6 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> -#include <asm/cpu_device_id.h>
> -#include <asm/intel-family.h>
>  #include <asm/platform_sst_audio.h>
>  #include <sound/jack.h>
>  #include <sound/pcm.h>
> @@ -40,6 +38,7 @@
>  #include <sound/soc-acpi.h>
>  #include "../atom/sst-atom-controls.h"
>  #include "../common/sst-dsp.h"
> +#include "../common/soc-intel-quirks.h"
>  
>  /* jd-inv + terminating entry */
>  #define MAX_NO_PROPS 2
> @@ -430,11 +429,6 @@ static struct snd_soc_card byt_cht_es8316_card = {
>  	.resume_post = byt_cht_es8316_resume,
>  };
>  
> -static const struct x86_cpu_id baytrail_cpu_ids[] = {
> -	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_SILVERMONT }, /* Valleyview */
> -	{}
> -};
> -
>  static const struct acpi_gpio_params first_gpio = { 0, 0, false };
>  
>  static const struct acpi_gpio_mapping byt_cht_es8316_gpios[] = {
> @@ -506,8 +500,8 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
>  	dmi_id = dmi_first_match(byt_cht_es8316_quirk_table);
>  	if (dmi_id) {
>  		quirk = (unsigned long)dmi_id->driver_data;
> -	} else if (x86_match_cpu(baytrail_cpu_ids) &&
> -	    mach->mach_params.acpi_ipc_irq_index == 0) {
> +	} else if (soc_intel_is_byt() &&
> +		   mach->mach_params.acpi_ipc_irq_index == 0) {
>  		/* On BYTCR default to SSP0, internal-mic-in2-map, mono-spk */
>  		quirk = BYT_CHT_ES8316_SSP0 | BYT_CHT_ES8316_INTMIC_IN2_MAP |
>  			BYT_CHT_ES8316_MONO_SPEAKER;
> diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
> index dc22df9a99fb..7aae7b78efba 100644
> --- a/sound/soc/intel/boards/bytcr_rt5640.c
> +++ b/sound/soc/intel/boards/bytcr_rt5640.c
> @@ -28,7 +28,6 @@
>  #include <linux/dmi.h>
>  #include <linux/input.h>
>  #include <linux/slab.h>
> -#include <asm/cpu_device_id.h>
>  #include <sound/pcm.h>
>  #include <sound/pcm_params.h>
>  #include <sound/soc.h>
> @@ -38,6 +37,7 @@
>  #include "../../codecs/rt5640.h"
>  #include "../atom/sst-atom-controls.h"
>  #include "../common/sst-dsp.h"
> +#include "../common/soc-intel-quirks.h"
>  
>  enum {
>  	BYT_RT5640_DMIC1_MAP,
> @@ -1130,18 +1130,6 @@ static struct snd_soc_card byt_rt5640_card = {
>  	.resume_post = byt_rt5640_resume,
>  };
>  
> -static bool is_valleyview(void)
> -{
> -	static const struct x86_cpu_id cpu_ids[] = {
> -		{ X86_VENDOR_INTEL, 6, 55 }, /* Valleyview, Bay Trail */
> -		{}
> -	};
> -
> -	if (!x86_match_cpu(cpu_ids))
> -		return false;
> -	return true;
> -}
> -
>  struct acpi_chan_package {   /* ACPICA seems to require 64 bit integers */
>  	u64 aif_value;       /* 1: AIF1, 2: AIF2 */
>  	u64 mclock_value;    /* usually 25MHz (0x17d7940), ignored */
> @@ -1190,7 +1178,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
>  	 * swap SSP0 if bytcr is detected
>  	 * (will be overridden if DMI quirk is detected)
>  	 */
> -	if (is_valleyview()) {
> +	if (soc_intel_is_byt()) {
>  		if (mach->mach_params.acpi_ipc_irq_index == 0)
>  			is_bytcr = true;
>  	}
> diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
> index ca657c3e5726..6df6435ea394 100644
> --- a/sound/soc/intel/boards/bytcr_rt5651.c
> +++ b/sound/soc/intel/boards/bytcr_rt5651.c
> @@ -30,8 +30,6 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/gpio/machine.h>
>  #include <linux/slab.h>
> -#include <asm/cpu_device_id.h>
> -#include <asm/intel-family.h>
>  #include <sound/pcm.h>
>  #include <sound/pcm_params.h>
>  #include <sound/soc.h>
> @@ -39,6 +37,7 @@
>  #include <sound/soc-acpi.h>
>  #include "../../codecs/rt5651.h"
>  #include "../atom/sst-atom-controls.h"
> +#include "../common/soc-intel-quirks.h"
>  
>  enum {
>  	BYT_RT5651_DMIC_MAP,
> @@ -852,16 +851,6 @@ static struct snd_soc_card byt_rt5651_card = {
>  	.resume_post = byt_rt5651_resume,
>  };
>  
> -static const struct x86_cpu_id baytrail_cpu_ids[] = {
> -	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_SILVERMONT }, /* Valleyview */
> -	{}
> -};
> -
> -static const struct x86_cpu_id cherrytrail_cpu_ids[] = {
> -	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_AIRMONT },     /* Braswell */
> -	{}
> -};
> -
>  static const struct acpi_gpio_params ext_amp_enable_gpios = { 0, 0, false };
>  
>  static const struct acpi_gpio_mapping cht_rt5651_gpios[] = {
> @@ -932,7 +921,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
>  	 * swap SSP0 if bytcr is detected
>  	 * (will be overridden if DMI quirk is detected)
>  	 */
> -	if (x86_match_cpu(baytrail_cpu_ids)) {
> +	if (soc_intel_is_byt()) {
>  		if (mach->mach_params.acpi_ipc_irq_index == 0)
>  			is_bytcr = true;
>  	}
> @@ -1001,7 +990,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Cherry Trail devices use an external amplifier enable gpio */
> -	if (x86_match_cpu(cherrytrail_cpu_ids) && !byt_rt5651_gpios)
> +	if (soc_intel_is_cht() && !byt_rt5651_gpios)
>  		byt_rt5651_gpios = cht_rt5651_gpios;
>  
>  	if (byt_rt5651_gpios) {
> diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c b/sound/soc/intel/boards/cht_bsw_rt5645.c
> index 32dbeaf1ab94..de5fe58ae3b4 100644
> --- a/sound/soc/intel/boards/cht_bsw_rt5645.c
> +++ b/sound/soc/intel/boards/cht_bsw_rt5645.c
> @@ -26,7 +26,6 @@
>  #include <linux/clk.h>
>  #include <linux/dmi.h>
>  #include <linux/slab.h>
> -#include <asm/cpu_device_id.h>
>  #include <sound/pcm.h>
>  #include <sound/pcm_params.h>
>  #include <sound/soc.h>
> @@ -34,6 +33,7 @@
>  #include <sound/soc-acpi.h>
>  #include "../../codecs/rt5645.h"
>  #include "../atom/sst-atom-controls.h"
> +#include "../common/soc-intel-quirks.h"
>  
>  #define CHT_PLAT_CLK_3_HZ	19200000
>  #define CHT_CODEC_DAI1	"rt5645-aif1"
> @@ -509,18 +509,6 @@ static char cht_rt5645_codec_name[SND_ACPI_I2C_ID_LEN];
>  static char cht_rt5645_codec_aif_name[12]; /*  = "rt5645-aif[1|2]" */
>  static char cht_rt5645_cpu_dai_name[10]; /*  = "ssp[0|2]-port" */
>  
> -static bool is_valleyview(void)
> -{
> -	static const struct x86_cpu_id cpu_ids[] = {
> -		{ X86_VENDOR_INTEL, 6, 55 }, /* Valleyview, Bay Trail */
> -		{}
> -	};
> -
> -	if (!x86_match_cpu(cpu_ids))
> -		return false;
> -	return true;
> -}
> -
>  struct acpi_chan_package {   /* ACPICA seems to require 64 bit integers */
>  	u64 aif_value;       /* 1: AIF1, 2: AIF2 */
>  	u64 mclock_value;    /* usually 25MHz (0x17d7940), ignored */
> @@ -585,7 +573,7 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
>  	 * swap SSP0 if bytcr is detected
>  	 * (will be overridden if DMI quirk is detected)
>  	 */
> -	if (is_valleyview()) {
> +	if (soc_intel_is_byt()) {
>  		if (mach->mach_params.acpi_ipc_irq_index == 0)
>  			is_bytcr = true;
>  	}
> diff --git a/sound/soc/intel/boards/sof_rt5682.c b/sound/soc/intel/boards/sof_rt5682.c
> index e441dc979966..355fd9730a44 100644
> --- a/sound/soc/intel/boards/sof_rt5682.c
> +++ b/sound/soc/intel/boards/sof_rt5682.c
> @@ -10,8 +10,6 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/dmi.h>
> -#include <asm/cpu_device_id.h>
> -#include <asm/intel-family.h>
>  #include <sound/core.h>
>  #include <sound/jack.h>
>  #include <sound/pcm.h>
> @@ -21,6 +19,7 @@
>  #include <sound/soc-acpi.h>
>  #include "../../codecs/rt5682.h"
>  #include "../../codecs/hdac_hdmi.h"
> +#include "../common/soc-intel-quirks.h"
>  
>  #define NAME_SIZE 32
>  
> @@ -304,12 +303,6 @@ static struct snd_soc_card sof_audio_card_rt5682 = {
>  	.late_probe = sof_card_late_probe,
>  };
>  
> -static const struct x86_cpu_id legacy_cpi_ids[] = {
> -	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_SILVERMONT }, /* Baytrail */
> -	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_AIRMONT }, /* Cherrytrail */
> -	{}
> -};
> -
>  static struct snd_soc_dai_link_component rt5682_component[] = {
>  	{
>  		.name = "i2c-10EC5682:00",
> @@ -498,7 +491,7 @@ static int sof_audio_probe(struct platform_device *pdev)
>  	if (!ctx)
>  		return -ENOMEM;
>  
> -	if (x86_match_cpu(legacy_cpi_ids)) {
> +	if (soc_intel_is_byt() || soc_intel_is_cht()) {
>  		is_legacy_cpu = 1;
>  		dmic_num = 0;
>  		hdmi_num = 0;
> diff --git a/sound/soc/intel/common/soc-intel-quirks.h b/sound/soc/intel/common/soc-intel-quirks.h
> new file mode 100644
> index 000000000000..4718fd3cf636
> --- /dev/null
> +++ b/sound/soc/intel/common/soc-intel-quirks.h
> @@ -0,0 +1,115 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * soc-intel-quirks.h - prototypes for quirk autodetection
> + *
> + * Copyright (c) 2019, Intel Corporation.
> + *
> + */
> +
> +#ifndef _SND_SOC_INTEL_QUIRKS_H
> +#define _SND_SOC_INTEL_QUIRKS_H
> +
> +#if IS_ENABLED(CONFIG_X86)
> +
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +#include <asm/iosf_mbi.h>
> +
> +#define ICPU(model)	{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, }
> +
> +#define SOC_INTEL_IS_CPU(soc, type)				\
> +static inline bool soc_intel_is_##soc(void)			\
> +{								\
> +	static const struct x86_cpu_id soc##_cpu_ids[] = {	\
> +		ICPU(type),					\
> +		{}						\
> +	};							\
> +	const struct x86_cpu_id *id;				\
> +								\
> +	id = x86_match_cpu(soc##_cpu_ids);			\
> +	if (id)							\
> +		return true;					\
> +	return false;						\
> +}
> +
> +SOC_INTEL_IS_CPU(byt, INTEL_FAM6_ATOM_SILVERMONT);
> +SOC_INTEL_IS_CPU(cht, INTEL_FAM6_ATOM_AIRMONT);
> +SOC_INTEL_IS_CPU(apl, INTEL_FAM6_ATOM_GOLDMONT);
> +SOC_INTEL_IS_CPU(glk, INTEL_FAM6_ATOM_GOLDMONT_PLUS);
> +
> +static inline bool soc_intel_is_byt_cr(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int status = 0;
> +
> +	if (!soc_intel_is_byt())
> +		return false;
> +
> +	if (iosf_mbi_available()) {
> +		u32 bios_status;
> +
> +		status = iosf_mbi_read(BT_MBI_UNIT_PMC, /* 0x04 PUNIT */
> +				       MBI_REG_READ, /* 0x10 */
> +				       0x006, /* BIOS_CONFIG */
> +				       &bios_status);
> +
> +		if (status) {
> +			dev_err(dev, "could not read PUNIT BIOS_CONFIG\n");
> +		} else {
> +			/* bits 26:27 mirror PMIC options */
> +			bios_status = (bios_status >> 26) & 3;
> +
> +			if (bios_status == 1 || bios_status == 3) {
> +				dev_info(dev, "Detected Baytrail-CR platform\n");
> +				return true;
> +			}
> +
> +			dev_info(dev, "BYT-CR not detected\n");
> +		}
> +	} else {
> +		dev_info(dev, "IOSF_MBI not available, no BYT-CR detection\n");
> +	}
> +
> +	if (!platform_get_resource(pdev, IORESOURCE_IRQ, 5)) {
> +		/*
> +		 * Some devices detected as BYT-T have only a single IRQ listed,
> +		 * causing platform_get_irq with index 5 to return -ENXIO.
> +		 * The correct IRQ in this case is at index 0, as on BYT-CR.
> +		 */
> +		dev_info(dev, "Falling back to Baytrail-CR platform\n");
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +#else
> +
> +static inline bool soc_intel_is_byt_cr(struct platform_device *pdev)
> +{
> +	return false;
> +}
> +
> +static inline bool soc_intel_is_byt(void)
> +{
> +	return false;
> +}
> +
> +static inline bool soc_intel_is_cht(void)
> +{
> +	return false;
> +}
> +
> +static inline bool soc_intel_is_apl(void)
> +{
> +	return false;
> +}
> +
> +static inline bool soc_intel_is_glk(void)
> +{
> +	return false;
> +}
> +
> +#endif
> +
> + #endif /* _SND_SOC_INTEL_QUIRKS_H */
> diff --git a/sound/soc/sof/sof-acpi-dev.c b/sound/soc/sof/sof-acpi-dev.c
> index 38062dd00dd2..93a8e15bbd2c 100644
> --- a/sound/soc/sof/sof-acpi-dev.c
> +++ b/sound/soc/sof/sof-acpi-dev.c
> @@ -15,10 +15,7 @@
>  #include <sound/soc-acpi.h>
>  #include <sound/soc-acpi-intel-match.h>
>  #include <sound/sof.h>
> -#ifdef CONFIG_X86
> -#include <asm/iosf_mbi.h>
> -#endif
> -
> +#include "../intel/common/soc-intel-quirks.h"
>  #include "ops.h"
>  
>  /* platform specific devices */
> @@ -105,56 +102,6 @@ static const struct sof_dev_desc sof_acpi_baytrail_desc = {
>  	.arch_ops = &sof_xtensa_arch_ops
>  };
>  
> -#ifdef CONFIG_X86 /* TODO: move this to common helper */
> -
> -static bool is_byt_cr(struct platform_device *pdev)
> -{
> -	struct device *dev = &pdev->dev;
> -	int status;
> -
> -	if (iosf_mbi_available()) {
> -		u32 bios_status;
> -		status = iosf_mbi_read(BT_MBI_UNIT_PMC, /* 0x04 PUNIT */
> -				       MBI_REG_READ, /* 0x10 */
> -				       0x006, /* BIOS_CONFIG */
> -				       &bios_status);
> -
> -		if (status) {
> -			dev_err(dev, "could not read PUNIT BIOS_CONFIG\n");
> -		} else {
> -			/* bits 26:27 mirror PMIC options */
> -			bios_status = (bios_status >> 26) & 3;
> -
> -			if (bios_status == 1 || bios_status == 3) {
> -				dev_info(dev, "Detected Baytrail-CR platform\n");
> -				return true;
> -			}
> -
> -			dev_info(dev, "BYT-CR not detected\n");
> -		}
> -	} else {
> -		dev_info(dev, "IOSF_MBI not available, no BYT-CR detection\n");
> -	}
> -
> -	if (platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) {
> -		/*
> -		 * Some devices detected as BYT-T have only a single IRQ listed,
> -		 * causing platform_get_irq with index 5 to return -ENXIO.
> -		 * The correct IRQ in this case is at index 0, as on BYT-CR.
> -		 */
> -		dev_info(dev, "Falling back to Baytrail-CR platform\n");
> -		return true;
> -	}
> -
> -	return false;
> -}
> -#else
> -static int is_byt_cr(struct platform_device *pdev)
> -{
> -	return 0;
> -}
> -#endif
> -
>  static const struct sof_dev_desc sof_acpi_cherrytrail_desc = {
>  	.machines = snd_soc_acpi_intel_cherrytrail_machines,
>  	.resindex_lpe_base = 0,
> @@ -209,7 +156,7 @@ static int sof_acpi_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  
>  #if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
> -	if (desc == &sof_acpi_baytrail_desc && is_byt_cr(pdev))
> +	if (desc == &sof_acpi_baytrail_desc && soc_intel_is_byt_cr(pdev))
>  		desc = &sof_acpi_baytrailcr_desc;
>  #endif
>  
> -- 
> 2.20.1
>

Mark Brown June 17, 2019, 2:47 p.m. UTC | #2

On Tue, May 28, 2019 at 03:02:55PM -0500, Pierre-Louis Bossart wrote:

> 2. is there a better way to do this for all Intel chips or do we keep
> this in sound/? Andy?

ARM has platform detection stuff in the architecture code, something
similar seems sensible for x86?

Pierre-Louis Bossart June 17, 2019, 3:31 p.m. UTC | #3

On 6/17/19 4:47 PM, Mark Brown wrote:
> On Tue, May 28, 2019 at 03:02:55PM -0500, Pierre-Louis Bossart wrote:
> 
>> 2. is there a better way to do this for all Intel chips or do we keep
>> this in sound/? Andy?
> 
> ARM has platform detection stuff in the architecture code, something
> similar seems sensible for x86?

Well yes, we already have x86_match_cpu() but that won't work with 
COMPILE_TEST (asm/ headers don't exist) and this leads to duplication of 
code. All we really need here is a yeah/nay answer from a help that 
hides those details away.

Andy Shevchenko June 17, 2019, 4:18 p.m. UTC | #4

On Mon, Jun 17, 2019 at 05:31:53PM +0200, Pierre-Louis Bossart wrote:
> On 6/17/19 4:47 PM, Mark Brown wrote:
> > On Tue, May 28, 2019 at 03:02:55PM -0500, Pierre-Louis Bossart wrote:
> > 
> > > 2. is there a better way to do this for all Intel chips or do we keep
> > > this in sound/? Andy?

It's better to discuss with x86 maintainers.

> > ARM has platform detection stuff in the architecture code, something
> > similar seems sensible for x86?
> 
> Well yes, we already have x86_match_cpu() but that won't work with
> COMPILE_TEST (asm/ headers don't exist) and this leads to duplication of
> code. All we really need here is a yeah/nay answer from a help that hides
> those details away.

I don't see much advantage here. Without specific driver data it will be
degraded to something like:

	if (bootcpu.model == INTEL_CPU_...)
		...

with slight exception to heterogeneous SoCs.

In order to be compile tested we might introduce a header under
include/platform_data/x86 with these inliners like:

static inline bool is_x86_model_XX(void)
{
	return bootcpu.model == XX; // it might be done in more generic way?
}

Amadeusz Sławiński June 18, 2019, 11:02 a.m. UTC | #5

On Mon, 17 Jun 2019 19:18:27 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Jun 17, 2019 at 05:31:53PM +0200, Pierre-Louis Bossart wrote:
> > On 6/17/19 4:47 PM, Mark Brown wrote:  
> > > On Tue, May 28, 2019 at 03:02:55PM -0500, Pierre-Louis Bossart
> > > wrote: 
> > > > 2. is there a better way to do this for all Intel chips or do
> > > > we keep this in sound/? Andy?  
> 
> It's better to discuss with x86 maintainers.
> 
> > > ARM has platform detection stuff in the architecture code,
> > > something similar seems sensible for x86?  
> > 
> > Well yes, we already have x86_match_cpu() but that won't work with
> > COMPILE_TEST (asm/ headers don't exist) and this leads to
> > duplication of code. All we really need here is a yeah/nay answer
> > from a help that hides those details away.  
> 
> I don't see much advantage here. Without specific driver data it will
> be degraded to something like:
> 
> 	if (bootcpu.model == INTEL_CPU_...)
> 		...
> 
> with slight exception to heterogeneous SoCs.
> 
> In order to be compile tested we might introduce a header under
> include/platform_data/x86 with these inliners like:
> 
> static inline bool is_x86_model_XX(void)
> {
> 	return bootcpu.model == XX; // it might be done in more
> generic way? }
> 

You might also want to look at other drivers that do some kind of
platform detection.

There is:
tools/power/x86/turbostat/turbostat.c
which has few is_xxx() functions, and:
drivers/gpu/drm/i915/i915_drv.h
which has quite a lot of IS_XXX macros, although they are used to
detect PCI VGA devices, but maybe some code could be shared, with
separate device specific ids.

Amadeusz

Cezary Rojewski June 18, 2019, 7:26 p.m. UTC | #6

On 2019-05-28 22:02, Pierre-Louis Bossart wrote:
> We have duplicated code in multiple locations (atom, machine drivers,
> SOF) to detect Baytrail, Cherrytrail and other SOCs. This is not very
> elegant, and introduces dependencies on CONFIG_X86 that prevent
> COMPILE_TEST from working.
> 
> Add common helpers to provide same functionality in a cleaner
> way. This will also help support the DMI-based quirks being introduced
> to handle SOF/SST autodetection.
> 
> FIXME:
> 0. does this fix COMPILE_TEST issues reported by Randy?
> 
> 1. should the include file moved to include/sound? this would be handy
> for the SOF/SST mutual exclusion?
> 
> 2. is there a better way to do this for all Intel chips or do we keep
> this in sound/? Andy?
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>   sound/soc/intel/atom/sst/sst_acpi.c           |  65 +---------
>   sound/soc/intel/boards/bxt_da7219_max98357a.c |  11 +-
>   sound/soc/intel/boards/bytcht_es8316.c        |  12 +-
>   sound/soc/intel/boards/bytcr_rt5640.c         |  16 +--
>   sound/soc/intel/boards/bytcr_rt5651.c         |  17 +--
>   sound/soc/intel/boards/cht_bsw_rt5645.c       |  16 +--
>   sound/soc/intel/boards/sof_rt5682.c           |  11 +-
>   sound/soc/intel/common/soc-intel-quirks.h     | 115 ++++++++++++++++++
>   sound/soc/sof/sof-acpi-dev.c                  |  57 +--------
>   9 files changed, 135 insertions(+), 185 deletions(-)
>   create mode 100644 sound/soc/intel/common/soc-intel-quirks.h
> 
> diff --git a/sound/soc/intel/atom/sst/sst_acpi.c b/sound/soc/intel/atom/sst/sst_acpi.c
> index ae17ce4677a5..06c4a2da900c 100644
> --- a/sound/soc/intel/atom/sst/sst_acpi.c
> +++ b/sound/soc/intel/atom/sst/sst_acpi.c
> @@ -38,12 +38,11 @@
>   #include <acpi/platform/aclinux.h>
>   #include <acpi/actypes.h>
>   #include <acpi/acpi_bus.h>
> -#include <asm/cpu_device_id.h>
> -#include <asm/iosf_mbi.h>
>   #include <sound/soc-acpi.h>
>   #include <sound/soc-acpi-intel-match.h>
>   #include "../sst-mfld-platform.h"
>   #include "../../common/sst-dsp.h"
> +#include "../../common/soc-intel-quirks.h"
>   #include "sst.h"
>   
>   /* LPE viewpoint addresses */
> @@ -243,64 +242,6 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
>   	return 0;
>   }
>   
> -static int is_byt(void)
> -{
> -	bool status = false;
> -	static const struct x86_cpu_id cpu_ids[] = {
> -		{ X86_VENDOR_INTEL, 6, 55 }, /* Valleyview, Bay Trail */
> -		{}
> -	};
> -	if (x86_match_cpu(cpu_ids))
> -		status = true;
> -	return status;
> -}
> -
> -static bool is_byt_cr(struct platform_device *pdev)
> -{
> -	struct device *dev = &pdev->dev;
> -	int status = 0;
> -
> -	if (!is_byt())
> -		return false;
> -
> -	if (iosf_mbi_available()) {
> -		u32 bios_status;
> -		status = iosf_mbi_read(BT_MBI_UNIT_PMC, /* 0x04 PUNIT */
> -				       MBI_REG_READ, /* 0x10 */
> -				       0x006, /* BIOS_CONFIG */
> -				       &bios_status);
> -
> -		if (status) {
> -			dev_err(dev, "could not read PUNIT BIOS_CONFIG\n");
> -		} else {
> -			/* bits 26:27 mirror PMIC options */
> -			bios_status = (bios_status >> 26) & 3;
> -
> -			if (bios_status == 1 || bios_status == 3) {
> -				dev_info(dev, "Detected Baytrail-CR platform\n");
> -				return true;
> -			}
> -
> -			dev_info(dev, "BYT-CR not detected\n");
> -		}
> -	} else {
> -		dev_info(dev, "IOSF_MBI not available, no BYT-CR detection\n");
> -	}
> -
> -	if (platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) {
> -		/*
> -		 * Some devices detected as BYT-T have only a single IRQ listed,
> -		 * causing platform_get_irq with index 5 to return -ENXIO.
> -		 * The correct IRQ in this case is at index 0, as on BYT-CR.
> -		 */
> -		dev_info(dev, "Falling back to Baytrail-CR platform\n");
> -		return true;
> -	}
> -
> -	return false;
> -}
> -
> -
>   static int sst_acpi_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -325,7 +266,7 @@ static int sst_acpi_probe(struct platform_device *pdev)
>   		return -ENODEV;
>   	}
>   
> -	if (is_byt())
> +	if (soc_intel_is_byt())
>   		mach->pdata = &byt_rvp_platform_data;
>   	else
>   		mach->pdata = &chv_platform_data;
> @@ -343,7 +284,7 @@ static int sst_acpi_probe(struct platform_device *pdev)
>   	if (ret < 0)
>   		return ret;
>   
> -	if (is_byt_cr(pdev)) {
> +	if (soc_intel_is_byt_cr(pdev)) {
>   		/* override resource info */
>   		byt_rvp_platform_data.res_info = &bytcr_res_info;
>   	}
> diff --git a/sound/soc/intel/boards/bxt_da7219_max98357a.c b/sound/soc/intel/boards/bxt_da7219_max98357a.c
> index d8c7d64bb1d7..d476c55f2efe 100644
> --- a/sound/soc/intel/boards/bxt_da7219_max98357a.c
> +++ b/sound/soc/intel/boards/bxt_da7219_max98357a.c
> @@ -16,7 +16,6 @@
>    * GNU General Public License for more details.
>    */
>   
> -#include <asm/cpu_device_id.h>
>   #include <linux/input.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
> @@ -29,6 +28,7 @@
>   #include "../../codecs/hdac_hdmi.h"
>   #include "../../codecs/da7219.h"
>   #include "../../codecs/da7219-aad.h"
> +#include "../common/soc-intel-quirks.h"
>   
>   #define BXT_DIALOG_CODEC_DAI	"da7219-hifi"
>   #define BXT_MAXIM_CODEC_DAI	"HiFi"
> @@ -579,11 +579,6 @@ static struct snd_soc_dai_link broxton_dais[] = {
>   	},
>   };
>   
> -static const struct x86_cpu_id glk_ids[] = {
> -	{ X86_VENDOR_INTEL, 6, 0x7A }, /* Geminilake CPU_ID */
> -	{}
> -};
> -
>   #define NAME_SIZE	32
>   static int bxt_card_late_probe(struct snd_soc_card *card)
>   {
> @@ -593,7 +588,7 @@ static int bxt_card_late_probe(struct snd_soc_card *card)
>   	int err, i = 0;
>   	char jack_name[NAME_SIZE];
>   
> -	if (x86_match_cpu(glk_ids))
> +	if (soc_intel_is_glk())
>   		snd_soc_dapm_add_routes(&card->dapm, gemini_map,
>   					ARRAY_SIZE(gemini_map));
>   	else
> @@ -656,7 +651,7 @@ static int broxton_audio_probe(struct platform_device *pdev)
>   
>   	broxton_audio_card.dev = &pdev->dev;
>   	snd_soc_card_set_drvdata(&broxton_audio_card, ctx);
> -	if (x86_match_cpu(glk_ids)) {
> +	if (soc_intel_is_glk()) {
>   		unsigned int i;
>   
>   		broxton_audio_card.name = "glkda7219max";
> diff --git a/sound/soc/intel/boards/bytcht_es8316.c b/sound/soc/intel/boards/bytcht_es8316.c
> index e8c585ffd04d..d08715ac3945 100644
> --- a/sound/soc/intel/boards/bytcht_es8316.c
> +++ b/sound/soc/intel/boards/bytcht_es8316.c
> @@ -30,8 +30,6 @@
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
>   #include <linux/slab.h>
> -#include <asm/cpu_device_id.h>
> -#include <asm/intel-family.h>
>   #include <asm/platform_sst_audio.h>
>   #include <sound/jack.h>
>   #include <sound/pcm.h>
> @@ -40,6 +38,7 @@
>   #include <sound/soc-acpi.h>
>   #include "../atom/sst-atom-controls.h"
>   #include "../common/sst-dsp.h"
> +#include "../common/soc-intel-quirks.h"
>   
>   /* jd-inv + terminating entry */
>   #define MAX_NO_PROPS 2
> @@ -430,11 +429,6 @@ static struct snd_soc_card byt_cht_es8316_card = {
>   	.resume_post = byt_cht_es8316_resume,
>   };
>   
> -static const struct x86_cpu_id baytrail_cpu_ids[] = {
> -	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_SILVERMONT }, /* Valleyview */
> -	{}
> -};
> -
>   static const struct acpi_gpio_params first_gpio = { 0, 0, false };
>   
>   static const struct acpi_gpio_mapping byt_cht_es8316_gpios[] = {
> @@ -506,8 +500,8 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
>   	dmi_id = dmi_first_match(byt_cht_es8316_quirk_table);
>   	if (dmi_id) {
>   		quirk = (unsigned long)dmi_id->driver_data;
> -	} else if (x86_match_cpu(baytrail_cpu_ids) &&
> -	    mach->mach_params.acpi_ipc_irq_index == 0) {
> +	} else if (soc_intel_is_byt() &&
> +		   mach->mach_params.acpi_ipc_irq_index == 0) {
>   		/* On BYTCR default to SSP0, internal-mic-in2-map, mono-spk */
>   		quirk = BYT_CHT_ES8316_SSP0 | BYT_CHT_ES8316_INTMIC_IN2_MAP |
>   			BYT_CHT_ES8316_MONO_SPEAKER;
> diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
> index dc22df9a99fb..7aae7b78efba 100644
> --- a/sound/soc/intel/boards/bytcr_rt5640.c
> +++ b/sound/soc/intel/boards/bytcr_rt5640.c
> @@ -28,7 +28,6 @@
>   #include <linux/dmi.h>
>   #include <linux/input.h>
>   #include <linux/slab.h>
> -#include <asm/cpu_device_id.h>
>   #include <sound/pcm.h>
>   #include <sound/pcm_params.h>
>   #include <sound/soc.h>
> @@ -38,6 +37,7 @@
>   #include "../../codecs/rt5640.h"
>   #include "../atom/sst-atom-controls.h"
>   #include "../common/sst-dsp.h"
> +#include "../common/soc-intel-quirks.h"
>   
>   enum {
>   	BYT_RT5640_DMIC1_MAP,
> @@ -1130,18 +1130,6 @@ static struct snd_soc_card byt_rt5640_card = {
>   	.resume_post = byt_rt5640_resume,
>   };
>   
> -static bool is_valleyview(void)
> -{
> -	static const struct x86_cpu_id cpu_ids[] = {
> -		{ X86_VENDOR_INTEL, 6, 55 }, /* Valleyview, Bay Trail */
> -		{}
> -	};
> -
> -	if (!x86_match_cpu(cpu_ids))
> -		return false;
> -	return true;
> -}
> -
>   struct acpi_chan_package {   /* ACPICA seems to require 64 bit integers */
>   	u64 aif_value;       /* 1: AIF1, 2: AIF2 */
>   	u64 mclock_value;    /* usually 25MHz (0x17d7940), ignored */
> @@ -1190,7 +1178,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
>   	 * swap SSP0 if bytcr is detected
>   	 * (will be overridden if DMI quirk is detected)
>   	 */
> -	if (is_valleyview()) {
> +	if (soc_intel_is_byt()) {
>   		if (mach->mach_params.acpi_ipc_irq_index == 0)
>   			is_bytcr = true;
>   	}
> diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
> index ca657c3e5726..6df6435ea394 100644
> --- a/sound/soc/intel/boards/bytcr_rt5651.c
> +++ b/sound/soc/intel/boards/bytcr_rt5651.c
> @@ -30,8 +30,6 @@
>   #include <linux/gpio/consumer.h>
>   #include <linux/gpio/machine.h>
>   #include <linux/slab.h>
> -#include <asm/cpu_device_id.h>
> -#include <asm/intel-family.h>
>   #include <sound/pcm.h>
>   #include <sound/pcm_params.h>
>   #include <sound/soc.h>
> @@ -39,6 +37,7 @@
>   #include <sound/soc-acpi.h>
>   #include "../../codecs/rt5651.h"
>   #include "../atom/sst-atom-controls.h"
> +#include "../common/soc-intel-quirks.h"
>   
>   enum {
>   	BYT_RT5651_DMIC_MAP,
> @@ -852,16 +851,6 @@ static struct snd_soc_card byt_rt5651_card = {
>   	.resume_post = byt_rt5651_resume,
>   };
>   
> -static const struct x86_cpu_id baytrail_cpu_ids[] = {
> -	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_SILVERMONT }, /* Valleyview */
> -	{}
> -};
> -
> -static const struct x86_cpu_id cherrytrail_cpu_ids[] = {
> -	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_AIRMONT },     /* Braswell */
> -	{}
> -};
> -
>   static const struct acpi_gpio_params ext_amp_enable_gpios = { 0, 0, false };
>   
>   static const struct acpi_gpio_mapping cht_rt5651_gpios[] = {
> @@ -932,7 +921,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
>   	 * swap SSP0 if bytcr is detected
>   	 * (will be overridden if DMI quirk is detected)
>   	 */
> -	if (x86_match_cpu(baytrail_cpu_ids)) {
> +	if (soc_intel_is_byt()) {
>   		if (mach->mach_params.acpi_ipc_irq_index == 0)
>   			is_bytcr = true;
>   	}
> @@ -1001,7 +990,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
>   	}
>   
>   	/* Cherry Trail devices use an external amplifier enable gpio */
> -	if (x86_match_cpu(cherrytrail_cpu_ids) && !byt_rt5651_gpios)
> +	if (soc_intel_is_cht() && !byt_rt5651_gpios)
>   		byt_rt5651_gpios = cht_rt5651_gpios;
>   
>   	if (byt_rt5651_gpios) {
> diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c b/sound/soc/intel/boards/cht_bsw_rt5645.c
> index 32dbeaf1ab94..de5fe58ae3b4 100644
> --- a/sound/soc/intel/boards/cht_bsw_rt5645.c
> +++ b/sound/soc/intel/boards/cht_bsw_rt5645.c
> @@ -26,7 +26,6 @@
>   #include <linux/clk.h>
>   #include <linux/dmi.h>
>   #include <linux/slab.h>
> -#include <asm/cpu_device_id.h>
>   #include <sound/pcm.h>
>   #include <sound/pcm_params.h>
>   #include <sound/soc.h>
> @@ -34,6 +33,7 @@
>   #include <sound/soc-acpi.h>
>   #include "../../codecs/rt5645.h"
>   #include "../atom/sst-atom-controls.h"
> +#include "../common/soc-intel-quirks.h"
>   
>   #define CHT_PLAT_CLK_3_HZ	19200000
>   #define CHT_CODEC_DAI1	"rt5645-aif1"
> @@ -509,18 +509,6 @@ static char cht_rt5645_codec_name[SND_ACPI_I2C_ID_LEN];
>   static char cht_rt5645_codec_aif_name[12]; /*  = "rt5645-aif[1|2]" */
>   static char cht_rt5645_cpu_dai_name[10]; /*  = "ssp[0|2]-port" */
>   
> -static bool is_valleyview(void)
> -{
> -	static const struct x86_cpu_id cpu_ids[] = {
> -		{ X86_VENDOR_INTEL, 6, 55 }, /* Valleyview, Bay Trail */
> -		{}
> -	};
> -
> -	if (!x86_match_cpu(cpu_ids))
> -		return false;
> -	return true;
> -}
> -
>   struct acpi_chan_package {   /* ACPICA seems to require 64 bit integers */
>   	u64 aif_value;       /* 1: AIF1, 2: AIF2 */
>   	u64 mclock_value;    /* usually 25MHz (0x17d7940), ignored */
> @@ -585,7 +573,7 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
>   	 * swap SSP0 if bytcr is detected
>   	 * (will be overridden if DMI quirk is detected)
>   	 */
> -	if (is_valleyview()) {
> +	if (soc_intel_is_byt()) {
>   		if (mach->mach_params.acpi_ipc_irq_index == 0)
>   			is_bytcr = true;
>   	}
> diff --git a/sound/soc/intel/boards/sof_rt5682.c b/sound/soc/intel/boards/sof_rt5682.c
> index e441dc979966..355fd9730a44 100644
> --- a/sound/soc/intel/boards/sof_rt5682.c
> +++ b/sound/soc/intel/boards/sof_rt5682.c
> @@ -10,8 +10,6 @@
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
>   #include <linux/dmi.h>
> -#include <asm/cpu_device_id.h>
> -#include <asm/intel-family.h>
>   #include <sound/core.h>
>   #include <sound/jack.h>
>   #include <sound/pcm.h>
> @@ -21,6 +19,7 @@
>   #include <sound/soc-acpi.h>
>   #include "../../codecs/rt5682.h"
>   #include "../../codecs/hdac_hdmi.h"
> +#include "../common/soc-intel-quirks.h"
>   
>   #define NAME_SIZE 32
>   
> @@ -304,12 +303,6 @@ static struct snd_soc_card sof_audio_card_rt5682 = {
>   	.late_probe = sof_card_late_probe,
>   };
>   
> -static const struct x86_cpu_id legacy_cpi_ids[] = {
> -	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_SILVERMONT }, /* Baytrail */
> -	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_AIRMONT }, /* Cherrytrail */
> -	{}
> -};
> -
>   static struct snd_soc_dai_link_component rt5682_component[] = {
>   	{
>   		.name = "i2c-10EC5682:00",
> @@ -498,7 +491,7 @@ static int sof_audio_probe(struct platform_device *pdev)
>   	if (!ctx)
>   		return -ENOMEM;
>   
> -	if (x86_match_cpu(legacy_cpi_ids)) {
> +	if (soc_intel_is_byt() || soc_intel_is_cht()) {
>   		is_legacy_cpu = 1;
>   		dmic_num = 0;
>   		hdmi_num = 0;
> diff --git a/sound/soc/intel/common/soc-intel-quirks.h b/sound/soc/intel/common/soc-intel-quirks.h
> new file mode 100644
> index 000000000000..4718fd3cf636
> --- /dev/null
> +++ b/sound/soc/intel/common/soc-intel-quirks.h
> @@ -0,0 +1,115 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * soc-intel-quirks.h - prototypes for quirk autodetection
> + *
> + * Copyright (c) 2019, Intel Corporation.
> + *
> + */
> +
> +#ifndef _SND_SOC_INTEL_QUIRKS_H
> +#define _SND_SOC_INTEL_QUIRKS_H
> +
> +#if IS_ENABLED(CONFIG_X86)
> +
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +#include <asm/iosf_mbi.h>
> +
> +#define ICPU(model)	{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, }
> +
> +#define SOC_INTEL_IS_CPU(soc, type)				\
> +static inline bool soc_intel_is_##soc(void)			\
> +{								\
> +	static const struct x86_cpu_id soc##_cpu_ids[] = {	\
> +		ICPU(type),					\

I understand there is a separate discussion going on, hope I don't get 
fried for throwing small code review.

Consider using same arg name for both ICPU and SOC_INTEL_IS_CPU macros, 
whether it's "model" or "type". It's more readable that way.

> +		{}						\
> +	};							\
> +	const struct x86_cpu_id *id;				\
> +								\
> +	id = x86_match_cpu(soc##_cpu_ids);			\
> +	if (id)							\
> +		return true;					\
> +	return false;						\

Tenary operator should prove usefull here.

> +}
> +
> +SOC_INTEL_IS_CPU(byt, INTEL_FAM6_ATOM_SILVERMONT);
> +SOC_INTEL_IS_CPU(cht, INTEL_FAM6_ATOM_AIRMONT);
> +SOC_INTEL_IS_CPU(apl, INTEL_FAM6_ATOM_GOLDMONT);
> +SOC_INTEL_IS_CPU(glk, INTEL_FAM6_ATOM_GOLDMONT_PLUS);

Hmm. Maybe go all-in and define SOC_INTEL_IS_CPU macro for the #else 
part too? Let it simply ignore args and retun false. If so, usages above 
could be moved outside of #ifndef block. Apart from _byt_cr case, that is.

This might save even more space in the future if you device to put more 
_IS_CPU for different platforms.

> +
> +static inline bool soc_intel_is_byt_cr(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int status = 0;
> +
> +	if (!soc_intel_is_byt())
> +		return false;
> +
> +	if (iosf_mbi_available()) {
> +		u32 bios_status;
> +
> +		status = iosf_mbi_read(BT_MBI_UNIT_PMC, /* 0x04 PUNIT */
> +				       MBI_REG_READ, /* 0x10 */
> +				       0x006, /* BIOS_CONFIG */
> +				       &bios_status);
> +
> +		if (status) {
> +			dev_err(dev, "could not read PUNIT BIOS_CONFIG\n");
> +		} else {
> +			/* bits 26:27 mirror PMIC options */
> +			bios_status = (bios_status >> 26) & 3;
> +
> +			if (bios_status == 1 || bios_status == 3) {
> +				dev_info(dev, "Detected Baytrail-CR platform\n");
> +				return true;
> +			}
> +
> +			dev_info(dev, "BYT-CR not detected\n");
> +		}
> +	} else {
> +		dev_info(dev, "IOSF_MBI not available, no BYT-CR detection\n");
> +	}
> +
> +	if (!platform_get_resource(pdev, IORESOURCE_IRQ, 5)) {
> +		/*
> +		 * Some devices detected as BYT-T have only a single IRQ listed,
> +		 * causing platform_get_irq with index 5 to return -ENXIO.
> +		 * The correct IRQ in this case is at index 0, as on BYT-CR.
> +		 */
> +		dev_info(dev, "Falling back to Baytrail-CR platform\n");
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +#else
> +
> +static inline bool soc_intel_is_byt_cr(struct platform_device *pdev)
> +{
> +	return false;
> +}
> +
> +static inline bool soc_intel_is_byt(void)
> +{
> +	return false;
> +}
> +
> +static inline bool soc_intel_is_cht(void)
> +{
> +	return false;
> +}
> +
> +static inline bool soc_intel_is_apl(void)
> +{
> +	return false;
> +}
> +
> +static inline bool soc_intel_is_glk(void)
> +{
> +	return false;
> +}
> +
> +#endif
> +
> + #endif /* _SND_SOC_INTEL_QUIRKS_H */
> diff --git a/sound/soc/sof/sof-acpi-dev.c b/sound/soc/sof/sof-acpi-dev.c
> index 38062dd00dd2..93a8e15bbd2c 100644
> --- a/sound/soc/sof/sof-acpi-dev.c
> +++ b/sound/soc/sof/sof-acpi-dev.c
> @@ -15,10 +15,7 @@
>   #include <sound/soc-acpi.h>
>   #include <sound/soc-acpi-intel-match.h>
>   #include <sound/sof.h>
> -#ifdef CONFIG_X86
> -#include <asm/iosf_mbi.h>
> -#endif
> -
> +#include "../intel/common/soc-intel-quirks.h"
>   #include "ops.h"
>   
>   /* platform specific devices */
> @@ -105,56 +102,6 @@ static const struct sof_dev_desc sof_acpi_baytrail_desc = {
>   	.arch_ops = &sof_xtensa_arch_ops
>   };
>   
> -#ifdef CONFIG_X86 /* TODO: move this to common helper */
> -
> -static bool is_byt_cr(struct platform_device *pdev)
> -{
> -	struct device *dev = &pdev->dev;
> -	int status;
> -
> -	if (iosf_mbi_available()) {
> -		u32 bios_status;
> -		status = iosf_mbi_read(BT_MBI_UNIT_PMC, /* 0x04 PUNIT */
> -				       MBI_REG_READ, /* 0x10 */
> -				       0x006, /* BIOS_CONFIG */
> -				       &bios_status);
> -
> -		if (status) {
> -			dev_err(dev, "could not read PUNIT BIOS_CONFIG\n");
> -		} else {
> -			/* bits 26:27 mirror PMIC options */
> -			bios_status = (bios_status >> 26) & 3;
> -
> -			if (bios_status == 1 || bios_status == 3) {
> -				dev_info(dev, "Detected Baytrail-CR platform\n");
> -				return true;
> -			}
> -
> -			dev_info(dev, "BYT-CR not detected\n");
> -		}
> -	} else {
> -		dev_info(dev, "IOSF_MBI not available, no BYT-CR detection\n");
> -	}
> -
> -	if (platform_get_resource(pdev, IORESOURCE_IRQ, 5) == NULL) {
> -		/*
> -		 * Some devices detected as BYT-T have only a single IRQ listed,
> -		 * causing platform_get_irq with index 5 to return -ENXIO.
> -		 * The correct IRQ in this case is at index 0, as on BYT-CR.
> -		 */
> -		dev_info(dev, "Falling back to Baytrail-CR platform\n");
> -		return true;
> -	}
> -
> -	return false;
> -}
> -#else
> -static int is_byt_cr(struct platform_device *pdev)
> -{
> -	return 0;
> -}
> -#endif
> -
>   static const struct sof_dev_desc sof_acpi_cherrytrail_desc = {
>   	.machines = snd_soc_acpi_intel_cherrytrail_machines,
>   	.resindex_lpe_base = 0,
> @@ -209,7 +156,7 @@ static int sof_acpi_probe(struct platform_device *pdev)
>   		return -ENODEV;
>   
>   #if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
> -	if (desc == &sof_acpi_baytrail_desc && is_byt_cr(pdev))
> +	if (desc == &sof_acpi_baytrail_desc && soc_intel_is_byt_cr(pdev))
>   		desc = &sof_acpi_baytrailcr_desc;
>   #endif
>   
>

Andy Shevchenko June 19, 2019, 11:08 a.m. UTC | #7

On Tue, Jun 18, 2019 at 09:26:50PM +0200, Cezary Rojewski wrote:
> On 2019-05-28 22:02, Pierre-Louis Bossart wrote:
> > We have duplicated code in multiple locations (atom, machine drivers,
> > SOF) to detect Baytrail, Cherrytrail and other SOCs. This is not very
> > elegant, and introduces dependencies on CONFIG_X86 that prevent
> > COMPILE_TEST from working.
> > 
> > Add common helpers to provide same functionality in a cleaner
> > way. This will also help support the DMI-based quirks being introduced
> > to handle SOF/SST autodetection.

> > +#define ICPU(model)	{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, }

We have macros for this in intel-family.h.

> > +#define SOC_INTEL_IS_CPU(soc, type)				\
> > +static inline bool soc_intel_is_##soc(void)			\
> > +{								\
> > +	static const struct x86_cpu_id soc##_cpu_ids[] = {	\
> > +		ICPU(type),					\
> 
> I understand there is a separate discussion going on, hope I don't get fried
> for throwing small code review.
> 
> Consider using same arg name for both ICPU and SOC_INTEL_IS_CPU macros,
> whether it's "model" or "type". It's more readable that way.
> 
> > +		{}						\
> > +	};							\
> > +	const struct x86_cpu_id *id;				\
> > +								\
> > +	id = x86_match_cpu(soc##_cpu_ids);			\
> > +	if (id)							\
> > +		return true;					\
> > +	return false;						\
> 
> Tenary operator should prove usefull here.

In this way it is simple
	return !!x86_match_cpu(...);

No conditional needed at all.

> 
> > +}

Pierre-Louis Bossart June 19, 2019, 11:52 a.m. UTC | #8

>>> Add common helpers to provide same functionality in a cleaner
>>> way. This will also help support the DMI-based quirks being introduced
>>> to handle SOF/SST autodetection.
> 
>>> +#define ICPU(model)	{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, }
> 
> We have macros for this in intel-family.h.

Andy, just to double-check, were you referring to the following macro?
#define INTEL_CPU_FAM6(_model, _driver_data)

Thanks!

Andy Shevchenko June 19, 2019, 1:04 p.m. UTC | #9

On Wed, Jun 19, 2019 at 01:52:57PM +0200, Pierre-Louis Bossart wrote:
> 
> > > > Add common helpers to provide same functionality in a cleaner
> > > > way. This will also help support the DMI-based quirks being introduced
> > > > to handle SOF/SST autodetection.
> > 
> > > > +#define ICPU(model)	{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, }
> > 
> > We have macros for this in intel-family.h.
> 
> Andy, just to double-check, were you referring to the following macro?
> #define INTEL_CPU_FAM6(_model, _driver_data)

Yes, and my patch to move pointer elsewhere.
Now I realize that the macro without data pointer wasn't accepted by some reason.

[RFC] ASoC: Intel: use common helpers to detect CPUs

Commit Message

Comments

Patch