* [U-Boot] [PATCH v2] imls: Add support to list images in NAND device
@ 2012-12-12 9:20 Vipin Kumar
2012-12-12 20:24 ` Wolfgang Denk
2012-12-12 23:00 ` Scott Wood
0 siblings, 2 replies; 9+ messages in thread
From: Vipin Kumar @ 2012-12-12 9:20 UTC (permalink / raw)
To: u-boot
imls does not list the images in NAND devices. This patch implements this
support for legacy type images.
Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
---
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(-)
diff --git a/README b/README
index 2077c3b..ec5c31e 100644
--- a/README
+++ b/README
@@ -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
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index d256ddf..8ee562a 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -83,6 +83,9 @@ extern flash_info_t flash_info[]; /* info for FLASH chips */
static int do_imls(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
#endif
+#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)
{
flash_info_t *info;
int i, j;
@@ -1224,6 +1227,134 @@ next_sector: ;
}
next_bank: ;
}
+}
+#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];
+ 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;
+ }
+
+ printf("\n");
+
+ for (nand_dev = 0; nand_dev < CONFIG_SYS_MAX_NAND_DEVICE; nand_dev++) {
+
+ 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;
+
+ read_size = sizeof(buffer);
+
+ ret = nand_read(nand, off, &read_size, buffer);
+ if (ret < 0 && ret != -EUCLEAN)
+ goto next_block;
+
+ 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 all images " \
+ "(Low memory)\n");
+ goto next_block;
+ }
+
+ ret = nand_read_skip_bad(nand, off, &read_size,
+ imgdata);
+ if (ret < 0 && ret != -EUCLEAN) {
+ free(imgdata);
+ goto next_block;
+ }
+
+ printf("Legacy Image at NAND device %d " \
+ "offset %08llX:\n", nand_dev, off);
+ 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 at NAND " \
+ "device %d offset %08llX:\n",
+ nand_dev, off);
+ printf(" Low memory(cannot allocate" \
+ " memory for image)\n");
+ goto next_block;
+ }
+
+ ret = nand_read_skip_bad(nand, off, &read_size,
+ imgdata);
+ if (ret < 0 && ret != -EUCLEAN) {
+ free(imgdata);
+ goto next_block;
+ }
+
+ if (!fit_check_format(imgdata)) {
+ free(imgdata);
+ goto next_block;
+ }
+
+ printf("FIT Image@NAND device %d " \
+ "offset %08llX:\n", nand_dev, off);
+
+ fit_print_contents(imgdata);
+ free(imgdata);
+ break;
+#endif
+ default:
+ goto next_block;
+ }
+
+next_block: ;
+ }
+ }
+}
+#endif
+
+#if defined(CONFIG_CMD_IMLS) || defined(CONFIG_CMD_IMLS_NAND)
+static int do_imls(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+#if defined(CONFIG_CMD_IMLS)
+ do_imls_flash();
+#endif
+
+#if defined(CONFIG_CMD_IMLS_NAND)
+ do_imls_nand();
+#endif
return (0);
}
--
1.8.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH v2] imls: Add support to list images in NAND device
2012-12-12 9:20 [U-Boot] [PATCH v2] imls: Add support to list images in NAND device Vipin Kumar
@ 2012-12-12 20:24 ` Wolfgang Denk
2012-12-13 5:51 ` Vipin Kumar
2012-12-12 23:00 ` Scott Wood
1 sibling, 1 reply; 9+ messages in thread
From: Wolfgang Denk @ 2012-12-12 20:24 UTC (permalink / raw)
To: u-boot
Dear Vipin Kumar,
In message <dba14c4ffef38a108f75342968bcd9ce2b75c4c7.1355303894.git.vipin.kumar@st.com> you wrote:
> imls does not list the images in NAND devices. This patch implements this
> support for legacy type images.
...
> -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?
> +static void do_imls_nand(void)
Ditto.
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
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_
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH v2] imls: Add support to list images in NAND device
2012-12-12 9:20 [U-Boot] [PATCH v2] imls: Add support to list images in NAND device Vipin Kumar
2012-12-12 20:24 ` Wolfgang Denk
@ 2012-12-12 23:00 ` Scott Wood
2012-12-13 6:10 ` Vipin Kumar
1 sibling, 1 reply; 9+ messages in thread
From: Scott Wood @ 2012-12-12 23:00 UTC (permalink / raw)
To: u-boot
On 12/12/2012 03:20:24 AM, Vipin Kumar wrote:
> imls does not list the images in NAND devices. This patch implements
> this
> support for legacy type images.
>
> Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
> ---
> 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(-)
>
> diff --git a/README b/README
> index 2077c3b..ec5c31e 100644
> --- a/README
> +++ b/README
> @@ -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
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.
> CONFIG_CMD_IMMAP * IMMR dump support
> CONFIG_CMD_IMPORTENV * import an environment
> CONFIG_CMD_INI * import data from an ini file
> into the env
> diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
> index d256ddf..8ee562a 100644
> --- a/common/cmd_bootm.c
> +++ b/common/cmd_bootm.c
> @@ -83,6 +83,9 @@ extern flash_info_t flash_info[]; /* info for FLASH
> chips */
> static int do_imls(cmd_tbl_t *cmdtp, int flag, int argc, char *
> const argv[]);
> #endif
>
> +#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/
> {
> flash_info_t *info;
> int i, j;
> @@ -1224,6 +1227,134 @@ next_sector: ;
> }
> next_bank: ;
> }
> +}
> +#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?
> + 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.
> +
> + 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.
> + 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;
> +
> + 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/
> + 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 all
> images " \
> + "(Low memory)\n");
Don't line-wrap error strings.
> + goto next_block;
> + }
> +
> + ret = nand_read_skip_bad(nand, off,
> &read_size,
> + 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.
> +
> + printf("Legacy Image at NAND device %d
> " \
> + "offset %08llX:\n", nand_dev,
> off);
> + 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 at
> NAND " \
> + "device %d offset
> %08llX:\n",
> + nand_dev, off);
> + printf(" Low memory(cannot
> allocate" \
> + " memory for
> image)\n");
> + 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)"?
> + }
> +
> + ret = nand_read_skip_bad(nand, off,
> &read_size,
> + 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);
> +
> + fit_print_contents(imgdata);
> + free(imgdata);
> + break;
> +#endif
> + default:
> + goto next_block;
> + }
The default: doesn't do anything -- just leave it out.
> +
> +next_block: ;
> + }
Instead of using goto please factor out the switch to its own function
and use return.
-Scott
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH v2] imls: Add support to list images in NAND device
2012-12-12 20:24 ` Wolfgang Denk
@ 2012-12-13 5:51 ` Vipin Kumar
0 siblings, 0 replies; 9+ messages in thread
From: Vipin Kumar @ 2012-12-13 5:51 UTC (permalink / raw)
To: u-boot
On 12/13/2012 1:54 AM, Wolfgang Denk wrote:
> Dear Vipin Kumar,
>
> In message<dba14c4ffef38a108f75342968bcd9ce2b75c4c7.1355303894.git.vipin.kumar@st.com> you wrote:
>> imls does not list the images in NAND devices. This patch implements this
>> support for legacy type images.
> ...
>> -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
>> +static void do_imls_nand(void)
>
> Ditto.
>
>
> Best regards,
>
> Wolfgang Denk
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH v2] imls: Add support to list images in NAND device
2012-12-12 23:00 ` Scott Wood
@ 2012-12-13 6:10 ` Vipin Kumar
2012-12-13 21:52 ` Scott Wood
0 siblings, 1 reply; 9+ messages in thread
From: Vipin Kumar @ 2012-12-13 6:10 UTC (permalink / raw)
To: u-boot
[...]
>>
>> README | 3 +-
>> common/cmd_bootm.c | 133
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 134 insertions(+), 2 deletions(-)
>>
>> diff --git a/README b/README
>> index 2077c3b..ec5c31e 100644
>> --- a/README
>> +++ b/README
>> @@ -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
>
> s/in flash/in NOR flash/
> s/in NAND device/in NAND flash/
>
OK
> Or better, just have one CONFIG_CMD_IMLS and 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
>> CONFIG_CMD_IMMAP * IMMR dump support
>> CONFIG_CMD_IMPORTENV * import an environment
>> CONFIG_CMD_INI * import data from an ini file
>> into the env
>> diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
>> index d256ddf..8ee562a 100644
>> --- a/common/cmd_bootm.c
>> +++ b/common/cmd_bootm.c
>> @@ -83,6 +83,9 @@ extern flash_info_t flash_info[]; /* info for FLASH
>> chips */
>> static int do_imls(cmd_tbl_t *cmdtp, int flag, int argc, char *
>> const argv[]);
>> #endif
>>
>> +#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
>> {
>> flash_info_t *info;
>> int i, j;
>> @@ -1224,6 +1227,134 @@ next_sector: ;
>> }
>> next_bank: ;
>> }
>> +}
>> +#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?
>
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
>> + 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
> And this command seems to operate on all devices, not just the current
> one.
>
Yes, that's why a copy paste
>> +
>> + 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
>> + 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;
>> +
>> + 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 !!
>> + 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 all
>> images " \
>> + "(Low memory)\n");
>
> Don't line-wrap error strings.
>
80 column ?
>> + goto next_block;
>> + }
>> +
>> + ret = nand_read_skip_bad(nand, off,
>> &read_size,
>> + 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.
>
Didn't get that.
>> +
>> + printf("Legacy Image at NAND device %d
>> " \
>> + "offset %08llX:\n", nand_dev,
>> off);
>> + 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 at
>> NAND " \
>> + "device %d offset
>> %08llX:\n",
>> + nand_dev, off);
>> + printf(" Low memory(cannot
>> allocate" \
>> + " memory for
>> image)\n");
>> + 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");
>> + }
>> +
>> + ret = nand_read_skip_bad(nand, off,
>> &read_size,
>> + 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);
>> +
>> + fit_print_contents(imgdata);
>> + free(imgdata);
>> + break;
>> +#endif
>> + default:
>> + goto next_block;
>> + }
>
> The default: doesn't do anything -- just leave it out.
>
OK
>> +
>> +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
> -Scott
> .
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH v2] imls: Add support to list images in NAND device
2012-12-13 6:10 ` Vipin Kumar
@ 2012-12-13 21:52 ` Scott Wood
2012-12-14 9:23 ` Vipin Kumar
0 siblings, 1 reply; 9+ messages in thread
From: Scott Wood @ 2012-12-13 21:52 UTC (permalink / raw)
To: u-boot
On 12/13/2012 12:10:58 AM, Vipin Kumar wrote:
>> Or better, just have one CONFIG_CMD_IMLS and 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
Still, it might be better to fix that than to build upon a
no-longer-accurate assumption.
>>> +#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
>
> 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?
>>> + 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;
>>> +
>>> + 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 !!
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.
>>> + 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 all
>>> images " \
>>> + "(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.
>> 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");
It's a little more verbose than I'd have done, but OK. Can you put a
space between "memory" and "(cannot", though?
-Scott
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH v2] imls: Add support to list images in NAND device
2012-12-13 21:52 ` Scott Wood
@ 2012-12-14 9:23 ` Vipin Kumar
2012-12-14 18:59 ` Scott Wood
0 siblings, 1 reply; 9+ messages in thread
From: Vipin Kumar @ 2012-12-14 9:23 UTC (permalink / raw)
To: u-boot
On 12/14/2012 3:22 AM, Scott Wood wrote:
> On 12/13/2012 12:10:58 AM, Vipin Kumar wrote:
>>> Or better, just have one CONFIG_CMD_IMLS and 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
>
> Still, it might be better to fix that than to build upon a
> no-longer-accurate assumption.
>
OK, I would try that in v4
>>>> +#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
>>
>> 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
>>>> + 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;
>>>> +
>>>> + 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 !!
>
> 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
>>>> + 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 all
>>>> images " \
>>>> + "(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.
>
Yes, thanks for reminding. The error strings are more readable already
in v3. Please take a look
>>> 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");
>
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH v2] imls: Add support to list images in NAND device
2012-12-14 9:23 ` Vipin Kumar
@ 2012-12-14 18:59 ` Scott Wood
2012-12-17 8:18 ` Vipin Kumar
0 siblings, 1 reply; 9+ messages in thread
From: Scott Wood @ 2012-12-14 18:59 UTC (permalink / raw)
To: u-boot
On 12/14/2012 03:23:26 AM, Vipin Kumar wrote:
> On 12/14/2012 3:22 AM, Scott Wood wrote:
>> On 12/13/2012 12:10:58 AM, Vipin Kumar wrote:
>>>>> + imgdata = malloc(read_size);
>>>>> + if (!imgdata) {
>>>>> + printf("Not able to list all
>>>>> images " \
>>>>> + "(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.
>>
>
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH v2] imls: Add support to list images in NAND device
2012-12-14 18:59 ` Scott Wood
@ 2012-12-17 8:18 ` Vipin Kumar
0 siblings, 0 replies; 9+ messages in thread
From: Vipin Kumar @ 2012-12-17 8:18 UTC (permalink / raw)
To: u-boot
On 12/15/2012 12:29 AM, Scott Wood wrote:
> On 12/14/2012 03:23:26 AM, Vipin Kumar wrote:
>> On 12/14/2012 3:22 AM, Scott Wood wrote:
>>> On 12/13/2012 12:10:58 AM, Vipin Kumar wrote:
>>>>>> + imgdata = malloc(read_size);
>>>>>> + if (!imgdata) {
>>>>>> + printf("Not able to list all
>>>>>> images " \
>>>>>> + "(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.
>>>
>>
>> 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
> -Scott
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-12-17 8:18 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-12 9:20 [U-Boot] [PATCH v2] imls: Add support to list images in NAND device Vipin Kumar
2012-12-12 20:24 ` Wolfgang Denk
2012-12-13 5:51 ` Vipin Kumar
2012-12-12 23:00 ` Scott Wood
2012-12-13 6:10 ` Vipin Kumar
2012-12-13 21:52 ` Scott Wood
2012-12-14 9:23 ` Vipin Kumar
2012-12-14 18:59 ` Scott Wood
2012-12-17 8:18 ` Vipin Kumar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox