* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-06-24 12:12 ` Jason Wang 0 siblings, 0 replies; 36+ messages in thread From: Jason Wang @ 2010-06-24 12:12 UTC (permalink / raw) To: grant.likely, roman.tereshonkov, david-b Cc: spi-devel-general, linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-06-24 12:12 ` Jason Wang 0 siblings, 0 replies; 36+ messages in thread From: Jason Wang @ 2010-06-24 12:12 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
[parent not found: <1277381565-6305-1-git-send-email-jason77.wang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer 2010-06-24 12:12 ` Jason Wang @ 2010-06-24 14:32 ` roman.tereshonkov at nokia.com -1 siblings, 0 replies; 36+ messages in thread From: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w @ 2010-06-24 14:32 UTC (permalink / raw) To: jason77.wang-Re5JQEeQqe8AvxtiuMwx3w, grant.likely-s3s/WqlpOiPyB63q8FvJNQ, david-b-yBeKhBN/0LDR7s880joybQ Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r >-----Original Message----- >From: ext Jason Wang [mailto:jason77.wang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org] >Sent: 24 June, 2010 15:13 >To: grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org; Tereshonkov Roman >(Nokia-D/Helsinki); david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org >Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; >linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.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-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 >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 > > ------------------------------------------------------------------------------ ThinkGeek and WIRED's GeekDad team up for the Ultimate GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the lucky parental unit. See the prize list and enter to win: http://p.sf.net/sfu/thinkgeek-promo ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-06-24 14:32 ` roman.tereshonkov at nokia.com 0 siblings, 0 replies; 36+ messages in thread From: roman.tereshonkov at nokia.com @ 2010-06-24 14:32 UTC (permalink / raw) To: linux-arm-kernel >-----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 > > ^ permalink raw reply [flat|nested] 36+ messages in thread
[parent not found: <E1C7579D1379824DAE67858071C810382DD83846FB-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org>]
* Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer 2010-06-24 14:32 ` roman.tereshonkov at nokia.com @ 2010-06-25 12:05 ` jason -1 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-06-25 12:05 UTC (permalink / raw) To: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w Cc: david-b-yBeKhBN/0LDR7s880joybQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org 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 >> >> >> ------------------------------------------------------------------------------ ThinkGeek and WIRED's GeekDad team up for the Ultimate GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the lucky parental unit. See the prize list and enter to win: http://p.sf.net/sfu/thinkgeek-promo ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-06-25 12:05 ` jason 0 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-06-25 12:05 UTC (permalink / raw) To: linux-arm-kernel 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 >> >> >> ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer 2010-06-25 12:05 ` jason @ 2010-06-27 6:08 ` Grant Likely -1 siblings, 0 replies; 36+ messages in thread From: Grant Likely @ 2010-06-27 6:08 UTC (permalink / raw) To: jason; +Cc: david-b, spi-devel-general, roman.tereshonkov, linux-arm-kernel On Fri, Jun 25, 2010 at 6:05 AM, jason <jason77.wang@gmail.com> wrote: > roman.tereshonkov@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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-06-27 6:08 ` Grant Likely 0 siblings, 0 replies; 36+ messages in thread From: Grant Likely @ 2010-06-27 6:08 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
[parent not found: <AANLkTinO3kndb9bIAGlz4-h2TW64NGthHBRcVlE3gqun-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer 2010-06-27 6:08 ` Grant Likely @ 2010-06-27 13:12 ` jason -1 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-06-27 13:12 UTC (permalink / raw) To: Grant Likely Cc: david-b-yBeKhBN/0LDR7s880joybQ, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r Grant Likely wrote: > On Fri, Jun 25, 2010 at 6:05 AM, jason <jason77.wang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > >> roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org 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 >>>> >>>> >>>> >>>> >> > > > > ------------------------------------------------------------------------------ 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-06-27 13:12 ` jason 0 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-06-27 13:12 UTC (permalink / raw) To: linux-arm-kernel 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 >>>> >>>> >>>> >>>> >> > > > > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer 2010-06-24 12:12 ` Jason Wang @ 2010-06-24 15:12 ` Grant Likely -1 siblings, 0 replies; 36+ messages in thread From: Grant Likely @ 2010-06-24 15:12 UTC (permalink / raw) To: Jason Wang Cc: david-b-yBeKhBN/0LDR7s880joybQ, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r On Thu, Jun 24, 2010 at 6:12 AM, Jason Wang <jason77.wang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-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 > 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. ------------------------------------------------------------------------------ ThinkGeek and WIRED's GeekDad team up for the Ultimate GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the lucky parental unit. See the prize list and enter to win: http://p.sf.net/sfu/thinkgeek-promo ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-06-24 15:12 ` Grant Likely 0 siblings, 0 replies; 36+ messages in thread From: Grant Likely @ 2010-06-24 15:12 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer 2010-06-24 15:12 ` Grant Likely @ 2010-06-25 12:30 ` jason -1 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-06-25 12:30 UTC (permalink / raw) To: Grant Likely Cc: david-b, spi-devel-general, roman.tereshonkov, linux-arm-kernel 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] ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-06-25 12:30 ` jason 0 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-06-25 12:30 UTC (permalink / raw) To: linux-arm-kernel 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] ^ permalink raw reply [flat|nested] 36+ messages in thread
[parent not found: <4C24A182.4060009-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer 2010-06-25 12:30 ` jason @ 2010-06-28 9:12 ` roman.tereshonkov at nokia.com -1 siblings, 0 replies; 36+ messages in thread From: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w @ 2010-06-28 9:12 UTC (permalink / raw) To: jason77.wang-Re5JQEeQqe8AvxtiuMwx3w, grant.likely-s3s/WqlpOiPyB63q8FvJNQ Cc: david-b-yBeKhBN/0LDR7s880joybQ, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r >-----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; >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 > >Grant Likely wrote: >> On Thu, Jun 24, 2010 at 6:12 AM, Jason Wang ><jason77.wang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-06-28 9:12 ` roman.tereshonkov at nokia.com 0 siblings, 0 replies; 36+ messages in thread From: roman.tereshonkov at nokia.com @ 2010-06-28 9:12 UTC (permalink / raw) To: linux-arm-kernel >-----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] > ^ permalink raw reply [flat|nested] 36+ messages in thread
[parent not found: <E1C7579D1379824DAE67858071C810382DD8495B26-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org>]
* Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer 2010-06-28 9:12 ` roman.tereshonkov at nokia.com @ 2010-06-28 12:59 ` jason -1 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-06-28 12:59 UTC (permalink / raw) To: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w Cc: david-b-yBeKhBN/0LDR7s880joybQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-06-28 12:59 ` jason 0 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-06-28 12:59 UTC (permalink / raw) To: linux-arm-kernel 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] >> >> ^ permalink raw reply related [flat|nested] 36+ messages in thread
* RE: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer 2010-06-28 12:59 ` jason @ 2010-06-29 10:20 ` roman.tereshonkov at nokia.com -1 siblings, 0 replies; 36+ messages in thread From: roman.tereshonkov @ 2010-06-29 10:20 UTC (permalink / raw) To: jason77.wang; +Cc: grant.likely, david-b, linux-arm-kernel, spi-devel-general 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@gmail.com] >Sent: 28 June, 2010 16:00 >To: Tereshonkov Roman (Nokia-D/Helsinki) >Cc: grant.likely@secretlab.ca; david-b@pacbell.net; >spi-devel-general@lists.sourceforge.net; >linux-arm-kernel@lists.infradead.org >Subject: Re: [PATCH] spi/omap2_mcspi: disable and enable chan >between each SPI transfer > >roman.tereshonkov@nokia.com wrote: >> >> >> >>> -----Original Message----- >>> From: ext jason [mailto:jason77.wang@gmail.com] >>> Sent: 25 June, 2010 15:31 >>> To: Grant Likely >>> Cc: Tereshonkov Roman (Nokia-D/Helsinki); david-b@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] >>> >>> > > ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-06-29 10:20 ` roman.tereshonkov at nokia.com 0 siblings, 0 replies; 36+ messages in thread From: roman.tereshonkov at nokia.com @ 2010-06-29 10:20 UTC (permalink / raw) To: linux-arm-kernel 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] >>> >>> > > ^ permalink raw reply [flat|nested] 36+ messages in thread
[parent not found: <E1C7579D1379824DAE67858071C810382DD84963BC-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org>]
* Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer 2010-06-29 10:20 ` roman.tereshonkov at nokia.com @ 2010-06-29 13:17 ` jason -1 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-06-29 13:17 UTC (permalink / raw) To: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w Cc: david-b-yBeKhBN/0LDR7s880joybQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-06-29 13:17 ` jason 0 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-06-29 13:17 UTC (permalink / raw) To: linux-arm-kernel 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] >>>> >>>> >>>> >> ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer 2010-06-29 10:20 ` roman.tereshonkov at nokia.com @ 2010-07-01 11:58 ` jason -1 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-07-01 11:58 UTC (permalink / raw) To: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w Cc: david-b-yBeKhBN/0LDR7s880joybQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f 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. > 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> --- 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> --- 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> --- 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 ------------------------------------------------------------------------------ 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-07-01 11:58 ` jason 0 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-07-01 11:58 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
[parent not found: <4C2C82E9.2010103-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer 2010-07-01 11:58 ` jason @ 2010-07-01 13:57 ` roman.tereshonkov at nokia.com -1 siblings, 0 replies; 36+ messages in thread From: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w @ 2010-07-01 13:57 UTC (permalink / raw) To: jason77.wang-Re5JQEeQqe8AvxtiuMwx3w Cc: david-b-yBeKhBN/0LDR7s880joybQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org] >Sent: 01 July, 2010 14:59 >To: Tereshonkov Roman (Nokia-MS/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: >> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >--- >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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >--- >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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >--- >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 > > > ------------------------------------------------------------------------------ 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-07-01 13:57 ` roman.tereshonkov at nokia.com 0 siblings, 0 replies; 36+ messages in thread From: roman.tereshonkov at nokia.com @ 2010-07-01 13:57 UTC (permalink / raw) To: linux-arm-kernel 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 > > > ^ permalink raw reply [flat|nested] 36+ messages in thread
[parent not found: <E1C7579D1379824DAE67858071C810382DD859571A-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org>]
* Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer 2010-07-01 13:57 ` roman.tereshonkov at nokia.com @ 2010-07-01 23:35 ` jason -1 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-07-01 23:35 UTC (permalink / raw) To: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w Cc: david-b-yBeKhBN/0LDR7s880joybQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org] >> Sent: 01 July, 2010 14:59 >> To: Tereshonkov Roman (Nokia-MS/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: >> >>> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >> --- >> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >> --- >> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >> --- >> 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 >> >> >> >> ------------------------------------------------------------------------------ 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-07-01 23:35 ` jason 0 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-07-01 23:35 UTC (permalink / raw) To: linux-arm-kernel 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 >> >> >> >> ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer 2010-07-01 13:57 ` roman.tereshonkov at nokia.com @ 2010-07-03 11:21 ` jason -1 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-07-03 11:21 UTC (permalink / raw) To: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w Cc: david-b-yBeKhBN/0LDR7s880joybQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> --- 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 ------------------------------------------------------------------------------ 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-07-03 11:21 ` jason 0 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-07-03 11:21 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer 2010-07-01 13:57 ` roman.tereshonkov at nokia.com @ 2010-07-13 13:26 ` jason -1 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-07-13 13:26 UTC (permalink / raw) To: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w Cc: david-b-yBeKhBN/0LDR7s880joybQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> --- 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-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org] >> Sent: 01 July, 2010 14:59 >> To: Tereshonkov Roman (Nokia-MS/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: >> >>> 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> ------------------------------------------------------------------------------ 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-07-13 13:26 ` jason 0 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-07-13 13:26 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply related [flat|nested] 36+ messages in thread
[parent not found: <4C3C6971.5010004-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer 2010-07-13 13:26 ` jason @ 2010-07-13 18:09 ` roman.tereshonkov at nokia.com -1 siblings, 0 replies; 36+ messages in thread From: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w @ 2010-07-13 18:09 UTC (permalink / raw) To: jason77.wang-Re5JQEeQqe8AvxtiuMwx3w Cc: david-b-yBeKhBN/0LDR7s880joybQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org] >Sent: 13 July, 2010 16:26 >To: Tereshonkov Roman (Nokia-MS/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 > >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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >--- >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-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org] >>> Sent: 01 July, 2010 14:59 >>> To: Tereshonkov Roman (Nokia-MS/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: >>> >>>> 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> > > ------------------------------------------------------------------------------ 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-07-13 18:09 ` roman.tereshonkov at nokia.com 0 siblings, 0 replies; 36+ messages in thread From: roman.tereshonkov at nokia.com @ 2010-07-13 18:09 UTC (permalink / raw) To: linux-arm-kernel 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> > > ^ permalink raw reply [flat|nested] 36+ messages in thread
[parent not found: <E1C7579D1379824DAE67858071C810382DD8772AB3-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org>]
* Re: [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer 2010-07-13 18:09 ` roman.tereshonkov at nokia.com @ 2010-07-13 23:48 ` jason -1 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-07-13 23:48 UTC (permalink / raw) To: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w Cc: david-b-yBeKhBN/0LDR7s880joybQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org] >> Sent: 13 July, 2010 16:26 >> To: Tereshonkov Roman (Nokia-MS/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 >> >> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >> --- >> 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-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org] >>>> Sent: 01 July, 2010 14:59 >>>> To: Tereshonkov Roman (Nokia-MS/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: >>>> >>>> >>>>> 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> >> >> >> ------------------------------------------------------------------------------ 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer @ 2010-07-13 23:48 ` jason 0 siblings, 0 replies; 36+ messages in thread From: jason @ 2010-07-13 23:48 UTC (permalink / raw) To: linux-arm-kernel 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> >> >> >> ^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2010-07-13 23:48 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-06-24 12:12 [PATCH] spi/omap2_mcspi: disable and enable chan between each SPI transfer Jason Wang
2010-06-24 12:12 ` Jason Wang
[not found] ` <1277381565-6305-1-git-send-email-jason77.wang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2010-06-24 14:32 ` roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w
2010-06-24 14:32 ` roman.tereshonkov at nokia.com
[not found] ` <E1C7579D1379824DAE67858071C810382DD83846FB-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org>
2010-06-25 12:05 ` jason
2010-06-25 12:05 ` jason
2010-06-27 6:08 ` Grant Likely
2010-06-27 6:08 ` Grant Likely
[not found] ` <AANLkTinO3kndb9bIAGlz4-h2TW64NGthHBRcVlE3gqun-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-06-27 13:12 ` jason
2010-06-27 13:12 ` jason
2010-06-24 15:12 ` Grant Likely
2010-06-24 15:12 ` Grant Likely
2010-06-25 12:30 ` jason
2010-06-25 12:30 ` jason
[not found] ` <4C24A182.4060009-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2010-06-28 9:12 ` roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w
2010-06-28 9:12 ` roman.tereshonkov at nokia.com
[not found] ` <E1C7579D1379824DAE67858071C810382DD8495B26-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org>
2010-06-28 12:59 ` jason
2010-06-28 12:59 ` jason
2010-06-29 10:20 ` roman.tereshonkov
2010-06-29 10:20 ` roman.tereshonkov at nokia.com
[not found] ` <E1C7579D1379824DAE67858071C810382DD84963BC-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org>
2010-06-29 13:17 ` jason
2010-06-29 13:17 ` jason
2010-07-01 11:58 ` jason
2010-07-01 11:58 ` jason
[not found] ` <4C2C82E9.2010103-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2010-07-01 13:57 ` roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w
2010-07-01 13:57 ` roman.tereshonkov at nokia.com
[not found] ` <E1C7579D1379824DAE67858071C810382DD859571A-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org>
2010-07-01 23:35 ` jason
2010-07-01 23:35 ` jason
2010-07-03 11:21 ` jason
2010-07-03 11:21 ` jason
2010-07-13 13:26 ` jason
2010-07-13 13:26 ` jason
[not found] ` <4C3C6971.5010004-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2010-07-13 18:09 ` roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w
2010-07-13 18:09 ` roman.tereshonkov at nokia.com
[not found] ` <E1C7579D1379824DAE67858071C810382DD8772AB3-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org>
2010-07-13 23:48 ` jason
2010-07-13 23:48 ` jason
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.