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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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

> 
>> +/* 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!

> 
>> +static int fbnic_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
>> +{
>> +	int scale = 16; /* scaled_ppm has 16 fractional places */
>> +	struct fbnic_dev *fbd = fbnic_from_ptp_info(ptp);
>> +	u64 scaled_delta, dclk_period;
>> +	unsigned long flags;
>> +	s64 delta;
>> +	int sgn;
>> +
>> +	sgn = scaled_ppm >= 0 ? 1 : -1;
>> +	scaled_ppm *= sgn;
>> +
>> +	/* d_clock is 600 MHz; which in Q16.32 fixed point ns is: */
>> +	dclk_period = (((u64)1000000000) << 32) / FBNIC_CLOCK_FREQ;
>> +
>> +	while (scaled_ppm > U64_MAX / dclk_period) {
>> +		scaled_ppm >>= 1;
>> +		scale--;
>> +	}
>> +
>> +	scaled_delta = (u64)scaled_ppm * dclk_period;
>> +	delta = div_u64(scaled_delta, 1000 * 1000) >> scale;
>> +	delta *= sgn;
> 
> 
> Please use adjust_by_scaled_ppm or diff_by_scaled_ppm. It makes use of
> mul_u64_u64_div_u64 where feasible to do the temporary multiplication
> step as 128 bit arithmetic.
> 

Got it, will use it in the next version.

>> +
>> +	spin_lock_irqsave(&fbd->time_lock, flags);
>> +	__fbnic_time_set_addend(fbd, dclk_period + delta);
>> +	fbnic_wr32(fbd, FBNIC_PTP_ADJUST, FBNIC_PTP_ADJUST_ADDEND_SET);
>> +
>> +	/* Flush, make sure FBNIC_PTP_ADD_VAL_* is stable for at least 4 clks */
>> +	fbnic_rd32(fbd, FBNIC_PTP_SPARE);
>> +	spin_unlock_irqrestore(&fbd->time_lock, flags);
>> +
>> +	return fbnic_present(fbd) ? 0 : -EIO;
>> +}
>> +
>> +static int fbnic_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
>> +{
>> +	struct fbnic_dev *fbd = fbnic_from_ptp_info(ptp);
>> +	struct fbnic_net *fbn;
>> +	unsigned long flags;
>> +
>> +	fbn = netdev_priv(fbd->netdev);
>> +
>> +	spin_lock_irqsave(&fbd->time_lock, flags);
>> +	u64_stats_update_begin(&fbn->time_seq);
>> +	WRITE_ONCE(fbn->time_offset, READ_ONCE(fbn->time_offset) + delta);
>> +	u64_stats_update_end(&fbn->time_seq);
>> +	spin_unlock_irqrestore(&fbd->time_lock, flags);
>> +
>> +	return 0;
>> +}
>> +
>> +static int
>> +fbnic_ptp_gettimex64(struct ptp_clock_info *ptp, struct timespec64 *ts,
>> +		     struct ptp_system_timestamp *sts)
>> +{
>> +	struct fbnic_dev *fbd = fbnic_from_ptp_info(ptp);
>> +	struct fbnic_net *fbn;
>> +	unsigned long flags;
>> +	u64 time_ns;
>> +	u32 hi, lo;
>> +
>> +	fbn = netdev_priv(fbd->netdev);
>> +
>> +	spin_lock_irqsave(&fbd->time_lock, flags);
>> +
>> +	do {
>> +		hi = fbnic_rd32(fbd, FBNIC_PTP_CTR_VAL_HI);
>> +		ptp_read_system_prets(sts);
>> +		lo = fbnic_rd32(fbd, FBNIC_PTP_CTR_VAL_LO);
>> +		ptp_read_system_postts(sts);
>> +		/* Similarly to comment above __fbnic_time_get_slow()
>> +		 * - this can be optimized if needed.
>> +		 */
>> +	} while (hi != fbnic_rd32(fbd, FBNIC_PTP_CTR_VAL_HI));
>> +
>> +	time_ns = ((u64)hi << 32 | lo) + fbn->time_offset;
>> +	spin_unlock_irqrestore(&fbd->time_lock, flags);
>> +
>> +	if (!fbnic_present(fbd))
>> +		return -EIO;
>> +
>> +	*ts = ns_to_timespec64(time_ns);
>> +
>> +	return 0;
>> +}
>> +
>> +static int
>> +fbnic_ptp_settime64(struct ptp_clock_info *ptp, const struct timespec64 *ts)
>> +{
>> +	struct fbnic_dev *fbd = fbnic_from_ptp_info(ptp);
>> +	struct fbnic_net *fbn;
>> +	unsigned long flags;
>> +	u64 dev_ns, host_ns;
>> +	int ret;
>> +
>> +	fbn = netdev_priv(fbd->netdev);
>> +
>> +	host_ns = timespec64_to_ns(ts);
>> +
>> +	spin_lock_irqsave(&fbd->time_lock, flags);
>> +
>> +	dev_ns = __fbnic_time_get_slow(fbd);
>> +
>> +	if (fbnic_present(fbd)) {
>> +		u64_stats_update_begin(&fbn->time_seq);
>> +		WRITE_ONCE(fbn->time_offset, host_ns - dev_ns);
>> +		u64_stats_update_end(&fbn->time_seq);
>> +		ret = 0;
>> +	} else {
>> +		ret = -EIO;
>> +	}
>> +	spin_unlock_irqrestore(&fbd->time_lock, flags);
>> +
>> +	return ret;
>> +}
>> +
> 
> Since all your operations are using a software offset and leaving the
> timer free-running, I think this really would make more sense using a
> timecounter and cyclecounter.
> 

Yeah, looks like it's better to use common interfaces.

>> +static const struct ptp_clock_info fbnic_ptp_info = {
>> +	.owner			= THIS_MODULE,
>> +	/* 1,000,000,000 - 1 PPB to ensure increment is positive
>> +	 * after max negative adjustment.
>> +	 */
>> +	.max_adj		= 999999999,
>> +	.do_aux_work		= fbnic_ptp_do_aux_work,
>> +	.adjfine		= fbnic_ptp_adjfine,
>> +	.adjtime		= fbnic_ptp_adjtime,
>> +	.gettimex64		= fbnic_ptp_gettimex64,
>> +	.settime64		= fbnic_ptp_settime64,
>> +};
>> +
>> +static void fbnic_ptp_reset(struct fbnic_dev *fbd)
>> +{
>> +	struct fbnic_net *fbn = netdev_priv(fbd->netdev);
>> +	u64 dclk_period;
>> +
>> +	fbnic_wr32(fbd, FBNIC_PTP_CTRL,
>> +		   FBNIC_PTP_CTRL_EN |
>> +		   FIELD_PREP(FBNIC_PTP_CTRL_TICK_IVAL, 1));
>> +
>> +	/* d_clock is 600 MHz; which in Q16.32 fixed point ns is: */
>> +	dclk_period = (((u64)1000000000) << 32) / FBNIC_CLOCK_FREQ;
>> +
>> +	__fbnic_time_set_addend(fbd, dclk_period);
>> +
>> +	fbnic_wr32(fbd, FBNIC_PTP_INIT_HI, 0);
>> +	fbnic_wr32(fbd, FBNIC_PTP_INIT_LO, 0);
>> +
>> +	fbnic_wr32(fbd, FBNIC_PTP_ADJUST, FBNIC_PTP_ADJUST_INIT);
>> +
>> +	fbnic_wr32(fbd, FBNIC_PTP_CTRL,
>> +		   FBNIC_PTP_CTRL_EN |
>> +		   FBNIC_PTP_CTRL_TQS_OUT_EN |
>> +		   FIELD_PREP(FBNIC_PTP_CTRL_MAC_OUT_IVAL, 3) |
>> +		   FIELD_PREP(FBNIC_PTP_CTRL_TICK_IVAL, 1));
>> +
>> +	fbnic_rd32(fbd, FBNIC_PTP_SPARE);
>> +
>> +	fbn->time_offset = 0;
>> +	fbn->time_high = 0;
> 
> Not entirely sure how it works for you, but we found that most users
> expect to minimize the time loss or changes due to a reset, and we
> re-apply the last known configuration during a reset instead of letting
> the clock reset back to base state.

In case of reset the counter will be re-initialized, which means the
stored offset will be invalid anyways. It's better to reset values back
to base state because it will make easier for the app to decide to make
a step rather then adjust frequency. And it will make clocks align
faster.