Re: [PATCH net] net: ethernet: ti: am65-cpts: keep port and direction when matching PTPv1 TX timestamps

[Siddharth Vadapalli <s-vadapalli@ti.com>]

On 10/30/26 6:49 PM, Jesper L. Nielsen via B4 Relay wrote:
> From: "Jesper L. Nielsen" <jln@pascal-audio.com>
> 
> The CPTS reports the message type of a timestamp event using the PTPv2
> header layout, i.e. the low nibble of the first header byte. In a PTPv1
> header that byte is the upper byte of versionPTP and always 0, so every
> PTPv1 event is reported as message type 0, while
> am65_skb_get_mtype_seqid() reads the PTPv1 message type from the control
> field. To make PTPv1 match at all, commit 1b9e743e923b ("net: ethernet:
> ti: am65-cpts: Fix PTPv1 message type on TX packets") overwrites the
> whole value compared against the skb with the skb's own value once the
> sequence ids are equal. That throws away the event type and the port
> number of the event as well, so a PTPv1 skb matches any TX event with
> the same sequence id on either port. For PTPv1 this defeats
> commit a23c657e332f ("net: ethernet: ti: am65-cpsw: Use also port number to identify timestamps").

nitpick: This could be wrapped as done for the previous instance above.
> The overwritten value also sticks for the rest of the TX queue walk.
> 
> Since every PTPv1 frame reads as message type 0, the hardware timestamps
> all PTPv1 frames to UDP ports 319 and 320, including Follow_Up and
> Delay_Resp, for which no skb waits. With a PTPv1 master on both ports
> whose sequence ids run in step (e.g. a Dante device with primary and

Does this mean there are two PTP grandmasters? Is CPSW configured in EMAC 
mode or Switch mode? Is the PTP application bound to both network 
interfaces? The reason for me asking this is that there is a single 
instance of CPTS and we can't have two PTP grandmasters simultaneously (if 
configured for EMAC mode). Could you please clarify the network topology 
in-case I have misunderstood it?

> secondary network started at the same time), the unclaimed TX event of
> Follow_Up N on one port is still on the event list when the other port
> queues Sync N, and am65_cpts_find_tx_ts() tries to match before it
> checks for expiry. Sync N then gets the transmit time of the other
> port's Follow_Up N, and the master announces a preciseOriginTimestamp up
> to one Sync interval early to every slave on that network.
> 
> For PTPv1 replace only the message type of the event with the skb's and
> keep its event type, port number and sequence id, and compute the value
> per skb.
> 
> Tested on AM62x with ti-linux-6.12.y and the port number commit backported:
> without this patch every boot in which both ports started PTP together
> (10 of 10) sent wrong Sync timestamps, with it none did (0 of 29).
> 
> The bug was tracked down, and this fix and changelog drafted, with the
> help of an AI coding assistant (Claude). I reviewed and tested it on
> hardware as described above.
> 
> Fixes: a23c657e332f ("net: ethernet: ti: am65-cpsw: Use also port number to identify timestamps")
> Signed-off-by: Jesper L. Nielsen <jln@pascal-audio.com>
> ---
>   drivers/net/ethernet/ti/am65-cpts.c | 32 ++++++++++++++++++++++++++------
>   1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
> index df5659ead7..e30fae0bcd 100644
> --- a/drivers/net/ethernet/ti/am65-cpts.c
> +++ b/drivers/net/ethernet/ti/am65-cpts.c
> @@ -788,6 +788,29 @@ static struct ptp_clock_info am65_ptp_info = {
>   	.do_aux_work	= am65_cpts_ts_work,
>   };
>   
> +/*
> + * The CPTS decodes the message type of an event using the PTPv2 header

nitpick: Unlike other subsystems, comments in netdev do not have an empty 
line at the start. So it should be changed to:
/* The CPTS decodes the message type of an event using the PTPv2 header
  * ...
  * ...
  */

Once the setup/topology is clear to me, I shall review the code below.

Regards,
Siddharth.

> + * layout. For PTPv1 that nibble is part of versionPTP and always reads 0,
> + * while am65_skb_get_mtype_seqid() takes the message type from the PTPv1
> + * control field. For PTPv1 take the message type from the skb, but keep
> + * the event type, port number and sequence id of the event so that an
> + * event only matches a packet of the same direction on the same port.
> + */
> +static u32 am65_cpts_event_mtype_seqid(struct am65_cpts_event *event,
> +				       u32 skb_mtype_seqid,
> +				       unsigned int ptp_class)
> +{
> +	u32 mtype_seqid = event->event1;
> +
> +	if (ptp_class & PTP_CLASS_V1) {
> +		mtype_seqid &= ~AM65_CPTS_EVENT_1_MESSAGE_TYPE_MASK;
> +		mtype_seqid |= skb_mtype_seqid &
> +			       AM65_CPTS_EVENT_1_MESSAGE_TYPE_MASK;
> +	}
> +
> +	return mtype_seqid;
> +}
> +
>   static bool am65_cpts_match_tx_ts(struct am65_cpts *cpts,
>   				  struct am65_cpts_event *event)
>   {
> @@ -797,7 +820,6 @@ static bool am65_cpts_match_tx_ts(struct am65_cpts *cpts,
>   	bool found = false;
>   	u32 mtype_seqid;
>   
> -	mtype_seqid = event->event1;
>   	__skb_queue_head_init(&txq_list);
>   
>   	spin_lock_irqsave(&cpts->txq.lock, flags);
> @@ -810,11 +832,9 @@ static bool am65_cpts_match_tx_ts(struct am65_cpts *cpts,
>   		struct am65_cpts_skb_cb_data *skb_cb =
>   					(struct am65_cpts_skb_cb_data *)skb->cb;
>   
> -		if ((ptp_classify_raw(skb) & PTP_CLASS_V1) &&
> -		    ((mtype_seqid & AM65_CPTS_EVENT_1_SEQUENCE_ID_MASK) ==
> -		     (skb_cb->skb_mtype_seqid & AM65_CPTS_EVENT_1_SEQUENCE_ID_MASK)))
> -			mtype_seqid = skb_cb->skb_mtype_seqid;
> -
> +		mtype_seqid = am65_cpts_event_mtype_seqid(event,
> +							  skb_cb->skb_mtype_seqid,
> +							  ptp_classify_raw(skb));
>   		if (mtype_seqid == skb_cb->skb_mtype_seqid) {
>   			u64 ns = event->timestamp;
>   
> 
> ---
> base-commit: 5838f10563d58afca6711edb5e4eda968ca181d9
> change-id: 20261030-am65-cpts-ptpv1-45cc3c7276d2
> 
> Best regards,
> --
> Jesper L. Nielsen <jln@pascal-audio.com>