[PATCH] ASoC: samsung-i2s: Maintain CDCLK settings across i2s_{shutdown/startup}

[PATCH] ASoC: samsung-i2s: Maintain CDCLK settings across i2s_{shutdown/startup}

[Sylwester Nawrocki]

Currently configuration of the CDCLK pad is being overwritten in
the i2s_shutdown() callback in order to gate the SoC output clock.
However if an ASoC machine driver doesn't restore that clock
settings each time after opening the sound device this results
in the CDCLK pin being permanently configured into input mode.
I.e. the output clock will always stay disabled.
Fix that by saving the CDCLKCON bit state in i2s_shutdown() and
and restoring it in the i2s_startup() callback.

Signed-off-by: Chen Zhen <zhen1.chen@samsung.com>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
 sound/soc/samsung/i2s.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
index 2ac76fa..3cb3e95 100644
--- a/sound/soc/samsung/i2s.c
+++ b/sound/soc/samsung/i2s.c
@@ -68,6 +68,8 @@ struct i2s_dai {
 #define DAI_OPENED	(1 << 0) /* Dai is opened */
 #define DAI_MANAGER	(1 << 1) /* Dai is the manager */
 	unsigned mode;
+	/* CDCLK pin direction: 0  - input, 1 - output */
+	unsigned int cdclk_out:1;
 	/* Driver for this DAI */
 	struct snd_soc_dai_driver i2s_dai_drv;
 	/* DMA parameters */
@@ -737,6 +739,9 @@ static int i2s_startup(struct snd_pcm_substream *substream,
 
 	spin_unlock_irqrestore(&lock, flags);
 
+	if (!is_opened(other) && i2s->cdclk_out)
+		i2s_set_sysclk(dai, SAMSUNG_I2S_CDCLK,
+				0, SND_SOC_CLOCK_OUT);
 	return 0;
 }
 
@@ -752,9 +757,13 @@ static void i2s_shutdown(struct snd_pcm_substream *substream,
 	i2s->mode &= ~DAI_OPENED;
 	i2s->mode &= ~DAI_MANAGER;
 
-	if (is_opened(other))
+	if (is_opened(other)) {
 		other->mode |= DAI_MANAGER;
-
+	} else {
+		u32 mod = readl(i2s->addr + I2SMOD);
+		i2s->cdclk_out = !(mod & MOD_CDCLKCON);
+		other->cdclk_out = i2s->cdclk_out;
+	}
 	/* Reset any constraint on RFS and BFS */
 	i2s->rfs = 0;
 	i2s->bfs = 0;
-- 
1.7.9.5

[PATCH] ASoC: samsung-i2s: Maintain CDCLK settings across i2s_{shutdown/startup}

[Sylwester Nawrocki]

Currently configuration of the CDCLK pad is being overwritten in
the i2s_shutdown() callback in order to gate the SoC output clock.
However if an ASoC machine driver doesn't restore that clock
settings each time after opening the sound device this results
in the CDCLK pin being permanently configured into input mode.
I.e. the output clock will always stay disabled.
Fix that by saving the CDCLKCON bit state in i2s_shutdown() and
and restoring it in the i2s_startup() callback.

Signed-off-by: Chen Zhen <zhen1.chen@samsung.com>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
 sound/soc/samsung/i2s.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
index 2ac76fa..3cb3e95 100644
--- a/sound/soc/samsung/i2s.c
+++ b/sound/soc/samsung/i2s.c
@@ -68,6 +68,8 @@ struct i2s_dai {
 #define DAI_OPENED	(1 << 0) /* Dai is opened */
 #define DAI_MANAGER	(1 << 1) /* Dai is the manager */
 	unsigned mode;
+	/* CDCLK pin direction: 0  - input, 1 - output */
+	unsigned int cdclk_out:1;
 	/* Driver for this DAI */
 	struct snd_soc_dai_driver i2s_dai_drv;
 	/* DMA parameters */
@@ -737,6 +739,9 @@ static int i2s_startup(struct snd_pcm_substream *substream,
 
 	spin_unlock_irqrestore(&lock, flags);
 
+	if (!is_opened(other) && i2s->cdclk_out)
+		i2s_set_sysclk(dai, SAMSUNG_I2S_CDCLK,
+				0, SND_SOC_CLOCK_OUT);
 	return 0;
 }
 
@@ -752,9 +757,13 @@ static void i2s_shutdown(struct snd_pcm_substream *substream,
 	i2s->mode &= ~DAI_OPENED;
 	i2s->mode &= ~DAI_MANAGER;
 
-	if (is_opened(other))
+	if (is_opened(other)) {
 		other->mode |= DAI_MANAGER;
-
+	} else {
+		u32 mod = readl(i2s->addr + I2SMOD);
+		i2s->cdclk_out = !(mod & MOD_CDCLKCON);
+		other->cdclk_out = i2s->cdclk_out;
+	}
 	/* Reset any constraint on RFS and BFS */
 	i2s->rfs = 0;
 	i2s->bfs = 0;
-- 
1.7.9.5

Re: [PATCH] ASoC: samsung-i2s: Maintain CDCLK settings across i2s_{shutdown/startup}

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 222 bytes --]

On Thu, Jul 10, 2014 at 06:11:13PM +0200, Sylwester Nawrocki wrote:
> Currently configuration of the CDCLK pad is being overwritten in
> the i2s_shutdown() callback in order to gate the SoC output clock.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] ASoC: samsung-i2s: Maintain CDCLK settings across i2s_{shutdown/startup}

[Mark Brown]

On Thu, Jul 10, 2014 at 06:11:13PM +0200, Sylwester Nawrocki wrote:
> Currently configuration of the CDCLK pad is being overwritten in
> the i2s_shutdown() callback in order to gate the SoC output clock.

Applied, thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140711/dd0f93e2/attachment-0001.sig>

Re: [PATCH] ASoC: samsung-i2s: Maintain CDCLK settings across i2s_{shutdown/startup}

[Daniel Drake]

Hi Sylwester,

On Thu, Jul 10, 2014 at 10:11 AM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> Currently configuration of the CDCLK pad is being overwritten in
> the i2s_shutdown() callback in order to gate the SoC output clock.
> However if an ASoC machine driver doesn't restore that clock
> settings each time after opening the sound device this results
> in the CDCLK pin being permanently configured into input mode.
> I.e. the output clock will always stay disabled.
> Fix that by saving the CDCLKCON bit state in i2s_shutdown() and
> and restoring it in the i2s_startup() callback.

I'm still having trouble in this area on ODROID. Basically, if you
start pulseaudio, all audio breaks. Even after you kill pulseaudio,
audio is still broken.
This happens because CDCLK gets disabled by i2s.c and never enabled again.

pulseaudio does:
1. i2s_startup for playback channel
2. i2s_startup for capture channel
3. i2s_shutdown for capture channel
4. i2s_shutdown for playback channel

In step 3 we disable CDCLK even though playback should still be active, oops.

In step 4 we do this:
        u32 mod = readl(i2s->addr + I2SMOD);
        i2s->cdclk_out = !(mod & MOD_CDCLKCON);

and now cdclk_out is always going to be 0, so we'll never turn it back on again.

Regardless of what we do now, I think there is a bug in i2s_shutdown
in that it should not do any real shutdown stuff if the other stream
(playback/capture) is open.

However I'm wondering if we should more thoroughly clean up CDCLK
handling. Right now CDCLK is enabled during boot in
odroidx2_late_probe() (even if you never play any audio), and then
sometimes disabled by i2s.c after the sound device has been opened and
then closed, with a half-broken attempt to sometimes enable it again
next time it is opened. We're in this situation because that setup is
pretty fragile and confusing...

Is CDCLK something ODROID-specific? Perhaps we could have
startup/shutdown hooks in odroidx2_max98090.c that start and stop the
clock (with proper refcounting), and remove CDCLK interaction from
i2s.c.

Is CDCLK something more generic for Samsung i2s devices? In that case
we could enable it in i2s_startup, disable it in i2s_shutdown, and
avoid interacting with it at all from odroidx2_max98090.c. (again with
proper refcounting)

Thanks
Daniel

[PATCH] ASoC: samsung-i2s: Maintain CDCLK settings across i2s_{shutdown/startup}

[Daniel Drake]

Hi Sylwester,

On Thu, Jul 10, 2014 at 10:11 AM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> Currently configuration of the CDCLK pad is being overwritten in
> the i2s_shutdown() callback in order to gate the SoC output clock.
> However if an ASoC machine driver doesn't restore that clock
> settings each time after opening the sound device this results
> in the CDCLK pin being permanently configured into input mode.
> I.e. the output clock will always stay disabled.
> Fix that by saving the CDCLKCON bit state in i2s_shutdown() and
> and restoring it in the i2s_startup() callback.

I'm still having trouble in this area on ODROID. Basically, if you
start pulseaudio, all audio breaks. Even after you kill pulseaudio,
audio is still broken.
This happens because CDCLK gets disabled by i2s.c and never enabled again.

pulseaudio does:
1. i2s_startup for playback channel
2. i2s_startup for capture channel
3. i2s_shutdown for capture channel
4. i2s_shutdown for playback channel

In step 3 we disable CDCLK even though playback should still be active, oops.

In step 4 we do this:
        u32 mod = readl(i2s->addr + I2SMOD);
        i2s->cdclk_out = !(mod & MOD_CDCLKCON);

and now cdclk_out is always going to be 0, so we'll never turn it back on again.

Regardless of what we do now, I think there is a bug in i2s_shutdown
in that it should not do any real shutdown stuff if the other stream
(playback/capture) is open.

However I'm wondering if we should more thoroughly clean up CDCLK
handling. Right now CDCLK is enabled during boot in
odroidx2_late_probe() (even if you never play any audio), and then
sometimes disabled by i2s.c after the sound device has been opened and
then closed, with a half-broken attempt to sometimes enable it again
next time it is opened. We're in this situation because that setup is
pretty fragile and confusing...

Is CDCLK something ODROID-specific? Perhaps we could have
startup/shutdown hooks in odroidx2_max98090.c that start and stop the
clock (with proper refcounting), and remove CDCLK interaction from
i2s.c.

Is CDCLK something more generic for Samsung i2s devices? In that case
we could enable it in i2s_startup, disable it in i2s_shutdown, and
avoid interacting with it at all from odroidx2_max98090.c. (again with
proper refcounting)

Thanks
Daniel