Hi Vadim,

On Thu, May 2, 2013 at 7:28 PM, Vadim Bendebury vbendeb@chromium.org wrote:

On Thu, Mar 21, 2013 at 11:29 PM, Rajeshwari Shinde rajeshwari.s@samsung.com wrote:

...

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

---

drivers/spi/exynos_spi.c | 62 ++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 54 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/exynos_spi.c b/drivers/spi/exynos_spi.c index be60ada..09e88d5 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) @@ -107,6 +108,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);
  

@@ -219,17 +222,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, preamable_count = 0;
  

preamable_count: the name is misspelled, and the variable is never modified.

...

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

@@ -240,6 +249,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;
  
  

@@ -252,13 +262,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);
  

•                   toread = out_bytes = 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);
  

•                   printf("count = %d\n", preamable_count);
  

•                   return -1;
            }
    }
    *dinp = rxp;
    *doutp = txp;
  

•   return 0;
  

}

/** @@ -278,6 +316,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) {
  

@@ -291,16 +330,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;
  

}

/** @@ -327,6 +372,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;
  

who sets this bit in the 'mode' field?

This is set by whoever creates the slave - in the case of snow it is cros_ec.

Regards, Simon

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; 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; }

/**

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.