[PATCH 01/13] spi: spi-mxs: Always set LOCK_CS

Trent Piepho

author

2013-10-01 20:14 UTC #0 lore raw

PATCH 1/13 spi: spi-mxs: Always set LOCK_CS

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 PIO code will explicitly set it for the first transfer, leave it set for intermediate transfers, and then clear it for the final transfer. It should not clear it.

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

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

Signed-off-by Trent Piepho <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Shawn Guo <[email protected]>

---
 drivers/spi/spi-mxs.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

drivers/spi/spi-mxs.c +2 -6

index de7b114..e6172ae 100644
@@ -79,6 +79,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) |

@@ -147,8 +149,6 @@ static inline void mxs_spi_enable(struct mxs_spi *spi)
 {
 	struct mxs_ssp *ssp = &spi->ssp;
 
-	writel(BM_SSP_CTRL0_LOCK_CS,
-		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 	writel(BM_SSP_CTRL0_IGNORE_CRC,
 		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
 }

@@ -157,8 +157,6 @@ static inline void mxs_spi_disable(struct mxs_spi *spi)
 {
 	struct mxs_ssp *ssp = &spi->ssp;
 
-	writel(BM_SSP_CTRL0_LOCK_CS,
-		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
 	writel(BM_SSP_CTRL0_IGNORE_CRC,
 		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 }

@@ -232,8 +230,6 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
 	ctrl0 &= ~BM_SSP_CTRL0_XFER_COUNT;
 	ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
 
-	if (*first)
-		ctrl0 |= BM_SSP_CTRL0_LOCK_CS;
 	if (!write)
 		ctrl0 |= BM_SSP_CTRL0_READ;

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk

Trent Piepho

author

2013-10-01 20:14 UTC #1 lore raw

PATCH 2/13 spi: spi-mxs: Remove mxs_spi_enable and mxs_spi_disable

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

Signed-off-by Trent Piepho <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Shawn Guo <[email protected]>

---
 drivers/spi/spi-mxs.c |   22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

drivers/spi/spi-mxs.c +4 ~~-18~~

index e6172ae..991ee01 100644
@@ -145,22 +145,6 @@ static void mxs_spi_set_cs(struct mxs_spi *spi, unsigned cs)
 	writel(select, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 }
 
-static inline void mxs_spi_enable(struct mxs_spi *spi)
-{
-	struct mxs_ssp *ssp = &spi->ssp;
-
-	writel(BM_SSP_CTRL0_IGNORE_CRC,
-		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
-}
-
-static inline void mxs_spi_disable(struct mxs_spi *spi)
-{
-	struct mxs_ssp *ssp = &spi->ssp;
-
-	writel(BM_SSP_CTRL0_IGNORE_CRC,
-		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
-}
-
 static int mxs_ssp_wait(struct mxs_spi *spi, int offset, int mask, bool set)
 {
 	const unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT);

@@ -335,13 +319,15 @@ static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
 	struct mxs_ssp *ssp = &spi->ssp;
 
 	if (*first)
-		mxs_spi_enable(spi);
+		writel(BM_SSP_CTRL0_IGNORE_CRC,
+		       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
 
 	mxs_spi_set_cs(spi, cs);
 
 	while (len--) {
 		if (*last && len == 0)
-			mxs_spi_disable(spi);
+			writel(BM_SSP_CTRL0_IGNORE_CRC,
+			       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
 
 		if (ssp->devid == IMX23_SSP) {
 			writel(BM_SSP_CTRL0_XFER_COUNT,

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk

Trent Piepho

author

2013-10-01 20:14 UTC #2 lore raw

PATCH 3/13 spi: spi-mxs: Always clear INGORE_CRC, to keep CS asserted

In reply to Trent Piepho

INGORE_CRC, better named DEASSERT_CS, should be cleared on all tranfers except the last. So instead of only clearing it on the first transfer, we can just always clear it. It will set on the last transfer.

This removes the only use of the "first" flag in the transfer functions, so that flag can be then be removed.

Signed-off-by Trent Piepho <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Shawn Guo <[email protected]>

---
 drivers/spi/spi-mxs.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/spi/spi-mxs.c +2 -3

index 991ee01..e2a9cc2 100644
@@ -318,9 +318,8 @@ static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
 {
 	struct mxs_ssp *ssp = &spi->ssp;
 
-	if (*first)
-		writel(BM_SSP_CTRL0_IGNORE_CRC,
-		       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
+	writel(BM_SSP_CTRL0_IGNORE_CRC,
+	       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
 
 	mxs_spi_set_cs(spi, cs);

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk

Trent Piepho

author

2013-10-01 20:14 UTC #3 lore raw

PATCH 4/13 spi: spi-mxs: Change flag arguments in txrx functions to bit flags

In reply to Trent Piepho

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

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

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

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

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

Signed-off-by Trent Piepho <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Shawn Guo <[email protected]>

---
 drivers/spi/spi-mxs.c |   55 ++++++++++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 24 deletions(-)

drivers/spi/spi-mxs.c +31 ~~-24~~

index e2a9cc2..090930a 100644
@@ -57,6 +57,13 @@
 
 #define SG_MAXLEN		0xff00
 
+/*
+ * Flags for txrx functions.  More efficient that using an argument register for
+ * each one.
+ */
+#define TXRX_WRITE		(1<<0)	/* This is a write */
+#define TXRX_DEASSERT_CS	(1<<1)	/* De-assert CS at end of txrx */
+
 struct mxs_spi {
 	struct mxs_ssp		ssp;
 	struct completion	c;

@@ -184,7 +191,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;

@@ -214,15 +221,19 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
 	ctrl0 &= ~BM_SSP_CTRL0_XFER_COUNT;
 	ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
 
-	if (!write)
+	if (!(flags & TXRX_WRITE))
 		ctrl0 |= BM_SSP_CTRL0_READ;
 
 	/* Queue the DMA data transfer. */
 	for (sg_count = 0; sg_count < sgs; sg_count++) {
+		/* Prepare the transfer descriptor. */
 		min = min(len, desc_len);
 
-		/* Prepare the transfer descriptor. */
-		if ((sg_count + 1 == sgs) && *last)
+		/*
+		 * De-assert CS on last segment if flag is set (i.e., no more
+		 * transfers will follow)
+		 */
+		if ((sg_count + 1 == sgs) && (flags & TXRX_DEASSERT_CS))
 			ctrl0 |= BM_SSP_CTRL0_IGNORE_CRC;
 
 		if (ssp->devid == IMX23_SSP) {

@@ -247,7 +258,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;

@@ -267,7 +278,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) {

@@ -304,7 +315,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);

@@ -314,7 +325,7 @@ err_mapped:
 
 static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
 			    unsigned char *buf, int len,
-			    int *first, int *last, int write)
+			    unsigned int flags)
 {
 	struct mxs_ssp *ssp = &spi->ssp;

@@ -324,7 +335,7 @@ static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
 	mxs_spi_set_cs(spi, cs);
 
 	while (len--) {
-		if (*last && len == 0)
+		if (len == 0 && (flags & TXRX_DEASSERT_CS))
 			writel(BM_SSP_CTRL0_IGNORE_CRC,
 			       ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);

@@ -337,7 +348,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

@@ -350,13 +361,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;

@@ -381,13 +392,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) {

@@ -396,10 +405,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");

@@ -424,11 +432,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) +

@@ -437,11 +445,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) {

@@ -450,7 +458,6 @@ static int mxs_spi_transfer_one(struct spi_master *master,
 		}
 
 		m->actual_length += t->len;
-		first = last = 0;
 	}
 
 	m->status = status;

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk

Trent Piepho

author

2013-10-01 20:14 UTC #4 lore raw

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

In reply to Trent Piepho

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

Setting DEASSERT_CS causes CS to be de-asserted at the end of the transfer. It should normally be set only for 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, it will remain set for all transfers in subsequent messages. This results in a CS pulse between transfers.

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

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

Signed-off-by Trent Piepho <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Shawn Guo <[email protected]>

---
 drivers/spi/spi-mxs.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/spi/spi-mxs.c +2 -1

index 090930a..68ea507 100644
@@ -218,7 +218,8 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi, int cs,
 	INIT_COMPLETION(spi->c);
 
 	ctrl0 = readl(ssp->base + HW_SSP_CTRL0);
-	ctrl0 &= ~BM_SSP_CTRL0_XFER_COUNT;
+	ctrl0 &= ~(BM_SSP_CTRL0_XFER_COUNT | BM_SSP_CTRL0_IGNORE_CRC |
+		 BM_SSP_CTRL0_READ);
 	ctrl0 |= BM_SSP_CTRL0_DATA_XFER | mxs_spi_cs_to_reg(cs);
 
 	if (!(flags & TXRX_WRITE))

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk

Trent Piepho

author

2013-10-01 20:15 UTC #5 lore raw

PATCH 6/13 spi: spi-mxs: Fix chip select control bits in DMA mode

In reply to 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 CTRL0 bit to address slave 1 would be still be set when addressing slave 0, resulting in slave 1 continuing to be addressed.

The message handling 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 handling function once. Now the DMA and PIO txrx functions don't need to care about CS at all.

Signed-off-by Trent Piepho <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Shawn Guo <[email protected]>

---
 drivers/spi/spi-mxs.c |   36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

drivers/spi/spi-mxs.c +13 ~~-23~~

index 68ea507..a9a273e 100644
@@ -140,18 +140,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);

@@ -189,7 +177,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)
 {

@@ -217,10 +205,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;

@@ -324,7 +313,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)
 {

@@ -333,8 +322,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,

@@ -396,9 +383,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) {

@@ -431,11 +421,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 {

@@ -444,11 +434,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);
 		}

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk

Trent Piepho

author

2013-10-01 20:15 UTC #6 lore raw

PATCH 7/13 spi: spi-mxs: Remove full duplex check, spi core already does it

In reply to 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 spi-mxs driver as well.

Signed-off-by Trent Piepho <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Shawn Guo <[email protected]>

---
 drivers/spi/spi-mxs.c |    6 ------
 1 file changed, 6 deletions(-)

drivers/spi/spi-mxs.c +0 -6

index a9a273e..de7387e 100644
@@ -399,12 +399,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.

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk

Trent Piepho

author

2013-10-01 20:15 UTC #7 lore raw

PATCH 8/13 spi: spi-mxs: Remove bogus setting of ssp clk rate field

In reply to 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 is set by mxs_ssp_set_clk_rate(), for SSP using drivers (like SPI and MMC) to *read* if they want to know the actual clock rate. The SPI driver isn't supposed to *write* to it.

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 <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Shawn Guo <[email protected]>

---
 drivers/spi/spi-mxs.c |    1 -
 1 file changed, 1 deletion(-)

drivers/spi/spi-mxs.c +0 -1

index de7387e..8cb5d8b 100644
@@ -536,7 +536,6 @@ static int mxs_spi_probe(struct platform_device *pdev)
 		goto out_dma_release;
 
 	clk_set_rate(ssp->clk, clk_freq);
-	ssp->clk_rate = clk_get_rate(ssp->clk) / 1000;
 
 	ret = stmp_reset_block(ssp->base);
 	if (ret)

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk

Trent Piepho

author

2013-10-01 20:15 UTC #8 lore raw

PATCH 9/13 spi: spi-mxs: Fix race in setup method

In reply to 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(). mxs_spi_setup_transfer() is already called for each transfer when they are actually performed in mxs_spi_transfer_one(), so the call in mxs_spi_setup() isn't necessary to setup anything.

Signed-off-by Trent Piepho <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Shawn Guo <[email protected]>

---
 drivers/spi/spi-mxs.c |   10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

drivers/spi/spi-mxs.c +1 -9

index 8cb5d8b..e9cbdb6 100644
@@ -103,21 +103,13 @@ static int mxs_spi_setup_transfer(struct spi_device *dev,
 
 static int mxs_spi_setup(struct spi_device *dev)
 {
-	int err = 0;
-
 	if (!dev->bits_per_word)
 		dev->bits_per_word = 8;
 
 	if (dev->mode & ~(SPI_CPOL | SPI_CPHA))
 		return -EINVAL;
 
-	err = mxs_spi_setup_transfer(dev, NULL);
-	if (err) {
-		dev_err(&dev->dev,
-			"Failed to setup transfer, error = %d\n", err);
-	}
-
-	return err;
+	return 0;
 }
 
 static uint32_t mxs_spi_cs_to_reg(unsigned cs)

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk

Trent Piepho

author

2013-10-01 20:15 UTC #9 lore raw

PATCH 10/13 spi: spi-mxs: Remove check of spi mode bits

In reply to Trent Piepho

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

Signed-off-by Trent Piepho <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Shawn Guo <[email protected]>

---
 drivers/spi/spi-mxs.c |    3 ---
 1 file changed, 3 deletions(-)

drivers/spi/spi-mxs.c +0 -3

index e9cbdb6..23cc412 100644
@@ -106,9 +106,6 @@ static int mxs_spi_setup(struct spi_device *dev)
 	if (!dev->bits_per_word)
 		dev->bits_per_word = 8;
 
-	if (dev->mode & ~(SPI_CPOL | SPI_CPHA))
-		return -EINVAL;
-
 	return 0;
 }

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk

Trent Piepho

author

2013-10-01 20:15 UTC #10 lore raw

PATCH 11/13 spi: spi-mxs: Clean up setup_transfer function

In reply to Trent Piepho

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

Fix indention of line-wrapped code to Linux standard.

The transfer pointer can be const.

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

Signed-off-by Trent Piepho <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Shawn Guo <[email protected]>

---
 drivers/spi/spi-mxs.c |   19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

drivers/spi/spi-mxs.c +8 ~~-11~~

index 23cc412..9e6101a 100644
@@ -70,17 +70,14 @@ struct mxs_spi {
 };
 
 static int mxs_spi_setup_transfer(struct spi_device *dev,
-				struct spi_transfer *t)
+				  const struct spi_transfer *t)
 {
 	struct mxs_spi *spi = spi_master_get_devdata(dev->master);
 	struct mxs_ssp *ssp = &spi->ssp;
-	uint32_t hz = 0;
+	const unsigned int hz = min(dev->max_speed_hz, t->speed_hz);
 
-	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 allowed\n");
 		return -EINVAL;
 	}

@@ -88,12 +85,12 @@ static int mxs_spi_setup_transfer(struct spi_device *dev,
 
 	writel(BM_SSP_CTRL0_LOCK_CS,
 		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+
 	writel(BF_SSP_CTRL1_SSP_MODE(BV_SSP_CTRL1_SSP_MODE__SPI) |
-		     BF_SSP_CTRL1_WORD_LENGTH
-		     (BV_SSP_CTRL1_WORD_LENGTH__EIGHT_BITS) |
-		     ((dev->mode & SPI_CPOL) ? BM_SSP_CTRL1_POLARITY : 0) |
-		     ((dev->mode & SPI_CPHA) ? BM_SSP_CTRL1_PHASE : 0),
-		     ssp->base + HW_SSP_CTRL1(ssp));
+	       BF_SSP_CTRL1_WORD_LENGTH(BV_SSP_CTRL1_WORD_LENGTH__EIGHT_BITS) |
+	       ((dev->mode & SPI_CPOL) ? BM_SSP_CTRL1_POLARITY : 0) |
+	       ((dev->mode & SPI_CPHA) ? BM_SSP_CTRL1_PHASE : 0),
+	       ssp->base + HW_SSP_CTRL1(ssp));
 
 	writel(0x0, ssp->base + HW_SSP_CMD0);
 	writel(0x0, ssp->base + HW_SSP_CMD1);

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk

Trent Piepho

author

2013-10-01 20:15 UTC #11 lore raw

PATCH 12/13 spi: spi-mxs: Don't set clock for each xfer

In reply to Trent Piepho

mxs_spi_setup_transfer() would set the SSP SCK rate every time it was called, which is before every transfer. It is uncommon for the SCK rate to change between transfers (or at all of that matter) and this causes many unnecessary reprogrammings of the clock registers.

Code changed to only set the rate when it changes. This significantly speeds up short SPI messages, especially messages made up of many transfers, as the calculation of the clock divisors is rather costly. 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 <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Shawn Guo <[email protected]>

---
 drivers/spi/spi-mxs.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

drivers/spi/spi-mxs.c +14 -1

index 9e6101a..759de70 100644
@@ -67,6 +67,7 @@
 struct mxs_spi {
 	struct mxs_ssp		ssp;
 	struct completion	c;
+	unsigned int		sck;	/* Rate requested (vs actual) */
 };
 
 static int mxs_spi_setup_transfer(struct spi_device *dev,

@@ -81,7 +82,19 @@ static int mxs_spi_setup_transfer(struct spi_device *dev,
 		return -EINVAL;
 	}
 
-	mxs_ssp_set_clk_rate(ssp, hz);
+	if (hz != spi->sck) {
+		mxs_ssp_set_clk_rate(ssp, hz);
+		/*
+		 * Save requested rate, hz, rather than the actual rate,
+		 * ssp->clk_rate.  Otherwise we would set the rate every trasfer
+		 * when the actual rate is not quite the same as requested rate.
+		 */
+		spi->sck = hz;
+		/*
+		 * Perhaps we should return an error if the actual clock is
+		 * nowhere close to what was requested?
+		 */
+	}
 
 	writel(BM_SSP_CTRL0_LOCK_CS,
 		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk

Trent Piepho

author

2013-10-01 20:15 UTC #12 lore raw

PATCH 13/13 spi: spi-mxs: Use u32 instead of uint32_t

In reply to Trent Piepho

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

Signed-off-by Trent Piepho <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Shawn Guo <[email protected]>

---
 drivers/spi/spi-mxs.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/spi/spi-mxs.c +5 -5

index 759de70..2a819f7 100644
@@ -119,9 +119,9 @@ static int mxs_spi_setup(struct spi_device *dev)
 	return 0;
 }
 
-static uint32_t mxs_spi_cs_to_reg(unsigned cs)
+static u32 mxs_spi_cs_to_reg(unsigned cs)
 {
-	uint32_t select = 0;
+	u32 select = 0;
 
 	/*
 	 * i.MX28 Datasheet: 17.10.1: HW_SSP_CTRL0

@@ -143,7 +143,7 @@ static int mxs_ssp_wait(struct mxs_spi *spi, int offset, int mask, bool set)
 {
 	const unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT);
 	struct mxs_ssp *ssp = &spi->ssp;
-	uint32_t reg;
+	u32 reg;
 
 	do {
 		reg = readl_relaxed(ssp->base + offset);

@@ -187,11 +187,11 @@ static int mxs_spi_txrx_dma(struct mxs_spi *spi,
 	const int sgs = DIV_ROUND_UP(len, desc_len);
 	int sg_count;
 	int min, ret;
-	uint32_t ctrl0;
+	u32 ctrl0;
 	struct page *vm_page;
 	void *sg_buf;
 	struct {
-		uint32_t		pio[4];
+		u32			pio[4];
 		struct scatterlist	sg;
 	} *dma_xfer;

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk

Marek Vasut

2013-10-03 16:02 UTC #13 lore raw

In reply to Trent Piepho

Dear Trent Piepho,

Quoted 20 lines

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 PIO code will explicitly set it for the first transfer, leave it set for intermediate transfers, and then clear it for the final transfer. It should not clear it.
The only reason to not set LOCK_CS would be to attempt an altered protocol where CS pulses between each word. Though don't get your hopes up if you want to do this, as the hardware doesn't appear to do this in any sane manner. It appears to be related to the hardware FIFO fill level.

Quoted 7 lines

The code can be simplified by just setting LOCK_CS once and then not needing to deal with it at all in the PIO and DMA transfer functions.
Signed-off-by Trent Piepho <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Shawn Guo <[email protected]>

[...]

This is not V1 of patches, but changelog is missing.

Otherwise, I'm out of the office, so I'll review and test on monday earliest.

Best regards,
Marek Vasut

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk