[PATCH 1/3] clk: socfpga: Fix integer overflow in clock calculation

[PATCH 1/3] clk: socfpga: Fix integer overflow in clock calculation

[dinguyen at altera.com]

From: Dinh Nguyen <dinguyen@altera.com>

Use 64-bit integer for calculating clock rate. Also use do_div for the
64-bit division.

Signed-off-by: Graham Moore <grmoore@altera.com>
Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/clk/socfpga/clk-pll.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/socfpga/clk-pll.c b/drivers/clk/socfpga/clk-pll.c
index 362004e..834b6e9 100644
--- a/drivers/clk/socfpga/clk-pll.c
+++ b/drivers/clk/socfpga/clk-pll.c
@@ -44,7 +44,8 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
 					 unsigned long parent_rate)
 {
 	struct socfpga_pll *socfpgaclk = to_socfpga_clk(hwclk);
-	unsigned long divf, divq, vco_freq, reg;
+	unsigned long divf, divq, reg;
+	unsigned long long vco_freq;
 	unsigned long bypass;
 
 	reg = readl(socfpgaclk->hw.reg);
@@ -54,8 +55,9 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
 
 	divf = (reg & SOCFPGA_PLL_DIVF_MASK) >> SOCFPGA_PLL_DIVF_SHIFT;
 	divq = (reg & SOCFPGA_PLL_DIVQ_MASK) >> SOCFPGA_PLL_DIVQ_SHIFT;
-	vco_freq = parent_rate * (divf + 1);
-	return vco_freq / (1 + divq);
+	vco_freq = (unsigned long long)parent_rate * (divf + 1);
+	do_div(vco_freq, (1 + divq));
+	return (unsigned long)vco_freq;
 }
 
 static struct clk_ops clk_pll_ops = {
-- 
1.7.9.5

[PATCH 2/3] clk: socfpga: Support multiple parents for the pll clocks

[dinguyen at altera.com]

From: Dinh Nguyen <dinguyen@altera.com>

The PLLs can be from 3 different sources: osc1, osc2, or the f2s_ref_clk.
Update the clock driver to be able to get the correct parent.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/clk/socfpga/clk-pll.c |   26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/socfpga/clk-pll.c b/drivers/clk/socfpga/clk-pll.c
index 834b6e9..88dafb5 100644
--- a/drivers/clk/socfpga/clk-pll.c
+++ b/drivers/clk/socfpga/clk-pll.c
@@ -38,6 +38,9 @@
 #define SOCFPGA_PLL_DIVQ_MASK		0x003F0000
 #define SOCFPGA_PLL_DIVQ_SHIFT		16
 
+#define CLK_MGR_PLL_CLK_SRC_SHIFT	22
+#define CLK_MGR_PLL_CLK_SRC_MASK	0x3
+
 #define to_socfpga_clk(p) container_of(p, struct socfpga_pll, hw.hw)
 
 static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
@@ -60,8 +63,19 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
 	return (unsigned long)vco_freq;
 }
 
+static u8 clk_pll_get_parent(struct clk_hw *hwclk)
+{
+	u32 pll_src;
+	struct socfpga_pll *socfpgaclk = to_socfpga_clk(hwclk);
+
+	pll_src = readl(socfpgaclk->hw.reg);
+	return (pll_src >> CLK_MGR_PLL_CLK_SRC_SHIFT) &
+			CLK_MGR_PLL_CLK_SRC_MASK;
+}
+
 static struct clk_ops clk_pll_ops = {
 	.recalc_rate = clk_pll_recalc_rate,
+	.get_parent = clk_pll_get_parent,
 };
 
 static __init struct clk *__socfpga_pll_init(struct device_node *node,
@@ -71,9 +85,10 @@ static __init struct clk *__socfpga_pll_init(struct device_node *node,
 	struct clk *clk;
 	struct socfpga_pll *pll_clk;
 	const char *clk_name = node->name;
-	const char *parent_name;
+	const char *parent_name[SOCFPGA_MAX_PARENTS];
 	struct clk_init_data init;
 	int rc;
+	int i = 0;
 
 	of_property_read_u32(node, "reg", &reg);
 
@@ -88,10 +103,13 @@ static __init struct clk *__socfpga_pll_init(struct device_node *node,
 	init.name = clk_name;
 	init.ops = ops;
 	init.flags = 0;
-	parent_name = of_clk_get_parent_name(node, 0);
-	init.parent_names = parent_name ? &parent_name : NULL;
-	init.num_parents = parent_name ? 1 : 0;
 
+	while (i < SOCFPGA_MAX_PARENTS && (parent_name[i] =
+			of_clk_get_parent_name(node, i)) != NULL)
+		i++;
+
+	init.num_parents = i;
+	init.parent_names = parent_name;
 	pll_clk->hw.hw.init = &init;
 
 	pll_clk->hw.bit_idx = SOCFPGA_PLL_EXT_ENA;
-- 
1.7.9.5

[PATCH 3/3] dts: socfpga: Update clock entry to support multiple parents

[dinguyen at altera.com]

From: Dinh Nguyen <dinguyen@altera.com>

The periph_pll and sdram_pll can have multiple parents. Update the device tree
to list all the possible parents for the PLLs. Add an entry for the the
f2s_sdram_ref_clk, which is a possible parent for the sdram_pll.

Also remove the clock-frequency entry in the f2s_periph_ref_clk, as this
property should be placed in dts file.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 arch/arm/boot/dts/socfpga.dtsi |   19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 7752d7c..6d7eaa4 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -92,7 +92,12 @@
 					#address-cells = <1>;
 					#size-cells = <0>;
 
-					osc: osc1 {
+					osc1: osc1 {
+						#clock-cells = <0>;
+						compatible = "fixed-clock";
+					};
+
+					osc2: osc2 {
 						#clock-cells = <0>;
 						compatible = "fixed-clock";
 					};
@@ -100,7 +105,11 @@
 					f2s_periph_ref_clk: f2s_periph_ref_clk {
 						#clock-cells = <0>;
 						compatible = "fixed-clock";
-						clock-frequency = <10000000>;
+					};
+
+					f2s_sdram_ref_clk: f2s_sdram_ref_clk {
+						#clock-cells = <0>;
+						compatible = "fixed-clock";
 					};
 
 					main_pll: main_pll {
@@ -108,7 +117,7 @@
 						#size-cells = <0>;
 						#clock-cells = <0>;
 						compatible = "altr,socfpga-pll-clock";
-						clocks = <&osc>;
+						clocks = <&osc1>;
 						reg = <0x40>;
 
 						mpuclk: mpuclk {
@@ -162,7 +171,7 @@
 						#size-cells = <0>;
 						#clock-cells = <0>;
 						compatible = "altr,socfpga-pll-clock";
-						clocks = <&osc>;
+						clocks = <&osc1>, <&osc2>, <&f2s_periph_ref_clk>;
 						reg = <0x80>;
 
 						emac0_clk: emac0_clk {
@@ -213,7 +222,7 @@
 						#size-cells = <0>;
 						#clock-cells = <0>;
 						compatible = "altr,socfpga-pll-clock";
-						clocks = <&osc>;
+						clocks = <&osc1>, <&osc2>, <&f2s_sdram_ref_clk>;
 						reg = <0xC0>;
 
 						ddr_dqs_clk: ddr_dqs_clk {
-- 
1.7.9.5

[PATCH 1/3] clk: socfpga: Fix integer overflow in clock calculation

[Mike Turquette]

Quoting dinguyen at altera.com (2014-02-19 13:11:10)
> From: Dinh Nguyen <dinguyen@altera.com>
> 
> Use 64-bit integer for calculating clock rate. Also use do_div for the
> 64-bit division.
> 
> Signed-off-by: Graham Moore <grmoore@altera.com>
> Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
> Cc: Mike Turquette <mturquette@linaro.org>
> Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>

Changes to the clk driver look good to me.

Regards,
Mike

> ---
>  drivers/clk/socfpga/clk-pll.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/socfpga/clk-pll.c b/drivers/clk/socfpga/clk-pll.c
> index 362004e..834b6e9 100644
> --- a/drivers/clk/socfpga/clk-pll.c
> +++ b/drivers/clk/socfpga/clk-pll.c
> @@ -44,7 +44,8 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
>                                          unsigned long parent_rate)
>  {
>         struct socfpga_pll *socfpgaclk = to_socfpga_clk(hwclk);
> -       unsigned long divf, divq, vco_freq, reg;
> +       unsigned long divf, divq, reg;
> +       unsigned long long vco_freq;
>         unsigned long bypass;
>  
>         reg = readl(socfpgaclk->hw.reg);
> @@ -54,8 +55,9 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
>  
>         divf = (reg & SOCFPGA_PLL_DIVF_MASK) >> SOCFPGA_PLL_DIVF_SHIFT;
>         divq = (reg & SOCFPGA_PLL_DIVQ_MASK) >> SOCFPGA_PLL_DIVQ_SHIFT;
> -       vco_freq = parent_rate * (divf + 1);
> -       return vco_freq / (1 + divq);
> +       vco_freq = (unsigned long long)parent_rate * (divf + 1);
> +       do_div(vco_freq, (1 + divq));
> +       return (unsigned long)vco_freq;
>  }
>  
>  static struct clk_ops clk_pll_ops = {
> -- 
> 1.7.9.5
> 

[PATCH 1/3] clk: socfpga: Fix integer overflow in clock calculation

[Dinh Nguyen]

On Wed, 2014-02-26 at 00:37 -0800, Mike Turquette wrote:
> Quoting dinguyen at altera.com (2014-02-19 13:11:10)
> > From: Dinh Nguyen <dinguyen@altera.com>
> > 
> > Use 64-bit integer for calculating clock rate. Also use do_div for the
> > 64-bit division.
> > 
> > Signed-off-by: Graham Moore <grmoore@altera.com>
> > Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
> > Cc: Mike Turquette <mturquette@linaro.org>
> > Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> 
> Changes to the clk driver look good to me.
> 

Can you apply the first 2 patches to your tree? I'll send the 3rd dts
patch through the arm-soc tree.

Thanks alot...
Dinh
> Regards,
> Mike
> 
> > ---
> >  drivers/clk/socfpga/clk-pll.c |    8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/clk/socfpga/clk-pll.c b/drivers/clk/socfpga/clk-pll.c
> > index 362004e..834b6e9 100644
> > --- a/drivers/clk/socfpga/clk-pll.c
> > +++ b/drivers/clk/socfpga/clk-pll.c
> > @@ -44,7 +44,8 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
> >                                          unsigned long parent_rate)
> >  {
> >         struct socfpga_pll *socfpgaclk = to_socfpga_clk(hwclk);
> > -       unsigned long divf, divq, vco_freq, reg;
> > +       unsigned long divf, divq, reg;
> > +       unsigned long long vco_freq;
> >         unsigned long bypass;
> >  
> >         reg = readl(socfpgaclk->hw.reg);
> > @@ -54,8 +55,9 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
> >  
> >         divf = (reg & SOCFPGA_PLL_DIVF_MASK) >> SOCFPGA_PLL_DIVF_SHIFT;
> >         divq = (reg & SOCFPGA_PLL_DIVQ_MASK) >> SOCFPGA_PLL_DIVQ_SHIFT;
> > -       vco_freq = parent_rate * (divf + 1);
> > -       return vco_freq / (1 + divq);
> > +       vco_freq = (unsigned long long)parent_rate * (divf + 1);
> > +       do_div(vco_freq, (1 + divq));
> > +       return (unsigned long)vco_freq;
> >  }
> >  
> >  static struct clk_ops clk_pll_ops = {
> > -- 
> > 1.7.9.5
> > 

[PATCH 1/3] clk: socfpga: Fix integer overflow in clock calculation

[Mike Turquette]

Quoting Dinh Nguyen (2014-02-26 07:33:06)
> On Wed, 2014-02-26 at 00:37 -0800, Mike Turquette wrote:
> > Quoting dinguyen at altera.com (2014-02-19 13:11:10)
> > > From: Dinh Nguyen <dinguyen@altera.com>
> > > 
> > > Use 64-bit integer for calculating clock rate. Also use do_div for the
> > > 64-bit division.
> > > 
> > > Signed-off-by: Graham Moore <grmoore@altera.com>
> > > Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
> > > Cc: Mike Turquette <mturquette@linaro.org>
> > > Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > 
> > Changes to the clk driver look good to me.
> > 
> 
> Can you apply the first 2 patches to your tree? I'll send the 3rd dts
> patch through the arm-soc tree.

Done!

Regards,
Mike

> 
> Thanks alot...
> Dinh
> > Regards,
> > Mike
> > 
> > > ---
> > >  drivers/clk/socfpga/clk-pll.c |    8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/clk/socfpga/clk-pll.c b/drivers/clk/socfpga/clk-pll.c
> > > index 362004e..834b6e9 100644
> > > --- a/drivers/clk/socfpga/clk-pll.c
> > > +++ b/drivers/clk/socfpga/clk-pll.c
> > > @@ -44,7 +44,8 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
> > >                                          unsigned long parent_rate)
> > >  {
> > >         struct socfpga_pll *socfpgaclk = to_socfpga_clk(hwclk);
> > > -       unsigned long divf, divq, vco_freq, reg;
> > > +       unsigned long divf, divq, reg;
> > > +       unsigned long long vco_freq;
> > >         unsigned long bypass;
> > >  
> > >         reg = readl(socfpgaclk->hw.reg);
> > > @@ -54,8 +55,9 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
> > >  
> > >         divf = (reg & SOCFPGA_PLL_DIVF_MASK) >> SOCFPGA_PLL_DIVF_SHIFT;
> > >         divq = (reg & SOCFPGA_PLL_DIVQ_MASK) >> SOCFPGA_PLL_DIVQ_SHIFT;
> > > -       vco_freq = parent_rate * (divf + 1);
> > > -       return vco_freq / (1 + divq);
> > > +       vco_freq = (unsigned long long)parent_rate * (divf + 1);
> > > +       do_div(vco_freq, (1 + divq));
> > > +       return (unsigned long)vco_freq;
> > >  }
> > >  
> > >  static struct clk_ops clk_pll_ops = {
> > > -- 
> > > 1.7.9.5
> > > 
> 
> 

[PATCH 1/3] clk: socfpga: Fix integer overflow in clock calculation

[Uwe Kleine-König]

On Wed, Feb 19, 2014 at 03:11:10PM -0600, dinguyen at altera.com wrote:
> From: Dinh Nguyen <dinguyen@altera.com>
> 
> Use 64-bit integer for calculating clock rate. Also use do_div for the
> 64-bit division.
Some details would be interesting, depending on that some clever math
would be a cheaper alternative.
 
> Signed-off-by: Graham Moore <grmoore@altera.com>
> Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
> Cc: Mike Turquette <mturquette@linaro.org>
> Cc: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  drivers/clk/socfpga/clk-pll.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/socfpga/clk-pll.c b/drivers/clk/socfpga/clk-pll.c
> index 362004e..834b6e9 100644
> --- a/drivers/clk/socfpga/clk-pll.c
> +++ b/drivers/clk/socfpga/clk-pll.c
> @@ -44,7 +44,8 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
>  					 unsigned long parent_rate)
>  {
>  	struct socfpga_pll *socfpgaclk = to_socfpga_clk(hwclk);
> -	unsigned long divf, divq, vco_freq, reg;
> +	unsigned long divf, divq, reg;
> +	unsigned long long vco_freq;
>  	unsigned long bypass;
>  
>  	reg = readl(socfpgaclk->hw.reg);
> @@ -54,8 +55,9 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
>  
>  	divf = (reg & SOCFPGA_PLL_DIVF_MASK) >> SOCFPGA_PLL_DIVF_SHIFT;
>  	divq = (reg & SOCFPGA_PLL_DIVQ_MASK) >> SOCFPGA_PLL_DIVQ_SHIFT;
> -	vco_freq = parent_rate * (divf + 1);
> -	return vco_freq / (1 + divq);
> +	vco_freq = (unsigned long long)parent_rate * (divf + 1);
> +	do_div(vco_freq, (1 + divq));
> +	return (unsigned long)vco_freq;

You want

	f = some value in [1; 8192]
	q = some value in [1; 64]

	return parent_rate * f / q;

(with mathematic rounding?). Depending on the values not only
parent_rate * f doesn't fit into a 32 bit value, but also
parent_rate * f / q. This isn't catched.

Something like that might work, too:

	i = parent_rate // q
	j = parent_rate % q

	return i * f + j * f / q

j * f cannot overflow because they are in the range [0; 63] and
[1; 8192] respectively. And if i * f overflows your vco_freq above is to
big to fit into an unsigned long, too.

I didn't check with paper and pencil or some testing if the results
match, but I think they do. I'm not sure how the two approaches compare
performance-wise, you use a 64bit division, I'm using two 32 bit
divisions. But I wouldn't be surprised if my approach was faster.
(Also note that gcc -O3 fails to use only two divisions for my algorithm
but uses three instead. There might be a kernel helper that is able to
calculate i and j above with a single division though.)

Also note that the cast in the return statement isn't needed. (It might
be good to have it though to signal that you might loose some bits for
the human reader though.)

(Reference: This is what I looked at:
	$ cat multdiv.c 
	unsigned multdiv_ukl(unsigned parent_rate, unsigned f, unsigned q)
	{
		unsigned i, j;

		i = parent_rate / q;
		j = parent_rate % q;

		return i * f + j * f / q;
	}

	unsigned multdiv_dinguyen(unsigned parent_rate, unsigned f, unsigned q)
	{
		return (unsigned long long)parent_rate * f / q;
	}
	$ arm-gcc -S -o - -O3 multdiv_ukl.c
	[...]
)

Best regards
Uwe

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