Hello Jagan,

Am 04.11.2014 21:55, schrieb Jagan Teki:
Hi Heiko Schocher,

Nice pick -

On 5 September 2014 11:08, Heiko Schocher<h...@denx.de>  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<h...@denx.de>
Cc: Scott Wood<scottw...@freescale.com>
Cc: Tom Rini<tr...@ti.com>

---
- 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to