Re: [PATCH net-next v3 2/5] eth: fbnic: add initial PHC support

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 7 Oct 2024 14:07:17 +0100 Vadim Fedorenko wrote:
> On 05/10/2024 00:05, Jacob Keller wrote:
> > On 10/3/2024 5:39 AM, Vadim Fedorenko wrote:  
> >> +/* FBNIC timing & PTP implementation
> >> + * Datapath uses truncated 40b timestamps for scheduling and event reporting.
> >> + * We need to promote those to full 64b, hence we periodically cache the top
> >> + * 32bit of the HW time counter. Since this makes our time reporting non-atomic
> >> + * we leave the HW clock free running and adjust time offsets in SW as needed.
> >> + * Time offset is 64bit - we need a seq counter for 32bit machines.
> >> + * Time offset and the cache of top bits are independent so we don't need
> >> + * a coherent snapshot of both - READ_ONCE()/WRITE_ONCE() + writer side lock
> >> + * are enough.
> >> + */
> >> +  
> > 
> > If you're going to implement adjustments only in software anyways, can
> > you use a timecounter+cyclecounter instead of re-implementing?  
> 
> Thanks for pointing this out, I'll make it with timecounter/cyclecounter

Please don't, the clock is synthonized, we only do simple offsetting.

> >> +/* Period of refresh of top bits of timestamp, give ourselves a 8x margin.
> >> + * This should translate to once a minute.
> >> + * The use of nsecs_to_jiffies() should be safe for a <=40b nsec value.
> >> + */
> >> +#define FBNIC_TS_HIGH_REFRESH_JIF	nsecs_to_jiffies((1ULL << 40) / 16)
> >> +
> >> +static struct fbnic_dev *fbnic_from_ptp_info(struct ptp_clock_info *ptp)
> >> +{
> >> +	return container_of(ptp, struct fbnic_dev, ptp_info);
> >> +}
> >> +
> >> +/* This function is "slow" because we could try guessing which high part
> >> + * is correct based on low instead of re-reading, and skip reading @hi
> >> + * twice altogether if @lo is far enough from 0.
> >> + */
> >> +static u64 __fbnic_time_get_slow(struct fbnic_dev *fbd)
> >> +{
> >> +	u32 hi, lo;
> >> +
> >> +	lockdep_assert_held(&fbd->time_lock);
> >> +
> >> +	do {
> >> +		hi = fbnic_rd32(fbd, FBNIC_PTP_CTR_VAL_HI);
> >> +		lo = fbnic_rd32(fbd, FBNIC_PTP_CTR_VAL_LO);
> >> +	} while (hi != fbnic_rd32(fbd, FBNIC_PTP_CTR_VAL_HI));
> >> +  
> > 
> > How long does it take hi to overflow? You may be able to get away
> > without looping.  
> 
> According to comment above it may take up to 8 minutes to overflow, but
> the updates to the cache should be done every minute. We do not expect
> this cycle to happen often.
> 
> > I think another way to implement this is to read lo, then hi, then lo
> > again, and if lo2 is smaller than lo, you know hi overflowed and you can
> > re-read hi  
> 
> That's an option too, I'll think of it, thanks!

The triple read is less neat in case hi jumps by more than 1.