[PATCH] MTD: fix dataflash 64-bit divisions

[PATCH] MTD: fix dataflash 64-bit divisions

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Date: Wed, 17 Dec 2008 19:42:38 +0200
Subject: [PATCH] MTD: fix dataflash 64-bit divisions

MTD has recently been upgraded for 64-bit support, see commit
number 69423d99fc182a81f3c5db3eb5c140acc6fc64be in the
mtd-2.6.git tree (git://git.infradead.org/mtd-2.6.git)
or see this URL:
http://git.infradead.org/mtd-2.6.git?a=commit;h=69423d99fc182a81f3c5db3eb5c140acc6fc64be

Some variables in MTD data structures which were 32-bit
became 64-bit. Namely, the 'size' field in 'struct mtd_info'
and the 'addr'/'len' fields in 'struct erase_info'. This
means we have to use 'do_div' to divide them.

This patch fixes the following linking error:
ERROR: "__udivdi3" [drivers/mtd/devices/mtd_dataflash.ko] undefined!
ERROR: "__umoddi3" [drivers/mtd/devices/mtd_dataflash.ko] undefined!

This patch changes divisions of 64-bit variable so that they use
'do_div'. This patch also change some print placeholders to
get rid of gcc warnings.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: David Brownell <david-b@pacbell.net>
---
 drivers/mtd/devices/mtd_dataflash.c |   22 +++++++++++++++-------
 1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index 6dd9aff..948193c 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -16,6 +16,7 @@
 #include <linux/device.h>
 #include <linux/mutex.h>
 #include <linux/err.h>
+#include <asm/div64.h>
 
 #include <linux/spi/spi.h>
 #include <linux/spi/flash.h>
@@ -152,15 +153,20 @@ static int dataflash_erase(struct mtd_info *mtd, struct erase_info *instr)
 	struct spi_message	msg;
 	unsigned		blocksize = priv->page_size << 3;
 	uint8_t			*command;
+	uint64_t		tmp;
 
-	DEBUG(MTD_DEBUG_LEVEL2, "%s: erase addr=0x%x len 0x%x\n",
+	DEBUG(MTD_DEBUG_LEVEL2, "%s: erase addr=0x%llx len 0x%llx\n",
 			spi->dev.bus_id,
 			instr->addr, instr->len);
 
 	/* Sanity checks */
-	if ((instr->addr + instr->len) > mtd->size
-			|| (instr->len % priv->page_size) != 0
-			|| (instr->addr % priv->page_size) != 0)
+	if (instr->addr + instr->len > mtd->size)
+		return -EINVAL;
+	tmp = instr->len;
+	if (do_div(tmp, priv->page_size))
+		return -EINVAL;
+	tmp = instr->addr;
+	if (do_div(tmp, priv->page_size))
 		return -EINVAL;
 
 	spi_message_init(&msg);
@@ -178,7 +184,9 @@ static int dataflash_erase(struct mtd_info *mtd, struct erase_info *instr)
 		/* Calculate flash page address; use block erase (for speed) if
 		 * we're at a block boundary and need to erase the whole block.
 		 */
-		pageaddr = instr->addr / priv->page_size;
+		tmp = instr->len;
+		do_div(tmp, priv->page_size);
+		pageaddr = tmp;
 		do_block = (pageaddr & 0x7) == 0 && instr->len >= blocksize;
 		pageaddr = pageaddr << priv->page_offset;
 
@@ -667,8 +675,8 @@ add_dataflash_otp(struct spi_device *spi, char *name,
 	if (revision >= 'c')
 		otp_tag = otp_setup(device, revision);
 
-	dev_info(&spi->dev, "%s (%d KBytes) pagesize %d bytes%s\n",
-			name, DIV_ROUND_UP(device->size, 1024),
+	dev_info(&spi->dev, "%s (%lld KBytes) pagesize %d bytes%s\n",
+			name, (device->size + 1023) >> 10,
 			pagesize, otp_tag);
 	dev_set_drvdata(&spi->dev, priv);
 
-- 
1.5.4.3


-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH] MTD: fix dataflash 64-bit divisions

[Nicolas Pitre]

On Wed, 17 Dec 2008, Artem Bityutskiy wrote:

> -	if ((instr->addr + instr->len) > mtd->size
> -			|| (instr->len % priv->page_size) != 0
> -			|| (instr->addr % priv->page_size) != 0)
> +	if (instr->addr + instr->len > mtd->size)
> +		return -EINVAL;
> +	tmp = instr->len;
> +	if (do_div(tmp, priv->page_size))
> +		return -EINVAL;
> +	tmp = instr->addr;
> +	if (do_div(tmp, priv->page_size))
>  		return -EINVAL;

Is it possible to have priv->page_size not a power of two?  If no then 
the above test is becoming rather costly, and something like:

	if ((instr->len | instr->addr) & (priv->page_size - 1))
		return -EINVAL;

would be much cheaper.

> -		pageaddr = instr->addr / priv->page_size;
> +		tmp = instr->len;
> +		do_div(tmp, priv->page_size);
> +		pageaddr = tmp;

Here I suggest you include <linux/math64.h> and use this instead:

	pageaddr = div_u64(instr->addr, priv->page_size);


Nicolas

Re: [PATCH] MTD: fix dataflash 64-bit divisions

[David Brownell]

On Wednesday 17 December 2008, Nicolas Pitre wrote:
> On Wed, 17 Dec 2008, Artem Bityutskiy wrote:
> 
> > -	if ((instr->addr + instr->len) > mtd->size
> > -			|| (instr->len % priv->page_size) != 0
> > -			|| (instr->addr % priv->page_size) != 0)
> > +	if (instr->addr + instr->len > mtd->size)
> > +		return -EINVAL;
> > +	tmp = instr->len;
> > +	if (do_div(tmp, priv->page_size))
> > +		return -EINVAL;
> > +	tmp = instr->addr;
> > +	if (do_div(tmp, priv->page_size))
> >  		return -EINVAL;
> 
> Is it possible to have priv->page_size not a power of two?

Yes, and in fact it's probably more common to have some
extra bytes at the end of a page.  See the table right
before dataflash_probe(); the one before jedec_probe()
lists parts that can be made to use binary page sizes.

If these were NAND chips you'd think of it as OOB area ...
except it's addressible the normal way.  In particular,
the common "read all bytes starting at page N" operation
include those extra bytes.  And you *do* want to use
those primitives, to avoid a roundtrip over SPI for each
1056 or 528 (etc) page.

Re: [PATCH] MTD: fix dataflash 64-bit divisions

[David Brownell]

On Wednesday 17 December 2008, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> Date: Wed, 17 Dec 2008 19:42:38 +0200
> Subject: [PATCH] MTD: fix dataflash 64-bit divisions
> 
> MTD has recently been upgraded for 64-bit support, see commit
> number 69423d99fc182a81f3c5db3eb5c140acc6fc64be in the
> mtd-2.6.git tree (git://git.infradead.org/mtd-2.6.git)

Hmm, in another thread I was just reading about how Linux
would only support the first 2 GBytes of a 4 GByte NAND
chip (Samsung MT29F16G08DAA) ... the updates to support
large pages were easy, but signed 32-bit offsets prevented
the full size from being recognized.  Slightly older parts
integrated four dies with 2K pages, not two with 4KB ones,
and gave no trouble.

Would this be part of a set of patches making 4 GByte
(and eventually, larger) NAND chips behave?

(There was one other issue reported with those chips,
having to do with wanting more ECC data than some old
kernel liked to store in the OOB area.)


> or see this URL:
> http://git.infradead.org/mtd-2.6.git?a=commit;h=69423d99fc182a81f3c5db3eb5c140acc6fc64be
> 
> Some variables in MTD data structures which were 32-bit
> became 64-bit. Namely, the 'size' field in 'struct mtd_info'
> and the 'addr'/'len' fields in 'struct erase_info'. This
> means we have to use 'do_div' to divide them.
> 
> This patch fixes the following linking error:
> ERROR: "__udivdi3" [drivers/mtd/devices/mtd_dataflash.ko] undefined!
> ERROR: "__umoddi3" [drivers/mtd/devices/mtd_dataflash.ko] undefined!
> 
> This patch changes divisions of 64-bit variable so that they use
> 'do_div'. This patch also change some print placeholders to
> get rid of gcc warnings.
> 
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: David Brownell <david-b@pacbell.net>

Looks OK to me, but I can't exactly test it in all its
glory.  ;)


> ---
>  drivers/mtd/devices/mtd_dataflash.c |   22 +++++++++++++++-------
>  1 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
> index 6dd9aff..948193c 100644
> --- a/drivers/mtd/devices/mtd_dataflash.c
> +++ b/drivers/mtd/devices/mtd_dataflash.c
> @@ -16,6 +16,7 @@
>  #include <linux/device.h>
>  #include <linux/mutex.h>
>  #include <linux/err.h>
> +#include <asm/div64.h>
>  
>  #include <linux/spi/spi.h>
>  #include <linux/spi/flash.h>
> @@ -152,15 +153,20 @@ static int dataflash_erase(struct mtd_info *mtd, struct erase_info *instr)
>  	struct spi_message	msg;
>  	unsigned		blocksize = priv->page_size << 3;
>  	uint8_t			*command;
> +	uint64_t		tmp;
>  
> -	DEBUG(MTD_DEBUG_LEVEL2, "%s: erase addr=0x%x len 0x%x\n",
> +	DEBUG(MTD_DEBUG_LEVEL2, "%s: erase addr=0x%llx len 0x%llx\n",
>  			spi->dev.bus_id,
>  			instr->addr, instr->len);
>  
>  	/* Sanity checks */
> -	if ((instr->addr + instr->len) > mtd->size
> -			|| (instr->len % priv->page_size) != 0
> -			|| (instr->addr % priv->page_size) != 0)
> +	if (instr->addr + instr->len > mtd->size)
> +		return -EINVAL;
> +	tmp = instr->len;
> +	if (do_div(tmp, priv->page_size))
> +		return -EINVAL;
> +	tmp = instr->addr;
> +	if (do_div(tmp, priv->page_size))
>  		return -EINVAL;
>  
>  	spi_message_init(&msg);
> @@ -178,7 +184,9 @@ static int dataflash_erase(struct mtd_info *mtd, struct erase_info *instr)
>  		/* Calculate flash page address; use block erase (for speed) if
>  		 * we're at a block boundary and need to erase the whole block.
>  		 */
> -		pageaddr = instr->addr / priv->page_size;
> +		tmp = instr->len;
> +		do_div(tmp, priv->page_size);
> +		pageaddr = tmp;
>  		do_block = (pageaddr & 0x7) == 0 && instr->len >= blocksize;
>  		pageaddr = pageaddr << priv->page_offset;
>  
> @@ -667,8 +675,8 @@ add_dataflash_otp(struct spi_device *spi, char *name,
>  	if (revision >= 'c')
>  		otp_tag = otp_setup(device, revision);
>  
> -	dev_info(&spi->dev, "%s (%d KBytes) pagesize %d bytes%s\n",
> -			name, DIV_ROUND_UP(device->size, 1024),
> +	dev_info(&spi->dev, "%s (%lld KBytes) pagesize %d bytes%s\n",
> +			name, (device->size + 1023) >> 10,
>  			pagesize, otp_tag);
>  	dev_set_drvdata(&spi->dev, priv);
>  
> -- 
> 1.5.4.3
> 
> 
> -- 
> Best regards,
> Artem Bityutskiy (Битюцкий Артём)
> 
> 

Re: [PATCH] MTD: fix dataflash 64-bit divisions

[Artem Bityutskiy]

On Wed, 2008-12-17 at 12:15 -0500, Nicolas Pitre wrote:
> > -		pageaddr = instr->addr / priv->page_size;
> > +		tmp = instr->len;
> > +		do_div(tmp, priv->page_size);
> > +		pageaddr = tmp;
> 
> Here I suggest you include <linux/math64.h> and use this instead:
> 
> 	pageaddr = div_u64(instr->addr, priv->page_size);

Indeed, these div_u64()/div_u64_rem() functions seem to be nicer. Never
used them before.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH] MTD: fix dataflash 64-bit divisions

[Artem Bityutskiy]

On Wed, 2008-12-17 at 09:56 -0800, David Brownell wrote:
> On Wednesday 17 December 2008, Artem Bityutskiy wrote:
> > From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> > Date: Wed, 17 Dec 2008 19:42:38 +0200
> > Subject: [PATCH] MTD: fix dataflash 64-bit divisions
> > 
> > MTD has recently been upgraded for 64-bit support, see commit
> > number 69423d99fc182a81f3c5db3eb5c140acc6fc64be in the
> > mtd-2.6.git tree (git://git.infradead.org/mtd-2.6.git)
> 
> Hmm, in another thread I was just reading about how Linux
> would only support the first 2 GBytes of a 4 GByte NAND
> chip (Samsung MT29F16G08DAA) ... the updates to support
> large pages were easy, but signed 32-bit offsets prevented
> the full size from being recognized.  Slightly older parts
> integrated four dies with 2K pages, not two with 4KB ones,
> and gave no trouble.

Yeah, we still do not support 4KiB pages, and >2GiB NANDs.

> Would this be part of a set of patches making 4 GByte
> (and eventually, larger) NAND chips behave?

Well, this patch does only part of the job - it changes
in-kernel API. Yes, makes >4GiB NANDs behave, and we tested
it with NAND simulator (nandsim).

However, all the user-space interfaces are still 32-bit.
And the interfaces are not extendible, so someone should
invent completely new MTD interfaces.

And to support 4KiB-page NANDs, which have 128bytes OOB,
one needs to change user-space interfaces (ioctls), because
they support 64-bit OOBs at max. On the other hand, I
personally do not care about OOB support, because it is
in general better to avoid any use of OOB.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH] MTD: fix dataflash 64-bit divisions

[David Brownell]

On Wednesday 17 December 2008, Artem Bityutskiy wrote:
> > Would this be part of a set of patches making 4 GByte
> > (and eventually, larger) NAND chips behave?
> 
> Well, this patch does only part of the job - it changes
> in-kernel API. Yes, makes >4GiB NANDs behave, and we tested
> it with NAND simulator (nandsim).

Good.  Larger parts seem to be easy to find nowadays,
and that particular limit seemed quite significant.


> However, all the user-space interfaces are still 32-bit.
> And the interfaces are not extendible, so someone should
> invent completely new MTD interfaces.
> 
> And to support 4KiB-page NANDs, which have 128bytes OOB,
> one needs to change user-space interfaces (ioctls), because
> they support 64-bit OOBs at max. On the other hand, I
> personally do not care about OOB support, because it is
> in general better to avoid any use of OOB.

This case was 2 GByte NAND chips with 4KiB pages.  The
important goal was being able to use them to hold much
filesystem data -- with 80 bytes hardware ECC data for
each 4KiB page -- and if the ioctls were also an issue,
I wouldn't have heard.

- Dave