Skip to content
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.

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

imgtool: Add support for write_size > 8 #1002

Closed
wants to merge 1 commit into from
Closed

imgtool: Add support for write_size > 8 #1002

wants to merge 1 commit into from

Conversation

arvinf
Copy link
Contributor

@arvinf arvinf commented May 8, 2021

Increase write_size paramter to 16 to support SoCs such as Atmel SAM4S or
SAMV70 with 128-bit default flash alignments.

Signed-off-by: Arvin Farahmand arvinf@ip-logix.com

Increase write_size paramter to 16 to support SoCs such as Atmel SAM4S or
SAMV70 with 128-bit default flash alignments.

Signed-off-by: Arvin Farahmand <arvinf@ip-logix.com>
@d3zd3z
Copy link
Member

d3zd3z commented May 11, 2021

If we plan to support these large alignments in the bootloader code, we should also add the 16-byte alignment to the end of image.rs. For large-writes, we test 128 and 512, but it looks like 16 will be common. A quick test with 'master' shows that setting the write alignment to 16 fails most tests at this point.

Copy link
Member

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

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

I'm ok with these changes, although '> 8' might be a little misleading, since this only supports a write size of 16 in addition to currently supported sizes.

@arvinf
Copy link
Contributor Author

arvinf commented May 11, 2021

I'll edit the commit message.

Copy link
Member

@utzig utzig left a comment

Choose a reason for hiding this comment

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

I believe this is misleading, since it won't really add any support for 16-byte alignments, only make a user believe it does by having that option in imgtool. Before bootutil supports alignments larger than 8-byte there is no reason to actually have this option. Also this duplicates effort which was already done in #949. Maybe you should work on improving that PR?

@arvinf
Copy link
Contributor Author

arvinf commented May 11, 2021

My understanding was that bootutil gets the alignment from the flash device and uses it as-is. I'll take a look at the PR you referenced.

@utzig
Copy link
Member

utzig commented May 11, 2021

My understanding was that bootutil gets the alignment from the flash device and uses it as-is. I'll take a look at the PR you referenced.

It gets the alignment from the flash driver, if it comes from HW or from a constant in the flash driver depends on the implementation of the flash backend in the OS port. The issue is that there is also a local constant called MAX_BOOT_ALIGN which sets the limit we allow, so no matter what imgtool might get in the command line, it won't just work out of the box. And btw imgtool also uses a similar constant afair.

@arvinf
Copy link
Contributor Author

arvinf commented May 11, 2021

Thank you for the clarification.

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the stale label Nov 11, 2021
@github-actions github-actions bot closed this Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants