bounds of pdiv in clk-pll14xx.c

bounds of pdiv in clk-pll14xx.c

[Rasmus Villemoes]

I'm a bit confused by the range of pdiv used in
imx_pll14xx_calc_settings(), introduced in commit b09c68dc57c9 (clk:
imx: pll14xx: Support dynamic rates).

We have this comment

        /*
         * Fractional PLL constrains:
         *
         * a) 6MHz <= prate <= 25MHz
         * b) 1 <= p <= 63 (1 <= p <= 4 prate = 24MHz)
         * c) 64 <= m <= 1023
         * d) 0 <= s <= 6
         * e) -32768 <= k <= 32767

and those values match what I can find in the reference manuals for the
imx8mm, imx8mn and imx8mp SOCs. But the code then only loops over 1 <= p
<= 7. I also don't really understand what the parenthesis

   (1 <= p <= 4 prate = 24MHz)

is supposed to mean. Is p restricted to <= 4 when the parent rate is
24MHz? That doesn't seem to make any sense, and in any case the loop
does go up to p==7.

It also seems that the built-in entries for 393216000 and 361267200 are
suboptimal. Using m=655, p=5, s=3, k=23593 would give 393216000 exactly,
and that set would be found by the loop if it wasn't being preceded by
the table lookup.

Similarly, m=151, p=5, s=1, k=-30933 would be found and results in
361267199, while an exact match (well, at least within 1Hz) is possible
with the wider range of p values: m=497, p=33, s=0, k=-16882.

I could understand if the hardware also imposes some limits on e.g. the
value of intermediate expressions like (m + k/65536)*F_in, but I can't
find any restrictions beyond those in the above comment (disregarding
the mysterious parenthetical).

So, what's the reality here?

Rasmus

Re: bounds of pdiv in clk-pll14xx.c

[Marco Felsch]

Hi,

+To Adrian since Sascha mentioned him in his commit.

On 23-06-21, Rasmus Villemoes wrote:
> I'm a bit confused by the range of pdiv used in
> imx_pll14xx_calc_settings(), introduced in commit b09c68dc57c9 (clk:
> imx: pll14xx: Support dynamic rates).
> 
> We have this comment
> 
>         /*
>          * Fractional PLL constrains:
>          *
>          * a) 6MHz <= prate <= 25MHz
>          * b) 1 <= p <= 63 (1 <= p <= 4 prate = 24MHz)
>          * c) 64 <= m <= 1023
>          * d) 0 <= s <= 6
>          * e) -32768 <= k <= 32767
> 
> and those values match what I can find in the reference manuals for the
> imx8mm, imx8mn and imx8mp SOCs. But the code then only loops over 1 <= p
> <= 7. I also don't really understand what the parenthesis
> 
>    (1 <= p <= 4 prate = 24MHz)
> 
> is supposed to mean. Is p restricted to <= 4 when the parent rate is
> 24MHz? That doesn't seem to make any sense, and in any case the loop
> does go up to p==7.

I have the exact same question since the '1 <= 7' restriction stops me
from getting the most precise video-pll frequency. If I set the
restiction according the reference manual I get an exact match.

With the following diff I be able to get an exact match:

8<---------------------------------------------------------------
diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index 7150c59bbfc95..d1d0d25fea2ce 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -186,7 +186,7 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat
 	}
 
 	/* Finally calculate best values */
-	for (pdiv = 1; pdiv <= 7; pdiv++) {
+	for (pdiv = 1; pdiv <= 63; pdiv++) {
 		for (sdiv = 0; sdiv <= 6; sdiv++) {
 			/* calc mdiv = round(rate * pdiv * 2^sdiv) / prate) */
 			mdiv = DIV_ROUND_CLOSEST(rate * (pdiv << sdiv), prate);
8<---------------------------------------------------------------

Regards,
  Marco

RE: [EXT] Re: bounds of pdiv in clk-pll14xx.c

[Adrian Alonso]

Hi,

+Bligaard

FRef for Audio/Video PLLs are usually 24Mhz/25Mhz;

But most common use case for dynamic reconf is for Audio PLL
Where prate = 2 4Mhz (FRef) could derive 44.1khz/44khz sample rates

b) 1 <= p <= 63; (1 <= p <= 4 if prate = 24MHz);

Found out this old commit log:

clk: imx: dynamic audio pll rate settings

Add support for dynamic audio pll rate settings
Calculate optimal dividers close to required user freq request
Fractional PLL constrains:
 a). 6MHz <= Fref <= 25MHz;
 b). 1 <= p <= 63; if Fref is external cristal Fref = 24Mhz
     1 <= p <= 4;
 c). 64 <= m <= 1023;
 d). 0 <= s <= 6;
 e). -32768 <= k <= 32767;

Usage example:
------------------------------------------------------------
cat /sys/devices/platform/30030000.sai/pll1
722534400
echo 589824000 > /sys/devices/platform/30030000.sai/pll1
------------------------------------------------------------

clk_int_pll1443x_recalc_rate: 589823982:393:1:4:14155
mdiv = 393; pdiv = 1; sdiv = 4; kdiv = 14155;
Audio PLL rate = 589823982 Hz

cat /sys/kernel/debug/clk/clk_summary
------------------------------------------------------------
 audio_pll2_ref_sel  0            0    24000000 0 0
  audio_pll2         0            0   589823982 0 0
   audio_pll2_bypass 0            0   589823982 0 0
    audio_pll2_out   0            0   589823982 0 0

Regards
Adrian

-----Original Message-----
From: Marco Felsch <m.felsch@pengutronix.de> 
Sent: Thursday, July 13, 2023 12:55 PM
To: Rasmus Villemoes <rasmus.villemoes@prevas.dk>; Adrian Alonso <adrian.alonso@nxp.com>
Cc: Abel Vesa <abelvesa@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>; linux-clk <linux-clk@vger.kernel.org>; Pengutronix Kernel Team <kernel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>
Subject: [EXT] Re: bounds of pdiv in clk-pll14xx.c

Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button


Hi,

+To Adrian since Sascha mentioned him in his commit.

On 23-06-21, Rasmus Villemoes wrote:
> I'm a bit confused by the range of pdiv used in 
> imx_pll14xx_calc_settings(), introduced in commit b09c68dc57c9 (clk:
> imx: pll14xx: Support dynamic rates).
>
> We have this comment
>
>         /*
>          * Fractional PLL constrains:
>          *
>          * a) 6MHz <= prate <= 25MHz
>          * b) 1 <= p <= 63 (1 <= p <= 4 prate = 24MHz)
>          * c) 64 <= m <= 1023
>          * d) 0 <= s <= 6
>          * e) -32768 <= k <= 32767
>
> and those values match what I can find in the reference manuals for 
> the imx8mm, imx8mn and imx8mp SOCs. But the code then only loops over 
> 1 <= p <= 7. I also don't really understand what the parenthesis
>
>    (1 <= p <= 4 prate = 24MHz)
>
> is supposed to mean. Is p restricted to <= 4 when the parent rate is 
> 24MHz? That doesn't seem to make any sense, and in any case the loop 
> does go up to p==7.

I have the exact same question since the '1 <= 7' restriction stops me from getting the most precise video-pll frequency. If I set the restiction according the reference manual I get an exact match.

With the following diff I be able to get an exact match:

8<---------------------------------------------------------------
diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c index 7150c59bbfc95..d1d0d25fea2ce 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -186,7 +186,7 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat
        }

        /* Finally calculate best values */
-       for (pdiv = 1; pdiv <= 7; pdiv++) {
+       for (pdiv = 1; pdiv <= 63; pdiv++) {
                for (sdiv = 0; sdiv <= 6; sdiv++) {
                        /* calc mdiv = round(rate * pdiv * 2^sdiv) / prate) */
                        mdiv = DIV_ROUND_CLOSEST(rate * (pdiv << sdiv), prate);
8<---------------------------------------------------------------

Regards,
  Marco

Re: [EXT] Re: bounds of pdiv in clk-pll14xx.c

[Marco Felsch]

Hi Adrian,

thanks for the fast reply :)

On 23-07-13, Adrian Alonso wrote:
> Hi,
> 
> +Bligaard
> 
> FRef for Audio/Video PLLs are usually 24Mhz/25Mhz;

All PLLs are sourced by the external 24MHz osc if I understood the
i.MX8M{N,M,P} reference manuals correctly.

> But most common use case for dynamic reconf is for Audio PLL
> Where prate = 2 4Mhz (FRef) could derive 44.1khz/44khz sample rates

Video-plls are very device/use-case specific too.

> b) 1 <= p <= 63; (1 <= p <= 4 if prate = 24MHz);
> 
> Found out this old commit log:
> 
> clk: imx: dynamic audio pll rate settings
> 
> Add support for dynamic audio pll rate settings
> Calculate optimal dividers close to required user freq request
> Fractional PLL constrains:
>  a). 6MHz <= Fref <= 25MHz;
>  b). 1 <= p <= 63; if Fref is external cristal Fref = 24Mhz
>      1 <= p <= 4;

Where is this restriction of 1 <= p <= 4 (fref = 24MHz) mentioned? I
wasn't able to find that limitation within the reference-manual nor the
datasheet.

>  c). 64 <= m <= 1023;
>  d). 0 <= s <= 6;
>  e). -32768 <= k <= 32767;
> 
> Usage example:
> ------------------------------------------------------------
> cat /sys/devices/platform/30030000.sai/pll1
> 722534400
> echo 589824000 > /sys/devices/platform/30030000.sai/pll1
> ------------------------------------------------------------
> 
> clk_int_pll1443x_recalc_rate: 589823982:393:1:4:14155
> mdiv = 393; pdiv = 1; sdiv = 4; kdiv = 14155;
> Audio PLL rate = 589823982 Hz
> 
> cat /sys/kernel/debug/clk/clk_summary
> ------------------------------------------------------------
>  audio_pll2_ref_sel  0            0    24000000 0 0
>   audio_pll2         0            0   589823982 0 0
>    audio_pll2_bypass 0            0   589823982 0 0
>     audio_pll2_out   0            0   589823982 0 0

With the reference manual mentioned: 1 <= p <= 63 restriction you may
find the exact clock rate of: 589824000. So question is why do we have
this limitation?

Regards,
  Marco

Re: [EXT] Re: bounds of pdiv in clk-pll14xx.c

[Adam Ford]

On Fri, Jul 14, 2023 at 2:22 AM Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> Hi Adrian,
>
> thanks for the fast reply :)
>
> On 23-07-13, Adrian Alonso wrote:
> > Hi,
> >
> > +Bligaard
> >
> > FRef for Audio/Video PLLs are usually 24Mhz/25Mhz;
>
> All PLLs are sourced by the external 24MHz osc if I understood the
> i.MX8M{N,M,P} reference manuals correctly.
>
> > But most common use case for dynamic reconf is for Audio PLL
> > Where prate = 2 4Mhz (FRef) could derive 44.1khz/44khz sample rates
>
> Video-plls are very device/use-case specific too.

I have spent a bunch of time trying to troubleshoot video stuff, and
some video_pll clocks are challenging to achieve, and I was trying to
figure out how to improve this driver.

>
> > b) 1 <= p <= 63; (1 <= p <= 4 if prate = 24MHz);
> >
> > Found out this old commit log:
> >
> > clk: imx: dynamic audio pll rate settings
> >
> > Add support for dynamic audio pll rate settings
> > Calculate optimal dividers close to required user freq request
> > Fractional PLL constrains:
> >  a). 6MHz <= Fref <= 25MHz;
> >  b). 1 <= p <= 63; if Fref is external cristal Fref = 24Mhz
> >      1 <= p <= 4;
>
> Where is this restriction of 1 <= p <= 4 (fref = 24MHz) mentioned? I
> wasn't able to find that limitation within the reference-manual nor the
> datasheet.

I looked too, and I couldn't find it either.

>
> >  c). 64 <= m <= 1023;
> >  d). 0 <= s <= 6;
> >  e). -32768 <= k <= 32767;
> >
> > Usage example:
> > ------------------------------------------------------------
> > cat /sys/devices/platform/30030000.sai/pll1
> > 722534400
> > echo 589824000 > /sys/devices/platform/30030000.sai/pll1
> > ------------------------------------------------------------
> >
> > clk_int_pll1443x_recalc_rate: 589823982:393:1:4:14155
> > mdiv = 393; pdiv = 1; sdiv = 4; kdiv = 14155;
> > Audio PLL rate = 589823982 Hz
> >
> > cat /sys/kernel/debug/clk/clk_summary
> > ------------------------------------------------------------
> >  audio_pll2_ref_sel  0            0    24000000 0 0
> >   audio_pll2         0            0   589823982 0 0
> >    audio_pll2_bypass 0            0   589823982 0 0
> >     audio_pll2_out   0            0   589823982 0 0
>
> With the reference manual mentioned: 1 <= p <= 63 restriction you may
> find the exact clock rate of: 589824000. So question is why do we have
> this limitation?

