Re: [PATCH] spi: spi-sun6i: implement DMA-based transfer mode

From: Maxime Ripard
Date: Mon Oct 19 2020 - 04:21:38 EST

Next message: Bernard Zhao: "[PATCH] drivers/thermal: optimize the for circle to run a bit fast"
Previous message: Pali Rohár: "Re: [PATCH v2] arm64: dts: marvell: espressobin: Update link to V7 schematic"
Next in thread: Alexander Kochetkov: "Re: [PATCH] spi: spi-sun6i: implement DMA-based transfer mode"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>

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: signature.asc
Description: PGP signature

Next message: Bernard Zhao: "[PATCH] drivers/thermal: optimize the for circle to run a bit fast"
Previous message: Pali Rohár: "Re: [PATCH v2] arm64: dts: marvell: espressobin: Update link to V7 schematic"
Next in thread: Alexander Kochetkov: "Re: [PATCH] spi: spi-sun6i: implement DMA-based transfer mode"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]