[PATCH/RFC 3/3] ARM: mach-shmobile: implement parent clock optimization

[PATCH/RFC 3/3] ARM: mach-shmobile: implement parent clock optimization

[Guennadi Liakhovetski]

On ap4evb PLLC2 is used as a parent clock for the HDMI clock. To configure the
HDMI clock with the highest precision this patch scans all available PLLC2
frequencies and picks up the best one.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 arch/arm/mach-shmobile/board-ap4evb.c |   60 +++++++++++++++++++++++++++++++++
 1 files changed, 60 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-shmobile/board-ap4evb.c b/arch/arm/mach-shmobile/board-ap4evb.c
index 7d0a8c4..8676e26 100644
--- a/arch/arm/mach-shmobile/board-ap4evb.c
+++ b/arch/arm/mach-shmobile/board-ap4evb.c
@@ -637,9 +637,69 @@ static struct platform_device lcdc1_device = {
 	},
 };
 
+#define MAX_DIVISOR 63
+
+static long ap4evb_clk_optimize(unsigned long target, unsigned long *best_freq,
+				unsigned long *parent_freq)
+{
+	struct cpufreq_frequency_table *freq, *best = NULL;
+	unsigned long error = ULONG_MAX, freq_high, freq_low, div;
+
+	for (freq = pllc2_clk.freq_table; freq->frequency != CPUFREQ_TABLE_END;
+	     freq++) {
+		if (freq->frequency < target) {
+			if (error > target - freq->frequency) {
+				error = target - freq->frequency;
+				best = freq;
+				if (best_freq)
+					*best_freq = freq->frequency;
+			}
+			continue;
+		}
+		div = freq->frequency / target;
+		if (div > MAX_DIVISOR)
+			div = MAX_DIVISOR;
+		freq_high = freq->frequency / div;
+		freq_low = freq->frequency / (div + 1);
+		if (freq_high - target < error) {
+			error = freq_high - target;
+			best = freq;
+			if (best_freq)
+				*best_freq = freq_high;
+		}
+		if (target - freq_low < error) {
+			error = target - freq_low;
+			best = freq;
+			if (best_freq)
+				*best_freq = freq_low;
+		}
+		pr_debug("%u / %lu = %lu, / %lu = %lu, best %lu, parent %u\n",
+			 freq->frequency, div, freq_high, div + 1, freq_low,
+			 *best_freq, best->frequency);
+		if (!error)
+			break;
+	}
+	if (parent_freq)
+		*parent_freq = best->frequency;
+	return error;
+}
+
+static int ap4evb_clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	return clk_set_rate(clk->parent, rate);
+}
+
+static unsigned long ap4evb_clk_get_rate(struct clk *clk)
+{
+	return clk_get_rate(clk->parent);
+}
+
 static struct sh_mobile_hdmi_info hdmi_info = {
 	.lcd_chan = &sh_mobile_lcdc1_info.ch[0],
 	.lcd_dev = &lcdc1_device.dev,
+	.clk_optimize_parent = ap4evb_clk_optimize,
+	.clk_set_rate_parent = ap4evb_clk_set_rate,
+	.clk_get_rate_parent = ap4evb_clk_get_rate,
 };
 
 static struct resource hdmi_resources[] = {
-- 
1.7.1

Re: [PATCH/RFC 3/3] ARM: mach-shmobile: implement parent clock optimization for HDMI

[Paul Mundt]

On Wed, Oct 27, 2010 at 06:24:26PM +0200, Guennadi Liakhovetski wrote:
> On ap4evb PLLC2 is used as a parent clock for the HDMI clock. To configure the
> HDMI clock with the highest precision this patch scans all available PLLC2
> frequencies and picks up the best one.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

So, what exactly does ap4evb_clk_optimize() do that you can't already do
with either clk_rate_table_round() or clk_rate_div_range_round()?

I really don't want to see this sort of rate table access going on
outside of the clock framework itself, so if we need to add or extend an
existing helper for a new usecase then that is highly preferable to
constantly reinventing things.

Re: [PATCH/RFC 3/3] ARM: mach-shmobile: implement parent clock

[Guennadi Liakhovetski]

On Thu, 28 Oct 2010, Paul Mundt wrote:

> On Wed, Oct 27, 2010 at 06:24:26PM +0200, Guennadi Liakhovetski wrote:
> > On ap4evb PLLC2 is used as a parent clock for the HDMI clock. To configure the
> > HDMI clock with the highest precision this patch scans all available PLLC2
> > frequencies and picks up the best one.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> So, what exactly does ap4evb_clk_optimize() do that you can't already do
> with either clk_rate_table_round() or clk_rate_div_range_round()?

No, it does something pretty different: it selects the best _parent_ 
frequency to optimize that of the _child_. So, it actually tells you "to 
configure your clock as close as possible to frequency X you have to 
reconfigure its parent with frequency Y." Which I don't think any of the 
aforementioned functions does.

> I really don't want to see this sort of rate table access going on
> outside of the clock framework itself, so if we need to add or extend an
> existing helper for a new usecase then that is highly preferable to
> constantly reinventing things.

Well, there is a reason, why this is marked an "RFC":-) I also don't like 
very much touching clock internals in platform code, but in a way it is 
specific for each case. This implementation only works for clocks, whose 
divisors are integer numbers in a certain range (without holes). Ok, with 
clk_get_parent() the other two callbacks .clk_set_rate_parent() and 
.clk_get_rate_parent() are automatically not needed, which is good! And I 
would be happy to try to convert the ap4evb_clk_optimize() function into a 
generic helper too, if we reach such an agreement.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH/RFC 3/3] ARM: mach-shmobile: implement parent clock optimization for HDMI

