Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1483264 > unrolled thread

[PATCH 0/9] net: ethernet: ti: cpts: update and fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/9] net: ethernet: ti: cpts: update and fixes Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-14 15:10 +0200
    [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-14 15:10 +0200
      Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period Richard Cochran <richardcochran@gmail.com> - 2016-09-14 16:30 +0200
        Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period Richard Cochran <richardcochran@gmail.com> - 2016-09-14 22:10 +0200
          Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-14 22:30 +0200
            Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period Richard Cochran <richardcochran@gmail.com> - 2016-09-14 22:50 +0200
              Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-14 23:00 +0200
        Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-14 22:10 +0200
    [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-14 15:10 +0200
      Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from  refclk freq Richard Cochran <richardcochran@gmail.com> - 2016-09-14 16:30 +0200
        Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from  refclk freq Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-14 22:10 +0200
      Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from  refclk freq Richard Cochran <richardcochran@gmail.com> - 2016-09-14 22:30 +0200
        Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from  refclk freq Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-14 23:00 +0200
          Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from  refclk freq Richard Cochran <richardcochran@gmail.com> - 2016-09-14 23:10 +0200
            Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from  refclk freq Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-14 23:20 +0200
        Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from  refclk freq Richard Cochran <richardcochran@gmail.com> - 2016-09-15 14:00 +0200
          Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from  refclk freq Richard Cochran <richardcochran@gmail.com> - 2016-09-15 16:00 +0200
    [PATCH 1/9] net: ethernet: ti: exclude cpts from build when disabled Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-14 15:10 +0200
    [PATCH 3/9] net: ethernet: ti: cpts: rework initialization/deinitialization Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-14 15:10 +0200
      Re: [PATCH 3/9] net: ethernet: ti: cpts: rework  initialization/deinitialization Richard Cochran <richardcochran@gmail.com> - 2016-09-14 16:00 +0200
        Re: [PATCH 3/9] net: ethernet: ti: cpts: rework  initialization/deinitialization Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-14 22:20 +0200
          Re: [PATCH 3/9] net: ethernet: ti: cpts: rework  initialization/deinitialization Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-14 22:40 +0200
            Re: [PATCH 3/9] net: ethernet: ti: cpts: rework  initialization/deinitialization Richard Cochran <richardcochran@gmail.com> - 2016-09-14 23:00 +0200
              Re: [PATCH 3/9] net: ethernet: ti: cpts: rework  initialization/deinitialization Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-14 23:00 +0200
          Re: [PATCH 3/9] net: ethernet: ti: cpts: rework  initialization/deinitialization Richard Cochran <richardcochran@gmail.com> - 2016-09-14 22:40 +0200
      Re: [PATCH 3/9] net: ethernet: ti: cpts: rework  initialization/deinitialization Richard Cochran <richardcochran@gmail.com> - 2016-09-15 10:20 +0200
    [PATCH 5/9] net: ethernet: ti: cpts: add return value to tx and rx timestamp funcitons Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-14 15:10 +0200
      Re: [PATCH 5/9] net: ethernet: ti: cpts: add return value to tx and  rx timestamp funcitons Richard Cochran <richardcochran@gmail.com> - 2016-09-14 16:10 +0200
    [PATCH 6/9] net: ethernet: ti: cpts: clean up event list if event pool is empty Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-14 15:10 +0200
      Re: [PATCH 6/9] net: ethernet: ti: cpts: clean up event list if  event pool is empty Richard Cochran <richardcochran@gmail.com> - 2016-09-14 16:20 +0200
        Re: [PATCH 6/9] net: ethernet: ti: cpts: clean up event list if event  pool is empty Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-14 22:00 +0200

Page 1 of 2  [1] 2  Next page →

#1483264 — [PATCH 0/9] net: ethernet: ti: cpts: update and fixes

Hi,

It is preparation serie intended to clean up and optimize TI CPTS driver to
facilitate further integration with other TI's SoCs like Keystone 2.
It also include some non critical fixes:
 net: ethernet: ti: exclude cpts from build when disabled
 net: ethernet: ti: cpts: fix overflow check period
 net: ethernet: ti: cpts: clean up event list if event pool is empty


Grygorii Strashko (7):
  net: ethernet: ti: exclude cpts from build when disabled
  net: ethernet: ti: cpsw: minimize direct access to struct cpts
  net: ethernet: ti: cpts: rework initialization/deinitialization
  net: ethernet: ti: cpts: move dt props parsing to cpts driver
  net: ethernet: ti: cpts: calc mult and shift from refclk freq
  net: ethernet: ti: cpts: fix overflow check period
  net: ethernet: ti: cpts: switch to readl/writel_relaxed()

WingMan Kwok (2):
  net: ethernet: ti: cpts: add return value to tx and rx timestamp
    funcitons
  net: ethernet: ti: cpts: clean up event list if event pool is empty

 Documentation/devicetree/bindings/net/cpsw.txt |   4 +-
 drivers/net/ethernet/ti/Makefile               |   3 +-
 drivers/net/ethernet/ti/cpsw.c                 |  83 ++++----
 drivers/net/ethernet/ti/cpsw.h                 |   2 -
 drivers/net/ethernet/ti/cpts.c                 | 256 ++++++++++++++++++-------
 drivers/net/ethernet/ti/cpts.h                 |  93 +++++++--
 6 files changed, 319 insertions(+), 122 deletions(-)

-- 
2.9.3

[toc] | [next] | [standalone]

#1483265 — [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period

The CPTS drivers uses 8sec period for overflow checking with
assumption that CPTS rftclk will not exceed 500MHz. But that's not
true on some TI's platforms (Kesytone 2). As result, it is possible that
CPTS counter will overflow more than once between two readings.

Hence, fix it by selecting overflow check period dynamically as
max_sec_before_overflow/2, where
 max_sec_before_overflow = max_counter_val / rftclk_freq.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/ethernet/ti/cpts.c | 16 +++++++++++-----
 drivers/net/ethernet/ti/cpts.h |  4 +---
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index 8046a21..cbe0974 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -251,7 +251,7 @@ static void cpts_overflow_check(struct work_struct *work)
 	cpts_write32(cpts, TS_PEND_EN, int_enable);
 	cpts_ptp_gettime(&cpts->info, &ts);
 	pr_debug("cpts overflow check at %lld.%09lu\n", ts.tv_sec, ts.tv_nsec);
-	schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
+	schedule_delayed_work(&cpts->overflow_work, cpts->ov_check_period);
 }
 
 static int cpts_match(struct sk_buff *skb, unsigned int ptp_class,
@@ -391,7 +391,7 @@ int cpts_register(struct cpts *cpts)
 	}
 	cpts->phc_index = ptp_clock_index(cpts->clock);
 
-	schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
+	schedule_delayed_work(&cpts->overflow_work, cpts->ov_check_period);
 	return 0;
 
 err_ptp:
@@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
 	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
@@ -442,11 +439,20 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
 	else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)
 		maxsec = 600;
 
+	/* Calc overflow check period (maxsec / 2) */
+	cpts->ov_check_period = (HZ * maxsec) / 2;
+	dev_info(cpts->dev, "cpts: overflow check period %lu\n",
+		 cpts->ov_check_period);
+
+	if (cpts->cc_mult || cpts->cc.shift)
+		return;
+
 	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);
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index 47026ec..e0e4a62b 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -97,9 +97,6 @@ enum {
 	CPTS_EV_TX,   /* Ethernet Transmit Event */
 };
 
-/* This covers any input clock up to about 500 MHz. */
-#define CPTS_OVERFLOW_PERIOD (HZ * 8)
-
 #define CPTS_FIFO_DEPTH 16
 #define CPTS_MAX_EVENTS 32
 
@@ -127,6 +124,7 @@ struct cpts {
 	struct list_head events;
 	struct list_head pool;
 	struct cpts_event pool_data[CPTS_MAX_EVENTS];
+	unsigned long ov_check_period;
 };
 
 int cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1483377 — Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period

On Wed, Sep 14, 2016 at 04:02:30PM +0300, Grygorii Strashko wrote:
> @@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
>  	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

This hunk has nothing to do with $subject.

> @@ -442,11 +439,20 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
>  	else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)
>  		maxsec = 600;
>  
> +	/* Calc overflow check period (maxsec / 2) */
> +	cpts->ov_check_period = (HZ * maxsec) / 2;
> +	dev_info(cpts->dev, "cpts: overflow check period %lu\n",
> +		 cpts->ov_check_period);
> +
> +	if (cpts->cc_mult || cpts->cc.shift)
> +		return;
> +
>  	clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);
>  
>  	cpts->cc_mult = mult;
>  	cpts->cc.mult = mult;
>  	cpts->cc.shift = shift;
> +

Nor does this.

Thanks,
Richard


>  	/* Check calculations and inform if not precise */
>  	frac = 0;
>  	ns = cyclecounter_cyc2ns(&cpts->cc, freq, cpts->cc.mask, &frac);
> diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
> index 47026ec..e0e4a62b 100644
> --- a/drivers/net/ethernet/ti/cpts.h
> +++ b/drivers/net/ethernet/ti/cpts.h
> @@ -97,9 +97,6 @@ enum {
>  	CPTS_EV_TX,   /* Ethernet Transmit Event */
>  };
>  
> -/* This covers any input clock up to about 500 MHz. */
> -#define CPTS_OVERFLOW_PERIOD (HZ * 8)
> -
>  #define CPTS_FIFO_DEPTH 16
>  #define CPTS_MAX_EVENTS 32
>  
> @@ -127,6 +124,7 @@ struct cpts {
>  	struct list_head events;
>  	struct list_head pool;
>  	struct cpts_event pool_data[CPTS_MAX_EVENTS];
> +	unsigned long ov_check_period;
>  };
>  
>  int cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
> -- 
> 2.9.3
> 

[toc] | [prev] | [next] | [standalone]

#1483606 — Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period

On Wed, Sep 14, 2016 at 11:03:18PM +0300, Grygorii Strashko wrote:
> On 09/14/2016 05:25 PM, Richard Cochran wrote:
> > On Wed, Sep 14, 2016 at 04:02:30PM +0300, Grygorii Strashko wrote:
> >> @@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
> >>  	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
> > 
> > This hunk has nothing to do with $subject.
> 
> Sry, but I did not get your comment here :(
> I'd happy to update patch according to your request, but could you provide more info here, pls?

You added that code in patch #7.  Then you moved it in patch #8.  You
could have made the code correct in patch #7 to begin with.

> >>  	cpts->cc_mult = mult;
> >>  	cpts->cc.mult = mult;
> >>  	cpts->cc.shift = shift;
> >> +

If you want a blank line here, then put into the original patch #7.

Thanks,
Richard

[toc] | [prev] | [next] | [standalone]

#1483639 — Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period

On 09/14/2016 11:08 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 11:03:18PM +0300, Grygorii Strashko wrote:
>> On 09/14/2016 05:25 PM, Richard Cochran wrote:
>>> On Wed, Sep 14, 2016 at 04:02:30PM +0300, Grygorii Strashko wrote:
>>>> @@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
>>>>  	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
>>>
>>> This hunk has nothing to do with $subject.
>>
>> Sry, but I did not get your comment here :(
>> I'd happy to update patch according to your request, but could you provide more info here, pls?
> 
> You added that code in patch #7.  Then you moved it in patch #8.  You
> could have made the code correct in patch #7 to begin with.
> 

Do you mean 
 -	if (cpts->cc_mult || cpts->cc.shift)
 -		return;
??

if yes then those changes are correct as from patch#7 point of 
view, as from patch#8 because they are separate standalone changes.
In patch patch#7 it reasonable to ball out earlier, while in patch#8
it required to move forward a bit as I need to know maxsec. 

Sry, that I'm bothering you with stupid questions.


-- 
regards,
-grygorii

[toc] | [prev] | [next] | [standalone]

#1483653 — Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period

On Wed, Sep 14, 2016 at 11:23:43PM +0300, Grygorii Strashko wrote:
> if yes then those changes are correct as from patch#7 point of 
> view, as from patch#8 because they are separate standalone changes.
> In patch patch#7 it reasonable to ball out earlier, while in patch#8
> it required to move forward a bit as I need to know maxsec. 

And what about the extra blank line?  AFAICT, placing the test later
in patch #7 is correct logic and has the advantage of not distracting
reviews with pointless churn!

Thanks,
Richard

[toc] | [prev] | [next] | [standalone]

#1483655 — Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period

On 09/14/2016 11:43 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 11:23:43PM +0300, Grygorii Strashko wrote:
>> if yes then those changes are correct as from patch#7 point of
>> view, as from patch#8 because they are separate standalone changes.
>> In patch patch#7 it reasonable to ball out earlier, while in patch#8
>> it required to move forward a bit as I need to know maxsec.
>
> And what about the extra blank line?  AFAICT, placing the test later
> in patch #7 is correct logic and has the advantage of not distracting
> reviews with pointless churn!
>

NP. I'll change it.

-- 
regards,
-grygorii

[toc] | [prev] | [next] | [standalone]

#1483608 — Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period

On 09/14/2016 05:25 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 04:02:30PM +0300, Grygorii Strashko wrote:
>> @@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
>>  	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
> 
> This hunk has nothing to do with $subject.

Sry, but I did not get your comment here :(
I'd happy to update patch according to your request, but could you provide more info here, pls?



> 
>> @@ -442,11 +439,20 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
>>  	else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)
>>  		maxsec = 600;
>>  
>> +	/* Calc overflow check period (maxsec / 2) */
>> +	cpts->ov_check_period = (HZ * maxsec) / 2;
>> +	dev_info(cpts->dev, "cpts: overflow check period %lu\n",
>> +		 cpts->ov_check_period);
>> +
>> +	if (cpts->cc_mult || cpts->cc.shift)
>> +		return;
>> +
>>  	clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);
>>  
>>  	cpts->cc_mult = mult;
>>  	cpts->cc.mult = mult;
>>  	cpts->cc.shift = shift;
>> +
> 
> Nor does this.



-- 
regards,
-grygorii

[toc] | [prev] | [next] | [standalone]

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

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(-)

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

[toc] | [prev] | [next] | [standalone]

#1483381 — 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(&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

[toc] | [prev] | [next] | [standalone]

#1483611 — 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(&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.

-- 
regards,
-grygorii

[toc] | [prev] | [next] | [standalone]

#1483640 — 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(&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));
> +}

[toc] | [prev] | [next] | [standalone]

#1483660 — 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(&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

[toc] | [prev] | [next] | [standalone]

#1483674 — 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

[toc] | [prev] | [next] | [standalone]

#1483681 — 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

[toc] | [prev] | [next] | [standalone]

#1484041 — 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(&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

[toc] | [prev] | [next] | [standalone]

#1484202 — 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

[toc] | [prev] | [next] | [standalone]

#1483267 — [PATCH 1/9] net: ethernet: ti: exclude cpts from build when disabled

TI CPTS feature declared as optional, but cpts.c module is
included in build always.
Exclude  cpts.c from build when CPTS is disabled in Kconfig and
optimize usage of CONFIG_TI_CPTS.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/ethernet/ti/Makefile |  3 ++-
 drivers/net/ethernet/ti/cpsw.c   | 21 ++++++++++++++++-----
 drivers/net/ethernet/ti/cpts.c   |  8 --------
 drivers/net/ethernet/ti/cpts.h   | 14 ++++++++++++--
 4 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/ti/Makefile b/drivers/net/ethernet/ti/Makefile
index d420d94..1e7c10b 100644
--- a/drivers/net/ethernet/ti/Makefile
+++ b/drivers/net/ethernet/ti/Makefile
@@ -12,8 +12,9 @@ obj-$(CONFIG_TI_DAVINCI_MDIO) += davinci_mdio.o
 obj-$(CONFIG_TI_DAVINCI_CPDMA) += davinci_cpdma.o
 obj-$(CONFIG_TI_CPSW_PHY_SEL) += cpsw-phy-sel.o
 obj-$(CONFIG_TI_CPSW_ALE) += cpsw_ale.o
+obj-$(CONFIG_TI_CPTS) += cpts.o
 obj-$(CONFIG_TI_CPSW) += ti_cpsw.o
-ti_cpsw-y := cpsw.o cpts.o
+ti_cpsw-y := cpsw.o
 
 obj-$(CONFIG_TI_KEYSTONE_NETCP) += keystone_netcp.o
 keystone_netcp-y := netcp_core.o
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index c6cff3d..b743bb1d 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1514,7 +1514,6 @@ fail:
 }
 
 #ifdef CONFIG_TI_CPTS
-
 static void cpsw_hwtstamp_v1(struct cpsw_common *cpsw)
 {
 	struct cpsw_slave *slave = &cpsw->slaves[cpsw->data.active_slave];
@@ -1661,7 +1660,16 @@ static int cpsw_hwtstamp_get(struct net_device *dev, struct ifreq *ifr)
 
 	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
 }
+#else
+static int cpsw_hwtstamp_get(struct net_device *dev, struct ifreq *ifr)
+{
+	return -EOPNOTSUPP;
+}
 
+static int cpsw_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
+{
+	return -EOPNOTSUPP;
+}
 #endif /*CONFIG_TI_CPTS*/
 
 static int cpsw_ndo_ioctl(struct net_device *dev, struct ifreq *req, int cmd)
@@ -1674,12 +1682,10 @@ static int cpsw_ndo_ioctl(struct net_device *dev, struct ifreq *req, int cmd)
 		return -EINVAL;
 
 	switch (cmd) {
-#ifdef CONFIG_TI_CPTS
 	case SIOCSHWTSTAMP:
 		return cpsw_hwtstamp_set(dev, req);
 	case SIOCGHWTSTAMP:
 		return cpsw_hwtstamp_get(dev, req);
-#endif
 	}
 
 	if (!cpsw->slaves[slave_no].phy)
@@ -1935,10 +1941,10 @@ static void cpsw_set_msglevel(struct net_device *ndev, u32 value)
 	priv->msg_enable = value;
 }
 
+#ifdef CONFIG_TI_CPTS
 static int cpsw_get_ts_info(struct net_device *ndev,
 			    struct ethtool_ts_info *info)
 {
-#ifdef CONFIG_TI_CPTS
 	struct cpsw_common *cpsw = ndev_to_cpsw(ndev);
 
 	info->so_timestamping =
@@ -1955,7 +1961,12 @@ static int cpsw_get_ts_info(struct net_device *ndev,
 	info->rx_filters =
 		(1 << HWTSTAMP_FILTER_NONE) |
 		(1 << HWTSTAMP_FILTER_PTP_V2_EVENT);
+	return 0;
+}
 #else
+static int cpsw_get_ts_info(struct net_device *ndev,
+			    struct ethtool_ts_info *info)
+{
 	info->so_timestamping =
 		SOF_TIMESTAMPING_TX_SOFTWARE |
 		SOF_TIMESTAMPING_RX_SOFTWARE |
@@ -1963,9 +1974,9 @@ static int cpsw_get_ts_info(struct net_device *ndev,
 	info->phc_index = -1;
 	info->tx_types = 0;
 	info->rx_filters = 0;
-#endif
 	return 0;
 }
+#endif
 
 static int cpsw_get_settings(struct net_device *ndev,
 			     struct ethtool_cmd *ecmd)
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index 85a55b4..aaab08e 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -31,8 +31,6 @@
 
 #include "cpts.h"
 
-#ifdef CONFIG_TI_CPTS
-
 #define cpts_read32(c, r)	__raw_readl(&c->reg->r)
 #define cpts_write32(c, v, r)	__raw_writel(v, &c->reg->r)
 
@@ -350,12 +348,9 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
 	skb_tstamp_tx(skb, &ssh);
 }
 
-#endif /*CONFIG_TI_CPTS*/
-
 int cpts_register(struct device *dev, struct cpts *cpts,
 		  u32 mult, u32 shift)
 {
-#ifdef CONFIG_TI_CPTS
 	int err, i;
 	unsigned long flags;
 
@@ -391,18 +386,15 @@ int cpts_register(struct device *dev, struct cpts *cpts,
 	schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
 
 	cpts->phc_index = ptp_clock_index(cpts->clock);
-#endif
 	return 0;
 }
 
 void cpts_unregister(struct cpts *cpts)
 {
-#ifdef CONFIG_TI_CPTS
 	if (cpts->clock) {
 		ptp_clock_unregister(cpts->clock);
 		cancel_delayed_work_sync(&cpts->overflow_work);
 	}
 	if (cpts->refclk)
 		cpts_clk_release(cpts);
-#endif
 }
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index 69a46b9..a68780d 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -130,6 +130,8 @@ struct cpts {
 #ifdef CONFIG_TI_CPTS
 void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
 void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb);
+int cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift);
+void cpts_unregister(struct cpts *cpts);
 #else
 static inline void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
 {
@@ -137,9 +139,17 @@ static inline void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
 static inline void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
 {
 }
+
+static inline int
+cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift)
+{
+	return 0;
+}
+
+static inline void cpts_unregister(struct cpts *cpts)
+{
+}
 #endif
 
-int cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift);
-void cpts_unregister(struct cpts *cpts);
 
 #endif
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1483268 — [PATCH 3/9] net: ethernet: ti: cpts: rework initialization/deinitialization

The current implementation CPTS initialization and deinitialization
(represented by cpts_register/unregister()) is pretty entangled and
has some issues, like:
- ptp clock registered before spinlock, which is protecting it, and
before timecounter and cyclecounter initialization;
- CPTS ref_clk requested using devm API while cpts_register() is
called from .ndo_open(), as result additional checks required;
- CPTS ref_clk is prepared, but never unprepared;
- CPTS is not disabled even when unregistered..

Hence, make things simpler and fix above issues by adding
cpts_create()/cpts_release() which should be called from
.probe()/.remove() respectively and move all static initialization
there. Clean up and update cpts_register/unregister() so PTP clock is
registered the last and unregistered first. In addition, this change
allows to clean up cpts.h for the case when CPTS is disabled.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/ethernet/ti/cpsw.c |  24 ++++----
 drivers/net/ethernet/ti/cpts.c | 125 ++++++++++++++++++++++++++---------------
 drivers/net/ethernet/ti/cpts.h |  26 +++++++--
 3 files changed, 113 insertions(+), 62 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 9b900f0..dfd5707 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1406,9 +1406,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
 		if (ret < 0)
 			goto err_cleanup;
 
