Re: [U-Boot] [PATCH] [UBI] UBI command support (take #2)
On Wednesday 19 November 2008, Kyungmin Park wrote: > > Could you please summarize the current state of adding UBI support? > > Not much as before, > > 1. Basic UBI command support. >read/write/update volume. Of course you can create & remove the volume. > 2. Send ubi device from environment variables instead of hard code value. > > I saw the NOR device support patch but not yet tested. > > Are there the more? > > > Is it correct to assume we are targetting the 2008.12 release with > > this stuff? > > Yes, I hope to merge it ASAP. I'll post some last-minute UBI fixup patches soon (this week). Best regards, Stefan = DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: [EMAIL PROTECTED] = ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] [UBI] UBI command support (take #2)
On Wed, Nov 19, 2008 at 6:17 AM, Wolfgang Denk <[EMAIL PROTECTED]> wrote: > Dear Kyungmin, dear Stefan, > > in message <[EMAIL PROTECTED]> you wrote: >> It supports basic operation such as create, remove, read, and write. > > Could you please summarize the current state of adding UBI support? > Not much as before, 1. Basic UBI command support. read/write/update volume. Of course you can create & remove the volume. 2. Send ubi device from environment variables instead of hard code value. I saw the NOR device support patch but not yet tested. Are there the more? > Is it correct to assume we are targetting the 2008.12 release with > this stuff? > Yes, I hope to merge it ASAP. Thank you, Kyungmin Park ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] [UBI] UBI command support (take #2)
Dear Kyungmin, dear Stefan, in message <[EMAIL PROTECTED]> you wrote: > It supports basic operation such as create, remove, read, and write. Could you please summarize the current state of adding UBI support? Is it correct to assume we are targetting the 2008.12 release with this stuff? 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: [EMAIL PROTECTED] Machines take me by surprise with great frequency. - Alan Turing ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] [UBI] UBI command support v3
On Mon, Nov 3, 2008 at 6:01 AM, Magnus Lilja <[EMAIL PROTECTED]> wrote: > Dear Kyungmin Park, > > Some more comments. > > 2008/10/28 Kyungmin Park <[EMAIL PROTECTED]>: >> It supports basic operation such as create, remove, read, and write. >> >> Signed-off-by: Kyungmin Park <[EMAIL PROTECTED]> >> --- >> diff --git a/common/cmd_ubi.c b/common/cmd_ubi.c >> new file mode 100644 >> index 000..8c46cca >> --- /dev/null >> +++ b/common/cmd_ubi.c >> +static int ubi_volume_read(char *volume, char *buf, size_t size) >> +{ >> + int err, lnum, off, len, tbuf_size, i = 0; >> + size_t count_save = size; >> + void *tbuf; >> + unsigned long long tmp; >> + struct ubi_volume *vol = NULL; >> + loff_t offp = 0; >> + >> + for (i = 0; i < ubi->vtbl_slots; i++) { >> + vol = ubi->volumes[i]; >> + if (vol && !strcmp(vol->name, volume)) { >> + printf("Volume %s found at volume id %d\n", >> + volume, vol->vol_id); >> + break; >> + } >> + } >> + if (i == ubi->vtbl_slots) { >> + printf("%s voume not found\n", volume); >> + return 0; >> + } >> + >> + printf("read %i bytes from volume %d to %x(buf address)\n", >> + (int) size, vol->vol_id, (unsigned)buf); >> + >> + if (vol->updating) { >> + printf("updating"); >> + return -EBUSY; >> + } >> + if (vol->upd_marker) { >> + printf("damaged volume, update marker is set"); >> + return -EBADF; >> + } >> + if (offp == vol->used_bytes) >> + return 0; >> + >> + if (size == 0) { >> + printf("Read [%lu] bytes\n", (unsigned long) >> vol->used_bytes); >> + size = vol->used_bytes; >> + } >> + >> + if (vol->corrupted) >> + printf("read from corrupted volume %d", vol->vol_id); >> + if (offp + size > vol->used_bytes) >> + count_save = size = vol->used_bytes - offp; >> + >> + tbuf_size = vol->usable_leb_size; >> + if (size < tbuf_size) >> + tbuf_size = ALIGN(size, ubi->min_io_size); >> + tbuf = malloc(tbuf_size); >> + if (!tbuf) { >> + printf("NO MEM\n"); >> + return -ENOMEM; >> + } >> + len = size > tbuf_size ? tbuf_size : size; >> + >> + tmp = offp; >> + off = do_div(tmp, vol->usable_leb_size); >> + lnum = tmp; >> + printf("off=%d lnum=%d\n", off, lnum); >> + do { >> + if (off + len >= vol->usable_leb_size) >> + len = vol->usable_leb_size - off; >> + >> + err = ubi_eba_read_leb(ubi, vol, lnum, tbuf, off, len, 0); >> + if (err) { >> + printf("read err %x\n", err); >> + break; >> + } >> + off += len; >> + if (off == vol->usable_leb_size) { >> + lnum += 1; >> + off -= vol->usable_leb_size; >> + } >> + >> + size -= len; >> + offp += len; >> + >> + printf("buf = %x\n", (unsigned)buf); >> + memcpy(buf, tbuf, len); >> + printf("buf[0] = %x\n", buf[0]); > > The printf()'s before and after memcpy() looks like stuff used during > development of this file and they should be removed. Yes, correct, I'll remove it > >> + >> + buf += len; >> + len = size > tbuf_size ? tbuf_size : size; >> + } while (size); >> + >> + free(tbuf); >> + return err ? err : count_save - size; >> +} >> + >> +static int do_ubi(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) >> +{ >> + size_t size = 0; >> + ulong addr = 0; >> + int err = 0; >> + >> + if (!ubi_initialized) { >> + err = ubi_board_scan(); >> + if (err) { >> + printf("UBI init error %d\n", err); >> + return err; >> + } >> + ubi = ubi_devices[0]; >> + ubi_initialized = 1; >> + } >> + >> + if (argc < 2) { >> + printf("Usage:\n%s\n", cmdtp->usage); >> + return 1; >> + } >> + if (strcmp(argv[1], "info") == 0) { >> + int layout = 0; >> + if (argc > 2 && !strncmp(argv[2], "l", 1)) >> + layout = 1; >> + return ubi_info(layout); >> + } >> + if (strncmp(argv[1], "create", 6) == 0) { >> + int dynamic = 1;/* default: dynamic volume */ >> + >> + /* Use maximum available size */ >> + size = 0; >> + >> + /* E.g., create volume size type */ >> + if (argc == 5) { >> + if (strncmp(argv[4], "s", 1) == 0) >> +
Re: [U-Boot] [PATCH] [UBI] UBI command support v3
Dear Kyungmin Park, Some more comments. 2008/10/28 Kyungmin Park <[EMAIL PROTECTED]>: > It supports basic operation such as create, remove, read, and write. > > Signed-off-by: Kyungmin Park <[EMAIL PROTECTED]> > --- > diff --git a/common/cmd_ubi.c b/common/cmd_ubi.c > new file mode 100644 > index 000..8c46cca > --- /dev/null > +++ b/common/cmd_ubi.c > +static int ubi_volume_read(char *volume, char *buf, size_t size) > +{ > + int err, lnum, off, len, tbuf_size, i = 0; > + size_t count_save = size; > + void *tbuf; > + unsigned long long tmp; > + struct ubi_volume *vol = NULL; > + loff_t offp = 0; > + > + for (i = 0; i < ubi->vtbl_slots; i++) { > + vol = ubi->volumes[i]; > + if (vol && !strcmp(vol->name, volume)) { > + printf("Volume %s found at volume id %d\n", > + volume, vol->vol_id); > + break; > + } > + } > + if (i == ubi->vtbl_slots) { > + printf("%s voume not found\n", volume); > + return 0; > + } > + > + printf("read %i bytes from volume %d to %x(buf address)\n", > + (int) size, vol->vol_id, (unsigned)buf); > + > + if (vol->updating) { > + printf("updating"); > + return -EBUSY; > + } > + if (vol->upd_marker) { > + printf("damaged volume, update marker is set"); > + return -EBADF; > + } > + if (offp == vol->used_bytes) > + return 0; > + > + if (size == 0) { > + printf("Read [%lu] bytes\n", (unsigned long) vol->used_bytes); > + size = vol->used_bytes; > + } > + > + if (vol->corrupted) > + printf("read from corrupted volume %d", vol->vol_id); > + if (offp + size > vol->used_bytes) > + count_save = size = vol->used_bytes - offp; > + > + tbuf_size = vol->usable_leb_size; > + if (size < tbuf_size) > + tbuf_size = ALIGN(size, ubi->min_io_size); > + tbuf = malloc(tbuf_size); > + if (!tbuf) { > + printf("NO MEM\n"); > + return -ENOMEM; > + } > + len = size > tbuf_size ? tbuf_size : size; > + > + tmp = offp; > + off = do_div(tmp, vol->usable_leb_size); > + lnum = tmp; > + printf("off=%d lnum=%d\n", off, lnum); > + do { > + if (off + len >= vol->usable_leb_size) > + len = vol->usable_leb_size - off; > + > + err = ubi_eba_read_leb(ubi, vol, lnum, tbuf, off, len, 0); > + if (err) { > + printf("read err %x\n", err); > + break; > + } > + off += len; > + if (off == vol->usable_leb_size) { > + lnum += 1; > + off -= vol->usable_leb_size; > + } > + > + size -= len; > + offp += len; > + > + printf("buf = %x\n", (unsigned)buf); > + memcpy(buf, tbuf, len); > + printf("buf[0] = %x\n", buf[0]); The printf()'s before and after memcpy() looks like stuff used during development of this file and they should be removed. > + > + buf += len; > + len = size > tbuf_size ? tbuf_size : size; > + } while (size); > + > + free(tbuf); > + return err ? err : count_save - size; > +} > + > +static int do_ubi(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) > +{ > + size_t size = 0; > + ulong addr = 0; > + int err = 0; > + > + if (!ubi_initialized) { > + err = ubi_board_scan(); > + if (err) { > + printf("UBI init error %d\n", err); > + return err; > + } > + ubi = ubi_devices[0]; > + ubi_initialized = 1; > + } > + > + if (argc < 2) { > + printf("Usage:\n%s\n", cmdtp->usage); > + return 1; > + } > + if (strcmp(argv[1], "info") == 0) { > + int layout = 0; > + if (argc > 2 && !strncmp(argv[2], "l", 1)) > + layout = 1; > + return ubi_info(layout); > + } > + if (strncmp(argv[1], "create", 6) == 0) { > + int dynamic = 1;/* default: dynamic volume */ > + > + /* Use maximum available size */ > + size = 0; > + > + /* E.g., create volume size type */ > + if (argc == 5) { > + if (strncmp(argv[4], "s", 1) == 0) > + dynamic = 0; > + else if (strncmp(argv[4], "d", 1) != 0) { > + printf("Incorrect type\n"); > + return 1; > + } > +
Re: [U-Boot] [PATCH] [UBI] UBI command support v3
Hi, On Sat, Nov 1, 2008 at 1:19 AM, Magnus Lilja <[EMAIL PROTECTED]> wrote: > 2008/10/28 Kyungmin Park <[EMAIL PROTECTED]>: >> It supports basic operation such as create, remove, read, and write. >> >> Signed-off-by: Kyungmin Park <[EMAIL PROTECTED]> > > Would it be possible to have the U-boot environment in the part of the > flash managed by UBI? I suppose it involves creating a U-Boot > Environment volume in UBI and then using ubi function to read/update > that volume. Principally yes. But not yet used This implementation only used kernel side UBI, rootfs, user data area. However the UBI supports multiple UBI devices. It cant combine several UBI device such as params UBI, kernel UBI, and rootfs UBI. Of course it depends on your configurations I will list the TODO list this one. Thank you, Kyungmin Park ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] [UBI] UBI command support v3
2008/10/28 Kyungmin Park <[EMAIL PROTECTED]>: > It supports basic operation such as create, remove, read, and write. > > Signed-off-by: Kyungmin Park <[EMAIL PROTECTED]> Would it be possible to have the U-boot environment in the part of the flash managed by UBI? I suppose it involves creating a U-Boot Environment volume in UBI and then using ubi function to read/update that volume. Regards, Magnus ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] [UBI] UBI command support
Dear Kyungmin Park, In message <[EMAIL PROTECTED]> you wrote: ... > >> + printf("Unknown UBI command or invalid number of arguments\n"); > > > > Print usage message instead. > > How to display usage? printf ("Usage:\n%s\n", cmdtp->usage); 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: [EMAIL PROTECTED] Misquotation is, in fact, the pride and privilege of the learned. A widely-read man never quotes accurately, for the rather obvious reason that he has read too widely. - Hesketh Pearson _Common Misquotations_ introduction ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] [UBI] UBI command support
Dear Wolfgang, On Tue, Oct 21, 2008 at 7:24 PM, Wolfgang Denk <[EMAIL PROTECTED]> wrote: > Dear Kyungmin Park, > > In message <[EMAIL PROTECTED]> you wrote: >> UBI command support >> >> Signed-off-by: Kyungmin Park <[EMAIL PROTECTED]> > ... >> +++ b/common/cmd_ubi.c >> @@ -0,0 +1,535 @@ >> +/* >> + * Unsorted Block Image commands >> + */ >> + >> +#include >> +#include > > GPL header missing. Okay I added. > >> +/* board/\*\/ubi.c */ > > What's the '\'s there? > >> +extern int ubi_board_scan(void); > > Please move the prototypes to some header file. > >> +/* drivers/mtd/ubi/build.c */ >> +extern struct ubi_device *ubi_devices[]; Moved to proper header files. >> + >> +/* Private own data */ >> +static struct ubi_device *ubi; >> +static int ubi_initialized; >> + >> +static void ubi_dump_vol_info(const struct ubi_volume *vol) >> +{ >> + dbg_msg("volume information dump:"); >> + dbg_msg("vol_id %d", vol->vol_id); >> + dbg_msg("reserved_pebs %d", vol->reserved_pebs); >> + dbg_msg("alignment %d", vol->alignment); >> + dbg_msg("data_pad%d", vol->data_pad); >> + dbg_msg("vol_type%d", vol->vol_type); >> + dbg_msg("name_len%d", vol->name_len); >> + dbg_msg("usable_leb_size %d", vol->usable_leb_size); >> + dbg_msg("used_ebs%d", vol->used_ebs); >> + dbg_msg("used_bytes %lld", vol->used_bytes); >> + dbg_msg("last_eb_bytes %d", vol->last_eb_bytes); >> + dbg_msg("corrupted %d", vol->corrupted); >> + dbg_msg("upd_marker %d", vol->upd_marker); > > Please use the standard debug() macro. It's not debug message, it's ubi msg. I fixed. > >> +static int ustrtoul(const char *cp, char **endp, unsigned int base) > > Maybe we should add this to some global place, like lib_generic ? Move to lib_generic/vsprintf.c > >> +unsigned long result = simple_strtoul(cp, endp, base); >> +switch (**endp) { >> +case 'G' : >> +result *= 1024; > > Please add a >/* fall through */ > comment to make clkear that it is intentional not to have a "break;" > here. > >> +case 'M': >> +result *= 1024; > > Ditto. Did > >> +static int verify_mkvol_req(const struct ubi_device *ubi, >> + const struct ubi_mkvol_req *req) >> +{ > ... >> +bad: >> + printf("bad volume creation request"); >> +// ubi_dbg_dump_mkvol_req(req); >> + return err; > > No C++ comments, please. And please don;t add dead code either. Yes I remove it > >> + tbuf = vmalloc(tbuf_size); > > Why do we need new alloc() stuff? > >> + >> + vfree(tbuf); > > Ditto? Use malloc & free. > >> +static int do_ubi(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) >> +{ >> + size_t size = 0; >> + ulong addr = 0; >> + int err = 0; >> + >> + if (!ubi_initialized) { >> + err = ubi_board_scan(); >> + if (err) { >> + printf("ubi initialize error %d\n", err); > > "UBI init error" -- maybe we can decode the error number into some > string? > >> + /* E.g., create volume size type */ >> + if (argc == 5) { >> + if (strncmp(argv[4], "s", 1) == 0) >> + dynamic = 0; >> + else if (strncmp(argv[4], "d", 1) != 0) { >> + printf("You give wrong type\n"); > > "Incorrect type". It would also be helpful if the incorrect parameter > gets printed, so the user sees what he passed. > >> + return 1; >> + } >> + argc--; >> + } >> + /* E.g., create volume size */ >> + if (argc == 4) { >> + err = parse_num(&size, argv[3]); >> + if (err) { >> + printf("You give correct size\n"); > > "Incorrect size". Please also print incorrect argument value. > >> + if (strncmp(argv[1], "write", 5) == 0) { >> + if (argc < 5) { >> + printf("You give wrong parameters\n"); > > Print usage message instead. > >> + } >> + >> + addr = simple_strtoul(argv[2], NULL, 16); >> + err = parse_num(&size, argv[4]); >> + if (err) { >> + printf("You give wrong size\n"); > > See above. > >> + if (err) { >> + printf("You give wrong size\n"); > > Ditto. > >> + printf("Unknown UBI command or invalid number of arguments\n"); > > Print usage message instead. > How to display usage? Thank you, Kyungmin Park ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] [UBI] UBI command support
Dear Kyungmin Park, In message <[EMAIL PROTECTED]> you wrote: > UBI command support > > Signed-off-by: Kyungmin Park <[EMAIL PROTECTED]> ... > +++ b/common/cmd_ubi.c > @@ -0,0 +1,535 @@ > +/* > + * Unsorted Block Image commands > + */ > + > +#include > +#include GPL header missing. > +/* board/\*\/ubi.c */ What's the '\'s there? > +extern int ubi_board_scan(void); Please move the prototypes to some header file. > +/* drivers/mtd/ubi/build.c */ > +extern struct ubi_device *ubi_devices[]; > + > +/* Private own data */ > +static struct ubi_device *ubi; > +static int ubi_initialized; > + > +static void ubi_dump_vol_info(const struct ubi_volume *vol) > +{ > + dbg_msg("volume information dump:"); > + dbg_msg("vol_id %d", vol->vol_id); > + dbg_msg("reserved_pebs %d", vol->reserved_pebs); > + dbg_msg("alignment %d", vol->alignment); > + dbg_msg("data_pad%d", vol->data_pad); > + dbg_msg("vol_type%d", vol->vol_type); > + dbg_msg("name_len%d", vol->name_len); > + dbg_msg("usable_leb_size %d", vol->usable_leb_size); > + dbg_msg("used_ebs%d", vol->used_ebs); > + dbg_msg("used_bytes %lld", vol->used_bytes); > + dbg_msg("last_eb_bytes %d", vol->last_eb_bytes); > + dbg_msg("corrupted %d", vol->corrupted); > + dbg_msg("upd_marker %d", vol->upd_marker); Please use the standard debug() macro. > +static int ustrtoul(const char *cp, char **endp, unsigned int base) Maybe we should add this to some global place, like lib_generic ? > +unsigned long result = simple_strtoul(cp, endp, base); > +switch (**endp) { > +case 'G' : > +result *= 1024; Please add a /* fall through */ comment to make clkear that it is intentional not to have a "break;" here. > +case 'M': > +result *= 1024; Ditto. > +static int verify_mkvol_req(const struct ubi_device *ubi, > + const struct ubi_mkvol_req *req) > +{ ... > +bad: > + printf("bad volume creation request"); > +// ubi_dbg_dump_mkvol_req(req); > + return err; No C++ comments, please. And please don;t add dead code either. > + tbuf = vmalloc(tbuf_size); Why do we need new alloc() stuff? > + > + vfree(tbuf); Ditto? > +static int do_ubi(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) > +{ > + size_t size = 0; > + ulong addr = 0; > + int err = 0; > + > + if (!ubi_initialized) { > + err = ubi_board_scan(); > + if (err) { > + printf("ubi initialize error %d\n", err); "UBI init error" -- maybe we can decode the error number into some string? > + /* E.g., create volume size type */ > + if (argc == 5) { > + if (strncmp(argv[4], "s", 1) == 0) > + dynamic = 0; > + else if (strncmp(argv[4], "d", 1) != 0) { > + printf("You give wrong type\n"); "Incorrect type". It would also be helpful if the incorrect parameter gets printed, so the user sees what he passed. > + return 1; > + } > + argc--; > + } > + /* E.g., create volume size */ > + if (argc == 4) { > + err = parse_num(&size, argv[3]); > + if (err) { > + printf("You give correct size\n"); "Incorrect size". Please also print incorrect argument value. > + if (strncmp(argv[1], "write", 5) == 0) { > + if (argc < 5) { > + printf("You give wrong parameters\n"); Print usage message instead. > + } > + > + addr = simple_strtoul(argv[2], NULL, 16); > + err = parse_num(&size, argv[4]); > + if (err) { > + printf("You give wrong size\n"); See above. > + if (err) { > + printf("You give wrong size\n"); Ditto. > + printf("Unknown UBI command or invalid number of arguments\n"); Print usage message instead. 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: [EMAIL PROTECTED] A stone was placed at a ford in a river with the inscription: "When this stone is covered it is dangerous to ford here." ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot