[PATCH 05/10] S3C I2S: lrsync function made to work with IRQs disabled.

[PATCH 05/10] S3C I2S: lrsync function made to work with IRQs disabled.

[Jassi]

s3c2412_snd_lrsync() maybe reached with IRQs disabled and if LRCLK
is dead due to improper initialization of CPU or CODEC, the system
gets stuck in the loop because jiffies may never get updated.
Implemented counter based wait mechanism for atleast the same
timeout period.

Signed-Off-by: Jassi <jassi.brar@samsung.com>
---
 sound/soc/s3c24xx/s3c-i2s-v2.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/sound/soc/s3c24xx/s3c-i2s-v2.c b/sound/soc/s3c24xx/s3c-i2s-v2.c
index aa7af0b..8865bc7 100644
--- a/sound/soc/s3c24xx/s3c-i2s-v2.c
+++ b/sound/soc/s3c24xx/s3c-i2s-v2.c
@@ -230,6 +230,8 @@ static void s3c2412_snd_rxctrl(struct s3c_i2sv2_info *i2s, int on)
 	pr_debug("%s: IIS: CON=%x MOD=%x FIC=%x\n", __func__, con, mod, fic);
 }
 
+#define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
+
 /*
  * Wait for the LR signal to allow synchronisation to the L/R clock
  * from the codec. May only be needed for slave mode.
@@ -237,19 +239,21 @@ static void s3c2412_snd_rxctrl(struct s3c_i2sv2_info *i2s, int on)
 static int s3c2412_snd_lrsync(struct s3c_i2sv2_info *i2s)
 {
 	u32 iiscon;
-	unsigned long timeout = jiffies + msecs_to_jiffies(5);
+	unsigned long loops = msecs_to_loops(5);
 
 	pr_debug("Entered %s\n", __func__);
 
-	while (1) {
+	while (--loops) {
 		iiscon = readl(i2s->regs + S3C2412_IISCON);
 		if (iiscon & S3C2412_IISCON_LRINDEX)
 			break;
 
-		if (timeout < jiffies) {
-			printk(KERN_ERR "%s: timeout\n", __func__);
-			return -ETIMEDOUT;
-		}
+	   	cpu_relax();
+	}
+
+	if (!loops) {
+		printk(KERN_ERR "%s: timeout\n", __func__);
+		return -ETIMEDOUT;
 	}
 
 	return 0;
-- 
1.6.2.5

[PATCH 05/10] S3C I2S: lrsync function made to work with IRQs disabled.

[Mark Brown]

On Tue, Sep 15, 2009 at 07:02:37PM +0900, Jassi wrote:
> s3c2412_snd_lrsync() maybe reached with IRQs disabled and if LRCLK
> is dead due to improper initialization of CPU or CODEC, the system
> gets stuck in the loop because jiffies may never get updated.
> Implemented counter based wait mechanism for atleast the same
> timeout period.

I'll apply this but...

> +#define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)

...this really should be moved into a standard header since it's a bit
of a namespace violation.  Also, there's a jiffies_to_msecs function.

[PATCH 05/10] S3C I2S: lrsync function made to work with IRQs disabled.

[Ben Dooks]

On Tue, Sep 15, 2009 at 07:02:37PM +0900, Jassi wrote:
> s3c2412_snd_lrsync() maybe reached with IRQs disabled and if LRCLK
> is dead due to improper initialization of CPU or CODEC, the system
> gets stuck in the loop because jiffies may never get updated.
> Implemented counter based wait mechanism for atleast the same
> timeout period.
> 
> Signed-Off-by: Jassi <jassi.brar@samsung.com>
> ---
>  sound/soc/s3c24xx/s3c-i2s-v2.c |   16 ++++++++++------
>  1 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/s3c24xx/s3c-i2s-v2.c b/sound/soc/s3c24xx/s3c-i2s-v2.c
> index aa7af0b..8865bc7 100644
> --- a/sound/soc/s3c24xx/s3c-i2s-v2.c
> +++ b/sound/soc/s3c24xx/s3c-i2s-v2.c
> @@ -230,6 +230,8 @@ static void s3c2412_snd_rxctrl(struct s3c_i2sv2_info *i2s, int on)
>  	pr_debug("%s: IIS: CON=%x MOD=%x FIC=%x\n", __func__, con, mod, fic);
>  }
>  
> +#define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
> +
>  /*
>   * Wait for the LR signal to allow synchronisation to the L/R clock
>   * from the codec. May only be needed for slave mode.
> @@ -237,19 +239,21 @@ static void s3c2412_snd_rxctrl(struct s3c_i2sv2_info *i2s, int on)
>  static int s3c2412_snd_lrsync(struct s3c_i2sv2_info *i2s)
>  {
>  	u32 iiscon;
> -	unsigned long timeout = jiffies + msecs_to_jiffies(5);
> +	unsigned long loops = msecs_to_loops(5);
>  
>  	pr_debug("Entered %s\n", __func__);
>  
> -	while (1) {
> +	while (--loops) {
>  		iiscon = readl(i2s->regs + S3C2412_IISCON);
>  		if (iiscon & S3C2412_IISCON_LRINDEX)
>  			break;
>  
> -		if (timeout < jiffies) {
> -			printk(KERN_ERR "%s: timeout\n", __func__);
> -			return -ETIMEDOUT;
> -		}
> +	   	cpu_relax();

I don't think cpu_relax() gives a defined 'timeout' length, thus this
loop is of a very indeterminate length of timeout.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

[PATCH 05/10] S3C I2S: lrsync function made to work with IRQs disabled.

[jassi brar]

On Wed, Sep 16, 2009 at 9:14 AM, Ben Dooks <ben-linux@fluff.org> wrote:
> On Tue, Sep 15, 2009 at 07:02:37PM +0900, Jassi wrote:
>> s3c2412_snd_lrsync() maybe reached with IRQs disabled and if LRCLK
>> is dead due to improper initialization of CPU or CODEC, the system
>> gets stuck in the loop because jiffies may never get updated.
>> Implemented counter based wait mechanism for atleast the same
>> timeout period.
>>
>> Signed-Off-by: Jassi <jassi.brar@samsung.com>
>> ---
>> ?sound/soc/s3c24xx/s3c-i2s-v2.c | ? 16 ++++++++++------
>> ?1 files changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/sound/soc/s3c24xx/s3c-i2s-v2.c b/sound/soc/s3c24xx/s3c-i2s-v2.c
>> index aa7af0b..8865bc7 100644
>> --- a/sound/soc/s3c24xx/s3c-i2s-v2.c
>> +++ b/sound/soc/s3c24xx/s3c-i2s-v2.c
>> @@ -230,6 +230,8 @@ static void s3c2412_snd_rxctrl(struct s3c_i2sv2_info *i2s, int on)
>> ? ? ? pr_debug("%s: IIS: CON=%x MOD=%x FIC=%x\n", __func__, con, mod, fic);
>> ?}
>>
>> +#define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
>> +
>> ?/*
>> ? * Wait for the LR signal to allow synchronisation to the L/R clock
>> ? * from the codec. May only be needed for slave mode.
>> @@ -237,19 +239,21 @@ static void s3c2412_snd_rxctrl(struct s3c_i2sv2_info *i2s, int on)
>> ?static int s3c2412_snd_lrsync(struct s3c_i2sv2_info *i2s)
>> ?{
>> ? ? ? u32 iiscon;
>> - ? ? unsigned long timeout = jiffies + msecs_to_jiffies(5);
>> + ? ? unsigned long loops = msecs_to_loops(5);
>>
>> ? ? ? pr_debug("Entered %s\n", __func__);
>>
>> - ? ? while (1) {
>> + ? ? while (--loops) {
>> ? ? ? ? ? ? ? iiscon = readl(i2s->regs + S3C2412_IISCON);
>> ? ? ? ? ? ? ? if (iiscon & S3C2412_IISCON_LRINDEX)
>> ? ? ? ? ? ? ? ? ? ? ? break;
>>
>> - ? ? ? ? ? ? if (timeout < jiffies) {
>> - ? ? ? ? ? ? ? ? ? ? printk(KERN_ERR "%s: timeout\n", __func__);
>> - ? ? ? ? ? ? ? ? ? ? return -ETIMEDOUT;
>> - ? ? ? ? ? ? }
>> + ? ? ? ? ? ? cpu_relax();
>
> I don't think cpu_relax() gives a defined 'timeout' length, thus this
> loop is of a very indeterminate length of timeout.
Ofcourse the total delay is non-deterministic. Besides, there is no
hard limit on the amount of time we want to wait on lrsync. And this
non-deterministic delay comes into picture only when the I2S is
broken. It is second best option to simply hanging.
Total wait period is _atleast_ what we want, as is mentioned in the changelog.

Besides, i think we can do without the cpu_relax(), no?

[PATCH 05/10] S3C I2S: lrsync function made to work with IRQs disabled.

[Mark Brown]

On Wed, Sep 16, 2009 at 10:03:41AM +0900, jassi brar wrote:
> On Wed, Sep 16, 2009 at 9:14 AM, Ben Dooks <ben-linux@fluff.org> wrote:

> > I don't think cpu_relax() gives a defined 'timeout' length, thus this
> > loop is of a very indeterminate length of timeout.

> Ofcourse the total delay is non-deterministic. Besides, there is no
> hard limit on the amount of time we want to wait on lrsync. And this
> non-deterministic delay comes into picture only when the I2S is
> broken. It is second best option to simply hanging.
> Total wait period is _atleast_ what we want, as is mentioned in the changelog.

The length of time you should need to wait for a LRCLK transition is
given by the sample rate - even 1ms is a very generous timeout for 8kHz,
which is the lowest sample rate you'd really expect to see.  The actual
value isn't terribly important since we should never hit it when things
aren't broken.