* [PATCH 03/10] S3C64XX I2S: Codec Clock Gating Option
@ 2009-09-15 10:02 Jassi
2009-09-15 10:44 ` Mark Brown
0 siblings, 1 reply; 6+ messages in thread
From: Jassi @ 2009-09-15 10:02 UTC (permalink / raw)
To: linux-arm-kernel
In SoC-Master mode, CODCLK maybe gated out as MCLK of the CODEC.
In SoC-Slave mode, CODCLK needs to be cut-off from PAD inorder
to avoid collision with MCLK from CODEC.
Option to gate/block CODCLK is implemented in this patch.
Signed-Off-by: Jassi <jassi.brar@samsung.com>
---
arch/arm/plat-s3c/include/plat/regs-s3c2412-iis.h | 2 ++
sound/soc/s3c24xx/s3c64xx-i2s.c | 8 ++++++++
sound/soc/s3c24xx/s3c64xx-i2s.h | 2 ++
3 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/arch/arm/plat-s3c/include/plat/regs-s3c2412-iis.h b/arch/arm/plat-s3c/include/plat/regs-s3c2412-iis.h
index 07659da..08ae50f 100644
--- a/arch/arm/plat-s3c/include/plat/regs-s3c2412-iis.h
+++ b/arch/arm/plat-s3c/include/plat/regs-s3c2412-iis.h
@@ -38,6 +38,8 @@
#define S3C64XX_IISMOD_BLC_24BIT (2 << 13)
#define S3C64XX_IISMOD_BLC_MASK (3 << 13)
+#define S3C64XX_IISMOD_CDCLK_EXT (1 << 12)
+
#define S3C64XX_IISMOD_IMS_PCLK (0 << 10)
#define S3C64XX_IISMOD_IMS_SYSMUX (1 << 10)
diff --git a/sound/soc/s3c24xx/s3c64xx-i2s.c b/sound/soc/s3c24xx/s3c64xx-i2s.c
index 3c06c40..6f8959d 100644
--- a/sound/soc/s3c24xx/s3c64xx-i2s.c
+++ b/sound/soc/s3c24xx/s3c64xx-i2s.c
@@ -99,6 +99,14 @@ static int s3c64xx_i2s_set_sysclk(struct snd_soc_dai *cpu_dai,
iismod |= S3C64XX_IISMOD_IMS_SYSMUX;
break;
+ case S3C64XX_CODCLKSRC_INT:
+ iismod &= ~S3C64XX_IISMOD_CDCLK_EXT;
+ break;
+
+ case S3C64XX_CODCLKSRC_EXT:
+ iismod |= S3C64XX_IISMOD_CDCLK_EXT;
+ break;
+
default:
return -EINVAL;
}
diff --git a/sound/soc/s3c24xx/s3c64xx-i2s.h b/sound/soc/s3c24xx/s3c64xx-i2s.h
index 02148ce..4197da5 100644
--- a/sound/soc/s3c24xx/s3c64xx-i2s.h
+++ b/sound/soc/s3c24xx/s3c64xx-i2s.h
@@ -25,6 +25,8 @@ struct clk;
#define S3C64XX_CLKSRC_PCLK (0)
#define S3C64XX_CLKSRC_MUX (1)
+#define S3C64XX_CODCLKSRC_INT (2)
+#define S3C64XX_CODCLKSRC_EXT (3)
extern struct snd_soc_dai s3c64xx_i2s_dai[];
--
1.6.2.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 03/10] S3C64XX I2S: Codec Clock Gating Option
2009-09-15 10:02 [PATCH 03/10] S3C64XX I2S: Codec Clock Gating Option Jassi
@ 2009-09-15 10:44 ` Mark Brown
2009-09-16 1:37 ` jassi brar
0 siblings, 1 reply; 6+ messages in thread
From: Mark Brown @ 2009-09-15 10:44 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Sep 15, 2009 at 07:02:35PM +0900, Jassi wrote:
> In SoC-Master mode, CODCLK maybe gated out as MCLK of the CODEC.
> In SoC-Slave mode, CODCLK needs to be cut-off from PAD inorder
> to avoid collision with MCLK from CODEC.
> Option to gate/block CODCLK is implemented in this patch.
Just to confuse matters the datasheet appears to use both CODCLK and
CDCLK to refer to the same signal (as far as I can tell, anyway -
there's only a couple of references to CODCLK). The drivers are
currently using CDCLK as the name, it'd be better to stick with just
one.
In terms of functionality it might be clearer to describe what's going
on here as switching the function of CDCLK from being an input to being
an output. The actual implementation appears to be done by gating the
output of the internal clock signal to the CDCLK pin but in terms of
what the user can see externally the clock isn't gated, it's still
active but it changed direction.
> +#define S3C64XX_IISMOD_CDCLK_EXT (1 << 12)
> +
The datasheet calls this bit CDLKCON - it'd be better to preserve that
naming unless there is a good reason to diverge?
> @@ -99,6 +99,14 @@ static int s3c64xx_i2s_set_sysclk(struct snd_soc_dai *cpu_dai,
> iismod |= S3C64XX_IISMOD_IMS_SYSMUX;
> break;
>
> + case S3C64XX_CODCLKSRC_INT:
> + iismod &= ~S3C64XX_IISMOD_CDCLK_EXT;
> + break;
> +
> + case S3C64XX_CODCLKSRC_EXT:
> + iismod |= S3C64XX_IISMOD_CDCLK_EXT;
> + break;
> +
This should really be done using the direction parameter - clk_id should
specify that CDCLK should be used and the direction parameter be used to
say if it's an input or output.
Ideally this would also be more joined up with the rest of the
configuration so the machine driver could just specify the master clock
for the IIS block and have all the clock switching (the system
controller mux and IMS) updated appropriately. That should make it
easier for users to set up the clocking since they don't need to worry
about the implementation details of the IIS block.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 03/10] S3C64XX I2S: Codec Clock Gating Option
2009-09-15 10:44 ` Mark Brown
@ 2009-09-16 1:37 ` jassi brar
2009-09-16 11:02 ` Mark Brown
0 siblings, 1 reply; 6+ messages in thread
From: jassi brar @ 2009-09-16 1:37 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Sep 15, 2009 at 7:44 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Sep 15, 2009 at 07:02:35PM +0900, Jassi wrote:
>> In SoC-Master mode, CODCLK maybe gated out as MCLK of the CODEC.
>> In SoC-Slave mode, CODCLK needs to be cut-off from PAD inorder
>> to avoid collision with MCLK from CODEC.
>> Option to gate/block CODCLK is implemented in this patch.
> In terms of functionality it might be clearer to describe what's going
> on here as switching the function of CDCLK from being an input to being
> an output. ?The actual implementation appears to be done by gating the
> output of the internal clock signal to the CDCLK pin but in terms of
> what the user can see externally the clock isn't gated, it's still
> active but it changed direction.
The bit IISMOD[12], if cleared, gates the RCLK towards the Xi2sCDCLK.
If this bit is set, it simply disconnects the RCLK and Xi2sCDCLK.
If the I2S controller wants to use clock from Xi2sCDCLK via
"audio-bus" through the MUX, then we want RCLK to be disconnected and
we set this bit 1.
If we provide the MCLK of the CODEC from RCLK of I2S controller, we
connect RCLK to Xi2sCDCLK which is usually connected to the MCLK of
the CODEC, In this case we gate RCLK by clearing this bit (note that
it is a not-gate)
>> +#define S3C64XX_IISMOD_CDCLK_EXT ? ? (1 << 12)
>> +
>
> The datasheet calls this bit CDLKCON - it'd be better to preserve that
> naming unless there is a good reason to diverge?
CON is better used for register defines.
This is but one bit: CodecClock is Internal or External.
So, yes, minor but some reason for that.
Btw, I have already notified this issue to our technical writers.
>> @@ -99,6 +99,14 @@ static int s3c64xx_i2s_set_sysclk(struct snd_soc_dai *cpu_dai,
>> ? ? ? ? ? ? ? iismod |= S3C64XX_IISMOD_IMS_SYSMUX;
>> ? ? ? ? ? ? ? break;
>>
>> + ? ? case S3C64XX_CODCLKSRC_INT:
>> + ? ? ? ? ? ? iismod &= ~S3C64XX_IISMOD_CDCLK_EXT;
>> + ? ? ? ? ? ? break;
>> +
>> + ? ? case S3C64XX_CODCLKSRC_EXT:
>> + ? ? ? ? ? ? iismod |= S3C64XX_IISMOD_CDCLK_EXT;
>> + ? ? ? ? ? ? break;
>> +
>
> This should really be done using the direction parameter - clk_id should
> specify that CDCLK should be used and the direction parameter be used to
> say if it's an input or output.
Again, the patch aims to simply fit in the place. For the current
state and design of our driver this change is least intrusive and most
simple.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 03/10] S3C64XX I2S: Codec Clock Gating Option
2009-09-16 1:37 ` jassi brar
@ 2009-09-16 11:02 ` Mark Brown
2009-09-16 20:12 ` Mark Brown
0 siblings, 1 reply; 6+ messages in thread
From: Mark Brown @ 2009-09-16 11:02 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Sep 16, 2009 at 10:37:09AM +0900, jassi brar wrote:
> On Tue, Sep 15, 2009 at 7:44 PM, Mark Brown
> > In terms of functionality it might be clearer to describe what's going
> > on here as switching the function of CDCLK from being an input to being
> > an output. ?The actual implementation appears to be done by gating the
> > output of the internal clock signal to the CDCLK pin but in terms of
> > what the user can see externally the clock isn't gated, it's still
> > active but it changed direction.
> The bit IISMOD[12], if cleared, gates the RCLK towards the Xi2sCDCLK.
> If this bit is set, it simply disconnects the RCLK and Xi2sCDCLK.
This is an implementation detail; it does this gating internally in
order to allow the pin to be used as an input. The potential confusion
here is that the user visible CDCLK can still have a live signal on it,
the thing that's been disconnected is an internal signal which can be
used to drive it. If you say it's gated that suggests that the signal
is stopped completely.
> >> + ? ? case S3C64XX_CODCLKSRC_INT:
> >> + ? ? ? ? ? ? iismod &= ~S3C64XX_IISMOD_CDCLK_EXT;
> >> + ? ? ? ? ? ? break;
> >> + ? ? case S3C64XX_CODCLKSRC_EXT:
> >> + ? ? ? ? ? ? iismod |= S3C64XX_IISMOD_CDCLK_EXT;
> >> + ? ? ? ? ? ? break;
> > This should really be done using the direction parameter - clk_id should
> > specify that CDCLK should be used and the direction parameter be used to
> > say if it's an input or output.
> Again, the patch aims to simply fit in the place. For the current
> state and design of our driver this change is least intrusive and most
> simple.
There's no meaningful extra effort required to do this properly and
given that all this patch does is implement an external interface it's
better to just get it right rather than spend the effort on fixups (and
merge issues).
In general, the wider the impact of your changes the more important it
is to get them right.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 03/10] S3C64XX I2S: Codec Clock Gating Option
2009-09-16 11:02 ` Mark Brown
@ 2009-09-16 20:12 ` Mark Brown
2009-09-16 23:59 ` jassi brar
0 siblings, 1 reply; 6+ messages in thread
From: Mark Brown @ 2009-09-16 20:12 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Sep 16, 2009 at 12:02:46PM +0100, Mark Brown wrote:
> There's no meaningful extra effort required to do this properly and
> given that all this patch does is implement an external interface it's
> better to just get it right rather than spend the effort on fixups (and
> merge issues).
I added the following patch to implement this, not tested yet.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 03/10] S3C64XX I2S: Codec Clock Gating Option
2009-09-16 20:12 ` Mark Brown
@ 2009-09-16 23:59 ` jassi brar
0 siblings, 0 replies; 6+ messages in thread
From: jassi brar @ 2009-09-16 23:59 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Sep 17, 2009 at 5:12 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Sep 16, 2009 at 12:02:46PM +0100, Mark Brown wrote:
>
>> There's no meaningful extra effort required to do this properly and
>> given that all this patch does is implement an external interface it's
>> better to just get it right rather than spend the effort on fixups (and
>> merge issues).
>
> I added the following patch to implement this, not tested yet.
>
> From 8bb014895547eeeb9aa61a654f24e41e15919304 Mon Sep 17 00:00:00 2001
> From: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Date: Wed, 16 Sep 2009 19:38:53 +0100
> Subject: [PATCH] ASoC: Add S3C64xx IIS CDCLK source selection
>
> CDCLK can either be an output generated by the CPU, intended for use
> as the CODEC master clock, or an input (probably from the CODEC)
> providing a master clock for the IIS block.
>
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
> ?arch/arm/plat-s3c/include/plat/regs-s3c2412-iis.h | ? ?2 ++
> ?sound/soc/s3c24xx/s3c64xx-i2s.c ? ? ? ? ? ? ? ? ? | ? 13 +++++++++++++
> ?sound/soc/s3c24xx/s3c64xx-i2s.h ? ? ? ? ? ? ? ? ? | ? ?1 +
> ?3 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/plat-s3c/include/plat/regs-s3c2412-iis.h b/arch/arm/plat-s3c/include/plat/regs-s3c2412-iis.h
> index 07659da..abf2fbc 100644
> --- a/arch/arm/plat-s3c/include/plat/regs-s3c2412-iis.h
> +++ b/arch/arm/plat-s3c/include/plat/regs-s3c2412-iis.h
> @@ -67,6 +67,8 @@
> ?#define S3C2412_IISMOD_BCLK_MASK ? ? ? (3 << 1)
> ?#define S3C2412_IISMOD_8BIT ? ? ? ? ? ?(1 << 0)
>
> +#define S3C64XX_IISMOD_CDCLKCON ? ? ? ? ? ? ? ?(1 << 12)
> +
> ?#define S3C2412_IISPSR_PSREN ? ? ? ? ? (1 << 15)
>
> ?#define S3C2412_IISFIC_TXFLUSH ? ? ? ? (1 << 15)
> diff --git a/sound/soc/s3c24xx/s3c64xx-i2s.c b/sound/soc/s3c24xx/s3c64xx-i2s.c
> index 3c06c40..aaf4520 100644
> --- a/sound/soc/s3c24xx/s3c64xx-i2s.c
> +++ b/sound/soc/s3c24xx/s3c64xx-i2s.c
> @@ -99,6 +99,19 @@ static int s3c64xx_i2s_set_sysclk(struct snd_soc_dai *cpu_dai,
> ? ? ? ? ? ? ? ?iismod |= S3C64XX_IISMOD_IMS_SYSMUX;
> ? ? ? ? ? ? ? ?break;
>
> + ? ? ? case S3C64XX_CLKSRC_CDCLK:
> + ? ? ? ? ? ? ? switch (dir) {
> + ? ? ? ? ? ? ? case SND_SOC_CLOCK_IN:
> + ? ? ? ? ? ? ? ? ? ? ? iismod |= S3C64XX_IISMOD_CDCLKCON;
> + ? ? ? ? ? ? ? ? ? ? ? break;
> + ? ? ? ? ? ? ? case SND_SOC_CLOCK_OUT:
> + ? ? ? ? ? ? ? ? ? ? ? iismod &= ~S3C64XX_IISMOD_CDCLKCON;
> + ? ? ? ? ? ? ? ? ? ? ? break;
> + ? ? ? ? ? ? ? default:
> + ? ? ? ? ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? break;
> +
> ? ? ? ?default:
> ? ? ? ? ? ? ? ?return -EINVAL;
> ? ? ? ?}
oops, all along i had misunderstood ur suggestion. This seems fine.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-09-16 23:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-09-15 10:02 [PATCH 03/10] S3C64XX I2S: Codec Clock Gating Option Jassi
2009-09-15 10:44 ` Mark Brown
2009-09-16 1:37 ` jassi brar
2009-09-16 11:02 ` Mark Brown
2009-09-16 20:12 ` Mark Brown
2009-09-16 23:59 ` jassi brar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).