[PATCH 0/2] ARM: OMAP2+: Fix dpll rounding

[PATCH 0/2] ARM: OMAP2+: Fix dpll rounding

[Tomi Valkeinen]

Hi,

We had a problem with beaglebone black's HDMI output, where setting some clock
rates to the display PLL failed oddly. The main issue was that the dpll code
doesn't actually try to round the given rate, it only finds exact matches. That
is what these patches fix (well, the second patch).

However, there is another issue, which made finding and debugging this a bit
difficult. When the dpll round_rate failed, it returned ~0 as a rate. However,
drivers/clk/clk-divider.c, which called the round_rate, didn't realize that is
an error value, and so didn't fail but proceeded, which didn't lead to anything
good.

This series doesn't try to fix that other issue.

There's also a third issue that I think happens more easily after this series
is applied: there are no checks for maximum rate for the DPLLs. I think the
maximum rate is generally 2GHz, but at least my BeagleBone Black refused to
lock the display PLL with anything over 1.3GHz-1.4GHz.

High PLL clock rates happen quite easily with a setup where there's a DPLL and
an additional divider, handled by clk-divider.c. When the clk-divider.c is
asked to provide a clock rate (that could be quite low), it'll try to find a
good clock from its parent, the DPLL, that it can divide down to the requested
clock. As the DPLL driver is only limited by its multiplier and divider
register widths, it'll gladly offer 2GHz+ clocks, which eventually will fail as
they can't be locked.

So this series is only a partial fix for the clock issues. Also, while the
board debugged was BB Black, I think the same code is used and thus the same
issues are present on all (or most) omap2+ SoCs.

 Tomi

Tomi Valkeinen (2):
  ARM: OMAP2+: fix rate prints
  ARM: OMAP2+: fix dpll round_rate() to actually round

 arch/arm/mach-omap2/clkt_dpll.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

-- 
1.8.3.2

[PATCH 1/2] ARM: OMAP2+: fix rate prints

[Tomi Valkeinen]

Printing with unsigned long rates with %ld gives wrong result if the
rate is high enough. Fix this by using %lu.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 arch/arm/mach-omap2/clkt_dpll.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/clkt_dpll.c b/arch/arm/mach-omap2/clkt_dpll.c
index 924c230f8948..1f1708ef77bb 100644
--- a/arch/arm/mach-omap2/clkt_dpll.c
+++ b/arch/arm/mach-omap2/clkt_dpll.c
@@ -306,7 +306,7 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
 
 	ref_rate = __clk_get_rate(dd->clk_ref);
 	clk_name = __clk_get_name(hw->clk);
-	pr_debug("clock: %s: starting DPLL round_rate, target rate %ld\n",
+	pr_debug("clock: %s: starting DPLL round_rate, target rate %lu\n",
 		 clk_name, target_rate);
 
 	scaled_rt_rp = target_rate / (ref_rate / DPLL_SCALE_FACTOR);
@@ -342,7 +342,7 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
 		if (r == DPLL_MULT_UNDERFLOW)
 			continue;
 
-		pr_debug("clock: %s: m = %d: n = %d: new_rate = %ld\n",
+		pr_debug("clock: %s: m = %d: n = %d: new_rate = %lu\n",
 			 clk_name, m, n, new_rate);
 
 		if (target_rate == new_rate) {
@@ -354,7 +354,7 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
 	}
 
 	if (target_rate != new_rate) {
-		pr_debug("clock: %s: cannot round to rate %ld\n",
+		pr_debug("clock: %s: cannot round to rate %lu\n",
 			 clk_name, target_rate);
 		return ~0;
 	}
-- 
1.8.3.2

Re: [PATCH 1/2] ARM: OMAP2+: fix rate prints

[Paul Walmsley]

On Fri, 17 Jan 2014, Tomi Valkeinen wrote:

> Printing with unsigned long rates with %ld gives wrong result if the
> rate is high enough. Fix this by using %lu.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Thanks, queued for v3.15. 

There's a series here that converts the OMAP clock code away from signed 
longs for clock rates that was originally intended for v3.14, but it's 
been delayed.


- Paul

[PATCH 2/2] ARM: OMAP2+: fix dpll round_rate() to actually round

[Tomi Valkeinen]

omap2_dpll_round_rate() doesn't actually round the given rate, even if
the name and the description so hints. Instead it only tries to find an
exact rate match, or if that fails, return ~0 as an error.

What this basically means is that the user of the clock needs to know
what rates the dpll can support, which obviously isn't right.

This patch adds a simple method of rounding: during the iteration, the
code keeps track of the closest rate match. If no exact match is found,
the closest is returned.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 arch/arm/mach-omap2/clkt_dpll.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-omap2/clkt_dpll.c b/arch/arm/mach-omap2/clkt_dpll.c
index 1f1708ef77bb..1b4e68dfb713 100644
--- a/arch/arm/mach-omap2/clkt_dpll.c
+++ b/arch/arm/mach-omap2/clkt_dpll.c
@@ -298,6 +298,7 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
 	struct dpll_data *dd;
 	unsigned long ref_rate;
 	const char *clk_name;
+	unsigned long diff, closest_diff = ~0;
 
 	if (!clk || !clk->dpll_data)
 		return ~0;
@@ -345,20 +346,26 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
 		pr_debug("clock: %s: m = %d: n = %d: new_rate = %lu\n",
 			 clk_name, m, n, new_rate);
 
-		if (target_rate == new_rate) {
+		diff = max(target_rate, new_rate) - min(target_rate, new_rate);
+
+		if (diff < closest_diff) {
+			closest_diff = diff;
+
 			dd->last_rounded_m = m;
 			dd->last_rounded_n = n;
-			dd->last_rounded_rate = target_rate;
-			break;
+			dd->last_rounded_rate = new_rate;
+
+			if (diff == 0)
+				break;
 		}
 	}
 
-	if (target_rate != new_rate) {
+	if (closest_diff == ~0) {
 		pr_debug("clock: %s: cannot round to rate %lu\n",
 			 clk_name, target_rate);
 		return ~0;
 	}
 
-	return target_rate;
+	return dd->last_rounded_rate;
 }
 
-- 
1.8.3.2

Re: [PATCH 2/2] ARM: OMAP2+: fix dpll round_rate() to actually round

[Tony Lindgren]

* Tomi Valkeinen <tomi.valkeinen@ti.com> [140116 23:47]:
> omap2_dpll_round_rate() doesn't actually round the given rate, even if
> the name and the description so hints. Instead it only tries to find an
> exact rate match, or if that fails, return ~0 as an error.
> 
> What this basically means is that the user of the clock needs to know
> what rates the dpll can support, which obviously isn't right.
> 
> This patch adds a simple method of rounding: during the iteration, the
> code keeps track of the closest rate match. If no exact match is found,
> the closest is returned.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Paul & Tero, please ack if you want me to queue this.

Tony

> ---
>  arch/arm/mach-omap2/clkt_dpll.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/clkt_dpll.c b/arch/arm/mach-omap2/clkt_dpll.c
> index 1f1708ef77bb..1b4e68dfb713 100644
> --- a/arch/arm/mach-omap2/clkt_dpll.c
> +++ b/arch/arm/mach-omap2/clkt_dpll.c
> @@ -298,6 +298,7 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
>  	struct dpll_data *dd;
>  	unsigned long ref_rate;
>  	const char *clk_name;
> +	unsigned long diff, closest_diff = ~0;
>  
>  	if (!clk || !clk->dpll_data)
>  		return ~0;
> @@ -345,20 +346,26 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
>  		pr_debug("clock: %s: m = %d: n = %d: new_rate = %lu\n",
>  			 clk_name, m, n, new_rate);
>  
> -		if (target_rate == new_rate) {
> +		diff = max(target_rate, new_rate) - min(target_rate, new_rate);
> +
> +		if (diff < closest_diff) {
> +			closest_diff = diff;
> +
>  			dd->last_rounded_m = m;
>  			dd->last_rounded_n = n;
> -			dd->last_rounded_rate = target_rate;
> -			break;
> +			dd->last_rounded_rate = new_rate;
> +
> +			if (diff == 0)
> +				break;
>  		}
>  	}
>  
> -	if (target_rate != new_rate) {
> +	if (closest_diff == ~0) {
>  		pr_debug("clock: %s: cannot round to rate %lu\n",
>  			 clk_name, target_rate);
>  		return ~0;
>  	}
>  
> -	return target_rate;
> +	return dd->last_rounded_rate;
>  }
>  
> -- 
> 1.8.3.2
> 

Re: [PATCH 2/2] ARM: OMAP2+: fix dpll round_rate() to actually round

[Tero Kristo]

On 02/14/2014 01:00 AM, Tony Lindgren wrote:
> * Tomi Valkeinen <tomi.valkeinen@ti.com> [140116 23:47]:
>> omap2_dpll_round_rate() doesn't actually round the given rate, even if
>> the name and the description so hints. Instead it only tries to find an
>> exact rate match, or if that fails, return ~0 as an error.
>>
>> What this basically means is that the user of the clock needs to know
>> what rates the dpll can support, which obviously isn't right.
>>
>> This patch adds a simple method of rounding: during the iteration, the
>> code keeps track of the closest rate match. If no exact match is found,
>> the closest is returned.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>
> Paul & Tero, please ack if you want me to queue this.

The patches look good to me and based on quick testing they don't seem 
to cause any visible problems (namely this one), so acked.

-Tero

>
> Tony
>
>> ---
>>   arch/arm/mach-omap2/clkt_dpll.c | 17 ++++++++++++-----
>>   1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/clkt_dpll.c b/arch/arm/mach-omap2/clkt_dpll.c
>> index 1f1708ef77bb..1b4e68dfb713 100644
>> --- a/arch/arm/mach-omap2/clkt_dpll.c
>> +++ b/arch/arm/mach-omap2/clkt_dpll.c
>> @@ -298,6 +298,7 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
>>   	struct dpll_data *dd;
>>   	unsigned long ref_rate;
>>   	const char *clk_name;
>> +	unsigned long diff, closest_diff = ~0;
>>
>>   	if (!clk || !clk->dpll_data)
>>   		return ~0;
>> @@ -345,20 +346,26 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
>>   		pr_debug("clock: %s: m = %d: n = %d: new_rate = %lu\n",
>>   			 clk_name, m, n, new_rate);
>>
>> -		if (target_rate == new_rate) {
>> +		diff = max(target_rate, new_rate) - min(target_rate, new_rate);
>> +
>> +		if (diff < closest_diff) {
>> +			closest_diff = diff;
>> +
>>   			dd->last_rounded_m = m;
>>   			dd->last_rounded_n = n;
>> -			dd->last_rounded_rate = target_rate;
>> -			break;
>> +			dd->last_rounded_rate = new_rate;
>> +
>> +			if (diff == 0)
>> +				break;
>>   		}
>>   	}
>>
>> -	if (target_rate != new_rate) {
>> +	if (closest_diff == ~0) {
>>   		pr_debug("clock: %s: cannot round to rate %lu\n",
>>   			 clk_name, target_rate);
>>   		return ~0;
>>   	}
>>
>> -	return target_rate;
>> +	return dd->last_rounded_rate;
>>   }
>>
>> --
>> 1.8.3.2
>>

Re: [PATCH 2/2] ARM: OMAP2+: fix dpll round_rate() to actually round

[Paul Walmsley]

On Fri, 17 Jan 2014, Tomi Valkeinen wrote:

> omap2_dpll_round_rate() doesn't actually round the given rate, even if
> the name and the description so hints. Instead it only tries to find an
> exact rate match, or if that fails, return ~0 as an error.

In the past, we had "rate tolerance" code, which allowed the clock code to 
return an approximate rate within some margin.  See for example commit 
241d3a8dca239610d3d991bf58d4fe38c2d86fd5 ("OMAP2+: clock: remove the DPLL 
rate tolerance code").  The rate tolerance was set at compile-time, but it 
was set on a per-PLL basis, which in theory allowed PLLs responsible for 
audio, etc. to have a very low rate tolerance, but PLLs that only drove 
internal functional blocks to have a high rate tolerance.  

Part of the reason why this was removed is because some of the TI hardware 
guys didn't want any PLL rates used that were not explicitly 
characterized.

> What this basically means is that the user of the clock needs to know
> what rates the dpll can support, which obviously isn't right.

In principle I agree with you, but I'm not sure that this rate rounding 
function is the solution.

> This patch adds a simple method of rounding: during the iteration, the 
> code keeps track of the closest rate match. If no exact match is found, 
> the closest is returned.

So that's one possible rounding policy; maybe it works fine for a display 
interface PLL, at least for some values of "closest rate".  But another 
might be "only allow a selection from a set of pre-determined rates 
characterized by the silicon validation team".  Or another rounding 
function might need to select a more distant rate that minimizes jitter, 
EMI, or power consumption.  

Seems to me that there needs to be some way for a clock user to 
communicate its requirements along these lines to the clock framework for 
use by the rate rounding code.  At the very least, some kind of [min, max] 
interval seems appropriate.

Also I've often thought that it would be useful for your purposes to be 
able to query a clock to return a list or some other parametric 
description of the rates that it can provide. 


- Paul

Re: [PATCH 2/2] ARM: OMAP2+: fix dpll round_rate() to actually round

[Paul Walmsley]

On Wed, 19 Feb 2014, Paul Walmsley wrote:

> On Fri, 17 Jan 2014, Tomi Valkeinen wrote:
> 
> > This patch adds a simple method of rounding: during the iteration, the 
> > code keeps track of the closest rate match. If no exact match is found, 
> > the closest is returned.
> 
> So that's one possible rounding policy; maybe it works fine for a display 
> interface PLL, at least for some values of "closest rate".  But another 
> might be "only allow a selection from a set of pre-determined rates 
> characterized by the silicon validation team".  Or another rounding 
> function might need to select a more distant rate that minimizes jitter, 
> EMI, or power consumption.  

Thought about this some more.  Do you only need this for the DSS PLL, or 
do you need it for one of the core OMAP PLLs?

If the former, then how about modifying your patch to create a separate 
round_rate function that's only used for the DSS PLL that implements the 
behavior that you want?

That would eliminate any risk of impacting other users on the system.  And 
would also allow this change to get into the codebase much faster, since 
there's no need for clk API changes, etc.



- Paul

Re: [PATCH 2/2] ARM: OMAP2+: fix dpll round_rate() to actually round

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1952 bytes --]

On 20/02/14 21:30, Paul Walmsley wrote:
> On Wed, 19 Feb 2014, Paul Walmsley wrote:
> 
>> On Fri, 17 Jan 2014, Tomi Valkeinen wrote:
>>
>>> This patch adds a simple method of rounding: during the iteration, the 
>>> code keeps track of the closest rate match. If no exact match is found, 
>>> the closest is returned.
>>
>> So that's one possible rounding policy; maybe it works fine for a display 
>> interface PLL, at least for some values of "closest rate".  But another 
>> might be "only allow a selection from a set of pre-determined rates 
>> characterized by the silicon validation team".  Or another rounding 
>> function might need to select a more distant rate that minimizes jitter, 
>> EMI, or power consumption.  
> 
> Thought about this some more.  Do you only need this for the DSS PLL, or 
> do you need it for one of the core OMAP PLLs?
> 
> If the former, then how about modifying your patch to create a separate 
> round_rate function that's only used for the DSS PLL that implements the 
> behavior that you want?
> 
> That would eliminate any risk of impacting other users on the system.  And 
> would also allow this change to get into the codebase much faster, since 
> there's no need for clk API changes, etc.

The DSS internal PLLs are handled by the DSS driver, which does all
kinds of iteration to find good clocks. This patch is for a dedicated
display PLL, present on, for example, BeagleBoneBlack.

If you think that's better approach, I can take a look how it can be
done (I'm not too familiar with the clock framework). Or maybe there's a
possibility to have a flag of some kind, which allows rounded values to
be returned? That sounds like an easy addition too.

Note that the same change is needed for DT and non-DT boots. Having
separate round function would mean create a new clock "driver" (i.e.
compatibility string), wouldn't it? Adding a flag sounds easier.

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] ARM: OMAP2+: fix dpll round_rate() to actually round

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 4991 bytes --]

On 20/02/14 21:30, Paul Walmsley wrote:
> On Wed, 19 Feb 2014, Paul Walmsley wrote:
> 
>> On Fri, 17 Jan 2014, Tomi Valkeinen wrote:
>>
>>> This patch adds a simple method of rounding: during the iteration, the 
>>> code keeps track of the closest rate match. If no exact match is found, 
>>> the closest is returned.
>>
>> So that's one possible rounding policy; maybe it works fine for a display 
>> interface PLL, at least for some values of "closest rate".  But another 
>> might be "only allow a selection from a set of pre-determined rates 
>> characterized by the silicon validation team".  Or another rounding 
>> function might need to select a more distant rate that minimizes jitter, 
>> EMI, or power consumption.  
> 
> Thought about this some more.  Do you only need this for the DSS PLL, or 
> do you need it for one of the core OMAP PLLs?
> 
> If the former, then how about modifying your patch to create a separate 
> round_rate function that's only used for the DSS PLL that implements the 
> behavior that you want?
> 
> That would eliminate any risk of impacting other users on the system.  And 
> would also allow this change to get into the codebase much faster, since 
> there's no need for clk API changes, etc.

How about this one:

From f5a78303411e9192899a6a681acac37f09f4cc3b Mon Sep 17 00:00:00 2001
From: Tomi Valkeinen <tomi.valkeinen@ti.com>
Date: Wed, 15 Jan 2014 11:45:07 +0200
Subject: [PATCH] ARM: OMAP2+: fix dpll round_rate() to actually round

omap2_dpll_round_rate() doesn't actually round the given rate, even if
the name and the description so hints. Instead it only tries to find an
exact rate match, or if that fails, return ~0 as an error.

What this basically means is that the user of the clock needs to know
what rates the dpll can support, which obviously isn't right.

This patch adds a simple method of rounding: during the iteration, the
code keeps track of the closest rate match. If no exact match is found,
the closest is returned.

However, as it is unclear whether current drivers rely on the current
behavior, the rounding functionality not enabled by default, but by
setting DPLL_USE_ROUNDED_RATE for the DPLL.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 arch/arm/mach-omap2/clkt_dpll.c | 23 ++++++++++++++++++-----
 drivers/clk/ti/dpll.c           |  3 +++
 include/linux/clk/ti.h          |  1 +
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-omap2/clkt_dpll.c b/arch/arm/mach-omap2/clkt_dpll.c
index 2649ce445845..fed7538e1eed 100644
--- a/arch/arm/mach-omap2/clkt_dpll.c
+++ b/arch/arm/mach-omap2/clkt_dpll.c
@@ -298,6 +298,8 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
 	struct dpll_data *dd;
 	unsigned long ref_rate;
 	const char *clk_name;
+	unsigned long diff, closest_diff = ~0;
+	bool use_rounding = clk->flags & DPLL_USE_ROUNDED_RATE;
 
 	if (!clk || !clk->dpll_data)
 		return ~0;
@@ -345,20 +347,31 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
 		pr_debug("clock: %s: m = %d: n = %d: new_rate = %lu\n",
 			 clk_name, m, n, new_rate);
 
-		if (target_rate == new_rate) {
+		diff = max(target_rate, new_rate) - min(target_rate, new_rate);
+
+		if ((use_rounding && diff < closest_diff) ||
+			(!use_rounding && diff == 0)) {
+			closest_diff = diff;
+
 			dd->last_rounded_m = m;
 			dd->last_rounded_n = n;
-			dd->last_rounded_rate = target_rate;
-			break;
+			dd->last_rounded_rate = new_rate;
+
+			if (diff == 0)
+				break;
 		}
 	}
 
-	if (target_rate != new_rate) {
+	if (closest_diff == ~0) {
 		pr_debug("clock: %s: cannot round to rate %lu\n",
 			 clk_name, target_rate);
 		return ~0;
 	}
 
-	return target_rate;
+	if (closest_diff > 0)
+		pr_debug("clock: %s: rounded rate %lu to %lu\n",
+			 clk_name, target_rate, dd->last_rounded_rate);
+
+	return dd->last_rounded_rate;
 }
 
diff --git a/drivers/clk/ti/dpll.c b/drivers/clk/ti/dpll.c
index 7e498a44f97d..c5858c46b58c 100644
--- a/drivers/clk/ti/dpll.c
+++ b/drivers/clk/ti/dpll.c
@@ -265,6 +265,9 @@ static void __init of_ti_dpll_setup(struct device_node *node,
 	if (dpll_mode)
 		dd->modes = dpll_mode;
 
+	if (of_property_read_bool(node, "ti,round-rate"))
+		clk_hw->flags |= DPLL_USE_ROUNDED_RATE;
+
 	ti_clk_register_dpll(&clk_hw->hw, node);
 	return;
 
diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
index 092b64168d7f..c9ed8b6b8513 100644
--- a/include/linux/clk/ti.h
+++ b/include/linux/clk/ti.h
@@ -155,6 +155,7 @@ struct clk_hw_omap {
 #define INVERT_ENABLE		(1 << 4)	/* 0 enables, 1 disables */
 #define CLOCK_CLKOUTX2		(1 << 5)
 #define MEMMAP_ADDRESSING	(1 << 6)
+#define DPLL_USE_ROUNDED_RATE	(1 << 7)	/* dpll's round_rate() returns rounded rate */
 
 /* CM_CLKEN_PLL*.EN* bit values - not all are available for every DPLL */
 #define DPLL_LOW_POWER_STOP	0x1
-- 
1.8.3.2




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH 2/2] ARM: OMAP2+: fix dpll round_rate() to actually round

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 5484 bytes --]

Hi Mike/Paul,

(sorry for top-posting)

any comments here, what do we do ? Do we split this patch ? Use v1 ? Use
v2 ?

cheers

On Wed, Mar 05, 2014 at 03:50:33PM +0200, Tomi Valkeinen wrote:
> On 20/02/14 21:30, Paul Walmsley wrote:
> > On Wed, 19 Feb 2014, Paul Walmsley wrote:
> > 
> >> On Fri, 17 Jan 2014, Tomi Valkeinen wrote:
> >>
> >>> This patch adds a simple method of rounding: during the iteration, the 
> >>> code keeps track of the closest rate match. If no exact match is found, 
> >>> the closest is returned.
> >>
> >> So that's one possible rounding policy; maybe it works fine for a display 
> >> interface PLL, at least for some values of "closest rate".  But another 
> >> might be "only allow a selection from a set of pre-determined rates 
> >> characterized by the silicon validation team".  Or another rounding 
> >> function might need to select a more distant rate that minimizes jitter, 
> >> EMI, or power consumption.  
> > 
> > Thought about this some more.  Do you only need this for the DSS PLL, or 
> > do you need it for one of the core OMAP PLLs?
> > 
> > If the former, then how about modifying your patch to create a separate 
> > round_rate function that's only used for the DSS PLL that implements the 
> > behavior that you want?
> > 
> > That would eliminate any risk of impacting other users on the system.  And 
> > would also allow this change to get into the codebase much faster, since 
> > there's no need for clk API changes, etc.
> 
> How about this one:
> 
> From f5a78303411e9192899a6a681acac37f09f4cc3b Mon Sep 17 00:00:00 2001
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Date: Wed, 15 Jan 2014 11:45:07 +0200
> Subject: [PATCH] ARM: OMAP2+: fix dpll round_rate() to actually round
> 
> omap2_dpll_round_rate() doesn't actually round the given rate, even if
> the name and the description so hints. Instead it only tries to find an
> exact rate match, or if that fails, return ~0 as an error.
> 
> What this basically means is that the user of the clock needs to know
> what rates the dpll can support, which obviously isn't right.
> 
> This patch adds a simple method of rounding: during the iteration, the
> code keeps track of the closest rate match. If no exact match is found,
> the closest is returned.
> 
> However, as it is unclear whether current drivers rely on the current
> behavior, the rounding functionality not enabled by default, but by
> setting DPLL_USE_ROUNDED_RATE for the DPLL.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  arch/arm/mach-omap2/clkt_dpll.c | 23 ++++++++++++++++++-----
>  drivers/clk/ti/dpll.c           |  3 +++
>  include/linux/clk/ti.h          |  1 +
>  3 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/clkt_dpll.c b/arch/arm/mach-omap2/clkt_dpll.c
> index 2649ce445845..fed7538e1eed 100644
> --- a/arch/arm/mach-omap2/clkt_dpll.c
> +++ b/arch/arm/mach-omap2/clkt_dpll.c
> @@ -298,6 +298,8 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
>  	struct dpll_data *dd;
>  	unsigned long ref_rate;
>  	const char *clk_name;
> +	unsigned long diff, closest_diff = ~0;
> +	bool use_rounding = clk->flags & DPLL_USE_ROUNDED_RATE;
>  
>  	if (!clk || !clk->dpll_data)
>  		return ~0;
> @@ -345,20 +347,31 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
>  		pr_debug("clock: %s: m = %d: n = %d: new_rate = %lu\n",
>  			 clk_name, m, n, new_rate);
>  
> -		if (target_rate == new_rate) {
> +		diff = max(target_rate, new_rate) - min(target_rate, new_rate);
> +
> +		if ((use_rounding && diff < closest_diff) ||
> +			(!use_rounding && diff == 0)) {
> +			closest_diff = diff;
> +
>  			dd->last_rounded_m = m;
>  			dd->last_rounded_n = n;
> -			dd->last_rounded_rate = target_rate;
> -			break;
> +			dd->last_rounded_rate = new_rate;
> +
> +			if (diff == 0)
> +				break;
>  		}
>  	}
>  
> -	if (target_rate != new_rate) {
> +	if (closest_diff == ~0) {
>  		pr_debug("clock: %s: cannot round to rate %lu\n",
>  			 clk_name, target_rate);
>  		return ~0;
>  	}
>  
> -	return target_rate;
> +	if (closest_diff > 0)
> +		pr_debug("clock: %s: rounded rate %lu to %lu\n",
> +			 clk_name, target_rate, dd->last_rounded_rate);
> +
> +	return dd->last_rounded_rate;
>  }
>  
> diff --git a/drivers/clk/ti/dpll.c b/drivers/clk/ti/dpll.c
> index 7e498a44f97d..c5858c46b58c 100644
> --- a/drivers/clk/ti/dpll.c
> +++ b/drivers/clk/ti/dpll.c
> @@ -265,6 +265,9 @@ static void __init of_ti_dpll_setup(struct device_node *node,
>  	if (dpll_mode)
>  		dd->modes = dpll_mode;
>  
> +	if (of_property_read_bool(node, "ti,round-rate"))
> +		clk_hw->flags |= DPLL_USE_ROUNDED_RATE;
> +
>  	ti_clk_register_dpll(&clk_hw->hw, node);
>  	return;
>  
> diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
> index 092b64168d7f..c9ed8b6b8513 100644
> --- a/include/linux/clk/ti.h
> +++ b/include/linux/clk/ti.h
> @@ -155,6 +155,7 @@ struct clk_hw_omap {
>  #define INVERT_ENABLE		(1 << 4)	/* 0 enables, 1 disables */
>  #define CLOCK_CLKOUTX2		(1 << 5)
>  #define MEMMAP_ADDRESSING	(1 << 6)
> +#define DPLL_USE_ROUNDED_RATE	(1 << 7)	/* dpll's round_rate() returns rounded rate */
>  
>  /* CM_CLKEN_PLL*.EN* bit values - not all are available for every DPLL */
>  #define DPLL_LOW_POWER_STOP	0x1
> -- 
> 1.8.3.2
> 
> 
> 



-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] ARM: OMAP2+: fix dpll round_rate() to actually round

[Nishanth Menon]

Tomi,


On Wed, Mar 5, 2014 at 7:50 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

[...]
>
> From f5a78303411e9192899a6a681acac37f09f4cc3b Mon Sep 17 00:00:00 2001
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Date: Wed, 15 Jan 2014 11:45:07 +0200
> Subject: [PATCH] ARM: OMAP2+: fix dpll round_rate() to actually round
>
> omap2_dpll_round_rate() doesn't actually round the given rate, even if
> the name and the description so hints. Instead it only tries to find an
> exact rate match, or if that fails, return ~0 as an error.
>
> What this basically means is that the user of the clock needs to know
> what rates the dpll can support, which obviously isn't right.
>
> This patch adds a simple method of rounding: during the iteration, the
> code keeps track of the closest rate match. If no exact match is found,
> the closest is returned.
>
> However, as it is unclear whether current drivers rely on the current
> behavior, the rounding functionality not enabled by default, but by
> setting DPLL_USE_ROUNDED_RATE for the DPLL.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  arch/arm/mach-omap2/clkt_dpll.c | 23 ++++++++++++++++++-----
>  drivers/clk/ti/dpll.c           |  3 +++
>  include/linux/clk/ti.h          |  1 +
>  3 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/clkt_dpll.c b/arch/arm/mach-omap2/clkt_dpll.c
> index 2649ce445845..fed7538e1eed 100644
> --- a/arch/arm/mach-omap2/clkt_dpll.c
> +++ b/arch/arm/mach-omap2/clkt_dpll.c
> @@ -298,6 +298,8 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
>         struct dpll_data *dd;
>         unsigned long ref_rate;
>         const char *clk_name;
> +       unsigned long diff, closest_diff = ~0;
> +       bool use_rounding = clk->flags & DPLL_USE_ROUNDED_RATE;
>
>         if (!clk || !clk->dpll_data)
>                 return ~0;
> @@ -345,20 +347,31 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
>                 pr_debug("clock: %s: m = %d: n = %d: new_rate = %lu\n",
>                          clk_name, m, n, new_rate);
>
> -               if (target_rate == new_rate) {
> +               diff = max(target_rate, new_rate) - min(target_rate, new_rate);
> +
> +               if ((use_rounding && diff < closest_diff) ||
> +                       (!use_rounding && diff == 0)) {
> +                       closest_diff = diff;
> +
>                         dd->last_rounded_m = m;
>                         dd->last_rounded_n = n;
> -                       dd->last_rounded_rate = target_rate;
> -                       break;
> +                       dd->last_rounded_rate = new_rate;
> +
> +                       if (diff == 0)
> +                               break;
>                 }
>         }
>
> -       if (target_rate != new_rate) {
> +       if (closest_diff == ~0) {
>                 pr_debug("clock: %s: cannot round to rate %lu\n",
>                          clk_name, target_rate);
>                 return ~0;
>         }
>
> -       return target_rate;
> +       if (closest_diff > 0)
> +               pr_debug("clock: %s: rounded rate %lu to %lu\n",
> +                        clk_name, target_rate, dd->last_rounded_rate);
> +
> +       return dd->last_rounded_rate;
>  }
>

> diff --git a/drivers/clk/ti/dpll.c b/drivers/clk/ti/dpll.c
> index 7e498a44f97d..c5858c46b58c 100644
> --- a/drivers/clk/ti/dpll.c
> +++ b/drivers/clk/ti/dpll.c
> @@ -265,6 +265,9 @@ static void __init of_ti_dpll_setup(struct device_node *node,
>         if (dpll_mode)
>                 dd->modes = dpll_mode;
>
> +       if (of_property_read_bool(node, "ti,round-rate"))
> +               clk_hw->flags |= DPLL_USE_ROUNDED_RATE;

binding is missing here -> Further, I'd suggest this be two patches:
a) introducing ti,round-rate (ti/dpll.c, clk/ti.h)
b) use it in clkt_dpll.c

> +
>         ti_clk_register_dpll(&clk_hw->hw, node);
>         return;
>
> diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
> index 092b64168d7f..c9ed8b6b8513 100644
> --- a/include/linux/clk/ti.h
> +++ b/include/linux/clk/ti.h
> @@ -155,6 +155,7 @@ struct clk_hw_omap {
>  #define INVERT_ENABLE          (1 << 4)        /* 0 enables, 1 disables */
>  #define CLOCK_CLKOUTX2         (1 << 5)
>  #define MEMMAP_ADDRESSING      (1 << 6)
> +#define DPLL_USE_ROUNDED_RATE  (1 << 7)        /* dpll's round_rate() returns rounded rate */
>
>  /* CM_CLKEN_PLL*.EN* bit values - not all are available for every DPLL */
>  #define DPLL_LOW_POWER_STOP    0x1
> --
> 1.8.3.2
>
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Re: [PATCH 2/2] ARM: OMAP2+: fix dpll round_rate() to actually round

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3314 bytes --]

On 19/02/14 21:49, Paul Walmsley wrote:
> On Fri, 17 Jan 2014, Tomi Valkeinen wrote:
> 
>> omap2_dpll_round_rate() doesn't actually round the given rate, even if
>> the name and the description so hints. Instead it only tries to find an
>> exact rate match, or if that fails, return ~0 as an error.
> 
> In the past, we had "rate tolerance" code, which allowed the clock code to 
> return an approximate rate within some margin.  See for example commit 
> 241d3a8dca239610d3d991bf58d4fe38c2d86fd5 ("OMAP2+: clock: remove the DPLL 
> rate tolerance code").  The rate tolerance was set at compile-time, but it 
> was set on a per-PLL basis, which in theory allowed PLLs responsible for 
> audio, etc. to have a very low rate tolerance, but PLLs that only drove 
> internal functional blocks to have a high rate tolerance.  
> 
> Part of the reason why this was removed is because some of the TI hardware 
> guys didn't want any PLL rates used that were not explicitly 
> characterized.

Ok.

>> What this basically means is that the user of the clock needs to know
>> what rates the dpll can support, which obviously isn't right.
> 
> In principle I agree with you, but I'm not sure that this rate rounding 
> function is the solution.
> 
>> This patch adds a simple method of rounding: during the iteration, the 
>> code keeps track of the closest rate match. If no exact match is found, 
>> the closest is returned.
> 
> So that's one possible rounding policy; maybe it works fine for a display 
> interface PLL, at least for some values of "closest rate".  But another 
> might be "only allow a selection from a set of pre-determined rates 
> characterized by the silicon validation team".  Or another rounding 
> function might need to select a more distant rate that minimizes jitter, 
> EMI, or power consumption.  
> 
> Seems to me that there needs to be some way for a clock user to 
> communicate its requirements along these lines to the clock framework for 
> use by the rate rounding code.  At the very least, some kind of [min, max] 
> interval seems appropriate.
> 
> Also I've often thought that it would be useful for your purposes to be 
> able to query a clock to return a list or some other parametric 
> description of the rates that it can provide.

I fully agree with all you said above.

However, I'm not trying to fix the omap clock framework here =). I just
want the displays to work properly in mainline kernel.

So, presuming this was merged, and gets display working, how would it
affect other users compared to the current state? The patched version
returns the same rate than before, if an exact match is found. Rounded
rate is only returned as a last option, instead of an error.

Do we have drivers that handle that error somehow, and then do something
(what?) to get some other rate?

If the clock path in question also has a divider component after the
pll, using clk-divider.c (which I guess is used in all/most of the
DPLLs?), things would go badly wrong if there's an error, as
clk-divider.c doesn't handle it.

So I can make no guarantees, but I have a hunch that all current users
ask for an exact clock, in which case this patch doesn't change their
behavior, except for display which it fixes.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH 2/2] ARM: OMAP2+: fix dpll round_rate() to actually round

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3580 bytes --]

On Wed, Feb 26, 2014 at 01:42:50PM +0200, Tomi Valkeinen wrote:
> On 19/02/14 21:49, Paul Walmsley wrote:
> > On Fri, 17 Jan 2014, Tomi Valkeinen wrote:
> > 
> >> omap2_dpll_round_rate() doesn't actually round the given rate, even if
> >> the name and the description so hints. Instead it only tries to find an
> >> exact rate match, or if that fails, return ~0 as an error.
> > 
> > In the past, we had "rate tolerance" code, which allowed the clock code to 
> > return an approximate rate within some margin.  See for example commit 
> > 241d3a8dca239610d3d991bf58d4fe38c2d86fd5 ("OMAP2+: clock: remove the DPLL 
> > rate tolerance code").  The rate tolerance was set at compile-time, but it 
> > was set on a per-PLL basis, which in theory allowed PLLs responsible for 
> > audio, etc. to have a very low rate tolerance, but PLLs that only drove 
> > internal functional blocks to have a high rate tolerance.  
> > 
> > Part of the reason why this was removed is because some of the TI hardware 
> > guys didn't want any PLL rates used that were not explicitly 
> > characterized.
> 
> Ok.
> 
> >> What this basically means is that the user of the clock needs to know
> >> what rates the dpll can support, which obviously isn't right.
> > 
> > In principle I agree with you, but I'm not sure that this rate rounding 
> > function is the solution.
> > 
> >> This patch adds a simple method of rounding: during the iteration, the 
> >> code keeps track of the closest rate match. If no exact match is found, 
> >> the closest is returned.
> > 
> > So that's one possible rounding policy; maybe it works fine for a display 
> > interface PLL, at least for some values of "closest rate".  But another 
> > might be "only allow a selection from a set of pre-determined rates 
> > characterized by the silicon validation team".  Or another rounding 
> > function might need to select a more distant rate that minimizes jitter, 
> > EMI, or power consumption.  
> > 
> > Seems to me that there needs to be some way for a clock user to 
> > communicate its requirements along these lines to the clock framework for 
> > use by the rate rounding code.  At the very least, some kind of [min, max] 
> > interval seems appropriate.
> > 
> > Also I've often thought that it would be useful for your purposes to be 
> > able to query a clock to return a list or some other parametric 
> > description of the rates that it can provide.
> 
> I fully agree with all you said above.
> 
> However, I'm not trying to fix the omap clock framework here =). I just
> want the displays to work properly in mainline kernel.
> 
> So, presuming this was merged, and gets display working, how would it
> affect other users compared to the current state? The patched version
> returns the same rate than before, if an exact match is found. Rounded
> rate is only returned as a last option, instead of an error.
> 
> Do we have drivers that handle that error somehow, and then do something
> (what?) to get some other rate?
> 
> If the clock path in question also has a divider component after the
> pll, using clk-divider.c (which I guess is used in all/most of the
> DPLLs?), things would go badly wrong if there's an error, as
> clk-divider.c doesn't handle it.
> 
> So I can make no guarantees, but I have a hunch that all current users
> ask for an exact clock, in which case this patch doesn't change their
> behavior, except for display which it fixes.

no further updates here ? Display is still broken on BBB :-(

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] ARM: OMAP2+: fix dpll round_rate() to actually round

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]

Hi,

On Fri, Jan 17, 2014 at 09:44:59AM +0200, Tomi Valkeinen wrote:
> omap2_dpll_round_rate() doesn't actually round the given rate, even if
> the name and the description so hints. Instead it only tries to find an
> exact rate match, or if that fails, return ~0 as an error.
> 
> What this basically means is that the user of the clock needs to know
> what rates the dpll can support, which obviously isn't right.
> 
> This patch adds a simple method of rounding: during the iteration, the
> code keeps track of the closest rate match. If no exact match is found,
> the closest is returned.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Tony, looks like this patch was lost. Are you taking it during the -rc ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] ARM: OMAP2+: fix dpll round_rate() to actually round

