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:

quoted

On 12/06/2016 07:40 AM, Richard Cochran wrote:

quoted

[ 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...
 

quoted

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.

quoted

- 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.
 

quoted

quoted

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.

quoted

quoted

			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.

quoted

		}
/* 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().

quoted

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.

-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html