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

[Maxime Ripard <maxime@cerno.tech>]

[-- 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 --]