This patch series is a continuation of [1] to provide fixes and improvements to
SOF drivers targeting the Vangogh platform, as found on the Valve's Steam Deck
OLED.
The previous series only handled the legacy ACP drivers.
[1]: https://lore.kernel.org/all/[email protected]/
Cristian Ciocaltea (11):
ASoC: amd: acp: Drop redundant initialization of machine driver data
ASoC: amd: acp: Make use of existing *_CODEC_DAI macros
ASoC: amd: acp: Add missing error handling in sof-mach
ASoC: amd: acp: Update MODULE_DESCRIPTION for sof-mach
ASoC: SOF: amd: Fix memory leak in amd_sof_acp_probe()
ASoC: SOF: amd: Optimize quirk for Valve Galileo
ASoC: SOF: core: Skip firmware test for undefined fw_name
ASoC: SOF: amd: Override default fw name for Valve Galileo
ASoC: SOF: amd: Compute file paths on firmware load
ASoC: amd: acp: Use correct DAI link ID for BT codec
ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT
sound/soc/amd/acp/acp-mach-common.c | 6 +++---
sound/soc/amd/acp/acp-mach.h | 2 +-
sound/soc/amd/acp/acp-sof-mach.c | 26 +++++++----------------
sound/soc/sof/amd/acp-loader.c | 32 +++++++++++++++++++++++------
sound/soc/sof/amd/acp.c | 30 ++++++++++++++++-----------
sound/soc/sof/amd/vangogh.c | 8 +++++++-
sound/soc/sof/fw-file-profile.c | 3 +++
sound/soc/sof/topology.c | 1 +
8 files changed, 66 insertions(+), 42 deletions(-)
--
2.43.0
The generic ACP machine driver provides macros for NAU88221 and MAX98388
codec DAI names, but in places it is still using directly the related
strings.
For consistency, replace all strings with the equivalent macros.
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
sound/soc/amd/acp/acp-mach-common.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/amd/acp/acp-mach-common.c b/sound/soc/amd/acp/acp-mach-common.c
index c90ec3419247..346f7514c81a 100644
--- a/sound/soc/amd/acp/acp-mach-common.c
+++ b/sound/soc/amd/acp/acp-mach-common.c
@@ -821,8 +821,8 @@ static const struct snd_soc_ops acp_card_maxim_ops = {
};
SND_SOC_DAILINK_DEF(max98388,
- DAILINK_COMP_ARRAY(COMP_CODEC("i2c-ADS8388:00", "max98388-aif1"),
- COMP_CODEC("i2c-ADS8388:01", "max98388-aif1")));
+ DAILINK_COMP_ARRAY(COMP_CODEC("i2c-ADS8388:00", MAX98388_CODEC_DAI),
+ COMP_CODEC("i2c-ADS8388:01", MAX98388_CODEC_DAI)));
static const struct snd_kcontrol_new max98388_controls[] = {
SOC_DAPM_PIN_SWITCH("Left Spk"),
@@ -1273,7 +1273,7 @@ static const struct snd_soc_ops acp_8821_ops = {
SND_SOC_DAILINK_DEF(nau8821,
DAILINK_COMP_ARRAY(COMP_CODEC("i2c-NVTN2020:00",
- "nau8821-hifi")));
+ NAU8821_CODEC_DAI)));
/* Declare DMIC codec components */
SND_SOC_DAILINK_DEF(dmic_codec,
--
2.43.0
Driver uses kasprintf() to initialize fw_{code,data}_bin members of
struct acp_dev_data, but kfree() is never called to deallocate the
memory, which results in a memory leak.
Fix the issue by switching to devm_kasprintf(). Additionally, ensure the
allocation was successful by checking the pointer validity.
Fixes: f7da88003c53 ("ASoC: SOF: amd: Enable signed firmware image loading for Vangogh platform")
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
sound/soc/sof/amd/acp.c | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c
index 603ea5fc0d0d..c6f637f29847 100644
--- a/sound/soc/sof/amd/acp.c
+++ b/sound/soc/sof/amd/acp.c
@@ -547,17 +547,27 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev)
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 = kasprintf(GFP_KERNEL, "%s/sof-%s-code.bin",
- plat_data->fw_filename_prefix,
- chip->name);
- adata->fw_data_bin = kasprintf(GFP_KERNEL, "%s/sof-%s-data.bin",
- plat_data->fw_filename_prefix,
- chip->name);
- adata->signed_fw_image = dmi_id->driver_data;
+ adata->fw_code_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
+ "%s/sof-%s-code.bin",
+ plat_data->fw_filename_prefix,
+ chip->name);
+ if (!adata->fw_code_bin) {
+ ret = -ENOMEM;
+ goto free_ipc_irq;
+ }
+
+ adata->fw_data_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
+ "%s/sof-%s-data.bin",
+ plat_data->fw_filename_prefix,
+ chip->name);
+ if (!adata->fw_data_bin) {
+ ret = -ENOMEM;
+ goto free_ipc_irq;
+ }
- dev_dbg(sdev->dev, "fw_code_bin:%s, fw_data_bin:%s\n", adata->fw_code_bin,
- adata->fw_data_bin);
+ adata->signed_fw_image = dmi_id->driver_data;
}
+
adata->enable_fw_debug = enable_fw_debug;
acp_memory_init(sdev);
--
2.43.0
The current MODULE_DESCRIPTION relates to a Chrome board, as that was
what the driver initially supported.
Nonetheless, it has since progressed incrementally and evolved into a
more comprehensive machine driver. Hence, update MODULE_DESCRIPTION to
better reflect this.
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
sound/soc/amd/acp/acp-sof-mach.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/amd/acp/acp-sof-mach.c b/sound/soc/amd/acp/acp-sof-mach.c
index 6f0ca23638af..19ff4fe5b1ea 100644
--- a/sound/soc/amd/acp/acp-sof-mach.c
+++ b/sound/soc/amd/acp/acp-sof-mach.c
@@ -166,7 +166,7 @@ static struct platform_driver acp_asoc_audio = {
module_platform_driver(acp_asoc_audio);
MODULE_IMPORT_NS(SND_SOC_AMD_MACH);
-MODULE_DESCRIPTION("ACP chrome SOF audio support");
+MODULE_DESCRIPTION("ACP SOF Machine Driver");
MODULE_ALIAS("platform:rt5682-rt1019");
MODULE_ALIAS("platform:rt5682-max");
MODULE_ALIAS("platform:rt5682s-max");
--
2.43.0
Some SOF drivers like AMD ACP do not always rely on a single static
firmware file, but may require multiple files having their names
dynamically computed on probe time, e.g. based on chip name.
In those cases, providing an invalid default_fw_filename in their
sof_dev_desc struct will prevent probing due to 'SOF firmware
and/or topology file not found' error.
Fix the issue by allowing drivers to omit initialization for this member
(or alternatively provide a dynamic override via ipc_file_profile_base)
and update sof_test_firmware_file() to verify the given profile data and
skip firmware testing if either fw_path or fw_name is not defined.
Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing")
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
sound/soc/sof/fw-file-profile.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
index 138a1ca2c4a8..e63700234df0 100644
--- a/sound/soc/sof/fw-file-profile.c
+++ b/sound/soc/sof/fw-file-profile.c
@@ -21,6 +21,9 @@ static int sof_test_firmware_file(struct device *dev,
const u32 *magic;
int ret;
+ if (!profile->fw_path || !profile->fw_name || !*profile->fw_name)
+ return 0;
+
fw_filename = kasprintf(GFP_KERNEL, "%s/%s", profile->fw_path,
profile->fw_name);
if (!fw_filename)
--
2.43.0
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 <[email protected]>
---
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 c6f637f29847..1e9840ae8938 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
Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
creation for I2S BT instance") added I2S BT support in ACP common
machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2:
[ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version 0:0:0-7863d
[ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0 Kernel ABI 3:23:0
[ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0 Kernel ABI 3:23:0
[ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id 2) not exist
[ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load header: -22
[ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22
[ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22
[ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22
[ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22
[ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max)
[ 8.545022] sof_mach: probe of nau8821-max failed with error -22
Move BT_BE_ID to the correct position in the enum.
Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink creation for I2S BT instance")
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
sound/soc/amd/acp/acp-mach.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h
index a48546d8d407..0c18ccd29305 100644
--- a/sound/soc/amd/acp/acp-mach.h
+++ b/sound/soc/amd/acp/acp-mach.h
@@ -27,8 +27,8 @@
enum be_id {
HEADSET_BE_ID = 0,
AMP_BE_ID,
- DMIC_BE_ID,
BT_BE_ID,
+ DMIC_BE_ID,
};
enum cpu_endpoints {
--
2.43.0
The ACP driver for Vangogh platform uses a quirk for Valve Galileo
device to setup a custom firmware loader, which neither requires nor
uses the firmware file indicated via the default_fw_filename member of
struct sof_dev_desc.
Since commit 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version
fallback if firmware files are missing"), the provided filename gets
verified and triggers a fatal error on probe:
[ 7.719337] snd_sof_amd_vangogh 0000:04:00.5: enabling device (0000 -> 0002)
[ 7.721486] snd_sof_amd_vangogh 0000:04:00.5: SOF firmware and/or topology file not found.
[ 7.721565] snd_sof_amd_vangogh 0000:04:00.5: Supported default profiles
[ 7.721569] snd_sof_amd_vangogh 0000:04:00.5: - ipc type 0 (Requested):
[ 7.721573] snd_sof_amd_vangogh 0000:04:00.5: Firmware file: amd/sof/sof-vangogh.ri
[ 7.721577] snd_sof_amd_vangogh 0000:04:00.5: Topology file: amd/sof-tplg/sof-vangogh-nau8821-max.tplg
[ 7.721582] snd_sof_amd_vangogh 0000:04:00.5: Check if you have 'sof-firmware' package installed.
[ 7.721585] snd_sof_amd_vangogh 0000:04:00.5: Optionally it can be manually downloaded from:
[ 7.721589] snd_sof_amd_vangogh 0000:04:00.5: https://github.com/thesofproject/sof-bin/
[ 7.721997] snd_sof_amd_vangogh: probe of 0000:04:00.5 failed with error -2
Skip testing the default firmware by overriding fw_name in
sof_vangogh_ops_init().
Fixes: d0dab6b76a9f ("ASoC: SOF: amd: Add sof support for vangogh platform")
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
sound/soc/sof/amd/vangogh.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/sound/soc/sof/amd/vangogh.c b/sound/soc/sof/amd/vangogh.c
index de15d21aa6d9..5843ff8a8b40 100644
--- a/sound/soc/sof/amd/vangogh.c
+++ b/sound/soc/sof/amd/vangogh.c
@@ -151,8 +151,14 @@ 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)
+ if (dmi_id && dmi_id->driver_data) {
sof_vangogh_ops.load_firmware = acp_sof_load_signed_firmware;
+ /*
+ * Board doesn't use the default firmware, hence override
+ * its name to prevent probe error due to fw validation.
+ */
+ sdev->pdata->ipc_file_profile_base.fw_name = "";
+ }
return 0;
}
--
2.43.0
Commit 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if
firmware files are missing") changed the order of some operations and
the firmware paths are not available anymore at snd_sof_probe() time.
Precisely, fw_filename_prefix is set by sof_select_ipc_and_paths() via
plat_data->fw_filename_prefix = out_profile.fw_path;
but sof_init_environment() which calls this function was moved from
snd_sof_device_probe() to sof_probe_continue(). Moreover,
snd_sof_probe() was moved from sof_probe_continue() to
sof_init_environment(), but before the call to
sof_select_ipc_and_paths().
The problem here is that amd_sof_acp_probe() uses fw_filename_prefix to
compute fw_code_bin and fw_data_bin paths, and because the field is not
yet initialized, the paths end up containing (null):
snd_sof_amd_vangogh 0000:04:00.5: Direct firmware load for (null)/sof-vangogh-code.bin failed with error -2
snd_sof_amd_vangogh 0000:04:00.5: sof signed firmware code bin is missing
snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP firmware -2
snd_sof_amd_vangogh: probe of 0000:04:00.5 failed with error -2
Move usage of fw_filename_prefix right before request_firmware() calls
in acp_sof_load_signed_firmware().
Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing")
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
sound/soc/sof/amd/acp-loader.c | 32 ++++++++++++++++++++++++++------
sound/soc/sof/amd/acp.c | 7 ++-----
2 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/sound/soc/sof/amd/acp-loader.c b/sound/soc/sof/amd/acp-loader.c
index e05eb7a86dd4..d2d21478399e 100644
--- a/sound/soc/sof/amd/acp-loader.c
+++ b/sound/soc/sof/amd/acp-loader.c
@@ -267,29 +267,49 @@ int acp_sof_load_signed_firmware(struct snd_sof_dev *sdev)
{
struct snd_sof_pdata *plat_data = sdev->pdata;
struct acp_dev_data *adata = plat_data->hw_pdata;
+ const char *fw_filename;
int ret;
- ret = request_firmware(&sdev->basefw.fw, adata->fw_code_bin, sdev->dev);
+ fw_filename = kasprintf(GFP_KERNEL, "%s/%s",
+ plat_data->fw_filename_prefix,
+ adata->fw_code_bin);
+ if (!fw_filename)
+ return -ENOMEM;
+
+ ret = request_firmware(&sdev->basefw.fw, fw_filename, sdev->dev);
if (ret < 0) {
+ kfree(fw_filename);
dev_err(sdev->dev, "sof signed firmware code bin is missing\n");
return ret;
} else {
- dev_dbg(sdev->dev, "request_firmware %s successful\n", adata->fw_code_bin);
+ dev_dbg(sdev->dev, "request_firmware %s successful\n", fw_filename);
}
+ kfree(fw_filename);
+
ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_IRAM, 0,
- (void *)sdev->basefw.fw->data, sdev->basefw.fw->size);
+ (void *)sdev->basefw.fw->data,
+ sdev->basefw.fw->size);
+
+ fw_filename = kasprintf(GFP_KERNEL, "%s/%s",
+ plat_data->fw_filename_prefix,
+ adata->fw_data_bin);
+ if (!fw_filename)
+ return -ENOMEM;
- ret = request_firmware(&adata->fw_dbin, adata->fw_data_bin, sdev->dev);
+ ret = request_firmware(&adata->fw_dbin, fw_filename, sdev->dev);
if (ret < 0) {
+ kfree(fw_filename);
dev_err(sdev->dev, "sof signed firmware data bin is missing\n");
return ret;
} else {
- dev_dbg(sdev->dev, "request_firmware %s successful\n", adata->fw_data_bin);
+ dev_dbg(sdev->dev, "request_firmware %s successful\n", fw_filename);
}
+ kfree(fw_filename);
ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_DRAM, 0,
- (void *)adata->fw_dbin->data, adata->fw_dbin->size);
+ (void *)adata->fw_dbin->data,
+ adata->fw_dbin->size);
return ret;
}
EXPORT_SYMBOL_NS(acp_sof_load_signed_firmware, SND_SOC_SOF_AMD_COMMON);
diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c
index 1e9840ae8938..87c5c71eac68 100644
--- a/sound/soc/sof/amd/acp.c
+++ b/sound/soc/sof/amd/acp.c
@@ -479,7 +479,6 @@ EXPORT_SYMBOL_NS(amd_sof_acp_resume, SND_SOC_SOF_AMD_COMMON);
int amd_sof_acp_probe(struct snd_sof_dev *sdev)
{
struct pci_dev *pci = to_pci_dev(sdev->dev);
- struct snd_sof_pdata *plat_data = sdev->pdata;
struct acp_dev_data *adata;
const struct sof_amd_acp_desc *chip;
const struct dmi_system_id *dmi_id;
@@ -547,8 +546,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev)
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,
- "%s/sof-%s-code.bin",
- plat_data->fw_filename_prefix,
+ "sof-%s-code.bin",
chip->name);
if (!adata->fw_code_bin) {
ret = -ENOMEM;
@@ -556,8 +554,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev)
}
adata->fw_data_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
- "%s/sof-%s-data.bin",
- plat_data->fw_filename_prefix,
+ "sof-%s-data.bin",
chip->name);
if (!adata->fw_data_bin) {
ret = -ENOMEM;
--
2.43.0
Commit efb931cdc4b9 ("ASoC: SOF: topology: Add support for AMD ACP
DAIs") registered "ACP" name for SOF_DAI_AMD_BT DAI type. However, some
boards, i.e. Steam Deck OLED, seem to require "ACPBT" for the same type:
[ 467.489680] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for 0x30030000 (msg/reply size: 16/0): -22
[ 467.492775] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_route_setup: route ACPBT2.IN -> BUF5.0 failed
[ 467.495839] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_set_up_all_pipelines: route set up failed
[ 467.499128] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22
[ 467.502210] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22
[ 467.505289] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22
[ 467.508430] sof_mach nau8821-max: ASoC: failed to instantiate card -22
[ 467.511725] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max)
[ 467.514861] sof_mach: probe of nau8821-max failed with error -22
Add "ACPBT" alias for "ACP" SOF DAI type.
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
sound/soc/sof/topology.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
index e3e7fbe40fa6..73bf791e7520 100644
--- a/sound/soc/sof/topology.c
+++ b/sound/soc/sof/topology.c
@@ -290,6 +290,7 @@ static const struct sof_dai_types sof_dais[] = {
{"SAI", SOF_DAI_IMX_SAI},
{"ESAI", SOF_DAI_IMX_ESAI},
{"ACP", SOF_DAI_AMD_BT},
+ {"ACPBT", SOF_DAI_AMD_BT},
{"ACPSP", SOF_DAI_AMD_SP},
{"ACPDMIC", SOF_DAI_AMD_DMIC},
{"ACPHS", SOF_DAI_AMD_HS},
--
2.43.0
On 12/10/23 02:23, Cristian Ciocaltea wrote:
> Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
> creation for I2S BT instance") added I2S BT support in ACP common
> machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2:
>
> [ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version 0:0:0-7863d
> [ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0 Kernel ABI 3:23:0
> [ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0 Kernel ABI 3:23:0
> [ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id 2) not exist
> [ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load header: -22
> [ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22
> [ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22
> [ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22
> [ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22
> [ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max)
> [ 8.545022] sof_mach: probe of nau8821-max failed with error -22
>
> Move BT_BE_ID to the correct position in the enum.
>
> Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink creation for I2S BT instance")
> Signed-off-by: Cristian Ciocaltea <[email protected]>
> ---
> sound/soc/amd/acp/acp-mach.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h
> index a48546d8d407..0c18ccd29305 100644
> --- a/sound/soc/amd/acp/acp-mach.h
> +++ b/sound/soc/amd/acp/acp-mach.h
> @@ -27,8 +27,8 @@
> enum be_id {
> HEADSET_BE_ID = 0,
> AMP_BE_ID,
> - DMIC_BE_ID,
> BT_BE_ID,
> + DMIC_BE_ID,
This will break the other platforms as this same enum used in topology
to create dailink.
> };
>
> enum cpu_endpoints {
On 12/10/23 02:23, Cristian Ciocaltea wrote:
> Commit efb931cdc4b9 ("ASoC: SOF: topology: Add support for AMD ACP
> DAIs") registered "ACP" name for SOF_DAI_AMD_BT DAI type. However, some
> boards, i.e. Steam Deck OLED, seem to require "ACPBT" for the same type:
>
> [ 467.489680] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for 0x30030000 (msg/reply size: 16/0): -22
> [ 467.492775] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_route_setup: route ACPBT2.IN -> BUF5.0 failed
> [ 467.495839] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_set_up_all_pipelines: route set up failed
> [ 467.499128] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22
> [ 467.502210] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22
> [ 467.505289] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22
> [ 467.508430] sof_mach nau8821-max: ASoC: failed to instantiate card -22
> [ 467.511725] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max)
> [ 467.514861] sof_mach: probe of nau8821-max failed with error -22
>
> Add "ACPBT" alias for "ACP" SOF DAI type.
>
> Signed-off-by: Cristian Ciocaltea <[email protected]>
> ---
> sound/soc/sof/topology.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
> index e3e7fbe40fa6..73bf791e7520 100644
> --- a/sound/soc/sof/topology.c
> +++ b/sound/soc/sof/topology.c
> @@ -290,6 +290,7 @@ static const struct sof_dai_types sof_dais[] = {
> {"SAI", SOF_DAI_IMX_SAI},
> {"ESAI", SOF_DAI_IMX_ESAI},
> {"ACP", SOF_DAI_AMD_BT},
> + {"ACPBT", SOF_DAI_AMD_BT},
No need to create the alias name, we can directly modify ACP to ACPBT as
ACP is not using anywhere.
> {"ACPSP", SOF_DAI_AMD_SP},
> {"ACPDMIC", SOF_DAI_AMD_DMIC},
> {"ACPHS", SOF_DAI_AMD_HS},
On 12/10/23 02:23, Cristian Ciocaltea wrote:
> 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 <[email protected]>
> ---
> 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 c6f637f29847..1e9840ae8938 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 */
If any changes in SOF drivers, first need to create a PR in SOF github.
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "Valve"),
> DMI_MATCH(DMI_PRODUCT_NAME, "Galileo"),
> - DMI_MATCH(DMI_PRODUCT_FAMILY, "Sephiroth"),
> },
> .driver_data = (void *)SECURED_FIRMWARE,
> },
On 12/10/23 02:23, Cristian Ciocaltea wrote:
> Commit 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if
> firmware files are missing") changed the order of some operations and
> the firmware paths are not available anymore at snd_sof_probe() time.
>
> Precisely, fw_filename_prefix is set by sof_select_ipc_and_paths() via
>
> plat_data->fw_filename_prefix = out_profile.fw_path;
>
> but sof_init_environment() which calls this function was moved from
> snd_sof_device_probe() to sof_probe_continue(). Moreover,
> snd_sof_probe() was moved from sof_probe_continue() to
> sof_init_environment(), but before the call to
> sof_select_ipc_and_paths().
>
> The problem here is that amd_sof_acp_probe() uses fw_filename_prefix to
> compute fw_code_bin and fw_data_bin paths, and because the field is not
> yet initialized, the paths end up containing (null):
>
> snd_sof_amd_vangogh 0000:04:00.5: Direct firmware load for (null)/sof-vangogh-code.bin failed with error -2
> snd_sof_amd_vangogh 0000:04:00.5: sof signed firmware code bin is missing
> snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP firmware -2
> snd_sof_amd_vangogh: probe of 0000:04:00.5 failed with error -2
>
> Move usage of fw_filename_prefix right before request_firmware() calls
> in acp_sof_load_signed_firmware().
>
> Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing")
> Signed-off-by: Cristian Ciocaltea <[email protected]>
> ---
> sound/soc/sof/amd/acp-loader.c | 32 ++++++++++++++++++++++++++------
> sound/soc/sof/amd/acp.c | 7 ++-----
> 2 files changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/sound/soc/sof/amd/acp-loader.c b/sound/soc/sof/amd/acp-loader.c
> index e05eb7a86dd4..d2d21478399e 100644
> --- a/sound/soc/sof/amd/acp-loader.c
> +++ b/sound/soc/sof/amd/acp-loader.c
> @@ -267,29 +267,49 @@ int acp_sof_load_signed_firmware(struct snd_sof_dev *sdev)
> {
> struct snd_sof_pdata *plat_data = sdev->pdata;
> struct acp_dev_data *adata = plat_data->hw_pdata;
> + const char *fw_filename;
> int ret;
>
> - ret = request_firmware(&sdev->basefw.fw, adata->fw_code_bin, sdev->dev);
> + fw_filename = kasprintf(GFP_KERNEL, "%s/%s",
> + plat_data->fw_filename_prefix,
> + adata->fw_code_bin);
File path already saved in adata->fw_code_bin in amd_sof_acp_probe function.
No need to get it again.
> + if (!fw_filename)
> + return -ENOMEM;
> +
> + ret = request_firmware(&sdev->basefw.fw, fw_filename, sdev->dev);
> if (ret < 0) {
> + kfree(fw_filename);
> dev_err(sdev->dev, "sof signed firmware code bin is missing\n");
> return ret;
> } else {
> - dev_dbg(sdev->dev, "request_firmware %s successful\n", adata->fw_code_bin);
> + dev_dbg(sdev->dev, "request_firmware %s successful\n", fw_filename);
> }
> + kfree(fw_filename);
> +
> ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_IRAM, 0,
> - (void *)sdev->basefw.fw->data, sdev->basefw.fw->size);
> + (void *)sdev->basefw.fw->data,
> + sdev->basefw.fw->size);
> +
> + fw_filename = kasprintf(GFP_KERNEL, "%s/%s",
> + plat_data->fw_filename_prefix,
> + adata->fw_data_bin);
> + if (!fw_filename)
> + return -ENOMEM;
>
> - ret = request_firmware(&adata->fw_dbin, adata->fw_data_bin, sdev->dev);
> + ret = request_firmware(&adata->fw_dbin, fw_filename, sdev->dev);
> if (ret < 0) {
> + kfree(fw_filename);
> dev_err(sdev->dev, "sof signed firmware data bin is missing\n");
> return ret;
>
> } else {
> - dev_dbg(sdev->dev, "request_firmware %s successful\n", adata->fw_data_bin);
> + dev_dbg(sdev->dev, "request_firmware %s successful\n", fw_filename);
> }
> + kfree(fw_filename);
>
> ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_DRAM, 0,
> - (void *)adata->fw_dbin->data, adata->fw_dbin->size);
> + (void *)adata->fw_dbin->data,
> + adata->fw_dbin->size);
> return ret;
> }
> EXPORT_SYMBOL_NS(acp_sof_load_signed_firmware, SND_SOC_SOF_AMD_COMMON);
> diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c
> index 1e9840ae8938..87c5c71eac68 100644
> --- a/sound/soc/sof/amd/acp.c
> +++ b/sound/soc/sof/amd/acp.c
> @@ -479,7 +479,6 @@ EXPORT_SYMBOL_NS(amd_sof_acp_resume, SND_SOC_SOF_AMD_COMMON);
> int amd_sof_acp_probe(struct snd_sof_dev *sdev)
> {
> struct pci_dev *pci = to_pci_dev(sdev->dev);
> - struct snd_sof_pdata *plat_data = sdev->pdata;
> struct acp_dev_data *adata;
> const struct sof_amd_acp_desc *chip;
> const struct dmi_system_id *dmi_id;
> @@ -547,8 +546,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev)
> 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,
> - "%s/sof-%s-code.bin",
> - plat_data->fw_filename_prefix,
> + "sof-%s-code.bin",
> chip->name);
> if (!adata->fw_code_bin) {
> ret = -ENOMEM;
> @@ -556,8 +554,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev)
> }
>
> adata->fw_data_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
> - "%s/sof-%s-data.bin",
> - plat_data->fw_filename_prefix,
> + "sof-%s-data.bin",
> chip->name);
> if (!adata->fw_data_bin) {
> ret = -ENOMEM;
On 12/10/23 05:33, Venkata Prasad Potturu wrote:
>
> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>> 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 <[email protected]>
>> ---
>> 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 c6f637f29847..1e9840ae8938 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 */
> If any changes in SOF drivers, first need to create a PR in SOF github.
This is just an optimization for the driver, no need to change anything
on the firmware side. The product family remains as is, but it's not
really required to match the board, i.e. the previous board was
"Jupiter", the next one will have a different product name.
>> .matches = {
>> DMI_MATCH(DMI_SYS_VENDOR, "Valve"),
>> DMI_MATCH(DMI_PRODUCT_NAME, "Galileo"),
>> - DMI_MATCH(DMI_PRODUCT_FAMILY, "Sephiroth"),
>> },
>> .driver_data = (void *)SECURED_FIRMWARE,
>> },
On 12/10/23 05:50, Venkata Prasad Potturu wrote:
>
> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>> Commit 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if
>> firmware files are missing") changed the order of some operations and
>> the firmware paths are not available anymore at snd_sof_probe() time.
>>
>> Precisely, fw_filename_prefix is set by sof_select_ipc_and_paths() via
>>
>> plat_data->fw_filename_prefix = out_profile.fw_path;
>>
>> but sof_init_environment() which calls this function was moved from
>> snd_sof_device_probe() to sof_probe_continue(). Moreover,
>> snd_sof_probe() was moved from sof_probe_continue() to
>> sof_init_environment(), but before the call to
>> sof_select_ipc_and_paths().
>>
>> The problem here is that amd_sof_acp_probe() uses fw_filename_prefix to
>> compute fw_code_bin and fw_data_bin paths, and because the field is not
>> yet initialized, the paths end up containing (null):
>>
>> snd_sof_amd_vangogh 0000:04:00.5: Direct firmware load for
>> (null)/sof-vangogh-code.bin failed with error -2
>> snd_sof_amd_vangogh 0000:04:00.5: sof signed firmware code bin is missing
>> snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP firmware -2
>> snd_sof_amd_vangogh: probe of 0000:04:00.5 failed with error -2
>>
>> Move usage of fw_filename_prefix right before request_firmware() calls
>> in acp_sof_load_signed_firmware().
>>
>> Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback
>> if firmware files are missing")
>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>> ---
>> sound/soc/sof/amd/acp-loader.c | 32 ++++++++++++++++++++++++++------
>> sound/soc/sof/amd/acp.c | 7 ++-----
>> 2 files changed, 28 insertions(+), 11 deletions(-)
>>
>> diff --git a/sound/soc/sof/amd/acp-loader.c
>> b/sound/soc/sof/amd/acp-loader.c
>> index e05eb7a86dd4..d2d21478399e 100644
>> --- a/sound/soc/sof/amd/acp-loader.c
>> +++ b/sound/soc/sof/amd/acp-loader.c
>> @@ -267,29 +267,49 @@ int acp_sof_load_signed_firmware(struct
>> snd_sof_dev *sdev)
>> {
>> struct snd_sof_pdata *plat_data = sdev->pdata;
>> struct acp_dev_data *adata = plat_data->hw_pdata;
>> + const char *fw_filename;
>> int ret;
>> - ret = request_firmware(&sdev->basefw.fw, adata->fw_code_bin,
>> sdev->dev);
>> + fw_filename = kasprintf(GFP_KERNEL, "%s/%s",
>> + plat_data->fw_filename_prefix,
>> + adata->fw_code_bin);
> File path already saved in adata->fw_code_bin in amd_sof_acp_probe
> function.
> No need to get it again.
As already stated in the patch description, fw_filename_prefix is not
available anymore when amd_sof_acp_probe() gets invoked, and that is the
root cause of ending up with (null) in the computed file path.
Hence, the patch ensures amd_sof_acp_probe() computes the file name,
without prefix, while acp_sof_load_signed_firmware() adds the prefix.
>> + if (!fw_filename)
>> + return -ENOMEM;
>> +
>> + ret = request_firmware(&sdev->basefw.fw, fw_filename, sdev->dev);
>> if (ret < 0) {
>> + kfree(fw_filename);
>> dev_err(sdev->dev, "sof signed firmware code bin is
>> missing\n");
>> return ret;
>> } else {
>> - dev_dbg(sdev->dev, "request_firmware %s successful\n",
>> adata->fw_code_bin);
>> + dev_dbg(sdev->dev, "request_firmware %s successful\n",
>> fw_filename);
>> }
>> + kfree(fw_filename);
>> +
>> ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_IRAM, 0,
>> - (void *)sdev->basefw.fw->data,
>> sdev->basefw.fw->size);
>> + (void *)sdev->basefw.fw->data,
>> + sdev->basefw.fw->size);
>> +
>> + fw_filename = kasprintf(GFP_KERNEL, "%s/%s",
>> + plat_data->fw_filename_prefix,
>> + adata->fw_data_bin);
>> + if (!fw_filename)
>> + return -ENOMEM;
>> - ret = request_firmware(&adata->fw_dbin, adata->fw_data_bin,
>> sdev->dev);
>> + ret = request_firmware(&adata->fw_dbin, fw_filename, sdev->dev);
>> if (ret < 0) {
>> + kfree(fw_filename);
>> dev_err(sdev->dev, "sof signed firmware data bin is
>> missing\n");
>> return ret;
>> } else {
>> - dev_dbg(sdev->dev, "request_firmware %s successful\n",
>> adata->fw_data_bin);
>> + dev_dbg(sdev->dev, "request_firmware %s successful\n",
>> fw_filename);
>> }
>> + kfree(fw_filename);
>> ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_DRAM, 0,
>> - (void *)adata->fw_dbin->data,
>> adata->fw_dbin->size);
>> + (void *)adata->fw_dbin->data,
>> + adata->fw_dbin->size);
>> return ret;
>> }
>> EXPORT_SYMBOL_NS(acp_sof_load_signed_firmware, SND_SOC_SOF_AMD_COMMON);
>> diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c
>> index 1e9840ae8938..87c5c71eac68 100644
>> --- a/sound/soc/sof/amd/acp.c
>> +++ b/sound/soc/sof/amd/acp.c
>> @@ -479,7 +479,6 @@ EXPORT_SYMBOL_NS(amd_sof_acp_resume,
>> SND_SOC_SOF_AMD_COMMON);
>> int amd_sof_acp_probe(struct snd_sof_dev *sdev)
>> {
>> struct pci_dev *pci = to_pci_dev(sdev->dev);
>> - struct snd_sof_pdata *plat_data = sdev->pdata;
>> struct acp_dev_data *adata;
>> const struct sof_amd_acp_desc *chip;
>> const struct dmi_system_id *dmi_id;
>> @@ -547,8 +546,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev)
>> 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,
>> - "%s/sof-%s-code.bin",
>> - plat_data->fw_filename_prefix,
>> + "sof-%s-code.bin",
>> chip->name);
>> if (!adata->fw_code_bin) {
>> ret = -ENOMEM;
>> @@ -556,8 +554,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev)
>> }
>> adata->fw_data_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
>> - "%s/sof-%s-data.bin",
>> - plat_data->fw_filename_prefix,
>> + "sof-%s-data.bin",
>> chip->name);
>> if (!adata->fw_data_bin) {
>> ret = -ENOMEM;
On 12/10/23 05:24, Venkata Prasad Potturu wrote:
>
> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>> Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
>> creation for I2S BT instance") added I2S BT support in ACP common
>> machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2:
>>
>> [ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version
>> 0:0:0-7863d
>> [ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0
>> Kernel ABI 3:23:0
>> [ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0
>> Kernel ABI 3:23:0
>> [ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id
>> 2) not exist
>> [ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load
>> header: -22
>> [ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component
>> load failed -22
>> [ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load
>> DSP topology -22
>> [ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at
>> snd_soc_component_probe on 0000:04:00.5: -22
>> [ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22
>> [ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register
>> card(sof-nau8821-max)
>> [ 8.545022] sof_mach: probe of nau8821-max failed with error -22
>>
>> Move BT_BE_ID to the correct position in the enum.
>>
>> Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
>> creation for I2S BT instance")
>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>> ---
>> sound/soc/amd/acp/acp-mach.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h
>> index a48546d8d407..0c18ccd29305 100644
>> --- a/sound/soc/amd/acp/acp-mach.h
>> +++ b/sound/soc/amd/acp/acp-mach.h
>> @@ -27,8 +27,8 @@
>> enum be_id {
>> HEADSET_BE_ID = 0,
>> AMP_BE_ID,
>> - DMIC_BE_ID,
>> BT_BE_ID,
>> + DMIC_BE_ID,
> This will break the other platforms as this same enum used in topology
> to create dailink.
If I understand this correctly, there is no consistency across firmware
regarding the IDs used for DAI link identification. What would be the
suggested solution in this case?
Thanks,
Cristian
>> };
>> enum cpu_endpoints {
On 12/10/23 05:30, Venkata Prasad Potturu wrote:
>
> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>> Commit efb931cdc4b9 ("ASoC: SOF: topology: Add support for AMD ACP
>> DAIs") registered "ACP" name for SOF_DAI_AMD_BT DAI type. However, some
>> boards, i.e. Steam Deck OLED, seem to require "ACPBT" for the same type:
>>
>> [ 467.489680] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for
>> 0x30030000 (msg/reply size: 16/0): -22
>> [ 467.492775] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_route_setup:
>> route ACPBT2.IN -> BUF5.0 failed
>> [ 467.495839] snd_sof_amd_vangogh 0000:04:00.5:
>> sof_ipc3_set_up_all_pipelines: route set up failed
>> [ 467.499128] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component
>> load failed -22
>> [ 467.502210] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load
>> DSP topology -22
>> [ 467.505289] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at
>> snd_soc_component_probe on 0000:04:00.5: -22
>> [ 467.508430] sof_mach nau8821-max: ASoC: failed to instantiate card -22
>> [ 467.511725] sof_mach nau8821-max: error -EINVAL: Failed to register
>> card(sof-nau8821-max)
>> [ 467.514861] sof_mach: probe of nau8821-max failed with error -22
>>
>> Add "ACPBT" alias for "ACP" SOF DAI type.
>>
>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>> ---
>> sound/soc/sof/topology.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
>> index e3e7fbe40fa6..73bf791e7520 100644
>> --- a/sound/soc/sof/topology.c
>> +++ b/sound/soc/sof/topology.c
>> @@ -290,6 +290,7 @@ static const struct sof_dai_types sof_dais[] = {
>> {"SAI", SOF_DAI_IMX_SAI},
>> {"ESAI", SOF_DAI_IMX_ESAI},
>> {"ACP", SOF_DAI_AMD_BT},
>> + {"ACPBT", SOF_DAI_AMD_BT},
> No need to create the alias name, we can directly modify ACP to ACPBT as
> ACP is not using anywhere.
Great, thanks, will do in v2.
>> {"ACPSP", SOF_DAI_AMD_SP},
>> {"ACPDMIC", SOF_DAI_AMD_DMIC},
>> {"ACPHS", SOF_DAI_AMD_HS},
On 12/10/23 14:38, Cristian Ciocaltea wrote:
> On 12/10/23 05:30, Venkata Prasad Potturu wrote:
>> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>>> Commit efb931cdc4b9 ("ASoC: SOF: topology: Add support for AMD ACP
>>> DAIs") registered "ACP" name for SOF_DAI_AMD_BT DAI type. However, some
>>> boards, i.e. Steam Deck OLED, seem to require "ACPBT" for the same type:
>>>
>>> [ 467.489680] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for
>>> 0x30030000 (msg/reply size: 16/0): -22
>>> [ 467.492775] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_route_setup:
>>> route ACPBT2.IN -> BUF5.0 failed
>>> [ 467.495839] snd_sof_amd_vangogh 0000:04:00.5:
>>> sof_ipc3_set_up_all_pipelines: route set up failed
>>> [ 467.499128] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component
>>> load failed -22
>>> [ 467.502210] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load
>>> DSP topology -22
>>> [ 467.505289] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at
>>> snd_soc_component_probe on 0000:04:00.5: -22
>>> [ 467.508430] sof_mach nau8821-max: ASoC: failed to instantiate card -22
>>> [ 467.511725] sof_mach nau8821-max: error -EINVAL: Failed to register
>>> card(sof-nau8821-max)
>>> [ 467.514861] sof_mach: probe of nau8821-max failed with error -22
>>>
>>> Add "ACPBT" alias for "ACP" SOF DAI type.
>>>
>>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>>> ---
>>> sound/soc/sof/topology.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
>>> index e3e7fbe40fa6..73bf791e7520 100644
>>> --- a/sound/soc/sof/topology.c
>>> +++ b/sound/soc/sof/topology.c
>>> @@ -290,6 +290,7 @@ static const struct sof_dai_types sof_dais[] = {
>>> {"SAI", SOF_DAI_IMX_SAI},
>>> {"ESAI", SOF_DAI_IMX_ESAI},
>>> {"ACP", SOF_DAI_AMD_BT},
>>> + {"ACPBT", SOF_DAI_AMD_BT},
>> No need to create the alias name, we can directly modify ACP to ACPBT as
>> ACP is not using anywhere.
> Great, thanks, will do in v2.
This should send to SOF git repo for rewiew, once SOF reviewers approved
this, again need to send to broonie git.
All the changes in sound/soc/sof/ path should go to SOF git.
>
>>> {"ACPSP", SOF_DAI_AMD_SP},
>>> {"ACPDMIC", SOF_DAI_AMD_DMIC},
>>> {"ACPHS", SOF_DAI_AMD_HS},
On 12/10/23 14:36, Cristian Ciocaltea wrote:
> On 12/10/23 05:24, Venkata Prasad Potturu wrote:
>> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>>> Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
>>> creation for I2S BT instance") added I2S BT support in ACP common
>>> machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2:
>>>
>>> [ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version
>>> 0:0:0-7863d
>>> [ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0
>>> Kernel ABI 3:23:0
>>> [ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0
>>> Kernel ABI 3:23:0
>>> [ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id
>>> 2) not exist
>>> [ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load
>>> header: -22
>>> [ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component
>>> load failed -22
>>> [ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load
>>> DSP topology -22
>>> [ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at
>>> snd_soc_component_probe on 0000:04:00.5: -22
>>> [ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22
>>> [ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register
>>> card(sof-nau8821-max)
>>> [ 8.545022] sof_mach: probe of nau8821-max failed with error -22
>>>
>>> Move BT_BE_ID to the correct position in the enum.
>>>
>>> Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
>>> creation for I2S BT instance")
>>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>>> ---
>>> sound/soc/amd/acp/acp-mach.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h
>>> index a48546d8d407..0c18ccd29305 100644
>>> --- a/sound/soc/amd/acp/acp-mach.h
>>> +++ b/sound/soc/amd/acp/acp-mach.h
>>> @@ -27,8 +27,8 @@
>>> enum be_id {
>>> HEADSET_BE_ID = 0,
>>> AMP_BE_ID,
>>> - DMIC_BE_ID,
>>> BT_BE_ID,
>>> + DMIC_BE_ID,
>> This will break the other platforms as this same enum used in topology
>> to create dailink.
> If I understand this correctly, there is no consistency across firmware
> regarding the IDs used for DAI link identification. What would be the
> suggested solution in this case?
These id values should be same in machine driver and topology file, then
only dailink can create without an error.
Always new be_id should add at the end only.
In this case BT_BE_ID should be at the end.
enum be_id {
HEADSET_BE_ID = 0,
AMP_BE_ID,
DMIC_BE_ID,
BT_BE_ID,
}
>
> Thanks,
> Cristian
>
>>> };
>>> enum cpu_endpoints {
On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>
> On 12/10/23 14:38, Cristian Ciocaltea wrote:
>> On 12/10/23 05:30, Venkata Prasad Potturu wrote:
>>> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>>>> Commit efb931cdc4b9 ("ASoC: SOF: topology: Add support for AMD ACP
>>>> DAIs") registered "ACP" name for SOF_DAI_AMD_BT DAI type. However,
>>>> some
>>>> boards, i.e. Steam Deck OLED, seem to require "ACPBT" for the same
>>>> type:
>>>>
>>>> [ 467.489680] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for
>>>> 0x30030000 (msg/reply size: 16/0): -22
>>>> [ 467.492775] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_route_setup:
>>>> route ACPBT2.IN -> BUF5.0 failed
>>>> [ 467.495839] snd_sof_amd_vangogh 0000:04:00.5:
>>>> sof_ipc3_set_up_all_pipelines: route set up failed
>>>> [ 467.499128] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component
>>>> load failed -22
>>>> [ 467.502210] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load
>>>> DSP topology -22
>>>> [ 467.505289] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at
>>>> snd_soc_component_probe on 0000:04:00.5: -22
>>>> [ 467.508430] sof_mach nau8821-max: ASoC: failed to instantiate
>>>> card -22
>>>> [ 467.511725] sof_mach nau8821-max: error -EINVAL: Failed to register
>>>> card(sof-nau8821-max)
>>>> [ 467.514861] sof_mach: probe of nau8821-max failed with error -22
>>>>
>>>> Add "ACPBT" alias for "ACP" SOF DAI type.
>>>>
>>>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>>>> ---
>>>> sound/soc/sof/topology.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
>>>> index e3e7fbe40fa6..73bf791e7520 100644
>>>> --- a/sound/soc/sof/topology.c
>>>> +++ b/sound/soc/sof/topology.c
>>>> @@ -290,6 +290,7 @@ static const struct sof_dai_types sof_dais[] = {
>>>> {"SAI", SOF_DAI_IMX_SAI},
>>>> {"ESAI", SOF_DAI_IMX_ESAI},
>>>> {"ACP", SOF_DAI_AMD_BT},
>>>> + {"ACPBT", SOF_DAI_AMD_BT},
>>> No need to create the alias name, we can directly modify ACP to ACPBT as
>>> ACP is not using anywhere.
>> Great, thanks, will do in v2.
> This should send to SOF git repo for rewiew, once SOF reviewers approved
> this, again need to send to broonie git.
> All the changes in sound/soc/sof/ path should go to SOF git.
Unfortunately I'm not familiar with the SOF dev workflow. So it's not
enough to have this patch cc-ed to [email protected]?
>>>> {"ACPSP", SOF_DAI_AMD_SP},
>>>> {"ACPDMIC", SOF_DAI_AMD_DMIC},
>>>> {"ACPHS", SOF_DAI_AMD_HS},
On 12/10/23 12:05, Venkata Prasad Potturu wrote:
>
> On 12/10/23 14:36, Cristian Ciocaltea wrote:
>> On 12/10/23 05:24, Venkata Prasad Potturu wrote:
>>> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>>>> Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
>>>> creation for I2S BT instance") added I2S BT support in ACP common
>>>> machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2:
>>>>
>>>> [ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version
>>>> 0:0:0-7863d
>>>> [ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0
>>>> Kernel ABI 3:23:0
>>>> [ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0
>>>> Kernel ABI 3:23:0
>>>> [ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id
>>>> 2) not exist
>>>> [ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load
>>>> header: -22
>>>> [ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component
>>>> load failed -22
>>>> [ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load
>>>> DSP topology -22
>>>> [ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at
>>>> snd_soc_component_probe on 0000:04:00.5: -22
>>>> [ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22
>>>> [ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register
>>>> card(sof-nau8821-max)
>>>> [ 8.545022] sof_mach: probe of nau8821-max failed with error -22
>>>>
>>>> Move BT_BE_ID to the correct position in the enum.
>>>>
>>>> Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
>>>> creation for I2S BT instance")
>>>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>>>> ---
>>>> sound/soc/amd/acp/acp-mach.h | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/sound/soc/amd/acp/acp-mach.h
>>>> b/sound/soc/amd/acp/acp-mach.h
>>>> index a48546d8d407..0c18ccd29305 100644
>>>> --- a/sound/soc/amd/acp/acp-mach.h
>>>> +++ b/sound/soc/amd/acp/acp-mach.h
>>>> @@ -27,8 +27,8 @@
>>>> enum be_id {
>>>> HEADSET_BE_ID = 0,
>>>> AMP_BE_ID,
>>>> - DMIC_BE_ID,
>>>> BT_BE_ID,
>>>> + DMIC_BE_ID,
>>> This will break the other platforms as this same enum used in topology
>>> to create dailink.
>> If I understand this correctly, there is no consistency across firmware
>> regarding the IDs used for DAI link identification. What would be the
>> suggested solution in this case?
>
> These id values should be same in machine driver and topology file, then
> only dailink can create without an error.
Yes, my point was that some topology files seem to require different IDs
for the same DAI link types. In this case the topology expects ID 2 for
BT, but other topologies would interpret that as DMIC.
> Always new be_id should add at the end only.
>
> In this case BT_BE_ID should be at the end.
>
> enum be_id {
> HEADSET_BE_ID = 0,
> AMP_BE_ID,
> DMIC_BE_ID,
> BT_BE_ID,
> }
So you are basically stating the firmware is broken and needs an update
to use ID 3 for BT, and there is nothing we can do about it on driver's
side. Is that correct?
>
>
>>
>> Thanks,
>> Cristian
>>
>>>> };
>>>> enum cpu_endpoints {
On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
> > This should send to SOF git repo for rewiew, once SOF reviewers approved
> > this, again need to send to broonie git.
> > All the changes in sound/soc/sof/ path should go to SOF git.
> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
> enough to have this patch cc-ed to [email protected]?
The SOF people basically do their own thing in github at
https://github.com/thesofproject/linux
with a github workflow and submit their patches upstream in batches a
few times a release, however my understanding is that their workflow can
cope with things going in directly upstream as well.
On 12/10/23 16:01, Mark Brown wrote:
> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>
>>> This should send to SOF git repo for rewiew, once SOF reviewers approved
>>> this, again need to send to broonie git.
>>> All the changes in sound/soc/sof/ path should go to SOF git.
>
>> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
>> enough to have this patch cc-ed to [email protected]?
>
> The SOF people basically do their own thing in github at
>
> https://github.com/thesofproject/linux
>
> with a github workflow and submit their patches upstream in batches a
> few times a release, however my understanding is that their workflow can
> cope with things going in directly upstream as well.
Thanks for clarifying, Mark! That would greatly simplify and speedup
the whole process, at least for trivial patches like this one.
On 12/10/23 16:02, Cristian Ciocaltea wrote:
> On 12/10/23 12:05, Venkata Prasad Potturu wrote:
>> On 12/10/23 14:36, Cristian Ciocaltea wrote:
>>> On 12/10/23 05:24, Venkata Prasad Potturu wrote:
>>>> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>>>>> Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
>>>>> creation for I2S BT instance") added I2S BT support in ACP common
>>>>> machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2:
>>>>>
>>>>> [ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version
>>>>> 0:0:0-7863d
>>>>> [ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0
>>>>> Kernel ABI 3:23:0
>>>>> [ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0
>>>>> Kernel ABI 3:23:0
>>>>> [ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id
>>>>> 2) not exist
>>>>> [ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load
>>>>> header: -22
>>>>> [ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component
>>>>> load failed -22
>>>>> [ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load
>>>>> DSP topology -22
>>>>> [ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at
>>>>> snd_soc_component_probe on 0000:04:00.5: -22
>>>>> [ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22
>>>>> [ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register
>>>>> card(sof-nau8821-max)
>>>>> [ 8.545022] sof_mach: probe of nau8821-max failed with error -22
>>>>>
>>>>> Move BT_BE_ID to the correct position in the enum.
>>>>>
>>>>> Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
>>>>> creation for I2S BT instance")
>>>>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>>>>> ---
>>>>> sound/soc/amd/acp/acp-mach.h | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/sound/soc/amd/acp/acp-mach.h
>>>>> b/sound/soc/amd/acp/acp-mach.h
>>>>> index a48546d8d407..0c18ccd29305 100644
>>>>> --- a/sound/soc/amd/acp/acp-mach.h
>>>>> +++ b/sound/soc/amd/acp/acp-mach.h
>>>>> @@ -27,8 +27,8 @@
>>>>> enum be_id {
>>>>> HEADSET_BE_ID = 0,
>>>>> AMP_BE_ID,
>>>>> - DMIC_BE_ID,
>>>>> BT_BE_ID,
>>>>> + DMIC_BE_ID,
>>>> This will break the other platforms as this same enum used in topology
>>>> to create dailink.
>>> If I understand this correctly, there is no consistency across firmware
>>> regarding the IDs used for DAI link identification. What would be the
>>> suggested solution in this case?
>> These id values should be same in machine driver and topology file, then
>> only dailink can create without an error.
> Yes, my point was that some topology files seem to require different IDs
> for the same DAI link types. In this case the topology expects ID 2 for
> BT, but other topologies would interpret that as DMIC.
>
>> Always new be_id should add at the end only.
>>
>> In this case BT_BE_ID should be at the end.
>>
>> enum be_id {
>> HEADSET_BE_ID = 0,
>> AMP_BE_ID,
>> DMIC_BE_ID,
>> BT_BE_ID,
>> }
> So you are basically stating the firmware is broken and needs an update
> to use ID 3 for BT, and there is nothing we can do about it on driver's
> side. Is that correct?
Yes, id 3 should be used for BT_BE_ID in topology file.
>
>>
>>> Thanks,
>>> Cristian
>>>
>>>>> };
>>>>> enum cpu_endpoints {
On 12/10/23 21:20, Cristian Ciocaltea wrote:
> On 12/10/23 16:01, Mark Brown wrote:
>> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>>>> This should send to SOF git repo for rewiew, once SOF reviewers approved
>>>> this, again need to send to broonie git.
>>>> All the changes in sound/soc/sof/ path should go to SOF git.
>>> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
>>> enough to have this patch cc-ed to [email protected]?
>> The SOF people basically do their own thing in github at
>>
>> https://github.com/thesofproject/linux
>>
>> with a github workflow and submit their patches upstream in batches a
>> few times a release, however my understanding is that their workflow can
>> cope with things going in directly upstream as well.
> Thanks for clarifying, Mark! That would greatly simplify and speedup
> the whole process, at least for trivial patches like this one.
Hi Cristian,
We have created a Pull request in SOF git hub for I2S BT support.
please hold v2 version SOF patches till below PR get's merged.
PR:- https://github.com/thesofproject/linux/pull/4742
On 10/12/23 19:31, Mark Brown wrote:
> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>>> This should send to SOF git repo for rewiew, once SOF reviewers approved
>>> this, again need to send to broonie git.
>>> All the changes in sound/soc/sof/ path should go to SOF git.
>> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
>> enough to have this patch cc-ed to [email protected]?
> The SOF people basically do their own thing in github at
>
> https://github.com/thesofproject/linux
>
> with a github workflow and submit their patches upstream in batches a
> few times a release, however my understanding is that their workflow can
> cope with things going in directly upstream as well.
If patches are directly pushed to alsa devel list instead of creating pull request for SOF patches
It will break SOF github work flow.
Validation across all the platforms is a potential challenge, and it will also create an overhead
to pull the patches which got merged in to for-next branch, before the all the patches pulled in to SOF
GitHub.
On 09/12/2023 22:53, Cristian Ciocaltea wrote:
> Some SOF drivers like AMD ACP do not always rely on a single static
> firmware file, but may require multiple files having their names
> dynamically computed on probe time, e.g. based on chip name.
I see, AMD vangogh needs two binary files to be loaded sometimes, it is not using the default name as it hints via the sof_dev_desc ("sof-vangogh.ri").
The constructed names for the two files are just using different pattern:
sof-%PLAT%.ri
vs
sof-%PLAT%-code.bin
sof-%PLAT%-data.bin
iow, instead of the combined .ri file which includes the code and data segment it has 'raw' bin files and cannot use the core for loading the firmware.
What is the reason for this? an .ri file can have two 'modules' one to be written to IRAM the other to DRAM.
sof_ipc3_load_fw_to_dsp()
> In those cases, providing an invalid default_fw_filename in their
> sof_dev_desc struct will prevent probing due to 'SOF firmware
> and/or topology file not found' error.
> Fix the issue by allowing drivers to omit initialization for this member
> (or alternatively provide a dynamic override via ipc_file_profile_base)
> and update sof_test_firmware_file() to verify the given profile data and
> skip firmware testing if either fw_path or fw_name is not defined.
>
> Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing")
> Signed-off-by: Cristian Ciocaltea <[email protected]>
> ---
> sound/soc/sof/fw-file-profile.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
> index 138a1ca2c4a8..e63700234df0 100644
> --- a/sound/soc/sof/fw-file-profile.c
> +++ b/sound/soc/sof/fw-file-profile.c
> @@ -21,6 +21,9 @@ static int sof_test_firmware_file(struct device *dev,
> const u32 *magic;
> int ret;
>
> + if (!profile->fw_path || !profile->fw_name || !*profile->fw_name)
> + return 0;
I would rather make the firmware file skipping based on custom loader use and print a dev_dbg() alongside:
diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
index 138a1ca2c4a8..7b91c9551ada 100644
--- a/sound/soc/sof/fw-file-profile.c
+++ b/sound/soc/sof/fw-file-profile.c
@@ -89,6 +89,15 @@ static int sof_test_topology_file(struct device *dev,
return ret;
}
+static bool sof_platform_uses_generic_loader(struct snd_sof_dev *sdev)
+{
+ if (sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_raw ||
+ sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_memcpy)
+ return true;
+
+ return false;
+}
+
static int
sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
enum sof_ipc_type ipc_type,
@@ -130,7 +139,8 @@ sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
* For default path and firmware name do a verification before
* continuing further.
*/
- if (base_profile->fw_path || base_profile->fw_name) {
+ if ((base_profile->fw_path || base_profile->fw_name) &&
+ sof_platform_uses_generic_loader(sdev)) {
ret = sof_test_firmware_file(dev, out_profile, &ipc_type);
if (ret)
return ret;
@@ -181,7 +191,8 @@ sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
out_profile->ipc_type = ipc_type;
/* Test only default firmware file */
- if (!base_profile->fw_path && !base_profile->fw_name)
+ if ((!base_profile->fw_path && !base_profile->fw_name) &&
+ sof_platform_uses_generic_loader(sdev))
ret = sof_test_firmware_file(dev, out_profile, NULL);
if (!ret)
@@ -265,7 +276,9 @@ static void sof_print_profile_info(struct snd_sof_dev *sdev,
"Using fallback IPC type %d (requested type was %d)\n",
profile->ipc_type, ipc_type);
- dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
+ /* The firmware path only valid when generic loader is used */
+ if (sof_platform_uses_generic_loader(sdev))
+ dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
dev_info(dev, " Firmware file: %s/%s\n", profile->fw_path, profile->fw_name);
if (profile->fw_lib_path)
> +
> fw_filename = kasprintf(GFP_KERNEL, "%s/%s", profile->fw_path,
> profile->fw_name);
> if (!fw_filename)
checking for custom loader allows you to drop the next patch.
I would also skip the fw path printing in case of a custom loader.
--
Péter
On 12/14/23 16:18, Péter Ujfalusi wrote:
Thanks for your time Peter!
>
> On 09/12/2023 22:53, Cristian Ciocaltea wrote:
>> Some SOF drivers like AMD ACP do not always rely on a single static
>> firmware file, but may require multiple files having their names
>> dynamically computed on probe time, e.g. based on chip name.
> I see, AMD vangogh needs two binary files to be loaded sometimes, it is not using the default name as it hints via the sof_dev_desc ("sof-vangogh.ri").
>
> The constructed names for the two files are just using different pattern:
> sof-%PLAT%.ri
> vs
> sof-%PLAT%-code.bin
> sof-%PLAT%-data.bin
>
> iow, instead of the combined .ri file which includes the code and data segment it has 'raw' bin files and cannot use the core for loading the firmware.
>
> What is the reason for this? an .ri file can have two 'modules' one to be written to IRAM the other to DRAM.
> sof_ipc3_load_fw_to_dsp()
For AMD Vangogh platform devices signed firmware image is required, so
split .ri image into code and data images.
Only Code.bin will be signed and loaded into corresponding IRAM location.
>
>> In those cases, providing an invalid default_fw_filename in their
>> sof_dev_desc struct will prevent probing due to 'SOF firmware
>> and/or topology file not found' error.
>
>> Fix the issue by allowing drivers to omit initialization for this member
>> (or alternatively provide a dynamic override via ipc_file_profile_base)
>> and update sof_test_firmware_file() to verify the given profile data and
>> skip firmware testing if either fw_path or fw_name is not defined.
>>
>> Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing")
>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>> ---
>> sound/soc/sof/fw-file-profile.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
>> index 138a1ca2c4a8..e63700234df0 100644
>> --- a/sound/soc/sof/fw-file-profile.c
>> +++ b/sound/soc/sof/fw-file-profile.c
>> @@ -21,6 +21,9 @@ static int sof_test_firmware_file(struct device *dev,
>> const u32 *magic;
>> int ret;
>>
>> + if (!profile->fw_path || !profile->fw_name || !*profile->fw_name)
>> + return 0;
> I would rather make the firmware file skipping based on custom loader use and print a dev_dbg() alongside:
>
> diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
> index 138a1ca2c4a8..7b91c9551ada 100644
> --- a/sound/soc/sof/fw-file-profile.c
> +++ b/sound/soc/sof/fw-file-profile.c
> @@ -89,6 +89,15 @@ static int sof_test_topology_file(struct device *dev,
> return ret;
> }
>
> +static bool sof_platform_uses_generic_loader(struct snd_sof_dev *sdev)
> +{
> + if (sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_raw ||
> + sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_memcpy)
> + return true;
> +
> + return false;
> +}
> +
> static int
> sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
> enum sof_ipc_type ipc_type,
> @@ -130,7 +139,8 @@ sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
> * For default path and firmware name do a verification before
> * continuing further.
> */
> - if (base_profile->fw_path || base_profile->fw_name) {
> + if ((base_profile->fw_path || base_profile->fw_name) &&
> + sof_platform_uses_generic_loader(sdev)) {
> ret = sof_test_firmware_file(dev, out_profile, &ipc_type);
> if (ret)
> return ret;
> @@ -181,7 +191,8 @@ sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
> out_profile->ipc_type = ipc_type;
>
> /* Test only default firmware file */
> - if (!base_profile->fw_path && !base_profile->fw_name)
> + if ((!base_profile->fw_path && !base_profile->fw_name) &&
> + sof_platform_uses_generic_loader(sdev))
> ret = sof_test_firmware_file(dev, out_profile, NULL);
>
> if (!ret)
> @@ -265,7 +276,9 @@ static void sof_print_profile_info(struct snd_sof_dev *sdev,
> "Using fallback IPC type %d (requested type was %d)\n",
> profile->ipc_type, ipc_type);
>
> - dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
> + /* The firmware path only valid when generic loader is used */
> + if (sof_platform_uses_generic_loader(sdev))
> + dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
>
> dev_info(dev, " Firmware file: %s/%s\n", profile->fw_path, profile->fw_name);
> if (profile->fw_lib_path)
>
>> +
>> fw_filename = kasprintf(GFP_KERNEL, "%s/%s", profile->fw_path,
>> profile->fw_name);
>> if (!fw_filename)
> checking for custom loader allows you to drop the next patch.
> I would also skip the fw path printing in case of a custom loader.
>
On 12/14/23 12:48, Péter Ujfalusi wrote:
>
>
> On 09/12/2023 22:53, Cristian Ciocaltea wrote:
>> Some SOF drivers like AMD ACP do not always rely on a single static
>> firmware file, but may require multiple files having their names
>> dynamically computed on probe time, e.g. based on chip name.
>
> I see, AMD vangogh needs two binary files to be loaded sometimes, it is not using the default name as it hints via the sof_dev_desc ("sof-vangogh.ri").
>
> The constructed names for the two files are just using different pattern:
> sof-%PLAT%.ri
> vs
> sof-%PLAT%-code.bin
> sof-%PLAT%-data.bin
>
> iow, instead of the combined .ri file which includes the code and data segment it has 'raw' bin files and cannot use the core for loading the firmware.
>
> What is the reason for this? an .ri file can have two 'modules' one to be written to IRAM the other to DRAM.
> sof_ipc3_load_fw_to_dsp()
>
>> In those cases, providing an invalid default_fw_filename in their
>> sof_dev_desc struct will prevent probing due to 'SOF firmware
>> and/or topology file not found' error.
>
>> Fix the issue by allowing drivers to omit initialization for this member
>> (or alternatively provide a dynamic override via ipc_file_profile_base)
>> and update sof_test_firmware_file() to verify the given profile data and
>> skip firmware testing if either fw_path or fw_name is not defined.
>>
>> Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing")
>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>> ---
>> sound/soc/sof/fw-file-profile.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
>> index 138a1ca2c4a8..e63700234df0 100644
>> --- a/sound/soc/sof/fw-file-profile.c
>> +++ b/sound/soc/sof/fw-file-profile.c
>> @@ -21,6 +21,9 @@ static int sof_test_firmware_file(struct device *dev,
>> const u32 *magic;
>> int ret;
>>
>> + if (!profile->fw_path || !profile->fw_name || !*profile->fw_name)
>> + return 0;
>
> I would rather make the firmware file skipping based on custom loader use and print a dev_dbg() alongside:
Agree, that's a better approach. Thanks for the hint!
> diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
> index 138a1ca2c4a8..7b91c9551ada 100644
> --- a/sound/soc/sof/fw-file-profile.c
> +++ b/sound/soc/sof/fw-file-profile.c
> @@ -89,6 +89,15 @@ static int sof_test_topology_file(struct device *dev,
> return ret;
> }
>
> +static bool sof_platform_uses_generic_loader(struct snd_sof_dev *sdev)
> +{
> + if (sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_raw ||
> + sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_memcpy)
> + return true;
> +
> + return false;
> +}
I would drop the conditional and simply return.
> static int
> sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
> enum sof_ipc_type ipc_type,
> @@ -130,7 +139,8 @@ sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
> * For default path and firmware name do a verification before
> * continuing further.
> */
> - if (base_profile->fw_path || base_profile->fw_name) {
> + if ((base_profile->fw_path || base_profile->fw_name) &&
> + sof_platform_uses_generic_loader(sdev)) {
> ret = sof_test_firmware_file(dev, out_profile, &ipc_type);
> if (ret)
> return ret;
> @@ -181,7 +191,8 @@ sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
> out_profile->ipc_type = ipc_type;
>
> /* Test only default firmware file */
> - if (!base_profile->fw_path && !base_profile->fw_name)
> + if ((!base_profile->fw_path && !base_profile->fw_name) &&
> + sof_platform_uses_generic_loader(sdev))
> ret = sof_test_firmware_file(dev, out_profile, NULL);
>
> if (!ret)
> @@ -265,7 +276,9 @@ static void sof_print_profile_info(struct snd_sof_dev *sdev,
> "Using fallback IPC type %d (requested type was %d)\n",
> profile->ipc_type, ipc_type);
>
> - dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
> + /* The firmware path only valid when generic loader is used */
> + if (sof_platform_uses_generic_loader(sdev))
> + dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
>
> dev_info(dev, " Firmware file: %s/%s\n", profile->fw_path, profile->fw_name);
> if (profile->fw_lib_path)
>
>> +
>> fw_filename = kasprintf(GFP_KERNEL, "%s/%s", profile->fw_path,
>> profile->fw_name);
>> if (!fw_filename)
>
> checking for custom loader allows you to drop the next patch.
Yes, I will take only the context information and use it to update the
current patch description.
> I would also skip the fw path printing in case of a custom loader.
Sure, makes sense.
Thanks for the review,
Cristian
On 14/12/2023 13:29, Cristian Ciocaltea wrote:
>> diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
>> index 138a1ca2c4a8..7b91c9551ada 100644
>> --- a/sound/soc/sof/fw-file-profile.c
>> +++ b/sound/soc/sof/fw-file-profile.c
>> @@ -89,6 +89,15 @@ static int sof_test_topology_file(struct device *dev,
>> return ret;
>> }
>>
>> +static bool sof_platform_uses_generic_loader(struct snd_sof_dev *sdev)
>> +{
>> + if (sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_raw ||
>> + sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_memcpy)
>> + return true;
>> +
>> + return false;
>> +}
>
> I would drop the conditional and simply return.
What do you mean? We need to check if the platform is using either type
of the generic load_firmware helper (the _memcpy is calling the _raw to
load the file).
--
Péter
On 12/14/23 13:35, Péter Ujfalusi wrote:
>
>
> On 14/12/2023 13:29, Cristian Ciocaltea wrote:
>>> diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
>>> index 138a1ca2c4a8..7b91c9551ada 100644
>>> --- a/sound/soc/sof/fw-file-profile.c
>>> +++ b/sound/soc/sof/fw-file-profile.c
>>> @@ -89,6 +89,15 @@ static int sof_test_topology_file(struct device *dev,
>>> return ret;
>>> }
>>>
>>> +static bool sof_platform_uses_generic_loader(struct snd_sof_dev *sdev)
>>> +{
>>> + if (sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_raw ||
>>> + sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_memcpy)
>>> + return true;
>>> +
>>> + return false;
>>> +}
>>
>> I would drop the conditional and simply return.
>
> What do you mean? We need to check if the platform is using either type
> of the generic load_firmware helper (the _memcpy is calling the _raw to
> load the file).
I mean to simply replace the if statement with:
static bool sof_platform_uses_generic_loader(struct snd_sof_dev *sdev)
{
return (sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_raw ||
sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_memcpy);
}
On 14/12/2023 12:48, Péter Ujfalusi wrote:
> @@ -265,7 +276,9 @@ static void sof_print_profile_info(struct snd_sof_dev *sdev,
> "Using fallback IPC type %d (requested type was %d)\n",
> profile->ipc_type, ipc_type);
>
> - dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
> + /* The firmware path only valid when generic loader is used */
> + if (sof_platform_uses_generic_loader(sdev))
> + dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
>
This is the correct section in here, sorry:
- dev_info(dev, " Firmware file: %s/%s\n", profile->fw_path, profile->fw_name);
+ /* The firmware path only valid when generic loader is used */
+ if (sof_platform_uses_generic_loader(sdev))
+ dev_info(dev, " Firmware file: %s/%s\n", profile->fw_path, profile->fw_name);
+
if (profile->fw_lib_path)
--
Péter
On 14/12/2023 13:40, Cristian Ciocaltea wrote:
> On 12/14/23 13:35, Péter Ujfalusi wrote:
>>
>>
>> On 14/12/2023 13:29, Cristian Ciocaltea wrote:
>>>> diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
>>>> index 138a1ca2c4a8..7b91c9551ada 100644
>>>> --- a/sound/soc/sof/fw-file-profile.c
>>>> +++ b/sound/soc/sof/fw-file-profile.c
>>>> @@ -89,6 +89,15 @@ static int sof_test_topology_file(struct device *dev,
>>>> return ret;
>>>> }
>>>>
>>>> +static bool sof_platform_uses_generic_loader(struct snd_sof_dev *sdev)
>>>> +{
>>>> + if (sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_raw ||
>>>> + sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_memcpy)
>>>> + return true;
>>>> +
>>>> + return false;
>>>> +}
>>>
>>> I would drop the conditional and simply return.
>>
>> What do you mean? We need to check if the platform is using either type
>> of the generic load_firmware helper (the _memcpy is calling the _raw to
>> load the file).
>
> I mean to simply replace the if statement with:
>
> static bool sof_platform_uses_generic_loader(struct snd_sof_dev *sdev)
> {
> return (sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_raw ||
> sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_memcpy);
> }
ah, OK.
--
Péter
On 14/12/2023 12:58, Venkata Prasad Potturu wrote:
>
> On 12/14/23 16:18, Péter Ujfalusi wrote:
> Thanks for your time Peter!
>>
>> On 09/12/2023 22:53, Cristian Ciocaltea wrote:
>>> Some SOF drivers like AMD ACP do not always rely on a single static
>>> firmware file, but may require multiple files having their names
>>> dynamically computed on probe time, e.g. based on chip name.
>> I see, AMD vangogh needs two binary files to be loaded sometimes, it
>> is not using the default name as it hints via the sof_dev_desc
>> ("sof-vangogh.ri").
>>
>> The constructed names for the two files are just using different pattern:
>> sof-%PLAT%.ri
>> vs
>> sof-%PLAT%-code.bin
>> sof-%PLAT%-data.bin
>>
>> iow, instead of the combined .ri file which includes the code and data
>> segment it has 'raw' bin files and cannot use the core for loading the
>> firmware.
>>
>> What is the reason for this? an .ri file can have two 'modules' one to
>> be written to IRAM the other to DRAM.
>> sof_ipc3_load_fw_to_dsp()
>
> For AMD Vangogh platform devices signed firmware image is required, so
> split .ri image into code and data images.
>
> Only Code.bin will be signed and loaded into corresponding IRAM location.
This is not different than what the Intel .ri files are made of. The
module which is to be loaded to IRAM is signed code the module which
goes to DRAM is not signed.
The loader itself is not looking into the sections of the .ri image, it
just parses the header and copies them where they belong.
if the issue is name collision then you could try to put the signed
firmware file under 'signed' folder (fw_path_postfix) of the platform
like Intel does with the community signed ones?
It would be great if somehow we can handle all of these in core, have
shared code and familiar prints among vendors, platforms..
Fwiw, I'm planning the path, filename creation to be moved to core for
the current platforms, but it implies that they do use single firmware file.
struct sof_dev_desc would only have two strings:
vendor - AMD / iMX / Intel / Mediatek
platform - tgl, vaggogh, etc
I need to adjust it based on what I have learned today about vangogh.
--
Péter
On 12/14/23 13:51, Péter Ujfalusi wrote:
>
>
> On 14/12/2023 12:48, Péter Ujfalusi wrote:
>> @@ -265,7 +276,9 @@ static void sof_print_profile_info(struct snd_sof_dev *sdev,
>> "Using fallback IPC type %d (requested type was %d)\n",
>> profile->ipc_type, ipc_type);
>>
>> - dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
>> + /* The firmware path only valid when generic loader is used */
>> + if (sof_platform_uses_generic_loader(sdev))
>> + dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
>>
>
> This is the correct section in here, sorry:
>
> - dev_info(dev, " Firmware file: %s/%s\n", profile->fw_path, profile->fw_name);
> + /* The firmware path only valid when generic loader is used */
> + if (sof_platform_uses_generic_loader(sdev))
> + dev_info(dev, " Firmware file: %s/%s\n", profile->fw_path, profile->fw_name);
> +
> if (profile->fw_lib_path)
No problem, thanks for the update!
On 12/11/23 07:58, Venkata Prasad Potturu wrote:
>
> On 12/10/23 21:20, Cristian Ciocaltea wrote:
>> On 12/10/23 16:01, Mark Brown wrote:
>>> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>>>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>>>>> This should send to SOF git repo for rewiew, once SOF reviewers
>>>>> approved
>>>>> this, again need to send to broonie git.
>>>>> All the changes in sound/soc/sof/ path should go to SOF git.
>>>> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
>>>> enough to have this patch cc-ed to
>>>> [email protected]?
>>> The SOF people basically do their own thing in github at
>>>
>>> https://github.com/thesofproject/linux
>>>
>>> with a github workflow and submit their patches upstream in batches a
>>> few times a release, however my understanding is that their workflow can
>>> cope with things going in directly upstream as well.
>> Thanks for clarifying, Mark! That would greatly simplify and speedup
>> the whole process, at least for trivial patches like this one.
>
> Hi Cristian,
>
> We have created a Pull request in SOF git hub for I2S BT support.
> please hold v2 version SOF patches till below PR get's merged.
> PR:- https://github.com/thesofproject/linux/pull/4742
Hi Venkata,
If this is going to be handled via the github workflow, this patch
should be removed from the series. Since there is no dependency on it,
I cannot see a reason to put v2 on hold.
Do I miss something?
Thanks,
Cristian
On 12/14/23 17:53, Cristian Ciocaltea wrote:
> On 12/11/23 07:58, Venkata Prasad Potturu wrote:
>> On 12/10/23 21:20, Cristian Ciocaltea wrote:
>>> On 12/10/23 16:01, Mark Brown wrote:
>>>> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>>>>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>>>>>> This should send to SOF git repo for rewiew, once SOF reviewers
>>>>>> approved
>>>>>> this, again need to send to broonie git.
>>>>>> All the changes in sound/soc/sof/ path should go to SOF git.
>>>>> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
>>>>> enough to have this patch cc-ed to
>>>>> [email protected]?
>>>> The SOF people basically do their own thing in github at
>>>>
>>>> https://github.com/thesofproject/linux
>>>>
>>>> with a github workflow and submit their patches upstream in batches a
>>>> few times a release, however my understanding is that their workflow can
>>>> cope with things going in directly upstream as well.
>>> Thanks for clarifying, Mark! That would greatly simplify and speedup
>>> the whole process, at least for trivial patches like this one.
>> Hi Cristian,
>>
>> We have created a Pull request in SOF git hub for I2S BT support.
>> please hold v2 version SOF patches till below PR get's merged.
>> PR:- https://github.com/thesofproject/linux/pull/4742
> Hi Venkata,
>
> If this is going to be handled via the github workflow, this patch
> should be removed from the series. Since there is no dependency on it,
> I cannot see a reason to put v2 on hold.
>
> Do I miss something?
Yes, need to drop this patch.
And it's better to follow the github workflow by creating a Pull request
in SOF github
for all sof driver related patches, rather than sending patches to
broonie git directly.
>
> Thanks,
> Cristian
On 12/14/23 17:27, Péter Ujfalusi wrote:
>
> On 14/12/2023 12:58, Venkata Prasad Potturu wrote:
>> On 12/14/23 16:18, Péter Ujfalusi wrote:
>> Thanks for your time Peter!
>>> On 09/12/2023 22:53, Cristian Ciocaltea wrote:
>>>> Some SOF drivers like AMD ACP do not always rely on a single static
>>>> firmware file, but may require multiple files having their names
>>>> dynamically computed on probe time, e.g. based on chip name.
>>> I see, AMD vangogh needs two binary files to be loaded sometimes, it
>>> is not using the default name as it hints via the sof_dev_desc
>>> ("sof-vangogh.ri").
>>>
>>> The constructed names for the two files are just using different pattern:
>>> sof-%PLAT%.ri
>>> vs
>>> sof-%PLAT%-code.bin
>>> sof-%PLAT%-data.bin
>>>
>>> iow, instead of the combined .ri file which includes the code and data
>>> segment it has 'raw' bin files and cannot use the core for loading the
>>> firmware.
>>>
>>> What is the reason for this? an .ri file can have two 'modules' one to
>>> be written to IRAM the other to DRAM.
>>> sof_ipc3_load_fw_to_dsp()
>> For AMD Vangogh platform devices signed firmware image is required, so
>> split .ri image into code and data images.
>>
>> Only Code.bin will be signed and loaded into corresponding IRAM location.
> This is not different than what the Intel .ri files are made of. The
> module which is to be loaded to IRAM is signed code the module which
> goes to DRAM is not signed.
> The loader itself is not looking into the sections of the .ri image, it
> just parses the header and copies them where they belong.
>
> if the issue is name collision then you could try to put the signed
> firmware file under 'signed' folder (fw_path_postfix) of the platform
> like Intel does with the community signed ones?
We have a limitation that code image can't be signed during compilation.
So splitting the .ri image into code and data bin and sign the code bin
and load into IRAM.
>
> It would be great if somehow we can handle all of these in core, have
> shared code and familiar prints among vendors, platforms..
>
> Fwiw, I'm planning the path, filename creation to be moved to core for
> the current platforms, but it implies that they do use single firmware file.
> struct sof_dev_desc would only have two strings:
> vendor - AMD / iMX / Intel / Mediatek
> platform - tgl, vaggogh, etc
>
> I need to adjust it based on what I have learned today about vangogh.
>
On 12/14/23 17:53, Cristian Ciocaltea wrote:
> On 12/11/23 07:58, Venkata Prasad Potturu wrote:
>> On 12/10/23 21:20, Cristian Ciocaltea wrote:
>>> On 12/10/23 16:01, Mark Brown wrote:
>>>> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>>>>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>>>>>> This should send to SOF git repo for rewiew, once SOF reviewers
>>>>>> approved
>>>>>> this, again need to send to broonie git.
>>>>>> All the changes in sound/soc/sof/ path should go to SOF git.
>>>>> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
>>>>> enough to have this patch cc-ed to
>>>>> [email protected]?
>>>> The SOF people basically do their own thing in github at
>>>>
>>>> https://github.com/thesofproject/linux
>>>>
>>>> with a github workflow and submit their patches upstream in batches a
>>>> few times a release, however my understanding is that their workflow can
>>>> cope with things going in directly upstream as well.
>>> Thanks for clarifying, Mark! That would greatly simplify and speedup
>>> the whole process, at least for trivial patches like this one.
>> Hi Cristian,
>>
>> We have created a Pull request in SOF git hub for I2S BT support.
>> please hold v2 version SOF patches till below PR get's merged.
>> PR:- https://github.com/thesofproject/linux/pull/4742
> Hi Venkata,
>
> If this is going to be handled via the github workflow, this patch
> should be removed from the series. Since there is no dependency on it,
> I cannot see a reason to put v2 on hold.
>
> Do I miss something?
Non-sof driver related patches can directly send to broonie git ad v2
series.
SOF driver patches should send to SOF github to avoid merge conflicts
as per guidelines of SOF community.
>
> Thanks,
> Cristian
On 12/14/23 15:15, Venkata Prasad Potturu wrote:
>
> On 12/14/23 17:53, Cristian Ciocaltea wrote:
>> On 12/11/23 07:58, Venkata Prasad Potturu wrote:
>>> On 12/10/23 21:20, Cristian Ciocaltea wrote:
>>>> On 12/10/23 16:01, Mark Brown wrote:
>>>>> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>>>>>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>>>>>>> This should send to SOF git repo for rewiew, once SOF reviewers
>>>>>>> approved
>>>>>>> this, again need to send to broonie git.
>>>>>>> All the changes in sound/soc/sof/ path should go to SOF git.
>>>>>> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
>>>>>> enough to have this patch cc-ed to
>>>>>> [email protected]?
>>>>> The SOF people basically do their own thing in github at
>>>>>
>>>>> https://github.com/thesofproject/linux
>>>>>
>>>>> with a github workflow and submit their patches upstream in batches a
>>>>> few times a release, however my understanding is that their
>>>>> workflow can
>>>>> cope with things going in directly upstream as well.
>>>> Thanks for clarifying, Mark! That would greatly simplify and speedup
>>>> the whole process, at least for trivial patches like this one.
>>> Hi Cristian,
>>>
>>> We have created a Pull request in SOF git hub for I2S BT support.
>>> please hold v2 version SOF patches till below PR get's merged.
>>> PR:- https://github.com/thesofproject/linux/pull/4742
>> Hi Venkata,
>>
>> If this is going to be handled via the github workflow, this patch
>> should be removed from the series. Since there is no dependency on it,
>> I cannot see a reason to put v2 on hold.
>>
>> Do I miss something?
> Non-sof driver related patches can directly send to broonie git ad v2
> series.
> SOF driver patches should send to SOF github to avoid merge conflicts
> as per guidelines of SOF community.
Honestly, I don't really see a high risk of conflicts, the patches are
not that complex and can be simply cherry-picked when needed. Moreover,
as we already had people reviewing this, splitting this up will only add
confusion and unnecessary burden.
Are there any specific changes you are concerned about and cannot be
really handled here?
On 12/14/23 22:12, Cristian Ciocaltea wrote:
> On 12/14/23 15:15, Venkata Prasad Potturu wrote:
>> On 12/14/23 17:53, Cristian Ciocaltea wrote:
>>> On 12/11/23 07:58, Venkata Prasad Potturu wrote:
>>>> On 12/10/23 21:20, Cristian Ciocaltea wrote:
>>>>> On 12/10/23 16:01, Mark Brown wrote:
>>>>>> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>>>>>>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>>>>>>>> This should send to SOF git repo for rewiew, once SOF reviewers
>>>>>>>> approved
>>>>>>>> this, again need to send to broonie git.
>>>>>>>> All the changes in sound/soc/sof/ path should go to SOF git.
>>>>>>> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
>>>>>>> enough to have this patch cc-ed to
>>>>>>> [email protected]?
>>>>>> The SOF people basically do their own thing in github at
>>>>>>
>>>>>> https://github.com/thesofproject/linux
>>>>>>
>>>>>> with a github workflow and submit their patches upstream in batches a
>>>>>> few times a release, however my understanding is that their
>>>>>> workflow can
>>>>>> cope with things going in directly upstream as well.
>>>>> Thanks for clarifying, Mark! That would greatly simplify and speedup
>>>>> the whole process, at least for trivial patches like this one.
>>>> Hi Cristian,
>>>>
>>>> We have created a Pull request in SOF git hub for I2S BT support.
>>>> please hold v2 version SOF patches till below PR get's merged.
>>>> PR:- https://github.com/thesofproject/linux/pull/4742
>>> Hi Venkata,
>>>
>>> If this is going to be handled via the github workflow, this patch
>>> should be removed from the series. Since there is no dependency on it,
>>> I cannot see a reason to put v2 on hold.
>>>
>>> Do I miss something?
>> Non-sof driver related patches can directly send to broonie git ad v2
>> series.
>> SOF driver patches should send to SOF github to avoid merge conflicts
>> as per guidelines of SOF community.
> Honestly, I don't really see a high risk of conflicts, the patches are
> not that complex and can be simply cherry-picked when needed. Moreover,
> as we already had people reviewing this, splitting this up will only add
> confusion and unnecessary burden.
>
> Are there any specific changes you are concerned about and cannot be
> really handled here?
This is not the concern about this patch series,
Generally sof driver patches sends to SOF git hub as a PR, these are the
guidelines by SOF maintainers.
If you still want to send alsa devel list directly, please discuss with
SOF maintainers.
On 12/15/23 11:58, Venkata Prasad Potturu wrote:
>
> On 12/14/23 22:12, Cristian Ciocaltea wrote:
>> On 12/14/23 15:15, Venkata Prasad Potturu wrote:
>>> On 12/14/23 17:53, Cristian Ciocaltea wrote:
>>>> On 12/11/23 07:58, Venkata Prasad Potturu wrote:
>>>>> On 12/10/23 21:20, Cristian Ciocaltea wrote:
>>>>>> On 12/10/23 16:01, Mark Brown wrote:
>>>>>>> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>>>>>>>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>>>>>>>>> This should send to SOF git repo for rewiew, once SOF reviewers
>>>>>>>>> approved
>>>>>>>>> this, again need to send to broonie git.
>>>>>>>>> All the changes in sound/soc/sof/ path should go to SOF git.
>>>>>>>> Unfortunately I'm not familiar with the SOF dev workflow. So
>>>>>>>> it's not
>>>>>>>> enough to have this patch cc-ed to
>>>>>>>> [email protected]?
>>>>>>> The SOF people basically do their own thing in github at
>>>>>>>
>>>>>>> https://github.com/thesofproject/linux
>>>>>>>
>>>>>>> with a github workflow and submit their patches upstream in
>>>>>>> batches a
>>>>>>> few times a release, however my understanding is that their
>>>>>>> workflow can
>>>>>>> cope with things going in directly upstream as well.
>>>>>> Thanks for clarifying, Mark! That would greatly simplify and speedup
>>>>>> the whole process, at least for trivial patches like this one.
>>>>> Hi Cristian,
>>>>>
>>>>> We have created a Pull request in SOF git hub for I2S BT support.
>>>>> please hold v2 version SOF patches till below PR get's merged.
>>>>> PR:- https://github.com/thesofproject/linux/pull/4742
>>>> Hi Venkata,
>>>>
>>>> If this is going to be handled via the github workflow, this patch
>>>> should be removed from the series. Since there is no dependency on it,
>>>> I cannot see a reason to put v2 on hold.
>>>>
>>>> Do I miss something?
>>> Non-sof driver related patches can directly send to broonie git ad v2
>>> series.
>>> SOF driver patches should send to SOF github to avoid merge conflicts
>>> as per guidelines of SOF community.
>> Honestly, I don't really see a high risk of conflicts, the patches are
>> not that complex and can be simply cherry-picked when needed. Moreover,
>> as we already had people reviewing this, splitting this up will only add
>> confusion and unnecessary burden.
>>
>> Are there any specific changes you are concerned about and cannot be
>> really handled here?
> This is not the concern about this patch series,
> Generally sof driver patches sends to SOF git hub as a PR, these are the
> guidelines by SOF maintainers.
> If you still want to send alsa devel list directly, please discuss with
> SOF maintainers.
I think this series makes sense as a whole and it's best to be handled
here, as it only provides trivial fixes to issues found on mainline.
If the SOF workflow is unable to integrate fixes submitted upstream, I
would perceive that as a significant drawback of adhering to that
process. It is hard to believe, though, that this is really the case.
Hence, I kindly ask everyone here to shed some light and help move this
forward.
Thank you,
Cristian
On Fri, Dec 15, 2023 at 12:57:34PM +0200, Cristian Ciocaltea wrote:
> If the SOF workflow is unable to integrate fixes submitted upstream, I
> would perceive that as a significant drawback of adhering to that
> process. It is hard to believe, though, that this is really the case.
As far as I'm aware they can cope fine with that, though it does help if
people try to avoid needless collisions. It *does* cause trouble to use
both github and upstream flows simultaneously and there's a preference
for pushing anything substantial through github but picking one or the
other works as far as I know.