Message-ID: <aJyqDxFwzKgmeUdA@lizhi-Precision-Tower-5810>
Date: Wed, 13 Aug 2025 11:06:55 -0400
From: Frank Li <Frank.li@....com>
To: Wei Fang <wei.fang@....com>
Cc: "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>,
	"richardcochran@...il.com" <richardcochran@...il.com>,
	Claudiu Manoil <claudiu.manoil@....com>,
	Vladimir Oltean <vladimir.oltean@....com>,
	Clark Wang <xiaoning.wang@....com>,
	"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"vadim.fedorenko@...ux.dev" <vadim.fedorenko@...ux.dev>,
	"shawnguo@...nel.org" <shawnguo@...nel.org>,
	"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
	"festevam@...il.com" <festevam@...il.com>,
	"F.S. Peng" <fushi.peng@....com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"kernel@...gutronix.de" <kernel@...gutronix.de>
Subject: Re: [PATCH v3 net-next 05/15] ptp: netc: add PTP_CLK_REQ_PPS support

On Wed, Aug 13, 2025 at 01:59:01AM +0000, Wei Fang wrote:
> > On Tue, Aug 12, 2025 at 05:46:24PM +0800, Wei Fang wrote:
> > > The NETC Timer is capable of generating a PPS interrupt to the host. To
> > > support this feature, a 64-bit alarm time (which is a integral second
> > > of PHC in the future) is set to TMR_ALARM, and the period is set to
> > > TMR_FIPER. The alarm time is compared to the current time on each update,
> > > then the alarm trigger is used as an indication to the TMR_FIPER starts
> > > down counting. After the period has passed, the PPS event is generated.
> > >
> > > According to the NETC block guide, the Timer has three FIPERs, any of
> > > which can be used to generate the PPS events, but in the current
> > > implementation, we only need one of them to implement the PPS feature,
> > > so FIPER 0 is used as the default PPS generator. Also, the Timer has
> > > 2 ALARMs, currently, ALARM 0 is used as the default time comparator.
> > >
> > > However, if there is a time drift when PPS is enabled, the PPS event will
> > > not be generated at an integral second of PHC. The suggested steps from
> > > IP team if time drift happens:
> >
> > according to patch, "drift" means timer adjust period?
>
> No only adjust period, but also including adjust time.

I think 'adjust period and time' is more accurate then drift.  drift always
happen. The problem should happen only at adjust.

>
> > netc_timer_adjust_period()
> >
> > generally, netc_timer_adjust_period() happen 4 times every second, does
> > disable/re-enable impact pps accurate?
>
> PPS needs to be re-enabled only when the integer part of the period changes.
> In this case, re-enabling PPS will result in a loss of PPS signal for 1 ~ 2 seconds.
> In most cases, only the fractional part of the period is adjusted, so there is no
> need to re-enable PPS.

Lost 1-2 second should be okay when adjust time, which only happen at
beginning of sync.

Does software enable/disable impact PPS accurate? For example:

suppose PPS plus at 10000ns position.

enable/disable software take 112ns, does PPS plus at 10112ns Or still at
10000ns position.

Frank

>
> >
> > >
> > > 1. Disable FIPER before adjusting the hardware time
> > > 2. Rearm ALARM after the time adjustment to make the next PPS event be
> > > generated at an integral second of PHC.
> > > 3. Re-enable FIPER.
> > >
> > > Signed-off-by: Wei Fang <wei.fang@....com>
> > >
> > > ---
> > > v2 changes:
> > > 1. Refine the subject and the commit message
> > > 2. Add a comment to netc_timer_enable_pps()
> > > 3. Remove the "nxp,pps-channel" logic from the driver
> > > v3 changes:
> > > 1. Use "2 * NSEC_PER_SEC" to instead of "2000000000U"
> > > 2. Improve the commit message
> > > 3. Add alarm related logic and the irq handler
> > > 4. Add tmr_emask to struct netc_timer to save the irq masks instead of
> > >    reading TMR_EMASK register
> > > 5. Remove pps_channel from struct netc_timer and remove
> > >    NETC_TMR_DEFAULT_PPS_CHANNEL
> > > ---
> > >  drivers/ptp/ptp_netc.c | 260
> > ++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 257 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/ptp/ptp_netc.c b/drivers/ptp/ptp_netc.c
> > > index cbe2a64d1ced..9026a967a5fe 100644
> > > --- a/drivers/ptp/ptp_netc.c
> > > +++ b/drivers/ptp/ptp_netc.c
> > > @@ -20,7 +20,14 @@
> > >  #define  TMR_CTRL_TE			BIT(2)
> > >  #define  TMR_COMP_MODE			BIT(15)
> > >  #define  TMR_CTRL_TCLK_PERIOD		GENMASK(25, 16)
> > > +#define  TMR_CTRL_FS			BIT(28)
> > >
> > > +#define NETC_TMR_TEVENT			0x0084
> > > +#define  TMR_TEVNET_PPEN(i)		BIT(7 - (i))
> > > +#define  TMR_TEVENT_PPEN_ALL		GENMASK(7, 5)
> > > +#define  TMR_TEVENT_ALMEN(i)		BIT(16 + (i))
> > > +
> > > +#define NETC_TMR_TEMASK			0x0088
> > >  #define NETC_TMR_CNT_L			0x0098
> > >  #define NETC_TMR_CNT_H			0x009c
> > >  #define NETC_TMR_ADD			0x00a0
> > > @@ -28,9 +35,19 @@
> > >  #define NETC_TMR_OFF_L			0x00b0
> > >  #define NETC_TMR_OFF_H			0x00b4
> > >
> > > +/* i = 0, 1, i indicates the index of TMR_ALARM */
> > > +#define NETC_TMR_ALARM_L(i)		(0x00b8 + (i) * 8)
> > > +#define NETC_TMR_ALARM_H(i)		(0x00bc + (i) * 8)
> > > +
> > > +/* i = 0, 1, 2. i indicates the index of TMR_FIPER. */
> > > +#define NETC_TMR_FIPER(i)		(0x00d0 + (i) * 4)
> > > +
> > >  #define NETC_TMR_FIPER_CTRL		0x00dc
> > >  #define  FIPER_CTRL_DIS(i)		(BIT(7) << (i) * 8)
> > >  #define  FIPER_CTRL_PG(i)		(BIT(6) << (i) * 8)
> > > +#define  FIPER_CTRL_FS_ALARM(i)		(BIT(5) << (i) * 8)
> > > +#define  FIPER_CTRL_PW(i)		(GENMASK(4, 0) << (i) * 8)
> > > +#define  FIPER_CTRL_SET_PW(i, v)	(((v) & GENMASK(4, 0)) << 8 * (i))
> > >
> > >  #define NETC_TMR_CUR_TIME_L		0x00f0
> > >  #define NETC_TMR_CUR_TIME_H		0x00f4
> > > @@ -39,6 +56,9 @@
> > >
> > >  #define NETC_TMR_FIPER_NUM		3
> > >  #define NETC_TMR_DEFAULT_PRSC		2
> > > +#define NETC_TMR_DEFAULT_ALARM		GENMASK_ULL(63, 0)
> > > +#define NETC_TMR_DEFAULT_FIPER		GENMASK(31, 0)
> > > +#define NETC_TMR_FIPER_MAX_PW		GENMASK(4, 0)
> > >
> > >  /* 1588 timer reference clock source select */
> > >  #define NETC_TMR_CCM_TIMER1		0 /* enet_timer1_clk_root, from
> > CCM */
> > > @@ -60,6 +80,10 @@ struct netc_timer {
> > >  	u32 oclk_prsc;
> > >  	/* High 32-bit is integer part, low 32-bit is fractional part */
> > >  	u64 period;
> > > +
> > > +	int irq;
> > > +	u32 tmr_emask;
> > > +	bool pps_enabled;
> > >  };
> > >
> > >  #define netc_timer_rd(p, o)		netc_read((p)->base + (o))
> > > @@ -124,6 +148,155 @@ static u64 netc_timer_cur_time_read(struct
> > netc_timer *priv)
> > >  	return ns;
> > >  }
> > >
> > > +static void netc_timer_alarm_write(struct netc_timer *priv,
> > > +				   u64 alarm, int index)
> > > +{
> > > +	u32 alarm_h = upper_32_bits(alarm);
> > > +	u32 alarm_l = lower_32_bits(alarm);
> > > +
> > > +	netc_timer_wr(priv, NETC_TMR_ALARM_L(index), alarm_l);
> > > +	netc_timer_wr(priv, NETC_TMR_ALARM_H(index), alarm_h);
> > > +}
> > > +
> > > +static u32 netc_timer_get_integral_period(struct netc_timer *priv)
> > > +{
> > > +	u32 tmr_ctrl, integral_period;
> > > +
> > > +	tmr_ctrl = netc_timer_rd(priv, NETC_TMR_CTRL);
> > > +	integral_period = FIELD_GET(TMR_CTRL_TCLK_PERIOD, tmr_ctrl);
> > > +
> > > +	return integral_period;
> > > +}
> > > +
> > > +static u32 netc_timer_calculate_fiper_pw(struct netc_timer *priv,
> > > +					 u32 fiper)
> > > +{
> > > +	u64 divisor, pulse_width;
> > > +
> > > +	/* Set the FIPER pulse width to half FIPER interval by default.
> > > +	 * pulse_width = (fiper / 2) / TMR_GCLK_period,
> > > +	 * TMR_GCLK_period = NSEC_PER_SEC / TMR_GCLK_freq,
> > > +	 * TMR_GCLK_freq = (clk_freq / oclk_prsc) Hz,
> > > +	 * so pulse_width = fiper * clk_freq / (2 * NSEC_PER_SEC * oclk_prsc).
> > > +	 */
> > > +	divisor = mul_u32_u32(2 * NSEC_PER_SEC, priv->oclk_prsc);
> > > +	pulse_width = div64_u64(mul_u32_u32(fiper, priv->clk_freq), divisor);
> > > +
> > > +	/* The FIPER_PW field only has 5 bits, need to update oclk_prsc */
> > > +	if (pulse_width > NETC_TMR_FIPER_MAX_PW)
> > > +		pulse_width = NETC_TMR_FIPER_MAX_PW;
> > > +
> > > +	return pulse_width;
> > > +}
> > > +
> > > +static void netc_timer_set_pps_alarm(struct netc_timer *priv, int channel,
> > > +				     u32 integral_period)
> > > +{
> > > +	u64 alarm;
> > > +
> > > +	/* Get the alarm value */
> > > +	alarm = netc_timer_cur_time_read(priv) +  NSEC_PER_MSEC;
> > > +	alarm = roundup_u64(alarm, NSEC_PER_SEC);
> > > +	alarm = roundup_u64(alarm, integral_period);
> > > +
> > > +	netc_timer_alarm_write(priv, alarm, 0);
> > > +}
> > > +
> > > +/* Note that users should not use this API to output PPS signal on
> > > + * external pins, because PTP_CLK_REQ_PPS trigger internal PPS event
> > > + * for input into kernel PPS subsystem. See:
> > > + *
> > https://lore.kernel.org/r/20201117213826.18235-1-a.fatoum@pengutronix.de
> > > + */
> > > +static int netc_timer_enable_pps(struct netc_timer *priv,
> > > +				 struct ptp_clock_request *rq, int on)
> > > +{
> > > +	u32 fiper, fiper_ctrl;
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&priv->lock, flags);
> > > +
> > > +	fiper_ctrl = netc_timer_rd(priv, NETC_TMR_FIPER_CTRL);
> > > +
> > > +	if (on) {
> > > +		u32 integral_period, fiper_pw;
> > > +
> > > +		if (priv->pps_enabled)
> > > +			goto unlock_spinlock;
> > > +
> > > +		integral_period = netc_timer_get_integral_period(priv);
> > > +		fiper = NSEC_PER_SEC - integral_period;
> > > +		fiper_pw = netc_timer_calculate_fiper_pw(priv, fiper);
> > > +		fiper_ctrl &= ~(FIPER_CTRL_DIS(0) | FIPER_CTRL_PW(0) |
> > > +				FIPER_CTRL_FS_ALARM(0));
> > > +		fiper_ctrl |= FIPER_CTRL_SET_PW(0, fiper_pw);
> > > +		priv->tmr_emask |= TMR_TEVNET_PPEN(0) |
> > TMR_TEVENT_ALMEN(0);
> > > +		priv->pps_enabled = true;
> > > +		netc_timer_set_pps_alarm(priv, 0, integral_period);
> > > +	} else {
> > > +		if (!priv->pps_enabled)
> > > +			goto unlock_spinlock;
> > > +
> > > +		fiper = NETC_TMR_DEFAULT_FIPER;
> > > +		priv->tmr_emask &= ~(TMR_TEVNET_PPEN(0) |
> > > +				     TMR_TEVENT_ALMEN(0));
> > > +		fiper_ctrl |= FIPER_CTRL_DIS(0);
> > > +		priv->pps_enabled = false;
> > > +		netc_timer_alarm_write(priv, NETC_TMR_DEFAULT_ALARM, 0);
> > > +	}
> > > +
> > > +	netc_timer_wr(priv, NETC_TMR_TEMASK, priv->tmr_emask);
> > > +	netc_timer_wr(priv, NETC_TMR_FIPER(0), fiper);
> > > +	netc_timer_wr(priv, NETC_TMR_FIPER_CTRL, fiper_ctrl);
> > > +
> > > +unlock_spinlock:
> > > +	spin_unlock_irqrestore(&priv->lock, flags);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void netc_timer_disable_pps_fiper(struct netc_timer *priv)
> > > +{
> > > +	u32 fiper_ctrl;
> > > +
> > > +	if (!priv->pps_enabled)
> > > +		return;
> > > +
> > > +	fiper_ctrl = netc_timer_rd(priv, NETC_TMR_FIPER_CTRL);
> > > +	fiper_ctrl |= FIPER_CTRL_DIS(0);
> > > +	netc_timer_wr(priv, NETC_TMR_FIPER(0), NETC_TMR_DEFAULT_FIPER);
> > > +	netc_timer_wr(priv, NETC_TMR_FIPER_CTRL, fiper_ctrl);
> > > +}
> > > +
> > > +static void netc_timer_enable_pps_fiper(struct netc_timer *priv)
> > > +{
> > > +	u32 fiper_ctrl, integral_period, fiper;
> > > +
> > > +	if (!priv->pps_enabled)
> > > +		return;
> > > +
> > > +	integral_period = netc_timer_get_integral_period(priv);
> > > +	fiper_ctrl = netc_timer_rd(priv, NETC_TMR_FIPER_CTRL);
> > > +	fiper_ctrl &= ~FIPER_CTRL_DIS(0);
> > > +	fiper = NSEC_PER_SEC - integral_period;
> > > +
> > > +	netc_timer_set_pps_alarm(priv, 0, integral_period);
> > > +	netc_timer_wr(priv, NETC_TMR_FIPER(0), fiper);
> > > +	netc_timer_wr(priv, NETC_TMR_FIPER_CTRL, fiper_ctrl);
> > > +}
> > > +
> > > +static int netc_timer_enable(struct ptp_clock_info *ptp,
> > > +			     struct ptp_clock_request *rq, int on)
> > > +{
> > > +	struct netc_timer *priv = ptp_to_netc_timer(ptp);
> > > +
> > > +	switch (rq->type) {
> > > +	case PTP_CLK_REQ_PPS:
> > > +		return netc_timer_enable_pps(priv, rq, on);
> > > +	default:
> > > +		return -EOPNOTSUPP;
> > > +	}
> > > +}
> > > +
> > >  static void netc_timer_adjust_period(struct netc_timer *priv, u64 period)
> > >  {
> > >  	u32 fractional_period = lower_32_bits(period);
> > > @@ -136,8 +309,11 @@ static void netc_timer_adjust_period(struct
> > netc_timer *priv, u64 period)
> > >  	old_tmr_ctrl = netc_timer_rd(priv, NETC_TMR_CTRL);
> > >  	tmr_ctrl = u32_replace_bits(old_tmr_ctrl, integral_period,
> > >  				    TMR_CTRL_TCLK_PERIOD);
> > > -	if (tmr_ctrl != old_tmr_ctrl)
> > > +	if (tmr_ctrl != old_tmr_ctrl) {
> > > +		netc_timer_disable_pps_fiper(priv);
> > >  		netc_timer_wr(priv, NETC_TMR_CTRL, tmr_ctrl);
> > > +		netc_timer_enable_pps_fiper(priv);
> > > +	}
> > >
> > >  	netc_timer_wr(priv, NETC_TMR_ADD, fractional_period);
> > >
> > > @@ -163,6 +339,8 @@ static int netc_timer_adjtime(struct ptp_clock_info
> > *ptp, s64 delta)
> > >
> > >  	spin_lock_irqsave(&priv->lock, flags);
> > >
> > > +	netc_timer_disable_pps_fiper(priv);
> > > +
> > >  	/* Adjusting TMROFF instead of TMR_CNT is that the timer
> > >  	 * counter keeps increasing during reading and writing
> > >  	 * TMR_CNT, which will cause latency.
> > > @@ -171,6 +349,8 @@ static int netc_timer_adjtime(struct ptp_clock_info
> > *ptp, s64 delta)
> > >  	tmr_off += delta;
> > >  	netc_timer_offset_write(priv, tmr_off);
> > >
> > > +	netc_timer_enable_pps_fiper(priv);
> > > +
> > >  	spin_unlock_irqrestore(&priv->lock, flags);
> > >
> > >  	return 0;
> > > @@ -205,8 +385,12 @@ static int netc_timer_settime64(struct
> > ptp_clock_info *ptp,
> > >  	unsigned long flags;
> > >
> > >  	spin_lock_irqsave(&priv->lock, flags);
> > > +
> > > +	netc_timer_disable_pps_fiper(priv);
> > >  	netc_timer_offset_write(priv, 0);
> > >  	netc_timer_cnt_write(priv, ns);
> > > +	netc_timer_enable_pps_fiper(priv);
> > > +
> > >  	spin_unlock_irqrestore(&priv->lock, flags);
> > >
> > >  	return 0;
> > > @@ -232,10 +416,13 @@ static const struct ptp_clock_info
> > netc_timer_ptp_caps = {
> > >  	.name		= "NETC Timer PTP clock",
> > >  	.max_adj	= 500000000,
> > >  	.n_pins		= 0,
> > > +	.n_alarm	= 2,
> > > +	.pps		= 1,
> > >  	.adjfine	= netc_timer_adjfine,
> > >  	.adjtime	= netc_timer_adjtime,
> > >  	.gettimex64	= netc_timer_gettimex64,
> > >  	.settime64	= netc_timer_settime64,
> > > +	.enable		= netc_timer_enable,
> > >  };
> > >
> > >  static void netc_timer_init(struct netc_timer *priv)
> > > @@ -252,7 +439,7 @@ static void netc_timer_init(struct netc_timer *priv)
> > >  	 * domain are not accessible.
> > >  	 */
> > >  	tmr_ctrl = FIELD_PREP(TMR_CTRL_CK_SEL, priv->clk_select) |
> > > -		   TMR_CTRL_TE;
> > > +		   TMR_CTRL_TE | TMR_CTRL_FS;
> > >  	netc_timer_wr(priv, NETC_TMR_CTRL, tmr_ctrl);
> > >  	netc_timer_wr(priv, NETC_TMR_PRSC, priv->oclk_prsc);
> > >
> > > @@ -372,6 +559,66 @@ static int netc_timer_parse_dt(struct netc_timer
> > *priv)
> > >  	return netc_timer_get_reference_clk_source(priv);
> > >  }
> > >
> > > +static irqreturn_t netc_timer_isr(int irq, void *data)
> > > +{
> > > +	struct netc_timer *priv = data;
> > > +	struct ptp_clock_event event;
> > > +	u32 tmr_event;
> > > +
> > > +	spin_lock(&priv->lock);
> > > +
> > > +	tmr_event = netc_timer_rd(priv, NETC_TMR_TEVENT);
> > > +	tmr_event &= priv->tmr_emask;
> > > +	/* Clear interrupts status */
> > > +	netc_timer_wr(priv, NETC_TMR_TEVENT, tmr_event);
> > > +
> > > +	if (tmr_event & TMR_TEVENT_ALMEN(0))
> > > +		netc_timer_alarm_write(priv, NETC_TMR_DEFAULT_ALARM, 0);
> > > +
> > > +	if (tmr_event & TMR_TEVENT_PPEN_ALL) {
> > > +		event.type = PTP_CLOCK_PPS;
> > > +		ptp_clock_event(priv->clock, &event);
> > > +	}
> > > +
> > > +	spin_unlock(&priv->lock);
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int netc_timer_init_msix_irq(struct netc_timer *priv)
> > > +{
> > > +	struct pci_dev *pdev = priv->pdev;
> > > +	char irq_name[64];
> > > +	int err, n;
> > > +
> > > +	n = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX);
> > > +	if (n != 1) {
> > > +		err = (n < 0) ? n : -EPERM;
> > > +		dev_err(&pdev->dev, "pci_alloc_irq_vectors() failed\n");
> > > +		return err;
> > > +	}
> > > +
> > > +	priv->irq = pci_irq_vector(pdev, 0);
> > > +	snprintf(irq_name, sizeof(irq_name), "ptp-netc %s", pci_name(pdev));
> > > +	err = request_irq(priv->irq, netc_timer_isr, 0, irq_name, priv);
> > > +	if (err) {
> > > +		dev_err(&pdev->dev, "request_irq() failed\n");
> > > +		pci_free_irq_vectors(pdev);
> > > +		return err;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void netc_timer_free_msix_irq(struct netc_timer *priv)
> > > +{
> > > +	struct pci_dev *pdev = priv->pdev;
> > > +
> > > +	disable_irq(priv->irq);
> > > +	free_irq(priv->irq, priv);
> > > +	pci_free_irq_vectors(pdev);
> > > +}
> > > +
> > >  static int netc_timer_probe(struct pci_dev *pdev,
> > >  			    const struct pci_device_id *id)
> > >  {
> > > @@ -395,17 +642,23 @@ static int netc_timer_probe(struct pci_dev *pdev,
> > >  	priv->oclk_prsc = NETC_TMR_DEFAULT_PRSC;
> > >  	spin_lock_init(&priv->lock);
> > >
> > > +	err = netc_timer_init_msix_irq(priv);
> > > +	if (err)
> > > +		goto timer_pci_remove;
> > > +
> > >  	netc_timer_init(priv);
> > >  	priv->clock = ptp_clock_register(&priv->caps, dev);
> > >  	if (IS_ERR(priv->clock)) {
> > >  		err = PTR_ERR(priv->clock);
> > > -		goto timer_pci_remove;
> > > +		goto free_msix_irq;
> > >  	}
> > >
> > >  	priv->phc_index = ptp_clock_index(priv->clock);
> > >
> > >  	return 0;
> > >
> > > +free_msix_irq:
> > > +	netc_timer_free_msix_irq(priv);
> > >  timer_pci_remove:
> > >  	netc_timer_pci_remove(pdev);
> > >
> > > @@ -417,6 +670,7 @@ static void netc_timer_remove(struct pci_dev *pdev)
> > >  	struct netc_timer *priv = pci_get_drvdata(pdev);
> > >
> > >  	ptp_clock_unregister(priv->clock);
> > > +	netc_timer_free_msix_irq(priv);
> > >  	netc_timer_pci_remove(pdev);
> > >  }
> > >
> > > --
> > > 2.34.1
> > >

Powered by blists - more mailing lists