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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Boris,

On 26/10/18 15:43, Boris Brezillon 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].

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://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_i3c_linux.git_log_-3Fh-3Di3c_next&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=TkhdwjEerP4IBiC_WzI_vEUyZD2Dcxl03UeycbwNI2Q&s=KK31_5lEEgNN2iyURFULH0HmjkOPWoarNiT1hehvIsY&e=

*** 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

Can you update the i3c/next tree?


Best regards,

Vitor Soares





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux