On 12/08/2026 15:17, Ahmad Byagowi wrote:
R4006 cards place a PCA9546 mux behind the FPGA I2C controller.
The mux exposes three LM75B temperature sensors, an SHT3x humidity
sensor, an ICP10100 pressure sensor, and an IS32FL3207 controller for
the GNSS and SMA LEDs.
Describe the per-card topology with software nodes and instantiate
standard I2C clients after their adapters appear. Keep the mux channel,
RISET value, and per-output current limit in the board profile, and
validate every profile index before constructing nodes.
Select the profile only on supported Time Card PCI devices whose
fixed-width EEPROM ID contains printable text, valid zero or 0xff
padding, and an R4006 prefix. Leave erased, malformed, and unknown IDs
unconfigured without changing the EEPROM data.
Leave the channel containing the BNO08x empty because no upstream
driver exists. Serialize topology changes with a private mutex and
stable device references. Retry transient setup failures, report
exhaustion once, and continue low-rate recovery so late adapter or
client availability can still complete setup.
Signed-off-by: Ahmad Byagowi <[email protected]>
---
drivers/ptp/ptp_ocp.c | 810 +++++++++++++++++++++++++++++++++++++++---
1 file changed, 764 insertions(+), 46 deletions(-)
Hi Ahmad,
It's partial review, because this patch mixes a lot of things. Consider
split it into multiple smaller changes to make review process a bit
easier.
[...]
@@ -414,6 +483,15 @@ struct ptp_ocp {
const struct ocp_sma_op *sma_op;
struct dpll_device *dpll;
dpll_tracker tracker;
+ const struct ptp_ocp_i2c_profile *i2c_profile;
+ struct ptp_ocp_i2c_topology *i2c_topology;
+ struct mutex i2c_topology_lock; /* Serializes topology updates. */
+ struct delayed_work i2c_work;
+ struct notifier_block i2c_notifier;
+ atomic_t i2c_retry_count;
atomic field in per-device structure? (later more on this)
+ bool i2c_root_present;
+ bool i2c_resources_ready;
+ bool i2c_notifier_registered;
int signals_nr;
int freq_in_nr;
};
@@ -444,6 +522,8 @@ static int ptp_ocp_signal_from_perout(struct ptp_ocp *bp,
int gen,
struct ptp_perout_request *req);
static int ptp_ocp_signal_enable(void *priv, u32 req, bool enable);
static int ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr);
+static int ptp_ocp_i2c_notifier_call(struct notifier_block *nb,
+ unsigned long action, void *data);
static int ptp_ocp_art_board_init(struct ptp_ocp *bp, struct ocp_resource *r);
@@ -488,6 +568,68 @@ static struct ptp_ocp_eeprom_map art_eeprom_map[] = {
{ }
};
+/* Channel 3's BNO08x at 0x4a has no upstream Linux driver, so omit it. */
comment says omit 0x4a ...
+static const struct ptp_ocp_i2c_device ptp_ocp_r4006_sensors[] = {
+ { "temperature@48", "national,lm75b", "lm75b", 0, 0x48 },
+ { "temperature@49", "national,lm75b", "lm75b", 0, 0x49 },
+ { "temperature@4a", "national,lm75b", "lm75b", 0, 0x4a },
... but you still put temperature sensor on 0x4a?
+ { "humidity@44", NULL, "sht3x", 1, 0x44 },
+ { "pressure@63", "invensense,icp10100", "icp10100", 2, 0x63 },
+};
+
[...]
-static void
+static bool
+ptp_ocp_has_eeprom_data(struct ptp_ocp *bp)
+{
+ return smp_load_acquire(&bp->has_eeprom_data);
+}
not sure it makes any sense to have one-line helper
+
+static int
ptp_ocp_read_eeprom(struct ptp_ocp *bp)
{
const struct ptp_ocp_eeprom_map *map;
struct nvmem_device *nvmem;
const void *tag;
- int ret;
-
- if (!bp->i2c_ctrl)
- return;
+ int ret = 0;
tag = NULL;
nvmem = NULL;
+ mutex_lock(&bp->eeprom_lock);
+ if (ptp_ocp_has_eeprom_data(bp))
+ goto out;
why do you need any smp_load_acquire semantic under mutex lock?
+ if (!bp->i2c_ctrl || !bp->eeprom_map) {
+ ret = -ENODEV;
+ goto out;
+ }
for (map = bp->eeprom_map; map->len; map++) {
if (map->tag != tag) {
@@ -1997,21 +2149,536 @@ ptp_ocp_read_eeprom(struct ptp_ocp *bp)
}
ret = nvmem_device_read(nvmem, map->off, map->len,
BP_MAP_ENTRY_ADDR(bp, map));
- if (ret != map->len)
+ if (ret != map->len) {
+ if (ret >= 0)
well, there is no way ret can be 0 if map->len > 0.
+ ret = -EIO;
goto fail;
+ }
}
- bp->has_eeprom_data = true;
+ /* Publish the EEPROM fields before readers observe valid data. */
+ smp_store_release(&bp->has_eeprom_data, true);
+ ret = 0;
out:
ptp_ocp_nvmem_device_put(&nvmem);
- return;
+ mutex_unlock(&bp->eeprom_lock);
+ return ret;
fail:
- dev_err(&bp->pdev->dev, "could not read eeprom: %d\n", ret);
goto out;
"fail" label just to go to "out"? remove it..
}
+static int
+ptp_ocp_i2c_adapter_match(struct device *dev, const void *data)
+{
+ return !!i2c_verify_adapter(dev);
+}
+
+static struct i2c_adapter *
+ptp_ocp_i2c_root_adapter(struct platform_device *i2c_ctrl)
+{
+ struct i2c_adapter *adapter;
+ struct device *dev;
+
+ dev = device_find_child(&i2c_ctrl->dev, NULL,
+ ptp_ocp_i2c_adapter_match);
+ if (!dev)
+ return NULL;
+
+ adapter = i2c_verify_adapter(dev);
+ if (!adapter || !try_module_get(adapter->owner)) {
device_find_child just checked that i2c_verify_adapter returns valid
pointer...
+ put_device(dev);
+ return NULL;
+ }
+
+ /* The caller owns the reference returned by device_find_child(). */
+ return adapter;
+}
+
+static bool
+ptp_ocp_i2c_supported(struct ptp_ocp *bp)
+{
+ /* PCI IDs identify FPGA images, not a unique PCB revision. */
+ return (bp->pdev->vendor == PCI_VENDOR_ID_META &&
+ bp->pdev->device == PCI_DEVICE_ID_META_TIMECARD) ||
+ (bp->pdev->vendor == PCI_VENDOR_ID_CELESTICA &&
+ bp->pdev->device == PCI_DEVICE_ID_CELESTICA_TIMECARD);
+}
+
+static bool
+ptp_ocp_board_id_valid(const u8 *board_id, size_t *text_len)
+{
+ unsigned int len;
+
+ for (len = 0; len < OCP_BOARD_ID_LEN; len++)
+ if (board_id[len] < 0x20 || board_id[len] > 0x7e)
+ break;
+
+ if (!len)
+ return false;
+
+ *text_len = len;
+ for (; len < OCP_BOARD_ID_LEN; len++)
+ if (board_id[len] != 0 && board_id[len] != 0xff)
+ return false;
+
+ return true;
+}
+
+static const struct ptp_ocp_i2c_profile *
+ptp_ocp_i2c_select_profile(struct ptp_ocp *bp)
+{
+ static const char r4006_id[] = "R4006";
+ size_t board_id_len;
+
+ if (!ptp_ocp_has_eeprom_data(bp))
+ return NULL;
+
+ if (!ptp_ocp_board_id_valid(bp->board_id, &board_id_len))
+ return NULL;
+
+ if (board_id_len >= sizeof(r4006_id) - 1 &&
+ !memcmp(bp->board_id, r4006_id, sizeof(r4006_id) - 1))
+ return &ptp_ocp_r4006_profile;
+
+ return NULL;
+}
that doesn't verify for a valid board id, but just for a alpha-numeric
crap with prefix. effectively can be replaced with a single memcmp() of
prefix string.
[...]
+static void
+ptp_ocp_i2c_kick(struct ptp_ocp *bp)
+{
+ if (!ptp_ocp_i2c_supported(bp))
+ return;
+ if (!READ_ONCE(bp->i2c_resources_ready))
+ return;
+ if (!READ_ONCE(bp->i2c_root_present))
+ return;
+
+ mod_delayed_work(system_wq, &bp->i2c_work, 1);
+}
+
+static void
+ptp_ocp_i2c_retry(struct ptp_ocp *bp, int error)
+{
+ unsigned long delay = HZ;
+ int retries;
+
+ if (!READ_ONCE(bp->i2c_resources_ready))
+ return;
+ if (!READ_ONCE(bp->i2c_root_present))
+ return;
+
+ retries = atomic_inc_return(&bp->i2c_retry_count);
+ if (retries >= OCP_I2C_RETRY_MAX) {
+ if (retries == OCP_I2C_RETRY_MAX) {
+ dev_err(&bp->pdev->dev,
+ "I2C topology failed after %d attempts: %pe; "
+ "retrying every %d seconds\n",
+ OCP_I2C_RETRY_MAX, ERR_PTR(error),
+ OCP_I2C_RECOVERY_SECS);
+ } else {
+ atomic_set(&bp->i2c_retry_count, OCP_I2C_RETRY_MAX);
+ dev_err_ratelimited(&bp->pdev->dev,
+ "I2C topology setup still failing:
%pe\n",
+ ERR_PTR(error));
+ }
+ delay = OCP_I2C_RECOVERY_SECS * HZ;
+ }
+
+ /* Preserve a faster rerun queued by an I2C bus notification. */
+ queue_delayed_work(system_wq, &bp->i2c_work, delay);
+}
+
+static int
+ptp_ocp_i2c_populate_topology(struct ptp_ocp *bp,
+ struct platform_device *i2c_ctrl)
+{
+ const struct software_node *node;
+ struct i2c_adapter *adapter;
+ unsigned int channel;
+ int err, ret = 0;
+
+ if (!READ_ONCE(bp->i2c_root_present))
+ return 0;
+ if (!ptp_ocp_i2c_supported(bp) || !bp->eeprom_map)
+ return 0;
+
+ adapter = ptp_ocp_i2c_root_adapter(i2c_ctrl);
+ if (!adapter)
+ return -EAGAIN;
+
+ if (!ptp_ocp_has_eeprom_data(bp)) {
+ ret = ptp_ocp_read_eeprom(bp);
+ if (ret)
+ goto out_put_adapter;
+ }
+ if (!ptp_ocp_has_eeprom_data(bp)) {
+ ret = -EAGAIN;
+ goto out_put_adapter;
+ }
+ if (!bp->i2c_profile)
+ bp->i2c_profile = ptp_ocp_i2c_select_profile(bp);
+ if (!bp->i2c_profile)
+ goto out_put_adapter;
+
+ ret = ptp_ocp_i2c_init_nodes(bp);
+ if (ret)
+ goto out_put_adapter;
+
+ node = &bp->i2c_topology->mux_node;
+ ret = ptp_ocp_i2c_add_device(adapter, node, OCP_I2C_MUX_TYPE,
+ OCP_I2C_MUX_ADDRESS);
+ if (ret)
+ goto out_put_adapter;
+
+ for (channel = 0; channel < OCP_I2C_MUX_CHANNELS; channel++) {
+ err = ptp_ocp_i2c_populate_channel(bp, channel);
+ if (err && !ret)
+ ret = err;
+ }
+
+out_put_adapter:
+ i2c_put_adapter(adapter);
+ return ret;
+}
+
+static void
+ptp_ocp_i2c_work(struct work_struct *work)
+{
+ struct ptp_ocp *bp = container_of(work, struct ptp_ocp, i2c_work.work);
+ struct platform_device *i2c_ctrl;
+ struct device *i2c_ctrl_dev;
+ int retries, ret = 0;
+
+ /* Pair with resource publication after registration. */
+ if (!smp_load_acquire(&bp->i2c_resources_ready))
+ return;
+ if (!ptp_ocp_i2c_supported(bp))
+ return;
+
+ mutex_lock(&bp->i2c_topology_lock);
+ if (!READ_ONCE(bp->i2c_resources_ready) ||
+ !READ_ONCE(bp->i2c_root_present)) {
+ mutex_unlock(&bp->i2c_topology_lock);
+ return;
+ }
+
+ i2c_ctrl = READ_ONCE(bp->i2c_ctrl);
+ if (!i2c_ctrl) {
+ ret = -EAGAIN;
+ goto out_unlock;
+ }
+
+ i2c_ctrl_dev = get_device(&i2c_ctrl->dev);
+ ret = ptp_ocp_i2c_populate_topology(bp, i2c_ctrl);
+ put_device(i2c_ctrl_dev);
+
+out_unlock:
+ mutex_unlock(&bp->i2c_topology_lock);
+
+ if (ret) {
+ ptp_ocp_i2c_retry(bp, ret);
+ return;
+ }
+
+ retries = atomic_xchg(&bp->i2c_retry_count, 0);
so basically there is delayes work to explore i2c bus. and it cannot run
multiple times in parallel. what else is expected to change
i2c_retry_count? why is it needed? why is it atomic?
+ if (retries >= OCP_I2C_RETRY_MAX)
+ dev_info(&bp->pdev->dev, "I2C topology setup recovered\n");
+}
