[PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

[PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

[Daniel Ribeiro]

Hi,

I am converting a driver from ssp.c API to pxa2xx_spi. But
when using PIO mode i am getting bogus rx data.

I did some tests, and it looks like CS is being deasserted
before the IO is done.

The following patch solved the issue for me.

Please CC me on replies, im not subscribed to spi-devel.

-- 
Daniel Ribeiro


-- patch --
pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

Signed-off-by: Daniel Ribeiro <drwyrm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

diff --git a/drivers/spi/pxa2xx_spi.c b/drivers/spi/pxa2xx_spi.c
index 0c452c4..4a9c34e 100644
--- a/drivers/spi/pxa2xx_spi.c
+++ b/drivers/spi/pxa2xx_spi.c
@@ -593,6 +593,10 @@ static void int_transfer_complete(struct
driver_data *drv_d {
        void __iomem *reg = drv_data->ioaddr;

+       if (wait_ssp_rx_stall(drv_data->ioaddr) == 0)
+               dev_err(&drv_data->pdev->dev,
+                       "interrupt_transfer: ssp rx stall failed\n");
+
        /* Stop SSP */
        write_SSSR(drv_data->clear_sr, reg);
        write_SSCR1(read_SSCR1(reg) & ~drv_data->int_cr1, reg);

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

[Daniel Ribeiro]

[-- Attachment #1: Type: text/plain, Size: 131 bytes --]

As it seems gmail web interface will not allow me to send an inline
patch. Here is
an attached version. Sorry.

-- 
Daniel Ribeiro

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: wait_rx_stall-before-deasserting-CS-on-PIO-mode.patch --]
[-- Type: text/x-diff; name=wait_rx_stall-before-deasserting-CS-on-PIO-mode.patch, Size: 645 bytes --]

pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

Signed-off-by: Daniel Ribeiro <drwyrm@gmail.com>

diff --git a/drivers/spi/pxa2xx_spi.c b/drivers/spi/pxa2xx_spi.c
index 0c452c4..4a9c34e 100644
--- a/drivers/spi/pxa2xx_spi.c
+++ b/drivers/spi/pxa2xx_spi.c
@@ -593,6 +593,10 @@ static void int_transfer_complete(struct driver_data *drv_data)
 {
 	void __iomem *reg = drv_data->ioaddr;
 
+	if (wait_ssp_rx_stall(drv_data->ioaddr) == 0)
+		dev_err(&drv_data->pdev->dev,
+			"interrupt_transfer: ssp rx stall failed\n");
+
 	/* Stop SSP */
 	write_SSSR(drv_data->clear_sr, reg);
 	write_SSCR1(read_SSCR1(reg) & ~drv_data->int_cr1, reg);

[-- Attachment #3: Type: text/plain, Size: 363 bytes --]

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

[-- Attachment #4: Type: text/plain, Size: 210 bytes --]

_______________________________________________
spi-devel-general mailing list
spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Re: [PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

[Ned Forrester]

Daniel Ribeiro wrote:
> Hi,
> 
> I am converting a driver from ssp.c API to pxa2xx_spi. But
> when using PIO mode i am getting bogus rx data.
> 
> I did some tests, and it looks like CS is being deasserted
> before the IO is done.
> 
> The following patch solved the issue for me.

Can you be more specific about your problem?  If CS is not being
asserted/deasserted at the right times, then that needs to be fixed.
What are you using for CS; this driver only works correctly when a GPIO
pin is assigned for CS, and is controlled with the cs_control() callback.

The below patch seems to paper over some other problem.
wait_ssp_rx_stall() is used to recover from errors, by flushing and
discarding words from the RX fifo.  Also, int_transfer_complete() is
only called from places where we know that all expected characters have
been received and transferred to memory.

This patch would "fix" a situation where extra words are in the RX FIFO
after the expected number have been read.  If there are more characters
in the FIFO than were transmitted, then that needs to be fixed and not
ignored.

Can you provide a patch or more information that addresses the
underlying problem?

> pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode
> 
> Signed-off-by: Daniel Ribeiro <drwyrm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> diff --git a/drivers/spi/pxa2xx_spi.c b/drivers/spi/pxa2xx_spi.c
> index 0c452c4..4a9c34e 100644
> --- a/drivers/spi/pxa2xx_spi.c
> +++ b/drivers/spi/pxa2xx_spi.c
> @@ -593,6 +593,10 @@ static void int_transfer_complete(struct driver_data *drv_data)
>  {
>  	void __iomem *reg = drv_data->ioaddr;
>  
> +	if (wait_ssp_rx_stall(drv_data->ioaddr) == 0)
> +		dev_err(&drv_data->pdev->dev,
> +			"interrupt_transfer: ssp rx stall failed\n");
> +
>  	/* Stop SSP */
>  	write_SSSR(drv_data->clear_sr, reg);
>  	write_SSCR1(read_SSCR1(reg) & ~drv_data->int_cr1, reg);
> 
> 


-- 
Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

[Daniel Ribeiro]

On Mon, Aug 18, 2008 at 3:53 PM, Ned Forrester <nforrester-/d+BM93fTQY@public.gmane.org> wrote:
> Can you be more specific about your problem?  If CS is not being
> asserted/deasserted at the right times, then that needs to be fixed.
> What are you using for CS; this driver only works correctly when a GPIO
> pin is assigned for CS, and is controlled with the cs_control() callback.

Yes, I´m using a gpio for CS. cs_control is OK.

> The below patch seems to paper over some other problem.
> wait_ssp_rx_stall() is used to recover from errors, by flushing and
> discarding words from the RX fifo.  Also, int_transfer_complete() is
> only called from places where we know that all expected characters have
> been received and transferred to memory.

Not really, something is really wrong with the call to int_transfer_complete.
If i replace the wait_ssp_rx_stall() call with a simple printk() call everything
works just fine. Same with an udelay(300).
I may be wrong, but it seems that interrupt_transfer calls int_transfer_complete
after the total amount of data is put on the FIFO, but it does not
guarantee that
the IO is actually done before calling it.

> This patch would "fix" a situation where extra words are in the RX FIFO
> after the expected number have been read.  If there are more characters
> in the FIFO than were transmitted, then that needs to be fixed and not
> ignored.
>
> Can you provide a patch or more information that addresses the
> underlying problem?

As I already said, i am converting a working driver from ssp.c´s
ssp_read_word/ssp_write_word to pxa2xx_spi. The driver works just
fine with:

gpio_set_value(cs, 0);
ssp_write_word(&ssp_dev, data);
ssp_read_word(&ssp_dev, &ret);
gpio_set_value(cs, 1);

After the conversion i ended with this code:
static int ezx_pcap_putget(u32 *data)
{
        struct spi_transfer t;
        struct spi_message m;

        spi_message_init(&m);
        t.len = 4;
        t.tx_buf = (u8 *)data;
        t.rx_buf = (u8 *)data;
        t.bits_per_word = 32;
        spi_message_add_tail(&t, &m);
        return spi_sync(pcap.spi, &m);
}

The driver is a for a PMIC, it has 32 25bit registers, which i need to
access trough 32bit spi IO.
Without the wait_ssp_rx_stall patch (or any other delay before the
CS_DEASSERT) this is what i get:

pcap_read: reg_num:3 value:00edf48a
pcap_write: reg_num:3 value:00123456
pcap_read: reg_num:3 value:00edf48a
pcap_read: reg_num:3 value:00edf48a

With the patch, or with an udelay(300) call instead of wait_ssp_rx_stall:
pcap_read: reg_num:3 value:00edf48a
pcap_write: reg_num:3 value:00123456
pcap_read: reg_num:3 value:00123456
pcap_read: reg_num:3 value:00123456

Since the udelay(300) call works, and it doesnt touch the ssp registers,
it looks like int_transfer_complete is being called before it should.

-- 
Daniel Ribeiro

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

[Ned Forrester]

Daniel Ribeiro wrote:
> On Mon, Aug 18, 2008 at 3:53 PM, Ned Forrester <nforrester-/d+BM93fTQY@public.gmane.org> wrote:
>> Can you be more specific about your problem?  If CS is not being
>> asserted/deasserted at the right times, then that needs to be fixed.
>> What are you using for CS; this driver only works correctly when a GPIO
>> pin is assigned for CS, and is controlled with the cs_control() callback.
> 
> Yes, IŽm using a gpio for CS. cs_control is OK.

OK.

>> The below patch seems to paper over some other problem.
>> wait_ssp_rx_stall() is used to recover from errors, by flushing and
>> discarding words from the RX fifo.  Also, int_transfer_complete() is
>> only called from places where we know that all expected characters have
>> been received and transferred to memory.
> 
> Not really, something is really wrong with the call to int_transfer_complete.
> If i replace the wait_ssp_rx_stall() call with a simple printk() call everything
> works just fine. Same with an udelay(300).
> I may be wrong, but it seems that interrupt_transfer calls int_transfer_complete
> after the total amount of data is put on the FIFO, but it does not
> guarantee that
> the IO is actually done before calling it.

Well, if you get the same effect by using a delay, then the problem is
not extra data in the fifo.  If you read the code in the driver
carefully, I think you will find that int_transfer_complete is called
after every expected character is read from the FIFO and after that
point, there is no opportunity to read anymore characters to memory.  I
am puzzled as to why a delay in int_transfer_complete would have any
influence because nothing is read from the FIFO to memory after it is
called.  See a suggestion below.

>> This patch would "fix" a situation where extra words are in the RX FIFO
>> after the expected number have been read.  If there are more characters
>> in the FIFO than were transmitted, then that needs to be fixed and not
>> ignored.
>>
>> Can you provide a patch or more information that addresses the
>> underlying problem?
> 
> As I already said, i am converting a working driver from ssp.cŽs
> ssp_read_word/ssp_write_word to pxa2xx_spi. The driver works just
> fine with:
> 
> gpio_set_value(cs, 0);
> ssp_write_word(&ssp_dev, data);
> ssp_read_word(&ssp_dev, &ret);
> gpio_set_value(cs, 1);
> 
> After the conversion i ended with this code:
> static int ezx_pcap_putget(u32 *data)
> {
>         struct spi_transfer t;
>         struct spi_message m;
> 
>         spi_message_init(&m);
>         t.len = 4;
>         t.tx_buf = (u8 *)data;
>         t.rx_buf = (u8 *)data;
>         t.bits_per_word = 32;
>         spi_message_add_tail(&t, &m);
>         return spi_sync(pcap.spi, &m);
> }

So far this looks good, but what have you declared about the chip in the
chip_info structure?  In particular, what timeout and FIFO thresholds?
What processor are you using, and which port on that processor; there is
a difference in the handling of the end of a transfer between the SSP
port on an PXA25x (which has no timeout interrupt) and all other ports
handled by this driver (including the NSSP port on the PXA25x and all
the ports on the PXA270).

> The driver is a for a PMIC, it has 32 25bit registers, which i need to
> access trough 32bit spi IO.
> Without the wait_ssp_rx_stall patch (or any other delay before the
> CS_DEASSERT) this is what i get:

OK, this should be possible.

> pcap_read: reg_num:3 value:00edf48a
> pcap_write: reg_num:3 value:00123456
> pcap_read: reg_num:3 value:00edf48a
> pcap_read: reg_num:3 value:00edf48a
> 
> With the patch, or with an udelay(300) call instead of wait_ssp_rx_stall:
> pcap_read: reg_num:3 value:00edf48a
> pcap_write: reg_num:3 value:00123456
> pcap_read: reg_num:3 value:00123456
> pcap_read: reg_num:3 value:00123456
> 
> Since the udelay(300) call works, and it doesnt touch the ssp registers,
> it looks like int_transfer_complete is being called before it should.

Very strange, but I can't draw the same conclusion.
int_transfer_complete is only called from 4 places in int_transfer, and
then only after verifying that the full count of receive characters has
been transfered to memory.  Even if the transfer was not complete in
hardware, or if the CS was dropped too soon, this delay would not fix
the problem.  There has to be something else going on.

I am not familiar with the chip you are using.  Are you certain that
when you write a register, you should expect back the same value that
you wrote, when received on the same transfer.  It makes sense that the
chip might do this, but I thought I should ask, because there are other
possibilities.

One thing that is odd about the setup of your message is the use of the
same buffer for transmit and receive.  I agree that this should be
possible, as nothing is received until the corresponding word has been
transmitted.  However, this might be obscuring some of the symptoms.
Can you try assigning separate buffers for tx and rx, and then test to
see what the results are with and without the added delay?  This might
shed some more light on what is happening.

Also try setting up the pxa2xx_spi driver to the correct bits-per-word
though chip_info and spi_setup(), and turn off (make 0) the per-transfer
bits-per-word, above.  This will test to see if there is an error
handling the per-transfer changes.  I don't think many applications use
the per-transfer changes, so that is another possible source of problems.

-- 
Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

[Daniel Ribeiro]

On Mon, Aug 18, 2008 at 5:47 PM, Ned Forrester <nforrester-/d+BM93fTQY@public.gmane.org> wrote:
> So far this looks good, but what have you declared about the chip in the
> chip_info structure?  In particular, what timeout and FIFO thresholds?
> What processor are you using, and which port on that processor; there is
> a difference in the handling of the end of a transfer between the SSP
> port on an PXA25x (which has no timeout interrupt) and all other ports
> handled by this driver (including the NSSP port on the PXA25x and all
> the ports on the PXA270).

.tx_threshold = 1,
.rx_threshold = 1,
.timeout = 1000,

Its a PXA270, SSP1.

>> pcap_read: reg_num:3 value:00edf48a
>> pcap_write: reg_num:3 value:00123456
>> pcap_read: reg_num:3 value:00edf48a
>> pcap_read: reg_num:3 value:00edf48a
>>
>> With the patch, or with an udelay(300) call instead of wait_ssp_rx_stall:
>> pcap_read: reg_num:3 value:00edf48a
>> pcap_write: reg_num:3 value:00123456
>> pcap_read: reg_num:3 value:00123456
>> pcap_read: reg_num:3 value:00123456
>>
>> Since the udelay(300) call works, and it doesnt touch the ssp registers,
>> it looks like int_transfer_complete is being called before it should.
>
> Very strange, but I can't draw the same conclusion.
> int_transfer_complete is only called from 4 places in int_transfer, and
> then only after verifying that the full count of receive characters has
> been transfered to memory.  Even if the transfer was not complete in
> hardware, or if the CS was dropped too soon, this delay would not fix
> the problem.  There has to be something else going on.
> I am not familiar with the chip you are using.  Are you certain that
> when you write a register, you should expect back the same value that
> you wrote, when received on the same transfer.  It makes sense that the
> chip might do this, but I thought I should ask, because there are other
> possibilities.

Its not the same transfer. pcap_read and pcap_write each call pcap_putget.
So, to _write_a_register_ i write then read a spi word. And to _read_a_register_
i write then read another spi word. Those write/read register operations can´t
happen on the same CS assertion.

> One thing that is odd about the setup of your message is the use of the
> same buffer for transmit and receive.  I agree that this should be
> possible, as nothing is received until the corresponding word has been
> transmitted.  However, this might be obscuring some of the symptoms.
> Can you try assigning separate buffers for tx and rx, and then test to
> see what the results are with and without the added delay?  This might
> shed some more light on what is happening.

I tried this already, used different buffers, and it makes no difference, same
wrong behaviour without the delay, and the same correct behaviour with the
delay. The fact of using the same buffer is not the issue.

> Also try setting up the pxa2xx_spi driver to the correct bits-per-word
> though chip_info and spi_setup(), and turn off (make 0) the per-transfer
> bits-per-word, above.  This will test to see if there is an error
> handling the per-transfer changes.  I don't think many applications use
> the per-transfer changes, so that is another possible source of problems.

Same. This doesnt seem to be the issue, as it only changes the ->read and
->write pointers from u8_(reader|writer) to u32_(reader|writer), i
certified that
the u32 functions are being used, this is enough for me.
It seems that checking the full count of receive/transmit bytes that
_gets_in/out_the_FIFO_ is not enough to assert that the data is actually
_transfered_to_the_hardware_.


I will try to explain how i came to this patch a little more verbose, see if
agree with me now:

do {
      if (drv_data->read(drv_data)) {
         int_transfer_complete(drv_data);
         return IRQ_HANDLED;
      }
   } while (drv_data->write(drv_data));

So, if i use u32_writer/u32_reader (t.bits_per_word = 32)
to write/read a _single_ 32bits word (t.len = 4),
the relevant code path on the above loop would be:

first u32_reader call:
read_SSSR(reg) & SSSR_RNE /* FIFO is still empty, u32_reader returns 0 */

first u32_writer call:
write_SSDR(*(u32 *)(drv_data->tx), reg);
drv_data->tx += 4;
/* data is transfered to FIFO, writer returns 1 */

second u32_reader call:
*(u32 *)(drv_data->rx) = read_SSDR(reg);
drv_data->rx += 4;
/* u32_reader doesnt loop becase rx == rx_end, reader returns 1 */

then it calls int_transfer_complete.

I dont see where it asserts that the hardware transfer was done. Or
read_SSDR/write_SSDR do blocking???

Shouldnt the driver wait SSSR_RNE be cleared before assuming that the
data was received?
while(read_SSSR(reg) & SSSR_RNE) ;

Shouldnt it wait SSSR_TNF be set, before assuming the data was written?
while(!(read_SSSR(reg) & SSSR_TNF)) ;

Shouldnt it check if the SSP port is not doing IO (SSSR_BSY) before
calling int_transfer_done?
while(read_SSSR(reg) & SSSR_BSY) ;

Any of the above 3 blocking loops on the first line of
int_transfer_complete() fixes the problem. :)
(yes, TNF works too, because threshold = 1. ;) )

And finally, note that ssp_rx_stall() doesnt reads/discards any _data_
from the port as you claimed,
it is just a busy loop, equivalent to the third (SSSR_BSY) example above.
And it IS called on dma_transfer_complete (by the way, when i
configure the driver to use DMA,
everything works as it should).

-- 
Daniel Ribeiro

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

[Ned Forrester]

Daniel Ribeiro wrote:
> On Mon, Aug 18, 2008 at 5:47 PM, Ned Forrester <nforrester-/d+BM93fTQY@public.gmane.org> wrote:
>> So far this looks good, but what have you declared about the chip in the
>> chip_info structure?  In particular, what timeout and FIFO thresholds?
>> What processor are you using, and which port on that processor; there is
>> a difference in the handling of the end of a transfer between the SSP
>> port on an PXA25x (which has no timeout interrupt) and all other ports
>> handled by this driver (including the NSSP port on the PXA25x and all
>> the ports on the PXA270).
> 
> .tx_threshold = 1,
> .rx_threshold = 1,
> .timeout = 1000,

The timeout is short (but may not be a problem for you, with only one
word transfers); we are having that discussion in another thread.  The
original author of this driver, Stephen Street, intended for the 1000
default timeout to be 1000usec.  Later we discovered that the
developer's manuals for the PXA25x and PXA270 processors do not specify
what frequency is being counted by the timer.  I have shown that on the
PXA255 it is counting 99.5MHz, but since both Stephen and I are using
PXA255, we do not know what clock is used on the PXA270 or the newer
PXA3xx.  Perhaps you could set up a test to answer that for the PXA 270,
but I digress.

At 99.5MHZ, a timer count of 1000 is only 10usec.  If the PXA270 uses
the same clock, you may be getting a timer interrupt before the hardware
even has time to clock out the bits, if you use a slow SSPSCLK.  I use a
value of 10000 with DMA driven transfers at 11MHZ, but I never want a
timeout to occur (my data flow is continuous, a timeout is an error); a
value of 100000 is more reasonable for PIO of large transfers, unless
you don't mind extra timeout interrupts that might occur when there are
latencies in processing (both PIO and DMA modes should be able to deal
with this, though in different ways).

I'm not sure that this has anything to do with your problem, though,
because with 1 word transfers, it should not matter if the single word
read is read by the read-while-transmit loop of int_transfer, or if it
is read (more commonly) by the timeout loop (higher in the same
routine); nor should it matter if the timeout interrupt is invoked more
than once to check the receiver fifo. The one data word should be there
already, or the driver will wait for the another timeout (the timer is
only stopped in int_transfer_complete, and that is only called after all
data is received).

Note that if the timeout interrupt occurs before the last (or in your
case, only) word arrives in the RX FIFO, then the timer will later be
reset when the word actually does arrive, and will thus timeout again.
There is a possible race condition that is correctly handled in the
driver by clearing the SSSR_TINT bit BEFORE checking for words in the
FIFO.  Otherwise, if the TINT bit were cleared afterward, the data word
might arrive after checking for data and before clearing the TINT bit,
thus setting up a situation where the timeout interrupt never gets set
again.

Hmmm... I guess all of the above is just me re-learning how the timeout
works, and is thus just for background information.  It looks like it
does not matter that much if the timeout is short or long: if it is too
short, there will be wasted interrupt services with nothing important to
do, and if it is too long then transfers are delayed unnecessarily, but
the data still flows.

> Its a PXA270, SSP1.
> 
>>> pcap_read: reg_num:3 value:00edf48a
>>> pcap_write: reg_num:3 value:00123456
>>> pcap_read: reg_num:3 value:00edf48a
>>> pcap_read: reg_num:3 value:00edf48a
>>>
>>> With the patch, or with an udelay(300) call instead of wait_ssp_rx_stall:
>>> pcap_read: reg_num:3 value:00edf48a
>>> pcap_write: reg_num:3 value:00123456
>>> pcap_read: reg_num:3 value:00123456
>>> pcap_read: reg_num:3 value:00123456
>>>
>>> Since the udelay(300) call works, and it doesnt touch the ssp registers,
>>> it looks like int_transfer_complete is being called before it should.
>> Very strange, but I can't draw the same conclusion.
>> int_transfer_complete is only called from 4 places in int_transfer, and
>> then only after verifying that the full count of receive characters has
>> been transfered to memory.  Even if the transfer was not complete in
>> hardware, or if the CS was dropped too soon, this delay would not fix
>> the problem.  There has to be something else going on.
>> I am not familiar with the chip you are using.  Are you certain that
>> when you write a register, you should expect back the same value that
>> you wrote, when received on the same transfer.  It makes sense that the
>> chip might do this, but I thought I should ask, because there are other
>> possibilities.
> 
> Its not the same transfer. pcap_read and pcap_write each call pcap_putget.
> So, to _write_a_register_ i write then read a spi word. And to _read_a_register_
> i write then read another spi word. Those write/read register operations canŽt
> happen on the same CS assertion.

Ahhh...  Sorry for the confusion.  I thought the above results were
supposed to show what was received during the receive portion of the
same transfer that was doing the writing.  No wonder we are talking at
cross purposes.  So, if I now understand, the write is simply failing,
and subsequent reads show the data unchanged from before the write.

>> One thing that is odd about the setup of your message is the use of the
>> same buffer for transmit and receive.  I agree that this should be
>> possible, as nothing is received until the corresponding word has been
>> transmitted.  However, this might be obscuring some of the symptoms.
>> Can you try assigning separate buffers for tx and rx, and then test to
>> see what the results are with and without the added delay?  This might
>> shed some more light on what is happening.
> 
> I tried this already, used different buffers, and it makes no difference, same
> wrong behaviour without the delay, and the same correct behaviour with the
> delay. The fact of using the same buffer is not the issue.

OK. Thanks for the info.

>> Also try setting up the pxa2xx_spi driver to the correct bits-per-word
>> though chip_info and spi_setup(), and turn off (make 0) the per-transfer
>> bits-per-word, above.  This will test to see if there is an error
>> handling the per-transfer changes.  I don't think many applications use
>> the per-transfer changes, so that is another possible source of problems.
> 
> Same. This doesnt seem to be the issue, as it only changes the ->read and
> ->write pointers from u8_(reader|writer) to u32_(reader|writer), i
> certified that
> the u32 functions are being used, this is enough for me.

Well, the u32 functions will be used regardless of whether you specify
32 bits/word as the default (through setup) or whether you specify it
per-transfer.  In any case, you tested this also, and it is ok.

> It seems that checking the full count of receive/transmit bytes that
> _gets_in/out_the_FIFO_ is not enough to assert that the data is actually
> _transfered_to_the_hardware_.

Now that I realize that you are talking about separate write and read
transfers, I will try to explain in that context.  Note that we are
discussing PIO mode below, DMA has similar but different considerations.

Checking the received bytes is a perfect measure of bytes transmitted.
Every SPI transfer, whether one word or many, and whether intended for
the purpose of writing or reading a chip, consists of both transmitting
and receiving.  It is the transmit that causes SSPSCLK to be generated,
and that simultaneously shifts out the transmit data and shifts in the
receive data.  Thus the RX fifo cannot register a received word until
all the clocks have occurred, and all data must be shifted out, and also
all data shifted in.

However, that does not say that the correct thing happened with the chip
select, just that there is a clock pulse on the wire for every bit of
the transfer.

> I will try to explain how i came to this patch a little more verbose, see if
> agree with me now:
> 
> do {
>       if (drv_data->read(drv_data)) {
>          int_transfer_complete(drv_data);
>          return IRQ_HANDLED;
>       }
>    } while (drv_data->write(drv_data));
> 
> So, if i use u32_writer/u32_reader (t.bits_per_word = 32)
> to write/read a _single_ 32bits word (t.len = 4),
> the relevant code path on the above loop would be:
> 
> first u32_reader call:
> read_SSSR(reg) & SSSR_RNE /* FIFO is still empty, u32_reader returns 0 */

Yes.

> first u32_writer call:
> write_SSDR(*(u32 *)(drv_data->tx), reg);
> drv_data->tx += 4;
> /* data is transfered to FIFO, writer returns 1 */

Yes.

> second u32_reader call:
> *(u32 *)(drv_data->rx) = read_SSDR(reg);
> drv_data->rx += 4;
> /* u32_reader doesnt loop becase rx == rx_end, reader returns 1 */

Not necessarily.  The most likely action at this point is that
u32_reader will find that the RX FIFO is still empty and thus will
return without actually reading; it takes 2.4us to clock out 32 bits at
the maximum clock rate of 13MHz (I think that rate is possible on a
PXA270).  All those extra reads are there, not necessarily to read
anything, but make sure, if anything is in the RX FIFO, that it is read
out to make room for whatever has been put in the TX FIFO.  Without all
these attempted reads, some of which might succeed, it is possible, with
higher TX and RX FIFO thresholds, to overrun the RX FIFO by having more
words in the TX FIFO then there are spaces in the RX FIFO.

At this point, in your case of a one word transfer, the int_transfer
notes that all words have been placed in the TX FIFO and thus disables
the TX Interrupt (TIE).  Then int_transfer returns from interrupt
service, and the processor waits for the timeout interrupt.

When the timeout interrupt occurs, the receiver will be emptied by the
earlier code in int_transfer, at: if (irq_status & SSSR_TINT) {

Then...

> then it calls int_transfer_complete.

Yes, after all the the expected characters have been received, whether
or not that happened during one call to int_transfer or during a second
call responding to the timeout interrupt.

> I dont see where it asserts that the hardware transfer was done. Or
> read_SSDR/write_SSDR do blocking???

u32_reader will not return true unless it has received all bytes, and it
will not receive bytes while the RX fifo is empty.  Thus the driver
blocks in the sense of return from interrupt, and then waits until there
is an interrupt (timeout), so that it can check the receiver again.

> Shouldnt the driver wait SSSR_RNE be cleared before assuming that the
> data was received?
> while(read_SSSR(reg) & SSSR_RNE) ;

It has to wait for SSSR_RNE to be SET to read data, and it does.  If
there are no hardware errors (clock glitches or whatever), then the
receiver cannot have words in it after the last expected one is read
out; none transmitted, none received.  At that point the receiver must
be empty; there is no need to check.

> Shouldnt it wait SSSR_TNF be set, before assuming the data was written?
> while(!(read_SSSR(reg) & SSSR_TNF)) ;

No.  If all data has been received, then, by definition, it was all
transmitted.  The receiver needs a clock to receive, and the clock is
enabled by transmit.  In any case, I don't think you are interpreting
SSSR_TNF correctly.  It is only clear if there are 16 words in the FIFO,
the setting of the threshold is not related to this bit.

Incidentally, in the writers, the actual fifo count is checked for 15,
because the logic of preventing receiver overruns requires that we never
put more than 15 in the transmit fifo; that is, we never want it full
and SSSR_TNF will never be clear.  The loop above will never evaluate
more than once. There must be a delay associated with reading SSSR.  I
don't know what that delay would be, but you may find out, below.

> Shouldnt it check if the SSP port is not doing IO (SSSR_BSY) before
> calling int_transfer_done?
> while(read_SSSR(reg) & SSSR_BSY) ;

That is not necessary.  All data has been clocked.

> Any of the above 3 blocking loops on the first line of
> int_transfer_complete() fixes the problem. :)
> (yes, TNF works too, because threshold = 1. ;) )

See above about TNF.  So it is not the value of TNF, but perhaps the
time spent reading it that matters.  I would be interested to know if
putting a printk inside the above while loops ever indicates that they
were evaluated more than once.

The first of your three is effectively part of the readers, you can't
get to int_transfer_complete without satisfying it, so I don't accept
your point.  But keep reading....

> And finally, note that ssp_rx_stall() doesnt reads/discards any _data_
> from the port as you claimed,
> it is just a busy loop, equivalent to the third (SSSR_BSY) example above.
> And it IS called on dma_transfer_complete (by the way, when i
> configure the driver to use DMA,
> everything works as it should).

My apologies.  I misread read_SSSR(ioaddr) as read_SSDR(ioaddr).  It has
been two years since my first descent into the details of this driver,
and I have not revisited wait_ssp_rx_stall recently.  I should not
operate on memory at my age.

Some of the extra work done in dma_transfer complete, prior to clearing
chip select (delay), *might* account for why it works and
int_transfer_complete does not work.

--

It remains to figure out why your fixes help, and what the real problem is.

I think it may go back to your original statement: "I did some tests,
and it looks like CS is being deasserted before the IO is done."

I gather you can scope these signals and measure the timing of CS as it
relates to clock and data.  What does the chip spec say about the
required relationship between CS and the other signals? I gather that
some chips require CS to stay asserted briefly after the end of the
clocks; this is the reason for having spi_transfer.delay_usecs.  Please
measure the delay from the last clock to dropping CS and compare that
with the requirements of the chip, with and without the changes you
tested above.  Does this shed light on the problem?

If the insertion of the register reads you tested above helped (they may
have added 100ns to the time CS is active after the last clock) then you
have stumbled on a known bug in the driver, that had not yet been fixed.
 This bug regarding CS changes was discussed in:

Re: [spi-devel-general] [PATCH] atmel_spi: support zero length transfer

See David's reply of: 2008-02-23 02:37
I raised the question but, since I don't use chip select at all, I have
not produced a patch.  There are other issues discussed there about zero
length transfers in DMA mode, and about doing DMA mapping in transfer(),
rather than in pump_transfers(), just in case you have time on your
hands. :-)

Basically, the spi_transfer.delay_usecs value is supposed to be applied
between the end of a transfer and any change in CS specified by a
spi_transfer.cs_change request.  In this driver, the order is mistakenly
reversed, and the delay is honored AFTER the CS has already changed.
Thus if your chip requires a delay between the end of a transfer and the
change in CS, and you set delay_usecs to request that, it will not be
honored as intended.

-- 
Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

[Daniel Ribeiro]

Hi!

You were right, there is nothing wrong with the spi transfer.

The problem is the hardware. This chip really needs some delay before
deasserting CS on register writes. Otherwise it just silently ignores
the register write.

I no longer think that anything is wrong with spi communication, i can
reliably read any chip register. The problem only occurs on register
writes, and not even on all registers.

On giveback(), if i add a delay _before_ cs_control(CS_DEASSERT), it works.
If i add it just _after_ cs_control() it does not work.

I see that there is a .delay_usecs field on spi_transfer:

 * @delay_usecs: microseconds to delay after this transfer before
 *      (optionally) changing the chipselect status, then starting
 *      the next transfer or completing this @spi_message.

But it is only called for the previous transfer when on RUNNING_STATE,
so a second spi_transfer is needed on the same spi_message to actually
use this feature.

Should i submit a patch to use this delay_usecs field also on
DONE_STATE, so it actually do something when set on the last
spi_transfer? Or should i just cheat pxa2xx_spi with:

        struct spi_transfer t[2];
        struct spi_message m;

        spi_message_init(&m);
        t[0].len = 4;
        t[0].tx_buf = (u8 *)data;
        t[0].rx_buf = (u8 *)data;
        t[0].bits_per_word = 32;
        t[0].delay_usecs = 300;
        spi_message_add_tail(&t[0], &m);

        /* trick pxa2xx_spi so it uses t[0].delay_usecs */
        t[1].len = 0;
        spi_message_add_tail(&t[1], &m);

        return spi_sync(pcap.spi, &m);

(with the above code my chip works great without changes to pxa2xx_spi.c)

My opinion is that this field should be used to delay also on
DONE_STATE: "microseconds to delay after this transfer before changing
the chipselect status, then starting the next transfer
*or*completing*this*spi_message*"

I will be happy to submit a patch to change this if you think the
delay on DONE_STATE is acceptable.

-- 
Daniel Ribeiro

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

[Ned Forrester]

Daniel Ribeiro wrote:
> Hi!
> 
> You were right, there is nothing wrong with the spi transfer.
> 
> The problem is the hardware. This chip really needs some delay before
> deasserting CS on register writes. Otherwise it just silently ignores
> the register write.
> 
> I no longer think that anything is wrong with spi communication, i can
> reliably read any chip register. The problem only occurs on register
> writes, and not even on all registers.
> 
> On giveback(), if i add a delay _before_ cs_control(CS_DEASSERT), it works.
> If i add it just _after_ cs_control() it does not work.
> 
> I see that there is a .delay_usecs field on spi_transfer:
> 
>  * @delay_usecs: microseconds to delay after this transfer before
>  *      (optionally) changing the chipselect status, then starting
>  *      the next transfer or completing this @spi_message.
> 
> But it is only called for the previous transfer when on RUNNING_STATE,
> so a second spi_transfer is needed on the same spi_message to actually
> use this feature.
> 
> Should i submit a patch to use this delay_usecs field also on
> DONE_STATE, so it actually do something when set on the last
> spi_transfer? Or should i just cheat pxa2xx_spi with:
> 
>         struct spi_transfer t[2];
>         struct spi_message m;
> 
>         spi_message_init(&m);
>         t[0].len = 4;
>         t[0].tx_buf = (u8 *)data;
>         t[0].rx_buf = (u8 *)data;
>         t[0].bits_per_word = 32;
>         t[0].delay_usecs = 300;
>         spi_message_add_tail(&t[0], &m);
> 
>         /* trick pxa2xx_spi so it uses t[0].delay_usecs */
>         t[1].len = 0;
>         spi_message_add_tail(&t[1], &m);
> 
>         return spi_sync(pcap.spi, &m);
> 
> (with the above code my chip works great without changes to pxa2xx_spi.c)
> 
> My opinion is that this field should be used to delay also on
> DONE_STATE: "microseconds to delay after this transfer before changing
> the chipselect status, then starting the next transfer
> *or*completing*this*spi_message*"
> 
> I will be happy to submit a patch to change this if you think the
> delay on DONE_STATE is acceptable.

Wow! Our messages crossed in the ether.  I'm glad we agree now on the
nature of the problem.  Yes, I think that delay_usecs should be honored
on every transfer on which it is non-zero.  As noted in my previous
email, there is a known bug in the driver in this regard, and if you
would like to submit a patch for it, that would be great.  Exactly what
that patch should look like, I'm not sure at the moment.  It is very
late where I live and I would prefer to consider this tomorrow.  I would
like to suggest/review this sort of patch, to make sure it is as
compatible as possible with my unreleased expansion of this driver.

I'm still curious to know how much delay was added by the various
patches you tested.  My hardware is not available at the moment, so I
cannot test, and I would be testing a PXA255, in any case.

-- 
Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

[Ned Forrester]

Daniel Ribeiro wrote:
> Hi!
> 
> You were right, there is nothing wrong with the spi transfer.
> 
> The problem is the hardware. This chip really needs some delay before
> deasserting CS on register writes. Otherwise it just silently ignores
> the register write.
> 
> I no longer think that anything is wrong with spi communication, i can
> reliably read any chip register. The problem only occurs on register
> writes, and not even on all registers.
> 
> On giveback(), if i add a delay _before_ cs_control(CS_DEASSERT), it works.
> If i add it just _after_ cs_control() it does not work.
> 
> I see that there is a .delay_usecs field on spi_transfer:
> 
>  * @delay_usecs: microseconds to delay after this transfer before
>  *      (optionally) changing the chipselect status, then starting
>  *      the next transfer or completing this @spi_message.
> 
> But it is only called for the previous transfer when on RUNNING_STATE,
> so a second spi_transfer is needed on the same spi_message to actually
> use this feature.

It is true that the delay is implemented only for the case of a message
continuing with another transfer (which is wrong), but also that the
delay is implemented after the CS change that occurs in either
int_transfer_complete or dma_transfer_complete.  Because of this, your
code fragment below works, not because you made the delay_usecs
parameter non-zero, but because you did not make the cs_change parameter
true.  Thus, when the double transfer is executed, the default behavior
of the driver is (correctly) to leave CS asserted through the next
transfer; the delay_usecs parameter adds additional delay, but I bet
that just executing the next zero-length transfer was sufficient delay.

The real problem is that either the delay or the call to deassert CS is
implemented in the wrong place.  It needs to be invoked prior to each
test and call of the cs_change parameter.  There are 3 places where this
could be done: giveback, dma_transfer_complete, and
int_transfer_complete.  By the time pump_transfers is executed, the CS
change, if any, has already taken place, and the next transfer has
already been selected so that pump_transfers has to reach back to
"previous" to fetch the delay, kind of awkward.

The down side of putting the delay in the logical place (before each
test/call for cs_change) is that it moves the udelay() call from soft
interrupt context (pump_transfers executing as a tasklet) to hard
interrupt context.  This is probably a bad idea.  See alternate below.

> Should i submit a patch to use this delay_usecs field also on
> DONE_STATE, so it actually do something when set on the last
> spi_transfer? Or should i just cheat pxa2xx_spi with:
> 
>         struct spi_transfer t[2];
>         struct spi_message m;
> 
>         spi_message_init(&m);
>         t[0].len = 4;
>         t[0].tx_buf = (u8 *)data;
>         t[0].rx_buf = (u8 *)data;
>         t[0].bits_per_word = 32;
>         t[0].delay_usecs = 300;
>         spi_message_add_tail(&t[0], &m);
> 
>         /* trick pxa2xx_spi so it uses t[0].delay_usecs */
>         t[1].len = 0;
>         spi_message_add_tail(&t[1], &m);
> 
>         return spi_sync(pcap.spi, &m);
> 
> (with the above code my chip works great without changes to pxa2xx_spi.c)

Agreed, but the driver still needs to be fixed because it does not
conform to the standard of the SPI core.

Interestingly, you have also stressed another known weakness in the
driver that was the main topic of the thread I pointed you to last
night: that the driver is not guaranteed to handle zero length transfers
properly.  That problem should be fixed at the same time.  One reason
that I have not fixed these known problems lack of time, but the other
reason is that I have no easy way to test them.  Now that there is a
willing tester, it is time to write a patch.

> My opinion is that this field should be used to delay also on
> DONE_STATE: "microseconds to delay after this transfer before changing
> the chipselect status, then starting the next transfer
> *or*completing*this*spi_message*"

I agree the driver needs a patch, but I think the right way to do it is
as follows:

1. Add a check for non-zero delay_usecs and resulting call to udelay in
giveback(), just before the test for cs_change and call to cs_control.
giveback() is executed in tasklet context (soft interrupt) and so this
will be no worst than the current udelay call.

2. Move the test for cs_change and call to cs_control out of both of
int_transfer_complete and dma_transfer complete, and put a single
test/call based on the "previous" pointer within the RUNNING_STATE test
of pump_transfers, and just after the test/execution of the delay.  This

3. Fix any issues with the execution of zero-length transfers.  (I still
have to look at that, but I think that only zero-length, non-descriptor
DMA fails; the current driver does not use descriptor DMA, but my
version does).

4. See if the driver has to handle transfers longer than 8191 bytes.  I
have this question open with David Brownell, and I await an answer, so
there may be action needed with that, though it could be implemented
separately at a later time.

> I will be happy to submit a patch to change this if you think the
> delay on DONE_STATE is acceptable.

Given that there is a bit more to this, and that we should try to make
this apply to earlier stable kernels, maybe I should try to write the
patch.  I will get back, hopefully today, with something.

-- 
Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

[David Brownell]

Catching up on old mail ...

On Monday 18 August 2008, Daniel Ribeiro wrote:
> As I already said, i am converting a working driver from ssp.c´s
> ssp_read_word/ssp_write_word to pxa2xx_spi. The driver works just
> fine with:
> 
> gpio_set_value(cs, 0);
> ssp_write_word(&ssp_dev, data);
> ssp_read_word(&ssp_dev, &ret);
> gpio_set_value(cs, 1);

That is, 64 bit times ... two sequential word transfers.


> After the conversion i ended with this code:
>
> static int ezx_pcap_putget(u32 *data)
> {
>         struct spi_transfer t;
>         struct spi_message m;
> 
>         spi_message_init(&m);
>         t.len = 4;
>         t.tx_buf = (u8 *)data;
>         t.rx_buf = (u8 *)data;
>         t.bits_per_word = 32;

That is, 32 bit times ... two *parallel* word transfers.


>         spi_message_add_tail(&t, &m);
>         return spi_sync(pcap.spi, &m);
> }

So "deasserting CS too early" was what you *requested* and
it's no wonder the driver did just that.  

You could instead

    return spi_write_then_read(pcap.spi, data, 4, data, 4);

to get the sequential (not concurrent!) I/O you need.  Be
careful of byte ordering, of course; you probably should
set spi->bits_per_word to 32 and then spi_setup(), as part
of your driver init.

- Dave


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

[Daniel Ribeiro]

Em Qui, 2008-09-11 às 10:41 -0700, David Brownell escreveu:
> You could instead
> 
>     return spi_write_then_read(pcap.spi, data, 4, data, 4);

Tried this already. data == 0x00000000.
Anyway, after the latest chipselect fixes, (deassert CS on
pump_transfer() || giveback() instead of int_transfer_complete()) the
code is working.

> to get the sequential (not concurrent!) I/O you need.  Be
> careful of byte ordering, of course; you probably should
> set spi->bits_per_word to 32 and then spi_setup(), as part
> of your driver init.

Yes, i guess i really should use spi_setup().
My current code is:

        spi_message_init(&m);
        t.len = 4;
        t.tx_buf = (u8 *)data;
        t.rx_buf = (u8 *)data;
        t.bits_per_word = 32;
        t.delay_usecs = 0;
        t.cs_change = 0;
        t.speed_hz = 13000000;
        spi_message_add_tail(&t, &m);
        return spi_sync(pcap.spi, &m);

I noted that if i dont set .delay_usecs it gets a default huge value,
same with .speed_hz, it dont use spi_board_info->max_speed_hz as
default. Would calling spi_setup() on my driver init solve this? Or is
this yet another issue?

Thanks for your reply.

--
Daniel Ribeiro



-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Re: [PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

[Ned Forrester]

Daniel Ribeiro wrote:
> Em Qui, 2008-09-11 Ã s 10:41 -0700, David Brownell escreveu:
> Yes, i guess i really should use spi_setup().
> My current code is:
> 
>         spi_message_init(&m);
>         t.len = 4;
>         t.tx_buf = (u8 *)data;
>         t.rx_buf = (u8 *)data;
>         t.bits_per_word = 32;
>         t.delay_usecs = 0;
>         t.cs_change = 0;
>         t.speed_hz = 13000000;
>         spi_message_add_tail(&t, &m);
>         return spi_sync(pcap.spi, &m);
> 
> I noted that if i dont set .delay_usecs it gets a default huge value,
> same with .speed_hz, it dont use spi_board_info->max_speed_hz as
> default. Would calling spi_setup() on my driver init solve this? Or is
> this yet another issue?

In a past message, you showed the automatic declarations that went with
the above code:

>	struct spi_transfer t;
>	struct spi_message m;

While spi_message_init() zeros the memory in "m", you don't show any
code above, other than your assignments, that clears the memory in "t".
Thus, if you omit the assignments for t.delay_usecs or t.speed_hz (or
any other member of t), then you are passing uninitialized memory.  You
could use memset to clear t, or dynamically allocate zeroed memory.

Normally, you use the per-transfer values:

	t.bits_per_word
	t.speed_hz

only when you need to make them different from the default values set
when you previously called spi_setup(), and the values of:

	t.delay_usecs
	t.cs_change

when you need control over the changes in CS between transfers, and
optionally a delay prior to changes in CS.  Errors in handling these
last two were addressed by the patch you tested, and which David pushed
recently.

-- 
Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode

[Ned Forrester]

Ned Forrester wrote:
> Daniel Ribeiro wrote:
> In a past message, you showed the automatic declarations that went with
> the above code:
> 
> >	struct spi_transfer t;
> >	struct spi_message m;
> 
> While spi_message_init() zeros the memory in "m", you don't show any
> code above, other than your assignments, that clears the memory in "t".
> Thus, if you omit the assignments for t.delay_usecs or t.speed_hz (or
> any other member of t), then you are passing uninitialized memory.  You
> could use memset to clear t, or dynamically allocate zeroed memory.
> 
> Normally, you use the per-transfer values:
> 
> 	t.bits_per_word
> 	t.speed_hz
> 
> only when you need to make them different from the default values set
> when you previously called spi_setup()

I forgot to mention that if either of these values is zero, then the
default parameters set by initialization or spi_setup are used.  If
either value is non-zero, then the defaults are overridden.  In your
case, the uninitialized memory overrode the defaults.

-- 
Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/