linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] spi/bcm63xx: fix multi transfer messages
@ 2012-11-14 22:22 Jonas Gorski
       [not found] ` <20121114231650.GC4536@opensource.wolfsonmicro.com>
  0 siblings, 1 reply; 7+ messages in thread
From: Jonas Gorski @ 2012-11-14 22:22 UTC (permalink / raw)
  To: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
  Cc: Maxime Bizon, Mark Brown, Florian Fainelli, Kevin Cernekee

The BCM63XX SPI controller does not support keeping CS asserted after
sending its buffer. This breaks common usages like spi_write_then_read,
where it is expected to be kept active during the whole transfers.

Work around this by combining the transfers into one if the buffer
allows. For spi_write_then_read, use the prepend byte feature to write
to "prepend" the write if it is less than 15 bytes, allowing the whole
fifo size for the read.

Signed-off-by: Jonas Gorski <jonas.gorski-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
Tested on a SPI conntected switch which required keeping CS active between
the register read command and reading the register contents.

Based on Mark's spi/next.

Not sure if this is stable material, as it's quite invasive.

 drivers/spi/spi-bcm63xx.c |  172 ++++++++++++++++++++++++++++++---------------
 1 file changed, 117 insertions(+), 55 deletions(-)

diff --git a/drivers/spi/spi-bcm63xx.c b/drivers/spi/spi-bcm63xx.c
index 6d97047..da557f9 100644
--- a/drivers/spi/spi-bcm63xx.c
+++ b/drivers/spi/spi-bcm63xx.c
@@ -37,6 +37,8 @@
 
 #define PFX		KBUILD_MODNAME
 
+#define BCM63XX_SPI_MAX_PREPEND		15
+
 struct bcm63xx_spi {
 	struct completion	done;
 
@@ -49,16 +51,10 @@ struct bcm63xx_spi {
 	unsigned int		msg_type_shift;
 	unsigned int		msg_ctl_width;
 
-	/* Data buffers */
-	const unsigned char	*tx_ptr;
-	unsigned char		*rx_ptr;
-
 	/* data iomem */
 	u8 __iomem		*tx_io;
 	const u8 __iomem	*rx_io;
 
-	int			remaining_bytes;
-
 	struct clk		*clk;
 	struct platform_device	*pdev;
 };
@@ -175,50 +171,60 @@ static int bcm63xx_spi_setup(struct spi_device *spi)
 	return 0;
 }
 
-/* Fill the TX FIFO with as many bytes as possible */
-static void bcm63xx_spi_fill_tx_fifo(struct bcm63xx_spi *bs)
-{
-	u8 size;
-
-	/* Fill the Tx FIFO with as many bytes as possible */
-	size = bs->remaining_bytes < bs->fifo_size ? bs->remaining_bytes :
-		bs->fifo_size;
-	memcpy_toio(bs->tx_io, bs->tx_ptr, size);
-	bs->remaining_bytes -= size;
-}
-
 static unsigned int bcm63xx_txrx_bufs(struct spi_device *spi,
-					struct spi_transfer *t)
+					struct spi_transfer *first,
+					unsigned int n_transfers)
 {
 	struct bcm63xx_spi *bs = spi_master_get_devdata(spi->master);
 	u16 msg_ctl;
 	u16 cmd;
+	unsigned int i, timeout, total_len = 0, prepend_len = 0, len = 0;
+	struct spi_transfer *t = first;
+	u8 rx_tail;
+	bool do_rx = false;
+	bool do_tx = false;
 
 	/* Disable the CMD_DONE interrupt */
 	bcm_spi_writeb(bs, 0, SPI_INT_MASK);
 
-	dev_dbg(&spi->dev, "txrx: tx %p, rx %p, len %d\n",
-		t->tx_buf, t->rx_buf, t->len);
+	if (n_transfers > 1 && t->tx_buf && t->len <= BCM63XX_SPI_MAX_PREPEND)
+		prepend_len = t->len;
 
-	/* Transmitter is inhibited */
-	bs->tx_ptr = t->tx_buf;
-	bs->rx_ptr = t->rx_buf;
+	/* prepare the buffer */
+	for (i = 0; i < n_transfers; i++) {
+		if (t->tx_buf) {
+			do_tx = true;
+			memcpy_toio(bs->tx_io + total_len, t->tx_buf, t->len);
+
+			/* don't prepend more than one tx */
+			if (t != first)
+				prepend_len = 0;
+		}
 
-	if (t->tx_buf) {
-		bs->remaining_bytes = t->len;
-		bcm63xx_spi_fill_tx_fifo(bs);
+		if (t->rx_buf) {
+			do_rx = true;
+			if (t == first)
+				prepend_len = 0;
+		}
+
+		total_len += t->len;
+
+		t = list_entry(t->transfer_list.next, struct spi_transfer,
+			       transfer_list);
 	}
 
+	len = total_len - prepend_len;
+
 	init_completion(&bs->done);
 
 	/* Fill in the Message control register */
-	msg_ctl = (t->len << SPI_BYTE_CNT_SHIFT);
+	msg_ctl = (len << SPI_BYTE_CNT_SHIFT);
 
-	if (t->rx_buf && t->tx_buf)
+	if (do_rx && do_tx && prepend_len == 0)
 		msg_ctl |= (SPI_FD_RW << bs->msg_type_shift);
-	else if (t->rx_buf)
+	else if (do_rx)
 		msg_ctl |= (SPI_HD_R << bs->msg_type_shift);
-	else if (t->tx_buf)
+	else if (do_tx)
 		msg_ctl |= (SPI_HD_W << bs->msg_type_shift);
 
 	switch (bs->msg_ctl_width) {
@@ -232,14 +238,41 @@ static unsigned int bcm63xx_txrx_bufs(struct spi_device *spi,
 
 	/* Issue the transfer */
 	cmd = SPI_CMD_START_IMMEDIATE;
-	cmd |= (0 << SPI_CMD_PREPEND_BYTE_CNT_SHIFT);
+	cmd |= (prepend_len << SPI_CMD_PREPEND_BYTE_CNT_SHIFT);
 	cmd |= (spi->chip_select << SPI_CMD_DEVICE_ID_SHIFT);
 	bcm_spi_writew(bs, cmd, SPI_CMD);
 
 	/* Enable the CMD_DONE interrupt */
 	bcm_spi_writeb(bs, SPI_INTR_CMD_DONE, SPI_INT_MASK);
 
-	return t->len - bs->remaining_bytes;
+	timeout = wait_for_completion_timeout(&bs->done, HZ);
+	if (!timeout)
+		return -ETIMEDOUT;
+
+	/* read out all data */
+	rx_tail = bcm_spi_readb(bs, SPI_RX_TAIL);
+
+	if (do_rx && rx_tail != len)
+		return -EINVAL;
+
+	if (!rx_tail)
+		return total_len;
+
+	len = 0;
+	t = first;
+	/* Read out all the data */
+	for (i = 0; i < n_transfers; i++) {
+		if (t->rx_buf)
+			memcpy_fromio(t->rx_buf, bs->rx_io + len, t->len);
+
+		if (t != first || prepend_len == 0)
+			len += t->len;
+
+		t = list_entry(t->transfer_list.next, struct spi_transfer,
+			       transfer_list);
+	}
+
+	return total_len;
 }
 
 static int bcm63xx_spi_prepare_transfer(struct spi_master *master)
@@ -264,42 +297,71 @@ static int bcm63xx_spi_transfer_one(struct spi_master *master,
 					struct spi_message *m)
 {
 	struct bcm63xx_spi *bs = spi_master_get_devdata(master);
-	struct spi_transfer *t;
+	struct spi_transfer *t, *first = NULL;
 	struct spi_device *spi = m->spi;
 	int status = 0;
-	unsigned int timeout = 0;
-
+	unsigned int n_transfers = 0, total_len = 0;
+	bool can_use_prepend = false;
+
+	/*
+	 * This SPI controller does not support keeping CS active after a
+	 * transfer, so we need to combine the transfers into one until we may
+	 * deassert CS.
+	 */
 	list_for_each_entry(t, &m->transfers, transfer_list) {
-		unsigned int len = t->len;
-		u8 rx_tail;
-
 		status = bcm63xx_spi_check_transfer(spi, t);
 		if (status < 0)
 			goto exit;
 
-		/* configure adapter for a new transfer */
-		bcm63xx_spi_setup_transfer(spi, t);
+		if (!first)
+			first = t;
 
-		while (len) {
-			/* send the data */
-			len -= bcm63xx_txrx_bufs(spi, t);
+		n_transfers++;
+		total_len += t->len;
 
-			timeout = wait_for_completion_timeout(&bs->done, HZ);
-			if (!timeout) {
-				status = -ETIMEDOUT;
-				goto exit;
-			}
+		if (n_transfers == 2 && !first->rx_buf && !t->tx_buf &&
+		    first->len <= BCM63XX_SPI_MAX_PREPEND)
+			can_use_prepend = true;
+		else if (can_use_prepend && t->tx_buf)
+			can_use_prepend = false;
+
+		if ((can_use_prepend &&
+		     total_len > (bs->fifo_size + BCM63XX_SPI_MAX_PREPEND)) ||
+		    (!can_use_prepend && total_len > bs->fifo_size)) {
+			status = -EINVAL;
+			goto exit;
+		}
 
-			/* read out all data */
-			rx_tail = bcm_spi_readb(bs, SPI_RX_TAIL);
+		/* all transfers have to be made at the same speed */
+		if (t->speed_hz != first->speed_hz) {
+			status = -EINVAL;
+			goto exit;
+		}
 
-			/* Read out all the data */
-			if (rx_tail)
-				memcpy_fromio(bs->rx_ptr, bs->rx_io, rx_tail);
+		/* CS will be deasserted directly after the transfer */
+		if (t->delay_usecs) {
+			status = -EINVAL;
+			goto exit;
 		}
 
-		m->actual_length += t->len;
+		if (t->cs_change ||
+		    list_is_last(&t->transfer_list, &m->transfers)) {
+			/* configure adapter for a new transfer */
+			bcm63xx_spi_setup_transfer(spi, first);
+
+			status = bcm63xx_txrx_bufs(spi, first, n_transfers);
+			if (status < 0)
+				goto exit;
+
+			m->actual_length += status;
+			first = NULL;
+			status = 0;
+			n_transfers = 0;
+			total_len = 0;
+			can_use_prepend = false;
+		}
 	}
+
 exit:
 	m->status = status;
 	spi_finalize_current_message(master);
-- 
1.7.10.4


------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] spi/bcm63xx: fix multi transfer messages
       [not found]   ` <20121114231650.GC4536-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
@ 2012-11-14 23:33     ` Jonas Gorski
       [not found]       ` <20121115011504.GA7599@opensource.wolfsonmicro.com>
  0 siblings, 1 reply; 7+ messages in thread
From: Jonas Gorski @ 2012-11-14 23:33 UTC (permalink / raw)
  To: Mark Brown
  Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	Kevin Cernekee, Florian Fainelli, Maxime Bizon

On 15 November 2012 00:17, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Wed, Nov 14, 2012 at 11:22:26PM +0100, Jonas Gorski wrote:
>> The BCM63XX SPI controller does not support keeping CS asserted after
>> sending its buffer. This breaks common usages like spi_write_then_read,
>> where it is expected to be kept active during the whole transfers.
>
>> Work around this by combining the transfers into one if the buffer
>> allows. For spi_write_then_read, use the prepend byte feature to write
>> to "prepend" the write if it is less than 15 bytes, allowing the whole
>> fifo size for the read.
>
> This is only going to work around a relatively small subset of cases so
> it doesn't seem worth rushing in.  The normal fix for such issues is to
> control /CS as a GPIO.

Well, one of the "small" subsets is SPI attached flash, those need
write then read or write then write with CS asserted for most
operations, too. And I'm not sure if controlling CS as a GPIO is an
option on bcm63xx, at least not for the CS's commonly used (the others
have GPIO overlays on some chips).
OTOH booting from SPI flash isn't properly supported yet anyway in
vanilla linux for bcm63xx.


Jonas

------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] spi/bcm63xx: fix multi transfer messages
       [not found]         ` <20121115011504.GA7599-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
@ 2012-11-26 13:20           ` Florian Fainelli
       [not found]             ` <20121126141005.GJ9411@opensource.wolfsonmicro.com>
  0 siblings, 1 reply; 7+ messages in thread
From: Florian Fainelli @ 2012-11-26 13:20 UTC (permalink / raw)
  To: Mark Brown
  Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	Kevin Cernekee, Maxime Bizon, Jonas Gorski

On Thursday 15 November 2012 10:15:08 Mark Brown wrote:
> On Thu, Nov 15, 2012 at 12:33:31AM +0100, Jonas Gorski wrote:
> > On 15 November 2012 00:17, Mark Brown
> 
> > > This is only going to work around a relatively small subset of cases so
> > > it doesn't seem worth rushing in.  The normal fix for such issues is to
> > > control /CS as a GPIO.
> 
> > Well, one of the "small" subsets is SPI attached flash, those need
> > write then read or write then write with CS asserted for most
> > operations, too. And I'm not sure if controlling CS as a GPIO is an
> 
> This is the case for essentially all devices with registers too.  If it
> were always fixing the issue (eg, by allocating a buffer if the one
> supplied isn't suitable) that'd be fine but instead it's going to work
> some but not all of the time which seems non-ideal

Devices that we care about on bcm63xx, which are connected to these built-in
chip-selects are:
- SPI flashes
- SPI Ethernet switches (register based)
- SPI SLIC/SLAC (register based as well)

I assume any hardware design which needs specific CS treatment would wire it
to a spare GPIO to fully control its assertion.

Would you accept this patch provided that Jonas updates his commit log with
this description of devices we support?
--
Florian


------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] spi/bcm63xx: fix multi transfer messages
       [not found]               ` <20121126141005.GJ9411-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
@ 2012-11-26 14:48                 ` Jonas Gorski
       [not found]                   ` <20121126150344.GN9411@opensource.wolfsonmicro.com>
  0 siblings, 1 reply; 7+ messages in thread
From: Jonas Gorski @ 2012-11-26 14:48 UTC (permalink / raw)
  To: Mark Brown
  Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	Kevin Cernekee, Florian Fainelli, Maxime Bizon

On 26 November 2012 15:10, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Mon, Nov 26, 2012 at 02:20:56PM +0100, Florian Fainelli wrote:
>
>> I assume any hardware design which needs specific CS treatment would wire it
>> to a spare GPIO to fully control its assertion.
>
> We're talking about very standard stuff here, not odd corner use cases.

Blame Broadcom engineers. And to top it, they did it again with their
"high speed" SPI controller for their newer chips; I have a driver
ready that will give you a deja vu.

>> Would you accept this patch provided that Jonas updates his commit log with
>> this description of devices we support?
>
> To be honest I'd rather see this as a preformance optimisation on top of
> a generic fix based on something like allocating buffers to linearise.

Just curious, where do you see a performance optimisation? At least I
did not intend to do any.

The buffers are hardware registers with a fixed length, there is no
allocation at all. If it were possible to expand/allocate them I'd
gladly do it. So this patch is the attempt to accept as many transfer
combinations as possible under these constraints.

> Otherwise we're into things like devices mostly working but then falling
> over for large transfers which is going to be annoying to debug.

Unfortunately this is a hardware limitation for which Broadcom does
not offer any workarounds (except this combination logic). And there
are millions of devices out there; you can't just tell the users
"please resolder the CS line so it can be controlled through GPIO. At
least not most of them ;-)

> As it stands as well as the commit log it'd definitely need some
> comments in the code explaining the cuteness and the FIFO size
> limitations (possibly even log messages if we hit them) - it's not the
> coalescing that's the issue for me, it's the very specific and not
> terribly obvious subset of cases it's worked around for.

Unfortunately it's (as Florian said) 99% of use cases for the SPI controller:

- Register accesses to managed switches (write-then-read, with small
messages (write 2 bytes then read up to 8 for reading, write up to 10
for writing registers).
- Flash access using m25p80 (these also need write-then-read access
for reading from the flash, or combining the two write transfers so
writes actually succeed).
- SLIC/SLAC accesses (usually write-then-read/write-write, too)
- anything that uses spi_write_then_read().

All these fail when not using the prepend bytes method or merging the
two transfers into one full duplex transfer.

But sure, I can add logging when transfers get rejected.


Jonas

------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] spi/bcm63xx: fix multi transfer messages
       [not found]                     ` <20121126150344.GN9411-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
@ 2012-11-26 15:23                       ` Jonas Gorski
       [not found]                         ` <20121126153321.GO9411@opensource.wolfsonmicro.com>
  0 siblings, 1 reply; 7+ messages in thread
From: Jonas Gorski @ 2012-11-26 15:23 UTC (permalink / raw)
  To: Mark Brown
  Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	Kevin Cernekee, Florian Fainelli, Maxime Bizon

On 26 November 2012 16:03, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Mon, Nov 26, 2012 at 03:48:55PM +0100, Jonas Gorski wrote:
>> On 26 November 2012 15:10, Mark Brown
>
>> > To be honest I'd rather see this as a preformance optimisation on top of
>> > a generic fix based on something like allocating buffers to linearise.
>
>> Just curious, where do you see a performance optimisation? At least I
>> did not intend to do any.
>
>> The buffers are hardware registers with a fixed length, there is no
>> allocation at all. If it were possible to expand/allocate them I'd
>> gladly do it. So this patch is the attempt to accept as many transfer
>> combinations as possible under these constraints.
>
> That's the problem - you're doing this in a fairly tricky and limited
> way using the hardware features.  I wouldn't want to be sitting trying
> to debug why one of my SPI drivers fails for some subset of I/O
> operations...  for example, if we implement bulk write support for
> register cache sync in regmap then suddenly devices will start failing
> if they end up writing more than N registers in a block during cache
> sync.
>
> 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:

controller asserts CS, sends transfer 1 (the write) to device, deasserts CS.
-> device discards response.
controller asserts CS, reads for transfer 2 (the read), deasserts CS.
-> device will not reply, the buffer contains only 0xff's.

I don't see how this patch will make it worse.

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.

>> > Otherwise we're into things like devices mostly working but then falling
>> > over for large transfers which is going to be annoying to debug.
>
>> Unfortunately this is a hardware limitation for which Broadcom does
>> not offer any workarounds (except this combination logic). And there
>> are millions of devices out there; you can't just tell the users
>> "please resolder the CS line so it can be controlled through GPIO. At
>> least not most of them ;-)
>
> 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.

>> 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.


Jonas

------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] spi/bcm63xx: fix multi transfer messages
       [not found]                           ` <20121126153321.GO9411-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
