[PATCH V2 0/2] spi/bcm63xx: fix multi transfer messages

[PATCH V2 0/2] spi/bcm63xx: fix multi transfer messages

[Jonas Gorski]

The bcm63xx SPI controller does not support keeping CS up after doing a
transfer. Since this is problematic for most typical use cases, this
patchset introduces a workaround by combining small enough messages
to one transfer, rejecting anything that can't be fulfilled with the
hardware.

Patch one properly rejects anything impossible to transfer with these
limitations.
Patch two introduces logic for combining transfers to one to be able to
use it for typical use cases (register accesses and flash access).

Build and run tested on a BCM6368 with a SPI controlled switch attached
requiring write-then-read with CS asserted.

Changes V1 -> V2:
 * split into two patches
 * fixed return type of bcm63xx_txrx_bufs()
 * slightly reworked bcm63xx_txrx_bufs, obsoleting one local variable
 * added a bit more comments in the code
 * added error messages indicated why transfers were rejected

Jonas Gorski (2):
  spi/bcm63xx: reject transfers unable to transfer
  spi/bcm63xx: work around inability to keep CS up

 drivers/spi/spi-bcm63xx.c |  179 +++++++++++++++++++++++++++++++--------------
 1 file changed, 125 insertions(+), 54 deletions(-)

-- 
1.7.10.4


------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_jan

[PATCH V2 1/2] spi/bcm63xx: reject transfers unable to transfer

[Jonas Gorski]

The hardware does not support keeping CS asserted after sending one
FIFO buffer worth of data, so reject transfers requiring CS being kept
asserted, either between transers or for a certain time after it,
or exceeding the FIFO size.

Signed-off-by: Jonas Gorski <jogo-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
---
 drivers/spi/spi-bcm63xx.c |   91 +++++++++++++++++++++------------------------
 1 file changed, 42 insertions(+), 49 deletions(-)

diff --git a/drivers/spi/spi-bcm63xx.c b/drivers/spi/spi-bcm63xx.c
index f44ab55..27667c1 100644
--- a/drivers/spi/spi-bcm63xx.c
+++ b/drivers/spi/spi-bcm63xx.c
@@ -49,16 +49,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,24 +169,13 @@ 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)
+static int bcm63xx_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 {
 	struct bcm63xx_spi *bs = spi_master_get_devdata(spi->master);
 	u16 msg_ctl;
 	u16 cmd;
+	u8 rx_tail;
+	unsigned int timeout = 0;
 
 	/* Disable the CMD_DONE interrupt */
 	bcm_spi_writeb(bs, 0, SPI_INT_MASK);
@@ -200,14 +183,8 @@ static unsigned int bcm63xx_txrx_bufs(struct spi_device *spi,
 	dev_dbg(&spi->dev, "txrx: tx %p, rx %p, len %d\n",
 		t->tx_buf, t->rx_buf, t->len);
 
-	/* Transmitter is inhibited */
-	bs->tx_ptr = t->tx_buf;
-	bs->rx_ptr = t->rx_buf;
-
-	if (t->tx_buf) {
-		bs->remaining_bytes = t->len;
-		bcm63xx_spi_fill_tx_fifo(bs);
-	}
+	if (t->tx_buf)
+		memcpy_toio(bs->tx_io, t->tx_buf, t->len);
 
 	init_completion(&bs->done);
 
@@ -239,7 +216,18 @@ static unsigned int bcm63xx_txrx_bufs(struct spi_device *spi,
 	/* 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);
+
+	/* Read out all the data */
+	if (rx_tail)
+		memcpy_fromio(t->rx_ptr, bs->rx_io, rx_tail);
+
+	return 0;
 }
 
 static int bcm63xx_spi_prepare_transfer(struct spi_master *master)
@@ -267,36 +255,41 @@ static int bcm63xx_spi_transfer_one(struct spi_master *master,
 	struct spi_transfer *t;
 	struct spi_device *spi = m->spi;
 	int status = 0;
-	unsigned int timeout = 0;
 
 	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);
+		/* we can only transfer one fifo worth of data */
+		if (t->len > bs->fifo_size) {
+			dev_err(&spi->dev, "unable to do transfers larger than FIFO size (%i > %i)\n",
+				t->len, bs->fifo_size);
+			status = -EINVAL;
+			goto exit;
+		}
 
-		while (len) {
-			/* send the data */
-			len -= bcm63xx_txrx_bufs(spi, t);
+		/* CS will be deasserted directly after transfer */
+		if (t->delay_usecs) {
+			dev_err(&spi->dev, "unable to keep CS asserted after transfer\n");
+			status = -EINVAL;
+			goto exit;
+		}
 
-			timeout = wait_for_completion_timeout(&bs->done, HZ);
-			if (!timeout) {
-				status = -ETIMEDOUT;
-				goto exit;
-			}
+		if (!t->cs_change &&
+		    !list_is_last(&t->transfer_list, &m->transfers)) {
+			dev_err(&spi->dev, "unable to keep CS asserted between transfers\n");
+			status = -EINVAL;
+			goto exit;
+		}
 
-			/* read out all data */
-			rx_tail = bcm_spi_readb(bs, SPI_RX_TAIL);
+		/* configure adapter for a new transfer */
+		bcm63xx_spi_setup_transfer(spi, t);
 
-			/* Read out all the data */
-			if (rx_tail)
-				memcpy_fromio(bs->rx_ptr, bs->rx_io, rx_tail);
-		}
+		/* send the data */
+		status = bcm63xx_txrx_bufs(spi, t);
+		if (status)
+			goto exit;
 
 		m->actual_length += t->len;
 	}
-- 
1.7.10.4


------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_jan

Re: [PATCH V2 1/2] spi/bcm63xx: reject transfers unable to transfer

[Grant Likely]

On Sun,  3 Feb 2013 15:15:12 +0100, Jonas Gorski <jogo-p3rKhJxN3npAfugRpC6u6w@public.gmane.org> wrote:
> The hardware does not support keeping CS asserted after sending one
> FIFO buffer worth of data, so reject transfers requiring CS being kept
> asserted, either between transers or for a certain time after it,
> or exceeding the FIFO size.
> 
> Signed-off-by: Jonas Gorski <jogo-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>

Applied, thanks.

g.


------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013 
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb

[PATCH V2 2/2] spi/bcm63xx: work around inability to keep CS up

[Jonas Gorski]

This SPI controller does not support keeping CS asserted after sending
a transfer.
Since messages expected on this SPI controller are rather short, we can
work around it for normal use cases by sending all transfers at once in
a big full duplex stream.

This means that we cannot change the speed between transfers if they
require CS to be kept asserted, but these would have been rejected
before anyway because of the inability of keeping CS asserted.

Signed-off-by: Jonas Gorski <jogo-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
---
V1 -> V2:
 * split out rejection logic into separate patch
 * fixed return type of bcm63xx_txrx_bufs()
 * slightly reworked bcm63xx_txrx_bufs, obsoleting one local variable

 drivers/spi/spi-bcm63xx.c |  134 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 106 insertions(+), 28 deletions(-)

diff --git a/drivers/spi/spi-bcm63xx.c b/drivers/spi/spi-bcm63xx.c
index 27667c1..9578af7 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;
 
@@ -169,13 +171,17 @@ static int bcm63xx_spi_setup(struct spi_device *spi)
 	return 0;
 }
 
-static int bcm63xx_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
+static int bcm63xx_txrx_bufs(struct spi_device *spi, struct spi_transfer *first,
+				unsigned int num_transfers)
 {
 	struct bcm63xx_spi *bs = spi_master_get_devdata(spi->master);
 	u16 msg_ctl;
 	u16 cmd;
 	u8 rx_tail;
-	unsigned int timeout = 0;
+	unsigned int i, timeout = 0, prepend_len = 0, len = 0;
+	struct spi_transfer *t = first;
+	bool do_rx = false;
+	bool do_tx = false;
 
 	/* Disable the CMD_DONE interrupt */
 	bcm_spi_writeb(bs, 0, SPI_INT_MASK);
@@ -183,19 +189,45 @@ static int bcm63xx_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 	dev_dbg(&spi->dev, "txrx: tx %p, rx %p, len %d\n",
 		t->tx_buf, t->rx_buf, t->len);
 
-	if (t->tx_buf)
-		memcpy_toio(bs->tx_io, t->tx_buf, t->len);
+	if (num_transfers > 1 && t->tx_buf && t->len <= BCM63XX_SPI_MAX_PREPEND)
+		prepend_len = t->len;
+
+	/* prepare the buffer */
+	for (i = 0; i < num_transfers; i++) {
+		if (t->tx_buf) {
+			do_tx = true;
+			memcpy_toio(bs->tx_io + len, t->tx_buf, t->len);
+
+			/* don't prepend more than one tx */
+			if (t != first)
+				prepend_len = 0;
+		}
+
+		if (t->rx_buf) {
+			do_rx = true;
+			/* prepend is half-duplex write only */
+			if (t == first)
+				prepend_len = 0;
+		}
+
+		len += t->len;
+
+		t = list_entry(t->transfer_list.next, struct spi_transfer,
+			       transfer_list);
+	}
+
+	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) {
@@ -209,7 +241,7 @@ static int bcm63xx_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 
 	/* 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);
 
@@ -223,9 +255,25 @@ static int bcm63xx_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 	/* read out all data */
 	rx_tail = bcm_spi_readb(bs, SPI_RX_TAIL);
 
+	if (do_rx && rx_tail != len)
+		return -EIO;
+
+	if (!rx_tail)
+		return 0;
+
+	len = 0;
+	t = first;
 	/* Read out all the data */
-	if (rx_tail)
-		memcpy_fromio(t->rx_ptr, bs->rx_io, rx_tail);
+	for (i = 0; i < num_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 0;
 }
@@ -252,46 +300,76 @@ 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 n_transfers = 0, total_len = 0;
+	bool can_use_prepend = false;
+
+	/*
+	 * This SPI controller does not support keeping CS active after a
+	 * transfer.
+	 * Work around this by merging as many transfers we can into one big
+	 * full-duplex transfers.
+	 */
 	list_for_each_entry(t, &m->transfers, transfer_list) {
 		status = bcm63xx_spi_check_transfer(spi, t);
 		if (status < 0)
 			goto exit;
 
+		if (!first)
+			first = t;
+
+		n_transfers++;
+		total_len += t->len;
+
+		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;
+
 		/* we can only transfer one fifo worth of data */
-		if (t->len > bs->fifo_size) {
+		if ((can_use_prepend &&
+		     total_len > (bs->fifo_size + BCM63XX_SPI_MAX_PREPEND)) ||
+		    (!can_use_prepend && total_len > bs->fifo_size)) {
 			dev_err(&spi->dev, "unable to do transfers larger than FIFO size (%i > %i)\n",
-				t->len, bs->fifo_size);
+				total_len, bs->fifo_size);
 			status = -EINVAL;
 			goto exit;
 		}
 
-		/* CS will be deasserted directly after transfer */
-		if (t->delay_usecs) {
-			dev_err(&spi->dev, "unable to keep CS asserted after transfer\n");
+		/* all combined transfers have to have the same speed */
+		if (t->speed_hz != first->speed_hz) {
+			dev_err(&spi->dev, "unable to change speed between transfers\n");
 			status = -EINVAL;
 			goto exit;
 		}
 
-		if (!t->cs_change &&
-		    !list_is_last(&t->transfer_list, &m->transfers)) {
-			dev_err(&spi->dev, "unable to keep CS asserted between transfers\n");
+		/* CS will be deasserted directly after transfer */
+		if (t->delay_usecs) {
+			dev_err(&spi->dev, "unable to keep CS asserted after transfer\n");
 			status = -EINVAL;
 			goto exit;
 		}
 
-		/* configure adapter for a new transfer */
-		bcm63xx_spi_setup_transfer(spi, t);
+		if (t->cs_change ||
+		    list_is_last(&t->transfer_list, &m->transfers)) {
+			/* configure adapter for a new transfer */
+			bcm63xx_spi_setup_transfer(spi, first);
 
-		/* send the data */
-		status = bcm63xx_txrx_bufs(spi, t);
-		if (status)
-			goto exit;
+			/* send the data */
+			status = bcm63xx_txrx_bufs(spi, first, n_transfers);
+			if (status)
+				goto exit;
+
+			m->actual_length += total_len;
 
-		m->actual_length += t->len;
+			first = NULL;
+			n_transfers = 0;
+			total_len = 0;
+			can_use_prepend = false;
+		}
 	}
 exit:
 	m->status = status;
-- 
1.7.10.4


------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_jan

Re: [PATCH V2 2/2] spi/bcm63xx: work around inability to keep CS up

[Grant Likely]

On Sun,  3 Feb 2013 15:15:13 +0100, Jonas Gorski <jogo-p3rKhJxN3npAfugRpC6u6w@public.gmane.org> wrote:
> This SPI controller does not support keeping CS asserted after sending
> a transfer.
> Since messages expected on this SPI controller are rather short, we can
> work around it for normal use cases by sending all transfers at once in
> a big full duplex stream.
> 
> This means that we cannot change the speed between transfers if they
> require CS to be kept asserted, but these would have been rejected
> before anyway because of the inability of keeping CS asserted.
> 
> Signed-off-by: Jonas Gorski <jogo-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>

Are you checking the state of transfer->cs_change when merging
transfers? If cs_change is set, then the transfers cannot be merged.

g.

> ---
> V1 -> V2:
>  * split out rejection logic into separate patch
>  * fixed return type of bcm63xx_txrx_bufs()
>  * slightly reworked bcm63xx_txrx_bufs, obsoleting one local variable
> 
>  drivers/spi/spi-bcm63xx.c |  134 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 106 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/spi/spi-bcm63xx.c b/drivers/spi/spi-bcm63xx.c
> index 27667c1..9578af7 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;
>  
> @@ -169,13 +171,17 @@ static int bcm63xx_spi_setup(struct spi_device *spi)
>  	return 0;
>  }
>  
> -static int bcm63xx_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
> +static int bcm63xx_txrx_bufs(struct spi_device *spi, struct spi_transfer *first,
> +				unsigned int num_transfers)
>  {
>  	struct bcm63xx_spi *bs = spi_master_get_devdata(spi->master);
>  	u16 msg_ctl;
>  	u16 cmd;
>  	u8 rx_tail;
> -	unsigned int timeout = 0;
> +	unsigned int i, timeout = 0, prepend_len = 0, len = 0;
> +	struct spi_transfer *t = first;
> +	bool do_rx = false;
> +	bool do_tx = false;
>  
>  	/* Disable the CMD_DONE interrupt */
>  	bcm_spi_writeb(bs, 0, SPI_INT_MASK);
> @@ -183,19 +189,45 @@ static int bcm63xx_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
>  	dev_dbg(&spi->dev, "txrx: tx %p, rx %p, len %d\n",
>  		t->tx_buf, t->rx_buf, t->len);
>  
> -	if (t->tx_buf)
> -		memcpy_toio(bs->tx_io, t->tx_buf, t->len);
> +	if (num_transfers > 1 && t->tx_buf && t->len <= BCM63XX_SPI_MAX_PREPEND)
> +		prepend_len = t->len;
> +
> +	/* prepare the buffer */
> +	for (i = 0; i < num_transfers; i++) {
> +		if (t->tx_buf) {
> +			do_tx = true;
> +			memcpy_toio(bs->tx_io + len, t->tx_buf, t->len);
> +
> +			/* don't prepend more than one tx */
> +			if (t != first)
> +				prepend_len = 0;
> +		}
> +
> +		if (t->rx_buf) {
> +			do_rx = true;
> +			/* prepend is half-duplex write only */
> +			if (t == first)
> +				prepend_len = 0;
> +		}
> +
> +		len += t->len;
> +
> +		t = list_entry(t->transfer_list.next, struct spi_transfer,
> +			       transfer_list);
> +	}
> +
> +	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) {
> @@ -209,7 +241,7 @@ static int bcm63xx_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
>  
>  	/* 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);
>  
> @@ -223,9 +255,25 @@ static int bcm63xx_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
>  	/* read out all data */
>  	rx_tail = bcm_spi_readb(bs, SPI_RX_TAIL);
>  
> +	if (do_rx && rx_tail != len)
> +		return -EIO;
> +
> +	if (!rx_tail)
> +		return 0;
> +
> +	len = 0;
> +	t = first;
>  	/* Read out all the data */
> -	if (rx_tail)
> -		memcpy_fromio(t->rx_ptr, bs->rx_io, rx_tail);
> +	for (i = 0; i < num_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 0;
>  }
> @@ -252,46 +300,76 @@ 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 n_transfers = 0, total_len = 0;
> +	bool can_use_prepend = false;
> +
> +	/*
> +	 * This SPI controller does not support keeping CS active after a
> +	 * transfer.
> +	 * Work around this by merging as many transfers we can into one big
> +	 * full-duplex transfers.
> +	 */
>  	list_for_each_entry(t, &m->transfers, transfer_list) {
>  		status = bcm63xx_spi_check_transfer(spi, t);
>  		if (status < 0)
>  			goto exit;
>  
> +		if (!first)
> +			first = t;
> +
> +		n_transfers++;
> +		total_len += t->len;
> +
> +		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;
> +
>  		/* we can only transfer one fifo worth of data */
> -		if (t->len > bs->fifo_size) {
> +		if ((can_use_prepend &&
> +		     total_len > (bs->fifo_size + BCM63XX_SPI_MAX_PREPEND)) ||
> +		    (!can_use_prepend && total_len > bs->fifo_size)) {
>  			dev_err(&spi->dev, "unable to do transfers larger than FIFO size (%i > %i)\n",
> -				t->len, bs->fifo_size);
> +				total_len, bs->fifo_size);
>  			status = -EINVAL;
>  			goto exit;
>  		}
>  
> -		/* CS will be deasserted directly after transfer */
> -		if (t->delay_usecs) {
> -			dev_err(&spi->dev, "unable to keep CS asserted after transfer\n");
> +		/* all combined transfers have to have the same speed */
> +		if (t->speed_hz != first->speed_hz) {
> +			dev_err(&spi->dev, "unable to change speed between transfers\n");
>  			status = -EINVAL;
>  			goto exit;
>  		}
>  
> -		if (!t->cs_change &&
> -		    !list_is_last(&t->transfer_list, &m->transfers)) {
> -			dev_err(&spi->dev, "unable to keep CS asserted between transfers\n");
> +		/* CS will be deasserted directly after transfer */
> +		if (t->delay_usecs) {
> +			dev_err(&spi->dev, "unable to keep CS asserted after transfer\n");
>  			status = -EINVAL;
>  			goto exit;
>  		}
>  
> -		/* configure adapter for a new transfer */
> -		bcm63xx_spi_setup_transfer(spi, t);
> +		if (t->cs_change ||
> +		    list_is_last(&t->transfer_list, &m->transfers)) {
> +			/* configure adapter for a new transfer */
> +			bcm63xx_spi_setup_transfer(spi, first);
>  
> -		/* send the data */
> -		status = bcm63xx_txrx_bufs(spi, t);
> -		if (status)
> -			goto exit;
> +			/* send the data */
> +			status = bcm63xx_txrx_bufs(spi, first, n_transfers);
> +			if (status)
> +				goto exit;
> +
> +			m->actual_length += total_len;
>  
> -		m->actual_length += t->len;
> +			first = NULL;
> +			n_transfers = 0;
> +			total_len = 0;
> +			can_use_prepend = false;
> +		}
>  	}
>  exit:
>  	m->status = status;
> -- 
> 1.7.10.4
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013 
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb

Re: [PATCH V2 2/2] spi/bcm63xx: work around inability to keep CS up

[Jonas Gorski]

On Tue, 05 Feb 2013 14:35:30 +0000
Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:

> On Sun,  3 Feb 2013 15:15:13 +0100, Jonas Gorski <jogo-p3rKhJxN3npAfugRpC6u6w@public.gmane.org> wrote:
> > This SPI controller does not support keeping CS asserted after sending
> > a transfer.
> > Since messages expected on this SPI controller are rather short, we can
> > work around it for normal use cases by sending all transfers at once in
> > a big full duplex stream.
> > 
> > This means that we cannot change the speed between transfers if they
> > require CS to be kept asserted, but these would have been rejected
> > before anyway because of the inability of keeping CS asserted.
> > 
> > Signed-off-by: Jonas Gorski <jogo-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
> 
> Are you checking the state of transfer->cs_change when merging
> transfers? If cs_change is set, then the transfers cannot be merged.

Yes, I do; I "flush" on each cs_change and after the last transfer:

> > +		if (t->cs_change ||
> > +		    list_is_last(&t->transfer_list, &m->transfers)) {
> > +			/* configure adapter for a new transfer */
> > +			bcm63xx_spi_setup_transfer(spi, first);
> >  
> > -		/* send the data */
> > -		status = bcm63xx_txrx_bufs(spi, t);
> > -		if (status)
> > -			goto exit;
> > +			/* send the data */
> > +			status = bcm63xx_txrx_bufs(spi, first, n_transfers);
> > +			if (status)
> > +				goto exit;
> > +
> > +			m->actual_length += total_len;
> >  
> > -		m->actual_length += t->len;
> > +			first = NULL;
> > +			n_transfers = 0;
> > +			total_len = 0;
> > +			can_use_prepend = false;
> > +		}
> >  	}
> >  exit:
> >  	m->status = status;
> > -- 
> > 1.7.10.4
> > 
> 
> -- 
> Grant Likely, B.Sc, P.Eng.
> Secret Lab Technologies, Ltd.

P.S: Sorry Grant for receiving this double - I can't email.

-- 
Jonas Gorski <jogo-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>

------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013 
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb

Re: [PATCH V2 2/2] spi/bcm63xx: work around inability to keep CS up

[Grant Likely]

On Tue, 5 Feb 2013 16:00:04 +0100, Jonas Gorski <jogo-p3rKhJxN3npAfugRpC6u6w@public.gmane.org> wrote:
> On Tue, 05 Feb 2013 14:35:30 +0000
> Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> 
> > On Sun,  3 Feb 2013 15:15:13 +0100, Jonas Gorski <jogo-p3rKhJxN3npAfugRpC6u6w@public.gmane.org> wrote:
> > > This SPI controller does not support keeping CS asserted after sending
> > > a transfer.
> > > Since messages expected on this SPI controller are rather short, we can
> > > work around it for normal use cases by sending all transfers at once in
> > > a big full duplex stream.
> > > 
> > > This means that we cannot change the speed between transfers if they
> > > require CS to be kept asserted, but these would have been rejected
> > > before anyway because of the inability of keeping CS asserted.
> > > 
> > > Signed-off-by: Jonas Gorski <jogo-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
> > 
> > Are you checking the state of transfer->cs_change when merging
> > transfers? If cs_change is set, then the transfers cannot be merged.
> 
> Yes, I do; I "flush" on each cs_change and after the last transfer:

Okay, applied. Thanks.

g.


------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013 
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb

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

[Florian Fainelli]

On 02/03/2013 03:15 PM, Jonas Gorski wrote:
> The bcm63xx SPI controller does not support keeping CS up after doing a
> transfer. Since this is problematic for most typical use cases, this
> patchset introduces a workaround by combining small enough messages
> to one transfer, rejecting anything that can't be fulfilled with the
> hardware.
>
> Patch one properly rejects anything impossible to transfer with these
> limitations.
> Patch two introduces logic for combining transfers to one to be able to
> use it for typical use cases (register accesses and flash access).
>
> Build and run tested on a BCM6368 with a SPI controlled switch attached
> requiring write-then-read with CS asserted.
>
> Changes V1 -> V2:
>   * split into two patches
>   * fixed return type of bcm63xx_txrx_bufs()
>   * slightly reworked bcm63xx_txrx_bufs, obsoleting one local variable
>   * added a bit more comments in the code
>   * added error messages indicated why transfers were rejected

Looks good, thanks!

Acked-by: Florian Fainelli <florian-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
--
Florian

------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_jan

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

[Grant Likely]

On Sun,  3 Feb 2013 15:15:11 +0100, Jonas Gorski <jogo-p3rKhJxN3npAfugRpC6u6w@public.gmane.org> wrote:
> The bcm63xx SPI controller does not support keeping CS up after doing a
> transfer. Since this is problematic for most typical use cases, this
> patchset introduces a workaround by combining small enough messages
> to one transfer, rejecting anything that can't be fulfilled with the
> hardware.
> 
> Patch one properly rejects anything impossible to transfer with these
> limitations.
> Patch two introduces logic for combining transfers to one to be able to
> use it for typical use cases (register accesses and flash access).
> 
> Build and run tested on a BCM6368 with a SPI controlled switch attached
> requiring write-then-read with CS asserted.
> 
> Changes V1 -> V2:
>  * split into two patches
>  * fixed return type of bcm63xx_txrx_bufs()
>  * slightly reworked bcm63xx_txrx_bufs, obsoleting one local variable
>  * added a bit more comments in the code
>  * added error messages indicated why transfers were rejected

Another option is to allow longer transfers if a GPIO is used for the CS
line. A lot of SPI controllers need to do that.

That isn't a critique of this patch because it is essentially a bug fix,
but rather a way you can work around the limitation.

g.

> 
> Jonas Gorski (2):
>   spi/bcm63xx: reject transfers unable to transfer
>   spi/bcm63xx: work around inability to keep CS up
> 
>  drivers/spi/spi-bcm63xx.c |  179 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 125 insertions(+), 54 deletions(-)
> 
> -- 
> 1.7.10.4
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013 
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb

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

[Grant Likely]

On Tue, 5 Feb 2013 15:04:41 +0000, Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Tue, Feb 05, 2013 at 02:30:40PM +0000, Grant Likely wrote:
> 
> > Another option is to allow longer transfers if a GPIO is used for the CS
> > line. A lot of SPI controllers need to do that.
> 
> > That isn't a critique of this patch because it is essentially a bug fix,
> > but rather a way you can work around the limitation.
> 
> Yeah, I did suggest that to them originally but apparently the hardware
> is sufficient fail that the pins can't be put into GPIO mode for the
> affected boards.

Hahaha.

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013 
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb