[U-Boot-Users] [PATCH] Add 'imload' command

[U-Boot-Users] [PATCH] Add 'imload' command

[Kumar Gala]

'imload' provides a more direct means to load from an image file.

Also created a load_image routine out of the code in do_bootm() that
is shared between do_bootm() and do_imload().

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---

Note, this is against the u-boot-testing new-image branch.

 common/cmd_bootm.c |  180 ++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 133 insertions(+), 47 deletions(-)

diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index 2ddb191..0211b86 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -112,6 +112,58 @@ static boot_os_fn do_bootm_artos;

 ulong load_addr = CFG_LOAD_ADDR;	/* Default Load Address */

+int load_image(image_header_t *hdr, ulong img_data, ulong img_len, ulong dst)
+{
+	const char	*type_name;
+	uint		unc_len = CFG_BOOTM_LEN;
+	ulong		end = 0;
+
+	type_name = image_get_type_name (image_get_type (hdr));
+
+	switch (image_get_comp (hdr)) {
+	case IH_COMP_NONE:
+		if (dst == (ulong)hdr) {
+			printf ("   XIP %s ... ", type_name);
+		} else {
+			printf ("   Loading %s ... ", type_name);
+
+			memmove_wd ((void *)dst, (void *)img_data,
+					img_len, CHUNKSZ);
+
+			end = dst + img_len;
+		}
+		break;
+	case IH_COMP_GZIP:
+		printf ("   Uncompressing %s ... ", type_name);
+		if (gunzip ((void *)dst, unc_len,
+				(uchar *)img_data, &img_len) != 0)
+			return -1;
+
+		end = dst + img_len;
+		break;
+#ifdef CONFIG_BZIP2
+	case IH_COMP_BZIP2:
+		printf ("   Uncompressing %s ... ", type_name);
+		/*
+		 * If we've got less than 4 MB of malloc() space,
+		 * use slower decompression algorithm which requires
+		 * at most 2300 KB of memory.
+		 */
+		if (BZ2_bzBuffToBuffDecompress ((char*)dst,
+				&unc_len, (char *)img_data, img_len,
+				CFG_MALLOC_LEN < (4096 * 1024), 0) != BZ_OK)
+			return -1;
+
+		end = dst + unc_len;
+		break;
+#endif /* CONFIG_BZIP2 */
+	default:
+		return -2;
+	}
+
+	puts ("OK\n");
+	return end;
+}

 /*******************************************************************/
 /* bootm - boot application image from image in memory */
