[PATCH] dmaengine: bcm2835-dma: Convert to use DMA pool

[PATCH] dmaengine: bcm2835-dma: Convert to use DMA pool

[Peter Ujfalusi]

f93178291712 dmaengine: bcm2835-dma: Fix memory leak when stopping a
	     running transfer

Fixed the memleak, but introduced another issue: the terminate_all callback
might be called with interrupts disabled and the dma_free_coherent() is
not allowed to be called when IRQs are disabled.
Convert the driver to use dma_pool_* for managing the list of control
blocks for the transfer.

Fixes: f93178291712 ("dmaengine: bcm2835-dma: Fix memory leak when stopping a running transfer")
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
Hi,

It was brought to my attention that the memleak fix broke the bcm2835 DMA. I did
not noticed the use of dma_free_coherent() in the driver when I did the memleak
fix.
Since the driver does leaking memory every time the audio is stopped, the other
option is to convert it to use DMA pool.
I do not have access the Raspberry Pi, so I can not test this patch but it
compiles ;)
Can someone test this one outif it is working?

Regards,
Peter

 drivers/dma/bcm2835-dma.c | 78 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 54 insertions(+), 24 deletions(-)

diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index c92d6a70ccf3..996c4b00d323 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -31,6 +31,7 @@
  */
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
+#include <linux/dmapool.h>
 #include <linux/err.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
@@ -62,6 +63,11 @@ struct bcm2835_dma_cb {
 	uint32_t pad[2];
 };
 
