Re: [PATCH v10 0/9] Add the I3C subsystem

[Boris Brezillon <boris.brezillon@xxxxxxxxxxx>]

On Fri, 26 Oct 2018 16:43:24 +0200
Boris Brezillon <boris.brezillon@xxxxxxxxxxx> wrote:

> Hi Greg,
> 
> I think we've reached a point where we can eventually consider the I3C
> framework for inclusion in 4.20 (5.0?). A few more issues were reported
> on v9 and fixed in v10. I can't guarantee that the implementation is
> free of bugs but I still think it's worth merging it in v4.20: it's a
> new subsystem, so we don't risk regressions, and the only way we can
> detect other issues is by having other people experiment with this
> implementation.
> 
> The only remaining concern raised by Arnd is the fact that both hosts
> and slaves share the same bus type and are differentiated thanks to
> their device_type, which IMHO is fine since this is what other
> subsystems do (plus I don't see other solutions to have both I3C
> devices and I3C buses represented under /sys/bus/i3c/).
> 
> I'd really like to get this series merged in 4.20 so that other
> contributors can work on top of it to add
> 
> 1/ new host drivers
> 2/ support for advanced features
> 3/ new device drivers
> 
> So, unless you have strong reasons to reject this request, and,
> assuming I get Rob's ack soon enough, I plan to send a PR for this
> framework next week.
> 
> Here is the usual description copy&pasted from the previous versions:
> 
> This patch series is adding a new subsystem to support I3C devices.
> 
> This is just adding support for basic features. Extra 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].
> 

Forgot to add:

Main changes between v9 and v10:
- Fix the example in the DT bindings file
- Dynamically allocate CCC payloads to make them DMA-safe
- Fix a deadlock
- Mention that i2c bufs are not guaranteed to be DMA-safe

> Main change between v8 and v9:
> - The DT bindings have been simplified to get rid of the I3C/I3C_DEV()
>   macros
> 
> Main change between v7 and v8:
> - The bus object is now embedded in the master_controller object (as
>   suggested by Arnd)
> 
> 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
> 
> *** BLURB HERE ***
> 
> Boris Brezillon (9):
>   i3c: Add core I3C infrastructure
>   docs: driver-api: Add I3C documentation
>   i3c: Add sysfs ABI spec
>   dt-bindings: i3c: Document core bindings
>   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 +
>  .../bindings/gpio/gpio-cdns-i3c.txt           |   39 +
>  .../bindings/i3c/cdns,i3c-master.txt          |   44 +
>  Documentation/devicetree/bindings/i3c/i3c.txt |  139 +
>  .../driver-api/i3c/device-driver-api.rst      |    9 +
>  Documentation/driver-api/i3c/index.rst        |   11 +
>  .../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/device.c                          |  233 ++
>  drivers/i3c/internals.h                       |   26 +
>  drivers/i3c/master.c                          | 2661 +++++++++++++++++
>  drivers/i3c/master/Kconfig                    |    6 +
>  drivers/i3c/master/Makefile                   |    1 +
>  drivers/i3c/master/i3c-master-cdns.c          | 1671 +++++++++++
>  include/linux/i3c/ccc.h                       |  385 +++
>  include/linux/i3c/device.h                    |  331 ++
>  include/linux/i3c/master.h                    |  648 ++++
>  include/linux/mod_devicetable.h               |   17 +
>  27 files changed, 7047 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/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/linux/i3c/ccc.h
>  create mode 100644 include/linux/i3c/device.h
>  create mode 100644 include/linux/i3c/master.h
>