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;
+}
