[alsa-devel] [PATCH 0/10] Kirkwood ASoC drivers fixes and improvements
The first five patches of this patch series fixes a number of problems with the Kirkwood audio driver, as seen on Dove platforms. The biggest issue is that they can cause the kernel to lock up solidly, requiring a reset to recover.
Other issues included in the first six patches are bugs found by the cubox folk.
The other five patches convert the I2S driver to use the devm_* APIs, improve the handling of the record and playback control registers so we're not reading and writing them as often, lift restrictions in the DMA driver which aren't necessary to impose, and add support for external clocks so that sample rates other than 44.1, 48 and 96kHz can be supported. Some of these changes prepare the driver to support the SPDIF output including passthrough mode; however, that support is not included in this patch set.
These patches probably need checking out on Kirkwood before they go into mainline, but given the severity of the kernel lockup, I suggest that once tested the first five patches go into -rc.
sound/soc/kirkwood/kirkwood-dma.c | 19 ++- sound/soc/kirkwood/kirkwood-i2s.c | 291 ++++++++++++++++++++++--------------- sound/soc/kirkwood/kirkwood.h | 11 +- 3 files changed, 197 insertions(+), 124 deletions(-)
ASoC: kirkwood-dma: remove channel restrictions ASoC: kirkwood-i2s: add support for external clock rates ASoC: kirkwood-dma: remove restriction on sample rates ASoC: kirkwood-i2s: better handling of play/record control registers ASoC: kirkwood-i2s: use devm_* APIs ASoC: kirkwood-i2s: more pause-mode fixes ASoC: kirkwood-i2s: fix DMA underruns ASoC: kirkwood-i2s: fix DCO lock detection ASoC: kirkwood-dma: don't ignore other irq causes on error ASoC: kirkwood-dma: fix use of virt_to_phys()
This is part of a patch found in Rabeeh Khoury's git tree for the cubox.
You can not use virt_to_phys() on the address returned from dma_alloc_coherent(); it may not be part of the kernel direct-mapped memory. Fix this to use the DMA address instead.
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- sound/soc/kirkwood/kirkwood-dma.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/sound/soc/kirkwood/kirkwood-dma.c b/sound/soc/kirkwood/kirkwood-dma.c index b9f1659..afe1930 100644 --- a/sound/soc/kirkwood/kirkwood-dma.c +++ b/sound/soc/kirkwood/kirkwood-dma.c @@ -178,7 +178,7 @@ static int kirkwood_dma_open(struct snd_pcm_substream *substream) }
dram = mv_mbus_dram_info(); - addr = virt_to_phys(substream->dma_buffer.area); + addr = substream->dma_buffer.addr; if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { prdata->play_stream = substream; kirkwood_dma_conf_mbus_windows(priv->io,
Ignoring the real cause of the interrupt is not a good idea; this behaviour has been observed to bring Dove platforms to silently lockup. Instead, on error fall through to the normal interrupt processing.
This is especially important on Dove platforms as errors are handled separately, and allows us to clear down the real cause of the interrupt.
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- sound/soc/kirkwood/kirkwood-dma.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/sound/soc/kirkwood/kirkwood-dma.c b/sound/soc/kirkwood/kirkwood-dma.c index afe1930..2ba0814 100644 --- a/sound/soc/kirkwood/kirkwood-dma.c +++ b/sound/soc/kirkwood/kirkwood-dma.c @@ -71,7 +71,6 @@ static irqreturn_t kirkwood_dma_irq(int irq, void *dev_id) printk(KERN_WARNING "%s: got err interrupt 0x%lx\n", __func__, cause); writel(cause, priv->io + KIRKWOOD_ERR_CAUSE); - return IRQ_HANDLED; }
/* we've enabled only bytes interrupts ... */
This is part of a patch found in Rabeeh Khoury's git tree for the cubox, which is further attributed to Sebastian Hesselbrath.
Rather than masking the KIRKWOOD_DCO_SPCR_STATUS register contents against the registers virtual address, let's actually use the bit definition for the locked status, as required in the documentation.
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- sound/soc/kirkwood/kirkwood-i2s.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index 542538d..485af80 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -95,7 +95,7 @@ static inline void kirkwood_set_dco(void __iomem *io, unsigned long rate) do { cpu_relax(); value = readl(io + KIRKWOOD_DCO_SPCR_STATUS); - value &= KIRKWOOD_DCO_SPCR_STATUS; + value &= KIRKWOOD_DCO_SPCR_STATUS_DCO_LOCK; } while (value == 0); }
Stress testing the driver with multiple start/stop events causes kirkwood-dma to report underrun errors (which used to cause the kernel to lock up solidly). This is because kirkwood-i2s is not respecting the restrictions imposed on clearing the 'pause' bit. Follow what the spec says; the busy bit must be read as being clear twice before the pause bit can be released. This solves the underruns.
However, it has been noticed that the busy bit occasionally does not clear itself, hence the waiting is bounded to 5ms maximum to avoid a new reason for the kernel to lockup.
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- sound/soc/kirkwood/kirkwood-i2s.c | 67 +++++++++++++++++++++---------------- 1 files changed, 38 insertions(+), 29 deletions(-)
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index 485af80..826306d 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -180,67 +180,76 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) { struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai); - unsigned long value; - - /* - * specs says KIRKWOOD_PLAYCTL must be read 2 times before - * changing it. So read 1 time here and 1 later. - */ - value = readl(priv->io + KIRKWOOD_PLAYCTL); + uint32_t ctl, value; + + ctl = readl(priv->io + KIRKWOOD_PLAYCTL); + if (ctl & KIRKWOOD_PLAYCTL_PAUSE) { + unsigned timeout = 5000; + /* + * The Armada510 spec says that if we enter pause mode, the + * busy bit must be read back as clear _twice_. Make sure + * we respect that otherwise we get DMA underruns. + */ + do { + value = ctl; + ctl = readl(priv->io + KIRKWOOD_PLAYCTL); + if (!((ctl | value) & KIRKWOOD_PLAYCTL_PLAY_BUSY)) + break; + udelay(1); + } while (timeout--); + + if ((ctl | value) & KIRKWOOD_PLAYCTL_PLAY_BUSY) + dev_notice(dai->dev, "timed out waiting for busy to deassert: %08x\n", + ctl); + }
switch (cmd) { case SNDRV_PCM_TRIGGER_START: /* stop audio, enable interrupts */ - value = readl(priv->io + KIRKWOOD_PLAYCTL); - value |= KIRKWOOD_PLAYCTL_PAUSE; - writel(value, priv->io + KIRKWOOD_PLAYCTL); + ctl |= KIRKWOOD_PLAYCTL_PAUSE; + writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
value = readl(priv->io + KIRKWOOD_INT_MASK); value |= KIRKWOOD_INT_CAUSE_PLAY_BYTES; writel(value, priv->io + KIRKWOOD_INT_MASK);
/* configure audio & enable i2s playback */ - value = readl(priv->io + KIRKWOOD_PLAYCTL); - value &= ~KIRKWOOD_PLAYCTL_BURST_MASK; - value &= ~(KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE + ctl &= ~KIRKWOOD_PLAYCTL_BURST_MASK; + ctl &= ~(KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE | KIRKWOOD_PLAYCTL_SPDIF_EN);
if (priv->burst == 32) - value |= KIRKWOOD_PLAYCTL_BURST_32; + ctl |= KIRKWOOD_PLAYCTL_BURST_32; else - value |= KIRKWOOD_PLAYCTL_BURST_128; - value |= KIRKWOOD_PLAYCTL_I2S_EN; - writel(value, priv->io + KIRKWOOD_PLAYCTL); + ctl |= KIRKWOOD_PLAYCTL_BURST_128; + ctl |= KIRKWOOD_PLAYCTL_I2S_EN; + writel(ctl, priv->io + KIRKWOOD_PLAYCTL); break;
case SNDRV_PCM_TRIGGER_STOP: /* stop audio, disable interrupts */ - value = readl(priv->io + KIRKWOOD_PLAYCTL); - value |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE; - writel(value, priv->io + KIRKWOOD_PLAYCTL); + ctl |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE; + writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
value = readl(priv->io + KIRKWOOD_INT_MASK); value &= ~KIRKWOOD_INT_CAUSE_PLAY_BYTES; writel(value, priv->io + KIRKWOOD_INT_MASK);
/* disable all playbacks */ - value = readl(priv->io + KIRKWOOD_PLAYCTL); - value &= ~(KIRKWOOD_PLAYCTL_I2S_EN | KIRKWOOD_PLAYCTL_SPDIF_EN); - writel(value, priv->io + KIRKWOOD_PLAYCTL); + ctl &= ~(KIRKWOOD_PLAYCTL_I2S_EN | KIRKWOOD_PLAYCTL_SPDIF_EN); + writel(ctl, priv->io + KIRKWOOD_PLAYCTL); break;
case SNDRV_PCM_TRIGGER_PAUSE_PUSH: case SNDRV_PCM_TRIGGER_SUSPEND: - value = readl(priv->io + KIRKWOOD_PLAYCTL); - value |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE; - writel(value, priv->io + KIRKWOOD_PLAYCTL); + ctl |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE; + writel(ctl, priv->io + KIRKWOOD_PLAYCTL); break;
case SNDRV_PCM_TRIGGER_RESUME: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: - value = readl(priv->io + KIRKWOOD_PLAYCTL); - value &= ~(KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE); - writel(value, priv->io + KIRKWOOD_PLAYCTL); + ctl &= ~(KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE); + writel(ctl, priv->io + KIRKWOOD_PLAYCTL); break;
default:
Don't even momentarily set the pause status when starting the channel; if we do, we should check the busy bit to ensure that we comply with the spec. In any case, it isn't necessary; we will not active on a START event so there is no need to pause the DMA.
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- sound/soc/kirkwood/kirkwood-i2s.c | 9 --------- 1 files changed, 0 insertions(+), 9 deletions(-)
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index 826306d..1d5db48 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -205,10 +205,6 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
switch (cmd) { case SNDRV_PCM_TRIGGER_START: - /* stop audio, enable interrupts */ - ctl |= KIRKWOOD_PLAYCTL_PAUSE; - writel(ctl, priv->io + KIRKWOOD_PLAYCTL); - value = readl(priv->io + KIRKWOOD_INT_MASK); value |= KIRKWOOD_INT_CAUSE_PLAY_BYTES; writel(value, priv->io + KIRKWOOD_INT_MASK); @@ -269,11 +265,6 @@ static int kirkwood_i2s_rec_trigger(struct snd_pcm_substream *substream,
switch (cmd) { case SNDRV_PCM_TRIGGER_START: - /* stop audio, enable interrupts */ - value = readl(priv->io + KIRKWOOD_RECCTL); - value |= KIRKWOOD_RECCTL_PAUSE; - writel(value, priv->io + KIRKWOOD_RECCTL); - value = readl(priv->io + KIRKWOOD_INT_MASK); value |= KIRKWOOD_INT_CAUSE_REC_BYTES; writel(value, priv->io + KIRKWOOD_INT_MASK);
Simplify the cleanup paths in the driver by using the devm_* APIs, ensuring that all error paths are correctly checked.
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- sound/soc/kirkwood/kirkwood-i2s.c | 53 ++++++++++-------------------------- sound/soc/kirkwood/kirkwood.h | 1 - 2 files changed, 15 insertions(+), 39 deletions(-)
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index 1d5db48..f059f40 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -406,57 +406,47 @@ static __devinit int kirkwood_i2s_dev_probe(struct platform_device *pdev) struct kirkwood_dma_data *priv; int err;
- priv = kzalloc(sizeof(struct kirkwood_dma_data), GFP_KERNEL); + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); if (!priv) { dev_err(&pdev->dev, "allocation failed\n"); - err = -ENOMEM; - goto error; + return -ENOMEM; } dev_set_drvdata(&pdev->dev, priv);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!mem) { dev_err(&pdev->dev, "platform_get_resource failed\n"); - err = -ENXIO; - goto err_alloc; + return -ENXIO; }
- priv->mem = request_mem_region(mem->start, SZ_16K, DRV_NAME); - if (!priv->mem) { - dev_err(&pdev->dev, "request_mem_region failed\n"); - err = -EBUSY; - goto err_alloc; - } - - priv->io = ioremap(priv->mem->start, SZ_16K); + priv->io = devm_request_and_ioremap(&pdev->dev, mem); if (!priv->io) { - dev_err(&pdev->dev, "ioremap failed\n"); - err = -ENOMEM; - goto err_iomem; + dev_err(&pdev->dev, "devm_request_and_ioremap failed\n"); + return -ENOMEM; }
priv->irq = platform_get_irq(pdev, 0); if (priv->irq <= 0) { dev_err(&pdev->dev, "platform_get_irq failed\n"); - err = -ENXIO; - goto err_ioremap; + return -ENXIO; }
if (!data) { dev_err(&pdev->dev, "no platform data ?!\n"); - err = -EINVAL; - goto err_ioremap; + return -EINVAL; }
priv->burst = data->burst;
- priv->clk = clk_get(&pdev->dev, NULL); + priv->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(priv->clk)) { dev_err(&pdev->dev, "no clock\n"); - err = PTR_ERR(priv->clk); - goto err_ioremap; + return PTR_ERR(priv->clk); } - clk_prepare_enable(priv->clk); + + err = clk_prepare_enable(priv->clk); + if (err < 0) + return err;
err = snd_soc_register_dai(&pdev->dev, &kirkwood_i2s_dai); if (!err) @@ -464,15 +454,7 @@ static __devinit int kirkwood_i2s_dev_probe(struct platform_device *pdev) dev_err(&pdev->dev, "snd_soc_register_dai failed\n");
clk_disable_unprepare(priv->clk); - clk_put(priv->clk); - -err_ioremap: - iounmap(priv->io); -err_iomem: - release_mem_region(priv->mem->start, SZ_16K); -err_alloc: - kfree(priv); -error: + return err; }
@@ -483,11 +465,6 @@ static __devexit int kirkwood_i2s_dev_remove(struct platform_device *pdev) snd_soc_unregister_dai(&pdev->dev);
clk_disable_unprepare(priv->clk); - clk_put(priv->clk); - - iounmap(priv->io); - release_mem_region(priv->mem->start, SZ_16K); - kfree(priv);
return 0; } diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h index f9084d8..7d92b9e 100644 --- a/sound/soc/kirkwood/kirkwood.h +++ b/sound/soc/kirkwood/kirkwood.h @@ -119,7 +119,6 @@ #define KIRKWOOD_SND_MAX_PERIOD_BYTES 0x4000
struct kirkwood_dma_data { - struct resource *mem; void __iomem *io; int irq; int burst;
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- sound/soc/kirkwood/kirkwood-i2s.c | 112 ++++++++++++++++++++++++------------- sound/soc/kirkwood/kirkwood.h | 2 + 2 files changed, 74 insertions(+), 40 deletions(-)
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index f059f40..823ef1e 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -113,15 +113,14 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai); - unsigned int i2s_reg, reg; - unsigned long i2s_value, value; + uint32_t ctl_play, ctl_rec; + unsigned int i2s_reg; + unsigned long i2s_value;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { i2s_reg = KIRKWOOD_I2S_PLAYCTL; - reg = KIRKWOOD_PLAYCTL; } else { i2s_reg = KIRKWOOD_I2S_RECCTL; - reg = KIRKWOOD_RECCTL; }
/* set dco conf */ @@ -130,9 +129,6 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream, i2s_value = readl(priv->io+i2s_reg); i2s_value &= ~KIRKWOOD_I2S_CTL_SIZE_MASK;
- value = readl(priv->io+reg); - value &= ~KIRKWOOD_PLAYCTL_SIZE_MASK; - /* * Size settings in play/rec i2s control regs and play/rec control * regs must be the same. @@ -140,38 +136,57 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream, switch (params_format(params)) { case SNDRV_PCM_FORMAT_S16_LE: i2s_value |= KIRKWOOD_I2S_CTL_SIZE_16; - value |= KIRKWOOD_PLAYCTL_SIZE_16_C; + ctl_play = KIRKWOOD_PLAYCTL_SIZE_16_C | + KIRKWOOD_PLAYCTL_I2S_EN; + ctl_rec = KIRKWOOD_RECCTL_SIZE_16_C | + KIRKWOOD_RECCTL_I2S_EN; break; /* * doesn't work... S20_3LE != kirkwood 20bit format ? * case SNDRV_PCM_FORMAT_S20_3LE: i2s_value |= KIRKWOOD_I2S_CTL_SIZE_20; - value |= KIRKWOOD_PLAYCTL_SIZE_20; + ctl_play = KIRKWOOD_PLAYCTL_SIZE_20 | + KIRKWOOD_PLAYCTL_I2S_EN; + ctl_rec = KIRKWOOD_RECCTL_SIZE_20 | + KIRKWOOD_RECCTL_I2S_EN; break; */ case SNDRV_PCM_FORMAT_S24_LE: i2s_value |= KIRKWOOD_I2S_CTL_SIZE_24; - value |= KIRKWOOD_PLAYCTL_SIZE_24; + ctl_play = KIRKWOOD_PLAYCTL_SIZE_24 | + KIRKWOOD_PLAYCTL_I2S_EN; + ctl_rec = KIRKWOOD_RECCTL_SIZE_24 | + KIRKWOOD_RECCTL_I2S_EN; break; case SNDRV_PCM_FORMAT_S32_LE: i2s_value |= KIRKWOOD_I2S_CTL_SIZE_32; - value |= KIRKWOOD_PLAYCTL_SIZE_32; + ctl_play = KIRKWOOD_PLAYCTL_SIZE_32 | + KIRKWOOD_PLAYCTL_I2S_EN; + ctl_rec = KIRKWOOD_RECCTL_SIZE_32 | + KIRKWOOD_RECCTL_I2S_EN; break; default: return -EINVAL; }
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { - value &= ~KIRKWOOD_PLAYCTL_MONO_MASK; if (params_channels(params) == 1) - value |= KIRKWOOD_PLAYCTL_MONO_BOTH; + ctl_play |= KIRKWOOD_PLAYCTL_MONO_BOTH; else - value |= KIRKWOOD_PLAYCTL_MONO_OFF; + ctl_play |= KIRKWOOD_PLAYCTL_MONO_OFF; + + priv->ctl_play &= ~(KIRKWOOD_PLAYCTL_MONO_MASK | + KIRKWOOD_PLAYCTL_I2S_EN | + KIRKWOOD_PLAYCTL_SPDIF_EN | + KIRKWOOD_PLAYCTL_SIZE_MASK); + priv->ctl_play |= ctl_play; + } else { + priv->ctl_rec &= ~KIRKWOOD_RECCTL_SIZE_MASK; + priv->ctl_rec |= ctl_rec; }
writel(i2s_value, priv->io+i2s_reg); - writel(value, priv->io+reg);
return 0; } @@ -205,20 +220,18 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
switch (cmd) { case SNDRV_PCM_TRIGGER_START: + /* configure */ + ctl = priv->ctl_play; + value = ctl & ~(KIRKWOOD_PLAYCTL_I2S_EN | + KIRKWOOD_PLAYCTL_SPDIF_EN); + writel(value, priv->io + KIRKWOOD_PLAYCTL); + + /* enable interrupts */ value = readl(priv->io + KIRKWOOD_INT_MASK); value |= KIRKWOOD_INT_CAUSE_PLAY_BYTES; writel(value, priv->io + KIRKWOOD_INT_MASK);
- /* configure audio & enable i2s playback */ - ctl &= ~KIRKWOOD_PLAYCTL_BURST_MASK; - ctl &= ~(KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE - | KIRKWOOD_PLAYCTL_SPDIF_EN); - - if (priv->burst == 32) - ctl |= KIRKWOOD_PLAYCTL_BURST_32; - else - ctl |= KIRKWOOD_PLAYCTL_BURST_128; - ctl |= KIRKWOOD_PLAYCTL_I2S_EN; + /* enable playback */ writel(ctl, priv->io + KIRKWOOD_PLAYCTL); break;
@@ -259,30 +272,24 @@ static int kirkwood_i2s_rec_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) { struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai); - unsigned long value; + uint32_t ctl, value;
value = readl(priv->io + KIRKWOOD_RECCTL);
switch (cmd) { case SNDRV_PCM_TRIGGER_START: + /* configure */ + ctl = priv->ctl_rec; + value = ctl & ~KIRKWOOD_RECCTL_I2S_EN; + writel(value, priv->io + KIRKWOOD_RECCTL); + + /* enable interrupts */ value = readl(priv->io + KIRKWOOD_INT_MASK); value |= KIRKWOOD_INT_CAUSE_REC_BYTES; writel(value, priv->io + KIRKWOOD_INT_MASK);
- /* configure audio & enable i2s record */ - value = readl(priv->io + KIRKWOOD_RECCTL); - value &= ~KIRKWOOD_RECCTL_BURST_MASK; - value &= ~KIRKWOOD_RECCTL_MONO; - value &= ~(KIRKWOOD_RECCTL_PAUSE | KIRKWOOD_RECCTL_MUTE - | KIRKWOOD_RECCTL_SPDIF_EN); - - if (priv->burst == 32) - value |= KIRKWOOD_RECCTL_BURST_32; - else - value |= KIRKWOOD_RECCTL_BURST_128; - value |= KIRKWOOD_RECCTL_I2S_EN; - - writel(value, priv->io + KIRKWOOD_RECCTL); + /* enable record */ + writel(ctl, priv->io + KIRKWOOD_RECCTL); break;
case SNDRV_PCM_TRIGGER_STOP: @@ -448,6 +455,31 @@ static __devinit int kirkwood_i2s_dev_probe(struct platform_device *pdev) if (err < 0) return err;
+ priv->extclk = clk_get(&pdev->dev, "extclk"); + if (!IS_ERR(priv->extclk)) { + if (priv->extclk == priv->clk) { + clk_put(priv->extclk); + priv->extclk = ERR_PTR(-EINVAL); + } else { + dev_info(&pdev->dev, "found external clock\n"); + clk_prepare_enable(priv->extclk); + soc_dai = &kirkwood_i2s_dai_extclk; + } + } + + /* Some sensible defaults - this reflects the powerup values */ + priv->ctl_play = KIRKWOOD_PLAYCTL_SIZE_24; + priv->ctl_rec = KIRKWOOD_RECCTL_SIZE_24; + + /* Select the burst size */ + if (data->burst == 32) { + priv->ctl_play |= KIRKWOOD_PLAYCTL_BURST_32; + priv->ctl_rec |= KIRKWOOD_RECCTL_BURST_32; + } else { + priv->ctl_play |= KIRKWOOD_PLAYCTL_BURST_128; + priv->ctl_rec |= KIRKWOOD_RECCTL_BURST_128; + } + err = snd_soc_register_dai(&pdev->dev, &kirkwood_i2s_dai); if (!err) return 0; diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h index 7d92b9e..6e3b14a 100644 --- a/sound/soc/kirkwood/kirkwood.h +++ b/sound/soc/kirkwood/kirkwood.h @@ -120,6 +120,8 @@
struct kirkwood_dma_data { void __iomem *io; + uint32_t ctl_play; + uint32_t ctl_rec; int irq; int burst; struct clk *clk;
This is part of a patch found in Rabeeh Khoury's git tree for the cubox.
The kirkwood DMA hardware for ASoC does not impose any restrictions on the sample rates available, so it's silly to impose an artificial set in the DMA code. The restrictions come from the availble clocks to the I2S module, which are already handled in the I2S part of the driver.
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- sound/soc/kirkwood/kirkwood-dma.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/sound/soc/kirkwood/kirkwood-dma.c b/sound/soc/kirkwood/kirkwood-dma.c index 2ba0814..73c90c6 100644 --- a/sound/soc/kirkwood/kirkwood-dma.c +++ b/sound/soc/kirkwood/kirkwood-dma.c @@ -22,8 +22,10 @@ #include "kirkwood.h"
#define KIRKWOOD_RATES \ - (SNDRV_PCM_RATE_44100 | \ - SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000) + (SNDRV_PCM_RATE_8000_192000 | \ + SNDRV_PCM_RATE_CONTINUOUS | \ + SNDRV_PCM_RATE_KNOT) + #define KIRKWOOD_FORMATS \ (SNDRV_PCM_FMTBIT_S16_LE | \ SNDRV_PCM_FMTBIT_S24_LE | \ @@ -43,8 +45,8 @@ static struct snd_pcm_hardware kirkwood_dma_snd_hw = { SNDRV_PCM_INFO_PAUSE), .formats = KIRKWOOD_FORMATS, .rates = KIRKWOOD_RATES, - .rate_min = 44100, - .rate_max = 96000, + .rate_min = 8000, + .rate_max = 384000, .channels_min = 1, .channels_max = 2, .buffer_bytes_max = KIRKWOOD_SND_MAX_PERIOD_BYTES * KIRKWOOD_SND_MAX_PERIODS,
This is part of a patch found in Rabeeh Khoury's git tree for the cubox, and cleaned up by me.
Some platforms provide an external clock which can be used to allow other sample rates to be selected. Provide support for this.
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- sound/soc/kirkwood/kirkwood-i2s.c | 70 ++++++++++++++++++++++++++++++++---- sound/soc/kirkwood/kirkwood.h | 8 ++++- 2 files changed, 69 insertions(+), 9 deletions(-)
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index 823ef1e..d3629d5 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -99,6 +99,29 @@ static inline void kirkwood_set_dco(void __iomem *io, unsigned long rate) } while (value == 0); }
+static void kirkwood_set_rate(struct snd_soc_dai *dai, + struct kirkwood_dma_data *priv, unsigned long rate) +{ + uint32_t clks_ctrl; + + if (rate == 44100 || rate == 48000 || rate == 96000) { + /* use internal dco for supported rates */ + dev_dbg(dai->dev, "%s: dco set rate = %lu\n", + __func__, rate); + kirkwood_set_dco(priv->io, rate); + + clks_ctrl = KIRKWOOD_MCLK_SOURCE_DCO; + } else if (!IS_ERR(priv->extclk)) { + /* use optional external clk for other rates */ + dev_dbg(dai->dev, "%s: extclk set rate = %lu -> %lu\n", + __func__, rate, 256 * rate); + clk_set_rate(priv->extclk, 256 * rate); + + clks_ctrl = KIRKWOOD_MCLK_SOURCE_EXTCLK; + } + writel(clks_ctrl, priv->io + KIRKWOOD_CLOCKS_CTRL); +} + static int kirkwood_i2s_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { @@ -123,8 +146,7 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream, i2s_reg = KIRKWOOD_I2S_RECCTL; }
- /* set dco conf */ - kirkwood_set_dco(priv->io, params_rate(params)); + kirkwood_set_rate(dai, priv, params_rate(params));
i2s_value = readl(priv->io+i2s_reg); i2s_value &= ~KIRKWOOD_I2S_CTL_SIZE_MASK; @@ -396,21 +418,45 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai = { .channels_min = 1, .channels_max = 2, .rates = KIRKWOOD_I2S_RATES, - .formats = KIRKWOOD_I2S_FORMATS,}, + .formats = KIRKWOOD_I2S_FORMATS, + }, .capture = { .channels_min = 1, .channels_max = 2, .rates = KIRKWOOD_I2S_RATES, - .formats = KIRKWOOD_I2S_FORMATS,}, + .formats = KIRKWOOD_I2S_FORMATS, + }, + .ops = &kirkwood_i2s_dai_ops, +}; + +static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk = { + .probe = kirkwood_i2s_probe, + .remove = kirkwood_i2s_remove, + .playback = { + .channels_min = 1, + .channels_max = 2, + .rates = SNDRV_PCM_RATE_8000_192000 | + SNDRV_PCM_RATE_CONTINUOUS | + SNDRV_PCM_RATE_KNOT, + .formats = KIRKWOOD_I2S_FORMATS, + }, + .capture = { + .channels_min = 1, + .channels_max = 2, + .rates = SNDRV_PCM_RATE_8000_192000 | + SNDRV_PCM_RATE_CONTINUOUS | + SNDRV_PCM_RATE_KNOT, + .formats = KIRKWOOD_I2S_FORMATS, + }, .ops = &kirkwood_i2s_dai_ops, };
static __devinit int kirkwood_i2s_dev_probe(struct platform_device *pdev) { - struct resource *mem; - struct kirkwood_asoc_platform_data *data = - pdev->dev.platform_data; + struct kirkwood_asoc_platform_data *data = pdev->dev.platform_data; + struct snd_soc_dai_driver *soc_dai = &kirkwood_i2s_dai; struct kirkwood_dma_data *priv; + struct resource *mem; int err;
priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); @@ -480,11 +526,15 @@ static __devinit int kirkwood_i2s_dev_probe(struct platform_device *pdev) priv->ctl_rec |= KIRKWOOD_RECCTL_BURST_128; }
- err = snd_soc_register_dai(&pdev->dev, &kirkwood_i2s_dai); + err = snd_soc_register_dai(&pdev->dev, soc_dai); if (!err) return 0; dev_err(&pdev->dev, "snd_soc_register_dai failed\n");
+ if (!IS_ERR(priv->extclk)) { + clk_disable_unprepare(priv->extclk); + clk_put(priv->extclk); + } clk_disable_unprepare(priv->clk);
return err; @@ -496,6 +546,10 @@ static __devexit int kirkwood_i2s_dev_remove(struct platform_device *pdev)
snd_soc_unregister_dai(&pdev->dev);
+ if (!IS_ERR(priv->extclk)) { + clk_disable_unprepare(priv->extclk); + clk_put(priv->extclk); + } clk_disable_unprepare(priv->clk);
return 0; diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h index 6e3b14a..4d92637 100644 --- a/sound/soc/kirkwood/kirkwood.h +++ b/sound/soc/kirkwood/kirkwood.h @@ -77,6 +77,11 @@ #define KIRKWOOD_DCO_SPCR_STATUS 0x120c #define KIRKWOOD_DCO_SPCR_STATUS_DCO_LOCK (1<<16)
+#define KIRKWOOD_CLOCKS_CTRL 0x1230 +#define KIRKWOOD_MCLK_SOURCE_MASK (3<<0) +#define KIRKWOOD_MCLK_SOURCE_DCO (0<<0) +#define KIRKWOOD_MCLK_SOURCE_EXTCLK (3<<0) + #define KIRKWOOD_ERR_CAUSE 0x1300 #define KIRKWOOD_ERR_MASK 0x1304
@@ -120,11 +125,12 @@
struct kirkwood_dma_data { void __iomem *io; + struct clk *clk; + struct clk *extclk; uint32_t ctl_play; uint32_t ctl_rec; int irq; int burst; - struct clk *clk; };
#endif
Argh. Just noticed the driver author is _not_ listed in MAINTAINTERS. I guess the driver author doesn't care about receiving patches for his driver then... If Arnaud does care then that needs to be fixed. I will not be spamming everyone again with this patch set just because someone can't be bothered to ensure that they have appropriate MAINTAINERS entries. I'm sure they can be dug out of the alsa-devel archives instead.
On Tue, Nov 20, 2012 at 12:17:26PM +0000, Russell King - ARM Linux wrote:
The first five patches of this patch series fixes a number of problems with the Kirkwood audio driver, as seen on Dove platforms. The biggest issue is that they can cause the kernel to lock up solidly, requiring a reset to recover.
Other issues included in the first six patches are bugs found by the cubox folk.
The other five patches convert the I2S driver to use the devm_* APIs, improve the handling of the record and playback control registers so we're not reading and writing them as often, lift restrictions in the DMA driver which aren't necessary to impose, and add support for external clocks so that sample rates other than 44.1, 48 and 96kHz can be supported. Some of these changes prepare the driver to support the SPDIF output including passthrough mode; however, that support is not included in this patch set.
These patches probably need checking out on Kirkwood before they go into mainline, but given the severity of the kernel lockup, I suggest that once tested the first five patches go into -rc.
sound/soc/kirkwood/kirkwood-dma.c | 19 ++- sound/soc/kirkwood/kirkwood-i2s.c | 291 ++++++++++++++++++++++--------------- sound/soc/kirkwood/kirkwood.h | 11 +- 3 files changed, 197 insertions(+), 124 deletions(-)
ASoC: kirkwood-dma: remove channel restrictions ASoC: kirkwood-i2s: add support for external clock rates ASoC: kirkwood-dma: remove restriction on sample rates ASoC: kirkwood-i2s: better handling of play/record control registers ASoC: kirkwood-i2s: use devm_* APIs ASoC: kirkwood-i2s: more pause-mode fixes ASoC: kirkwood-i2s: fix DMA underruns ASoC: kirkwood-i2s: fix DCO lock detection ASoC: kirkwood-dma: don't ignore other irq causes on error ASoC: kirkwood-dma: fix use of virt_to_phys()
This is part of a patch found in Rabeeh Khoury's git tree for the cubox.
With SPDIF passthrough, we are not restricted to just two channels of audio; we can support however many channels the non-audio stream can itself support. In any case, kirkwood-dma is not involved in the format selection. So yet rid of this restriction.
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- sound/soc/kirkwood/kirkwood-dma.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/sound/soc/kirkwood/kirkwood-dma.c b/sound/soc/kirkwood/kirkwood-dma.c index 73c90c6..58d5a96 100644 --- a/sound/soc/kirkwood/kirkwood-dma.c +++ b/sound/soc/kirkwood/kirkwood-dma.c @@ -29,7 +29,9 @@ #define KIRKWOOD_FORMATS \ (SNDRV_PCM_FMTBIT_S16_LE | \ SNDRV_PCM_FMTBIT_S24_LE | \ - SNDRV_PCM_FMTBIT_S32_LE) + SNDRV_PCM_FMTBIT_S32_LE | \ + SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE | \ + SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_BE)
struct kirkwood_dma_priv { struct snd_pcm_substream *play_stream; @@ -48,7 +50,7 @@ static struct snd_pcm_hardware kirkwood_dma_snd_hw = { .rate_min = 8000, .rate_max = 384000, .channels_min = 1, - .channels_max = 2, + .channels_max = 8, .buffer_bytes_max = KIRKWOOD_SND_MAX_PERIOD_BYTES * KIRKWOOD_SND_MAX_PERIODS, .period_bytes_min = KIRKWOOD_SND_MIN_PERIOD_BYTES, .period_bytes_max = KIRKWOOD_SND_MAX_PERIOD_BYTES,
On Tue, Nov 20, 2012 at 12:17:26PM +0000, Russell King - ARM Linux wrote:
The first five patches of this patch series fixes a number of problems with the Kirkwood audio driver, as seen on Dove platforms. The biggest issue is that they can cause the kernel to lock up solidly, requiring a reset to recover.
Applied all, thanks.
On Wed, Nov 21, 2012 at 10:40:45AM +0900, Mark Brown wrote:
On Tue, Nov 20, 2012 at 12:17:26PM +0000, Russell King - ARM Linux wrote:
The first five patches of this patch series fixes a number of problems with the Kirkwood audio driver, as seen on Dove platforms. The biggest issue is that they can cause the kernel to lock up solidly, requiring a reset to recover.
Applied all, thanks.
I've been discussing this with Sebastian Hesselbarth and he's agreed to have his sign-off on a bunch of these patches... I guess not now.
On Wed, Nov 21, 2012 at 09:41:42AM +0000, Russell King - ARM Linux wrote:
I've been discussing this with Sebastian Hesselbarth and he's agreed to have his sign-off on a bunch of these patches... I guess not now.
It's on a separate branch which I've not produced a signed tag for so I can rebase and add if you/he send me something.
At Wed, 21 Nov 2012 18:47:21 +0900, Mark Brown wrote:
On Wed, Nov 21, 2012 at 09:41:42AM +0000, Russell King - ARM Linux wrote:
I've been discussing this with Sebastian Hesselbarth and he's agreed to have his sign-off on a bunch of these patches... I guess not now.
It's on a separate branch which I've not produced a signed tag for so I can rebase and add if you/he send me something.
Hm, isn't a branch/tag I already pulled in this morning?
Takashi
On Wed, Nov 21, 2012 at 10:59:49AM +0100, Takashi Iwai wrote:
Mark Brown wrote:
It's on a separate branch which I've not produced a signed tag for so I can rebase and add if you/he send me something.
Hm, isn't a branch/tag I already pulled in this morning?
Gah, right - sorry. Half of it is in what you pulled, half isn't. The extra stuff I can still fix up.
On Wed, Nov 21, 2012 at 06:47:21PM +0900, Mark Brown wrote:
On Wed, Nov 21, 2012 at 09:41:42AM +0000, Russell King - ARM Linux wrote:
I've been discussing this with Sebastian Hesselbarth and he's agreed to have his sign-off on a bunch of these patches... I guess not now.
It's on a separate branch which I've not produced a signed tag for so I can rebase and add if you/he send me something.
That's a good idea; I did say that the patches should be tested on Kirkwood first. Also, Sebastian has been doing his own independent testing of them last night... unsuccessfully from what I read on IRC, but that may not be due to the patches themselves but the amount of churn going on with the Dove architecture having broken something else in the clk stuff.
So you might get some tested-by's at some point, or you might be asked to hold off on them.
Also remember that Arnaud hasn't seen these patches (unless they got through the alsa mailing list moderation and he's looked them up in the archives.)
participants (4)
-
Mark Brown -
Russell King -
Russell King - ARM Linux -
Takashi Iwai