[PATCH 1/5] spi/mxs: Fix extra CS pulses and read mode in multi-transfer messages

[PATCH 1/5] 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.

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, as the hardware doesn't appear to do this in any sane
manner.

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.

The spi transfer "cs_change" flag is ignored by the driver.

The driver passes a "first" and "last" flag to the transfer functions
for a message, so they can know how to set these bits.  It does this
by passing them as pointers.  There is no reason to do this, as the
flags are not modified in the transfer function.  And since LOCK_CS
can be set and left that way, there is no need for a "first" flag at
all.

This patch fixes DEASSERT_CS and READ being left on in DMA mode,
eliminates the flags being passed as separate pointers, and adds
support for the cs_change flag.

Note that while using the cs_change flag to leave CS asserted after
the final transfer works, getting the CS to automatically turn off
when a different slave is addressed might not work.

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 |   86 +++++++++++++++++++++----------------------------
 1 file changed, 36 insertions(+), 50 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 22a0af0..aa77d96b9 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -58,6 +58,11 @@
 
 #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;
@@ -91,6 +96,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) |
@@ -155,26 +162,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_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);
-}
-
-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);
-}
-
 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);
@@ -214,7 +201,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;
@@ -241,20 +228,21 @@ 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 (*first)
-		ctrl0 |= BM_SSP_CTRL0_LOCK_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) {
@@ -279,7 +267,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;
@@ -299,7 +287,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) {
@@ -336,7 +324,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);
@@ -346,18 +334,20 @@ 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;
 
-	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);
+		if (len == 0 && (flags & TXRX_DEASSERT_CS))
+			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,
@@ -368,7 +358,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
@@ -381,13 +371,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;
@@ -412,13 +402,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) {
@@ -427,10 +415,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");
@@ -455,11 +442,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) +
@@ -468,11 +455,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) {
@@ -481,7 +468,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 1/5] 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.

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, as the hardware doesn't appear to do this in any sane
manner.

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.

The spi transfer "cs_change" flag is ignored by the driver.

The driver passes a "first" and "last" flag to the transfer functions
for a message, so they can know how to set these bits.  It does this
by passing them as pointers.  There is no reason to do this, as the
flags are not modified in the transfer function.  And since LOCK_CS
can be set and left that way, there is no need for a "first" flag at
all.

This patch fixes DEASSERT_CS and READ being left on in DMA mode,
eliminates the flags being passed as separate pointers, and adds
support for the cs_change flag.

Note that while using the cs_change flag to leave CS asserted after
the final transfer works, getting the CS to automatically turn off
when a different slave is addressed might not work.

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 |   86 +++++++++++++++++++++----------------------------
 1 file changed, 36 insertions(+), 50 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index 22a0af0..aa77d96b9 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -58,6 +58,11 @@
 
 #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;
@@ -91,6 +96,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) |
@@ -155,26 +162,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_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);
-}
-
-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);
-}
-
 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);
@@ -214,7 +201,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;
@@ -241,20 +228,21 @@ 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 (*first)
-		ctrl0 |= BM_SSP_CTRL0_LOCK_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) {
@@ -279,7 +267,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;
@@ -299,7 +287,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) {
@@ -336,7 +324,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);
@@ -346,18 +334,20 @@ 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;
 
-	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);
+		if (len == 0 && (flags & TXRX_DEASSERT_CS))
+			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,
@@ -368,7 +358,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
@@ -381,13 +371,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;
@@ -412,13 +402,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) {
@@ -427,10 +415,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");
@@ -455,11 +442,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) +
@@ -468,11 +455,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) {
@@ -481,7 +468,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 2/5] 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 |   40 +++++++++++++++-------------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index aa77d96b9..5d63b21 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -130,9 +130,9 @@ static int mxs_spi_setup(struct spi_device *dev)
 	return err;
 }
 
-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
@@ -150,18 +150,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);
@@ -199,7 +187,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)
 {
@@ -227,10 +215,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;
@@ -332,7 +321,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)
 {
@@ -342,8 +331,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,
@@ -405,9 +392,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) {
 
@@ -440,11 +430,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 {
@@ -453,11 +443,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 2/5] 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 |   40 +++++++++++++++-------------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index aa77d96b9..5d63b21 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -130,9 +130,9 @@ static int mxs_spi_setup(struct spi_device *dev)
 	return err;
 }
 
-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
@@ -150,18 +150,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);
@@ -199,7 +187,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)
 {
@@ -227,10 +215,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;
@@ -332,7 +321,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)
 {
@@ -342,8 +331,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,
@@ -405,9 +392,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) {
 
@@ -440,11 +430,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 {
@@ -453,11 +443,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 3/5] 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 5d63b21..7218006 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -408,12 +408,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 3/5] 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 5d63b21..7218006 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -408,12 +408,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 4/5] 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 7218006..fc52f78 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -587,7 +587,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 4/5] 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 7218006..fc52f78 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -587,7 +587,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 5/5] spi/mxs: Fix multiple slave bug and don't set clock for each xfer

[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().

Now that mxs_spi_setup_transfer() won't be called with a NULL transfer,
since it's only purpose is to setup a transfer, the code can be
simplified.

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 |   54 ++++++++++++++++++++++++-------------------------
 1 file changed, 26 insertions(+), 28 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index fc52f78..b60baab 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -66,44 +66,47 @@
 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,
-				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;
-
-	bits_per_word = dev->bits_per_word;
-	if (t && t->bits_per_word)
-		bits_per_word = t->bits_per_word;
+	const unsigned int bits_per_word = t->bits_per_word ? : dev->bits_per_word;
+	const unsigned int hz = t->speed_hz ? min(dev->max_speed_hz, t->speed_hz) :
+					      dev->max_speed_hz;
 
 	if (bits_per_word != 8) {
-		dev_err(&dev->dev, "%s, unsupported bits_per_word=%d\n",
-					__func__, bits_per_word);
+		dev_err(&dev->dev, "Unsupported bits per word of %d\n",
+			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;
 	}
 
-	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? */
+	}
 
 	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);
@@ -113,21 +116,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))
+	if (dev->bits_per_word != 8)
 		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->mode & ~(SPI_CPOL | SPI_CPHA))
+		return -EINVAL;
 
-	return err;
+	return 0;
 }
 
 static u32 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 5/5] spi/mxs: Fix multiple slave bug and don't set clock for each xfer

[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().

Now that mxs_spi_setup_transfer() won't be called with a NULL transfer,
since it's only purpose is to setup a transfer, the code can be
simplified.

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 |   54 ++++++++++++++++++++++++-------------------------
 1 file changed, 26 insertions(+), 28 deletions(-)

diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index fc52f78..b60baab 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -66,44 +66,47 @@
 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,
-				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;
-
-	bits_per_word = dev->bits_per_word;
-	if (t && t->bits_per_word)
-		bits_per_word = t->bits_per_word;
+	const unsigned int bits_per_word = t->bits_per_word ? : dev->bits_per_word;
+	const unsigned int hz = t->speed_hz ? min(dev->max_speed_hz, t->speed_hz) :
+					      dev->max_speed_hz;
 
 	if (bits_per_word != 8) {
-		dev_err(&dev->dev, "%s, unsupported bits_per_word=%d\n",
-					__func__, bits_per_word);
+		dev_err(&dev->dev, "Unsupported bits per word of %d\n",
+			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;
 	}
 
-	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? */
+	}
 
 	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);
@@ -113,21 +116,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))
+	if (dev->bits_per_word != 8)
 		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->mode & ~(SPI_CPOL | SPI_CPHA))
+		return -EINVAL;
 
-	return err;
+	return 0;
 }
 
 static u32 mxs_spi_cs_to_reg(unsigned cs)
-- 
1.7.10.4

Re: [PATCH 1/5] spi/mxs: Fix extra CS pulses and read mode in multi-transfer messages

[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, as the hardware doesn't appear to do this in any sane
> manner.
> 
> 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.
> 
> The spi transfer "cs_change" flag is ignored by the driver.
> 
> The driver passes a "first" and "last" flag to the transfer functions
> for a message, so they can know how to set these bits.  It does this
> by passing them as pointers.  There is no reason to do this, as the
> flags are not modified in the transfer function.  And since LOCK_CS
> can be set and left that way, there is no need for a "first" flag at
> all.
> 
> This patch fixes DEASSERT_CS and READ being left on in DMA mode,
> eliminates the flags being passed as separate pointers, and adds
> support for the cs_change flag.
> 
> Note that while using the cs_change flag to leave CS asserted after
> the final transfer works, getting the CS to automatically turn off
> when a different slave is addressed might not work.
> 
> 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 |   86
> +++++++++++++++++++++---------------------------- 1 file changed, 36
> insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> index 22a0af0..aa77d96b9 100644
> --- a/drivers/spi/spi-mxs.c
> +++ b/drivers/spi/spi-mxs.c
> @@ -58,6 +58,11 @@
> 
>  #define SG_MAXLEN		0xff00
> 
> +/* Flags for txrx functions.  More efficient that using an argument
> register for + * each one. */

Fix the comment please.

/*
 * Multiline comment should really
 * be like this.
 */

> +#define TXRX_WRITE		1	/* This is a write */
> +#define TXRX_DEASSERT_CS	2	/* De-assert CS at end of txrx */

New flags? I'm sure the GCC can optimize function parameters pretty well, esp. 
if you make the bool.

>  struct mxs_spi {
>  	struct mxs_ssp		ssp;
>  	struct completion	c;
> @@ -91,6 +96,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) |
> @@ -155,26 +162,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_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);
> -}
> -
> -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);
> -}
> -
>  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);
> @@ -214,7 +201,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;
> @@ -241,20 +228,21 @@ 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;

Well ... ctrl0 &= ~(FOO | BAR | BAZ); ... won't cut it?

>  	ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
> 
> -	if (*first)
> -		ctrl0 |= BM_SSP_CTRL0_LOCK_CS;

The DMA will overwrite ctrl0 in each turn, did you try very long (>64kB long) 
transfers with your adjustments? To test this, you can try

$ dd if=/dev/mtdN of=/dev/null bs=1M

on sufficiently large SPI flash supporting the FAST_READ opcode.

> -	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) */

Wrong multi-line comment. See Documentation/CodingStyle chapter 8.

> +		if ((sg_count + 1 == sgs) && (flags & TXRX_DEASSERT_CS))
>  			ctrl0 |= BM_SSP_CTRL0_IGNORE_CRC;
> 
>  		if (ssp->devid == IMX23_SSP) {
> @@ -279,7 +267,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);

