> +#define AVS_FW_INIT_TIMEOUT_MS 3000
another timeout?
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?
> +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.
> +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
> +
> + 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.
> + 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;
> +}
