[RFC PATCH] MTD: atmel_nand: optimize read/write buffer functions

[RFC PATCH] MTD: atmel_nand: optimize read/write buffer functions

[Nicolas Ferre]

For PIO NAND access functions, we use the features of the SMC:
- no need to take into account the NAND bus width: SMC will deal with this
- a word aligned memcpy on the NAND chip-select space is able to generate
  proper SMC behavior while optimizing AHB bus usage thanks to optimized memcpy
  implementation.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/mtd/nand/atmel_nand.c |   71 +++++++++++++++++-----------------------
 1 files changed, 30 insertions(+), 41 deletions(-)

diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index b300705..cb8a04b 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -160,37 +160,6 @@ static int atmel_nand_device_ready(struct mtd_info *mtd)
                 !!host->board->rdy_pin_active_low;
 }
 
-/*
- * Minimal-overhead PIO for data access.
- */
-static void atmel_read_buf8(struct mtd_info *mtd, u8 *buf, int len)
-{
-	struct nand_chip	*nand_chip = mtd->priv;
-
-	__raw_readsb(nand_chip->IO_ADDR_R, buf, len);
-}
-
-static void atmel_read_buf16(struct mtd_info *mtd, u8 *buf, int len)
-{
-	struct nand_chip	*nand_chip = mtd->priv;
-
-	__raw_readsw(nand_chip->IO_ADDR_R, buf, len / 2);
-}
-
-static void atmel_write_buf8(struct mtd_info *mtd, const u8 *buf, int len)
-{
-	struct nand_chip	*nand_chip = mtd->priv;
-
-	__raw_writesb(nand_chip->IO_ADDR_W, buf, len);
-}
-
-static void atmel_write_buf16(struct mtd_info *mtd, const u8 *buf, int len)
-{
-	struct nand_chip	*nand_chip = mtd->priv;
-
-	__raw_writesw(nand_chip->IO_ADDR_W, buf, len / 2);
-}
-
 static void dma_complete_func(void *completion)
 {
 	complete(completion);
@@ -265,33 +234,53 @@ err_buf:
 static void atmel_read_buf(struct mtd_info *mtd, u8 *buf, int len)
 {
 	struct nand_chip *chip = mtd->priv;
-	struct atmel_nand_host *host = chip->priv;
+	u32 align;
+	u8 *pbuf;
 
 	if (use_dma && len > mtd->oobsize)
 		/* only use DMA for bigger than oob size: better performances */
 		if (atmel_nand_dma_op(mtd, buf, len, 1) == 0)
 			return;
 
-	if (host->board->bus_width_16)
-		atmel_read_buf16(mtd, buf, len);
-	else
-		atmel_read_buf8(mtd, buf, len);
+	/* if no DMA operation possible, use PIO */
+	pbuf = buf;
+	align = 0x03 & ((unsigned)pbuf);
+
+	if (align) {
+		u32 align_len = 4 - align;
+
+		/* non aligned buffer: re-align to next word boundary */
+		ioread8_rep(chip->IO_ADDR_R, pbuf, align_len);
+		pbuf += align_len;
+		len -= align_len;
+	}
+	memcpy((void *)pbuf, chip->IO_ADDR_R, len);
 }
 
 static void atmel_write_buf(struct mtd_info *mtd, const u8 *buf, int len)
 {
 	struct nand_chip *chip = mtd->priv;
-	struct atmel_nand_host *host = chip->priv;
+	u32 align;
+	const u8 *pbuf;
 
 	if (use_dma && len > mtd->oobsize)
 		/* only use DMA for bigger than oob size: better performances */
 		if (atmel_nand_dma_op(mtd, (void *)buf, len, 0) == 0)
 			return;
 
-	if (host->board->bus_width_16)
-		atmel_write_buf16(mtd, buf, len);
-	else
-		atmel_write_buf8(mtd, buf, len);
+	/* if no DMA operation possible, use PIO */
+	pbuf = buf;
+	align = 0x03 & ((unsigned)pbuf);
+
+	if (align) {
+		u32 align_len = 4 - align;
+
+		/* non aligned buffer: re-align to next word boundary */
+		iowrite8_rep(chip->IO_ADDR_W, pbuf, align_len);
+		pbuf += align_len;
+		len -= align_len;
+	}
+	memcpy(chip->IO_ADDR_W, (void *)pbuf, len);
 }
 
 /*
-- 
1.7.3

[RFC PATCH] MTD: atmel_nand: optimize read/write buffer functions

[Uwe Kleine-König]

On Tue, Jun 28, 2011 at 01:50:56PM +0200, Nicolas Ferre wrote:
> For PIO NAND access functions, we use the features of the SMC:
> - no need to take into account the NAND bus width: SMC will deal with this
> - a word aligned memcpy on the NAND chip-select space is able to generate
>   proper SMC behavior while optimizing AHB bus usage thanks to optimized memcpy
>   implementation.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
>  drivers/mtd/nand/atmel_nand.c |   71 +++++++++++++++++-----------------------
>  1 files changed, 30 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index b300705..cb8a04b 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -160,37 +160,6 @@ static int atmel_nand_device_ready(struct mtd_info *mtd)
>                  !!host->board->rdy_pin_active_low;
>  }
>  
> -/*
> - * Minimal-overhead PIO for data access.
> - */
> -static void atmel_read_buf8(struct mtd_info *mtd, u8 *buf, int len)
> -{
> -	struct nand_chip	*nand_chip = mtd->priv;
> -
> -	__raw_readsb(nand_chip->IO_ADDR_R, buf, len);
> -}
> -
> -static void atmel_read_buf16(struct mtd_info *mtd, u8 *buf, int len)
> -{
> -	struct nand_chip	*nand_chip = mtd->priv;
> -
> -	__raw_readsw(nand_chip->IO_ADDR_R, buf, len / 2);
> -}
> -
> -static void atmel_write_buf8(struct mtd_info *mtd, const u8 *buf, int len)
> -{
> -	struct nand_chip	*nand_chip = mtd->priv;
> -
> -	__raw_writesb(nand_chip->IO_ADDR_W, buf, len);
> -}
> -
> -static void atmel_write_buf16(struct mtd_info *mtd, const u8 *buf, int len)
> -{
> -	struct nand_chip	*nand_chip = mtd->priv;
> -
> -	__raw_writesw(nand_chip->IO_ADDR_W, buf, len / 2);
> -}
> -
>  static void dma_complete_func(void *completion)
>  {
>  	complete(completion);
> @@ -265,33 +234,53 @@ err_buf:
>  static void atmel_read_buf(struct mtd_info *mtd, u8 *buf, int len)
>  {
>  	struct nand_chip *chip = mtd->priv;
> -	struct atmel_nand_host *host = chip->priv;
> +	u32 align;
> +	u8 *pbuf;
>  
>  	if (use_dma && len > mtd->oobsize)
>  		/* only use DMA for bigger than oob size: better performances */
>  		if (atmel_nand_dma_op(mtd, buf, len, 1) == 0)
>  			return;
>  
> -	if (host->board->bus_width_16)
> -		atmel_read_buf16(mtd, buf, len);
> -	else
> -		atmel_read_buf8(mtd, buf, len);
> +	/* if no DMA operation possible, use PIO */
> +	pbuf = buf;
> +	align = 0x03 & ((unsigned)pbuf);
> +
> +	if (align) {
> +		u32 align_len = 4 - align;
> +
> +		/* non aligned buffer: re-align to next word boundary */
> +		ioread8_rep(chip->IO_ADDR_R, pbuf, align_len);
> +		pbuf += align_len;
> +		len -= align_len;
> +	}
> +	memcpy((void *)pbuf, chip->IO_ADDR_R, len);
I think you don't need to cast to (void *). I think you need to cast the
2nd parameter instead because sparse don't like you passing an void
__iomem *.
Is it correct to read from chip->IO_ADDR_R, don't you need
chip->IO_ADDR_R + align_len? Taking this into account, does it really
help to align pbuf?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[RFC PATCH] MTD: atmel_nand: optimize read/write buffer functions

[Nicolas Ferre]

Le 28/06/2011 13:10, Uwe Kleine-K?nig :
> On Tue, Jun 28, 2011 at 01:50:56PM +0200, Nicolas Ferre wrote:
>> For PIO NAND access functions, we use the features of the SMC:
>> - no need to take into account the NAND bus width: SMC will deal with this
>> - a word aligned memcpy on the NAND chip-select space is able to generate
>>   proper SMC behavior while optimizing AHB bus usage thanks to optimized memcpy
>>   implementation.
>>
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>>  drivers/mtd/nand/atmel_nand.c |   71 +++++++++++++++++-----------------------
>>  1 files changed, 30 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
>> index b300705..cb8a04b 100644
>> --- a/drivers/mtd/nand/atmel_nand.c
>> +++ b/drivers/mtd/nand/atmel_nand.c
>> @@ -160,37 +160,6 @@ static int atmel_nand_device_ready(struct mtd_info *mtd)
>>                  !!host->board->rdy_pin_active_low;
>>  }
>>  
>> -/*
>> - * Minimal-overhead PIO for data access.
>> - */
>> -static void atmel_read_buf8(struct mtd_info *mtd, u8 *buf, int len)
>> -{
>> -	struct nand_chip	*nand_chip = mtd->priv;
>> -
>> -	__raw_readsb(nand_chip->IO_ADDR_R, buf, len);
>> -}
>> -
>> -static void atmel_read_buf16(struct mtd_info *mtd, u8 *buf, int len)
>> -{
>> -	struct nand_chip	*nand_chip = mtd->priv;
>> -
>> -	__raw_readsw(nand_chip->IO_ADDR_R, buf, len / 2);
>> -}
>> -
>> -static void atmel_write_buf8(struct mtd_info *mtd, const u8 *buf, int len)
>> -{
>> -	struct nand_chip	*nand_chip = mtd->priv;
>> -
>> -	__raw_writesb(nand_chip->IO_ADDR_W, buf, len);
>> -}
>> -
>> -static void atmel_write_buf16(struct mtd_info *mtd, const u8 *buf, int len)
>> -{
>> -	struct nand_chip	*nand_chip = mtd->priv;
>> -
>> -	__raw_writesw(nand_chip->IO_ADDR_W, buf, len / 2);
>> -}
>> -
>>  static void dma_complete_func(void *completion)
>>  {
>>  	complete(completion);
>> @@ -265,33 +234,53 @@ err_buf:
>>  static void atmel_read_buf(struct mtd_info *mtd, u8 *buf, int len)
>>  {
>>  	struct nand_chip *chip = mtd->priv;
>> -	struct atmel_nand_host *host = chip->priv;
>> +	u32 align;
>> +	u8 *pbuf;
>>  
>>  	if (use_dma && len > mtd->oobsize)
>>  		/* only use DMA for bigger than oob size: better performances */
>>  		if (atmel_nand_dma_op(mtd, buf, len, 1) == 0)
>>  			return;
>>  
>> -	if (host->board->bus_width_16)
>> -		atmel_read_buf16(mtd, buf, len);
>> -	else
>> -		atmel_read_buf8(mtd, buf, len);
>> +	/* if no DMA operation possible, use PIO */
>> +	pbuf = buf;
>> +	align = 0x03 & ((unsigned)pbuf);
>> +
>> +	if (align) {
>> +		u32 align_len = 4 - align;
>> +
>> +		/* non aligned buffer: re-align to next word boundary */
>> +		ioread8_rep(chip->IO_ADDR_R, pbuf, align_len);
>> +		pbuf += align_len;
>> +		len -= align_len;
>> +	}
>> +	memcpy((void *)pbuf, chip->IO_ADDR_R, len);
> I think you don't need to cast to (void *). I think you need to cast the
> 2nd parameter instead because sparse don't like you passing an void
> __iomem *.

Yes but if I try to cast it, sparse will notice it anyway. So yes, it
seems that I am doing something _bad_ and that the use of memcpy() here
is not the preferred way to deal with hardware FIFO.
On the other hand, the memcpy_fromio() function is far from being as
optimized as the ARM memcpy() implementation... I am wandering what is
the best solution?

> Is it correct to read from chip->IO_ADDR_R, don't you need
> chip->IO_ADDR_R + align_len?

No, it is a FIFO, so each reading from it will give the next data.

> Taking this into account, does it really help to align pbuf?

Aligning to next 32bit word address avoid having to deal with memcpy()
alignment management that reads twice to rearrange data (loosing datas
in the case of a FIFO ;-)). So yes, aligning really helps.

So, all this leads me to append RFC to this patch title...

Best regards,
-- 
Nicolas Ferre

[RFC PATCH] MTD: atmel_nand: optimize read/write buffer functions

[Russell King - ARM Linux]

On Tue, Jun 28, 2011 at 01:10:43PM +0200, Uwe Kleine-K?nig wrote:
> > @@ -265,33 +234,53 @@ err_buf:
> >  static void atmel_read_buf(struct mtd_info *mtd, u8 *buf, int len)
> >  {
> >  	struct nand_chip *chip = mtd->priv;
> > -	struct atmel_nand_host *host = chip->priv;
> > +	u32 align;
> > +	u8 *pbuf;
> >  
> >  	if (use_dma && len > mtd->oobsize)
> >  		/* only use DMA for bigger than oob size: better performances */
> >  		if (atmel_nand_dma_op(mtd, buf, len, 1) == 0)
> >  			return;
> >  
> > -	if (host->board->bus_width_16)
> > -		atmel_read_buf16(mtd, buf, len);
> > -	else
> > -		atmel_read_buf8(mtd, buf, len);
> > +	/* if no DMA operation possible, use PIO */
> > +	pbuf = buf;
> > +	align = 0x03 & ((unsigned)pbuf);
> > +
> > +	if (align) {
> > +		u32 align_len = 4 - align;
> > +
> > +		/* non aligned buffer: re-align to next word boundary */
> > +		ioread8_rep(chip->IO_ADDR_R, pbuf, align_len);
> > +		pbuf += align_len;
> > +		len -= align_len;
> > +	}
> > +	memcpy((void *)pbuf, chip->IO_ADDR_R, len);
> I think you don't need to cast to (void *). I think you need to cast the
> 2nd parameter instead because sparse don't like you passing an void
> __iomem *.
> Is it correct to read from chip->IO_ADDR_R, don't you need
> chip->IO_ADDR_R + align_len? Taking this into account, does it really
> help to align pbuf?

I think you need to read Documentation/bus-virt-phys-mapping.txt,
particularly the part after "NOTE NOTE NOTE".

Dereferencing ioremap'd memory is not permitted.  That includes passing
it to memcpy.  Even with a cast.

[RFC PATCH] MTD: atmel_nand: optimize read/write buffer functions

[Nicolas Ferre]

Le 28/06/2011 16:59, Russell King - ARM Linux :
> On Tue, Jun 28, 2011 at 01:10:43PM +0200, Uwe Kleine-K?nig wrote:
>>> @@ -265,33 +234,53 @@ err_buf:
>>>  static void atmel_read_buf(struct mtd_info *mtd, u8 *buf, int len)
>>>  {
>>>  	struct nand_chip *chip = mtd->priv;
>>> -	struct atmel_nand_host *host = chip->priv;
>>> +	u32 align;
>>> +	u8 *pbuf;
>>>  
>>>  	if (use_dma && len > mtd->oobsize)
>>>  		/* only use DMA for bigger than oob size: better performances */
>>>  		if (atmel_nand_dma_op(mtd, buf, len, 1) == 0)
>>>  			return;
>>>  
>>> -	if (host->board->bus_width_16)
>>> -		atmel_read_buf16(mtd, buf, len);
>>> -	else
>>> -		atmel_read_buf8(mtd, buf, len);
>>> +	/* if no DMA operation possible, use PIO */
>>> +	pbuf = buf;
>>> +	align = 0x03 & ((unsigned)pbuf);
>>> +
>>> +	if (align) {
>>> +		u32 align_len = 4 - align;
>>> +
>>> +		/* non aligned buffer: re-align to next word boundary */
>>> +		ioread8_rep(chip->IO_ADDR_R, pbuf, align_len);
>>> +		pbuf += align_len;
>>> +		len -= align_len;
>>> +	}
>>> +	memcpy((void *)pbuf, chip->IO_ADDR_R, len);
>> I think you don't need to cast to (void *). I think you need to cast the
>> 2nd parameter instead because sparse don't like you passing an void
>> __iomem *.
>> Is it correct to read from chip->IO_ADDR_R, don't you need
>> chip->IO_ADDR_R + align_len? Taking this into account, does it really
>> help to align pbuf?
> 
> I think you need to read Documentation/bus-virt-phys-mapping.txt,
> particularly the part after "NOTE NOTE NOTE".
> 
> Dereferencing ioremap'd memory is not permitted.  That includes passing
> it to memcpy.  Even with a cast.

So that means that I should use memcpy_fromio() even if the code if far
less optimized.

Shouldn't I re-implement some kind of IO copying function to deal with
this IO memory so that I could take advantage of 8 words bursts?

Best regards,
-- 
Nicolas Ferre

[RFC PATCH] MTD: atmel_nand: optimize read/write buffer functions

[Russell King - ARM Linux]

On Wed, Jun 29, 2011 at 03:09:59PM +0200, Nicolas Ferre wrote:
> Le 28/06/2011 16:59, Russell King - ARM Linux :
> > I think you need to read Documentation/bus-virt-phys-mapping.txt,
> > particularly the part after "NOTE NOTE NOTE".
> > 
> > Dereferencing ioremap'd memory is not permitted.  That includes passing
> > it to memcpy.  Even with a cast.
> 
> So that means that I should use memcpy_fromio() even if the code if far
> less optimized.
> 
> Shouldn't I re-implement some kind of IO copying function to deal with
> this IO memory so that I could take advantage of 8 words bursts?

You could improve the IO memcpy/set etc implementations, which are
currently mostly unloved - I think that's a catch-22 which really needs
solving.  They're not efficient because no one has taken the time to use
them, and everyone's avoiding them because they're not very efficient.
So, as no one's using them no one's motivated to improve them.

[RFC PATCH] MTD: atmel_nand: optimize read/write buffer functions

[Nicolas Ferre]

Le 29/06/2011 15:31, Russell King - ARM Linux :
> On Wed, Jun 29, 2011 at 03:09:59PM +0200, Nicolas Ferre wrote:
>> Le 28/06/2011 16:59, Russell King - ARM Linux :
>>> I think you need to read Documentation/bus-virt-phys-mapping.txt,
>>> particularly the part after "NOTE NOTE NOTE".
>>>
>>> Dereferencing ioremap'd memory is not permitted.  That includes passing
>>> it to memcpy.  Even with a cast.
>>
>> So that means that I should use memcpy_fromio() even if the code if far
>> less optimized.
>>
>> Shouldn't I re-implement some kind of IO copying function to deal with
>> this IO memory so that I could take advantage of 8 words bursts?
> 
> You could improve the IO memcpy/set etc implementations, which are
> currently mostly unloved - I think that's a catch-22 which really needs
> solving.  They're not efficient because no one has taken the time to use
> them, and everyone's avoiding them because they're not very efficient.
> So, as no one's using them no one's motivated to improve them.

Ok, so I use them in my following patch.

And as a long-term exercise, I will try to have a look at those IO
memcpy/set functions for ARM...

Thanks for you advices, best regards,
-- 
Nicolas Ferre

[PATCH V2] MTD: atmel_nand: optimize read/write buffer functions

[Nicolas Ferre]

For PIO NAND access functions, we use the features of the SMC:
- no need to take into account the NAND bus width: SMC will deal with this
- use of an IO memcpy on the NAND chip-select space is able to generate
  proper SMC behavior.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
V2: use of memcpy_fromio/memcpy_toio for proper handling of memory types.

 drivers/mtd/nand/atmel_nand.c |   45 +++-------------------------------------
 1 files changed, 4 insertions(+), 41 deletions(-)

diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index bbdf062..37b2f70 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -160,37 +160,6 @@ static int atmel_nand_device_ready(struct mtd_info *mtd)
                 !!host->board->rdy_pin_active_low;
 }
 
-/*
- * Minimal-overhead PIO for data access.
- */
-static void atmel_read_buf8(struct mtd_info *mtd, u8 *buf, int len)
-{
-	struct nand_chip	*nand_chip = mtd->priv;
-
-	__raw_readsb(nand_chip->IO_ADDR_R, buf, len);
-}
-
-static void atmel_read_buf16(struct mtd_info *mtd, u8 *buf, int len)
-{
-	struct nand_chip	*nand_chip = mtd->priv;
-
-	__raw_readsw(nand_chip->IO_ADDR_R, buf, len / 2);
-}
-
-static void atmel_write_buf8(struct mtd_info *mtd, const u8 *buf, int len)
-{
-	struct nand_chip	*nand_chip = mtd->priv;
-
-	__raw_writesb(nand_chip->IO_ADDR_W, buf, len);
-}
-
-static void atmel_write_buf16(struct mtd_info *mtd, const u8 *buf, int len)
-{
-	struct nand_chip	*nand_chip = mtd->priv;
-
-	__raw_writesw(nand_chip->IO_ADDR_W, buf, len / 2);
-}
-
 static void dma_complete_func(void *completion)
 {
 	complete(completion);
@@ -265,33 +234,27 @@ err_buf:
 static void atmel_read_buf(struct mtd_info *mtd, u8 *buf, int len)
 {
 	struct nand_chip *chip = mtd->priv;
-	struct atmel_nand_host *host = chip->priv;
 
 	if (use_dma && len > mtd->oobsize)
 		/* only use DMA for bigger than oob size: better performances */
 		if (atmel_nand_dma_op(mtd, buf, len, 1) == 0)
 			return;
 
-	if (host->board->bus_width_16)
-		atmel_read_buf16(mtd, buf, len);
-	else
-		atmel_read_buf8(mtd, buf, len);
+	/* if no DMA operation possible, use PIO */
+	memcpy_fromio(buf, chip->IO_ADDR_R, len);
 }
 
 static void atmel_write_buf(struct mtd_info *mtd, const u8 *buf, int len)
 {
 	struct nand_chip *chip = mtd->priv;
-	struct atmel_nand_host *host = chip->priv;
 
 	if (use_dma && len > mtd->oobsize)
 		/* only use DMA for bigger than oob size: better performances */
 		if (atmel_nand_dma_op(mtd, (void *)buf, len, 0) == 0)
 			return;
 
-	if (host->board->bus_width_16)
-		atmel_write_buf16(mtd, buf, len);
-	else
-		atmel_write_buf8(mtd, buf, len);
+	/* if no DMA operation possible, use PIO */
+	memcpy_toio(chip->IO_ADDR_W, buf, len);
 }
 
 /*
-- 
1.7.3

[PATCH V2] MTD: atmel_nand: optimize read/write buffer functions

[Artem Bityutskiy]

On Mon, 2011-07-04 at 16:17 +0200, Nicolas Ferre wrote:
> For PIO NAND access functions, we use the features of the SMC:
> - no need to take into account the NAND bus width: SMC will deal with this
> - use of an IO memcpy on the NAND chip-select space is able to generate
>   proper SMC behavior.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> V2: use of memcpy_fromio/memcpy_toio for proper handling of memory types.

Pushed to l2-mtd-2.6.git, thanks!

-- 
Best Regards,
Artem Bityutskiy