I think this is unneeded.

>  		len -= min;
>  		buf += min;
> @@ -299,7 +287,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) {
> @@ -336,7 +324,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);
> @@ -346,18 +334,20 @@ 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;
> 
> -	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);
> +		if (len == 0 && (flags & TXRX_DEASSERT_CS))
> +			writel(BM_SSP_CTRL0_IGNORE_CRC,
> +			       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);

Ok, I'll stop here. You mixed two kinds of changes here:
1) The "fixup" of the flags
2) The adjustments to handling the IGNORE_CRC and LOCK_CS

Split the patch please.

------------------------------------------------------------------------------
Own the Future-Intel&reg; 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://p.sf.net/sfu/intel_levelupd2d

[PATCH 1/5] spi/mxs: Fix extra CS pulses and read mode in multi-transfer messages

[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, as the hardware doesn't appear to do this in any sane
> manner.
> 
> 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.
> 
> The spi transfer "cs_change" flag is ignored by the driver.
> 
> The driver passes a "first" and "last" flag to the transfer functions
> for a message, so they can know how to set these bits.  It does this
> by passing them as pointers.  There is no reason to do this, as the
> flags are not modified in the transfer function.  And since LOCK_CS
> can be set and left that way, there is no need for a "first" flag at
> all.
> 
> This patch fixes DEASSERT_CS and READ being left on in DMA mode,
> eliminates the flags being passed as separate pointers, and adds
> support for the cs_change flag.
> 
> Note that while using the cs_change flag to leave CS asserted after
> the final transfer works, getting the CS to automatically turn off
> when a different slave is addressed might not work.
> 
> 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 |   86
> +++++++++++++++++++++---------------------------- 1 file changed, 36
> insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> index 22a0af0..aa77d96b9 100644
> --- a/drivers/spi/spi-mxs.c
> +++ b/drivers/spi/spi-mxs.c
> @@ -58,6 +58,11 @@
> 
>  #define SG_MAXLEN		0xff00
> 
> +/* Flags for txrx functions.  More efficient that using an argument
> register for + * each one. */

Fix the comment please.

/*
 * Multiline comment should really
 * be like this.
 */

> +#define TXRX_WRITE		1	/* This is a write */
> +#define TXRX_DEASSERT_CS	2	/* De-assert CS at end of txrx */

New flags? I'm sure the GCC can optimize function parameters pretty well, esp. 
if you make the bool.

>  struct mxs_spi {
>  	struct mxs_ssp		ssp;
>  	struct completion	c;
> @@ -91,6 +96,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) |
> @@ -155,26 +162,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_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);
> -}
> -
> -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);
> -}
> -
>  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);
> @@ -214,7 +201,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;
> @@ -241,20 +228,21 @@ 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;

Well ... ctrl0 &= ~(FOO | BAR | BAZ); ... won't cut it?

>  	ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
> 
> -	if (*first)
> -		ctrl0 |= BM_SSP_CTRL0_LOCK_CS;

The DMA will overwrite ctrl0 in each turn, did you try very long (>64kB long) 
transfers with your adjustments? To test this, you can try

$ dd if=/dev/mtdN of=/dev/null bs=1M

on sufficiently large SPI flash supporting the FAST_READ opcode.

> -	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) */

Wrong multi-line comment. See Documentation/CodingStyle chapter 8.

> +		if ((sg_count + 1 == sgs) && (flags & TXRX_DEASSERT_CS))
>  			ctrl0 |= BM_SSP_CTRL0_IGNORE_CRC;
> 
>  		if (ssp->devid == IMX23_SSP) {
> @@ -279,7 +267,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);

I think this is unneeded.

>  		len -= min;
>  		buf += min;
> @@ -299,7 +287,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) {
> @@ -336,7 +324,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);
> @@ -346,18 +334,20 @@ 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;
> 
> -	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);
> +		if (len == 0 && (flags & TXRX_DEASSERT_CS))
> +			writel(BM_SSP_CTRL0_IGNORE_CRC,
> +			       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);

Ok, I'll stop here. You mixed two kinds of changes here:
1) The "fixup" of the flags
2) The adjustments to handling the IGNORE_CRC and LOCK_CS

Split the patch please.

Re: [PATCH 2/5] 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.

Ok, lemme ask this one more time -- will the DMA work with long transfers where 
the CTRL0 will be overwritten on each turn? Did you actually test this?

> 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 |   40 +++++++++++++++-------------------------
>  1 file changed, 15 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> index aa77d96b9..5d63b21 100644
> --- a/drivers/spi/spi-mxs.c
> +++ b/drivers/spi/spi-mxs.c
> @@ -130,9 +130,9 @@ static int mxs_spi_setup(struct spi_device *dev)
>  	return err;
>  }
> 
> -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;

This change is unneeded, remove it.

>  	/*
>  	 * i.MX28 Datasheet: 17.10.1: HW_SSP_CTRL0
> @@ -150,18 +150,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);
> @@ -199,7 +187,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)
>  {
> @@ -227,10 +215,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;
> @@ -332,7 +321,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)
>  {
> @@ -342,8 +331,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,
> @@ -405,9 +392,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) {
> 
> @@ -440,11 +430,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 {
> @@ -453,11 +443,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);
>  		}

Best regards,
Marek Vasut

------------------------------------------------------------------------------
Own the Future-Intel&reg; 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://p.sf.net/sfu/intel_levelupd2d

[PATCH 2/5] 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.

Ok, lemme ask this one more time -- will the DMA work with long transfers where 
the CTRL0 will be overwritten on each turn? Did you actually test this?

> 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 |   40 +++++++++++++++-------------------------
>  1 file changed, 15 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> index aa77d96b9..5d63b21 100644
> --- a/drivers/spi/spi-mxs.c
> +++ b/drivers/spi/spi-mxs.c
> @@ -130,9 +130,9 @@ static int mxs_spi_setup(struct spi_device *dev)
>  	return err;
>  }
> 
> -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;

This change is unneeded, remove it.

>  	/*
>  	 * i.MX28 Datasheet: 17.10.1: HW_SSP_CTRL0
> @@ -150,18 +150,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);
> @@ -199,7 +187,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)
>  {
> @@ -227,10 +215,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;
> @@ -332,7 +321,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)
>  {
> @@ -342,8 +331,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,
> @@ -405,9 +392,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) {
> 
> @@ -440,11 +430,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 {
> @@ -453,11 +443,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);
>  		}

Best regards,
Marek Vasut

Re: [PATCH 4/5] spi/mxs: Remove bogus setting of ssp clk rate field

[Marek Vasut]

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

I suspect this patch should also fix drivers/clk/mxs/clk-ssp.c then?

> 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 7218006..fc52f78 100644
> --- a/drivers/spi/spi-mxs.c
> +++ b/drivers/spi/spi-mxs.c
> @@ -587,7 +587,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);

Best regards,
Marek Vasut

------------------------------------------------------------------------------
Own the Future-Intel&reg; 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://p.sf.net/sfu/intel_levelupd2d

[PATCH 4/5] spi/mxs: Remove bogus setting of ssp clk rate field

[Marek Vasut]

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

I suspect this patch should also fix drivers/clk/mxs/clk-ssp.c then?

> 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 7218006..fc52f78 100644
> --- a/drivers/spi/spi-mxs.c
> +++ b/drivers/spi/spi-mxs.c
> @@ -587,7 +587,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);

Best regards,
Marek Vasut

Re: [PATCH 5/5] spi/mxs: Fix multiple slave bug and don't set clock for each xfer

[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().
> 
> Now that mxs_spi_setup_transfer() won't be called with a NULL transfer,
> since it's only purpose is to setup a transfer, the code can be
> simplified.
> 
> 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.

This patch is full of unrelated changes, the relevant parts are not clear. 
Please clean up and resubmit with only the relevant changes.

> 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 |   54
> ++++++++++++++++++++++++------------------------- 1 file changed, 26
> insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> index fc52f78..b60baab 100644
> --- a/drivers/spi/spi-mxs.c
> +++ b/drivers/spi/spi-mxs.c
> @@ -66,44 +66,47 @@
>  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,
> -				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;
> -
> -	bits_per_word = dev->bits_per_word;
> -	if (t && t->bits_per_word)
> -		bits_per_word = t->bits_per_word;
> +	const unsigned int bits_per_word = t->bits_per_word ? :
> dev->bits_per_word; +	const unsigned int hz = t->speed_hz ?
> min(dev->max_speed_hz, t->speed_hz) : +					      
dev->max_speed_hz;
> 
>  	if (bits_per_word != 8) {
> -		dev_err(&dev->dev, "%s, unsupported bits_per_word=%d\n",
> -					__func__, bits_per_word);
> +		dev_err(&dev->dev, "Unsupported bits per word of %d\n",
> +			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;
>  	}
> 
> -	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? */
> +	}
> 
>  	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);
> @@ -113,21 +116,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))
> +	if (dev->bits_per_word != 8)
>  		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->mode & ~(SPI_CPOL | SPI_CPHA))
> +		return -EINVAL;
> 
> -	return err;
> +	return 0;
>  }
> 
>  static u32 mxs_spi_cs_to_reg(unsigned cs)

Best regards,
Marek Vasut

------------------------------------------------------------------------------
Own the Future-Intel&reg; 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://p.sf.net/sfu/intel_levelupd2d

[PATCH 5/5] spi/mxs: Fix multiple slave bug and don't set clock for each xfer

[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().
> 
> Now that mxs_spi_setup_transfer() won't be called with a NULL transfer,
> since it's only purpose is to setup a transfer, the code can be
> simplified.
> 
> 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.

This patch is full of unrelated changes, the relevant parts are not clear. 
Please clean up and resubmit with only the relevant changes.

> 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 |   54
> ++++++++++++++++++++++++------------------------- 1 file changed, 26
> insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> index fc52f78..b60baab 100644
> --- a/drivers/spi/spi-mxs.c
> +++ b/drivers/spi/spi-mxs.c
> @@ -66,44 +66,47 @@
>  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,
> -				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;
> -
> -	bits_per_word = dev->bits_per_word;
> -	if (t && t->bits_per_word)
> -		bits_per_word = t->bits_per_word;
> +	const unsigned int bits_per_word = t->bits_per_word ? :
> dev->bits_per_word; +	const unsigned int hz = t->speed_hz ?
> min(dev->max_speed_hz, t->speed_hz) : +					      
dev->max_speed_hz;
> 
>  	if (bits_per_word != 8) {
> -		dev_err(&dev->dev, "%s, unsupported bits_per_word=%d\n",
> -					__func__, bits_per_word);
> +		dev_err(&dev->dev, "Unsupported bits per word of %d\n",
> +			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;
>  	}
> 
> -	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? */
> +	}
> 
>  	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);
> @@ -113,21 +116,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))
> +	if (dev->bits_per_word != 8)
>  		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->mode & ~(SPI_CPOL | SPI_CPHA))
> +		return -EINVAL;
> 
> -	return err;
> +	return 0;
>  }
> 
>  static u32 mxs_spi_cs_to_reg(unsigned cs)

