[PATCH 00/15] ASoC: Intel: avs: Fixes and new boards support
Two fixes are leading the way - one addresses the incorrect DMA mask assignment (typo) at driver probe. The other, fixes a potential buffer overflow when copying data received from firmware to kernel buffer. However unlikely, the fix should still be there.
Then a range of patches providing the support for: - AML with rt286 (machine board) - KBL-R for rt298 (codec) - KBL-R with rt298 (machine board) - APL/KBL with da7219 (machine board) - Addition of all the missing SKL-based PCI ids to core.c
Of the remaining changes, only one stands out - special case is provided for "unsupported" IPCs. The driver supports a range of platforms, however, on some generations given IPC may not be supported. Such call shall not be treated as "invalid" - those are two different scenarios.
Everything else in the patchset is mostly a readability improvement: spelling fixes and log messages issues, code simplification.
Amadeusz Sławiński (4): ASoC: codecs: rt298: Add quirk for KBL-R RVP platform ASoC: Intel: avs: Add quirk for KBL-R RVP platform ASoC: Intel: avs: Support da7219 on both KBL and APL ASoC: Intel: avs: Add missing include to HDA board
Cezary Rojewski (11): ASoC: Intel: avs: Fix DMA mask assignment ASoC: Intel: avs: Fix potential RX buffer overflow ASoC: Intel: avs: Support AML with rt286 configuration ASoC: Intel: avs: Add missing SKL-based device IDs ASoC: Intel: avs: Simplify d0ix disabling routine ASoC: Intel: avs: Do not reuse msg between different IPC handlers ASoC: Intel: avs: Do not treat unsupported IPCs as invalid ASoC: Intel: avs: Do not print IPC error message twice ASoC: Intel: avs: Simplify ignore_fw_version description ASoC: Intel: avs: Simplify log control for SKL ASoC: codecs: hda: Fix spelling error in log message
sound/soc/codecs/hda.c | 2 +- sound/soc/codecs/rt298.c | 7 +++++++ sound/soc/intel/avs/apl.c | 6 ++++-- sound/soc/intel/avs/avs.h | 4 +++- sound/soc/intel/avs/board_selection.c | 6 ++++++ sound/soc/intel/avs/boards/da7219.c | 6 +++++- sound/soc/intel/avs/boards/hdaudio.c | 1 + sound/soc/intel/avs/boards/rt298.c | 24 ++++++++++++++++++++++-- sound/soc/intel/avs/core.c | 6 +++++- sound/soc/intel/avs/ipc.c | 5 +++-- sound/soc/intel/avs/loader.c | 2 +- sound/soc/intel/avs/messages.c | 19 ++++--------------- sound/soc/intel/avs/messages.h | 2 ++ sound/soc/intel/avs/skl.c | 4 ++-- 14 files changed, 66 insertions(+), 28 deletions(-)
On 2022-10-10 2:07 PM, Cezary Rojewski wrote:
...
Amadeusz Sławiński (4): ASoC: codecs: rt298: Add quirk for KBL-R RVP platform ASoC: Intel: avs: Add quirk for KBL-R RVP platform ASoC: Intel: avs: Support da7219 on both KBL and APL ASoC: Intel: avs: Add missing include to HDA board
Forgotten to add my signed-off tag for Amadeo's patches. Sorry for that, resending.
Regards, Czarek
Spelling error leads to incorrect behavior when setting up DMA mask.
Fixes: a5bbbde2b81e ("ASoC: Intel: avs: Use helper function to set up DMA") Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/avs/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c index c50c20fd681a..58db13374166 100644 --- a/sound/soc/intel/avs/core.c +++ b/sound/soc/intel/avs/core.c @@ -440,7 +440,7 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id) if (bus->mlcap) snd_hdac_ext_bus_get_ml_capabilities(bus);
- if (!dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64))) + if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64))) dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); dma_set_max_seg_size(dev, UINT_MAX);
If an event caused firmware to return invalid RX size for LARGE_CONFIG_GET, memcpy_fromio() could end up copying too many bytes. Fix by utilizing min_t().
Reported-by: CoolStar coolstarorganization@gmail.com Fixes: f14a1c5a9f83 ("ASoC: Intel: avs: Add module management requests") Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/avs/ipc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/soc/intel/avs/ipc.c b/sound/soc/intel/avs/ipc.c index 020d85c7520d..77da206f7dbb 100644 --- a/sound/soc/intel/avs/ipc.c +++ b/sound/soc/intel/avs/ipc.c @@ -192,7 +192,8 @@ static void avs_dsp_receive_rx(struct avs_dev *adev, u64 header) /* update size in case of LARGE_CONFIG_GET */ if (msg.msg_target == AVS_MOD_MSG && msg.global_msg_type == AVS_MOD_LARGE_CONFIG_GET) - ipc->rx.size = msg.ext.large_config.data_off_size; + ipc->rx.size = min_t(u32, AVS_MAILBOX_SIZE, + msg.ext.large_config.data_off_size);
memcpy_fromio(ipc->rx.data, avs_uplink_addr(adev), ipc->rx.size); trace_avs_msg_payload(ipc->rx.data, ipc->rx.size);
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
KBL-R RVP platforms also use combojack, so we need to enable that configuration for them.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- sound/soc/codecs/rt298.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/sound/soc/codecs/rt298.c b/sound/soc/codecs/rt298.c index b0b53d4f07df..5f36064ed6e6 100644 --- a/sound/soc/codecs/rt298.c +++ b/sound/soc/codecs/rt298.c @@ -1166,6 +1166,13 @@ static const struct dmi_system_id force_combo_jack_table[] = { DMI_MATCH(DMI_PRODUCT_NAME, "Geminilake") } }, + { + .ident = "Intel Kabylake R RVP", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Intel Corporation"), + DMI_MATCH(DMI_PRODUCT_NAME, "Kabylake Client platform") + } + }, { } };
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
KBL-R RVPs contain built-in rt298 codec which requires different PLL clock and .dai_fmt configuration than seen on other boards.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- sound/soc/intel/avs/boards/rt298.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/sound/soc/intel/avs/boards/rt298.c b/sound/soc/intel/avs/boards/rt298.c index b28d36872dcb..58c9d9edecf0 100644 --- a/sound/soc/intel/avs/boards/rt298.c +++ b/sound/soc/intel/avs/boards/rt298.c @@ -6,6 +6,7 @@ // Amadeusz Slawinski amadeuszx.slawinski@linux.intel.com //
+#include <linux/dmi.h> #include <linux/module.h> #include <sound/jack.h> #include <sound/pcm.h> @@ -14,6 +15,16 @@ #include <sound/soc-acpi.h> #include "../../../codecs/rt298.h"
+static const struct dmi_system_id kblr_dmi_table[] = { + { + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Intel Corporation"), + DMI_MATCH(DMI_BOARD_NAME, "Kabylake R DDR4 RVP"), + }, + }, + {} +}; + static const struct snd_kcontrol_new card_controls[] = { SOC_DAPM_PIN_SWITCH("Headphone Jack"), SOC_DAPM_PIN_SWITCH("Mic Jack"), @@ -96,9 +107,15 @@ avs_rt298_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_param { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0); + unsigned int clk_freq; int ret;
- ret = snd_soc_dai_set_sysclk(codec_dai, RT298_SCLK_S_PLL, 19200000, SND_SOC_CLOCK_IN); + if (dmi_first_match(kblr_dmi_table)) + clk_freq = 24000000; + else + clk_freq = 19200000; + + ret = snd_soc_dai_set_sysclk(codec_dai, RT298_SCLK_S_PLL, clk_freq, SND_SOC_CLOCK_IN); if (ret < 0) dev_err(rtd->dev, "Set codec sysclk failed: %d\n", ret);
@@ -139,7 +156,10 @@ static int avs_create_dai_link(struct device *dev, const char *platform_name, in dl->platforms = platform; dl->num_platforms = 1; dl->id = 0; - dl->dai_fmt = SND_SOC_DAIFMT_DSP_A | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS; + if (dmi_first_match(kblr_dmi_table)) + dl->dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS; + else + dl->dai_fmt = SND_SOC_DAIFMT_DSP_A | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS; dl->init = avs_rt298_codec_init; dl->be_hw_params_fixup = avs_rt298_be_fixup; dl->ops = &avs_rt298_ops;
ACPI ID of INT343A signals rt286 device for SKL, KBL and AML platforms. Add the missing AML entry.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/avs/board_selection.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/soc/intel/avs/board_selection.c b/sound/soc/intel/avs/board_selection.c index 87f9c18be238..95afbac0f358 100644 --- a/sound/soc/intel/avs/board_selection.c +++ b/sound/soc/intel/avs/board_selection.c @@ -29,6 +29,12 @@ static const struct dmi_system_id kbl_dmi_table[] = { DMI_MATCH(DMI_BOARD_NAME, "Skylake Y LPDDR3 RVP3"), }, }, + { + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Intel Corporation"), + DMI_MATCH(DMI_BOARD_NAME, "AmberLake Y"), + }, + }, {} };
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
KBL and APL devices use same codec but have different clock, so it must be set appropriately depending on device.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- sound/soc/intel/avs/boards/da7219.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/sound/soc/intel/avs/boards/da7219.c b/sound/soc/intel/avs/boards/da7219.c index 02ae542ad779..503a967a1c3a 100644 --- a/sound/soc/intel/avs/boards/da7219.c +++ b/sound/soc/intel/avs/boards/da7219.c @@ -6,6 +6,7 @@ //
#include <linux/module.h> +#include <linux/platform_data/x86/soc.h> #include <linux/platform_device.h> #include <sound/jack.h> #include <sound/pcm.h> @@ -80,7 +81,10 @@ static int avs_da7219_codec_init(struct snd_soc_pcm_runtime *runtime) int ret;
jack = snd_soc_card_get_drvdata(card); - clk_freq = 19200000; + if (soc_intel_is_apl()) + clk_freq = 19200000; + else /* kbl */ + clk_freq = 24576000;
ret = snd_soc_dai_set_sysclk(codec_dai, DA7219_CLKSRC_MCLK, clk_freq, SND_SOC_CLOCK_IN); if (ret) {
Enable additional SKL-based configurations by filling device ID table with new entries.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/avs/core.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c index 58db13374166..6503c5bfd63d 100644 --- a/sound/soc/intel/avs/core.c +++ b/sound/soc/intel/avs/core.c @@ -667,7 +667,11 @@ static const struct avs_spec apl_desc = {
static const struct pci_device_id avs_ids[] = { { PCI_VDEVICE(INTEL, 0x9d70), (unsigned long)&skl_desc }, /* SKL */ + { PCI_VDEVICE(INTEL, 0xa170), (unsigned long)&skl_desc }, /* SKL-H */ { PCI_VDEVICE(INTEL, 0x9d71), (unsigned long)&skl_desc }, /* KBL */ + { PCI_VDEVICE(INTEL, 0xa171), (unsigned long)&skl_desc }, /* KBL-H */ + { PCI_VDEVICE(INTEL, 0xa2f0), (unsigned long)&skl_desc }, /* KBL-S */ + { PCI_VDEVICE(INTEL, 0xa3f0), (unsigned long)&skl_desc }, /* CML-V */ { PCI_VDEVICE(INTEL, 0x5a98), (unsigned long)&apl_desc }, /* APL */ { PCI_VDEVICE(INTEL, 0x3198), (unsigned long)&apl_desc }, /* GML */ { 0 }
No need to atomic_add_return(1) when there is atomic_inc_return() available.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/avs/ipc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/avs/ipc.c b/sound/soc/intel/avs/ipc.c index 77da206f7dbb..152f8d0bdf8e 100644 --- a/sound/soc/intel/avs/ipc.c +++ b/sound/soc/intel/avs/ipc.c @@ -74,7 +74,7 @@ int avs_dsp_disable_d0ix(struct avs_dev *adev) struct avs_ipc *ipc = adev->ipc;
/* Prevent PG only on the first disable. */ - if (atomic_add_return(1, &ipc->d0ix_disable_depth) == 1) { + if (atomic_inc_return(&ipc->d0ix_disable_depth) == 1) { cancel_delayed_work_sync(&ipc->d0ix_work); return avs_dsp_set_d0ix(adev, false); }
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
In some configurations board fails to compile due to missing header. Add it to fix build.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- sound/soc/intel/avs/boards/hdaudio.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/intel/avs/boards/hdaudio.c b/sound/soc/intel/avs/boards/hdaudio.c index 073663ba140d..e68c4c7aa2ba 100644 --- a/sound/soc/intel/avs/boards/hdaudio.c +++ b/sound/soc/intel/avs/boards/hdaudio.c @@ -6,6 +6,7 @@ // Amadeusz Slawinski amadeuszx.slawinski@linux.intel.com //
+#include <linux/module.h> #include <linux/platform_device.h> #include <sound/hda_codec.h> #include <sound/hda_i915.h>
While LOG_BUFFER_STATUS is a simple notification with only one meaningful field, same message ptr shall not be reused for two different handlers.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/avs/apl.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/sound/soc/intel/avs/apl.c b/sound/soc/intel/avs/apl.c index b8e2b23c9f64..7c8ce98eda9d 100644 --- a/sound/soc/intel/avs/apl.c +++ b/sound/soc/intel/avs/apl.c @@ -133,12 +133,14 @@ static int apl_coredump(struct avs_dev *adev, union avs_notify_msg *msg) buf = apl_log_payload_addr(addr); memcpy_fromio(&layout, addr, sizeof(layout)); if (!apl_is_entry_stackdump(buf + layout.read_ptr)) { + union avs_notify_msg lbs_msg = AVS_NOTIFICATION(LOG_BUFFER_STATUS); + /* * DSP awaits the remaining logs to be * gathered before dumping stack */ - msg->log.core = msg->ext.coredump.core_id; - avs_dsp_op(adev, log_buffer_status, msg); + lbs_msg.log.core = msg->ext.coredump.core_id; + avs_dsp_op(adev, log_buffer_status, &lbs_msg); }
pos = dump + AVS_FW_REGS_SIZE;
Utilize NOT_SUPPORTED status code to differentiate between unsupported and invalid requests. Skip over error paths if it is the former that is communicated by the base firmware.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/avs/avs.h | 4 +++- sound/soc/intel/avs/messages.h | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h index 92e37722d280..91f78eb11bc1 100644 --- a/sound/soc/intel/avs/avs.h +++ b/sound/soc/intel/avs/avs.h @@ -220,8 +220,10 @@ static inline void avs_ipc_err(struct avs_dev *adev, struct avs_ipc_msg *tx, /* * If IPC channel is blocked e.g.: due to ongoing recovery, * -EPERM error code is expected and thus it's not an actual error. + * + * Unsupported IPCs are of no harm either. */ - if (error == -EPERM) + if (error == -EPERM || error == AVS_IPC_NOT_SUPPORTED) dev_dbg(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", name, tx->glb.primary, tx->glb.ext.val, error); else diff --git a/sound/soc/intel/avs/messages.h b/sound/soc/intel/avs/messages.h index c0f90dba9af8..02b3b7a74783 100644 --- a/sound/soc/intel/avs/messages.h +++ b/sound/soc/intel/avs/messages.h @@ -150,6 +150,8 @@ union avs_module_msg { }; } __packed;
+#define AVS_IPC_NOT_SUPPORTED 15 + union avs_reply_msg { u64 val; struct {
ENABLE_LOGS and SYSTEM_TIME IPCs call LARGE_CONFIG_SET internally which dumps an error message in case of an error. There is no need to repeat the process in the top level handler.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/avs/messages.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/sound/soc/intel/avs/messages.c b/sound/soc/intel/avs/messages.c index d4bcee1aabcf..6b0fecbf07c3 100644 --- a/sound/soc/intel/avs/messages.c +++ b/sound/soc/intel/avs/messages.c @@ -687,20 +687,13 @@ int avs_ipc_get_modules_info(struct avs_dev *adev, struct avs_mods_info **info)
int avs_ipc_set_enable_logs(struct avs_dev *adev, u8 *log_info, size_t size) { - int ret; - - ret = avs_ipc_set_large_config(adev, AVS_BASEFW_MOD_ID, AVS_BASEFW_INST_ID, - AVS_BASEFW_ENABLE_LOGS, log_info, size); - if (ret) - dev_err(adev->dev, "enable logs failed: %d\n", ret); - - return ret; + return avs_ipc_set_large_config(adev, AVS_BASEFW_MOD_ID, AVS_BASEFW_INST_ID, + AVS_BASEFW_ENABLE_LOGS, log_info, size); }
int avs_ipc_set_system_time(struct avs_dev *adev) { struct avs_sys_time sys_time; - int ret; u64 us;
/* firmware expects UTC time in micro seconds */ @@ -708,12 +701,8 @@ int avs_ipc_set_system_time(struct avs_dev *adev) sys_time.val_l = us & UINT_MAX; sys_time.val_u = us >> 32;
- ret = avs_ipc_set_large_config(adev, AVS_BASEFW_MOD_ID, AVS_BASEFW_INST_ID, - AVS_BASEFW_SYSTEM_TIME, (u8 *)&sys_time, sizeof(sys_time)); - if (ret) - dev_err(adev->dev, "set system time failed: %d\n", ret); - - return ret; + return avs_ipc_set_large_config(adev, AVS_BASEFW_MOD_ID, AVS_BASEFW_INST_ID, + AVS_BASEFW_SYSTEM_TIME, (u8 *)&sys_time, sizeof(sys_time)); }
int avs_ipc_copier_set_sink_format(struct avs_dev *adev, u16 module_id,
Reword the parameter description to drop any confusion regarding its purpose.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/avs/loader.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/avs/loader.c b/sound/soc/intel/avs/loader.c index 9e3f8ff33a87..2d80a271eb50 100644 --- a/sound/soc/intel/avs/loader.c +++ b/sound/soc/intel/avs/loader.c @@ -43,7 +43,7 @@ /* Occasionally, engineering (release candidate) firmware is provided for testing. */ static bool debug_ignore_fw_version; module_param_named(ignore_fw_version, debug_ignore_fw_version, bool, 0444); -MODULE_PARM_DESC(ignore_fw_version, "Verify FW version 0=yes (default), 1=no"); +MODULE_PARM_DESC(ignore_fw_version, "Ignore firmware version check 0=no (default), 1=yes");
#define AVS_LIB_NAME_SIZE 8
Loop only till the actual number of AudioDSP cores, not the value of mask made from said number.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/avs/skl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/intel/avs/skl.c b/sound/soc/intel/avs/skl.c index bda5ec7510fe..dc98b5cf900f 100644 --- a/sound/soc/intel/avs/skl.c +++ b/sound/soc/intel/avs/skl.c @@ -28,12 +28,12 @@ static int skl_enable_logs(struct avs_dev *adev, enum avs_log_enable enable, u32
info->core_mask = resource_mask; if (enable) - for_each_set_bit(i, &resource_mask, GENMASK(num_cores, 0)) { + for_each_set_bit(i, &resource_mask, num_cores) { info->logs_core[i].enable = enable; info->logs_core[i].min_priority = *priorities++; } else - for_each_set_bit(i, &resource_mask, GENMASK(num_cores, 0)) + for_each_set_bit(i, &resource_mask, num_cores) info->logs_core[i].enable = enable;
ret = avs_ipc_set_enable_logs(adev, (u8 *)info, size);
To improve readability.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/codecs/hda.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/hda.c b/sound/soc/codecs/hda.c index ad20a3dff9b7..712d8ba85f0e 100644 --- a/sound/soc/codecs/hda.c +++ b/sound/soc/codecs/hda.c @@ -213,7 +213,7 @@ static int hda_codec_probe(struct snd_soc_component *component)
patch = (hda_codec_patch_t)codec->preset->driver_data; if (!patch) { - dev_err(&hdev->dev, "no patch specified?\n"); + dev_err(&hdev->dev, "no patch specified\n"); ret = -EINVAL; goto err; }
participants (1)
-
Cezary Rojewski