| Message ID | 20180905154108.20770-1-boris.brezillon@bootlin.com |
|---|---|
| Headers | show |
| Series | Add the I3C subsystem | expand |
On Wed, Sep 5, 2018 at 5:41 PM Boris Brezillon <boris.brezillon@bootlin.com> wrote: > --- > Changes in v7: > - Stop representing the I3C master as a device under the I3C bus > - Enforce a 1:1 relationship between i3c_bus and i3c_master_controller > objects Thanks for implementing those changes. What is your feeling so far about the difference? Has it gotten much simpler as I was hoping? I definitely like this version much better. I have found a couple of things that I point out below that could be improved (or me being proven wrong on them), but overall I think it looks great and I don't see major issues. Great work! > +struct i3c_bus *i3c_bus_create(struct i3c_master_controller *master) > +{ > + struct i3c_bus *i3cbus; > + int ret; > + > + i3cbus = kzalloc(sizeof(*i3cbus), GFP_KERNEL); > + if (!i3cbus) > + return ERR_PTR(-ENOMEM); I find it a bit confusing to have separate i3c_master_controller and i3c_bus structures with this version. Why not merge the two structures into one now and move the bus management into master.c? > +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. > + Apparently the new gmail version decided to cut off the second half of your email after this line when I hit reply, which makes it much harder for me to continue a proper review. I fear I'll have to get a real email client again :( > + * The I3C bus is represented with its own object and not implicitly described > + * by the I3C master to cope with the multi-master functionality, where one bus > + * can be shared amongst several masters, each of them requesting bus ownership > + * when they need to. This comment is now stale, even without merging the structures, right? > +struct i3c_master_controller { > + struct device *parent; > + struct i3c_dev_desc *this; > + struct i2c_adapter i2c; I think the 'parent' pointer is better omitted, it should always be the same as master->dev->parent, right? Since it contains an i2c_adapter, maybe a good name for the combined i3c_master_controller+i3c_bus structure would be 'i3c_adapter'? +#define i3c_bus_for_each_i2cdev(bus, dev) \ + list_for_each_entry(dev, &(bus)->devs.i2c, common.node) + +#define i3c_bus_for_each_i3cdev(bus, dev) \ + list_for_each_entry(dev, &(bus)->devs.i3c, common.node) I wonder if it would simplify things to drop the i2c and i3c device lists and instead implement these for_each loops based on device_for_each_child() with a check of the bus_type==&i2c_bus/&i3c_bus. That might help with locking and keeping the two lists synchronized, which may or may not be a problem here. Arnd
On Thu, 6 Sep 2018 15:38:49 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > On Wed, Sep 5, 2018 at 5:41 PM Boris Brezillon > <boris.brezillon@bootlin.com> wrote: > > > --- > > Changes in v7: > > - Stop representing the I3C master as a device under the I3C bus > > - Enforce a 1:1 relationship between i3c_bus and i3c_master_controller > > objects > > Thanks for implementing those changes. What is your feeling so far > about the difference? Has it gotten much simpler as I was hoping? Hm, to be honest I don't see a big difference in term of simplicity, but that's probably because I kept the bus/master concepts separated (even if the 1:1 relationship is enforced). > > I definitely like this version much better. I have found a couple of > things that I point out below that could be improved (or me being > proven wrong on them), but overall I think it looks great and I don't > see major issues. > > Great work! Thanks, I'm glad you think these changes go in the right direction. Will try to address your new comments. > > > +struct i3c_bus *i3c_bus_create(struct i3c_master_controller *master) > > +{ > > + struct i3c_bus *i3cbus; > > + int ret; > > + > > + i3cbus = kzalloc(sizeof(*i3cbus), GFP_KERNEL); > > + if (!i3cbus) > > + return ERR_PTR(-ENOMEM); > > I find it a bit confusing to have separate i3c_master_controller > and i3c_bus structures with this version. Why not merge the > two structures into one now and move the bus management > into master.c? Yes, I considered this approach. Not sure it would make the whole code a lot simpler though, and it was definitely more work on my side, so I decided to delay that and wait for someone to explicitly request this change ;-). I also like the bus/master separation in that it allows one to clearly identify the properties that are bus (speed, mode, ...) related and those that are master controller oriented (PID, DCR, ...). > > > +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. > > + > > Apparently the new gmail version decided to cut off the second half of your > email after this line when I hit reply, which makes it much harder for me > to continue a proper review. I fear I'll have to get a real email client > again :( > > > + * The I3C bus is represented with its own object and not implicitly described > > + * by the I3C master to cope with the multi-master functionality, where one bus > > + * can be shared amongst several masters, each of them requesting bus ownership > > + * when they need to. > > This comment is now stale, even without merging the structures, right? Yep, it is. I'll drop it. > > > +struct i3c_master_controller { > > + struct device *parent; > > + struct i3c_dev_desc *this; > > + struct i2c_adapter i2c; > > I think the 'parent' pointer is better omitted, it should always be > the same as master->dev->parent, right? Absolutely. > > Since it contains an i2c_adapter, maybe a good name for the > combined i3c_master_controller+i3c_bus structure would > be 'i3c_adapter'? Hm, I'd really like to keep the name master and not adapter, just because that's how it's named in the spec. > > +#define i3c_bus_for_each_i2cdev(bus, dev) \ > + list_for_each_entry(dev, &(bus)->devs.i2c, common.node) > + > +#define i3c_bus_for_each_i3cdev(bus, dev) \ > + list_for_each_entry(dev, &(bus)->devs.i3c, common.node) > > I wonder if it would simplify things to drop the i2c and i3c > device lists and instead implement these for_each loops > based on device_for_each_child() with a check of the > bus_type==&i2c_bus/&i3c_bus. We don't store i3c/i3c_device objects in those lists, but i3c/i2c_dev_desc objects, so device_for_each_child() won't work. Remember that i3c devs can be present but not registered yet, and we need to have those representations somehow attached to the master/bus. Regarding the possibility of merging those 2 lists together, we could do it, but I find it simpler to have them separated. > That might help with locking > and keeping the two lists synchronized, which may or > may not be a problem here. Regarding the locking aspects. Anything touching those lists is either init code (which should be serialized) or DAA related, which requires the bus lock to be held in write mode (exclusive access mode). Do you see any other possible race? Thanks, Boris
On Thu, Sep 6, 2018 at 4:01 PM Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Thu, 6 Sep 2018 15:38:49 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > > On Wed, Sep 5, 2018 at 5:41 PM Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > > > > +struct i3c_bus *i3c_bus_create(struct i3c_master_controller *master) > > > +{ > > > + struct i3c_bus *i3cbus; > > > + int ret; > > > + > > > + i3cbus = kzalloc(sizeof(*i3cbus), GFP_KERNEL); > > > + if (!i3cbus) > > > + return ERR_PTR(-ENOMEM); > > > > I find it a bit confusing to have separate i3c_master_controller > > and i3c_bus structures with this version. Why not merge the > > two structures into one now and move the bus management > > into master.c? > > Yes, I considered this approach. Not sure it would make the whole code a > lot simpler though, and it was definitely more work on my side, so I > decided to delay that and wait for someone to explicitly request this > change ;-). > > I also like the bus/master separation in that it allows one to clearly > identify the properties that are bus (speed, mode, ...) related and > those that are master controller oriented (PID, DCR, ...). Maybe you could make the bus a member of the master rather than a pointer from it? That would still let us avoid the separate dynamic allocations, and not having to worry about separate lifetime rules for the two, but also group the bus properties together a bit more as you said. It can also have code deal with the the bus object without including the structure definition of the master (if that is desirable). > > Since it contains an i2c_adapter, maybe a good name for the > > combined i3c_master_controller+i3c_bus structure would > > be 'i3c_adapter'? > > Hm, I'd really like to keep the name master and not adapter, just > because that's how it's named in the spec. Fair enough. > We don't store i3c/i3c_device objects in those lists, but > i3c/i2c_dev_desc objects, so device_for_each_child() won't work. > Remember that i3c devs can be present but not registered yet, and we > need to have those representations somehow attached to the master/bus. Ah right, I remember you explained that before. > Regarding the possibility of merging those 2 lists together, we could > do it, but I find it simpler to have them separated. Agreed. It would only make sense if we had the combined list already. > > That might help with locking > > and keeping the two lists synchronized, which may or > > may not be a problem here. > > Regarding the locking aspects. Anything touching those lists is either > init code (which should be serialized) or DAA related, which requires > the bus lock to be held in write mode (exclusive access mode). Do you > see any other possible race? No, nothing specific, it was just a general feeling. Arnd
Hi Boris, On 05-09-2018 16:40, Boris Brezillon wrote: > +i3c_master_register_new_i3c_devs(struct i3c_master_controller *master) > +{ > + struct i3c_dev_desc *desc; > + int ret; > + > + if (!master->init_done) > + return; > + If you have a hot-join and call i3c_master_do_daa this function will return without create the i3c_device. > + i3c_bus_for_each_i3cdev(master->bus, desc) { > + if (desc->dev || !desc->info.dyn_addr || desc == master->this) > + 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; > + 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; > + > + ret = device_register(&desc->dev->dev); > + if (ret) > + dev_err(master->parent, > + "Failed to add I3C device (err = %d)\n", ret); > + } > +} > Best regards, Vitor Soares
Hi Vitor, Sorry for the late reply, I was not at the office last week. On Tue, 11 Sep 2018 11:04:07 +0100 vitor <Vitor.Soares@synopsys.com> wrote: > Hi Boris, > > On 05-09-2018 16:40, Boris Brezillon wrote: > > +i3c_master_register_new_i3c_devs(struct i3c_master_controller *master) > > +{ > > + struct i3c_dev_desc *desc; > > + int ret; > > + > > + if (!master->init_done) > > + return; > > + > If you have a hot-join and call i3c_master_do_daa this function will > return without create the i3c_device. Hm, you mean if hot-join happens when the I3C master is not yet registered? That shouldn't be a problem since all devices will be registered at the end of i3c_master_register(). Am I missing something? Is this a problem you actually face or just something you found out doing code inspection? > > > + i3c_bus_for_each_i3cdev(master->bus, desc) { > > + if (desc->dev || !desc->info.dyn_addr || desc == master->this) > > + 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; > > + 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; > > + > > + ret = device_register(&desc->dev->dev); > > + if (ret) > > + dev_err(master->parent, > > + "Failed to add I3C device (err = %d)\n", ret); > > + } > > +} Regards, Boris
This patch series is adding a new subsystem to support I3C devices. This is just adding basic features of the protocol, and new features will be added afterwards. 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 I3C bus and I3C master controller are now tightly coupled even though they're still allocated separately. There's now a 1:1 relationship between these objects, and the I3C master is no longer represented under the I3C bus object. Arnd, let me know if you had something different in mind, and I'll rework the implementation accordingly. - 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 (will be added in separate patch series after initial support has been accepted/merged): - 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 Note that this patchset is available on the linux-i3c repo[1]. Main changes between v6 and v7: - I3C bus/master representations have been reworked to match what other subsystems are doing (master implicitly represented by the bus object) - I3C dev registration has been fixed - I3C bus mode selection has been fixed - Calls to readsl/writesl() in the Cadence I3C master driver have been fixed 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 [1]https://git.kernel.org/pub/scm/linux/kernel/git/i3c/linux.git/log/?h=i3c/next 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 | 146 ++ .../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 | 12 + 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 | 623 ++++++ drivers/i3c/device.c | 233 +++ drivers/i3c/internals.h | 36 + drivers/i3c/master.c | 2074 ++++++++++++++++++++ drivers/i3c/master/Kconfig | 6 + 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 | 653 ++++++ include/linux/mod_devicetable.h | 17 + 29 files changed, 7124 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