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@xxxxxxxxxxx> > >> --- > >> .../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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
