- Sound-open-firmware - mailman.alsa-project.org

Re: [PATCH] ASoC: SOF: Clean up sof_ipc_flood_dfs_write()
by Péter Ujfalusi 04 Apr '24

04 Apr '24

On 04/04/2024 10:34, Dan Carpenter wrote: > This function doesn't support partial writes so using > simple_write_to_buffer() doesn't really make sense. It's better to > just use copy_from_user(). > > Signed-off-by: Dan Carpenter <dan.carpenter(a)linaro.org> Thanks, it makes sense. Acked-by: Peter Ujfalusi <peter.ujfalusi(a)linux.intel.com> > --- > sound/soc/sof/sof-client-ipc-flood-test.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/sound/soc/sof/sof-client-ipc-flood-test.c b/sound/soc/sof/sof-client-ipc-flood-test.c > index c0d6723aed59..1b2e9e25a836 100644 > --- a/sound/soc/sof/sof-client-ipc-flood-test.c > +++ b/sound/soc/sof/sof-client-ipc-flood-test.c > @@ -160,15 +160,20 @@ static ssize_t sof_ipc_flood_dfs_write(struct file *file, const char __user *buf > unsigned long ipc_count = 0; > struct dentry *dentry; > int err; > - size_t size; > char *string; > int ret; > > + if (*ppos != 0) > + return -EINVAL; > + > string = kzalloc(count + 1, GFP_KERNEL); > if (!string) > return -ENOMEM; > > - size = simple_write_to_buffer(string, count, ppos, buffer, count); > + if (copy_from_user(string, buffer, count)) { > + ret = -EFAULT; > + goto out; > + } > > /* > * write op is only supported for ipc_flood_count or > @@ -198,7 +203,7 @@ static ssize_t sof_ipc_flood_dfs_write(struct file *file, const char __user *buf > /* limit max duration/ipc count for flood test */ > if (flood_duration_test) { > if (!ipc_duration_ms) { > - ret = size; > + ret = count; > goto out; > } > > @@ -207,7 +212,7 @@ static ssize_t sof_ipc_flood_dfs_write(struct file *file, const char __user *buf > ipc_duration_ms = MAX_IPC_FLOOD_DURATION_MS; > } else { > if (!ipc_count) { > - ret = size; > + ret = count; > goto out; > } > > @@ -231,9 +236,9 @@ static ssize_t sof_ipc_flood_dfs_write(struct file *file, const char __user *buf > if (err < 0) > dev_err_ratelimited(dev, "debugfs write failed to idle %d\n", err); > > - /* return size if test is successful */ > + /* return count if test is successful */ > if (ret >= 0) > - ret = size; > + ret = count; > out: > kfree(string); > return ret; -- Péter

1 0

Re: [PATCH] ASoC: SOF: Disable pointless writes to debugfs file
by Péter Ujfalusi 04 Apr '24

04 Apr '24

On 04/04/2024 10:38, Dan Carpenter wrote: > The permissions on this debugfs file are 0444 so it can't be written to. > And writing to the file hasn't done anything since commit 6e9548cdb30e > ("ASoC: SOF: Convert the generic IPC flood test into SOF client"). > Delete the write function. I agree, it is not used anymore and ... > > Signed-off-by: Dan Carpenter <dan.carpenter(a)linaro.org> Acked-by: Peter Ujfalusi <peter.ujfalusi(a)linux.intel.com> > --- > I haven't tested this patch and there is a risk that it breaks some of > the test scripts. Another option would be to just make it a dummy > function that does { return count; }. But I thought I would try the > better option first. > > Please review this extra carefully. > > sound/soc/sof/debug.c | 19 ------------------- > 1 file changed, 19 deletions(-) > > diff --git a/sound/soc/sof/debug.c b/sound/soc/sof/debug.c > index 7c8aafca8fde..937e51b02a24 100644 > --- a/sound/soc/sof/debug.c > +++ b/sound/soc/sof/debug.c > @@ -19,24 +19,6 @@ > #include "sof-priv.h" > #include "ops.h" > > -static ssize_t sof_dfsentry_write(struct file *file, const char __user *buffer, > - size_t count, loff_t *ppos) > -{ > - size_t size; > - char *string; > - int ret; > - > - string = kzalloc(count+1, GFP_KERNEL); > - if (!string) > - return -ENOMEM; > - > - size = simple_write_to_buffer(string, count, ppos, buffer, count); > - ret = size; > - > - kfree(string); This was not too constructive ;) > - return ret; > -} > - > static ssize_t sof_dfsentry_read(struct file *file, char __user *buffer, > size_t count, loff_t *ppos) > { > @@ -126,7 +108,6 @@ static const struct file_operations sof_dfs_fops = { > .open = simple_open, > .read = sof_dfsentry_read, > .llseek = default_llseek, > - .write = sof_dfsentry_write, > }; > > /* create FS entry for debug files that can expose DSP memories, registers */ -- Péter

