[U-Boot] [PATCH 0/5] DW SPI: fixes and improvements

[U-Boot] [PATCH 0/5] DW SPI: fixes and improvements

[Eugeniy Paltsev]

Various fixes and improvements of designware spi driver.

Eugeniy Paltsev (5):
  DW SPI: fix tx data loss on FIFO flush
  DW SPI: fix transmit only mode
  DW SPI: refactor poll_transfer functions
  DW SPI: add option to use external gpio for chip select
  DW SPI: use 32 bit access instead of 16 and 32 bit mix

 drivers/spi/designware_spi.c | 132 ++++++++++++++++++++++++++++---------------
 1 file changed, 87 insertions(+), 45 deletions(-)

-- 
2.14.3

[U-Boot] [PATCH 1/5] DW SPI: fix tx data loss on FIFO flush

[Eugeniy Paltsev]

In current implementation if some data still exists in Tx FIFO it
can be silently flushed, i.e. dropped on disabling of the controller,
which happens when writing 0 to DW_SPI_SSIENR (it happens in the
beginning of new transfer)

So add wait for current transmit operation to complete to be sure
that current transmit operation is finished before new one.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 drivers/spi/designware_spi.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index c501aeea16..71d8839d55 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -338,6 +338,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 {
 	struct udevice *bus = dev->parent;
 	struct dw_spi_priv *priv = dev_get_priv(bus);
+	unsigned long start;
 	const u8 *tx = dout;
 	u8 *rx = din;
 	int ret = 0;
@@ -394,6 +395,22 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	/* Start transfer in a polling loop */
 	ret = poll_transfer(priv);
 
+	/*
+	 * Wait for current transmit operation to complete.
+	 * Otherwise if some data still exists in Tx FIFO it can be
+	 * silently flushed, i.e. dropped on disabling of the controller,
+	 * which happens when writing 0 to DW_SPI_SSIENR which happens
+	 * in the beginning of new transfer.
+	 */
+	start = get_timer(0);
+	while (!(dw_readl(priv, DW_SPI_SR) & SR_TF_EMPT) ||
+		(dw_readl(priv, DW_SPI_SR) & SR_BUSY)) {
+		if (get_timer(start) > RX_TIMEOUT) {
+			ret = -ETIMEDOUT;
+			break;
+		}
+	}
+
 	return ret;
 }
 
-- 
2.14.3

[U-Boot] [PATCH 1/5] DW SPI: fix tx data loss on FIFO flush

[Jagan Teki]

On Mon, Mar 5, 2018 at 6:47 PM, Eugeniy Paltsev
<Eugeniy.Paltsev@synopsys.com> wrote:
> In current implementation if some data still exists in Tx FIFO it
> can be silently flushed, i.e. dropped on disabling of the controller,
> which happens when writing 0 to DW_SPI_SSIENR (it happens in the
> beginning of new transfer)
>
> So add wait for current transmit operation to complete to be sure
> that current transmit operation is finished before new one.
>
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> ---
>  drivers/spi/designware_spi.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
> index c501aeea16..71d8839d55 100644
> --- a/drivers/spi/designware_spi.c
> +++ b/drivers/spi/designware_spi.c
> @@ -338,6 +338,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
>  {
>         struct udevice *bus = dev->parent;
>         struct dw_spi_priv *priv = dev_get_priv(bus);
> +       unsigned long start;
>         const u8 *tx = dout;
>         u8 *rx = din;
>         int ret = 0;
> @@ -394,6 +395,22 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
>         /* Start transfer in a polling loop */
>         ret = poll_transfer(priv);
>
> +       /*
> +        * Wait for current transmit operation to complete.
> +        * Otherwise if some data still exists in Tx FIFO it can be
> +        * silently flushed, i.e. dropped on disabling of the controller,
> +        * which happens when writing 0 to DW_SPI_SSIENR which happens
> +        * in the beginning of new transfer.
> +        */
> +       start = get_timer(0);
> +       while (!(dw_readl(priv, DW_SPI_SR) & SR_TF_EMPT) ||
> +               (dw_readl(priv, DW_SPI_SR) & SR_BUSY)) {
> +               if (get_timer(start) > RX_TIMEOUT) {
> +                       ret = -ETIMEDOUT;
> +                       break;
> +               }
> +       }

Use wait_for_bit

[U-Boot] [PATCH 2/5] DW SPI: fix transmit only mode

[Eugeniy Paltsev]

In current implementation we get -ETIMEDOUT error when we try to use
transmit only mode (SPI_TMOD_TO)
This happens because in transmit only mode input FIFO never gets any data
which breaks our logic in dw_reader(): we are waiting until RX data will be
ready in dw_reader, but this newer happens, so we return with error.

Fix that by using SPI_TMOD_TR instead of SPI_TMOD_TO which allows to use
RX FIFO.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 drivers/spi/designware_spi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index 71d8839d55..37ce16dc11 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -360,7 +360,11 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	else if (rx)
 		priv->tmode = SPI_TMOD_RO;
 	else
-		priv->tmode = SPI_TMOD_TO;
+		/*
+		 * In transmit only mode (SPI_TMOD_TO) input FIFO never gets
+		 * any data which breaks our logic in poll_transfer() above.
+		 */
+		priv->tmode = SPI_TMOD_TR;
 
 	cr0 &= ~SPI_TMOD_MASK;
 	cr0 |= (priv->tmode << SPI_TMOD_OFFSET);
-- 
2.14.3

[U-Boot] [PATCH 3/5] DW SPI: refactor poll_transfer functions

[Eugeniy Paltsev]

There is no sense in waiting for RX data in dw_reader function:
there is no chance that RX data will appear in RX FIFO if
RX FIFO is empty after previous TX write in dw_writer function.
So get rid of this waiting. After that we can get rid of dw_reader
return value and make it returning void. After that we can get rid
of dw_reader return value check in poll_transfer function.

With these changes we're getting closer to Linux DW SPI driver.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 drivers/spi/designware_spi.c | 26 ++++----------------------
 1 file changed, 4 insertions(+), 22 deletions(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index 37ce16dc11..630dc17f16 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -285,28 +285,16 @@ static void dw_writer(struct dw_spi_priv *priv)
 	}
 }
 
