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/
> +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.
> + - 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?
> +/**
> + * 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.
> + struct mutex lock;
> + int max_adapters;
> +
> + struct notifier_block i2c_nb;
Undocumented?
...
> +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()?
> + 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/
Best regards,
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
