diff mbox

[v6,10/10] ASoC: rockchip_i2s: modify DMA max burst to 1

Message ID 1444873008-2589-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shawn Lin Oct. 15, 2015, 1:36 a.m. UTC 
From: Yiwei Cai <cain.cai@rock-chips.com>

Test with command -
arecord -D hw:0,0 /tmp/a.wav, there are the error dump:
dma-pl330 ffb20000.dma-controller: fill_queue:2251 Bad Desc(7)

This error is happening when no a multiple of burst size * burst
length are coming in. The root cause is pl330 dma controller on
Rockchips' platform cannot support DMAFLUSHP instruction which make
dma to flush the req of non-aligned or non-multiple of what we set
before. The saftest way is to set dma max burst to 1.

Signed-off-by: Yiwei Cai <cain.cai@rock-chips.com>
Fixes: 4495c89fc ("ASoC: add driver for Rockchip RK3xxx I2S")
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
cc: Addy Ke <addy.ke@rock-chips.com>
cc: Jianqun Xu <xjq@rock-chips.com>
cc: Heiko Stuebner <heiko@sntech.de>
cc: Olof Johansson <olof@lixom.net>
cc: Doug Anderson <dianders@chromium.org>
cc: Sonny Rao <sonnyrao@chromium.org>

Acked-by: Mark Brown <broonie@kernel.org>
---

Changes in v6:
- remove quirks and get dma caps in order to limit burst

Changes in v5:
- use switch statement for dma_quirk's manipulation

Changes in v4: None
Changes in v3: None
Changes in v2: None
Changes in v1: None

 sound/soc/rockchip/rockchip_i2s.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Comments

kernel test robot Oct. 15, 2015, 3:20 a.m. UTC | #1 
Hi Yiwei,

[auto build test ERROR on rockchip/for-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Shawn-Lin/Fix-broken-DMAFLUSHP-on-Rockchips-platform/20151015-094613
config: x86_64-allmodconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   sound/soc/rockchip/rockchip_i2s.c:467:13: sparse: undefined identifier 'snd_dmaengine_pcm_get_caps'
   sound/soc/rockchip/rockchip_i2s.c:468:29: sparse: expected structure or union
   sound/soc/rockchip/rockchip_i2s.c: In function 'rockchip_i2s_probe':
>> sound/soc/rockchip/rockchip_i2s.c:467:6: error: implicit declaration of function 'snd_dmaengine_pcm_get_caps' [-Werror=implicit-function-declaration]
     if (snd_dmaengine_pcm_get_caps(&pdev->dev, &dma_caps) == 0) {
         ^
>> sound/soc/rockchip/rockchip_i2s.c:468:15: error: request for member 'max_burst' in something not a structure or union
      if (dma_caps.max_burst > 4) {
                  ^
   cc1: some warnings being treated as errors

vim +/snd_dmaengine_pcm_get_caps +467 sound/soc/rockchip/rockchip_i2s.c

   461		i2s->playback_dma_data.addr = res->start + I2S_TXDR;
   462		i2s->playback_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
   463	
   464		i2s->capture_dma_data.addr = res->start + I2S_RXDR;
   465		i2s->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
   466	
 > 467		if (snd_dmaengine_pcm_get_caps(&pdev->dev, &dma_caps) == 0) {
 > 468			if (dma_caps.max_burst > 4) {
   469				i2s->playback_dma_data.maxburst = 4;
   470				i2s->capture_dma_data.maxburst = 4;
   471			} else {

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Lars-Peter Clausen Oct. 15, 2015, 8:53 a.m. UTC | #2 
On 10/15/2015 03:36 AM, Shawn Lin wrote:
[...]
> +
> +	if (snd_dmaengine_pcm_get_caps(&pdev->dev, &dma_caps) == 0) {
> +		if (dma_caps.max_burst > 4) {
> +			i2s->playback_dma_data.maxburst = 4;
> +			i2s->capture_dma_data.maxburst = 4;
> +		} else {
> +			i2s->playback_dma_data.maxburst = 1;
> +			i2s->capture_dma_data.maxburst = 1;

So this is what this is all about? I though you might have to program some
FIFO threshold registers in the peripheral itself.

But it seems all this does is to read the maximum burst length from the DMA
controller only to tell the DMA controller that this is the maximum burst
length it should use. That seems rather unnecessary.

The maxburst field of the dma_data indicates the maximum burst length that
the audio peripheral can handle. Typically this is the number of samples the
audio FIFO can receive without overflowing after sending the DMA request
signal. Since as the name suggests this is the maximum burst size the DMA
controller is free to choose a burst size smaller than this when writing
data to the peripheral.

So in your case instead of introducing all these facilities to query the
maximum burst size it should be OK to simply reduce the burst size in the
DMA controller itself when it gets a request with a burst size larger than
it can handle, or is there a reason why this is not possible?

- Lars
Jianqun Xu Oct. 15, 2015, 10:47 a.m. UTC | #3 
? 2015?10?15? 16:53, Lars-Peter Clausen ??:
> On 10/15/2015 03:36 AM, Shawn Lin wrote:
> [...]
>> +
>> +	if (snd_dmaengine_pcm_get_caps(&pdev->dev, &dma_caps) == 0) {
>> +		if (dma_caps.max_burst > 4) {
>> +			i2s->playback_dma_data.maxburst = 4;
>> +			i2s->capture_dma_data.maxburst = 4;
>> +		} else {
>> +			i2s->playback_dma_data.maxburst = 1;
>> +			i2s->capture_dma_data.maxburst = 1;
> So this is what this is all about? I though you might have to program some
> FIFO threshold registers in the peripheral itself.
>
> But it seems all this does is to read the maximum burst length from the DMA
> controller only to tell the DMA controller that this is the maximum burst
> length it should use. That seems rather unnecessary.
>
> The maxburst field of the dma_data indicates the maximum burst length that
> the audio peripheral can handle. Typically this is the number of samples the
> audio FIFO can receive without overflowing after sending the DMA request
> signal. Since as the name suggests this is the maximum burst size the DMA
> controller is free to choose a burst size smaller than this when writing
> data to the peripheral.
>
> So in your case instead of introducing all these facilities to query the
> maximum burst size it should be OK to simply reduce the burst size in the
> DMA controller itself when it gets a request with a burst size larger than
> it can handle, or is there a reason why this is not possible?
Agree with Lars, it's better to fix on DMA driver side since issue 
caused by dma-controller instead of i2s controller
>
> - Lars
>
>
>
diff mbox

Patch

diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
index b936102..f00200a 100644
--- a/sound/soc/rockchip/rockchip_i2s.c
+++ b/sound/soc/rockchip/rockchip_i2s.c
@@ -418,6 +418,7 @@  static int rockchip_i2s_probe(struct platform_device *pdev)
 	struct rk_i2s_dev *i2s;
 	struct resource *res;
 	void __iomem *regs;
+	struct dma_slave_caps *dma_caps;
 	int ret;
 
 	i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
@@ -459,11 +460,24 @@  static int rockchip_i2s_probe(struct platform_device *pdev)
 
 	i2s->playback_dma_data.addr = res->start + I2S_TXDR;
 	i2s->playback_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
-	i2s->playback_dma_data.maxburst = 4;
 
 	i2s->capture_dma_data.addr = res->start + I2S_RXDR;
 	i2s->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
-	i2s->capture_dma_data.maxburst = 4;
+
+	if (snd_dmaengine_pcm_get_caps(&pdev->dev, &dma_caps) == 0) {
+		if (dma_caps.max_burst > 4) {
+			i2s->playback_dma_data.maxburst = 4;
+			i2s->capture_dma_data.maxburst = 4;
+		} else {
+			i2s->playback_dma_data.maxburst = 1;
+			i2s->capture_dma_data.maxburst = 1;
+		}
+	} else {
+		i2s->playback_dma_data.maxburst = 1;
+		i2s->capture_dma_data.maxburst = 1;
+		dev_info(&pdev->dev,
+			 "Can't get dma caps, default limit maxburst to 1.\n");
+	}
 
 	i2s->dev = &pdev->dev;
 	dev_set_drvdata(&pdev->dev, i2s);