Re: [PATCH v3 1/2] ASoC: SOF: amd: Move signed_fw_image to struct acp_quirk_entry

Il 20/02/24 21:16, Cristian Ciocaltea ha scritto:

The signed_fw_image member of struct sof_amd_acp_desc is used to enable signed firmware support in the driver via the acp_sof_quirk_table.

In preparation to support additional use cases of the quirk table (i.e. adding new flags), move signed_fw_image to a new struct acp_quirk_entry and update all references to it accordingly.

No functional changes intended.

Signed-off-by: Cristian Ciocaltea cristian.ciocaltea@collabora.com

sound/soc/sof/amd/acp-loader.c | 2 +- sound/soc/sof/amd/acp.c | 47 ++++++++++++++++++---------------- sound/soc/sof/amd/acp.h | 6 ++++- sound/soc/sof/amd/vangogh.c | 9 +++++-- 4 files changed, 38 insertions(+), 26 deletions(-)

diff --git a/sound/soc/sof/amd/acp-loader.c b/sound/soc/sof/amd/acp-loader.c index d2d21478399e..aad904839b81 100644 --- a/sound/soc/sof/amd/acp-loader.c +++ b/sound/soc/sof/amd/acp-loader.c @@ -173,7 +173,7 @@ int acp_dsp_pre_fw_run(struct snd_sof_dev *sdev)

adata = sdev->pdata->hw_pdata;

• if (adata->signed_fw_image)

• if (adata->quirks && adata->quirks->signed_fw_image) size_fw = adata->fw_bin_size - ACP_FIRMWARE_SIGNATURE; else size_fw = adata->fw_bin_size;

diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c index 9b3c26210db3..9d9197fa83ed 100644 --- a/sound/soc/sof/amd/acp.c +++ b/sound/soc/sof/amd/acp.c @@ -20,12 +20,14 @@ #include "acp.h" #include "acp-dsp-offset.h"

-#define SECURED_FIRMWARE 1

• static bool enable_fw_debug; module_param(enable_fw_debug, bool, 0444); MODULE_PARM_DESC(enable_fw_debug, "Enable Firmware debug");

+static struct acp_quirk_entry quirk_valve_galileo = {

• .signed_fw_image = true,

Hello Cristian,

are you sure that a structure holding "quirks" is the right choice here?

That probably comes as a personal preference, but I would simply pass a `u32 flags` and structure the quirks as bits.

#define ACP_SIGNED_FW_IMAGE BIT(0) #define ACP_SOMETHING_ELSE BIT(1)

flags = BIT(SIGNED_FW_IMAGE) | BIT(SOMETHING_ELSE);

if (flags & BIT(SIGNED_FW_IMAGE)) do_something()

What do you think?

Cheers, Angelo

+};

• const struct dmi_system_id acp_sof_quirk_table[] = { { /* Steam Deck OLED device */

@@ -33,7 +35,7 @@ const struct dmi_system_id acp_sof_quirk_table[] = { DMI_MATCH(DMI_SYS_VENDOR, "Valve"), DMI_MATCH(DMI_PRODUCT_NAME, "Galileo"), },

• .driver_data = (void *)SECURED_FIRMWARE,
  

• .driver_data = &quirk_valve_galileo,
  

  }, {} };

@@ -254,7 +256,7 @@ int configure_and_run_sha_dma(struct acp_dev_data *adata, void *image_addr, } }

• if (adata->signed_fw_image)

• if (adata->quirks && adata->quirks->signed_fw_image) snd_sof_dsp_write(sdev, ACP_DSP_BAR, ACP_SHA_DMA_INCLUDE_HDR, ACP_SHA_HEADER);

  snd_sof_dsp_write(sdev, ACP_DSP_BAR, ACP_SHA_DMA_STRT_ADDR, start_addr);

@@ -738,26 +740,27 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev) sdev->debug_box.offset = sdev->host_box.offset + sdev->host_box.size; sdev->debug_box.size = BOX_SIZE_1024;

• adata->signed_fw_image = false; dmi_id = dmi_first_match(acp_sof_quirk_table);
• if (dmi_id && dmi_id->driver_data) {

• adata->fw_code_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
  

• 				    "sof-%s-code.bin",
  

• 				    chip->name);
  

• if (!adata->fw_code_bin) {
  

• 	ret = -ENOMEM;
  

• 	goto free_ipc_irq;
  

• if (dmi_id) {

• adata->quirks = dmi_id->driver_data;
  

• if (adata->quirks->signed_fw_image) {
  

• 	adata->fw_code_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
  

• 					    "sof-%s-code.bin",
  

• 					    chip->name);
  

• 	if (!adata->fw_code_bin) {
  

• 		ret = -ENOMEM;
  

• 		goto free_ipc_irq;
  

• 	}
  

• 	adata->fw_data_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
  

• 					    "sof-%s-data.bin",
  

• 					    chip->name);
  

• 	if (!adata->fw_data_bin) {
  

• 		ret = -ENOMEM;
  

• 		goto free_ipc_irq;
  

• 	}
  

  }

• adata->fw_data_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
  

• 				    "sof-%s-data.bin",
  

• 				    chip->name);
  

• if (!adata->fw_data_bin) {
  

• 	ret = -ENOMEM;
  

• 	goto free_ipc_irq;
  

• }
  

• adata->signed_fw_image = dmi_id->driver_data;
  

  }

  adata->enable_fw_debug = enable_fw_debug;

diff --git a/sound/soc/sof/amd/acp.h b/sound/soc/sof/amd/acp.h index 947068da39b5..b648ed194b9f 100644 --- a/sound/soc/sof/amd/acp.h +++ b/sound/soc/sof/amd/acp.h @@ -207,6 +207,10 @@ struct sof_amd_acp_desc { u64 sdw_acpi_dev_addr; };

+struct acp_quirk_entry {

• bool signed_fw_image;

+};

• /* Common device data struct for ACP devices */ struct acp_dev_data { struct snd_sof_dev *dev;

@@ -236,7 +240,7 @@ struct acp_dev_data { u8 *data_buf; dma_addr_t sram_dma_addr; u8 *sram_data_buf;

• bool signed_fw_image;

• struct acp_quirk_entry *quirks; struct dma_descriptor dscr_info[ACP_MAX_DESC]; struct acp_dsp_stream stream_buf[ACP_MAX_STREAM]; struct acp_dsp_stream *dtrace_stream;

diff --git a/sound/soc/sof/amd/vangogh.c b/sound/soc/sof/amd/vangogh.c index de15d21aa6d9..bc6ffdb5471a 100644 --- a/sound/soc/sof/amd/vangogh.c +++ b/sound/soc/sof/amd/vangogh.c @@ -143,6 +143,7 @@ EXPORT_SYMBOL_NS(sof_vangogh_ops, SND_SOC_SOF_AMD_COMMON); int sof_vangogh_ops_init(struct snd_sof_dev *sdev) { const struct dmi_system_id *dmi_id;

• struct acp_quirk_entry *quirks;

  /* common defaults */ memcpy(&sof_vangogh_ops, &sof_acp_common_ops, sizeof(struct snd_sof_dsp_ops));

@@ -151,8 +152,12 @@ int sof_vangogh_ops_init(struct snd_sof_dev *sdev) sof_vangogh_ops.num_drv = ARRAY_SIZE(vangogh_sof_dai);

dmi_id = dmi_first_match(acp_sof_quirk_table);

• if (dmi_id && dmi_id->driver_data)

• sof_vangogh_ops.load_firmware = acp_sof_load_signed_firmware;
  

• if (dmi_id) {

• quirks = dmi_id->driver_data;
  

• if (quirks->signed_fw_image)
  

• 	sof_vangogh_ops.load_firmware = acp_sof_load_signed_firmware;
  

• }

  return 0; }