Re: [PATCH net v2] net: stmmac: Fix E2E delay mechanism

[G Thomas, Rohan]

Hi Russell,

Thanks for reviewing the patch.

On 12/4/2025 4:49 PM, Russell King (Oracle) wrote:
> On Thu, Dec 04, 2025 at 10:58:40AM +0100, Paolo Abeni wrote:
> > On 11/29/25 4:07 AM, Rohan G Thomas wrote:
> > > For E2E delay mechanism, "received DELAY_REQ without timestamp" error
> > > messages show up for dwmac v3.70+ and dwxgmac IPs.
> > >
> > > This issue affects socfpga platforms, Agilex7 (dwmac 3.70) and
> > > Agilex5 (dwxgmac). According to the databook, to enable timestamping
> > > for all events, the SNAPTYPSEL bits in the MAC_Timestamp_Control
> > > register must be set to 2'b01, and the TSEVNTENA bit must be cleared
> > > to 0'b0.
> > >
> > > Commit 3cb958027cb8 ("net: stmmac: Fix E2E delay mechanism") already
> > > addresses this problem for all dwmacs above version v4.10. However,
> > > same holds true for v3.70 and above, as well as for dwxgmac. Updates
> > > the check accordingly.
> > >
> > > Fixes: 14f347334bf2 ("net: stmmac: Correctly take timestamp for PTPv2")
> > > Fixes: f2fb6b6275eb ("net: stmmac: enable timestamp snapshot for required PTP packets in dwmac v5.10a")
> > > Fixes: 3cb958027cb8 ("net: stmmac: Fix E2E delay mechanism")
> > > Signed-off-by: Rohan G Thomas <rohan.g.thomas@xxxxxxxxxx>
> > > ---
> > > v1 -> v2:
> > >     - Rebased patch to net tree
> > >     - Replace core_type with has_xgmac
> > >     - Nit changes in the commit message
> > >     - Link: https://lore.kernel.org/all/20251125-ext-ptp-fix-v1-1-83f9f069cb36@xxxxxxxxxx/
> >
> > Given there is some uncertain WRT the exact oldest version to be used,
> > it would be great to have some 3rd party testing/feedback on this. Let's
> > wait a little more.
>
> As I said, in the v3.74 documentation, it is stated that the SNAPTYPSEL
> functions changed between v3.50 and v3.60, so I think it would be better
> to propose a patch to test for < v3.6.

I tested this on socfpga platforms like Agilex7 which are using
3.7x, but don't have any platforms with dwmac <= v3.6.

> Alternatively, if someone has the pre-v3.6 databook to check what the
> SNAPTYPSEL definition is and compare it with the v3.6+ definition, that
> would also be a good thing to do.
>
>  From the 3.74:
>
> SNAPTYPSEL
> 00		?
> 01		?
> 10		Sync, Delay_Req
> 11		Sync, PDelay_Req, PDelay_Resp
>
> TSEVNTENA
> 0		All messages except Announce, Management and Signalling
> 1		Sync, Delay_Req, PDelay_Req, PDelay_Resp
>
> No table is provided, so it's difficult to know what all the bit
> combinations do for v3.74.

In 3.73a databook, Table 6-70 has the following information and this is
similar to v5.1 and v5.3. But don't have 3.74 databook.

SNAPTYPSEL	TSMSTRENA	TSEVNTENA	PTP Messages
00		X 		0 		SYNC, Follow_Up,
						Delay_Req, Delay_Resp
00 		0 		1 		SYNC
00 		1 		1 		Delay_Req
01 		X 		0 		SYNC, Follow_Up,
						Delay_Req, Delay_Resp,
						Pdelay_Req, Pdelay_Resp,
						Pdelay_Resp_Follow_Up
01 		0 		1 		SYNC, Pdelay_Req,
						Pdelay_Resp
01 		1 		1 		Delay_Req, Pdelay_Req,
						Pdelay_Resp
10 		X 		X 		SYNC, Delay_Req
11 		X 		X 		Pdelay_Req, Pdelay_Resp

>  From STM32MP151 documentation (v4.2 according to GMAC4_VERSION
> register):
>
> SNAPTYPSEL	TSMSTRENA	TSEVNTENA
> 00		x		0		Sync, Delay_Req
> 00		0		1		Delay_Req
> 00		1		1		Sync
> 01		x		0		Sync, PDelay_Req, PDelay_Resp
> 01		0		1		Sync, Delay_Req, PDelay_Req,
> 						PDelay_Resp
> 01		1		1		Sync, PDelay_Req, PDelay_Resp
> 10		x		x		Sync, Delay_Req
> 11		x		x		Sync, PDelay_Req, PDelay_Resp
>
> For iMX8MP (v5.1) and STM32MP23/25xx (v5.3) documentatiion:
>
> SNAPTYPSEL	TSMSTRENA	TSEVNTENA
> 00		x		0		Sync, Follow_Up, Delay_Req,
> 						Delay_Resp
> 00		0		1		Sync
> 00		1		1		Delay_Req
> 01		x		0		Sync, Follow_Up, Delay_Req,
> 						Delay_Resp, PDelay_Req,
> 						PDelay_Resp
> 01		0		1		Sync, PDelay_Req, PDelay_Resp
> 01		1		1		Delay_Req, PDelay_Req,
> 						PDelay_Resp
> 10		x		x		Sync, Delay_Req
> 11		x		x		PDelay_Req, PDelay_Resp
>
> Differences:
> 00 x 0 - adds Follow_Up
> 00 X 1 - TSMSTRENA bit inverted
> 01 x 0 - adds Follow_Up, Delay_Req, Delay_Resp
> 01 0 1 - removes Delay_Req
> 01 1 1 - removes Sync, adds Delay_Req
> 11 x x - removes Sync
>
> So, it looks like there's another difference between v4.2 and v5.1.
>
> If the STM32MP151 (v4.2) documentation is correct, then from what I see
> in the driver, if HWTSTAMP_FILTER_PTP_V1_L4_SYNC is requested, we set
> SNAPTYPSEL=00 TSMSTRENA=0 TSEVNTENA=1, which semects Delay_Req messages
> only, but on iMX8MP this selects Sync messages.
>
> HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ is the opposite (due to the
> inversion of TSMSTRENA) for SNAPTYPSEL=00.
>
> For HWTSTAMP_FILTER_PTP_V2_EVENT, we currently set SNAPTYPSEL=01
> TSMSTRENA=0 and TSEVNTENA=1 for cores < v4.1:
> - For STM32MP151 (v4.2) we get Sync, PDelay_Req, PDelay_Resp but
>    _not_ Delay_Req. Seems broken.
> - For iMX8MP (v5.1) and STM32MP23/25xx (v5.3), we get
>    Sync, Follow_Up, Delay_Req, Delay_Resp, PDelay_Req, PDelay_Resp
>
> Basically, the conclusion I am coming to is that Synopsys's idea
> of "lets tell the hardware what _kind_ of PTP clock we want to be,
> whether we're master, etc" is subject to multiple revisions in
> terms of which messages each mode selects, and it would have been
> _far_ simpler and easier to understand had they just provided a
> 16-bit bitfield of message types to accept.
>
> So, I'm wary about this change - I think there's more "mess"
> here than just that single version check in
> HWTSTAMP_FILTER_PTP_V2_EVENT, I think it's a lot more complicated.
> I'm not sure what the best solution is right now, because I don't
> have the full information, but it looks to me like the current
> approach does not result in the expected configuration for each
> of the dwmac core versions, and there are multiple issues here.

Best Regards,
Rohan