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
