Patchwork [U-Boot,V4,04/11] imximage: prepare to move static variables to struct data_src

login
register
mail settings
Submitter Troy Kisky
Date Nov. 28, 2012, 1:31 a.m.
Message ID <1354066303-29762-5-git-send-email-troy.kisky@boundarydevices.com>
Download mbox | patch
Permalink /patch/202335/
State Changes Requested
Delegated to: Stefano Babic
Headers show

Comments

Troy Kisky - Nov. 28, 2012, 1:31 a.m.
Need to move accesses to the static variables to
a function where struct data_src is used.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>

---
v3: new patch
---
 tools/imximage.c |   24 +++++++++++++-----------
 tools/imximage.h |    3 +++
 2 files changed, 16 insertions(+), 11 deletions(-)
Wolfgang Denk - Nov. 28, 2012, 9:38 a.m.
Dear Troy Kisky,

In message <1354066303-29762-5-git-send-email-troy.kisky@boundarydevices.com> you wrote:
> Need to move accesses to the static variables to
> a function where struct data_src is used.

Could you please elucidate why exactly this is _needed_?

> +	/* Be able to detect if the cfg file has no BOOT_FROM tag */
> +	g_flash_offset = FLASH_OFFSET_UNDEFINED;
> +	memset(&ds, 0, sizeof(struct data_src));

Is this initialization really needed?

Best regards,

Wolfgang Denk
Troy Kisky - Nov. 28, 2012, 6:36 p.m.
On 11/28/2012 2:38 AM, Wolfgang Denk wrote:
> Dear Troy Kisky,
>
> In message <1354066303-29762-5-git-send-email-troy.kisky@boundarydevices.com> you wrote:
>> Need to move accesses to the static variables to
>> a function where struct data_src is used.
> Could you please elucidate why exactly this is _needed_?

My goal was to reduce the number of static variables, but strictly speaking
it has little benefit other than giving me a warm fuzzy feeling.

I'm not that only one that dislikes static though.

>
>> +	/* Be able to detect if the cfg file has no BOOT_FROM tag */
>> +	g_flash_offset = FLASH_OFFSET_UNDEFINED;
>> +	memset(&ds, 0, sizeof(struct data_src));
> Is this initialization really needed?
>
> Best regards,
>
> Wolfgang Denk
>
ds is on the stack, and even if not needed now, I like to avoid future 
random bugs.

Troy
Wolfgang Denk - Nov. 28, 2012, 8:30 p.m.
Dear Troy Kisky,

In message <50B659AD.9090704@boundarydevices.com> you wrote:
>
> > Could you please elucidate why exactly this is _needed_?
> 
> My goal was to reduce the number of static variables, but strictly speaking
> it has little benefit other than giving me a warm fuzzy feeling.
> 
> I'm not that only one that dislikes static though.
...
> ds is on the stack, and even if not needed now, I like to avoid future 
> random bugs.

Did you check the impact of your changes on the memory footprint?

Changing code that uses a static variable initialized (implicitly or
explicitly) to zero [which results in allocation of the BSS segment,
i. e. zero space in the code or in the image file] into real code
is something that is a bit of expensive just for satisfying random
"dislikes".

I think you should better leave that as is.  The code is pretty
efficent that way, and you increase it for little or no benefit.

Best regards,

Wolfgang Denk

Patch

diff --git a/tools/imximage.c b/tools/imximage.c
index 97e5c4b..3a010a6 100644
--- a/tools/imximage.c
+++ b/tools/imximage.c
@@ -407,8 +407,11 @@  static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd,
 		break;
 	}
 }
-static uint32_t parse_cfg_file(struct imx_header *imxhdr, char *name)
+
+static int parse_cfg_file(struct imx_header *imxhdr, char *name,
+		uint32_t entry_point)
 {
+	struct data_src ds;
 	FILE *fd = NULL;
 	char *line = NULL;
 	char *token, *saveptr1, *saveptr2;
@@ -418,6 +421,10 @@  static uint32_t parse_cfg_file(struct imx_header *imxhdr, char *name)
 	int dcd_len = 0;
 	int32_t cmd;
 
+	/* Be able to detect if the cfg file has no BOOT_FROM tag */
+	g_flash_offset = FLASH_OFFSET_UNDEFINED;
+	memset(&ds, 0, sizeof(struct data_src));
+	ds.imxhdr = imxhdr;
 	/*
 	 * In order to not change the old imx cfg file
 	 * by adding VERSION command into it, here need
@@ -465,10 +472,10 @@  static uint32_t parse_cfg_file(struct imx_header *imxhdr, char *name)
 		fprintf(stderr, "Error: No BOOT_FROM tag in %s\n", name);
 		exit(EXIT_FAILURE);
 	}
-	return dcd_len;
+	/* Set the imx header */
+	return (*set_imx_hdr)(imxhdr, dcd_len, entry_point, g_flash_offset);
 }
 
-
 static int imximage_check_image_types(uint8_t type)
 {
 	if (type == IH_TYPE_IMXIMAGE)
@@ -510,21 +517,16 @@  int imximage_vrec_header(struct mkimage_params *params,
 		struct image_type_params *tparams)
 {
 	struct imx_header *imxhdr;
-	uint32_t dcd_len;
 
 	imxhdr = calloc(1, MAX_HEADER_SIZE);
 	if (!imxhdr) {
 		fprintf(stderr, "Error: out of memory\n");
 		exit(EXIT_FAILURE);
 	}
-	/* Be able to detect if the cfg file has no BOOT_FROM tag */
-	g_flash_offset = FLASH_OFFSET_UNDEFINED;
-	/* Parse dcd configuration file */
-	dcd_len = parse_cfg_file(imxhdr, params->imagename);
 
-	/* Set the imx header */
-	imximage_params.header_size = (*set_imx_hdr)(imxhdr, dcd_len,
-			params->ep, g_flash_offset);
+	/* Parse dcd configuration file */
+	imximage_params.header_size = parse_cfg_file(imxhdr, params->imagename,
+			params->ep);
 	imximage_params.hdr = imxhdr;
 	return 0;
 }
diff --git a/tools/imximage.h b/tools/imximage.h
index 0f39447..2895378 100644
--- a/tools/imximage.h
+++ b/tools/imximage.h
@@ -171,4 +171,7 @@  typedef void (*set_dcd_rst_t)(struct imx_header *imxhdr,
 typedef int (*set_imx_hdr_t)(struct imx_header *imxhdr, uint32_t dcd_len,
 		uint32_t entry_point, uint32_t flash_offset);
 
+struct data_src {
+	struct imx_header *imxhdr;
+};
 #endif /* _IMXIMAGE_H_ */