Support dynamic/patched NAND ENV offset

Hi!

I've first sent this on Feb 17, 2007. Unfortunately no reply was received. I think this is a quite useful feature, since a compile time offset into the NAND flash for the environment just doesn't work well with bad blocks ;)

This is the current version of the patch. I'd love to see it included. Thanks!

---

This patch adds support for CFG_ENV_OFFSET_PATCHED and CFG_ENV_OFFSET_OOB.

Both try to solve the problem of fixing the environment location in NAND flash at compile time, which doesn't work well if the NAND flash has a bad block at exactly that location.

CFG_ENV_OFFSET_PATCHED puts the environment in a global variable. You can then use the linker script to put that variable to a fixed location in the u-boot image. Then you can use bianry patching during the production flash process.

The idea of CFG_ENV_OFFSET_OOB is to store the environment offset in the NAND OOB data of block 0. We can do this in case the vendor makes a guarantee that block 0 never is a factory-default bad block.

Index: u-boot/common/Makefile
===================================================================
@@ -55,6 +55,7 @@
 COBJS-$(CONFIG_CMD_DISPLAY) += cmd_display.o
 COBJS-$(CONFIG_CMD_DOC) += cmd_doc.o
 COBJS-$(CONFIG_CMD_DTT) += cmd_dtt.o
+COBJS-y += cmd_dynenv.o
 COBJS-y += cmd_eeprom.o
 COBJS-$(CONFIG_CMD_ELF) += cmd_elf.o
 COBJS-$(CONFIG_CMD_EXT2) += cmd_ext2.o

Index: u-boot/common/env_nand.c
===================================================================
@@ -292,6 +292,33 @@
 	int crc1_ok = 0, crc2_ok = 0;
 	env_t *tmp_env1, *tmp_env2;
 
+#if defined(CFG_ENV_OFFSET_OOB)
+	struct mtd_info *mtd = &nand_info[0];
+	struct nand_chip *this = mtd->priv;
+	int buf_len;
+	uint8_t *buf;
+
+	buf_len = (1 << this->bbt_erase_shift);
+	buf_len += (buf_len >> this->page_shift) * mtd->oobsize;
+	buf = malloc(buf_len);
+	if (!buf)
+		return;
+
+	nand_read_raw(mtd, buf, 0, mtd->oobblock, mtd->oobsize);
+	if (buf[mtd->oobblock + 8 + 0] == 'E' &&
+	    buf[mtd->oobblock + 8 + 1] == 'N' &&
+	    buf[mtd->oobblock + 8 + 2] == 'V' &&
+	    buf[mtd->oobblock + 8 + 3] == '0') {
+		CFG_ENV_OFFSET = *((unsigned long *) &buf[mtd->oobblock + 8 + 4]);
+		/* fall through to the normal environment reading code below */
+		free(buf);
+		puts("Found Environment offset in OOB..\n");
+	} else {
+		free(buf);
+		return use_default();
+	}
+#endif
+
 	total = CFG_ENV_SIZE;
 
 	tmp_env1 = (env_t *) malloc(CFG_ENV_SIZE);

Index: u-boot/common/environment.c
===================================================================
@@ -29,6 +29,12 @@
 #undef	__ASSEMBLY__
 #include <environment.h>
 
+#if defined(CFG_ENV_OFFSET_PATCHED)
+unsigned long env_offset = CFG_ENV_OFFSET_PATCHED;
+#elif defined(CFG_ENV_OFFSET_OOB)
+unsigned long env_offset = CFG_ENV_OFFSET_OOB;
+#endif
+
 /*
  * Handle HOSTS that have prepended
  * crap on symbol names, not TARGETS.

Index: u-boot/include/environment.h
===================================================================
@@ -70,6 +70,10 @@
 #endif	/* CFG_ENV_IS_IN_FLASH */
 
 #if defined(CFG_ENV_IS_IN_NAND)
+#if defined(CFG_ENV_OFFSET_PATCHED) || defined(CFG_ENV_OFFSET_OOB)
+extern unsigned long env_offset;
+#define CFG_ENV_OFFSET env_offset
+#else
 # ifndef CFG_ENV_OFFSET
 #  error "Need to define CFG_ENV_OFFSET when using CFG_ENV_IS_IN_NAND"
 # endif

@@ -82,6 +86,7 @@
 # ifdef CFG_ENV_IS_EMBEDDED
 #  define ENV_IS_EMBEDDED	1
 # endif
+#endif /* CFG_ENV_NAND_PATCHED */
 #endif /* CFG_ENV_IS_IN_NAND */
 
 #ifdef USE_HOSTCC

Index: u-boot/common/cmd_nand.h
===================================================================
@@ -0,0 +1,33 @@
+/*
+ * cmd_nand.h - Convenience functions
+ *
+ * (C) Copyright 2006-2007 OpenMoko, Inc.
+ * Author: Werner Almesberger <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#ifndef CMD_NAND_H
+#define CMD_NAND_H
+
+#include <nand.h>
+
+
+/* common/cmd_nand.c */
+int arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off,
+  ulong *size);
+
+#endif /* CMD_NAND_H */

Index: u-boot/common/cmd_nand.c
===================================================================
@@ -90,7 +90,7 @@
 	return (*p != '\0' && *endptr == '\0') ? 1 : 0;
 }
 
-static int
+int
 arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off, size_t *size)
 {
 	int idx = nand_curr_device;

- Harald Welte <[email protected]>          	        http://openmoko.org/
============================================================================
Software for the world's first truly open Free Software mobile phone

It works if you allow room for bad blocks within each partition, and treat the environment as its own partition. Current u-boot supports skipping bad blocks within a desginated environment region.

-Scott

which wastes a lot of space, if you have something like a 128kByte erase-block-size (like most 2kByte page size NAND's today)... so if you want to have redundancy and use some spare blocks you will end up with something on the order of 512kByte of wasted flash space to store a couple of hundreds of bytes environment. Not very elegant.

Furthermore, if you want to make sure it always works with any of your components that are within the spec of the manufacturer, then you will waste even more. The problem is that a new virgin component e.g. a 64MByte flash from Samsung can have already as many as 1.3MBytes of bad blocks. There is no guarantee that they are all within one of your partitions. So to only cope with factory bad blocks, you would have to have 1.3MBytes of spare space in each of your partitions! Don't you think that's a bit too much waste of space? (you can read the full rationale at http://wiki.openmoko.org/wiki/NAND_bad_blocks)

So I'd say that having the address of the environment block stored in the out-of-band area of the first block (which is always guaranteed to be good by the manufacturer) sounds like a much more elegant solution.

Therefore, I still believe that such a feature is useful and should be merged into u-boot. If there are problems with my particular implementation, I'm happy to address them.

I also have another patchset for what I call 'dynpart' support, i.e. the dynamic calculation of a unit-specific partition table that ensures the net size of partitions are as per spec, no matter how many of the factory default blocks are located where. So it would even support NAND devices with a worse spec than the ones that we were using.

Cheers,

- Harald Welte <[email protected]>          	        http://openmoko.org/
============================================================================
Software for the world's first truly open Free Software mobile phone

Fair enough...

Can you base it off of the testing branch of the u-boot-nand-flash repo?

Interesting... Would such a patch eliminate the need for this one, by making the environment a dynamic partition? Is there any (plan for) Linux support?

-Scott

What's the relation of this to the mtdparts handling in U-Boot and Linux?

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: wd at denx.de
Mistakes are often the stepping stones to utter failure.

see my other mail in this thread (just posted).

More precisely, the flow of events in a full dynenv + dynpart setup (like the three openmoko devices so far) is:

1) u-boot creates a bad block table as part of the production process of the device
2) afterwards, the device-unique partition table is created (net partition sizes as per board-level spec), including an environment partition.
3) the standard regular partition table is stored in the environment
4) the environment including the mtdparts variable is saved to the env partition
5) the nand flash offset to the environment partition is stored in the out-of-band area of the first (known-always-good) nand flash block

During boot, the regular mechanism of passing 'mtdparts' to the kernel is used.

This was the most elegant scheme that I could come up with to support a large number of factory-bad blocks at any given location in the flash while still keeping the amount of overhead per partition low.

Cheers,

- Harald Welte <[email protected]>          	        http://openmoko.org/
============================================================================
Software for the world's first truly open Free Software mobile phone

How do you "create" the partition table? Do you use the "mtdparts" command for this?

How do you store it? In the same way as "mtdparts" is working?

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: wd at denx.de
... Jesus cried with a loud voice: Lazarus, come forth; the bug  hath
been  found  and  thy  program  runneth.  And  he  that was dead came
forth...                              -- John 11:43-44 [version 2.0?]

There is a new 'dynpart' command, which when executed uses the compile-time board-level net partiton sizes, combined with the bad-block-table to generate the device-specific 'dynamic' partition table. The result is stored in the mtdparts environment variable. Everything else is standard u-boot/kernel behaviour.

Please see the attached patch (just for reference, not inclusion yet) for the details of the implementation. This code was used successfully in the production/flashing of a couple of thousand devices, using the Samsung NAND flash chips I mentioned (up to 1.3MByte of bad blocks within 64MByte of total flash). We have seen quite a number of different device-specific partition tables, and everything has worked fine so far.

Just like with the 'dynenv' patch, I think it is something quite useful (if not neccessarry) for the economic large-scale production of NAND-flash only devices. We can always talk about the implementation details, and I'm willing to address any feedback regarding it.

yes.

- Harald Welte <[email protected]>          	        http://openmoko.org/
============================================================================
Software for the world's first truly open Free Software mobile phone
-------------- next part --------------
commit e05835df019027391f58f9d8ce5e1257d6924798
Author: Harald Welte <[email protected]>
Date:   Fri Apr 11 14:26:48 2008 +0100

    nand-dynamic_partitions.patch
    This patch adds support for 'dynamic partitions'.  This basically
    works as follows:
    * The nand code generates a bad-block-table at the first scan of the chip
    * The dynamic partition code calculates the raw partition sizes based on
      the bad block table.  E.g. if you have a partition of size 0x30000, and there are
      two bad blocks (0x4000 each) in it, the raw size will increase to 0x38000, and the
      following partitions get shifted towards the end of flash.
    
    Please note that currently the desired partition sizes are stored at compile-time
    in an array in drivers/nand/nand_bbt.c, so this definitely needs to change before
    submitting/merging upstream.
    
    In order to calculate the partiton map (and set mtdparts accordingly), you can use
    the 'dynpart' command at the prompt.  Use 'saveenv' to make the setting permanent.
    
    Signed-off-by: Harald Welte <[email protected]>

index 598574a..206a0d4 100644
@@ -494,3 +494,14 @@ void neo1973_led(int led, int on)
 }
 
 
+
+/* The sum of all part_size[]s must equal to the NAND size, i.e., 0x4000000.
+   "initrd" is sized such that it can hold two uncompressed 16 bit 640*480
+   images: 640*480*2*2 = 1228800 < 1245184. */
+
+unsigned int dynpart_size[] = {
+    CFG_UBOOT_SIZE, 0x4000, 0x200000, 0xa0000, 0x3d5c000-CFG_UBOOT_SIZE, 0 };
+char *dynpart_names[] = {
+    "u-boot", "u-boot_env", "kernel", "splash", "rootfs", NULL };
+
+

index 8c0e77f..9b3df4e 100644
@@ -150,3 +150,9 @@ int dram_init (void)
 
 	return 0;
 }
+
+unsigned int dynpart_size[] = {
+    CFG_UBOOT_SIZE, 0x4000, 0x200000, 0xa0000, 0x3d5c000-CFG_UBOOT_SIZE, 0 };
+char *dynpart_names[] = {
+    "u-boot", "u-boot_env", "kernel", "splash", "rootfs", NULL };
+

index 4dd6148..91f7623 100644
@@ -60,7 +60,7 @@ int do_dynenv(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 		buf[2] = 'V';
 		buf[3] = '0';
 
-		if (arg_off_size(argc-2, argv+2, mtd, &addr, &dummy) < 0) {
+		if (arg_off_size(argc-2, argv+2, mtd, &addr, &dummy, 1) < 0) {
 			printf("Offset or partition name expected\n");
 			goto fail;
 		}

index 7977060..bb46f34 100644
@@ -93,7 +93,7 @@ static inline int str2long(char *p, ulong *num)
 }
 
 int
-arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off, ulong *size)
+arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off, ulong *size, int net)
 {
 	int idx = nand_curr_device;
 #if defined(CONFIG_CMD_JFFS2) && defined(CONFIG_JFFS2_CMDLINE)

@@ -114,10 +114,17 @@ arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off, ulong *size)
 					printf("'%s' is not a number\n", argv[1]);
 					return -1;
 				}
-				if (*size > part->size)
-					*size = part->size;
+				if (*size > part->size) {
+					if (net)
+						*size = nand_net_part_size(part);
+					else
+						*size = part->size;
+				}
 			} else {
-				*size = part->size;
+				if (net)
+					*size = nand_net_part_size(part);
+				else
+					*size = part->size;
 			}
 			idx = dev->id->num;
 			*nand = nand_info[idx];

@@ -257,7 +264,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 
 		printf("\nNAND %s: ", scrub ? "scrub" : "erase");
 		/* skip first two or three arguments, look for offset and size */
-		if (arg_off_size(argc - o, argv + o, nand, &off, &size) != 0)
+		if (arg_off_size(argc - o, argv + o, nand, &off, &size, 0) != 0)
 			return 1;
 
 		memset(&opts, 0, sizeof(opts));

@@ -319,7 +326,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 
 		read = strncmp(cmd, "read", 4) == 0; /* 1 = read, 0 = write */
 		printf("\nNAND %s: ", read ? "read" : "write");
-		if (arg_off_size(argc - 3, argv + 3, nand, &off, &size) != 0)
+		if (arg_off_size(argc - 3, argv + 3, nand, &off, &size, 1) != 0)
 			return 1;
 
 		s = strchr(cmd, '.');

@@ -441,7 +448,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 	}
 
 	if (strcmp(cmd, "unlock") == 0) {
-		if (arg_off_size(argc - 2, argv + 2, nand, &off, &size) < 0)
+		if (arg_off_size(argc - 2, argv + 2, nand, &off, &size, 0) < 0)
 			return 1;
 
 		if (!nand_unlock(nand, off, size)) {

index 2122fd5..6638d8d 100644
@@ -92,6 +92,7 @@
 #define CONFIG_CMD_BSP
 #define CONFIG_CMD_ELF
 #define CONFIG_CMD_MISC
+#define CONFIG_CMD_JFFS2
 #define CONFIG_CMD_DIAG
 #define CONFIG_CMD_SAVES
 #define CONFIG_CMD_NAND

@@ -201,13 +202,13 @@
 #define CONFIG_FAT		1
 #define CONFIG_SUPPORT_VFAT
 
-#if 0
+#if 1
 /* JFFS2 driver */
 #define CONFIG_JFFS2_CMDLINE	1
 #define CONFIG_JFFS2_NAND	1
 #define CONFIG_JFFS2_NAND_DEV	0
-#define CONFIG_JFFS2_NAND_OFF	0x634000
-#define CONFIG_JFFS2_NAND_SIZE	0x39cc000
+//#define CONFIG_JFFS2_NAND_OFF	0x634000
+//#define CONFIG_JFFS2_NAND_SIZE	0x39cc000
 #endif
 
 /* ATAG configuration */

@@ -249,4 +250,9 @@
 #define CONFIG_DRIVER_PCF50606		1
 #define CONFIG_RTC_PCF50606		1
 
+#define MTDIDS_DEFAULT	"nand0=neo1973-nand"
+#define MTPARTS_DEFAULT	"neo1973-nand:256k(u-boot),16k(u-boot_env),2M(kernel),640k(splash),-(jffs2)"
+#define CFG_NAND_DYNPART_MTD_KERNEL_NAME "neo1973-nand"
+#define CONFIG_NAND_DYNPART
+
 #endif	/* __CONFIG_H */

index bd21ae3..06c4972 100644
@@ -275,5 +275,7 @@
 
 #define MTDIDS_DEFAULT		"nand0=qt2410-nand"
 #define MTPARTS_DEFAULT		"qt2410-nand:192k(u-boot),8k(u-boot_env),2M(kernel),2M(splash),-(jffs2)"
+#define CFG_NAND_DYNPART_MTD_KERNEL_NAME "qt2410-nand"
+#define CONFIG_NAND_DYNPART
 
 #endif	/* __CONFIG_H */

index c1ded14..7f3ac3b 100644
@@ -28,6 +28,6 @@
 
 /* common/cmd_nand.c */
 int arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off,
-  ulong *size);
+  ulong *size, int net);
 
 #endif /* UTIL_H */

