[RFC/PATCH] mtd: m25p80: set writebufsize

[RFC/PATCH] mtd: m25p80: set writebufsize

[Brian Norris]

Using UBI on m25p80 can give messages like:

    UBI error: io_init: bad write buffer size 0 for 1 min. I/O unit

We need to initialize writebufsize; I think "page_size" is the correct
"bufsize", although I'm not sure. Comments?

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/devices/m25p80.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index f83e4d0..8808da9 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -932,6 +932,7 @@ static int __devinit m25p_probe(struct spi_device *spi)
 	ppdata.of_node = spi->dev.of_node;
 	flash->mtd.dev.parent = &spi->dev;
 	flash->page_size = info->page_size;
+	flash->mtd.writebufsize = flash->page_size;
 
 	if (info->addr_width)
 		flash->addr_width = info->addr_width;
-- 
1.7.5.4

Re: [RFC/PATCH] mtd: m25p80: set writebufsize

[Shawn J. Goff]

Yes, I ran into this problem myself and fixed it in a similar way.

Even after adding this fix, it turned out that I still couldn't boot
from a UBI volume because SPI comes up after UBI, so the UBI tries to
attach, but there is no MTD device to attach to.

On Tue, Jan 31, 2012 at 3:06 AM, Brian Norris
<computersforpeace@gmail.com> wrote:
> Using UBI on m25p80 can give messages like:
>
>    UBI error: io_init: bad write buffer size 0 for 1 min. I/O unit
>
> We need to initialize writebufsize; I think "page_size" is the correct
> "bufsize", although I'm not sure. Comments?
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/devices/m25p80.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index f83e4d0..8808da9 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -932,6 +932,7 @@ static int __devinit m25p_probe(struct spi_device *spi)
>        ppdata.of_node = spi->dev.of_node;
>        flash->mtd.dev.parent = &spi->dev;
>        flash->page_size = info->page_size;
> +       flash->mtd.writebufsize = flash->page_size;
>
>        if (info->addr_width)
>                flash->addr_width = info->addr_width;
> --
> 1.7.5.4
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC/PATCH] mtd: m25p80: set writebufsize

[Brian Norris]

On Tue, Jan 31, 2012 at 6:08 AM, Shawn J. Goff <shawn7400@gmail.com> wrote:
> Yes, I ran into this problem myself and fixed it in a similar way.

I'm wondering now: how do we guarantee that we're picking the
*correct* writebufsize and not just one that *works*? It's only used
by UBI, I think, so it depends on UBI's needs.

> Even after adding this fix, it turned out that I still couldn't boot
> from a UBI volume because SPI comes up after UBI, so the UBI tries to
> attach, but there is no MTD device to attach to.

Well, the boot order problem is a different issue :)

Brian

Re: [RFC/PATCH] mtd: m25p80: set writebufsize

[Shmulik Ladkani]

On Tue, 31 Jan 2012 09:46:40 -0800 Brian Norris <computersforpeace@gmail.com> wrote:
> I'm wondering now: how do we guarantee that we're picking the
> *correct* writebufsize and not just one that *works*? It's only used
> by UBI, I think, so it depends on UBI's needs.
> 

According to mtd.h, 'writebufsize' should be set to the "Size of the
write buffer used by the MTD".

m25p->page_size should be set to the size of the device's data buffer
(used by the Page Program operation).

Hence for m25p80, 'flash->mtd.writebufsize = flash->page_size' seems
right.

BTW 'writebufsize' is propagated to UBI's 'max_write_size' which
currently affects UBIFS write-buffer size.

It should be set to "the maximum amount of bytes the underlying flash is
able to program at a time" (according to ubifs/io.c).

Hence again, for m25p80, 'flash->page_size' seems adequate.

Regards
Shmulik

Re: [RFC/PATCH] mtd: m25p80: set writebufsize

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 2626 bytes --]

