The following patchset provides corrections for PIO-mode
and support for DMA transfers in sun6i SPI driver.
Changes in v2:
1) Fixed issue with misplacing a piece of code that requires access
to the transfer structure into sun6i_spi_prepare_message() function
where the transfer structure is not available.
2) Fixed issue with passing an invalid argument into devm_request_irq()
function.
Changes in v3:
1) Restored processing of 3/4 FIFO full interrupt.
2) Debug log enhancements.
Sergey Suloev (6):
spi: sun6i: coding style/readability improvements
spi: sun6i: handle chip select polarity flag
spi: sun6i: restrict transfer length in PIO-mode
spi: sun6i: use completion provided by SPI core
spi: sun6i: introduce register set/unset helpers
spi: sun6i: add DMA transfers support
drivers/spi/spi-sun6i.c | 526 ++++++++++++++++++++++++++++++++++++------------
1 file changed, 402 insertions(+), 124 deletions(-)
--
2.16.2
DMA transfers are now available for sun6i and sun8i SoCs.
The DMA mode is used automatically as soon as requested
transfer length is more than FIFO length.
Changes in v3:
1) Debug log enhancements.
Signed-off-by: Sergey Suloev <[email protected]>
---
drivers/spi/spi-sun6i.c | 331 ++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 294 insertions(+), 37 deletions(-)
diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 2fa9d6e..7f41871 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.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>
@@ -55,17 +57,20 @@
#define SUN6I_FIFO_CTL_REG 0x18
#define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_MASK 0xff
-#define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS 0
+#define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_POS 0
+#define SUN6I_FIFO_CTL_RF_DRQ_EN BIT(8)
#define SUN6I_FIFO_CTL_RF_RST BIT(15)
#define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_MASK 0xff
-#define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS 16
+#define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_POS 16
+#define SUN6I_FIFO_CTL_TF_DRQ_EN BIT(24)
#define SUN6I_FIFO_CTL_TF_RST BIT(31)
+#define SUN6I_FIFO_CTL_DMA_DEDICATE BIT(9)|BIT(25)
#define SUN6I_FIFO_STA_REG 0x1c
#define SUN6I_FIFO_STA_RF_CNT_MASK 0x7f
-#define SUN6I_FIFO_STA_RF_CNT_BITS 0
+#define SUN6I_FIFO_STA_RF_CNT_POS 0
#define SUN6I_FIFO_STA_TF_CNT_MASK 0x7f
-#define SUN6I_FIFO_STA_TF_CNT_BITS 16
+#define SUN6I_FIFO_STA_TF_CNT_POS 16
#define SUN6I_CLK_CTL_REG 0x24
#define SUN6I_CLK_CTL_CDR2_MASK 0xff
@@ -135,7 +140,7 @@ static inline u32 sun6i_spi_get_tx_fifo_count(struct sun6i_spi *sspi)
{
u32 reg = sun6i_spi_read(sspi, SUN6I_FIFO_STA_REG);
- reg >>= SUN6I_FIFO_STA_TF_CNT_BITS;
+ reg >>= SUN6I_FIFO_STA_TF_CNT_POS;
return reg & SUN6I_FIFO_STA_TF_CNT_MASK;
}
@@ -148,7 +153,7 @@ static inline void sun6i_spi_drain_fifo(struct sun6i_spi *sspi, int len)
/* See how much data is available */
reg = sun6i_spi_read(sspi, SUN6I_FIFO_STA_REG);
reg &= SUN6I_FIFO_STA_RF_CNT_MASK;
- cnt = reg >> SUN6I_FIFO_STA_RF_CNT_BITS;
+ cnt = reg >> SUN6I_FIFO_STA_RF_CNT_POS;
if (len > cnt)
len = cnt;
@@ -177,6 +182,15 @@ static inline void sun6i_spi_fill_fifo(struct sun6i_spi *sspi, int len)
}
}
+static bool sun6i_spi_can_dma(struct spi_master *master,
+ struct spi_device *spi,
+ struct spi_transfer *tfr)
+{
+ struct sun6i_spi *sspi = spi_master_get_devdata(master);
+
+ return tfr->len > sspi->fifo_depth;
+}
+
static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
{
struct sun6i_spi *sspi = spi_master_get_devdata(spi->master);
@@ -208,6 +222,9 @@ static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
struct spi_master *master = spi->master;
struct sun6i_spi *sspi = spi_master_get_devdata(master);
+ if (master->can_dma)
+ return SUN6I_MAX_XFER_SIZE;
+
return sspi->fifo_depth;
}
@@ -268,16 +285,187 @@ static int sun6i_spi_wait_for_transfer(struct spi_device *spi,
return 0;
}
+static void sun6i_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 sun6i_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 = sun6i_spi_dma_callback;
+ chan_desc->callback_param = master;
+
+ *cookie = dmaengine_submit(chan_desc);
+ dma_async_issue_pending(master->dma_tx);
+
+ return 0;
+}
+
+static int sun6i_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 = sun6i_spi_dma_callback;
+ chan_desc->callback_param = master;
+
+ *cookie = dmaengine_submit(chan_desc);
+ dma_async_issue_pending(master->dma_rx);
+
+ return 0;
+}
+
+static int sun6i_spi_transfer_one_dma(struct spi_device *spi,
+ struct spi_transfer *tfr)
+{
+ struct spi_master *master = spi->master;
+ struct sun6i_spi *sspi = spi_master_get_devdata(master);
+ dma_cookie_t tx_cookie = 0,rx_cookie = 0;
+ enum dma_status status;
+ int ret;
+ u32 reg, trig_level = 0;
+
+ dev_dbg(&master->dev, "Using DMA mode for transfer\n");
+
+ /* Disable interrupts */
+ sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0);
+
+ reg = sun6i_spi_read(sspi, SUN6I_FIFO_CTL_REG);
+
+ if (sspi->tx_buf) {
+ ret = sun6i_spi_dmap_prep_tx(master, tfr, &tx_cookie);
+ if (ret)
+ goto out;
+
+ reg |= SUN6I_FIFO_CTL_TF_DRQ_EN;
+
+ trig_level = sspi->fifo_depth;
+ reg &= ~SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_MASK;
+ reg |= (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_POS);
+ }
+
+ if (sspi->rx_buf) {
+ ret = sun6i_spi_dmap_prep_rx(master, tfr, &rx_cookie);
+ if (ret)
+ goto out;
+
+ reg |= SUN6I_FIFO_CTL_RF_DRQ_EN;
+
+ trig_level = 1;
+ reg &= ~SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_MASK;
+ reg |= (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_POS);
+ }
+
+ /* Enable Dedicated DMA requests */
+ sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
+ reg | SUN6I_FIFO_CTL_DMA_DEDICATE);
+
+ /* Start transfer */
+ sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_XCH);
+
+ ret = sun6i_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);
+ }
+
+ sun6i_spi_drain_fifo(sspi, sspi->fifo_depth);
+
+ return ret;
+}
+
+static int sun6i_spi_transfer_one_pio(struct spi_device *spi,
+ struct spi_transfer *tfr)
+{
+ struct spi_master *master = spi->master;
+ struct sun6i_spi *sspi = spi_master_get_devdata(master);
+ unsigned int trig_level;
+ int ret;
+
+ /* Disable DMA requests */
+ sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG, 0);
+
+ /*
+ * Setup FIFO interrupt trigger level
+ * Here we choose 3/4 of the full fifo depth, as it's the hardcoded
+ * value used in old generation of Allwinner SPI controller.
+ * (See spi-sun4i.c)
+ */
+ trig_level = sspi->fifo_depth / 4 * 3;
+ sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
+ (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_POS));
+
+ sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
+
+ /* Enable the interrupts */
+ sun6i_spi_set(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC |
+ SUN6I_INT_CTL_RF_RDY);
+
+ /* Start transfer */
+ sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_XCH);
+
+ ret = sun6i_spi_wait_for_transfer(spi, tfr);
+
+ sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0);
+
+ return ret;
+}
+
static int sun6i_spi_transfer_one(struct spi_master *master,
struct spi_device *spi,
struct spi_transfer *tfr)
{
struct sun6i_spi *sspi = spi_master_get_devdata(master);
- unsigned int mclk_rate, div, timeout;
- unsigned int start, end, tx_time;
- unsigned int trig_level;
+ unsigned int mclk_rate, div;
unsigned int tx_len = 0;
- int ret = 0;
u32 reg;
/* A zero length transfer never finishes if programmed
@@ -285,10 +473,15 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
if (!tfr->len)
return 0;
- /* Don't support transfer larger than the FIFO */
- if (tfr->len > sspi->fifo_depth)
+ if (tfr->len > SUN6I_MAX_XFER_SIZE)
return -EMSGSIZE;
+ if (!master->can_dma) {
+ /* Don't support transfer larger than the FIFO */
+ if (tfr->len > sspi->fifo_depth)
+ return -EMSGSIZE;
+ }
+
sspi->tx_buf = tfr->tx_buf;
sspi->rx_buf = tfr->rx_buf;
sspi->len = tfr->len;
@@ -300,16 +493,6 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
SUN6I_FIFO_CTL_RF_RST | SUN6I_FIFO_CTL_TF_RST);
- /*
- * Setup FIFO interrupt trigger level
- * Here we choose 3/4 of the full fifo depth, as it's the hardcoded
- * value used in old generation of Allwinner SPI controller.
- * (See spi-sun4i.c)
- */
- trig_level = sspi->fifo_depth / 4 * 3;
- sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
- (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
-
/*
* If it's a TX only transfer, we don't want to fill the RX
* FIFO with bogus data
@@ -364,22 +547,10 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
sun6i_spi_write(sspi, SUN6I_BURST_CTL_CNT_REG,
SUN6I_BURST_CTL_CNT_STC(tx_len));
- /* Fill the TX FIFO */
- sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
-
- /* Enable the interrupts */
- sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
- SUN6I_INT_CTL_RF_RDY);
-
- /* Start the transfer */
- sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_XCH);
-
- /* Wait for completion */
- ret = sun6i_spi_wait_for_transfer(spi, tfr);
-
- sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0);
+ if (sun6i_spi_can_dma(master, spi, tfr))
+ return sun6i_spi_transfer_one_dma(spi, tfr);
- return ret;
+ return sun6i_spi_transfer_one_pio(spi, tfr);
}
static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
@@ -409,6 +580,77 @@ static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
return IRQ_NONE;
}
+static int sun6i_spi_dma_setup(struct platform_device *pdev,
+ struct resource *res)
+{
+ struct spi_master *master = platform_get_drvdata(pdev);
+ struct sun6i_spi *sspi = spi_master_get_devdata(master);
+ struct dma_slave_config dma_sconf;
+ int ret;
+
+ master->dma_tx = dma_request_slave_channel_reason(&pdev->dev, "tx");
+ if (IS_ERR(master->dma_tx)) {
+ dev_err(&pdev->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 + SUN6I_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(&pdev->dev, "Unable to configure DMA TX slave\n");
+ goto err_rel_tx;
+ }
+
+ master->dma_rx = dma_request_slave_channel_reason(&pdev->dev, "rx");
+ if (IS_ERR(master->dma_rx)) {
+ dev_err(&pdev->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 + SUN6I_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(&pdev->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 = sun6i_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 sun6i_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 sun6i_spi_runtime_resume(struct device *dev)
{
struct spi_master *master = dev_get_drvdata(dev);
@@ -530,6 +772,16 @@ static int sun6i_spi_probe(struct platform_device *pdev)
goto err_free_master;
}
+ ret = sun6i_spi_dma_setup(pdev, 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
@@ -556,14 +808,19 @@ err_pm_disable:
pm_runtime_disable(&pdev->dev);
sun6i_spi_runtime_suspend(&pdev->dev);
err_free_master:
+ sun6i_spi_dma_release(master);
spi_master_put(master);
return ret;
}
static int sun6i_spi_remove(struct platform_device *pdev)
{
+ struct spi_master *master = platform_get_drvdata(pdev);
+
pm_runtime_force_suspend(&pdev->dev);
+ sun6i_spi_dma_release(master);
+
return 0;
}
--
2.16.2
As long as the completion is 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.
Changes in v2:
1) Fixed issue with passing an invalid argument into devm_request_irq()
function.
Signed-off-by: Sergey Suloev <[email protected]>
---
drivers/spi/spi-sun6i.c | 52 ++++++++++++++++++++++++++++---------------------
1 file changed, 30 insertions(+), 22 deletions(-)
diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index c09ad10..0912404 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -99,8 +99,6 @@ struct sun6i_spi {
struct clk *mclk;
struct reset_control *rstc;
- struct completion done;
-
const u8 *tx_buf;
u8 *rx_buf;
int len;
@@ -246,6 +244,30 @@ static int sun6i_spi_prepare_message(struct spi_master *master,
return 0;
}
+static int sun6i_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;
+
+ /* smart wait for completion */
+ 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 sun6i_spi_transfer_one(struct spi_master *master,
struct spi_device *spi,
struct spi_transfer *tfr)
@@ -267,7 +289,6 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
if (tfr->len > sspi->fifo_depth)
return -EMSGSIZE;
- reinit_completion(&sspi->done);
sspi->tx_buf = tfr->tx_buf;
sspi->rx_buf = tfr->rx_buf;
sspi->len = tfr->len;
@@ -358,21 +379,9 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_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;
- }
+ /* Wait for completion */
+ ret = sun6i_spi_wait_for_transfer(spi, tfr);
-out:
sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0);
return ret;
@@ -380,7 +389,8 @@ out:
static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
{
- struct sun6i_spi *sspi = dev_id;
+ struct spi_master *master = dev_id;
+ struct sun6i_spi *sspi = spi_master_get_devdata(master);
u32 status;
status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
@@ -389,7 +399,7 @@ static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
if (status & SUN6I_INT_CTL_TC) {
sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_TC);
sun6i_spi_drain_fifo(sspi, sspi->fifo_depth);
- complete(&sspi->done);
+ spi_finalize_current_transfer(master);
return IRQ_HANDLED;
}
@@ -496,7 +506,7 @@ static int sun6i_spi_probe(struct platform_device *pdev)
}
ret = devm_request_irq(&pdev->dev, irq, sun6i_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;
@@ -518,8 +528,6 @@ static int sun6i_spi_probe(struct platform_device *pdev)
goto err_free_master;
}
- init_completion(&sspi->done);
-
sspi->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
if (IS_ERR(sspi->rstc)) {
dev_err(&pdev->dev, "Couldn't get reset controller\n");
--
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-sun6i.c | 37 ++++++++++++++++---------------------
1 file changed, 16 insertions(+), 21 deletions(-)
diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 0912404..2fa9d6e 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -115,29 +115,29 @@ static inline void sun6i_spi_write(struct sun6i_spi *sspi, u32 reg, u32 value)
writel(value, sspi->base_addr + reg);
}
-static inline u32 sun6i_spi_get_tx_fifo_count(struct sun6i_spi *sspi)
+static inline void sun6i_spi_set(struct sun6i_spi *sspi, u32 addr, u32 val)
{
- u32 reg = sun6i_spi_read(sspi, SUN6I_FIFO_STA_REG);
-
- reg >>= SUN6I_FIFO_STA_TF_CNT_BITS;
+ u32 reg = sun6i_spi_read(sspi, addr);
- return reg & SUN6I_FIFO_STA_TF_CNT_MASK;
+ reg |= val;
+ sun6i_spi_write(sspi, addr, reg);
}
-static inline void sun6i_spi_enable_interrupt(struct sun6i_spi *sspi, u32 mask)
+static inline void sun6i_spi_unset(struct sun6i_spi *sspi, u32 addr, u32 val)
{
- u32 reg = sun6i_spi_read(sspi, SUN6I_INT_CTL_REG);
+ u32 reg = sun6i_spi_read(sspi, addr);
- reg |= mask;
- sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, reg);
+ reg &= ~val;
+ sun6i_spi_write(sspi, addr, reg);
}
-static inline void sun6i_spi_disable_interrupt(struct sun6i_spi *sspi, u32 mask)
+static inline u32 sun6i_spi_get_tx_fifo_count(struct sun6i_spi *sspi)
{
- u32 reg = sun6i_spi_read(sspi, SUN6I_INT_CTL_REG);
+ u32 reg = sun6i_spi_read(sspi, SUN6I_FIFO_STA_REG);
- reg &= ~mask;
- sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, reg);
+ reg >>= SUN6I_FIFO_STA_TF_CNT_BITS;
+
+ return reg & SUN6I_FIFO_STA_TF_CNT_MASK;
}
static inline void sun6i_spi_drain_fifo(struct sun6i_spi *sspi, int len)
@@ -310,18 +310,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
-
- reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
/*
* If it's a TX only transfer, we don't want to fill the RX
* FIFO with bogus data
*/
if (sspi->rx_buf)
- reg &= ~SUN6I_TFR_CTL_DHB;
+ sun6i_spi_unset(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_DHB);
else
- reg |= SUN6I_TFR_CTL_DHB;
-
- sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg);
+ sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_DHB);
/* Ensure that we have a parent clock fast enough */
@@ -376,8 +372,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
SUN6I_INT_CTL_RF_RDY);
/* Start the transfer */
- reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
- sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_CTL_XCH);
+ sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_XCH);
/* Wait for completion */
ret = sun6i_spi_wait_for_transfer(spi, tfr);
--
2.16.2
Minor changes to fulfill the coding style and improve
the readability of the code.
Changes in v2:
1) Fixed issue with misplacing a piece of code that requires access
to the transfer structure into sun6i_spi_prepare_message() function
where the transfer structure is not available.
Signed-off-by: Sergey Suloev <[email protected]>
---
drivers/spi/spi-sun6i.c | 97 +++++++++++++++++++++++++++++--------------------
1 file changed, 58 insertions(+), 39 deletions(-)
diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 8533f4e..88ad45e 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -88,8 +88,12 @@
#define SUN6I_TXDATA_REG 0x200
#define SUN6I_RXDATA_REG 0x300
+#define SUN6I_SPI_MODE_BITS (SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST)
+
+#define SUN6I_SPI_MAX_SPEED_HZ 100000000
+#define SUN6I_SPI_MIN_SPEED_HZ 3000
+
struct sun6i_spi {
- struct spi_master *master;
void __iomem *base_addr;
struct clk *hclk;
struct clk *mclk;
@@ -189,6 +193,9 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
else
reg &= ~SUN6I_TFR_CTL_CS_LEVEL;
+ /* We want to control the chip select manually */
+ reg |= SUN6I_TFR_CTL_CS_MANUAL;
+
sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg);
}
@@ -197,6 +204,39 @@ static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
return SUN6I_MAX_XFER_SIZE - 1;
}
+static int sun6i_spi_prepare_message(struct spi_master *master,
+ struct spi_message *msg)
+{
+ struct spi_device *spi = msg->spi;
+ struct sun6i_spi *sspi = spi_master_get_devdata(master);
+ u32 reg;
+
+ /*
+ * Setup the transfer control register: Chip Select,
+ * polarities, etc.
+ */
+ reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
+
+ if (spi->mode & SPI_CPOL)
+ reg |= SUN6I_TFR_CTL_CPOL;
+ else
+ reg &= ~SUN6I_TFR_CTL_CPOL;
+
+ if (spi->mode & SPI_CPHA)
+ reg |= SUN6I_TFR_CTL_CPHA;
+ else
+ reg &= ~SUN6I_TFR_CTL_CPHA;
+
+ if (spi->mode & SPI_LSB_FIRST)
+ reg |= SUN6I_TFR_CTL_FBS;
+ else
+ reg &= ~SUN6I_TFR_CTL_FBS;
+
+ sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg);
+
+ return 0;
+}
+
static int sun6i_spi_transfer_one(struct spi_master *master,
struct spi_device *spi,
struct spi_transfer *tfr)
@@ -235,27 +275,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
(trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) |
(trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS));
- /*
- * Setup the transfer control register: Chip Select,
- * polarities, etc.
- */
- reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
-
- if (spi->mode & SPI_CPOL)
- reg |= SUN6I_TFR_CTL_CPOL;
- else
- reg &= ~SUN6I_TFR_CTL_CPOL;
-
- if (spi->mode & SPI_CPHA)
- reg |= SUN6I_TFR_CTL_CPHA;
- else
- reg &= ~SUN6I_TFR_CTL_CPHA;
-
- if (spi->mode & SPI_LSB_FIRST)
- reg |= SUN6I_TFR_CTL_FBS;
- else
- reg &= ~SUN6I_TFR_CTL_FBS;
+ reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
/*
* If it's a TX only transfer, we don't want to fill the RX
* FIFO with bogus data
@@ -265,11 +286,9 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
else
reg |= SUN6I_TFR_CTL_DHB;
- /* We want to control the chip select manually */
- reg |= SUN6I_TFR_CTL_CS_MANUAL;
-
sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg);
+
/* Ensure that we have a parent clock fast enough */
mclk_rate = clk_get_rate(sspi->mclk);
if (mclk_rate < (2 * tfr->speed_hz)) {
@@ -442,12 +461,24 @@ static int sun6i_spi_probe(struct platform_device *pdev)
struct resource *res;
int ret = 0, irq;
- master = spi_alloc_master(&pdev->dev, sizeof(struct sun6i_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 = SUN6I_SPI_MAX_SPEED_HZ;
+ master->min_speed_hz = SUN6I_SPI_MIN_SPEED_HZ;
+ master->num_chipselect = 4;
+ master->mode_bits = SUN6I_SPI_MODE_BITS;
+ master->bits_per_word_mask = SPI_BPW_MASK(8);
+ master->set_cs = sun6i_spi_set_cs;
+ master->prepare_message = sun6i_spi_prepare_message;
+ master->transfer_one = sun6i_spi_transfer_one;
+ master->max_transfer_size = sun6i_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);
@@ -466,26 +497,14 @@ static int sun6i_spi_probe(struct platform_device *pdev)
}
ret = devm_request_irq(&pdev->dev, irq, sun6i_spi_handler,
- 0, "sun6i-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;
sspi->fifo_depth = (unsigned long)of_device_get_match_data(&pdev->dev);
- master->max_speed_hz = 100 * 1000 * 1000;
- master->min_speed_hz = 3 * 1000;
- master->set_cs = sun6i_spi_set_cs;
- master->transfer_one = sun6i_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 = sun6i_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");
@@ -525,7 +544,7 @@ static int sun6i_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;
}
--
2.16.2
The chip select polarity flag is declared as supported
but is not handled in the code.
Signed-off-by: Sergey Suloev <[email protected]>
---
drivers/spi/spi-sun6i.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 88ad45e..78acc1f 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -193,6 +193,12 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
else
reg &= ~SUN6I_TFR_CTL_CS_LEVEL;
+ /* Handle chip select "reverse" polarity */
+ if (spi->mode & SPI_CS_HIGH)
+ reg &= ~SUN6I_TFR_CTL_SPOL;
+ else
+ reg |= SUN6I_TFR_CTL_SPOL;
+
/* We want to control the chip select manually */
reg |= SUN6I_TFR_CTL_CS_MANUAL;
--
2.16.2
There is no need to handle 3/4 empty interrupt as the maximum
supported transfer length in PIO mode is equal to FIFO depth,
i.e. 128 bytes for sun6i and 64 bytes for sun8i SoCs.
Changes in v3:
1) Restored processing of 3/4 FIFO full interrupt.
Signed-off-by: Sergey Suloev <[email protected]>
---
drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------
1 file changed, 17 insertions(+), 24 deletions(-)
diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 78acc1f..c09ad10 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -207,7 +207,10 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
{
- return SUN6I_MAX_XFER_SIZE - 1;
+ struct spi_master *master = spi->master;
+ struct sun6i_spi *sspi = spi_master_get_devdata(master);
+
+ return sspi->fifo_depth;
}
static int sun6i_spi_prepare_message(struct spi_master *master,
@@ -255,8 +258,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
int ret = 0;
u32 reg;
- if (tfr->len > SUN6I_MAX_XFER_SIZE)
- return -EINVAL;
+ /* A zero length transfer never finishes if programmed
+ in the hardware */
+ if (!tfr->len)
+ return 0;
+
+ /* Don't support transfer larger than the FIFO */
+ if (tfr->len > sspi->fifo_depth)
+ return -EMSGSIZE;
reinit_completion(&sspi->done);
sspi->tx_buf = tfr->tx_buf;
@@ -278,8 +287,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
*/
trig_level = sspi->fifo_depth / 4 * 3;
sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
- (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) |
- (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS));
+ (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
@@ -343,11 +351,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
/* Enable the interrupts */
- sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC);
sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
SUN6I_INT_CTL_RF_RDY);
- if (tx_len > sspi->fifo_depth)
- sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ);
/* Start the transfer */
reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
@@ -376,7 +381,9 @@ out:
static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
{
struct sun6i_spi *sspi = dev_id;
- u32 status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
+ u32 status;
+
+ status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
/* Transfer complete */
if (status & SUN6I_INT_CTL_TC) {
@@ -388,26 +395,12 @@ static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
/* Receive FIFO 3/4 full */
if (status & SUN6I_INT_CTL_RF_RDY) {
- sun6i_spi_drain_fifo(sspi, SUN6I_FIFO_DEPTH);
+ sun6i_spi_drain_fifo(sspi, sspi->fifo_depth);
/* Only clear the interrupt _after_ draining the FIFO */
sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_RF_RDY);
return IRQ_HANDLED;
}
- /* Transmit FIFO 3/4 empty */
- if (status & SUN6I_INT_CTL_TF_ERQ) {
- sun6i_spi_fill_fifo(sspi, SUN6I_FIFO_DEPTH);
-
- if (!sspi->len)
- /* nothing left to transmit */
- sun6i_spi_disable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ);
-
- /* Only clear the interrupt _after_ re-seeding the FIFO */
- sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_TF_ERQ);
-
- return IRQ_HANDLED;
- }
-
return IRQ_NONE;
}
--
2.16.2
Hi Sergey,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on spi/for-next]
[also build test ERROR on v4.16 next-20180403]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sergey-Suloev/spi-Add-support-for-DMA-transfers-in-sun6i-SPI-driver/20180404-053231
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=alpha
Note: the linux-review/Sergey-Suloev/spi-Add-support-for-DMA-transfers-in-sun6i-SPI-driver/20180404-053231 HEAD a0c010a285d830f07bb81ea59eaea8773d78b74c builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
drivers//spi/spi-sun6i.c: In function 'sun6i_spi_transfer_one':
>> drivers//spi/spi-sun6i.c:371:2: error: implicit declaration of function 'sun6i_spi_enable_interrupt'; did you mean 'sun6i_spi_transfer_one'? [-Werror=implicit-function-declaration]
sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
^~~~~~~~~~~~~~~~~~~~~~~~~~
sun6i_spi_transfer_one
drivers//spi/spi-sun6i.c:277:27: warning: unused variable 'tx_time' [-Wunused-variable]
unsigned int start, end, tx_time;
^~~~~~~
drivers//spi/spi-sun6i.c:277:22: warning: unused variable 'end' [-Wunused-variable]
unsigned int start, end, tx_time;
^~~
drivers//spi/spi-sun6i.c:277:15: warning: unused variable 'start' [-Wunused-variable]
unsigned int start, end, tx_time;
^~~~~
drivers//spi/spi-sun6i.c:276:31: warning: unused variable 'timeout' [-Wunused-variable]
unsigned int mclk_rate, div, timeout;
^~~~~~~
cc1: some warnings being treated as errors
vim +371 drivers//spi/spi-sun6i.c
43836daab Sergey Suloev 2018-04-03 270
3558fe900 Maxime Ripard 2014-02-05 271 static int sun6i_spi_transfer_one(struct spi_master *master,
3558fe900 Maxime Ripard 2014-02-05 272 struct spi_device *spi,
3558fe900 Maxime Ripard 2014-02-05 273 struct spi_transfer *tfr)
3558fe900 Maxime Ripard 2014-02-05 274 {
3558fe900 Maxime Ripard 2014-02-05 275 struct sun6i_spi *sspi = spi_master_get_devdata(master);
3558fe900 Maxime Ripard 2014-02-05 276 unsigned int mclk_rate, div, timeout;
719bd6542 Michal Suchanek 2016-06-13 277 unsigned int start, end, tx_time;
913f536c6 Icenowy Zheng 2017-03-06 278 unsigned int trig_level;
3558fe900 Maxime Ripard 2014-02-05 279 unsigned int tx_len = 0;
3558fe900 Maxime Ripard 2014-02-05 280 int ret = 0;
3558fe900 Maxime Ripard 2014-02-05 281 u32 reg;
3558fe900 Maxime Ripard 2014-02-05 282
e31cf0250 Sergey Suloev 2018-04-03 283 /* A zero length transfer never finishes if programmed
e31cf0250 Sergey Suloev 2018-04-03 284 in the hardware */
e31cf0250 Sergey Suloev 2018-04-03 285 if (!tfr->len)
e31cf0250 Sergey Suloev 2018-04-03 286 return 0;
e31cf0250 Sergey Suloev 2018-04-03 287
e31cf0250 Sergey Suloev 2018-04-03 288 /* Don't support transfer larger than the FIFO */
e31cf0250 Sergey Suloev 2018-04-03 289 if (tfr->len > sspi->fifo_depth)
e31cf0250 Sergey Suloev 2018-04-03 290 return -EMSGSIZE;
3558fe900 Maxime Ripard 2014-02-05 291
3558fe900 Maxime Ripard 2014-02-05 292 sspi->tx_buf = tfr->tx_buf;
3558fe900 Maxime Ripard 2014-02-05 293 sspi->rx_buf = tfr->rx_buf;
3558fe900 Maxime Ripard 2014-02-05 294 sspi->len = tfr->len;
3558fe900 Maxime Ripard 2014-02-05 295
3558fe900 Maxime Ripard 2014-02-05 296 /* Clear pending interrupts */
3558fe900 Maxime Ripard 2014-02-05 297 sun6i_spi_write(sspi, SUN6I_INT_STA_REG, ~0);
3558fe900 Maxime Ripard 2014-02-05 298
3558fe900 Maxime Ripard 2014-02-05 299 /* Reset FIFO */
3558fe900 Maxime Ripard 2014-02-05 300 sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
3558fe900 Maxime Ripard 2014-02-05 301 SUN6I_FIFO_CTL_RF_RST | SUN6I_FIFO_CTL_TF_RST);
3558fe900 Maxime Ripard 2014-02-05 302
3558fe900 Maxime Ripard 2014-02-05 303 /*
913f536c6 Icenowy Zheng 2017-03-06 304 * Setup FIFO interrupt trigger level
913f536c6 Icenowy Zheng 2017-03-06 305 * Here we choose 3/4 of the full fifo depth, as it's the hardcoded
913f536c6 Icenowy Zheng 2017-03-06 306 * value used in old generation of Allwinner SPI controller.
913f536c6 Icenowy Zheng 2017-03-06 307 * (See spi-sun4i.c)
913f536c6 Icenowy Zheng 2017-03-06 308 */
913f536c6 Icenowy Zheng 2017-03-06 309 trig_level = sspi->fifo_depth / 4 * 3;
913f536c6 Icenowy Zheng 2017-03-06 310 sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
e31cf0250 Sergey Suloev 2018-04-03 311 (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
913f536c6 Icenowy Zheng 2017-03-06 312
3558fe900 Maxime Ripard 2014-02-05 313 /*
3558fe900 Maxime Ripard 2014-02-05 314 * If it's a TX only transfer, we don't want to fill the RX
3558fe900 Maxime Ripard 2014-02-05 315 * FIFO with bogus data
3558fe900 Maxime Ripard 2014-02-05 316 */
3558fe900 Maxime Ripard 2014-02-05 317 if (sspi->rx_buf)
2c98d976c Sergey Suloev 2018-04-03 318 sun6i_spi_unset(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_DHB);
3558fe900 Maxime Ripard 2014-02-05 319 else
2c98d976c Sergey Suloev 2018-04-03 320 sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_DHB);
3558fe900 Maxime Ripard 2014-02-05 321
09d186f3c Sergey Suloev 2018-04-03 322
3558fe900 Maxime Ripard 2014-02-05 323 /* Ensure that we have a parent clock fast enough */
3558fe900 Maxime Ripard 2014-02-05 324 mclk_rate = clk_get_rate(sspi->mclk);
47284e3e0 Marcus Weseloh 2015-11-08 325 if (mclk_rate < (2 * tfr->speed_hz)) {
47284e3e0 Marcus Weseloh 2015-11-08 326 clk_set_rate(sspi->mclk, 2 * tfr->speed_hz);
3558fe900 Maxime Ripard 2014-02-05 327 mclk_rate = clk_get_rate(sspi->mclk);
3558fe900 Maxime Ripard 2014-02-05 328 }
3558fe900 Maxime Ripard 2014-02-05 329
3558fe900 Maxime Ripard 2014-02-05 330 /*
3558fe900 Maxime Ripard 2014-02-05 331 * Setup clock divider.
3558fe900 Maxime Ripard 2014-02-05 332 *
3558fe900 Maxime Ripard 2014-02-05 333 * We have two choices there. Either we can use the clock
3558fe900 Maxime Ripard 2014-02-05 334 * divide rate 1, which is calculated thanks to this formula:
3558fe900 Maxime Ripard 2014-02-05 335 * SPI_CLK = MOD_CLK / (2 ^ cdr)
3558fe900 Maxime Ripard 2014-02-05 336 * Or we can use CDR2, which is calculated with the formula:
3558fe900 Maxime Ripard 2014-02-05 337 * SPI_CLK = MOD_CLK / (2 * (cdr + 1))
3558fe900 Maxime Ripard 2014-02-05 338 * Wether we use the former or the latter is set through the
3558fe900 Maxime Ripard 2014-02-05 339 * DRS bit.
3558fe900 Maxime Ripard 2014-02-05 340 *
3558fe900 Maxime Ripard 2014-02-05 341 * First try CDR2, and if we can't reach the expected
3558fe900 Maxime Ripard 2014-02-05 342 * frequency, fall back to CDR1.
3558fe900 Maxime Ripard 2014-02-05 343 */
47284e3e0 Marcus Weseloh 2015-11-08 344 div = mclk_rate / (2 * tfr->speed_hz);
3558fe900 Maxime Ripard 2014-02-05 345 if (div <= (SUN6I_CLK_CTL_CDR2_MASK + 1)) {
3558fe900 Maxime Ripard 2014-02-05 346 if (div > 0)
3558fe900 Maxime Ripard 2014-02-05 347 div--;
3558fe900 Maxime Ripard 2014-02-05 348
3558fe900 Maxime Ripard 2014-02-05 349 reg = SUN6I_CLK_CTL_CDR2(div) | SUN6I_CLK_CTL_DRS;
3558fe900 Maxime Ripard 2014-02-05 350 } else {
47284e3e0 Marcus Weseloh 2015-11-08 351 div = ilog2(mclk_rate) - ilog2(tfr->speed_hz);
3558fe900 Maxime Ripard 2014-02-05 352 reg = SUN6I_CLK_CTL_CDR1(div);
3558fe900 Maxime Ripard 2014-02-05 353 }
3558fe900 Maxime Ripard 2014-02-05 354
3558fe900 Maxime Ripard 2014-02-05 355 sun6i_spi_write(sspi, SUN6I_CLK_CTL_REG, reg);
3558fe900 Maxime Ripard 2014-02-05 356
3558fe900 Maxime Ripard 2014-02-05 357 /* Setup the transfer now... */
3558fe900 Maxime Ripard 2014-02-05 358 if (sspi->tx_buf)
3558fe900 Maxime Ripard 2014-02-05 359 tx_len = tfr->len;
3558fe900 Maxime Ripard 2014-02-05 360
3558fe900 Maxime Ripard 2014-02-05 361 /* Setup the counters */
3558fe900 Maxime Ripard 2014-02-05 362 sun6i_spi_write(sspi, SUN6I_BURST_CNT_REG, SUN6I_BURST_CNT(tfr->len));
3558fe900 Maxime Ripard 2014-02-05 363 sun6i_spi_write(sspi, SUN6I_XMIT_CNT_REG, SUN6I_XMIT_CNT(tx_len));
3558fe900 Maxime Ripard 2014-02-05 364 sun6i_spi_write(sspi, SUN6I_BURST_CTL_CNT_REG,
3558fe900 Maxime Ripard 2014-02-05 365 SUN6I_BURST_CTL_CNT_STC(tx_len));
3558fe900 Maxime Ripard 2014-02-05 366
3558fe900 Maxime Ripard 2014-02-05 367 /* Fill the TX FIFO */
10565dfd3 Milo Kim 2016-10-28 368 sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
3558fe900 Maxime Ripard 2014-02-05 369
3558fe900 Maxime Ripard 2014-02-05 370 /* Enable the interrupts */
913f536c6 Icenowy Zheng 2017-03-06 @371 sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
913f536c6 Icenowy Zheng 2017-03-06 372 SUN6I_INT_CTL_RF_RDY);
3558fe900 Maxime Ripard 2014-02-05 373
3558fe900 Maxime Ripard 2014-02-05 374 /* Start the transfer */
2c98d976c Sergey Suloev 2018-04-03 375 sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_XCH);
3558fe900 Maxime Ripard 2014-02-05 376
43836daab Sergey Suloev 2018-04-03 377 /* Wait for completion */
43836daab Sergey Suloev 2018-04-03 378 ret = sun6i_spi_wait_for_transfer(spi, tfr);
3558fe900 Maxime Ripard 2014-02-05 379
3558fe900 Maxime Ripard 2014-02-05 380 sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0);
3558fe900 Maxime Ripard 2014-02-05 381
3558fe900 Maxime Ripard 2014-02-05 382 return ret;
3558fe900 Maxime Ripard 2014-02-05 383 }
3558fe900 Maxime Ripard 2014-02-05 384
:::::: The code at line 371 was first introduced by commit
:::::: 913f536c6c18a2e19e32f06971101c1d0ae3739c spi: sun6i: Allow transfers larger than FIFO size
:::::: TO: Icenowy Zheng <[email protected]>
:::::: CC: Mark Brown <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tue, Apr 03, 2018 at 06:44:44PM +0300, Sergey Suloev wrote:
> Minor changes to fulfill the coding style and improve
> the readability of the code.
>
> Changes in v2:
> 1) Fixed issue with misplacing a piece of code that requires access
> to the transfer structure into sun6i_spi_prepare_message() function
> where the transfer structure is not available.
This shouldn't be in your commit log.
> Signed-off-by: Sergey Suloev <[email protected]>
> ---
> drivers/spi/spi-sun6i.c | 97 +++++++++++++++++++++++++++++--------------------
> 1 file changed, 58 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
> index 8533f4e..88ad45e 100644
> --- a/drivers/spi/spi-sun6i.c
> +++ b/drivers/spi/spi-sun6i.c
> @@ -88,8 +88,12 @@
> #define SUN6I_TXDATA_REG 0x200
> #define SUN6I_RXDATA_REG 0x300
>
> +#define SUN6I_SPI_MODE_BITS (SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST)
> +
> +#define SUN6I_SPI_MAX_SPEED_HZ 100000000
> +#define SUN6I_SPI_MIN_SPEED_HZ 3000
> +
> struct sun6i_spi {
> - struct spi_master *master;
> void __iomem *base_addr;
> struct clk *hclk;
> struct clk *mclk;
> @@ -189,6 +193,9 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
> else
> reg &= ~SUN6I_TFR_CTL_CS_LEVEL;
>
> + /* We want to control the chip select manually */
> + reg |= SUN6I_TFR_CTL_CS_MANUAL;
> +
> sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg);
> }
>
> @@ -197,6 +204,39 @@ static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
> return SUN6I_MAX_XFER_SIZE - 1;
> }
>
> +static int sun6i_spi_prepare_message(struct spi_master *master,
> + struct spi_message *msg)
> +{
> + struct spi_device *spi = msg->spi;
> + struct sun6i_spi *sspi = spi_master_get_devdata(master);
> + u32 reg;
> +
> + /*
> + * Setup the transfer control register: Chip Select,
> + * polarities, etc.
> + */
> + reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
> +
> + if (spi->mode & SPI_CPOL)
> + reg |= SUN6I_TFR_CTL_CPOL;
> + else
> + reg &= ~SUN6I_TFR_CTL_CPOL;
> +
> + if (spi->mode & SPI_CPHA)
> + reg |= SUN6I_TFR_CTL_CPHA;
> + else
> + reg &= ~SUN6I_TFR_CTL_CPHA;
> +
> + if (spi->mode & SPI_LSB_FIRST)
> + reg |= SUN6I_TFR_CTL_FBS;
> + else
> + reg &= ~SUN6I_TFR_CTL_FBS;
> +
> + sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg);
> +
> + return 0;
> +}
> +
You're doing way more than just "coding style improvements", please
split the patches accordingly.
And slow down between versions, you had 0 review on the v1 and the v2,
and you're at v3 already in just a couple of days.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Apr 03, 2018 at 06:44:46PM +0300, Sergey Suloev wrote:
> There is no need to handle 3/4 empty interrupt as the maximum
> supported transfer length in PIO mode is equal to FIFO depth,
> i.e. 128 bytes for sun6i and 64 bytes for sun8i SoCs.
>
> Changes in v3:
> 1) Restored processing of 3/4 FIFO full interrupt.
>
> Signed-off-by: Sergey Suloev <[email protected]>
> ---
> drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------
> 1 file changed, 17 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
> index 78acc1f..c09ad10 100644
> --- a/drivers/spi/spi-sun6i.c
> +++ b/drivers/spi/spi-sun6i.c
> @@ -207,7 +207,10 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
>
> static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
> {
> - return SUN6I_MAX_XFER_SIZE - 1;
> + struct spi_master *master = spi->master;
> + struct sun6i_spi *sspi = spi_master_get_devdata(master);
> +
> + return sspi->fifo_depth;
Doesn't that effectively revert 3288d5cb40c0 ?
Why do you need to do so?
> }
>
> static int sun6i_spi_prepare_message(struct spi_master *master,
> @@ -255,8 +258,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
> int ret = 0;
> u32 reg;
>
> - if (tfr->len > SUN6I_MAX_XFER_SIZE)
> - return -EINVAL;
> + /* A zero length transfer never finishes if programmed
> + in the hardware */
Improper comment style here. Please make sure to run checkpatch before
sending your patches.
> + if (!tfr->len)
> + return 0;
Can that case even happen?
> + /* Don't support transfer larger than the FIFO */
> + if (tfr->len > sspi->fifo_depth)
> + return -EMSGSIZE;
You're changing the return type, why?
> reinit_completion(&sspi->done);
> sspi->tx_buf = tfr->tx_buf;
> @@ -278,8 +287,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
> */
> trig_level = sspi->fifo_depth / 4 * 3;
> sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
> - (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) |
> - (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS));
> + (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
>
>
> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
> @@ -343,11 +351,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
> sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
>
> /* Enable the interrupts */
> - sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC);
> sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
> SUN6I_INT_CTL_RF_RDY);
> - if (tx_len > sspi->fifo_depth)
> - sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ);
This would also need to be explained in your commit log.
>
> /* Start the transfer */
> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
> @@ -376,7 +381,9 @@ out:
> static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
> {
> struct sun6i_spi *sspi = dev_id;
> - u32 status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
> + u32 status;
> +
> + status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
Why is this change needed?
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Apr 03, 2018 at 06:44:47PM +0300, Sergey Suloev wrote:
> As long as the completion is 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 would need to be split in two patches, one to add the waiting
function, the other one to convert to the core struct completion
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Apr 03, 2018 at 06:44:49PM +0300, Sergey Suloev wrote:
> DMA transfers are now available for sun6i and sun8i SoCs.
> The DMA mode is used automatically as soon as requested
> transfer length is more than FIFO length.
>
> Changes in v3:
> 1) Debug log enhancements.
>
> Signed-off-by: Sergey Suloev <[email protected]>
> ---
> drivers/spi/spi-sun6i.c | 331 ++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 294 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
> index 2fa9d6e..7f41871 100644
> --- a/drivers/spi/spi-sun6i.c
> +++ b/drivers/spi/spi-sun6i.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>
> @@ -55,17 +57,20 @@
>
> #define SUN6I_FIFO_CTL_REG 0x18
> #define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_MASK 0xff
> -#define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS 0
> +#define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_POS 0
Why are you changing that name
> +#define SUN6I_FIFO_CTL_RF_DRQ_EN BIT(8)
> #define SUN6I_FIFO_CTL_RF_RST BIT(15)
> #define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_MASK 0xff
> -#define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS 16
> +#define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_POS 16
Ditto.
> +#define SUN6I_FIFO_CTL_TF_DRQ_EN BIT(24)
> #define SUN6I_FIFO_CTL_TF_RST BIT(31)
> +#define SUN6I_FIFO_CTL_DMA_DEDICATE BIT(9)|BIT(25)
>
> #define SUN6I_FIFO_STA_REG 0x1c
> #define SUN6I_FIFO_STA_RF_CNT_MASK 0x7f
> -#define SUN6I_FIFO_STA_RF_CNT_BITS 0
> +#define SUN6I_FIFO_STA_RF_CNT_POS 0
Ditto.
> #define SUN6I_FIFO_STA_TF_CNT_MASK 0x7f
> -#define SUN6I_FIFO_STA_TF_CNT_BITS 16
> +#define SUN6I_FIFO_STA_TF_CNT_POS 16
Ditto.
>
> #define SUN6I_CLK_CTL_REG 0x24
> #define SUN6I_CLK_CTL_CDR2_MASK 0xff
> @@ -135,7 +140,7 @@ static inline u32 sun6i_spi_get_tx_fifo_count(struct sun6i_spi *sspi)
> {
> u32 reg = sun6i_spi_read(sspi, SUN6I_FIFO_STA_REG);
>
> - reg >>= SUN6I_FIFO_STA_TF_CNT_BITS;
> + reg >>= SUN6I_FIFO_STA_TF_CNT_POS;
>
> return reg & SUN6I_FIFO_STA_TF_CNT_MASK;
> }
> @@ -148,7 +153,7 @@ static inline void sun6i_spi_drain_fifo(struct sun6i_spi *sspi, int len)
> /* See how much data is available */
> reg = sun6i_spi_read(sspi, SUN6I_FIFO_STA_REG);
> reg &= SUN6I_FIFO_STA_RF_CNT_MASK;
> - cnt = reg >> SUN6I_FIFO_STA_RF_CNT_BITS;
> + cnt = reg >> SUN6I_FIFO_STA_RF_CNT_POS;
>
> if (len > cnt)
> len = cnt;
> @@ -177,6 +182,15 @@ static inline void sun6i_spi_fill_fifo(struct sun6i_spi *sspi, int len)
> }
> }
>
> +static bool sun6i_spi_can_dma(struct spi_master *master,
> + struct spi_device *spi,
> + struct spi_transfer *tfr)
> +{
> + struct sun6i_spi *sspi = spi_master_get_devdata(master);
> +
> + return tfr->len > sspi->fifo_depth;
> +}
> +
> static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
> {
> struct sun6i_spi *sspi = spi_master_get_devdata(spi->master);
> @@ -208,6 +222,9 @@ static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
> struct spi_master *master = spi->master;
> struct sun6i_spi *sspi = spi_master_get_devdata(master);
>
> + if (master->can_dma)
> + return SUN6I_MAX_XFER_SIZE;
> +
> return sspi->fifo_depth;
> }
>
> @@ -268,16 +285,187 @@ static int sun6i_spi_wait_for_transfer(struct spi_device *spi,
> return 0;
> }
>
> +static void sun6i_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 sun6i_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 = sun6i_spi_dma_callback;
> + chan_desc->callback_param = master;
> +
> + *cookie = dmaengine_submit(chan_desc);
> + dma_async_issue_pending(master->dma_tx);
> +
> + return 0;
> +}
> +
> +static int sun6i_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 = sun6i_spi_dma_callback;
> + chan_desc->callback_param = master;
> +
> + *cookie = dmaengine_submit(chan_desc);
> + dma_async_issue_pending(master->dma_rx);
> +
> + return 0;
> +}
> +
> +static int sun6i_spi_transfer_one_dma(struct spi_device *spi,
> + struct spi_transfer *tfr)
> +{
> + struct spi_master *master = spi->master;
> + struct sun6i_spi *sspi = spi_master_get_devdata(master);
> + dma_cookie_t tx_cookie = 0,rx_cookie = 0;
> + enum dma_status status;
> + int ret;
> + u32 reg, trig_level = 0;
> +
> + dev_dbg(&master->dev, "Using DMA mode for transfer\n");
> +
> + /* Disable interrupts */
> + sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0);
> +
> + reg = sun6i_spi_read(sspi, SUN6I_FIFO_CTL_REG);
> +
> + if (sspi->tx_buf) {
> + ret = sun6i_spi_dmap_prep_tx(master, tfr, &tx_cookie);
> + if (ret)
> + goto out;
> +
> + reg |= SUN6I_FIFO_CTL_TF_DRQ_EN;
> +
> + trig_level = sspi->fifo_depth;
> + reg &= ~SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_MASK;
> + reg |= (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_POS);
> + }
> +
> + if (sspi->rx_buf) {
> + ret = sun6i_spi_dmap_prep_rx(master, tfr, &rx_cookie);
> + if (ret)
> + goto out;
> +
> + reg |= SUN6I_FIFO_CTL_RF_DRQ_EN;
> +
> + trig_level = 1;
> + reg &= ~SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_MASK;
> + reg |= (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_POS);
> + }
> +
> + /* Enable Dedicated DMA requests */
> + sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
> + reg | SUN6I_FIFO_CTL_DMA_DEDICATE);
> +
> + /* Start transfer */
> + sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_XCH);
> +
> + ret = sun6i_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");
> + }
I'm not sure why that would be needed.
> +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);
> + }
That can be addressed through additional labels.
> + sun6i_spi_drain_fifo(sspi, sspi->fifo_depth);
Why are you doing a PIO access here?
> + return ret;
> +}
> +
> +static int sun6i_spi_transfer_one_pio(struct spi_device *spi,
> + struct spi_transfer *tfr)
> +{
> + struct spi_master *master = spi->master;
> + struct sun6i_spi *sspi = spi_master_get_devdata(master);
> + unsigned int trig_level;
> + int ret;
> +
> + /* Disable DMA requests */
> + sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG, 0);
> +
> + /*
> + * Setup FIFO interrupt trigger level
> + * Here we choose 3/4 of the full fifo depth, as it's the hardcoded
> + * value used in old generation of Allwinner SPI controller.
> + * (See spi-sun4i.c)
> + */
> + trig_level = sspi->fifo_depth / 4 * 3;
> + sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
> + (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_POS));
> +
> + sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
> +
> + /* Enable the interrupts */
> + sun6i_spi_set(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC |
> + SUN6I_INT_CTL_RF_RDY);
> +
> + /* Start transfer */
> + sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_XCH);
> +
> + ret = sun6i_spi_wait_for_transfer(spi, tfr);
> +
> + sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0);
> +
> + return ret;
> +}
> +
> static int sun6i_spi_transfer_one(struct spi_master *master,
> struct spi_device *spi,
> struct spi_transfer *tfr)
> {
> struct sun6i_spi *sspi = spi_master_get_devdata(master);
> - unsigned int mclk_rate, div, timeout;
> - unsigned int start, end, tx_time;
> - unsigned int trig_level;
> + unsigned int mclk_rate, div;
> unsigned int tx_len = 0;
> - int ret = 0;
> u32 reg;
>
> /* A zero length transfer never finishes if programmed
> @@ -285,10 +473,15 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
> if (!tfr->len)
> return 0;
>
> - /* Don't support transfer larger than the FIFO */
> - if (tfr->len > sspi->fifo_depth)
> + if (tfr->len > SUN6I_MAX_XFER_SIZE)
> return -EMSGSIZE;
>
> + if (!master->can_dma) {
> + /* Don't support transfer larger than the FIFO */
> + if (tfr->len > sspi->fifo_depth)
> + return -EMSGSIZE;
> + }
> +
> sspi->tx_buf = tfr->tx_buf;
> sspi->rx_buf = tfr->rx_buf;
> sspi->len = tfr->len;
> @@ -300,16 +493,6 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
> sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
> SUN6I_FIFO_CTL_RF_RST | SUN6I_FIFO_CTL_TF_RST);
>
> - /*
> - * Setup FIFO interrupt trigger level
> - * Here we choose 3/4 of the full fifo depth, as it's the hardcoded
> - * value used in old generation of Allwinner SPI controller.
> - * (See spi-sun4i.c)
> - */
> - trig_level = sspi->fifo_depth / 4 * 3;
> - sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
> - (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
> -
> /*
> * If it's a TX only transfer, we don't want to fill the RX
> * FIFO with bogus data
> @@ -364,22 +547,10 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
> sun6i_spi_write(sspi, SUN6I_BURST_CTL_CNT_REG,
> SUN6I_BURST_CTL_CNT_STC(tx_len));
>
> - /* Fill the TX FIFO */
> - sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
> -
> - /* Enable the interrupts */
> - sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
> - SUN6I_INT_CTL_RF_RDY);
> -
> - /* Start the transfer */
> - sun6i_spi_set(sspi, SUN6I_TFR_CTL_REG, SUN6I_TFR_CTL_XCH);
> -
> - /* Wait for completion */
> - ret = sun6i_spi_wait_for_transfer(spi, tfr);
> -
> - sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0);
> + if (sun6i_spi_can_dma(master, spi, tfr))
> + return sun6i_spi_transfer_one_dma(spi, tfr);
>
> - return ret;
> + return sun6i_spi_transfer_one_pio(spi, tfr);
You should really split the function creation here into a separate patch
> }
>
> static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
> @@ -409,6 +580,77 @@ static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
> return IRQ_NONE;
> }
>
> +static int sun6i_spi_dma_setup(struct platform_device *pdev,
> + struct resource *res)
> +{
> + struct spi_master *master = platform_get_drvdata(pdev);
> + struct sun6i_spi *sspi = spi_master_get_devdata(master);
> + struct dma_slave_config dma_sconf;
> + int ret;
> +
> + master->dma_tx = dma_request_slave_channel_reason(&pdev->dev, "tx");
> + if (IS_ERR(master->dma_tx)) {
> + dev_err(&pdev->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 + SUN6I_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(&pdev->dev, "Unable to configure DMA TX slave\n");
> + goto err_rel_tx;
> + }
> +
> + master->dma_rx = dma_request_slave_channel_reason(&pdev->dev, "rx");
> + if (IS_ERR(master->dma_rx)) {
> + dev_err(&pdev->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 + SUN6I_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(&pdev->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 = sun6i_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 sun6i_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 sun6i_spi_runtime_resume(struct device *dev)
> {
> struct spi_master *master = dev_get_drvdata(dev);
> @@ -530,6 +772,16 @@ static int sun6i_spi_probe(struct platform_device *pdev)
> goto err_free_master;
> }
>
> + ret = sun6i_spi_dma_setup(pdev, res);
> + if (ret) {
> + if (ret == -EPROBE_DEFER) {
> + /* wait for the dma driver to load */
There's no need for that comment.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Apr 03, 2018 at 06:44:48PM +0300, Sergey Suloev wrote:
> Two helper functions were added in order to set/unset
> specified flags in registers.
>
> Signed-off-by: Sergey Suloev <[email protected]>
Again, I'm not really sure what the benefit of that is. Your diffstat
is pretty much identical, so it's basically just moving code around.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On 04/04/2018 09:50 AM, Maxime Ripard wrote:
> On Tue, Apr 03, 2018 at 06:44:46PM +0300, Sergey Suloev wrote:
>> There is no need to handle 3/4 empty interrupt as the maximum
>> supported transfer length in PIO mode is equal to FIFO depth,
>> i.e. 128 bytes for sun6i and 64 bytes for sun8i SoCs.
>>
>> Changes in v3:
>> 1) Restored processing of 3/4 FIFO full interrupt.
>>
>> Signed-off-by: Sergey Suloev <[email protected]>
>> ---
>> drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------
>> 1 file changed, 17 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
>> index 78acc1f..c09ad10 100644
>> --- a/drivers/spi/spi-sun6i.c
>> +++ b/drivers/spi/spi-sun6i.c
>> @@ -207,7 +207,10 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
>>
>> static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
>> {
>> - return SUN6I_MAX_XFER_SIZE - 1;
>> + struct spi_master *master = spi->master;
>> + struct sun6i_spi *sspi = spi_master_get_devdata(master);
>> +
>> + return sspi->fifo_depth;
> Doesn't that effectively revert 3288d5cb40c0 ?
>
> Why do you need to do so?
Need what?
Change is supposed to restrict max transfer size for PIO mode otherwise
it will fail.
The maximum transfer length = FIFO depth for PIO mode.
>
>> }
>>
>> static int sun6i_spi_prepare_message(struct spi_master *master,
>> @@ -255,8 +258,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>> int ret = 0;
>> u32 reg;
>>
>> - if (tfr->len > SUN6I_MAX_XFER_SIZE)
>> - return -EINVAL;
>> + /* A zero length transfer never finishes if programmed
>> + in the hardware */
> Improper comment style here. Please make sure to run checkpatch before
> sending your patches.
ok
>
>> + if (!tfr->len)
>> + return 0;
> Can that case even happen?
Not sure, just for safety.
>
>> + /* Don't support transfer larger than the FIFO */
>> + if (tfr->len > sspi->fifo_depth)
>> + return -EMSGSIZE;
> You're changing the return type, why?
As? a more appropriate one
>
>> reinit_completion(&sspi->done);
>> sspi->tx_buf = tfr->tx_buf;
>> @@ -278,8 +287,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>> */
>> trig_level = sspi->fifo_depth / 4 * 3;
>> sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
>> - (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) |
>> - (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS));
>> + (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
>>
>>
>> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>> @@ -343,11 +351,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>> sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
>>
>> /* Enable the interrupts */
>> - sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC);
>> sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
>> SUN6I_INT_CTL_RF_RDY);
>> - if (tx_len > sspi->fifo_depth)
>> - sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ);
> This would also need to be explained in your commit log.
What exactly and in what way ?
>
>>
>> /* Start the transfer */
>> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>> @@ -376,7 +381,9 @@ out:
>> static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
>> {
>> struct sun6i_spi *sspi = dev_id;
>> - u32 status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
>> + u32 status;
>> +
>> + status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
> Why is this change needed?
A minor one, for readability.
>
> Maxime
>
On Wed, Apr 04, 2018 at 02:35:14PM +0300, Sergey Suloev wrote:
> On 04/04/2018 09:50 AM, Maxime Ripard wrote:
> > On Tue, Apr 03, 2018 at 06:44:46PM +0300, Sergey Suloev wrote:
> > > There is no need to handle 3/4 empty interrupt as the maximum
> > > supported transfer length in PIO mode is equal to FIFO depth,
> > > i.e. 128 bytes for sun6i and 64 bytes for sun8i SoCs.
> > >
> > > Changes in v3:
> > > 1) Restored processing of 3/4 FIFO full interrupt.
> > >
> > > Signed-off-by: Sergey Suloev <[email protected]>
> > > ---
> > > drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------
> > > 1 file changed, 17 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
> > > index 78acc1f..c09ad10 100644
> > > --- a/drivers/spi/spi-sun6i.c
> > > +++ b/drivers/spi/spi-sun6i.c
> > > @@ -207,7 +207,10 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
> > > static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
> > > {
> > > - return SUN6I_MAX_XFER_SIZE - 1;
> > > + struct spi_master *master = spi->master;
> > > + struct sun6i_spi *sspi = spi_master_get_devdata(master);
> > > +
> > > + return sspi->fifo_depth;
> > Doesn't that effectively revert 3288d5cb40c0 ?
> >
> > Why do you need to do so?
>
> Need what?
Why do you need to revert that change.
> Change is supposed to restrict max transfer size for PIO mode otherwise it
> will fail.
> The maximum transfer length = FIFO depth for PIO mode.
The point of that patch was precisely to allow to send more data than
the FIFO. You're breaking that behaviour without any justification,
and this is not ok.
> >
> > > }
> > > static int sun6i_spi_prepare_message(struct spi_master *master,
> > > @@ -255,8 +258,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
> > > int ret = 0;
> > > u32 reg;
> > > - if (tfr->len > SUN6I_MAX_XFER_SIZE)
> > > - return -EINVAL;
> > > + /* A zero length transfer never finishes if programmed
> > > + in the hardware */
> > Improper comment style here. Please make sure to run checkpatch before
> > sending your patches.
> ok
> >
> > > + if (!tfr->len)
> > > + return 0;
> > Can that case even happen?
> Not sure, just for safety.
> >
> > > + /* Don't support transfer larger than the FIFO */
> > > + if (tfr->len > sspi->fifo_depth)
> > > + return -EMSGSIZE;
> > You're changing the return type, why?
> As? a more appropriate one
The fact that it's more appropriate should be justified.
> >
> > > reinit_completion(&sspi->done);
> > > sspi->tx_buf = tfr->tx_buf;
> > > @@ -278,8 +287,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
> > > */
> > > trig_level = sspi->fifo_depth / 4 * 3;
> > > sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
> > > - (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) |
> > > - (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS));
> > > + (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
> > > reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
> > > @@ -343,11 +351,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
> > > sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
> > > /* Enable the interrupts */
> > > - sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC);
> > > sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
> > > SUN6I_INT_CTL_RF_RDY);
> > > - if (tx_len > sspi->fifo_depth)
> > > - sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ);
> > This would also need to be explained in your commit log.
> What exactly and in what way ?
You should explain, at least:
A) What is the current behaviour
B) Why that is a problem, or what problem does it cause
C) What solution you implement and why you think it's justified
> >
> > > /* Start the transfer */
> > > reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
> > > @@ -376,7 +381,9 @@ out:
> > > static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
> > > {
> > > struct sun6i_spi *sspi = dev_id;
> > > - u32 status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
> > > + u32 status;
> > > +
> > > + status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
> > Why is this change needed?
> A minor one, for readability.
That's arguable, and you should have a single logical change per
patch. So this doesn't belong in this one.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On 04/05/2018 12:19 PM, Maxime Ripard wrote:
> On Wed, Apr 04, 2018 at 02:35:14PM +0300, Sergey Suloev wrote:
>> On 04/04/2018 09:50 AM, Maxime Ripard wrote:
>>> On Tue, Apr 03, 2018 at 06:44:46PM +0300, Sergey Suloev wrote:
>>>> There is no need to handle 3/4 empty interrupt as the maximum
>>>> supported transfer length in PIO mode is equal to FIFO depth,
>>>> i.e. 128 bytes for sun6i and 64 bytes for sun8i SoCs.
>>>>
>>>> Changes in v3:
>>>> 1) Restored processing of 3/4 FIFO full interrupt.
>>>>
>>>> Signed-off-by: Sergey Suloev <[email protected]>
>>>> ---
>>>> drivers/spi/spi-sun6i.c | 41 +++++++++++++++++------------------------
>>>> 1 file changed, 17 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
>>>> index 78acc1f..c09ad10 100644
>>>> --- a/drivers/spi/spi-sun6i.c
>>>> +++ b/drivers/spi/spi-sun6i.c
>>>> @@ -207,7 +207,10 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
>>>> static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
>>>> {
>>>> - return SUN6I_MAX_XFER_SIZE - 1;
>>>> + struct spi_master *master = spi->master;
>>>> + struct sun6i_spi *sspi = spi_master_get_devdata(master);
>>>> +
>>>> + return sspi->fifo_depth;
>>> Doesn't that effectively revert 3288d5cb40c0 ?
>>>
>>> Why do you need to do so?
>> Need what?
> Why do you need to revert that change.
>
>> Change is supposed to restrict max transfer size for PIO mode otherwise it
>> will fail.
>> The maximum transfer length = FIFO depth for PIO mode.
> The point of that patch was precisely to allow to send more data than
> the FIFO. You're breaking that behaviour without any justification,
> and this is not ok.
I am sorry, but you can't. That's a hardware limitation.
>>>> }
>>>> static int sun6i_spi_prepare_message(struct spi_master *master,
>>>> @@ -255,8 +258,14 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>>>> int ret = 0;
>>>> u32 reg;
>>>> - if (tfr->len > SUN6I_MAX_XFER_SIZE)
>>>> - return -EINVAL;
>>>> + /* A zero length transfer never finishes if programmed
>>>> + in the hardware */
>>> Improper comment style here. Please make sure to run checkpatch before
>>> sending your patches.
>> ok
>>>> + if (!tfr->len)
>>>> + return 0;
>>> Can that case even happen?
>> Not sure, just for safety.
>>>> + /* Don't support transfer larger than the FIFO */
>>>> + if (tfr->len > sspi->fifo_depth)
>>>> + return -EMSGSIZE;
>>> You're changing the return type, why?
>> As? a more appropriate one
> The fact that it's more appropriate should be justified.
>
>>>> reinit_completion(&sspi->done);
>>>> sspi->tx_buf = tfr->tx_buf;
>>>> @@ -278,8 +287,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>>>> */
>>>> trig_level = sspi->fifo_depth / 4 * 3;
>>>> sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
>>>> - (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) |
>>>> - (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS));
>>>> + (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS));
>>>> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>>>> @@ -343,11 +351,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>>>> sun6i_spi_fill_fifo(sspi, sspi->fifo_depth);
>>>> /* Enable the interrupts */
>>>> - sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, SUN6I_INT_CTL_TC);
>>>> sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TC |
>>>> SUN6I_INT_CTL_RF_RDY);
>>>> - if (tx_len > sspi->fifo_depth)
>>>> - sun6i_spi_enable_interrupt(sspi, SUN6I_INT_CTL_TF_ERQ);
>>> This would also need to be explained in your commit log.
>> What exactly and in what way ?
> You should explain, at least:
> A) What is the current behaviour
> B) Why that is a problem, or what problem does it cause
> C) What solution you implement and why you think it's justified
>
>>>> /* Start the transfer */
>>>> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>>>> @@ -376,7 +381,9 @@ out:
>>>> static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
>>>> {
>>>> struct sun6i_spi *sspi = dev_id;
>>>> - u32 status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
>>>> + u32 status;
>>>> +
>>>> + status = sun6i_spi_read(sspi, SUN6I_INT_STA_REG);
>>> Why is this change needed?
>> A minor one, for readability.
> That's arguable, and you should have a single logical change per
> patch. So this doesn't belong in this one.
>
> Maxime
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Apr 05, 2018 at 11:19:13AM +0200, Maxime Ripard wrote:
> On Wed, Apr 04, 2018 at 02:35:14PM +0300, Sergey Suloev wrote:
> > What exactly and in what way ?
> You should explain, at least:
> A) What is the current behaviour
> B) Why that is a problem, or what problem does it cause
> C) What solution you implement and why you think it's justified
Right, this is key - the top level problem with most of this patch set
is that it's hard to understand what the changes are intended to do or
why. It's really important that people reading the changes be able to
understand what's going on, especially if technical problems have been
found since that tends to make people look more closely. Part of this
is about splitting the changes out so that each patch does one thing
(which makes it easier to understand them) and part of it is about
explaining those changes clearly.
On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote:
> On 04/05/2018 12:19 PM, Maxime Ripard wrote:
> > The point of that patch was precisely to allow to send more data than
> > the FIFO. You're breaking that behaviour without any justification,
> > and this is not ok.
> I am sorry, but you can't. That's a hardware limitation.
Are you positive about that? Normally you can add things to hardware
FIFOs while they're being drained so so long as you can keep data
flowing in at least as fast as it's being consumed.
On 04/05/2018 04:17 PM, Mark Brown wrote:
> On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote:
>> On 04/05/2018 12:19 PM, Maxime Ripard wrote:
>>> The point of that patch was precisely to allow to send more data than
>>> the FIFO. You're breaking that behaviour without any justification,
>>> and this is not ok.
>> I am sorry, but you can't. That's a hardware limitation.
> Are you positive about that? Normally you can add things to hardware
> FIFOs while they're being drained so so long as you can keep data
> flowing in at least as fast as it's being consumed.
Well, normally yes, but this is not the case with the hardware that I
own. My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a
transfer larger than FIFO then TC interrupt never happens.
On Thu, Apr 05, 2018 at 04:44:16PM +0300, Sergey Suloev wrote:
> On 04/05/2018 04:17 PM, Mark Brown wrote:
> > On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote:
> > > On 04/05/2018 12:19 PM, Maxime Ripard wrote:
> > > > The point of that patch was precisely to allow to send more data than
> > > > the FIFO. You're breaking that behaviour without any justification,
> > > > and this is not ok.
> > > I am sorry, but you can't. That's a hardware limitation.
> > Are you positive about that? Normally you can add things to hardware
> > FIFOs while they're being drained so so long as you can keep data
> > flowing in at least as fast as it's being consumed.
>
> Well, normally yes, but this is not the case with the hardware that I own.
> My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a transfer
> larger than FIFO then TC interrupt never happens.
Because you're not supposed to have a transfer larger than the FIFO,
but to have to setup at first a transfer the size of the FIFO, and
then when it's (or starts to be) depleted, fill it up again.
That's the point of the patch you're reverting, and if it doesn't
work, you should make it work and not simply revert it.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On 04/06/2018 10:34 AM, Maxime Ripard wrote:
> On Thu, Apr 05, 2018 at 04:44:16PM +0300, Sergey Suloev wrote:
>> On 04/05/2018 04:17 PM, Mark Brown wrote:
>>> On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote:
>>>> On 04/05/2018 12:19 PM, Maxime Ripard wrote:
>>>>> The point of that patch was precisely to allow to send more data than
>>>>> the FIFO. You're breaking that behaviour without any justification,
>>>>> and this is not ok.
>>>> I am sorry, but you can't. That's a hardware limitation.
>>> Are you positive about that? Normally you can add things to hardware
>>> FIFOs while they're being drained so so long as you can keep data
>>> flowing in at least as fast as it's being consumed.
>> Well, normally yes, but this is not the case with the hardware that I own.
>> My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a transfer
>> larger than FIFO then TC interrupt never happens.
> Because you're not supposed to have a transfer larger than the FIFO,
> but to have to setup at first a transfer the size of the FIFO, and
> then when it's (or starts to be) depleted, fill it up again.
According to what you said the driver must implement
"transfer_one_message" instead of "transfer_one"
>
> That's the point of the patch you're reverting, and if it doesn't
> work, you should make it work and not simply revert it.
>
> Maxime
>
On 04/06/2018 10:34 AM, Maxime Ripard wrote:
> On Thu, Apr 05, 2018 at 04:44:16PM +0300, Sergey Suloev wrote:
>> On 04/05/2018 04:17 PM, Mark Brown wrote:
>>> On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote:
>>>> On 04/05/2018 12:19 PM, Maxime Ripard wrote:
>>>>> The point of that patch was precisely to allow to send more data than
>>>>> the FIFO. You're breaking that behaviour without any justification,
>>>>> and this is not ok.
>>>> I am sorry, but you can't. That's a hardware limitation.
>>> Are you positive about that? Normally you can add things to hardware
>>> FIFOs while they're being drained so so long as you can keep data
>>> flowing in at least as fast as it's being consumed.
>> Well, normally yes, but this is not the case with the hardware that I own.
>> My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a transfer
>> larger than FIFO then TC interrupt never happens.
> Because you're not supposed to have a transfer larger than the FIFO,
> but to have to setup at first a transfer the size of the FIFO, and
> then when it's (or starts to be) depleted, fill it up again.
According to what you said the driver must implement
"transfer_one_message" instead of "transfer_one",
because the maximum transfer length is 64 bytes (for sun4i) and we
shouldn't allow "transfer_one" handle
more than 64 bytes. Otherwise it breaks the concept.
>
> That's the point of the patch you're reverting, and if it doesn't
> work, you should make it work and not simply revert it.
>
> Maxime
>
On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote:
> On 04/06/2018 10:34 AM, Maxime Ripard wrote:
> > On Thu, Apr 05, 2018 at 04:44:16PM +0300, Sergey Suloev wrote:
> > > On 04/05/2018 04:17 PM, Mark Brown wrote:
> > > > On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote:
> > > > > On 04/05/2018 12:19 PM, Maxime Ripard wrote:
> > > > > > The point of that patch was precisely to allow to send more data than
> > > > > > the FIFO. You're breaking that behaviour without any justification,
> > > > > > and this is not ok.
> > > > > I am sorry, but you can't. That's a hardware limitation.
> > > > Are you positive about that? Normally you can add things to hardware
> > > > FIFOs while they're being drained so so long as you can keep data
> > > > flowing in at least as fast as it's being consumed.
> > > Well, normally yes, but this is not the case with the hardware that I own.
> > > My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a transfer
> > > larger than FIFO then TC interrupt never happens.
> > Because you're not supposed to have a transfer larger than the FIFO,
> > but to have to setup at first a transfer the size of the FIFO, and
> > then when it's (or starts to be) depleted, fill it up again.
>
> According to what you said the driver must implement
> "transfer_one_message" instead of "transfer_one"
I'm not sure what makes you think that I said that.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On 04/09/2018 12:27 PM, Maxime Ripard wrote:
> On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote:
>> On 04/06/2018 10:34 AM, Maxime Ripard wrote:
>>> On Thu, Apr 05, 2018 at 04:44:16PM +0300, Sergey Suloev wrote:
>>>> On 04/05/2018 04:17 PM, Mark Brown wrote:
>>>>> On Thu, Apr 05, 2018 at 12:59:35PM +0300, Sergey Suloev wrote:
>>>>>> On 04/05/2018 12:19 PM, Maxime Ripard wrote:
>>>>>>> The point of that patch was precisely to allow to send more data than
>>>>>>> the FIFO. You're breaking that behaviour without any justification,
>>>>>>> and this is not ok.
>>>>>> I am sorry, but you can't. That's a hardware limitation.
>>>>> Are you positive about that? Normally you can add things to hardware
>>>>> FIFOs while they're being drained so so long as you can keep data
>>>>> flowing in at least as fast as it's being consumed.
>>>> Well, normally yes, but this is not the case with the hardware that I own.
>>>> My a20 (BPiM1+) and a31 (BPiM2) boards behaves differently. With a transfer
>>>> larger than FIFO then TC interrupt never happens.
>>> Because you're not supposed to have a transfer larger than the FIFO,
>>> but to have to setup at first a transfer the size of the FIFO, and
>>> then when it's (or starts to be) depleted, fill it up again.
>> According to what you said the driver must implement
>> "transfer_one_message" instead of "transfer_one"
> I'm not sure what makes you think that I said that.
>
> Maxime
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Because current implementation tries to send more than FIFO-depth of
data in a single call to "transfer_one" which is wrong.
On Mon, Apr 09, 2018 at 01:26:23PM +0300, Sergey Suloev wrote:
> On 04/09/2018 12:27 PM, Maxime Ripard wrote:
> > On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote:
> > > On 04/06/2018 10:34 AM, Maxime Ripard wrote:
> > > According to what you said the driver must implement
> > > "transfer_one_message" instead of "transfer_one"
> > I'm not sure what makes you think that I said that.
> Because current implementation tries to send more than FIFO-depth of data in
> a single call to "transfer_one" which is wrong.
No, that's absolutely not the case. All any of these functions has to
do is transfer whatever they were asked to, how they do it is not at all
important to the framework.
On 04/09/2018 01:50 PM, Mark Brown wrote:
> On Mon, Apr 09, 2018 at 01:26:23PM +0300, Sergey Suloev wrote:
>> On 04/09/2018 12:27 PM, Maxime Ripard wrote:
>>> On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote:
>>>> On 04/06/2018 10:34 AM, Maxime Ripard wrote:
>>>> According to what you said the driver must implement
>>>> "transfer_one_message" instead of "transfer_one"
>>> I'm not sure what makes you think that I said that.
>> Because current implementation tries to send more than FIFO-depth of data in
>> a single call to "transfer_one" which is wrong.
> No, that's absolutely not the case. All any of these functions has to
> do is transfer whatever they were asked to, how they do it is not at all
> important to the framework.
I think you don't fully understand the issue. Let's talk about sun4i
and? sun6i SPI? drivers separately.
sun4i
1)it is correctly declaring max_transfer_size=FIFO depth for PIO mode?
but transfer_one() function doesn't follow the declaration allowing PIO
transfers longer than FIFO depth? by just refilling FIFO using 3/4 FIFO
empty interrupt. I can definitely state here that long transfers WON'T
WORK on real hardware. I tested it and that's why I can say that. But as
soon as sun4i SPI driver? is correctly declaring max_transfer_size then
"smart" clients will work well by limiting a single transfer size to
FIFO depth. I tested it with real hardware, again.
sun6i
2) it allows PIO transfers of any length by declaring max_transfer_size
to a huge number, i.e. you can ONLY make this driver work in PIO mode?
by limiting a single transfer size to FIFO depth (64 or 128 bytes) on
client side? and ignore max_transfer_size? exposed by the driver. Again,
tested with real hardware.
All above doesn't work for DMA mode as there is no such limitation.
I can't clearly explain what is happening in the hardware in PIO mode
but it seems that TC interrupt doesn't arrive in time when refilling
FIFO multiple times takes place and every long transfer will end up with
a timeout error.
On Mon, Apr 09, 2018 at 02:10:40PM +0300, Sergey Suloev wrote:
> On 04/09/2018 01:50 PM, Mark Brown wrote:
> > On Mon, Apr 09, 2018 at 01:26:23PM +0300, Sergey Suloev wrote:
> > > Because current implementation tries to send more than FIFO-depth of data in
> > > a single call to "transfer_one" which is wrong.
> > No, that's absolutely not the case. All any of these functions has to
> > do is transfer whatever they were asked to, how they do it is not at all
> > important to the framework.
> I think you don't fully understand the issue. Let's talk about sun4i and?
> sun6i SPI? drivers separately.
> sun4i
> 1)it is correctly declaring max_transfer_size=FIFO depth for PIO mode? but
> transfer_one() function doesn't follow the declaration allowing PIO
> transfers longer than FIFO depth? by just refilling FIFO using 3/4 FIFO
> empty interrupt. I can definitely state here that long transfers WON'T WORK
> on real hardware. I tested it and that's why I can say that. But as soon as
> sun4i SPI driver? is correctly declaring max_transfer_size then "smart"
> clients will work well by limiting a single transfer size to FIFO depth. I
> tested it with real hardware, again.
None of this is in the slightest bit related to the use of transfer_one()
vs transfer_one_message(). These are all implementation details and
will so far as I can tell apply equally to both APIs.
On Mon, Apr 09, 2018 at 02:10:40PM +0300, Sergey Suloev wrote:
> On 04/09/2018 01:50 PM, Mark Brown wrote:
> > On Mon, Apr 09, 2018 at 01:26:23PM +0300, Sergey Suloev wrote:
> > > On 04/09/2018 12:27 PM, Maxime Ripard wrote:
> > > > On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote:
> > > > > On 04/06/2018 10:34 AM, Maxime Ripard wrote:
> > > > > According to what you said the driver must implement
> > > > > "transfer_one_message" instead of "transfer_one"
> > > > I'm not sure what makes you think that I said that.
> > > Because current implementation tries to send more than FIFO-depth of data in
> > > a single call to "transfer_one" which is wrong.
> > No, that's absolutely not the case. All any of these functions has to
> > do is transfer whatever they were asked to, how they do it is not at all
> > important to the framework.
>
> I think you don't fully understand the issue. Let's talk about sun4i and?
> sun6i SPI? drivers separately.
>
> sun4i
>
> 1)it is correctly declaring max_transfer_size=FIFO depth for PIO mode? but
> transfer_one() function doesn't follow the declaration allowing PIO
> transfers longer than FIFO depth? by just refilling FIFO using 3/4 FIFO
> empty interrupt. I can definitely state here that long transfers WON'T WORK
> on real hardware.
Surely the original author of the patch allowing to do just that
disagrees with you. And it's not about the hardware itself, it's about
how the driver operates as well.
> I tested it and that's why I can say that.
Then it must be fixed, and not silently reverted.
> But as soon as sun4i SPI driver? is correctly declaring
> max_transfer_size then "smart" clients will work well by limiting a
> single transfer size to FIFO depth. I tested it with real hardware,
> again.
This is really not my point. What would prevent you from doing
multiple transfers in that case, and filling the FIFO entirely,
waiting for it to be done, then resuming until you have sent the right
number of bytes?
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
On 04/09/2018 02:36 PM, Maxime Ripard wrote:
> On Mon, Apr 09, 2018 at 02:10:40PM +0300, Sergey Suloev wrote:
>> On 04/09/2018 01:50 PM, Mark Brown wrote:
>>> On Mon, Apr 09, 2018 at 01:26:23PM +0300, Sergey Suloev wrote:
>>>> On 04/09/2018 12:27 PM, Maxime Ripard wrote:
>>>>> On Fri, Apr 06, 2018 at 06:48:23PM +0300, Sergey Suloev wrote:
>>>>>> On 04/06/2018 10:34 AM, Maxime Ripard wrote:
>>>>>> According to what you said the driver must implement
>>>>>> "transfer_one_message" instead of "transfer_one"
>>>>> I'm not sure what makes you think that I said that.
>>>> Because current implementation tries to send more than FIFO-depth of data in
>>>> a single call to "transfer_one" which is wrong.
>>> No, that's absolutely not the case. All any of these functions has to
>>> do is transfer whatever they were asked to, how they do it is not at all
>>> important to the framework.
>> I think you don't fully understand the issue. Let's talk about sun4i and
>> sun6i SPI? drivers separately.
>>
>> sun4i
>>
>> 1)it is correctly declaring max_transfer_size=FIFO depth for PIO mode? but
>> transfer_one() function doesn't follow the declaration allowing PIO
>> transfers longer than FIFO depth? by just refilling FIFO using 3/4 FIFO
>> empty interrupt. I can definitely state here that long transfers WON'T WORK
>> on real hardware.
> Surely the original author of the patch allowing to do just that
> disagrees with you.
I am not getting the point why the driver is declaring the max transfer
length value and not following the rule.
> And it's not about the hardware itself, it's about
> how the driver operates as well.
>
>> I tested it and that's why I can say that.
> Then it must be fixed, and not silently reverted.
>
>> But as soon as sun4i SPI driver? is correctly declaring
>> max_transfer_size then "smart" clients will work well by limiting a
>> single transfer size to FIFO depth. I tested it with real hardware,
>> again.
> This is really not my point. What would prevent you from doing
> multiple transfers in that case, and filling the FIFO entirely,
> waiting for it to be done, then resuming until you have sent the right
> number of bytes?
Because it makes no sense IMHO. I can't see any single point in allowing
long PIO transfers. Can you find at least one ?
I think we should reuse as much SPI core code as possible. The SPI core
can handle an SPI message with multiple transfers,
all we need is to have max_transfer_size = FIFO depth and restrict it in
transfer_one().
>
> Maxime
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Apr 09, 2018 at 02:59:57PM +0300, Sergey Suloev wrote:
> > > But as soon as sun4i SPI driver? is correctly declaring
> > > max_transfer_size then "smart" clients will work well by limiting a
> > > single transfer size to FIFO depth. I tested it with real hardware,
> > > again.
> > This is really not my point. What would prevent you from doing
> > multiple transfers in that case, and filling the FIFO entirely,
> > waiting for it to be done, then resuming until you have sent the right
> > number of bytes?
>
> Because it makes no sense IMHO. I can't see any single point in allowing
> long PIO transfers. Can you find at least one ?
I'm probably going to state the obvious here, but to allow long transfers?
> I think we should reuse as much SPI core code as possible. The SPI
> core can handle an SPI message with multiple transfers, all we need
> is to have max_transfer_size = FIFO depth and restrict it in
> transfer_one().
There's not a single call to the max_transfer_size hook in the SPI
core in 4.16, so that seems a bit too optimistic.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com