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

[boris brezillon <b.brezillon@overkiz.com>]

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.

>
> 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.

> 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