On Wed, 2012-02-01 at 11:11 +0200, Shmulik Ladkani wrote:
> On Tue, 31 Jan 2012 09:46:40 -0800 Brian Norris <computersforpeace@gmail.com> wrote:
> > I'm wondering now: how do we guarantee that we're picking the
> > *correct* writebufsize and not just one that *works*? It's only used
> > by UBI, I think, so it depends on UBI's needs.
> > 
> 
> According to mtd.h, 'writebufsize' should be set to the "Size of the
> write buffer used by the MTD".
> 
> m25p->page_size should be set to the size of the device's data buffer
> (used by the Page Program operation).
> 
> Hence for m25p80, 'flash->mtd.writebufsize = flash->page_size' seems
> right.
> 
> BTW 'writebufsize' is propagated to UBI's 'max_write_size' which
> currently affects UBIFS write-buffer size.
> 
> It should be set to "the maximum amount of bytes the underlying flash is
> able to program at a time" (according to ubifs/io.c).
> 
> Hence again, for m25p80, 'flash->page_size' seems adequate.

This parameter was introduced for NOR flashes, and in NAND flashes
context it looks weird. Probably we could document this better. But
basically, it describes how much data the driver is able to write at a
time. All NAND drivers write only one page at a time, so this parameter
should be set to NAND page size.

Imagine if you created a striping layer on top of 2 MTD devices to
speed-up the I/O. In this case you would most probably make
'writebufsize' = 2xNAND pages. UBIFS would then try to do I/O in
'writebufsize' units for optimal performance, unless there is a sync
command for example, in which case UBIFS would write the last chunk of
date in smaller 'min_io_size' (=NAND page size) unit to waste less
space.

UBIFS also needs to know max_write_size for recovery to calculate how
fare a corruption caused by an unclean reboot can span (on the above
example, a normal corruption can only affect 2 NAND pages).

To summarize:

mtd->writesize = UBIFS->min_io_size - normal min. I/O unit
mtd->writebufsize = UBIFS->max_write_size - how much data the driver may
write at a time, defines the max. corruption size, gives fastest I/O.

And there is also mtd->subpage_shft -> sub-page size :-) This exists on
some NANDs. UBI uses it to pack its headers more tightly and waste less
space. We do not make this to be our min_io_size because we assume the
I/O speed in subpage-sized units is slow. Indeed, in case of normal
NANDs the whole page is written anyway, just padded with 0xFFs.

Anyway, pushed to l2-mtd.git, thanks! Added
Cc: stable@kernel.org [2.6.38+] as well.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC/PATCH] mtd: m25p80: set writebufsize

[Shmulik Ladkani]

On Fri, 03 Feb 2012 08:14:56 +0200 Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2012-02-01 at 11:11 +0200, Shmulik Ladkani wrote:
> > According to mtd.h, 'writebufsize' should be set to the "Size of the
> > write buffer used by the MTD".
> > 
> > m25p->page_size should be set to the size of the device's data buffer
> > (used by the Page Program operation).
> > 
> > Hence for m25p80, 'flash->mtd.writebufsize = flash->page_size' seems
> > right.
> > 
> > BTW 'writebufsize' is propagated to UBI's 'max_write_size' which
> > currently affects UBIFS write-buffer size.
> > 
> > It should be set to "the maximum amount of bytes the underlying flash is
> > able to program at a time" (according to ubifs/io.c).
> > 
> > Hence again, for m25p80, 'flash->page_size' seems adequate.
> 
> This parameter was introduced for NOR flashes, and in NAND flashes
> context it looks weird. Probably we could document this better. But
> basically, it describes how much data the driver is able to write at a
> time. All NAND drivers write only one page at a time, so this parameter
> should be set to NAND page size.

Sorry, prev message was vague.

Note m25p is Serial NOR Flash (its 'writesize' is set to 1, as it can be
programmed byte at a time).

m25p->page_size denotes the *maximum* number of bytes which can be
programmed using the "Page Program" instruction of the serial interface.

This number equals the size of the device's internal write buffer.
Which also means "the maximum amount of bytes the underlying flash is
able to program at a time".

(To be honest, this is correct for two m25p devices I've worked with,
according to their data sheets. I beleive it should be correct for other
m25p devices as well)

For these reasons, setting m25p's mtd.writebufsize to m25p->page_size is
adequate.

Regards
Shmulik