imls: Add support to list images in NAND device
PATCH v2 1/1 imls: Add support to list images in NAND device
imls does not list the images in NAND devices. This patch implements this support for legacy type images.
---
Hello Scott,
There has been sometime since you reviewed the first version of this patch.
http://lists.denx.de/pipermail/u-boot/2012-November/139631.html
I am sorry for a late response on this. I was waiting for other comments if any on the whole patch-set
Regards
Vipin
README | 3 +- common/cmd_bootm.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 134 insertions(+), 2 deletions(-)
README +2 -1
@@ -831,7 +831,8 @@ The following options need to be configured: CONFIG_CMD_I2C * I2C serial bus support CONFIG_CMD_IDE * IDE harddisk support CONFIG_CMD_IMI iminfo - CONFIG_CMD_IMLS List all found images + CONFIG_CMD_IMLS List all images found in flash + CONFIG_CMD_IMLS_NAND List all images found in NAND device CONFIG_CMD_IMMAP * IMMR dump support CONFIG_CMD_IMPORTENV * import an environment CONFIG_CMD_INI * import data from an ini file into the env
common/cmd_bootm.c +132 -1
151 lines not on this page · raw message
Signature · 1 line
1.8.0
PATCH v2 1/1 imls: Add support to list images in NAND device
Dear Vipin Kumar,
Quoted 2 lines · In message <dba14c4ffef38a108f75342968bcd9ce2b75c4c7.1355303894.git.vipin.kumar…
imls does not list the images in NAND devices. This patch implements this support for legacy type images.
...
common/cmd_bootm.c U_BOOT_CMD( · line 1181
Quoted 2 lines
common/cmd_bootm.c +1
-1-static int do_imls (cmd_tbl_t *cmdtp,int flag,int argc,char *const argv[]) +static void do_imls_flash (void )
Why is this void? Should we not return error codes? Or at least be able to?
Quoted 1 line
+static void
do_imls_nand(void)
Ditto.
Best regards,
Wolfgang Denk
Signature · 6 lines
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 Far back in the mists of ancient time, in the great and glorious days of the former Galactic Empire, life was wild, rich and largely tax free. - Douglas Adams, _The Hitchhiker's Guide to the Galaxy_
PATCH v2 1/1 imls: Add support to list images in NAND device
Quoted 12 lines · Wolfgang Denk
Dear Vipin Kumar,
Quoted 2 lines · In message<dba14c4ffef38a108f75342968bcd9ce2b75c4c7.1355303894.git.vipin.kumar@…
imls does not list the images in NAND devices. This patch implements this support for legacy type images.
...
common/cmd_bootm.c U_BOOT_CMD( · line 1181
Quoted 2 lines
common/cmd_bootm.c +1
-1-static int do_imls (cmd_tbl_t *cmdtp,int flag,int argc,char *const argv[]) +static void do_imls_flash (void )Why is this void? Should we not return error codes? Or at least be able to?
Yes, I agree. I would change this
Quoted 9 lines
Quoted 1 line
+static void
do_imls_nand(void)Ditto.
Best regards,
Wolfgang Denk
PATCH v2 1/1 imls: Add support to list images in NAND device
In reply to Vipin KumarQuoted 35 lines · Vipin Kumar
imls does not list the images in NAND devices. This patch implements this
support for legacy type images.Signed-off-by Vipin Kumar <[email protected]>---
Hello Scott,There has been sometime since you reviewed the first version of this patch.
http://lists.denx.de/pipermail/u-boot/2012-November/139631.htmlI am sorry for a late response on this. I was waiting for other comments if any
on the whole patch-setRegards
VipinREADME | 3 +- common/cmd_bootm.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 134 insertions(+), 2 deletions(-)README +2
-1@@ -831,7 +831,8 @@ The following options need to be configured: CONFIG_CMD_I2C * I2C serial bus support CONFIG_CMD_IDE * IDE harddisk support CONFIG_CMD_IMI iminfo - CONFIG_CMD_IMLS List all found images + CONFIG_CMD_IMLS List all images found in flash + CONFIG_CMD_IMLS_NAND List all images found in NAND
Quoted 1 line
device
s/in flash/in NOR flash/
s/in NAND device/in NAND flash/
Or better, just have one CONFIG_CMD_IMLS and have it operate on whatever flash types are configured into U-Boot.
Quoted 21 lines
README +0
-0CONFIG_CMD_IMMAP * IMMR dump support CONFIG_CMD_IMPORTENV * import an environment CONFIG_CMD_INI * import data from an ini fileinto the env
common/cmd_bootm.c +0
-0@@ -83,6 +83,9 @@ extern flash_info_t flash_info[]; /* info for FLASHchips */
static int do_imls(cmd_tbl_t *cmdtp, int flag, int argc, char *
const argv[]);
#endifcommon/cmd_bootm.c +3
-0+#include <linux/err.h> +#include <nand.h> +#ifdef CONFIG_SILENT_CONSOLE static void fixup_silent_linux (void );#endif
common/cmd_bootm.c U_BOOT_CMD( · line 1182
Quoted 6 lines
common/cmd_bootm.c +1
-1@@ -1175,7 +1178,7 @@ U_BOOT_CMD(/* imls - list all images found in flash */ /*******************************************************************/ #if defined(CONFIG_CMD_IMLS ) -static int do_imls (cmd_tbl_t *cmdtp,int flag,int argc,char * const argv[]) +static void do_imls_flash (void )
s/flash/nor/
Quoted 8 lines
common/cmd_bootm.c +0
-0{flash_info_t *info;common/cmd_bootm.c +1
-0int i, j;@@ -1224,6 +1227,134 @@ next_sector: ; } next_bank: ; } +}
common/cmd_bootm.c next_sector: ; · line 1240
Quoted 10 lines
common/cmd_bootm.c +10
-0+#endif + +#if defined(CONFIG_CMD_IMLS_NAND ) +static void do_imls_nand (void ) +{ +nand_info_t *nand; +int nand_dev = nand_curr_device; +size_t read_size; +loff_t off; +u8 buffer[512 ];
Why 512?
common/cmd_bootm.c next_sector: ; · line 1247
Quoted 7 lines
common/cmd_bootm.c +7
-0+const image_header_t *header = (const image_header_t *)buffer; + +/* the following commands operate on the current device */ +if (nand_dev <0 || nand_dev >=CONFIG_SYS_MAX_NAND_DEVICE ) { +puts ("\nNo NAND devices available\n" ); +return ; + }
"following commands", plural?
And this command seems to operate on all devices, not just the current one.
common/cmd_bootm.c next_sector: ; · line 1252
Quoted 6 lines
common/cmd_bootm.c +5
-0+ +printf ("\n" ); + +for (nand_dev =0 ; nand_dev <CONFIG_SYS_MAX_NAND_DEVICE ; nand_dev++) { +
Don't put a blank line inside braces at the beginning/end of blocks.
Quoted 7 lines
common/cmd_bootm.c +6
-0+ nand = &nand_info[nand_dev]; +if (!nand->name || !nand->size) +continue ; + +for (off =0 ; off < nand->size; off += nand->erasesize) { +int ret;
common/cmd_bootm.c next_sector: ; · line 1268
Quoted 10 lines
common/cmd_bootm.c +10
-0+void *imgdata; + +if (nand_block_isbad (nand, off)) + goto next_block; + + read_size =sizeof (buffer); + + ret =nand_read (nand, off, &read_size, buffer); +if (ret <0 && ret != -EUCLEAN ) + goto next_block;
s/goto next_block/continue/
Quoted 14 lines
common/cmd_bootm.c +12
-0+ header = (const image_header_t *)buffer; + +switch (genimg_get_format (buffer)) { +case IMAGE_FORMAT_LEGACY : +if (!image_check_hcrc (header)) + goto next_block; + + read_size =image_get_image_size (header); + + imgdata =malloc (read_size); +if (!imgdata) { + printf("Not able to list allimages " \
common/cmd_bootm.c next_sector: ; · line 1282
Quoted 1 line
common/cmd_bootm.c +1
-0+ "(Low memory)\n");
Don't line-wrap error strings.
common/cmd_bootm.c next_sector: ; · line 1291
Quoted 10 lines
common/cmd_bootm.c +4
-0+ goto next_block; + } + + ret = nand_read_skip_bad(nand, off,
&read_size,common/cmd_bootm.c +5
-0+ imgdata); +if (ret <0 && ret != -EUCLEAN ) { +free (imgdata); + goto next_block; + }
s/goto next_block/break/g
...or better, factor the switch out to its own function.
Quoted 30 lines
common/cmd_bootm.c +2
-0+ + printf("Legacy Image at NAND device %d" \
common/cmd_bootm.c +1
-0+ "offset %08llX:\n", nand_dev,off);
common/cmd_bootm.c +17
-0+image_print_contents (imgdata); + +puts (" Verifying Checksum ... " ); +if (!image_check_dcrc (imgdata)) +puts ("Bad Data CRC\n" ); +else +puts ("OK\n" ); + +free (imgdata); +break ; +#if defined(CONFIG_FIT ) +case IMAGE_FORMAT_FIT : + read_size =fit_get_size (buffer); + + imgdata =malloc (read_size); +if (!imgdata) { + printf("May be a FIT Image atNAND " \
common/cmd_bootm.c +1
-0+ "device %d offset%08llX:\n",
common/cmd_bootm.c +2
-0+ nand_dev, off); + printf(" Low memory(cannotallocate" \
common/cmd_bootm.c +1
-0+ " memory forimage)\n");
common/cmd_bootm.c
Quoted 1 line
common/cmd_bootm.c +1
-0+ goto next_block;
Why is the no-memory error message different for FIT versus legacy images? I realize that at this point you don't know if it's a FIT versus some other dtb, but why do you print the device and offset here but not in the legacy case? Why "Low memory(cannot allocate memory for image)" versus just " (Low memory)"?
Quoted 18 lines
common/cmd_bootm.c +3
-0+ } + + ret = nand_read_skip_bad(nand, off,
&read_size,common/cmd_bootm.c +13
-0+ imgdata); +if (ret <0 && ret != -EUCLEAN ) { +free (imgdata); + goto next_block; + } + +if (!fit_check_format (imgdata)) { +free (imgdata); + goto next_block; + } + + printf("FIT Image at NAND device %d " \ + "offset %08llX:\n", nand_dev,off);
common/cmd_bootm.c next_sector: ; · line 1340
Quoted 8 lines
common/cmd_bootm.c +8
-0+ +fit_print_contents (imgdata); +free (imgdata); +break ; +#endif +default : + goto next_block; + }
The default: doesn't do anything -- just leave it out.
common/cmd_bootm.c next_sector: ; · line 1343
Quoted 3 lines
common/cmd_bootm.c +3
-0+ +next_block: ; + }
Instead of using goto please factor out the switch to its own function and use return.
-Scott
PATCH v2 1/1 imls: Add support to list images in NAND device
[...]
Quoted 6 lines
Quoted 6 lines
README | 3 +- common/cmd_bootm.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 134 insertions(+), 2 deletions(-)
Quoted 16 lines
Quoted 11 lines
README +2
-1@@ -831,7 +831,8 @@ The following options need to be configured: CONFIG_CMD_I2C * I2C serial bus support CONFIG_CMD_IDE * IDE harddisk support CONFIG_CMD_IMI iminfo - CONFIG_CMD_IMLS List all found images + CONFIG_CMD_IMLS List all images found in flash + CONFIG_CMD_IMLS_NAND List all images found in NANDQuoted 1 line
device
s/in flash/in NOR flash/
s/in NAND device/in NAND flash/
OK
Quoted 3 lines
Or better, just have one
CONFIG_CMD_IMLSand have it operate on whatever flash types are configured into U-Boot.
I didn't do it because until now the CONFIG_CMD_IMLS config is tightly bound with flash only eg config_cmd_default.h enables CONFIG_CMD_IMLS only when CONFIG_SYS_NO_FLASH is not defined. I thought there might be other places as well
Quoted 14 lines
Quoted 14 lines
CONFIG_CMD_IMMAP* IMMR dump support
CONFIG_CMD_IMPORTENV* import an environment
CONFIG_CMD_INI* import data from an ini file
into the envcommon/cmd_bootm.c +0
-0@@ -83,6 +83,9 @@ extern flash_info_t flash_info[]; /* info for FLASHchips */
static int do_imls(cmd_tbl_t *cmdtp, int flag, int argc, char *
const argv[]);
#endif
Quoted 16 lines
common/cmd_bootm.c U_BOOT_CMD( · line 1182
Quoted 13 lines
common/cmd_bootm.c +4
-1+#include <linux/err.h> +#include <nand.h> +#ifdef CONFIG_SILENT_CONSOLE static void fixup_silent_linux (void );#endif @@ -1175,7 +1178,7 @@ U_BOOT_CMD(/* imls - list all images found in flash */ /*******************************************************************/ #if defined(CONFIG_CMD_IMLS ) -static int do_imls (cmd_tbl_t *cmdtp,int flag,int argc,char * const argv[]) +static void do_imls_flash (void )s/flash/nor/
OK
Quoted 12 lines
Quoted 12 lines
common/cmd_bootm.c +0
-0{flash_info_t *info;int i, j;
common/cmd_bootm.c +5
-0@@ -1224,6 +1227,134 @@ next_sector: ; } next_bank: ; } +} +#endif + +#if defined(CONFIG_CMD_IMLS_NAND ) +static void do_imls_nand (void )
Quoted 9 lines
common/cmd_bootm.c next_sector: ; · line 1240
Quoted 6 lines
common/cmd_bootm.c +6
-0+{ +nand_info_t *nand; +int nand_dev = nand_curr_device; +size_t read_size; +loff_t off; +u8 buffer[512 ];Why 512?
Basically there are 2 image types supported as of today.
* Legacy: 64 byte header
* FIT: < 512 byte header
After reading the first 512 bytes from each block of NAND, we try to validate the header and only if the header validation is successful, we malloc the space for the whole image and read the image into it
Quoted 10 lines
common/cmd_bootm.c next_sector: ; · line 1247
Quoted 7 lines
common/cmd_bootm.c +7
-0+const image_header_t *header = (const image_header_t *)buffer; + +/* the following commands operate on the current device */ +if (nand_dev<0 || nand_dev>=CONFIG_SYS_MAX_NAND_DEVICE ) { +puts ("\nNo NAND devices available\n" ); +return ; + }"following commands", plural?
I think its a copy paste error
Quoted 3 lines
And this command seems to operate on all devices, not just the current one.
Yes, that's why a copy paste
Quoted 9 lines
common/cmd_bootm.c next_sector: ; · line 1252
Quoted 6 lines
common/cmd_bootm.c +5
-0+ +printf ("\n" ); + +for (nand_dev =0 ; nand_dev<CONFIG_SYS_MAX_NAND_DEVICE ; nand_dev++) { +Don't put a blank line inside braces at the beginning/end of blocks.
OK
Quoted 11 lines
Quoted 11 lines
common/cmd_bootm.c +10
-0+ nand =&nand_info[nand_dev]; +if (!nand->name || !nand->size) +continue ; + +for (off =0 ; off< nand->size; off += nand->erasesize) { +int ret; +void *imgdata; + +if (nand_block_isbad (nand, off)) + goto next_block;
Quoted 9 lines
common/cmd_bootm.c next_sector: ; · line 1268
Quoted 6 lines
common/cmd_bootm.c +6
-0+ + read_size =sizeof (buffer); + + ret =nand_read (nand, off,&read_size, buffer); +if (ret<0 && ret != -EUCLEAN ) + goto next_block;s/goto next_block/continue/
hmmm, OK.
I copied the original code ie for listing images from nor flash. Should I also correct it !!
Quoted 9 lines
Quoted 9 lines
common/cmd_bootm.c +8
-0+ header = (const image_header_t *)buffer; + +switch (genimg_get_format (buffer)) { +case IMAGE_FORMAT_LEGACY : +if (!image_check_hcrc (header)) + goto next_block; + + read_size =image_get_image_size (header);
Quoted 9 lines
common/cmd_bootm.c next_sector: ; · line 1282
Quoted 6 lines
common/cmd_bootm.c +4
-0+ + imgdata =malloc (read_size); +if (!imgdata) { + printf("Not able to list allimages " \
common/cmd_bootm.c +1
-0+ "(Low memory)\n");Don't line-wrap error strings.
80 column ?
Quoted 13 lines
common/cmd_bootm.c next_sector: ; · line 1291
Quoted 10 lines
common/cmd_bootm.c +4
-0+ goto next_block; + } + + ret = nand_read_skip_bad(nand, off,
&read_size,common/cmd_bootm.c +5
-0+ imgdata); +if (ret<0 && ret != -EUCLEAN ) { +free (imgdata); + goto next_block; + }s/goto next_block/break/g
Quoted 2 lines
...or better, factor the switch out to its own function.
Didn't get that.
Quoted 29 lines
Quoted 29 lines
common/cmd_bootm.c +2
-0+ + printf("Legacy Image at NAND device %d" \
common/cmd_bootm.c +1
-0+ "offset %08llX:\n", nand_dev,off);
common/cmd_bootm.c +17
-0+image_print_contents (imgdata); + +puts (" Verifying Checksum ... " ); +if (!image_check_dcrc (imgdata)) +puts ("Bad Data CRC\n" ); +else +puts ("OK\n" ); + +free (imgdata); +break ; +#if defined(CONFIG_FIT ) +case IMAGE_FORMAT_FIT : + read_size =fit_get_size (buffer); + + imgdata =malloc (read_size); +if (!imgdata) { + printf("May be a FIT Image atNAND " \
common/cmd_bootm.c +1
-0+ "device %d offset%08llX:\n",
common/cmd_bootm.c +2
-0+ nand_dev, off); + printf(" Low memory(cannotallocate" \
common/cmd_bootm.c +1
-0+ " memory for
Quoted 9 lines
common/cmd_bootm.c
Quoted 2 lines
image)\n");
common/cmd_bootm.c +1
-0+ goto next_block;Why is the no-memory error message different for FIT versus legacy images? I realize that at this point you don't know if it's a FIT versus some other dtb, but why do you print the device and offset here but not in the legacy case? Why "Low memory(cannot allocate memory for image)" versus just " (Low memory)"?
Typo :(
I would give the following print for both
printf("May be a FIT Image at NAND " \
"device %d offset %08llX:\n",
nand_dev, off);
printf(" Low memory(cannot allocate" \
" memory for image)\n");Quoted 20 lines
Quoted 20 lines
common/cmd_bootm.c +3
-0+ } + + ret = nand_read_skip_bad(nand, off,
&read_size,common/cmd_bootm.c +13
-0+ imgdata); +if (ret<0 && ret != -EUCLEAN ) { +free (imgdata); + goto next_block; + } + +if (!fit_check_format (imgdata)) { +free (imgdata); + goto next_block; + } + + printf("FIT Image at NAND device %d " \ + "offset %08llX:\n", nand_dev,off);
common/cmd_bootm.c +2
-0+ +fit_print_contents (imgdata);
Quoted 9 lines
common/cmd_bootm.c next_sector: ; · line 1340
Quoted 6 lines
common/cmd_bootm.c +6
-0+free (imgdata); +break ; +#endif +default : + goto next_block; + }The default: doesn't do anything -- just leave it out.
OK
Quoted 7 lines
common/cmd_bootm.c next_sector: ; · line 1343
Quoted 3 lines
common/cmd_bootm.c +3
-0+ +next_block: ; + }Instead of using goto please factor out the switch to its own function and use return.
Ah, now I get your point
Thanks
I would send a v3 soon
Quoted 3 lines
-Scott
.
PATCH v2 1/1 imls: Add support to list images in NAND device
Quoted 8 lines · Vipin Kumar
Quoted 3 lines
Or better, just have one
CONFIG_CMD_IMLSand have it operate on whatever flash types are configured into U-Boot.I didn't do it because until now the
CONFIG_CMD_IMLSconfig is tightly bound with flash only egconfig_cmd_default.henablesCONFIG_CMD_IMLSonly whenCONFIG_SYS_NO_FLASHis not defined. I thought there might be other places as well
Still, it might be better to fix that than to build upon a no-longer-accurate assumption.
Quoted 12 lines
Quoted 11 lines
common/cmd_bootm.c next_sector: ; · line 1240
Quoted 8 lines
common/cmd_bootm.c +8
-0+#if defined(CONFIG_CMD_IMLS_NAND ) +static void do_imls_nand (void ) +{ +nand_info_t *nand; +int nand_dev = nand_curr_device; +size_t read_size; +loff_t off; +u8 buffer[512 ];Why 512?
Quoted 7 lines
Basically there are 2 image types supported as of today.
* Legacy: 64 byte header
* FIT: < 512 byte headerAfter reading the first 512 bytes from each block of NAND, we try to validate the header and only if the header validation is successful, we malloc the space for the whole image and read the image into it
Do you really need 512 bytes for fdt_check_header() to work?
Quoted 21 lines
Quoted 11 lines
Quoted 11 lines
common/cmd_bootm.c +5
-0+ nand =&nand_info[nand_dev]; +if (!nand->name || !nand->size) +continue ; + +for (off =0 ; off< nand->size; off += nand->erasesize){
common/cmd_bootm.c +5
-0+int ret; +void *imgdata; + +if (nand_block_isbad (nand, off)) + goto next_block;Quoted 9 lines
common/cmd_bootm.c next_sector: ; · line 1268
Quoted 6 lines
common/cmd_bootm.c +6
-0+ + read_size =sizeof (buffer); + + ret =nand_read (nand, off,&read_size, buffer); +if (ret<0 && ret != -EUCLEAN ) + goto next_block;s/goto next_block/continue/
Quoted 3 lines
hmmm, OK.
I copied the original code ie for listing images from nor flash. Should I also correct it !!
If you want to do that as a separate patch, that's fine -- but the code is sufficiently different that I don't think there's a strong consistency argument to be made.
Quoted 19 lines
Quoted 9 lines
Quoted 9 lines
common/cmd_bootm.c +8
-0+ header = (const image_header_t *)buffer; + +switch (genimg_get_format (buffer)) { +case IMAGE_FORMAT_LEGACY : +if (!image_check_hcrc (header)) + goto next_block; + + read_size =
image_get_image_size(header);Quoted 9 lines
common/cmd_bootm.c next_sector: ; · line 1282
Quoted 6 lines
common/cmd_bootm.c +4
-0+ + imgdata =malloc (read_size); +if (!imgdata) { + printf("Not able to list allimages " \
common/cmd_bootm.c +1
-0+ "(Low memory)\n");Don't line-wrap error strings.
Quoted 1 line
80 column ?
Error strings are an exception for the sake of greppability. From Linux's Documentation/CodingStyle:
Statements longer than 80 columns will be broken into sensible
chunks, unless
exceeding 80 columns significantly increases readability and does
not hide
information. Descendants are always substantially shorter than the
parent and
are placed substantially to the right. The same applies to function
headers
with a long argument list. However, never break user-visible strings
such as
printk messages, because that breaks the ability to grep for them.
Quoted 9 lines
Quoted 8 lines
Why is the no-memory error message different for FIT versus legacy images? I realize that at this point you don't know if it's a FIT versus some other dtb, but why do you print the device and offset here
but not in the legacy case? Why "Low memory(cannot allocate memory for
image)" versus just " (Low memory)"?
Quoted 7 lines
Typo :(
I would give the following print for bothprintf("May be a FIT Image at NAND " \ "device %d offset %08llX:\n", nand_dev, off); printf(" Low memory(cannot allocate" \ " memory for image)\n");
It's a little more verbose than I'd have done, but OK. Can you put a space between "memory" and "(cannot", though?
-Scott
PATCH v2 1/1 imls: Add support to list images in NAND device
Quoted 5 lines · Scott Wood
Quoted 4 lines · Vipin Kumar
Quoted 3 lines
Or better, just have one
CONFIG_CMD_IMLSand have it operate on whatever flash types are configured into U-Boot.
Quoted 8 lines
Quoted 4 lines
I didn't do it because until now the
CONFIG_CMD_IMLSconfig is tightly bound with flash only egconfig_cmd_default.henablesCONFIG_CMD_IMLSonly whenCONFIG_SYS_NO_FLASHis not defined. I thought there might be other places as wellStill, it might be better to fix that than to build upon a no-longer-accurate assumption.
OK, I would try that in v4
Quoted 16 lines
Quoted 16 lines
Quoted 11 lines
common/cmd_bootm.c next_sector: ; · line 1240
Quoted 8 lines
common/cmd_bootm.c +8
-0+#if defined(CONFIG_CMD_IMLS_NAND ) +static void do_imls_nand (void ) +{ +nand_info_t *nand; +int nand_dev = nand_curr_device; +size_t read_size; +loff_t off; +u8 buffer[512 ];Why 512?
Basically there are 2 image types supported as of today.
* Legacy: 64 byte header
* FIT:< 512 byte header
Quoted 6 lines
Quoted 3 lines
After reading the first 512 bytes from each block of NAND, we try to validate the header and only if the header validation is successful, we malloc the space for the whole image and read the image into it
Do you really need 512 bytes for
fdt_check_header()to work?
I have reduced it already to 64 bytes in v3
Quoted 21 lines
Quoted 21 lines
Quoted 20 lines
Quoted 7 lines
common/cmd_bootm.c +5
-0+ nand =&nand_info[nand_dev]; +if (!nand->name || !nand->size) +continue ; + +for (off =0 ; off< nand->size; off += nand->erasesize){
common/cmd_bootm.c +1
-0+int ret;common/cmd_bootm.c next_sector: ; · line 1268
Quoted 10 lines
common/cmd_bootm.c +10
-0+void *imgdata; + +if (nand_block_isbad (nand, off)) + goto next_block; + + read_size =sizeof (buffer); + + ret =nand_read (nand, off,&read_size, buffer); +if (ret<0 && ret != -EUCLEAN ) + goto next_block;s/goto next_block/continue/
Quoted 8 lines
Quoted 3 lines
hmmm, OK.
I copied the original code ie for listing images from nor flash. Should I also correct it !!If you want to do that as a separate patch, that's fine -- but the code is sufficiently different that I don't think there's a strong consistency argument to be made.
Check if it is OK in v3
Quoted 24 lines
Quoted 19 lines
Quoted 9 lines
Quoted 9 lines
common/cmd_bootm.c +8
-0+ header = (const image_header_t *)buffer; + +switch (genimg_get_format (buffer)) { +case IMAGE_FORMAT_LEGACY : +if (!image_check_hcrc (header)) + goto next_block; + + read_size =
image_get_image_size(header);Quoted 9 lines
common/cmd_bootm.c next_sector: ; · line 1282
Quoted 6 lines
common/cmd_bootm.c +4
-0+ + imgdata =malloc (read_size); +if (!imgdata) { + printf("Not able to list allimages " \
common/cmd_bootm.c +1
-0+ "(Low memory)\n");Don't line-wrap error strings.
Quoted 1 line
80 column ?
Error strings are an exception for the sake of greppability. From Linux's
Documentation/CodingStyle:
Quoted 12 lines
Statements longer than 80 columns will be broken into sensible
chunks, unless
exceeding 80 columns significantly increases readability and does
not hide
information. Descendants are always substantially shorter than the
parent and
are placed substantially to the right. The same applies to function
headers
with a long argument list. However, never break user-visible strings
such as
printk messages, because that breaks the ability to grep for them.
Yes, thanks for reminding. The error strings are more readable already in v3. Please take a look
Quoted 17 lines
Quoted 9 lines
Quoted 8 lines
Why is the no-memory error message different for FIT versus legacy images? I realize that at this point you don't know if it's a FIT versus some other dtb, but why do you print the device and offset here
but not in the legacy case? Why "Low memory(cannot allocate memory for
image)" versus just " (Low memory)"?Quoted 7 lines
Typo :(
I would give the following print for bothprintf("May be a FIT Image at NAND " \ "device %d offset %08llX:\n", nand_dev, off); printf(" Low memory(cannot allocate" \ " memory for image)\n");
Quoted 3 lines
It's a little more verbose than I'd have done, but OK. Can you put a space between "memory" and "(cannot", though?
Sure. I will do that in v4
-Vipin
PATCH v2 1/1 imls: Add support to list images in NAND device
Quoted 33 lines · Vipin Kumar
Quoted 15 lines · Scott Wood
Quoted 10 lines · Vipin Kumar
Quoted 8 lines
common/cmd_bootm.c next_sector: ; · line 1282
Quoted 5 lines
common/cmd_bootm.c +3
-0+ imgdata =malloc (read_size); +if (!imgdata) { + printf("Not able to list allimages " \
common/cmd_bootm.c +1
-0+ "(Low memory)\n");Don't line-wrap error strings.
80 column ?
Error strings are an exception for the sake of greppability. From Linux's
Documentation/CodingStyle:Quoted 16 lines
Statements longer than 80 columns will be broken into sensible
chunks, unless
exceeding 80 columns significantly increases readability and does
not hide
information. Descendants are always substantially shorter than
the
parent and
are placed substantially to the right. The same applies to
function
headers
with a long argument list. However, never break user-visible
strings
such as
printk messages, because that breaks the ability to grep for
them.
Quoted 2 lines
Yes, thanks for reminding. The error strings are more readable already in v3. Please take a look
No, you're still breaking up strings (and you also have a totally unnecessary backslash). If it's on one line in the output, it should be on one line in the source.
-Scott
PATCH v2 1/1 imls: Add support to list images in NAND device
Quoted 34 lines · Scott Wood
Quoted 33 lines · Vipin Kumar
Quoted 31 lines · Scott Wood
Quoted 10 lines · Vipin Kumar
Quoted 8 lines
common/cmd_bootm.c next_sector: ; · line 1282
Quoted 5 lines
common/cmd_bootm.c +3
-0+ imgdata =malloc (read_size); +if (!imgdata) { + printf("Not able to list allimages " \
common/cmd_bootm.c +1
-0+ "(Low memory)\n");Don't line-wrap error strings.
80 column ?
Error strings are an exception for the sake of greppability. From Linux's
Documentation/CodingStyle:Statements longer than 80 columns will be broken into sensible
chunks, unless
exceeding 80 columns significantly increases readability and does
not hide
information. Descendants are always substantially shorter than
the
parent and
are placed substantially to the right. The same applies to
function
headers
with a long argument list. However, never break user-visible
strings
such as
printk messages, because that breaks the ability to grep for
them.
Quoted 7 lines
Quoted 2 lines
Yes, thanks for reminding. The error strings are more readable already in v3. Please take a look
No, you're still breaking up strings (and you also have a totally unnecessary backslash). If it's on one line in the output, it should be on one line in the source.
Yes, got it. Please check v4. I will send it out soon
Quoted 2 lines
-Scott