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

Tomi Valkeinen
2023-03-17T12:37:38Z

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@ideasonboard.com> wrote:
>
>> From: Luca Ceresoli <luca@lucaceresoli.net>
>>
>> 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@lucaceresoli.net>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>
> 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@lucaceresoli.net>
>> +Author: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> +
>> +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@lucaceresoli.net>
>> + * Copyright (c) 2022,2023 Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> + *
>> + * 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@lucaceresoli.net>
>> + * Copyright (c) 2022,2023 Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> + *
>> + * 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