Re: pxa2xx_spi with SFRM

[Vernon Sauder <vernoninhand-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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