| Message ID | 1405323547-32588-3-git-send-email-hs@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Heiko Schocher |
| Headers | show |
On Mon, 2014-07-14 at 09:39 +0200, Heiko Schocher wrote: > move common functions from cmd_nand.c (for calculating offset > and size from cmdline paramter) to common place, so they could > used from other commands which use mtd partitions. > > For onenand the arg_off_size() is left in common/cmd_onenand.c. > It should use now the common arg_off() function, but as I could > not test onenand I let it there ... > > Signed-off-by: Heiko Schocher <hs@denx.de> > Cc: Scott Wood <scottwood@freescale.com> > Cc: Tom Rini <trini@ti.com> > --- > common/cmd_nand.c | 140 +++++++++--------------------------------------- > common/cmd_onenand.c | 19 +++---- > drivers/mtd/Makefile | 4 +- > drivers/mtd/mtd_uboot.c | 114 +++++++++++++++++++++++++++++++++++++++ > include/linux/mtd/mtd.h | 7 +++ > 5 files changed, 154 insertions(+), 130 deletions(-) > create mode 100644 drivers/mtd/mtd_uboot.c ACK the concept, some nits below. Have you given any more thought to formally taking over the custodianship? If not, Tom, are you expecting another pull request from me or were you going to apply Heiko's patches directly? > @@ -322,7 +213,12 @@ int do_nand_env_oob(cmd_tbl_t *cmdtp, int argc, char *const argv[]) > goto usage; > > /* We don't care about size, or maxsize. */ > - if (arg_off(argv[2], &idx, &addr, &maxsize, &maxsize)) { > + if (arg_off(argv[2], &idx, &addr, &maxsize, &maxsize, > + MTD_DEV_TYPE_NAND, nand_info[idx].size)) { > + puts("Offset or partition name expected\n"); > + return 1; > + } MTD_DEV_TYPE_NAND should be aligned with argv[2] (if you're going to do alignment rather than just a couple tabs), not arg_off. Likewise elsewhere. > diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c > new file mode 100644 > index 0000000..66e49c3 > --- /dev/null > +++ b/drivers/mtd/mtd_uboot.c > @@ -0,0 +1,114 @@ > +/* > + * (C) Copyright 2014 > + * Heiko Schocher, DENX Software Engineering, hs@denx.de. > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > +#include <common.h> > +#include <linux/mtd/mtd.h> > +#include <jffs2/jffs2.h> What do you use from the jffs2 header? > +inline int str2off(const char *p, loff_t *num) > +{ > + char *endptr; > + > + *num = simple_strtoull(p, &endptr, 16); > + return *p != '\0' && *endptr == '\0'; > +} > + > +inline int str2long(const char *p, ulong *num) > +{ > + char *endptr; > + > + *num = simple_strtoul(p, &endptr, 16); > + return *p != '\0' && *endptr == '\0'; > +} Should drop the inline -- it didn't make much sense in the old location as it wasn't in a header file (GCC can auto-inline where appropriate), and it makes even less sense if it's not static. -Scott
Hello Scott, Am 30.07.2014 02:29, schrieb Scott Wood: > On Mon, 2014-07-14 at 09:39 +0200, Heiko Schocher wrote: >> move common functions from cmd_nand.c (for calculating offset >> and size from cmdline paramter) to common place, so they could >> used from other commands which use mtd partitions. >> >> For onenand the arg_off_size() is left in common/cmd_onenand.c. >> It should use now the common arg_off() function, but as I could >> not test onenand I let it there ... >> >> Signed-off-by: Heiko Schocher<hs@denx.de> >> Cc: Scott Wood<scottwood@freescale.com> >> Cc: Tom Rini<trini@ti.com> >> --- >> common/cmd_nand.c | 140 +++++++++--------------------------------------- >> common/cmd_onenand.c | 19 +++---- >> drivers/mtd/Makefile | 4 +- >> drivers/mtd/mtd_uboot.c | 114 +++++++++++++++++++++++++++++++++++++++ >> include/linux/mtd/mtd.h | 7 +++ >> 5 files changed, 154 insertions(+), 130 deletions(-) >> create mode 100644 drivers/mtd/mtd_uboot.c > > ACK the concept, some nits below. > > Have you given any more thought to formally taking over the > custodianship? If not, Tom, are you expecting another pull request from > me or were you going to apply Heiko's patches directly? I thinking about it, yes, but I am really busy ... but it seems nobody else want to volunteer ... I discussed with Wolfgang, if it would make sense to create a mtd branch ... so, if you could not longer maintain the nand branch, we can set it to orphaned (in the hope, someone will volunteer ...) and I can also handle the nand patches ... but Wolfgang is currently on vacation, so this step maybe take a while ... >> @@ -322,7 +213,12 @@ int do_nand_env_oob(cmd_tbl_t *cmdtp, int argc, char *const argv[]) >> goto usage; >> >> /* We don't care about size, or maxsize. */ >> - if (arg_off(argv[2],&idx,&addr,&maxsize,&maxsize)) { >> + if (arg_off(argv[2],&idx,&addr,&maxsize,&maxsize, >> + MTD_DEV_TYPE_NAND, nand_info[idx].size)) { >> + puts("Offset or partition name expected\n"); >> + return 1; >> + } > > MTD_DEV_TYPE_NAND should be aligned with argv[2] (if you're going to do > alignment rather than just a couple tabs), not arg_off. Likewise > elsewhere. fixed. >> diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c >> new file mode 100644 >> index 0000000..66e49c3 >> --- /dev/null >> +++ b/drivers/mtd/mtd_uboot.c >> @@ -0,0 +1,114 @@ >> +/* >> + * (C) Copyright 2014 >> + * Heiko Schocher, DENX Software Engineering, hs@denx.de. >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> +#include<common.h> >> +#include<linux/mtd/mtd.h> >> +#include<jffs2/jffs2.h> > > What do you use from the jffs2 header? Yes, good question ... because missing common mtd defines, like: struct mtd_device { found in include/jffs2/load_kernel.h used in common/cmd_nand.c for example ... its really time to cleanup the mtd subsystem! Where to move this common defines? include/linux/mtd/mtd.h ? But before the mtd/ubi/ubifs sync patches are not in mainline, I do not want change this ... maybe it is okay to do this in a second step? >> +inline int str2off(const char *p, loff_t *num) >> +{ >> + char *endptr; >> + >> + *num = simple_strtoull(p,&endptr, 16); >> + return *p != '\0'&& *endptr == '\0'; >> +} >> + >> +inline int str2long(const char *p, ulong *num) >> +{ >> + char *endptr; >> + >> + *num = simple_strtoul(p,&endptr, 16); >> + return *p != '\0'&& *endptr == '\0'; >> +} > > Should drop the inline -- it didn't make much sense in the old location > as it wasn't in a header file (GCC can auto-inline where appropriate), > and it makes even less sense if it's not static. removed, thanks for the review! bye, Heiko
On Mon, 2014-08-11 at 10:40 +0200, Heiko Schocher wrote: > Hello Scott, > > Am 30.07.2014 02:29, schrieb Scott Wood: > > On Mon, 2014-07-14 at 09:39 +0200, Heiko Schocher wrote: > >> move common functions from cmd_nand.c (for calculating offset > >> and size from cmdline paramter) to common place, so they could > >> used from other commands which use mtd partitions. > >> > >> For onenand the arg_off_size() is left in common/cmd_onenand.c. > >> It should use now the common arg_off() function, but as I could > >> not test onenand I let it there ... > >> > >> Signed-off-by: Heiko Schocher<hs@denx.de> > >> Cc: Scott Wood<scottwood@freescale.com> > >> Cc: Tom Rini<trini@ti.com> > >> --- > >> common/cmd_nand.c | 140 +++++++++--------------------------------------- > >> common/cmd_onenand.c | 19 +++---- > >> drivers/mtd/Makefile | 4 +- > >> drivers/mtd/mtd_uboot.c | 114 +++++++++++++++++++++++++++++++++++++++ > >> include/linux/mtd/mtd.h | 7 +++ > >> 5 files changed, 154 insertions(+), 130 deletions(-) > >> create mode 100644 drivers/mtd/mtd_uboot.c > > > > ACK the concept, some nits below. > > > > Have you given any more thought to formally taking over the > > custodianship? If not, Tom, are you expecting another pull request from > > me or were you going to apply Heiko's patches directly? > > I thinking about it, yes, but I am really busy ... but it seems > nobody else want to volunteer ... I discussed with Wolfgang, if > it would make sense to create a mtd branch ... so, if you could not > longer maintain the nand branch, we can set it to orphaned (in the > hope, someone will volunteer ...) and I can also handle the nand > patches ... but Wolfgang is currently on vacation, so this step > maybe take a while ... OK. I can still review things now and then, but it's getting hard to be responsive to the extent expected of a custodian. > >> diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c > >> new file mode 100644 > >> index 0000000..66e49c3 > >> --- /dev/null > >> +++ b/drivers/mtd/mtd_uboot.c > >> @@ -0,0 +1,114 @@ > >> +/* > >> + * (C) Copyright 2014 > >> + * Heiko Schocher, DENX Software Engineering, hs@denx.de. > >> + * > >> + * SPDX-License-Identifier: GPL-2.0+ > >> + */ > >> +#include<common.h> > >> +#include<linux/mtd/mtd.h> > >> +#include<jffs2/jffs2.h> > > > > What do you use from the jffs2 header? > > Yes, good question ... because missing common mtd defines, like: > > struct mtd_device { > > found in include/jffs2/load_kernel.h > used in common/cmd_nand.c for example ... its really time to > cleanup the mtd subsystem! > > Where to move this common defines? include/linux/mtd/mtd.h ? > > But before the mtd/ubi/ubifs sync patches are not in mainline, I do > not want change this ... maybe it is okay to do this in a second step? Sure. -Scott
diff --git a/common/cmd_nand.c b/common/cmd_nand.c index f9ced9d..d6232e3 100644 --- a/common/cmd_nand.c +++ b/common/cmd_nand.c @@ -133,115 +133,6 @@ static int set_dev(int dev) return 0; } -static inline int str2off(const char *p, loff_t *num) -{ - char *endptr; - - *num = simple_strtoull(p, &endptr, 16); - return *p != '\0' && *endptr == '\0'; -} - -static inline int str2long(const char *p, ulong *num) -{ - char *endptr; - - *num = simple_strtoul(p, &endptr, 16); - return *p != '\0' && *endptr == '\0'; -} - -static int get_part(const char *partname, int *idx, loff_t *off, loff_t *size, - loff_t *maxsize) -{ -#ifdef CONFIG_CMD_MTDPARTS - struct mtd_device *dev; - struct part_info *part; - u8 pnum; - int ret; - - ret = mtdparts_init(); - if (ret) - return ret; - - ret = find_dev_and_part(partname, &dev, &pnum, &part); - if (ret) - return ret; - - if (dev->id->type != MTD_DEV_TYPE_NAND) { - puts("not a NAND device\n"); - return -1; - } - - *off = part->offset; - *size = part->size; - *maxsize = part->size; - *idx = dev->id->num; - - ret = set_dev(*idx); - if (ret) - return ret; - - return 0; -#else - puts("offset is not a number\n"); - return -1; -#endif -} - -static int arg_off(const char *arg, int *idx, loff_t *off, loff_t *size, - loff_t *maxsize) -{ - if (!str2off(arg, off)) - return get_part(arg, idx, off, size, maxsize); - - if (*off >= nand_info[*idx].size) { - puts("Offset exceeds device limit\n"); - return -1; - } - - *maxsize = nand_info[*idx].size - *off; - *size = *maxsize; - return 0; -} - -static int arg_off_size(int argc, char *const argv[], int *idx, - loff_t *off, loff_t *size, loff_t *maxsize) -{ - int ret; - - if (argc == 0) { - *off = 0; - *size = nand_info[*idx].size; - *maxsize = *size; - goto print; - } - - ret = arg_off(argv[0], idx, off, size, maxsize); - if (ret) - return ret; - - if (argc == 1) - goto print; - - if (!str2off(argv[1], size)) { - printf("'%s' is not a number\n", argv[1]); - return -1; - } - - if (*size > *maxsize) { - puts("Size exceeds partition or device limit\n"); - return -1; - } - -print: - printf("device %d ", *idx); - if (*size == nand_info[*idx].size) - puts("whole chip\n"); - else - printf("offset 0x%llx, size 0x%llx\n", - (unsigned long long)*off, (unsigned long long)*size); - return 0; -} - #ifdef CONFIG_CMD_NAND_LOCK_UNLOCK static void print_status(ulong start, ulong end, ulong erasesize, int status) { @@ -322,7 +213,12 @@ int do_nand_env_oob(cmd_tbl_t *cmdtp, int argc, char *const argv[]) goto usage; /* We don't care about size, or maxsize. */ - if (arg_off(argv[2], &idx, &addr, &maxsize, &maxsize)) { + if (arg_off(argv[2], &idx, &addr, &maxsize, &maxsize, + MTD_DEV_TYPE_NAND, nand_info[idx].size)) { + puts("Offset or partition name expected\n"); + return 1; + } + if (set_dev(idx)) { puts("Offset or partition name expected\n"); return 1; } @@ -592,7 +488,10 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) printf("\nNAND %s: ", cmd); /* skip first two or three arguments, look for offset and size */ if (arg_off_size(argc - o, argv + o, &dev, &off, &size, - &maxsize) != 0) + &maxsize, MTD_DEV_TYPE_NAND, nand_info[dev].size) != 0) + return 1; + + if (set_dev(dev)) return 1; nand = &nand_info[dev]; @@ -654,7 +553,11 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (s && !strcmp(s, ".raw")) { raw = 1; - if (arg_off(argv[3], &dev, &off, &size, &maxsize)) + if (arg_off(argv[3], &dev, &off, &size, &maxsize, + MTD_DEV_TYPE_NAND, nand_info[dev].size)) + return 1; + + if (set_dev(dev)) return 1; if (argc > 4 && !str2long(argv[4], &pagecount)) { @@ -669,8 +572,12 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) rwsize = pagecount * (nand->writesize + nand->oobsize); } else { - if (arg_off_size(argc - 3, argv + 3, &dev, - &off, &size, &maxsize) != 0) + if (arg_off_size(argc - 3, argv + 3, &dev, &off, &size, + &maxsize, MTD_DEV_TYPE_NAND, + nand_info[dev].size) != 0) + return 1; + + if (set_dev(dev)) return 1; /* size is unspecified */ @@ -816,7 +723,10 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) allexcept = 1; if (arg_off_size(argc - 2, argv + 2, &dev, &off, &size, - &maxsize) < 0) + &maxsize, MTD_DEV_TYPE_NAND, nand_info[dev].size) < 0) + return 1; + + if (set_dev(dev)) return 1; if (!nand_unlock(&nand_info[dev], off, size, allexcept)) { diff --git a/common/cmd_onenand.c b/common/cmd_onenand.c index 06cc140..feab01a 100644 --- a/common/cmd_onenand.c +++ b/common/cmd_onenand.c @@ -24,15 +24,8 @@ static struct mtd_info *mtd; static loff_t next_ofs; static loff_t skip_ofs; -static inline int str2long(char *p, ulong *num) -{ - char *endptr; - - *num = simple_strtoul(p, &endptr, 16); - return (*p != '\0' && *endptr == '\0') ? 1 : 0; -} - -static int arg_off_size(int argc, char * const argv[], ulong *off, size_t *size) +static int arg_off_size_onenand(int argc, char * const argv[], ulong *off, + size_t *size) { if (argc >= 1) { if (!(str2long(argv[0], off))) { @@ -399,7 +392,7 @@ static int do_onenand_read(cmd_tbl_t * cmdtp, int flag, int argc, char * const a addr = (ulong)simple_strtoul(argv[1], NULL, 16); printf("\nOneNAND read: "); - if (arg_off_size(argc - 2, argv + 2, &ofs, &len) != 0) + if (arg_off_size_onenand(argc - 2, argv + 2, &ofs, &len) != 0) return 1; ret = onenand_block_read(ofs, len, &retlen, (u8 *)addr, oob); @@ -425,7 +418,7 @@ static int do_onenand_write(cmd_tbl_t * cmdtp, int flag, int argc, char * const addr = (ulong)simple_strtoul(argv[1], NULL, 16); printf("\nOneNAND write: "); - if (arg_off_size(argc - 2, argv + 2, &ofs, &len) != 0) + if (arg_off_size_onenand(argc - 2, argv + 2, &ofs, &len) != 0) return 1; ret = onenand_block_write(ofs, len, &retlen, (u8 *)addr, withoob); @@ -461,7 +454,7 @@ static int do_onenand_erase(cmd_tbl_t * cmdtp, int flag, int argc, char * const printf("\nOneNAND erase: "); /* skip first two or three arguments, look for offset and size */ - if (arg_off_size(argc, argv, &ofs, &len) != 0) + if (arg_off_size_onenand(argc, argv, &ofs, &len) != 0) return 1; ret = onenand_block_erase(ofs, len, force); @@ -486,7 +479,7 @@ static int do_onenand_test(cmd_tbl_t * cmdtp, int flag, int argc, char * const a printf("\nOneNAND test: "); /* skip first two or three arguments, look for offset and size */ - if (arg_off_size(argc - 1, argv + 1, &ofs, &len) != 0) + if (arg_off_size_onenand(argc - 1, argv + 1, &ofs, &len) != 0) return 1; ret = onenand_block_test(ofs, len); diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile index 5467a951..a623f4c 100644 --- a/drivers/mtd/Makefile +++ b/drivers/mtd/Makefile @@ -5,8 +5,8 @@ # SPDX-License-Identifier: GPL-2.0+ # -ifneq (,$(findstring y,$(CONFIG_MTD_DEVICE)$(CONFIG_CMD_NAND)$(CONFIG_CMD_ONENAND))) -obj-y += mtdcore.o +ifneq (,$(findstring y,$(CONFIG_MTD_DEVICE)$(CONFIG_CMD_NAND)$(CONFIG_CMD_ONENAND)$(CONFIG_CMD_SF))) +obj-y += mtdcore.o mtd_uboot.o endif obj-$(CONFIG_MTD_PARTITIONS) += mtdpart.o obj-$(CONFIG_MTD_CONCAT) += mtdconcat.o diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c new file mode 100644 index 0000000..66e49c3 --- /dev/null +++ b/drivers/mtd/mtd_uboot.c @@ -0,0 +1,114 @@ +/* + * (C) Copyright 2014 + * Heiko Schocher, DENX Software Engineering, hs@denx.de. + * + * SPDX-License-Identifier: GPL-2.0+ + */ +#include <common.h> +#include <linux/mtd/mtd.h> +#include <jffs2/jffs2.h> + +inline int str2off(const char *p, loff_t *num) +{ + char *endptr; + + *num = simple_strtoull(p, &endptr, 16); + return *p != '\0' && *endptr == '\0'; +} + +inline int str2long(const char *p, ulong *num) +{ + char *endptr; + + *num = simple_strtoul(p, &endptr, 16); + return *p != '\0' && *endptr == '\0'; +} + +static int get_part(const char *partname, int *idx, loff_t *off, loff_t *size, + loff_t *maxsize, int devtype) +{ +#ifdef CONFIG_CMD_MTDPARTS + struct mtd_device *dev; + struct part_info *part; + u8 pnum; + int ret; + + ret = mtdparts_init(); + if (ret) + return ret; + + ret = find_dev_and_part(partname, &dev, &pnum, &part); + if (ret) + return ret; + + if (dev->id->type != devtype) { + printf("not same typ %d != %d\n", dev->id->type, devtype); + return -1; + } + + *off = part->offset; + *size = part->size; + *maxsize = part->size; + *idx = dev->id->num; + + return 0; +#else + puts("offset is not a number\n"); + return -1; +#endif +} + +int arg_off(const char *arg, int *idx, loff_t *off, loff_t *size, + loff_t *maxsize, int devtype, int chipsize) +{ + if (!str2off(arg, off)) + return get_part(arg, idx, off, size, maxsize, devtype); + + if (*off >= chipsize) { + puts("Offset exceeds device limit\n"); + return -1; + } + + *maxsize = chipsize - *off; + *size = *maxsize; + return 0; +} + +int arg_off_size(int argc, char *const argv[], int *idx, loff_t *off, + loff_t *size, loff_t *maxsize, int devtype, int chipsize) +{ + int ret; + + if (argc == 0) { + *off = 0; + *size = chipsize; + *maxsize = *size; + goto print; + } + + ret = arg_off(argv[0], idx, off, size, maxsize, devtype, chipsize); + if (ret) + return ret; + + if (argc == 1) + goto print; + + if (!str2off(argv[1], size)) { + printf("'%s' is not a number\n", argv[1]); + return -1; + } + + if (*size > *maxsize) { + puts("Size exceeds partition or device limit\n"); + return -1; + } + +print: + printf("device %d ", *idx); + if (*size == chipsize) + puts("whole chip\n"); + else + printf("offset 0x%llx, size 0x%llx\n", + (unsigned long long)*off, (unsigned long long)*size); + return 0; +} diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index e6cfd55..8561b78 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -483,5 +483,12 @@ int add_mtd_device(struct mtd_info *mtd); int del_mtd_device(struct mtd_info *mtd); int add_mtd_partitions(struct mtd_info *, const struct mtd_partition *, int); int del_mtd_partitions(struct mtd_info *); + +int str2off(const char *p, loff_t *num); +int str2long(const char *p, ulong *num); +int arg_off(const char *arg, int *idx, loff_t *off, loff_t *size, + loff_t *maxsize, int devtype, int chipsize); +int arg_off_size(int argc, char *const argv[], int *idx, loff_t *off, + loff_t *size, loff_t *maxsize, int devtype, int chipsize); #endif #endif /* __MTD_MTD_H__ */
move common functions from cmd_nand.c (for calculating offset and size from cmdline paramter) to common place, so they could used from other commands which use mtd partitions. For onenand the arg_off_size() is left in common/cmd_onenand.c. It should use now the common arg_off() function, but as I could not test onenand I let it there ... Signed-off-by: Heiko Schocher <hs@denx.de> Cc: Scott Wood <scottwood@freescale.com> Cc: Tom Rini <trini@ti.com> --- common/cmd_nand.c | 140 +++++++++--------------------------------------- common/cmd_onenand.c | 19 +++---- drivers/mtd/Makefile | 4 +- drivers/mtd/mtd_uboot.c | 114 +++++++++++++++++++++++++++++++++++++++ include/linux/mtd/mtd.h | 7 +++ 5 files changed, 154 insertions(+), 130 deletions(-) create mode 100644 drivers/mtd/mtd_uboot.c