Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mengdonglin wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/ext_manifest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ int ext_man_write_cavs_25(struct image *image)

header.version_major = mod_ext->ext_mod_config_array->header.version_major;
header.version_minor = mod_ext->ext_mod_config_array->header.version_minor;
header.version_micro = mod_ext->ext_mod_config_array->header.version_micro;
header.num_module_entries = count;
header.id = EXTENDED_MANIFEST_MAGIC_HEADER_ID;
header.len = sizeof(const struct fw_ext_man_cavs_header);
Expand Down
3 changes: 2 additions & 1 deletion src/include/rimage/cavs/cavs_ext_manifest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ struct fw_ext_man_cavs_header {
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 */

@lgirdwood lgirdwood May 3, 2022

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 •
edited
Loading

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!

@RanderWang RanderWang May 4, 2022

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.

@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.

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.

@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.

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?

uint32_t num_module_entries;
} __attribute__((packed));

Expand All @@ -238,7 +239,7 @@ struct fw_ext_mod_config_header {
uint32_t guid[4]; /* Module GUID */
uint16_t version_major; /* Module version */
uint16_t version_minor; /* Module version */
uint16_t version_hotfix; /* Module version */
uint16_t version_micro; /* Module version */
uint16_t version_build; /* Module version */
enum mod_type module_type;
uint32_t init_settings_min_size; /* Minimum size of initialization settings (in bytes) */
Expand Down
2 changes: 1 addition & 1 deletion src/include/rimage/css.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ struct image;
struct fw_version {
uint16_t major_version;
uint16_t minor_version;
uint16_t hotfix_version;
uint16_t micro_version;
uint16_t build_version;
} __attribute__((packed));

Expand Down
1 change: 1 addition & 0 deletions src/include/rimage/rimage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ struct image {
char* fw_ver_build_string;
uint16_t fw_ver_major;
uint16_t fw_ver_minor;
uint16_t fw_ver_micro;
uint16_t fw_ver_build;
};

Expand Down
2 changes: 1 addition & 1 deletion src/include/rimage/sof/user/manifest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ struct sof_man_fw_header {
uint32_t feature_mask;
uint16_t major_version;
uint16_t minor_version;
uint16_t hotfix_version;
uint16_t micro_version;
uint16_t build_version;
uint32_t num_module_entries;
uint32_t hw_buf_base_addr;
Expand Down
9 changes: 9 additions & 0 deletions src/manifest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,6 +776,7 @@ int man_write_fw_v1_5(struct image *image)
/* firmware and build version */
m->desc.header.major_version = image->fw_ver_major;
m->desc.header.minor_version = image->fw_ver_minor;
m->desc.header.micro_version = image->fw_ver_micro;
m->desc.header.build_version = image->fw_ver_build;

/* create each module */
Expand Down Expand Up @@ -845,6 +846,7 @@ int man_write_fw_v1_5_sue(struct image *image)
/* firmware and build version */
m->desc.header.major_version = image->fw_ver_major;
m->desc.header.minor_version = image->fw_ver_minor;
m->desc.header.micro_version = image->fw_ver_micro;
m->desc.header.build_version = image->fw_ver_build;

/* create each module - subtract the boot loader exec header */
Expand Down Expand Up @@ -913,9 +915,11 @@ int man_write_fw_v1_8(struct image *image)
/* firmware and build version */
m->css.version.major_version = image->fw_ver_major;
m->css.version.minor_version = image->fw_ver_minor;
m->css.version.micro_version = image->fw_ver_micro;
m->css.version.build_version = image->fw_ver_build;
m->desc.header.major_version = image->fw_ver_major;
m->desc.header.minor_version = image->fw_ver_minor;
m->desc.header.micro_version = image->fw_ver_micro;
m->desc.header.build_version = image->fw_ver_build;

/* create each module */
Expand Down Expand Up @@ -1022,6 +1026,7 @@ int man_write_fw_meu_v1_5(struct image *image)
/* firmware and build version */
desc->header.major_version = image->fw_ver_major;
desc->header.minor_version = image->fw_ver_minor;
desc->header.micro_version = image->fw_ver_micro;
desc->header.build_version = image->fw_ver_build;

/* create each module */
Expand Down Expand Up @@ -1102,6 +1107,7 @@ int man_write_fw_meu_v1_8(struct image *image)
/* firmware and build version */
desc->header.major_version = image->fw_ver_major;
desc->header.minor_version = image->fw_ver_minor;
desc->header.micro_version = image->fw_ver_micro;
desc->header.build_version = image->fw_ver_build;

/* create each module */
Expand Down Expand Up @@ -1182,6 +1188,7 @@ int man_write_fw_meu_v2_5(struct image *image)
/* firmware and build version */
desc->header.major_version = image->fw_ver_major;
desc->header.minor_version = image->fw_ver_minor;
desc->header.micro_version = image->fw_ver_micro;
desc->header.build_version = image->fw_ver_build;

/* create each module */
Expand Down Expand Up @@ -1263,9 +1270,11 @@ int man_write_fw_v2_5(struct image *image)
/* firmware and build version */
m->css.version.major_version = image->fw_ver_major;
m->css.version.minor_version = image->fw_ver_minor;
m->css.version.micro_version = image->fw_ver_micro;
m->css.version.build_version = image->fw_ver_build;
m->desc.header.major_version = image->fw_ver_major;
m->desc.header.minor_version = image->fw_ver_minor;
m->desc.header.micro_version = image->fw_ver_micro;
m->desc.header.build_version = image->fw_ver_build;

/* create each module */
Expand Down
10 changes: 6 additions & 4 deletions src/rimage.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,19 +115,21 @@ int main(int argc, char *argv[])
return -EINVAL;
}

/* firmware version and build id */
/* firmware version that can be major.minor or major.minor.micro */
if (image.fw_ver_string) {
ret = sscanf(image.fw_ver_string, "%hu.%hu",
ret = sscanf(image.fw_ver_string, "%hu.%hu.%hu",
&image.fw_ver_major,
&image.fw_ver_minor);
&image.fw_ver_minor,
&image.fw_ver_micro);

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

@aiChaoSONG aiChaoSONG May 4, 2022 •
edited
Loading

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.

@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.

@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 •
edited
Loading

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

@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.

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

@mengdonglin mengdonglin May 4, 2022 •
edited
Loading

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

fprintf(stderr,
"error: cannot parse firmware version\n");
return -EINVAL;
}
}

/* firmware build id */
if (image.fw_ver_build_string) {
ret = sscanf(image.fw_ver_build_string, "%hu",
&image.fw_ver_build);
Expand Down