+struct bcm2835_cb_entry {
+	struct bcm2835_dma_cb *cb;
+	dma_addr_t paddr;
+};
+
 struct bcm2835_chan {
 	struct virt_dma_chan vc;
 	struct list_head node;
@@ -72,18 +78,18 @@ struct bcm2835_chan {
 
 	int ch;
 	struct bcm2835_desc *desc;
+	struct dma_pool *cb_pool;
 
 	void __iomem *chan_base;
 	int irq_number;
 };
 
 struct bcm2835_desc {
+	struct bcm2835_chan *c;
 	struct virt_dma_desc vd;
 	enum dma_transfer_direction dir;
 
-	unsigned int control_block_size;
-	struct bcm2835_dma_cb *control_block_base;
-	dma_addr_t control_block_base_phys;
+	struct bcm2835_cb_entry *cb_list;
 
 	unsigned int frames;
 	size_t size;
@@ -143,10 +149,13 @@ static inline struct bcm2835_desc *to_bcm2835_dma_desc(
 static void bcm2835_dma_desc_free(struct virt_dma_desc *vd)
 {
 	struct bcm2835_desc *desc = container_of(vd, struct bcm2835_desc, vd);
-	dma_free_coherent(desc->vd.tx.chan->device->dev,
-			desc->control_block_size,
-			desc->control_block_base,
-			desc->control_block_base_phys);
+	int 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);
 }
 
@@ -199,7 +208,7 @@ static void bcm2835_dma_start_desc(struct bcm2835_chan *c)
 
 	c->desc = d = to_bcm2835_dma_desc(&vd->tx);
 
-	writel(d->control_block_base_phys, c->chan_base + BCM2835_DMA_ADDR);
+	writel(d->cb_list[0].paddr, c->chan_base + BCM2835_DMA_ADDR);
 	writel(BCM2835_DMA_ACTIVE, c->chan_base + BCM2835_DMA_CS);
 }
 
@@ -232,9 +241,16 @@ static irqreturn_t bcm2835_dma_callback(int irq, void *data)
 static int bcm2835_dma_alloc_chan_resources(struct dma_chan *chan)
 {
 	struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
+	struct device *dev = c->vc.chan.device->dev;
+
+	dev_dbg(dev, "Allocating DMA channel %d\n", c->ch);
 
-	dev_dbg(c->vc.chan.device->dev,
-			"Allocating DMA channel %d\n", c->ch);
+	c->cb_pool = dma_pool_create(dev_name(dev), dev,
+				     sizeof(struct bcm2835_dma_cb), 0, 0);
+	if (!c->cb_pool) {
+		dev_err(dev, "unable to allocate descriptor pool\n");
+		return -ENOMEM;
+	}
 
 	return request_irq(c->irq_number,
 			bcm2835_dma_callback, 0, "DMA IRQ", c);
@@ -246,6 +262,7 @@ static void bcm2835_dma_free_chan_resources(struct dma_chan *chan)
 
 	vchan_free_chan_resources(&c->vc);
 	free_irq(c->irq_number, c);
+	dma_pool_destroy(c->cb_pool);
 
 	dev_dbg(c->vc.chan.device->dev, "Freeing DMA channel %u\n", c->ch);
 }
@@ -261,8 +278,7 @@ static size_t bcm2835_dma_desc_size_pos(struct bcm2835_desc *d, dma_addr_t addr)
 	size_t size;
 
 	for (size = i = 0; i < d->frames; i++) {
-		struct bcm2835_dma_cb *control_block =
-			&d->control_block_base[i];
+		struct bcm2835_dma_cb *control_block = d->cb_list[i].cb;
 		size_t this_size = control_block->length;
 		dma_addr_t dma;
 
@@ -343,6 +359,7 @@ static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic(
 	dma_addr_t dev_addr;
 	unsigned int es, sync_type;
 	unsigned int frame;
+	int i;
 
 	/* Grab configuration */
 	if (!is_slave_direction(direction)) {
@@ -374,27 +391,31 @@ static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic(
 	if (!d)
 		return NULL;
 
+	d->c = c;
 	d->dir = direction;
 	d->frames = buf_len / period_len;
 
-	/* Allocate memory for control blocks */
-	d->control_block_size = d->frames * sizeof(struct bcm2835_dma_cb);
-	d->control_block_base = dma_zalloc_coherent(chan->device->dev,
-			d->control_block_size, &d->control_block_base_phys,
-			GFP_NOWAIT);
-
-	if (!d->control_block_base) {
+	d->cb_list = kcalloc(d->frames, sizeof(*d->cb_list), GFP_KERNEL);
+	if (!d->cb_list) {
 		kfree(d);
 		return NULL;
 	}
+	/* 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;
+	}
 
 	/*
 	 * Iterate over all frames, create a control block
 	 * for each frame and link them together.
 	 */
 	for (frame = 0; frame < d->frames; frame++) {
-		struct bcm2835_dma_cb *control_block =
-			&d->control_block_base[frame];
+		struct bcm2835_dma_cb *control_block = d->cb_list[frame].cb;
 
 		/* Setup adresses */
 		if (d->dir == DMA_DEV_TO_MEM) {
@@ -428,12 +449,21 @@ static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic(
 		 * This DMA engine driver currently only supports cyclic DMA.
 		 * Therefore, wrap around at number of frames.
 		 */
-		control_block->next = d->control_block_base_phys +
-			sizeof(struct bcm2835_dma_cb)
-			* ((frame + 1) % d->frames);
+		control_block->next = d->cb_list[((frame + 1) % d->frames)].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,
-- 
2.6.2

[PATCH] dmaengine: bcm2835-dma: Convert to use DMA pool

[Peter Ujfalusi]

Hi,

On 11/16/2015 01:09 PM, Peter Ujfalusi wrote:
> f93178291712 dmaengine: bcm2835-dma: Fix memory leak when stopping a
> 	     running transfer
> 
> Fixed the memleak, but introduced another issue: the terminate_all callback
> might be called with interrupts disabled and the dma_free_coherent() is
> not allowed to be called when IRQs are disabled.
> Convert the driver to use dma_pool_* for managing the list of control
> blocks for the transfer.

FWIW: the patch has been tested and verified on Raspbery Pi:
https://github.com/raspberrypi/linux/pull/1178#issuecomment-157026794
https://github.com/raspberrypi/linux/pull/1178#issuecomment-157030190

It needed some modification since the Raspberry Pi kernel have non upstreamed
changes in bcm2835-dma driver (slave_sg support for example).

It would be great if this patch can make it to 4.4 as a fix.

Thanks,
P?ter

> 
> Fixes: f93178291712 ("dmaengine: bcm2835-dma: Fix memory leak when stopping a running transfer")
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
> Hi,
> 
> It was brought to my attention that the memleak fix broke the bcm2835 DMA. I did
> not noticed the use of dma_free_coherent() in the driver when I did the memleak
> fix.
> Since the driver does leaking memory every time the audio is stopped, the other
> option is to convert it to use DMA pool.
> I do not have access the Raspberry Pi, so I can not test this patch but it
> compiles ;)
> Can someone test this one out if it is working?
> 
> Regards,
> Peter
> 
>  drivers/dma/bcm2835-dma.c | 78 ++++++++++++++++++++++++++++++++---------------
>  1 file changed, 54 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
> index c92d6a70ccf3..996c4b00d323 100644
> --- a/drivers/dma/bcm2835-dma.c
> +++ b/drivers/dma/bcm2835-dma.c
> @@ -31,6 +31,7 @@
>   */
>  #include <linux/dmaengine.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/dmapool.h>
>  #include <linux/err.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
> @@ -62,6 +63,11 @@ struct bcm2835_dma_cb {
>  	uint32_t pad[2];
>  };
>  
> +struct bcm2835_cb_entry {
> +	struct bcm2835_dma_cb *cb;
> +	dma_addr_t paddr;
> +};
> +
>  struct bcm2835_chan {
>  	struct virt_dma_chan vc;
>  	struct list_head node;
> @@ -72,18 +78,18 @@ struct bcm2835_chan {
>  
>  	int ch;
>  	struct bcm2835_desc *desc;
> +	struct dma_pool *cb_pool;
>  
>  	void __iomem *chan_base;
>  	int irq_number;
>  };
>  
>  struct bcm2835_desc {
> +	struct bcm2835_chan *c;
>  	struct virt_dma_desc vd;
>  	enum dma_transfer_direction dir;
>  
> -	unsigned int control_block_size;
> -	struct bcm2835_dma_cb *control_block_base;
> -	dma_addr_t control_block_base_phys;
> +	struct bcm2835_cb_entry *cb_list;
>  
>  	unsigned int frames;
>  	size_t size;
> @@ -143,10 +149,13 @@ static inline struct bcm2835_desc *to_bcm2835_dma_desc(
>  static void bcm2835_dma_desc_free(struct virt_dma_desc *vd)
>  {
>  	struct bcm2835_desc *desc = container_of(vd, struct bcm2835_desc, vd);
> -	dma_free_coherent(desc->vd.tx.chan->device->dev,
> -			desc->control_block_size,
> -			desc->control_block_base,
> -			desc->control_block_base_phys);
> +	int 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);
>  }
>  
> @@ -199,7 +208,7 @@ static void bcm2835_dma_start_desc(struct bcm2835_chan *c)
>  
>  	c->desc = d = to_bcm2835_dma_desc(&vd->tx);
>  
> -	writel(d->control_block_base_phys, c->chan_base + BCM2835_DMA_ADDR);
> +	writel(d->cb_list[0].paddr, c->chan_base + BCM2835_DMA_ADDR);
>  	writel(BCM2835_DMA_ACTIVE, c->chan_base + BCM2835_DMA_CS);
>  }
>  
> @@ -232,9 +241,16 @@ static irqreturn_t bcm2835_dma_callback(int irq, void *data)
>  static int bcm2835_dma_alloc_chan_resources(struct dma_chan *chan)
>  {
>  	struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
> +	struct device *dev = c->vc.chan.device->dev;
> +
> +	dev_dbg(dev, "Allocating DMA channel %d\n", c->ch);
>  
> -	dev_dbg(c->vc.chan.device->dev,
> -			"Allocating DMA channel %d\n", c->ch);
> +	c->cb_pool = dma_pool_create(dev_name(dev), dev,
> +				     sizeof(struct bcm2835_dma_cb), 0, 0);
> +	if (!c->cb_pool) {
> +		dev_err(dev, "unable to allocate descriptor pool\n");
> +		return -ENOMEM;
> +	}
>  
>  	return request_irq(c->irq_number,
>  			bcm2835_dma_callback, 0, "DMA IRQ", c);
> @@ -246,6 +262,7 @@ static void bcm2835_dma_free_chan_resources(struct dma_chan *chan)
>  
>  	vchan_free_chan_resources(&c->vc);
>  	free_irq(c->irq_number, c);
> +	dma_pool_destroy(c->cb_pool);
>  
>  	dev_dbg(c->vc.chan.device->dev, "Freeing DMA channel %u\n", c->ch);
>  }
> @@ -261,8 +278,7 @@ static size_t bcm2835_dma_desc_size_pos(struct bcm2835_desc *d, dma_addr_t addr)
>  	size_t size;
>  
>  	for (size = i = 0; i < d->frames; i++) {
> -		struct bcm2835_dma_cb *control_block =
> -			&d->control_block_base[i];
> +		struct bcm2835_dma_cb *control_block = d->cb_list[i].cb;
>  		size_t this_size = control_block->length;
>  		dma_addr_t dma;
>  
> @@ -343,6 +359,7 @@ static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic(
>  	dma_addr_t dev_addr;
>  	unsigned int es, sync_type;
>  	unsigned int frame;
> +	int i;
>  
>  	/* Grab configuration */
>  	if (!is_slave_direction(direction)) {
> @@ -374,27 +391,31 @@ static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic(
>  	if (!d)
>  		return NULL;
>  
> +	d->c = c;
>  	d->dir = direction;
>  	d->frames = buf_len / period_len;
>  
> -	/* Allocate memory for control blocks */
> -	d->control_block_size = d->frames * sizeof(struct bcm2835_dma_cb);
> -	d->control_block_base = dma_zalloc_coherent(chan->device->dev,
> -			d->control_block_size, &d->control_block_base_phys,
> -			GFP_NOWAIT);
> -
> -	if (!d->control_block_base) {
> +	d->cb_list = kcalloc(d->frames, sizeof(*d->cb_list), GFP_KERNEL);
> +	if (!d->cb_list) {
>  		kfree(d);
>  		return NULL;
>  	}
> +	/* 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;
> +	}
>  
>  	/*
>  	 * Iterate over all frames, create a control block
>  	 * for each frame and link them together.
>  	 */
>  	for (frame = 0; frame < d->frames; frame++) {
> -		struct bcm2835_dma_cb *control_block =
> -			&d->control_block_base[frame];
> +		struct bcm2835_dma_cb *control_block = d->cb_list[frame].cb;
>  
>  		/* Setup adresses */
>  		if (d->dir == DMA_DEV_TO_MEM) {
> @@ -428,12 +449,21 @@ static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic(
>  		 * This DMA engine driver currently only supports cyclic DMA.
>  		 * Therefore, wrap around at number of frames.
>  		 */
> -		control_block->next = d->control_block_base_phys +
> -			sizeof(struct bcm2835_dma_cb)
> -			* ((frame + 1) % d->frames);
> +		control_block->next = d->cb_list[((frame + 1) % d->frames)].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,
> 

[PATCH] dmaengine: bcm2835-dma: Convert to use DMA pool

[Stefan Wahren]

Hi Peter,

Am 17.11.2015 um 08:46 schrieb Peter Ujfalusi:
> Hi,
>
> On 11/16/2015 01:09 PM, Peter Ujfalusi wrote:
>> f93178291712 dmaengine: bcm2835-dma: Fix memory leak when stopping a
>> 	     running transfer
>>
>> Fixed the memleak, but introduced another issue: the terminate_all callback
>> might be called with interrupts disabled and the dma_free_coherent() is
>> not allowed to be called when IRQs are disabled.
>> Convert the driver to use dma_pool_* for managing the list of control
>> blocks for the transfer.
>
> FWIW: the patch has been tested and verified on Raspbery Pi:
> https://github.com/raspberrypi/linux/pull/1178#issuecomment-157026794
> https://github.com/raspberrypi/linux/pull/1178#issuecomment-157030190
>
> It needed some modification since the Raspberry Pi kernel have non upstreamed
> changes in bcm2835-dma driver (slave_sg support for example).
>
> It would be great if this patch can make it to 4.4 as a fix.
>
> Thanks,
> P?ter
>
>>
>> Fixes: f93178291712 ("dmaengine: bcm2835-dma: Fix memory leak when stopping a running transfer")
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>> Hi,
>>
>> It was brought to my attention that the memleak fix broke the bcm2835 DMA. I did
>> not noticed the use of dma_free_coherent() in the driver when I did the memleak
>> fix.
>> Since the driver does leaking memory every time the audio is stopped, the other
>> option is to convert it to use DMA pool.
>> I do not have access the Raspberry Pi, so I can not test this patch but it
>> compiles ;)
>> Can someone test this one out if it is working?

i tried to test it yesterday with dmatest until i noticed that the 
bcm2835 dmaengine didn't provide the necessary capabilities (DMA_MEMCPY).

Sorry, i'm not an expert here. Could you please provide at least one 
test scenario which should work with Linux 4.4-rc1 without any special 
hardware?

Do you know which bcm2835 drivers make use of the relevant code?

Btw if you want that somebody test the patch for you, then it's possible 
to mark it as request for testing "[PATCH RFT]".

Best regards
Stefan

>>
>> Regards,
>> Peter
>>

[PATCH] dmaengine: bcm2835-dma: Convert to use DMA pool

[Peter Ujfalusi]

On 11/17/2015 11:04 AM, Stefan Wahren wrote:
> Hi Peter,
> 
> Am 17.11.2015 um 08:46 schrieb Peter Ujfalusi:
>> Hi,
>>
>> On 11/16/2015 01:09 PM, Peter Ujfalusi wrote:
>>> f93178291712 dmaengine: bcm2835-dma: Fix memory leak when stopping a
>>>          running transfer
>>>
>>> Fixed the memleak, but introduced another issue: the terminate_all callback
>>> might be called with interrupts disabled and the dma_free_coherent() is
>>> not allowed to be called when IRQs are disabled.
>>> Convert the driver to use dma_pool_* for managing the list of control
>>> blocks for the transfer.
>>
>> FWIW: the patch has been tested and verified on Raspbery Pi:
>> https://github.com/raspberrypi/linux/pull/1178#issuecomment-157026794
>> https://github.com/raspberrypi/linux/pull/1178#issuecomment-157030190
>>
>> It needed some modification since the Raspberry Pi kernel have non upstreamed
>> changes in bcm2835-dma driver (slave_sg support for example).
>>
>> It would be great if this patch can make it to 4.4 as a fix.
>>
>> Thanks,
>> P?ter
>>
>>>
>>> Fixes: f93178291712 ("dmaengine: bcm2835-dma: Fix memory leak when stopping
>>> a running transfer")
>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>> ---
>>> Hi,
>>>
>>> It was brought to my attention that the memleak fix broke the bcm2835 DMA.
>>> I did
>>> not noticed the use of dma_free_coherent() in the driver when I did the
>>> memleak
>>> fix.
>>> Since the driver does leaking memory every time the audio is stopped, the
>>> other
>>> option is to convert it to use DMA pool.
>>> I do not have access the Raspberry Pi, so I can not test this patch but it
>>> compiles ;)
>>> Can someone test this one out if it is working?
> 
> i tried to test it yesterday with dmatest until i noticed that the bcm2835
> dmaengine didn't provide the necessary capabilities (DMA_MEMCPY).
> 
> Sorry, i'm not an expert here. Could you please provide at least one test
> scenario which should work with Linux 4.4-rc1 without any special hardware?

AFAIK in mainline the bcm2835-dma is only capable of CYCLIC transfer which is
used by audio. I think you would need add on module for Raspberry Pi and
enable the needed modules and use the correct DT to boot the board.
W/O additional HW there might be a way to test the audio with a dummy codec,
but I'm not familiar with bcm2835 so not sure what you need for that.

> Do you know which bcm2835 drivers make use of the relevant code?

Since the DMA is only supporting CYCLIC, you will need I2S audio to test this.
The guys over Raspberry Pi tested the patch and audio and their non mainline
slave_sg (MMC with DMA) is working. I have asked in the linked thread if they
could reply to this patch. I hope some of them will do that.

> Btw if you want that somebody test the patch for you, then it's possible to
> mark it as request for testing "[PATCH RFT]".

Yes, I should have added the RFT, true.

-- 
P?ter

[PATCH] dmaengine: bcm2835-dma: Convert to use DMA pool

[Matthias Reichl]

On Mon, Nov 16, 2015 at 01:09:03PM +0200, Peter Ujfalusi wrote:
> f93178291712 dmaengine: bcm2835-dma: Fix memory leak when stopping a
> 	     running transfer
> 
> Fixed the memleak, but introduced another issue: the terminate_all callback
> might be called with interrupts disabled and the dma_free_coherent() is
> not allowed to be called when IRQs are disabled.
> Convert the driver to use dma_pool_* for managing the list of control
> blocks for the transfer.
> 
> Fixes: f93178291712 ("dmaengine: bcm2835-dma: Fix memory leak when stopping a running transfer")
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Tested-by: Matthias Reichl <hias@horus.com>

Thanks a lot, the fix seems to be working fine with mainline 4.4-rc1,
4.4-rc2 and the 4.3 Raspberry Pi tree.

On the RPi tree you can simply enable the rpi-dac overlay for testing,
"dtoverlay=rpi-dac" in config.txt, it'll work without any additional
hardware.

As Peter mentioned I extended his patch to the non-mainlined slave_sg
code in the RPi tree. I've been using this during the last week and
haven't noticed any issues.

For testing with 4.4-rc mainline I used the (quick-and-dirty) code
below. It adds the missing PCM configuration to bcm2835-i2s, adds
some DT fixes (so DT and bcm2835-i2s are similar to the RPi tree)
and installs a card with a dummy-codec. You can then use aplay to
test the DMA code path and don't need any additional hardware.

I've checked the I2S GPIOs on a RPi B+ with my scope, PCM-clock
and frame-clock are fine and pcm-data-out also looks OK.

There's a small gotcha though: bcm2835-i2s hasn't been converted
to the new clock driver yet and also can't coexist with it
(overlapping iomem regions...). Fortunately, this can be solved
by disabling the new clock driver in the devicetree.

Here are the steps I used for testing:

- revert commits 121432c7a02f and 94cb7f76caa0 to disable the
  new clock driver in DT
- apply the test-code patch below
- make bcm2835_defconfig
- enable bcm2835-dma and bcm2835-i2s in kernel config:
  CONFIG_SOUND=y
  CONFIG_SND=y
  CONFIG_SND_SOC=y
  CONFIG_SND_BCM2835_SOC_I2S=y
  CONFIG_DMADEVICES=y
  CONFIG_DMA_BCM2835=y
- (optionally) apply Peter's DMA patch
- on the RPi use aplay to play a 44.1kHz 16-bit stereo WAV
  (32bit and other sample rates will work, too)

so long,

Hias

---

diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index 2029394..c3a1cfc 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -102,13 +102,13 @@
 
 		i2s: i2s at 7e203000 {
 			compatible = "brcm,bcm2835-i2s";
-			reg = <0x7e203000 0x20>,
-			      <0x7e101098 0x02>;
+			reg = <0x7e203000 0x24>,
+			      <0x7e101098 0x08>;
 
 			dmas = <&dma 2>,
 			       <&dma 3>;
 			dma-names = "tx", "rx";
-			status = "disabled";
+			status = "okay";
 		};
 
 		spi: spi at 7e204000 {
@@ -189,4 +189,9 @@
 			clock-frequency = <250000000>;
 		};
 	};
+
+	sound {
+		compatible = "brcm,i2s-dummy-card";
+		i2s-controller = <&i2s>;
+	};
 };
diff --git a/sound/soc/bcm/Makefile b/sound/soc/bcm/Makefile
index bc816b7..d599b62 100644
--- a/sound/soc/bcm/Makefile
+++ b/sound/soc/bcm/Makefile
@@ -1,5 +1,7 @@
 # BCM2835 Platform Support
 snd-soc-bcm2835-i2s-objs := bcm2835-i2s.o
+snd-soc-i2s-dummy-card-objs := i2s-dummy-card.o
 
 obj-$(CONFIG_SND_BCM2835_SOC_I2S) += snd-soc-bcm2835-i2s.o
+obj-$(CONFIG_SND_BCM2835_SOC_I2S) += snd-soc-i2s-dummy-card.o
 
diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c
index 8c435be..b1ff172 100644
--- a/sound/soc/bcm/bcm2835-i2s.c
+++ b/sound/soc/bcm/bcm2835-i2s.c
@@ -784,6 +784,24 @@ static const struct snd_soc_component_driver bcm2835_i2s_component = {
 	.name		= "bcm2835-i2s-comp",
 };
 
+static const struct snd_pcm_hardware bcm2835_pcm_hardware = {
+	.info			= SNDRV_PCM_INFO_INTERLEAVED |
+				  SNDRV_PCM_INFO_JOINT_DUPLEX,
+	.formats		= SNDRV_PCM_FMTBIT_S16_LE |
+				  SNDRV_PCM_FMTBIT_S32_LE,
+	.period_bytes_min	= 32,
+	.period_bytes_max	= SZ_64K - 4,
+	.periods_min		= 2,
+	.periods_max		= 255,
+	.buffer_bytes_max	= SZ_512K
+};
+
+static const struct snd_dmaengine_pcm_config bcm2835_dmaengine_pcm_config = {
+	.pcm_hardware = &bcm2835_pcm_hardware,
+	.prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
+	.prealloc_buffer_size = SZ_1M,
+};
+
 static int bcm2835_i2s_probe(struct platform_device *pdev)
 {
 	struct bcm2835_i2s_dev *dev;
@@ -848,7 +866,9 @@ static int bcm2835_i2s_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
+	ret = devm_snd_dmaengine_pcm_register(&pdev->dev,
+			&bcm2835_dmaengine_pcm_config,
+			SND_DMAENGINE_PCM_FLAG_COMPAT);
 	if (ret) {
 		dev_err(&pdev->dev, "Could not register PCM: %d\n", ret);
 		return ret;
diff --git a/sound/soc/bcm/i2s-dummy-card.c b/sound/soc/bcm/i2s-dummy-card.c
new file mode 100644
index 0000000..b3ad842
--- /dev/null
+++ b/sound/soc/bcm/i2s-dummy-card.c
@@ -0,0 +1,87 @@
+/*
+ * Dummy card driver for testing bcm2835-i2s
+ *
+ * Author:	Matthias Reichl <hias@horus.com>
+ *		Copyright 2015
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include <sound/core.h>
+#include <sound/soc.h>
+
+static struct snd_soc_dai_link snd_i2s_dummy_card_dai[] = {
+{
+	.name		= "i2s-dummy",
+	.stream_name	= "i2s-dummy HiFi",
+	.codec_name	= "snd-soc-dummy",
+	.codec_dai_name	= "snd-soc-dummy-dai",
+	.dai_fmt	= SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
+				SND_SOC_DAIFMT_CBS_CFS,
+},
+};
+
+static struct snd_soc_card snd_i2s_dummy_card = {
+	.name		= "i2s_dummy_card",
+	.dai_link	= snd_i2s_dummy_card_dai,
+	.num_links	= ARRAY_SIZE(snd_i2s_dummy_card_dai),
+};
+
+static int snd_i2s_dummy_card_probe(struct platform_device *pdev)
+{
+	int ret = 0;
+	struct device_node *i2s_node;
+
+	if (!pdev->dev.of_node) {
+		dev_err(&pdev->dev, "no of_node, exiting\n");
+		return -ENODEV;
+	}
+
+	i2s_node = of_parse_phandle(pdev->dev.of_node, "i2s-controller", 0);
+
+	if (!i2s_node) {
+		dev_err(&pdev->dev, "error getting i2s-controller, exiting\n");
+		return -ENODEV;
+	}
+
+	snd_i2s_dummy_card.dev = &pdev->dev;
+	snd_i2s_dummy_card_dai[0].cpu_of_node = i2s_node;
+	snd_i2s_dummy_card_dai[0].platform_of_node = i2s_node;
+
+	ret = devm_snd_soc_register_card(&pdev->dev, &snd_i2s_dummy_card);
+	if (ret)
+		dev_err(&pdev->dev, "snd_soc_register_card() failed: %d\n", ret);
+
+	return ret;
+}
+
+static const struct of_device_id snd_i2s_dummy_card_of_match[] = {
+	{ .compatible = "brcm,i2s-dummy-card", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, snd_i2s_dummy_card_of_match);
+
+static struct platform_driver snd_i2s_dummy_card_driver = {
+	.driver	= {
+		.name = "snd-i2s-dummy-card",
+		.of_match_table = snd_i2s_dummy_card_of_match,
+	},
+	.probe	= snd_i2s_dummy_card_probe,
+};
+
+module_platform_driver(snd_i2s_dummy_card_driver);
+
+MODULE_AUTHOR("Matthias Reichl <hias@horus.com>");
+MODULE_DESCRIPTION("ASoC dummy card driver for testing bcm2835-i2s");
+MODULE_LICENSE("GPL v2");

[PATCH] dmaengine: bcm2835-dma: Convert to use DMA pool

[Stefan Wahren]

Hi Matthias,

Am 23.11.2015 um 14:20 schrieb Matthias Reichl:
> On Mon, Nov 16, 2015 at 01:09:03PM +0200, Peter Ujfalusi wrote:
>> f93178291712 dmaengine: bcm2835-dma: Fix memory leak when stopping a
>> 	     running transfer
>>
>> Fixed the memleak, but introduced another issue: the terminate_all callback
>> might be called with interrupts disabled and the dma_free_coherent() is
>> not allowed to be called when IRQs are disabled.
>> Convert the driver to use dma_pool_* for managing the list of control
>> blocks for the transfer.
>>
>> Fixes: f93178291712 ("dmaengine: bcm2835-dma: Fix memory leak when stopping a running transfer")
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>
> Tested-by: Matthias Reichl <hias@horus.com>

good job!

Stefan

>
> Thanks a lot, the fix seems to be working fine with mainline 4.4-rc1,
> 4.4-rc2 and the 4.3 Raspberry Pi tree.
>
> On the RPi tree you can simply enable the rpi-dac overlay for testing,
> "dtoverlay=rpi-dac" in config.txt, it'll work without any additional
> hardware.
>
> As Peter mentioned I extended his patch to the non-mainlined slave_sg
> code in the RPi tree. I've been using this during the last week and
> haven't noticed any issues.
>
> For testing with 4.4-rc mainline I used the (quick-and-dirty) code
> below. It adds the missing PCM configuration to bcm2835-i2s, adds
> some DT fixes (so DT and bcm2835-i2s are similar to the RPi tree)
> and installs a card with a dummy-codec. You can then use aplay to
> test the DMA code path and don't need any additional hardware.
>
> I've checked the I2S GPIOs on a RPi B+ with my scope, PCM-clock
> and frame-clock are fine and pcm-data-out also looks OK.
>
> There's a small gotcha though: bcm2835-i2s hasn't been converted
> to the new clock driver yet and also can't coexist with it
> (overlapping iomem regions...). Fortunately, this can be solved
> by disabling the new clock driver in the devicetree.
>
> Here are the steps I used for testing:
>
> - revert commits 121432c7a02f and 94cb7f76caa0 to disable the
>    new clock driver in DT
> - apply the test-code patch below
> - make bcm2835_defconfig
> - enable bcm2835-dma and bcm2835-i2s in kernel config:
>    CONFIG_SOUND=y
>    CONFIG_SND=y
>    CONFIG_SND_SOC=y
>    CONFIG_SND_BCM2835_SOC_I2S=y
>    CONFIG_DMADEVICES=y
>    CONFIG_DMA_BCM2835=y
> - (optionally) apply Peter's DMA patch
> - on the RPi use aplay to play a 44.1kHz 16-bit stereo WAV
>    (32bit and other sample rates will work, too)
>
> so long,
>
> Hias
>
> ---
>
> diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
> index 2029394..c3a1cfc 100644
> --- a/arch/arm/boot/dts/bcm2835.dtsi
> +++ b/arch/arm/boot/dts/bcm2835.dtsi
> @@ -102,13 +102,13 @@
>
>   		i2s: i2s at 7e203000 {
>   			compatible = "brcm,bcm2835-i2s";
> -			reg = <0x7e203000 0x20>,
> -			      <0x7e101098 0x02>;
> +			reg = <0x7e203000 0x24>,
> +			      <0x7e101098 0x08>;
>
>   			dmas = <&dma 2>,
>   			       <&dma 3>;
>   			dma-names = "tx", "rx";
> -			status = "disabled";
> +			status = "okay";
>   		};
>
>   		spi: spi at 7e204000 {
> @@ -189,4 +189,9 @@
>   			clock-frequency = <250000000>;
>   		};
>   	};
> +
> +	sound {
> +		compatible = "brcm,i2s-dummy-card";
> +		i2s-controller = <&i2s>;
> +	};
>   };
> diff --git a/sound/soc/bcm/Makefile b/sound/soc/bcm/Makefile
> index bc816b7..d599b62 100644
> --- a/sound/soc/bcm/Makefile
> +++ b/sound/soc/bcm/Makefile
> @@ -1,5 +1,7 @@
>   # BCM2835 Platform Support
>   snd-soc-bcm2835-i2s-objs := bcm2835-i2s.o
> +snd-soc-i2s-dummy-card-objs := i2s-dummy-card.o
>
>   obj-$(CONFIG_SND_BCM2835_SOC_I2S) += snd-soc-bcm2835-i2s.o
> +obj-$(CONFIG_SND_BCM2835_SOC_I2S) += snd-soc-i2s-dummy-card.o
>
> diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c
> index 8c435be..b1ff172 100644
> --- a/sound/soc/bcm/bcm2835-i2s.c
> +++ b/sound/soc/bcm/bcm2835-i2s.c
> @@ -784,6 +784,24 @@ static const struct snd_soc_component_driver bcm2835_i2s_component = {
>   	.name		= "bcm2835-i2s-comp",
>   };
>
> +static const struct snd_pcm_hardware bcm2835_pcm_hardware = {
> +	.info			= SNDRV_PCM_INFO_INTERLEAVED |
> +				  SNDRV_PCM_INFO_JOINT_DUPLEX,
> +	.formats		= SNDRV_PCM_FMTBIT_S16_LE |
> +				  SNDRV_PCM_FMTBIT_S32_LE,
> +	.period_bytes_min	= 32,
> +	.period_bytes_max	= SZ_64K - 4,
> +	.periods_min		= 2,
> +	.periods_max		= 255,
> +	.buffer_bytes_max	= SZ_512K
> +};
> +
> +static const struct snd_dmaengine_pcm_config bcm2835_dmaengine_pcm_config = {
> +	.pcm_hardware = &bcm2835_pcm_hardware,
> +	.prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
> +	.prealloc_buffer_size = SZ_1M,
> +};
> +
>   static int bcm2835_i2s_probe(struct platform_device *pdev)
>   {
>   	struct bcm2835_i2s_dev *dev;
> @@ -848,7 +866,9 @@ static int bcm2835_i2s_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>
> -	ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
> +	ret = devm_snd_dmaengine_pcm_register(&pdev->dev,
> +			&bcm2835_dmaengine_pcm_config,
> +			SND_DMAENGINE_PCM_FLAG_COMPAT);
>   	if (ret) {
>   		dev_err(&pdev->dev, "Could not register PCM: %d\n", ret);
>   		return ret;
> diff --git a/sound/soc/bcm/i2s-dummy-card.c b/sound/soc/bcm/i2s-dummy-card.c
> new file mode 100644
> index 0000000..b3ad842
> --- /dev/null
> +++ b/sound/soc/bcm/i2s-dummy-card.c
> @@ -0,0 +1,87 @@
> +/*
> + * Dummy card driver for testing bcm2835-i2s
> + *
> + * Author:	Matthias Reichl <hias@horus.com>
> + *		Copyright 2015
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#include <sound/core.h>
> +#include <sound/soc.h>
> +
> +static struct snd_soc_dai_link snd_i2s_dummy_card_dai[] = {
> +{
> +	.name		= "i2s-dummy",
> +	.stream_name	= "i2s-dummy HiFi",
> +	.codec_name	= "snd-soc-dummy",
> +	.codec_dai_name	= "snd-soc-dummy-dai",
> +	.dai_fmt	= SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
> +				SND_SOC_DAIFMT_CBS_CFS,
> +},
> +};
> +
> +static struct snd_soc_card snd_i2s_dummy_card = {
> +	.name		= "i2s_dummy_card",
> +	.dai_link	= snd_i2s_dummy_card_dai,
> +	.num_links	= ARRAY_SIZE(snd_i2s_dummy_card_dai),
> +};
> +
> +static int snd_i2s_dummy_card_probe(struct platform_device *pdev)
> +{
> +	int ret = 0;
> +	struct device_node *i2s_node;
> +
> +	if (!pdev->dev.of_node) {
> +		dev_err(&pdev->dev, "no of_node, exiting\n");
> +		return -ENODEV;
> +	}
> +
> +	i2s_node = of_parse_phandle(pdev->dev.of_node, "i2s-controller", 0);
> +
> +	if (!i2s_node) {
> +		dev_err(&pdev->dev, "error getting i2s-controller, exiting\n");
> +		return -ENODEV;
> +	}
> +
> +	snd_i2s_dummy_card.dev = &pdev->dev;
> +	snd_i2s_dummy_card_dai[0].cpu_of_node = i2s_node;
> +	snd_i2s_dummy_card_dai[0].platform_of_node = i2s_node;
> +
> +	ret = devm_snd_soc_register_card(&pdev->dev, &snd_i2s_dummy_card);
> +	if (ret)
> +		dev_err(&pdev->dev, "snd_soc_register_card() failed: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id snd_i2s_dummy_card_of_match[] = {
> +	{ .compatible = "brcm,i2s-dummy-card", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, snd_i2s_dummy_card_of_match);
> +
> +static struct platform_driver snd_i2s_dummy_card_driver = {
> +	.driver	= {
> +		.name = "snd-i2s-dummy-card",
> +		.of_match_table = snd_i2s_dummy_card_of_match,
> +	},
> +	.probe	= snd_i2s_dummy_card_probe,
> +};
> +
> +module_platform_driver(snd_i2s_dummy_card_driver);
> +
> +MODULE_AUTHOR("Matthias Reichl <hias@horus.com>");
> +MODULE_DESCRIPTION("ASoC dummy card driver for testing bcm2835-i2s");
> +MODULE_LICENSE("GPL v2");
>

[PATCH] dmaengine: bcm2835-dma: Convert to use DMA pool

[Eric Anholt]

Peter Ujfalusi <peter.ujfalusi@ti.com> writes:

> f93178291712 dmaengine: bcm2835-dma: Fix memory leak when stopping a
> 	     running transfer
>
> Fixed the memleak, but introduced another issue: the terminate_all callback
> might be called with interrupts disabled and the dma_free_coherent() is
> not allowed to be called when IRQs are disabled.
> Convert the driver to use dma_pool_* for managing the list of control
> blocks for the transfer.
>
> Fixes: f93178291712 ("dmaengine: bcm2835-dma: Fix memory leak when stopping a running transfer")
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

It sounds like you got positive testing feedback.  Using the DMA pool
for our bcm2835_dma_cbs makes sense to me, too.

Reviewed-by: Eric Anholt <eric@anholt.net>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151124/938864d8/attachment.sig>

[PATCH] dmaengine: bcm2835-dma: Convert to use DMA pool

[Vinod Koul]

On Mon, Nov 16, 2015 at 01:09:03PM +0200, Peter Ujfalusi wrote:
> f93178291712 dmaengine: bcm2835-dma: Fix memory leak when stopping a
> 	     running transfer
> 
> Fixed the memleak, but introduced another issue: the terminate_all callback
> might be called with interrupts disabled and the dma_free_coherent() is
> not allowed to be called when IRQs are disabled.
> Convert the driver to use dma_pool_* for managing the list of control
> blocks for the transfer.

Applied, thanks

-- 
~Vinod