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

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

[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.

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 <jason77.wang@gmail.com>
---
 drivers/spi/omap2_mcspi.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
index b3a94ca..1e3cb7b 100644
--- a/drivers/spi/omap2_mcspi.c
+++ b/drivers/spi/omap2_mcspi.c
@@ -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;
 			}
 		}
@@ -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);
@@ -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;
 }
 
@@ -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

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

[roman.tereshonkov at nokia.com]

 

>-----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 <jason77.wang@gmail.com>
>---
> drivers/spi/omap2_mcspi.c |   16 ++++++----------
> 1 files changed, 6 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>index b3a94ca..1e3cb7b 100644
>--- a/drivers/spi/omap2_mcspi.c
>+++ b/drivers/spi/omap2_mcspi.c
>@@ -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;
> 			}
> 		}
>@@ -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);
>@@ -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);

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.



>@@ -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;
> }
> 
>@@ -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
>
>

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

[jason]

roman.tereshonkov at nokia.com wrote:
>  
>
>   
[snip]
>> -----Original Message-----
>>
>> 					}
>> 					c = 0;
>> -				} else if (c == 0 && tx == NULL) {
>> -					omap2_mcspi_set_enable(spi, 0);
>> 				}
>>
>> 				*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.
>   
>> @@ -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;
>> }
>>
>> @@ -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
>>
>>
>>     

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

[Grant Likely]

On Fri, Jun 25, 2010 at 6:05 AM, jason <jason77.wang@gmail.com> wrote:
> roman.tereshonkov at nokia.com wrote:
>>
>>
>>
>
> [snip]
>>>
>>> -----Original Message-----
>>>
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?c = 0;
>>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? } else if (c == 0 && tx == NULL) {
>>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? omap2_mcspi_set_enable(spi, 0);
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}
>>>
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?*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.

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.

>
> Thanks,
> Jason.
>>
>>
>>>
>>> @@ -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;
>>> }
>>>
>>> @@ -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
>>>
>>>
>>>
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

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

[jason]

Grant Likely wrote:
> On Fri, Jun 25, 2010 at 6:05 AM, jason <jason77.wang@gmail.com> wrote:
>   
>> roman.tereshonkov at nokia.com wrote:
>>     
>>>
>>>       
>> [snip]
>>     
>>>> -----Original Message-----
>>>>
>>>>                                        }
>>>>                                        c = 0;
>>>> -                               } else if (c == 0 && tx == NULL) {
>>>> -                                       omap2_mcspi_set_enable(spi, 0);
>>>>                                }
>>>>
>>>>                                *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.
>>     
>
> 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.
>
>   
Hi grant,
OK, i will prepare V2 and send out for review.

Hi roman,
I don't know if there is a TI omap3530evm board at your hand, if yes, 
you can
test touchscreen driver(ads7846) via tslib(ts_calibrate, ts_test).

Thanks,
Jason.
>> Thanks,
>> Jason.
>>     
>>>       
>>>> @@ -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;
>>>> }
>>>>
>>>> @@ -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
>>>>
>>>>
>>>>
>>>>         
>>     
>
>
>
>   

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

[Grant Likely]

On Thu, Jun 24, 2010 at 6:12 AM, Jason Wang <jason77.wang@gmail.com> wrote:
> 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.

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.

>
> Signed-off-by: Jason Wang <jason77.wang@gmail.com>
> ---
> ?drivers/spi/omap2_mcspi.c | ? 16 ++++++----------
> ?1 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
> index b3a94ca..1e3cb7b 100644
> --- a/drivers/spi/omap2_mcspi.c
> +++ b/drivers/spi/omap2_mcspi.c
> @@ -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;
> ? ? ? ? ? ? ? ? ? ? ? ?}
> ? ? ? ? ? ? ? ?}
> @@ -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);
> @@ -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;
> ?}
>
> @@ -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
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

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

[jason]

Grant Likely wrote:
> On Thu, Jun 24, 2010 at 6:12 AM, Jason Wang <jason77.wang@gmail.com> wrote:
>   
>> 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.
>>     
>
> 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.
>
>   
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.

>> Signed-off-by: Jason Wang <jason77.wang@gmail.com>
>> ---
>>  drivers/spi/omap2_mcspi.c |   16 ++++++----------
>>  1 files changed, 6 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>>     
[snip]

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

[roman.tereshonkov at nokia.com]

 

>-----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
>
>Grant Likely wrote:
>> On Thu, Jun 24, 2010 at 6:12 AM, Jason Wang 
><jason77.wang@gmail.com> wrote:
>>   
>>> 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.
>>>     
>>
>> 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.
>>
>>   
>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).
>

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

>
>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.
>
>>> Signed-off-by: Jason Wang <jason77.wang@gmail.com>
>>> ---
>>>  drivers/spi/omap2_mcspi.c |   16 ++++++----------
>>>  1 files changed, 6 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>>>     
>[snip]
>

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

[jason]

roman.tereshonkov at nokia.com wrote:
>  
>
>   
>> -----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]
>> 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
>
>   
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 <jason77.wang@gmail.com>
---
drivers/spi/omap2_mcspi.c | 19 ++++++++++++++-----
1 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
index b3a94ca..ddfe4b0 100644
--- a/drivers/spi/omap2_mcspi.c
+++ b/drivers/spi/omap2_mcspi.c
@@ -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) {
@@ -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
@@ -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);

-- 
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 <jason77.wang@gmail.com>
---
drivers/spi/omap2_mcspi.c | 18 ++++++++++++++----
1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
index b3a94ca..96c508a 100644
--- a/drivers/spi/omap2_mcspi.c
+++ b/drivers/spi/omap2_mcspi.c
@@ -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) {
@@ -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");
+
+ 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
@@ -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);

-- 
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 <jason77.wang@gmail.com>
---
drivers/spi/omap2_mcspi.c | 19 ++++++++++++++-----
1 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
index b3a94ca..5d71cad 100644
--- a/drivers/spi/omap2_mcspi.c
+++ b/drivers/spi/omap2_mcspi.c
@@ -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,
@@ -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) {
@@ -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");
+
+ 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
@@ -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);

-- 
1.5.6.5






>> 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.
>>
>>     
>>>> Signed-off-by: Jason Wang <jason77.wang@gmail.com>
>>>> ---
>>>>  drivers/spi/omap2_mcspi.c |   16 ++++++----------
>>>>  1 files changed, 6 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>>>>     
>>>>         
>> [snip]
>>
>>     

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

[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.
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: 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
>
>roman.tereshonkov at nokia.com wrote:
>>  
>>
>>   
>>> -----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]
>>> 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
>>
>>   
>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 <jason77.wang@gmail.com>
>---
>drivers/spi/omap2_mcspi.c | 19 ++++++++++++++-----
>1 files changed, 14 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>index b3a94ca..ddfe4b0 100644
>--- a/drivers/spi/omap2_mcspi.c
>+++ b/drivers/spi/omap2_mcspi.c
>@@ -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) {
>@@ -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
>@@ -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);
>
>-- 
>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 <jason77.wang@gmail.com>
>---
>drivers/spi/omap2_mcspi.c | 18 ++++++++++++++----
>1 files changed, 14 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>index b3a94ca..96c508a 100644
>--- a/drivers/spi/omap2_mcspi.c
>+++ b/drivers/spi/omap2_mcspi.c
>@@ -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) {
>@@ -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");
>+
>+ 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
>@@ -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);
>
>-- 
>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 <jason77.wang@gmail.com>
>---
>drivers/spi/omap2_mcspi.c | 19 ++++++++++++++-----
>1 files changed, 14 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>index b3a94ca..5d71cad 100644
>--- a/drivers/spi/omap2_mcspi.c
>+++ b/drivers/spi/omap2_mcspi.c
>@@ -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,
>@@ -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) {
>@@ -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");
>+
>+ 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
>@@ -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);
>
>-- 
>1.5.6.5
>
>
>
>
>
>
>>> 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.
>>>
>>>     
>>>>> Signed-off-by: Jason Wang <jason77.wang@gmail.com>
>>>>> ---
>>>>>  drivers/spi/omap2_mcspi.c |   16 ++++++----------
>>>>>  1 files changed, 6 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>>>>>     
>>>>>         
>>> [snip]
>>>
>>>     
>
>

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

[jason]

roman.tereshonkov at nokia.com wrote:
> 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.
>   
I add polling RXS bit and read rx register after TX_ONLY and before 
triggering RX_ONLY just to prove
this triggering is not necessary(test case1).
> 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.
>   
My 2nd test case is to read data after TX_ONLY and after triggering 
RX_ONLY, get wrong word just as
test case 1.
My 3rd test case revert a commit at first, then to read data after 
TX_ONLY and after triggering RX_ONLY,
this time get right word, the difference between case 3 and case 2 is 
only reverting a commit.
> 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.
>
>   
OK, i will try to generate a log as your suggestion.

