[PATCH v10] ptp: ocp: add CPLD ISP support for ADVA TimeCard X1

[PATCH v10] ptp: ocp: add CPLD ISP support for ADVA TimeCard X1

[Sagi Maimon]

The ADVA TimeCard X1 (PCI device 0x0410) uses a Lattice MachXO3 CPLD
that is programmed over I2C using in-system programming (ISP).

The CPLD is connected to a secondary I2C bus shared with the onboard
MicroBlaze soft CPU.

Add support for CPLD access and firmware updates on the ADVA TimeCard X1
board by arbitration of the shared I2C bus, CPLD ISP command handling,
status polling, and firmware upload operations using the firmware-upload
subsystem.

Add the following X1-only user-visible interfaces:

  /sys/class/timecard/ocpN/cpld_device_id
        report the 32-bit Lattice MachXO3 CPLD device ID

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

Firmware updates are performed through the firmware-upload framework,
which acquires ownership of the shared I2C bus, erases the CPLD
configuration flash, programs the image page-by-page, and activates
the new image using the MachXO3 REFRESH command.

All CPLD operations are serialized and coordinated with the MicroBlaze
firmware to ensure exclusive access to the shared I2C bus. The added
interfaces are available only on ADVA TimeCard X1 boards.

Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---

 Addressed comments from:
  - Vadim Fedorenko :https://lore.kernel.org/all/CAMuE1bEaRBJVSJYONJ2o3adOOzayv0ayNZ2qCB1zxuodpbiR=Q@mail.gmail.com/

 Changes since v9:
 - Remove duplicate header includes.
 - Fix reverse Christmas tree variable ordering.
 - Use get_unaligned_be32() for CPLD device ID decoding.
 - Make constant command argument arrays static.

 Documentation/ABI/testing/sysfs-timecard |  34 ++
 drivers/ptp/ptp_ocp.c                    | 495 ++++++++++++++++++++++-
 2 files changed, 525 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-timecard b/Documentation/ABI/testing/sysfs-timecard
index 3ae41b7634ac..f7c9955acb0a 100644
--- a/Documentation/ABI/testing/sysfs-timecard
+++ b/Documentation/ABI/testing/sysfs-timecard
@@ -11,6 +11,40 @@ Contact:	Jonathan Lemon <jonathan.lemon@gmail.com>
 Description:	This directory contains the attributes of the Nth timecard
 		registered.
 
+What:		/sys/class/timecard/ocpN/cpld_device_id
+Date:		July 2026
+Contact:	Sagi Maimon <maimon.sagi@gmail.com>
+Description:	(RO) The 32-bit Lattice device ID of the TAP CPLD, reported as
+		a hex string, e.g. "0xe12bc043".
+
+		Only present on ADVA x1 TAP boards (PCI ID 0xad5a:0x0410).
+		The Lattice LCMXO3LF-210 reports 0xe12bc043.
+
+		The driver acquires the MicroBlaze I2C bus internally before
+		issuing the READ_IDCODE command; no bus arbitration is required
+		from userspace.
+
+What:		/sys/class/timecard/ocpN/cpld_status
+Date:		July 2026
+Contact:	Sagi Maimon <maimon.sagi@gmail.com>
+Description:	(RO) 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.
+
+		The driver acquires the MicroBlaze I2C bus internally; no bus
+		arbitration is required from userspace.
+
+		To program new CPLD firmware use the standard kernel
+		firmware-upload interface registered at:
+		/sys/class/firmware/adva-cpld/
+
 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 35e911f1ad78..cb259413042b 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -24,6 +24,9 @@
 #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
 
@@ -85,6 +88,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 +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,12 @@ 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 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)
@@ -449,6 +460,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;
 
@@ -1273,6 +1286,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,
 		},
 	},
 	{ }
@@ -3197,6 +3211,20 @@ ptp_ocp_adva_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
 		return err;
 	ptp_ocp_sma_init(bp);
 
+	if (info->has_cpld) {
+		struct fw_upload *fwl;
+
+		fwl = firmware_upload_register(THIS_MODULE, &bp->pdev->dev,
+					       "adva-cpld",
+					       &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);
 }
 
@@ -4224,6 +4252,442 @@ static const struct ocp_attr_group art_timecard_groups[] = {
 	{ },
 };
 