Maybe the new command should be implemented as subcommand to the "mtdparts" command so we have just one interface to manipulate the "mtdparts" variable.

Thanks.

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: wd at denx.de
America has been discovered before, but it has always been hushed up.
- Oscar Wilde

Hi Wolfgang,

thanks again for your quick response. This is really encouraging me to continue merging/submitting at the same pace ;)

ok, no problem with that, I will change this before re-sending the patch (against u-boot-nand-flash/testing).

Cheers,

- Harald Welte <[email protected]>          	        http://openmoko.org/
============================================================================
Software for the world's first truly open Free Software mobile phone

This patch series is based on the idea of Harald Welte <[email protected]> and the comments of Wolfgang Denk <[email protected]> [1]. I started with Harald's original patch and migrated it to a new mtdparts sub-command and added an interface to the new functionality via a 'mtdparts add' variant.

I tried to keep it at the level of the mtd subsystem. Whereas the dynparts patch was limited to NAND flashes, I believe this patch will work on any mtd device that can report bad blocks.

These new commands can be useful when gang programming NAND chips where the gang programmer is capable only of skipping bad blocks. One can use a master image that contains images of each of the partitions padded-out to their spec'd sizes; when u-boot first comes up 'mtdparts default; mtdparts spread' (or a seq of 'mtdpart add.e' commands) will produce a partition table that matches what was put their by the gang-programmer.

It can also be useful when doing in-situ programming with u-boot as the flash programmer as demonstrated by the openmoko project's use of the 'dynpart' command [2] upon which this patch series was based.

---

NOTE: I'm not sure what to call the new subcommands, I chose 'spread' because of the way it changes the existing mtdparts variable; however, I am open to suggestions for a different name. I chose add.e/add.i because of the behaviour of the write.e/write.i commands; however, I am once again open to suggestion.

Testing was performed in the u-boot-omap-l1 tree [3]. Here is an example u-boot console session to demonstrate how the commands work:

---

U-Boot > nand bad

Device 0 bad blocks:
062c0000
0a140000
128a0000
12e20000
18bc0000
1ff80000
1ffa0000
1ffc0000
1ffe0000
U-Boot > mtdparts default
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,112m(rootfs_a),4m(kernel_b),4m(initrd_b),112m(rootfs_b)
U-Boot >
U-Boot > mtdparts spread
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b) U-Boot >
U-Boot > mtdpart del rootfs_b
U-Boot > mtdpart add.e nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b)

U-Boot > mtdpart del rootfs_b
U-Boot > mtdpart add.i nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b) U-Boot >
U-Boot > mtdparts del rootfs_b
U-Boot > mtdparts add nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),112m(rootfs_b)

---

Here are the MAKEALL results:

./MAKEALL arm on master
--------------------- SUMMARY ----------------------------
Boards compiled: 150
Boards with warnings or errors: 47 ( shannon evb4510 lpc2292sodimm modnet50 SMN42 guruplug mv88f6281gtw_ge netstar openrd_base rd6281a sheevaplug suen3 afeb9260 at91cap9adk at91rm9200dk at91rm9200ek at91sam9260ek at91sam9261ek at91sam9263ek at91sam9g10ek at91sam9g20ek at91sam9m10g45ek at91sam9rlek cmc_pu2 CPUAT91 CPU9260 CPU9G20 csb637 eb_cpux9k2 kb9202 meesc mp2usb m501sk otc570 pm9261 pm9263 SBC35_A9G20 TNY_A9260 TNY_A9G20 actux1 actux2 actux3 actux4 ixdp425 ixdpg425 pdnb3 scpu )
----------------------------------------------------------

./MAKEALL arm with this patch series applied
--------------------- SUMMARY ----------------------------
Boards compiled: 150
Boards with warnings or errors: 47 ( shannon evb4510 lpc2292sodimm modnet50 SMN42 guruplug mv88f6281gtw_ge netstar openrd_base rd6281a sheevaplug suen3 afeb9260 at91cap9adk at91rm9200dk at91rm9200ek at91sam9260ek at91sam9261ek at91sam9263ek at91sam9g10ek at91sam9g20ek at91sam9m10g45ek at91sam9rlek cmc_pu2 CPUAT91 CPU9260 CPU9G20 csb637 eb_cpux9k2 kb9202 meesc mp2usb m501sk otc570 pm9261 pm9263 SBC35_A9G20 TNY_A9260 TNY_A9G20 actux1 actux2 actux3 actux4 ixdp425 ixdpg425 pdnb3 scpu )
----------------------------------------------------------

[1] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/43549
[2] http://wiki.openmoko.org/wiki/NAND_bad_blocks
[3] git://arago-project.org/git/people/sekhar/u-boot-omapl1.git

Dear Ben Gardiner,

Has this been tested on NOR flash devices, too?

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: wd at denx.de
Life is a process, not a principle, a mystery  to  be  lived,  not  a
problem to be solved. - Gerard Straub, television producer and author
(stolen from Frank Herbert??)

Dear Ben Gardiner,

Can we have a configuration that is neutral to the memory footprint for such boards that don't want to use any of the new features? It seems the patches always add ~ 200 bytes to the code size (on PowerPC).

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: wd at denx.de
Computers make excellent and efficient servants, but I have  no  wish
to  serve under them. Captain, a starship also runs on loyalty to one
man. And nothing can replace it or him.
	-- Spock, "The Ultimate Computer", stardate 4729.4

Dear Wolfgang Denk,

No, I'm sorry it hasn't -- but I'm glad you asked. On a closer look I think the changes in this patch series could result in the dereference of a null pointer when using a NOR flash device. The features are centered around calls to mtd->block_isbad() and I don't think this function pointer is set for NOR flash devices. I will add 'if(mtd->block_isbad())' checks to the code to prevent the potential dereference of a null pointer and resubmit.

Yes, absolutely. I think that it is patch 2/4 which does not have the surrounding ifdefs of the other patches. I will make the changes in that patch conditional on a CONFIG_ define and resubmit shortly.

I was supposed to double check the image sizes before and after the patch -- as is directed in your wiki instructions, sorry for the omission. I will include a summary of the size differences in powerpc images with the resubmission of this patch series -- along with a './MAKEALL powerpc' output -- also as directed in your wiki instructions.

Best Regards,

Ben Gardiner

---
Nanometrics Inc.
+1 (613) 592-6776 x239
http://www.nanometrics.ca

This patch series is based on the idea of Harald Welte <[email protected]> and the comments of Wolfgang Denk <[email protected]> [1]. I started with Harald's original patch and migrated it to a new mtdparts sub-command and added an interface to the new functionality via a 'mtdparts add' variant.

I tried to keep it at the level of the mtd subsystem. Whereas the dynparts patch was limited to NAND flashes, I believe this patch will work on any mtd device that can report bad blocks.

These new commands can be useful when gang programming NAND chips where the gang programmer is capable only of skipping bad blocks. One can use a master image that contains images of each of the partitions padded-out to their spec'd sizes; when u-boot first comes up 'mtdparts default; mtdparts spread' (or a seq of 'mtdpart add.e' commands) will produce a partition table that matches what was put there by the gang-programmer.

It can also be useful when doing in-situ programming with u-boot being the flash programmer as demonstrated by the openmoko project's use of the 'dynpart' command [2] upon which this patch series was based.

---

Changes in v2:
* formating: spaces after 'if' and for
* printing net partition sizes feature is now conditional on the new CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES macro; patch 2/4 was adding 264 bytes to the virtlab2 build -- now it adds 0 bytes -- see below for more binary size impact details
* changed the net_part_size method to return the net size instead of using an output variable
* checking mtd->block_isbad function pointer before dereferencing
* there were some trailing whitespace errors when applying 3/4 and 4/4 that I have fixed now

NOTE: I'm not sure what to call the new subcommands, I chose 'spread' because of the way it changes the existing mtdparts variable; however, I am open to suggestions for a different name. I chose add.e/add.i because of the behaviour of the write.e/write.i commands; however, I am again open to suggestions.

Testing was performed in the u-boot-omap-l1 tree [3]. Here is an example u-boot console session to demonstrate how the commands work:

-------------------------------------------------------------------------------
U-Boot > mtdparts default
U-Boot > nand bad

Device 0 bad blocks:
062c0000
0a140000
128a0000
12e20000
18bc0000
1ff80000
1ffa0000
1ffc0000
1ffe0000
U-Boot > mtdparts

device nand0 <davinci_nand.1>, # parts = 11
#: name size net size offset
mask_flags

 0: zero                0x000c0000      0x000c0000      0x00000000      1
 1: conf                0x00200000      0x00200000      0x000c0000      0
 2: kernel_a            0x00400000      0x00400000      0x002c0000      0
 3: initrd_a            0x00400000      0x00400000      0x006c0000      0
 4: rootfs_a            0x07020000      0x07000000 (!)  0x00ac0000      0
 5: kernel_b            0x00400000      0x00400000      0x07ae0000      0
 6: initrd_b            0x00400000      0x00400000      0x07ee0000      0
 7: rootfs_b            0x07020000      0x07000000 (!)  0x082e0000      0

active partition: nand0,0 - (zero) 0x00020000 @ 0x00000000

defaults:
mtdids : nand0=davinci_nand.1
mtdparts: mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a), 4m(initrd_a),112m(rootfs_a),4m(kernel_b),4m(initrd_b),112m(rootfs_b) U-Boot >
U-Boot > mtdparts spread
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b) U-Boot > mtdparts

device nand0 <davinci_nand.1>, # parts = 11
#: name size net size offset
mask_flags

 0: zero                0x000c0000      0x000c0000      0x00000000      1
 1: conf                0x00200000      0x00200000      0x000c0000      0
 2: kernel_a            0x00400000      0x00400000      0x002c0000      0
 3: initrd_a            0x00400000      0x00400000      0x006c0000      0
 4: rootfs_a            0x07040000      0x07020000 (!)  0x00ac0000      0
 5: kernel_b            0x00400000      0x00400000      0x07b00000      0
 6: initrd_b            0x00400000      0x00400000      0x07f00000      0
 7: rootfs_b            0x07040000      0x07020000 (!)  0x08300000      0

active partition: nand0,0 - (zero) 0x00020000 @ 0x00000000

defaults:
mtdids : nand0=davinci_nand.1
mtdparts: mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a), 4m(initrd_a),112m(rootfs_a),4m(kernel_b),4m(initrd_b),112m(rootfs_b) U-Boot >
U-Boot > mtdpart del rootfs_b
U-Boot > mtdpart add.e nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b)

U-Boot > mtdpart del rootfs_b
U-Boot > mtdpart add.i nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b) U-Boot >
U-Boot > mtdparts del rootfs_b
U-Boot > mtdparts add nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),112m(rootfs_b)
-------------------------------------------------------------------------------

Here are the MAKEALL results:

./MAKEALL arm on master
--------------------- SUMMARY ----------------------------
Boards compiled: 150
Boards with warnings or errors: 47 ( shannon evb4510 lpc2292sodimm modnet50 SMN42 guruplug mv88f6281gtw_ge netstar openrd_base rd6281a sheevaplug suen3 afeb9260 at91cap9adk at91rm9200dk at91rm9200ek at91sam9260ek at91sam9261ek at91sam9263ek at91sam9g10ek at91sam9g20ek at91sam9m10g45ek at91sam9rlek cmc_pu2 CPUAT91 CPU9260 CPU9G20 csb637 eb_cpux9k2 kb9202 meesc mp2usb m501sk otc570 pm9261 pm9263 SBC35_A9G20 TNY_A9260 TNY_A9G20 actux1 actux2 actux3 actux4 ixdp425 ixdpg425 pdnb3 scpu )
----------------------------------------------------------

./MAKEALL arm with this patch series applied
--------------------- SUMMARY ----------------------------
Boards compiled: 150
Boards with warnings or errors: 47 ( shannon evb4510 lpc2292sodimm modnet50 SMN42 guruplug mv88f6281gtw_ge netstar openrd_base rd6281a sheevaplug suen3 afeb9260 at91cap9adk at91rm9200dk at91rm9200ek at91sam9260ek at91sam9261ek at91sam9263ek at91sam9g10ek at91sam9g20ek at91sam9m10g45ek at91sam9rlek cmc_pu2 CPUAT91 CPU9260 CPU9G20 csb637 eb_cpux9k2 kb9202 meesc mp2usb m501sk otc570 pm9261 pm9263 SBC35_A9G20 TNY_A9260 TNY_A9G20 actux1 actux2 actux3 actux4 ixdp425 ixdpg425 pdnb3 scpu )
----------------------------------------------------------

./MAKEALL powerpc with this patch series applied --------------------- SUMMARY ----------------------------
Boards compiled: 354
Boards with warnings or errors: 15 ( MVBC_P BMW caddy2 MPC8536DS MPC8536DS_NAND MPC8536DS_SDCARD MPC8536DS_SPIFLASH MPC8544DS P2020DS P2020DS_36BIT PM854 AP1000 EVB64260 P3G4 ZUMA )
----------------------------------------------------------

The binary size impact was checked using the da830evm config + NAND enabled for arm and the virtlab2 config for ppc. The entire patch series saves 64 bytes on arm and 4 bytes on ppc -- due to the regrouping of calls in 1/4. With all the config options introduced in this series enabled the patch series adds 1096 bytes on arm and 260 bytes on ppc.

[1] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/43549
[2] http://wiki.openmoko.org/wiki/NAND_bad_blocks
[3] git://arago-project.org/git/people/sekhar/u-boot-omapl1.git

[PATCH v3 1/4] mtdparts: regroup calls to get_mtd_device_nm [PATCH v3 2/4] mtdparts: show net size in mtdparts list [PATCH v3 3/4] mtdparts: add new sub-command "spread" [PATCH v3 4/4] mtdparts: new add.e: add part skipping bad blocks

 cmd_mtdparts.c |  256 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 237 insertions(+), 19 deletions(-)

This patch series is based on the idea of Harald Welte <[email protected]> and the comments of Wolfgang Denk <[email protected]> [1]. I started with Harald's original patch and migrated it to a new mtdparts sub-command and added an interface to the new functionality via a 'mtdparts add' variant.

I tried to keep it at the level of the mtd subsystem. Whereas the dynparts patch was limited to NAND flashes, I believe this patch will work on any mtd device that can report bad blocks.

These new commands can be useful when gang programming NAND chips where the gang programmer is capable only of skipping bad blocks. One can use a master image that contains images of each of the partitions padded-out to their spec'd sizes; when u-boot first comes up 'mtdparts default; mtdparts spread' (or a seq of 'mtdpart add.e' commands) will produce a partition table that matches what was put there by the gang-programmer.

It can also be useful when doing in-situ programming with u-boot being the flash programmer as demonstrated by the openmoko project's use of the 'dynpart' command [2] upon which this patch series was based.

---

V2:
* formating: spaces after 'if' and for
* printing net partition sizes feature is now conditional on the new CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES macro; patch 2/4 was adding 264 bytes to the virtlab2 build -- now it adds 0 bytes -- see below for more binary size impact details
* changed the net_part_size method to return the net size instead of using an output variable
* checking mtd->block_isbad function pointer before dereferencing
* there were some trailing whitespace errors when applying 3/4 and 4/4 that I have fixed now

V3:
* rebased to 54841ab50c20d6fa6c9cc3eb826989da3a22d934 of
git://git.denx.de/u-boot.git
* more checkpatch fixes
* adding openmoko to the copyright statements in cmd_mtdparts.c

NOTE: I'm not sure what to call the new subcommands, I chose 'spread' because of the way it changes the existing mtdparts variable; however, I am open to suggestions for a different name. I chose add.e/add.i because of the behaviour of the write.e/write.i commands; however, I am again open to suggestions.

Testing was performed in the u-boot-omap-l1 tree [3]. Here is an example u-boot console session to demonstrate how the commands work:

-------------------------------------------------------------------------------
U-Boot > mtdparts default
U-Boot > nand bad

Device 0 bad blocks:
062c0000
0a140000
128a0000
12e20000
18bc0000
1ff80000
1ffa0000
1ffc0000
1ffe0000
U-Boot > mtdparts

device nand0 <davinci_nand.1>, # parts = 11
#: name size net size offset
mask_flags

 0: zero                0x000c0000      0x000c0000      0x00000000      1
 1: conf                0x00200000      0x00200000      0x000c0000      0
 2: kernel_a            0x00400000      0x00400000      0x002c0000      0
 3: initrd_a            0x00400000      0x00400000      0x006c0000      0
 4: rootfs_a            0x07020000      0x07000000 (!)  0x00ac0000      0
 5: kernel_b            0x00400000      0x00400000      0x07ae0000      0
 6: initrd_b            0x00400000      0x00400000      0x07ee0000      0
 7: rootfs_b            0x07020000      0x07000000 (!)  0x082e0000      0

active partition: nand0,0 - (zero) 0x00020000 @ 0x00000000

defaults:
mtdids : nand0=davinci_nand.1
mtdparts: mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a), 4m(initrd_a),112m(rootfs_a),4m(kernel_b),4m(initrd_b),112m(rootfs_b) U-Boot >
U-Boot > mtdparts spread
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b) U-Boot > mtdparts

device nand0 <davinci_nand.1>, # parts = 11
#: name size net size offset
mask_flags

 0: zero                0x000c0000      0x000c0000      0x00000000      1
 1: conf                0x00200000      0x00200000      0x000c0000      0
 2: kernel_a            0x00400000      0x00400000      0x002c0000      0
 3: initrd_a            0x00400000      0x00400000      0x006c0000      0
 4: rootfs_a            0x07040000      0x07020000 (!)  0x00ac0000      0
 5: kernel_b            0x00400000      0x00400000      0x07b00000      0
 6: initrd_b            0x00400000      0x00400000      0x07f00000      0
 7: rootfs_b            0x07040000      0x07020000 (!)  0x08300000      0

active partition: nand0,0 - (zero) 0x00020000 @ 0x00000000

defaults:
mtdids : nand0=davinci_nand.1
mtdparts: mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a), 4m(initrd_a),112m(rootfs_a),4m(kernel_b),4m(initrd_b),112m(rootfs_b) U-Boot >
U-Boot > mtdpart del rootfs_b
U-Boot > mtdpart add.e nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b)

U-Boot > mtdpart del rootfs_b
U-Boot > mtdpart add.i nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b) U-Boot >
U-Boot > mtdparts del rootfs_b
U-Boot > mtdparts add nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),112m(rootfs_b)
-------------------------------------------------------------------------------

I verified the patches with checkpatch. The checkpatch.pl output follows:

total: 0 errors, 0 warnings, 71 lines checked

0001-PATCH-v3-1-4-mtdparts-regroup-calls-to-get_mtd_devic.patch has no obvious style problems and is ready for submission.
total: 0 errors, 0 warnings, 85 lines checked

0002-PATCH-v3-2-4-mtdparts-show-net-size-in-mtdparts-list.patch has no obvious style problems and is ready for submission.
total: 0 errors, 0 warnings, 148 lines checked

0003-PATCH-v3-3-4-mtdparts-add-new-sub-command-spread.patch has no obvious style problems and is ready for submission.
total: 0 errors, 0 warnings, 69 lines checked

0004-PATCH-v3-4-4-mtdparts-new-add.e-add-part-skipping-ba.patch has no obvious style problems and is ready for submission.

I tested the binary size and compiler warnings on ARM9 and 8xx with the following commands:

#checkout u-boot/master, apply changes to da850evm_config for testing, #commit
./MAKEALL ARM9 2>&1 > ../makeall-master.log
./MAKEALL 8xx 2>&1 > ../makeall-master.log
#apply patches, commit
./MAKEALL ARM9 2>&1 > ../makeall-spread.log
./MAKEALL 8xx 2>&1 > ../makeall-8xx-spread.log
diff -burp ../makeall-8xx-master.log ../makeall-8xx-spread.log diff -burp ../makeall-master.log ../makeall-spread.log

The diff output of the 8xx build shows all binaries text sections decreased in size -- except for kmsupx4 and mgsuvd which appear larger but are the same size when built manually:
@@ -45,7 +45,7 @@ Configuring for FLAGADM board...
121164 9096 14388 144648 23508 ./u-boot
Configuring for FPS850L board...
text data bss dec hex filename

- 233060	  12636	  25472	 271168	  42340	./u-boot
+ 233036	  12632	  25472	 271140	  42324	./u-boot

Configuring for GEN860T board...
text data bss dec hex filename
207072 32432 18752 258256 3f0d0 ./u-boot
@@ -102,7 +102,7 @@ Configuring for lwmon board...
214504 34208 218388 467100 7209c ./u-boot
Configuring for kmsupx4 board...
text data bss dec hex filename

- 274920	  19924	  28748	 323592	  4f008	./u-boot
+ 274936	  19920	  28748	 323604	  4f014	./u-boot

Configuring for MBX board...
text data bss dec hex filename
104736 7388 13016 125140 1e8d4 ./u-boot
@@ -111,7 +111,7 @@ Configuring for MBX860T board...
92380 6424 13172 111976 1b568 ./u-boot
Configuring for mgsuvd board...
text data bss dec hex filename

- 277012	  20048	  28752	 325812	  4f8b4	./u-boot
+ 277028	  20044	  28752	 325824	  4f8c0	./u-boot

Configuring for MHPC board...
text data bss dec hex filename
150944 21984 16776 189704 2e508 ./u-boot
@@ -201,19 +201,19 @@ Configuring for TOP860 board...
160456 27360 17336 205152 32160 ./u-boot
Configuring for TQM823L board...
text data bss dec hex filename

- 256664	  13888	  25700	 296252	  4853c	./u-boot
+ 256640	  13884	  25700	 296224	  48520	./u-boot
 Configuring for TQM823L_LCD board...
    text	   data	    bss	    dec	    hex	filename
- 265408	  29636	  25732	 320776	  4e508	./u-boot
+ 265384	  29632	  25732	 320748	  4e4ec	./u-boot
 Configuring for TQM850L board...
    text	   data	    bss	    dec	    hex	filename
- 256544	  13868	  25700	 296112	  484b0	./u-boot
+ 256520	  13864	  25700	 296084	  48494	./u-boot
 Configuring for TQM855L board...
    text	   data	    bss	    dec	    hex	filename
- 258544	  13964	  27236	 299744	  492e0	./u-boot
+ 258520	  13960	  27236	 299716	  492c4	./u-boot
 Configuring for TQM860L board...
    text	   data	    bss	    dec	    hex	filename
- 258592	  13972	  27236	 299800	  49318	./u-boot
+ 258568	  13968	  27236	 299772	  492fc	./u-boot

Configuring for TQM885D board...
text data bss dec hex filename
209964 14680 26052 250696 3d348 ./u-boot

The diff output of the ARM9 build shows a decrease in text size for all but the da850evm config in which the feature introduced by this patch series were enabled:
@@ -48,7 +48,7 @@ Configuring for da830evm board...
147617 4888 295320 447825 6d551 ./u-boot
Configuring for da850evm board...
text data bss dec hex filename

- 198497	  10332	 296608	 505437	  7b65d	./u-boot
+ 199590	  10332	 296608	 506530	  7baa2	./u-boot

Configuring for edb9301 board...
text data bss dec hex filename
133899 3772 213400 351071 55b5f ./u-boot
@@ -83,13 +83,13 @@ kirkwood_egiga.c:620: warning: dereferen
166164 8214 260736 435114 6a3aa ./u-boot
Configuring for imx27lite board...

    text	   data	    bss	    dec	    hex	filename
- 195862	   9876	 236052	 441790	  6bdbe	./u-boot
+ 195784	   9876	 236052	 441712	  6bd70	./u-boot
 Configuring for lpd7a400 board...
    text	   data	    bss	    dec	    hex	filename
   96349	   3368	  14496	 114213	  1be25	./u-boot
 Configuring for magnesium board...
    text	   data	    bss	    dec	    hex	filename
- 195897	   9892	 236052	 441841	  6bdf1	./u-boot
+ 195819	   9892	 236052	 441763	  6bda3	./u-boot

Configuring for mv88f6281gtw_ge board...
kirkwood_egiga.c: In function 'kwgbe_recv':
kirkwood_egiga.c:620: warning: dereferencing type-punned pointer will break strict-aliasing rules
@@ -100,10 +100,10 @@ Configuring for mx1ads board...
105339 3740 14580 123659 1e30b ./u-boot
Configuring for mx1fs2 board...

    text	   data	    bss	    dec	    hex	filename
-  97987	   3240	   4124	 105351	  19b87	./u-boot
+  97921	   3240	   4124	 105285	  19b45	./u-boot
 Configuring for netstar board...
    text	   data	    bss	    dec	    hex	filename
- 173323	   8060	  14688	 196071	  2fde7	./u-boot
+ 173269	   8060	  14688	 196017	  2fdb1	./u-boot

Configuring for nhk8815 board...
text data bss dec hex filename
175012 10436 37144 222592 36580 ./u-boot
@@ -129,7 +129,7 @@ Configuring for openrd_base board...
kirkwood_egiga.c: In function 'kwgbe_recv':
kirkwood_egiga.c:620: warning: dereferencing type-punned pointer will break strict-aliasing rules

    text	   data	    bss	    dec	    hex	filename
- 302083	   9002	 286656	 597741	  91eed	./u-boot
+ 302055	   9002	 286656	 597713	  91ed1	./u-boot

Configuring for rd6281a board...
kirkwood_egiga.c: In function 'kwgbe_recv':
kirkwood_egiga.c:620: warning: dereferencing type-punned pointer will break strict-aliasing rules
@@ -145,7 +145,7 @@ Configuring for sheevaplug board...
kirkwood_egiga.c: In function 'kwgbe_recv':
kirkwood_egiga.c:620: warning: dereferencing type-punned pointer will break strict-aliasing rules

    text	   data	    bss	    dec	    hex	filename
- 319712	   8990	 286784	 615486	  9643e	./u-boot
+ 319684	   8990	 286784	 615458	  96422	./u-boot

Configuring for smdk2400 board...
text data bss dec hex filename
97632 3440 14500 115572 1c374 ./u-boot
@@ -168,10 +168,10 @@ Configuring for suen3 board...
kirkwood_egiga.c: In function 'kwgbe_recv':
kirkwood_egiga.c:620: warning: dereferencing type-punned pointer will break strict-aliasing rules
text data bss dec hex filename

- 244086	  12716	  25128	 281930	  44d4a	./u-boot
+ 244054	  12716	  25128	 281898	  44d2a	./u-boot
 Configuring for trab board...
    text	   data	    bss	    dec	    hex	filename
- 217847	  14027	 424192	 656066	  a02c2	./u-boot
+ 217781	  14027	 424192	 656000	  a0280	./u-boot

Configuring for VCMA9 board...
text data bss dec hex filename
177726 7504 261900 447130 6d29a ./u-boot
@@ -189,7 +189,7 @@ Variant:: PB926EJ-S
85977 2920 12132 101029 18aa5 ./u-boot
Configuring for voiceblue board...

    text	   data	    bss	    dec	    hex	filename
- 143684	   4708	  18916	 167308	  28d8c	./u-boot
+ 143614	   4708	  18916	 167238	  28d46	./u-boot

Configuring for davinci_dvevm board...
text data bss dec hex filename
178646 5500 297984 482130 75b52 ./u-boot
@@ -204,13 +204,13 @@ Configuring for davinci_sonata board...
145390 5296 55068 205754 323ba ./u-boot
Configuring for davinci_dm355evm board...

    text	   data	    bss	    dec	    hex	filename
- 207288	   8516	  40864	 256668	  3ea9c	./u-boot
+ 207252	   8516	  40864	 256632	  3ea78	./u-boot
 Configuring for davinci_dm355leopard board...
    text	   data	    bss	    dec	    hex	filename
- 206398	   7904	  40864	 255166	  3e4be	./u-boot
+ 206362	   7904	  40864	 255130	  3e49a	./u-boot
 Configuring for davinci_dm365evm board...
    text	   data	    bss	    dec	    hex	filename
- 243459	   8704	 297752	 549915	  8641b	./u-boot
+ 243423	   8704	 297752	 549879	  863f7	./u-boot

Configuring for davinci_dm6467evm board...
text data bss dec hex filename
91806 4776 26100 122682 1df3a ./u-boot

The binary size impact was checked in a couple specific cases also: using the da830evm config + NAND enabled for arm and the virtlab2 config for ppc. In these cases the entire patch series saves 64 bytes on arm and 4 bytes on ppc -- due to the regrouping of calls in 1/4. With all the config options introduced in this series enabled the patch series adds 1096 bytes on arm and 260 bytes on ppc.

[1] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/43549
[2] http://wiki.openmoko.org/wiki/NAND_bad_blocks
[3] git://arago-project.org/git/people/sekhar/u-boot-omapl1.git

Hi Wolfgang,

I rebased the mtdparts bad block skipping patches and reposted a v3 series [1] in the hopes that it would be considered for inclusion into v2010.09 . I hope that I have addressed the concerns you raised in your reviews [2] [3].

I said this earlier to Scott Wood: I am very much a novice here so I hope it is not rude of me to ask: is there any chance of having the "[PATCH v3 0/4] mtdparts: add bad-block skipping" [1] series included in v2010.09?
Are there any concerns with respect to the functionality that this patch provides?
Is there anything else that I can provide to help with getting this patch included?

Best Regards,
Ben Gardiner

[1] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/80843
[2] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/79418
[3] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/79419

---
Nanometrics Inc.
+1 (613) 592-6776 x239
http://www.nanometrics.ca

[PATCH v4 1/4] mtdparts: regroup calls to get_mtd_device_nm [PATCH v4 2/4] mtdparts: show net size in mtdparts list [PATCH v4 3/4] mtdparts: add new sub-command "spread" [PATCH v4 4/4] mtdparts: new add.spread: add part skipping bad blocks

 common/cmd_mtdparts.c |  268 +++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 246 insertions(+), 22 deletions(-)

This patch series is based on the idea of Harald Welte <[email protected]> and the comments of Wolfgang Denk <[email protected]> [1]. I started with Harald's original patch and migrated it to a new mtdparts sub-command and added an interface to the new functionality via a 'mtdparts add' variant.

I tried to keep it at the level of the mtd subsystem. Whereas the dynparts patch was limited to NAND flashes, I believe this patch will work on any mtd device that can report bad blocks.

These new commands can be useful when gang programming NAND chips where the gang programmer is capable only of skipping bad blocks. One can use a master image that contains images of each of the partitions padded-out to their spec'd sizes; when u-boot first comes up 'mtdparts default; mtdparts spread' (or a seq of 'mtdpart add.spread' commands) will produce a partition table that matches what was put there by the gang-programmer.

It can also be useful when doing in-situ programming with u-boot being the flash programmer as demonstrated by the openmoko project's use of the 'dynpart' command [2] upon which this patch series was based.

[1] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/43549
[2] http://wiki.openmoko.org/wiki/NAND_bad_blocks

---

V2:
* formating: spaces after 'if' and for
* printing net partition sizes feature is now conditional on the new CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES macro; patch 2/4 was adding 264 bytes to the virtlab2 build -- now it adds 0 bytes -- see below for more binary size impact details
* changed the net_part_size method to return the net size instead of using an output variable
* checking mtd->block_isbad function pointer before dereferencing
* there were some trailing whitespace errors when applying 3/4 and 4/4 that I have fixed now

V3:
* rebased to 54841ab50c20d6fa6c9cc3eb826989da3a22d934 of
git://git.denx.de/u-boot.git
* more checkpatch fixes
* adding openmoko to the copyright statements in cmd_mtdparts.c

V4:
* rebased to b417260d871d4d8d336c160d95ed40cc8c0fb0fa of
git://git.denx.de/u-boot.git
* re-grouped list_partition #ifdefs into one
* removed changelog and my (C) from file header
* added source of port to the commit message
* fixed multi-line comment style
* indentation only by tabs
* check for s == NULL when looking for '.' in command
* do not include support for the '.i' synonym
* rename to 'add.spread' to match 'mtdparts spread' as per Scott Wood's suggestion.

Testing was performed with da850evm.h plus NAND enabled. Here is an example u-boot console session to demonstrate how the commands work:

-------------------------------------------------------------------------------
U-Boot > mtdparts default
U-Boot > nand bad

Device 0 bad blocks:
062c0000
0a140000
128a0000
12e20000
18bc0000
1ff80000
1ffa0000
1ffc0000
1ffe0000
U-Boot > mtdparts

device nand0 <davinci_nand.1>, # parts = 11
#: name size net size offset
mask_flags

 0: zero                0x000c0000      0x000c0000      0x00000000      1
 1: conf                0x00200000      0x00200000      0x000c0000      0
 2: kernel_a            0x00400000      0x00400000      0x002c0000      0
 3: initrd_a            0x00400000      0x00400000      0x006c0000      0
 4: rootfs_a            0x07020000      0x07000000 (!)  0x00ac0000      0
 5: kernel_b            0x00400000      0x00400000      0x07ae0000      0
 6: initrd_b            0x00400000      0x00400000      0x07ee0000      0
 7: rootfs_b            0x07020000      0x07000000 (!)  0x082e0000      0

active partition: nand0,0 - (zero) 0x00020000 @ 0x00000000

defaults:
mtdids : nand0=davinci_nand.1
mtdparts: mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a), 4m(initrd_a),112m(rootfs_a),4m(kernel_b),4m(initrd_b),112m(rootfs_b) U-Boot >
U-Boot > mtdparts spread
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b) U-Boot > mtdparts

device nand0 <davinci_nand.1>, # parts = 11
#: name size net size offset
mask_flags

 0: zero                0x000c0000      0x000c0000      0x00000000      1
 1: conf                0x00200000      0x00200000      0x000c0000      0
 2: kernel_a            0x00400000      0x00400000      0x002c0000      0
 3: initrd_a            0x00400000      0x00400000      0x006c0000      0
 4: rootfs_a            0x07040000      0x07020000 (!)  0x00ac0000      0
 5: kernel_b            0x00400000      0x00400000      0x07b00000      0
 6: initrd_b            0x00400000      0x00400000      0x07f00000      0
 7: rootfs_b            0x07040000      0x07020000 (!)  0x08300000      0

active partition: nand0,0 - (zero) 0x00020000 @ 0x00000000

defaults:
mtdids : nand0=davinci_nand.1
mtdparts: mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a), 4m(initrd_a),112m(rootfs_a),4m(kernel_b),4m(initrd_b),112m(rootfs_b) U-Boot >
U-Boot > mtdpart del rootfs_b
U-Boot > mtdpart add.spread nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b)

U-Boot > mtdpart del rootfs_b
U-Boot > mtdpart add.spread nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b) U-Boot >
U-Boot > mtdparts del rootfs_b
U-Boot > mtdparts add nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),112m(rootfs_b)
-------------------------------------------------------------------------------

I verified the patches with checkpatch.

I tested the binary size and compiler warnings on ARM9 and 8xx. The patch series saves 24 bytes of text on 8xx and 54 bytes of text on arm9 -- when none of the config options are enabled. When the config options are enabled, text grows by 1023 bytes on arm9.

Ben Gardiner (5):
mtdparts: regroup calls to get_mtd_device_nm
mtd: add an mtd method for get_len_incl_bad()
mtdparts: show net size in mtdparts list
mtdparts: add new sub-command "spread"
mtdparts: new add.spread: add part skipping bad blocks

 common/cmd_mtdparts.c        |  263 ++++++++++++++++++++++++++++++++++++-----
 drivers/mtd/mtdcore.c        |   45 +++++++
 drivers/mtd/nand/nand_util.c |    6 +
 include/linux/mtd/mtd.h      |    4 +-
 4 files changed, 285 insertions(+), 33 deletions(-)

This patch series is based on the idea of Harald Welte <[email protected]> and the comments of Wolfgang Denk <[email protected]> [1]. I started with Harald's original patch and migrated it to a new mtdparts sub-command and added an interface to the new functionality via a 'mtdparts add' variant.

I tried to keep it at the level of the mtd subsystem. Whereas the dynparts patch was limited to NAND flashes, I believe this patch will work on any mtd device that can report bad blocks.

These new commands can be useful when gang programming NAND chips where the gang programmer is capable only of skipping bad blocks. One can use a master image that contains images of each of the partitions padded-out to their spec'd sizes; when u-boot first comes up 'mtdparts default; mtdparts spread' (or a seq of 'mtdpart add.spread' commands) will produce a partition table that matches what was put there by the gang-programmer.

It can also be useful when doing in-situ programming with u-boot being the flash programmer as demonstrated by the openmoko project's use of the 'dynpart' command [2] upon which this patch series was based.

[1] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/43549
[2] http://wiki.openmoko.org/wiki/NAND_bad_blocks

---

V2:
* formating: spaces after 'if' and for
* printing net partition sizes feature is now conditional on the new CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES macro; patch 2/4 was adding 264 bytes to the virtlab2 build -- now it adds 0 bytes -- see below for more binary size impact details
* changed the net_part_size method to return the net size instead of using an output variable
* checking mtd->block_isbad function pointer before dereferencing
* there were some trailing whitespace errors when applying 3/4 and 4/4 that I have fixed now

V3:
* rebased to 54841ab50c20d6fa6c9cc3eb826989da3a22d934 of
git://git.denx.de/u-boot.git
* more checkpatch fixes
* adding openmoko to the copyright statements in cmd_mtdparts.c

V4:
* rebased to b417260d871d4d8d336c160d95ed40cc8c0fb0fa of
git://git.denx.de/u-boot.git
* re-grouped list_partition #ifdefs into one
* removed changelog and my (C) from file header
* added source of port to the commit message
* fixed multi-line comment style
* indentation only by tabs
* check for s == NULL when looking for '.' in command
* do not include support for the '.i' synonym
* rename to 'add.spread' to match 'mtdparts spread' as per Scott Wood's suggestion.

V5:
* rebased to 962ad59e25640e586e2bceabf67a628a27f8f508 of
git://git.denx.de/u-boot.git
* renumbered patches; V5 introduces patch 2/5
* return uint64_t instead of u32 for net_size
* do a quick if((cond) return
* calculate net_size by adding-up good blocks instead of subtracting bad blocks
* try to strike a balance; reuse more code between the branches of #if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) in print_partition_table
* don't push dangling lines to the right
* fix spelling errors in comments
* more hanging arguments alignment
* refactor the spread_partition function so that it delegates the bad-block counting to the mtd_get_len_incl_bad function -- as per Scott Woods' suggestion.
* don't reproduce the same call to spread_partitions on either side of the check for existing device -- as per Scott Wood's comments.

Testing was performed with da850evm.h plus NAND enabled. Here is an example u-boot console session to demonstrate how the commands work:

-------------------------------------------------------------------------------
U-Boot > mtdparts default
U-Boot > nand bad

Device 0 bad blocks:
062c0000
0a140000
128a0000
12e20000
18bc0000
1ff80000
1ffa0000
1ffc0000
1ffe0000
U-Boot > mtdparts

device nand0 <davinci_nand.1>, # parts = 11
#: name size net size offset
mask_flags

 0: zero                0x000c0000      0x000c0000      0x00000000      1
 1: conf                0x00200000      0x00200000      0x000c0000      0
 2: kernel_a            0x00400000      0x00400000      0x002c0000      0
 3: initrd_a            0x00400000      0x00400000      0x006c0000      0
 4: rootfs_a            0x07020000      0x07000000 (!)  0x00ac0000      0
 5: kernel_b            0x00400000      0x00400000      0x07ae0000      0
 6: initrd_b            0x00400000      0x00400000      0x07ee0000      0
 7: rootfs_b            0x07020000      0x07000000 (!)  0x082e0000      0

active partition: nand0,0 - (zero) 0x00020000 @ 0x00000000

defaults:
mtdids : nand0=davinci_nand.1
mtdparts: mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a), 4m(initrd_a),112m(rootfs_a),4m(kernel_b),4m(initrd_b),112m(rootfs_b) U-Boot >
U-Boot > mtdparts spread
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b) U-Boot > mtdparts

device nand0 <davinci_nand.1>, # parts = 11
#: name size net size offset
mask_flags

 0: zero                0x000c0000      0x000c0000      0x00000000      1
 1: conf                0x00200000      0x00200000      0x000c0000      0
 2: kernel_a            0x00400000      0x00400000      0x002c0000      0
 3: initrd_a            0x00400000      0x00400000      0x006c0000      0
 4: rootfs_a            0x07040000      0x07020000 (!)  0x00ac0000      0
 5: kernel_b            0x00400000      0x00400000      0x07b00000      0
 6: initrd_b            0x00400000      0x00400000      0x07f00000      0
 7: rootfs_b            0x07040000      0x07020000 (!)  0x08300000      0

active partition: nand0,0 - (zero) 0x00020000 @ 0x00000000

defaults:
mtdids : nand0=davinci_nand.1
mtdparts: mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a), 4m(initrd_a),112m(rootfs_a),4m(kernel_b),4m(initrd_b),112m(rootfs_b) U-Boot >
U-Boot > mtdpart del rootfs_b
U-Boot > mtdpart add.spread nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b)

U-Boot > mtdpart del rootfs_b
U-Boot > mtdpart add.spread nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b) U-Boot >
U-Boot > mtdparts del rootfs_b
U-Boot > mtdparts add nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),112m(rootfs_b)
-------------------------------------------------------------------------------

I verified the patches with checkpatch.

I confirmed that MAKEALL for 'ARM9' and '8xx' did not introduce any additional warnings.

I tested the binary size and compiler warnings on ARM9 and 8xx. The patch series saves 24 bytes of text on 8xx and 74 bytes of text on arm9 -- when none of the config options are enabled. When the config options are enabled, text grows by 1321 bytes on arm9.

The logic to 'spread' mtd partitions needs to calculate the length in the mtd device, including bad blocks.

This patch introduces a new function, mtd_get_len_incl_bad that can return both the length including bad blocks and whether that length was truncated on the device. This new function will be used by the mtdparts spread command later in this series. The definition of the function is #ifdef'd out in configurations that do not use the new 'mtdparts spread' command.

---

Note: the mtd_get_len_incl_bad() function could also be used by the get_len_incl_bad() function in nand_util.c except for the fact that boards can enable NAND support without enabling MTD support. A note has been added to get_len_incl_bad() to remind us to refactor when/if MTD support is available whenever NAND support is enabled.

V5:
* introduced in v5 of this patchset

---
 drivers/mtd/mtdcore.c        |   44 ++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/nand_util.c |    6 +++++
 include/linux/mtd/mtd.h      |    4 ++-
 3 files changed, 53 insertions(+), 1 deletions(-)

index 6eb52ed..cb86657 100644
@@ -142,3 +142,47 @@ void put_mtd_device(struct mtd_info *mtd)
 	c = --mtd->usecount;
 	BUG_ON(c < 0);
 }
+
+#if defined(CONFIG_CMD_MTDPARTS_SPREAD)
+/**
+ * mtd_get_len_incl_bad
+ *
+ * Check if length including bad blocks fits into device.
+ *
+ * @param mtd an MTD device
+ * @param offset offset in flash
+ * @param length image length
+ * @return image length including bad blocks in *len_incl_bad and whether or not
+ *         the length returned was truncated in *truncated
+ */
+void mtd_get_len_incl_bad(struct mtd_info *mtd, uint64_t offset,
+			  const uint64_t length, uint64_t *len_incl_bad,
+			  int *truncated)
+{
+	*truncated = 0;
+	*len_incl_bad = 0;
+
+	if (!mtd->block_isbad) {
+		*len_incl_bad = length;
+		return;
+	}
+
+	uint64_t len_excl_bad = 0;
+	uint64_t block_len;
+
+	while (len_excl_bad < length) {
+		block_len = mtd->erasesize - (offset & (mtd->erasesize - 1));
+
+		if (!mtd->block_isbad(mtd, offset & ~(mtd->erasesize - 1)))
+			len_excl_bad += block_len;
+
+		*len_incl_bad += block_len;
+		offset       += block_len;
+
+		if (offset >= mtd->size) {
+			*truncated = 1;
+			break;
+		}
+	}
+}
+#endif /* defined(CONFIG_CMD_MTDPARTS_SPREAD) */

index 29c42f7..622237f 100644
@@ -435,6 +435,12 @@ int nand_unlock(struct mtd_info *mtd, ulong start, ulong length)
 static size_t get_len_incl_bad (nand_info_t *nand, loff_t offset,
 				const size_t length)
 {
+	/*
+	 * TODO: replace this implementation with a call to
+	 * mtd_get_len_incl_bad((struct mtd_info *) nand, ...)
+	 * when CONFIG_MTD_DEVICE is required for NAND support
+	 */
+
 	size_t len_incl_bad = 0;
 	size_t len_excl_bad = 0;
 	size_t block_len;

index 16556c4..8e8ec7c 100644
@@ -259,7 +259,9 @@ extern struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num);
 extern struct mtd_info *get_mtd_device_nm(const char *name);
 
 extern void put_mtd_device(struct mtd_info *mtd);
-
+extern void mtd_get_len_incl_bad(struct mtd_info *mtd, uint64_t offset,
+				 const uint64_t length, uint64_t *len_incl_bad,
+				 int *truncated);
 /* XXX U-BOOT XXX */
 #if 0
 struct mtd_notifier {

1.7.0.4

Good point. Will do.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

The logic to 'spread' mtd partitions needs to calculate the length in the mtd device, including bad blocks.

This patch introduces a new function, mtd_get_len_incl_bad that can return both the length including bad blocks and whether that length was truncated on the device. This new function will be used by the mtdparts spread command later in this series. The definition of the function is #ifdef'd out in configurations that do not use the new 'mtdparts spread' command.

---

Note: the mtd_get_len_incl_bad() function could also be used by the get_len_incl_bad() function in nand_util.c except for the fact that boards can enable NAND support without enabling MTD support. A note has been added to get_len_incl_bad() to remind us to refactor when/if MTD support is available whenever NAND support is enabled.

V5:
* introduced in v5 of this patchset

V6:
* return zero length and truncated if mtd_get_len_incl_bad is called with offset>=size -- as per Scott Wood's review comments

---
 drivers/mtd/mtdcore.c        |   49 ++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/nand_util.c |    6 +++++
 include/linux/mtd/mtd.h      |    4 ++-
 3 files changed, 58 insertions(+), 1 deletions(-)

index 6eb52ed..78f2a08 100644
@@ -142,3 +142,52 @@ void put_mtd_device(struct mtd_info *mtd)
 	c = --mtd->usecount;
 	BUG_ON(c < 0);
 }
+
+#if defined(CONFIG_CMD_MTDPARTS_SPREAD)
+/**
+ * mtd_get_len_incl_bad
+ *
+ * Check if length including bad blocks fits into device.
+ *
+ * @param mtd an MTD device
+ * @param offset offset in flash
+ * @param length image length
+ * @return image length including bad blocks in *len_incl_bad and whether or not
+ *         the length returned was truncated in *truncated
+ */
+void mtd_get_len_incl_bad(struct mtd_info *mtd, uint64_t offset,
+			  const uint64_t length, uint64_t *len_incl_bad,
+			  int *truncated)
+{
+	*truncated = 0;
+	*len_incl_bad = 0;
+
+	if (offset >= mtd->size) {
+		*truncated = 1;
+		return;
+	}
+
+	if (!mtd->block_isbad) {
+		*len_incl_bad = length;
+		return;
+	}
+
+	uint64_t len_excl_bad = 0;
+	uint64_t block_len;
+
+	while (len_excl_bad < length) {
+		block_len = mtd->erasesize - (offset & (mtd->erasesize - 1));
+
+		if (!mtd->block_isbad(mtd, offset & ~(mtd->erasesize - 1)))
+			len_excl_bad += block_len;
+
+		*len_incl_bad += block_len;
+		offset       += block_len;
+
+		if (offset >= mtd->size) {
+			*truncated = 1;
+			break;
+		}
+	}
+}
+#endif /* defined(CONFIG_CMD_MTDPARTS_SPREAD) */

index 29c42f7..622237f 100644
@@ -435,6 +435,12 @@ int nand_unlock(struct mtd_info *mtd, ulong start, ulong length)
 static size_t get_len_incl_bad (nand_info_t *nand, loff_t offset,
 				const size_t length)
 {
+	/*
+	 * TODO: replace this implementation with a call to
+	 * mtd_get_len_incl_bad((struct mtd_info *) nand, ...)
+	 * when CONFIG_MTD_DEVICE is required for NAND support
+	 */
+
 	size_t len_incl_bad = 0;
 	size_t len_excl_bad = 0;
 	size_t block_len;

index 16556c4..8e8ec7c 100644
@@ -259,7 +259,9 @@ extern struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num);
 extern struct mtd_info *get_mtd_device_nm(const char *name);
 
 extern void put_mtd_device(struct mtd_info *mtd);
-
+extern void mtd_get_len_incl_bad(struct mtd_info *mtd, uint64_t offset,
+				 const uint64_t length, uint64_t *len_incl_bad,
+				 int *truncated);
 /* XXX U-BOOT XXX */
 #if 0
 struct mtd_notifier {

1.7.0.4

This patch adds an additional column to the output of list_partitions. The additional column will contain the net size and a '(!)' beside it if the net size is not equal to the partition size.

---

V2:
* formatting: spaces after 'if' and 'for'
* the entire new feature is conditional on a macro, there is now a zero-byte binary size impact when the macro is not defined.
* return the net parition size directly from net_part_size instead of using an output variable

V3:
* rebased to 54841ab50c20d6fa6c9cc3eb826989da3a22d934 of
git://git.denx.de/u-boot.git
* fix line length over 80 chars
* update copyright of cmd_mtdparts.c

V4:
* rebased to b417260d871d4d8d336c160d95ed40cc8c0fb0fa of
git://git.denx.de/u-boot.git
* removed copyright statement and changelog from file header
* re-grouped list_partition #ifdefs into one
* fixed multi-line comment style

V5:
* rebased to 962ad59e25640e586e2bceabf67a628a27f8f508 of
git://git.denx.de/u-boot.git
* renumbered from 2/4 to 3/5
* return uint64_t instead of u32 for net_size
* do a quick if((cond) return
* calculate net_size by adding-up good blocks instead of subtracting bad blocks
* try to strike a balance; reuse more code between the branches of #if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) in print_partition_table

---
 common/cmd_mtdparts.c |   74 +++++++++++++++++++++++++++++++++++++++++++-----
 drivers/mtd/mtdcore.c |    5 ++-
 2 files changed, 69 insertions(+), 10 deletions(-)

index 772ad54..a8912ed 100644
@@ -1215,38 +1215,96 @@ static int generate_mtdparts_save(char *buf, u32 buflen)
 	return ret;
 }
 
+#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
 /**
- * Format and print out a partition list for each device from global device
- * list.
+ * Get the net size (w/o bad blocks) of the given partition.
+ *
+ * @param mtd the mtd info
+ * @param part the partition
+ * @return the calculated net size of this partition
  */
-static void list_partitions(void)
+static uint64_t net_part_size(struct mtd_info *mtd, struct part_info *part)
+{
+	uint64_t gross_size, trailing_bad_size = 0;
+	int truncated = 0;
+
+	mtd_get_len_incl_bad(mtd, part->offset, part->size, &gross_size,
+			     &truncated);
+
+	if (!truncated) {
+			mtd_get_len_incl_bad(mtd, part->offset + part->size,
+					     mtd->erasesize, &trailing_bad_size,
+					     &truncated);
+			trailing_bad_size -= mtd->erasesize;
+	}
+
+	return part->size - (gross_size - trailing_bad_size - part->size);
+}
+#endif
+
+static void print_partition_table(void)
 {
 	struct list_head *dentry, *pentry;
 	struct part_info *part;
 	struct mtd_device *dev;
 	int part_num;
 
-	debug("\n---list_partitions---\n");
-	list_for_each(dentry, &devices) {
+list_for_each(dentry, &devices) {
 		dev = list_entry(dentry, struct mtd_device, link);
+		/* list partitions for given device */
+		part_num = 0;
+#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
+		struct mtd_info *mtd;
+
+		if (get_mtd_info(dev->id->type, dev->id->num, &mtd))
+			return;
+
+		printf("\ndevice %s%d <%s>, # parts = %d\n",
+				MTD_DEV_TYPE(dev->id->type), dev->id->num,
+				dev->id->mtd_id, dev->num_parts);
+		printf(" #: name\t\tsize\t\tnet size\toffset\t\tmask_flags\n");
+
+		list_for_each(pentry, &dev->parts) {
+			u32 net_size;
+			char *size_note;
+
+			part = list_entry(pentry, struct part_info, link);
+			net_size = net_part_size(mtd, part);
+			size_note = part->size == net_size ? " " : " (!)";
+			printf("%2d: %-20s0x%08x\t0x%08x%s\t0x%08x\t%d\n",
+					part_num, part->name, part->size,
+					net_size, size_note, part->offset,
+					part->mask_flags);
+#else /* !defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) */
 		printf("\ndevice %s%d <%s>, # parts = %d\n",
 				MTD_DEV_TYPE(dev->id->type), dev->id->num,
 				dev->id->mtd_id, dev->num_parts);
 		printf(" #: name\t\tsize\t\toffset\t\tmask_flags\n");
 
-		/* list partitions for given device */
-		part_num = 0;
 		list_for_each(pentry, &dev->parts) {
 			part = list_entry(pentry, struct part_info, link);
 			printf("%2d: %-20s0x%08x\t0x%08x\t%d\n",
 					part_num, part->name, part->size,
 					part->offset, part->mask_flags);
-
+#endif /* defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) */
 			part_num++;
 		}
 	}
+
 	if (list_empty(&devices))
 		printf("no partitions defined\n");
+}
+
+/**
+ * Format and print out a partition list for each device from global device
+ * list.
+ */
+static void list_partitions(void)
+{
+	struct part_info *part;
+
+	debug("\n---list_partitions---\n");
+	print_partition_table();
 
 	/* current_mtd_dev is not NULL only when we have non empty device list */
 	if (current_mtd_dev) {

index cb86657..211b993 100644
@@ -143,7 +143,8 @@ void put_mtd_device(struct mtd_info *mtd)
 	BUG_ON(c < 0);
 }
 
-#if defined(CONFIG_CMD_MTDPARTS_SPREAD)
+#if defined(CONFIG_CMD_MTDPARTS_SPREAD) || \
+    defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
 /**
  * mtd_get_len_incl_bad
  *

@@ -185,4 +186,4 @@ void mtd_get_len_incl_bad(struct mtd_info *mtd, uint64_t offset,
 		}
 	}
 }
-#endif /* defined(CONFIG_CMD_MTDPARTS_SPREAD) */
+#endif

1.7.0.4

Right -- I was trying to maximize reuse of the new function but failed. But you're right that it just isn't suitable for reuse here. I'll go back to counting good blocks.

Sorry.

Yes it doesn't scale well with the number of fields printed -- see below.

I'm really glad for all your input on these patches -- your patience with my endless revisions appears equally as endless.

But I'm not sure what I'm supposed to do with this block now since Wolfgang Denk <[email protected]> has asked for me to 1) not use a bunch of #ifdefs to define the field [1] 2) not introduce changes that increase the code size on boards that do not enable this feature [2] ... and you have asked me to do the opposite in both cases here.

I will definitely re-spin a v6 with all of the mistakes you have pointed out fixed. But I think I have to stick with zero-impact-on-size patch and the minimal #ifdefs there since Wolfgang Denk <[email protected]> has asked for that specifically.

Best Regards,
Ben Gardiner

[1] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/82208
[2] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/79419

---
Nanometrics Inc.
http://www.nanometrics.ca

Sometimes a request seems perfectly reasonable until you see what the consequences are. :-)

Wolfgang, how do you want this to be handled?

-Scott

This patch adds an additional column to the output of list_partitions. The additional column will contain the net size and a '(!)' beside it if the net size is not equal to the partition size.

---

V2:
* formatting: spaces after 'if' and 'for'
* the entire new feature is conditional on a macro, there is now a zero-byte binary size impact when the macro is not defined.
* return the net parition size directly from net_part_size instead of using an output variable

V3:
* rebased to 54841ab50c20d6fa6c9cc3eb826989da3a22d934 of
git://git.denx.de/u-boot.git
* fix line length over 80 chars
* update copyright of cmd_mtdparts.c

V4:
* rebased to b417260d871d4d8d336c160d95ed40cc8c0fb0fa of
git://git.denx.de/u-boot.git
* removed copyright statement and changelog from file header
* re-grouped list_partition #ifdefs into one
* fixed multi-line comment style

V5:
* rebased to 962ad59e25640e586e2bceabf67a628a27f8f508 of
git://git.denx.de/u-boot.git
* renumbered from 2/4 to 3/5
* return uint64_t instead of u32 for net_size
* do a quick if((cond) return
* calculate net_size by adding-up good blocks instead of subtracting bad blocks
* try to strike a balance; reuse more code between the branches of #if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) in print_partition_table

V6:
* fix indentation on list_entry
* don't use mtd_get_len_incl_bad to get net_size anymore as it was a bad idea. Just count up the good blocks.

---
 common/cmd_mtdparts.c |   68 +++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 61 insertions(+), 7 deletions(-)

index 772ad54..266844f 100644
@@ -1215,38 +1215,92 @@ static int generate_mtdparts_save(char *buf, u32 buflen)
 	return ret;
 }
 
+#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
 /**
- * Format and print out a partition list for each device from global device
- * list.
+ * Get the net size (w/o bad blocks) of the given partition.
+ *
+ * @param mtd the mtd info
+ * @param part the partition
+ * @return the calculated net size of this partition
  */
-static void list_partitions(void)
+static uint64_t net_part_size(struct mtd_info *mtd, struct part_info *part)
+{
+	if (!mtd->block_isbad)
+		return part->size;
+
+	uint64_t i, net_size = 0;
+
+	for (i = 0; i < part->size; i += mtd->erasesize) {
+		if (!mtd->block_isbad(mtd, part->offset + i))
+			net_size += mtd->erasesize;
+	}
+	return net_size;
+}
+#endif
+
+static void print_partition_table(void)
 {
 	struct list_head *dentry, *pentry;
 	struct part_info *part;
 	struct mtd_device *dev;
 	int part_num;
 
-	debug("\n---list_partitions---\n");
 	list_for_each(dentry, &devices) {
 		dev = list_entry(dentry, struct mtd_device, link);
+		/* list partitions for given device */
+		part_num = 0;
+#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
+		struct mtd_info *mtd;
+
+		if (get_mtd_info(dev->id->type, dev->id->num, &mtd))
+			return;
+
+		printf("\ndevice %s%d <%s>, # parts = %d\n",
+				MTD_DEV_TYPE(dev->id->type), dev->id->num,
+				dev->id->mtd_id, dev->num_parts);
+		printf(" #: name\t\tsize\t\tnet size\toffset\t\tmask_flags\n");
+
+		list_for_each(pentry, &dev->parts) {
+			u32 net_size;
+			char *size_note;
+
+			part = list_entry(pentry, struct part_info, link);
+			net_size = net_part_size(mtd, part);
+			size_note = part->size == net_size ? " " : " (!)";
+			printf("%2d: %-20s0x%08x\t0x%08x%s\t0x%08x\t%d\n",
+					part_num, part->name, part->size,
+					net_size, size_note, part->offset,
+					part->mask_flags);
+#else /* !defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) */
 		printf("\ndevice %s%d <%s>, # parts = %d\n",
 				MTD_DEV_TYPE(dev->id->type), dev->id->num,
 				dev->id->mtd_id, dev->num_parts);
 		printf(" #: name\t\tsize\t\toffset\t\tmask_flags\n");
 
-		/* list partitions for given device */
-		part_num = 0;
 		list_for_each(pentry, &dev->parts) {
 			part = list_entry(pentry, struct part_info, link);
 			printf("%2d: %-20s0x%08x\t0x%08x\t%d\n",
 					part_num, part->name, part->size,
 					part->offset, part->mask_flags);
-
+#endif /* defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) */
 			part_num++;
 		}
 	}
+
 	if (list_empty(&devices))
 		printf("no partitions defined\n");
+}
+
+/**
+ * Format and print out a partition list for each device from global device
+ * list.
+ */
+static void list_partitions(void)
+{
+	struct part_info *part;
+
+	debug("\n---list_partitions---\n");
+	print_partition_table();
 
 	/* current_mtd_dev is not NULL only when we have non empty device list */
 	if (current_mtd_dev) {

1.7.0.4

Ben Gardiner (5):
mtdparts: regroup calls to get_mtd_device_nm
mtd: add an mtd method for get_len_incl_bad()
mtdparts: show net size in mtdparts list
mtdparts: add new sub-command "spread"
mtdparts: new add.spread: add part skipping bad blocks

 common/cmd_mtdparts.c        |  260 +++++++++++++++++++++++++++++++++++++-----
 drivers/mtd/mtdcore.c        |   49 ++++++++
 drivers/mtd/nand/nand_util.c |    6 +
 include/linux/mtd/mtd.h      |    4 +-
 4 files changed, 287 insertions(+), 32 deletions(-)

This patch series is based on the idea of Harald Welte <[email protected]> and the comments of Wolfgang Denk <[email protected]> [1]. I started with Harald's original patch and migrated it to a new mtdparts sub-command and added an interface to the new functionality via a 'mtdparts add' variant.

I tried to keep it at the level of the mtd subsystem. Whereas the dynparts patch was limited to NAND flashes, I believe this patch will work on any mtd device that can report bad blocks.

These new commands can be useful when gang programming NAND chips where the gang programmer is capable only of skipping bad blocks. One can use a master image that contains images of each of the partitions padded-out to their spec'd sizes; when u-boot first comes up 'mtdparts default; mtdparts spread' (or a seq of 'mtdpart add.spread' commands) will produce a partition table that matches what was put there by the gang-programmer.

It can also be useful when doing in-situ programming with u-boot being the flash programmer as demonstrated by the openmoko project's use of the 'dynpart' command [2] upon which this patch series was based.

[1] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/43549
[2] http://wiki.openmoko.org/wiki/NAND_bad_blocks

---

V2:
* formating: spaces after 'if' and for
* printing net partition sizes feature is now conditional on the new CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES macro; patch 2/4 was adding 264 bytes to the virtlab2 build -- now it adds 0 bytes -- see below for more binary size impact details
* changed the net_part_size method to return the net size instead of using an output variable
* checking mtd->block_isbad function pointer before dereferencing
* there were some trailing whitespace errors when applying 3/4 and 4/4 that I have fixed now

V3:
* rebased to 54841ab50c20d6fa6c9cc3eb826989da3a22d934 of
git://git.denx.de/u-boot.git
* more checkpatch fixes
* adding openmoko to the copyright statements in cmd_mtdparts.c

V4:
* rebased to b417260d871d4d8d336c160d95ed40cc8c0fb0fa of
git://git.denx.de/u-boot.git
* re-grouped list_partition #ifdefs into one
* removed changelog and my (C) from file header
* added source of port to the commit message
* fixed multi-line comment style
* indentation only by tabs
* check for s == NULL when looking for '.' in command
* do not include support for the '.i' synonym
* rename to 'add.spread' to match 'mtdparts spread' as per Scott Wood's suggestion.

V5:
* rebased to 962ad59e25640e586e2bceabf67a628a27f8f508 of
git://git.denx.de/u-boot.git
* renumbered patches; V5 introduces patch 2/5
* return uint64_t instead of u32 for net_size
* do a quick if((cond) return
* calculate net_size by adding-up good blocks instead of subtracting bad blocks
* try to strike a balance; reuse more code between the branches of #if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) in print_partition_table
* don't push dangling lines to the right
* fix spelling errors in comments
* more hanging arguments alignment
* refactor the spread_partition function so that it delegates the bad-block counting to the mtd_get_len_incl_bad function -- as per Scott Woods' suggestion.
* don't reproduce the same call to spread_partitions on either side of the check for existing device -- as per Scott Wood's comments.

V6:
* return zero length and truncated if mtd_get_len_incl_bad is called with offset>=size -- as per Scott Wood's review comments
* fix indentation on list_entry
* don't use mtd_get_len_incl_bad to get net_size anymore as it was a bad idea. Just count up the good blocks.
* check truncated return value to handle trying to absorb bad blocks past the end of the device -- as per Scott Wood's review comments

Testing was performed with da850evm.h plus NAND enabled. Here is an example u-boot console session to demonstrate how the commands work:

-------------------------------------------------------------------------------
U-Boot > mtdparts default
U-Boot > nand bad

Device 0 bad blocks:
062c0000
0a140000
128a0000
12e20000
18bc0000
1ff80000
1ffa0000
1ffc0000
1ffe0000
U-Boot > mtdparts

device nand0 <davinci_nand.1>, # parts = 11
#: name size net size offset
mask_flags

 0: zero                0x000c0000      0x000c0000      0x00000000      1
 1: conf                0x00200000      0x00200000      0x000c0000      0
 2: kernel_a            0x00400000      0x00400000      0x002c0000      0
 3: initrd_a            0x00400000      0x00400000      0x006c0000      0
 4: rootfs_a            0x07020000      0x07000000 (!)  0x00ac0000      0
 5: kernel_b            0x00400000      0x00400000      0x07ae0000      0
 6: initrd_b            0x00400000      0x00400000      0x07ee0000      0
 7: rootfs_b            0x07020000      0x07000000 (!)  0x082e0000      0

active partition: nand0,0 - (zero) 0x00020000 @ 0x00000000

defaults:
mtdids : nand0=davinci_nand.1
mtdparts: mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a), 4m(initrd_a),112m(rootfs_a),4m(kernel_b),4m(initrd_b),112m(rootfs_b) U-Boot >
U-Boot > mtdparts spread
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b) U-Boot > mtdparts

device nand0 <davinci_nand.1>, # parts = 11
#: name size net size offset
mask_flags

 0: zero                0x000c0000      0x000c0000      0x00000000      1
 1: conf                0x00200000      0x00200000      0x000c0000      0
 2: kernel_a            0x00400000      0x00400000      0x002c0000      0
 3: initrd_a            0x00400000      0x00400000      0x006c0000      0
 4: rootfs_a            0x07040000      0x07020000 (!)  0x00ac0000      0
 5: kernel_b            0x00400000      0x00400000      0x07b00000      0
 6: initrd_b            0x00400000      0x00400000      0x07f00000      0
 7: rootfs_b            0x07040000      0x07020000 (!)  0x08300000      0

active partition: nand0,0 - (zero) 0x00020000 @ 0x00000000

defaults:
mtdids : nand0=davinci_nand.1
mtdparts: mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a), 4m(initrd_a),112m(rootfs_a),4m(kernel_b),4m(initrd_b),112m(rootfs_b) U-Boot >
U-Boot > mtdpart del rootfs_b
U-Boot > mtdpart add.spread nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b)

U-Boot > mtdpart del rootfs_b
U-Boot > mtdpart add.spread nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),114816k(rootfs_b) U-Boot >
U-Boot > mtdparts del rootfs_b
U-Boot > mtdparts add nand0 112m rootfs_b
U-Boot > print mtdparts mtdparts=mtdparts=davinci_nand.1:768k(zero)ro,2m(conf),4m(kernel_a),4m(initrd_a) ,114816k(rootfs_a),4m(kernel_b),4m(initrd_b),112m(rootfs_b)
-------------------------------------------------------------------------------

I verified the patches with checkpatch.

I confirmed that MAKEALL for 'ARM9' and '8xx' did not introduce any additional warnings.

I tested the binary size and compiler warnings on ARM9 and 8xx. The patch series saves 24 bytes of text on 8xx and 30 bytes of text on arm9 -- when none of the config options are enabled. When the config options are enabled, text grows by 1413 bytes on arm9.

Applied to u-boot-nand-flash next

Sent a followup patch for a couple minor issues.

-Scott

The get_mtd_device_nm function is called in a couple places and the string that is passed to it is not really used after the calls.

This patch regroups the calls to this function into a new function, get_mtd_info.

---

V2:
* formatting: add space after 'if'
* added acked-by tag as requested by Stefan

V3:
* rebased to 54841ab50c20d6fa6c9cc3eb826989da3a22d934 of
git://git.denx.de/u-boot.git

V4:
* rebased to b417260d871d4d8d336c160d95ed40cc8c0fb0fa of
git://git.denx.de/u-boot.git
* fixed multi-line comment style

---
 common/cmd_mtdparts.c |   45 ++++++++++++++++++++++++++++++---------------
 1 files changed, 30 insertions(+), 15 deletions(-)

index ceec5a9..772ad54 100644
@@ -286,6 +286,29 @@ static void current_save(void)
 	index_partitions();
 }
 
