[PATCH 0/2] drivers/mtd: add a core

[PATCH 0/2] drivers/mtd: add a core

[Robert Jarzmik]

This patchset aims at bringing a common core to all mtd
devices. This minimal core provides :
 - add_mtd_device()
 - del_mtd_device()

The add_mtd_device() creates 2 character devices, as
explained in the second commit.

This core will be used by drivers/mtd/devices/* (soon to
come), and hopefully drivers/mtd/nand/* if we agree on
add_mtd_device() functionality.

The core should provide read/write/erase capability which
suits all the devices (ie. nand, nor and bare).

If no agreement emerges, this core will be moved into
drivers/mtd/devices.

The nand devices are left as they were, and not converted to
the new core. This conversion will only be possible after
some feedback about the needs of nand legacy drivers.

Cheers.

--
Robert

Robert Jarzmik (2):
  drivers/mtd: introduce {add,del}_nand_device
  drivers/mtd: add a core mtd handler

 drivers/mtd/Kconfig               |    5 +
 drivers/mtd/Makefile              |    2 +
 drivers/mtd/core.c                |  314 +++++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/atmel_nand.c     |    2 +-
 drivers/mtd/nand/diskonchip.c     |    8 +-
 drivers/mtd/nand/nand.c           |    4 +-
 drivers/mtd/nand/nand.h           |    3 +
 drivers/mtd/nand/nand_base.c      |    2 +-
 drivers/mtd/nand/nand_imx.c       |    2 +-
 drivers/mtd/nand/nand_omap_gpmc.c |    2 +-
 drivers/mtd/nand/nand_s3c2410.c   |    2 +-
 drivers/mtd/nand/nomadik_nand.c   |    2 +-
 12 files changed, 336 insertions(+), 12 deletions(-)
 create mode 100644 drivers/mtd/core.c

-- 
1.7.5.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/2] drivers/mtd: introduce {add,del}_nand_device

[Robert Jarzmik]

As legacy nand support has already defined the functions
add_mtd_device and del_mtd_device, rename these to
add_nand_device and del_nand_device, as they are nand
specialized and not mtd generic.

This prevents them from clashing with the core generic add
and del mtd device.

If consensus if reached further, these nand functions could
be replaced by the core ones, as long as the core
functionality is judged rich enough to suit nand
maintainers.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/mtd/nand/atmel_nand.c     |    2 +-
 drivers/mtd/nand/diskonchip.c     |    8 ++++----
 drivers/mtd/nand/nand.c           |    4 ++--
 drivers/mtd/nand/nand.h           |    3 +++
 drivers/mtd/nand/nand_base.c      |    2 +-
 drivers/mtd/nand/nand_imx.c       |    2 +-
 drivers/mtd/nand/nand_omap_gpmc.c |    2 +-
 drivers/mtd/nand/nand_s3c2410.c   |    2 +-
 drivers/mtd/nand/nomadik_nand.c   |    2 +-
 9 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index 8cc1b51..663f51b 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -485,7 +485,7 @@ static int __init atmel_nand_probe(struct device_d *dev)
 		goto err_scan_tail;
 	}
 
-	add_mtd_device(mtd);
+	add_nand_device(mtd);
 
 	if (!res)
 		return res;
diff --git a/drivers/mtd/nand/diskonchip.c b/drivers/mtd/nand/diskonchip.c
index 2433945..688a7a9 100644
--- a/drivers/mtd/nand/diskonchip.c
+++ b/drivers/mtd/nand/diskonchip.c
@@ -1358,7 +1358,7 @@ static int __init nftl_scan_bbt(struct mtd_info *mtd)
 	   At least as nand_bbt.c is currently written. */
 	if ((ret = nand_scan_bbt(mtd, NULL)))
 		return ret;
-	add_mtd_device(mtd);
+	add_nand_device(mtd);
 #ifdef CONFIG_MTD_PARTITIONS
 	if (!no_autopart)
 		add_mtd_partitions(mtd, parts, numparts);
@@ -1418,7 +1418,7 @@ static int __init inftl_scan_bbt(struct mtd_info *mtd)
 	   do without it for non-INFTL use, since all it gives us is
 	   autopartitioning, but I want to give it more thought. */
 	if (!numparts) return -EIO;
-	add_mtd_device(mtd);
+	add_nand_device(mtd);
 #ifdef CONFIG_MTD_PARTITIONS
 	if (!no_autopart)
 		add_mtd_partitions(mtd, parts, numparts);
@@ -1687,9 +1687,9 @@ static inline int __init doc_probe(unsigned long physadr)
 		/* DBB note: i believe nand_release is necessary here, as
 		   buffers may have been allocated in nand_base.  Check with
 		   Thomas. FIX ME! */
-		/* nand_release will call del_mtd_device, but we haven't yet
+		/* nand_release will call del_nand_device, but we haven't yet
 		   added it.  This is handled without incident by
-		   del_mtd_device, as far as I can tell. */
+		   del_nand_device, as far as I can tell. */
 		nand_release(mtd);
 		kfree(mtd);
 		goto fail;
diff --git a/drivers/mtd/nand/nand.c b/drivers/mtd/nand/nand.c
index 6db21d6..84d1183 100644
--- a/drivers/mtd/nand/nand.c
+++ b/drivers/mtd/nand/nand.c
@@ -249,7 +249,7 @@ static void nand_exit_oob_cdev(struct mtd_info *mtd)
 }
 #endif
 
-int add_mtd_device(struct mtd_info *mtd)
+int add_nand_device(struct mtd_info *mtd)
 {
 	char str[16];
 
@@ -279,7 +279,7 @@ int add_mtd_device(struct mtd_info *mtd)
 	return 0;
 }
 
-int del_mtd_device (struct mtd_info *mtd)
+int del_nand_device(struct mtd_info *mtd)
 {
 	unregister_device(&mtd->class_dev);
 	nand_exit_oob_cdev(mtd);
diff --git a/drivers/mtd/nand/nand.h b/drivers/mtd/nand/nand.h
index 123258d..c8c7c51 100644
--- a/drivers/mtd/nand/nand.h
+++ b/drivers/mtd/nand/nand.h
@@ -1,6 +1,9 @@
 #ifndef __NAND_H
 #define __NAND_H
 
+int add_nand_device(struct mtd_info *mtd);
+int del_nand_device(struct mtd_info *mtd);
+
 int nand_read_oob_std(struct mtd_info *mtd, struct nand_chip *chip,
 			     int page, int sndcmd);
 int nand_write_oob_std(struct mtd_info *mtd, struct nand_chip *chip,
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index ec0f4a3..01244f0 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1461,7 +1461,7 @@ void nand_release(struct mtd_info *mtd)
 	struct nand_chip *chip = mtd->priv;
 
 	/* Deregister the device */
-	del_mtd_device(mtd);
+	del_nand_device(mtd);
 
 	/* Free bad block table memory */
 	kfree(chip->bbt);
diff --git a/drivers/mtd/nand/nand_imx.c b/drivers/mtd/nand/nand_imx.c
index c8c69d6..d5d1226 100644
--- a/drivers/mtd/nand/nand_imx.c
+++ b/drivers/mtd/nand/nand_imx.c
@@ -1176,7 +1176,7 @@ static int __init imxnd_probe(struct device_d *dev)
 		goto escan;
 	}
 
-	add_mtd_device(mtd);
+	add_nand_device(mtd);
 
 	dev->priv = host;
 
diff --git a/drivers/mtd/nand/nand_omap_gpmc.c b/drivers/mtd/nand/nand_omap_gpmc.c
index d5e642a..8d4d7b4 100644
--- a/drivers/mtd/nand/nand_omap_gpmc.c
+++ b/drivers/mtd/nand/nand_omap_gpmc.c
@@ -956,7 +956,7 @@ static int gpmc_nand_probe(struct device_d *pdev)
 	dev_set_param(pdev, "eccmode", ecc_mode_strings[pdata->ecc_mode]);
 
 	/* We are all set to register with the system now! */
-	err = add_mtd_device(minfo);
+	err = add_nand_device(minfo);
 	if (err) {
 		dev_dbg(pdev, "device registration failed\n");
 		goto out_release_mem;
diff --git a/drivers/mtd/nand/nand_s3c2410.c b/drivers/mtd/nand/nand_s3c2410.c
index c5f5d97..f5910e1 100644
--- a/drivers/mtd/nand/nand_s3c2410.c
+++ b/drivers/mtd/nand/nand_s3c2410.c
@@ -485,7 +485,7 @@ static int s3c24x0_nand_probe(struct device_d *dev)
 		goto on_error;
 	}
 
-	return add_mtd_device(mtd);
+	return add_nand_device(mtd);
 	
 on_error:
 	free(host);
diff --git a/drivers/mtd/nand/nomadik_nand.c b/drivers/mtd/nand/nomadik_nand.c
index c1e93ad..2719f8b 100644
--- a/drivers/mtd/nand/nomadik_nand.c
+++ b/drivers/mtd/nand/nomadik_nand.c
@@ -220,7 +220,7 @@ static int nomadik_nand_probe(struct device_d *dev)
 	}
 
 	pr_info("Registering %s as whole device\n", mtd->name);
-	add_mtd_device(mtd);
+	add_nand_device(mtd);
 
 	return 0;
 
-- 
1.7.5.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] drivers/mtd: add a core mtd handler

[Robert Jarzmik]

Handles MTD device add and delete, as well as basic file
operations.

Each MTD device addition will bring 2 caracter devices:
 - /dev/mtd<N> : pure data device
 - /dev/mtdoob<N> : data+OOB device
The comment in the core explains the layout of mtdoob
device.

/dev/mtdoob is clearly designed to provide a simple way to
flash and read back a partition with all ECC and OOB filled
in. Some funky IPLs need special OOB to read the SPL, and
this device gives full access to all data of the chip (in
band and out of band).

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/mtd/Kconfig  |    5 +
 drivers/mtd/Makefile |    2 +
 drivers/mtd/core.c   |  314 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 321 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mtd/core.c

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index c499a44..3736d93 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -1,6 +1,11 @@
 menuconfig MTD
         bool "Memory Technology Device (MTD) support"
 
+config MTD_WRITE
+	bool
+	default y
+	prompt "Support writing to MTD devices"
+
 if MTD
 
 source "drivers/mtd/devices/Kconfig"
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index 85bed11..d3acc12 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -1,3 +1,5 @@
+obj-$(CONFIG_MTD)	+= core.o
 obj-$(CONFIG_NAND)	+= nand/
 obj-$(CONFIG_UBI)	+= ubi/
+obj-y			+= devices/
 obj-$(CONFIG_PARTITION_NEED_MTD)	+= partition.o
diff --git a/drivers/mtd/core.c b/drivers/mtd/core.c
new file mode 100644
index 0000000..b26573d
--- /dev/null
+++ b/drivers/mtd/core.c
@@ -0,0 +1,314 @@
+/*
+ * MTD core functions
+ *
+ * Copyright (C) 2011 Robert Jarzmik
+ *
+ * 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.
+ *
+ * The core provides 2 functions:
+ *  - add_mtd_device() : add a MTD device
+ *  - del_mtd_device() : removes a previously added MTD device
+ *
+ * Once a device is added, 2 character devices are created :
+ *  - mtd<N>
+ *  - mtdoob<N>
+ *
+ * Device mtd<N> provides access to the MTD "pages", ie. all the real data,
+ * excepting OOB. This means for reads OOB is not given back, and for writes
+ * that devices inner write function decides how to handle OOB of a page (if
+ * applicable).
+ *
+ * Device mtd_oob<N> provides acces to the MTD "pages+OOB". For example if a MTD
+ * has pages of 512 bytes and OOB of 16 bytes, mtd_oob<N> will be made of blocks
+ * of 528 bytes, with page data being followed by OOB.
+ * The layout will be: <page0> <oob0> <page1> <oob1> ... <pageN> <oobN>.
+ * This means that a read at offset 516 of 20 bytes will give the 12 last bytes
+ * of the OOB of page0, and the 8 first bytes of page1.
+ * Same thing applies for writes, which have to be page+oob aligned (ie. offset
+ * and size should be multiples of (mtd->writesize + mtd->oobsize)).
+ */
+
+#include <common.h>
+#include <malloc.h>
+#include <ioctl.h>
+#include <errno.h>
+#include <linux/mtd/mtd.h>
+
+static ssize_t mtd_read(struct cdev *cdev, void *buf, size_t count,
+			ulong offset, ulong flags)
+{
+	struct mtd_info *mtd = cdev->priv;
+	size_t retlen;
+	int ret;
+
+	ret = mtd->read(mtd, offset, count, &retlen, buf);
+	if (ret) {
+		printf("err %d\n", ret);
+		return ret;
+	}
+	return retlen;
+}
+
+static ssize_t mtd_read_unaligned(struct mtd_info *mtd, void *dst, size_t count,
+				  int skip, ulong offset)
+{
+	struct mtd_oob_ops ops;
+	ssize_t ret;
+	int partial = 0;
+	void *tmp = dst;
+
+	if (skip || count < mtd->writesize + mtd->oobsize)
+		partial = 1;
+	if (partial)
+		tmp = malloc(mtd->writesize + mtd->oobsize);
+	if (!tmp)
+		return -ENOMEM;
+	ops.mode = MTD_OOB_RAW;
+	ops.datbuf = tmp;
+	ops.len = mtd->writesize;
+	ops.oobbuf = tmp + mtd->writesize;
+	ops.ooblen = mtd->oobsize;
+	ret = mtd->read_oob(mtd, offset, &ops);
+	if (ret)
+		goto err;
+	if (partial)
+		memcpy(dst, tmp + skip, count);
+	ret = count - skip;
+err:
+	if (partial)
+		free(tmp);
+
+	return ret;
+}
+
+static ssize_t mtd_read_oob(struct cdev *cdev, void *buf, size_t count,
+			    ulong offset, ulong flags)
+{
+	struct mtd_info *mtd = cdev->priv;
+	ssize_t retlen = 0, ret = 1, toread;
+	ulong numpage;
+	int skip;
+
+	numpage = offset / (mtd->writesize + mtd->oobsize);
+	skip = offset % (mtd->writesize + mtd->oobsize);
+
+	while (ret > 0 && count > 0) {
+		toread = min_t(int, count, mtd->writesize + mtd->oobsize);
+		ret = mtd_read_unaligned(mtd, buf, toread,
+					 skip, numpage++ * mtd->writesize);
+		buf += ret;
+		skip = 0;
+		count -= ret;
+		retlen += ret;
+	}
+	if (ret < 0)
+		printf("err %d\n", ret);
+	else
+		ret = retlen;
+	return ret;
+}
+
+#ifdef MTD_WRITE
+static ssize_t mtd_write(struct cdev *cdev, const void *buf, size_t count,
+			 ulong offset, ulong flags)
+{
+	struct mtd_info *mtd = cdev->priv;
+	size_t retlen, now;
+	int ret = 0;
+	void *wrbuf = NULL;
+
+	ret = mtd->write(mtd, offset, count, &retlen, buf);
+	if (ret) {
+		printf("err %d\n", ret);
+		return ret;
+	}
+	return retlen;
+}
+
+static ssize_t mtd_write_oob(struct cdev *cdev, const void *buf, size_t count,
+			     ulong offset, ulong flags)
+{
+	struct mtd_info *mtd = cdev->priv;
+	ulong numpage;
+	struct mtd_oob_ops ops;
+	size_t retlen = 0;
+	int ret = 0;
+
+	if (offset % (mtd->writesize + mtd->oobsize) ||
+	    count % (mtd->writesize + mtd->oobsize))
+		return -EINVAL;
+	numpage = offset / (mtd->writesize + mtd->oobsize);
+
+	while (!ret && count > 0) {
+		ops.mode = MTD_OOB_RAW;
+		ops.datbuf = buf + retlen;
+		ops.len = min_t(int, count, mtd->writesize);
+		ops.oobbuf = buf + retlen + mtd->writesize;
+		ops.ooblen = mtd->oobsize;
+		ret = mtd->write_oob(mtd, mtd->writesize * numpage++, &ops);
+		count -= ops.retlen + ops.oobretlen;
+		retlen += ops.len + ops.oobretlen;
+	}
+	if (ret) {
+		printf("err %d\n", ret);
+		return ret;
+	} else {
+		return retlen;
+	}
+}
+
+static ssize_t mtd_erase(struct cdev *cdev, size_t count, unsigned long offset)
+{
+	struct mtd_info *mtd = cdev->priv;
+	struct erase_mtd erase;
+	int ret;
+
+	memset(&erase, 0, sizeof(erase));
+	erase.mtd = mtd;
+	erase.addr = offset;
+	erase.len = mtd->erasesize;
+
+	while (count > 0) {
+		debug("erase %d %d\n", erase.addr, erase.len);
+
+		ret = mtd->block_isbad(mtd, erase.addr);
+		if (ret > 0) {
+			printf("Skipping bad block at 0x%08x\n", erase.addr);
+		} else {
+			ret = mtd->erase(mtd, &erase);
+			if (ret)
+				return ret;
+		}
+
+		erase.addr += mtd->erasesize;
+		count -= count > mtd->erasesize ? mtd->erasesize : count;
+	}
+
+	return 0;
+}
+
+static ssize_t mtd_erase_oob(struct cdev *cdev, size_t count, ulong offset)
+{
+	offset = offset / (mtd->writesize + mtd->oobsize) * mtd->writesize;
+	count = count / (mtd->writesize + mtd->oobsize) * mtd->writesize;
+	return mtd_erase(cdev, count, offset);
+}
+
+#else
+static ssize_t mtd_write(struct cdev *cdev, const void *buf, size_t _count,
+			 ulong offset, ulong flags)
+{
+	return 0;
+}
+static ssize_t mtd_write_oob(struct cdev *cdev, const void *buf, size_t count,
+			     ulong offset, ulong flags)
+{
+	return 0;
+}
+static ssize_t mtd_erase(struct cdev *cdev, size_t count, ulong offset)
+{
+	return 0;
+}
+static ssize_t mtd_erase_oob(struct cdev *cdev, size_t count, ulong offset)
+{
+	return 0;
+}
+#endif
+
+static int mtd_ioctl(struct cdev *cdev, int request, void *buf)
+{
+	struct mtd_info *mtd = cdev->priv;
+	struct mtd_info_user *user = buf;
+
+	switch (request) {
+	case MEMGETBADBLOCK:
+		debug("MEMGETBADBLOCK: 0x%08lx\n", (off_t)buf);
+		return mtd->block_isbad(mtd, (off_t)buf);
+#ifdef CONFIG_MTD_WRITE
+	case MEMSETBADBLOCK:
+		debug("MEMSETBADBLOCK: 0x%08lx\n", (off_t)buf);
+		return mtd->block_markbad(mtd, (off_t)buf);
+#endif
+	case MEMGETINFO:
+		user->type	= mtd->type;
+		user->flags	= mtd->flags;
+		user->size	= mtd->size;
+		user->erasesize	= mtd->erasesize;
+		user->oobsize	= mtd->oobsize;
+		user->mtd	= mtd;
+		/* The below fields are obsolete */
+		user->ecctype	= -1;
+		user->eccsize	= 0;
+		return 0;
+	}
+
+	return 0;
+}
+
+static const struct file_operations mtd_fops = {
+	.read		= mtd_read,
+	.write		= mtd_write,
+	.erase		= mtd_erase,
+	.ioctl		= mtd_ioctl,
+	.lseek		= dev_lseek_default,
+};
+
+static const struct file_operations mtd_oob_fops = {
+	.read		= mtd_read_oob,
+	.write		= mtd_write_oob,
+	.erase		= mtd_erase_oob,
+	.ioctl		= mtd_ioctl,
+	.lseek		= dev_lseek_default,
+};
+
+int add_mtd_device(struct mtd_info *mtd)
+{
+	char str[16];
+
+	sprintf(mtd->class_dev.name, "mtd");
+	mtd->class_dev.id = -1;
+	register_device(&mtd->class_dev);
+	sprintf(str, "%u", mtd->size);
+	dev_add_param_fixed(&mtd->class_dev, "size", str);
+	sprintf(str, "%u", mtd->erasesize);
+	dev_add_param_fixed(&mtd->class_dev, "erasesize", str);
+	sprintf(str, "%u", mtd->writesize);
+	dev_add_param_fixed(&mtd->class_dev, "writesize", str);
+	sprintf(str, "%u", mtd->oobsize);
+	dev_add_param_fixed(&mtd->class_dev, "oobsize", str);
+
+	mtd->cdev.ops = (struct file_operations *)&mtd_fops;
+	mtd->cdev.size = mtd->size;
+	mtd->cdev.name = asprintf("mtd%d", mtd->class_dev.id);
+	mtd->cdev.priv = mtd;
+	mtd->cdev.dev = &mtd->class_dev;
+	mtd->cdev.mtd = mtd;
+	devfs_create(&mtd->cdev);
+
+	mtd->cdev_oob.ops = (struct file_operations *)&mtd_oob_fops;
+	mtd->cdev_oob.size = mtd->size / mtd->writesize *
+		(mtd->writesize + mtd->oobsize);
+	mtd->cdev_oob.name = asprintf("mtdoob%d", mtd->class_dev.id);
+	mtd->cdev_oob.priv = mtd;
+	mtd->cdev_oob.dev = &mtd->class_dev;
+	mtd->cdev_oob.mtd = mtd;
+	devfs_create(&mtd->cdev_oob);
+
+	return 0;
+}
+
+int del_mtd_device(struct mtd_info *mtd)
+{
+	unregister_device(&mtd->class_dev);
+	free(mtd->param_size.value);
+	free(mtd->cdev.name);
+	return 0;
+}
-- 
1.7.5.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/2] drivers/mtd: add a core

