[PATCH v2 0/2] tools: mkimage: cleanups + allow to create legacy image with type flat_dt

[PATCH v2 0/2] tools: mkimage: cleanups + allow to create legacy image with type flat_dt

[Marc Kleine-Budde]

Hello,

the first patch cleans up the error message output if struct
image_type_params::set_header is set, the other adds support for
creating legacy images with type flat_dt.

regards,
Marc

Changes since v1:
- introduce new image type (suggested by Sean Anderson)
  instead of adding new cmd line parameter

[PATCH v2 1/2] tools: mkimage: don't print error message "Success" in case of failure

[Marc Kleine-Budde]

In case there's no struct image_type_params::set_header callback, no
"errno" will be set. Don't fail with an error message, followed by
"Success". Remove the printing of the human readable "errno" value.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 tools/mkimage.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/mkimage.c b/tools/mkimage.c
index 30c6df77081f..35a6b1fb799c 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -760,8 +760,8 @@ int main(int argc, char **argv)
 	if (tparams->set_header)
 		tparams->set_header (ptr, &sbuf, ifd, &params);
 	else {
-		fprintf (stderr, "%s: Can't set header for %s: %s\n",
-			params.cmdname, tparams->name, strerror(errno));
+		fprintf (stderr, "%s: Can't set header for %s\n",
+			params.cmdname, tparams->name);
 		exit (EXIT_FAILURE);
 	}
 
-- 
2.35.1

[PATCH v2 2/2] tools: mkimage: add new image type "flat_dt_legacy"

[Marc Kleine-Budde]

If the user select the image type "flat_dt" a FIT image will be build.
This breaks the legacy use case of putting a Flat Device Tree into a
legacy u-boot image.

Add a new image type "flat_dt_legacy" to build a legacy u-boot image
with a "flat_dt" type.

Link: https://lore.kernel.org/all/20221028155205.ojw6tcso2fofgnhm@pengutronix.de
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 boot/image.c          |  1 +
 include/image.h       |  1 +
 tools/default_image.c | 11 +++++++++--
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/boot/image.c b/boot/image.c
index 9f95b3260a80..9d7e57dee985 100644
--- a/boot/image.c
+++ b/boot/image.c
@@ -180,6 +180,7 @@ static const table_entry_t uimage_type[] = {
 	{	IH_TYPE_COPRO, "copro", "Coprocessor Image"},
 	{	IH_TYPE_SUNXI_EGON, "sunxi_egon",  "Allwinner eGON Boot Image" },
 	{	IH_TYPE_SUNXI_TOC0, "sunxi_toc0",  "Allwinner TOC0 Boot Image" },
+	{	IH_TYPE_FLATDT_LEGACY, "flat_dt_legacy", "Flat Device Tree legacy Image", },
 	{	-1,		    "",		  "",			},
 };
 
diff --git a/include/image.h b/include/image.h
index d7d6a3fe5b81..e578e2c5f1fd 100644
--- a/include/image.h
+++ b/include/image.h
@@ -229,6 +229,7 @@ enum {
 	IH_TYPE_COPRO,			/* Coprocessor Image for remoteproc*/
 	IH_TYPE_SUNXI_EGON,		/* Allwinner eGON Boot Image */
 	IH_TYPE_SUNXI_TOC0,		/* Allwinner TOC0 Boot Image */
+	IH_TYPE_FLATDT_LEGACY,		/* Binary Flat Device Tree Blob	in a legacy image */
 
 	IH_TYPE_COUNT,			/* Number of image types */
 };
diff --git a/tools/default_image.c b/tools/default_image.c
index 4a067e65862e..3b49f0d70e29 100644
--- a/tools/default_image.c
+++ b/tools/default_image.c
@@ -27,7 +27,8 @@ static struct legacy_img_hdr header;
 static int image_check_image_types(uint8_t type)
 {
 	if (((type > IH_TYPE_INVALID) && (type < IH_TYPE_FLATDT)) ||
-	    (type == IH_TYPE_KERNEL_NOLOAD) || (type == IH_TYPE_FIRMWARE_IVT))
+	    (type == IH_TYPE_KERNEL_NOLOAD) || (type == IH_TYPE_FIRMWARE_IVT) ||
+	    (type == IH_TYPE_FLATDT_LEGACY))
 		return EXIT_SUCCESS;
 	else
 		return EXIT_FAILURE;
