[PATCH V2 01/12] spi/mxs: Always set LOCK_CS

[PATCH V2 01/12] spi/mxs: Always set LOCK_CS

[Trent Piepho]

There are two bits which control the CS line in the CTRL0 register:
LOCK_CS and IGNORE_CRC.  The latter would be better named DEASSERT_CS
in SPI mode.

LOCK_CS keeps CS asserted though the entire transfer.  This should
always be set.  The DMA code will always set it, explicitly on the
first segment of the first transfer, and then implicitly on all the
rest by never clearing the bit from the value read from the ctrl0
register.

The only reason to not set LOCK_CS would be to attempt an altered
protocol where CS pulses between each word.  Though don't get your
hopes up if you want to do this, as the hardware doesn't appear to do
this in any sane manner.

The code can be simplified by just setting LOCK_CS once and then not
needing to deal with it in the PIO and DMA transfer functions.

Signed-off-by: Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-mxs.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 22a0af0..9334167 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -91,6 +91,8 @@ static int mxs_spi_setup_transfer(struct spi_device *dev,
 
 	mxs_ssp_set_clk_rate(ssp, hz);
 
+	writel(BM_SSP_CTRL0_LOCK_CS,
+		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 	writel(BF_SSP_CTRL1_SSP_MODE(BV_SSP_CTRL1_SSP_MODE__SPI) |
 		     BF_SSP_CTRL1_WORD_LENGTH
 		     (BV_SSP_CTRL1_WORD_LENGTH__EIGHT_BITS) |
@@ -159,8 +161,6 @@ static inline void mxs_spi_enable(struct mxs_spi *spi)
 {
 	struct mxs_ssp *ssp = &spi->ssp;
 
-	writel(BM_SSP_CTRL0_LOCK_CS,
-		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 	writel(BM_SSP_CTRL0_IGNORE_CRC,
 		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
 }
@@ -169,8 +169,6 @@ static inline void mxs_spi_disable(struct mxs_spi *spi)
 {
 	struct mxs_ssp *ssp = &spi->ssp;
 
-	writel(BM_SSP_CTRL0_LOCK_CS,
-		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
 	writel(BM_SSP_CTRL0_IGNORE_CRC,
 		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 }
@@ -244,8 +242,6 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
 	ctrl0 &= ~BM_SSP_CTRL0_XFER_COUNT;
 	ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
 
-	if (*first)
-		ctrl0 |= BM_SSP_CTRL0_LOCK_CS;
 	if (!write)
 		ctrl0 |= BM_SSP_CTRL0_READ;
 
-- 
1.7.10.4


------------------------------------------------------------------------------
Own the Future-Intel(R) Level Up Game Demo Contest 2013
Rise to greatness in Intel's independent game demo contest. Compete 
for recognition, cash, and the chance to get your game on Steam. 
$5K grand prize plus 10 genre and skill prizes. Submit your demo 
by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2

[PATCH V2 01/12] spi/mxs: Always set LOCK_CS

[Trent Piepho]

There are two bits which control the CS line in the CTRL0 register:
LOCK_CS and IGNORE_CRC.  The latter would be better named DEASSERT_CS
in SPI mode.

LOCK_CS keeps CS asserted though the entire transfer.  This should
always be set.  The DMA code will always set it, explicitly on the
first segment of the first transfer, and then implicitly on all the
rest by never clearing the bit from the value read from the ctrl0
register.

The only reason to not set LOCK_CS would be to attempt an altered
protocol where CS pulses between each word.  Though don't get your
hopes up if you want to do this, as the hardware doesn't appear to do
this in any sane manner.

The code can be simplified by just setting LOCK_CS once and then not
needing to deal with it in the PIO and DMA transfer functions.

Signed-off-by: Trent Piepho <tpiepho@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/spi/spi-mxs.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 22a0af0..9334167 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -91,6 +91,8 @@ static int mxs_spi_setup_transfer(struct spi_device *dev,
 
 	mxs_ssp_set_clk_rate(ssp, hz);
 
+	writel(BM_SSP_CTRL0_LOCK_CS,
+		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 	writel(BF_SSP_CTRL1_SSP_MODE(BV_SSP_CTRL1_SSP_MODE__SPI) |
 		     BF_SSP_CTRL1_WORD_LENGTH
 		     (BV_SSP_CTRL1_WORD_LENGTH__EIGHT_BITS) |
@@ -159,8 +161,6 @@ static inline void mxs_spi_enable(struct mxs_spi *spi)
 {
 	struct mxs_ssp *ssp = &spi->ssp;
 
-	writel(BM_SSP_CTRL0_LOCK_CS,
-		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 	writel(BM_SSP_CTRL0_IGNORE_CRC,
 		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
 }
@@ -169,8 +169,6 @@ static inline void mxs_spi_disable(struct mxs_spi *spi)
 {
 	struct mxs_ssp *ssp = &spi->ssp;
 
-	writel(BM_SSP_CTRL0_LOCK_CS,
-		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
 	writel(BM_SSP_CTRL0_IGNORE_CRC,
 		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 }
@@ -244,8 +242,6 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
 	ctrl0 &= ~BM_SSP_CTRL0_XFER_COUNT;
 	ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
 
-	if (*first)
-		ctrl0 |= BM_SSP_CTRL0_LOCK_CS;
 	if (!write)
 		ctrl0 |= BM_SSP_CTRL0_READ;
 
-- 
1.7.10.4

[PATCH V2 02/12] spi/mxs: Remove mxs_spi_enable and mxs_spi_disable

[Trent Piepho]

These functions do nothing but one single writel call and are only
called in once.  And the names really aren't accurate or clear, since
they don't enable or disble SPI.  Rather they set the bit that
controls the state of CS at the end of transfer.  It easier to follow
the code to just set this bit with a writel() along with all the other
bits.

Signed-off-by: Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-mxs.c |   24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 9334167..7eb4bc9 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -157,22 +157,6 @@ static void mxs_spi_set_cs(struct mxs_spi *spi, unsigned cs)
 	writel(select, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 }
 
-static inline void mxs_spi_enable(struct mxs_spi *spi)
-{
-	struct mxs_ssp *ssp = &spi->ssp;
-
-	writel(BM_SSP_CTRL0_IGNORE_CRC,
-		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
-}
-
-static inline void mxs_spi_disable(struct mxs_spi *spi)
-{
-	struct mxs_ssp *ssp = &spi->ssp;
-
-	writel(BM_SSP_CTRL0_IGNORE_CRC,
-		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
-}
-
 static int mxs_ssp_wait(struct mxs_spi *spi, int offset, int mask, bool set)
 {
 	const unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT);
@@ -346,14 +330,16 @@ static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
 {
 	struct mxs_ssp *ssp = &spi->ssp;
 
-	if (*first)
-		mxs_spi_enable(spi);
+	/* Clear this now, set it on last transfer if needed */
+	writel(BM_SSP_CTRL0_IGNORE_CRC,
+	       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
 
 	mxs_spi_set_cs(spi, cs);
 
 	while (len--) {
 		if (*last && len == 0)
-			mxs_spi_disable(spi);
+			writel(BM_SSP_CTRL0_IGNORE_CRC,
+			       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 
 		if (ssp->devid == IMX23_SSP) {
 			writel(BM_SSP_CTRL0_XFER_COUNT,
-- 
1.7.10.4


------------------------------------------------------------------------------
Own the Future-Intel(R) Level Up Game Demo Contest 2013
Rise to greatness in Intel's independent game demo contest. Compete 
for recognition, cash, and the chance to get your game on Steam. 
$5K grand prize plus 10 genre and skill prizes. Submit your demo 
by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2

[PATCH V2 02/12] spi/mxs: Remove mxs_spi_enable and mxs_spi_disable

[Trent Piepho]

These functions do nothing but one single writel call and are only
called in once.  And the names really aren't accurate or clear, since
they don't enable or disble SPI.  Rather they set the bit that
controls the state of CS at the end of transfer.  It easier to follow
the code to just set this bit with a writel() along with all the other
bits.

Signed-off-by: Trent Piepho <tpiepho@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/spi/spi-mxs.c |   24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 9334167..7eb4bc9 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -157,22 +157,6 @@ static void mxs_spi_set_cs(struct mxs_spi *spi, unsigned cs)
 	writel(select, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 }
 
-static inline void mxs_spi_enable(struct mxs_spi *spi)
-{
-	struct mxs_ssp *ssp = &spi->ssp;
-
-	writel(BM_SSP_CTRL0_IGNORE_CRC,
-		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
-}
-
-static inline void mxs_spi_disable(struct mxs_spi *spi)
-{
-	struct mxs_ssp *ssp = &spi->ssp;
-
-	writel(BM_SSP_CTRL0_IGNORE_CRC,
-		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
-}
-
 static int mxs_ssp_wait(struct mxs_spi *spi, int offset, int mask, bool set)
 {
 	const unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT);
@@ -346,14 +330,16 @@ static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
 {
 	struct mxs_ssp *ssp = &spi->ssp;
 
-	if (*first)
-		mxs_spi_enable(spi);
+	/* Clear this now, set it on last transfer if needed */
+	writel(BM_SSP_CTRL0_IGNORE_CRC,
+	       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
 
 	mxs_spi_set_cs(spi, cs);
 
 	while (len--) {
 		if (*last && len == 0)
-			mxs_spi_disable(spi);
+			writel(BM_SSP_CTRL0_IGNORE_CRC,
+			       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 
 		if (ssp->devid == IMX23_SSP) {
 			writel(BM_SSP_CTRL0_XFER_COUNT,
-- 
1.7.10.4

[PATCH V2 03/12] spi/mxs: Change flag arguments in txrx functions to bit flags

[Trent Piepho]

There are three flag arguments to the PIO and DMA txrx functions.  Two
are passed as pointers to integers, even though they are input only
and not modified, which makes no sense to do.  The third is passed as
an integer.

The compiler must use an argument register or stack variable for each
flag this way.  By using bitflags in a single flag argument more
efficient and smaller code is produced since all the flags can fit in
a single register.  And all the flag arguments get cumbersome,
especially when more are added for things like GPIO chipselects.

The "first" flag is never used, so can just be deleted.

The "last" flag is renamed to DEASSERT_CS, since that's really what it
does.  The spi_transfer cs_change flag means that CS might be
de-asserted on a transfer which is not last and not de-assert on the
last transfer, so it is not which transfer is the last we need to know
but rather the transfers which after which CS should be de-asserted.

This also extends the driver to not ignore cs_change when setting the
DEASSERT_CS nee "last" flag.

Signed-off-by: Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-mxs.c |   55 ++++++++++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 7eb4bc9..3064304 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -58,6 +58,13 @@
 
 #define SG_MAXLEN		0xff00
 
+/*
+ * Flags for txrx functions.  More efficient that using an argument register for
+ * each one.
+ */
+#define TXRX_WRITE		1	/* This is a write */
+#define TXRX_DEASSERT_CS	2	/* De-assert CS at end of txrx */
+
 struct mxs_spi {
 	struct mxs_ssp		ssp;
 	struct completion	c;
@@ -196,7 +203,7 @@ static irqreturn_t mxs_ssp_irq_handler(int irq, void *dev_id)
 
 static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
 			    unsigned char *buf, int len,
-			    int *first, int *last, int write)
+			    unsigned int flags)
 {
 	struct mxs_ssp *ssp = &spi->ssp;
 	struct dma_async_tx_descriptor *desc = NULL;
@@ -226,15 +233,19 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
 	ctrl0 &= ~BM_SSP_CTRL0_XFER_COUNT;
 	ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
 
-	if (!write)
+	if (!(flags & TXRX_WRITE))
 		ctrl0 |= BM_SSP_CTRL0_READ;
 
 	/* Queue the DMA data transfer. */
 	for (sg_count = 0; sg_count < sgs; sg_count++) {
+		/* Prepare the transfer descriptor. */
 		min = min(len, desc_len);
 
-		/* Prepare the transfer descriptor. */
-		if ((sg_count + 1 == sgs) && *last)
+		/*
+		 * De-assert CS on last segment if flag is set (i.e., no more
+		 * transfers will follow)
+		 */
+		if ((sg_count + 1 == sgs) && (flags & TXRX_DEASSERT_CS))
 			ctrl0 |= BM_SSP_CTRL0_IGNORE_CRC;
 
 		if (ssp->devid == IMX23_SSP) {
@@ -259,7 +270,7 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
 
 		sg_init_one(&dma_xfer[sg_count].sg, sg_buf, min);
 		ret = dma_map_sg(ssp->dev, &dma_xfer[sg_count].sg, 1,
-			write ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+			(flags & TXRX_WRITE) ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
 
 		len -= min;
 		buf += min;
@@ -279,7 +290,7 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
 
 		desc = dmaengine_prep_slave_sg(ssp->dmach,
 				&dma_xfer[sg_count].sg, 1,
-				write ? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM,
+				(flags & TXRX_WRITE) ? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM,
 				DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 
 		if (!desc) {
@@ -316,7 +327,7 @@ err_vmalloc:
 	while (--sg_count >= 0) {
 err_mapped:
 		dma_unmap_sg(ssp->dev, &dma_xfer[sg_count].sg, 1,
-			write ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+			(flags & TXRX_WRITE) ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
 	}
 
 	kfree(dma_xfer);
@@ -326,7 +337,7 @@ err_mapped:
 
 static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
 			    unsigned char *buf, int len,
-			    int *first, int *last, int write)
+			    unsigned int flags)
 {
 	struct mxs_ssp *ssp = &spi->ssp;
 
@@ -337,7 +348,7 @@ static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
 	mxs_spi_set_cs(spi, cs);
 
 	while (len--) {
-		if (*last && len == 0)
+		if (len == 0 && (flags & TXRX_DEASSERT_CS))
 			writel(BM_SSP_CTRL0_IGNORE_CRC,
 			       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 
@@ -350,7 +361,7 @@ static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
 			writel(1, ssp->base + HW_SSP_XFER_SIZE);
 		}
 
-		if (write)
+		if (flags & TXRX_WRITE)
 			writel(BM_SSP_CTRL0_READ,
 				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
 		else
@@ -363,13 +374,13 @@ static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
 		if (mxs_ssp_wait(spi, HW_SSP_CTRL0, BM_SSP_CTRL0_RUN, 1))
 			return -ETIMEDOUT;
 
-		if (write)
+		if (flags & TXRX_WRITE)
 			writel(*buf, ssp->base + HW_SSP_DATA(ssp));
 
 		writel(BM_SSP_CTRL0_DATA_XFER,
 			     ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 
-		if (!write) {
+		if (!(flags & TXRX_WRITE)) {
 			if (mxs_ssp_wait(spi, HW_SSP_STATUS(ssp),
 						BM_SSP_STATUS_FIFO_EMPTY, 0))
 				return -ETIMEDOUT;
@@ -394,13 +405,11 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 {
 	struct mxs_spi *spi = spi_master_get_devdata(master);
 	struct mxs_ssp *ssp = &spi->ssp;
-	int first, last;
 	struct spi_transfer *t, *tmp_t;
+	unsigned int flag;
 	int status = 0;
 	int cs;
 
-	first = last = 0;
-
 	cs = m->spi->chip_select;
 
 	list_for_each_entry_safe(t, tmp_t, &m->transfers, transfer_list) {
@@ -409,10 +418,9 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 		if (status)
 			break;
 
-		if (&t->transfer_list == m->transfers.next)
-			first = 1;
-		if (&t->transfer_list == m->transfers.prev)
-			last = 1;
+		/* De-assert on last transfer, inverted by cs_change flag */
+		flag = (&t->transfer_list == m->transfers.prev) ^ t->cs_change ?
+		       TXRX_DEASSERT_CS : 0;
 		if ((t->rx_buf && t->tx_buf) || (t->rx_dma && t->tx_dma)) {
 			dev_err(ssp->dev,
 				"Cannot send and receive simultaneously\n");
@@ -437,11 +445,11 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 			if (t->tx_buf)
 				status = mxs_spi_txrx_pio(spi, cs,
 						(void *)t->tx_buf,
-						t->len, &first, &last, 1);
+						t->len, flag | TXRX_WRITE);
 			if (t->rx_buf)
 				status = mxs_spi_txrx_pio(spi, cs,
 						t->rx_buf, t->len,
-						&first, &last, 0);
+						flag);
 		} else {
 			writel(BM_SSP_CTRL1_DMA_ENABLE,
 				ssp->base + HW_SSP_CTRL1(ssp) +
@@ -450,11 +458,11 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 			if (t->tx_buf)
 				status = mxs_spi_txrx_dma(spi, cs,
 						(void *)t->tx_buf, t->len,
-						&first, &last, 1);
+						flag | TXRX_WRITE);
 			if (t->rx_buf)
 				status = mxs_spi_txrx_dma(spi, cs,
 						t->rx_buf, t->len,
-						&first, &last, 0);
+						flag);
 		}
 
 		if (status) {
@@ -463,7 +471,6 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 		}
 
 		m->actual_length += t->len;
-		first = last = 0;
 	}
 
 	m->status = status;
-- 
1.7.10.4


------------------------------------------------------------------------------
Own the Future-Intel(R) Level Up Game Demo Contest 2013
Rise to greatness in Intel's independent game demo contest. Compete 
for recognition, cash, and the chance to get your game on Steam. 
$5K grand prize plus 10 genre and skill prizes. Submit your demo 
by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2

[PATCH V2 03/12] spi/mxs: Change flag arguments in txrx functions to bit flags

[Trent Piepho]

There are three flag arguments to the PIO and DMA txrx functions.  Two
are passed as pointers to integers, even though they are input only
and not modified, which makes no sense to do.  The third is passed as
an integer.

The compiler must use an argument register or stack variable for each
flag this way.  By using bitflags in a single flag argument more
efficient and smaller code is produced since all the flags can fit in
a single register.  And all the flag arguments get cumbersome,
especially when more are added for things like GPIO chipselects.

The "first" flag is never used, so can just be deleted.

The "last" flag is renamed to DEASSERT_CS, since that's really what it
does.  The spi_transfer cs_change flag means that CS might be
de-asserted on a transfer which is not last and not de-assert on the
last transfer, so it is not which transfer is the last we need to know
but rather the transfers which after which CS should be de-asserted.

This also extends the driver to not ignore cs_change when setting the
DEASSERT_CS nee "last" flag.

Signed-off-by: Trent Piepho <tpiepho@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/spi/spi-mxs.c |   55 ++++++++++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 7eb4bc9..3064304 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -58,6 +58,13 @@
 
 #define SG_MAXLEN		0xff00
 
+/*
+ * Flags for txrx functions.  More efficient that using an argument register for
+ * each one.
+ */
+#define TXRX_WRITE		1	/* This is a write */
+#define TXRX_DEASSERT_CS	2	/* De-assert CS at end of txrx */
+
 struct mxs_spi {
 	struct mxs_ssp		ssp;
 	struct completion	c;
@@ -196,7 +203,7 @@ static irqreturn_t mxs_ssp_irq_handler(int irq, void *dev_id)
 
 static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
 			    unsigned char *buf, int len,
-			    int *first, int *last, int write)
+			    unsigned int flags)
 {
 	struct mxs_ssp *ssp = &spi->ssp;
 	struct dma_async_tx_descriptor *desc = NULL;
@@ -226,15 +233,19 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
 	ctrl0 &= ~BM_SSP_CTRL0_XFER_COUNT;
 	ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
 
-	if (!write)
+	if (!(flags & TXRX_WRITE))
 		ctrl0 |= BM_SSP_CTRL0_READ;
 
 	/* Queue the DMA data transfer. */
 	for (sg_count = 0; sg_count < sgs; sg_count++) {
+		/* Prepare the transfer descriptor. */
 		min = min(len, desc_len);
 
-		/* Prepare the transfer descriptor. */
-		if ((sg_count + 1 == sgs) && *last)
+		/*
+		 * De-assert CS on last segment if flag is set (i.e., no more
+		 * transfers will follow)
+		 */
+		if ((sg_count + 1 == sgs) && (flags & TXRX_DEASSERT_CS))
 			ctrl0 |= BM_SSP_CTRL0_IGNORE_CRC;
 
 		if (ssp->devid == IMX23_SSP) {
@@ -259,7 +270,7 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
 
 		sg_init_one(&dma_xfer[sg_count].sg, sg_buf, min);
 		ret = dma_map_sg(ssp->dev, &dma_xfer[sg_count].sg, 1,
-			write ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+			(flags & TXRX_WRITE) ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
 
 		len -= min;
 		buf += min;
@@ -279,7 +290,7 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
 
 		desc = dmaengine_prep_slave_sg(ssp->dmach,
 				&dma_xfer[sg_count].sg, 1,
-				write ? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM,
+				(flags & TXRX_WRITE) ? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM,
 				DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 
 		if (!desc) {
@@ -316,7 +327,7 @@ err_vmalloc:
 	while (--sg_count >= 0) {
 err_mapped:
 		dma_unmap_sg(ssp->dev, &dma_xfer[sg_count].sg, 1,
-			write ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+			(flags & TXRX_WRITE) ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
 	}
 
 	kfree(dma_xfer);
@@ -326,7 +337,7 @@ err_mapped:
 
 static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
 			    unsigned char *buf, int len,
-			    int *first, int *last, int write)
+			    unsigned int flags)
 {
 	struct mxs_ssp *ssp = &spi->ssp;
 
@@ -337,7 +348,7 @@ static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
 	mxs_spi_set_cs(spi, cs);
 
 	while (len--) {
-		if (*last && len == 0)
+		if (len == 0 && (flags & TXRX_DEASSERT_CS))
 			writel(BM_SSP_CTRL0_IGNORE_CRC,
 			       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 
@@ -350,7 +361,7 @@ static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
 			writel(1, ssp->base + HW_SSP_XFER_SIZE);
 		}
 
-		if (write)
+		if (flags & TXRX_WRITE)
 			writel(BM_SSP_CTRL0_READ,
 				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
 		else
@@ -363,13 +374,13 @@ static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
 		if (mxs_ssp_wait(spi, HW_SSP_CTRL0, BM_SSP_CTRL0_RUN, 1))
 			return -ETIMEDOUT;
 
-		if (write)
+		if (flags & TXRX_WRITE)
 			writel(*buf, ssp->base + HW_SSP_DATA(ssp));
 
 		writel(BM_SSP_CTRL0_DATA_XFER,
 			     ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 
-		if (!write) {
+		if (!(flags & TXRX_WRITE)) {
 			if (mxs_ssp_wait(spi, HW_SSP_STATUS(ssp),
 						BM_SSP_STATUS_FIFO_EMPTY, 0))
 				return -ETIMEDOUT;
@@ -394,13 +405,11 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 {
 	struct mxs_spi *spi = spi_master_get_devdata(master);
 	struct mxs_ssp *ssp = &spi->ssp;
-	int first, last;
 	struct spi_transfer *t, *tmp_t;
+	unsigned int flag;
 	int status = 0;
 	int cs;
 
-	first = last = 0;
-
 	cs = m->spi->chip_select;
 
 	list_for_each_entry_safe(t, tmp_t, &m->transfers, transfer_list) {
@@ -409,10 +418,9 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 		if (status)
 			break;
 
-		if (&t->transfer_list == m->transfers.next)
-			first = 1;
-		if (&t->transfer_list == m->transfers.prev)
-			last = 1;
+		/* De-assert on last transfer, inverted by cs_change flag */
+		flag = (&t->transfer_list == m->transfers.prev) ^ t->cs_change ?
+		       TXRX_DEASSERT_CS : 0;
 		if ((t->rx_buf && t->tx_buf) || (t->rx_dma && t->tx_dma)) {
 			dev_err(ssp->dev,
 				"Cannot send and receive simultaneously\n");
@@ -437,11 +445,11 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 			if (t->tx_buf)
 				status = mxs_spi_txrx_pio(spi, cs,
 						(void *)t->tx_buf,
-						t->len, &first, &last, 1);
+						t->len, flag | TXRX_WRITE);
 			if (t->rx_buf)
 				status = mxs_spi_txrx_pio(spi, cs,
 						t->rx_buf, t->len,
-						&first, &last, 0);
+						flag);
 		} else {
 			writel(BM_SSP_CTRL1_DMA_ENABLE,
 				ssp->base + HW_SSP_CTRL1(ssp) +
@@ -450,11 +458,11 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 			if (t->tx_buf)
 				status = mxs_spi_txrx_dma(spi, cs,
 						(void *)t->tx_buf, t->len,
-						&first, &last, 1);
+						flag | TXRX_WRITE);
 			if (t->rx_buf)
 				status = mxs_spi_txrx_dma(spi, cs,
 						t->rx_buf, t->len,
-						&first, &last, 0);
+						flag);
 		}
 
 		if (status) {
@@ -463,7 +471,6 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 		}
 
 		m->actual_length += t->len;
-		first = last = 0;
 	}
 
 	m->status = status;
-- 
1.7.10.4

[PATCH V2 04/12] spi/mxs: Fix extra CS pulses and read mode in multi-transfer messages

[Trent Piepho]

There are two bits which control the CS line in the CTRL0 register:
LOCK_CS and IGNORE_CRC.  The latter would be better named DEASSERT_CS
in SPI mode.

Setting DEASSERT_CS causes CS to be de-asserted at the end of the
transfer.  It would normally be set on the final segment of the final
transfer.  The DMA code explicitly sets it in this case, but because
it never clears the bit from the ctrl0 register is will remain set for
all transfers in subsequent messages.  This results in a CS pulse
between transfers.

There is a similar problem with the read mode bit never being cleared
in DMA mode.

This patch fixes DEASSERT_CS and READ being left on in DMA mode.

Signed-off-by: Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-mxs.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 3064304..86a714b 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -230,7 +230,8 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
 	INIT_COMPLETION(spi->c);
 
 	ctrl0 = readl(ssp->base + HW_SSP_CTRL0);
-	ctrl0 &= ~BM_SSP_CTRL0_XFER_COUNT;
+	ctrl0 &= ~(BM_SSP_CTRL0_XFER_COUNT | BM_SSP_CTRL0_IGNORE_CRC |
+		 BM_SSP_CTRL0_READ);
 	ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
 
 	if (!(flags & TXRX_WRITE))
-- 
1.7.10.4


------------------------------------------------------------------------------
Own the Future-Intel(R) Level Up Game Demo Contest 2013
Rise to greatness in Intel's independent game demo contest. Compete 
for recognition, cash, and the chance to get your game on Steam. 
$5K grand prize plus 10 genre and skill prizes. Submit your demo 
by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2

[PATCH V2 04/12] spi/mxs: Fix extra CS pulses and read mode in multi-transfer messages

[Trent Piepho]

There are two bits which control the CS line in the CTRL0 register:
LOCK_CS and IGNORE_CRC.  The latter would be better named DEASSERT_CS
in SPI mode.

Setting DEASSERT_CS causes CS to be de-asserted at the end of the
transfer.  It would normally be set on the final segment of the final
transfer.  The DMA code explicitly sets it in this case, but because
it never clears the bit from the ctrl0 register is will remain set for
all transfers in subsequent messages.  This results in a CS pulse
between transfers.

There is a similar problem with the read mode bit never being cleared
in DMA mode.

This patch fixes DEASSERT_CS and READ being left on in DMA mode.

Signed-off-by: Trent Piepho <tpiepho@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/spi/spi-mxs.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 3064304..86a714b 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -230,7 +230,8 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
 	INIT_COMPLETION(spi->c);
 
 	ctrl0 = readl(ssp->base + HW_SSP_CTRL0);
-	ctrl0 &= ~BM_SSP_CTRL0_XFER_COUNT;
+	ctrl0 &= ~(BM_SSP_CTRL0_XFER_COUNT | BM_SSP_CTRL0_IGNORE_CRC |
+		 BM_SSP_CTRL0_READ);
 	ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
 
 	if (!(flags & TXRX_WRITE))
-- 
1.7.10.4

[PATCH V2 05/12] spi/mxs: Fix chip select control bits in DMA mode

[Trent Piepho]

In DMA mode the chip select control bits would be ORed into the CTRL0
register without first clearing the bits.  This means that after
addressing slave 1 the bit would be still be set when addressing slave
0, resulting in slave 1 continuing to be addressed.

The message handing function would pass the cs value to the txrx
function, which would re-program the bits on each transfer in the
message.  The selected cs does not change during a message so this is
inefficient.  It also means there are two different sets of code for
selecting the CS, one for PIO that worked and one for DMA that didn't.

Change the code to set the CS bits in the message transfer function
once.  Now the DMA and PIO txrx functions don't need to care about CS
at all.

Signed-off-by: Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-mxs.c |   36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 86a714b..ad3fd9c 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -152,18 +152,6 @@ static uint32_t mxs_spi_cs_to_reg(unsigned cs)
 	return select;
 }
 
-static void mxs_spi_set_cs(struct mxs_spi *spi, unsigned cs)
-{
-	const uint32_t mask =
-		BM_SSP_CTRL0_WAIT_FOR_CMD | BM_SSP_CTRL0_WAIT_FOR_IRQ;
-	uint32_t select;
-	struct mxs_ssp *ssp = &spi->ssp;
-
-	writel(mask, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
-	select = mxs_spi_cs_to_reg(cs);
-	writel(select, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
-}
-
 static int mxs_ssp_wait(struct mxs_spi *spi, int offset, int mask, bool set)
 {
 	const unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT);
@@ -201,7 +189,7 @@ static irqreturn_t mxs_ssp_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
+static int mxs_spi_txrx_dma(struct mxs_spi *spi,
 			    unsigned char *buf, int len,
 			    unsigned int flags)
 {
@@ -229,10 +217,11 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
 
 	INIT_COMPLETION(spi->c);
 
+	/* Chip select was already programmed into CTRL0 */
 	ctrl0 = readl(ssp->base + HW_SSP_CTRL0);
 	ctrl0 &= ~(BM_SSP_CTRL0_XFER_COUNT | BM_SSP_CTRL0_IGNORE_CRC |
 		 BM_SSP_CTRL0_READ);
-	ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
+	ctrl0 |= BM_SSP_CTRL0_DATA_XFER;
 
 	if (!(flags & TXRX_WRITE))
 		ctrl0 |= BM_SSP_CTRL0_READ;
@@ -336,7 +325,7 @@ err_mapped:
 	return ret;
 }
 
-static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
+static int mxs_spi_txrx_pio(struct mxs_spi *spi,
 			    unsigned char *buf, int len,
 			    unsigned int flags)
 {
@@ -346,8 +335,6 @@ static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
 	writel(BM_SSP_CTRL0_IGNORE_CRC,
 	       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
 
-	mxs_spi_set_cs(spi, cs);
-
 	while (len--) {
 		if (len == 0 && (flags & TXRX_DEASSERT_CS))
 			writel(BM_SSP_CTRL0_IGNORE_CRC,
@@ -409,9 +396,12 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 	struct spi_transfer *t, *tmp_t;
 	unsigned int flag;
 	int status = 0;
-	int cs;
 
-	cs = m->spi->chip_select;
+	/* Program CS register bits here, it will be used for all transfers. */
+	writel(BM_SSP_CTRL0_WAIT_FOR_CMD | BM_SSP_CTRL0_WAIT_FOR_IRQ,
+	       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
+	writel(mxs_spi_cs_to_reg(m->spi->chip_select),
+	       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 
 	list_for_each_entry_safe(t, tmp_t, &m->transfers, transfer_list) {
 
@@ -444,11 +434,11 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 				STMP_OFFSET_REG_CLR);
 
 			if (t->tx_buf)
-				status = mxs_spi_txrx_pio(spi, cs,
+				status = mxs_spi_txrx_pio(spi,
 						(void *)t->tx_buf,
 						t->len, flag | TXRX_WRITE);
 			if (t->rx_buf)
-				status = mxs_spi_txrx_pio(spi, cs,
+				status = mxs_spi_txrx_pio(spi,
 						t->rx_buf, t->len,
 						flag);
 		} else {
@@ -457,11 +447,11 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 				STMP_OFFSET_REG_SET);
 
 			if (t->tx_buf)
-				status = mxs_spi_txrx_dma(spi, cs,
+				status = mxs_spi_txrx_dma(spi,
 						(void *)t->tx_buf, t->len,
 						flag | TXRX_WRITE);
 			if (t->rx_buf)
-				status = mxs_spi_txrx_dma(spi, cs,
+				status = mxs_spi_txrx_dma(spi,
 						t->rx_buf, t->len,
 						flag);
 		}
-- 
1.7.10.4


------------------------------------------------------------------------------
Own the Future-Intel(R) Level Up Game Demo Contest 2013
Rise to greatness in Intel's independent game demo contest. Compete 
for recognition, cash, and the chance to get your game on Steam. 
$5K grand prize plus 10 genre and skill prizes. Submit your demo 
by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2

[PATCH V2 05/12] spi/mxs: Fix chip select control bits in DMA mode

[Trent Piepho]

In DMA mode the chip select control bits would be ORed into the CTRL0
register without first clearing the bits.  This means that after
addressing slave 1 the bit would be still be set when addressing slave
0, resulting in slave 1 continuing to be addressed.

The message handing function would pass the cs value to the txrx
function, which would re-program the bits on each transfer in the
message.  The selected cs does not change during a message so this is
inefficient.  It also means there are two different sets of code for
selecting the CS, one for PIO that worked and one for DMA that didn't.

Change the code to set the CS bits in the message transfer function
once.  Now the DMA and PIO txrx functions don't need to care about CS
at all.

Signed-off-by: Trent Piepho <tpiepho@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/spi/spi-mxs.c |   36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 86a714b..ad3fd9c 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -152,18 +152,6 @@ static uint32_t mxs_spi_cs_to_reg(unsigned cs)
 	return select;
 }
 
-static void mxs_spi_set_cs(struct mxs_spi *spi, unsigned cs)
-{
-	const uint32_t mask =
-		BM_SSP_CTRL0_WAIT_FOR_CMD | BM_SSP_CTRL0_WAIT_FOR_IRQ;
-	uint32_t select;
-	struct mxs_ssp *ssp = &spi->ssp;
-
-	writel(mask, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
-	select = mxs_spi_cs_to_reg(cs);
-	writel(select, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
-}
-
 static int mxs_ssp_wait(struct mxs_spi *spi, int offset, int mask, bool set)
 {
 	const unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT);
@@ -201,7 +189,7 @@ static irqreturn_t mxs_ssp_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
+static int mxs_spi_txrx_dma(struct mxs_spi *spi,
 			    unsigned char *buf, int len,
 			    unsigned int flags)
 {
@@ -229,10 +217,11 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
 
 	INIT_COMPLETION(spi->c);
 
+	/* Chip select was already programmed into CTRL0 */
 	ctrl0 = readl(ssp->base + HW_SSP_CTRL0);
 	ctrl0 &= ~(BM_SSP_CTRL0_XFER_COUNT | BM_SSP_CTRL0_IGNORE_CRC |
 		 BM_SSP_CTRL0_READ);
-	ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
+	ctrl0 |= BM_SSP_CTRL0_DATA_XFER;
 
 	if (!(flags & TXRX_WRITE))
 		ctrl0 |= BM_SSP_CTRL0_READ;
@@ -336,7 +325,7 @@ err_mapped:
 	return ret;
 }
 
-static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
+static int mxs_spi_txrx_pio(struct mxs_spi *spi,
 			    unsigned char *buf, int len,
 			    unsigned int flags)
 {
@@ -346,8 +335,6 @@ static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
 	writel(BM_SSP_CTRL0_IGNORE_CRC,
 	       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
 
-	mxs_spi_set_cs(spi, cs);
-
 	while (len--) {
 		if (len == 0 && (flags & TXRX_DEASSERT_CS))
 			writel(BM_SSP_CTRL0_IGNORE_CRC,
@@ -409,9 +396,12 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 	struct spi_transfer *t, *tmp_t;
 	unsigned int flag;
 	int status = 0;
-	int cs;
 
-	cs = m->spi->chip_select;
+	/* Program CS register bits here, it will be used for all transfers. */
+	writel(BM_SSP_CTRL0_WAIT_FOR_CMD | BM_SSP_CTRL0_WAIT_FOR_IRQ,
+	       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
+	writel(mxs_spi_cs_to_reg(m->spi->chip_select),
+	       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 
 	list_for_each_entry_safe(t, tmp_t, &m->transfers, transfer_list) {
 
@@ -444,11 +434,11 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 				STMP_OFFSET_REG_CLR);
 
 			if (t->tx_buf)
-				status = mxs_spi_txrx_pio(spi, cs,
+				status = mxs_spi_txrx_pio(spi,
 						(void *)t->tx_buf,
 						t->len, flag | TXRX_WRITE);
 			if (t->rx_buf)
-				status = mxs_spi_txrx_pio(spi, cs,
+				status = mxs_spi_txrx_pio(spi,
 						t->rx_buf, t->len,
 						flag);
 		} else {
@@ -457,11 +447,11 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 				STMP_OFFSET_REG_SET);
 
 			if (t->tx_buf)
-				status = mxs_spi_txrx_dma(spi, cs,
+				status = mxs_spi_txrx_dma(spi,
 						(void *)t->tx_buf, t->len,
 						flag | TXRX_WRITE);
 			if (t->rx_buf)
-				status = mxs_spi_txrx_dma(spi, cs,
+				status = mxs_spi_txrx_dma(spi,
 						t->rx_buf, t->len,
 						flag);
 		}
-- 
1.7.10.4

[PATCH V2 06/12] spi/mxs: Remove full duplex check, spi core already does it

[Trent Piepho]

Because the driver sets the SPI_MASTER_HALF_DUPLEX flag, the spi core
will check transfers to insure they are not full duplex.  It's not
necessary to check that in the mxs driver as well.

Signed-off-by: Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-mxs.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index ad3fd9c..208de97 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -412,12 +412,6 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 		/* De-assert on last transfer, inverted by cs_change flag */
 		flag = (&t->transfer_list == m->transfers.prev) ^ t->cs_change ?
 		       TXRX_DEASSERT_CS : 0;
-		if ((t->rx_buf && t->tx_buf) || (t->rx_dma && t->tx_dma)) {
-			dev_err(ssp->dev,
-				"Cannot send and receive simultaneously\n");
-			status = -EINVAL;
-			break;
-		}
 
 		/*
 		 * Small blocks can be transfered via PIO.
-- 
1.7.10.4


------------------------------------------------------------------------------
Own the Future-Intel(R) Level Up Game Demo Contest 2013
Rise to greatness in Intel's independent game demo contest. Compete 
for recognition, cash, and the chance to get your game on Steam. 
$5K grand prize plus 10 genre and skill prizes. Submit your demo 
by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2

[PATCH V2 06/12] spi/mxs: Remove full duplex check, spi core already does it

[Trent Piepho]

Because the driver sets the SPI_MASTER_HALF_DUPLEX flag, the spi core
will check transfers to insure they are not full duplex.  It's not
necessary to check that in the mxs driver as well.

Signed-off-by: Trent Piepho <tpiepho@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/spi/spi-mxs.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index ad3fd9c..208de97 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -412,12 +412,6 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 		/* De-assert on last transfer, inverted by cs_change flag */
 		flag = (&t->transfer_list == m->transfers.prev) ^ t->cs_change ?
 		       TXRX_DEASSERT_CS : 0;
-		if ((t->rx_buf && t->tx_buf) || (t->rx_dma && t->tx_dma)) {
-			dev_err(ssp->dev,
-				"Cannot send and receive simultaneously\n");
-			status = -EINVAL;
-			break;
-		}
 
 		/*
 		 * Small blocks can be transfered via PIO.
-- 
1.7.10.4

[PATCH V2 07/12] spi/mxs: Remove bogus setting of ssp clk rate field

[Trent Piepho]

The ssp struct has a clock rate field, to provide the actual value, in Hz,
of the SSP output clock (the rate of SSP_SCK) after mxs_ssp_set_clk_rate()
is called.  It should be read-only, except for mxs_ssp_set_clk_rate().

For some reason the spi-mxs driver decides to write to this field on init,
and sets it to the value of the SSP input clock (clk_sspN, from the MXS
clocking block) in kHz.  It shouldn't be setting the value, and certainly
shouldn't be setting it with the wrong clock in the wrong units.

Signed-off-by: Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-mxs.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 208de97..103c478 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -591,7 +591,6 @@ static int mxs_spi_probe(struct platform_device *pdev)
 
 	clk_prepare_enable(ssp->clk);
 	clk_set_rate(ssp->clk, clk_freq);
-	ssp->clk_rate = clk_get_rate(ssp->clk) / 1000;
 
 	stmp_reset_block(ssp->base);
 
-- 
1.7.10.4


------------------------------------------------------------------------------
Own the Future-Intel(R) Level Up Game Demo Contest 2013
Rise to greatness in Intel's independent game demo contest. Compete 
for recognition, cash, and the chance to get your game on Steam. 
$5K grand prize plus 10 genre and skill prizes. Submit your demo 
by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2

[PATCH V2 07/12] spi/mxs: Remove bogus setting of ssp clk rate field

[Trent Piepho]

The ssp struct has a clock rate field, to provide the actual value, in Hz,
of the SSP output clock (the rate of SSP_SCK) after mxs_ssp_set_clk_rate()
is called.  It should be read-only, except for mxs_ssp_set_clk_rate().

For some reason the spi-mxs driver decides to write to this field on init,
and sets it to the value of the SSP input clock (clk_sspN, from the MXS
clocking block) in kHz.  It shouldn't be setting the value, and certainly
shouldn't be setting it with the wrong clock in the wrong units.

Signed-off-by: Trent Piepho <tpiepho@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/spi/spi-mxs.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 208de97..103c478 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -591,7 +591,6 @@ static int mxs_spi_probe(struct platform_device *pdev)
 
 	clk_prepare_enable(ssp->clk);
 	clk_set_rate(ssp->clk, clk_freq);
-	ssp->clk_rate = clk_get_rate(ssp->clk) / 1000;
 
 	stmp_reset_block(ssp->base);
 
-- 
1.7.10.4

[PATCH V2 08/12] spi/mxs: Fix race in setup method

[Trent Piepho]

Despite many warnings in the SPI documentation and code, the spi-mxs
driver sets shared chip registers in the ->setup method.  This method can
be called when transfers are in progress on other slaves controlled by the
master.  Setting registers or any other shared state will corrupt those
transfers.

So fix mxs_spi_setup() to not call mxs_spi_setup_transfer().

Signed-off-by: Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-mxs.c |   11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 103c478..2a2147a 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -115,21 +115,16 @@ static int mxs_spi_setup_transfer(struct spi_device *dev,
 
 static int mxs_spi_setup(struct spi_device *dev)
 {
-	int err = 0;
-
 	if (!dev->bits_per_word)
 		dev->bits_per_word = 8;
 
 	if (dev->mode & ~(SPI_CPOL | SPI_CPHA))
 		return -EINVAL;
 
-	err = mxs_spi_setup_transfer(dev, NULL);
-	if (err) {
-		dev_err(&dev->dev,
-			"Failed to setup transfer, error = %d\n", err);
-	}
+	if (dev->bits_per_word != 8)
+		return -EINVAL;
 
-	return err;
+	return 0;
 }
 
 static uint32_t mxs_spi_cs_to_reg(unsigned cs)
-- 
1.7.10.4


------------------------------------------------------------------------------
Own the Future-Intel(R) Level Up Game Demo Contest 2013
Rise to greatness in Intel's independent game demo contest. Compete 
for recognition, cash, and the chance to get your game on Steam. 
$5K grand prize plus 10 genre and skill prizes. Submit your demo 
by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2

[PATCH V2 08/12] spi/mxs: Fix race in setup method

[Trent Piepho]

Despite many warnings in the SPI documentation and code, the spi-mxs
driver sets shared chip registers in the ->setup method.  This method can
be called when transfers are in progress on other slaves controlled by the
master.  Setting registers or any other shared state will corrupt those
transfers.

So fix mxs_spi_setup() to not call mxs_spi_setup_transfer().

Signed-off-by: Trent Piepho <tpiepho@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/spi/spi-mxs.c |   11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 103c478..2a2147a 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -115,21 +115,16 @@ static int mxs_spi_setup_transfer(struct spi_device *dev,
 
 static int mxs_spi_setup(struct spi_device *dev)
 {
-	int err = 0;
-
 	if (!dev->bits_per_word)
 		dev->bits_per_word = 8;
 
 	if (dev->mode & ~(SPI_CPOL | SPI_CPHA))
 		return -EINVAL;
 
-	err = mxs_spi_setup_transfer(dev, NULL);
-	if (err) {
-		dev_err(&dev->dev,
-			"Failed to setup transfer, error = %d\n", err);
-	}
+	if (dev->bits_per_word != 8)
+		return -EINVAL;
 
-	return err;
+	return 0;
 }
 
 static uint32_t mxs_spi_cs_to_reg(unsigned cs)
-- 
1.7.10.4

[PATCH V2 09/12] spi/mxs: Remove check of spi mode bits

[Trent Piepho]

The spi core already checks for a slave setting mode bits that we
didn't list as supported when the master was registered.  There is no
need to do it again in the master driver.

Signed-off-by: Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-mxs.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 2a2147a..d71029e 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -118,9 +118,6 @@ static int mxs_spi_setup(struct spi_device *dev)
 	if (!dev->bits_per_word)
 		dev->bits_per_word = 8;
 
-	if (dev->mode & ~(SPI_CPOL | SPI_CPHA))
-		return -EINVAL;
-
 	if (dev->bits_per_word != 8)
 		return -EINVAL;
 
-- 
1.7.10.4


------------------------------------------------------------------------------
Own the Future-Intel(R) Level Up Game Demo Contest 2013
Rise to greatness in Intel's independent game demo contest. Compete 
for recognition, cash, and the chance to get your game on Steam. 
$5K grand prize plus 10 genre and skill prizes. Submit your demo 
by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2

[PATCH V2 09/12] spi/mxs: Remove check of spi mode bits

[Trent Piepho]

The spi core already checks for a slave setting mode bits that we
didn't list as supported when the master was registered.  There is no
need to do it again in the master driver.

Signed-off-by: Trent Piepho <tpiepho@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/spi/spi-mxs.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 2a2147a..d71029e 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -118,9 +118,6 @@ static int mxs_spi_setup(struct spi_device *dev)
 	if (!dev->bits_per_word)
 		dev->bits_per_word = 8;
 
-	if (dev->mode & ~(SPI_CPOL | SPI_CPHA))
-		return -EINVAL;
-
 	if (dev->bits_per_word != 8)
 		return -EINVAL;
 
-- 
1.7.10.4

[PATCH V2 10/12] spi/mxs: Clean up setup_transfer function

[Trent Piepho]

It can't be called with a NULL transfer anymore so it can be
simplified to not check for that.

Change printouts to more closely match what the spi core would print.
I.e., don't print function names.  Try to make them clearer.

Fix indention of line-wrapped code to Linux standard.

The transfer pointer can be const.

It's not necessary to check if the spi_transfer's bit per word is
zero.  The spi core automatically inserts the spi_device's bits per
word in that case.

It's not necessary to check if the spi_transfer's speed_hz is zero, as
the spi core also fills it in from the spi_device.  However, the spi
core does not check if spi_device's speed is zero so we have to do
that still.

Signed-off-by: Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-mxs.c |   30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index d71029e..a7d5f85 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -71,28 +71,20 @@ struct mxs_spi {
 };
 
 static int mxs_spi_setup_transfer(struct spi_device *dev,
-				struct spi_transfer *t)
+				  const struct spi_transfer *t)
 {
 	struct mxs_spi *spi = spi_master_get_devdata(dev->master);
 	struct mxs_ssp *ssp = &spi->ssp;
-	uint8_t bits_per_word;
-	uint32_t hz = 0;
+	const unsigned int hz = min(dev->max_speed_hz, t->speed_hz);
 
-	bits_per_word = dev->bits_per_word;
-	if (t && t->bits_per_word)
-		bits_per_word = t->bits_per_word;
-
-	if (bits_per_word != 8) {
-		dev_err(&dev->dev, "%s, unsupported bits_per_word=%d\n",
-					__func__, bits_per_word);
+	if (t->bits_per_word != 8) {
+		dev_err(&dev->dev, "Unsupported bits per word of %d\n",
+			t->bits_per_word);
 		return -EINVAL;
 	}
 
-	hz = dev->max_speed_hz;
-	if (t && t->speed_hz)
-		hz = min(hz, t->speed_hz);
 	if (hz == 0) {
-		dev_err(&dev->dev, "Cannot continue with zero clock\n");
+		dev_err(&dev->dev, "SPI clock rate of zero not possible\n");
 		return -EINVAL;
 	}
 
@@ -100,12 +92,12 @@ static int mxs_spi_setup_transfer(struct spi_device *dev,
 
 	writel(BM_SSP_CTRL0_LOCK_CS,
 		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+
 	writel(BF_SSP_CTRL1_SSP_MODE(BV_SSP_CTRL1_SSP_MODE__SPI) |
-		     BF_SSP_CTRL1_WORD_LENGTH
-		     (BV_SSP_CTRL1_WORD_LENGTH__EIGHT_BITS) |
-		     ((dev->mode & SPI_CPOL) ? BM_SSP_CTRL1_POLARITY : 0) |
-		     ((dev->mode & SPI_CPHA) ? BM_SSP_CTRL1_PHASE : 0),
-		     ssp->base + HW_SSP_CTRL1(ssp));
+	       BF_SSP_CTRL1_WORD_LENGTH(BV_SSP_CTRL1_WORD_LENGTH__EIGHT_BITS) |
+	       ((dev->mode & SPI_CPOL) ? BM_SSP_CTRL1_POLARITY : 0) |
+	       ((dev->mode & SPI_CPHA) ? BM_SSP_CTRL1_PHASE : 0),
+	       ssp->base + HW_SSP_CTRL1(ssp));
 
 	writel(0x0, ssp->base + HW_SSP_CMD0);
 	writel(0x0, ssp->base + HW_SSP_CMD1);
-- 
1.7.10.4


------------------------------------------------------------------------------
Own the Future-Intel(R) Level Up Game Demo Contest 2013
Rise to greatness in Intel's independent game demo contest. Compete 
for recognition, cash, and the chance to get your game on Steam. 
$5K grand prize plus 10 genre and skill prizes. Submit your demo 
by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2

[PATCH V2 10/12] spi/mxs: Clean up setup_transfer function

[Trent Piepho]

It can't be called with a NULL transfer anymore so it can be
simplified to not check for that.

Change printouts to more closely match what the spi core would print.
I.e., don't print function names.  Try to make them clearer.

Fix indention of line-wrapped code to Linux standard.

The transfer pointer can be const.

It's not necessary to check if the spi_transfer's bit per word is
zero.  The spi core automatically inserts the spi_device's bits per
word in that case.

It's not necessary to check if the spi_transfer's speed_hz is zero, as
the spi core also fills it in from the spi_device.  However, the spi
core does not check if spi_device's speed is zero so we have to do
that still.

Signed-off-by: Trent Piepho <tpiepho@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/spi/spi-mxs.c |   30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index d71029e..a7d5f85 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -71,28 +71,20 @@ struct mxs_spi {
 };
 
 static int mxs_spi_setup_transfer(struct spi_device *dev,
-				struct spi_transfer *t)
+				  const struct spi_transfer *t)
 {
 	struct mxs_spi *spi = spi_master_get_devdata(dev->master);
 	struct mxs_ssp *ssp = &spi->ssp;
-	uint8_t bits_per_word;
-	uint32_t hz = 0;
+	const unsigned int hz = min(dev->max_speed_hz, t->speed_hz);
 
-	bits_per_word = dev->bits_per_word;
-	if (t && t->bits_per_word)
-		bits_per_word = t->bits_per_word;
-
-	if (bits_per_word != 8) {
-		dev_err(&dev->dev, "%s, unsupported bits_per_word=%d\n",
-					__func__, bits_per_word);
+	if (t->bits_per_word != 8) {
+		dev_err(&dev->dev, "Unsupported bits per word of %d\n",
+			t->bits_per_word);
 		return -EINVAL;
 	}
 
-	hz = dev->max_speed_hz;
-	if (t && t->speed_hz)
-		hz = min(hz, t->speed_hz);
 	if (hz == 0) {
-		dev_err(&dev->dev, "Cannot continue with zero clock\n");
+		dev_err(&dev->dev, "SPI clock rate of zero not possible\n");
 		return -EINVAL;
 	}
 
@@ -100,12 +92,12 @@ static int mxs_spi_setup_transfer(struct spi_device *dev,
 
 	writel(BM_SSP_CTRL0_LOCK_CS,
 		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+
 	writel(BF_SSP_CTRL1_SSP_MODE(BV_SSP_CTRL1_SSP_MODE__SPI) |
-		     BF_SSP_CTRL1_WORD_LENGTH
-		     (BV_SSP_CTRL1_WORD_LENGTH__EIGHT_BITS) |
-		     ((dev->mode & SPI_CPOL) ? BM_SSP_CTRL1_POLARITY : 0) |
-		     ((dev->mode & SPI_CPHA) ? BM_SSP_CTRL1_PHASE : 0),
-		     ssp->base + HW_SSP_CTRL1(ssp));
+	       BF_SSP_CTRL1_WORD_LENGTH(BV_SSP_CTRL1_WORD_LENGTH__EIGHT_BITS) |
+	       ((dev->mode & SPI_CPOL) ? BM_SSP_CTRL1_POLARITY : 0) |
+	       ((dev->mode & SPI_CPHA) ? BM_SSP_CTRL1_PHASE : 0),
+	       ssp->base + HW_SSP_CTRL1(ssp));
 
 	writel(0x0, ssp->base + HW_SSP_CMD0);
 	writel(0x0, ssp->base + HW_SSP_CMD1);
-- 
1.7.10.4

[PATCH V2 11/12] spi/mxs: Don't set clock for each xfer

[Trent Piepho]

mxs_spi_setup_transfer() would set the SSP SCK rate every time it was
called, which is before each transfer.  It is uncommon for the SCK rate to
change between transfers and this causes unnecessary reprogramming of the
clock registers.  Changed to only set the rate when it has changed.

This significantly speeds up short SPI messages, especially messages made
up of many transfers.  On an iMX287, using spidev with messages that
consist of 511 transfers of 4 bytes each at an SCK of 48 MHz, the
effective transfer rate more than doubles from about 290 KB/sec to 600
KB/sec.

Signed-off-by: Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-mxs.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index a7d5f85..12794ea 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -68,6 +68,7 @@
 struct mxs_spi {
 	struct mxs_ssp		ssp;
 	struct completion	c;
+	unsigned int		sck;	/* Rate requested (vs actual) */
 };
 
 static int mxs_spi_setup_transfer(struct spi_device *dev,
@@ -88,7 +89,19 @@ static int mxs_spi_setup_transfer(struct spi_device *dev,
 		return -EINVAL;
 	}
 
-	mxs_ssp_set_clk_rate(ssp, hz);
+	if (hz != spi->sck) {
+		mxs_ssp_set_clk_rate(ssp, hz);
+		/*
+		 * Save requested value, not actual value from ssp->clk_rate.
+		 * Otherwise we would set the rate again each trasfer when
+		 * actual is not quite the same as requested.
+		 */
+		spi->sck = hz;
+		/*
+		 * Perhaps we should return an error if the actual clock is
+		 * nowhere close to what was requested?
+		 */
+	}
 
 	writel(BM_SSP_CTRL0_LOCK_CS,
 		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
-- 
1.7.10.4


------------------------------------------------------------------------------
Own the Future-Intel(R) Level Up Game Demo Contest 2013
Rise to greatness in Intel's independent game demo contest. Compete 
for recognition, cash, and the chance to get your game on Steam. 
$5K grand prize plus 10 genre and skill prizes. Submit your demo 
by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2

[PATCH V2 11/12] spi/mxs: Don't set clock for each xfer

[Trent Piepho]

mxs_spi_setup_transfer() would set the SSP SCK rate every time it was
called, which is before each transfer.  It is uncommon for the SCK rate to
change between transfers and this causes unnecessary reprogramming of the
clock registers.  Changed to only set the rate when it has changed.

This significantly speeds up short SPI messages, especially messages made
up of many transfers.  On an iMX287, using spidev with messages that
consist of 511 transfers of 4 bytes each at an SCK of 48 MHz, the
effective transfer rate more than doubles from about 290 KB/sec to 600
KB/sec.

Signed-off-by: Trent Piepho <tpiepho@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/spi/spi-mxs.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index a7d5f85..12794ea 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -68,6 +68,7 @@
 struct mxs_spi {
 	struct mxs_ssp		ssp;
 	struct completion	c;
+	unsigned int		sck;	/* Rate requested (vs actual) */
 };
 
 static int mxs_spi_setup_transfer(struct spi_device *dev,
@@ -88,7 +89,19 @@ static int mxs_spi_setup_transfer(struct spi_device *dev,
 		return -EINVAL;
 	}
 
-	mxs_ssp_set_clk_rate(ssp, hz);
+	if (hz != spi->sck) {
+		mxs_ssp_set_clk_rate(ssp, hz);
+		/*
+		 * Save requested value, not actual value from ssp->clk_rate.
+		 * Otherwise we would set the rate again each trasfer when
+		 * actual is not quite the same as requested.
+		 */
+		spi->sck = hz;
+		/*
+		 * Perhaps we should return an error if the actual clock is
+		 * nowhere close to what was requested?
+		 */
+	}
 
 	writel(BM_SSP_CTRL0_LOCK_CS,
 		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
-- 
1.7.10.4

[PATCH V2 12/12] spi/mxs: Use u32 instead of uint32_t

[Trent Piepho]

It's consistent with all the other spi drivers that way.

Signed-off-by: Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-mxs.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 12794ea..aafd069 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -129,9 +129,9 @@ static int mxs_spi_setup(struct spi_device *dev)
 	return 0;
 }
 
-static uint32_t mxs_spi_cs_to_reg(unsigned cs)
+static u32 mxs_spi_cs_to_reg(unsigned cs)
 {
-	uint32_t select = 0;
+	u32 select = 0;
 
 	/*
 	 * i.MX28 Datasheet: 17.10.1: HW_SSP_CTRL0
@@ -153,7 +153,7 @@ static int mxs_ssp_wait(struct mxs_spi *spi, int offset, int mask, bool set)
 {
 	const unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT);
 	struct mxs_ssp *ssp = &spi->ssp;
-	uint32_t reg;
+	u32 reg;
 
 	do {
 		reg = readl_relaxed(ssp->base + offset);
@@ -197,11 +197,11 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi,
 	const int sgs = DIV_ROUND_UP(len, desc_len);
 	int sg_count;
 	int min, ret;
-	uint32_t ctrl0;
+	u32 ctrl0;
 	struct page *vm_page;
 	void *sg_buf;
 	struct {
-		uint32_t		pio[4];
+		u32			pio[4];
 		struct scatterlist	sg;
 	} *dma_xfer;
 
-- 
1.7.10.4


------------------------------------------------------------------------------
Own the Future-Intel(R) Level Up Game Demo Contest 2013
Rise to greatness in Intel's independent game demo contest. Compete 
for recognition, cash, and the chance to get your game on Steam. 
$5K grand prize plus 10 genre and skill prizes. Submit your demo 
by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2

[PATCH V2 12/12] spi/mxs: Use u32 instead of uint32_t

[Trent Piepho]

It's consistent with all the other spi drivers that way.

Signed-off-by: Trent Piepho <tpiepho@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/spi/spi-mxs.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 12794ea..aafd069 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -129,9 +129,9 @@ static int mxs_spi_setup(struct spi_device *dev)
 	return 0;
 }
 
-static uint32_t mxs_spi_cs_to_reg(unsigned cs)
+static u32 mxs_spi_cs_to_reg(unsigned cs)
 {
-	uint32_t select = 0;
+	u32 select = 0;
 
 	/*
 	 * i.MX28 Datasheet: 17.10.1: HW_SSP_CTRL0
@@ -153,7 +153,7 @@ static int mxs_ssp_wait(struct mxs_spi *spi, int offset, int mask, bool set)
 {
 	const unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT);
 	struct mxs_ssp *ssp = &spi->ssp;
-	uint32_t reg;
+	u32 reg;
 
 	do {
 		reg = readl_relaxed(ssp->base + offset);
@@ -197,11 +197,11 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi,
 	const int sgs = DIV_ROUND_UP(len, desc_len);
 	int sg_count;
 	int min, ret;
-	uint32_t ctrl0;
+	u32 ctrl0;
 	struct page *vm_page;
 	void *sg_buf;
 	struct {
-		uint32_t		pio[4];
+		u32			pio[4];
 		struct scatterlist	sg;
 	} *dma_xfer;
 
-- 
1.7.10.4

Re: [PATCH V2 01/12] spi/mxs: Always set LOCK_CS

[Marek Vasut]

Dear Trent Piepho,

> There are two bits which control the CS line in the CTRL0 register:
> LOCK_CS and IGNORE_CRC.  The latter would be better named DEASSERT_CS
> in SPI mode.
> 
> LOCK_CS keeps CS asserted though the entire transfer.  This should
> always be set.  The DMA code will always set it, explicitly on the
> first segment of the first transfer, and then implicitly on all the
> rest by never clearing the bit from the value read from the ctrl0
> register.
> 
> The only reason to not set LOCK_CS would be to attempt an altered
> protocol where CS pulses between each word.  Though don't get your
> hopes up if you want to do this, as the hardware doesn't appear to do
> this in any sane manner.

Can you please elaborate on this part above? The description is very vague.

Fabio, can you review this too please?

> The code can be simplified by just setting LOCK_CS once and then not
> needing to deal with it in the PIO and DMA transfer functions.
> 
> Signed-off-by: Trent Piepho <tpiepho@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/spi/spi-mxs.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> index 22a0af0..9334167 100644
> --- a/drivers/spi/spi-mxs.c
> +++ b/drivers/spi/spi-mxs.c
> @@ -91,6 +91,8 @@ static int mxs_spi_setup_transfer(struct spi_device *dev,
> 
>  	mxs_ssp_set_clk_rate(ssp, hz);
> 
> +	writel(BM_SSP_CTRL0_LOCK_CS,
> +		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
>  	writel(BF_SSP_CTRL1_SSP_MODE(BV_SSP_CTRL1_SSP_MODE__SPI) |
>  		     BF_SSP_CTRL1_WORD_LENGTH
>  		     (BV_SSP_CTRL1_WORD_LENGTH__EIGHT_BITS) |
> @@ -159,8 +161,6 @@ static inline void mxs_spi_enable(struct mxs_spi *spi)
>  {
>  	struct mxs_ssp *ssp = &spi->ssp;
> 
> -	writel(BM_SSP_CTRL0_LOCK_CS,
> -		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
>  	writel(BM_SSP_CTRL0_IGNORE_CRC,
>  		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
>  }
> @@ -169,8 +169,6 @@ static inline void mxs_spi_disable(struct mxs_spi *spi)
>  {
>  	struct mxs_ssp *ssp = &spi->ssp;
> 
> -	writel(BM_SSP_CTRL0_LOCK_CS,
> -		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
>  	writel(BM_SSP_CTRL0_IGNORE_CRC,
>  		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
>  }
> @@ -244,8 +242,6 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int
> cs, ctrl0 &= ~BM_SSP_CTRL0_XFER_COUNT;
>  	ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
> 
> -	if (*first)
> -		ctrl0 |= BM_SSP_CTRL0_LOCK_CS;
>  	if (!write)
>  		ctrl0 |= BM_SSP_CTRL0_READ;

Best regards,
Marek Vasut

[PATCH V2 01/12] spi/mxs: Always set LOCK_CS

[Marek Vasut]

Dear Trent Piepho,

> There are two bits which control the CS line in the CTRL0 register:
> LOCK_CS and IGNORE_CRC.  The latter would be better named DEASSERT_CS
> in SPI mode.
> 
> LOCK_CS keeps CS asserted though the entire transfer.  This should
> always be set.  The DMA code will always set it, explicitly on the
> first segment of the first transfer, and then implicitly on all the
> rest by never clearing the bit from the value read from the ctrl0
> register.
> 
> The only reason to not set LOCK_CS would be to attempt an altered
> protocol where CS pulses between each word.  Though don't get your
> hopes up if you want to do this, as the hardware doesn't appear to do
> this in any sane manner.

Can you please elaborate on this part above? The description is very vague.

Fabio, can you review this too please?

> The code can be simplified by just setting LOCK_CS once and then not
> needing to deal with it in the PIO and DMA transfer functions.
> 
> Signed-off-by: Trent Piepho <tpiepho@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/spi/spi-mxs.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> index 22a0af0..9334167 100644
> --- a/drivers/spi/spi-mxs.c
> +++ b/drivers/spi/spi-mxs.c
> @@ -91,6 +91,8 @@ static int mxs_spi_setup_transfer(struct spi_device *dev,
> 
>  	mxs_ssp_set_clk_rate(ssp, hz);
> 
> +	writel(BM_SSP_CTRL0_LOCK_CS,
> +		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
>  	writel(BF_SSP_CTRL1_SSP_MODE(BV_SSP_CTRL1_SSP_MODE__SPI) |
>  		     BF_SSP_CTRL1_WORD_LENGTH
>  		     (BV_SSP_CTRL1_WORD_LENGTH__EIGHT_BITS) |
> @@ -159,8 +161,6 @@ static inline void mxs_spi_enable(struct mxs_spi *spi)
>  {
>  	struct mxs_ssp *ssp = &spi->ssp;
> 
> -	writel(BM_SSP_CTRL0_LOCK_CS,
> -		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
>  	writel(BM_SSP_CTRL0_IGNORE_CRC,
>  		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
>  }
> @@ -169,8 +169,6 @@ static inline void mxs_spi_disable(struct mxs_spi *spi)
>  {
>  	struct mxs_ssp *ssp = &spi->ssp;
> 
> -	writel(BM_SSP_CTRL0_LOCK_CS,
> -		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
>  	writel(BM_SSP_CTRL0_IGNORE_CRC,
>  		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
>  }
> @@ -244,8 +242,6 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int
> cs, ctrl0 &= ~BM_SSP_CTRL0_XFER_COUNT;
>  	ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
> 
> -	if (*first)
> -		ctrl0 |= BM_SSP_CTRL0_LOCK_CS;
>  	if (!write)
>  		ctrl0 |= BM_SSP_CTRL0_READ;

Best regards,
Marek Vasut

Re: [PATCH V2 03/12] spi/mxs: Change flag arguments in txrx functions to bit flags

[Marek Vasut]

Dear Trent Piepho,

> There are three flag arguments to the PIO and DMA txrx functions.  Two
> are passed as pointers to integers, even though they are input only
> and not modified, which makes no sense to do.  The third is passed as
> an integer.
> 
> The compiler must use an argument register or stack variable for each
> flag this way.  By using bitflags in a single flag argument more
> efficient and smaller code is produced since all the flags can fit in
> a single register.  And all the flag arguments get cumbersome,
> especially when more are added for things like GPIO chipselects.
> 
> The "first" flag is never used, so can just be deleted.
> 
> The "last" flag is renamed to DEASSERT_CS, since that's really what it
> does.  The spi_transfer cs_change flag means that CS might be
> de-asserted on a transfer which is not last and not de-assert on the
> last transfer, so it is not which transfer is the last we need to know
> but rather the transfers which after which CS should be de-asserted.
> 
> This also extends the driver to not ignore cs_change when setting the
> DEASSERT_CS nee "last" flag.
> 
> Signed-off-by: Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> Cc: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/spi/spi-mxs.c |   55
> ++++++++++++++++++++++++++++--------------------- 1 file changed, 31
> insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> index 7eb4bc9..3064304 100644
> --- a/drivers/spi/spi-mxs.c
> +++ b/drivers/spi/spi-mxs.c
> @@ -58,6 +58,13 @@
> 
>  #define SG_MAXLEN		0xff00
> 
> +/*
> + * Flags for txrx functions.  More efficient that using an argument
> register for + * each one.
> + */
> +#define TXRX_WRITE		1	/* This is a write */
> +#define TXRX_DEASSERT_CS	2	/* De-assert CS at end of txrx */
> +

Use (1 << n) here as these are bitfield flags.

[...]

> @@ -409,10 +418,9 @@ static int mxs_spi_transfer_one(struct spi_master
> *master, if (status)
>  			break;
> 
> -		if (&t->transfer_list == m->transfers.next)
> -			first = 1;
> -		if (&t->transfer_list == m->transfers.prev)
> -			last = 1;
> +		/* De-assert on last transfer, inverted by cs_change flag */
> +		flag = (&t->transfer_list == m->transfers.prev) ^ t->cs_change ?
> +		       TXRX_DEASSERT_CS : 0;

Make this into an if-else block, this really is hard to parse at first glance.

>  		if ((t->rx_buf && t->tx_buf) || (t->rx_dma && t->tx_dma)) {
>  			dev_err(ssp->dev,
>  				"Cannot send and receive simultaneously\n");
[...]

------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html

[PATCH V2 03/12] spi/mxs: Change flag arguments in txrx functions to bit flags

[Marek Vasut]

Dear Trent Piepho,

> There are three flag arguments to the PIO and DMA txrx functions.  Two
> are passed as pointers to integers, even though they are input only
> and not modified, which makes no sense to do.  The third is passed as
> an integer.
> 
> The compiler must use an argument register or stack variable for each
> flag this way.  By using bitflags in a single flag argument more
> efficient and smaller code is produced since all the flags can fit in
> a single register.  And all the flag arguments get cumbersome,
> especially when more are added for things like GPIO chipselects.
> 
> The "first" flag is never used, so can just be deleted.
> 
> The "last" flag is renamed to DEASSERT_CS, since that's really what it
> does.  The spi_transfer cs_change flag means that CS might be
> de-asserted on a transfer which is not last and not de-assert on the
> last transfer, so it is not which transfer is the last we need to know
> but rather the transfers which after which CS should be de-asserted.
> 
> This also extends the driver to not ignore cs_change when setting the
> DEASSERT_CS nee "last" flag.
> 
> Signed-off-by: Trent Piepho <tpiepho@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/spi/spi-mxs.c |   55
> ++++++++++++++++++++++++++++--------------------- 1 file changed, 31
> insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> index 7eb4bc9..3064304 100644
> --- a/drivers/spi/spi-mxs.c
> +++ b/drivers/spi/spi-mxs.c
> @@ -58,6 +58,13 @@
> 
>  #define SG_MAXLEN		0xff00
> 
> +/*
> + * Flags for txrx functions.  More efficient that using an argument
> register for + * each one.
> + */
> +#define TXRX_WRITE		1	/* This is a write */
> +#define TXRX_DEASSERT_CS	2	/* De-assert CS at end of txrx */
> +

Use (1 << n) here as these are bitfield flags.

[...]

> @@ -409,10 +418,9 @@ static int mxs_spi_transfer_one(struct spi_master
> *master, if (status)
>  			break;
> 
> -		if (&t->transfer_list == m->transfers.next)
> -			first = 1;
> -		if (&t->transfer_list == m->transfers.prev)
> -			last = 1;
> +		/* De-assert on last transfer, inverted by cs_change flag */
> +		flag = (&t->transfer_list == m->transfers.prev) ^ t->cs_change ?
> +		       TXRX_DEASSERT_CS : 0;

Make this into an if-else block, this really is hard to parse at first glance.

>  		if ((t->rx_buf && t->tx_buf) || (t->rx_dma && t->tx_dma)) {
>  			dev_err(ssp->dev,
>  				"Cannot send and receive simultaneously\n");
[...]

Re: [PATCH V2 05/12] spi/mxs: Fix chip select control bits in DMA mode

[Marek Vasut]

Dear Trent Piepho,

> In DMA mode the chip select control bits would be ORed into the CTRL0
> register without first clearing the bits.  This means that after
> addressing slave 1 the bit would be still be set when addressing slave
> 0, resulting in slave 1 continuing to be addressed.
> 
> The message handing function would pass the cs value to the txrx
> function, which would re-program the bits on each transfer in the
> message.  The selected cs does not change during a message so this is
> inefficient.  It also means there are two different sets of code for
> selecting the CS, one for PIO that worked and one for DMA that didn't.
> 
> Change the code to set the CS bits in the message transfer function
> once.  Now the DMA and PIO txrx functions don't need to care about CS
> at all.
> 
> Signed-off-by: Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> Cc: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

[...]

> @@ -409,9 +396,12 @@ static int mxs_spi_transfer_one(struct spi_master
> *master, struct spi_transfer *t, *tmp_t;
>  	unsigned int flag;
>  	int status = 0;
> -	int cs;
> 
> -	cs = m->spi->chip_select;
> +	/* Program CS register bits here, it will be used for all transfers. */
> +	writel(BM_SSP_CTRL0_WAIT_FOR_CMD | BM_SSP_CTRL0_WAIT_FOR_IRQ,
> +	       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
> +	writel(mxs_spi_cs_to_reg(m->spi->chip_select),
> +	       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);

I have no problem with this patch in general, but put this stuff back into a 
function with proper description stating that these bits are not what their name 
means. Having this stuff in the code just like that looks like hell and is 
absolutely not clear about what it really does. Besides, having such things well 
encapsulated makes further hacking on/maintainance of the driver much easier.

>  	list_for_each_entry_safe(t, tmp_t, &m->transfers, transfer_list) {
> 

[...]
Best regards,
Marek Vasut

------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html

[PATCH V2 05/12] spi/mxs: Fix chip select control bits in DMA mode

[Marek Vasut]

Dear Trent Piepho,

> In DMA mode the chip select control bits would be ORed into the CTRL0
> register without first clearing the bits.  This means that after
> addressing slave 1 the bit would be still be set when addressing slave
> 0, resulting in slave 1 continuing to be addressed.
> 
> The message handing function would pass the cs value to the txrx
> function, which would re-program the bits on each transfer in the
> message.  The selected cs does not change during a message so this is
> inefficient.  It also means there are two different sets of code for
> selecting the CS, one for PIO that worked and one for DMA that didn't.
> 
> Change the code to set the CS bits in the message transfer function
> once.  Now the DMA and PIO txrx functions don't need to care about CS
> at all.
> 
> Signed-off-by: Trent Piepho <tpiepho@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>

[...]

> @@ -409,9 +396,12 @@ static int mxs_spi_transfer_one(struct spi_master
> *master, struct spi_transfer *t, *tmp_t;
>  	unsigned int flag;
>  	int status = 0;
> -	int cs;
> 
> -	cs = m->spi->chip_select;
> +	/* Program CS register bits here, it will be used for all transfers. */
> +	writel(BM_SSP_CTRL0_WAIT_FOR_CMD | BM_SSP_CTRL0_WAIT_FOR_IRQ,
> +	       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
> +	writel(mxs_spi_cs_to_reg(m->spi->chip_select),
> +	       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);

I have no problem with this patch in general, but put this stuff back into a 
function with proper description stating that these bits are not what their name 
means. Having this stuff in the code just like that looks like hell and is 
absolutely not clear about what it really does. Besides, having such things well 
encapsulated makes further hacking on/maintainance of the driver much easier.

>  	list_for_each_entry_safe(t, tmp_t, &m->transfers, transfer_list) {
> 

[...]
Best regards,
Marek Vasut

Re: [PATCH V2 08/12] spi/mxs: Fix race in setup method

[Marek Vasut]

Dear Trent Piepho,

> Despite many warnings in the SPI documentation and code, the spi-mxs
> driver sets shared chip registers in the ->setup method.  This method can
> be called when transfers are in progress on other slaves controlled by the
> master.  Setting registers or any other shared state will corrupt those
> transfers.
> 
> So fix mxs_spi_setup() to not call mxs_spi_setup_transfer().
> 
> Signed-off-by: Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> Cc: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/spi/spi-mxs.c |   11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> index 103c478..2a2147a 100644
> --- a/drivers/spi/spi-mxs.c
> +++ b/drivers/spi/spi-mxs.c
> @@ -115,21 +115,16 @@ static int mxs_spi_setup_transfer(struct spi_device
> *dev,
> 
>  static int mxs_spi_setup(struct spi_device *dev)
>  {
> -	int err = 0;
> -
>  	if (!dev->bits_per_word)
>  		dev->bits_per_word = 8;
> 
>  	if (dev->mode & ~(SPI_CPOL | SPI_CPHA))
>  		return -EINVAL;
> 
> -	err = mxs_spi_setup_transfer(dev, NULL);
> -	if (err) {
> -		dev_err(&dev->dev,
> -			"Failed to setup transfer, error = %d\n", err);
> -	}
> +	if (dev->bits_per_word != 8)
> +		return -EINVAL;

What's this new addition doing here?

btw. I was under the impression the MXS SSP block can handle other word-widths 
than 8 bit, no ?

Best regards,
Marek Vasut

------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html

[PATCH V2 08/12] spi/mxs: Fix race in setup method

[Marek Vasut]

Dear Trent Piepho,

> Despite many warnings in the SPI documentation and code, the spi-mxs
> driver sets shared chip registers in the ->setup method.  This method can
> be called when transfers are in progress on other slaves controlled by the
> master.  Setting registers or any other shared state will corrupt those
> transfers.
> 
> So fix mxs_spi_setup() to not call mxs_spi_setup_transfer().
> 
> Signed-off-by: Trent Piepho <tpiepho@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/spi/spi-mxs.c |   11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> index 103c478..2a2147a 100644
> --- a/drivers/spi/spi-mxs.c
> +++ b/drivers/spi/spi-mxs.c
> @@ -115,21 +115,16 @@ static int mxs_spi_setup_transfer(struct spi_device
> *dev,
> 
>  static int mxs_spi_setup(struct spi_device *dev)
>  {
> -	int err = 0;
> -
>  	if (!dev->bits_per_word)
>  		dev->bits_per_word = 8;
> 
>  	if (dev->mode & ~(SPI_CPOL | SPI_CPHA))
>  		return -EINVAL;
> 
> -	err = mxs_spi_setup_transfer(dev, NULL);
> -	if (err) {
> -		dev_err(&dev->dev,
> -			"Failed to setup transfer, error = %d\n", err);
> -	}
> +	if (dev->bits_per_word != 8)
> +		return -EINVAL;

What's this new addition doing here?

btw. I was under the impression the MXS SSP block can handle other word-widths 
than 8 bit, no ?

Best regards,
Marek Vasut

Re: [PATCH V2 10/12] spi/mxs: Clean up setup_transfer function

[Marek Vasut]

Dear Trent Piepho,

> It can't be called with a NULL transfer anymore so it can be
> simplified to not check for that.
> 
> Change printouts to more closely match what the spi core would print.
> I.e., don't print function names.  Try to make them clearer.
> 
> Fix indention of line-wrapped code to Linux standard.
> 
> The transfer pointer can be const.
> 
> It's not necessary to check if the spi_transfer's bit per word is
> zero.  The spi core automatically inserts the spi_device's bits per
> word in that case.
> 
> It's not necessary to check if the spi_transfer's speed_hz is zero, as
> the spi core also fills it in from the spi_device.  However, the spi
> core does not check if spi_device's speed is zero so we have to do
> that still.
> 
> Signed-off-by: Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> Cc: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Ok, this is again combining cleanup and fixup/feature changes. You know what to 
do by now.

btw it might be nice to reorder the series so that cleanups go in first as 
they're easy and fixes afterwards.

Best regards,
Marek Vasut

------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html

[PATCH V2 10/12] spi/mxs: Clean up setup_transfer function

[Marek Vasut]

Dear Trent Piepho,

> It can't be called with a NULL transfer anymore so it can be
> simplified to not check for that.
> 
> Change printouts to more closely match what the spi core would print.
> I.e., don't print function names.  Try to make them clearer.
> 
> Fix indention of line-wrapped code to Linux standard.
> 
> The transfer pointer can be const.
> 
> It's not necessary to check if the spi_transfer's bit per word is
> zero.  The spi core automatically inserts the spi_device's bits per
> word in that case.
> 
> It's not necessary to check if the spi_transfer's speed_hz is zero, as
> the spi core also fills it in from the spi_device.  However, the spi
> core does not check if spi_device's speed is zero so we have to do
> that still.
> 
> Signed-off-by: Trent Piepho <tpiepho@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>

Ok, this is again combining cleanup and fixup/feature changes. You know what to 
do by now.

btw it might be nice to reorder the series so that cleanups go in first as 
they're easy and fixes afterwards.

Best regards,
Marek Vasut

Re: [PATCH V2 11/12] spi/mxs: Don't set clock for each xfer

[Marek Vasut]

Dear Trent Piepho,

> mxs_spi_setup_transfer() would set the SSP SCK rate every time it was
> called, which is before each transfer.  It is uncommon for the SCK rate to
> change between transfers and this causes unnecessary reprogramming of the
> clock registers.  Changed to only set the rate when it has changed.
> 
> This significantly speeds up short SPI messages, especially messages made
> up of many transfers.  On an iMX287, using spidev with messages that
> consist of 511 transfers of 4 bytes each at an SCK of 48 MHz, the
> effective transfer rate more than doubles from about 290 KB/sec to 600
> KB/sec.
> 
> Signed-off-by: Trent Piepho <tpiepho-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> Cc: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/spi/spi-mxs.c |   15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> index a7d5f85..12794ea 100644
> --- a/drivers/spi/spi-mxs.c
> +++ b/drivers/spi/spi-mxs.c
> @@ -68,6 +68,7 @@
>  struct mxs_spi {
>  	struct mxs_ssp		ssp;
>  	struct completion	c;

Stick the comment here.

> +	unsigned int		sck;	/* Rate requested (vs actual) */
>  };
> 
>  static int mxs_spi_setup_transfer(struct spi_device *dev,
> @@ -88,7 +89,19 @@ static int mxs_spi_setup_transfer(struct spi_device
> *dev, return -EINVAL;
>  	}
> 
> -	mxs_ssp_set_clk_rate(ssp, hz);
> +	if (hz != spi->sck) {
> +		mxs_ssp_set_clk_rate(ssp, hz);
> +		/*
> +		 * Save requested value, not actual value from ssp->clk_rate.
> +		 * Otherwise we would set the rate again each trasfer when
> +		 * actual is not quite the same as requested.
> +		 */
> +		spi->sck = hz;
> +		/*
> +		 * Perhaps we should return an error if the actual clock is
> +		 * nowhere close to what was requested?
> +		 */

Maybe you want to check the ssp->ssp_clk vs. hz indeed.

> +	}
> 
>  	writel(BM_SSP_CTRL0_LOCK_CS,
>  		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);

Best regards,
Marek Vasut

------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html

[PATCH V2 11/12] spi/mxs: Don't set clock for each xfer

[Marek Vasut]

Dear Trent Piepho,

> mxs_spi_setup_transfer() would set the SSP SCK rate every time it was
> called, which is before each transfer.  It is uncommon for the SCK rate to
> change between transfers and this causes unnecessary reprogramming of the
> clock registers.  Changed to only set the rate when it has changed.
> 
> This significantly speeds up short SPI messages, especially messages made
> up of many transfers.  On an iMX287, using spidev with messages that
> consist of 511 transfers of 4 bytes each at an SCK of 48 MHz, the
> effective transfer rate more than doubles from about 290 KB/sec to 600
> KB/sec.
> 
> Signed-off-by: Trent Piepho <tpiepho@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/spi/spi-mxs.c |   15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> index a7d5f85..12794ea 100644
> --- a/drivers/spi/spi-mxs.c
> +++ b/drivers/spi/spi-mxs.c
> @@ -68,6 +68,7 @@
>  struct mxs_spi {
>  	struct mxs_ssp		ssp;
>  	struct completion	c;

Stick the comment here.

> +	unsigned int		sck;	/* Rate requested (vs actual) */
>  };
> 
>  static int mxs_spi_setup_transfer(struct spi_device *dev,
> @@ -88,7 +89,19 @@ static int mxs_spi_setup_transfer(struct spi_device
> *dev, return -EINVAL;
>  	}
> 
> -	mxs_ssp_set_clk_rate(ssp, hz);
> +	if (hz != spi->sck) {
> +		mxs_ssp_set_clk_rate(ssp, hz);
> +		/*
> +		 * Save requested value, not actual value from ssp->clk_rate.
> +		 * Otherwise we would set the rate again each trasfer when
> +		 * actual is not quite the same as requested.
> +		 */
> +		spi->sck = hz;
> +		/*
> +		 * Perhaps we should return an error if the actual clock is
> +		 * nowhere close to what was requested?
> +		 */

Maybe you want to check the ssp->ssp_clk vs. hz indeed.

> +	}
> 
>  	writel(BM_SSP_CTRL0_LOCK_CS,
>  		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);

Best regards,
Marek Vasut

Re: [PATCH V2 01/12] spi/mxs: Always set LOCK_CS

[Shawn Guo]

On Tue, Apr 02, 2013 at 05:19:44AM -0700, Trent Piepho wrote:
> There are two bits which control the CS line in the CTRL0 register:
> LOCK_CS and IGNORE_CRC.  The latter would be better named DEASSERT_CS
> in SPI mode.
> 
> LOCK_CS keeps CS asserted though the entire transfer.  This should
> always be set.  The DMA code will always set it, explicitly on the
> first segment of the first transfer, and then implicitly on all the
> rest by never clearing the bit from the value read from the ctrl0
> register.
> 
> The only reason to not set LOCK_CS would be to attempt an altered
> protocol where CS pulses between each word.  Though don't get your
> hopes up if you want to do this, as the hardware doesn't appear to do
> this in any sane manner.
> 
> The code can be simplified by just setting LOCK_CS once and then not
> needing to deal with it in the PIO and DMA transfer functions.
> 
> Signed-off-by: Trent Piepho <tpiepho@gmail.com>

Trent,

Just FYI, Mark Brown is taking care of SPI subsystem patches recently.
Please remember copy him on the series when you repost, if you want to
see the series get applied eventually :)

Shawn

[PATCH V2 01/12] spi/mxs: Always set LOCK_CS

[Shawn Guo]

On Tue, Apr 02, 2013 at 05:19:44AM -0700, Trent Piepho wrote:
> There are two bits which control the CS line in the CTRL0 register:
> LOCK_CS and IGNORE_CRC.  The latter would be better named DEASSERT_CS
> in SPI mode.
> 
> LOCK_CS keeps CS asserted though the entire transfer.  This should
> always be set.  The DMA code will always set it, explicitly on the
> first segment of the first transfer, and then implicitly on all the
> rest by never clearing the bit from the value read from the ctrl0
> register.
> 
> The only reason to not set LOCK_CS would be to attempt an altered
> protocol where CS pulses between each word.  Though don't get your
> hopes up if you want to do this, as the hardware doesn't appear to do
> this in any sane manner.
> 
> The code can be simplified by just setting LOCK_CS once and then not
> needing to deal with it in the PIO and DMA transfer functions.
> 
> Signed-off-by: Trent Piepho <tpiepho@gmail.com>

Trent,

Just FYI, Mark Brown is taking care of SPI subsystem patches recently.
Please remember copy him on the series when you repost, if you want to
see the series get applied eventually :)

Shawn

Re: [PATCH V2 08/12] spi/mxs: Fix race in setup method

[Trent Piepho]

On Tue, Apr 2, 2013 at 4:31 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>>  static int mxs_spi_setup(struct spi_device *dev)
>>  {
>> -     int err = 0;
>> -
>>       if (!dev->bits_per_word)
>>               dev->bits_per_word = 8;
>>
>>       if (dev->mode & ~(SPI_CPOL | SPI_CPHA))
>>               return -EINVAL;
>>
>> -     err = mxs_spi_setup_transfer(dev, NULL);
>> -     if (err) {
>> -             dev_err(&dev->dev,
>> -                     "Failed to setup transfer, error = %d\n", err);
>> -     }
>> +     if (dev->bits_per_word != 8)
>> +             return -EINVAL;
>
> What's this new addition doing here?

It's not new.  The only useful thing mxs_spi_setup_transfer() (which
is no longer called) did in this instance was make that check.

> btw. I was under the impression the MXS SSP block can handle other word-widths
> than 8 bit, no ?

In theory it can, however the driver only supports 8-bits and will
reject anything else.

------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html

[PATCH V2 08/12] spi/mxs: Fix race in setup method

[Trent Piepho]

On Tue, Apr 2, 2013 at 4:31 PM, Marek Vasut <marex@denx.de> wrote:
>>  static int mxs_spi_setup(struct spi_device *dev)
>>  {
>> -     int err = 0;
>> -
>>       if (!dev->bits_per_word)
>>               dev->bits_per_word = 8;
>>
>>       if (dev->mode & ~(SPI_CPOL | SPI_CPHA))
>>               return -EINVAL;
>>
>> -     err = mxs_spi_setup_transfer(dev, NULL);
>> -     if (err) {
>> -             dev_err(&dev->dev,
>> -                     "Failed to setup transfer, error = %d\n", err);
>> -     }
>> +     if (dev->bits_per_word != 8)
>> +             return -EINVAL;
>
> What's this new addition doing here?

It's not new.  The only useful thing mxs_spi_setup_transfer() (which
is no longer called) did in this instance was make that check.

> btw. I was under the impression the MXS SSP block can handle other word-widths
> than 8 bit, no ?

In theory it can, however the driver only supports 8-bits and will
reject anything else.

Re: [PATCH V2 08/12] spi/mxs: Fix race in setup method

[Marek Vasut]

Dear Trent Piepho,

> On Tue, Apr 2, 2013 at 4:31 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> >>  static int mxs_spi_setup(struct spi_device *dev)
> >>  {
> >> 
> >> -     int err = 0;
> >> -
> >> 
> >>       if (!dev->bits_per_word)
> >>       
> >>               dev->bits_per_word = 8;
> >>       
> >>       if (dev->mode & ~(SPI_CPOL | SPI_CPHA))
> >>       
> >>               return -EINVAL;
> >> 
> >> -     err = mxs_spi_setup_transfer(dev, NULL);
> >> -     if (err) {
> >> -             dev_err(&dev->dev,
> >> -                     "Failed to setup transfer, error = %d\n", err);
> >> -     }
> >> +     if (dev->bits_per_word != 8)
> >> +             return -EINVAL;
> > 
> > What's this new addition doing here?
> 
> It's not new.

It is new in the context of this patch and it's not described in the commit 
message.

> The only useful thing mxs_spi_setup_transfer() (which
> is no longer called) did in this instance was make that check.
> 
> > btw. I was under the impression the MXS SSP block can handle other
> > word-widths than 8 bit, no ?
> 
> In theory it can,

In practice too ;-)

> however the driver only supports 8-bits and will
> reject anything else.

Then please at least add a comment about this.

Best regards,
Marek Vasut

------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html

[PATCH V2 08/12] spi/mxs: Fix race in setup method

[Marek Vasut]

Dear Trent Piepho,

> On Tue, Apr 2, 2013 at 4:31 PM, Marek Vasut <marex@denx.de> wrote:
> >>  static int mxs_spi_setup(struct spi_device *dev)
> >>  {
> >> 
> >> -     int err = 0;
> >> -
> >> 
> >>       if (!dev->bits_per_word)
> >>       
> >>               dev->bits_per_word = 8;
> >>       
> >>       if (dev->mode & ~(SPI_CPOL | SPI_CPHA))
> >>       
> >>               return -EINVAL;
> >> 
> >> -     err = mxs_spi_setup_transfer(dev, NULL);
> >> -     if (err) {
> >> -             dev_err(&dev->dev,
> >> -                     "Failed to setup transfer, error = %d\n", err);
> >> -     }
> >> +     if (dev->bits_per_word != 8)
> >> +             return -EINVAL;
> > 
> > What's this new addition doing here?
> 
> It's not new.

It is new in the context of this patch and it's not described in the commit 
message.

> The only useful thing mxs_spi_setup_transfer() (which
> is no longer called) did in this instance was make that check.
> 
> > btw. I was under the impression the MXS SSP block can handle other
> > word-widths than 8 bit, no ?
> 
> In theory it can,

In practice too ;-)

> however the driver only supports 8-bits and will
> reject anything else.

Then please at least add a comment about this.

Best regards,
Marek Vasut

Re: [PATCH V2 08/12] spi/mxs: Fix race in setup method

[Trent Piepho]

On Apr 2, 2013 7:27 PM, "Marek Vasut" <marex-ynQEQJNshbs@public.gmane.org> wrote:
>
> > The only useful thing mxs_spi_setup_transfer() (which
> > is no longer called) did in this instance was make that check.
> >
> > > btw. I was under the impression the MXS SSP block can handle other
> > > word-widths than 8 bit, no ?
> >
> > In theory it can,
>
> In practice too ;-)
>
> > however the driver only supports 8-bits and will
> > reject anything else.
>
> Then please at least add a comment about this.

What does that have to do with this patch?  There was no comment about
it before.  You're insisting that changes be broken up to the extent
that changing one line of code takes multiple patches!  Now you want a
comment about existing driver behavior added to patch that doesn't
change that behavior?

Documenting the driver behavior would seem to be a job for the driver
maintainer.  If doing this is something I need to do, then maybe I
should maintain it?

------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html

[PATCH V2 08/12] spi/mxs: Fix race in setup method

[Trent Piepho]

On Apr 2, 2013 7:27 PM, "Marek Vasut" <marex@denx.de> wrote:
>
> > The only useful thing mxs_spi_setup_transfer() (which
> > is no longer called) did in this instance was make that check.
> >
> > > btw. I was under the impression the MXS SSP block can handle other
> > > word-widths than 8 bit, no ?
> >
> > In theory it can,
>
> In practice too ;-)
>
> > however the driver only supports 8-bits and will
> > reject anything else.
>
> Then please at least add a comment about this.

What does that have to do with this patch?  There was no comment about
it before.  You're insisting that changes be broken up to the extent
that changing one line of code takes multiple patches!  Now you want a
comment about existing driver behavior added to patch that doesn't
change that behavior?

Documenting the driver behavior would seem to be a job for the driver
maintainer.  If doing this is something I need to do, then maybe I
should maintain it?

Re: [PATCH V2 08/12] spi/mxs: Fix race in setup method

[Marek Vasut]

Dear Trent Piepho,

> On Apr 2, 2013 7:27 PM, "Marek Vasut" <marex-ynQEQJNshbs@public.gmane.org> wrote:
> > > The only useful thing mxs_spi_setup_transfer() (which
> > > is no longer called) did in this instance was make that check.
> > > 
> > > > btw. I was under the impression the MXS SSP block can handle other
> > > > word-widths than 8 bit, no ?
> > > 
> > > In theory it can,
> > 
> > In practice too ;-)
> > 
> > > however the driver only supports 8-bits and will
> > > reject anything else.
> > 
> > Then please at least add a comment about this.
> 
> What does that have to do with this patch?

You're adding a piece of code. This change is
a) not documented in the commit message
b) unrelated to this fix, seems to be more fitting in 10/12 or something -- 
maybe even shuffling the patches a bit might help

So add a comment to make this clearer for people who hack on this after you.

> There was no comment about
> it before.  You're insisting that changes be broken up to the extent
> that changing one line of code takes multiple patches!  Now you want a
> comment about existing driver behavior added to patch that doesn't
> change that behavior?
> 
> Documenting the driver behavior would seem to be a job for the driver
> maintainer.  If doing this is something I need to do, then maybe I
> should maintain it?

You ain't rubbing people the right way at all, I tell you. This pushy/offensive 
behavior helps noone, calm down please. It is usually a good idea to sleep on 
your reply or take your time.

Ad maintainership -- there's no maintainer of the driver, there's only Fabio, 
Shawn, me and the SPI subsystem guys. And it works well. It's everybodys' code 
and everyone's welcome to contribute. What's the problem? 

Best regards,
Marek Vasut

------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html

[PATCH V2 08/12] spi/mxs: Fix race in setup method

[Marek Vasut]

Dear Trent Piepho,

> On Apr 2, 2013 7:27 PM, "Marek Vasut" <marex@denx.de> wrote:
> > > The only useful thing mxs_spi_setup_transfer() (which
> > > is no longer called) did in this instance was make that check.
> > > 
> > > > btw. I was under the impression the MXS SSP block can handle other
> > > > word-widths than 8 bit, no ?
> > > 
> > > In theory it can,
> > 
> > In practice too ;-)
> > 
> > > however the driver only supports 8-bits and will
> > > reject anything else.
> > 
> > Then please at least add a comment about this.
> 
> What does that have to do with this patch?

You're adding a piece of code. This change is
a) not documented in the commit message
b) unrelated to this fix, seems to be more fitting in 10/12 or something -- 
maybe even shuffling the patches a bit might help

So add a comment to make this clearer for people who hack on this after you.

> There was no comment about
> it before.  You're insisting that changes be broken up to the extent
> that changing one line of code takes multiple patches!  Now you want a
> comment about existing driver behavior added to patch that doesn't
> change that behavior?
> 
> Documenting the driver behavior would seem to be a job for the driver
> maintainer.  If doing this is something I need to do, then maybe I
> should maintain it?

You ain't rubbing people the right way at all, I tell you. This pushy/offensive 
behavior helps noone, calm down please. It is usually a good idea to sleep on 
your reply or take your time.

Ad maintainership -- there's no maintainer of the driver, there's only Fabio, 
Shawn, me and the SPI subsystem guys. And it works well. It's everybodys' code 
and everyone's welcome to contribute. What's the problem? 

Best regards,
Marek Vasut

Re: [PATCH V2 08/12] spi/mxs: Fix race in setup method

[Trent Piepho]

On Tue, Apr 2, 2013 at 11:50 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> Dear Trent Piepho,
>
>> On Apr 2, 2013 7:27 PM, "Marek Vasut" <marex-ynQEQJNshbs@public.gmane.org> wrote:
>> > > The only useful thing mxs_spi_setup_transfer() (which
>> > > is no longer called) did in this instance was make that check.
>> > >
>> > > > btw. I was under the impression the MXS SSP block can handle other
>> > > > word-widths than 8 bit, no ?
>> > >
>> > > In theory it can,
>> >
>> > In practice too ;-)
>> >
>> > > however the driver only supports 8-bits and will
>> > > reject anything else.
>> >
>> > Then please at least add a comment about this.
>>
>> What does that have to do with this patch?
>
> You're adding a piece of code. This change is
> a) not documented in the commit message
> b) unrelated to this fix, seems to be more fitting in 10/12 or something --
> maybe even shuffling the patches a bit might help

It's not changing the behavior and it's not new code.  It's existing
code that was in a different function, but that function does other
things which can't be done at this point.  That's in the patch
description.  It is obviously part of the this patch since not calling
mxs_spi_setup_transfer() would include continuing to have the same
behavior w.r.t. checking the spi device settings that was already in
existence.

> So add a comment to make this clearer for people who hack on this after you.
>
>> There was no comment about
>> it before.  You're insisting that changes be broken up to the extent
>> that changing one line of code takes multiple patches!  Now you want a
>> comment about existing driver behavior added to patch that doesn't
>> change that behavior?
>>
>> Documenting the driver behavior would seem to be a job for the driver
>> maintainer.  If doing this is something I need to do, then maybe I
>> should maintain it?
>
> You ain't rubbing people the right way at all, I tell you. This pushy/offensive
> behavior helps noone, calm down please. It is usually a good idea to sleep on
> your reply or take your time.

Well neither are you!  I put up with your nitpicking far more than I
should have.  Move a comment from one line to another?  Really?  You
want me to waste the time to make a new patch series just because you
think a four word comment would look nicer one line up?  That's
ridiculous!  I think you've gone beyond the point of finding flaws or
mistakes (have you found any actual bugs in any of my code?) and are
just being obstructionist.

> Ad maintainership -- there's no maintainer of the driver, there's only Fabio,
> Shawn, me and the SPI subsystem guys. And it works well. It's everybodys' code
> and everyone's welcome to contribute. What's the problem?

I've spent far more time dealing with demands to split tiny patches
into even tinier patches and tweak the exact character placement to
your personal desires than I did to find and fix the bugs in the first
place!  I think you should accept that if you didn't fix the driver
before now, then someone else is going to fix it or replace it.  So
you had your opportunity to get every last character exactly the way
you wanted it and that passed.  I fixed it.  So I get to use my
judgement and write what I think is best.  If you can find any actual
bugs, or even CodingStyle errors, or merely that a majority of
existing kernel code is stylistically different, or different way of
doing something that has an object improvement, then I'm all ears.
But there is no need for you to complain about completely irrelevant
details.  I've had people submit patches to my code that didn't do
things the way I would have done it.  But you know what, if I wanted
it done my way I should have done it myself when I had the chance.

Since I seem to know how this driver works better than anyone else who
has come forward, maybe I should maintain it?  That way I can spend my
time fixing it instead or dealing making silly changes to patches.

------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html

[PATCH V2 08/12] spi/mxs: Fix race in setup method

[Trent Piepho]

On Tue, Apr 2, 2013 at 11:50 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Trent Piepho,
>
>> On Apr 2, 2013 7:27 PM, "Marek Vasut" <marex@denx.de> wrote:
>> > > The only useful thing mxs_spi_setup_transfer() (which
>> > > is no longer called) did in this instance was make that check.
>> > >
>> > > > btw. I was under the impression the MXS SSP block can handle other
>> > > > word-widths than 8 bit, no ?
>> > >
>> > > In theory it can,
>> >
>> > In practice too ;-)
>> >
>> > > however the driver only supports 8-bits and will
>> > > reject anything else.
>> >
>> > Then please at least add a comment about this.
>>
>> What does that have to do with this patch?
>
> You're adding a piece of code. This change is
> a) not documented in the commit message
> b) unrelated to this fix, seems to be more fitting in 10/12 or something --
> maybe even shuffling the patches a bit might help

It's not changing the behavior and it's not new code.  It's existing
code that was in a different function, but that function does other
things which can't be done at this point.  That's in the patch
description.  It is obviously part of the this patch since not calling
mxs_spi_setup_transfer() would include continuing to have the same
behavior w.r.t. checking the spi device settings that was already in
existence.

> So add a comment to make this clearer for people who hack on this after you.
>
>> There was no comment about
>> it before.  You're insisting that changes be broken up to the extent
>> that changing one line of code takes multiple patches!  Now you want a
>> comment about existing driver behavior added to patch that doesn't
>> change that behavior?
>>
>> Documenting the driver behavior would seem to be a job for the driver
>> maintainer.  If doing this is something I need to do, then maybe I
>> should maintain it?
>
> You ain't rubbing people the right way at all, I tell you. This pushy/offensive
> behavior helps noone, calm down please. It is usually a good idea to sleep on
> your reply or take your time.

Well neither are you!  I put up with your nitpicking far more than I
should have.  Move a comment from one line to another?  Really?  You
want me to waste the time to make a new patch series just because you
think a four word comment would look nicer one line up?  That's
ridiculous!  I think you've gone beyond the point of finding flaws or
mistakes (have you found any actual bugs in any of my code?) and are
just being obstructionist.

> Ad maintainership -- there's no maintainer of the driver, there's only Fabio,
> Shawn, me and the SPI subsystem guys. And it works well. It's everybodys' code
> and everyone's welcome to contribute. What's the problem?

I've spent far more time dealing with demands to split tiny patches
into even tinier patches and tweak the exact character placement to
your personal desires than I did to find and fix the bugs in the first
place!  I think you should accept that if you didn't fix the driver
before now, then someone else is going to fix it or replace it.  So
you had your opportunity to get every last character exactly the way
you wanted it and that passed.  I fixed it.  So I get to use my
judgement and write what I think is best.  If you can find any actual
bugs, or even CodingStyle errors, or merely that a majority of
existing kernel code is stylistically different, or different way of
doing something that has an object improvement, then I'm all ears.
But there is no need for you to complain about completely irrelevant
details.  I've had people submit patches to my code that didn't do
things the way I would have done it.  But you know what, if I wanted
it done my way I should have done it myself when I had the chance.

Since I seem to know how this driver works better than anyone else who
has come forward, maybe I should maintain it?  That way I can spend my
time fixing it instead or dealing making silly changes to patches.

Re: [PATCH V2 01/12] spi/mxs: Always set LOCK_CS

[Marek Vasut]

Hello Trent,

> There are two bits which control the CS line in the CTRL0 register:
> LOCK_CS and IGNORE_CRC.  The latter would be better named DEASSERT_CS
> in SPI mode.
> 
> LOCK_CS keeps CS asserted though the entire transfer.  This should
> always be set.  The DMA code will always set it, explicitly on the
> first segment of the first transfer, and then implicitly on all the
> rest by never clearing the bit from the value read from the ctrl0
> register.
> 
> The only reason to not set LOCK_CS would be to attempt an altered
> protocol where CS pulses between each word.  Though don't get your
> hopes up if you want to do this, as the hardware doesn't appear to do
> this in any sane manner.
> 
> The code can be simplified by just setting LOCK_CS once and then not
> needing to deal with it in the PIO and DMA transfer functions.
> 
> Signed-off-by: Trent Piepho <tpiepho@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>

Will we see a V3 of this stuff? I don't want to see this lost.

Best regards,
Marek Vasut

[PATCH V2 01/12] spi/mxs: Always set LOCK_CS

[Marek Vasut]

Hello Trent,

> There are two bits which control the CS line in the CTRL0 register:
> LOCK_CS and IGNORE_CRC.  The latter would be better named DEASSERT_CS
> in SPI mode.
> 
> LOCK_CS keeps CS asserted though the entire transfer.  This should
> always be set.  The DMA code will always set it, explicitly on the
> first segment of the first transfer, and then implicitly on all the
> rest by never clearing the bit from the value read from the ctrl0
> register.
> 
> The only reason to not set LOCK_CS would be to attempt an altered
> protocol where CS pulses between each word.  Though don't get your
> hopes up if you want to do this, as the hardware doesn't appear to do
> this in any sane manner.
> 
> The code can be simplified by just setting LOCK_CS once and then not
> needing to deal with it in the PIO and DMA transfer functions.
> 
> Signed-off-by: Trent Piepho <tpiepho@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>

Will we see a V3 of this stuff? I don't want to see this lost.

Best regards,
Marek Vasut

Re: [PATCH V2 01/12] spi/mxs: Always set LOCK_CS

[Fabio Estevam]

On Tue, Apr 2, 2013 at 8:18 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> Dear Trent Piepho,
>
>> There are two bits which control the CS line in the CTRL0 register:
>> LOCK_CS and IGNORE_CRC.  The latter would be better named DEASSERT_CS
>> in SPI mode.
>>
>> LOCK_CS keeps CS asserted though the entire transfer.  This should
>> always be set.  The DMA code will always set it, explicitly on the
>> first segment of the first transfer, and then implicitly on all the
>> rest by never clearing the bit from the value read from the ctrl0
>> register.
>>
>> The only reason to not set LOCK_CS would be to attempt an altered
>> protocol where CS pulses between each word.  Though don't get your
>> hopes up if you want to do this, as the hardware doesn't appear to do
>> this in any sane manner.
>
> Can you please elaborate on this part above? The description is very vague.
>
> Fabio, can you review this too please?

Sure, it would be nice if Trent could resend this series and copy the
SPI maintainer, Mark Brown.

------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!
http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk

[PATCH V2 01/12] spi/mxs: Always set LOCK_CS

[Fabio Estevam]

On Tue, Apr 2, 2013 at 8:18 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Trent Piepho,
>
>> There are two bits which control the CS line in the CTRL0 register:
>> LOCK_CS and IGNORE_CRC.  The latter would be better named DEASSERT_CS
>> in SPI mode.
>>
>> LOCK_CS keeps CS asserted though the entire transfer.  This should
>> always be set.  The DMA code will always set it, explicitly on the
>> first segment of the first transfer, and then implicitly on all the
>> rest by never clearing the bit from the value read from the ctrl0
>> register.
>>
>> The only reason to not set LOCK_CS would be to attempt an altered
>> protocol where CS pulses between each word.  Though don't get your
>> hopes up if you want to do this, as the hardware doesn't appear to do
>> this in any sane manner.
>
> Can you please elaborate on this part above? The description is very vague.
>
> Fabio, can you review this too please?

Sure, it would be nice if Trent could resend this series and copy the
SPI maintainer, Mark Brown.

Re: [PATCH V2 01/12] spi/mxs: Always set LOCK_CS

[Lothar Waßmann]

Hi,

Fabio Estevam writes:
> On Tue, Apr 2, 2013 at 8:18 PM, Marek Vasut <marex@denx.de> wrote:
> > Dear Trent Piepho,
> >
> >> There are two bits which control the CS line in the CTRL0 register:
> >> LOCK_CS and IGNORE_CRC.  The latter would be better named DEASSERT_CS
> >> in SPI mode.
> >>
> >> LOCK_CS keeps CS asserted though the entire transfer.  This should
> >> always be set.  The DMA code will always set it, explicitly on the
> >> first segment of the first transfer, and then implicitly on all the
> >> rest by never clearing the bit from the value read from the ctrl0
> >> register.
> >>
> >> The only reason to not set LOCK_CS would be to attempt an altered
> >> protocol where CS pulses between each word.  Though don't get your
> >> hopes up if you want to do this, as the hardware doesn't appear to do
> >> this in any sane manner.
> >
> > Can you please elaborate on this part above? The description is very vague.
> >
From my experience the HW deasserts CS whenever the output fifo runs
empty.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 01/12] spi/mxs: Always set LOCK_CS

[Lothar Waßmann]

Hi,

Fabio Estevam writes:
> On Tue, Apr 2, 2013 at 8:18 PM, Marek Vasut <marex@denx.de> wrote:
> > Dear Trent Piepho,
> >
> >> There are two bits which control the CS line in the CTRL0 register:
> >> LOCK_CS and IGNORE_CRC.  The latter would be better named DEASSERT_CS
> >> in SPI mode.
> >>
> >> LOCK_CS keeps CS asserted though the entire transfer.  This should
> >> always be set.  The DMA code will always set it, explicitly on the
> >> first segment of the first transfer, and then implicitly on all the
> >> rest by never clearing the bit from the value read from the ctrl0
> >> register.
> >>
> >> The only reason to not set LOCK_CS would be to attempt an altered
> >> protocol where CS pulses between each word.  Though don't get your
> >> hopes up if you want to do this, as the hardware doesn't appear to do
> >> this in any sane manner.
> >
> > Can you please elaborate on this part above? The description is very vague.
> >
>From my experience the HW deasserts CS whenever the output fifo runs
empty.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

Re: [PATCH V2 01/12] spi/mxs: Always set LOCK_CS

[Trent Piepho]

On Wed, Jul 10, 2013 at 8:29 AM, Lothar Waßmann <LW-AvR2QvxeiV7DiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>wrote:

> > >> always be set.  The DMA code will always set it, explicitly on the
> > >> first segment of the first transfer, and then implicitly on all the
> > >> rest by never clearing the bit from the value read from the ctrl0
> > >> register.
> > >>
> > >> The only reason to not set LOCK_CS would be to attempt an altered
> > >> protocol where CS pulses between each word.  Though don't get your
> > >> hopes up if you want to do this, as the hardware doesn't appear to do
> > >> this in any sane manner.
> > >
> > > Can you please elaborate on this part above? The description is very
> vague.
> > >
> > >> LOCK_CS keeps CS asserted though the entire transfer.  This should
> From my experience the HW deasserts CS whenever the output fifo runs
> empty.
>
>
I think that might be the case.  Since I have no need to add support for
some non-standard SPI protocol nobody has wanted, I didn't spend the time
to reverse engineer how exactly it worked.  I think what would happen is
that if you used multi-word transfers (and the current PIO mode in the
driver ONLY uses single word transfers!) you would sometimes get a CS pulse
between each word and sometimes every four words.  It seemed to make a
difference if one used multi-word PIO with a FIFO, which the current driver
doesn't support, or multi-word DMA.

I did spend enough time on it to determine that, "always set LOCK_CS all
the time for normal SPI," is correct.  The driver's method of setting in
for all transfers but the last, except in DMA mode where it's set on the
last transfer too, apparently by accident, is unnecessarily complex and
inefficient.  It only "works" because the current PIO code only supports
single word transfers at the hardware level.
------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!
http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk