[PATCH v5 1/1] ptp: ocp: add CPLD ISP support for ADVA TimeCard X1

[PATCH v5 1/1] 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 taking ownership of this bus and
exposing the required interfaces through sysfs, allowing userspace tools
to perform CPLD programming.

To limit the scope of this functionality, sysfs-based I2C access is
restricted to the ADVA TimeCard X1 variant and only for the two I2C
slave addresses used during ISP (0x40 CPLD, 0x74 mux).

Add two sysfs attributes under /sys/class/timecard/ocpN/ (x1 only):

i2c_bus_ctrl  - arbitrate the shared I2C bus from the MicroBlaze via
                a three-step read/write/poll handshake

cpld_i2c_xfer - binary passthrough for I2C transactions to the CPLD
                and its PCA9548 mux; one atomic request per write()

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

 Addressed comments from:
  - Simon Horman : https://lore.kernel.org/all/20260715111559.1920391-1-horms@kernel.org/
 
 Changes since v4:
  - Move mutex_init(&bp->tap_i2c_lock) and bp->tap_i2c_adap_nr = -1 to
  ptp_ocp_probe() before ptp_ocp_register_resources(). On adva_x1, the
  I2C resource is registered before board_init runs; if xiic-i2c probes
  synchronously the notifier writes tap_i2c_adap_nr, which board_init
  then clobbers with -1, breaking cpld_i2c_xfer permanently.

  - Drop the if (tap_i2c_adap_nr != 0) guard in ptp_ocp_detach(). The
  notifier has no variant filter so non-x1 boards can leave adap_nr != 0,
  triggering mutex_destroy on an uninitialised mutex. Also, nr=0 is a
  valid adapter number. Since mutex_init is now unconditional, so is
  mutex_destroy.

  - Fix bounce-buffer leak in ptp_ocp_cpld_i2c_write(). Assigning
  msg->buf = i2c_get_dma_safe_msg_buf() makes buf == msg->buf, so
  i2c_put_dma_safe_msg_buf() returns without kfree() on every call.
  Replace with kzalloc()/kfree() and set I2C_M_DMA_SAFE. 
 
 drivers/ptp/ptp_ocp.c | 236 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 231 insertions(+), 5 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 35e911f1ad78..91fa06f0e44c 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -163,7 +163,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 +417,11 @@ struct ptp_ocp {
 	dpll_tracker tracker;
 	int signals_nr;
 	int freq_in_nr;
+	/* cpld_i2c_xfer sysfs (adva_x1) */
+	struct mutex		tap_i2c_lock;
+	int			tap_i2c_adap_nr; /* adapter nr; -1 if absent */
+	u8			tap_i2c_rsp[21]; /* [status, read_data...] */
+	size_t			tap_i2c_rsp_len;
 };
 
 #define OCP_REQ_TIMESTAMP	BIT(0)
@@ -4224,6 +4230,205 @@ static const struct ocp_attr_group art_timecard_groups[] = {
 	{ },
 };
 
+/*
+ * i2c_bus_ctrl exposes the MicroBlaze I2C bus arbitration register.
+ *
+ * The shared bus requires a three-step handshake before use:
+ *   read 0x00000000 (free) -> write 0x0000ffff (request) ->
+ *   poll until 0xffffffff (MicroBlaze confirms release).
+ *
+ * The poll is a PCIe non-posted read, which also flushes the preceding
+ * posted write to the FPGA, so no separate kernel read-back is needed.
+ */
+static ssize_t
+i2c_bus_ctrl_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct ptp_ocp *bp = dev_get_drvdata(dev);
+
+	if (!bp->pps_select)
+		return -ENODEV;
+	return sysfs_emit(buf, "0x%08x\n",
+			  ioread32(&bp->pps_select->i2c_bus_ctrl));
+}
+
+static ssize_t
+i2c_bus_ctrl_store(struct device *dev, struct device_attribute *attr,
+		   const char *buf, size_t count)
+{
+	struct ptp_ocp *bp = dev_get_drvdata(dev);
+	u32 val;
+
+	if (!bp->pps_select)
+		return -ENODEV;
+	if (kstrtou32(buf, 0, &val))
+		return -EINVAL;
+	iowrite32(val, &bp->pps_select->i2c_bus_ctrl);
+	return count;
+}
+
+static DEVICE_ATTR_RW(i2c_bus_ctrl);
+
+/*
+ * cpld_i2c_xfer - sysfs binary I2C passthrough for adva_x1 TAP CPLD.
+ *
+ * write: [addr][write_len][read_len][flags][write_data...]
+ *   flags bit 0: I2C_M_NOSTART on the read segment
+ * read:  [status][read_data...]
+ *   status 0 = success, else positive errno
+ *
+ * Only addresses 0x40 (CPLD) and 0x74 (mux) are permitted.
+ */
+#define TAP_I2C_ALLOWED_ADDRS_NUM  2
+static const u8 tap_i2c_allowed_addrs[TAP_I2C_ALLOWED_ADDRS_NUM] = {
+	0x40, /* CPLD */
+	0x74, /* mux  */
+};
+
+/*
+ * tap_i2c_errno_to_byte - encode a kernel errno as a one-byte status.
+ *
+ * Errnos > 255 (e.g. ENOTSUPP=524) or multiples of 256 would truncate
+ * to a wrong or zero value with a plain (u8) cast.  Map those to EIO.
+ */
+static u8 tap_i2c_errno_to_byte(int err)
+{
+	int val = (err < 0) ? -err : EIO;
+
+	return (val > 0 && val <= 0xFF) ? (u8)val : EIO;
+}
+
+#define TAP_I2C_REQ_HDR_LEN   4
+#define TAP_I2C_MAX_WRITE_LEN 67
+#define TAP_I2C_MAX_READ_LEN  20
+#define TAP_I2C_FLAG_NOSTART  BIT(0)
+
+static ssize_t
+ptp_ocp_cpld_i2c_write(struct file *file, struct kobject *kobj,
+		       const struct bin_attribute *attr,
+		       char *buf, loff_t off, size_t count)
+{
+	struct ptp_ocp *bp = dev_get_drvdata(kobj_to_dev(kobj));
+	const u8 *req = (const u8 *)buf;
+	u8 addr, write_len, read_len, flags;
+	struct i2c_adapter *adap;
+	struct i2c_msg msgs[2];
+	u8 *rdbuf = NULL;
+	int nmsgs, ret, i;
+
+	/* Each write is one atomic request; non-zero offset means a
+	 * mid-buffer pwrite() which would misparse the header.
+	 */
+	if (off != 0)
+		return -EINVAL;
+	if (count < TAP_I2C_REQ_HDR_LEN || count > TAP_I2C_REQ_HDR_LEN + TAP_I2C_MAX_WRITE_LEN)
+		return -EINVAL;
+
+	addr      = req[0];
+	write_len = req[1];
+	read_len  = req[2];
+	flags     = req[3];
+
+	/* Validate */
+	for (i = 0; i < TAP_I2C_ALLOWED_ADDRS_NUM; i++)
+		if (addr == tap_i2c_allowed_addrs[i])
+			break;
+	if (i == TAP_I2C_ALLOWED_ADDRS_NUM)
+		return -EPERM;
+
+	if (write_len > TAP_I2C_MAX_WRITE_LEN)
+		return -EINVAL;
+	if (read_len > TAP_I2C_MAX_READ_LEN)
+		return -EINVAL;
+	if (write_len + TAP_I2C_REQ_HDR_LEN > count)
+		return -EINVAL;
+	if (write_len == 0 && read_len == 0)
+		return -EINVAL;
+	/* I2C_M_NOSTART suppresses the repeated START between write and read
+	 * segments; it has no meaning on a first-and-only message.
+	 */
+	if ((flags & TAP_I2C_FLAG_NOSTART) && write_len == 0)
+		return -EINVAL;
+
+	/* i2c_get_adapter() takes a reference under core_lock; safe against
+	 * concurrent adapter unbind.
+	 */
+	adap = i2c_get_adapter(READ_ONCE(bp->tap_i2c_adap_nr));
+	if (!adap)
+		return -ENODEV;
+
+	nmsgs = 0;
+	if (write_len > 0) {
+		msgs[nmsgs].addr  = addr;
+		msgs[nmsgs].flags = 0;
+		msgs[nmsgs].len   = write_len;
+		msgs[nmsgs].buf   = (u8 *)req + TAP_I2C_REQ_HDR_LEN;
+		nmsgs++;
+	}
+	if (read_len > 0) {
+		u16 rd_flags = I2C_M_RD;
+
+		if (flags & TAP_I2C_FLAG_NOSTART)
+			rd_flags |= I2C_M_NOSTART;
+		msgs[nmsgs].addr  = addr;
+		msgs[nmsgs].flags = rd_flags | I2C_M_DMA_SAFE;
+		msgs[nmsgs].len   = read_len;
+		rdbuf = kzalloc(read_len, GFP_KERNEL);
+		if (!rdbuf) {
+			i2c_put_adapter(adap);
+			return -ENOMEM;
+		}
+		msgs[nmsgs].buf = rdbuf;
+		nmsgs++;
+	}
+
+	/* Serialise transfer+publish so concurrent writers cannot overwrite
+	 * each other's response in tap_i2c_rsp.
+	 */
+	mutex_lock(&bp->tap_i2c_lock);
+	ret = i2c_transfer(adap, msgs, nmsgs);
+	if (ret == nmsgs) {
+		bp->tap_i2c_rsp[0] = 0;
+		if (read_len > 0)
+			memcpy(&bp->tap_i2c_rsp[1], rdbuf, read_len);
+		bp->tap_i2c_rsp_len = 1 + read_len;
+		ret = count;
+	} else {
+		bp->tap_i2c_rsp[0]  = tap_i2c_errno_to_byte(ret);
+		bp->tap_i2c_rsp_len = 1;
+		ret = (ret < 0) ? ret : -EIO;
+	}
+	mutex_unlock(&bp->tap_i2c_lock);
+	kfree(rdbuf);
+	i2c_put_adapter(adap);
+
+	return ret;
+}
+
+static ssize_t
+ptp_ocp_cpld_i2c_read(struct file *file, struct kobject *kobj,
+		      const struct bin_attribute *attr,
+		      char *buf, loff_t off, size_t count)
+{
+	struct ptp_ocp *bp = dev_get_drvdata(kobj_to_dev(kobj));
+	ssize_t ret;
+
+	mutex_lock(&bp->tap_i2c_lock);
+	if (off >= bp->tap_i2c_rsp_len) {
+		ret = 0;
+	} else {
+		ret = min(count, bp->tap_i2c_rsp_len - (size_t)off);
+		memcpy(buf, bp->tap_i2c_rsp + off, ret);
+	}
+	mutex_unlock(&bp->tap_i2c_lock);
+	return ret;
+}
+
+static const struct bin_attribute tap_i2c_bin_attr = {
+	.attr  = { .name = "cpld_i2c_xfer", .mode = 0600 },
+	.write = ptp_ocp_cpld_i2c_write,
+	.read  = ptp_ocp_cpld_i2c_read,
+};
+
 static struct attribute *adva_timecard_attrs[] = {
 	&dev_attr_serialnum.attr,
 	&dev_attr_gnss_sync.attr,
@@ -4272,11 +4477,18 @@ 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_i2c_bus_ctrl.attr,
+	NULL,
+};
+
+static const struct bin_attribute *const bin_adva_x1_timecard_attrs[] = {
+	&tap_i2c_bin_attr,
 	NULL,
 };
 
 static const struct attribute_group adva_timecard_x1_group = {
-	.attrs = adva_timecard_x1_attrs,
+	.attrs     = adva_timecard_x1_attrs,
+	.bin_attrs = bin_adva_x1_timecard_attrs,
 };
 
 static const struct ocp_attr_group adva_timecard_x1_groups[] = {
@@ -4902,6 +5114,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->tap_i2c_lock);
 	device_unregister(&bp->dev);
 }
 
@@ -5093,6 +5306,14 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	bp->n_irqs = err;
 	pci_set_master(pdev);
 
+	/* Initialise before ptp_ocp_register_resources() so that the I2C bus
+	 * notifier (ptp_ocp_i2c_notifier_call) cannot fire and write
+	 * tap_i2c_adap_nr before ptp_ocp_adva_board_init() overwrites it
+	 * with -1, leaving the adapter number lost for the device lifetime.
+	 */
+	mutex_init(&bp->tap_i2c_lock);
+	bp->tap_i2c_adap_nr = -1;
+
 	err = ptp_ocp_register_resources(bp, id->driver_data);
 	if (err)
 		goto out;
@@ -5217,11 +5438,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 number; cpld_i2c_xfer uses i2c_get_adapter()
+		 * for a reference-counted, unbind-safe lookup.
+		 */
+		WRITE_ONCE(bp->tap_i2c_adap_nr, i2c_verify_adapter(child)->nr);
+	} else {
+		WRITE_ONCE(bp->tap_i2c_adap_nr, -1);	/* invalidate before free */
 		sysfs_remove_link(&bp->dev.kobj, "i2c");
-
+	}
 	return 0;
 }
 
-- 
2.47.0

Re: [PATCH v5 1/1] 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 patch adds sysfs interfaces (i2c_bus_ctrl and cpld_i2c_xfer) on the
ADVA TimeCard X1 variant so userspace can program the Lattice MachXO3
CPLD over a shared I2C bus.  A new mutex, adapter-number cache and
response buffer are added to struct ptp_ocp for the passthrough path.

