diff mbox

[7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq

Message ID 20160914130231.3035-8-grygorii.strashko@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Grygorii Strashko Sept. 14, 2016, 1:02 p.m. UTC
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(-)

Comments

Richard Cochran Sept. 14, 2016, 2:22 p.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grygorii Strashko Sept. 14, 2016, 7:59 p.m. UTC | #2
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.
Richard Cochran Sept. 14, 2016, 8:26 p.m. UTC | #3
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));
> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grygorii Strashko Sept. 14, 2016, 8:47 p.m. UTC | #4
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.
Richard Cochran Sept. 14, 2016, 9:03 p.m. UTC | #5
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grygorii Strashko Sept. 14, 2016, 9:14 p.m. UTC | #6
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.

[...]
Richard Cochran Sept. 15, 2016, 11:58 a.m. UTC | #7
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

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Cochran Sept. 15, 2016, 1:49 p.m. UTC | #8
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 }