Re: [PATCH v2 2/2] clk: add accuracy support for fixed clock

Mike Turquette
2013-11-27T18:10:53Z

Quoting boris brezillon (2013-11-27 09:19:08)
> Hi Jason,
>
> On 27/11/2013 15:56, Jason Cooper wrote:
> > Boris,
> >
> > Thanks for posting this series. Bear with me as I'm attempting to give
> > MikeT a hand.
> Nice to hear.
> Mike already took a look at this series, but I'm happy to get more
> feedbacks.
>
> > Don't be afraid to tell me a question is stupid :-)
> Your questions are far from stupid ;-).
>
> >
> > On Wed, Nov 27, 2013 at 01:44:45PM +0100, Boris BREZILLON wrote:
> >> This patch adds support for accuracy retrieval on fixed clocks.
> >> It also adds a new dt property called 'clock-accuracy' to define the clock
> >> accuracy.
> >>
> >> This can be usefull for oscillator (RC, crystal, ...) definitions which are
> >> always given an accuracy characteristic.
> > I think we need to be more explicit in the binding and the API,
> > especially when providing a method to recalculate the accuracy. I
> > presume this recalculated value would be relative against the root
> > clock?
>
> Yes, indirectly.
> Actually the clk accuracy depends on the whole clock chain, and is
> calculated
> either by comparing the real clk rate to the theorical clk rate
> (accuracy = absolute_value((theorical_clk_rate - real_clk_rate)) /
> theorical_clk_rate),
> or by adding all the accuracies (expressed in ppm, ppb or ppt) of the
> clk chain
> (accuracy = current_clk_accuracy + parent_clk_accuracy).
>
> Say you have a root clk with a +-10000 ppb accuracy, then a pll multiplying
> this root clk by 40 and introducing a possible drift of +- 1000 ppb and
> eventually a system clk based on this pll with a perfect div by 2 prescaler
> (accuracy = 0 ppb).
>
> If I understand correctly how accuracy propagates accross the clk tree,
> you'll end up with a system clk with a +- 11000 ppb accuracy.
>
> e.g.:
> root clk = 12MHz +- 10000 ppb => 12 MHz * (1 - (10000 / 10^9)) < root
> clk < 12 MHz * (1 + (10000 / 10^9))
> => 11,99988 MHz <
> root clk < 12,00012 MHz
> pll clk = ((root clk) * 40) +- 1000 ppb => (11,99988 MHz * 40) * (1 -
> (1000 / 10^9)) < pll clk < (12,00012 MHz * 40) * (1 + (1000 / 10^9))
> =>
> 479,994720005 MHz < pll clk < 480,005280005 MHz
>
> system clk = ((pll clk) / 2) +- XXX ppb => 479,994720005 MHz / 2 <
> system clk < 480,005280005 MHz / 2
> =>
> 239,997360002 MHz < system clk < 240,002640002 MHz
> => system
> clk accuracy = 0,002640002 / 240 = 11000 ppb
>
> Please tell me if my assumptions are false.
> >
> > There really needs to be two attributes here: the rated accuracy from
> > the manufacturer, and the calculated accuracy wrt another clock in the
> > system. We only need a binding for the manufacturer rating since the
> > calculated accuracy is determined at runtime.
>
> Actually when I proposed this new functionnality I only had the theorical
> (or manufacturer rated) accuracy in mind.
> But providing an estimated accuracy (based on another clk) sounds
> interresting if your reference clk is an extremly accurate one.

Is there a need to model clock accuracy across the clock chain? I'm OK
modeling it in DT, and the code to do it in the clk framework isn't very
much ... but I also wonder if we're just adding complexity for no
reason.

>
> >
> > I would also prefer to see an unknown accuracy be -1.
> I decided to keep 0 as a default value for unimplemented recalc_accuracy
> (or unspecified fixed accuracy) to keep existing implementation coherent.
>
> 0 means the clk is perfect, and I thought it would be easier to handle a
> perfect clk (even if this is not really the case) than handling an error
> case.
>
> Another aspect is the propagation of the clk accuracy accross the clk tree.
> Returning -1 in the middle of the clk chain will drop the previous clk
> accuracy
> calculation.
>
> Anyway, I can change this if you think this is more appropriate.

What about the absence of the property? Instead of requiring a -1 value
can we simply detect that the property does not exist? This is nicer for
backwards compatibility with existing DTS.

Regards,
Mike

>
> > There are already
> > clocks on the market (PPS reference clocks) with accuracies of
> > 0.1ppb/day [1]. Obviously, these aren't system clocks. So the limit on
> > accuracy may be a non-issue. However, it may be worth changing the
> > binding property to express the units.
> Wow, 0.1 ppb, this is impressive :-).
>
>
> This needs more than changing the dt bindings: I currently store the
> accuracy value in an unsigned long field, and expressing this in ppt
> (parts per trillion) may implies storing this in an u64 field (or store a
> unit field).
>
>
> >
> >> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> >> ---
> >> .../devicetree/bindings/clock/fixed-clock.txt | 3 ++
> >> drivers/clk/clk-fixed-rate.c | 43 +++++++++++++++++---
> >> include/linux/clk-provider.h | 4 ++
> >> 3 files changed, 44 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/fixed-clock.txt b/Documentation/devicetree/bindings/clock/fixed-clock.txt
> >> index 0b1fe78..48ea0ad 100644
> >> --- a/Documentation/devicetree/bindings/clock/fixed-clock.txt
> >> +++ b/Documentation/devicetree/bindings/clock/fixed-clock.txt
> >> @@ -10,6 +10,8 @@ Required properties:
> >> - clock-frequency : frequency of clock in Hz. Should be a single cell.
> >>
> >> Optional properties:
> >> +- clock-accuracy : accuracy of clock in ppb (parts per billion).
> >> + Should be a single cell.
> > I would prefer to call this property 'clock-rated-ppb'.
>
> Depending on what we choose to do with the accuracy field, this might be
> an option.
>
> >
> >> - gpios : From common gpio binding; gpio connection to clock enable pin.
> >> - clock-output-names : From common clock binding.
> >>
> >> @@ -18,4 +20,5 @@ Example:
> >> compatible = "fixed-clock";
> >> #clock-cells = <0>;
> >> clock-frequency = <1000000000>;
> >> + clock-accuracy = <100>;
> >> };
> > thx,
> >
> > Jason.
> >
> > [1] http://www.vectron.com/products/modules/md-010.htm
>
> Thanks for your review, and don't hesitate to ask more questions, or to
> point out
> incoherencies in my approach (I'm not an expert in clk and clk accuracy
> calculation,
> and I might be wrong ;-)).
>
> Best Regards,
>
> Boris