On 2022-02-25 3:15 AM, Pierre-Louis Bossart wrote:
+#define AVS_FW_INIT_TIMEOUT_MS 3000
another timeout?
Different operation, different timeout. What's wrong with that? And we are not repeating it three times (skl/bxt/cnl) as it's done in skylake-driver!
skl-sst.c:#define SKL_BASEFW_TIMEOUT 300 skl-sst.c:#define SKL_INIT_TIMEOUT 1000 bxt-sst.c:#define BXT_BASEFW_TIMEOUT 3000 cnl-sst.c:#define CNL_INIT_TIMEOUT 300 cnl-sst.c:#define CNL_BASEFW_TIMEOUT 3000
+#define AVS_ROOT_DIR "intel/avs" +#define AVS_BASEFW_FILENAME "dsp_basefw.bin" +#define AVS_EXT_MANIFEST_MAGIC 0x31454124 +#define SKL_MANIFEST_MAGIC 0x00000006 +#define SKL_ADSPFW_OFFSET 0x284
+static bool debug_ignore_fw_version_check;
this_is_a_very_long_variable_name_isn_t_it?
Can drop "_check", dropping anything else makes this ambiguous. Also, this variable is used just once, in the very line below.
+module_param_named(ignore_fw_version, debug_ignore_fw_version_check, bool, 0444); +MODULE_PARM_DESC(ignore_fw_version, "Verify FW version 0=yes (default), 1=no");
You should clarify the purpose of the version check, and why this driver needs different firmware binaries and versions than what was already released to linux-firmware.
Ack. Indeed the name by itself may not be sufficient.
+static int avs_fw_manifest_strip_verify(struct avs_dev *adev, struct firmware *fw,
const struct avs_fw_version *min)+{
- struct avs_fw_manifest *man;
- int offset, ret;
- ret = avs_fw_ext_manifest_strip(fw);
- if (ret)
return ret;- offset = avs_fw_manifest_offset(fw);
- if (offset < 0)
return offset;- if (fw->size < offset + sizeof(*man))
return -EINVAL;- if (!min)
return 0;- man = (struct avs_fw_manifest *)(fw->data + offset);
- if (man->version.major != min->major ||
man->version.minor != min->minor ||man->version.hotfix != min->hotfix ||man->version.build < min->build) {dev_warn(adev->dev, "bad FW version %d.%d.%d.%d, expected %d.%d.%d.%d or newer\n",man->version.major, man->version.minor,man->version.hotfix, man->version.build,min->major, min->minor, min->hotfix, min->build);usually when the relevant firmware is not found, the distributions and users like to see a message informing them of the location of the firmware binaries.
see thread with Bruce Perens and Jaroslav at https://github.com/thesofproject/sof-bin/issues/22
That's a helpful link and a good conversation, thank you. I see that Bruce mentions /lib/firmware as 3rd in presented order and that's where we intend to have firmware files. We are not planning to have a separate repo for base firmware/library binaries if that's the question.
Given the above, I'm not sure if we should be mentioning the need to update /lib/firmware explicitly as people already recognize it as one of the default locations to check.
if (!debug_ignore_fw_version_check)return -EINVAL;- }
- return 0;
+}
+static int avs_dsp_load_basefw(struct avs_dev *adev) +{
- const struct avs_fw_version *min_req;
- const struct avs_spec *const spec = adev->spec;
- const struct firmware *fw;
- struct firmware stripped_fw;
- char *filename;
- int ret;
- filename = kasprintf(GFP_KERNEL, "%s/%s/%s", AVS_ROOT_DIR, spec->name,
AVS_BASEFW_FILENAME);- if (!filename)
return -ENOMEM;- ret = avs_request_firmware(adev, &fw, filename);
- kfree(filename);
- if (ret < 0) {
dev_err(adev->dev, "request firmware failed: %d\n", ret);return ret;- }
- stripped_fw = *fw;
- min_req = &adev->spec->min_fw_version;
- ret = avs_fw_manifest_strip_verify(adev, &stripped_fw, min_req);
- if (ret < 0) {
dev_err(adev->dev, "invalid firmware data: %d\n", ret);should you not release the firmware in all error cases?
if this is handled at a higher level, please add a comment.
I'll sync with Amadeo regarding this. It seems it's cleared only on ->remove().
return ret;- }
- ret = avs_dsp_op(adev, load_basefw, &stripped_fw);
- if (ret < 0) {
dev_err(adev->dev, "basefw load failed: %d\n", ret);return ret;- }
- ret = wait_for_completion_timeout(&adev->fw_ready,
msecs_to_jiffies(AVS_FW_INIT_TIMEOUT_MS));- if (!ret) {
dev_err(adev->dev, "firmware ready timeout\n");avs_dsp_core_disable(adev, AVS_MAIN_CORE_MASK);return -ETIMEDOUT;- }
- return 0;
+}