[PATCH net] net: ethernet: ti: am65-cpts: adjust perout genf following ptp changes

[PATCH net] net: ethernet: ti: am65-cpts: adjust perout genf following ptp changes

[James Clark]

A periodic output requested by PTP_PEROUT_REQUEST2 drifts away from
the PHC as soon as the PHC frequency is adjusted, for example by ptp4l
or ts2phc: at +10 ppm each edge is 10 us later than the one before.

The GenF that generates the output counts its period in ref_clk
cycles, which the PPM correction of the CPTS clock does not reach;
each GenF has its own PPM registers. am65_cpts_ptp_adjfine() programs
them only for the GenF used for PPS, and am65_cpts_perout_enable_hw()
starts every GenF with them cleared, so a GenF used for a periodic
output is never corrected.

Commit eb9233ce6751 ("net: ethernet: ti: am65-cpts: adjust pps following
ptp changes") added the correction for the PPS GenF, and
commit 7849c42da2ca ("net: ethernet: ti: am65-cpts: adjust estf following
ptp changes") passed the same values to the ESTFs.

In am65_cpts_ptp_adjfine(), update every enabled GenF the way the PPS
GenF was updated: read its control register, set or clear the PPM
direction bit and write it back with the PPM value. The PPS GenF and the
ESTFs get exactly the values they got before. In
am65_cpts_perout_enable_hw(), start a GenF with the current correction
instead of none; this applies to the PPS GenF too.

Tested on a BeagleY-AI (J722S) with the GENF0 output looped back to
the HW1_TS_PUSH input (EXTTS 0). With the PHC frequency adjusted by
+10 ppm before the periodic output is started, the edges drift 10 us
per second against the PHC without this patch (290 us after 29 s);
with it, they stay 20 to 23 ns after the PHC second. This also works
if the frequency is adjusted after the periodic output has started.

Fixes: f6bd59526ca5 ("net: ethernet: ti: introduce am654 common platform time sync driver")
Signed-off-by: James Clark <jjc@jclark.com>
---
Test details: testptp from tools/testing/selftests/ptp. Before is the
stock BeagleBoard 7.2.9 kernel; after is this patch on net/main.

  testptp -d /dev/ptp0 -f 10000
  testptp -d /dev/ptp0 -i 0 -p 1000000000
  testptp -d /dev/ptp0 -i 0 -e 30

Before:
  event index 0 at 32.000000023
  event index 0 at 33.000010023
  event index 0 at 34.000020023
  ...
  event index 0 at 61.000290023

After:
  event index 0 at 33.000000023
  event index 0 at 34.000000021
  event index 0 at 35.000000020
  ...
  event index 0 at 62.000000022

 drivers/net/ethernet/ti/am65-cpts.c | 67 +++++++++++++++++++----------
 1 file changed, 45 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
index df5659ead72f..0d71147f3c29 100644
--- a/drivers/net/ethernet/ti/am65-cpts.c
+++ b/drivers/net/ethernet/ti/am65-cpts.c
@@ -176,6 +176,9 @@ struct am65_cpts {
 	struct mutex ptp_clk_lock; /* PHC access sync */
 	u64 timestamp;
 	u32 genf_enable;
+	bool genf_neg_adj;
+	u32 genf_ppm_hi;
+	u32 genf_ppm_low;
 	u32 hw_ts_enable;
 	u32 estf_enable;
 	struct sk_buff_head txq;
@@ -436,9 +439,11 @@ static int am65_cpts_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 {
 	struct am65_cpts *cpts = container_of(ptp, struct am65_cpts, ptp_info);
 	u32 estf_ctrl_val = 0, estf_ppm_hi = 0, estf_ppm_low = 0;
+	u32 genf_ctrl_val[AM65_CPTS_GENF_MAX_NUM];
+	u32 genf_ppm_hi, genf_ppm_low;
 	s32 ppb = scaled_ppm_to_ppb(scaled_ppm);
 	int pps_index = cpts->pps_genf_idx;
-	u64 adj_period, pps_adj_period;
+	u64 adj_period, genf_adj_period;
 	u32 ctrl_val, ppm_hi, ppm_low;
 	unsigned long flags;
 	int neg_adj = 0, i;
@@ -470,20 +475,33 @@ static int am65_cpts_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 	ppm_hi = upper_32_bits(adj_period) & 0x3FF;
 	ppm_low = lower_32_bits(adj_period);
 
+	/* GenF PPM will do correction using cpts refclk tick which is
+	 * (cpts->ts_add_val + 1) ns, so GenF length PPM adj period
+	 * need to be corrected.
+	 */
+	genf_adj_period = adj_period * (cpts->ts_add_val + 1);
+	genf_ppm_hi = upper_32_bits(genf_adj_period) & 0x3FF;
+	genf_ppm_low = lower_32_bits(genf_adj_period);
+	cpts->genf_neg_adj = neg_adj;
+	cpts->genf_ppm_hi = genf_ppm_hi;
+	cpts->genf_ppm_low = genf_ppm_low;
+
+	for (i = 0; i < AM65_CPTS_GENF_MAX_NUM; i++) {
+		if (cpts->genf_enable & BIT(i)) {
+			genf_ctrl_val[i] = am65_cpts_read32(cpts,
+							    genf[i].control);
+			if (neg_adj)
+				genf_ctrl_val[i] &= ~BIT(1);
+			else
+				genf_ctrl_val[i] |= BIT(1);
+		}
+	}
+
 	if (cpts->pps_enabled) {
-		estf_ctrl_val = am65_cpts_read32(cpts, genf[pps_index].control);
-		if (neg_adj)
-			estf_ctrl_val &= ~BIT(1);
-		else
-			estf_ctrl_val |= BIT(1);
-
-		/* GenF PPM will do correction using cpts refclk tick which is
-		 * (cpts->ts_add_val + 1) ns, so GenF length PPM adj period
-		 * need to be corrected.
-		 */
-		pps_adj_period = adj_period * (cpts->ts_add_val + 1);
-		estf_ppm_hi = upper_32_bits(pps_adj_period) & 0x3FF;
-		estf_ppm_low = lower_32_bits(pps_adj_period);
+		/* am65_cpts_perout_enable_hw() sets pps_index in genf_enable */
+		estf_ctrl_val = genf_ctrl_val[pps_index];
+		estf_ppm_hi = genf_ppm_hi;
+		estf_ppm_low = genf_ppm_low;
 	}
 
 	spin_lock_irqsave(&cpts->lock, flags);
@@ -500,10 +518,13 @@ static int am65_cpts_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 	am65_cpts_write32(cpts, ppm_hi, ts_ppm_hi);
 	am65_cpts_write32(cpts, ppm_low, ts_ppm_low);
 
-	if (cpts->pps_enabled) {
-		am65_cpts_write32(cpts, estf_ctrl_val, genf[pps_index].control);
-		am65_cpts_write32(cpts, estf_ppm_hi, genf[pps_index].ppm_hi);
-		am65_cpts_write32(cpts, estf_ppm_low, genf[pps_index].ppm_low);
+	for (i = 0; i < AM65_CPTS_GENF_MAX_NUM; i++) {
+		if (cpts->genf_enable & BIT(i)) {
+			am65_cpts_write32(cpts, genf_ctrl_val[i],
+					  genf[i].control);
+			am65_cpts_write32(cpts, genf_ppm_hi, genf[i].ppm_hi);
+			am65_cpts_write32(cpts, genf_ppm_low, genf[i].ppm_low);
+		}
 	}
 
 	for (i = 0; i < AM65_CPTS_ESTF_MAX_NUM; i++) {
@@ -513,7 +534,6 @@ static int am65_cpts_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 			am65_cpts_write32(cpts, estf_ppm_low, estf[i].ppm_low);
 		}
 	}
-	/* All GenF/EstF can be updated here the same way */
 	spin_unlock_irqrestore(&cpts->lock, flags);
 
 	mutex_unlock(&cpts->ptp_clk_lock);
@@ -679,9 +699,12 @@ static void am65_cpts_perout_enable_hw(struct am65_cpts *cpts,
 		val = lower_32_bits(cycles);
 		am65_cpts_write32(cpts, val, genf[req->index].length);
 
-		am65_cpts_write32(cpts, 0, genf[req->index].control);
-		am65_cpts_write32(cpts, 0, genf[req->index].ppm_hi);
-		am65_cpts_write32(cpts, 0, genf[req->index].ppm_low);
+		am65_cpts_write32(cpts, cpts->genf_neg_adj ? 0 : BIT(1),
+				  genf[req->index].control);
+		am65_cpts_write32(cpts, cpts->genf_ppm_hi,
+				  genf[req->index].ppm_hi);
+		am65_cpts_write32(cpts, cpts->genf_ppm_low,
+				  genf[req->index].ppm_low);
 
 		cpts->genf_enable |= BIT(req->index);
 	} else {
-- 
2.56.0

Re: [PATCH net] net: ethernet: ti: am65-cpts: adjust perout genf following ptp changes

[Jacob Keller]

On 10/29/2026 6:55 AM, James Clark wrote:
> A periodic output requested by PTP_PEROUT_REQUEST2 drifts away from
> the PHC as soon as the PHC frequency is adjusted, for example by ptp4l
> or ts2phc: at +10 ppm each edge is 10 us later than the one before.
> 
> The GenF that generates the output counts its period in ref_clk
> cycles, which the PPM correction of the CPTS clock does not reach;
> each GenF has its own PPM registers. am65_cpts_ptp_adjfine() programs
> them only for the GenF used for PPS, and am65_cpts_perout_enable_hw()
> starts every GenF with them cleared, so a GenF used for a periodic
> output is never corrected.
> 
> Commit eb9233ce6751 ("net: ethernet: ti: am65-cpts: adjust pps following
> ptp changes") added the correction for the PPS GenF, and
> commit 7849c42da2ca ("net: ethernet: ti: am65-cpts: adjust estf following
> ptp changes") passed the same values to the ESTFs.
> 
> In am65_cpts_ptp_adjfine(), update every enabled GenF the way the PPS
> GenF was updated: read its control register, set or clear the PPM
> direction bit and write it back with the PPM value. The PPS GenF and the
> ESTFs get exactly the values they got before. In
> am65_cpts_perout_enable_hw(), start a GenF with the current correction
> instead of none; this applies to the PPS GenF too.
> 
> Tested on a BeagleY-AI (J722S) with the GENF0 output looped back to
> the HW1_TS_PUSH input (EXTTS 0). With the PHC frequency adjusted by
> +10 ppm before the periodic output is started, the edges drift 10 us
> per second against the PHC without this patch (290 us after 29 s);
> with it, they stay 20 to 23 ns after the PHC second. This also works
> if the frequency is adjusted after the periodic output has started.
> 

Makes sense. My only concern is how long does it take to finish the
adjustment to fix the PPS? Could that impact the rate at which you can
perform frequency adjustments? In typical deployments, frequency
adjustments happen at every PTP sync event, which could be many times
per second.


Also, is there any loss inbetween when the correction comes to each GenF
that could still create (a much smaller) inaccuracy between them that
compounds over time with each frequency adjustment? Or does this updated
correction keep everything aligned by the next pulse?

Obviously this is still better than what was happening before where the
corrections weren't even applied.

> Fixes: f6bd59526ca5 ("net: ethernet: ti: introduce am654 common platform time sync driver")
> Signed-off-by: James Clark <jjc@jclark.com>
> ---
> Test details: testptp from tools/testing/selftests/ptp. Before is the
> stock BeagleBoard 7.2.9 kernel; after is this patch on net/main.
> 
>   testptp -d /dev/ptp0 -f 10000
>   testptp -d /dev/ptp0 -i 0 -p 1000000000
>   testptp -d /dev/ptp0 -i 0 -e 30
> 
> Before:
>   event index 0 at 32.000000023
>   event index 0 at 33.000010023
>   event index 0 at 34.000020023
>   ...
>   event index 0 at 61.000290023
> 
> After:
>   event index 0 at 33.000000023
>   event index 0 at 34.000000021
>   event index 0 at 35.000000020
>   ...
>   event index 0 at 62.000000022
> 
>  drivers/net/ethernet/ti/am65-cpts.c | 67 +++++++++++++++++++----------
>  1 file changed, 45 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
> index df5659ead72f..0d71147f3c29 100644
> --- a/drivers/net/ethernet/ti/am65-cpts.c
> +++ b/drivers/net/ethernet/ti/am65-cpts.c
> @@ -176,6 +176,9 @@ struct am65_cpts {
>  	struct mutex ptp_clk_lock; /* PHC access sync */
>  	u64 timestamp;
>  	u32 genf_enable;
> +	bool genf_neg_adj;
> +	u32 genf_ppm_hi;
> +	u32 genf_ppm_low;
>  	u32 hw_ts_enable;
>  	u32 estf_enable;
>  	struct sk_buff_head txq;
> @@ -436,9 +439,11 @@ static int am65_cpts_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
>  {
>  	struct am65_cpts *cpts = container_of(ptp, struct am65_cpts, ptp_info);
>  	u32 estf_ctrl_val = 0, estf_ppm_hi = 0, estf_ppm_low = 0;
> +	u32 genf_ctrl_val[AM65_CPTS_GENF_MAX_NUM];
> +	u32 genf_ppm_hi, genf_ppm_low;
>  	s32 ppb = scaled_ppm_to_ppb(scaled_ppm);
>  	int pps_index = cpts->pps_genf_idx;
> -	u64 adj_period, pps_adj_period;
> +	u64 adj_period, genf_adj_period;
>  	u32 ctrl_val, ppm_hi, ppm_low;
>  	unsigned long flags;
>  	int neg_adj = 0, i;
> @@ -470,20 +475,33 @@ static int am65_cpts_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
>  	ppm_hi = upper_32_bits(adj_period) & 0x3FF;
>  	ppm_low = lower_32_bits(adj_period);
>  
> +	/* GenF PPM will do correction using cpts refclk tick which is
> +	 * (cpts->ts_add_val + 1) ns, so GenF length PPM adj period
> +	 * need to be corrected.
> +	 */
> +	genf_adj_period = adj_period * (cpts->ts_add_val + 1);
> +	genf_ppm_hi = upper_32_bits(genf_adj_period) & 0x3FF;
> +	genf_ppm_low = lower_32_bits(genf_adj_period);
> +	cpts->genf_neg_adj = neg_adj;
> +	cpts->genf_ppm_hi = genf_ppm_hi;
> +	cpts->genf_ppm_low = genf_ppm_low;
> +
> +	for (i = 0; i < AM65_CPTS_GENF_MAX_NUM; i++) {
> +		if (cpts->genf_enable & BIT(i)) {
> +			genf_ctrl_val[i] = am65_cpts_read32(cpts,
> +							    genf[i].control);
> +			if (neg_adj)
> +				genf_ctrl_val[i] &= ~BIT(1);
> +			else
> +				genf_ctrl_val[i] |= BIT(1);
> +		}
> +	}
> +
>  	if (cpts->pps_enabled) {
> -		estf_ctrl_val = am65_cpts_read32(cpts, genf[pps_index].control);
> -		if (neg_adj)
> -			estf_ctrl_val &= ~BIT(1);
> -		else
> -			estf_ctrl_val |= BIT(1);
> -
> -		/* GenF PPM will do correction using cpts refclk tick which is
> -		 * (cpts->ts_add_val + 1) ns, so GenF length PPM adj period
> -		 * need to be corrected.
> -		 */
> -		pps_adj_period = adj_period * (cpts->ts_add_val + 1);
> -		estf_ppm_hi = upper_32_bits(pps_adj_period) & 0x3FF;
> -		estf_ppm_low = lower_32_bits(pps_adj_period);
> +		/* am65_cpts_perout_enable_hw() sets pps_index in genf_enable */
> +		estf_ctrl_val = genf_ctrl_val[pps_index];
> +		estf_ppm_hi = genf_ppm_hi;
> +		estf_ppm_low = genf_ppm_low;
>  	}
>  
>  	spin_lock_irqsave(&cpts->lock, flags);
> @@ -500,10 +518,13 @@ static int am65_cpts_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
>  	am65_cpts_write32(cpts, ppm_hi, ts_ppm_hi);
>  	am65_cpts_write32(cpts, ppm_low, ts_ppm_low);
>  
> -	if (cpts->pps_enabled) {
> -		am65_cpts_write32(cpts, estf_ctrl_val, genf[pps_index].control);
> -		am65_cpts_write32(cpts, estf_ppm_hi, genf[pps_index].ppm_hi);
> -		am65_cpts_write32(cpts, estf_ppm_low, genf[pps_index].ppm_low);
> +	for (i = 0; i < AM65_CPTS_GENF_MAX_NUM; i++) {
> +		if (cpts->genf_enable & BIT(i)) {
> +			am65_cpts_write32(cpts, genf_ctrl_val[i],
> +					  genf[i].control);
> +			am65_cpts_write32(cpts, genf_ppm_hi, genf[i].ppm_hi);
> +			am65_cpts_write32(cpts, genf_ppm_low, genf[i].ppm_low);
> +		}
>  	}
>  
>  	for (i = 0; i < AM65_CPTS_ESTF_MAX_NUM; i++) {
> @@ -513,7 +534,6 @@ static int am65_cpts_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
>  			am65_cpts_write32(cpts, estf_ppm_low, estf[i].ppm_low);
>  		}
>  	}
> -	/* All GenF/EstF can be updated here the same way */
>  	spin_unlock_irqrestore(&cpts->lock, flags);
>  
>  	mutex_unlock(&cpts->ptp_clk_lock);
> @@ -679,9 +699,12 @@ static void am65_cpts_perout_enable_hw(struct am65_cpts *cpts,
>  		val = lower_32_bits(cycles);
>  		am65_cpts_write32(cpts, val, genf[req->index].length);
>  
> -		am65_cpts_write32(cpts, 0, genf[req->index].control);
> -		am65_cpts_write32(cpts, 0, genf[req->index].ppm_hi);
> -		am65_cpts_write32(cpts, 0, genf[req->index].ppm_low);
> +		am65_cpts_write32(cpts, cpts->genf_neg_adj ? 0 : BIT(1),
> +				  genf[req->index].control);
> +		am65_cpts_write32(cpts, cpts->genf_ppm_hi,
> +				  genf[req->index].ppm_hi);
> +		am65_cpts_write32(cpts, cpts->genf_ppm_low,
> +				  genf[req->index].ppm_low);
>  
>  		cpts->genf_enable |= BIT(req->index);
>  	} else {

Re: [PATCH net] net: ethernet: ti: am65-cpts: adjust perout genf following ptp changes

[James Clark]

On 10/29/26 22:57, Jacob Keller wrote:
> Makes sense. My only concern is how long does it take to finish the
> adjustment to fix the PPS? Could that impact the rate at which you can
> perform frequency adjustments? In typical deployments, frequency
> adjustments happen at every PTP sync event, which could be many times
> per second.

Hi Jacob, thanks for taking a look at this.

The register reads and writes happen over the SoC interconnect and so
are fast: I timed clock_adjtime(ADJ_FREQUENCY) on a BeagleY-AI and each
enabled GenF adds 0.84 us. I also tested with PTP sync events at 128
syncs per second, and it worked fine.

> Also, is there any loss inbetween when the correction comes to each GenF
> that could still create (a much smaller) inaccuracy between them that
> compounds over time with each frequency adjustment? Or does this updated
> correction keep everything aligned by the next pulse?

There is an issue here with how the hardware works, which I think means
that it is not possible to guarantee that the periodic output is
absolutely in sync with the PHC in all situations. The hardware has two
separate counters: a timestamp counter used by the PHC and a GenF
counter used by the periodic output. Each of these has its own PPM
counter used to apply frequency corrections. Each PPM counter is a
down-counter: it loads the period from the PPM registers, counts down to
zero, makes a one-tick adjustment in the direction set at that moment,
and then loads the period again (as described for GenF in
12.2.1.4.7.11.2 of the TRM). adjfine() just writes the PPM registers;
the hardware does not give access to the PPM counters. The PPM counters
are not in sync, so they can sample the frequency from their
corresponding registers at different times, and the corrections applied
to the timestamp and GenF counters can accordingly be different.

I tested that the hardware behaves consistently with the TRM. As before,
the GENF0 output was looped back to EXTTS 0. Writing a constant
frequency every 10 ms, there is no drift. But by writing a frequency
that alternates between +0.1 and -0.1 ppm every 10 ms, I was able to
produce a drift of 100 ns/s.

But in practice, this does not seem to be a significant problem, at
least on my particular board. I tested with ptp4l locked to a
GPS-disciplined grandmaster, at 1, 16 and 128 syncs a second, for 60, 10
and 5 minutes, and did not see any drift. I should mention though that
this board's crystal is 10 ppm slow, so the adjustment is large and the
PPM counter periods correspondingly short. The results might be worse on
a board with a more accurate crystal.

In any case, as far as I can see this is how the hardware is designed to
work, and I think this patch makes it work as designed.

> Obviously this is still better than what was happening before where the
> corrections weren't even applied.

Indeed.

James