(no subject)

[Bryan Wu]

[PATCH 01/16] Blackfin SPI Driver: ensure cache coherency before doing DMA

[Bryan Wu]

From: Vitja Makarov <vitja.makarov@gmail.com>

flush or invalidate caches before doing DMA transfer, if needed.

Signed-off-by: Vitja Makarov <vitja.makarov@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/spi/spi_bfin5xx.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index 7fea3cf..e9b7366 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -26,6 +26,10 @@
 #include <asm/portmux.h>
 #include <asm/bfin5xx_spi.h>
 
+/* reserved_mem_dcache_on and cache friends */
+#include <asm/cplbinit.h>
+#include <asm/cacheflush.h>
+
 #define DRV_NAME	"bfin-spi"
 #define DRV_AUTHOR	"Bryan Wu, Luke Yang"
 #define DRV_DESC	"Blackfin BF5xx on-chip SPI Controller Driver"
@@ -795,6 +799,12 @@ static void pump_transfers(unsigned long data)
 			/* set transfer mode, and enable SPI */
 			dev_dbg(&drv_data->pdev->dev, "doing DMA in.\n");
 
+			/* invalidate caches, if needed */
+			if (bfin_addr_dcachable((unsigned long) drv_data->rx))
+				invalidate_dcache_range((unsigned long) drv_data->rx,
+							(unsigned long) (drv_data->rx +
+							drv_data->len));
+
 			/* clear tx reg soformer data is not shifted out */
 			write_TDBR(drv_data, 0xFFFF);
 
@@ -815,6 +825,12 @@ static void pump_transfers(unsigned long data)
 		} else if (drv_data->tx != NULL) {
 			dev_dbg(&drv_data->pdev->dev, "doing DMA out.\n");
 
+			/* flush caches, if needed */
+			if (bfin_addr_dcachable((unsigned long) drv_data->tx))
+				flush_dcache_range((unsigned long) drv_data->tx,
+						(unsigned long) (drv_data->tx +
+						drv_data->len));
+
 			/* start dma */
 			dma_enable_irq(drv_data->dma_channel);
 			dma_config = (RESTART | dma_width | DI_EN);
-- 
1.5.6.3

Re: [PATCH 01/16] Blackfin SPI Driver: ensure cache coherency before doing DMA

[David Brownell]

On Monday 17 November 2008, Bryan Wu wrote:
>                         /* set transfer mode, and enable SPI */
>                         dev_dbg(&drv_data->pdev->dev, "doing DMA in.\n");
>  
> +                       /* invalidate caches, if needed */
> +                       if (bfin_addr_dcachable((unsigned long) drv_data->rx))
> +                               invalidate_dcache_range((unsigned long) drv_data->rx,
> +                                                       (unsigned long) (drv_data->rx +
> +                                                       drv_data->len));

Could you explain why you're not using dma_map_*() calls
or the rx_dma (and tx_dma) addresses the caller may pass?

As a rule, you should use the standard kernel interfaces
for such stuff instead of platform-specific calls like
those two.  There are a LOT more developers who can find
and fix bugs that way.


Also, this patch affects the "not full duplex" branch of
this routine.  DMA here seems unusually convoluted ... but
if you didn't invalidate the cache (RX path) before
flushing it (TX path) and instead did it the other way
aroound, would you actually *need* separate branches?


> +
>                         /* clear tx reg soformer data is not shifted out */
>                         write_TDBR(drv_data, 0xFFFF);
>  

Re: [PATCH 01/16] Blackfin SPI Driver: ensure cache coherency before doing DMA

[Mike Frysinger]

On Thu, Nov 20, 2008 at 15:24, David Brownell wrote:
> On Monday 17 November 2008, Bryan Wu wrote:
>>                         /* set transfer mode, and enable SPI */
>>                         dev_dbg(&drv_data->pdev->dev, "doing DMA in.\n");
>>
>> +                       /* invalidate caches, if needed */
>> +                       if (bfin_addr_dcachable((unsigned long) drv_data->rx))
>> +                               invalidate_dcache_range((unsigned long) drv_data->rx,
>> +                                                       (unsigned long) (drv_data->rx +
>> +                                                       drv_data->len));
>
> Could you explain why you're not using dma_map_*() calls
> or the rx_dma (and tx_dma) addresses the caller may pass?

i'm not familiar with said API.  i worked with what was there already.

> As a rule, you should use the standard kernel interfaces
> for such stuff instead of platform-specific calls like
> those two.  There are a LOT more developers who can find
> and fix bugs that way.

could you elaborate on the common calls that would replace these ?

> Also, this patch affects the "not full duplex" branch of
> this routine.  DMA here seems unusually convoluted ... but
> if you didn't invalidate the cache (RX path) before
> flushing it (TX path) and instead did it the other way
> aroound, would you actually *need* separate branches?

it's convoluted because the hardware sucks.  it cant do full duplex
with DMA.  full duplex only works in the non-DMA case.
-mike

Re: [PATCH 01/16] Blackfin SPI Driver: ensure cache coherency before doing DMA

[David Brownell]

On Thursday 20 November 2008, Mike Frysinger wrote:
> On Thu, Nov 20, 2008 at 15:24, David Brownell wrote:
> > On Monday 17 November 2008, Bryan Wu wrote:
> >>                         /* set transfer mode, and enable SPI */
> >>                         dev_dbg(&drv_data->pdev->dev, "doing DMA in.\n");
> >>
> >> +                       /* invalidate caches, if needed */
> >> +                       if (bfin_addr_dcachable((unsigned long) drv_data->rx))
> >> +                               invalidate_dcache_range((unsigned long) drv_data->rx,
> >> +                                                       (unsigned long) (drv_data->rx +
> >> +                                                       drv_data->len));
> >
> > Could you explain why you're not using dma_map_*() calls
> > or the rx_dma (and tx_dma) addresses the caller may pass?
> 
> i'm not familiar with said API.  i worked with what was there already.

I see.


> > As a rule, you should use the standard kernel interfaces
> > for such stuff instead of platform-specific calls like
> > those two.  There are a LOT more developers who can find
> > and fix bugs that way.
> 
> could you elaborate on the common calls that would replace these ?

See Documentation/DMA-(mapping,API}.txt ... the "mapping"
document's section on what memory may be used for DMA is
not otherwise replicated in the description of the "generic"
versions of those API calls.

Basically, dma_map_single(), dma_unmap_single() ... and
remember that the caller may have done the mappings for
you already.


> > Also, this patch affects the "not full duplex" branch of
> > this routine.  DMA here seems unusually convoluted ... but
> > if you didn't invalidate the cache (RX path) before
> > flushing it (TX path) and instead did it the other way
> > aroound, would you actually *need* separate branches?
> 
> it's convoluted because the hardware sucks.  it cant do full duplex
> with DMA.  full duplex only works in the non-DMA case.

Ah, ok.  Sucky hardware -- been there, done that, still doing.  ;)

It'd be nice if one of patches snuck in a comment on that
point:  "Full duplex only works for non-DMA transfers."
Same rationale:  you may know this hardware inside out,
but the next person won't.

- Dave



> -mike
> 
> 

Re: [PATCH 01/16] Blackfin SPI Driver: ensure cache coherency before doing DMA

[Mike Frysinger]

On Thu, Nov 20, 2008 at 16:47, David Brownell wrote:
> On Thursday 20 November 2008, Mike Frysinger wrote:
>> On Thu, Nov 20, 2008 at 15:24, David Brownell wrote:
>> > On Monday 17 November 2008, Bryan Wu wrote:
>> >>                         /* set transfer mode, and enable SPI */
>> >>                         dev_dbg(&drv_data->pdev->dev, "doing DMA in.\n");
>> >>
>> >> +                       /* invalidate caches, if needed */
>> >> +                       if (bfin_addr_dcachable((unsigned long) drv_data->rx))
>> >> +                               invalidate_dcache_range((unsigned long) drv_data->rx,
>> >> +                                                       (unsigned long) (drv_data->rx +
>> >> +                                                       drv_data->len));
>> >
>> > As a rule, you should use the standard kernel interfaces
>> > for such stuff instead of platform-specific calls like
>> > those two.  There are a LOT more developers who can find
>> > and fix bugs that way.
>>
>> could you elaborate on the common calls that would replace these ?
>
> See Documentation/DMA-(mapping,API}.txt ... the "mapping"
> document's section on what memory may be used for DMA is
> not otherwise replicated in the description of the "generic"
> versions of those API calls.
>
> Basically, dma_map_single(), dma_unmap_single() ... and
> remember that the caller may have done the mappings for
> you already.