Best regards,
Marek Vasut

Re: [PATCH 2/5] spi/mxs: Fix chip select control bits in DMA mode

[Trent Piepho]

On Mon, Apr 1, 2013 at 4:13 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>>
>> 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.
>
> Ok, lemme ask this one more time -- will the DMA work with long transfers where
> the CTRL0 will be overwritten on each turn? Did you actually test this?

I don't have SPI flash, so I can't test SPI flash.  I can test other
spi devices and by capturing the pins with  a logic analyzer.  It does
work after the patch, and does not work before the patch.  The error
should be obvious from looking at the code.

ctrl0 is not overwritten, from scratch, each on transfer.  The ctrl0
value in the PIO part of the DMA is based on the current value of
ctrl0 with additional bits ORed in.   The flaw here is that bits that
should not be set are not masked out.

>>
>> -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'll make it a separate patch.

------------------------------------------------------------------------------
Own the Future-Intel&reg; 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://p.sf.net/sfu/intel_levelupd2d

[PATCH 2/5] spi/mxs: Fix chip select control bits in DMA mode

[Trent Piepho]

On Mon, Apr 1, 2013 at 4:13 PM, Marek Vasut <marex@denx.de> wrote:
>>
>> 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.
>
> Ok, lemme ask this one more time -- will the DMA work with long transfers where
> the CTRL0 will be overwritten on each turn? Did you actually test this?

I don't have SPI flash, so I can't test SPI flash.  I can test other
spi devices and by capturing the pins with  a logic analyzer.  It does
work after the patch, and does not work before the patch.  The error
should be obvious from looking at the code.

ctrl0 is not overwritten, from scratch, each on transfer.  The ctrl0
value in the PIO part of the DMA is based on the current value of
ctrl0 with additional bits ORed in.   The flaw here is that bits that
should not be set are not masked out.

>>
>> -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'll make it a separate patch.

Re: [PATCH 2/5] spi/mxs: Fix chip select control bits in DMA mode

[Marek Vasut]

Dear Trent Piepho,

> On Mon, Apr 1, 2013 at 4:13 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> >> 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.
> > 
> > Ok, lemme ask this one more time -- will the DMA work with long transfers
> > where the CTRL0 will be overwritten on each turn? Did you actually test
> > this?
> 
> I don't have SPI flash, so I can't test SPI flash.  I can test other
> spi devices and by capturing the pins with  a logic analyzer.  It does
> work after the patch, and does not work before the patch.  The error
> should be obvious from looking at the code.
> 
> ctrl0 is not overwritten, from scratch, each on transfer.  The ctrl0
> value in the PIO part of the DMA is based on the current value of
> ctrl0 with additional bits ORed in.   The flaw here is that bits that
> should not be set are not masked out.
> 
> >> -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'll make it a separate patch.

This is completely irrelevant change, please just submit the relevant patches.

Best regards,
Marek Vasut

------------------------------------------------------------------------------
Own the Future-Intel&reg; 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://p.sf.net/sfu/intel_levelupd2d

[PATCH 2/5] spi/mxs: Fix chip select control bits in DMA mode

[Marek Vasut]

Dear Trent Piepho,

> On Mon, Apr 1, 2013 at 4:13 PM, Marek Vasut <marex@denx.de> wrote:
> >> 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.
> > 
> > Ok, lemme ask this one more time -- will the DMA work with long transfers
> > where the CTRL0 will be overwritten on each turn? Did you actually test
> > this?
> 
> I don't have SPI flash, so I can't test SPI flash.  I can test other
> spi devices and by capturing the pins with  a logic analyzer.  It does
> work after the patch, and does not work before the patch.  The error
> should be obvious from looking at the code.
> 
> ctrl0 is not overwritten, from scratch, each on transfer.  The ctrl0
> value in the PIO part of the DMA is based on the current value of
> ctrl0 with additional bits ORed in.   The flaw here is that bits that
> should not be set are not masked out.
> 
> >> -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'll make it a separate patch.

This is completely irrelevant change, please just submit the relevant patches.

Best regards,
Marek Vasut

Re: [PATCH 4/5] spi/mxs: Remove bogus setting of ssp clk rate field

[Trent Piepho]

On Mon, Apr 1, 2013 at 4:16 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>> 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.
>
> I suspect this patch should also fix drivers/clk/mxs/clk-ssp.c then?

Why do you say that?  I see no problem with clk-ssp.c, as setting the
clk_rate field in the ssp struct to the actual programmed rate makes
sense.  The code in spi-mxs.c just makes no sense.  I suspect it was
added by mistake when porting the driver.

------------------------------------------------------------------------------
Own the Future-Intel&reg; 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://p.sf.net/sfu/intel_levelupd2d

[PATCH 4/5] spi/mxs: Remove bogus setting of ssp clk rate field

[Trent Piepho]

On Mon, Apr 1, 2013 at 4:16 PM, Marek Vasut <marex@denx.de> wrote:
>> 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.
>
> I suspect this patch should also fix drivers/clk/mxs/clk-ssp.c then?

Why do you say that?  I see no problem with clk-ssp.c, as setting the
clk_rate field in the ssp struct to the actual programmed rate makes
sense.  The code in spi-mxs.c just makes no sense.  I suspect it was
added by mistake when porting the driver.

Re: [PATCH 4/5] spi/mxs: Remove bogus setting of ssp clk rate field

[Marek Vasut]

Dear Trent Piepho,

> On Mon, Apr 1, 2013 at 4:16 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> >> 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.
> > 
> > I suspect this patch should also fix drivers/clk/mxs/clk-ssp.c then?
> 
> Why do you say that?  I see no problem with clk-ssp.c, as setting the
> clk_rate field in the ssp struct to the actual programmed rate makes
> sense.  The code in spi-mxs.c just makes no sense.  I suspect it was
> added by mistake when porting the driver.

Either remove it altogether if it's unused OR make sure it's inited to some sane 
value from the start.

Best regards,
Marek Vasut

------------------------------------------------------------------------------
Own the Future-Intel&reg; 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://p.sf.net/sfu/intel_levelupd2d

[PATCH 4/5] spi/mxs: Remove bogus setting of ssp clk rate field

[Marek Vasut]

Dear Trent Piepho,

> On Mon, Apr 1, 2013 at 4:16 PM, Marek Vasut <marex@denx.de> wrote:
> >> 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.
> > 
> > I suspect this patch should also fix drivers/clk/mxs/clk-ssp.c then?
> 
> Why do you say that?  I see no problem with clk-ssp.c, as setting the
> clk_rate field in the ssp struct to the actual programmed rate makes
> sense.  The code in spi-mxs.c just makes no sense.  I suspect it was
> added by mistake when porting the driver.

Either remove it altogether if it's unused OR make sure it's inited to some sane 
value from the start.

Best regards,
Marek Vasut

Re: [PATCH 2/5] spi/mxs: Fix chip select control bits in DMA mode

[Trent Piepho]

On Mon, Apr 1, 2013 at 4:30 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:

>> >> -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'll make it a separate patch.
>
> This is completely irrelevant change, please just submit the relevant patches.

Kernel code should use u16, u32, etc. instead of the uint16_t,
uint32_t types.  The rest of the driver uses them.  Why should this
one function use a different type than the rest?  It's ugly and
inconsistent.

And really, it's just as relevant as insisting that multiline patches
use some exact format, which checkpatch.pl doesn't complain about.

------------------------------------------------------------------------------
Own the Future-Intel&reg; 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://p.sf.net/sfu/intel_levelupd2d

[PATCH 2/5] spi/mxs: Fix chip select control bits in DMA mode

[Trent Piepho]

On Mon, Apr 1, 2013 at 4:30 PM, Marek Vasut <marex@denx.de> wrote:

>> >> -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'll make it a separate patch.
>
> This is completely irrelevant change, please just submit the relevant patches.

Kernel code should use u16, u32, etc. instead of the uint16_t,
uint32_t types.  The rest of the driver uses them.  Why should this
one function use a different type than the rest?  It's ugly and
inconsistent.

And really, it's just as relevant as insisting that multiline patches
use some exact format, which checkpatch.pl doesn't complain about.

Re: [PATCH 2/5] spi/mxs: Fix chip select control bits in DMA mode

[Marek Vasut]

Dear Trent Piepho,

> On Mon, Apr 1, 2013 at 4:30 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> >> >> -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'll make it a separate patch.
> > 
> > This is completely irrelevant change, please just submit the relevant
> > patches.
> 
> Kernel code should use u16, u32, etc. instead of the uint16_t,
> uint32_t types.  The rest of the driver uses them.  Why should this
> one function use a different type than the rest?  It's ugly and
> inconsistent.

Is this documented somewhere please? I see only a mention about this in chapter 
5 of Documentation/CodingStyle , section (d) . Either way, separate such 
cosmetic change into another series so they're not in the way of relevant stuff.

> And really, it's just as relevant as insisting that multiline patches
> use some exact format, which checkpatch.pl doesn't complain about.

The checkpatch is not almighty tool, Documentation/CodingStyle describes how 
code should be written/annotated/documented.

Best regards,
Marek Vasut

------------------------------------------------------------------------------
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 2/5] spi/mxs: Fix chip select control bits in DMA mode

[Marek Vasut]

Dear Trent Piepho,

> On Mon, Apr 1, 2013 at 4:30 PM, Marek Vasut <marex@denx.de> wrote:
> >> >> -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'll make it a separate patch.
> > 
> > This is completely irrelevant change, please just submit the relevant
> > patches.
> 
> Kernel code should use u16, u32, etc. instead of the uint16_t,
> uint32_t types.  The rest of the driver uses them.  Why should this
> one function use a different type than the rest?  It's ugly and
> inconsistent.

Is this documented somewhere please? I see only a mention about this in chapter 
5 of Documentation/CodingStyle , section (d) . Either way, separate such 
cosmetic change into another series so they're not in the way of relevant stuff.

> And really, it's just as relevant as insisting that multiline patches
> use some exact format, which checkpatch.pl doesn't complain about.

The checkpatch is not almighty tool, Documentation/CodingStyle describes how 
code should be written/annotated/documented.

Best regards,
Marek Vasut

Re: [PATCH 4/5] spi/mxs: Remove bogus setting of ssp clk rate field

[Trent Piepho]

On Mon, Apr 1, 2013 at 4:37 PM, Marek Vasut <marex@denx.de> wrote:
>> On Mon, Apr 1, 2013 at 4:16 PM, Marek Vasut <marex@denx.de> wrote:
>> >> 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.
>> >
>> > I suspect this patch should also fix drivers/clk/mxs/clk-ssp.c then?
>>
>> Why do you say that?  I see no problem with clk-ssp.c, as setting the
>> clk_rate field in the ssp struct to the actual programmed rate makes
>> sense.  The code in spi-mxs.c just makes no sense.  I suspect it was
>> added by mistake when porting the driver.
>
> Either remove it altogether if it's unused OR make sure it's inited to some sane
> value from the start.

This field doesn't need to be initted by a user of the common SSP
clock code, as it is not an input to that code.  It is set by the SSP
clock code.  After the SSP clock is programmed, it can be read to find
the true SSP rate.  There is no need for any user of the SSP code to
set the clk_rate field, and other than this one incorrect line, no
user does so.  It's not used currently in the SPI driver at all, but
it certainly could be.  The SSP code is shared with the MMC driver
that can drive an SSP port in SD/MMC mode.  The mxs-mmc code does need
the actual SSP clock and does use the field.  That code does not write
to ssp->clk_rate, because as I have said, it is in output from the SSP
clock code.

Since the ssp struct is part of the spi master device data, it would
be initialized to zero when allocated by spi_master_alloc().  The SPI
clock isn't part of the spi master, but of a spi slave and a spi
transfer.  Thus there is no sensible value to initialize the spi clock
to at the time the spi master is created.  Which is fine.  The code
doesn't try to and other spi drivers don't either.  At the time a spi
slave is created the spi clock could be programmed.  But that would
introduce a race.  The proper thing to do, and what the driver does,
is to program the spi clock with the requested clock for a spi
transfer.

[PATCH 4/5] spi/mxs: Remove bogus setting of ssp clk rate field

[Trent Piepho]

On Mon, Apr 1, 2013 at 4:37 PM, Marek Vasut <marex@denx.de> wrote:
>> On Mon, Apr 1, 2013 at 4:16 PM, Marek Vasut <marex@denx.de> wrote:
>> >> 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.
>> >
>> > I suspect this patch should also fix drivers/clk/mxs/clk-ssp.c then?
>>
>> Why do you say that?  I see no problem with clk-ssp.c, as setting the
>> clk_rate field in the ssp struct to the actual programmed rate makes
>> sense.  The code in spi-mxs.c just makes no sense.  I suspect it was
>> added by mistake when porting the driver.
>
> Either remove it altogether if it's unused OR make sure it's inited to some sane
> value from the start.

This field doesn't need to be initted by a user of the common SSP
clock code, as it is not an input to that code.  It is set by the SSP
clock code.  After the SSP clock is programmed, it can be read to find
the true SSP rate.  There is no need for any user of the SSP code to
set the clk_rate field, and other than this one incorrect line, no
user does so.  It's not used currently in the SPI driver at all, but
it certainly could be.  The SSP code is shared with the MMC driver
that can drive an SSP port in SD/MMC mode.  The mxs-mmc code does need
the actual SSP clock and does use the field.  That code does not write
to ssp->clk_rate, because as I have said, it is in output from the SSP
clock code.

Since the ssp struct is part of the spi master device data, it would
be initialized to zero when allocated by spi_master_alloc().  The SPI
clock isn't part of the spi master, but of a spi slave and a spi
transfer.  Thus there is no sensible value to initialize the spi clock
to at the time the spi master is created.  Which is fine.  The code
doesn't try to and other spi drivers don't either.  At the time a spi
slave is created the spi clock could be programmed.  But that would
introduce a race.  The proper thing to do, and what the driver does,
is to program the spi clock with the requested clock for a spi
transfer.

Re: [PATCH 4/5] spi/mxs: Remove bogus setting of ssp clk rate field

[Marek Vasut]

Dear Trent Piepho,

> On Mon, Apr 1, 2013 at 4:37 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> >> On Mon, Apr 1, 2013 at 4:16 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> >> >> 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.
> >> > 
> >> > I suspect this patch should also fix drivers/clk/mxs/clk-ssp.c then?
> >> 
> >> Why do you say that?  I see no problem with clk-ssp.c, as setting the
> >> clk_rate field in the ssp struct to the actual programmed rate makes
> >> sense.  The code in spi-mxs.c just makes no sense.  I suspect it was
> >> added by mistake when porting the driver.
> > 
> > Either remove it altogether if it's unused OR make sure it's inited to
> > some sane value from the start.
> 
> This field doesn't need to be initted by a user of the common SSP
> clock code, as it is not an input to that code.  It is set by the SSP
> clock code.  After the SSP clock is programmed, it can be read to find
> the true SSP rate.  There is no need for any user of the SSP code to
> set the clk_rate field, and other than this one incorrect line, no
> user does so.  It's not used currently in the SPI driver at all, but
> it certainly could be.  The SSP code is shared with the MMC driver
> that can drive an SSP port in SD/MMC mode.  The mxs-mmc code does need
> the actual SSP clock and does use the field.  That code does not write
> to ssp->clk_rate, because as I have said, it is in output from the SSP
> clock code.

If the clk_rate is at least inited to zero, then this patch does makes sense.

[...]

Best regards,
Marek Vasut

------------------------------------------------------------------------------
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 4/5] spi/mxs: Remove bogus setting of ssp clk rate field

[Marek Vasut]

Dear Trent Piepho,

> On Mon, Apr 1, 2013 at 4:37 PM, Marek Vasut <marex@denx.de> wrote:
> >> On Mon, Apr 1, 2013 at 4:16 PM, Marek Vasut <marex@denx.de> wrote:
> >> >> 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.
> >> > 
> >> > I suspect this patch should also fix drivers/clk/mxs/clk-ssp.c then?
> >> 
> >> Why do you say that?  I see no problem with clk-ssp.c, as setting the
> >> clk_rate field in the ssp struct to the actual programmed rate makes
> >> sense.  The code in spi-mxs.c just makes no sense.  I suspect it was
> >> added by mistake when porting the driver.
> > 
> > Either remove it altogether if it's unused OR make sure it's inited to
> > some sane value from the start.
> 
> This field doesn't need to be initted by a user of the common SSP
> clock code, as it is not an input to that code.  It is set by the SSP
> clock code.  After the SSP clock is programmed, it can be read to find
> the true SSP rate.  There is no need for any user of the SSP code to
> set the clk_rate field, and other than this one incorrect line, no
> user does so.  It's not used currently in the SPI driver at all, but
> it certainly could be.  The SSP code is shared with the MMC driver
> that can drive an SSP port in SD/MMC mode.  The mxs-mmc code does need
> the actual SSP clock and does use the field.  That code does not write
> to ssp->clk_rate, because as I have said, it is in output from the SSP
> clock code.

If the clk_rate is at least inited to zero, then this patch does makes sense.

[...]

Best regards,
Marek Vasut

Re: [PATCH 1/5] spi/mxs: Fix extra CS pulses and read mode in multi-transfer messages

[Trent Piepho]

On Mon, Apr 1, 2013 at 4:11 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>> +#define TXRX_WRITE           1       /* This is a write */
>> +#define TXRX_DEASSERT_CS     2       /* De-assert CS at end of txrx */
>
> New flags? I'm sure the GCC can optimize function parameters pretty well, esp.
> if you make the bool.

Nope.  I checked the actual compiled code.  Each bool takes another
parameter.  Since the txrx functions are large and called from
multiple sites, they are not inlined.  Gcc is not able to combine all
the bool arguments into one single bitfield argument.

On arm, to check if an argument on the stack is true requires two
instructions.  First an ldr to move the argument into a register and
then a cmp to check it for zero.  Checking for a bit is the basically
the same, first an ldr and then a tst.  So there is no performance
advantage of "if(x)" vs "if(x&32)".  But, if one uses bit flags then
the same flag word can be used for every flag, avoiding the need to
load a different word for each flag.

So using the flags avoids extra instructions in the caller for each
call to place each flag in its own boolean on the stack or in an
argument register (depending on the # of function arguments), and then
avoids extra code in the callee to load any arguments on the stack
into a register.  It can make code in loops faster and smaller by
using fewer register and avoiding a register load inside the loop.

BTW, after converting the code to bool arguments instead of flags:
add/remove: 0/0 grow/shrink: 4/0 up/down: 130/0 (130)
function                                     old     new   delta
mxs_spi_transfer_one                         808     900     +92
mxs_spi_txrx_pio                             492     508     +16
mxs_spi_txrx_dma                            1188    1204     +16
__UNIQUE_ID_vermagic0                         73      79      +6

Bool arguments for each flag is larger than using one argument and bit
flags.  And there are more flags needed to support things like GPIO CS
lines.

>>       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;
>
> Well ... ctrl0 &= ~(FOO | BAR | BAZ); ... won't cut it?

Well, by De Morgan's law the two expressions are the same.  And they
are the same number of characters.  And both will produce a compile
time constant and thus the same code.  However, I like the ~FOO & ~BAR
& ~BAR form better as there is greater parallelism between each term
of the expression.

>>       ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
>>
>> -     if (*first)
>> -             ctrl0 |= BM_SSP_CTRL0_LOCK_CS;
>
> The DMA will overwrite ctrl0 in each turn, did you try very long (>64kB long)
> transfers with your adjustments? To test this, you can try
>
> $ dd if=/dev/mtdN of=/dev/null bs=1M
>
> on sufficiently large SPI flash supporting the FAST_READ opcode.

Don't have SPI flash.  I did test with logic analyzer and the current
code is clearly broken and my changes clearly fix it.  It should be
obvious from looking a the code that it is wrong.  The DMA does write
ctrl0 on each segment of each transfer, but look at the previous
paragraph to see where it comes from.  Once READ or IGNORE_CRC are
set, nothing will clear them until PIO mode is used.  It's the same
with the bug fix for 3.9 that clears the XFER_COUNT bits in DMA mode.
Strangely, I didn't notice that bug since all my xfers are the same
length!  But I do switch between slaves, between read and write, and
use messages with multiple transfers, in DMA mode, all of which are
broken in the current driver.

>> +             if ((sg_count + 1 == sgs) && (flags & TXRX_DEASSERT_CS))
>>                       ctrl0 |= BM_SSP_CTRL0_IGNORE_CRC;
>>
>>               if (ssp->devid == IMX23_SSP) {
>> @@ -279,7 +267,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);
>
> I think this is unneeded.

Whatis ?  Setting the DMA direction or using flags?

Look at it this way:

Existing code that uses the first/last flags is wrong and needs to be
changed.  Therefor code using 'first' and 'last' will be changed.

Passing the flags as pointers is bad practice and makes no sense to
do.  Does it make any sense to re-write code fix the way it uses first
and last, then re-write those same lines a second time to not use
pointers?

If the way the flags are passed should change, then why not do it the
most efficient way.  It isn't any more complicated and produces better
code.  One flag per parameter becomes cumbersome when more flags are
added for additional features.

I'm all for splitting my patches up.  I sent five patches when I bet 9
out 10 developers would have just done one.  But I don't think it
makes any sense to re-write the same line of code over and over again
in successive patches in a series before getting to the final version.
 It just makes more churn to review.

I hate it when I get a series of patches and spot a flaw, take the
time to write up the flaw and how it should be fixed, only to discover
that the flaw is fixed later on in the series and I wasted my time.

------------------------------------------------------------------------------
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 1/5] spi/mxs: Fix extra CS pulses and read mode in multi-transfer messages

[Trent Piepho]

On Mon, Apr 1, 2013 at 4:11 PM, Marek Vasut <marex@denx.de> wrote:
>> +#define TXRX_WRITE           1       /* This is a write */
>> +#define TXRX_DEASSERT_CS     2       /* De-assert CS at end of txrx */
>
> New flags? I'm sure the GCC can optimize function parameters pretty well, esp.
> if you make the bool.

Nope.  I checked the actual compiled code.  Each bool takes another
parameter.  Since the txrx functions are large and called from
multiple sites, they are not inlined.  Gcc is not able to combine all
the bool arguments into one single bitfield argument.

On arm, to check if an argument on the stack is true requires two
instructions.  First an ldr to move the argument into a register and
then a cmp to check it for zero.  Checking for a bit is the basically
the same, first an ldr and then a tst.  So there is no performance
advantage of "if(x)" vs "if(x&32)".  But, if one uses bit flags then
the same flag word can be used for every flag, avoiding the need to
load a different word for each flag.

So using the flags avoids extra instructions in the caller for each
call to place each flag in its own boolean on the stack or in an
argument register (depending on the # of function arguments), and then
avoids extra code in the callee to load any arguments on the stack
into a register.  It can make code in loops faster and smaller by
using fewer register and avoiding a register load inside the loop.

BTW, after converting the code to bool arguments instead of flags:
add/remove: 0/0 grow/shrink: 4/0 up/down: 130/0 (130)
function                                     old     new   delta
mxs_spi_transfer_one                         808     900     +92
mxs_spi_txrx_pio                             492     508     +16
mxs_spi_txrx_dma                            1188    1204     +16
__UNIQUE_ID_vermagic0                         73      79      +6

Bool arguments for each flag is larger than using one argument and bit
flags.  And there are more flags needed to support things like GPIO CS
lines.

>>       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;
>
> Well ... ctrl0 &= ~(FOO | BAR | BAZ); ... won't cut it?

Well, by De Morgan's law the two expressions are the same.  And they
are the same number of characters.  And both will produce a compile
time constant and thus the same code.  However, I like the ~FOO & ~BAR
& ~BAR form better as there is greater parallelism between each term
of the expression.

>>       ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
>>
>> -     if (*first)
>> -             ctrl0 |= BM_SSP_CTRL0_LOCK_CS;
>
> The DMA will overwrite ctrl0 in each turn, did you try very long (>64kB long)
> transfers with your adjustments? To test this, you can try
>
> $ dd if=/dev/mtdN of=/dev/null bs=1M
>
> on sufficiently large SPI flash supporting the FAST_READ opcode.

Don't have SPI flash.  I did test with logic analyzer and the current
code is clearly broken and my changes clearly fix it.  It should be
obvious from looking a the code that it is wrong.  The DMA does write
ctrl0 on each segment of each transfer, but look at the previous
paragraph to see where it comes from.  Once READ or IGNORE_CRC are
set, nothing will clear them until PIO mode is used.  It's the same
with the bug fix for 3.9 that clears the XFER_COUNT bits in DMA mode.
Strangely, I didn't notice that bug since all my xfers are the same
length!  But I do switch between slaves, between read and write, and
use messages with multiple transfers, in DMA mode, all of which are
broken in the current driver.

>> +             if ((sg_count + 1 == sgs) && (flags & TXRX_DEASSERT_CS))
>>                       ctrl0 |= BM_SSP_CTRL0_IGNORE_CRC;
>>
>>               if (ssp->devid == IMX23_SSP) {
>> @@ -279,7 +267,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);
>
> I think this is unneeded.

Whatis ?  Setting the DMA direction or using flags?

Look at it this way:

Existing code that uses the first/last flags is wrong and needs to be
changed.  Therefor code using 'first' and 'last' will be changed.

Passing the flags as pointers is bad practice and makes no sense to
do.  Does it make any sense to re-write code fix the way it uses first
and last, then re-write those same lines a second time to not use
pointers?

If the way the flags are passed should change, then why not do it the
most efficient way.  It isn't any more complicated and produces better
code.  One flag per parameter becomes cumbersome when more flags are
added for additional features.

I'm all for splitting my patches up.  I sent five patches when I bet 9
out 10 developers would have just done one.  But I don't think it
makes any sense to re-write the same line of code over and over again
in successive patches in a series before getting to the final version.
 It just makes more churn to review.

I hate it when I get a series of patches and spot a flaw, take the
time to write up the flaw and how it should be fixed, only to discover
that the flaw is fixed later on in the series and I wasted my time.

Re: [PATCH 2/5] spi/mxs: Fix chip select control bits in DMA mode

[Trent Piepho]

On Mon, Apr 1, 2013 at 5:02 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>> On Mon, Apr 1, 2013 at 4:30 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>> >> >> -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'll make it a separate patch.
>> >
>> > This is completely irrelevant change, please just submit the relevant
>> > patches.
>>
>> Kernel code should use u16, u32, etc. instead of the uint16_t,
>> uint32_t types.  The rest of the driver uses them.  Why should this
>> one function use a different type than the rest?  It's ugly and
>> inconsistent.
>
> Is this documented somewhere please? I see only a mention about this in chapter
> 5 of Documentation/CodingStyle , section (d) . Either way, separate such
> cosmetic change into another series so they're not in the way of relevant stuff.

It's pretty clearly existing practice:
$ perl -ne '@x=/\buint32_t\b/g;$x+=$#x+1;END{print "$x\n";}' drivers/spi/*.[ch]
9
$ perl -ne '@x=/\bu32\b/g;$x+=$#x+1;END{print "$x\n";}' drivers/spi/*.[ch]
501

------------------------------------------------------------------------------
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 2/5] spi/mxs: Fix chip select control bits in DMA mode

[Trent Piepho]

On Mon, Apr 1, 2013 at 5:02 PM, Marek Vasut <marex@denx.de> wrote:
>> On Mon, Apr 1, 2013 at 4:30 PM, Marek Vasut <marex@denx.de> wrote:
>> >> >> -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'll make it a separate patch.
>> >
>> > This is completely irrelevant change, please just submit the relevant
>> > patches.
>>
>> Kernel code should use u16, u32, etc. instead of the uint16_t,
>> uint32_t types.  The rest of the driver uses them.  Why should this
>> one function use a different type than the rest?  It's ugly and
>> inconsistent.
>
> Is this documented somewhere please? I see only a mention about this in chapter
> 5 of Documentation/CodingStyle , section (d) . Either way, separate such
> cosmetic change into another series so they're not in the way of relevant stuff.

It's pretty clearly existing practice:
$ perl -ne '@x=/\buint32_t\b/g;$x+=$#x+1;END{print "$x\n";}' drivers/spi/*.[ch]
9
$ perl -ne '@x=/\bu32\b/g;$x+=$#x+1;END{print "$x\n";}' drivers/spi/*.[ch]
501

Re: [PATCH 1/5] spi/mxs: Fix extra CS pulses and read mode in multi-transfer messages

[Marek Vasut]

Dear Trent Piepho,

> On Mon, Apr 1, 2013 at 4:11 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> >> +#define TXRX_WRITE           1       /* This is a write */
> >> +#define TXRX_DEASSERT_CS     2       /* De-assert CS at end of txrx */
> > 
> > New flags? I'm sure the GCC can optimize function parameters pretty well,
> > esp. if you make the bool.
> 
> Nope.  I checked the actual compiled code.  Each bool takes another
> parameter.  Since the txrx functions are large and called from
> multiple sites, they are not inlined.  Gcc is not able to combine all
> the bool arguments into one single bitfield argument.

That's pretty poor, oh well.

btw. is it necessary to define new flags or is it possible to reuse some?

[...]

> >>       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;
> > 
> > Well ... ctrl0 &= ~(FOO | BAR | BAZ); ... won't cut it?
> 
> Well, by De Morgan's law the two expressions are the same.  And they
> are the same number of characters.  And both will produce a compile
> time constant and thus the same code.  However, I like the ~FOO & ~BAR
> & ~BAR form better as there is greater parallelism between each term
> of the expression.

What about the law of readability of code ? ;-)

> >>       ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
> >> 
> >> -     if (*first)
> >> -             ctrl0 |= BM_SSP_CTRL0_LOCK_CS;
> > 
> > The DMA will overwrite ctrl0 in each turn, did you try very long (>64kB
> > long) transfers with your adjustments? To test this, you can try
> > 
> > $ dd if=/dev/mtdN of=/dev/null bs=1M
> > 
> > on sufficiently large SPI flash supporting the FAST_READ opcode.
> 
> Don't have SPI flash.  I did test with logic analyzer and the current
> code is clearly broken and my changes clearly fix it.

Good, but you clearly didn't test it in the most used case. This will clearly 
piss off a lot of people if you break something. And this will clearly not be 
good ;-)

I'm clearly planning to test the code, but only once it's clearly possible to 
tell apart churn from the actual fix (see my other rambling, just reminding 
you). So I'll wait for V2 and give it a go on a large flash.

