| Message ID | 20180719152930.3715-1-boris.brezillon@bootlin.com |
|---|---|
| Headers | show |
| Series | Add the I3C subsystem | expand |
On Thu, Jul 19, 2018 at 5:29 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > - 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. > > Discussion around the bus/master/dev representation is still ongoing, > with Arnd opting for a simple approach where > * the bus is implicitly represented by the master device > * the master is not represented as a device under the I3C bus > * only remote I3C devices are exposed and possibly duplicated if > several masters controlling the same bus are exposed to the same > Linux instance > and Peter preferring the representation where the bus is a separate > object. IIRC, Wolfram was in favor of the "bus is a separate object" > too. > > If possible, I'd like to close this discussion soon, no matter which > solution is chosen. ... > Missing features in this preliminary version: ... > - no support for multi-master and the associated concepts (mastership > handover, support for secondary masters, ...) Let's try to come to a conclusion to this discussion, this is the main show-stopper for inclusion that I see, as changing the fundamental design would be hard to do once we do it one way or the other, and the structure is exposed to user space. Peter and Wolfram, could you explain what scenario you can see that would require handing over ownership of a device from one i3c master to another i3c master when both are controlled by the same Linux instance? To me this seems like a rather odd scenario, and supporting it properly requires significant complexity once we try to support the dynamic handover of the bus between two of our own masters. It seems more likely to me that we could deal with this case by requiring either that each bus is controlled by at most one master device in Linux, or at least that when we have two masters on the same bus that they each control a non-overlapping set of slave devices. Either way we'd be able to represent the structure as a normal tree in the firmware (DT or ACPI) as well as in sysfs. 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 2018-07-20 10:52, Arnd Bergmann wrote: > On Thu, Jul 19, 2018 at 5:29 PM, Boris Brezillon > <boris.brezillon@bootlin.com> wrote: > >> - 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. >> >> Discussion around the bus/master/dev representation is still ongoing, >> with Arnd opting for a simple approach where >> * the bus is implicitly represented by the master device >> * the master is not represented as a device under the I3C bus >> * only remote I3C devices are exposed and possibly duplicated if >> several masters controlling the same bus are exposed to the same >> Linux instance >> and Peter preferring the representation where the bus is a separate >> object. IIRC, Wolfram was in favor of the "bus is a separate object" >> too. >> >> If possible, I'd like to close this discussion soon, no matter which >> solution is chosen. > ... >> Missing features in this preliminary version: > ... >> - no support for multi-master and the associated concepts (mastership >> handover, support for secondary masters, ...) > > Let's try to come to a conclusion to this discussion, this is the main > show-stopper for inclusion that I see, as changing the fundamental > design would be hard to do once we do it one way or the other, > and the structure is exposed to user space. > > Peter and Wolfram, could you explain what scenario you can see that > would require handing over ownership of a device from one i3c master > to another i3c master when both are controlled by the same Linux > instance? > > To me this seems like a rather odd scenario, and supporting it > properly requires significant complexity once we try to support the > dynamic handover of the bus between two of our own masters. > > It seems more likely to me that we could deal with this case by > requiring either that each bus is controlled by at most one master > device in Linux, or at least that when we have two masters on > the same bus that they each control a non-overlapping set of > slave devices. Either way we'd be able to represent the structure > as a normal tree in the firmware (DT or ACPI) as well as in > sysfs. I have not read much of the I3C spec. I'm just coming from the current situation with I2C and the i2c-demux-pinctrl driver which tries to retrofit this into the I2C world and is not doing a grand job. And how could it? If you can acknowledge that i2c-demux-pinctrl is needed for I2C but for some reason is not needed for I3C because of something that differs between I2C and I3C, then fine, by all means ditch the explicit bus object. But in my mind splitting up the devices on the same bus between several of our own masters and then not have a single object for the bus is going to cause headaches down the line. Be it address clashes or trouble with master ping-pong or whatever. I think the reason for i2c-demux-pinctrl is that some (most?) I2C hardware suffers from quirks and one way to work around it is to make selected accesses from a different master. I expect I3C HW to also suffer from quirks... Maybe a bit-bang I3C master isn't feasible for some fundamental reason? But if it is, then I'd say that it's just a matter of time until someone finds a situation where such a thing could be used to work around some I3C quirk. And then it might be too expensive to always use the bit-bang master for the affected device. But what do I know? Don't let me hold this series back... 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 Fri, 20 Jul 2018 11:57:56 +0200 Peter Rosin <peda@axentia.se> wrote: > > Maybe a bit-bang I3C master isn't feasible for some fundamental > reason? No, it's clearly not. The way an I3C master is supposed to switch from open-drain to push-pull during a transaction or the concept of IBIs are the first things that come to mind, but I guess you have plenty of other reasons preventing you from implementing a i3c-bit-bang master. -- 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 have not read much of the I3C spec. I'm just coming from the > current situation with I2C and the i2c-demux-pinctrl driver which > tries to retrofit this into the I2C world and is not doing a grand > job. And how could it? > > If you can acknowledge that i2c-demux-pinctrl is needed for I2C > but for some reason is not needed for I3C because of something > that differs between I2C and I3C, then fine, by all means ditch > the explicit bus object. > > But in my mind splitting up the devices on the same bus between > several of our own masters and then not have a single object for > the bus is going to cause headaches down the line. Be it address > clashes or trouble with master ping-pong or whatever. > > I think the reason for i2c-demux-pinctrl is that some (most?) I2C > hardware suffers from quirks and one way to work around it is to > make selected accesses from a different master. I expect I3C HW > to also suffer from quirks... > > Maybe a bit-bang I3C master isn't feasible for some fundamental > reason? But if it is, then I'd say that it's just a matter of time > until someone finds a situation where such a thing could be used to > work around some I3C quirk. And then it might be too expensive to > always use the bit-bang master for the affected device. > > But what do I know? Don't let me hold this series back... Thanks, Peter. You nailed it, the I2C use case (and its limits). From what I know about I3C, bit-banging doesn't sound very feasible to me, so the situation might be a bit different. Still, no one knows about future I3C use cases and HW quirks. This is why I encouraged the seperate bus object because back then it was said it was easy to do and implement. If this now becomes a show-stopper, we can surely re-decide. I am not strong on this point, it was just something which would have helped I2C. (And for that matter, we (= the Renesas Upstream Kernel Team) was discussing something similar to the i2c demuxer for SPI, too. We have multiple IP cores which can do SPI on R-Car, all with their pros and cons)
On 2018-07-20 12:05, Boris Brezillon wrote: > On Fri, 20 Jul 2018 11:57:56 +0200 Peter Rosin <peda@axentia.se> wrote: >> Maybe a bit-bang I3C master isn't feasible for some fundamental >> reason? > > No, it's clearly not. The way an I3C master is supposed to switch from > open-drain to push-pull during a transaction or the concept of IBIs > are the first things that come to mind, but I guess you have plenty of > other reasons preventing you from implementing a i3c-bit-bang master. Why can't a bit-banger switch from open-drain to push-pull? That's just a matter of disconnecting some pull-ups, no? And maybe you know that no device on your bus uses IBIs? Maybe none of reasons for why bit-banging isn't feasible are applicable? I.e. maybe there can be a I3C bit-banger that perhaps isn't generic and isn't full-featured, but does work just fine in some specific setup? 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 Fri, Jul 20, 2018 at 12:12 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > >> I have not read much of the I3C spec. I'm just coming from the >> current situation with I2C and the i2c-demux-pinctrl driver which >> tries to retrofit this into the I2C world and is not doing a grand >> job. And how could it? >> >> If you can acknowledge that i2c-demux-pinctrl is needed for I2C >> but for some reason is not needed for I3C because of something >> that differs between I2C and I3C, then fine, by all means ditch >> the explicit bus object. >> >> But in my mind splitting up the devices on the same bus between >> several of our own masters and then not have a single object for >> the bus is going to cause headaches down the line. Be it address >> clashes or trouble with master ping-pong or whatever. >> >> I think the reason for i2c-demux-pinctrl is that some (most?) I2C >> hardware suffers from quirks and one way to work around it is to >> make selected accesses from a different master. I expect I3C HW >> to also suffer from quirks... >> >> Maybe a bit-bang I3C master isn't feasible for some fundamental >> reason? But if it is, then I'd say that it's just a matter of time >> until someone finds a situation where such a thing could be used to >> work around some I3C quirk. And then it might be too expensive to >> always use the bit-bang master for the affected device. >> >> But what do I know? Don't let me hold this series back... > > Thanks, Peter. You nailed it, the I2C use case (and its limits). From > what I know about I3C, bit-banging doesn't sound very feasible to me, so > the situation might be a bit different. Still, no one knows about future > I3C use cases and HW quirks. This is why I encouraged the seperate bus > object because back then it was said it was easy to do and implement. If > this now becomes a show-stopper, we can surely re-decide. I am not > strong on this point, it was just something which would have helped I2C. > (And for that matter, we (= the Renesas Upstream Kernel Team) was > discussing something similar to the i2c demuxer for SPI, too. We have > multiple IP cores which can do SPI on R-Car, all with their pros and > cons) I think we need to distinguish between demuxing and i3c master handover here, they are two separate issues that both need to be solved and that are not too hard to do, but we get into trouble if we combine them in arbitrary ways, which is what I'm concerned about: * What I understand from reading i2c-demux-pinctrl.c, a slave device will only ever be observable from one master at a time, when you switch over, all children get removed on one master and added to the other one, to be probed again by their respective drivers. I can see this as a useful feature on i3c as well, in particular to deal with the situation where we have i2c slaves connected to a pinmux that can switch them between an i3c master and an i2c-only master (possibly a gpio based one). That particular use case however doesn't seem to fix well in the current code, which is structure around i3c buses. * The other thing we definitely have to support for i3c is to deal with handing over control of the bus between the i3c master owned by Linux, and other masters that are /not/ owned by the same Linux instance. This is the part that the spec discusses in much detail, with the intention of temporarily giving up control of the bus to let another master do its thing on a shared slave without user interaction. Combining the two quickly gets nasty I think. The current design seems to imply that a device driver could keep talking to a slave while it is being reparented from one master to another. I can't think of a good reason why we would possibly want that, but it definitely opens up questions in what happens to e.g. the sysfs representation, lock order, and power management that I'd rather not have to think about. 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
> * What I understand from reading i2c-demux-pinctrl.c, a slave device > will only ever be observable from one master at a time, when you > switch over, all children get removed on one master and added to > the other one, to be probed again by their respective drivers. Yes. The very first versions of the demuxer tried to do it in a hot-swapping like fashion but then I switched over because of... > it definitely opens up questions in what happens to e.g. the sysfs > representation, lock order, and power management that I'd rather > not have to think about. ... this! There are dragons, I can tell you :) > * The other thing we definitely have to support for i3c is to deal with > handing over control of the bus between the i3c master owned > by Linux, and other masters that are /not/ owned by the same > Linux instance. This is the part that the spec discusses in much > detail, with the intention of temporarily giving up control of the > bus to let another master do its thing on a shared slave without > user interaction. I can't comment about this one.
On 2018-07-20 12:57, Arnd Bergmann wrote: > * What I understand from reading i2c-demux-pinctrl.c, a slave device > will only ever be observable from one master at a time, when you > switch over, all children get removed on one master and added to > the other one, to be probed again by their respective drivers. > I can see this as a useful feature on i3c as well, in particular to > deal with the situation where we have i2c slaves connected to a > pinmux that can switch them between an i3c master and an > i2c-only master (possibly a gpio based one). That particular use > case however doesn't seem to fix well in the current code, which > is structure around i3c buses. It's pretty easy to come up with examples where this reprobing is not desirable at all. E.g. if one of the involved I2C devices is a HDMI encoder (I have a TDA19988 here) sitting in the middle of the graphics pipeline. Blink-blink on the screen because some *other* unrelated device needed to be accessed by an alternative master. Not pretty. (No, I don't suffer from this since I don't need the demuxer. Which is fortunate. This was just an example, I'm sure there are others.) 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 Fri, Jul 20, 2018 at 1:13 PM, Peter Rosin <peda@axentia.se> wrote: > On 2018-07-20 12:57, Arnd Bergmann wrote: >> * What I understand from reading i2c-demux-pinctrl.c, a slave device >> will only ever be observable from one master at a time, when you >> switch over, all children get removed on one master and added to >> the other one, to be probed again by their respective drivers. >> I can see this as a useful feature on i3c as well, in particular to >> deal with the situation where we have i2c slaves connected to a >> pinmux that can switch them between an i3c master and an >> i2c-only master (possibly a gpio based one). That particular use >> case however doesn't seem to fix well in the current code, which >> is structure around i3c buses. > > It's pretty easy to come up with examples where this reprobing is > not desirable at all. E.g. if one of the involved I2C devices is > a HDMI encoder (I have a TDA19988 here) sitting in the middle of the > graphics pipeline. Blink-blink on the screen because some *other* > unrelated device needed to be accessed by an alternative master. Not > pretty. Agreed, we definitely don't want to reprobe all devices during normal operation for i3c master handover. What is the least contrived use case that you can think of where we would want to use one master to talk to one device on the bus, but another master to talk to another device on the same bus? I still hope that we can decide that this is not a useful scenario at all. If we find a case in which it is needed, we could still deal with it like this: - enumerate all slaves connected to the bus for each of the two masters - mark each slave as status="enabled" in at most one of the buses, and as disabled everywhere else - Use dynamic handover according to the bus protocol to switch masters without having Linux even know that the two buses are shared. That scenario would then fall completely into the "secondary master handover" category but require no special handling in the i3c layer beyond what we need for secondary masters that are managed by something outside of the kernel's score (a microcontroller, firmware, ...). 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 2018-07-20 13:28, Arnd Bergmann wrote: > On Fri, Jul 20, 2018 at 1:13 PM, Peter Rosin <peda@axentia.se> wrote: >> On 2018-07-20 12:57, Arnd Bergmann wrote: >>> * What I understand from reading i2c-demux-pinctrl.c, a slave device >>> will only ever be observable from one master at a time, when you >>> switch over, all children get removed on one master and added to >>> the other one, to be probed again by their respective drivers. >>> I can see this as a useful feature on i3c as well, in particular to >>> deal with the situation where we have i2c slaves connected to a >>> pinmux that can switch them between an i3c master and an >>> i2c-only master (possibly a gpio based one). That particular use >>> case however doesn't seem to fix well in the current code, which >>> is structure around i3c buses. >> >> It's pretty easy to come up with examples where this reprobing is >> not desirable at all. E.g. if one of the involved I2C devices is >> a HDMI encoder (I have a TDA19988 here) sitting in the middle of the >> graphics pipeline. Blink-blink on the screen because some *other* >> unrelated device needed to be accessed by an alternative master. Not >> pretty. > > Agreed, we definitely don't want to reprobe all devices during normal > operation for i3c master handover. > > What is the least contrived use case that you can think of where we > would want to use one master to talk to one device on the bus, > but another master to talk to another device on the same bus? > I still hope that we can decide that this is not a useful scenario > at all. > > If we find a case in which it is needed, we could still deal with it > like this: > - enumerate all slaves connected to the bus for each of the > two masters > - mark each slave as status="enabled" in at most one of the > buses, and as disabled everywhere else > - Use dynamic handover according to the bus protocol to > switch masters without having Linux even know that the > two buses are shared. > > That scenario would then fall completely into the "secondary > master handover" category but require no special handling > in the i3c layer beyond what we need for secondary masters > that are managed by something outside of the kernel's > score (a microcontroller, firmware, ...). The worst case is not mentioned above, where a single device benefits from being accessed by two different masters. That happens e.g. if one master is speedy but triggers a quirk and one master is slow and reliable, and the device must use the speedy master for some things (high bandwidth data?) but can't use it for other things (configuration?) and must fall back to the slow and reliable master for that. I don't have an actual example for I2C, maybe Wolfram does? But I can invent a case. E.g. the speedy DMA-enabled master cannot generate RESTART, which is a must for (re-)configuration, but not for passing data to the device. Also consider some future HW that has several I3C blocks, but they are not identical. There's one beefy kind and one slim kind (I'm sure you can find HW with different flavors of I2C blocks). Even if the HW designers intended for one type of block to be superior in every aspect, they might have made a mistake? This HW also has a pinmux, so the SW is free to route different I3C blocks to the actual I3C bus. Maybe the involved I3C blocks don't see the bus when the pinmux is in the "wrong" state? Then you can't do a normal handover... But if you *want* to end up with "that's just too contrived", then of course that's where you'll end up. 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 Fri, 20 Jul 2018 13:28:10 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > On Fri, Jul 20, 2018 at 1:13 PM, Peter Rosin <peda@axentia.se> wrote: > > On 2018-07-20 12:57, Arnd Bergmann wrote: > >> * What I understand from reading i2c-demux-pinctrl.c, a slave device > >> will only ever be observable from one master at a time, when you > >> switch over, all children get removed on one master and added to > >> the other one, to be probed again by their respective drivers. > >> I can see this as a useful feature on i3c as well, in particular to > >> deal with the situation where we have i2c slaves connected to a > >> pinmux that can switch them between an i3c master and an > >> i2c-only master (possibly a gpio based one). That particular use > >> case however doesn't seem to fix well in the current code, which > >> is structure around i3c buses. > > > > It's pretty easy to come up with examples where this reprobing is > > not desirable at all. E.g. if one of the involved I2C devices is > > a HDMI encoder (I have a TDA19988 here) sitting in the middle of the > > graphics pipeline. Blink-blink on the screen because some *other* > > unrelated device needed to be accessed by an alternative master. Not > > pretty. > > Agreed, we definitely don't want to reprobe all devices during normal > operation for i3c master handover. > Re-probing would not happen, no matter the solution we choose. It's that, in one case, you would have X virtual/linux devices representing the same physical device and in the other case, you would just have one, and everytime a transfer is requested by the driver, the core would pick the appropriate master to do it (most likely the one in control of the bus at that time) > What is the least contrived use case that you can think of where we > would want to use one master to talk to one device on the bus, > but another master to talk to another device on the same bus? The only reason I see for this use case is when you have 2 masters, each of them having different capabilities: device A needs cap X device B need cap Y master N provides cap X but not cap Y master M provides cap Y but not cap X In this case you'd want device A to use master M and device B to use master N. > I still hope that we can decide that this is not a useful scenario > at all. I'm not saying this scenario is about to happen IRL, just describing one potential reason for having 2 different masters connected to the same bus and both exposed to Linux. > > If we find a case in which it is needed, we could still deal with it > like this: > - enumerate all slaves connected to the bus for each of the > two masters That's what will happen if you don't share the same bus representation. > - mark each slave as status="enabled" in at most one of the > buses, and as disabled everywhere else We shouldn't need to do that. We can just let the driver check whether the master provides the necessary capabilities to efficiently communicate with the device, and if it does not just return -ENOTSUPP in the ->probe() function. This way you'll have a device, but not driver controlling it on one bus, and on the other bus, you'll have another device (which points to the same physical device) this time with a driver attached to it. > - Use dynamic handover according to the bus protocol to > switch masters without having Linux even know that the > two buses are shared. Yep. > > That scenario would then fall completely into the "secondary > master handover" category but require no special handling > in the i3c layer beyond what we need for secondary masters > that are managed by something outside of the kernel's > score (a microcontroller, firmware, ...). I definitely agree that both options should work. It's just that having X times the same physical device represented in Linux seemed like a bad thing to me, but it's also not something I'm strongly opposed to. 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
> I don't have an actual example for I2C, maybe Wolfram does? But I can > invent a case. E.g. the speedy DMA-enabled master cannot generate > RESTART, which is a must for (re-)configuration, but not for passing > data to the device. DMA capable controllers may also not react adequate to the slave doing clock stretching (which is forbidden in I3C). Renesas R-Car Gen2 has two I2C IP cores. One can do DMA and automatic transfers to the PMIC, the other has I2C slave functionality. One cannot do I2C_SMBUS_QUICK, the other can. And some more kind of quirks. Sometimes you can mux the pins to GPIO, so you have a third option. This setup is the reason the demux driver exists. > Also consider some future HW that has several I3C blocks, but they > are not identical. There's one beefy kind and one slim kind (I'm sure > you can find HW with different flavors of I2C blocks). Even if the > HW designers intended for one type of block to be superior in every > aspect, they might have made a mistake? This HW also has a pinmux, so > the SW is free to route different I3C blocks to the actual I3C bus. So, basically this is what happened with R-Car. Now, I tend to think that I3C is much more complex and noone would put two I3C IP cores into on SoC. But it was not too long ago that I wouldn't believe someone put two different I2C IP cores into a SoC. Then again, it happened when I2C was around for 35 years...
On Fri, Jul 20, 2018 at 3:17 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Fri, 20 Jul 2018 13:28:10 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > >> On Fri, Jul 20, 2018 at 1:13 PM, Peter Rosin <peda@axentia.se> wrote: >> > On 2018-07-20 12:57, Arnd Bergmann wrote: >> >> * What I understand from reading i2c-demux-pinctrl.c, a slave device >> >> will only ever be observable from one master at a time, when you >> >> switch over, all children get removed on one master and added to >> >> the other one, to be probed again by their respective drivers. >> >> I can see this as a useful feature on i3c as well, in particular to >> >> deal with the situation where we have i2c slaves connected to a >> >> pinmux that can switch them between an i3c master and an >> >> i2c-only master (possibly a gpio based one). That particular use >> >> case however doesn't seem to fix well in the current code, which >> >> is structure around i3c buses. >> > >> > It's pretty easy to come up with examples where this reprobing is >> > not desirable at all. E.g. if one of the involved I2C devices is >> > a HDMI encoder (I have a TDA19988 here) sitting in the middle of the >> > graphics pipeline. Blink-blink on the screen because some *other* >> > unrelated device needed to be accessed by an alternative master. Not >> > pretty. >> >> Agreed, we definitely don't want to reprobe all devices during normal >> operation for i3c master handover. >> > > Re-probing would not happen, no matter the solution we choose. It's > that, in one case, you would have X virtual/linux devices representing > the same physical device and in the other case, you would just have > one, and everytime a transfer is requested by the driver, the core > would pick the appropriate master to do it (most likely the one in > control of the bus at that time) I think this is one of the cases I'd want to avoid: controlling multiple masters that are active at the same time without going through the handover. If we have an actual pinmux between two masters and only one of them can even see the bus, I think we should go through a complete remove/probe cycle the way that the i2c-demux-pinctrl does today. If OTOH we a primary/secondary master pair with handover capability, I would prefer to not see one slave on both devices at the same time, or (ideally) only use one of the two masters and disable the other one completely. >> If we find a case in which it is needed, we could still deal with it >> like this: >> - enumerate all slaves connected to the bus for each of the >> two masters > > That's what will happen if you don't share the same bus representation. To clarify: I meant list them in the DT representation, not enumerate them in Linux during boot. Sorry for using a misleading description here. >> - mark each slave as status="enabled" in at most one of the >> buses, and as disabled everywhere else > > We shouldn't need to do that. We can just let the driver check whether > the master provides the necessary capabilities to efficiently > communicate with the device, and if it does not just return -ENOTSUPP > in the ->probe() function. This way you'll have a device, but not > driver controlling it on one bus, and on the other bus, you'll have > another device (which points to the same physical device) this time > with a driver attached to it. I'd still hope that we can completely avoid that case and never have the case where one physical device has two live representations in the kernel. It /could/ still be done of course, but would not always do the right thing, depending on the type of device (a temperature sensor could just be probed twice without problems, a network device probably cannot) 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, Jul 20, 2018 at 5:41 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > >> I don't have an actual example for I2C, maybe Wolfram does? But I can >> invent a case. E.g. the speedy DMA-enabled master cannot generate >> RESTART, which is a must for (re-)configuration, but not for passing >> data to the device. > > DMA capable controllers may also not react adequate to the slave doing > clock stretching (which is forbidden in I3C). > > Renesas R-Car Gen2 has two I2C IP cores. One can do DMA and automatic > transfers to the PMIC, the other has I2C slave functionality. One cannot > do I2C_SMBUS_QUICK, the other can. And some more kind of quirks. > Sometimes you can mux the pins to GPIO, so you have a third option. > > This setup is the reason the demux driver exists. Have you run into scenarios where you dynamically switch between the two masters in order to do different things (on one slave, or on multiple slaves), or could you always decide on one of them at boot time with that particular chip? >> Also consider some future HW that has several I3C blocks, but they >> are not identical. There's one beefy kind and one slim kind (I'm sure >> you can find HW with different flavors of I2C blocks). Even if the >> HW designers intended for one type of block to be superior in every >> aspect, they might have made a mistake? This HW also has a pinmux, so >> the SW is free to route different I3C blocks to the actual I3C bus. > > So, basically this is what happened with R-Car. Now, I tend to think > that I3C is much more complex and noone would put two I3C IP cores into > on SoC. But it was not too long ago that I wouldn't believe someone put > two different I2C IP cores into a SoC. Then again, it happened when I2C > was around for 35 years... I think an SoC design we will likely see is an i3c master multiplexed with an i2c master to access one bus. The i2c master can then use clock stretching and other things that may not work in the i3c master, and it may be used in the absence of proper i3c drivers in the OS. However, that case cannot be handled with the abstraction in the proposed i3c framework, which can only deal with multiple i3c standard compliant masters. I'm also not sure if it can be added to the i2c-demux-pinctrl driver. 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
Hi Arnd, On Tue, 24 Jul 2018 16:03:38 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > On Fri, Jul 20, 2018 at 3:17 PM, Boris Brezillon > <boris.brezillon@bootlin.com> wrote: > > On Fri, 20 Jul 2018 13:28:10 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > > > >> On Fri, Jul 20, 2018 at 1:13 PM, Peter Rosin <peda@axentia.se> wrote: > >> > On 2018-07-20 12:57, Arnd Bergmann wrote: > >> >> * What I understand from reading i2c-demux-pinctrl.c, a slave device > >> >> will only ever be observable from one master at a time, when you > >> >> switch over, all children get removed on one master and added to > >> >> the other one, to be probed again by their respective drivers. > >> >> I can see this as a useful feature on i3c as well, in particular to > >> >> deal with the situation where we have i2c slaves connected to a > >> >> pinmux that can switch them between an i3c master and an > >> >> i2c-only master (possibly a gpio based one). That particular use > >> >> case however doesn't seem to fix well in the current code, which > >> >> is structure around i3c buses. > >> > > >> > It's pretty easy to come up with examples where this reprobing is > >> > not desirable at all. E.g. if one of the involved I2C devices is > >> > a HDMI encoder (I have a TDA19988 here) sitting in the middle of the > >> > graphics pipeline. Blink-blink on the screen because some *other* > >> > unrelated device needed to be accessed by an alternative master. Not > >> > pretty. > >> > >> Agreed, we definitely don't want to reprobe all devices during normal > >> operation for i3c master handover. > >> > > > > Re-probing would not happen, no matter the solution we choose. It's > > that, in one case, you would have X virtual/linux devices representing > > the same physical device and in the other case, you would just have > > one, and everytime a transfer is requested by the driver, the core > > would pick the appropriate master to do it (most likely the one in > > control of the bus at that time) > > I think this is one of the cases I'd want to avoid: controlling multiple > masters that are active at the same time without going through > the handover. That's simply not possible, the I3C protocol forbids it. There can only be one active master on the bus at any point in time. > > If we have an actual pinmux between two masters and only one > of them can even see the bus, I think we should go through a > complete remove/probe cycle the way that the i2c-demux-pinctrl > does today. If OTOH we a primary/secondary master pair with > handover capability, I would prefer to not see one slave on > both devices at the same time, or (ideally) only use one of the > two masters and disable the other one completely. Again, you don't have a choice because it's part of the protocol. At any time, you only have one active master on the bus, and other masters are acting as slaves until they gain bus ownership (if they ever do). Say that device A wants to do an HDR transfer on the bus, and HDR is only supported by master X, but master Y is currently owning the bus. Master X will first have to request bus ownership before doing the transfer requested by device A. Now, imagine that device A wants to do an SDR transfer which is supported by both master X and master Y, and master Y is in control. Instead of requesting a bus handover, the framework would just automatically decide to do the transfer through master Y. That's the sort of things this separate bus/master representation allows. > > >> If we find a case in which it is needed, we could still deal with it > >> like this: > >> - enumerate all slaves connected to the bus for each of the > >> two masters > > > > That's what will happen if you don't share the same bus representation. > > To clarify: I meant list them in the DT representation, not enumerate > them in Linux during boot. Sorry for using a misleading description here. Okay. > > >> - mark each slave as status="enabled" in at most one of the > >> buses, and as disabled everywhere else > > > > We shouldn't need to do that. We can just let the driver check whether > > the master provides the necessary capabilities to efficiently > > communicate with the device, and if it does not just return -ENOTSUPP > > in the ->probe() function. This way you'll have a device, but not > > driver controlling it on one bus, and on the other bus, you'll have > > another device (which points to the same physical device) this time > > with a driver attached to it. > > I'd still hope that we can completely avoid that case and never > have the case where one physical device has two live > representations in the kernel. It /could/ still be done of course, > but would not always do the right thing, depending on the > type of device (a temperature sensor could just be probed > twice without problems, a network device probably cannot) Not really feasible if we don't share the same bus representation. So, that means you hope we'll never have a real case where 2 masters are connected to the same physical bus and both exposed to the same Linux instance. I'm still unsure what you think adds complexity in the current approach. When I implemented it, it looked like is was almost the same (in term of complexity) to have a bus object separated from the master, but I'm probably missing something. Anyway, here's what I propose. I'll work on a v7 where the bus object is tied to the master (and not exposed in sysfs or the DT representation) and the master itself is not represented as a device on the bus. This way you'll have both solutions to compare them and take a decision. 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 Tue, Jul 24, 2018 at 4:28 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > Hi Arnd, > > On Tue, 24 Jul 2018 16:03:38 +0200 > Arnd Bergmann <arnd@arndb.de> wrote: > >> On Fri, Jul 20, 2018 at 3:17 PM, Boris Brezillon >> <boris.brezillon@bootlin.com> wrote: >> > On Fri, 20 Jul 2018 13:28:10 +0200 Arnd Bergmann <arnd@arndb.de> wrote: >> > >> >> On Fri, Jul 20, 2018 at 1:13 PM, Peter Rosin <peda@axentia.se> wrote: >> >> > On 2018-07-20 12:57, Arnd Bergmann wrote: >> >> >> * What I understand from reading i2c-demux-pinctrl.c, a slave device >> >> >> will only ever be observable from one master at a time, when you >> >> >> switch over, all children get removed on one master and added to >> >> >> the other one, to be probed again by their respective drivers. >> >> >> I can see this as a useful feature on i3c as well, in particular to >> >> >> deal with the situation where we have i2c slaves connected to a >> >> >> pinmux that can switch them between an i3c master and an >> >> >> i2c-only master (possibly a gpio based one). That particular use >> >> >> case however doesn't seem to fix well in the current code, which >> >> >> is structure around i3c buses. >> >> > >> >> > It's pretty easy to come up with examples where this reprobing is >> >> > not desirable at all. E.g. if one of the involved I2C devices is >> >> > a HDMI encoder (I have a TDA19988 here) sitting in the middle of the >> >> > graphics pipeline. Blink-blink on the screen because some *other* >> >> > unrelated device needed to be accessed by an alternative master. Not >> >> > pretty. >> >> >> >> Agreed, we definitely don't want to reprobe all devices during normal >> >> operation for i3c master handover. >> >> >> > >> > Re-probing would not happen, no matter the solution we choose. It's >> > that, in one case, you would have X virtual/linux devices representing >> > the same physical device and in the other case, you would just have >> > one, and everytime a transfer is requested by the driver, the core >> > would pick the appropriate master to do it (most likely the one in >> > control of the bus at that time) >> >> I think this is one of the cases I'd want to avoid: controlling multiple >> masters that are active at the same time without going through >> the handover. > > That's simply not possible, the I3C protocol forbids it. There can only > be one active master on the bus at any point in time. Ok, it sounded like that's what you wanted to do here. >> If we have an actual pinmux between two masters and only one >> of them can even see the bus, I think we should go through a >> complete remove/probe cycle the way that the i2c-demux-pinctrl >> does today. If OTOH we a primary/secondary master pair with >> handover capability, I would prefer to not see one slave on >> both devices at the same time, or (ideally) only use one of the >> two masters and disable the other one completely. > > Again, you don't have a choice because it's part of the protocol. At > any time, you only have one active master on the bus, and other masters > are acting as slaves until they gain bus ownership (if they ever do). > Say that device A wants to do an HDR transfer on the bus, and HDR is > only supported by master X, but master Y is currently owning the bus. > Master X will first have to request bus ownership before doing the > transfer requested by device A. > > Now, imagine that device A wants to do an SDR transfer which is > supported by both master X and master Y, and master Y is in control. > Instead of requesting a bus handover, the framework would just > automatically decide to do the transfer through master Y. That's the > sort of things this separate bus/master representation allows. That's not the case I was describing here, I was thinking of what Wolfram described with the Renesas SoC that has two i2c masters multiplexed through the pinmux layer. I would assume that we can still do the same thing in i3c by shutting down the current master without a handover, and reprobing everything from scratch. If only one of the two masters is physically connected to the bus at any time, the handover protocol certainly wouldn't apply. >> >> - mark each slave as status="enabled" in at most one of the >> >> buses, and as disabled everywhere else >> > >> > We shouldn't need to do that. We can just let the driver check whether >> > the master provides the necessary capabilities to efficiently >> > communicate with the device, and if it does not just return -ENOTSUPP >> > in the ->probe() function. This way you'll have a device, but not >> > driver controlling it on one bus, and on the other bus, you'll have >> > another device (which points to the same physical device) this time >> > with a driver attached to it. >> >> I'd still hope that we can completely avoid that case and never >> have the case where one physical device has two live >> representations in the kernel. It /could/ still be done of course, >> but would not always do the right thing, depending on the >> type of device (a temperature sensor could just be probed >> twice without problems, a network device probably cannot) > > Not really feasible if we don't share the same bus representation. So, > that means you hope we'll never have a real case where 2 masters are > connected to the same physical bus and both exposed to the same Linux > instance. Why not? As I described in my earlier mail, we just need to make sure that either one of the two masters gets all the devices and the other master is completely disabled, or each master gets a subset of the devices and all other devices are marked as status="disabled" in DT to prevent them from being bound to a driver more than once. > I'm still unsure what you think adds complexity in the current > approach. When I implemented it, it looked like is was almost the same > (in term of complexity) to have a bus object separated from the master, > but I'm probably missing something. > > Anyway, here's what I propose. I'll work on a v7 where the bus object > is tied to the master (and not exposed in sysfs or the DT > representation) and the master itself is not represented as a device on > the bus. This way you'll have both solutions to compare them and take a > decision. That sounds helpful, thanks a lot! 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
Hi Arnd, On Tue, Jul 24, 2018 at 5:05 PM Arnd Bergmann <arnd@arndb.de> wrote: > On Tue, Jul 24, 2018 at 4:28 PM, Boris Brezillon > <boris.brezillon@bootlin.com> wrote: > > On Tue, 24 Jul 2018 16:03:38 +0200 > > Arnd Bergmann <arnd@arndb.de> wrote: > >> On Fri, Jul 20, 2018 at 3:17 PM, Boris Brezillon > >> <boris.brezillon@bootlin.com> wrote: > >> > On Fri, 20 Jul 2018 13:28:10 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > >> >> On Fri, Jul 20, 2018 at 1:13 PM, Peter Rosin <peda@axentia.se> wrote: > >> >> > On 2018-07-20 12:57, Arnd Bergmann wrote: > >> >> >> * What I understand from reading i2c-demux-pinctrl.c, a slave device > >> >> >> will only ever be observable from one master at a time, when you > >> >> >> switch over, all children get removed on one master and added to > >> >> >> the other one, to be probed again by their respective drivers. > >> >> >> I can see this as a useful feature on i3c as well, in particular to > >> >> >> deal with the situation where we have i2c slaves connected to a > >> >> >> pinmux that can switch them between an i3c master and an > >> >> >> i2c-only master (possibly a gpio based one). That particular use > >> >> >> case however doesn't seem to fix well in the current code, which > >> >> >> is structure around i3c buses. > >> >> > > >> >> > It's pretty easy to come up with examples where this reprobing is > >> >> > not desirable at all. E.g. if one of the involved I2C devices is > >> >> > a HDMI encoder (I have a TDA19988 here) sitting in the middle of the > >> >> > graphics pipeline. Blink-blink on the screen because some *other* > >> >> > unrelated device needed to be accessed by an alternative master. Not > >> >> > pretty. > >> >> > >> >> Agreed, we definitely don't want to reprobe all devices during normal > >> >> operation for i3c master handover. > >> >> > >> > > >> > Re-probing would not happen, no matter the solution we choose. It's > >> > that, in one case, you would have X virtual/linux devices representing > >> > the same physical device and in the other case, you would just have > >> > one, and everytime a transfer is requested by the driver, the core > >> > would pick the appropriate master to do it (most likely the one in > >> > control of the bus at that time) > >> > >> I think this is one of the cases I'd want to avoid: controlling multiple > >> masters that are active at the same time without going through > >> the handover. > > > > That's simply not possible, the I3C protocol forbids it. There can only > > be one active master on the bus at any point in time. > > Ok, it sounded like that's what you wanted to do here. > > >> If we have an actual pinmux between two masters and only one > >> of them can even see the bus, I think we should go through a > >> complete remove/probe cycle the way that the i2c-demux-pinctrl > >> does today. If OTOH we a primary/secondary master pair with > >> handover capability, I would prefer to not see one slave on > >> both devices at the same time, or (ideally) only use one of the > >> two masters and disable the other one completely. > > > > Again, you don't have a choice because it's part of the protocol. At > > any time, you only have one active master on the bus, and other masters > > are acting as slaves until they gain bus ownership (if they ever do). > > Say that device A wants to do an HDR transfer on the bus, and HDR is > > only supported by master X, but master Y is currently owning the bus. > > Master X will first have to request bus ownership before doing the > > transfer requested by device A. > > > > Now, imagine that device A wants to do an SDR transfer which is > > supported by both master X and master Y, and master Y is in control. > > Instead of requesting a bus handover, the framework would just > > automatically decide to do the transfer through master Y. That's the > > sort of things this separate bus/master representation allows. > > That's not the case I was describing here, I was thinking of what > Wolfram described with the Renesas SoC that has two i2c masters > multiplexed through the pinmux layer. I would assume that we > can still do the same thing in i3c by shutting down the current > master without a handover, and reprobing everything from scratch. The major disadvantage of reprobing is that it may cause visual disturbances when i2c slaves are involved with e.g. the display pipeline (think HDMI encoders etc.). Gr{oetje,eeting}s, Geert
On Tue, Jul 24, 2018 at 5:15 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, Jul 24, 2018 at 5:05 PM Arnd Bergmann <arnd@arndb.de> wrote: >> That's not the case I was describing here, I was thinking of what >> Wolfram described with the Renesas SoC that has two i2c masters >> multiplexed through the pinmux layer. I would assume that we >> can still do the same thing in i3c by shutting down the current >> master without a handover, and reprobing everything from scratch. > > The major disadvantage of reprobing is that it may cause visual disturbances > when i2c slaves are involved with e.g. the display pipeline (think HDMI encoders > etc.). Do you mean we should reuse the device pointer and association with the driver even when we switch out the i3c master using the pinmux? Or do you mean we need to be prepared for driving a single slave through multiple masters over the lifetime of that device, but using the i3c master handover protocol? In the second case, how do we decide which master to use for accessing a device for a given request? 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
Hi Arnd, On Tue, Jul 24, 2018 at 5:40 PM Arnd Bergmann <arnd@arndb.de> wrote: > On Tue, Jul 24, 2018 at 5:15 PM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > > On Tue, Jul 24, 2018 at 5:05 PM Arnd Bergmann <arnd@arndb.de> wrote: > > >> That's not the case I was describing here, I was thinking of what > >> Wolfram described with the Renesas SoC that has two i2c masters > >> multiplexed through the pinmux layer. I would assume that we > >> can still do the same thing in i3c by shutting down the current > >> master without a handover, and reprobing everything from scratch. > > > > The major disadvantage of reprobing is that it may cause visual disturbances > > when i2c slaves are involved with e.g. the display pipeline (think HDMI encoders > > etc.). > > Do you mean we should reuse the device pointer and association with > the driver even when we switch out the i3c master using the pinmux? > > Or do you mean we need to be prepared for driving a single > slave through multiple masters over the lifetime of that device, > but using the i3c master handover protocol? > In the second case, how do we decide which master to use > for accessing a device for a given request? I'll have to defer to Wolfram. He's the i2c and muxing expert. Gr{oetje,eeting}s, Geert
> > Renesas R-Car Gen2 has two I2C IP cores. One can do DMA and automatic > > transfers to the PMIC, the other has I2C slave functionality. One cannot > > do I2C_SMBUS_QUICK, the other can. And some more kind of quirks. > > Sometimes you can mux the pins to GPIO, so you have a third option. > > > > This setup is the reason the demux driver exists. > > Have you run into scenarios where you dynamically switch between > the two masters in order to do different things (on one slave, or > on multiple slaves), or could you always decide on one of them > at boot time with that particular chip? My personal use case is debugging. R-Car H2 is great because I can always pinmux this or that I2C IP core to the same set of pins, and in 2 out of 4 cases even GPIO bitbang on top of that. So, it is great to compare behaviour, do scopes with the same type of setup, etc... For that, I do runtime switches, but the slaves are not really under real usage. I have absolutely no idea how $customers use it, sadly. > I think an SoC design we will likely see is an i3c master multiplexed with > an i2c master to access one bus. The i2c master can then use clock > stretching and other things that may not work in the i3c master, and it > may be used in the absence of proper i3c drivers in the OS. Multiplexed? Well, as soon you want to use I3C features like IBI, this is not going to work, right? It will not even work with Linux being an I2C slave itself. Or do you mean running the I3C and I2C controller simultaneously using the same wires? > However, that case cannot be handled with the abstraction in the > proposed i3c framework, which can only deal with multiple i3c > standard compliant masters. I'm also not sure if it can be added > to the i2c-demux-pinctrl driver. The I2C demuxer maps the whole bus to an i2c_adapter. You cannot select a master per client.
On Tue, Jul 24, 2018 at 5:46 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Arnd, > > On Tue, Jul 24, 2018 at 5:40 PM Arnd Bergmann <arnd@arndb.de> wrote: >> On Tue, Jul 24, 2018 at 5:15 PM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >> > On Tue, Jul 24, 2018 at 5:05 PM Arnd Bergmann <arnd@arndb.de> wrote: >> >> >> That's not the case I was describing here, I was thinking of what >> >> Wolfram described with the Renesas SoC that has two i2c masters >> >> multiplexed through the pinmux layer. I would assume that we >> >> can still do the same thing in i3c by shutting down the current >> >> master without a handover, and reprobing everything from scratch. >> > >> > The major disadvantage of reprobing is that it may cause visual disturbances >> > when i2c slaves are involved with e.g. the display pipeline (think HDMI encoders >> > etc.). >> >> Do you mean we should reuse the device pointer and association with >> the driver even when we switch out the i3c master using the pinmux? >> >> Or do you mean we need to be prepared for driving a single >> slave through multiple masters over the lifetime of that device, >> but using the i3c master handover protocol? >> In the second case, how do we decide which master to use >> for accessing a device for a given request? > > I'll have to defer to Wolfram. He's the i2c and muxing expert. On i2c, we only have the first case, and Wolfram said that it intentionally does the reprobe to avoid the problems we discussed. The question is what to do about this if it happens again on i3c. Peter seemed to think that it was possibly something we might have to handle, while Boris said that it wouldn't be because it's not coverered by the i3c spec. The second case is the one that started the discussion, and this is where I said I'd prefer to associate each slave with at most one master at boot time, while the current v6 patch is prepared for having one slave be accessed alternatingly by multiple masters using the master handover, though so far nobody has been able to describe exactly how we'd pick which master is active at what point, or what specific scenario would require it. 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
> The major disadvantage of reprobing is that it may cause visual disturbances > when i2c slaves are involved with e.g. the display pipeline (think HDMI encoders > etc.). Another one is that you might find bugs related to re-binding which can be surprisingly hard to solve... IIRC we had a case with the regulator subsystem where rebinding the PMIC which regulated the CPU is still unresolved. I think. I'd need to recheck to be sure.
On Tue, Jul 24, 2018 at 5:57 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > >> > Renesas R-Car Gen2 has two I2C IP cores. One can do DMA and automatic >> > transfers to the PMIC, the other has I2C slave functionality. One cannot >> > do I2C_SMBUS_QUICK, the other can. And some more kind of quirks. >> > Sometimes you can mux the pins to GPIO, so you have a third option. >> > >> > This setup is the reason the demux driver exists. >> >> Have you run into scenarios where you dynamically switch between >> the two masters in order to do different things (on one slave, or >> on multiple slaves), or could you always decide on one of them >> at boot time with that particular chip? > > My personal use case is debugging. R-Car H2 is great because I can > always pinmux this or that I2C IP core to the same set of pins, and in 2 > out of 4 cases even GPIO bitbang on top of that. So, it is great to > compare behaviour, do scopes with the same type of setup, etc... > For that, I do runtime switches, but the slaves are not really under > real usage. Ok, so runtime here still means it's chosen by an operator (i.e. you), not part of regular operation. > I have absolutely no idea how $customers use it, sadly. Right. >> I think an SoC design we will likely see is an i3c master multiplexed with >> an i2c master to access one bus. The i2c master can then use clock >> stretching and other things that may not work in the i3c master, and it >> may be used in the absence of proper i3c drivers in the OS. > > Multiplexed? Well, as soon you want to use I3C features like IBI, this > is not going to work, right? It will not even work with Linux being an > I2C slave itself. Or do you mean running the I3C and I2C controller > simultaneously using the same wires? I meant multiplexing it through the pinmux framework, with one of the two being active at any time. Obviously this makes no sense for i3c slaves, but it can be useful if the bus only contains i2c slaves. >> However, that case cannot be handled with the abstraction in the >> proposed i3c framework, which can only deal with multiple i3c >> standard compliant masters. I'm also not sure if it can be added >> to the i2c-demux-pinctrl driver. > > The I2C demuxer maps the whole bus to an i2c_adapter. You cannot select > a master per client. What I meant here was switching the bus between an i2c master and an i3c master like you do with the i2c demuxer. Right now, this wouldn't work because i2c and i3c use different representations in DT and in Linux for the same devices. 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 Tue, 24 Jul 2018 17:57:17 +0200 Wolfram Sang <wsa@the-dreams.de> wrote: > > I think an SoC design we will likely see is an i3c master multiplexed with > > an i2c master to access one bus. The i2c master can then use clock > > stretching and other things that may not work in the i3c master, and it > > may be used in the absence of proper i3c drivers in the OS. > > Multiplexed? Well, as soon you want to use I3C features like IBI, this > is not going to work, right? It will not even work with Linux being an > I2C slave itself. Or do you mean running the I3C and I2C controller > simultaneously using the same wires? I think we should switch to an I3C mindset. It seems the use cases we're discussing so far are I2C use cases. Keep in mind that I3C bus management is completely different from I2C one (event if I3C is backward compatible with I2C, it's just a feature, not the foundation of the I3C protocol). -- 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, 24 Jul 2018 17:58:29 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > On Tue, Jul 24, 2018 at 5:46 PM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > > Hi Arnd, > > > > On Tue, Jul 24, 2018 at 5:40 PM Arnd Bergmann <arnd@arndb.de> wrote: > >> On Tue, Jul 24, 2018 at 5:15 PM, Geert Uytterhoeven > >> <geert@linux-m68k.org> wrote: > >> > On Tue, Jul 24, 2018 at 5:05 PM Arnd Bergmann <arnd@arndb.de> wrote: > >> > >> >> That's not the case I was describing here, I was thinking of what > >> >> Wolfram described with the Renesas SoC that has two i2c masters > >> >> multiplexed through the pinmux layer. I would assume that we > >> >> can still do the same thing in i3c by shutting down the current > >> >> master without a handover, and reprobing everything from scratch. > >> > > >> > The major disadvantage of reprobing is that it may cause visual disturbances > >> > when i2c slaves are involved with e.g. the display pipeline (think HDMI encoders > >> > etc.). > >> > >> Do you mean we should reuse the device pointer and association with > >> the driver even when we switch out the i3c master using the pinmux? > >> > >> Or do you mean we need to be prepared for driving a single > >> slave through multiple masters over the lifetime of that device, > >> but using the i3c master handover protocol? > >> In the second case, how do we decide which master to use > >> for accessing a device for a given request? > > > > I'll have to defer to Wolfram. He's the i2c and muxing expert. > > On i2c, we only have the first case, and Wolfram said that it > intentionally does the reprobe to avoid the problems we discussed. > The question is what to do about this if it happens again on i3c. > Peter seemed to think that it was possibly something we might > have to handle, while Boris said that it wouldn't be because it's > not coverered by the i3c spec. > > The second case is the one that started the discussion, and > this is where I said I'd prefer to associate each slave with at > most one master at boot time, while the current v6 patch > is prepared for having one slave be accessed alternatingly > by multiple masters using the master handover, though so > far nobody has been able to describe exactly how we'd pick > which master is active at what point, Even if it's not yet implemented, I have everything in place to figure this out (see the ->cur_master field in the i3c_bus object). Now, what's missing is a list of possible masters attached to an i3c device so that the framework can pick the most appropriate one at runtime and initiate mastership handover if required (if the selected master is not the currently active one). The selection logic should look like this: if (active_master supports requested feature) use active master else pick an inactive one that has relevant caps and initiate mastership handover (+ update bus->cur_master) > or what specific scenario > would require it. I think I described a scenario (masters having different capabilities all connected to the same bus), though I don't know how likely this use case is :-/. -- 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, Jul 24, 2018 at 6:14 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Tue, 24 Jul 2018 17:58:29 +0200 > Arnd Bergmann <arnd@arndb.de> wrote: > >> On Tue, Jul 24, 2018 at 5:46 PM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >> > On Tue, Jul 24, 2018 at 5:40 PM Arnd Bergmann <arnd@arndb.de> wrote: >> >> On Tue, Jul 24, 2018 at 5:15 PM, Geert Uytterhoeven >> >> <geert@linux-m68k.org> wrote: >> >> > On Tue, Jul 24, 2018 at 5:05 PM Arnd Bergmann <arnd@arndb.de> wrote: >> The second case is the one that started the discussion, and >> this is where I said I'd prefer to associate each slave with at >> most one master at boot time, while the current v6 patch >> is prepared for having one slave be accessed alternatingly >> by multiple masters using the master handover, though so >> far nobody has been able to describe exactly how we'd pick >> which master is active at what point, > > Even if it's not yet implemented, I have everything in place to figure > this out (see the ->cur_master field in the i3c_bus object). Now, > what's missing is a list of possible masters attached to an i3c device > so that the framework can pick the most appropriate one at runtime and > initiate mastership handover if required (if the selected master is not > the currently active one). > > The selection logic should look like this: > > if (active_master supports requested feature) > use active master > else > pick an inactive one that has relevant caps and initiate > mastership handover (+ update bus->cur_master) How would you deal with soft requirements like performance? E.g. if you have one master that can do large transfers faster through a special DMA engine, and other master that can be faster for small transfers, but both support all capabilities for that device, won't you need some complex logic to avoid being stuck with a slow master indefinitely? >> or what specific scenario >> would require it. > > I think I described a scenario (masters having different > capabilities all connected to the same bus), though I don't know how > likely this use case is :-/. I was looking for something more specific here. What (lack of) capabilities could two i3c controllers have that require you to use both of them for the same device, rather than picking a master for each slave with the right feature set? 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 Tue, 24 Jul 2018 18:25:22 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > On Tue, Jul 24, 2018 at 6:14 PM, Boris Brezillon > <boris.brezillon@bootlin.com> wrote: > > On Tue, 24 Jul 2018 17:58:29 +0200 > > Arnd Bergmann <arnd@arndb.de> wrote: > > > >> On Tue, Jul 24, 2018 at 5:46 PM, Geert Uytterhoeven > >> <geert@linux-m68k.org> wrote: > >> > On Tue, Jul 24, 2018 at 5:40 PM Arnd Bergmann <arnd@arndb.de> wrote: > >> >> On Tue, Jul 24, 2018 at 5:15 PM, Geert Uytterhoeven > >> >> <geert@linux-m68k.org> wrote: > >> >> > On Tue, Jul 24, 2018 at 5:05 PM Arnd Bergmann <arnd@arndb.de> wrote: > >> The second case is the one that started the discussion, and > >> this is where I said I'd prefer to associate each slave with at > >> most one master at boot time, while the current v6 patch > >> is prepared for having one slave be accessed alternatingly > >> by multiple masters using the master handover, though so > >> far nobody has been able to describe exactly how we'd pick > >> which master is active at what point, > > > > Even if it's not yet implemented, I have everything in place to figure > > this out (see the ->cur_master field in the i3c_bus object). Now, > > what's missing is a list of possible masters attached to an i3c device > > so that the framework can pick the most appropriate one at runtime and > > initiate mastership handover if required (if the selected master is not > > the currently active one). > > > > The selection logic should look like this: > > > > if (active_master supports requested feature) > > use active master > > else > > pick an inactive one that has relevant caps and initiate > > mastership handover (+ update bus->cur_master) > > How would you deal with soft requirements like performance? > E.g. if you have one master that can do large transfers faster > through a special DMA engine, and other master that can > be faster for small transfers, but both support all capabilities > for that device, won't you need some complex logic to avoid > being stuck with a slow master indefinitely? True. > > >> or what specific scenario > >> would require it. > > > > I think I described a scenario (masters having different > > capabilities all connected to the same bus), though I don't know how > > likely this use case is :-/. > > I was looking for something more specific here. What (lack of) > capabilities could two i3c controllers have that require you to > use both of them for the same device, rather than picking > a master for each slave with the right feature set? Hehe, if I had a clear answer to this question we wouldn't have this discussion :-). I gave you an example: - master A supporting IBIs but not HDR transactions - master B supporting HDR modes but not IBIs but as I said, I'm not sure how likely this example is... The question is more, should we design things so that we can at some point implement a solution to support those funky setups, or should we just ignore it and risk breaking sysfs/DT ABI when/if we have to support that? This is really an open question. I initially went for the former, but have no objection switching to the latter. -- 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, Jul 24, 2018 at 6:54 PM, Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Tue, 24 Jul 2018 18:25:22 +0200 > Arnd Bergmann <arnd@arndb.de> wrote: > >> On Tue, Jul 24, 2018 at 6:14 PM, Boris Brezillon >> <boris.brezillon@bootlin.com> wrote: >> > On Tue, 24 Jul 2018 17:58:29 +0200 >> > Arnd Bergmann <arnd@arndb.de> wrote: >> >> or what specific scenario would require it. >> > >> > I think I described a scenario (masters having different >> > capabilities all connected to the same bus), though I don't know how >> > likely this use case is :-/. >> >> I was looking for something more specific here. What (lack of) >> capabilities could two i3c controllers have that require you to >> use both of them for the same device, rather than picking >> a master for each slave with the right feature set? > > Hehe, if I had a clear answer to this question we wouldn't have this > discussion :-). I gave you an example: > > - master A supporting IBIs but not HDR transactions > - master B supporting HDR modes but not IBIs > > but as I said, I'm not sure how likely this example is... I'd say for a specific example like that, the person that did the SoC integration should find a new job outside of hardware design ;-) I suppose the point is really that this is only preparation for something completely unexpected, and any specific example one could come up with is very unlikely to occur in real hardware. > The question is more, should we design things so that we can at some > point implement a solution to support those funky setups, or should we > just ignore it and risk breaking sysfs/DT ABI when/if we have to support > that? > > This is really an open question. I initially went for the former, but > have no objection switching to the latter. For me it's mainly a feeling that the risk of something going wrong with the current design is bigger than it actually solving problems we will encounter later. I hope that when you do a v7 version for comparison, I'll be able to pinpoint specific aspects that are better rather than being that unspecific. (note: I'll be on vacation next week and won't be able to review it until I'm back). Let me try to summarize the points made so far: 1. If we need a way for dynamic handover between two of our own masters and are not prepared for it now, some hardware designs may end up being unusable junk. Hopefully those cases are rare and found early during design when the hardware can still be changed to something that works. 2. If we design a system that does allow that handover and we don't need it, the biggest risk is introducing complexity in the system that makes it harder to use and debug for everyone. 3. The case where we have two masters on a bus, but each slave is only ever driven by one master can easily be added later, by adding some DT description for that machine as I described, but no extra code or DT bindings, or reprobing of devices during handover. 4. Handing over between an i2c master and an i3c master cannot be done with the current design either way, and could only ever work in very limited scenarios. The same is true for i3c masters that can be connected to the same bus, but not at the same time (like the case with multiplexed i2c masters today). 5. The debug scenario that Wolfram described might be handled with a separate bus structure and handing over behind the curtains, but does not require it to be done without a reprobe. I can imagine several other (simpler) designs that would allow doing this. 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
> > My personal use case is debugging. R-Car H2 is great because I can > > always pinmux this or that I2C IP core to the same set of pins, and in 2 > > out of 4 cases even GPIO bitbang on top of that. So, it is great to > > compare behaviour, do scopes with the same type of setup, etc... > > For that, I do runtime switches, but the slaves are not really under > > real usage. > > Ok, so runtime here still means it's chosen by an operator (i.e. you), > not part of regular operation. Yes. > I meant multiplexing it through the pinmux framework, with one of the > two being active at any time. Obviously this makes no sense for > i3c slaves, but it can be useful if the bus only contains i2c slaves. Unless Linux is not an I2C slave itself. > What I meant here was switching the bus between an i2c master and an > i3c master like you do with the i2c demuxer. Right now, this wouldn't > work because i2c and i3c use different representations in DT and in > Linux for the same devices. I see. Thanks for the clarification.
On 7/19/2018 9:29 AM, 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. > - 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: > - I3C HDR modes are not supported > - 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 > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- > Changes in v6: > - Add I3C/I2C dev descriptors to simplify I3C driver controllers when > migrating resources from on I3C device slot to anoter > - Add I3C error codes and return them when doing SDR/priv and CCC > transfers. This allows us to properly detect when no devices acked > a CCC command, which in some cases is a valid situation (no I3C > devices on the bus) > - Remove __packed specifiers where unneeded > - Allocate all IBI slots in one call using kcalloc() > > Changes in v5: > - Rename the address sysfs entry into dynamic_address to clarify things > - Document that we expect buffers passed to i3c_device_do_priv_xfers() > to be DMA-able > - Fix DEFSLVS CCC command > - s/2017/2018/ in copyright headers > - Fix SPDX header in internals.h > - Fix coding style issues > > Changes in v4: > - none > > Changes in v3: > - Fix locking issues > - Explicitly include a bunch of headers (reported by Randy Dunlap) > - Rename {i2c,i3c}-scl-frequency DT prop into {i2c,i3c}-scl-hz > - Do not use BIT() macro in mod_devicetable.h > - Fix typos > - Fix/enhance some kernel doc headers > - Rework the bus initialization code to simplify master drivers > - Assign dynamic address with SETDASA if the device has a static > address and the DT has a valid assigned-address property > - Rework the LVR extraction in DT parsing code > - Add code to detect when a device is re-attached to the bus after > losing its dynamic address. In this case we know try to re-assign the > old address, and most importantly, the I3C device driver sees the same > device instance, not a new one > - Add an ->i2c_funcs() hook to let the master declare which I2C features > it supports > - Unexport a few functions > - Remove support for HDR mode since we have no real user yet > > Changes in v2: > - Fix a bunch of mistake I made with the device model (pointed by GKH) > - Move the documentation out of this commit (pointed by GKH) > - only source drivers/i3c/master/Kconfig when CONFIG_I3C is enabled > (pointed by GKH) > - Add IBI infrastructure > - Add helpers to ease support for Hot Join (most of the logic is > delegated to I3C controller drivers) > - move the doc out of this commit to improve readability > - Fix a few bugs in device probing/remove (detected after trying to > load/unload modules in various orders) > - Add a module_i3c_i2c_driver() macro to ease integration of drivers > for devices that support both I3C and I2C mode > --- > drivers/Kconfig | 2 + > drivers/Makefile | 2 +- > drivers/i3c/Kconfig | 24 + > drivers/i3c/Makefile | 4 + > drivers/i3c/core.c | 606 ++++++++++++ > drivers/i3c/device.c | 233 +++++ > drivers/i3c/internals.h | 36 + > drivers/i3c/master.c | 2058 > +++++++++++++++++++++++++++++++++++++++ > drivers/i3c/master/Kconfig | 0 > drivers/i3c/master/Makefile | 0 > include/linux/i3c/ccc.h | 385 ++++++++ > include/linux/i3c/device.h | 331 +++++++ > include/linux/i3c/master.h | 652 +++++++++++++ > include/linux/mod_devicetable.h | 17 + > 14 files changed, 4349 insertions(+), 1 deletion(-) > 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 include/linux/i3c/ccc.h > create mode 100644 include/linux/i3c/device.h > create mode 100644 include/linux/i3c/master.h > > diff --git a/drivers/Kconfig b/drivers/Kconfig > index 95b9ccc08165..80f6aebc896f 100644 > --- a/drivers/Kconfig > +++ b/drivers/Kconfig > @@ -55,6 +55,8 @@ source "drivers/char/Kconfig" > > source "drivers/i2c/Kconfig" > > +source "drivers/i3c/Kconfig" > + > source "drivers/spi/Kconfig" > > source "drivers/spmi/Kconfig" > diff --git a/drivers/Makefile b/drivers/Makefile > index 24cd47014657..999239dc29d4 100644 > --- a/drivers/Makefile > +++ b/drivers/Makefile > @@ -111,7 +111,7 @@ obj-$(CONFIG_SERIO) += input/serio/ > obj-$(CONFIG_GAMEPORT) += input/gameport/ > obj-$(CONFIG_INPUT) += input/ > obj-$(CONFIG_RTC_LIB) += rtc/ > -obj-y += i2c/ media/ > +obj-y += i2c/ i3c/ media/ > obj-$(CONFIG_PPS) += pps/ > obj-y += ptp/ > obj-$(CONFIG_W1) += w1/ > diff --git a/drivers/i3c/Kconfig b/drivers/i3c/Kconfig > new file mode 100644 > index 000000000000..30a441506f61 > --- /dev/null > +++ b/drivers/i3c/Kconfig > @@ -0,0 +1,24 @@ > +# SPDX-License-Identifier: GPL-2.0 > + > +menuconfig I3C > + tristate "I3C support" > + select I2C > + help > + I3C is a serial protocol standardized by the MIPI alliance. > + > + It's supposed to be backward compatible with I2C while providing > + support for high speed transfers and native interrupt support > + without the need for extra pins. > + > + The I3C protocol also standardizes the slave device types and is > + mainly designed to communicate with sensors. > + > + If you want I3C support, you should say Y here and also to the > + specific driver for your bus adapter(s) below. > + > + This I3C support can also be built as a module. If so, the module > + will be called i3c. > + > +if I3C > +source "drivers/i3c/master/Kconfig" > +endif # I3C > diff --git a/drivers/i3c/Makefile b/drivers/i3c/Makefile > new file mode 100644 > index 000000000000..3b6d1502d6e6 > --- /dev/null > +++ b/drivers/i3c/Makefile > @@ -0,0 +1,4 @@ > +# SPDX-License-Identifier: GPL-2.0 > +i3c-y := core.o device.o master.o > +obj-$(CONFIG_I3C) += i3c.o > +obj-$(CONFIG_I3C) += master/ > diff --git a/drivers/i3c/core.c b/drivers/i3c/core.c > new file mode 100644 > index 000000000000..5c62192ff876 > --- /dev/null > +++ b/drivers/i3c/core.c > @@ -0,0 +1,606 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2018 Cadence Design Systems Inc. > + * > + * Author: Boris Brezillon <boris.brezillon@bootlin.com> > + */ > + > +#include <linux/device.h> > +#include <linux/idr.h> > +#include <linux/init.h> > +#include <linux/list.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of_device.h> > +#include <linux/rwsem.h> > +#include <linux/slab.h> > + > +#include "internals.h" > + > +static DEFINE_IDR(i3c_bus_idr); > +static DEFINE_MUTEX(i3c_core_lock); > + > +/** > + * i3c_bus_maintenance_lock - Lock the bus for a maintenance operation > + * @bus: I3C bus to take the lock on > + * > + * This function takes the bus lock so that no other operations can occur on > + * the bus. This is needed for all kind of bus maintenance operation, like > + * - enabling/disabling slave events > + * - re-triggering DAA > + * - changing the dynamic address of a device > + * - relinquishing mastership > + * - ... > + * > + * The reason for this kind of locking is that we don't want drivers and core > + * logic to rely on I3C device information that could be changed behind their > + * back. > + */ > +void i3c_bus_maintenance_lock(struct i3c_bus *bus) > +{ > + down_write(&bus->lock); > +} > +EXPORT_SYMBOL_GPL(i3c_bus_maintenance_lock); > + > +/** > + * i3c_bus_maintenance_unlock - Release the bus lock after a maintenance > + * operation > + * @bus: I3C bus to release the lock on > + * > + * Should be called when the bus maintenance operation is done. See > + * i3c_bus_maintenance_lock() for more details on what these > maintenance > + * operations are. > + */ > +void i3c_bus_maintenance_unlock(struct i3c_bus *bus) > +{ > + up_write(&bus->lock); > +} > +EXPORT_SYMBOL_GPL(i3c_bus_maintenance_unlock); > + > +/** > + * i3c_bus_normaluse_lock - Lock the bus for a normal operation > + * @bus: I3C bus to take the lock on > + * > + * This function takes the bus lock for any operation that is not a > maintenance > + * operation (see i3c_bus_maintenance_lock() for a non-exhaustive list of > + * maintenance operations). Basically all communications with I3C devices > are > + * normal operations (HDR, SDR transfers or CCC commands that do not > change bus > + * state or I3C dynamic address). > + * > + * Note that this lock is not guaranteeing serialization of normal operations. > + * In other words, transfer requests passed to the I3C master can be > submitted > + * in parallel and I3C master drivers have to use their own locking to make > + * sure two different communications are not inter-mixed, or access to the > + * output/input queue is not done while the engine is busy. > + */ > +void i3c_bus_normaluse_lock(struct i3c_bus *bus) > +{ > + down_read(&bus->lock); > +} > +EXPORT_SYMBOL_GPL(i3c_bus_normaluse_lock); > + > +/** > + * i3c_bus_normaluse_unlock - Release the bus lock after a normal > operation > + * @bus: I3C bus to release the lock on > + * > + * Should be called when a normal operation is done. See > + * i3c_bus_normaluse_lock() for more details on what these normal > operations > + * are. > + */ > +void i3c_bus_normaluse_unlock(struct i3c_bus *bus) > +{ > + up_read(&bus->lock); > +} > +EXPORT_SYMBOL_GPL(i3c_bus_normaluse_unlock); > + > +static ssize_t bcr_show(struct device *dev, > + struct device_attribute *da, > + char *buf) > +{ > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > + ssize_t ret; > + > + i3c_bus_normaluse_lock(i3cdev->bus); > + ret = sprintf(buf, "%x\n", i3cdev->desc->info.bcr); > + i3c_bus_normaluse_unlock(i3cdev->bus); > + > + return ret; > +} > +static DEVICE_ATTR_RO(bcr); > + > +static ssize_t dcr_show(struct device *dev, > + struct device_attribute *da, > + char *buf) > +{ > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > + ssize_t ret; > + > + i3c_bus_normaluse_lock(i3cdev->bus); > + ret = sprintf(buf, "%x\n", i3cdev->desc->info.dcr); > + i3c_bus_normaluse_unlock(i3cdev->bus); > + > + return ret; > +} > +static DEVICE_ATTR_RO(dcr); > + > +static ssize_t pid_show(struct device *dev, > + struct device_attribute *da, > + char *buf) > +{ > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > + ssize_t ret; > + > + i3c_bus_normaluse_lock(i3cdev->bus); > + ret = sprintf(buf, "%llx\n", i3cdev->desc->info.pid); > + i3c_bus_normaluse_unlock(i3cdev->bus); > + > + return ret; > +} > +static DEVICE_ATTR_RO(pid); > + > +static ssize_t dynamic_address_show(struct device *dev, > + struct device_attribute *da, > + char *buf) > +{ > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > + ssize_t ret; > + > + i3c_bus_normaluse_lock(i3cdev->bus); > + ret = sprintf(buf, "%02x\n", i3cdev->desc->info.dyn_addr); > + i3c_bus_normaluse_unlock(i3cdev->bus); > + > + return ret; > +} > +static DEVICE_ATTR_RO(dynamic_address); > + > +static const char * const hdrcap_strings[] = { > + "hdr-ddr", "hdr-tsp", "hdr-tsl", > +}; > + > +static ssize_t hdrcap_show(struct device *dev, > + struct device_attribute *da, > + char *buf) > +{ > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > + ssize_t offset = 0, ret; > + unsigned long caps; > + int mode; > + > + i3c_bus_normaluse_lock(i3cdev->bus); > + caps = i3cdev->desc->info.hdr_cap; > + for_each_set_bit(mode, &caps, 8) { > + if (mode >= ARRAY_SIZE(hdrcap_strings)) > + break; > + > + if (!hdrcap_strings[mode]) > + continue; > + > + ret = sprintf(buf + offset, offset ? " %s" : "%s", > + hdrcap_strings[mode]); > + if (ret < 0) > + goto out; > + > + offset += ret; > + } > + > + ret = sprintf(buf + offset, "\n"); > + if (ret < 0) > + goto out; > + > + ret = offset + ret; > + > +out: > + i3c_bus_normaluse_unlock(i3cdev->bus); > + > + return ret; > +} > +static DEVICE_ATTR_RO(hdrcap); > + > +static struct attribute *i3c_device_attrs[] = { > + &dev_attr_bcr.attr, > + &dev_attr_dcr.attr, > + &dev_attr_pid.attr, > + &dev_attr_dynamic_address.attr, > + &dev_attr_hdrcap.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(i3c_device); > + > +static int i3c_device_uevent(struct device *dev, struct kobj_uevent_env > *env) > +{ > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > + struct i3c_device_info devinfo; > + u16 manuf, part, ext; > + > + i3c_device_get_info(i3cdev, &devinfo); > + manuf = I3C_PID_MANUF_ID(devinfo.pid); > + part = I3C_PID_PART_ID(devinfo.pid); > + ext = I3C_PID_EXTRA_INFO(devinfo.pid); > + > + if (I3C_PID_RND_LOWER_32BITS(devinfo.pid)) > + return add_uevent_var(env, > "MODALIAS=i3c:dcr%02Xmanuf%04X", > + devinfo.dcr, manuf); > + > + return add_uevent_var(env, > + > "MODALIAS=i3c:dcr%02Xmanuf%04Xpart%04xext%04x", > + devinfo.dcr, manuf, part, ext); > +} > + > +const struct device_type i3c_device_type = { > + .groups = i3c_device_groups, > + .uevent = i3c_device_uevent, > +}; > + > +const struct device_type i3c_master_type = { > + .groups = i3c_device_groups, > +}; > + > +static const struct i3c_device_id * > +i3c_device_match_id(struct i3c_device *i3cdev, > + const struct i3c_device_id *id_table) > +{ > + struct i3c_device_info devinfo; > + const struct i3c_device_id *id; > + > + i3c_device_get_info(i3cdev, &devinfo); > + > + /* > + * The lower 32bits of the provisional ID is just filled with a random > + * value, try to match using DCR info. > + */ > + if (!I3C_PID_RND_LOWER_32BITS(devinfo.pid)) { > + u16 manuf = I3C_PID_MANUF_ID(devinfo.pid); > + u16 part = I3C_PID_PART_ID(devinfo.pid); > + u16 ext_info = I3C_PID_EXTRA_INFO(devinfo.pid); > + > + /* First try to match by manufacturer/part ID. */ > + for (id = id_table; id->match_flags != 0; id++) { > + if ((id->match_flags & > I3C_MATCH_MANUF_AND_PART) != > + I3C_MATCH_MANUF_AND_PART) > + continue; > + > + if (manuf != id->manuf_id || part != id->part_id) > + continue; > + > + if ((id->match_flags & I3C_MATCH_EXTRA_INFO) && > + ext_info != id->extra_info) > + continue; > + > + return id; > + } > + } > + > + /* Fallback to DCR match. */ > + for (id = id_table; id->match_flags != 0; id++) { > + if ((id->match_flags & I3C_MATCH_DCR) && > + id->dcr == devinfo.dcr) > + return id; > + } > + > + return NULL; > +} > + > +static int i3c_device_match(struct device *dev, struct device_driver *drv) > +{ > + struct i3c_device *i3cdev; > + struct i3c_driver *i3cdrv; > + > + if (dev->type != &i3c_device_type) > + return 0; > + > + i3cdev = dev_to_i3cdev(dev); > + i3cdrv = drv_to_i3cdrv(drv); > + if (i3c_device_match_id(i3cdev, i3cdrv->id_table)) > + return 1; > + > + return 0; > +} > + > +static int i3c_device_probe(struct device *dev) > +{ > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > + struct i3c_driver *driver = drv_to_i3cdrv(dev->driver); > + > + return driver->probe(i3cdev); > +} > + > +static int i3c_device_remove(struct device *dev) > +{ > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > + struct i3c_driver *driver = drv_to_i3cdrv(dev->driver); > + int ret; > + > + ret = driver->remove(i3cdev); > + if (ret) > + return ret; > + > + i3c_device_free_ibi(i3cdev); > + > + return ret; > +} > + > +struct bus_type i3c_bus_type = { > + .name = "i3c", > + .match = i3c_device_match, > + .probe = i3c_device_probe, > + .remove = i3c_device_remove, > +}; > + > +enum i3c_addr_slot_status i3c_bus_get_addr_slot_status(struct i3c_bus > *bus, > + u16 addr) > +{ > + int status, bitpos = addr * 2; > + > + if (addr > I2C_MAX_ADDR) > + return I3C_ADDR_SLOT_RSVD; > + > + status = bus->addrslots[bitpos / BITS_PER_LONG]; > + status >>= bitpos % BITS_PER_LONG; > + > + return status & I3C_ADDR_SLOT_STATUS_MASK; > +} > + > +void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr, > + enum i3c_addr_slot_status status) > +{ > + int bitpos = addr * 2; > + unsigned long *ptr; > + > + if (addr > I2C_MAX_ADDR) > + return; > + > + ptr = bus->addrslots + (bitpos / BITS_PER_LONG); > + *ptr &= ~(I3C_ADDR_SLOT_STATUS_MASK << (bitpos % > BITS_PER_LONG)); > + *ptr |= status << (bitpos % BITS_PER_LONG); > +} > + > +bool i3c_bus_dev_addr_is_avail(struct i3c_bus *bus, u8 addr) > +{ > + enum i3c_addr_slot_status status; > + > + status = i3c_bus_get_addr_slot_status(bus, addr); > + > + return status == I3C_ADDR_SLOT_FREE; > +} > + > +int i3c_bus_get_free_addr(struct i3c_bus *bus, u8 start_addr) > +{ > + enum i3c_addr_slot_status status; > + u8 addr; > + > + for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) { > + status = i3c_bus_get_addr_slot_status(bus, addr); > + if (status == I3C_ADDR_SLOT_FREE) > + return addr; > + } > + > + return -ENOMEM; > +} > + > +static void i3c_bus_init_addrslots(struct i3c_bus *bus) > +{ > + int i; > + > + /* Addresses 0 to 7 are reserved. */ > + for (i = 0; i < 8; i++) > + i3c_bus_set_addr_slot_status(bus, i, > I3C_ADDR_SLOT_RSVD); > + > + /* > + * Reserve broadcast address and all addresses that might collide > + * with the broadcast address when facing a single bit error. > + */ > + i3c_bus_set_addr_slot_status(bus, I3C_BROADCAST_ADDR, > + I3C_ADDR_SLOT_RSVD); > + for (i = 0; i < 7; i++) > + i3c_bus_set_addr_slot_status(bus, I3C_BROADCAST_ADDR ^ > BIT(i), > + I3C_ADDR_SLOT_RSVD); > +} > + > +static const char * const i3c_bus_mode_strings[] = { > + [I3C_BUS_MODE_PURE] = "pure", > + [I3C_BUS_MODE_MIXED_FAST] = "mixed-fast", > + [I3C_BUS_MODE_MIXED_SLOW] = "mixed-slow", > +}; > + > +static ssize_t mode_show(struct device *dev, > + struct device_attribute *da, > + char *buf) > +{ > + struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev); > + ssize_t ret; > + > + i3c_bus_normaluse_lock(i3cbus); > + if (i3cbus->mode < 0 || > + i3cbus->mode > ARRAY_SIZE(i3c_bus_mode_strings) || > + !i3c_bus_mode_strings[i3cbus->mode]) > + ret = sprintf(buf, "unknown\n"); > + else > + ret = sprintf(buf, "%s\n", i3c_bus_mode_strings[i3cbus- > >mode]); > + i3c_bus_normaluse_unlock(i3cbus); > + > + return ret; > +} > +static DEVICE_ATTR_RO(mode); > + > +static ssize_t current_master_show(struct device *dev, > + struct device_attribute *da, > + char *buf) > +{ > + struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev); > + ssize_t ret; > + > + i3c_bus_normaluse_lock(i3cbus); > + ret = sprintf(buf, "%d-%llx\n", i3cbus->id, > + i3cbus->cur_master->info.pid); > + i3c_bus_normaluse_unlock(i3cbus); > + > + return ret; > +} > +static DEVICE_ATTR_RO(current_master); > + > +static ssize_t i3c_scl_frequency_show(struct device *dev, > + struct device_attribute *da, > + char *buf) > +{ > + struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev); > + ssize_t ret; > + > + i3c_bus_normaluse_lock(i3cbus); > + ret = sprintf(buf, "%ld\n", i3cbus->scl_rate.i3c); > + i3c_bus_normaluse_unlock(i3cbus); > + > + return ret; > +} > +static DEVICE_ATTR_RO(i3c_scl_frequency); > + > +static ssize_t i2c_scl_frequency_show(struct device *dev, > + struct device_attribute *da, > + char *buf) > +{ > + struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev); > + ssize_t ret; > + > + i3c_bus_normaluse_lock(i3cbus); > + ret = sprintf(buf, "%ld\n", i3cbus->scl_rate.i2c); > + i3c_bus_normaluse_unlock(i3cbus); > + > + return ret; > +} > +static DEVICE_ATTR_RO(i2c_scl_frequency); > + > +static struct attribute *i3c_busdev_attrs[] = { > + &dev_attr_mode.attr, > + &dev_attr_current_master.attr, > + &dev_attr_i3c_scl_frequency.attr, > + &dev_attr_i2c_scl_frequency.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(i3c_busdev); > + > +static void i3c_busdev_release(struct device *dev) > +{ > + struct i3c_bus *bus = container_of(dev, struct i3c_bus, dev); > + > + WARN_ON(!list_empty(&bus->devs.i2c) || !list_empty(&bus- > >devs.i3c)); > + > + mutex_lock(&i3c_core_lock); > + idr_remove(&i3c_bus_idr, bus->id); > + mutex_unlock(&i3c_core_lock); > + > + of_node_put(bus->dev.of_node); > + kfree(bus); > +} > + > +static const struct device_type i3c_busdev_type = { > + .groups = i3c_busdev_groups, > +}; > + > +void i3c_bus_unref(struct i3c_bus *bus) > +{ > + put_device(&bus->dev); > +} > + > +struct i3c_bus *i3c_bus_create(struct device *parent) > +{ > + struct i3c_bus *i3cbus; > + int ret; > + > + i3cbus = kzalloc(sizeof(*i3cbus), GFP_KERNEL); > + if (!i3cbus) > + return ERR_PTR(-ENOMEM); > + > + init_rwsem(&i3cbus->lock); > + INIT_LIST_HEAD(&i3cbus->devs.i2c); > + INIT_LIST_HEAD(&i3cbus->devs.i3c); > + i3c_bus_init_addrslots(i3cbus); > + i3cbus->mode = I3C_BUS_MODE_PURE; > + i3cbus->dev.parent = parent; > + i3cbus->dev.of_node = of_node_get(parent->of_node); > + i3cbus->dev.bus = &i3c_bus_type; > + i3cbus->dev.type = &i3c_busdev_type; > + i3cbus->dev.release = i3c_busdev_release; > + > + mutex_lock(&i3c_core_lock); > + ret = idr_alloc(&i3c_bus_idr, i3cbus, 0, 0, GFP_KERNEL); > + mutex_unlock(&i3c_core_lock); > + if (ret < 0) > + goto err_free_bus; > + > + i3cbus->id = ret; > + device_initialize(&i3cbus->dev); > + > + return i3cbus; > + > +err_free_bus: > + kfree(i3cbus); > + > + return ERR_PTR(ret); > +} > + > +void i3c_bus_unregister(struct i3c_bus *bus) > +{ > + device_unregister(&bus->dev); > +} > + > +int i3c_bus_register(struct i3c_bus *i3cbus) > +{ > + struct i2c_dev_desc *desc; > + > + i3c_bus_for_each_i2cdev(i3cbus, desc) { > + switch (desc->boardinfo->lvr & I3C_LVR_I2C_INDEX_MASK) { > + case I3C_LVR_I2C_INDEX(0): > + if (i3cbus->mode < I3C_BUS_MODE_MIXED_FAST) > + i3cbus->mode = > I3C_BUS_MODE_MIXED_FAST; > + break; > + > + case I3C_LVR_I2C_INDEX(1): > + case I3C_LVR_I2C_INDEX(2): > + if (i3cbus->mode < I3C_BUS_MODE_MIXED_SLOW) > + i3cbus->mode = > I3C_BUS_MODE_MIXED_SLOW; > + break; > + > + default: > + return -EINVAL; > + } > + } > + > + if (!i3cbus->scl_rate.i3c) > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > + > + if (!i3cbus->scl_rate.i2c) { > + if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW) > + i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; > + else > + i3cbus->scl_rate.i2c = > I3C_BUS_I2C_FM_PLUS_SCL_RATE; > + } > + > + /* > + * I3C/I2C frequency may have been overridden, check that user- > provided > + * values are not exceeding max possible frequency. > + */ > + if (i3cbus->scl_rate.i3c > I3C_BUS_MAX_I3C_SCL_RATE || > + i3cbus->scl_rate.i2c > I3C_BUS_I2C_FM_PLUS_SCL_RATE) { > + return -EINVAL; > + } > + > + dev_set_name(&i3cbus->dev, "i3c-%d", i3cbus->id); > + > + return device_add(&i3cbus->dev); > +} > + > +static int __init i3c_init(void) > +{ > + return bus_register(&i3c_bus_type); > +} > +subsys_initcall(i3c_init); > + > +static void __exit i3c_exit(void) > +{ > + idr_destroy(&i3c_bus_idr); > + bus_unregister(&i3c_bus_type); > +} > +module_exit(i3c_exit); > + > +MODULE_AUTHOR("Boris Brezillon <boris.brezillon@bootlin.com>"); > +MODULE_DESCRIPTION("I3C core"); > +MODULE_LICENSE("GPL v2"); > diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c > new file mode 100644 > index 000000000000..69cc040c3a1c > --- /dev/null > +++ b/drivers/i3c/device.c > @@ -0,0 +1,233 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2018 Cadence Design Systems Inc. > + * > + * Author: Boris Brezillon <boris.brezillon@bootlin.com> > + */ > + > +#include <linux/atomic.h> > +#include <linux/bug.h> > +#include <linux/completion.h> > +#include <linux/device.h> > +#include <linux/mutex.h> > +#include <linux/slab.h> > + > +#include "internals.h" > + > +/** > + * i3c_device_do_priv_xfers() - do I3C SDR private transfers directed to a > + * specific device > + * > + * @dev: device with which the transfers should be done > + * @xfers: array of transfers > + * @nxfers: number of transfers > + * > + * Initiate one or several private SDR transfers with @dev. > + * > + * This function can sleep and thus cannot be called in atomic context. > + * > + * Return: 0 in case of success, a negative error core otherwise. > + */ > +int i3c_device_do_priv_xfers(struct i3c_device *dev, > + struct i3c_priv_xfer *xfers, > + int nxfers) > +{ > + int ret, i; > + > + if (nxfers < 1) > + return 0; > + > + for (i = 0; i < nxfers; i++) { > + if (!xfers[i].len || !xfers[i].data.in) > + return -EINVAL; > + } > + > + i3c_bus_normaluse_lock(dev->bus); > + ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers); > + i3c_bus_normaluse_unlock(dev->bus); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers); > + > +/** > + * i3c_device_get_info() - get I3C device information > + * > + * @dev: device we want information on > + * @info: the information object to fill in > + * > + * Retrieve I3C dev info. > + */ > +void i3c_device_get_info(struct i3c_device *dev, > + struct i3c_device_info *info) > +{ > + if (!info) > + return; > + > + i3c_bus_normaluse_lock(dev->bus); > + if (dev->desc) > + *info = dev->desc->info; > + i3c_bus_normaluse_unlock(dev->bus); > +} > +EXPORT_SYMBOL_GPL(i3c_device_get_info); > + > +/** > + * i3c_device_disable_ibi() - Disable IBIs coming from a specific device > + * @dev: device on which IBIs should be disabled > + * > + * This function disable IBIs coming from a specific device and wait for > + * all pending IBIs to be processed. > + * > + * Return: 0 in case of success, a negative error core otherwise. > + */ > +int i3c_device_disable_ibi(struct i3c_device *dev) > +{ > + int ret = -ENOENT; > + > + i3c_bus_normaluse_lock(dev->bus); > + if (dev->desc) { > + mutex_lock(&dev->desc->ibi_lock); > + ret = i3c_dev_disable_ibi_locked(dev->desc); > + mutex_unlock(&dev->desc->ibi_lock); > + } > + i3c_bus_normaluse_unlock(dev->bus); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(i3c_device_disable_ibi); > + > +/** > + * i3c_device_enable_ibi() - Enable IBIs coming from a specific device > + * @dev: device on which IBIs should be enabled > + * > + * This function enable IBIs coming from a specific device and wait for > + * all pending IBIs to be processed. This should be called on a device > + * where i3c_device_request_ibi() has succeeded. > + * > + * Note that IBIs from this device might be received before this function > + * returns to its caller. > + * > + * Return: 0 in case of success, a negative error core otherwise. > + */ > +int i3c_device_enable_ibi(struct i3c_device *dev) > +{ > + int ret = -ENOENT; > + > + i3c_bus_normaluse_lock(dev->bus); > + if (dev->desc) { > + mutex_lock(&dev->desc->ibi_lock); > + ret = i3c_dev_enable_ibi_locked(dev->desc); > + mutex_unlock(&dev->desc->ibi_lock); > + } > + i3c_bus_normaluse_unlock(dev->bus); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(i3c_device_enable_ibi); > + > +/** > + * i3c_device_request_ibi() - Request an IBI > + * @dev: device for which we should enable IBIs > + * @req: setup requested for this IBI > + * > + * This function is responsible for pre-allocating all resources needed to > + * process IBIs coming from @dev. When this function returns, the IBI is not > + * enabled until i3c_device_enable_ibi() is called. > + * > + * Return: 0 in case of success, a negative error core otherwise. > + */ > +int i3c_device_request_ibi(struct i3c_device *dev, > + const struct i3c_ibi_setup *req) > +{ > + int ret = -ENOENT; > + > + if (!req->handler || !req->num_slots) > + return -EINVAL; > + > + i3c_bus_normaluse_lock(dev->bus); > + if (dev->desc) { > + mutex_lock(&dev->desc->ibi_lock); > + ret = i3c_dev_request_ibi_locked(dev->desc, req); > + mutex_unlock(&dev->desc->ibi_lock); > + } > + i3c_bus_normaluse_unlock(dev->bus); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(i3c_device_request_ibi); > + > +/** > + * i3c_device_free_ibi() - Free all resources needed for IBI handling > + * @dev: device on which you want to release IBI resources > + * > + * This function is responsible for de-allocating resources previously > + * allocated by i3c_device_request_ibi(). It should be called after disabling > + * IBIs with i3c_device_disable_ibi(). > + */ > +void i3c_device_free_ibi(struct i3c_device *dev) > +{ > + i3c_bus_normaluse_lock(dev->bus); > + if (dev->desc) { > + mutex_lock(&dev->desc->ibi_lock); > + i3c_dev_free_ibi_locked(dev->desc); > + mutex_unlock(&dev->desc->ibi_lock); > + } > + i3c_bus_normaluse_unlock(dev->bus); > +} > +EXPORT_SYMBOL_GPL(i3c_device_free_ibi); > + > +/** > + * i3cdev_to_dev() - Returns the device embedded in @i3cdev > + * @i3cdev: I3C device > + * > + * Return: a pointer to a device object. > + */ > +struct device *i3cdev_to_dev(struct i3c_device *i3cdev) > +{ > + return &i3cdev->dev; > +} > +EXPORT_SYMBOL_GPL(i3cdev_to_dev); > + > +/** > + * dev_to_i3cdev() - Returns the I3C device containing @dev > + * @dev: device object > + * > + * Return: a pointer to an I3C device object. > + */ > +struct i3c_device *dev_to_i3cdev(struct device *dev) > +{ > + return container_of(dev, struct i3c_device, dev); > +} > +EXPORT_SYMBOL_GPL(dev_to_i3cdev); > + > +/** > + * i3c_driver_register_with_owner() - register an I3C device driver > + * > + * @drv: driver to register > + * @owner: module that owns this driver > + * > + * Register @drv to the core. > + * > + * Return: 0 in case of success, a negative error core otherwise. > + */ > +int i3c_driver_register_with_owner(struct i3c_driver *drv, struct module > *owner) > +{ > + drv->driver.owner = owner; > + drv->driver.bus = &i3c_bus_type; > + > + return driver_register(&drv->driver); > +} > +EXPORT_SYMBOL_GPL(i3c_driver_register_with_owner); > + > +/** > + * i3c_driver_unregister() - unregister an I3C device driver > + * > + * @drv: driver to unregister > + * > + * Unregister @drv. > + */ > +void i3c_driver_unregister(struct i3c_driver *drv) > +{ > + driver_unregister(&drv->driver); > +} > +EXPORT_SYMBOL_GPL(i3c_driver_unregister); > diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h > new file mode 100644 > index 000000000000..ced88435466f > --- /dev/null > +++ b/drivers/i3c/internals.h > @@ -0,0 +1,36 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2018 Cadence Design Systems Inc. > + * > + * Author: Boris Brezillon <boris.brezillon@bootlin.com> > + */ > + > +#ifndef I3C_INTERNALS_H > +#define I3C_INTERNALS_H > + > +#include <linux/i3c/master.h> > + > +extern struct bus_type i3c_bus_type; > +extern const struct device_type i3c_master_type; > +extern const struct device_type i3c_device_type; > + > +void i3c_bus_unref(struct i3c_bus *bus); > +struct i3c_bus *i3c_bus_create(struct device *parent); > +void i3c_bus_unregister(struct i3c_bus *bus); > +int i3c_bus_register(struct i3c_bus *i3cbus); > +int i3c_bus_get_free_addr(struct i3c_bus *bus, u8 start_addr); > +bool i3c_bus_dev_addr_is_avail(struct i3c_bus *bus, u8 addr); > +void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr, > + enum i3c_addr_slot_status status); > +enum i3c_addr_slot_status i3c_bus_get_addr_slot_status(struct i3c_bus > *bus, > + u16 addr); > + > +int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, > + struct i3c_priv_xfer *xfers, > + int nxfers); > +int i3c_dev_disable_ibi_locked(struct i3c_dev_desc *dev); > +int i3c_dev_enable_ibi_locked(struct i3c_dev_desc *dev); > +int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev, > + const struct i3c_ibi_setup *req); > +void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev); > +#endif /* I3C_INTERNAL_H */ > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > new file mode 100644 > index 000000000000..d04e9ff9c850 > --- /dev/null > +++ b/drivers/i3c/master.c > @@ -0,0 +1,2058 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2018 Cadence Design Systems Inc. > + * > + * Author: Boris Brezillon <boris.brezillon@bootlin.com> > + */ > + > +#include <linux/atomic.h> > +#include <linux/bug.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/export.h> > +#include <linux/kernel.h> > +#include <linux/list.h> > +#include <linux/of.h> > +#include <linux/slab.h> > +#include <linux/spinlock.h> > +#include <linux/workqueue.h> > + > +#include "internals.h" > + > +static struct i3c_master_controller * > +i2c_adapter_to_i3c_master(struct i2c_adapter *adap) > +{ > + return container_of(adap, struct i3c_master_controller, i2c); > +} > + > +static struct i2c_adapter * > +i3c_master_to_i2c_adapter(struct i3c_master_controller *master) > +{ > + return &master->i2c; > +} > + > +static void i3c_master_free_i2c_dev(struct i2c_dev_desc *dev) > +{ > + kfree(dev); > +} > + > +static struct i2c_dev_desc * > +i3c_master_alloc_i2c_dev(struct i3c_master_controller *master, > + const struct i2c_dev_boardinfo *boardinfo) > +{ > + struct i2c_dev_desc *dev; > + > + dev = kzalloc(sizeof(*dev), GFP_KERNEL); > + if (!dev) > + return ERR_PTR(-ENOMEM); > + > + dev->common.master = master; > + dev->boardinfo = boardinfo; > + > + return dev; > +} > + > +static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller > *master, > + struct i3c_ccc_cmd *cmd) > +{ > + int ret; > + > + if (!cmd || !master) > + return -EINVAL; > + > + if (WARN_ON(master->init_done && > + !rwsem_is_locked(&master->bus->lock))) > + return -EINVAL; > + > + if (!master->ops->send_ccc_cmd) > + return -ENOTSUPP; > + > + if ((cmd->id & I3C_CCC_DIRECT) && (!cmd->dests || !cmd->ndests)) > + return -EINVAL; > + > + if (master->ops->supports_ccc_cmd && > + !master->ops->supports_ccc_cmd(master, cmd)) > + return -ENOTSUPP; > + > + ret = master->ops->send_ccc_cmd(master, cmd); > + if (ret) { > + if (cmd->err != I3C_ERROR_UNKNOWN) > + return cmd->err; > + > + return ret; > + } > + > + return 0; > +} > + > +static struct i2c_dev_desc * > +i3c_master_find_i2c_dev_by_addr(const struct i3c_master_controller > *master, > + u16 addr) > +{ > + struct i2c_dev_desc *dev; > + > + i3c_bus_for_each_i2cdev(master->bus, dev) { > + if (dev->boardinfo->base.addr == addr) > + return dev; > + } > + > + return NULL; > +} > + > +/** > + * i3c_master_get_free_addr() - get a free address on the bus > + * @master: I3C master object > + * @start_addr: where to start searching > + * > + * This function must be called with the bus lock held in write mode. > + * > + * Return: the first free address starting at @start_addr (included) or - > ENOMEM > + * if there's no more address available. > + */ > +int i3c_master_get_free_addr(struct i3c_master_controller *master, > + u8 start_addr) > +{ > + return i3c_bus_get_free_addr(master->bus, start_addr); > +} > +EXPORT_SYMBOL_GPL(i3c_master_get_free_addr); > + > +static void i3c_device_release(struct device *dev) > +{ > + struct i3c_device *i3cdev = dev_to_i3cdev(dev); > + > + WARN_ON(i3cdev->desc); > + > + of_node_put(i3cdev->dev.of_node); > + kfree(i3cdev); > +} > + > +static void i3c_master_free_i3c_dev(struct i3c_dev_desc *dev) > +{ > + kfree(dev); > +} > + > +static struct i3c_dev_desc * > +i3c_master_alloc_i3c_dev(struct i3c_master_controller *master, > + const struct i3c_device_info *info) > +{ > + struct i3c_dev_desc *dev; > + > + dev = kzalloc(sizeof(*dev), GFP_KERNEL); > + if (!dev) > + return ERR_PTR(-ENOMEM); > + > + dev->common.master = master; > + dev->info = *info; > + mutex_init(&dev->ibi_lock); > + > + return dev; > +} > + > +static int i3c_master_rstdaa_locked(struct i3c_master_controller *master, > + u8 addr) > +{ > + struct i3c_ccc_cmd_dest dest = { }; > + struct i3c_ccc_cmd cmd = { }; > + enum i3c_addr_slot_status addrstat; > + > + if (!master) > + return -EINVAL; > + > + addrstat = i3c_bus_get_addr_slot_status(master->bus, addr); > + if (addr != I3C_BROADCAST_ADDR && addrstat != > I3C_ADDR_SLOT_I3C_DEV) > + return -EINVAL; > + > + dest.addr = addr; > + cmd.dests = &dest; > + cmd.ndests = 1; > + cmd.rnw = false; > + cmd.id = I3C_CCC_RSTDAA(addr == I3C_BROADCAST_ADDR); > + > + return i3c_master_send_ccc_cmd_locked(master, &cmd); > +} > + > +/** > + * i3c_master_entdaa_locked() - start a DAA (Dynamic Address Assignment) > + * procedure > + * @master: master used to send frames on the bus > + * > + * Send a ENTDAA CCC command to start a DAA procedure. > + * > + * Note that this function only sends the ENTDAA CCC command, all the > logic > + * behind dynamic address assignment has to be handled in the I3C master > + * driver. > + * > + * This function must be called with the bus lock held in write mode. > + * > + * Return: 0 in case of success, a positive I3C error code if the error is > + * one of the official Mx error codes, and a negative error code otherwise. > + */ > +int i3c_master_entdaa_locked(struct i3c_master_controller *master) > +{ > + struct i3c_ccc_cmd_dest dest = { }; > + struct i3c_ccc_cmd cmd = { }; > + > + dest.addr = I3C_BROADCAST_ADDR; > + cmd.dests = &dest; > + cmd.ndests = 1; > + cmd.rnw = false; > + cmd.id = I3C_CCC_ENTDAA; > + > + return i3c_master_send_ccc_cmd_locked(master, &cmd); > +} > +EXPORT_SYMBOL_GPL(i3c_master_entdaa_locked); > + > +/** > + * i3c_master_disec_locked() - send a DISEC CCC command > + * @master: master used to send frames on the bus > + * @addr: a valid I3C slave address or %I3C_BROADCAST_ADDR > + * @evts: events to disable > + * > + * Send a DISEC CCC command to disable some or all events coming from a > + * specific slave, or all devices if @addr is %I3C_BROADCAST_ADDR. > + * > + * This function must be called with the bus lock held in write mode. > + * > + * Return: 0 in case of success, a positive I3C error code if the error is > + * one of the official Mx error codes, and a negative error code otherwise. > + */ > +int i3c_master_disec_locked(struct i3c_master_controller *master, u8 addr, > + u8 evts) > +{ > + struct i3c_ccc_events events = { > + .events = evts, > + }; > + struct i3c_ccc_cmd_dest dest = { > + .addr = addr, > + .payload.len = sizeof(events), > + .payload.data = &events, > + }; > + struct i3c_ccc_cmd cmd = { > + .id = I3C_CCC_DISEC(addr == I3C_BROADCAST_ADDR), > + .dests = &dest, > + .ndests = 1, > + }; > + > + return i3c_master_send_ccc_cmd_locked(master, &cmd); > +} > +EXPORT_SYMBOL_GPL(i3c_master_disec_locked); > + > +/** > + * i3c_master_enec_locked() - send an ENEC CCC command > + * @master: master used to send frames on the bus > + * @addr: a valid I3C slave address or %I3C_BROADCAST_ADDR > + * @evts: events to disable > + * > + * Sends an ENEC CCC command to enable some or all events coming from a > + * specific slave, or all devices if @addr is %I3C_BROADCAST_ADDR. > + * > + * This function must be called with the bus lock held in write mode. > + * > + * Return: 0 in case of success, a positive I3C error code if the error is > + * one of the official Mx error codes, and a negative error code otherwise. > + */ > +int i3c_master_enec_locked(struct i3c_master_controller *master, u8 addr, > + u8 evts) > +{ > + struct i3c_ccc_events events = { > + .events = evts, > + }; > + struct i3c_ccc_cmd_dest dest = { > + .addr = addr, > + .payload.len = sizeof(events), > + .payload.data = &events, > + }; > + struct i3c_ccc_cmd cmd = { > + .id = I3C_CCC_ENEC(addr == I3C_BROADCAST_ADDR), > + .dests = &dest, > + .ndests = 1, > + }; > + > + return i3c_master_send_ccc_cmd_locked(master, &cmd); > +} > +EXPORT_SYMBOL_GPL(i3c_master_enec_locked); > + > +/** > + * i3c_master_defslvs_locked() - send a DEFSLVS CCC command > + * @master: master used to send frames on the bus > + * > + * Send a DEFSLVS CCC command containing all the devices known to the > @master. > + * This is useful when you have secondary masters on the bus to propagate > + * device information. > + * > + * This should be called after all I3C devices have been discovered (in other > + * words, after the DAA procedure has finished) and instantiated in > + * &i3c_master_controller_ops->bus_init(). > + * It should also be called if a master ACKed an Hot-Join request and > assigned > + * a dynamic address to the device joining the bus. > + * > + * This function must be called with the bus lock held in write mode. > + * > + * Return: 0 in case of success, a positive I3C error code if the error is > + * one of the official Mx error codes, and a negative error code otherwise. > + */ > +int i3c_master_defslvs_locked(struct i3c_master_controller *master) > +{ > + struct i3c_ccc_cmd_dest dest = { > + .addr = I3C_BROADCAST_ADDR, > + }; > + struct i3c_ccc_cmd cmd = { > + .id = I3C_CCC_DEFSLVS, > + .dests = &dest, > + .ndests = 1, > + }; > + struct i3c_ccc_defslvs *defslvs; > + struct i3c_ccc_dev_desc *desc; > + struct i3c_dev_desc *i3cdev; > + struct i2c_dev_desc *i2cdev; > + struct i3c_bus *bus; > + bool send = false; > + int ndevs = 0, ret; > + > + if (!master) > + return -EINVAL; > + > + bus = i3c_master_get_bus(master); > + i3c_bus_for_each_i3cdev(bus, i3cdev) { > + ndevs++; > + > + if (i3cdev == master->this) > + continue; > + > + if (I3C_BCR_DEVICE_ROLE(i3cdev->info.bcr) == > + I3C_BCR_I3C_MASTER) > + send = true; > + } > + > + /* No other master on the bus, skip DEFSLVS. */ > + if (!send) > + return 0; > + > + i3c_bus_for_each_i2cdev(bus, i2cdev) > + ndevs++; > + > + dest.payload.len = sizeof(*defslvs) + > + ((ndevs - 1) * sizeof(struct i3c_ccc_dev_desc)); > + defslvs = kzalloc(dest.payload.len, GFP_KERNEL); > + if (!defslvs) > + return -ENOMEM; > + > + dest.payload.data = defslvs; > + > + defslvs->count = ndevs; > + defslvs->master.bcr = master->this->info.bcr; > + defslvs->master.dcr = master->this->info.dcr; > + defslvs->master.dyn_addr = master->this->info.dyn_addr << 1; > + defslvs->master.static_addr = I3C_BROADCAST_ADDR << 1; > + > + desc = defslvs->slaves; > + i3c_bus_for_each_i2cdev(bus, i2cdev) { > + desc->lvr = i2cdev->boardinfo->lvr; > + desc->static_addr = i2cdev->boardinfo->base.addr << 1; > + desc++; > + } > + > + i3c_bus_for_each_i3cdev(bus, i3cdev) { > + /* Skip the I3C dev representing this master. */ > + if (i3cdev == master->this) > + continue; > + > + desc->bcr = i3cdev->info.bcr; > + desc->dcr = i3cdev->info.dcr; > + desc->dyn_addr = i3cdev->info.dyn_addr << 1; > + desc->static_addr = i3cdev->info.static_addr << 1; > + desc++; > + } > + > + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); > + kfree(defslvs); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(i3c_master_defslvs_locked); > + > +static int i3c_master_setdasa_locked(struct i3c_master_controller *master, > + u8 static_addr, u8 dyn_addr) > +{ > + struct i3c_ccc_setda setda = { > + .addr = dyn_addr << 1, > + }; > + struct i3c_ccc_cmd_dest dest = { > + .addr = static_addr, > + .payload.len = sizeof(setda), > + .payload.data = &setda, > + }; > + struct i3c_ccc_cmd cmd = { > + .rnw = false, > + .id = I3C_CCC_SETDASA, > + .dests = &dest, > + .ndests = 1, > + }; > + > + if (!dyn_addr || !static_addr) > + return -EINVAL; > + > + return i3c_master_send_ccc_cmd_locked(master, &cmd); > +} > + > +static int i3c_master_setnewda_locked(struct i3c_master_controller > *master, > + u8 oldaddr, u8 newaddr) > +{ > + struct i3c_ccc_setda setda = { > + .addr = newaddr << 1, > + }; > + struct i3c_ccc_cmd_dest dest = { > + .addr = oldaddr, > + .payload.len = sizeof(setda), > + .payload.data = &setda, > + }; > + struct i3c_ccc_cmd cmd = { > + .rnw = false, > + .id = I3C_CCC_SETNEWDA, > + .dests = &dest, > + .ndests = 1, > + }; > + > + if (!oldaddr || !newaddr) > + return -EINVAL; > + > + return i3c_master_send_ccc_cmd_locked(master, &cmd); > +} > + > +static int i3c_master_getmrl_locked(struct i3c_master_controller *master, > + struct i3c_device_info *info) > +{ > + struct i3c_ccc_mrl mrl; > + struct i3c_ccc_cmd_dest dest = { > + .addr = info->dyn_addr, > + .payload.len = sizeof(mrl), > + .payload.data = &mrl, > + }; > + struct i3c_ccc_cmd cmd = { > + .rnw = true, > + .id = I3C_CCC_GETMRL, > + .dests = &dest, > + .ndests = 1, > + }; > + int ret; > + > + /* > + * When the device does not have IBI payload GETMRL only returns 2 > + * bytes of data. > + */ > + if (!(info->bcr & I3C_BCR_IBI_PAYLOAD)) > + dest.payload.len -= 1; > + > + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); > + if (ret) > + return ret; > + > + if (dest.payload.len != sizeof(mrl)) > + return -EIO; > + > + info->max_read_len = be16_to_cpu(mrl.read_len); > + > + if (info->bcr & I3C_BCR_IBI_PAYLOAD) > + info->max_ibi_len = mrl.ibi_len; > + > + return 0; > +} > + > +static int i3c_master_getmwl_locked(struct i3c_master_controller *master, > + struct i3c_device_info *info) > +{ > + struct i3c_ccc_mwl mwl; > + struct i3c_ccc_cmd_dest dest = { > + .addr = info->dyn_addr, > + .payload.len = sizeof(mwl), > + .payload.data = &mwl, > + }; > + struct i3c_ccc_cmd cmd = { > + .rnw = true, > + .id = I3C_CCC_GETMWL, > + .dests = &dest, > + .ndests = 1, > + }; > + int ret; > + > + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); > + if (ret) > + return ret; > + > + if (dest.payload.len != sizeof(mwl)) > + return -EIO; > + > + info->max_write_len = be16_to_cpu(mwl.len); > + > + return 0; > +} > + > +static int i3c_master_getmxds_locked(struct i3c_master_controller > *master, > + struct i3c_device_info *info) > +{ > + struct i3c_ccc_getmxds getmaxds; > + struct i3c_ccc_cmd_dest dest = { > + .addr = info->dyn_addr, > + .payload.len = sizeof(getmaxds), > + .payload.data = &getmaxds, > + }; > + struct i3c_ccc_cmd cmd = { > + .rnw = true, > + .id = I3C_CCC_GETMXDS, > + .dests = &dest, > + .ndests = 1, > + }; > + int ret; > + > + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); > + if (ret) > + return ret; > + > + if (dest.payload.len != 2 && dest.payload.len != 5) > + return -EIO; > + > + info->max_read_ds = getmaxds.maxrd; > + info->max_read_ds = getmaxds.maxwr; > + if (dest.payload.len == 5) > + info->max_read_turnaround = getmaxds.maxrdturn[0] | > + ((u32)getmaxds.maxrdturn[1] << 8) > | > + ((u32)getmaxds.maxrdturn[2] << > 16); > + > + return 0; > +} > + > +static int i3c_master_gethdrcap_locked(struct i3c_master_controller > *master, > + struct i3c_device_info *info) > +{ > + struct i3c_ccc_gethdrcap gethdrcap; > + struct i3c_ccc_cmd_dest dest = { > + .addr = info->dyn_addr, > + .payload.len = sizeof(gethdrcap), > + .payload.data = &gethdrcap, > + }; > + struct i3c_ccc_cmd cmd = { > + .rnw = true, > + .id = I3C_CCC_GETHDRCAP, > + .dests = &dest, > + .ndests = 1, > + }; > + int ret; > + > + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); > + if (ret) > + return ret; > + > + if (dest.payload.len != 1) > + return -EIO; > + > + info->hdr_cap = gethdrcap.modes; > + > + return 0; > +} > + > +static int i3c_master_getpid_locked(struct i3c_master_controller *master, > + struct i3c_device_info *info) > +{ > + struct i3c_ccc_getpid getpid; > + struct i3c_ccc_cmd_dest dest = { > + .addr = info->dyn_addr, > + .payload.len = sizeof(struct i3c_ccc_getpid), > + .payload.data = &getpid, > + }; > + struct i3c_ccc_cmd cmd = { > + .rnw = true, > + .id = I3C_CCC_GETPID, > + .dests = &dest, > + .ndests = 1, > + }; > + int ret, i; > + > + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); > + if (ret) > + return ret; > + > + info->pid = 0; > + for (i = 0; i < sizeof(getpid.pid); i++) { > + int sft = (sizeof(getpid.pid) - i - 1) * 8; > + > + info->pid |= (u64)getpid.pid[i] << sft; > + } > + > + return 0; > +} > + > +static int i3c_master_getbcr_locked(struct i3c_master_controller *master, > + struct i3c_device_info *info) > +{ > + struct i3c_ccc_getbcr getbcr; > + struct i3c_ccc_cmd_dest dest = { > + .addr = info->dyn_addr, > + .payload.len = sizeof(struct i3c_ccc_getbcr), > + .payload.data = &getbcr, > + }; > + struct i3c_ccc_cmd cmd = { > + .rnw = true, > + .id = I3C_CCC_GETBCR, > + .dests = &dest, > + .ndests = 1, > + }; > + int ret; > + > + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); > + if (ret) > + return ret; > + > + info->bcr = getbcr.bcr; > + > + return 0; > +} > + > +static int i3c_master_getdcr_locked(struct i3c_master_controller *master, > + struct i3c_device_info *info) > +{ > + struct i3c_ccc_getdcr getdcr; > + struct i3c_ccc_cmd_dest dest = { > + .addr = info->dyn_addr, > + .payload.len = sizeof(struct i3c_ccc_getdcr), > + .payload.data = &getdcr, > + }; > + struct i3c_ccc_cmd cmd = { > + .rnw = true, > + .id = I3C_CCC_GETDCR, > + .dests = &dest, > + .ndests = 1, > + }; > + int ret; > + > + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); > + if (ret) > + return ret; > + > + info->dcr = getdcr.dcr; > + > + return 0; > +} > + > +static int i3c_master_retrieve_dev_info(struct i3c_dev_desc *dev) > +{ > + struct i3c_master_controller *master = i3c_dev_get_master(dev); > + enum i3c_addr_slot_status slot_status; > + int ret; > + > + if (!dev->info.dyn_addr) > + return -EINVAL; > + > + slot_status = i3c_bus_get_addr_slot_status(master->bus, > + dev->info.dyn_addr); > + if (slot_status == I3C_ADDR_SLOT_RSVD || > + slot_status == I3C_ADDR_SLOT_I2C_DEV) > + return -EINVAL; > + > + ret = i3c_master_getpid_locked(master, &dev->info); > + if (ret) > + return ret; > + > + ret = i3c_master_getbcr_locked(master, &dev->info); > + if (ret) > + return ret; > + > + ret = i3c_master_getdcr_locked(master, &dev->info); > + if (ret) > + return ret; > + > + if (dev->info.bcr & I3C_BCR_MAX_DATA_SPEED_LIM) { > + ret = i3c_master_getmxds_locked(master, &dev->info); > + if (ret) > + return ret; > + } > + > + if (dev->info.bcr & I3C_BCR_IBI_PAYLOAD) > + dev->info.max_ibi_len = 1; > + > + i3c_master_getmrl_locked(master, &dev->info); > + i3c_master_getmwl_locked(master, &dev->info); > + > + if (dev->info.bcr & I3C_BCR_HDR_CAP) { > + ret = i3c_master_gethdrcap_locked(master, &dev->info); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +static void i3c_master_put_i3c_addrs(struct i3c_dev_desc *dev) > +{ > + struct i3c_master_controller *master = i3c_dev_get_master(dev); > + > + if (dev->info.static_addr) > + i3c_bus_set_addr_slot_status(master->bus, > + dev->info.static_addr, > + I3C_ADDR_SLOT_FREE); > + > + if (dev->info.dyn_addr) > + i3c_bus_set_addr_slot_status(master->bus, dev- > >info.dyn_addr, > + I3C_ADDR_SLOT_FREE); > + > + if (dev->boardinfo && dev->boardinfo->init_dyn_addr) > + i3c_bus_set_addr_slot_status(master->bus, dev- > >info.dyn_addr, > + I3C_ADDR_SLOT_FREE); > +} > + > +static int i3c_master_get_i3c_addrs(struct i3c_dev_desc *dev) > +{ > + struct i3c_master_controller *master = i3c_dev_get_master(dev); > + enum i3c_addr_slot_status status; > + > + if (!dev->info.static_addr && !dev->info.dyn_addr) > + return 0; > + > + if (dev->info.static_addr) { > + status = i3c_bus_get_addr_slot_status(master->bus, > + dev->info.static_addr); > + if (status != I3C_ADDR_SLOT_FREE) > + return -EBUSY; > + > + i3c_bus_set_addr_slot_status(master->bus, > + dev->info.static_addr, > + I3C_ADDR_SLOT_I3C_DEV); > + } > + > + /* > + * ->init_dyn_addr should have been reserved before that, so, if > we're > + * trying to apply a pre-reserved dynamic address, we should not try > + * to reserve the address slot a second time. > + */ > + if (dev->info.dyn_addr && > + (!dev->boardinfo || > + dev->boardinfo->init_dyn_addr != dev->info.dyn_addr)) { > + status = i3c_bus_get_addr_slot_status(master->bus, > + dev->info.dyn_addr); > + if (status != I3C_ADDR_SLOT_FREE) > + goto err_release_static_addr; > + > + i3c_bus_set_addr_slot_status(master->bus, dev- > >info.dyn_addr, > + I3C_ADDR_SLOT_I3C_DEV); > + } > + > + return 0; > + > +err_release_static_addr: > + if (dev->info.static_addr) > + i3c_bus_set_addr_slot_status(master->bus, > + dev->info.static_addr, > + I3C_ADDR_SLOT_FREE); > + > + return -EBUSY; > +} > + > +static int i3c_master_attach_i3c_dev(struct i3c_master_controller *master, > + struct i3c_dev_desc *dev) > +{ > + int ret; > + > + /* > + * We don't attach devices to the controller until they are > + * addressable on the bus. > + */ > + if (!dev->info.static_addr && !dev->info.dyn_addr) > + return 0; > + > + ret = i3c_master_get_i3c_addrs(dev); > + if (ret) > + return ret; > + > + /* Do not attach the master device itself. */ > + if (master->this != dev && master->ops->attach_i3c_dev) { > + ret = master->ops->attach_i3c_dev(dev); > + if (ret) { > + i3c_master_put_i3c_addrs(dev); > + return ret; > + } > + } > + > + list_add_tail(&dev->common.node, &master->bus->devs.i3c); > + > + return 0; > +} > + > +static int i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev, > + u8 old_dyn_addr) > +{ > + struct i3c_master_controller *master = i3c_dev_get_master(dev); > + enum i3c_addr_slot_status status; > + int ret; > + > + if (dev->info.dyn_addr != old_dyn_addr) { > + status = i3c_bus_get_addr_slot_status(master->bus, > + dev->info.dyn_addr); > + if (status != I3C_ADDR_SLOT_FREE) > + return -EBUSY; > + i3c_bus_set_addr_slot_status(master->bus, > + dev->info.dyn_addr, > + I3C_ADDR_SLOT_I3C_DEV); > + } > + > + if (master->ops->reattach_i3c_dev) { > + ret = master->ops->reattach_i3c_dev(dev, old_dyn_addr); > + if (ret) { > + i3c_master_put_i3c_addrs(dev); > + return ret; > + } > + } > + > + return 0; > +} > + > +static void i3c_master_detach_i3c_dev(struct i3c_dev_desc *dev) > +{ > + struct i3c_master_controller *master = i3c_dev_get_master(dev); > + > + /* Do not detach the master device itself. */ > + if (master->this != dev && master->ops->detach_i3c_dev) > + master->ops->detach_i3c_dev(dev); > + > + i3c_master_put_i3c_addrs(dev); > + list_del(&dev->common.node); > +} > + > +static int i3c_master_attach_i2c_dev(struct i3c_master_controller *master, > + struct i2c_dev_desc *dev) > +{ > + int ret; > + > + if (master->ops->attach_i2c_dev) { > + ret = master->ops->attach_i2c_dev(dev); > + if (ret) > + return ret; > + } > + > + list_add_tail(&dev->common.node, &master->bus->devs.i2c); > + > + return 0; > +} > + > +static void i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev) > +{ > + struct i3c_master_controller *master = i2c_dev_get_master(dev); > + > + list_del(&dev->common.node); > + > + if (master->ops->detach_i2c_dev) > + master->ops->detach_i2c_dev(dev); > +} > + > +static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) > +{ > + struct i3c_master_controller *master = i3c_dev_get_master(dev); > + struct i3c_device_info info; > + int ret; > + > + if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr || > + !dev->boardinfo->static_addr) > + return; > + > + ret = i3c_master_setdasa_locked(master, dev->info.static_addr, > + dev->boardinfo->init_dyn_addr); > + if (ret) > + return; > + > + dev->info.dyn_addr = dev->boardinfo->init_dyn_addr; > + ret = i3c_master_reattach_i3c_dev(dev, 0); > + if (ret) > + return; > + > + ret = i3c_master_retrieve_dev_info(dev); > + if (ret) > + goto err_rstdaa; > + > + dev->info = info; > + > + return; > + > +err_rstdaa: > + i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr); > +} > + > +static void > +i3c_master_register_new_i3c_devs(struct i3c_master_controller *master) > +{ > + struct i3c_dev_desc *desc; > + int ret; > + > + if (!master->init_done) > + return; > + > + i3c_bus_for_each_i3cdev(master->bus, desc) { > + if (desc->dev || !desc->info.dyn_addr) > + continue; > + > + desc->dev = kzalloc(sizeof(*desc->dev), GFP_KERNEL); > + if (!desc->dev) > + continue; > + > + desc->dev->bus = master->bus; > + desc->dev->desc = desc; > + desc->dev->dev.parent = &master->bus->dev; > + if (desc == master->this) > + desc->dev->dev.type = &i3c_master_type; > + else > + desc->dev->dev.type = &i3c_device_type; > + desc->dev->dev.bus = &i3c_bus_type; > + desc->dev->dev.release = i3c_device_release; > + dev_set_name(&desc->dev->dev, "%d-%llx", master->bus- > >id, > + desc->info.pid); > + > + if (desc->boardinfo) > + desc->dev->dev.of_node = desc->boardinfo- > >of_node; > + > + pr_info("%s:%i\n", __func__, __LINE__); > + if (ret) > + dev_err(master->parent, > + "Failed to add I3C device (err = %d)\n", ret); > + } > +} The compiler gives a warning that "ret" may be used uninitialized in the above function, which is true since "ret" is never assigned a value. Also, I wonder about the "pr_info" call here, should there be a more detailed message here about the device being registered, other than the file name and line number? Otherwise, I have gone through the code in the process of writing the first iteration of the Qualcomm I3C master driver, and overall the framework looks good. Will update with any additional feedback I have. -- Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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 Mike, On Fri, 3 Aug 2018 15:38:43 -0600 <mshettel@codeaurora.org> wrote: > > +static void > > +i3c_master_register_new_i3c_devs(struct i3c_master_controller *master) > > +{ > > + struct i3c_dev_desc *desc; > > + int ret; > > + > > + if (!master->init_done) > > + return; > > + > > + i3c_bus_for_each_i3cdev(master->bus, desc) { > > + if (desc->dev || !desc->info.dyn_addr) > > + continue; > > + > > + desc->dev = kzalloc(sizeof(*desc->dev), GFP_KERNEL); > > + if (!desc->dev) > > + continue; > > + > > + desc->dev->bus = master->bus; > > + desc->dev->desc = desc; > > + desc->dev->dev.parent = &master->bus->dev; > > + if (desc == master->this) > > + desc->dev->dev.type = &i3c_master_type; > > + else > > + desc->dev->dev.type = &i3c_device_type; > > + desc->dev->dev.bus = &i3c_bus_type; > > + desc->dev->dev.release = i3c_device_release; > > + dev_set_name(&desc->dev->dev, "%d-%llx", master->bus- > > >id, > > + desc->info.pid); > > + > > + if (desc->boardinfo) > > + desc->dev->dev.of_node = desc->boardinfo- > > >of_node; > > + > > + pr_info("%s:%i\n", __func__, __LINE__); > > + if (ret) > > + dev_err(master->parent, > > + "Failed to add I3C device (err = %d)\n", > ret); > > + } > > +} > > The compiler gives a warning that "ret" may be used uninitialized in the > above function, which is true since "ret" is never assigned a value. Also, > I wonder about the "pr_info" call here, should there be a more detailed > message here about the device being registered, other than the file name and > line number? Yes, kbuild already reported that problem. I messed up when selecting the lines to add to my commit and added this trace instead of ret = device_register(&desc->dev->dev); Will be fixed in v7. > > Otherwise, I have gone through the code in the process of writing the first > iteration of the Qualcomm I3C master driver, and overall the framework looks > good. Will update with any additional feedback I have. Thanks for looking at the framework code. Your driver is in my TODO list. 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
Hi Boris, On 19-07-2018 16:29, Boris Brezillon wrote: > +int i3c_bus_register(struct i3c_bus *i3cbus) > +{ > + struct i2c_dev_desc *desc; > + > + i3c_bus_for_each_i2cdev(i3cbus, desc) { > + switch (desc->boardinfo->lvr & I3C_LVR_I2C_INDEX_MASK) { > + case I3C_LVR_I2C_INDEX(0): > + if (i3cbus->mode < I3C_BUS_MODE_MIXED_FAST) > + i3cbus->mode = I3C_BUS_MODE_MIXED_FAST; > + break; > + > + case I3C_LVR_I2C_INDEX(1): > + case I3C_LVR_I2C_INDEX(2): > + if (i3cbus->mode < I3C_BUS_MODE_MIXED_SLOW) > + i3cbus->mode = I3C_BUS_MODE_MIXED_SLOW; > + break; > + > + default: > + return -EINVAL; > + } > + } > + > + if (!i3cbus->scl_rate.i3c) > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > + > + if (!i3cbus->scl_rate.i2c) { > + if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW) > + i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; > + else > + i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE; > + } > + > + /* > + * I3C/I2C frequency may have been overridden, check that user-provided > + * values are not exceeding max possible frequency. > + */ > + if (i3cbus->scl_rate.i3c > I3C_BUS_MAX_I3C_SCL_RATE || > + i3cbus->scl_rate.i2c > I3C_BUS_I2C_FM_PLUS_SCL_RATE) { > + return -EINVAL; > + } > + > + dev_set_name(&i3cbus->dev, "i3c-%d", i3cbus->id); > + > + return device_add(&i3cbus->dev); > +} During the tests of the bus with i2c devices I found the i2c_dev_desc objects aren't allocated before this function. This cause i3cbus->mode = I3C_BUS_MODE_PURE. I want to do something for the slave and secondary master, do you already have infrastructure that you can share? Best regards, Vitor Soares
Hi Vitor, On Wed, 22 Aug 2018 17:43:34 +0100 vitor <Vitor.Soares@synopsys.com> wrote: > Hi Boris, > > > On 19-07-2018 16:29, Boris Brezillon wrote: > > +int i3c_bus_register(struct i3c_bus *i3cbus) > > +{ > > + struct i2c_dev_desc *desc; > > + > > + i3c_bus_for_each_i2cdev(i3cbus, desc) { > > + switch (desc->boardinfo->lvr & I3C_LVR_I2C_INDEX_MASK) { > > + case I3C_LVR_I2C_INDEX(0): > > + if (i3cbus->mode < I3C_BUS_MODE_MIXED_FAST) > > + i3cbus->mode = I3C_BUS_MODE_MIXED_FAST; > > + break; > > + > > + case I3C_LVR_I2C_INDEX(1): > > + case I3C_LVR_I2C_INDEX(2): > > + if (i3cbus->mode < I3C_BUS_MODE_MIXED_SLOW) > > + i3cbus->mode = I3C_BUS_MODE_MIXED_SLOW; > > + break; > > + > > + default: > > + return -EINVAL; > > + } > > + } > > + > > + if (!i3cbus->scl_rate.i3c) > > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > + > > + if (!i3cbus->scl_rate.i2c) { > > + if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW) > > + i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; > > + else > > + i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE; > > + } > > + > > + /* > > + * I3C/I2C frequency may have been overridden, check that user-provided > > + * values are not exceeding max possible frequency. > > + */ > > + if (i3cbus->scl_rate.i3c > I3C_BUS_MAX_I3C_SCL_RATE || > > + i3cbus->scl_rate.i2c > I3C_BUS_I2C_FM_PLUS_SCL_RATE) { > > + return -EINVAL; > > + } > > + > > + dev_set_name(&i3cbus->dev, "i3c-%d", i3cbus->id); > > + > > + return device_add(&i3cbus->dev); > > +} > During the tests of the bus with i2c devices I found the i2c_dev_desc > objects aren't allocated before this function. This cause i3cbus->mode = > I3C_BUS_MODE_PURE. I just checked and DT parsing (+ I2C descs creation) is done before i3c_bus_register() is called, so we should be good. How did you declare your I2C devices (right now, only DT declaration is supported). > > I want to do something for the slave and secondary master, do you > already have infrastructure that you can share? What do you mean? Regards, Boris
Hi Boris, On 24-08-2018 13:39, Boris Brezillon wrote: > Hi Vitor, > > On Wed, 22 Aug 2018 17:43:34 +0100 > vitor <Vitor.Soares@synopsys.com> wrote: > >> Hi Boris, >> >> >> On 19-07-2018 16:29, Boris Brezillon wrote: >>> +int i3c_bus_register(struct i3c_bus *i3cbus) >>> +{ >>> + struct i2c_dev_desc *desc; >>> + >>> + i3c_bus_for_each_i2cdev(i3cbus, desc) { >>> + switch (desc->boardinfo->lvr & I3C_LVR_I2C_INDEX_MASK) { >>> + case I3C_LVR_I2C_INDEX(0): >>> + if (i3cbus->mode < I3C_BUS_MODE_MIXED_FAST) >>> + i3cbus->mode = I3C_BUS_MODE_MIXED_FAST; >>> + break; >>> + >>> + case I3C_LVR_I2C_INDEX(1): >>> + case I3C_LVR_I2C_INDEX(2): >>> + if (i3cbus->mode < I3C_BUS_MODE_MIXED_SLOW) >>> + i3cbus->mode = I3C_BUS_MODE_MIXED_SLOW; >>> + break; >>> + >>> + default: >>> + return -EINVAL; >>> + } >>> + } >>> + >>> + if (!i3cbus->scl_rate.i3c) >>> + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; >>> + >>> + if (!i3cbus->scl_rate.i2c) { >>> + if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW) >>> + i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; >>> + else >>> + i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE; >>> + } >>> + >>> + /* >>> + * I3C/I2C frequency may have been overridden, check that user-provided >>> + * values are not exceeding max possible frequency. >>> + */ >>> + if (i3cbus->scl_rate.i3c > I3C_BUS_MAX_I3C_SCL_RATE || >>> + i3cbus->scl_rate.i2c > I3C_BUS_I2C_FM_PLUS_SCL_RATE) { >>> + return -EINVAL; >>> + } >>> + >>> + dev_set_name(&i3cbus->dev, "i3c-%d", i3cbus->id); >>> + >>> + return device_add(&i3cbus->dev); >>> +} >> During the tests of the bus with i2c devices I found the i2c_dev_desc >> objects aren't allocated before this function. This cause i3cbus->mode = >> I3C_BUS_MODE_PURE. > I just checked and DT parsing (+ I2C descs creation) is done before > i3c_bus_register() is called, so we should be good. How did you declare > your I2C devices (right now, only DT declaration is supported). During the DT parsing, you create the i2c_dev_boardinfo. the i2c_dev_desc is created in i3c_master_bus_init() which is after the i3c_mater_create_bus(). One possible way to fix this is to pass master also to i3c_bus_register and iterate over i2c_dev_board_info list. >> I want to do something for the slave and secondary master, do you >> already have infrastructure that you can share? > What do you mean? > > Regards, > > Boris I want start to add the secondary master functionality but it is also necessary to add the infrastructure to the subsystem. So, to avoid duplicated work can you share your plans for the secondary master? Best regards, Vitor Soares
Hi Vitor, On Fri, 24 Aug 2018 18:52:52 +0100 vitor <Vitor.Soares@synopsys.com> wrote: > Hi Boris, > > > On 24-08-2018 13:39, Boris Brezillon wrote: > > Hi Vitor, > > > > On Wed, 22 Aug 2018 17:43:34 +0100 > > vitor <Vitor.Soares@synopsys.com> wrote: > > > >> Hi Boris, > >> > >> > >> On 19-07-2018 16:29, Boris Brezillon wrote: > >>> +int i3c_bus_register(struct i3c_bus *i3cbus) > >>> +{ > >>> + struct i2c_dev_desc *desc; > >>> + > >>> + i3c_bus_for_each_i2cdev(i3cbus, desc) { > >>> + switch (desc->boardinfo->lvr & I3C_LVR_I2C_INDEX_MASK) { > >>> + case I3C_LVR_I2C_INDEX(0): > >>> + if (i3cbus->mode < I3C_BUS_MODE_MIXED_FAST) > >>> + i3cbus->mode = I3C_BUS_MODE_MIXED_FAST; > >>> + break; > >>> + > >>> + case I3C_LVR_I2C_INDEX(1): > >>> + case I3C_LVR_I2C_INDEX(2): > >>> + if (i3cbus->mode < I3C_BUS_MODE_MIXED_SLOW) > >>> + i3cbus->mode = I3C_BUS_MODE_MIXED_SLOW; > >>> + break; > >>> + > >>> + default: > >>> + return -EINVAL; > >>> + } > >>> + } > >>> + > >>> + if (!i3cbus->scl_rate.i3c) > >>> + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > >>> + > >>> + if (!i3cbus->scl_rate.i2c) { > >>> + if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW) > >>> + i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; > >>> + else > >>> + i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE; > >>> + } > >>> + > >>> + /* > >>> + * I3C/I2C frequency may have been overridden, check that user-provided > >>> + * values are not exceeding max possible frequency. > >>> + */ > >>> + if (i3cbus->scl_rate.i3c > I3C_BUS_MAX_I3C_SCL_RATE || > >>> + i3cbus->scl_rate.i2c > I3C_BUS_I2C_FM_PLUS_SCL_RATE) { > >>> + return -EINVAL; > >>> + } > >>> + > >>> + dev_set_name(&i3cbus->dev, "i3c-%d", i3cbus->id); > >>> + > >>> + return device_add(&i3cbus->dev); > >>> +} > >> During the tests of the bus with i2c devices I found the i2c_dev_desc > >> objects aren't allocated before this function. This cause i3cbus->mode = > >> I3C_BUS_MODE_PURE. > > I just checked and DT parsing (+ I2C descs creation) is done before > > i3c_bus_register() is called, so we should be good. How did you declare > > your I2C devices (right now, only DT declaration is supported). > During the DT parsing, you create the i2c_dev_boardinfo. the > i2c_dev_desc is created in i3c_master_bus_init() which is after the > i3c_mater_create_bus(). Oops, you're right. > One possible way to fix this is to pass master > also to i3c_bus_register and iterate over i2c_dev_board_info list. Yes, that's the proper fix. I'll do that in v7. > > >> I want to do something for the slave and secondary master, do you > >> already have infrastructure that you can share? > > What do you mean? > > > > Regards, > > > > Boris > > I want start to add the secondary master functionality but it is also > necessary to add the infrastructure to the subsystem. > So, to avoid duplicated work can you share your plans for the secondary > master? Well, before even considering supporting secondary master registration, we need to handle mastership handover. As for the DAA operation, it's likely to be host specific, so we'll have to add a new hook to the i3c_master_controller_ops struct. Once you've done that, we'll have trigger a mastership handover everytime an I3C driver tries to send a frame on the bus, and the master this frame should do through is not in control of the bus. That should be pretty easy for the nominal case, but error cases are likely to be hard to deal with. Note that I have a ->cur_master field in the i3c_bus object which stores allows us to track whose the currently active master. If master->this != master->bus->cur_master that means you need to start a mastership handover procedure. That's all I thought about for now, and we'll probably face other problems when implementing it. Let me know if you have other questions, and don't hesitate to share your code early during the development phase. Also note that the bus representation is likely to change based on Arnd's feedback, so you might have to rework your implementation a bit at some point. Regards, Boris
Hi Boris, The DT Bindings say "The node describing an I3C bus should be named i3c-master.". Do you have a field for secondary master? On 24-08-2018 19:16, Boris Brezillon wrote: > Well, before even considering supporting secondary master registration, > we need to handle mastership handover. As for the DAA operation, it's > likely to be host specific, so we'll have to add a new hook to the > i3c_master_controller_ops struct. Do you mean when master try to delegate the bus ownership through GETACCMST? or to get the bus ownership with IBI-MR? I think that could be useful to pass the ibi type on request_ibi(), there is some case where the master doesn't support IBI-MR. > Once you've done that, we'll have trigger a mastership handover > everytime an I3C driver tries to send a frame on the bus, and the > master this frame should do through is not in control of the bus. That > should be pretty easy for the nominal case, but error cases are likely > to be hard to deal with. > Note that I have a ->cur_master field in the i3c_bus object which > stores allows us to track whose the currently active master. If > master->this != master->bus->cur_master that means you need to start a > mastership handover procedure. > > That's all I thought about for now, and we'll probably face other > problems when implementing it. Let me know if you have other questions, > and don't hesitate to share your code early during the development > phase. > > Also note that the bus representation is likely to change based on > Arnd's feedback, so you might have to rework your implementation a bit > at some point. > > Regards, > > Boris Best regards, Vitor Soares
Hi Vitor, On Tue, 28 Aug 2018 12:50:12 +0100 vitor <Vitor.Soares@synopsys.com> wrote: > Hi Boris, > > The DT Bindings say "The node describing an I3C bus should be named > i3c-master.". Do you have a field for secondary master? > > On 24-08-2018 19:16, Boris Brezillon wrote: > > Well, before even considering supporting secondary master registration, > > we need to handle mastership handover. As for the DAA operation, it's > > likely to be host specific, so we'll have to add a new hook to the > > i3c_master_controller_ops struct. > Do you mean when master try to delegate the bus ownership through > GETACCMST? or to get the bus ownership with IBI-MR? I think we need to support both. > > I think that could be useful to pass the ibi type on request_ibi(), > there is some case where the master doesn't support IBI-MR. Actually, I was planning on making it completely separate from regular slave IBIs. That is, the master controller driver would demux the slave, MR and Hot Join IBIs, and if there's an MR request, queue a mastership handover work to the workqueue (pretty much what we do for Hot-Join already). Mastership handover is anyway likely to be IP specific, so I don't think there's a need to make it look like a regular IBI. Regarding whether IBI-MR support should be exposed to the I3C framework or not depends on how much will be automated on the framework side. I don't the answer yet, but that's probably something will figure out along the road. Regards, Boris
Hi Vitor,
I have already implemented Mastership request/handover but we are waiting for Boris’s patch to be accepted and merged. Anyway, my comments below.
On 8/28/18, 2:02 PM, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote:
EXTERNAL MAIL
Hi Vitor,
On Tue, 28 Aug 2018 12:50:12 +0100
vitor <Vitor.Soares@synopsys.com> wrote:
> Hi Boris,
>
> The DT Bindings say "The node describing an I3C bus should be named
> i3c-master.". Do you have a field for secondary master?
I think we don’t need separate field for secondary master. Main and secondary masters
support similar functionalities. It’s enough to have this state internally and do mastership it it's needed.
>
> On 24-08-2018 19:16, Boris Brezillon wrote:
> > Well, before even considering supporting secondary master registration,
> > we need to handle mastership handover. As for the DAA operation, it's
> > likely to be host specific, so we'll have to add a new hook to the
> > i3c_master_controller_ops struct.
> Do you mean when master try to delegate the bus ownership through
> GETACCMST? or to get the bus ownership with IBI-MR?
I think we need to support both.
I agree.
>
> I think that could be useful to pass the ibi type on request_ibi(),
> there is some case where the master doesn't support IBI-MR.
Actually, I was planning on making it completely separate from
regular slave IBIs. That is, the master controller driver would demux
the slave, MR and Hot Join IBIs, and if there's an MR request, queue a
mastership handover work to the workqueue (pretty much what we do for
Hot-Join already). Mastership handover is anyway likely to be IP
specific, so I don't think there's a need to make it look like a
regular IBI.
I think it's better to have separate function to do mastership request.
Regarding whether IBI-MR support should be exposed to the I3C framework
or not depends on how much will be automated on the framework side. I
don't the answer yet, but that's probably something will figure out
along the road.
My current implementation is: when request_mastership field
of i3c_master_controller_ops structure is set, master driver supports mastership requests.
That's how I check if this is supported or not.
Regards,
Boris
Regards,
Przemek
Hi Przemek, On Tue, 28 Aug 2018 12:55:20 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > Hi Vitor, > > I have already implemented Mastership request/handover but we are waiting for Boris’s patch to be accepted and merged. Anyway, my comments below. > > On 8/28/18, 2:02 PM, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote: > > EXTERNAL MAIL > > > Hi Vitor, > > On Tue, 28 Aug 2018 12:50:12 +0100 > vitor <Vitor.Soares@synopsys.com> wrote: > > > Hi Boris, > > > > The DT Bindings say "The node describing an I3C bus should be named > > i3c-master.". Do you have a field for secondary master? > > I think we don’t need separate field for secondary master. Main and secondary masters > support similar functionalities. It’s enough to have this state internally and do mastership it it's needed. > > > > > On 24-08-2018 19:16, Boris Brezillon wrote: > > > Well, before even considering supporting secondary master registration, > > > we need to handle mastership handover. As for the DAA operation, it's > > > likely to be host specific, so we'll have to add a new hook to the > > > i3c_master_controller_ops struct. > > Do you mean when master try to delegate the bus ownership through > > GETACCMST? or to get the bus ownership with IBI-MR? > > I think we need to support both. > > I agree. > > > > > I think that could be useful to pass the ibi type on request_ibi(), > > there is some case where the master doesn't support IBI-MR. > > Actually, I was planning on making it completely separate from > regular slave IBIs. That is, the master controller driver would demux > the slave, MR and Hot Join IBIs, and if there's an MR request, queue a > mastership handover work to the workqueue (pretty much what we do for > Hot-Join already). Mastership handover is anyway likely to be IP > specific, so I don't think there's a need to make it look like a > regular IBI. > > I think it's better to have separate function to do mastership request. > > Regarding whether IBI-MR support should be exposed to the I3C framework > or not depends on how much will be automated on the framework side. I > don't the answer yet, but that's probably something will figure out > along the road. > > My current implementation is: when request_mastership field > of i3c_master_controller_ops structure is set, master driver supports mastership requests. > That's how I check if this is supported or not. Can you maybe host your code on a public repo (I can push it for you if needed) so that you and Vitor can start discussing implementation details. Thanks, Boris
On Tue, 28 Aug 2018 12:55:20 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > Hi Vitor, > > I have already implemented Mastership request/handover but we are > waiting for Boris’s patch to be accepted and merged. My bad. I didn't have time to work on the bus/master rework suggested by Arnd before going on vacation. I hope I'll be able to work on that soon.
Hi Boris,
On 8/28/18, 3:01 PM, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote:
EXTERNAL MAIL
Hi Przemek,
On Tue, 28 Aug 2018 12:55:20 +0000
Przemyslaw Gaj <pgaj@cadence.com> wrote:
> Hi Vitor,
>
> I have already implemented Mastership request/handover but we are waiting for Boris’s patch to be accepted and merged. Anyway, my comments below.
>
> On 8/28/18, 2:02 PM, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote:
>
> EXTERNAL MAIL
>
>
> Hi Vitor,
>
> On Tue, 28 Aug 2018 12:50:12 +0100
> vitor <Vitor.Soares@synopsys.com> wrote:
>
> > Hi Boris,
> >
> > The DT Bindings say "The node describing an I3C bus should be named
> > i3c-master.". Do you have a field for secondary master?
>
> I think we don’t need separate field for secondary master. Main and secondary masters
> support similar functionalities. It’s enough to have this state internally and do mastership it it's needed.
>
> >
> > On 24-08-2018 19:16, Boris Brezillon wrote:
> > > Well, before even considering supporting secondary master registration,
> > > we need to handle mastership handover. As for the DAA operation, it's
> > > likely to be host specific, so we'll have to add a new hook to the
> > > i3c_master_controller_ops struct.
> > Do you mean when master try to delegate the bus ownership through
> > GETACCMST? or to get the bus ownership with IBI-MR?
>
> I think we need to support both.
>
> I agree.
>
> >
> > I think that could be useful to pass the ibi type on request_ibi(),
> > there is some case where the master doesn't support IBI-MR.
>
> Actually, I was planning on making it completely separate from
> regular slave IBIs. That is, the master controller driver would demux
> the slave, MR and Hot Join IBIs, and if there's an MR request, queue a
> mastership handover work to the workqueue (pretty much what we do for
> Hot-Join already). Mastership handover is anyway likely to be IP
> specific, so I don't think there's a need to make it look like a
> regular IBI.
>
> I think it's better to have separate function to do mastership request.
>
> Regarding whether IBI-MR support should be exposed to the I3C framework
> or not depends on how much will be automated on the framework side. I
> don't the answer yet, but that's probably something will figure out
> along the road.
>
> My current implementation is: when request_mastership field
> of i3c_master_controller_ops structure is set, master driver supports mastership requests.
> That's how I check if this is supported or not.
Can you maybe host your code on a public repo (I can push it for you if
needed) so that you and Vitor can start discussing implementation
details.
Sure! Please give me some time. I'll try to do it this week or early next week.
Thanks,
Boris
Thanks,
Przemek
Hi Przemyslaw On 28-08-2018 13:55, Przemyslaw Gaj wrote: > Hi Vitor, > > I have already implemented Mastership request/handover but we are waiting for Boris’s patch to be accepted and merged. Anyway, my comments below. > > On 8/28/18, 2:02 PM, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote: > > EXTERNAL MAIL > > > Hi Vitor, > > On Tue, 28 Aug 2018 12:50:12 +0100 > vitor <Vitor.Soares@synopsys.com> wrote: > > > Hi Boris, > > > > The DT Bindings say "The node describing an I3C bus should be named > > i3c-master.". Do you have a field for secondary master? > > I think we don’t need separate field for secondary master. Main and secondary masters > support similar functionalities. It’s enough to have this state internally and do mastership it it's needed. Yes, you are right. > > > > > On 24-08-2018 19:16, Boris Brezillon wrote: > > > Well, before even considering supporting secondary master registration, > > > we need to handle mastership handover. As for the DAA operation, it's > > > likely to be host specific, so we'll have to add a new hook to the > > > i3c_master_controller_ops struct. > > Do you mean when master try to delegate the bus ownership through > > GETACCMST? or to get the bus ownership with IBI-MR? > > I think we need to support both. > > I agree. That's ok to me. > > > > > I think that could be useful to pass the ibi type on request_ibi(), > > there is some case where the master doesn't support IBI-MR. > > Actually, I was planning on making it completely separate from > regular slave IBIs. That is, the master controller driver would demux > the slave, MR and Hot Join IBIs, and if there's an MR request, queue a > mastership handover work to the workqueue (pretty much what we do for > Hot-Join already). Mastership handover is anyway likely to be IP > specific, so I don't think there's a need to make it look like a > regular IBI. > > I think it's better to have separate function to do mastership request. > > Regarding whether IBI-MR support should be exposed to the I3C framework > or not depends on how much will be automated on the framework side. I > don't the answer yet, but that's probably something will figure out > along the road. > > My current implementation is: when request_mastership field > of i3c_master_controller_ops structure is set, master driver supports mastership requests. > That's how I check if this is supported or not. > > Regards, > > Boris > > Regards, > Przemek > when you say request_mastership, do you mean the current master do the mastership hand-off or the secondary master request to be current master? So, per my understanding since the Main master support the hand-off of the bus you accept all incoming MR, right? Or do you check all devices BCR? Best regards, Vitor Soares
Hi Vitor,
On 8/30/18, 3:57 PM, "vitor" <Vitor.Soares@synopsys.com> wrote:
EXTERNAL MAIL
Hi Przemyslaw
Just Przemek :)
On 28-08-2018 13:55, Przemyslaw Gaj wrote:
> Hi Vitor,
>
> I have already implemented Mastership request/handover but we are waiting for Boris’s patch to be accepted and merged. Anyway, my comments below.
>
> On 8/28/18, 2:02 PM, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote:
>
> EXTERNAL MAIL
>
>
> Hi Vitor,
>
> On Tue, 28 Aug 2018 12:50:12 +0100
> vitor <Vitor.Soares@synopsys.com> wrote:
>
> > Hi Boris,
> >
> > The DT Bindings say "The node describing an I3C bus should be named
> > i3c-master.". Do you have a field for secondary master?
>
> I think we don’t need separate field for secondary master. Main and secondary masters
> support similar functionalities. It’s enough to have this state internally and do mastership it it's needed.
Yes, you are right.
>
> >
> > On 24-08-2018 19:16, Boris Brezillon wrote:
> > > Well, before even considering supporting secondary master registration,
> > > we need to handle mastership handover. As for the DAA operation, it's
> > > likely to be host specific, so we'll have to add a new hook to the
> > > i3c_master_controller_ops struct.
> > Do you mean when master try to delegate the bus ownership through
> > GETACCMST? or to get the bus ownership with IBI-MR?
>
> I think we need to support both.
>
> I agree.
That's ok to me.
>
> >
> > I think that could be useful to pass the ibi type on request_ibi(),
> > there is some case where the master doesn't support IBI-MR.
>
> Actually, I was planning on making it completely separate from
> regular slave IBIs. That is, the master controller driver would demux
> the slave, MR and Hot Join IBIs, and if there's an MR request, queue a
> mastership handover work to the workqueue (pretty much what we do for
> Hot-Join already). Mastership handover is anyway likely to be IP
> specific, so I don't think there's a need to make it look like a
> regular IBI.
>
> I think it's better to have separate function to do mastership request.
>
> Regarding whether IBI-MR support should be exposed to the I3C framework
> or not depends on how much will be automated on the framework side. I
> don't the answer yet, but that's probably something will figure out
> along the road.
>
> My current implementation is: when request_mastership field
> of i3c_master_controller_ops structure is set, master driver supports mastership requests.
> That's how I check if this is supported or not.
>
> Regards,
>
> Boris
>
> Regards,
> Przemek
>
when you say request_mastership, do you mean the current master do the
mastership hand-off or the secondary master request to be current master?
I mean secondary master requests to be current master. Current master do the mastership
hand-off using GETACCMST command.
So, per my understanding since the Main master support the hand-off of
the bus you accept all incoming MR, right? Or do you check all devices BCR?
I'm not sure what do you mean here. Mastership request(MR) is from secondary master
to current master. Current master can NACK this request if for example it comes from
wrong device. If it's ok, current master sends GETACCMST command and secondary master
may ACK or NACK this command. It it's acked, secondary master becomes current master.
Best regards,
Vitor Soares
Please let me know if something is unclear.
Regards,
Przemek
Hi Przemek, On 30-08-2018 20:00, Przemyslaw Gaj wrote: > So, per my understanding since the Main master support the hand-off of > the bus you accept all incoming MR, right? Or do you check all devices BCR? > > I'm not sure what do you mean here. Mastership request(MR) is from secondary master > to current master. Current master can NACK this request if for example it comes from > wrong device. If it's ok, current master sends GETACCMST command and secondary master > may ACK or NACK this command. It it's acked, secondary master becomes current master. > > Best regards, > Vitor Soares > > Please let me know if something is unclear. > > Regards, > Przemek > Sorry, it s not clear yet. For instances there is a bus with several secondary master. If each of them request the bus mastership (one at a time), will you accept all by default? Because you can only accept only some of them. Regards, Vitor Soares
Hi Vitor,
On 9/3/18, 11:33 AM, "vitor" <Vitor.Soares@synopsys.com> wrote:
EXTERNAL MAIL
Hi Przemek,
On 30-08-2018 20:00, Przemyslaw Gaj wrote:
> So, per my understanding since the Main master support the hand-off of
> the bus you accept all incoming MR, right? Or do you check all devices BCR?
>
> I'm not sure what do you mean here. Mastership request(MR) is from secondary master
> to current master. Current master can NACK this request if for example it comes from
> wrong device. If it's ok, current master sends GETACCMST command and secondary master
> may ACK or NACK this command. It it's acked, secondary master becomes current master.
>
> Best regards,
> Vitor Soares
>
> Please let me know if something is unclear.
>
> Regards,
> Przemek
>
Sorry, it s not clear yet.
For instances there is a bus with several secondary master. If each of
them request the bus mastership (one at a time), will you accept all by
default? Because you can only accept only some of them.
First of all, only devices which have Mastership request event enabled (ENMR slave event)
can request mastership. Second thing is that this device needs to have IBI enabled in
current master. I accept all such devices. There is no additional logic for now, we can add it
in the future.
I was very busy recently. I'm almost ready to publish my changes related to mastership request.
I'm sorry for the delay.
Regards,
Vitor Soares
Regards,
Przemek
Hi Boris, Vitor,
This repository does not contain full kernel sources, but it should be enough to discuss mastership request feature.
https://github.com/przemekgaj/i3c-linux/commit/d54fe68a9d3e573c0c454a2c6f1afafc20142ec5
Please keep in mind that this is initial version, but mastership works correctly. I added one property to DT to reflect relationship between masters.
It is possible to request mastership on demand (using sysfs. Useful in case when Linux machine is equipped only with secondary master controller) or automatically change operation mode when device driver wants to read/write something from/to device.
I'm sure I will have to rework something because this was implemented on sources from PATCH v4. I saw that Boris released v7 yesterday :)
Thanks,
Przemek
On 9/3/18, 11:33 AM, "vitor" <Vitor.Soares@synopsys.com> wrote:
EXTERNAL MAIL
Hi Przemek,
On 30-08-2018 20:00, Przemyslaw Gaj wrote:
> So, per my understanding since the Main master support the hand-off of
> the bus you accept all incoming MR, right? Or do you check all devices BCR?
>
> I'm not sure what do you mean here. Mastership request(MR) is from secondary master
> to current master. Current master can NACK this request if for example it comes from
> wrong device. If it's ok, current master sends GETACCMST command and secondary master
> may ACK or NACK this command. It it's acked, secondary master becomes current master.
>
> Best regards,
> Vitor Soares
>
> Please let me know if something is unclear.
>
> Regards,
> Przemek
>
Sorry, it s not clear yet.
For instances there is a bus with several secondary master. If each of
them request the bus mastership (one at a time), will you accept all by
default? Because you can only accept only some of them.
Regards,
Vitor Soares
On Thu, Sep 6, 2018 at 2:43 PM Przemyslaw Gaj <pgaj@cadence.com> wrote: > > Hi Boris, Vitor, > > This repository does not contain full kernel sources, but it should be enough to discuss mastership request feature. > https://github.com/przemekgaj/i3c-linux/commit/d54fe68a9d3e573c0c454a2c6f1afafc20142ec5 > > Please keep in mind that this is initial version, but mastership works correctly. I added one property to DT to reflect relationship between masters. > It is possible to request mastership on demand (using sysfs. Useful in case when Linux machine is equipped only with secondary master controller) or automatically change operation mode when device driver wants to read/write something from/to device. > > I'm sure I will have to rework something because this was implemented on sources from PATCH v4. I saw that Boris released v7 yesterday :) Can you explain the reason for having a user space interface and DT property? I thought we had concluded earlier that we wouldn't need that, but it's possible that I missed something in the discussion since then. Arnd
On Thu, 6 Sep 2018 14:59:46 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > On Thu, Sep 6, 2018 at 2:43 PM Przemyslaw Gaj <pgaj@cadence.com> wrote: > > > > Hi Boris, Vitor, > > > > This repository does not contain full kernel sources, but it should be enough to discuss mastership request feature. > > https://github.com/przemekgaj/i3c-linux/commit/d54fe68a9d3e573c0c454a2c6f1afafc20142ec5 > > > > Please keep in mind that this is initial version, but mastership works correctly. I added one property to DT to reflect relationship between masters. > > It is possible to request mastership on demand (using sysfs. Useful in case when Linux machine is equipped only with secondary master controller) or automatically change operation mode when device driver wants to read/write something from/to device. > > > > I'm sure I will have to rework something because this was implemented on sources from PATCH v4. I saw that Boris released v7 yesterday :) > > > Can you explain the reason for having a user space interface and DT property? > I thought we had concluded earlier that we wouldn't need that, but it's possible > that I missed something in the discussion since then. I don't think the sysfs knob is needed, this being said, after thinking a bit more about mastership handover and the secondary master case, I think we have something important to solve. When a master is not in control of the bus, it gets informed of devices present on the bus by monitoring DAA or DEFSLVS broadcast events. That means the secondary master should populate the bus with I3C/I2C devices on such events, but that's not enough, because DEFSLVS/DAA do not provide all device info. Some of them (like read/write/ibi limitations) require extra CCC commands, and, to send those CCC commands, the secondary master must claim the bus. We could add a case where we declare devices as partially discovered until the master acquires ownership of the bus, but that means part of the data returned by i3c_device_get_info() will be inaccurate, which might have an impact on some i3c driver ->probe() functions. We could also say that partially discovered devices should not be registered to the device model, but we then hit the problem of "who can force the secondary master to claim the bus if there's no users?".
On Thu, 6 Sep 2018 15:14:37 +0200 Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Thu, 6 Sep 2018 14:59:46 +0200 > Arnd Bergmann <arnd@arndb.de> wrote: > > > On Thu, Sep 6, 2018 at 2:43 PM Przemyslaw Gaj <pgaj@cadence.com> wrote: > > > > > > Hi Boris, Vitor, > > > > > > This repository does not contain full kernel sources, but it should be enough to discuss mastership request feature. > > > https://github.com/przemekgaj/i3c-linux/commit/d54fe68a9d3e573c0c454a2c6f1afafc20142ec5 > > > > > > Please keep in mind that this is initial version, but mastership works correctly. I added one property to DT to reflect relationship between masters. > > > It is possible to request mastership on demand (using sysfs. Useful in case when Linux machine is equipped only with secondary master controller) or automatically change operation mode when device driver wants to read/write something from/to device. > > > > > > I'm sure I will have to rework something because this was implemented on sources from PATCH v4. I saw that Boris released v7 yesterday :) > > > > > > Can you explain the reason for having a user space interface and DT property? > > I thought we had concluded earlier that we wouldn't need that, but it's possible > > that I missed something in the discussion since then. > > I don't think the sysfs knob is needed, this being said, after thinking > a bit more about mastership handover and the secondary master case, I > think we have something important to solve. > > When a master is not in control of the bus, it gets informed of devices > present on the bus by monitoring DAA or DEFSLVS broadcast events. That > means the secondary master should populate the bus with I3C/I2C devices > on such events, but that's not enough, because DEFSLVS/DAA do not > provide all device info. Some of them (like read/write/ibi limitations) > require extra CCC commands, and, to send those CCC commands, the > secondary master must claim the bus. We could add a case where we > declare devices as partially discovered until the master acquires > ownership of the bus, but that means part of the data returned by > i3c_device_get_info() will be inaccurate, which might have an impact on > some i3c driver ->probe() functions. Hm, one possible solution would be to register partially discovered devices to the device model and let i3c_device_get_info() claim the bus and request missing data when needed. This way, if the driver needs to call i3c_device_get_info() in its probe path, it should work just fine.
On Thu, Sep 6, 2018 at 3:21 PM Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > On Thu, 6 Sep 2018 15:14:37 +0200 > Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > When a master is not in control of the bus, it gets informed of devices > > present on the bus by monitoring DAA or DEFSLVS broadcast events. That > > means the secondary master should populate the bus with I3C/I2C devices > > on such events, but that's not enough, because DEFSLVS/DAA do not > > provide all device info. Some of them (like read/write/ibi limitations) > > require extra CCC commands, and, to send those CCC commands, the > > secondary master must claim the bus. We could add a case where we > > declare devices as partially discovered until the master acquires > > ownership of the bus, but that means part of the data returned by > > i3c_device_get_info() will be inaccurate, which might have an impact on > > some i3c driver ->probe() functions. > > Hm, one possible solution would be to register partially discovered > devices to the device model and let i3c_device_get_info() claim the bus > and request missing data when needed. This way, if the driver needs to > call i3c_device_get_info() in its probe path, it should work just fine. I guess you could also call i3c_device_get_info() in the common i3c_probe() function before calling into the driver. However, either way, we may still have a problem here: if the current master decides not to hand over master access to us at all, the probe() function will be blocked indefinitely, and that may stop us from probing other devices later on, or hang the module loader (depending on what context that probe() is called from). Using a timeout here could avoid the hang, but leads to other potential issues, e.g. how to decide whether to retry the probe later. Arnd
On 9/6/18, 3:14 PM, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote:
EXTERNAL MAIL
On Thu, 6 Sep 2018 14:59:46 +0200
Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Sep 6, 2018 at 2:43 PM Przemyslaw Gaj <pgaj@cadence.com> wrote:
> >
> > Hi Boris, Vitor,
> >
> > This repository does not contain full kernel sources, but it should be enough to discuss mastership request feature.
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_przemekgaj_i3c-2Dlinux_commit_d54fe68a9d3e573c0c454a2c6f1afafc20142ec5&d=DwICAg&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=CMnAfM_OfpqcWZRfiqcRWw&m=OPSa25ENnrF0Qv70DG49ZngfdygJZubjo3TOgBA3pJ4&s=_C6i1KPplQGcWvQYPkrGx7V3TjKDJvqt3KG-vcdU2K4&e=
> >
> > Please keep in mind that this is initial version, but mastership works correctly. I added one property to DT to reflect relationship between masters.
> > It is possible to request mastership on demand (using sysfs. Useful in case when Linux machine is equipped only with secondary master controller) or automatically change operation mode when device driver wants to read/write something from/to device.
> >
> > I'm sure I will have to rework something because this was implemented on sources from PATCH v4. I saw that Boris released v7 yesterday :)
>
>
> Can you explain the reason for having a user space interface and DT property?
> I thought we had concluded earlier that we wouldn't need that, but it's possible
> that I missed something in the discussion since then.
I don't think the sysfs knob is needed, this being said, after thinking
a bit more about mastership handover and the secondary master case, I
think we have something important to solve.
When a master is not in control of the bus, it gets informed of devices
present on the bus by monitoring DAA or DEFSLVS broadcast events. That
means the secondary master should populate the bus with I3C/I2C devices
on such events, but that's not enough, because DEFSLVS/DAA do not
provide all device info. Some of them (like read/write/ibi limitations)
require extra CCC commands, and, to send those CCC commands, the
secondary master must claim the bus. We could add a case where we
declare devices as partially discovered until the master acquires
ownership of the bus, but that means part of the data returned by
i3c_device_get_info() will be inaccurate, which might have an impact on
some i3c driver ->probe() functions.
How do you want to handle cases when secondary master joins the bus after
DEFSLVS? Of course we can send DEFSLVS after secondary master joins the bus.
We could also say that partially discovered devices should not be
registered to the device model, but we then hit the problem of "who can
force the secondary master to claim the bus if there's no users?".
Now I feel like I missed something. Do you want to populate second instance
of the same physical bus? If yes, then we don't need to have reference
between masters in DT.
Hi, On 06-09-2018 14:20, Boris Brezillon wrote: > On Thu, 6 Sep 2018 15:14:37 +0200 > Boris Brezillon <boris.brezillon@bootlin.com> wrote: > >> On Thu, 6 Sep 2018 14:59:46 +0200 >> Arnd Bergmann <arnd@arndb.de> wrote: >> >>> On Thu, Sep 6, 2018 at 2:43 PM Przemyslaw Gaj <pgaj@cadence.com> wrote: >>>> Hi Boris, Vitor, >>>> >>>> This repository does not contain full kernel sources, but it should be enough to discuss mastership request feature. >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_przemekgaj_i3c-2Dlinux_commit_d54fe68a9d3e573c0c454a2c6f1afafc20142ec5&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=Q9DWw3KGmshGw0f5QTiffbpbESyUlPx6KmASuDBtX9Y&s=HHE_y1kyMszJvP_tSP9JkDlPYxDywBeHwkMGgCR11uI&e= >>>> >>>> Please keep in mind that this is initial version, but mastership works correctly. I added one property to DT to reflect relationship between masters. >>>> It is possible to request mastership on demand (using sysfs. Useful in case when Linux machine is equipped only with secondary master controller) or automatically change operation mode when device driver wants to read/write something from/to device. >>>> >>>> I'm sure I will have to rework something because this was implemented on sources from PATCH v4. I saw that Boris released v7 yesterday :) >>> >>> Can you explain the reason for having a user space interface and DT property? >>> I thought we had concluded earlier that we wouldn't need that, but it's possible >>> that I missed something in the discussion since then. >> I don't think the sysfs knob is needed, this being said, after thinking >> a bit more about mastership handover and the secondary master case, I >> think we have something important to solve. >> >> When a master is not in control of the bus, it gets informed of devices >> present on the bus by monitoring DAA or DEFSLVS broadcast events. That >> means the secondary master should populate the bus with I3C/I2C devices >> on such events, but that's not enough, because DEFSLVS/DAA do not >> provide all device info.Some of them (like read/write/ibi limitations) >> require extra CCC commands, and, to send those CCC commands, the >> secondary master must claim the bus. We could add a case where we >> declare devices as partially discovered until the master acquires >> ownership of the bus, but that means part of the data returned by >> i3c_device_get_info() will be inaccurate, which might have an impact on >> some i3c driver ->probe() functions. > Hm, one possible solution would be to register partially discovered > devices to the device model and let i3c_device_get_info() claim the bus > and request missing data when needed. This way, if the driver needs to > call i3c_device_get_info() in its probe path, it should work just fine. Why don't use the i3c_master_add_i3c_dev_locked that job? It create, attach and retrieve the device info. This can be triggered after the secondary master receive ENEC MR until them is keep in the driver memory. Best regards, Vitor Soares
On Thu, 6 Sep 2018 13:47:29 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > On 9/6/18, 3:14 PM, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote: > > EXTERNAL MAIL > > > On Thu, 6 Sep 2018 14:59:46 +0200 > Arnd Bergmann <arnd@arndb.de> wrote: > > > On Thu, Sep 6, 2018 at 2:43 PM Przemyslaw Gaj <pgaj@cadence.com> wrote: > > > > > > Hi Boris, Vitor, > > > > > > This repository does not contain full kernel sources, but it should be enough to discuss mastership request feature. > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_przemekgaj_i3c-2Dlinux_commit_d54fe68a9d3e573c0c454a2c6f1afafc20142ec5&d=DwICAg&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=CMnAfM_OfpqcWZRfiqcRWw&m=OPSa25ENnrF0Qv70DG49ZngfdygJZubjo3TOgBA3pJ4&s=_C6i1KPplQGcWvQYPkrGx7V3TjKDJvqt3KG-vcdU2K4&e= > > > > > > Please keep in mind that this is initial version, but mastership works correctly. I added one property to DT to reflect relationship between masters. > > > It is possible to request mastership on demand (using sysfs. Useful in case when Linux machine is equipped only with secondary master controller) or automatically change operation mode when device driver wants to read/write something from/to device. > > > > > > I'm sure I will have to rework something because this was implemented on sources from PATCH v4. I saw that Boris released v7 yesterday :) > > > > > > Can you explain the reason for having a user space interface and DT property? > > I thought we had concluded earlier that we wouldn't need that, but it's possible > > that I missed something in the discussion since then. > > I don't think the sysfs knob is needed, this being said, after thinking > a bit more about mastership handover and the secondary master case, I > think we have something important to solve. > > When a master is not in control of the bus, it gets informed of devices > present on the bus by monitoring DAA or DEFSLVS broadcast events. That > means the secondary master should populate the bus with I3C/I2C devices > on such events, but that's not enough, because DEFSLVS/DAA do not > provide all device info. Some of them (like read/write/ibi limitations) > require extra CCC commands, and, to send those CCC commands, the > secondary master must claim the bus. We could add a case where we > declare devices as partially discovered until the master acquires > ownership of the bus, but that means part of the data returned by > i3c_device_get_info() will be inaccurate, which might have an impact on > some i3c driver ->probe() functions. > > How do you want to handle cases when secondary master joins the bus after > DEFSLVS? Of course we can send DEFSLVS after secondary master joins the bus. That's already the case ;-). Every time the current master discovers another master, it's sends a DEFSLVS at the end of the DAA procedure. > > We could also say that partially discovered devices should not be > registered to the device model, but we then hit the problem of "who can > force the secondary master to claim the bus if there's no users?". > > Now I feel like I missed something. Do you want to populate second instance > of the same physical bus? If yes, then we don't need to have reference > between masters in DT. This is the discussion we've had about 1 month ago with Arnd, Wolfram and Peter, and the conclusion was that 2 different masters connected to the same physical bus should expose 2 different buses (which means having the same physical devices exposed 2 times in Linux).
On Thu, 6 Sep 2018 14:50:03 +0100 vitor <Vitor.Soares@synopsys.com> wrote: > Hi, > > > On 06-09-2018 14:20, Boris Brezillon wrote: > > On Thu, 6 Sep 2018 15:14:37 +0200 > > Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > > >> On Thu, 6 Sep 2018 14:59:46 +0200 > >> Arnd Bergmann <arnd@arndb.de> wrote: > >> > >>> On Thu, Sep 6, 2018 at 2:43 PM Przemyslaw Gaj <pgaj@cadence.com> wrote: > >>>> Hi Boris, Vitor, > >>>> > >>>> This repository does not contain full kernel sources, but it should be enough to discuss mastership request feature. > >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_przemekgaj_i3c-2Dlinux_commit_d54fe68a9d3e573c0c454a2c6f1afafc20142ec5&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=Q9DWw3KGmshGw0f5QTiffbpbESyUlPx6KmASuDBtX9Y&s=HHE_y1kyMszJvP_tSP9JkDlPYxDywBeHwkMGgCR11uI&e= > >>>> > >>>> Please keep in mind that this is initial version, but mastership works correctly. I added one property to DT to reflect relationship between masters. > >>>> It is possible to request mastership on demand (using sysfs. Useful in case when Linux machine is equipped only with secondary master controller) or automatically change operation mode when device driver wants to read/write something from/to device. > >>>> > >>>> I'm sure I will have to rework something because this was implemented on sources from PATCH v4. I saw that Boris released v7 yesterday :) > >>> > >>> Can you explain the reason for having a user space interface and DT property? > >>> I thought we had concluded earlier that we wouldn't need that, but it's possible > >>> that I missed something in the discussion since then. > >> I don't think the sysfs knob is needed, this being said, after thinking > >> a bit more about mastership handover and the secondary master case, I > >> think we have something important to solve. > >> > >> When a master is not in control of the bus, it gets informed of devices > >> present on the bus by monitoring DAA or DEFSLVS broadcast events. That > >> means the secondary master should populate the bus with I3C/I2C devices > >> on such events, but that's not enough, because DEFSLVS/DAA do not > >> provide all device info.Some of them (like read/write/ibi limitations) > >> require extra CCC commands, and, to send those CCC commands, the > >> secondary master must claim the bus. We could add a case where we > >> declare devices as partially discovered until the master acquires > >> ownership of the bus, but that means part of the data returned by > >> i3c_device_get_info() will be inaccurate, which might have an impact on > >> some i3c driver ->probe() functions. > > Hm, one possible solution would be to register partially discovered > > devices to the device model and let i3c_device_get_info() claim the bus > > and request missing data when needed. This way, if the driver needs to > > call i3c_device_get_info() in its probe path, it should work just fine. > > Why don't use the i3c_master_add_i3c_dev_locked that job? It create, > attach and retrieve the device info. When will you call i3c_master_add_i3c_dev_locked()? After receiving a DEFSLVS interrupt/event? When that happens you're not in control of the bus, which means you'll have to force bus ownership handover. Is this really what we want? These are not rhetorical questions, I'm really asking for your opinion here. > This can be triggered after the secondary master receive ENEC MR until > them is keep in the driver memory. But that means no-one will actually trigger a mastership request, because devices won't be registered until all info are available. If we take this path, we should have a way to explicitly trigger a mastership request (sysfs knob or any other means).
On 9/6/18, 4:10 PM, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote:
EXTERNAL MAIL
On Thu, 6 Sep 2018 13:47:29 +0000
Przemyslaw Gaj <pgaj@cadence.com> wrote:
> On 9/6/18, 3:14 PM, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote:
>
> EXTERNAL MAIL
>
>
> On Thu, 6 Sep 2018 14:59:46 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:
>
> > On Thu, Sep 6, 2018 at 2:43 PM Przemyslaw Gaj <pgaj@cadence.com> wrote:
> > >
> > > Hi Boris, Vitor,
> > >
> > > This repository does not contain full kernel sources, but it should be enough to discuss mastership request feature.
> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_przemekgaj_i3c-2Dlinux_commit_d54fe68a9d3e573c0c454a2c6f1afafc20142ec5&d=DwICAg&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=CMnAfM_OfpqcWZRfiqcRWw&m=OPSa25ENnrF0Qv70DG49ZngfdygJZubjo3TOgBA3pJ4&s=_C6i1KPplQGcWvQYPkrGx7V3TjKDJvqt3KG-vcdU2K4&e=
> > >
> > > Please keep in mind that this is initial version, but mastership works correctly. I added one property to DT to reflect relationship between masters.
> > > It is possible to request mastership on demand (using sysfs. Useful in case when Linux machine is equipped only with secondary master controller) or automatically change operation mode when device driver wants to read/write something from/to device.
> > >
> > > I'm sure I will have to rework something because this was implemented on sources from PATCH v4. I saw that Boris released v7 yesterday :)
> >
> >
> > Can you explain the reason for having a user space interface and DT property?
> > I thought we had concluded earlier that we wouldn't need that, but it's possible
> > that I missed something in the discussion since then.
>
> I don't think the sysfs knob is needed, this being said, after thinking
> a bit more about mastership handover and the secondary master case, I
> think we have something important to solve.
>
> When a master is not in control of the bus, it gets informed of devices
> present on the bus by monitoring DAA or DEFSLVS broadcast events. That
> means the secondary master should populate the bus with I3C/I2C devices
> on such events, but that's not enough, because DEFSLVS/DAA do not
> provide all device info. Some of them (like read/write/ibi limitations)
> require extra CCC commands, and, to send those CCC commands, the
> secondary master must claim the bus. We could add a case where we
> declare devices as partially discovered until the master acquires
> ownership of the bus, but that means part of the data returned by
> i3c_device_get_info() will be inaccurate, which might have an impact on
> some i3c driver ->probe() functions.
>
> How do you want to handle cases when secondary master joins the bus after
> DEFSLVS? Of course we can send DEFSLVS after secondary master joins the bus.
That's already the case ;-). Every time the current master discovers
another master, it's sends a DEFSLVS at the end of the DAA procedure.
>
> We could also say that partially discovered devices should not be
> registered to the device model, but we then hit the problem of "who can
> force the secondary master to claim the bus if there's no users?".
>
> Now I feel like I missed something. Do you want to populate second instance
> of the same physical bus? If yes, then we don't need to have reference
> between masters in DT.
This is the discussion we've had about 1 month ago with Arnd, Wolfram
and Peter, and the conclusion was that 2 different masters connected to
the same physical bus should expose 2 different buses (which means
having the same physical devices exposed 2 times in Linux).
Ok, I implemented it after our discussion, I think it was in May. I even had
version where every master had own bus.
Hi Boris, On 06-09-2018 15:14, Boris Brezillon wrote: > On Thu, 6 Sep 2018 14:50:03 +0100 > vitor <Vitor.Soares@synopsys.com> wrote: > >> Hi, >> >> >> On 06-09-2018 14:20, Boris Brezillon wrote: >>> On Thu, 6 Sep 2018 15:14:37 +0200 >>> Boris Brezillon <boris.brezillon@bootlin.com> wrote: >>> >>>> On Thu, 6 Sep 2018 14:59:46 +0200 >>>> Arnd Bergmann <arnd@arndb.de> wrote: >>>> >>>>> On Thu, Sep 6, 2018 at 2:43 PM Przemyslaw Gaj <pgaj@cadence.com> wrote: >>>>>> Hi Boris, Vitor, >>>>>> >>>>>> This repository does not contain full kernel sources, but it should be enough to discuss mastership request feature. >>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_przemekgaj_i3c-2Dlinux_commit_d54fe68a9d3e573c0c454a2c6f1afafc20142ec5&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=Q9DWw3KGmshGw0f5QTiffbpbESyUlPx6KmASuDBtX9Y&s=HHE_y1kyMszJvP_tSP9JkDlPYxDywBeHwkMGgCR11uI&e= >>>>>> >>>>>> Please keep in mind that this is initial version, but mastership works correctly. I added one property to DT to reflect relationship between masters. >>>>>> It is possible to request mastership on demand (using sysfs. Useful in case when Linux machine is equipped only with secondary master controller) or automatically change operation mode when device driver wants to read/write something from/to device. >>>>>> >>>>>> I'm sure I will have to rework something because this was implemented on sources from PATCH v4. I saw that Boris released v7 yesterday :) >>>>> Can you explain the reason for having a user space interface and DT property? >>>>> I thought we had concluded earlier that we wouldn't need that, but it's possible >>>>> that I missed something in the discussion since then. >>>> I don't think the sysfs knob is needed, this being said, after thinking >>>> a bit more about mastership handover and the secondary master case, I >>>> think we have something important to solve. >>>> >>>> When a master is not in control of the bus, it gets informed of devices >>>> present on the bus by monitoring DAA or DEFSLVS broadcast events. That >>>> means the secondary master should populate the bus with I3C/I2C devices >>>> on such events, but that's not enough, because DEFSLVS/DAA do not >>>> provide all device info.Some of them (like read/write/ibi limitations) >>>> require extra CCC commands, and, to send those CCC commands, the >>>> secondary master must claim the bus. We could add a case where we >>>> declare devices as partially discovered until the master acquires >>>> ownership of the bus, but that means part of the data returned by >>>> i3c_device_get_info() will be inaccurate, which might have an impact on >>>> some i3c driver ->probe() functions. >>> Hm, one possible solution would be to register partially discovered >>> devices to the device model and let i3c_device_get_info() claim the bus >>> and request missing data when needed. This way, if the driver needs to >>> call i3c_device_get_info() in its probe path, it should work just fine. >> Why don't use the i3c_master_add_i3c_dev_locked that job? It create, >> attach and retrieve the device info. > When will you call i3c_master_add_i3c_dev_locked()? After receiving a > DEFSLVS interrupt/event? When that happens you're not in control of the > bus, which means you'll have to force bus ownership handover. Is this > really what we want? > > These are not rhetorical questions, I'm really asking for your opinion > here. What I understand from last discussion was that every time that device need to do something (private messages) on the bus and don't have the bus control it should force ownership handover. If my understanding is correct this can be also applied to CCC commands. > >> This can be triggered after the secondary master receive ENEC MR until >> them is keep in the driver memory. > But that means no-one will actually trigger a mastership request, > because devices won't be registered until all info are available. If we > take this path, we should have a way to explicitly trigger a mastership > request (sysfs knob or any other means). By the current flow that we have now, we enable the IBI events at the end of .do_daa. At this time the main master bus is initialized with all devices. From the point of view of secondary master, it first participate on DAA process, next it send its info (Main master do i3c_master_retrieve_dev_info()) and them receive DEFSLVS. In the stage it cannot request the bus mastership it need the MR enable. So, what I would suggest is to keep DEFSLVS data in secondary master driver memory . When secondary master receive the MR enable, it can get the bus ownership and do i3c_master_add_i3c_dev_locked() and each device in DEFSLVS command. After this step delegate the bus ownership to the main master. What do you think about this? Best regards, Vitor Soares
On Thu, 6 Sep 2018 16:17:58 +0100 vitor <Vitor.Soares@synopsys.com> wrote: > Hi Boris, > > > On 06-09-2018 15:14, Boris Brezillon wrote: > > On Thu, 6 Sep 2018 14:50:03 +0100 > > vitor <Vitor.Soares@synopsys.com> wrote: > > > >> Hi, > >> > >> > >> On 06-09-2018 14:20, Boris Brezillon wrote: > >>> On Thu, 6 Sep 2018 15:14:37 +0200 > >>> Boris Brezillon <boris.brezillon@bootlin.com> wrote: > >>> > >>>> On Thu, 6 Sep 2018 14:59:46 +0200 > >>>> Arnd Bergmann <arnd@arndb.de> wrote: > >>>> > >>>>> On Thu, Sep 6, 2018 at 2:43 PM Przemyslaw Gaj <pgaj@cadence.com> wrote: > >>>>>> Hi Boris, Vitor, > >>>>>> > >>>>>> This repository does not contain full kernel sources, but it should be enough to discuss mastership request feature. > >>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_przemekgaj_i3c-2Dlinux_commit_d54fe68a9d3e573c0c454a2c6f1afafc20142ec5&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=Q9DWw3KGmshGw0f5QTiffbpbESyUlPx6KmASuDBtX9Y&s=HHE_y1kyMszJvP_tSP9JkDlPYxDywBeHwkMGgCR11uI&e= > >>>>>> > >>>>>> Please keep in mind that this is initial version, but mastership works correctly. I added one property to DT to reflect relationship between masters. > >>>>>> It is possible to request mastership on demand (using sysfs. Useful in case when Linux machine is equipped only with secondary master controller) or automatically change operation mode when device driver wants to read/write something from/to device. > >>>>>> > >>>>>> I'm sure I will have to rework something because this was implemented on sources from PATCH v4. I saw that Boris released v7 yesterday :) > >>>>> Can you explain the reason for having a user space interface and DT property? > >>>>> I thought we had concluded earlier that we wouldn't need that, but it's possible > >>>>> that I missed something in the discussion since then. > >>>> I don't think the sysfs knob is needed, this being said, after thinking > >>>> a bit more about mastership handover and the secondary master case, I > >>>> think we have something important to solve. > >>>> > >>>> When a master is not in control of the bus, it gets informed of devices > >>>> present on the bus by monitoring DAA or DEFSLVS broadcast events. That > >>>> means the secondary master should populate the bus with I3C/I2C devices > >>>> on such events, but that's not enough, because DEFSLVS/DAA do not > >>>> provide all device info.Some of them (like read/write/ibi limitations) > >>>> require extra CCC commands, and, to send those CCC commands, the > >>>> secondary master must claim the bus. We could add a case where we > >>>> declare devices as partially discovered until the master acquires > >>>> ownership of the bus, but that means part of the data returned by > >>>> i3c_device_get_info() will be inaccurate, which might have an impact on > >>>> some i3c driver ->probe() functions. > >>> Hm, one possible solution would be to register partially discovered > >>> devices to the device model and let i3c_device_get_info() claim the bus > >>> and request missing data when needed. This way, if the driver needs to > >>> call i3c_device_get_info() in its probe path, it should work just fine. > >> Why don't use the i3c_master_add_i3c_dev_locked that job? It create, > >> attach and retrieve the device info. > > When will you call i3c_master_add_i3c_dev_locked()? After receiving a > > DEFSLVS interrupt/event? When that happens you're not in control of the > > bus, which means you'll have to force bus ownership handover. Is this > > really what we want? > > > > These are not rhetorical questions, I'm really asking for your opinion > > here. > What I understand from last discussion was that every time that device > need to do something (private messages) on the bus and don't have the > bus control it should force ownership handover. > If my understanding is correct this can be also applied to CCC commands. Sure, but the question is more, when do we want to do that? > > > > >> This can be triggered after the secondary master receive ENEC MR until > >> them is keep in the driver memory. > > But that means no-one will actually trigger a mastership request, > > because devices won't be registered until all info are available. If we > > take this path, we should have a way to explicitly trigger a mastership > > request (sysfs knob or any other means). > By the current flow that we have now, we enable the IBI events at the > end of .do_daa. At this time the main master bus is initialized with all > devices. > > From the point of view of secondary master, it first participate on DAA > process, next it send its info (Main master do > i3c_master_retrieve_dev_info()) and them receive DEFSLVS. In the stage > it cannot request the bus mastership it need the MR enable. You mean the broadcast ENEC(MR) event, right? Then yes, this one is probably missing (or maybe I added it in the master controller driver, I don't remember). > > So, what I would suggest is to keep DEFSLVS data in secondary master > driver memory . When secondary master receive the MR enable, it can get > the bus ownership and do i3c_master_add_i3c_dev_locked() and each device > in DEFSLVS command. After this step delegate the bus ownership to the > main master. That's an option, indeed. > > What do you think about this? Sounds like a good start. If MR is rejected by the master, we will just keep all devices in an unregistered state. We can also add a sysfs entry to manually re-trigger this operation in case the initial one failed.
On 9/6/18, 6:07 PM, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote:
EXTERNAL MAIL
On Thu, 6 Sep 2018 16:17:58 +0100
vitor <Vitor.Soares@synopsys.com> wrote:
> Hi Boris,
>
>
> On 06-09-2018 15:14, Boris Brezillon wrote:
> > On Thu, 6 Sep 2018 14:50:03 +0100
> > vitor <Vitor.Soares@synopsys.com> wrote:
> >
> >> Hi,
> >>
> >>
> >> On 06-09-2018 14:20, Boris Brezillon wrote:
> >>> On Thu, 6 Sep 2018 15:14:37 +0200
> >>> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> >>>
> >>>> On Thu, 6 Sep 2018 14:59:46 +0200
> >>>> Arnd Bergmann <arnd@arndb.de> wrote:
> >>>>
> >>>>> On Thu, Sep 6, 2018 at 2:43 PM Przemyslaw Gaj <pgaj@cadence.com> wrote:
> >>>>>> Hi Boris, Vitor,
> >>>>>>
> >>>>>> This repository does not contain full kernel sources, but it should be enough to discuss mastership request feature.
> >>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_przemekgaj_i3c-2Dlinux_commit_d54fe68a9d3e573c0c454a2c6f1afafc20142ec5&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=Q9DWw3KGmshGw0f5QTiffbpbESyUlPx6KmASuDBtX9Y&s=HHE_y1kyMszJvP_tSP9JkDlPYxDywBeHwkMGgCR11uI&e=
> >>>>>>
> >>>>>> Please keep in mind that this is initial version, but mastership works correctly. I added one property to DT to reflect relationship between masters.
> >>>>>> It is possible to request mastership on demand (using sysfs. Useful in case when Linux machine is equipped only with secondary master controller) or automatically change operation mode when device driver wants to read/write something from/to device.
> >>>>>>
> >>>>>> I'm sure I will have to rework something because this was implemented on sources from PATCH v4. I saw that Boris released v7 yesterday :)
> >>>>> Can you explain the reason for having a user space interface and DT property?
> >>>>> I thought we had concluded earlier that we wouldn't need that, but it's possible
> >>>>> that I missed something in the discussion since then.
> >>>> I don't think the sysfs knob is needed, this being said, after thinking
> >>>> a bit more about mastership handover and the secondary master case, I
> >>>> think we have something important to solve.
> >>>>
> >>>> When a master is not in control of the bus, it gets informed of devices
> >>>> present on the bus by monitoring DAA or DEFSLVS broadcast events. That
> >>>> means the secondary master should populate the bus with I3C/I2C devices
> >>>> on such events, but that's not enough, because DEFSLVS/DAA do not
> >>>> provide all device info.Some of them (like read/write/ibi limitations)
> >>>> require extra CCC commands, and, to send those CCC commands, the
> >>>> secondary master must claim the bus. We could add a case where we
> >>>> declare devices as partially discovered until the master acquires
> >>>> ownership of the bus, but that means part of the data returned by
> >>>> i3c_device_get_info() will be inaccurate, which might have an impact on
> >>>> some i3c driver ->probe() functions.
> >>> Hm, one possible solution would be to register partially discovered
> >>> devices to the device model and let i3c_device_get_info() claim the bus
> >>> and request missing data when needed. This way, if the driver needs to
> >>> call i3c_device_get_info() in its probe path, it should work just fine.
> >> Why don't use the i3c_master_add_i3c_dev_locked that job? It create,
> >> attach and retrieve the device info.
> > When will you call i3c_master_add_i3c_dev_locked()? After receiving a
> > DEFSLVS interrupt/event? When that happens you're not in control of the
> > bus, which means you'll have to force bus ownership handover. Is this
> > really what we want?
> >
> > These are not rhetorical questions, I'm really asking for your opinion
> > here.
> What I understand from last discussion was that every time that device
> need to do something (private messages) on the bus and don't have the
> bus control it should force ownership handover.
> If my understanding is correct this can be also applied to CCC commands.
Sure, but the question is more, when do we want to do that?
>
> >
> >> This can be triggered after the secondary master receive ENEC MR until
> >> them is keep in the driver memory.
> > But that means no-one will actually trigger a mastership request,
> > because devices won't be registered until all info are available. If we
> > take this path, we should have a way to explicitly trigger a mastership
> > request (sysfs knob or any other means).
> By the current flow that we have now, we enable the IBI events at the
> end of .do_daa. At this time the main master bus is initialized with all
> devices.
>
> From the point of view of secondary master, it first participate on DAA
> process, next it send its info (Main master do
> i3c_master_retrieve_dev_info()) and them receive DEFSLVS. In the stage
> it cannot request the bus mastership it need the MR enable.
You mean the broadcast ENEC(MR) event, right? Then yes, this one is
probably missing (or maybe I added it in the master controller driver,
I don't remember).
>
> So, what I would suggest is to keep DEFSLVS data in secondary master
> driver memory . When secondary master receive the MR enable, it can get
> the bus ownership and do i3c_master_add_i3c_dev_locked() and each device
> in DEFSLVS command. After this step delegate the bus ownership to the
> main master.
That's an option, indeed.
>
> What do you think about this?
Sounds like a good start. If MR is rejected by the master, we will just
keep all devices in an unregistered state. We can also add a sysfs
entry to manually re-trigger this operation in case the initial one
failed.
This everything sounds like my previous version. Enec event was missing,
everything was triggered manually from sysfs.
Hi Boris, Vitor,
On 9/6/18, 6:07 PM, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote:
EXTERNAL MAIL
On Thu, 6 Sep 2018 16:17:58 +0100
vitor <Vitor.Soares@synopsys.com> wrote:
> Hi Boris,
>
>
> On 06-09-2018 15:14, Boris Brezillon wrote:
> > On Thu, 6 Sep 2018 14:50:03 +0100
> > vitor <Vitor.Soares@synopsys.com> wrote:
> >
> >> Hi,
> >>
> >>
> >> On 06-09-2018 14:20, Boris Brezillon wrote:
> >>> On Thu, 6 Sep 2018 15:14:37 +0200
> >>> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> >>>
> >>>> On Thu, 6 Sep 2018 14:59:46 +0200
> >>>> Arnd Bergmann <arnd@arndb.de> wrote:
> >>>>
> >>>>> On Thu, Sep 6, 2018 at 2:43 PM Przemyslaw Gaj <pgaj@cadence.com> wrote:
> >>>>>> Hi Boris, Vitor,
> >>>>>>
> >>>>>> This repository does not contain full kernel sources, but it should be enough to discuss mastership request feature.
> >>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_przemekgaj_i3c-2Dlinux_commit_d54fe68a9d3e573c0c454a2c6f1afafc20142ec5&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=Q9DWw3KGmshGw0f5QTiffbpbESyUlPx6KmASuDBtX9Y&s=HHE_y1kyMszJvP_tSP9JkDlPYxDywBeHwkMGgCR11uI&e=
> >>>>>>
> >>>>>> Please keep in mind that this is initial version, but mastership works correctly. I added one property to DT to reflect relationship between masters.
> >>>>>> It is possible to request mastership on demand (using sysfs. Useful in case when Linux machine is equipped only with secondary master controller) or automatically change operation mode when device driver wants to read/write something from/to device.
> >>>>>>
> >>>>>> I'm sure I will have to rework something because this was implemented on sources from PATCH v4. I saw that Boris released v7 yesterday :)
> >>>>> Can you explain the reason for having a user space interface and DT property?
> >>>>> I thought we had concluded earlier that we wouldn't need that, but it's possible
> >>>>> that I missed something in the discussion since then.
> >>>> I don't think the sysfs knob is needed, this being said, after thinking
> >>>> a bit more about mastership handover and the secondary master case, I
> >>>> think we have something important to solve.
> >>>>
> >>>> When a master is not in control of the bus, it gets informed of devices
> >>>> present on the bus by monitoring DAA or DEFSLVS broadcast events. That
> >>>> means the secondary master should populate the bus with I3C/I2C devices
> >>>> on such events, but that's not enough, because DEFSLVS/DAA do not
> >>>> provide all device info.Some of them (like read/write/ibi limitations)
> >>>> require extra CCC commands, and, to send those CCC commands, the
> >>>> secondary master must claim the bus. We could add a case where we
> >>>> declare devices as partially discovered until the master acquires
> >>>> ownership of the bus, but that means part of the data returned by
> >>>> i3c_device_get_info() will be inaccurate, which might have an impact on
> >>>> some i3c driver ->probe() functions.
> >>> Hm, one possible solution would be to register partially discovered
> >>> devices to the device model and let i3c_device_get_info() claim the bus
> >>> and request missing data when needed. This way, if the driver needs to
> >>> call i3c_device_get_info() in its probe path, it should work just fine.
> >> Why don't use the i3c_master_add_i3c_dev_locked that job? It create,
> >> attach and retrieve the device info.
> > When will you call i3c_master_add_i3c_dev_locked()? After receiving a
> > DEFSLVS interrupt/event? When that happens you're not in control of the
> > bus, which means you'll have to force bus ownership handover. Is this
> > really what we want?
> >
> > These are not rhetorical questions, I'm really asking for your opinion
> > here.
> What I understand from last discussion was that every time that device
> need to do something (private messages) on the bus and don't have the
> bus control it should force ownership handover.
> If my understanding is correct this can be also applied to CCC commands.
Sure, but the question is more, when do we want to do that?
>
> >
> >> This can be triggered after the secondary master receive ENEC MR until
> >> them is keep in the driver memory.
> > But that means no-one will actually trigger a mastership request,
> > because devices won't be registered until all info are available. If we
> > take this path, we should have a way to explicitly trigger a mastership
> > request (sysfs knob or any other means).
> By the current flow that we have now, we enable the IBI events at the
> end of .do_daa. At this time the main master bus is initialized with all
> devices.
>
> From the point of view of secondary master, it first participate on DAA
> process, next it send its info (Main master do
> i3c_master_retrieve_dev_info()) and them receive DEFSLVS. In the stage
> it cannot request the bus mastership it need the MR enable.
You mean the broadcast ENEC(MR) event, right? Then yes, this one is
probably missing (or maybe I added it in the master controller driver,
I don't remember).
>
> So, what I would suggest is to keep DEFSLVS data in secondary master
> driver memory . When secondary master receive the MR enable, it can get
> the bus ownership and do i3c_master_add_i3c_dev_locked() and each device
> in DEFSLVS command. After this step delegate the bus ownership to the
> main master.
That's an option, indeed.
>
> What do you think about this?
Sounds like a good start. If MR is rejected by the master, we will just
keep all devices in an unregistered state. We can also add a sysfs
entry to manually re-trigger this operation in case the initial one
failed.
I found that version. I added also mastership request after ENEC(MR) event to run
tests and verify if everything works correctly. It looked quite simple. Secondary master
does not give control back automatically after bus initialization and master doesn't
yet request mastership when device driver wants to use the bus / transfer data.
This can be the base for further works.
https://github.com/przemekgaj/i3c-linux/tree/mastership_takeover
Regards,
Przemek
Hi, On 06-09-2018 17:17, Przemyslaw Gaj wrote: > > On 9/6/18, 6:07 PM, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote: > > EXTERNAL MAIL > > > On Thu, 6 Sep 2018 16:17:58 +0100 > vitor <Vitor.Soares@synopsys.com> wrote: > > > Hi Boris, > > > > > > On 06-09-2018 15:14, Boris Brezillon wrote: > > > On Thu, 6 Sep 2018 14:50:03 +0100 > > > vitor <Vitor.Soares@synopsys.com> wrote: > > > > > >> Hi, > > >> > > >> > > >> On 06-09-2018 14:20, Boris Brezillon wrote: > > >>> On Thu, 6 Sep 2018 15:14:37 +0200 > > >>> Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > >>> > > >>>> On Thu, 6 Sep 2018 14:59:46 +0200 > > >>>> Arnd Bergmann <arnd@arndb.de> wrote: > > >>>> > > >>>>> On Thu, Sep 6, 2018 at 2:43 PM Przemyslaw Gaj <pgaj@cadence.com> wrote: > > >>>>>> Hi Boris, Vitor, > > >>>>>> > > >>>>>> This repository does not contain full kernel sources, but it should be enough to discuss mastership request feature. > > >>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_przemekgaj_i3c-2Dlinux_commit_d54fe68a9d3e573c0c454a2c6f1afafc20142ec5&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=Q9DWw3KGmshGw0f5QTiffbpbESyUlPx6KmASuDBtX9Y&s=HHE_y1kyMszJvP_tSP9JkDlPYxDywBeHwkMGgCR11uI&e= > > >>>>>> > > >>>>>> Please keep in mind that this is initial version, but mastership works correctly. I added one property to DT to reflect relationship between masters. > > >>>>>> It is possible to request mastership on demand (using sysfs. Useful in case when Linux machine is equipped only with secondary master controller) or automatically change operation mode when device driver wants to read/write something from/to device. > > >>>>>> > > >>>>>> I'm sure I will have to rework something because this was implemented on sources from PATCH v4. I saw that Boris released v7 yesterday :) > > >>>>> Can you explain the reason for having a user space interface and DT property? > > >>>>> I thought we had concluded earlier that we wouldn't need that, but it's possible > > >>>>> that I missed something in the discussion since then. > > >>>> I don't think the sysfs knob is needed, this being said, after thinking > > >>>> a bit more about mastership handover and the secondary master case, I > > >>>> think we have something important to solve. > > >>>> > > >>>> When a master is not in control of the bus, it gets informed of devices > > >>>> present on the bus by monitoring DAA or DEFSLVS broadcast events. That > > >>>> means the secondary master should populate the bus with I3C/I2C devices > > >>>> on such events, but that's not enough, because DEFSLVS/DAA do not > > >>>> provide all device info.Some of them (like read/write/ibi limitations) > > >>>> require extra CCC commands, and, to send those CCC commands, the > > >>>> secondary master must claim the bus. We could add a case where we > > >>>> declare devices as partially discovered until the master acquires > > >>>> ownership of the bus, but that means part of the data returned by > > >>>> i3c_device_get_info() will be inaccurate, which might have an impact on > > >>>> some i3c driver ->probe() functions. > > >>> Hm, one possible solution would be to register partially discovered > > >>> devices to the device model and let i3c_device_get_info() claim the bus > > >>> and request missing data when needed. This way, if the driver needs to > > >>> call i3c_device_get_info() in its probe path, it should work just fine. > > >> Why don't use the i3c_master_add_i3c_dev_locked that job? It create, > > >> attach and retrieve the device info. > > > When will you call i3c_master_add_i3c_dev_locked()? After receiving a > > > DEFSLVS interrupt/event? When that happens you're not in control of the > > > bus, which means you'll have to force bus ownership handover. Is this > > > really what we want? > > > > > > These are not rhetorical questions, I'm really asking for your opinion > > > here. > > What I understand from last discussion was that every time that device > > need to do something (private messages) on the bus and don't have the > > bus control it should force ownership handover. > > If my understanding is correct this can be also applied to CCC commands. > > Sure, but the question is more, when do we want to do that? > > > > > > > > >> This can be triggered after the secondary master receive ENEC MR until > > >> them is keep in the driver memory. > > > But that means no-one will actually trigger a mastership request, > > > because devices won't be registered until all info are available. If we > > > take this path, we should have a way to explicitly trigger a mastership > > > request (sysfs knob or any other means). > > By the current flow that we have now, we enable the IBI events at the > > end of .do_daa. At this time the main master bus is initialized with all > > devices. > > > > From the point of view of secondary master, it first participate on DAA > > process, next it send its info (Main master do > > i3c_master_retrieve_dev_info()) and them receive DEFSLVS. In the stage > > it cannot request the bus mastership it need the MR enable. > > You mean the broadcast ENEC(MR) event, right? Then yes, this one is > probably missing (or maybe I added it in the master controller driver, > I don't remember). > > > > > So, what I would suggest is to keep DEFSLVS data in secondary master > > driver memory . When secondary master receive the MR enable, it can get > > the bus ownership and do i3c_master_add_i3c_dev_locked() and each device > > in DEFSLVS command. After this step delegate the bus ownership to the > > main master. > > That's an option, indeed. > > > > > What do you think about this? > > Sounds like a good start. If MR is rejected by the master, we will just > keep all devices in an unregistered state. We can also add a sysfs > entry to manually re-trigger this operation in case the initial one > failed. > > This everything sounds like my previous version. Enec event was missing, > everything was triggered manually from sysfs. > Does make sense to do the ->bus_init() only after receive the first ENEC-MR event? The bus initialization assume that we already know the I2C devices present on the line (bus->mode) and i3c_master_set_info only have meaning if the device will act as a master otherwise it can act as "slave". Best regards, Vitor Soares
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. Discussion around the bus/master/dev representation is still ongoing, with Arnd opting for a simple approach where * the bus is implicitly represented by the master device * the master is not represented as a device under the I3C bus * only remote I3C devices are exposed and possibly duplicated if several masters controlling the same bus are exposed to the same Linux instance and Peter preferring the representation where the bus is a separate object. IIRC, Wolfram was in favor of the "bus is a separate object" too. If possible, I'd like to close this discussion soon, no matter which solution is chosen. - 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 Main changes between v5 and v6: - Introduce {i3c,i2c}_dev_desc structures to better match how I3C master controllers (reservation of one HW slot for each device attached to the bus). With this solution, the resource migration that happens when a device lose its dynamic address and is re-assigned a different address is simplified on the driver side, because most of it is now handled in the core (reserve a new dev slot, reserve IBI resources and free all resources attached to the old slot) - Add I3C error codes (M0 to M2) so that the core and device drivers can have fine grained information on what caused an EIO error. Only minor things happened between v3 and v5 (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 | 606 ++++++ drivers/i3c/device.c | 233 +++ drivers/i3c/internals.h | 36 + drivers/i3c/master.c | 2058 ++++++++++++++++++++ drivers/i3c/master/Kconfig | 5 + drivers/i3c/master/Makefile | 1 + drivers/i3c/master/i3c-master-cdns.c | 1668 ++++++++++++++++ include/dt-bindings/i3c/i3c.h | 28 + include/linux/i3c/ccc.h | 385 ++++ include/linux/i3c/device.h | 331 ++++ include/linux/i3c/master.h | 652 +++++++ include/linux/mod_devicetable.h | 17 + 29 files changed, 7036 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