-static int dw_reader(struct dw_spi_priv *priv)
+static void dw_reader(struct dw_spi_priv *priv)
 {
-	unsigned start = get_timer(0);
-	u32 max;
+	u32 max = rx_max(priv);
 	u16 rxw;
 
-	/* Wait for rx data to be ready */
-	while (rx_max(priv) == 0) {
-		if (get_timer(start) > RX_TIMEOUT)
-			return -ETIMEDOUT;
-	}
-
-	max = rx_max(priv);
-
 	while (max--) {
 		rxw = dw_readw(priv, DW_SPI_DR);
 		debug("%s: rx=0x%02x\n", __func__, rxw);
 
-		/*
-		 * Care about rx only if the transfer's original "rx" is
-		 * not null
-		 */
+		/* Care about rx if the transfer's original "rx" is not null */
 		if (priv->rx_end - priv->len) {
 			if (priv->bits_per_word == 8)
 				*(u8 *)(priv->rx) = rxw;
@@ -315,19 +303,13 @@ static int dw_reader(struct dw_spi_priv *priv)
 		}
 		priv->rx += priv->bits_per_word >> 3;
 	}
-
-	return 0;
 }
 
 static int poll_transfer(struct dw_spi_priv *priv)
 {
-	int ret;
-
 	do {
 		dw_writer(priv);
-		ret = dw_reader(priv);
-		if (ret < 0)
-			return ret;
+		dw_reader(priv);
 	} while (priv->rx_end > priv->rx);
 
 	return 0;
-- 
2.14.3

[U-Boot] [PATCH 4/5] DW SPI: add option to use external gpio for chip select

[Eugeniy Paltsev]

DW SPI internal chip select management has limitation:
it hold CS line in active state only when the FIFO is not
empty. If the FIFO freed before we add new data the SPI transaction will
be broken.

So add option to use external gpio for chip select. Gpio can be added
via device tree using standard gpio bindings.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 drivers/spi/designware_spi.c | 51 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index 630dc17f16..099c9c4d03 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -10,6 +10,7 @@
  * SPDX-License-Identifier:	GPL-2.0
  */
 
+#include <asm-generic/gpio.h>
 #include <common.h>
 #include <clk.h>
 #include <dm.h>
@@ -97,6 +98,8 @@ struct dw_spi_priv {
 	struct clk clk;
 	unsigned long bus_clk_rate;
 
+	struct gpio_desc cs_gpio;	/* External chip-select gpio */
+
 	int bits_per_word;
 	u8 cs;			/* chip select pin */
 	u8 tmode;		/* TR/TO/RO/EEPROM */
@@ -130,6 +133,32 @@ static inline void dw_writew(struct dw_spi_priv *priv, u32 offset, u16 val)
 	__raw_writew(val, priv->regs + offset);
 }
 
+static int request_gpio_cs(struct udevice *bus)
+{
+#if defined(CONFIG_DM_GPIO) && !defined(CONFIG_SPL_BUILD)
+	struct dw_spi_priv *priv = dev_get_priv(bus);
+	int ret;
+
+	/* External chip select gpio line is optional */
+	ret = gpio_request_by_name(bus, "cs-gpio", 0, &priv->cs_gpio, 0);
+	if (ret == -ENOENT)
+		return 0;
+
+	if (ret < 0) {
+		printf("Error: %d: Can't get %s gpio!\n", ret, bus->name);
+		return ret;
+	}
+
+	if (dm_gpio_is_valid(&priv->cs_gpio)) {
+		dm_gpio_set_dir_flags(&priv->cs_gpio,
+				      GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
+	}
+
+	debug("%s: used external gpio for CS management\n", __func__);
+#endif
+	return 0;
+}
+
 static int dw_spi_ofdata_to_platdata(struct udevice *bus)
 {
 	struct dw_spi_platdata *plat = bus->platdata;
@@ -144,7 +173,7 @@ static int dw_spi_ofdata_to_platdata(struct udevice *bus)
 	debug("%s: regs=%p max-frequency=%d\n", __func__, plat->regs,
 	      plat->frequency);
 
-	return 0;
+	return request_gpio_cs(bus);
 }
 
 static inline void spi_enable_chip(struct dw_spi_priv *priv, int enable)
@@ -315,6 +344,18 @@ static int poll_transfer(struct dw_spi_priv *priv)
 	return 0;
 }
 
+static void external_cs_manage(struct udevice *dev, bool on)
+{
+#if defined(CONFIG_DM_GPIO) && !defined(CONFIG_SPL_BUILD)
+	struct dw_spi_priv *priv = dev_get_priv(dev->parent);
+
+	if (!dm_gpio_is_valid(&priv->cs_gpio))
+		return;
+
+	dm_gpio_set_value(&priv->cs_gpio, on ? 1 : 0);
+#endif
+}
+
 static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 		       const void *dout, void *din, unsigned long flags)
 {
@@ -333,6 +374,10 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 		return -1;
 	}
 
