| Message ID | 20180622104930.32050-1-boris.brezillon@bootlin.com |
|---|---|
| Headers | show |
| Series | Add the I3C subsystem | expand |
Hi, On 06/22/2018 03:49 AM, Boris Brezillon wrote: > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 71c0ab46f216..19ed6006aea1 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -898,6 +898,17 @@ config GPIO_TS4900 > > endmenu > > +menu "I3C GPIO expanders" > + depends on I3C > + > +config GPIO_CDNS_I3C > + tristate "Cadence I3C GPIO expander" > + select GPIOLIB_IRQCHIP > + help > + Say yes here to enabled the driver for Cadence I3C GPIO expander. to enable the driver > + > +endmenu
On Fri, 22 Jun 2018 09:04:47 -0700 Randy Dunlap <rdunlap@infradead.org> wrote: > Hi, > > On 06/22/2018 03:49 AM, Boris Brezillon wrote: > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > > index 71c0ab46f216..19ed6006aea1 100644 > > --- a/drivers/gpio/Kconfig > > +++ b/drivers/gpio/Kconfig > > @@ -898,6 +898,17 @@ config GPIO_TS4900 > > > > endmenu > > > > +menu "I3C GPIO expanders" > > + depends on I3C > > + > > +config GPIO_CDNS_I3C > > + tristate "Cadence I3C GPIO expander" > > + select GPIOLIB_IRQCHIP > > + help > > + Say yes here to enabled the driver for Cadence I3C GPIO expander. > > to enable the driver I'll fix this typo Thanks, Boris -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-06-22 12:49, Boris Brezillon wrote: > Add core infrastructure to support I3C in Linux and document it. > > This infrastructure is not complete yet and will be extended over > time. > > There are a few design choices that are worth mentioning because they > impact the way I3C device drivers can interact with their devices: > > - all functions used to send I3C/I2C frames must be called in > non-atomic context. Mainly done this way to ease implementation, but > this is still open to discussion. Please let me know if you think > it's worth considering an asynchronous model here > - the bus element is a separate object and is not implicitly described > by the master (as done in I2C). The reason is that I want to be able > to handle multiple master connected to the same bus and visible to > Linux. > In this situation, we should only have one instance of the device and > not one per master, and sharing the bus object would be part of the > solution to gracefully handle this case. > I'm not sure we will ever need to deal with multiple masters > controlling the same bus and exposed under Linux, but separating the > bus and master concept is pretty easy, hence the decision to do it > like that. > The other benefit of separating the bus and master concepts is that > master devices appear under the bus directory in sysfs. Are bus multiplexers relevant to I3C? The locking needed for handling muxes for I2C is, well, convoluted... Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Peter, On Fri, 22 Jun 2018 23:35:34 +0200 Peter Rosin <peda@axentia.se> wrote: > On 2018-06-22 12:49, Boris Brezillon wrote: > > Add core infrastructure to support I3C in Linux and document it. > > > > This infrastructure is not complete yet and will be extended over > > time. > > > > There are a few design choices that are worth mentioning because they > > impact the way I3C device drivers can interact with their devices: > > > > - all functions used to send I3C/I2C frames must be called in > > non-atomic context. Mainly done this way to ease implementation, but > > this is still open to discussion. Please let me know if you think > > it's worth considering an asynchronous model here > > - the bus element is a separate object and is not implicitly described > > by the master (as done in I2C). The reason is that I want to be able > > to handle multiple master connected to the same bus and visible to > > Linux. > > In this situation, we should only have one instance of the device and > > not one per master, and sharing the bus object would be part of the > > solution to gracefully handle this case. > > I'm not sure we will ever need to deal with multiple masters > > controlling the same bus and exposed under Linux, but separating the > > bus and master concept is pretty easy, hence the decision to do it > > like that. > > The other benefit of separating the bus and master concepts is that > > master devices appear under the bus directory in sysfs. > > Are bus multiplexers relevant to I3C? Not yet, but who knows. > The locking needed for handling > muxes for I2C is, well, convoluted... Do you remember what was the problem? Anyway, I'd really like to have basic support upstreamed before we start considering advanced use cases that do not exist yet. Don't get me wrong, I'm not against having the multiplexer/locking discussion, but it should not block inclusion of the I3C subsystem. Regards, Boris -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-06-23 12:17, Boris Brezillon wrote: > Hi Peter, > > On Fri, 22 Jun 2018 23:35:34 +0200 > Peter Rosin <peda@axentia.se> wrote: > >> On 2018-06-22 12:49, Boris Brezillon wrote: >>> Add core infrastructure to support I3C in Linux and document it. >>> >>> This infrastructure is not complete yet and will be extended over >>> time. >>> >>> There are a few design choices that are worth mentioning because they >>> impact the way I3C device drivers can interact with their devices: >>> >>> - all functions used to send I3C/I2C frames must be called in >>> non-atomic context. Mainly done this way to ease implementation, but >>> this is still open to discussion. Please let me know if you think >>> it's worth considering an asynchronous model here >>> - the bus element is a separate object and is not implicitly described >>> by the master (as done in I2C). The reason is that I want to be able >>> to handle multiple master connected to the same bus and visible to >>> Linux. >>> In this situation, we should only have one instance of the device and >>> not one per master, and sharing the bus object would be part of the >>> solution to gracefully handle this case. >>> I'm not sure we will ever need to deal with multiple masters >>> controlling the same bus and exposed under Linux, but separating the >>> bus and master concept is pretty easy, hence the decision to do it >>> like that. >>> The other benefit of separating the bus and master concepts is that >>> master devices appear under the bus directory in sysfs. >> >> Are bus multiplexers relevant to I3C? > > Not yet, but who knows. > >> The locking needed for handling >> muxes for I2C is, well, convoluted... > > Do you remember what was the problem? > > Anyway, I'd really like to have basic support upstreamed before we > start considering advanced use cases that do not exist yet. Don't get > me wrong, I'm not against having the multiplexer/locking discussion, > but it should not block inclusion of the I3C subsystem. I'm trying to avoid the unfortunate situation in I2C where there are two slightly incompatible locking schemes for muxes. There's probably nothing to worry about until the first I3C mux is added though. But since I2C devices are supposedly compatible with I3C that might be the case from day one? --- If I read your code right, I3C client drivers will typically call i3c_device_do_priv_xfer (instead of i2c_transfer/i2c_smbus_xfer) and i3c_device_do_prov_xfer will grab the i3c_bus_normaluse_lock during the transfer. This seems equivalent to normal use in I2C with i2c_transfer/i2c_smbus_xfer. When muxes are thrown into the mix, you find yourself needing to do more than the "real" transfer with some lock held. In I2C there is an unlocked __i2c_transfer, and locking/unlocking is exposed. Muxes typically grab the lock, set the mux in the appropriate position, do an unlocked __i2c_transfer, optionally sets the mux in some default position and then lets go of the lock. See e.g. i2c/muxes/i2c-mux-pca954x.c However, that is the simple case. There are also muxes that are controlled with GPIO pins, and that gets hairy if the GPIO pins are controlled from the same I2C bus that is muxed. The GPIO driver would have to know that some GPIO pins need to use unlocked I2C transfers for that to work with the above locking scheme. And no GPIO driver does not want to know about that at all. I.e. you have two fundamentally different requirement depending on if the GPIO pins controlling the mux are controlled using the muxed bus or if the pins are controlled in some completely unrelated way. The latter case is probably the normal case, with the GPIO mux controlled directly from some SoC pins. In the latter case you also want to prevent any transfer on the bus while the GPIO pins for the mux are changing, i.e. the total opposite of the former case. It gets really really hairy if you have multiple levels of muxes... There are a some old drivers (e.g. i2c/busses/i2c-amd756-s4882.c) that handles this by simply bypassing the GPIO subsystem, but that is not really an option if some pins are used to mux the I2C bus while some pins are used for other things. I don't know if this affects I3C before muxes are added, but I suspect muxes will happen sooner rather than later, since the spec mentions that the bus only support 11 devices maximum. 11 don't seem like a lot, and it seems likely that there will be a need to have more devices somehow... But just maybe, in order to not run into the above situation, it needs to be handled right from the start with preparatory and cleanup stages of each transfers, or something? Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 23 Jun 2018 23:40:36 +0200 Peter Rosin <peda@axentia.se> wrote: > On 2018-06-23 12:17, Boris Brezillon wrote: > > Hi Peter, > > > > On Fri, 22 Jun 2018 23:35:34 +0200 > > Peter Rosin <peda@axentia.se> wrote: > > > >> On 2018-06-22 12:49, Boris Brezillon wrote: > >>> Add core infrastructure to support I3C in Linux and document it. > >>> > >>> This infrastructure is not complete yet and will be extended over > >>> time. > >>> > >>> There are a few design choices that are worth mentioning because they > >>> impact the way I3C device drivers can interact with their devices: > >>> > >>> - all functions used to send I3C/I2C frames must be called in > >>> non-atomic context. Mainly done this way to ease implementation, but > >>> this is still open to discussion. Please let me know if you think > >>> it's worth considering an asynchronous model here > >>> - the bus element is a separate object and is not implicitly described > >>> by the master (as done in I2C). The reason is that I want to be able > >>> to handle multiple master connected to the same bus and visible to > >>> Linux. > >>> In this situation, we should only have one instance of the device and > >>> not one per master, and sharing the bus object would be part of the > >>> solution to gracefully handle this case. > >>> I'm not sure we will ever need to deal with multiple masters > >>> controlling the same bus and exposed under Linux, but separating the > >>> bus and master concept is pretty easy, hence the decision to do it > >>> like that. > >>> The other benefit of separating the bus and master concepts is that > >>> master devices appear under the bus directory in sysfs. > >> > >> Are bus multiplexers relevant to I3C? > > > > Not yet, but who knows. > > > >> The locking needed for handling > >> muxes for I2C is, well, convoluted... > > > > Do you remember what was the problem? > > > > Anyway, I'd really like to have basic support upstreamed before we > > start considering advanced use cases that do not exist yet. Don't get > > me wrong, I'm not against having the multiplexer/locking discussion, > > but it should not block inclusion of the I3C subsystem. > > I'm trying to avoid the unfortunate situation in I2C where there > are two slightly incompatible locking schemes for muxes. There's > probably nothing to worry about until the first I3C mux is added > though. But since I2C devices are supposedly compatible with I3C > that might be the case from day one? The I²C backward compatibility is implemented in a pretty simple way, so I don't think you'll have problems coming from the I3C part on this (unless it needs special hooks in i2c_adapter to work properly). This being said, if the I2C framework is already not able to properly handle the cases you describe below, the I3C layer won't solve that :-P. > > --- > > If I read your code right, I3C client drivers will typically call > i3c_device_do_priv_xfer (instead of i2c_transfer/i2c_smbus_xfer) > and i3c_device_do_prov_xfer will grab the i3c_bus_normaluse_lock > during the transfer. This seems equivalent to normal use in > I2C with i2c_transfer/i2c_smbus_xfer. Note that the bus lock is a read/write lock which is mostly taken in read mode (AKA normaluse mode). The only situation where it's taken in write mode (AKA maintenance mode) is when a bus maintenance operation is done. In this case, we need to block all future transfers, because maintenance operations might change dynamic device addresses, which would make future transfers irrelevant if they were queued before the maintenance operation is finished. The bus lock does not guarantee proper serialization of bus accesses. This task is left to the controller drivers, since this is what they tend to optimize (by queuing transfers at the HW level). > > When muxes are thrown into the mix, you find yourself needing to > do more than the "real" transfer with some lock held. In I2C there > is an unlocked __i2c_transfer, and locking/unlocking is exposed. > Muxes typically grab the lock, set the mux in the appropriate > position, do an unlocked __i2c_transfer, optionally sets the mux > in some default position and then lets go of the lock. See e.g. > i2c/muxes/i2c-mux-pca954x.c > > However, that is the simple case. There are also muxes that are > controlled with GPIO pins, and that gets hairy if the GPIO pins > are controlled from the same I2C bus that is muxed. The GPIO > driver would have to know that some GPIO pins need to use unlocked > I2C transfers for that to work with the above locking scheme. And > no GPIO driver does not want to know about that at all. I.e. you > have two fundamentally different requirement depending on if the > GPIO pins controlling the mux are controlled using the muxed bus > or if the pins are controlled in some completely unrelated way. > The latter case is probably the normal case, with the GPIO mux > controlled directly from some SoC pins. In the latter case you > also want to prevent any transfer on the bus while the GPIO pins > for the mux are changing, i.e. the total opposite of the former > case. It gets really really hairy if you have multiple levels > of muxes... > > There are a some old drivers (e.g. i2c/busses/i2c-amd756-s4882.c) > that handles this by simply bypassing the GPIO subsystem, but > that is not really an option if some pins are used to mux the > I2C bus while some pins are used for other things. I see. > > I don't know if this affects I3C before muxes are added, but I > suspect muxes will happen sooner rather than later, since the > spec mentions that the bus only support 11 devices maximum. 11 > don't seem like a lot, and it seems likely that there will be > a need to have more devices somehow... I can't tell, and that's the whole problem here. How can you design a solution for something that does not exist yet? Fixing the I2C muxing logic, if it needs to be, is something I can understand. But how can you envision what I3C muxes (if they ever exist) will look like? > > But just maybe, in order to not run into the above situation, it > needs to be handled right from the start with preparatory and > cleanup stages of each transfers, or something? How about applying this approach to I2C first and see how it flies. Changing the I3C framework afterwards (when I3C muxes come in) shouldn't be that complicated. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-06-24 14:02, Boris Brezillon wrote: > On Sat, 23 Jun 2018 23:40:36 +0200 > Peter Rosin <peda@axentia.se> wrote: > >> On 2018-06-23 12:17, Boris Brezillon wrote: >>> Hi Peter, >>> >>> On Fri, 22 Jun 2018 23:35:34 +0200 >>> Peter Rosin <peda@axentia.se> wrote: >>> >>>> On 2018-06-22 12:49, Boris Brezillon wrote: >>>>> Add core infrastructure to support I3C in Linux and document it. >>>>> >>>>> This infrastructure is not complete yet and will be extended over >>>>> time. >>>>> >>>>> There are a few design choices that are worth mentioning because they >>>>> impact the way I3C device drivers can interact with their devices: >>>>> >>>>> - all functions used to send I3C/I2C frames must be called in >>>>> non-atomic context. Mainly done this way to ease implementation, but >>>>> this is still open to discussion. Please let me know if you think >>>>> it's worth considering an asynchronous model here >>>>> - the bus element is a separate object and is not implicitly described >>>>> by the master (as done in I2C). The reason is that I want to be able >>>>> to handle multiple master connected to the same bus and visible to >>>>> Linux. >>>>> In this situation, we should only have one instance of the device and >>>>> not one per master, and sharing the bus object would be part of the >>>>> solution to gracefully handle this case. >>>>> I'm not sure we will ever need to deal with multiple masters >>>>> controlling the same bus and exposed under Linux, but separating the >>>>> bus and master concept is pretty easy, hence the decision to do it >>>>> like that. >>>>> The other benefit of separating the bus and master concepts is that >>>>> master devices appear under the bus directory in sysfs. >>>> >>>> Are bus multiplexers relevant to I3C? >>> >>> Not yet, but who knows. >>> >>>> The locking needed for handling >>>> muxes for I2C is, well, convoluted... >>> >>> Do you remember what was the problem? >>> >>> Anyway, I'd really like to have basic support upstreamed before we >>> start considering advanced use cases that do not exist yet. Don't get >>> me wrong, I'm not against having the multiplexer/locking discussion, >>> but it should not block inclusion of the I3C subsystem. >> >> I'm trying to avoid the unfortunate situation in I2C where there >> are two slightly incompatible locking schemes for muxes. There's >> probably nothing to worry about until the first I3C mux is added >> though. But since I2C devices are supposedly compatible with I3C >> that might be the case from day one? > > The I²C backward compatibility is implemented in a pretty simple way, so > I don't think you'll have problems coming from the I3C part on this > (unless it needs special hooks in i2c_adapter to work properly). This > being said, if the I2C framework is already not able to properly > handle the cases you describe below, the I3C layer won't solve > that :-P. > >> >> --- >> >> If I read your code right, I3C client drivers will typically call >> i3c_device_do_priv_xfer (instead of i2c_transfer/i2c_smbus_xfer) >> and i3c_device_do_prov_xfer will grab the i3c_bus_normaluse_lock >> during the transfer. This seems equivalent to normal use in >> I2C with i2c_transfer/i2c_smbus_xfer. > > Note that the bus lock is a read/write lock which is mostly taken in > read mode (AKA normaluse mode). The only situation where it's taken in > write mode (AKA maintenance mode) is when a bus maintenance operation is > done. In this case, we need to block all future transfers, because > maintenance operations might change dynamic device addresses, which > would make future transfers irrelevant if they were queued before the > maintenance operation is finished. > > The bus lock does not guarantee proper serialization of bus accesses. > This task is left to the controller drivers, since this is what they > tend to optimize (by queuing transfers at the HW level). Oh. Will that design decision (localized serialization) not make it extremely hard to implement muxing (and gating and other stuff that you need, at least for I2C) that is controller independent? >> When muxes are thrown into the mix, you find yourself needing to >> do more than the "real" transfer with some lock held. In I2C there >> is an unlocked __i2c_transfer, and locking/unlocking is exposed. >> Muxes typically grab the lock, set the mux in the appropriate >> position, do an unlocked __i2c_transfer, optionally sets the mux >> in some default position and then lets go of the lock. See e.g. >> i2c/muxes/i2c-mux-pca954x.c >> >> However, that is the simple case. There are also muxes that are >> controlled with GPIO pins, and that gets hairy if the GPIO pins >> are controlled from the same I2C bus that is muxed. The GPIO >> driver would have to know that some GPIO pins need to use unlocked >> I2C transfers for that to work with the above locking scheme. And >> no GPIO driver does not want to know about that at all. I.e. you >> have two fundamentally different requirement depending on if the >> GPIO pins controlling the mux are controlled using the muxed bus >> or if the pins are controlled in some completely unrelated way. >> The latter case is probably the normal case, with the GPIO mux >> controlled directly from some SoC pins. In the latter case you >> also want to prevent any transfer on the bus while the GPIO pins >> for the mux are changing, i.e. the total opposite of the former >> case. It gets really really hairy if you have multiple levels >> of muxes... >> >> There are a some old drivers (e.g. i2c/busses/i2c-amd756-s4882.c) >> that handles this by simply bypassing the GPIO subsystem, but >> that is not really an option if some pins are used to mux the >> I2C bus while some pins are used for other things. > > I see. > >> >> I don't know if this affects I3C before muxes are added, but I >> suspect muxes will happen sooner rather than later, since the >> spec mentions that the bus only support 11 devices maximum. 11 >> don't seem like a lot, and it seems likely that there will be >> a need to have more devices somehow... > > I can't tell, and that's the whole problem here. How can you design a > solution for something that does not exist yet? Fixing the I2C muxing > logic, if it needs to be, is something I can understand. But how can you > envision what I3C muxes (if they ever exist) will look like? Yeah, you have a point there. One problem is that I don't even see how the situation can be unified for I2C... >> >> But just maybe, in order to not run into the above situation, it >> needs to be handled right from the start with preparatory and >> cleanup stages of each transfers, or something? > > How about applying this approach to I2C first and see how it flies. > Changing the I3C framework afterwards (when I3C muxes come in) > shouldn't be that complicated. That would require more thinking first, and I fear that the overhaul will be bigger than what is called for given the rather fringe cases that cause problems. Note that I'm not trying to block I3C, I'm just trying to trigger some thinking before the fact... Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 24 Jun 2018 23:55:34 +0200 Peter Rosin <peda@axentia.se> wrote: > On 2018-06-24 14:02, Boris Brezillon wrote: > > On Sat, 23 Jun 2018 23:40:36 +0200 > > Peter Rosin <peda@axentia.se> wrote: > > > >> On 2018-06-23 12:17, Boris Brezillon wrote: > >>> Hi Peter, > >>> > >>> On Fri, 22 Jun 2018 23:35:34 +0200 > >>> Peter Rosin <peda@axentia.se> wrote: > >>> > >>>> On 2018-06-22 12:49, Boris Brezillon wrote: > >>>>> Add core infrastructure to support I3C in Linux and document it. > >>>>> > >>>>> This infrastructure is not complete yet and will be extended over > >>>>> time. > >>>>> > >>>>> There are a few design choices that are worth mentioning because they > >>>>> impact the way I3C device drivers can interact with their devices: > >>>>> > >>>>> - all functions used to send I3C/I2C frames must be called in > >>>>> non-atomic context. Mainly done this way to ease implementation, but > >>>>> this is still open to discussion. Please let me know if you think > >>>>> it's worth considering an asynchronous model here > >>>>> - the bus element is a separate object and is not implicitly described > >>>>> by the master (as done in I2C). The reason is that I want to be able > >>>>> to handle multiple master connected to the same bus and visible to > >>>>> Linux. > >>>>> In this situation, we should only have one instance of the device and > >>>>> not one per master, and sharing the bus object would be part of the > >>>>> solution to gracefully handle this case. > >>>>> I'm not sure we will ever need to deal with multiple masters > >>>>> controlling the same bus and exposed under Linux, but separating the > >>>>> bus and master concept is pretty easy, hence the decision to do it > >>>>> like that. > >>>>> The other benefit of separating the bus and master concepts is that > >>>>> master devices appear under the bus directory in sysfs. > >>>> > >>>> Are bus multiplexers relevant to I3C? > >>> > >>> Not yet, but who knows. > >>> > >>>> The locking needed for handling > >>>> muxes for I2C is, well, convoluted... > >>> > >>> Do you remember what was the problem? > >>> > >>> Anyway, I'd really like to have basic support upstreamed before we > >>> start considering advanced use cases that do not exist yet. Don't get > >>> me wrong, I'm not against having the multiplexer/locking discussion, > >>> but it should not block inclusion of the I3C subsystem. > >> > >> I'm trying to avoid the unfortunate situation in I2C where there > >> are two slightly incompatible locking schemes for muxes. There's > >> probably nothing to worry about until the first I3C mux is added > >> though. But since I2C devices are supposedly compatible with I3C > >> that might be the case from day one? > > > > The I²C backward compatibility is implemented in a pretty simple way, so > > I don't think you'll have problems coming from the I3C part on this > > (unless it needs special hooks in i2c_adapter to work properly). This > > being said, if the I2C framework is already not able to properly > > handle the cases you describe below, the I3C layer won't solve > > that :-P. > > > >> > >> --- > >> > >> If I read your code right, I3C client drivers will typically call > >> i3c_device_do_priv_xfer (instead of i2c_transfer/i2c_smbus_xfer) > >> and i3c_device_do_prov_xfer will grab the i3c_bus_normaluse_lock > >> during the transfer. This seems equivalent to normal use in > >> I2C with i2c_transfer/i2c_smbus_xfer. > > > > Note that the bus lock is a read/write lock which is mostly taken in > > read mode (AKA normaluse mode). The only situation where it's taken in > > write mode (AKA maintenance mode) is when a bus maintenance operation is > > done. In this case, we need to block all future transfers, because > > maintenance operations might change dynamic device addresses, which > > would make future transfers irrelevant if they were queued before the > > maintenance operation is finished. > > > > The bus lock does not guarantee proper serialization of bus accesses. > > This task is left to the controller drivers, since this is what they > > tend to optimize (by queuing transfers at the HW level). > > Oh. Will that design decision (localized serialization) not make it > extremely hard to implement muxing (and gating and other stuff that > you need, at least for I2C) that is controller independent? The I²C framework has its own set of locks, and as I said earlier, I'm just implementing an i2c_adapter, so every I²C transfer will go through the standard I²C stack before being passed to the I3C framework (and then the I3C controller driver). I3C transfers going on the bus should have no impact here since they don't change the I2C mux state. Regarding the fact that we might need a lock to get exclusive access on the I3C bus, it might become true at some point, but it clearly isn't right now. So instead of adding complexity for something we don't need, I decided to only add the locking that I knew was required. Anyway, we're making assumptions on things we're not able to test or even validate based on a specification for a potential I3C mux, so I really think we should wait for the problem to actually appear instead of trying to fix it now. Also, I have the feeling that I3C muxes will be closer to routers/bridges in their approach (the master bus would encapsulate frames that it wants to send on the sub-bus and the bridge would extract those frames and forward them). Also, dummy muxing is a hard thing to do in I3C because of the IBI stuff. If you really mux the bus in HW, you'll lose the ability of receiving IBIs on the non-active buses. Of course, these are just speculations, but I don't see how dummy I3C muxes could work. > > >> When muxes are thrown into the mix, you find yourself needing to > >> do more than the "real" transfer with some lock held. In I2C there > >> is an unlocked __i2c_transfer, and locking/unlocking is exposed. > >> Muxes typically grab the lock, set the mux in the appropriate > >> position, do an unlocked __i2c_transfer, optionally sets the mux > >> in some default position and then lets go of the lock. See e.g. > >> i2c/muxes/i2c-mux-pca954x.c > >> > >> However, that is the simple case. There are also muxes that are > >> controlled with GPIO pins, and that gets hairy if the GPIO pins > >> are controlled from the same I2C bus that is muxed. The GPIO > >> driver would have to know that some GPIO pins need to use unlocked > >> I2C transfers for that to work with the above locking scheme. And > >> no GPIO driver does not want to know about that at all. I.e. you > >> have two fundamentally different requirement depending on if the > >> GPIO pins controlling the mux are controlled using the muxed bus > >> or if the pins are controlled in some completely unrelated way. > >> The latter case is probably the normal case, with the GPIO mux > >> controlled directly from some SoC pins. In the latter case you > >> also want to prevent any transfer on the bus while the GPIO pins > >> for the mux are changing, i.e. the total opposite of the former > >> case. It gets really really hairy if you have multiple levels > >> of muxes... > >> > >> There are a some old drivers (e.g. i2c/busses/i2c-amd756-s4882.c) > >> that handles this by simply bypassing the GPIO subsystem, but > >> that is not really an option if some pins are used to mux the > >> I2C bus while some pins are used for other things. > > > > I see. > > > >> > >> I don't know if this affects I3C before muxes are added, but I > >> suspect muxes will happen sooner rather than later, since the > >> spec mentions that the bus only support 11 devices maximum. 11 > >> don't seem like a lot, and it seems likely that there will be > >> a need to have more devices somehow... > > > > I can't tell, and that's the whole problem here. How can you design a > > solution for something that does not exist yet? Fixing the I2C muxing > > logic, if it needs to be, is something I can understand. But how can you > > envision what I3C muxes (if they ever exist) will look like? > > Yeah, you have a point there. One problem is that I don't even see > how the situation can be unified for I2C... That's a problem, indeed. I guess there was discussion with the I²C maintainers in the past, what was the outcome? > > >> > >> But just maybe, in order to not run into the above situation, it > >> needs to be handled right from the start with preparatory and > >> cleanup stages of each transfers, or something? > > > > How about applying this approach to I2C first and see how it flies. > > Changing the I3C framework afterwards (when I3C muxes come in) > > shouldn't be that complicated. > > That would require more thinking first, and I fear that the overhaul > will be bigger than what is called for given the rather fringe cases > that cause problems. Ok. > > Note that I'm not trying to block I3C, I'm just trying to trigger > some thinking before the fact... Just because the I3C framework is upstreamed doesn't mean everything is set in stone. If we need to change the locking-scheme because a new use case forces us to rework it, I'll be the first one to push for it, but right now, I don't see what we can do given the information we have. Regards, Boris -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > Add a driver for Cadence I3C GPIO expander. > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > + scratchbuf = kmalloc(sizeof(*scratchbuf) * 2, GFP_KERNEL); kmalloc_array() ? > + ret = i3c_device_do_priv_xfers(gpioc->i3cdev, xfers, > + ARRAY_SIZE(xfers)); One line? > +static void cdns_i3c_gpio_set_multiple(struct gpio_chip *g, > + unsigned long *mask, > + unsigned long *bits) > +{ > + struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g); > + u8 newovr; > + int ret; > + > + newovr = (gpioc->ovr & ~(*mask)) | (*bits & *mask); > + if (newovr == gpioc->ovr) > + return; > + > + ret = cdns_i3c_gpio_write_reg(gpioc, OVR, newovr); > + if (!ret) > + gpioc->ovr = newovr; You don't change mask here, why do you need a pointer to it? > +} > +static int cdns_i3c_gpio_get_multiple(struct gpio_chip *g, > + unsigned long *mask, > + unsigned long *bits) > +{ > + struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g); > + int ret; > + u8 ivr; > + > + ret = cdns_i3c_gpio_read_reg(gpioc, IVR, &ivr); > + if (ret) > + return ret; > + > + *bits = ivr & *mask & gpioc->dir; > + *bits |= gpioc->ovr & *mask & ~gpioc->dir; Ditto. > + > + return 0; > +} > +static void cdns_i3c_gpio_ibi_handler(struct i3c_device *i3cdev, > + const struct i3c_ibi_payload *payload) > +{ > + struct cdns_i3c_gpio *gpioc = i3cdev_get_drvdata(i3cdev); > + u8 isr = 0; Perhaps you need to check the result of _read_reg() below instead of dummy assignment? > + int i; > + > + cdns_i3c_gpio_read_reg(gpioc, ISR, &isr); > + for (i = 0; i < 8; i++) { > + unsigned int irq; > + > + if (!(BIT(i) & isr & gpioc->imr)) > + continue; for_each_set_bit() ? > + > + irq = irq_find_mapping(gpioc->gpioc.irq.domain, i); > + handle_nested_irq(irq); > + } > +} > +static const struct i3c_device_id cdns_i3c_gpio_ids[] = { > + I3C_DEVICE(0x1c9, 0x0, NULL), > + { /* sentinel */ }, Slightly better without comma. > +}; > +MODULE_DEVICE_TABLE(i3c, cdns_i3c_gpio_ids);
Hi Andy, On Tue, 26 Jun 2018 22:07:03 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon > <boris.brezillon@bootlin.com> wrote: > > Add a driver for Cadence I3C GPIO expander. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > + scratchbuf = kmalloc(sizeof(*scratchbuf) * 2, GFP_KERNEL); > > kmalloc_array() ? > > > + ret = i3c_device_do_priv_xfers(gpioc->i3cdev, xfers, > > + ARRAY_SIZE(xfers)); > > One line? > > > > +static void cdns_i3c_gpio_set_multiple(struct gpio_chip *g, > > + unsigned long *mask, > > + unsigned long *bits) > > +{ > > + struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g); > > + u8 newovr; > > + int ret; > > + > > + newovr = (gpioc->ovr & ~(*mask)) | (*bits & *mask); > > + if (newovr == gpioc->ovr) > > + return; > > + > > + ret = cdns_i3c_gpio_write_reg(gpioc, OVR, newovr); > > + if (!ret) > > + gpioc->ovr = newovr; > > You don't change mask here, why do you need a pointer to it? What are you talking about??!!! This is part of the gpio_chip interface that drivers have to implement. You can't decide that mask should not be a pointer. And if you actually look at the code, you'll probably see that the reason we're passed a pointer here is that the GPIO chip might expose more GPIOs than can be represented by an unsigned long, hence the array of unsigned long. I'll say it here because this is not the first time I'm pissed off by one of your review. You seem to review everything that is posted on the LKML, and most of the time your reviews are superficial (focused on tiny details or coding style issues). Don't you have better things to do? I mean, I'm fine when I get such comments from people that also do useful comments, but yours are most of the time useless and sometime even wrong because you didn't time to look at the code in details. > > > +} > > > +static int cdns_i3c_gpio_get_multiple(struct gpio_chip *g, > > + unsigned long *mask, > > + unsigned long *bits) > > +{ > > + struct cdns_i3c_gpio *gpioc = gpioc_to_cdns_gpioc(g); > > + int ret; > > + u8 ivr; > > + > > + ret = cdns_i3c_gpio_read_reg(gpioc, IVR, &ivr); > > + if (ret) > > + return ret; > > + > > + *bits = ivr & *mask & gpioc->dir; > > + *bits |= gpioc->ovr & *mask & ~gpioc->dir; > > Ditto. > > > + > > + return 0; > > +} > > > +static void cdns_i3c_gpio_ibi_handler(struct i3c_device *i3cdev, > > + const struct i3c_ibi_payload *payload) > > +{ > > + struct cdns_i3c_gpio *gpioc = i3cdev_get_drvdata(i3cdev); > > > + u8 isr = 0; > > Perhaps you need to check the result of _read_reg() below instead of > dummy assignment? > > > + int i; > > + > > + cdns_i3c_gpio_read_reg(gpioc, ISR, &isr); > > > + for (i = 0; i < 8; i++) { > > + unsigned int irq; > > + > > + if (!(BIT(i) & isr & gpioc->imr)) > > + continue; > > for_each_set_bit() ? > > > + > > + irq = irq_find_mapping(gpioc->gpioc.irq.domain, i); > > + handle_nested_irq(irq); > > + } > > +} > > > +static const struct i3c_device_id cdns_i3c_gpio_ids[] = { > > + I3C_DEVICE(0x1c9, 0x0, NULL), > > > + { /* sentinel */ }, > > Slightly better without comma. > > > +}; > > +MODULE_DEVICE_TABLE(i3c, cdns_i3c_gpio_ids); > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 26, 2018 at 10:56 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Tue, 26 Jun 2018 22:07:03 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon >> <boris.brezillon@bootlin.com> wrote: >> > Add a driver for Cadence I3C GPIO expander. >> > >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> >> >> > + scratchbuf = kmalloc(sizeof(*scratchbuf) * 2, GFP_KERNEL); >> >> kmalloc_array() ? >> >> > + ret = i3c_device_do_priv_xfers(gpioc->i3cdev, xfers, >> > + ARRAY_SIZE(xfers)); >> >> One line? >> >> You don't change mask here, why do you need a pointer to it? > > What are you talking about??!!! This is part of the gpio_chip interface > that drivers have to implement. You can't decide that mask should not be > a pointer. And if you actually look at the code, you'll probably see > that the reason we're passed a pointer here is that the GPIO chip might > expose more GPIOs than can be represented by an unsigned long, hence > the array of unsigned long. > > I'll say it here because this is not the first time I'm pissed off by > one of your review. Like 'I like offending people, because I think people who get offended should be offended.' ? I'm not that guy, relax. > You seem to review everything that is posted on the LKML, That's not true. > and most of the time your reviews are superficial (focused on tiny > details or coding style issues). Don't you have better things to do? If your code is written using ancient style and / or API it's not good to start with. Isn't it? Otherwise, why we do introduce new internal APIs, for what purpose? On top of that you might find more interesting reviews where I pointed out to a memory leak or other nasty stuff. But of course you prefer not to norice that. I understand you. > I mean, I'm fine when I get such comments from people that also do > useful comments, but yours are most of the time useless and sometime > even wrong because you didn't time to look at the code in details. Calm down, please. You would simple ignore them. Your choice. But okay, I would try to avoid your stuff to make you happier. Sorry for disturbing. >> Ditto. In these two comments I'm indeed wrong.
Hi, On 06/22/2018 03:49 AM, Boris Brezillon wrote: > Add the I3C documentation describing the protocol, the master driver API > and the device driver API. > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- > Changes in v5: > - Remove useless conf.py file > - Add SPDX headers > > Changes in v2: > - Moved out of patch "i3c: Add core I3C infrastructure" > - Add link to the I3C spec > - Move rst files in Documentation/driver-api/i3c/ > --- > Documentation/driver-api/i3c/device-driver-api.rst | 9 + > Documentation/driver-api/i3c/index.rst | 11 ++ > Documentation/driver-api/i3c/master-driver-api.rst | 10 + > Documentation/driver-api/i3c/protocol.rst | 203 +++++++++++++++++++++ > Documentation/driver-api/index.rst | 1 + > 5 files changed, 234 insertions(+) > create mode 100644 Documentation/driver-api/i3c/device-driver-api.rst > create mode 100644 Documentation/driver-api/i3c/index.rst > create mode 100644 Documentation/driver-api/i3c/master-driver-api.rst > create mode 100644 Documentation/driver-api/i3c/protocol.rst > diff --git a/Documentation/driver-api/i3c/protocol.rst b/Documentation/driver-api/i3c/protocol.rst > new file mode 100644 > index 000000000000..a768afa7f12a > --- /dev/null > +++ b/Documentation/driver-api/i3c/protocol.rst > @@ -0,0 +1,203 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +============ > +I3C protocol > +============ > + > +Disclaimer > +========== > + > +This chapter will focus on aspects that matter to software developers. For > +everything hardware related (like how things are transmitted on the bus, how > +collisions are prevented, ...) please have a look at the I3C specification. > + > +This document is just a brief introduction to the I3C protocol and the concepts > +it brings on the table. If you need more information, please refer to the MIPI brings to the table. > +I3C specification (can be downloaded here > +http://resources.mipi.org/mipi-i3c-v1-download). > + > +Introduction > +============ > + > +The I3C (pronounced 'eye-three-see') is a MIPI standardized protocol designed > +to overcome I2C limitations (limited speed, external signals needed for > +interrupts, no automatic detection of the devices connected to the bus, ...) > +while remaining power-efficient. > + > +I3C Bus > +======= > + > +An I3C bus is made of several I3C devices and possibly some I2C devices as > +well, but let's focus on I3C devices for now. > + > +An I3C device on the I3C bus can have one of the following roles: > + > +* Master: the device is driving the bus. It's the one in charge of initiating > + transactions or deciding who is allowed to talk on the bus (slave generated > + events are possible in I3C, see below). > +* Slave: the device acts as a slave, and is not able to send frames to another > + slave on the bus. The device can still send events to the master on > + its own initiative if the master allowed it. > + > +I3C is a multi-master protocol, so there might be several masters on a bus, > +though only one device can act as a master at a given time. In order to gain > +bus ownership, a master has to follow a specific procedure. > + > +Each device on the I3C bus has to be assigned a dynamic address to be able to > +communicate. Until this is done, the device should only respond to a limited > +set of commands. If it has a static address (also called legacy I2C address), > +the device can reply to I2C transfers. > + > +In addition to these per-device addresses, the protocol defines a broadcast > +address in order to address all devices on the bus. > + > +Once a dynamic address has been assigned to a device, this address will be used > +for any direct communication with the device. Note that even after being > +assigned a dynamic address, the device should still process broadcast messages. > + > +I3C Device discovery > +==================== > + > +The I3C protocol defines a mechanism to automatically discover devices present > +on the bus, their capabilities and the functionalities they provide. In this > +regard I3C is closer to a discoverable bus like USB than it is to I2C or SPI. > + > +The discovery mechanism is called DAA (Dynamic Address Assignment), because it > +not only discovers devices but also assigns them a dynamic address. > + > +During DAA, each I3C device reports 3 important things: > + > +* BCR: Bus Characteristic Register. This 8-bit register describes the device bus > + related capabilities > +* DCR: Device Characteristic Register. This 8-bit register describes the > + functionalities provided by the device > +* Provisional ID: A 48-bit unique identifier. On a given bus there should be no > + Provisional ID collision, otherwise the discovery mechanism may fail. > + > +I3C slave events > +================ > + > +The I3C protocol allows slaves to generate events on their own, and thus allows > +them to take temporary control of the bus. > + > +This mechanism is called IBI for In Band Interrupts, and as stated in the name, > +it allows devices to generate interrupts without requiring an external signal. > + > +During DAA, each device on the bus has been assigned an address, and this > +address will serve as a priority identifier to determine who wins if 2 different > +devices are generating an interrupt at the same moment on the bus (the lower the > +dynamic address the higher the priority). > + > +Masters are allowed to inhibit interrupts if they want to. This inhibition > +request can be broadcasted (applies to all devices) or sent to a specific can be broadcast > +device. > + > +I3C Hot-Join > +============ > + > +The Hot-Join mechanism is similart to USB hotplug. This mechanism allows similar > +slaves to join the bus after it has been initialized by the master. > + > +This covers the following use cases: > + > +* the device is not powered when the bus is probed > +* the device is hotplugged on the bus through an extension board > + > +This mechanism is relying on slave events to inform the master that a new > +device joined the bus and is waiting for a dynamic address. > + > +The master is then free to address the request as it wishes: ignore it or > +assign a dynamic address to the slave. > + > +I3C transfer types > +================== > + > +If you omit SMBus (which is just a standardization on how to access registers > +exposed by I2C devices), I2C has only one transfer type. > + > +I3C defines 3 different classes of transfer in addition to I2C transfers which > +are here for backward compatibility with I2C devices. > + > +I3C CCC commands > +---------------- > + > +CCC (Common Command Code) commands are meant to be used for anything that is > +related to bus management and all features that are common to a set of devices. > + > +CCC commands contain an 8-bit CCC id describing the command that is executed. preferably s/id/ID/ > +The MSB of this id specifies whether this is a broadcast command (bit7 = 0) or a ditto, s/id/ID/ > +unicast one (bit7 = 1). > + > +The command ID can be followed by a payload. Depending on the command, this > +payload is either sent by the master sending the command (write CCC command), > +or sent by the slave receiving the command (read CCC command). Of course, read > +accesses only apply to unicast commands. > +Note that, when sending a CCC command to a specific device, the device address > +is passed in the first byte of the payload. > + > +The payload length is not explicitly passed on the bus, and should be extracted > +from the CCC id. s/id/ID/ > + > +Note that vendors can use a dedicated range of CCC ids for their own commands IDs > +(0x61-0x7f and 0xe0-0xef). > + > +I3C Private SDR transfers > +------------------------- > + > +Private SDR (Single Data Rate) transfers should be used for anything that is > +device specific and does not require high transfer speed. > + > +It is the equivalent of I2C transfers but in the I3C world. Each transfer is > +passed the device address (dynamic address assigned during DAA), a payload > +and a direction. > + > +The only difference with I2C is that the transfer is much faster (typical SCL what is SCL? It's not used anywhere else in this doc. > +frequency is 12.5MHz). > + > +I3C HDR commands > +---------------- > + > +HDR commands should be used for anything that is device specific and requires > +high transfer speed. > + > +The first thing attached to an HDR command is the HDR mode. There are currently > +3 different modes defined by the I3C specification (refer to the specification > +for more details): > + > +* HDR-DDR: Double Data Rate mode > +* HDR-TSP: Ternary Symbol Pure. Only usable on busses with no I2C devices > +* HDR-TSL: Ternary Symbol Legacy. Usable on busses with I2C devices > + > +When sending an HDR command, the whole bus has to enter HDR mode, which is done > +using a broadcast CCC command. > +Once the bus has entered a specific HDR mode, the master sends the HDR command. > +An HDR command is made of: > + > +* one 16-bits command word > +* N 16-bits data words I supposed the I3C spec will tell me the byte order of these words on the bus? or this doc could tell us here. > + > +Those words may be wrapped with specific preambles/post-ambles which depend on > +the chosen HDR mode and are detailed here (see the specification for more > +details). > + > +The 16-bits command word is made of: > + > +* bit[15]: direction bit, read is 1 write is 0 read is 1, write is 0 > +* bit[14:8]: command code. Identifies the command being executed, the amount of > + data words and their meaning > +* bit[7:1]: I3C address of the device this command is addressed to > +* bit[0]: reserved/parity-bit > + > +Backward compatibility with I2C devices > +======================================= > + > +The I3C protocol has been designed to be backward compatible with I2C devices. > +This backward compatibility allows one to connect a mix of I2C and I3C devices > +on the same bus, though, in order to be really efficient, I2C devices should > +be equipped with 50 ns spike filters. > + > +I2C devices can't be discovered like I3C ones and have to be statically > +declared. In order to let the master know what these devices are capable of > +(both in terms of bus related limitations and functionalities), the software > +has to provide some information, which is done through the LVR (Legacy I2C > +Virtual Register). and you can add (if you want to): Reviewed-by: Randy Dunlap <rdunlap@infradead.org> thanks,
On Tue, 26 Jun 2018 23:44:36 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Jun 26, 2018 at 10:56 PM, Boris Brezillon > <boris.brezillon@bootlin.com> wrote: > > On Tue, 26 Jun 2018 22:07:03 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >> On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon > >> <boris.brezillon@bootlin.com> wrote: > >> > Add a driver for Cadence I3C GPIO expander. > >> > > >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > >> > >> > + scratchbuf = kmalloc(sizeof(*scratchbuf) * 2, GFP_KERNEL); > >> > >> kmalloc_array() ? > >> > >> > + ret = i3c_device_do_priv_xfers(gpioc->i3cdev, xfers, > >> > + ARRAY_SIZE(xfers)); > >> > >> One line? > >> > > >> You don't change mask here, why do you need a pointer to it? > > > > What are you talking about??!!! This is part of the gpio_chip interface > > that drivers have to implement. You can't decide that mask should not be > > a pointer. And if you actually look at the code, you'll probably see > > that the reason we're passed a pointer here is that the GPIO chip might > > expose more GPIOs than can be represented by an unsigned long, hence > > the array of unsigned long. > > > > > I'll say it here because this is not the first time I'm pissed off by > > one of your review. > > Like 'I like offending people, because I think people who get offended > should be offended.' ? > I'm not that guy, relax. I'm not offended, just annoyed, and not because I have things to change in my patchset (I'm used to that and that's something I comply with most of the time), but because the only reviews I had from you were of this kind (nitpicking on minor stuff). > > > You seem to review everything that is posted on the LKML, > > That's not true. > > > and most of the time your reviews are superficial (focused on tiny > > details or coding style issues). Don't you have better things to do? > > If your code is written using ancient style and / or API it's not good > to start with. > Isn't it? Otherwise, why we do introduce new internal APIs, for what purpose? Come on! - kzalloc() vs kmalloc_array() - for (i = 0; i < n, i++) { if (BIT(x) & var) ... } vs for_each_bit_set() (which is not even applicable here because I'm not manipulating an unsigned long) Where do you see ancient style APIs here? And that's not even the problem. I'm okay to fix those things, but you only ever do these kind of reviews, and sometime you even act like you know better than the developer or the maintainer how the code should be organized without even digging enough to have a clue (your comment on the fact that mask should not be a pointer is a good example of such situations). > > On top of that you might find more interesting reviews where I pointed > out to a memory leak or other nasty stuff. But of course you prefer > not to norice that. Yes, sometime you have valid point, but it gets lost in all the noise. > I understand you. > > > I mean, I'm fine when I get such comments from people that also do > > useful comments, but yours are most of the time useless and sometime > > even wrong because you didn't time to look at the code in details. > > Calm down, please. Why? Because I say you didn't look at the code in details? Isn't it true? Did you look at what I3C is, how it works or how the I3C framework is architectured? Because that's the kind of reviews I'd love to have on this series. > You would simple ignore them. Your choice. That's not my point. My point is, maybe you should do less reviews but spend more time on each of them so you don't just scratch the surface with comments I could get from a tool like checkpatch. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Randy, On Tue, 26 Jun 2018 14:07:49 -0700 Randy Dunlap <rdunlap@infradead.org> wrote: > > + > > +I3C Private SDR transfers > > +------------------------- > > + > > +Private SDR (Single Data Rate) transfers should be used for anything that is > > +device specific and does not require high transfer speed. > > + > > +It is the equivalent of I2C transfers but in the I3C world. Each transfer is > > +passed the device address (dynamic address assigned during DAA), a payload > > +and a direction. > > + > > +The only difference with I2C is that the transfer is much faster (typical SCL > > what is SCL? It's not used anywhere else in this doc. It's an acronym used by I²C, it means Serial Clock Line. I'll just replace that by "typical clock frequency is 12.5MHz". > > > +frequency is 12.5MHz). > > + > > +I3C HDR commands > > +---------------- > > + > > +HDR commands should be used for anything that is device specific and requires > > +high transfer speed. > > + > > +The first thing attached to an HDR command is the HDR mode. There are currently > > +3 different modes defined by the I3C specification (refer to the specification > > +for more details): > > + > > +* HDR-DDR: Double Data Rate mode > > +* HDR-TSP: Ternary Symbol Pure. Only usable on busses with no I2C devices > > +* HDR-TSL: Ternary Symbol Legacy. Usable on busses with I2C devices > > + > > +When sending an HDR command, the whole bus has to enter HDR mode, which is done > > +using a broadcast CCC command. > > +Once the bus has entered a specific HDR mode, the master sends the HDR command. > > +An HDR command is made of: > > + > > +* one 16-bits command word > > +* N 16-bits data words > > I supposed the I3C spec will tell me the byte order of these words on the bus? > or this doc could tell us here. It's big endian. I'll make it clear in this doc. I'll also fix all the other mistakes you pointed out. > > and you can add (if you want to): > Reviewed-by: Randy Dunlap <rdunlap@infradead.org> I'll definitely add your R-b. Thanks for the review. Boris -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 27, 2018 at 12:46 AM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Tue, 26 Jun 2018 23:44:36 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> On Tue, Jun 26, 2018 at 10:56 PM, Boris Brezillon >> <boris.brezillon@bootlin.com> wrote: >> > On Tue, 26 Jun 2018 22:07:03 +0300 >> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> >> On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon >> >> <boris.brezillon@bootlin.com> wrote: >> > I'll say it here because this is not the first time I'm pissed off by >> > one of your review. >> >> Like 'I like offending people, because I think people who get offended >> should be offended.' ? >> I'm not that guy, relax. > > I'm not offended, just annoyed, and not because I have things to change > in my patchset (I'm used to that and that's something I comply with > most of the time), but because the only reviews I had from you were of > this kind (nitpicking on minor stuff). OK, point taken. >> > and most of the time your reviews are superficial (focused on tiny >> > details or coding style issues). Don't you have better things to do? >> >> If your code is written using ancient style and / or API it's not good >> to start with. >> Isn't it? Otherwise, why we do introduce new internal APIs, for what purpose? > > Come on! > > - kzalloc() vs kmalloc_array() > - for (i = 0; i < n, i++) { if (BIT(x) & var) ... } > vs > for_each_bit_set() (which is not even applicable here because I'm > not manipulating an unsigned long) > > Where do you see ancient style APIs here? Both. kmalloc_array() and for_each_set_bit() were introduced quite long time ago. On top of that you are open coding, if I'm not mistaken, cpu_to_be32() and be32_to_cpu(). ...and more I believe. > And that's not even the problem. I'm okay to fix those things, but you > only ever do these kind of reviews, and sometime you even act like you > know better than the developer or the maintainer how the code should be > organized without even digging enough to have a clue (your comment on > the fact that mask should not be a pointer is a good example of such > situations). "sometime". Yes, I admit my mistakes. >> On top of that you might find more interesting reviews where I pointed >> out to a memory leak or other nasty stuff. But of course you prefer >> not to norice that. > > Yes, sometime you have valid point, but it gets lost in all the noise. Btw, you forgot to call of_node_put() on error path in one case. Did I again missed the details? >> > I mean, I'm fine when I get such comments from people that also do >> > useful comments, but yours are most of the time useless and sometime >> > even wrong because you didn't time to look at the code in details. >> >> Calm down, please. > > Why? Because I say you didn't look at the code in details? Isn't it > true? Did you look at what I3C is, how it works or how the I3C framework > is architectured? Because that's the kind of reviews I'd love to have on > this series. You got me. I have a copy of the spec, this require a bit of time to go through. For now I can offer you to consider IBI implementation using IRQ chip framework. It would give few advantages (like we do this for GPIO), such as IRQ statistics or wake up capabilities. >> You would simple ignore them. Your choice. > > That's not my point. My point is, maybe you should do less reviews but > spend more time on each of them Good point. > so you don't just scratch the surface > with comments I could get from a tool like checkpatch. Perhaps you should run checkpatch yourself then?
Hi Andy, On Wed, 27 Jun 2018 20:53:51 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Jun 27, 2018 at 12:46 AM, Boris Brezillon > <boris.brezillon@bootlin.com> wrote: > > On Tue, 26 Jun 2018 23:44:36 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >> On Tue, Jun 26, 2018 at 10:56 PM, Boris Brezillon > >> <boris.brezillon@bootlin.com> wrote: > >> > On Tue, 26 Jun 2018 22:07:03 +0300 > >> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >> >> On Fri, Jun 22, 2018 at 1:49 PM, Boris Brezillon > >> >> <boris.brezillon@bootlin.com> wrote: > > >> > I'll say it here because this is not the first time I'm pissed off by > >> > one of your review. > >> > >> Like 'I like offending people, because I think people who get offended > >> should be offended.' ? > >> I'm not that guy, relax. > > > > I'm not offended, just annoyed, and not because I have things to change > > in my patchset (I'm used to that and that's something I comply with > > most of the time), but because the only reviews I had from you were of > > this kind (nitpicking on minor stuff). > > OK, point taken. > > >> > and most of the time your reviews are superficial (focused on tiny > >> > details or coding style issues). Don't you have better things to do? > >> > >> If your code is written using ancient style and / or API it's not good > >> to start with. > >> Isn't it? Otherwise, why we do introduce new internal APIs, for what purpose? > > > > Come on! > > > > - kzalloc() vs kmalloc_array() > > - for (i = 0; i < n, i++) { if (BIT(x) & var) ... } > > vs > > for_each_bit_set() (which is not even applicable here because I'm > > not manipulating an unsigned long) > > > > Where do you see ancient style APIs here? > > Both. kmalloc_array() and for_each_set_bit() were introduced quite > long time ago. I mean, kzalloc() is not deprecated AFAIK and I don't really see the benefit of using kmalloc_array(), but if that makes you happy, let's go for kmalloc_array(). For for_each_bit_set() it would require changing the type of isr + having a temporary variable to get the reg value into an u8. Again, I can do the change, but I don't really see how it improves the code. > > On top of that you are open coding, if I'm not mistaken, cpu_to_be32() > and be32_to_cpu(). Care to point where? > ...and more I believe. Care to point what? > >> On top of that you might find more interesting reviews where I pointed > >> out to a memory leak or other nasty stuff. But of course you prefer > >> not to norice that. > > > > Yes, sometime you have valid point, but it gets lost in all the noise. > > Btw, you forgot to call of_node_put() on error path in one case. In this driver or another patch of the series? I don't see any of_node_get() call in this GPIO driver, but maybe it's something done in one of the GPIO helper. > Did I > again missed the details? > > >> > I mean, I'm fine when I get such comments from people that also do > >> > useful comments, but yours are most of the time useless and sometime > >> > even wrong because you didn't time to look at the code in details. > >> > >> Calm down, please. > > > > Why? Because I say you didn't look at the code in details? Isn't it > > true? Did you look at what I3C is, how it works or how the I3C framework > > is architectured? Because that's the kind of reviews I'd love to have on > > this series. > > You got me. > I have a copy of the spec, this require a bit of time to go through. Cool! > > For now I can offer you to consider IBI implementation using IRQ chip framework. > It would give few advantages (like we do this for GPIO), such as IRQ > statistics or wake up capabilities. Just pasting the comment I made in my cover letter: " Main changes between the initial RFC and this v2 are: - Add a generic infrastructure to support IBIs. It's worth mentioning that I tried exposing IBIs as a regular IRQs, but after several attempts and a discussion with Mark Zyngier, it appeared that it was not really fitting in the Linux IRQ model (the fact that you have payload attached to IBIs, the fact that most of the time an IBI will generate a transfer on the bus which has to be done in an atomic context, ...) The counterpart of this decision is the latency induced by the workqueue approach, but since I don't have real use cases, I don't know if this can be a problem or not. " To sum-up, yes I tried implementing what you suggest, and it ended being messy for 2 main reasons: 1/ IBIs have payload attached to them, and exposing IBIs are regular IRQs meant adding a payload queuing mechanism to the i3c_device so that the I3C device driver could dequeue each payload independently. Clearly not impossible to deal with, but conceptually far from the concept of IRQs we have in Linux. 2/ The I3C APIs are meant to be used in non-atomic context, but if you expose IBIs are regular IRQs, the irqchip will have to mask/unmask the IRQs from an atomic context, and masking/unmasking IRQs almost always implies doing a CCC or priv SDR transfer. Plus, most of the time you'll have to retrieve extra info from the IRQ handler, which again means sending frames on the I3C bus. We could make that work by either supporting async transfers or having the irq chip handle the IRQ handlers from a thread. But both options add extra complexity for no real benefit given that IBIs are already not exactly fitting in the Linux IRQ model. The discussion I had with Mark Zyngier finished convincing me that IBIs would be better exposed with their own API. > > >> You would simple ignore them. Your choice. > > > > That's not my point. My point is, maybe you should do less reviews but > > spend more time on each of them > > Good point. > > > so you don't just scratch the surface > > with comments I could get from a tool like checkpatch. > > Perhaps you should run checkpatch yourself then? > I do run checkpatch --strict and fix most of the thing reported except those hurting readability. I don't remember seeing checkpatch complain about kzalloc() usage, and I guess it's not smart enough to detect that for_each_bit_set() can be used to replace the "for() if (BIT(x) & val)" pattern. Anyway, thanks for having a look at the I3C spec and starting the discussion on IBIs. I guess I owe you an apology. Regards, Boris -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 22, 2018 at 12:49 PM Boris Brezillon <boris.brezillon@bootlin.com> wrote: > Add a driver for Cadence I3C GPIO expander. > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> Acked-by: Linus Walleij <linus.walleij@linaro.org> If you need to merge this through the subsystem merge. I see there was some heat in this discussion between you and Andy, and it seems to be resolved. Which is good because you are both senior contributors that I value a lot. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2018-06-27 at 21:36 +0200, Boris Brezillon wrote: > I mean, kzalloc() is not deprecated AFAIK and I don't really see the > benefit of using kmalloc_array(), but if that makes you happy, let's go > for kmalloc_array(). kcalloc > I do run checkpatch --strict and fix most of the thing reported except > those hurting readability. I don't remember seeing checkpatch complain > about kzalloc() usage, and I guess it's not smart enough to detect that > for_each_bit_set() can be used to replace the "for() if (BIT(x) & val)" > pattern. That would not an appropriate conversion suggestion in any case. coccinelle could at least look at whether or not x is allocated as a bitmap via DECLARE_BITMAP or bitmap_alloc -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 28, 2018 at 1:54 AM, Joe Perches <joe@perches.com> wrote: > On Wed, 2018-06-27 at 21:36 +0200, Boris Brezillon wrote: >> I mean, kzalloc() is not deprecated AFAIK and I don't really see the >> benefit of using kmalloc_array(), but if that makes you happy, let's go >> for kmalloc_array(). > > kcalloc IIRC in the original code kmalloc(x*y) is used. >> I do run checkpatch --strict and fix most of the thing reported except >> those hurting readability. I don't remember seeing checkpatch complain >> about kzalloc() usage, and I guess it's not smart enough to detect that >> for_each_bit_set() can be used to replace the "for() if (BIT(x) & val)" >> pattern. > > That would not an appropriate conversion suggestion in any case. > coccinelle could at least look at whether or not x is allocated > as a bitmap via DECLARE_BITMAP or bitmap_alloc Huh?! bitmap operations are working against unsigned long *. one long is also a bitmap. So, that coccinelle scripts must be fixed accordingly.
On Thu, 2018-06-28 at 03:00 +0300, Andy Shevchenko wrote: > bitmap operations are working against unsigned long *. one long is > also a bitmap. Should only be so if declared via DECLARE_BITMAP > So, that coccinelle scripts must be fixed accordingly. We disagree about a non-existent script. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I see there was some heat in this discussion between you > and Andy, and it seems to be resolved. Which is good > because you are both senior contributors that I value > a lot. +1
Hi Boris, On 22-06-2018 11:49, Boris Brezillon wrote: > +static int of_i3c_master_add_i3c_dev(struct i3c_master_controller *master, > + struct device_node *node, u32 *reg) > +{ > + struct i3c_device_info info = { }; > + enum i3c_addr_slot_status addrstatus; > + struct i3c_device *i3cdev; > + u32 init_dyn_addr = 0; > + > + if (reg[0]) { > + if (reg[0] > I3C_MAX_ADDR) > + return -EINVAL; > + > + addrstatus = i3c_bus_get_addr_slot_status(master->bus, reg[0]); > + if (addrstatus != I3C_ADDR_SLOT_FREE) > + return -EINVAL; > + } > + > + info.static_addr = reg[0]; > + > + if (!of_property_read_u32(node, "assigned-address", &init_dyn_addr)) { > + if (init_dyn_addr > I3C_MAX_ADDR) > + return -EINVAL; > + > + addrstatus = i3c_bus_get_addr_slot_status(master->bus, > + init_dyn_addr); > + if (addrstatus != I3C_ADDR_SLOT_FREE) > + return -EINVAL; > + } > + > + info.pid = ((u64)reg[1] << 32) | reg[2]; > + > + if ((info.pid & GENMASK_ULL(63, 48)) || > + I3C_PID_RND_LOWER_32BITS(info.pid)) > + return -EINVAL; > + > + i3cdev = i3c_master_alloc_i3c_dev(master, &info, &i3c_device_type); > + if (IS_ERR(i3cdev)) > + return PTR_ERR(i3cdev); > + > + i3cdev->init_dyn_addr = init_dyn_addr; > + i3cdev->dev.of_node = node; > + list_add_tail(&i3cdev->common.node, &master->bus->devs.i3c); > + > + return 0; > +} > + I'm writing the driver for the Synopsys master and but now I getting an issue. I use the "slot" of the device to do all transfers, something like you use in DAA. I using the master_priv to save the "slot" per device but the problem is when I call the i3c_master_add_i3c_dev_locked() to retrieve the info I don't have it yet. From my analysis this can be solve with: - send PID, BCR and DCR when I call i3c_master_add_i3c_dev_locked() or similar function. - Pre-allocate an i3c_device -> attach it (slot data goes to master_priv) -> retrieve info -> if there is already an i3c_device with same PID destroy the pre-allocated one. - Replace the info.dyn_address with a structure with dyn_address and slot and use it in CCC structure. This is something that will need to be supported for I3C HCI spec too. Do you have any suggestion? Best regards, Vitor Soares -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Vitor, On Thu, 28 Jun 2018 16:38:56 +0100 vitor <Vitor.Soares@synopsys.com> wrote: > Hi Boris, > > > On 22-06-2018 11:49, Boris Brezillon wrote: > > +static int of_i3c_master_add_i3c_dev(struct i3c_master_controller *master, > > + struct device_node *node, u32 *reg) > > +{ > > + struct i3c_device_info info = { }; > > + enum i3c_addr_slot_status addrstatus; > > + struct i3c_device *i3cdev; > > + u32 init_dyn_addr = 0; > > + > > + if (reg[0]) { > > + if (reg[0] > I3C_MAX_ADDR) > > + return -EINVAL; > > + > > + addrstatus = i3c_bus_get_addr_slot_status(master->bus, reg[0]); > > + if (addrstatus != I3C_ADDR_SLOT_FREE) > > + return -EINVAL; > > + } > > + > > + info.static_addr = reg[0]; > > + > > + if (!of_property_read_u32(node, "assigned-address", &init_dyn_addr)) { > > + if (init_dyn_addr > I3C_MAX_ADDR) > > + return -EINVAL; > > + > > + addrstatus = i3c_bus_get_addr_slot_status(master->bus, > > + init_dyn_addr); > > + if (addrstatus != I3C_ADDR_SLOT_FREE) > > + return -EINVAL; > > + } > > + > > + info.pid = ((u64)reg[1] << 32) | reg[2]; > > + > > + if ((info.pid & GENMASK_ULL(63, 48)) || > > + I3C_PID_RND_LOWER_32BITS(info.pid)) > > + return -EINVAL; > > + > > + i3cdev = i3c_master_alloc_i3c_dev(master, &info, &i3c_device_type); > > + if (IS_ERR(i3cdev)) > > + return PTR_ERR(i3cdev); > > + > > + i3cdev->init_dyn_addr = init_dyn_addr; > > + i3cdev->dev.of_node = node; > > + list_add_tail(&i3cdev->common.node, &master->bus->devs.i3c); > > + > > + return 0; > > +} > > + > > I'm writing the driver for the Synopsys master and but now I getting an > issue. > > I use the "slot" of the device to do all transfers, something like you > use in DAA. I using the master_priv to save the "slot" per device but > the problem is when I call the i3c_master_add_i3c_dev_locked() to > retrieve the info I don't have it yet. Hm, I knew that might become a problem at some point. The Cadence IP does not need the slot index because it works with addresses and figure the device slot out of this address, but it looks like others don't go this road. > > From my analysis this can be solve with: > - send PID, BCR and DCR when I call i3c_master_add_i3c_dev_locked() > or similar function. Except these are not the only thing we retrieve before attaching the device. Also, if we go this road, that means we don't have the same path for devices whose dynamic address is assigned through SETDASA, and those that are discovered using DAA. > - Pre-allocate an i3c_device -> attach it (slot data goes to > master_priv) -> retrieve info -> if there is already an i3c_device with > same PID destroy the pre-allocated one. That's the very reason I didn't go this road. It gets messy if we already know this device. This being said, among all the options you list here, this is the one I prefer. Let's see if we can standardize the resource allocation/free process and let ->attach/detach() only take care of the device/bus configuration. > - Replace the info.dyn_address with a structure with dyn_address > and slot and use it in CCC structure. I'd really like to keep the device-slot-id a priv information, because we don't know how other IPs will deal with I3C device resources. > > This is something that will need to be supported for I3C HCI spec too. I agree. > Do you have any suggestion? I'll try to come up with something. Need to think a bit more about it. Thanks, Boris -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 22, 2018 at 12:49 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > Add core infrastructure to support I3C in Linux and document it. > > This infrastructure is not complete yet and will be extended over > time. > > There are a few design choices that are worth mentioning because they > impact the way I3C device drivers can interact with their devices.. I just realized I replied to the wrong version when I reviewed v4 of this patch. I think most of my comments still apply, so please see https://lore.kernel.org/lkml/CAK8P3a1aZXf2sQW2mgwJScycKPhdoOAwxRjm5cQG83513uc3fg@mail.gmail.com/T/#u and ignore anything that has changed in the meantime. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 22, 2018 at 12:49 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > Add a driver for Cadence I3C master IP. The driver seems very well-written and shows that the framework got that side of the interface right. Just one thing I noticed: > + > +static void cdns_i3c_master_handle_ibi(struct cdns_i3c_master *master, > + u32 ibir) > + > + for (i = 0; i < IBIR_XFER_BYTES(ibir); i += 4) { > + u32 tmp = readl(master->regs + IBI_DATA_FIFO); > + > + for (j = 0; j < 4 && i + j < dev->ibi->max_payload_len; j++) > + buf[i + j] = tmp >> (j * 8); > + } This seems to be a rather inefficient way to open-code a readsl(). I suppose you need to handle length that is not a multiple of 4, right? Maybe do it like size_t length = IBIR_XFER_BYTES(ibir); readsl(master->regs + IBI_DATA_FIFO, buf, length & ~3); if (length & 3) { u32 tmp = __raw_readl(master->regs + IBI_DATA_FIFO); memcpy(buf + length & ~3, length & 3); } Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
This patch series is a proposal for a new I3C subsystem. This infrastructure is not complete yet and will be extended over time. There are a few design choices that are worth mentioning because they impact the way I3C device drivers can interact with their devices: - all functions used to send I3C/I2C frames must be called in non-atomic context. Mainly done this way to ease implementation, but this is still open to discussion. Please let me know if you think it's worth considering an asynchronous model here - the bus element is a separate object and is not implicitly described by the master (as done in I2C). The reason is that I want to be able to handle multiple master connected to the same bus and visible to Linux. In this situation, we should only have one instance of the device and not one per master, and sharing the bus object would be part of the solution to gracefully handle this case. I'm not sure if we will ever need to deal with multiple masters controlling the same bus and exposed under Linux, but separating the bus and master concept is pretty easy, hence the decision to do it now, just in case we need it some day. The other benefit of separating the bus and master concepts is that master devices appear under the bus directory in sysfs. - I2C backward compatibility has been designed to be transparent to I2C drivers and the I2C subsystem. The I3C master just registers an I2C adapter which creates a new I2C bus. I'd say that, from a representation PoV it's not ideal because what should appear as a single I3C bus exposing I3C and I2C devices here appears as 2 different busses connected to each other through the parenting (the I3C master is the parent of the I2C and I3C busses). On the other hand, I don't see a better solution if we want something that is not invasive. Missing features in this preliminary version: - support for HDR modes (has been removed because of lack of real users) - no support for multi-master and the associated concepts (mastership handover, support for secondary masters, ...) - I2C devices can only be described using DT because this is the only use case I have. However, the framework can easily be extended with ACPI and board info support - I3C slave framework. This has been completely omitted, but shouldn't have a huge impact on the I3C framework because I3C slaves don't see the whole bus, it's only about handling master requests and generating IBIs. Some of the struct, constant and enum definitions could be shared, but most of the I3C slave framework logic will be different Only minor things have changes since v3, you can go check the changelog in each patch for more details. Main changes between v2 and v3 are: - Reworked the DT bindings as suggested by Rob - Reworked the bus initialization step as suggested by Vitor - Added a driver for an I3C GPIO expander Main changes between the initial RFC and this v2 are: - Add a generic infrastructure to support IBIs. It's worth mentioning that I tried exposing IBIs as a regular IRQs, but after several attempts and a discussion with Mark Zyngier, it appeared that it was not really fitting in the Linux IRQ model (the fact that you have payload attached to IBIs, the fact that most of the time an IBI will generate a transfer on the bus which has to be done in an atomic context, ...) The counterpart of this decision is the latency induced by the workqueue approach, but since I don't have real use cases, I don't know if this can be a problem or not. - Add helpers to support Hot Join - Add support for IBIs and Hot Join in Cadence I3C master driver - Address several issues in how I was using the device model Thanks, Boris Boris Brezillon (10): i3c: Add core I3C infrastructure docs: driver-api: Add I3C documentation i3c: Add sysfs ABI spec dt-bindings: i3c: Document core bindings dt-bindings: i3c: Add macros to help fill I3C/I2C device's reg property MAINTAINERS: Add myself as the I3C subsystem maintainer i3c: master: Add driver for Cadence IP dt-bindings: i3c: Document Cadence I3C master bindings gpio: Add a driver for Cadence I3C GPIO expander dt-bindings: gpio: Add bindings for Cadence I3C gpio expander Documentation/ABI/testing/sysfs-bus-i3c | 95 ++ .../devicetree/bindings/gpio/gpio-cdns-i3c.txt | 39 + .../devicetree/bindings/i3c/cdns,i3c-master.txt | 44 + Documentation/devicetree/bindings/i3c/i3c.txt | 140 ++ Documentation/driver-api/i3c/device-driver-api.rst | 9 + Documentation/driver-api/i3c/index.rst | 11 + Documentation/driver-api/i3c/master-driver-api.rst | 10 + Documentation/driver-api/i3c/protocol.rst | 203 +++ Documentation/driver-api/index.rst | 1 + MAINTAINERS | 10 + drivers/Kconfig | 2 + drivers/Makefile | 2 +- drivers/gpio/Kconfig | 11 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-cdns-i3c.c | 411 +++++ drivers/i3c/Kconfig | 24 + drivers/i3c/Makefile | 4 + drivers/i3c/core.c | 620 +++++++ drivers/i3c/device.c | 294 ++++ drivers/i3c/internals.h | 28 + drivers/i3c/master.c | 1722 ++++++++++++++++++++ drivers/i3c/master/Kconfig | 5 + drivers/i3c/master/Makefile | 1 + drivers/i3c/master/i3c-master-cdns.c | 1651 +++++++++++++++++++ include/dt-bindings/i3c/i3c.h | 28 + include/linux/i3c/ccc.h | 382 +++++ include/linux/i3c/device.h | 307 ++++ include/linux/i3c/master.h | 587 +++++++ include/linux/mod_devicetable.h | 17 + 29 files changed, 6658 insertions(+), 1 deletion(-) create mode 100644 Documentation/ABI/testing/sysfs-bus-i3c create mode 100644 Documentation/devicetree/bindings/gpio/gpio-cdns-i3c.txt create mode 100644 Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt create mode 100644 Documentation/devicetree/bindings/i3c/i3c.txt create mode 100644 Documentation/driver-api/i3c/device-driver-api.rst create mode 100644 Documentation/driver-api/i3c/index.rst create mode 100644 Documentation/driver-api/i3c/master-driver-api.rst create mode 100644 Documentation/driver-api/i3c/protocol.rst create mode 100644 drivers/gpio/gpio-cdns-i3c.c create mode 100644 drivers/i3c/Kconfig create mode 100644 drivers/i3c/Makefile create mode 100644 drivers/i3c/core.c create mode 100644 drivers/i3c/device.c create mode 100644 drivers/i3c/internals.h create mode 100644 drivers/i3c/master.c create mode 100644 drivers/i3c/master/Kconfig create mode 100644 drivers/i3c/master/Makefile create mode 100644 drivers/i3c/master/i3c-master-cdns.c create mode 100644 include/dt-bindings/i3c/i3c.h create mode 100644 include/linux/i3c/ccc.h create mode 100644 include/linux/i3c/device.h create mode 100644 include/linux/i3c/master.h