Thanks,
Jason.
> Regards
> Roman Tereshonkov
>
>
>   
>> -----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
>>
>> roman.tereshonkov at nokia.com wrote:
>>     
>>>  
>>>
>>>   
>>>       
>>>> -----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]
>>     
>>>> 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
>>>
>>>   
>>>       
>> 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 <jason77.wang@gmail.com>
>> ---
>> drivers/spi/omap2_mcspi.c | 19 ++++++++++++++-----
>> 1 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>> index b3a94ca..ddfe4b0 100644
>> --- a/drivers/spi/omap2_mcspi.c
>> +++ b/drivers/spi/omap2_mcspi.c
>> @@ -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) {
>> @@ -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
>> @@ -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);
>>
>> -- 
>> 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 <jason77.wang@gmail.com>
>> ---
>> drivers/spi/omap2_mcspi.c | 18 ++++++++++++++----
>> 1 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>> index b3a94ca..96c508a 100644
>> --- a/drivers/spi/omap2_mcspi.c
>> +++ b/drivers/spi/omap2_mcspi.c
>> @@ -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) {
>> @@ -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");
>> +
>> + 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
>> @@ -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);
>>
>> -- 
>> 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 <jason77.wang@gmail.com>
>> ---
>> drivers/spi/omap2_mcspi.c | 19 ++++++++++++++-----
>> 1 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>> index b3a94ca..5d71cad 100644
>> --- a/drivers/spi/omap2_mcspi.c
>> +++ b/drivers/spi/omap2_mcspi.c
>> @@ -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,
>> @@ -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) {
>> @@ -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");
>> +
>> + 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
>> @@ -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);
>>
>> -- 
>> 1.5.6.5
>>
>>
>>
>>
>>
>>
>>     
>>>> 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.
>>>>
>>>>     
>>>>         
>>>>>> Signed-off-by: Jason Wang <jason77.wang@gmail.com>
>>>>>> ---
>>>>>>  drivers/spi/omap2_mcspi.c |   16 ++++++----------
>>>>>>  1 files changed, 6 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>>>>>>     
>>>>>>         
>>>>>>             
>>>> [snip]
>>>>
>>>>     
>>>>         
>>     

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

[jason]

roman.tereshonkov at nokia.com wrote:
> 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
>
>
>   
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 <jason77.wang@gmail.com>
---
drivers/spi/omap2_mcspi.c | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
index b3a94ca..893a124 100644
--- a/drivers/spi/omap2_mcspi.c
+++ b/drivers/spi/omap2_mcspi.c
@@ -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);
@@ -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) {
@@ -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);

-- 
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 <jason77.wang@gmail.com>
---
drivers/spi/omap2_mcspi.c | 13 ++++++++++---
1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
index b3a94ca..cf8066e 100644
--- a/drivers/spi/omap2_mcspi.c
+++ b/drivers/spi/omap2_mcspi.c
@@ -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,
@@ -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);
@@ -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) {
@@ -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);

-- 
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 <jason77.wang@gmail.com>
---
drivers/spi/omap2_mcspi.c | 21 +++++++++++++++------
1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
index b3a94ca..7449365 100644
--- a/drivers/spi/omap2_mcspi.c
+++ b/drivers/spi/omap2_mcspi.c
@@ -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;
}
}
@@ -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);
}
+
+ omap2_mcspi_set_enable(spi, 0);
+
return count;
}

@@ -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);
@@ -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;
}

@@ -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;
@@ -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);

-- 
1.5.6.5

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

[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.

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

>-----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
>
>roman.tereshonkov at nokia.com wrote:
>> 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
>>
>>
>>   
>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 <jason77.wang@gmail.com>
>---
>drivers/spi/omap2_mcspi.c | 11 +++++++++--
>1 files changed, 9 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>index b3a94ca..893a124 100644
>--- a/drivers/spi/omap2_mcspi.c
>+++ b/drivers/spi/omap2_mcspi.c
>@@ -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);
>@@ -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) {
>@@ -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);
>
>-- 
>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 <jason77.wang@gmail.com>
>---
>drivers/spi/omap2_mcspi.c | 13 ++++++++++---
>1 files changed, 10 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>index b3a94ca..cf8066e 100644
>--- a/drivers/spi/omap2_mcspi.c
>+++ b/drivers/spi/omap2_mcspi.c
>@@ -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,
>@@ -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);
>@@ -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) {
>@@ -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);
>
>-- 
>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 <jason77.wang@gmail.com>
>---
>drivers/spi/omap2_mcspi.c | 21 +++++++++++++++------
>1 files changed, 15 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>index b3a94ca..7449365 100644
>--- a/drivers/spi/omap2_mcspi.c
>+++ b/drivers/spi/omap2_mcspi.c
>@@ -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;
>}
>}
>@@ -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);
>}
>+
>+ omap2_mcspi_set_enable(spi, 0);
>+
>return count;
>}
>
>@@ -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);
>@@ -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;
>}
>
>@@ -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;
>@@ -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);
>
>-- 
>1.5.6.5
>
>
>

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

