Message-ID: <b281b472-f911-1f04-2cc8-c3713e771bf6@ideasonboard.com>
Date:   Fri, 17 Mar 2023 14:36:25 +0200
From:   Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To:     Luca Ceresoli <luca.ceresoli@...tlin.com>,
        Wolfram Sang <wsa@...nel.org>
Cc:     linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        Matti Vaittinen <Matti.Vaittinen@...rohmeurope.com>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Peter Rosin <peda@...ntia.se>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Michael Tretter <m.tretter@...gutronix.de>,
        Hans Verkuil <hverkuil@...all.nl>,
        Mike Pagano <mpagano@...too.org>,
        Krzysztof Hałasa <khalasa@...p.pl>,
        Marek Vasut <marex@...x.de>,
        Satish Nagireddy <satish.nagireddy@...cruise.com>,
        Luca Ceresoli <luca@...aceresoli.net>
Subject: Re: [PATCH v10 1/8] i2c: add I2C Address Translator (ATR) support

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

Powered by blists - more mailing lists