Re: [PATCH RFC net-next 3/5] net: phy: add Marvell PHY PTP support

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

On Mon, 14 Apr 2025 15:55:09 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Mon, Apr 14, 2025 at 04:43:14PM +0200, Kory Maincent wrote:
> > On Fri, 11 Apr 2025 22:26:42 +0100
> > Russell King <rmk+kernel@armlinux.org.uk> wrote:
> >   
> > > Add PTP basic support for Marvell 88E151x single port PHYs.  These
> > > PHYs support timestamping the egress and ingress of packets, but does
> > > not support any packet modification, nor do we support any filtering
> > > beyond selecting packets that the hardware recognises as PTP/802.1AS.
> > > 
> > > The PHYs support hardware pins for providing an external clock for the
> > > TAI counter, and a separate pin that can be used for event capture or
> > > generation of a trigger (either a pulse or periodic). Only event
> > > capture is supported.
> > > 
> > > We currently use a delayed work to poll for the timestamps which is
> > > far from ideal, but we also provide a function that can be called from
> > > an interrupt handler - which would be good to tie into the main Marvell
> > > PHY driver.
> > > 
> > > The driver takes inspiration from the Marvell 88E6xxx DSA and DP83640
> > > drivers. The hardware is very similar to the implementation found in
> > > the 88E6xxx DSA driver, but the access methods are very different,
> > > although it may be possible to create a library that both can use
> > > along with accessor functions.  
> > 
> > It seems a lot less stable than the first version of the driver.
> > 
> > Lots of overrun:
> > ptp4l[949.894]: port 1 (eth0): assuming the grand master role
> > [  954.899275] Marvell 88E1510 ff0d0000.ethernet-ffffffff:01: tx timestamp
> > overrun (stat=0x5 seq=0) [  954.908670] Marvell 88E1510
> > ff0d0000.ethernet-ffffffff:01: rx timestamp overrun (q=1 stat=0x5 seq=0)  
> 
> I've explained why this happens - it will occur if timestamping has
> been enabled but there's been no packets to be stamped for a while
> but there _have_ been packets on the network that the hardware
> decided to stamp. There is no way around this because the driver isn't
> told when to shutdown timestamping.

Yes but I didn't faced this on your first version. Maybe there is a way to
avoid this.
I have not yet dive into your code to check the changes but I will.
 
> > PTP L2 in master mode is not working at all maybe because there is no
> > configuration of PTP global config 1 register.  
> 
> No, sorry, wrong. There is, and you haven't read what I've written
> previously if you're still thinking this.
> 
> Look in "ptp: marvell: add core support for Marvell PTP v2.1".
> 
> Look at marvell_tai_global_config(). (I've explained why it's there.)
> 
> Look at:
> 
> +       /* MsdIDTSEn - Enable timestamping on all PTP MessageIDs */
> +       err = tai->ops->ptp_global_write(tai->dev, PTPG_CONFIG_1,
> +                                        MV_PTP_MSD_ID_TS_EN);

Oop sorry, missed that custom ops.
 
> > Faced also a case where it has really high offsets and I need to reboot the
> > board to see precise synchronization again:
> > ptp4l[4649.618]: port 1: LISTENING to UNCALIBRATED on RS_SLAVE
> > ptp4l[4652.519]: master offset 7650600217 s0 freq +32767998 path delay
> > -33923681843 ptp4l[4653.487]: master offset 7617827584 s1 freq      +6 path
> > delay -33923681843 ptp4l[4654.454]: master offset     -27201 s2 freq
> > -27195 path delay -33923681843 ptp4l[4654.454]: port 1: UNCALIBRATED to
> > SLAVE on MASTER_CLOCK_SELECTED ptp4l[4655.422]: master offset 33926879673
> > s2 freq +32767999 path delay -67850561538 ptp4l[4656.389]: master offset
> > 33649307442 s2 freq +32767999 path delay -67605734496 ptp4l[4657.356]:
> > master offset 33454635535 s2 freq +32767999 path delay -67443834753  
> 
> I don't see that, I get way smaller offsets with this when using the
> ZII rev B platform as the master.
> 
> > The PTP set as master mode seems really buggy.  
> 
> That's something I didn't test, but as the hardware is symmetrical (there
> aren't separate settings for PTP MSGIDs that get stamped on the transmit
> and receive paths) I'm not sure what's going on. If one enables
> timestamping for MSG ID 0 (Sync) then the hardware will stamp packets
> received and transmitted that have a PTP MSG ID 0.
> 
> In any case, I can't do any testing now, sorry.

Yes, I will investigate.

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