From: Lanqing Liu <[email protected]>
In SPI read-only mode, we will always return the writing length,
which is always the power of "bits_per_word", but the length unit
using by users is byte.
Thus this patch fixes the returning length by getting from
read_bufs() function to get the correct length.
Signed-off-by: Lanqing Liu <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
---
drivers/spi/spi-sprd.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/spi-sprd.c b/drivers/spi/spi-sprd.c
index 8daa24e..fa324ce 100644
--- a/drivers/spi/spi-sprd.c
+++ b/drivers/spi/spi-sprd.c
@@ -380,7 +380,7 @@ static int sprd_spi_txrx_bufs(struct spi_device *sdev, struct spi_transfer *t)
{
struct sprd_spi *ss = spi_controller_get_devdata(sdev->controller);
u32 trans_len = ss->trans_len, len;
- int ret, write_size = 0;
+ int ret, write_size = 0, read_size = 0;
while (trans_len) {
len = trans_len > SPRD_SPI_FIFO_SIZE ? SPRD_SPI_FIFO_SIZE :
@@ -416,13 +416,15 @@ static int sprd_spi_txrx_bufs(struct spi_device *sdev, struct spi_transfer *t)
goto complete;
if (ss->trans_mode & SPRD_SPI_RX_MODE)
- ss->read_bufs(ss, len);
+ read_size += ss->read_bufs(ss, len);
trans_len -= len;
}
- ret = write_size;
-
+ if (ss->trans_mode & SPRD_SPI_TX_MODE)
+ ret = write_size;
+ else
+ ret = read_size;
complete:
sprd_spi_enter_idle(ss);
--
1.7.9.5
From: Lanqing Liu <[email protected]>
The SPI irq event will use to complete the SPI work in the SPI DMA mode,
so this patch is a preparation for the following DMA mode support.
Signed-off-by: Lanqing Liu <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
---
drivers/spi/spi-sprd.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/drivers/spi/spi-sprd.c b/drivers/spi/spi-sprd.c
index fa324ce..bfba30b 100644
--- a/drivers/spi/spi-sprd.c
+++ b/drivers/spi/spi-sprd.c
@@ -133,6 +133,7 @@ struct sprd_spi {
void __iomem *base;
struct device *dev;
struct clk *clk;
+ int irq;
u32 src_clk;
u32 hw_mode;
u32 trans_len;
@@ -141,6 +142,7 @@ struct sprd_spi {
u32 hw_speed_hz;
u32 len;
int status;
+ struct completion xfer_completion;
const void *tx_buf;
void *rx_buf;
int (*read_bufs)(struct sprd_spi *ss, u32 len);
@@ -575,6 +577,45 @@ static int sprd_spi_transfer_one(struct spi_controller *sctlr,
return ret;
}
+static irqreturn_t sprd_spi_handle_irq(int irq, void *data)
+{
+ struct sprd_spi *ss = (struct sprd_spi *)data;
+ u32 val = readl_relaxed(ss->base + SPRD_SPI_INT_MASK_STS);
+
+ if (val & SPRD_SPI_MASK_TX_END) {
+ writel_relaxed(SPRD_SPI_TX_END_CLR, ss->base + SPRD_SPI_INT_CLR);
+ if (!(ss->trans_mode & SPRD_SPI_RX_MODE))
+ complete(&ss->xfer_completion);
+ return IRQ_HANDLED;
+ }
+
+ if (val & SPRD_SPI_MASK_RX_END) {
+ writel_relaxed(SPRD_SPI_RX_END_CLR, ss->base + SPRD_SPI_INT_CLR);
+ complete(&ss->xfer_completion);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int sprd_spi_irq_init(struct platform_device *pdev, struct sprd_spi *ss)
+{
+ int ret;
+
+ ss->irq = platform_get_irq(pdev, 0);
+ if (ss->irq < 0) {
+ dev_err(&pdev->dev, "failed to get irq resource\n");
+ return ss->irq;
+ }
+
+ ret = devm_request_irq(&pdev->dev, ss->irq, sprd_spi_handle_irq,
+ 0, pdev->name, ss);
+ if (ret)
+ dev_err(&pdev->dev, "failed to request spi irq %d, ret = %d\n",
+ ss->irq, ret);
+
+ return ret;
+}
+
static int sprd_spi_clk_init(struct platform_device *pdev, struct sprd_spi *ss)
{
struct clk *clk_spi, *clk_parent;
@@ -635,11 +676,16 @@ static int sprd_spi_probe(struct platform_device *pdev)
sctlr->max_speed_hz = min_t(u32, ss->src_clk >> 1,
SPRD_SPI_MAX_SPEED_HZ);
+ init_completion(&ss->xfer_completion);
platform_set_drvdata(pdev, sctlr);
ret = sprd_spi_clk_init(pdev, ss);
if (ret)
goto free_controller;
+ ret = sprd_spi_irq_init(pdev, ss);
+ if (ret)
+ goto free_controller;
+
ret = clk_prepare_enable(ss->clk);
if (ret)
goto free_controller;
--
1.7.9.5
From: Lanqing Liu <[email protected]>
Add DMA mode support for the Spreadtrum SPI controller, and we will enable
SPI interrupt to help to complete the SPI transfer work in DMA mode.
Signed-off-by: Lanqing Liu <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
---
drivers/spi/spi-sprd.c | 291 +++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 288 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi-sprd.c b/drivers/spi/spi-sprd.c
index bfba30b..da93016 100644
--- a/drivers/spi/spi-sprd.c
+++ b/drivers/spi/spi-sprd.c
@@ -2,6 +2,9 @@
// Copyright (C) 2018 Spreadtrum Communications Inc.
#include <linux/clk.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/dma/sprd-dma.h>
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/iopoll.h>
@@ -9,6 +12,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/of_dma.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/spi/spi.h>
@@ -128,9 +132,26 @@
#define SPRD_SPI_DEFAULT_SOURCE 26000000
#define SPRD_SPI_MAX_SPEED_HZ 48000000
#define SPRD_SPI_AUTOSUSPEND_DELAY 100
+#define SPRD_SPI_DMA_STEP 8
+
+enum sprd_spi_dma_channel {
+ SPI_RX,
+ SPI_TX,
+ SPI_MAX,
+};
+
+struct sprd_spi_dma {
+ bool enable;
+ struct dma_chan *dma_chan[SPI_MAX];
+ u32 slave_id[SPI_MAX];
+ enum dma_slave_buswidth width;
+ u32 fragmens_len;
+ u32 rx_len;
+};
struct sprd_spi {
void __iomem *base;
+ phys_addr_t phy_base;
struct device *dev;
struct clk *clk;
int irq;
@@ -142,6 +163,7 @@ struct sprd_spi {
u32 hw_speed_hz;
u32 len;
int status;
+ struct sprd_spi_dma dma;
struct completion xfer_completion;
const void *tx_buf;
void *rx_buf;
@@ -433,6 +455,186 @@ static int sprd_spi_txrx_bufs(struct spi_device *sdev, struct spi_transfer *t)
return ret;
}
+static void sprd_spi_dma_enable(struct sprd_spi *ss, bool enable)
+{
+ u32 val = readl_relaxed(ss->base + SPRD_SPI_CTL2);
+
+ if (enable)
+ val |= SPRD_SPI_DMA_EN;
+ else
+ val &= ~SPRD_SPI_DMA_EN;
+
+ writel_relaxed(val, ss->base + SPRD_SPI_CTL2);
+}
+
+static int sprd_spi_dma_submit(struct dma_chan *dma_chan,
+ struct dma_slave_config *c,
+ struct sg_table *sg,
+ enum dma_transfer_direction dir)
+{
+ struct dma_async_tx_descriptor *desc;
+ dma_cookie_t cookie;
+ unsigned long flags;
+ int ret;
+
+ ret = dmaengine_slave_config(dma_chan, c);
+ if (ret < 0)
+ return ret;
+
+ flags = SPRD_DMA_FLAGS(SPRD_DMA_CHN_MODE_NONE, SPRD_DMA_NO_TRG,
+ SPRD_DMA_FRAG_REQ, SPRD_DMA_TRANS_INT);
+ desc = dmaengine_prep_slave_sg(dma_chan, sg->sgl, sg->nents, dir, flags);
+ if (!desc)
+ return -ENODEV;
+
+ cookie = dmaengine_submit(desc);
+ if (dma_submit_error(cookie))
+ return dma_submit_error(cookie);
+
+ dma_async_issue_pending(dma_chan);
+
+ return 0;
+}
+
+static int sprd_spi_dma_rx_config(struct sprd_spi *ss, struct spi_transfer *t)
+{
+ struct dma_chan *dma_chan = ss->dma.dma_chan[SPI_RX];
+ struct dma_slave_config config = {
+ .src_addr = ss->phy_base,
+ .src_addr_width = ss->dma.width,
+ .dst_addr_width = ss->dma.width,
+ .dst_maxburst = ss->dma.fragmens_len,
+ .slave_id = ss->dma.slave_id[SPI_RX],
+ };
+ int ret;
+
+ ret = sprd_spi_dma_submit(dma_chan, &config, &t->rx_sg, DMA_DEV_TO_MEM);
+ if (ret)
+ return ret;
+
+ return ss->dma.rx_len;
+}
+
+static int sprd_spi_dma_tx_config(struct sprd_spi *ss, struct spi_transfer *t)
+{
+ struct dma_chan *dma_chan = ss->dma.dma_chan[SPI_TX];
+ struct dma_slave_config config = {
+ .dst_addr = ss->phy_base,
+ .src_addr_width = ss->dma.width,
+ .dst_addr_width = ss->dma.width,
+ .src_maxburst = ss->dma.fragmens_len,
+ .slave_id = ss->dma.slave_id[SPI_TX],
+ };
+ int ret;
+
+ ret = sprd_spi_dma_submit(dma_chan, &config, &t->tx_sg, DMA_MEM_TO_DEV);
+ if (ret)
+ return ret;
+
+ return t->len;
+}
+
+static int sprd_spi_dma_request(struct sprd_spi *ss)
+{
+ ss->dma.dma_chan[SPI_RX] = dma_request_chan(ss->dev, "rx_chn");
+ if (IS_ERR_OR_NULL(ss->dma.dma_chan[SPI_RX])) {
+ if (PTR_ERR(ss->dma.dma_chan[SPI_RX]) == -EPROBE_DEFER)
+ return PTR_ERR(ss->dma.dma_chan[SPI_RX]);
+
+ dev_err(ss->dev, "request RX DMA channel failed!\n");
+ return PTR_ERR(ss->dma.dma_chan[SPI_RX]);
+ }
+
+ ss->dma.dma_chan[SPI_TX] = dma_request_chan(ss->dev, "tx_chn");
+ if (IS_ERR_OR_NULL(ss->dma.dma_chan[SPI_TX])) {
+ if (PTR_ERR(ss->dma.dma_chan[SPI_TX]) == -EPROBE_DEFER)
+ return PTR_ERR(ss->dma.dma_chan[SPI_TX]);
+
+ dev_err(ss->dev, "request TX DMA channel failed!\n");
+ dma_release_channel(ss->dma.dma_chan[SPI_RX]);
+ return PTR_ERR(ss->dma.dma_chan[SPI_TX]);
+ }
+
+ return 0;
+}
+
+static void sprd_spi_dma_release(struct sprd_spi *ss)
+{
+ dma_release_channel(ss->dma.dma_chan[SPI_RX]);
+ dma_release_channel(ss->dma.dma_chan[SPI_TX]);
+}
+
+static int sprd_spi_dma_txrx_bufs(struct spi_device *sdev,
+ struct spi_transfer *t)
+{
+ struct sprd_spi *ss = spi_master_get_devdata(sdev->master);
+ u32 trans_len = ss->trans_len;
+ int ret, write_size = 0;
+
+ reinit_completion(&ss->xfer_completion);
+ if (ss->trans_mode & SPRD_SPI_TX_MODE) {
+ write_size = sprd_spi_dma_tx_config(ss, t);
+ sprd_spi_set_tx_length(ss, trans_len);
+
+ /*
+ * For our 3 wires mode or dual TX line mode, we need
+ * to request the controller to transfer.
+ */
+ if (ss->hw_mode & SPI_3WIRE || ss->hw_mode & SPI_TX_DUAL)
+ sprd_spi_tx_req(ss);
+ } else {
+ sprd_spi_set_rx_length(ss, trans_len);
+
+ /*
+ * For our 3 wires mode or dual TX line mode, we need
+ * to request the controller to read.
+ */
+ if (ss->hw_mode & SPI_3WIRE || ss->hw_mode & SPI_TX_DUAL)
+ sprd_spi_rx_req(ss);
+ else
+ write_size = ss->write_bufs(ss, trans_len);
+ }
+
+ if (write_size < 0) {
+ ret = write_size;
+ dev_err(ss->dev, "failed to write, ret = %d\n", ret);
+ goto trans_complete;
+ }
+
+ if (ss->trans_mode & SPRD_SPI_RX_MODE) {
+ /*
+ * Set up the DMA receive data length, which must be an
+ * integral multiple of fragment length. But when the length
+ * of received data is less than fragment length, DMA can be
+ * configured to receive data according to the actual length
+ * of received data.
+ */
+ ss->dma.rx_len = t->len > ss->dma.fragmens_len ?
+ (t->len - t->len % ss->dma.fragmens_len) :
+ t->len;
+ ret = sprd_spi_dma_rx_config(ss, t);
+ if (ret < 0) {
+ dev_err(&sdev->dev,
+ "failed to configure rx DMA, ret = %d\n", ret);
+ goto trans_complete;
+ }
+ }
+
+ sprd_spi_dma_enable(ss, true);
+ wait_for_completion(&(ss->xfer_completion));
+
+ if (ss->trans_mode & SPRD_SPI_TX_MODE)
+ ret = write_size;
+ else
+ ret = ss->dma.rx_len;
+
+trans_complete:
+ sprd_spi_dma_enable(ss, false);
+ sprd_spi_enter_idle(ss);
+
+ return ret;
+}
+
static void sprd_spi_set_speed(struct sprd_spi *ss, u32 speed_hz)
{
/*
@@ -488,6 +690,18 @@ static void sprd_spi_init_hw(struct sprd_spi *ss, struct spi_transfer *t)
val &= ~SPRD_SPI_DATA_LINE2_EN;
writel_relaxed(val, ss->base + SPRD_SPI_CTL7);
+
+ if (ss->dma.enable) {
+ /* Clear interrupt status before enabling interrupt. */
+ writel_relaxed(SPRD_SPI_TX_END_CLR | SPRD_SPI_RX_END_CLR,
+ ss->base + SPRD_SPI_INT_CLR);
+
+ /* Enable SPI interrupt only in DMA mode. */
+ val = readl_relaxed(ss->base + SPRD_SPI_INT_EN);
+ writel_relaxed(val | SPRD_SPI_TX_END_INT_EN |
+ SPRD_SPI_RX_END_INT_EN,
+ ss->base + SPRD_SPI_INT_EN);
+ }
}
static int sprd_spi_setup_transfer(struct spi_device *sdev,
@@ -518,16 +732,22 @@ static int sprd_spi_setup_transfer(struct spi_device *sdev,
ss->trans_len = t->len;
ss->read_bufs = sprd_spi_read_bufs_u8;
ss->write_bufs = sprd_spi_write_bufs_u8;
+ ss->dma.width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ ss->dma.fragmens_len = SPRD_SPI_DMA_STEP;
break;
case 16:
ss->trans_len = t->len >> 1;
ss->read_bufs = sprd_spi_read_bufs_u16;
ss->write_bufs = sprd_spi_write_bufs_u16;
+ ss->dma.width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+ ss->dma.fragmens_len = SPRD_SPI_DMA_STEP << 1;
break;
case 32:
ss->trans_len = t->len >> 2;
ss->read_bufs = sprd_spi_read_bufs_u32;
ss->write_bufs = sprd_spi_write_bufs_u32;
+ ss->dma.width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+ ss->dma.fragmens_len = SPRD_SPI_DMA_STEP << 2;
break;
default:
return -EINVAL;
@@ -559,13 +779,18 @@ static int sprd_spi_transfer_one(struct spi_controller *sctlr,
struct spi_device *sdev,
struct spi_transfer *t)
{
+ struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
int ret;
ret = sprd_spi_setup_transfer(sdev, t);
if (ret)
goto setup_err;
- ret = sprd_spi_txrx_bufs(sdev, t);
+ if (ss->dma.enable)
+ ret = sprd_spi_dma_txrx_bufs(sdev, t);
+ else
+ ret = sprd_spi_txrx_bufs(sdev, t);
+
if (ret == t->len)
ret = 0;
else if (ret >= 0)
@@ -591,6 +816,11 @@ static irqreturn_t sprd_spi_handle_irq(int irq, void *data)
if (val & SPRD_SPI_MASK_RX_END) {
writel_relaxed(SPRD_SPI_RX_END_CLR, ss->base + SPRD_SPI_INT_CLR);
+ if (ss->dma.rx_len < ss->len) {
+ ss->rx_buf += ss->dma.rx_len;
+ ss->dma.rx_len +=
+ ss->read_bufs(ss, ss->len - ss->dma.rx_len);
+ }
complete(&ss->xfer_completion);
}
@@ -646,6 +876,43 @@ static int sprd_spi_clk_init(struct platform_device *pdev, struct sprd_spi *ss)
return 0;
}
+static bool sprd_spi_can_dma(struct spi_controller *sctlr,
+ struct spi_device *spi, struct spi_transfer *t)
+{
+ struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
+
+ return ss->dma.enable;
+}
+
+static int sprd_spi_dma_init(struct platform_device *pdev, struct sprd_spi *ss)
+{
+ struct device_node *np = pdev->dev.of_node;
+ int ret;
+
+ ret = sprd_spi_dma_request(ss);
+ if (ret) {
+ if (ret == -EPROBE_DEFER)
+ return ret;
+
+ dev_warn(&pdev->dev,
+ "failed to request dma, enter no dma mode, ret = %d\n",
+ ret);
+
+ return 0;
+ }
+
+ if (of_property_read_u32_array(np, "sprd,dma-slave-ids",
+ ss->dma.slave_id, SPI_MAX)) {
+ dev_warn(&pdev->dev,
+ "failed to request dma slave id, enter no dma mode\n");
+ return 0;
+ }
+
+ ss->dma.enable = true;
+
+ return 0;
+}
+
static int sprd_spi_probe(struct platform_device *pdev)
{
struct spi_controller *sctlr;
@@ -666,12 +933,14 @@ static int sprd_spi_probe(struct platform_device *pdev)
goto free_controller;
}
+ ss->phy_base = res->start;
ss->dev = &pdev->dev;
sctlr->dev.of_node = pdev->dev.of_node;
sctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE | SPI_TX_DUAL;
sctlr->bus_num = pdev->id;
sctlr->set_cs = sprd_spi_chipselect;
sctlr->transfer_one = sprd_spi_transfer_one;
+ sctlr->can_dma = sprd_spi_can_dma;
sctlr->auto_runtime_pm = true;
sctlr->max_speed_hz = min_t(u32, ss->src_clk >> 1,
SPRD_SPI_MAX_SPEED_HZ);
@@ -686,10 +955,14 @@ static int sprd_spi_probe(struct platform_device *pdev)
if (ret)
goto free_controller;
- ret = clk_prepare_enable(ss->clk);
+ ret = sprd_spi_dma_init(pdev, ss);
if (ret)
goto free_controller;
+ ret = clk_prepare_enable(ss->clk);
+ if (ret)
+ goto release_dma;
+
ret = pm_runtime_set_active(&pdev->dev);
if (ret < 0)
goto disable_clk;
@@ -718,6 +991,8 @@ static int sprd_spi_probe(struct platform_device *pdev)
pm_runtime_disable(&pdev->dev);
disable_clk:
clk_disable_unprepare(ss->clk);
+release_dma:
+ sprd_spi_dma_release(ss);
free_controller:
spi_controller_put(sctlr);
@@ -748,6 +1023,9 @@ static int __maybe_unused sprd_spi_runtime_suspend(struct device *dev)
struct spi_controller *sctlr = dev_get_drvdata(dev);
struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
+ if (ss->dma.enable)
+ sprd_spi_dma_release(ss);
+
clk_disable_unprepare(ss->clk);
return 0;
@@ -763,7 +1041,14 @@ static int __maybe_unused sprd_spi_runtime_resume(struct device *dev)
if (ret)
return ret;
- return 0;
+ if (!ss->dma.enable)
+ return 0;
+
+ ret = sprd_spi_dma_request(ss);
+ if (ret)
+ clk_disable_unprepare(ss->clk);
+
+ return ret;
}
static const struct dev_pm_ops sprd_spi_pm_ops = {
--
1.7.9.5
On Tue, Jan 15, 2019 at 09:46:53PM +0800, Baolin Wang wrote:
> From: Lanqing Liu <[email protected]>
>
> Add DMA mode support for the Spreadtrum SPI controller, and we will enable
> SPI interrupt to help to complete the SPI transfer work in DMA mode.
Again this looks good, one thing to check though:
> +static bool sprd_spi_can_dma(struct spi_controller *sctlr,
> + struct spi_device *spi, struct spi_transfer *t)
> +{
> + struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
> +
> + return ss->dma.enable;
> +}
Is it always a benefit to use DMA? One reason this is a function rather
than just a property is that even if the device can DMA any size of
transfer often for short transfers the performance is better if you use
PIO since the overhead of setting up the DMA controller, waiting and
then taking the interrupt when it finishes ends up being more than the
cost of just reading and writing directly to the FIFOs for PIO.
From: Lanqing Liu <[email protected]>
Add the DMA properties for the SPI dma mode.
Signed-off-by: Lanqing Liu <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
---
Documentation/devicetree/bindings/spi/spi-sprd.txt | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/spi/spi-sprd.txt b/Documentation/devicetree/bindings/spi/spi-sprd.txt
index bad211a..01ef53f 100644
--- a/Documentation/devicetree/bindings/spi/spi-sprd.txt
+++ b/Documentation/devicetree/bindings/spi/spi-sprd.txt
@@ -14,6 +14,12 @@ Required properties:
address on the SPI bus. Should be set to 1.
- #size-cells: Should be set to 0.
+Optional properties:
+dma-names: Should contain names of the SPI used DMA channel.
+dmas: Should contain DMA channels which the SPI used sorted in the
+ same order as the dma-names property.
+sprd,dma-slave-ids: Should contain the DMA number that the SPI hardware required.
+
Example:
spi0: spi@70a00000{
compatible = "sprd,sc9860-spi";
@@ -21,6 +27,9 @@ spi0: spi@70a00000{
interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
clock-names = "spi", "source","enable";
clocks = <&clk_spi0>, <&ext_26m>, <&clk_ap_apb_gates 5>;
+ dma-names = "rx_chn", "tx_chn";
+ dmas = <&apdma 11>, <&apdma 12>;
+ sprd,dma-slave-ids = <11 12>;
#address-cells = <1>;
#size-cells = <0>;
};
--
1.7.9.5
On Tue, Jan 15, 2019 at 09:46:51PM +0800, Baolin Wang wrote:
This looks good, just one small issue and a thing to check:
> +static irqreturn_t sprd_spi_handle_irq(int irq, void *data)
> +{
> + struct sprd_spi *ss = (struct sprd_spi *)data;
> + u32 val = readl_relaxed(ss->base + SPRD_SPI_INT_MASK_STS);
> +
> + if (val & SPRD_SPI_MASK_TX_END) {
> + writel_relaxed(SPRD_SPI_TX_END_CLR, ss->base + SPRD_SPI_INT_CLR);
> + if (!(ss->trans_mode & SPRD_SPI_RX_MODE))
> + complete(&ss->xfer_completion);
> + return IRQ_HANDLED;
> + }
> +
> + if (val & SPRD_SPI_MASK_RX_END) {
> + writel_relaxed(SPRD_SPI_RX_END_CLR, ss->base + SPRD_SPI_INT_CLR);
> + complete(&ss->xfer_completion);
> + }
> +
> + return IRQ_HANDLED;
> +}
This will return IRQ_HANDLED no matter if there was an interrupt
actually handled. That means that if something goes wrong due to some
bug or a hardware change (eg, a new version of the IP) and there's
another interrupt fired we won't clear it and the interrupt core won't
be able to detect that it's a spurious interrupt and use its own error
handling. It's better to return IRQ_NONE in that case.
> + ret = devm_request_irq(&pdev->dev, ss->irq, sprd_spi_handle_irq,
> + 0, pdev->name, ss);
> + if (ret)
> + dev_err(&pdev->dev, "failed to request spi irq %d, ret = %d\n",
> + ss->irq, ret);
Are you sure it's safe to use devm_request_irq(), especially when
unloading the driver? Using it means that we will only disable the
interrupt after the driver's remove function has finished so there's a
danger of an interrupt firing when some of the resources the hander has
used are still in use. I didn't spot any issues, just something to
check especially with the later patches building on top of this.
The patch
spi: sprd: Fix the error data length in SPI read-only mode
has been applied to the spi tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
From 63f5ffc46d4f53b309a93b83420b2e4bd0da5aba Mon Sep 17 00:00:00 2001
From: Lanqing Liu <[email protected]>
Date: Tue, 15 Jan 2019 21:46:50 +0800
Subject: [PATCH] spi: sprd: Fix the error data length in SPI read-only mode
In SPI read-only mode, we will always return the writing length,
which is always the power of "bits_per_word", but the length unit
using by users is byte.
Thus this patch fixes the returning length by getting from
read_bufs() function to get the correct length.
Signed-off-by: Lanqing Liu <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
drivers/spi/spi-sprd.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/spi-sprd.c b/drivers/spi/spi-sprd.c
index 8daa24eec624..fa324ce194b2 100644
--- a/drivers/spi/spi-sprd.c
+++ b/drivers/spi/spi-sprd.c
@@ -380,7 +380,7 @@ static int sprd_spi_txrx_bufs(struct spi_device *sdev, struct spi_transfer *t)
{
struct sprd_spi *ss = spi_controller_get_devdata(sdev->controller);
u32 trans_len = ss->trans_len, len;
- int ret, write_size = 0;
+ int ret, write_size = 0, read_size = 0;
while (trans_len) {
len = trans_len > SPRD_SPI_FIFO_SIZE ? SPRD_SPI_FIFO_SIZE :
@@ -416,13 +416,15 @@ static int sprd_spi_txrx_bufs(struct spi_device *sdev, struct spi_transfer *t)
goto complete;
if (ss->trans_mode & SPRD_SPI_RX_MODE)
- ss->read_bufs(ss, len);
+ read_size += ss->read_bufs(ss, len);
trans_len -= len;
}
- ret = write_size;
-
+ if (ss->trans_mode & SPRD_SPI_TX_MODE)
+ ret = write_size;
+ else
+ ret = read_size;
complete:
sprd_spi_enter_idle(ss);
--
2.20.1
Hi Mark,
On Tue, 15 Jan 2019 at 22:25, Mark Brown <[email protected]> wrote:
>
> On Tue, Jan 15, 2019 at 09:46:51PM +0800, Baolin Wang wrote:
>
> This looks good, just one small issue and a thing to check:
>
> > +static irqreturn_t sprd_spi_handle_irq(int irq, void *data)
> > +{
> > + struct sprd_spi *ss = (struct sprd_spi *)data;
> > + u32 val = readl_relaxed(ss->base + SPRD_SPI_INT_MASK_STS);
> > +
> > + if (val & SPRD_SPI_MASK_TX_END) {
> > + writel_relaxed(SPRD_SPI_TX_END_CLR, ss->base + SPRD_SPI_INT_CLR);
> > + if (!(ss->trans_mode & SPRD_SPI_RX_MODE))
> > + complete(&ss->xfer_completion);
> > + return IRQ_HANDLED;
> > + }
> > +
> > + if (val & SPRD_SPI_MASK_RX_END) {
> > + writel_relaxed(SPRD_SPI_RX_END_CLR, ss->base + SPRD_SPI_INT_CLR);
> > + complete(&ss->xfer_completion);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
>
> This will return IRQ_HANDLED no matter if there was an interrupt
> actually handled. That means that if something goes wrong due to some
> bug or a hardware change (eg, a new version of the IP) and there's
> another interrupt fired we won't clear it and the interrupt core won't
> be able to detect that it's a spurious interrupt and use its own error
> handling. It's better to return IRQ_NONE in that case.
Yes, you are correct and will fix in next version.
> > + ret = devm_request_irq(&pdev->dev, ss->irq, sprd_spi_handle_irq,
> > + 0, pdev->name, ss);
> > + if (ret)
> > + dev_err(&pdev->dev, "failed to request spi irq %d, ret = %d\n",
> > + ss->irq, ret);
>
> Are you sure it's safe to use devm_request_irq(), especially when
> unloading the driver? Using it means that we will only disable the
> interrupt after the driver's remove function has finished so there's a
> danger of an interrupt firing when some of the resources the hander has
> used are still in use. I didn't spot any issues, just something to
> check especially with the later patches building on top of this.
Yes, we missed this issue, thanks for pointing out this. After some
discussion with Lanqing, we think we need call the
spi_controller_suspend() manually in remove function to avoid this
issue.
Thanks for your comments.
--
Baolin Wang
Best Regards
Hi Mark,
On Tue, 15 Jan 2019 at 22:30, Mark Brown <[email protected]> wrote:
>
> On Tue, Jan 15, 2019 at 09:46:53PM +0800, Baolin Wang wrote:
> > From: Lanqing Liu <[email protected]>
> >
> > Add DMA mode support for the Spreadtrum SPI controller, and we will enable
> > SPI interrupt to help to complete the SPI transfer work in DMA mode.
>
> Again this looks good, one thing to check though:
>
> > +static bool sprd_spi_can_dma(struct spi_controller *sctlr,
> > + struct spi_device *spi, struct spi_transfer *t)
> > +{
> > + struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
> > +
> > + return ss->dma.enable;
> > +}
>
> Is it always a benefit to use DMA? One reason this is a function rather
> than just a property is that even if the device can DMA any size of
> transfer often for short transfers the performance is better if you use
> PIO since the overhead of setting up the DMA controller, waiting and
> then taking the interrupt when it finishes ends up being more than the
> cost of just reading and writing directly to the FIFOs for PIO.
Firstly we want to simplify the transfer, once we enable the DMA, we
always want to use DMA mode. But I think you are reasonable, we will
optimize the can_dma() ops for short transfers to improve the
performance. Thanks for your comments.
--
Baolin Wang
Best Regards
On Tue, Jan 15, 2019 at 7:47 AM Baolin Wang <[email protected]> wrote:
>
> From: Lanqing Liu <[email protected]>
The email address should be updated with unisoc.com.
> Add the DMA properties for the SPI dma mode.
>
> Signed-off-by: Lanqing Liu <[email protected]>
> Signed-off-by: Baolin Wang <[email protected]>
> ---
> Documentation/devicetree/bindings/spi/spi-sprd.txt | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/spi/spi-sprd.txt b/Documentation/devicetree/bindings/spi/spi-sprd.txt
> index bad211a..01ef53f 100644
> --- a/Documentation/devicetree/bindings/spi/spi-sprd.txt
> +++ b/Documentation/devicetree/bindings/spi/spi-sprd.txt
> @@ -14,6 +14,12 @@ Required properties:
> address on the SPI bus. Should be set to 1.
> - #size-cells: Should be set to 0.
>
> +Optional properties:
> +dma-names: Should contain names of the SPI used DMA channel.
> +dmas: Should contain DMA channels which the SPI used sorted in the
> + same order as the dma-names property.
> +sprd,dma-slave-ids: Should contain the DMA number that the SPI hardware required.
> +
> Example:
> spi0: spi@70a00000{
> compatible = "sprd,sc9860-spi";
> @@ -21,6 +27,9 @@ spi0: spi@70a00000{
> interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> clock-names = "spi", "source","enable";
> clocks = <&clk_spi0>, <&ext_26m>, <&clk_ap_apb_gates 5>;
> + dma-names = "rx_chn", "tx_chn";
> + dmas = <&apdma 11>, <&apdma 12>;
> + sprd,dma-slave-ids = <11 12>;
When would this be different values from what's in 'dmas'?
> #address-cells = <1>;
> #size-cells = <0>;
> };
> --
> 1.7.9.5
>
On Mon, 21 Jan 2019 at 21:53, Rob Herring <[email protected]> wrote:
>
> On Tue, Jan 15, 2019 at 7:47 AM Baolin Wang <[email protected]> wrote:
> >
> > From: Lanqing Liu <[email protected]>
>
> The email address should be updated with unisoc.com.
Sure.
>
> > Add the DMA properties for the SPI dma mode.
> >
> > Signed-off-by: Lanqing Liu <[email protected]>
> > Signed-off-by: Baolin Wang <[email protected]>
> > ---
> > Documentation/devicetree/bindings/spi/spi-sprd.txt | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/spi/spi-sprd.txt b/Documentation/devicetree/bindings/spi/spi-sprd.txt
> > index bad211a..01ef53f 100644
> > --- a/Documentation/devicetree/bindings/spi/spi-sprd.txt
> > +++ b/Documentation/devicetree/bindings/spi/spi-sprd.txt
> > @@ -14,6 +14,12 @@ Required properties:
> > address on the SPI bus. Should be set to 1.
> > - #size-cells: Should be set to 0.
> >
> > +Optional properties:
> > +dma-names: Should contain names of the SPI used DMA channel.
> > +dmas: Should contain DMA channels which the SPI used sorted in the
> > + same order as the dma-names property.
> > +sprd,dma-slave-ids: Should contain the DMA number that the SPI hardware required.
> > +
> > Example:
> > spi0: spi@70a00000{
> > compatible = "sprd,sc9860-spi";
> > @@ -21,6 +27,9 @@ spi0: spi@70a00000{
> > interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> > clock-names = "spi", "source","enable";
> > clocks = <&clk_spi0>, <&ext_26m>, <&clk_ap_apb_gates 5>;
> > + dma-names = "rx_chn", "tx_chn";
> > + dmas = <&apdma 11>, <&apdma 12>;
> > + sprd,dma-slave-ids = <11 12>;
>
> When would this be different values from what's in 'dmas'?
Slave id is not always same with the DMA channel number in 'dmas', and
it is just coincident for SPI driver. Moreover for different SoC , the
slave ids for DMA engine consumers are not same. So we need one
property to specify the slave id for the consumers to trigger DMA
transfer.
--
Baolin Wang
Best Regards
Hi Baolin,
On Tue, Jan 22, 2019 at 3:23 AM Baolin Wang <[email protected]> wrote:
> On Mon, 21 Jan 2019 at 21:53, Rob Herring <[email protected]> wrote:
> > On Tue, Jan 15, 2019 at 7:47 AM Baolin Wang <[email protected]> wrote:
> > > From: Lanqing Liu <[email protected]>
> >
> > The email address should be updated with unisoc.com.
>
> Sure.
>
> >
> > > Add the DMA properties for the SPI dma mode.
> > >
> > > Signed-off-by: Lanqing Liu <[email protected]>
> > > Signed-off-by: Baolin Wang <[email protected]>
> > > ---
> > > Documentation/devicetree/bindings/spi/spi-sprd.txt | 9 +++++++++
> > > 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/spi/spi-sprd.txt b/Documentation/devicetree/bindings/spi/spi-sprd.txt
> > > index bad211a..01ef53f 100644
> > > --- a/Documentation/devicetree/bindings/spi/spi-sprd.txt
> > > +++ b/Documentation/devicetree/bindings/spi/spi-sprd.txt
> > > @@ -14,6 +14,12 @@ Required properties:
> > > address on the SPI bus. Should be set to 1.
> > > - #size-cells: Should be set to 0.
> > >
> > > +Optional properties:
> > > +dma-names: Should contain names of the SPI used DMA channel.
> > > +dmas: Should contain DMA channels which the SPI used sorted in the
> > > + same order as the dma-names property.
> > > +sprd,dma-slave-ids: Should contain the DMA number that the SPI hardware required.
> > > +
> > > Example:
> > > spi0: spi@70a00000{
> > > compatible = "sprd,sc9860-spi";
> > > @@ -21,6 +27,9 @@ spi0: spi@70a00000{
> > > interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> > > clock-names = "spi", "source","enable";
> > > clocks = <&clk_spi0>, <&ext_26m>, <&clk_ap_apb_gates 5>;
> > > + dma-names = "rx_chn", "tx_chn";
> > > + dmas = <&apdma 11>, <&apdma 12>;
> > > + sprd,dma-slave-ids = <11 12>;
> >
> > When would this be different values from what's in 'dmas'?
>
> Slave id is not always same with the DMA channel number in 'dmas', and
> it is just coincident for SPI driver. Moreover for different SoC , the
> slave ids for DMA engine consumers are not same. So we need one
> property to specify the slave id for the consumers to trigger DMA
> transfer.
Perhaps the DMA controller should use #dma-cells = <2>, so you can specify
the second value in the dmas property?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert,
On Tue, 22 Jan 2019 at 16:11, Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Baolin,
>
> On Tue, Jan 22, 2019 at 3:23 AM Baolin Wang <[email protected]> wrote:
> > On Mon, 21 Jan 2019 at 21:53, Rob Herring <[email protected]> wrote:
> > > On Tue, Jan 15, 2019 at 7:47 AM Baolin Wang <[email protected]> wrote:
> > > > From: Lanqing Liu <[email protected]>
> > >
> > > The email address should be updated with unisoc.com.
> >
> > Sure.
> >
> > >
> > > > Add the DMA properties for the SPI dma mode.
> > > >
> > > > Signed-off-by: Lanqing Liu <[email protected]>
> > > > Signed-off-by: Baolin Wang <[email protected]>
> > > > ---
> > > > Documentation/devicetree/bindings/spi/spi-sprd.txt | 9 +++++++++
> > > > 1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/spi/spi-sprd.txt b/Documentation/devicetree/bindings/spi/spi-sprd.txt
> > > > index bad211a..01ef53f 100644
> > > > --- a/Documentation/devicetree/bindings/spi/spi-sprd.txt
> > > > +++ b/Documentation/devicetree/bindings/spi/spi-sprd.txt
> > > > @@ -14,6 +14,12 @@ Required properties:
> > > > address on the SPI bus. Should be set to 1.
> > > > - #size-cells: Should be set to 0.
> > > >
> > > > +Optional properties:
> > > > +dma-names: Should contain names of the SPI used DMA channel.
> > > > +dmas: Should contain DMA channels which the SPI used sorted in the
> > > > + same order as the dma-names property.
> > > > +sprd,dma-slave-ids: Should contain the DMA number that the SPI hardware required.
> > > > +
> > > > Example:
> > > > spi0: spi@70a00000{
> > > > compatible = "sprd,sc9860-spi";
> > > > @@ -21,6 +27,9 @@ spi0: spi@70a00000{
> > > > interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> > > > clock-names = "spi", "source","enable";
> > > > clocks = <&clk_spi0>, <&ext_26m>, <&clk_ap_apb_gates 5>;
> > > > + dma-names = "rx_chn", "tx_chn";
> > > > + dmas = <&apdma 11>, <&apdma 12>;
> > > > + sprd,dma-slave-ids = <11 12>;
> > >
> > > When would this be different values from what's in 'dmas'?
> >
> > Slave id is not always same with the DMA channel number in 'dmas', and
> > it is just coincident for SPI driver. Moreover for different SoC , the
> > slave ids for DMA engine consumers are not same. So we need one
> > property to specify the slave id for the consumers to trigger DMA
> > transfer.
>
> Perhaps the DMA controller should use #dma-cells = <2>, so you can specify
> the second value in the dmas property?
Yes, that's a good point. I will try to change our DMA driver. Thanks.
--
Baolin Wang
Best Regards