| Message ID | 20190816163157.25314-3-h.feurstein@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Improve phc2sys precision for mv88e6xxx switch in combination with imx6-fec | expand |
> @@ -45,7 +45,8 @@ static int mv88e6xxx_smi_direct_write(struct mv88e6xxx_chip *chip, > { > int ret; > > - ret = mdiobus_write_nested(chip->bus, dev, reg, data); > + ret = mdiobus_write_sts_nested(chip->bus, dev, reg, data, > + chip->ptp_sts); > if (ret < 0) > return ret; > Please also make a similar change to mv88e6xxx_smi_indirect_write(). The last write in that function should be timestamped. Vivien, please could you think about these changes with respect to RMU. We probably want to skip the RMU in this case, so we get slow but uniform jitter, vs fast and unpredictable jitter from using the RMU. Thanks Andrew
On Mon, 19 Aug 2019 15:27:33 +0200, Andrew Lunn <andrew@lunn.ch> wrote: > > @@ -45,7 +45,8 @@ static int mv88e6xxx_smi_direct_write(struct mv88e6xxx_chip *chip, > > { > > int ret; > > > > - ret = mdiobus_write_nested(chip->bus, dev, reg, data); > > + ret = mdiobus_write_sts_nested(chip->bus, dev, reg, data, > > + chip->ptp_sts); > > if (ret < 0) > > return ret; > > > > Please also make a similar change to mv88e6xxx_smi_indirect_write(). > The last write in that function should be timestamped. > > Vivien, please could you think about these changes with respect to > RMU. We probably want to skip the RMU in this case, so we get slow but > uniform jitter, vs fast and unpredictable jitter from using the RMU. The RMU will have its own mv88e6xxx_bus_ops.
On Mon, Aug 19, 2019 at 11:16:39AM -0400, Vivien Didelot wrote: > On Mon, 19 Aug 2019 15:27:33 +0200, Andrew Lunn <andrew@lunn.ch> wrote: > > > @@ -45,7 +45,8 @@ static int mv88e6xxx_smi_direct_write(struct mv88e6xxx_chip *chip, > > > { > > > int ret; > > > > > > - ret = mdiobus_write_nested(chip->bus, dev, reg, data); > > > + ret = mdiobus_write_sts_nested(chip->bus, dev, reg, data, > > > + chip->ptp_sts); > > > if (ret < 0) > > > return ret; > > > > > > > Please also make a similar change to mv88e6xxx_smi_indirect_write(). > > The last write in that function should be timestamped. > > > > Vivien, please could you think about these changes with respect to > > RMU. We probably want to skip the RMU in this case, so we get slow but > > uniform jitter, vs fast and unpredictable jitter from using the RMU. > > The RMU will have its own mv88e6xxx_bus_ops. Yes, that is what i was expecting. But for this operation, triggering a PTP timestamp, we probably want it to fall back to MDIO, which is much more deterministic. Andrew
Hi Andrew, Am Mo., 19. Aug. 2019 um 15:27 Uhr schrieb Andrew Lunn <andrew@lunn.ch>: > > > @@ -45,7 +45,8 @@ static int mv88e6xxx_smi_direct_write(struct mv88e6xxx_chip *chip, > > { > > int ret; > > > > - ret = mdiobus_write_nested(chip->bus, dev, reg, data); > > + ret = mdiobus_write_sts_nested(chip->bus, dev, reg, data, > > + chip->ptp_sts); > > if (ret < 0) > > return ret; > > > > Please also make a similar change to mv88e6xxx_smi_indirect_write(). > The last write in that function should be timestamped. Since it is already the last write it should be already ok (The mv88e6xxx_smi_indirect_write calls the mv88e6xxx_smi_direct_write which initiates the timestamping). Hubert
On Mon, Aug 19, 2019 at 07:14:25PM +0200, Hubert Feurstein wrote: > Hi Andrew, > > Am Mo., 19. Aug. 2019 um 15:27 Uhr schrieb Andrew Lunn <andrew@lunn.ch>: > > > > > @@ -45,7 +45,8 @@ static int mv88e6xxx_smi_direct_write(struct mv88e6xxx_chip *chip, > > > { > > > int ret; > > > > > > - ret = mdiobus_write_nested(chip->bus, dev, reg, data); > > > + ret = mdiobus_write_sts_nested(chip->bus, dev, reg, data, > > > + chip->ptp_sts); > > > if (ret < 0) > > > return ret; > > > > > > > Please also make a similar change to mv88e6xxx_smi_indirect_write(). > > The last write in that function should be timestamped. > Since it is already the last write it should be already ok (The > mv88e6xxx_smi_indirect_write > calls the mv88e6xxx_smi_direct_write which initiates the timestamping). Hi Hubert But you are also time stamping the first write as well. And it seems like it is not completely for free. So it would be nice to limit it to the write which actually matters. Andrew
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h index 01963ee94c50..9e14dc406415 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h +++ b/drivers/net/dsa/mv88e6xxx/chip.h @@ -277,6 +277,8 @@ struct mv88e6xxx_chip { struct ptp_clock_info ptp_clock_info; struct delayed_work tai_event_work; struct ptp_pin_desc pin_config[MV88E6XXX_MAX_GPIO]; + struct ptp_system_timestamp *ptp_sts; + u16 trig_config; u16 evcap_config; u16 enable_count; diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c index 073cbd0bb91b..cf6e52ee9e0a 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); + chip->ptp_sts = sts; ns = timecounter_read(&chip->tstamp_tc); + chip->ptp_sts = NULL; 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; diff --git a/drivers/net/dsa/mv88e6xxx/smi.c b/drivers/net/dsa/mv88e6xxx/smi.c index 5fc78a063843..e3b0096a9d94 100644 --- a/drivers/net/dsa/mv88e6xxx/smi.c +++ b/drivers/net/dsa/mv88e6xxx/smi.c @@ -45,7 +45,8 @@ static int mv88e6xxx_smi_direct_write(struct mv88e6xxx_chip *chip, { int ret; - ret = mdiobus_write_nested(chip->bus, dev, reg, data); + ret = mdiobus_write_sts_nested(chip->bus, dev, reg, data, + chip->ptp_sts); if (ret < 0) return ret;
This adds support for the PTP_SYS_OFFSET_EXTENDED ioctl. Signed-off-by: Hubert Feurstein <h.feurstein@gmail.com> --- drivers/net/dsa/mv88e6xxx/chip.h | 2 ++ drivers/net/dsa/mv88e6xxx/ptp.c | 11 +++++++---- drivers/net/dsa/mv88e6xxx/smi.c | 3 ++- 3 files changed, 11 insertions(+), 5 deletions(-)