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

fiptool: Add --pad command line option #952

Closed
wants to merge 1 commit into from
Closed

fiptool: Add --pad command line option #952

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented May 25, 2017

Allow each image to reserve some padding space before alignment.

Use case is --align 0x4000 --pad 64 to make sure sufficient
room is available to prepend (or append) vendor-specific headers.

Helps resolve afaerber/meson-tools#3.

Change-Id: I31e59456600b72cd30cde0f653ec6523480a5823
Signed-off-by: Andreas Färber [email protected]
Signed-off-by: Dimitris Papastamos [email protected]

Copy link
Contributor

@afaerber afaerber left a comment

Choose a reason for hiding this comment

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

Thanks, but "dp-arm" is not an acceptable Signed-off-by.

@ghost
Copy link
Author

ghost commented May 25, 2017

Thanks, but "dp-arm" is not an acceptable Signed-off-by.

All of my patches use that form. I will change it for new submissions.

@afaerber
Copy link
Contributor

It violates contributing.md and the Linux DCO.

@masahir0y
Copy link
Contributor

Use case is --align 0x4000 --pad 64 to make sure sufficient
room is available to prepend (or append) vendor-specific headers.

From this statement, it was not clear whether --pad means --pre-pad or --post-pad.

If I understood the source code correctly, it looks like --post-pad.
It it OK if you are sure --pre-pad is unneeded.

Here, I mean --pre-pad aligns the start of extra space,
and --post-pad aligns the start of image bodies.

Allow each image to reserve some padding space before alignment.

Use case is --align 0x4000 --pad 64 to make sure sufficient
room is available to prepend (or append) vendor-specific headers.

Helps resolve afaerber/meson-tools#3.

Change-Id: I31e59456600b72cd30cde0f653ec6523480a5823
Signed-off-by: Andreas Färber <[email protected]>
Signed-off-by: Dimitris Papastamos <[email protected]>
@ghost
Copy link
Author

ghost commented May 25, 2017

Here, I mean --pre-pad aligns the start of extra space,
and --post-pad aligns the start of image bodies.

Yes, I think it is --post-pad according to this definition. I guess the reason --pre-pad is not going to be particularly useful is because the image will not run correctly if the start of the image contains extra headers (offsets will be incorrect). There's no magic done in the TF loader to cope with this situation.

@afaerber
Copy link
Contributor

afaerber commented May 25, 2017

Actually the Amlogic firmware does need a pre-pad, but the proprietary tool (and currently my tool) assumes a post-pad. For a --pre-pad much more code would need to be changed than just the alignment lines. Not sure if it's worth implementing that, given that a simple cat command could achieve the same effect for a fixed size header. On the other hand, by that argument, I could just cat a dd if=/dev/zero of=pad bs=1 count=N file to the end of my blobs and use unmodified --align and drop the PR. Thoughts?

@masahir0y
Copy link
Contributor

On the other hand, by that argument, I could just cat a dd if=/dev/zero of=pad bs=1 count=N file to the end of my blobs and use unmodified --align and drop the PR. Thoughts

Yeah, I was also thinking this.

Append (or prepend) headers, then put the images into FIP
will be easier than
Make extra spaces in FIP, then embed headers

@danh-arm
Copy link
Contributor

Hi @afaerber. We don't have a strong view on whether it's better to use dd before putting images into the FIP or to use the feature in this PR. If you say this PR satisfies your needs and you will use it, we will merge, otherwise we'll drop it.

@@ -729,7 +749,8 @@ static void create_usage(void)
printf("fiptool create [opts] FIP_FILENAME\n");
printf("\n");
printf("Options:\n");
printf(" --align <value>\t\tEach image is aligned to <value> (default: 1).\n");
printf(" --align <bytes>\t\tEach image is aligned to <bytes> (default: 1).\n");
printf(" --pad <bytes>\t\t\tEach image is padded by <bytes> (default: 0).\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: Could say "... padded at the end by...". Same for other cases below.

@danh-arm
Copy link
Contributor

jenkins: retest this please

@ghost
Copy link
Author

ghost commented Jun 19, 2017

@afaerber ping

@davidcunado-arm
Copy link
Contributor

@afaerber
We're still pending your feedback on this PR. Please could you confirm that this PR satisfies your needs and you will use it?
I'll look to either close it or merge it by Wednesday 28-JUN depending on what your feedback is.

@davidcunado-arm
Copy link
Contributor

Closing PR - it has not been confirmed that this change will be used. Please re-open if this PR is still required.

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.

4 participants