> It should be obvious from looking a the code that it is wrong.

Sorry, it is not obvious.

> The DMA does write
> ctrl0 on each segment of each transfer, but look at the previous
> paragraph to see where it comes from.  Once READ or IGNORE_CRC are
> set, nothing will clear them until PIO mode is used.

When using a flash, the DMA and PIO mode usage is intermixed. Usually, the PIO 
is used to program the command and then DMA to transmit the payload. It's hardly 
ever PIO only orr DMA only for the whole transfer.

> It's the same
> with the bug fix for 3.9 that clears the XFER_COUNT bits in DMA mode.
> Strangely, I didn't notice that bug since all my xfers are the same
> length!  But I do switch between slaves, between read and write, and
> use messages with multiple transfers, in DMA mode, all of which are
> broken in the current driver.

btw. are you using MX23 or MX28 to test this?

> >> +             if ((sg_count + 1 == sgs) && (flags & TXRX_DEASSERT_CS))
> >> 
> >>                       ctrl0 |= BM_SSP_CTRL0_IGNORE_CRC;
> >>               
> >>               if (ssp->devid == IMX23_SSP) {
> >> 
> >> @@ -279,7 +267,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);
> > 
> > I think this is unneeded.
> 
> Whatis ?  Setting the DMA direction or using flags?
> 
> Look at it this way:
> 
> Existing code that uses the first/last flags is wrong and needs to be
> changed.  Therefor code using 'first' and 'last' will be changed.
> 
> Passing the flags as pointers is bad practice and makes no sense to
> do.  Does it make any sense to re-write code fix the way it uses first
> and last, then re-write those same lines a second time to not use
> pointers?

You can always flip it the other way -- first rework the use of flags, then 
apply the fix. It'd make the patch much easier to understand, don't you think?

> If the way the flags are passed should change, then why not do it the
> most efficient way.  It isn't any more complicated and produces better
> code.  One flag per parameter becomes cumbersome when more flags are
> added for additional features.
> 
> I'm all for splitting my patches up.  I sent five patches when I bet 9
> out 10 developers would have just done one.  But I don't think it
> makes any sense to re-write the same line of code over and over again
> in successive patches in a series before getting to the final version.
>  It just makes more churn to review.

Splitting the patchset to make it more understandable is OK though. And I'm 
really getting lost in this patch.

> I hate it when I get a series of patches and spot a flaw, take the
> time to write up the flaw and how it should be fixed, only to discover
> that the flaw is fixed later on in the series and I wasted my time.

Best regards,
Marek Vasut

------------------------------------------------------------------------------
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 1/5] spi/mxs: Fix extra CS pulses and read mode in multi-transfer messages

[Marek Vasut]

Dear Trent Piepho,

> On Mon, Apr 1, 2013 at 4:11 PM, Marek Vasut <marex@denx.de> wrote:
> >> +#define TXRX_WRITE           1       /* This is a write */
> >> +#define TXRX_DEASSERT_CS     2       /* De-assert CS at end of txrx */
> > 
> > New flags? I'm sure the GCC can optimize function parameters pretty well,
> > esp. if you make the bool.
> 
> Nope.  I checked the actual compiled code.  Each bool takes another
> parameter.  Since the txrx functions are large and called from
> multiple sites, they are not inlined.  Gcc is not able to combine all
> the bool arguments into one single bitfield argument.

That's pretty poor, oh well.

btw. is it necessary to define new flags or is it possible to reuse some?

[...]

> >>       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;
> > 
> > Well ... ctrl0 &= ~(FOO | BAR | BAZ); ... won't cut it?
> 
> Well, by De Morgan's law the two expressions are the same.  And they
> are the same number of characters.  And both will produce a compile
> time constant and thus the same code.  However, I like the ~FOO & ~BAR
> & ~BAR form better as there is greater parallelism between each term
> of the expression.

What about the law of readability of code ? ;-)

> >>       ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
> >> 
> >> -     if (*first)
> >> -             ctrl0 |= BM_SSP_CTRL0_LOCK_CS;
> > 
> > The DMA will overwrite ctrl0 in each turn, did you try very long (>64kB
> > long) transfers with your adjustments? To test this, you can try
> > 
> > $ dd if=/dev/mtdN of=/dev/null bs=1M
> > 
> > on sufficiently large SPI flash supporting the FAST_READ opcode.
> 
> Don't have SPI flash.  I did test with logic analyzer and the current
> code is clearly broken and my changes clearly fix it.

Good, but you clearly didn't test it in the most used case. This will clearly 
piss off a lot of people if you break something. And this will clearly not be 
good ;-)

I'm clearly planning to test the code, but only once it's clearly possible to 
tell apart churn from the actual fix (see my other rambling, just reminding 
you). So I'll wait for V2 and give it a go on a large flash.

> It should be obvious from looking a the code that it is wrong.

Sorry, it is not obvious.

> The DMA does write
> ctrl0 on each segment of each transfer, but look at the previous
> paragraph to see where it comes from.  Once READ or IGNORE_CRC are
> set, nothing will clear them until PIO mode is used.

When using a flash, the DMA and PIO mode usage is intermixed. Usually, the PIO 
is used to program the command and then DMA to transmit the payload. It's hardly 
ever PIO only orr DMA only for the whole transfer.

> It's the same
> with the bug fix for 3.9 that clears the XFER_COUNT bits in DMA mode.
> Strangely, I didn't notice that bug since all my xfers are the same
> length!  But I do switch between slaves, between read and write, and
> use messages with multiple transfers, in DMA mode, all of which are
> broken in the current driver.

btw. are you using MX23 or MX28 to test this?

> >> +             if ((sg_count + 1 == sgs) && (flags & TXRX_DEASSERT_CS))
> >> 
> >>                       ctrl0 |= BM_SSP_CTRL0_IGNORE_CRC;
> >>               
> >>               if (ssp->devid == IMX23_SSP) {
> >> 
> >> @@ -279,7 +267,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);
> > 
> > I think this is unneeded.
> 
> Whatis ?  Setting the DMA direction or using flags?
> 
> Look at it this way:
> 
> Existing code that uses the first/last flags is wrong and needs to be
> changed.  Therefor code using 'first' and 'last' will be changed.
> 
> Passing the flags as pointers is bad practice and makes no sense to
> do.  Does it make any sense to re-write code fix the way it uses first
> and last, then re-write those same lines a second time to not use
> pointers?

You can always flip it the other way -- first rework the use of flags, then 
apply the fix. It'd make the patch much easier to understand, don't you think?

> If the way the flags are passed should change, then why not do it the
> most efficient way.  It isn't any more complicated and produces better
> code.  One flag per parameter becomes cumbersome when more flags are
> added for additional features.
> 
> I'm all for splitting my patches up.  I sent five patches when I bet 9
> out 10 developers would have just done one.  But I don't think it
> makes any sense to re-write the same line of code over and over again
> in successive patches in a series before getting to the final version.
>  It just makes more churn to review.

Splitting the patchset to make it more understandable is OK though. And I'm 
really getting lost in this patch.

> I hate it when I get a series of patches and spot a flaw, take the
> time to write up the flaw and how it should be fixed, only to discover
> that the flaw is fixed later on in the series and I wasted my time.

Best regards,
Marek Vasut

Re: [PATCH 1/5] spi/mxs: Fix extra CS pulses and read mode in multi-transfer messages

[Trent Piepho]

On Mon, Apr 1, 2013 at 9:22 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>> On Mon, Apr 1, 2013 at 4:11 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>> >> +#define TXRX_WRITE           1       /* This is a write */
>> >> +#define TXRX_DEASSERT_CS     2       /* De-assert CS at end of txrx */
>
> btw. is it necessary to define new flags or is it possible to reuse some?

I don't see any others that would make sense.  The driver doesn't
define any flags at all.  The spi layer doesn't have ones that match.
And this is for a private static interface inside the driver.  It
wouldn't be right to co-opt flags defined for some other purpose.

>> >>       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;
>> >
>> > Well ... ctrl0 &= ~(FOO | BAR | BAZ); ... won't cut it?
>>
>> Well, by De Morgan's law the two expressions are the same.  And they
>> are the same number of characters.  And both will produce a compile
>> time constant and thus the same code.  However, I like the ~FOO & ~BAR
>> & ~BAR form better as there is greater parallelism between each term
>> of the expression.
>
> What about the law of readability of code ? ;-)

Don't see anything in CodingStyle that one should be preferred over
the other.  I think this way is more readable than the other.
Generally you can think of masking off bits via bitwise-and and
setting bits with bitwise-or.  So if you want to do a sequence of
masks, then using a sequence of bitwise-ands is the most readable and
straightforward code IMHO.  Combing bits to mask with bitwise-or, then
inverting the set and masking with that seems like a less clear way of
doing the same thing.  And this way the existing tokens aren't
modified, so there is less churn!

>> The DMA does write
>> ctrl0 on each segment of each transfer, but look at the previous
>> paragraph to see where it comes from.  Once READ or IGNORE_CRC are
>> set, nothing will clear them until PIO mode is used.
>
> When using a flash, the DMA and PIO mode usage is intermixed. Usually, the PIO
> is used to program the command and then DMA to transmit the payload. It's hardly
> ever PIO only orr DMA only for the whole transfer.

Probably why the problem hasn't been noticed.  It's obvious from a
cursory examination of the driver that bits in ctrl0 are only SET in
the DMA code, never cleared.  The PIO code does clear them.

>> It's the same
>> with the bug fix for 3.9 that clears the XFER_COUNT bits in DMA mode.
>> Strangely, I didn't notice that bug since all my xfers are the same
>> length!  But I do switch between slaves, between read and write, and
>> use messages with multiple transfers, in DMA mode, all of which are
>> broken in the current driver.
>
> btw. are you using MX23 or MX28 to test this?

IMX28.

>> Existing code that uses the first/last flags is wrong and needs to be
>> changed.  Therefor code using 'first' and 'last' will be changed.
>>
>> Passing the flags as pointers is bad practice and makes no sense to
>> do.  Does it make any sense to re-write code fix the way it uses first
>> and last, then re-write those same lines a second time to not use
>> pointers?
>
> You can always flip it the other way -- first rework the use of flags, then
> apply the fix. It'd make the patch much easier to understand, don't you think?

So I should change 'first' to not be a pointer to an integer in one
patch, then delete the flag entirely in another patch?  What's the
point of changing code just to delete it immediately afterward?  I'd
call that useless churn.

