[PATCH net-next v13 0/2] ptp: ocp: add TAP CPLD support for ADVA TimeCard X1

[PATCH net-next v13 0/2] ptp: ocp: add TAP CPLD support for ADVA TimeCard X1

[Sagi Maimon]

The ADVA TimeCard X1 (PCI device 0x0410) carries a Lattice MachXO3 CPLD
on the TMC I2C bus.  This series adds access to it and an update path
through the kernel firmware-upload subsystem.

The card has two I2C controllers.  Linux registers only the one at
0x00150000, normally routed to the config EEPROMs; the one at 0x00120000
belongs to the on-card MicroBlaze and is routed to the TMC or M.2 bus as
its firmware selects.  A request/grant handshake re-routes the host
controller onto the TMC bus, where the PCA9548 and the CPLD behind its
channel 0 live.  Everything here is built on that handshake.

Patch 1 adds the arbitration and the read-only interfaces on top of it.
Patch 2 adds the flashing.

v12: https://lore.kernel.org/netdev/20260902044232.21188-1-maimon.sagi@gmail.com/T/#u

Changes since v12 - all from the automated review of v12:

 - The CPLD device ID is no longer read from ptp_ocp_devlink_info_get().
   DEVLINK_CMD_INFO_GET carries no GENL_ADMIN_PERM, and that read claims
   the i2c adapter and drives the on-card mux for up to two seconds,
   which any user could have repeated to keep the EEPROM, nvmem and at24
   paths off the controller.  The same operation is root-only through
   cpld_status.  The driver now reads the ID once from its own worker,
   as soon as the i2c adapter has registered, and info_get() reports the
   cached value.  Patch 1.
 - As a result info_get() can no longer fail because of the CPLD: it
   skips cpld.id when the value is not available.  Returning an error
   there made devlink discard the whole reply, and -ENODEV was reachable
   on any card where the i2c adapter never bound, which also aborted
   'devlink dev info' for every other instance on the host.  Patch 1.
 - cpld.id is now reported before the EEPROM data rather than after its
   early return, so it no longer depends on the EEPROM having been read.
   Patch 1.
 - adva_x1_bus_claim() re-checks that the adapter it just took a
   reference to is still this card's: i2c_del_adapter() frees the idr
   slot before the bus notifier clears the cached number, so the number
   could already have been recycled.  Patch 1.
 - The probe error paths unregister devlink before ptp_ocp_detach(),
   which destroys cpld_lock and releases the i2c controller.  Patch 1.
 - I2C_M_DMA_SAFE is no longer set.  No adapter that can bind here does
   DMA, and the read half of the scratch buffer is an interior pointer
   that does not satisfy the flag's alignment contract; the core is left
   free to bounce.  Patch 1.
 - The firmware-upload callbacks map errnos onto the documented
   fw_upload_err categories: -ENODEV and a failed transfer are no longer
   reported as a timeout, and a CPLD FAILED status is no longer reported
   as one either.  Patch 2.
 - adva_cpld_prepare() checks for a cancellation on entry, as its own
   comment promised, instead of cycling the CPLD into configuration mode
   for an upload that was already aborted.  Patch 2.
 - Added Documentation/networking/devlink/ptp_ocp.rst describing the
   driver-specific cpld.id version, with the index and MAINTAINERS
   entries.  Patch 1.
 - Comments in patch 1 no longer describe the firmware-upload path that
   only patch 2 adds, and the lines that exceeded 80 columns are wrapped.

Three points from that review are deliberately not addressed here,
because changing them needs a full programming cycle on the card to
validate and I would rather not fold untested changes into a respin:

 - poll_complete() samples DONE before REFRESH only, and does not check
   the sysCONFIG error field the way drivers/fpga/lattice-sysconfig.c
   does.
 - adva_x1_mblaze_release() does not wait for the firmware to route the
   controller back before the adapter lock is dropped.
 - A cancel arriving between cleanup() and fw_upload_prog_complete()
   stays set and aborts the next upload.

Tested on an ADVA TimeCard X1: the driver loads with the i2c adapter
registering after probe, cpld.id and cpld_status read back correctly,
and the wire byte sequences are unchanged from v12, which programmed the
part successfully.

Sagi Maimon (2):
  ptp: ocp: add TAP CPLD access for ADVA TimeCard X1
  ptp: ocp: add TAP CPLD firmware upload for ADVA TimeCard X1

 Documentation/ABI/testing/sysfs-timecard     |  24 +
 Documentation/networking/devlink/index.rst   |   1 +
 Documentation/networking/devlink/ptp_ocp.rst |  35 +
 MAINTAINERS                                  |   1 +
 drivers/ptp/Kconfig                          |   2 +
 drivers/ptp/ptp_ocp.c                        | 761 ++++++++++++++++++-
 6 files changed, 818 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/networking/devlink/ptp_ocp.rst


base-commit: c29b37ed7a4d9856ed758a82282456d69cee2ed1
-- 
2.47.0

[PATCH net-next v13 1/2] ptp: ocp: add TAP CPLD access for ADVA TimeCard X1

[Sagi Maimon]

The ADVA TimeCard X1 (PCI device 0x0410) carries a Lattice MachXO3 CPLD
on the TMC I2C bus.  The card has two I2C controllers: Linux registers
only the one at 0x00150000, normally routed to the config EEPROMs, while
the one at 0x00120000 belongs to the on-card MicroBlaze and is routed to
either the TMC or the M.2 bus as its firmware selects.

A request/grant handshake re-routes the host controller from the EEPROM
segment onto the TMC bus, leaving the MicroBlaze with M.2 only.  The
PCA9548 at 0x74 and the CPLD at 0x40 behind its channel 0 are reachable
from the host only while that grant is held.

Add the arbitration and the read-only interfaces built on it.  Because
the handshake changes what the host adapter is wired to, an operation
takes the i2c core adapter lock for the whole grant window and uses
__i2c_transfer() internally; without that, a concurrent transfer from
ptp_ocp_read_eeprom(), from the nvmem attributes or from the at24 sysfs
files would be issued onto the TMC bus instead of to the EEPROM.

The Lattice device ID is a fixed property of the part, so report it as
the fixed devlink version "cpld.id" rather than as a sysfs attribute,
and cache it so the arbitration is paid once per card.

Add one X1-only attribute:

  /sys/class/timecard/ocpN/cpld_status
        report the CPLD status register, including the DONE, BUSY and
        FAILED indicators

A read arbitrates for the shared bus and reprograms the on-card mux, so
it is root-only and takes cpld_lock interruptibly.

Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 Documentation/ABI/testing/sysfs-timecard     |  20 +
 Documentation/networking/devlink/index.rst   |   1 +
 Documentation/networking/devlink/ptp_ocp.rst |  35 ++
 MAINTAINERS                                  |   1 +
 drivers/ptp/ptp_ocp.c                        | 388 ++++++++++++++++++-
 5 files changed, 439 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/networking/devlink/ptp_ocp.rst

diff --git a/Documentation/ABI/testing/sysfs-timecard b/Documentation/ABI/testing/sysfs-timecard
index 3ae41b7634ac..70ecc15b347f 100644
--- a/Documentation/ABI/testing/sysfs-timecard
+++ b/Documentation/ABI/testing/sysfs-timecard
@@ -11,6 +11,26 @@ Contact:	Jonathan Lemon <jonathan.lemon@gmail.com>
 Description:	This directory contains the attributes of the Nth timecard
 		registered.
 
+What:		/sys/class/timecard/ocpN/cpld_status
+Date:		July 2026
+Contact:	Sagi Maimon <maimon.sagi@gmail.com>
+Description:	(RO, root only) The status register of the TAP CPLD, in
+		human-readable form:
+
+		  done=<0|1> busy=<0|1> failed=<0|1>
+
+		Only present on ADVA x1 TAP boards (PCI ID 0xad5a:0x0410).
+
+		done=1 indicates the configuration flash was successfully
+		programmed and is active.  busy=1 means an internal operation
+		is in progress.  failed=1 means the last ISC operation failed.
+
+		A read arbitrates for the shared I2C bus and reprograms the
+		on-card mux, so it is restricted to root.  The Lattice device
+		ID of the CPLD is read once at probe and reported as the
+		fixed "cpld.id" version by devlink dev info.
+
+
 What:		/sys/class/timecard/ocpN/available_clock_sources
 Date:		September 2021
 Contact:	Jonathan Lemon <jonathan.lemon@gmail.com>
diff --git a/Documentation/networking/devlink/index.rst b/Documentation/networking/devlink/index.rst
index d4a83fdcff7f..1af780c811ee 100644
--- a/Documentation/networking/devlink/index.rst
+++ b/Documentation/networking/devlink/index.rst
@@ -99,6 +99,7 @@ parameters, info versions, and other features it supports.
    nfp
    octeontx2
    prestera
+   ptp_ocp
    qed
    sfc
    stmmac
diff --git a/Documentation/networking/devlink/ptp_ocp.rst b/Documentation/networking/devlink/ptp_ocp.rst
new file mode 100644
index 000000000000..3f997cd9f3a3
--- /dev/null
+++ b/Documentation/networking/devlink/ptp_ocp.rst
@@ -0,0 +1,35 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+========================
+ptp_ocp devlink support
+========================
+
+This document describes the devlink features implemented by the ``ptp_ocp``
+device driver.
+
+Info versions
+=============
+
+The ``ptp_ocp`` driver reports the following versions
+
+.. list-table:: devlink info versions implemented
+   :widths: 5 5 90
+
+   * - Name
+     - Type
+     - Description
+   * - ``fw``
+     - running
+     - Version of the firmware running on the card.  Reported as ``loader``
+       instead when the card is running the firmware loader.
+   * - ``board.id``
+     - fixed
+     - Board identifier, read from the on-card EEPROM.
+   * - ``cpld.id``
+     - fixed
+     - Lattice device ID (IDCODE) of the TAP CPLD, formatted as ``0x%08x``.
+       Only present on ADVA TimeCard X1 boards, which are the only ones
+       carrying that CPLD.  Reading it claims the shared I2C bus and
+       reprograms the on-card mux, so the driver does that once from its
+       own worker and reports the cached value here; the version is
+       omitted until that read has succeeded.
diff --git a/MAINTAINERS b/MAINTAINERS
index 0b42e898f4d8..6216244edb04 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20485,6 +20485,7 @@ OPENCOMPUTE PTP CLOCK DRIVER
 M:	Vadim Fedorenko <vadim.fedorenko@linux.dev>
 L:	netdev@vger.kernel.org
 S:	Maintained
+F:	Documentation/networking/devlink/ptp_ocp.rst
 F:	drivers/ptp/ptp_ocp.c
 
 OPENCORES I2C BUS DRIVER
diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 3d26ec1f7b9e..7db1dc9b94bd 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -24,6 +24,8 @@
 #include <linux/nvmem-consumer.h>
 #include <linux/crc16.h>
 #include <linux/dpll.h>
+#include <linux/unaligned.h>
+#include <linux/delay.h>
 
 #define PCI_DEVICE_ID_META_TIMECARD		0x0400
 
@@ -85,6 +87,8 @@ struct ptp_ocp_adva_info {
 	u8				signals_nr;
 	u8				freq_in_nr;
 	const struct ocp_attr_group	*attr_groups;
+	/* x1: TAP CPLD present */
+	bool				has_cpld;
 };
 
 #define OCP_CTRL_ENABLE		BIT(0)
@@ -163,7 +167,8 @@ struct gpio_reg {
 	u32	gpio1;
 	u32	__pad0;
 	u32	gpio2;
-	u32	__pad1;
+	/* adva_x1: I2C bus ownership register; reserved on other variants */
+	u32	i2c_bus_ctrl;
 };
 
 struct irig_master_reg {
@@ -416,6 +421,21 @@ struct ptp_ocp {
 	dpll_tracker tracker;
 	int signals_nr;
 	int freq_in_nr;
+	/* adva_x1 CPLD I2C (internal use only) */
+	/* serialises CPLD operations */
+	struct mutex		cpld_lock;
+	/* I2C adapter nr; -1 if absent */
+	int			cpld_i2c_adap_nr;
+	/* claimed adapter; valid under cpld_lock */
+	struct i2c_adapter	*cpld_adap;
+	/* scratch, write half then read half; valid under cpld_lock */
+	u8			*cpld_buf;
+	/* Lattice device ID; 0 if unread */
+	u32			cpld_id;
+	/* the one-shot ID read has been attempted */
+	bool			cpld_id_tried;
+	/* x1 TAP CPLD present */
+	bool			has_cpld;
 };
 
 #define OCP_REQ_TIMESTAMP	BIT(0)
@@ -1273,6 +1293,7 @@ static struct ocp_resource ocp_adva_x1_resource[] = {
 			.signals_nr   = 4,
 			.freq_in_nr   = 4,
 			.attr_groups  = adva_timecard_x1_groups,
+			.has_cpld     = true,
 		},
 	},
 	{ }
@@ -2166,6 +2187,17 @@ ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
 	if (err)
 		return err;
 
+	/* Read by the worker, not here: this command is unprivileged and
+	 * reading the ID claims the I2C bus.  0 means unread - skip it
+	 * rather than fail, devlink discards the whole reply on error.
+	 */
+	if (bp->cpld_id) {
+		sprintf(buf, "0x%08x", bp->cpld_id);
+		err = devlink_info_version_fixed_put(req, "cpld.id", buf);
+		if (err)
+			return err;
+	}
+
 	if (!bp->has_eeprom_data) {
 		ptp_ocp_read_eeprom(bp);
 		if (!bp->has_eeprom_data)
@@ -3199,6 +3231,7 @@ ptp_ocp_adva_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
 		return err;
 	ptp_ocp_sma_init(bp);
 
+	bp->has_cpld = info->has_cpld;
 	return ptp_ocp_init_clock(bp, &info->servo);
 }
 
@@ -4226,6 +4259,319 @@ static const struct ocp_attr_group art_timecard_groups[] = {
 	{ },
 };
 
+/*
+ * Internal helpers for the adva_x1 TAP CPLD (Lattice LCMXO3LF-2100).
+ *
+ * The card has two I2C controllers; Linux registers only 0x00150000.
+ * The i2c_bus_ctrl handshake re-routes what each one is wired to:
+ *
+ *   grant 0:  0x150000 -> EEPROMs     0x120000 -> TMC or M.2, MB's choice
+ *   grant 1:  0x150000 -> TMC bus     0x120000 -> M.2
+ *
+ * The PCA9548 at 0x74 and the CPLD at 0x40 behind its channel 0 sit on
+ * the TMC bus, so they are reachable only while the grant is held.  For
+ * that same window the EEPROMs are not behind the adapter at all, so a
+ * CPLD operation holds cpld_lock and the i2c adapter lock across it to
+ * keep the EEPROM and nvmem paths off the controller.
+ *
+ * No raw I2C access is exposed to userspace, only the attributes below.
+ */
+
+#define ADVA_MUX_ADDR     0x74
+#define ADVA_CPLD_ADDR    0x40
+#define ADVA_MUX_CHANNEL  0
+
+/* Longest command or reply; bounds each half of the per-claim scratch. */
+#define ADVA_CPLD_XFER_MAX 32
+
+#define MBLAZE_REQUEST    0x0000aaaaU
+#define MBLAZE_GRANTED    0x5555aaaaU
+#define MBLAZE_RELEASE    0x55550000U
+#define MBLAZE_RETRIES    200
+#define MBLAZE_RETRY_US   10000
+
+/* Lattice LCMXO3LF ISC command codes */
+#define CPLD_CMD_READ_ID      0xE0000000UL
+#define CPLD_CMD_READ_STATUS  0x3C000000UL
+
+/* Status register bit positions (Lattice LCMXO3LF datasheet) */
+#define CPLD_STATUS_DONE   BIT(8)
+#define CPLD_STATUS_BUSY   BIT(12)
+#define CPLD_STATUS_FAILED BIT(13)
+
+/*
+ * adva_x1_i2c_xfer() - issue a single I2C transaction on the TMC bus.
+ *
+ * Writes @cmd when it is not negative, followed by @wlen bytes of @wdata,
+ * then reads @rlen bytes if asked.  A NULL @wdata sends zeros.
+ *
+ * The message is assembled in the scratch buffer taken by
+ * adva_x1_bus_claim(), so an opcode and its arguments are copied exactly
+ * once.  I2C_M_DMA_SAFE is deliberately not set: no adapter that can bind
+ * here does DMA, and the two halves are not separately aligned, so the
+ * core is left free to bounce.
+ *
+ * Caller must hold that claim, hence __i2c_transfer() over i2c_transfer().
+ */
+static int adva_x1_i2c_xfer(struct ptp_ocp *bp, u8 addr, int cmd,
+			    const void *wdata, u8 wlen,
+			    void *rdata, u8 rlen)
+{
+	u8 *wbuf, *rbuf;
+	struct i2c_adapter *adap;
+	struct i2c_msg msgs[2];
+	int nmsgs = 0, ret;
+	u16 hdr = cmd >= 0;
+
+	lockdep_assert_held(&bp->cpld_lock);
+
+	adap = bp->cpld_adap;
+	if (!adap || !bp->cpld_buf)
+		return -ENODEV;
+
+	if (hdr + wlen > ADVA_CPLD_XFER_MAX || rlen > ADVA_CPLD_XFER_MAX)
+		return -EINVAL;
+
+	wbuf = bp->cpld_buf;
+	rbuf = bp->cpld_buf + ADVA_CPLD_XFER_MAX;
+
+	if (hdr + wlen) {
+		if (hdr)
+			wbuf[0] = cmd;
+		if (wdata)
+			memcpy(wbuf + hdr, wdata, wlen);
+		else
+			memset(wbuf + hdr, 0, wlen);
+		msgs[nmsgs++] = (struct i2c_msg){
+			.addr  = addr,
+			.flags = 0,
+			.len   = hdr + wlen,
+			.buf   = wbuf,
+		};
+	}
+	if (rlen) {
+		msgs[nmsgs++] = (struct i2c_msg){
+			.addr  = addr,
+			.flags = I2C_M_RD,
+			.len   = rlen,
+			.buf   = rbuf,
+		};
+	}
+
+	ret = __i2c_transfer(adap, msgs, nmsgs);
+	if (ret != nmsgs)
+		return (ret < 0) ? ret : -EIO;
+
+	if (rdata && rlen)
+		memcpy(rdata, rbuf, rlen);
+
+	return 0;
+}
+
+static void adva_x1_mblaze_release(struct ptp_ocp *bp)
+{
+	if (bp->pps_select)
+		iowrite32(MBLAZE_RELEASE, &bp->pps_select->i2c_bus_ctrl);
+}
+
+/* Acquire the shared I2C bus from the MicroBlaze firmware.  Returns with no
+ * request outstanding on failure, so the firmware is never left granting a
+ * segment to a host that has given up waiting for it.
+ */
+static int adva_x1_mblaze_acquire(struct ptp_ocp *bp)
+{
+	u32 val;
+	int i;
+
+	if (!bp->pps_select)
+		return -ENODEV;
+
+	/* Drop a request left by a caller that died mid-sequence; cpld_lock
+	 * keeps live ones out.  The read back only flushes the posted write.
+	 */
+	iowrite32(0, &bp->pps_select->i2c_bus_ctrl);
+	ioread32(&bp->pps_select->i2c_bus_ctrl);
+
+	iowrite32(MBLAZE_REQUEST, &bp->pps_select->i2c_bus_ctrl);
+	for (i = 0; i < MBLAZE_RETRIES; i++) {
+		usleep_range(MBLAZE_RETRY_US, MBLAZE_RETRY_US + 1000);
+		val = ioread32(&bp->pps_select->i2c_bus_ctrl);
+		if (val == MBLAZE_GRANTED)
+			return 0;
+	}
+
+	adva_x1_mblaze_release(bp);
+	return -ETIMEDOUT;
+}
+
+/* Route the host controller back to the EEPROMs and release the adapter.
+ * Safe after a failed claim: it also clears a request that was never granted.
+ */
+static void adva_x1_bus_release(struct ptp_ocp *bp)
+{
+	struct i2c_adapter *adap = bp->cpld_adap;
+
+	if (!adap)
+		return;
+
+	adva_x1_mblaze_release(bp);
+	bp->cpld_adap = NULL;
+	kfree(bp->cpld_buf);
+	bp->cpld_buf = NULL;
+	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+	i2c_put_adapter(adap);
+}
+
+/*
+ * Claim the TMC bus for a CPLD operation.  Holding the adapter lock over
+ * the handshake keeps ptp_ocp_read_eeprom(), the nvmem attributes and the
+ * at24 sysfs files off the controller while it is routed away from the
+ * EEPROMs, so an EEPROM read blocks for as long as the claim is held.
+ */
+static int adva_x1_bus_claim(struct ptp_ocp *bp)
+{
+	struct i2c_adapter *adap;
+	int ret;
+
+	lockdep_assert_held(&bp->cpld_lock);
+
+	adap = i2c_get_adapter(READ_ONCE(bp->cpld_i2c_adap_nr));
+	if (!adap)
+		return -ENODEV;
+
+	/* The number is freed before the notifier clears it, so it can
+	 * already be another adapter's.  Check this is still ours.
+	 */
+	if (!bp->i2c_ctrl || adap->dev.parent != &bp->i2c_ctrl->dev) {
+		i2c_put_adapter(adap);
+		return -ENODEV;
+	}
+
+	/* One scratch buffer per claim, not per transfer. */
+	bp->cpld_buf = kzalloc(2 * ADVA_CPLD_XFER_MAX, GFP_KERNEL);
+	if (!bp->cpld_buf) {
+		i2c_put_adapter(adap);
+		return -ENOMEM;
+	}
+
+	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+	bp->cpld_adap = adap;
+
+	ret = adva_x1_mblaze_acquire(bp);
+	if (ret)
+		adva_x1_bus_release(bp);
+
+	return ret;
+}
+
+/* Select a mux channel, or deselect all with ch < 0 - the power-on state.
+ * The mux is on the TMC bus, so what it is left set to never affects the
+ * EEPROM paths.
+ */
+static int adva_x1_mux_select(struct ptp_ocp *bp, int ch)
+{
+	u8 val = (ch >= 0) ? BIT(ch) : 0;
+
+	return adva_x1_i2c_xfer(bp, ADVA_MUX_ADDR, val, NULL, 0, NULL, 0);
+}
+
+/*
+ * Send a 4-byte command then read data back without an intermediate STOP
+ * (Lattice combined write->repeated-START->read).  Two messages in one
+ * transfer is exactly that, so no protocol-mangling flag is needed.
+ */
+static int adva_x1_cpld_cmd_read(struct ptp_ocp *bp,
+				 u32 cmd_be, u8 *out, u8 out_len)
+{
+	__be32 cmd = cpu_to_be32(cmd_be);
+
+	return adva_x1_i2c_xfer(bp, ADVA_CPLD_ADDR, -1, &cmd, 4, out, out_len);
+}
+
+static int adva_x1_cpld_read_status(struct ptp_ocp *bp, u32 *status)
+{
+	u8 buf[4];
+	int ret;
+
+	ret = adva_x1_cpld_cmd_read(bp, CPLD_CMD_READ_STATUS, buf, 4);
+	if (ret)
+		return ret;
+	*status = get_unaligned_be32(buf);
+	return 0;
+}
+
+/*
+ * Read the Lattice device ID into bp->cpld_id.  Done once, off the
+ * unprivileged devlink path, which reports the cached value only.
+ * Returns -EBUSY if the bus is busy, so the caller can retry rather than
+ * wait out a firmware upload.  The LCMXO3LF-2100 IDCODE is 0xe12bc043.
+ */
+static int adva_x1_cpld_read_id(struct ptp_ocp *bp)
+{
+	u8 data[4];
+	int ret;
+
+	if (!mutex_trylock(&bp->cpld_lock))
+		return -EBUSY;
+
+	ret = adva_x1_bus_claim(bp);
+	if (ret)
+		goto out;
+	ret = adva_x1_mux_select(bp, ADVA_MUX_CHANNEL);
+	if (ret)
+		goto release;
+	ret = adva_x1_cpld_cmd_read(bp, CPLD_CMD_READ_ID, data, 4);
+	if (!ret)
+		bp->cpld_id = get_unaligned_be32(data);
+	adva_x1_mux_select(bp, -1);
+release:
+	adva_x1_bus_release(bp);
+out:
+	mutex_unlock(&bp->cpld_lock);
+	if (ret)
+		dev_dbg(&bp->pdev->dev,
+			"CPLD device ID unavailable: %d\n", ret);
+
+	return ret;
+}
+
+/*
+ * cpld_status - show the status register of the TAP CPLD.
+ *
+ * Returns a human-readable string: "done=<0|1> busy=<0|1> failed=<0|1>\n"
+ */
+static ssize_t
+cpld_status_show(struct device *dev, struct device_attribute *attr,
+		 char *buf)
+{
+	struct ptp_ocp *bp = dev_get_drvdata(dev);
+	u32 st = 0;
+	int ret;
+
+	/* A CPLD operation can hold cpld_lock a long time; stay killable. */
+	ret = mutex_lock_interruptible(&bp->cpld_lock);
+	if (ret)
+		return ret;
+
+	ret = adva_x1_bus_claim(bp);
+	if (ret)
+		goto out;
+	ret = adva_x1_mux_select(bp, ADVA_MUX_CHANNEL);
+	if (ret)
+		goto release;
+	ret = adva_x1_cpld_read_status(bp, &st);
+	adva_x1_mux_select(bp, -1);
+release:
+	adva_x1_bus_release(bp);
+out:
+	mutex_unlock(&bp->cpld_lock);
+	return ret ? ret : sysfs_emit(buf, "done=%u busy=%u failed=%u\n",
+				      !!(st & CPLD_STATUS_DONE),
+				      !!(st & CPLD_STATUS_BUSY),
+				      !!(st & CPLD_STATUS_FAILED));
+}
+static DEVICE_ATTR_ADMIN_RO(cpld_status);
+
 static struct attribute *adva_timecard_attrs[] = {
 	&dev_attr_serialnum.attr,
 	&dev_attr_gnss_sync.attr,
@@ -4274,6 +4620,7 @@ static struct attribute *adva_timecard_x1_attrs[] = {
 	&dev_attr_ts_window_adjust.attr,
 	&dev_attr_utc_tai_offset.attr,
 	&dev_attr_tod_correction.attr,
+	&dev_attr_cpld_status.attr,
 	NULL,
 };
 
@@ -4904,6 +5251,7 @@ ptp_ocp_detach(struct ptp_ocp *bp)
 		clk_hw_unregister_fixed_rate(bp->i2c_clk);
 	if (bp->n_irqs)
 		pci_free_irq_vectors(bp->pdev);
+	mutex_destroy(&bp->cpld_lock);
 	device_unregister(&bp->dev);
 }
 
@@ -5052,6 +5400,14 @@ ptp_ocp_sync_work(struct work_struct *work)
 
 	bp->sync = sync;
 
+	/* The i2c adapter is registered after probe returns, so the ID is
+	 * read from here, once, as soon as the adapter shows up.
+	 */
+	if (bp->has_cpld && !bp->cpld_id_tried &&
+	    READ_ONCE(bp->cpld_i2c_adap_nr) >= 0 &&
+	    adva_x1_cpld_read_id(bp) != -EBUSY)
+		bp->cpld_id_tried = true;
+
 	queue_delayed_work(system_power_efficient_wq, &bp->sync_work, HZ);
 }
 
@@ -5080,6 +5436,17 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (err)
 		goto out_disable;
 
+	/* Must be before the first error path that calls ptp_ocp_detach(),
+	 * so mutex_destroy() always runs on an initialised mutex.
+	 * Must also be before ptp_ocp_register_resources(): the I2C bus
+	 * notifier (ptp_ocp_i2c_notifier_call) fires when the adapter
+	 * registers and stores the adapter number in cpld_i2c_adap_nr; the
+	 * -1 sentinel below must already be written so that a notifier
+	 * firing during registration is never overwritten by this init.
+	 */
+	mutex_init(&bp->cpld_lock);
+	bp->cpld_i2c_adap_nr = -1;
+
 	INIT_DELAYED_WORK(&bp->sync_work, ptp_ocp_sync_work);
 
 	/* compat mode.
@@ -5119,12 +5486,12 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (IS_ERR(bp->dpll)) {
 		err = PTR_ERR(bp->dpll);
 		dev_err(&pdev->dev, "dpll_device_alloc failed\n");
-		goto out;
+		goto out_devlink;
 	}
 
 	err = dpll_device_register(bp->dpll, DPLL_TYPE_PPS, &dpll_ops, bp);
 	if (err)
-		goto out;
+		goto out_devlink;
 
 	for (i = 0; i < OCP_SMA_NUM; i++) {
 		bp->sma[i].dpll_pin = dpll_pin_get(clkid, i, THIS_MODULE,
@@ -5151,6 +5518,9 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		dpll_pin_put(bp->sma[i].dpll_pin, &bp->sma[i].tracker);
 	}
 	dpll_device_put(bp->dpll, &bp->tracker);
+out_devlink:
+	/* detach() destroys cpld_lock; no devlink op may still reach it */
+	devlink_unregister(devlink);
 out:
 	ptp_ocp_detach(bp);
 out_disable:
@@ -5219,11 +5589,17 @@ ptp_ocp_i2c_notifier_call(struct notifier_block *nb,
 
 found:
 	bp = dev_get_drvdata(dev);
-	if (add)
+	if (add) {
 		ptp_ocp_symlink(bp, child, "i2c");
-	else
+		/* Cache the number; adva_x1_bus_claim() takes the reference
+		 * and re-checks the parent, as the number can be recycled.
+		 */
+		WRITE_ONCE(bp->cpld_i2c_adap_nr, i2c_verify_adapter(child)->nr);
+	} else {
+		/* invalidate before free */
+		WRITE_ONCE(bp->cpld_i2c_adap_nr, -1);
 		sysfs_remove_link(&bp->dev.kobj, "i2c");
-
+	}
 	return 0;
 }
 
-- 
2.47.0

Re: [PATCH net-next v13 1/2] ptp: ocp: add TAP CPLD access for ADVA TimeCard X1

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 3 (1) · Low: 5

- [Medium] ptp_ocp_probe() silently fixes a pre-existing devlink teardown
  bug.
- [Medium] In ptp_ocp_sync_work() (drivers/ptp/ptp_ocp.c:5406-5409) the
  one-shot flag bp->cpld_id_tried is latched for every return value of…
- [Medium] The grant hand-back is fire-and-forget.
- [Low] The comment above adva_x1_cpld_read_id()
  (drivers/ptp/ptp_ocp.c:4503-4508) states "Returns -EBUSY if the bus is…
- [Low] ptp_ocp_i2c_notifier_call() (drivers/ptp/ptp_ocp.c:5590-5602)
  publishes and invalidates bp->cpld_i2c_adap_nr for *any* i2c adapter…
- [Low] The new Documentation/ABI/testing/sysfs-timecard entry for
  cpld_status states "The Lattice device ID of the CPLD is read once at…
- [Low] Documentation/networking/devlink/ptp_ocp.rst claims to describe
  "the devlink features implemented by the ptp_ocp device driver" and is…
- [Low] bp->cpld_id is written by adva_x1_cpld_read_id()
  (drivers/ptp/ptp_ocp.c:4525) from the sync worker under cpld_lock, but…

Pre-existing issues:
- [High] In ptp_ocp_probe(), if any dpll_pin_get() or dpll_pin_register()
  fails after dpll_device_register() succeeded, the out_dpll path…
- [Medium] In ptp_ocp_probe(), when dpll_device_register() fails (line
  5493-5494) the code jumps to the new out_devlink label, which calls…

--

ptp: ocp: add TAP CPLD access for ADVA TimeCard X1

This adds a request/grant handshake that re-routes the host I2C controller
from the config EEPROMs onto the TMC bus, plus read-only access to the
Lattice CPLD behind the PCA9548 on that bus.  The CPLD device ID is read
once from the sync worker and reported as the fixed devlink version
"cpld.id", and a new root-only cpld_status sysfs attribute reports the
CPLD status register.

A few questions below.

> diff --git a/Documentation/ABI/testing/sysfs-timecard b/Documentation/ABI/testing/sysfs-timecard
> index 3ae41b7634ac..70ecc15b347f 100644
> --- a/Documentation/ABI/testing/sysfs-timecard
> +++ b/Documentation/ABI/testing/sysfs-timecard
> @@ -11,6 +11,26 @@ Contact:	Jonathan Lemon <jonathan.lemon@gmail.com>
>  Description:	This directory contains the attributes of the Nth timecard
>  		registered.
>  
> +What:		/sys/class/timecard/ocpN/cpld_status
> +Date:		July 2026
> +Contact:	Sagi Maimon <maimon.sagi@gmail.com>
> +Description:	(RO, root only) The status register of the TAP CPLD, in
> +		human-readable form:
> +
> +		  done=<0|1> busy=<0|1> failed=<0|1>
> +
> +		Only present on ADVA x1 TAP boards (PCI ID 0xad5a:0x0410).
> +
> +		done=1 indicates the configuration flash was successfully
> +		programmed and is active.  busy=1 means an internal operation
> +		is in progress.  failed=1 means the last ISC operation failed.
> +
> +		A read arbitrates for the shared I2C bus and reprograms the
> +		on-card mux, so it is restricted to root.  The Lattice device
> +		ID of the CPLD is read once at probe and reported as the
> +		fixed "cpld.id" version by devlink dev info.

[Severity: Low]
Is the "read once at probe" wording accurate?  ptp_ocp_probe() only
initialises cpld_lock and sets cpld_i2c_adap_nr = -1; the only caller of
adva_x1_cpld_read_id() is ptp_ocp_sync_work(), so the earliest the ID can
be read is one sync_work tick (HZ) after probe returns, and only once the
adapter number has been cached by the notifier.

Documentation/networking/devlink/ptp_ocp.rst added by the same patch says
the opposite:

  so the driver does that once from its
  own worker and reports the cached value here

Should the ABI text be reworded to match, so the two files agree?

> diff --git a/Documentation/networking/devlink/ptp_ocp.rst b/Documentation/networking/devlink/ptp_ocp.rst
> new file mode 100644
> index 000000000000..3f997cd9f3a3
> --- /dev/null
> +++ b/Documentation/networking/devlink/ptp_ocp.rst
> @@ -0,0 +1,35 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +========================
> +ptp_ocp devlink support
> +========================
> +
> +This document describes the devlink features implemented by the ``ptp_ocp``
> +device driver.
> +
> +Info versions
> +=============

[Severity: Low]
The file states it describes "the devlink features implemented by the
ptp_ocp device driver", and index.rst says each driver file documents
"parameters, info versions, and other features it supports", but only info
versions are covered here.

ptp_ocp_devlink_ops also implements flash update:

	.flash_update = ptp_ocp_devlink_flash_update,
	.info_get = ptp_ocp_devlink_info_get,

Would it make sense to add a flash update section, or to narrow the
introductory sentence to info versions?

[ ... ]

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 3d26ec1f7b9e..7db1dc9b94bd 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c

[ ... ]

> @@ -2166,6 +2187,17 @@ ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
>  	if (err)
>  		return err;
>  
> +	/* Read by the worker, not here: this command is unprivileged and
> +	 * reading the ID claims the I2C bus.  0 means unread - skip it
> +	 * rather than fail, devlink discards the whole reply on error.
> +	 */
> +	if (bp->cpld_id) {
> +		sprintf(buf, "0x%08x", bp->cpld_id);
> +		err = devlink_info_version_fixed_put(req, "cpld.id", buf);
> +		if (err)
> +			return err;
> +	}

[Severity: Low]
Should these two reads of bp->cpld_id use READ_ONCE()?

The field is written in adva_x1_cpld_read_id() under cpld_lock:

	if (!ret)
		bp->cpld_id = get_unaligned_be32(data);

while this path reads it with no lock at all, so the test and the sprintf()
argument are two independent plain loads that the compiler may reload.
Today the value only ever transitions 0 -> ID once, so the effect is limited
to a KCSAN report, but the neighbouring lock-free field is already annotated
(READ_ONCE(bp->cpld_i2c_adap_nr) / WRITE_ONCE()), so the asymmetry stands
out.  bp->cpld_id_tried is a plain read/write as well.

[ ... ]

> @@ -4226,6 +4259,319 @@ static const struct ocp_attr_group art_timecard_groups[] = {
>  	{ },
>  };
>  

[ ... ]

> +static void adva_x1_mblaze_release(struct ptp_ocp *bp)
> +{
> +	if (bp->pps_select)
> +		iowrite32(MBLAZE_RELEASE, &bp->pps_select->i2c_bus_ctrl);
> +}
> +
> +/* Acquire the shared I2C bus from the MicroBlaze firmware.  Returns with no
> + * request outstanding on failure, so the firmware is never left granting a
> + * segment to a host that has given up waiting for it.
> + */
> +static int adva_x1_mblaze_acquire(struct ptp_ocp *bp)
> +{
> +	u32 val;
> +	int i;
> +
> +	if (!bp->pps_select)
> +		return -ENODEV;
> +
> +	/* Drop a request left by a caller that died mid-sequence; cpld_lock
> +	 * keeps live ones out.  The read back only flushes the posted write.
> +	 */
> +	iowrite32(0, &bp->pps_select->i2c_bus_ctrl);
> +	ioread32(&bp->pps_select->i2c_bus_ctrl);
> +
> +	iowrite32(MBLAZE_REQUEST, &bp->pps_select->i2c_bus_ctrl);
> +	for (i = 0; i < MBLAZE_RETRIES; i++) {
> +		usleep_range(MBLAZE_RETRY_US, MBLAZE_RETRY_US + 1000);
> +		val = ioread32(&bp->pps_select->i2c_bus_ctrl);
> +		if (val == MBLAZE_GRANTED)
> +			return 0;
> +	}
> +
> +	adva_x1_mblaze_release(bp);
> +	return -ETIMEDOUT;
> +}
> +
> +/* Route the host controller back to the EEPROMs and release the adapter.
> + * Safe after a failed claim: it also clears a request that was never granted.
> + */
> +static void adva_x1_bus_release(struct ptp_ocp *bp)
> +{
> +	struct i2c_adapter *adap = bp->cpld_adap;
> +
> +	if (!adap)
> +		return;
> +
> +	adva_x1_mblaze_release(bp);
> +	bp->cpld_adap = NULL;
> +	kfree(bp->cpld_buf);
> +	bp->cpld_buf = NULL;
> +	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> +	i2c_put_adapter(adap);
> +}

[Severity: Medium]
Can I2C traffic be issued on the TMC bus after the release?

The acquire direction of this handshake is fully synchronised: the posted
write is flushed with ioread32() and then the grant is polled for up to
200 x 10 ms.  The release direction in adva_x1_mblaze_release() is a bare
iowrite32(MBLAZE_RELEASE, ...) with no read-back to flush it, and
adva_x1_bus_release() then calls i2c_unlock_bus() immediately, without
polling for confirmation that the MicroBlaze has re-routed the 0x150000
controller back to the EEPROM segment.

As soon as i2c_unlock_bus() returns, a waiting ptp_ocp_read_eeprom(), an
nvmem attribute read or an at24 sysfs transfer can start clocking out
transactions:

	adva_x1_mblaze_release(bp);	/* posted, not flushed */
	...
	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);

If the firmware's re-route latency is non-zero, would those transactions
land on the TMC bus, i.e. the exact wrong-bus traffic the commit message
says the adapter lock exists to prevent?  An EEPROM read could then fill
bp->serial / bp->board_id from an unrelated TMC slave, and an at24 write
could be delivered to one.  Since the acquire path treats this register
transition as slow enough to need a flush plus a 2 s poll, should the
release path flush and confirm too?

[ ... ]

> +/*
> + * Read the Lattice device ID into bp->cpld_id.  Done once, off the
> + * unprivileged devlink path, which reports the cached value only.
> + * Returns -EBUSY if the bus is busy, so the caller can retry rather than
> + * wait out a firmware upload.  The LCMXO3LF-2100 IDCODE is 0xe12bc043.
> + */
> +static int adva_x1_cpld_read_id(struct ptp_ocp *bp)
> +{
> +	u8 data[4];
> +	int ret;
> +
> +	if (!mutex_trylock(&bp->cpld_lock))
> +		return -EBUSY;

[Severity: Low]
The comment says "Returns -EBUSY if the bus is busy, so the caller can
retry rather than wait", but the only source of -EBUSY is this
mutex_trylock() on cpld_lock.  Actual I2C bus contention is handled with a
blocking lock inside adva_x1_bus_claim():

	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);

so when ptp_ocp_read_eeprom(), an nvmem attribute or an at24 transfer holds
the root adapter lock, the sync worker waits there instead of returning
-EBUSY.  Could the comment say cpld_lock rather than "the bus"?  The
reference to waiting out a firmware upload only becomes meaningful with the
later "add TAP CPLD firmware upload for ADVA TimeCard X1" patch, which adds
a long-running cpld_lock holder.

> +	ret = adva_x1_bus_claim(bp);
> +	if (ret)
> +		goto out;
> +	ret = adva_x1_mux_select(bp, ADVA_MUX_CHANNEL);
> +	if (ret)
> +		goto release;
> +	ret = adva_x1_cpld_cmd_read(bp, CPLD_CMD_READ_ID, data, 4);
> +	if (!ret)
> +		bp->cpld_id = get_unaligned_be32(data);
> +	adva_x1_mux_select(bp, -1);
> +release:
> +	adva_x1_bus_release(bp);
> +out:
> +	mutex_unlock(&bp->cpld_lock);
> +	if (ret)
> +		dev_dbg(&bp->pdev->dev,
> +			"CPLD device ID unavailable: %d\n", ret);
> +
> +	return ret;
> +}

[ ... ]

> @@ -5052,6 +5400,14 @@ ptp_ocp_sync_work(struct work_struct *work)
>  
>  	bp->sync = sync;
>  
> +	/* The i2c adapter is registered after probe returns, so the ID is
> +	 * read from here, once, as soon as the adapter shows up.
> +	 */
> +	if (bp->has_cpld && !bp->cpld_id_tried &&
> +	    READ_ONCE(bp->cpld_i2c_adap_nr) >= 0 &&
> +	    adva_x1_cpld_read_id(bp) != -EBUSY)
> +		bp->cpld_id_tried = true;
> +
>  	queue_delayed_work(system_power_efficient_wq, &bp->sync_work, HZ);
>  }

[Severity: Medium]
Should transient failures also keep the retry alive here?  Only -EBUSY
avoids latching cpld_id_tried, and -EBUSY comes solely from the
mutex_trylock() in adva_x1_cpld_read_id().  Every other failure looks
transient:

  - -ETIMEDOUT from adva_x1_mblaze_acquire() when the MicroBlaze does not
    grant the TMC segment within the 200 x 10 ms poll (e.g. busy firmware
    right after power-up)
  - -ENOMEM from the kzalloc() in adva_x1_bus_claim()
  - the controller errno or -EIO from __i2c_transfer()

Any one of those on the single attempt leaves bp->cpld_id == 0 for the life
of the card, so the "if (bp->cpld_id)" test in ptp_ocp_devlink_info_get()
never becomes true and cpld.id is omitted permanently, with only a dev_dbg()
to show why.

Documentation/networking/devlink/ptp_ocp.rst says:

  the version is
  omitted until that read has succeeded.

which reads as though the driver keeps trying.  Would treating -ETIMEDOUT
and -ENOMEM the same way as -EBUSY match that description?

> @@ -5119,12 +5486,12 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (IS_ERR(bp->dpll)) {
>  		err = PTR_ERR(bp->dpll);
>  		dev_err(&pdev->dev, "dpll_device_alloc failed\n");
> -		goto out;
> +		goto out_devlink;
>  	}
>  
>  	err = dpll_device_register(bp->dpll, DPLL_TYPE_PPS, &dpll_ops, bp);
>  	if (err)
> -		goto out;
> +		goto out_devlink;

[Severity: Medium]
These two redirects look like a fix for a separate, existing bug rather than
part of the CPLD feature.

devlink_register(devlink) runs before both call sites, and in the baseline
both used "goto out", reaching ptp_ocp_detach() and then
devlink_free(devlink) with the instance still registered.  devlink_free()
starts with:

net/devlink/core.c:devlink_free() {
	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
	...
}

so the old paths would trip that assertion.  The new comment on the label
attributes the change to cpld_lock teardown instead:

	/* detach() destroys cpld_lock; no devlink op may still reach it */

Could this be split out as its own patch with a Fixes: tag, so it can be
reviewed and backported independently of the CPLD work?

[Severity: Medium]
This isn't a bug introduced by this patch (the baseline used "goto out"
here, which skipped the same call), but since these gotos are being rewritten
anyway: on the dpll_device_register() failure path, is the reference taken a
few lines earlier by dpll_device_get(clkid, 0, THIS_MODULE, &bp->tracker)
ever dropped?

out_devlink falls into "out: ptp_ocp_detach(bp)", ptp_ocp_detach() does not
touch bp->dpll, and only out_dpll calls dpll_device_put().  So a failing
dpll_device_register() (-ENOMEM or -EEXIST) leaves the dpll_device in
dpll_device_xa with a leaked reference and a stale ref-tracker entry.

>  
>  	for (i = 0; i < OCP_SMA_NUM; i++) {
>  		bp->sma[i].dpll_pin = dpll_pin_get(clkid, i, THIS_MODULE,
> @@ -5151,6 +5518,9 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		dpll_pin_put(bp->sma[i].dpll_pin, &bp->sma[i].tracker);
>  	}
>  	dpll_device_put(bp->dpll, &bp->tracker);
> +out_devlink:
> +	/* detach() destroys cpld_lock; no devlink op may still reach it */
> +	devlink_unregister(devlink);
>  out:
>  	ptp_ocp_detach(bp);
>  out_disable:

[Severity: High]
This is a pre-existing issue and not introduced by this patch (the same code
is in the baseline; the patch only redirects the earlier gotos), but it sits
directly in the path being edited.

Should out_dpll call dpll_device_unregister(bp->dpll, &dpll_ops, bp) before
dpll_device_put()?  dpll_device_register() takes a second reference and
stores bp:

drivers/dpll/dpll_core.c:dpll_device_register() {
	...
	reg->ops = ops;
	reg->priv = priv;
	dpll->type = type;
	__dpll_device_hold(dpll, &reg->tracker);
	...
}

and only dpll_device_unregister() drops it:

drivers/dpll/dpll_core.c:dpll_device_unregister() {
	...
	list_del(&reg->list);
	__dpll_device_put(dpll, &reg->tracker);
	kfree(reg);
	...
}

If a dpll_pin_get() or dpll_pin_register() fails after dpll_device_register()
succeeded, out_dpll drops only bp->tracker, so the device stays registered
and visible to userspace with reg->priv still pointing at bp, while probe
continues to ptp_ocp_detach() and devlink_free(devlink), which frees the
storage holding bp.  Can a later dpll netlink get or dump then call the
dpll_ops callbacks on freed memory, in addition to leaking the registration
object?

> @@ -5219,11 +5589,17 @@ ptp_ocp_i2c_notifier_call(struct notifier_block *nb,
>  
>  found:
>  	bp = dev_get_drvdata(dev);
> -	if (add)
> +	if (add) {
>  		ptp_ocp_symlink(bp, child, "i2c");
> -	else
> +		/* Cache the number; adva_x1_bus_claim() takes the reference
> +		 * and re-checks the parent, as the number can be recycled.
> +		 */
> +		WRITE_ONCE(bp->cpld_i2c_adap_nr, i2c_verify_adapter(child)->nr);
> +	} else {
> +		/* invalidate before free */
> +		WRITE_ONCE(bp->cpld_i2c_adap_nr, -1);
>  		sysfs_remove_link(&bp->dev.kobj, "i2c");
> -
> +	}
>  	return 0;
>  }

[Severity: Low]
Should this side apply the same identity check that the consumer applies?

The notifier publishes and invalidates cpld_i2c_adap_nr for any i2c adapter
whose ancestor walk reaches a device bound to ptp_ocp, while
adva_x1_bus_claim() insists on the card's own controller:

	if (!bp->i2c_ctrl || adap->dev.parent != &bp->i2c_ctrl->dev) {
		i2c_put_adapter(adap);
		return -ENODEV;
	}

i2c-mux child adapters pass the notifier filter but fail that test, because
i2c_mux_add_adapter() sets:

	priv->adap.dev.parent = &parent->dev;

so if root instantiates a mux on the card's adapter (writing "pca9548 0x74"
to new_device), each child's ADD notification overwrites cpld_i2c_adap_nr
with the mux segment number and every later claim returns -ENODEV; a child's
DEL notification writes -1 while the card's own controller is still alive.
There is no re-arm path in either case, so cpld_status and the one-shot
cpld.id read stay disabled for the life of the card.  Would testing
i2c_verify_adapter(child)->dev.parent == &bp->i2c_ctrl->dev here avoid that?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908122328.52641-1-maimon.sagi%40gmail.com

Re: [PATCH net-next v13 1/2] ptp: ocp: add TAP CPLD access for ADVA TimeCard X1

[Jakub Kicinski]

On Tue,  8 Sep 2026 15:23:27 +0300 Sagi Maimon wrote:
> +		  done=<0|1> busy=<0|1> failed=<0|1>

I think the sysfs rules are "one value per file"
We can stretch it a bit and say that this file contains flags

So format like done or busy or failed or multiple eg "busy failed"
But the X=[0|1] is too much

Also I think the AI is right this one commit does too many things.

[PATCH net-next v13 2/2] ptp: ocp: add TAP CPLD firmware upload for ADVA TimeCard X1

[Sagi Maimon]

The Lattice MachXO3 CPLD on the ADVA TimeCard X1 is programmed over I2C
using in-system programming (ISP).  Build on the TMC bus arbitration
added previously and expose the update path through the kernel
firmware-upload subsystem.

The framework acquires the bus, erases the configuration flash, programs
the image page-by-page and activates it with the MachXO3 REFRESH
command.  The upload node is registered per card as adva-cpld.N, using
the same index as the owning ocpN device, so a host with more than one
X1 board gets one node each:

  /sys/class/firmware/adva-cpld.N/

The whole prepare/write/poll_complete/cleanup sequence runs under
cpld_lock and the i2c adapter lock, so an EEPROM read blocks for as long
as programming takes; the alternative is reading the TMC bus instead.
The upload is unregistered first on detach, which cancels and flushes an
in-flight programming cycle while the I2C controller is still up.

Select FW_LOADER and FW_UPLOAD, as the documented update path does not
exist without them.

Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 Documentation/ABI/testing/sysfs-timecard |   4 +
 drivers/ptp/Kconfig                      |   2 +
 drivers/ptp/ptp_ocp.c                    | 381 ++++++++++++++++++++++-
 3 files changed, 383 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-timecard b/Documentation/ABI/testing/sysfs-timecard
index 70ecc15b347f..8c0d41c941ad 100644
--- a/Documentation/ABI/testing/sysfs-timecard
+++ b/Documentation/ABI/testing/sysfs-timecard
@@ -30,6 +30,10 @@ Description:	(RO, root only) The status register of the TAP CPLD, in
 		ID of the CPLD is read once at probe and reported as the
 		fixed "cpld.id" version by devlink dev info.
 
+		To program new CPLD firmware use the standard kernel
+		firmware-upload interface, registered per card at:
+		/sys/class/firmware/adva-cpld.N/
+		where N is the index of this ocpN device.
 
 What:		/sys/class/timecard/ocpN/available_clock_sources
 Date:		September 2021
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index feb50f8cc406..24f047c37e4c 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -219,6 +219,8 @@ config PTP_1588_CLOCK_OCP
 	select NET_DEVLINK
 	select CRC16
 	select DPLL
+	select FW_LOADER
+	select FW_UPLOAD
 	help
 	  This driver adds support for an OpenCompute time card.
 
diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 7db1dc9b94bd..6eefc3db586f 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -20,12 +20,14 @@
 #include <linux/spi/altera.h>
 #include <net/devlink.h>
 #include <linux/i2c.h>
+#include <linux/iopoll.h>
 #include <linux/mtd/mtd.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/crc16.h>
 #include <linux/dpll.h>
 #include <linux/unaligned.h>
 #include <linux/delay.h>
+#include <linux/firmware.h>
 
 #define PCI_DEVICE_ID_META_TIMECARD		0x0400
 
@@ -436,6 +438,12 @@ struct ptp_ocp {
 	bool			cpld_id_tried;
 	/* x1 TAP CPLD present */
 	bool			has_cpld;
+	/* firmware upload handle; NULL if absent */
+	struct fw_upload	*cpld_fw_upload;
+	/* cancellation requested */
+	bool			cpld_cancel;
+	/* EN_CFG_TP issued but not yet REFRESH'd */
+	bool			cpld_in_config_mode;
 };
 
 #define OCP_REQ_TIMESTAMP	BIT(0)
@@ -469,6 +477,8 @@ static int ptp_ocp_art_board_init(struct ptp_ocp *bp, struct ocp_resource *r);
 
 static int ptp_ocp_adva_board_init(struct ptp_ocp *bp, struct ocp_resource *r);
 
+static const struct fw_upload_ops adva_cpld_upload_ops;
+
 static const struct ocp_sma_op ocp_adva_sma_op;
 static const struct ocp_sma_op ocp_adva_x1_sma_op;
 
@@ -3232,6 +3242,29 @@ ptp_ocp_adva_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
 	ptp_ocp_sma_init(bp);
 
 	bp->has_cpld = info->has_cpld;
+	if (bp->has_cpld) {
+		struct fw_upload *fwl;
+		const char *name;
+
+		/* One instance per card, numbered like the ocpN device.
+		 * firmware_upload_register() keeps the pointer rather than
+		 * copying the string, so it has to outlive the registration.
+		 */
+		name = devm_kasprintf(&bp->pdev->dev, GFP_KERNEL,
+				      "adva-cpld.%d", bp->id);
+		if (!name)
+			return -ENOMEM;
+
+		fwl = firmware_upload_register(THIS_MODULE, &bp->pdev->dev,
+					       name, &adva_cpld_upload_ops, bp);
+		if (IS_ERR(fwl))
+			dev_warn(&bp->pdev->dev,
+				 "CPLD firmware upload unavailable: %pe\n",
+				 fwl);
+		else
+			bp->cpld_fw_upload = fwl;
+	}
+
 	return ptp_ocp_init_clock(bp, &info->servo);
 }
 
@@ -4293,6 +4326,15 @@ static const struct ocp_attr_group art_timecard_groups[] = {
 /* Lattice LCMXO3LF ISC command codes */
 #define CPLD_CMD_READ_ID      0xE0000000UL
 #define CPLD_CMD_READ_STATUS  0x3C000000UL
+#define CPLD_CMD_EN_CFG_TP    0x74   /* enable config, transparent mode */
+#define CPLD_CMD_DIS_CFG      0x26
+#define CPLD_CMD_ERASE        0x0E
+#define CPLD_CMD_RESET_ADDR   0x46
+#define CPLD_CMD_WRITE_PAGE   0x70
+#define CPLD_CMD_SET_DONE     0x5E
+#define CPLD_CMD_REFRESH      0x79
+#define CPLD_PAGE_SIZE        16
+#define CPLD_POLL_US          10000  /* status poll interval while busy */
 
 /* Status register bit positions (Lattice LCMXO3LF datasheet) */
 #define CPLD_STATUS_DONE   BIT(8)
@@ -4307,9 +4349,10 @@ static const struct ocp_attr_group art_timecard_groups[] = {
  *
  * The message is assembled in the scratch buffer taken by
  * adva_x1_bus_claim(), so an opcode and its arguments are copied exactly
- * once.  I2C_M_DMA_SAFE is deliberately not set: no adapter that can bind
- * here does DMA, and the two halves are not separately aligned, so the
- * core is left free to bounce.
+ * once and an upload costs one allocation, not one per page.
+ * I2C_M_DMA_SAFE is deliberately not set: no adapter that can bind here
+ * does DMA, and the two halves are not separately aligned, so the core is
+ * left free to bounce.
  *
  * Caller must hold that claim, hence __i2c_transfer() over i2c_transfer().
  */
@@ -4447,7 +4490,9 @@ static int adva_x1_bus_claim(struct ptp_ocp *bp)
 		return -ENODEV;
 	}
 
-	/* One scratch buffer per claim, not per transfer. */
+	/* One scratch buffer per claim rather than per transfer: an upload
+	 * holds the claim for the whole image.
+	 */
 	bp->cpld_buf = kzalloc(2 * ADVA_CPLD_XFER_MAX, GFP_KERNEL);
 	if (!bp->cpld_buf) {
 		i2c_put_adapter(adap);
@@ -4475,6 +4520,48 @@ static int adva_x1_mux_select(struct ptp_ocp *bp, int ch)
 	return adva_x1_i2c_xfer(bp, ADVA_MUX_ADDR, val, NULL, 0, NULL, 0);
 }
 
+/*
+ * Argument bytes that follow an ISC opcode.  Returns NULL with @nargs set
+ * when the arguments are all zero: adva_x1_i2c_xfer() zeroes the buffer.
+ */
+static const u8 *adva_x1_cpld_args(u8 cmd, u8 *nargs)
+{
+	static const u8 en_cfg_tp[] = { 0x08, 0x00 };
+	/* cfg sector only */
+	static const u8 erase_cfg[] = { 0x04, 0x00, 0x00 };
+
+	switch (cmd) {
+	case CPLD_CMD_EN_CFG_TP:
+		*nargs = sizeof(en_cfg_tp);
+		return en_cfg_tp;
+	case CPLD_CMD_ERASE:
+		*nargs = sizeof(erase_cfg);
+		return erase_cfg;
+	case CPLD_CMD_RESET_ADDR:
+	case CPLD_CMD_SET_DONE:
+		*nargs = 3;
+		return NULL;
+	case CPLD_CMD_DIS_CFG:
+	case CPLD_CMD_REFRESH:
+		*nargs = 2;
+		return NULL;
+	default:
+		*nargs = 0;
+		return NULL;
+	}
+}
+
+/* Send an ISC command with the fixed arguments that belong to it. */
+static int adva_x1_cpld_write(struct ptp_ocp *bp, u8 cmd)
+{
+	const u8 *args;
+	u8 nargs;
+
+	args = adva_x1_cpld_args(cmd, &nargs);
+
+	return adva_x1_i2c_xfer(bp, ADVA_CPLD_ADDR, cmd, args, nargs, NULL, 0);
+}
+
 /*
  * Send a 4-byte command then read data back without an intermediate STOP
  * (Lattice combined write->repeated-START->read).  Two messages in one
@@ -4500,6 +4587,55 @@ static int adva_x1_cpld_read_status(struct ptp_ocp *bp, u32 *status)
 	return 0;
 }
 
+/* Poll the status register until the CPLD goes idle, or @max_ms elapses.
+ * The deadline is on wall time, so the I2C transactions count against it,
+ * and the status is read once more after it expires before giving up.
+ */
+static int adva_x1_cpld_wait_ready(struct ptp_ocp *bp, unsigned int max_ms)
+{
+	u32 status = 0;
+	int err, ret;
+
+	ret = read_poll_timeout(adva_x1_cpld_read_status, err,
+				err || READ_ONCE(bp->cpld_cancel) ||
+				(status & CPLD_STATUS_FAILED) ||
+				!(status & CPLD_STATUS_BUSY),
+				CPLD_POLL_US, max_ms * USEC_PER_MSEC, false,
+				bp, &status);
+	if (ret)
+		return ret;
+	if (READ_ONCE(bp->cpld_cancel))
+		return -ECANCELED;
+	if (err || (status & CPLD_STATUS_FAILED))
+		return -EIO;
+
+	return 0;
+}
+
+/* Map an errno onto the category userspace reads back from
+ * /sys/class/firmware/adva-cpld.N/error.  A step aborted by cancel() must
+ * be reported as such, not as a HW error.
+ */
+static enum fw_upload_err adva_cpld_err(struct ptp_ocp *bp, int err)
+{
+	if (READ_ONCE(bp->cpld_cancel))
+		return FW_UPLOAD_ERR_CANCELED;
+
+	switch (err) {
+	case -ECANCELED:
+		return FW_UPLOAD_ERR_CANCELED;
+	case -ETIMEDOUT:
+		return FW_UPLOAD_ERR_TIMEOUT;
+	case -ENOMEM:
+		return FW_UPLOAD_ERR_BUSY;
+	case -ENODEV:
+	case -EIO:		/* FAILED status, or a short transfer */
+		return FW_UPLOAD_ERR_HW_ERROR;
+	default:		/* from the i2c controller itself */
+		return FW_UPLOAD_ERR_RW_ERROR;
+	}
+}
+
 /*
  * Read the Lattice device ID into bp->cpld_id.  Done once, off the
  * unprivileged devlink path, which reports the cached value only.
@@ -4572,6 +4708,234 @@ cpld_status_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_ADMIN_RO(cpld_status);
 
+/*
+ * adva_x1 CPLD firmware-upload callbacks.
+ *
+ * The kernel firmware-upload subsystem (CONFIG_FW_UPLOAD) exposes:
+ *   /sys/class/firmware/adva-cpld.N/{data,loading,status,error,...}
+ * where N is the index of the owning ocpN device.
+ * Userspace writes the raw binary page data directly — no /lib/firmware/
+ * staging file is needed.
+ *
+ * Callback sequence driven by the framework:
+ *   prepare()      - validate size, acquire bus, enable config, erase flash
+ *   write()        - program one 16-byte page per call
+ *   poll_complete()- set DONE, REFRESH, wait for CPLD to reboot
+ *   cancel()       - set flag; checked on entry to each callback
+ *   cleanup()      - release bus resources (called on success or failure)
+ */
+static enum fw_upload_err
+adva_cpld_prepare(struct fw_upload *fwl, const u8 *data, u32 size)
+{
+	enum fw_upload_err ret = FW_UPLOAD_ERR_NONE;
+	struct ptp_ocp *bp = fwl->dd_handle;
+	int err;
+
+	/* Do not clear cpld_cancel here: fw_upload_start() queues the work
+	 * before this runs, so a cancel may already have arrived - honour it
+	 * before touching the hardware.  It is cleared once the upload is
+	 * over, on every exit below and in cleanup().
+	 */
+	if (READ_ONCE(bp->cpld_cancel)) {
+		WRITE_ONCE(bp->cpld_cancel, false);
+		return FW_UPLOAD_ERR_CANCELED;
+	}
+
+	if (!size || size % CPLD_PAGE_SIZE) {
+		WRITE_ONCE(bp->cpld_cancel, false);
+		return FW_UPLOAD_ERR_INVALID_SIZE;
+	}
+
+	bp->cpld_in_config_mode = false;
+
+	mutex_lock(&bp->cpld_lock);
+
+	err = adva_x1_bus_claim(bp);
+	if (err) {
+		ret = adva_cpld_err(bp, err);
+		goto err_unlock;
+	}
+
+	err = adva_x1_mux_select(bp, ADVA_MUX_CHANNEL);
+	if (err) {
+		ret = adva_cpld_err(bp, err);
+		goto err_release;
+	}
+
+	/* Set before issuing EN_CFG_TP, not after it completes: the CPLD may
+	 * have entered configuration mode even if the write reports an error
+	 * or the wait below times out, and err_deselect only sends DIS_CFG
+	 * when this is set.  A DIS_CFG to a device that never entered the
+	 * mode is harmless; leaving it enabled is not.
+	 */
+	bp->cpld_in_config_mode = true;
+
+	err = adva_x1_cpld_write(bp, CPLD_CMD_EN_CFG_TP);
+	if (!err)
+		err = adva_x1_cpld_wait_ready(bp, 5000);
+	if (err) {
+		ret = adva_cpld_err(bp, err);
+		goto err_deselect;
+	}
+
+	if (READ_ONCE(bp->cpld_cancel)) {
+		ret = FW_UPLOAD_ERR_CANCELED;
+		goto err_deselect;
+	}
+
+	err = adva_x1_cpld_write(bp, CPLD_CMD_ERASE);
+	if (!err)
+		err = adva_x1_cpld_wait_ready(bp, 15000);
+	if (err) {
+		ret = adva_cpld_err(bp, err);
+		goto err_deselect;
+	}
+
+	if (READ_ONCE(bp->cpld_cancel)) {
+		ret = FW_UPLOAD_ERR_CANCELED;
+		goto err_deselect;
+	}
+
+	err = adva_x1_cpld_write(bp, CPLD_CMD_RESET_ADDR);
+	if (err) {
+		ret = adva_cpld_err(bp, err);
+		goto err_deselect;
+	}
+
+	/* cleanup() unlocks everything.  fw_upload_main() only pairs it with
+	 * a prepare() that succeeded, so the error paths below unlock here
+	 * instead; hand the context to cleanup() for sparse's benefit.
+	 */
+	__release(&bp->cpld_lock);
+	return FW_UPLOAD_ERR_NONE;
+
+err_deselect:
+	if (bp->cpld_in_config_mode) {
+		adva_x1_cpld_write(bp, CPLD_CMD_DIS_CFG);
+		bp->cpld_in_config_mode = false;
+	}
+	adva_x1_mux_select(bp, -1);
+err_release:
+	adva_x1_bus_release(bp);
+err_unlock:
+	WRITE_ONCE(bp->cpld_cancel, false);
+	mutex_unlock(&bp->cpld_lock);
+	return ret;
+}
+
+static enum fw_upload_err
+adva_cpld_write(struct fw_upload *fwl, const u8 *data,
+		u32 offset, u32 size, u32 *written)
+{
+	struct ptp_ocp *bp = fwl->dd_handle;
+	u8 args[3 + CPLD_PAGE_SIZE] = { 0x00, 0x00, 0x01 };
+	int err;
+
+	lockdep_assert_held(&bp->cpld_lock);
+
+	if (READ_ONCE(bp->cpld_cancel))
+		return FW_UPLOAD_ERR_CANCELED;
+
+	if (size < CPLD_PAGE_SIZE)
+		return FW_UPLOAD_ERR_INVALID_SIZE;
+
+	memcpy(&args[3], data + offset, CPLD_PAGE_SIZE);
+
+	err = adva_x1_i2c_xfer(bp, ADVA_CPLD_ADDR, CPLD_CMD_WRITE_PAGE,
+			       args, sizeof(args), NULL, 0);
+	if (!err)
+		err = adva_x1_cpld_wait_ready(bp, 100);
+	if (err)
+		return adva_cpld_err(bp, err);
+
+	*written = CPLD_PAGE_SIZE;
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static enum fw_upload_err
+adva_cpld_poll_complete(struct fw_upload *fwl)
+{
+	struct ptp_ocp *bp = fwl->dd_handle;
+	int err;
+	u32 st;
+
+	lockdep_assert_held(&bp->cpld_lock);
+
+	if (READ_ONCE(bp->cpld_cancel))
+		return FW_UPLOAD_ERR_CANCELED;
+
+	err = adva_x1_cpld_write(bp, CPLD_CMD_SET_DONE);
+	if (!err)
+		err = adva_x1_cpld_wait_ready(bp, 1000);
+	if (err)
+		return adva_cpld_err(bp, err);
+
+	err = adva_x1_cpld_read_status(bp, &st);
+	if (err)
+		return adva_cpld_err(bp, err);
+	if (!(st & CPLD_STATUS_DONE))
+		return FW_UPLOAD_ERR_HW_ERROR;
+
+	err = adva_x1_cpld_write(bp, CPLD_CMD_REFRESH);
+	if (err)
+		return adva_cpld_err(bp, err);
+
+	/* REFRESH reboots the CPLD out of configuration mode, so cleanup()
+	 * must not send DIS_CFG afterwards even if the checks below fail.
+	 */
+	bp->cpld_in_config_mode = false;
+
+	/* The new image is already running at this point, so a segment that
+	 * is not back yet must not be reported as a failed update: retry the
+	 * reselect instead of sampling the mux once at a fixed delay.
+	 */
+	msleep(1500);
+	if (read_poll_timeout(adva_x1_mux_select, err, !err, CPLD_POLL_US,
+			      3000 * USEC_PER_MSEC, false,
+			      bp, ADVA_MUX_CHANNEL))
+		return FW_UPLOAD_ERR_TIMEOUT;
+
+	err = adva_x1_cpld_wait_ready(bp, 3000);
+	if (err)
+		return adva_cpld_err(bp, err);
+
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static void
+adva_cpld_cancel(struct fw_upload *fwl)
+{
+	struct ptp_ocp *bp = fwl->dd_handle;
+
+	WRITE_ONCE(bp->cpld_cancel, true);
+}
+
+static void
+adva_cpld_cleanup(struct fw_upload *fwl)
+{
+	struct ptp_ocp *bp = fwl->dd_handle;
+
+	__acquire(&bp->cpld_lock);	/* held since prepare() returned ok */
+	lockdep_assert_held(&bp->cpld_lock);
+
+	if (bp->cpld_in_config_mode) {
+		adva_x1_cpld_write(bp, CPLD_CMD_DIS_CFG);
+		bp->cpld_in_config_mode = false;
+	}
+	adva_x1_mux_select(bp, -1);
+	adva_x1_bus_release(bp);
+	WRITE_ONCE(bp->cpld_cancel, false);
+	mutex_unlock(&bp->cpld_lock);
+}
+
+static const struct fw_upload_ops adva_cpld_upload_ops = {
+	.prepare	 = adva_cpld_prepare,
+	.write		 = adva_cpld_write,
+	.poll_complete	 = adva_cpld_poll_complete,
+	.cancel		 = adva_cpld_cancel,
+	.cleanup	 = adva_cpld_cleanup,
+};
+
 static struct attribute *adva_timecard_attrs[] = {
 	&dev_attr_serialnum.attr,
 	&dev_attr_gnss_sync.attr,
@@ -5214,6 +5578,15 @@ ptp_ocp_detach(struct ptp_ocp *bp)
 {
 	int i;
 
+	/* Must come first: cancels and flushes an in-flight upload while the
+	 * I2C controller is still up, and drops cpld_lock so a cpld_status
+	 * reader cannot stall ptp_ocp_attr_group_del() below.
+	 */
+	if (bp->cpld_fw_upload) {
+		firmware_upload_unregister(bp->cpld_fw_upload);
+		bp->cpld_fw_upload = NULL;
+	}
+
 	ptp_ocp_debugfs_remove_device(bp);
 	ptp_ocp_detach_sysfs(bp);
 	ptp_ocp_attr_group_del(bp);
-- 
2.47.0

Re: [PATCH net-next v13 2/2] ptp: ocp: add TAP CPLD firmware upload for ADVA TimeCard X1

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 3 · Low: 3

- [Medium] bp->cpld_cancel is a single per-device boolean cleared at
  driver-chosen points (entry of adva_cpld_prepare(), prepare()'s…
- [Medium] adva_x1_cpld_wait_ready() returns as soon as
  READ_ONCE(bp->cpld_cancel) is observed (-ECANCELED) or a status read…
- [Medium] adva_cpld_poll_complete() reports FW_UPLOAD_ERR_NONE without
  verifying that the CPLD reconfigured successfully.
- [Low] The new paragraph in Documentation/ABI/testing/sysfs-timecard
  directs userspace to program the CPLD via /sys/class/firmware/adva-…
- [Low] In adva_cpld_poll_complete() the post-REFRESH mux-reselect poll
  `read_poll_timeout(adva_x1_mux_select, err, !err, CPLD_POLL_US, 3000 *…
- [Low] adva_x1_cpld_wait_ready() exits its poll as soon as the status
  read fails (`err` is part of the termination condition) but then…

Pre-existing issues:
- [High] ptp_ocp_detach() relies on firmware_upload_unregister() to
  cancel *and* flush an in-flight upload before cpld_lock, the i2c_ctrl…

--

ptp: ocp: add TAP CPLD firmware upload for ADVA TimeCard X1

This adds an in-system programming path for the Lattice MachXO3 CPLD on the
ADVA TimeCard X1, exposed through the kernel firmware-upload subsystem as
/sys/class/firmware/adva-cpld.N/.  The prepare/write/poll_complete/cleanup
callbacks run under cpld_lock with the i2c adapter lock held, and the upload
node is unregistered first in ptp_ocp_detach().

A few questions below.

> diff --git a/Documentation/ABI/testing/sysfs-timecard b/Documentation/ABI/testing/sysfs-timecard
> index 70ecc15b347f..8c0d41c941ad 100644
> --- a/Documentation/ABI/testing/sysfs-timecard
> +++ b/Documentation/ABI/testing/sysfs-timecard
> @@ -30,6 +30,10 @@ Description:	(RO, root only) The status register of the TAP CPLD, in
>  		ID of the CPLD is read once at probe and reported as the
>  		fixed "cpld.id" version by devlink dev info.
>  
> +		To program new CPLD firmware use the standard kernel
> +		firmware-upload interface, registered per card at:
> +		/sys/class/firmware/adva-cpld.N/
> +		where N is the index of this ocpN device.

[Severity: Low]
Should this paragraph also state the image format the driver accepts?

adva_cpld_prepare() rejects anything that is not an exact multiple of
CPLD_PAGE_SIZE:

	if (!size || size % CPLD_PAGE_SIZE) {
		WRITE_ONCE(bp->cpld_cancel, false);
		return FW_UPLOAD_ERR_INVALID_SIZE;
	}

and adva_cpld_write() copies the bytes verbatim into the page-program
payload, so the node takes a raw page-aligned bitstream rather than a
Lattice .jed/.hex container.  The internal comment above the callbacks does
say "Userspace writes the raw binary page data directly", but the ABI text
does not, and the only feedback a user gets from a container file is the
framework's generic "invalid-file-size" in the error attribute.

[ ... ]

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 7db1dc9b94bd..6eefc3db586f 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c

[ ... ]

> @@ -4500,6 +4587,55 @@ static int adva_x1_cpld_read_status(struct ptp_ocp *bp, u32 *status)
>  	return 0;
>  }
>  
> +/* Poll the status register until the CPLD goes idle, or @max_ms elapses.
> + * The deadline is on wall time, so the I2C transactions count against it,
> + * and the status is read once more after it expires before giving up.
> + */
> +static int adva_x1_cpld_wait_ready(struct ptp_ocp *bp, unsigned int max_ms)
> +{
> +	u32 status = 0;
> +	int err, ret;
> +
> +	ret = read_poll_timeout(adva_x1_cpld_read_status, err,
> +				err || READ_ONCE(bp->cpld_cancel) ||
> +				(status & CPLD_STATUS_FAILED) ||
> +				!(status & CPLD_STATUS_BUSY),
> +				CPLD_POLL_US, max_ms * USEC_PER_MSEC, false,
> +				bp, &status);
> +	if (ret)
> +		return ret;
> +	if (READ_ONCE(bp->cpld_cancel))
> +		return -ECANCELED;
> +	if (err || (status & CPLD_STATUS_FAILED))
> +		return -EIO;
> +
> +	return 0;
> +}

[Severity: Low]
Is collapsing every transport errno to -EIO here intentional?

adva_x1_cpld_read_status() returns the underlying I2C errno (-ETIMEDOUT,
-ENXIO, -EREMOTEIO, ...), and err is part of the termination condition, so
the poll exits with that errno available.  It is then replaced:

	if (err || (status & CPLD_STATUS_FAILED))
		return -EIO;

adva_cpld_err() distinguishes those categories, but by the time it is
consulted the distinction is gone:

	case -ETIMEDOUT:
		return FW_UPLOAD_ERR_TIMEOUT;
	...
	case -EIO:		/* FAILED status, or a short transfer */
		return FW_UPLOAD_ERR_HW_ERROR;

so an I2C controller timeout during a status poll is reported as "hw-error"
instead of "timeout", and a NACK as "hw-error" instead of
"read-write-error".  The same -EIO is also used for a genuine CPLD FAILED
status, so the two are indistinguishable in the error attribute.  Would
returning err when it is non-zero, and -EIO only for CPLD_STATUS_FAILED,
keep the mapping useful?

> +
> +/* Map an errno onto the category userspace reads back from
> + * /sys/class/firmware/adva-cpld.N/error.  A step aborted by cancel() must
> + * be reported as such, not as a HW error.
> + */
> +static enum fw_upload_err adva_cpld_err(struct ptp_ocp *bp, int err)
> +{
> +	if (READ_ONCE(bp->cpld_cancel))
> +		return FW_UPLOAD_ERR_CANCELED;

[ ... ]

> @@ -4572,6 +4708,234 @@ cpld_status_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_ADMIN_RO(cpld_status);
>  

[ ... ]

> +static enum fw_upload_err
> +adva_cpld_prepare(struct fw_upload *fwl, const u8 *data, u32 size)
> +{
> +	enum fw_upload_err ret = FW_UPLOAD_ERR_NONE;
> +	struct ptp_ocp *bp = fwl->dd_handle;
> +	int err;
> +
> +	/* Do not clear cpld_cancel here: fw_upload_start() queues the work
> +	 * before this runs, so a cancel may already have arrived - honour it
> +	 * before touching the hardware.  It is cleared once the upload is
> +	 * over, on every exit below and in cleanup().
> +	 */
> +	if (READ_ONCE(bp->cpld_cancel)) {
> +		WRITE_ONCE(bp->cpld_cancel, false);
> +		return FW_UPLOAD_ERR_CANCELED;
> +	}

[Severity: Medium]
Can this consume a cpld_cancel left over from the previous upload?

adva_cpld_cleanup() clears the flag and unlocks, but the framework only
returns progress to IDLE afterwards:

drivers/base/firmware_loader/sysfs_upload.c:fw_upload_main() {
	...
done:
	if (fwlp->ops->cleanup)
		fwlp->ops->cleanup(fwl);
	...
	fw_upload_prog_complete(fwlp);
}

In that window cancel_store() still accepts a cancel, since it only rejects
the IDLE state:

	mutex_lock(&fwlp->lock);
	if (fwlp->progress == FW_UPLOAD_PROG_IDLE) {
		mutex_unlock(&fwlp->lock);
		return -ENODEV;
	}

	fwlp->ops->cancel(fwlp->fw_upload);

so adva_cpld_cancel() sets cpld_cancel = true with no upload running, and
nothing clears it until the next adva_cpld_prepare(), which aborts a fresh
and unrelated upload with "user-abort".

Relatedly, adva_cpld_err() tests the same flag before looking at err, so a
cancel racing a real transfer failure reports a hardware or I2C error as a
user abort.  Would tying cancellation to an upload generation counter rather
than a bare bool cleared from several driver-side points close both cases?

> +
> +	if (!size || size % CPLD_PAGE_SIZE) {
> +		WRITE_ONCE(bp->cpld_cancel, false);
> +		return FW_UPLOAD_ERR_INVALID_SIZE;
> +	}
> +
> +	bp->cpld_in_config_mode = false;
> +
> +	mutex_lock(&bp->cpld_lock);
> +
> +	err = adva_x1_bus_claim(bp);
> +	if (err) {
> +		ret = adva_cpld_err(bp, err);
> +		goto err_unlock;
> +	}

[ ... ]

> +	err = adva_x1_cpld_write(bp, CPLD_CMD_ERASE);
> +	if (!err)
> +		err = adva_x1_cpld_wait_ready(bp, 15000);
> +	if (err) {
> +		ret = adva_cpld_err(bp, err);
> +		goto err_deselect;
> +	}

[ ... ]

> +err_deselect:
> +	if (bp->cpld_in_config_mode) {
> +		adva_x1_cpld_write(bp, CPLD_CMD_DIS_CFG);
> +		bp->cpld_in_config_mode = false;
> +	}
> +	adva_x1_mux_select(bp, -1);
> +err_release:
> +	adva_x1_bus_release(bp);
> +err_unlock:
> +	WRITE_ONCE(bp->cpld_cancel, false);
> +	mutex_unlock(&bp->cpld_lock);
> +	return ret;
> +}

[Severity: Medium]
Can DIS_CFG reach the CPLD while it is still busy erasing?

adva_x1_cpld_wait_ready() returns as soon as the cancel flag is seen, or as
soon as a status read fails, i.e. possibly with CPLD_STATUS_BUSY still
asserted:

	ret = read_poll_timeout(adva_x1_cpld_read_status, err,
				err || READ_ONCE(bp->cpld_cancel) ||
				(status & CPLD_STATUS_FAILED) ||
				!(status & CPLD_STATUS_BUSY),

A "echo 1 > cancel" during the 15 s config-sector erase therefore lands in
err_deselect with the erase in flight.  The abort path then sends DIS_CFG,
discards its result and clears the bookkeeping unconditionally:

	if (bp->cpld_in_config_mode) {
		adva_x1_cpld_write(bp, CPLD_CMD_DIS_CFG);
		bp->cpld_in_config_mode = false;
	}

before deselecting the mux and handing the TMC segment back to the
MicroBlaze.  If the ISC_DISABLE is not latched (device busy, NACK, I2C
error) the part stays in transparent configuration mode with a partially
erased configuration flash, while the driver records that it left the mode,
so nothing will issue DIS_CFG again.  adva_cpld_cleanup() has the same shape
for a canceled page write.

Would waiting for BUSY to clear under a hard deadline before DIS_CFG, and
keeping cpld_in_config_mode set when the DIS_CFG transfer itself fails, be
safer here?

> +
> +static enum fw_upload_err
> +adva_cpld_write(struct fw_upload *fwl, const u8 *data,
> +		u32 offset, u32 size, u32 *written)
> +{

[ ... ]

> +static enum fw_upload_err
> +adva_cpld_poll_complete(struct fw_upload *fwl)
> +{
> +	struct ptp_ocp *bp = fwl->dd_handle;
> +	int err;
> +	u32 st;
> +
> +	lockdep_assert_held(&bp->cpld_lock);
> +
> +	if (READ_ONCE(bp->cpld_cancel))
> +		return FW_UPLOAD_ERR_CANCELED;
> +
> +	err = adva_x1_cpld_write(bp, CPLD_CMD_SET_DONE);
> +	if (!err)
> +		err = adva_x1_cpld_wait_ready(bp, 1000);
> +	if (err)
> +		return adva_cpld_err(bp, err);
> +
> +	err = adva_x1_cpld_read_status(bp, &st);
> +	if (err)
> +		return adva_cpld_err(bp, err);
> +	if (!(st & CPLD_STATUS_DONE))
> +		return FW_UPLOAD_ERR_HW_ERROR;
> +
> +	err = adva_x1_cpld_write(bp, CPLD_CMD_REFRESH);
> +	if (err)
> +		return adva_cpld_err(bp, err);
> +
> +	/* REFRESH reboots the CPLD out of configuration mode, so cleanup()
> +	 * must not send DIS_CFG afterwards even if the checks below fail.
> +	 */
> +	bp->cpld_in_config_mode = false;
> +
> +	/* The new image is already running at this point, so a segment that
> +	 * is not back yet must not be reported as a failed update: retry the
> +	 * reselect instead of sampling the mux once at a fixed delay.
> +	 */
> +	msleep(1500);
> +	if (read_poll_timeout(adva_x1_mux_select, err, !err, CPLD_POLL_US,
> +			      3000 * USEC_PER_MSEC, false,
> +			      bp, ADVA_MUX_CHANNEL))
> +		return FW_UPLOAD_ERR_TIMEOUT;

[Severity: Low]
Should this poll also look at bp->cpld_cancel?

The exit condition is only !err, and the failure branch returns
FW_UPLOAD_ERR_TIMEOUT without going through adva_cpld_err(), so a cancel
arriving while the mux keeps failing neither ends the loop nor produces
FW_UPLOAD_ERR_CANCELED, and the flush_work() inside
firmware_upload_unregister() waits out the full 3 s on top of the preceding
msleep(1500).  The adva_x1_cpld_wait_ready() below does honour the flag, but
only once the mux answers.

> +
> +	err = adva_x1_cpld_wait_ready(bp, 3000);
> +	if (err)
> +		return adva_cpld_err(bp, err);
> +
> +	return FW_UPLOAD_ERR_NONE;
> +}

[Severity: Medium]
Is the CPLD state validated after REFRESH before reporting success?

CPLD_STATUS_DONE is only sampled before CPLD_CMD_REFRESH.  The single
post-REFRESH check is adva_x1_cpld_wait_ready(), which accepts any status
with BUSY clear and CPLD_STATUS_FAILED clear, and discards the status word,
so neither DONE nor the MachXO3 configuration error field is examined after
the device reloads the newly programmed image.  A CRC or preamble error, or
a refresh that leaves DONE deasserted, would be reported to userspace as a
successful update.

The other in-tree MachXO drivers do check both after refresh:

drivers/fpga/machxo2-spi.c:machxo2_write_complete() {
	...
		if (!test_bit(BUSY, &status) && test_bit(DONE, &status) &&
		    get_err(&status) == ENOERR)
			break;
	...
}

Would a similar DONE plus error-field check after REFRESH be appropriate
here?

> +
> +static void
> +adva_cpld_cancel(struct fw_upload *fwl)
> +{
> +	struct ptp_ocp *bp = fwl->dd_handle;
> +
> +	WRITE_ONCE(bp->cpld_cancel, true);
> +}
> +
> +static void
> +adva_cpld_cleanup(struct fw_upload *fwl)
> +{
> +	struct ptp_ocp *bp = fwl->dd_handle;
> +
> +	__acquire(&bp->cpld_lock);	/* held since prepare() returned ok */
> +	lockdep_assert_held(&bp->cpld_lock);
> +
> +	if (bp->cpld_in_config_mode) {
> +		adva_x1_cpld_write(bp, CPLD_CMD_DIS_CFG);
> +		bp->cpld_in_config_mode = false;
> +	}
> +	adva_x1_mux_select(bp, -1);
> +	adva_x1_bus_release(bp);
> +	WRITE_ONCE(bp->cpld_cancel, false);
> +	mutex_unlock(&bp->cpld_lock);
> +}

[ ... ]

> @@ -5214,6 +5578,15 @@ ptp_ocp_detach(struct ptp_ocp *bp)
>  {
>  	int i;
>  
> +	/* Must come first: cancels and flushes an in-flight upload while the
> +	 * I2C controller is still up, and drops cpld_lock so a cpld_status
> +	 * reader cannot stall ptp_ocp_attr_group_del() below.
> +	 */
> +	if (bp->cpld_fw_upload) {
> +		firmware_upload_unregister(bp->cpld_fw_upload);
> +		bp->cpld_fw_upload = NULL;
> +	}
> +
>  	ptp_ocp_debugfs_remove_device(bp);
>  	ptp_ocp_detach_sysfs(bp);
>  	ptp_ocp_attr_group_del(bp);

[Severity: High]
This window is not introduced by this patch, but is the cancel-and-flush
guarantee the comment relies on unconditional?

firmware_upload_unregister() skips both the cancel and the flush when it
samples IDLE:

drivers/base/firmware_loader/sysfs_upload.c:firmware_upload_unregister() {
	mutex_lock(&fw_upload_priv->lock);
	if (fw_upload_priv->progress == FW_UPLOAD_PROG_IDLE) {
		mutex_unlock(&fw_upload_priv->lock);
		goto unregister;
	}
	...
	flush_work(&fw_upload_priv->work);

unregister:
	device_unregister(&fw_sysfs->dev);
	module_put(module);
}

A "echo 0 > loading" taking the same lock immediately afterwards queues the
work:

fw_upload_start() {
	...
	fwlp->progress = FW_UPLOAD_PROG_RECEIVING;
	...
	queue_work(system_long_wq, &fwlp->work);
}

and the device_unregister() above then reaches fw_dev_release() ->
fw_upload_free(), which kfree()s fw_upload_priv, the structure embedding
that work_struct, and fw_upload itself.

A queued fw_upload_main() would then run adva_cpld_prepare() with
fwl->dd_handle == bp after ptp_ocp_detach() has done
mutex_destroy(&bp->cpld_lock) and devlink_free() has freed bp.  Since
prepare() now takes cpld_lock, the i2c root-adapter lock, an
i2c_get_adapter() reference and a kzalloc'd cpld_buf, and only drops them in
cleanup(), the same window could also leave the i2c bus lock held and the
adapter reference leaked.

The commit message states this as a guarantee:

  "The upload is unregistered first on detach, which cancels and flushes an
  in-flight programming cycle while the I2C controller is still up."

Could that be qualified, or could the driver gate adva_cpld_prepare() on its
own "detaching" flag so it does not depend on the core flushing?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908122328.52641-1-maimon.sagi%40gmail.com

Re: [PATCH net-next v13 2/2] ptp: ocp: add TAP CPLD firmware upload for ADVA TimeCard X1

[Jakub Kicinski]

On Tue,  8 Sep 2026 15:23:28 +0300 Sagi Maimon wrote:
> The Lattice MachXO3 CPLD on the ADVA TimeCard X1 is programmed over I2C
> using in-system programming (ISP).  Build on the TMC bus arbitration
> added previously and expose the update path through the kernel
> firmware-upload subsystem.
> 
> The framework acquires the bus, erases the configuration flash, programs
> the image page-by-page and activates it with the MachXO3 REFRESH
> command.  The upload node is registered per card as adva-cpld.N, using
> the same index as the owning ocpN device, so a host with more than one
> X1 board gets one node each:
> 
>   /sys/class/firmware/adva-cpld.N/
> 
> The whole prepare/write/poll_complete/cleanup sequence runs under
> cpld_lock and the i2c adapter lock, so an EEPROM read blocks for as long
> as programming takes; the alternative is reading the TMC bus instead.
> The upload is unregistered first on detach, which cancels and flushes an
> in-flight programming cycle while the I2C controller is still up.
> 
> Select FW_LOADER and FW_UPLOAD, as the documented update path does not
> exist without them.

Why add another FW flashing interface when the device already supports
devlink flash? You can use component names to disambiguate which FW we
are targeting.