With this patchset the phc2sys synchronisation precision improved to +/-555ns on an IMX6DL with an MV88E6220 switch attached.
This patchset takes into account the comments from the following discussions:
- https://lkml.org/lkml/2019/8/2/1364
- https://lkml.org/lkml/2019/8/5/169
Patch 01 adds the required infrastructure in the MDIO layer.
Patch 02 adds support for the PTP_SYS_OFFSET_EXTENDED ioctl in the mv88e6xxx driver.
Patch 03 adds support for the PTP system timestamping in the imx-fec driver.
The following tests show the improvement caused by each patch. The system clock precision was set to 15ns instead of 333ns (as described in https://lkml.org/lkml/2019/8/2/1364).
Without this patchset applied, the phc2sys synchronisation performance was very poor:
offset: min -27120 max 28840 mean 2.44 stddev 8040.78 count 1236
delay: min 282103 max 386385 mean 352568.03 stddev 27814.27 count 1236
(test runtime 20 minutes)
Results after appling patch 01 and 02:
offset: min -12316 max 13314 mean -9.38 stddev 4274.82 count 1022
delay: min 69977 max 96266 mean 87939.04 stddev 6466.17 count 1022
(test runtime 16 minutes)
Results after appling patch 03:
offset: min -788 max 528 mean -0.06 stddev 185.02 count 7171
delay: min 1773 max 2031 mean 1909.43 stddev 33.74 count 7171
(test runtime 119 minutes)
Hubert Feurstein (3):
net: mdio: add support for passing a PTP system timestamp to the
mii_bus driver
net: dsa: mv88e6xxx: extend PTP gettime function to read system clock
net: fec: add support for PTP system timestamping for MDIO devices
drivers/net/dsa/mv88e6xxx/chip.h | 2 +
drivers/net/dsa/mv88e6xxx/ptp.c | 11 ++-
drivers/net/dsa/mv88e6xxx/smi.c | 3 +-
drivers/net/ethernet/freescale/fec_main.c | 3 +
drivers/net/phy/mdio_bus.c | 105 ++++++++++++++++++++++
include/linux/mdio.h | 7 ++
include/linux/phy.h | 25 ++++++
7 files changed, 151 insertions(+), 5 deletions(-)
--
2.22.1
In order to improve the synchronisation precision of phc2sys (from
the linuxptp project) for devices like switches which are attached
to the MDIO bus, it is necessary the get the system timestamps as
close as possible to the access which causes the PTP timestamp
register to be snapshotted in the switch hardware. Usually this is
triggered by an MDIO write access, the snapshotted timestamp is then
transferred by several MDIO reads.
This patch adds the required infrastructure to solve the problem described
above.
Signed-off-by: Hubert Feurstein <[email protected]>
---
drivers/net/phy/mdio_bus.c | 105 +++++++++++++++++++++++++++++++++++++
include/linux/mdio.h | 7 +++
include/linux/phy.h | 25 +++++++++
3 files changed, 137 insertions(+)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index bd04fe762056..167a21f267fa 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -34,6 +34,7 @@
#include <linux/phy.h>
#include <linux/io.h>
#include <linux/uaccess.h>
+#include <linux/ptp_clock_kernel.h>
#define CREATE_TRACE_POINTS
#include <trace/events/mdio.h>
@@ -697,6 +698,110 @@ int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
}
EXPORT_SYMBOL(mdiobus_write);
+/**
+ * __mdiobus_write_sts - Unlocked version of the mdiobus_write_sts function
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @regnum: register number to write
+ * @val: value to write to @regnum
+ * @sts: the ptp system timestamp
+ *
+ * Write a MDIO bus register and request the MDIO bus driver to take the
+ * system timestamps when sts-pointer is valid. When the bus driver doesn't
+ * support this, the timestamps are taken in this function instead.
+ *
+ * In order to improve the synchronisation precision of phc2sys (from
+ * the linuxptp project) for devices like switches which are attached
+ * to the MDIO bus, it is necessary the get the system timestamps as
+ * close as possible to the access which causes the PTP timestamp
+ * register to be snapshotted in the switch hardware. Usually this is
+ * triggered by an MDIO write access, the snapshotted timestamp is then
+ * transferred by several MDIO reads.
+ *
+ * Caller must hold the mdio bus lock.
+ *
+ * NOTE: MUST NOT be called from interrupt context.
+ */
+int __mdiobus_write_sts(struct mii_bus *bus, int addr, u32 regnum, u16 val,
+ struct ptp_system_timestamp *sts)
+{
+ int retval;
+
+ WARN_ON_ONCE(!mutex_is_locked(&bus->mdio_lock));
+
+ if (!bus->ptp_sts_supported)
+ ptp_read_system_prets(sts);
+
+ bus->ptp_sts = sts;
+ retval = __mdiobus_write(bus, addr, regnum, val);
+ bus->ptp_sts = NULL;
+
+ if (!bus->ptp_sts_supported)
+ ptp_read_system_postts(sts);
+
+ return retval;
+}
+EXPORT_SYMBOL(__mdiobus_write_sts);
+
+/**
+ * mdiobus_write_sts - Convenience function for writing a given MII mgmt
+ * register
+ *
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @regnum: register number to write
+ * @val: value to write to @regnum
+ * @sts: the ptp system timestamp
+ *
+ * NOTE: MUST NOT be called from interrupt context,
+ * because the bus read/write functions may wait for an interrupt
+ * to conclude the operation.
+ */
+int mdiobus_write_sts(struct mii_bus *bus, int addr, u32 regnum, u16 val,
+ struct ptp_system_timestamp *sts)
+{
+ int retval;
+
+ BUG_ON(in_interrupt());
+
+ mutex_lock(&bus->mdio_lock);
+ retval = __mdiobus_write_sts(bus, addr, regnum, val, sts);
+ mutex_unlock(&bus->mdio_lock);
+
+ return retval;
+}
+EXPORT_SYMBOL(mdiobus_write_sts);
+
+/**
+ * mdiobus_write_sts_nested - Nested version of the mdiobus_write_sts function
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @regnum: register number to write
+ * @val: value to write to @regnum
+ * @sts: the ptp system timestamp
+ *
+ * In case of nested MDIO bus access avoid lockdep false positives by
+ * using mutex_lock_nested().
+ *
+ * NOTE: MUST NOT be called from interrupt context,
+ * because the bus read/write functions may wait for an interrupt
+ * to conclude the operation.
+ */
+int mdiobus_write_sts_nested(struct mii_bus *bus, int addr, u32 regnum, u16 val,
+ struct ptp_system_timestamp *sts)
+{
+ int retval;
+
+ BUG_ON(in_interrupt());
+
+ mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+ retval = __mdiobus_write_sts(bus, addr, regnum, val, sts);
+ mutex_unlock(&bus->mdio_lock);
+
+ return retval;
+}
+EXPORT_SYMBOL(mdiobus_write_sts_nested);
+
/**
* mdio_bus_match - determine if given MDIO driver supports the given
* MDIO device
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index e8242ad88c81..d65625c75b15 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -9,6 +9,7 @@
#include <uapi/linux/mdio.h>
#include <linux/mod_devicetable.h>
+struct ptp_system_timestamp;
struct gpio_desc;
struct mii_bus;
@@ -305,11 +306,17 @@ static inline void mii_10gbt_stat_mod_linkmode_lpa_t(unsigned long *advertising,
int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum);
int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val);
+int __mdiobus_write_sts(struct mii_bus *bus, int addr, u32 regnum, u16 val,
+ struct ptp_system_timestamp *sts);
int mdiobus_read(struct mii_bus *bus, int addr, u32 regnum);
int mdiobus_read_nested(struct mii_bus *bus, int addr, u32 regnum);
int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val);
int mdiobus_write_nested(struct mii_bus *bus, int addr, u32 regnum, u16 val);
+int mdiobus_write_sts(struct mii_bus *bus, int addr, u32 regnum, u16 val,
+ struct ptp_system_timestamp *sts);
+int mdiobus_write_sts_nested(struct mii_bus *bus, int addr, u32 regnum, u16 val,
+ struct ptp_system_timestamp *sts);
int mdiobus_register_device(struct mdio_device *mdiodev);
int mdiobus_unregister_device(struct mdio_device *mdiodev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 462b90b73f93..15afe9c5256b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -252,6 +252,31 @@ struct mii_bus {
int reset_delay_us;
/* RESET GPIO descriptor pointer */
struct gpio_desc *reset_gpiod;
+
+ /* PTP system timestamping support
+ *
+ * In order to improve the synchronisation precision of phc2sys (from
+ * the linuxptp project) for devices like switches which are attached
+ * to the MDIO bus, it is necessary the get the system timestamps as
+ * close as possible to the access which causes the PTP timestamp
+ * register to be snapshotted in the switch hardware. Usually this is
+ * triggered by an MDIO write access, the snapshotted timestamp is then
+ * transferred by several MDIO reads.
+ *
+ * The switch driver can use mdio_write_sts*() to pass through the
+ * system timestamp pointer @ptp_sts to the MDIO bus driver. The bus
+ * driver simply has to do the following calls in its write handler:
+ * ptp_read_system_prets(bus->ptp_sts);
+ * writel(value, mdio-register)
+ * ptp_read_system_postts(bus->ptp_sts);
+ *
+ * The ptp_read_system_*ts functions already check the ptp_sts pointer.
+ *
+ * @ptp_sts_supported: Must be set to true when the MDIO bus driver
+ * takes the timestamps as described above.
+ */
+ struct ptp_system_timestamp *ptp_sts;
+ bool ptp_sts_supported;
};
#define to_mii_bus(d) container_of(d, struct mii_bus, dev)
--
2.22.1
This adds support for the PTP_SYS_OFFSET_EXTENDED ioctl.
Signed-off-by: Hubert Feurstein <[email protected]>
---
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(-)
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;
--
2.22.1
In order to improve the synchronisation precision of phc2sys (from
the linuxptp project) for devices like switches which are attached
to the MDIO bus, it is necessary the get the system timestamps as
close as possible to the access which causes the PTP timestamp
register to be snapshotted in the switch hardware. Usually this is
triggered by an MDIO write access, the snapshotted timestamp is then
transferred by several MDIO reads.
The ptp_read_system_*ts functions already check the ptp_sts pointer.
Signed-off-by: Hubert Feurstein <[email protected]>
---
drivers/net/ethernet/freescale/fec_main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 2f6057e7335d..60e866631b61 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1815,10 +1815,12 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
reinit_completion(&fep->mdio_done);
/* start a write op */
+ ptp_read_system_prets(bus->ptp_sts);
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);
+ ptp_read_system_postts(bus->ptp_sts);
/* wait for end of transfer */
time_left = wait_for_completion_timeout(&fep->mdio_done,
@@ -2034,6 +2036,7 @@ static int fec_enet_mii_init(struct platform_device *pdev)
pdev->name, fep->dev_id + 1);
fep->mii_bus->priv = fep;
fep->mii_bus->parent = &pdev->dev;
+ fep->mii_bus->ptp_sts_supported = true;
node = of_get_child_by_name(pdev->dev.of_node, "mdio");
err = of_mdiobus_register(fep->mii_bus, node);
--
2.22.1
On Fri, Aug 16, 2019 at 06:31:55PM +0200, Hubert Feurstein wrote:
>
> int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum);
> int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val);
> +int __mdiobus_write_sts(struct mii_bus *bus, int addr, u32 regnum, u16 val,
> + struct ptp_system_timestamp *sts);
>
> int mdiobus_read(struct mii_bus *bus, int addr, u32 regnum);
> int mdiobus_read_nested(struct mii_bus *bus, int addr, u32 regnum);
> int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val);
> int mdiobus_write_nested(struct mii_bus *bus, int addr, u32 regnum, u16 val);
> +int mdiobus_write_sts(struct mii_bus *bus, int addr, u32 regnum, u16 val,
> + struct ptp_system_timestamp *sts);
> +int mdiobus_write_sts_nested(struct mii_bus *bus, int addr, u32 regnum, u16 val,
> + struct ptp_system_timestamp *sts);
Following the pattern, you have made three new global
mdiobus_write_sts() functions. However, your patch set only uses
mdiobus_write_sts_nested().
Please don't add global functions with no users. Let the first user
add them, if and when the need arises.
Thanks,
Richard
On Fri, Aug 16, 2019 at 06:31:55PM +0200, Hubert Feurstein wrote:
> In order to improve the synchronisation precision of phc2sys (from
> the linuxptp project) for devices like switches which are attached
> to the MDIO bus, it is necessary the get the system timestamps as
> close as possible to the access which causes the PTP timestamp
> register to be snapshotted in the switch hardware. Usually this is
> triggered by an MDIO write access, the snapshotted timestamp is then
> transferred by several MDIO reads.
>
> This patch adds the required infrastructure to solve the problem described
> above.
>
> Signed-off-by: Hubert Feurstein <[email protected]>
> ---
> drivers/net/phy/mdio_bus.c | 105 +++++++++++++++++++++++++++++++++++++
> include/linux/mdio.h | 7 +++
> include/linux/phy.h | 25 +++++++++
> 3 files changed, 137 insertions(+)
>
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index bd04fe762056..167a21f267fa 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -34,6 +34,7 @@
> #include <linux/phy.h>
> #include <linux/io.h>
> #include <linux/uaccess.h>
> +#include <linux/ptp_clock_kernel.h>
>
> #define CREATE_TRACE_POINTS
> #include <trace/events/mdio.h>
> @@ -697,6 +698,110 @@ int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
> }
> EXPORT_SYMBOL(mdiobus_write);
>
> +/**
> + * __mdiobus_write_sts - Unlocked version of the mdiobus_write_sts function
> + * @bus: the mii_bus struct
> + * @addr: the phy address
> + * @regnum: register number to write
> + * @val: value to write to @regnum
> + * @sts: the ptp system timestamp
> + *
> + * Write a MDIO bus register and request the MDIO bus driver to take the
> + * system timestamps when sts-pointer is valid. When the bus driver doesn't
> + * support this, the timestamps are taken in this function instead.
> + *
> + * In order to improve the synchronisation precision of phc2sys (from
> + * the linuxptp project) for devices like switches which are attached
> + * to the MDIO bus, it is necessary the get the system timestamps as
> + * close as possible to the access which causes the PTP timestamp
> + * register to be snapshotted in the switch hardware. Usually this is
> + * triggered by an MDIO write access, the snapshotted timestamp is then
> + * transferred by several MDIO reads.
> + *
> + * Caller must hold the mdio bus lock.
> + *
> + * NOTE: MUST NOT be called from interrupt context.
> + */
> +int __mdiobus_write_sts(struct mii_bus *bus, int addr, u32 regnum, u16 val,
> + struct ptp_system_timestamp *sts)
> +{
> + int retval;
> +
> + WARN_ON_ONCE(!mutex_is_locked(&bus->mdio_lock));
> +
> + if (!bus->ptp_sts_supported)
> + ptp_read_system_prets(sts);
How expensive is ptp_read_system_prets()? My original suggestion was
to unconditionally call it here, and then let the driver overwrite it
if it supports finer grained time stamping. MDIO is slow, so as long
as ptp_read_system_prets() is not too expensive, i prefer KISS.
Andrew
> @@ -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
Hi Andrew,
On Mon, 19 Aug 2019 at 16:17, Andrew Lunn <[email protected]> wrote:
>
> On Fri, Aug 16, 2019 at 06:31:55PM +0200, Hubert Feurstein wrote:
> > In order to improve the synchronisation precision of phc2sys (from
> > the linuxptp project) for devices like switches which are attached
> > to the MDIO bus, it is necessary the get the system timestamps as
> > close as possible to the access which causes the PTP timestamp
> > register to be snapshotted in the switch hardware. Usually this is
> > triggered by an MDIO write access, the snapshotted timestamp is then
> > transferred by several MDIO reads.
> >
> > This patch adds the required infrastructure to solve the problem described
> > above.
> >
> > Signed-off-by: Hubert Feurstein <[email protected]>
> > ---
> > drivers/net/phy/mdio_bus.c | 105 +++++++++++++++++++++++++++++++++++++
> > include/linux/mdio.h | 7 +++
> > include/linux/phy.h | 25 +++++++++
> > 3 files changed, 137 insertions(+)
> >
> > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> > index bd04fe762056..167a21f267fa 100644
> > --- a/drivers/net/phy/mdio_bus.c
> > +++ b/drivers/net/phy/mdio_bus.c
> > @@ -34,6 +34,7 @@
> > #include <linux/phy.h>
> > #include <linux/io.h>
> > #include <linux/uaccess.h>
> > +#include <linux/ptp_clock_kernel.h>
> >
> > #define CREATE_TRACE_POINTS
> > #include <trace/events/mdio.h>
> > @@ -697,6 +698,110 @@ int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
> > }
> > EXPORT_SYMBOL(mdiobus_write);
> >
> > +/**
> > + * __mdiobus_write_sts - Unlocked version of the mdiobus_write_sts function
> > + * @bus: the mii_bus struct
> > + * @addr: the phy address
> > + * @regnum: register number to write
> > + * @val: value to write to @regnum
> > + * @sts: the ptp system timestamp
> > + *
> > + * Write a MDIO bus register and request the MDIO bus driver to take the
> > + * system timestamps when sts-pointer is valid. When the bus driver doesn't
> > + * support this, the timestamps are taken in this function instead.
> > + *
> > + * In order to improve the synchronisation precision of phc2sys (from
> > + * the linuxptp project) for devices like switches which are attached
> > + * to the MDIO bus, it is necessary the get the system timestamps as
> > + * close as possible to the access which causes the PTP timestamp
> > + * register to be snapshotted in the switch hardware. Usually this is
> > + * triggered by an MDIO write access, the snapshotted timestamp is then
> > + * transferred by several MDIO reads.
> > + *
> > + * Caller must hold the mdio bus lock.
> > + *
> > + * NOTE: MUST NOT be called from interrupt context.
> > + */
> > +int __mdiobus_write_sts(struct mii_bus *bus, int addr, u32 regnum, u16 val,
> > + struct ptp_system_timestamp *sts)
> > +{
> > + int retval;
> > +
> > + WARN_ON_ONCE(!mutex_is_locked(&bus->mdio_lock));
> > +
> > + if (!bus->ptp_sts_supported)
> > + ptp_read_system_prets(sts);
>
> How expensive is ptp_read_system_prets()? My original suggestion was
> to unconditionally call it here, and then let the driver overwrite it
> if it supports finer grained time stamping. MDIO is slow, so as long
> as ptp_read_system_prets() is not too expensive, i prefer KISS.
>
> Andrew
While that works for the pre_ts, it doesn't work for the post_ts (the
MDIO bus core will unconditionally overwrite the system timestamp from
the driver).
Unless you're suggesting to keep the pre_ts unconditional and the
post_ts under the "if" condition, which is a bit odd.
According to my tests with a scope (measuring the width between SPI
transfers with and without the ptp_read_system_*ts calls), two calls
to ktime_get_real_ts64 amount to around 750 ns on a 1200 MHz Cortex A7
core, or around 90 clock cycles.
Regards,
-Vladimir
On Mon, 19 Aug 2019 at 16:34, Vladimir Oltean <[email protected]> wrote:
>
> Hi Andrew,
>
> On Mon, 19 Aug 2019 at 16:17, Andrew Lunn <[email protected]> wrote:
> >
> > On Fri, Aug 16, 2019 at 06:31:55PM +0200, Hubert Feurstein wrote:
> > > In order to improve the synchronisation precision of phc2sys (from
> > > the linuxptp project) for devices like switches which are attached
> > > to the MDIO bus, it is necessary the get the system timestamps as
> > > close as possible to the access which causes the PTP timestamp
> > > register to be snapshotted in the switch hardware. Usually this is
> > > triggered by an MDIO write access, the snapshotted timestamp is then
> > > transferred by several MDIO reads.
> > >
> > > This patch adds the required infrastructure to solve the problem described
> > > above.
> > >
> > > Signed-off-by: Hubert Feurstein <[email protected]>
> > > ---
> > > drivers/net/phy/mdio_bus.c | 105 +++++++++++++++++++++++++++++++++++++
> > > include/linux/mdio.h | 7 +++
> > > include/linux/phy.h | 25 +++++++++
> > > 3 files changed, 137 insertions(+)
> > >
> > > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> > > index bd04fe762056..167a21f267fa 100644
> > > --- a/drivers/net/phy/mdio_bus.c
> > > +++ b/drivers/net/phy/mdio_bus.c
> > > @@ -34,6 +34,7 @@
> > > #include <linux/phy.h>
> > > #include <linux/io.h>
> > > #include <linux/uaccess.h>
> > > +#include <linux/ptp_clock_kernel.h>
> > >
> > > #define CREATE_TRACE_POINTS
> > > #include <trace/events/mdio.h>
> > > @@ -697,6 +698,110 @@ int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
> > > }
> > > EXPORT_SYMBOL(mdiobus_write);
> > >
> > > +/**
> > > + * __mdiobus_write_sts - Unlocked version of the mdiobus_write_sts function
> > > + * @bus: the mii_bus struct
> > > + * @addr: the phy address
> > > + * @regnum: register number to write
> > > + * @val: value to write to @regnum
> > > + * @sts: the ptp system timestamp
> > > + *
> > > + * Write a MDIO bus register and request the MDIO bus driver to take the
> > > + * system timestamps when sts-pointer is valid. When the bus driver doesn't
> > > + * support this, the timestamps are taken in this function instead.
> > > + *
> > > + * In order to improve the synchronisation precision of phc2sys (from
> > > + * the linuxptp project) for devices like switches which are attached
> > > + * to the MDIO bus, it is necessary the get the system timestamps as
> > > + * close as possible to the access which causes the PTP timestamp
> > > + * register to be snapshotted in the switch hardware. Usually this is
> > > + * triggered by an MDIO write access, the snapshotted timestamp is then
> > > + * transferred by several MDIO reads.
> > > + *
> > > + * Caller must hold the mdio bus lock.
> > > + *
> > > + * NOTE: MUST NOT be called from interrupt context.
> > > + */
> > > +int __mdiobus_write_sts(struct mii_bus *bus, int addr, u32 regnum, u16 val,
> > > + struct ptp_system_timestamp *sts)
> > > +{
> > > + int retval;
> > > +
> > > + WARN_ON_ONCE(!mutex_is_locked(&bus->mdio_lock));
> > > +
> > > + if (!bus->ptp_sts_supported)
> > > + ptp_read_system_prets(sts);
> >
> > How expensive is ptp_read_system_prets()? My original suggestion was
> > to unconditionally call it here, and then let the driver overwrite it
> > if it supports finer grained time stamping. MDIO is slow, so as long
> > as ptp_read_system_prets() is not too expensive, i prefer KISS.
> >
> > Andrew
>
> While that works for the pre_ts, it doesn't work for the post_ts (the
> MDIO bus core will unconditionally overwrite the system timestamp from
> the driver).
> Unless you're suggesting to keep the pre_ts unconditional and the
> post_ts under the "if" condition, which is a bit odd.
> According to my tests with a scope (measuring the width between SPI
> transfers with and without the ptp_read_system_*ts calls), two calls
> to ktime_get_real_ts64 amount to around 750 ns on a 1200 MHz Cortex A7
> core, or around 90 clock cycles.
900 clock cycles, my bad.
>
> Regards,
> -Vladimir
> > > How expensive is ptp_read_system_prets()? My original suggestion was
> > > to unconditionally call it here, and then let the driver overwrite it
> > > if it supports finer grained time stamping. MDIO is slow, so as long
> > > as ptp_read_system_prets() is not too expensive, i prefer KISS.
> > >
> > > Andrew
> >
> > While that works for the pre_ts, it doesn't work for the post_ts (the
> > MDIO bus core will unconditionally overwrite the system timestamp from
> > the driver).
> > Unless you're suggesting to keep the pre_ts unconditional and the
> > post_ts under the "if" condition, which is a bit odd.
> > According to my tests with a scope (measuring the width between SPI
> > transfers with and without the ptp_read_system_*ts calls), two calls
> > to ktime_get_real_ts64 amount to around 750 ns on a 1200 MHz Cortex A7
> > core, or around 90 clock cycles.
>
> 900 clock cycles, my bad.
That is quite a lot. I was just expecting it to read a free running
clock and maybe do some unit conversions. 900 cycles suggests it is
doing a lot more.
So please keep with the idea of the bus driver indicating if it
supports the time stamping. But please make it a generic bus->flags,
and bit 0 indicating time stamping. At some point in the future, it
would be useful to indicate if the bus supports c45, which would be
another use of flags.
Thanks
Andrew
On Mon, 19 Aug 2019 15:27:33 +0200, Andrew Lunn <[email protected]> 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 <[email protected]> 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 <[email protected]>:
>
> > @@ -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 <[email protected]>:
> >
> > > @@ -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