+
+/**
+ * Produce a mtd_info given a type and num.
+ *
+ * @param type mtd type
+ * @param num mtd number
+ * @param mtd a pointer to an mtd_info instance (output)
+ * @return 0 if device is valid, 1 otherwise
+ */
+static int get_mtd_info(u8 type, u8 num, struct mtd_info **mtd)
+{
+	char mtd_dev[16];
+
+	sprintf(mtd_dev, "%s%d", MTD_DEV_TYPE(type), num);
+	*mtd = get_mtd_device_nm(mtd_dev);
+	if (IS_ERR(*mtd)) {
+		printf("Device %s not found!\n", mtd_dev);
+		return 1;
+	}
+
+	return 0;
+}
+
 /**
  * Performs sanity check for supplied flash partition.
  * Table of existing MTD flash devices is searched and partition device

@@ -297,17 +320,12 @@ static void current_save(void)
  */
 static int part_validate_eraseblock(struct mtdids *id, struct part_info *part)
 {
-	struct mtd_info *mtd;
-	char mtd_dev[16];
+	struct mtd_info *mtd = NULL;
 	int i, j;
 	ulong start;
 
-	sprintf(mtd_dev, "%s%d", MTD_DEV_TYPE(id->type), id->num);
-	mtd = get_mtd_device_nm(mtd_dev);
-	if (IS_ERR(mtd)) {
-		printf("Partition %s not found on device %s!\n", part->name, mtd_dev);
+	if (get_mtd_info(id->type, id->num, &mtd))
 		return 1;
-	}
 
 	part->sector_size = mtd->erasesize;
 

@@ -684,20 +702,17 @@ static int part_parse(const char *const partdef, const char **ret, struct part_i
 /**
  * Check device number to be within valid range for given device type.
  *
- * @param dev device to validate
+ * @param type mtd type
+ * @param num mtd number
+ * @param size a pointer to the size of the mtd device (output)
  * @return 0 if device is valid, 1 otherwise
  */
 int mtd_device_validate(u8 type, u8 num, u32 *size)
 {
-	struct mtd_info *mtd;
-	char mtd_dev[16];
+	struct mtd_info *mtd = NULL;
 
-	sprintf(mtd_dev, "%s%d", MTD_DEV_TYPE(type), num);
-	mtd = get_mtd_device_nm(mtd_dev);
-	if (IS_ERR(mtd)) {
-		printf("Device %s not found!\n", mtd_dev);
+	if (get_mtd_info(type, num, &mtd))
 		return 1;
-	}
 
 	*size = mtd->size;
 

1.7.0.4

This patch adds an additional column to the output of list_partitions. The additional column will contain the net size and a '(!)' beside it if the net size is not equal to the partition size.

---

V2:
* formatting: spaces after 'if' and 'for'
* the entire new feature is conditional on a macro, there is now a zero-byte binary size impact when the macro is not defined.
* return the net parition size directly from net_part_size instead of using an output variable

V3:
* rebased to 54841ab50c20d6fa6c9cc3eb826989da3a22d934 of
git://git.denx.de/u-boot.git
* fix line length over 80 chars
* update copyright of cmd_mtdparts.c

V4:
* rebased to b417260d871d4d8d336c160d95ed40cc8c0fb0fa of
git://git.denx.de/u-boot.git
* removed copyright statement and changelog from file header
* re-grouped list_partition #ifdefs into one
* fixed multi-line comment style

---
 common/cmd_mtdparts.c |   70 +++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 65 insertions(+), 5 deletions(-)

index 772ad54..500a38e 100644
@@ -1215,18 +1215,65 @@ static int generate_mtdparts_save(char *buf, u32 buflen)
 	return ret;
 }
 
+#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
 /**
- * Format and print out a partition list for each device from global device
- * list.
+ * Get the net size (w/o bad blocks) of the given partition.
+ *
+ * @param mtd the mtd info
+ * @param part the partition
+ * @return the calculated net size of this partition
  */
-static void list_partitions(void)
+static u32 net_part_size(struct mtd_info *mtd, struct part_info *part)
+{
+	if (mtd->block_isbad) {
+		u32 i, bb_delta = 0;
+
+		for (i = 0; i < part->size; i += mtd->erasesize) {
+			if (mtd->block_isbad(mtd, part->offset + i))
+				bb_delta += mtd->erasesize;
+		}
+
+		return part->size - bb_delta;
+	} else {
+		return part->size;
+	}
+}
+#endif
+
+static void print_partition_table(void)
 {
 	struct list_head *dentry, *pentry;
 	struct part_info *part;
 	struct mtd_device *dev;
 	int part_num;
 
-	debug("\n---list_partitions---\n");
+#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
+	list_for_each(dentry, &devices) {
+		struct mtd_info *mtd;
+
+		dev = list_entry(dentry, struct mtd_device, link);
+		printf("\ndevice %s%d <%s>, # parts = %d\n",
+				MTD_DEV_TYPE(dev->id->type), dev->id->num,
+				dev->id->mtd_id, dev->num_parts);
+		printf(" #: name\t\tsize\t\tnet size\toffset\t\tmask_flags\n");
+
+		if (get_mtd_info(dev->id->type, dev->id->num, &mtd))
+			return;
+
+		/* list partitions for given device */
+		part_num = 0;
+		list_for_each(pentry, &dev->parts) {
+			u32 net_size;
+			char *size_note;
+
+			part = list_entry(pentry, struct part_info, link);
+			net_size = net_part_size(mtd, part);
+			size_note = part->size == net_size ? " " : " (!)";
+			printf("%2d: %-20s0x%08x\t0x%08x%s\t0x%08x\t%d\n",
+					part_num, part->name, part->size,
+					net_size, size_note, part->offset,
+					part->mask_flags);
+#else /* !defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) */
 	list_for_each(dentry, &devices) {
 		dev = list_entry(dentry, struct mtd_device, link);
 		printf("\ndevice %s%d <%s>, # parts = %d\n",

@@ -1241,12 +1288,25 @@ static void list_partitions(void)
 			printf("%2d: %-20s0x%08x\t0x%08x\t%d\n",
 					part_num, part->name, part->size,
 					part->offset, part->mask_flags);
-
+#endif /* defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) */
 			part_num++;
 		}
 	}
+
 	if (list_empty(&devices))
 		printf("no partitions defined\n");
+}
+
+/**
+ * Format and print out a partition list for each device from global device
+ * list.
+ */
+static void list_partitions(void)
+{
+	struct part_info *part;
+
+	debug("\n---list_partitions---\n");
+	print_partition_table();
 
 	/* current_mtd_dev is not NULL only when we have non empty device list */
 	if (current_mtd_dev) {

1.7.0.4

Hi Scott,

Thank you for reviewing patches 2-4.

My mistake.

Will do.

Right. I think you told me this in the env.oob review also. I'll keep this in mind for future submissions.

I definitely could. :)

I had everything-possible shared between the branches in v3 but I think I took it too far since:

I'll try my best to strike a balance here in v5.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

I understand that (at some point) we need to support part->size uint64_t but the HEAD of u-boot [1], u-boot-nand-flash/master [2] and u-boot-nand-flash/next [3] have u32 for part->size. Changing this to uint64_t would require reformatting the list_partitions size column as a 64bit hex number -- as would treating the net_size there as a uint64_t. This would be a change with impact of greater scope than intended with this patchset.

I hope it will be acceptable to do the net_part_size return as uin64_t and continue treating both the part->size and net_size as u32?

Best Regards,
Ben Gardiner

[1] http://git.denx.de/?p=u-boot.git;a=blob;f=include/jffs2/load_kernel.h;h=906eb3d3cdbe9f5b539c6341c873ed87de876be5;hb=HEAD#l49
[2] http://git.denx.de/?p=u-boot/u-boot-nand-flash.git;a=blob;f=include/jffs2/load_kernel.h;h=906eb3d3cdbe9f5b539c6341c873ed87de876be5;hb=refs/heads/master#l49
[3] http://git.denx.de/?p=u-boot/u-boot-nand-flash.git;a=blob;f=include/jffs2/load_kernel.h;h=906eb3d3cdbe9f5b539c6341c873ed87de876be5;hb=refs/heads/next#l49

---
Nanometrics Inc.
http://www.nanometrics.ca

I see, I was looking at mtd_partition, not part_info. Why are we using jffs2's partition struct for anything other than jffs2? Why does jffs2 have its own partition struct at all?

-Scott

I don't know why. I can only offer the following information:

Cscope says that include/jffs2/load_kernel.h is the file in which part_info is defined.

'git blame' indicates that part_info has been used in common/cmd_mtdparts.c since 68d7d65100e84df00bca971c114092731b441090 when Stefan Roese <[email protected]> extracted the command from common/cmd_jffs2.c

The users of mtd_partition (add_one_partition and add_mtd_partitions) are called from ubi_dev_scan where the mtd_partition structure is intialized from a part_info structure.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
+1 (613) 592-6776 x239
http://www.nanometrics.ca

This patch introduces the 'spread' sub-command of the mtdparts command. This command will modify the existing mtdparts variable by increasing the size of the partitions such that 1) each partition's net size is at least as large as the size specified in the mtdparts variable and 2) each partition starts on a good block.

The new subcommand is implemented by iterating over the mtd device partitions and collecting a bad blocks count in each -- including any trailing bad blocks -- and then modifying that partitions's part_info structure and checking if the modification affects the next partition.

This patch is based on a port of the 'dynnamic partitions' feature by Harald Welte <[email protected]>; ported from commit e05835df019027391f58f9d8ce5e1257d6924798 of
git://git.openmoko.org/u-boot.git. Whereas Harald's feature used a compile-time array to specify partitions, the feature introduced by this patch uses the mtdparts environment variable.

---

V2:
* formatting: spaces after 'if' and 'for'
* trailing whitespace removed
* check for null mtd->block_isbad before dereferencing

V3:
* rebased to 54841ab50c20d6fa6c9cc3eb826989da3a22d934 of
git://git.denx.de/u-boot.git
* fix more checkpatch errors
* update copyright statement of cmd_mtdparts.c to include openmoko's copyright of the 'dynamic partitions' functionality using commit e05835df019027391f58f9d8ce5e1257d6924798 of
git://git.openmoko.org/u-boot.git as reference.

V4:
* rebased to b417260d871d4d8d336c160d95ed40cc8c0fb0fa of
git://git.denx.de/u-boot.git
* fixed multi-line comment style
* removed changelog and my (C) from file header
* added source of port to the commit message
* fixed multi-line comment style
* indentation only by tabs

---
 common/cmd_mtdparts.c |  117 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 116 insertions(+), 1 deletions(-)

index 500a38e..7e2e232 100644
@@ -15,6 +15,9 @@
  *   Parsing routines are based on driver/mtd/cmdline.c from the linux 2.4
  *   kernel tree.
  *
+ * (C) Copyright 2008
+ * Harald Welte, OpenMoko, Inc., Harald Welte <[email protected]>
+ *
  *   $Id: cmdlinepart.c,v 1.17 2004/11/26 11:18:47 lavinen Exp $
  *   Copyright 2002 SYSGO Real-Time Solutions GmbH
  *

@@ -1430,6 +1433,105 @@ static int delete_partition(const char *id)
 	return 1;
 }
 
+#if defined(CONFIG_CMD_MTDPARTS_SPREAD)
+/**
+ * Increase the size of the given partition so that it's net size is at least
+ * as large as the size member and such that the next parition would start on a
+ * good blcok if it were adjacent to this partition.
+ *
+ * @param mtd the mtd device
+ * @param part the partition
+ * @param next_offset pointer to the offset of the next partition after this
+ *                    partition's size has been modified (output)
+ */
+static void spread_partition(struct mtd_info *mtd, struct part_info *part,
+							 u32 *next_offset)
+{
+	if (!mtd->block_isbad)
+		goto out;
+
+	u32 i, bb_delta = 0;
+
+	for (i = part->offset; i - bb_delta < part->offset + part->size;
+						i += mtd->erasesize) {
+		if (mtd->block_isbad(mtd, i))
+			bb_delta += mtd->erasesize;
+	}
+
+	/*
+	 * Absorb bad blocks immeadiately following this
+	 * partition also into the partition, such that
+	 * the next partition starts with a good block.
+	 */
+	while (i < mtd->size && mtd->block_isbad(mtd, i)) {
+		bb_delta += mtd->erasesize;
+		i += mtd->erasesize;
+	}
+
+	if (part->offset + part->size + bb_delta > mtd->size) {
+		part->size = mtd->size - part->offset - bb_delta;
+		printf("truncated partition %s to %d bytes\n", part->name,
+							      part->size);
+	}
+
+	part->size += bb_delta;
+
+out:
+	*next_offset = part->offset + part->size;
+}
+
+/**
+ * Adjust all of the partition sizes, such that all partitions are at least
+ * as big as their mtdparts environment variable sizes and they each start
+ * on a good block.
+ *
+ * @return 0 on success, 1 otherwise
+ */
+static int spread_partitions(void)
+{
+	struct list_head *dentry, *pentry;
+	struct mtd_device *dev;
+	struct part_info *part;
+	struct mtd_info *mtd;
+	int part_num;
+	u32 cur_offs;
+
+	list_for_each(dentry, &devices) {
+		dev = list_entry(dentry, struct mtd_device, link);
+
+		if (get_mtd_info(dev->id->type, dev->id->num, &mtd))
+			return 1;
+
+		part_num = 0;
+		cur_offs = 0;
+		list_for_each(pentry, &dev->parts) {
+			part = list_entry(pentry, struct part_info, link);
+
+			debug("spread_partitions: device = %s%d, partition %d ="
+				" (%s) 0x%08x at 0x%08x\n",
+				MTD_DEV_TYPE(dev->id->type), dev->id->num,
+				part_num, part->name, part->size,
+						    part->offset);
+
+			if (cur_offs > part->offset)
+				part->offset = cur_offs;
+
+			spread_partition(mtd, part, &cur_offs);
+
+			part_num++;
+		}
+	}
+
+	index_partitions();
+
+	if (generate_mtdparts_save(last_parts, MTDPARTS_MAXLEN) != 0) {
+		printf("generated mtdparts too long, reseting to null\n");
+		return 1;
+	}
+	return 0;
+}
+#endif /* CONFIG_CMD_MTDPARTS_SPREAD */
+
 /**
  * Accept character string describing mtd partitions and call device_parse()
  * for each entry. Add created devices to the global devices list.

@@ -1920,6 +2022,11 @@ int do_mtdparts(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		return delete_partition(argv[2]);
 	}
 
+#if defined(CONFIG_CMD_MTDPARTS_SPREAD)
+	if ((argc == 2) && (strcmp(argv[1], "spread") == 0))
+		return spread_partitions();
+#endif /* CONFIG_CMD_MTDPARTS_SPREAD */
+
 	return cmd_usage(cmdtp);
 }
 

@@ -1943,7 +2050,15 @@ U_BOOT_CMD(
 	"mtdparts add <mtd-dev> <size>[@<offset>] [<name>] [ro]\n"
 	"    - add partition\n"
 	"mtdparts default\n"
-	"    - reset partition table to defaults\n\n"
+	"    - reset partition table to defaults\n"
+#if defined(CONFIG_CMD_MTDPARTS_SPREAD)
+	"mtdparts spread\n"
+	"    - adjust the sizes of the partitions so they are\n"
+	"      at least as big as the mtdparts variable specifies\n"
+	"      and they each start on a good block\n\n"
+#else
+	"\n"
+#endif /* CONFIG_CMD_MTDPARTS_SPREAD */
 	"-----\n\n"
 	"this command uses three environment variables:\n\n"
 	"'partition' - keeps current partition identifier\n\n"

1.7.0.4

Ok.

Yes, I think so. Good point.

Right. I think I forgot also about this formatting requirement which you pointed out in the env.oob review. I'll get it right soon enough.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

I have performed a refactoring but I have reached an impasse: the 'mtdparts spread' command is written for mtd devices whereas the get_len_incl_bad() function is for NAND devices. I extracted a function, mtd_get_len_incl_bad(), to which both the spread_partition and nand_utils.c:get_len_incl_bad() function then delegated. But since a board may have NAND enabled but not MTD_DEVICE (i.e. guruplug) I get link errors sometimes. ATM I'm thinking of leaving the original implementation of get_len_incl_bad in an #else. An alternative is to move 'mtdparts spread' to 'nand mtdparts.spread' -- only OneNAND and NAND devices (currently) have bad_block functions.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
+1 (613) 592-6776 x239
http://www.nanometrics.ca

I figured the NAND code could just call the MTD-ized get_len_incl_bad() directly.

Grr... Eventually we ought to make NAND depend on MTD_DEVICE. It's 808 bytes currently in my build, but if we could get rid of/reduce specialized client code, it could more than make up for it.

For now, I guess don't worry about sharing the code.

There's too much duplication between NAND and OneNAND as is; I'd rather do it at the MTD layer.

-Scott

Ok -- thanks, Scott. I'll re-spin on Monday.

Whew -- I'm glad I don't have to migrate all of the patches to new files. :)

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

Plus, I've got some changes to the NAND command/util code I'm about to send out that touch this -- if sharing is going to be a pain, I can go back to the version that only passes back "fits with bad blocks", "fits with no bad blocks", or "doesn't fit", and doesn't deal with 64-bit sizes because it's only used by read/write which is limited by pointer size. That simpler version is 128 bytes smaller in my build.

-Scott

I imagine you don't have to go back. I wouldn't want to make the merge harder; but as long as there is a way to get the the size-including-bad-blocks and truncation status given an offset and target size. Please continue your work and I'll find a way to make 'mtdparts spread' fit with it after your post..

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
+1 (613) 592-6776 x239
http://www.nanometrics.ca

I already did; it's smaller and slightly simpler for what it currently needs to do. It would still be easy to switch to using the MTD function later.

-Scott

Ok, no problem. If you're interested in taking this series through the nand-testing tree I would be happy to rebase this series and integrate with your MTD changes.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

This patch introduces the 'spread' sub-command of the mtdparts command. This command will modify the existing mtdparts variable by increasing the size of the partitions such that 1) each partition's net size is at least as large as the size specified in the mtdparts variable and 2) each partition starts on a good block.

The new subcommand is implemented by iterating over the mtd device partitions and collecting a bad blocks count in each -- including any trailing bad blocks -- and then modifying that partitions's part_info structure and checking if the modification affects the next partition.

This patch is based on a port of the 'dynnamic partitions' feature by Harald Welte <[email protected]>; ported from commit e05835df019027391f58f9d8ce5e1257d6924798 of
git://git.openmoko.org/u-boot.git. Whereas Harald's feature used a compile-time array to specify partitions, the feature introduced by this patch uses the mtdparts environment variable.

---

V2:
* formatting: spaces after 'if' and 'for'
* trailing whitespace removed
* check for null mtd->block_isbad before dereferencing

V3:
* rebased to 54841ab50c20d6fa6c9cc3eb826989da3a22d934 of
git://git.denx.de/u-boot.git
* fix more checkpatch errors
* update copyright statement of cmd_mtdparts.c to include openmoko's copyright of the 'dynamic partitions' functionality using commit e05835df019027391f58f9d8ce5e1257d6924798 of
git://git.openmoko.org/u-boot.git as reference.

V4:
* rebased to b417260d871d4d8d336c160d95ed40cc8c0fb0fa of
git://git.denx.de/u-boot.git
* fixed multi-line comment style
* removed changelog and my (C) from file header
* added source of port to the commit message
* fixed multi-line comment style
* indentation only by tabs

V5:
* rebased to 962ad59e25640e586e2bceabf67a628a27f8f508 of
git://git.denx.de/u-boot.git
* renumbered from 3/4 to 4/5
* use uint64_t for net_size
* don't push dangling lines to the right
* offsets to uint64_t
* fix spelling errors in comments
* more hanging arguments alignment
* refactor the spread_partition function so that it delegates the bad-block counting to the mtd_get_len_incl_bad function -- as per Scott Woods' suggestion.

---
 common/cmd_mtdparts.c |  110 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 109 insertions(+), 1 deletions(-)

index a8912ed..fb8c77b 100644
@@ -15,6 +15,9 @@
  *   Parsing routines are based on driver/mtd/cmdline.c from the linux 2.4
  *   kernel tree.
  *
+ * (C) Copyright 2008
+ * Harald Welte, OpenMoko, Inc., Harald Welte <[email protected]>
+ *
  *   $Id: cmdlinepart.c,v 1.17 2004/11/26 11:18:47 lavinen Exp $
  *   Copyright 2002 SYSGO Real-Time Solutions GmbH
  *

@@ -1428,6 +1431,98 @@ static int delete_partition(const char *id)
 	return 1;
 }
 
