Skip to content

Write firmware file micro version to manifest for cAVS platforms - #91

Closed
mengdonglin wants to merge 1 commit into
thesofproject:mainfrom
mengdonglin:micro-version
Closed

Write firmware file micro version to manifest for cAVS platforms#91
mengdonglin wants to merge 1 commit into
thesofproject:mainfrom
mengdonglin:micro-version

Conversation

@mengdonglin

Copy link
Copy Markdown
Contributor

Fix #90

SOF CMake defines SOF_MAJOR, SOF_MINOR and SOF_MICRO for 3 fields
of firmware file version major.minor.micro

This patch changes hotfix_version to micro_version in headers to align
with SOF CMake, and adds fw_ver_micro to firmware image context.

If SOF CMake passes firmware file micro version in the version string,
rimage will write it to different manifest headers for cAVS platforms.
If micro version isn't passed to rimage, it's 0 by default.

This feature will allow sof_ri_info.py to dump entire file version from
a FW binary for cAVS platforms.

There will be a SOF PR to let CMake pass FW file micro version to rimage.

@mengdonglin

Copy link
Copy Markdown
Contributor Author

thesofproject/sof#5760 is the PR let CMake pass FW file micro version to rimage.

SOF CMake defines SOF_MAJOR, SOF_MINOR and SOF_MICRO for 3 fields
of firmware file version major.minor.micro

This patch changes hotfix_version to micro_version in headers to align
with SOF CMake, and adds fw_ver_micro to firmware image context.

If SOF CMake passes firmware file micro version in the version string,
rimage will write it to different manifest headers for cAVS platforms.
If micro version isn't passed to rimage, it's 0 by default.

This feature will allow sof_ri_info.py to dump entire file version from
a FW binary for cAVS platforms.

Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
uint32_t len; /* sizeof(Extend Manifest) in bytes */
uint16_t version_major; /* Version of Extended Manifest structure */
uint16_t version_minor; /* Version of Extended Manifest structure */
uint16_t version_micro; /* Version of Extended Manifest structure */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wont this break manifest parsing on Windows ? @RanderWang can you comment.

@mengdonglin mengdonglin May 4, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgirdwood Rander is on vacation this week. Let's wait for his comments next week. Hope I'm not breaking Windows. Thank you for raising this risk!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows driver will take version_micro as num_module_entries we don't change header file used by windows, and this will result to error.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows driver will take version_micro as num_module_entries we don't change header file used by windows, and this will result to error.

This only applies to the extended manifest. So I think we can skip the micro version only for cavs extended manifest.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid breaking Windows, I wrote a new PR #92 that will not change manifest header definitions but write the micro version to the unused hotfix_version field of manifest header. Can you help review the new PR#92?

Comment thread src/rimage.c
&image.fw_ver_micro);

if (ret != 2) {
if (ret != 2 && ret != 3) {

@aiChaoSONG aiChaoSONG May 4, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return value of sscanf is the number of receiving arguments successfully assigned or EOF, I don't think we should allow partial assignment. the ret here must be 3 for successful condition.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aiChaoSONG Thanks for your comments. This is to be compatible with current SOF CMake that only passes 2 version arguments (major.minor) to rimage. If this rimage PR gets merged at first, SOF building will fail as CMake doesn't pass 3 version arguments (major.minor.micro) as rimage expects.

If SOF CMake thesofproject/sof#5760 can also be merged, I can update rimage to only support 3 version arguments.

@aiChaoSONG aiChaoSONG May 4, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even the PR get merged, it will not be used, rimage is a submodule of SOF, unless rimage is upgraded on SOF side, new changes will never be used. so it's safe.

You can check that SOF currently uses rimage@ee5c3e1, but the newest rimage commit is 05be213

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks! I Will update PR after Rander comments if this PR will impact Windows.

@mengdonglin mengdonglin May 4, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this in new PR #92 @aiChaoSONG

@mengdonglin

Copy link
Copy Markdown
Contributor Author

Close this PR and submitted a new PR #92 to address all review comments. @lgirdwood @aiChaoSONG @RanderWang

@mengdonglin mengdonglin closed this May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Add SOF micro version to ADSP manifest header

4 participants