Re: [PATCH RFC v9 01/20] clk: divider: Correct parent clk round rate if no bestdiv is normally found

[Sascha Hauer]

On Mon, Feb 16, 2015 at 01:18:13PM +0200, Tomi Valkeinen wrote:
> On 13/02/15 20:57, Sascha Hauer wrote:
> > On Fri, Feb 13, 2015 at 04:35:36PM +0200, Tomi Valkeinen wrote:
> >> On 12/02/15 15:41, Sascha Hauer wrote:
> >>
> >>> Tomis patch is based on the assumption that clk_set_rate(clk_round_rate(rate))
> >>> is equal to clk_round_rate(rate). So when this assumption is wrong then
> >>> it should simply be reverted.
> >>
> >> When is it not equal?
> >>
> >> I agree that doing clk_set_rate(clk, clk_round_rate(clk, rate)) is
> >> pointless, but shouldn't it still work?
> >>
> >> And we can forget about clk_round_rate. Without my patch, this would
> >> behave oddly also:
> >>
> >> rate = clk_get_rate(clk);
> >> clk_set_rate(clk, rate);
> >>
> >> The end result could be something else than 'rate'.
> > 
> > I agree that it's a bit odd, but I think it has to be like this.
> > Consider that you request a rate of 100Hz, but the clock can only
> > produce 99.5Hz, so due to rounding clk_round_rate() returns 99Hz.
> > Now when you request 99Hz from clk_set_rate() the 99.5Hz value
> > can't be used because it's too high.
> 
> Would that problem better be fixed by changing the clock driver so that
> when asked for 99Hz, it would look for rates less than 100Hz?
> 
> I think the old behavior was so odd that I would call it broken, so I
> hope the current problems can be fixed via some other ways than breaking
> it again.

I gave it a try, but so far I have no idea how to implement the divider
correctly and bullet proof.

What I have so far is a test which creates some cascaded dividers and sets
rates on them. The test iterates over the frequency range and a) calls
clk_round_rate with the iterator, b) sets the clk to the iterator, c)
sets the clk to the rounded rate.

Be prepared for surprises and try to fix the results... I failed for
now and wonder if the approach to the divider is wrong.

Sascha