[alsa-devel] [PATCH 0/7] Kirkwood ASoC updates
Mark,
Here are a set of patches I've had for a while as part of my DPCM work. I think it's worth getting these merged.
The first patch fixes a bug at ASoC level with debugfs handling - it is not permitted to create more than one debugfs directory with the same name in the same parent directory - and this can happen when the dummy platform and codec are used.
The second patch adds a helper definition to handle both I2S and SPDIF bits in the RECCTL register in the same way that we already do with the PLAYCTL register.
Patch 3 fixes the RECCTL enable masking - we would accumulate enable bits but never clear any of them.
Patch 4 makes the driver's handling of the PLAYCTL register slightly more conformant to the documentation - which requires that the appropriate mute bits are set while the respective output interface is disabled.
Patch 5 updates my previous attempt to properly fix this. The previous attempt did stop things going wrong from the hardware perspective, but we are left with the occasional stuck-busy. Although that was harmless, this patch stops this occuring in addition to the previous fix.
Patch 6 lifts some of the driver restrictions on the period size and number of periods - there's no reason to restrict these values, so let's give userspace more flexibility.
Patch 7 adds NO_PERIOD_WAKEUP support for kirkwood, permitting interrupt-less operation.
Avoid creating duplicate directories by prefixing codecs and platforms with their separate identifiers. This avoids snd-soc-dummy (which can appear both as a dummy platform and a dummy codec on the same card) from clashing.
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- sound/soc/soc-core.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index b87d7d882e6d..91120b8e283e 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -270,12 +270,32 @@ static const struct file_operations codec_reg_fops = { .llseek = default_llseek, };
+static struct dentry *soc_debugfs_create_dir(struct dentry *parent, + const char *fmt, ...) +{ + struct dentry *de; + va_list ap; + char *s; + + va_start(ap, fmt); + s = kvasprintf(GFP_KERNEL, fmt, ap); + va_end(ap); + + if (!s) + return NULL; + + de = debugfs_create_dir(s, parent); + kfree(s); + + return de; +} + static void soc_init_codec_debugfs(struct snd_soc_codec *codec) { struct dentry *debugfs_card_root = codec->card->debugfs_card_root;
- codec->debugfs_codec_root = debugfs_create_dir(codec->name, - debugfs_card_root); + codec->debugfs_codec_root = soc_debugfs_create_dir(debugfs_card_root, + "codec:%s", codec->name); if (!codec->debugfs_codec_root) { dev_warn(codec->dev, "ASoC: Failed to create codec debugfs directory\n"); @@ -306,8 +326,8 @@ static void soc_init_platform_debugfs(struct snd_soc_platform *platform) { struct dentry *debugfs_card_root = platform->card->debugfs_card_root;
- platform->debugfs_platform_root = debugfs_create_dir(platform->name, - debugfs_card_root); + platform->debugfs_platform_root = soc_debugfs_create_dir(debugfs_card_root, + "platform:%s", platform->name); if (!platform->debugfs_platform_root) { dev_warn(platform->dev, "ASoC: Failed to create platform debugfs directory\n");
On 06/26/2014 04:22 PM, Russell King wrote:
Avoid creating duplicate directories by prefixing codecs and platforms with their separate identifiers. This avoids snd-soc-dummy (which can appear both as a dummy platform and a dummy codec on the same card) from clashing.
Do we actually want to create debugfs entries for the snd-soc-dummies? There shouldn't be any meaningful information in there.
If we want to and if we change the naming scheme of the debugfs entries anyway we should also add the component name to the entry name. This fixes a issue with devices that register multiple components.
The other thing is that we are working on generalizing the ASoC code and getting rid of the distinction between CODECs and platforms. Which means there won't really a way to get the prefix anymore soon.
- Lars
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk
sound/soc/soc-core.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index b87d7d882e6d..91120b8e283e 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -270,12 +270,32 @@ static const struct file_operations codec_reg_fops = { .llseek = default_llseek, };
+static struct dentry *soc_debugfs_create_dir(struct dentry *parent,
- const char *fmt, ...)
+{
- struct dentry *de;
- va_list ap;
- char *s;
- va_start(ap, fmt);
- s = kvasprintf(GFP_KERNEL, fmt, ap);
- va_end(ap);
- if (!s)
return NULL;- de = debugfs_create_dir(s, parent);
- kfree(s);
- return de;
+}
- static void soc_init_codec_debugfs(struct snd_soc_codec *codec) { struct dentry *debugfs_card_root = codec->card->debugfs_card_root;
- codec->debugfs_codec_root = debugfs_create_dir(codec->name,
debugfs_card_root);
- codec->debugfs_codec_root = soc_debugfs_create_dir(debugfs_card_root,
"codec:%s", codec->name);@@ -306,8 +326,8 @@ static void soc_init_platform_debugfs(struct snd_soc_platform *platform) { struct dentry *debugfs_card_root = platform->card->debugfs_card_root;
- platform->debugfs_platform_root = debugfs_create_dir(platform->name,
debugfs_card_root);
- platform->debugfs_platform_root = soc_debugfs_create_dir(debugfs_card_root,
"platform:%s", platform->name);
On Fri, Jun 27, 2014 at 04:45:11AM +0200, Lars-Peter Clausen wrote:
On 06/26/2014 04:22 PM, Russell King wrote:
Avoid creating duplicate directories by prefixing codecs and platforms with their separate identifiers. This avoids snd-soc-dummy (which can appear both as a dummy platform and a dummy codec on the same card) from clashing.
Do we actually want to create debugfs entries for the snd-soc-dummies? There shouldn't be any meaningful information in there.
On the one hand they're virtual things that we probably should be hiding but then again part of what we're trying to do is stub things out as they would be normally so special casing potentically causes problems.
If we want to and if we change the naming scheme of the debugfs entries anyway we should also add the component name to the entry name. This fixes a issue with devices that register multiple components.
The other thing is that we are working on generalizing the ASoC code and getting rid of the distinction between CODECs and platforms. Which means there won't really a way to get the prefix anymore soon.
Right, these are both issues. However I think the patch is OK for now, it does avoid an actual problem so I'll apply it for now - we can further improve it later.
Add a KIRKWOOD_RECCTL_ENABLE_MASK definition to complement the existing PLAYCTL definition, and make use of it where we wish to clear both enable bits.
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- sound/soc/kirkwood/kirkwood-i2s.c | 7 +++---- sound/soc/kirkwood/kirkwood.h | 3 +++ 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index 9f842222e798..55af6c8e4603 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -322,8 +322,7 @@ static int kirkwood_i2s_rec_trigger(struct snd_pcm_substream *substream, else ctl &= ~KIRKWOOD_RECCTL_I2S_EN; /* spdif */
- value = ctl & ~(KIRKWOOD_RECCTL_I2S_EN | - KIRKWOOD_RECCTL_SPDIF_EN); + value = ctl & ~KIRKWOOD_RECCTL_ENABLE_MASK; writel(value, priv->io + KIRKWOOD_RECCTL);
/* enable interrupts */ @@ -347,7 +346,7 @@ static int kirkwood_i2s_rec_trigger(struct snd_pcm_substream *substream,
/* disable all records */ value = readl(priv->io + KIRKWOOD_RECCTL); - value &= ~(KIRKWOOD_RECCTL_I2S_EN | KIRKWOOD_RECCTL_SPDIF_EN); + value &= ~KIRKWOOD_RECCTL_ENABLE_MASK; writel(value, priv->io + KIRKWOOD_RECCTL); break;
@@ -411,7 +410,7 @@ static int kirkwood_i2s_init(struct kirkwood_dma_data *priv) writel(value, priv->io + KIRKWOOD_PLAYCTL);
value = readl(priv->io + KIRKWOOD_RECCTL); - value &= ~(KIRKWOOD_RECCTL_I2S_EN | KIRKWOOD_RECCTL_SPDIF_EN); + value &= ~KIRKWOOD_RECCTL_ENABLE_MASK; writel(value, priv->io + KIRKWOOD_RECCTL);
return 0; diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h index bf23afbba1d7..ab21de090938 100644 --- a/sound/soc/kirkwood/kirkwood.h +++ b/sound/soc/kirkwood/kirkwood.h @@ -38,6 +38,9 @@ #define KIRKWOOD_RECCTL_SIZE_24 (1<<0) #define KIRKWOOD_RECCTL_SIZE_32 (0<<0)
+#define KIRKWOOD_RECCTL_ENABLE_MASK (KIRKWOOD_RECCTL_SPDIF_EN | \ + KIRKWOOD_RECCTL_I2S_EN) + #define KIRKWOOD_REC_BUF_ADDR 0x1004 #define KIRKWOOD_REC_BUF_SIZE 0x1008 #define KIRKWOOD_REC_BYTE_COUNT 0x100C
Since we wish to disable capture inputs for some formats, we need to ensure that we clear the enable bits in our cached record control register. This seems to have been missed, resulting in the register only accumulating enable bits.
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- sound/soc/kirkwood/kirkwood-i2s.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index 55af6c8e4603..ef1a164d8703 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -212,7 +212,8 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream, KIRKWOOD_PLAYCTL_SIZE_MASK); priv->ctl_play |= ctl_play; } else { - priv->ctl_rec &= ~KIRKWOOD_RECCTL_SIZE_MASK; + priv->ctl_rec &= ~(KIRKWOOD_RECCTL_ENABLE_MASK | + KIRKWOOD_RECCTL_SIZE_MASK); priv->ctl_rec |= ctl_rec; }
The spec requires that the mute bits must be set while the channel is disabled. Ensure that this is the case by providing a helper which ensures that the appropriate mute bit is set while the enable bit is clear.
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- sound/soc/kirkwood/kirkwood-i2s.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index ef1a164d8703..b601ad680d7b 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -222,6 +222,15 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream, return 0; }
+static unsigned kirkwood_i2s_play_mute(unsigned ctl) +{ + if (!(ctl & KIRKWOOD_PLAYCTL_I2S_EN)) + ctl |= KIRKWOOD_PLAYCTL_I2S_MUTE; + if (!(ctl & KIRKWOOD_PLAYCTL_SPDIF_EN)) + ctl |= KIRKWOOD_PLAYCTL_SPDIF_MUTE; + return ctl; +} + static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) { @@ -257,7 +266,7 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream, ctl &= ~KIRKWOOD_PLAYCTL_SPDIF_EN; /* i2s */ else ctl &= ~KIRKWOOD_PLAYCTL_I2S_EN; /* spdif */ - + ctl = kirkwood_i2s_play_mute(ctl); value = ctl & ~KIRKWOOD_PLAYCTL_ENABLE_MASK; writel(value, priv->io + KIRKWOOD_PLAYCTL);
@@ -296,6 +305,7 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream, case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: ctl &= ~(KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE | KIRKWOOD_PLAYCTL_SPDIF_MUTE); + ctl = kirkwood_i2s_play_mute(ctl); writel(ctl, priv->io + KIRKWOOD_PLAYCTL); break;
We still see the occasional timeout waiting for busy to clear. As the spec is contradictory, and we know that the current implementation doesn't work, try an alternative interpretation from the spec. This one appears to work - I have yet to find any issue with it during my testing over several months.
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- sound/soc/kirkwood/kirkwood-i2s.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index b601ad680d7b..e98650c01eba 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -238,7 +238,7 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream, uint32_t ctl, value;
ctl = readl(priv->io + KIRKWOOD_PLAYCTL); - if (ctl & KIRKWOOD_PLAYCTL_PAUSE) { + if ((ctl & KIRKWOOD_PLAYCTL_ENABLE_MASK) == 0) { unsigned timeout = 5000; /* * The Armada510 spec says that if we enter pause mode, the
There is no hardware restriction requiring a minimum of 8 periods, or a minimum of 2048 bytes in a period. Let's drop these values so that userspace has more flexibility in choosing these parameters.
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- sound/soc/kirkwood/kirkwood.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h index ab21de090938..90e32a781424 100644 --- a/sound/soc/kirkwood/kirkwood.h +++ b/sound/soc/kirkwood/kirkwood.h @@ -124,9 +124,9 @@
/* Theses values come from the marvell alsa driver */ /* need to find where they come from */ -#define KIRKWOOD_SND_MIN_PERIODS 8 +#define KIRKWOOD_SND_MIN_PERIODS 2 #define KIRKWOOD_SND_MAX_PERIODS 16 -#define KIRKWOOD_SND_MIN_PERIOD_BYTES 0x800 +#define KIRKWOOD_SND_MIN_PERIOD_BYTES 256 #define KIRKWOOD_SND_MAX_PERIOD_BYTES 0x8000 #define KIRKWOOD_SND_MAX_BUFFER_BYTES (KIRKWOOD_SND_MAX_PERIOD_BYTES \ * KIRKWOOD_SND_MAX_PERIODS)
Permit ALSA to run without hardware interrupts from the audio interface. Instead, ALSA will use a kernel timer to decide when to check the buffer state, resulting in a lighter workload for the CPU.
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- sound/soc/kirkwood/kirkwood-dma.c | 11 ++++++----- sound/soc/kirkwood/kirkwood-i2s.c | 9 ++++++--- 2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/sound/soc/kirkwood/kirkwood-dma.c b/sound/soc/kirkwood/kirkwood-dma.c index aac22fccdcdc..4cf2245950d7 100644 --- a/sound/soc/kirkwood/kirkwood-dma.c +++ b/sound/soc/kirkwood/kirkwood-dma.c @@ -28,11 +28,12 @@ static struct kirkwood_dma_data *kirkwood_priv(struct snd_pcm_substream *subs) }
static struct snd_pcm_hardware kirkwood_dma_snd_hw = { - .info = (SNDRV_PCM_INFO_INTERLEAVED | - SNDRV_PCM_INFO_MMAP | - SNDRV_PCM_INFO_MMAP_VALID | - SNDRV_PCM_INFO_BLOCK_TRANSFER | - SNDRV_PCM_INFO_PAUSE), + .info = SNDRV_PCM_INFO_INTERLEAVED | + SNDRV_PCM_INFO_MMAP | + SNDRV_PCM_INFO_MMAP_VALID | + SNDRV_PCM_INFO_BLOCK_TRANSFER | + SNDRV_PCM_INFO_PAUSE | + SNDRV_PCM_INFO_NO_PERIOD_WAKEUP, .buffer_bytes_max = KIRKWOOD_SND_MAX_BUFFER_BYTES, .period_bytes_min = KIRKWOOD_SND_MIN_PERIOD_BYTES, .period_bytes_max = KIRKWOOD_SND_MAX_PERIOD_BYTES, diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index e98650c01eba..0704cd6d2314 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -234,6 +234,7 @@ static unsigned kirkwood_i2s_play_mute(unsigned ctl) static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) { + struct snd_pcm_runtime *runtime = substream->runtime; struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai); uint32_t ctl, value;
@@ -271,9 +272,11 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream, 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); + if (!runtime->no_period_wakeup) { + value = readl(priv->io + KIRKWOOD_INT_MASK); + value |= KIRKWOOD_INT_CAUSE_PLAY_BYTES; + writel(value, priv->io + KIRKWOOD_INT_MASK); + }
/* enable playback */ writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
On Thu, Jun 26, 2014 at 03:22:31PM +0100, Russell King - ARM Linux wrote:
Mark,
Here are a set of patches I've had for a while as part of my DPCM work. I think it's worth getting these merged.
Hi Russell, Mark
I tested simple playback of WAV and MP3 files on a Kirkwood based HP T5325 machine. No obvious regressions.
Tested-by: Andrew Lunn andrew@lunn.ch
Patch 7 adds NO_PERIOD_WAKEUP support for kirkwood, permitting interrupt-less operation.
When using mpg123 to play back mp3 files, i get about 9 interrupts per second for kirkwood-i2s. Does this sound right?
Andrew
On Thu, Jun 26, 2014 at 08:10:12PM +0200, Andrew Lunn wrote:
On Thu, Jun 26, 2014 at 03:22:31PM +0100, Russell King - ARM Linux wrote:
Mark,
Here are a set of patches I've had for a while as part of my DPCM work. I think it's worth getting these merged.
Hi Russell, Mark
I tested simple playback of WAV and MP3 files on a Kirkwood based HP T5325 machine. No obvious regressions.
Tested-by: Andrew Lunn andrew@lunn.ch
Patch 7 adds NO_PERIOD_WAKEUP support for kirkwood, permitting interrupt-less operation.
When using mpg123 to play back mp3 files, i get about 9 interrupts per second for kirkwood-i2s. Does this sound right?
Whether this feature is taken advantage of depends on the application. For example, with pulseaudio, I get:
21: 0 main-interrupt-ctrl 21 kirkwood-i2s
participants (5)
-
Andrew Lunn -
Lars-Peter Clausen -
Mark Brown -
Russell King -
Russell King - ARM Linux