+/*
+ * Internal helpers for the adva_x1 TAP CPLD (Lattice LCMXO3LF-210).
+ *
+ * The CPLD sits at I2C address 0x40 behind a PCA9548 mux (0x74) on
+ * channel 0.  The I2C bus is shared with the MicroBlaze firmware;
+ * cpld_lock + mblaze acquire/release provide mutual exclusion for the
+ * full duration of any CPLD operation.  No raw I2C access is exposed
+ * to userspace; only the high-level attributes below are.
+ */
+
+#define ADVA_MUX_ADDR     0x74
+#define ADVA_CPLD_ADDR    0x40
+#define ADVA_MUX_CHANNEL  0
+
+#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
+#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
+
+/* 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 CPLD bus.
+ *
+ * All buffers are heap-allocated internally to guarantee DMA safety for
+ * the Xilinx I2C controller.  Caller must hold bp->cpld_lock.
+ */
+static int adva_x1_i2c_xfer(struct ptp_ocp *bp,
+			    u8 addr, const void *wdata, u8 wlen,
+			    void *rdata, u8 rlen, bool nostart)
+{
+	u8 *wbuf = NULL, *rbuf = NULL;
+	struct i2c_adapter *adap;
+	struct i2c_msg msgs[2];
+	int nmsgs = 0, ret;
+
+	adap = i2c_get_adapter(READ_ONCE(bp->cpld_i2c_adap_nr));
+	if (!adap)
+		return -ENODEV;
+
+	if (wlen) {
+		wbuf = kmemdup(wdata, wlen, GFP_KERNEL);
+		if (!wbuf) {
+			ret = -ENOMEM;
+			goto put;
+		}
+		msgs[nmsgs++] = (struct i2c_msg){
+			.addr  = addr,
+			.flags = I2C_M_DMA_SAFE,
+			.len   = wlen,
+			.buf   = wbuf,
+		};
+	}
+	if (rlen) {
+		rbuf = kzalloc(rlen, GFP_KERNEL);
+		if (!rbuf) {
+			ret = -ENOMEM;
+			goto put;
+		}
+		msgs[nmsgs++] = (struct i2c_msg){
+			.addr  = addr,
+			.flags = I2C_M_RD | I2C_M_DMA_SAFE |
+				 (nostart ? I2C_M_NOSTART : 0),
+			.len   = rlen,
+			.buf   = rbuf,
+		};
+	}
+
+	ret = i2c_transfer(adap, msgs, nmsgs);
+	if (ret == nmsgs) {
+		if (rdata && rlen)
+			memcpy(rdata, rbuf, rlen);
+		ret = 0;
+	} else {
+		ret = (ret < 0) ? ret : -EIO;
+	}
+put:
+	kfree(wbuf);
+	kfree(rbuf);
+	i2c_put_adapter(adap);
+	return ret;
+}
+
+/* Acquire the shared I2C bus from the MicroBlaze firmware. */
+static int adva_x1_mblaze_acquire(struct ptp_ocp *bp)
+{
+	u32 val;
+	int i;
+
+	if (!bp->pps_select)
+		return -ENODEV;
+
+	/* Release any stale grant left by a previous crashed caller. */
+	iowrite32(0, &bp->pps_select->i2c_bus_ctrl);
+	val = ioread32(&bp->pps_select->i2c_bus_ctrl);
+	if (val != 0)
+		return -EBUSY;
+
+	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;
+	}
+	return -ETIMEDOUT;
+}
+
+static void adva_x1_mblaze_release(struct ptp_ocp *bp)
+{
+	if (bp->pps_select)
+		iowrite32(MBLAZE_RELEASE, &bp->pps_select->i2c_bus_ctrl);
+}
+
+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, 1, NULL, 0, false);
+}
+
+/* Send 1-byte ISC command + optional arguments. */
+static int adva_x1_cpld_write(struct ptp_ocp *bp,
+			      u8 cmd, const u8 *args, u8 nargs)
+{
+	u8 buf[1 + 64];
+
+	if (nargs > 64)
+		return -EINVAL;
+	buf[0] = cmd;
+	if (nargs)
+		memcpy(&buf[1], args, nargs);
+	return adva_x1_i2c_xfer(bp, ADVA_CPLD_ADDR, buf, 1 + nargs,
+				 NULL, 0, false);
+}
+
+/*
+ * Send a 4-byte command then read data back without an intermediate STOP
+ * (Lattice combined write→repeated-START→read).
+ */
+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, &cmd, 4, out, out_len, true);
+}
+
+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;
+}
+
+static int adva_x1_cpld_wait_ready(struct ptp_ocp *bp, unsigned int max_ms)
+{
+	unsigned int elapsed = 0;
+	u32 status;
+
+	while (elapsed < max_ms) {
+		if (adva_x1_cpld_read_status(bp, &status))
+			return -EIO;
+		if (status & CPLD_STATUS_FAILED)
+			return -EIO;
+		if (!(status & CPLD_STATUS_BUSY))
+			return 0;
+		usleep_range(100000, 101000);
+		elapsed += 100;
+	}
+	return -ETIMEDOUT;
+}
+
+/*
+ * cpld_device_id - show the Lattice device ID of the TAP CPLD.
+ *
+ * Returns the 32-bit ID as a hex string, e.g. "0x612bc043\n".
+ * Lattice LCMXO3LF-210 reports 0x612BC043.
+ */
+static ssize_t
+cpld_device_id_show(struct device *dev, struct device_attribute *attr,
+		    char *buf)
+{
+	struct ptp_ocp *bp = dev_get_drvdata(dev);
+	u8 data[4];
+	u32 id = 0;
+	int ret;
+
+	mutex_lock(&bp->cpld_lock);
+	ret = adva_x1_mblaze_acquire(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)
+		id = get_unaligned_be32(data);
+	adva_x1_mux_select(bp, -1);
+release:
+	adva_x1_mblaze_release(bp);
+out:
+	mutex_unlock(&bp->cpld_lock);
+	return ret ? ret : sysfs_emit(buf, "0x%08x\n", id);
+}
+static DEVICE_ATTR_RO(cpld_device_id);
+
+/*
+ * 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;
+
+	mutex_lock(&bp->cpld_lock);
+	ret = adva_x1_mblaze_acquire(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_mblaze_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_RO(cpld_status);
+
+/*
+ * adva_x1 CPLD firmware-upload callbacks.
+ *
+ * The kernel firmware-upload subsystem (CONFIG_FW_UPLOAD) exposes:
+ *   /sys/class/firmware/adva-cpld/{data,loading,status,error,...}
+ * 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)
+{
+	static const u8 era_args[3] = { 0x04, 0x00, 0x00 }; /* cfg sector only */
+	static const u8 en_args[2]  = { 0x08, 0x00 };
+	static const u8 dis_args[2] = { 0x00, 0x00 };
+	static const u8 zero3[3]    = { 0 };
+	enum fw_upload_err ret = FW_UPLOAD_ERR_NONE;
+	struct ptp_ocp *bp = fwl->dd_handle;
+
+	if (!size || size % CPLD_PAGE_SIZE)
+		return FW_UPLOAD_ERR_INVALID_SIZE;
+
+	bp->cpld_cancel		= false;
+	bp->cpld_in_config_mode	= false;
+
+	mutex_lock(&bp->cpld_lock);
+
+	if (adva_x1_mblaze_acquire(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;
+	}
+
+	if (adva_x1_cpld_write(bp, CPLD_CMD_EN_CFG_TP, en_args, 2) ||
+	    adva_x1_cpld_wait_ready(bp, 5000)) {
+		ret = FW_UPLOAD_ERR_HW_ERROR;
+		goto err_deselect;
+	}
+	bp->cpld_in_config_mode = true;
+
+	if (bp->cpld_cancel) {
+		ret = FW_UPLOAD_ERR_CANCELED;
+		goto err_deselect;
+	}
+
+	if (adva_x1_cpld_write(bp, CPLD_CMD_ERASE, era_args, 3) ||
+	    adva_x1_cpld_wait_ready(bp, 15000)) {
+		ret = FW_UPLOAD_ERR_HW_ERROR;
+		goto err_deselect;
+	}
+
+	if (bp->cpld_cancel) {
+		ret = FW_UPLOAD_ERR_CANCELED;
+		goto err_deselect;
+	}
+
+	if (adva_x1_cpld_write(bp, CPLD_CMD_RESET_ADDR, zero3, 3)) {
+		ret = FW_UPLOAD_ERR_HW_ERROR;
+		goto err_deselect;
+	}
+
+	return FW_UPLOAD_ERR_NONE;	/* cleanup() will unlock everything */
+
+err_deselect:
+	if (bp->cpld_in_config_mode) {
+		adva_x1_cpld_write(bp, CPLD_CMD_DIS_CFG, dis_args, 2);
+		bp->cpld_in_config_mode = false;
+	}
+	adva_x1_mux_select(bp, -1);
+err_release:
+	adva_x1_mblaze_release(bp);
+err_unlock:
+	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 page_args[3 + CPLD_PAGE_SIZE];
+
+	if (bp->cpld_cancel)
+		return FW_UPLOAD_ERR_CANCELED;
+
+	if (size < CPLD_PAGE_SIZE)
+		return FW_UPLOAD_ERR_INVALID_SIZE;
+
+	page_args[0] = 0x00;
+	page_args[1] = 0x00;
+	page_args[2] = 0x01;
+	memcpy(&page_args[3], data + offset, CPLD_PAGE_SIZE);
+
+	if (adva_x1_cpld_write(bp, CPLD_CMD_WRITE_PAGE,
+			       page_args, 3 + CPLD_PAGE_SIZE) ||
+	    adva_x1_cpld_wait_ready(bp, 100))
+		return FW_UPLOAD_ERR_HW_ERROR;
+
+	*written = CPLD_PAGE_SIZE;
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static enum fw_upload_err
+adva_cpld_poll_complete(struct fw_upload *fwl)
+{
+	static const u8 ref_args[2] = { 0x00, 0x00 };
+	static const u8 zero3[3]    = { 0 };
+	struct ptp_ocp *bp = fwl->dd_handle;
+	u32 st;
+
+	if (bp->cpld_cancel)
+		return FW_UPLOAD_ERR_CANCELED;
+
+	if (adva_x1_cpld_write(bp, CPLD_CMD_SET_DONE, zero3, 3) ||
+	    adva_x1_cpld_wait_ready(bp, 1000))
+		return FW_UPLOAD_ERR_HW_ERROR;
+
+	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, ref_args, 2))
+		return FW_UPLOAD_ERR_HW_ERROR;
+
+	/* CPLD reboots after REFRESH; re-select mux once it comes back up */
+	msleep(1500);
+	adva_x1_mux_select(bp, ADVA_MUX_CHANNEL);
+	if (adva_x1_cpld_wait_ready(bp, 3000))
+		return FW_UPLOAD_ERR_TIMEOUT;
+
+	bp->cpld_in_config_mode = false;
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static void
+adva_cpld_cancel(struct fw_upload *fwl)
+{
+	struct ptp_ocp *bp = fwl->dd_handle;
+
+	bp->cpld_cancel = true;
+}
+
+static void
+adva_cpld_cleanup(struct fw_upload *fwl)
+{
+	static const u8 dis_args[2] = { 0x00, 0x00 };
+	struct ptp_ocp *bp = fwl->dd_handle;
+
+	if (bp->cpld_in_config_mode) {
+		adva_x1_cpld_write(bp, CPLD_CMD_DIS_CFG, dis_args, 2);
+		bp->cpld_in_config_mode = false;
+	}
+	adva_x1_mux_select(bp, -1);
+	adva_x1_mblaze_release(bp);
+	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,
@@ -4272,6 +4736,8 @@ 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_device_id.attr,
+	&dev_attr_cpld_status.attr,
 	NULL,
 };
 