1 0

[PATCH AUTOSEL 6.6 04/75] ASoC: SOF: amd: Optimize quirk for Valve Galileo
by Sasha Levin 29 Mar '24

29 Mar '24

From: Cristian Ciocaltea <cristian.ciocaltea(a)collabora.com> [ Upstream commit a13f0c3c0e8fb3e61fbfd99c6b350cf9be0c4660 ] Valve's Steam Deck OLED is uniquely identified by vendor and product name (Galileo) DMI fields. Simplify the quirk by removing the unnecessary match on product family. Additionally, fix the related comment as it points to the old product variant. Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea(a)collabora.com> Reviewed-by: Emil Velikov <emil.velikov(a)collabora.com> Link: https://msgid.link/r/20231219030728.2431640-7-cristian.ciocaltea@collabora.… Signed-off-by: Mark Brown <broonie(a)kernel.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- sound/soc/sof/amd/acp.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c index 19a801908b56d..f3768651d6fd8 100644 --- a/sound/soc/sof/amd/acp.c +++ b/sound/soc/sof/amd/acp.c @@ -28,11 +28,10 @@ MODULE_PARM_DESC(enable_fw_debug, "Enable Firmware debug"); const struct dmi_system_id acp_sof_quirk_table[] = { { - /* Valve Jupiter device */ + /* Steam Deck OLED device */ .matches = { DMI_MATCH(DMI_SYS_VENDOR, "Valve"), DMI_MATCH(DMI_PRODUCT_NAME, "Galileo"), - DMI_MATCH(DMI_PRODUCT_FAMILY, "Sephiroth"), }, .driver_data = (void *)SECURED_FIRMWARE, }, -- 2.43.0

1 0

[PATCH AUTOSEL 6.8 06/98] ASoC: SOF: amd: Optimize quirk for Valve Galileo
by Sasha Levin 29 Mar '24

29 Mar '24

From: Cristian Ciocaltea <cristian.ciocaltea(a)collabora.com> [ Upstream commit a13f0c3c0e8fb3e61fbfd99c6b350cf9be0c4660 ] Valve's Steam Deck OLED is uniquely identified by vendor and product name (Galileo) DMI fields. Simplify the quirk by removing the unnecessary match on product family. Additionally, fix the related comment as it points to the old product variant. Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea(a)collabora.com> Reviewed-by: Emil Velikov <emil.velikov(a)collabora.com> Link: https://msgid.link/r/20231219030728.2431640-7-cristian.ciocaltea@collabora.… Signed-off-by: Mark Brown <broonie(a)kernel.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- sound/soc/sof/amd/acp.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c index 07632ae6ccf5e..7bf5a12da1cea 100644 --- a/sound/soc/sof/amd/acp.c +++ b/sound/soc/sof/amd/acp.c @@ -28,11 +28,10 @@ MODULE_PARM_DESC(enable_fw_debug, "Enable Firmware debug"); const struct dmi_system_id acp_sof_quirk_table[] = { { - /* Valve Jupiter device */ + /* Steam Deck OLED device */ .matches = { DMI_MATCH(DMI_SYS_VENDOR, "Valve"), DMI_MATCH(DMI_PRODUCT_NAME, "Galileo"), - DMI_MATCH(DMI_PRODUCT_FAMILY, "Sephiroth"), }, .driver_data = (void *)SECURED_FIRMWARE, }, -- 2.43.0

1 0

Re: [PATCH v3 0/2] ASoC: SOF: amd: Skip IRAM/DRAM size modification for Steam Deck OLED
by Mark Brown 15 Mar '24

15 Mar '24

