Re: pxa2xx_spi with SFRM

Re: pxa2xx_spi with SFRM

[nforrester-/d+BM93fTQY]

I am as sea right now, with very limited email access.  If you can
wait until Wed or Thursday of next week, I might be of more help.  In
the mean time perhaps someone else will reply.

Since the orignal author, Stephen Steet, became silent on this driver,
I and several others have submitted patches for it.  I use an
extensivley modified version of pxa2xx_spi.c for one of my projects
and thus I have some familiarity with the driver, though somewhat less
with the details of SPI (I use the TI synchonous mode) and none at all
with flash on SPI.  I also work on a PXA255, and the PXA270 has some
differences.

> I am using a custom PXA270 board and tried to use the SSP port to
> communication with a SPI Flash. After reading docs on how to configure
> the spi_master and spi devices, I have the device driver's probe being
> called. But I cannot get the pxa2xx_spi driver to work correctly. I
> can connect either a SD card or a M26P16 Flash chip to the SPI port on
> my board. Neither device driver can completely operate their device.
>
> It looks like it is impossible for the device drivers to control the
> chip select (CS) line. If I use the manual cs_control callback, the
> timing is invalid because the SSP clock keeps running.

The cs_control() callback is the normal way of controlling chip select
with this driver.  That allows you to assign any GPIO pin or polarity.
I understand that this differs from way that the spi core expects chip
select to be handeled; in particular, the driver does not honor the
SPI_CS_HIGH nor SPI_LSB_FIRST bits in the spi_device.mode.  The PXA
SSP controllers do not have a dedicated chipselect mechanism
associated with the SSP controllers (outside of the single FRAME pin,
which may or may not do what you want), thus the use of GPIO lines is
required.

What version of the kernel/driver are you using?  There have been some
recent changes, and there may still be a bug regarding CS changes that
was discussed in:
Re: [spi-devel-general] odd behaviour of chipselect with bitbang driver?
on about 04/24/08 15:55.  I raised the question but, since I don't use
chipselect at all, I have not produced a patch.

The fact that the clock free runs strikes me as strange, I don't think
any of the supported SPI_MODE_n combinations can produce a free
running clock, though they do control polarity and phase of the clock.
If your problem involves one extra clock edge, you may have the wrong
setting for SPI_MODE_n.`

> That means that
> several bits are clocked out before the SSP controller starts to drive
> the TX line correctly. If the SFRM signal is used, it does not allow
> the driver to keep CS active for multiple transactions as they expect
> and assume. The spi_sync call takes an spi_message which contains a
> list of transactions to send. Normally, the device will need the CS
> active during the complete message. But the SSP controller deactivates
> SFRM when it is done with each buffer. For instance, the M25P16
> datasheet indicates that CS has to stay active from the READ command
> through the complete data transfer. When it goes high, it resets the
> command interface.

The driver should honor the per-transfer spi_transfer.cs_change flag
when used with a cs_control routine.  If you want the CS to stay
asserted between transfers, be sure the cs_change flag is 0.

> It looks like the SSP port needs to have a register bit to turn off
> the clock when there is nothing to transmit. Or a bit to tell the SSP
> controller to leave CS active. Or I can change the HW so CS gates the
> clock and a manual CS chip operates the device CS pin. I might also
> try the bitbanging SPI driver next to see if that can operate with the
> Flash chip.

pxa2xx_spi.c uses format SSCR0_Motorola.  I think, as I said above,
that this mode does not allow the clock to free run.  I expect that
you have some other problem.  As I understand it (and I may be wrong,
since I don't use Motorola SPI format) the clock is only active if
data is being transfered, and data is only transferred if there is
something the in the transmit FIFO to go out (and thus something to
receive at the same time).  I wonder if you have a zero byte in the TX
FIFO when you think you have nothing.  Perhaps spi_transfer.len is not
set right.

> Is there some other setting I am missing here? Is anyone else using
> the M25P16 chip with PXA270 SPI driver?



----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.


-------------------------------------------------------------------------
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: pxa2xx_spi with SFRM

[Jonathan Cameron]

nforrester-/d+BM93fTQY@public.gmane.org wrote:
> I am as sea right now, with very limited email access.  If you can
> wait until Wed or Thursday of next week, I might be of more help.  In
> the mean time perhaps someone else will reply.
> 
> Since the orignal author, Stephen Steet, became silent on this driver,
> I and several others have submitted patches for it.  I use an
> extensivley modified version of pxa2xx_spi.c for one of my projects
> and thus I have some familiarity with the driver, though somewhat less
> with the details of SPI (I use the TI synchonous mode) and none at all
> with flash on SPI.  I also work on a PXA255, and the PXA270 has some
> differences.
I can offer a few additional comments on this (as a PXA271 spi user)

>> I am using a custom PXA270 board and tried to use the SSP port to
>> communication with a SPI Flash. After reading docs on how to configure
>> the spi_master and spi devices, I have the device driver's probe being
>> called. But I cannot get the pxa2xx_spi driver to work correctly. I
>> can connect either a SD card or a M26P16 Flash chip to the SPI port on
>> my board. Neither device driver can completely operate their device.
>>
>> It looks like it is impossible for the device drivers to control the
>> chip select (CS) line. If I use the manual cs_control callback, the
>> timing is invalid because the SSP clock keeps running.
> 
> The cs_control() callback is the normal way of controlling chip select
> with this driver.  That allows you to assign any GPIO pin or polarity.
> I understand that this differs from way that the spi core expects chip
> select to be handeled; in particular, the driver does not honor the
> SPI_CS_HIGH nor SPI_LSB_FIRST bits in the spi_device.mode.  The PXA
> SSP controllers do not have a dedicated chipselect mechanism
> associated with the SSP controllers (outside of the single FRAME pin,
> which may or may not do what you want), thus the use of GPIO lines is
> required.
> 
> What version of the kernel/driver are you using?  There have been some
> recent changes, and there may still be a bug regarding CS changes that
> was discussed in:
> Re: [spi-devel-general] odd behaviour of chipselect with bitbang driver?
> on about 04/24/08 15:55.  I raised the question but, since I don't use
> chipselect at all, I have not produced a patch.
> 
> The fact that the clock free runs strikes me as strange, I don't think
> any of the supported SPI_MODE_n combinations can produce a free
> running clock, though they do control polarity and phase of the clock.
> If your problem involves one extra clock edge, you may have the wrong
> setting for SPI_MODE_n.`
Although the clock can free run in some of the other (non spi modes)
the driver doesn't support this which means the behaviour is somewhat
odd!  Any chance you can make available a picture of an oscilloscope trace
showing SFRM and the clock?  That'll probably give sufficient clues to
allow us to figure out what is going wrong.  I've used gpio based chip
selects on the pxa271 wihout problems (though it does tend to slow down
your transfers somewhat so is best avoided where possible).
>> That means that
>> several bits are clocked out before the SSP controller starts to drive
>> the TX line correctly. If the SFRM signal is used, it does not allow
>> the driver to keep CS active for multiple transactions as they expect
>> and assume. The spi_sync call takes an spi_message which contains a
>> list of transactions to send. Normally, the device will need the CS
>> active during the complete message. But the SSP controller deactivates
>> SFRM when it is done with each buffer. For instance, the M25P16
>> datasheet indicates that CS has to stay active from the READ command
>> through the complete data transfer. When it goes high, it resets the
>> command interface.
> 
> The driver should honor the per-transfer spi_transfer.cs_change flag
> when used with a cs_control routine.  If you want the CS to stay
> asserted between transfers, be sure the cs_change flag is 0.

Indeed it should but it doesn't.  Unfortunately the hardware doesn't
seem to have the ability to do this, so it will only work if using
a gpio as the chip select.  Guess we really ought to add some stuff to
the docs to make this clear (it wasted a good few hours of my time
failing to understand why it didn't do anything!)  For short
transfers (sub 16 x 32bits) you can get away with doing the endianess
changes necessary and sending as a single multiword transfer.
Afraid I'm usually dealing with sensors, adc's etc and they don't
tend to need larger transfers than this between deasserting the chip
select.
 
>> It looks like the SSP port needs to have a register bit to turn off
>> the clock when there is nothing to transmit. Or a bit to tell the SSP
>> controller to leave CS active. Or I can change the HW so CS gates the
>> clock and a manual CS chip operates the device CS pin. I might also
>> try the bitbanging SPI driver next to see if that can operate with the
>> Flash chip.
> 
> pxa2xx_spi.c uses format SSCR0_Motorola.  I think, as I said above,
> that this mode does not allow the clock to free run.  I expect that
> you have some other problem.  As I understand it (and I may be wrong,
> since I don't use Motorola SPI format) the clock is only active if
> data is being transfered, and data is only transferred if there is
> something the in the transmit FIFO to go out (and thus something to
> receive at the same time).

That should certainly be the case.  I guess it's just possible that SSCR1 
is preset by something else to continuous clock mode? Don't think the driver
explicitly clears it and a quick glance suggests elements of that register
are set from chip specific data.

>  I wonder if you have a zero byte in the TX
> FIFO when you think you have nothing.  Perhaps spi_transfer.len is not
> set right.
> 
>> Is there some other setting I am missing here? Is anyone else using
>> the M25P16 chip with PXA270 SPI driver?
Not that chip, but I've got a good scope wired up to spi on
a pxa271 so should be able to debug this without the chip.

--
Jonathan Cameron

-------------------------------------------------------------------------
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: pxa2xx_spi with SFRM

[Vernon Sauder]

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

First, thanks for your replies. I am sorry. I left town for the weekend 
and I forgot to include all the details.

Jonathan Cameron wrote:
> nforrester-/d+BM93fTQY@public.gmane.org wrote:
>   
>> I am as sea right now, with very limited email access.  If you can
>> wait until Wed or Thursday of next week, I might be of more help.  In
>> the mean time perhaps someone else will reply.
>>
>> Since the orignal author, Stephen Steet, became silent on this driver,
>> I and several others have submitted patches for it.  I use an
>> extensivley modified version of pxa2xx_spi.c for one of my projects
>> and thus I have some familiarity with the driver, though somewhat less
>> with the details of SPI (I use the TI synchonous mode) and none at all
>> with flash on SPI.  I also work on a PXA255, and the PXA270 has some
>> differences.
>>     
> I can offer a few additional comments on this (as a PXA271 spi user)
>   
Good. We have a nice set of different HW. Sorry to hear that Steve 
became quiet.
>   
>>> I am using a custom PXA270 board and tried to use the SSP port to
>>> communication with a SPI Flash. After reading docs on how to configure
>>> the spi_master and spi devices, I have the device driver's probe being
>>> called. But I cannot get the pxa2xx_spi driver to work correctly. I
>>> can connect either a SD card or a M26P16 Flash chip to the SPI port on
>>> my board. Neither device driver can completely operate their device.
>>>
>>> It looks like it is impossible for the device drivers to control the
>>> chip select (CS) line. If I use the manual cs_control callback, the
>>> timing is invalid because the SSP clock keeps running.
>>>       
>> The cs_control() callback is the normal way of controlling chip select
>> with this driver.  That allows you to assign any GPIO pin or polarity.
>> I understand that this differs from way that the spi core expects chip
>> select to be handeled; in particular, the driver does not honor the
>> SPI_CS_HIGH nor SPI_LSB_FIRST bits in the spi_device.mode.  The PXA
>> SSP controllers do not have a dedicated chipselect mechanism
>> associated with the SSP controllers (outside of the single FRAME pin,
>> which may or may not do what you want), thus the use of GPIO lines is
>> required.
>>
>> What version of the kernel/driver are you using?  There have been some
>> recent changes, and there may still be a bug regarding CS changes that
>> was discussed in:
>> Re: [spi-devel-general] odd behaviour of chipselect with bitbang driver?
>> on about 04/24/08 15:55.  I raised the question but, since I don't use
>> chipselect at all, I have not produced a patch.
>>
>>     
I am using 2.6.24.4 on custom HW. I am using the FRAME pin as the chip 
select so I was hoping I did not have to use cs_control().

I'll look up that patch, thanks.
>> The fact that the clock free runs strikes me as strange, I don't think
>> any of the supported SPI_MODE_n combinations can produce a free
>> running clock, though they do control polarity and phase of the clock.
>> If your problem involves one extra clock edge, you may have the wrong
>> setting for SPI_MODE_n.`
>>     
> Although the clock can free run in some of the other (non spi modes)
> the driver doesn't support this which means the behaviour is somewhat
> odd!  Any chance you can make available a picture of an oscilloscope trace
> showing SFRM and the clock?  That'll probably give sufficient clues to
> allow us to figure out what is going wrong.  I've used gpio based chip
> selects on the pxa271 wihout problems (though it does tend to slow down
> your transfers somewhat so is best avoided where possible).
>   
I attached a Logic Analyzer screen shot of m25p80 driver starting to 
query the device. The RX line is floating when the m25pxx chip is not 
driving it. But notice that the clocks are running before the FRM/CS 
line is active. If I use cs_control(), then there would be several clock 
edges (when using high speeds) before the PXA SSP controller would start 
clocking data.
>>> That means that
>>> several bits are clocked out before the SSP controller starts to drive
>>> the TX line correctly. If the SFRM signal is used, it does not allow
>>> the driver to keep CS active for multiple transactions as they expect
>>> and assume. The spi_sync call takes an spi_message which contains a
>>> list of transactions to send. Normally, the device will need the CS
>>> active during the complete message. But the SSP controller deactivates
>>> SFRM when it is done with each buffer. For instance, the M25P16
>>> datasheet indicates that CS has to stay active from the READ command
>>> through the complete data transfer. When it goes high, it resets the
>>> command interface.
>>>       
>> The driver should honor the per-transfer spi_transfer.cs_change flag
>> when used with a cs_control routine.  If you want the CS to stay
>> asserted between transfers, be sure the cs_change flag is 0.
>>     
>
> Indeed it should but it doesn't.  Unfortunately the hardware doesn't
> seem to have the ability to do this, so it will only work if using
> a gpio as the chip select.  Guess we really ought to add some stuff to
> the docs to make this clear (it wasted a good few hours of my time
> failing to understand why it didn't do anything!)  For short
> transfers (sub 16 x 32bits) you can get away with doing the endianess
> changes necessary and sending as a single multiword transfer.
> Afraid I'm usually dealing with sensors, adc's etc and they don't
> tend to need larger transfers than this between deasserting the chip
> select.
>  
>   
>>> It looks like the SSP port needs to have a register bit to turn off
>>> the clock when there is nothing to transmit. Or a bit to tell the SSP
>>> controller to leave CS active. Or I can change the HW so CS gates the
>>> clock and a manual CS chip operates the device CS pin. I might also
>>> try the bitbanging SPI driver next to see if that can operate with the
>>> Flash chip.
>>>       
>> pxa2xx_spi.c uses format SSCR0_Motorola.  I think, as I said above,
>> that this mode does not allow the clock to free run.  I expect that
>> you have some other problem.  As I understand it (and I may be wrong,
>> since I don't use Motorola SPI format) the clock is only active if
>> data is being transfered, and data is only transferred if there is
>> something the in the transmit FIFO to go out (and thus something to
>> receive at the same time).
>>     
>
> That should certainly be the case.  I guess it's just possible that SSCR1 
> is preset by something else to continuous clock mode? Don't think the driver
> explicitly clears it and a quick glance suggests elements of that register
> are set from chip specific data.
>   
Here are the registers:

root@inhand-ft4 [~] # md 0x41000000 0x3f
00000000  87 19 00 00 00 00 00 00  24 f0 00 00 00 00 00 00  |........$.......|
00000010  70 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |p...............|
00000020  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000030


>>  I wonder if you have a zero byte in the TX
>> FIFO when you think you have nothing.  Perhaps spi_transfer.len is not
>> set right.
>>
>>     
>>> Is there some other setting I am missing here? Is anyone else using
>>> the M25P16 chip with PXA270 SPI driver?
>>>       
> Not that chip, but I've got a good scope wired up to spi on
> a pxa271 so should be able to debug this without the chip.
>   
I may be able to send you a chip if you like.

I did some further research on this and found that the spidev driver 
works to at least query the flash chip. From userspace, I simply use a 
single transfer (via IOCTL) for each command. But that does not give me 
the m25p80 driver on top of the SPI; I would be re-inventing the wheel.

I also modified spi.c file to make spi_write_then_read() use one 
transfer so the m25p80 driver's JEDEC probe works. It is an example of 
the difference that would have to be done to get the pxa27x to work right.

--- ./src.old/drivers/spi/spi.c	2008-03-24 14:49:18.000000000 -0400
+++ ./src.cache/drivers/spi/spi.c	2008-08-11 17:49:50.000000000 -0400
@@ -564,7 +566,6 @@
 #define	SPI_BUFSIZ	max(32,SMP_CACHE_BYTES)
 
 static u8	*buf;
-
 /**
  * spi_write_then_read - SPI synchronous write followed by read
  * @spi: device with which data will be exchanged
@@ -592,7 +593,7 @@
 
 	int			status;
 	struct spi_message	message;
-	struct spi_transfer	x[2];
+	struct spi_transfer	x;
 	u8			*local_buf;
 
 	/* Use preallocated DMA-safe buffer.  We can't avoid copying here,
@@ -603,15 +604,9 @@
 		return -EINVAL;
 
 	spi_message_init(&message);
-	memset(x, 0, sizeof x);
-	if (n_tx) {
-		x[0].len = n_tx;
-		spi_message_add_tail(&x[0], &message);
-	}
-	if (n_rx) {
-		x[1].len = n_rx;
-		spi_message_add_tail(&x[1], &message);
-	}
+	memset(&x, 0, sizeof x);
+	x.len = n_tx + n_rx;
+	spi_message_add_tail(&x, &message);
 
 	/* ... unless someone else is using the pre-allocated buffer */
 	if (!mutex_trylock(&lock)) {
@@ -622,15 +617,15 @@
 		local_buf = buf;
 
 	memcpy(local_buf, txbuf, n_tx);
-	x[0].tx_buf = local_buf;
-	x[1].rx_buf = local_buf + n_tx;
+	x.tx_buf = local_buf;
+	x.rx_buf = local_buf;
 
 	/* do the i/o */
 	status = spi_sync(spi, &message);
 	if (status == 0)
-		memcpy(rxbuf, x[1].rx_buf, n_rx);
+		memcpy(rxbuf, x.rx_buf + n_tx, n_rx);
 
-	if (x[0].tx_buf == buf)
+	if (x.tx_buf == buf)
 		mutex_unlock(&lock);
 	else
 		kfree(local_buf);



Let me know if there is anything else I can do to clarify this. I want 
to know what the SPI is capable of and what the limitations are.

Vernon Sauder


[-- Attachment #2: Pxa270Spi.jpg --]
[-- Type: image/jpeg, Size: 60723 bytes --]

[-- 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: pxa2xx_spi with SFRM

[Ned Forrester]

Vernon Sauder wrote:
> First, thanks for your replies. I am sorry. I left town for the weekend 
> and I forgot to include all the details.
> 
> Jonathan Cameron wrote:
>> nforrester-/d+BM93fTQY@public.gmane.org wrote:
>>>> I am using a custom PXA270 board and tried to use the SSP port to
>>>> communication with a SPI Flash. After reading docs on how to configure
>>>> the spi_master and spi devices, I have the device driver's probe being
>>>> called. But I cannot get the pxa2xx_spi driver to work correctly. I
>>>> can connect either a SD card or a M26P16 Flash chip to the SPI port on
>>>> my board. Neither device driver can completely operate their device.
>>>>
>>>> It looks like it is impossible for the device drivers to control the
>>>> chip select (CS) line. If I use the manual cs_control callback, the
>>>> timing is invalid because the SSP clock keeps running.
>>>>       
>>> The cs_control() callback is the normal way of controlling chip select
>>> with this driver.  That allows you to assign any GPIO pin or polarity.
>>> I understand that this differs from way that the spi core expects chip
>>> select to be handeled; in particular, the driver does not honor the
>>> SPI_CS_HIGH nor SPI_LSB_FIRST bits in the spi_device.mode.  The PXA
>>> SSP controllers do not have a dedicated chipselect mechanism
>>> associated with the SSP controllers (outside of the single FRAME pin,
>>> which may or may not do what you want), thus the use of GPIO lines is
>>> required.
>>>
>>> What version of the kernel/driver are you using?  There have been some
>>> recent changes, and there may still be a bug regarding CS changes that
>>> was discussed in:
>>> Re: [spi-devel-general] odd behaviour of chipselect with bitbang driver?
>>> on about 04/24/08 15:55.  I raised the question but, since I don't use
>>> chipselect at all, I have not produced a patch.
>>>
>>>     
> I am using 2.6.24.4 on custom HW. I am using the FRAME pin as the chip 
> select so I was hoping I did not have to use cs_control().

I agree with Cameron (below) that the Frame pin cannot be controlled to
maintain CS across transfers.  I think you will need to switch to a GPIO
pin and use the cs_control callback.  If your hardware is cast in stone,
you might be able to selectively set the Frame pin as a GPIO for use
with cs_control(), and leave the clock and the two data pins attached to
the SSP peripheral.

I don't see any functional patches that were applied after 3/24/08 when
2.6.24.4 was released, other than one that affects suspend/resume.  As
long as you are not resuming, you don't need the latest.

>>> The fact that the clock free runs strikes me as strange, I don't think
>>> any of the supported SPI_MODE_n combinations can produce a free
>>> running clock, though they do control polarity and phase of the clock.
>>> If your problem involves one extra clock edge, you may have the wrong
>>> setting for SPI_MODE_n.`
>>>     
>> Although the clock can free run in some of the other (non spi modes)
>> the driver doesn't support this which means the behaviour is somewhat
>> odd!  Any chance you can make available a picture of an oscilloscope trace
>> showing SFRM and the clock?  That'll probably give sufficient clues to
>> allow us to figure out what is going wrong.  I've used gpio based chip
>> selects on the pxa271 wihout problems (though it does tend to slow down
>> your transfers somewhat so is best avoided where possible).
>>   
> I attached a Logic Analyzer screen shot of m25p80 driver starting to 
> query the device. The RX line is floating when the m25pxx chip is not 
> driving it. But notice that the clocks are running before the FRM/CS 
> line is active. If I use cs_control(), then there would be several clock 
> edges (when using high speeds) before the PXA SSP controller would start 
> clocking data.

I can see from your logic analyzer trace, that the clock is running
before the transfer starts, but it should not be doing this, and such a
problem has not been reported by others, so far as I know.  You really
need to find out why this is happening.  Are you sure that you are using
the correct pins on the chip?  What pins and Alternate Function settings
are you using?

>>>> That means that
>>>> several bits are clocked out before the SSP controller starts to drive
>>>> the TX line correctly. If the SFRM signal is used, it does not allow
>>>> the driver to keep CS active for multiple transactions as they expect
>>>> and assume. The spi_sync call takes an spi_message which contains a
>>>> list of transactions to send. Normally, the device will need the CS
>>>> active during the complete message. But the SSP controller deactivates
>>>> SFRM when it is done with each buffer. For instance, the M25P16
>>>> datasheet indicates that CS has to stay active from the READ command
>>>> through the complete data transfer. When it goes high, it resets the
>>>> command interface.
>>>>       
>>> The driver should honor the per-transfer spi_transfer.cs_change flag
>>> when used with a cs_control routine.  If you want the CS to stay
>>> asserted between transfers, be sure the cs_change flag is 0.
>>>     
>> Indeed it should but it doesn't.  Unfortunately the hardware doesn't
>> seem to have the ability to do this, so it will only work if using
>> a gpio as the chip select.  Guess we really ought to add some stuff to
>> the docs to make this clear (it wasted a good few hours of my time
>> failing to understand why it didn't do anything!)  For short
>> transfers (sub 16 x 32bits) you can get away with doing the endianess
>> changes necessary and sending as a single multiword transfer.
>> Afraid I'm usually dealing with sensors, adc's etc and they don't
>> tend to need larger transfers than this between deasserting the chip
>> select.

I agree that the Frame pin cannot be used to hold CS across transfers,
and that is why I said "The driver should honor the per-transfer
spi_transfer.cs_change flag when used with a cs_control routine.",
meaning: only with use of a GPIO pin for CS.

>>>> It looks like the SSP port needs to have a register bit to turn off
>>>> the clock when there is nothing to transmit. Or a bit to tell the SSP
>>>> controller to leave CS active. Or I can change the HW so CS gates the
>>>> clock and a manual CS chip operates the device CS pin. I might also
>>>> try the bitbanging SPI driver next to see if that can operate with the
>>>> Flash chip.
>>>>       
>>> pxa2xx_spi.c uses format SSCR0_Motorola.  I think, as I said above,
>>> that this mode does not allow the clock to free run.  I expect that
>>> you have some other problem.  As I understand it (and I may be wrong,
>>> since I don't use Motorola SPI format) the clock is only active if
>>> data is being transfered, and data is only transferred if there is
>>> something the in the transmit FIFO to go out (and thus something to
>>> receive at the same time).
>>>     
>> That should certainly be the case.  I guess it's just possible that SSCR1 
>> is preset by something else to continuous clock mode? Don't think the driver
>> explicitly clears it and a quick glance suggests elements of that register
>> are set from chip specific data.

The driver explicitly loads the SSCR1 register, and all un-specified
bits are set to zero.

> Here are the registers:
> 
> root@inhand-ft4 [~] # md 0x41000000 0x3f
> 00000000  87 19 00 00 00 00 00 00  24 f0 00 00 00 00 00 00  |........$.......|
> 00000010  70 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |p...............|
> 00000020  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
> *
> 00000030

My translation is that you are using SSP1 on the pxa270, and that the
register values are:

Name	Addr		Value
SSCR0	0x41000000	0x00001987
			clock divisor=1a (19+1)
			on-board 13MHz clock source
			SSE=1 (enabled)
			Motorola SPI format
			NOT Network mode

SSCR1	0x41000004	0x00000000
			No external clock or ROWT set
			No interrupts or service requests enabled
			Master mode
			TX and RX thresholds = 1
I'm surprised that bits not set in this register.  It is normal for an
idle interface to have the timeout interrupt, and either service
requests or interrupts clear, but then I would expect SSE to be clear in
SSCR0.  Also I would expect the TX and RX thresholds to be set to the
mid point = 8; you can set other values, but I wonder if you did.

SSSR	0x41000008	0x0000f024
			Rx and Tx fifos empty
			no interrupts pending
SSITR	0x4100000c	0x00000000
			off = normal
SSDR	0x41000010	0x00000070
			last data read
SSTO	0x41000028	0x00000000
			timeout of 0 means no timeout
This is normal for an idle interface, as the driver zeros the timeout
when activity is complete.

SSPSP	0x4100002c	0x00000000
			Not used, 0=normal
SSTSA	0x41000030	unknown
SSRSA	0x41000034	unknown
SSTSS	0x41000038	unknown
These registers are not listed above, but I think they are not used,
except in network mode, which is disabled in SSCR0.

SSACD	0x4100003c	unknown
I'm not sure if this register is used when not in network mode.  It
affects the clock selection and clock rate, and appears to be mutually
exclusive with the SCR value in SSCR0.

I don't see anything above that would cause the SSPSCLK to free run.  Be
sure you are using SSPSCLK and not SSPSYSCLK, though the latter does not
seem possible, based on your logic analyzer trace.  The only strange
thing about the register state is that SSE is enabled, while every other
register implies that the interface should be idle.

-- 
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: pxa2xx_spi with SFRM

[Vernon Sauder]

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

Again, thanks for the help.

Ned Forrester wrote:
> I can see from your logic analyzer trace, that the clock is running
> before the transfer starts, but it should not be doing this, and such a
> problem has not been reported by others, so far as I know.  You really
> need to find out why this is happening.  Are you sure that you are using
> the correct pins on the chip?  What pins and Alternate Function settings
> are you using?  
In investigating this, I remembered that I was setting the SSPEXTCLK pin 
also. It turns out that disabling the alternate function on that pin 
turned off the outbound clock. I have no idea why but it turns out that 
this was wrong anyway (no external pullups) so it is "fixed" now.
>
> I agree that the Frame pin cannot be used to hold CS across transfers,
> and that is why I said "The driver should honor the per-transfer
> spi_transfer.cs_change flag when used with a cs_control routine.",
> meaning: only with use of a GPIO pin for CS.
>
>   
And now that the clock is off, the cs_control routing is viable and 
works fine.
>> Here are the registers:
>>
>> root@inhand-ft4 [~] # md 0x41000000 0x3f
>> 00000000  87 19 00 00 00 00 00 00  24 f0 00 00 00 00 00 00  |........$.......|
>> 00000010  70 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |p...............|
>> 00000020  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
>> *
>> 00000030
>>     
>
> My translation is that you are using SSP1 on the pxa270, and that the
> register values are:
>
> Name	Addr		Value
> SSCR0	0x41000000	0x00001987
> 			clock divisor=1a (19+1)
> 			on-board 13MHz clock source
> 			SSE=1 (enabled)
> 			Motorola SPI format
> 			NOT Network mode
>
> SSCR1	0x41000004	0x00000000
> 			No external clock or ROWT set
> 			No interrupts or service requests enabled
> 			Master mode
> 			TX and RX thresholds = 1
> I'm surprised that bits not set in this register.  It is normal for an
> idle interface to have the timeout interrupt, and either service
> requests or interrupts clear, but then I would expect SSE to be clear in
> SSCR0.  Also I would expect the TX and RX thresholds to be set to the
> mid point = 8; you can set other values, but I wonder if you did.
>   
This is another problem. The thresholds and timeouts are taken from the 
pxa2xx_spi_chip structure where cs_control is specified. So once I tried 
to use cs_control, the timeout and thresholds were wrong. They driver 
does not use defaults if these are zero. If timeout is zero, the kernel 
hangs while trying to read the trailing bytes. Here is a patch to use 
defaults if the timeout or thresholds are zero. (It is also attached due 
to my poor mailer.)

--- linux-2.6.24.4/drivers/spi/pxa2xx_spi.c    2008-03-24 
14:49:18.000000000 -0400
+++ src.cache/drivers/spi/pxa2xx_spi.c    2008-08-14 20:52:15.000000000 
-0400
@@ -1142,11 +1142,12 @@
         if (chip_info->cs_control)
             chip->cs_control = chip_info->cs_control;
 
+        if (chip_info->timeout)
         chip->timeout = chip_info->timeout;
 
-        chip->threshold = (SSCR1_RxTresh(chip_info->rx_threshold) &
+        chip->threshold = (SSCR1_RxTresh(chip_info->rx_threshold ? : 12) &
                                 SSCR1_RFT) |
-                (SSCR1_TxTresh(chip_info->tx_threshold) &
+                (SSCR1_TxTresh(chip_info->tx_threshold ? : 4) &
                                 SSCR1_TFT);
 
         chip->enable_dma = chip_info->dma_burst_size != 0

I can submit this more formally if you like. Let me know where.

> SSSR	0x41000008	0x0000f024
> 			Rx and Tx fifos empty
> 			no interrupts pending
> SSITR	0x4100000c	0x00000000
> 			off = normal
> SSDR	0x41000010	0x00000070
> 			last data read
> SSTO	0x41000028	0x00000000
> 			timeout of 0 means no timeout
> This is normal for an idle interface, as the driver zeros the timeout
> when activity is complete.
>
> SSPSP	0x4100002c	0x00000000
> 			Not used, 0=normal
> SSTSA	0x41000030	unknown
> SSRSA	0x41000034	unknown
> SSTSS	0x41000038	unknown
> These registers are not listed above, but I think they are not used,
> except in network mode, which is disabled in SSCR0.
>
> SSACD	0x4100003c	unknown
> I'm not sure if this register is used when not in network mode.  It
> affects the clock selection and clock rate, and appears to be mutually
> exclusive with the SCR value in SSCR0.
>
> I don't see anything above that would cause the SSPSCLK to free run.  Be
> sure you are using SSPSCLK and not SSPSYSCLK, though the latter does not
> seem possible, based on your logic analyzer trace.  The only strange
> thing about the register state is that SSE is enabled, while every other
> register implies that the interface should be idle.
>
>   
Here are the new register values:


SSP1 Control Register 0 (0x41000000)
SSP1_SSCR0               0x00002087  00000000 00000000 00100000 10000111
SSP1_SSCR0_MOD                    0  Network Mode
SSP1_SSCR0_ACS                    0  Audio Clock Select
SSP1_SSCR0_FRDC                   0  Frame Rate Divider Control
SSP1_SSCR0_TIM                    0  Tx FIFO Underrun IRQ Mask
SSP1_SSCR0_RIM                    0  Rx FIFO Underrun IRQ Mask
SSP1_SSCR0_NCS                    0  Nework Clock Select
SSP1_SSCR0_EDSS                   0  Extended Data Size Select
SSP1_SSCR0_SCR                   20  Serial Clock Rate
SSP1_SSCR0_SSE                    1  SSP Enable
SSP1_SSCR0_ECS                    0  External Clock Select
SSP1_SSCR0_FRF                    0  Frame Format 0=SPI 1=TI 2=Microwire 
3=PSP
SSP1_SSCR0_DSS                    3  Data Size Select (-1) hi bit is EDSS

** SSE is always set. The driver only clears it if it sees an error. Is 
this a bad thing?

SSP1 Control Register 1 (0x41000004)
SSP1_SSCR1               0x00000ec0  00000000 00000000 00001110 11000000
SSP1_SSCR1_TTELP                  0  TXD Tristate on Last Phase
SSP1_SSCR1_TTE                    0  TXD Tristate Enable
SSP1_SSCR1_EBCEI                  0  Enable Bit Count Error IRQ
SSP1_SSCR1_SCFR                   0  Slave Clock Free Running
SSP1_SSCR1_ECRA                   0  Enable Clock Request A
SSP1_SSCR1_ECRB                   0  Enable Clock Request B
SSP1_SSCR1_SCLKDIR                0  SSPSCLKx Direction
SSP1_SSCR1_SFRMDIR                0  SFRM Direction
SSP1_SSCR1_RWOT                   0  Receive w/o Tx
SSP1_SSCR1_TRAIL                  0  Trailing Byte
SSP1_SSCR1_TSRE                   0  Tx Service Request Enable
SSP1_SSCR1_RSRE                   0  Rx Service Request Enable
SSP1_SSCR1_TINTE                  0  Receiver Time-out IRQ Enable
SSP1_SSCR1_PINTE                  0  Peripheral Trailing Byte IRQ Enable
SSP1_SSCR1_IFS                    0  Invert Frame Signal
SSP1_SSCR1_STRF                   0  Select FIFO for EFWR
SSP1_SSCR1_EFWR                   0  Enable FIFO Write/Read (test mode)
SSP1_SSCR1_RFT                    3  Rx FIFO Threshold
SSP1_SSCR1_TFT                    b  Tx FIFO Threshold
SSP1_SSCR1_MWDS                   0  Microwire Tx Data Size
SSP1_SSCR1_SPH                    0  SPI SSPSCLK Phase
SSP1_SSCR1_SPO                    0  SPI SSPSCLK Polarity
SSP1_SSCR1_LBM                    0  Loop-back test mode
SSP1_SSCR1_TIE                    0  Tx FIFO IRQ Enable
SSP1_SSCR1_RIE                    0  Rx FIFO IRQ Enable

** Thresholds are now as set by defaults. Do you have a good way to 
figure out what they should be?

SSP1 Status Register (0x41000008)
SSP1_SSSR                0x0000f024  00000000 00000000 11110000 00100100
SSP1_SSSR_BCE                     0  Bit Count error
SSP1_SSSR_CSS                     0  Clock Sync Status
SSP1_SSSR_TUR                     0  Tx FIFO Underrun
SSP1_SSSR_EOC                     0  End of Chain
SSP1_SSSR_TINT                    0  Time-out IRQ
SSP1_SSSR_PINT                    0  Peripheral Trailing Byte IRQ
SSP1_SSSR_RFL                     f  Rx FIFO Level
SSP1_SSSR_TFL                     0  Tx FIFO Level
SSP1_SSSR_ROR                     0  Rx FIFO Overrun
SSP1_SSSR_RFS                     0  Rx FIFO Service
SSP1_SSSR_TFS                     1  Tx FIFO Service
SSP1_SSSR_BSY                     0  Busy
SSP1_SSSR_RNE                     0  Rx FIFO Not Empty
SSP1_SSSR_TNF                     1  Tx FIFO Not Full

SSP1 Interrupt Test Register (0x4100000c)
SSP1_SSITR               0x00000000  00000000 00000000 00000000 00000000

SSP1 Data Read/Write Register (0x41000010)
SSP1_SSDR                0x000000ff  00000000 00000000 00000000 11111111

SSP1 Time-Out Register (0x41000028)
SSP1_SSTO                0x00000000  00000000 00000000 00000000 00000000
SSP1_SSTO                         0  SSP1 Time-Out Register

** It is zero after the transfer.

SSP1 Programmable Serial Protocol Register (0x4100002c)
SSP1_SSPSP               0x00000000  00000000 00000000 00000000 00000000

SSP1 Tx Timeslot Active Register (0x41000030)
SSP1_SSTSA               0x00000000  00000000 00000000 00000000 00000000
SSP1_SSTSA                        0  SSP1 Tx Timeslot Active Register

SSP1 Rx Timeslot Active Register (0x41000034)
SSP1_SSRSA               0x00000000  00000000 00000000 00000000 00000000
SSP1_SSRSA                        0  SSP1 Rx Timeslot Active Register

SSP1 Timeslot Status Register (0x41000038)
SSP1_SSTSS               0x00000000  00000000 00000000 00000000 00000000
SSP1_SSTSS                        0  SSP1 Timeslot Status Register

SSP1 Audio Clock Divider Register (0x4100003c)
SSP1_SSACD               0x00000000  00000000 00000000 00000000 00000000
SSP1_SSACD                        0  SSP1 Audio Clock Divider Register


Attached is the platform file I have created for the Fingertip4 SSP.

On a side note, there is an MTD error when using the SPI Flash with the 
pxa2xx_spi driver.

root@inhand-ft4 [~] # flash_erase /dev/mtd_spi
Erase Total 1 Units
Performing Flash Erase of length 65536 at offset 0x0 done # <- why *64K* ??
root@inhand-ft4 [~] # echo -n hithere123 > /dev/mtd_spi
root@inhand-ft4 [~] # hexdump -C -n512 /dev/mtdblock_spi
00000000  68 69 74 68 65 72 65 31  32 33 ff ff ff ff ff ff  
|hithere123......|
00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*
00000200
root@inhand-ft4 [~] # echo -n hithere444xxx > /dev/mtdblock_spi
[  231.370000] pxa2xx-spi pxa2xx-spi.1: pump_transfers: transfer length 
greater than 8191
 # maybe 64K??
[  231.380000] end_request: I/O error, dev mtdblock_spi, sector 0
[  231.380000] Buffer I/O error on device mtdblock_spi, logical block 0
[  231.380000] lost page write due to I/O error on mtdblock_spi
root@inhand-ft4 [~] # hexdump -C -n512 /dev/mtdblock_spi
00000000  68 69 74 68 65 72 65 31  32 33 ff ff ff ff ff ff  
|hithere123......|
00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*
00000200

I have not done a great deal of testing with MTD yet but I can read and 
write a small test file. I was hoping to use the mtdblock interface for 
this application. I will take a little time and see if I can figure out 
what part is wrong here.


Thanks again for your help.
Vern

[-- Attachment #2: pxa2xx-spi_timeout_optional.patch --]
[-- Type: text/plain, Size: 785 bytes --]

diff -U3 --exclude-from=/tmp/tmp.WAjUp31037 -rNbw -U3 linux-2.6.24.4/drivers/spi/pxa2xx_spi.c src.cache/drivers/spi/pxa2xx_spi.c
--- linux-2.6.24.4/drivers/spi/pxa2xx_spi.c	2008-03-24 14:49:18.000000000 -0400
+++ src.cache/drivers/spi/pxa2xx_spi.c	2008-08-14 20:52:15.000000000 -0400
@@ -1142,11 +1142,12 @@
 		if (chip_info->cs_control)
 			chip->cs_control = chip_info->cs_control;
 
+		if (chip_info->timeout)
 		chip->timeout = chip_info->timeout;
 
-		chip->threshold = (SSCR1_RxTresh(chip_info->rx_threshold) &
+		chip->threshold = (SSCR1_RxTresh(chip_info->rx_threshold ? : 12) &
 								SSCR1_RFT) |
-				(SSCR1_TxTresh(chip_info->tx_threshold) &
+				(SSCR1_TxTresh(chip_info->tx_threshold ? : 4) &
 								SSCR1_TFT);
 
 		chip->enable_dma = chip_info->dma_burst_size != 0

[-- Attachment #3: ft4-ssp.c --]
[-- Type: text/x-csrc, Size: 7718 bytes --]

/*
 * InHand SSP platform driver
 *
 * Copyright: 2007-2008 Inhand Electronics, Inc.
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 2 as
 * published by the Free Software Foundation.
 */

#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/irq.h>
#include <linux/delay.h>
#include <linux/spi/spi.h>
#include <linux/spi/flash.h>

#include <asm/hardware.h>
#include <asm/arch/pxa-regs.h>
#include <asm/arch/ssp.h>
#include <asm/arch/pxa2xx_spi.h>

#include <linux/inhand-types.h>
#include <linux/pxa_gpio.h>

MODULE_AUTHOR("Vernon Sauder <vsauder-xB3BArjMkznQT0dZR+AlfA@public.gmane.org>");
MODULE_DESCRIPTION("InHand SSP platform driver");
MODULE_LICENSE("GPL");

#define MHZ *1000*1000
#define KHZ *1000

#define SSP_REG_LEN 0x40

// GPIO pins to initialize for SSP ports 1 & 2
// FT4 does not have port 3
static u32 ssp_gpio_init_list[] __initdata = {
#ifdef CONFIG_SSP1_EN
	// pins needed for SSP1
	(23 | GPIO_ALT_FN_2_OUT), 	// CLK
	//(24 | GPIO_ALT_FN_2_OUT), 	// Frame
	(25 | GPIO_ALT_FN_2_OUT), 	// TX
	(26 | GPIO_ALT_FN_1_IN), 	// RX
	// don't enable EXTCLK unless you actually are using it. Otherwise,
	// the SSPCLK will always run (for some strange reason)
	//(27 | GPIO_ALT_FN_2_IN), 	// EXTCLK
#endif
#ifdef CONFIG_SSP2_EN
	// pins needed for SSP2
	(36 | GPIO_ALT_FN_2_OUT), 	// CLK
	(37 | GPIO_ALT_FN_2_OUT), 	// Frame
	(13 | GPIO_ALT_FN_1_OUT), 	// TX
	(29 | GPIO_ALT_FN_1_OUT), 	// RX (yes, out)
	// don't enable EXTCLK unless you actually are using it. Otherwise,
	// the SSPCLK will always run (for some strange reason)
	//(22 | GPIO_ALT_FN_1_IN), 	// EXTCLK
#endif
};


// SPI Chips

#ifdef CONFIG_SSP1_EN

// enable as needed
//#define M25P16		// SPI FLash
#define SD		// driver for SD card in SPI mode

// use FRM as CS0
static gpreg ssp1_cs0_gpio = {
	.name = "SSP1 CS0",
	.offset = 24,
	.output = 1,
	.act_low = 1,
	.drv = &pxa_gpreg_drv,
};

// use EXTCLK as CS1
static gpreg ssp1_cs1_gpio = {
	.name = "SSP1 CS1",
	.offset = 27,
	.output = 1,
	.act_low = 1,
	.drv = &pxa_gpreg_drv,

};

// SPI driver callback to manipulate CS0
static void ssp_cs0_cb(u32 command)
{
	if (command == PXA2XX_CS_ASSERT)
		gpreg_activate(&ssp1_cs0_gpio);
	else
		gpreg_deactivate(&ssp1_cs0_gpio);
}

// SPI driver callback to manipulate CS1
static void ssp_cs1_cb(u32 command)
{
	if (command == PXA2XX_CS_ASSERT)
		gpreg_activate(&ssp1_cs1_gpio);
	else
		gpreg_deactivate(&ssp1_cs1_gpio);
}

/* SPI chip data */
static struct pxa2xx_spi_chip ssp1_cs0_settings = {
	.tx_threshold	= 12,
	.rx_threshold	= 4,
	.cs_control		= ssp_cs0_cb,
};
static struct pxa2xx_spi_chip ssp1_cs1_settings = {
	.cs_control		= ssp_cs1_cb,
};

#ifdef M25P16
// declare the type of SPI flash and MTD config info
static struct flash_platform_data pdata_m25p16 = {
	.name = "spi",
	.type = "m25p16",
};
#endif

// The set of devices that are on the SPI (1) bus.
static struct spi_board_info spi1_chips[] __initdata = {
	// This setup is invalid because they all use CS0. If this were a real
	// setup, they would all use different controller_data's. For this example,
	// it allows each device to be used by physically connecting and
	// disconnecting (and insmod/rmmod) as needed for testing.
	// 'spidev' is a user-mode interface. It can theoritically be used alongside
	// a kernel-mode driver as long as they are not used at the same time. The
	// spidev interface allows user-mode debugging of the kernel-mode driver.
	// If a real user-mode driver was used, the kernel driver would need to be disabled.
	// The .chip_select field is better called device-index.
	{
		.modalias		= "spidev",
		//.platform_data	= 0,
		.controller_data = &ssp1_cs0_settings,
		//.irq			= DEV_IRQ,
		//.mode 		= SPI_CPOL | SPI_CPHA,
		.max_speed_hz	= 10 MHZ,		// max - PXA only does 13MHz
		.bus_num		= 1,			// SSP1
		.chip_select	= 0,			// device number on bus (0-based)
	},
	{
		.modalias		= "spidev",
		.controller_data = &ssp1_cs1_settings,
		.max_speed_hz	= 10 MHZ,		// max - PXA only does 13MHz
		.bus_num		= 1,			// SSP1
		.chip_select	= 1,			// device number on bus (0-based)
	},
#ifdef M25P16
	{	// SPI FLash
		.modalias		= "m25p80",
		.platform_data	= &pdata_m25p16,
		.controller_data = &ssp1_cs0_settings,
		.max_speed_hz	= 20 MHZ,
		.bus_num		= 1,
		.chip_select	= 2,	// device number on bus (0-based)
	},
#endif
#ifdef SD
	{
		.modalias		= "mmc_spi",
		.controller_data = &ssp1_cs0_settings,
		.max_speed_hz	= 20 MHZ,
		.bus_num		= 1,
		#ifdef M25P16
			.chip_select	= 3,	// device number on bus (0-based)
		#else
			.chip_select	= 2,	// device number on bus (0-based)
		#endif
	},
#endif
};

static struct resource pxa_ssp1_resources[] = {
	{
		.start	= __PREG(SSCR0_P(1)),	/* Start address of SSP1 registers */
		.end	= __PREG(SSCR0_P(1)) + SSP_REG_LEN - 1,
		.flags	= IORESOURCE_MEM,
	},
	{
		.start	= IRQ_SSP,
		.end	= IRQ_SSP,
		.flags	= IORESOURCE_IRQ,
	},
};

static struct pxa2xx_spi_master pxa_ssp1_master_info = {
	.ssp_type		= PXA27x_SSP,
	.clock_enable	= CKEN_SSP1,
	.num_chipselect	= ARRAY_SIZE(spi1_chips),	/* number of chips attached to SSP */
	.enable_dma		= 1,
};

static struct platform_device pxa_ssp1 = {
	.name		= "pxa2xx-spi",			/* driver to use */
	.id			= 1,					/* Bus number, MUST MATCH SSP number 1..n */
	.resource	= pxa_ssp1_resources,
	.num_resources = ARRAY_SIZE(pxa_ssp1_resources),
	.dev = {
		.platform_data	= &pxa_ssp1_master_info,
	},
};
#else
#define spi1_chips 0
#endif

#ifdef CONFIG_SSP2_EN
// The set of devices that are on the SPI (2) bus.
static struct spi_board_info spi2_chips[] __initdata = {
	// Enable an spidev "chip" for debugging
	{
		.modalias		= "spidev",
		.max_speed_hz	= 20 MHZ,		// max - PXA only does 13MHz
		.bus_num		= 2,			// SSP1
		.chip_select	= 0,			// device number on bus (0-based)
	},
};

static struct resource pxa_ssp2_resources[] = {
	{
		.start  = __PREG(SSCR0_P(2)),	/* Start address of SSP2 registers */
		.end    = __PREG(SSCR0_P(2)) + SSP_REG_LEN - 1,
		.flags  = IORESOURCE_MEM,
	},
	{
		.start  = IRQ_SSP2,
		.end    = IRQ_SSP2,
		.flags  = IORESOURCE_IRQ,
	},
};

static struct pxa2xx_spi_master pxa_ssp2_master_info = {
	.ssp_type = PXA27x_SSP,
	.clock_enable = CKEN_SSP2,
	.num_chipselect	= ARRAY_SIZE(spi2_chips),	/* number of chips attached to SSP */
	.enable_dma = 0,
};

static struct platform_device pxa_ssp2 = {
	.name 		= "pxa2xx-spi",			/* driver to use */
	.id 		= 2,					/* Bus number, MUST MATCH SSP number 1..n */
	.resource 	= pxa_ssp2_resources,
	.num_resources = ARRAY_SIZE(pxa_ssp2_resources),
	.dev = {
		.platform_data = &pxa_ssp2_master_info, /* Passed to driver */
	},
};
#else
#define spi2_chips 0
#endif

static struct platform_device *ssp_devices[] __initdata = {
#ifdef CONFIG_SSP1_EN
    &pxa_ssp1,
#endif
#ifdef CONFIG_SSP2_EN
    &pxa_ssp2,
#endif
};

// the set of gpregs that need to be initialized
static gpreg* sspregs[] __initdata = {
#ifdef CONFIG_SSP1_EN
	&ssp1_cs0_gpio,
	&ssp1_cs1_gpio,
#endif
#ifdef CONFIG_SSP2_EN
#endif
};


// Module initialization
// called from platform
extern int __init ft4_ssp_init(void)
{
	pr_info("Installing SSP drivers/devices\n");
	/* SSP GPIO setup */
	pxa_gpio_init(ssp_gpio_init_list, ARRAY_SIZE(ssp_gpio_init_list));
	// initialize our GPIO pins
	gpreg **reg = sspregs;
	for (int i = 0; i < ARRAY_SIZE(sspregs); i++, reg++) {
		if (gpreg_init(*reg)) {
			return -ENODEV;
		}
	}
	// register ssp controllers
	int err = platform_add_devices(ssp_devices, ARRAY_SIZE(ssp_devices));
	// register spi chips
	if (spi1_chips)
		spi_register_board_info(spi1_chips, ARRAY_SIZE(spi1_chips));
	if (spi2_chips)
		spi_register_board_info(spi2_chips, ARRAY_SIZE(spi2_chips));
	return err;
}

[-- Attachment #4: 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 #5: 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

Limitations on transfer length [was: pxa2xx_spi with SFRM]

[Ned Forrester]

Vernon Sauder wrote:
> Ned Forrester wrote:

> On a side note, there is an MTD error when using the SPI Flash with the 
> pxa2xx_spi driver.
> 
> root@inhand-ft4 [~] # flash_erase /dev/mtd_spi
> Erase Total 1 Units
> Performing Flash Erase of length 65536 at offset 0x0 done # <- why *64K* ??
> root@inhand-ft4 [~] # echo -n hithere123 > /dev/mtd_spi
> root@inhand-ft4 [~] # hexdump -C -n512 /dev/mtdblock_spi
> 00000000  68 69 74 68 65 72 65 31  32 33 ff ff ff ff ff ff  
> |hithere123......|
> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
> |................|
> *
> 00000200
> root@inhand-ft4 [~] # echo -n hithere444xxx > /dev/mtdblock_spi
> [  231.370000] pxa2xx-spi pxa2xx-spi.1: pump_transfers: transfer length 
> greater than 8191

This message is issued by pxa2xx_spi.c in pump_transfers() if the
transfer length is illegal.  You may have uncovered a bug here.  I think
the 8191 limitation on transfer length only applies to DMA, as that is
the length of the DMA counter.  The test for this should probably be
performed only if DMA is in use.  I don't think Stephen or I ever
expected any transfer to approach that length, in either PIO or DMA
mode.  Is this really a legitimate transfer request?  Something that can
only work in PIO mode and not in DMA mode?  That seems improper to me.

>  # maybe 64K??
> [  231.380000] end_request: I/O error, dev mtdblock_spi, sector 0
> [  231.380000] Buffer I/O error on device mtdblock_spi, logical block 0
> [  231.380000] lost page write due to I/O error on mtdblock_spi
> root@inhand-ft4 [~] # hexdump -C -n512 /dev/mtdblock_spi
> 00000000  68 69 74 68 65 72 65 31  32 33 ff ff ff ff ff ff  
> |hithere123......|
> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
> |................|
> *
> 00000200
> 
> I have not done a great deal of testing with MTD yet but I can read and 
> write a small test file. I was hoping to use the mtdblock interface for 
> this application. I will take a little time and see if I can figure out 
> what part is wrong here.
> 
> 
> Thanks again for your help.
> Vern

David,

Is there supposed to be any practical limit to the length of a transfer?
 It seems that the /dev/mtdblock_spi driver has requested a transfer in
excess of 8191 bytes (though I don't know how long a transfer it
requested).

In this case, I think pxa2xx_spi.c could handle long transfers in PIO
mode, but not if DMA has been requested.  It looks like we may have put
a bug in pxa2xx_spi by performing the length test even when DMA is not
requested.

Is it proper to require that PIO mode be used if long transfers will be
requested?

I see several possibilities (and there are probably more):

1. Make long transfers illegal (probably both undesirable and
unenforceable).

2. Require PIO mode to be used by protocol drivers for long transfers
(but how would they know how long is too long?).

3. Simply reject the call to transfer() if too long a transfer is
requested with DMA.

4. Silently, or with warning, revert to PIO mode if a long transfer is
requested with DMA.

5. Require that pxa2xx_spi break long DMA transfers into the required
number of pieces to be performed in separate DMAs, silently giving back
the message after it has completed all of it (this requires a lot of
extra book-keeping in pxa2xx_spi).



-- 
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: pxa2xx_spi with SFRM

[Ned Forrester]

Resend due to bad cc: address...

Vernon Sauder wrote:

> This is another problem. The thresholds and timeouts are taken from the 
> pxa2xx_spi_chip structure where cs_control is specified. So once I tried 
> to use cs_control, the timeout and thresholds were wrong. They driver 
> does not use defaults if these are zero. If timeout is zero, the kernel 
> hangs while trying to read the trailing bytes. Here is a patch to use 
> defaults if the timeout or thresholds are zero. (It is also attached due 
> to my poor mailer.)
> 
> --- linux-2.6.24.4/drivers/spi/pxa2xx_spi.c    2008-03-24 
> 14:49:18.000000000 -0400
> +++ src.cache/drivers/spi/pxa2xx_spi.c    2008-08-14 20:52:15.000000000 
> -0400
> @@ -1142,11 +1142,12 @@
>          if (chip_info->cs_control)
>              chip->cs_control = chip_info->cs_control;
>  
> +        if (chip_info->timeout)
>          chip->timeout = chip_info->timeout;
>  
> -        chip->threshold = (SSCR1_RxTresh(chip_info->rx_threshold) &
> +        chip->threshold = (SSCR1_RxTresh(chip_info->rx_threshold ? : 12) &
>                                  SSCR1_RFT) |
> -                (SSCR1_TxTresh(chip_info->tx_threshold) &
> +                (SSCR1_TxTresh(chip_info->tx_threshold ? : 4) &
>                                  SSCR1_TFT);
>  
>          chip->enable_dma = chip_info->dma_burst_size != 0
> 
> I can submit this more formally if you like. Let me know where.

I'm not sure that implementing defaults is a good idea.  I prefer the
current scheme of forcing the user to make a considered choice.  In
particular, 0 is a valid value for the timeout and thresholds (it means
none in the case of timeout and one in the case of thresholds), so if
these structure elements weren't already defined as unsigned, -1 would
be a better flag to trigger use of a default.

More specifically, I don't see in the patch where there is a default for
timeout; you simply leave it undefined if a value of 0 was specified;
there is no "else" for "if (chip_info->timeout)".  In any case, it would
be difficult at best to offer a default value, since this is a division
of an unspecified clock.  It turns out that the developer's manuals for
both PXA255 and PXA270 do not adequately specify what clock is used for
the timer.  Thus a numerical value that is appropriate for one may not
be appropriate for another, nor for the new PXA3xx processors.  We just
don't know.  Furthermore, an appropriate default depends on the rate of
SSPSCLK, which is a variable.  There are also application specific
considerations.

The defaults of 12 and 4 that you specified above for thresholds are not
actually the preferred values.  When I was working with Stephen on some
issues with this driver, we concluded that 8 and 8 were better choices.
 When Stephen submitted the patches, he corrected this in the example
structures in Documentation/spi/pxa2xx_spi, but forgot to do so in the
text describing the threshold values.  You can see the changes he made at:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=8d94cc50aa4f1448a6483975097805eb8d6be0e0

I am curious about the kernel hanging with a zero timeout.  I would
expect that the trailing bytes would never be read, but this driver
should not hang waiting for them, because it simply is not responding to
an interrupt that never happens.  Perhaps the hang occurs at a higher
level when the message is never given back.  Do you really mean that the
kernel hangs (all processes stop) or just that the MMC operation never
finishes?  Are you using PIO (interrupt) mode or DMA when you access the
driver (or is that hidden from you by the MMC drivers that you are using)?

> Here are the new register values:
> 
> 
> SSP1 Control Register 0 (0x41000000)
> SSP1_SSCR0               0x00002087  00000000 00000000 00100000 10000111
> SSP1_SSCR0_MOD                    0  Network Mode
> SSP1_SSCR0_ACS                    0  Audio Clock Select
> SSP1_SSCR0_FRDC                   0  Frame Rate Divider Control
> SSP1_SSCR0_TIM                    0  Tx FIFO Underrun IRQ Mask
> SSP1_SSCR0_RIM                    0  Rx FIFO Underrun IRQ Mask
> SSP1_SSCR0_NCS                    0  Nework Clock Select
> SSP1_SSCR0_EDSS                   0  Extended Data Size Select
> SSP1_SSCR0_SCR                   20  Serial Clock Rate
> SSP1_SSCR0_SSE                    1  SSP Enable
> SSP1_SSCR0_ECS                    0  External Clock Select
> SSP1_SSCR0_FRF                    0  Frame Format 0=SPI 1=TI 2=Microwire 
> 3=PSP
> SSP1_SSCR0_DSS                    3  Data Size Select (-1) hi bit is EDSS
> 
> ** SSE is always set. The driver only clears it if it sees an error. Is 
> this a bad thing?

My mistake.  I was looking at my expanded version of the driver, but
even there, SSE is cleared in xxx_transfer_complete() only on certain
errors.  Your behavior is normal.  Sorry.

> SSP1 Control Register 1 (0x41000004)
> SSP1_SSCR1               0x00000ec0  00000000 00000000 00001110 11000000
> SSP1_SSCR1_TTELP                  0  TXD Tristate on Last Phase
> SSP1_SSCR1_TTE                    0  TXD Tristate Enable
> SSP1_SSCR1_EBCEI                  0  Enable Bit Count Error IRQ
> SSP1_SSCR1_SCFR                   0  Slave Clock Free Running
> SSP1_SSCR1_ECRA                   0  Enable Clock Request A
> SSP1_SSCR1_ECRB                   0  Enable Clock Request B
> SSP1_SSCR1_SCLKDIR                0  SSPSCLKx Direction
> SSP1_SSCR1_SFRMDIR                0  SFRM Direction
> SSP1_SSCR1_RWOT                   0  Receive w/o Tx
> SSP1_SSCR1_TRAIL                  0  Trailing Byte
> SSP1_SSCR1_TSRE                   0  Tx Service Request Enable
> SSP1_SSCR1_RSRE                   0  Rx Service Request Enable
> SSP1_SSCR1_TINTE                  0  Receiver Time-out IRQ Enable
> SSP1_SSCR1_PINTE                  0  Peripheral Trailing Byte IRQ Enable
> SSP1_SSCR1_IFS                    0  Invert Frame Signal
> SSP1_SSCR1_STRF                   0  Select FIFO for EFWR
> SSP1_SSCR1_EFWR                   0  Enable FIFO Write/Read (test mode)
> SSP1_SSCR1_RFT                    3  Rx FIFO Threshold
> SSP1_SSCR1_TFT                    b  Tx FIFO Threshold
> SSP1_SSCR1_MWDS                   0  Microwire Tx Data Size
> SSP1_SSCR1_SPH                    0  SPI SSPSCLK Phase
> SSP1_SSCR1_SPO                    0  SPI SSPSCLK Polarity
> SSP1_SSCR1_LBM                    0  Loop-back test mode
> SSP1_SSCR1_TIE                    0  Tx FIFO IRQ Enable
> SSP1_SSCR1_RIE                    0  Rx FIFO IRQ Enable
> 
> ** Thresholds are now as set by defaults. Do you have a good way to 
> figure out what they should be?

See comments above about good values of thresholds, and defaults as
policy.  In general, I found the driver fairly robust for all values of
the thresholds when in PIO mode.  Personally, I use 8 and 8 for
11Mbit/sec DMA mode. I think it is fair to use whatever works in your
application.  Here is a slightly relevant comment from my discussion
with Stephen on Oct 1, 2006 (which somehow never made onto the spi-devel
mailing list, in spite of being addressed there):

>> 5. Coding of threshold as a function of bits/word and dma burst size:
>> 
>> This change was a lot more complicated than I thought, partly because 
>> there are two places in the driver where this computation needs to be 
>> done: in setup, and also in pump_transfers if a new bits/word is 
>> attached to an spi_transfer.  I have created a new routine that computes 
>> the proper dma_threshold from bits/word and burst_size, and I have 
>> called it from setup and pump_transfers.  In setup, it changes the 
>> values stored in struct chip_data, but in pump_transfers it only changes 
>> local "use once" values.  It is possible for a combination of bits/word 
>> and burst_size to yield invalid threshold settings (any threshold more 
>> than 1/2 the fifo, 8 words, because only 2, 4, 8 and 16 would match the 
>> burst size, but 16 causes overruns).  To deal with this, the burst size 
>> is reduced as required to keep the threshold at 8.  If this reduction 
>> occurs in setup, a dev_warn is issued to let the user know; if the 
>> reduction happens in pump_transfers, the change is silent.  Change the 
>> reporting if you think something else is better.
>> 
>> [snip] 
>>
>> ----Testing
>> 
>> This patch has been tested for reading the correct number of bytes 
>> without overruns or permanent timeouts; I did not use loopback and pass 
>> test data to verify on read, but I could do that.  The test was done 
>> with randomized transfer lengths between 4 and >4096 bytes (in multiples 
>> of 4 bytes, I suppose I could do it in integer bytes), with all word 
>> sizes from 4 to 32 bits.  Baud rate was changed from 3.6864MHZ to 
>> 100kHz.  Both DMA and PIO mode were tested.  In DMA mode various 
>> combinations of word size and burst size were used.  In PIO mode various 
>> threshold settings from tx=1 and rx=15, to tx=15 and rx=1 were tested. 
>> All modes worked.  With PIO mode fixed to prevent overruns, the 
>> operation becomes insensitive to values of threshold and timeout.  In 
>> DMA mode, it is necessary to keep timeout longer than one word time

I think the  "1" and "15" above correspond to threshold register values
of "0" and "14"; that is the above numbers are the number of words in
the FIFO that trigger the interrupt/service request.

OK on the rest of the register contents.

> On a side note, there is an MTD error when using the SPI Flash with the 
> pxa2xx_spi driver.
> 
> root@inhand-ft4 [~] # flash_erase /dev/mtd_spi
> Erase Total 1 Units
> Performing Flash Erase of length 65536 at offset 0x0 done # <- why *64K* ??
> root@inhand-ft4 [~] # echo -n hithere123 > /dev/mtd_spi
> root@inhand-ft4 [~] # hexdump -C -n512 /dev/mtdblock_spi
> 00000000  68 69 74 68 65 72 65 31  32 33 ff ff ff ff ff ff  
> |hithere123......|
> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
> |................|
> *
> 00000200
> root@inhand-ft4 [~] # echo -n hithere444xxx > /dev/mtdblock_spi
> [  231.370000] pxa2xx-spi pxa2xx-spi.1: pump_transfers: transfer length 
> greater than 8191

This message is issued by the driver in pump_transfers() if the transfer
length is illegal.  You may have uncovered a bug here.  I think the 8191
limitation on transfer length only applies to DMA, as that is the length
of the DMA counter.  The test for this should probably be performed only
if DMA is in use.  I don't think Stephen or I ever expected any transfer
to approach that length, in either PIO or DMA mode.  Is this really a
legitimate transfer request?  Something that can only work in PIO mode
and not in DMA mode?  That seems improper to me.

As noted in the comments in pxa2xx_spi.c, this test really belongs in
transfer(), not in pump_transfers(), so that a "bad" transfer in a
message is never accepted, rather than been dropped after a message has
been started.  I figured the test location was not important if it never
failed anyway.

>  # maybe 64K??
> [  231.380000] end_request: I/O error, dev mtdblock_spi, sector 0
> [  231.380000] Buffer I/O error on device mtdblock_spi, logical block 0
> [  231.380000] lost page write due to I/O error on mtdblock_spi
> root@inhand-ft4 [~] # hexdump -C -n512 /dev/mtdblock_spi
> 00000000  68 69 74 68 65 72 65 31  32 33 ff ff ff ff ff ff  
> |hithere123......|
> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
> |................|
> *
> 00000200
> 
> I have not done a great deal of testing with MTD yet but I can read and 
> write a small test file. I was hoping to use the mtdblock interface for 
> this application. I will take a little time and see if I can figure out 
> what part is wrong here.
> 
> 
> Thanks again for your help.
> Vern

-- 
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: pxa2xx_spi with SFRM

[Vernon Sauder]

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

Ned Forrester wrote:

>I'm not sure that implementing defaults is a good idea.  I prefer the
>current scheme of forcing the user to make a considered choice.  In
>particular, 0 is a valid value for the timeout and thresholds (it means
>none in the case of timeout and one in the case of thresholds), so if
>these structure elements weren't already defined as unsigned, -1 would
>be a better flag to trigger use of a default.

I added the defaults because there is an inconsistency in the API. When
I added cs_control, it did not force me to make a "considered
choice"; it forced me to debug a driver to determine why it didn't
work anymore. The .h file mentions that the settings are for "DMA
tuning" so I did not initialize them. I only added cs_control and left
the other members zero. Either the code or the documentation needs a
small update, IMHO. I can send a patch but it looks like I might be
duplicating work that has already been done and got lost somewhere.

Zero is not a valid timeout if the driver relies on the timeout
interrupt to receive trailing bytes. If timeout is zero, the
timeout interrupt will not happen, right?

A threshold of 0 is invalid. It will actually become 16:

#define SSCR1_TxTresh(x) (((x) - 1) << 6) /* level [1..16] */

>More specifically, I don't see in the patch where there is a default
>for timeout; you simply leave it undefined if a value of 0 was
>specified; there is no "else" for "if (chip_info->timeout)".  

The timeout default is set a couple lines above when the chip struct is
malloc'd. The patch just does not overwrite it.

>In any
>case, it would be difficult at best to offer a default value, since
>this is a division of an unspecified clock.  It turns out that the
>developer's manuals for both PXA255 and PXA270 do not adequately
>specify what clock is used for the timer.  Thus a numerical value that
>is appropriate for one may not be appropriate for another, nor for the
>new PXA3xx processors.  We just don't know.  Furthermore, an
>appropriate default depends on the rate of SSPSCLK, which is a
>variable.  There are also application specific considerations.

Which again leads me to ask, how would I know if the value I picked is
good, or right, or wrong?

>The defaults of 12 and 4 that you specified above for thresholds are
>not actually the preferred values.  When I was working with Stephen on
>some issues with this driver, we concluded that 8 and 8 were better
>choices.

The 12/4 values are the defaults used in _probe(). No real comment there
on why those values are chosen. The defaults if chip_info is missing are
1 & 1. All these defaults could be consolidated a little. :)  And maybe
explained. Or maybe not needed at all if the platform *must* pass them
in!

>I am curious about the kernel hanging with a zero timeout.  I would
>expect that the trailing bytes would never be read, but this driver
>should not hang waiting for them, because it simply is not responding
>to an interrupt that never happens.  Perhaps the hang occurs at a
>higher level when the message is never given back.  Do you really mean
>that the kernel hangs (all processes stop) or just that the MMC
>operation never finishes?  Are you using PIO (interrupt) mode or DMA
>when you access the driver (or is that hidden from you by the MMC
>drivers that you are using)?

I believe this was PIO. You are correct, only the MMC hung, not the
kernel, my bad. I think it was stuck in this code:

static irqreturn_t interrupt_transfer(struct driver_data *drv_data)
<snip>
	if (drv_data->tx == drv_data->tx_end) {
		write_SSCR1(read_SSCR1(reg) & ~SSCR1_TIE, reg);
		/* PXA25x_SSP has no timeout, read trailing bytes */
		if (drv_data->ssp_type == PXA25x_SSP) {
			if (!wait_ssp_rx_stall(reg))
			{
				int_error_stop(drv_data, "interrupt_transfer: "
						"rx stall failed");
				return IRQ_HANDLED;
			}
			if (!drv_data->read(drv_data))
			{
				int_error_stop(drv_data,
						"interrupt_transfer: "
						"trailing byte read failed");
				return IRQ_HANDLED;
			}
			int_transfer_complete(drv_data);
		}
	}

	/* We did something */
	return IRQ_HANDLED;
}

It basically thought the TX was done but it was missing RX data. I can
try to recreate this if you need.

>> 
>> ** Thresholds are now as set by defaults. Do you have a good way to 
>> figure out what they should be?
>
>See comments above about good values of thresholds, and defaults as
>policy.  In general, I found the driver fairly robust for all values of
>the thresholds when in PIO mode.  Personally, I use 8 and 8 for
>11Mbit/sec DMA mode. I think it is fair to use whatever works in your
>application.  Here is a slightly relevant comment from my discussion
>with Stephen on Oct 1, 2006 (which somehow never made onto the
>spi-devel mailing list, in spite of being addressed there):
>
>>> 5. Coding of threshold as a function of bits/word and dma burst
>>> size:
>>> 
>>> This change was a lot more complicated than I thought, partly
>>> because there are two places in the driver where this computation
>>> needs to be done: in setup, and also in pump_transfers if a new
>>> bits/word is attached to an spi_transfer.  I have created a new
>>> routine that computes the proper dma_threshold from bits/word and
>>> burst_size, and I have called it from setup and pump_transfers.  In
>>> setup, it changes the values stored in struct chip_data, but in
>>> pump_transfers it only changes local "use once" values.  It is
>>> possible for a combination of bits/word and burst_size to yield
>>> invalid threshold settings (any threshold more than 1/2 the fifo, 8
>>> words, because only 2, 4, 8 and 16 would match the burst size, but
>>> 16 causes overruns).  To deal with this, the burst size is reduced
>>> as required to keep the threshold at 8.  If this reduction occurs
>>> in setup, a dev_warn is issued to let the user know; if the
>>> reduction happens in pump_transfers, the change is silent.  Change
>>> the reporting if you think something else is better.
>>> 
>>> [snip] 
>>>
>>> ----Testing
>>> 
>>> This patch has been tested for reading the correct number of bytes 
>>> without overruns or permanent timeouts; I did not use loopback and
>>> pass test data to verify on read, but I could do that.  The test
>>> was done with randomized transfer lengths between 4 and >4096 bytes
>>> (in multiples of 4 bytes, I suppose I could do it in integer
>>> bytes), with all word sizes from 4 to 32 bits.  Baud rate was
>>> changed from 3.6864MHZ to 100kHz.  Both DMA and PIO mode were
>>> tested.  In DMA mode various combinations of word size and burst
>>> size were used.  In PIO mode various threshold settings from tx=1
>>> and rx=15, to tx=15 and rx=1 were tested. All modes worked.  With
>>> PIO mode fixed to prevent overruns, the operation becomes
>>> insensitive to values of threshold and timeout.  In DMA mode, it is
>>> necessary to keep timeout longer than one word time

How do I calculate this? What is the symptom if it is too short?

>I think the  "1" and "15" above correspond to threshold register values
>of "0" and "14"; that is the above numbers are the number of words in
>the FIFO that trigger the interrupt/service request.

All this still leaves me slightly confused. (And I didn't even touch the
dma_burst_size member yet.) I assume that the correct value for all of
these settings must be determined empirically, not just theoretically.
How can I tell what the correct or optimum values are? Do I just use a
performance test on the Flash chip or MMC device? Or can I look at the
logic analyzer output and see what they should be? Or is there a debug
printk that needs to be enabled?

To the point of my setup, I may not have the right settings yet because 
the clock is stopping between each byte transferred. See attached logic
analyzer printout. Is this normal? Does this indicate a setting that
needs to be tweaked? Is the DMA not keeping up? The chip should be
capable of up to 75MHz, and in the printout, it is obvious that the
chip is ready before the PXA. I also tested PIO mode and it looks
identical.

The gaps indicate a 1MHZ period so I tried 1MHz. That was fine. I tried
2MHz and it is still fine. Requesting 4MHz (actual 3.125), there is a
short pause after the first byte, but then it is fine. It might be a PXA
SSP controller "bug"?

<opinion> As a user, it would seem that the SSP should work with
minimum fuss. If there are values that enable 99% of devices to work,
then why would you *not* want them to be the defaults? If the user
needs to tweak the values because they need more performance, then they
can. But if they only need a low bandwidth connection, causing
transfers to hang because they did not provide a "know-able" value
seems less than useful. I know it took me entirely too long to get this
working. I am only trying to help the next user that needs this.
</opinion>

I'll follow up on the MTD bug on the other thread.

--
Thanks again for your help and have a good day,
Vern

[-- Attachment #2: SpiStutter.PNG --]
[-- Type: image/png, Size: 31914 bytes --]

[-- 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: pxa2xx_spi with SFRM

[Ned Forrester]

Vernon Sauder wrote:
> Ned Forrester wrote:
> 
>> I'm not sure that implementing defaults is a good idea.  I prefer the
>> current scheme of forcing the user to make a considered choice.  In
>> particular, 0 is a valid value for the timeout and thresholds (it means
>> none in the case of timeout and one in the case of thresholds), so if
>> these structure elements weren't already defined as unsigned, -1 would
>> be a better flag to trigger use of a default.
> 
> I added the defaults because there is an inconsistency in the API. When
> I added cs_control, it did not force me to make a "considered
> choice"; it forced me to debug a driver to determine why it didn't
> work anymore. The .h file mentions that the settings are for "DMA
> tuning" so I did not initialize them. I only added cs_control and left
> the other members zero. Either the code or the documentation needs a
> small update, IMHO. I can send a patch but it looks like I might be
> duplicating work that has already been done and got lost somewhere.

Granted, documentation can always be improved.  I don't know of any
other proposed patches that would address your concern.

> Zero is not a valid timeout if the driver relies on the timeout
> interrupt to receive trailing bytes. If timeout is zero, the
> timeout interrupt will not happen, right?

Whether zero is valid depends on the application.  If the data never
stop and the user does not want a timeout interrupt to be serviced then
either zero or a large number is appropriate; alternatively, if the data
always come in bursts that exactly match the FIFO threshold, then a
timeout is not required.  I freely admit that both of these scenarios
border on implausible, and in each case a long timeout would suffice.

The substantive issue is that Intel/Marvel failed to correctly/clearly
identify the clock that is used for the timeout.  This makes it
impossible to put a number in the register that means anything.  The
number might even have to change for each member of PXA series, but we
don't have enough information to know that.

The other issue is that one application's trailing byte timeout is
another application's disrupted transfer.  That is to say, what is long
enough for one application is likely too short for another, and
likewise: what is long enough for the slow application is likely too
long for a fast one.

How about you propose a number to use a default.  I can't think of a
good value that would work for most.  I use 10000 which translates to
100us on a PXA255, but I don't know what that means on other processors,
and it may not be long enough for some PIO applications.

> A threshold of 0 is invalid. It will actually become 16:
> 
> #define SSCR1_TxTresh(x) (((x) - 1) << 6) /* level [1..16] */

Darn, I forgot about the macro.  I dealt with all the basics of this
driver about 2 years ago, and I have forgotten much.

>> More specifically, I don't see in the patch where there is a default
>> for timeout; you simply leave it undefined if a value of 0 was
>> specified; there is no "else" for "if (chip_info->timeout)".  
> 
> The timeout default is set a couple lines above when the chip struct is
> malloc'd. The patch just does not overwrite it.

Sorry, I forgot that, too.  I see that Stephen did put 1000 in the code,
but that dates from the time that he was trying to scale it to
microseconds and the value was not changed when he removed the scaling.
 Now that we know that the clock on a PXA255 is 99.5MHz and not
3.6864MHz, 1000 is probably much too small for a default (10us).  The
problem is that we don't know the timer clock on any other processor in
the PXA series.  If you want to add to the knowledge you can measure it
for the PXA270.

>> In any
>> case, it would be difficult at best to offer a default value, since
>> this is a division of an unspecified clock.  It turns out that the
>> developer's manuals for both PXA255 and PXA270 do not adequately
>> specify what clock is used for the timer.  Thus a numerical value that
>> is appropriate for one may not be appropriate for another, nor for the
>> new PXA3xx processors.  We just don't know.  Furthermore, an
>> appropriate default depends on the rate of SSPSCLK, which is a
>> variable.  There are also application specific considerations.
> 
> Which again leads me to ask, how would I know if the value I picked is
> good, or right, or wrong?

If you need faster response to trailing bytes, make the value smaller.
If you don't want timeouts to happen in the middle of a transfer,
probably terminating the transfer early, make it longer.  If we knew
what the scaling to real time was supposed to be on all PXA processors,
then it would be easier for users to pick an appropriate value based on
how long they would like to wait before assuming that there are only
trailing bytes left to process.  Even the driver author does not know
what the processor load is on the target machine, so it is hard to know
how long one should wait to declare the transfer done.

I agree that it is tricky to know what the driver is doing without
putting in GPIO probes (for a scope) or additional printks.  If a
timeout happens too soon in a transaction, the remainder of the
transaction will happen in polled mode, which is likely to be slow.

>> The defaults of 12 and 4 that you specified above for thresholds are
>> not actually the preferred values.  When I was working with Stephen on
>> some issues with this driver, we concluded that 8 and 8 were better
>> choices.
> 
> The 12/4 values are the defaults used in _probe(). No real comment there
> on why those values are chosen. The defaults if chip_info is missing are
> 1 & 1. All these defaults could be consolidated a little. :)  And maybe
> explained. Or maybe not needed at all if the platform *must* pass them
> in!

I guess the default value business is really half baked, a historical
accident.  probe() sets some hardware registers, but nothing is really
used until pump_transfers() loads the first transfer, and that is done
from values set in setup().  Probably all these values should be set
from the same #define to avoid this confusion.  12/4 will work for PIO
mode, but 8/8 is better for DMA and will also work for PIO (at least on
the PXA255, where the maximum SSPSCLK is 3.6864MHz; I think the PXA270
can have an internal clock of 13MHZ).

>> I am curious about the kernel hanging with a zero timeout.  I would
>> expect that the trailing bytes would never be read, but this driver
>> should not hang waiting for them, because it simply is not responding
>> to an interrupt that never happens.  Perhaps the hang occurs at a
>> higher level when the message is never given back.  Do you really mean
>> that the kernel hangs (all processes stop) or just that the MMC
>> operation never finishes?  Are you using PIO (interrupt) mode or DMA
>> when you access the driver (or is that hidden from you by the MMC
>> drivers that you are using)?
> 
> I believe this was PIO. You are correct, only the MMC hung, not the
> kernel, my bad. I think it was stuck in this code:
> 
> static irqreturn_t interrupt_transfer(struct driver_data *drv_data)
> <snip>
> 	if (drv_data->tx == drv_data->tx_end) {
> 		write_SSCR1(read_SSCR1(reg) & ~SSCR1_TIE, reg);
> 		/* PXA25x_SSP has no timeout, read trailing bytes */
> 		if (drv_data->ssp_type == PXA25x_SSP) {
> 			if (!wait_ssp_rx_stall(reg))
> 			{
> 				int_error_stop(drv_data, "interrupt_transfer: "
> 						"rx stall failed");
> 				return IRQ_HANDLED;
> 			}
> 			if (!drv_data->read(drv_data))
> 			{
> 				int_error_stop(drv_data,
> 						"interrupt_transfer: "
> 						"trailing byte read failed");
> 				return IRQ_HANDLED;
> 			}
> 			int_transfer_complete(drv_data);
> 		}
> 	}
> 
> 	/* We did something */
> 	return IRQ_HANDLED;
> }
> 
> It basically thought the TX was done but it was missing RX data. I can
> try to recreate this if you need.

I don't think it can be "stuck" there.  Surely if the timeout interrupt
never happens, the driver will never read the trailing bytes and thus
will never giveback the message, but the code fragment above has no
loops in it, and it is only executed when there is an interrupt to
service.  All the above code does is turn off the TX interrupt if all
characters have been transmitted.  (The rest reads trailing bytes on the
SSP (not NSSP) port of a PXA25x, but that type of port does not even
exist on a PXA270).  This code turns off tx interrupts and then the
driver waits for the already enabled timeout interrupt (which never
happens if timeout is zero).

>>> ** Thresholds are now as set by defaults. Do you have a good way to 
>>> figure out what they should be?
>> See comments above about good values of thresholds, and defaults as
>> policy.  In general, I found the driver fairly robust for all values of
>> the thresholds when in PIO mode.  Personally, I use 8 and 8 for
>> 11Mbit/sec DMA mode. I think it is fair to use whatever works in your
>> application.  Here is a slightly relevant comment from my discussion
>> with Stephen on Oct 1, 2006 (which somehow never made onto the
>> spi-devel mailing list, in spite of being addressed there):
>>
>>>>
>>>> [snip] 
>>>>
> 
> How do I calculate this? What is the symptom if it is too short?

Do you mean the timeout or the threshold?

In PIO mode, if the threshold is too low, too many interrupts are
fielded, and processor load increases.  If the threshold is too large,
there is more danger of RX FIFO overruns, though I don't think that can
actually happen anymore in PIO mode (since the 1/27/07 commit-date patch).

In DMA mode, the threshold must be set so that there is TX room in the
FIFO for burst-size bytes, or sufficient words in the RX FIFO for
burst-size bytes. (remembering that that the FIFO is 16 words deep,
regardless of word size, while the burst size is measured in bytes).
The frequency with which the DMA engine must service the FIFO is
determined more by the burst size than by the threshold.  If the first
rule is followed, then I don't think that there can be any
overrun/underrun occurrences.  Note that DMA thresholds are computed
internally by the driver for DMA mode, based on the requested burst
size; poor choices of burst size are over-ridden, and if that happens in
setup (rather than per-transfer), a kernel warning is issued.

With the timeout, if it is too short, and it occurs in the middle of a
transfer, rather than for just the trailing bytes, then the interrupt
service routine will read until the FIFO is empty and then stop.  This
will likely cause the transfer to be short with the actual_length
parameter having the expected value.  If the timeout is too long, then
you are forced to wait for the data, making the operation of the
interface slow.

>> I think the  "1" and "15" above correspond to threshold register values
>> of "0" and "14"; that is the above numbers are the number of words in
>> the FIFO that trigger the interrupt/service request.
> 
> All this still leaves me slightly confused. (And I didn't even touch the
> dma_burst_size member yet.) I assume that the correct value for all of
> these settings must be determined empirically, not just theoretically.
> How can I tell what the correct or optimum values are? Do I just use a
> performance test on the Flash chip or MMC device? Or can I look at the
> logic analyzer output and see what they should be? Or is there a debug
> printk that needs to be enabled?

I think it is empirical for the most part.  I realize that is not a very
satisfactory answer, but this is a relatively new driver, and my sense
is that only a dozen or so people are using it, each for a very targeted
purpose (maybe there are many silent users, but as you found, it is hard
to use this silently; it needs help).  Perhaps the ability to attach
memory cards will make that the "default" application that the driver
should be tuned for, though it was originally written for handling A/D
converters.

> To the point of my setup, I may not have the right settings yet because 
> the clock is stopping between each byte transferred. See attached logic
> analyzer printout. Is this normal? 

Hard to say.  I only use DMA mode, transferring blocks of 1024 32-bit
words.  I suspect that you are using PIO mode (not sure on that).  Still
I am puzzled about the gaps between bytes, even in PIO mode, I would
think the driver would load the TX FIFO faster than 1us.  Gaps between
FIFO loads don't surprise me; interrupt latency for Linux running on a
PXA255 seems to be about 100-200us on average.

> Does this indicate a setting that
> needs to be tweaked? Is the DMA not keeping up? 

Is this really DMA mode?  I can't see why there would be gaps unless the
 burst was set low.  I have never actually looked at the bus, so I don't
know if this is normal.  My system runs on an external 11.2MHz clock,
and uses read-without-transmit (RWOT) (this driver is really runs as a
slave, but it does not have to know that) so there is no way to measure
what the DMA engine is doing internally between the FIFO and the memory.

Burst and threshold should have defaulted to 8/8/8 for DMA mode and
8-bit transfers.  Those should be the best settings.

In this protocol, are the transfers that you show single byte transfers
or should they have been many bytes?  If each is a single transfer, then
I am not surprise to see 1us gaps; there is a lot more processing that
goes on between transfers, than between the bytes of one transfer.

I debug this stuff with a large /proc file that shows me all the
register and variable values at the last entry to an interrupt routine.
 This is one of the things that makes my version of the driver huge, by
comparison with the standard driver. Every time I look back at where I
started, I am surprised by the simplicity of the original! :-)

> The chip should be
> capable of up to 75MHz, and in the printout, it is obvious that the
> chip is ready before the PXA. I also tested PIO mode and it looks
> identical.

Since the interface only operates a 13MHz max, I doubt you expect to
reach 75MHz.  :-)

> The gaps indicate a 1MHZ period so I tried 1MHz. That was fine. I tried
> 2MHz and it is still fine. Requesting 4MHz (actual 3.125), there is a
> short pause after the first byte, but then it is fine. It might be a PXA
> SSP controller "bug"?

OK, now I am confused again.  Your trace shows about 600ns/byte, which
is a bit-clock of 13MHz, not 1, 2 or 3.1.  I'm not sure what you are
asking here.  I am not aware of any bugs that would make the interface
slow.  Are you saying that the byte rate is actually faster if you
request a slower SSPSCLK?

> <opinion> As a user, it would seem that the SSP should work with
> minimum fuss. If there are values that enable 99% of devices to work,
> then why would you *not* want them to be the defaults? If the user
> needs to tweak the values because they need more performance, then they
> can. But if they only need a low bandwidth connection, causing
> transfers to hang because they did not provide a "know-able" value
> seems less than useful. I know it took me entirely too long to get this
> working. I am only trying to help the next user that needs this.
> </opinion>

I agree with your sentiment.  What I am not sure of is whether there are
even 90% of applications that could use a single group of settings.  I
think the DMA burst and the thresholds could likely be set to reasonable
defaults: burst = bits/word (for 8, 16, and 32 bit words, this results
in burst equaling half the FIFO), and thresholds of 8/8 (again, half the
FIFO).  For the timeout, I'm less sure of the best approach; probably it
should be very long, a millisecond or more (if we know what that
translates to on a PXA270), and let users reduce it if they want faster
response.  My main concern is the poor documentation of this in the PXA
developer's manuals.

-- 
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: pxa2xx_spi with SFRM

[Ned Forrester]

Ned Forrester wrote:
> Vernon Sauder wrote:
>> <opinion> As a user, it would seem that the SSP should work with
>> minimum fuss. If there are values that enable 99% of devices to work,
>> then why would you *not* want them to be the defaults? If the user
>> needs to tweak the values because they need more performance, then they
>> can. But if they only need a low bandwidth connection, causing
>> transfers to hang because they did not provide a "know-able" value
>> seems less than useful. I know it took me entirely too long to get this
>> working. I am only trying to help the next user that needs this.
>> </opinion>
> 
> I agree with your sentiment.  What I am not sure of is whether there are
> even 90% of applications that could use a single group of settings.  I
> think the DMA burst and the thresholds could likely be set to reasonable
> defaults: burst = bits/word (for 8, 16, and 32 bit words, this results
> in burst equaling half the FIFO), and thresholds of 8/8 (again, half the
> FIFO).  For the timeout, I'm less sure of the best approach; probably it
> should be very long, a millisecond or more (if we know what that
> translates to on a PXA270), and let users reduce it if they want faster
> response.  My main concern is the poor documentation of this in the PXA
> developer's manuals.

Vernon, have you had any more thoughts about this.  I am working on a
patch to deal with problems that were discovered a while ago, and which
finally tripped up another user (Re: [spi-devel-general] [PATCH]
pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode).  I
wondered if we are ready to converge on any changes.  On the one hand it
would be easy to throw improvement in the defaults in the with the other
changes, but on the other hand I think they are independent, and so they
should probably be in separate patches.

As a consequence of studying the driver for the other patch (regarding
transfer delays and chip select), I have a new understanding of the
timeout that simplifies the problems I discussed above.  I am now
reasonably sure (but cannot test) that a timeout value that is "too
short" will only result in excess interrupt service for unneeded
timeouts, and not in early termination of a transfer.  In
interrupt_transfer, acceptance of a timeout interrupt is conditioned on
being able to retrieve all expected characters from the RX FIFO; in
dma_transfer, acceptance is similarly conditioned on the whole transmit
process being complete (and thus the receive process is done or will be
in nsec).

The result is that neither a too short, nor a too long timeout is fatal,
and thus it IS reasonable to more firmly establish a default timeout,
even it is short.

-- 
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: pxa2xx_spi with SFRM

[Vernon Sauder]

Ned Forrester wrote:

>Ned Forrester wrote:
>> Vernon Sauder wrote:
>>> <opinion> As a user, it would seem that the SSP should work with
>>> minimum fuss. If there are values that enable 99% of devices to
>>> work, then why would you *not* want them to be the defaults? If the
>>> user needs to tweak the values because they need more performance,
>>> then they can. But if they only need a low bandwidth connection,
>>> causing transfers to hang because they did not provide a
>>> "know-able" value seems less than useful. I know it took me
>>> entirely too long to get this working. I am only trying to help the
>>> next user that needs this. </opinion>
>> 
>> I agree with your sentiment.  What I am not sure of is whether there
>> are even 90% of applications that could use a single group of
>> settings.  I think the DMA burst and the thresholds could likely be
>> set to reasonable defaults: burst = bits/word (for 8, 16, and 32 bit
>> words, this results in burst equaling half the FIFO), and thresholds
>> of 8/8 (again, half the FIFO).  For the timeout, I'm less sure of
>> the best approach; probably it should be very long, a millisecond or
>> more (if we know what that translates to on a PXA270), and let users
>> reduce it if they want faster response.  My main concern is the poor
>> documentation of this in the PXA developer's manuals.
>
>Vernon, have you had any more thoughts about this.  I am working on a
>patch to deal with problems that were discovered a while ago, and which
>finally tripped up another user (Re: [spi-devel-general] [PATCH]
>pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode).  I
>wondered if we are ready to converge on any changes.  On the one hand
>it would be easy to throw improvement in the defaults in the with the
>other changes, but on the other hand I think they are independent, and
>so they should probably be in separate patches.
>
>As a consequence of studying the driver for the other patch (regarding
>transfer delays and chip select), I have a new understanding of the
>timeout that simplifies the problems I discussed above.  I am now
>reasonably sure (but cannot test) that a timeout value that is "too
>short" will only result in excess interrupt service for unneeded
>timeouts, and not in early termination of a transfer.  In
>interrupt_transfer, acceptance of a timeout interrupt is conditioned on
>being able to retrieve all expected characters from the RX FIFO; in
>dma_transfer, acceptance is similarly conditioned on the whole transmit
>process being complete (and thus the receive process is done or will be
>in nsec).
>
>The result is that neither a too short, nor a too long timeout is
>fatal, and thus it IS reasonable to more firmly establish a default
>timeout, even it is short.
>

Would you be in favor of a patch like this? If the code does not
match the documentation, let me know if the documentation is correct. I
can then figure out what the code should do.

Regards,
Vern
---
pxa2xx_spi: Fix chip_info defaults and documentation.

Make the chip info structure data optional by providing reasonable
defaults. Improve corresponding documentation.

DMA can determine appropriate dma_burst_size and thresholds
automatically so use DMA even if dma_burst_size is not specified.

Signed-off-by: Vernon Sauder <VernonInHand-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
--- linux-2.6.24.4/drivers/spi/pxa2xx_spi.c	2008-03-24 14:49:18.000000000 -0400
+++ src.cache/drivers/spi/pxa2xx_spi.c	2008-08-20 13:20:19.000000000 -0400
@@ -44,6 +44,10 @@
 
 #define MAX_BUSES 3
 
+#define RX_THRESH_DFLT 	8
+#define TX_THRESH_DFLT 	8
+#define TIMOUT_DFLT		1000
+
 #define DMA_INT_MASK (DCSR_ENDINTR | DCSR_STARTINTR | DCSR_BUSERR)
 #define RESET_DMA_CHANNEL (DCSR_NODESC | DMA_INT_MASK)
 #define IS_DMA_ALIGNED(x) (((u32)(x)&0x07)==0)
@@ -1126,8 +1130,7 @@
 
 		chip->cs_control = null_cs_control;
 		chip->enable_dma = 0;
-		chip->timeout = 1000;
-		chip->threshold = SSCR1_RxTresh(1) | SSCR1_TxTresh(1);
+		chip->timeout = TIMOUT_DFLT;
 		chip->dma_burst_size = drv_data->master_info->enable_dma ?
 					DCMD_BURST8 : 0;
 	}
@@ -1138,25 +1141,28 @@
 
 	/* chip_info isn't always needed */
 	chip->cr1 = 0;
+	uint tx_thres = TX_THRESH_DFLT;
+	uint rx_thres = RX_THRESH_DFLT;
 	if (chip_info) {
 		if (chip_info->cs_control)
 			chip->cs_control = chip_info->cs_control;
-
- 		chip->timeout = chip_info->timeout;
+		if (chip_info->timeout)
+ 			chip->timeout = chip_info->timeout;
+		if (chip_info->tx_threshold)
+			tx_thres = chip_info->tx_threshold;
+		if (chip_info->rx_threshold)
+			rx_thres = chip_info->rx_threshold;
 
-		chip->threshold = (SSCR1_RxTresh(chip_info->rx_threshold) &
-								SSCR1_RFT) |
-				(SSCR1_TxTresh(chip_info->tx_threshold) &
-								SSCR1_TFT);
-
-		chip->enable_dma = chip_info->dma_burst_size != 0
-					&& drv_data->master_info->enable_dma;
+		chip->enable_dma = drv_data->master_info->enable_dma;
 		chip->dma_threshold = 0;
 
 		if (chip_info->enable_loopback)
 			chip->cr1 = SSCR1_LBM;
 	}
 
+	chip->threshold = (SSCR1_RxTresh(rx_thres) & SSCR1_RFT) |
+			(SSCR1_TxTresh(tx_thres) & SSCR1_TFT);
+
 	/* set dma burst and threshold outside of chip_info path so that if
 	 * chip_info goes away after setting chip->enable_dma, the
 	 * burst and threshold can still respond to changes in bits_per_word */
@@ -1196,17 +1202,19 @@
 
 	/* NOTE:  PXA25x_SSP _could_ use external clocking ... */
 	if (drv_data->ssp_type != PXA25x_SSP)
-		dev_dbg(&spi->dev, "%d bits/word, %d Hz, mode %d\n",
+		dev_dbg(&spi->dev, "%d bits/word, %d Hz, mode %d, %s\n",
 				spi->bits_per_word,
 				(CLOCK_SPEED_HZ)
 					/ (1 + ((chip->cr0 & SSCR0_SCR) >> 8)),
-				spi->mode & 0x3);
+				spi->mode & 0x3,
+				chip->enable_dma ? "DMA" : "PIO");
 	else
-		dev_dbg(&spi->dev, "%d bits/word, %d Hz, mode %d\n",
+		dev_dbg(&spi->dev, "%d bits/word, %d Hz, mode %d, %s\n",
 				spi->bits_per_word,
 				(CLOCK_SPEED_HZ/2)
 					/ (1 + ((chip->cr0 & SSCR0_SCR) >> 8)),
-				spi->mode & 0x3);
+				spi->mode & 0x3,
+				chip->enable_dma ? "DMA" : "PIO");
 
 	if (spi->bits_per_word <= 8) {
 		chip->n_bytes = 1;
@@ -1457,7 +1465,8 @@
 
 	/* Load default SSP configuration */
 	write_SSCR0(0, drv_data->ioaddr);
-	write_SSCR1(SSCR1_RxTresh(4) | SSCR1_TxTresh(12), drv_data->ioaddr);
+	write_SSCR1(SSCR1_RxTresh(RX_THRESH_DFLT) | SSCR1_TxTresh(TX_THRESH_DFLT),
+				drv_data->ioaddr);
 	write_SSCR0(SSCR0_SerClkDiv(2)
 			| SSCR0_Motorola
 			| SSCR0_DataSize(8),
--- linux-2.6.24.4/Documentation/spi/pxa2xx	2008-03-24 14:49:18.000000000 -0400
+++ src.cache/Documentation/spi/pxa2xx	2008-08-20 13:14:01.000000000 -0400
@@ -96,7 +96,7 @@
 information via the structure "pxa2xx_spi_chip" found in
 "include/asm-arm/arch-pxa/pxa2xx_spi.h".  The pxa2xx_spi master controller driver
 will uses the configuration whenever the driver communicates with the slave
-device.
+device. All fields are optional.
 
 struct pxa2xx_spi_chip {
 	u8 tx_threshold;
@@ -112,14 +112,17 @@
 performance of pxa2xx_spi driver and misconfiguration will result in rx
 fifo overruns (especially in PIO mode transfers). Good default values are
 
-	.tx_threshold = 12,
-	.rx_threshold = 4,
+	.tx_threshold = 8,
+	.rx_threshold = 8,
+
+The range is 1 to 16 where zero indicates "use default".
 
 The "pxa2xx_spi_chip.dma_burst_size" field is used to configure PXA2xx DMA
 engine and is related the "spi_device.bits_per_word" field.  Read and understand
 the PXA2xx "Developer Manual" sections on the DMA controller and SSP Controllers
 to determine the correct value. An SSP configured for byte-wide transfers would
-use a value of 8.
+use a value of 8. The driver will determine a reasonable default if
+dma_burst_size == 0.
 
 The "pxa2xx_spi_chip.timeout" fields is used to efficiently handle
 trailing bytes in the SSP receiver fifo.  The correct value for this field is
@@ -135,7 +138,10 @@
 The "pxa2xx_spi_chip.cs_control" field is used to point to a board specific
 function for asserting/deasserting a slave device chip select.  If the field is
 NULL, the pxa2xx_spi master controller driver assumes that the SSP port is
-configured to use SSPFRM instead.
+configured to use SSPFRM instead. NOTE: the SPI driver cannot control the chip
+select if SSPFRM is used so each transfer will assert/deassert SSPFRM around it.
+Many devices need chip select asserted around the complete message. The 
+cs_control method should use the SSPFRM pin as a GPIO to accomodate these chips.
 
 NSSP SALVE SAMPLE
 -----------------
@@ -206,16 +212,15 @@
 
 DMA and PIO I/O Support
 -----------------------
-The pxa2xx_spi driver support both DMA and interrupt driven PIO message
+The pxa2xx_spi driver supports both DMA and interrupt driven PIO message
 transfers.  The driver defaults to PIO mode and DMA transfers must enabled by
-setting the "enable_dma" flag in the "pxa2xx_spi_master" structure and
-ensuring that the "pxa2xx_spi_chip.dma_burst_size" field is non-zero.  The DMA
+setting the "enable_dma" flag in the "pxa2xx_spi_master" structure.  The DMA
 mode support both coherent and stream based DMA mappings.
 
 The following logic is used to determine the type of I/O to be used on
 a per "spi_transfer" basis:
 
-if !enable_dma or dma_burst_size == 0 then
+if !enable_dma then
 	always use PIO transfers
 
 if spi_message.is_dma_mapped and rx_dma_buf != 0 and tx_dma_buf != 0 then

-------------------------------------------------------------------------
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: pxa2xx_spi with SFRM

[Ned Forrester]

Vernon Sauder wrote:
> Ned Forrester wrote:
> 
>> Ned Forrester wrote:
>>> Vernon Sauder wrote:
>>>> <opinion> As a user, it would seem that the SSP should work with
>>>> minimum fuss. If there are values that enable 99% of devices to
>>>> work, then why would you *not* want them to be the defaults? If the
>>>> user needs to tweak the values because they need more performance,
>>>> then they can. But if they only need a low bandwidth connection,
>>>> causing transfers to hang because they did not provide a
>>>> "know-able" value seems less than useful. I know it took me
>>>> entirely too long to get this working. I am only trying to help the
>>>> next user that needs this. </opinion>
>>> I agree with your sentiment.  What I am not sure of is whether there
>>> are even 90% of applications that could use a single group of
>>> settings.  I think the DMA burst and the thresholds could likely be
>>> set to reasonable defaults: burst = bits/word (for 8, 16, and 32 bit
>>> words, this results in burst equaling half the FIFO), and thresholds
>>> of 8/8 (again, half the FIFO).  For the timeout, I'm less sure of
>>> the best approach; probably it should be very long, a millisecond or
>>> more (if we know what that translates to on a PXA270), and let users
>>> reduce it if they want faster response.  My main concern is the poor
>>> documentation of this in the PXA developer's manuals.
>> Vernon, have you had any more thoughts about this.  I am working on a
>> patch to deal with problems that were discovered a while ago, and which
>> finally tripped up another user (Re: [spi-devel-general] [PATCH]
>> pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode).  I
>> wondered if we are ready to converge on any changes.  On the one hand
>> it would be easy to throw improvement in the defaults in the with the
>> other changes, but on the other hand I think they are independent, and
>> so they should probably be in separate patches.
>>
>> As a consequence of studying the driver for the other patch (regarding
>> transfer delays and chip select), I have a new understanding of the
>> timeout that simplifies the problems I discussed above.  I am now
>> reasonably sure (but cannot test) that a timeout value that is "too
>> short" will only result in excess interrupt service for unneeded
>> timeouts, and not in early termination of a transfer.  In
>> interrupt_transfer, acceptance of a timeout interrupt is conditioned on
>> being able to retrieve all expected characters from the RX FIFO; in
>> dma_transfer, acceptance is similarly conditioned on the whole transmit
>> process being complete (and thus the receive process is done or will be
>> in nsec).
>>
>> The result is that neither a too short, nor a too long timeout is
>> fatal, and thus it IS reasonable to more firmly establish a default
>> timeout, even it is short.
>>
> 
> Would you be in favor of a patch like this? If the code does not
> match the documentation, let me know if the documentation is correct. I
> can then figure out what the code should do.
> 
> Regards,
> Vern

Looks fine, except as noted below...

Also, you should make patches with something equivalent to:

rm -f $patch_file $tmp_file
LC_ALL=C TZ=UTC0 diff -Nurp $src_dir $patch_dir > $tmp_file
cat $patch_text > $patch_file
diffstat -p 1 -w 70 $tmp_file >> $patch_file
echo "" >> $patch_file
cat $tmp_file >> $patch_file
rm $tmp_file

Note the environment variables and flags passed to diff, and the use of
diffstat.

> ---
> pxa2xx_spi: Fix chip_info defaults and documentation.
> 
> Make the chip info structure data optional by providing reasonable
> defaults. Improve corresponding documentation.
> 
> DMA can determine appropriate dma_burst_size and thresholds
> automatically so use DMA even if dma_burst_size is not specified.
> 
> Signed-off-by: Vernon Sauder <VernonInHand-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> --- linux-2.6.24.4/drivers/spi/pxa2xx_spi.c	2008-03-24 14:49:18.000000000 -0400
> +++ src.cache/drivers/spi/pxa2xx_spi.c	2008-08-20 13:20:19.000000000 -0400
> @@ -44,6 +44,10 @@
>  
>  #define MAX_BUSES 3
>  
> +#define RX_THRESH_DFLT 	8
> +#define TX_THRESH_DFLT 	8
> +#define TIMOUT_DFLT		1000
> +
>  #define DMA_INT_MASK (DCSR_ENDINTR | DCSR_STARTINTR | DCSR_BUSERR)
>  #define RESET_DMA_CHANNEL (DCSR_NODESC | DMA_INT_MASK)
>  #define IS_DMA_ALIGNED(x) (((u32)(x)&0x07)==0)
> @@ -1126,8 +1130,7 @@
>  
>  		chip->cs_control = null_cs_control;
>  		chip->enable_dma = 0;
> -		chip->timeout = 1000;
> -		chip->threshold = SSCR1_RxTresh(1) | SSCR1_TxTresh(1);
> +		chip->timeout = TIMOUT_DFLT;
>  		chip->dma_burst_size = drv_data->master_info->enable_dma ?
>  					DCMD_BURST8 : 0;
>  	}
> @@ -1138,25 +1141,28 @@
>  
>  	/* chip_info isn't always needed */
>  	chip->cr1 = 0;
> +	uint tx_thres = TX_THRESH_DFLT;
> +	uint rx_thres = RX_THRESH_DFLT;
>  	if (chip_info) {
>  		if (chip_info->cs_control)
>  			chip->cs_control = chip_info->cs_control;
> -
> - 		chip->timeout = chip_info->timeout;
> +		if (chip_info->timeout)
> + 			chip->timeout = chip_info->timeout;
> +		if (chip_info->tx_threshold)
> +			tx_thres = chip_info->tx_threshold;
> +		if (chip_info->rx_threshold)
> +			rx_thres = chip_info->rx_threshold;
>  
> -		chip->threshold = (SSCR1_RxTresh(chip_info->rx_threshold) &
> -								SSCR1_RFT) |
> -				(SSCR1_TxTresh(chip_info->tx_threshold) &
> -								SSCR1_TFT);
> -
> -		chip->enable_dma = chip_info->dma_burst_size != 0
> -					&& drv_data->master_info->enable_dma;
> +		chip->enable_dma = drv_data->master_info->enable_dma;
>  		chip->dma_threshold = 0;
>  
>  		if (chip_info->enable_loopback)
>  			chip->cr1 = SSCR1_LBM;
>  	}
>  
> +	chip->threshold = (SSCR1_RxTresh(rx_thres) & SSCR1_RFT) |
> +			(SSCR1_TxTresh(tx_thres) & SSCR1_TFT);
> +
>  	/* set dma burst and threshold outside of chip_info path so that if
>  	 * chip_info goes away after setting chip->enable_dma, the
>  	 * burst and threshold can still respond to changes in bits_per_word */
> @@ -1196,17 +1202,19 @@
>  
>  	/* NOTE:  PXA25x_SSP _could_ use external clocking ... */
>  	if (drv_data->ssp_type != PXA25x_SSP)
> -		dev_dbg(&spi->dev, "%d bits/word, %d Hz, mode %d\n",
> +		dev_dbg(&spi->dev, "%d bits/word, %d Hz, mode %d, %s\n",
>  				spi->bits_per_word,
>  				(CLOCK_SPEED_HZ)
>  					/ (1 + ((chip->cr0 & SSCR0_SCR) >> 8)),
> -				spi->mode & 0x3);
> +				spi->mode & 0x3,
> +				chip->enable_dma ? "DMA" : "PIO");

Oops! You are not patching the latest version of the code.  This version
predates 1/26/08.  CLOCK_SPEED_HZ is no longer part of the code.  The
patch will be rejected if it does not apply against the current git
tree.  The latest version can be found at:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob_plain;f=drivers/spi/pxa2xx_spi.c;hb=HEAD

>  	else
> -		dev_dbg(&spi->dev, "%d bits/word, %d Hz, mode %d\n",
> +		dev_dbg(&spi->dev, "%d bits/word, %d Hz, mode %d, %s\n",
>  				spi->bits_per_word,
>  				(CLOCK_SPEED_HZ/2)

I note that when this line was changed in 1/26/08 commit, the author of
that patch left out the /2, leaving the two branches of this if/else
identical.  Please restore the /2 when you submit this patch.

>  					/ (1 + ((chip->cr0 & SSCR0_SCR) >> 8)),
> -				spi->mode & 0x3);
> +				spi->mode & 0x3,
> +				chip->enable_dma ? "DMA" : "PIO");
>  
>  	if (spi->bits_per_word <= 8) {
>  		chip->n_bytes = 1;
> @@ -1457,7 +1465,8 @@
>  
>  	/* Load default SSP configuration */
>  	write_SSCR0(0, drv_data->ioaddr);
> -	write_SSCR1(SSCR1_RxTresh(4) | SSCR1_TxTresh(12), drv_data->ioaddr);
> +	write_SSCR1(SSCR1_RxTresh(RX_THRESH_DFLT) | SSCR1_TxTresh(TX_THRESH_DFLT),
> +				drv_data->ioaddr);
>  	write_SSCR0(SSCR0_SerClkDiv(2)
>  			| SSCR0_Motorola
>  			| SSCR0_DataSize(8),
> --- linux-2.6.24.4/Documentation/spi/pxa2xx	2008-03-24 14:49:18.000000000 -0400
> +++ src.cache/Documentation/spi/pxa2xx	2008-08-20 13:14:01.000000000 -0400
> @@ -96,7 +96,7 @@
>  information via the structure "pxa2xx_spi_chip" found in
>  "include/asm-arm/arch-pxa/pxa2xx_spi.h".  The pxa2xx_spi master controller driver

Again, you are trying to patch an obsolete version.  I'm not sure if the
correctly made patch will apply to older versions, but this whole patch
is not essential to back port, as it is a "clarification" and not a bug
fix.  I realize that you consider it a bug fix, and in a way I agree,
but it is not, in the sense that older drivers can be used if all fields
in this structure are filled in.

>  will uses the configuration whenever the driver communicates with the slave
> -device.
> +device. All fields are optional.

Do you need to note that unused fields should be set to zero, or is
these always zeroed by the way the struct pxa2xx_spi_chip is declared
(external or static declaration)?  I have forgotten and you probably
looked at that recently.

>  
>  struct pxa2xx_spi_chip {
>  	u8 tx_threshold;
> @@ -112,14 +112,17 @@
>  performance of pxa2xx_spi driver and misconfiguration will result in rx
>  fifo overruns (especially in PIO mode transfers). Good default values are
>  
> -	.tx_threshold = 12,
> -	.rx_threshold = 4,
> +	.tx_threshold = 8,
> +	.rx_threshold = 8,
> +
> +The range is 1 to 16 where zero indicates "use default".
>  
>  The "pxa2xx_spi_chip.dma_burst_size" field is used to configure PXA2xx DMA
>  engine and is related the "spi_device.bits_per_word" field.  Read and understand
>  the PXA2xx "Developer Manual" sections on the DMA controller and SSP Controllers
>  to determine the correct value. An SSP configured for byte-wide transfers would
> -use a value of 8.
> +use a value of 8. The driver will determine a reasonable default if
> +dma_burst_size == 0.
>  
>  The "pxa2xx_spi_chip.timeout" fields is used to efficiently handle
>  trailing bytes in the SSP receiver fifo.  The correct value for this field is
> @@ -135,7 +138,10 @@
>  The "pxa2xx_spi_chip.cs_control" field is used to point to a board specific
>  function for asserting/deasserting a slave device chip select.  If the field is
>  NULL, the pxa2xx_spi master controller driver assumes that the SSP port is
> -configured to use SSPFRM instead.
> +configured to use SSPFRM instead. NOTE: the SPI driver cannot control the chip
> +select if SSPFRM is used so each transfer will assert/deassert SSPFRM around it.
> +Many devices need chip select asserted around the complete message. The 
> +cs_control method should use the SSPFRM pin as a GPIO to accomodate these chips.

*could* use the SSPFRM pin.  It can also use any other GPIO line, and at
most one could use SSPFRM if there are multiple chips on the bus.

>  
>  NSSP SALVE SAMPLE
>  -----------------
> @@ -206,16 +212,15 @@
>  
>  DMA and PIO I/O Support
>  -----------------------
> -The pxa2xx_spi driver support both DMA and interrupt driven PIO message
> +The pxa2xx_spi driver supports both DMA and interrupt driven PIO message
>  transfers.  The driver defaults to PIO mode and DMA transfers must enabled by

Add "be" between "must enabled".

> -setting the "enable_dma" flag in the "pxa2xx_spi_master" structure and
> -ensuring that the "pxa2xx_spi_chip.dma_burst_size" field is non-zero.  The DMA
> +setting the "enable_dma" flag in the "pxa2xx_spi_master" structure.  The DMA
>  mode support both coherent and stream based DMA mappings.

As long as you are fixing, then "supports" above.

>  
>  The following logic is used to determine the type of I/O to be used on
>  a per "spi_transfer" basis:
>  
> -if !enable_dma or dma_burst_size == 0 then
> +if !enable_dma then
>  	always use PIO transfers

Well, that is true, but the user should probably note that simply
requesting DMA via the enable_dma flag is not sufficient to guarantee
that DMA will be used.  There are a number of criteria in
map_dma_buffers that must be met.  I guess that is what the rest of that
list is trying to say.

Also note that the very recent patch that I submitted, tries to address
a problem that you encountered with transfer lengths longer than 8192;
in that case, I changed the behavior from "fail" to "do it in PIO mode
with rate limited warning".  Some day the driver could be rewritten to
bust long transfers and do the pieces by DMA, but that is too ambitious
for now.  So you could add to the list of states that transfers longer
than 8191 will be PIO.  Have you tested that patch to see if it fixes
any of your other problems?

I'm not sure I believe the words about coherent and stream DMA, but I
will have to check that later.

>  
>  if spi_message.is_dma_mapped and rx_dma_buf != 0 and tx_dma_buf != 0 then
> 
> 


-- 
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: pxa2xx_spi with SFRM

[Vernon Sauder]

Ned,

Sorry for the slow response. Real world/work intrudes sometimes.

Here is another version of the patch along with responses to your
comments.

Ned Forrester wrote:

>Vernon Sauder wrote:
>> 
>> Would you be in favor of a patch like this? If the code does not
>> match the documentation, let me know if the documentation is
>> correct. I can then figure out what the code should do.
>> 
>> Regards,
>> Vern
>
>Looks fine, except as noted below...
>
>Also, you should make patches with something equivalent to:
>
>rm -f $patch_file $tmp_file
>LC_ALL=C TZ=UTC0 diff -Nurp $src_dir $patch_dir > $tmp_file
>cat $patch_text > $patch_file
>diffstat -p 1 -w 70 $tmp_file >> $patch_file
>echo "" >> $patch_file
>cat $tmp_file >> $patch_file
>rm $tmp_file
>
>Note the environment variables and flags passed to diff, and the use of
>diffstat.

Thanks. This time I used git format-patch because I used a git tree.

>> ---
>> pxa2xx_spi: Fix chip_info defaults and documentation.
>> 
>> Make the chip info structure data optional by providing reasonable
>> defaults. Improve corresponding documentation.
>> 
>> DMA can determine appropriate dma_burst_size and thresholds
>> automatically so use DMA even if dma_burst_size is not specified.
>> 
>> Signed-off-by: Vernon Sauder <VernonInHand-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> --- linux-2.6.24.4/drivers/spi/pxa2xx_spi.c	2008-03-24
>> 14:49:18.000000000 -0400 +++
>> src.cache/drivers/spi/pxa2xx_spi.c	2008-08-20
>> 13:20:19.000000000 -0400 @@ -44,6 +44,10 @@ 
>>  #define MAX_BUSES 3
>>  
>> +#define RX_THRESH_DFLT 	8
>> +#define TX_THRESH_DFLT 	8
>> +#define TIMOUT_DFLT		1000
>> +
>>  #define DMA_INT_MASK (DCSR_ENDINTR | DCSR_STARTINTR | DCSR_BUSERR)
>>  #define RESET_DMA_CHANNEL (DCSR_NODESC | DMA_INT_MASK)
>>  #define IS_DMA_ALIGNED(x) (((u32)(x)&0x07)==0)
>> @@ -1126,8 +1130,7 @@
>>  
>>  		chip->cs_control = null_cs_control;
>>  		chip->enable_dma = 0;
>> -		chip->timeout = 1000;
>> -		chip->threshold = SSCR1_RxTresh(1) |
>> SSCR1_TxTresh(1);
>> +		chip->timeout = TIMOUT_DFLT;
>>  		chip->dma_burst_size =
>> drv_data->master_info->enable_dma ? DCMD_BURST8 : 0;
>>  	}
>> @@ -1138,25 +1141,28 @@
>>  
>>  	/* chip_info isn't always needed */
>>  	chip->cr1 = 0;
>> +	uint tx_thres = TX_THRESH_DFLT;
>> +	uint rx_thres = RX_THRESH_DFLT;
>>  	if (chip_info) {
>>  		if (chip_info->cs_control)
>>  			chip->cs_control = chip_info->cs_control;
>> -
>> - 		chip->timeout = chip_info->timeout;
>> +		if (chip_info->timeout)
>> + 			chip->timeout = chip_info->timeout;
>> +		if (chip_info->tx_threshold)
>> +			tx_thres = chip_info->tx_threshold;
>> +		if (chip_info->rx_threshold)
>> +			rx_thres = chip_info->rx_threshold;
>>  
>> -		chip->threshold =
>> (SSCR1_RxTresh(chip_info->rx_threshold) &
>> -
>> SSCR1_RFT) |
>> -
>> (SSCR1_TxTresh(chip_info->tx_threshold) &
>> -
>> SSCR1_TFT); -
>> -		chip->enable_dma = chip_info->dma_burst_size != 0
>> -					&&
>> drv_data->master_info->enable_dma;
>> +		chip->enable_dma =
>> drv_data->master_info->enable_dma; chip->dma_threshold = 0;
>>  
>>  		if (chip_info->enable_loopback)
>>  			chip->cr1 = SSCR1_LBM;
>>  	}
>>  
>> +	chip->threshold = (SSCR1_RxTresh(rx_thres) & SSCR1_RFT) |
>> +			(SSCR1_TxTresh(tx_thres) & SSCR1_TFT);
>> +
>>  	/* set dma burst and threshold outside of chip_info path so
>> that if
>>  	 * chip_info goes away after setting chip->enable_dma, the
>>  	 * burst and threshold can still respond to changes in
>> bits_per_word */ @@ -1196,17 +1202,19 @@
>>  
>>  	/* NOTE:  PXA25x_SSP _could_ use external clocking ... */
>>  	if (drv_data->ssp_type != PXA25x_SSP)
>> -		dev_dbg(&spi->dev, "%d bits/word, %d Hz, mode %d\n",
>> +		dev_dbg(&spi->dev, "%d bits/word, %d Hz, mode %d,
>> %s\n", spi->bits_per_word,
>>  				(CLOCK_SPEED_HZ)
>>  					/ (1 + ((chip->cr0 &
>> SSCR0_SCR) >> 8)),
>> -				spi->mode & 0x3);
>> +				spi->mode & 0x3,
>> +				chip->enable_dma ? "DMA" : "PIO");
>
>Oops! You are not patching the latest version of the code.  This
>version predates 1/26/08.  CLOCK_SPEED_HZ is no longer part of the
>code.  The patch will be rejected if it does not apply against the
>current git tree.  The latest version can be found at:
>
>http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob_plain;f=drivers/spi/pxa2xx_spi.c;hb=HEAD
>

This patch is against the latest Linus tree.

>>  	else
>> -		dev_dbg(&spi->dev, "%d bits/word, %d Hz, mode %d\n",
>> +		dev_dbg(&spi->dev, "%d bits/word, %d Hz, mode %d,
>> %s\n", spi->bits_per_word,
>>  				(CLOCK_SPEED_HZ/2)
>
>I note that when this line was changed in 1/26/08 commit, the author of
>that patch left out the /2, leaving the two branches of this if/else
>identical.  Please restore the /2 when you submit this patch.
>

Done.

>>  					/ (1 + ((chip->cr0 &
>> SSCR0_SCR) >> 8)),
>> -				spi->mode & 0x3);
>> +				spi->mode & 0x3,
>> +				chip->enable_dma ? "DMA" : "PIO");
>>  
>>  	if (spi->bits_per_word <= 8) {
>>  		chip->n_bytes = 1;
>> @@ -1457,7 +1465,8 @@
>>  
>>  	/* Load default SSP configuration */
>>  	write_SSCR0(0, drv_data->ioaddr);
>> -	write_SSCR1(SSCR1_RxTresh(4) | SSCR1_TxTresh(12),
>> drv_data->ioaddr);
>> +	write_SSCR1(SSCR1_RxTresh(RX_THRESH_DFLT) |
>> SSCR1_TxTresh(TX_THRESH_DFLT),
>> +				drv_data->ioaddr);
>>  	write_SSCR0(SSCR0_SerClkDiv(2)
>>  			| SSCR0_Motorola
>>  			| SSCR0_DataSize(8),
>> --- linux-2.6.24.4/Documentation/spi/pxa2xx	2008-03-24
>> 14:49:18.000000000 -0400 +++
>> src.cache/Documentation/spi/pxa2xx	2008-08-20
>> 13:14:01.000000000 -0400 @@ -96,7 +96,7 @@ information via the
>> structure "pxa2xx_spi_chip" found in
>> "include/asm-arm/arch-pxa/pxa2xx_spi.h".  The pxa2xx_spi master
>> controller driver
>
>Again, you are trying to patch an obsolete version.  I'm not sure if
>the correctly made patch will apply to older versions, but this whole
>patch is not essential to back port, as it is a "clarification" and
>not a bug fix.  I realize that you consider it a bug fix, and in a way
>I agree, but it is not, in the sense that older drivers can be used if
>all fields in this structure are filled in.
>
>>  will uses the configuration whenever the driver communicates with
>> the slave -device.
>> +device. All fields are optional.
>
>Do you need to note that unused fields should be set to zero, or is
>these always zeroed by the way the struct pxa2xx_spi_chip is declared
>(external or static declaration)?  I have forgotten and you probably
>looked at that recently.

C requires all file-scope variables without initializers to be in the
BSS which is cleared at startup.

>>  
>>  struct pxa2xx_spi_chip {
>>  	u8 tx_threshold;
>> @@ -112,14 +112,17 @@
>>  performance of pxa2xx_spi driver and misconfiguration will result
>> in rx fifo overruns (especially in PIO mode transfers). Good default
>> values are 
>> -	.tx_threshold = 12,
>> -	.rx_threshold = 4,
>> +	.tx_threshold = 8,
>> +	.rx_threshold = 8,
>> +
>> +The range is 1 to 16 where zero indicates "use default".
>>  
>>  The "pxa2xx_spi_chip.dma_burst_size" field is used to configure
>> PXA2xx DMA engine and is related the "spi_device.bits_per_word"
>> field.  Read and understand the PXA2xx "Developer Manual" sections
>> on the DMA controller and SSP Controllers to determine the correct
>> value. An SSP configured for byte-wide transfers would -use a value
>> of 8. +use a value of 8. The driver will determine a reasonable
>> default if +dma_burst_size == 0.
>>  
>>  The "pxa2xx_spi_chip.timeout" fields is used to efficiently handle
>>  trailing bytes in the SSP receiver fifo.  The correct value for
>> this field is @@ -135,7 +138,10 @@
>>  The "pxa2xx_spi_chip.cs_control" field is used to point to a board
>> specific function for asserting/deasserting a slave device chip
>> select.  If the field is NULL, the pxa2xx_spi master controller
>> driver assumes that the SSP port is -configured to use SSPFRM
>> instead. +configured to use SSPFRM instead. NOTE: the SPI driver
>> cannot control the chip +select if SSPFRM is used so each transfer
>> will assert/deassert SSPFRM around it. +Many devices need chip
>> select asserted around the complete message. The +cs_control method
>> should use the SSPFRM pin as a GPIO to accomodate these chips.
>
>*could* use the SSPFRM pin.  It can also use any other GPIO line, and
>at most one could use SSPFRM if there are multiple chips on the bus.

Done.

>>  
>>  NSSP SALVE SAMPLE
>>  -----------------
>> @@ -206,16 +212,15 @@
>>  
>>  DMA and PIO I/O Support
>>  -----------------------
>> -The pxa2xx_spi driver support both DMA and interrupt driven PIO
>> message +The pxa2xx_spi driver supports both DMA and interrupt
>> driven PIO message transfers.  The driver defaults to PIO mode and
>> DMA transfers must enabled by
>
>Add "be" between "must enabled".

Done.

>> -setting the "enable_dma" flag in the "pxa2xx_spi_master" structure
>> and -ensuring that the "pxa2xx_spi_chip.dma_burst_size" field is
>> non-zero.  The DMA +setting the "enable_dma" flag in the
>> "pxa2xx_spi_master" structure.  The DMA mode support both coherent
>> and stream based DMA mappings.
>
>As long as you are fixing, then "supports" above.
>

Done.

>>  
>>  The following logic is used to determine the type of I/O to be used
>> on a per "spi_transfer" basis:
>>  
>> -if !enable_dma or dma_burst_size == 0 then
>> +if !enable_dma then
>>  	always use PIO transfers
>
>Well, that is true, but the user should probably note that simply
>requesting DMA via the enable_dma flag is not sufficient to guarantee
>that DMA will be used.  There are a number of criteria in
>map_dma_buffers that must be met.  I guess that is what the rest of
>that list is trying to say.
>
>Also note that the very recent patch that I submitted, tries to address
>a problem that you encountered with transfer lengths longer than 8192;
>in that case, I changed the behavior from "fail" to "do it in PIO mode
>with rate limited warning".  Some day the driver could be rewritten to
>bust long transfers and do the pieces by DMA, but that is too ambitious
>for now.  So you could add to the list of states that transfers longer
>than 8191 will be PIO.  Have you tested that patch to see if it fixes
>any of your other problems?
>

I did not try that patch yet. I added a comment to the doc about this.

>I'm not sure I believe the words about coherent and stream DMA, but I
>will have to check that later.

OK

Patch against Linus 2.6 tree after
4c246edd2550304df5b766cc841584b2bb058843. It is compile tested only.

---
Subject: [PATCH] pxa2xx_spi: Fix chip_info defaults and documentation.

Make the chip info structure data optional by providing reasonable
defaults. Improve corresponding documentation.

DMA can determine appropriate dma_burst_size and thresholds
automatically so use DMA even if dma_burst_size is not specified.

Signed-off-by: Vernon Sauder <VernonInHand-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 Documentation/spi/pxa2xx |   31 ++++++++++++++++++++-----------
 drivers/spi/pxa2xx_spi.c |   43 ++++++++++++++++++++++++++-----------------
 2 files changed, 46 insertions(+), 28 deletions(-)

diff --git a/Documentation/spi/pxa2xx b/Documentation/spi/pxa2xx
index bbe8dee..92e3a37 100644
--- a/Documentation/spi/pxa2xx
+++ b/Documentation/spi/pxa2xx
@@ -96,7 +96,7 @@ Each slave device attached to the PXA must provide slave specific configuration
 information via the structure "pxa2xx_spi_chip" found in
 "arch/arm/mach-pxa/include/mach/pxa2xx_spi.h".  The pxa2xx_spi master controller driver
 will uses the configuration whenever the driver communicates with the slave
-device.
+device. All fields are optional.
 
 struct pxa2xx_spi_chip {
 	u8 tx_threshold;
@@ -112,14 +112,17 @@ used to configure the SSP hardware fifo.  These fields are critical to the
 performance of pxa2xx_spi driver and misconfiguration will result in rx
 fifo overruns (especially in PIO mode transfers). Good default values are
 
-	.tx_threshold = 12,
-	.rx_threshold = 4,
+	.tx_threshold = 8,
+	.rx_threshold = 8,
+
+The range is 1 to 16 where zero indicates "use default".
 
 The "pxa2xx_spi_chip.dma_burst_size" field is used to configure PXA2xx DMA
 engine and is related the "spi_device.bits_per_word" field.  Read and understand
 the PXA2xx "Developer Manual" sections on the DMA controller and SSP Controllers
 to determine the correct value. An SSP configured for byte-wide transfers would
-use a value of 8.
+use a value of 8. The driver will determine a reasonable default if
+dma_burst_size == 0.
 
 The "pxa2xx_spi_chip.timeout" fields is used to efficiently handle
 trailing bytes in the SSP receiver fifo.  The correct value for this field is
@@ -135,7 +138,10 @@ testing.
 The "pxa2xx_spi_chip.cs_control" field is used to point to a board specific
 function for asserting/deasserting a slave device chip select.  If the field is
 NULL, the pxa2xx_spi master controller driver assumes that the SSP port is
-configured to use SSPFRM instead.
+configured to use SSPFRM instead. NOTE: the SPI driver cannot control the chip
+select if SSPFRM is used so each transfer will assert/deassert SSPFRM around it.
+Many devices need chip select asserted around the complete message. The 
+cs_control method could use the SSPFRM pin as a GPIO to accomodate these chips.
 
 NSSP SALVE SAMPLE
 -----------------
@@ -206,18 +212,21 @@ static void __init streetracer_init(void)
 
 DMA and PIO I/O Support
 -----------------------
-The pxa2xx_spi driver support both DMA and interrupt driven PIO message
-transfers.  The driver defaults to PIO mode and DMA transfers must enabled by
-setting the "enable_dma" flag in the "pxa2xx_spi_master" structure and
-ensuring that the "pxa2xx_spi_chip.dma_burst_size" field is non-zero.  The DMA
-mode support both coherent and stream based DMA mappings.
+The pxa2xx_spi driver supports both DMA and interrupt driven PIO message
+transfers.  The driver defaults to PIO mode and DMA transfers must be enabled
+by setting the "enable_dma" flag in the "pxa2xx_spi_master" structure.  The DMA
+mode supports both coherent and stream based DMA mappings.
 
 The following logic is used to determine the type of I/O to be used on
 a per "spi_transfer" basis:
 
-if !enable_dma or dma_burst_size == 0 then
+if !enable_dma then
 	always use PIO transfers
 
+if spi_message.len > 8192 then
+	print "rate limited" warning
+	use PIO transfers
+
 if spi_message.is_dma_mapped and rx_dma_buf != 0 and tx_dma_buf != 0 then
 	use coherent DMA mode
 
diff --git a/drivers/spi/pxa2xx_spi.c b/drivers/spi/pxa2xx_spi.c
index 34c7c98..239a26c 100644
--- a/drivers/spi/pxa2xx_spi.c
+++ b/drivers/spi/pxa2xx_spi.c
@@ -47,6 +47,10 @@ MODULE_ALIAS("platform:pxa2xx-spi");
 
 #define MAX_BUSES 3
 
+#define RX_THRESH_DFLT 	8
+#define TX_THRESH_DFLT 	8
+#define TIMOUT_DFLT		1000
+
 #define DMA_INT_MASK (DCSR_ENDINTR | DCSR_STARTINTR | DCSR_BUSERR)
 #define RESET_DMA_CHANNEL (DCSR_NODESC | DMA_INT_MASK)
 #define IS_DMA_ALIGNED(x) (((u32)(x)&0x07)==0)
@@ -1105,6 +1109,8 @@ static int setup(struct spi_device *spi)
 	struct driver_data *drv_data = spi_master_get_devdata(spi->master);
 	struct ssp_device *ssp = drv_data->ssp;
 	unsigned int clk_div;
+	uint tx_thres = TX_THRESH_DFLT;
+	uint rx_thres = RX_THRESH_DFLT;
 
 	if (!spi->bits_per_word)
 		spi->bits_per_word = 8;
@@ -1143,8 +1149,7 @@ static int setup(struct spi_device *spi)
 
 		chip->cs_control = null_cs_control;
 		chip->enable_dma = 0;
-		chip->timeout = 1000;
-		chip->threshold = SSCR1_RxTresh(1) | SSCR1_TxTresh(1);
+		chip->timeout = TIMOUT_DFLT;
 		chip->dma_burst_size = drv_data->master_info->enable_dma ?
 					DCMD_BURST8 : 0;
 	}
@@ -1159,21 +1164,22 @@ static int setup(struct spi_device *spi)
 		if (chip_info->cs_control)
 			chip->cs_control = chip_info->cs_control;
 
-		chip->timeout = chip_info->timeout;
-
-		chip->threshold = (SSCR1_RxTresh(chip_info->rx_threshold) &
-								SSCR1_RFT) |
-				(SSCR1_TxTresh(chip_info->tx_threshold) &
-								SSCR1_TFT);
-
-		chip->enable_dma = chip_info->dma_burst_size != 0
-					&& drv_data->master_info->enable_dma;
+		if (chip_info->timeout)
+			chip->timeout = chip_info->timeout;
+		if (chip_info->tx_threshold)
+			tx_thres = chip_info->tx_threshold;
+		if (chip_info->rx_threshold)
+			rx_thres = chip_info->rx_threshold;
+		chip->enable_dma = drv_data->master_info->enable_dma;
 		chip->dma_threshold = 0;
 
 		if (chip_info->enable_loopback)
 			chip->cr1 = SSCR1_LBM;
 	}
 
+	chip->threshold = (SSCR1_RxTresh(rx_thres) & SSCR1_RFT) |
+			(SSCR1_TxTresh(tx_thres) & SSCR1_TFT);
+
 	/* set dma burst and threshold outside of chip_info path so that if
 	 * chip_info goes away after setting chip->enable_dma, the
 	 * burst and threshold can still respond to changes in bits_per_word */
@@ -1202,17 +1208,19 @@ static int setup(struct spi_device *spi)
 
 	/* NOTE:  PXA25x_SSP _could_ use external clocking ... */
 	if (drv_data->ssp_type != PXA25x_SSP)
-		dev_dbg(&spi->dev, "%d bits/word, %ld Hz, mode %d\n",
+		dev_dbg(&spi->dev, "%d bits/word, %ld Hz, mode %d, %s\n",
 				spi->bits_per_word,
 				clk_get_rate(ssp->clk)
 					/ (1 + ((chip->cr0 & SSCR0_SCR) >> 8)),
-				spi->mode & 0x3);
+				spi->mode & 0x3,
+				chip->enable_dma ? "DMA" : "PIO");
 	else
-		dev_dbg(&spi->dev, "%d bits/word, %ld Hz, mode %d\n",
+		dev_dbg(&spi->dev, "%d bits/word, %ld Hz, mode %d, %s\n",
 				spi->bits_per_word,
-				clk_get_rate(ssp->clk)
+				clk_get_rate(ssp->clk) / 2
 					/ (1 + ((chip->cr0 & SSCR0_SCR) >> 8)),
-				spi->mode & 0x3);
+				spi->mode & 0x3,
+				chip->enable_dma ? "DMA" : "PIO");
 
 	if (spi->bits_per_word <= 8) {
 		chip->n_bytes = 1;
@@ -1432,7 +1440,8 @@ static int __init pxa2xx_spi_probe(struct platform_device *pdev)
 
 	/* Load default SSP configuration */
 	write_SSCR0(0, drv_data->ioaddr);
-	write_SSCR1(SSCR1_RxTresh(4) | SSCR1_TxTresh(12), drv_data->ioaddr);
+	write_SSCR1(SSCR1_RxTresh(RX_THRESH_DFLT) | SSCR1_TxTresh(TX_THRESH_DFLT),
+				drv_data->ioaddr);
 	write_SSCR0(SSCR0_SerClkDiv(2)
 			| SSCR0_Motorola
 			| SSCR0_DataSize(8),
-- 
1.5.2.4


-------------------------------------------------------------------------
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: pxa2xx_spi with SFRM

[Ned Forrester]

With the one small change re: 8191, below, this looks good to go.
For what it's worth, you can add below your sign-off:

Reviewed-by: Ned Forrester <nforrester-/d+BM93fTQY@public.gmane.org>

It's time to add an appropriate [Patch] subject line to the patch, as
outlined in the docs, and email it to David Brownell
<david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>, with CC to the list.

Thanks for pushing this; I know it takes time, and you are right that it
needed to be done.

Vernon Sauder wrote:
> Ned,
> 
> Sorry for the slow response. Real world/work intrudes sometimes.
> 
> Here is another version of the patch along with responses to your
> comments.
> 
> Ned Forrester wrote:
> 
>> Vernon Sauder wrote:
>>
>> Also note that the very recent patch that I submitted, tries to address
>> a problem that you encountered with transfer lengths longer than 8192;
>> in that case, I changed the behavior from "fail" to "do it in PIO mode
>> with rate limited warning".  Some day the driver could be rewritten to
>> bust long transfers and do the pieces by DMA, but that is too ambitious
>> for now.  So you could add to the list of states that transfers longer
>> than 8191 will be PIO.  Have you tested that patch to see if it fixes
>> any of your other problems?
>>
> 
> I did not try that patch yet. I added a comment to the doc about this.

If you would try that it would be appreciated.  I was waiting for your
test to see if it fixed at least one of the MMC problems you were having
(rejected long transfers) before I ask David to push the patch upstream.

--------------------------------
> 
> Patch against Linus 2.6 tree after
> 4c246edd2550304df5b766cc841584b2bb058843. It is compile tested only.
> 
> ---
> Subject: [PATCH] pxa2xx_spi: Fix chip_info defaults and documentation.
> 
> Make the chip info structure data optional by providing reasonable
> defaults. Improve corresponding documentation.
> 
> DMA can determine appropriate dma_burst_size and thresholds
> automatically so use DMA even if dma_burst_size is not specified.
> 
> Signed-off-by: Vernon Sauder <VernonInHand-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  Documentation/spi/pxa2xx |   31 ++++++++++++++++++++-----------
>  drivers/spi/pxa2xx_spi.c |   43 ++++++++++++++++++++++++++-----------------
>  2 files changed, 46 insertions(+), 28 deletions(-)
> 

[snip]

>  The following logic is used to determine the type of I/O to be used on
>  a per "spi_transfer" basis:
>  
> -if !enable_dma or dma_burst_size == 0 then
> +if !enable_dma then
>  	always use PIO transfers
>  
> +if spi_message.len > 8192 then

Oops.  if > 8191 (or >=8192), a count of 8192 is not possible in the DMA
controller.



-- 
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: Limitations on transfer length [was: pxa2xx_spi with SFRM]

[David Brownell]

On Friday 15 August 2008, Ned Forrester wrote:
> David,
> 
> Is there supposed to be any practical limit to the length of a transfer?

Physical memory.  :)


>  It seems that the /dev/mtdblock_spi driver has requested a transfer in
> excess of 8191 bytes (though I don't know how long a transfer it
> requested).

SPI flash is often used for things like downloading FPGA
configuration bitstreams or first stage bootloaders (after
stage zero, an SOC's boot rom).

To support those use cases, SPI flash basically has no limit
on the number of bytes that can be read at once.  And the
Linux drivers for those SPI flash chips happily leverage
those requests.

Not many other applications wor that way though.

 
> In this case, I think pxa2xx_spi.c could handle long transfers in PIO
> mode, but not if DMA has been requested.  It looks like we may have put
> a bug in pxa2xx_spi by performing the length test even when DMA is not
> requested.
> 
> Is it proper to require that PIO mode be used if long transfers will be
> requested?

It's a fair workaround, but sub-optimal.  Recall that large
transfers are the place you most want to use DMA ... :)


> 5. Require that pxa2xx_spi break long DMA transfers into the required
> number of pieces to be performed in separate DMAs, silently giving back
> the message after it has completed all of it (this requires a lot of
> extra book-keeping in pxa2xx_spi).

If this happens a lot, that's the way to go.  I'd expect the driver
can be simplified to make this hurt less.

- 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: pxa2xx_spi with SFRM

[David Brownell]

On Friday 15 August 2008, Vernon Sauder wrote:
> >I think the  "1" and "15" above correspond to threshold register values
> >of "0" and "14"; that is the above numbers are the number of words in
> >the FIFO that trigger the interrupt/service request.
> 
> All this still leaves me slightly confused. (And I didn't even touch the
> dma_burst_size member yet.) I assume that the correct value for all of
> these settings must be determined empirically, not just theoretically.

I think you're probably right, although I'd say "optimum" not "correct".
Incorrect values should be rejected (or at least ignored) by the driver.


> How can I tell what the correct or optimum values are? Do I just use a
> performance test on the Flash chip or MMC device? Or can I look at the
> logic analyzer output and see what they should be? Or is there a debug
> printk that needs to be enabled?

I think the idea was that since the PXA hardware manuals don't
offer suggestions, this was a "punt it all to the board maintainer"
kind of solution.

- 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: Limitations on transfer length [was: pxa2xx_spi with SFRM]

[Vernon Sauder]

Ned Forrester wrote:
> Vernon Sauder wrote:
>> Ned Forrester wrote:
> 
>> On a side note, there is an MTD error when using the SPI Flash with the 
>> pxa2xx_spi driver.
>>
>> root@inhand-ft4 [~] # flash_erase /dev/mtd_spi
>> Erase Total 1 Units
>> Performing Flash Erase of length 65536 at offset 0x0 done # <- why *64K* ??
>> root@inhand-ft4 [~] # echo -n hithere123 > /dev/mtd_spi
>> root@inhand-ft4 [~] # hexdump -C -n512 /dev/mtdblock_spi
>> 00000000  68 69 74 68 65 72 65 31  32 33 ff ff ff ff ff ff  
>> |hithere123......|
>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
>> |................|
>> *
>> 00000200
>> root@inhand-ft4 [~] # echo -n hithere444xxx > /dev/mtdblock_spi
>> [  231.370000] pxa2xx-spi pxa2xx-spi.1: pump_transfers: transfer length 
>> greater than 8191
> 
> This message is issued by pxa2xx_spi.c in pump_transfers() if the
> transfer length is illegal.  You may have uncovered a bug here.  I think
> the 8191 limitation on transfer length only applies to DMA, as that is
> the length of the DMA counter.  The test for this should probably be
> performed only if DMA is in use.  I don't think Stephen or I ever
> expected any transfer to approach that length, in either PIO or DMA
> mode.  Is this really a legitimate transfer request?  Something that can
> only work in PIO mode and not in DMA mode?  That seems improper to me.
> 
>>  # maybe 64K??
>> [  231.380000] end_request: I/O error, dev mtdblock_spi, sector 0
>> [  231.380000] Buffer I/O error on device mtdblock_spi, logical block 0
>> [  231.380000] lost page write due to I/O error on mtdblock_spi
>> root@inhand-ft4 [~] # hexdump -C -n512 /dev/mtdblock_spi
>> 00000000  68 69 74 68 65 72 65 31  32 33 ff ff ff ff ff ff  
>> |hithere123......|
>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
>> |................|
>> *
>> 00000200
>>
>> I have not done a great deal of testing with MTD yet but I can read and 
>> write a small test file. I was hoping to use the mtdblock interface for 
>> this application. I will take a little time and see if I can figure out 
>> what part is wrong here.
>>
>>
>> Thanks again for your help.
>> Vern
> 

Ned,

I finally had some time to test this again. For refreshing, this is
testing an m25p80 MTD device on the PXA270 SPI bus.

Good news and Bad news. Good  news is that the error is gone with your
patch on 2.6.24.4. Bad news is that it there are still cases where the
kernel crashes or locks up using 2.6.27-rc8.

One time, this much of an error message got out before the kernel
completely locked up:

root@inhand-ft4 [~] # cat /tmp/k > /dev/mtd_spi
cat: Write Error: Bad address
[  151.497694] ------------[ cut here ]------------
[  151.503356] WARNING: at kernel/mutex.c:135
__mutex_lock_slowpath+0x9c/0x200()
[  151.510452] Modules linked in: spidev

And here is another dump that appeared after multiple writes: (Notice
first the failed writes. I don't know what is causing that. The HW setup
is slightly different.)

root@inhand-ft4 [~] # echo 1234567890 > /dev/mtd_spi
root@inhand-ft4 [~] # hexdump -C -n65536 /dev/mtd_spi
00000000  31 32 33 34 30 00 00 00  00 00 00 00 00 00 00 00
|12340...........|
00000010  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
|................|
*
00001000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
|................|
*
00010000
root@inhand-ft4 [~] # echo 1234567890 > /dev/mtd_spi
[  332.052355] slab error in verify_redzone_free(): cache `size-32':
memory outside object was overw
ritten
[  332.061741] [<c014c84c>] (dump_stack+0x0/0x14) from [<c01b1ad0>]
(__slab_error+0x28/0x30)
[  332.069951] [<c01b1aa8>] (__slab_error+0x0/0x30) from [<c01b2770>]
(cache_free_debugcheck+0x194/0
x2c8)
[  332.079236] [<c01b25dc>] (cache_free_debugcheck+0x0/0x2c8) from
[<c01b3120>] (kfree+0x94/0x114)
[  332.087911] [<c01b308c>] (kfree+0x0/0x114) from [<c02c9c00>]
(mtd_write+0x11c/0x240)
[  332.095652]  r8:07932c80 r7:386e438b r6:c71d2000 r5:c71ecee0 r4:00020000
[  332.102342] [<c02c9ae4>] (mtd_write+0x0/0x240) from [<c01b7aa8>]
(vfs_write+0xb8/0x144)
[  332.110337] [<c01b79f0>] (vfs_write+0x0/0x144) from [<c01b80e0>]
(sys_write+0x4c/0x80)
[  332.118237]  r7:00000004 r6:00000000 r5:00000000 r4:c782e4f4
[  332.123888] [<c01b8094>] (sys_write+0x0/0x80) from [<c0148d20>]
(ret_fast_syscall+0x0/0x2c)
[  332.132221]  r6:40147194 r5:40017000 r4:0000000b
[  332.136834] c71eced8: redzone 1:0xd84156c5635688c0, redzone 2:0x0.
root@inhand-ft4 [~] #
root@inhand-ft4 [~] # [  334.229714] Unable to handle kernel NULL
pointer dereference at virtual add
ress 00000000
[  334.237798] pgd = c0004000
[  334.240485] [00000000] *pgd=00000000
[  334.244053] Internal error: Oops: 17 [#1] PREEMPT
[  334.248730] Modules linked in: spidev pxa2xx_spi m25p80 hci_uart
bluetooth ohci_hcd sd_mod usb_st
orage libusual usbcore orinoco_cs orinoco hermes snd_pxa2xx_ac97
snd_ac97_codec snd_pxa2xx_pcm rtc_s
a1100 mmc_block pxamci mmc_core ft4_batt
[  334.269794] CPU: 0    Not tainted  (2.6.27-rc8 #7)
[  334.274610] PC is at list_del+0x14/0x9c
[  334.278444] LR is at free_block+0x88/0x1cc
[  334.282519] pc : [<c02704e4>]    lr : [<c01b2d1c>]    psr: 00000093
[  334.282533] sp : c7827ef0  ip : c7827f08  fp : c7827f04
[  334.293937] r10: c7809840  r9 : c7954000  r8 : c78043d0
[  334.299134] r7 : c71ecf14  r6 : 00000000  r5 : c7805c2c  r4 : c7805c2c
[  334.305629] r3 : 00000000  r2 : c198ba80  r1 : c7805c2c  r0 : c71ecf14
[  334.312120] Flags: nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
Segment kernel
[  334.319475] Control: 0000397f  Table: a7950000  DAC: 00000017
[  334.325185] Process events/0 (pid: 5, stack limit = 0xc7826268)
[  334.331070] Stack: (0xc7827ef0 to 0xc7828000)
[  334.335399] 7ee0:                                     00000008
c7805c2c c7827f40 c7827f08
[  334.343678] 7f00: c01b2d1c c02704dc 00000000 00000001 c7805c2c
00000000 c7805c2c 00000001
[  334.351957] 7f20: c7805c1c c7809840 c01b33dc 00000000 00000000
c7827f60 c7827f44 c01b2f10
[  334.360237] 7f40: c01b2ca0 c78043d0 c7809840 00000000 c0481494
c7827f84 c7827f64 c01b3434
[  334.368516] 7f60: c01b2e6c 00000000 c0481498 c78012b0 c0481494
c7826000 c7827fa8 c7827f88
[  334.376796] 7f80: c01777c0 c01b33e8 c7827fb8 c78012b0 c7826000
00000000 00000000 c7827fd8
[  334.385074] 7fa0: c7827fac c0177ab4 c01776ec 00000000 c781dc00
c017b7bc c7827fb8 c7827fb8
[  334.393355] 7fc0: c7826000 c78012b0 c01779cc c7827ff4 c7827fdc
c017b784 c01779d8 00000000
[  334.401633] 7fe0: 00000000 00000000 00000000 c7827ff8 c0168360
c017b738 11d97d47 a5f725a5
[  334.409913] Backtrace:
[  334.412354] [<c02704d0>] (list_del+0x0/0x9c) from [<c01b2d1c>]
(free_block+0x88/0x1cc)
[  334.420256]  r4:c7805c2c
[  334.422775] [<c01b2c94>] (free_block+0x0/0x1cc) from [<c01b2f10>]
(drain_array+0xb0/0xfc)
[  334.430936] [<c01b2e60>] (drain_array+0x0/0xfc) from [<c01b3434>]
(cache_reap+0x58/0x134)
[  334.439095]  r7:c0481494 r6:00000000 r5:c7809840 r4:c78043d0
[  334.444745] [<c01b33dc>] (cache_reap+0x0/0x134) from [<c01777c0>]
(run_workqueue+0xe0/0x1ac)
[  334.453189]  r7:c7826000 r6:c0481494 r5:c78012b0 r4:c0481498
[  334.458840] [<c01776e0>] (run_workqueue+0x0/0x1ac) from [<c0177ab4>]
(worker_thread+0xe8/0xfc)
[  334.467429]  r8:00000000 r7:00000000 r6:c7826000 r5:c78012b0 r4:c7827fb8
[  334.474123] [<c01779cc>] (worker_thread+0x0/0xfc) from [<c017b784>]
(kthread+0x58/0x90)
[  334.482111]  r6:c01779cc r5:c78012b0 r4:c7826000
[  334.486719] [<c017b72c>] (kthread+0x0/0x90) from [<c0168360>]
(do_exit+0x0/0x794)
[  334.494198]  r6:00000000 r5:00000000 r4:00000000
[  334.498807] Code: e92dd810 e24cb004 e24dd004 e5903004 (e593c000)
[  334.505751] ---[ end trace 3637d999aba17a07 ]---
[  334.510408] note: events/0[5] exited with preempt_count 1


I wanted to send this so you know. There is the possibility that the
2.6.27 I am using is not completely configured and/or patched. It was
based on Linus' git tree but I have not completed testing all my board
patches. This feature is not being requested any longer so I don't have
a lot of pressure to fix it right now. As I get closer to finishing the
project, I might need to resolve this.

-- 
Regards,
 Vernon Sauder :)


-------------------------------------------------------------------------
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: Limitations on transfer length [was: pxa2xx_spi with SFRM]

[Ned Forrester]

Sorry for the delayed response....

Vernon Sauder wrote:
> I finally had some time to test this again. For refreshing, this is
> testing an m25p80 MTD device on the PXA270 SPI bus.
> 
> Good news and Bad news. Good  news is that the error is gone with your
> patch on 2.6.24.4. Bad news is that it there are still cases where the
> kernel crashes or locks up using 2.6.27-rc8.

if you snapshotted 2.6.27-rc8, pxa2xx_spi would not have built properly.
 There was an error in a patch of mine that was fixed by a 2008-10-01
patch by Mike Rapoport, which would have been included in 2.6.27-rc9.
You probably figured that out.

> One time, this much of an error message got out before the kernel
> completely locked up:
> 
> root@inhand-ft4 [~] # cat /tmp/k > /dev/mtd_spi
> cat: Write Error: Bad address
> [  151.497694] ------------[ cut here ]------------
> [  151.503356] WARNING: at kernel/mutex.c:135
> __mutex_lock_slowpath+0x9c/0x200()
> [  151.510452] Modules linked in: spidev
> 
> And here is another dump that appeared after multiple writes: (Notice
> first the failed writes. I don't know what is causing that. The HW setup
> is slightly different.)

[snip crash dump]

I don't know if it could be related, but there is another thread "PXA270
SSP DMA Corruption" in which Scott Merritt reports trouble with the
combination of spidev and pxa2xx_spi.  He is also up-to-date with
2.6.27. He does not report kernel crashes, but simply corrupted data.

I looked at spidev today (for the first time) and noticed that it uses
the same buffer for rx and tx.  That should be alright, except that I
don't think that pxa2xx_spi handles that case correctly in DMA mode.
pxa2xx_spi will try to dma_map_single() the same buffer twice, once in
each direction.

Are you using PIO or DMA mode?  If DMA, can you try setting
struct pxa2xx_spi_master.enable_dma = 0;

If that works better, then maybe you have the same problem.

> I wanted to send this so you know. There is the possibility that the
> 2.6.27 I am using is not completely configured and/or patched. It was
> based on Linus' git tree but I have not completed testing all my board
> patches. This feature is not being requested any longer so I don't have
> a lot of pressure to fix it right now. As I get closer to finishing the
> project, I might need to resolve this.

Maybe if you have some other issues, that would explain the difference
in symptoms between your application and Merritt's.  Alternatively, it
could be different ways that messages are formatted by the chain of code
that accesses MTD.  Merritt is directly using the ioctl interface of spidev.

-- 
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=/

pxa2xx_spi with SFRM

[Vernon Sauder]

Hi,

I am using a custom PXA270 board and tried to use the SSP port to 
communication with a SPI Flash. After reading docs on how to configure 
the spi_master and spi devices, I have the device driver's probe being 
called. But I cannot get the pxa2xx_spi driver to work correctly. I can 
connect either a SD card or a M26P16 Flash chip to the SPI port on my 
board. Neither device driver can completely operate their device.

It looks like it is impossible for the device drivers to control the 
chip select (CS) line. If I use the manual cs_control callback, the 
timing is invalid because the SSP clock keeps running. That means that 
several bits are clocked out before the SSP controller starts to drive 
the TX line correctly. If the SFRM signal is used, it does not allow the 
driver to keep CS active for multiple transactions as they expect and 
assume. The spi_sync call takes an spi_message which contains a list of 
transactions to send. Normally, the device will need the CS active 
during the complete message. But the SSP controller deactivates SFRM 
when it is done with each buffer. For instance, the M25P16 datasheet 
indicates that CS has to stay active from the READ command through the 
complete data transfer. When it goes high, it resets the command interface.

It looks like the SSP port needs to have a register bit to turn off the 
clock when there is nothing to transmit. Or a bit to tell the SSP 
controller to leave CS active. Or I can change the HW so CS gates the 
clock and a manual CS chip operates the device CS pin. I might also try 
the bitbanging SPI driver next to see if that can operate with the Flash 
chip.

Is there some other setting I am missing here? Is anyone else using the 
M25P16 chip with PXA270 SPI driver?

Vernon Sauder

-------------------------------------------------------------------------
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=/