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

[Kory Maincent <kory.maincent@bootlin.com>]

On Wed, 16 Apr 2025 10:22:47 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Wed, Apr 16, 2025 at 10:48:49AM +0200, Kory Maincent wrote:
> > On Fri, 11 Apr 2025 22:26:37 +0100
> > Russell King <rmk+kernel@armlinux.org.uk> 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.

Yes, I was not clear I was referring to the message type.
 
> 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.

Have you already seen such cases?

> 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.

Yes, in any case, using two queues like that prevents the PTP master from
working properly on my board. I will try to investigate the issue.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com