Subject: Re: [PATCH RFC net-next 2/5] ptp: marvell: add core support for Marvell PTP v2.1

From: "Casper Andersson" <[email-masked]>
Date: Fri, 03 Oct 2025 15:09:18 +0200

Hi, I'm also late to the ball here.

On Fri Jun 13, 2025 at 5:19 PM CEST, Kory Maincent wrote:
> Hello Russell,
>
> Le Wed, 16 Apr 2025 10:22:47 +0100,
> "Russell King (Oracle)" <[email-masked]> a écrit :
>
>> On Wed, Apr 16, 2025 at 10:48:49AM +0200, Kory Maincent wrote:
>> > On Fri, 11 Apr 2025 22:26:37 +0100
>> > Russell King <[email-masked]> wrote:  
>> > > Provide core support for the Marvell PTP v2.1 implementations, which
>> > > consist of a TAI (time application interface) and timestamping blocks.
>> > > This hardware can be found in Marvell 88E151x PHYs, Armada 38x and
>> > > Armada 37xx (mvneta), as well as Marvell DSA devices.
>> > > 
>> > > Support for both arrival timestamps is supported, we use arrival 1 for
>> > > PTP peer delay messages, and arrival 0 for all other messages.
>> > > 
>> > > External event capture is also supported.
>> > > 
>> > > PPS output and trigger generation is not supported.
>> > > 
>> > > This core takes inspiration from the existing Marvell 88E6xxx DSA PTP
>> > > code and DP83640 drivers. Like the original 88E6xxx DSA code, we
>> > > use a delayed work to keep the cycle counter updated, and a separate
>> > > delayed work for event capture.
>> > > 
>> > > We expose the ptp clock aux work to allow users to support single and
>> > > multi-port designs - where there is one Marvell TAI instance and a
>> > > number of Marvell TS instances.  
>> > 
>> > ...
>> >   
>> > > +#define MV_PTP_MSGTYPE_DELAY_RESP	9
>> > > +
>> > > +/* This defines which incoming or outgoing PTP frames are timestampped */
>> > > +#define MV_PTP_MSD_ID_TS_EN	(BIT(PTP_MSGTYPE_SYNC) | \
>> > > +				 BIT(PTP_MSGTYPE_DELAY_REQ) | \
>> > > +				 BIT(MV_PTP_MSGTYPE_DELAY_RESP))
>> > > +/* Direct Sync messages to Arr0 and delay messages to Arr1 */
>> > > +#define MV_PTP_TS_ARR_PTR	(BIT(PTP_MSGTYPE_DELAY_REQ) | \
>> > > +				 BIT(MV_PTP_MSGTYPE_DELAY_RESP))  
>> > 
>> > Why did you have chosen to use two queues with two separate behavior?
>> > I have tried using only one queue and the PTP as master behaves correctly
>> > without all these overrun. It is way better with one queue.
>> > Maybe it was not the best approach if you want to use the two queues.    
>> 
>> First, both queues have the same behaviour.
>> 
>> Second, because they *aren't* queues as they can only stamp one message.
>> The sync messages come from the master on a regular basis. The delay
>> response messages come from the master in response to a delay request
>> message, the timing of which is determined by the local slave.
>> 
>> If the local end sends a delay request just at the point that the master
>> sends a sync message causing the master to immediately follow the sync
>> message with the delay response message, then we could get an overrun
>> on a single queue - because we'll stamp the sync message and if we don't
>> read the timestamp quickly enough, the stamp registers will be busy
>> preventing the timestamp of the delay response being captured.
>
> I just come back on this. I think you are wrong.
> We are using different registers to save timestamp for departure or arrival
> packets.
> If there is a sync message and a delay request message (the delay response is
> not timestamped) at the same time, the sync message timestamp will be save in
> the departure registers and the delay request timestamp in the arrival registers
> (or the contrary if the Marvell PTP is a follower). There is no possibility to
> loose the timestamp on that case, therefore using two registers for arrival
> packets is useless.

E2E timestamping should never have any collision on RX timestamps. But
P2P can. Sync, PdelayReq, and PdelayResp all need to be RX timestamped.
Running normally with ptp4l it worked pretty fine with PdelayResp on a
separate "queue". But Calnex Paragon-X (PTP tester) sends Sync and
PdelayReq almost simultaneously, causing collision. So there really
isn't a perfect solution to this. This was my experience from a while
back with Russell's original RFC from years ago.

Some variants of this PTP block (e.g. the 1512p and mv88e6390x I have
access to) have the ability to store the timestamp in the reserved2
field, which makes RX timestamps much simpler and less error-prone (but
they also support the full onestep offloading using Time Arrays which
requires a totally different implementation).

BR
Casper

>
> FYI, I just tested the patch series on the Espressobin which have a mv88e6xxx
> and the PTP worked well. 
>
> Do you now If you will have time to send a v2 or should I take the lead on
> this series? 
>
>> With the overruns that I've seen, they've always been on the second
>> "queue" and have always been for a sequence number several in the past
>> beyond the point that the overrun has been reported. However, the
>> packet which the sequence number matches had already been received -
>> and several others have also been received. I've been wondering if it's
>> a hardware bug, or maybe it's something other bits of the kernel is
>> doing wrong.
>
> I think we should drop this as I don't see anything relevant to use two
> "queues" except overrun cases.
>
> Regards,