spi_bfin: wait for tx to complete on some cs_chg paths

PATCH In review David Brownell 3 participants 5 messages Jan 31, 2008 – Feb 2, 2008 3 messages not in the archive SPI

lore

Share & tools

lore

David Brownell

author

2008-01-31 20:19 UTC #0 lore raw

PATCH 3/6 spi_bfin: wait for tx to complete on some cs_chg paths

From: Bryan Wu <[email protected]>

PBX 2 SPI devices need the nonstandard "cs change per word" mechanism.

This patch is one of three updating this driver to make the last data bits get sent before advancing the transfer ... in this case, before the chipselect gets deactivated.

Signed-off-by Bryan Wu <[email protected]>

Signed-off-by David Brownell <[email protected]>

---
 drivers/spi/spi_bfin5xx.c |   40 ++++++++++++----------------------------
 1 file changed, 12 insertions(+), 28 deletions(-)

drivers/spi/spi_bfin5xx.c +12 ~~-28~~

@@ -294,16 +294,14 @@ static void u8_cs_chg_writer(struct driv
 {
 	struct chip_data *chip = drv_data->cur_chip;
 
-	/* poll for SPI completion before start */
-	while (!(read_STAT(drv_data) & BIT_STAT_SPIF))
-		cpu_relax();
-
 	while (drv_data->tx < drv_data->tx_end) {
 		cs_active(drv_data, chip);
 
 		write_TDBR(drv_data, (*(u8 *) (drv_data->tx)));
 		while (read_STAT(drv_data) & BIT_STAT_TXS)
 			cpu_relax();
+		while (!(read_STAT(drv_data) & BIT_STAT_SPIF))
+			cpu_relax();
 
 		cs_deactive(drv_data, chip);

@@ -342,31 +340,20 @@ static void u8_cs_chg_reader(struct driv
 {
 	struct chip_data *chip = drv_data->cur_chip;
 
-	/* poll for SPI completion before start */
-	while (!(read_STAT(drv_data) & BIT_STAT_SPIF))
-		cpu_relax();
-
-	/* clear TDBR buffer before read(else it will be shifted out) */
-	write_TDBR(drv_data, 0xFFFF);
+	while (drv_data->rx < drv_data->rx_end) {
+		cs_active(drv_data, chip);
+		read_RDBR(drv_data);	/* kick off */
 
-	cs_active(drv_data, chip);
-	dummy_read(drv_data);
+		while (!(read_STAT(drv_data) & BIT_STAT_RXS))
+			cpu_relax();
+		while (!(read_STAT(drv_data) & BIT_STAT_SPIF))
+			cpu_relax();
 
-	while (drv_data->rx < drv_data->rx_end - 1) {
+		*(u8 *) (drv_data->rx) = read_SHAW(drv_data);
 		cs_deactive(drv_data, chip);
 
-		while (!(read_STAT(drv_data) & BIT_STAT_RXS))
-			cpu_relax();
-		cs_active(drv_data, chip);
-		*(u8 *) (drv_data->rx) = read_RDBR(drv_data);
 		++drv_data->rx;
 	}
-	cs_deactive(drv_data, chip);
-
-	while (!(read_STAT(drv_data) & BIT_STAT_RXS))
-		cpu_relax();
-	*(u8 *) (drv_data->rx) = read_SHAW(drv_data);
-	++drv_data->rx;
 }
 
 static void u8_duplex(struct driver_data *drv_data)

@@ -392,15 +379,12 @@ static void u8_cs_chg_duplex(struct driv
 {
 	struct chip_data *chip = drv_data->cur_chip;
 
-	/* poll for SPI completion before start */
-	while (!(read_STAT(drv_data) & BIT_STAT_SPIF))
-		cpu_relax();
-
 	while (drv_data->rx < drv_data->rx_end) {
 		cs_active(drv_data, chip);
 
 		write_TDBR(drv_data, (*(u8 *) (drv_data->tx)));
-		while (read_STAT(drv_data) & BIT_STAT_TXS)
+
+		while (!(read_STAT(drv_data) & BIT_STAT_SPIF))
 			cpu_relax();
 		while (!(read_STAT(drv_data) & BIT_STAT_RXS))
 			cpu_relax();

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

A message that is not in the archive (sent off-list or not yet received): 200801311219.11238.david-b-yBeKhBN/[email protected]

Andrew Morton

2008-01-31 21:26 UTC #2 lore raw

Review

Quoted 2 lines · On Thu, 31 Jan 2008 12:19:11 -0800 David Brownell…

(quoted diff) +2 -0

+		while (!(read_STAT(drv_data) & BIT_STAT_SPIF))
+			cpu_relax();

I'd suggest that this commonly-occurring code sequence be implemented in a standalone function. That'll probably produce less code and you can also add a timeout+printk+BUG (or whatever) to that function, rather than just mysteriously locking up if something goes wrong.

A message that is not in the archive (sent off-list or not yet received): 20080131132631.91075a19.akpm-de/[email protected]

David Brownell

author

2008-01-31 21:53 UTC #4 lore raw

Quoted 8 lines · Andrew Morton

Review
Quoted 2 lines · On Thu, 31 Jan 2008 12:19:11 -0800 David Brownell…
(quoted diff) +2 -0
+		while (!(read_STAT(drv_data) & BIT_STAT_SPIF))
+			cpu_relax();
I'd suggest that this commonly-occurring code sequence be implemented in a standalone function.

The bitmask to check can be a parameter too, as well as the termination result after the mask. That will allow other loops to get properly limited too -- e.g. wait till RXS or TXS clears, not just SPIF getting set.

Quoted 3 lines

That'll probably produce less code and you can also
add a timeout+printk+BUG (or whatever) to that function, rather than just mysteriously locking up if something goes wrong.

A message that is not in the archive (sent off-list or not yet received): 200801311353.34281.david-b-yBeKhBN/[email protected]

Bryan Wu

2008-02-02 06:20 UTC #6 lore raw

Quoted 10 lines · David Brownell

Quoted 8 lines · Andrew Morton
Review
Quoted 2 lines · On Thu, 31 Jan 2008 12:19:11 -0800 David Brownell…
(quoted diff) +2 -0
+		while (!(read_STAT(drv_data) & BIT_STAT_SPIF))
+			cpu_relax();
I'd suggest that this commonly-occurring code sequence be implemented in a standalone function.

Quoted 5 lines

The bitmask to check can be a parameter too, as well as the termination result after the mask. That will allow other loops to get properly limited too -- e.g. wait till RXS or TXS clears, not just SPIF getting set.

Before SPIF getting set, we do wait for RXS or TXS clears in the code, right?

Quoted 4 lines

Quoted 4 lines
That'll probably produce less code and you can also
add a timeout+printk+BUG (or whatever) to that function, rather than just mysteriously locking up if something goes wrong.

I will pull out these things to a generic standalone function later.

Thanks a lot
-Bryan

David Brownell

author

2008-02-02 06:59 UTC #7 lore raw

Quoted 7 lines · Bryan Wu

Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
+ while (!(read_STAT(drv_data) & BIT_STAT_SPIF)) + cpu_relax();
I'd suggest that this commonly-occurring code sequence be implemented in a standalone function.

Quoted 8 lines

Quoted 5 lines
The bitmask to check can be a parameter too, as well as the termination result after the mask. That will allow other loops to get properly limited too -- e.g. wait till RXS or TXS clears, not just SPIF getting set.
Before SPIF getting set, we do wait for RXS or TXS clears in the code, right?

Sure, but it's the same kind of loop-forever thing. All "loop-forever" primitives should be replaced by ones that have some kind of ceiling, since it's far from unheard-of that hardware glitch.

My observation was that the standalone function can trivially handle "loop-till-set" as well as "loop-till-clear".

- Dave