[Paul Mundt]

On Thu, Oct 28, 2010 at 08:29:54AM +0200, Guennadi Liakhovetski wrote:
> On Thu, 28 Oct 2010, Paul Mundt wrote:
> 
> > On Wed, Oct 27, 2010 at 06:24:26PM +0200, Guennadi Liakhovetski wrote:
> > > On ap4evb PLLC2 is used as a parent clock for the HDMI clock. To configure the
> > > HDMI clock with the highest precision this patch scans all available PLLC2
> > > frequencies and picks up the best one.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > 
> > So, what exactly does ap4evb_clk_optimize() do that you can't already do
> > with either clk_rate_table_round() or clk_rate_div_range_round()?
> 
> No, it does something pretty different: it selects the best _parent_ 
> frequency to optimize that of the _child_. So, it actually tells you "to 
> configure your clock as close as possible to frequency X you have to 
> reconfigure its parent with frequency Y." Which I don't think any of the 
> aforementioned functions does.
> 
Ok, that sounds fine.

> Well, there is a reason, why this is marked an "RFC":-) I also don't like 
> very much touching clock internals in platform code, but in a way it is 
> specific for each case. This implementation only works for clocks, whose 
> divisors are integer numbers in a certain range (without holes). Ok, with 
> clk_get_parent() the other two callbacks .clk_set_rate_parent() and 
> .clk_get_rate_parent() are automatically not needed, which is good! And I 
> would be happy to try to convert the ap4evb_clk_optimize() function into a 
> generic helper too, if we reach such an agreement.
> 
I wonder if this is something you can use/adapt clk_rate_round_helper()
for? In any event, the best place for this is in drivers/sh/clk/core.c
along with the other helpers. The ranged div iterator already takes the
parent frequency in to account, so you might be able to use a similar
approach for an iterator here, too (albeit probably inverted). If you
introduce a generic clk_rate_parent_round() or whatever then that's
pretty easy to support without any of the nasty layering violations.

I don't like having this sort of knowledge in the platform code because
there is really nothing platform specific about it. If a platform happens
to be the first to exhibit a certain type of clock topology interactivity,
then that's something the clock framework needs to expand to accomodate.
Most of these things will invariably show up on other platforms in the
future, and by then the same algorithm will have been copied all over the
place, mangled, and probably broken in a dozen different hard to debug
ways. Keeping all of the rounders centrally managed will help prevent
this, regardless of what sort of obscure topology perversion the hardware
folks come up with :-)