LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: David Brownell <david-b@pacbell.net>
To: Ned Forrester <nforrester@whoi.edu>
Cc: spi-devel-general@lists.sourceforge.net, marc.pignat@hevs.ch,
	linux-kernel@vger.kernel.org, anemo@mba.ocn.ne.jp
Subject: Re: [spi-devel-general] [PATCH] atmel_spi: support zero length	 transfer
Date: Fri, 22 Feb 2008 18:37:01 -0800	[thread overview]
Message-ID: <200802221837.02012.david-b@pacbell.net> (raw)
In-Reply-To: <47BF2455.8030904@whoi.edu>

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



  reply	other threads:[~2008-02-23  2:37 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-20 15:54 Atsushi Nemoto
2008-02-20 17:55 ` Marc Pignat
2008-02-21  1:52   ` Atsushi Nemoto
2008-02-21  9:26     ` Marc Pignat
2008-02-21 19:23       ` David Brownell
2008-02-22  9:30         ` Marc Pignat
2008-02-22 14:15           ` Atsushi Nemoto
2008-02-22 14:28             ` [spi-devel-general] " Ned Forrester
2008-02-22 19:06               ` David Brownell
2008-02-22 19:52                 ` Ned Forrester
2008-02-22 18:58             ` David Brownell
2008-02-23  2:55           ` David Brownell
2008-02-25  8:15             ` Marc Pignat
2008-02-22 14:07         ` [spi-devel-general] " Ned Forrester
2008-02-22 19:02           ` David Brownell
2008-02-22 19:36             ` Ned Forrester
2008-02-23  2:37               ` David Brownell [this message]
2008-02-25  0:25               ` Atsushi Nemoto

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200802221837.02012.david-b@pacbell.net \
    --to=david-b@pacbell.net \
    --cc=anemo@mba.ocn.ne.jp \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marc.pignat@hevs.ch \
    --cc=nforrester@whoi.edu \
    --cc=spi-devel-general@lists.sourceforge.net \
    --subject='Re: [spi-devel-general] [PATCH] atmel_spi: support zero length	 transfer' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).