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

[jason <jason77.wang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org] 
>> Sent: 28 June, 2010 16:00
>> To: Tereshonkov Roman (Nokia-D/Helsinki)
>> Cc: grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org; david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org; 
>> spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; 
>> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> Subject: Re: [PATCH] spi/omap2_mcspi: disable and enable chan 
>> between each SPI transfer
>>
>> roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org wrote:
>>     
>>>  
>>>
>>>   
>>>       
>>>> -----Original Message-----
>>>> From: ext jason [mailto:jason77.wang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org] 
>>>> Sent: 25 June, 2010 15:31
>>>> To: Grant Likely
>>>> Cc: Tereshonkov Roman (Nokia-D/Helsinki); david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org; 
>>>>     
>>>>     
>>>>         
>> [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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>>> ---
>>>>>>  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]
>>>>
>>>>     
>>>>         
>>     


------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first