RE: [PATCH net-next 1/4] net: wangxun: Add support for PTP clock

["Jiawen Wu" <jiawenwu@trustnetic.com>]

On Mon, Jan 6, 2025 10:27 PM, Andrew Lunn wrote:
> > > > +	smp_mb(); /* Force any pending update before accessing. */
> > > > +	incval = READ_ONCE(wx->base_incval);
> > > > +	incval = adjust_by_scaled_ppm(incval, ppb);
> > > > +
> > > > +	mask = (wx->mac.type == wx_mac_em) ? 0x7FFFFFF : 0xFFFFFF;
> > > > +	if (incval > mask)
> > > > +		dev_warn(&wx->pdev->dev,
> > > > +			 "PTP ppb adjusted SYSTIME rate overflowed!\n");
> > >
> > > There is no return here, you just keep going. What happens if there is
> > > an overflow?
> >
> > If there is an overflow, the calibration value of this second will be
> > inaccurate. But it does not affect the calibration value of the next
> > second. And this rarely happens.
> 
> If this is a onetime event you don't really care about, is a
> dev_warn() justified? Do you want to be handling the user questions
> about what it means, when all you are going to say is, ignore it, it
> does not really matter?

I'll remove the dev_warn() to avoid user confusion.

> 
> > > > +/**
> > > > + * wx_ptp_tx_hwtstamp_work
> > > > + * @work: pointer to the work struct
> > > > + *
> > > > + * This work item polls TSYNCTXCTL valid bit to determine when a Tx hardware
> > > > + * timestamp has been taken for the current skb. It is necessary, because the
> > > > + * descriptor's "done" bit does not correlate with the timestamp event.
> > > > + */
> > >
> > > Are you saying the "done" bit can be set, but the timestamp is not yet
> > > in place? I've not read the whole patch, but do you start polling once
> > > "done" is set, or as soon at the skbuff is queues for transmission?
> >
> > The descriptor's "done" bit cannot be used as a basis for Tx hardware
> > timestamp. So we should poll the valid bit in the register.
> 
> You did not answer my question. When do you start polling?

As soon at the skbuff is queues for transmission.