Re: [PATCH 3/5] dw_spi: rework message processing

[Grant Likely]

On Thu, Jun 16, 2011 at 09:03:49AM -0700, Dirk Brandewie wrote:
> On 06/16/2011 06:14 AM, Grant Likely wrote:
> >On Wed, Jun 15, 2011 at 10:23:06AM -0700, dirk.brandewie@xxxxxxxxx wrote:
> >>From: Dirk Brandewie<dirk.brandewie@xxxxxxxxx>
> >>
> >>NOTE: patch created git format-patch --break-rewrites=/50%
> >>
> >>This patch reworks the message pump worker thread function to run
> >>until all messages queued to the driver have been handled. The
> >>function to handle individual spi_transfers is now a synchronus
> >>function the tasklet to handle spi_transfers has been removed. Work
> >>for the worker thread is only queued in host controller transfer
> >>function.
> >>
> >>Psuedo code for new thread function:
> >>   message = get_message()
> >>   while (message){
> >>     for_each_transfer_in_msg(message){
> >>       transfer_setup(transfer)
> >>       do_transfer()
> >>     }
> >>     complete_message()
> >>     message = get_message()
> >>   }
> >>
> >>Changes that fell out of the message thread changes:
> >>Non-DMA transfers that are larger than the size of the controller FIFO
> >>are handled as interrupt driven transfers.
> >>
> >>Common FIFO handling functions shared PIO and interrupt transfers.
> >>
> >>Simplified queue stop/start funcitons.
> >>
> >>Cleanup fixes:
> >>Changed exported all exported function names to have dw_spi_ prefix
> >>
> >>Removed support for registering chip select control function. Setting
> >>the slave chip select is handled by setting the SER (Slave enable
> >>register)
> >
> >What about for implementations that use a GPIO for the SPI chip
> >select?  It is very common for board designs to use GPIOs for
> >multiplexing the SPI bus.
> 
> OK I can put that back. I took it out for the following reason.
> 
> The Slave Enable Register (SER) must be set for the IP block to
> start the transfer.  The bit in SER directly control a separate chip
> select pin while there is an active transfer.  This gives four chip
> selects in the direct mapping case and up to fifteen if the slave
> chip selects are muxed together.
> 
> >
> >>
> >>Removed code that looked at the cs_change hint in the
> >>spi_transfer. Software has no contorl over whether the slave chip
> >>select is de-asserted at the end of the transfer.  Once the TX FIFO
> >>goes empty the slave chip select is dropped.
> >
> >This sounds wrong.  cs_change is *not* merely a hint.  It must be respected.
> >If the driver has no direct control over the CS line, then it is
> >incumbent on the driver to guaranteed that the cs deassert condition
> >does not occur.  This will probably mean chaining up all the transfers
> >in a message so that the TX FIFO remains full.  If cs_change is
> >requested, then the FIFO must be allowed to empty before kicking of
> >the next group of transfers.
> >
> 
> I agree it is wrong that the driver can not directly control the chip select :-)
> 
> I could probably manage to get the PIO and interrupt case to honor
> this but would add a fair bit of complexity, I have no idea how to
> make the DMA case work I am open to suggestions :-).  The current
> driver has this issue I just made it explicit maybe we should add a
> warning message in the driver to let client driver writers aware of
> the Silicon behaviour.

Use scatter/gather DMA or a bounce buffer to chain up contiguous
transfers into a single DMA operation.  "Best effort" is not okay in
this situation, the behaviour is required even if it does add
complexity.

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/