On Tue, 20 Feb 2024 22:16:02 +0200, Cristian Ciocaltea wrote: > This patch series restores audio support on Valve's Steam Deck OLED model, which > broke after the recent introduction of ACP/PSP communication for IRAM/DRAM fence > register programming. > > Changes in v3: > - Drop a patch file that was sent by mistake in v2 > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/2] ASoC: SOF: amd: Move signed_fw_image to struct acp_quirk_entry commit: 33c3d813330718c403a60d220f03fbece0f4fb5c [2/2] ASoC: SOF: amd: Skip IRAM/DRAM size modification for Steam Deck OLED commit: 094d11768f740f11483dad4efcd9bbcffa4ce146 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark

1 0

[PATCH AUTOSEL 6.6 19/21] ASoC: SOF: ipc4-pcm: Workaround for crashed firmware on system suspend
by Sasha Levin 29 Feb '24

29 Feb '24

From: Peter Ujfalusi <peter.ujfalusi(a)linux.intel.com> [ Upstream commit c40aad7c81e5fba34b70123ed7ce3397fa62a4d2 ] When the system is suspended while audio is active, the sof_ipc4_pcm_hw_free() is invoked to reset the pipelines since during suspend the DSP is turned off, streams will be re-started after resume. If the firmware crashes during while audio is running (or when we reset the stream before suspend) then the sof_ipc4_set_multi_pipeline_state() will fail with IPC error and the state change is interrupted. This will cause misalignment between the kernel and firmware state on next DSP boot resulting errors returned by firmware for IPC messages, eventually failing the audio resume. On stream close the errors are ignored so the kernel state will be corrected on the next DSP boot, so the second boot after the DSP panic. If sof_ipc4_trigger_pipelines() is called from sof_ipc4_pcm_hw_free() then state parameter is SOF_IPC4_PIPE_RESET and only in this case. Treat a forced pipeline reset similarly to how we treat a pcm_free by ignoring error on state sending to allow the kernel's state to be consistent with the state the firmware will have after the next boot. Link: https://github.com/thesofproject/sof/issues/8721 Signed-off-by: Peter Ujfalusi <peter.ujfalusi(a)linux.intel.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan(a)linux.intel.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> Reviewed-by: Bard Liao <yung-chuan.liao(a)linux.intel.com> Link: https://msgid.link/r/20240213115233.15716-1-peter.ujfalusi@linux.intel.com Signed-off-by: Mark Brown <broonie(a)kernel.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- sound/soc/sof/ipc4-pcm.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/sound/soc/sof/ipc4-pcm.c b/sound/soc/sof/ipc4-pcm.c index db19cd03ecad8..e8acf60c27a74 100644 --- a/sound/soc/sof/ipc4-pcm.c +++ b/sound/soc/sof/ipc4-pcm.c @@ -377,7 +377,18 @@ static int sof_ipc4_trigger_pipelines(struct snd_soc_component *component, ret = sof_ipc4_set_multi_pipeline_state(sdev, state, trigger_list); if (ret < 0) { dev_err(sdev->dev, "failed to set final state %d for all pipelines\n", state); - goto free; + /* + * workaround: if the firmware is crashed while setting the + * pipelines to reset state we must ignore the error code and + * reset it to 0. + * Since the firmware is crashed we will not send IPC messages + * and we are going to see errors printed, but the state of the + * widgets will be correct for the next boot. + */ + if (sdev->fw_state != SOF_FW_CRASHED || state != SOF_IPC4_PIPE_RESET) + goto free; + + ret = 0; } /* update RUNNING/RESET state for all pipelines that were just triggered */ -- 2.43.0

1 0

[PATCH AUTOSEL 6.7 23/26] ASoC: SOF: ipc4-pcm: Workaround for crashed firmware on system suspend
by Sasha Levin 29 Feb '24

29 Feb '24

From: Peter Ujfalusi <peter.ujfalusi(a)linux.intel.com> [ Upstream commit c40aad7c81e5fba34b70123ed7ce3397fa62a4d2 ] When the system is suspended while audio is active, the sof_ipc4_pcm_hw_free() is invoked to reset the pipelines since during suspend the DSP is turned off, streams will be re-started after resume. If the firmware crashes during while audio is running (or when we reset the stream before suspend) then the sof_ipc4_set_multi_pipeline_state() will fail with IPC error and the state change is interrupted. This will cause misalignment between the kernel and firmware state on next DSP boot resulting errors returned by firmware for IPC messages, eventually failing the audio resume. On stream close the errors are ignored so the kernel state will be corrected on the next DSP boot, so the second boot after the DSP panic. If sof_ipc4_trigger_pipelines() is called from sof_ipc4_pcm_hw_free() then state parameter is SOF_IPC4_PIPE_RESET and only in this case. Treat a forced pipeline reset similarly to how we treat a pcm_free by ignoring error on state sending to allow the kernel's state to be consistent with the state the firmware will have after the next boot. Link: https://github.com/thesofproject/sof/issues/8721 Signed-off-by: Peter Ujfalusi <peter.ujfalusi(a)linux.intel.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan(a)linux.intel.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> Reviewed-by: Bard Liao <yung-chuan.liao(a)linux.intel.com> Link: https://msgid.link/r/20240213115233.15716-1-peter.ujfalusi@linux.intel.com Signed-off-by: Mark Brown <broonie(a)kernel.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- sound/soc/sof/ipc4-pcm.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/sound/soc/sof/ipc4-pcm.c b/sound/soc/sof/ipc4-pcm.c index 39039a647cca3..ea70c0d7cf75a 100644 --- a/sound/soc/sof/ipc4-pcm.c +++ b/sound/soc/sof/ipc4-pcm.c @@ -413,7 +413,18 @@ static int sof_ipc4_trigger_pipelines(struct snd_soc_component *component, ret = sof_ipc4_set_multi_pipeline_state(sdev, state, trigger_list); if (ret < 0) { dev_err(sdev->dev, "failed to set final state %d for all pipelines\n", state); - goto free; + /* + * workaround: if the firmware is crashed while setting the + * pipelines to reset state we must ignore the error code and + * reset it to 0. + * Since the firmware is crashed we will not send IPC messages + * and we are going to see errors printed, but the state of the + * widgets will be correct for the next boot. + */ + if (sdev->fw_state != SOF_FW_CRASHED || state != SOF_IPC4_PIPE_RESET) + goto free; + + ret = 0; } /* update RUNNING/RESET state for all pipelines that were just triggered */ -- 2.43.0

1 0

Re: [PATCH v3 1/2] ASoC: SOF: amd: Move signed_fw_image to struct acp_quirk_entry
by AngeloGioacchino Del Regno 21 Feb '24

21 Feb '24

Il 21/02/24 11:29, Cristian Ciocaltea ha scritto: > On 2/21/24 11:35, AngeloGioacchino Del Regno wrote: >> Il 20/02/24 21:16, Cristian Ciocaltea ha scritto: >>> The signed_fw_image member of struct sof_amd_acp_desc is used to enable >>> signed firmware support in the driver via the acp_sof_quirk_table. >>> >>> In preparation to support additional use cases of the quirk table (i.e. >>> adding new flags), move signed_fw_image to a new struct acp_quirk_entry >>> and update all references to it accordingly. >>> >>> No functional changes intended. >>> >>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea(a)collabora.com> >>> --- >>> sound/soc/sof/amd/acp-loader.c | 2 +- >>> sound/soc/sof/amd/acp.c | 47 ++++++++++++++++++---------------- >>> sound/soc/sof/amd/acp.h | 6 ++++- >>> sound/soc/sof/amd/vangogh.c | 9 +++++-- >>> 4 files changed, 38 insertions(+), 26 deletions(-) >>> >>> diff --git a/sound/soc/sof/amd/acp-loader.c >>> b/sound/soc/sof/amd/acp-loader.c >>> index d2d21478399e..aad904839b81 100644 >>> --- a/sound/soc/sof/amd/acp-loader.c >>> +++ b/sound/soc/sof/amd/acp-loader.c >>> @@ -173,7 +173,7 @@ int acp_dsp_pre_fw_run(struct snd_sof_dev *sdev) >>> adata = sdev->pdata->hw_pdata; >>> - if (adata->signed_fw_image) >>> + if (adata->quirks && adata->quirks->signed_fw_image) >>> size_fw = adata->fw_bin_size - ACP_FIRMWARE_SIGNATURE; >>> else >>> size_fw = adata->fw_bin_size; >>> diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c >>> index 9b3c26210db3..9d9197fa83ed 100644 >>> --- a/sound/soc/sof/amd/acp.c >>> +++ b/sound/soc/sof/amd/acp.c >>> @@ -20,12 +20,14 @@ >>> #include "acp.h" >>> #include "acp-dsp-offset.h" >>> -#define SECURED_FIRMWARE 1 >>> - >>> static bool enable_fw_debug; >>> module_param(enable_fw_debug, bool, 0444); >>> MODULE_PARM_DESC(enable_fw_debug, "Enable Firmware debug"); >>> +static struct acp_quirk_entry quirk_valve_galileo = { >>> + .signed_fw_image = true, >> >> Hello Cristian, >> >> are you sure that a structure holding "quirks" is the right choice here? >> >> That probably comes as a personal preference, but I would simply pass a >> `u32 flags` >> and structure the quirks as bits. >> >> #define ACP_SIGNED_FW_IMAGE BIT(0) >> #define ACP_SOMETHING_ELSE BIT(1) >> >> flags = BIT(SIGNED_FW_IMAGE) | BIT(SOMETHING_ELSE); That should've been flags = SIGNED_FW_IMAGE | SOMETHING_ELSE; >> >> if (flags & BIT(SIGNED_FW_IMAGE)) and this if (flags & SIGNED_FW_IMAGE) ... I have no idea why I added a nested BIT(BIT()) in there, something in my brain started ticking sideways, lol. >> do_something() >> >> What do you think? > > Hi Angelo, > > The flags approach was actually my first thought and I think that would > have been the best choice if the quirks usage was limited to a single > file (acp.c). > > Since they need to be exposed externally as well (acp-loader.c, > vangogh.c) and already using a dedicated member in struct > sof_amd_acp_desc related to the existing quirk, I found the "quirks" > struct solution a bit more natural/convenient to follow (I've done a bit > of research before and noticed other drivers having similar handling). > > However, as you already pointed out, it may also come down to individual > preferences, so I'm open to using the flags if there is not enough > reasoning to stick with the current implementation. Of course the definitions should be put in a "common header" for that to actually work in your described situation, but it's not a big deal. Mine wasn't a strong opinion: that does actually matter in case you expect more quirks (or something that is not a quirk, but a functional flag instead) to eventually get there... but otherwise, it's actually the same. It's your choice in the end, I'm fine with both anyway :-) Cheers, Angelo

