From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Date: Wed, 12 Dec 2012 17:00:50 -0600 Subject: [U-Boot] [PATCH v2] imls: Add support to list images in NAND device In-Reply-To: (from vipin.kumar@st.com on Wed Dec 12 03:20:24 2012) Message-ID: <1355353250.28445.14@snotra> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > --- > 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 > +#include > + > #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