-		if (cpts_register(cpsw->dev, cpsw->cpts,
-				  cpsw->data.cpts_clock_mult,
-				  cpsw->data.cpts_clock_shift))
+		if (cpts_register(cpsw->cpts))
 			dev_err(priv->dev, "error registering cpts device\n");
 
 	}
@@ -2551,6 +2549,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	struct cpdma_params		dma_params;
 	struct cpsw_ale_params		ale_params;
 	void __iomem			*ss_regs;
+	void __iomem			*cpts_regs;
 	struct resource			*res, *ss_res;
 	const struct of_device_id	*of_id;
 	struct gpio_descs		*mode;
@@ -2575,12 +2574,6 @@ static int cpsw_probe(struct platform_device *pdev)
 	priv->dev  = &ndev->dev;
 	priv->msg_enable = netif_msg_init(debug_level, CPSW_DEBUG);
 	cpsw->rx_packet_max = max(rx_packet_max, 128);
-	cpsw->cpts = devm_kzalloc(&pdev->dev, sizeof(struct cpts), GFP_KERNEL);
-	if (!cpsw->cpts) {
-		dev_err(&pdev->dev, "error allocating cpts\n");
-		ret = -ENOMEM;
-		goto clean_ndev_ret;
-	}
 
 	mode = devm_gpiod_get_array_optional(&pdev->dev, "mode", GPIOD_OUT_LOW);
 	if (IS_ERR(mode)) {
@@ -2669,7 +2662,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	switch (cpsw->version) {
 	case CPSW_VERSION_1:
 		cpsw->host_port_regs = ss_regs + CPSW1_HOST_PORT_OFFSET;
-		cpsw->cpts->reg      = ss_regs + CPSW1_CPTS_OFFSET;
+		cpts_regs		= ss_regs + CPSW1_CPTS_OFFSET;
 		cpsw->hw_stats	     = ss_regs + CPSW1_HW_STATS;
 		dma_params.dmaregs   = ss_regs + CPSW1_CPDMA_OFFSET;
 		dma_params.txhdp     = ss_regs + CPSW1_STATERAM_OFFSET;
@@ -2683,7 +2676,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	case CPSW_VERSION_3:
 	case CPSW_VERSION_4:
 		cpsw->host_port_regs = ss_regs + CPSW2_HOST_PORT_OFFSET;
-		cpsw->cpts->reg      = ss_regs + CPSW2_CPTS_OFFSET;
+		cpts_regs		= ss_regs + CPSW2_CPTS_OFFSET;
 		cpsw->hw_stats	     = ss_regs + CPSW2_HW_STATS;
 		dma_params.dmaregs   = ss_regs + CPSW2_CPDMA_OFFSET;
 		dma_params.txhdp     = ss_regs + CPSW2_STATERAM_OFFSET;
@@ -2749,6 +2742,14 @@ static int cpsw_probe(struct platform_device *pdev)
 		goto clean_dma_ret;
 	}
 
+	cpsw->cpts = cpts_create(cpsw->dev, cpts_regs,
+				 cpsw->data.cpts_clock_mult,
+				 cpsw->data.cpts_clock_shift);
+	if (IS_ERR(cpsw->cpts)) {
+		ret = PTR_ERR(cpsw->cpts);
+		goto clean_ale_ret;
+	}
+
 	ndev->irq = platform_get_irq(pdev, 1);
 	if (ndev->irq < 0) {
 		dev_err(priv->dev, "error getting irq resource\n");
@@ -2857,6 +2858,7 @@ static int cpsw_remove(struct platform_device *pdev)
 		unregister_netdev(cpsw->slaves[1].ndev);
 	unregister_netdev(ndev);
 
+	cpts_release(cpsw->cpts);
 	cpsw_ale_destroy(cpsw->ale);
 	cpdma_ctlr_destroy(cpsw->dma);
 	of_platform_depopulate(&pdev->dev);
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index aaab08e..a46478e 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -228,22 +228,6 @@ static void cpts_overflow_check(struct work_struct *work)
 	schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
 }
 
-static void cpts_clk_init(struct device *dev, struct cpts *cpts)
-{
-	cpts->refclk = devm_clk_get(dev, "cpts");
-	if (IS_ERR(cpts->refclk)) {
-		dev_err(dev, "Failed to get cpts refclk\n");
-		cpts->refclk = NULL;
-		return;
-	}
-	clk_prepare_enable(cpts->refclk);
-}
-
-static void cpts_clk_release(struct cpts *cpts)
-{
-	clk_disable(cpts->refclk);
-}
-
 static int cpts_match(struct sk_buff *skb, unsigned int ptp_class,
 		      u16 ts_seqid, u8 ts_msgtype)
 {
@@ -323,7 +307,7 @@ void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
 	u64 ns;
 	struct skb_shared_hwtstamps *ssh;
 
-	if (!cpts->rx_enable)
+	if (!cpts || !cpts->rx_enable)
 		return;
 	ns = cpts_find_ts(cpts, skb, CPTS_EV_RX);
 	if (!ns)
@@ -338,7 +322,7 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
 	u64 ns;
 	struct skb_shared_hwtstamps ssh;
 
-	if (!(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
+	if (!cpts || !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
 		return;
 	ns = cpts_find_ts(cpts, skb, CPTS_EV_TX);
 	if (!ns)
@@ -348,53 +332,102 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
 	skb_tstamp_tx(skb, &ssh);
 }
 
-int cpts_register(struct device *dev, struct cpts *cpts,
-		  u32 mult, u32 shift)
+int cpts_register(struct cpts *cpts)
 {
 	int err, i;
-	unsigned long flags;
 
-	cpts->info = cpts_info;
-	cpts->clock = ptp_clock_register(&cpts->info, dev);
-	if (IS_ERR(cpts->clock)) {
-		err = PTR_ERR(cpts->clock);
-		cpts->clock = NULL;
-		return err;
-	}
-	spin_lock_init(&cpts->lock);
-
-	cpts->cc.read = cpts_systim_read;
-	cpts->cc.mask = CLOCKSOURCE_MASK(32);
-	cpts->cc_mult = mult;
-	cpts->cc.mult = mult;
-	cpts->cc.shift = shift;
+	if (!cpts)
+		return -EINVAL;
 
 	INIT_LIST_HEAD(&cpts->events);
 	INIT_LIST_HEAD(&cpts->pool);
 	for (i = 0; i < CPTS_MAX_EVENTS; i++)
 		list_add(&cpts->pool_data[i].list, &cpts->pool);
 
-	cpts_clk_init(dev, cpts);
+	clk_enable(cpts->refclk);
+
 	cpts_write32(cpts, CPTS_EN, control);
 	cpts_write32(cpts, TS_PEND_EN, int_enable);
 
-	spin_lock_irqsave(&cpts->lock, flags);
+	cpts->cc.mult = cpts->cc_mult;
 	timecounter_init(&cpts->tc, &cpts->cc, ktime_to_ns(ktime_get_real()));
-	spin_unlock_irqrestore(&cpts->lock, flags);
-
-	INIT_DELAYED_WORK(&cpts->overflow_work, cpts_overflow_check);
-	schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
 
+	cpts->info = cpts_info;
+	cpts->clock = ptp_clock_register(&cpts->info, cpts->dev);
+	if (IS_ERR(cpts->clock)) {
+		err = PTR_ERR(cpts->clock);
+		cpts->clock = NULL;
+		goto err_ptp;
+	}
 	cpts->phc_index = ptp_clock_index(cpts->clock);
+
+	schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
 	return 0;
+
+err_ptp:
+	clk_disable(cpts->refclk);
+	return err;
 }
 
 void cpts_unregister(struct cpts *cpts)
 {
-	if (cpts->clock) {
-		ptp_clock_unregister(cpts->clock);
-		cancel_delayed_work_sync(&cpts->overflow_work);
+	if (!cpts)
+		return;
+
+	if (WARN_ON(!cpts->clock))
+		return;
+
+	cancel_delayed_work_sync(&cpts->overflow_work);
+
+	ptp_clock_unregister(cpts->clock);
+	cpts->clock = NULL;
+
+	cpts_write32(cpts, 0, int_enable);
+	cpts_write32(cpts, 0, control);
+
+	clk_disable(cpts->refclk);
+}
+
+struct cpts *cpts_create(struct device *dev, void __iomem *regs,
+			 u32 mult, u32 shift)
+{
+	struct cpts *cpts;
+
+	if (!regs || !dev)
+		return ERR_PTR(-EINVAL);
+
+	cpts = devm_kzalloc(dev, sizeof(*cpts), GFP_KERNEL);
+	if (!cpts)
+		return ERR_PTR(-ENOMEM);
+
+	cpts->dev = dev;
+	cpts->reg = (struct cpsw_cpts __iomem *)regs;
+	spin_lock_init(&cpts->lock);
+	INIT_DELAYED_WORK(&cpts->overflow_work, cpts_overflow_check);
+
+	cpts->refclk = devm_clk_get(dev, "cpts");
+	if (IS_ERR(cpts->refclk)) {
+		dev_err(dev, "Failed to get cpts refclk\n");
+		return ERR_PTR(PTR_ERR(cpts->refclk));
 	}
-	if (cpts->refclk)
-		cpts_clk_release(cpts);
+
+	clk_prepare(cpts->refclk);
+
+	cpts->cc.read = cpts_systim_read;
+	cpts->cc.mask = CLOCKSOURCE_MASK(32);
+	cpts->cc.shift = shift;
+	cpts->cc_mult = mult;
+
+	return cpts;
+}
+
+void cpts_release(struct cpts *cpts)
+{
+	if (!cpts)
+		return;
+
+	if (!cpts->refclk)
+		return;
+
+	clk_unprepare(cpts->refclk);
 }
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index fec753c..0c02f48 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -20,6 +20,8 @@
 #ifndef _TI_CPTS_H_
 #define _TI_CPTS_H_
 
+#ifdef CONFIG_TI_CPTS
+
 #include <linux/clk.h>
 #include <linux/clkdev.h>
 #include <linux/clocksource.h>
@@ -108,10 +110,10 @@ struct cpts_event {
 };
 
 struct cpts {
+	struct device *dev;
 	struct cpsw_cpts __iomem *reg;
 	int tx_enable;
 	int rx_enable;
-#ifdef CONFIG_TI_CPTS
 	struct ptp_clock_info info;
 	struct ptp_clock *clock;
 	spinlock_t lock; /* protects time registers */
@@ -124,14 +126,15 @@ struct cpts {
 	struct list_head events;
 	struct list_head pool;
 	struct cpts_event pool_data[CPTS_MAX_EVENTS];
-#endif
 };
 
-#ifdef CONFIG_TI_CPTS
 void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
 void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb);
-int cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift);
+int cpts_register(struct cpts *cpts);
 void cpts_unregister(struct cpts *cpts);
+struct cpts *cpts_create(struct device *dev, void __iomem *regs,
+			 u32 mult, u32 shift);
+void cpts_release(struct cpts *cpts);
 
 static inline void cpts_rx_enable(struct cpts *cpts, int enable)
 {
@@ -156,6 +159,8 @@ static inline bool cpts_is_tx_enabled(struct cpts *cpts)
 }
 
 #else
+struct cpts;
+
 static inline void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
 {
 }
@@ -163,8 +168,19 @@ static inline void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
 {
 }
 
+static inline
+struct cpts *cpts_create(struct device *dev, void __iomem *regs,
+			 u32 mult, u32 shift)
+{
+	return NULL;
+}
+
+static inline void cpts_release(struct cpts *cpts)
+{
+}
+
 static inline int
-cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift)
+cpts_register(struct cpts *cpts)
 {
 	return 0;
 }
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1483319 — Re: [PATCH 3/9] net: ethernet: ti: cpts: rework initialization/deinitialization

On Wed, Sep 14, 2016 at 04:02:25PM +0300, Grygorii Strashko wrote:
> @@ -323,7 +307,7 @@ void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>  	u64 ns;
>  	struct skb_shared_hwtstamps *ssh;
>  
> -	if (!cpts->rx_enable)
> +	if (!cpts || !cpts->rx_enable)
>  		return;

This function is in the hot path, and you have added a pointless new
test.  Don't do that.

>  	ns = cpts_find_ts(cpts, skb, CPTS_EV_RX);
>  	if (!ns)
> @@ -338,7 +322,7 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>  	u64 ns;
>  	struct skb_shared_hwtstamps ssh;
>  
> -	if (!(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
> +	if (!cpts || !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
>  		return;

Same here.

>  	ns = cpts_find_ts(cpts, skb, CPTS_EV_TX);
>  	if (!ns)
> @@ -348,53 +332,102 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>  	skb_tstamp_tx(skb, &ssh);
>  }
>  
> -int cpts_register(struct device *dev, struct cpts *cpts,
> -		  u32 mult, u32 shift)
> +int cpts_register(struct cpts *cpts)
>  {
>  	int err, i;
> -	unsigned long flags;
>  
> -	cpts->info = cpts_info;
> -	cpts->clock = ptp_clock_register(&cpts->info, dev);
> -	if (IS_ERR(cpts->clock)) {
> -		err = PTR_ERR(cpts->clock);
> -		cpts->clock = NULL;
> -		return err;
> -	}
> -	spin_lock_init(&cpts->lock);
> -
> -	cpts->cc.read = cpts_systim_read;
> -	cpts->cc.mask = CLOCKSOURCE_MASK(32);
> -	cpts->cc_mult = mult;
> -	cpts->cc.mult = mult;
> -	cpts->cc.shift = shift;
> +	if (!cpts)
> +		return -EINVAL;

Not hot path, but still silly.  The caller should never pass NULL.

Thanks,
Richard

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web