Re: [PATCH v2 1/2] clk: change clk_ops' ->round_rate() prototype

[Michael Turquette <mturquette@linaro.org>]

Quoting Jon Hunter (2015-06-05 01:46:09)
> =

> On 05/06/15 00:02, Paul Walmsley wrote:
> > Hi folks
> > =

> > just a brief comment on this one:
> > =

> > On Thu, 30 Apr 2015, Boris Brezillon wrote:
> > =

> >> Clock rates are stored in an unsigned long field, but ->round_rate()
> >> (which returns a rounded rate from a requested one) returns a long
> >> value (errors are reported using negative error codes), which can lead
> >> to long overflow if the clock rate exceed 2Ghz.
> >>
> >> Change ->round_rate() prototype to return 0 or an error code, and pass=
 the
> >> requested rate as a pointer so that it can be adjusted depending on
> >> hardware capabilities.
> > =

> > ...
> > =

> >> diff --git a/Documentation/clk.txt b/Documentation/clk.txt
> >> index 0e4f90a..fca8b7a 100644
> >> --- a/Documentation/clk.txt
> >> +++ b/Documentation/clk.txt
> >> @@ -68,8 +68,8 @@ the operations defined in clk.h:
> >>              int             (*is_enabled)(struct clk_hw *hw);
> >>              unsigned long   (*recalc_rate)(struct clk_hw *hw,
> >>                                              unsigned long parent_rate=
);
> >> -            long            (*round_rate)(struct clk_hw *hw,
> >> -                                            unsigned long rate,
> >> +            int             (*round_rate)(struct clk_hw *hw,
> >> +                                            unsigned long *rate,
> >>                                              unsigned long *parent_rat=
e);
> >>              long            (*determine_rate)(struct clk_hw *hw,
> >>                                              unsigned long rate,
> > =

> > I'd suggest that we should probably go straight to 64-bit rates.  There =

> > are already plenty of clock sources that can generate rates higher than =

> > 4GiHz.

Paul,

I agree. Changing the underlying struct clk.rate to 64 bits is on my
radar.

> =

> An alternative would be to introduce to a frequency "base" the default
> could be Hz (for backwards compatibility), but for CPUs we probably only
> care about MHz (or may be kHz) and so 32-bits would still suffice. Even
> if CPUs cared about Hz they could still use Hz, but in that case they
> probably don't care about GHz. Obviously, we don't want to break DT
> compatibility but may be the frequency base could be defined in DT and
> if it is missing then Hz is assumed. Just a thought ...

I was thinking of doing it the other way. E.g. use 64-bit rates
throughout the clk framework and have a base unit of millihertz instead
of hertz. 64 bits gives us a LOT of room to grow, and the current base
of hertz completely ignores all applications with fractional hertz
requirements.

Your point came up a while ago in the context of cpufreq and the clk
framework agreeing on the type for a rate[0]. The consensus was to
continue using KHz as the base type for that subsystem.

[0] https://lkml.org/lkml/2014/5/26/582
(There is more to the above conversation that the mail archives didn't
seem to catch :-/ )

Regards,
Mike

> =

> Jon