A few questions inline.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 35e911f1ad78..91fa06f0e44c 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c

[ ... ]

> @@ -4224,6 +4230,205 @@ static const struct ocp_attr_group art_timecard_groups[] = {
>  	{ },
>  };
>  
> +/*
> + * i2c_bus_ctrl exposes the MicroBlaze I2C bus arbitration register.

[Low]
Should Documentation/ABI/testing/sysfs-timecard grow entries for the two
new attributes added here (i2c_bus_ctrl and cpld_i2c_xfer)?  The other
timecard sysfs files are described there, and cpld_i2c_xfer in
particular defines a private binary protocol (header layout, allowed
slave addresses, response-buffer semantics) that userspace cannot
discover from the filesystem alone.

[ ... ]

> @@ -4902,6 +5114,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->tap_i2c_lock);
>  	device_unregister(&bp->dev);
>  }

[Medium]
Is mutex_destroy() here always called on an initialised mutex?

Looking at ptp_ocp_probe() below, mutex_init(&bp->tap_i2c_lock) runs
only after pci_alloc_irq_vectors() succeeds:

	err = pci_alloc_irq_vectors(pdev, 1, 17, PCI_IRQ_MSI | PCI_IRQ_MSIX);
	if (err < 0) {
		dev_err(&pdev->dev, "alloc_irq_vectors err: %d\n", err);
		goto out;
	}
	bp->n_irqs = err;
	pci_set_master(pdev);

	mutex_init(&bp->tap_i2c_lock);
	bp->tap_i2c_adap_nr = -1;

If pci_alloc_irq_vectors() fails, the "goto out" reaches
ptp_ocp_detach(), which now unconditionally calls
mutex_destroy(&bp->tap_i2c_lock) on a bp that was zero-initialised by
devlink_alloc().

With CONFIG_DEBUG_MUTEXES=y, mutex_destroy() checks lock->magic and
would splat via DEBUG_LOCKS_WARN_ON(lock->magic != lock) on that error
path.

Would it be simpler to move the mutex_init() (and the tap_i2c_adap_nr
= -1 assignment) earlier, e.g. right after ptp_ocp_device_init()
succeeds, so every path that reaches ptp_ocp_detach() sees an
initialised mutex?

> @@ -5093,6 +5306,14 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	bp->n_irqs = err;
>  	pci_set_master(pdev);
>  
> +	/* Initialise before ptp_ocp_register_resources() so that the I2C bus
> +	 * notifier (ptp_ocp_i2c_notifier_call) cannot fire and write
> +	 * tap_i2c_adap_nr before ptp_ocp_adva_board_init() overwrites it
> +	 * with -1, leaving the adapter number lost for the device lifetime.
> +	 */
> +	mutex_init(&bp->tap_i2c_lock);
> +	bp->tap_i2c_adap_nr = -1;
> +
>  	err = ptp_ocp_register_resources(bp, id->driver_data);
>  	if (err)
>  		goto out;

[Low]
Does this comment describe what the code actually does?

The comment says the ordering matters because
ptp_ocp_adva_board_init() would otherwise overwrite tap_i2c_adap_nr
with -1.  Looking at ptp_ocp_adva_board_init(), it only touches
flash_start, eeprom_map, sma_op, signals_nr, freq_in_nr, fw_tag,
fw_version, fw_cap and calls a couple of sub-init routines.  The -1
assignment is done here in ptp_ocp_probe() directly, not in
ptp_ocp_adva_board_init().

Should the comment be reworded to refer to the actual initialiser?

[ ... ]
-- 
pw-bot: cr