[U-Boot] [PATCH v2 0/5] boot_get_fdt: clean up and use 'fdtaddr' as fallback for Android
The main feature added in this series is being able to boot Android images with empty/non-DTB contents in the second area by using the address stored in the 'fdtaddr' environment variable. More details in "fdt: boot_get_fdt: android: use ENV 'fdtaddr' as fallback".
This is combined with a number of small-sized (review-friendly) cleanup patches, attempting to increase the readability of boot_get_fdt(). They are partitioned into functional (assumed to add a change in runtime behavior) and non-functional changes.
Comments/thoughts appreciated! TIA!
Changes in v2: - Dropped centralized fdt_len verification since it only makes sense for DT blobs embedded in the U-Boot-supported images (in which case two FDT size values are available and can be compared, one from the booted image via an image-specific API and one from the FDT itself via fdt_totalsize). Centralized fdt_len verification doesn't make much sense in case of user-provided DT blobs (via boot{m,*} command or via some environment variable) - Fixed 'booti' regression on arm64/R-Car3 - Fixed a number of typos - Link v1: https://patchwork.ozlabs.org/patch/1071586/
Eugeniu Rosca (5): fdt: boot_get_fdt: remove redundant zeroing out fdt: boot_get_fdt: really boot w/o FDT when "goto no_fdt" fdt: boot_get_fdt: simplify no_fdt handling (non-functional) fdt: boot_get_fdt: android: compress handling (non-functional) fdt: boot_get_fdt: android: use ENV 'fdtaddr' as fallback
common/image-fdt.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)
Paranoid programming [1] lies at the foundation of proper software development, but the repetitive zeroing-out of output arguments in the context of the same function rather clutters the code and inhibits further refactoring/optimization than is doing any good.
In boot_get_fdt(), we already perform zero/NULL-initialization of *of_flat_tree and *of_size at the beginning of the function, so doing the same at function error-out is redundant/superfluous.
Moreover, keeping the code unchanged might encourage the developers to update *of_flat_tree and *of_size during some interim computations, which is against the current design of boot_get_fdt(). Currently, writing useful data into these arguments happens just before successfully returning from boot_get_fdt() and it should better stay so.
[1] https://blog.regehr.org/archives/1106
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com --- Changes in v2: - s/zeroint-out/zeroing-out/ in commit description - Link v1: https://patchwork.ozlabs.org/patch/1071586/ --- common/image-fdt.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/common/image-fdt.c b/common/image-fdt.c index 01186aeac7a4..1817ce6bce30 100644 --- a/common/image-fdt.c +++ b/common/image-fdt.c @@ -489,8 +489,6 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch, no_fdt: ok_no_fdt = 1; error: - *of_flat_tree = NULL; - *of_size = 0; if (!select && ok_no_fdt) { debug("Continuing to boot without FDT\n"); return 0;
Hi Eugeniu,
On Mon, 1 Apr 2019 at 03:46, Eugeniu Rosca erosca@de.adit-jv.com wrote:
Paranoid programming [1] lies at the foundation of proper software development, but the repetitive zeroing-out of output arguments in the context of the same function rather clutters the code and inhibits further refactoring/optimization than is doing any good.
In boot_get_fdt(), we already perform zero/NULL-initialization of *of_flat_tree and *of_size at the beginning of the function, so doing the same at function error-out is redundant/superfluous.
Moreover, keeping the code unchanged might encourage the developers to update *of_flat_tree and *of_size during some interim computations, which is against the current design of boot_get_fdt(). Currently, writing useful data into these arguments happens just before successfully returning from boot_get_fdt() and it should better stay so.
[1] https://blog.regehr.org/archives/1106
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
Changes in v2:
- s/zeroint-out/zeroing-out/ in commit description
- Link v1: https://patchwork.ozlabs.org/patch/1071586/
common/image-fdt.c | 2 -- 1 file changed, 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
But please update the comment to for the function:
* of_flat_tree and of_size are set to 0 if no fdt exists
Regards, Simon
Hi Simon,
On Wed, Apr 17, 2019 at 09:33:06PM -0700, Simon Glass wrote:
Hi Eugeniu,
On Mon, 1 Apr 2019 at 03:46, Eugeniu Rosca erosca@de.adit-jv.com wrote:
Paranoid programming [1] lies at the foundation of proper software development, but the repetitive zeroing-out of output arguments in the context of the same function rather clutters the code and inhibits further refactoring/optimization than is doing any good.
In boot_get_fdt(), we already perform zero/NULL-initialization of *of_flat_tree and *of_size at the beginning of the function, so doing the same at function error-out is redundant/superfluous.
Moreover, keeping the code unchanged might encourage the developers to update *of_flat_tree and *of_size during some interim computations, which is against the current design of boot_get_fdt(). Currently, writing useful data into these arguments happens just before successfully returning from boot_get_fdt() and it should better stay so.
[1] https://blog.regehr.org/archives/1106
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
Changes in v2:
- s/zeroint-out/zeroing-out/ in commit description
- Link v1: https://patchwork.ozlabs.org/patch/1071586/
common/image-fdt.c | 2 -- 1 file changed, 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
But please update the comment to for the function:
of_flat_tree and of_size are set to 0 if no fdt exists
Thank you very much for the review. Since the patch is part of a series and there are no other comments except this one, should I decouple it and send as v3 standalone or there is still some chance for getting feedback for the other patches (and sending an update for the whole series)?
Regards, Simon
Best regards, Eugeniu.
Hi Eugeniu,
On Thu, 18 Apr 2019 at 03:18, Eugeniu Rosca erosca@de.adit-jv.com wrote:
Hi Simon,
On Wed, Apr 17, 2019 at 09:33:06PM -0700, Simon Glass wrote:
Hi Eugeniu,
On Mon, 1 Apr 2019 at 03:46, Eugeniu Rosca erosca@de.adit-jv.com wrote:
Paranoid programming [1] lies at the foundation of proper software development, but the repetitive zeroing-out of output arguments in the context of the same function rather clutters the code and inhibits further refactoring/optimization than is doing any good.
In boot_get_fdt(), we already perform zero/NULL-initialization of *of_flat_tree and *of_size at the beginning of the function, so doing the same at function error-out is redundant/superfluous.
Moreover, keeping the code unchanged might encourage the developers to update *of_flat_tree and *of_size during some interim computations, which is against the current design of boot_get_fdt(). Currently, writing useful data into these arguments happens just before successfully returning from boot_get_fdt() and it should better stay so.
[1] https://blog.regehr.org/archives/1106
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
Changes in v2:
- s/zeroint-out/zeroing-out/ in commit description
- Link v1: https://patchwork.ozlabs.org/patch/1071586/
common/image-fdt.c | 2 -- 1 file changed, 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
But please update the comment to for the function:
of_flat_tree and of_size are set to 0 if no fdt existsThank you very much for the review. Since the patch is part of a series and there are no other comments except this one, should I decouple it and send as v3 standalone or there is still some chance for getting feedback for the other patches (and sending an update for the whole series)?
I don't think there are any hard conventions. You can certainly resend v3 of just that one patch. But I don't think anyone would mind if you sent v3 of the whole series.
Regards, Simon
Hi Eugeniu,
On Thu, 18 Apr 2019 at 03:18, Eugeniu Rosca erosca@de.adit-jv.com wrote:
Hi Simon,
On Wed, Apr 17, 2019 at 09:33:06PM -0700, Simon Glass wrote:
Hi Eugeniu,
On Mon, 1 Apr 2019 at 03:46, Eugeniu Rosca erosca@de.adit-jv.com wrote:
Paranoid programming [1] lies at the foundation of proper software development, but the repetitive zeroing-out of output arguments in the context of the same function rather clutters the code and inhibits further refactoring/optimization than is doing any good.
In boot_get_fdt(), we already perform zero/NULL-initialization of *of_flat_tree and *of_size at the beginning of the function, so doing the same at function error-out is redundant/superfluous.
Moreover, keeping the code unchanged might encourage the developers to update *of_flat_tree and *of_size during some interim computations, which is against the current design of boot_get_fdt(). Currently, writing useful data into these arguments happens just before successfully returning from boot_get_fdt() and it should better stay so.
[1] https://blog.regehr.org/archives/1106
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
Changes in v2:
- s/zeroint-out/zeroing-out/ in commit description
- Link v1: https://patchwork.ozlabs.org/patch/1071586/
common/image-fdt.c | 2 -- 1 file changed, 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
But please update the comment to for the function:
of_flat_tree and of_size are set to 0 if no fdt existsThank you very much for the review. Since the patch is part of a series and there are no other comments except this one, should I decouple it and send as v3 standalone or there is still some chance for getting feedback for the other patches (and sending an update for the whole series)?
I don't think there are any hard conventions. You can certainly resend v3 of just that one patch. But I don't think anyone would mind if you sent v3 of the whole series.
Regards, Simon
Applied to u-boot-dm, thanks!
Hi Simon,
On Wed, Apr 17, 2019 at 09:33:06PM -0700, Simon Glass wrote:
Hi Eugeniu,
On Mon, 1 Apr 2019 at 03:46, Eugeniu Rosca erosca@de.adit-jv.com wrote:
Paranoid programming [1] lies at the foundation of proper software development, but the repetitive zeroing-out of output arguments in the context of the same function rather clutters the code and inhibits further refactoring/optimization than is doing any good.
In boot_get_fdt(), we already perform zero/NULL-initialization of *of_flat_tree and *of_size at the beginning of the function, so doing the same at function error-out is redundant/superfluous.
Moreover, keeping the code unchanged might encourage the developers to update *of_flat_tree and *of_size during some interim computations, which is against the current design of boot_get_fdt(). Currently, writing useful data into these arguments happens just before successfully returning from boot_get_fdt() and it should better stay so.
[1] https://blog.regehr.org/archives/1106
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
Changes in v2:
- s/zeroint-out/zeroing-out/ in commit description
- Link v1: https://patchwork.ozlabs.org/patch/1071586/
common/image-fdt.c | 2 -- 1 file changed, 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
But please update the comment to for the function:
of_flat_tree and of_size are set to 0 if no fdt exists
Queued. Thanks.
Regards, Simon
The 'no_fdt' goto label was introduced by v2015.01 commit [0] and it had two review stages [1-2]. The *documented* purpose behind commit [0] is (excerpt from commit description):
allows both FDT and non-FDT kernels to boot by making the third parameter to the bootm/bootz optional
While [1] and [2] share the same goal, they have very different implementations: - [1] was based on a very simple 'argc' check at function error out with returning success to the caller if the third parameter was NOT passed to bootm/bootz command. This approach had the downside of returning success to the caller even in case of legitimate internal errors, which should halt booting. - [2] added the "no_fdt" label and several "goto no_fdt" statements. This allowed to report the legitimate internal errors to the caller.
IOW the major difference between [1] and [2] is: - [1] boot w/o FDT if FDT address is not passed to boot{m,z,*} - [2] give *freedom* to the developer to boot w/o FDT from any (more or less) arbitrary point in the function flow (and here comes the peculiar aspect, which looks to be a leftover from [1]) with the precondition that the 3rd argument (FDT address) is NOT provided to boot{m,z,*}. In practice, this means that only a subset of "goto no_fdt" end up booting w/o FDT while the other subset is returning an error to the caller.
This patch removes the peculiar behavior described above, such that "goto no_fdt" performs really what it tells to the developer.
The motivation of this patch is to decrease the unneeded complexity and increase the readability of boot_get_fdt().
[0] 48aead71c1ad ("fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined") [1] https://patchwork.ozlabs.org/patch/412923/ ("[U-Boot,v1] fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined") [2] https://patchwork.ozlabs.org/patch/415635/ ("[U-Boot,v2] fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined")
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com --- Changes in v2: - NA - Link v1: https://patchwork.ozlabs.org/patch/1071587/ --- common/image-fdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/image-fdt.c b/common/image-fdt.c index 1817ce6bce30..c335e7e2f220 100644 --- a/common/image-fdt.c +++ b/common/image-fdt.c @@ -489,7 +489,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch, no_fdt: ok_no_fdt = 1; error: - if (!select && ok_no_fdt) { + if (ok_no_fdt) { debug("Continuing to boot without FDT\n"); return 0; }
On Mon, 1 Apr 2019 at 03:46, Eugeniu Rosca erosca@de.adit-jv.com wrote:
The 'no_fdt' goto label was introduced by v2015.01 commit [0] and it had two review stages [1-2]. The *documented* purpose behind commit [0] is (excerpt from commit description):
allows both FDT and non-FDT kernels to boot by making the third parameter to the bootm/bootz optional
While [1] and [2] share the same goal, they have very different implementations:
- [1] was based on a very simple 'argc' check at function error out with returning success to the caller if the third parameter was NOT passed to bootm/bootz command. This approach had the downside of returning success to the caller even in case of legitimate internal errors, which should halt booting.
- [2] added the "no_fdt" label and several "goto no_fdt" statements. This allowed to report the legitimate internal errors to the caller.
IOW the major difference between [1] and [2] is:
- [1] boot w/o FDT if FDT address is not passed to boot{m,z,*}
- [2] give *freedom* to the developer to boot w/o FDT from any (more or less) arbitrary point in the function flow (and here comes the peculiar aspect, which looks to be a leftover from [1]) with the precondition that the 3rd argument (FDT address) is NOT provided to boot{m,z,*}. In practice, this means that only a subset of "goto no_fdt" end up booting w/o FDT while the other subset is returning an error to the caller.
This patch removes the peculiar behavior described above, such that "goto no_fdt" performs really what it tells to the developer.
The motivation of this patch is to decrease the unneeded complexity and increase the readability of boot_get_fdt().
[0] 48aead71c1ad ("fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined") [1] https://patchwork.ozlabs.org/patch/412923/ ("[U-Boot,v1] fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined") [2] https://patchwork.ozlabs.org/patch/415635/ ("[U-Boot,v2] fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined")
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
Changes in v2:
- NA
- Link v1: https://patchwork.ozlabs.org/patch/1071587/
common/image-fdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
Can we create a test of the boot_get_fdt() behaviour?
We have test_fit and test_vboot as possible examples, although perhaps a test that directly calls this function would be better?
Regards, Simon
diff --git a/common/image-fdt.c b/common/image-fdt.c index 1817ce6bce30..c335e7e2f220 100644 --- a/common/image-fdt.c +++ b/common/image-fdt.c @@ -489,7 +489,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch, no_fdt: ok_no_fdt = 1; error:
if (!select && ok_no_fdt) {
if (ok_no_fdt) { debug("Continuing to boot without FDT\n"); return 0; }-- 2.21.0
On Mon, 1 Apr 2019 at 03:46, Eugeniu Rosca erosca@de.adit-jv.com wrote:
The 'no_fdt' goto label was introduced by v2015.01 commit [0] and it had two review stages [1-2]. The *documented* purpose behind commit [0] is (excerpt from commit description):
allows both FDT and non-FDT kernels to boot by making the third parameter to the bootm/bootz optional
While [1] and [2] share the same goal, they have very different implementations:
- [1] was based on a very simple 'argc' check at function error out with returning success to the caller if the third parameter was NOT passed to bootm/bootz command. This approach had the downside of returning success to the caller even in case of legitimate internal errors, which should halt booting.
- [2] added the "no_fdt" label and several "goto no_fdt" statements. This allowed to report the legitimate internal errors to the caller.
IOW the major difference between [1] and [2] is:
- [1] boot w/o FDT if FDT address is not passed to boot{m,z,*}
- [2] give *freedom* to the developer to boot w/o FDT from any (more or less) arbitrary point in the function flow (and here comes the peculiar aspect, which looks to be a leftover from [1]) with the precondition that the 3rd argument (FDT address) is NOT provided to boot{m,z,*}. In practice, this means that only a subset of "goto no_fdt" end up booting w/o FDT while the other subset is returning an error to the caller.
This patch removes the peculiar behavior described above, such that "goto no_fdt" performs really what it tells to the developer.
The motivation of this patch is to decrease the unneeded complexity and increase the readability of boot_get_fdt().
[0] 48aead71c1ad ("fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined") [1] https://patchwork.ozlabs.org/patch/412923/ ("[U-Boot,v1] fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined") [2] https://patchwork.ozlabs.org/patch/415635/ ("[U-Boot,v2] fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined")
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
Changes in v2:
- NA
- Link v1: https://patchwork.ozlabs.org/patch/1071587/
common/image-fdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
Can we create a test of the boot_get_fdt() behaviour?
We have test_fit and test_vboot as possible examples, although perhaps a test that directly calls this function would be better?
Regards, Simon
Applied to u-boot-dm, thanks!
Hi Simon,
On Sun, Apr 21, 2019 at 07:38:52PM -0700, sjg@google.com wrote: [..]
Can we create a test of the boot_get_fdt() behaviour?
We have test_fit and test_vboot as possible examples, although perhaps a test that directly calls this function would be better?
On my list. Thanks.
Regards, Simon
Applied to u-boot-dm, thanks!
Increase the readability of boot_get_fdt(). No change in behavior is expected.
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com --- Changes in v2: - NA - Link v1: https://patchwork.ozlabs.org/patch/1071588/ --- common/image-fdt.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/common/image-fdt.c b/common/image-fdt.c index c335e7e2f220..68bcab85baf4 100644 --- a/common/image-fdt.c +++ b/common/image-fdt.c @@ -279,7 +279,6 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch, int fdt_noffset; #endif const char *select = NULL; - int ok_no_fdt = 0;
*of_flat_tree = NULL; *of_size = 0; @@ -487,12 +486,9 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch, return 0;
no_fdt: - ok_no_fdt = 1; + debug("Continuing to boot without FDT\n"); + return 0; error: - if (ok_no_fdt) { - debug("Continuing to boot without FDT\n"); - return 0; - } return 1; }
On Mon, 1 Apr 2019 at 03:47, Eugeniu Rosca erosca@de.adit-jv.com wrote:
Increase the readability of boot_get_fdt(). No change in behavior is expected.
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
Changes in v2:
- NA
- Link v1: https://patchwork.ozlabs.org/patch/1071588/
common/image-fdt.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Mon, 1 Apr 2019 at 03:47, Eugeniu Rosca erosca@de.adit-jv.com wrote:
Increase the readability of boot_get_fdt(). No change in behavior is expected.
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
Changes in v2:
- NA
- Link v1: https://patchwork.ozlabs.org/patch/1071588/
common/image-fdt.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot-dm, thanks!
Prepare for booting Android images which lack any DTB in the second area by using 'fdtaddr' environment variable as source/address of FDT. No functional/behavioral change expected in this patch.
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com --- Changes in v2: - Restored fdt_totalsize() check - Link v1: https://patchwork.ozlabs.org/patch/1071590/ --- common/image-fdt.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/common/image-fdt.c b/common/image-fdt.c index 68bcab85baf4..a5d8b41d0209 100644 --- a/common/image-fdt.c +++ b/common/image-fdt.c @@ -461,17 +461,16 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch, struct andr_img_hdr *hdr = buf; ulong fdt_data, fdt_len;
- if (android_image_get_second(hdr, &fdt_data, &fdt_len) != 0) - goto no_fdt; + if (!android_image_get_second(hdr, &fdt_data, &fdt_len) && + !fdt_check_header((char *)fdt_data)) { + fdt_blob = (char *)fdt_data; + if (fdt_totalsize(fdt_blob) != fdt_len) + goto error;
- fdt_blob = (char *)fdt_data; - if (fdt_check_header(fdt_blob) != 0) + debug("## Using FDT in Android image second area\n"); + } else { goto no_fdt; - - if (fdt_totalsize(fdt_blob) != fdt_len) - goto error; - - debug("## Using FDT found in Android image second area\n"); + } #endif } else { debug("## No Flattened Device Tree\n");
On Mon, 1 Apr 2019 at 03:47, Eugeniu Rosca erosca@de.adit-jv.com wrote:
Prepare for booting Android images which lack any DTB in the second area by using 'fdtaddr' environment variable as source/address of FDT. No functional/behavioral change expected in this patch.
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
Changes in v2:
- Restored fdt_totalsize() check
- Link v1: https://patchwork.ozlabs.org/patch/1071590/
common/image-fdt.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Mon, 1 Apr 2019 at 03:47, Eugeniu Rosca erosca@de.adit-jv.com wrote:
Prepare for booting Android images which lack any DTB in the second area by using 'fdtaddr' environment variable as source/address of FDT. No functional/behavioral change expected in this patch.
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
Changes in v2:
- Restored fdt_totalsize() check
- Link v1: https://patchwork.ozlabs.org/patch/1071590/
common/image-fdt.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot-dm, thanks!
Our platform doesn't store the DTB into the Android image second area, but rather copies the DTB to RAM from a dedicated dtb.img partition [0], prior to booting the Android image by calling bootm.
Similar to [1], we find it useful to just call 'bootm' and have the right DTB being passed to OS (assuming its address has been previously stored in 'fdtaddr' by calling `fdt addr <dtb-addr>`).
Booting Android with DTB from 'fdtaddr' will only occur if: - No DTB is embedded in the second area of Android image - 'fdtaddr' points to a valid DTB in RAM
[0] https://source.android.com/devices/architecture/dto/partitions [1] https://patchwork.ozlabs.org/patch/1046652/ ("Support boot Android image without address on bootm command")
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com --- Changes in v2: - Removed fdt_totalsize() computation, since FDT size verification is only relevant for blobs embedded in U-Boot-supported images - Link v1: https://patchwork.ozlabs.org/patch/1071591/ --- common/image-fdt.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/common/image-fdt.c b/common/image-fdt.c index a5d8b41d0209..3aa5ffff0f69 100644 --- a/common/image-fdt.c +++ b/common/image-fdt.c @@ -469,7 +469,15 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
debug("## Using FDT in Android image second area\n"); } else { - goto no_fdt; + fdt_addr = env_get_hex("fdtaddr", 0); + if (!fdt_addr) + goto no_fdt; + + fdt_blob = map_sysmem(fdt_addr, 0); + if (fdt_check_header(fdt_blob)) + goto no_fdt; + + debug("## Using FDT at ${fdtaddr}=Ox%lx\n", fdt_addr); } #endif } else {
Hi All, especially the Android specialists,
On Mon, Apr 01, 2019 at 12:52:52PM +0200, Eugeniu Rosca wrote:
Our platform doesn't store the DTB into the Android image second area, but rather copies the DTB to RAM from a dedicated dtb.img partition [0], prior to booting the Android image by calling bootm.
Similar to [1], we find it useful to just call 'bootm' and have the right DTB being passed to OS (assuming its address has been previously stored in 'fdtaddr' by calling `fdt addr <dtb-addr>`).
Booting Android with DTB from 'fdtaddr' will only occur if:
- No DTB is embedded in the second area of Android image
- 'fdtaddr' points to a valid DTB in RAM
[0] https://source.android.com/devices/architecture/dto/partitions [1] https://patchwork.ozlabs.org/patch/1046652/ ("Support boot Android image without address on bootm command")
FWIW, I believe this patch could make use of the "second address" [1] stored in the Android image to fulfill my use-case, even if the second area is left empty. I believe some consensus is needed about that in community.
With this alternative approach, changing the DTB RAM address would mean regenerating the Android image, so it would be somewhat less flexible.
I am curious if anybody has gone through the same use-case, as it is hard to believe I am the first one booting an image lacking the DTB in its second area.
Best regards, Eugeniu.
[1] => iminfo 4c000000
## Checking Image at 4c000000 ... Android image found kernel size: 85b9d1 kernel address: 48080000 ramdisk size: 54ddbc ramdisk addrress: 4a180000 second size: 0 second address: 48000800 tags address: 48000100 page size: 800 os_version: 1200012a (ver: 0.9.0, level: 2018.10) name: cmdline: buildvariant=userdebug
Hi Eugeniu,
On Mon, 1 Apr 2019 at 03:54, Eugeniu Rosca erosca@de.adit-jv.com wrote:
Our platform doesn't store the DTB into the Android image second area, but rather copies the DTB to RAM from a dedicated dtb.img partition [0], prior to booting the Android image by calling bootm.
Similar to [1], we find it useful to just call 'bootm' and have the right DTB being passed to OS (assuming its address has been previously stored in 'fdtaddr' by calling `fdt addr <dtb-addr>`).
Booting Android with DTB from 'fdtaddr' will only occur if:
- No DTB is embedded in the second area of Android image
- 'fdtaddr' points to a valid DTB in RAM
[0] https://source.android.com/devices/architecture/dto/partitions [1] https://patchwork.ozlabs.org/patch/1046652/ ("Support boot Android image without address on bootm command")
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
Changes in v2:
- Removed fdt_totalsize() computation, since FDT size verification is only relevant for blobs embedded in U-Boot-supported images
- Link v1: https://patchwork.ozlabs.org/patch/1071591/
common/image-fdt.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
We could really use a simple test of Android booting (not the whole process, just the setup and the different cases we support)
Regards, Simon
Hi Eugeniu,
On Mon, 1 Apr 2019 at 03:54, Eugeniu Rosca erosca@de.adit-jv.com wrote:
Our platform doesn't store the DTB into the Android image second area, but rather copies the DTB to RAM from a dedicated dtb.img partition [0], prior to booting the Android image by calling bootm.
Similar to [1], we find it useful to just call 'bootm' and have the right DTB being passed to OS (assuming its address has been previously stored in 'fdtaddr' by calling `fdt addr <dtb-addr>`).
Booting Android with DTB from 'fdtaddr' will only occur if:
- No DTB is embedded in the second area of Android image
- 'fdtaddr' points to a valid DTB in RAM
[0] https://source.android.com/devices/architecture/dto/partitions [1] https://patchwork.ozlabs.org/patch/1046652/ ("Support boot Android image without address on bootm command")
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
Changes in v2:
- Removed fdt_totalsize() computation, since FDT size verification is only relevant for blobs embedded in U-Boot-supported images
- Link v1: https://patchwork.ozlabs.org/patch/1071591/
common/image-fdt.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
We could really use a simple test of Android booting (not the whole process, just the setup and the different cases we support)
Regards, Simon
Applied to u-boot-dm, thanks!
participants (3)
-
Eugeniu Rosca -
Simon Glass -
sjg＠google.com