[PATCH net-next 0/5] net: mdio: add timestamped MDIO writes for PHY gettimex64

[PATCH net-next 0/5] net: mdio: add timestamped MDIO writes for PHY gettimex64

[James Clark]

PTP_SYS_OFFSET_EXTENDED uses gettimex64 to bracket each PHC read with
system clock reads. Some PHYs have a PTP hardware clock (PHC) that is
read over MDIO and sometimes multiple MDIO transfers are needed to read
the clock. This leads to brackets in the tens of microseconds.
Applications that synchronize the system clock from the PHC typically
use the midpoint of the bracket as the system time of the PHC reading.
With multiple transfers, the midpoint of the bracket is typically far
from the time when the PHC reading actually occurred.

For example, using PTP_SYS_OFFSET_EXTENDED on the Raspberry Pi CM5 to
synchronize the system clock from the BCM54210PE PHY PHC leads to the
system clock being 32 µs ahead of the PHC. On the CM4, it is about 7 µs.
(The difference is because the MDIO clock frequency is higher on the
CM4: 10 MHz as opposed to 2.08 MHz on the CM5.) The brackets are about
74 µs on the CM5 and 42 µs on the CM4. In comparison, the MAC PHC
on the CM5 has brackets of about 1 µs. See below for measurement
process.

This series introduces a new MDIO operation that allows PHY drivers to
ask MDIO bus drivers to compute a bracket for the completion of an MDIO
write transfer. It then makes bcm-phy-ptp use this operation, and
implements it for both the macb and UniMAC drivers. A bus driver
computes a bracket for completion of a transfer by shifting the bracket
for the start of the MDIO transfer by bounds on its duration.

The effect of this series is to reduce PTP_SYS_OFFSET_EXTENDED brackets
from 74 µs to about 1.3 µs on the CM5, and from 42 µs to about 0.9 µs on
the CM4. The synchronization error drops from 32 µs and 7 µs to tenths
of microseconds, below the level that can be reliably measured with the
methods below.

Note that SPI introduced a similar facility to allow a device driver to
obtain timestamp bounds from the bus controller in commit 79591b7db21d
("spi: Add a PTP system timestamp to the transfer structure"). This
series implements the operation for only one PHY driver. But I surveyed
other PHY drivers and identified at least two for which this operation
could be implemented: micrel (specifically LAN8814) and dp83640,
although they would need updating to use gettimex64 first.

The system clock synchronization accuracy can be measured on the CM5 by
making use of its second PHC. /dev/ptp0 is the BCM54210PE PHC; /dev/ptp1
is the macb MAC PHC. PTP_SYS_OFFSET_EXTENDED brackets with macb are
about 1 µs after upstream commit 9ca4ba242591 ("net: macb: fix ordering
around PTP timestamp read"). So we can use the following measurement
approach: select /dev/ptp1 as the PHC for packet timestamping and then
synchronize it to a PTP grandmaster; synchronize /dev/ptp0 to a PPS from
a GPS using ts2phc or satpulse; synchronize the system clock from
/dev/ptp1 using phc2sys or chrony; then measure the offset between the
system clock and the result of PTP_SYS_OFFSET_EXTENDED.

An alternative way to check, which does not require a PTP grandmaster,
is to connect the same PPS signal to both the GPIO PPS pin (header pin
12) and to SYNC_OUT. Then have an NTP server discipline the system clock
using the GPS PPS signal, and as before measure the offset between the
system clock and the result of PTP_SYS_OFFSET_EXTENDED. But kernel PPS
has a significant bias (of the order of 10 µs). However, I have
developed a tool called ppsbias (https://github.com/jclark/ppsbias) to
measure this, which works by polling GPIO memory. If you configure the
NTP server with the offset from ppsbias, then you can get an estimate
that is accurate to within about 1 µs. The results from ppsbias are
consistent with results from using PTP with MAC PHC. On a CM4, this is
the only method available, since /dev/ptp1 is not available.

James Clark (5):
  net: mdio: add timestamped write operation
  net: phy: broadcom: use timestamped MDIO writes in gettimex64
  ptp: add functions to adjust system timestamps
  net: macb: implement timestamped MDIO writes
  net: mdio: bcm-unimac: implement timestamped MDIO writes

 drivers/net/ethernet/cadence/macb.h      |   1 +
 drivers/net/ethernet/cadence/macb_main.c |  95 ++++++++++++++++++--
 drivers/net/mdio/mdio-bcm-unimac.c       | 105 ++++++++++++++++++++++-
 drivers/net/mdio/mdio-mux.c              |  27 ++++++
 drivers/net/phy/bcm-phy-lib.c            |  25 ++++++
 drivers/net/phy/bcm-phy-lib.h            |   7 ++
 drivers/net/phy/bcm-phy-ptp.c            |  27 ++++--
 drivers/net/phy/mdio_bus.c               | 100 +++++++++++++++++++++
 include/linux/mdio.h                     |   4 +
 include/linux/phy.h                      |  37 ++++++++
 include/linux/ptp_clock_kernel.h         |  48 +++++++++++
 11 files changed, 459 insertions(+), 17 deletions(-)


base-commit: 45ad84d2800e4a092fb8d96006a533b2d0ab13f6
-- 
2.56.0

[PATCH net-next 1/5] net: mdio: add timestamped write operation

[James Clark]

Add an optional write_sts bus operation returning system timestamp
bounds for completion of an MDIO write. This allows PHY drivers to
obtain tighter bounds when implementing gettimex64. SPI introduced a
similar facility to allow a device driver to obtain timestamp bounds
from the bus controller in commit 79591b7db21d ("spi: Add a PTP system
timestamp to the transfer structure").

The core provides no fallback, because a bus driver's write may return
before the transfer has completed.

Signed-off-by: James Clark <jjc@jclark.com>
Assisted-by: LLM
---
 drivers/net/mdio/mdio-mux.c |  27 ++++++++++
 drivers/net/phy/mdio_bus.c  | 100 ++++++++++++++++++++++++++++++++++++
 include/linux/mdio.h        |   4 ++
 include/linux/phy.h         |  37 +++++++++++++
 4 files changed, 168 insertions(+)

diff --git a/drivers/net/mdio/mdio-mux.c b/drivers/net/mdio/mdio-mux.c
index fe0e46bd796..ed463a087ce 100644
--- a/drivers/net/mdio/mdio-mux.c
+++ b/drivers/net/mdio/mdio-mux.c
@@ -123,6 +123,31 @@ static int mdio_mux_write_c45(struct mii_bus *bus, int phy_id, int dev_addr,
 	return r;
 }
 
+static int mdio_mux_write_sts(struct mii_bus *bus, int phy_id, int regnum,
+			      u16 val, struct ptp_system_timestamp *sts)
+{
+	struct mdio_mux_child_bus *cb = bus->priv;
+	struct mdio_mux_parent_bus *pb = cb->parent;
+
+	int r;
+
+	mutex_lock_nested(&pb->mii_bus->mdio_lock, MDIO_MUTEX_MUX);
+	r = pb->switch_fn(pb->current_child, cb->bus_number, pb->switch_data);
+	/* write_sts must not return -EOPNOTSUPP. */
+	if (r == -EOPNOTSUPP)
+		r = -EIO;
+	if (r)
+		goto out;
+
+	pb->current_child = cb->bus_number;
+
+	r = pb->mii_bus->write_sts(pb->mii_bus, phy_id, regnum, val, sts);
+out:
+	mutex_unlock(&pb->mii_bus->mdio_lock);
+
+	return r;
+}
+
 static int parent_count;
 
 static void mdio_mux_uninit_children(struct mdio_mux_parent_bus *pb)
@@ -222,6 +247,8 @@ int mdio_mux_init(struct device *dev,
 			cb->mii_bus->read_c45 = mdio_mux_read_c45;
 		if (parent_bus->write_c45)
 			cb->mii_bus->write_c45 = mdio_mux_write_c45;
+		if (parent_bus->write_sts)
+			cb->mii_bus->write_sts = mdio_mux_write_sts;
 		r = of_mdiobus_register(cb->mii_bus, child_bus_node);
 		if (r) {
 			mdiobus_free(cb->mii_bus);
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 00d0e4159e9..8e734227b2c 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -18,6 +18,7 @@
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/phy.h>
+#include <linux/ptp_clock_kernel.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/string.h>
@@ -145,6 +146,105 @@ int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
 }
 EXPORT_SYMBOL(__mdiobus_write);
 
+/**
+ * __mdiobus_write_sts - Timestamped version of the __mdiobus_write function
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @regnum: register number to write
+ * @val: value to write to @regnum
+ * @sts: system timestamps bounding completion, or NULL
+ *
+ * Return: Zero if successful, negative error code on failure. Returns
+ *	   -EBUSY or -EINVAL if the system timestamps are not valid. If @sts
+ *	   is not NULL, -EOPNOTSUPP is returned only if
+ *	   mdiobus_supports_write_sts() is false.
+ *
+ * Write a MDIO bus register, with system timestamps bounding completion;
+ * a transfer is considered complete on the rising edge of the MDC
+ * that clocks the last data bit. Caller must hold the mdio bus lock.
+ *
+ * For clocks that can be stepped, validate the clock generation through
+ * the raw time of the upper bound.
+ *
+ * If @sts is NULL, perform an ordinary write.
+ *
+ * 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)
+{
+	struct system_time_snapshot now;
+	ktime_t deadline;
+	int err;
+
+	if (!sts)
+		return __mdiobus_write(bus, addr, regnum, val);
+
+	lockdep_assert_held_once(&bus->mdio_lock);
+
+	if (addr >= PHY_MAX_ADDR)
+		return -ENXIO;
+
+	if (bus->write_sts)
+		err = bus->write_sts(bus, addr, regnum, val, sts);
+	else
+		err = -EOPNOTSUPP;
+
+	trace_mdio_access(bus, 0, addr, regnum, val, err);
+	mdiobus_stats_acct(&bus->stats[addr], false, err);
+
+	if (err)
+		return err;
+
+	if (!sts->pre_sts.valid || !sts->post_sts.valid)
+		return -EINVAL;
+
+	if (sts->clockid == CLOCK_MONOTONIC ||
+	    sts->clockid == CLOCK_MONOTONIC_RAW)
+		return 0;
+
+	/* Fail if the clock was stepped; callers must retry anyway. */
+	if (sts->pre_sts.clock_was_set_seq != sts->post_sts.clock_was_set_seq)
+		return -EBUSY;
+
+	/* The shifted upper bound can be later than actual completion. */
+	deadline = ktime_add_ns(ktime_get_raw(), NSEC_PER_MSEC);
+	for (;;) {
+		ktime_get_snapshot_id(sts->clockid, &now);
+		if (!now.valid)
+			return -EINVAL;
+
+		if (now.clock_was_set_seq != sts->pre_sts.clock_was_set_seq)
+			return -EBUSY;
+
+		if (!ktime_before(now.monoraw, sts->post_sts.monoraw))
+			return 0;
+
+		/* Cap the wait at 1 ms, which is more than any single
+		 * write's delay. This guards against an aux clock being
+		 * disabled and reenabled, which restarts its raw time.
+		 */
+		if (!ktime_before(ktime_get_raw(), deadline))
+			return -EBUSY;
+
+		cpu_relax();
+	}
+}
+EXPORT_SYMBOL_GPL(__mdiobus_write_sts);
+
+/**
+ * mdiobus_supports_write_sts - Check for timestamped write support
+ * @bus: the mii_bus struct
+ *
+ * Return: true if __mdiobus_write_sts() can return system timestamps
+ * for writes on @bus.
+ */
+bool mdiobus_supports_write_sts(struct mii_bus *bus)
+{
+	return bus->write_sts;
+}
+EXPORT_SYMBOL_GPL(mdiobus_supports_write_sts);
+
 /**
  * __mdiobus_modify_changed - Unlocked version of the mdiobus_modify function
  * @bus: the mii_bus struct
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index a7d9e3ae362..07b6b0c5f8f 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -11,6 +11,7 @@
 
 struct gpio_desc;
 struct mii_bus;
+struct ptp_system_timestamp;
 struct reset_control;
 
 /* Multiple levels of nesting are possible. However typically this is
@@ -574,6 +575,9 @@ static inline void mii_c73_mod_linkmode(unsigned long *adv, u16 *lpa)
 
 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);
+bool mdiobus_supports_write_sts(struct mii_bus *bus);
 int __mdiobus_modify(struct mii_bus *bus, int addr, u32 regnum, u16 mask,
 		     u16 set);
 int __mdiobus_modify_changed(struct mii_bus *bus, int addr, u32 regnum,
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7c5098a0dd6..d4f57c9bb7a 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -374,6 +374,14 @@ struct mii_bus {
 	/** @write_c45: Perform a C45 write transfer on the bus */
 	int (*write_c45)(struct mii_bus *bus, int addr, int devnum,
 			 int regnum, u16 val);
+	/**
+	 * @write_sts: Perform a write transfer on the bus,
+	 * with system timestamps bounding its completion. Set only
+	 * if timestamps can always be provided. Must not return
+	 * -EOPNOTSUPP.
+	 */
+	int (*write_sts)(struct mii_bus *bus, int addr, int regnum, u16 val,
+			 struct ptp_system_timestamp *sts);
 	/** @reset: Perform a reset of the bus */
 	int (*reset)(struct mii_bus *bus);
 	/**
@@ -1792,6 +1800,35 @@ static inline int __phy_write(struct phy_device *phydev, u32 regnum, u16 val)
 			       val);
 }
 
+/**
+ * phy_supports_write_sts - Check for timestamped PHY register writes
+ * @phydev: the phy_device struct
+ *
+ * Return: true if __phy_write_sts() can return system timestamps.
+ */
+static inline bool phy_supports_write_sts(struct phy_device *phydev)
+{
+	return mdiobus_supports_write_sts(phydev->mdio.bus);
+}
+
+/**
+ * __phy_write_sts - Write a PHY register with a frame-end timestamp
+ * @phydev: the phy_device struct
+ * @regnum: clause 22 register number
+ * @val: value to write
+ * @sts: system timestamp bounds, or NULL
+ *
+ * Return: As for __mdiobus_write_sts().
+ *
+ * The caller must hold the MDIO bus lock.
+ */
+static inline int __phy_write_sts(struct phy_device *phydev, u32 regnum, u16 val,
+				  struct ptp_system_timestamp *sts)
+{
+	return __mdiobus_write_sts(phydev->mdio.bus, phydev->mdio.addr,
+				   regnum, val, sts);
+}
+
 /**
  * __phy_modify_changed() - Convenience function for modifying a PHY register
  * @phydev: a pointer to a &struct phy_device
-- 
2.56.0

[PATCH net-next 2/5] net: phy: broadcom: use timestamped MDIO writes in gettimex64

[James Clark]

Use __phy_write_sts() to obtain tighter system timestamp bounds in
gettimex64 by timestamping completion of the write that triggers
capture of the PHC time.

The PHY captures its time a short delay after the end of the write.
End-to-end measurements show that the delay is small, within the
measurement accuracy of a few tenths of a microsecond.

This patch does not try to fix the error handling in the existing
framesync code, which ignores MDIO write errors.

Signed-off-by: James Clark <jjc@jclark.com>
Assisted-by: LLM
---
Does Broadcom know the delay from the end of the MDIO write to the
framesync capture in the BCM54210PE? If so, I can add compensation
for it.

 drivers/net/phy/bcm-phy-lib.c | 25 +++++++++++++++++++++++++
 drivers/net/phy/bcm-phy-lib.h |  7 +++++++
 drivers/net/phy/bcm-phy-ptp.c | 27 +++++++++++++++++++++------
 3 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index b64beade8dd..022f5e5e6ce 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -42,6 +42,31 @@ int bcm_phy_write_exp(struct phy_device *phydev, u16 reg, u16 val)
 }
 EXPORT_SYMBOL_GPL(bcm_phy_write_exp);
 
+static int __bcm_phy_write_exp_sts(struct phy_device *phydev, u16 reg, u16 val,
+				   struct ptp_system_timestamp *sts)
+{
+	int rc;
+
+	rc = __phy_write(phydev, MII_BCM54XX_EXP_SEL, reg);
+	if (rc < 0)
+		return rc;
+
+	return __phy_write_sts(phydev, MII_BCM54XX_EXP_DATA, val, sts);
+}
+
+int bcm_phy_write_exp_sts(struct phy_device *phydev, u16 reg, u16 val,
+			  struct ptp_system_timestamp *sts)
+{
+	int rc;
+
+	phy_lock_mdio_bus(phydev);
+	rc = __bcm_phy_write_exp_sts(phydev, reg, val, sts);
+	phy_unlock_mdio_bus(phydev);
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(bcm_phy_write_exp_sts);
+
 int __bcm_phy_read_exp(struct phy_device *phydev, u16 reg)
 {
 	int val;
diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
index bba94ce9619..365cb3d1c32 100644
--- a/drivers/net/phy/bcm-phy-lib.h
+++ b/drivers/net/phy/bcm-phy-lib.h
@@ -34,6 +34,8 @@ int __bcm_phy_write_exp(struct phy_device *phydev, u16 reg, u16 val);
 int __bcm_phy_read_exp(struct phy_device *phydev, u16 reg);
 int __bcm_phy_modify_exp(struct phy_device *phydev, u16 reg, u16 mask, u16 set);
 int bcm_phy_write_exp(struct phy_device *phydev, u16 reg, u16 val);
+int bcm_phy_write_exp_sts(struct phy_device *phydev, u16 reg, u16 val,
+			  struct ptp_system_timestamp *sts);
 int bcm_phy_read_exp(struct phy_device *phydev, u16 reg);
 int bcm_phy_modify_exp(struct phy_device *phydev, u16 reg, u16 mask, u16 set);
 
@@ -48,6 +50,11 @@ static inline int bcm_phy_read_exp_sel(struct phy_device *phydev, u16 reg)
 	return bcm_phy_read_exp(phydev, reg | MII_BCM54XX_EXP_SEL_ER);
 }
 
+static inline bool bcm_phy_supports_write_exp_sts(struct phy_device *phydev)
+{
+	return phy_supports_write_sts(phydev);
+}
+
 int bcm54xx_auxctl_write(struct phy_device *phydev, u16 regnum, u16 val);
 int bcm54xx_auxctl_read(struct phy_device *phydev, u16 regnum);
 
diff --git a/drivers/net/phy/bcm-phy-ptp.c b/drivers/net/phy/bcm-phy-ptp.c
index 65d609ed69f..2d8b377010b 100644
--- a/drivers/net/phy/bcm-phy-ptp.c
+++ b/drivers/net/phy/bcm-phy-ptp.c
@@ -214,22 +214,34 @@ static void bcm_ptp_framesync(struct phy_device *phydev, u16 ctrl)
 	bcm_phy_write_exp(phydev, NSE_CTRL, ctrl | NSE_CPU_FRAMESYNC);
 }
 
+static int bcm_ptp_framesync_sts(struct phy_device *phydev, u16 ctrl,
+				 struct ptp_system_timestamp *sts)
+{
+	return bcm_phy_write_exp_sts(phydev, NSE_CTRL,
+				     ctrl | NSE_CPU_FRAMESYNC, sts);
+}
+
 static int bcm_ptp_framesync_ts(struct phy_device *phydev,
 				struct ptp_system_timestamp *sts,
 				struct timespec64 *ts,
 				u16 orig_ctrl)
 {
 	u16 ctrl, reg;
-	int i;
+	int i, err = 0;
 
 	ctrl = bcm_ptp_framesync_disable(phydev, orig_ctrl);
 
-	ptp_read_system_prets(sts);
-
 	/* trigger framesync + capture */
-	bcm_ptp_framesync(phydev, ctrl | NSE_CAPTURE_EN);
-
-	ptp_read_system_postts(sts);
+	if (sts && bcm_phy_supports_write_exp_sts(phydev)) {
+		/* cannot ignore error since sts may be uninitialized,
+		 * but still poll for any triggered capture
+		 */
+		err = bcm_ptp_framesync_sts(phydev, ctrl | NSE_CAPTURE_EN, sts);
+	} else {
+		ptp_read_system_prets(sts);
+		bcm_ptp_framesync(phydev, ctrl | NSE_CAPTURE_EN);
+		ptp_read_system_postts(sts);
+	}
 
 	/* poll for FSYNC interrupt from TS capture */
 	for (i = 0; i < 10; i++) {
@@ -242,6 +254,9 @@ static int bcm_ptp_framesync_ts(struct phy_device *phydev,
 
 	bcm_ptp_framesync_restore(phydev, orig_ctrl);
 
+	if (err)
+		return err;
+
 	return reg & INTC_FSYNC ? 0 : -ETIMEDOUT;
 }
 
-- 
2.56.0

[PATCH net-next 3/5] ptp: add functions to adjust system timestamps

[James Clark]

Add ptp_adjust_system_prets() and ptp_adjust_system_postts(), which
shift the pre or post system timestamp by a given duration.

An MDIO bus driver can take system timestamps around the command that
starts a write, and knows how long after that command the write
completes, so it can shift the timestamps to bound the completion of
the write.

Signed-off-by: James Clark <jjc@jclark.com>
Assisted-by: LLM
---
The time of the selected clock is moved by the CLOCK_MONOTONIC_RAW
duration without scaling it for that clock's frequency correction,
because timekeeping has no interface for that. While the clock is
synchronized the error is negligible: under 2 ns for a 50 ppm
correction over the 30 us MDIO write time in this series. It is larger
only while the clock is being slewed. An interface along these lines
would allow it to be done properly:

	/* Scale a CLOCK_MONOTONIC_RAW duration to the rate of @clock_id */
	u64 ktime_scale_raw_ns(clockid_t clock_id, u64 ns);

 include/linux/ptp_clock_kernel.h | 48 ++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 36a27a91059..b4ac64228f1 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -520,4 +520,52 @@ static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
 		ktime_get_snapshot_id(sts->clockid, &sts->post_sts);
 }
 
+static inline void __ptp_adjust_snapshot(struct system_time_snapshot *snap,
+					 s64 ns)
+{
+	if (!snap->valid)
+		return;
+	snap->systime = ktime_add_ns(snap->systime, ns);
+	snap->monoraw = ktime_add_ns(snap->monoraw, ns);
+	/* No counter value corresponds to the adjusted times. */
+	snap->cycles = 0;
+	snap->cs_id = CSID_GENERIC;
+	snap->hw_cycles = 0;
+	snap->hw_csid = CSID_GENERIC;
+}
+
+/**
+ * ptp_adjust_system_prets - Shift the lower system timestamp bound
+ * @sts: system timestamps, or NULL
+ * @ns: CLOCK_MONOTONIC_RAW nanoseconds to add
+ *
+ * Add @ns to the selected clock's time and to the CLOCK_MONOTONIC_RAW
+ * time of the lower bound, and clear its clocksource counter values.
+ * @ns is not scaled for the selected clock's frequency correction, so
+ * the bound is off by @ns times that correction.
+ */
+static inline void ptp_adjust_system_prets(struct ptp_system_timestamp *sts,
+					   s64 ns)
+{
+	if (sts)
+		__ptp_adjust_snapshot(&sts->pre_sts, ns);
+}
+
+/**
+ * ptp_adjust_system_postts - Shift the upper system timestamp bound
+ * @sts: system timestamps, or NULL
+ * @ns: CLOCK_MONOTONIC_RAW nanoseconds to add
+ *
+ * Add @ns to the selected clock's time and to the CLOCK_MONOTONIC_RAW
+ * time of the upper bound, and clear its clocksource counter values.
+ * @ns is not scaled for the selected clock's frequency correction, so
+ * the bound is off by @ns times that correction.
+ */
+static inline void ptp_adjust_system_postts(struct ptp_system_timestamp *sts,
+					    s64 ns)
+{
+	if (sts)
+		__ptp_adjust_snapshot(&sts->post_sts, ns);
+}
+
 #endif
-- 
2.56.0

[PATCH net-next 4/5] net: macb: implement timestamped MDIO writes

[James Clark]

Implement write_sts to provide system timestamp bounds for completion
of an MDIO write. This enables tighter system timestamp bounds in PHY
implementations of gettimex64.

Take system timestamps around the command register write, then add
the time from the command write until the MDC edge that clocks the
last data bit, calculated from the peripheral clock rate and the
configured MDC divider.

On RP1 that edge comes 63.5 MDC periods after the command write. This
was measured on a Raspberry Pi CM5 with its BCM54210PE PHY. The PHY's
PHC was read with PTP_SYS_OFFSET_EXTENDED while the MDC divisor was
switched between 48 and 128 through /dev/mem. Any error in the number
of periods would make the PHC offset jump at each switch. After
compensating for drift, the jump with 63.5 periods was under 2 ns.
The MDC divider does not appear to run freely: the spread of the PHC
offsets did not change with the divisor.

Signed-off-by: James Clark <jjc@jclark.com>
Assisted-by: LLM
---
The 63.5-period delay has been measured only on RP1, but the code
assumes it holds for all MACB and GEM variants. I am not sure whether
that is a reasonable assumption; if not, write_sts could be enabled
only for RP1.

 drivers/net/ethernet/cadence/macb.h      |  1 +
 drivers/net/ethernet/cadence/macb_main.c | 95 ++++++++++++++++++++++--
 2 files changed, 88 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 1cb2778fe49..3670731690f 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -1334,6 +1334,7 @@ struct macb {
 	struct macb_or_gem_ops	macbgem_ops;
 
 	struct mii_bus		*mii_bus;
+	unsigned long		mdio_sts_rate;
 	struct phylink		*phylink;
 	struct phylink_config	phylink_config;
 	struct phylink_pcs	phylink_usx_pcs;
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index c4223ca848d..cec6e5a48e9 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -384,11 +384,47 @@ static int macb_mdio_read_c45(struct mii_bus *bus, int mii_id, int devad,
 	return status;
 }
 
-static int macb_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
-			       u16 value)
+static u64 macb_mdio_sts_delay(struct macb *bp)
+{
+	static const u16 gem_divisors[] = {
+		[GEM_CLK_DIV8] = 8,
+		[GEM_CLK_DIV16] = 16,
+		[GEM_CLK_DIV32] = 32,
+		[GEM_CLK_DIV48] = 48,
+		[GEM_CLK_DIV64] = 64,
+		[GEM_CLK_DIV96] = 96,
+		[GEM_CLK_DIV128] = 128,
+		[GEM_CLK_DIV224] = 224,
+	};
+	static const u16 macb_divisors[] = {
+		[MACB_CLK_DIV8] = 8,
+		[MACB_CLK_DIV16] = 16,
+		[MACB_CLK_DIV32] = 32,
+		[MACB_CLK_DIV64] = 64,
+	};
+	unsigned long rate = READ_ONCE(bp->mdio_sts_rate);
+	u32 config = macb_readl(bp, NCFGR);
+	u32 divisor;
+
+	if (macb_is_gem(bp))
+		divisor = gem_divisors[GEM_BFEXT(CLK, config)];
+	else
+		divisor = macb_divisors[MACB_BFEXT(CLK, config)];
+
+	/* On RP1 the MDC edge that clocks the last bit of a clause 22
+	 * write comes 63.5 periods after the command write.
+	 */
+	return div64_ul(127ULL * divisor * NSEC_PER_SEC, 2 * rate);
+}
+
+static int macb_mdio_write_c22_sts(struct mii_bus *bus, int mii_id, int regnum,
+				   u16 value, struct ptp_system_timestamp *sts)
 {
 	struct macb *bp = bus->priv;
+	unsigned long flags;
+	u64 delay_ns;
 	int status;
+	u32 cmd;
 
 	status = pm_runtime_resume_and_get(&bp->pdev->dev);
 	if (status < 0)
@@ -398,12 +434,32 @@ static int macb_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
 	if (status < 0)
 		goto mdio_write_exit;
 
-	macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C22_SOF)
-			      | MACB_BF(RW, MACB_MAN_C22_WRITE)
-			      | MACB_BF(PHYA, mii_id)
-			      | MACB_BF(REGA, regnum)
-			      | MACB_BF(CODE, MACB_MAN_C22_CODE)
-			      | MACB_BF(DATA, value)));
+	cmd = MACB_BF(SOF, MACB_MAN_C22_SOF)
+	      | MACB_BF(RW, MACB_MAN_C22_WRITE)
+	      | MACB_BF(PHYA, mii_id)
+	      | MACB_BF(REGA, regnum)
+	      | MACB_BF(CODE, MACB_MAN_C22_CODE)
+	      | MACB_BF(DATA, value);
+
+	if (sts) {
+		delay_ns = macb_mdio_sts_delay(bp);
+		local_irq_save(flags);
+		ptp_read_system_prets(sts);
+		/* macb_writel() is relaxed; order it after the timestamp. */
+		mb();
+	}
+	macb_writel(bp, MAN, cmd);
+	if (sts) {
+		/* Flush the posted write before taking the upper bound. */
+		macb_readl(bp, NSR);
+		/* Order the read-back before the system timestamp. */
+		rmb();
+		ptp_read_system_postts(sts);
+		local_irq_restore(flags);
+
+		ptp_adjust_system_prets(sts, delay_ns);
+		ptp_adjust_system_postts(sts, delay_ns);
+	}
 
 	status = macb_mdio_wait_for_idle(bp);
 	if (status < 0)
@@ -415,6 +471,12 @@ static int macb_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
 	return status;
 }
 
+static int macb_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
+			       u16 value)
+{
+	return macb_mdio_write_c22_sts(bus, mii_id, regnum, value, NULL);
+}
+
 static int macb_mdio_write_c45(struct mii_bus *bus, int mii_id,
 			       int devad, int regnum,
 			       u16 value)
@@ -1141,6 +1203,13 @@ static int macb_mdiobus_register(struct macb *bp, struct device_node *mdio_np)
 	return mdiobus_register(bp->mii_bus);
 }
 
+static bool macb_mdio_init_sts(struct macb *bp)
+{
+	bp->mdio_sts_rate = clk_get_rate(bp->pclk);
+
+	return bp->mdio_sts_rate != 0;
+}
+
 static int macb_mii_init(struct macb *bp)
 {
 	struct device_node *mdio_np, *np = bp->pdev->dev.of_node;
@@ -1166,6 +1235,8 @@ static int macb_mii_init(struct macb *bp)
 	bp->mii_bus->name = "MACB_mii_bus";
 	bp->mii_bus->read = &macb_mdio_read_c22;
 	bp->mii_bus->write = &macb_mdio_write_c22;
+	if (macb_mdio_init_sts(bp))
+		bp->mii_bus->write_sts = &macb_mdio_write_c22_sts;
 	bp->mii_bus->read_c45 = &macb_mdio_read_c45;
 	bp->mii_bus->write_c45 = &macb_mdio_write_c45;
 	snprintf(bp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
@@ -3095,12 +3166,20 @@ static void macb_configure_dma(struct macb *bp)
 
 static void macb_init_hw(struct macb *bp)
 {
+	unsigned long rate;
 	u32 config;
 
 	macb_reset_hw(bp);
 	macb_set_hwaddr(bp);
 
 	config = macb_mdc_clk_div(bp);
+	/* Record the pclk rate the MDC divider is chosen from, for
+	 * write_sts, which can't call clk_get_rate() under the MDIO bus
+	 * lock.
+	 */
+	rate = clk_get_rate(bp->pclk);
+	if (rate)
+		WRITE_ONCE(bp->mdio_sts_rate, rate);
 	/* Make eth data aligned.
 	 * If RSC capable, that offset is ignored by HW.
 	 */
-- 
2.56.0

[PATCH net-next 5/5] net: mdio: bcm-unimac: implement timestamped MDIO writes

[James Clark]

Implement write_sts to provide system timestamp bounds for completion
of an MDIO write. This enables tighter system timestamp bounds in PHY
implementations of gettimex64.

Take system timestamps around the command start, then add the time
from the command start until the MDC edge that clocks the last data
bit, calculated from the reference clock rate and the configured MDC
divider.

On BCM2711 that edge comes 64 MDC periods after the command start, on
average. This was measured on a Raspberry Pi CM4 with its BCM54210PE
PHY. The PHY's PHC was read with PTP_SYS_OFFSET_EXTENDED while the MDC
divider was switched between 9 and 39 through /dev/mem. Any error in
the number of periods would make the PHC offset jump at each switch.
After compensating for drift, the jump with 64 periods was under 10 ns.
The MDC divider appears to run freely: the PHC offsets spread evenly
over one MDC period at each divider. So use 63.5 and 64.5 periods as
the lower and upper bounds of the delay.

Use a 200 MHz reference rate for BCM2711 GENET, whose clock is not
described in DT.

Signed-off-by: James Clark <jjc@jclark.com>
Assisted-by: LLM
---
When there is no clock, unimac_mdio_clk_set() assumes a 250 MHz
reference rate, and no in-tree DT gives GENET or UniMAC a clock. For
BCM2711 I have no documentation of the reference rate or of the clock
that supplies it, but MDIO busy times measured at several MDC dividers
fit a rate of 200 MHz. This patch checks the parent's compatible string,
which is unsatisfactory. I would prefer to get the rate from DT, and
would welcome suggestions for the right DT description.

The delay of 63.5 to 64.5 MDC periods has been measured only on
BCM2711, but the code assumes it holds for any UniMAC.

 drivers/net/mdio/mdio-bcm-unimac.c | 105 ++++++++++++++++++++++++++++-
 1 file changed, 102 insertions(+), 3 deletions(-)

diff --git a/drivers/net/mdio/mdio-bcm-unimac.c b/drivers/net/mdio/mdio-bcm-unimac.c
index 31e396cc9fb..e1ae806d1a8 100644
--- a/drivers/net/mdio/mdio-bcm-unimac.c
+++ b/drivers/net/mdio/mdio-bcm-unimac.c
@@ -16,6 +16,7 @@
 #include <linux/phy.h>
 #include <linux/platform_data/mdio-bcm-unimac.h>
 #include <linux/platform_device.h>
+#include <linux/ptp_clock_kernel.h>
 #include <linux/sched.h>
 
 #define MDIO_CMD		0x00
@@ -42,6 +43,7 @@ struct unimac_mdio_priv {
 	void			*wait_func_data;
 	struct clk		*clk;
 	u32			clk_freq;
+	unsigned long		mdio_ref_rate;
 };
 
 static inline u32 unimac_mdio_readl(struct unimac_mdio_priv *priv, u32 offset)
@@ -73,6 +75,27 @@ static inline void unimac_mdio_start(struct unimac_mdio_priv *priv)
 	unimac_mdio_writel(priv, reg, MDIO_CMD);
 }
 
+static void unimac_mdio_start_sts(struct unimac_mdio_priv *priv,
+				  struct ptp_system_timestamp *sts)
+{
+	unsigned long flags;
+	u32 reg;
+
+	reg = unimac_mdio_readl(priv, MDIO_CMD);
+	reg |= MDIO_START_BUSY;
+	local_irq_save(flags);
+	ptp_read_system_prets(sts);
+	/* Order the timestamp before the relaxed command write. */
+	mb();
+	unimac_mdio_writel(priv, reg, MDIO_CMD);
+	/* Flush the posted write before taking the upper bound. */
+	unimac_mdio_readl(priv, MDIO_CMD);
+	/* Order the read-back before the system timestamp. */
+	rmb();
+	ptp_read_system_postts(sts);
+	local_irq_restore(flags);
+}
+
 static int unimac_mdio_poll(void *wait_func_data)
 {
 	struct unimac_mdio_priv *priv = wait_func_data;
@@ -127,10 +150,41 @@ static int unimac_mdio_read(struct mii_bus *bus, int phy_id, int reg)
 	return ret;
 }
 
-static int unimac_mdio_write(struct mii_bus *bus, int phy_id,
-			     int reg, u16 val)
+static int unimac_mdio_sts_delays(struct unimac_mdio_priv *priv,
+				  u64 *pre_ns, u64 *post_ns)
+{
+	u32 cmd, config, divisor;
+	int ret;
+
+	/* The delays assume the controller is idle. */
+	ret = read_poll_timeout(unimac_mdio_readl, cmd,
+				!(cmd & MDIO_START_BUSY),
+				1, 1000, false, priv, MDIO_CMD);
+	if (ret)
+		return ret;
+
+	config = unimac_mdio_readl(priv, MDIO_CFG);
+	if (config & MDIO_SUPP_PREAMBLE)
+		return -EIO;
+	divisor = 2 * (((config >> MDIO_CLK_DIV_SHIFT) & MDIO_CLK_DIV_MASK) + 1);
+	/* On BCM2711 the MDC divider runs freely, so the MDC edge that
+	 * clocks the last bit of a write comes 63.5 to 64.5 periods after
+	 * the command start.
+	 */
+	*pre_ns = div64_ul(127ULL * divisor * NSEC_PER_SEC,
+			   2 * priv->mdio_ref_rate);
+	*post_ns = div64_ul(129ULL * divisor * NSEC_PER_SEC +
+			    2 * priv->mdio_ref_rate - 1,
+			    2 * priv->mdio_ref_rate);
+
+	return 0;
+}
+
+static int unimac_mdio_write_sts(struct mii_bus *bus, int phy_id, int reg,
+				 u16 val, struct ptp_system_timestamp *sts)
 {
 	struct unimac_mdio_priv *priv = bus->priv;
+	u64 pre_ns, post_ns;
 	u32 cmd;
 	int ret;
 
@@ -138,19 +192,38 @@ static int unimac_mdio_write(struct mii_bus *bus, int phy_id,
 	if (ret)
 		return ret;
 
+	if (sts) {
+		ret = unimac_mdio_sts_delays(priv, &pre_ns, &post_ns);
+		if (ret)
+			goto out;
+	}
+
 	/* Prepare the write operation */
 	cmd = MDIO_WR | (phy_id << MDIO_PMD_SHIFT) |
 		(reg << MDIO_REG_SHIFT) | (0xffff & val);
 	unimac_mdio_writel(priv, cmd, MDIO_CMD);
 
-	unimac_mdio_start(priv);
+	if (sts) {
+		unimac_mdio_start_sts(priv, sts);
+		ptp_adjust_system_prets(sts, pre_ns);
+		ptp_adjust_system_postts(sts, post_ns);
+	} else {
+		unimac_mdio_start(priv);
+	}
 
 	ret = priv->wait_func(priv->wait_func_data);
+out:
 	clk_disable_unprepare(priv->clk);
 
 	return ret;
 }
 
+static int unimac_mdio_write(struct mii_bus *bus, int phy_id,
+			     int reg, u16 val)
+{
+	return unimac_mdio_write_sts(bus, phy_id, reg, val, NULL);
+}
+
 /* Workaround for integrated BCM7xxx Gigabit PHYs which have a problem with
  * their internal MDIO management controller making them fail to successfully
  * be read from or written to for the first transaction.  We insert a dummy
@@ -234,6 +307,30 @@ static int unimac_mdio_clk_set(struct unimac_mdio_priv *priv)
 	return ret;
 }
 
+static bool unimac_mdio_init_sts(struct unimac_mdio_priv *priv,
+				 struct device *dev)
+{
+	u32 config;
+
+	/* The reference rate is fixed, so read it once. */
+	priv->mdio_ref_rate = clk_get_rate(priv->clk);
+	/* BCM2711's 200 MHz GENET reference clock is not described in DT. */
+	if (!priv->mdio_ref_rate && dev->parent &&
+	    of_device_is_compatible(dev->parent->of_node,
+				    "brcm,bcm2711-genet-v5"))
+		priv->mdio_ref_rate = 200000000;
+
+	if (!priv->mdio_ref_rate)
+		return false;
+
+	if (clk_prepare_enable(priv->clk))
+		return false;
+	config = unimac_mdio_readl(priv, MDIO_CFG);
+	clk_disable_unprepare(priv->clk);
+
+	return !(config & MDIO_SUPP_PREAMBLE);
+}
+
 static int unimac_mdio_probe(struct platform_device *pdev)
 {
 	struct unimac_mdio_pdata *pdata = pdev->dev.platform_data;
@@ -292,6 +389,8 @@ static int unimac_mdio_probe(struct platform_device *pdev)
 	bus->parent = &pdev->dev;
 	bus->read = unimac_mdio_read;
 	bus->write = unimac_mdio_write;
+	if (unimac_mdio_init_sts(priv, &pdev->dev))
+		bus->write_sts = unimac_mdio_write_sts;
 	bus->reset = unimac_mdio_reset;
 	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-%d", pdev->name, pdev->id);
 
-- 
2.56.0

Re: [PATCH net-next 5/5] net: mdio: bcm-unimac: implement timestamped MDIO writes

[Florian Fainelli]

On 10/9/26 07:35, James Clark wrote:
> Implement write_sts to provide system timestamp bounds for completion
> of an MDIO write. This enables tighter system timestamp bounds in PHY
> implementations of gettimex64.
> 
> Take system timestamps around the command start, then add the time
> from the command start until the MDC edge that clocks the last data
> bit, calculated from the reference clock rate and the configured MDC
> divider.
> 
> On BCM2711 that edge comes 64 MDC periods after the command start, on
> average. This was measured on a Raspberry Pi CM4 with its BCM54210PE
> PHY. The PHY's PHC was read with PTP_SYS_OFFSET_EXTENDED while the MDC
> divider was switched between 9 and 39 through /dev/mem. Any error in
> the number of periods would make the PHC offset jump at each switch.
> After compensating for drift, the jump with 64 periods was under 10 ns.
> The MDC divider appears to run freely: the PHC offsets spread evenly
> over one MDC period at each divider. So use 63.5 and 64.5 periods as
> the lower and upper bounds of the delay.
> 
> Use a 200 MHz reference rate for BCM2711 GENET, whose clock is not
> described in DT.
> 
> Signed-off-by: James Clark <jjc@jclark.com>
> Assisted-by: LLM
> ---
> When there is no clock, unimac_mdio_clk_set() assumes a 250 MHz
> reference rate, and no in-tree DT gives GENET or UniMAC a clock. For
> BCM2711 I have no documentation of the reference rate or of the clock
> that supplies it, but MDIO busy times measured at several MDC dividers
> fit a rate of 200 MHz. This patch checks the parent's compatible string,
> which is unsatisfactory. I would prefer to get the rate from DT, and
> would welcome suggestions for the right DT description.

You can, and should define a chip-specific compatible string for the 
MDIO contorller node. We have one for 2711 already for GENET 
(brcm,bcm2711-genet-v5), so you could define brcm,bcm2711-genet-mdio-v5

The clock frequency is fixed, so you could also provide a fixed clock to 
ensure that the clock frequency is derived correctly. I will check the 
actual clocking because 200MHz sounds odd to me, since the RGMII 
interface does require 125MHz and therefore a 250MHz source makes that easy.
-- 
Florian

Re: [PATCH net-next 5/5] net: mdio: bcm-unimac: implement timestamped MDIO writes

[James Clark]

On Fri, Oct 9, 2026 at 11:04 PM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
> The clock frequency is fixed, so you could also provide a fixed clock to
> ensure that the clock frequency is derived correctly. I will check the
> actual clocking because 200MHz sounds odd to me, since the RGMII
> interface does require 125MHz and therefore a 250MHz source makes that easy.

Thanks very much for looking at this.

Here is how I arrived at 200MHz. On BCM2711 (CM4) I polled START_BUSY
in MDIO_CMD with the MDC divider in MDIO_CFG set to 9, 14, 19, 29 and
39. The mean busy time of a write was 6.42, 9.66, 12.89, 19.30 and
25.63us. A clause 22 write clocks out 64 bits, so assuming it takes 64
MDC periods, the MDC period is 100, 151, 201, 302 and 400ns. The
comment in unimac_mdio_clk_set() gives the divider formula as MDC =
ref / (2 * (div + 1)), which implies in each case a reference rate of
about 200MHz.

Could it be that there is a separate 200MHz clock driving the MDIO?

James

Re: [PATCH net-next 0/5] net: mdio: add timestamped MDIO writes for PHY gettimex64

[James Clark]

On Fri, Oct 9, 2026 at 9:35 PM James Clark <jjc@jclark.com> wrote:

>  Some PHYs have a PTP hardware clock (PHC) that is
> read over MDIO and sometimes multiple MDIO transfers are needed to read
> the clock.
...
> This series introduces a new MDIO operation that allows PHY drivers to
> ask MDIO bus drivers to compute a bracket for the completion of an MDIO
> write transfer.

I have just become aware of an earlier discussion of a similar
problem, in 2019, about the PHC of mv88e6xxx switches:

https://lore.kernel.org/all/20190805082642.12873-1-hubert.feurstein@vahle.at/

In the relevant respects the situation is the same as with the BCM
PHY: the PHC is read over MDIO with several transfers, and one write
fixes the moment at which the clock is sampled. Richard and Andrew
suggested taking the system timestamps in the MDIO bus driver, around
that write, and Hubert Feurstein posted a series implementing this:

v1: https://lore.kernel.org/r/20190816163157.25314-1-h.feurstein@gmail.com
v2: https://lore.kernel.org/r/20190819172827.9550-1-hubert.feurstein@vahle.at
v3: https://lore.kernel.org/r/20190820084833.6019-1-hubert.feurstein@vahle.at

Like Hubert's v2 and v3, this series shifts the timestamps to allow
for the time the transfer takes. The discussion of that raised some
points, two of which this series already addresses. First, the meaning
of the timestamps returned by the MDIO core operation is clear: they
bound the end of the MDIO write transfer. Second, the bus driver uses
a lower and an upper bound on the transfer time, including the
preamble, which have been measured on hardware. The lower and upper
timestamps taken around the start of the transfer are shifted
separately, so that the returned bounds are guaranteed to contain the
end of the transfer.

The third point is the delay between the end of the transfer and the
device acting on the write, which I raised in the notes to patch 2. I
am making end-to-end measurements to establish a bound for this delay,
and I plan that in v2, in the absence of definitive data from
Broadcom, the PHY driver will shift post_ts by that bound.

This series also differs in having a write_sts operation, rather than
an sts pointer in struct mii_bus. This allows the bus driver to
compute the transfer time bounds at each call, from the same divider
and clock rate that it uses to set the MDC speed. Unlike Andrew's
suggestion, there is no fallback in the core, because a bus driver's
write may return before the transfer has completed.

The earlier discussion also raised interrupts arriving between the
timestamps and the write, and the latency of the posted write. In this
series, the bus drivers take the timestamps with local interrupts
disabled, and flush the posted write before taking the upper
timestamp.

James

Re: [PATCH net-next 1/5] net: mdio: add timestamped write operation

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] __mdiobus_write_sts() in drivers/net/phy/mdio_bus.c can return
  -EINVAL or -EBUSY after bus->write_sts() has already succeeded and the…
- [Low] In __mdiobus_write_sts() (drivers/net/phy/mdio_bus.c:223-226),
  the comment says the 1 ms cap 'guards against an aux clock being…

--

net: mdio: add timestamped write operation

This adds an optional write_sts mii_bus operation. It returns PTP system
timestamps that bound the completion of an MDIO write, and comes with the
__mdiobus_write_sts() and __phy_write_sts() helpers. The core checks the
returned timestamps. For clocks that can be stepped, it also busy-waits
until the clock's raw time reaches the upper bound. mdio-mux passes the
operation through to the parent bus.

> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 00d0e4159e9bb..8e734227b2c67 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c

[ ... ]

> @@ -145,6 +146,105 @@ int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
>  }
>  EXPORT_SYMBOL(__mdiobus_write);
>  
> +/**
> + * __mdiobus_write_sts - Timestamped version of the __mdiobus_write function

[ ... ]

> + * Return: Zero if successful, negative error code on failure. Returns
> + *	   -EBUSY or -EINVAL if the system timestamps are not valid. If @sts
> + *	   is not NULL, -EOPNOTSUPP is returned only if
> + *	   mdiobus_supports_write_sts() is false.

[ ... ]

> +	if (bus->write_sts)
> +		err = bus->write_sts(bus, addr, regnum, val, sts);
> +	else
> +		err = -EOPNOTSUPP;
> +
> +	trace_mdio_access(bus, 0, addr, regnum, val, err);
> +	mdiobus_stats_acct(&bus->stats[addr], false, err);
> +
> +	if (err)
> +		return err;
> +
> +	if (!sts->pre_sts.valid || !sts->post_sts.valid)
> +		return -EINVAL;
> +
> +	if (sts->clockid == CLOCK_MONOTONIC ||
> +	    sts->clockid == CLOCK_MONOTONIC_RAW)
> +		return 0;
> +
> +	/* Fail if the clock was stepped; callers must retry anyway. */
> +	if (sts->pre_sts.clock_was_set_seq != sts->post_sts.clock_was_set_seq)
> +		return -EBUSY;

[Severity: Low]
These -EINVAL and -EBUSY returns in __mdiobus_write_sts() happen after
bus->write_sts() has already returned 0, so the PHY register has already
changed. The same is true for the !now.valid return, the sequence mismatch
return and the 1 ms deadline return in the loop below.

The same codes can also mean the write never happened. Bus drivers are only
told not to return -EOPNOTSUPP. Also, mdio_mux_write_sts() passes a
switch_fn error straight through without doing the write:

    r = pb->switch_fn(pb->current_child, cb->bus_number, pb->switch_data);
    ...
    if (r)
        goto out;

For example, the meson g12a mux switch callback returns -EINVAL in its
default case.

Should the kernel-doc say that -EBUSY or -EINVAL from timestamp validation
means the write was performed? The doc says "Returns -EBUSY or -EINVAL if
the system timestamps are not valid", and the comment here says "callers
must retry anyway". Together, they suggest a retry is always safe.

A caller of __phy_write_sts() cannot tell "register not written" apart from
"register written, timestamps unusable". Could a retry of a non-idempotent
write apply it twice? Examples are a time load, a time adjust or a
self-clearing trigger.

The bcm_ptp_framesync_ts() user in bcm-phy-ptp.c handles this correctly. It
always polls for the capture and restores NSE_CTRL, whatever the error. So
nothing misbehaves today. The gap is in the API contract for future users.

> +
> +	/* The shifted upper bound can be later than actual completion. */
> +	deadline = ktime_add_ns(ktime_get_raw(), NSEC_PER_MSEC);
> +	for (;;) {
> +		ktime_get_snapshot_id(sts->clockid, &now);
> +		if (!now.valid)
> +			return -EINVAL;
> +
> +		if (now.clock_was_set_seq != sts->pre_sts.clock_was_set_seq)
> +			return -EBUSY;
> +
> +		if (!ktime_before(now.monoraw, sts->post_sts.monoraw))
> +			return 0;
> +
> +		/* Cap the wait at 1 ms, which is more than any single
> +		 * write's delay. This guards against an aux clock being
> +		 * disabled and reenabled, which restarts its raw time.
> +		 */
> +		if (!ktime_before(ktime_get_raw(), deadline))
> +			return -EBUSY;

[Severity: Low]
Does the 1 ms cap fully guard against an aux clock being disabled and then
re-enabled?

aux_clock_enable() clears the shadow timekeeper:

kernel/time/timekeeping.c:aux_clock_enable() {
    ...
	/* Remove leftovers of a previous registration */
	memset(aux_tks, 0, sizeof(*aux_tks));
    ...
	timekeeping_update_from_shadow(aux_tkd, TK_UPDATE_ALL);
}

This resets clock_was_set_seq, the raw time and offs_aux to 0. Then
TK_UPDATE_ALL increments clock_was_set_seq to 1.

An aux clock that was enabled and never set has seq 1. After a disable and
re-enable it has seq 1 again. The clock_was_set_seq comparisons in
__mdiobus_write_sts() therefore cannot see the reset. The only guard left
is the now.monoraw >= post_sts.monoraw check.

Two cases look like they get through.

In the first case, the aux clock was enabled recently, so post_sts.monoraw
is small. The re-enable happens while the provider sleeps after taking its
snapshots, for example in a wait_event_timeout() based wait such as
bcmgenet_mii_wait() with HZ / 100. The restarted raw time can then pass
post_sts.monoraw before this loop runs. The deadline is only computed after
write_sts() returns, so it does not help here.

In the second case, the disable and re-enable happen on another CPU between
the pre and post snapshots. post_sts then has a small monoraw from the new
epoch, and the loop returns 0 at once. Nothing checks that pre_sts comes
before post_sts.

In both cases gettimex64 would report bounds that cross an aux clock reset
as valid, instead of returning -EBUSY. This needs a privileged user to
toggle the aux clock during the write, so the window is narrow.

Should the comment be reworded to describe what the cap actually covers? Or
should the reset be detected some other way?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009143506.2507607-1-jjc%40jclark.com

Re: [PATCH net-next 2/5] net: phy: broadcom: use timestamped MDIO writes in gettimex64

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In bcm_ptp_framesync_ts() the new branch uses the bounds from
  bcm_ptp_framesync_sts() -> bcm_phy_write_exp_sts() ->…
- [Low] At this commit, no MDIO bus controller implements bus->write_sts.

--

net: phy: broadcom: use timestamped MDIO writes in gettimex64

This patch adds bcm_phy_write_exp_sts() to bcm-phy-lib, which uses
__phy_write_sts() for the EXP_DATA write. When the MDIO bus supports
timestamped writes, bcm_ptp_framesync_ts() now uses it, so the gettimex64
system timestamps only bracket completion of the write that triggers the
framesync. Otherwise the existing prets/postts path is still used.

> diff --git a/drivers/net/phy/bcm-phy-ptp.c b/drivers/net/phy/bcm-phy-ptp.c
> index 65d609ed69fb4..2d8b377010bdb 100644
> --- a/drivers/net/phy/bcm-phy-ptp.c
> +++ b/drivers/net/phy/bcm-phy-ptp.c
> @@ -214,22 +214,34 @@ static void bcm_ptp_framesync(struct phy_device *phydev, u16 ctrl)
>  	bcm_phy_write_exp(phydev, NSE_CTRL, ctrl | NSE_CPU_FRAMESYNC);
>  }
>  
> +static int bcm_ptp_framesync_sts(struct phy_device *phydev, u16 ctrl,
> +				 struct ptp_system_timestamp *sts)
> +{
> +	return bcm_phy_write_exp_sts(phydev, NSE_CTRL,
> +				     ctrl | NSE_CPU_FRAMESYNC, sts);
> +}
> +
>  static int bcm_ptp_framesync_ts(struct phy_device *phydev,
>  				struct ptp_system_timestamp *sts,
>  				struct timespec64 *ts,
>  				u16 orig_ctrl)
>  {
>  	u16 ctrl, reg;
> -	int i;
> +	int i, err = 0;
>  
>  	ctrl = bcm_ptp_framesync_disable(phydev, orig_ctrl);
>  
> -	ptp_read_system_prets(sts);
> -
>  	/* trigger framesync + capture */
> -	bcm_ptp_framesync(phydev, ctrl | NSE_CAPTURE_EN);
> -
> -	ptp_read_system_postts(sts);
> +	if (sts && bcm_phy_supports_write_exp_sts(phydev)) {

[Severity: Low]
Can this branch be reached at this point in the series?

bcm_phy_supports_write_exp_sts()->phy_supports_write_sts()->
mdiobus_supports_write_sts() just returns bus->write_sts. In this tree,
the only place that sets write_sts is mdio_mux_init(), and it only
forwards the parent bus's callback:

drivers/net/mdio/mdio-mux.c:mdio_mux_init() {
    ...
		if (parent_bus->write_sts)
			cb->mii_bus->write_sts = mdio_mux_write_sts;
    ...
}

So bcm_ptp_framesync_sts() is never called yet, and gettimex64 behaves
the same as it did before this patch.

Later patches in the series add real implementers ("net: macb: implement
timestamped MDIO writes" and "net: mdio: bcm-unimac: implement timestamped
MDIO writes"), so this resolves once the whole series is applied.

Could the commit message say that the tighter bounds depend on those bus
drivers?

> +		/* cannot ignore error since sts may be uninitialized,
> +		 * but still poll for any triggered capture
> +		 */
> +		err = bcm_ptp_framesync_sts(phydev, ctrl | NSE_CAPTURE_EN, sts);

[Severity: Medium]
Can the PHC capture land after post_sts here?

This path is bcm_ptp_framesync_sts()->bcm_phy_write_exp_sts()->
__phy_write_sts(), and the bounds it produces are used unchanged as the
bracket around the PHC capture. The kernel-doc for __mdiobus_write_sts()
defines those bounds as bracketing completion, where:

    a transfer is considered complete on the rising edge of the MDC
    that clocks the last data bit.

The commit message says "The PHY captures its time a short delay after
the end of the write". However, nothing here adds that delay to post_sts
or shifts both bounds by a known offset.

struct ptp_system_timestamp documents post_sts as:

    @post_sts:	system time snapshot after capturing PHC

The bus implementations later in the series make this window about one
MDC period wide. unimac_mdio_sts_delays() uses 63.5 and 64.5 MDC periods
for the pre and post bounds, and macb shifts both bounds by the same
delay_ns.

If the MDC edge falls within the PHY capture delay of the upper end of
that window, the actual heartbeat capture happens after post_sts. Would
that give phc2sys or chrony a biased offset with an error bound that is
too small?

Until Broadcom confirms the delay, should post_sts be extended by a
conservative upper bound, such as the measured few tenths of a
microsecond?

> +	} else {
> +		ptp_read_system_prets(sts);
> +		bcm_ptp_framesync(phydev, ctrl | NSE_CAPTURE_EN);
> +		ptp_read_system_postts(sts);
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009143506.2507607-1-jjc%40jclark.com

Re: [PATCH net-next 3/5] ptp: add functions to adjust system timestamps

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] ptp: direct addition of unscaled raw ns to slewed systime

--

> diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h
> @@ -520,4 +520,52 @@ static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
>  		ktime_get_snapshot_id(sts->clockid, &sts->post_sts);
>  }
>  
> +static inline void __ptp_adjust_snapshot(struct system_time_snapshot *snap,
> +					 s64 ns)
> +{
> +	if (!snap->valid)
> +		return;
> +	snap->systime = ktime_add_ns(snap->systime, ns);
> +	snap->monoraw = ktime_add_ns(snap->monoraw, ns);
> +	/* No counter value corresponds to the adjusted times. */
> +	snap->cycles = 0;
> +	snap->cs_id = CSID_GENERIC;
> +	snap->hw_cycles = 0;
> +	snap->hw_csid = CSID_GENERIC;
> +}
> +
> +/**
> + * ptp_adjust_system_prets - Shift the lower system timestamp bound
> + * @sts: system timestamps, or NULL
> + * @ns: CLOCK_MONOTONIC_RAW nanoseconds to add
> + *
> + * Add @ns to the selected clock's time and to the CLOCK_MONOTONIC_RAW
> + * time of the lower bound, and clear its clocksource counter values.
> + * @ns is not scaled for the selected clock's frequency correction, so
> + * the bound is off by @ns times that correction.
> + */
> +static inline void ptp_adjust_system_prets(struct ptp_system_timestamp *sts,
> +					   s64 ns)
> +{
> +	if (sts)
> +		__ptp_adjust_snapshot(&sts->pre_sts, ns);
> +}
> +
> +/**
> + * ptp_adjust_system_postts - Shift the upper system timestamp bound
> + * @sts: system timestamps, or NULL
> + * @ns: CLOCK_MONOTONIC_RAW nanoseconds to add
> + *
> + * Add @ns to the selected clock's time and to the CLOCK_MONOTONIC_RAW
> + * time of the upper bound, and clear its clocksource counter values.
> + * @ns is not scaled for the selected clock's frequency correction, so
> + * the bound is off by @ns times that correction.
> + */
> +static inline void ptp_adjust_system_postts(struct ptp_system_timestamp *sts,
> +					    s64 ns)
> +{
> +	if (sts)
> +		__ptp_adjust_snapshot(&sts->post_sts, ns);
> +}
> +
>  #endif

[Severity: Medium]
__ptp_adjust_snapshot() adds ns straight onto snap->systime, and the
kerneldoc defines ns as a CLOCK_MONOTONIC_RAW duration. When
sts->clockid is CLOCK_MONOTONIC_RAW, systime is a copy of monoraw and
this is exact. For CLOCK_REALTIME, CLOCK_MONOTONIC, CLOCK_BOOTTIME or
an aux clock it is not: systime is in the selected clock's timescale,
which runs at the NTP/adjtimex corrected rate rather than the raw
rate. After the shift, systime and monoraw no longer describe the same
instant, and systime is off by ns times the current frequency
correction.

You already note this in the kerneldoc and below the cut line. For the
30 us MDIO write in this series the error stays at a few ns, even
during a large slew. My concern is the API scope.
ptp_adjust_system_prets() and ptp_adjust_system_postts() go into a
public header and accept any s64 ns. A later caller compensating for a
longer delay, such as a slow bus or a firmware round trip, would
silently move the bound by the wrong amount. The error is largest
exactly while the servo is slewing.

That matters most when pre_sts is moved forward. If it moves too far,
it can end up after the event it is supposed to precede. Userspace then
gets a pre/post window from PTP_SYS_OFFSET_EXTENDED that does not
actually bound the PHC read.

Could the ktime_scale_raw_ns() style helper you sketched be added to
timekeeping as part of this series, so the shift is done in the right
timescale? If that is out of scope, could the helpers document, or
enforce, a small upper bound on ns? Alternatively, should this stay
local to the MDIO driver until timekeeping can do the conversion?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009143506.2507607-1-jjc%40jclark.com

Re: [PATCH net-next 4/5] net: macb: implement timestamped MDIO writes

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] macb_mdio_sts_delay() applies one fixed delay to every MACB,
  GEM and EMAC variant: 63.5 MDC periods from the MAN command write to…
- [Medium] bp->mdio_sts_rate is refreshed only in macb_mii_init() (via
  macb_mdio_init_sts()) and in macb_init_hw() (on open and resume).
- [Low] The delay in macb_mdio_sts_delay() comes from two values that
  nothing keeps consistent with each other: bp->mdio_sts_rate…

--

net: macb: implement timestamped MDIO writes

This adds a write_sts callback to the macb MDIO bus that takes system
timestamps around the MAN command register write. Both bounds are then
shifted by 63.5 MDC periods, using a cached pclk rate and the CLK divider
read from NCFGR, to estimate the MDC edge that clocks the last data bit.
The 63.5 period figure was measured on RP1 with a BCM54210PE PHY.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index c4223ca848df8..cec6e5a48e9f0 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -384,11 +384,47 @@ static int macb_mdio_read_c45(struct mii_bus *bus, int mii_id, int devad,
>  	return status;
>  }
>  
> -static int macb_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
> -			       u16 value)
> +static u64 macb_mdio_sts_delay(struct macb *bp)
> +{

[ ... ]

> +	unsigned long rate = READ_ONCE(bp->mdio_sts_rate);
> +	u32 config = macb_readl(bp, NCFGR);
> +	u32 divisor;
> +
> +	if (macb_is_gem(bp))
> +		divisor = gem_divisors[GEM_BFEXT(CLK, config)];
> +	else
> +		divisor = macb_divisors[MACB_BFEXT(CLK, config)];
> +
> +	/* On RP1 the MDC edge that clocks the last bit of a clause 22
> +	 * write comes 63.5 periods after the command write.
> +	 */
> +	return div64_ul(127ULL * divisor * NSEC_PER_SEC, 2 * rate);

[Severity: Medium]
The 63.5 period figure was measured only on RP1. Even so, macb_mii_init()
installs write_sts for every device that macb_probe() brings up. The only
gate is a non-zero pclk rate from macb_mdio_init_sts():

	if (macb_mdio_init_sts(bp))
		bp->mii_bus->write_sts = &macb_mdio_write_c22_sts;

That covers Zynq-7000, ZynqMP, Versal, SAMA5/SAMA7, SiFive, EyeQ5, PIC64
and plain MACB. It also covers the AT91RM9200 EMAC (emac_config with
at91ether_init) and macb_pci. In macb_pci, pclk is a fixed nominal 50 MHz
clock from clk_register_fixed_rate().

The mii_bus write_sts documentation says to set the callback "only if
timestamps can always be provided". __mdiobus_write_sts() also requires
the bounds to contain the MDC rising edge that clocks the last data bit.

Both bounds are shifted by the same predicted delay. Some integrations
could start the MDC frame at a different phase, for example with a
free-running divider or extra synchronizer cycles between MAN and MDC.
Would those report an interval that misses the real edge by up to one MDC
period (about 400 ns at 2.5 MHz)?

The bcm-unimac patch in this series found that hardware like this exists:

drivers/net/mdio/mdio-bcm-unimac.c:unimac_mdio_sts_delays() {
    /* On BCM2711 the MDC divider runs freely, so the MDC edge that
     * clocks the last bit of a write comes 63.5 to 64.5 periods after
     * the command start.
     */
}

Once this is applied, Broadcom bcm-phy-lib PHYs that check
phy_supports_write_sts() stop using their fallback path and use these
bounds on every macb variant. mdio-mux children inherit the callback too.

Should write_sts be limited to RP1, for example with a MACB_CAPS_* flag
set in raspberrypi_rp1_config?

Also, the commit message does not say that the delay was validated only
on RP1. That caveat is below the --- line and will be dropped when the
patch is applied. Could it go into the commit message?

[ ... ]

> @@ -1141,6 +1203,13 @@ static int macb_mdiobus_register(struct macb *bp, struct device_node *mdio_np)
>  	return mdiobus_register(bp->mii_bus);
>  }
>  
> +static bool macb_mdio_init_sts(struct macb *bp)
> +{
> +	bp->mdio_sts_rate = clk_get_rate(bp->pclk);

[Severity: Medium]
bp->mdio_sts_rate is refreshed only here and in macb_init_hw(), on open
and resume. There is no clk notifier and no rate exclusivity. What
happens to the bounds when pclk changes after that?

On Zynq-7000 the GEM pclk is <&clkc 30> (gem0_aper). It is gated from
cpu_1x, which is divided from cpu_div. cpu_6or4x has CLK_SET_RATE_PARENT
on that same cpu_div. cpufreq-dt-platdev allowlists "xlnx,zynq-7000", and
zynq-7000.dtsi has 666667 and 333334 kHz operating points.

When cpufreq drops from 666 MHz to 333 MHz, pclk halves but NCFGR keeps
the old divider, so the real time to the last MDC edge doubles. Take
GEM_CLK_DIV48 with pclk going from about 111 MHz to about 55 MHz.
macb_mdio_sts_delay() would add about 27.4 us instead of about 54.9 us.
post_sts would then land tens of microseconds before the real completion.
If the rate goes up instead, pre_sts lands after it.

The error lasts until the next macb_init_hw(). It also applies while the
interface is down, because the probe-time rate stored here is used then.

> +
> +	return bp->mdio_sts_rate != 0;
> +}
> +

[ ... ]

> @@ -3095,12 +3166,20 @@ static void macb_configure_dma(struct macb *bp)
>  
>  static void macb_init_hw(struct macb *bp)
>  {
> +	unsigned long rate;
>  	u32 config;
>  
>  	macb_reset_hw(bp);
>  	macb_set_hwaddr(bp);
>  
>  	config = macb_mdc_clk_div(bp);
> +	/* Record the pclk rate the MDC divider is chosen from, for
> +	 * write_sts, which can't call clk_get_rate() under the MDIO bus
> +	 * lock.
> +	 */
> +	rate = clk_get_rate(bp->pclk);
> +	if (rate)
> +		WRITE_ONCE(bp->mdio_sts_rate, rate);

[Severity: Low]
The new rate is published here, but the matching CLK divider reaches
NCFGR only later in macb_init_hw():

	macb_writel(bp, NCFGR, config);

Nothing here takes bus->mdio_lock. macb_mdio_write_c22_sts() holds only
mdio_lock. It can be reached from a PTP_SYS_OFFSET_EXTENDED ioctl on the
PHY PHC while macb_open() -> macb_init_hw() is running, because the PHC
is registered at probe.

Suppose a timestamped write lands between this WRITE_ONCE() and the
NCFGR write. Could macb_mdio_sts_delay() pair the new rate with the old
divisor? Both bounds would then be shifted by a delay that matches
neither configuration.

There are two related gaps. This rate comes from a separate
clk_get_rate() call, not the one macb_mdc_clk_div() / gem_mdc_clk_div()
used to choose the divider. And macb_mdio_write_c22_sts() samples
delay_ns before local_irq_save(), so an NCFGR rewrite between the sample
and the MAN write is not covered either.

A mismatch needs pclk to have changed since NCFGR was last programmed.
That can happen on Zynq-7000 with cpufreq-dt.

>  	/* Make eth data aligned.
>  	 * If RSC capable, that offset is ignored by HW.
>  	 */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009143506.2507607-1-jjc%40jclark.com

Re: [PATCH net-next 5/5] net: mdio: bcm-unimac: implement timestamped MDIO writes

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Two things in the timestamped-write path are only known to
  hold on BCM2711, but the patch applies them to every UniMAC/GENET/ASP…
- [Low] The driver now uses two different reference rates for the same
  BCM2711 MDIO block.

--

net: mdio: bcm-unimac: implement timestamped MDIO writes

This adds a write_sts implementation to the UniMAC MDIO driver. It takes
system timestamps around the MDIO command start and shifts them by 63.5
to 64.5 MDC periods, computed from the reference rate and the MDC
divider. When no clock is present on BCM2711 GENET, it uses a hardcoded
200 MHz reference rate.

> diff --git a/drivers/net/mdio/mdio-bcm-unimac.c b/drivers/net/mdio/mdio-bcm-unimac.c
> index 31e396cc9fbf2..e1ae806d1a84e 100644
> --- a/drivers/net/mdio/mdio-bcm-unimac.c
> +++ b/drivers/net/mdio/mdio-bcm-unimac.c

[ ... ]

> @@ -127,10 +150,41 @@ static int unimac_mdio_read(struct mii_bus *bus, int phy_id, int reg)
>  	return ret;
>  }
>  
> -static int unimac_mdio_write(struct mii_bus *bus, int phy_id,
> -			     int reg, u16 val)
> +static int unimac_mdio_sts_delays(struct unimac_mdio_priv *priv,
> +				  u64 *pre_ns, u64 *post_ns)
> +{

[ ... ]

> +	config = unimac_mdio_readl(priv, MDIO_CFG);
> +	if (config & MDIO_SUPP_PREAMBLE)
> +		return -EIO;
> +	divisor = 2 * (((config >> MDIO_CLK_DIV_SHIFT) & MDIO_CLK_DIV_MASK) + 1);
> +	/* On BCM2711 the MDC divider runs freely, so the MDC edge that
> +	 * clocks the last bit of a write comes 63.5 to 64.5 periods after
> +	 * the command start.
> +	 */
> +	*pre_ns = div64_ul(127ULL * divisor * NSEC_PER_SEC,
> +			   2 * priv->mdio_ref_rate);
> +	*post_ns = div64_ul(129ULL * divisor * NSEC_PER_SEC +
> +			    2 * priv->mdio_ref_rate - 1,
> +			    2 * priv->mdio_ref_rate);

[Severity: Medium]
The comment says the 63.5 to 64.5 period model comes from BCM2711. Does
it also hold on other UniMAC, GENET and ASP MDIO controllers?

unimac_mdio_init_sts() turns on write_sts for any controller where
clk_get_rate() is non-zero and MDIO_SUPP_PREAMBLE is clear. The
bcm2711-genet-v5 compatible check only supplies a missing rate. It
doesn't limit the model to BCM2711:

	priv->mdio_ref_rate = clk_get_rate(priv->clk);
	...
	if (!priv->mdio_ref_rate)
		return false;
	...
	return !(config & MDIO_SUPP_PREAMBLE);

Two setups that the bindings allow reach this path:

- A standalone UniMAC or ASP MDIO node with a clocks property.
  unimac_mdio_probe() picks it up with devm_clk_get_optional(), and
  brcm,unimac-mdio.yaml allows clocks.

- A GENET node with the "enet" main clock, which bcmgenet_mii_register()
  passes as ppd.clk.

In the GENET case, is the main clock really the MDC divider's reference?
A wrong rate moves both bounds by about 64 times the period error. For
example, 250 MHz instead of 200 MHz at divider 9 shifts them by about
1.6 us, while the window is only about one MDC period wide.

On those parts the start-to-edge latency or the reference rate could
differ. The [pre, post] interval would then miss the real edge without
any warning.

Would that break the mii_bus.write_sts contract in include/linux/phy.h?
It says "Set only if timestamps can always be provided". It would also
make PTP_SYS_OFFSET_EXTENDED report wrong PHC offsets with no error.

No in-tree DT gives GENET or UniMAC a clock today, so in-tree only
BCM2711 enables this. Could write_sts be limited to BCM2711 until other
variants have been measured?

Also, the note below the --- line says the model is assumed for every
UniMAC, but that note is dropped when the patch is applied.

> +
> +	return 0;
> +}

[ ... ]

> @@ -234,6 +307,30 @@ static int unimac_mdio_clk_set(struct unimac_mdio_priv *priv)
>  	return ret;
>  }
>  
> +static bool unimac_mdio_init_sts(struct unimac_mdio_priv *priv,
> +				 struct device *dev)
> +{
> +	u32 config;
> +
> +	/* The reference rate is fixed, so read it once. */
> +	priv->mdio_ref_rate = clk_get_rate(priv->clk);
> +	/* BCM2711's 200 MHz GENET reference clock is not described in DT. */
> +	if (!priv->mdio_ref_rate && dev->parent &&
> +	    of_device_is_compatible(dev->parent->of_node,
> +				    "brcm,bcm2711-genet-v5"))
> +		priv->mdio_ref_rate = 200000000;

[Severity: Low]
This sets a 200 MHz reference rate for BCM2711. But
unimac_mdio_clk_set() runs straight after this in unimac_mdio_probe(),
and again from unimac_mdio_resume(). It still assumes 250 MHz for the
same hardware:

	rate = clk_get_rate(priv->clk);
	if (!rate)
		rate = 250000000;

	div = (rate / (2 * priv->clk_freq)) - 1;

bcm2711-rpi-400.dts sets clock-frequency = <1950000> on &genet_mdio. So
clk_set programs div = 250000000 / 3900000 - 1 = 63.

If 200 MHz is right, MDC then runs at 200 MHz / 128, about 1.56 MHz,
instead of the 1.95 MHz that was requested. The correct divider would be
50.

The timestamps stay consistent, because unimac_mdio_sts_delays() reads
the divider back from MDIO_CFG. The MDC mis-programming also existed
before this patch.

Still, the file now has two different reference rates for the same
block. Should unimac_mdio_clk_set() use priv->mdio_ref_rate when it is
set?

> +
> +	if (!priv->mdio_ref_rate)
> +		return false;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009143506.2507607-1-jjc%40jclark.com

Re: [PATCH net-next 5/5] net: mdio: bcm-unimac: implement timestamped MDIO writes

[Nicolai Buchwitz]

Hi Florian, hi James

On 9.10.2026 18:04, Florian Fainelli wrote:

> [...]

>> When there is no clock, unimac_mdio_clk_set() assumes a 250 MHz
>> reference rate, and no in-tree DT gives GENET or UniMAC a clock. For
>> BCM2711 I have no documentation of the reference rate or of the clock
>> that supplies it, but MDIO busy times measured at several MDC dividers
>> fit a rate of 200 MHz. This patch checks the parent's compatible 
>> string,
>> which is unsatisfactory. I would prefer to get the rate from DT, and
>> would welcome suggestions for the right DT description.
> 
> You can, and should define a chip-specific compatible string for the 
> MDIO contorller node. We have one for 2711 already for GENET 
> (brcm,bcm2711-genet-v5), so you could define brcm,bcm2711-genet-mdio-v5
> 
> The clock frequency is fixed, so you could also provide a fixed clock 
> to ensure that the clock frequency is derived correctly. I will check 
> the actual clocking because 200MHz sounds odd to me, since the RGMII 
> interface does require 125MHz and therefore a 250MHz source makes that 
> easy.

I dug a bit deeper on a CM4: AFAIU the MDC runs off the GISB clock,
which the firmware sets to 200 MHz (CPRMAN 0x1d8, PLLD_PER 750 MHz /
3.75). Bumping that divider to 4.0 at runtime moves the MDIO busy time
by exactly the same ratio, so that's the one. The 250 MHz you have in
mind is genet250 / genet125, the RGMII side, a different generator.

So instead of a fixed clock or a compatible match this could just be
BCM2711_CLOCK_GISB in clk-bcm2835 (critical, firmware owns it) and
clocks = <&clocks BCM2711_CLOCK_GISB> on the mdio node. I can send that
if it helps.

Regards,
Nicolai

Re: [PATCH net-next 5/5] net: mdio: bcm-unimac: implement timestamped MDIO writes

[James Clark]

On Sat, Oct 10, 2026 at 10:18 PM Nicolai Buchwitz <nb@tipi-net.de> wrote:
> I dug a bit deeper on a CM4: AFAIU the MDC runs off the GISB clock,
> which the firmware sets to 200 MHz (CPRMAN 0x1d8, PLLD_PER 750 MHz /
> 3.75). Bumping that divider to 4.0 at runtime moves the MDIO busy time
> by exactly the same ratio, so that's the one. The 250 MHz you have in
> mind is genet250 / genet125, the RGMII side, a different generator.
>
> So instead of a fixed clock or a compatible match this could just be
> BCM2711_CLOCK_GISB in clk-bcm2835 (critical, firmware owns it) and
> clocks = <&clocks BCM2711_CLOCK_GISB> on the mdio node. I can send that
> if it helps.

Thank you very much for looking into this. I think this would solve
the problem from my end.

Let me just confirm how I would need to adapt my patch with that fix:
in the pdata branch of the probe, I would call devm_clk_get_optional
to find the clock on the mdio node, falling back to pdata->clk. I
would still add the chip-specific compatible string because the number
of MDC periods has only been measured on that specific hardware.

James

Re: [PATCH net-next 4/5] net: macb: implement timestamped MDIO writes

[Théo Lebrun]

On Sun Oct 11, 2026 at 11:23 AM CEST, Théo Lebrun wrote:
>> -static int macb_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
>> -			       u16 value)
>> +static u64 macb_mdio_sts_delay(struct macb *bp)
>> +{
>> +	static const u16 gem_divisors[] = {
>> +		[GEM_CLK_DIV8] = 8,
>> +		[GEM_CLK_DIV16] = 16,
>> +		[GEM_CLK_DIV32] = 32,
>> +		[GEM_CLK_DIV48] = 48,
>> +		[GEM_CLK_DIV64] = 64,
>> +		[GEM_CLK_DIV96] = 96,
>> +		[GEM_CLK_DIV128] = 128,
>> +		[GEM_CLK_DIV224] = 224,
>> +	};
>> +	static const u16 macb_divisors[] = {
>> +		[MACB_CLK_DIV8] = 8,
>> +		[MACB_CLK_DIV16] = 16,
>> +		[MACB_CLK_DIV32] = 32,
>> +		[MACB_CLK_DIV64] = 64,
>> +	};
>
> Do I sound crazy if I say we could use a single array for both?
> Both GEM_CLK_DIV* and MACB_CLK_DIV* are enums and are ordered in the
> same way. First four GEM_* are the same ones as MACB_* and it will
> never change.

Ignore this comment because gem_divisors[3] != macb_divisors[3].

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com