+	/* Start the transaction if necessary. */
+	if (flags & SPI_XFER_BEGIN)
+		external_cs_manage(dev, false);
+
 	cr0 = (priv->bits_per_word - 1) | (priv->type << SPI_FRF_OFFSET) |
 		(priv->mode << SPI_MODE_OFFSET) |
 		(priv->tmode << SPI_TMOD_OFFSET);
@@ -397,6 +442,10 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 		}
 	}
 
+	/* Stop the transaction if necessary */
+	if (flags & SPI_XFER_END)
+		external_cs_manage(dev, true);
+
 	return ret;
 }
 
-- 
2.14.3

[U-Boot] [PATCH 5/5] DW SPI: use 32 bit access instead of 16 and 32 bit mix

[Eugeniy Paltsev]

Current DW SPI driver uses 32 bit access for some registers and
16 bit access for others. So if DW SPI IP is connected via bus
which doesn't support 16 bit access we will get bus error.

Fix that by switching to 32 bit access only instead of 16 and 32 bit mix

Additional Documentation to Support this Change:
The DW_apb_ssi databook states:
"All registers in the DW_apb_ssi are addressed at 32-bit boundaries
to remain consistent with the AHB bus. Where the physical size of
any register is less than 32-bits wide, the upper unused bits of
the 32-bit boundary are reserved. Writing to these bits has no
effect; reading from these bits returns 0." [1]

[1] Section 6.1 of dw_apb_ssi.pdf (version 3.22a)

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 drivers/spi/designware_spi.c | 44 +++++++++++++++++---------------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index 099c9c4d03..24034c79b3 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -113,26 +113,16 @@ struct dw_spi_priv {
 	void *rx_end;
 };
 
-static inline u32 dw_readl(struct dw_spi_priv *priv, u32 offset)
+static inline u32 dw_read(struct dw_spi_priv *priv, u32 offset)
 {
 	return __raw_readl(priv->regs + offset);
 }
 
