[U-Boot] [PATCH v2] EXYNOS: SPI: Support SPI_PREAMBLE mode
Minkyu Kang
mk7.kang at samsung.com
Tue May 21 14:30:51 CEST 2013
Dear Rajeshwari Shinde,
On 12/05/13 00:04, Simon Glass wrote:
> From: Rajeshwari Shinde <rajeshwari.s at 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 at chromium.org>
> Signed-off-by: Rajeshwari Shinde <rajeshwari.s at 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, ®s->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(®s->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(®s->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.
More information about the U-Boot
mailing list