* [PATCH 0/2] drivers/mtd: add a core
@ 2011-12-12 17:14 Robert Jarzmik
2011-12-12 17:14 ` [PATCH 1/2] drivers/mtd: introduce {add,del}_nand_device Robert Jarzmik
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Robert Jarzmik @ 2011-12-12 17:14 UTC (permalink / raw)
To: barebox
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
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/2] drivers/mtd: introduce {add,del}_nand_device 2011-12-12 17:14 [PATCH 0/2] drivers/mtd: add a core Robert Jarzmik @ 2011-12-12 17:14 ` Robert Jarzmik 2011-12-12 17:14 ` [PATCH 2/2] drivers/mtd: add a core mtd handler Robert Jarzmik 2011-12-13 9:21 ` [PATCH 0/2] drivers/mtd: add a core Sascha Hauer 2 siblings, 0 replies; 13+ messages in thread From: Robert Jarzmik @ 2011-12-12 17:14 UTC (permalink / raw) To: barebox 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] drivers/mtd: add a core mtd handler 2011-12-12 17:14 [PATCH 0/2] drivers/mtd: add a core Robert Jarzmik 2011-12-12 17:14 ` [PATCH 1/2] drivers/mtd: introduce {add,del}_nand_device Robert Jarzmik @ 2011-12-12 17:14 ` Robert Jarzmik 2011-12-13 9:21 ` [PATCH 0/2] drivers/mtd: add a core Sascha Hauer 2 siblings, 0 replies; 13+ messages in thread From: Robert Jarzmik @ 2011-12-12 17:14 UTC (permalink / raw) To: barebox 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] drivers/mtd: add a core 2011-12-12 17:14 [PATCH 0/2] drivers/mtd: add a core Robert Jarzmik 2011-12-12 17:14 ` [PATCH 1/2] drivers/mtd: introduce {add,del}_nand_device Robert Jarzmik 2011-12-12 17:14 ` [PATCH 2/2] drivers/mtd: add a core mtd handler Robert Jarzmik @ 2011-12-13 9:21 ` Sascha Hauer 2011-12-13 10:46 ` Robert Jarzmik 2011-12-13 10:51 ` Robert Jarzmik 2 siblings, 2 replies; 13+ messages in thread From: Sascha Hauer @ 2011-12-13 9:21 UTC (permalink / raw) To: Robert Jarzmik; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] drivers/mtd: add a core 2011-12-13 9:21 ` [PATCH 0/2] drivers/mtd: add a core Sascha Hauer @ 2011-12-13 10:46 ` Robert Jarzmik 2011-12-13 11:11 ` Sascha Hauer 2011-12-13 10:51 ` Robert Jarzmik 1 sibling, 1 reply; 13+ messages in thread From: Robert Jarzmik @ 2011-12-13 10:46 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] drivers/mtd: add a core 2011-12-13 10:46 ` Robert Jarzmik @ 2011-12-13 11:11 ` Sascha Hauer 0 siblings, 0 replies; 13+ messages in thread From: Sascha Hauer @ 2011-12-13 11:11 UTC (permalink / raw) To: Robert Jarzmik; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] drivers/mtd: add a core 2011-12-13 9:21 ` [PATCH 0/2] drivers/mtd: add a core Sascha Hauer 2011-12-13 10:46 ` Robert Jarzmik @ 2011-12-13 10:51 ` Robert Jarzmik 2011-12-13 11:29 ` Sascha Hauer 1 sibling, 1 reply; 13+ messages in thread From: Robert Jarzmik @ 2011-12-13 10:51 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] drivers/mtd: add a core 2011-12-13 10:51 ` Robert Jarzmik @ 2011-12-13 11:29 ` Sascha Hauer 2011-12-13 12:35 ` Robert Jarzmik 0 siblings, 1 reply; 13+ messages in thread From: Sascha Hauer @ 2011-12-13 11:29 UTC (permalink / raw) To: Robert Jarzmik; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] drivers/mtd: add a core 2011-12-13 11:29 ` Sascha Hauer @ 2011-12-13 12:35 ` Robert Jarzmik 2011-12-13 18:58 ` Sascha Hauer 0 siblings, 1 reply; 13+ messages in thread From: Robert Jarzmik @ 2011-12-13 12:35 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] drivers/mtd: add a core 2011-12-13 12:35 ` Robert Jarzmik @ 2011-12-13 18:58 ` Sascha Hauer 2011-12-14 10:01 ` Robert Jarzmik 0 siblings, 1 reply; 13+ messages in thread From: Sascha Hauer @ 2011-12-13 18:58 UTC (permalink / raw) To: Robert Jarzmik; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] drivers/mtd: add a core 2011-12-13 18:58 ` Sascha Hauer @ 2011-12-14 10:01 ` Robert Jarzmik 2011-12-14 11:02 ` Sascha Hauer 0 siblings, 1 reply; 13+ messages in thread From: Robert Jarzmik @ 2011-12-14 10:01 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] drivers/mtd: add a core 2011-12-14 10:01 ` Robert Jarzmik @ 2011-12-14 11:02 ` Sascha Hauer 2011-12-14 14:20 ` Robert Jarzmik 0 siblings, 1 reply; 13+ messages in thread From: Sascha Hauer @ 2011-12-14 11:02 UTC (permalink / raw) To: Robert Jarzmik; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] drivers/mtd: add a core 2011-12-14 11:02 ` Sascha Hauer @ 2011-12-14 14:20 ` Robert Jarzmik 0 siblings, 0 replies; 13+ messages in thread From: Robert Jarzmik @ 2011-12-14 14:20 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-12-14 14:20 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-12-12 17:14 [PATCH 0/2] drivers/mtd: add a core Robert Jarzmik
2011-12-12 17:14 ` [PATCH 1/2] drivers/mtd: introduce {add,del}_nand_device Robert Jarzmik
2011-12-12 17:14 ` [PATCH 2/2] drivers/mtd: add a core mtd handler Robert Jarzmik
2011-12-13 9:21 ` [PATCH 0/2] drivers/mtd: add a core Sascha Hauer
2011-12-13 10:46 ` Robert Jarzmik
2011-12-13 11:11 ` Sascha Hauer
2011-12-13 10:51 ` Robert Jarzmik
2011-12-13 11:29 ` Sascha Hauer
2011-12-13 12:35 ` Robert Jarzmik
2011-12-13 18:58 ` Sascha Hauer
2011-12-14 10:01 ` Robert Jarzmik
2011-12-14 11:02 ` Sascha Hauer
2011-12-14 14:20 ` Robert Jarzmik
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox