[U-Boot] [PATCH v2 0/5] boot_get_fdt: clean up and use 'fdtaddr' as fallback for Android

[U-Boot] [PATCH v2 0/5] boot_get_fdt: clean up and use 'fdtaddr' as fallback for Android

[Eugeniu Rosca]

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(-)

-- 
2.21.0

[U-Boot] [PATCH v2 1/5] fdt: boot_get_fdt: remove redundant zeroing out

[Eugeniu Rosca]

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;
-- 
2.21.0

[U-Boot] [PATCH v2 2/5] fdt: boot_get_fdt: really boot w/o FDT when "goto no_fdt"

[Eugeniu Rosca]

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;
 	}
-- 
2.21.0

[U-Boot] [PATCH v2 3/5] fdt: boot_get_fdt: simplify no_fdt handling (non-functional)

[Eugeniu Rosca]

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;
 }
 
-- 
2.21.0

[U-Boot] [PATCH v2 4/5] fdt: boot_get_fdt: android: compress handling (non-functional)

[Eugeniu Rosca]

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");
-- 
2.21.0

[U-Boot] [PATCH v2 5/5] fdt: boot_get_fdt: android: use ENV 'fdtaddr' as fallback

[Eugeniu Rosca]

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 {
-- 
2.21.0

[U-Boot] [PATCH v2 5/5] fdt: boot_get_fdt: android: use ENV 'fdtaddr' as fallback

[Eugeniu Rosca]

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

[U-Boot] [PATCH v2 1/5] fdt: boot_get_fdt: remove redundant zeroing out

[Simon Glass]

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

[U-Boot] [PATCH v2 1/5] fdt: boot_get_fdt: remove redundant zeroing out

[Eugeniu Rosca]

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.

[U-Boot] [PATCH v2 1/5] fdt: boot_get_fdt: remove redundant zeroing out

[Simon Glass]

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 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)?

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

[U-Boot] [PATCH v2 2/5] fdt: boot_get_fdt: really boot w/o FDT when "goto no_fdt"

[Simon Glass]

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
>

[U-Boot] [PATCH v2 3/5] fdt: boot_get_fdt: simplify no_fdt handling (non-functional)

[Simon Glass]

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>

[U-Boot] [PATCH v2 4/5] fdt: boot_get_fdt: android: compress handling (non-functional)

[Simon Glass]

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>

[U-Boot] [PATCH v2 5/5] fdt: boot_get_fdt: android: use ENV 'fdtaddr' as fallback

[Simon Glass]

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

[U-Boot] [PATCH v2 5/5] fdt: boot_get_fdt: android: use ENV 'fdtaddr' as fallback

[sjg at google.com]

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!

[U-Boot] [PATCH v2 4/5] fdt: boot_get_fdt: android: compress handling (non-functional)

[sjg at google.com]

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!

[U-Boot] [PATCH v2 3/5] fdt: boot_get_fdt: simplify no_fdt handling (non-functional)

[sjg at google.com]

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!

[U-Boot] [PATCH v2 2/5] fdt: boot_get_fdt: really boot w/o FDT when "goto no_fdt"

[sjg at google.com]

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!

[U-Boot] [PATCH v2 1/5] fdt: boot_get_fdt: remove redundant zeroing out

[sjg at google.com]

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 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)?

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!

[U-Boot] [PATCH v2 1/5] fdt: boot_get_fdt: remove redundant zeroing out

[Eugeniu Rosca]

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

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH v2 2/5] fdt: boot_get_fdt: really boot w/o FDT when "goto no_fdt"

[Eugeniu Rosca]

Hi Simon,

On Sun, Apr 21, 2019 at 07:38:52PM -0700, sjg at 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!

-- 
Best Regards,
Eugeniu.