Re: [PATCH] spi/bcm63xx: fix multi transfer messages

[Florian Fainelli <florian-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>]

Le lundi 26 novembre 2012 16:33:21, Mark Brown a écrit :
> On Mon, Nov 26, 2012 at 04:23:18PM +0100, Jonas Gorski wrote:
> > On 26 November 2012 16:03, Mark Brown
> > 
> > > At least with the current approach these devices will work at all which
> > > is much simpler to understand and debug.
> > 
> > No they don't. If you do a spi_write_then_read on this controller with
> 
> > the current code the following will happen:
> Sorry, there was a missing not there.
> 
> > Also with the current code writes with more than buffer size would
> > silently fail, while with my changes they would at least be rejected
> > so you know that they failed. There is no way of writing or reading
> > more than that fits into the hardware FIFO buffer in one go. After
> > having read/written that amount CS will be deasserted. There is no
> > possiblity of refilling the buffers before it deasserts CS. And this
> > usually leads to an aborted command if the device expected more data.
> 
> What, wait a minute.  Are you saying that this hardware can't do
> transfers of more than a single FIFO at all?  That's appaulingly crap,
> we need some way for client drivers to tell they're dealing with such
> controllers.  Things like regmap really need to know so they can provide
> fallbacks.

No, the hardware cannot do transfers which are wider than a FIFO size, we 
could obviously workaround that by allocating a bounce buffer and re-fill the 
FIFO when the transfer is wider. You can call that crap, but that is just 
hardware out there now, so we need to cope with that.

> 
> If the hardware is that bad I guess we can't do any better than your
> patch but we need to be clear what's going on.  I would recommend doing
> two patches, one of which enforces checks on what can actually work and
> a second which extends this to the limited set of things that it can be
> made to cope with.

Ok, makes sense.

> 
> > > Like I've said several times now just linearise everything before you
> > > talk to the controller which is your second option here...
> > 
> > ? This is exactly what I'm trying to do with this patch.
> 
> No, you're doing stuff with the FIFO which is obviously talking to the
> hardware...

The way it works is pretty simple, you explicitely need to *copy* the actual 
data to the FIFO hardware buffer and tell the SPI controller to process the 
FIFO contents. There is no way to provide a DMA-able linearized buffer.

> 
> > >> All these fail when not using the prepend bytes method or merging the
> > >> two transfers into one full duplex transfer.
> > > 
> > > ...and will work for everything at the expsense of being slower due to
> > > the need to allocate the buffer.
> > 
> > I do not see where you get this "allocate" from. The tx/rx buffers are
> > hardware registers, always there, with a fixed length, and any data
> > sent to the SPI device needs to be written there beforehand, then a
> > "write now" command needs to be issued to a different register.
> 
> I think this is the bit above about the hardware being awful.  This is
> again something unclear from your patch, it's not clear that this is
> something the hardware needs rather than a cute performance trick to
> avoid linearising things in memory.

Just to clarify things a little here, as you have been mentionning linearizing 
several times, and I suspect Jonas and myself have a different understand of 
what you mean here.

Considering that there is no way to hand off a DMA-able buffer to the hardware, 
what Jonas does here, is walk the list of transfers and copy the transfer 
buffers to the FIFO, actually peforming linearization in the FIFO hardware 
buffer.

If you meant something like allocating a kernel buffer and linearize the 
transfer list buffers there, I see no point in doing this considering the 
limitations mentionned before, we would end up with copying the entire kernel 
buffer back to the FIFO hardware buffer. The only advantage being that we do not 
have the FIFO size limitation.

There are quite some quirky SPI controllers supported, yet that one is still 
pretty straight forward to be worked around.
-- 
Florian

------------------------------------------------------------------------------
Keep yourself connected to Go Parallel: 
DESIGN Expert tips on starting your parallel project right.
http://goparallel.sourceforge.net
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general