Re: [PATCH v10 1/8] i2c: add I2C Address Translator (ATR) support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 17/03/2023 11:16, Luca Ceresoli wrote:
Hi Tomi, Wolfram,

On Wed, 22 Feb 2023 15:29:00 +0200
Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> wrote:

From: Luca Ceresoli <luca@xxxxxxxxxxxxxxxx>

An ATR is a device that looks similar to an i2c-mux: it has an I2C
slave "upstream" port and N master "downstream" ports, and forwards
transactions from upstream to the appropriate downstream port. But it
is different in that the forwarded transaction has a different slave
address. The address used on the upstream bus is called the "alias"
and is (potentially) different from the physical slave address of the
downstream chip.

Add a helper file (just like i2c-mux.c for a mux or switch) to allow
implementing ATR features in a device driver. The helper takes care or
adapter creation/destruction and translates addresses at each transaction.

Signed-off-by: Luca Ceresoli <luca@xxxxxxxxxxxxxxxx>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>

Wolfram, I think Tomi improved this work as much as currently possible
and this patch now looks extremely good to me. I wish we had this in
mainline soon. Does it make sense for me to send a Reviewed-by tag,
given I already have a S-o-b one?

I have a few _extremely_ minor notes below, but I hope they won't
slow down merging this work. They can definitely be addressed as a
follow-up patch after merging this.

Thank you a lot Tomi for having persisted in improving the ATR code!

diff --git a/Documentation/i2c/muxes/i2c-atr.rst b/Documentation/i2c/muxes/i2c-atr.rst
new file mode 100644
index 000000000000..da226fd4de63
--- /dev/null
+++ b/Documentation/i2c/muxes/i2c-atr.rst
@@ -0,0 +1,97 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=====================
+Kernel driver i2c-atr
+=====================
+
+Author: Luca Ceresoli <luca@xxxxxxxxxxxxxxxx>
+Author: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
+
+Description
+-----------
+
+An I2C Address Translator (ATR) is a device with an I2C slave parent
+("upstream") port and N I2C master child ("downstream") ports, and
+forwards transactions from upstream to the appropriate downstream port
+with a modified slave address. The address used on the parent bus is
+called the "alias" and is (potentially) different from the physical
+slave address of the child bus. Address translation is done by the
+hardware.
+
+An ATR looks similar to an i2c-mux except:
+ - the address on the parent and child busses can be different
+ - there is normally no need to select the child port; the alias used on the
+   parent bus implies it
+
+The ATR functionality can be provided by a chip with many other
+features. This file provides a helper to implement an ATR within your
+driver.
+
+The ATR creates a new I2C "child" adapter on each child bus. Adding
+devices on the child bus ends up in invoking the driver code to select
+an available alias. Maintaining an appropriate pool of available aliases
+and picking one for each new device is up to the driver implementer. The
+ATR maintains an table of currently assigned alias and uses it to modify

s/an table/a table/

Right.

+all I2C transactions directed to devices on the child buses.
+
+A typical example follows.
+
+Topology::
+
+                      Slave X @ 0x10
+              .-----.   |
+  .-----.     |     |---+---- B
+  | CPU |--A--| ATR |
+  `-----'     |     |---+---- C
+              `-----'   |
+                      Slave Y @ 0x10
+
+Alias table:
+
+A, B and C are three physical I2C busses, electrically independent from
+each other. The ATR receives the transactions initiated on bus A and
+propagates them on bus B or bus C or none depending on the device address
+in the transaction and based on the alias table.
+
+Alias table:
+
+.. table::
+
+   ===============   =====
+   Client            Alias
+   ===============   =====
+   X (bus B, 0x10)   0x20
+   Y (bus C, 0x10)   0x30
+   ===============   =====
+
+Transaction:
+
+ - Slave X driver sends a transaction (on adapter B), slave address 0x10

s/sends/requests/ is possibly better to clarify there is still no
electrical transaction yet at this step, as we are still in software.

I don't like "requests" too much either, but I see your point and I think it's better than "sends".

+ - ATR driver finds slave X is on bus B and has alias 0x20, rewrites
+   messages with address 0x20, forwards to adapter A
+ - Physical I2C transaction on bus A, slave address 0x20
+ - ATR chip detects transaction on address 0x20, finds it in table,
+   propagates transaction on bus B with address translated to 0x10,
+   keeps clock streched on bus A waiting for reply
+ - Slave X chip (on bus B) detects transaction at its own physical
+   address 0x10 and replies normally
+ - ATR chip stops clock stretching and forwards reply on bus A,
+   with address translated back to 0x20
+ - ATR driver receives the reply, rewrites messages with address 0x10
+   as they were initially
+ - Slave X driver gets back the msgs[], with reply and address 0x10
+
+Usage:
+
+ 1. In your driver (typically in the probe function) add an ATR by
+    calling i2c_atr_new() passing your attach/detach callbacks
+ 2. When the attach callback is called pick an appropriate alias,
+    configure it in your chip and return the chosen alias in the
+    alias_id parameter
+ 3. When the detach callback is called, deconfigure the alias from
+    your chip and put it back in the pool for later usage
+
+I2C ATR functions and data structures
+-------------------------------------
+
+.. kernel-doc:: include/linux/i2c-atr.h

...

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
new file mode 100644
index 000000000000..5ab890b83670
--- /dev/null
+++ b/drivers/i2c/i2c-atr.c
@@ -0,0 +1,548 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * I2C Address Translator
+ *
+ * Copyright (c) 2019,2022 Luca Ceresoli <luca@xxxxxxxxxxxxxxxx>
+ * Copyright (c) 2022,2023 Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
+ *
+ * Originally based on i2c-mux.c

Not quite anymore I think... should this line be removed?

Well, it still originally based on i2c-mux. Maybe it doesn't resemble i2c-mux much anymore, but the above line is a kind of thanks for the original authors.

+/**
+ * struct i2c_atr - The I2C ATR instance
+ * @parent:    The parent &struct i2c_adapter
+ * @dev:       The device that owns the I2C ATR instance
+ * @ops:       &struct i2c_atr_ops
+ * @priv:      Private driver data, set with i2c_atr_set_driver_data()
+ * @algo:      The &struct i2c_algorithm for adapters
+ * @lock:      Lock for the I2C bus segment (see &struct i2c_lock_operations)
+ * @max_adapters: Maximum number of adapters this I2C ATR can have
+ * @adapter:   Array of adapters
+ */
+struct i2c_atr {
+	struct i2c_adapter *parent;
+	struct device *dev;
+	const struct i2c_atr_ops *ops;
+
+	void *priv;
+
+	struct i2c_algorithm algo;
+	/* lock for the I2C bus segment (see struct i2c_lock_operations) */

This comment is identical to the one in the kerneldoc comments just
above, I'd just remove it.

checkpatch wants an explicit comment for each lock.

+	struct mutex lock;
+	int max_adapters;
+
+	struct notifier_block i2c_nb;

Undocumented?

Indeed, I'll add something here.

...

+void i2c_atr_delete(struct i2c_atr *atr)
+{

Maybe here we could iterate over atr->adapter[] and if any is != NULL
just call BUG_ON() or WARN()?

Yes, good idea.

+	bus_unregister_notifier(&i2c_bus_type, &atr->i2c_nb);
+	mutex_destroy(&atr->lock);
+	kfree(atr);
+}
+EXPORT_SYMBOL_NS_GPL(i2c_atr_delete, I2C_ATR);

...

diff --git a/include/linux/i2c-atr.h b/include/linux/i2c-atr.h
new file mode 100644
index 000000000000..7596f70ce1ab
--- /dev/null
+++ b/include/linux/i2c-atr.h
@@ -0,0 +1,116 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * I2C Address Translator
+ *
+ * Copyright (c) 2019,2022 Luca Ceresoli <luca@xxxxxxxxxxxxxxxx>
+ * Copyright (c) 2022,2023 Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
+ *
+ * Based on i2c-mux.h

As above, this does not apply very much anymore as it did in v1.

...

+/**
+ * i2c_atr_delete - Delete an I2C ATR helper.
+ * @atr: I2C ATR helper to be deleted.
+ *
+ * Precondition: all the adapters added with i2c_atr_add_adapter() mumst be

s/mumst/must/

Yep.

Thanks for the comments!

 Tomi




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux