Re: [U-Boot] [PATCH 19/26] mkimage: Support adding device tree files to a FIT

[Simon Glass]

Hi Tom,

On 11 February 2016 at 09:36, Tom Rini  wrote:
> On Thu, Jan 28, 2016 at 09:39:39AM -0700, Simon Glass wrote:
>
>> To make the auto-FIT feature useful we need to be able to provide a list of
>> device tree files on the command line for mkimage to add into the FIT. Add
>> support for this feature.
> [snip]
>> +.BI "\-b
>> +Specifies that the following arguments are device tree binary files (.dtb).
>> +
>> +.TP
>>  .BI "\-c [" "comment" "]"
>>  Specifies a comment to be added when signing. This is typically a useful
>>  message which describes how the image was signed or some other useful
>> @@ -125,6 +129,10 @@ This can be used to sign images with additional keys 
>> after initial image
>>  creation.
>>
>>  .TP
>> +.BI "\-i
>> +Specifies that the following arguments are image files.
>> +
>> +.TP
>>  .BI "\-k [" "key_directory" "]"
>>  Specifies the directory containing keys to use for signing. This directory
>>  should contain a private key file .key for use with signing and a
>> @@ -191,6 +199,15 @@ is required.
>>  .br
>>  .B -c """Kernel 4.4 image for production devices""" -d vmlinuz kernel.itb
>>  .fi
>> +.P
>> +Create a FIT image containing a kernel and some device tree files, using
>> +automatic mode. No .its file is required.
>> +.nf
>> +.B mkimage -f auto -A arm -O linux -T kernel -C none -a 43e0 -e 0 
>> +.br
>> +.B -c """Kernel 4.4 image for production devices""" -d vmlinuz 
>> +.B -b rk3288-firefly.dts rk3288-jerry.dts -i kernel.itb
>> +.fi
>
> OK, I'm confused in a few ways here.  First, we don't need quotes around
> the space separated list of dts files?

No, the -b option introduces the list, which can be as long as needed.
I want to make it possible to specify a list easily.

> Second, we need dts not dtb?

Eek, no, it should be dtb.

> Third, I think this would be a better example if (as I suspect we need
> really) it was a full path to the output rather than assuming that we've
> copied the files to $PWD.

OK

> Fourth, -i is inputs or outputs?  The help
> reads like inputs and is where I assume we would say vmlinuz here and
> not use -d and then end with our output of kernel.itb.  Thanks!

-i introduces the image file, whic his an output. The -d provides the
datafile, which is the file that is is put inside the FIT.

Maybe we can change things to make this a bit clearer. We could
perhaps have an option to end the list of dtb files, instead of the
option to signal the output file is next? Or maybe we can avoid -i
altogether since the last arg should be the output file.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 19/26] mkimage: Support adding device tree files to a FIT

[Tom Rini]

On Fri, Feb 12, 2016 at 08:54:09AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On 11 February 2016 at 09:36, Tom Rini  wrote:
> > On Thu, Jan 28, 2016 at 09:39:39AM -0700, Simon Glass wrote:
> >
> >> To make the auto-FIT feature useful we need to be able to provide a list of
> >> device tree files on the command line for mkimage to add into the FIT. Add
> >> support for this feature.
> > [snip]
> >> +.BI "\-b
> >> +Specifies that the following arguments are device tree binary files 
> >> (.dtb).
> >> +
> >> +.TP
> >>  .BI "\-c [" "comment" "]"
> >>  Specifies a comment to be added when signing. This is typically a useful
> >>  message which describes how the image was signed or some other useful
> >> @@ -125,6 +129,10 @@ This can be used to sign images with additional keys 
> >> after initial image
> >>  creation.
> >>
> >>  .TP
> >> +.BI "\-i
> >> +Specifies that the following arguments are image files.
> >> +
> >> +.TP
> >>  .BI "\-k [" "key_directory" "]"
> >>  Specifies the directory containing keys to use for signing. This directory
> >>  should contain a private key file .key for use with signing and a
> >> @@ -191,6 +199,15 @@ is required.
> >>  .br
> >>  .B -c """Kernel 4.4 image for production devices""" -d vmlinuz kernel.itb
> >>  .fi
> >> +.P
> >> +Create a FIT image containing a kernel and some device tree files, using
> >> +automatic mode. No .its file is required.
> >> +.nf
> >> +.B mkimage -f auto -A arm -O linux -T kernel -C none -a 43e0 -e 0 
> >> +.br
> >> +.B -c """Kernel 4.4 image for production devices""" -d vmlinuz 
> >> +.B -b rk3288-firefly.dts rk3288-jerry.dts -i kernel.itb
> >> +.fi
> >
> > OK, I'm confused in a few ways here.  First, we don't need quotes around
> > the space separated list of dts files?
> 
> No, the -b option introduces the list, which can be as long as needed.
> I want to make it possible to specify a list easily.