1 0

Re: [PATCH v3 1/2] ASoC: SOF: amd: Move signed_fw_image to struct acp_quirk_entry
by AngeloGioacchino Del Regno 21 Feb '24

21 Feb '24

Il 20/02/24 21:16, Cristian Ciocaltea ha scritto: > The signed_fw_image member of struct sof_amd_acp_desc is used to enable > signed firmware support in the driver via the acp_sof_quirk_table. > > In preparation to support additional use cases of the quirk table (i.e. > adding new flags), move signed_fw_image to a new struct acp_quirk_entry > and update all references to it accordingly. > > No functional changes intended. > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea(a)collabora.com> > --- > sound/soc/sof/amd/acp-loader.c | 2 +- > sound/soc/sof/amd/acp.c | 47 ++++++++++++++++++---------------- > sound/soc/sof/amd/acp.h | 6 ++++- > sound/soc/sof/amd/vangogh.c | 9 +++++-- > 4 files changed, 38 insertions(+), 26 deletions(-) > > diff --git a/sound/soc/sof/amd/acp-loader.c b/sound/soc/sof/amd/acp-loader.c > index d2d21478399e..aad904839b81 100644 > --- a/sound/soc/sof/amd/acp-loader.c > +++ b/sound/soc/sof/amd/acp-loader.c > @@ -173,7 +173,7 @@ int acp_dsp_pre_fw_run(struct snd_sof_dev *sdev) > > adata = sdev->pdata->hw_pdata; > > - if (adata->signed_fw_image) > + if (adata->quirks && adata->quirks->signed_fw_image) > size_fw = adata->fw_bin_size - ACP_FIRMWARE_SIGNATURE; > else > size_fw = adata->fw_bin_size; > diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c > index 9b3c26210db3..9d9197fa83ed 100644 > --- a/sound/soc/sof/amd/acp.c > +++ b/sound/soc/sof/amd/acp.c > @@ -20,12 +20,14 @@ > #include "acp.h" > #include "acp-dsp-offset.h" > > -#define SECURED_FIRMWARE 1 > - > static bool enable_fw_debug; > module_param(enable_fw_debug, bool, 0444); > MODULE_PARM_DESC(enable_fw_debug, "Enable Firmware debug"); > > +static struct acp_quirk_entry quirk_valve_galileo = { > + .signed_fw_image = true, Hello Cristian, are you sure that a structure holding "quirks" is the right choice here? That probably comes as a personal preference, but I would simply pass a `u32 flags` and structure the quirks as bits. #define ACP_SIGNED_FW_IMAGE BIT(0) #define ACP_SOMETHING_ELSE BIT(1) flags = BIT(SIGNED_FW_IMAGE) | BIT(SOMETHING_ELSE); if (flags & BIT(SIGNED_FW_IMAGE)) do_something() What do you think? Cheers, Angelo > +}; > + > const struct dmi_system_id acp_sof_quirk_table[] = { > { > /* Steam Deck OLED device */ > @@ -33,7 +35,7 @@ const struct dmi_system_id acp_sof_quirk_table[] = { > DMI_MATCH(DMI_SYS_VENDOR, "Valve"), > DMI_MATCH(DMI_PRODUCT_NAME, "Galileo"), > }, > - .driver_data = (void *)SECURED_FIRMWARE, > + .driver_data = &quirk_valve_galileo, > }, > {} > }; > @@ -254,7 +256,7 @@ int configure_and_run_sha_dma(struct acp_dev_data *adata, void *image_addr, > } > } > > - if (adata->signed_fw_image) > + if (adata->quirks && adata->quirks->signed_fw_image) > snd_sof_dsp_write(sdev, ACP_DSP_BAR, ACP_SHA_DMA_INCLUDE_HDR, ACP_SHA_HEADER); > > snd_sof_dsp_write(sdev, ACP_DSP_BAR, ACP_SHA_DMA_STRT_ADDR, start_addr); > @@ -738,26 +740,27 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev) > sdev->debug_box.offset = sdev->host_box.offset + sdev->host_box.size; > sdev->debug_box.size = BOX_SIZE_1024; > > - adata->signed_fw_image = false; > dmi_id = dmi_first_match(acp_sof_quirk_table); > - if (dmi_id && dmi_id->driver_data) { > - adata->fw_code_bin = devm_kasprintf(sdev->dev, GFP_KERNEL, > - "sof-%s-code.bin", > - chip->name); > - if (!adata->fw_code_bin) { > - ret = -ENOMEM; > - goto free_ipc_irq; > + if (dmi_id) { > + adata->quirks = dmi_id->driver_data; > + > + if (adata->quirks->signed_fw_image) { > + adata->fw_code_bin = devm_kasprintf(sdev->dev, GFP_KERNEL, > + "sof-%s-code.bin", > + chip->name); > + if (!adata->fw_code_bin) { > + ret = -ENOMEM; > + goto free_ipc_irq; > + } > + > + adata->fw_data_bin = devm_kasprintf(sdev->dev, GFP_KERNEL, > + "sof-%s-data.bin", > + chip->name); > + if (!adata->fw_data_bin) { > + ret = -ENOMEM; > + goto free_ipc_irq; > + } > } > - > - adata->fw_data_bin = devm_kasprintf(sdev->dev, GFP_KERNEL, > - "sof-%s-data.bin", > - chip->name); > - if (!adata->fw_data_bin) { > - ret = -ENOMEM; > - goto free_ipc_irq; > - } > - > - adata->signed_fw_image = dmi_id->driver_data; > } > > adata->enable_fw_debug = enable_fw_debug; > diff --git a/sound/soc/sof/amd/acp.h b/sound/soc/sof/amd/acp.h > index 947068da39b5..b648ed194b9f 100644 > --- a/sound/soc/sof/amd/acp.h > +++ b/sound/soc/sof/amd/acp.h > @@ -207,6 +207,10 @@ struct sof_amd_acp_desc { > u64 sdw_acpi_dev_addr; > }; > > +struct acp_quirk_entry { > + bool signed_fw_image; > +}; > + > /* Common device data struct for ACP devices */ > struct acp_dev_data { > struct snd_sof_dev *dev; > @@ -236,7 +240,7 @@ struct acp_dev_data { > u8 *data_buf; > dma_addr_t sram_dma_addr; > u8 *sram_data_buf; > - bool signed_fw_image; > + struct acp_quirk_entry *quirks; > struct dma_descriptor dscr_info[ACP_MAX_DESC]; > struct acp_dsp_stream stream_buf[ACP_MAX_STREAM]; > struct acp_dsp_stream *dtrace_stream; > diff --git a/sound/soc/sof/amd/vangogh.c b/sound/soc/sof/amd/vangogh.c > index de15d21aa6d9..bc6ffdb5471a 100644 > --- a/sound/soc/sof/amd/vangogh.c > +++ b/sound/soc/sof/amd/vangogh.c > @@ -143,6 +143,7 @@ EXPORT_SYMBOL_NS(sof_vangogh_ops, SND_SOC_SOF_AMD_COMMON); > int sof_vangogh_ops_init(struct snd_sof_dev *sdev) > { > const struct dmi_system_id *dmi_id; > + struct acp_quirk_entry *quirks; > > /* common defaults */ > memcpy(&sof_vangogh_ops, &sof_acp_common_ops, sizeof(struct snd_sof_dsp_ops)); > @@ -151,8 +152,12 @@ int sof_vangogh_ops_init(struct snd_sof_dev *sdev) > sof_vangogh_ops.num_drv = ARRAY_SIZE(vangogh_sof_dai); > > dmi_id = dmi_first_match(acp_sof_quirk_table); > - if (dmi_id && dmi_id->driver_data) > - sof_vangogh_ops.load_firmware = acp_sof_load_signed_firmware; > + if (dmi_id) { > + quirks = dmi_id->driver_data; > + > + if (quirks->signed_fw_image) > + sof_vangogh_ops.load_firmware = acp_sof_load_signed_firmware; > + } > > return 0; > }

