diff mbox

[v4,09/13] net: ethernet: ti: cpts: rework initialization/deinitialization

Message ID 20161205200525.16664-10-grygorii.strashko@ti.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Grygorii Strashko Dec. 5, 2016, 8:05 p.m. UTC
The current implementation CPTS initialization and deinitialization
(represented by cpts_register/unregister()) does too many static
initialization from .ndo_open(), which is reasonable to do once at probe
time instead, and also require caller to allocate memory for struct cpts,
which is internal for CPTS driver in general.

This patch splits CPTS initialization and deinitialization on two parts:

- static initializtion cpts_create()/cpts_release() which expected to be
executed when parent driver is probed/removed;

- dynamic part cpts_register/unregister() which expected to be executed
when network device is opened/closed.

As result, current code of CPTS parent driver - CPSW - will be simplified
(and it also will allow simplify adding support for Keystone 2 devices in
the future), plus more initialization errors will be catched earlier. 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 | 102 ++++++++++++++++++++++++-----------------
 drivers/net/ethernet/ti/cpts.h |  26 +++++++++--
 3 files changed, 95 insertions(+), 57 deletions(-)

Comments

Richard Cochran Dec. 6, 2016, 1:40 p.m. UTC | #1
On Mon, Dec 05, 2016 at 02:05:21PM -0600, Grygorii Strashko wrote:
> @@ -372,34 +354,27 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>  }
>  EXPORT_SYMBOL_GPL(cpts_tx_timestamp);
>  
> -int cpts_register(struct device *dev, struct cpts *cpts,
> -		  u32 mult, u32 shift)
> +int cpts_register(struct cpts *cpts)
>  {
>  	int err, i;
>  
> -	cpts->info = cpts_info;
> -	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;
> -
>  	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);
>  
> +	/* reinitialize cc.mult to original value as it can be modified
> +	 * by cpts_ptp_adjfreq().
> +	 */
> +	cpts->cc.mult = cpts->cc_mult;

This still isn't quite right.  First of all, you shouldn't clobber the
learned cc.mult value in cpts_register().  Presumably, if PTP had been
run on this port before, then the learned frequency is approximately
correct, and it should be left alone.

[ BTW, resetting the timecounter here makes no sense either.  Why
  reset the clock just because the interface goes down?  ]

Secondly, you have made the initialization order of these fields hard
to follow.  With the whole series applied:

probe()
	cpts_create()
		cpts_of_parse()
		{
			/* Set cc_mult but not cc.mult! */
			set cc_mult
			set cc.shift
		}
		cpts_calc_mult_shift()
		{
			/* Set them both. */
			cpts->cc_mult = mult;
			cpts->cc.mult = mult;
			cpts->cc.shift = shift;
		}
/* later on */
cpts_register()
	cpts->cc.mult = cpts->cc_mult;

There is no need for such complexity.  Simply set cc.mult in
cpts_create() _once_, immediately after the call to
cpts_calc_mult_shift().

You can remove the assignment from cpts_calc_mult_shift() and
cpts_register().

Thanks,
Richard
Grygorii Strashko Dec. 6, 2016, 4:45 p.m. UTC | #2
On 12/06/2016 07:40 AM, Richard Cochran wrote:
> On Mon, Dec 05, 2016 at 02:05:21PM -0600, Grygorii Strashko wrote:
>> @@ -372,34 +354,27 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>>  }
>>  EXPORT_SYMBOL_GPL(cpts_tx_timestamp);
>>  
>> -int cpts_register(struct device *dev, struct cpts *cpts,
>> -		  u32 mult, u32 shift)
>> +int cpts_register(struct cpts *cpts)
>>  {
>>  	int err, i;
>>  
>> -	cpts->info = cpts_info;
>> -	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;
>> -
>>  	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);
>>  
>> +	/* reinitialize cc.mult to original value as it can be modified
>> +	 * by cpts_ptp_adjfreq().
>> +	 */
>> +	cpts->cc.mult = cpts->cc_mult;
> 
> This still isn't quite right.  First of all, you shouldn't clobber the
> learned cc.mult value in cpts_register().  Presumably, if PTP had been
> run on this port before, then the learned frequency is approximately
> correct, and it should be left alone.
> 
> [ BTW, resetting the timecounter here makes no sense either.  Why
>   reset the clock just because the interface goes down?  ]
> 

Huh. This is how it works now (even before my changes) - this is just refactoring!
(really new thing is the only cpts_calc_mult_shift()).

Also, this is how cpts is supported now as part of cpsw (and keystone):
configure cpsw (cpts)
- ifup
   cpsw (*soft_reset*, full reconfiguration of cpsw)
  (start cpts) - cpts/ptp active

- ifdown
   if last netdev - shutdown/poweroff cpsw (cpts)

in other words, cpts/ptp is expected to work once and until at least one cpsw netdev is active.

Also there are additional questions such as:
- is there guarantee that cpsw port will be connected to the same network after ifup?
- should there be possibility to reset cc.mult if it's value will be kept from the previous run?

> Secondly, you have made the initialization order of these fields hard
> to follow.  With the whole series applied:
> 
> probe()
> 	cpts_create()
> 		cpts_of_parse()
> 		{
> 			/* Set cc_mult but not cc.mult! */
> 			set cc_mult
> 			set cc.shift
> 		}
> 		cpts_calc_mult_shift()
> 		{
> 			/* Set them both. */
> 			cpts->cc_mult = mult;
> 			cpts->cc.mult = mult; 

^^ this assignment of cpts->cc.mult not required.

> 			cpts->cc.shift = shift;
			

only in case there were not set in DT before
(I have a requirement to support both - DT and cpts_calc_mult_shift and
 that introduces a bit of complexity)

Also, I've tried not to add more fields in struct cpts.

> 		}
> /* later on */
> cpts_register()
> 	cpts->cc.mult = cpts->cc_mult;
> 
> There is no need for such complexity.  Simply set cc.mult in
> cpts_create() _once_, immediately after the call to
> cpts_calc_mult_shift().
> 
> You can remove the assignment from cpts_calc_mult_shift() and
> cpts_register().

Just to clarify: do you propose to get rid of cpts->cc_mult at all?

static int cpts_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
{
...
	if (ppb < 0) {
		neg_adj = 1;
		ppb = -ppb;
	}
	mult = cpts->cc_mult;
		^^^^^^^^^^^^^^
	adj = mult;
	adj *= ppb;
	diff = div_u64(adj, 1000000000ULL);
...
	cpts->cc.mult = neg_adj ? mult - diff : mult + diff;

Honestly, i'd not prefer to change functional behavior of ptp clock as part of
this series.
Richard Cochran Dec. 6, 2016, 5:18 p.m. UTC | #3
On Tue, Dec 06, 2016 at 10:45:55AM -0600, Grygorii Strashko wrote:
> On 12/06/2016 07:40 AM, Richard Cochran wrote:
> > [ BTW, resetting the timecounter here makes no sense either.  Why
> >   reset the clock just because the interface goes down?  ]
> > 
> 
> Huh. This is how it works now (even before my changes) - this is just refactoring!
> (really new thing is the only cpts_calc_mult_shift()).

The cpts_register() used to be called from probe(), but this changed
without any review in f280e89ad.  That wasn't your fault, but still...
 
> Also there are additional questions such as:
> - is there guarantee that cpsw port will be connected to the same network after ifup?

The network is not relevant.  PTP time is a global standard, just like
with NTP.  We don't reset the NTP clock with ifup/down, do we?

> - should there be possibility to reset cc.mult if it's value will be kept from the previous run?

cc.mult will change naturally according to the operation of the user
space PTP stack.  There is no need to reset it that I can see.
 
> > Secondly, you have made the initialization order of these fields hard
> > to follow.  With the whole series applied:
> > 
> > probe()
> > 	cpts_create()
> > 		cpts_of_parse()
> > 		{
> > 			/* Set cc_mult but not cc.mult! */
> > 			set cc_mult
> > 			set cc.shift
> > 		}
> > 		cpts_calc_mult_shift()
> > 		{
> > 			/* Set them both. */
> > 			cpts->cc_mult = mult;
> > 			cpts->cc.mult = mult; 
> 
> ^^ this assignment of cpts->cc.mult not required.

You wrote the code, not me.
 
> > 			cpts->cc.shift = shift;
> 			
> 
> only in case there were not set in DT before
> (I have a requirement to support both - DT and cpts_calc_mult_shift and
>  that introduces a bit of complexity)
> 
> Also, I've tried not to add more fields in struct cpts.
> 
> > 		}
> > /* later on */
> > cpts_register()
> > 	cpts->cc.mult = cpts->cc_mult;
> > 
> > There is no need for such complexity.  Simply set cc.mult in
> > cpts_create() _once_, immediately after the call to
> > cpts_calc_mult_shift().
> > 
> > You can remove the assignment from cpts_calc_mult_shift() and
> > cpts_register().
> 
> Just to clarify: do you propose to get rid of cpts->cc_mult at all?

No.  Read what I said before:

   There is no need for such complexity.  Simply set cc.mult in
   cpts_create() _once_, immediately after the call to
   cpts_calc_mult_shift().

> Honestly, i'd not prefer to change functional behavior of ptp clock as part of
> this series.

Fair enough.  The bogus clock reset existed before your series.

But please don't obfuscate simple initialization routines.  The way
you set cc.mult and cc_mult is illogical and convoluted.

Thanks,
Richard
Grygorii Strashko Dec. 6, 2016, 5:49 p.m. UTC | #4
Hi Richard,

On 12/06/2016 11:18 AM, Richard Cochran wrote:
> On Tue, Dec 06, 2016 at 10:45:55AM -0600, Grygorii Strashko wrote:
>> On 12/06/2016 07:40 AM, Richard Cochran wrote:
>>> [ BTW, resetting the timecounter here makes no sense either.  Why
>>>   reset the clock just because the interface goes down?  ]
>>>
>>
>> Huh. This is how it works now (even before my changes) - this is just refactoring!
>> (really new thing is the only cpts_calc_mult_shift()).
> 
> The cpts_register() used to be called from probe(), but this changed
> without any review in f280e89ad.  That wasn't your fault, but still...
>  
>> Also there are additional questions such as:
>> - is there guarantee that cpsw port will be connected to the same network after ifup?
> 
> The network is not relevant.  PTP time is a global standard, just like
> with NTP.  We don't reset the NTP clock with ifup/down, do we?

But we do reset whole cpsw :( and that's required to support PM use cases as
suspend/resume.

There are also PM requirement to shutdown cpsw in case all interfaces are down.

More over, there are requirement to minimize cpsw power consumption in case all links are
disconnected (and cpts is special case here).

So, at least resetting of the timecounter still required.



> 
>> - should there be possibility to reset cc.mult if it's value will be kept from the previous run?
> 
> cc.mult will change naturally according to the operation of the user
> space PTP stack.  There is no need to reset it that I can see.
>  
>>> Secondly, you have made the initialization order of these fields hard
>>> to follow.  With the whole series applied:
>>>
>>> probe()
>>> 	cpts_create()
>>> 		cpts_of_parse()
>>> 		{
>>> 			/* Set cc_mult but not cc.mult! */
>>> 			set cc_mult
>>> 			set cc.shift
>>> 		}
>>> 		cpts_calc_mult_shift()
>>> 		{
>>> 			/* Set them both. */
>>> 			cpts->cc_mult = mult;
>>> 			cpts->cc.mult = mult; 
>>
>> ^^ this assignment of cpts->cc.mult not required.
> 
> You wrote the code, not me.

Sry, I meant, thank for catching this.

>  
>>> 			cpts->cc.shift = shift;
>> 			
>>
>> only in case there were not set in DT before
>> (I have a requirement to support both - DT and cpts_calc_mult_shift and
>>  that introduces a bit of complexity)
>>
>> Also, I've tried not to add more fields in struct cpts.
>>
>>> 		}
>>> /* later on */
>>> cpts_register()
>>> 	cpts->cc.mult = cpts->cc_mult;
>>>
>>> There is no need for such complexity.  Simply set cc.mult in
>>> cpts_create() _once_, immediately after the call to
>>> cpts_calc_mult_shift().
>>>
>>> You can remove the assignment from cpts_calc_mult_shift() and
>>> cpts_register().
>>
>> Just to clarify: do you propose to get rid of cpts->cc_mult at all?
> 
> No.  Read what I said before:
> 
>    There is no need for such complexity.  Simply set cc.mult in
>    cpts_create() _once_, immediately after the call to
>    cpts_calc_mult_shift().
> 
>> Honestly, i'd not prefer to change functional behavior of ptp clock as part of
>> this series.
> 
> Fair enough.  The bogus clock reset existed before your series.
> 
> But please don't obfuscate simple initialization routines.  The way
> you set cc.mult and cc_mult is illogical and convoluted.
> 

Ok. I'll try to optimize it following your directions.

Thanks.
Richard Cochran Dec. 6, 2016, 6:04 p.m. UTC | #5
On Tue, Dec 06, 2016 at 11:49:14AM -0600, Grygorii Strashko wrote:
> But we do reset whole cpsw :( and that's required to support PM use cases as
> suspend/resume.

The code is resetting the clock unconditionally on ifup/down.  That
sucks.  If you reset the clock *only* after resume, that would be ok.
 
> There are also PM requirement to shutdown cpsw in case all interfaces are down.

Well, those requirements are not too smart.  As an end user, I expect
that ifdown/up does not change the time.  There isn't any reason to
reset the clock in this case.

> More over, there are requirement to minimize cpsw power consumption in case all links are
> disconnected (and cpts is special case here).
> 
> So, at least resetting of the timecounter still required.

Only if you follow that poorly conceived PM plan.  Anyhow, I agree
that it isn't the task of your present series to fix that.

> Ok. I'll try to optimize it following your directions.

What I would like to see is: initialize the cyclecounter fields
exactly once.

Thanks,
Richard
diff mbox

Patch

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index ec05e20..deb008a 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1486,9 +1486,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");
 
 	}
@@ -2796,6 +2794,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;
@@ -2823,12 +2822,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)) {
@@ -2916,7 +2909,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;
@@ -2930,7 +2923,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;
@@ -2997,6 +2990,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");
@@ -3112,6 +3113,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);
 	cpsw_remove_dt(pdev);
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index fe1bb7f..9356803 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -248,24 +248,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)
-{
-	if (!cpts->refclk) {
-		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_unprepare(cpts->refclk);
-}
-
 static int cpts_match(struct sk_buff *skb, unsigned int ptp_class,
 		      u16 ts_seqid, u8 ts_msgtype)
 {
@@ -372,34 +354,27 @@  void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
 }
 EXPORT_SYMBOL_GPL(cpts_tx_timestamp);
 
-int cpts_register(struct device *dev, struct cpts *cpts,
-		  u32 mult, u32 shift)
+int cpts_register(struct cpts *cpts)
 {
 	int err, i;
 
-	cpts->info = cpts_info;
-	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;
-
 	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);
 
+	/* reinitialize cc.mult to original value as it can be modified
+	 * by cpts_ptp_adjfreq().
+	 */
+	cpts->cc.mult = cpts->cc_mult;
 	timecounter_init(&cpts->tc, &cpts->cc, ktime_to_ns(ktime_get_real()));
 
-	INIT_DELAYED_WORK(&cpts->overflow_work, cpts_overflow_check);
-
-	cpts->clock = ptp_clock_register(&cpts->info, dev);
+	cpts->clock = ptp_clock_register(&cpts->info, cpts->dev);
 	if (IS_ERR(cpts->clock)) {
 		err = PTR_ERR(cpts->clock);
 		cpts->clock = NULL;
@@ -412,27 +387,72 @@  int cpts_register(struct device *dev, struct cpts *cpts,
 	return 0;
 
 err_ptp:
-	if (cpts->refclk)
-		cpts_clk_release(cpts);
+	clk_disable(cpts->refclk);
 	return err;
 }
 EXPORT_SYMBOL_GPL(cpts_register);
 
 void cpts_unregister(struct cpts *cpts)
 {
-	if (cpts->clock) {
-		ptp_clock_unregister(cpts->clock);
-		cancel_delayed_work_sync(&cpts->overflow_work);
-	}
+	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);
 
-	if (cpts->refclk)
-		cpts_clk_release(cpts);
+	clk_disable(cpts->refclk);
 }
 EXPORT_SYMBOL_GPL(cpts_unregister);
 
+struct cpts *cpts_create(struct device *dev, void __iomem *regs,
+			 u32 mult, u32 shift)
+{
+	struct cpts *cpts;
+
+	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));
+	}
+
+	clk_prepare(cpts->refclk);
+
+	cpts->cc.read = cpts_systim_read;
+	cpts->cc.mask = CLOCKSOURCE_MASK(32);
+	cpts->cc.shift = shift;
+	cpts->cc_mult = mult;
+	cpts->info = cpts_info;
+
+	return cpts;
+}
+EXPORT_SYMBOL_GPL(cpts_create);
+
+void cpts_release(struct cpts *cpts)
+{
+	if (!cpts)
+		return;
+
+	if (WARN_ON(!cpts->clock))
+		return;
+
+	clk_unprepare(cpts->refclk);
+}
+EXPORT_SYMBOL_GPL(cpts_release);
+
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("TI CPTS driver");
 MODULE_AUTHOR("Richard Cochran <richardcochran@gmail.com>");
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index 29a1e80c..e7d857c 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_
 
+#if IS_ENABLED(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;
-#if IS_ENABLED(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
 };
 
-#if IS_ENABLED(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)
 {
@@ -154,6 +157,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)
 {
 }
@@ -161,8 +166,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;
 }