OK.  I don't see how that works in the code, but if that's the intent
and it works, OK :)

> > Second, we need dts not dtb?
> 
> Eek, no, it should be dtb.
> 
> > Third, I think this would be a better example if (as I suspect we need
> > really) it was a full path to the output rather than assuming that we've
> > copied the files to $PWD.
> 
> OK
> 
> > Fourth, -i is inputs or outputs?  The help
> > reads like inputs and is where I assume we would say vmlinuz here and
> > not use -d and then end with our output of kernel.itb.  Thanks!
> 
> -i introduces the image file, whic his an output. The -d provides the
> datafile, which is the file that is is put inside the FIT.
> 
> Maybe we can change things to make this a bit clearer. We could
> perhaps have an option to end the list of dtb files, instead of the
> option to signal the output file is next? Or maybe we can avoid -i
> altogether since the last arg should be the output file.

Yes, I don't think we need -i at all.  The last argument is the output,
always, and -f auto instead of -f some-file.its is all we need to know
it's a FIT output.  And, ah, so I guess this is how -b is implemented
then.  Yes, this seems rather awkward.  Maybe we instead implement a
sometimes optional (ie previous use cases) -o flag to say output file is
... and also make sure it's clear in the man page at least that -b
consumes following arguments as the DT list until another flag is
passed?

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 19/26] mkimage: Support adding device tree files to a FIT

[Tom Rini]

On Thu, Jan 28, 2016 at 09:39:39AM -0700, Simon Glass wrote:

> To make the auto-FIT feature useful we need to be able to provide a list of
> device tree files on the command line for mkimage to add into the FIT. Add
> support for this feature.
[snip]
> +.BI "\-b
> +Specifies that the following arguments are device tree binary files (.dtb).
> +
> +.TP
>  .BI "\-c [" "comment" "]"
>  Specifies a comment to be added when signing. This is typically a useful
>  message which describes how the image was signed or some other useful
> @@ -125,6 +129,10 @@ This can be used to sign images with additional keys 
> after initial image
>  creation.
>  
>  .TP
> +.BI "\-i
> +Specifies that the following arguments are image files.
> +
> +.TP
>  .BI "\-k [" "key_directory" "]"
>  Specifies the directory containing keys to use for signing. This directory
>  should contain a private key file .key for use with signing and a
> @@ -191,6 +199,15 @@ is required.
>  .br
>  .B -c """Kernel 4.4 image for production devices""" -d vmlinuz kernel.itb
>  .fi
> +.P
> +Create a FIT image containing a kernel and some device tree files, using
> +automatic mode. No .its file is required.
> +.nf
> +.B mkimage -f auto -A arm -O linux -T kernel -C none -a 43e0 -e 0 
> +.br
> +.B -c """Kernel 4.4 image for production devices""" -d vmlinuz 
> +.B -b rk3288-firefly.dts rk3288-jerry.dts -i kernel.itb
> +.fi

OK, I'm confused in a few ways here.  First, we don't need quotes around
the space separated list of dts files?  Second, we need dts not dtb?
Third, I think this would be a better example if (as I suspect we need
really) it was a full path to the output rather than assuming that we've
copied the files to $PWD.  Fourth, -i is inputs or outputs?  The help
reads like inputs and is where I assume we would say vmlinuz here and
not use -d and then end with our output of kernel.itb.  Thanks!

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 19/26] mkimage: Support adding device tree files to a FIT

[Simon Glass]

To make the auto-FIT feature useful we need to be able to provide a list of
device tree files on the command line for mkimage to add into the FIT. Add
support for this feature.

So far there is no support for hashing or verified boot using this method.
For those cases, a .its file must still be provided.

Signed-off-by: Simon Glass 
---

 doc/mkimage.1 | 17 ++
 tools/fit_image.c | 96 ++-
 tools/imagetool.h |  9 ++
 tools/mkimage.c   | 38 +-
 4 files changed, 151 insertions(+), 9 deletions(-)

diff --git a/doc/mkimage.1 b/doc/mkimage.1
index 146c114..b1b45d7 100644
--- a/doc/mkimage.1
+++ b/doc/mkimage.1
@@ -97,6 +97,10 @@ Set XIP (execute in place) flag.
 .B Create FIT image:
 
 .TP
+.BI "\-b
+Specifies that the following arguments are device tree binary files (.dtb).
+
+.TP
 .BI "\-c [" "comment" "]"
 Specifies a comment to be added when signing. This is typically a useful
 message which describes how the image was signed or some other useful
@@ -125,6 +129,10 @@ This can be used to sign images with additional keys after 
initial image
 creation.
 
 .TP
+.BI "\-i
+Specifies that the following arguments are image files.
+
+.TP
 .BI "\-k [" "key_directory" "]"
 Specifies the directory containing keys to use for signing. This directory
 should contain a private key file .key for use with signing and a
@@ -191,6 +199,15 @@ is required.
 .br
 .B -c """Kernel 4.4 image for production devices""" -d vmlinuz kernel.itb
 .fi
+.P
+Create a FIT image containing a kernel and some device tree files, using
+automatic mode. No .its file is required.
+.nf
+.B mkimage -f auto -A arm -O linux -T kernel -C none -a 43e0 -e 0 
+.br
+.B -c """Kernel 4.4 image for production devices""" -d vmlinuz 
+.B -b rk3288-firefly.dts rk3288-jerry.dts -i kernel.itb
+.fi
 
 .SH HOMEPAGE
 http://www.denx.de/wiki/U-Boot/WebHome
diff --git a/tools/fit_image.c b/tools/fit_image.c
index 343de60..0d8007b 100644
--- a/tools/fit_image.c
+++ b/tools/fit_image.c
@@ -77,6 +77,7 @@ err_keydest:
  */
 static int fit_calc_size(struct image_tool_params *params)
 {
+   struct content_info *cont;
int size, total_size;
 
size = imagetool_get_filesize(params, params->datafile);
@@ -84,8 +85,14 @@ static int fit_calc_size(struct image_tool_params *params)
return -1;
 
total_size = size;
+   for (cont = params->content_head; cont; cont = cont->next) {
+   size = imagetool_get_filesize(params, cont->fname);
+   if (size < 0)
+   return -1;
 
-   /* TODO(s...@chromium.org): Add in the size of any other files */
+   /* Add space for properties */
+   total_size += size + 300;
+   }
 
/* Add plenty of space for headers, properties, nodes, etc. */
total_size += 4096;
@@ -141,15 +148,40 @@ static int fdt_property_strf(void *fdt, const char *name, 
const char *fmt, ...)
return fdt_property_string(fdt, name, str);
 }
 
+static void get_basename(char *str, int size, const char *fname)
+{
+   const char *p, *start, *end;
+   int len;
+
+   /*
+* Use the base name as the 'name' field. So for example:
+*
+* "arch/arm/dts/sun7i-a20-bananapro.dtb"
+* becomes "sun7i-a20-bananapro"
+*/
+   p = strrchr(fname, '/');
+   start = p ? p + 1 : fname;
+   p = strrchr(fname, '.');
+   end = p ? p : fname + strlen(fname);
+   len = end - start;
+   if (len >= size)
+   len = size - 1;
+   memcpy(str, start, len);
+   str[len] = '\0';
+}
+
 /**
  * fit_write_images() - Write out a list of images to the FIT
  *
- * Include the main image (params->datafile).
+ * We always include the main image (params->datafile). If there are device
+ * tree files, we include an fdt@ node for each of those too.
  */
 static int fit_write_images(struct image_tool_params *params, char *fdt)
 {
+   struct content_info *cont;
const char *typename;
char str[100];
+   int upto;
int ret;
 
fdt_begin_node(fdt, "images");
@@ -176,6 +208,27 @@ static int fit_write_images(struct image_tool_params 
*params, char *fdt)
return ret;
fdt_end_node(fdt);
 
+   /* Now the device tree files if available */
+   upto = 0;
+   for (cont = params->content_head; cont; cont = cont->next) {
+   if (cont->type != IH_TYPE_FLATDT)
+   continue;
+   snprintf(str, sizeof(str), "%s@%d", FIT_FDT_PROP, ++upto);
+   fdt_begin_node(fdt, str);
+
+   get_basename(str, sizeof(str), cont->fname);
+   fdt_property_string(fdt, "description", str);
+   ret = fdt_property_file(params, fdt, "data", cont->fname);
+   if (ret)
+   return ret;
+   fdt_property_string(fdt, "type",