It also makes a patch series a PITA to deal with, as a change to
intermediate patch 1 creates a conflict when trying to apply
intermediate patch 2, 3, 4 and so on.  Instead of spending 5 mins
creating V2 of one patch that needs a change, you have to speed an
hour fixing V2 of an entire series.  And that problem is only caused
by trying to create the extra intermediate stages, that no one
actually cares about and will never use, with various known flaws that
are already fixed.

------------------------------------------------------------------------------
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 1/5] spi/mxs: Fix extra CS pulses and read mode in multi-transfer messages

[Trent Piepho]

On Mon, Apr 1, 2013 at 9:22 PM, Marek Vasut <marex@denx.de> wrote:
>> On Mon, Apr 1, 2013 at 4:11 PM, Marek Vasut <marex@denx.de> wrote:
>> >> +#define TXRX_WRITE           1       /* This is a write */
>> >> +#define TXRX_DEASSERT_CS     2       /* De-assert CS at end of txrx */
>
> btw. is it necessary to define new flags or is it possible to reuse some?

I don't see any others that would make sense.  The driver doesn't
define any flags at all.  The spi layer doesn't have ones that match.
And this is for a private static interface inside the driver.  It
wouldn't be right to co-opt flags defined for some other purpose.

>> >>       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;
>> >
>> > Well ... ctrl0 &= ~(FOO | BAR | BAZ); ... won't cut it?
>>
>> Well, by De Morgan's law the two expressions are the same.  And they
>> are the same number of characters.  And both will produce a compile
>> time constant and thus the same code.  However, I like the ~FOO & ~BAR
>> & ~BAR form better as there is greater parallelism between each term
>> of the expression.
>
> What about the law of readability of code ? ;-)

Don't see anything in CodingStyle that one should be preferred over
the other.  I think this way is more readable than the other.
Generally you can think of masking off bits via bitwise-and and
setting bits with bitwise-or.  So if you want to do a sequence of
masks, then using a sequence of bitwise-ands is the most readable and
straightforward code IMHO.  Combing bits to mask with bitwise-or, then
inverting the set and masking with that seems like a less clear way of
doing the same thing.  And this way the existing tokens aren't
modified, so there is less churn!

>> The DMA does write
>> ctrl0 on each segment of each transfer, but look at the previous
>> paragraph to see where it comes from.  Once READ or IGNORE_CRC are
>> set, nothing will clear them until PIO mode is used.
>
> When using a flash, the DMA and PIO mode usage is intermixed. Usually, the PIO
> is used to program the command and then DMA to transmit the payload. It's hardly
> ever PIO only orr DMA only for the whole transfer.

Probably why the problem hasn't been noticed.  It's obvious from a
cursory examination of the driver that bits in ctrl0 are only SET in
the DMA code, never cleared.  The PIO code does clear them.

>> It's the same
>> with the bug fix for 3.9 that clears the XFER_COUNT bits in DMA mode.
>> Strangely, I didn't notice that bug since all my xfers are the same
>> length!  But I do switch between slaves, between read and write, and
>> use messages with multiple transfers, in DMA mode, all of which are
>> broken in the current driver.
>
> btw. are you using MX23 or MX28 to test this?

IMX28.

>> Existing code that uses the first/last flags is wrong and needs to be
>> changed.  Therefor code using 'first' and 'last' will be changed.
>>
>> Passing the flags as pointers is bad practice and makes no sense to
>> do.  Does it make any sense to re-write code fix the way it uses first
>> and last, then re-write those same lines a second time to not use
>> pointers?
>
> You can always flip it the other way -- first rework the use of flags, then
> apply the fix. It'd make the patch much easier to understand, don't you think?

So I should change 'first' to not be a pointer to an integer in one
patch, then delete the flag entirely in another patch?  What's the
point of changing code just to delete it immediately afterward?  I'd
call that useless churn.

It also makes a patch series a PITA to deal with, as a change to
intermediate patch 1 creates a conflict when trying to apply
intermediate patch 2, 3, 4 and so on.  Instead of spending 5 mins
creating V2 of one patch that needs a change, you have to speed an
hour fixing V2 of an entire series.  And that problem is only caused
by trying to create the extra intermediate stages, that no one
actually cares about and will never use, with various known flaws that
are already fixed.

Re: [PATCH 1/5] spi/mxs: Fix extra CS pulses and read mode in multi-transfer messages

[Marek Vasut]

Dear Trent Piepho,

> On Mon, Apr 1, 2013 at 9:22 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> >> On Mon, Apr 1, 2013 at 4:11 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> >> >> +#define TXRX_WRITE           1       /* This is a write */
> >> >> +#define TXRX_DEASSERT_CS     2       /* De-assert CS at end of txrx
> >> >> */
> > 
> > btw. is it necessary to define new flags or is it possible to reuse some?
> 
> I don't see any others that would make sense.  The driver doesn't
> define any flags at all.  The spi layer doesn't have ones that match.
> And this is for a private static interface inside the driver.  It
> wouldn't be right to co-opt flags defined for some other purpose.

Ok, makes sense then.

btw. if defining bitfield flags, it makes more sense to use (1 << n) notation to 
make it more readable and also less prone to people using the actual flag value 
as a bitshift argument.

> >> >>       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;
> >> > 
> >> > Well ... ctrl0 &= ~(FOO | BAR | BAZ); ... won't cut it?
> >> 
> >> Well, by De Morgan's law the two expressions are the same.  And they
> >> are the same number of characters.  And both will produce a compile
> >> time constant and thus the same code.  However, I like the ~FOO & ~BAR
> >> & ~BAR form better as there is greater parallelism between each term
> >> of the expression.
> > 
> > What about the law of readability of code ? ;-)
> 
> Don't see anything in CodingStyle that one should be preferred over
> the other.

There ain't any I'm aware of, but to paraphrase you, let's keep the format 
that's already used in the driver ;-) :

114         if (dev->mode & ~(SPI_CPOL | SPI_CPHA))

> I think this way is more readable than the other.
> Generally you can think of masking off bits via bitwise-and and
> setting bits with bitwise-or.  So if you want to do a sequence of
> masks, then using a sequence of bitwise-ands is the most readable and
> straightforward code IMHO.  Combing bits to mask with bitwise-or, then
> inverting the set and masking with that seems like a less clear way of
> doing the same thing.  And this way the existing tokens aren't
> modified, so there is less churn!
> 
> >> The DMA does write
> >> ctrl0 on each segment of each transfer, but look at the previous
> >> paragraph to see where it comes from.  Once READ or IGNORE_CRC are
> >> set, nothing will clear them until PIO mode is used.
> > 
> > When using a flash, the DMA and PIO mode usage is intermixed. Usually,
> > the PIO is used to program the command and then DMA to transmit the
> > payload. It's hardly ever PIO only orr DMA only for the whole transfer.
> 
> Probably why the problem hasn't been noticed.  It's obvious from a
> cursory examination of the driver that bits in ctrl0 are only SET in
> the DMA code, never cleared.  The PIO code does clear them.
> 
> >> It's the same
> >> with the bug fix for 3.9 that clears the XFER_COUNT bits in DMA mode.
> >> Strangely, I didn't notice that bug since all my xfers are the same
> >> length!  But I do switch between slaves, between read and write, and
> >> use messages with multiple transfers, in DMA mode, all of which are
> >> broken in the current driver.
> > 
> > btw. are you using MX23 or MX28 to test this?
> 
> IMX28.

Is that any mainline board?

> >> Existing code that uses the first/last flags is wrong and needs to be
> >> changed.  Therefor code using 'first' and 'last' will be changed.
> >> 
> >> Passing the flags as pointers is bad practice and makes no sense to
> >> do.  Does it make any sense to re-write code fix the way it uses first
> >> and last, then re-write those same lines a second time to not use
> >> pointers?
> > 
> > You can always flip it the other way -- first rework the use of flags,
> > then apply the fix. It'd make the patch much easier to understand, don't
> > you think?
> 
> So I should change 'first' to not be a pointer to an integer in one
> patch, then delete the flag entirely in another patch?

I'd say make a patch (0001) that implements the transition to using your newly 
defined flags and then make a patch (0002) that "Fix extra CS pulses and read 
mode in multi-transfer messages". One patch shall do one thing, that's the usual 
rule of the thumb. Obviously keep them in a series if these patches shall go in 
together. And why doesn't squashing them all together work you might ask -- 
because reviewing the result is hard.

[...]

Best regards,
Marek Vasut

------------------------------------------------------------------------------
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 1/5] spi/mxs: Fix extra CS pulses and read mode in multi-transfer messages

[Marek Vasut]

Dear Trent Piepho,

> On Mon, Apr 1, 2013 at 9:22 PM, Marek Vasut <marex@denx.de> wrote:
> >> On Mon, Apr 1, 2013 at 4:11 PM, Marek Vasut <marex@denx.de> wrote:
> >> >> +#define TXRX_WRITE           1       /* This is a write */
> >> >> +#define TXRX_DEASSERT_CS     2       /* De-assert CS at end of txrx
> >> >> */
> > 
> > btw. is it necessary to define new flags or is it possible to reuse some?
> 
> I don't see any others that would make sense.  The driver doesn't
> define any flags at all.  The spi layer doesn't have ones that match.
> And this is for a private static interface inside the driver.  It
> wouldn't be right to co-opt flags defined for some other purpose.

Ok, makes sense then.

btw. if defining bitfield flags, it makes more sense to use (1 << n) notation to 
make it more readable and also less prone to people using the actual flag value 
as a bitshift argument.

> >> >>       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;
> >> > 
> >> > Well ... ctrl0 &= ~(FOO | BAR | BAZ); ... won't cut it?
> >> 
> >> Well, by De Morgan's law the two expressions are the same.  And they
> >> are the same number of characters.  And both will produce a compile
> >> time constant and thus the same code.  However, I like the ~FOO & ~BAR
> >> & ~BAR form better as there is greater parallelism between each term
> >> of the expression.
> > 
> > What about the law of readability of code ? ;-)
> 
> Don't see anything in CodingStyle that one should be preferred over
> the other.

There ain't any I'm aware of, but to paraphrase you, let's keep the format 
that's already used in the driver ;-) :

114         if (dev->mode & ~(SPI_CPOL | SPI_CPHA))