When you do this, what values of mpsk do you get (or at least what value of p)?

I am going to re-test some of the video_pll stuff with p between 1 and
63 instead of 1 to 4 as well.  I am hoping it will solve some LCDIF
issues.

adam
>
> Regards,
>   Marco

[PATCH] clk: imx: pll14xx: align pdiv with reference manual

[Marco Felsch]

The PLL14xx hardware can be found on i.MX8M{M,N,P} SoCs and always come
with a 6-bit pre-divider. Neither the reference manuals nor the
datasheets of these SoCs do mention any restrictions. Furthermore the
current code doesn't respect the restrictions from the comment too.

Therefore drop the restriction and align the max pre-divider (pdiv)
value to 63 to get more accurate frequencies.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Hi Adam,

here is the patch I made for setting the exact video-pll settings.

Regards,
  Marco


 drivers/clk/imx/clk-pll14xx.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index 7150c59bbfc95..dc6bc21dff41f 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -139,11 +139,10 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat
 	/*
 	 * Fractional PLL constrains:
 	 *
-	 * a) 6MHz <= prate <= 25MHz
-	 * b) 1 <= p <= 63 (1 <= p <= 4 prate = 24MHz)
-	 * c) 64 <= m <= 1023
-	 * d) 0 <= s <= 6
-	 * e) -32768 <= k <= 32767
+	 * a) 1 <= p <= 63
+	 * b) 64 <= m <= 1023
+	 * c) 0 <= s <= 6
+	 * d) -32768 <= k <= 32767
 	 *
 	 * fvco = (m * 65536 + k) * prate / (p * 65536)
 	 */
@@ -186,7 +185,7 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat
 	}
 
 	/* Finally calculate best values */
-	for (pdiv = 1; pdiv <= 7; pdiv++) {
+	for (pdiv = 1; pdiv <= 63; pdiv++) {
 		for (sdiv = 0; sdiv <= 6; sdiv++) {
 			/* calc mdiv = round(rate * pdiv * 2^sdiv) / prate) */
 			mdiv = DIV_ROUND_CLOSEST(rate * (pdiv << sdiv), prate);
-- 
2.39.2

Re: [EXT] Re: bounds of pdiv in clk-pll14xx.c

[Ahmad Fatoum]

Hello Adrian,

[Cc'ing Peng Fan and Jacky Bay who also worked on the downstream driver]

On 13.07.23 23:59, Adrian Alonso wrote:
> Hi,
> 
> +Bligaard
> 
> FRef for Audio/Video PLLs are usually 24Mhz/25Mhz;
> 
> But most common use case for dynamic reconf is for Audio PLL
> Where prate = 2 4Mhz (FRef) could derive 44.1khz/44khz sample rates
> 
> b) 1 <= p <= 63; (1 <= p <= 4 if prate = 24MHz);
> 
> Found out this old commit log:
> 
> clk: imx: dynamic audio pll rate settings
> 
> Add support for dynamic audio pll rate settings
> Calculate optimal dividers close to required user freq request
> Fractional PLL constrains:
>  a). 6MHz <= Fref <= 25MHz;
>  b). 1 <= p <= 63; if Fref is external cristal Fref = 24Mhz

imx8mm.dtsi/imx8mn.dtsi initialize audio_pll1 and audio_pll2
with 393216000 and 361267200 Hz respectively, but the hardcoded
parameters in the table result in 393215995 and 361267196 Hz
respectively. With Marco's patch restoring 63 as upper bound for
pdiv, we can achieve the exact frequency if we just drop these
two hardcoded frequencies from the table.

I wonder why the parameters were chosen the way they are, despite
them not achieving the required frequencies exactly. Do you have
some insight into why this could be and if we shouldn't just drop:

  PLL_1443X_RATE(393216000U, 262, 2, 3, 9437),
  PLL_1443X_RATE(361267200U, 361, 3, 3, 17511),

in favor of the dynamic determination. FTR, the parameters it
chooses (with Marco's patch applied) are:

/*               rate     mdiv  pdiv  sdiv   kdiv */
PLL_1443X_RATE(393216000, 655,    5,    3,  23593),
PLL_1443X_RATE(361267200, 497,   33,    0, -16882),
 

Cheers,
Ahmad

>      1 <= p <= 4;
>  c). 64 <= m <= 1023;
>  d). 0 <= s <= 6;
>  e). -32768 <= k <= 32767;
> 
> Usage example:
> ------------------------------------------------------------
> cat /sys/devices/platform/30030000.sai/pll1
> 722534400
> echo 589824000 > /sys/devices/platform/30030000.sai/pll1
> ------------------------------------------------------------
> 
> clk_int_pll1443x_recalc_rate: 589823982:393:1:4:14155
> mdiv = 393; pdiv = 1; sdiv = 4; kdiv = 14155;
> Audio PLL rate = 589823982 Hz
> 
> cat /sys/kernel/debug/clk/clk_summary
> ------------------------------------------------------------
>  audio_pll2_ref_sel  0            0    24000000 0 0
>   audio_pll2         0            0   589823982 0 0
>    audio_pll2_bypass 0            0   589823982 0 0
>     audio_pll2_out   0            0   589823982 0 0
> 
> Regards
> Adrian

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] clk: imx: pll14xx: align pdiv with reference manual

[Abel Vesa]

On 23-07-14 15:49:38, Marco Felsch wrote:
> The PLL14xx hardware can be found on i.MX8M{M,N,P} SoCs and always come
> with a 6-bit pre-divider. Neither the reference manuals nor the
> datasheets of these SoCs do mention any restrictions. Furthermore the
> current code doesn't respect the restrictions from the comment too.
> 
> Therefore drop the restriction and align the max pre-divider (pdiv)
> value to 63 to get more accurate frequencies.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

I'm OK with this:

Reviewed-by: Abel Vesa <abel.vesa@linaro.org>

> ---
> Hi Adam,
> 
> here is the patch I made for setting the exact video-pll settings.
> 
> Regards,
>   Marco
> 
> 
>  drivers/clk/imx/clk-pll14xx.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index 7150c59bbfc95..dc6bc21dff41f 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c
> @@ -139,11 +139,10 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat
>  	/*
>  	 * Fractional PLL constrains:
>  	 *
> -	 * a) 6MHz <= prate <= 25MHz
> -	 * b) 1 <= p <= 63 (1 <= p <= 4 prate = 24MHz)
> -	 * c) 64 <= m <= 1023
> -	 * d) 0 <= s <= 6
> -	 * e) -32768 <= k <= 32767
> +	 * a) 1 <= p <= 63
> +	 * b) 64 <= m <= 1023
> +	 * c) 0 <= s <= 6
> +	 * d) -32768 <= k <= 32767
>  	 *
>  	 * fvco = (m * 65536 + k) * prate / (p * 65536)
>  	 */
> @@ -186,7 +185,7 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat
>  	}
>  
>  	/* Finally calculate best values */
> -	for (pdiv = 1; pdiv <= 7; pdiv++) {
> +	for (pdiv = 1; pdiv <= 63; pdiv++) {
>  		for (sdiv = 0; sdiv <= 6; sdiv++) {
>  			/* calc mdiv = round(rate * pdiv * 2^sdiv) / prate) */
>  			mdiv = DIV_ROUND_CLOSEST(rate * (pdiv << sdiv), prate);
> -- 
> 2.39.2
> 

Re: [PATCH] clk: imx: pll14xx: align pdiv with reference manual

[Adam Ford]

On Tue, Jul 25, 2023 at 2:38 AM Abel Vesa <abel.vesa@linaro.org> wrote:
>
> On 23-07-14 15:49:38, Marco Felsch wrote:
> > The PLL14xx hardware can be found on i.MX8M{M,N,P} SoCs and always come
> > with a 6-bit pre-divider. Neither the reference manuals nor the
> > datasheets of these SoCs do mention any restrictions. Furthermore the
> > current code doesn't respect the restrictions from the comment too.
> >
> > Therefore drop the restriction and align the max pre-divider (pdiv)
> > value to 63 to get more accurate frequencies.

I like this.

> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
>
> I'm OK with this:
>
> Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
>

Reviewed-by: Adam Ford <aford173@gmail.com>

> > ---
> > Hi Adam,
> >
> > here is the patch I made for setting the exact video-pll settings.
> >
> > Regards,
> >   Marco
> >
> >
> >  drivers/clk/imx/clk-pll14xx.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> > index 7150c59bbfc95..dc6bc21dff41f 100644
> > --- a/drivers/clk/imx/clk-pll14xx.c
> > +++ b/drivers/clk/imx/clk-pll14xx.c
> > @@ -139,11 +139,10 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat
> >       /*
> >        * Fractional PLL constrains:
> >        *
> > -      * a) 6MHz <= prate <= 25MHz
> > -      * b) 1 <= p <= 63 (1 <= p <= 4 prate = 24MHz)
> > -      * c) 64 <= m <= 1023
> > -      * d) 0 <= s <= 6
> > -      * e) -32768 <= k <= 32767
> > +      * a) 1 <= p <= 63
> > +      * b) 64 <= m <= 1023
> > +      * c) 0 <= s <= 6
> > +      * d) -32768 <= k <= 32767
> >        *
> >        * fvco = (m * 65536 + k) * prate / (p * 65536)
> >        */
> > @@ -186,7 +185,7 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat
> >       }
> >
> >       /* Finally calculate best values */
> > -     for (pdiv = 1; pdiv <= 7; pdiv++) {
> > +     for (pdiv = 1; pdiv <= 63; pdiv++) {
> >               for (sdiv = 0; sdiv <= 6; sdiv++) {
> >                       /* calc mdiv = round(rate * pdiv * 2^sdiv) / prate) */
> >                       mdiv = DIV_ROUND_CLOSEST(rate * (pdiv << sdiv), prate);
> > --
> > 2.39.2
> >

Re: bounds of pdiv in clk-pll14xx.c

[Sascha Hauer]

On Wed, Jun 21, 2023 at 01:24:12PM +0200, Rasmus Villemoes wrote:
> I'm a bit confused by the range of pdiv used in
> imx_pll14xx_calc_settings(), introduced in commit b09c68dc57c9 (clk:
> imx: pll14xx: Support dynamic rates).
> 
> We have this comment
> 
>         /*
>          * Fractional PLL constrains:
>          *
>          * a) 6MHz <= prate <= 25MHz
>          * b) 1 <= p <= 63 (1 <= p <= 4 prate = 24MHz)
>          * c) 64 <= m <= 1023
>          * d) 0 <= s <= 6
>          * e) -32768 <= k <= 32767
> 
> and those values match what I can find in the reference manuals for the
> imx8mm, imx8mn and imx8mp SOCs. But the code then only loops over 1 <= p
> <= 7. I also don't really understand what the parenthesis
> 
>    (1 <= p <= 4 prate = 24MHz)
> 
> is supposed to mean. Is p restricted to <= 4 when the parent rate is
> 24MHz? That doesn't seem to make any sense, and in any case the loop
> does go up to p==7.

The original patch from Adrian that my patch is based on indeed had 1 <=
p <= 4. I did some digging but I haven't found the reason why this was
changed to 1 <= p <= 7 in the patch I upstreamed. I can imagine that at
some point we realized that we also got suboptimal rates and therefore
changed it, but I don't know.

> 
> It also seems that the built-in entries for 393216000 and 361267200 are
> suboptimal. Using m=655, p=5, s=3, k=23593 would give 393216000 exactly,
> and that set would be found by the loop if it wasn't being preceded by
> the table lookup.

I gave the table precedence in case it contains qualified known good
values. If these values turn out to be not so good after all then I
suggest that we remove them.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] clk: imx: pll14xx: align pdiv with reference manual

[Adam Ford]

On Fri, Jul 14, 2023 at 8:49 AM Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> The PLL14xx hardware can be found on i.MX8M{M,N,P} SoCs and always come
> with a 6-bit pre-divider. Neither the reference manuals nor the
> datasheets of these SoCs do mention any restrictions. Furthermore the
> current code doesn't respect the restrictions from the comment too.
>
> Therefore drop the restriction and align the max pre-divider (pdiv)
> value to 63 to get more accurate frequencies.
>

Should this get a fixes tag since it appears to fix a bug?


> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> Hi Adam,
>
> here is the patch I made for setting the exact video-pll settings.
>
> Regards,
>   Marco
>
>
>  drivers/clk/imx/clk-pll14xx.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index 7150c59bbfc95..dc6bc21dff41f 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c
> @@ -139,11 +139,10 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat
>         /*
>          * Fractional PLL constrains:
>          *
> -        * a) 6MHz <= prate <= 25MHz
> -        * b) 1 <= p <= 63 (1 <= p <= 4 prate = 24MHz)
> -        * c) 64 <= m <= 1023
> -        * d) 0 <= s <= 6
> -        * e) -32768 <= k <= 32767
> +        * a) 1 <= p <= 63
> +        * b) 64 <= m <= 1023
> +        * c) 0 <= s <= 6
> +        * d) -32768 <= k <= 32767
>          *
>          * fvco = (m * 65536 + k) * prate / (p * 65536)
>          */
> @@ -186,7 +185,7 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat
>         }
>
>         /* Finally calculate best values */
> -       for (pdiv = 1; pdiv <= 7; pdiv++) {
> +       for (pdiv = 1; pdiv <= 63; pdiv++) {
>                 for (sdiv = 0; sdiv <= 6; sdiv++) {
>                         /* calc mdiv = round(rate * pdiv * 2^sdiv) / prate) */
>                         mdiv = DIV_ROUND_CLOSEST(rate * (pdiv << sdiv), prate);
> --
> 2.39.2
>

Re: [PATCH] clk: imx: pll14xx: align pdiv with reference manual

[Marco Felsch]

On 23-08-06, Adam Ford wrote:
> On Fri, Jul 14, 2023 at 8:49 AM Marco Felsch <m.felsch@pengutronix.de> wrote:
> >
> > The PLL14xx hardware can be found on i.MX8M{M,N,P} SoCs and always come
> > with a 6-bit pre-divider. Neither the reference manuals nor the
> > datasheets of these SoCs do mention any restrictions. Furthermore the
> > current code doesn't respect the restrictions from the comment too.
> >
> > Therefore drop the restriction and align the max pre-divider (pdiv)
> > value to 63 to get more accurate frequencies.
> >
> 
> Should this get a fixes tag since it appears to fix a bug?

Good question, I don't see it as a bug instead it has limited support.
Ahmad, prepared a bugfix since he found out that a value comming from
the pre defined pll-table is not valid. His bugfix need this patch to be
able to calc the correct value, so I think add a fixes tag would be
okay to get his bugfix backported as well.

> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > Hi Adam,
> >
> > here is the patch I made for setting the exact video-pll settings.
> >
> > Regards,
> >   Marco
> >
> >
> >  drivers/clk/imx/clk-pll14xx.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> > index 7150c59bbfc95..dc6bc21dff41f 100644
> > --- a/drivers/clk/imx/clk-pll14xx.c
> > +++ b/drivers/clk/imx/clk-pll14xx.c
> > @@ -139,11 +139,10 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat
> >         /*
> >          * Fractional PLL constrains:
> >          *
> > -        * a) 6MHz <= prate <= 25MHz
> > -        * b) 1 <= p <= 63 (1 <= p <= 4 prate = 24MHz)
> > -        * c) 64 <= m <= 1023
> > -        * d) 0 <= s <= 6
> > -        * e) -32768 <= k <= 32767
> > +        * a) 1 <= p <= 63
> > +        * b) 64 <= m <= 1023
> > +        * c) 0 <= s <= 6
> > +        * d) -32768 <= k <= 32767
> >          *
> >          * fvco = (m * 65536 + k) * prate / (p * 65536)
> >          */
> > @@ -186,7 +185,7 @@ static void imx_pll14xx_calc_settings(struct clk_pll14xx *pll, unsigned long rat
> >         }
> >
> >         /* Finally calculate best values */
> > -       for (pdiv = 1; pdiv <= 7; pdiv++) {
> > +       for (pdiv = 1; pdiv <= 63; pdiv++) {
> >                 for (sdiv = 0; sdiv <= 6; sdiv++) {
> >                         /* calc mdiv = round(rate * pdiv * 2^sdiv) / prate) */
> >                         mdiv = DIV_ROUND_CLOSEST(rate * (pdiv << sdiv), prate);
> > --
> > 2.39.2
> >
>