| Message ID | 20220206115939.3091265-1-luca@lucaceresoli.net |
|---|---|
| Headers | show |
| Series | Hi, | expand |
Morning Tomi, On 2/7/22 18:23, Tomi Valkeinen wrote: > On 07/02/2022 16:38, Vaittinen, Matti wrote: >> Hi again Luca, >> >> On 2/7/22 16:07, Luca Ceresoli wrote: >>> Hi Matti, >>> >>> On 07/02/22 14:21, Vaittinen, Matti wrote: >>>> Hi dee Ho peeps, >>>> >>>> On 2/7/22 14:06, Tomi Valkeinen wrote: >>>>> Hi Luca, >>>>> >>>>> On 06/02/2022 13:59, Luca Ceresoli wrote: >>>>>> this RFCv3, codename "FOSDEM Fries", of RFC patches to support the TI >>>>>> DS90UB9xx serializer/deserializer chipsets with I2C address >>>>>> translation. >>>> >>>> >>>> I am not sure if I am poking in the nest of the wasps - but there's one >>>> major difference with the work I've done and with Toni's / Luca's work. >>> >>> You are. ;) >>> >>>> The TI DES drivers (like ub960 driver) packs pretty much everything >>>> under single driver at media/i2c - which (in my opinion) makes the >>>> driver pretty large one. >>>> >>>> My approach is/was to utilize MFD - and prepare the regmap + IRQs in >>>> the >>>> MFD (as is pretty usual) - and parse that much of the device-tree that >>>> we see how many SER devices are there - and that I get the non I2C >>>> related DES<=>SER link parameters set. After that I do kick alive the >>>> separate MFD cells for ATR, pinctrl/GPIO and media. >>>> >>>> The ATR driver instantiates the SER I2C devices like Toni's ub960 does. >>>> The SER compatible is once again matched in MFD (for SER) - which again >>>> provides regmap for SER, does initial I2C writes so SER starts >>>> responding to I2C reads and then kicks cells for media and >>>> pinctrl/gpio. >>>> >>>> I believe splitting the functionality to MFD subdevices makes drivers >>>> slightly clearer. You'll get GPIOs/pinctrl under pinctrl as usual, >>>> regmaps/IRQ-chips under MFD and only media/v4l2 related parts under >>>> media. >>> >>> There has been quite a fiery discussion about this in the past, you can >>> grab some popcorn and read >>> https://lore.kernel.org/linux-media/20181008211205.2900-1-vz@mleia.com/T/#m9b01af81665ac956af3c6d57810239420c3f8cee >>> >>> >>> TL;DR: there have been strong opposition the the MFD idea. >> >> Hm. I may be missing something but I didn't see opposition to using MFD >> or splitting the drivers. I do see opposition to adding _functionality_ >> in MFD. If I read this correctly, Lee did oppose adding the I2C stuff, >> sysfs attributes etc in MFD. Quoting his reply: >> >> "This driver does too much real work ('stuff') to be an MFD driver. >> MFD drivers should not need to care of; links, gates, modes, pixels, >> frequencies maps or properties. Nor should they contain elaborate >> sysfs structures to control the aforementioned 'stuff'. >> >> Granted, there may be some code in there which could be appropriate >> for an MFD driver. However most of it needs moving out into a >> function driver (or two)." >> >> And I tend to agree with Lee here. I would not put I2C bridge stuff or >> sysfs attributes in MFD. But I think it does not mean SERDESes should >> not use MFD when they clearly contain more IP blocks than the >> video/media ones :) I am confident Lee and others might be much more >> welcoming for driver which simply configures regmap and kicks subdriver >> for doing the ATR / I2C stuff. > > I admit that I don't know MFD drivers too well, but I was thinking about > this some time back and I wasn't quite sure about using MFD here. > > My thinking was that MFD is fine and good when a device contains more or > less independent functionalities, like a PMIC with, say, gpios and > regulators, both of which just work as long as the PMIC is powered up. > > Here all the functionalities depend on the link (fpdlink or some other > "link" =), and the serializers. In other words, the link status or any > changes to the link or the serializers might affect the GPIO/I2C/IRQ > functionalities. My use case has been such that once the link between DES & SER established, it should not go away. If it does it is some kind of an error and there is no recovery mechanims (at least not yet). Hence I haven't prepared full solution how to handle dropping/re-connecting the link or re-initializing des/ser/slaves. > So, I don't have any clear concern here. Just a vague feeling that the > functionalities in this kind of devices may be more tightly tied > together than in normal MFDs. I could be totally wrong here. I can't prove you're wrong even if that would be so cool :p I guess a lot of this boils down how the SER behaves when link is dropped. Does it maintain the configuration or reset to some other state? And what happens on des & what we need to do in order to reconnect. My initial feeling is that the DES should always be available as it is directly connected to I2C. So DES should always be there. Access to SERs and the devices on remote buses is naturally depending on the link. So dropping the link means access to SERs and remote devices start failing - which is probably visible to the MFD sub-devices as failing regmap accesses. This needs then appropriate handling. After that being said, I think we can't get over this problem even when not using MFD. As far as I read your code, the SER and DES have independent drivers also when MFD is not used. So dropping the link is still someting that pulls the legs from the SER, right? I also guess the remote I2C devices like sensors are also implemented as independent drivers. Well, (I hope) I'll see where I end up with my code... It really makes this discussion a bit dull when I can't just show the code for comparison :/ I don't (yet) see why the MFD approach could not work, and I still think it's worth trying - but I now certainly understand why you hesitated using MFD. Thanks for taking the time to explain this to me. Best Regards --Matti
On 2/8/22 10:28, Tomi Valkeinen wrote: > I'm curious, why do you think using MFDs makes the driver so much > cleaner? The current fpdlink driver is in one file, but, say, if we > split it to multiple files, based on the function, while still keeping > it as a single driver, would that be so much different from an MFD > solution? Is there something in the MFD approach that makes the code > simpler? Fair question. I personally have two reasons - one technical which I could just throw here and hope everyone buys it :) But I think the main reason for me to initially think of MFD is not a technical one. Last few years I've spent working with PMICs/chargers - which were MFD to the bone. Before that I worked on a proprietary clocking/all-purpose FPGA as well as with ASIC routing RP3/RP301 links + providing timing facilities / clocks. Those were also MFD devices - and one of those used MFD drivers, the other didn't although it really should have. So the non technical reason for me is that I am used to seeing multi-function devices implemented as MFD devices - hence I immediately saw the SERDES as one too. One the technical benefit from MFD is that it (in many cases) allows one to use standard way to re-use code. Eg, it's not a rare case that same HW blocks are used in many projects. One can for example see three different PMICs, all having same RTC and clk blocks, while different regulators and GPIOs - or some just omitting one of those. MFD allows 'collecting' these IP blocks under different umbrellas by kicking same subdevices alive via different MFD devices in a standard way. The IP block (say GPIO controller) we are driving can be implemented on SER connected by FPDLINK III to DES - or it can be implemented in PMIC - the absolutely same standrd (mfd sub) platform GPIO driver can be used in both cases. Other than that, the use of MFD allows one to write pinctrl/gpio driver as any pinctrl/gpio platform device driver. It will be looking familiar to anyone who has worked with pinctrl/gpio - even if he has never seen media/v4l2 ;) This is where my thought of clarity came from. Rest is slightly offtopic - you can stop reading. I am not sure how TI does this and if you know whether same blocks can be used in other devices. I just have learned never to trust it when a HW engineer (in Nokia, Rohm or other company) tells me "this is the last IC using this technology". It may be my problem though as nor do I buy it if someone says me: "the next version will be just same to this previous - there is no software impact" :rolleyes: I for example once heard that when the "next product" maintained same register offsets for some functions - but did add new ones - and changed the registers from 16bit => 32bit and connecting bus from PCIe => I2C... I remember that project with a nicknames 're-estimate' 'reschedule' and 'panic-button' :) Yours -- Matti
Hi Andy, thank you for the _very_ detailed review and apologies for not having found the time to reply until now. I'm OK with most of your comments, so I'm not commenting on them for brevity. Below my comments on the remaining topics. On 08/02/22 12:16, Andy Shevchenko wrote: > On Mon, Feb 7, 2022 at 7:55 PM Luca Ceresoli <luca@lucaceresoli.net> 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. > > Why I2C mux driver can't be updated to support this feature? My first version did that. But it was very complex to shoehorn the ATR features in the i2c-mux code which already handles various [corner] cases. If memory serves, code reuse was limited to the trivial code: allocations, cleanups and the like. The root reason is that an atr and a mux have a similar electric topology, but they do very different things. An mux need to be commanded to switch from one downstream bus to another, an atr does not. An atr modifies the transaction, including the speed, a mux does not. >> 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: > > While this is from a code perspective, maybe i2c mux and this one can > still share some parts? Possibly. I'd have to look into that in more detail. I must say having a separate file allowed me to be free to implement whatever is best for atr. With that done I would certainly make sense to check whether there are still enough commonalities to share code, maybe in a .c file with shared functions. >> +config I2C_ATR >> + tristate "I2C Address Translator (ATR) support" >> + help >> + Enable support for I2C Address Translator (ATR) chips. >> + >> + An ATR allows accessing multiple I2C busses from a single >> + physical bus via address translation instead of bus selection as >> + i2c-muxes do. > > What would be the module name? Isn't the module name written in Kconfig files just to avoid checkpatch complain about "too few doc lines"? :) Oook, it's i2s-atr anyway. >> +/** > > Is this a kernel doc formatted documentation? > Haven't you got a warning? Not from checkpatch, but I got one from the kernel test robot. Will fix. [...] >> + * >> + * 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 >> + * 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: >> + * >> + * Client Alias >> + * ------------- >> + * X 0x20 >> + * Y 0x30 >> + * >> + * Transaction: >> + * >> + * - Slave X driver sends a transaction (on adapter B), slave address 0x10 >> + * - ATR driver rewrites messages with address 0x20, forwards to adapter A >> + * - Physical I2C transaction on bus A, slave address 0x20 >> + * - ATR chip propagates transaction on bus B with address translated to 0x10 >> + * - Slave X chip replies on bus B >> + * - ATR chip forwards reply on bus A >> + * - ATR driver rewrites messages with address 0x10 >> + * - 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 >> + * >> + * Originally based on i2c-mux.c >> + */ > > Shouldn't this comment be somewhere under Documentation/ ? Uhm, yes, I agree it's a good idea to move this entire comment there. >> + if (dev->of_node) { > > This check can be dropped, also please use device property and fwnode > APIs. No good of having OF-centric generic modules nowadays. Sure! This code was written in another decade and I didn't update it... As you noticed elsewhere it also honors the old, strict 80-chars per line limit in various places where it makes no sense anymore. >> + WARN(sysfs_create_link(&chan->adap.dev.kobj, &dev->kobj, "atr_device"), >> + "can't create symlink to atr device\n"); >> + snprintf(symlink_name, sizeof(symlink_name), "channel-%u", chan_id); >> + WARN(sysfs_create_link(&dev->kobj, &chan->adap.dev.kobj, symlink_name), >> + "can't create symlink for channel %u\n", chan_id); > > Doesn't sysfs already has a warning when it's really needed? I have to check that. I usually don't add unnecessary log messages. [...] >> +#include <linux/i2c.h> >> +#include <linux/mutex.h> > > Missed types.h > > Missed struct device; Not sure I got your point here. This file has some 'struct device *', which do not need a declaration, and has zero non-pointer uses of 'struct device'.
Hi dee Ho peeps! On 2/6/22 13:59, 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. > snip > diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig > index 438905e2a1d0..c6d1a345ea6d 100644 > --- a/drivers/i2c/Kconfig > +++ b/drivers/i2c/Kconfig > @@ -71,6 +71,15 @@ config I2C_MUX > > source "drivers/i2c/muxes/Kconfig" > > +config I2C_ATR > + tristate "I2C Address Translator (ATR) support" > + help > + Enable support for I2C Address Translator (ATR) chips. > + > + An ATR allows accessing multiple I2C busses from a single > + physical bus via address translation instead of bus selection as > + i2c-muxes do. > + I continued playing with the ROHM (de-)serializer and ended up having .config where the I2C_ATR was ='m', while my ATR driver was ='y' even though it selects the I2C_ATR. Yep, most probably my error somewhere. Anyways, this made me think that most of the I2C_ATR users are likely to just silently select the I2C_ATR, right? The I2C_ATR has no much reason to be compiled in w/o users, right? So perhaps the menu entry for selecting the I2C_ATR could be dropped(?) Do we really need this entry in already long list of configs to be manually picked? snip > +struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev, > + const struct i2c_atr_ops *ops, int max_adapters) > +{ > + struct i2c_atr *atr; > + > + if (!ops || !ops->attach_client || !ops->detach_client) > + return ERR_PTR(-EINVAL); > + I believe that most of the attach_client implementations will have similar approach of allocating and populating an address-pool and searching for first unused address. As a 'further dev' it'd be great to see a common helper implementation for attach/detach - perhaps so that the atr drivers would only need to specify the slave-address configuration register(s) / mask and the use a 'generic' attach/detach helpers. Well, just thinking how to reduce the code from actual IC drivers but this is really not something that is required during this initial series :) Also, devm-variants would be great - although that falls to the same category of things that do not need to be done immediately - but would perhaps be worth considering in the future. so, perhaps reconsider the Kconfig but for what-ever it is worth: Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Yours Matti
Hi Matti, On 16/03/22 15:11, Vaittinen, Matti wrote: > Hi dee Ho peeps! > > On 2/6/22 13:59, 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. >> > > snip > >> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig >> index 438905e2a1d0..c6d1a345ea6d 100644 >> --- a/drivers/i2c/Kconfig >> +++ b/drivers/i2c/Kconfig >> @@ -71,6 +71,15 @@ config I2C_MUX >> >> source "drivers/i2c/muxes/Kconfig" >> >> +config I2C_ATR >> + tristate "I2C Address Translator (ATR) support" >> + help >> + Enable support for I2C Address Translator (ATR) chips. >> + >> + An ATR allows accessing multiple I2C busses from a single >> + physical bus via address translation instead of bus selection as >> + i2c-muxes do. >> + > > I continued playing with the ROHM (de-)serializer and ended up having > .config where the I2C_ATR was ='m', while my ATR driver was ='y' even > though it selects the I2C_ATR. > > Yep, most probably my error somewhere. > > Anyways, this made me think that most of the I2C_ATR users are likely to > just silently select the I2C_ATR, right? The I2C_ATR has no much reason > to be compiled in w/o users, right? So perhaps the menu entry for > selecting the I2C_ATR could be dropped(?) Do we really need this entry > in already long list of configs to be manually picked? Maybe we could make it a blind option, sure. The only reason it could be useful that it's visible is that one might implement a user driver could be written out of tree. I don't care very much about that, but it is possible. Maybe it's the reason for I2C_MUX to be a visible option too. Peter? >> +struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev, >> + const struct i2c_atr_ops *ops, int max_adapters) >> +{ >> + struct i2c_atr *atr; >> + >> + if (!ops || !ops->attach_client || !ops->detach_client) >> + return ERR_PTR(-EINVAL); >> + > > I believe that most of the attach_client implementations will have > similar approach of allocating and populating an address-pool and > searching for first unused address. As a 'further dev' it'd be great to > see a common helper implementation for attach/detach - perhaps so that > the atr drivers would only need to specify the slave-address > configuration register(s) / mask and the use a 'generic' attach/detach > helpers. Well, just thinking how to reduce the code from actual IC > drivers but this is really not something that is required during this > initial series :) > > Also, devm-variants would be great - although that falls to the same > category of things that do not need to be done immediately - but would > perhaps be worth considering in the future. Both of your proposals make sense, however I did deliberately not generalize too much because I knew only one chipset. I don't like trying to generalize for an unpredictable future use case, it generally leads (me) to generalizing in the wrong direction. That means you'd be very welcome to propose helpers and/or devm variants, possibly in the same patchset as the first Rohm serdes driver. ;) > Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Thanks for your review!