spi/omap2_mcspi: disable and enable chan between each SPI transfer

-----Original Message-----
From: ext Jason Wang [mailto:jason77.wang at gmail.com]
Sent: 24 June, 2010 15:13
To: grant.likely at secretlab.ca; Tereshonkov Roman (Nokia-D/Helsinki); david-b at pacbell.net
Cc: spi-devel-general at lists.sourceforge.net; linux-arm-kernel at lists.infradead.org
Subject: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer

In current design, the SPI channel is always enable during the period of handling a SPI message, it is risky when more than one SPI transfer are included in a message. Current working route like that:
[
SPI channel enable
configure channel
handle transfer #1
configure channel
handle transfer #2
...
SPI channel disable
]
If we can disable channel before configure it and reenable channel after it is configured, it will be more safe.

The commit a330ce2 "omap2_mcspi: Flush posted writes" make this risk to a real problem for ads7846 driver on omap3530evm. The ads7846 driver will send a SPI messge which includes,
[
TX_ONLY transfer (1 byte)
RX_ONLY transfer (2 bytes)
TX_ONLY transfer (1 byte)
RX_ONLY transfer (2 bytes)
]
If we don't add disable/reenable channel between TX and RX transfers, the RX transfer will get wrong datas sent from slave.

Signed-off-by Jason Wang <[email protected]>

---
drivers/spi/omap2_mcspi.c | 16 ++++++----------

1 files changed, 6 insertions(+), 10 deletions(-)

drivers/spi/omap2_mcspi.c +0 -1

index b3a94ca..1e3cb7b 100644
@@ -408,7 +408,6 @@ omap2_mcspi_txrx_dma(struct spi_device 
 *spi, struct spi_transfer *xfer)
 				count -= (word_len <= 8)  ? 2 :
 					(word_len <= 16) ? 4 :
 					/* word_len <= 32 */ 8;
-				omap2_mcspi_set_enable(spi, 1);
 				return count;
 			}

		}

drivers/spi/omap2_mcspi.c +3 -3

@@ -430,8 +429,10 @@ omap2_mcspi_txrx_dma(struct spi_device 
 *spi, struct spi_transfer *xfer)
 				 (word_len <= 16) ? 2 :
 			       /* word_len <= 32 */ 4;
 		}
-		omap2_mcspi_set_enable(spi, 1);
 	}
+
+	omap2_mcspi_set_enable(spi, 0);
+
 	return count;
 }

@@ -517,8 +518,6 @@ omap2_mcspi_txrx_pio(struct spi_device 
 *spi, struct spi_transfer *xfer)
 						goto out;
 					}
 					c = 0;
-				} else if (c == 0 && tx == NULL) {
-					omap2_mcspi_set_enable(spi, 0);
 				}
 

				*rx++ = __raw_readl(rx_reg);

drivers/spi/omap2_mcspi.c +0 -2

@@ -570,8 +569,6 @@ omap2_mcspi_txrx_pio(struct spi_device 
 *spi, struct spi_transfer *xfer)
 						goto out;
 					}
 					c = 0;
-				} else if (c == 0 && tx == NULL) {
-					omap2_mcspi_set_enable(spi, 0);
 				}
 

				*rx++ = __raw_readl(rx_reg);

drivers/spi/omap2_mcspi.c +0 -2

@@ -623,8 +620,6 @@ omap2_mcspi_txrx_pio(struct spi_device 
 *spi, struct spi_transfer *xfer)
 						goto out;
 					}
 					c = 0;
-				} else if (c == 0 && tx == NULL) {
-					omap2_mcspi_set_enable(spi, 0);
 				}
 

				*rx++ = __raw_readl(rx_reg);

drivers/spi/omap2_mcspi.c +0 -0

@@ -646,7 +641,7 @@ omap2_mcspi_txrx_pio(struct spi_device 
 *spi, struct spi_transfer *xfer)
 			dev_err(&spi->dev, "EOT timed out\n");
 	}

out:

drivers/spi/omap2_mcspi.c +1 -1

-	omap2_mcspi_set_enable(spi, 1);
+	omap2_mcspi_set_enable(spi, 0);

	return count - c;
}

drivers/spi/omap2_mcspi.c +0 -1

@@ -894,7 +889,6 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 		cs = spi->controller_state;
 		cd = spi->controller_data;
 
-		omap2_mcspi_set_enable(spi, 1);
 		list_for_each_entry(t, &m->transfers, transfer_list) {
 			if (t->tx_buf == NULL && t->rx_buf == 

NULL && t->len) {
				status = -EINVAL;

drivers/spi/omap2_mcspi.c +2 -0

@@ -931,6 +925,8 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 
 			mcspi_write_chconf0(spi, chconf);
 
+			omap2_mcspi_set_enable(spi, 1);
+
 			if (t->len) {
 				unsigned	count;

Signature · 2 lines

-- 
1.5.6.5

Quoted 8 lines

-----Original Message-----

					}
					c = 0;

(quoted diff) +0 -2

-				} else if (c == 0 && tx == NULL) {
-					omap2_mcspi_set_enable(spi, 0);

				}

Quoted 2 lines

				*rx++ = __raw_readl(rx_reg);

Why do you do this?
Reading the last word from the shift register buffer leads to the receiving new data to the buffer.
The channel needs to be disabled before the last word reading.

Quoted 36 lines

drivers/spi/omap2_mcspi.c +0 -0

@@ -646,7 +641,7 @@ omap2_mcspi_txrx_pio(struct spi_device 
 *spi, struct spi_transfer *xfer)
 			dev_err(&spi->dev, "EOT timed out\n");
 	}

out:

(quoted diff) +1 -1

-	omap2_mcspi_set_enable(spi, 1);
+	omap2_mcspi_set_enable(spi, 0);

	return count - c;
}

drivers/spi/omap2_mcspi.c +0 -1

@@ -894,7 +889,6 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 		cs = spi->controller_state;
 		cd = spi->controller_data;
 
-		omap2_mcspi_set_enable(spi, 1);
 		list_for_each_entry(t, &m->transfers, transfer_list) {
 			if (t->tx_buf == NULL && t->rx_buf == 

NULL && t->len) {
				status = -EINVAL;

drivers/spi/omap2_mcspi.c +2 -0

@@ -931,6 +925,8 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 
 			mcspi_write_chconf0(spi, chconf);
 
+			omap2_mcspi_set_enable(spi, 1);
+
 			if (t->len) {
 				unsigned	count;

Signature · 2 lines

-- 
1.5.6.5

Quoted 3 lines · roman.tereshonkov at nokia.com

[snip]

Quoted 9 lines

Quoted 9 lines

-----Original Message-----

? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?c = 0;
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? } else if (c == 0 && tx == NULL) {
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? omap2_mcspi_set_enable(spi, 0); ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}

Quoted 10 lines

Quoted 2 lines

? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?*rx++ = __raw_readl(rx_reg);

Why do you do this?
Reading the last word from the shift register buffer leads to the receiving new data to the buffer.
The channel needs to be disabled before the last word reading.

Yes, you are right. If this patch can be accepted, i will remove those changes in the V2 patch.
Please see my email replying grant likely's questions.

Thanks,
Jason.

Quoted 38 lines

Quoted 38 lines

(quoted diff) +0 -0

@@ -646,7 +641,7 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct
 spi_transfer *xfer)
 ? ? ? ? ? ? ? ? ? ? ? ?dev_err(&spi->dev, "EOT timed out\n");
 ? ? ? ?}

out:
- ? ? ? omap2_mcspi_set_enable(spi, 1);
+ ? ? ? omap2_mcspi_set_enable(spi, 0);
? ? ? ?return count - c;
}

(quoted diff) +0 -1

@@ -894,7 +889,6 @@ static void omap2_mcspi_work(struct work_struct
 *work)
 ? ? ? ? ? ? ? ?cs = spi->controller_state;
 ? ? ? ? ? ? ? ?cd = spi->controller_data;
 
- ? ? ? ? ? ? ? omap2_mcspi_set_enable(spi, 1);
 ? ? ? ? ? ? ? ?list_for_each_entry(t, &m->transfers, transfer_list) {

? ? ? ? ? ? ? ? ? ? ? ?if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?status = -EINVAL;

(quoted diff) +2 -1

@@ -931,6 +925,8 @@ static void omap2_mcspi_work(struct work_struct
 *work)
 
 ? ? ? ? ? ? ? ? ? ? ? ?mcspi_write_chconf0(spi, chconf);
 
+ ? ? ? ? ? ? ? ? ? ? ? omap2_mcspi_set_enable(spi, 1);
+
 ? ? ? ? ? ? ? ? ? ? ? ?if (t->len) {
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned ? ? ? ?count;
 
--

1.5.6.5

Quoted 25 lines · jason

Quoted 2 lines · roman.tereshonkov at nokia.com

[snip]

Quoted 8 lines

Quoted 8 lines

-----Original Message-----

                                       }
                                       c = 0;

drivers/spi/omap2_mcspi.c +0 -2

-                               } else if (c == 0 && tx == NULL) {
-                                       omap2_mcspi_set_enable(spi, 0);
                               }

Quoted 11 lines

Quoted 3 lines

                               *rx++ = __raw_readl(rx_reg);

Why do you do this?
Reading the last word from the shift register buffer leads to the receiving new data to the buffer.
The channel needs to be disabled before the last word reading.

Quoted 4 lines

Yes, you are right. If this patch can be accepted, i will remove those changes in the V2 patch.
Please see my email replying grant likely's questions.

Hi Jason.

I'm assuming I can ignore this patch and wait for your V2. I'll still want an ack from other omap spi users before I apply it though.

g.

Quoted 41 lines

Thanks,
Jason.

Quoted 37 lines

Quoted 37 lines

drivers/spi/omap2_mcspi.c +0 -0

@@ -646,7 +641,7 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct
 spi_transfer *xfer)
                       dev_err(&spi->dev, "EOT timed out\n");
       }

out:

drivers/spi/omap2_mcspi.c +1 -1

-       omap2_mcspi_set_enable(spi, 1);
+       omap2_mcspi_set_enable(spi, 0);
       return count - c;

}

drivers/spi/omap2_mcspi.c +0 -1

@@ -894,7 +889,6 @@ static void omap2_mcspi_work(struct work_struct
 *work)
               cs = spi->controller_state;
               cd = spi->controller_data;
 
-               omap2_mcspi_set_enable(spi, 1);
               list_for_each_entry(t, &m->transfers, transfer_list) {
                       if (t->tx_buf == NULL && t->rx_buf == NULL &&

t->len) {
                               status = -EINVAL;

drivers/spi/omap2_mcspi.c +2 -1

@@ -931,6 +925,8 @@ static void omap2_mcspi_work(struct work_struct
 *work)
 
                       mcspi_write_chconf0(spi, chconf);
 
+                       omap2_mcspi_set_enable(spi, 1);
+
                       if (t->len) {
                               unsigned        count;
 
--

1.5.6.5

In current design, the SPI channel is always enable during the period of handling a SPI message, it is risky when more than one SPI transfer are included in a message. Current working route like that:
[
?SPI channel enable
?configure channel
?handle transfer #1
?configure channel
?handle transfer #2
?...
?SPI channel disable
]
If we can disable channel before configure it and reenable channel after it is configured, it will be more safe.

The commit a330ce2 "omap2_mcspi: Flush posted writes" make this risk to a real problem for ads7846 driver on omap3530evm. The ads7846 driver will send a SPI messge which includes,
[
?TX_ONLY transfer (1 byte)
?RX_ONLY transfer (2 bytes)
?TX_ONLY transfer (1 byte)
?RX_ONLY transfer (2 bytes)
]
If we don't add disable/reenable channel between TX and RX transfers, the RX transfer will get wrong datas sent from slave.

Signed-off-by Jason Wang <[email protected]>

---
?drivers/spi/omap2_mcspi.c | ? 16 ++++++---------- ?1 files changed, 6 insertions(+), 10 deletions(-)

drivers/spi/omap2_mcspi.c +0 -0

index b3a94ca..1e3cb7b 100644
@@ -408,7 +408,6 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)

? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?count -= (word_len <= 8) ?? 2 :
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(word_len <= 16) ? 4 :
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?/* word_len <= 32 */ 8;
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? omap2_mcspi_set_enable(spi, 1); ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?return count;
? ? ? ? ? ? ? ? ? ? ? ?}
? ? ? ? ? ? ? ?}

drivers/spi/omap2_mcspi.c +0 -0

@@ -430,8 +429,10 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)

? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (word_len <= 16) ? 2 :
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /* word_len <= 32 */ 4; ? ? ? ? ? ? ? ?}
- ? ? ? ? ? ? ? omap2_mcspi_set_enable(spi, 1); ? ? ? ?}

drivers/spi/omap2_mcspi.c +3 -0

+
+ ? ? ? omap2_mcspi_set_enable(spi, 0);
+

? ? ? ?return count;
?}

drivers/spi/omap2_mcspi.c +0 -6

@@ -517,8 +518,6 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct spi_transfer *xfer)
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?goto out;
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?c = 0;
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? } else if (c == 0 && tx == NULL) {
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? omap2_mcspi_set_enable(spi, 0);
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}
 
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?*rx++ = __raw_readl(rx_reg);

@@ -570,8 +569,6 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct spi_transfer *xfer)
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?goto out;
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?c = 0;
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? } else if (c == 0 && tx == NULL) {
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? omap2_mcspi_set_enable(spi, 0);
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}
 
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?*rx++ = __raw_readl(rx_reg);

@@ -623,8 +620,6 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct spi_transfer *xfer)
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?goto out;
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?c = 0;
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? } else if (c == 0 && tx == NULL) {
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? omap2_mcspi_set_enable(spi, 0);
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}
 
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?*rx++ = __raw_readl(rx_reg);

@@ -646,7 +641,7 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct spi_transfer *xfer)
 ? ? ? ? ? ? ? ? ? ? ? ?dev_err(&spi->dev, "EOT timed out\n");
 ? ? ? ?}

?out:
- ? ? ? omap2_mcspi_set_enable(spi, 1);
+ ? ? ? omap2_mcspi_set_enable(spi, 0);
? ? ? ?return count - c;
?}

drivers/spi/omap2_mcspi.c +2 -2

@@ -894,7 +889,6 @@ static void omap2_mcspi_work(struct work_struct *work)
 ? ? ? ? ? ? ? ?cs = spi->controller_state;
 ? ? ? ? ? ? ? ?cd = spi->controller_data;
 
