Patchwork mtd: add NOR flash write buffer size reporting for UBI/UBIFS

login
register
mail settings
Submitter Anatolij Gustschin
Date Dec. 6, 2010, 8:26 p.m.
Message ID <1291667217-13097-1-git-send-email-agust@denx.de>
Download mbox | patch
Permalink /patch/74464/
State New
Headers show

Comments

Anatolij Gustschin - Dec. 6, 2010, 8:26 p.m.
Add "write_buffer_size" field to struct mtd_info. This field
will be used to set minimal I/O unit size (min_io_size) for UBI
on NOR flash. In case of NOR flash minimal I/O size must be equal
to the maximal size of the write buffer used by embedded flash
programming algorithm.

Flash programming from prepared write buffer performed in one
programming operation could be interrupted by a power cut or
a system reset causing corrupted (partially written) areas in
a flash sector. Knowing the size of potentially corrupted areas
UBIFS scanning and recovery algorithms are able to perform
successful recovery.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
 drivers/mtd/chips/cfi_cmdset_0001.c |    1 +
 drivers/mtd/chips/cfi_cmdset_0002.c |    4 ++++
 drivers/mtd/chips/cfi_cmdset_0020.c |    1 +
 drivers/mtd/mtdconcat.c             |    1 +
 drivers/mtd/mtdpart.c               |    1 +
 drivers/mtd/ubi/build.c             |    5 ++++-
 include/linux/mtd/mtd.h             |   12 ++++++++++++
 7 files changed, 24 insertions(+), 1 deletions(-)
Artem Bityutskiy - Dec. 14, 2010, 4:01 p.m.
Hi, thank you for the patch, but I have few requests.

On Mon, 2010-12-06 at 21:26 +0100, Anatolij Gustschin wrote:
> Add "write_buffer_size" field to struct mtd_info. This field
> will be used to set minimal I/O unit size (min_io_size) for UBI
> on NOR flash. In case of NOR flash minimal I/O size must be equal
> to the maximal size of the write buffer used by embedded flash
> programming algorithm.
> 
> Flash programming from prepared write buffer performed in one
> programming operation could be interrupted by a power cut or
> a system reset causing corrupted (partially written) areas in
> a flash sector. Knowing the size of potentially corrupted areas
> UBIFS scanning and recovery algorithms are able to perform
> successful recovery.
> 
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> ---
>  drivers/mtd/chips/cfi_cmdset_0001.c |    1 +
>  drivers/mtd/chips/cfi_cmdset_0002.c |    4 ++++
>  drivers/mtd/chips/cfi_cmdset_0020.c |    1 +
>  drivers/mtd/mtdconcat.c             |    1 +
>  drivers/mtd/mtdpart.c               |    1 +
>  drivers/mtd/ubi/build.c             |    5 ++++-
>  include/linux/mtd/mtd.h             |   12 ++++++++++++
>  7 files changed, 24 insertions(+), 1 deletions(-)

Could you please keep MTD and UBI changes in separate patches?

>  	/*
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index fe8d77e..cb902d1 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -144,6 +144,18 @@ struct mtd_info {
>  	 */
>  	uint32_t writesize;
>  
> +	/*
> +	 * Sets minimal I/O unit size (min_io_size) for UBI on NOR flash.
> +	 * In case of NOR flash minimal I/O size must be equal to the size
> +	 * of the write buffer used by internal flash programming algorithm.
> +	 * This requirement results from the fact that the flash programming
> +	 * operation could be interrupted by a power cut or a system reset
> +	 * causing corrupted (partially written) areas in a flash sector.
> +	 * Knowing the size of potentially corrupted areas UBIFS scanning
> +	 * and recovery algorithms are able to perform successful recovery.
> +	 */
> +	uint32_t write_buffer_size;

Err, no, please, add comment like that to UBI, because it MTD subsystem
is wrong place to discuss UBIFS issues.

But here, instead, explain what is this and how is it different to
writesize.

Also, what write_buffer_size is in case of NAND? I think it should be
equivalent to mtd->writesize.

If we think in terms of general MTD device model:

1. mtd->writesize is the minimal amount of bytes you can write at a
time.

2. but the MTD device can have a "write-buffer", which means it can
write multiple mtd->writesize chunks at a time. And the size of this
buffer has to be "mtd->write_buffer_size" bytes.

So if you write 4 * mtd->writesize bytes, and mtd->write_buffer_size is
2*mtd->writesize, then MTD driver can (but does not have to) do 2
mtd->writesize write operations, not 4.

So I expect a comment like that instead.

To conclude, can you please:

0. Submit an mtd.h patch which just adds the field and nice comment.
1. Submit a NOR-specific patch.
2. Submit a NAND-specific patch - just do mtd->write_buffer_size =
mtd->writesuze in 'nand_scan_tail()'.
3. Submit a OneNAND-specific patch - do the same in 'onenand_scan()'.
4. Submit a patch for mtdpart and mtdconcat.
5. Submit an UBI patch

Yes, it is more job for you, but I hope I'll not scary you off with this
request :-) Thanks!

N.B. And actually, looking at other fields in mtd_info (e.g.,
numeraseregions) makes me think that it is better to name the new field
"writebufsize" instead. This would be more consistent and shorter.

Patch

diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
index 44cbfc0..3705da5 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -455,6 +455,7 @@  struct mtd_info *cfi_cmdset_0001(struct map_info *map, int primary)
 	mtd->flags   = MTD_CAP_NORFLASH;
 	mtd->name    = map->name;
 	mtd->writesize = 1;
+	mtd->write_buffer_size = 1 << cfi->cfiq->MaxBufWriteSize;
 
 	mtd->reboot_notifier.notifier_call = cfi_intelext_reboot;
 
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 9d68ab9..33989ce 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -428,6 +428,10 @@  struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
 	mtd->flags   = MTD_CAP_NORFLASH;
 	mtd->name    = map->name;
 	mtd->writesize = 1;
+	mtd->write_buffer_size = 1 << cfi->cfiq->MaxBufWriteSize;
+
+	DEBUG(MTD_DEBUG_LEVEL3, "MTD %s(): write buffer size %d\n",
+		__func__, mtd->write_buffer_size);
 
 	mtd->reboot_notifier.notifier_call = cfi_amdstd_reboot;
 
diff --git a/drivers/mtd/chips/cfi_cmdset_0020.c b/drivers/mtd/chips/cfi_cmdset_0020.c
index 314af1f..54ad184 100644
--- a/drivers/mtd/chips/cfi_cmdset_0020.c
+++ b/drivers/mtd/chips/cfi_cmdset_0020.c
@@ -238,6 +238,7 @@  static struct mtd_info *cfi_staa_setup(struct map_info *map)
 	mtd->resume = cfi_staa_resume;
 	mtd->flags = MTD_CAP_NORFLASH & ~MTD_BIT_WRITEABLE;
 	mtd->writesize = 8; /* FIXME: Should be 0 for STMicro flashes w/out ECC */
+	mtd->write_buffer_size = 1 << cfi->cfiq->MaxBufWriteSize;
 	map->fldrv = &cfi_staa_chipdrv;
 	__module_get(THIS_MODULE);
 	mtd->name = map->name;
diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
index bf8de09..53a5e94 100644
--- a/drivers/mtd/mtdconcat.c
+++ b/drivers/mtd/mtdconcat.c
@@ -776,6 +776,7 @@  struct mtd_info *mtd_concat_create(struct mtd_info *subdev[],	/* subdevices to c
 	concat->mtd.size = subdev[0]->size;
 	concat->mtd.erasesize = subdev[0]->erasesize;
 	concat->mtd.writesize = subdev[0]->writesize;
+	concat->mtd.write_buffer_size = subdev[0]->write_buffer_size;
 	concat->mtd.subpage_sft = subdev[0]->subpage_sft;
 	concat->mtd.oobsize = subdev[0]->oobsize;
 	concat->mtd.oobavail = subdev[0]->oobavail;
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 1047ff0..ef37476 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -384,6 +384,7 @@  static struct mtd_part *allocate_partition(struct mtd_info *master,
 	slave->mtd.flags = master->flags & ~part->mask_flags;
 	slave->mtd.size = part->size;
 	slave->mtd.writesize = master->writesize;
+	slave->mtd.write_buffer_size = master->write_buffer_size;
 	slave->mtd.oobsize = master->oobsize;
 	slave->mtd.oobavail = master->oobavail;
 	slave->mtd.subpage_sft = master->subpage_sft;
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 5ebe280..9db6fff 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -670,9 +670,12 @@  static int io_init(struct ubi_device *ubi)
 	if (ubi->mtd->type == MTD_NORFLASH) {
 		ubi_assert(ubi->mtd->writesize == 1);
 		ubi->nor_flash = 1;
+		ubi_assert(ubi->mtd->write_buffer_size > 0);
+		ubi->min_io_size = ubi->mtd->write_buffer_size;
+	} else {
+		ubi->min_io_size = ubi->mtd->writesize;
 	}
 
-	ubi->min_io_size = ubi->mtd->writesize;
 	ubi->hdrs_min_io_size = ubi->mtd->writesize >> ubi->mtd->subpage_sft;
 
 	/*
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index fe8d77e..cb902d1 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -144,6 +144,18 @@  struct mtd_info {
 	 */
 	uint32_t writesize;
 
+	/*
+	 * Sets minimal I/O unit size (min_io_size) for UBI on NOR flash.
+	 * In case of NOR flash minimal I/O size must be equal to the size
+	 * of the write buffer used by internal flash programming algorithm.
+	 * This requirement results from the fact that the flash programming
+	 * operation could be interrupted by a power cut or a system reset
+	 * causing corrupted (partially written) areas in a flash sector.
+	 * Knowing the size of potentially corrupted areas UBIFS scanning
+	 * and recovery algorithms are able to perform successful recovery.
+	 */
+	uint32_t write_buffer_size;
+
 	uint32_t oobsize;   // Amount of OOB data per block (e.g. 16)
 	uint32_t oobavail;  // Available OOB bytes per block