RE: [Intel-wired-lan] [PATCH iwl-net 08/10] idpf: add Tx timestamp flows

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Olech, Milena wrote:
> On 11/15/2024 12:20 AM, Willem de Bruijn wrote:
> 
> > Milena Olech wrote:
> > > Add functions to request Tx timestamp for the PTP packets, read the Tx
> > > timestamp when the completion tag for that packet is being received,
> > > extend the Tx timestamp value and set the supported timestamping modes.
> > > 
> > > Tx timestamp is requested for the PTP packets by setting a TSYN bit and
> > > index value in the Tx context descriptor. The driver assumption is that
> > > the Tx timestamp value is ready to be read when the completion tag is
> > > received. Then the driver schedules delayed work and the Tx timestamp
> > > value read is requested through virtchnl message. At the end, the Tx
> > > timestamp value is extended to 64-bit and provided back to the skb.
> > > 
> > > Co-developed-by: Josh Hay <joshua.a.hay@intel.com>
> > > Signed-off-by: Josh Hay <joshua.a.hay@intel.com>
> > > Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> > > Signed-off-by: Milena Olech <milena.olech@intel.com>

> > > +/**
> > > + * idpf_ptp_tstamp_extend_32b_to_64b - Convert a 32b nanoseconds Tx timestamp
> > > + *				       to 64b.
> > > + * @cached_phc_time: recently cached copy of PHC time
> > > + * @in_timestamp: Ingress/egress 32b nanoseconds timestamp value
> > > + *
> > > + * Hardware captures timestamps which contain only 32 bits of nominal
> > > + * nanoseconds, as opposed to the 64bit timestamps that the stack expects.
> > > + *
> > > + * Return: Tx timestamp value extended to 64 bits based on cached PHC time.
> > > + */
> > > +u64 idpf_ptp_tstamp_extend_32b_to_64b(u64 cached_phc_time, u32 in_timestamp)
> > > +{
> > > +	u32 delta, phc_lo;
> > > +	u64 ns;
> > > +
> > > +	phc_lo = lower_32_bits(cached_phc_time);
> > > +	delta = in_timestamp - phc_lo;
> > > +
> > > +	if (delta > S32_MAX) {
> > > +		delta = phc_lo - in_timestamp;
> > > +		ns = cached_phc_time - delta;
> > > +	} else {
> > > +		ns = cached_phc_time + delta;
> > > +	}
> > > +
> > > +	return ns;
> > > +}
> > 
> > Consider a standard timecounter to estimate a device clock.
> 
> You mean to rely on standard timecounter instead of cached PHC time?
> Can you please clarify?

Yes. To clarify: this is a suggestion to consider. Feel free to skip.

A timecounter/cyclecounter maintains an estimate on a clock. That
is more precise than just using the last cached value, and preferable
over open coding such an estimation algorithm.

Other drivers already use such a struct, I assume to estimate their
PTP device clock.