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@xxxxxxxxxxxxxxxx> >>> >>> --- >>> >>> 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
