[U-Boot] [PATCH 0/5] DW SPI: fixes and improvements
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(-)
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; }
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
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);
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;
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; }
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);
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
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?
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
participants (3)
-
Alexey Brodkin -
Eugeniy Paltsev -
Jagan Teki