[jason]

roman.tereshonkov at nokia.com wrote:
> 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
>
>   
OK.

Thanks,
Jason.
>> -----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
>>
>> roman.tereshonkov at nokia.com wrote:
>>     
>>> 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
>>>
>>>
>>>   
>>>       
>> 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 <jason77.wang@gmail.com>
>> ---
>> drivers/spi/omap2_mcspi.c | 11 +++++++++--
>> 1 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>> index b3a94ca..893a124 100644
>> --- a/drivers/spi/omap2_mcspi.c
>> +++ b/drivers/spi/omap2_mcspi.c
>> @@ -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);
>> @@ -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) {
>> @@ -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);
>>
>> -- 
>> 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 <jason77.wang@gmail.com>
>> ---
>> drivers/spi/omap2_mcspi.c | 13 ++++++++++---
>> 1 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>> index b3a94ca..cf8066e 100644
>> --- a/drivers/spi/omap2_mcspi.c
>> +++ b/drivers/spi/omap2_mcspi.c
>> @@ -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,
>> @@ -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);
>> @@ -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) {
>> @@ -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);
>>
>> -- 
>> 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 <jason77.wang@gmail.com>
>> ---
>> drivers/spi/omap2_mcspi.c | 21 +++++++++++++++------
>> 1 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>> index b3a94ca..7449365 100644
>> --- a/drivers/spi/omap2_mcspi.c
>> +++ b/drivers/spi/omap2_mcspi.c
>> @@ -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;
>> }
>> }
>> @@ -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);
>> }
>> +
>> + omap2_mcspi_set_enable(spi, 0);
>> +
>> return count;
>> }
>>
>> @@ -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);
>> @@ -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;
>> }
>>
>> @@ -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;
>> @@ -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);
>>
>> -- 
>> 1.5.6.5
>>
>>
>>
>>     

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

[jason]

roman.tereshonkov at nokia.com wrote:
> 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 roman,
The test is designed just as your suggestion.

We get status reg=0x5(RX Full & TX FULL at the same time) in the first 
round in RX_ONLY
transfer. It seems that the RX_ONLY triggering write is not finished yet 
but we have received something.
I guess the last received data in TX_ONLY transfer affect the FXS bit in 
the first round of RX_ONLY transfer.


<4>======SPI MESSAGE BEGIN======
<7>ads7846 spi1.0: work tx reg(1): 00000000
<7>ads7846 spi1.0: after do stat reg: 00000006
<7>ads7846 spi1.0: write-8 93
<7>ads7846 spi1.0: after write tx reg: 00000093
<7>ads7846 spi1.0: before while(c) stat reg: 00000006
<7>ads7846 spi1.0: work tx reg(1): 00000093
<7>ads7846 spi1.0: after do stat reg: 00000000
<7>ads7846 spi1.0: status reg: 00000005
<7>ads7846 spi1.0: read-8 f0
<7>ads7846 spi1.0: before while(c) stat reg: 00000007
<7>ads7846 spi1.0: after do stat reg: 00000007
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 77
<7>ads7846 spi1.0: before while(c) stat reg: 00000006
<7>ads7846 spi1.0: work tx reg(1): 00000000
<7>ads7846 spi1.0: after do stat reg: 00000006
<7>ads7846 spi1.0: write-8 93
<7>ads7846 spi1.0: after write tx reg: 00000093
<7>ads7846 spi1.0: before while(c) stat reg: 00000006
<7>ads7846 spi1.0: work tx reg(1): 00000093
<7>ads7846 spi1.0: after do stat reg: 00000000
<7>ads7846 spi1.0: status reg: 00000005
<7>ads7846 spi1.0: read-8 f8
<7>ads7846 spi1.0: before while(c) stat reg: 00000007
<7>ads7846 spi1.0: after do stat reg: 00000007
<7>ads7846 spi1.0: status reg: 00000007
<7>ads7846 spi1.0: read-8 75
<7>ads7846 spi1.0: before while(c) stat reg: 00000006
<4>.....SPI MESSAGE END.....


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 <jason77.wang@gmail.com>
---
drivers/spi/omap2_mcspi.c | 32 ++++++++++++++++++++++++++++++--
1 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
index b3a94ca..76bbdeb 100644
--- a/drivers/spi/omap2_mcspi.c
+++ b/drivers/spi/omap2_mcspi.c
@@ -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 */
@@ -482,6 +484,10 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct 
spi_transfer *xfer)
tx = xfer->tx_buf;