+#if defined(CONFIG_CMD_MTDPARTS_SPREAD)
+/**
+ * Increase the size of the given partition so that it's net size is at least
+ * as large as the size member and such that the next partition would start on a
+ * good block if it were adjacent to this partition.
+ *
+ * @param mtd the mtd device
+ * @param part the partition
+ * @param next_offset pointer to the offset of the next partition after this
+ *                    partition's size has been modified (output)
+ */
+static void spread_partition(struct mtd_info *mtd, struct part_info *part,
+			     uint64_t *next_offset)
+{
+	uint64_t net_size, padding_size = 0;
+	int truncated;
+
+	mtd_get_len_incl_bad(mtd, part->offset, part->size, &net_size,
+			     &truncated);
+
+	/*
+	 * Absorb bad blocks immediately following this
+	 * partition also into the partition, such that
+	 * the next partition starts with a good block.
+	 */
+	if (!truncated) {
+		mtd_get_len_incl_bad(mtd, part->offset + net_size,
+				     mtd->erasesize, &padding_size, &truncated);
+		padding_size -= mtd->erasesize;
+	}
+
+	if (truncated) {
+		printf("truncated partition %s to %lld bytes\n", part->name,
+		       (uint64_t) net_size + padding_size);
+	}
+
+	part->size = net_size + padding_size;
+	*next_offset = part->offset + part->size;
+}
+
+/**
+ * Adjust all of the partition sizes, such that all partitions are at least
+ * as big as their mtdparts environment variable sizes and they each start
+ * on a good block.
+ *
+ * @return 0 on success, 1 otherwise
+ */
+static int spread_partitions(void)
+{
+	struct list_head *dentry, *pentry;
+	struct mtd_device *dev;
+	struct part_info *part;
+	struct mtd_info *mtd;
+	int part_num;
+	uint64_t cur_offs;
+
+	list_for_each(dentry, &devices) {
+		dev = list_entry(dentry, struct mtd_device, link);
+
+		if (get_mtd_info(dev->id->type, dev->id->num, &mtd))
+			return 1;
+
+		part_num = 0;
+		cur_offs = 0;
+		list_for_each(pentry, &dev->parts) {
+			part = list_entry(pentry, struct part_info, link);
+
+			debug("spread_partitions: device = %s%d, partition %d ="
+				" (%s) 0x%08x at 0x%08x\n",
+				MTD_DEV_TYPE(dev->id->type), dev->id->num,
+				part_num, part->name, part->size,
+				part->offset);
+
+			if (cur_offs > part->offset)
+				part->offset = cur_offs;
+
+			spread_partition(mtd, part, &cur_offs);
+
+			part_num++;
+		}
+	}
+
+	index_partitions();
+
+	if (generate_mtdparts_save(last_parts, MTDPARTS_MAXLEN) != 0) {
+		printf("generated mtdparts too long, reseting to null\n");
+		return 1;
+	}
+	return 0;
+}
+#endif /* CONFIG_CMD_MTDPARTS_SPREAD */
+
 /**
  * Accept character string describing mtd partitions and call device_parse()
  * for each entry. Add created devices to the global devices list.

@@ -1918,6 +2013,11 @@ int do_mtdparts(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		return delete_partition(argv[2]);
 	}
 
+#if defined(CONFIG_CMD_MTDPARTS_SPREAD)
+	if ((argc == 2) && (strcmp(argv[1], "spread") == 0))
+		return spread_partitions();
+#endif /* CONFIG_CMD_MTDPARTS_SPREAD */
+
 	return cmd_usage(cmdtp);
 }
 

@@ -1941,7 +2041,15 @@ U_BOOT_CMD(
 	"mtdparts add <mtd-dev> <size>[@<offset>] [<name>] [ro]\n"
 	"    - add partition\n"
 	"mtdparts default\n"
-	"    - reset partition table to defaults\n\n"
+	"    - reset partition table to defaults\n"
+#if defined(CONFIG_CMD_MTDPARTS_SPREAD)
+	"mtdparts spread\n"
+	"    - adjust the sizes of the partitions so they are\n"
+	"      at least as big as the mtdparts variable specifies\n"
+	"      and they each start on a good block\n\n"
+#else
+	"\n"
+#endif /* CONFIG_CMD_MTDPARTS_SPREAD */
 	"-----\n\n"
 	"this command uses three environment variables:\n\n"
 	"'partition' - keeps current partition identifier\n\n"

1.7.0.4

Grr, s/quick/quirk/

-Scot

This (arbitrary) decision to re-assign the bad-blocks that would normally be at the start of the next partition to the end of this partition is carried forward from the design of Harald Welte <[email protected]> from the openmoko u-boot feature [1].

Since the behaviour of the read and write commands (as you well know) is to skip bad blocks, the same end result of any read or write would be obtained regardless of whether the bad-blocks were assigned to the end of this partition or the start of the the next partition -- I think this is what you are getting at with your question: there is nothing special about the first block of a partition.

One particular advantage to assigning these bad blocks to the end of the partitions is that reads and writes on any partitions occurring later during the execution of u-boot (and of Linux if the same mtdparts are passed as a boot variable) will not have to skip immediately past the bad block(s) at the beginning of the partition.

I can easily reverse the behaviour here to keep the bad blocks at the beginning of the current partition if that is what you would prefer. But unless you say so I will keep it as-is to preserve the design from openmoko.

Got it.

You're absolutely right I will add an additional check of the truncated return value.

Best Regards,
Ben Gardiner

[1] http://git.openmoko.org/?p=u-boot.git;a=commitdiff;h=e05835df019027391f58f9d8ce5e1257d6924798

---
Nanometrics Inc.
http://www.nanometrics.ca

This patch introduces the 'spread' sub-command of the mtdparts command. This command will modify the existing mtdparts variable by increasing the size of the partitions such that 1) each partition's net size is at least as large as the size specified in the mtdparts variable and 2) each partition starts on a good block.

The new subcommand is implemented by iterating over the mtd device partitions and collecting a bad blocks count in each -- including any trailing bad blocks -- and then modifying that partitions's part_info structure and checking if the modification affects the next partition.

This patch is based on a port of the 'dynnamic partitions' feature by Harald Welte <[email protected]>; ported from commit e05835df019027391f58f9d8ce5e1257d6924798 of
git://git.openmoko.org/u-boot.git. Whereas Harald's feature used a compile-time array to specify partitions, the feature introduced by this patch uses the mtdparts environment variable.

---

V2:
* formatting: spaces after 'if' and 'for'
* trailing whitespace removed
* check for null mtd->block_isbad before dereferencing

V3:
* rebased to 54841ab50c20d6fa6c9cc3eb826989da3a22d934 of
git://git.denx.de/u-boot.git
* fix more checkpatch errors
* update copyright statement of cmd_mtdparts.c to include openmoko's copyright of the 'dynamic partitions' functionality using commit e05835df019027391f58f9d8ce5e1257d6924798 of
git://git.openmoko.org/u-boot.git as reference.

V4:
* rebased to b417260d871d4d8d336c160d95ed40cc8c0fb0fa of
git://git.denx.de/u-boot.git
* fixed multi-line comment style
* removed changelog and my (C) from file header
* added source of port to the commit message
* fixed multi-line comment style
* indentation only by tabs

V5:
* rebased to 962ad59e25640e586e2bceabf67a628a27f8f508 of
git://git.denx.de/u-boot.git
* renumbered from 3/4 to 4/5
* use uint64_t for net_size
* don't push dangling lines to the right
* offsets to uint64_t
* fix spelling errors in comments
* more hanging arguments alignment
* refactor the spread_partition function so that it delegates the bad-block counting to the mtd_get_len_incl_bad function -- as per Scott Woods' suggestion.

V6:
* check truncated return value to handle trying to absorb bad blocks past the end of the device -- as per Scott Wood's review comments

---
 common/cmd_mtdparts.c |  113 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 112 insertions(+), 1 deletions(-)

index 266844f..347e409 100644
@@ -15,6 +15,9 @@
  *   Parsing routines are based on driver/mtd/cmdline.c from the linux 2.4
  *   kernel tree.
  *
+ * (C) Copyright 2008
+ * Harald Welte, OpenMoko, Inc., Harald Welte <[email protected]>
+ *
  *   $Id: cmdlinepart.c,v 1.17 2004/11/26 11:18:47 lavinen Exp $
  *   Copyright 2002 SYSGO Real-Time Solutions GmbH
  *

@@ -1424,6 +1427,101 @@ static int delete_partition(const char *id)
 	return 1;
 }
 
+#if defined(CONFIG_CMD_MTDPARTS_SPREAD)
+/**
+ * Increase the size of the given partition so that it's net size is at least
+ * as large as the size member and such that the next partition would start on a
+ * good block if it were adjacent to this partition.
+ *
+ * @param mtd the mtd device
+ * @param part the partition
+ * @param next_offset pointer to the offset of the next partition after this
+ *                    partition's size has been modified (output)
+ */
+static void spread_partition(struct mtd_info *mtd, struct part_info *part,
+			     uint64_t *next_offset)
+{
+	uint64_t net_size, padding_size = 0;
+	int truncated;
+
+	mtd_get_len_incl_bad(mtd, part->offset, part->size, &net_size,
+			     &truncated);
+
+	/*
+	 * Absorb bad blocks immediately following this
+	 * partition also into the partition, such that
+	 * the next partition starts with a good block.
+	 */
+	if (!truncated) {
+		mtd_get_len_incl_bad(mtd, part->offset + net_size,
+				     mtd->erasesize, &padding_size, &truncated);
+		if (truncated)
+			padding_size = 0;
+		else
+			padding_size -= mtd->erasesize;
+	}
+
+	if (truncated) {
+		printf("truncated partition %s to %lld bytes\n", part->name,
+		       (uint64_t) net_size + padding_size);
+	}
+
+	part->size = net_size + padding_size;
+	*next_offset = part->offset + part->size;
+}
+
+/**
+ * Adjust all of the partition sizes, such that all partitions are at least
+ * as big as their mtdparts environment variable sizes and they each start
+ * on a good block.
+ *
+ * @return 0 on success, 1 otherwise
+ */
+static int spread_partitions(void)
+{
+	struct list_head *dentry, *pentry;
+	struct mtd_device *dev;
+	struct part_info *part;
+	struct mtd_info *mtd;
+	int part_num;
+	uint64_t cur_offs;
+
+	list_for_each(dentry, &devices) {
+		dev = list_entry(dentry, struct mtd_device, link);
+
+		if (get_mtd_info(dev->id->type, dev->id->num, &mtd))
+			return 1;
+
+		part_num = 0;
+		cur_offs = 0;
+		list_for_each(pentry, &dev->parts) {
+			part = list_entry(pentry, struct part_info, link);
+
+			debug("spread_partitions: device = %s%d, partition %d ="
+				" (%s) 0x%08x at 0x%08x\n",
+				MTD_DEV_TYPE(dev->id->type), dev->id->num,
+				part_num, part->name, part->size,
+				part->offset);
+
+			if (cur_offs > part->offset)
+				part->offset = cur_offs;
+
+			spread_partition(mtd, part, &cur_offs);
+
+			part_num++;
+		}
+	}
+
+	index_partitions();
+
+	if (generate_mtdparts_save(last_parts, MTDPARTS_MAXLEN) != 0) {
+		printf("generated mtdparts too long, reseting to null\n");
+		return 1;
+	}
+	return 0;
+}
+#endif /* CONFIG_CMD_MTDPARTS_SPREAD */
+
 /**
  * Accept character string describing mtd partitions and call device_parse()
  * for each entry. Add created devices to the global devices list.

@@ -1914,6 +2012,11 @@ int do_mtdparts(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		return delete_partition(argv[2]);
 	}
 
+#if defined(CONFIG_CMD_MTDPARTS_SPREAD)
+	if ((argc == 2) && (strcmp(argv[1], "spread") == 0))
+		return spread_partitions();
+#endif /* CONFIG_CMD_MTDPARTS_SPREAD */
+
 	return cmd_usage(cmdtp);
 }
 

@@ -1937,7 +2040,15 @@ U_BOOT_CMD(
 	"mtdparts add <mtd-dev> <size>[@<offset>] [<name>] [ro]\n"
 	"    - add partition\n"
 	"mtdparts default\n"
-	"    - reset partition table to defaults\n\n"
+	"    - reset partition table to defaults\n"
+#if defined(CONFIG_CMD_MTDPARTS_SPREAD)
+	"mtdparts spread\n"
+	"    - adjust the sizes of the partitions so they are\n"
+	"      at least as big as the mtdparts variable specifies\n"
+	"      and they each start on a good block\n\n"
+#else
+	"\n"
+#endif /* CONFIG_CMD_MTDPARTS_SPREAD */
 	"-----\n\n"
 	"this command uses three environment variables:\n\n"
 	"'partition' - keeps current partition identifier\n\n"

1.7.0.4

This patch adds a new 'mtdparts add' variant: add.spread. This command variant adds a new partition to the mtdparts variable but also increases the partitions size by skipping bad blocks and aggregating any additional bad blocks found at the end of the partition.

---

V2:
* formatting: spaces after 'if' and 'for'
* trailing whitespace removed

V3:
* rebased to 54841ab50c20d6fa6c9cc3eb826989da3a22d934 of
git://git.denx.de/u-boot.git
* fix more checkpatch errors
* updating copyright to include addition of add.e command

V4:
* rebased to b417260d871d4d8d336c160d95ed40cc8c0fb0fa of
git://git.denx.de/u-boot.git
* removed changelog from file header
* check for s == NULL when looking for '.' in command
* do not include support for the '.i' synonym
* rename to 'add.spread' to match 'mtdparts spread' as per Scott Wood's suggestion.

---
 common/cmd_mtdparts.c |   36 +++++++++++++++++++++++++++++++++++-
 1 files changed, 35 insertions(+), 1 deletions(-)

index 7e2e232..6e42eff 100644
@@ -1959,9 +1959,14 @@ int do_mtdparts(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	}
 
 	/* mtdparts add <mtd-dev> <size>[@<offset>] <name> [ro] */
-	if (((argc == 5) || (argc == 6)) && (strcmp(argv[1], "add") == 0)) {
+	if (((argc == 5) || (argc == 6)) && (strncmp(argv[1], "add", 3) == 0)) {
 #define PART_ADD_DESC_MAXLEN 64
 		char tmpbuf[PART_ADD_DESC_MAXLEN];
+#if defined(CONFIG_CMD_MTDPARTS_SPREAD)
+		char *s;
+		struct mtd_info *mtd;
+		u32 next_offset;
+#endif
 		u8 type, num, len;
 		struct mtd_device *dev;
 		struct mtd_device *dev_tmp;

@@ -1996,11 +2001,36 @@ int do_mtdparts(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		debug("+ %s\t%d\t%s\n", MTD_DEV_TYPE(dev->id->type),
 				dev->id->num, dev->id->mtd_id);
 
+#if defined(CONFIG_CMD_MTDPARTS_SPREAD)
+		s = strchr(argv[1], '.');
+
+		if (get_mtd_info(dev->id->type, dev->id->num, &mtd))
+			return 1;
+#endif
+
 		if ((dev_tmp = device_find(dev->id->type, dev->id->num)) == NULL) {
+#if defined(CONFIG_CMD_MTDPARTS_SPREAD)
+			if (s && !strcmp(s, ".spread")) {
+				p = list_entry(dev->parts.next,
+					       struct part_info, link);
+				spread_partition(mtd, p, &next_offset);
+
+				debug("increased %s to %d bytes\n", p->name,
+								    p->size);
+			}
+#endif
 			device_add(dev);
 		} else {
 			/* merge new partition with existing ones*/
 			p = list_entry(dev->parts.next, struct part_info, link);
+#if defined(CONFIG_CMD_MTDPARTS_SPREAD)
+			if (s && !strcmp(s, ".spread")) {
+				spread_partition(mtd, p, &next_offset);
+
+				debug("increased %s to %d bytes\n", p->name,
+								    p->size);
+			}
+#endif
 			if (part_add(dev_tmp, p) != 0) {
 				device_del(dev);
 				return 1;

@@ -2049,6 +2079,10 @@ U_BOOT_CMD(
 	"    - delete partition (e.g. part-id = nand0,1)\n"
 	"mtdparts add <mtd-dev> <size>[@<offset>] [<name>] [ro]\n"
 	"    - add partition\n"
+#if defined(CONFIG_CMD_MTDPARTS_SPREAD)
+	"mtdparts add.spread <mtd-dev> <size>[@<offset>] [<name>] [ro]\n"
+	"    - add partition, padding size by skipping bad blocks\n"
+#endif
 	"mtdparts default\n"
 	"    - reset partition table to defaults\n"
 #if defined(CONFIG_CMD_MTDPARTS_SPREAD)

1.7.0.4

Thanks for pointing that out -- I see it now.

Ok, I'll give it a shot.

Thank you again for your review and detailed comments. I appreciate you taking the time to help me get these patches prepared. I will integrate your comments on patches 2-4 shortly.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

This patch adds a new 'mtdparts add' variant: add.spread. This command variant adds a new partition to the mtdparts variable but also increases the partitions size by skipping bad blocks and aggregating any additional bad blocks found at the end of the partition.

---

V2:
* formatting: spaces after 'if' and 'for'
* trailing whitespace removed

V3:
* rebased to 54841ab50c20d6fa6c9cc3eb826989da3a22d934 of
git://git.denx.de/u-boot.git
* fix more checkpatch errors
* updating copyright to include addition of add.e command