[Tony Lindgren]

* Felipe Balbi <balbi@ti.com> [140424 11:29]:
> Hi,
> 
> On Fri, Jan 17, 2014 at 09:44:59AM +0200, Tomi Valkeinen wrote:
> > omap2_dpll_round_rate() doesn't actually round the given rate, even if
> > the name and the description so hints. Instead it only tries to find an
> > exact rate match, or if that fails, return ~0 as an error.
> > 
> > What this basically means is that the user of the clock needs to know
> > what rates the dpll can support, which obviously isn't right.
> > 
> > This patch adds a simple method of rounding: during the iteration, the
> > code keeps track of the closest rate match. If no exact match is found,
> > the closest is returned.
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> Tony, looks like this patch was lost. Are you taking it during the -rc ?

Would like to hear what Paul thinks of the updated patch suggested
by Tomi first.

Tony

Re: [PATCH 2/2] ARM: OMAP2+: fix dpll round_rate() to actually round

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]

On Thu, Apr 24, 2014 at 11:44:58AM -0700, Tony Lindgren wrote:
> * Felipe Balbi <balbi@ti.com> [140424 11:29]:
> > Hi,
> > 
> > On Fri, Jan 17, 2014 at 09:44:59AM +0200, Tomi Valkeinen wrote:
> > > omap2_dpll_round_rate() doesn't actually round the given rate, even if
> > > the name and the description so hints. Instead it only tries to find an
> > > exact rate match, or if that fails, return ~0 as an error.
> > > 
> > > What this basically means is that the user of the clock needs to know
> > > what rates the dpll can support, which obviously isn't right.
> > > 
> > > This patch adds a simple method of rounding: during the iteration, the
> > > code keeps track of the closest rate match. If no exact match is found,
> > > the closest is returned.
> > > 
> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > 
> > Tony, looks like this patch was lost. Are you taking it during the -rc ?
> 
> Would like to hear what Paul thinks of the updated patch suggested
> by Tomi first.

