PLL computation in TLV320AIC3x SoC driver

PLL computation in TLV320AIC3x SoC driver

[Peter Meerwald]

Hello,

I'm trying to use the SoC TLV320AIC3x codec driver with sysclk 16384000 
and ran into some problems with setting PLL; below is a patch against 
linux-2.6-asoc

note that the original code uses variables pll_r and pll_p instead of the 
loop variable r and p to compute tmp, this seems broken

further, the original code does not respect the constraints on j (>= 4, <= 
55 for d==0) according to the codec's datasheet, and similarly for d!=0

I've tested the code with a number of reasonable sysclk values and got 
sane PLL values; please apply if acceptable

thanks, regards, p.


diff --git a/sound/soc/codecs/tlv320aic3x.c 
b/sound/soc/codecs/tlv320aic3x.c
index 3395cf9..e84e473 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -766,9 +766,10 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
        struct snd_soc_codec *codec = socdev->card->codec;
        struct aic3x_priv *aic3x = codec->private_data;
        int codec_clk = 0, bypass_pll = 0, fsref, last_clk = 0;
-       u8 data, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1;
-       u16 pll_d = 1;
+       u8 data, j, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1;
+       u16 d, pll_d = 1;
        u8 reg;
+       int clk;
 
        /* select data word length */
        data =
@@ -835,47 +836,62 @@ static int aic3x_hw_params(struct snd_pcm_substream 
*substream,
                return 0;
 
        /* Use PLL
-        * find an apropriate setup for j, d, r and p by iterating over
-        * p and r - j and d are calculated for each fraction.
-        * Up to 128 values are probed, the closest one wins the game.
+        * find an appropriate setup for j, d, r and p by iterating over
+        * p, r and j first, then trying to compute the fraction d.
+        * Up to 6528 values are probed, the closest one wins the game.
         * The sysclk is divided by 1000 to prevent integer overflows.
         */
        codec_clk = (2048 * fsref) / (aic3x->sysclk / 1000);
 
-       for (r = 1; r <= 16; r++)
-               for (p = 1; p <= 8; p++) {
-                       int clk, tmp = (codec_clk * pll_r * 10) / pll_p;
-                       u8 j = tmp / 10000;
-                       u16 d = tmp % 10000;
-
-                       if (j > 63)
-                               continue;
-
-                       if (d != 0 && aic3x->sysclk < 10000000)
-                               continue;
-
-                       /* This is actually 1000 * ((j + (d/10000)) * r) / p
-                        * The term had to be converted to get rid of the
-                        * division by 10000 */
-                       clk = ((10000 * j * r) + (d * r)) / (10 * p);
-
-                       /* check whether this values get closer than the best
-                        * ones we had before */
-                       if (abs(codec_clk - clk) < abs(codec_clk - last_clk)) {
-                               pll_j = j; pll_d = d; pll_r = r; pll_p = p;
-                               last_clk = clk;
-                       }
-
-                       /* Early exit for exact matches */
-                       if (clk == codec_clk)
-                               break;
-               }
+    for (r = 1; r <= 16; r++)
+        for (p = 1; p <= 8; p++) {
+            for (j = 4; j <= 55; j++) {
+                /* This is actually 1000 * ((j + (d/10000)) * r) / p
+                 * The term had to be converted to get rid of the
+                 * division by 10000; d = 0 here */
+                int clk = (1000 * j * r) / p;
+
+                /* check whether this values get closer than the best
+                 * ones we had before */
+                if (abs(codec_clk - clk) < abs(codec_clk - last_clk)) {
+                    pll_j = j; pll_d = 0; pll_r = r; pll_p = p;
+                    last_clk = clk;
+                }
+
+                /* Early exit for exact matches */
+                if (clk == codec_clk)
+                    goto found;
+            }
+        }
+
+    /* try with d != 0 */
+    for (p = 1; p <= 8; p++) {
+        j = codec_clk * p / 1000;
+
+        if (j < 4 || j > 11) continue;
+
+        /* do not use codec_clk here since we'd loose precision */
+        d = ((2048 * fsref * 10) / (aic3x->sysclk / 1000)) % 10000;
+        clk = (10000 * j + d) / (10 * p);
+
+        /* check whether this values get closer than the best
+         * ones we had before */
+        if (abs(codec_clk - clk) < abs(codec_clk - last_clk)) {
+            pll_j = j; pll_d = d; pll_r = 1; pll_p = 1;
+            last_clk = clk;
+        }
+
+        /* Early exit for exact matches */
+        if (clk == codec_clk)
+            goto found;
+    }
 
        if (last_clk == 0) {
                printk(KERN_ERR "%s(): unable to setup PLL\n", __func__);
                return -EINVAL;
        }
 
+found:
        data = aic3x_read_reg_cache(codec, AIC3X_PLL_PROGA_REG);
        aic3x_write(codec, AIC3X_PLL_PROGA_REG, data | (pll_p << PLLP_SHIFT));
        aic3x_write(codec, AIC3X_OVRF_STATUS_AND_PLLR_REG, pll_r << PLLR_SHIFT);

-- 

Peter Meerwald
Kaigasse 3 / 8
A-5020 Salzburg / AUSTRIA
+43-664-2444418 (mobile)

Re: PLL computation in TLV320AIC3x SoC driver

[Vladimir Barinov]

Hello Peter,

Peter Meerwald wrote:
> Hello,
>
> I'm trying to use the SoC TLV320AIC3x codec driver with sysclk 16384000 
> and ran into some problems with setting PLL; below is a patch against 
> linux-2.6-asoc
>
>   
I've made the simple test application to calculate pll_p/r/j/d values 
using current tlv320aic3x clock calculation scheme and I've got:

samplerate=8000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=(sysclk 
* [pll_j].[pll_d] * pll_r) / (2048 * pll_p) = 48000
samplerate=11025: pll_p=1, pll_r=1, pll_j=5, pll_d=5120, hence FSREF= 44096
samplerate=16000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000
samplerate=22050: pll_p=1, pll_r=1, pll_j=5, pll_d=5120, hence FSREF= 44096
samplerate=32000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000
samplerate=44100: pll_p=1, pll_r=1, pll_j=5, pll_d=5120, hence FSREF= 44096
samplerate=48000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000
samplerate=64000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000, 
note that here we used  DUAL_RATE_MODE, hence FSREF=96000
samplerate=88200: pll_p=1, pll_r=1, pll_j=5, pll_d=5120, hence FSREF= 
44096, note that here we used  DUAL_RATE_MODE hence FSREF=88192
samplerate=96000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000, 
note that here we used  DUAL_RATE_MODE hence FSREF = 96000

Then the AIC3X_SAMPLE_RATE_SEL_REG is calculated by simple formula based 
on fsref.

Hence according to current code the calculations of desired FSREF is 
correct for sysclk=16384000

I've not tried it with your patch since I don't actually understand what 
do you fix? :)

