Re: [U-Boot] [PATCH 3/8] tools: mkimage: Move image header code to image specific files

Dear Prafulla Wadaskar,

In message 1248804270-13715-3-git-send-email-prafulla@marvell.com you wrote:

This is second step towards cleaning mkimage code for kwbimage support in clean way. In this patch-

1. The image_get_header_size function call is replaced by sizeof(image_header_t) in default_image.c

I still fail to understand why this would be needed, or even wanted.

2. image header code moved form mkimage.c to default_image .c

This looks incorrect to me (not to mention the "form/from" typo): I see no code being moved, only variable declarations.

+/*

• • Default image parameters
• */

+struct image_type_params defimage_params = {

• .header_size = sizeof(image_header_t),
• .hdr = (void*)&header,

+};

+void *image_get_tparams (void){

• return (void *)&defimage_params;

+}

We need better documentation. Why would that function be needed?

int image_check_params (struct mkimage_params *params) { return ((params->dflag && (params->fflag || params->lflag)) || @@ -100,13 +114,13 @@ void image_set_header (char *ptr, struct stat *sbuf, image_header_t * hdr = (image_header_t *)ptr;

checksum = crc32 (0,

• 	(const char *)(ptr + image_get_header_size ()),
  

• 	sbuf->st_size - image_get_header_size ());
  

• 	(const char *)(ptr + sizeof(image_header_t)),
  

• 	sbuf->st_size - sizeof(image_header_t));
  

I cannot help it, but this change looks like a step backward to me. Why don't we use the header_size entry from the image_type_params here, too?

...

--- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -49,9 +49,6 @@ struct mkimage_params params = { .cmdname = "", };

-image_header_t header; -image_header_t *hdr = &header;

int main (int argc, char **argv) { @@ -59,6 +56,7 @@ main (int argc, char **argv) struct stat sbuf; unsigned char *ptr; int retval;

• struct image_type_params *tparams = image_get_tparams ();

  params.cmdname = *argv;

@@ -163,7 +161,7 @@ NXTARG: ; params.ep = params.addr; /* If XIP, entry point must be after the U-Boot header */ if (params.xflag)

• 	params.ep += image_get_header_size ();
  

• 	params.ep += tparams->header_size;
  

Would it not be better to stick with the image_get_header_size() function, and instead make it aware which image type we are asking for?

The old code was homnogenuous: it consistently used image_get_header_size(); now we have a sizeof() here and a ptr->header_size there.

I really dislike that.

Best regards,

Wolfgang Denk