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

[Grygorii Strashko]

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.

-- 
regards,
-grygorii