Paul, any chance you can review Tomi's v2 ? It'd be very nice to get
display working on BBB.

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] ARM: OMAP2+: fix dpll round_rate() to actually round

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1947 bytes --]

On 29/04/14 18:51, Felipe Balbi wrote:
> On Thu, Apr 24, 2014 at 11:44:58AM -0700, Tony Lindgren wrote:
>> * Felipe Balbi <balbi@ti.com> [140424 11:29]:
>>> Hi,
>>>
>>> On Fri, Jan 17, 2014 at 09:44:59AM +0200, Tomi Valkeinen wrote:
>>>> omap2_dpll_round_rate() doesn't actually round the given rate, even if
>>>> the name and the description so hints. Instead it only tries to find an
>>>> exact rate match, or if that fails, return ~0 as an error.
>>>>
>>>> What this basically means is that the user of the clock needs to know
>>>> what rates the dpll can support, which obviously isn't right.
>>>>
>>>> This patch adds a simple method of rounding: during the iteration, the
>>>> code keeps track of the closest rate match. If no exact match is found,
>>>> the closest is returned.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>
>>> Tony, looks like this patch was lost. Are you taking it during the -rc ?
>>
>> Would like to hear what Paul thinks of the updated patch suggested
>> by Tomi first.
> 
> Paul, any chance you can review Tomi's v2 ? It'd be very nice to get
> display working on BBB.

Btw, I'm fine with my v2 patch, but I think the original one is fine also.

It's true that the original patch changes the dpll behavior when an
exact match is not found. However, I think our drivers always request an
exact match, and in that case the original patch doesn't change the
behavior in practice.

In theory it's possible that a driver requests a non-exact clock from
the dpll, and when it gets an error, it does something else. But, if I'm
not mistaken, all our dplls have a clk-divider after them. And as
clk-divider doesn't handle the error from dpll in any way, and instead
returns bogus rates, I presume all our drivers use exact clocks.

So v2 is perhaps slightly safer option, but I'm not sure if the added
complexity (even if quite small) is worth it.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] ARM: OMAP2+: fix dpll round_rate() to actually round

[Paul Walmsley]

Hi,

On Tue, 29 Apr 2014, Tomi Valkeinen wrote:

> On 29/04/14 18:51, Felipe Balbi wrote:
> > On Thu, Apr 24, 2014 at 11:44:58AM -0700, Tony Lindgren wrote:
> >> * Felipe Balbi <balbi@ti.com> [140424 11:29]:
> >>> Hi,
> >>>
> >>> On Fri, Jan 17, 2014 at 09:44:59AM +0200, Tomi Valkeinen wrote:
> >>>> omap2_dpll_round_rate() doesn't actually round the given rate, even if
> >>>> the name and the description so hints. Instead it only tries to find an
> >>>> exact rate match, or if that fails, return ~0 as an error.
> >>>>
> >>>> What this basically means is that the user of the clock needs to know
> >>>> what rates the dpll can support, which obviously isn't right.
> >>>>
> >>>> This patch adds a simple method of rounding: during the iteration, the
> >>>> code keeps track of the closest rate match. If no exact match is found,
> >>>> the closest is returned.
> >>>>
> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >>>
> >>> Tony, looks like this patch was lost. Are you taking it during the -rc ?
> >>
> >> Would like to hear what Paul thinks of the updated patch suggested
> >> by Tomi first.
> > 
> > Paul, any chance you can review Tomi's v2 ? It'd be very nice to get
> > display working on BBB.
> 
> Btw, I'm fine with my v2 patch, but I think the original one is fine also.
> 
> It's true that the original patch changes the dpll behavior when an
> exact match is not found. However, I think our drivers always request an
> exact match, and in that case the original patch doesn't change the
> behavior in practice.
> 
> In theory it's possible that a driver requests a non-exact clock from
> the dpll, and when it gets an error, it does something else.

The path that worries me at the moment is the set-rate path.  That calls 
__clk_round_rate() (if the user hasn't called it already) and silently 
tries to set the clock to the altered rate.

So I think I'd prefer the v2 patches - since at least then, we can control 
the scope of the altered behavior.  Tomi, care to address Nishanth's 
comments on your v2 patches from his April 30th mail?

> But, if I'm not mistaken, all our dplls have a clk-divider after them. 
> And as clk-divider doesn't handle the error from dpll in any way, and 
> instead returns bogus rates, I presume all our drivers use exact clocks.

Sigh, sounds like another bug to fix...


- Paul

Re: [PATCH 2/2] ARM: OMAP2+: fix dpll round_rate() to actually round

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1139 bytes --]

On 08/05/14 01:16, Paul Walmsley wrote:

>> It's true that the original patch changes the dpll behavior when an
>> exact match is not found. However, I think our drivers always request an
>> exact match, and in that case the original patch doesn't change the
>> behavior in practice.
>>
>> In theory it's possible that a driver requests a non-exact clock from
>> the dpll, and when it gets an error, it does something else.
> 
> The path that worries me at the moment is the set-rate path.  That calls 
> __clk_round_rate() (if the user hasn't called it already) and silently 
> tries to set the clock to the altered rate.

Hmm, so you mean a driver could call set_rate, and presume it only uses
exact rates the dpll can produce, and presumes that set_rate returns an
error if the dpll cannot produce the requested rate?

Isn't that what I said? If a driver has such behavior, I think it still
doesn't work, as (correct me if I'm wrong) we always have the
clk-divider after a dpll. And the clk-divider doesn't handle the error,
so neither can the driver.

Or what kind of scenario do you have in mind?

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]