From: Maxime Ripard <maxime@cerno.tech> To: Alexander Kochetkov <al.kochet@gmail.com> Cc: Mark Brown <broonie@kernel.org>, Chen-Yu Tsai <wens@csie.org>, linux-spi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] spi: spi-sun6i: implement DMA-based transfer mode Date: Mon, 19 Oct 2020 10:21:29 +0200 [thread overview] Message-ID: <20201019082129.myxpxla5xwoqwldo@gilmour.lan> (raw) In-Reply-To: <20201015154740.20825-1-al.kochet@gmail.com> [-- Attachment #1: Type: text/plain, Size: 7028 bytes --] Hi! On Thu, Oct 15, 2020 at 06:47:40PM +0300, Alexander Kochetkov wrote: > DMA-based transfer will be enabled if data length is larger than FIFO size > (64 bytes for A64). This greatly reduce number of interrupts for > transferring data. > > For smaller data size PIO mode will be used. In PIO mode whole buffer will > be loaded into FIFO. > > If driver failed to request DMA channels then it fallback for PIO mode. > > Tested on SOPINE (https://www.pine64.org/sopine/) > > Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com> Thanks for working on this, it's been a bit overdue > --- > drivers/spi/spi-sun6i.c | 171 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 159 insertions(+), 12 deletions(-) > > diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c > index 19238e1b76b4..38e5b8af7da6 100644 > --- a/drivers/spi/spi-sun6i.c > +++ b/drivers/spi/spi-sun6i.c > @@ -18,6 +18,7 @@ > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > #include <linux/reset.h> > +#include <linux/dmaengine.h> > > #include <linux/spi/spi.h> > > @@ -52,10 +53,12 @@ > > #define SUN6I_FIFO_CTL_REG 0x18 > #define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_MASK 0xff > +#define SUN6I_FIFO_CTL_RF_DRQ_EN BIT(8) > #define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS 0 > #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_DRQ_EN BIT(24) > #define SUN6I_FIFO_CTL_TF_RST BIT(31) > > #define SUN6I_FIFO_STA_REG 0x1c > @@ -83,6 +86,8 @@ > struct sun6i_spi { > struct spi_master *master; > void __iomem *base_addr; > + dma_addr_t dma_addr_rx; > + dma_addr_t dma_addr_tx; > struct clk *hclk; > struct clk *mclk; > struct reset_control *rstc; > @@ -92,6 +97,7 @@ struct sun6i_spi { > const u8 *tx_buf; > u8 *rx_buf; > int len; > + bool use_dma; > unsigned long fifo_depth; > }; > > @@ -182,6 +188,66 @@ static size_t sun6i_spi_max_transfer_size(struct spi_device *spi) > return SUN6I_MAX_XFER_SIZE - 1; > } > > +static int sun6i_spi_prepare_dma(struct sun6i_spi *sspi, > + struct spi_transfer *tfr) > +{ > + struct dma_async_tx_descriptor *rxdesc, *txdesc; > + struct spi_master *master = sspi->master; > + > + rxdesc = NULL; > + if (tfr->rx_buf) { > + struct dma_slave_config rxconf = { > + .direction = DMA_DEV_TO_MEM, > + .src_addr = sspi->dma_addr_rx, > + .src_addr_width = 1, > + .src_maxburst = 1, > + }; That doesn't really look optimal, the controller seems to be able to read / write 32 bits at a time from its FIFO and we probably can increase the burst length too? > + > + dmaengine_slave_config(master->dma_rx, &rxconf); > + > + rxdesc = dmaengine_prep_slave_sg( > + master->dma_rx, > + tfr->rx_sg.sgl, tfr->rx_sg.nents, > + DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT); checkpatch --strict complains about this one (your line shouldn't end with a parenthesis). > + if (!rxdesc) > + return -EINVAL; > + } > + > + txdesc = NULL; > + if (tfr->tx_buf) { > + struct dma_slave_config txconf = { > + .direction = DMA_MEM_TO_DEV, > + .dst_addr = sspi->dma_addr_tx, > + .dst_addr_width = 1, > + .dst_maxburst = 1, > + }; > + > + dmaengine_slave_config(master->dma_tx, &txconf); > + > + txdesc = dmaengine_prep_slave_sg( > + master->dma_tx, > + tfr->tx_sg.sgl, tfr->tx_sg.nents, > + DMA_MEM_TO_DEV, DMA_PREP_INTERRUPT); > + if (!txdesc) { > + if (rxdesc) > + dmaengine_terminate_sync(master->dma_rx); > + return -EINVAL; > + } > + } > + > + if (tfr->rx_buf) { > + dmaengine_submit(rxdesc); > + dma_async_issue_pending(master->dma_rx); > + } > + > + if (tfr->tx_buf) { > + dmaengine_submit(txdesc); > + dma_async_issue_pending(master->dma_tx); > + } > + > + return 0; > +} > + > static int sun6i_spi_transfer_one(struct spi_master *master, > struct spi_device *spi, > struct spi_transfer *tfr) > @@ -201,6 +267,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master, > sspi->tx_buf = tfr->tx_buf; > sspi->rx_buf = tfr->rx_buf; > sspi->len = tfr->len; > + sspi->use_dma = master->can_dma ? > + master->can_dma(master, spi, tfr) : false; > > /* Clear pending interrupts */ > sun6i_spi_write(sspi, SUN6I_INT_STA_REG, ~0); > @@ -216,9 +284,17 @@ static int sun6i_spi_transfer_one(struct spi_master *master, > * (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) | > - (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS)); > + reg = (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) | > + (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS); > + > + if (sspi->use_dma) { > + if (tfr->tx_buf) > + reg |= SUN6I_FIFO_CTL_TF_DRQ_EN; > + if (tfr->rx_buf) > + reg |= SUN6I_FIFO_CTL_RF_DRQ_EN; > + } > + > + sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG, reg); > > /* > * Setup the transfer control register: Chip Select, > @@ -300,16 +376,28 @@ static int sun6i_spi_transfer_one(struct spi_master *master, > sun6i_spi_write(sspi, SUN6I_XMIT_CNT_REG, tx_len); > sun6i_spi_write(sspi, SUN6I_BURST_CTL_CNT_REG, tx_len); > > - /* Fill the TX FIFO */ > - sun6i_spi_fill_fifo(sspi); > + if (!sspi->use_dma) { > + /* Fill the TX FIFO */ > + sun6i_spi_fill_fifo(sspi); > + } else { > + ret = sun6i_spi_prepare_dma(sspi, tfr); > + if (ret) { > + dev_warn(&master->dev, > + "%s: prepare DMA failed, ret=%d", > + dev_name(&spi->dev), ret); > + return ret; > + } > + } > > /* Enable the interrupts */ > reg = SUN6I_INT_CTL_TC; > > - if (rx_len > sspi->fifo_depth) > - reg |= SUN6I_INT_CTL_RF_RDY; > - if (tx_len > sspi->fifo_depth) > - reg |= SUN6I_INT_CTL_TF_ERQ; > + if (!sspi->use_dma) { > + if (rx_len > sspi->fifo_depth) > + reg |= SUN6I_INT_CTL_RF_RDY; > + if (tx_len > sspi->fifo_depth) > + reg |= SUN6I_INT_CTL_TF_ERQ; > + } > > sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, reg); > > @@ -332,6 +420,11 @@ static int sun6i_spi_transfer_one(struct spi_master *master, > > sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0); > > + if (ret && sspi->use_dma) { > + dmaengine_terminate_sync(master->dma_rx); > + dmaengine_terminate_sync(master->dma_tx); > + } > + > return ret; > } > > @@ -343,7 +436,8 @@ static irqreturn_t sun6i_spi_handler(int irq, void *dev_id) > /* Transfer complete */ > if (status & SUN6I_INT_CTL_TC) { > sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_TC); > - sun6i_spi_drain_fifo(sspi); > + if (!sspi->use_dma) > + sun6i_spi_drain_fifo(sspi); Is it causing any issue? The FIFO will be drained only if there's something remaining in the FIFO, which shouldn't happen with DMA? Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Maxime Ripard <maxime@cerno.tech> To: Alexander Kochetkov <al.kochet@gmail.com> Cc: Chen-Yu Tsai <wens@csie.org>, Mark Brown <broonie@kernel.org>, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-spi@vger.kernel.org Subject: Re: [PATCH] spi: spi-sun6i: implement DMA-based transfer mode Date: Mon, 19 Oct 2020 10:21:29 +0200 [thread overview] Message-ID: <20201019082129.myxpxla5xwoqwldo@gilmour.lan> (raw) In-Reply-To: <20201015154740.20825-1-al.kochet@gmail.com> [-- Attachment #1.1: Type: text/plain, Size: 7028 bytes --] Hi! On Thu, Oct 15, 2020 at 06:47:40PM +0300, Alexander Kochetkov wrote: > DMA-based transfer will be enabled if data length is larger than FIFO size > (64 bytes for A64). This greatly reduce number of interrupts for > transferring data. > > For smaller data size PIO mode will be used. In PIO mode whole buffer will > be loaded into FIFO. > > If driver failed to request DMA channels then it fallback for PIO mode. > > Tested on SOPINE (https://www.pine64.org/sopine/) > > Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com> Thanks for working on this, it's been a bit overdue > --- > drivers/spi/spi-sun6i.c | 171 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 159 insertions(+), 12 deletions(-) > > diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c > index 19238e1b76b4..38e5b8af7da6 100644 > --- a/drivers/spi/spi-sun6i.c > +++ b/drivers/spi/spi-sun6i.c > @@ -18,6 +18,7 @@ > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > #include <linux/reset.h> > +#include <linux/dmaengine.h> > > #include <linux/spi/spi.h> > > @@ -52,10 +53,12 @@ > > #define SUN6I_FIFO_CTL_REG 0x18 > #define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_MASK 0xff > +#define SUN6I_FIFO_CTL_RF_DRQ_EN BIT(8) > #define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS 0 > #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_DRQ_EN BIT(24) > #define SUN6I_FIFO_CTL_TF_RST BIT(31) > > #define SUN6I_FIFO_STA_REG 0x1c > @@ -83,6 +86,8 @@ > struct sun6i_spi { > struct spi_master *master; > void __iomem *base_addr; > + dma_addr_t dma_addr_rx; > + dma_addr_t dma_addr_tx; > struct clk *hclk; > struct clk *mclk; > struct reset_control *rstc; > @@ -92,6 +97,7 @@ struct sun6i_spi { > const u8 *tx_buf; > u8 *rx_buf; > int len; > + bool use_dma; > unsigned long fifo_depth; > }; > > @@ -182,6 +188,66 @@ static size_t sun6i_spi_max_transfer_size(struct spi_device *spi) > return SUN6I_MAX_XFER_SIZE - 1; > } > > +static int sun6i_spi_prepare_dma(struct sun6i_spi *sspi, > + struct spi_transfer *tfr) > +{ > + struct dma_async_tx_descriptor *rxdesc, *txdesc; > + struct spi_master *master = sspi->master; > + > + rxdesc = NULL; > + if (tfr->rx_buf) { > + struct dma_slave_config rxconf = { > + .direction = DMA_DEV_TO_MEM, > + .src_addr = sspi->dma_addr_rx, > + .src_addr_width = 1, > + .src_maxburst = 1, > + }; That doesn't really look optimal, the controller seems to be able to read / write 32 bits at a time from its FIFO and we probably can increase the burst length too? > + > + dmaengine_slave_config(master->dma_rx, &rxconf); > + > + rxdesc = dmaengine_prep_slave_sg( > + master->dma_rx, > + tfr->rx_sg.sgl, tfr->rx_sg.nents, > + DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT); checkpatch --strict complains about this one (your line shouldn't end with a parenthesis). > + if (!rxdesc) > + return -EINVAL; > + } > + > + txdesc = NULL; > + if (tfr->tx_buf) { > + struct dma_slave_config txconf = { > + .direction = DMA_MEM_TO_DEV, > + .dst_addr = sspi->dma_addr_tx, > + .dst_addr_width = 1, > + .dst_maxburst = 1, > + }; > + > + dmaengine_slave_config(master->dma_tx, &txconf); > + > + txdesc = dmaengine_prep_slave_sg( > + master->dma_tx, > + tfr->tx_sg.sgl, tfr->tx_sg.nents, > + DMA_MEM_TO_DEV, DMA_PREP_INTERRUPT); > + if (!txdesc) { > + if (rxdesc) > + dmaengine_terminate_sync(master->dma_rx); > + return -EINVAL; > + } > + } > + > + if (tfr->rx_buf) { > + dmaengine_submit(rxdesc); > + dma_async_issue_pending(master->dma_rx); > + } > + > + if (tfr->tx_buf) { > + dmaengine_submit(txdesc); > + dma_async_issue_pending(master->dma_tx); > + } > + > + return 0; > +} > + > static int sun6i_spi_transfer_one(struct spi_master *master, > struct spi_device *spi, > struct spi_transfer *tfr) > @@ -201,6 +267,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master, > sspi->tx_buf = tfr->tx_buf; > sspi->rx_buf = tfr->rx_buf; > sspi->len = tfr->len; > + sspi->use_dma = master->can_dma ? > + master->can_dma(master, spi, tfr) : false; > > /* Clear pending interrupts */ > sun6i_spi_write(sspi, SUN6I_INT_STA_REG, ~0); > @@ -216,9 +284,17 @@ static int sun6i_spi_transfer_one(struct spi_master *master, > * (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) | > - (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS)); > + reg = (trig_level << SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS) | > + (trig_level << SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS); > + > + if (sspi->use_dma) { > + if (tfr->tx_buf) > + reg |= SUN6I_FIFO_CTL_TF_DRQ_EN; > + if (tfr->rx_buf) > + reg |= SUN6I_FIFO_CTL_RF_DRQ_EN; > + } > + > + sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG, reg); > > /* > * Setup the transfer control register: Chip Select, > @@ -300,16 +376,28 @@ static int sun6i_spi_transfer_one(struct spi_master *master, > sun6i_spi_write(sspi, SUN6I_XMIT_CNT_REG, tx_len); > sun6i_spi_write(sspi, SUN6I_BURST_CTL_CNT_REG, tx_len); > > - /* Fill the TX FIFO */ > - sun6i_spi_fill_fifo(sspi); > + if (!sspi->use_dma) { > + /* Fill the TX FIFO */ > + sun6i_spi_fill_fifo(sspi); > + } else { > + ret = sun6i_spi_prepare_dma(sspi, tfr); > + if (ret) { > + dev_warn(&master->dev, > + "%s: prepare DMA failed, ret=%d", > + dev_name(&spi->dev), ret); > + return ret; > + } > + } > > /* Enable the interrupts */ > reg = SUN6I_INT_CTL_TC; > > - if (rx_len > sspi->fifo_depth) > - reg |= SUN6I_INT_CTL_RF_RDY; > - if (tx_len > sspi->fifo_depth) > - reg |= SUN6I_INT_CTL_TF_ERQ; > + if (!sspi->use_dma) { > + if (rx_len > sspi->fifo_depth) > + reg |= SUN6I_INT_CTL_RF_RDY; > + if (tx_len > sspi->fifo_depth) > + reg |= SUN6I_INT_CTL_TF_ERQ; > + } > > sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, reg); > > @@ -332,6 +420,11 @@ static int sun6i_spi_transfer_one(struct spi_master *master, > > sun6i_spi_write(sspi, SUN6I_INT_CTL_REG, 0); > > + if (ret && sspi->use_dma) { > + dmaengine_terminate_sync(master->dma_rx); > + dmaengine_terminate_sync(master->dma_tx); > + } > + > return ret; > } > > @@ -343,7 +436,8 @@ static irqreturn_t sun6i_spi_handler(int irq, void *dev_id) > /* Transfer complete */ > if (status & SUN6I_INT_CTL_TC) { > sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_TC); > - sun6i_spi_drain_fifo(sspi); > + if (!sspi->use_dma) > + sun6i_spi_drain_fifo(sspi); Is it causing any issue? The FIFO will be drained only if there's something remaining in the FIFO, which shouldn't happen with DMA? Maxime [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-10-19 8:21 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-15 15:47 [PATCH] spi: spi-sun6i: implement DMA-based transfer mode Alexander Kochetkov 2020-10-15 15:47 ` Alexander Kochetkov 2020-10-19 8:21 ` Maxime Ripard [this message] 2020-10-19 8:21 ` Maxime Ripard 2020-10-19 13:17 ` Alexander Kochetkov 2020-10-19 13:17 ` Alexander Kochetkov 2020-10-21 16:39 ` Maxime Ripard 2020-10-21 16:39 ` Maxime Ripard 2020-10-19 17:43 ` Alexander Kochetkov 2020-10-19 17:43 ` Alexander Kochetkov 2020-10-20 3:52 ` Chen-Yu Tsai 2020-10-20 3:52 ` Chen-Yu Tsai 2020-10-21 15:03 ` Maxime Ripard 2020-10-21 15:03 ` Maxime Ripard
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20201019082129.myxpxla5xwoqwldo@gilmour.lan \ --to=maxime@cerno.tech \ --cc=al.kochet@gmail.com \ --cc=broonie@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-spi@vger.kernel.org \ --cc=wens@csie.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.