@@ -120,7 +172,6 @@ int do_bootm (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 {
 	ulong		iflag;
 	const char	*type_name;
-	uint		unc_len = CFG_BOOTM_LEN;
 	int		verify = getenv_verify();

 	image_header_t	*hdr;
@@ -160,61 +211,23 @@ int do_bootm (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 	image_start = (ulong)hdr;
 	image_end = image_get_image_end (hdr);
 	load_start = image_get_load (hdr);
-	load_end = 0;

-	switch (image_get_comp (hdr)) {
-	case IH_COMP_NONE:
-		if (image_get_load (hdr) == (ulong)hdr) {
-			printf ("   XIP %s ... ", type_name);
-		} else {
-			printf ("   Loading %s ... ", type_name);
-
-			memmove_wd ((void *)image_get_load (hdr),
-				   (void *)os_data, os_len, CHUNKSZ);
-
-			load_end = load_start + os_len;
-			puts("OK\n");
-		}
-		break;
-	case IH_COMP_GZIP:
-		printf ("   Uncompressing %s ... ", type_name);
-		if (gunzip ((void *)image_get_load (hdr), unc_len,
-					(uchar *)os_data, &os_len) != 0) {
-			puts ("GUNZIP ERROR - must RESET board to recover\n");
-			show_boot_progress (-6);
-			do_reset (cmdtp, flag, argc, argv);
-		}
+	load_end = load_image(hdr, os_data, os_len, load_start);

-		load_end = load_start + os_len;
-		break;
-#ifdef CONFIG_BZIP2
-	case IH_COMP_BZIP2:
-		printf ("   Uncompressing %s ... ", type_name);
-		/*
-		 * If we've got less than 4 MB of malloc() space,
-		 * use slower decompression algorithm which requires
-		 * at most 2300 KB of memory.
-		 */
-		int i = BZ2_bzBuffToBuffDecompress ((char*)image_get_load (hdr),
-					&unc_len, (char *)os_data, os_len,
-					CFG_MALLOC_LEN < (4096 * 1024), 0);
-		if (i != BZ_OK) {
-			printf ("BUNZIP2 ERROR %d - must RESET board to recover\n", i);
-			show_boot_progress (-6);
-			do_reset (cmdtp, flag, argc, argv);
-		}
+	if (load_end == -1) {
+		printf("%s ERROR - must RESET board to recover\n", type_name);
+		show_boot_progress (-6);
+		do_reset (cmdtp, flag, argc, argv);
+	}

-		load_end = load_start + unc_len;
-		break;
-#endif /* CONFIG_BZIP2 */
-	default:
+	if (load_end == -2) {
 		if (iflag)
 			enable_interrupts();
 		printf ("Unimplemented compression type %d\n", image_get_comp (hdr));
 		show_boot_progress (-7);
 		return 1;
 	}
-	puts ("OK\n");
+
 	debug ("   kernel loaded@0x%08lx, end = 0x%08lx\n", load_start, load_end);
 	show_boot_progress (7);

@@ -482,6 +495,79 @@ U_BOOT_CMD(
 );
 #endif

+/*******************************************************************/
+/* imload - load image file */
+/*******************************************************************/
+#if defined(CONFIG_CMD_IMLOAD)
+int do_imload (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
+{
+	ulong	addr = load_addr;
+	int	end;
+	int	verify = getenv_verify();
+	char	buf[12];
+	image_header_t *h;
+
+	if (argc > 1)
+		addr = simple_strtoul(argv[1], NULL, 16);
+
+	h = (image_header_t *)addr;
+
+	if (!image_check_magic (h)) {
+		puts ("   Bad Magic Number\n");
+		return 1;
+	}
+
+	if (!image_check_hcrc (h)) {
+		puts ("   Bad Header Checksum\n");
+		return 1;
+	}
+
+	print_image_hdr(h);
+
+	if (verify) {
+		puts ("   Verifying Checksum ... ");
+		if (!image_check_dcrc (h)) {
+			printf ("Bad Data CRC\n");
+			return 1;
+		}
+		puts ("OK\n");
+	}
+
+	if (argc == 3)
+		addr = simple_strtoul(argv[2], NULL, 16);
+	else
+		addr = image_get_load(h);
+
+	printf("\n   Loading image at %08x\n", addr);
+
+	end = load_image(h, image_get_data(h), image_get_data_size(h), addr);
+
+	if (end > 0) {
+		sprintf(buf, "%lX", addr);
+		setenv("fileaddr", buf);
+		sprintf(buf, "%lX", end - addr);
+		setenv("filesize", buf);
+		return 0;
+	}
+
+	if (end == -1)
+		puts("\n   Decompression Error\n");
+
+	return 1;
+}
+
+U_BOOT_CMD(
+	imload,	3,	1,	do_imload,
+	"imload  - load image from application image\n",
+	"[addr] [load addr]\n"
+	"    - load image from application image located at address 'addr'\n"
+	"      or $loadaddr if 'addr' is not provided to either the load\n"
+	"      address specified in the application image header or the\n"
+	"      'load addr' argument; this includes verification of the\n"
+	"      image contents (magic number, header and payload checksums)\n"
+);
+#endif
+

 /*******************************************************************/
 /* imls - list all images found in flash */
-- 
1.5.3.8

[U-Boot-Users] [PATCH] Add 'imload' command

[Bartlomiej Sieka]

Kumar Gala wrote:
> 'imload' provides a more direct means to load from an image file.
> 
> Also created a load_image routine out of the code in do_bootm() that
> is shared between do_bootm() and do_imload().
> 
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> ---
> 
> Note, this is against the u-boot-testing new-image branch.

Thanks.

Two comments:
- The load_image routine (and consequently imload commad) will not work 
when the image is stored in Data Flash.
- The code as-is will clearly not work with the new image format -- how 
about we wait a few days for the new format patchset I've mentioned in 
my previous email today? Note that the patchset will have a routine that 
you could use to deal with the Data Flash scenario.

Regards,
Bartlomiej

[U-Boot-Users] [PATCH] Add 'imload' command

[Kumar Gala]

On Feb 13, 2008, at 4:11 AM, Bartlomiej Sieka wrote:

> Kumar Gala wrote:
>> 'imload' provides a more direct means to load from an image file.
>> Also created a load_image routine out of the code in do_bootm() that
>> is shared between do_bootm() and do_imload().
>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>> ---
>> Note, this is against the u-boot-testing new-image branch.
>
> Thanks.
>
> Two comments:
> - The load_image routine (and consequently imload commad) will not  
> work when the image is stored in Data Flash.

what's the issue here?

> - The code as-is will clearly not work with the new image format --  
> how about we wait a few days for the new format patchset I've  
> mentioned in my previous email today? Note that the patchset will  
> have a routine that you could use to deal with the Data Flash  
> scenario.


I'm concerned about how long it will be before people adopt the new  
image format.  Also, do you at least have a spec for the new image  
format?

- k

[U-Boot-Users] [PATCH] Add 'imload' command

[Bartlomiej Sieka]

Kumar Gala wrote:
> 
> On Feb 13, 2008, at 4:11 AM, Bartlomiej Sieka wrote:
> 
>> Kumar Gala wrote:
>>> 'imload' provides a more direct means to load from an image file.
>>> Also created a load_image routine out of the code in do_bootm() that
>>> is shared between do_bootm() and do_imload().
>>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>>> ---
>>> Note, this is against the u-boot-testing new-image branch.
>>
>> Thanks.
>>
>> Two comments:
>> - The load_image routine (and consequently imload commad) will not 
>> work when the image is stored in Data Flash.
> 
> what's the issue here?

Please have a look at code under CONFIG_HAS_DATAFLASH in get_kernel()
(formerly in do_bootm()), especially the read_dataflash() function. The
issue is that you have to copy data from Data Flash in a specific way in
order to have random access to it. So for example this line in your code:
type_name = image_get_type_name (image_get_type (hdr));
will effectively try to access hdr->ih_type, which will not work when
hdr is an address in Data Flash.

> 
>> - The code as-is will clearly not work with the new image format -- 
>> how about we wait a few days for the new format patchset I've 
>> mentioned in my previous email today? Note that the patchset will have 
>> a routine that you could use to deal with the Data Flash scenario.
> 
> 
> I'm concerned about how long it will be before people adopt the new 
> image format.  Also, do you at least have a spec for the new image format?

There has been quite a bit information on the new image format posted to
the list, please refer to the following threads:
http://www.nabble.com/RFC%3A-New-U-boot-image-format-to14277371.html#a14417957
http://www.nabble.com/RFC%3A-New-uImage-format-bindings-to14417699.html#a14417699
http://www.nabble.com/RFC%3A-new-bootm-syntax-to14594482.html#a14594482

Please let me know if you have any comments or questions.

Regards,
Bartlomiej

[U-Boot-Users] [PATCH] Add 'imload' command

[Kumar Gala]

On Feb 13, 2008, at 1:55 PM, Bartlomiej Sieka wrote:

> Kumar Gala wrote:
>> On Feb 13, 2008, at 4:11 AM, Bartlomiej Sieka wrote:
>>> Kumar Gala wrote:
>>>> 'imload' provides a more direct means to load from an image file.
>>>> Also created a load_image routine out of the code in do_bootm()  
>>>> that
>>>> is shared between do_bootm() and do_imload().
>>>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>>>> ---
>>>> Note, this is against the u-boot-testing new-image branch.
>>>
>>> Thanks.
>>>
>>> Two comments:
>>> - The load_image routine (and consequently imload commad) will not  
>>> work when the image is stored in Data Flash.
>> what's the issue here?
>
> Please have a look at code under CONFIG_HAS_DATAFLASH in get_kernel()
> (formerly in do_bootm()), especially the read_dataflash() function.  
> The
> issue is that you have to copy data from Data Flash in a specific  
> way in
> order to have random access to it. So for example this line in your  
> code:
> type_name = image_get_type_name (image_get_type (hdr));
> will effectively try to access hdr->ih_type, which will not work when
> hdr is an address in Data Flash.

Just to be clear, at a quick glance, I assume load_image will work ok  
from do_bootm() but not from do_imload().  (since do_bootm is calling  
get_kernel).

>>> - The code as-is will clearly not work with the new image format  
>>> -- how about we wait a few days for the new format patchset I've  
>>> mentioned in my previous email today? Note that the patchset will  
>>> have a routine that you could use to deal with the Data Flash  
>>> scenario.
>> I'm concerned about how long it will be before people adopt the new  
>> image format.  Also, do you at least have a spec for the new image  
>> format?
>
> There has been quite a bit information on the new image format  
> posted to
> the list, please refer to the following threads:
> http://www.nabble.com/RFC%3A-New-U-boot-image-format-to14277371.html#a14417957

> http://www.nabble.com/RFC%3A-New-uImage-format-bindings-to14417699.html#a14417699

this one seem to have what I'm looking for, for some reason I didn't  
see it.
>
> http://www.nabble.com/RFC%3A-new-bootm-syntax- 
> to14594482.html#a14594482
>
> Please let me know if you have any comments or questions.

- k

[U-Boot-Users] [PATCH] Add 'imload' command

[Bartlomiej Sieka]

Kumar Gala wrote:
[...]
>>>> - The load_image routine (and consequently imload commad) will not 
>>>> work when the image is stored in Data Flash.
>>> what's the issue here?
>>
>> Please have a look at code under CONFIG_HAS_DATAFLASH in get_kernel()
>> (formerly in do_bootm()), especially the read_dataflash() function. The
>> issue is that you have to copy data from Data Flash in a specific way in
>> order to have random access to it. So for example this line in your code:
>> type_name = image_get_type_name (image_get_type (hdr));
>> will effectively try to access hdr->ih_type, which will not work when
>> hdr is an address in Data Flash.
> 
> Just to be clear, at a quick glance, I assume load_image will work ok 
> from do_bootm() but not from do_imload().  (since do_bootm is calling 
> get_kernel).

Correct.

Regards,
Bartlomiej

[U-Boot-Users] [PATCH] Add 'imload' command

[Kumar Gala]

On Feb 13, 2008, at 2:22 PM, Bartlomiej Sieka wrote:

> Kumar Gala wrote:
> [...]
>>>>> - The load_image routine (and consequently imload commad) will  
>>>>> not work when the image is stored in Data Flash.
>>>> what's the issue here?
>>>
>>> Please have a look at code under CONFIG_HAS_DATAFLASH in  
>>> get_kernel()
>>> (formerly in do_bootm()), especially the read_dataflash()  
>>> function. The
>>> issue is that you have to copy data from Data Flash in a specific  
>>> way in
>>> order to have random access to it. So for example this line in  
>>> your code:
>>> type_name = image_get_type_name (image_get_type (hdr));
>>> will effectively try to access hdr->ih_type, which will not work  
>>> when
>>> hdr is an address in Data Flash.
>> Just to be clear, at a quick glance, I assume load_image will work  
>> ok from do_bootm() but not from do_imload().  (since do_bootm is  
>> calling get_kernel).
>
> Correct.

Ok, I don't know much about the DATAFLASH device, but is it any  
different than reading from something like NAND or a HD?  It seems  
like we shouldn't burden the code w/providing special handling for  
accessing it if its not directly memory accessible like NOR flash is.

- k

[U-Boot-Users] [PATCH] Add 'imload' command

[Wolfgang Denk]

In message <Pine.LNX.4.64.0802122307330.22593@blarg.am.freescale.net> you wrote:
> 'imload' provides a more direct means to load from an image file.

What exactly is an "image file" here (which sorts of images are
allowed?) and what does "load" mean (which actions are performed on
the file)?

> Also created a load_image routine out of the code in do_bootm() that
> is shared between do_bootm() and do_imload().

Hm... bootm is restrictred to a certain sub-set of image types. imload
on the other hand sounds pretty generic to me and should not have such
restrictions.

> +	switch (image_get_comp (hdr)) {
> +	case IH_COMP_NONE:
> +		if (dst == (ulong)hdr) {
> +			printf ("   XIP %s ... ", type_name);

This is an ugly hack in the old code that should be removed by the new
implementation.

> +	"imload  - load image from application image\n",
> +	"[addr] [load addr]\n"

What exactly is an "application image" ?

> +	"    - load image from application image located at address 'addr'\n"
> +	"      or $loadaddr if 'addr' is not provided to either the load\n"
> +	"      address specified in the application image header or the\n"
> +	"      'load addr' argument; this includes verification of the\n"
> +	"      image contents (magic number, header and payload checksums)\n"

Please be terse. We don't want to spend the memory footprint on prosa.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Data is a lot like humans: It is  born.  Matures.  Gets  married  to
other  data, divorced. Gets old. One thing that it doesn't do is die.
It has to be killed."                                 - Arthur Miller

[U-Boot-Users] [PATCH] Add 'imload' command

[Kumar Gala]

On Feb 13, 2008, at 4:31 PM, Wolfgang Denk wrote:

> In message <Pine.LNX. 
> 4.64.0802122307330.22593 at blarg.am.freescale.net> you wrote:
>> 'imload' provides a more direct means to load from an image file.
>
> What exactly is an "image file" here (which sorts of images are
> allowed?) and what does "load" mean (which actions are performed on
> the file)?

its suppose to be a 'uImage' not sure what the base way to refer to  
that is.

>> Also created a load_image routine out of the code in do_bootm() that
>> is shared between do_bootm() and do_imload().
>
> Hm... bootm is restrictred to a certain sub-set of image types. imload
> on the other hand sounds pretty generic to me and should not have such
> restrictions.

what other image formats should we be supporting?

>> +	switch (image_get_comp (hdr)) {
>> +	case IH_COMP_NONE:
>> +		if (dst == (ulong)hdr) {
>> +			printf ("   XIP %s ... ", type_name);
>
> This is an ugly hack in the old code that should be removed by the new
> implementation.
>
>> +	"imload  - load image from application image\n",
>> +	"[addr] [load addr]\n"
>
> What exactly is an "application image" ?

I stole this from iminfo help.

>> +	"    - load image from application image located at address  
>> 'addr'\n"
>> +	"      or $loadaddr if 'addr' is not provided to either the load\n"
>> +	"      address specified in the application image header or the\n"
>> +	"      'load addr' argument; this includes verification of the\n"
>> +	"      image contents (magic number, header and payload checksums) 
>> \n"
>
> Please be terse. We don't want to spend the memory footprint on prosa.

no problem :)

- k

[U-Boot-Users] [PATCH] Add 'imload' command

[Wolfgang Denk]

In message <7DB626E7-34A4-4EAA-B470-E2CC49F97CA3@kernel.crashing.org> you wrote:
> 
> > What exactly is an "image file" here (which sorts of images are
> > allowed?) and what does "load" mean (which actions are performed on
> > the file)?
> 
> its suppose to be a 'uImage' not sure what the base way to refer to  
> that is.

Why just uImage?

> what other image formats should we be supporting?

All?

> > What exactly is an "application image" ?
> 
> I stole this from iminfo help.

Argh... where is the brown paper bag?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The first 90% of a project takes 90% of the time, the last 10%  takes
the other 90% of the time.

[U-Boot-Users] [PATCH] Add 'imload' command

[Kumar Gala]

On Feb 13, 2008, at 5:01 PM, Wolfgang Denk wrote:

> In message <7DB626E7-34A4-4EAA-B470- 
> E2CC49F97CA3 at kernel.crashing.org> you wrote:
>>
>>> What exactly is an "image file" here (which sorts of images are
>>> allowed?) and what does "load" mean (which actions are performed on
>>> the file)?
>>
>> its suppose to be a 'uImage' not sure what the base way to refer to
>> that is.
>
> Why just uImage?

what other image formats do we support?  I assume we'll extend it for  
the new format style once that exists.

>> what other image formats should we be supporting?
>
> All?

I feel like I'm missing something here.  I agree that IH_TYPE_MULTI  
needs to be supported, but I don't see why the code doesn't work for  
the bulk of the other IH_TYPE_* variants.  It appears the only  
compression types we support today ore NONE, GZIP, and BZIP2.

So what am I missing?

>>> What exactly is an "application image" ?
>>
>> I stole this from iminfo help.
>
> Argh... where is the brown paper bag?

:)

- k

[U-Boot-Users] [PATCH] Add 'imload' command

[Wolfgang Denk]

In message <3192541E-1FBD-4638-A317-3A25F316CD8C@kernel.crashing.org> you wrote:
> 
> > Why just uImage?
> 
> what other image formats do we support?  I assume we'll extend it for  
> the new format style once that exists.

uImage means "OS Kernel Image" (IH_TYPE_KERNEL in include/image.h).

> I feel like I'm missing something here.  I agree that IH_TYPE_MULTI  
> needs to be supported, but I don't see why the code doesn't work for  
> the bulk of the other IH_TYPE_* variants.  It appears the only  
> compression types we support today ore NONE, GZIP, and BZIP2.
> 
> So what am I missing?

I think it makes sense  to  allow  support  for  Standalone  Program,
RAMDisk,  Multi-File, Firmware, Script, Filesystem, and FDT images as
well.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Cigarette, n.: A fire at one end, a fool at the other, and a  bit  of
tobacco in between.

[U-Boot-Users] [PATCH] Add 'imload' command

[Kumar Gala]

On Feb 13, 2008, at 6:37 PM, Wolfgang Denk wrote:

> In message <3192541E-1FBD-4638- 
> A317-3A25F316CD8C at kernel.crashing.org> you wrote:
>>
>>> Why just uImage?
>>
>> what other image formats do we support?  I assume we'll extend it for
>> the new format style once that exists.
>
> uImage means "OS Kernel Image" (IH_TYPE_KERNEL in include/image.h).

What's the term to specify an image wrapped via mkimage?

>> I feel like I'm missing something here.  I agree that IH_TYPE_MULTI
>> needs to be supported, but I don't see why the code doesn't work for
>> the bulk of the other IH_TYPE_* variants.  It appears the only
>> compression types we support today ore NONE, GZIP, and BZIP2.
>>
>> So what am I missing?
>
> I think it makes sense  to  allow  support  for  Standalone  Program,
> RAMDisk,  Multi-File, Firmware, Script, Filesystem, and FDT images as
> well.

agreed, I think the only one I don't support right now is multi- 
image.  (however, I'm not sure what an image of type Filesystem  
actually means).

- k

[U-Boot-Users] [PATCH] Add 'imload' command

[Grant Likely]

On Feb 13, 2008 12:55 PM, Bartlomiej Sieka <tur@semihalf.com> wrote:
> Kumar Gala wrote:
> >
> > On Feb 13, 2008, at 4:11 AM, Bartlomiej Sieka wrote:
> >
> >> Kumar Gala wrote:
> >>> 'imload' provides a more direct means to load from an image file.
> >>> Also created a load_image routine out of the code in do_bootm() that
> >>> is shared between do_bootm() and do_imload().
> >>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> >>> ---
> >>> Note, this is against the u-boot-testing new-image branch.
> >>
> >> Thanks.
> >>
> >> Two comments:
> >> - The load_image routine (and consequently imload commad) will not
> >> work when the image is stored in Data Flash.
> >
> > what's the issue here?
>
> Please have a look at code under CONFIG_HAS_DATAFLASH in get_kernel()
> (formerly in do_bootm()), especially the read_dataflash() function. The
> issue is that you have to copy data from Data Flash in a specific way in
> order to have random access to it. So for example this line in your code:
> type_name = image_get_type_name (image_get_type (hdr));
> will effectively try to access hdr->ih_type, which will not work when
> hdr is an address in Data Flash.

Ugh, please don't continue down that path.  Dataflash is a serial
flash technology, but the driver pretends that it is memory mapped.
It is not a good abstraction that I really think needs to be removed.
I don't think it is a good idea to add that mis-feature into new
commands.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

[U-Boot-Users] [PATCH] Add 'imload' command

[Kumar Gala]

On Feb 13, 2008, at 7:13 PM, Grant Likely wrote:

> On Feb 13, 2008 12:55 PM, Bartlomiej Sieka <tur@semihalf.com> wrote:
>> Kumar Gala wrote:
>>>
>>> On Feb 13, 2008, at 4:11 AM, Bartlomiej Sieka wrote:
>>>
>>>> Kumar Gala wrote:
>>>>> 'imload' provides a more direct means to load from an image file.
>>>>> Also created a load_image routine out of the code in do_bootm()  
>>>>> that
>>>>> is shared between do_bootm() and do_imload().
>>>>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>>>>> ---
>>>>> Note, this is against the u-boot-testing new-image branch.
>>>>
>>>> Thanks.
>>>>
>>>> Two comments:
>>>> - The load_image routine (and consequently imload commad) will not
>>>> work when the image is stored in Data Flash.
>>>
>>> what's the issue here?
>>
>> Please have a look at code under CONFIG_HAS_DATAFLASH in get_kernel()
>> (formerly in do_bootm()), especially the read_dataflash() function.  
>> The
>> issue is that you have to copy data from Data Flash in a specific  
>> way in
>> order to have random access to it. So for example this line in your  
>> code:
>> type_name = image_get_type_name (image_get_type (hdr));
>> will effectively try to access hdr->ih_type, which will not work when
>> hdr is an address in Data Flash.
>
> Ugh, please don't continue down that path.  Dataflash is a serial
> flash technology, but the driver pretends that it is memory mapped.
> It is not a good abstraction that I really think needs to be removed.
> I don't think it is a good idea to add that mis-feature into new
> commands.

agreed.  I don't plan on supporting it.

- k

[U-Boot-Users] [PATCH] Add 'imload' command

[Kumar Gala]

>> +	switch (image_get_comp (hdr)) {
>> +	case IH_COMP_NONE:
>> +		if (dst == (ulong)hdr) {
>> +			printf ("   XIP %s ... ", type_name);
>
> This is an ugly hack in the old code that should be removed by the new
> implementation.

can you explain this further, I'm happy to git rid of it, just need to  
understand the hack.

- k

[U-Boot-Users] [PATCH] Add 'imload' command

[Bartlomiej Sieka]

Grant Likely wrote:
> On Feb 13, 2008 12:55 PM, Bartlomiej Sieka <tur@semihalf.com> wrote:
>> Kumar Gala wrote:
>>> On Feb 13, 2008, at 4:11 AM, Bartlomiej Sieka wrote:
>>>
>>>> Kumar Gala wrote:
>>>>> 'imload' provides a more direct means to load from an image file.
>>>>> Also created a load_image routine out of the code in do_bootm() that
>>>>> is shared between do_bootm() and do_imload().
>>>>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>>>>> ---
>>>>> Note, this is against the u-boot-testing new-image branch.
>>>> Thanks.
>>>>
>>>> Two comments:
>>>> - The load_image routine (and consequently imload commad) will not
>>>> work when the image is stored in Data Flash.
>>> what's the issue here?
>> Please have a look at code under CONFIG_HAS_DATAFLASH in get_kernel()
>> (formerly in do_bootm()), especially the read_dataflash() function. The
>> issue is that you have to copy data from Data Flash in a specific way in
>> order to have random access to it. So for example this line in your code:
>> type_name = image_get_type_name (image_get_type (hdr));
>> will effectively try to access hdr->ih_type, which will not work when
>> hdr is an address in Data Flash.
> 
> Ugh, please don't continue down that path.  Dataflash is a serial
> flash technology, but the driver pretends that it is memory mapped.
> It is not a good abstraction that I really think needs to be removed.

Hi Grant,

Not sure if your comment was directed to Kumar or me, but I'm replying
just in case.

I'm all for removing the code under CONFIG_HAS_DATAFLASH from at least
boot-related functions (haven't looked at other affected places in
U-Boot). We've not removed it in the new image format work, because by
default we try to preserve the status quo. When the new format handling
is introduced, we don't want to break too many things that people are
used to.

Regards,
Bartlomiej

[U-Boot-Users] [PATCH] Add 'imload' command

[Detlev Zundel]

Hi Grant,

> Ugh, please don't continue down that path.  Dataflash is a serial
> flash technology, but the driver pretends that it is memory mapped.
> It is not a good abstraction that I really think needs to be removed.
> I don't think it is a good idea to add that mis-feature into new
> commands.

Seconded.  That abstraction always irritated me a lot and made the
code only harder to read.

Cheers
  Detlev

-- 
Markov does it in chains.
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de