Message-ID: <51dede3c-545b-b66a-5e89-9e889d784eb9@axentia.se>
Date:   Wed, 4 Sep 2019 08:09:01 +0000
From:   Peter Rosin <peda@...ntia.se>
To:     Luca Ceresoli <luca@...aceresoli.net>,
        jacopo mondi <jacopo@...ndi.org>
CC:     "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Wolfram Sang <wsa@...-dreams.de>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Kieran Bingham <kieran.bingham@...asonboard.com>,
        Vladimir Zapolskiy <vz@...ia.com>
Subject: Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support

Hi!

[ Sorry about my absence. I've been meaning to comment on this series
  for a long time, but work and family keep interfering... ]

On 2019-09-03 09:31, Luca Ceresoli wrote:
> Hi Jacopo,
> 
> thanks for your feedback.
> 
> On 01/09/19 16:31, jacopo mondi wrote:
>> Hi Luca,
>>    thanks for keep pushing this series! I hope we can use part of this
>> for the (long time) on-going GMSL work...
>>
>> I hope you will be patient enough to provide (another :) overview
>> of this work during the BoF Wolfram has organized at LPC for the next
>> week.
> 
> Sure!
> 
>> In the meantime I would have some comments after having a read at the
>> series and trying to apply its concept to GMSL
>>
>> On Tue, Jul 23, 2019 at 10:37:19PM +0200, Luca Ceresoli wrote:
>>> 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 is
>>> 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>
>>>
>>> ---
>>>
>>> Changes RFCv1 -> RFCv2:
>>>
>>>  RFCv1 was implemented inside i2c-mux.c and added yet more complexity
>>>  there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR
>>>  features are not in a MUX and vice versa, the overlapping is low. This was
>>>  almost a complete rewrite, but for the records here are the main
>>>  differences from the old implementation:
>>
>> I'm not an i2c expert, but this looks very similar to me to an
>> augmented i2c-mux with the following additional features:
>> - automatic selection of the i2c address to use for the children
>>   behind the mux
>> - automatic translation of the addresses the logical aliases to
>>   the actual physical addresses of the slaves (with the help of the
>>   deserializer address translation feature in this case).
> 
> A notable difference in the hardware is that a mux needs an explicit
> procedure to select a port. That's why the select() op is mandatory for
> muxes. The ATR, at least in the DS90UB9xx case, selects the port
> automatically based on the slave address. So I added an optional
> select() op in the atr, but I suspect it's useless for "real" ATRs.
> 
> More differences derive from how Linux implements muxes. The i2c-mux
> code has several flags for different locking schemes, and it has a

It's two locking schemes if you count them carefully, so several is
a bit of an exaggeration. But agreed, two is more than I prefer.

> fairly complex DT parsing scheme. These are mostly a historical burden.
> Adding the ATR features to i2c-mux.c was very tricky and error-prone due
> to all of this code, that's why I have moved ATR to its own file in RFCv2.

Anyway, the locking in i2c-mux may be complex, but it does solve real
problems. The way I read this series, these problems are not dealt with
and therefore the ATR code will not function in all surroundings.

Some things to think about:
- What happens when you put a mux behind an ATR?
- What happens when you put an ATR behind a mux?
- What happens when you put an ATR between two muxes?
- Does it make a difference if the mux is parent-locked or mux-locked?
- What happens if client drivers lock the adapter in order to silence the
  bus for some reason or to keep two xfers together or something, and
  then do unlocked xfers?
- Can you put an ATR behind another ATR?
etc

I'm not saying that these things must be handled, and maybe they are
handled already, and maybe some of the combinations are not valid at
all. But the possibilities and limitations should be understood. And
preferably documented.

My gut feeling (without spending much time on it) is that ATR as
implemented in this series behave approximately like mux-locked muxes,
meaning that it is not obviously safe to put a parent-locked mux behind
an ATR and making it impossible for client devices behind an ATR to force
xfers to happen back-to-back from the root adapter.

And finally, I feel that it is not wise to introduce a third locking
scheme in the I2C topology. It's bad enough with two. Why not make the
ATR locking exactly match the mux-locked scheme to reduce the number
of cases one needs to consider? But maybe that's just me?

Cheers,
Peter

Powered by blists - more mailing lists