@@ -94,6 +95,7 @@ static void image_set_header(void *ptr, struct stat *sbuf, int ifd,
 	uint32_t imagesize;
 	uint32_t ep;
 	uint32_t addr;
+	int type;
 	struct legacy_img_hdr *hdr = (struct legacy_img_hdr *)ptr;
 
 	checksum = crc32(0,
@@ -113,6 +115,11 @@ static void image_set_header(void *ptr, struct stat *sbuf, int ifd,
 	else
 		imagesize = sbuf->st_size - sizeof(struct legacy_img_hdr);
 
+	if (params->type == IH_TYPE_FLATDT_LEGACY)
+		type = IH_TYPE_FLATDT;
+	else
+		type = params->type;
+
 	if (params->os == IH_OS_TEE) {
 		addr = optee_image_get_load_addr(hdr);
 		ep = optee_image_get_entry_point(hdr);
@@ -127,7 +134,7 @@ static void image_set_header(void *ptr, struct stat *sbuf, int ifd,
 	image_set_dcrc(hdr, checksum);
 	image_set_os(hdr, params->os);
 	image_set_arch(hdr, params->arch);
-	image_set_type(hdr, params->type);
+	image_set_type(hdr, type);
 	image_set_comp(hdr, params->comp);
 
 	image_set_name(hdr, params->imagename);
-- 
2.35.1

Re: [PATCH v2 1/2] tools: mkimage: don't print error message "Success" in case of failure

[Simon Glass]

On Mon, 31 Oct 2022 at 08:51, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> In case there's no struct image_type_params::set_header callback, no
> "errno" will be set. Don't fail with an error message, followed by
> "Success". Remove the printing of the human readable "errno" value.
>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  tools/mkimage.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 1/2] tools: mkimage: don't print error message "Success" in case of failure

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 885 bytes --]

On 31.10.2022 13:27:10, Simon Glass wrote:
> On Mon, 31 Oct 2022 at 08:51, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >
> > In case there's no struct image_type_params::set_header callback, no
> > "errno" will be set. Don't fail with an error message, followed by
> > "Success". Remove the printing of the human readable "errno" value.
> >
> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > ---
> >  tools/mkimage.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>

Has this patch already been applied?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] tools: mkimage: add new image type "flat_dt_legacy"

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3709 bytes --]

On 31.10.2022 15:51:21, Marc Kleine-Budde wrote:
> If the user select the image type "flat_dt" a FIT image will be build.
> This breaks the legacy use case of putting a Flat Device Tree into a
> legacy u-boot image.
> 
> Add a new image type "flat_dt_legacy" to build a legacy u-boot image
> with a "flat_dt" type.
> 
> Link: https://lore.kernel.org/all/20221028155205.ojw6tcso2fofgnhm@pengutronix.de
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

Sean, what about this approach compared to adding the new command line
parameter?

Marc

> ---
>  boot/image.c          |  1 +
>  include/image.h       |  1 +
>  tools/default_image.c | 11 +++++++++--
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/boot/image.c b/boot/image.c
> index 9f95b3260a80..9d7e57dee985 100644
> --- a/boot/image.c
> +++ b/boot/image.c
> @@ -180,6 +180,7 @@ static const table_entry_t uimage_type[] = {
>  	{	IH_TYPE_COPRO, "copro", "Coprocessor Image"},
>  	{	IH_TYPE_SUNXI_EGON, "sunxi_egon",  "Allwinner eGON Boot Image" },
>  	{	IH_TYPE_SUNXI_TOC0, "sunxi_toc0",  "Allwinner TOC0 Boot Image" },
> +	{	IH_TYPE_FLATDT_LEGACY, "flat_dt_legacy", "Flat Device Tree legacy Image", },
>  	{	-1,		    "",		  "",			},
>  };
>  
> diff --git a/include/image.h b/include/image.h
> index d7d6a3fe5b81..e578e2c5f1fd 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -229,6 +229,7 @@ enum {
>  	IH_TYPE_COPRO,			/* Coprocessor Image for remoteproc*/
>  	IH_TYPE_SUNXI_EGON,		/* Allwinner eGON Boot Image */
>  	IH_TYPE_SUNXI_TOC0,		/* Allwinner TOC0 Boot Image */
> +	IH_TYPE_FLATDT_LEGACY,		/* Binary Flat Device Tree Blob	in a legacy image */
>  
>  	IH_TYPE_COUNT,			/* Number of image types */
>  };
> diff --git a/tools/default_image.c b/tools/default_image.c
> index 4a067e65862e..3b49f0d70e29 100644
> --- a/tools/default_image.c
> +++ b/tools/default_image.c
> @@ -27,7 +27,8 @@ static struct legacy_img_hdr header;
>  static int image_check_image_types(uint8_t type)
>  {
>  	if (((type > IH_TYPE_INVALID) && (type < IH_TYPE_FLATDT)) ||
> -	    (type == IH_TYPE_KERNEL_NOLOAD) || (type == IH_TYPE_FIRMWARE_IVT))
> +	    (type == IH_TYPE_KERNEL_NOLOAD) || (type == IH_TYPE_FIRMWARE_IVT) ||
> +	    (type == IH_TYPE_FLATDT_LEGACY))
>  		return EXIT_SUCCESS;
>  	else
>  		return EXIT_FAILURE;
> @@ -94,6 +95,7 @@ static void image_set_header(void *ptr, struct stat *sbuf, int ifd,
>  	uint32_t imagesize;
>  	uint32_t ep;
>  	uint32_t addr;
> +	int type;
>  	struct legacy_img_hdr *hdr = (struct legacy_img_hdr *)ptr;
>  
>  	checksum = crc32(0,
> @@ -113,6 +115,11 @@ static void image_set_header(void *ptr, struct stat *sbuf, int ifd,
>  	else
>  		imagesize = sbuf->st_size - sizeof(struct legacy_img_hdr);
>  
> +	if (params->type == IH_TYPE_FLATDT_LEGACY)
> +		type = IH_TYPE_FLATDT;
> +	else
> +		type = params->type;
> +
>  	if (params->os == IH_OS_TEE) {
>  		addr = optee_image_get_load_addr(hdr);
>  		ep = optee_image_get_entry_point(hdr);
> @@ -127,7 +134,7 @@ static void image_set_header(void *ptr, struct stat *sbuf, int ifd,
>  	image_set_dcrc(hdr, checksum);
>  	image_set_os(hdr, params->os);
>  	image_set_arch(hdr, params->arch);
> -	image_set_type(hdr, params->type);
> +	image_set_type(hdr, type);
>  	image_set_comp(hdr, params->comp);
>  
>  	image_set_name(hdr, params->imagename);
> -- 
> 2.35.1
> 
> 
> 

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] tools: mkimage: don't print error message "Success" in case of failure

[Simon Glass]

+Tom Rini

Hi Marc,

On Wed, 16 Nov 2022 at 03:42, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 31.10.2022 13:27:10, Simon Glass wrote:
> > On Mon, 31 Oct 2022 at 08:51, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > >
> > > In case there's no struct image_type_params::set_header callback, no
> > > "errno" will be set. Don't fail with an error message, followed by
> > > "Success". Remove the printing of the human readable "errno" value.
> > >
> > > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > > ---
> > >  tools/mkimage.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Has this patch already been applied?

No, you can see the status here:

https://patchwork.ozlabs.org/project/uboot/patch/20221031145121.236877-2-mkl@pengutronix.de/

Regards,
SImon

Re: [PATCH v2 2/2] tools: mkimage: add new image type "flat_dt_legacy"

[Sean Anderson]

On 11/16/22 05:49, Marc Kleine-Budde wrote:
> On 31.10.2022 15:51:21, Marc Kleine-Budde wrote:
>> If the user select the image type "flat_dt" a FIT image will be build.
>> This breaks the legacy use case of putting a Flat Device Tree into a
>> legacy u-boot image.
>>
>> Add a new image type "flat_dt_legacy" to build a legacy u-boot image
>> with a "flat_dt" type.
>>
>> Link: https://lore.kernel.org/all/20221028155205.ojw6tcso2fofgnhm@pengutronix.de
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> Sean, what about this approach compared to adding the new command line
> parameter?
> 

This is good. Maybe we should just name it fdt?

--Sean

>> ---
>>   boot/image.c          |  1 +
>>   include/image.h       |  1 +
>>   tools/default_image.c | 11 +++++++++--
>>   3 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/boot/image.c b/boot/image.c
>> index 9f95b3260a80..9d7e57dee985 100644
>> --- a/boot/image.c
>> +++ b/boot/image.c
>> @@ -180,6 +180,7 @@ static const table_entry_t uimage_type[] = {
>>   	{	IH_TYPE_COPRO, "copro", "Coprocessor Image"},
>>   	{	IH_TYPE_SUNXI_EGON, "sunxi_egon",  "Allwinner eGON Boot Image" },
>>   	{	IH_TYPE_SUNXI_TOC0, "sunxi_toc0",  "Allwinner TOC0 Boot Image" },
>> +	{	IH_TYPE_FLATDT_LEGACY, "flat_dt_legacy", "Flat Device Tree legacy Image", },
>>   	{	-1,		    "",		  "",			},
>>   };
>>   
>> diff --git a/include/image.h b/include/image.h
>> index d7d6a3fe5b81..e578e2c5f1fd 100644
>> --- a/include/image.h
>> +++ b/include/image.h
>> @@ -229,6 +229,7 @@ enum {
>>   	IH_TYPE_COPRO,			/* Coprocessor Image for remoteproc*/
>>   	IH_TYPE_SUNXI_EGON,		/* Allwinner eGON Boot Image */
>>   	IH_TYPE_SUNXI_TOC0,		/* Allwinner TOC0 Boot Image */
>> +	IH_TYPE_FLATDT_LEGACY,		/* Binary Flat Device Tree Blob	in a legacy image */
>>   
>>   	IH_TYPE_COUNT,			/* Number of image types */
>>   };
>> diff --git a/tools/default_image.c b/tools/default_image.c
>> index 4a067e65862e..3b49f0d70e29 100644
>> --- a/tools/default_image.c
>> +++ b/tools/default_image.c
>> @@ -27,7 +27,8 @@ static struct legacy_img_hdr header;
>>   static int image_check_image_types(uint8_t type)
>>   {
>>   	if (((type > IH_TYPE_INVALID) && (type < IH_TYPE_FLATDT)) ||
>> -	    (type == IH_TYPE_KERNEL_NOLOAD) || (type == IH_TYPE_FIRMWARE_IVT))
>> +	    (type == IH_TYPE_KERNEL_NOLOAD) || (type == IH_TYPE_FIRMWARE_IVT) ||
>> +	    (type == IH_TYPE_FLATDT_LEGACY))
>>   		return EXIT_SUCCESS;
>>   	else
>>   		return EXIT_FAILURE;
>> @@ -94,6 +95,7 @@ static void image_set_header(void *ptr, struct stat *sbuf, int ifd,
>>   	uint32_t imagesize;
>>   	uint32_t ep;
>>   	uint32_t addr;
>> +	int type;
>>   	struct legacy_img_hdr *hdr = (struct legacy_img_hdr *)ptr;
>>   
>>   	checksum = crc32(0,
>> @@ -113,6 +115,11 @@ static void image_set_header(void *ptr, struct stat *sbuf, int ifd,
>>   	else
>>   		imagesize = sbuf->st_size - sizeof(struct legacy_img_hdr);
>>   
>> +	if (params->type == IH_TYPE_FLATDT_LEGACY)
>> +		type = IH_TYPE_FLATDT;
>> +	else
>> +		type = params->type;
>> +
>>   	if (params->os == IH_OS_TEE) {
>>   		addr = optee_image_get_load_addr(hdr);
>>   		ep = optee_image_get_entry_point(hdr);
>> @@ -127,7 +134,7 @@ static void image_set_header(void *ptr, struct stat *sbuf, int ifd,
>>   	image_set_dcrc(hdr, checksum);
>>   	image_set_os(hdr, params->os);
>>   	image_set_arch(hdr, params->arch);
>> -	image_set_type(hdr, params->type);
>> +	image_set_type(hdr, type);
>>   	image_set_comp(hdr, params->comp);
>>   
>>   	image_set_name(hdr, params->imagename);
>> -- 
>> 2.35.1
>>
>>
>>
> 

Re: [PATCH v2 2/2] tools: mkimage: add new image type "flat_dt_legacy"

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2092 bytes --]

On 17.11.2022 00:01:19, Sean Anderson wrote:
> On 11/16/22 05:49, Marc Kleine-Budde wrote:
> > On 31.10.2022 15:51:21, Marc Kleine-Budde wrote:
> > > If the user select the image type "flat_dt" a FIT image will be build.
> > > This breaks the legacy use case of putting a Flat Device Tree into a
> > > legacy u-boot image.
> > > 
> > > Add a new image type "flat_dt_legacy" to build a legacy u-boot image
> > > with a "flat_dt" type.
> > > 
> > > Link: https://lore.kernel.org/all/20221028155205.ojw6tcso2fofgnhm@pengutronix.de
> > > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > 
> > Sean, what about this approach compared to adding the new command line
> > parameter?
> > 
> 
> This is good. Maybe we should just name it fdt?

There is already the "flat_dt" in boot/image.c, which is the "new" image
type:

> 	{	IH_TYPE_FLATDT,        "flat_dt",        "Flat Device Tree", },
[...]
>+      {	IH_TYPE_FLATDT_LEGACY, "flat_dt_legacy", "Flat Device Tree legacy Image", },

I need a legacy image, where the type is set to IH_TYPE_FLATDT. Maybe
"legacy_flat_dt" or "legacy_fdt" would be an appropriate name, too.

As this string is user facing I think it should have "legacy" in it.
I think "flat_dt" and "fdt" is just too similar and the user can't see
the difference, IMHO.

> > > --- a/boot/image.c
> > > +++ b/boot/image.c
> > > @@ -180,6 +180,7 @@ static const table_entry_t uimage_type[] = {
> > >   	{	IH_TYPE_COPRO, "copro", "Coprocessor Image"},
> > >   	{	IH_TYPE_SUNXI_EGON, "sunxi_egon",  "Allwinner eGON Boot Image" },
> > >   	{	IH_TYPE_SUNXI_TOC0, "sunxi_toc0",  "Allwinner TOC0 Boot Image" },
> > > +         {	IH_TYPE_FLATDT_LEGACY, "flat_dt_legacy", "Flat Device Tree legacy Image", },
> > >   	{	-1,		    "",		  "",			},
> > >   };

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] tools: mkimage: add new image type "flat_dt_legacy"

[Sean Anderson]

On 11/17/22 06:30, Marc Kleine-Budde wrote:
> On 17.11.2022 00:01:19, Sean Anderson wrote:
>> On 11/16/22 05:49, Marc Kleine-Budde wrote:
>>> On 31.10.2022 15:51:21, Marc Kleine-Budde wrote:
>>>> If the user select the image type "flat_dt" a FIT image will be build.
>>>> This breaks the legacy use case of putting a Flat Device Tree into a
>>>> legacy u-boot image.
>>>>
>>>> Add a new image type "flat_dt_legacy" to build a legacy u-boot image
>>>> with a "flat_dt" type.
>>>>
>>>> Link: https://lore.kernel.org/all/20221028155205.ojw6tcso2fofgnhm@pengutronix.de
>>>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>>>
>>> Sean, what about this approach compared to adding the new command line
>>> parameter?
>>>
>>
>> This is good. Maybe we should just name it fdt?
> 
> There is already the "flat_dt" in boot/image.c, which is the "new" image
> type:

>> 	{	IH_TYPE_FLATDT,        "flat_dt",        "Flat Device Tree", },
> [...]
>> +      {	IH_TYPE_FLATDT_LEGACY, "flat_dt_legacy", "Flat Device Tree legacy Image", },
> 
> I need a legacy image, where the type is set to IH_TYPE_FLATDT. Maybe
> "legacy_flat_dt" or "legacy_fdt" would be an appropriate name, too.
> 
> As this string is user facing I think it should have "legacy" in it.

fdt_legacy?

I just think flat_dt_legacy is quite wordy.

--Sean

> I think "flat_dt" and "fdt" is just too similar and the user can't see
> the difference, IMHO.
> 
>>>> --- a/boot/image.c
>>>> +++ b/boot/image.c
>>>> @@ -180,6 +180,7 @@ static const table_entry_t uimage_type[] = {
>>>>    	{	IH_TYPE_COPRO, "copro", "Coprocessor Image"},
>>>>    	{	IH_TYPE_SUNXI_EGON, "sunxi_egon",  "Allwinner eGON Boot Image" },
>>>>    	{	IH_TYPE_SUNXI_TOC0, "sunxi_toc0",  "Allwinner TOC0 Boot Image" },
>>>> +         {	IH_TYPE_FLATDT_LEGACY, "flat_dt_legacy", "Flat Device Tree legacy Image", },
>>>>    	{	-1,		    "",		  "",			},
>>>>    };
> 
> regards,
> Marc
>