1 0

[PATCH] ASoC: SOF: amd: fix soundwire dependencies
by Arnd Bergmann 21 Feb '24

21 Feb '24

From: Arnd Bergmann <arnd(a)arndb.de> The soundwire-amd driver has a bit of a layering violation requiring the SOF driver to directly call into its exported symbols rather than through an abstraction. The SND_SOC_SOF_AMD_SOUNDWIRE Kconfig symbol tries to deal with the dependency by selecting SOUNDWIRE_AMD in a complicated set of conditions, but gets it wrong for a configuration involving SND_SOC_SOF_AMD_COMMON=y, SND_SOC_SOF_AMD_ACP63=m, and SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE=m SOUNDWIRE_AMD=m, which results in a link failure: ld.lld: error: undefined symbol: sdw_amd_get_slave_info >>> referenced by acp-common.c ld.lld: error: undefined symbol: amd_sdw_scan_controller ld.lld: error: undefined symbol: sdw_amd_probe ld.lld: error: undefined symbol: sdw_amd_exit >>> referenced by acp.c >>> sound/soc/sof/amd/acp.o:(amd_sof_acp_remove) in archive vmlinux.a In essence, the SND_SOC_SOF_AMD_COMMON option cannot be built-in when trying to link against a modular SOUNDWIRE_AMD driver. Since CONFIG_SOUNDWIRE_AMD is a user-visible option, it really should never be selected by another driver in the first place, so replace the extra complexity with a normal Kconfig dependency in SND_SOC_SOF_AMD_SOUNDWIRE, plus a top-level check that forbids any of the AMD SOF drivers from being built-in with CONFIG_SOUNDWIRE_AMD=m. In normal configs, they should all either be built-in or all loadable modules anyway, so this simplification does not limit any real usecases. Fixes: d948218424bf ("ASoC: SOF: amd: add code for invoking soundwire manager helper functions") Signed-off-by: Arnd Bergmann <arnd(a)arndb.de> --- sound/soc/sof/amd/Kconfig | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sound/soc/sof/amd/Kconfig b/sound/soc/sof/amd/Kconfig index c3bbe6c70fb2..2729c6eb3feb 100644 --- a/sound/soc/sof/amd/Kconfig +++ b/sound/soc/sof/amd/Kconfig @@ -6,6 +6,7 @@ config SND_SOC_SOF_AMD_TOPLEVEL tristate "SOF support for AMD audio DSPs" + depends on SOUNDWIRE_AMD || !SOUNDWIRE_AMD depends on X86 || COMPILE_TEST help This adds support for Sound Open Firmware for AMD platforms. @@ -62,15 +63,14 @@ config SND_SOC_SOF_ACP_PROBES config SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE tristate - select SOUNDWIRE_AMD if SND_SOC_SOF_AMD_SOUNDWIRE != n select SND_AMD_SOUNDWIRE_ACPI if ACPI config SND_SOC_SOF_AMD_SOUNDWIRE tristate "SOF support for SoundWire based AMD platforms" default SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE depends on SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE - depends on ACPI && SOUNDWIRE - depends on !(SOUNDWIRE=m && SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE=y) + depends on ACPI + depends on SOUNDWIRE_AMD help This adds support for SoundWire with Sound Open Firmware for AMD platforms. -- 2.39.2

4 9