DMAEngine support for sun4i, sun5i & sun7i

drivers/dma/sun4i-dma.c new file +15 -0

+static struct sun4i_dma_pchan *find_and_use_pchan(struct sun4i_dma_dev *priv,
+                                                 struct sun4i_dma_vchan *vchan)
+{
+       struct sun4i_dma_pchan *pchan = NULL, *pchans = priv->pchans;
+       unsigned long flags;
+       int i, max;
+
+       /*
+        * pchans 0-NDMA_NR_MAX_CHANNELS are normal, and
+        * NDMA_NR_MAX_CHANNELS+ are dedicated ones
+        */
+       if (vchan->is_dedicated) {
+               i = NDMA_NR_MAX_CHANNELS;
+               max = DMA_NR_MAX_CHANNELS;
+       } else {

drivers/dma/sun4i-dma.c new file +24 -0

+/**
+ * Generate a promise, to be used in a normal DMA contract.
+ *
+ * A NDMA promise contains all the information required to program the
+ * normal part of the DMA Engine and get data copied. A non-executed
+ * promise will live in the demands list on a contract. Once it has been
+ * completed, it will be moved to the completed demands list for later freeing.
+ * All linked promises will be freed when the corresponding contract is freed
+ */
+static struct sun4i_dma_promise *
+generate_ndma_promise(struct dma_chan *chan, dma_addr_t src, dma_addr_t dest,
+		      size_t len, struct dma_slave_config *sconfig)
+{
+	struct sun4i_dma_promise *promise;
+	int ret;
+
+	promise = kzalloc(sizeof(*promise), GFP_NOWAIT);
+	if (!promise)
+		return NULL;
+
+	promise->src = src;
+	promise->dst = dest;
+	promise->len = len;
+	promise->cfg = NDMA_CFG_LOADING | NDMA_CFG_BYTE_COUNT_MODE_REMAIN;

drivers/dma/sun4i-dma.c new file +6 -0

+static irqreturn_t sun4i_dma_submit_work(int irq, void *dev_id)
+{
+	struct sun4i_dma_dev *priv = dev_id;
+	struct sun4i_dma_vchan *vchan;
+	unsigned long flags;
+	int i;

Hi,

Overall it looks very nice, thanks.

Quoted 19 lines · Emilio L?pez

drivers/dma/sun4i-dma.c new file +19 -0

+static struct sun4i_dma_pchan *find_and_use_pchan(struct sun4i_dma_dev *priv,
+                                                 struct sun4i_dma_vchan *vchan)
+{
+       struct sun4i_dma_pchan *pchan = NULL, *pchans = priv->pchans;
+       unsigned long flags;
+       int i, max;
+
+       /*
+        * pchans 0-NDMA_NR_MAX_CHANNELS are normal, and
+        * NDMA_NR_MAX_CHANNELS+ are dedicated ones
+        */
+       if (vchan->is_dedicated) {
+               i = NDMA_NR_MAX_CHANNELS;
+               max = DMA_NR_MAX_CHANNELS;
+       } else {
+               i = 0;
+               max = NDMA_NR_MAX_CHANNELS;
+       }
+

drivers/dma/sun4i-dma.c line 262

Quoted 6 lines

drivers/dma/sun4i-dma.c new file +6 -0

+       spin_lock_irqsave(&priv->lock, flags);
+       for_each_clear_bit_from(i, &priv->pchans_used, max) {
+               pchan = &pchans[i];
+               pchan->vchan = vchan;
+               set_bit(i, priv->pchans_used);
+               break;

ffz instead?

drivers/dma/sun4i-dma.c line 340

Quoted 10 lines

drivers/dma/sun4i-dma.c new file +10 -0

+static int execute_vchan_pending(struct sun4i_dma_dev *priv,
+				 struct sun4i_dma_vchan *vchan)
+{
+	struct sun4i_dma_promise *promise = NULL;
+	struct sun4i_dma_contract *contract = NULL;
+	struct sun4i_dma_pchan *pchan;
+	struct virt_dma_desc *vd;
+	int ret;
+
+	lockdep_assert_held(&vchan->vc.lock);

So this has to be called with a lock taken?

You should mention that somewhere.
Usually, such fonctions are also prefixed by "__"

Quoted 24 lines

drivers/dma/sun4i-dma.c new file +24 -0

+/**
+ * Generate a promise, to be used in a normal DMA contract.
+ *
+ * A NDMA promise contains all the information required to program the
+ * normal part of the DMA Engine and get data copied. A non-executed
+ * promise will live in the demands list on a contract. Once it has been
+ * completed, it will be moved to the completed demands list for later freeing.
+ * All linked promises will be freed when the corresponding contract is freed
+ */
+static struct sun4i_dma_promise *
+generate_ndma_promise(struct dma_chan *chan, dma_addr_t src, dma_addr_t dest,
+		      size_t len, struct dma_slave_config *sconfig)
+{
+	struct sun4i_dma_promise *promise;
+	int ret;
+
+	promise = kzalloc(sizeof(*promise), GFP_NOWAIT);
+	if (!promise)
+		return NULL;
+
+	promise->src = src;
+	promise->dst = dest;
+	promise->len = len;
+	promise->cfg = NDMA_CFG_LOADING | NDMA_CFG_BYTE_COUNT_MODE_REMAIN;

drivers/dma/sun4i-dma.c line 431

Quoted 10 lines

drivers/dma/sun4i-dma.c new file +10 -0

+
+	/* Use sensible default values if client is using undefined ones */
+	if (sconfig->src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
+		sconfig->src_addr_width = sconfig->dst_addr_width;
+	if (sconfig->dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
+		sconfig->dst_addr_width = sconfig->src_addr_width;
+	if (sconfig->src_maxburst == 0)
+		sconfig->src_maxburst = sconfig->dst_maxburst;
+	if (sconfig->dst_maxburst == 0)
+		sconfig->dst_maxburst = sconfig->src_maxburst;

I'm not sure what's the default policy on that. Vinod?

Quoted 6 lines

drivers/dma/sun4i-dma.c new file +6 -0

+static irqreturn_t sun4i_dma_submit_work(int irq, void *dev_id)
+{
+	struct sun4i_dma_dev *priv = dev_id;
+	struct sun4i_dma_vchan *vchan;
+	unsigned long flags;
+	int i;

drivers/dma/sun4i-dma.c line 1067

Quoted 10 lines

drivers/dma/sun4i-dma.c new file +10 -0

+
+	for (i = 0; i < DMA_NR_MAX_VCHANS; i++) {
+		vchan = &priv->vchans[i];
+		spin_lock_irqsave(&vchan->vc.lock, flags);
+		execute_vchan_pending(priv, vchan);
+		spin_unlock_irqrestore(&vchan->vc.lock, flags);
+	}
+
+	return IRQ_HANDLED;
+}

Judging from Russell's comment here, that should be in the interrupt handler

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/277737.html

Hi,

El 05/08/14 a las 17:00, Maxime Ripard escibi?:

Quoted 28 lines

Hi,

Overall it looks very nice, thanks.

Quoted 23 lines · Emilio L?pez

+static struct sun4i_dma_pchan *find_and_use_pchan(struct sun4i_dma_dev *priv,
+ struct sun4i_dma_vchan
*vchan)

drivers/dma/sun4i-dma.c new file +18 -0

+{
+       struct sun4i_dma_pchan *pchan = NULL, *pchans = priv->pchans;
+       unsigned long flags;
+       int i, max;
+
+       /*
+        * pchans 0-NDMA_NR_MAX_CHANNELS are normal, and
+        * NDMA_NR_MAX_CHANNELS+ are dedicated ones
+        */
+       if (vchan->is_dedicated) {
+               i = NDMA_NR_MAX_CHANNELS;
+               max = DMA_NR_MAX_CHANNELS;
+       } else {
+               i = 0;
+               max = NDMA_NR_MAX_CHANNELS;
+       }
+
+       spin_lock_irqsave(&priv->lock, flags);

Quoted 8 lines

drivers/dma/sun4i-dma.c line 262

Quoted 5 lines

drivers/dma/sun4i-dma.c new file +5 -0

+       for_each_clear_bit_from(i, &priv->pchans_used, max) {
+               pchan = &pchans[i];
+               pchan->vchan = vchan;
+               set_bit(i, priv->pchans_used);
+               break;

ffz instead?

I think it'd look way more messy with ffz, as it only works on unsigned longs and it's undefined on the all-1s case.

We could have something like this, but I don't see much point in what's basically expanding the macro

        spin_lock_irqsave(&priv->lock, flags);
        i = find_next_zero_bit(&priv->pchans_used, max, i);
        if (i < max) {
                pchan = &pchans[i];
                pchan->vchan = vchan;
                set_bit(i, priv->pchans_used);
        }
        spin_unlock_irqrestore(&priv->lock, flags);

Quoted 14 lines

drivers/dma/sun4i-dma.c line 340

Quoted 10 lines

drivers/dma/sun4i-dma.c new file +10 -0

+static int execute_vchan_pending(struct sun4i_dma_dev *priv,
+                                struct sun4i_dma_vchan *vchan)
+{
+       struct sun4i_dma_promise *promise = NULL;
+       struct sun4i_dma_contract *contract = NULL;
+       struct sun4i_dma_pchan *pchan;
+       struct virt_dma_desc *vd;
+       int ret;
+
+       lockdep_assert_held(&vchan->vc.lock);

So this has to be called with a lock taken?

Quoted 2 lines

You should mention that somewhere.
Usually, such fonctions are also prefixed by "__"

Ok, I added a comment and the prefix now.

Quoted 65 lines

Quoted 31 lines

+/**
+ * Generate a promise, to be used in a normal DMA contract.
+ *
+ * A NDMA promise contains all the information required to program the
+ * normal part of the DMA Engine and get data copied. A non-executed
+ * promise will live in the demands list on a contract. Once it has been
+ * completed, it will be moved to the completed demands list for later freeing.
+ * All linked promises will be freed when the corresponding contract is freed
+ */

(quoted diff) +2 -0

+static struct sun4i_dma_promise *
+generate_ndma_promise(struct dma_chan *chan, dma_addr_t src, dma_addr_t

dest,

drivers/dma/sun4i-dma.c new file +13 -0

+                     size_t len, struct dma_slave_config *sconfig)
+{
+       struct sun4i_dma_promise *promise;
+       int ret;
+
+       promise = kzalloc(sizeof(*promise), GFP_NOWAIT);
+       if (!promise)
+               return NULL;
+
+       promise->src = src;
+       promise->dst = dest;
+       promise->len = len;
+       promise->cfg = NDMA_CFG_LOADING |

NDMA_CFG_BYTE_COUNT_MODE_REMAIN;

(quoted diff) +2 -0

+
+       /* Use sensible default values if client is using undefined ones

*/

drivers/dma/sun4i-dma.c line 431

Quoted 8 lines

drivers/dma/sun4i-dma.c new file +8 -0

+       if (sconfig->src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
+               sconfig->src_addr_width = sconfig->dst_addr_width;
+       if (sconfig->dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
+               sconfig->dst_addr_width = sconfig->src_addr_width;
+       if (sconfig->src_maxburst == 0)
+               sconfig->src_maxburst = sconfig->dst_maxburst;
+       if (sconfig->dst_maxburst == 0)
+               sconfig->dst_maxburst = sconfig->src_maxburst;

I'm not sure what's the default policy on that. Vinod?

Quoted 6 lines

drivers/dma/sun4i-dma.c new file +6 -0

+static irqreturn_t sun4i_dma_submit_work(int irq, void *dev_id)
+{
+       struct sun4i_dma_dev *priv = dev_id;
+       struct sun4i_dma_vchan *vchan;
+       unsigned long flags;
+       int i;

drivers/dma/sun4i-dma.c line 1067

Quoted 10 lines

drivers/dma/sun4i-dma.c new file +10 -0

+
+       for (i = 0; i < DMA_NR_MAX_VCHANS; i++) {
+               vchan = &priv->vchans[i];
+               spin_lock_irqsave(&vchan->vc.lock, flags);
+               execute_vchan_pending(priv, vchan);
+               spin_unlock_irqrestore(&vchan->vc.lock, flags);
+       }
+
+       return IRQ_HANDLED;
+}

Judging from Russell's comment here, that should be in the interrupt handler

Quoted 1 line

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/277737.html

Wouldn't that make the interrupt handler quite big time wise? I was under the impression they should remain as short as possible. LDD3 says "The programmer should be careful to write a routine that executes in a minimum amount of time (...)" on chapter 10

I did try something mid-way a bit ago; I added code to see if we could issue the next DMA operation from the current set, but it was just extra complexity for a noop in performance, as they're sets of one on all cases I tried.

It's worth noting that the very latency sensitive DMA users we will have (ie, audio) are already completely managed in the IRQ handler thanks to the less flexible concept of cyclic transfers.

Cheers,

Emilio

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi,

El 05/08/14 a las 17:00, Maxime Ripard escibi?:

Quoted 24 lines

Hi,

Overall it looks very nice, thanks.

Quoted 19 lines · Emilio L?pez

drivers/dma/sun4i-dma.c new file +19 -0

+static struct sun4i_dma_pchan *find_and_use_pchan(struct sun4i_dma_dev *priv,
+                                                 struct sun4i_dma_vchan *vchan)
+{
+       struct sun4i_dma_pchan *pchan = NULL, *pchans = priv->pchans;
+       unsigned long flags;
+       int i, max;
+
+       /*
+        * pchans 0-NDMA_NR_MAX_CHANNELS are normal, and
+        * NDMA_NR_MAX_CHANNELS+ are dedicated ones
+        */
+       if (vchan->is_dedicated) {
+               i = NDMA_NR_MAX_CHANNELS;
+               max = DMA_NR_MAX_CHANNELS;
+       } else {
+               i = 0;
+               max = NDMA_NR_MAX_CHANNELS;
+       }
+

Quoted 8 lines

drivers/dma/sun4i-dma.c line 262

Quoted 6 lines

drivers/dma/sun4i-dma.c new file +6 -0

+       spin_lock_irqsave(&priv->lock, flags);
+       for_each_clear_bit_from(i, &priv->pchans_used, max) {
+               pchan = &pchans[i];
+               pchan->vchan = vchan;
+               set_bit(i, priv->pchans_used);
+               break;

ffz instead?

I think it'd look way more messy with ffz, as it only works on unsigned longs and it's undefined on the all-1s case.

We could have something like this, but I don't see much point in what's basically expanding the macro

	spin_lock_irqsave(&priv->lock, flags);
	i = find_next_zero_bit(&priv->pchans_used, max, i);
	if (i < max) {
		pchan = &pchans[i];
		pchan->vchan = vchan;
		set_bit(i, priv->pchans_used);
	}
	spin_unlock_irqrestore(&priv->lock, flags);

Quoted 20 lines

Quoted 6 lines

drivers/dma/sun4i-dma.c new file +6 -0

+static irqreturn_t sun4i_dma_submit_work(int irq, void *dev_id)
+{
+	struct sun4i_dma_dev *priv = dev_id;
+	struct sun4i_dma_vchan *vchan;
+	unsigned long flags;
+	int i;

drivers/dma/sun4i-dma.c line 1067

Quoted 10 lines

drivers/dma/sun4i-dma.c new file +10 -0

+
+	for (i = 0; i < DMA_NR_MAX_VCHANS; i++) {
+		vchan = &priv->vchans[i];
+		spin_lock_irqsave(&vchan->vc.lock, flags);
+		execute_vchan_pending(priv, vchan);
+		spin_unlock_irqrestore(&vchan->vc.lock, flags);
+	}
+
+	return IRQ_HANDLED;
+}

Judging from Russell's comment here, that should be in the interrupt handler

Quoted 1 line

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/277737.html

Wouldn't that make the interrupt handler quite big time wise? I was under the impression they should remain as short as possible. LDD3 says "The programmer should be careful to write a routine that executes in a minimum amount of time (...)" on chapter 10

I did try something mid-way a bit ago; I added code to see if we could issue the next DMA operation from the current set, but it was just extra complexity for a noop in performance, as they're sets of one on all cases I tried.

It's worth noting that the very latency sensitive DMA users we will have (ie, audio) are already completely managed in the IRQ handler thanks to the less flexible concept of cyclic transfers.

Quoted 42 lines · Emilio L?pez

Hi,

El 05/08/14 a las 17:00, Maxime Ripard escibi?:

Quoted 24 lines

Hi,

Overall it looks very nice, thanks.

Quoted 19 lines · Emilio L?pez

drivers/dma/sun4i-dma.c new file +19 -0

+static struct sun4i_dma_pchan *find_and_use_pchan(struct sun4i_dma_dev *priv,
+                                                 struct sun4i_dma_vchan *vchan)
+{
+       struct sun4i_dma_pchan *pchan = NULL, *pchans = priv->pchans;
+       unsigned long flags;
+       int i, max;
+
+       /*
+        * pchans 0-NDMA_NR_MAX_CHANNELS are normal, and
+        * NDMA_NR_MAX_CHANNELS+ are dedicated ones
+        */
+       if (vchan->is_dedicated) {
+               i = NDMA_NR_MAX_CHANNELS;
+               max = DMA_NR_MAX_CHANNELS;
+       } else {
+               i = 0;
+               max = NDMA_NR_MAX_CHANNELS;
+       }
+

Quoted 8 lines

drivers/dma/sun4i-dma.c line 262

Quoted 6 lines

drivers/dma/sun4i-dma.c new file +6 -0

+       spin_lock_irqsave(&priv->lock, flags);
+       for_each_clear_bit_from(i, &priv->pchans_used, max) {
+               pchan = &pchans[i];
+               pchan->vchan = vchan;
+               set_bit(i, priv->pchans_used);
+               break;

ffz instead?

I think it'd look way more messy with ffz, as it only works on unsigned longs and it's undefined on the all-1s case.

We could have something like this, but I don't see much point in what's basically expanding the macro

Quoted 8 lines

      spin_lock_irqsave(&priv->lock, flags);
      i = find_next_zero_bit(&priv->pchans_used, max, i);
      if (i < max) {
              pchan = &pchans[i];
              pchan->vchan = vchan;
              set_bit(i, priv->pchans_used);
      }
      spin_unlock_irqrestore(&priv->lock, flags);

What about

spin_lock_irqsave(&priv->lock, flags);
i = ffz(&priv->pchans_used);
if (!i)
return NULL;

pchan = &pchans[i];
pchan->vchan = vchan;
set_bit(i, priv->pchans_used);

spin_unlock_irqrestore(&priv->lock, flags);

It looks quite clean to me.

Quoted 20 lines

Quoted 20 lines

Quoted 6 lines

drivers/dma/sun4i-dma.c new file +6 -0

+static irqreturn_t sun4i_dma_submit_work(int irq, void *dev_id)
+{
+   struct sun4i_dma_dev *priv = dev_id;
+   struct sun4i_dma_vchan *vchan;
+   unsigned long flags;
+   int i;

drivers/dma/sun4i-dma.c line 1067

Quoted 10 lines

drivers/dma/sun4i-dma.c new file +10 -0

+
+   for (i = 0; i < DMA_NR_MAX_VCHANS; i++) {
+           vchan = &priv->vchans[i];
+           spin_lock_irqsave(&vchan->vc.lock, flags);
+           execute_vchan_pending(priv, vchan);
+           spin_unlock_irqrestore(&vchan->vc.lock, flags);
+   }
+
+   return IRQ_HANDLED;
+}

Judging from Russell's comment here, that should be in the interrupt handler

Quoted 6 lines

Quoted 1 line

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/277737.html

Wouldn't that make the interrupt handler quite big time wise? I was under the impression they should remain as short as possible. LDD3 says "The programmer should be careful to write a routine that executes in a minimum amount of time (...)" on chapter 10

Yeah, but if that involves delaying a lot a task that should have been performed asap, it would make sense.

Plus, besides the loop in the contract's demands, execute_vchan_pending is actually rather small. Did you timed it to have such concerns?

Quoted 5 lines

I did try something mid-way a bit ago; I added code to see if we could issue the next DMA operation from the current set, but it was just extra complexity for a noop in performance, as they're sets of one on all cases I tried.

Quoted 3 lines

It's worth noting that the very latency sensitive DMA users we will have (ie, audio) are already completely managed in the IRQ handler thanks to the less flexible concept of cyclic transfers.

It's not just a matter of latency, but also of throughput. If you wait for the tasklet softirq to be executed to schedule a new vchan, you aren't going to do anything during that time, while you could have.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

drivers/spi/spi-sun4i.c +31 -0

+	if (sun4i_spi_can_dma(master, spi, tfr)) {
+		dev_dbg(&sspi->master->dev, "Using DMA mode for transfer\n");
+
+		if (sspi->tx_buf) {
+			desc_tx = dmaengine_prep_slave_sg(sspi->tx_dma_chan,
+					tfr->tx_sg.sgl, tfr->tx_sg.nents,
+					DMA_TO_DEVICE,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+			if (!desc_tx) {
+				dev_err(&sspi->master->dev,
+					"Couldn't prepare dma slave\n");
+				return -EIO;
+			}
+
+			dmaengine_submit(desc_tx);
+		}
+
+		if (sspi->rx_buf) {
+			desc_rx = dmaengine_prep_slave_sg(sspi->rx_dma_chan,
+					tfr->rx_sg.sgl, tfr->rx_sg.nents,
+					DMA_FROM_DEVICE,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+			if (!desc_rx) {
+				dev_err(&sspi->master->dev,
+					"Couldn't prepare dma slave\n");
+				return -EIO;
+			}
+
+			dmaengine_submit(desc_rx);
+		}
+

drivers/spi/spi-sun4i.c +67 -3

+	} else {
+		dev_dbg(&sspi->master->dev, "Using PIO mode for transfer\n");
+
+		/* Disable DMA requests */
+		reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
+		sun4i_spi_write(sspi, SUN4I_CTL_REG,
+				reg & ~SUN4I_CTL_DMAMC_DEDICATED);
+		sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, 0);
+
+		/* Fill the TX FIFO */
+		sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
+	}
+
 	/* Start the transfer */
 	reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
 	sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);

@@ -286,7 +359,12 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 		goto out;
 	}
 
-	sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+	if (sun4i_spi_can_dma(master, spi, tfr) && desc_rx) {
+		/* The receive transfer should be the last one to finish */
+		dma_wait_for_async_tx(desc_rx);
+	} else {
+		sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+	}
 
 out:
 	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);

@@ -351,6 +429,7 @@ static int sun4i_spi_runtime_suspend(struct device *dev)
 
 static int sun4i_spi_probe(struct platform_device *pdev)
 {
+	struct dma_slave_config dma_sconfig;
 	struct spi_master *master;
 	struct sun4i_spi *sspi;
 	struct resource	*res;

@@ -386,7 +465,10 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 		goto err_free_master;
 	}
 
+	init_completion(&sspi->done);
 	sspi->master = master;
+	master->can_dma = sun4i_spi_can_dma;
+	master->prepare_message = sun4i_spi_prepare_message;
 	master->set_cs = sun4i_spi_set_cs;
 	master->transfer_one = sun4i_spi_transfer_one;
 	master->num_chipselect = 4;

@@ -409,7 +491,45 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 		goto err_free_master;
 	}
 
-	init_completion(&sspi->done);
+	sspi->tx_dma_chan = dma_request_slave_channel_reason(&pdev->dev, "tx");
+	if (IS_ERR(sspi->tx_dma_chan)) {
+		dev_err(&pdev->dev, "Unable to acquire DMA channel TX\n");
+		ret = PTR_ERR(sspi->tx_dma_chan);
+		goto err_free_master;
+	}
+
+	dma_sconfig.direction = DMA_MEM_TO_DEV;
+	dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.dst_addr = res->start + SUN4I_TXDATA_REG;
+	dma_sconfig.src_maxburst = 1;
+	dma_sconfig.dst_maxburst = 1;
+
+	ret = dmaengine_slave_config(sspi->tx_dma_chan, &dma_sconfig);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to configure TX DMA slave\n");
+		goto err_tx_dma_release;
+	}
+
+	sspi->rx_dma_chan = dma_request_slave_channel_reason(&pdev->dev, "rx");
+	if (IS_ERR(sspi->rx_dma_chan)) {
+		dev_err(&pdev->dev, "Unable to acquire DMA channel RX\n");
+		ret = PTR_ERR(sspi->rx_dma_chan);
+		goto err_tx_dma_release;
+	}
+
+	dma_sconfig.direction = DMA_DEV_TO_MEM;
+	dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.src_addr = res->start + SUN4I_RXDATA_REG;
+	dma_sconfig.src_maxburst = 1;
+	dma_sconfig.dst_maxburst = 1;
+
+	ret = dmaengine_slave_config(sspi->rx_dma_chan, &dma_sconfig);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to configure RX DMA slave\n");
+		goto err_rx_dma_release;
+	}
 
 	/*
 	 * This wake-up/shutdown pattern is to be able to have the

@@ -418,7 +538,7 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 	ret = sun4i_spi_runtime_resume(&pdev->dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Couldn't resume the device\n");
-		goto err_free_master;
+		goto err_rx_dma_release;
 	}
 
 	pm_runtime_set_active(&pdev->dev);

@@ -436,6 +556,10 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
 	sun4i_spi_runtime_suspend(&pdev->dev);
+err_rx_dma_release:
+	dma_release_channel(sspi->rx_dma_chan);
+err_tx_dma_release:
+	dma_release_channel(sspi->tx_dma_chan);
 err_free_master:
 	spi_master_put(master);
 	return ret;

Hi,

Quoted 37 lines · Emilio L?pez

drivers/spi/spi-sun4i.c +37 -0

+	if (sun4i_spi_can_dma(master, spi, tfr)) {
+		dev_dbg(&sspi->master->dev, "Using DMA mode for transfer\n");
+
+		if (sspi->tx_buf) {
+			desc_tx = dmaengine_prep_slave_sg(sspi->tx_dma_chan,
+					tfr->tx_sg.sgl, tfr->tx_sg.nents,
+					DMA_TO_DEVICE,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+			if (!desc_tx) {
+				dev_err(&sspi->master->dev,
+					"Couldn't prepare dma slave\n");
+				return -EIO;
+			}
+
+			dmaengine_submit(desc_tx);
+		}
+
+		if (sspi->rx_buf) {
+			desc_rx = dmaengine_prep_slave_sg(sspi->rx_dma_chan,
+					tfr->rx_sg.sgl, tfr->rx_sg.nents,
+					DMA_FROM_DEVICE,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+			if (!desc_rx) {
+				dev_err(&sspi->master->dev,
+					"Couldn't prepare dma slave\n");
+				return -EIO;
+			}
+
+			dmaengine_submit(desc_rx);
+		}
+
+		/* Enable DMA requests */
+		reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
+		sun4i_spi_write(sspi, SUN4I_CTL_REG,
+				reg | SUN4I_CTL_DMAMC_DEDICATED);
+		sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG,
+				SUN4I_DMA_CTL_TF_NOT_FULL |

drivers/spi/spi-sun4i.c static int sun4i_spi_transfer_one(struct spi_master *master, · line 337

Quoted 4 lines

drivers/spi/spi-sun4i.c +4 -0

+				SUN4I_DMA_CTL_RF_READY);
+
+		dma_async_issue_pending(sspi->rx_dma_chan);
+		dma_async_issue_pending(sspi->tx_dma_chan);

I guess the DMA levels setup and the issue pending calls could be moved to the if statements above about wether the RX and TX buffers are here or not.

Quoted 118 lines

drivers/spi/spi-sun4i.c +67 -3

+	} else {
+		dev_dbg(&sspi->master->dev, "Using PIO mode for transfer\n");
+
+		/* Disable DMA requests */
+		reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
+		sun4i_spi_write(sspi, SUN4I_CTL_REG,
+				reg & ~SUN4I_CTL_DMAMC_DEDICATED);
+		sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, 0);
+
+		/* Fill the TX FIFO */
+		sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
+	}
+
  	/* Start the transfer */
  	reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
  	sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);

@@ -286,7 +359,12 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
  		goto out;
  	}
 
-	sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+	if (sun4i_spi_can_dma(master, spi, tfr) && desc_rx) {
+		/* The receive transfer should be the last one to finish */
+		dma_wait_for_async_tx(desc_rx);
+	} else {
+		sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+	}
 
  out:
  	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);

@@ -351,6 +429,7 @@ static int sun4i_spi_runtime_suspend(struct device *dev)
 
  static int sun4i_spi_probe(struct platform_device *pdev)
  {
+	struct dma_slave_config dma_sconfig;
  	struct spi_master *master;
  	struct sun4i_spi *sspi;
  	struct resource	*res;

@@ -386,7 +465,10 @@ static int sun4i_spi_probe(struct platform_device *pdev)
  		goto err_free_master;
  	}
 
+	init_completion(&sspi->done);
  	sspi->master = master;
+	master->can_dma = sun4i_spi_can_dma;
+	master->prepare_message = sun4i_spi_prepare_message;
  	master->set_cs = sun4i_spi_set_cs;
  	master->transfer_one = sun4i_spi_transfer_one;
  	master->num_chipselect = 4;

@@ -409,7 +491,45 @@ static int sun4i_spi_probe(struct platform_device *pdev)
  		goto err_free_master;
  	}
 
-	init_completion(&sspi->done);
+	sspi->tx_dma_chan = dma_request_slave_channel_reason(&pdev->dev, "tx");
+	if (IS_ERR(sspi->tx_dma_chan)) {
+		dev_err(&pdev->dev, "Unable to acquire DMA channel TX\n");
+		ret = PTR_ERR(sspi->tx_dma_chan);
+		goto err_free_master;
+	}
+
+	dma_sconfig.direction = DMA_MEM_TO_DEV;
+	dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.dst_addr = res->start + SUN4I_TXDATA_REG;
+	dma_sconfig.src_maxburst = 1;
+	dma_sconfig.dst_maxburst = 1;
+
+	ret = dmaengine_slave_config(sspi->tx_dma_chan, &dma_sconfig);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to configure TX DMA slave\n");
+		goto err_tx_dma_release;
+	}
+
+	sspi->rx_dma_chan = dma_request_slave_channel_reason(&pdev->dev, "rx");
+	if (IS_ERR(sspi->rx_dma_chan)) {
+		dev_err(&pdev->dev, "Unable to acquire DMA channel RX\n");
+		ret = PTR_ERR(sspi->rx_dma_chan);
+		goto err_tx_dma_release;
+	}
+
+	dma_sconfig.direction = DMA_DEV_TO_MEM;
+	dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	dma_sconfig.src_addr = res->start + SUN4I_RXDATA_REG;
+	dma_sconfig.src_maxburst = 1;
+	dma_sconfig.dst_maxburst = 1;
+
+	ret = dmaengine_slave_config(sspi->rx_dma_chan, &dma_sconfig);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to configure RX DMA slave\n");
+		goto err_rx_dma_release;
+	}
 
  	/*
  	 * This wake-up/shutdown pattern is to be able to have the

@@ -418,7 +538,7 @@ static int sun4i_spi_probe(struct platform_device *pdev)
  	ret = sun4i_spi_runtime_resume(&pdev->dev);
  	if (ret) {
  		dev_err(&pdev->dev, "Couldn't resume the device\n");
-		goto err_free_master;
+		goto err_rx_dma_release;
  	}
 
  	pm_runtime_set_active(&pdev->dev);

@@ -436,6 +556,10 @@ static int sun4i_spi_probe(struct platform_device *pdev)
  err_pm_disable:
  	pm_runtime_disable(&pdev->dev);
  	sun4i_spi_runtime_suspend(&pdev->dev);
+err_rx_dma_release:
+	dma_release_channel(sspi->rx_dma_chan);
+err_tx_dma_release:
+	dma_release_channel(sspi->tx_dma_chan);
  err_free_master:
  	spi_master_put(master);
  	return ret;

@@ -443,8 +567,17 @@ err_free_master:
 

drivers/spi/spi-sun4i.c err_free_master: · line 575

Quoted 8 lines

drivers/spi/spi-sun4i.c +6 -0

  static int sun4i_spi_remove(struct platform_device *pdev)
  {
+	struct spi_master *master = platform_get_drvdata(pdev);
+	struct sun4i_spi *sspi = spi_master_get_devdata(master);
+
+	if (pm_runtime_active(&pdev->dev))
+		sun4i_spi_runtime_suspend(&pdev->dev);
+

This doesn't really belong in this patch, please send a separate one.

Let's add a node to represent the A10 DMA controller on the device tree.

Signed-off-by Emilio L?pez <[email protected]>

Reviewed-by Chen-Yu Tsai <[email protected]>