- ? ? ? ? ? ? ? omap2_mcspi_set_enable(spi, 1);
 ? ? ? ? ? ? ? ?list_for_each_entry(t, &m->transfers, transfer_list) {
 ? ? ? ? ? ? ? ? ? ? ? ?if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?status = -EINVAL;

@@ -931,6 +925,8 @@ static void omap2_mcspi_work(struct work_struct *work)
 
 ? ? ? ? ? ? ? ? ? ? ? ?mcspi_write_chconf0(spi, chconf);
 
+ ? ? ? ? ? ? ? ? ? ? ? omap2_mcspi_set_enable(spi, 1);
+
 ? ? ? ? ? ? ? ? ? ? ? ?if (t->len) {
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned ? ? ? ?count;
 
--

1.5.6.5

Quoted 15 lines · Jason Wang

In current design, the SPI channel is always enable during the period of handling a SPI message, it is risky when more than one SPI transfer are included in a message. Current working route like that:
[
SPI channel enable
configure channel
handle transfer #1
configure channel
handle transfer #2
...
SPI channel disable
]
If we can disable channel before configure it and reenable channel after it is configured, it will be more safe.

Quoted 12 lines

The commit a330ce2 "omap2_mcspi: Flush posted writes" make this risk to a real problem for ads7846 driver on omap3530evm. The ads7846 driver will send a SPI messge which includes,
[
TX_ONLY transfer (1 byte)
RX_ONLY transfer (2 bytes)
TX_ONLY transfer (1 byte)
RX_ONLY transfer (2 bytes)
]
If we don't add disable/reenable channel between TX and RX transfers, the RX transfer will get wrong datas sent from slave.

I don't understand the problem description. Are you talking about manipulation of the SS line, or a problem with the internal state on the mcspi controller device? What is the cause of the wrong data (ie. is there a protocol error on the wire, or is the SPI controller not performing the read correctly?)

Thanks,
g.

Quoted 7 lines

Signed-off-by Jason Wang <[email protected]>

---
 drivers/spi/omap2_mcspi.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

drivers/spi/omap2_mcspi.c +0 -0

-----Original Message-----
From: ext jason [mailto:jason77.wang at gmail.com]
Sent: 25 June, 2010 15:31
To: Grant Likely
Cc: Tereshonkov Roman (Nokia-D/Helsinki); david-b at pacbell.net; spi-devel-general at lists.sourceforge.net; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer

Quoted 1 line · Grant Likely

On Thu, Jun 24, 2010 at 6:12 AM, Jason Wang

Quoted 2 lines · <[email protected]> wrote:

Quoted 2 lines

In current design, the SPI channel is always enable during

the period

Quoted 1 line

Quoted 1 line

of handling a SPI message, it is risky when more than one

SPI transfer

Quoted 13 lines

Quoted 13 lines

are included in a message. Current working route like that:
[
SPI channel enable
configure channel
handle transfer #1
configure channel
handle transfer #2
...
SPI channel disable
]
If we can disable channel before configure it and reenable channel after it is configured, it will be more safe.

Quoted 10 lines

Quoted 10 lines

The commit a330ce2 "omap2_mcspi: Flush posted writes" make this risk to a real problem for ads7846 driver on omap3530evm. The ads7846 driver will send a SPI messge which includes,
[
TX_ONLY transfer (1 byte)
RX_ONLY transfer (2 bytes)
TX_ONLY transfer (1 byte)
RX_ONLY transfer (2 bytes)
]
If we don't add disable/reenable channel between TX and RX

transfers,

Quoted 6 lines

Quoted 2 lines

the RX transfer will get wrong datas sent from slave.

I don't understand the problem description. Are you talking about manipulation of the SS line, or a problem with the internal state on the mcspi controller device? What is the cause of the wrong

data (ie.

Quoted 7 lines

is there a protocol error on the wire, or is the SPI controller not performing the read correctly?)

Thanks,
g.

Sorry for unclear description. The problem is like that, when a TX_ONLY
transfer is
followed by a RX_ONLY transfer, the first data received in RX_ONLY transfer will be
the last data(random data) which is received in TX_ONLY transfer instead
of a meaningful data.

Thing like following logic,
In TX_ONLY transfer:
write data1(trigger transmit), (will receive a data simultaneously, and
keep it in RX register)
...
write dataN(last data), (will receive the last random data and keep it in RX register)
in the following RX_ONLY transfer:
change SPI controller to RX_ONLY mode,
the RXS bit will be set to status register immediately, write a dummy data to TX register to trigger this session(in fact this
trigger doesn't take effect),
check the RXS bit and read the first data(the first data is the last random data received in TX_ONLY transfer).

But if we disable this channel after the TX_ONLY transfer and reenable
it before the RX_ONLY transfer,
the trigger action will take effect and a meaningful data will be transfered to RX register.

Thanks,
Jason.

Quoted 7 lines

Quoted 7 lines

Signed-off-by Jason Wang <[email protected]>

---
 drivers/spi/omap2_mcspi.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

drivers/spi/omap2_mcspi.c +0 -0

[snip]

Quoted 10 lines

-----Original Message-----
From: ext jason [mailto:jason77.wang at gmail.com]
Sent: 25 June, 2010 15:31
To: Grant Likely
Cc: Tereshonkov Roman (Nokia-D/Helsinki); david-b at pacbell.net;

Quoted 8 lines

the RXS bit will be set to status register immediately, write a dummy data to TX register to trigger this session(in fact this
trigger doesn't take effect),
check the RXS bit and read the first data(the first data is the last random data received in TX_ONLY transfer).

For me it is very strange that you have RXS bit set after TX_ONLY transmission. The problem might be elsewhere and your solution will just mask the real problem. Can you enable the spi debug and error logs and show them for your broken case.

Regards
Roman Tereshonkov

Quoted 21 lines

But if we disable this channel after the TX_ONLY transfer and reenable
it before the RX_ONLY transfer,
the trigger action will take effect and a meaningful data will be transfered to RX register.

Thanks,
Jason.

Quoted 8 lines

Quoted 8 lines

Signed-off-by Jason Wang <[email protected]>

---
 drivers/spi/omap2_mcspi.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

drivers/spi/omap2_mcspi.c +0 -0

[snip]

-----Original Message-----
From: ext jason [mailto:jason77.wang at gmail.com]
Sent: 28 June, 2010 16:00
To: Tereshonkov Roman (Nokia-D/Helsinki)
Cc: grant.likely at secretlab.ca; david-b at pacbell.net; spi-devel-general at lists.sourceforge.net; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer

Quoted 10 lines · roman.tereshonkov at nokia.com

Quoted 10 lines

-----Original Message-----
From: ext jason [mailto:jason77.wang at gmail.com]
Sent: 25 June, 2010 15:31
To: Grant Likely
Cc: Tereshonkov Roman (Nokia-D/Helsinki); david-b at pacbell.net;

[snip]

Quoted 5 lines

Quoted 5 lines

the RXS bit will be set to status register immediately, write a dummy data to TX register to trigger this session(in fact this
trigger doesn't take effect),
check the RXS bit and read the first data(the first data is

the last

Quoted 5 lines

Quoted 3 lines

random data received in TX_ONLY transfer).

For me it is very strange that you have RXS bit set after

TX_ONLY transmission.

Quoted 1 line

The problem might be elsewhere and your solution will just

mask the real problem.

Quoted 1 line

Can you enable the spi debug and error logs and show them

for your broken case.

Quoted 5 lines

Regards
Roman Tereshonkov

Hi Roman,
I don't know how to enable the spi debug and generate an error log, but
i design
3 testcases to investigate this issue. These 3 testcases are based off the latest upstream.
test1: After TX_ONLY transfer, we will change this channel to RX_ONLY mode. And then we
will write a dummy data to trigger this RX transaction, here i add a polling RXS bit
before write triggering data, from the result, we can see we successfully to get a RXS
bit and read an unmeaningful data.(this prove the write triggering data
is not necessary
to start a rx transaction).
test2: This time i move the polling RXS bit to the location after the write trigger data, we can get
RXS bit as well and read an unmeaningful data too. The data read out is
identical to the one
in test1.(This prove the write trigger data don't take effect in fact).
test3: Except i revert the commit a330ce2 "omap2_mcspi: Flush posted writes", other is same as the test2.
But this time, we read out a correct data ads7846 sending out. (I don't
know why this commit can
affect the result, maybe it is time issue. If we can disable this channel after TX_ONLY transfer and
reenable it before RX_ONLY transfer, we will not get RXS bit in test1, and will read out a correct data
both in test2 and test3).

Test1 result output & patch:
# =====SPI MESSAGE BEGIN=====
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = f0
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = f0
*****SPI MESSAGE END*****
=====SPI MESSAGE BEGIN=====
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = f0
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = f0
*****SPI MESSAGE END*****

Subject: [PATCH] SPI/test1: get rx_data in RX_ONLY transfer before trigger writing

get rx_data in RX_ONLY transfer before trigger writing dummy data to TX register.

Signed-off-by Jason Wang <[email protected]>

---
drivers/spi/omap2_mcspi.c | 19 ++++++++++++++-----

1 files changed, 14 insertions(+), 5 deletions(-)

drivers/spi/omap2_mcspi.c +1 -1

index b3a94ca..ddfe4b0 100644
@@ -893,7 +893,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 spi = m->spi;
 cs = spi->controller_state;
 cd = spi->controller_data;
-
+ printk("=====SPI MESSAGE BEGIN=====\n");
 omap2_mcspi_set_enable(spi, 1);
 list_for_each_entry(t, &m->transfers, transfer_list) {

if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {

drivers/spi/omap2_mcspi.c +11 -2

@@ -928,17 +928,26 @@ static void omap2_mcspi_work(struct work_struct 
 *work)
 if (t->len > ((cs->word_len + 7) >> 3))
 chconf |= OMAP2_MCSPI_CHCONF_TURBO;
 }
-
+ printk("tansfer%s: conf=%x, data=%x\n", t->tx_buf ? "TX" : "RX",
+ chconf, t->tx_buf ? *(u8 *)(t->tx_buf) : 0);
 mcspi_write_chconf0(spi, chconf);
 
 if (t->len) {
 unsigned count;
 
 /* RX_ONLY mode needs dummy data in TX reg */
- if (t->tx_buf == NULL)
+ if (t->tx_buf == NULL) {
+ u8 rx_data;
+ if (mcspi_wait_for_reg_bit(cs->base + OMAP2_MCSPI_CHSTAT0,
+ OMAP2_MCSPI_CHSTAT_RXS) < 0)
+ dev_err(&spi->dev, "RXS timed out\n");
+
+ rx_data = __raw_readl(cs->base + OMAP2_MCSPI_RX0);
+ printk("rx_data = %x\n", rx_data);
+

__raw_writel(0, cs->base
+ OMAP2_MCSPI_TX0);
-
+ }
if (m->is_dma_mapped || t->len >= DMA_MIN_BYTES)
count = omap2_mcspi_txrx_dma(spi, t);
else

drivers/spi/omap2_mcspi.c +1 -1

@@ -971,7 +980,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 omap2_mcspi_force_cs(spi, 0);
 
 omap2_mcspi_set_enable(spi, 0);
-
+ printk("*****SPI MESSAGE END*****\n");
 m->status = status;
 m->complete(m->context);

Signature · 32 lines

-- 
1.5.6.5


Test2 result output & patch:
# =====SPI MESSAGE BEGIN=====
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = f0
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = f0
*****SPI MESSAGE END*****
=====SPI MESSAGE BEGIN=====
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = f0
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = f0
*****SPI MESSAGE END*****

Subject: [PATCH] SPI/test2: get rx_data in RX_ONLY transfer after 
trigger writing

get rx_data in RX_ONLY transfer after trigger writing dummy data to
TX register.

Signed-off-by: Jason Wang <[email protected]>
---
drivers/spi/omap2_mcspi.c | 18 ++++++++++++++----
1 files changed, 14 insertions(+), 4 deletions(-)

drivers/spi/omap2_mcspi.c +1 -1

index b3a94ca..96c508a 100644
@@ -893,7 +893,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 spi = m->spi;
 cs = spi->controller_state;
 cd = spi->controller_data;
-
+ printk("=====SPI MESSAGE BEGIN=====\n");
 omap2_mcspi_set_enable(spi, 1);
 list_for_each_entry(t, &m->transfers, transfer_list) {

if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {

drivers/spi/omap2_mcspi.c +4 -2

@@ -928,17 +928,27 @@ static void omap2_mcspi_work(struct work_struct 
 *work)
 if (t->len > ((cs->word_len + 7) >> 3))
 chconf |= OMAP2_MCSPI_CHCONF_TURBO;
 }
-
+ printk("tansfer%s: conf=%x, data=%x\n", t->tx_buf ? "TX" : "RX",
+ chconf, t->tx_buf ? *(u8 *)(t->tx_buf) : 0);
 mcspi_write_chconf0(spi, chconf);
 
 if (t->len) {
 unsigned count;
 
 /* RX_ONLY mode needs dummy data in TX reg */
- if (t->tx_buf == NULL)
+ if (t->tx_buf == NULL) {
+ u8 rx_data;

__raw_writel(0, cs->base
+ OMAP2_MCSPI_TX0);

+ if (mcspi_wait_for_reg_bit(cs->base + OMAP2_MCSPI_CHSTAT0,
+ OMAP2_MCSPI_CHSTAT_RXS) < 0)
+ dev_err(&spi->dev, "RXS timed out\n");

drivers/spi/omap2_mcspi.c +5 -0

+
+ rx_data = __raw_readl(cs->base + OMAP2_MCSPI_RX0);
+ printk("rx_data = %x\n", rx_data);
+
+ }

if (m->is_dma_mapped || t->len >= DMA_MIN_BYTES)
count = omap2_mcspi_txrx_dma(spi, t);
else

drivers/spi/omap2_mcspi.c +1 -1

@@ -971,7 +981,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 omap2_mcspi_force_cs(spi, 0);
 
 omap2_mcspi_set_enable(spi, 0);
-
+ printk("*****SPI MESSAGE END*****\n");
 m->status = status;
 m->complete(m->context);

Signature · 33 lines

-- 
1.5.6.5


Test3 result output & patch:
# =====SPI MESSAGE BEGIN=====
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = 77
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = 76
*****SPI MESSAGE END*****
=====SPI MESSAGE BEGIN=====
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = 75
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = 74
*****SPI MESSAGE END*****

Subject: [PATCH] SPI/test3: get rx_data in RX_ONLY transfer after 
trigger writing

At frist revert the commit a330ce2 "omap2_mcspi: Flush posted writes",
then get rx_data in RX_ONLY transfer after trigger writing dummy data
to TX register.

Signed-off-by: Jason Wang <[email protected]>
---
drivers/spi/omap2_mcspi.c | 19 ++++++++++++++-----
1 files changed, 14 insertions(+), 5 deletions(-)

drivers/spi/omap2_mcspi.c +0 -0

index b3a94ca..5d71cad 100644
@@ -204,7 +204,6 @@ static inline void 

mcspi_write_chconf0(const struct
spi_device *spi, u32 val)

cs->chconf0 = val;
mcspi_write_cs_reg(spi, OMAP2_MCSPI_CHCONF0, val);
- mcspi_read_cs_reg(spi, OMAP2_MCSPI_CHCONF0); }

static void omap2_mcspi_set_dma_req(const struct spi_device *spi,

drivers/spi/omap2_mcspi.c +1 -1

@@ -893,7 +892,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 spi = m->spi;
 cs = spi->controller_state;
 cd = spi->controller_data;
-
+ printk("=====SPI MESSAGE BEGIN=====\n");
 omap2_mcspi_set_enable(spi, 1);
 list_for_each_entry(t, &m->transfers, transfer_list) {

if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {

drivers/spi/omap2_mcspi.c +4 -2

@@ -928,17 +927,27 @@ static void omap2_mcspi_work(struct work_struct 
 *work)
 if (t->len > ((cs->word_len + 7) >> 3))
 chconf |= OMAP2_MCSPI_CHCONF_TURBO;
 }
-
+ printk("tansfer%s: conf=%x, data=%x\n", t->tx_buf ? "TX" : "RX",
+ chconf, t->tx_buf ? *(u8 *)(t->tx_buf) : 0);
 mcspi_write_chconf0(spi, chconf);
 
 if (t->len) {
 unsigned count;
 
 /* RX_ONLY mode needs dummy data in TX reg */
- if (t->tx_buf == NULL)
+ if (t->tx_buf == NULL) {
+ u8 rx_data;

__raw_writel(0, cs->base
+ OMAP2_MCSPI_TX0);

+ if (mcspi_wait_for_reg_bit(cs->base + OMAP2_MCSPI_CHSTAT0,
+ OMAP2_MCSPI_CHSTAT_RXS) < 0)
+ dev_err(&spi->dev, "RXS timed out\n");

drivers/spi/omap2_mcspi.c +5 -0

+
+ rx_data = __raw_readl(cs->base + OMAP2_MCSPI_RX0);
+ printk("rx_data = %x\n", rx_data);
+
+ }

if (m->is_dma_mapped || t->len >= DMA_MIN_BYTES)
count = omap2_mcspi_txrx_dma(spi, t);
else

drivers/spi/omap2_mcspi.c +1 -1

@@ -971,7 +980,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 omap2_mcspi_force_cs(spi, 0);
 
 omap2_mcspi_set_enable(spi, 0);
-
+ printk("*****SPI MESSAGE END*****\n");
 m->status = status;
 m->complete(m->context);

Signature · 2 lines

-- 
1.5.6.5

Quoted 21 lines

Quoted 21 lines

But if we disable this channel after the TX_ONLY transfer and reenable
it before the RX_ONLY transfer,
the trigger action will take effect and a meaningful data will be transfered to RX register.

Thanks,
Jason.

Quoted 8 lines

Quoted 8 lines

Signed-off-by Jason Wang <[email protected]>

---
 drivers/spi/omap2_mcspi.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

drivers/spi/omap2_mcspi.c +0 -0

[snip]

Hi Jason,

It is a little bit hard to analyze your logs.
1. You showed the bytes read in your own way but there is the data reading in omap2_mcspi_txrx_pio function also.

2. For your third test case. You try to read data after TX_ONLY, before triggering RX_ONLY, and get the right word. Looks like there is something wrong.

Try to log MCSPI_CHxSTAT register to follow when RXS bit (register RX is full) is set.
And do not use the RX register reading in you own way. If you need RX logging do it from omap2_mcspi_txrx_pio function.

Regards
Roman Tereshonkov

Quoted 394 lines

-----Original Message-----
From: ext jason [mailto:jason77.wang at gmail.com]
Sent: 28 June, 2010 16:00
To: Tereshonkov Roman (Nokia-D/Helsinki)
Cc: grant.likely at secretlab.ca; david-b at pacbell.net; spi-devel-general at lists.sourceforge.net; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer

Quoted 12 lines · roman.tereshonkov at nokia.com

Quoted 12 lines

-----Original Message-----
From: ext jason [mailto:jason77.wang at gmail.com]
Sent: 25 June, 2010 15:31
To: Grant Likely
Cc: Tereshonkov Roman (Nokia-D/Helsinki); david-b at pacbell.net;

[snip]

Quoted 6 lines

Quoted 6 lines

the RXS bit will be set to status register immediately, write a dummy data to TX register to trigger this session(in fact this
trigger doesn't take effect),
check the RXS bit and read the first data(the first data is

the last

Quoted 6 lines

Quoted 4 lines

random data received in TX_ONLY transfer).

For me it is very strange that you have RXS bit set after

TX_ONLY transmission.

Quoted 2 lines

The problem might be elsewhere and your solution will just

mask the real problem.

Quoted 2 lines

Can you enable the spi debug and error logs and show them

for your broken case.

Quoted 5 lines

Regards
Roman Tereshonkov

Hi Roman,
I don't know how to enable the spi debug and generate an error log, but
i design
3 testcases to investigate this issue. These 3 testcases are based off the latest upstream.
test1: After TX_ONLY transfer, we will change this channel to RX_ONLY mode. And then we
will write a dummy data to trigger this RX transaction, here i add a polling RXS bit
before write triggering data, from the result, we can see we successfully to get a RXS
bit and read an unmeaningful data.(this prove the write triggering data
is not necessary
to start a rx transaction).
test2: This time i move the polling RXS bit to the location after the write trigger data, we can get
RXS bit as well and read an unmeaningful data too. The data read out is
identical to the one
in test1.(This prove the write trigger data don't take effect in fact).
test3: Except i revert the commit a330ce2 "omap2_mcspi: Flush posted writes", other is same as the test2.
But this time, we read out a correct data ads7846 sending out. (I don't
know why this commit can
affect the result, maybe it is time issue. If we can disable this channel after TX_ONLY transfer and
reenable it before RX_ONLY transfer, we will not get RXS bit in test1, and will read out a correct data
both in test2 and test3).

Test1 result output & patch:
# =====SPI MESSAGE BEGIN=====
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = f0
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = f0
*****SPI MESSAGE END*****
=====SPI MESSAGE BEGIN=====
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = f0
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = f0
*****SPI MESSAGE END*****

Subject: [PATCH] SPI/test1: get rx_data in RX_ONLY transfer before trigger writing

get rx_data in RX_ONLY transfer before trigger writing dummy data to TX register.

Signed-off-by Jason Wang <[email protected]>

---
drivers/spi/omap2_mcspi.c | 19 ++++++++++++++-----

1 files changed, 14 insertions(+), 5 deletions(-)

drivers/spi/omap2_mcspi.c +1 -1

index b3a94ca..ddfe4b0 100644
@@ -893,7 +893,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 spi = m->spi;
 cs = spi->controller_state;
 cd = spi->controller_data;
-
+ printk("=====SPI MESSAGE BEGIN=====\n");
 omap2_mcspi_set_enable(spi, 1);
 list_for_each_entry(t, &m->transfers, transfer_list) {

if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {

drivers/spi/omap2_mcspi.c +11 -2

@@ -928,17 +928,26 @@ static void omap2_mcspi_work(struct work_struct 
 *work)
 if (t->len > ((cs->word_len + 7) >> 3))
 chconf |= OMAP2_MCSPI_CHCONF_TURBO;
 }
-
+ printk("tansfer%s: conf=%x, data=%x\n", t->tx_buf ? "TX" : "RX",
+ chconf, t->tx_buf ? *(u8 *)(t->tx_buf) : 0);
 mcspi_write_chconf0(spi, chconf);
 
 if (t->len) {
 unsigned count;
 
 /* RX_ONLY mode needs dummy data in TX reg */
- if (t->tx_buf == NULL)
+ if (t->tx_buf == NULL) {
+ u8 rx_data;
+ if (mcspi_wait_for_reg_bit(cs->base + OMAP2_MCSPI_CHSTAT0,
+ OMAP2_MCSPI_CHSTAT_RXS) < 0)
+ dev_err(&spi->dev, "RXS timed out\n");
+
+ rx_data = __raw_readl(cs->base + OMAP2_MCSPI_RX0);
+ printk("rx_data = %x\n", rx_data);
+

__raw_writel(0, cs->base
+ OMAP2_MCSPI_TX0);
-
+ }
if (m->is_dma_mapped || t->len >= DMA_MIN_BYTES)
count = omap2_mcspi_txrx_dma(spi, t);
else

drivers/spi/omap2_mcspi.c +1 -1

@@ -971,7 +980,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 omap2_mcspi_force_cs(spi, 0);
 
 omap2_mcspi_set_enable(spi, 0);
-
+ printk("*****SPI MESSAGE END*****\n");
 m->status = status;
 m->complete(m->context);

Signature · 32 lines

-- 
1.5.6.5


Test2 result output & patch:
# =====SPI MESSAGE BEGIN=====
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = f0
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = f0
*****SPI MESSAGE END*****
=====SPI MESSAGE BEGIN=====
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = f0
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = f0
*****SPI MESSAGE END*****

Subject: [PATCH] SPI/test2: get rx_data in RX_ONLY transfer after 
trigger writing

get rx_data in RX_ONLY transfer after trigger writing dummy data to
TX register.

Signed-off-by: Jason Wang <[email protected]>
---
drivers/spi/omap2_mcspi.c | 18 ++++++++++++++----
1 files changed, 14 insertions(+), 4 deletions(-)

drivers/spi/omap2_mcspi.c +1 -1

index b3a94ca..96c508a 100644
@@ -893,7 +893,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 spi = m->spi;
 cs = spi->controller_state;
 cd = spi->controller_data;
-
+ printk("=====SPI MESSAGE BEGIN=====\n");
 omap2_mcspi_set_enable(spi, 1);
 list_for_each_entry(t, &m->transfers, transfer_list) {

if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {

drivers/spi/omap2_mcspi.c +4 -2

@@ -928,17 +928,27 @@ static void omap2_mcspi_work(struct work_struct 
 *work)
 if (t->len > ((cs->word_len + 7) >> 3))
 chconf |= OMAP2_MCSPI_CHCONF_TURBO;
 }
-
+ printk("tansfer%s: conf=%x, data=%x\n", t->tx_buf ? "TX" : "RX",
+ chconf, t->tx_buf ? *(u8 *)(t->tx_buf) : 0);
 mcspi_write_chconf0(spi, chconf);
 
 if (t->len) {
 unsigned count;
 
 /* RX_ONLY mode needs dummy data in TX reg */
- if (t->tx_buf == NULL)
+ if (t->tx_buf == NULL) {
+ u8 rx_data;

__raw_writel(0, cs->base
+ OMAP2_MCSPI_TX0);

+ if (mcspi_wait_for_reg_bit(cs->base + OMAP2_MCSPI_CHSTAT0,
+ OMAP2_MCSPI_CHSTAT_RXS) < 0)
+ dev_err(&spi->dev, "RXS timed out\n");

drivers/spi/omap2_mcspi.c +5 -0

+
+ rx_data = __raw_readl(cs->base + OMAP2_MCSPI_RX0);
+ printk("rx_data = %x\n", rx_data);
+
+ }

if (m->is_dma_mapped || t->len >= DMA_MIN_BYTES)
count = omap2_mcspi_txrx_dma(spi, t);
else

drivers/spi/omap2_mcspi.c +1 -1

@@ -971,7 +981,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 omap2_mcspi_force_cs(spi, 0);
 
 omap2_mcspi_set_enable(spi, 0);
-
+ printk("*****SPI MESSAGE END*****\n");
 m->status = status;
 m->complete(m->context);

Signature · 33 lines

-- 
1.5.6.5


Test3 result output & patch:
# =====SPI MESSAGE BEGIN=====
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = 77
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = 76
*****SPI MESSAGE END*****
=====SPI MESSAGE BEGIN=====
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = 75
tansferTX: conf=1123d4, data=93
tansferRX: conf=1113d4, data=0
rx_data = 74
*****SPI MESSAGE END*****

Subject: [PATCH] SPI/test3: get rx_data in RX_ONLY transfer after 
trigger writing

At frist revert the commit a330ce2 "omap2_mcspi: Flush posted writes",
then get rx_data in RX_ONLY transfer after trigger writing dummy data
to TX register.

Signed-off-by: Jason Wang <[email protected]>
---
drivers/spi/omap2_mcspi.c | 19 ++++++++++++++-----
1 files changed, 14 insertions(+), 5 deletions(-)

drivers/spi/omap2_mcspi.c +0 -0

index b3a94ca..5d71cad 100644
@@ -204,7 +204,6 @@ static inline void 

mcspi_write_chconf0(const struct
spi_device *spi, u32 val)

cs->chconf0 = val;
mcspi_write_cs_reg(spi, OMAP2_MCSPI_CHCONF0, val);
- mcspi_read_cs_reg(spi, OMAP2_MCSPI_CHCONF0); }

static void omap2_mcspi_set_dma_req(const struct spi_device *spi,

drivers/spi/omap2_mcspi.c +1 -1

@@ -893,7 +892,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 spi = m->spi;
 cs = spi->controller_state;
 cd = spi->controller_data;
-
+ printk("=====SPI MESSAGE BEGIN=====\n");
 omap2_mcspi_set_enable(spi, 1);
 list_for_each_entry(t, &m->transfers, transfer_list) {

if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {

drivers/spi/omap2_mcspi.c +4 -2

@@ -928,17 +927,27 @@ static void omap2_mcspi_work(struct work_struct 
 *work)
 if (t->len > ((cs->word_len + 7) >> 3))
 chconf |= OMAP2_MCSPI_CHCONF_TURBO;
 }
-
+ printk("tansfer%s: conf=%x, data=%x\n", t->tx_buf ? "TX" : "RX",
+ chconf, t->tx_buf ? *(u8 *)(t->tx_buf) : 0);
 mcspi_write_chconf0(spi, chconf);
 
 if (t->len) {
 unsigned count;
 
 /* RX_ONLY mode needs dummy data in TX reg */
- if (t->tx_buf == NULL)
+ if (t->tx_buf == NULL) {
+ u8 rx_data;

__raw_writel(0, cs->base
+ OMAP2_MCSPI_TX0);

+ if (mcspi_wait_for_reg_bit(cs->base + OMAP2_MCSPI_CHSTAT0,
+ OMAP2_MCSPI_CHSTAT_RXS) < 0)
+ dev_err(&spi->dev, "RXS timed out\n");

drivers/spi/omap2_mcspi.c +5 -0

+
+ rx_data = __raw_readl(cs->base + OMAP2_MCSPI_RX0);
+ printk("rx_data = %x\n", rx_data);
+
+ }

if (m->is_dma_mapped || t->len >= DMA_MIN_BYTES)
count = omap2_mcspi_txrx_dma(spi, t);
else

drivers/spi/omap2_mcspi.c +1 -1

@@ -971,7 +980,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 omap2_mcspi_force_cs(spi, 0);
 
 omap2_mcspi_set_enable(spi, 0);
-
+ printk("*****SPI MESSAGE END*****\n");
 m->status = status;
 m->complete(m->context);

Signature · 2 lines

-- 
1.5.6.5

Quoted 24 lines

Quoted 24 lines

But if we disable this channel after the TX_ONLY transfer and reenable
it before the RX_ONLY transfer,
the trigger action will take effect and a meaningful data will be transfered to RX register.

Thanks,
Jason.

Quoted 9 lines

Quoted 9 lines

Signed-off-by Jason Wang <[email protected]>

---
 drivers/spi/omap2_mcspi.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

drivers/spi/omap2_mcspi.c +0 -0

[snip]

Hi Jason,

It is a little bit hard to analyze your logs.
1. You showed the bytes read in your own way but there is the data reading in omap2_mcspi_txrx_pio function also.
2. For your third test case. You try to read data after TX_ONLY, before triggering RX_ONLY, and get the right word. Looks like there is something wrong.

Try to log MCSPI_CHxSTAT register to follow when RXS bit (register RX is full) is set.
And do not use the RX register reading in you own way. If you need RX logging do it from omap2_mcspi_txrx_pio function.

Regards
Roman Tereshonkov

-----Original Message-----
From: ext jason [mailto:jason77.wang at gmail.com]
Sent: 01 July, 2010 14:59
To: Tereshonkov Roman (Nokia-MS/Helsinki)
Cc: grant.likely at secretlab.ca; david-b at pacbell.net; spi-devel-general at lists.sourceforge.net; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer

Quoted 4 lines · roman.tereshonkov at nokia.com

Hi Jason,

It is a little bit hard to analyze your logs.
1. You showed the bytes read in your own way but there is

the data reading in omap2_mcspi_txrx_pio function also.

Quoted 1 line

2. For your third test case. You try to read data after

TX_ONLY, before triggering RX_ONLY, and get the right word.

Quoted 3 lines

Looks like there is something wrong.

Try to log MCSPI_CHxSTAT register to follow when RXS bit

(register RX is full) is set.

Quoted 1 line

And do not use the RX register reading in you own way. If

you need RX logging do it from omap2_mcspi_txrx_pio function.

Quoted 7 lines

Regards
Roman Tereshonkov

Hi roman,

This time i enable SPI_DEBUG first then design 3 test cases to investigate spi issue.
test 1: print tx data, status reg(when RXS bit set) and rx data in txrx_pio func.
test 2: revert the commit a330ce2 "omap2_mcspi: Flush posted writes", other is same as test 1.
test 3: disable and enable channel between each SPI transfer, other is same as test 1.

The output of test2 and test3 is same and is right, while test1's output is
wrong.

The ads7846 driver works like that, when i touch the top-left corner of the
touchscreen, it will send a READ-Y command first(8 bit word 0x93, TX_ONLY transfer), then it will receive y coordinate(two 8 bit words, MSB
12bits are meaningful, correct data should be around 0x77nn, RX_ONLY transfer).

the output of test1 is:
<4>======SPI MESSAGE BEGIN======
<7>ads7846 spi1.0: write-8 93
<7>ads7846 spi1.0: status reg: 00000005
<7>ads7846 spi1.0: read-8 f0
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 77
<7>ads7846 spi1.0: write-8 93
<7>ads7846 spi1.0: status reg: 00000005
<7>ads7846 spi1.0: read-8 f0
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 77
<4>.....SPI MESSAGE END.....

the output of test2 is:
<4>======SPI MESSAGE BEGIN======
<7>ads7846 spi1.0: write-8 93
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 77
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 b0
<7>ads7846 spi1.0: write-8 93
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 77
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 88
<4>.....SPI MESSAGE END.....

the output of test3 is:
<4>======SPI MESSAGE BEGIN======
<7>ads7846 spi1.0: write-8 93
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 77
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 68
<7>ads7846 spi1.0: write-8 93
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 77
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 98
<4>.....SPI MESSAGE END.....

test1 patch:
Subject: [PATCH] SPI/test1: print tx data & rx data when ads7846 works

when we touch the top-left corner of the touchscreen, the ads7846 driver will send a read-y command(one 8-bit word, 0x93) and receive y coordinate(two 8-bit words, MSB 12bits are meaningful). now print all tx word, staus reg and rx word when RXS bit is set. Test1 only add debug output and have no other modification.

Signed-off-by Jason Wang <[email protected]>

---
drivers/spi/omap2_mcspi.c | 11 +++++++++--

1 files changed, 9 insertions(+), 2 deletions(-)

drivers/spi/omap2_mcspi.c +7 -0

index b3a94ca..893a124 100644
@@ -40,6 +40,8 @@
 #include <plat/clock.h>
 #include <plat/mcspi.h>
 
+#define VERBOSE
+
 #define OMAP2_MCSPI_MAX_FREQ 48000000
 
 /* OMAP2 has 3 SPI controllers, while OMAP3 has 4 */

@@ -502,6 +504,11 @@ omap2_mcspi_txrx_pio(struct spi_device 
 *spi, struct 
 spi_transfer *xfer)
 goto out;
 }
 
+#ifdef VERBOSE
+ dev_dbg(&spi->dev, "status reg: %08x\n",
+ __raw_readl(chstat_reg));
+#endif
+

if (c == 1 && tx == NULL &&
(l & OMAP2_MCSPI_CHCONF_TURBO)) {
omap2_mcspi_set_enable(spi, 0);

drivers/spi/omap2_mcspi.c +1 -1

@@ -893,7 +900,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 spi = m->spi;
 cs = spi->controller_state;
 cd = spi->controller_data;
-
+ printk("======SPI MESSAGE BEGIN======\n");
 omap2_mcspi_set_enable(spi, 1);
 list_for_each_entry(t, &m->transfers, transfer_list) {

if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {

drivers/spi/omap2_mcspi.c +1 -1

@@ -971,7 +978,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 omap2_mcspi_force_cs(spi, 0);
 
 omap2_mcspi_set_enable(spi, 0);
-
+ printk(".....SPI MESSAGE END.....\n");
 m->status = status;
 m->complete(m->context);

Signature · 17 lines

-- 
1.5.6.5


test2 patch:
Subject: [PATCH] SPI/test2: print tx data & rx data when ads7846 works

when we touch the top-left corner of the touchscreen, the ads7846
driver will send a read-y command(one 8-bit word, 0x93) and receive
y coordinate(two 8-bit words, MSB 12bits are meaningful). now print
all tx word and rx word. In test2, i revert the commit a330ce2
"omap2_mcspi: Flush posted writes".

Signed-off-by: Jason Wang <[email protected]>
---
drivers/spi/omap2_mcspi.c | 13 ++++++++++---
1 files changed, 10 insertions(+), 3 deletions(-)

drivers/spi/omap2_mcspi.c +2 -0

index b3a94ca..cf8066e 100644
@@ -40,6 +40,8 @@
 #include <plat/clock.h>
 #include <plat/mcspi.h>
 
+#define VERBOSE
+
 #define OMAP2_MCSPI_MAX_FREQ 48000000
 
 /* OMAP2 has 3 SPI controllers, while OMAP3 has 4 */

@@ -204,7 +206,7 @@ static inline void 

mcspi_write_chconf0(const struct
spi_device *spi, u32 val)

cs->chconf0 = val;
mcspi_write_cs_reg(spi, OMAP2_MCSPI_CHCONF0, val);
- mcspi_read_cs_reg(spi, OMAP2_MCSPI_CHCONF0);
+ /* mcspi_read_cs_reg(spi, OMAP2_MCSPI_CHCONF0); */ }

static void omap2_mcspi_set_dma_req(const struct spi_device *spi,

drivers/spi/omap2_mcspi.c +5 -0

@@ -502,6 +504,11 @@ omap2_mcspi_txrx_pio(struct spi_device 
 *spi, struct 
 spi_transfer *xfer)
 goto out;
 }
 
+#ifdef VERBOSE
+ dev_dbg(&spi->dev, "status reg: %08x\n",
+ __raw_readl(chstat_reg));
+#endif
+

if (c == 1 && tx == NULL &&
(l & OMAP2_MCSPI_CHCONF_TURBO)) {
omap2_mcspi_set_enable(spi, 0);

drivers/spi/omap2_mcspi.c +1 -1

@@ -893,7 +900,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 spi = m->spi;
 cs = spi->controller_state;
 cd = spi->controller_data;
-
+ printk("======SPI MESSAGE BEGIN======\n");
 omap2_mcspi_set_enable(spi, 1);
 list_for_each_entry(t, &m->transfers, transfer_list) {

if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {

drivers/spi/omap2_mcspi.c +1 -1

@@ -971,7 +978,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 omap2_mcspi_force_cs(spi, 0);
 
 omap2_mcspi_set_enable(spi, 0);
-
+ printk(".....SPI MESSAGE END.....\n");
 m->status = status;
 m->complete(m->context);

Signature · 17 lines

-- 
1.5.6.5


test3 patch:
Subject: [PATCH] SPI/test3: print tx data & rx data when ads7846 works

when we touch the top-left corner of the touchscreen, the ads7846
driver will send a read-y command(one 8-bit word, 0x93) and receive
y coordinate(two 8-bit words, MSB 12bits are meaningful). now print
all tx word and rx word. In test3, i add disable channel and reenable
channel between each SPI transfer.

Signed-off-by: Jason Wang <[email protected]>
---
drivers/spi/omap2_mcspi.c | 21 +++++++++++++++------
1 files changed, 15 insertions(+), 6 deletions(-)

drivers/spi/omap2_mcspi.c +2 -0

index b3a94ca..7449365 100644
@@ -40,6 +40,8 @@
 #include <plat/clock.h>
 #include <plat/mcspi.h>
 
+#define VERBOSE
+
 #define OMAP2_MCSPI_MAX_FREQ 48000000
 
 /* OMAP2 has 3 SPI controllers, while OMAP3 has 4 */

@@ -408,7 +410,6 @@ omap2_mcspi_txrx_dma(struct spi_device 
 *spi, struct 
 spi_transfer *xfer)

count -= (word_len <= 8) ? 2 :
(word_len <= 16) ? 4 :
/* word_len <= 32 */ 8;
- omap2_mcspi_set_enable(spi, 1);
return count;
}
}

drivers/spi/omap2_mcspi.c +0 -0

@@ -430,8 +431,10 @@ omap2_mcspi_txrx_dma(struct spi_device 
 *spi, struct 
 spi_transfer *xfer)

(word_len <= 16) ? 2 :
/* word_len <= 32 */ 4;
}
- omap2_mcspi_set_enable(spi, 1);
}

drivers/spi/omap2_mcspi.c +3 -0

+
+ omap2_mcspi_set_enable(spi, 0);
+

return count;
}

drivers/spi/omap2_mcspi.c +5 -0

@@ -502,6 +505,11 @@ omap2_mcspi_txrx_pio(struct spi_device 
 *spi, struct 
 spi_transfer *xfer)
 goto out;
 }
 
+#ifdef VERBOSE
+ dev_dbg(&spi->dev, "status reg: %08x\n",
+ __raw_readl(chstat_reg));
+#endif
+

if (c == 1 && tx == NULL &&
(l & OMAP2_MCSPI_CHCONF_TURBO)) {
omap2_mcspi_set_enable(spi, 0);

drivers/spi/omap2_mcspi.c +0 -0

@@ -646,7 +654,7 @@ omap2_mcspi_txrx_pio(struct spi_device 
 *spi, struct 
 spi_transfer *xfer)
 dev_err(&spi->dev, "EOT timed out\n");
 }

out:
- omap2_mcspi_set_enable(spi, 1);
+ omap2_mcspi_set_enable(spi, 0);
return count - c;
}

drivers/spi/omap2_mcspi.c +1 -2

@@ -893,8 +901,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 spi = m->spi;
 cs = spi->controller_state;
 cd = spi->controller_data;
-
- omap2_mcspi_set_enable(spi, 1);
+ printk("======SPI MESSAGE BEGIN======\n");
 list_for_each_entry(t, &m->transfers, transfer_list) {
 if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {

status = -EINVAL;

drivers/spi/omap2_mcspi.c +3 -1

@@ -931,6 +938,8 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 
 mcspi_write_chconf0(spi, chconf);
 
+ omap2_mcspi_set_enable(spi, 1);
+
 if (t->len) {
 unsigned count;

@@ -971,7 +980,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 omap2_mcspi_force_cs(spi, 0);
 
 omap2_mcspi_set_enable(spi, 0);
-
+ printk(".....SPI MESSAGE END.....\n");
 m->status = status;
 m->complete(m->context);

Signature · 2 lines

-- 
1.5.6.5

Hi Jason,

Your logs do not show what I wanted to see.
But what I can see now at least is the case when TX is full and RX is full at the same time.

1. Put
dev_dbg(&spi->dev, "status reg: %08x\n", __raw_readl(chstat_reg)); after "do" and before "while (c)" in omap2_mcspi_txrx_pio function. I want to see how status is changed before and after TX or RX transaction.
2. Also try to make fake reading
__raw_readl(tx_reg)
after TX write in omap2_mcspi_txrx_pio.
and
__raw_readl(cs->base + OMAP2_MCSPI_TX0);
in mcspi_work function.
This should exclude the posted write effect if such present.

If you put more logging info from other spi registers it might be also usefull in problem analyzing. And it is better to concentrate on your test case 1.
So as it is the test which gives the bug with unknown yet nature.

Regards
Roman Tereshonkov

Quoted 359 lines

-----Original Message-----
From: ext jason [mailto:jason77.wang at gmail.com]
Sent: 01 July, 2010 14:59
To: Tereshonkov Roman (Nokia-MS/Helsinki)
Cc: grant.likely at secretlab.ca; david-b at pacbell.net; spi-devel-general at lists.sourceforge.net; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer

Quoted 5 lines · roman.tereshonkov at nokia.com

Hi Jason,

It is a little bit hard to analyze your logs.
1. You showed the bytes read in your own way but there is

the data reading in omap2_mcspi_txrx_pio function also.

Quoted 2 lines

2. For your third test case. You try to read data after

TX_ONLY, before triggering RX_ONLY, and get the right word.

Quoted 4 lines

Looks like there is something wrong.

Try to log MCSPI_CHxSTAT register to follow when RXS bit

(register RX is full) is set.

Quoted 2 lines

And do not use the RX register reading in you own way. If

you need RX logging do it from omap2_mcspi_txrx_pio function.

Quoted 6 lines

Regards
Roman Tereshonkov

Hi roman,

This time i enable SPI_DEBUG first then design 3 test cases to investigate spi issue.
test 1: print tx data, status reg(when RXS bit set) and rx data in txrx_pio func.
test 2: revert the commit a330ce2 "omap2_mcspi: Flush posted writes", other is same as test 1.
test 3: disable and enable channel between each SPI transfer, other is same as test 1.

The output of test2 and test3 is same and is right, while test1's output is
wrong.

The ads7846 driver works like that, when i touch the top-left corner of the
touchscreen, it will send a READ-Y command first(8 bit word 0x93, TX_ONLY transfer), then it will receive y coordinate(two 8 bit words, MSB
12bits are meaningful, correct data should be around 0x77nn, RX_ONLY transfer).

the output of test1 is:
<4>======SPI MESSAGE BEGIN======
<7>ads7846 spi1.0: write-8 93
<7>ads7846 spi1.0: status reg: 00000005
<7>ads7846 spi1.0: read-8 f0
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 77
<7>ads7846 spi1.0: write-8 93
<7>ads7846 spi1.0: status reg: 00000005
<7>ads7846 spi1.0: read-8 f0
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 77
<4>.....SPI MESSAGE END.....

the output of test2 is:
<4>======SPI MESSAGE BEGIN======
<7>ads7846 spi1.0: write-8 93
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 77
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 b0
<7>ads7846 spi1.0: write-8 93
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 77
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 88
<4>.....SPI MESSAGE END.....

the output of test3 is:
<4>======SPI MESSAGE BEGIN======
<7>ads7846 spi1.0: write-8 93
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 77
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 68
<7>ads7846 spi1.0: write-8 93
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 77
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 98
<4>.....SPI MESSAGE END.....

test1 patch:
Subject: [PATCH] SPI/test1: print tx data & rx data when ads7846 works

when we touch the top-left corner of the touchscreen, the ads7846 driver will send a read-y command(one 8-bit word, 0x93) and receive y coordinate(two 8-bit words, MSB 12bits are meaningful). now print all tx word, staus reg and rx word when RXS bit is set. Test1 only add debug output and have no other modification.

Signed-off-by Jason Wang <[email protected]>

---
drivers/spi/omap2_mcspi.c | 11 +++++++++--

1 files changed, 9 insertions(+), 2 deletions(-)

drivers/spi/omap2_mcspi.c +7 -0

index b3a94ca..893a124 100644
@@ -40,6 +40,8 @@
 #include <plat/clock.h>
 #include <plat/mcspi.h>
 
+#define VERBOSE
+
 #define OMAP2_MCSPI_MAX_FREQ 48000000
 
 /* OMAP2 has 3 SPI controllers, while OMAP3 has 4 */

@@ -502,6 +504,11 @@ omap2_mcspi_txrx_pio(struct spi_device 
 *spi, struct 
 spi_transfer *xfer)
 goto out;
 }
 
+#ifdef VERBOSE
+ dev_dbg(&spi->dev, "status reg: %08x\n",
+ __raw_readl(chstat_reg));
+#endif
+

if (c == 1 && tx == NULL &&
(l & OMAP2_MCSPI_CHCONF_TURBO)) {
omap2_mcspi_set_enable(spi, 0);

drivers/spi/omap2_mcspi.c +1 -1

@@ -893,7 +900,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 spi = m->spi;
 cs = spi->controller_state;
 cd = spi->controller_data;
-
+ printk("======SPI MESSAGE BEGIN======\n");
 omap2_mcspi_set_enable(spi, 1);
 list_for_each_entry(t, &m->transfers, transfer_list) {

if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {

drivers/spi/omap2_mcspi.c +1 -1

@@ -971,7 +978,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 omap2_mcspi_force_cs(spi, 0);
 
 omap2_mcspi_set_enable(spi, 0);
-
+ printk(".....SPI MESSAGE END.....\n");
 m->status = status;
 m->complete(m->context);

Signature · 17 lines

-- 
1.5.6.5


test2 patch:
Subject: [PATCH] SPI/test2: print tx data & rx data when ads7846 works

when we touch the top-left corner of the touchscreen, the ads7846
driver will send a read-y command(one 8-bit word, 0x93) and receive
y coordinate(two 8-bit words, MSB 12bits are meaningful). now print
all tx word and rx word. In test2, i revert the commit a330ce2
"omap2_mcspi: Flush posted writes".

Signed-off-by: Jason Wang <[email protected]>
---
drivers/spi/omap2_mcspi.c | 13 ++++++++++---
1 files changed, 10 insertions(+), 3 deletions(-)

drivers/spi/omap2_mcspi.c +2 -0

index b3a94ca..cf8066e 100644
@@ -40,6 +40,8 @@
 #include <plat/clock.h>
 #include <plat/mcspi.h>
 
+#define VERBOSE
+
 #define OMAP2_MCSPI_MAX_FREQ 48000000
 
 /* OMAP2 has 3 SPI controllers, while OMAP3 has 4 */

@@ -204,7 +206,7 @@ static inline void 

mcspi_write_chconf0(const struct
spi_device *spi, u32 val)

cs->chconf0 = val;
mcspi_write_cs_reg(spi, OMAP2_MCSPI_CHCONF0, val);
- mcspi_read_cs_reg(spi, OMAP2_MCSPI_CHCONF0);
+ /* mcspi_read_cs_reg(spi, OMAP2_MCSPI_CHCONF0); */ }

static void omap2_mcspi_set_dma_req(const struct spi_device *spi,

drivers/spi/omap2_mcspi.c +5 -0

@@ -502,6 +504,11 @@ omap2_mcspi_txrx_pio(struct spi_device 
 *spi, struct 
 spi_transfer *xfer)
 goto out;
 }
 
+#ifdef VERBOSE
+ dev_dbg(&spi->dev, "status reg: %08x\n",
+ __raw_readl(chstat_reg));
+#endif
+

if (c == 1 && tx == NULL &&
(l & OMAP2_MCSPI_CHCONF_TURBO)) {
omap2_mcspi_set_enable(spi, 0);

drivers/spi/omap2_mcspi.c +1 -1

@@ -893,7 +900,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 spi = m->spi;
 cs = spi->controller_state;
 cd = spi->controller_data;
-
+ printk("======SPI MESSAGE BEGIN======\n");
 omap2_mcspi_set_enable(spi, 1);
 list_for_each_entry(t, &m->transfers, transfer_list) {

if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {

drivers/spi/omap2_mcspi.c +1 -1

@@ -971,7 +978,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 omap2_mcspi_force_cs(spi, 0);
 
 omap2_mcspi_set_enable(spi, 0);
-
+ printk(".....SPI MESSAGE END.....\n");
 m->status = status;
 m->complete(m->context);

Signature · 17 lines

-- 
1.5.6.5


test3 patch:
Subject: [PATCH] SPI/test3: print tx data & rx data when ads7846 works

when we touch the top-left corner of the touchscreen, the ads7846
driver will send a read-y command(one 8-bit word, 0x93) and receive
y coordinate(two 8-bit words, MSB 12bits are meaningful). now print
all tx word and rx word. In test3, i add disable channel and reenable
channel between each SPI transfer.

Signed-off-by: Jason Wang <[email protected]>
---
drivers/spi/omap2_mcspi.c | 21 +++++++++++++++------
1 files changed, 15 insertions(+), 6 deletions(-)

drivers/spi/omap2_mcspi.c +2 -0

index b3a94ca..7449365 100644
@@ -40,6 +40,8 @@
 #include <plat/clock.h>
 #include <plat/mcspi.h>
 
+#define VERBOSE
+
 #define OMAP2_MCSPI_MAX_FREQ 48000000
 
 /* OMAP2 has 3 SPI controllers, while OMAP3 has 4 */

@@ -408,7 +410,6 @@ omap2_mcspi_txrx_dma(struct spi_device 
 *spi, struct 
 spi_transfer *xfer)

count -= (word_len <= 8) ? 2 :
(word_len <= 16) ? 4 :
/* word_len <= 32 */ 8;
- omap2_mcspi_set_enable(spi, 1);
return count;
}
}

drivers/spi/omap2_mcspi.c +0 -0

@@ -430,8 +431,10 @@ omap2_mcspi_txrx_dma(struct spi_device 
 *spi, struct 
 spi_transfer *xfer)

(word_len <= 16) ? 2 :
/* word_len <= 32 */ 4;
}
- omap2_mcspi_set_enable(spi, 1);
}

drivers/spi/omap2_mcspi.c +3 -0

+
+ omap2_mcspi_set_enable(spi, 0);
+

return count;
}

drivers/spi/omap2_mcspi.c +5 -0

@@ -502,6 +505,11 @@ omap2_mcspi_txrx_pio(struct spi_device 
 *spi, struct 
 spi_transfer *xfer)
 goto out;
 }
 
+#ifdef VERBOSE
+ dev_dbg(&spi->dev, "status reg: %08x\n",
+ __raw_readl(chstat_reg));
+#endif
+

if (c == 1 && tx == NULL &&
(l & OMAP2_MCSPI_CHCONF_TURBO)) {
omap2_mcspi_set_enable(spi, 0);

drivers/spi/omap2_mcspi.c +0 -0

@@ -646,7 +654,7 @@ omap2_mcspi_txrx_pio(struct spi_device 
 *spi, struct 
 spi_transfer *xfer)
 dev_err(&spi->dev, "EOT timed out\n");
 }

out:
- omap2_mcspi_set_enable(spi, 1);
+ omap2_mcspi_set_enable(spi, 0);
return count - c;
}

drivers/spi/omap2_mcspi.c +1 -2

@@ -893,8 +901,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 spi = m->spi;
 cs = spi->controller_state;
 cd = spi->controller_data;
-
- omap2_mcspi_set_enable(spi, 1);
+ printk("======SPI MESSAGE BEGIN======\n");
 list_for_each_entry(t, &m->transfers, transfer_list) {
 if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {

status = -EINVAL;

drivers/spi/omap2_mcspi.c +3 -1

@@ -931,6 +938,8 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 
 mcspi_write_chconf0(spi, chconf);
 
+ omap2_mcspi_set_enable(spi, 1);
+
 if (t->len) {
 unsigned count;

@@ -971,7 +980,7 @@ static void omap2_mcspi_work(struct 
 work_struct *work)
 omap2_mcspi_force_cs(spi, 0);
 
 omap2_mcspi_set_enable(spi, 0);
-
+ printk(".....SPI MESSAGE END.....\n");
 m->status = status;
 m->complete(m->context);

Signature · 2 lines

-- 
1.5.6.5

Hi Jason,

Your logs do not show what I wanted to see.
But what I can see now at least is the case when TX is full and RX is full at the same time.

1. Put
dev_dbg(&spi->dev, "status reg: %08x\n", __raw_readl(chstat_reg)); after "do" and before "while (c)" in omap2_mcspi_txrx_pio function. I want to see how status is changed before and after TX or RX transaction.
2. Also try to make fake reading
__raw_readl(tx_reg)
after TX write in omap2_mcspi_txrx_pio.
and
__raw_readl(cs->base + OMAP2_MCSPI_TX0);
in mcspi_work function.
This should exclude the posted write effect if such present.

If you put more logging info from other spi registers it might be also usefull in problem analyzing. And it is better to concentrate on your test case 1.
So as it is the test which gives the bug with unknown yet nature.

Regards
Roman Tereshonkov

Hi Jason,

Your logs do not show what I wanted to see.
But what I can see now at least is the case when TX is full and RX is full at the same time.

1. Put
dev_dbg(&spi->dev, "status reg: %08x\n", __raw_readl(chstat_reg)); after "do" and before "while (c)" in omap2_mcspi_txrx_pio function. I want to see how status is changed before and after TX or RX transaction.
2. Also try to make fake reading
__raw_readl(tx_reg)
after TX write in omap2_mcspi_txrx_pio.
and
__raw_readl(cs->base + OMAP2_MCSPI_TX0);
in mcspi_work function.
This should exclude the posted write effect if such present.

If you put more logging info from other spi registers it might be also usefull in problem analyzing. And it is better to concentrate on your test case 1.
So as it is the test which gives the bug with unknown yet nature.

Regards
Roman Tereshonkov

Quoted 34 lines

-----Original Message-----
From: ext jason [mailto:jason77.wang at gmail.com]
Sent: 01 July, 2010 14:59
To: Tereshonkov Roman (Nokia-MS/Helsinki)
Cc: grant.likely at secretlab.ca; david-b at pacbell.net; spi-devel-general at lists.sourceforge.net; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer

Quoted 5 lines · roman.tereshonkov at nokia.com

Hi Jason,

It is a little bit hard to analyze your logs.
1. You showed the bytes read in your own way but there is

the data reading in omap2_mcspi_txrx_pio function also.

Quoted 2 lines

2. For your third test case. You try to read data after

TX_ONLY, before triggering RX_ONLY, and get the right word.

Quoted 4 lines

Looks like there is something wrong.

Try to log MCSPI_CHxSTAT register to follow when RXS bit

(register RX is full) is set.

Quoted 2 lines

And do not use the RX register reading in you own way. If

you need RX logging do it from omap2_mcspi_txrx_pio function.

Quoted 1 line

-----Original Message-----
From: ext jason [mailto:jason77.wang at gmail.com]
Sent: 13 July, 2010 16:26
To: Tereshonkov Roman (Nokia-MS/Helsinki)
Cc: grant.likely at secretlab.ca; david-b at pacbell.net; spi-devel-general at lists.sourceforge.net; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer

Hi roman,

How about my test case, does it expose the real problem?

I know my original patch was a big change both for PIO mode and for DMA mode, because there is no DMA mode device on my platform and i can't validate whether DMA mode works fine with that patch, it is a bit
risky to apply that patch to upstream.

This time i limit my patch to PIO mode, In theory, it is safe for all PIO transfers.
The side effect is to add a little bit overhead for a sequence of TX_ONLY transfers.
But it really can solve my touch panel issue.

Is the following patch acceptable?

From 7b310e690fcccff400233a4c3340f42168016c92 Mon Sep 17 00:00:00 2001
From: Jason Wang <[email protected]>
Date: Tue, 13 Jul 2010 18:05:59 +0800
Subject: [PATCH] spi/omap2_mcspi: disable channel after TX_ONLY transfer
in PIO mode

In TX_ONLY transfer, the spi controller will receive datas simultaneously and hold them in the rx register, these datas will affect the direct following RX_ONLY transfer. So add disable channel to purge those rx datas.

Signed-off-by Jason Wang <[email protected]>

---
drivers/spi/omap2_mcspi.c | 6 ++++++

1 files changed, 6 insertions(+), 0 deletions(-)

drivers/spi/omap2_mcspi.c +6 -0

index b3a94ca..43fab41 100644
@@ -644,6 +644,12 @@ omap2_mcspi_txrx_pio(struct spi_device 
 *spi, struct 
 spi_transfer *xfer)
 } else if (mcspi_wait_for_reg_bit(chstat_reg,
 OMAP2_MCSPI_CHSTAT_EOT) < 0)
 dev_err(&spi->dev, "EOT timed out\n");
+
+ /* disable chan to purge rx datas received in TX_ONLY transfer,
+ * otherwise these rx datas will affect the direct following
+ * RX_ONLY transfer.
+ */
+ omap2_mcspi_set_enable(spi, 0);
 }

out:
omap2_mcspi_set_enable(spi, 1);

Signature · 1 line

1.5.6.5

Quoted 4 lines · roman.tereshonkov at nokia.com

Hi Jason,

Your logs do not show what I wanted to see.
But what I can see now at least is the case when TX is full

and RX is full at the same time.

Quoted 3 lines

1. Put

	dev_dbg(&spi->dev, "status reg: %08x\n", 

__raw_readl(chstat_reg));

Quoted 1 line

after "do" and before "while (c)" in

omap2_mcspi_txrx_pio function.

Quoted 1 line

I want to see how status is changed before and after TX

or RX transaction.

Quoted 9 lines

2. Also try to make fake reading
__raw_readl(tx_reg)
after TX write in omap2_mcspi_txrx_pio.
and
__raw_readl(cs->base + OMAP2_MCSPI_TX0);
in mcspi_work function.
This should exclude the posted write effect if such present.

If you put more logging info from other spi registers it

might be also usefull in problem analyzing.

Quoted 43 lines

And it is better to concentrate on your test case 1.
So as it is the test which gives the bug with unknown yet nature.

Regards
Roman Tereshonkov

Quoted 34 lines

-----Original Message-----
From: ext jason [mailto:jason77.wang at gmail.com]
Sent: 01 July, 2010 14:59
To: Tereshonkov Roman (Nokia-MS/Helsinki)
Cc: grant.likely at secretlab.ca; david-b at pacbell.net; spi-devel-general at lists.sourceforge.net; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer

Quoted 5 lines · roman.tereshonkov at nokia.com

Hi Jason,

It is a little bit hard to analyze your logs.
1. You showed the bytes read in your own way but there is

the data reading in omap2_mcspi_txrx_pio function also.

Quoted 2 lines

2. For your third test case. You try to read data after

TX_ONLY, before triggering RX_ONLY, and get the right word.

Quoted 4 lines

Looks like there is something wrong.

Try to log MCSPI_CHxSTAT register to follow when RXS bit

(register RX is full) is set.

Quoted 2 lines

And do not use the RX register reading in you own way. If

you need RX logging do it from omap2_mcspi_txrx_pio function.

Quoted 1 line

<snip>

Hi,

I tried to find the board with the same touchscreen but failed.

Your patch solves the problem but does not fix the bug.
It would be nice if you find the roots of the bug to exclude future headackes.

I am leaving to vacation and will be silent some time.

Regards
Roman Tereshonkov

Quoted 162 lines

-----Original Message-----
From: ext jason [mailto:jason77.wang at gmail.com]
Sent: 13 July, 2010 16:26
To: Tereshonkov Roman (Nokia-MS/Helsinki)
Cc: grant.likely at secretlab.ca; david-b at pacbell.net; spi-devel-general at lists.sourceforge.net; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer

Hi roman,

How about my test case, does it expose the real problem?

I know my original patch was a big change both for PIO mode and for DMA mode, because there is no DMA mode device on my platform and i can't validate whether DMA mode works fine with that patch, it is a bit
risky to apply that patch to upstream.

This time i limit my patch to PIO mode, In theory, it is safe for all PIO transfers.
The side effect is to add a little bit overhead for a sequence of TX_ONLY transfers.
But it really can solve my touch panel issue.

Is the following patch acceptable?

From 7b310e690fcccff400233a4c3340f42168016c92 Mon Sep 17 00:00:00 2001
From: Jason Wang <[email protected]>
Date: Tue, 13 Jul 2010 18:05:59 +0800
Subject: [PATCH] spi/omap2_mcspi: disable channel after TX_ONLY transfer
in PIO mode

In TX_ONLY transfer, the spi controller will receive datas simultaneously and hold them in the rx register, these datas will affect the direct following RX_ONLY transfer. So add disable channel to purge those rx datas.

Signed-off-by Jason Wang <[email protected]>

---
drivers/spi/omap2_mcspi.c | 6 ++++++

1 files changed, 6 insertions(+), 0 deletions(-)

drivers/spi/omap2_mcspi.c +6 -0

index b3a94ca..43fab41 100644
@@ -644,6 +644,12 @@ omap2_mcspi_txrx_pio(struct spi_device 
 *spi, struct 
 spi_transfer *xfer)
 } else if (mcspi_wait_for_reg_bit(chstat_reg,
 OMAP2_MCSPI_CHSTAT_EOT) < 0)
 dev_err(&spi->dev, "EOT timed out\n");
+
+ /* disable chan to purge rx datas received in TX_ONLY transfer,
+ * otherwise these rx datas will affect the direct following
+ * RX_ONLY transfer.
+ */
+ omap2_mcspi_set_enable(spi, 0);
 }

out:
omap2_mcspi_set_enable(spi, 1);

Signature · 1 line

1.5.6.5

Quoted 5 lines · roman.tereshonkov at nokia.com

Hi Jason,

Your logs do not show what I wanted to see.
But what I can see now at least is the case when TX is full

and RX is full at the same time.

Quoted 3 lines

1. Put

	dev_dbg(&spi->dev, "status reg: %08x\n", 

__raw_readl(chstat_reg));

Quoted 2 lines

after "do" and before "while (c)" in

omap2_mcspi_txrx_pio function.

Quoted 2 lines

I want to see how status is changed before and after TX

or RX transaction.

Quoted 10 lines

2. Also try to make fake reading
__raw_readl(tx_reg)
after TX write in omap2_mcspi_txrx_pio.
and
__raw_readl(cs->base + OMAP2_MCSPI_TX0);
in mcspi_work function.
This should exclude the posted write effect if such present.

If you put more logging info from other spi registers it

might be also usefull in problem analyzing.

Quoted 54 lines

And it is better to concentrate on your test case 1.
So as it is the test which gives the bug with unknown yet nature.

Regards
Roman Tereshonkov

Quoted 44 lines

-----Original Message-----
From: ext jason [mailto:jason77.wang at gmail.com]
Sent: 01 July, 2010 14:59
To: Tereshonkov Roman (Nokia-MS/Helsinki)
Cc: grant.likely at secretlab.ca; david-b at pacbell.net; spi-devel-general at lists.sourceforge.net; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer

Quoted 6 lines · roman.tereshonkov at nokia.com

Hi Jason,

It is a little bit hard to analyze your logs.
1. You showed the bytes read in your own way but there is

the data reading in omap2_mcspi_txrx_pio function also.

Quoted 3 lines

2. For your third test case. You try to read data after

TX_ONLY, before triggering RX_ONLY, and get the right word.

Quoted 5 lines

Looks like there is something wrong.

Try to log MCSPI_CHxSTAT register to follow when RXS bit

(register RX is full) is set.

Quoted 3 lines

And do not use the RX register reading in you own way. If

you need RX logging do it from omap2_mcspi_txrx_pio function.

Quoted 2 lines

<snip>