Re: [PATCH 1/4] clk: Introduce 'clk_find_nearest_rate()'

[Boris BREZILLON <boris.brezillon@free-electrons.com>]

On Tue, 1 Jul 2014 08:32:14 +0200
Boris BREZILLON <boris.brezillon@free-electrons.com> wrote:

> Hi Sören,
> 
> On Mon, 30 Jun 2014 17:12:23 -0700
> Sören Brinkmann <soren.brinkmann@xilinx.com> wrote:
> 
> > Hi Boris,
> > 
> > On Mon, 2014-06-30 at 09:27PM +0200, Boris BREZILLON wrote:
> > > Hello Soren,
> > > 
> > > On Mon, 30 Jun 2014 09:56:33 -0700
> > > Soren Brinkmann <soren.brinkmann@xilinx.com> wrote:
> > > 
> > > > Introduce a new API function to find the rate a clock can
> > > > provide which is closest to a given rate.
> > > > 
> > > > clk_round_rate() leaves it to the clock driver how rounding is
> > > > done. Commonly implementations round down due to use-cases that
> > > > have a certain frequency maximum that must not be exceeded.
> > > 
> > > I had the same concern (how could a driver decide whether it
> > > should round up, down or to the closest value), but had a slightly
> > > different approach in mind.
> > > 
> > > AFAIU, you're assuming the clock is always a power of two (which
> > > is true most of the time, but some clock implementation might
> > > differ, i.e. a PLL accept any kind of multiplier not necessarily
> > > a power of 2).
> > 
> > No, the idea is to always work. There should not be any such
> > limitation. Where do you see that?
> 
> My bad, I should have read the code more carefully.
> BTW, it could help readers if you add some comments to explain how you
> are finding the nearest rate.
> 
> My main concern with this approach is that you can spend some time
> iterating to find the nearest rate where a clk driver would find it
> quite quickly (because it knows exactly how the clk works and what are
> the possible clk muxing and clk modifiers options).
> 
> > 
> > > 
> > > How about improving the clk_ops interface instead by adding a new
> > > method
> > > 
> > > 	long (*round_rate_with_constraint)(struct clk_hw *hw,
> > > 					   unsigned long
> > > requested_rate, unsigned long *parent_rate,
> > > 					   enum clk_round_type
> > > type);
> > > 
> > > with
> > > 
> > > enum clk_round_type {
> > > 	CLK_ROUND_DOWN,
> > > 	CLK_ROUND_UP,
> > > 	CLK_ROUND_CLOSEST,
> > > };
> > 
> > I thought about that, but the find_nearest() did already exist more
> > or less and in the end it is not much of a difference, IMHO. If it
> > turns out that the others are needed as well and somebody
> > implements it, they could be added as another convenience function
> > like I did, and/or it could be wrapped in the function you propose
> > here.
> >
> > > 
> > > or just adding these 3 methods:
> > > 
> > > 	long (*round_rate_up)(struct clk_hw *hw,
> > > 			      unsigned long requested_rate,
> > > 			      unsigned long *parent_rate);
> > > 
> > > 	long (*round_rate_down)(struct clk_hw *hw,
> > > 				unsigned long requested_rate,
> > > 				unsigned long *parent_rate);
> > > 
> > > 	long (*round_rate_closest)(struct clk_hw *hw,
> > > 				   unsigned long requested_rate,
> > > 				   unsigned long *parent_rate);
> > 
> > That would be quite a change for clock drivers. So far, there are
> > not many restrictions on round_rate. I think there has already been
> > a discussion in this direction that went nowhere.
> > https://lkml.org/lkml/2010/7/14/260
> > 
> 
> Not if we keep these (or this, depending on the solution you chose)
> functions optional, and return -ENOTSUP, if they're not implemented.

Or even better: fall back to your implementation.

> 
> > > 
> > > and let the round_rate method implement the default behaviour.
> > 
> > There is no real default. Rounding is not specified for the current
> > API.
> 
> What I meant by default behavior is the behavior already implemented
> by the clock driver (either round up, down or closest).
> This way you do not introduce regressions with existing users, and can
> use new methods in new use cases.
> 
> > 
> > > 
> > > This way you could add 3 functions to the API:
> > > 
> > > clk_round_closest (or clk_find_nearest_rate as you called it),
> > > clk_round_up and clk_round_down, and let the clk driver
> > > implementation return the appropriate rate.
> > 
> > I'd say the current patch set does kind of align with that, doesn't
> > it? We can add the find_nearest_rate() (there was a discussion on
> > the name, ane we decided against round_closest in favor of the
> > current proposal) now and the other functions could be added later
> > if people find them to be useful. And if they all get added we can
> > think about consolidating them if it made sense.
> 
> Yes sure.
> 
> Best Regards,
> 
> Boris
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com