-static inline void dw_writel(struct dw_spi_priv *priv, u32 offset, u32 val)
+static inline void dw_write(struct dw_spi_priv *priv, u32 offset, u32 val)
 {
 	__raw_writel(val, priv->regs + offset);
 }
 
-static inline u16 dw_readw(struct dw_spi_priv *priv, u32 offset)
-{
-	return __raw_readw(priv->regs + offset);
-}
-
-static inline void dw_writew(struct dw_spi_priv *priv, u32 offset, u16 val)
-{
-	__raw_writew(val, priv->regs + offset);
-}
-
 static int request_gpio_cs(struct udevice *bus)
 {
 #if defined(CONFIG_DM_GPIO) && !defined(CONFIG_SPL_BUILD)
@@ -178,14 +168,14 @@ static int dw_spi_ofdata_to_platdata(struct udevice *bus)
 
 static inline void spi_enable_chip(struct dw_spi_priv *priv, int enable)
 {
-	dw_writel(priv, DW_SPI_SSIENR, (enable ? 1 : 0));
+	dw_write(priv, DW_SPI_SSIENR, (enable ? 1 : 0));
 }
 
 /* Restart the controller, disable all interrupts, clean rx fifo */
 static void spi_hw_init(struct dw_spi_priv *priv)
 {
 	spi_enable_chip(priv, 0);
-	dw_writel(priv, DW_SPI_IMR, 0xff);
+	dw_write(priv, DW_SPI_IMR, 0xff);
 	spi_enable_chip(priv, 1);
 
 	/*
@@ -196,13 +186,13 @@ static void spi_hw_init(struct dw_spi_priv *priv)
 		u32 fifo;
 
 		for (fifo = 1; fifo < 256; fifo++) {
-			dw_writew(priv, DW_SPI_TXFLTR, fifo);
-			if (fifo != dw_readw(priv, DW_SPI_TXFLTR))
+			dw_write(priv, DW_SPI_TXFLTR, fifo);
+			if (fifo != dw_read(priv, DW_SPI_TXFLTR))
 				break;
 		}
 
 		priv->fifo_len = (fifo == 1) ? 0 : fifo;
-		dw_writew(priv, DW_SPI_TXFLTR, 0);
+		dw_write(priv, DW_SPI_TXFLTR, 0);
 	}
 	debug("%s: fifo_len=%d\n", __func__, priv->fifo_len);
 }
@@ -271,7 +261,7 @@ static inline u32 tx_max(struct dw_spi_priv *priv)
 	u32 tx_left, tx_room, rxtx_gap;
 
 	tx_left = (priv->tx_end - priv->tx) / (priv->bits_per_word >> 3);
-	tx_room = priv->fifo_len - dw_readw(priv, DW_SPI_TXFLR);
+	tx_room = priv->fifo_len - dw_read(priv, DW_SPI_TXFLR);
 
 	/*
 	 * Another concern is about the tx/rx mismatch, we
@@ -292,7 +282,7 @@ static inline u32 rx_max(struct dw_spi_priv *priv)
 {
 	u32 rx_left = (priv->rx_end - priv->rx) / (priv->bits_per_word >> 3);
 
-	return min_t(u32, rx_left, dw_readw(priv, DW_SPI_RXFLR));
+	return min_t(u32, rx_left, dw_read(priv, DW_SPI_RXFLR));
 }
 
 static void dw_writer(struct dw_spi_priv *priv)
@@ -308,7 +298,7 @@ static void dw_writer(struct dw_spi_priv *priv)
 			else
 				txw = *(u16 *)(priv->tx);
 		}
-		dw_writew(priv, DW_SPI_DR, txw);
+		dw_write(priv, DW_SPI_DR, txw);
 		debug("%s: tx=0x%02x\n", __func__, txw);
 		priv->tx += priv->bits_per_word >> 3;
 	}
@@ -320,7 +310,7 @@ static void dw_reader(struct dw_spi_priv *priv)
 	u16 rxw;
 
 	while (max--) {
-		rxw = dw_readw(priv, DW_SPI_DR);
+		rxw = dw_read(priv, DW_SPI_DR);
 		debug("%s: rx=0x%02x\n", __func__, rxw);
 
 		/* Care about rx if the transfer's original "rx" is not null */
@@ -409,8 +399,8 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 
 	debug("%s: cr0=%08x\n", __func__, cr0);
 	/* Reprogram cr0 only if changed */
-	if (dw_readw(priv, DW_SPI_CTRL0) != cr0)
-		dw_writew(priv, DW_SPI_CTRL0, cr0);
+	if (dw_read(priv, DW_SPI_CTRL0) != cr0)
+		dw_write(priv, DW_SPI_CTRL0, cr0);
 
 	/*
 	 * Configure the desired SS (slave select 0...3) in the controller
@@ -418,7 +408,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	 * automatically. So no cs_activate() etc is needed in this driver.
 	 */
 	cs = spi_chip_select(dev);
-	dw_writel(priv, DW_SPI_SER, 1 << cs);
+	dw_write(priv, DW_SPI_SER, 1 << cs);
 
 	/* Enable controller after writing control registers */
 	spi_enable_chip(priv, 1);
@@ -434,8 +424,8 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	 * in the beginning of new transfer.
 	 */
 	start = get_timer(0);
-	while (!(dw_readl(priv, DW_SPI_SR) & SR_TF_EMPT) ||
-		(dw_readl(priv, DW_SPI_SR) & SR_BUSY)) {
+	while (!(dw_read(priv, DW_SPI_SR) & SR_TF_EMPT) ||
+		(dw_read(priv, DW_SPI_SR) & SR_BUSY)) {
 		if (get_timer(start) > RX_TIMEOUT) {
 			ret = -ETIMEDOUT;
 			break;
@@ -464,7 +454,7 @@ static int dw_spi_set_speed(struct udevice *bus, uint speed)
 	/* clk_div doesn't support odd number */
 	clk_div = priv->bus_clk_rate / speed;
 	clk_div = (clk_div + 1) & 0xfffe;
-	dw_writel(priv, DW_SPI_BAUDR, clk_div);
+	dw_write(priv, DW_SPI_BAUDR, clk_div);
 
 	/* Enable controller after writing control registers */
 	spi_enable_chip(priv, 1);
-- 
2.14.3

[U-Boot] [PATCH 0/5] DW SPI: fixes and improvements

[Jagan Teki]

On Mon, Mar 5, 2018 at 6:47 PM, Eugeniy Paltsev
<Eugeniy.Paltsev@synopsys.com> wrote:
> Various fixes and improvements of designware spi driver.
>
> Eugeniy Paltsev (5):
>   DW SPI: fix tx data loss on FIFO flush
>   DW SPI: fix transmit only mode
>   DW SPI: refactor poll_transfer functions
>   DW SPI: add option to use external gpio for chip select
>   DW SPI: use 32 bit access instead of 16 and 32 bit mix

Can someone test this series?

Reviewed-by: Jagan Teki <jagan@openedev.com>

[U-Boot] [PATCH 0/5] DW SPI: fixes and improvements

[Jagan Teki]

On Tue, Mar 13, 2018 at 8:44 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
> On Mon, Mar 5, 2018 at 6:47 PM, Eugeniy Paltsev
> <Eugeniy.Paltsev@synopsys.com> wrote:
>> Various fixes and improvements of designware spi driver.
>>
>> Eugeniy Paltsev (5):
>>   DW SPI: fix tx data loss on FIFO flush
>>   DW SPI: fix transmit only mode
>>   DW SPI: refactor poll_transfer functions
>>   DW SPI: add option to use external gpio for chip select
>>   DW SPI: use 32 bit access instead of 16 and 32 bit mix
>
> Can someone test this series?

Ping?

[U-Boot] [PATCH 0/5] DW SPI: fixes and improvements

[Alexey Brodkin]

Hi Jagan,

On Thu, 2018-03-22 at 00:04 +0530, Jagan Teki wrote:
> On Tue, Mar 13, 2018 at 8:44 PM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
> > On Mon, Mar 5, 2018 at 6:47 PM, Eugeniy Paltsev
> > <Eugeniy.Paltsev@synopsys.com> wrote:
> > > Various fixes and improvements of designware spi driver.
> > > 
> > > Eugeniy Paltsev (5):
> > >   DW SPI: fix tx data loss on FIFO flush
> > >   DW SPI: fix transmit only mode
> > >   DW SPI: refactor poll_transfer functions
> > >   DW SPI: add option to use external gpio for chip select
> > >   DW SPI: use 32 bit access instead of 16 and 32 bit mix
> > 
> > Can someone test this series?

We did test it on or platform (Synopsys HSDK) where it works just fine.

-Alexey