Patchwork [U-Boot,v2] EXYNOS: SPI: Support SPI_PREAMBLE mode

login
register
mail settings
Submitter Simon Glass
Date May 11, 2013, 3:04 p.m.
Message ID <1368284655-10201-1-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/243138/
State Superseded
Delegated to: Jagannadha Sutradharudu Teki
Headers show

Comments

Simon Glass - May 11, 2013, 3:04 p.m.
From: Rajeshwari Shinde <rajeshwari.s@samsung.com>

Support interfaces with a preamble before each received message.

We handle this when the client has requested a SPI_XFER_END, meaning
that we must close of the transaction. In this case we read until we
see the preamble (or a timeout occurs), skipping all data before and
including the preamble. The client will receive only data bytes after
the preamble.

Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Rajeshwari Shinde <rajeshwari.s@samsung.com>
---
Changes in v2:
- Remove preamable_count variable which is not really needed
- Fix checkpatch warning (multiple assignments)

 drivers/spi/exynos_spi.c | 62 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 8 deletions(-)
Simon Glass - May 11, 2013, 3:05 p.m.
On Sat, May 11, 2013 at 9:04 AM, Simon Glass <sjg@chromium.org> wrote:
> From: Rajeshwari Shinde <rajeshwari.s@samsung.com>
>
> Support interfaces with a preamble before each received message.
>
> We handle this when the client has requested a SPI_XFER_END, meaning
> that we must close of the transaction. In this case we read until we
> see the preamble (or a timeout occurs), skipping all data before and
> including the preamble. The client will receive only data bytes after
> the preamble.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Rajeshwari Shinde <rajeshwari.s@samsung.com>
> ---
> Changes in v2:
> - Remove preamable_count variable which is not really needed
> - Fix checkpatch warning (multiple assignments)

This v2 patch fixes the nits reported.

Acked-by: Simon Glass <sjg@chromium.org>
Minkyu Kang - May 21, 2013, 12:30 p.m.
Dear Rajeshwari Shinde,

On 12/05/13 00:04, Simon Glass wrote:
> From: Rajeshwari Shinde <rajeshwari.s@samsung.com>
> 
> Support interfaces with a preamble before each received message.
> 
> We handle this when the client has requested a SPI_XFER_END, meaning
> that we must close of the transaction. In this case we read until we
> see the preamble (or a timeout occurs), skipping all data before and
> including the preamble. The client will receive only data bytes after
> the preamble.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Rajeshwari Shinde <rajeshwari.s@samsung.com>
> ---
> Changes in v2:
> - Remove preamable_count variable which is not really needed
> - Fix checkpatch warning (multiple assignments)
> 
>  drivers/spi/exynos_spi.c | 62 +++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 54 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/exynos_spi.c b/drivers/spi/exynos_spi.c
> index 607e1cd..b7b2ea5 100644
> --- a/drivers/spi/exynos_spi.c
> +++ b/drivers/spi/exynos_spi.c
> @@ -51,6 +51,7 @@ struct exynos_spi_slave {
>  	unsigned int mode;
>  	enum periph_id periph_id;	/* Peripheral ID for this device */
>  	unsigned int fifo_size;
> +	int skip_preamble;
>  };
>  
>  static struct spi_bus *spi_get_bus(unsigned dev_index)
> @@ -105,6 +106,8 @@ struct spi_slave *spi_setup_slave(unsigned int busnum, unsigned int cs,
>  	else
>  		spi_slave->fifo_size = 256;
>  
> +	spi_slave->skip_preamble = 0;
> +
>  	spi_slave->freq = bus->frequency;
>  	if (max_hz)
>  		spi_slave->freq = min(max_hz, spi_slave->freq);
> @@ -217,17 +220,23 @@ static void spi_request_bytes(struct exynos_spi *regs, int count)
>  	writel(count | SPI_PACKET_CNT_EN, &regs->pkt_cnt);
>  }
>  
> -static void spi_rx_tx(struct exynos_spi_slave *spi_slave, int todo,
> -			void **dinp, void const **doutp)
> +static int spi_rx_tx(struct exynos_spi_slave *spi_slave, int todo,
> +			void **dinp, void const **doutp, unsigned long flags)
>  {
>  	struct exynos_spi *regs = spi_slave->regs;
>  	uchar *rxp = *dinp;
>  	const uchar *txp = *doutp;
>  	int rx_lvl, tx_lvl;
>  	uint out_bytes, in_bytes;
> +	int toread;
> +	unsigned start = get_timer(0);
> +	int stopping;
>  
>  	out_bytes = in_bytes = todo;
>  
> +	stopping = spi_slave->skip_preamble && (flags & SPI_XFER_END) &&
> +					!(spi_slave->mode & SPI_SLAVE);
> +
>  	/*
>  	 * If there's something to send, do a software reset and set a
>  	 * transaction size.
> @@ -238,6 +247,7 @@ static void spi_rx_tx(struct exynos_spi_slave *spi_slave, int todo,
>  	 * Bytes are transmitted/received in pairs. Wait to receive all the
>  	 * data because then transmission will be done as well.
>  	 */
> +	toread = in_bytes;

please add blank line here.

>  	while (in_bytes) {
>  		int temp;
>  
> @@ -250,13 +260,41 @@ static void spi_rx_tx(struct exynos_spi_slave *spi_slave, int todo,
>  		}
>  		if (rx_lvl > 0 && in_bytes) {

I think, in_bytes is always true.
It's a redundant checking.

>  			temp = readl(&regs->rx_data);
> -			if (rxp)
> +			if (!rxp && !stopping) {
> +				in_bytes--;
> +			} else if (spi_slave->skip_preamble) {
> +				if (temp == SPI_PREAMBLE_END_BYTE) {
> +					spi_slave->skip_preamble = 0;
> +					stopping = 0;
> +				}
> +			} else {
>  				*rxp++ = temp;
> -			in_bytes--;
> +				in_bytes--;
> +			}
> +			toread--;
> +		}


I think, this if statement is not logical.
The checking condition is differ, so it looks confused.

How about this?

+                       temp = readl(&regs->rx_data);
+                       if (spi_slave->skip_preamble) {
+                               if (temp == SPI_PREAMBLE_END_BYTE) {
+                                       spi_slave->skip_preamble = 0;
+                                       stopping = 0;
+                               }
+                       } else {
+                               if (rxp || stopping)
+                                       *rxp++ = temp;
+                               in_bytes--;
+                       }
+
+                       toread--;


> +		/*
> +		 * We have run out of input data, but haven't read enough
> +		 * bytes after the preamble yet. Read some more, and make
> +		 * sure that we transmit dummy bytes too, to keep things
> +		 * going.
> +		 */

This comment is what for?
please move this comment into the brace.

> +		else if (in_bytes && !toread) {

in_bytes.. ditto.

And please fix brace.
} else if (

> +			assert(!out_bytes);
> +			out_bytes = in_bytes;
> +			toread = in_bytes;
> +			txp = NULL;
> +			spi_request_bytes(regs, toread);
> +		}
> +		if (spi_slave->skip_preamble && get_timer(start) > 100) {
> +			printf("SPI timeout: in_bytes=%d, out_bytes=%d, ",
> +			       in_bytes, out_bytes);
> +			return -1;
>  		}
>  	}
>  	*dinp = rxp;
>  	*doutp = txp;

please add blank line here.

> +	return 0;
>  }
>  
>  /**
> @@ -276,6 +314,7 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
>  	struct exynos_spi_slave *spi_slave = to_exynos_spi(slave);
>  	int upto, todo;
>  	int bytelen;
> +	int ret = 0;
>  
>  	/* spi core configured to do 8 bit transfers */
>  	if (bitlen % 8) {
> @@ -289,16 +328,22 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
>  
>  	/* Exynos SPI limits each transfer to 65535 bytes */
>  	bytelen =  bitlen / 8;
> -	for (upto = 0; upto < bytelen; upto += todo) {
> +	for (upto = 0; !ret && upto < bytelen; upto += todo) {
>  		todo = min(bytelen - upto, (1 << 16) - 1);
> -		spi_rx_tx(spi_slave, todo, &din, &dout);
> +		ret = spi_rx_tx(spi_slave, todo, &din, &dout, flags);

It looks good.
But I think, it's better to checking the error explicitly.

if (!ret)
	break;

>  	}
>  
>  	/* Stop the transaction, if necessary. */
> -	if ((flags & SPI_XFER_END))
> +	if ((flags & SPI_XFER_END) && !(spi_slave->mode & SPI_SLAVE)) {
>  		spi_cs_deactivate(slave);
> +		if (spi_slave->skip_preamble) {
> +			assert(!spi_slave->skip_preamble);
> +			debug("Failed to complete premable transaction\n");
> +			ret = -1;
> +		}
> +	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /**
> @@ -325,6 +370,7 @@ void spi_cs_activate(struct spi_slave *slave)
>  
>  	clrbits_le32(&spi_slave->regs->cs_reg, SPI_SLAVE_SIG_INACT);
>  	debug("Activate CS, bus %d\n", spi_slave->slave.bus);
> +	spi_slave->skip_preamble = spi_slave->mode & SPI_PREAMBLE;
>  }
>  
>  /**
> 

Thanks,
Minkyu Kang.

Patch

diff --git a/drivers/spi/exynos_spi.c b/drivers/spi/exynos_spi.c
index 607e1cd..b7b2ea5 100644
--- a/drivers/spi/exynos_spi.c
+++ b/drivers/spi/exynos_spi.c
@@ -51,6 +51,7 @@  struct exynos_spi_slave {
 	unsigned int mode;
 	enum periph_id periph_id;	/* Peripheral ID for this device */
 	unsigned int fifo_size;
+	int skip_preamble;
 };
 
 static struct spi_bus *spi_get_bus(unsigned dev_index)
@@ -105,6 +106,8 @@  struct spi_slave *spi_setup_slave(unsigned int busnum, unsigned int cs,
 	else
 		spi_slave->fifo_size = 256;
 
+	spi_slave->skip_preamble = 0;
+
 	spi_slave->freq = bus->frequency;
 	if (max_hz)
 		spi_slave->freq = min(max_hz, spi_slave->freq);
@@ -217,17 +220,23 @@  static void spi_request_bytes(struct exynos_spi *regs, int count)
 	writel(count | SPI_PACKET_CNT_EN, &regs->pkt_cnt);
 }
 
-static void spi_rx_tx(struct exynos_spi_slave *spi_slave, int todo,
-			void **dinp, void const **doutp)
+static int spi_rx_tx(struct exynos_spi_slave *spi_slave, int todo,
+			void **dinp, void const **doutp, unsigned long flags)
 {
 	struct exynos_spi *regs = spi_slave->regs;
 	uchar *rxp = *dinp;
 	const uchar *txp = *doutp;
 	int rx_lvl, tx_lvl;
 	uint out_bytes, in_bytes;
+	int toread;
+	unsigned start = get_timer(0);
+	int stopping;
 
 	out_bytes = in_bytes = todo;
 
+	stopping = spi_slave->skip_preamble && (flags & SPI_XFER_END) &&
+					!(spi_slave->mode & SPI_SLAVE);
+
 	/*
 	 * If there's something to send, do a software reset and set a
 	 * transaction size.
@@ -238,6 +247,7 @@  static void spi_rx_tx(struct exynos_spi_slave *spi_slave, int todo,
 	 * Bytes are transmitted/received in pairs. Wait to receive all the
 	 * data because then transmission will be done as well.
 	 */
+	toread = in_bytes;
 	while (in_bytes) {
 		int temp;
 
@@ -250,13 +260,41 @@  static void spi_rx_tx(struct exynos_spi_slave *spi_slave, int todo,
 		}
 		if (rx_lvl > 0 && in_bytes) {
 			temp = readl(&regs->rx_data);
-			if (rxp)
+			if (!rxp && !stopping) {
+				in_bytes--;
+			} else if (spi_slave->skip_preamble) {
+				if (temp == SPI_PREAMBLE_END_BYTE) {
+					spi_slave->skip_preamble = 0;
+					stopping = 0;
+				}
+			} else {
 				*rxp++ = temp;
-			in_bytes--;
+				in_bytes--;
+			}
+			toread--;
+		}
+		/*
+		 * We have run out of input data, but haven't read enough
+		 * bytes after the preamble yet. Read some more, and make
+		 * sure that we transmit dummy bytes too, to keep things
+		 * going.
+		 */
+		else if (in_bytes && !toread) {
+			assert(!out_bytes);
+			out_bytes = in_bytes;
+			toread = in_bytes;
+			txp = NULL;
+			spi_request_bytes(regs, toread);
+		}
+		if (spi_slave->skip_preamble && get_timer(start) > 100) {
+			printf("SPI timeout: in_bytes=%d, out_bytes=%d, ",
+			       in_bytes, out_bytes);
+			return -1;
 		}
 	}
 	*dinp = rxp;
 	*doutp = txp;
+	return 0;
 }
 
 /**
@@ -276,6 +314,7 @@  int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 	struct exynos_spi_slave *spi_slave = to_exynos_spi(slave);
 	int upto, todo;
 	int bytelen;
+	int ret = 0;
 
 	/* spi core configured to do 8 bit transfers */
 	if (bitlen % 8) {
@@ -289,16 +328,22 @@  int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 
 	/* Exynos SPI limits each transfer to 65535 bytes */
 	bytelen =  bitlen / 8;
-	for (upto = 0; upto < bytelen; upto += todo) {
+	for (upto = 0; !ret && upto < bytelen; upto += todo) {
 		todo = min(bytelen - upto, (1 << 16) - 1);
-		spi_rx_tx(spi_slave, todo, &din, &dout);
+		ret = spi_rx_tx(spi_slave, todo, &din, &dout, flags);
 	}
 
 	/* Stop the transaction, if necessary. */
-	if ((flags & SPI_XFER_END))
+	if ((flags & SPI_XFER_END) && !(spi_slave->mode & SPI_SLAVE)) {
 		spi_cs_deactivate(slave);
+		if (spi_slave->skip_preamble) {
+			assert(!spi_slave->skip_preamble);
+			debug("Failed to complete premable transaction\n");
+			ret = -1;
+		}
+	}
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -325,6 +370,7 @@  void spi_cs_activate(struct spi_slave *slave)
 
 	clrbits_le32(&spi_slave->regs->cs_reg, SPI_SLAVE_SIG_INACT);
 	debug("Activate CS, bus %d\n", spi_slave->slave.bus);
+	spi_slave->skip_preamble = spi_slave->mode & SPI_PREAMBLE;
 }
 
 /**