Re: [RFC] net: dsa: mv88e6xxx: ptp: improve phc2sys precision for mv88e6xxx switch in combination with imx6-fec

[Miroslav Lichvar]

On Mon, Aug 05, 2019 at 12:54:49PM +0300, Vladimir Oltean wrote:
> - Along the lines of the above, I wonder how badly would the
> maintainers shout at the proposal of adding a struct
> ptp_system_timestamp pointer and its associated lock in struct device.
> That way at least you don't have to change the API, like you did with
> mdiobus_write_nested_ptp. Relatively speaking, this is the least
> amount of intrusion (although, again, far from beautiful).

That would make sense to me, if there are other drivers that could use
it.

> I also added Miroslav Lichvar, who originally created the
> PTP_SYS_OFFSET_EXTENDED ioctl, maybe he can share some ideas on how it
> is best served.

The idea behind that ioctl was to allow drivers to take the system
timestamps as close to the reading of the HW clock as possible,
excluding delays (and jitter) due to other operations that are
necessary to get that timestamp. In the ethernet drivers that was a
single PCI read. If in this case it's a "write" operation that
triggers the reading of the HW clock, then I think the patch is
using is the ptp_read_system_*ts() calls correctly.

> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -1814,11 +1814,25 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
> >
> >         reinit_completion(&fep->mdio_done);
> >
> > -       /* start a write op */
> > -       writel(FEC_MMFR_ST | FEC_MMFR_OP_WRITE |
> > -               FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(regnum) |
> > -               FEC_MMFR_TA | FEC_MMFR_DATA(value),
> > -               fep->hwp + FEC_MII_DATA);
> > +       if (bus->ptp_sts) {
> > +               unsigned long flags = 0;
> > +               local_irq_save(flags);
> > +               __iowmb();
> > +               /* >Take the timestamp *after* the memory barrier */
> > +               ptp_read_system_prets(bus->ptp_sts);
> > +               writel_relaxed(FEC_MMFR_ST | FEC_MMFR_OP_WRITE |
> > +                       FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(regnum) |
> > +                       FEC_MMFR_TA | FEC_MMFR_DATA(value),
> > +                       fep->hwp + FEC_MII_DATA);
> > +               ptp_read_system_postts(bus->ptp_sts);
> > +               local_irq_restore(flags);
> > +       } else {
> > +               /* start a write op */
> > +               writel(FEC_MMFR_ST | FEC_MMFR_OP_WRITE |
> > +                       FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(regnum) |
> > +                       FEC_MMFR_TA | FEC_MMFR_DATA(value),
> > +                       fep->hwp + FEC_MII_DATA);
> > +       }

-- 
Miroslav Lichvar