[PATCH net-next v2] net: dsa: mv88e6xxx: extend PTP gettime function to read system clock

[PATCH net-next v2] net: dsa: mv88e6xxx: extend PTP gettime function to read system clock

[Hubert Feurstein]

From: Hubert Feurstein <h.feurstein@gmail.com>

This adds support for the PTP_SYS_OFFSET_EXTENDED ioctl.

Signed-off-by: Hubert Feurstein <h.feurstein@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/ptp.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c
index 073cbd0bb91b..2ebc7db0fd4a 100644
--- a/drivers/net/dsa/mv88e6xxx/ptp.c
+++ b/drivers/net/dsa/mv88e6xxx/ptp.c
@@ -235,14 +235,17 @@ static int mv88e6xxx_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 	return 0;
 }
 
-static int mv88e6xxx_ptp_gettime(struct ptp_clock_info *ptp,
-				 struct timespec64 *ts)
+static int mv88e6xxx_ptp_gettimex(struct ptp_clock_info *ptp,
+				  struct timespec64 *ts,
+				  struct ptp_system_timestamp *sts)
 {
 	struct mv88e6xxx_chip *chip = ptp_to_chip(ptp);
 	u64 ns;
 
 	mv88e6xxx_reg_lock(chip);
+	ptp_read_system_prets(sts);
 	ns = timecounter_read(&chip->tstamp_tc);
+	ptp_read_system_postts(sts);
 	mv88e6xxx_reg_unlock(chip);
 
 	*ts = ns_to_timespec64(ns);
@@ -426,7 +429,7 @@ static void mv88e6xxx_ptp_overflow_check(struct work_struct *work)
 	struct mv88e6xxx_chip *chip = dw_overflow_to_chip(dw);
 	struct timespec64 ts;
 
-	mv88e6xxx_ptp_gettime(&chip->ptp_clock_info, &ts);
+	mv88e6xxx_ptp_gettimex(&chip->ptp_clock_info, &ts, NULL);
 
 	schedule_delayed_work(&chip->overflow_work,
 			      MV88E6XXX_TAI_OVERFLOW_PERIOD);
@@ -472,7 +475,7 @@ int mv88e6xxx_ptp_setup(struct mv88e6xxx_chip *chip)
 	chip->ptp_clock_info.max_adj    = MV88E6XXX_MAX_ADJ_PPB;
 	chip->ptp_clock_info.adjfine	= mv88e6xxx_ptp_adjfine;
 	chip->ptp_clock_info.adjtime	= mv88e6xxx_ptp_adjtime;
-	chip->ptp_clock_info.gettime64	= mv88e6xxx_ptp_gettime;
+	chip->ptp_clock_info.gettimex64	= mv88e6xxx_ptp_gettimex;
 	chip->ptp_clock_info.settime64	= mv88e6xxx_ptp_settime;
 	chip->ptp_clock_info.enable	= ptp_ops->ptp_enable;
 	chip->ptp_clock_info.verify	= ptp_ops->ptp_verify;
-- 
2.22.0

Re: [PATCH net-next v2] net: dsa: mv88e6xxx: extend PTP gettime function to read system clock

[Andrew Lunn]

On Mon, Aug 05, 2019 at 10:26:42AM +0200, Hubert Feurstein wrote:
> From: Hubert Feurstein <h.feurstein@gmail.com>

Hi Hubert

In your RFC patch, there was some interesting numbers. Can you provide
numbers of just this patch? How much of an improvement does it make?

Your RFC patch pushed these ptp_read_system_{pre|post}ts() calls down
into the MDIO driver. This patch is much less invasive, but i wonder
how much a penalty you paid?

Did you also try moving these calls into global2_avb.c, around the one
write that really matters?

It was speculated that the jitter comes from contention on the mdio
bus lock. Did you investigate this? If you can prove this true, one
thing to try is to take the mdio bus lock in the mv88e6xxx driver,
take the start timestamp, call __mdiobus_write(), and then the end
timestamp. The bus contention is then outside your time snapshot.

We could even think about adding a mdiobus_write variant which does
all this. I'm sure other DSA drivers would find it useful, if it
really does help.

	   Andrew

Re: [PATCH net-next v2] net: dsa: mv88e6xxx: extend PTP gettime function to read system clock

[Hubert Feurstein]

Hi Andrew,

Am Mo., 5. Aug. 2019 um 15:58 Uhr schrieb Andrew Lunn <andrew@lunn.ch>:
>
> On Mon, Aug 05, 2019 at 10:26:42AM +0200, Hubert Feurstein wrote:
> > From: Hubert Feurstein <h.feurstein@gmail.com>
>
> Hi Hubert
>
> In your RFC patch, there was some interesting numbers. Can you provide
> numbers of just this patch? How much of an improvement does it make?
>
> Your RFC patch pushed these ptp_read_system_{pre|post}ts() calls down
> into the MDIO driver. This patch is much less invasive, but i wonder
> how much a penalty you paid?

I mentioned the numbers already in my RFC mail.
Without this patch a quick test-run gave me this result:
  Min:          -17829 ns
  Max:          21694 ns
  StdDev:       8520 ns
  Count:        127

With this patch applied, the results improved:
  Min:          -12144 ns
  Max:          10891 ns
  StdDev:       4046,71 ns
  Count:        112

I know, the sample count for the statistics (only 112 samples!) is not
really big.
However, even with this low number of samples I already got too high min / max
values.

>
> Did you also try moving these calls into global2_avb.c, around the one
> write that really matters?
>
> It was speculated that the jitter comes from contention on the mdio
> bus lock. Did you investigate this? If you can prove this true, one
> thing to try is to take the mdio bus lock in the mv88e6xxx driver,
> take the start timestamp, call __mdiobus_write(), and then the end
> timestamp. The bus contention is then outside your time snapshot.
>

I've tested this now:

diff --git a/drivers/net/dsa/mv88e6xxx/smi.c b/drivers/net/dsa/mv88e6xxx/smi.c
index 801fd4abba5a..fc7f9318df52 100644
--- a/drivers/net/dsa/mv88e6xxx/smi.c
+++ b/drivers/net/dsa/mv88e6xxx/smi.c
@@ -40,12 +40,27 @@ static int mv88e6xxx_smi_direct_read(struct
mv88e6xxx_chip *chip,
       return 0;
}

+static int mv88e6xxx_mdiobus_write_nested(struct mv88e6xxx_chip
*chip, int addr, u32 regnum, u16 val)
+{
+       int err;
+
+       BUG_ON(in_interrupt());
+
+       mutex_lock_nested(&chip->bus->mdio_lock, MDIO_MUTEX_NESTED);
+       ptp_read_system_prets(chip->ptp_sts);
+       err = __mdiobus_write(chip->bus, addr, regnum, val);
+       ptp_read_system_postts(chip->ptp_sts);
+       mutex_unlock(&chip->bus->mdio_lock);
+
+       return err;
+}
+
static int mv88e6xxx_smi_direct_write(struct mv88e6xxx_chip *chip,
                                     int dev, int reg, u16 data)
{
       int ret;

-       ret = mdiobus_write_nested_ptp(chip->bus, dev, reg, data,
chip->ptp_sts);
+       ret = mv88e6xxx_mdiobus_write_nested(chip, dev, reg, data);
       if (ret < 0)
               return ret;

The result was:
Min:  -8052
Max:  9988
StdDev: 2490.17
Count: 3592

It got improved, but you still have the unpredictable latencies caused by the
mdio_done-completion (=> wait_for_completion_timeout) in imx_fec.

> We could even think about adding a mdiobus_write variant which does
> all this. I'm sure other DSA drivers would find it useful, if it
> really does help.
>
>            Andrew

Hubert

Re: [PATCH net-next v2] net: dsa: mv88e6xxx: extend PTP gettime function to read system clock

[Richard Cochran]

On Mon, Aug 05, 2019 at 07:12:40PM +0200, Hubert Feurstein wrote:
> It got improved, but you still have the unpredictable latencies caused by the
> mdio_done-completion (=> wait_for_completion_timeout) in imx_fec.

Yes, that is the important point.

Please take a look at other mmi_bus.write() implementations and see if
you can place the timestamps into the mii_bus layer.

Thanks,
Richard

Re: [PATCH net-next v2] net: dsa: mv88e6xxx: extend PTP gettime function to read system clock

[Andrew Lunn]

> +static int mv88e6xxx_mdiobus_write_nested(struct mv88e6xxx_chip
> *chip, int addr, u32 regnum, u16 val)
> +{
> +       int err;
> +
> +       BUG_ON(in_interrupt());
> +
> +       mutex_lock_nested(&chip->bus->mdio_lock, MDIO_MUTEX_NESTED);
> +       ptp_read_system_prets(chip->ptp_sts);
> +       err = __mdiobus_write(chip->bus, addr, regnum, val);
> +       ptp_read_system_postts(chip->ptp_sts);
> +       mutex_unlock(&chip->bus->mdio_lock);
> +
> +       return err;
> +}
> +
> static int mv88e6xxx_smi_direct_write(struct mv88e6xxx_chip *chip,
>                                      int dev, int reg, u16 data)
> {
>        int ret;
> 
> -       ret = mdiobus_write_nested_ptp(chip->bus, dev, reg, data,
> chip->ptp_sts);
> +       ret = mv88e6xxx_mdiobus_write_nested(chip, dev, reg, data);
>        if (ret < 0)
>                return ret;
> 
> The result was:
> Min:  -8052
> Max:  9988
> StdDev: 2490.17
> Count: 3592
> 
> It got improved, but you still have the unpredictable latencies caused by the
> mdio_done-completion (=> wait_for_completion_timeout) in imx_fec.

O.K. So lets think about a more generic solution we can use inside the
mdio bus driver. I don't know if adding an sts pointer to struct
device will be accepted. But adding one to struct mii_bus should be
O.K. It can be assigned to once the mdio_lock is taken, to avoid race
conditions. Add mdio_ptp_read_system_prets(bus) and
mdio_ptp_read_system_postts(bus) which the bus driver can use.

We also need a fallback in case the bus driver does not use them, so
something like:

mdiobus_write_sts(...)
{
        int retval;

        BUG_ON(in_interrupt());

        mutex_lock(&bus->mdio_lock);
	bus->sts = sts;
	sts->post_ts = 0;

	ktime_get_real_ts64(&sts->pre_ts);

        retval = __mdiobus_write(bus, addr, regnum, val);

	if (!sts->post_ts)
	   ktime_get_real_ts64(sts->post_ts)

	bus->sts = NULL;
        mutex_unlock(&bus->mdio_lock);

        return retval;
}

So at worse case, we get the time around the whole write operation,
but the MDIO bus driver can overwrite the pre_ts and set post_ts,
using mdio_ptp_read_system_prets(bus) and
mdio_ptp_read_system_postts(bus).

A similar scheme could be implemented to SPI devices, if the SPI
maintainer would accepted a sts pointer in the SPI bus driver
structure.

	Andrew