[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

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 1/5] net: mdio: add timestamped write operation

[Andrew Lunn]

> +/**
> + * __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.
> + *

Please don't do that. We don't want drivers calling this when they
don't need to.

> +		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;

A bit banging MDIO bus might be slower.

Also, i've not thought about the mux case, where the write gets
blocked until some other operation is performed on the shared hardware
of the mux.

> +	/**
> +	 * @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);

I suggest putting C22 in the comment. At some point, somebody might
need C45. And that gets complex because of C45 over C22.

Is there any need for read_sts?

	Andrew

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

[James Clark]

On Sun, Oct 11, 2026 at 9:03 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > + * If @sts is NULL, perform an ordinary write.
> > + *
>
> Please don't do that. We don't want drivers calling this when they
> don't need to.

Agreed.

> > +             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;
>
> A bit banging MDIO bus might be slower.

The comment is misleading; I'll fix it. The 1 ms doesn't limit how
long the write takes, but how long the core waits after write_sts()
returns. It allows for a driver that returns before the transfer is
complete, at worst as soon as it has started. A bit-banging driver
returns only after the last bit, so it doesn't wait at all.

> Also, i've not thought about the mux case, where the write gets
> blocked until some other operation is performed on the shared hardware
> of the mux.

In the mux case, mdio_mux_write_sts() takes the parent bus lock and
switches the mux before calling the parent's write_sts(). So any time
spent blocked on the mux comes before the lower bound. Or did you have
another case in mind?

> > +     /**
> > +      * @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);
>
> I suggest putting C22 in the comment. At some point, somebody might
> need C45. And that gets complex because of C45 over C22.

Agreed.

> Is there any need for read_sts?

Not for this series, I think. The BCM54210PE captures its time on a
write, so a timestamped read isn't needed here. I'd leave read_sts
until a driver needs one.

James

[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

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

[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

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

[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

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 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

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

[James Clark]

Hi Theo,

Thanks for looking at this! I am very new to this, so I really
appreciate any guidance.

(For some reason your message didn't make it into Lore, although your
followup did.)

On Sun, Oct 11, 2026 at 4:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> I'm invoking my colleague @Maxime to get his opinion. What impacts the
> MDIO write_c22 duration and how likely is it that this duration varies
> (1) on the same MAC+PHY combo, (2) with a different PHY or (3) with a
> different MACB revision?

(3) is the one case where I think it could possibly vary.  But 63.5
periods makes sense assuming the first bit starts at the moment the
command is written and the PHY acts on it when it samples the last of
64 bits at the rising edge half-way through the last period. In 5/5
there's a different situation: there UniMAC's MDC is free-running, so
the frame can start up to a period after the command write, leading to
a possible range of 63.5-64.5. So I don't think 63.5 is as arbitrary
as it might at first seem.

> Sashiko makes an interesting point with PCLK that might change over
> time, giving an example (which I've not checked) where it would be
> derived from the CPU clk which might change with cpufreq.

I tried a bunch of different approaches before landing on what I sent
in v1, and ran Sashiko locally to review. And every approach had its
problems. Sashiko is right here, but on the other hand the existing
macb_mdc_clk_div() (it's only called at probe, open and resume) has
the same problem.

My plan for v2 was to limit this to RP1, as Sashiko suggested, with a
MACB_CAPS_MDIO_WRITE_STS flag
set in raspberrypi_rp1_config. This would solve the PCLK point since
the PCLK is fixed in the RP1.

> If you share more details on your testing setup it would help for us to
> reproduce on other hardware to gather more data points.

I created a gist with the code I used (this is all agent-written)

https://gist.github.com/jclark/372822d80ba27a9648d1aea939191b53

Key point is that the measurement technique depends on there being a
PHY with a PHC connected over MDIO, which is the case when write_sts
is useful. I believe the RP1 is the only such case in the tree at the
moment. If in the future there's another case, then it can be measured
and added using the same technique.

> I'm wondering if we could do better than a hardcoded delay? For example
> we could busy-wait for NSR.IDLE inside the prets/postts region, but
> that implies at least 25µs with IRQs disabled. Or we could do a few
> measurements at init to sample our platform delay?

On the RP1, IDLE is not the edge: IDLE comes at 65.0 periods. The only
way we could measure the edge is by using the PHY as an external
reference.

> > +     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)];
>
> I'm a bit surprised to see you reading NCFGR.CLK and derive the current
> divisor from that. The divisor is set by us in macb_mdc_clk_div(), with
> only input being the PCLK rate.

This was something a local Sashiko review found in an earlier version.
The divider in hardware isn't always the one macb_mdc_clk_div() chose:
after suspend with the interface down, macb_resume() doesn't reprogram
NCFGR.

Incidentally, it is also what made the measurement technique work: the
bounds follow a divider changed through /dev/mem.

> > +     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);
>
> An unconditional readl for flushing the posted write won't do it.
> MACB is over PCI only on very few platforms. I guess the straightforward
> solution is a POSTED_WRITES flag that is then read from here.

This is only under sts.  Assuming write_sts becomes RP1 only, then it
is only over PCIe. I am not sure if I have understood what you are
getting at here.

> > +             /* Order the read-back before the system timestamp. */
> > +             rmb();
>
> Why rmb() and not mb()?

The rmb() here only has to order the NSR read before the post
timestamp read, since by the same device rule that read can't complete
before the MAN write has arrived.

> > +     /* 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);
>
> Can you clarify here and in the commit message why clk_get_rate() is
> unacceptable under the MDIO bus lock?
>
> Seeing your comment, my guess is an ABBA deadlock risk between
> clk_prepare_lock and mii_bus->mdio_lock but please write it out.

Your guess is right. I tried this earlier and a local Sashiko review
found the problem:

bcm_ptp_gettimex()
  bcm_phy_write_exp_sts()
    phy_lock_mdio_bus()
    __mdiobus_write_sts()
      macb_mdio_write_c22_sts()
        macb_mdio_sts_delay()
          clk_get_rate()
            clk_prepare_lock()

clk_summary_show()
  clk_prepare_lock()
  clk_summary_show_one()
    clk_core_is_enabled()
      en8811h_clk_is_enabled()
        air_phy_buckpbus_reg_read()
          phy_select_page()
            phy_save_page()
              phy_lock_mdio_bus()

I will clarify this in the comment and commit message.

James

[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.
A