> I think this way is more readable than the other.
> Generally you can think of masking off bits via bitwise-and and
> setting bits with bitwise-or.  So if you want to do a sequence of
> masks, then using a sequence of bitwise-ands is the most readable and
> straightforward code IMHO.  Combing bits to mask with bitwise-or, then
> inverting the set and masking with that seems like a less clear way of
> doing the same thing.  And this way the existing tokens aren't
> modified, so there is less churn!
> 
> >> The DMA does write
> >> ctrl0 on each segment of each transfer, but look at the previous
> >> paragraph to see where it comes from.  Once READ or IGNORE_CRC are
> >> set, nothing will clear them until PIO mode is used.
> > 
> > When using a flash, the DMA and PIO mode usage is intermixed. Usually,
> > the PIO is used to program the command and then DMA to transmit the
> > payload. It's hardly ever PIO only orr DMA only for the whole transfer.
> 
> Probably why the problem hasn't been noticed.  It's obvious from a
> cursory examination of the driver that bits in ctrl0 are only SET in
> the DMA code, never cleared.  The PIO code does clear them.
> 
> >> It's the same
> >> with the bug fix for 3.9 that clears the XFER_COUNT bits in DMA mode.
> >> Strangely, I didn't notice that bug since all my xfers are the same
> >> length!  But I do switch between slaves, between read and write, and
> >> use messages with multiple transfers, in DMA mode, all of which are
> >> broken in the current driver.
> > 
> > btw. are you using MX23 or MX28 to test this?
> 
> IMX28.

Is that any mainline board?

> >> Existing code that uses the first/last flags is wrong and needs to be
> >> changed.  Therefor code using 'first' and 'last' will be changed.
> >> 
> >> Passing the flags as pointers is bad practice and makes no sense to
> >> do.  Does it make any sense to re-write code fix the way it uses first
> >> and last, then re-write those same lines a second time to not use
> >> pointers?
> > 
> > You can always flip it the other way -- first rework the use of flags,
> > then apply the fix. It'd make the patch much easier to understand, don't
> > you think?
> 
> So I should change 'first' to not be a pointer to an integer in one
> patch, then delete the flag entirely in another patch?

I'd say make a patch (0001) that implements the transition to using your newly 
defined flags and then make a patch (0002) that "Fix extra CS pulses and read 
mode in multi-transfer messages". One patch shall do one thing, that's the usual 
rule of the thumb. Obviously keep them in a series if these patches shall go in 
together. And why doesn't squashing them all together work you might ask -- 
because reviewing the result is hard.

[...]

Best regards,
Marek Vasut

Re: [PATCH 1/5] spi/mxs: Fix extra CS pulses and read mode in multi-transfer messages

[Trent Piepho]

On Tue, Apr 2, 2013 at 3:32 AM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>>
>> Don't see anything in CodingStyle that one should be preferred over
>> the other.
>
> There ain't any I'm aware of, but to paraphrase you, let's keep the format
> that's already used in the driver ;-) :
>
> 114         if (dev->mode & ~(SPI_CPOL | SPI_CPHA))

Thanks for pointing that out, as that code can be deleted.  Which
means I don't need to be consistent with it anymore and can create a
new standard, to be followed henceforth, now and forever.

But it looks like the or method is more common in the kernel code.  So
I'll change it in V2.

>> > btw. are you using MX23 or MX28 to test this?
>>
>> IMX28.
>
> Is that any mainline board?

Nope, custom hardware.

>> >> Existing code that uses the first/last flags is wrong and needs to be
>> >> changed.  Therefor code using 'first' and 'last' will be changed.
>> >>
>> >> Passing the flags as pointers is bad practice and makes no sense to
>> >> do.  Does it make any sense to re-write code fix the way it uses first
>> >> and last, then re-write those same lines a second time to not use
>> >> pointers?
>> >
>> > You can always flip it the other way -- first rework the use of flags,
>> > then apply the fix. It'd make the patch much easier to understand, don't
>> > you think?
>>
>> So I should change 'first' to not be a pointer to an integer in one
>> patch, then delete the flag entirely in another patch?
>
> I'd say make a patch (0001) that implements the transition to using your newly
> defined flags and then make a patch (0002) that "Fix extra CS pulses and read
> mode in multi-transfer messages". One patch shall do one thing, that's the usual
> rule of the thumb. Obviously keep them in a series if these patches shall go in
> together. And why doesn't squashing them all together work you might ask --
> because reviewing the result is hard.

5 patches have now been split into 12.

------------------------------------------------------------------------------
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 1/5] spi/mxs: Fix extra CS pulses and read mode in multi-transfer messages

[Trent Piepho]

On Tue, Apr 2, 2013 at 3:32 AM, Marek Vasut <marex@denx.de> wrote:
>>
>> Don't see anything in CodingStyle that one should be preferred over
>> the other.
>
> There ain't any I'm aware of, but to paraphrase you, let's keep the format
> that's already used in the driver ;-) :
>
> 114         if (dev->mode & ~(SPI_CPOL | SPI_CPHA))

Thanks for pointing that out, as that code can be deleted.  Which
means I don't need to be consistent with it anymore and can create a
new standard, to be followed henceforth, now and forever.

But it looks like the or method is more common in the kernel code.  So
I'll change it in V2.

>> > btw. are you using MX23 or MX28 to test this?
>>
>> IMX28.
>
> Is that any mainline board?

Nope, custom hardware.

>> >> Existing code that uses the first/last flags is wrong and needs to be
>> >> changed.  Therefor code using 'first' and 'last' will be changed.
>> >>
>> >> Passing the flags as pointers is bad practice and makes no sense to
>> >> do.  Does it make any sense to re-write code fix the way it uses first
>> >> and last, then re-write those same lines a second time to not use
>> >> pointers?
>> >
>> > You can always flip it the other way -- first rework the use of flags,
>> > then apply the fix. It'd make the patch much easier to understand, don't
>> > you think?
>>
>> So I should change 'first' to not be a pointer to an integer in one
>> patch, then delete the flag entirely in another patch?
>
> I'd say make a patch (0001) that implements the transition to using your newly
> defined flags and then make a patch (0002) that "Fix extra CS pulses and read
> mode in multi-transfer messages". One patch shall do one thing, that's the usual
> rule of the thumb. Obviously keep them in a series if these patches shall go in
> together. And why doesn't squashing them all together work you might ask --
> because reviewing the result is hard.

5 patches have now been split into 12.

Re: [PATCH 1/5] spi/mxs: Fix extra CS pulses and read mode in multi-transfer messages

[Marek Vasut]

Dear Trent Piepho,

> On Tue, Apr 2, 2013 at 3:32 AM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> >> Don't see anything in CodingStyle that one should be preferred over
> >> the other.
> > 
> > There ain't any I'm aware of, but to paraphrase you, let's keep the
> > format that's already used in the driver ;-) :
> > 
> > 114         if (dev->mode & ~(SPI_CPOL | SPI_CPHA))
> 
> Thanks for pointing that out, as that code can be deleted.  Which
> means I don't need to be consistent with it anymore and can create a
> new standard, to be followed henceforth, now and forever.
> 
> But it looks like the or method is more common in the kernel code.  So
> I'll change it in V2.
> 
> >> > btw. are you using MX23 or MX28 to test this?
> >> 
> >> IMX28.
> > 
> > Is that any mainline board?
> 
> Nope, custom hardware.
> 
> >> >> Existing code that uses the first/last flags is wrong and needs to be
> >> >> changed.  Therefor code using 'first' and 'last' will be changed.
> >> >> 
> >> >> Passing the flags as pointers is bad practice and makes no sense to
> >> >> do.  Does it make any sense to re-write code fix the way it uses
> >> >> first and last, then re-write those same lines a second time to not
> >> >> use pointers?
> >> > 
> >> > You can always flip it the other way -- first rework the use of flags,
> >> > then apply the fix. It'd make the patch much easier to understand,
> >> > don't you think?
> >> 
> >> So I should change 'first' to not be a pointer to an integer in one
> >> patch, then delete the flag entirely in another patch?
> > 
> > I'd say make a patch (0001) that implements the transition to using your
> > newly defined flags and then make a patch (0002) that "Fix extra CS
> > pulses and read mode in multi-transfer messages". One patch shall do one
> > thing, that's the usual rule of the thumb. Obviously keep them in a
> > series if these patches shall go in together. And why doesn't squashing
> > them all together work you might ask -- because reviewing the result is
> > hard.
> 
> 5 patches have now been split into 12.

Much better, yes.

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 1/5] spi/mxs: Fix extra CS pulses and read mode in multi-transfer messages

[Marek Vasut]

Dear Trent Piepho,

> On Tue, Apr 2, 2013 at 3:32 AM, Marek Vasut <marex@denx.de> wrote:
> >> Don't see anything in CodingStyle that one should be preferred over
> >> the other.
> > 
> > There ain't any I'm aware of, but to paraphrase you, let's keep the
> > format that's already used in the driver ;-) :
> > 
> > 114         if (dev->mode & ~(SPI_CPOL | SPI_CPHA))
> 
> Thanks for pointing that out, as that code can be deleted.  Which
> means I don't need to be consistent with it anymore and can create a
> new standard, to be followed henceforth, now and forever.
> 
> But it looks like the or method is more common in the kernel code.  So
> I'll change it in V2.
> 
> >> > btw. are you using MX23 or MX28 to test this?
> >> 
> >> IMX28.
> > 
> > Is that any mainline board?
> 
> Nope, custom hardware.
> 
> >> >> Existing code that uses the first/last flags is wrong and needs to be
> >> >> changed.  Therefor code using 'first' and 'last' will be changed.
> >> >> 
> >> >> Passing the flags as pointers is bad practice and makes no sense to
> >> >> do.  Does it make any sense to re-write code fix the way it uses
> >> >> first and last, then re-write those same lines a second time to not
> >> >> use pointers?
> >> > 
> >> > You can always flip it the other way -- first rework the use of flags,
> >> > then apply the fix. It'd make the patch much easier to understand,
> >> > don't you think?
> >> 
> >> So I should change 'first' to not be a pointer to an integer in one
> >> patch, then delete the flag entirely in another patch?
> > 
> > I'd say make a patch (0001) that implements the transition to using your
> > newly defined flags and then make a patch (0002) that "Fix extra CS
> > pulses and read mode in multi-transfer messages". One patch shall do one
> > thing, that's the usual rule of the thumb. Obviously keep them in a
> > series if these patches shall go in together. And why doesn't squashing
> > them all together work you might ask -- because reviewing the result is
> > hard.
> 
> 5 patches have now been split into 12.

Much better, yes.

Best regards,
Marek Vasut