Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
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
Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
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(, , 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 | 0xa000 |28 | | 250 | 0x8000 |29 | | 500 | 0x8000 |30 | | 1000 | 0x8000 |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
Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
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. [...] -- regards, -grygorii
Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
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
Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
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(, , 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.
--
regards,
-grygorii
Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
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(, , 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(>cc, freq, cpts->cc.mask, );
> +
> + 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));
> +}
Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
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(, , 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(>cc, freq, cpts->cc.mask, );
+
+ 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.
--
regards,
-grygorii
Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
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(, , 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(>cc, freq, cpts->cc.mask, );
> +
> + 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