these arent required to provide coherent memory right ?  if that's the
case, i can take a look at getting things updated.

>> > Also, this patch affects the "not full duplex" branch of
>> > this routine.  DMA here seems unusually convoluted ... but
>> > if you didn't invalidate the cache (RX path) before
>> > flushing it (TX path) and instead did it the other way
>> > aroound, would you actually *need* separate branches?
>>
>> it's convoluted because the hardware sucks.  it cant do full duplex
>> with DMA.  full duplex only works in the non-DMA case.
>
> Ah, ok.  Sucky hardware -- been there, done that, still doing.  ;)
>
> It'd be nice if one of patches snuck in a comment on that
> point:  "Full duplex only works for non-DMA transfers."
> Same rationale:  you may know this hardware inside out,
> but the next person won't.

well, hopefully if they dont know they wont be touching the driver ;).
 i'll add a comment in this code chunk.  thanks for the feedback.
-mike

Re: [PATCH 01/16] Blackfin SPI Driver: ensure cache coherency before doing DMA

[David Brownell]

On Thursday 20 November 2008, Mike Frysinger wrote:
> On Thu, Nov 20, 2008 at 16:47, David Brownell wrote:

> > Basically, dma_map_single(), dma_unmap_single() ... and
> > remember that the caller may have done the mappings for
> > you already.
> 
> these arent required to provide coherent memory right ?  if that's the
> case, i can take a look at getting things updated.

Right.  If caller provides dma-coherent memory to you, they
must also have provided the DMA addresses the controller
driver should be using ... so you won't need dma mapping
calls on those paths.


> > It'd be nice if one of patches snuck in a comment on that
> > point:  "Full duplex only works for non-DMA transfers."
> > Same rationale:  you may know this hardware inside out,
> > but the next person won't.
> 
> well, hopefully if they dont know they wont be touching the driver ;).
>  i'll add a comment in this code chunk.  thanks for the feedback.

The way it usually works is someone observes a problem and
then starts experimenting on relevant code.  So they learn
a bit while debugging ... and code authors/maintainers need
to keep that learning curve from being too steep.  ;)

- Dave

Re: [PATCH 01/16] Blackfin SPI Driver: ensure cache coherency before doing DMA

[Bryan Wu]

On Fri, Nov 21, 2008 at 6:05 AM, David Brownell <david-b@pacbell.net> wrote:
> On Thursday 20 November 2008, Mike Frysinger wrote:
>> On Thu, Nov 20, 2008 at 16:47, David Brownell wrote:
>
>> > Basically, dma_map_single(), dma_unmap_single() ... and
>> > remember that the caller may have done the mappings for
>> > you already.
>>
>> these arent required to provide coherent memory right ?  if that's the
>> case, i can take a look at getting things updated.
>
> Right.  If caller provides dma-coherent memory to you, they
> must also have provided the DMA addresses the controller
> driver should be using ... so you won't need dma mapping
> calls on those paths.
>
>
>> > It'd be nice if one of patches snuck in a comment on that
>> > point:  "Full duplex only works for non-DMA transfers."
>> > Same rationale:  you may know this hardware inside out,
>> > but the next person won't.
>>
>> well, hopefully if they dont know they wont be touching the driver ;).
>>  i'll add a comment in this code chunk.  thanks for the feedback.
>
> The way it usually works is someone observes a problem and
> then starts experimenting on relevant code.  So they learn
> a bit while debugging ... and code authors/maintainers need
> to keep that learning curve from being too steep.  ;)
>
> - Dave
>
>

Hi Dave,

As you acked this patchset, is that possible to merge it?
Or need Andrew to keep them in -mm for a while.

Thanks
-Bryan

Re: [PATCH 01/16] Blackfin SPI Driver: ensure cache coherency before doing DMA

[David Brownell]

On Thursday 05 February 2009, Bryan Wu wrote:
> As you acked this patchset, is that possible to merge it?
> Or need Andrew to keep them in -mm for a while.

ISTR *NOT* acking the whole series though.  In fact, I recall
giving a generic NAK part way through because of patches that
didn't include comments (after manually NAKing many individual
patches) and ones where $SUBJECT didn't match the patch comments.
Plus even a few which seemed to be wrong.

(And I've said the same thing about Blackfin patches before,
too.  Please help the message get through to your co-workers:
make sure the patches come with useful public descriptions.)

At this point, best to resend the whole series, with the
updates I requested.  The few patches which I did ack can
include that ack.

- Dave

Re: [PATCH 01/16] Blackfin SPI Driver: ensure cache coherency before doing DMA

[Bryan Wu]

On Fri, Feb 6, 2009 at 7:51 AM, David Brownell <david-b@pacbell.net> wrote:
> On Thursday 05 February 2009, Bryan Wu wrote:
>> As you acked this patchset, is that possible to merge it?
>> Or need Andrew to keep them in -mm for a while.
>
> ISTR *NOT* acking the whole series though.  In fact, I recall
> giving a generic NAK part way through because of patches that
> didn't include comments (after manually NAKing many individual
> patches) and ones where $SUBJECT didn't match the patch comments.
> Plus even a few which seemed to be wrong.
>
> (And I've said the same thing about Blackfin patches before,
> too.  Please help the message get through to your co-workers:
> make sure the patches come with useful public descriptions.)
>
> At this point, best to resend the whole series, with the
> updates I requested.  The few patches which I did ack can
> include that ack.
>

OK, I will resend these 16 patches and next new 5 patches soon.
Thanks a lot

-Bryan

[PATCH 02/16] Blackfin SPI Driver: Fix erroneous SPI Clock divisor calculation

[Bryan Wu]

From: Michael Hennerich <michael.hennerich@analog.com>

Fix erroneous SPI Clock divisor calculation. Make sure SPI_BAUD is
always >= 2. Writing a value of 0 or 1 to the SPI_BAUD register disables
the serial clock.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 arch/blackfin/include/asm/bfin5xx_spi.h |    2 ++
 drivers/spi/spi_bfin5xx.c               |    3 +++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/blackfin/include/asm/bfin5xx_spi.h b/arch/blackfin/include/asm/bfin5xx_spi.h
index 9fa1915..b16c3cd 100644
--- a/arch/blackfin/include/asm/bfin5xx_spi.h
+++ b/arch/blackfin/include/asm/bfin5xx_spi.h
@@ -21,6 +21,8 @@
 #ifndef _SPI_CHANNEL_H_
 #define _SPI_CHANNEL_H_
 
+#define MIN_SPI_BAUD_VAL	2
+
 #define SPI_READ              0
 #define SPI_WRITE             1
 
diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index e9b7366..a38106a 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -158,6 +158,9 @@ static u16 hz_to_spi_baud(u32 speed_hz)
 	if ((sclk % (2 * speed_hz)) > 0)
 		spi_baud++;
 
+	if (spi_baud < MIN_SPI_BAUD_VAL)
+		spi_baud = MIN_SPI_BAUD_VAL;
+
 	return spi_baud;
 }
 
-- 
1.5.6.3

Re: [PATCH 02/16] Blackfin SPI Driver: Fix erroneous SPI Clock divisor calculation

[David Brownell]

On Monday 17 November 2008, Bryan Wu wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> Fix erroneous SPI Clock divisor calculation. Make sure SPI_BAUD is
> always >= 2. Writing a value of 0 or 1 to the SPI_BAUD register disables
> the serial clock.
> 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>

> ---
>  arch/blackfin/include/asm/bfin5xx_spi.h |    2 ++
>  drivers/spi/spi_bfin5xx.c               |    3 +++
>  2 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/blackfin/include/asm/bfin5xx_spi.h b/arch/blackfin/include/asm/bfin5xx_spi.h
> index 9fa1915..b16c3cd 100644
> --- a/arch/blackfin/include/asm/bfin5xx_spi.h
> +++ b/arch/blackfin/include/asm/bfin5xx_spi.h
> @@ -21,6 +21,8 @@
>  #ifndef _SPI_CHANNEL_H_
>  #define _SPI_CHANNEL_H_
>  
> +#define MIN_SPI_BAUD_VAL	2
> +
>  #define SPI_READ              0
>  #define SPI_WRITE             1
>  
> diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
> index e9b7366..a38106a 100644
> --- a/drivers/spi/spi_bfin5xx.c
> +++ b/drivers/spi/spi_bfin5xx.c
> @@ -158,6 +158,9 @@ static u16 hz_to_spi_baud(u32 speed_hz)
>  	if ((sclk % (2 * speed_hz)) > 0)
>  		spi_baud++;
>  
> +	if (spi_baud < MIN_SPI_BAUD_VAL)
> +		spi_baud = MIN_SPI_BAUD_VAL;
> +
>  	return spi_baud;
>  }
>  
> -- 
> 1.5.6.3
> 
> 

