| Message ID | 20160914130231.3035-8-grygorii.strashko@ti.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Sep 14, 2016 at 04:02:29PM +0300, Grygorii Strashko wrote: > @@ -35,6 +33,8 @@ Optional properties: > For example in dra72x-evm, pcf gpio has to be > driven low so that cpsw slave 0 and phy data > lines are connected via mux. > +- cpts_clock_mult : Numerator to convert input clock ticks into nanoseconds > +- cpts_clock_shift : Denominator to convert input clock ticks into nanoseconds You should explain to the reader how these will be calculated when the properties are missing. > diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c > index ff8bb85..8046a21 100644 > --- a/drivers/net/ethernet/ti/cpts.c > +++ b/drivers/net/ethernet/ti/cpts.c > @@ -418,18 +418,60 @@ void cpts_unregister(struct cpts *cpts) > clk_disable(cpts->refclk); > } > > +static void cpts_calc_mult_shift(struct cpts *cpts) > +{ > + u64 maxsec; > + u32 freq; > + u32 mult; > + u32 shift; > + u64 ns; > + u64 frac; Why so many new lines? This isn't good style. Please combine variables of the same type into one line and sort the lists alphabetically. > + if (cpts->cc_mult || cpts->cc.shift) > + return; > + > + freq = clk_get_rate(cpts->refclk); > + > + /* Calc the maximum number of seconds which we can run before > + * wrapping around. > + */ > + maxsec = cpts->cc.mask; > + do_div(maxsec, freq); > + if (!maxsec) > + maxsec = 1; > + else if (maxsec > 600 && cpts->cc.mask > UINT_MAX) > + maxsec = 600; > + > + clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec); > + > + cpts->cc_mult = mult; > + cpts->cc.mult = mult; > + cpts->cc.shift = shift; > + /* Check calculations and inform if not precise */ Contrary to this comment, you are not making any kind of judgment about whether the calculations are precise or not. > + frac = 0; > + ns = cyclecounter_cyc2ns(&cpts->cc, freq, cpts->cc.mask, &frac); > + > + dev_info(cpts->dev, > + "CPTS: ref_clk_freq:%u calc_mult:%u calc_shift:%u error:%lld nsec/sec\n", > + freq, cpts->cc_mult, cpts->cc.shift, (ns - NSEC_PER_SEC)); > +} > + Thanks, Richard
On 09/14/2016 05:22 PM, Richard Cochran wrote: > On Wed, Sep 14, 2016 at 04:02:29PM +0300, Grygorii Strashko wrote: >> @@ -35,6 +33,8 @@ Optional properties: >> For example in dra72x-evm, pcf gpio has to be >> driven low so that cpsw slave 0 and phy data >> lines are connected via mux. >> +- cpts_clock_mult : Numerator to convert input clock ticks into nanoseconds >> +- cpts_clock_shift : Denominator to convert input clock ticks into nanoseconds > > You should explain to the reader how these will be calculated when the > properties are missing. Not sure how full should it be explained in bindings - I'll try. > >> diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c >> index ff8bb85..8046a21 100644 >> --- a/drivers/net/ethernet/ti/cpts.c >> +++ b/drivers/net/ethernet/ti/cpts.c >> @@ -418,18 +418,60 @@ void cpts_unregister(struct cpts *cpts) >> clk_disable(cpts->refclk); >> } >> >> +static void cpts_calc_mult_shift(struct cpts *cpts) >> +{ >> + u64 maxsec; >> + u32 freq; >> + u32 mult; >> + u32 shift; >> + u64 ns; >> + u64 frac; > > Why so many new lines? This isn't good style. Please combine > variables of the same type into one line and sort the lists > alphabetically. Its matter of preferences :), but sure - I'll update. > >> + if (cpts->cc_mult || cpts->cc.shift) >> + return; >> + >> + freq = clk_get_rate(cpts->refclk); >> + >> + /* Calc the maximum number of seconds which we can run before >> + * wrapping around. >> + */ >> + maxsec = cpts->cc.mask; >> + do_div(maxsec, freq); >> + if (!maxsec) >> + maxsec = 1; >> + else if (maxsec > 600 && cpts->cc.mask > UINT_MAX) >> + maxsec = 600; >> + >> + clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec); >> + >> + cpts->cc_mult = mult; >> + cpts->cc.mult = mult; >> + cpts->cc.shift = shift; >> + /* Check calculations and inform if not precise */ > > Contrary to this comment, you are not making any kind of judgment > about whether the calculations are precise or not. > >> + frac = 0; >> + ns = cyclecounter_cyc2ns(&cpts->cc, freq, cpts->cc.mask, &frac); >> + >> + dev_info(cpts->dev, >> + "CPTS: ref_clk_freq:%u calc_mult:%u calc_shift:%u error:%lld nsec/sec\n", >> + freq, cpts->cc_mult, cpts->cc.shift, (ns - NSEC_PER_SEC)); >> +} >> + > Thanks for the review.
On Wed, Sep 14, 2016 at 04:02:29PM +0300, Grygorii Strashko wrote: > +static void cpts_calc_mult_shift(struct cpts *cpts) > +{ > + u64 maxsec; > + u32 freq; > + u32 mult; > + u32 shift; > + u64 ns; > + u64 frac; > + > + if (cpts->cc_mult || cpts->cc.shift) > + return; > + > + freq = clk_get_rate(cpts->refclk); > + > + /* Calc the maximum number of seconds which we can run before > + * wrapping around. > + */ > + maxsec = cpts->cc.mask; > + do_div(maxsec, freq); > + if (!maxsec) > + maxsec = 1; This doesn't pass the smell test. If the max counter value is M, you are figuring M*1/F which is the time in seconds corresponding to M. We set M=2^32-1, and so 'freq' would have to be greater than 4 GHz in order for 'maxsec' to be zero. Do you really expect such high frequency input clocks? > + else if (maxsec > 600 && cpts->cc.mask > UINT_MAX) > + maxsec = 600; What is this all about? cc.mask is always 2^32 - 1. > + clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec); > + > + cpts->cc_mult = mult; > + cpts->cc.mult = mult; In order to get good resolution on the frequency adjustment, we want to keep 'mult' as large as possible. I don't see your code doing this. We can rely on the watchdog reader (work queue) to prevent overflows. Thanks, Richard > + cpts->cc.shift = shift; > + /* Check calculations and inform if not precise */ > + frac = 0; > + ns = cyclecounter_cyc2ns(&cpts->cc, freq, cpts->cc.mask, &frac); > + > + dev_info(cpts->dev, > + "CPTS: ref_clk_freq:%u calc_mult:%u calc_shift:%u error:%lld nsec/sec\n", > + freq, cpts->cc_mult, cpts->cc.shift, (ns - NSEC_PER_SEC)); > +}
On 09/14/2016 11:26 PM, Richard Cochran wrote: > On Wed, Sep 14, 2016 at 04:02:29PM +0300, Grygorii Strashko wrote: >> +static void cpts_calc_mult_shift(struct cpts *cpts) >> +{ >> + u64 maxsec; >> + u32 freq; >> + u32 mult; >> + u32 shift; >> + u64 ns; >> + u64 frac; >> + >> + if (cpts->cc_mult || cpts->cc.shift) >> + return; >> + >> + freq = clk_get_rate(cpts->refclk); >> + >> + /* Calc the maximum number of seconds which we can run before >> + * wrapping around. >> + */ >> + maxsec = cpts->cc.mask; >> + do_div(maxsec, freq); >> + if (!maxsec) >> + maxsec = 1; > > This doesn't pass the smell test. If the max counter value is M, you > are figuring M*1/F which is the time in seconds corresponding to M. > We set M=2^32-1, and so 'freq' would have to be greater than 4 GHz in > order for 'maxsec' to be zero. Do you really expect such high > frequency input clocks? no. can drop it. > >> + else if (maxsec > 600 && cpts->cc.mask > UINT_MAX) >> + maxsec = 600; > > What is this all about? cc.mask is always 2^32 - 1. Oh. Not sure if we will update CPTS to support this, but on KS2 E/L (66AK2E) CPTS counter can work in 64bit mode. > >> + clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec); >> + >> + cpts->cc_mult = mult; >> + cpts->cc.mult = mult; > > In order to get good resolution on the frequency adjustment, we want > to keep 'mult' as large as possible. I don't see your code doing > this. We can rely on the watchdog reader (work queue) to prevent > overflows. As I understand (and tested), clocks_calc_mult_shift() will return max possible mult which can be used without overflow. if calculated results do not satisfy end user - the custom values can be passed in DT.
On Wed, Sep 14, 2016 at 11:47:46PM +0300, Grygorii Strashko wrote: > As I understand (and tested), clocks_calc_mult_shift() will > return max possible mult which can be used without overflow. Yes, BUT the returned values depends on the @maxsec input. As the kerneldec says, * Larger ranges may reduce the conversion accuracy by chosing smaller * mult and shift factors. In addition to that, frequency tuning by calculating a percentage of 'mult', and if 'mult' is small, then the tuning resolution is poor. So we don't want @maxsec as large as possible, but as small as possible. > if calculated results do not satisfy end user - the custom values can > be passed in DT. If we calculate automatically, then the result had better well be optimal or nearly so. Otherwise, we should leave it as a manual input via DTS, IMHO, so that someone is forced to check the values. Thanks, Richard
On 09/15/2016 12:03 AM, Richard Cochran wrote: > On Wed, Sep 14, 2016 at 11:47:46PM +0300, Grygorii Strashko wrote: >> As I understand (and tested), clocks_calc_mult_shift() will >> return max possible mult which can be used without overflow. > > Yes, BUT the returned values depends on the @maxsec input. As the > kerneldec says, > > * Larger ranges may reduce the conversion accuracy by chosing smaller > * mult and shift factors. > > In addition to that, frequency tuning by calculating a percentage of > 'mult', and if 'mult' is small, then the tuning resolution is poor. > > So we don't want @maxsec as large as possible, but as small as > possible. > Ok. So, will it work if maxsec will be ~= (maxsec / 2) + 1? + 1 to cover potential delays of overflow check work execution. [...]
On Wed, Sep 14, 2016 at 10:26:19PM +0200, Richard Cochran wrote: > On Wed, Sep 14, 2016 at 04:02:29PM +0300, Grygorii Strashko wrote: > > + clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec); > > + > > + cpts->cc_mult = mult; > > + cpts->cc.mult = mult; > > In order to get good resolution on the frequency adjustment, we want > to keep 'mult' as large as possible. I don't see your code doing > this. We can rely on the watchdog reader (work queue) to prevent > overflows. I took a closer look, and assuming cc.mask = 2^32 - 1, then using clocks_calc_mult_shift() produces good results for a reasonable range of input frequencies. Keeping 'maxsec' constant at 4 we have: | Freq. MHz | mult | shift | |-----------+------------+-------| | 100 | 0xa0000000 | 28 | | 250 | 0x80000000 | 29 | | 500 | 0x80000000 | 30 | | 1000 | 0x80000000 | 31 | Can the input clock be higher than 1 GHz? If not, I suggest using clocks_calc_mult_shift() with maxsec=4 and a setting the watchdog also to 4*HZ. Thanks, Richard
On Thu, Sep 15, 2016 at 01:58:15PM +0200, Richard Cochran wrote: > Can the input clock be higher than 1 GHz? If not, I suggest using > clocks_calc_mult_shift() with maxsec=4 and a setting the watchdog also > to 4*HZ. On second thought, with the new 12% timer batching, using 4*HZ for 32 bits of 1 GHz is cutting it too close. So just keep it like you had it, with maxsec=mask/freq and timeout=maxsec/2, to stay on the safe side. Thanks, Richard
diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt index 5ad439f..88f81c7 100644 --- a/Documentation/devicetree/bindings/net/cpsw.txt +++ b/Documentation/devicetree/bindings/net/cpsw.txt @@ -20,8 +20,6 @@ Required properties: - slaves : Specifies number for slaves - active_slave : Specifies the slave to use for time stamping, ethtool and SIOCGMIIPHY -- cpts_clock_mult : Numerator to convert input clock ticks into nanoseconds -- cpts_clock_shift : Denominator to convert input clock ticks into nanoseconds Optional properties: - ti,hwmods : Must be "cpgmac0" @@ -35,6 +33,8 @@ Optional properties: For example in dra72x-evm, pcf gpio has to be driven low so that cpsw slave 0 and phy data lines are connected via mux. +- cpts_clock_mult : Numerator to convert input clock ticks into nanoseconds +- cpts_clock_shift : Denominator to convert input clock ticks into nanoseconds Slave Properties: diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c index ff8bb85..8046a21 100644 --- a/drivers/net/ethernet/ti/cpts.c +++ b/drivers/net/ethernet/ti/cpts.c @@ -418,18 +418,60 @@ void cpts_unregister(struct cpts *cpts) clk_disable(cpts->refclk); } +static void cpts_calc_mult_shift(struct cpts *cpts) +{ + u64 maxsec; + u32 freq; + u32 mult; + u32 shift; + u64 ns; + u64 frac; + + if (cpts->cc_mult || cpts->cc.shift) + return; + + freq = clk_get_rate(cpts->refclk); + + /* Calc the maximum number of seconds which we can run before + * wrapping around. + */ + maxsec = cpts->cc.mask; + do_div(maxsec, freq); + if (!maxsec) + maxsec = 1; + else if (maxsec > 600 && cpts->cc.mask > UINT_MAX) + maxsec = 600; + + clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec); + + cpts->cc_mult = mult; + cpts->cc.mult = mult; + cpts->cc.shift = shift; + /* Check calculations and inform if not precise */ + frac = 0; + ns = cyclecounter_cyc2ns(&cpts->cc, freq, cpts->cc.mask, &frac); + + dev_info(cpts->dev, + "CPTS: ref_clk_freq:%u calc_mult:%u calc_shift:%u error:%lld nsec/sec\n", + freq, cpts->cc_mult, cpts->cc.shift, (ns - NSEC_PER_SEC)); +} + static int cpts_of_parse(struct cpts *cpts, struct device_node *node) { int ret = -EINVAL; u32 prop; - if (of_property_read_u32(node, "cpts_clock_mult", &prop)) - goto of_error; - cpts->cc_mult = prop; + cpts->cc_mult = 0; + if (!of_property_read_u32(node, "cpts_clock_mult", &prop)) + cpts->cc_mult = prop; - if (of_property_read_u32(node, "cpts_clock_shift", &prop)) - goto of_error; - cpts->cc.shift = prop; + cpts->cc.shift = 0; + if (!of_property_read_u32(node, "cpts_clock_shift", &prop)) + cpts->cc.shift = prop; + + if ((cpts->cc_mult && !cpts->cc.shift) || + (!cpts->cc_mult && cpts->cc.shift)) + goto of_error; return 0; @@ -471,6 +513,8 @@ struct cpts *cpts_create(struct device *dev, void __iomem *regs, cpts->cc.read = cpts_systim_read; cpts->cc.mask = CLOCKSOURCE_MASK(32); + cpts_calc_mult_shift(cpts); + return cpts; }
The cyclecounter mult and shift values can be calculated based on the CPTS rftclk frequency and timekeepnig framework provides required algos and API's. Hence, calc mult and shift basing on CPTS rftclk frequency if both cpts_clock_shift and cpts_clock_mult properties are not provided in DT (the basis of calculation algorithm is borrowed from __clocksource_update_freq_scale()). After this change cpts_clock_shift and cpts_clock_mult DT properties will become optional. Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> --- Documentation/devicetree/bindings/net/cpsw.txt | 4 +- drivers/net/ethernet/ti/cpts.c | 56 +++++++++++++++++++++++--- 2 files changed, 52 insertions(+), 8 deletions(-)