Re: [PATCH v4 00/10] Add the I3C subsystem

From: Boris Brezillon
Date: Mon Apr 23 2018 - 13:38:27 EST


Hi,

On Fri, 30 Mar 2018 09:47:41 +0200
Boris Brezillon <boris.brezillon@xxxxxxxxxxx> wrote:

> This patch series is a proposal for a new I3C subsystem.

This v4 has been sent almost a month ago and I didn't get any feedback
so far apart from Rob's R-b. Greg, is there any chance we can get these
patches merged in 4.18? If not, could you tell me what should be
addressed/improved/reworked?

Thanks,

Boris

>
> This infrastructure is not complete yet and will be extended over
> time.
>
> There are a few design choices that are worth mentioning because they
> impact the way I3C device drivers can interact with their devices:
>
> - all functions used to send I3C/I2C frames must be called in
> non-atomic context. Mainly done this way to ease implementation, but
> this is still open to discussion. Please let me know if you think it's
> worth considering an asynchronous model here
> - the bus element is a separate object and is not implicitly described
> by the master (as done in I2C). The reason is that I want to be able
> to handle multiple master connected to the same bus and visible to
> Linux.
> In this situation, we should only have one instance of the device and
> not one per master, and sharing the bus object would be part of the
> solution to gracefully handle this case.
> I'm not sure if we will ever need to deal with multiple masters
> controlling the same bus and exposed under Linux, but separating the
> bus and master concept is pretty easy, hence the decision to do it
> now, just in case we need it some day.
> The other benefit of separating the bus and master concepts is that
> master devices appear under the bus directory in sysfs.
> - I2C backward compatibility has been designed to be transparent to I2C
> drivers and the I2C subsystem. The I3C master just registers an I2C
> adapter which creates a new I2C bus. I'd say that, from a
> representation PoV it's not ideal because what should appear as a
> single I3C bus exposing I3C and I2C devices here appears as 2
> different busses connected to each other through the parenting (the
> I3C master is the parent of the I2C and I3C busses).
> On the other hand, I don't see a better solution if we want something
> that is not invasive.
>
> Missing features in this preliminary version:
> - support for HDR modes (has been removed because of lack of real users)
> - no support for multi-master and the associated concepts (mastership
> handover, support for secondary masters, ...)
> - I2C devices can only be described using DT because this is the only
> use case I have. However, the framework can easily be extended with
> ACPI and board info support
> - I3C slave framework. This has been completely omitted, but shouldn't
> have a huge impact on the I3C framework because I3C slaves don't see
> the whole bus, it's only about handling master requests and generating
> IBIs. Some of the struct, constant and enum definitions could be
> shared, but most of the I3C slave framework logic will be different
>
> 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
>
> Note that I2C changes have been sent separately [1] this time. Other
> than that, no big changes in this version, I just addressed the
> comments I received from Thomas, Peter, Geert and Rob.
>
> Thanks,
>
> Boris
>
> [1]https://patchwork.ozlabs.org/project/linux-i2c/list/?series=35687
>
> 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/conf.py | 10 +
> Documentation/driver-api/i3c/device-driver-api.rst | 7 +
> Documentation/driver-api/i3c/index.rst | 9 +
> Documentation/driver-api/i3c/master-driver-api.rst | 8 +
> Documentation/driver-api/i3c/protocol.rst | 201 +++
> Documentation/driver-api/index.rst | 1 +
> MAINTAINERS | 9 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 2 +-
> drivers/gpio/Kconfig | 11 +
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-cdns-i3c.c | 380 +++++
> drivers/i3c/Kconfig | 24 +
> drivers/i3c/Makefile | 4 +
> drivers/i3c/core.c | 620 +++++++
> drivers/i3c/device.c | 294 ++++
> drivers/i3c/internals.h | 28 +
> drivers/i3c/master.c | 1723 ++++++++++++++++++++
> drivers/i3c/master/Kconfig | 5 +
> drivers/i3c/master/Makefile | 1 +
> drivers/i3c/master/i3c-master-cdns.c | 1650 +++++++++++++++++++
> include/dt-bindings/i3c/i3c.h | 28 +
> include/linux/i3c/ccc.h | 382 +++++
> include/linux/i3c/device.h | 305 ++++
> include/linux/i3c/master.h | 587 +++++++
> include/linux/mod_devicetable.h | 17 +
> 30 files changed, 6626 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/conf.py
> 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
>