@ 2012-11-27 20:41                             ` Florian Fainelli
       [not found]                               ` <20121128092628.GI32691@opensource.wolfsonmicro.com>
  0 siblings, 1 reply; 7+ messages in thread
From: Florian Fainelli @ 2012-11-27 20:41 UTC (permalink / raw)
  To: Mark Brown
  Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	Kevin Cernekee, Maxime Bizon, Jonas Gorski

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] spi/bcm63xx: fix multi transfer messages
       [not found]                                 ` <20121128092628.GI32691-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
@ 2012-11-28  9:29                                   ` Florian Fainelli
  0 siblings, 0 replies; 7+ messages in thread
From: Florian Fainelli @ 2012-11-28  9:29 UTC (permalink / raw)
  To: Mark Brown
  Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	Kevin Cernekee, Maxime Bizon, Jonas Gorski

On Wednesday 28 November 2012 09:26:28 Mark Brown wrote:
> On Tue, Nov 27, 2012 at 09:41:43PM +0100, Florian Fainelli wrote:
> > 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:
> 
> > > > 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.
> 
> Sorry, I'm confused now.  Your first comment above sounded like
> refilling the FIFO wasn't an option?

I realized this right after writing it, this would be a workaround, not a real
solution, because on the wire, this would still appear as a different SPI
transfer. Whenever your transfer spans accross a hardware FIFO size, you have
to generate as many distinct "hardware" SPI transfers at the controller level.

> 
> > 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.
> 
> That seems like a pretty big advantage to be honest - like I say, it's
> very difficult to run generic code on top of SPI controllers with random
> properties especially since they're non-discoverable.
> 
> > There are quite some quirky SPI controllers supported, yet that one is still 
> > pretty straight forward to be worked around.
> 
> Well, what you're saying is that it's just not possible to work around
> it at all.

Correct, the best we can do is add some kind of flag and tell the SPI subsystem
we don't support physical transfers more than N sized.
--
Florian


------------------------------------------------------------------------------
Keep yourself connected to Go Parallel: 
INSIGHTS What's next for parallel hardware, programming and related areas?
Interviews and blogs by thought leaders keep you ahead of the curve.
http://goparallel.sourceforge.net
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2012-11-28  9:29 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-14 22:22 [PATCH] spi/bcm63xx: fix multi transfer messages Jonas Gorski
     [not found] ` <20121114231650.GC4536@opensource.wolfsonmicro.com>
     [not found]   ` <20121114231650.GC4536-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
2012-11-14 23:33     ` Jonas Gorski
     [not found]       ` <20121115011504.GA7599@opensource.wolfsonmicro.com>
     [not found]         ` <20121115011504.GA7599-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
2012-11-26 13:20           ` Florian Fainelli
     [not found]             ` <20121126141005.GJ9411@opensource.wolfsonmicro.com>
     [not found]               ` <20121126141005.GJ9411-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
2012-11-26 14:48                 ` Jonas Gorski
     [not found]                   ` <20121126150344.GN9411@opensource.wolfsonmicro.com>
     [not found]                     ` <20121126150344.GN9411-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
2012-11-26 15:23                       ` Jonas Gorski
     [not found]                         ` <20121126153321.GO9411@opensource.wolfsonmicro.com>
     [not found]                           ` <20121126153321.GO9411-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
2012-11-27 20:41                             ` Florian Fainelli
     [not found]                               ` <20121128092628.GI32691@opensource.wolfsonmicro.com>
     [not found]                                 ` <20121128092628.GI32691-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
2012-11-28  9:29                                   ` Florian Fainelli

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).