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

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

[Eugeniy Paltsev]

Various fixes and improvements of designware spi driver.

Changes v1->v2:
 * Use readl_poll_timeout macros instead of custom code.

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 v2 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>
---
Changes v1->v2:
 * Use readl_poll_timeout macros instead of custom code.

 drivers/spi/designware_spi.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index c501aeea16..5e196b21c9 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -18,6 +18,7 @@
 #include <spi.h>
 #include <fdtdec.h>
 #include <linux/compat.h>
+#include <linux/iopoll.h>
 #include <asm/io.h>
 
 DECLARE_GLOBAL_DATA_PTR;
@@ -342,6 +343,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	u8 *rx = din;
 	int ret = 0;
 	u32 cr0 = 0;
+	u32 val;
 	u32 cs;
 
 	/* spi core configured to do 8 bit transfers */
@@ -394,6 +396,19 @@ 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.
+	 */
+	if (readl_poll_timeout(priv->regs + DW_SPI_SR, val,
+			       !(val & SR_TF_EMPT) || (val & SR_BUSY),
+			       RX_TIMEOUT * 1000)) {
+		ret = -ETIMEDOUT;
+	}
+
 	return ret;
 }
 
-- 
2.14.3

[U-Boot] [PATCH v2 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>
---
Changes v1->v2:
 * None.

 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 5e196b21c9..3296441606 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -361,7 +361,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 v2 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>
---
Changes v1->v2:
 * None.

 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 3296441606..b51242c862 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -286,28 +286,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;
@@ -316,19 +304,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 v2 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>
---
Changes v1->v2:
 * None.

 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 b51242c862..06f777461e 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>
@@ -98,6 +99,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 */
@@ -131,6 +134,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;
@@ -145,7 +174,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)
@@ -316,6 +345,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)
 {
@@ -334,6 +375,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);
@@ -395,6 +440,10 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 		ret = -ETIMEDOUT;
 	}
 
+	/* Stop the transaction if necessary */
+	if (flags & SPI_XFER_END)
+		external_cs_manage(dev, true);
+
 	return ret;
 }
 
-- 
2.14.3

[U-Boot] [PATCH v2 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>
---
Changes v1->v2:
 * None.

 drivers/spi/designware_spi.c | 40 +++++++++++++++-------------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index 06f777461e..0e93b62eee 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -114,26 +114,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)
@@ -179,14 +169,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);
 
 	/*
@@ -197,13 +187,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);
 }
@@ -272,7 +262,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
@@ -293,7 +283,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)
@@ -309,7 +299,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;
 	}
@@ -321,7 +311,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 */
@@ -410,8 +400,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
@@ -419,7 +409,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);
@@ -462,7 +452,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 v2 0/5] DW SPI: fixes and improvements

[Jagan Teki]

On Thu, Mar 22, 2018 at 4:20 PM, Eugeniy Paltsev
<Eugeniy.Paltsev@synopsys.com> wrote:
> Various fixes and improvements of designware spi driver.
>
> Changes v1->v2:
>  * Use readl_poll_timeout macros instead of custom code.
>
> 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(-)

Applied to u-boot-spi/master

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

[Alexey Brodkin]

Hi Jagan,

On Thu, 2018-03-22 at 23:03 +0530, Jagan Teki wrote:
> On Thu, Mar 22, 2018 at 4:20 PM, Eugeniy Paltsev
> <Eugeniy.Paltsev@synopsys.com> wrote:
> > Various fixes and improvements of designware spi driver.
> > 
> > Changes v1->v2:
> >   * Use readl_poll_timeout macros instead of custom code.
> > 
> > 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(-)
> 
> Applied to u-boot-spi/master

I guess that will be merged in U-Boot master tree sometime soon so
that will be a part of the next u-boot release, right?

-Alexey

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

[Jagan Teki]

On Thu, Mar 22, 2018 at 11:05 PM, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:
> Hi Jagan,
>
> On Thu, 2018-03-22 at 23:03 +0530, Jagan Teki wrote:
>> On Thu, Mar 22, 2018 at 4:20 PM, Eugeniy Paltsev
>> <Eugeniy.Paltsev@synopsys.com> wrote:
>> > Various fixes and improvements of designware spi driver.
>> >
>> > Changes v1->v2:
>> >   * Use readl_poll_timeout macros instead of custom code.
>> >
>> > 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(-)
>>
>> Applied to u-boot-spi/master
>
> I guess that will be merged in U-Boot master tree sometime soon so
> that will be a part of the next u-boot release, right?

Yes, I will send PR soon.

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

[Alexey Brodkin]

Cool,thanks!

-Alexey

On Thu, 2018-03-22 at 23:42 +0530, Jagan Teki wrote:
> On Thu, Mar 22, 2018 at 11:05 PM, Alexey Brodkin
> <Alexey.Brodkin@synopsys.com> wrote:
> > Hi Jagan,
> > 
> > On Thu, 2018-03-22 at 23:03 +0530, Jagan Teki wrote:
> > > On Thu, Mar 22, 2018 at 4:20 PM, Eugeniy Paltsev
> > > <Eugeniy.Paltsev@synopsys.com> wrote:
> > > > Various fixes and improvements of designware spi driver.
> > > > 
> > > > Changes v1->v2:
> > > >   * Use readl_poll_timeout macros instead of custom code.
> > > > 
> > > > 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(-)
> > > 
> > > Applied to u-boot-spi/master
> > 
> > I guess that will be merged in U-Boot master tree sometime soon so
> > that will be a part of the next u-boot release, right?
> 
> Yes, I will send PR soon.