A spi transfer with zero length is not invalid. Such transfer can be
used to achieve delay before first CLK edge after chipselect assertion.
Signed-off-by: Atsushi Nemoto <[email protected]>
---
diff --git a/drivers/spi/atmel_spi.c b/drivers/spi/atmel_spi.c
index 293b7ca..5dff5e0 100644
--- a/drivers/spi/atmel_spi.c
+++ b/drivers/spi/atmel_spi.c
@@ -606,7 +606,7 @@ static int atmel_spi_transfer(struct spi_device *spi, struct spi_message *msg)
return -ESHUTDOWN;
list_for_each_entry(xfer, &msg->transfers, transfer_list) {
- if (!(xfer->tx_buf || xfer->rx_buf)) {
+ if (!(xfer->tx_buf || xfer->rx_buf) && xfer->len) {
dev_dbg(&spi->dev, "missing rx or tx buf\n");
return -EINVAL;
}
Hi!
On Wednesday 20 February 2008, you wrote:
> A spi transfer with zero length is not invalid. Such transfer can be
> used to achieve delay before first CLK edge after chipselect assertion.
How long will be that delay?
If they are really users of that kind of thing, this should be fixed by adding
a "delay_us_before_xfer" field in the struct spi_transfer.
Have you tested it? I think if you start a transfer with 0 len, the ENDRX bit
will never rise, however, I'm not sure about this.
Regards
Marc
On Wed, 20 Feb 2008 18:55:01 +0100, Marc Pignat <[email protected]> wrote:
> > A spi transfer with zero length is not invalid. Such transfer can be
> > used to achieve delay before first CLK edge after chipselect assertion.
> How long will be that delay?
My funny custom device requires 100us or so. Unfortunately atmel_spi
can not use such a slow bitrate.
> If they are really users of that kind of thing, this should be fixed by adding
> a "delay_us_before_xfer" field in the struct spi_transfer.
Yes, it would be an another way to achieve it. But as long as zero
length transfer is legal on this API, I don't want to add other
fields.
> Have you tested it? I think if you start a transfer with 0 len, the ENDRX bit
> will never rise, however, I'm not sure about this.
Yes. I tested it on AT91SAM9260 and it seems ENDRX rises soon.
Though it can be possible to avoid starting DMA for zero length
transfer, I think it is not worth to optimize for such a rare case.
---
Atsushi Nemoto
Hi!
On Thursday 21 February 2008, Atsushi Nemoto wrote:
...
> Yes. I tested it on AT91SAM9260 and it seems ENDRX rises soon.
> Though it can be possible to avoid starting DMA for zero length
> transfer, I think it is not worth to optimize for such a rare case.
Ok, verified to work on at91rm9200, should be tested on AVR32.
David, do you think writing 0 bytes is a valid use of this API?
IMHO, we should add a fied to the spi_transfer struct.
Regards
Marc
> David, do you think writing 0 bytes is a valid use of this API?
Just a zero byte transfer ... no, though it depends what you mean
by "valid". (I'm not sure I'd expect all controller drivers to
reject such requests.) That has no effect on bits-on-the-wire,
and would make trouble for various DMA engines.
Passing zero bytes to get an inline delay at an exact spot in the
overall protocol message ... I don't see why not. Better than
adding delay fields for every spot it might be needed by various
oddball devices, for sure!!
> IMHO, we should add a fied to the spi_transfer struct.
What would that do, exactly?
This *specific* usage might arguably belong in spi_device, since
it's a delay required after chipselect goes active and before any
bytes get transferred. It's clearly not a per-transfer thing, and
should also apply after a temporary mid-message chip deselect.
And it would probably deserve a mode flag (sigh) unless someone
can update every master controller driver.
- Dave
Hi Dave!
On Thursday 21 February 2008, David Brownell wrote:
> > David, do you think writing 0 bytes is a valid use of this API?
>
> Just a zero byte transfer ... no, though it depends what you mean
> by "valid". (I'm not sure I'd expect all controller drivers to
> reject such requests.) That has no effect on bits-on-the-wire,
> and would make trouble for various DMA engines.
So, the behaviour is undefined, something between 'crash my dma engine',
'assert chip select and wait some time', or 'do_nothing'...
...
> This *specific* usage might arguably belong in spi_device, since
> it's a delay required after chipselect goes active and before any
> bytes get transferred. It's clearly not a per-transfer thing, and
> should also apply after a temporary mid-message chip deselect.
Ack, should be in spi_device (same remark for spi_transfer->delay_usec?).
>
> And it would probably deserve a mode flag (sigh) unless someone
> can update every master controller driver.
Supporting the zero-len-write means checking and perhpaps updating each driver
for the benefit of having an unknown length delay.
We should add the delay field in the spi_device, but this means more work.
Is this kind of device so common that we need to do all that work? or can we
leave it as is (verified to work only with atmel_spi)?
Regards
Marc
On Fri, 22 Feb 2008 10:30:31 +0100, Marc Pignat <[email protected]> wrote:
> > > David, do you think writing 0 bytes is a valid use of this API?
> >
> > Just a zero byte transfer ... no, though it depends what you mean
> > by "valid". (I'm not sure I'd expect all controller drivers to
> > reject such requests.) That has no effect on bits-on-the-wire,
> > and would make trouble for various DMA engines.
> So, the behaviour is undefined, something between 'crash my dma engine',
> 'assert chip select and wait some time', or 'do_nothing'...
If the driver could not handle zero length transfer, then the driver
should reject it (just like unsupported transfer mode). Then the
behavior will be 'assert chip select and wait some time' or 'rejected
by the driver'.
> > And it would probably deserve a mode flag (sigh) unless someone
> > can update every master controller driver.
> Supporting the zero-len-write means checking and perhpaps updating
> each driver for the benefit of having an unknown length delay.
>
> We should add the delay field in the spi_device, but this means more work.
>
> Is this kind of device so common that we need to do all that work? or can we
> leave it as is (verified to work only with atmel_spi)?
I think my case is not so common. But if the driver could support
zero length transfer easily, there is no reason to reject it.
And if nobody wanted to support zero length transfer on that driver,
it would be no reason to update it ;)
---
Atsushi Nemoto
Atsushi Nemoto wrote:
> If the driver could not handle zero length transfer, then the driver
> should reject it (just like unsupported transfer mode). Then the
> behavior will be 'assert chip select and wait some time' or 'rejected
> by the driver'.
This would be OK. It would not be hard to fix pxa2xx_spi, for example,
to reject zero-length transfers in DMA mode, as long as it is acceptable
to reject the message in mid-message. If it were necessary to scan a
whole message for zero-length transfers and refuse to queue an offending
message, then that adds burden to all messages.
--
Ned Forrester [email protected]
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
David Brownell wrote:
>> David, do you think writing 0 bytes is a valid use of this API?
>
> Just a zero byte transfer ... no, though it depends what you mean
> by "valid". (I'm not sure I'd expect all controller drivers to
> reject such requests.) That has no effect on bits-on-the-wire,
> and would make trouble for various DMA engines.
FWIW, the pxa2xx_spi driver does not, near as I can tell, reject zero
length transfers, it will go through the motions, the same as for any
other transfer.
However, if the transfer is by DMA, note that the PXA255 and PXA270
Developer's Manuals have the following language regarding DMA lengths:
"LEN = 0 means zero bytes for descriptor-fetch transactions.
LEN = 0 is an invalid setting for no-descriptor-fetch
transactions. Programming LEN = 0 in the descriptor-fetch mode
when DCMD[CmpEn] is clear (normal data transfer mode) causes
the channel to immediately discard the descriptor after it is
fetched from memory. If the descriptor chain has more
descriptors, the channel fetches the next valid descriptor.
The channel stops if the descriptor chain has no more
descriptors."
Because the pxa2xx_spi driver does not currently use DMA descriptors,
zero length DMAs are invalid. I don't know what the DMA controller will
do if given a zero length. If it were using using descriptors (as in my
development code, useful only when chaining transfers that don't need
any SSP parameters or chip selects changed, nor any time delays), then a
zero length transfer would only introduce the insignificant delay of
fetching one 4-word descriptor.
If, on the other hand, DMA is not used, it looks like the zero length
case is handled gracefully. The chip select and other SSP registers are
set, but no bytes are transferred. It does not look like any particular
delay would be involved in this, as all transfers within a message are
handled in interrupt context with an ISR and tasklet. There is not much
code being executed to limit the minimum delay, and the maximum would
depend on interrupt/tasklet latency.
> Passing zero bytes to get an inline delay at an exact spot in the
> overall protocol message ... I don't see why not. Better than
> adding delay fields for every spot it might be needed by various
> oddball devices, for sure!!
I agree with Marc: any such delay will be undefined, in the general
case. It might work for a specific driver implementation.
--
Ned Forrester [email protected]
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
Quoth Atsushi Nemoto on Fri, 22 Feb 2008:
> On Fri, 22 Feb 2008 10:30:31 +0100, Marc Pignat <[email protected]> wrote:
> > > > David, do you think writing 0 bytes is a valid use of this API?
> > >
> > > Just a zero byte transfer ... no, though it depends what you mean
> > > by "valid". (I'm not sure I'd expect all controller drivers to
> > > reject such requests.) That has no effect on bits-on-the-wire,
> > > and would make trouble for various DMA engines.
> >
> > So, the behaviour is undefined,
Not what I said. To repeat: it makes sense to pass zero bytes
in at least one case, which is not "just" a zero byte transfer.
And in a practical sense, until we have some kind of regression
testing scheme -- with some kind of "golden device" -- it's not
very sensible for any SPI Protocol Driver to expect that all SPI
Master Controller Drivers act consistently in such cases.
> > something between 'crash my dma engine',
> > 'assert chip select and wait some time', or 'do_nothing'...
>
> If the driver could not handle zero length transfer, then the driver
> should reject it (just like unsupported transfer mode).
Exactly. Behaviors like "crash my DMA engine" are clearly "invalid",
in *ALL* cases. Bugs to get fixed as soon as they're noticed.
> Then the
> behavior will be 'assert chip select and wait some time' or 'rejected
> by the driver'.
The "wait" mode is what started this thread -- not "just" a zero
byte transfer, but one which does real work.
For "just" a zero byte transfer, I see two main implementation
options ... with no compelling reason to force either one.
- "ignored" ... the implementation sibling of "wait"
- "rejected" ... more work
The argument for "rejected" would seem to be only that this is a
case of "protocol drivers should not do this". But if they don't,
then the difference doesn't matter.
> > > And it would probably deserve a mode flag (sigh) unless someone
> > > can update every master controller driver.
> >
> > Supporting the zero-len-write means checking and perhpaps updating
> > each driver for the benefit of having an unknown length delay.
> >
> > We should add the delay field in the spi_device, but this means more work.
> >
> > Is this kind of device so common that we need to do all that work? or can we
> > leave it as is (verified to work only with atmel_spi)?
>
> I think my case is not so common. But if the driver could support
> zero length transfer easily, there is no reason to reject it.
>
> And if nobody wanted to support zero length transfer on that driver,
> it would be no reason to update it ;)
So long as the controller driver doesn't misbehave, I can't see any
reason to worry about this behavior.
- Dave
> >> David, do you think writing 0 bytes is a valid use of this API?
> >
> > Just a zero byte transfer ... no, though it depends what you mean
> > by "valid". (I'm not sure I'd expect all controller drivers to
> > reject such requests.) That has no effect on bits-on-the-wire,
> > and would make trouble for various DMA engines.
>
> FWIW, the pxa2xx_spi driver does not, near as I can tell, reject zero
> length transfers, it will go through the motions, the same as for any
> other transfer.
Makes sense to me ... although:
> However, if the transfer is by DMA, note that the PXA255 and PXA270
> Developer's Manuals have the following language regarding DMA lengths:
>
> LEN = 0 means zero bytes for descriptor-fetch transactions.
> LEN = 0 is an invalid setting for no-descriptor-fetch
> transactions. ...
>
> Because the pxa2xx_spi driver does not currently use DMA descriptors,
> zero length DMAs are invalid.
In that case the pxa2xx_spi driver should add a special case to
avoid starting such transfers in DMA mode.
> > Passing zero bytes to get an inline delay at an exact spot in the
> > overall protocol message ... I don't see why not. Better than
> > adding delay fields for every spot it might be needed by various
> > oddball devices, for sure!!
>
> I agree with Marc: any such delay will be undefined, in the general
> case. It might work for a specific driver implementation.
Is that what Marc said? I couldn't tell. In any case, I disagree;
the semantics of that delay are clearly defined.
- Dave
> > If the driver could not handle zero length transfer, then the driver
> > should reject it (just like unsupported transfer mode). Then the
> > behavior will be 'assert chip select and wait some time' or 'rejected
> > by the driver'.
>
> This would be OK. It would not be hard to fix pxa2xx_spi, for example,
> to reject zero-length transfers in DMA mode, as long as it is acceptable
> to reject the message in mid-message.
Such "illegal message" rejection is best done early; "fail-fast".
Mid-message rejection isn't wrong, but it's a rude policy.
It'd be best to fix pxa2xx_spi to not start zero-length DMAs.
> If it were necessary to scan a
> whole message for zero-length transfers and refuse to queue an offending
> message, then that adds burden to all messages.
Sanity checking messages as they're submitted is easy; and it's
also the natural point for setting up DMA mappings (and making
those cachelines available for better use).
- Dave
David Brownell wrote:
>> However, if the transfer is by DMA, note that the PXA255 and PXA270
>> Developer's Manuals have the following language regarding DMA lengths:
>>
>> LEN = 0 means zero bytes for descriptor-fetch transactions.
>> LEN = 0 is an invalid setting for no-descriptor-fetch
>> transactions. ...
>>
>> Because the pxa2xx_spi driver does not currently use DMA descriptors,
>> zero length DMAs are invalid.
>
> In that case the pxa2xx_spi driver should add a special case to
> avoid starting such transfers in DMA mode.
So, what I think you said is that it would be better for pxa2xx_spi to
silently ignore a zero-length message, passing it back with the rest of
the message when all is complete, than to reject the message. I see no
reason why that could not be done, though it may be tricky to set other
things like SSP modes and chip select and *not* start the DMA. It would
have to be tested, so I'm not sure when I could try that.
>> I agree with Marc: any such delay will be undefined, in the general
>> case. It might work for a specific driver implementation.
>
> Is that what Marc said? I couldn't tell. In any case, I disagree;
> the semantics of that delay are clearly define.
Maybe I am missing something. Aren't we talking about a transfer in a
message, with or without other transfers, who's only unique
characteristic is that that its length is zero? What is clearly
defined about what should happen during that transfer? I think (maybe
we are all confused) that Marc and I are talking about the delay, likely
short, caused by the processing or ignoring of that zero length transfer.
Or are you and Atsushi talking about using spi_transfer.delay_usecs
*with* a zero length transfer to effectively put a delay between the
assertion of CS and the start of the first clock? If so, then I guess I
missed the original point. Sorry.
--
By the way, reading spi.h again, it looks like spi_transfer.delay_usecs
is supposed to be implemented between the last bit movement of a
transfer and any change of CS at the end of a transfer. Is that right?
I think that pxa2xx_spi is dropping CS, if requested, immediately at
the end of transfer, and then putting spi_transfer.delay_usecs between
that transfer and the next.
--
Ned Forrester [email protected]
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
David Brownell wrote:
>> This would be OK. It would not be hard to fix pxa2xx_spi, for example,
>> to reject zero-length transfers in DMA mode, as long as it is acceptable
>> to reject the message in mid-message.
>
> Such "illegal message" rejection is best done early; "fail-fast".
> Mid-message rejection isn't wrong, but it's a rude policy.
>
> It'd be best to fix pxa2xx_spi to not start zero-length DMAs.
So again, same as my last reply, it would be best to handle a zero
length transfer silently, as if it were non-zero, but just skipping over
any invalid hardware operations due to the zero length.
>> If it were necessary to scan a
>> whole message for zero-length transfers and refuse to queue an offending
>> message, then that adds burden to all messages.
>
> Sanity checking messages as they're submitted is easy; and it's
> also the natural point for setting up DMA mappings (and making
> those cachelines available for better use).
Hmmm.... Obviously I have much to learn about modern computers. It had
not occurred to me, even after reading "Linux Device Drivers", Corbet,
et.al, that by DMA mapping, one frees the cache for other uses. It
makes sense. In my application I pass many large buffers to the master
driver, and I map them in the protocol driver. I did that without good
reason, but now I see it was the proper choice.
Unfortunately, pxa2xx_spi does any DMA mapping not done by the protocol
driver in pump_transfers, as each transfer is submitted to the hardware.
There is not currently any message checking done in transfer(), the
only error return from that is if the master driver is shutdown (queue
stopped). The current scheme is to return the message with non-zero
spi_message.status if it has failed *after* execution has been
attempted. I don't look forward to making major changes, if we really
want all DMA mapping and error checking to be in transfer(), though I
see no reason why it could not be done.
--
Ned Forrester [email protected]
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
On Friday 22 February 2008, Ned Forrester wrote:
>
> So, what I think you said is that it would be better for pxa2xx_spi to
> silently ignore a zero-length message, passing it back with the rest of
> the message when all is complete, than to reject the message.
Yes. Remember that the reason to _use_ such a message is to get the
inline delay ... and that if you've got code to handle that case
(transfer.len == 0 && transfer.delay_usecs != 0) it's almost trivial
to support the degenerate "delay_usecs == 0" case too.
> I see no
> reason why that could not be done, though it may be tricky to set other
> things like SSP modes and chip select and *not* start the DMA. It would
> have to be tested, so I'm not sure when I could try that.
Normally I'd expect it's little more than a "goto", but the pxa2xx_spi
structure is relatively complex.
> >> I agree with Marc: any such delay will be undefined, in the general
> >> case. It might work for a specific driver implementation.
> >
> > Is that what Marc said? I couldn't tell. In any case, I disagree;
> > the semantics of that delay are clearly define.
>
> Maybe I am missing something. Aren't we talking about a transfer in a
> message, with or without other transfers, who's only unique
> characteristic is that that its length is zero?
There were two cases ... delay_usecs being zero, or not. In either
case, the semantics are the same: after the transfer (len == 0),
delay that many microseconds (zero or more).
> Or are you and Atsushi talking about using spi_transfer.delay_usecs
> *with* a zero length transfer to effectively put a delay between the
> assertion of CS and the start of the first clock? If so, then I guess I
> missed the original point. Sorry.
As I noted, there are two cases. The reason to use a zero length
transfer is to get a delay ... in his case, specifically before
the first clock, but in general *anywhere* in the transfer. But
the degenerate case is "delay for zero microseconds".
> --
>
> By the way, reading spi.h again, it looks like spi_transfer.delay_usecs
> is supposed to be implemented between the last bit movement of a
> transfer and any change of CS at the end of a transfer. Is that right?
Yes.
> I think that pxa2xx_spi is dropping CS, if requested, immediately at
> the end of transfer, and then putting spi_transfer.delay_usecs between
> that transfer and the next.
That's a bug then; it will matter with some drivers.
> >> ?????If it were necessary to scan a
> >> whole message for zero-length transfers and refuse to queue an offending
> >> message, then that adds burden to all messages.
> >
> > Sanity checking messages as they're submitted is easy; and it's
> > also the natural point for setting up DMA mappings (and making
> > those cachelines available for better use).
>
> Hmmm.... Obviously I have much to learn about modern computers. ?It had
> not occurred to me, even after reading "Linux Device Drivers", Corbet,
> et.al, that by DMA mapping, one frees the cache for other uses. ?It
> makes sense.
That's certainly what happens on systems where the buffer is from
"normal" memory and the cache is DMA-incoherent ... like most ARMs,
and probably the majority of non-x86 hardware. Think of that as
being a level or two deeper than what LDD covers.
> In my application I pass many large buffers to the master
> driver, and I map them in the protocol driver. ?I did that without good
> reason, but now I see it was the proper choice.
>
> Unfortunately, pxa2xx_spi does any DMA mapping not done by the protocol
> driver in pump_transfers, as each transfer is submitted to the hardware.
That's not wrong ... but it's sub-optimal: those cache lines could
have been doing better things all that time, and cleaning them out
in the middle of a transfer will slow it down by some amount.
> ?There is not currently any message checking done in transfer(), the
> only error return from that is if the master driver is shutdown (queue
> stopped). ?The current scheme is to return the message with non-zero
> spi_message.status if it has failed *after* execution has been
> attempted.
Again ... not wrong, but sub-optimal: when it happens (which won't be
common, fortunately!) the SPI slave will be left in a rude state. And
the protocol driver may not know how to recover.
> I don't look forward to making major changes, if we really
> want all DMA mapping and error checking to be in transfer(), though I
> see no reason why it could not be done.
There's no rush on cleanups like that. They'd be reasonable tasks for
someone with some time to spare, who wanted to get their feet wet in
some driver updates and who could set up some kind of test rig. (Like
one with a programmable slave that could be made to do all sorts of stuff
that might otherwise be a bit uncommon. A microcontroller slave would be
easy ... programming a CPLD or FPGA would be much trickier!)
- Dave
> > > David, do you think writing 0 bytes is a valid use of this API?
> >
> > Just a zero byte transfer ... no, though it depends what you mean
> > by "valid". (I'm not sure I'd expect all controller drivers to
> > reject such requests.) That has no effect on bits-on-the-wire,
> > and would make trouble for various DMA engines.
>
> So, the behaviour is undefined, something between 'crash my dma engine',
> 'assert chip select and wait some time', or 'do_nothing'...
No, it's fully defined. "Crash my engine" is not OK. The delay
is controlled by transfer.delay_usecs ... possibly zero, which is
best viewed as a degenerate case.
> Is this kind of device so common that we need to do all that work? or can we
> leave it as is (verified to work only with atmel_spi)?
You can't avoid testing each combination of SPI protocol driver
and SPI master controller driver you rely on ... especially when
protocol tweaking options (cs_change, delay_usecs, bits_per_word,
etc) are used at the per-transfer level. This driver stack isn't
as readily tested as, say, USB.
However, protocol drivers should be able to rely on controller
drivers to reject requests that they don't even try to handle;
that's basic.
- Dave
On Fri, 22 Feb 2008 14:36:53 -0500, Ned Forrester <[email protected]> wrote:
> Or are you and Atsushi talking about using spi_transfer.delay_usecs
> *with* a zero length transfer to effectively put a delay between the
> assertion of CS and the start of the first clock? If so, then I guess I
> missed the original point. Sorry.
Yes. I want to use zero length transfer with delay_usecs to achieve
CS-CLK delay for my funny device. And I was too lazy to reject true
nop transfer.
---
Atsushi Nemoto
On Saturday 23 February 2008, David Brownell wrote:
...
> No, it's fully defined. "Crash my engine" is not OK. The delay
> is controlled by transfer.delay_usecs ... possibly zero, which is
> best viewed as a degenerate case.
Ooops, I missed that (the delay *is* defined in transfer.delay_usecs).
So I agree, it's fully defined.
Regards
Marc