[PATCH 03/16] Blackfin SPI Driver: move bfin_addr_dcachable() and friends into the cacheflush header where it belongs

[Bryan Wu]

From: Mike Frysinger <vapier.adi@gmail.com>

Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/spi/spi_bfin5xx.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index a38106a..6c5671e 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -25,9 +25,6 @@
 #include <asm/dma.h>
 #include <asm/portmux.h>
 #include <asm/bfin5xx_spi.h>
-
-/* reserved_mem_dcache_on and cache friends */
-#include <asm/cplbinit.h>
 #include <asm/cacheflush.h>
 
 #define DRV_NAME	"bfin-spi"
-- 
1.5.6.3

Re: [PATCH 03/16] Blackfin SPI Driver: move bfin_addr_dcachable() and friends into the cacheflush header where it belongs

[David Brownell]

On Monday 17 November 2008, Bryan Wu wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>
> 
> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>

... except that you really need to have patch comments,
not just overlong $SUBJECT lines, so please fix that.


> ---
>  drivers/spi/spi_bfin5xx.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
> index a38106a..6c5671e 100644
> --- a/drivers/spi/spi_bfin5xx.c
> +++ b/drivers/spi/spi_bfin5xx.c
> @@ -25,9 +25,6 @@
>  #include <asm/dma.h>
>  #include <asm/portmux.h>
>  #include <asm/bfin5xx_spi.h>
> -
> -/* reserved_mem_dcache_on and cache friends */
> -#include <asm/cplbinit.h>
>  #include <asm/cacheflush.h>
>  
>  #define DRV_NAME	"bfin-spi"
> -- 
> 1.5.6.3
> 
> 

[PATCH 04/16] Blackfin SPI Driver: use len_in_bytes when we care about the number of bytes transferred

[Bryan Wu]

From: Mike Frysinger <vapier.adi@gmail.com>

Use len_in_bytes when we care about the number of bytes transferred
rather than the number of spi transactions. (this value will be the
same for 8bit transfers, but not any other sizes)

Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/spi/spi_bfin5xx.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index 6c5671e..cc415e8 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -803,7 +803,7 @@ static void pump_transfers(unsigned long data)
 			if (bfin_addr_dcachable((unsigned long) drv_data->rx))
 				invalidate_dcache_range((unsigned long) drv_data->rx,
 							(unsigned long) (drv_data->rx +
-							drv_data->len));
+							drv_data->len_in_bytes));
 
 			/* clear tx reg soformer data is not shifted out */
 			write_TDBR(drv_data, 0xFFFF);
@@ -829,7 +829,7 @@ static void pump_transfers(unsigned long data)
 			if (bfin_addr_dcachable((unsigned long) drv_data->tx))
 				flush_dcache_range((unsigned long) drv_data->tx,
 						(unsigned long) (drv_data->tx +
-						drv_data->len));
+						drv_data->len_in_bytes));
 
 			/* start dma */
 			dma_enable_irq(drv_data->dma_channel);
@@ -892,7 +892,7 @@ static void pump_transfers(unsigned long data)
 			message->state = ERROR_STATE;
 		} else {
 			/* Update total byte transfered */
-			message->actual_length += drv_data->len;
+			message->actual_length += drv_data->len_in_bytes;
 
 			/* Move to next transfer of this msg */
 			message->state = next_transfer(drv_data);
-- 
1.5.6.3

Re: [PATCH 04/16] Blackfin SPI Driver: use len_in_bytes when we care about the number of bytes transferred

[David Brownell]

On Monday 17 November 2008, Bryan Wu wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>
> 
> Use len_in_bytes when we care about the number of bytes transferred
> rather than the number of spi transactions. (this value will be the
> same for 8bit transfers, but not any other sizes)
> 
> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>


> ---
>  drivers/spi/spi_bfin5xx.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
> index 6c5671e..cc415e8 100644
> --- a/drivers/spi/spi_bfin5xx.c
> +++ b/drivers/spi/spi_bfin5xx.c
> @@ -803,7 +803,7 @@ static void pump_transfers(unsigned long data)
>  			if (bfin_addr_dcachable((unsigned long) drv_data->rx))
>  				invalidate_dcache_range((unsigned long) drv_data->rx,
>  							(unsigned long) (drv_data->rx +
> -							drv_data->len));
> +							drv_data->len_in_bytes));
>  
>  			/* clear tx reg soformer data is not shifted out */
>  			write_TDBR(drv_data, 0xFFFF);
> @@ -829,7 +829,7 @@ static void pump_transfers(unsigned long data)
>  			if (bfin_addr_dcachable((unsigned long) drv_data->tx))
>  				flush_dcache_range((unsigned long) drv_data->tx,
>  						(unsigned long) (drv_data->tx +
> -						drv_data->len));
> +						drv_data->len_in_bytes));
>  
>  			/* start dma */
>  			dma_enable_irq(drv_data->dma_channel);
> @@ -892,7 +892,7 @@ static void pump_transfers(unsigned long data)
>  			message->state = ERROR_STATE;
>  		} else {
>  			/* Update total byte transfered */
> -			message->actual_length += drv_data->len;
> +			message->actual_length += drv_data->len_in_bytes;
>  
>  			/* Move to next transfer of this msg */
>  			message->state = next_transfer(drv_data);
> -- 
> 1.5.6.3
> 
> 

[PATCH 05/16] Blackfin SPI Driver: pass DMA overflow error to the higher level

[Bryan Wu]

From: Mike Frysinger <vapier.adi@gmail.com>

If the SPI bus registers a receive overflow error,
pass the result back up to the higher levels.

Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/spi/spi_bfin5xx.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index cc415e8..642c402 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -559,6 +559,7 @@ static irqreturn_t dma_irq_handler(int irq, void *dev_id)
 	struct driver_data *drv_data = dev_id;
 	struct chip_data *chip = drv_data->cur_chip;
 	struct spi_message *msg = drv_data->cur_msg;
+	u16 spistat = read_STAT(drv_data);
 
 	dev_dbg(&drv_data->pdev->dev, "in dma_irq_handler\n");
 	clear_dma_irqstat(drv_data->dma_channel);
@@ -582,13 +583,18 @@ static irqreturn_t dma_irq_handler(int irq, void *dev_id)
 	while (!(read_STAT(drv_data) & SPIF))
 		cpu_relax();
 
-	msg->actual_length += drv_data->len_in_bytes;
+	if (spistat & RBSY) {
+		msg->state = ERROR_STATE;
+		dev_err(&drv_data->pdev->dev, "dma receive: fifo/buffer overflow\n");
+	} else {
+		msg->actual_length += drv_data->len_in_bytes;
 
-	if (drv_data->cs_change)
-		cs_deactive(drv_data, chip);
+		if (drv_data->cs_change)
+			cs_deactive(drv_data, chip);
 
-	/* Move to next transfer */
-	msg->state = next_transfer(drv_data);
+		/* Move to next transfer */
+		msg->state = next_transfer(drv_data);
+	}
 
 	/* Schedule transfer tasklet */
 	tasklet_schedule(&drv_data->pump_transfers);
-- 
1.5.6.3

Re: [PATCH 05/16] Blackfin SPI Driver: pass DMA overflow error to the higher level

[David Brownell]

On Monday 17 November 2008, Bryan Wu wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>
> 
> If the SPI bus registers a receive overflow error,
> pass the result back up to the higher levels.
> 
> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>

> ---
>  drivers/spi/spi_bfin5xx.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
> index cc415e8..642c402 100644
> --- a/drivers/spi/spi_bfin5xx.c
> +++ b/drivers/spi/spi_bfin5xx.c
> @@ -559,6 +559,7 @@ static irqreturn_t dma_irq_handler(int irq, void *dev_id)
>  	struct driver_data *drv_data = dev_id;
>  	struct chip_data *chip = drv_data->cur_chip;
>  	struct spi_message *msg = drv_data->cur_msg;
> +	u16 spistat = read_STAT(drv_data);
>  
>  	dev_dbg(&drv_data->pdev->dev, "in dma_irq_handler\n");
>  	clear_dma_irqstat(drv_data->dma_channel);
> @@ -582,13 +583,18 @@ static irqreturn_t dma_irq_handler(int irq, void *dev_id)
>  	while (!(read_STAT(drv_data) & SPIF))
>  		cpu_relax();
>  
> -	msg->actual_length += drv_data->len_in_bytes;
> +	if (spistat & RBSY) {
> +		msg->state = ERROR_STATE;
> +		dev_err(&drv_data->pdev->dev, "dma receive: fifo/buffer overflow\n");
> +	} else {
> +		msg->actual_length += drv_data->len_in_bytes;
>  
> -	if (drv_data->cs_change)
> -		cs_deactive(drv_data, chip);
> +		if (drv_data->cs_change)
> +			cs_deactive(drv_data, chip);
>  
> -	/* Move to next transfer */
> -	msg->state = next_transfer(drv_data);
> +		/* Move to next transfer */
> +		msg->state = next_transfer(drv_data);
> +	}
>  
>  	/* Schedule transfer tasklet */
>  	tasklet_schedule(&drv_data->pump_transfers);
> -- 
> 1.5.6.3
> 
> 

[PATCH 06/16] Blackfin SPI Driver: unify duplicated code in dma read/write paths

[Bryan Wu]

From: Mike Frysinger <vapier.adi@gmail.com>

Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/spi/spi_bfin5xx.c |   47 ++++++++++++++++++++------------------------
 1 files changed, 21 insertions(+), 26 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index 642c402..df424a2 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -755,18 +755,19 @@ static void pump_transfers(unsigned long data)
 	if (!full_duplex && drv_data->cur_chip->enable_dma
 				&& drv_data->len > 6) {
 
+		unsigned long dma_start_addr;
+
 		disable_dma(drv_data->dma_channel);
 		clear_dma_irqstat(drv_data->dma_channel);
 		bfin_spi_disable(drv_data);
 
 		/* config dma channel */
 		dev_dbg(&drv_data->pdev->dev, "doing dma transfer\n");
+		set_dma_x_count(drv_data->dma_channel, drv_data->len);
 		if (width == CFG_SPI_WORDSIZE16) {
-			set_dma_x_count(drv_data->dma_channel, drv_data->len);
 			set_dma_x_modify(drv_data->dma_channel, 2);
 			dma_width = WDSIZE_16;
 		} else {
-			set_dma_x_count(drv_data->dma_channel, drv_data->len);
 			set_dma_x_modify(drv_data->dma_channel, 1);
 			dma_width = WDSIZE_8;
 		}
@@ -801,6 +802,7 @@ static void pump_transfers(unsigned long data)
 		}
 
 		/* In dma mode, rx or tx must be NULL in one transfer */
+		dma_config = (RESTART | dma_width | DI_EN);
 		if (drv_data->rx != NULL) {
 			/* set transfer mode, and enable SPI */
 			dev_dbg(&drv_data->pdev->dev, "doing DMA in.\n");
@@ -814,19 +816,9 @@ static void pump_transfers(unsigned long data)
 			/* clear tx reg soformer data is not shifted out */
 			write_TDBR(drv_data, 0xFFFF);
 
-			set_dma_x_count(drv_data->dma_channel, drv_data->len);
-
-			/* start dma */
-			dma_enable_irq(drv_data->dma_channel);
-			dma_config = (WNR | RESTART | dma_width | DI_EN);
-			set_dma_config(drv_data->dma_channel, dma_config);
-			set_dma_start_addr(drv_data->dma_channel,
-					(unsigned long)drv_data->rx);
-			enable_dma(drv_data->dma_channel);
-
-			/* start SPI transfer */
-			write_CTRL(drv_data,
-				(cr | CFG_SPI_DMAREAD | BIT_CTL_ENABLE));
+			dma_config |= WNR;
+			dma_start_addr = (unsigned long)drv_data->rx;
+			cr |= CFG_SPI_DMAREAD;
 
 		} else if (drv_data->tx != NULL) {
 			dev_dbg(&drv_data->pdev->dev, "doing DMA out.\n");
@@ -837,18 +829,21 @@ static void pump_transfers(unsigned long data)
 						(unsigned long) (drv_data->tx +
 						drv_data->len_in_bytes));
 
-			/* start dma */
-			dma_enable_irq(drv_data->dma_channel);
-			dma_config = (RESTART | dma_width | DI_EN);
-			set_dma_config(drv_data->dma_channel, dma_config);
-			set_dma_start_addr(drv_data->dma_channel,
-					(unsigned long)drv_data->tx);
-			enable_dma(drv_data->dma_channel);
+			dma_start_addr = (unsigned long)drv_data->tx;
+			cr |= CFG_SPI_DMAWRITE;
+
+		} else
+			BUG();
+
+		/* start dma */
+		dma_enable_irq(drv_data->dma_channel);
+		set_dma_config(drv_data->dma_channel, dma_config);
+		set_dma_start_addr(drv_data->dma_channel, dma_start_addr);
+		enable_dma(drv_data->dma_channel);
+
+		/* start SPI transfer */
+		write_CTRL(drv_data, (cr | BIT_CTL_ENABLE));
 
