The following patchset provides corrections for PIO-mode
and support for DMA transfers in sun4i SPI driver.
Changes in v2:
1) Restored processing of 3/4 FIFO full interrupt.
2) Debug log enhancements.
Sergey Suloev (6):
spi: core: handle timeout error from transfer_one()
spi: sun4i: restrict transfer length in PIO-mode
spi: sun4i: coding style/readability improvements
spi: sun4i: use completion provided by SPI core driver
spi: sun4i: introduce register set/unset helpers
spi: sun4i: add DMA transfers support
drivers/spi/spi-sun4i.c | 442 +++++++++++++++++++++++++++++++++++++-----------
drivers/spi/spi.c | 5 +-
2 files changed, 347 insertions(+), 100 deletions(-)
--
2.16.2
DMA transfers are now available for sun4i-family SoCs.
The DMA mode is used automatically as soon as requested
transfer length is more than FIFO length.
Changes in v2:
1) Debug log enhancements.
Signed-off-by: Sergey Suloev <[email protected]>
---
drivers/spi/spi-sun4i.c | 299 ++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 277 insertions(+), 22 deletions(-)
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index d81d31c..dda7922 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -14,6 +14,8 @@
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/device.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/module.h>
@@ -39,6 +41,7 @@
#define SUN4I_CTL_CPHA BIT(2)
#define SUN4I_CTL_CPOL BIT(3)
#define SUN4I_CTL_CS_ACTIVE_LOW BIT(4)
+#define SUN4I_CTL_DMA_DEDICATED BIT(5)
#define SUN4I_CTL_LMTF BIT(6)
#define SUN4I_CTL_TF_RST BIT(8)
#define SUN4I_CTL_RF_RST BIT(9)
@@ -58,6 +61,8 @@
#define SUN4I_INT_STA_REG 0x10
#define SUN4I_DMA_CTL_REG 0x14
+#define SUN4I_CTL_DMA_RF_READY BIT(0)
+#define SUN4I_CTL_DMA_TF_NOT_FULL BIT(10)
#define SUN4I_WAIT_REG 0x18
@@ -169,6 +174,13 @@ static inline void sun4i_spi_fill_fifo(struct sun4i_spi *sspi, int len)
}
}
+static bool sun4i_spi_can_dma(struct spi_master *master,
+ struct spi_device *spi,
+ struct spi_transfer *tfr)
+{
+ return tfr->len > SUN4I_FIFO_DEPTH;
+}
+
static void sun4i_spi_set_cs(struct spi_device *spi, bool enable)
{
struct sun4i_spi *sspi = spi_master_get_devdata(spi->master);
@@ -208,6 +220,11 @@ static void sun4i_spi_set_cs(struct spi_device *spi, bool enable)
static size_t sun4i_spi_max_transfer_size(struct spi_device *spi)
{
+ struct spi_master *master = spi->master;
+
+ if (master->can_dma)
+ return SUN4I_MAX_XFER_SIZE;
+
return SUN4I_FIFO_DEPTH;
}
@@ -235,6 +252,164 @@ static int sun4i_spi_wait_for_transfer(struct spi_device *spi,
return 0;
}
+static void sun4i_spi_dma_callback(void *param)
+{
+ struct spi_master *master = param;
+
+ dev_dbg(&master->dev, "DMA transfer complete\n");
+ spi_finalize_current_transfer(master);
+}
+
+static int sun4i_spi_dmap_prep_tx(struct spi_master *master,
+ struct spi_transfer *tfr,
+ dma_cookie_t *cookie)
+{
+ struct dma_async_tx_descriptor *chan_desc = NULL;
+
+ chan_desc = dmaengine_prep_slave_sg(master->dma_tx,
+ tfr->tx_sg.sgl, tfr->tx_sg.nents,
+ DMA_TO_DEVICE,
+ DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+ if (!chan_desc) {
+ dev_err(&master->dev,
+ "Couldn't prepare TX DMA slave\n");
+ return -EIO;
+ }
+
+ chan_desc->callback = sun4i_spi_dma_callback;
+ chan_desc->callback_param = master;
+
+ *cookie = dmaengine_submit(chan_desc);
+ dma_async_issue_pending(master->dma_tx);
+
+ return 0;
+}
+
+static int sun4i_spi_dmap_prep_rx(struct spi_master *master,
+ struct spi_transfer *tfr,
+ dma_cookie_t *cookie)
+{
+ struct dma_async_tx_descriptor *chan_desc = NULL;
+
+ chan_desc = dmaengine_prep_slave_sg(master->dma_rx,
+ tfr->rx_sg.sgl, tfr->rx_sg.nents,
+ DMA_FROM_DEVICE,
+ DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+ if (!chan_desc) {
+ dev_err(&master->dev,
+ "Couldn't prepare RX DMA slave\n");
+ return -EIO;
+ }
+
+ chan_desc->callback = sun4i_spi_dma_callback;
+ chan_desc->callback_param = master;
+
+ *cookie = dmaengine_submit(chan_desc);
+ dma_async_issue_pending(master->dma_rx);
+
+ return 0;
+}
+
+static int sun4i_spi_transfer_one_dma(struct spi_device *spi,
+ struct spi_transfer *tfr)
+{
+ struct spi_master *master = spi->master;
+ struct sun4i_spi *sspi = spi_master_get_devdata(master);
+ dma_cookie_t tx_cookie = 0, rx_cookie = 0;
+ enum dma_status status;
+ int ret;
+ u32 reg = 0;
+
+ dev_dbg(&master->dev, "Using DMA mode for transfer\n");
+
+ /* Disable interrupts */
+ sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
+
+ if (sspi->tx_buf) {
+ ret = sun4i_spi_dmap_prep_tx(master, tfr, &tx_cookie);
+ if (ret)
+ goto out;
+
+ reg |= SUN4I_CTL_DMA_TF_NOT_FULL;
+ }
+
+ if (sspi->rx_buf) {
+ ret = sun4i_spi_dmap_prep_rx(master, tfr, &rx_cookie);
+ if (ret)
+ goto out;
+
+ reg |= SUN4I_CTL_DMA_RF_READY;
+ }
+
+ sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, reg);
+
+ /* Dedicated DMA requests */
+ sun4i_spi_set(sspi, SUN4I_CTL_REG, SUN4I_CTL_DMA_DEDICATED);
+
+ /* Start transfer */
+ sun4i_spi_set(sspi, SUN4I_CTL_REG, SUN4I_CTL_XCH);
+
+ /* Wait for completion */
+ ret = sun4i_spi_wait_for_transfer(spi, tfr);
+ if (ret)
+ goto out;
+
+ if (sspi->tx_buf && (status = dma_async_is_tx_complete(master->dma_tx,
+ tx_cookie, NULL, NULL))) {
+ dev_warn(&master->dev,
+ "DMA returned completion status of: %s\n",
+ status == DMA_ERROR ? "error" : "in progress");
+ }
+ if (sspi->rx_buf && (status = dma_async_is_tx_complete(master->dma_rx,
+ rx_cookie, NULL, NULL))) {
+ dev_warn(&master->dev,
+ "DMA returned completion status of: %s\n",
+ status == DMA_ERROR ? "error" : "in progress");
+ }
+
+out:
+ if (ret) {
+ dev_dbg(&master->dev, "DMA channel teardown\n");
+
+ if (sspi->tx_buf)
+ dmaengine_terminate_sync(master->dma_tx);
+ if (sspi->rx_buf)
+ dmaengine_terminate_sync(master->dma_rx);
+ }
+
+ sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+
+ return ret;
+}
+
+static int sun4i_spi_transfer_one_pio(struct spi_device *spi,
+ struct spi_transfer *tfr)
+{
+ struct spi_master *master = spi->master;
+ struct sun4i_spi *sspi = spi_master_get_devdata(master);
+ int ret;
+
+ /* Explicit disable DMA requests */
+ sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, 0);
+ sun4i_spi_unset(sspi, SUN4I_CTL_REG, SUN4I_CTL_DMA_DEDICATED);
+
+ /* Fill the TX FIFO */
+ sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
+
+ /* Enable the interrupts */
+ sun4i_spi_set(sspi, SUN4I_INT_CTL_REG, SUN4I_INT_CTL_TC |
+ SUN4I_INT_CTL_RF_F34);
+
+ /* Start transfer */
+ sun4i_spi_set(sspi, SUN4I_CTL_REG, SUN4I_CTL_XCH);
+
+ ret = sun4i_spi_wait_for_transfer(spi, tfr);
+
+ sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
+
+ return ret;
+}
+
static int sun4i_spi_transfer_one(struct spi_master *master,
struct spi_device *spi,
struct spi_transfer *tfr)
@@ -242,13 +417,22 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
struct sun4i_spi *sspi = spi_master_get_devdata(master);
unsigned int mclk_rate, div;
unsigned int tx_len = 0;
- int ret = 0;
u32 reg;
- /* We don't support transfers larger than FIFO depth */
- if (tfr->len > SUN4I_FIFO_DEPTH)
+ /* A zero length transfer never finishes if programmed
+ in the hardware */
+ if (!tfr->len)
+ return 0;
+
+ if (tfr->len > SUN4I_MAX_XFER_SIZE)
return -EMSGSIZE;
+ if (!master->can_dma) {
+ /* Don't support transfer larger than the FIFO */
+ if (tfr->len > SUN4I_FIFO_DEPTH)
+ return -EMSGSIZE;
+ }
+
sspi->tx_buf = tfr->tx_buf;
sspi->rx_buf = tfr->rx_buf;
sspi->len = tfr->len;
@@ -335,23 +519,10 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
sun4i_spi_write(sspi, SUN4I_BURST_CNT_REG, SUN4I_BURST_CNT(tfr->len));
sun4i_spi_write(sspi, SUN4I_XMIT_CNT_REG, SUN4I_XMIT_CNT(tx_len));
- /*
- * Fill the TX FIFO
- */
- sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
-
- /* Enable the interrupts */
- sun4i_spi_set(sspi, SUN4I_INT_CTL_REG, SUN4I_INT_CTL_TC |
- SUN4I_INT_CTL_RF_F34);
-
- /* Start the transfer */
- sun4i_spi_set(sspi, SUN4I_CTL_REG, SUN4I_CTL_XCH);
-
- ret = sun4i_spi_wait_for_transfer(spi, tfr);
-
- sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
+ if (sun4i_spi_can_dma(master, spi, tfr))
+ return sun4i_spi_transfer_one_dma(spi, tfr);
- return ret;
+ return sun4i_spi_transfer_one_pio(spi, tfr);
}
static irqreturn_t sun4i_spi_handler(int irq, void *dev_id)
@@ -364,8 +535,7 @@ static irqreturn_t sun4i_spi_handler(int irq, void *dev_id)
/* Transfer complete */
if (status & SUN4I_INT_CTL_TC) {
- sun4i_spi_write(sspi, SUN4I_INT_STA_REG,
- SUN4I_INT_CTL_TC);
+ sun4i_spi_write(sspi, SUN4I_INT_STA_REG, SUN4I_INT_CTL_TC);
sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
spi_finalize_current_transfer(master);
return IRQ_HANDLED;
@@ -422,6 +592,76 @@ static int sun4i_spi_runtime_suspend(struct device *dev)
return 0;
}
+static int sun4i_spi_dma_setup(struct device *dev,
+ struct resource *res)
+{
+ struct spi_master *master = dev_get_drvdata(dev);
+ struct dma_slave_config dma_sconf;
+ int ret;
+
+ master->dma_tx = dma_request_slave_channel_reason(dev, "tx");
+ if (IS_ERR(master->dma_tx)) {
+ dev_err(dev, "Unable to acquire DMA TX channel\n");
+ ret = PTR_ERR(master->dma_tx);
+ goto out;
+ }
+
+ dma_sconf.direction = DMA_MEM_TO_DEV;
+ dma_sconf.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconf.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconf.dst_addr = res->start + SUN4I_TXDATA_REG;
+ dma_sconf.dst_maxburst = 1;
+ dma_sconf.src_maxburst = 1;
+
+ ret = dmaengine_slave_config(master->dma_tx, &dma_sconf);
+ if (ret) {
+ dev_err(dev, "Unable to configure DMA TX slave\n");
+ goto err_rel_tx;
+ }
+
+ master->dma_rx = dma_request_slave_channel_reason(dev, "rx");
+ if (IS_ERR(master->dma_rx)) {
+ dev_err(dev, "Unable to acquire DMA RX channel\n");
+ ret = PTR_ERR(master->dma_rx);
+ goto err_rel_tx;
+ }
+
+ dma_sconf.direction = DMA_DEV_TO_MEM;
+ dma_sconf.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconf.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconf.src_addr = res->start + SUN4I_RXDATA_REG;
+ dma_sconf.src_maxburst = 1;
+ dma_sconf.dst_maxburst = 1;
+
+ ret = dmaengine_slave_config(master->dma_rx, &dma_sconf);
+ if (ret) {
+ dev_err(dev, "Unable to configure DMA RX slave\n");
+ goto err_rel_rx;
+ }
+
+ /* don't set can_dma unless both channels are valid*/
+ master->can_dma = sun4i_spi_can_dma;
+
+ return 0;
+
+err_rel_rx:
+ dma_release_channel(master->dma_rx);
+err_rel_tx:
+ dma_release_channel(master->dma_tx);
+out:
+ master->dma_tx = NULL;
+ master->dma_rx = NULL;
+ return ret;
+}
+
+static void sun4i_spi_dma_release(struct spi_master *master)
+{
+ if (master->can_dma) {
+ dma_release_channel(master->dma_rx);
+ dma_release_channel(master->dma_tx);
+ }
+}
+
static int sun4i_spi_probe(struct platform_device *pdev)
{
struct spi_master *master;
@@ -484,6 +724,16 @@ static int sun4i_spi_probe(struct platform_device *pdev)
goto err_free_master;
}
+ ret = sun4i_spi_dma_setup(&pdev->dev, res);
+ if (ret) {
+ if (ret == -EPROBE_DEFER) {
+ /* wait for the dma driver to load */
+ goto err_free_master;
+ }
+ dev_warn(&pdev->dev,
+ "Unable to setup DMA channels: DMA transfers disabled\n");
+ }
+
/*
* This wake-up/shutdown pattern is to be able to have the
* device woken up, even if runtime_pm is disabled
@@ -500,7 +750,7 @@ static int sun4i_spi_probe(struct platform_device *pdev)
ret = devm_spi_register_master(&pdev->dev, master);
if (ret) {
- dev_err(&pdev->dev, "cannot register SPI master\n");
+ dev_err(&pdev->dev, "Couldn't register SPI master\n");
goto err_pm_disable;
}
@@ -510,14 +760,19 @@ err_pm_disable:
pm_runtime_disable(&pdev->dev);
sun4i_spi_runtime_suspend(&pdev->dev);
err_free_master:
+ sun4i_spi_dma_release(master);
spi_master_put(master);
return ret;
}
static int sun4i_spi_remove(struct platform_device *pdev)
{
+ struct spi_master *master = platform_get_drvdata(pdev);
+
pm_runtime_force_suspend(&pdev->dev);
+ sun4i_spi_dma_release(master);
+
return 0;
}
--
2.16.2
There is no need to handle the 3/4 FIFO empty interrupt
as the maximum supported transfer length in PIO mode
is 64 bytes.
As long as a problem was reported previously with filling FIFO
on A10s we want to stick with 63 bytes depth.
Changes in v2:
1) Restored processing of 3/4 FIFO full interrupt.
Signed-off-by: Sergey Suloev <[email protected]>
---
drivers/spi/spi-sun4i.c | 37 ++++++++++---------------------------
1 file changed, 10 insertions(+), 27 deletions(-)
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index 4141003..08fd007 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -22,7 +22,12 @@
#include <linux/spi/spi.h>
-#define SUN4I_FIFO_DEPTH 64
+/*
+ * FIFO length is 64 bytes
+ * But filling the FIFO fully might cause a timeout
+ * on some devices, for example on spi2 on A10s
+ */
+#define SUN4I_FIFO_DEPTH 63
#define SUN4I_RXDATA_REG 0x00
@@ -202,7 +207,7 @@ static void sun4i_spi_set_cs(struct spi_device *spi, bool enable)
static size_t sun4i_spi_max_transfer_size(struct spi_device *spi)
{
- return SUN4I_FIFO_DEPTH - 1;
+ return SUN4I_FIFO_DEPTH;
}
static int sun4i_spi_transfer_one(struct spi_master *master,
@@ -216,11 +221,8 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
int ret = 0;
u32 reg;
- /* We don't support transfer larger than the FIFO */
- if (tfr->len > SUN4I_MAX_XFER_SIZE)
- return -EMSGSIZE;
-
- if (tfr->tx_buf && tfr->len >= SUN4I_MAX_XFER_SIZE)
+ /* We don't support transfers larger than FIFO depth */
+ if (tfr->len > SUN4I_FIFO_DEPTH)
return -EMSGSIZE;
reinit_completion(&sspi->done);
@@ -313,17 +315,12 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
/*
* Fill the TX FIFO
- * Filling the FIFO fully causes timeout for some reason
- * at least on spi2 on A10s
*/
- sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
+ sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
/* Enable the interrupts */
sun4i_spi_enable_interrupt(sspi, SUN4I_INT_CTL_TC |
SUN4I_INT_CTL_RF_F34);
- /* Only enable Tx FIFO interrupt if we really need it */
- if (tx_len > SUN4I_FIFO_DEPTH)
- sun4i_spi_enable_interrupt(sspi, SUN4I_INT_CTL_TF_E34);
/* Start the transfer */
reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
@@ -371,20 +368,6 @@ static irqreturn_t sun4i_spi_handler(int irq, void *dev_id)
return IRQ_HANDLED;
}
- /* Transmit FIFO 3/4 empty */
- if (status & SUN4I_INT_CTL_TF_E34) {
- sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
-
- if (!sspi->len)
- /* nothing left to transmit */
- sun4i_spi_disable_interrupt(sspi, SUN4I_INT_CTL_TF_E34);
-
- /* Only clear the interrupt _after_ re-seeding the FIFO */
- sun4i_spi_write(sspi, SUN4I_INT_STA_REG, SUN4I_INT_CTL_TF_E34);
-
- return IRQ_HANDLED;
- }
-
return IRQ_NONE;
}
--
2.16.2
Two helper functions were added in order to set/unset
specified flags in registers.
Signed-off-by: Sergey Suloev <[email protected]>
---
drivers/spi/spi-sun4i.c | 40 +++++++++++++++++++---------------------
1 file changed, 19 insertions(+), 21 deletions(-)
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index 9d1bc20..d81d31c 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -107,29 +107,29 @@ static inline void sun4i_spi_write(struct sun4i_spi *sspi, u32 reg, u32 value)
writel(value, sspi->base_addr + reg);
}
-static inline u32 sun4i_spi_get_tx_fifo_count(struct sun4i_spi *sspi)
+static inline void sun4i_spi_set(struct sun4i_spi *sspi, u32 addr, u32 val)
{
- u32 reg = sun4i_spi_read(sspi, SUN4I_FIFO_STA_REG);
-
- reg >>= SUN4I_FIFO_STA_TF_CNT_BITS;
+ u32 reg = sun4i_spi_read(sspi, addr);
- return reg & SUN4I_FIFO_STA_TF_CNT_MASK;
+ reg |= val;
+ sun4i_spi_write(sspi, addr, reg);
}
-static inline void sun4i_spi_enable_interrupt(struct sun4i_spi *sspi, u32 mask)
+static inline void sun4i_spi_unset(struct sun4i_spi *sspi, u32 addr, u32 val)
{
- u32 reg = sun4i_spi_read(sspi, SUN4I_INT_CTL_REG);
+ u32 reg = sun4i_spi_read(sspi, addr);
- reg |= mask;
- sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, reg);
+ reg &= ~val;
+ sun4i_spi_write(sspi, addr, reg);
}
-static inline void sun4i_spi_disable_interrupt(struct sun4i_spi *sspi, u32 mask)
+static inline u32 sun4i_spi_get_tx_fifo_count(struct sun4i_spi *sspi)
{
- u32 reg = sun4i_spi_read(sspi, SUN4I_INT_CTL_REG);
+ u32 reg = sun4i_spi_read(sspi, SUN4I_FIFO_STA_REG);
- reg &= ~mask;
- sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, reg);
+ reg >>= SUN4I_FIFO_STA_TF_CNT_BITS;
+
+ return reg & SUN4I_FIFO_STA_TF_CNT_MASK;
}
static inline void sun4i_spi_drain_fifo(struct sun4i_spi *sspi, int len)
@@ -256,13 +256,12 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
/* Clear pending interrupts */
sun4i_spi_write(sspi, SUN4I_INT_STA_REG, ~0);
+ /* Reset FIFOs */
+ sun4i_spi_set(sspi, SUN4I_CTL_REG,
+ SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST);
reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
- /* Reset FIFOs */
- sun4i_spi_write(sspi, SUN4I_CTL_REG,
- reg | SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST);
-
/*
* Setup the transfer control register: Chip Select,
* polarities, etc.
@@ -342,12 +341,11 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
/* Enable the interrupts */
- sun4i_spi_enable_interrupt(sspi, SUN4I_INT_CTL_TC |
- SUN4I_INT_CTL_RF_F34);
+ sun4i_spi_set(sspi, SUN4I_INT_CTL_REG, SUN4I_INT_CTL_TC |
+ SUN4I_INT_CTL_RF_F34);
/* Start the transfer */
- reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
- sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
+ sun4i_spi_set(sspi, SUN4I_CTL_REG, SUN4I_CTL_XCH);
ret = sun4i_spi_wait_for_transfer(spi, tfr);
--
2.16.2
As long as the completion already provided by the SPI core
then there is no need to waste extra-memory on this.
Also a waiting function was added to avoid code duplication.
Signed-off-by: Sergey Suloev <[email protected]>
---
drivers/spi/spi-sun4i.c | 62 ++++++++++++++++++++++++++++---------------------
1 file changed, 35 insertions(+), 27 deletions(-)
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index 899e956..9d1bc20 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -92,8 +92,6 @@ struct sun4i_spi {
struct clk *hclk;
struct clk *mclk;
- struct completion done;
-
const u8 *tx_buf;
u8 *rx_buf;
int len;
@@ -213,13 +211,36 @@ static size_t sun4i_spi_max_transfer_size(struct spi_device *spi)
return SUN4I_FIFO_DEPTH;
}
+static int sun4i_spi_wait_for_transfer(struct spi_device *spi,
+ struct spi_transfer *tfr)
+{
+ struct spi_master *master = spi->master;
+ unsigned int start, end, tx_time;
+ unsigned int timeout;
+
+ /* calc required timeout from given speed & len values */
+ tx_time = max(tfr->len * 8 * 2 / (tfr->speed_hz / 1000), 100U);
+ start = jiffies;
+ timeout = wait_for_completion_timeout(&master->xfer_completion,
+ msecs_to_jiffies(tx_time));
+ end = jiffies;
+ if (!timeout) {
+ dev_warn(&master->dev,
+ "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
+ dev_name(&spi->dev), tfr->len, tfr->speed_hz,
+ jiffies_to_msecs(end - start), tx_time);
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
static int sun4i_spi_transfer_one(struct spi_master *master,
struct spi_device *spi,
struct spi_transfer *tfr)
{
struct sun4i_spi *sspi = spi_master_get_devdata(master);
- unsigned int mclk_rate, div, timeout;
- unsigned int start, end, tx_time;
+ unsigned int mclk_rate, div;
unsigned int tx_len = 0;
int ret = 0;
u32 reg;
@@ -228,7 +249,6 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
if (tfr->len > SUN4I_FIFO_DEPTH)
return -EMSGSIZE;
- reinit_completion(&sspi->done);
sspi->tx_buf = tfr->tx_buf;
sspi->rx_buf = tfr->rx_buf;
sspi->len = tfr->len;
@@ -329,22 +349,8 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
- tx_time = max(tfr->len * 8 * 2 / (tfr->speed_hz / 1000), 100U);
- start = jiffies;
- timeout = wait_for_completion_timeout(&sspi->done,
- msecs_to_jiffies(tx_time));
- end = jiffies;
- if (!timeout) {
- dev_warn(&master->dev,
- "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
- dev_name(&spi->dev), tfr->len, tfr->speed_hz,
- jiffies_to_msecs(end - start), tx_time);
- ret = -ETIMEDOUT;
- goto out;
- }
-
+ ret = sun4i_spi_wait_for_transfer(spi, tfr);
-out:
sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
return ret;
@@ -352,14 +358,18 @@ out:
static irqreturn_t sun4i_spi_handler(int irq, void *dev_id)
{
- struct sun4i_spi *sspi = dev_id;
- u32 status = sun4i_spi_read(sspi, SUN4I_INT_STA_REG);
+ struct spi_master *master = dev_id;
+ struct sun4i_spi *sspi = spi_master_get_devdata(master);
+ u32 status;
+
+ status = sun4i_spi_read(sspi, SUN4I_INT_STA_REG);
/* Transfer complete */
if (status & SUN4I_INT_CTL_TC) {
- sun4i_spi_write(sspi, SUN4I_INT_STA_REG, SUN4I_INT_CTL_TC);
+ sun4i_spi_write(sspi, SUN4I_INT_STA_REG,
+ SUN4I_INT_CTL_TC);
sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
- complete(&sspi->done);
+ spi_finalize_current_transfer(master);
return IRQ_HANDLED;
}
@@ -456,7 +466,7 @@ static int sun4i_spi_probe(struct platform_device *pdev)
}
ret = devm_request_irq(&pdev->dev, irq, sun4i_spi_handler,
- 0, dev_name(&pdev->dev), sspi);
+ 0, dev_name(&pdev->dev), master);
if (ret) {
dev_err(&pdev->dev, "Cannot request IRQ\n");
goto err_free_master;
@@ -476,8 +486,6 @@ static int sun4i_spi_probe(struct platform_device *pdev)
goto err_free_master;
}
- init_completion(&sspi->done);
-
/*
* This wake-up/shutdown pattern is to be able to have the
* device woken up, even if runtime_pm is disabled
--
2.16.2
Minor changes to fulfill the coding style and
improve the readability.
Signed-off-by: Sergey Suloev <[email protected]>
---
drivers/spi/spi-sun4i.c | 32 +++++++++++++++++---------------
1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index 08fd007..899e956 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -83,8 +83,11 @@
#define SUN4I_FIFO_STA_TF_CNT_MASK 0x7f
#define SUN4I_FIFO_STA_TF_CNT_BITS 16
+#define SUN4I_SPI_MAX_SPEED_HZ 100 * 1000 * 1000
+#define SUN4I_SPI_MIN_SPEED_HZ 3 * 1000
+#define SUN4I_SPI_MODE_BITS (SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST)
+
struct sun4i_spi {
- struct spi_master *master;
void __iomem *base_addr;
struct clk *hclk;
struct clk *mclk;
@@ -418,12 +421,23 @@ static int sun4i_spi_probe(struct platform_device *pdev)
struct resource *res;
int ret = 0, irq;
- master = spi_alloc_master(&pdev->dev, sizeof(struct sun4i_spi));
+ master = spi_alloc_master(&pdev->dev, sizeof(*sspi));
if (!master) {
dev_err(&pdev->dev, "Unable to allocate SPI Master\n");
return -ENOMEM;
}
+ master->max_speed_hz = SUN4I_SPI_MAX_SPEED_HZ;
+ master->min_speed_hz = SUN4I_SPI_MIN_SPEED_HZ;
+ master->num_chipselect = 4;
+ master->mode_bits = SUN4I_SPI_MODE_BITS;
+ master->bits_per_word_mask = SPI_BPW_MASK(8);
+ master->set_cs = sun4i_spi_set_cs;
+ master->transfer_one = sun4i_spi_transfer_one;
+ master->max_transfer_size = sun4i_spi_max_transfer_size;
+ master->dev.of_node = pdev->dev.of_node;
+ master->auto_runtime_pm = true;
+
platform_set_drvdata(pdev, master);
sspi = spi_master_get_devdata(master);
@@ -442,24 +456,12 @@ static int sun4i_spi_probe(struct platform_device *pdev)
}
ret = devm_request_irq(&pdev->dev, irq, sun4i_spi_handler,
- 0, "sun4i-spi", sspi);
+ 0, dev_name(&pdev->dev), sspi);
if (ret) {
dev_err(&pdev->dev, "Cannot request IRQ\n");
goto err_free_master;
}
- sspi->master = master;
- master->max_speed_hz = 100 * 1000 * 1000;
- master->min_speed_hz = 3 * 1000;
- master->set_cs = sun4i_spi_set_cs;
- master->transfer_one = sun4i_spi_transfer_one;
- master->num_chipselect = 4;
- master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
- master->bits_per_word_mask = SPI_BPW_MASK(8);
- master->dev.of_node = pdev->dev.of_node;
- master->auto_runtime_pm = true;
- master->max_transfer_size = sun4i_spi_max_transfer_size;
-
sspi->hclk = devm_clk_get(&pdev->dev, "ahb");
if (IS_ERR(sspi->hclk)) {
dev_err(&pdev->dev, "Unable to acquire AHB clock\n");
--
2.16.2
As long as sun4i/sun6i SPI drivers have overriden the default
"wait for completion" procedure then we need to properly
handle -ETIMEDOUT error from transfer_one().
Signed-off-by: Sergey Suloev <[email protected]>
---
drivers/spi/spi.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index b33a727..2dcd4f6 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1028,7 +1028,7 @@ static int spi_transfer_one_message(struct spi_controller *ctlr,
reinit_completion(&ctlr->xfer_completion);
ret = ctlr->transfer_one(ctlr, msg->spi, xfer);
- if (ret < 0) {
+ if (ret < 0 && ret != -ETIMEDOUT) {
SPI_STATISTICS_INCREMENT_FIELD(statm,
errors);
SPI_STATISTICS_INCREMENT_FIELD(stats,
@@ -1051,7 +1051,7 @@ static int spi_transfer_one_message(struct spi_controller *ctlr,
msecs_to_jiffies(ms));
}
- if (ms == 0) {
+ if (ms == 0 || ret == -ETIMEDOUT) {
SPI_STATISTICS_INCREMENT_FIELD(statm,
timedout);
SPI_STATISTICS_INCREMENT_FIELD(stats,
@@ -1059,6 +1059,7 @@ static int spi_transfer_one_message(struct spi_controller *ctlr,
dev_err(&msg->spi->dev,
"SPI transfer timed out\n");
msg->status = -ETIMEDOUT;
+ ret = 0;
}
} else {
if (xfer->len)
--
2.16.2
On Tue, Apr 03, 2018 at 06:29:00PM +0300, Sergey Suloev wrote:
> As long as sun4i/sun6i SPI drivers have overriden the default
> "wait for completion" procedure then we need to properly
> handle -ETIMEDOUT error from transfer_one().
Why is this connected to those drivers specifically?
On 04/03/2018 06:52 PM, Mark Brown wrote:
> On Tue, Apr 03, 2018 at 06:29:00PM +0300, Sergey Suloev wrote:
>> As long as sun4i/sun6i SPI drivers have overriden the default
>> "wait for completion" procedure then we need to properly
>> handle -ETIMEDOUT error from transfer_one().
> Why is this connected to those drivers specifically?
These 2 drivers have their own "waiting" code and not using the code
from SPI core.
On Tue, Apr 03, 2018 at 07:00:55PM +0300, Sergey Suloev wrote:
> On 04/03/2018 06:52 PM, Mark Brown wrote:
> > On Tue, Apr 03, 2018 at 06:29:00PM +0300, Sergey Suloev wrote:
> > > As long as sun4i/sun6i SPI drivers have overriden the default
> > > "wait for completion" procedure then we need to properly
> > > handle -ETIMEDOUT error from transfer_one().
> > Why is this connected to those drivers specifically?
> These 2 drivers have their own "waiting" code and not using the code from
> SPI core.
Does this not apply to any other driver - why is this something we only
have to do when these drivers do it? That's what's setting off alarm
bells.
On 04/03/2018 07:18 PM, Mark Brown wrote:
> On Tue, Apr 03, 2018 at 07:00:55PM +0300, Sergey Suloev wrote:
>> On 04/03/2018 06:52 PM, Mark Brown wrote:
>>> On Tue, Apr 03, 2018 at 06:29:00PM +0300, Sergey Suloev wrote:
>>>> As long as sun4i/sun6i SPI drivers have overriden the default
>>>> "wait for completion" procedure then we need to properly
>>>> handle -ETIMEDOUT error from transfer_one().
>>> Why is this connected to those drivers specifically?
>> These 2 drivers have their own "waiting" code and not using the code from
>> SPI core.
> Does this not apply to any other driver - why is this something we only
> have to do when these drivers do it? That's what's setting off alarm
> bells.
sun4i/sun6i drivers have let's say "smart" waiting while SPI core uses a
fixed interval to wait.
I can't say for every SPI driver in kernel, that's outside of my area of
expertise.
On Tue, Apr 03, 2018 at 07:24:11PM +0300, Sergey Suloev wrote:
> On 04/03/2018 07:18 PM, Mark Brown wrote:
> > On Tue, Apr 03, 2018 at 07:00:55PM +0300, Sergey Suloev wrote:
> > > On 04/03/2018 06:52 PM, Mark Brown wrote:
> > > > On Tue, Apr 03, 2018 at 06:29:00PM +0300, Sergey Suloev wrote:
> > > > > As long as sun4i/sun6i SPI drivers have overriden the default
> > > > > "wait for completion" procedure then we need to properly
> > > > > handle -ETIMEDOUT error from transfer_one().
> > > > Why is this connected to those drivers specifically?
> > > These 2 drivers have their own "waiting" code and not using the code from
> > > SPI core.
> > Does this not apply to any other driver - why is this something we only
> > have to do when these drivers do it? That's what's setting off alarm
> > bells.
>
> sun4i/sun6i drivers have let's say "smart" waiting while SPI core uses a
> fixed interval to wait.
>
> I can't say for every SPI driver in kernel, that's outside of my area of
> expertise.
I'm not sure what's specific about the sun4i / sun6i case here. Your
patch doesn't have anything to do with the delay before the timeout,
but the fact that we return -ETIMEDOUT in the first place.
And I'm pretty sure that papering over an error returned by a driver
is not the right thing to do.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Apr 03, 2018 at 06:29:01PM +0300, Sergey Suloev wrote:
> There is no need to handle the 3/4 FIFO empty interrupt
> as the maximum supported transfer length in PIO mode
> is 64 bytes.
> As long as a problem was reported previously with filling FIFO
> on A10s we want to stick with 63 bytes depth.
>
> Changes in v2:
> 1) Restored processing of 3/4 FIFO full interrupt.
>
> Signed-off-by: Sergey Suloev <[email protected]>
> ---
> drivers/spi/spi-sun4i.c | 37 ++++++++++---------------------------
> 1 file changed, 10 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index 4141003..08fd007 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -22,7 +22,12 @@
>
> #include <linux/spi/spi.h>
>
> -#define SUN4I_FIFO_DEPTH 64
> +/*
> + * FIFO length is 64 bytes
> + * But filling the FIFO fully might cause a timeout
> + * on some devices, for example on spi2 on A10s
> + */
> +#define SUN4I_FIFO_DEPTH 63
The FIFO depth is 64 bytes, so the code should remain the same at
least from that regard.
> #define SUN4I_RXDATA_REG 0x00
>
> @@ -202,7 +207,7 @@ static void sun4i_spi_set_cs(struct spi_device *spi, bool enable)
>
> static size_t sun4i_spi_max_transfer_size(struct spi_device *spi)
> {
> - return SUN4I_FIFO_DEPTH - 1;
> + return SUN4I_FIFO_DEPTH;
> }
>
> static int sun4i_spi_transfer_one(struct spi_master *master,
> @@ -216,11 +221,8 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
> int ret = 0;
> u32 reg;
>
> - /* We don't support transfer larger than the FIFO */
> - if (tfr->len > SUN4I_MAX_XFER_SIZE)
> - return -EMSGSIZE;
> -
> - if (tfr->tx_buf && tfr->len >= SUN4I_MAX_XFER_SIZE)
> + /* We don't support transfers larger than FIFO depth */
> + if (tfr->len > SUN4I_FIFO_DEPTH)
> return -EMSGSIZE;
This essentially reverts 196737912da5, why?
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Apr 03, 2018 at 06:29:02PM +0300, Sergey Suloev wrote:
> Minor changes to fulfill the coding style and
> improve the readability.
Again, this is a very subjective statement. Overall, I'm not convinced
by the idea of moving code around and shuffling variables assignments
just for the sake of it.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Apr 03, 2018 at 06:29:03PM +0300, Sergey Suloev wrote:
> As long as the completion already provided by the SPI core
> then there is no need to waste extra-memory on this.
> Also a waiting function was added to avoid code duplication.
This should also be split in two parts.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Apr 03, 2018 at 06:29:04PM +0300, Sergey Suloev wrote:
> Two helper functions were added in order to set/unset
> specified flags in registers.
Again, the diffstat is pretty neutral, so what is the benefit?
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Wed, Apr 04, 2018 at 09:08:18AM +0200, Maxime Ripard wrote:
> And I'm pretty sure that papering over an error returned by a driver
> is not the right thing to do.
We've got specific error handling for timeouts - they get accounted for
separately in the stats. It *shouldn't* affect actual operation and
AFAICT it doesn't. I think the main problem here is that the commit
message is very unclear.
On 04/04/2018 10:08 AM, Maxime Ripard wrote:
> On Tue, Apr 03, 2018 at 07:24:11PM +0300, Sergey Suloev wrote:
>> On 04/03/2018 07:18 PM, Mark Brown wrote:
>>> On Tue, Apr 03, 2018 at 07:00:55PM +0300, Sergey Suloev wrote:
>>>> On 04/03/2018 06:52 PM, Mark Brown wrote:
>>>>> On Tue, Apr 03, 2018 at 06:29:00PM +0300, Sergey Suloev wrote:
>>>>>> As long as sun4i/sun6i SPI drivers have overriden the default
>>>>>> "wait for completion" procedure then we need to properly
>>>>>> handle -ETIMEDOUT error from transfer_one().
>>>>> Why is this connected to those drivers specifically?
>>>> These 2 drivers have their own "waiting" code and not using the code from
>>>> SPI core.
>>> Does this not apply to any other driver - why is this something we only
>>> have to do when these drivers do it? That's what's setting off alarm
>>> bells.
>> sun4i/sun6i drivers have let's say "smart" waiting while SPI core uses a
>> fixed interval to wait.
>>
>> I can't say for every SPI driver in kernel, that's outside of my area of
>> expertise.
> I'm not sure what's specific about the sun4i / sun6i case here. Your
> patch doesn't have anything to do with the delay before the timeout,
> but the fact that we return -ETIMEDOUT in the first place.
>
> And I'm pretty sure that papering over an error returned by a driver
> is not the right thing to do.
>
> Maxime
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
do you mean the changes in spi.c are not required at all ?
My point was to eat ETIMEDOUT error from transfer_one() as it is just a
mark and
shouldn't be handled as a normal error.