diff mbox

[V2,6/8] dmaengine: bcm2835: move controlblock chain generation into separate method

Message ID 1452187987-2605-7-git-send-email-kernel@martin.sperl.org (mailing list archive)
State New, archived
Headers show

Commit Message

Martin Sperl Jan. 7, 2016, 5:33 p.m. UTC 
From: Martin Sperl <kernel@martin.sperl.org>

In preparation of adding slave_sg functionality this patch moves the
generation/allocation of bcm2835_desc and the building of
the corresponding DMA-control-block chain from bcm2835_dma_prep_dma_cyclic
into the newly created method bcm2835_dma_create_cb_chain.

This new code also takes the limits of LITE channels into account
and splits the control-block transfers at the correct location.

Tested Audio output with a Hifiberry I2S card.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/dma/bcm2835-dma.c |  288 ++++++++++++++++++++++++++++++---------------
 1 file changed, 191 insertions(+), 97 deletions(-)

--
1.7.10.4

Comments

Vinod Koul Jan. 13, 2016, 1:23 p.m. UTC | #1 
On Thu, Jan 07, 2016 at 05:33:04PM +0000, kernel@martin.sperl.org wrote:

> +static inline bool bcm2835_dma_is_lite(struct bcm2835_chan *c)
> +{
> +	/* dma channels >= 7 are LITE channels */
> +	return (c->ch >= 7);

Why not DT data here as well

> +}
> +
> +static inline size_t bcm2835_dma_max_frame_length(struct bcm2835_chan *c)
> +{
> +	/* lite and normal channels have different max frame length */
> +	return bcm2835_dma_is_lite(c) ? MAX_LITE_DMA_LEN : MAX_DMA_LEN;

Or rather get length from DT..

> +static struct bcm2835_desc *bcm2835_dma_create_cb_chain(
> +	struct dma_chan *chan, enum dma_transfer_direction direction,
> +	bool cyclic, u32 info, u32 finalextrainfo, size_t frames,
> +	dma_addr_t src, dma_addr_t dst, size_t buf_len,
> +	size_t period_len, gfp_t gfp)
> +{
> +	struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
> +	size_t len = buf_len, total_len;
> +	size_t frame;
> +	struct bcm2835_desc *d;
> +	struct bcm2835_cb_entry *cb_entry;
> +	struct bcm2835_dma_cb *control_block;
> +	size_t max_len = bcm2835_dma_max_frame_length(c);
> +
> +	/* allocate and setup the descriptor. */
> +	d = kzalloc(sizeof(*d) + frames * sizeof(struct bcm2835_cb_entry),
> +		    gfp);

odd style.. btw should flag be GFP_NOWAIT ..?

> +		/* fill in the control block */
> +		control_block = cb_entry->cb;
> +		control_block->info = info;
> +		control_block->src = src;
> +		control_block->dst = dst;
> +		if (buf_len) {
> +			control_block->length = min(max_len, len);
> +			if (period_len &&
> +			    (total_len + control_block->length >=
> +			     period_len)) {
> +				/* set to end of period_len */
> +				control_block->length =
> +					period_len - total_len;
> +				/* add extrainfo when cyclic */
> +				if (cyclic)
> +					control_block->info |=
> +						finalextrainfo;
> +				/* and reset total_len */
> +				total_len = 0;
> +			}

this looks hard to read, perhpas a helper will make it look better

> +	/* the last frame requires extra flags */
> +	d->cb_list[d->frames - 1].cb->info |= finalextrainfo;
> +
> +	/* check the size - if there is some missmatch,
> +	 * then this is detected here
> +	 */

this is not kernel style for multi-line comments

>  	/* Grab configuration */
>  	if (!is_slave_direction(direction)) {
> -		dev_err(chan->device->dev, "%s: bad direction?\n", __func__);
> +		dev_err(chan->device->dev,
> +			"%s: bad direction?\n", __func__);

unrelated change

> -	/* Now allocate and setup the descriptor. */
> -	d = kzalloc(sizeof(*d), GFP_NOWAIT);
> -	if (!d)
> -		return NULL;
> +	/* warn if buf_len is not a multiple of period_lenas this may leed
> +	 * to unexpected latencies for interrupts and thus audiable clicks
> +	 */

here too

>  	/*
> -	 * Iterate over all frames, create a control block
> -	 * for each frame and link them together.
> +	 * allocate the CB chain
> +	 * note that we need to use GFP_ATOMIC, as the ALSA i2s dmaengine

dmaengine drivers use GFP_NOWAIT in these cases
Martin Sperl Jan. 13, 2016, 1:38 p.m. UTC | #2 
On 13.01.2016 14:23, Vinod Koul wrote:
> On Thu, Jan 07, 2016 at 05:33:04PM +0000, kernel@martin.sperl.org wrote:
>
>> +static inline bool bcm2835_dma_is_lite(struct bcm2835_chan *c)
>> +{
>> +	/* dma channels >= 7 are LITE channels */
>> +	return (c->ch >= 7);
>
> Why not DT data here as well

See other email with comments on DT.

>
>> +}
>> +
>> +static inline size_t bcm2835_dma_max_frame_length(struct bcm2835_chan *c)
>> +{
>> +	/* lite and normal channels have different max frame length */
>> +	return bcm2835_dma_is_lite(c) ? MAX_LITE_DMA_LEN : MAX_DMA_LEN;
>
> Or rather get length from DT..
See other email with comments on DT.

>> +	/* allocate and setup the descriptor. */
>> +	d = kzalloc(sizeof(*d) + frames * sizeof(struct bcm2835_cb_entry),
>> +		    gfp);
>
> odd style.. btw should flag be GFP_NOWAIT ..?
gfp was used as a method-argument - see your comment below.

>
>> +		/* fill in the control block */
>> +		control_block = cb_entry->cb;
>> +		control_block->info = info;
>> +		control_block->src = src;
>> +		control_block->dst = dst;
>> +		if (buf_len) {
>> +			control_block->length = min(max_len, len);
>> +			if (period_len &&
>> +			    (total_len + control_block->length >=
>> +			     period_len)) {
>> +				/* set to end of period_len */
>> +				control_block->length =
>> +					period_len - total_len;
>> +				/* add extrainfo when cyclic */
>> +				if (cyclic)
>> +					control_block->info |=
>> +						finalextrainfo;
>> +				/* and reset total_len */
>> +				total_len = 0;
>> +			}
>
> this looks hard to read, perhpas a helper will make it look better
Perhaps - I can look...
>
>> +	/* the last frame requires extra flags */
>> +	d->cb_list[d->frames - 1].cb->info |= finalextrainfo;
>> +
>> +	/* check the size - if there is some missmatch,
>> +	 * then this is detected here
>> +	 */
>
> this is not kernel style for multi-line comments
I know - this came up with a different patch-set

>
>>   	/* Grab configuration */
>>   	if (!is_slave_direction(direction)) {
>> -		dev_err(chan->device->dev, "%s: bad direction?\n", __func__);
>> +		dev_err(chan->device->dev,
>> +			"%s: bad direction?\n", __func__);
>
> unrelated change
OK

>
>> -	/* Now allocate and setup the descriptor. */
>> -	d = kzalloc(sizeof(*d), GFP_NOWAIT);
>> -	if (!d)
>> -		return NULL;
>> +	/* warn if buf_len is not a multiple of period_lenas this may leed
>> +	 * to unexpected latencies for interrupts and thus audiable clicks
>> +	 */
>
> here too

as this code is new and there are a few more issues that have been seen
it was included here...

>
>>   	/*
>> -	 * Iterate over all frames, create a control block
>> -	 * for each frame and link them together.
>> +	 * allocate the CB chain
>> +	 * note that we need to use GFP_ATOMIC, as the ALSA i2s dmaengine
>
> dmaengine drivers use GFP_NOWAIT in these cases
OK we were not sure if this was "permissible" or would result in rare
issues...

I will wait on the generic DT discussion before sending any updates
incorporating both...
Eric Anholt Feb. 18, 2016, 3:24 a.m. UTC | #3 
kernel@martin.sperl.org writes:

> From: Martin Sperl <kernel@martin.sperl.org>
>
> In preparation of adding slave_sg functionality this patch moves the
> generation/allocation of bcm2835_desc and the building of
> the corresponding DMA-control-block chain from bcm2835_dma_prep_dma_cyclic
> into the newly created method bcm2835_dma_create_cb_chain.
>
> This new code also takes the limits of LITE channels into account
> and splits the control-block transfers at the correct location.
>
> Tested Audio output with a Hifiberry I2S card.
>
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
> ---
>  drivers/dma/bcm2835-dma.c |  288 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 191 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
> index 43758ba..41a4f0b 100644
> --- a/drivers/dma/bcm2835-dma.c
> +++ b/drivers/dma/bcm2835-dma.c
> @@ -90,12 +90,12 @@ struct bcm2835_desc {
>  	struct virt_dma_desc vd;
>  	enum dma_transfer_direction dir;
>
> -	struct bcm2835_cb_entry *cb_list;
> -
>  	unsigned int frames;
>  	size_t size;
>
>  	bool cyclic;
> +
> +	struct bcm2835_cb_entry cb_list[];
>  };
>
>  #define BCM2835_DMA_CS		0x00
> @@ -181,6 +181,22 @@ struct bcm2835_desc {
>  #define BCM2835_DMA_IRQ_SHARED		11
>  #define BCM2835_DMA_IRQ_ALL		12
>
> +/* the max dma length for different channels */
> +#define MAX_DMA_LEN SZ_1G
> +#define MAX_LITE_DMA_LEN (SZ_64K - 4)
> +
> +static inline bool bcm2835_dma_is_lite(struct bcm2835_chan *c)
> +{
> +	/* dma channels >= 7 are LITE channels */
> +	return (c->ch >= 7);
> +}

You can ask a channel if it's a LITE engine by checking if the DEBUG reg
has bit 28 set.  Then you don't need to have the software/DT guessing
about it.

However, I'm disappointed to see these changes for adding LITE support
included in the same commit as refactoring chain generation into a
separate function.
Martin Sperl Feb. 29, 2016, 6:14 p.m. UTC | #4 
> On 18.02.2016, at 04:24, Eric Anholt <eric@anholt.net> wrote:
> 
> kernel@martin.sperl.org writes:
> 
>> From: Martin Sperl <kernel@martin.sperl.org>
>> 
>> In preparation of adding slave_sg functionality this patch moves the
>> generation/allocation of bcm2835_desc and the building of
>> the corresponding DMA-control-block chain from bcm2835_dma_prep_dma_cyclic
>> into the newly created method bcm2835_dma_create_cb_chain.
>> 
>> This new code also takes the limits of LITE channels into account
>> and splits the control-block transfers at the correct location.
>> 
>> Tested Audio output with a Hifiberry I2S card.
>> 
>> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
>> ---
>> drivers/dma/bcm2835-dma.c |  288 ++++++++++++++++++++++++++++++---------------
>> 1 file changed, 191 insertions(+), 97 deletions(-)
>> 
>> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
>> index 43758ba..41a4f0b 100644
>> --- a/drivers/dma/bcm2835-dma.c
>> +++ b/drivers/dma/bcm2835-dma.c
>> @@ -90,12 +90,12 @@ struct bcm2835_desc {
>> 	struct virt_dma_desc vd;
>> 	enum dma_transfer_direction dir;
>> 
>> -	struct bcm2835_cb_entry *cb_list;
>> -
>> 	unsigned int frames;
>> 	size_t size;
>> 
>> 	bool cyclic;
>> +
>> +	struct bcm2835_cb_entry cb_list[];
>> };
>> 
>> #define BCM2835_DMA_CS		0x00
>> @@ -181,6 +181,22 @@ struct bcm2835_desc {
>> #define BCM2835_DMA_IRQ_SHARED		11
>> #define BCM2835_DMA_IRQ_ALL		12
>> 
>> +/* the max dma length for different channels */
>> +#define MAX_DMA_LEN SZ_1G
>> +#define MAX_LITE_DMA_LEN (SZ_64K - 4)
>> +
>> +static inline bool bcm2835_dma_is_lite(struct bcm2835_chan *c)
>> +{
>> +	/* dma channels >= 7 are LITE channels */
>> +	return (c->ch >= 7);
>> +}
> 
> You can ask a channel if it's a LITE engine by checking if the DEBUG reg
> has bit 28 set.  Then you don't need to have the software/DT guessing
> about it.
I will implement that - it is simple.

> However, I'm disappointed to see these changes for adding LITE support
> included in the same commit as refactoring chain generation into a
> separate function.
I will separate this “LITE-channel handling” out into a separate patch.

Martin
diff mbox

Patch

diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index 43758ba..41a4f0b 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -90,12 +90,12 @@  struct bcm2835_desc {
 	struct virt_dma_desc vd;
 	enum dma_transfer_direction dir;

-	struct bcm2835_cb_entry *cb_list;
-
 	unsigned int frames;
 	size_t size;

 	bool cyclic;
+
+	struct bcm2835_cb_entry cb_list[];
 };

 #define BCM2835_DMA_CS		0x00
@@ -181,6 +181,22 @@  struct bcm2835_desc {
 #define BCM2835_DMA_IRQ_SHARED		11
 #define BCM2835_DMA_IRQ_ALL		12

+/* the max dma length for different channels */
+#define MAX_DMA_LEN SZ_1G
+#define MAX_LITE_DMA_LEN (SZ_64K - 4)
+
+static inline bool bcm2835_dma_is_lite(struct bcm2835_chan *c)
+{
+	/* dma channels >= 7 are LITE channels */
+	return (c->ch >= 7);
+}
+
+static inline size_t bcm2835_dma_max_frame_length(struct bcm2835_chan *c)
+{
+	/* lite and normal channels have different max frame length */
+	return bcm2835_dma_is_lite(c) ? MAX_LITE_DMA_LEN : MAX_DMA_LEN;
+}
+
 static inline struct bcm2835_dmadev *to_bcm2835_dma_dev(struct dma_device *d)
 {
 	return container_of(d, struct bcm2835_dmadev, ddev);
@@ -197,19 +213,140 @@  static inline struct bcm2835_desc *to_bcm2835_dma_desc(
 	return container_of(t, struct bcm2835_desc, vd.tx);
 }

-static void bcm2835_dma_desc_free(struct virt_dma_desc *vd)
+static void bcm2835_dma_free_cb_chain(struct bcm2835_desc *desc)
 {
-	struct bcm2835_desc *desc = container_of(vd, struct bcm2835_desc, vd);
-	int i;
+	size_t i;

 	for (i = 0; i < desc->frames; i++)
 		dma_pool_free(desc->c->cb_pool, desc->cb_list[i].cb,
 			      desc->cb_list[i].paddr);

-	kfree(desc->cb_list);
 	kfree(desc);
 }

+static void bcm2835_dma_desc_free(struct virt_dma_desc *vd)
+{
+	bcm2835_dma_free_cb_chain(
+		container_of(vd, struct bcm2835_desc, vd));
+}
+
+static inline size_t bcm2835_dma_frames_for_length(size_t len,
+						   size_t period_len)
+{
+	return DIV_ROUND_UP(len, period_len);
+}
+
+/**
+ * bcm2835_dma_create_cb_chain - create a control block and fills data in
+ *
+ * @chan:           the @dma_chan for which we run this
+ * @direction:      the direction in which we transfer
+ * @cyclic:         it is a cyclic transfer
+ * @info:           the default info bits to apply per controlblock
+ * @finalextrainfo: additional bits in last controlblock
+ *                  (or when period_len is reached in case of cyclic)
+ * @frames:         number of controlblocks to allocate
+ * @src:            the src address to assign (if the S_INC bit is set
+ *                  in @info, then it gets incremented)
+ * @dst:            the dst address to assign (if the D_INC bit is set
+ *                  in @info, then it gets incremented)
+ * @buf_len:        the full buffer length (may also be 0)
+ * @period_len:     the period length when to apply @finalextrainfo
+ *                  in addition to the last transfer
+ *                  this will also break some control-blocks early
+ * @gfp:            the GFP flag to use for allocation
+ */
+static struct bcm2835_desc *bcm2835_dma_create_cb_chain(
+	struct dma_chan *chan, enum dma_transfer_direction direction,
+	bool cyclic, u32 info, u32 finalextrainfo, size_t frames,
+	dma_addr_t src, dma_addr_t dst, size_t buf_len,
+	size_t period_len, gfp_t gfp)
+{
+	struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
+	size_t len = buf_len, total_len;
+	size_t frame;
+	struct bcm2835_desc *d;
+	struct bcm2835_cb_entry *cb_entry;
+	struct bcm2835_dma_cb *control_block;
+	size_t max_len = bcm2835_dma_max_frame_length(c);
+
+	/* allocate and setup the descriptor. */
+	d = kzalloc(sizeof(*d) + frames * sizeof(struct bcm2835_cb_entry),
+		    gfp);
+	if (!d)
+		return NULL;
+
+	d->c = c;
+	d->dir = direction;
+	d->cyclic = cyclic;
+
+	/*
+	 * Iterate over all frames, create a control block
+	 * for each frame and link them together.
+	 */
+	for (frame = 0, total_len = 0; frame < frames; d->frames++, frame++) {
+		cb_entry = &d->cb_list[frame];
+		cb_entry->cb = dma_pool_alloc(c->cb_pool, gfp,
+					      &cb_entry->paddr);
+		if (!cb_entry->cb)
+			goto error_cb;
+
+		/* fill in the control block */
+		control_block = cb_entry->cb;
+		control_block->info = info;
+		control_block->src = src;
+		control_block->dst = dst;
+		if (buf_len) {
+			control_block->length = min(max_len, len);
+			if (period_len &&
+			    (total_len + control_block->length >=
+			     period_len)) {
+				/* set to end of period_len */
+				control_block->length =
+					period_len - total_len;
+				/* add extrainfo when cyclic */
+				if (cyclic)
+					control_block->info |=
+						finalextrainfo;
+				/* and reset total_len */
+				total_len = 0;
+			}
+		}
+		control_block->stride = 0;
+		control_block->next = 0;
+
+		/* link this the last controlblock */
+		if (frame)
+			d->cb_list[frame - 1].cb->next = cb_entry->paddr;
+
+		/* update src and dst and length */
+		if (buf_len)
+			len -= control_block->length;
+		if (src && (info & BCM2835_DMA_S_INC))
+			src += control_block->length;
+		if (dst && (info & BCM2835_DMA_D_INC))
+			dst += control_block->length;
+
+		/* Length of total transfer */
+		d->size += control_block->length;
+	}
+
+	/* the last frame requires extra flags */
+	d->cb_list[d->frames - 1].cb->info |= finalextrainfo;
+
+	/* check the size - if there is some missmatch,
+	 * then this is detected here
+	 */
+	if (buf_len && (d->size != buf_len))
+		goto error_cb;
+
+	return d;
+error_cb:
+	bcm2835_dma_free_cb_chain(d);
+
+	return NULL;
+}
+
 static int bcm2835_dma_abort(void __iomem *chan_base)
 {
 	unsigned long cs;
@@ -413,117 +550,74 @@  static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic(
 	unsigned long flags)
 {
 	struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
-	enum dma_slave_buswidth dev_width;
 	struct bcm2835_desc *d;
-	dma_addr_t dev_addr;
-	unsigned int es, sync_type;
-	unsigned int frame;
-	int i;
+	dma_addr_t src, dst;
+	u32 info = BCM2835_DMA_WAIT_RESP;
+	u32 extra = BCM2835_DMA_INT_EN;
+	size_t max_len = bcm2835_dma_max_frame_length(c);
+	size_t frames;

 	/* Grab configuration */
 	if (!is_slave_direction(direction)) {
-		dev_err(chan->device->dev, "%s: bad direction?\n", __func__);
+		dev_err(chan->device->dev,
+			"%s: bad direction?\n", __func__);
 		return NULL;
 	}

-	if (direction == DMA_DEV_TO_MEM) {
-		dev_addr = c->cfg.src_addr;
-		dev_width = c->cfg.src_addr_width;
-		sync_type = BCM2835_DMA_S_DREQ;
-	} else {
-		dev_addr = c->cfg.dst_addr;
-		dev_width = c->cfg.dst_addr_width;
-		sync_type = BCM2835_DMA_D_DREQ;
-	}
-
-	/* Bus width translates to the element size (ES) */
-	switch (dev_width) {
-	case DMA_SLAVE_BUSWIDTH_4_BYTES:
-		es = BCM2835_DMA_DATA_TYPE_S32;
-		break;
-	default:
+	if (!buf_len) {
+		dev_err(chan->device->dev,
+			"%s: bad buffer length (= 0)\n", __func__);
 		return NULL;
 	}

-	/* Now allocate and setup the descriptor. */
-	d = kzalloc(sizeof(*d), GFP_NOWAIT);
-	if (!d)
-		return NULL;
+	/* warn if buf_len is not a multiple of period_lenas this may leed
+	 * to unexpected latencies for interrupts and thus audiable clicks
+	 */
+	if (buf_len % period_len)
+		dev_warn(chan->device->dev,
+			 "%s: buffer_length (%zd) is not a multiple of period_len (%zd)\n",
+			 __func__, buf_len, period_len);

-	d->c = c;
-	d->dir = direction;
-	d->frames = buf_len / period_len;
-	d->cyclic = true;
+	/* Setup DREQ channel */
+	if (c->dreq != 0)
+		info |= BCM2835_DMA_PER_MAP(c->dreq);

-	d->cb_list = kcalloc(d->frames, sizeof(*d->cb_list), GFP_KERNEL);
-	if (!d->cb_list) {
-		kfree(d);
-		return NULL;
+	if (direction == DMA_DEV_TO_MEM) {
+		if (c->cfg.src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES)
+			return NULL;
+		src = c->cfg.src_addr;
+		dst = buf_addr;
+		info |= BCM2835_DMA_S_DREQ | BCM2835_DMA_D_INC;
+	} else {
+		if (c->cfg.dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES)
+			return NULL;
+		dst = c->cfg.dst_addr;
+		src = buf_addr;
+		info |= BCM2835_DMA_D_DREQ | BCM2835_DMA_S_INC;
 	}
-	/* Allocate memory for control blocks */
-	for (i = 0; i < d->frames; i++) {
-		struct bcm2835_cb_entry *cb_entry = &d->cb_list[i];

-		cb_entry->cb = dma_pool_zalloc(c->cb_pool, GFP_ATOMIC,
-					       &cb_entry->paddr);
-		if (!cb_entry->cb)
-			goto error_cb;
-	}
+	/* calculate number of frames */
+	frames = /* number of periods */
+		 DIV_ROUND_UP(buf_len, period_len) *
+		 /* number of frames per period */
+		 bcm2835_dma_frames_for_length(period_len, max_len);

 	/*
-	 * Iterate over all frames, create a control block
-	 * for each frame and link them together.
+	 * allocate the CB chain
+	 * note that we need to use GFP_ATOMIC, as the ALSA i2s dmaengine
+	 * implementation calls prep_dma_cyclic with interrupts disabled.
 	 */
-	for (frame = 0; frame < d->frames; frame++) {
-		struct bcm2835_dma_cb *control_block = d->cb_list[frame].cb;
-
-		/* Setup adresses */
-		if (d->dir == DMA_DEV_TO_MEM) {
-			control_block->info = BCM2835_DMA_D_INC;
-			control_block->src = dev_addr;
-			control_block->dst = buf_addr + frame * period_len;
-		} else {
-			control_block->info = BCM2835_DMA_S_INC;
-			control_block->src = buf_addr + frame * period_len;
-			control_block->dst = dev_addr;
-		}
-
-		/* Enable interrupt */
-		control_block->info |= BCM2835_DMA_INT_EN;
-
-		/* Setup synchronization */
-		if (sync_type != 0)
-			control_block->info |= sync_type;
-
-		/* Setup DREQ channel */
-		if (c->dreq != 0)
-			control_block->info |=
-				BCM2835_DMA_PER_MAP(c->dreq);
-
-		/* Length of a frame */
-		control_block->length = period_len;
-		d->size += control_block->length;
+	d = bcm2835_dma_create_cb_chain(chan, direction, true,
+					info, extra,
+					frames, src, dst, buf_len,
+					period_len, GFP_ATOMIC);
+	if (!d)
+		return NULL;

-		/*
-		 * Next block is the next frame.
-		 * This DMA engine driver currently only supports cyclic DMA.
-		 * Therefore, wrap around at number of frames.
-		 */
-		control_block->next = d->cb_list[((frame + 1) % d->frames)].paddr;
-	}
+	/* wrap around into a loop */
+	d->cb_list[d->frames - 1].cb->next = d->cb_list[0].paddr;

 	return vchan_tx_prep(&c->vc, &d->vd, flags);
-error_cb:
-	i--;
-	for (; i >= 0; i--) {
-		struct bcm2835_cb_entry *cb_entry = &d->cb_list[i];
-
-		dma_pool_free(c->cb_pool, cb_entry->cb, cb_entry->paddr);
-	}
-
-	kfree(d->cb_list);
-	kfree(d);
-	return NULL;
 }

 static int bcm2835_dma_slave_config(struct dma_chan *chan,