do {
+#ifdef VERBOSE
+ dev_dbg(&spi->dev, "after do stat reg: %08x\n",
+ __raw_readl(chstat_reg));
+#endif
c -= 1;
if (tx != NULL) {
if (mcspi_wait_for_reg_bit(chstat_reg,
@@ -494,6 +500,12 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct 
spi_transfer *xfer)
word_len, *tx);
#endif
__raw_writel(*tx++, tx_reg);
+
+#ifdef VERBOSE
+ dev_dbg(&spi->dev, "after write tx reg: %08x\n",
+ __raw_readl(tx_reg));
+#endif
+
}
if (rx != NULL) {
if (mcspi_wait_for_reg_bit(chstat_reg,
@@ -502,6 +514,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);
@@ -527,6 +544,11 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct 
spi_transfer *xfer)
word_len, *(rx - 1));
#endif
}
+
+#ifdef VERBOSE
+ dev_dbg(&spi->dev, "before while(c) stat reg: %08x\n",
+ __raw_readl(chstat_reg));
+#endif
} while (c);
} else if (word_len <= 16) {
u16 *rx;
@@ -893,9 +915,15 @@ 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) {
+
+#ifdef VERBOSE
+ dev_dbg(&spi->dev, "work tx reg(1): %08x\n",
+ __raw_readl(cs->base + OMAP2_MCSPI_TX0));
+#endif
+
if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {
status = -EINVAL;
break;
@@ -971,7 +999,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);

-- 
1.5.6.5

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

[jason]

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 <jason77.wang@gmail.com>
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 <jason77.wang@gmail.com>
---
drivers/spi/omap2_mcspi.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
index b3a94ca..43fab41 100644
--- a/drivers/spi/omap2_mcspi.c
+++ b/drivers/spi/omap2_mcspi.c
@@ -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);
-- 
1.5.6.5



roman.tereshonkov at nokia.com wrote:
> 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
>
>   
>> -----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
>>
>> roman.tereshonkov at nokia.com wrote:
>>     
>>> 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.
>>     
>>>       
<snip>

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

[roman.tereshonkov at nokia.com]

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


>-----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 <jason77.wang@gmail.com>
>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 <jason77.wang@gmail.com>
>---
>drivers/spi/omap2_mcspi.c | 6 ++++++
>1 files changed, 6 insertions(+), 0 deletions(-)
>
>diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>index b3a94ca..43fab41 100644
>--- a/drivers/spi/omap2_mcspi.c
>+++ b/drivers/spi/omap2_mcspi.c
>@@ -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);
>-- 
>1.5.6.5
>
>
>
>roman.tereshonkov at nokia.com wrote:
>> 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
>>
>>   
>>> -----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
>>>
>>> roman.tereshonkov at nokia.com wrote:
>>>     
>>>> 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.
>>>     
>>>>       
><snip>
>
>

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

[jason]

roman.tereshonkov at nokia.com wrote:
> 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
>
>   
OK, Thanks, If possible, i will ask for help from IC designers to 
explain it.

Thanks,
Jason.
>   
>> -----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 <jason77.wang@gmail.com>
>> 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 <jason77.wang@gmail.com>
>> ---
>> drivers/spi/omap2_mcspi.c | 6 ++++++
>> 1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>> index b3a94ca..43fab41 100644
>> --- a/drivers/spi/omap2_mcspi.c
>> +++ b/drivers/spi/omap2_mcspi.c
>> @@ -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);
>> -- 
>> 1.5.6.5
>>
>>
>>
>> roman.tereshonkov at nokia.com wrote:
>>     
>>> 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
>>>
>>>   
>>>       
>>>> -----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
>>>>
>>>> roman.tereshonkov at nokia.com wrote:
>>>>     
>>>>         
>>>>> 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.
>>>>     
>>>>         
>>>>>       
>>>>>           
>> <snip>
>>
>>
>>