ARM: imx: correct Audio/Video PLL rate calculation formula

PATCH Superseded Anson Huang 3 participants 4 messages May 7, 2015 1 message not in the archive

Subsystems (3)

Arm i.MX and Layerscape Common clk NXP i.MX clock drivers

lore

Share & tools

lore

Anson Huang

author

2015-05-07 13:56 UTC #0 lore raw

PATCH v2 1/1 ARM: imx: correct Audio/Video PLL rate calculation formula

The audio/video PLL's rate calculation formula is as below in RM:

Fref * (DIV_SELECT + NUM / DENOM),

in original clk-pllv3's code, below code is used:

(parent_rate * div) + ((parent_rate / mfd) * mfn

as it does NOT consider the float data using div, so below calculation formula should be used as a decent method:

(parent_rate * div) + ((parent_rate * mfn) / mfd)

and we also need to consider parent_rate * mfd may exceed a 32 bit value, 64 bit value should be used.

Below is one example of old/new formula's difference:

On i.MX7D, DRAM PLL is a Audio/Video type PLL, the target freq is 1066MHz, the register settings are as below:

PLL_DDRn: 8060202C -> div = 0x2C
DDR_NUM: 06AAAC4D -> mfn = 0x6AAAC4D
DDR_DENOM: 100003EC -> mfd = 0x100003EC

parent_rate = 24MHz.

with old formula, the (parent_rate / mfd) * mfn = 0 with new formula, the (parent_rate * mfn) / mfd = 10, so old formula gets PLL_DDR = 1056MHz, new formuls gets PLL_DDR = 1066MHz.

Signed-off-by Anson Huang <[email protected]>

---
 drivers/clk/imx/clk-pllv3.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/clk/imx/clk-pllv3.c +6 -1

index 641ebc5..b2f8893 100644
@@ -203,8 +203,13 @@ static unsigned long clk_pllv3_av_recalc_rate(struct clk_hw *hw,
 	u32 mfn = readl_relaxed(pll->base + PLL_NUM_OFFSET);
 	u32 mfd = readl_relaxed(pll->base + PLL_DENOM_OFFSET);
 	u32 div = readl_relaxed(pll->base) & pll->div_mask;
+	s64 temp64 = parent_rate;
+
+	temp64 *= mfn;
+	do_div(temp64, mfd);
+
+	return (parent_rate * div) + temp64;
 
-	return (parent_rate * div) + ((parent_rate / mfd) * mfn);
 }
 
 static long clk_pllv3_av_round_rate(struct clk_hw *hw, unsigned long rate,

Signature · 1 line

1.9.1

Lothar Waßmann

2015-05-07 07:21 UTC #1 lore raw

PATCH v2 1/1 ARM: imx: correct Audio/Video PLL rate calculation formula

Hi,

Quoted 46 lines

The audio/video PLL's rate calculation formula is as below in RM:
Fref * (DIV_SELECT + NUM / DENOM),
in original clk-pllv3's code, below code is used:
(parent_rate * div) + ((parent_rate / mfd) * mfn
as it does NOT consider the float data using div, so below calculation formula should be used as a decent method:
(parent_rate * div) + ((parent_rate * mfn) / mfd)
and we also need to consider parent_rate * mfd may exceed a 32 bit value, 64 bit value should be used.
Below is one example of old/new formula's difference:
On i.MX7D, DRAM PLL is a Audio/Video type PLL, the target freq is 1066MHz, the register settings are as below:
PLL_DDRn: 8060202C -> div = 0x2C
DDR_NUM: 06AAAC4D -> mfn = 0x6AAAC4D
DDR_DENOM: 100003EC -> mfd = 0x100003EC
parent_rate = 24MHz.
with old formula, the (parent_rate / mfd) * mfn = 0 with new formula, the (parent_rate * mfn) / mfd = 10, so old formula gets PLL_DDR = 1056MHz, new formuls gets PLL_DDR = 1066MHz.
Signed-off-by Anson Huang <[email protected]>
---
 drivers/clk/imx/clk-pllv3.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/clk/imx/clk-pllv3.c +1 -0
index 641ebc5..b2f8893 100644
@@ -203,8 +203,13 @@ static unsigned long clk_pllv3_av_recalc_rate(struct clk_hw *hw,
 	u32 mfn = readl_relaxed(pll->base + PLL_NUM_OFFSET);
 	u32 mfd = readl_relaxed(pll->base + PLL_DENOM_OFFSET);
 	u32 div = readl_relaxed(pll->base) & pll->div_mask;
+	s64 temp64 = parent_rate;

drivers/clk/imx/clk-pllv3.c static unsigned long clk_pllv3_av_recalc_rate(struct clk_hw *hw, · line 215

Quoted 10 lines

drivers/clk/imx/clk-pllv3.c +5 -1

+
+	temp64 *= mfn;
+	do_div(temp64, mfd);
+
+	return (parent_rate * div) + temp64;
 
-	return (parent_rate * div) + ((parent_rate / mfd) * mfn);
 }
 
 static long clk_pllv3_av_round_rate(struct clk_hw *hw, unsigned long rate,

This function should be updated too, since it uses the same formula as above!

Lothar Wa?mann

Signature · 9 lines

___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

Uwe Kleine-König

2015-05-07 07:25 UTC #2 lore raw

PATCH v2 1/1 ARM: imx: correct Audio/Video PLL rate calculation formula

In reply to Anson Huang

Hello,

Quoted 7 lines · Anson Huang

The audio/video PLL's rate calculation formula is as below in RM:
Fref * (DIV_SELECT + NUM / DENOM),
in original clk-pllv3's code, below code is used:
(parent_rate * div) + ((parent_rate / mfd) * mfn

Here a closing ) is missing

Quoted 14 lines

as it does NOT consider the float data using div, so below calculation formula should be used as a decent method:
(parent_rate * div) + ((parent_rate * mfn) / mfd)
and we also need to consider parent_rate * mfd may exceed a 32 bit value, 64 bit value should be used.
Below is one example of old/new formula's difference:
On i.MX7D, DRAM PLL is a Audio/Video type PLL, the target freq is 1066MHz, the register settings are as below:

Quoted 8 lines

PLL_DDRn: 8060202C -> div = 0x2C
DDR_NUM: 06AAAC4D -> mfn = 0x6AAAC4D
DDR_DENOM: 100003EC -> mfd = 0x100003EC
parent_rate = 24MHz.
with old formula, the (parent_rate / mfd) * mfn = 0 with new formula, the (parent_rate * mfn) / mfd = 10, so old formula gets

That's wrong. (parent_rate * mfn) / mfd == 10000000.

Quoted 1 line

PLL_DDR = 1056MHz, new formuls gets PLL_DDR = 1066MHz.

So which one is better? To see this it would be nice to also point out the exact value here.

Another way to get to a more exact calculation but without a 64 bit division is using shifted math. The formula gets more difficult though, so probably not worth the effort.

Quoted 10 lines

Signed-off-by Anson Huang <[email protected]>
---
 drivers/clk/imx/clk-pllv3.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/clk/imx/clk-pllv3.c +0 -0
index 641ebc5..b2f8893 100644

drivers/clk/imx/clk-pllv3.c static unsigned long clk_pllv3_av_recalc_rate(struct clk_hw *hw, · line 206

Quoted 4 lines

drivers/clk/imx/clk-pllv3.c +1 -0

@@ -203,8 +203,13 @@ static unsigned long clk_pllv3_av_recalc_rate(struct clk_hw *hw,
 	u32 mfn = readl_relaxed(pll->base + PLL_NUM_OFFSET);
 	u32 mfd = readl_relaxed(pll->base + PLL_DENOM_OFFSET);
 	u32 div = readl_relaxed(pll->base) & pll->div_mask;
+	s64 temp64 = parent_rate;

Why are you using a signed type here? do_div used unsigned anyhow IIRC.

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

A message that is not in the archive (sent off-list or not yet received): [email protected]

Uwe Kleine-König

2015-05-07 07:58 UTC #4 lore raw

PATCH v2 1/1 ARM: imx: correct Audio/Video PLL rate calculation formula

Hello,

Quoted 41 lines · Anson Huang

Quoted 12 lines · Uwe Kleine-K?nig
Hello,
Quoted 7 lines · Anson Huang
The audio/video PLL's rate calculation formula is as below in RM:
Fref * (DIV_SELECT + NUM / DENOM),
in original clk-pllv3's code, below code is used:
(parent_rate * div) + ((parent_rate / mfd) * mfn
Here a closing ) is missing
Thanks, will fix it later.
Quoted 18 lines
Quoted 18 lines
as it does NOT consider the float data using div, so below calculation formula should be used as a decent method:
(parent_rate * div) + ((parent_rate * mfn) / mfd)
and we also need to consider parent_rate * mfd may exceed a 32 bit value, 64 bit value should be used.
Below is one example of old/new formula's difference:
On i.MX7D, DRAM PLL is a Audio/Video type PLL, the target freq is 1066MHz, the register settings are as below:
PLL_DDRn: 8060202C -> div = 0x2C
DDR_NUM: 06AAAC4D -> mfn = 0x6AAAC4D
DDR_DENOM: 100003EC -> mfd = 0x100003EC
Quoted 6 lines
Quoted 4 lines
parent_rate = 24MHz.
with old formula, the (parent_rate / mfd) * mfn = 0 with new formula, the (parent_rate * mfn) / mfd = 10, so old formula gets
That's wrong. (parent_rate * mfn) / mfd == 10000000.

Quoted 7 lines

OK, I forgot to mention the result is 10MHz.
Quoted 3 lines
Quoted 1 line
PLL_DDR = 1056MHz, new formuls gets PLL_DDR = 1066MHz.
So which one is better? To see this it would be nice to also point out the exact value here.
"the target freq is 1066MHz", means 1066MHz should be the correct value.

I would still write:

So the old formula results in 1056 MHz, while the updated calculation yields 1066 MHz which exactly matches the real rate.

Quoted 7 lines

Quoted 4 lines
Another way to get to a more exact calculation but without a 64 bit division is using shifted math. The formula gets more difficult though, so probably not worth the effort.
It is NOT easy to understand if using shift, and in other functions of this file, they all use div, so keeping them aligned is also acceptable?

sure, I quickly tried to come up with the needed formula that uses shifted math, but dropped it after a few minutes. As the calculation isn't done in a fast patch probably keeping the 64bit division is the way to go.

Quoted 16 lines

Quoted 5 lines
Quoted 5 lines
Signed-off-by Anson Huang <[email protected]>
---
 drivers/clk/imx/clk-pllv3.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
Quoted 11 lines
drivers/clk/imx/clk-pllv3.c static unsigned long clk_pllv3_av_recalc_rate(struct clk_hw *hw, · line 206
Quoted 9 lines
drivers/clk/imx/clk-pllv3.c +1 -0
index 641ebc5..b2f8893 100644
@@ -203,8 +203,13 @@ static unsigned long clk_pllv3_av_recalc_rate(struct clk_hw *hw,
 	u32 mfn = readl_relaxed(pll->base + PLL_NUM_OFFSET);
 	u32 mfd = readl_relaxed(pll->base + PLL_DENOM_OFFSET);
 	u32 div = readl_relaxed(pll->base) & pll->div_mask;
+	s64 temp64 = parent_rate;
Why are you using a signed type here? do_div used unsigned anyhow IIRC.

Quoted 3 lines

I use u64 before, but when I did V2 patch, I notice that all the div used in this file are using a s64, so I change it to s64 to keep them aligned, so which one is the decent one?

Looking at the implementation of do_div it used unsigned math. So better use u64 and change the other variables to u64, too (in a separate patch).

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |