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

["G Thomas, Rohan" <rohan.g.thomas@altera.com>]

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@altera.com>
>>> ---
>>> 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@altera.com/
>>
>> 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