-			/* start SPI transfer */
-			write_CTRL(drv_data,
-				(cr | CFG_SPI_DMAWRITE | BIT_CTL_ENABLE));
-		}
 	} else {
 		/* IO mode write then read */
 		dev_dbg(&drv_data->pdev->dev, "doing IO transfer\n");
-- 
1.5.6.3

Re: [PATCH 06/16] Blackfin SPI Driver: unify duplicated code in dma read/write paths

[David Brownell]

On Monday 17 November 2008, Bryan Wu wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>
> 
> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>

assuming:  this patch grows a real patch comment
instead of only having the long $SUBJECT ...


> ---
>  drivers/spi/spi_bfin5xx.c |   47 ++++++++++++++++++++------------------------
>  1 files changed, 21 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
> index 642c402..df424a2 100644
> --- a/drivers/spi/spi_bfin5xx.c
> +++ b/drivers/spi/spi_bfin5xx.c
> @@ -755,18 +755,19 @@ static void pump_transfers(unsigned long data)
>  	if (!full_duplex && drv_data->cur_chip->enable_dma
>  				&& drv_data->len > 6) {
>  
> +		unsigned long dma_start_addr;
> +
>  		disable_dma(drv_data->dma_channel);
>  		clear_dma_irqstat(drv_data->dma_channel);
>  		bfin_spi_disable(drv_data);
>  
>  		/* config dma channel */
>  		dev_dbg(&drv_data->pdev->dev, "doing dma transfer\n");
> +		set_dma_x_count(drv_data->dma_channel, drv_data->len);
>  		if (width == CFG_SPI_WORDSIZE16) {
> -			set_dma_x_count(drv_data->dma_channel, drv_data->len);
>  			set_dma_x_modify(drv_data->dma_channel, 2);
>  			dma_width = WDSIZE_16;
>  		} else {
> -			set_dma_x_count(drv_data->dma_channel, drv_data->len);
>  			set_dma_x_modify(drv_data->dma_channel, 1);
>  			dma_width = WDSIZE_8;
>  		}
> @@ -801,6 +802,7 @@ static void pump_transfers(unsigned long data)
>  		}
>  
>  		/* In dma mode, rx or tx must be NULL in one transfer */
> +		dma_config = (RESTART | dma_width | DI_EN);
>  		if (drv_data->rx != NULL) {
>  			/* set transfer mode, and enable SPI */
>  			dev_dbg(&drv_data->pdev->dev, "doing DMA in.\n");
> @@ -814,19 +816,9 @@ static void pump_transfers(unsigned long data)
>  			/* clear tx reg soformer data is not shifted out */
>  			write_TDBR(drv_data, 0xFFFF);
>  
> -			set_dma_x_count(drv_data->dma_channel, drv_data->len);
> -
> -			/* start dma */
> -			dma_enable_irq(drv_data->dma_channel);
> -			dma_config = (WNR | RESTART | dma_width | DI_EN);
> -			set_dma_config(drv_data->dma_channel, dma_config);
> -			set_dma_start_addr(drv_data->dma_channel,
> -					(unsigned long)drv_data->rx);
> -			enable_dma(drv_data->dma_channel);
> -
> -			/* start SPI transfer */
> -			write_CTRL(drv_data,
> -				(cr | CFG_SPI_DMAREAD | BIT_CTL_ENABLE));
> +			dma_config |= WNR;
> +			dma_start_addr = (unsigned long)drv_data->rx;
> +			cr |= CFG_SPI_DMAREAD;
>  
>  		} else if (drv_data->tx != NULL) {
>  			dev_dbg(&drv_data->pdev->dev, "doing DMA out.\n");
> @@ -837,18 +829,21 @@ static void pump_transfers(unsigned long data)
>  						(unsigned long) (drv_data->tx +
>  						drv_data->len_in_bytes));
>  
> -			/* start dma */
> -			dma_enable_irq(drv_data->dma_channel);
> -			dma_config = (RESTART | dma_width | DI_EN);
> -			set_dma_config(drv_data->dma_channel, dma_config);
> -			set_dma_start_addr(drv_data->dma_channel,
> -					(unsigned long)drv_data->tx);
> -			enable_dma(drv_data->dma_channel);
> +			dma_start_addr = (unsigned long)drv_data->tx;
> +			cr |= CFG_SPI_DMAWRITE;
> +
> +		} else
> +			BUG();
> +
> +		/* start dma */
> +		dma_enable_irq(drv_data->dma_channel);
> +		set_dma_config(drv_data->dma_channel, dma_config);
> +		set_dma_start_addr(drv_data->dma_channel, dma_start_addr);
> +		enable_dma(drv_data->dma_channel);
> +
> +		/* start SPI transfer */
> +		write_CTRL(drv_data, (cr | BIT_CTL_ENABLE));
>  
> -			/* start SPI transfer */
> -			write_CTRL(drv_data,
> -				(cr | CFG_SPI_DMAWRITE | BIT_CTL_ENABLE));
> -		}
>  	} else {
>  		/* IO mode write then read */
>  		dev_dbg(&drv_data->pdev->dev, "doing IO transfer\n");
> -- 
> 1.5.6.3
> 
> 

[PATCH 07/16] Blackfin SPI Driver: drop bogus cast and touchup dma label

[Bryan Wu]

From: Mike Frysinger <vapier.adi@gmail.com>

Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/spi/spi_bfin5xx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index df424a2..a7c8976 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -1076,13 +1076,13 @@ static int setup(struct spi_device *spi)
 	 */
 	if (chip->enable_dma && !drv_data->dma_requested) {
 		/* register dma irq handler */
-		if (request_dma(drv_data->dma_channel, "BF53x_SPI_DMA") < 0) {
+		if (request_dma(drv_data->dma_channel, "BFIN_SPI_DMA") < 0) {
 			dev_dbg(&spi->dev,
 				"Unable to request BlackFin SPI DMA channel\n");
 			return -ENODEV;
 		}
 		if (set_dma_callback(drv_data->dma_channel,
-			(void *)dma_irq_handler, drv_data) < 0) {
+		    dma_irq_handler, drv_data) < 0) {
 			dev_dbg(&spi->dev, "Unable to set dma callback\n");
 			return -EPERM;
 		}
-- 
1.5.6.3

Re: [PATCH 07/16] Blackfin SPI Driver: drop bogus cast and touchup dma label

[David Brownell]

On Monday 17 November 2008, Bryan Wu wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>
> 
> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>

... again, please to include real patch comments
not just long $SUBJECT lines

> ---
>  drivers/spi/spi_bfin5xx.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
> index df424a2..a7c8976 100644
> --- a/drivers/spi/spi_bfin5xx.c
> +++ b/drivers/spi/spi_bfin5xx.c
> @@ -1076,13 +1076,13 @@ static int setup(struct spi_device *spi)
>  	 */
>  	if (chip->enable_dma && !drv_data->dma_requested) {
>  		/* register dma irq handler */
> -		if (request_dma(drv_data->dma_channel, "BF53x_SPI_DMA") < 0) {
> +		if (request_dma(drv_data->dma_channel, "BFIN_SPI_DMA") < 0) {
>  			dev_dbg(&spi->dev,
>  				"Unable to request BlackFin SPI DMA channel\n");
>  			return -ENODEV;
>  		}
>  		if (set_dma_callback(drv_data->dma_channel,
> -			(void *)dma_irq_handler, drv_data) < 0) {
> +		    dma_irq_handler, drv_data) < 0) {
>  			dev_dbg(&spi->dev, "Unable to set dma callback\n");
>  			return -EPERM;
>  		}
> -- 
> 1.5.6.3
> 
> 

[PATCH 08/16] Blackfin SPI Driver: add a few more debug messages in useful places

[Bryan Wu]

From: Mike Frysinger <vapier.adi@gmail.com>

Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/spi/spi_bfin5xx.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index a7c8976..e293d19 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -559,9 +559,13 @@ static irqreturn_t dma_irq_handler(int irq, void *dev_id)
 	struct driver_data *drv_data = dev_id;
 	struct chip_data *chip = drv_data->cur_chip;
 	struct spi_message *msg = drv_data->cur_msg;
+	unsigned short dmastat = get_dma_curr_irqstat(drv_data->dma_channel);
 	u16 spistat = read_STAT(drv_data);
 
-	dev_dbg(&drv_data->pdev->dev, "in dma_irq_handler\n");
+	dev_dbg(&drv_data->pdev->dev,
+		"in dma_irq_handler dmastat:0x%x spistat:0x%x\n",
+		dmastat, spistat);
+
 	clear_dma_irqstat(drv_data->dma_channel);
 
 	/* Wait for DMA to complete */
@@ -631,6 +635,7 @@ static void pump_transfers(unsigned long data)
 
 	 /* Handle for abort */
 	if (message->state == ERROR_STATE) {
+		dev_dbg(&drv_data->pdev->dev, "transfer: we've hit an error\n");
 		message->status = -EIO;
 		giveback(drv_data);
 		return;
@@ -638,6 +643,7 @@ static void pump_transfers(unsigned long data)
 
 	/* Handle end of message */
 	if (message->state == DONE_STATE) {
+		dev_dbg(&drv_data->pdev->dev, "transfer: all done!\n");
 		message->status = 0;
 		giveback(drv_data);
 		return;
@@ -645,6 +651,7 @@ static void pump_transfers(unsigned long data)
 
 	/* Delay if requested at end of transfer */
 	if (message->state == RUNNING_STATE) {
+		dev_dbg(&drv_data->pdev->dev, "transfer: still running ...\n");
 		previous = list_entry(transfer->transfer_list.prev,
 				      struct spi_transfer, transfer_list);
 		if (previous->delay_usecs)
@@ -805,7 +812,8 @@ static void pump_transfers(unsigned long data)
 		dma_config = (RESTART | dma_width | DI_EN);
 		if (drv_data->rx != NULL) {
 			/* set transfer mode, and enable SPI */
-			dev_dbg(&drv_data->pdev->dev, "doing DMA in.\n");
+			dev_dbg(&drv_data->pdev->dev, "doing DMA in to %p (size %zx)\n",
+				drv_data->rx, drv_data->len_in_bytes);
 
 			/* invalidate caches, if needed */
 			if (bfin_addr_dcachable((unsigned long) drv_data->rx))
-- 
1.5.6.3

Re: [PATCH 08/16] Blackfin SPI Driver: add a few more debug messages in useful places

[David Brownell]

On Monday 17 November 2008, Bryan Wu wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>
> 
> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>

Grr.  I'm tired of acking with a "please provide a real
patch comment".  Consider all the other patches in this
series that don't have patch comments as getting NAKs,
for that reason.


> ---
>  drivers/spi/spi_bfin5xx.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
> index a7c8976..e293d19 100644
> --- a/drivers/spi/spi_bfin5xx.c
> +++ b/drivers/spi/spi_bfin5xx.c
> @@ -559,9 +559,13 @@ static irqreturn_t dma_irq_handler(int irq, void *dev_id)
>  	struct driver_data *drv_data = dev_id;
>  	struct chip_data *chip = drv_data->cur_chip;
>  	struct spi_message *msg = drv_data->cur_msg;
> +	unsigned short dmastat = get_dma_curr_irqstat(drv_data->dma_channel);
>  	u16 spistat = read_STAT(drv_data);
>  
> -	dev_dbg(&drv_data->pdev->dev, "in dma_irq_handler\n");
> +	dev_dbg(&drv_data->pdev->dev,
> +		"in dma_irq_handler dmastat:0x%x spistat:0x%x\n",
> +		dmastat, spistat);
> +
>  	clear_dma_irqstat(drv_data->dma_channel);
>  
>  	/* Wait for DMA to complete */
> @@ -631,6 +635,7 @@ static void pump_transfers(unsigned long data)
>  
>  	 /* Handle for abort */
>  	if (message->state == ERROR_STATE) {
> +		dev_dbg(&drv_data->pdev->dev, "transfer: we've hit an error\n");
>  		message->status = -EIO;
>  		giveback(drv_data);
>  		return;
> @@ -638,6 +643,7 @@ static void pump_transfers(unsigned long data)
>  
>  	/* Handle end of message */
>  	if (message->state == DONE_STATE) {
> +		dev_dbg(&drv_data->pdev->dev, "transfer: all done!\n");
>  		message->status = 0;
>  		giveback(drv_data);
>  		return;
> @@ -645,6 +651,7 @@ static void pump_transfers(unsigned long data)
>  
>  	/* Delay if requested at end of transfer */
>  	if (message->state == RUNNING_STATE) {
> +		dev_dbg(&drv_data->pdev->dev, "transfer: still running ...\n");
>  		previous = list_entry(transfer->transfer_list.prev,
>  				      struct spi_transfer, transfer_list);
>  		if (previous->delay_usecs)
> @@ -805,7 +812,8 @@ static void pump_transfers(unsigned long data)
>  		dma_config = (RESTART | dma_width | DI_EN);
>  		if (drv_data->rx != NULL) {
>  			/* set transfer mode, and enable SPI */
> -			dev_dbg(&drv_data->pdev->dev, "doing DMA in.\n");
> +			dev_dbg(&drv_data->pdev->dev, "doing DMA in to %p (size %zx)\n",
> +				drv_data->rx, drv_data->len_in_bytes);
>  
>  			/* invalidate caches, if needed */
>  			if (bfin_addr_dcachable((unsigned long) drv_data->rx))
> -- 
> 1.5.6.3
> 
> 

[PATCH 09/16] Blackfin SPI Driver: do not check for SPI errors if DMA itself did not flag any

[Bryan Wu]

From: Mike Frysinger <vapier.adi@gmail.com>

Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/spi/spi_bfin5xx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index e293d19..3f4a603 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -587,7 +587,7 @@ static irqreturn_t dma_irq_handler(int irq, void *dev_id)
 	while (!(read_STAT(drv_data) & SPIF))
 		cpu_relax();
 
-	if (spistat & RBSY) {
+	if ((dmastat & DMA_ERR) && (spistat & RBSY)) {
 		msg->state = ERROR_STATE;
 		dev_err(&drv_data->pdev->dev, "dma receive: fifo/buffer overflow\n");
 	} else {
-- 
1.5.6.3

[PATCH 10/16] Blackfin SPI Driver: use the properl BIT_CTL_xxx defines

[Bryan Wu]

From: Mike Frysinger <vapier.adi@gmail.com>

use the properl BIT_CTL_... defines rather than the internal driv
er CFG_SPI_... defines

Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/spi/spi_bfin5xx.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index 3f4a603..4343a92 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -798,7 +798,7 @@ static void pump_transfers(unsigned long data)
 
 			/* start SPI transfer */
 			write_CTRL(drv_data,
-				(cr | CFG_SPI_DMAWRITE | BIT_CTL_ENABLE));
+				(cr | BIT_CTL_TIMOD_DMA_TX | BIT_CTL_ENABLE));
 
 			/* just return here, there can only be one transfer
 			 * in this mode
@@ -821,12 +821,9 @@ static void pump_transfers(unsigned long data)
 							(unsigned long) (drv_data->rx +
 							drv_data->len_in_bytes));
 
-			/* clear tx reg soformer data is not shifted out */
-			write_TDBR(drv_data, 0xFFFF);
-
 			dma_config |= WNR;
 			dma_start_addr = (unsigned long)drv_data->rx;
-			cr |= CFG_SPI_DMAREAD;
+			cr |= BIT_CTL_TIMOD_DMA_RX | BIT_CTL_SENDOPT;
 
 		} else if (drv_data->tx != NULL) {
 			dev_dbg(&drv_data->pdev->dev, "doing DMA out.\n");
@@ -838,7 +835,7 @@ static void pump_transfers(unsigned long data)
 						drv_data->len_in_bytes));
 
 			dma_start_addr = (unsigned long)drv_data->tx;
-			cr |= CFG_SPI_DMAWRITE;
+			cr |= BIT_CTL_TIMOD_DMA_TX;
 
 		} else
 			BUG();
-- 
1.5.6.3

Re: [PATCH 10/16] Blackfin SPI Driver: use the properl BIT_CTL_xxx defines

[David Brownell]

On Monday 17 November 2008, Bryan Wu wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>
> 
> use the properl BIT_CTL_... defines rather than the internal driv
> er CFG_SPI_... defines
> 

> @@ -821,12 +821,9 @@ static void pump_transfers(unsigned long data)
>  							(unsigned long) (drv_data->rx +
>  							drv_data->len_in_bytes));
>  
> -			/* clear tx reg soformer data is not shifted out */
> -			write_TDBR(drv_data, 0xFFFF);
> -

This is unrelated to a BIT_CTL #define.  It's a bug in
either the patch or its comment.


>  			dma_config |= WNR;
>  			dma_start_addr = (unsigned long)drv_data->rx;

[PATCH 11/16] Blackfin SPI Driver: remove duplicated MAX_SPI_SSEL and remove unnecessary array size

[Bryan Wu]

From: Mike Frysinger <vapier.adi@gmail.com>

Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/spi/spi_bfin5xx.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index 4343a92..fba77c2 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -198,8 +198,6 @@ static void cs_deactive(struct driver_data *drv_data, struct chip_data *chip)
 		udelay(chip->cs_chg_udelay);
 }
 
-#define MAX_SPI_SSEL	7
-
 /* stop controller and re-config current chip*/
 static void restore_state(struct driver_data *drv_data)
 {
@@ -997,7 +995,7 @@ static int transfer(struct spi_device *spi, struct spi_message *msg)
 
 #define MAX_SPI_SSEL	7
 
-static u16 ssel[3][MAX_SPI_SSEL] = {
+static u16 ssel[][MAX_SPI_SSEL] = {
 	{P_SPI0_SSEL1, P_SPI0_SSEL2, P_SPI0_SSEL3,
 	P_SPI0_SSEL4, P_SPI0_SSEL5,
 	P_SPI0_SSEL6, P_SPI0_SSEL7},
-- 
1.5.6.3

[PATCH 12/16] Blackfin SPI Driver: get dma working for SPI flashes

[Bryan Wu]

From: Mike Frysinger <vapier.adi@gmail.com>

When using a BF533-STAMP here with a W25X10 SPI flash. It works fine when
enable_dma is disabled, but doesn't work at all when turning DMA on.
We get just 0xff bytes back when trying to read the device.

If change the code around so that it programs the SPI first and then
enables DMA, it seems to work a lot better ...

Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/spi/spi_bfin5xx.c |   26 ++++++++++++++++----------
 1 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index fba77c2..5cd4873 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -760,11 +760,10 @@ static void pump_transfers(unsigned long data)
 	if (!full_duplex && drv_data->cur_chip->enable_dma
 				&& drv_data->len > 6) {
 
-		unsigned long dma_start_addr;
+		unsigned long dma_start_addr, flags;
 
 		disable_dma(drv_data->dma_channel);
 		clear_dma_irqstat(drv_data->dma_channel);
-		bfin_spi_disable(drv_data);
 
 		/* config dma channel */
 		dev_dbg(&drv_data->pdev->dev, "doing dma transfer\n");
@@ -795,8 +794,7 @@ static void pump_transfers(unsigned long data)
 			enable_dma(drv_data->dma_channel);
 
 			/* start SPI transfer */
-			write_CTRL(drv_data,
-				(cr | BIT_CTL_TIMOD_DMA_TX | BIT_CTL_ENABLE));
+			write_CTRL(drv_data, cr | BIT_CTL_TIMOD_DMA_TX);
 
 			/* just return here, there can only be one transfer
 			 * in this mode
@@ -838,14 +836,22 @@ static void pump_transfers(unsigned long data)
 		} else
 			BUG();
 
-		/* start dma */
-		dma_enable_irq(drv_data->dma_channel);
-		set_dma_config(drv_data->dma_channel, dma_config);
+		/* oh man, here there be monsters ... and i dont mean the
+		 * fluffy cute ones from pixar, i mean the kind that'll eat
+		 * your data, kick your dog, and love it all.  do *not* try
+		 * and change these lines unless you (1) heavily test DMA
+		 * with SPI flashes on a loaded system (e.g. ping floods),
+		 * (2) know just how broken the DMA engine interaction with
+		 * the SPI peripheral is, and (3) have someone else to blame
+		 * when you screw it all up anyways.
+		 */
 		set_dma_start_addr(drv_data->dma_channel, dma_start_addr);
+		set_dma_config(drv_data->dma_channel, dma_config);
+		local_irq_save(flags);
 		enable_dma(drv_data->dma_channel);
-
-		/* start SPI transfer */
-		write_CTRL(drv_data, (cr | BIT_CTL_ENABLE));
+		write_CTRL(drv_data, cr);
+		dma_enable_irq(drv_data->dma_channel);
+		local_irq_restore(flags);
 
 	} else {
 		/* IO mode write then read */
-- 
1.5.6.3

Re: [PATCH 12/16] Blackfin SPI Driver: get dma working for SPI flashes

[David Brownell]

On Monday 17 November 2008, Bryan Wu wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>
> 
> When using a BF533-STAMP here with a W25X10 SPI flash. It works fine when
> enable_dma is disabled, but doesn't work at all when turning DMA on.
> We get just 0xff bytes back when trying to read the device.
> 
> If change the code around so that it programs the SPI first and then
> enables DMA, it seems to work a lot better ...
> 
> Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>

... and I observe some MM patches from you related to bugs
identifying those chips; good to know I wasn't the only
one to trip over those regressions!


> ---
>  drivers/spi/spi_bfin5xx.c |   26 ++++++++++++++++----------
>  1 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
> index fba77c2..5cd4873 100644
> --- a/drivers/spi/spi_bfin5xx.c
> +++ b/drivers/spi/spi_bfin5xx.c
> @@ -760,11 +760,10 @@ static void pump_transfers(unsigned long data)
>  	if (!full_duplex && drv_data->cur_chip->enable_dma
>  				&& drv_data->len > 6) {
>  
> -		unsigned long dma_start_addr;
> +		unsigned long dma_start_addr, flags;
>  
>  		disable_dma(drv_data->dma_channel);
>  		clear_dma_irqstat(drv_data->dma_channel);
> -		bfin_spi_disable(drv_data);
>  
>  		/* config dma channel */
>  		dev_dbg(&drv_data->pdev->dev, "doing dma transfer\n");
> @@ -795,8 +794,7 @@ static void pump_transfers(unsigned long data)
>  			enable_dma(drv_data->dma_channel);
>  
>  			/* start SPI transfer */
> -			write_CTRL(drv_data,
> -				(cr | BIT_CTL_TIMOD_DMA_TX | BIT_CTL_ENABLE));
> +			write_CTRL(drv_data, cr | BIT_CTL_TIMOD_DMA_TX);
>  
>  			/* just return here, there can only be one transfer
>  			 * in this mode
> @@ -838,14 +836,22 @@ static void pump_transfers(unsigned long data)
>  		} else
>  			BUG();
>  
> -		/* start dma */
> -		dma_enable_irq(drv_data->dma_channel);
> -		set_dma_config(drv_data->dma_channel, dma_config);
> +		/* oh man, here there be monsters ... and i dont mean the
> +		 * fluffy cute ones from pixar, i mean the kind that'll eat
> +		 * your data, kick your dog, and love it all.  do *not* try
> +		 * and change these lines unless you (1) heavily test DMA
> +		 * with SPI flashes on a loaded system (e.g. ping floods),
> +		 * (2) know just how broken the DMA engine interaction with
> +		 * the SPI peripheral is, and (3) have someone else to blame
> +		 * when you screw it all up anyways.
> +		 */
>  		set_dma_start_addr(drv_data->dma_channel, dma_start_addr);
> +		set_dma_config(drv_data->dma_channel, dma_config);
> +		local_irq_save(flags);
>  		enable_dma(drv_data->dma_channel);
> -
> -		/* start SPI transfer */
> -		write_CTRL(drv_data, (cr | BIT_CTL_ENABLE));
> +		write_CTRL(drv_data, cr);
> +		dma_enable_irq(drv_data->dma_channel);
> +		local_irq_restore(flags);
>  
>  	} else {
>  		/* IO mode write then read */
> -- 
> 1.5.6.3
> 
> 

[PATCH 13/16] Blackfin SPI Driver: add timeout while waiting for SPIF in dma irq handler

[Bryan Wu]

From: Mike Frysinger <vapier.adi@gmail.com>

Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/spi/spi_bfin5xx.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index 5cd4873..c2fc3a9 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -557,6 +557,7 @@ static irqreturn_t dma_irq_handler(int irq, void *dev_id)
 	struct driver_data *drv_data = dev_id;
 	struct chip_data *chip = drv_data->cur_chip;
 	struct spi_message *msg = drv_data->cur_msg;
+	unsigned long timeout;
 	unsigned short dmastat = get_dma_curr_irqstat(drv_data->dma_channel);
 	u16 spistat = read_STAT(drv_data);
 
@@ -582,8 +583,17 @@ static irqreturn_t dma_irq_handler(int irq, void *dev_id)
 			cpu_relax();
 	}
 
+	dev_dbg(&drv_data->pdev->dev,
+		"in dma_irq_handler dmastat:0x%x spistat:0x%x\n",
+		dmastat, read_STAT(drv_data));
+
+	timeout = jiffies + HZ;
 	while (!(read_STAT(drv_data) & SPIF))
-		cpu_relax();
+		if (!time_before(jiffies, timeout)) {
+			dev_warn(&drv_data->pdev->dev, "timeout waiting for SPIF");
+			break;
+		} else
+			cpu_relax();
 
 	if ((dmastat & DMA_ERR) && (spistat & RBSY)) {
 		msg->state = ERROR_STATE;
-- 
1.5.6.3

[PATCH 14/16] Blackfin SPI Driver: tweak magic spi dma sequence to get it working on BF54x

[Bryan Wu]

From: Mike Frysinger <vapier.adi@gmail.com>

Signed-off-by: Mike Frysinger <vapier.adi@gmail.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/spi/spi_bfin5xx.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index c2fc3a9..9c602ad 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -858,8 +858,9 @@ static void pump_transfers(unsigned long data)
 		set_dma_start_addr(drv_data->dma_channel, dma_start_addr);
 		set_dma_config(drv_data->dma_channel, dma_config);
 		local_irq_save(flags);
-		enable_dma(drv_data->dma_channel);
+		SSYNC();
 		write_CTRL(drv_data, cr);
+		enable_dma(drv_data->dma_channel);
 		dma_enable_irq(drv_data->dma_channel);
 		local_irq_restore(flags);
 
-- 
1.5.6.3

[PATCH 15/16] Blackfin SPI Driver: fix bug - spi controller driver does not assert/deassert CS correctly

[Bryan Wu]

From: Yi Li <yi.li@analog.com>

This bug can be observed when two SPI devices are sharing the spi bus:
One device is set as SPI CS 7, another one is using SPI CS 4.

In spi_bfin5xx.c: cs_active(), cs_deactive() are used to control SPI_FLG
register. From the debug bellow:

cs_active: flag: 0x7f91, chip->flag: 0x7f80, cs: 7
cs_active: flag: 0xef91, chip->flag: 0xef10, cs: 4

When device A (cs_7) activate CS 7, SPI_FLG is set as 0x7f91 (however,
SPI_FLG should be set as 0x7f80, or 0x6f91 if in broadcast mode).

Due to some HW bug (very possibly), if SPI_FLG is set as 0x7f91,
SPISSEL7 is asserted, however SPISSEL4 will be asserted too (I can see
this using the scope). This is unreasonable according to HRM.

Signed-off-by: Yi Li <yi.li@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/spi/spi_bfin5xx.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index 9c602ad..8cf5d6e 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -189,6 +189,7 @@ static void cs_deactive(struct driver_data *drv_data, struct chip_data *chip)
 {
 	u16 flag = read_FLAG(drv_data);
 
+	flag &= ~chip->flag;
 	flag |= (chip->flag << 8);
 
 	write_FLAG(drv_data, flag);
@@ -1032,7 +1033,6 @@ static int setup(struct spi_device *spi)
 	struct bfin5xx_spi_chip *chip_info = NULL;
 	struct chip_data *chip;
 	struct driver_data *drv_data = spi_master_get_devdata(spi->master);
-	u8 spi_flg;
 
 	/* Abort device setup if requested features are not supported */
 	if (spi->mode & ~(SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST)) {
@@ -1115,8 +1115,7 @@ static int setup(struct spi_device *spi)
 	 * SPI_BAUD, not the real baudrate
 	 */
 	chip->baud = hz_to_spi_baud(spi->max_speed_hz);
-	spi_flg = ~(1 << (spi->chip_select));
-	chip->flag = ((u16) spi_flg << 8) | (1 << (spi->chip_select));
+	chip->flag = 1 << (spi->chip_select);
 	chip->chip_select_num = spi->chip_select;
 
 	switch (chip->bits_per_word) {
-- 
1.5.6.3

Re: [PATCH 15/16] Blackfin SPI Driver: fix bug - spi controller driver does not assert/deassert CS correctly

[David Brownell]

On Monday 17 November 2008, Bryan Wu wrote:
> From: Yi Li <yi.li@analog.com>
> 
> This bug can be observed when two SPI devices are sharing the spi bus:
> One device is set as SPI CS 7, another one is using SPI CS 4.
> 
> In spi_bfin5xx.c: cs_active(), cs_deactive() are used to control SPI_FLG
> register. From the debug bellow:
> 
> cs_active: flag: 0x7f91, chip->flag: 0x7f80, cs: 7
> cs_active: flag: 0xef91, chip->flag: 0xef10, cs: 4
> 
> When device A (cs_7) activate CS 7, SPI_FLG is set as 0x7f91 (however,
> SPI_FLG should be set as 0x7f80, or 0x6f91 if in broadcast mode).
> 
> Due to some HW bug (very possibly), if SPI_FLG is set as 0x7f91,
> SPISSEL7 is asserted, however SPISSEL4 will be asserted too (I can see
> this using the scope). This is unreasonable according to HRM.
> 
> Signed-off-by: Yi Li <yi.li@analog.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>

Yay!  Real patch comments!  Although ... it doesn't exactly
say *how* it was fixed, which would make the comments better.
("Clear flags that were wrongly left set." maybe.)

Saying what was fixed is at least a start.


> ---
>  drivers/spi/spi_bfin5xx.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
> index 9c602ad..8cf5d6e 100644
> --- a/drivers/spi/spi_bfin5xx.c
> +++ b/drivers/spi/spi_bfin5xx.c
> @@ -189,6 +189,7 @@ static void cs_deactive(struct driver_data *drv_data, struct chip_data *chip)
>  {
>  	u16 flag = read_FLAG(drv_data);
>  
> +	flag &= ~chip->flag;
>  	flag |= (chip->flag << 8);
>  
>  	write_FLAG(drv_data, flag);
> @@ -1032,7 +1033,6 @@ static int setup(struct spi_device *spi)
>  	struct bfin5xx_spi_chip *chip_info = NULL;
>  	struct chip_data *chip;
>  	struct driver_data *drv_data = spi_master_get_devdata(spi->master);
> -	u8 spi_flg;
>  
>  	/* Abort device setup if requested features are not supported */
>  	if (spi->mode & ~(SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST)) {
> @@ -1115,8 +1115,7 @@ static int setup(struct spi_device *spi)
>  	 * SPI_BAUD, not the real baudrate
>  	 */
>  	chip->baud = hz_to_spi_baud(spi->max_speed_hz);
> -	spi_flg = ~(1 << (spi->chip_select));
> -	chip->flag = ((u16) spi_flg << 8) | (1 << (spi->chip_select));
> +	chip->flag = 1 << (spi->chip_select);
>  	chip->chip_select_num = spi->chip_select;
>  
>  	switch (chip->bits_per_word) {
> -- 
> 1.5.6.3
> 
> 

[PATCH 16/16] Blackfin SPI Driver: fix bug - correct usage of struct spi_transfer.cs_change

[Bryan Wu]

From: Yi Li <yi.li@analog.com>

According to comments in linux/spi/spi.h:

 * All SPI transfers start with the relevant chipselect active.  Normally
 * it stays selected until after the last transfer in a message. Drivers
 * can affect the chipselect signal using cs_change.
 *
 * (i) If the transfer isn't the last one in the message, this flag is
 * used to make the chipselect briefly go inactive in the middle of the
 * message.  Toggling chipselect in this way may be needed to terminate
 * a chip command, letting a single spi_message perform all of group of
 * chip transactions together.
 *
 * (ii) When the transfer is the last one in the message, the chip may
 * stay selected until the next transfer.  On multi-device SPI busses
 * with nothing blocking messages going to other devices, this is just
 * a performance hint; starting a message to another device deselects
 * this one.  But in other cases, this can be used to ensure correctness.
 * Some devices need protocol transactions to be built from a series of
 * spi_message submissions, where the content of one message is determined
 * by the results of previous messages and where the whole transaction
 * ends when the chipselect goes intactive.

Signed-off-by: Yi Li <yi.li@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 drivers/spi/spi_bfin5xx.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index 8cf5d6e..0e3102a 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -540,15 +540,13 @@ static void giveback(struct driver_data *drv_data)
 
 	msg->state = NULL;
 
-	/* disable chip select signal. And not stop spi in autobuffer mode */
-	if (drv_data->tx_dma != 0xFFFF) {
-		cs_deactive(drv_data, chip);
-		bfin_spi_disable(drv_data);
-	}
-
 	if (!drv_data->cs_change)
 		cs_deactive(drv_data, chip);
 
+	/* Not stop spi in autobuffer mode */
+	if (drv_data->tx_dma != 0xFFFF)
+		bfin_spi_disable(drv_data);
+
 	if (msg->complete)
 		msg->complete(msg->context);
 }
@@ -757,7 +755,8 @@ static void pump_transfers(unsigned long data)
 
 	write_STAT(drv_data, BIT_STAT_CLR);
 	cr = (read_CTRL(drv_data) & (~BIT_CTL_TIMOD));
-	cs_active(drv_data, chip);
+	if (drv_data->cs_change)
+		cs_active(drv_data, chip);
 
 	dev_dbg(&drv_data->pdev->dev,
 		"now pumping a transfer: width is %d, len is %d\n",
@@ -920,6 +919,8 @@ static void pump_transfers(unsigned long data)
 			message->state = next_transfer(drv_data);
 		}
 
+		if (drv_data->cs_change)
+			cs_active(drv_data, chip);
 		/* Schedule next transfer tasklet */
 		tasklet_schedule(&drv_data->pump_transfers);
 
-- 
1.5.6.3

Re: [PATCH 16/16] Blackfin SPI Driver: fix bug - correct usage of struct spi_transfer.cs_change

[David Brownell]

On Monday 17 November 2008, Bryan Wu wrote:
> From: Yi Li <yi.li@analog.com>
> 
> According to comments in linux/spi/spi.h:
> 
>  * All SPI transfers start with the relevant chipselect active.  Normally
>  * it stays selected until after the last transfer in a message. Drivers
>  * can affect the chipselect signal using cs_change.
>  *
>  * (i) If the transfer isn't the last one in the message, this flag is
>  * used to make the chipselect briefly go inactive in the middle of the
>  * message.  Toggling chipselect in this way may be needed to terminate
>  * a chip command, letting a single spi_message perform all of group of
>  * chip transactions together.
>  *
>  * (ii) When the transfer is the last one in the message, the chip may
>  * stay selected until the next transfer.  On multi-device SPI busses
>  * with nothing blocking messages going to other devices, this is just
>  * a performance hint; starting a message to another device deselects
>  * this one.  But in other cases, this can be used to ensure correctness.
>  * Some devices need protocol transactions to be built from a series of
>  * spi_message submissions, where the content of one message is determined
>  * by the results of previous messages and where the whole transaction
>  * ends when the chipselect goes intactive.
> 
> Signed-off-by: Yi Li <yi.li@analog.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>

> ---
>  drivers/spi/spi_bfin5xx.c |   15 ++++++++-------
>  1 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
> index 8cf5d6e..0e3102a 100644
> --- a/drivers/spi/spi_bfin5xx.c
> +++ b/drivers/spi/spi_bfin5xx.c
> @@ -540,15 +540,13 @@ static void giveback(struct driver_data *drv_data)
>  
>  	msg->state = NULL;
>  
> -	/* disable chip select signal. And not stop spi in autobuffer mode */
> -	if (drv_data->tx_dma != 0xFFFF) {
> -		cs_deactive(drv_data, chip);
> -		bfin_spi_disable(drv_data);
> -	}
> -
>  	if (!drv_data->cs_change)
>  		cs_deactive(drv_data, chip);
>  
> +	/* Not stop spi in autobuffer mode */
> +	if (drv_data->tx_dma != 0xFFFF)
> +		bfin_spi_disable(drv_data);
> +
>  	if (msg->complete)
>  		msg->complete(msg->context);
>  }
> @@ -757,7 +755,8 @@ static void pump_transfers(unsigned long data)
>  
>  	write_STAT(drv_data, BIT_STAT_CLR);
>  	cr = (read_CTRL(drv_data) & (~BIT_CTL_TIMOD));
> -	cs_active(drv_data, chip);
> +	if (drv_data->cs_change)
> +		cs_active(drv_data, chip);
>  
>  	dev_dbg(&drv_data->pdev->dev,
>  		"now pumping a transfer: width is %d, len is %d\n",
> @@ -920,6 +919,8 @@ static void pump_transfers(unsigned long data)
>  			message->state = next_transfer(drv_data);
>  		}
>  
> +		if (drv_data->cs_change)
> +			cs_active(drv_data, chip);
>  		/* Schedule next transfer tasklet */
>  		tasklet_schedule(&drv_data->pump_transfers);
>  
> -- 
> 1.5.6.3
> 
>