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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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.

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.

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!