[Sascha Hauer]

Hi Robert,

On Mon, Dec 12, 2011 at 06:14:04PM +0100, Robert Jarzmik wrote:
> This patchset aims at bringing a common core to all mtd
> devices. This minimal core provides :
>  - add_mtd_device()
>  - del_mtd_device()
> 
> The add_mtd_device() creates 2 character devices, as
> explained in the second commit.
> 
> This core will be used by drivers/mtd/devices/* (soon to
> come), and hopefully drivers/mtd/nand/* if we agree on
> add_mtd_device() functionality.
> 
> The core should provide read/write/erase capability which
> suits all the devices (ie. nand, nor and bare).
> 
> If no agreement emerges, this core will be moved into
> drivers/mtd/devices.
> 
> The nand devices are left as they were, and not converted to
> the new core. This conversion will only be possible after
> some feedback about the needs of nand legacy drivers.

The mtd core looks mostly looks like a duplicate of
drivers/mtd/nand/nand.c. The differences are:

- nand.c contains a 'all 0xff' check in its write function. This
  is to make sure not to actually write data when it's all 0xff.
  Some nand chips need this because they can't be written twice
  even if we write with all 1.
- The nand_oob device is replaced with a device containing both
  oob and data.

I created the nand_oob device mainly for debugging purposes. It can be
convenient to be able to see the oob data. As this has no practical
use besides debugging it can be easily replaced with an interleaved
data/oob device. The oob device is quite inconvenient to use anyway
since it requires some calculating to get the oob data for a given
page.

So if no protests from other side come we can:

- git mv drivers/mtd/nand/nand.c drivers/mtd/core.c
- replace the oob device with the data+oob device
- apply whatever other fixes you need

I should be able to test the result on raw nand devices.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/2] drivers/mtd: add a core

[Robert Jarzmik]

Sascha Hauer <s.hauer@pengutronix.de> writes:

> So if no protests from other side come we can:
>
> - git mv drivers/mtd/nand/nand.c drivers/mtd/core.c
> - replace the oob device with the data+oob device
> - apply whatever other fixes you need
Sure, that would be great.
If no one complains in the next 5 days, I'll provide a V2 of the patch doing it
your way :
 - patch1: move nand.c into core.c
 - patch2: convert all existing nand devices to core.c (ie. add_mtd_device)
   *Warning* The device will be named "/dev/mtd<N>" and not "/dev/nand<N>". This
   can break things, especially if legacy board code relies on the "nand" device
   naming. Solutions:
     (a) Add a parameter to add_mtd_device: add_mtd_device(struct mtd_info *mtd,
     char *basename)
         => if basename == NULL, then use "mtd"
         => if basename != NULL, use basename for device name
     (b) Create a specialized add_nand_device()
     (c) Convert all legacy boards from "nand" to "mtd"
 - patch3: amend core.c to bring in the device+oob function

Does it look good to you ?

Cheers.

-- 
Robert

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/2] drivers/mtd: add a core

[Robert Jarzmik]

Sascha Hauer <s.hauer@pengutronix.de> writes:

> I created the nand_oob device mainly for debugging purposes. It can be
> convenient to be able to see the oob data. As this has no practical
> use besides debugging it can be easily replaced with an interleaved
> data/oob device. The oob device is quite inconvenient to use anyway
> since it requires some calculating to get the oob data for a given
> page.
True. What we would need to make it simple :
 - have arithmetic expressions in hush
 - have a "dd" command with options skip,bs,count
   => that is actually a requirement to flash (as cp uses blocks of 4096, while
   flash with oob wants block of writesize+oobsize which are seldom multiples of
   512).

I was thinking that a "dd" command would be handy. Would you accept it in
barebox ?

Cheers.

-- 
Robert

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/2] drivers/mtd: add a core

[Sascha Hauer]

On Tue, Dec 13, 2011 at 11:46:55AM +0100, Robert Jarzmik wrote:
> Sascha Hauer <s.hauer@pengutronix.de> writes:
> 
> > So if no protests from other side come we can:
> >
> > - git mv drivers/mtd/nand/nand.c drivers/mtd/core.c
> > - replace the oob device with the data+oob device
> > - apply whatever other fixes you need
> Sure, that would be great.
> If no one complains in the next 5 days, I'll provide a V2 of the patch doing it
> your way :
>  - patch1: move nand.c into core.c
>  - patch2: convert all existing nand devices to core.c (ie. add_mtd_device)
>    *Warning* The device will be named "/dev/mtd<N>" and not "/dev/nand<N>". This
>    can break things, especially if legacy board code relies on the "nand" device
>    naming. Solutions:
>      (a) Add a parameter to add_mtd_device: add_mtd_device(struct mtd_info *mtd,
>      char *basename)
>          => if basename == NULL, then use "mtd"
>          => if basename != NULL, use basename for device name
>      (b) Create a specialized add_nand_device()
>      (c) Convert all legacy boards from "nand" to "mtd"
>  - patch3: amend core.c to bring in the device+oob function
> 
> Does it look good to you ?

Yeah, sounds good.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/2] drivers/mtd: add a core

[Sascha Hauer]

On Tue, Dec 13, 2011 at 11:51:10AM +0100, Robert Jarzmik wrote:
> Sascha Hauer <s.hauer@pengutronix.de> writes:
> 
> > I created the nand_oob device mainly for debugging purposes. It can be
> > convenient to be able to see the oob data. As this has no practical
> > use besides debugging it can be easily replaced with an interleaved
> > data/oob device. The oob device is quite inconvenient to use anyway
> > since it requires some calculating to get the oob data for a given
> > page.
> True. What we would need to make it simple :
>  - have arithmetic expressions in hush

Uhh, have you looked at the code? You can hardly even fix a bug
without introducing another one :(

>  - have a "dd" command with options skip,bs,count
>    => that is actually a requirement to flash (as cp uses blocks of 4096, while
>    flash with oob wants block of writesize+oobsize which are seldom multiples of
>    512).

I don't know much about disk-on-chip. Do you really have to write
the images completely with oob data?

However, I don't like the idea that we have to use a special command
to flash an image.

The Nand flashes we support all can have bad blocks. For this we have
the 'bb' devices. So currently what you see in /dev/ is:

crw-------     262144 dev/nand0.barebox.bb
crw-------     131072 dev/nand0.bareboxenv.bb
crw-------    2097152 dev/nand0.kernel.bb
crw-------   64552960 dev/nand0.root.bb
crw-------   64618496 dev/nand0.root
crw-------    2097152 dev/nand0.kernel
crw-------     131072 dev/nand0.bareboxenv
crw-------     262144 dev/nand0.barebox
cr--------    2097152 dev/nand_oob0
crw-------   67108864 dev/nand0

/dev/nand0 is the full raw nand device. /dev/nand0.barebox is an example
for a partition on this device (also raw, with bad blocks).
/dev/nand0.barebox.bb is this partition, but this device automatically
skips bad blocks and this also makes sure that only writesize aligned
accesses go to the underlying layers. This way we can simply do a
'cp image /dev/nand0.kernel.bb' or a 'tftp barebox
/dev/nand0.barebox.bb'

Would that be suitable for disk-on-chip aswell?

> 
> I was thinking that a "dd" command would be handy. Would you accept it in
> barebox ?

If it can do something else than the memcpy command, why not? The memcpy
command is quite flexible and not limited to memory, see here:

http://wiki.barebox.org/doku.php?id=commands:memcpy

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/2] drivers/mtd: add a core

[Robert Jarzmik]

Sascha Hauer <s.hauer@pengutronix.de> writes:

> On Tue, Dec 13, 2011 at 11:51:10AM +0100, Robert Jarzmik wrote:
>> Sascha Hauer <s.hauer@pengutronix.de> writes:
>> 
>> > I created the nand_oob device mainly for debugging purposes. It can be
>> > convenient to be able to see the oob data. As this has no practical
>> > use besides debugging it can be easily replaced with an interleaved
>> > data/oob device. The oob device is quite inconvenient to use anyway
>> > since it requires some calculating to get the oob data for a given
>> > page.
>> True. What we would need to make it simple :
>>  - have arithmetic expressions in hush
>
> Uhh, have you looked at the code? You can hardly even fix a bug
> without introducing another one :(
Ah, a pity.

>>  - have a "dd" command with options skip,bs,count
>>    => that is actually a requirement to flash (as cp uses blocks of 4096, while
>>    flash with oob wants block of writesize+oobsize which are seldom multiples of
>>    512).
>
> I don't know much about disk-on-chip. Do you really have to write
> the images completely with oob data?
For the SPL, yes. The disk-on-chip IPL finds the SPL by checking the OOB of each
block : if it begins with "BIPO000, BIPO001, .. BIPO00<N>", then it's taken as
the Nth block of the SPL. The OOB part is crucial to load the SPL.

I think this is done that way so that even if there is a worn out block in the
middle (ie. a block that cannot be fixed anymore by ECC), it is skipped as it
has no more the "BIPOxxx" signature.

> However, I don't like the idea that we have to use a special command
> to flash an image.
...zip...
>
> /dev/nand0 is the full raw nand device. /dev/nand0.barebox is an example
> for a partition on this device (also raw, with bad blocks).
> /dev/nand0.barebox.bb is this partition, but this device automatically
> skips bad blocks and this also makes sure that only writesize aligned
> accesses go to the underlying layers. This way we can simply do a
> 'cp image /dev/nand0.kernel.bb' or a 'tftp barebox
> /dev/nand0.barebox.bb'
This relies on the fact that you assume your device writesize is a multiple of
512. If you write OOB as well, you're almost sure you won't have chunks of 512
bytes.

And when you say "/dev/nand0" is the full raw nand device, I think you mean the
"full raw *data* device without the OOB". The full raw device would be all
programmable flash memory, which encompasses data and OOB.

Now imagine this usecase : a new wonderfull flash filesystem is developped
... let's call it WFFS (wonderful filesystem). You don't want to have its
support to barebox (lack of time, resources), but you'd like to flash a
pre-prepared partition so that the linux kernel can use it as it's root
partition. How do you do it from your bootloader ?
If you had the /dev/mtdoob0 device, whatever filesystem structure is thought of,
the flashing method will always work.

> Would that be suitable for disk-on-chip aswell?
No, I don't think so, as the OOB has to be written as well, and therefore
multiples of 528 bytes should be possible. Note that if "cp" had a parameter for
the size of its buffer (currently 4096), then the "dd" would not be needed
anymore. A "cp -bs=528 image /dev/nand0.kernel" or "cp -bs=4224" would be
enough.

>> I was thinking that a "dd" command would be handy. Would you accept it in
>> barebox ?
>
> If it can do something else than the memcpy command, why not? The memcpy
> command is quite flexible and not limited to memory, see here:
But it suffers the same problem as cp, its buffer size of 4096, which is not a
multiple of 528 (in my case). So no, IMO it doesn't solve the problem.

Cheers.

-- 
Robert

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/2] drivers/mtd: add a core

[Sascha Hauer]

On Tue, Dec 13, 2011 at 01:35:56PM +0100, Robert Jarzmik wrote:
> Sascha Hauer <s.hauer@pengutronix.de> writes:
> 
> > On Tue, Dec 13, 2011 at 11:51:10AM +0100, Robert Jarzmik wrote:
> >> Sascha Hauer <s.hauer@pengutronix.de> writes:
> >> 
> >> > I created the nand_oob device mainly for debugging purposes. It can be
> >> > convenient to be able to see the oob data. As this has no practical
> >> > use besides debugging it can be easily replaced with an interleaved
> >> > data/oob device. The oob device is quite inconvenient to use anyway
> >> > since it requires some calculating to get the oob data for a given
> >> > page.
> >> True. What we would need to make it simple :
> >>  - have arithmetic expressions in hush
> >
> > Uhh, have you looked at the code? You can hardly even fix a bug
> > without introducing another one :(
> Ah, a pity.
> 
> >>  - have a "dd" command with options skip,bs,count
> >>    => that is actually a requirement to flash (as cp uses blocks of 4096, while
> >>    flash with oob wants block of writesize+oobsize which are seldom multiples of
> >>    512).
> >
> > I don't know much about disk-on-chip. Do you really have to write
> > the images completely with oob data?
> For the SPL, yes. The disk-on-chip IPL finds the SPL by checking the OOB of each
> block : if it begins with "BIPO000, BIPO001, .. BIPO00<N>", then it's taken as
> the Nth block of the SPL. The OOB part is crucial to load the SPL.
> 
> I think this is done that way so that even if there is a worn out block in the
> middle (ie. a block that cannot be fixed anymore by ECC), it is skipped as it
> has no more the "BIPOxxx" signature.
> 
> > However, I don't like the idea that we have to use a special command
> > to flash an image.
> ...zip...
> >
> > /dev/nand0 is the full raw nand device. /dev/nand0.barebox is an example
> > for a partition on this device (also raw, with bad blocks).
> > /dev/nand0.barebox.bb is this partition, but this device automatically
> > skips bad blocks and this also makes sure that only writesize aligned
> > accesses go to the underlying layers. This way we can simply do a
> > 'cp image /dev/nand0.kernel.bb' or a 'tftp barebox
> > /dev/nand0.barebox.bb'
> This relies on the fact that you assume your device writesize is a multiple of
> 512. If you write OOB as well, you're almost sure you won't have chunks of 512
> bytes.

No, it doesn't. The bb device handles whatever size it passed to it.
Given your 528 byte example and a cp buffer size of 4096 bytes the bb
devices would do the following:

- write 7 * 528 bytes to the device
- buffer 4096 - (7 * 528) = 400 bytes until the next write
- Now we have 4096 + 400 bytes, write 8 * 528 bytes to the device
- buffer 4224 - (8 * 512) = 128 bytes

and so on. The fact that lseek is not implemented makes sure that we can
safely buffer until the next write call from cp. The remaining buffer
bytes are flushed on device close.

That said, the current implementation indeed passes 512b or 2k down
since we do not have oob data in the bb devices, but it is not limited
to multiple of these sizes as input data.

> 
> And when you say "/dev/nand0" is the full raw nand device, I think you mean the
> "full raw *data* device without the OOB". The full raw device would be all
> programmable flash memory, which encompasses data and OOB.
> 
> Now imagine this usecase : a new wonderfull flash filesystem is developped
> ... let's call it WFFS (wonderful filesystem). You don't want to have its
> support to barebox (lack of time, resources), but you'd like to flash a
> pre-prepared partition so that the linux kernel can use it as it's root
> partition. How do you do it from your bootloader ?
> If you had the /dev/mtdoob0 device, whatever filesystem structure is thought of,
> the flashing method will always work.

Luckily my WFFS of the day is UBIFS which does not use oob data at all.
I'm glad it doesn't use them, because on some i.MX processors we can
only do full page writes including oob data. Additionally the hardware
ecc engine also protects the oob data which means that the classical
jffs2 usecase where jffs2 first writes its cleanmarkers to oob and the
data aftwerwards is not possible on these devices.

> 
> > Would that be suitable for disk-on-chip aswell?
> No, I don't think so, as the OOB has to be written as well, and therefore
> multiples of 528 bytes should be possible. Note that if "cp" had a parameter for
> the size of its buffer (currently 4096), then the "dd" would not be needed
> anymore. A "cp -bs=528 image /dev/nand0.kernel" or "cp -bs=4224" would be
> enough.

As explained above this won't be a problem. I see another problem
though: The oob layout is often dictated by hardware ecc engines. To
handle this the mtd layer has this oob_avail/oob_free/oob_pos[] thingy.
How does your mtd+oob device look like? Is it raw or does it care about
the the nonfree bytes in ecc? In mtd terms it would be MTD_OOB_RAW vs.
MTD_OOB_AUTO.

Do you need the mtd+oob device for writing the SPL or also for writing
your WFFS? In case of only SPL you might also add a specialized command
which automatically writes the user data and generates the BIP000n into
oob on the fly. It would have the advantage that you do not need special
host tools to generate an image.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/2] drivers/mtd: add a core

[Robert Jarzmik]

Sascha Hauer <s.hauer@pengutronix.de> writes:
> No, it doesn't. The bb device handles whatever size it passed to it.
> Given your 528 byte example and a cp buffer size of 4096 bytes the bb
> devices would do the following:
>
> - write 7 * 528 bytes to the device
> - buffer 4096 - (7 * 528) = 400 bytes until the next write
> - Now we have 4096 + 400 bytes, write 8 * 528 bytes to the device
> - buffer 4224 - (8 * 512) = 128 bytes
>
> and so on. The fact that lseek is not implemented makes sure that we can
> safely buffer until the next write call from cp. The remaining buffer
> bytes are flushed on device close.
Ah I see it now, you're actually talking about the nand.bb buffer, and not the
cp buffer, ie. nand_bb.writebuf. This buffer "stores" the extra bytes until the
next write.

>> No, I don't think so, as the OOB has to be written as well, and therefore
>> multiples of 528 bytes should be possible. Note that if "cp" had a parameter for
>> the size of its buffer (currently 4096), then the "dd" would not be needed
>> anymore. A "cp -bs=528 image /dev/nand0.kernel" or "cp -bs=4224" would be
>> enough.
>
> As explained above this won't be a problem. I see another problem
> though: The oob layout is often dictated by hardware ecc engines. To
> handle this the mtd layer has this oob_avail/oob_free/oob_pos[] thingy.
> How does your mtd+oob device look like? Is it raw or does it care about
> the the nonfree bytes in ecc? In mtd terms it would be MTD_OOB_RAW vs.
> MTD_OOB_AUTO.
It looks like pages of 512 bytes + OOB of 16 bytes (7 free, 1 for Hamming code,
7 for BCH code, 1 free).
And I definitely choose RAW, as it encompassed the AUTO case (ie. all ECC is
correctly filled in the provided source image), and the RAW case (ie. the ECC
provided can have wanted errors (thing security markers here)).
I'm sure here that this device should be raw. I could create a device with
autooob, but I don't see a usecase for it.

> Do you need the mtd+oob device for writing the SPL or also for writing
> your WFFS? In case of only SPL you might also add a specialized command
> which automatically writes the user data and generates the BIP000n into
> oob on the fly.
Ah, I won't make a specialized command. As the IPL is *rewritable*, the marker
can change, and I'll have to change the command. Better have a generic approach
here, whether would that be the "dd" option, or the specialized unseekable and
buffered "mtdoob" device.

Now, to finish this discussion, let me sum up :
 - you think the specialized unseekable buffered "mtdoob" device is better that
 a "dd" command to flash partitions because the specialized device can be filled
 in by existing "cp" or "tftp" commands, right ?
 - I think it's cleaner to do a "cp file | dd bs=528 of=/dev/mtdoob"
   I feel really nervous about the "unseekable buffered" part. It doesn't allow
   partial writes (you have to define subpartitions for that).
   Moreover, I think "dd" can have other fields of application, and is a usefull
   tool around.

But as I'm rather open minded, I'll let you choose between :
 (1) specialized mtdoob device
 (2) dd command
 (3) dd command and specialized mtdoob device

I'll encourage you considering (2) or (3) :)

Cheers.

-- 
Robert

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/2] drivers/mtd: add a core

[Sascha Hauer]

On Wed, Dec 14, 2011 at 11:01:58AM +0100, Robert Jarzmik wrote:
> Sascha Hauer <s.hauer@pengutronix.de> writes:
> > No, it doesn't. The bb device handles whatever size it passed to it.
> > Given your 528 byte example and a cp buffer size of 4096 bytes the bb
> > devices would do the following:
> >
> > - write 7 * 528 bytes to the device
> > - buffer 4096 - (7 * 528) = 400 bytes until the next write
> > - Now we have 4096 + 400 bytes, write 8 * 528 bytes to the device
> > - buffer 4224 - (8 * 512) = 128 bytes
> >
> > and so on. The fact that lseek is not implemented makes sure that we can
> > safely buffer until the next write call from cp. The remaining buffer
> > bytes are flushed on device close.
> Ah I see it now, you're actually talking about the nand.bb buffer, and not the
> cp buffer, ie. nand_bb.writebuf. This buffer "stores" the extra bytes until the
> next write.
> 
> >> No, I don't think so, as the OOB has to be written as well, and therefore
> >> multiples of 528 bytes should be possible. Note that if "cp" had a parameter for
> >> the size of its buffer (currently 4096), then the "dd" would not be needed
> >> anymore. A "cp -bs=528 image /dev/nand0.kernel" or "cp -bs=4224" would be
> >> enough.
> >
> > As explained above this won't be a problem. I see another problem
> > though: The oob layout is often dictated by hardware ecc engines. To
> > handle this the mtd layer has this oob_avail/oob_free/oob_pos[] thingy.
> > How does your mtd+oob device look like? Is it raw or does it care about
> > the the nonfree bytes in ecc? In mtd terms it would be MTD_OOB_RAW vs.
> > MTD_OOB_AUTO.
> It looks like pages of 512 bytes + OOB of 16 bytes (7 free, 1 for Hamming code,
> 7 for BCH code, 1 free).
> And I definitely choose RAW, as it encompassed the AUTO case (ie. all ECC is
> correctly filled in the provided source image), and the RAW case (ie. the ECC
> provided can have wanted errors (thing security markers here)).
> I'm sure here that this device should be raw. I could create a device with
> autooob, but I don't see a usecase for it.

Yeah, it should be raw.

> 
> > Do you need the mtd+oob device for writing the SPL or also for writing
> > your WFFS? In case of only SPL you might also add a specialized command
> > which automatically writes the user data and generates the BIP000n into
> > oob on the fly.
> Ah, I won't make a specialized command. As the IPL is *rewritable*, the marker
> can change, and I'll have to change the command. Better have a generic approach
> here, whether would that be the "dd" option, or the specialized unseekable and
> buffered "mtdoob" device.
> 
> Now, to finish this discussion, let me sum up :
>  - you think the specialized unseekable buffered "mtdoob" device is better that
>  a "dd" command to flash partitions because the specialized device can be filled
>  in by existing "cp" or "tftp" commands, right ?

Right

>  - I think it's cleaner to do a "cp file | dd bs=528 of=/dev/mtdoob"

With this approach (apart from | which we don't have in barebox) how do
you handle bad blocks? will they be silently skipped (that is the file
position is silently increased by one block by the device driver) or
will dd fail in this case? How is lseek handled? Will it position the
file pointer to the physical address on the device or will lseek
skip bad blocks?

>    I feel really nervous about the "unseekable buffered" part. It doesn't allow
>    partial writes (you have to define subpartitions for that).

Do you really need partial writes if you have one partition for the IPL
and one for the SPL?
BTW I have some work in progress to overcome this no lseek limitation.

>    Moreover, I think "dd" can have other fields of application, and is a usefull
>    tool around.

Don't mind. If you find it useful I will accept a patch for it. A
command which can be switched off costs nothing. And once I find
a usecase for that I'll be happy that it's there.

> 
> But as I'm rather open minded, I'll let you choose between :
>  (1) specialized mtdoob device
>  (2) dd command
>  (3) dd command and specialized mtdoob device
> 
> I'll encourage you considering (2) or (3) :)

The mtdoob device still does not feel very good to me. What about
partitions? If you partition a mtd device the partitions are normally
512b * numpages. On the mtdoob device they are 528b * numpages which
means that when you have something like this in your environment:

addpart nand0 128k(barebox),128k(env)

The corresponding partition on a mtd+oob device would be:

addpart nand0mtdoob 135168(barebox),135168(env)

There is no easy way in the environment to keep this in sync.

I get the feeling that what we really want is to resemble the nand_write
mtd utility for barebox. It could do everything you want without
changing the current mtd layer and it would also be possible for other
people to turn it off without overhead.

(Another idea I have (though I'm unsure if it's good), is: the bb
partitions are currently created with the 'nand -a' command. This
command with different parameters could also create other partition
types:

 -b	create a oob only device
 -c	create a mtd+oob device
)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/2] drivers/mtd: add a core

[Robert Jarzmik]

Sascha Hauer <s.hauer@pengutronix.de> writes:

> On Wed, Dec 14, 2011 at 11:01:58AM +0100, Robert Jarzmik wrote:
>>  - I think it's cleaner to do a "cp file | dd bs=528 of=/dev/mtdoob"
>
> With this approach (apart from | which we don't have in barebox) how do
> you handle bad blocks?
> will they be silently skipped (that is the file
> position is silently increased by one block by the device driver) or
> will dd fail in this case?
For the lack of piped command, that's an issue I hadn't thought of. I thought
that "struct pipe" in hush.c implied piped execution ... silly me.

> How is lseek handled? Will it position the
> file pointer to the physical address on the device or will lseek
> skip bad blocks?
No skip of bad blocks, simple physical positionning.

> Do you really need partial writes if you have one partition for the IPL
> and one for the SPL?
> BTW I have some work in progress to overcome this no lseek limitation.
No I don't need them ATM.

> Don't mind. If you find it useful I will accept a patch for it. A
> command which can be switched off costs nothing. And once I find
> a usecase for that I'll be happy that it's there.
Cool.

> The mtdoob device still does not feel very good to me. What about
> partitions? If you partition a mtd device the partitions are normally
> 512b * numpages. On the mtdoob device they are 528b * numpages which
> means that when you have something like this in your environment:
>
> addpart nand0 128k(barebox),128k(env)
>
> The corresponding partition on a mtd+oob device would be:
>
> addpart nand0mtdoob 135168(barebox),135168(env)
>
> There is no easy way in the environment to keep this in sync.
True, the "135168" does look ugly indeed :)

> I get the feeling that what we really want is to resemble the nand_write
> mtd utility for barebox. It could do everything you want without
> changing the current mtd layer and it would also be possible for other
> people to turn it off without overhead.
Yes, sounds good to shift the complexity in a nand dedicated command.
Let's call it option (4).

The tradeoff is that you won't be able to "TFTP" directly into a partition, as
there is no "tfp | nandwrite -o /dev/mtd0" possible.
Note: that doesn't bother me much, because for security I'd rather have the
update process in 2 steps :
 - first from TFTP to local file (in RAM or on SDCard)
 - then from RAM/SDCard onto the MTD device

> (Another idea I have (though I'm unsure if it's good), is: the bb
> partitions are currently created with the 'nand -a' command. This
> command with different parameters could also create other partition
> types:
>
>  -b	create a oob only device
>  -c	create a mtd+oob device
Yep, that's option (5).

As this discussion goes on, I shifted a bit my mind now. I prefer the
"nandwrite" command (with --autoplace, --noecc, --oob, --raw, --start,
--length), and the complementary "nandread", which should handle all nand
specific write needs (and my beloved dd of course :))
I was even thinking of having a unique "nand" command :
 - nand -a <dev> (legacy)
 - nand -d <dev> (legacy)
 - nand -b <ofs> <dev> (legacy)
 - nand --write [--autoplace] [--noecc] [--oob] [--skipbad] [--raw]
   [--start ofs] [--length lg] <dev> <srcfile>
 - nand --read [--noecc] [--oob] [--start ofs] [--length lg] <dev> <dstfile>

The mtd+oob device would not be necessary with this command, and offsets and
sizes could benefit the "128k" notation.

Cheers.

-- 
Robert

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox