From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Wed, 05 Nov 2014 13:38:26 +0100 Subject: [U-Boot] [PATCH v3 2/3] mtd, nand: move common functions from cmd_nand.c to common place In-Reply-To: References: <1409895536-18092-1-git-send-email-hs@denx.de> <1409895536-18092-3-git-send-email-hs@denx.de> Message-ID: <545A1A42.1000305@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Jagan, Am 04.11.2014 21:55, schrieb Jagan Teki: > Hi Heiko Schocher, > > Nice pick - > > On 5 September 2014 11:08, 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 >> Cc: Scott Wood >> Cc: Tom Rini >> >> --- >> - changes for v2: >> none >> - changes for v3: >> - add comments from scott wood: >> - align MTD_DEV_TYPE_NAND correct >> - remove unnecessary inline >> - rework "jffs2 header" problem later >> - rebase with d6c1ffc7d23f4fe4ae8c91101861055b8e1501b6 >> --- >> 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 >> >> diff --git a/common/cmd_nand.c b/common/cmd_nand.c >> index f9ced9d..099ba00 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) > > Is this a new function call arg_off_size_onenand again, i guess it's > common call arg_off_size() > Please check? The arg_off_size() differs from the arg_off_size_onenand(), thats the reason why I let the "arg_off_size_onenand()" in ./common/cmd_onenand.c I have no board with onenand availiable for testing, so it is to risky to me, to change the code in ./common/cmd_onenand.c This should be done from someone, which can really test it! >> 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 > > I'm thinking its better to be this file in common instead of > drivers/mtd because this more reusable code > for command stuff instead of mtd core. > > And name something like mtd_common or make sense to appear command's > usage instead of > main mtd stuff, IMHO. Currently this is only possible on mtd partitions ... for example: if ((*off + *size) > mtd->size) { printf("total chip size (0x%llx) exceeded!\n", mtd->size); return -1; } if (*size == mtd->size) puts("whole chip\n"); This works only for mtd devices ... so it should be in drivers/mtd ... the name of the file ... Hmm... I have no real preference ... "mtd_uboot.c" "mtd_common.c" "mtd_uboot_common.c" What do others think? [...] bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany