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

[PATCH net-next v12 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.

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

Changes since v11 - all from Vadim's review of the ISC command path:

 - adva_x1_i2c_xfer() takes the opcode as its own argument and assembles
   the message in a scratch buffer taken once per bus claim, so the
   opcode and its arguments are copied exactly once and a firmware upload
   costs one allocation instead of one per 16-byte page.  The bound check
   lives here now, against the scratch size.  Patch 1.
 - adva_x1_cpld_write() takes just the opcode; the per-command argument
   bytes come from adva_x1_cpld_args(), a switch-case helper returning a
   pointer to the static data.  This drops the seven per-caller argument
   arrays, including the dis_args duplicated between prepare() and
   cleanup().  Patch 2.
 - Commands whose arguments are all zero (RESET_ADDR, SET_DONE, DIS_CFG,
   REFRESH) pass NULL and the transfer sends zeros, so the zero3 arrays
   are gone.
 - Comment fix: the LCMXO3LF-2100 IDCODE is 0xe12bc043 (0x612bc043 is the
   4300-LUT part).  Patch 1.

No functional change: the byte sequences on the wire are unchanged, and
CPLD programming was verified on an ADVA TimeCard X1.

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 +
 drivers/ptp/Kconfig                      |   2 +
 drivers/ptp/ptp_ocp.c                    | 693 ++++++++++++++++++++++-
 3 files changed, 715 insertions(+), 4 deletions(-)


base-commit: c29b37ed7a4d9856ed758a82282456d69cee2ed1
-- 
2.47.0

[PATCH net-next v12 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 ++
 drivers/ptp/ptp_ocp.c                    | 362 ++++++++++++++++++++++-
 2 files changed, 378 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-timecard b/Documentation/ABI/testing/sysfs-timecard
index 3ae41b7634ac..b384e837c5e6 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 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/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 3d26ec1f7b9e..0c5c3ec8de5f 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,7 @@ struct ptp_ocp_adva_info {
 	u8				signals_nr;
 	u8				freq_in_nr;
 	const struct ocp_attr_group	*attr_groups;
+	bool				has_cpld; /* x1: supports CPLD firmware upload */
 };
 
 #define OCP_CTRL_ENABLE		BIT(0)
@@ -163,7 +166,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 +420,13 @@ struct ptp_ocp {
 	dpll_tracker tracker;
 	int signals_nr;
 	int freq_in_nr;
+	/* adva_x1 CPLD I2C (internal use only) */
+	struct mutex		cpld_lock;            /* serialises CPLD operations */
+	int			cpld_i2c_adap_nr;     /* I2C adapter nr; -1 if absent */
+	struct i2c_adapter	*cpld_adap;           /* claimed adapter; valid under cpld_lock */
+	u8			*cpld_buf;            /* DMA-safe scratch; valid under cpld_lock */
+	u32			cpld_id;              /* cached Lattice device ID; 0 if unread */
+	bool			has_cpld;             /* x1 TAP CPLD present */
 };
 
 #define OCP_REQ_TIMESTAMP	BIT(0)
@@ -451,6 +462,7 @@ static int ptp_ocp_adva_board_init(struct ptp_ocp *bp, struct ocp_resource *r);
 
 static const struct ocp_sma_op ocp_adva_sma_op;
 static const struct ocp_sma_op ocp_adva_x1_sma_op;
+static int adva_x1_cpld_device_id(struct ptp_ocp *bp, u32 *id);
 
 static const struct ocp_attr_group fb_timecard_groups[];
 
@@ -1273,6 +1285,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,
 		},
 	},
 	{ }
@@ -2185,6 +2198,19 @@ ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
 	if (err)
 		return err;
 
+	if (bp->has_cpld) {
+		u32 id;
+
+		err = adva_x1_cpld_device_id(bp, &id);
+		if (err)
+			return err;
+
+		sprintf(buf, "0x%08x", id);
+		err = devlink_info_version_fixed_put(req, "cpld.id", buf);
+		if (err)
+			return err;
+	}
+
 	return 0;
 }
 
@@ -3199,6 +3225,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 +4253,315 @@ 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(), which the Xilinx controller needs for DMA safety:
+ * an opcode and its arguments are copied exactly once.
+ *
+ * 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 = I2C_M_DMA_SAFE,
+			.len   = hdr + wlen,
+			.buf   = wbuf,
+		};
+	}
+	if (rlen) {
+		msgs[nmsgs++] = (struct i2c_msg){
+			.addr  = addr,
+			.flags = I2C_M_RD | I2C_M_DMA_SAFE,
+			.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.  A firmware upload holds it across the whole prepare/write/poll
+ * sequence, so an EEPROM read blocks for as long as programming takes.
+ */
+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;
+
+	/* 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 of the TAP CPLD.  It is a fixed property of
+ * the part, so cache it and pay the bus arbitration only once.  The
+ * LCMXO3LF-2100 IDCODE is 0xe12bc043.
+ */
+static int adva_x1_cpld_device_id(struct ptp_ocp *bp, u32 *id)
+{
+	u8 data[4];
+	int ret;
+
+	if (bp->cpld_id) {
+		*id = bp->cpld_id;
+		return 0;
+	}
+
+	/* 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_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)
+		*id = bp->cpld_id;
+
+	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 +4610,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 +5241,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);
 }
 
@@ -5080,6 +5418,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.
@@ -5219,11 +5568,16 @@ 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 adapter nr; used by the CPLD status/id/upload paths
+		 * for reference-counted unbind-safe adapter access.
+		 */
+		WRITE_ONCE(bp->cpld_i2c_adap_nr, i2c_verify_adapter(child)->nr);
+	} else {
+		WRITE_ONCE(bp->cpld_i2c_adap_nr, -1); /* invalidate before free */
 		sysfs_remove_link(&bp->dev.kobj, "i2c");
-
+	}
 	return 0;
 }
 
-- 
2.47.0

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

[netdev-bot+sashiko]

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

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

- [High] The CPLD bus-arbitration/mux-reprogramming operation that the
  patch deliberately restricts to root via…
- [High] `ptp_ocp_devlink_info_get()` propagates any failure of the new
  `adva_x1_cpld_device_id()` read out of the `->info_get()` callback…
- [Medium] `adva_x1_i2c_xfer()` marks both messages `I2C_M_DMA_SAFE`
  (`.flags = I2C_M_DMA_SAFE` for the write and `I2C_M_RD |…
- [Medium] The new `cpld.id` reporting block is appended at the end of
  `ptp_ocp_devlink_info_get()`, i.e. after the pre-existing `if…
- [Medium] `adva_x1_bus_claim()` resolves the I2C adapter from a cached
  adapter *number* (`i2c_get_adapter(READ_ONCE(bp->cpld_i2c_adap_nr))`)…
- [Medium] `ptp_ocp_detach()` now calls `mutex_destroy(&bp->cpld_lock)`,
  but on the probe error paths detach runs while the devlink instance is…
- [Medium] Acquiring the shared I2C segment is a polled firmware
  handshake (`iowrite32(MBLAZE_REQUEST)` then up to 200 x 10 ms polling…
- [Low] Several new comments in this patch document a CPLD firmware-
  upload path that does not exist in the tree at this commit: the struct…
- [Low] The patch introduces the driver-specific devlink version name
  `cpld.id` without the driver-specific devlink documentation that…

--

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

This adds a MicroBlaze request/grant handshake that re-routes the host I2C
controller of the ADVA TimeCard X1 from the config EEPROMs onto the TMC bus,
plus read-only accessors for the Lattice CPLD behind the PCA9548 there.  The
CPLD device ID is exported as the fixed devlink version cpld.id and the status
register as a new root-only cpld_status sysfs attribute.

A few questions below.

> diff --git a/Documentation/ABI/testing/sysfs-timecard b/Documentation/ABI/testing/sysfs-timecard
> index 3ae41b7634ac9..b384e837c5e64 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 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/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 3d26ec1f7b9ec..0c5c3ec8de5fa 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c

[ ... ]

> @@ -85,6 +87,7 @@ struct ptp_ocp_adva_info {
>  	u8				signals_nr;
>  	u8				freq_in_nr;
>  	const struct ocp_attr_group	*attr_groups;
> +	bool				has_cpld; /* x1: supports CPLD firmware upload */
>  };

[Severity: Low]

This isn't a bug, but the comment describes a firmware upload path that does
not exist yet at this commit.  Here has_cpld only gates the devlink cpld.id
version and the cpld_status attribute.

Two other new comments have the same forward reference.  In
adva_x1_bus_claim():

	 * EEPROMs.  A firmware upload holds it across the whole prepare/write/poll
	 * sequence, so an EEPROM read blocks for as long as programming takes.

and in ptp_ocp_i2c_notifier_call():

		/* Cache adapter nr; used by the CPLD status/id/upload paths

The follow-up patch in the series that adds firmware_upload_register() and
the prepare/write/poll_complete ops makes all three accurate, so this is only
about patch 1 reading on its own.  At this commit the longest hold of the
adapter lock is the ~2s MBLAZE_RETRIES timeout, not a programming cycle.

[ ... ]

> @@ -416,6 +420,13 @@ struct ptp_ocp {
>  	dpll_tracker tracker;
>  	int signals_nr;
>  	int freq_in_nr;
> +	/* adva_x1 CPLD I2C (internal use only) */
> +	struct mutex		cpld_lock;            /* serialises CPLD operations */
> +	int			cpld_i2c_adap_nr;     /* I2C adapter nr; -1 if absent */
> +	struct i2c_adapter	*cpld_adap;           /* claimed adapter; valid under cpld_lock */
> +	u8			*cpld_buf;            /* DMA-safe scratch; valid under cpld_lock */
> +	u32			cpld_id;              /* cached Lattice device ID; 0 if unread */
> +	bool			has_cpld;             /* x1 TAP CPLD present */
>  };

[ ... ]

> @@ -2185,6 +2198,19 @@ ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
>  	if (err)
>  		return err;
>  
> +	if (bp->has_cpld) {
> +		u32 id;
> +
> +		err = adva_x1_cpld_device_id(bp, &id);

[Severity: High]

The same hardware operation is root-only through sysfs:

	static DEVICE_ATTR_ADMIN_RO(cpld_status);

and the new ABI text says "A read arbitrates for the shared I2C bus and
reprograms the on-card mux, so it is restricted to root."

Is that restriction bypassed here?  DEVLINK_CMD_INFO_GET is registered in
net/devlink/netlink_gen.c with only .flags = GENL_CMD_CAP_DO and
GENL_CMD_CAP_DUMP, without GENL_ADMIN_PERM, so any user can walk:

devlink dev info
  devlink_nl_info_get_doit()
    ptp_ocp_devlink_info_get()
      adva_x1_cpld_device_id()
        adva_x1_bus_claim()
          i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER)
          adva_x1_mblaze_acquire()

adva_x1_mblaze_acquire() then sleeps in a 200 x usleep_range(10000, 11000)
loop while the adapter lock is held and the controller is routed away from the
EEPROMs.

Since bp->cpld_id is assigned only on a successful read, a card where the
handshake does not complete repeats the whole ~2s arbitration on every call:

	if (bp->cpld_id) {
		*id = bp->cpld_id;
		return 0;
	}

Can an unprivileged loop over devlink dev info keep the EEPROM, nvmem and at24
paths off the controller indefinitely this way, and should the devlink path
either use a value cached at probe time or be gated the way cpld_status is?

> +		if (err)
> +			return err;
> +

[Severity: High]

Should a failure of this optional version string abort the entire info reply?
devlink discards the message when ->info_get() returns non-zero, so the fw
version, serial number and board id that were already put above are lost.

For the dump form, devlink_nl_info_get_dump_one() tolerates exactly one errno:

	if (err == -EOPNOTSUPP)
		err = 0;
	return err;

so -ENODEV, -ENOMEM, -ETIMEDOUT or -ERESTARTSYS coming out of the new path
aborts devlink dev info for every devlink instance on the host, not just this
card.

-ENODEV is reachable in an ordinary configuration rather than a race:
bp->cpld_i2c_adap_nr keeps the -1 sentinel written in ptp_ocp_probe() whenever
no i2c adapter device ever appears (xiic-i2c not built or not bound, or the
i2c resource skipped), and adva_x1_bus_claim() starts with

	adap = i2c_get_adapter(READ_ONCE(bp->cpld_i2c_adap_nr));
	if (!adap)
		return -ENODEV;

Would skipping the cpld.id put on error be better here, matching how the
function already degrades for missing EEPROM data?

> +		sprintf(buf, "0x%08x", id);
> +		err = devlink_info_version_fixed_put(req, "cpld.id", buf);
> +		if (err)
> +			return err;
> +	}
> +
>  	return 0;
>  }

[Severity: Medium]

This block is appended after the pre-existing early return earlier in the same
function:

	if (!bp->has_eeprom_data) {
		ptp_ocp_read_eeprom(bp);
		if (!bp->has_eeprom_data)
			return 0;
	}

The CPLD id is read over the TMC I2C bus and has no dependency on bp->serial
or bp->board_id, yet ptp_ocp_read_eeprom() takes its fail: path when
ptp_ocp_nvmem_device_get() returns an error pointer (at24/nvmem provider not
bound) or nvmem_device_read() returns a short count, leaving has_eeprom_data
false.

On such a card devlink dev info succeeds but never reports cpld.id, while the
new ABI entry states:

		The Lattice device
		ID of the CPLD is reported as the fixed "cpld.id" version by
		devlink dev info.

Should the cpld.id put happen before the EEPROM early return?

[Severity: Low]

cpld.id is a driver-specific devlink version name.
Documentation/networking/devlink/devlink-info.rst says that driver-specific
names are a last resort and "should be documented in the driver-specific
file", but there is no ptp_ocp file under Documentation/networking/devlink/
and no entry for one in its index.  The only description of cpld.id is the
prose inside the sysfs-timecard entry for cpld_status.  Could a devlink
driver documentation file be added for it?

[ ... ]

> @@ -4226,6 +4253,315 @@ static const struct ocp_attr_group art_timecard_groups[] = {
>  	{ },
>  };
>  
> +/*
> + * Internal helpers for the adva_x1 TAP CPLD (Lattice LCMXO3LF-2100).

[ ... ]

> +/*
> + * 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(), which the Xilinx controller needs for DMA safety:
> + * an opcode and its arguments are copied exactly once.
> + *
> + * 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)
> +{

[ ... ]

> +	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 = I2C_M_DMA_SAFE,
> +			.len   = hdr + wlen,
> +			.buf   = wbuf,
> +		};
> +	}
> +	if (rlen) {
> +		msgs[nmsgs++] = (struct i2c_msg){
> +			.addr  = addr,
> +			.flags = I2C_M_RD | I2C_M_DMA_SAFE,
> +			.len   = rlen,
> +			.buf   = rbuf,
> +		};
> +	}

[Severity: Medium]

Do these two buffers really satisfy the I2C_M_DMA_SAFE contract?  Both are
halves of a single 64-byte allocation made in adva_x1_bus_claim():

	bp->cpld_buf = kzalloc(2 * ADVA_CPLD_XFER_MAX, GFP_KERNEL);

with rbuf = bp->cpld_buf + 32.  Setting I2C_M_DMA_SAFE tells the core the
buffer can be handed straight to the adapter's DMA mapping:

drivers/i2c/i2c-core-base.c:i2c_get_dma_safe_msg_buf() {
	if (msg->flags & I2C_M_DMA_SAFE)
		return msg->buf;
	...
}

so no bounce buffer is allocated and the interior pointer is used as is.
kmalloc only guarantees alignment for the start of the allocation, so
cpld_buf + 32 is merely 32-byte aligned and shares a cacheline with the write
half on any architecture with a 64-byte or larger cacheline.  That puts the
DMA_TO_DEVICE and DMA_FROM_DEVICE buffers of the same transfer in one line.

The justification in the comment above also does not seem to hold:
drivers/i2c/busses/i2c-xiic.c has no dma_map_single(), no
i2c_get_dma_safe_msg_buf() and no dmaengine use at all, so nothing behind this
card needs the flag.  Would dropping I2C_M_DMA_SAFE (letting the core bounce
if a future adapter does DMA), or using two separate allocations, be the
better option?

[ ... ]

> +static void adva_x1_mblaze_release(struct ptp_ocp *bp)
> +{
> +	if (bp->pps_select)
> +		iowrite32(MBLAZE_RELEASE, &bp->pps_select->i2c_bus_ctrl);
> +}

[Severity: Medium]

Acquiring the segment is a polled handshake, but releasing it is a single
fire-and-forget write with no wait for the MicroBlaze to route controller
0x00150000 back to the EEPROM segment.  adva_x1_bus_release() unlocks the
adapter immediately after:

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

Can the first EEPROM/nvmem/at24 transfer that was queued behind the CPLD
operation be issued while routing is still on the TMC bus?  That is the window
the commit message says the adapter lock closes:

    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.

Depending on hardware latency the result would be either a NAK surfacing as
"could not read eeprom", or a reply from an unrelated TMC device that gets
cached as serial number / board id.

There is a related question about the register values.  MBLAZE_RELEASE
(0x55550000) leaves the 0x5555 half that MBLAZE_GRANTED (0x5555aaaa) encodes
as the grant, and the next acquire writes 0 over that firmware-owned field
and discards the read-back:

	iowrite32(0, &bp->pps_select->i2c_bus_ctrl);
	ioread32(&bp->pps_select->i2c_bus_ctrl);

If that half is not host-writable, can the exact-match test val ==
MBLAZE_GRANTED be satisfied on the first poll by a stale grant?  Would polling
with a bounded timeout until the grant pattern is gone, before unlocking the
adapter, be more robust?

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

[ ... ]

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

[ ... ]

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

[Severity: Medium]

Is the adapter identity guaranteed here?  The number is snapshotted first and
the reference is taken afterwards inside i2c_get_adapter(), which blocks on
the i2c core_lock that i2c_del_adapter() and i2c_add_adapter() hold while
releasing and allocating adapter numbers.  i2c_del_adapter() frees the idr
slot before device_del() fires the notifier that resets the cached number:

drivers/i2c/i2c-core-base.c:i2c_del_adapter() {
	found = idr_find(&i2c_adapter_idr, adap->nr);
	if (found == adap)
		idr_replace(&i2c_adapter_idr, NULL, adap->nr);
	...
}

and only later does the driver see:

		WRITE_ONCE(bp->cpld_i2c_adap_nr, -1); /* invalidate before free */

So if the card's xiic-i2c adapter is unbound after the snapshot and the number
is recycled by an unrelated adapter, can this claim return that foreign
adapter?  adva_x1_mux_select() and adva_x1_cpld_cmd_read() would then write a
PCA9548 channel select to 0x74 and Lattice ISC opcodes to 0x40 on somebody
else's bus while holding its root adapter lock.

Nothing re-validates ownership (for example adap->dev.parent ==
&bp->i2c_ctrl->dev), so the notifier comment describing this as
"reference-counted unbind-safe adapter access" does not seem to match.  Would
storing a refcounted adapter pointer from the notifier, or checking the
parent after i2c_get_adapter(), close this?

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

[ ... ]

> +static DEVICE_ATTR_ADMIN_RO(cpld_status);
> +
>  static struct attribute *adva_timecard_attrs[] = {

[ ... ]

> @@ -4904,6 +5241,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);
>  }

[Severity: Medium]

Is cpld_lock still reachable when it is destroyed here?  On the probe error
paths ptp_ocp_detach() runs while the devlink instance is still registered:
ptp_ocp_probe() calls devlink_register(devlink) and the later failures of
dpll_device_get(), dpll_device_register() and dpll_pin_register() fall through
to

out:
	ptp_ocp_detach(bp);
out_disable:
	pci_disable_device(pdev);
out_free:
	devlink_free(devlink);

with no devlink_unregister() in between.  The remove path does the right
ordering, calling devlink_unregister() before ptp_ocp_detach().

Since this patch makes ptp_ocp_devlink_info_get() take cpld_lock and then
touch bp->pps_select MMIO and claim the i2c adapter, and
DEVLINK_CMD_INFO_GET needs no privilege, can a concurrent devlink dev info in
that window lock a mutex that mutex_destroy() has already reset, and drive a
device whose i2c controller and IRQ vectors detach has released?

The comment added at the mutex_init() site reasons only about destroying an
initialised mutex:

	/* Must be before the first error path that calls ptp_ocp_detach(),
	 * so mutex_destroy() always runs on an initialised mutex.

Would unregistering devlink on the probe error path before detach, or moving
mutex_destroy() after all users are drained, be the fix?

> @@ -5219,11 +5568,16 @@ 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 adapter nr; used by the CPLD status/id/upload paths
> +		 * for reference-counted unbind-safe adapter access.
> +		 */
> +		WRITE_ONCE(bp->cpld_i2c_adap_nr, i2c_verify_adapter(child)->nr);
> +	} else {
> +		WRITE_ONCE(bp->cpld_i2c_adap_nr, -1); /* invalidate before free */
>  		sysfs_remove_link(&bp->dev.kobj, "i2c");
> -
> +	}
>  	return 0;
>  }

Thanks.

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

[PATCH net-next v12 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                    | 335 ++++++++++++++++++++++-
 3 files changed, 339 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-timecard b/Documentation/ABI/testing/sysfs-timecard
index b384e837c5e6..41eeadd46330 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 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 0c5c3ec8de5f..86d1c1a311cc 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
 
@@ -427,6 +429,9 @@ struct ptp_ocp {
 	u8			*cpld_buf;            /* DMA-safe scratch; valid under cpld_lock */
 	u32			cpld_id;              /* cached Lattice device ID; 0 if unread */
 	bool			has_cpld;             /* x1 TAP CPLD present */
+	struct fw_upload	*cpld_fw_upload;      /* firmware upload handle; NULL if absent */
+	bool			cpld_cancel;          /* cancellation requested */
+	bool			cpld_in_config_mode;  /* EN_CFG_TP issued but not yet REFRESH'd */
 };
 
 #define OCP_REQ_TIMESTAMP	BIT(0)
@@ -460,6 +465,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;
 static int adva_x1_cpld_device_id(struct ptp_ocp *bp, u32 *id);
@@ -3226,6 +3233,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);
 }
 
@@ -4287,6 +4317,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)
@@ -4301,7 +4340,8 @@ static const struct ocp_attr_group art_timecard_groups[] = {
  *
  * The message is assembled in the scratch buffer taken by
  * adva_x1_bus_claim(), which the Xilinx controller needs for DMA safety:
- * an opcode and its arguments are copied exactly once.
+ * an opcode and its arguments are copied exactly once, and a firmware
+ * upload costs one allocation rather than one per page.
  *
  * Caller must hold that claim, hence __i2c_transfer() over i2c_transfer().
  */
@@ -4432,7 +4472,10 @@ static int adva_x1_bus_claim(struct ptp_ocp *bp)
 	if (!adap)
 		return -ENODEV;
 
-	/* One scratch buffer per claim, not per transfer. */
+	/* One scratch buffer per claim rather than per transfer: a firmware
+	 * upload holds the claim for the whole image, so this is a single
+	 * allocation instead of one for each 16-byte page.
+	 */
 	bp->cpld_buf = kzalloc(2 * ADVA_CPLD_XFER_MAX, GFP_KERNEL);
 	if (!bp->cpld_buf) {
 		i2c_put_adapter(adap);
@@ -4460,6 +4503,47 @@ 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 };
+	static const u8 erase_cfg[] = { 0x04, 0x00, 0x00 }; /* cfg sector only */
+
+	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
@@ -4485,6 +4569,38 @@ 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;
+}
+
+/* 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)
+{
+	return READ_ONCE(bp->cpld_cancel) ? FW_UPLOAD_ERR_CANCELED
+					  : FW_UPLOAD_ERR_HW_ERROR;
+}
+
 /*
  * Read the Lattice device ID of the TAP CPLD.  It is a fixed property of
  * the part, so cache it and pay the bus arbitration only once.  The
@@ -4562,6 +4678,212 @@ 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 at the start of 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;
+
+	/* Do not clear cpld_cancel here: fw_upload_start() queues the work
+	 * before this runs, so a cancel may already have arrived.  It is
+	 * cleared once the upload is over, on every exit below and in
+	 * cleanup().
+	 */
+	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);
+
+	if (adva_x1_bus_claim(bp)) {
+		ret = FW_UPLOAD_ERR_TIMEOUT;
+		goto err_unlock;
+	}
+
+	if (adva_x1_mux_select(bp, ADVA_MUX_CHANNEL)) {
+		ret = FW_UPLOAD_ERR_HW_ERROR;
+		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;
+
+	if (adva_x1_cpld_write(bp, CPLD_CMD_EN_CFG_TP) ||
+	    adva_x1_cpld_wait_ready(bp, 5000)) {
+		ret = adva_cpld_err(bp);
+		goto err_deselect;
+	}
+
+	if (READ_ONCE(bp->cpld_cancel)) {
+		ret = FW_UPLOAD_ERR_CANCELED;
+		goto err_deselect;
+	}
+
+	if (adva_x1_cpld_write(bp, CPLD_CMD_ERASE) ||
+	    adva_x1_cpld_wait_ready(bp, 15000)) {
+		ret = adva_cpld_err(bp);
+		goto err_deselect;
+	}
+
+	if (READ_ONCE(bp->cpld_cancel)) {
+		ret = FW_UPLOAD_ERR_CANCELED;
+		goto err_deselect;
+	}
+
+	if (adva_x1_cpld_write(bp, CPLD_CMD_RESET_ADDR)) {
+		ret = FW_UPLOAD_ERR_HW_ERROR;
+		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 };
+
+	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);
+
+	if (adva_x1_i2c_xfer(bp, ADVA_CPLD_ADDR, CPLD_CMD_WRITE_PAGE,
+			     args, sizeof(args), NULL, 0) ||
+	    adva_x1_cpld_wait_ready(bp, 100))
+		return adva_cpld_err(bp);
+
+	*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;
+
+	if (adva_x1_cpld_write(bp, CPLD_CMD_SET_DONE) ||
+	    adva_x1_cpld_wait_ready(bp, 1000))
+		return adva_cpld_err(bp);
+
+	if (adva_x1_cpld_read_status(bp, &st) || !(st & CPLD_STATUS_DONE))
+		return FW_UPLOAD_ERR_HW_ERROR;
+
+	if (adva_x1_cpld_write(bp, CPLD_CMD_REFRESH))
+		return FW_UPLOAD_ERR_HW_ERROR;
+
+	/* 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;
+
+	if (adva_x1_cpld_wait_ready(bp, 3000))
+		return READ_ONCE(bp->cpld_cancel) ? FW_UPLOAD_ERR_CANCELED
+						  : FW_UPLOAD_ERR_TIMEOUT;
+
+	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,
@@ -5204,6 +5526,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 v12 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 4 potential
issue(s) to consider:

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

- [Medium] The upload callbacks emit firmware-upload error categories
  that contradict the meanings documented for enum fw_upload_err in…
- [Medium] bp->cpld_cancel is cleared by the driver in
  adva_cpld_cleanup() (ptp_ocp.c:4875) and on adva_cpld_prepare()'s…
- [Medium] Programming success is declared without validating the full
  MachXO3 sysCONFIG status: the driver defines only DONE/BUSY/FAILED…
- [Low] The new callback block comment states 'cancel() - set flag;
  checked at the start of each callback', but adva_cpld_prepare() does…

Pre-existing issues:
- [Low] adva_x1_bus_claim() makes a single 2 * ADVA_CPLD_XFER_MAX
  (64-byte) kzalloc and adva_x1_i2c_xfer() splits it into wbuf at offset…

--

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

This adds an in-system programming path for the Lattice MachXO3 TAP CPLD on
the ADVA TimeCard X1, driven through the kernel firmware-upload subsystem
and registered per card as adva-cpld.N.  The prepare/write/poll_complete/
cleanup sequence runs under cpld_lock while holding the i2c adapter lock.

A few questions about the new upload callbacks below.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 0c5c3ec8de5fa..86d1c1a311cc5 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c

[ ... ]

> @@ -4287,6 +4317,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)

[ ... ]

> @@ -4432,7 +4472,10 @@ static int adva_x1_bus_claim(struct ptp_ocp *bp)
>  	if (!adap)
>  		return -ENODEV;
>  
> -	/* One scratch buffer per claim, not per transfer. */
> +	/* One scratch buffer per claim rather than per transfer: a firmware
> +	 * upload holds the claim for the whole image, so this is a single
> +	 * allocation instead of one for each 16-byte page.
> +	 */
>  	bp->cpld_buf = kzalloc(2 * ADVA_CPLD_XFER_MAX, GFP_KERNEL);
>  	if (!bp->cpld_buf) {
>  		i2c_put_adapter(adap);

[Severity: Low]
This isn't a bug introduced by this patch, but this patch makes the single
scratch allocation part of the documented rationale, so it seems worth
raising.  adva_x1_i2c_xfer() splits this one 64-byte allocation into two
halves:

    wbuf = bp->cpld_buf;
    rbuf = bp->cpld_buf + ADVA_CPLD_XFER_MAX;

and then marks both messages I2C_M_DMA_SAFE in the same __i2c_transfer().
Is the second half really DMA-safe in the sense that flag asserts?  With a
64-byte cacheline both halves share one line, so for the combined
write -> repeated-START -> read used by adva_x1_cpld_cmd_read() the
TO_DEVICE mapping of wbuf and the FROM_DEVICE mapping of rbuf would cover
the same line on a non-coherent master, and a writeback of the dirty
transmit half could clobber the status bytes the programming state machine
polls.

No adapter that can bind here honours the flag today (the xiic-i2c
controller instantiated by ocp_adva_x1_resource does not use DMA), so this
looks inert on this hardware.  Would separate allocations, or aligning each
half to ARCH_DMA_MINALIGN, be preferable given the flag's stated contract?

> @@ -4485,6 +4569,38 @@ 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;
> +}
> +
> +/* 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)
> +{
> +	return READ_ONCE(bp->cpld_cancel) ? FW_UPLOAD_ERR_CANCELED
> +					  : FW_UPLOAD_ERR_HW_ERROR;
> +}
> +

[Severity: Medium]
Are the firmware-upload error categories chosen here the ones userspace
expects?  The values map straight onto the strings in fw_upload_err_str[]
in drivers/base/firmware_loader/sysfs_upload.c, which are what shows up in
/sys/class/firmware/adva-cpld.N/error.

adva_cpld_err() reports any transport failure as FW_UPLOAD_ERR_HW_ERROR, so
a failed __i2c_transfer() in adva_x1_i2c_xfer() never surfaces as
FW_UPLOAD_ERR_RW_ERROR, which include/linux/firmware.h documents as "read or
write to HW failed".

In adva_cpld_prepare(), every adva_x1_bus_claim() failure becomes a timeout:

    if (adva_x1_bus_claim(bp)) {
            ret = FW_UPLOAD_ERR_TIMEOUT;

but adva_x1_bus_claim() also returns -ENODEV immediately when there is no
adapter (cpld_i2c_adap_nr is -1), and -ENOMEM when the scratch kzalloc
fails.  Should those be reported as "timeout on handshake with HW"?

And in the opposite direction, the last check in adva_cpld_poll_complete():

    if (adva_x1_cpld_wait_ready(bp, 3000))
            return READ_ONCE(bp->cpld_cancel) ? FW_UPLOAD_ERR_CANCELED
                                              : FW_UPLOAD_ERR_TIMEOUT;

reports a timeout even when wait_ready() returned -EIO because
CPLD_STATUS_FAILED was asserted or the status read itself failed.

> @@ -4562,6 +4678,212 @@ 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 at the start of 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;
> +
> +	/* Do not clear cpld_cancel here: fw_upload_start() queues the work
> +	 * before this runs, so a cancel may already have arrived.  It is
> +	 * cleared once the upload is over, on every exit below and in
> +	 * cleanup().
> +	 */
> +	if (!size || size % CPLD_PAGE_SIZE) {
> +		WRITE_ONCE(bp->cpld_cancel, false);
> +		return FW_UPLOAD_ERR_INVALID_SIZE;
> +	}

[Severity: Low]
The block comment above says cancel() sets a flag "checked at the start of
each callback", but does adva_cpld_prepare() actually check it at entry?

For an invalid size the pending cancellation is cleared here and the result
is reported as invalid-file-size rather than user-abort.

For a valid image, the bus is claimed, the mux is switched and
CPLD_CMD_EN_CFG_TP is issued before the first effective test of
bp->cpld_cancel inside adva_x1_cpld_wait_ready(), so an already-cancelled
upload still cycles the CPLD into configuration mode and back out via
DIS_CFG.  Would an explicit check before the size validation match the
comment?

> +
> +	bp->cpld_in_config_mode = false;
> +
> +	mutex_lock(&bp->cpld_lock);
> +
> +	if (adva_x1_bus_claim(bp)) {
> +		ret = FW_UPLOAD_ERR_TIMEOUT;
> +		goto err_unlock;
> +	}
> +

[ ... ]

> +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_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;
> +
> +	if (adva_x1_cpld_write(bp, CPLD_CMD_SET_DONE) ||
> +	    adva_x1_cpld_wait_ready(bp, 1000))
> +		return adva_cpld_err(bp);
> +
> +	if (adva_x1_cpld_read_status(bp, &st) || !(st & CPLD_STATUS_DONE))
> +		return FW_UPLOAD_ERR_HW_ERROR;
> +
> +	if (adva_x1_cpld_write(bp, CPLD_CMD_REFRESH))
> +		return FW_UPLOAD_ERR_HW_ERROR;

[Severity: Medium]
Is the post-REFRESH state checked strictly enough to call the update
successful?

DONE is sampled only before REFRESH.  After REFRESH the only remaining
check is adva_x1_cpld_wait_ready(bp, 3000), which returns 0 as soon as a
status read succeeds with BUSY and FAILED clear, so an image that reboots
with DONE clear is still reported as FW_UPLOAD_ERR_NONE.

The status bit definitions added here also cover only DONE, BUSY and
FAILED.  drivers/fpga/lattice-sysconfig.h defines a three-bit error field
for the same family:

    #define SYSCONFIG_STATUS_ERR            GENMASK(25, 23)

and lattice-sysconfig.c requires that field to be zero, DONE set and BUSY
clear before declaring programming complete.  Should this driver re-read
status after REFRESH and apply the same three conditions, so a nonzero
command or CRC error code with FAILED clear is not treated as success?

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

[ ... ]

> +	if (adva_x1_cpld_wait_ready(bp, 3000))
> +		return READ_ONCE(bp->cpld_cancel) ? FW_UPLOAD_ERR_CANCELED
> +						  : FW_UPLOAD_ERR_TIMEOUT;
> +
> +	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);
> +}

[Severity: Medium]
Can bp->cpld_cancel survive past the upload that owns it and abort the next
one?

cleanup() clears the flag, but the core still reports the operation as
active at that point.  In fw_upload_main() (sysfs_upload.c):

    done:
            if (fwlp->ops->cleanup)
                    fwlp->ops->cleanup(fwl);
            ...
            fw_upload_prog_complete(fwlp);

progress only becomes FW_UPLOAD_PROG_IDLE in fw_upload_prog_complete(),
while cancel_store() calls ops->cancel() for any non-IDLE progress.  So:

    worker                              other task
    adva_cpld_cleanup()
      WRITE_ONCE(cpld_cancel, false)
      mutex_unlock(&bp->cpld_lock)
                                        echo 1 > cancel
                                        progress != IDLE
                                        adva_cpld_cancel()
                                          WRITE_ONCE(cpld_cancel, true)
    fw_upload_prog_complete()

Nothing clears it again, because adva_cpld_prepare() deliberately does not
reset the flag on entry.  The next upload then hits it on the first
iteration of adva_x1_cpld_wait_ready():

    err || READ_ONCE(bp->cpld_cancel) || ...

which returns -ECANCELED, and prepare() reports user-abort for an upload
nobody cancelled, after EN_CFG_TP has already put the CPLD into
configuration mode.

The err_unlock path in adva_cpld_prepare() clears the flag with the same
timing.  Since the flag's lifetime is managed entirely outside fwlp->lock,
the only lock that orders cancel delivery, READ_ONCE/WRITE_ONCE cannot
close this.  Would tying the state to an upload generation, or resetting it
under the same serialization, work better here?

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

[ ... ]

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