@@ -4902,6 +5368,11 @@ 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);
+	if (bp->cpld_fw_upload) {
+		firmware_upload_unregister(bp->cpld_fw_upload);
+		bp->cpld_fw_upload = NULL;
+	}
+	mutex_destroy(&bp->cpld_lock);
 	device_unregister(&bp->dev);
 }
 
@@ -5078,6 +5549,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.
@@ -5217,11 +5699,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 cpld_device_id/cpld_status/cpld_program
+		 * 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 v10] ptp: ocp: add CPLD ISP support for ADVA TimeCard X1

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ptp: ocp: add CPLD ISP support for ADVA TimeCard X1

This adds Lattice MachXO3 CPLD in-system-programming support for the ADVA
TimeCard X1 over the card's I2C bus, arbitrated with the on-card MicroBlaze
firmware.  Two new read-only sysfs attributes (cpld_device_id, cpld_status)
are added along with firmware-upload callbacks for reflashing the CPLD.

A few questions about the new code below.

First, on the commit message:

    The CPLD is connected to a secondary I2C bus shared with the onboard
    MicroBlaze soft CPU.

    All CPLD operations are serialized and coordinated with the MicroBlaze
    firmware to ensure exclusive access to the shared I2C bus.

[Severity: Low]
Which bus is the "secondary" one here?  ocp_adva_x1_resource[] registers
only one I2C controller (OCP_I2C_RESOURCE(i2c_ctrl), xiic-i2c at
0x00150000), and that same adapter carries the on-card EEPROMs:

	{ I2C_BOARD_INFO("24c02", 0x50) },
	{ I2C_BOARD_INFO("24mac402", 0x58), .platform_data = "mac" },

adva_x1_i2c_xfer() then uses bp->cpld_i2c_adap_nr, which the notifier fills
in from that single adapter.

If it is the same adapter, is the "exclusive access" claim complete?  The
pre-existing EEPROM traffic (serialnum_show() -> ptp_ocp_read_eeprom(), and
the nvmem read paths) does not acquire MicroBlaze ownership, and it is not
aware that the CPLD paths leave the PCA9548 with all channels deselected
via adva_x1_mux_select(bp, -1).

> diff --git a/Documentation/ABI/testing/sysfs-timecard b/Documentation/ABI/testing/sysfs-timecard
> index 3ae41b7634ac9..f7c9955acb0a9 100644
> --- a/Documentation/ABI/testing/sysfs-timecard
> +++ b/Documentation/ABI/testing/sysfs-timecard
> @@ -11,6 +11,40 @@ Contact:	Jonathan Lemon <jonathan.lemon@gmail.com>
>  Description:	This directory contains the attributes of the Nth timecard
>  		registered.
>  
> +What:		/sys/class/timecard/ocpN/cpld_device_id
> +Date:		July 2026
> +Contact:	Sagi Maimon <maimon.sagi@gmail.com>
> +Description:	(RO) The 32-bit Lattice device ID of the TAP CPLD, reported as
> +		a hex string, e.g. "0xe12bc043".
> +
> +		Only present on ADVA x1 TAP boards (PCI ID 0xad5a:0x0410).
> +		The Lattice LCMXO3LF-210 reports 0xe12bc043.

[Severity: Low]
Which of the two documented IDCODEs is the right one?  This file says the
LCMXO3LF-210 reports 0xe12bc043 (twice, including the example output),
while the comment above cpld_device_id_show() in the same patch says:

	 * Returns the 32-bit ID as a hex string, e.g. "0x612bc043\n".
	 * Lattice LCMXO3LF-210 reports 0x612BC043.

Userspace that validates the attribute against the documented constant can
only match one of them.  The leading 0xe looks like it came from the
CPLD_CMD_READ_ID opcode 0xE0000000.

[ ... ]

> +		The driver acquires the MicroBlaze I2C bus internally; no bus
> +		arbitration is required from userspace.
> +
> +		To program new CPLD firmware use the standard kernel
> +		firmware-upload interface registered at:
> +		/sys/class/firmware/adva-cpld/
> +

[Severity: Medium]
Is this path documented unconditionally on purpose?  PTP_1588_CLOCK_OCP in
drivers/ptp/Kconfig selects NET_DEVLINK, CRC16 and DPLL, but not FW_UPLOAD,
and this patch does not touch any Kconfig file.

With CONFIG_FW_UPLOAD=n, firmware_upload_register() is the inline stub in
include/linux/firmware.h that returns ERR_PTR(-EINVAL), so
ptp_ocp_adva_board_init() only prints the dev_warn() and
/sys/class/firmware/adva-cpld/ never appears, while the upload callbacks
become dead code.  Should PTP_1588_CLOCK_OCP select FW_UPLOAD?

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 35e911f1ad78c..cb259413042b8 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c

[ ... ]

> @@ -416,6 +421,12 @@ 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 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 */
>  };

[ ... ]

> @@ -3197,6 +3211,20 @@ ptp_ocp_adva_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
>  		return err;
>  	ptp_ocp_sma_init(bp);
>  
> +	if (info->has_cpld) {
> +		struct fw_upload *fwl;
> +
> +		fwl = firmware_upload_register(THIS_MODULE, &bp->pdev->dev,
> +					       "adva-cpld",
> +					       &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;
> +	}
> +

[Severity: Medium]
What happens with two X1 cards in the same host?  The name passed here is
the fixed string "adva-cpld", and firmware_upload_register() ->
fw_create_instance() does:

	dev_set_name(f_dev, "%s", fw_name);
	f_dev->parent = device;
	f_dev->class = &firmware_class;

so /sys/class/firmware/adva-cpld can only be created once and the second
device_add() should fail with -EEXIST.  Since the failure is only a
dev_warn() here, the second card loses the documented update path silently.
Every other interface of this driver is per-card (ocpN); would an
instance-qualified name be better here?

>  	return ptp_ocp_init_clock(bp, &info->servo);
>  }
>  
> @@ -4224,6 +4252,442 @@ static const struct ocp_attr_group art_timecard_groups[] = {
>  	{ },
>  };
>  

[ ... ]

> +/*
> + * adva_x1_i2c_xfer() - issue a single I2C transaction on the CPLD bus.
> + *
> + * All buffers are heap-allocated internally to guarantee DMA safety for
> + * the Xilinx I2C controller.  Caller must hold bp->cpld_lock.
> + */
> +static int adva_x1_i2c_xfer(struct ptp_ocp *bp,
> +			    u8 addr, const void *wdata, u8 wlen,
> +			    void *rdata, u8 rlen, bool nostart)
> +{

[Severity: Low]
This isn't a bug, but the "Caller must hold bp->cpld_lock" contract is not
expressed to the tools.  adva_cpld_prepare() returns with the mutex held
and adva_cpld_cleanup() unlocks a mutex it never took, and since
mutex_lock()/mutex_unlock() carry __acquires()/__releases() annotations,
sparse reports a context imbalance for both.  Would
__acquires(&bp->cpld_lock)/__releases(&bp->cpld_lock) plus
lockdep_assert_held(&bp->cpld_lock) in adva_x1_i2c_xfer(),
adva_cpld_write() and adva_cpld_poll_complete() make the contract
enforceable?

[ ... ]

> +	if (rlen) {
> +		rbuf = kzalloc(rlen, GFP_KERNEL);
> +		if (!rbuf) {
> +			ret = -ENOMEM;
> +			goto put;
> +		}
> +		msgs[nmsgs++] = (struct i2c_msg){
> +			.addr  = addr,
> +			.flags = I2C_M_RD | I2C_M_DMA_SAFE |
> +				 (nostart ? I2C_M_NOSTART : 0),
> +			.len   = rlen,
> +			.buf   = rbuf,
> +		};
> +	}

[Severity: Low]
Is I2C_M_NOSTART the flag that was intended here?  The only caller that
passes nostart=true is adva_x1_cpld_cmd_read(), whose comment says:

	 * Send a 4-byte command then read data back without an intermediate STOP
	 * (Lattice combined write→repeated-START→read).

I2C_M_NOSTART asks the adapter to skip the (re)START and to not resend the
slave address with the R/W bit, which is the opposite of a repeated-START
read, and it requires the adapter to advertise I2C_FUNC_NOSTART.  The
xiic-i2c controller this driver instantiates advertises only:

	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;

and never looks at I2C_M_NOSTART, and the core does not reject unsupported
protocol-mangling flags, so the flag is silently dropped and the transfer
becomes the plain write + repeated-START read that the ISC protocol wants.
On an adapter that does honour the flag (i2c-algo-bit, i2c-gpio, tegra,
brcmstb, pxa, bcm-kona) the read would return bus garbage as an ISC status
word, and i2c-bcm-iproc rejects it outright.  Two messages with no
NOSTART flag express the intended sequence; alternatively, should this
check i2c_check_functionality(adap, I2C_FUNC_NOSTART)?

[ ... ]

> +/* Acquire the shared I2C bus from the MicroBlaze firmware. */
> +static int adva_x1_mblaze_acquire(struct ptp_ocp *bp)
> +{
> +	u32 val;
> +	int i;
> +
> +	if (!bp->pps_select)
> +		return -ENODEV;
> +
> +	/* Release any stale grant left by a previous crashed caller. */
> +	iowrite32(0, &bp->pps_select->i2c_bus_ctrl);
> +	val = ioread32(&bp->pps_select->i2c_bus_ctrl);
> +	if (val != 0)
> +		return -EBUSY;

[Severity: Medium]
Can this -EBUSY check ever mean anything?  The register is overwritten with
0 before it is read, so the read only observes the value this function just
wrote, and a non-zero result means the MicroBlaze wrote something in
between rather than "bus busy".

Two consequences worth confirming:

The comment says the write clears a stale grant from a crashed caller, but
the code cannot distinguish that from a grant the MicroBlaze is currently
relying on, since it never inspects the register first.

Back-to-back reads of cpld_status/cpld_device_id can spuriously return
-EBUSY if the MicroBlaze has not yet consumed the MBLAZE_RELEASE written
by adva_x1_mblaze_release(), which is fire-and-forget with no
acknowledgement.

bp->cpld_lock only serialises the host side, so what provides exclusion
against the MicroBlaze in this window?

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

[Severity: Medium]
Should the pending MBLAZE_REQUEST be withdrawn on this timeout path?  The
request stays in i2c_bus_ctrl when -ETIMEDOUT is returned, and none of the
three callers releases it:

cpld_device_id_show() and cpld_status_show():

	ret = adva_x1_mblaze_acquire(bp);
	if (ret)
		goto out;		/* skips the release: label */

adva_cpld_prepare():

	if (adva_x1_mblaze_acquire(bp)) {
		ret = FW_UPLOAD_ERR_TIMEOUT;
		goto err_unlock;	/* skips err_release */
	}

So adva_x1_mblaze_release() is never called and the MicroBlaze still sees a
host request after the host gave up.  If it later grants it, it believes the
host owns the segment while nothing on the host side will ever send
MBLAZE_RELEASE.  The only recovery is the unconditional iowrite32(0) at the
top of some later acquire, which writes 0 rather than MBLAZE_RELEASE and
depends on another CPLD operation being requested at all.

[ ... ]

> +static int adva_x1_cpld_wait_ready(struct ptp_ocp *bp, unsigned int max_ms)
> +{
> +	unsigned int elapsed = 0;
> +	u32 status;
> +
> +	while (elapsed < max_ms) {
> +		if (adva_x1_cpld_read_status(bp, &status))
> +			return -EIO;
> +		if (status & CPLD_STATUS_FAILED)
> +			return -EIO;
> +		if (!(status & CPLD_STATUS_BUSY))
> +			return 0;
> +		usleep_range(100000, 101000);
> +		elapsed += 100;
> +	}
> +	return -ETIMEDOUT;
> +}

[Severity: High]
Does this loop poll one time too few?  The sleep and the elapsed update are
the last statements of the iteration, so the status is never re-read after
the final sleep and the loop performs floor(max_ms / 100) reads.

With the 100 ms budget used per page in adva_cpld_write() that means
exactly one read:

	elapsed = 0
	adva_x1_cpld_read_status()	-> BUSY set
	usleep_range(100000, 101000)
	elapsed = 100
	while (elapsed < 100)		-> false
	return -ETIMEDOUT;

adva_cpld_write() turns that into FW_UPLOAD_ERR_HW_ERROR and
fw_upload_main() aborts the upload, but adva_cpld_prepare() has already run
CPLD_CMD_ERASE, so the configuration flash is left erased and only
partially programmed with DONE clear.  A page program that legitimately
asserts BUSY for tPROG at that single poll would fail on the very first
page.

The same off-by-one shortens the 5000/15000/1000/3000 ms budgets by one
poll, and elapsed does not account for the time spent in the I2C status
transaction itself.  Should the status be re-checked after the last sleep?

[Jakub] Please use standard iopoll.h helpers or gate the timeout on a
jiffies-based deadline, not just loop count. And check before return
like the AI suggests..

> +
> +/*
> + * cpld_device_id - show the Lattice device ID of the TAP CPLD.
> + *
> + * Returns the 32-bit ID as a hex string, e.g. "0x612bc043\n".
> + * Lattice LCMXO3LF-210 reports 0x612BC043.
> + */
> +static ssize_t
> +cpld_device_id_show(struct device *dev, struct device_attribute *attr,
> +		    char *buf)
> +{
> +	struct ptp_ocp *bp = dev_get_drvdata(dev);
> +	u8 data[4];
> +	u32 id = 0;
> +	int ret;
> +
> +	mutex_lock(&bp->cpld_lock);

[Severity: Medium]
Can this block uninterruptibly for the whole duration of an upload?
bp->cpld_lock is taken in adva_cpld_prepare() and only dropped in
adva_cpld_cleanup(), covering the 15000 ms erase wait, one page write plus
wait per 16 bytes of image, SET_DONE (1000 ms), msleep(1500) and the
3000 ms post-REFRESH wait.

Both show() functions use plain mutex_lock(), so a "cat
/sys/class/timecard/ocpN/cpld_status" sits in TASK_UNINTERRUPTIBLE and is
unkillable for that whole time, which can also trip the hung-task detector.
kernfs drains in-flight attribute readers, so that stuck reader also stalls
ptp_ocp_attr_group_del() and device removal.  Would
mutex_lock_interruptible(), or a trylock returning -EBUSY while an upload
is in progress, be preferable here?

[Jakub] you seem to be doing multiple things in one patch, please make this
a patch series and separate the FW flashing form the rest. 

[ ... ]

> +static DEVICE_ATTR_RO(cpld_device_id);

[Severity: Medium]
Should these two attributes be world readable?  DEVICE_ATTR_RO() gives mode
0444 for both cpld_device_id and cpld_status, and they are added
unconditionally to adva_timecard_x1_attrs[].  A read is not a passive
register access: it takes bp->cpld_lock, overwrites the shared
host/MicroBlaze arbitration register with iowrite32(0, ...) before checking
ownership, sleeps up to MBLAZE_RETRIES * ~10 ms (about 2.2 s) inside
adva_x1_mblaze_acquire() while holding the mutex, and reprograms the shared
PCA9548 (select channel 0, then deselect all channels).

So any local user can loop on cpld_status and repeatedly clobber the
arbitration state the card firmware relies on, change mux state other I2C
clients on the same adapter depend on, and block uninterruptibly for the
length of a root-initiated upload.  Would 0400/0440 be more appropriate?

[Jakub] why is this not reported in devlink info? If it's the SKU/device id
of the CPLD it should be a "fixed" version?
I'll stop reviewing now, rest of the AI comments sent without checking...

[ ... ]

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

[ ... ]

> +	if (!size || size % CPLD_PAGE_SIZE)
> +		return FW_UPLOAD_ERR_INVALID_SIZE;
> +
> +	bp->cpld_cancel		= false;
> +	bp->cpld_in_config_mode	= false;
> +
> +	mutex_lock(&bp->cpld_lock);

[Severity: Medium]
Can this reset discard a cancel request that has already arrived?
fw_upload_start() sets progress = FW_UPLOAD_PROG_RECEIVING and queues the
work before dropping fwlp->lock, and cancel_store() /
firmware_upload_unregister() only refuse to call ops->cancel() when
progress == FW_UPLOAD_PROG_IDLE:

	cancel_store()/firmware_upload_unregister()
	    adva_cpld_cancel()
	        bp->cpld_cancel = true;

	worker
	    adva_cpld_prepare()
	        bp->cpld_cancel = false;	/* request lost */

In the unregister path that matters: firmware_upload_unregister() calls
cancel() and then flush_work(), so with the request lost, unbind/PCI remove
waits for the full erase (up to 15 s) plus every 16-byte page program.

Two related points: bp->cpld_cancel is a plain bool shared between the
worker and the cancelling task with no READ_ONCE/WRITE_ONCE, atomic or
lock, and it is written here outside bp->cpld_lock; and it is never checked
inside adva_x1_cpld_wait_ready(), so a cancel arriving during the 15 s
erase or a page wait is not observed at all.

[ ... ]

> +	if (adva_x1_cpld_write(bp, CPLD_CMD_EN_CFG_TP, en_args, 2) ||
> +	    adva_x1_cpld_wait_ready(bp, 5000)) {
> +		ret = FW_UPLOAD_ERR_HW_ERROR;
> +		goto err_deselect;
> +	}
> +	bp->cpld_in_config_mode = true;

[Severity: Medium]
Is the flag set too late here?  The short-circuit means
adva_x1_cpld_wait_ready() only runs when the EN_CFG_TP write already
succeeded, so on a wait failure (BUSY timeout, FAILED bit, or an I2C error
in the status read) the CPLD has accepted enable-configuration while
bp->cpld_in_config_mode is still false.

err_deselect is guarded by that flag:

	err_deselect:
		if (bp->cpld_in_config_mode) {
			adva_x1_cpld_write(bp, CPLD_CMD_DIS_CFG, dis_args, 2);

so CPLD_CMD_DIS_CFG is never sent, the mux is deselected, the bus is handed
back and the CPLD stays in ISC transparent configuration mode.
fw_upload_main() does not call cleanup() when prepare() fails, so there is
no second chance to send DIS_CFG, and the next upload attempt issues
EN_CFG_TP to an already-enabled device.

[ ... ]

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

[ ... ]

> +	if (adva_x1_cpld_write(bp, CPLD_CMD_WRITE_PAGE,
> +			       page_args, 3 + CPLD_PAGE_SIZE) ||
> +	    adva_x1_cpld_wait_ready(bp, 100))
> +		return FW_UPLOAD_ERR_HW_ERROR;
> +
> +	*written = CPLD_PAGE_SIZE;
> +	return FW_UPLOAD_ERR_NONE;
> +}
> +
> +static enum fw_upload_err
> +adva_cpld_poll_complete(struct fw_upload *fwl)
> +{

[ ... ]

> +	if (adva_x1_cpld_write(bp, CPLD_CMD_REFRESH, ref_args, 2))
> +		return FW_UPLOAD_ERR_HW_ERROR;
> +
> +	/* CPLD reboots after REFRESH; re-select mux once it comes back up */
> +	msleep(1500);
> +	adva_x1_mux_select(bp, ADVA_MUX_CHANNEL);
> +	if (adva_x1_cpld_wait_ready(bp, 3000))
> +		return FW_UPLOAD_ERR_TIMEOUT;
> +
> +	bp->cpld_in_config_mode = false;
> +	return FW_UPLOAD_ERR_NONE;
> +}

[Severity: Medium]
Can this report a failure for an update that actually succeeded?  The
adva_x1_mux_select() return value is discarded, so if the segment is not
responsive at exactly the 1500 ms mark the mux write is NAKed silently and
the following READ_STATUS goes to a bus with no channel enabled:

	adva_x1_cpld_wait_ready(bp, 3000)
	    adva_x1_cpld_read_status()	-> I2C error
	    return -EIO
	return FW_UPLOAD_ERR_TIMEOUT;

At that point DONE was already verified set and REFRESH was accepted, so
the new image is running, yet userspace sees a failed update and is likely
to erase and reprogram again.  Would a bounded retry of the post-reset mux
reselect help here?

Also, bp->cpld_in_config_mode is still true on this return path, so
adva_cpld_cleanup() then sends DIS_CFG to a device that already left
configuration mode.

[ ... ]

> @@ -4902,6 +5368,11 @@ 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);
> +	if (bp->cpld_fw_upload) {
> +		firmware_upload_unregister(bp->cpld_fw_upload);
> +		bp->cpld_fw_upload = NULL;
> +	}
> +	mutex_destroy(&bp->cpld_lock);
>  	device_unregister(&bp->dev);
>  }

[Severity: Low]
This isn't a use-after-free: platform_device_unregister(bp->i2c_ctrl) ->
xiic_i2c_remove() -> i2c_del_adapter() does

	init_completion(&adap->dev_released);
	device_unregister(&adap->dev);
	wait_for_completion(&adap->dev_released);

so the adapter cannot go away while adva_x1_i2c_xfer() holds the
i2c_get_adapter() reference, and later lookups just return NULL so the
helper returns -ENODEV.

Would it still be cleaner to move firmware_upload_unregister() above
platform_device_unregister(bp->i2c_ctrl) and pci_free_irq_vectors()?  As
written, an in-flight upload aborts mid-programming with -ENODEV instead of
being cancelled and flushed first, and i2c_del_adapter() blocks for the
duration of whatever transfer is in flight.

>  
> @@ -5217,11 +5699,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 cpld_device_id/cpld_status/cpld_program
> +		 * for reference-counted unbind-safe adapter access.
> +		 */

[Severity: Low]
What is cpld_program?  adva_timecard_x1_attrs[] only gains
&dev_attr_cpld_device_id.attr and &dev_attr_cpld_status.attr, and
programming goes through the fw_upload_ops callbacks, so no such attribute
or function exists in the tree.  Looks like a leftover from an earlier
revision.

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