[Patch 0/1] spi: Fix pxa2xx_spi.c transfer delay, cs change, transfer length

[Patch 0/1] spi: Fix pxa2xx_spi.c transfer delay, cs change, transfer length

[Ned Forrester]

My next message of this subject contains a patch for many of the issues
raised by the threads:

[PATCH] atmel_spi: support zero length transfer, 2/22/08
[PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode,
8/18/08
pxa2xx_spi with SFRM, 8/7/08

This patch is created to apply to the development kernel: 2.6.27-rc3.

David:

This patch is preliminary.  I have compiled it on 2.6.20, but it
awaits testing by the folks reporting problems; I don't have any
hardware to test it on.  Hopefully it is clean.  Your general comments
will be appreciated, particularly on the revised handling of
transfers over 8191 bytes long (or on my related post to you:

Limitations on transfer length [was: pxa2xx_spi	with SFRM], 8/15/08

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


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

Re: [Patch 0/1] spi: Fix pxa2xx_spi.c transfer delay, cs change, transfer length

[Daniel Ribeiro]

On Wed, Aug 20, 2008 at 12:31 AM, Ned Forrester <nforrester-/d+BM93fTQY@public.gmane.org> wrote:
> My next message of this subject contains a patch for many of the issues
> raised by the threads:
>
> [PATCH] atmel_spi: support zero length transfer, 2/22/08
> [PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode,
> 8/18/08
> pxa2xx_spi with SFRM, 8/7/08

I tested the patch regarding the CS problems, and it works fine.

Thanks Ned.

-- 
Daniel Ribeiro

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

[Patch 0/1] spi: Fix pxa2xx_spi.c transfer delay, cs change, transfer length

[Ned Forrester]

David:

I think enough time has elapsed for any objections to surface.  Is there
still time to push this patch for 2.6.27?  The actual patch is contained
in my email of 8/19/08:

[spi-devel-general] [Patch 1/1] spi: Fix pxa2xx_spi.c transfer delay, cs
change, transfer length

Also, the patch should be directed at the stable maintainers for
addition to any kernel as far back as 2.6.20. I'm not sure how to do
direct that, but I expect that you do.

Thanks.

> My next message of this subject contains a patch for many of the issues
> raised by the threads:
> 
> [PATCH] atmel_spi: support zero length transfer, 2/22/08
> [PATCH] pxa2xx_spi: wait_rx_stall before deasserting CS on PIO mode,
> 8/18/08
> pxa2xx_spi with SFRM, 8/7/08
> 
> This patch is created to apply to the development kernel: 2.6.27-rc3.
> 
> David:
> 
> This patch is preliminary.  I have compiled it on 2.6.20, but it
> awaits testing by the folks reporting problems; I don't have any
> hardware to test it on.  Hopefully it is clean.  Your general comments
> will be appreciated, particularly on the revised handling of
> transfers over 8191 bytes long (or on my related post to you:
> 
> Limitations on transfer length [was: pxa2xx_spi	with SFRM], 8/15/08

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


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

Re: [Patch 0/1] spi: Fix pxa2xx_spi.c transfer delay, cs change, transfer length

[David Brownell]

On Thursday 04 September 2008, Ned Forrester wrote:
> I think enough time has elapsed for any objections to surface.  Is there
> still time to push this patch for 2.6.27? 

I enclose a slightly cleaned-up version.  Comments, checkpath.pl,
and so on.  Could you eyeball make sure it still behaves?

Also, this is too much for one patch.  The transfer_delay and
cs_change stuff (fixing bugs #1-4) are plausibly one patch.
And the DMA stuff (#5-6) is plausibly a different patch.

Could you split those two out?  Then I'll submit them.

There should still be time; thanks for the ping.


> Also, the patch should be directed at the stable maintainers for
> addition to any kernel as far back as 2.6.20. I'm not sure how to do
> direct that, but I expect that you do.

One just cc's stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org ... I don't think the stable
team maintains anything that old though.  I cc'd Eric (at Marvell)
who is probably worth keeping in the loop in terms of PXA fixes.
The PXA platform is now getting an overhaul to help make the
PXA3xx changes work better ... yay!

- Dave

p.s. Eric, it seems the pxa2xx_spi driver is in need of more
     active maintaining than it's got.  I seem to have a few
     other patches for it in my mailbox too...

========= CUT HERE
From: Ned Forrester <nforrester-/d+BM93fTQY@public.gmane.org>

Fixes several related bugs in the pxa2xx_spi driver.  These
bugs are in all versions of this driver (except for #6, which
was introduced in the 2.6.20 kernel) and prevent using it
with chips like m25p16 flash.

 1. The spi_transfer.cs_change flag is handled too early:
    before spi_transfer.delay_usecs applies, thus making the
    delay ineffective at holding chip select.

 2. spi_transfer.delay_usecs is ignored on the last transfer
    of a message (likewise not holding chipselect long enough).

 3. If spi_transfer.cs_change is set on the last transfer, the
    chip select is always disabled, instead of the intended
    meaning: optionally holding chip select enabled for the
    next message.  (Impact at least MMC-over-SPI.)

Those first three bugs were fixed with a relocation of delays
and chip select de-assertions.

 4. If a message has the cs_change flag set on the last transfer,
    and had the chip select stayed enabled as requested (see 3,
    above), it would not have been disabled if the next message is
    for a different chip.  Fixed by dropping chip select regardless
    of cs_change at end of a message, if there is no next message
    or if the next message is for a different chip.  

 5. Zero length transfers are permitted for use to insert timing,
    but pxa2xx_spi.c will fail if this is requested in DMA mode.
    Fixed by using programmed I/O (PIO) mode for such transfers.

 6. Transfers larger than 8191 are not permitted in DMA mode.  A
    test for length rejects all large transfers regardless of DMA
    or PIO mode.  Worked around by rejecting only large transfers
    with DMA mapped buffers, and forcing all other transfers larger than 8191
    to use PIO mode.  A rate limited warning is issued for DMA
    transfers forced to PIO mode.

This patch should apply to all kernels back to and including 2.6.20;
it was test patched against 2.6.20.  An additional patch would be
required for older kernels, but those versions are very buggy anyway.  

There are some additional issues when GPIOs aren't being used as
chipselects.  Probably the SSFRM support should be removed, and
the GPIO support switched over to use standard GPIO calls (and to
support SPI_CS_HIGH).

Signed-off-by: Ned Forrester <nforrester-/d+BM93fTQY@public.gmane.org>
Cc: Vernon Sauder <vernoninhand-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
# NYET-Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
 drivers/spi/pxa2xx_spi.c |  116 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 91 insertions(+), 25 deletions(-)

--- a/drivers/spi/pxa2xx_spi.c
+++ b/drivers/spi/pxa2xx_spi.c
@@ -47,9 +47,10 @@ MODULE_ALIAS("platform:pxa2xx-spi");
 
 #define MAX_BUSES 3
 
-#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)
+#define DMA_INT_MASK		(DCSR_ENDINTR | DCSR_STARTINTR | DCSR_BUSERR)
+#define RESET_DMA_CHANNEL	(DCSR_NODESC | DMA_INT_MASK)
+#define IS_DMA_ALIGNED(x)	(((x) & 0x07) == 0)
+#define MAX_DMA_LEN		8191
 
 /*
  * for testing SSCR1 changes that require SSP restart, basically
@@ -144,7 +145,6 @@ struct driver_data {
 	size_t tx_map_len;
 	u8 n_bytes;
 	u32 dma_width;
-	int cs_change;
 	int (*write)(struct driver_data *drv_data);
 	int (*read)(struct driver_data *drv_data);
 	irqreturn_t (*transfer_handler)(struct driver_data *drv_data);
@@ -406,8 +406,45 @@ static void giveback(struct driver_data 
 					struct spi_transfer,
 					transfer_list);
 
+	/* Delay if requested before any change in chip select */
+	if (last_transfer->delay_usecs)
+		udelay(last_transfer->delay_usecs);
+
+	/* Drop chip select UNLESS cs_change is true or we are returning
+	 * a message with an error, or next message is for another chip
+	 */
 	if (!last_transfer->cs_change)
 		drv_data->cs_control(PXA2XX_CS_DEASSERT);
+	else {
+		struct spi_message *next_msg;
+
+		/* Holding of cs was hinted, but we need to make sure
+		 * the next message is for the same chip.  Don't waste
+		 * time with the following tests unless this was hinted.
+		 *
+		 * We cannot postpone this until pump_messages, because
+		 * after calling msg->complete (below) the driver that
+		 * sent the current message could be unloaded, which
+		 * could invalidate the cs_control() callback...
+		 */
+
+		/* get a pointer to the next message, if any */
+		spin_lock_irqsave(&drv_data->lock, flags);
+		if (list_empty(&drv_data->queue))
+			next_msg = NULL;
+		else
+			next_msg = list_entry(drv_data->queue.next,
+					struct spi_message, queue);
+		spin_unlock_irqrestore(&drv_data->lock, flags);
+
+		/* see if the next and current messages point
+		 * to the same chip
+		 */
+		if (next_msg && next_msg->spi != msg->spi)
+			next_msg = NULL;
+		if (!next_msg || msg->state == ERROR_STATE)
+			drv_data->cs_control(PXA2XX_CS_DEASSERT);
+	}
 
 	msg->state = NULL;
 	if (msg->complete)
@@ -490,10 +527,9 @@ static void dma_transfer_complete(struct
 	msg->actual_length += drv_data->len -
 				(drv_data->rx_end - drv_data->rx);
 
-	/* Release chip select if requested, transfer delays are
-	 * handled in pump_transfers */
-	if (drv_data->cs_change)
-		drv_data->cs_control(PXA2XX_CS_DEASSERT);
+	/* Transfer delays and chip select release are
+	 * handled in pump_transfers or giveback
+	 */
 
 	/* Move to next transfer */
 	msg->state = next_transfer(drv_data);
@@ -602,10 +638,9 @@ static void int_transfer_complete(struct
 	drv_data->cur_msg->actual_length += drv_data->len -
 				(drv_data->rx_end - drv_data->rx);
 
-	/* Release chip select if requested, transfer delays are
-	 * handled in pump_transfers */
-	if (drv_data->cs_change)
-		drv_data->cs_control(PXA2XX_CS_DEASSERT);
+	/* Transfer delays and chip select release are
+	 * handled in pump_transfers or giveback
+	 */
 
 	/* Move to next transfer */
 	drv_data->cur_msg->state = next_transfer(drv_data);
@@ -840,23 +875,40 @@ static void pump_transfers(unsigned long
 		return;
 	}
 
-	/* Delay if requested at end of transfer*/
+	/* Delay if requested at end of transfer before CS change */
 	if (message->state == RUNNING_STATE) {
 		previous = list_entry(transfer->transfer_list.prev,
 					struct spi_transfer,
 					transfer_list);
 		if (previous->delay_usecs)
 			udelay(previous->delay_usecs);
+
+		/* Drop chip select only if cs_change is requested */
+		if (previous->cs_change)
+			drv_data->cs_control(PXA2XX_CS_DEASSERT);
 	}
 
-	/* Check transfer length */
-	if (transfer->len > 8191)
-	{
-		dev_warn(&drv_data->pdev->dev, "pump_transfers: transfer "
-				"length greater than 8191\n");
-		message->status = -EINVAL;
-		giveback(drv_data);
-		return;
+	/* Check for transfers that need multiple DMA segments */
+	if (transfer->len > MAX_DMA_LEN && chip->enable_dma) {
+
+		/* reject already-mapped transfers; PIO won't always work */
+		if (message->is_dma_mapped
+				|| transfer->rx_dma || transfer->tx_dma) {
+			dev_err(&drv_data->pdev->dev,
+				"pump_transfers: mapped transfer length "
+				"of %lu is greater than %d\n",
+				transfer->len, MAX_DMA_LEN);
+			message->status = -EINVAL;
+			giveback(drv_data);
+			return;
+		}
+
+		/* warn ... we force this to PIO mode */
+		if (printk_ratelimit())
+			dev_warn(&message->spi->dev, "pump_transfers: "
+				"DMA disabled for transfer length %ld "
+				"greater than %d\n",
+				(long)drv_data->len, MAX_DMA_LEN);
 	}
 
 	/* Setup the transfer state based on the type of transfer */
@@ -878,7 +930,6 @@ static void pump_transfers(unsigned long
 	drv_data->len = transfer->len & DCMD_LENGTH;
 	drv_data->write = drv_data->tx ? chip->write : null_writer;
 	drv_data->read = drv_data->rx ? chip->read : null_reader;
-	drv_data->cs_change = transfer->cs_change;
 
 	/* Change speed and bit per word on a per transfer */
 	cr0 = chip->cr0;
@@ -925,7 +976,7 @@ static void pump_transfers(unsigned long
 							&dma_thresh))
 				if (printk_ratelimit())
 					dev_warn(&message->spi->dev,
-						"pump_transfer: "
+						"pump_transfers: "
 						"DMA burst size reduced to "
 						"match bits_per_word\n");
 		}
@@ -939,8 +990,23 @@ static void pump_transfers(unsigned long
 
 	message->state = RUNNING_STATE;
 
-	/* Try to map dma buffer and do a dma transfer if successful */
-	if ((drv_data->dma_mapped = map_dma_buffers(drv_data))) {
+	/* Try to map dma buffer and do a dma transfer if successful, but
+	 * only if the length is non-zero and less than MAX_DMA_LEN.
+	 *
+	 * Zero-length non-descriptor DMA is illegal on PXA2xx; force use
+	 * of PIO instead.  Care is needed above because the transfer may
+	 * have have been passed with buffers that are already dma mapped.
+	 * A zero-length transfer in PIO mode will not try to write/read
+	 * to/from the buffers
+	 *
+	 * REVISIT large transfers are exactly where we most want to be
+	 * using DMA.  If this happens much, split those transfers into
+	 * multiple DMA segments rather than forcing PIO.
+	 */
+	drv_data->dma_mapped = 0;
+	if (drv_data->len > 0 && drv_data->len <= MAX_DMA_LEN)
+		drv_data->dma_mapped = map_dma_buffers(drv_data);
+	if (drv_data->dma_mapped) {
 
 		/* Ensure we have the correct interrupt handler */
 		drv_data->transfer_handler = dma_transfer;


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

Re: [Patch 0/1] spi: Fix pxa2xx_spi.c transfer delay, cs change, transfer length

[Eric Miao]

Is it possible that the GPIO CS patch merged first before this one?
And the patch below includes actually multiple changes, which
makes the merge not so straight-forward.

Ned, I think you have reviewed my patch, and will it be convenient
for you to update this one against mine? I have a lot of other stuffs
depending on that, so it will be appreciated if you do. Thanks.

- eric

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

Re: [Patch 0/1] spi: Fix pxa2xx_spi.c transfer delay, cs change, transfer length

[David Brownell]

On Monday 08 September 2008, Eric Miao wrote:
> Is it possible that the GPIO CS patch merged first before this one?

I'd say no ... the bugfixes should go into 2.6.27,
but your GPIO CS stuff is a feature/cleanup change
so it's not a candidate for 2.6.27 (or 2.6.27.1 etc).


> And the patch below includes actually multiple changes, which
> makes the merge not so straight-forward.

As you saw in my comments.  But the main thing is
that his chipselect-related bugfixes conflicted
with your new feature code.  :)

- Dave



> Ned, I think you have reviewed my patch, and will it be convenient
> for you to update this one against mine? I have a lot of other stuffs
> depending on that, so it will be appreciated if you do. Thanks.
> 
> - eric
> 
> 



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

Re: [Patch 0/1] spi: Fix pxa2xx_spi.c transfer delay, cs change, transfer length

[Ned Forrester]

David Brownell wrote:
> On Monday 08 September 2008, Eric Miao wrote:
>> Is it possible that the GPIO CS patch merged first before this one?
> 
> I'd say no ... the bugfixes should go into 2.6.27,
> but your GPIO CS stuff is a feature/cleanup change
> so it's not a candidate for 2.6.27 (or 2.6.27.1 etc).
> 
> 
>> And the patch below includes actually multiple changes, which
>> makes the merge not so straight-forward.
> 
> As you saw in my comments.  But the main thing is
> that his chipselect-related bugfixes conflicted
> with your new feature code.  :)
> 
> - Dave

OK, Dave, I will reply to your comments tomorrow and I will see if I can
split the patch in two.  I was hoping not to do that as the patch is not
very big, but I expect it can be done.  Sigh.

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


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

Re: [Patch 0/1] spi: Fix pxa2xx_spi.c transfer delay, cs change, transfer length

[Eric Miao]

> OK, Dave, I will reply to your comments tomorrow and I will see if I can
> split the patch in two.  I was hoping not to do that as the patch is not
> very big, but I expect it can be done.  Sigh.
>

Include me in the loop so that I know where to rebase. Thanks.

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

Re: [Patch 0/1] spi: Fix pxa2xx_spi.c transfer delay, cs change, transfer length

[Ned Forrester]

Eric Miao wrote:
> Is it possible that the GPIO CS patch merged first before this one?
> And the patch below includes actually multiple changes, which
> makes the merge not so straight-forward.

I guess I don't have the whole picture.  There must be other patches
that support the gpio_ calls.  If you are saying that some of those
patches have already merged, or are farther up the chain, then I guess I
should accommodate that.

> Ned, I think you have reviewed my patch, and will it be convenient
> for you to update this one against mine? I have a lot of other stuffs
> depending on that, so it will be appreciated if you do. Thanks.

Not convenient, but possible.  That would prevent the patch from
applying to earlier kernels.  Because the patch fixes operational bugs,
I think porting is important.  Of course, the original patch can be
offered to stable, and a new one submitted to the current kernel.

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


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

Re: [Patch 0/1] spi: Fix pxa2xx_spi.c transfer delay, cs change, transfer length

[Ned Forrester]

David Brownell wrote:
> On Thursday 04 September 2008, Ned Forrester wrote:
>> I think enough time has elapsed for any objections to surface.  Is there
>> still time to push this patch for 2.6.27? 
> 
> I enclose a slightly cleaned-up version.  Comments, checkpath.pl,
> and so on.  Could you eyeball make sure it still behaves?
> 
> Also, this is too much for one patch.  The transfer_delay and
> cs_change stuff (fixing bugs #1-4) are plausibly one patch.
> And the DMA stuff (#5-6) is plausibly a different patch.
> 
> Could you split those two out?  Then I'll submit them.

I'm working on it, (emergencies yesterday, sorry), but I have questions...

>> Also, the patch should be directed at the stable maintainers for
>> addition to any kernel as far back as 2.6.20. I'm not sure how to do
>> direct that, but I expect that you do.
> 
> One just cc's stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org ... I don't think the stable
> team maintains anything that old though.  I cc'd Eric (at Marvell)
> who is probably worth keeping in the loop in terms of PXA fixes.
> The PXA platform is now getting an overhaul to help make the
> PXA3xx changes work better ... yay!

Am I right to assume that you could/should/would make that cc: after you
add your Acked-by:

I don't expect that they would go back as far as 2.6.20, though
individuals could do so if they are still using old kernels.  I would
expect the stable maintainers to apply this as far back as they choose.

> p.s. Eric, it seems the pxa2xx_spi driver is in need of more
>      active maintaining than it's got.  I seem to have a few
>      other patches for it in my mailbox too...

As some of you know, but perhaps not Eric, I am creating a driver that
is a major expansion of pxa2xx_spi.c, so I am trying to stay involved
with pxa2xx_spi.c development to limit structural changes that might
make my work more difficult.  This may be a fool's errand, because my
changes are so significant that they likely never will be accepted into
the driver.  I think I have preserved all the existing functionality,
while adding the ability to operate in various formats, and as a slave
(limited to a single chip on the bus with a predictable transfer
pattern) at the maximum bus speed of 13Mbit/sec.  This driver is finally
working, as of last month, but is a wreck in terms of coding style, so
it is not ready for consideration.

More about that some other time.  I just thought it might be useful to
state my intentions and reason for interest.

> ========= CUT HERE
> From: Ned Forrester <nforrester-/d+BM93fTQY@public.gmane.org>
> 

[snip]

> There are some additional issues when GPIOs aren't being used as
> chipselects.  Probably the SSFRM support should be removed, and
> the GPIO support switched over to use standard GPIO calls (and to
> support SPI_CS_HIGH).

I accept your other changes to my patch notes, but I disagree that the
above paragraph is related to this patch.  I will leave it out unless
you strongly object.

Note that the driver does not specifically do anything to support using
SSPFRM as chip select.  The documentation says that if no cs_control
routine is specified, then the driver assumes use of SSPFRM, but really
it does nothing.  The real assumption is that the user knows what's
right, and has either hooked up (and is happy with) SSPFRM, or is using
a (single) chip that doesn't care.

Possibly a change to Documentation/spi/pxa2xx should be made to
highlight the ways in which use of SSPFRM differs from use of a GPIO.
Maybe I will take a shot at that with this patch.

> Signed-off-by: Ned Forrester <nforrester-/d+BM93fTQY@public.gmane.org>
> Cc: Vernon Sauder <vernoninhand-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> # NYET-Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
>  drivers/spi/pxa2xx_spi.c |  116 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 91 insertions(+), 25 deletions(-)
> 
> --- a/drivers/spi/pxa2xx_spi.c
> +++ b/drivers/spi/pxa2xx_spi.c
> @@ -47,9 +47,10 @@ MODULE_ALIAS("platform:pxa2xx-spi");
>  
>  #define MAX_BUSES 3
>  
> -#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)
> +#define DMA_INT_MASK		(DCSR_ENDINTR | DCSR_STARTINTR | DCSR_BUSERR)
> +#define RESET_DMA_CHANNEL	(DCSR_NODESC | DMA_INT_MASK
> +#define IS_DMA_ALIGNED(x)	(((x) & 0x07) == 0)

You are asking to change unrelated things here, but I will not object.
I don't know if there are any consequences to dropping the u32 cast;
that was in Stephen's earliest version, and I have not messed with those
things.

> +		/* see if the next and current messages point
> +		 * to the same chip
> +		 */
> +		if (next_msg && next_msg->spi != msg->spi)
> +			next_msg = NULL;

You have changed this from:
		if (next_msg)
			if (next_msg->spi != msg->spi)
				next_msg = NULL;

My understanding is that C does not guarantee the order of evaluation in
a compound conditional.  If "next_msg->spi != msg->spi" is evaluated
before "next_msg" a segmentation fault may occur.  Am I out of date on this?

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


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

Re: [Patch 0/1] spi: Fix pxa2xx_spi.c transfer delay, cs change, transfer length

[David Brownell]

> > I enclose a slightly cleaned-up version.  Comments, checkpath.pl,
> > and so on.  Could you eyeball make sure it still behaves?
> > 
> > Also, this is too much for one patch.  The transfer_delay and
> > cs_change stuff (fixing bugs #1-4) are plausibly one patch.
> > And the DMA stuff (#5-6) is plausibly a different patch.
> > 
> > Could you split those two out?  Then I'll submit them.
> 
> I'm working on it, (emergencies yesterday, sorry), but I have questions...

Stuff happens.  :)

 
> >> Also, the patch should be directed at the stable maintainers for
> >> addition to any kernel as far back as 2.6.20. I'm not sure how to do
> >> direct that, but I expect that you do.
> > 
> > One just cc's stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org ... I don't think the stable
> > team maintains anything that old though.  I cc'd Eric (at Marvell)
> > who is probably worth keeping in the loop in terms of PXA fixes.
> > The PXA platform is now getting an overhaul to help make the
> > PXA3xx changes work better ... yay!
> 
> Am I right to assume that you could/should/would make that cc: after you
> add your Acked-by:

Yeah, but I don't think that particular order matters much.


> > p.s. Eric, it seems the pxa2xx_spi driver is in need of more
> >      active maintaining than it's got.  I seem to have a few
> >      other patches for it in my mailbox too...
> 
> As some of you know, but perhaps not Eric, I am creating a driver that
> is a major expansion of pxa2xx_spi.c, so I am trying to stay involved
> with pxa2xx_spi.c development to limit structural changes that might
> make my work more difficult.  This may be a fool's errand, because my
> changes are so significant that they likely never will be accepted into
> the driver. 

At one point we had discussed having something like an SSP/DMA engine
driver, on top of which could live various protcols ... SPI, maybe I2S
for Alsa-SOC (if AC97 isn't appropriate), data capture/streaming stuff
(which ISTR was more your interest) ... like that?
 

> > There are some additional issues when GPIOs aren't being used as
> > chipselects.  Probably the SSFRM support should be removed, and
> > the GPIO support switched over to use standard GPIO calls (and to
> > support SPI_CS_HIGH).
> 
> I accept your other changes to my patch notes, but I disagree that the
> above paragraph is related to this patch.  I will leave it out unless
> you strongly object.

OK.


> Note that the driver does not specifically do anything to support using
> SSPFRM as chip select.  The documentation says that if no cs_control
> routine is specified, then the driver assumes use of SSPFRM, but really
> it does nothing.  The real assumption is that the user knows what's
> right, and has either hooked up (and is happy with) SSPFRM, or is using
> a (single) chip that doesn't care.
> 
> Possibly a change to Documentation/spi/pxa2xx should be made to
> highlight the ways in which use of SSPFRM differs from use of a GPIO.
> Maybe I will take a shot at that with this patch.

Separate, please.  I'll repost the relevant patch from Vernon ... that
seems like it should become a part of that.

 
> > +		/* see if the next and current messages point
> > +		 * to the same chip
> > +		 */
> > +		if (next_msg && next_msg->spi != msg->spi)
> > +			next_msg = NULL;
> 
> You have changed this from:
> 		if (next_msg)
> 			if (next_msg->spi != msg->spi)
> 				next_msg = NULL;
> 
> My understanding is that C does not guarantee the order of evaluation in
> a compound conditional.  If "next_msg->spi != msg->spi" is evaluated
> before "next_msg" a segmentation fault may occur.  Am I out of date on this?

For "A && B", it's guaranteed that A runs before B;
both values are evaluated as nonzero/true vs zero/false.

For "X & Y" it isn't ... but those values are bit masks
not simple true/false, so they're not usually equivalent.

C has worked that way forever.

- Dave

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

Re: [Patch 0/1] spi: Fix pxa2xx_spi.c transfer delay, cs change, transfer length

[Ned Forrester]

David Brownell wrote:
>> As some of you know, but perhaps not Eric, I am creating a driver that
>> is a major expansion of pxa2xx_spi.c, so I am trying to stay involved
>> with pxa2xx_spi.c development to limit structural changes that might
>> make my work more difficult.  This may be a fool's errand, because my
>> changes are so significant that they likely never will be accepted into
>> the driver. 
> 
> At one point we had discussed having something like an SSP/DMA engine
> driver, on top of which could live various protcols ... SPI, maybe I2S
> for Alsa-SOC (if AC97 isn't appropriate), data capture/streaming stuff
> (which ISTR was more your interest) ... like that?

I have a very narrow view at that moment, and my understanding of the
kernel is especially narrow.  I don't know enough about what you are
suggesting to offer an opinion.  It might work out.  Yes, streaming data
is my interest.

>> Possibly a change to Documentation/spi/pxa2xx should be made to
>> highlight the ways in which use of SSPFRM differs from use of a GPIO.
>> Maybe I will take a shot at that with this patch.
> 
> Separate, please.  I'll repost the relevant patch from Vernon ... that
> seems like it should become a part of that.

Interesting.  OK.  Vernon slipped a note or two about DMA length,
related to one of the two patches you will get today, so maybe his would
work for the SSPFRM issue, too.

> For "A && B", it's guaranteed that A runs before B;
> both values are evaluated as nonzero/true vs zero/false.
> 
> C has worked that way forever.

Hmmm... OK.  I guess I have spent the last 28yrs trying to avoid
reliance on that for naught. :-)

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