Regards,
Vladimir
> note that the original code uses variables pll_r and pll_p instead of the 
> loop variable r and p to compute tmp, this seems broken
>
> further, the original code does not respect the constraints on j (>= 4, <= 
> 55 for d==0) according to the codec's datasheet, and similarly for d!=0
>
> I've tested the code with a number of reasonable sysclk values and got 
> sane PLL values; please apply if acceptable
>   
> thanks, regards, p.
>
>
> diff --git a/sound/soc/codecs/tlv320aic3x.c 
> b/sound/soc/codecs/tlv320aic3x.c
> index 3395cf9..e84e473 100644
> --- a/sound/soc/codecs/tlv320aic3x.c
> +++ b/sound/soc/codecs/tlv320aic3x.c
> @@ -766,9 +766,10 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
>         struct snd_soc_codec *codec = socdev->card->codec;
>         struct aic3x_priv *aic3x = codec->private_data;
>         int codec_clk = 0, bypass_pll = 0, fsref, last_clk = 0;
> -       u8 data, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1;
> -       u16 pll_d = 1;
> +       u8 data, j, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1;
> +       u16 d, pll_d = 1;
>         u8 reg;
> +       int clk;
>  
>         /* select data word length */
>         data =
> @@ -835,47 +836,62 @@ static int aic3x_hw_params(struct snd_pcm_substream 
> *substream,
>                 return 0;
>  
>         /* Use PLL
> -        * find an apropriate setup for j, d, r and p by iterating over
> -        * p and r - j and d are calculated for each fraction.
> -        * Up to 128 values are probed, the closest one wins the game.
> +        * find an appropriate setup for j, d, r and p by iterating over
> +        * p, r and j first, then trying to compute the fraction d.
> +        * Up to 6528 values are probed, the closest one wins the game.
>          * The sysclk is divided by 1000 to prevent integer overflows.
>          */
>         codec_clk = (2048 * fsref) / (aic3x->sysclk / 1000);
>  
> -       for (r = 1; r <= 16; r++)
> -               for (p = 1; p <= 8; p++) {
> -                       int clk, tmp = (codec_clk * pll_r * 10) / pll_p;
> -                       u8 j = tmp / 10000;
> -                       u16 d = tmp % 10000;
> -
> -                       if (j > 63)
> -                               continue;
> -
> -                       if (d != 0 && aic3x->sysclk < 10000000)
> -                               continue;
> -
> -                       /* This is actually 1000 * ((j + (d/10000)) * r) / p
> -                        * The term had to be converted to get rid of the
> -                        * division by 10000 */
> -                       clk = ((10000 * j * r) + (d * r)) / (10 * p);
> -
> -                       /* check whether this values get closer than the best
> -                        * ones we had before */
> -                       if (abs(codec_clk - clk) < abs(codec_clk - last_clk)) {
> -                               pll_j = j; pll_d = d; pll_r = r; pll_p = p;
> -                               last_clk = clk;
> -                       }
> -
> -                       /* Early exit for exact matches */
> -                       if (clk == codec_clk)
> -                               break;
> -               }
> +    for (r = 1; r <= 16; r++)
> +        for (p = 1; p <= 8; p++) {
> +            for (j = 4; j <= 55; j++) {
> +                /* This is actually 1000 * ((j + (d/10000)) * r) / p
> +                 * The term had to be converted to get rid of the
> +                 * division by 10000; d = 0 here */
> +                int clk = (1000 * j * r) / p;
> +
> +                /* check whether this values get closer than the best
> +                 * ones we had before */
> +                if (abs(codec_clk - clk) < abs(codec_clk - last_clk)) {
> +                    pll_j = j; pll_d = 0; pll_r = r; pll_p = p;
> +                    last_clk = clk;
> +                }
> +
> +                /* Early exit for exact matches */
> +                if (clk == codec_clk)
> +                    goto found;
> +            }
> +        }
> +
> +    /* try with d != 0 */
> +    for (p = 1; p <= 8; p++) {
> +        j = codec_clk * p / 1000;
> +
> +        if (j < 4 || j > 11) continue;
> +
> +        /* do not use codec_clk here since we'd loose precision */
> +        d = ((2048 * fsref * 10) / (aic3x->sysclk / 1000)) % 10000;
> +        clk = (10000 * j + d) / (10 * p);
> +
> +        /* check whether this values get closer than the best
> +         * ones we had before */
> +        if (abs(codec_clk - clk) < abs(codec_clk - last_clk)) {
> +            pll_j = j; pll_d = d; pll_r = 1; pll_p = 1;
> +            last_clk = clk;
> +        }
> +
> +        /* Early exit for exact matches */
> +        if (clk == codec_clk)
> +            goto found;
> +    }
>  
>         if (last_clk == 0) {
>                 printk(KERN_ERR "%s(): unable to setup PLL\n", __func__);
>                 return -EINVAL;
>         }
>  
> +found:
>         data = aic3x_read_reg_cache(codec, AIC3X_PLL_PROGA_REG);
>         aic3x_write(codec, AIC3X_PLL_PROGA_REG, data | (pll_p << PLLP_SHIFT));
>         aic3x_write(codec, AIC3X_OVRF_STATUS_AND_PLLR_REG, pll_r << PLLR_SHIFT);
>
>   

Re: PLL computation in TLV320AIC3x SoC driver

[Mark Brown]

On Tue, Dec 08, 2009 at 12:28:30PM +0100, Peter Meerwald wrote:

> I'm trying to use the SoC TLV320AIC3x codec driver with sysclk 16384000 
> and ran into some problems with setting PLL; below is a patch against 
> linux-2.6-asoc

Please submit patches against the most recent for- branch of

  git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound-2.6.git

You've also forgotten your Signed-off-by.

> further, the original code does not respect the constraints on j (>= 4, <= 
> 55 for d==0) according to the codec's datasheet, and similarly for d!=0

Might be helpful to point out what these constraints are and how the
code fails to respect them - given that your change is a total rewrite
of the relevant code it's not as easy as it might be to spot what's
changed.

Re: PLL computation in TLV320AIC3x SoC driver

[Liam Girdwood]

On Tue, 2009-12-08 at 12:28 +0100, Peter Meerwald wrote:
> Hello,
> 
> I'm trying to use the SoC TLV320AIC3x codec driver with sysclk 16384000 
> and ran into some problems with setting PLL; below is a patch against 
> linux-2.6-asoc
> 
> note that the original code uses variables pll_r and pll_p instead of the 
> loop variable r and p to compute tmp, this seems broken
> 
> further, the original code does not respect the constraints on j (>= 4, <= 
> 55 for d==0) according to the codec's datasheet, and similarly for d!=0
> 
> I've tested the code with a number of reasonable sysclk values and got 
> sane PLL values; please apply if acceptable
> 

Can you confirm you also tested against 12.288MHz and 11.2869MHz ?

If so, Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>

Thanks

Liam

Re: PLL computation in TLV320AIC3x SoC driver

[Peter Meerwald]

Hello Vladimir,

> > I'm trying to use the SoC TLV320AIC3x codec driver with sysclk 16384000 and
> > ran into some problems with setting PLL; below is a patch against
> > linux-2.6-asoc

> I've made the simple test application to calculate pll_p/r/j/d values using
> current tlv320aic3x clock calculation scheme and I've got:
> samplerate=8000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=(sysclk *
> [pll_j].[pll_d] * pll_r) / (2048 * pll_p) = 48000
> samplerate=11025: pll_p=1, pll_r=1, pll_j=5, pll_d=5120, hence FSREF= 44096
> samplerate=16000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000
> samplerate=22050: pll_p=1, pll_r=1, pll_j=5, pll_d=5120, hence FSREF= 44096
> samplerate=32000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000
> samplerate=44100: pll_p=1, pll_r=1, pll_j=5, pll_d=5120, hence FSREF= 44096
> samplerate=48000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000
> samplerate=64000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000, note
> that here we used  DUAL_RATE_MODE, hence FSREF=96000
> samplerate=88200: pll_p=1, pll_r=1, pll_j=5, pll_d=5120, hence FSREF= 44096,
> note that here we used  DUAL_RATE_MODE hence FSREF=88192
> samplerate=96000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000, note
> that here we used  DUAL_RATE_MODE hence FSREF = 96000

> Hence according to current code the calculations of desired FSREF is correct
> for sysclk=16384000
> I've not tried it with your patch since I don't actually understand what do
> you fix? :)

1. loop variables are r and p but for computation of j the variables 
pll_r and pll_p are used; I think r and p should be used in the 
computation of j, your code below:

    for (r = 1; r <= 16; r++)
        for (p = 1; p <= 8; p++) {
            int clk, tmp = (codec_clk * pll_r * 10) / pll_p;
            u8 j = tmp / 10000;

further, the codec datasheet mandates certain ranges for 'good 
performance', 4 <= j <= 55 (for d==0) and 4 <= j <= 11 (for d!=0) which 
are not checked for

2. d should actually be 5125 and not 5120

3. appropriate is spell appropriately :)

> > note that the original code uses variables pll_r and pll_p instead of the
> > loop variable r and p to compute tmp, this seems broken
> > 
> > further, the original code does not respect the constraints on j (>= 4, <=
> > 55 for d==0) according to the codec's datasheet, and similarly for d!=0
> > 
> > I've tested the code with a number of reasonable sysclk values and got sane
> > PLL values; please apply if acceptable
> >   thanks, regards, p.
> > 
> > 
> > diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
> > index 3395cf9..e84e473 100644
> > --- a/sound/soc/codecs/tlv320aic3x.c
> > +++ b/sound/soc/codecs/tlv320aic3x.c
> > @@ -766,9 +766,10 @@ static int aic3x_hw_params(struct snd_pcm_substream
> > *substream,
> >         struct snd_soc_codec *codec = socdev->card->codec;
> >         struct aic3x_priv *aic3x = codec->private_data;
> >         int codec_clk = 0, bypass_pll = 0, fsref, last_clk = 0;
> > -       u8 data, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1;
> > -       u16 pll_d = 1;
> > +       u8 data, j, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1;
> > +       u16 d, pll_d = 1;
> >         u8 reg;
> > +       int clk;
> >          /* select data word length */
> >         data =
> > @@ -835,47 +836,62 @@ static int aic3x_hw_params(struct snd_pcm_substream
> > *substream,
> >                 return 0;
> >          /* Use PLL
> > -        * find an apropriate setup for j, d, r and p by iterating over
> > -        * p and r - j and d are calculated for each fraction.
> > -        * Up to 128 values are probed, the closest one wins the game.
> > +        * find an appropriate setup for j, d, r and p by iterating over
> > +        * p, r and j first, then trying to compute the fraction d.
> > +        * Up to 6528 values are probed, the closest one wins the game.
> >          * The sysclk is divided by 1000 to prevent integer overflows.
> >          */
> >         codec_clk = (2048 * fsref) / (aic3x->sysclk / 1000);
> >  -       for (r = 1; r <= 16; r++)
> > -               for (p = 1; p <= 8; p++) {
> > -                       int clk, tmp = (codec_clk * pll_r * 10) / pll_p;
> > -                       u8 j = tmp / 10000;
> > -                       u16 d = tmp % 10000;
> > -
> > -                       if (j > 63)
> > -                               continue;
> > -
> > -                       if (d != 0 && aic3x->sysclk < 10000000)
> > -                               continue;
> > -
> > -                       /* This is actually 1000 * ((j + (d/10000)) * r) / p
> > -                        * The term had to be converted to get rid of the
> > -                        * division by 10000 */
> > -                       clk = ((10000 * j * r) + (d * r)) / (10 * p);
> > -
> > -                       /* check whether this values get closer than the
> > best
> > -                        * ones we had before */
> > -                       if (abs(codec_clk - clk) < abs(codec_clk -
> > last_clk)) {
> > -                               pll_j = j; pll_d = d; pll_r = r; pll_p = p;
> > -                               last_clk = clk;
> > -                       }
> > -
> > -                       /* Early exit for exact matches */
> > -                       if (clk == codec_clk)
> > -                               break;
> > -               }
> > +    for (r = 1; r <= 16; r++)
> > +        for (p = 1; p <= 8; p++) {
> > +            for (j = 4; j <= 55; j++) {
> > +                /* This is actually 1000 * ((j + (d/10000)) * r) / p
> > +                 * The term had to be converted to get rid of the
> > +                 * division by 10000; d = 0 here */
> > +                int clk = (1000 * j * r) / p;
> > +
> > +                /* check whether this values get closer than the best
> > +                 * ones we had before */
> > +                if (abs(codec_clk - clk) < abs(codec_clk - last_clk)) {
> > +                    pll_j = j; pll_d = 0; pll_r = r; pll_p = p;
> > +                    last_clk = clk;
> > +                }
> > +
> > +                /* Early exit for exact matches */
> > +                if (clk == codec_clk)
> > +                    goto found;
> > +            }
> > +        }
> > +
> > +    /* try with d != 0 */
> > +    for (p = 1; p <= 8; p++) {
> > +        j = codec_clk * p / 1000;
> > +
> > +        if (j < 4 || j > 11) continue;
> > +
> > +        /* do not use codec_clk here since we'd loose precision */
> > +        d = ((2048 * fsref * 10) / (aic3x->sysclk / 1000)) % 10000;
> > +        clk = (10000 * j + d) / (10 * p);
> > +
> > +        /* check whether this values get closer than the best
> > +         * ones we had before */
> > +        if (abs(codec_clk - clk) < abs(codec_clk - last_clk)) {
> > +            pll_j = j; pll_d = d; pll_r = 1; pll_p = 1;
> > +            last_clk = clk;
> > +        }
> > +
> > +        /* Early exit for exact matches */
> > +        if (clk == codec_clk)
> > +            goto found;
> > +    }
> >          if (last_clk == 0) {
> >                 printk(KERN_ERR "%s(): unable to setup PLL\n", __func__);
> >                 return -EINVAL;
> >         }
> >  +found:
> >         data = aic3x_read_reg_cache(codec, AIC3X_PLL_PROGA_REG);
> >         aic3x_write(codec, AIC3X_PLL_PROGA_REG, data | (pll_p <<
> > PLLP_SHIFT));
> >         aic3x_write(codec, AIC3X_OVRF_STATUS_AND_PLLR_REG, pll_r <<
> > PLLR_SHIFT);
> > 
> >   

-- 

Peter Meerwald
Kaigasse 3 / 8
A-5020 Salzburg / AUSTRIA
+43-664-2444418 (mobile)

Re: PLL computation in TLV320AIC3x SoC driver

[Vladimir Barinov]

Peter Meerwald wrote:
> Hello Vladimir,
>
>   
>>> I'm trying to use the SoC TLV320AIC3x codec driver with sysclk 16384000 and
>>> ran into some problems with setting PLL; below is a patch against
>>> linux-2.6-asoc
>>>       
>
>   
>> I've made the simple test application to calculate pll_p/r/j/d values using
>> current tlv320aic3x clock calculation scheme and I've got:
>> samplerate=8000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=(sysclk *
>> [pll_j].[pll_d] * pll_r) / (2048 * pll_p) = 48000
>> samplerate=11025: pll_p=1, pll_r=1, pll_j=5, pll_d=5120, hence FSREF= 44096
>> samplerate=16000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000
>> samplerate=22050: pll_p=1, pll_r=1, pll_j=5, pll_d=5120, hence FSREF= 44096
>> samplerate=32000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000
>> samplerate=44100: pll_p=1, pll_r=1, pll_j=5, pll_d=5120, hence FSREF= 44096
>> samplerate=48000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000
>> samplerate=64000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000, note
>> that here we used  DUAL_RATE_MODE, hence FSREF=96000
>> samplerate=88200: pll_p=1, pll_r=1, pll_j=5, pll_d=5120, hence FSREF= 44096,
>> note that here we used  DUAL_RATE_MODE hence FSREF=88192
>> samplerate=96000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000, note
>> that here we used  DUAL_RATE_MODE hence FSREF = 96000
>>     
>
>   
>> Hence according to current code the calculations of desired FSREF is correct
>> for sysclk=16384000
>> I've not tried it with your patch since I don't actually understand what do
>> you fix? :)
>>     
>
> 1. loop variables are r and p but for computation of j the variables 
> pll_r and pll_p are used; I think r and p should be used in the 
> computation of j, your code below:
>
>     for (r = 1; r <= 16; r++)
>         for (p = 1; p <= 8; p++) {
>             int clk, tmp = (codec_clk * pll_r * 10) / pll_p;
>             u8 j = tmp / 10000;
>
> further, the codec datasheet mandates certain ranges for 'good 
> performance', 4 <= j <= 55 (for d==0) and 4 <= j <= 11 (for d!=0) which 
> are not checked for
>   
Correct.
> 2. d should actually be 5125 and not 5120
>   
Now I see, I've verified your code handles this. This actually produce 
more accuracy for FSREF.

I just tested your patch works fine with sysclk=16384000 and 12288000 
but fails with 11286900: it returns FSREF=45467 instead of desired 
44100. I think that this is a valuable difference. Please check it.

Regards,
Vladimir
> 3. appropriate is spell appropriately :)
>
>   
>>> note that the original code uses variables pll_r and pll_p instead of the
>>> loop variable r and p to compute tmp, this seems broken
>>>
>>> further, the original code does not respect the constraints on j (>= 4, <=
>>> 55 for d==0) according to the codec's datasheet, and similarly for d!=0
>>>
>>> I've tested the code with a number of reasonable sysclk values and got sane
>>> PLL values; please apply if acceptable
>>>   thanks, regards, p.
>>>
>>>
>>> diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
>>> index 3395cf9..e84e473 100644
>>> --- a/sound/soc/codecs/tlv320aic3x.c
>>> +++ b/sound/soc/codecs/tlv320aic3x.c
>>> @@ -766,9 +766,10 @@ static int aic3x_hw_params(struct snd_pcm_substream
>>> *substream,
>>>         struct snd_soc_codec *codec = socdev->card->codec;
>>>         struct aic3x_priv *aic3x = codec->private_data;
>>>         int codec_clk = 0, bypass_pll = 0, fsref, last_clk = 0;
>>> -       u8 data, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1;
>>> -       u16 pll_d = 1;
>>> +       u8 data, j, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1;
>>> +       u16 d, pll_d = 1;
>>>         u8 reg;
>>> +       int clk;
>>>          /* select data word length */
>>>         data =
>>> @@ -835,47 +836,62 @@ static int aic3x_hw_params(struct snd_pcm_substream
>>> *substream,
>>>                 return 0;
>>>          /* Use PLL
>>> -        * find an apropriate setup for j, d, r and p by iterating over
>>> -        * p and r - j and d are calculated for each fraction.
>>> -        * Up to 128 values are probed, the closest one wins the game.
>>> +        * find an appropriate setup for j, d, r and p by iterating over
>>> +        * p, r and j first, then trying to compute the fraction d.
>>> +        * Up to 6528 values are probed, the closest one wins the game.
>>>          * The sysclk is divided by 1000 to prevent integer overflows.
>>>          */
>>>         codec_clk = (2048 * fsref) / (aic3x->sysclk / 1000);
>>>  -       for (r = 1; r <= 16; r++)
>>> -               for (p = 1; p <= 8; p++) {
>>> -                       int clk, tmp = (codec_clk * pll_r * 10) / pll_p;
>>> -                       u8 j = tmp / 10000;
>>> -                       u16 d = tmp % 10000;
>>> -
>>> -                       if (j > 63)
>>> -                               continue;
>>> -
>>> -                       if (d != 0 && aic3x->sysclk < 10000000)
>>> -                               continue;
>>> -
>>> -                       /* This is actually 1000 * ((j + (d/10000)) * r) / p
>>> -                        * The term had to be converted to get rid of the
>>> -                        * division by 10000 */
>>> -                       clk = ((10000 * j * r) + (d * r)) / (10 * p);
>>> -
>>> -                       /* check whether this values get closer than the
>>> best
>>> -                        * ones we had before */
>>> -                       if (abs(codec_clk - clk) < abs(codec_clk -
>>> last_clk)) {
>>> -                               pll_j = j; pll_d = d; pll_r = r; pll_p = p;
>>> -                               last_clk = clk;
>>> -                       }
>>> -
>>> -                       /* Early exit for exact matches */
>>> -                       if (clk == codec_clk)
>>> -                               break;
>>> -               }
>>> +    for (r = 1; r <= 16; r++)
>>> +        for (p = 1; p <= 8; p++) {
>>> +            for (j = 4; j <= 55; j++) {
>>> +                /* This is actually 1000 * ((j + (d/10000)) * r) / p
>>> +                 * The term had to be converted to get rid of the
>>> +                 * division by 10000; d = 0 here */
>>> +                int clk = (1000 * j * r) / p;
>>> +
>>> +                /* check whether this values get closer than the best
>>> +                 * ones we had before */
>>> +                if (abs(codec_clk - clk) < abs(codec_clk - last_clk)) {
>>> +                    pll_j = j; pll_d = 0; pll_r = r; pll_p = p;
>>> +                    last_clk = clk;
>>> +                }
>>> +
>>> +                /* Early exit for exact matches */
>>> +                if (clk == codec_clk)
>>> +                    goto found;
>>> +            }
>>> +        }
>>> +
>>> +    /* try with d != 0 */
>>> +    for (p = 1; p <= 8; p++) {
>>> +        j = codec_clk * p / 1000;
>>> +
>>> +        if (j < 4 || j > 11) continue;
>>> +
>>> +        /* do not use codec_clk here since we'd loose precision */
>>> +        d = ((2048 * fsref * 10) / (aic3x->sysclk / 1000)) % 10000;
>>> +        clk = (10000 * j + d) / (10 * p);
>>> +
>>> +        /* check whether this values get closer than the best
>>> +         * ones we had before */
>>> +        if (abs(codec_clk - clk) < abs(codec_clk - last_clk)) {
>>> +            pll_j = j; pll_d = d; pll_r = 1; pll_p = 1;
>>> +            last_clk = clk;
>>> +        }
>>> +
>>> +        /* Early exit for exact matches */
>>> +        if (clk == codec_clk)
>>> +            goto found;
>>> +    }
>>>          if (last_clk == 0) {
>>>                 printk(KERN_ERR "%s(): unable to setup PLL\n", __func__);
>>>                 return -EINVAL;
>>>         }
>>>  +found:
>>>         data = aic3x_read_reg_cache(codec, AIC3X_PLL_PROGA_REG);
>>>         aic3x_write(codec, AIC3X_PLL_PROGA_REG, data | (pll_p <<
>>> PLLP_SHIFT));
>>>         aic3x_write(codec, AIC3X_OVRF_STATUS_AND_PLLR_REG, pll_r <<
>>> PLLR_SHIFT);
>>>
>>>   
>>>       
>
>   

Re: PLL computation in TLV320AIC3x SoC driver

[Vladimir Barinov]

Vladimir Barinov wrote:
> Peter Meerwald wrote:
>> Hello Vladimir,
>>
>>  
>>>> I'm trying to use the SoC TLV320AIC3x codec driver with sysclk 
>>>> 16384000 and
>>>> ran into some problems with setting PLL; below is a patch against
>>>> linux-2.6-asoc
>>>>       
>>
>>  
>>> I've made the simple test application to calculate pll_p/r/j/d 
>>> values using
>>> current tlv320aic3x clock calculation scheme and I've got:
>>> samplerate=8000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence 
>>> FSREF=(sysclk *
>>> [pll_j].[pll_d] * pll_r) / (2048 * pll_p) = 48000
>>> samplerate=11025: pll_p=1, pll_r=1, pll_j=5, pll_d=5120, hence 
>>> FSREF= 44096
>>> samplerate=16000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000
>>> samplerate=22050: pll_p=1, pll_r=1, pll_j=5, pll_d=5120, hence 
>>> FSREF= 44096
>>> samplerate=32000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000
>>> samplerate=44100: pll_p=1, pll_r=1, pll_j=5, pll_d=5120, hence 
>>> FSREF= 44096
>>> samplerate=48000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence FSREF=48000
>>> samplerate=64000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence 
>>> FSREF=48000, note
>>> that here we used  DUAL_RATE_MODE, hence FSREF=96000
>>> samplerate=88200: pll_p=1, pll_r=1, pll_j=5, pll_d=5120, hence 
>>> FSREF= 44096,
>>> note that here we used  DUAL_RATE_MODE hence FSREF=88192
>>> samplerate=96000: pll_p=1, pll_r=1, pll_j=6, pll_d=0, hence 
>>> FSREF=48000, note
>>> that here we used  DUAL_RATE_MODE hence FSREF = 96000
>>>     
>>
>>  
>>> Hence according to current code the calculations of desired FSREF is 
>>> correct
>>> for sysclk=16384000
>>> I've not tried it with your patch since I don't actually understand 
>>> what do
>>> you fix? :)
>>>     
>>
>> 1. loop variables are r and p but for computation of j the variables 
>> pll_r and pll_p are used; I think r and p should be used in the 
>> computation of j, your code below:
>>
>>     for (r = 1; r <= 16; r++)
>>         for (p = 1; p <= 8; p++) {
>>             int clk, tmp = (codec_clk * pll_r * 10) / pll_p;
>>             u8 j = tmp / 10000;
>>
>> further, the codec datasheet mandates certain ranges for 'good 
>> performance', 4 <= j <= 55 (for d==0) and 4 <= j <= 11 (for d!=0) 
>> which are not checked for
>>   
> Correct.
>> 2. d should actually be 5125 and not 5120
>>   
> Now I see, I've verified your code handles this. This actually produce 
> more accuracy for FSREF.
>
> I just tested your patch works fine with sysclk=16384000 and 12288000 
> but fails with 11286900: it returns FSREF=45467 instead of desired 
> 44100. I think that this is a valuable difference. Please check it.
Please check  also 33868800  sysclk, it  returns  FSREF=47545 instead 
of  48000.
>
> Regards,
> Vladimir
>> 3. appropriate is spell appropriately :)
>>
>>  
>>>> note that the original code uses variables pll_r and pll_p instead 
>>>> of the
>>>> loop variable r and p to compute tmp, this seems broken
>>>>
>>>> further, the original code does not respect the constraints on j 
>>>> (>= 4, <=
>>>> 55 for d==0) according to the codec's datasheet, and similarly for 
>>>> d!=0
>>>>
>>>> I've tested the code with a number of reasonable sysclk values and 
>>>> got sane
>>>> PLL values; please apply if acceptable
>>>>   thanks, regards, p.
>>>>
>>>>
>>>> diff --git a/sound/soc/codecs/tlv320aic3x.c 
>>>> b/sound/soc/codecs/tlv320aic3x.c
>>>> index 3395cf9..e84e473 100644
>>>> --- a/sound/soc/codecs/tlv320aic3x.c
>>>> +++ b/sound/soc/codecs/tlv320aic3x.c
>>>> @@ -766,9 +766,10 @@ static int aic3x_hw_params(struct 
>>>> snd_pcm_substream
>>>> *substream,
>>>>         struct snd_soc_codec *codec = socdev->card->codec;
>>>>         struct aic3x_priv *aic3x = codec->private_data;
>>>>         int codec_clk = 0, bypass_pll = 0, fsref, last_clk = 0;
>>>> -       u8 data, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1;
>>>> -       u16 pll_d = 1;
>>>> +       u8 data, j, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1;
>>>> +       u16 d, pll_d = 1;
>>>>         u8 reg;
>>>> +       int clk;
>>>>          /* select data word length */
>>>>         data =
>>>> @@ -835,47 +836,62 @@ static int aic3x_hw_params(struct 
>>>> snd_pcm_substream
>>>> *substream,
>>>>                 return 0;
>>>>          /* Use PLL
>>>> -        * find an apropriate setup for j, d, r and p by iterating 
>>>> over
>>>> -        * p and r - j and d are calculated for each fraction.
>>>> -        * Up to 128 values are probed, the closest one wins the game.
>>>> +        * find an appropriate setup for j, d, r and p by iterating 
>>>> over
>>>> +        * p, r and j first, then trying to compute the fraction d.
>>>> +        * Up to 6528 values are probed, the closest one wins the 
>>>> game.
>>>>          * The sysclk is divided by 1000 to prevent integer overflows.
>>>>          */
>>>>         codec_clk = (2048 * fsref) / (aic3x->sysclk / 1000);
>>>>  -       for (r = 1; r <= 16; r++)
>>>> -               for (p = 1; p <= 8; p++) {
>>>> -                       int clk, tmp = (codec_clk * pll_r * 10) / 
>>>> pll_p;
>>>> -                       u8 j = tmp / 10000;
>>>> -                       u16 d = tmp % 10000;
>>>> -
>>>> -                       if (j > 63)
>>>> -                               continue;
>>>> -
>>>> -                       if (d != 0 && aic3x->sysclk < 10000000)
>>>> -                               continue;
>>>> -
>>>> -                       /* This is actually 1000 * ((j + (d/10000)) 
>>>> * r) / p
>>>> -                        * The term had to be converted to get rid 
>>>> of the
>>>> -                        * division by 10000 */
>>>> -                       clk = ((10000 * j * r) + (d * r)) / (10 * p);
>>>> -
>>>> -                       /* check whether this values get closer 
>>>> than the
>>>> best
>>>> -                        * ones we had before */
>>>> -                       if (abs(codec_clk - clk) < abs(codec_clk -
>>>> last_clk)) {
>>>> -                               pll_j = j; pll_d = d; pll_r = r; 
>>>> pll_p = p;
>>>> -                               last_clk = clk;
>>>> -                       }
>>>> -
>>>> -                       /* Early exit for exact matches */
>>>> -                       if (clk == codec_clk)
>>>> -                               break;
>>>> -               }
>>>> +    for (r = 1; r <= 16; r++)
>>>> +        for (p = 1; p <= 8; p++) {
>>>> +            for (j = 4; j <= 55; j++) {
>>>> +                /* This is actually 1000 * ((j + (d/10000)) * r) / p
>>>> +                 * The term had to be converted to get rid of the
>>>> +                 * division by 10000; d = 0 here */
>>>> +                int clk = (1000 * j * r) / p;
>>>> +
>>>> +                /* check whether this values get closer than the best
>>>> +                 * ones we had before */
>>>> +                if (abs(codec_clk - clk) < abs(codec_clk - 
>>>> last_clk)) {
>>>> +                    pll_j = j; pll_d = 0; pll_r = r; pll_p = p;
>>>> +                    last_clk = clk;
>>>> +                }
>>>> +
>>>> +                /* Early exit for exact matches */
>>>> +                if (clk == codec_clk)
>>>> +                    goto found;
>>>> +            }
>>>> +        }
>>>> +
>>>> +    /* try with d != 0 */
>>>> +    for (p = 1; p <= 8; p++) {
>>>> +        j = codec_clk * p / 1000;
>>>> +
>>>> +        if (j < 4 || j > 11) continue;
>>>> +
>>>> +        /* do not use codec_clk here since we'd loose precision */
>>>> +        d = ((2048 * fsref * 10) / (aic3x->sysclk / 1000)) % 10000;
>>>> +        clk = (10000 * j + d) / (10 * p);
>>>> +
>>>> +        /* check whether this values get closer than the best
>>>> +         * ones we had before */
>>>> +        if (abs(codec_clk - clk) < abs(codec_clk - last_clk)) {
>>>> +            pll_j = j; pll_d = d; pll_r = 1; pll_p = 1;
>>>> +            last_clk = clk;
>>>> +        }
>>>> +
>>>> +        /* Early exit for exact matches */
>>>> +        if (clk == codec_clk)
>>>> +            goto found;
>>>> +    }
>>>>          if (last_clk == 0) {
>>>>                 printk(KERN_ERR "%s(): unable to setup PLL\n", 
>>>> __func__);
>>>>                 return -EINVAL;
>>>>         }
>>>>  +found:
>>>>         data = aic3x_read_reg_cache(codec, AIC3X_PLL_PROGA_REG);
>>>>         aic3x_write(codec, AIC3X_PLL_PROGA_REG, data | (pll_p <<
>>>> PLLP_SHIFT));
>>>>         aic3x_write(codec, AIC3X_OVRF_STATUS_AND_PLLR_REG, pll_r <<
>>>> PLLR_SHIFT);
>>>>
>>>>         
>>
>>   
>

Re: PLL computation in TLV320AIC3x SoC driver

[Peter Meerwald]

Hello,

> > > > > I'm trying to use the SoC TLV320AIC3x codec driver with sysclk
> > > > > 16384000 and
> > > > > ran into some problems with setting PLL; below is a patch against
> > > > > linux-2.6-asoc

> > I just tested your patch works fine with sysclk=16384000 and 12288000 but
> > fails with 11286900: it returns FSREF=45467 instead of desired 44100. I
> > think that this is a valuable difference. Please check it.
> Please check  also 33868800  sysclk, it  returns  FSREF=47545 instead of
> 48000.

I have put the proposed code at http://pmeerw.net/clk/ for review;

'make test' gives me:
clk(16384000,48000): r 1 p 1 d 0 j 6; 6000 == 6000; 48000.000000
clk_orig(16384000,48000): r 1 p 1 d 0 j 6; 6000 == 6000; 48000.000000
clk(16384000,44100): r 1 p 1 d 5125 j 5; 5512 == 5512; 44100.000000
clk_orig(16384000,44100): r 1 p 1 d 5120 j 5; 5512 == 5512; 44096.000000
clk(11286900,48000): r 1 p 1 d 7095 j 8; 8709 == 8710; 47999.636499
clk_orig(11286900,48000): r 1 p 1 d 7100 j 8; 8710 == 8710; 48002.392090
clk(11286900,44100): r 1 p 1 d 19 j 8; 8001 == 8002; 44099.924370
clk_orig(11286900,44100): r 1 p 1 d 20 j 8; 8002 == 8002; 44100.475488
clk(12288000,48000): r 1 p 1 d 0 j 8; 8000 == 8000; 48000.000000
clk_orig(12288000,48000): r 1 p 1 d 0 j 8; 8000 == 8000; 48000.000000
clk(12288000,44100): r 1 p 1 d 3500 j 7; 7350 == 7350; 44100.000000
clk_orig(12288000,44100): r 1 p 1 d 3500 j 7; 7350 == 7350; 44100.000000
clk(33868800,48000): r 1 p 4 d 6099 j 11; 2902 == 2902; 47999.680313
clk_orig(33868800,48000): r 1 p 1 d 9020 j 2; 2902 == 2902; 47991.825000
clk(33868800,44100): r 1 p 3 d 0 j 8; 2666 == 2666; 44100.000000
clk_orig(33868800,44100): r 1 p 1 d 6660 j 2; 2666 == 2666; 44088.975000
clk(12000000,48000): r 1 p 1 d 1920 j 8; 8192 == 8192; 48000.000000
clk_orig(12000000,48000): r 1 p 1 d 1920 j 8; 8192 == 8192; 48000.000000
clk(12000000,44100): r 1 p 1 d 5264 j 7; 7526 == 7526; 44100.000000
clk_orig(12000000,44100): r 1 p 1 d 5260 j 7; 7526 == 7526; 44097.656250

where clk is my proposed code and clk_orig is the code currently in 
tlv320aic3x.c; results include all test cases mentioned

in parenthesis  is the input (sysclk, fsref), the last column is the 
approximation to fsref

note that the proposed code has less deviation from fsref in all cases and 
manages to stay within recommended settings for sysclk 33868800 

I have modified the following line to get rid of rounding errors as much 
as possible:
/* do not use codec_clk here since we'd loose precision */
d = ((2048 * p * fsref) - j * sysclk) * 100 / (sysclk/100);
and fixed a bug which always set pll_p = 1 in case d!=0

if code looks good, I'll submit a patch against linux-sound-2.6 as Mark 
suggested

regards, p.

-- 

Peter Meerwald
Kaigasse 3 / 8
A-5020 Salzburg / AUSTRIA
+43-664-2444418 (mobile)

Re: PLL computation in TLV320AIC3x SoC driver

[Vladimir Barinov]

Hello Peter,

Peter Meerwald wrote:
> Hello,
>
>   
>>>>>> I'm trying to use the SoC TLV320AIC3x codec driver with sysclk
>>>>>> 16384000 and
>>>>>> ran into some problems with setting PLL; below is a patch against
>>>>>> linux-2.6-asoc
>>>>>>             
>
>   
>>> I just tested your patch works fine with sysclk=16384000 and 12288000 but
>>> fails with 11286900: it returns FSREF=45467 instead of desired 44100. I
>>> think that this is a valuable difference. Please check it.
>>>       
>> Please check  also 33868800  sysclk, it  returns  FSREF=47545 instead of
>> 48000.
>>     
>
> I have put the proposed code at http://pmeerw.net/clk/ for review;
>
>
>   
> note that the proposed code has less deviation from fsref in all cases and 
> manages to stay within recommended settings for sysclk 33868800 
>
> I have modified the following line to get rid of rounding errors as much 
> as possible:
> /* do not use codec_clk here since we'd loose precision */
> d = ((2048 * p * fsref) - j * sysclk) * 100 / (sysclk/100);
> and fixed a bug which always set pll_p = 1 in case d!=0
>
>   
I confirm that it handles all cases with best precision now.
> if code looks good, I'll submit a patch against linux-sound-2.6 as Mark 
> suggested
>
>   
I'm ok to submit.

Re: PLL computation in TLV320AIC3x SoC driver

[Peter Meerwald]

> >>>>>> I'm trying to use the SoC TLV320AIC3x codec driver with sysclk
> >>>>>> 16384000 and
> > I have put the proposed code at http://pmeerw.net/clk/ for review;
> I confirm that it handles all cases with best precision now.
> I'm ok to submit.



fix precision of PLL computation for TLV320AIC3x SoC driver, 
test results are at http://pmeerw.net/clk

Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>

---
 sound/soc/codecs/tlv320aic3x.c |   80 ++++++++++++++++++++++++---------------
 1 files changed, 49 insertions(+), 31 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index 2b4dc2b..6f81a00 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -765,9 +765,10 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_codec *codec = socdev->card->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
 	int codec_clk = 0, bypass_pll = 0, fsref, last_clk = 0;
-	u8 data, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1;
-	u16 pll_d = 1;
+	u8 data, j, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1;
+	u16 d, pll_d = 1;
 	u8 reg;
+	int clk;
 
 	/* select data word length */
 	data =
@@ -833,48 +834,65 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	if (bypass_pll)
 		return 0;
 
-	/* Use PLL
-	 * find an apropriate setup for j, d, r and p by iterating over
-	 * p and r - j and d are calculated for each fraction.
-	 * Up to 128 values are probed, the closest one wins the game.
+	/* Use PLL, compute apropriate setup for j, d, r and p, the closest
+	 * one wins the game. Try with d==0 first, next with d!=0.
+	 * Constraints for j are according to the datasheet.
 	 * The sysclk is divided by 1000 to prevent integer overflows.
 	 */
+
 	codec_clk = (2048 * fsref) / (aic3x->sysclk / 1000);
 
-	for (r = 1; r <= 16; r++)
-		for (p = 1; p <= 8; p++) {
-			int clk, tmp = (codec_clk * pll_r * 10) / pll_p;
-			u8 j = tmp / 10000;
-			u16 d = tmp % 10000;
+    for (r = 1; r <= 16; r++)
+        for (p = 1; p <= 8; p++) {
+            for (j = 4; j <= 55; j++) {
+                /* This is actually 1000 * ((j + (d/10000)) * r) / p
+                 * The term had to be converted to get rid of the
+                 * division by 10000; d = 0 here */
+                int clk = (1000 * j * r) / p;
 
-			if (j > 63)
-				continue;
+                /* check whether this values get closer than the best
+                 * ones we had before */
+                if (abs(codec_clk - clk) < abs(codec_clk - last_clk)) {
+                    pll_j = j; pll_d = 0; pll_r = r; pll_p = p;
+                    last_clk = clk;
+                }
 
-			if (d != 0 && aic3x->sysclk < 10000000)
-				continue;
+                /* Early exit for exact matches */
+                if (clk == codec_clk)
+                    goto found;
+            }
+        }
 
-			/* This is actually 1000 * ((j + (d/10000)) * r) / p
-			 * The term had to be converted to get rid of the
-			 * division by 10000 */
-			clk = ((10000 * j * r) + (d * r)) / (10 * p);
-
-			/* check whether this values get closer than the best
-			 * ones we had before */
-			if (abs(codec_clk - clk) < abs(codec_clk - last_clk)) {
-				pll_j = j; pll_d = d; pll_r = r; pll_p = p;
-				last_clk = clk;
-			}
-
-			/* Early exit for exact matches */
-			if (clk == codec_clk)
-				break;
-		}
+    /* try with d != 0 */
+    for (p = 1; p <= 8; p++) {
+
+        j = codec_clk * p / 1000;
+
+        if (j < 4 || j > 11) continue;
+
+        /* do not use codec_clk here since we'd loose precision */
+        d = ((2048 * p * fsref) - j * aic3x->sysclk) * 100 / (aic3x->sysclk/100);
+
+        clk = (10000 * j + d) / (10 * p);
+
+        /* check whether this values get closer than the best
+         * ones we had before */
+        if (abs(codec_clk - clk) < abs(codec_clk - last_clk)) {
+            pll_j = j; pll_d = d; pll_r = 1; pll_p = p;
+            last_clk = clk;
+        }
+
+        /* Early exit for exact matches */
+        if (clk == codec_clk)
+            goto found;
+    }
 
 	if (last_clk == 0) {
 		printk(KERN_ERR "%s(): unable to setup PLL\n", __func__);
 		return -EINVAL;
 	}
 
+found:
 	data = aic3x_read_reg_cache(codec, AIC3X_PLL_PROGA_REG);
 	aic3x_write(codec, AIC3X_PLL_PROGA_REG, data | (pll_p << PLLP_SHIFT));
 	aic3x_write(codec, AIC3X_OVRF_STATUS_AND_PLLR_REG, pll_r << PLLR_SHIFT);

-- 

Peter Meerwald
Kaigasse 3 / 8
A-5020 Salzburg / AUSTRIA
+43-664-2444418 (mobile)

Re: PLL computation in TLV320AIC3x SoC driver

[Mark Brown]

On Mon, Dec 14, 2009 at 11:24:58AM +0100, Peter Meerwald wrote:

> +    /* try with d != 0 */
> +    for (p = 1; p <= 8; p++) {
> +
> +        j = codec_clk * p / 1000;

There are many codign style issues around the indentation in this code -
could you please run the patch through scripts/checkpatch.pl and clean
up the issues it identifies?

Re: PLL computation in TLV320AIC3x SoC driver

[Vladimir Barinov]

Hello Peter,

Peter Meerwald wrote:
>
> fix precision of PLL computation for TLV320AIC3x SoC driver, 
> test results are at http://pmeerw.net/clk
>
> Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
>   
if you could make checkpatch.pl passed then:
Acked-by: Vladimir Barinov <vova.barinov@gmail.com>

Regards,
Vladimir

Re: PLL computation in TLV320AIC3x SoC driver

[Peter Meerwald]

> There are many codign style issues around the indentation in this code -
> could you please run the patch through scripts/checkpatch.pl and clean
> up the issues it identifies?

cleaned up patch below...


fix precision of PLL computation for TLV320AIC3x SoC driver, 
test results are at http://pmeerw.net/clk

Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
Acked-by: Vladimir Barinov <vova.barinov@gmail.com>

---

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index 2b4dc2b..5a8f53c 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -765,9 +765,10 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_codec *codec = socdev->card->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
 	int codec_clk = 0, bypass_pll = 0, fsref, last_clk = 0;
-	u8 data, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1;
-	u16 pll_d = 1;
+	u8 data, j, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1;
+	u16 d, pll_d = 1;
 	u8 reg;
+	int clk;
 
 	/* select data word length */
 	data =
@@ -833,48 +834,70 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	if (bypass_pll)
 		return 0;
 
-	/* Use PLL
-	 * find an apropriate setup for j, d, r and p by iterating over
-	 * p and r - j and d are calculated for each fraction.
-	 * Up to 128 values are probed, the closest one wins the game.
+	/* Use PLL, compute apropriate setup for j, d, r and p, the closest
+	 * one wins the game. Try with d==0 first, next with d!=0.
+	 * Constraints for j are according to the datasheet.
 	 * The sysclk is divided by 1000 to prevent integer overflows.
 	 */
+
 	codec_clk = (2048 * fsref) / (aic3x->sysclk / 1000);
 
 	for (r = 1; r <= 16; r++)
 		for (p = 1; p <= 8; p++) {
-			int clk, tmp = (codec_clk * pll_r * 10) / pll_p;
-			u8 j = tmp / 10000;
-			u16 d = tmp % 10000;
+			for (j = 4; j <= 55; j++) {
+				/* This is actually 1000*((j+(d/10000))*r)/p
+				 * The term had to be converted to get
+				 * rid of the division by 10000; d = 0 here
+				 */
+				int clk = (1000 * j * r) / p;
+
+				/* Check whether this values get closer than
+				 * the best ones we had before
+				 */
+				if (abs(codec_clk - clk) <
+					abs(codec_clk - last_clk)) {
+					pll_j = j; pll_d = 0;
+					pll_r = r; pll_p = p;
+					last_clk = clk;
+				}
+
+				/* Early exit for exact matches */
+				if (clk == codec_clk)
+					goto found;
+			}
+		}
 
-			if (j > 63)
-				continue;
+	/* try with d != 0 */
+	for (p = 1; p <= 8; p++) {
+		j = codec_clk * p / 1000;
 
-			if (d != 0 && aic3x->sysclk < 10000000)
-				continue;
+		if (j < 4 || j > 11)
+			continue;
 
-			/* This is actually 1000 * ((j + (d/10000)) * r) / p
-			 * The term had to be converted to get rid of the
-			 * division by 10000 */
-			clk = ((10000 * j * r) + (d * r)) / (10 * p);
+		/* do not use codec_clk here since we'd loose precision */
+		d = ((2048 * p * fsref) - j * aic3x->sysclk)
+			* 100 / (aic3x->sysclk/100);
 
-			/* check whether this values get closer than the best
-			 * ones we had before */
-			if (abs(codec_clk - clk) < abs(codec_clk - last_clk)) {
-				pll_j = j; pll_d = d; pll_r = r; pll_p = p;
-				last_clk = clk;
-			}
+		clk = (10000 * j + d) / (10 * p);
 
-			/* Early exit for exact matches */
-			if (clk == codec_clk)
-				break;
+		/* check whether this values get closer than the best
+		 * ones we had before */
+		if (abs(codec_clk - clk) < abs(codec_clk - last_clk)) {
+			pll_j = j; pll_d = d; pll_r = 1; pll_p = p;
+			last_clk = clk;
 		}
 
+		/* Early exit for exact matches */
+		if (clk == codec_clk)
+			goto found;
+	}
+
 	if (last_clk == 0) {
 		printk(KERN_ERR "%s(): unable to setup PLL\n", __func__);
 		return -EINVAL;
 	}
 
+found:
 	data = aic3x_read_reg_cache(codec, AIC3X_PLL_PROGA_REG);
 	aic3x_write(codec, AIC3X_PLL_PROGA_REG, data | (pll_p << PLLP_SHIFT));
 	aic3x_write(codec, AIC3X_OVRF_STATUS_AND_PLLR_REG, pll_r << PLLR_SHIFT);

-- 

Peter Meerwald
Kaigasse 3 / 8
A-5020 Salzburg / AUSTRIA
+43-664-2444418 (mobile)

Re: PLL computation in TLV320AIC3x SoC driver

[Mark Brown]

On Mon, Dec 14, 2009 at 02:44:56PM +0100, Peter Meerwald wrote:
> 
> > There are many codign style issues around the indentation in this code -
> > could you please run the patch through scripts/checkpatch.pl and clean
> > up the issues it identifies?

> cleaned up patch below...

Applied, thanks.